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



##########
File path: tests/www/test_security.py
##########
@@ -29,6 +29,7 @@
 from airflow.models.dag import DAG
 from airflow.security import permissions
 from airflow.www import app as application
+from airflow.www.fab_security.manager import AnonymousUser

Review comment:
       This is the only change in this file -- is it needed?

##########
File path: airflow/www/fab_security/manager.py
##########
@@ -85,6 +85,33 @@ def _oauth_tokengetter(token=None):
     return token
 
 
+class AnonymousUser(AnonymousUserMixin):
+    """User object used when no active user is logged in."""
+
+    _roles = set()
+    _perms = set()
+
+    @property
+    def roles(self):
+        if not self._roles:
+            public_role = current_app.appbuilder.get_app.config["AUTH_ROLE_PUBLIC"]
+            self._roles = {current_app.appbuilder.sm.find_role(public_role)} if public_role else set()
+        return self._roles
+
+    @roles.setter
+    def roles(self, roles):
+        self._roles = roles
+        self._perms = set()
+
+    @property
+    def perms(self):
+        if not self._perms:
+            self._perms = set()
+            for role in self.roles:
+                self._perms.update({(perm.action.name, perm.resource.name) for perm in role.permissions})
+        return self._perms

Review comment:
       `from airflow.compat.functools import cached_property` and then:
   
   ```suggestion
       @cached_property
       def perms(self):
           _perms = set()
           for role in self.roles:
               _perms.update({(perm.action.name, perm.resource.name) for perm in role.permissions})
           return _perms
   ```
   And then `del self.perms` in the `@roles.setter` to clear the cache.
   
   (This is all functionally the same, I just find the delcarative `@cached_property` clearer to read.)

##########
File path: airflow/api_connexion/parameters.py
##########
@@ -100,7 +100,7 @@ def apply_sorting(query, order_by, to_replace=None, allowed_attrs=None):
     if to_replace:
         lstriped_orderby = to_replace.get(lstriped_orderby, lstriped_orderby)
     if order_by[0] == "-":
-        order_by = f"{lstriped_orderby} desc"
+        order_by = desc(text(lstriped_orderby))

Review comment:
       Unrelated change in this file?

##########
File path: airflow/www/auth.py
##########
@@ -35,7 +35,7 @@ def decorated(*args, **kwargs):
             __tracebackhide__ = True  # Hide from pytest traceback.
 
             appbuilder = current_app.appbuilder
-            if not g.user.is_anonymous and not appbuilder.sm.current_user_has_permissions():
+            if not (g.user.is_anonymous or g.user.perms):

Review comment:
       See this one @jhtimmins 

##########
File path: airflow/www/fab_security/sqla/manager.py
##########
@@ -339,7 +333,7 @@ def filter_roles_by_perm_with_action(self, action_name: str, role_ids: List[int]
             )
         ).all()
 
-    def get_role_permissions_from_db(self, role_id: int) -> List[Permission]:
+    def get_permissions_from_roles(self, role_id: int) -> List[Permission]:

Review comment:
       ```suggestion
       def get_permissions_from_role(self, role_id: int) -> List[Permission]:
   ```
   Should be singular given it takes a single role_id. (Possibly also called `_from_role_id`, but don't mind on that one)




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