shahar1 commented on code in PR #40703:
URL: https://github.com/apache/airflow/pull/40703#discussion_r1675711163
##########
airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -1102,81 +1103,103 @@ def sync_perm_for_dag(
:param dag_id: the ID of the DAG whose permissions should be updated
:param access_control: a dict where each key is a role name and
- each value is a set() of action names (e.g.,
+ each value is a set() of action names in case of DAGs resource
(e.g.,
{'can_read'}
+ or the value can be a dict where each key is a resource name and
+ each value is a set() of action names (e.g., {'DAG Runs':
{'can_read'}})
Review Comment:
Try to clarify the description, it's quite hard to read (same goes for
similar docstrings)
##########
airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -1093,7 +1094,7 @@ def is_dag_resource(self, resource_name: str) -> bool:
def sync_perm_for_dag(
self,
dag_id: str,
- access_control: dict[str, Collection[str]] | None = None,
Review Comment:
Instead of removing the type hint - make it up to date (same goes for the
other mentions of `access_control` in methods' signatures)
##########
airflow/models/dag.py:
##########
@@ -906,21 +908,33 @@ def
_upgrade_outdated_dag_access_control(access_control=None):
"""
if access_control is None:
return None
- new_perm_mapping = {
+ new_dag_perm_mapping = {
permissions.DEPRECATED_ACTION_CAN_DAG_READ:
permissions.ACTION_CAN_READ,
permissions.DEPRECATED_ACTION_CAN_DAG_EDIT:
permissions.ACTION_CAN_EDIT,
}
+
+ def update_old_perm(permission: str):
+ new_perm = new_dag_perm_mapping.get(permission, permission)
+ if new_perm != permission:
+ warnings.warn(
+ f"The '{permission}' permission is deprecated. Please use
'{new_perm}'.",
+ RemovedInAirflow3Warning,
+ stacklevel=3,
+ )
Review Comment:
Could you please add a test for this warning?
##########
airflow/security/permissions.py:
##########
@@ -64,19 +67,38 @@
DEPRECATED_ACTION_CAN_DAG_READ = "can_dag_read"
DEPRECATED_ACTION_CAN_DAG_EDIT = "can_dag_edit"
-DAG_ACTIONS = {ACTION_CAN_READ, ACTION_CAN_EDIT, ACTION_CAN_DELETE}
+
+class ResourceDetails(TypedDict):
+ """Details of a resource (actions and prefix)."""
+
+ actions: set[str]
+ prefix: str
+
+
+RESOURCE_DETAILS_MAP = {
+ RESOURCE_DAG: ResourceDetails(
+ actions={ACTION_CAN_READ, ACTION_CAN_EDIT, ACTION_CAN_DELETE},
prefix=RESOURCE_DAG_PREFIX
+ ),
+ RESOURCE_DAG_RUN: ResourceDetails(
+ actions={ACTION_CAN_READ, ACTION_CAN_CREATE, ACTION_CAN_DELETE,
ACTION_CAN_ACCESS_MENU},
+ prefix=RESOURCE_DAG_RUN_PREFIX,
+ ),
+}
+PREFIX_RESOURCES_MAP = {
+ prefix: resource for resource, actions in RESOURCE_DETAILS_MAP.items() for
prefix in actions["prefix"]
+}
-def resource_name_for_dag(root_dag_id: str) -> str:
Review Comment:
I'm quite concerned of backward compatbility issues - FAB provider in an
outdated version will fail importing `DAG_ACTIONS` and `resource_name_for_dag`
as that they don't exist anymore.
##########
airflow/providers/fab/auth_manager/fab_auth_manager.py:
##########
@@ -412,7 +413,33 @@ def _is_authorized_dag(
if details and details.id:
# Check whether the user has permissions to access a specific DAG
- resource_dag_name = self._resource_name_for_dag(details.id)
+ resource_dag_name = self._resource_name(details.id, RESOURCE_DAG)
+ return self._is_authorized(method=method,
resource_type=resource_dag_name, user=user)
+
+ return False
+
+ def _is_authorized_dag_run(
+ self,
+ method: ResourceMethod,
+ details: DagDetails | None = None,
+ user: BaseUser | None = None,
+ ) -> bool:
+ """
+ Return whether the user is authorized to perform a given action on a
DAG Run.
+
+ :param method: the method to perform
+ :param details: optional details about the DAG
+ :param user: the user to perform the action on. If not provided (or
None), it uses the current user
Review Comment:
```suggestion
:param details: optional, details about the DAG
:param user: optional, the user to perform the action on. If not
provided, it uses the current 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
