[GitHub] [airflow] ashb commented on a change in pull request #11189: Prepend `DAG:` to dag permissions

[GitBox <gi...@apache.org>]

ashb commented on a change in pull request #11189:
URL: https://github.com/apache/airflow/pull/11189#discussion_r503123428



##########
File path: airflow/www/security.py
##########
@@ -520,18 +550,23 @@ def merge_pv(perm, view_menu):
     def update_admin_perm_view(self):
         """
         Admin should has all the permission-views, except the dag views.
-        because Admin have already have all_dags permission.
+        because Admin have already have Dag permission.
         Add the missing ones to the table for admin.
 
         :return: None.
         """
-        all_dag_view = self.find_view_menu('all_dags')
-        dag_perm_ids = [self.find_permission('can_dag_edit').id, self.find_permission('can_dag_read').id]
+        all_dag_view = self.find_view_menu(permissions.RESOURCE_ALL_DAGS)
+        pvs = (

Review comment:
       ```suggestion
           dag_pvs = (
   ```

##########
File path: airflow/www/security.py
##########
@@ -377,13 +406,13 @@ def has_all_dags_access(self):
         """
         Has all the dag access in any of the 3 cases:
         1. Role needs to be in (Admin, Viewer, User, Op).
-        2. Has can_dag_read permission on all_dags view.
-        3. Has can_dag_edit permission on all_dags view.
+        2. Has can_read permission on dags view.
+        3. Has can_edit permission on dags view.

Review comment:
       ```suggestion
           2. Has can_read permission on AllDags view.
           3. Has can_edit permission on AllDags view.
   ```
   
   (Not as part of this PR, but why on earth do we check the role membership at all? We should just use the permissions system!) 

##########
File path: airflow/models/dag.py
##########
@@ -176,7 +176,7 @@ class DAG(BaseDag, LoggingMixin):
         that it is executed when the dag succeeds.
     :type on_success_callback: callable
     :param access_control: Specify optional DAG-level permissions, e.g.,
-        "{'role1': {'can_dag_read'}, 'role2': {'can_dag_read', 'can_dag_edit'}}"
+        "{'role1': {'can_read'}, 'role2': {'can_read', 'can_edit'}}"

Review comment:
       I think somewhere we should have an "upgrade" of the old style of permissions names for this param -- otherwise when upgrading custom DAG permissions would stop working and not give any obvoius reason why.

##########
File path: airflow/www/views.py
##########
@@ -545,7 +546,7 @@ def dag_stats(self, session=None):
         dr = models.DagRun
 
         allowed_dag_ids = current_app.appbuilder.sm.get_accessible_dag_ids(g.user)
-        if 'all_dags' in allowed_dag_ids:
+        if permissions.RESOURCE_ALL_DAGS in allowed_dag_ids:

Review comment:
       ```suggestion
           if permissions.RESOURCE_ALL_DAGS in allowed_dag_ids:
   ```
   
   This won't ever be true anymore will it, cos `get_accessible_dag_ids` already handles this an filters it to return all dag ids. (This is probably a separate PR, and this change is fine here)

##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -3595,7 +3595,7 @@ def test_execute_queries_count_with_harvested_dags(self, expected_query_count, d
             job.heartbeat = mock.MagicMock()
             job.processor_agent = mock_agent
 
-            with assert_queries_count(expected_query_count):
+            with assert_queries_count(expected_query_count, margin_of_error=5):

Review comment:
       Here too - revert the changes to this file please.

##########
File path: airflow/api_connexion/security.py
##########
@@ -54,7 +55,7 @@ def check_authorization(
         return
     appbuilder = current_app.appbuilder
     for permission in permissions:
-        if permission in (('can_read', 'Dag'), ('can_edit', 'Dag')):
+        if permission in (('can_read', RESOURCE_ALL_DAGS), ('can_edit', RESOURCE_ALL_DAGS)):

Review comment:
       I'm not sure about this one -- the permission we declare on the models feels like it should still be `Dag` -- otherwise it reads as if we are asking explicitly for permission on AllDags, not just "the requested Dag".

##########
File path: airflow/cli/commands/role_command.py
##########
@@ -29,9 +29,7 @@ def roles_list(args):
     roles = appbuilder.sm.get_all_roles()
     print("Existing roles:\n")
     role_names = sorted([[r.name] for r in roles])
-    msg = tabulate(role_names,
-                   headers=['Role'],
-                   tablefmt=args.output)
+    msg = tabulate(role_names, headers=['Role'], tablefmt=args.output)

Review comment:
       Could you undo this please? (We like to not have whitespace-only/style changes in files otherwise untouched in a PR)

##########
File path: airflow/www/security.py
##########
@@ -320,33 +307,75 @@ def get_accessible_dags(self, user_actions, user, session=None):
             for permission in role.permissions:
                 resource = permission.view_menu.name
                 action = permission.permission.name
-                if action in user_actions:
+                if action not in user_actions:
+                    continue
+
+                if resource.startswith(permissions.RESOURCE_DAG_PREFIX):
+                    resources.add(resource[len(permissions.RESOURCE_DAG_PREFIX) :])
+                else:
                     resources.add(resource)
 
-        if bool({'Dag', 'all_dags'}.intersection(resources)):
+        if permissions.RESOURCE_ALL_DAGS in resources:
             return session.query(DagModel)
 
         return session.query(DagModel).filter(DagModel.dag_id.in_(resources))
 
-    def has_access(self, permission, view_name, user=None) -> bool:
+    def can_read_dag(self, dag_id, user=None) -> bool:
+        """Determines whether a user has DAG read access."""
+        if not user:
+            user = g.user
+        prefixed_dag_id = self.prefixed_dag_id(dag_id)
+        return self._has_view_access(
+            user, permissions.ACTION_CAN_READ, permissions.RESOURCE_ALL_DAGS
+        ) or self._has_view_access(user, permissions.ACTION_CAN_READ, prefixed_dag_id)
+
+    def can_edit_dag(self, dag_id, user=None) -> bool:
+        """Determines whether a user has DAG edit access."""
+        if not user:
+            user = g.user
+        prefixed_dag_id = self.prefixed_dag_id(dag_id)
+
+        return self._has_view_access(
+            user, permissions.ACTION_CAN_EDIT, permissions.RESOURCE_ALL_DAGS
+        ) or self._has_view_access(user, permissions.ACTION_CAN_EDIT, prefixed_dag_id)
+
+    def prefixed_dag_id(self, dag_id):
+        """Returns the permission name for a DAG id."""
+        if dag_id == permissions.RESOURCE_ALL_DAGS:
+            return dag_id
+
+        if dag_id.startswith(permissions.RESOURCE_DAG_PREFIX):
+            return dag_id
+        return f"{permissions.RESOURCE_DAG_PREFIX}{dag_id}"
+
+    def has_access(self, permission, resource, user=None) -> bool:
         """
         Verify whether a given user could perform certain permission
         (e.g can_read, can_write) on the given dag_id.
 
         :param permission: permission on dag_id(e.g can_read, can_edit).
         :type permission: str
-        :param view_name: name of view-menu(e.g dag id is a view-menu as well).
-        :type view_name: str
+        :param resource: name of view-menu or resource.
+        :type resource: str
         :param user: user name
         :type user: str
         :return: a bool whether user could perform certain permission on the dag_id.
         :rtype bool
         """
         if not user:
             user = g.user
+
         if user.is_anonymous:
-            return self.is_item_public(permission, view_name)
-        return self._has_view_access(user, permission, view_name)
+            return self.is_item_public(permission, resource)
+
+        has_access = self._has_view_access(user, permission, resource)
+
+        if permission == permissions.ACTION_CAN_READ:
+            has_access |= self.can_read_dag(resource, user)
+        elif permission == permissions.ACTION_CAN_EDIT:
+            has_access |= self.can_edit_dag(resource, user)

Review comment:
       I think this is going to return true too often:
   
   For example, if the permission check is `can_edit on Variables`, then can_edit_dag checks for the following:
   
   - "can_edit on AllDags" or
   - "can_edit on DAG:Variables"
   
   This means the if anyone has can_edit on AllDags, they get can_edit on _everything_.
   
   We need to improve the tests as this wasn't caught by them and so needs to be.
   

##########
File path: tests/test_utils/asserts.py
##########
@@ -69,16 +72,22 @@ def after_cursor_execute(self, *args, **kwargs):
 
 
 @contextmanager
-def assert_queries_count(expected_count, message_fmt=None):
+def assert_queries_count(expected_count, message_fmt=None, margin_of_error=0):

Review comment:
       Now that we've quarantined the failing tests (for now) could you revert the change to this file please?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org