[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1334485212
##
airflow/www/views.py:
##
@@ -1313,23 +1290,12 @@ def last_dagruns(self, session: Session = NEW_SESSION):
return flask.json.jsonify(resp)
@expose("/code")
-@auth.has_access(
-[
-(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
-(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_CODE),
-]
-)
def legacy_code(self):
"""Redirect from url param."""
return redirect(url_for("Airflow.code", **sanitize_args(request.args)))
@expose("/dags//code")
-@auth.has_access(
-[
-(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
-(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_CODE),
-]
-)
+@auth.has_access_dag("GET", DagAccessEntity.CODE)
Review Comment:
See comment
[here](https://github.com/apache/airflow/pull/33213#discussion_r1318202392).
Basically we decided to go with `Literal` instead of `enum`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1323120010 ## airflow/auth/managers/fab/fab_auth_manager.py: ## @@ -87,35 +132,216 @@ def is_logged_in(self) -> bool: """Return whether the user is logged in.""" return not self.get_user().is_anonymous +def is_authorized_configuration(self, *, method: ResourceMethod, user: BaseUser | None = None) -> bool: +return self._is_authorized(method=method, resource_type=RESOURCE_CONFIG, user=user) + +def is_authorized_cluster_activity(self, *, method: ResourceMethod, user: BaseUser | None = None) -> bool: +return self._is_authorized(method=method, resource_type=RESOURCE_CLUSTER_ACTIVITY, user=user) + +def is_authorized_connection( +self, +*, +method: ResourceMethod, +connection_details: ConnectionDetails | None = None, +user: BaseUser | None = None, +) -> bool: +return self._is_authorized(method=method, resource_type=RESOURCE_CONNECTION, user=user) + +def is_authorized_dag( +self, +*, +method: ResourceMethod, +dag_access_entity: DagAccessEntity | None = None, +dag_details: DagDetails | None = None, +user: BaseUser | None = None, +) -> bool: +""" +Return whether the user is authorized to access the dag. + +There are multiple scenarios: + +1. ``dag_access`` is not provided which means the user wants to access the DAG itself and not a sub +entity (e.g. DAG runs). +2. ``dag_access`` is provided which means the user wants to access a sub entity of the DAG +(e.g. DAG runs). +a. If ``method`` is GET, then check the user has READ permissions on the DAG and the sub entity +b. Else, check the user has EDIT permissions on the DAG and ``method`` on the sub entity + +:param method: The method to authorize. +:param dag_access_entity: The dag access entity. +:param dag_details: The dag details. +:param user: The user. +""" +if not dag_access_entity: +# Scenario 1 +return self._is_authorized_dag(method=method, dag_details=dag_details, user=user) +else: +# Scenario 2 +resource_type = self._get_fab_resource_type(dag_access_entity) +dag_method: ResourceMethod = cast(ResourceMethod, "GET" if method == "GET" else "PUT") Review Comment: Interesting, it was not `mypy` which complained but my IDE, `mypy` is happy about that. I removed it then :) Thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1319941307 ## airflow/auth/managers/fab/fab_auth_manager.py: ## @@ -87,35 +132,216 @@ def is_logged_in(self) -> bool: """Return whether the user is logged in.""" return not self.get_user().is_anonymous +def is_authorized_configuration(self, *, method: ResourceMethod, user: BaseUser | None = None) -> bool: +return self._is_authorized(method=method, resource_type=RESOURCE_CONFIG, user=user) + +def is_authorized_cluster_activity(self, *, method: ResourceMethod, user: BaseUser | None = None) -> bool: +return self._is_authorized(method=method, resource_type=RESOURCE_CLUSTER_ACTIVITY, user=user) + +def is_authorized_connection( +self, +*, +method: ResourceMethod, +connection_details: ConnectionDetails | None = None, +user: BaseUser | None = None, +) -> bool: +return self._is_authorized(method=method, resource_type=RESOURCE_CONNECTION, user=user) + +def is_authorized_dag( +self, +*, +method: ResourceMethod, +dag_access_entity: DagAccessEntity | None = None, +dag_details: DagDetails | None = None, +user: BaseUser | None = None, +) -> bool: +""" +Return whether the user is authorized to access the dag. + +There are multiple scenarios: + +1. ``dag_access`` is not provided which means the user wants to access the DAG itself and not a sub +entity (e.g. DAG runs). +2. ``dag_access`` is provided which means the user wants to access a sub entity of the DAG +(e.g. DAG runs). +a. If ``method`` is GET, then check the user has READ permissions on the DAG and the sub entity +b. Else, check the user has EDIT permissions on the DAG and ``method`` on the sub entity + +:param method: The method to authorize. +:param dag_access_entity: The dag access entity. +:param dag_details: The dag details. +:param user: The user. +""" +if not dag_access_entity: +# Scenario 1 +return self._is_authorized_dag(method=method, dag_details=dag_details, user=user) +else: +# Scenario 2 +resource_type = self._get_fab_resource_type(dag_access_entity) +dag_method: ResourceMethod = cast(ResourceMethod, "GET" if method == "GET" else "PUT") Review Comment: I wish but the reason I did it is because Mypy complained :/ I dont like it either -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1318645914 ## airflow/auth/managers/models/resource_method.py: ## @@ -0,0 +1,38 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from enum import Enum + + +class ResourceMethod(Enum): Review Comment: You are correct, we do not expect to receive foreign values, thus we can use `Literal` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1318619327
##
airflow/auth/managers/fab/fab_auth_manager.py:
##
@@ -87,35 +131,231 @@ def is_logged_in(self) -> bool:
"""Return whether the user is logged in."""
return not self.get_user().is_anonymous
+def is_authorized_configuration(self, *, method: ResourceMethod, user:
BaseUser | None = None) -> bool:
+return self._is_authorized(method=method,
resource_type=RESOURCE_CONFIG, user=user)
+
+def is_authorized_cluster_activity(self, *, method: ResourceMethod, user:
BaseUser | None = None) -> bool:
+return self._is_authorized(method=method,
resource_type=RESOURCE_CLUSTER_ACTIVITY, user=user)
+
+def is_authorized_connection(
+self,
+*,
+method: ResourceMethod,
+connection_details: ConnectionDetails | None = None,
+user: BaseUser | None = None,
+) -> bool:
+return self._is_authorized(method=method,
resource_type=RESOURCE_CONNECTION, user=user)
+
+def is_authorized_dag(
+self,
+*,
+method: ResourceMethod,
+dag_access_entity: DagAccessEntity | None = None,
+dag_details: DagDetails | None = None,
+user: BaseUser | None = None,
+) -> bool:
+"""
+Return whether the user is authorized to access the dag.
+
+There are multiple scenarios:
+
+1. ``dag_access`` is not provided which means the user wants to access
the DAG itself and not a sub
+entity (e.g. DAG runs).
+2. ``dag_access`` is provided which means the user wants to access a
sub entity of the DAG
+(e.g. DAG runs).
+a. If ``method`` is GET, then check the user has READ permissions
on the DAG and the sub entity
+b. Else, check the user has EDIT permissions on the DAG and
``method`` on the sub entity
+
+:param method: The method to authorize.
+:param dag_access_entity: The dag access entity.
+:param dag_details: The dag details.
+:param user: The user.
+"""
+if not dag_access_entity:
+# Scenario 1
+return self._is_authorized_dag(method=method,
dag_details=dag_details, user=user)
+else:
+# Scenario 2
+resource_type = self._get_fab_resource_type(dag_access_entity)
+dag_method = ResourceMethod.GET if method == ResourceMethod.GET
else ResourceMethod.PUT
+
+return self._is_authorized_dag(
+method=dag_method, dag_details=dag_details, user=user
+) and self._is_authorized(method=method,
resource_type=resource_type, user=user)
+
+def is_authorized_dataset(self, *, method: ResourceMethod, user: BaseUser
| None = None) -> bool:
+return self._is_authorized(method=method,
resource_type=RESOURCE_DATASET, user=user)
+
+def is_authorized_variable(self, *, method: ResourceMethod, user: BaseUser
| None = None) -> bool:
+return self._is_authorized(method=method,
resource_type=RESOURCE_VARIABLE, user=user)
+
+def is_authorized_website(self, *, user: BaseUser | None = None) -> bool:
+return self._is_authorized(method=ResourceMethod.GET,
resource_type=RESOURCE_WEBSITE, user=user)
+
def get_security_manager_override_class(self) -> type:
"""Return the security manager override."""
from airflow.auth.managers.fab.security_manager.override import
FabAirflowSecurityManagerOverride
return FabAirflowSecurityManagerOverride
-def url_for(self, *args, **kwargs):
-"""Wrapper to allow mocking without having to import at the top of the
file."""
-from flask import url_for
-
-return url_for(*args, **kwargs)
-
def get_url_login(self, **kwargs) -> str:
"""Return the login page url."""
if not self.security_manager.auth_view:
raise AirflowException("`auth_view` not defined in the security
manager.")
if "next_url" in kwargs and kwargs["next_url"]:
-return
self.url_for(f"{self.security_manager.auth_view.endpoint}.login",
next=kwargs["next_url"])
+return
self._url_for(f"{self.security_manager.auth_view.endpoint}.login",
next=kwargs["next_url"])
else:
-return
self.url_for(f"{self.security_manager.auth_view.endpoint}.login")
+return
self._url_for(f"{self.security_manager.auth_view.endpoint}.login")
def get_url_logout(self):
"""Return the logout page url."""
if not self.security_manager.auth_view:
raise AirflowException("`auth_view` not defined in the security
manager.")
-return
self.url_for(f"{self.security_manager.auth_view.endpoint}.logout")
+return
self._url_for(f"{self.security_manager.auth_view.endpoint}.logout")
def get_url_user_profile(self) -> str | None:
"""Return the url to a page displaying info
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1317858875 ## airflow/auth/managers/base_auth_manager.py: ## @@ -102,3 +154,8 @@ def security_manager(self, security_manager: AirflowSecurityManager): :param security_manager: the security manager """ self._security_manager = security_manager + +@staticmethod +def is_dag_resource(resource_type: ResourceType) -> bool: +"""Determines if a resource relates to a DAG.""" +return resource_type == ResourceType.DAG Review Comment: Done :). Feel free to take a look if you're interested :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1317858529 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: Done. I really like it how it is now :) I agree it simplifies many things and the new "category" of resources it just great :D -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1317392293 ## airflow/auth/managers/base_auth_manager.py: ## @@ -102,3 +154,8 @@ def security_manager(self, security_manager: AirflowSecurityManager): :param security_manager: the security manager """ self._security_manager = security_manager + +@staticmethod +def is_dag_resource(resource_type: ResourceType) -> bool: +"""Determines if a resource relates to a DAG.""" +return resource_type == ResourceType.DAG Review Comment: I'll remove this one, I am currently working on refactoring this PR ## airflow/auth/managers/base_auth_manager.py: ## @@ -102,3 +154,8 @@ def security_manager(self, security_manager: AirflowSecurityManager): :param security_manager: the security manager """ self._security_manager = security_manager + +@staticmethod +def is_dag_resource(resource_type: ResourceType) -> bool: +"""Determines if a resource relates to a DAG.""" +return resource_type == ResourceType.DAG Review Comment: Putting it as draft for now -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1309104106 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: I see. I agree it makes more sense, I like the grouping of resources. I felt I followed blindly the way FAB defines resources but I definitely think, this new categorization is better. I think I'll go with that. I'll update the PR and see where it goes :) As usual, thanks for the comments! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1307665731 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: I have a second thought... > How many resource types we are going to have ... Can we enumerate them now? My understanding is the number of resource type will not change, hence from [permissions.py](https://github.com/apache/airflow/blob/main/airflow/security/permissions.py#L19), there are 40 resources types and as such, we should have 40 methods? This looks really massive. Also, regarding FAB auth manager, even though we consider this auth manager as an exception, all `is_authorized_` methods are going to basically call one private `_is_authorized` method because this is the same implementation. That makes me wonder whether other auth manager wont be in the same situation. We can easily imagine one auth manager with this pseudo code as implementation of `is_authorized`: ```python def is_authorized( self, action: ResourceMethod, resource_type: ResourceType, resource_details: ResourceDetails | None = None, user: BaseUser | None = None, ) -> bool: # Fetch the backend client of the service used tu make authorization decision client = return client.is_authorized( action=action.value, resource_type=resource_type.value extra=resource_details.to_dict(), ) ``` This is obviously some simplified code and some logic is probably missing but the idea is to basically pass the information to the underlying service which will make the authorization decision. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1307665731 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: I have a second thought... > How many resource types we are going to have ... Can we enumerate them now? My understanding is the number of resource type will not change, hence from [permissions.py](https://github.com/apache/airflow/blob/main/airflow/security/permissions.py#L19), there are 40 resources types and as such, we should have 40 methods? This looks really massive. Also, regarding FAB auth manager, even though we consider this auth manager as an exception, all `is_authorized_` methods are going to basically call one private `_is_authorized` method because this is the same implementation. That makes me wonder whether other auth manager wont be in the same situation. We can easily imagine one auth manager with this pseudo code as implementation of `is_authorized`: ``` def is_authorized( self, action: ResourceMethod, resource_type: ResourceType, resource_details: ResourceDetails | None = None, user: BaseUser | None = None, ) -> bool: # Fetch the backend client of the service used tu make authorization decision client = return client.is_authorized( action=action.value, resource_type=resource_type.value extra=resource_details.to_dict(), ) ``` This is obviously some simplified code and some logic is probably missing but the idea is to basically pass the information to the underlying service which will make the authorization decision. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1306146100 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: My only worry is ... what if we add a new resource type in the future in Airflow (like `dataset` that has been introduced not a long time ago). We would need to create a `is_authorized_` in `base_auth_manager` but we cannot make it abstract because that would be breaking change. What should be the process here? I am not saying this is a downside of using different methods because we would still have the same problem with one method (the unique method would not handle the new resource type) but I am just wondering what should be the process here in such situation -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1306055807 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: But I just realized we would still need to test the resource type to format the request to the backend service. Ok, I am with you now :D -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1306045944 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: I see, but then dont you think one generic method is enough? I guess it can depend on the auth manager but most likely the implementation of all these methods will be strictly the same. You just need to pass this `resource_id` information to the backend service you are using to perform authorization -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1306045944 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: I see, but then dont you think one generic method is enough? I guess it can depend on the auth manager bust most likely the implementation of all these methods will be strictly the same. You just need to pass this `resource_id` information to the backend service you are using to perform authorization -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1306020079 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: Even if we have 10, does it make sense to have `is_authorized_variable`, `is_authorized_connection`, `is_authorized_connection`, `is_authorized_configuration` which most likely will have their own `ResourceDetails` which will reference only the ID? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1305941953 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: I agree with **ALMOST** everything. At the end, I definitely agree with you we might end up having multiple private `_is_authorized_dag` ... There is one thing I dont really like with that though. We can certainly say that the resource attribute `id` is something common to all or the vast majority of resource types in Airflow. Example: when a user wants to modify a Variable `test-variable`, we definitely should call `is_authorized` with the variable ID `test-variable`. And so on for all resource types (there might be some exceptions). FAB auth manager might handle only the DAG ids but I dont think we should have this limitation in all auth managers. As a user/admin I'd like to be able to specify permissions by resource ID across all resource types. That would mean then having an `is_authorized` per resource type, which I think ... is massive. So I have a proposal: why dont we add an optional parameter `resource_id` to the `is_authorized` method and we drop `resource_details`. This method would be used for any resource type with no additional details. For resources where additional details are needed when calling `is_authorized`, we would expose individual `is_authorized_` API with associated `resource_details` (basically, your proposal) without the resource ID since it is already provided as individual parameter. WDYT? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1305851514 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: I definitely dont like having multiple `is_authorized` methods. I really like the fact we have one entry point for authorization: `is_authorized`. It makes it cleaner and simpler to me (very subjective I know). What if we use dictionary? How could we use dictionary and have only one method `is_authorized`? > Having a dictionary (and well defined structure of it "per resource typ" - seems more complex, but in fact it is esier. we are not tied with fields of ResourceDetails. How are you thinking achieving this? Something like [schemadict](https://pypi.org/project/schemadict/) and we would have one schema per resource type? Performance wise I am not sure this is the right decision since `is_authorized` is called quite a lot -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1305746442 ## airflow/auth/managers/base_auth_manager.py: ## @@ -63,6 +68,69 @@ def get_user_id(self) -> str: def is_logged_in(self) -> bool: """Return whether the user is logged in.""" +@abstractmethod +def is_authorized( +self, +action: ResourceAction, +resource_type: str, +resource_details: ResourceDetails | None = None, +user: BaseUser | None = None, +) -> bool: +""" +Return whether the user is authorized to perform a given action. + +.. code-block:: python + +# Check whether the logged-in user has permission to read the DAG "my_dag_id" +get_auth_manager().is_authorized( +Action.GET, +Resource.DAG, +ResourceDetails( +id="my_dag_id", +), +) + +:param action: the action to perform +:param resource_type: the type of resource the user attempts to perform the action on +:param resource_details: optional details about the resource itself +:param user: the user to perform the action on. If not provided (or None), it uses the current user +""" + +def is_all_authorized( +self, +actions: Sequence[AuthorizedAction], +) -> bool: +""" +Wrapper around `is_authorized` to check whether the user is authorized to perform several actions. + +:param actions: the list of actions to check. Each item represents the list of parameters of +`is_authorized` +""" +return all( +self.is_authorized( +action=action.action, +resource_type=action.resource_type, +resource_details=action.resource_details, +user=action.user, +) +for action in actions +) + +def _get_root_dag_id(self, dag_id: str) -> str: +""" +Return the root DAG id in case of sub DAG, return the DAG id otherwise. + +:param dag_id: the DAG id +""" +if "." in dag_id: Review Comment: I moved it to FAB auth manager to keep it backward compatible. As you said, it is now an implementation detail of FAB auth manager -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r130428 ## airflow/auth/managers/models/resource_details.py: ## @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass +class ResourceDetails: +""" +Represents the details of a resource. + +All fields must be optional. These details can be used in authorization decision. +""" + +id: str | None = None Review Comment: Oh I see. But would not be the same then to create a ResourceDetails dataclass per resource type and allowing any dataclass as part of the `is_authorized` method? Somehow I prefer dataclass than dict :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1304884883 ## airflow/auth/managers/base_auth_manager.py: ## @@ -63,6 +68,69 @@ def get_user_id(self) -> str: def is_logged_in(self) -> bool: """Return whether the user is logged in.""" +@abstractmethod +def is_authorized( +self, +action: ResourceAction, +resource_type: str, Review Comment: Haha, it has already been called out by @vandonr-amz, I was thinking of doing it in a separate PR but 2 callouts seems enough to justify it doing it here. I'll do it here -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1304884086 ## airflow/auth/managers/models/resource_action.py: ## @@ -0,0 +1,38 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from enum import Enum + + +class ResourceAction(Enum): Review Comment: I am good with that -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1304883915 ## airflow/auth/managers/base_auth_manager.py: ## @@ -63,6 +68,69 @@ def get_user_id(self) -> str: def is_logged_in(self) -> bool: """Return whether the user is logged in.""" +@abstractmethod +def is_authorized( +self, +action: ResourceAction, +resource_type: str, +resource_details: ResourceDetails | None = None, +user: BaseUser | None = None, +) -> bool: +""" +Return whether the user is authorized to perform a given action. + +.. code-block:: python + +# Check whether the logged-in user has permission to read the DAG "my_dag_id" +get_auth_manager().is_authorized( +Action.GET, +Resource.DAG, +ResourceDetails( +id="my_dag_id", +), +) + +:param action: the action to perform +:param resource_type: the type of resource the user attempts to perform the action on +:param resource_details: optional details about the resource itself +:param user: the user to perform the action on. If not provided (or None), it uses the current user +""" + +def is_all_authorized( +self, +actions: Sequence[AuthorizedAction], +) -> bool: +""" +Wrapper around `is_authorized` to check whether the user is authorized to perform several actions. + +:param actions: the list of actions to check. Each item represents the list of parameters of +`is_authorized` +""" +return all( +self.is_authorized( +action=action.action, +resource_type=action.resource_type, +resource_details=action.resource_details, +user=action.user, +) +for action in actions +) + +def _get_root_dag_id(self, dag_id: str) -> str: +""" +Return the root DAG id in case of sub DAG, return the DAG id otherwise. + +:param dag_id: the DAG id +""" +if "." in dag_id: Review Comment: Sounds good to me then :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1295119967
##
airflow/auth/managers/fab/fab_auth_manager.py:
##
@@ -17,13 +17,37 @@
# under the License.
from __future__ import annotations
+import itertools
+
from flask import url_for
from flask_login import current_user
from airflow import AirflowException
from airflow.auth.managers.base_auth_manager import BaseAuthManager
from airflow.auth.managers.fab.models import User
from airflow.auth.managers.fab.security_manager.override import
FabAirflowSecurityManagerOverride
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_action import ResourceAction
+from airflow.auth.managers.models.resource_details import ResourceDetails
+from airflow.security.permissions import (
+ACTION_CAN_ACCESS_MENU,
+ACTION_CAN_CREATE,
+ACTION_CAN_DELETE,
+ACTION_CAN_EDIT,
+ACTION_CAN_READ,
+RESOURCE_DAG,
+RESOURCE_DAG_PREFIX,
+)
+
+_MAP_ACTION_NAME_TO_FAB_ACTION_NAME = {
+ResourceAction.POST: [ACTION_CAN_CREATE],
+# ACTION_CAN_READ and ACTION_CAN_ACCESS_MENU are merged into because they
are very similar.
+# We can assume that if a user has permissions to read variables, they
also have permissions to access
+# the menu "Variables".
+ResourceAction.GET: [ACTION_CAN_READ, ACTION_CAN_ACCESS_MENU],
Review Comment:
It depends. To my understanding, some resources can only be linked to action
`ACTION_CAN_ACCESS_MENU` (e.g. `RESOURCE_DOCS_MENU`). Thus, if we do what you
suggested, these user experiences will be broken because they dont have `READ`
permissions for this given resource but only `ACTION_CAN_ACCESS_MENU`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1295078542
##
airflow/auth/managers/fab/fab_auth_manager.py:
##
@@ -17,13 +17,37 @@
# under the License.
from __future__ import annotations
+import itertools
+
from flask import url_for
from flask_login import current_user
from airflow import AirflowException
from airflow.auth.managers.base_auth_manager import BaseAuthManager
from airflow.auth.managers.fab.models import User
from airflow.auth.managers.fab.security_manager.override import
FabAirflowSecurityManagerOverride
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_action import ResourceAction
+from airflow.auth.managers.models.resource_details import ResourceDetails
+from airflow.security.permissions import (
+ACTION_CAN_ACCESS_MENU,
+ACTION_CAN_CREATE,
+ACTION_CAN_DELETE,
+ACTION_CAN_EDIT,
+ACTION_CAN_READ,
+RESOURCE_DAG,
+RESOURCE_DAG_PREFIX,
+)
+
+_MAP_ACTION_NAME_TO_FAB_ACTION_NAME = {
+ResourceAction.POST: [ACTION_CAN_CREATE],
+# ACTION_CAN_READ and ACTION_CAN_ACCESS_MENU are merged into because they
are very similar.
+# We can assume that if a user has permissions to read variables, they
also have permissions to access
+# the menu "Variables".
+ResourceAction.GET: [ACTION_CAN_READ, ACTION_CAN_ACCESS_MENU],
Review Comment:
The only concern is, if user X has `ACCESS_MENU` to given resource but not
`READ` permissions, it will be breaking experience for him. We have to give
`READ` permissions to user who have `ACCESS_MENU`.
But I agree, we might add too much complexity here to handle this. As you
say, we might want to remove `ACTION_CAN_ACCESS_MENU`, one way to do it it to
basically convert `ACTION_CAN_ACCESS_MENU` to `ACTION_CAN_READ` when loading
permissions for a given user
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1295115248
##
airflow/auth/managers/fab/fab_auth_manager.py:
##
@@ -17,13 +17,37 @@
# under the License.
from __future__ import annotations
+import itertools
+
from flask import url_for
from flask_login import current_user
from airflow import AirflowException
from airflow.auth.managers.base_auth_manager import BaseAuthManager
from airflow.auth.managers.fab.models import User
from airflow.auth.managers.fab.security_manager.override import
FabAirflowSecurityManagerOverride
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_action import ResourceAction
+from airflow.auth.managers.models.resource_details import ResourceDetails
+from airflow.security.permissions import (
+ACTION_CAN_ACCESS_MENU,
+ACTION_CAN_CREATE,
+ACTION_CAN_DELETE,
+ACTION_CAN_EDIT,
+ACTION_CAN_READ,
+RESOURCE_DAG,
+RESOURCE_DAG_PREFIX,
+)
+
+_MAP_ACTION_NAME_TO_FAB_ACTION_NAME = {
+ResourceAction.POST: [ACTION_CAN_CREATE],
+# ACTION_CAN_READ and ACTION_CAN_ACCESS_MENU are merged into because they
are very similar.
+# We can assume that if a user has permissions to read variables, they
also have permissions to access
+# the menu "Variables".
+ResourceAction.GET: [ACTION_CAN_READ, ACTION_CAN_ACCESS_MENU],
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1295081179
##
airflow/auth/managers/fab/fab_auth_manager.py:
##
@@ -81,3 +148,35 @@ def get_url_user_profile(self) -> str | None:
if not self.security_manager.user_view:
return None
return url_for(f"{self.security_manager.user_view.endpoint}.userinfo")
+
+@staticmethod
+def _get_fab_actions(action: ResourceAction) -> list[str]:
+"""
+Convert the action to a list of FAB actions.
+
+:param action: the action to convert
+
+:meta private:
+"""
+if action not in _MAP_ACTION_NAME_TO_FAB_ACTION_NAME:
+raise AirflowException(f"Unknown action: {action}")
+return _MAP_ACTION_NAME_TO_FAB_ACTION_NAME[action]
+
+@staticmethod
+def _resource_name_for_dag(dag_id: str) -> str:
+"""
+Returns the FAB resource name for a DAG id.
+
+Note that since a sub-DAG should follow the permission of its parent
DAG, you should pass
+``DagModel.root_dag_id`` to this function, for a subdag. A normal dag
should pass the
+``DagModel.dag_id``.
Review Comment:
Agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1295078542
##
airflow/auth/managers/fab/fab_auth_manager.py:
##
@@ -17,13 +17,37 @@
# under the License.
from __future__ import annotations
+import itertools
+
from flask import url_for
from flask_login import current_user
from airflow import AirflowException
from airflow.auth.managers.base_auth_manager import BaseAuthManager
from airflow.auth.managers.fab.models import User
from airflow.auth.managers.fab.security_manager.override import
FabAirflowSecurityManagerOverride
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_action import ResourceAction
+from airflow.auth.managers.models.resource_details import ResourceDetails
+from airflow.security.permissions import (
+ACTION_CAN_ACCESS_MENU,
+ACTION_CAN_CREATE,
+ACTION_CAN_DELETE,
+ACTION_CAN_EDIT,
+ACTION_CAN_READ,
+RESOURCE_DAG,
+RESOURCE_DAG_PREFIX,
+)
+
+_MAP_ACTION_NAME_TO_FAB_ACTION_NAME = {
+ResourceAction.POST: [ACTION_CAN_CREATE],
+# ACTION_CAN_READ and ACTION_CAN_ACCESS_MENU are merged into because they
are very similar.
+# We can assume that if a user has permissions to read variables, they
also have permissions to access
+# the menu "Variables".
+ResourceAction.GET: [ACTION_CAN_READ, ACTION_CAN_ACCESS_MENU],
Review Comment:
The only concern is, if user X has `ACCESS_MENU` to given resource but not
`READ` permissions, it will be breaking experience for him. We have to give
`READ` permissions to user who have `ACCESS_MENU`.
But I agree, we might too much complexity here to handle this. As you say,
we might want to remove `ACTION_CAN_ACCESS_MENU`, one way to do it it to
basically convert `ACTION_CAN_ACCESS_MENU` to `ACTION_CAN_READ` when loading
permissions for a given user
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1294952463 ## airflow/auth/managers/base_auth_manager.py: ## @@ -54,6 +58,67 @@ def get_user_id(self) -> str: def is_logged_in(self) -> bool: """Return whether the user is logged in.""" +@abstractmethod +def is_authorized( +self, +action: ResourceAction, +resource_type: str, Review Comment: I'll create an issue for it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1294951943 ## airflow/auth/managers/base_auth_manager.py: ## @@ -54,6 +58,67 @@ def get_user_id(self) -> str: def is_logged_in(self) -> bool: """Return whether the user is logged in.""" +@abstractmethod +def is_authorized( +self, +action: ResourceAction, +resource_type: str, +resource_details: ResourceDetails | None = None, +user: BaseUser | None = None, +) -> bool: +""" +Return whether the user is authorized to perform a given action. + +.. code-block:: python + +# Check whether the logged-in user has permission to read the DAG "my_dag_id" +get_auth_manager().is_authorized( +Action.GET, +Resource.DAG, +ResourceDetails( +id="my_dag_id", +), +) + +:param action: the action to perform +:param resource_type: the type of resource the user attempts to perform the action on +:param resource_details: optional details about the resource itself +:param user: the user to perform the action on. If not provided (or None), it uses the current user +""" + +def is_all_authorized( +self, +actions: Sequence[tuple[ResourceAction, str, ResourceDetails | None]], Review Comment: Done -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #33213: Implement `is_authorized()` in auth manager
vincbeck commented on code in PR #33213: URL: https://github.com/apache/airflow/pull/33213#discussion_r1294751595 ## airflow/auth/managers/base_auth_manager.py: ## @@ -54,6 +58,67 @@ def get_user_id(self) -> str: def is_logged_in(self) -> bool: """Return whether the user is logged in.""" +@abstractmethod +def is_authorized( +self, +action: ResourceAction, +resource_type: str, +resource_details: ResourceDetails | None = None, +user: BaseUser | None = None, +) -> bool: +""" +Return whether the user is authorized to perform a given action. + +.. code-block:: python + +# Check whether the logged-in user has permission to read the DAG "my_dag_id" +get_auth_manager().is_authorized( +Action.GET, +Resource.DAG, +ResourceDetails( +id="my_dag_id", +), +) + +:param action: the action to perform +:param resource_type: the type of resource the user attempts to perform the action on +:param resource_details: optional details about the resource itself +:param user: the user to perform the action on. If not provided (or None), it uses the current user +""" + +def is_all_authorized( +self, +actions: Sequence[tuple[ResourceAction, str, ResourceDetails | None]], Review Comment: I agree, that would be better -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
