[GitHub] [airflow] ashb commented on a change in pull request #19294: Simplify fab has access lookup

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

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



##########
File path: airflow/api_connexion/endpoints/role_and_permission_endpoint.py
##########
@@ -68,11 +68,19 @@ def get_roles(*, order_by: str = "name", limit: int, offset: Optional[int] = Non
     appbuilder = current_app.appbuilder
     session = appbuilder.get_session
     total_entries = session.query(func.count(Role.id)).scalar()
+    direction = "desc" if order_by.startswith("-") else "asc"
     to_replace = {"role_id": "id"}
+    order_param = order_by.strip("-")
+    order_param = to_replace.get(order_param, order_param)
     allowed_filter_attrs = ["role_id", "name"]
+    if order_by not in allowed_filter_attrs:
+        raise BadRequest(
+            detail=f"Ordering with '{order_by}' is disallowed or "
+            f"the attribute does not exist on the model"
+        )
+
     query = session.query(Role)
-    query = apply_sorting(query, order_by, to_replace, allowed_filter_attrs)
-    roles = query.offset(offset).limit(limit).all()
+    roles = query.order_by(getattr(getattr(Role, order_param), direction)()).offset(offset).limit(limit).all()

Review comment:
       ```suggestion
       roles = query.order_by(direction(getattr(Role, order_param)).offset(offset).limit(limit).all()
   ```

##########
File path: airflow/api_connexion/endpoints/role_and_permission_endpoint.py
##########
@@ -68,11 +68,19 @@ def get_roles(*, order_by: str = "name", limit: int, offset: Optional[int] = Non
     appbuilder = current_app.appbuilder
     session = appbuilder.get_session
     total_entries = session.query(func.count(Role.id)).scalar()
+    direction = "desc" if order_by.startswith("-") else "asc"

Review comment:
       Rather than the double `getattr` we should use the standalone asc/desc functions https://docs.sqlalchemy.org/en/13/core/sqlelement.html?highlight=desc#sqlalchemy.sql.expression.desc
   
   ```suggestion
       direction = desc if order_by.startswith("-") else asc
   ```
   
   etc

##########
File path: airflow/www/security.py
##########
@@ -397,62 +354,35 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
         """
         if not user:
             user = g.user
+        if (action_name, resource_name) in user.perms:
+            return True
 
-        if user.is_anonymous:
-            user.roles = self.get_user_roles(user)
-
-        has_access = self._has_access(user, action_name, resource_name)
-        # FAB built-in view access method. Won't work for AllDag access.
         if self.is_dag_resource(resource_name):
-            if action_name == permissions.ACTION_CAN_READ:
-                has_access |= self.can_read_dag(resource_name, user)
-            elif action_name == permissions.ACTION_CAN_EDIT:
-                has_access |= self.can_edit_dag(resource_name, user)
-
-        return has_access
-
-    def _has_access(self, user: User, action_name: str, resource_name: str) -> bool:
-        """
-        Wraps the FAB built-in view access method. Won't work for AllDag access.
-
-        :param user: user object
-        :param action_name: action_name on resource (e.g can_read, can_edit).
-        :param resource_name: name of resource.
-        :return: a bool whether user could perform certain action on the resource.
-        :rtype bool
-        """
-        return bool(self._has_resource_access(user, action_name, resource_name))
+            if (action_name, permissions.RESOURCE_DAG) in user.perms:
+                return True
+            return (action_name, resource_name) in user.perms
 
-    def _get_and_cache_perms(self):
-        """Cache permissions"""
-        self.perms = self.get_current_user_permissions()
+        return False
 
-    def _has_role(self, role_name_or_list):
+    def _has_role(self, role_name_or_list, user):
         """Whether the user has this role name"""
         if not isinstance(role_name_or_list, list):
             role_name_or_list = [role_name_or_list]
-        return any(r.name in role_name_or_list for r in self.get_user_roles())
+        return any(r.name in role_name_or_list for r in user.roles)
 
-    def _has_perm(self, action_name, resource_name):
-        """Whether the user has this perm"""
-        if hasattr(self, 'perms') and self.perms is not None:
-            if (action_name, resource_name) in self.perms:
-                return True
-        # rebuild the permissions set
-        self._get_and_cache_perms()
-        return (action_name, resource_name) in self.perms
-
-    def has_all_dags_access(self):
+    def has_all_dags_access(self, user):
         """
         Has all the dag access in any of the 3 cases:
         1. Role needs to be in (Admin, Viewer, User, Op).
         2. Has can_read action on dags resource.
         3. Has can_edit action on dags resource.
         """
+        if not user:
+            user = g.user
         return (
-            self._has_role(['Admin', 'Viewer', 'Op', 'User'])
-            or self._has_perm(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG)
-            or self._has_perm(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG)
+            self._has_role(['Admin', 'Viewer', 'Op', 'User'], user)

Review comment:
       (Unrelated to this PR, we should remove this Role check -- those roles _have_ these permissions and by having this here it means you _can't_ remove access to all dags from the stock roles.)




-- 
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-unsubscribe@airflow.apache.org

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