[GitHub] [airflow] jhtimmins commented on a change in pull request #12458: Improve www.security.get_accessible_dags() and webserver performance

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

jhtimmins commented on a change in pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#discussion_r526623519



##########
File path: airflow/www/security.py
##########
@@ -261,6 +261,8 @@ def get_accessible_dags(self, user_actions, user, session=None):
         for role in user_query.roles:
             for permission in role.permissions:
                 resource = permission.view_menu.name
+                if resource == permissions.RESOURCE_DAG:
+                    return session.query(DagModel)

Review comment:
       This check needs to take place after the action check on line 264. This loops over all the permissions that a user has from their various roles, and if that permission resource is either `Dags` or a dag_id, then it checks whether the action for that permission matches the `user_actions` the function is looking up.
   
   With the ordering included in this code, if the user has either `Dags.can_read` or `Dags.can_edit`, all dags will be returned. But if `user_actions == ['can_edit']`, all dags will get returned if the user has _any_ dag resource permission. So if they can view all dags, this function will incorrectly indicate that they can also edit all dags as well.
   
   We can fix this by first checking that the permission's action is one we care about (listed in `user_actions`). I've included a fix and a test to check for this in https://github.com/apache/airflow/pull/12473.
   
   Feel free to incorporate those changes into this PR, then we can delete the other PR.




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