Simplify fab has access lookup #19294

jhtimmins · 2021-10-28T21:06:49Z

Simplifies the FAB security manager code by doing the following.

Modifies permission lookup by accessing User.perms property method and checking if a permission pair is included in the user's list of perms.
Extends AnonymousUserMixin to create AnonymousUser, which has both perms and roles property methods. Removes the need for custom auth logic for anonymous users.
Removes helper methods that provide little value, including:

.get_readable_dags()
.current_user_has_permissions()
._has_resource_access()
.user_has_perms()
.user_has_roles()

Removes unused methods:

SecurityManager.is_item_public()

Moves the code to create a root dag resource name to Permissions.resource_name_for_dag()

This is the next piece of the permissions refactor.

andrewgodwin · 2021-11-02T17:17:41Z

airflow/www/fab_security/sqla/models.py

+    def perms(self):
+        perms = set()
+        for role in self.roles:
+            perms.update((perm.action.name, perm.resource.name) for perm in role.permissions)


I think you may need to do this as a single query for efficiency reasons?

I added lazy=joined to several model fields, which causes prefetching. It now runs 11 queries instead of 49.

@andrewgodwin that said, this PR does add some overall time to the tests. pytest tests/views --with-db-init runs in 1:53 on main and 2:00 on my branch. How significant is that?

airflow/api_connexion/endpoints/dag_run_endpoint.py

airflow/www/fab_security/manager.py

uranusjr · 2021-11-11T09:26:24Z

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


I feel the previous version is a bit easier to understand (i.e. “not anonymous but does not have any permissions”)

Suggested change

if not (g.user.is_anonymous or g.user.perms):

if not g.user.is_anonymous and not g.user.perms:

See this one @jhtimmins

ashb

LGTM -- a few small comments to address or say "naaah" to

tests/www/test_security.py

ashb · 2021-12-07T11:21:43Z

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


Suggested change

def get_permissions_from_roles(self, role_id: int) -> List[Permission]:

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)

Its actually not used anymore so should be removed entirely

ashb · 2021-12-07T11:28:17Z

airflow/www/fab_security/manager.py

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


from airflow.compat.functools import cached_property and then:

Suggested change

@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

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

I'll make this change in a follow up PR

ashb · 2021-12-07T11:28:54Z

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


See this one @jhtimmins

ashb · 2021-12-07T11:29:59Z

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


Unrelated change in this file?

jhtimmins · 2022-01-29T01:54:00Z

@ashb The most recent changes address a bug that only arises with MSSQL. Bc my changes to the permission models include prefetching of data, the parameters.apply_sorting() method doesn't work anymore https://github.com/apache/airflow/blob/main/airflow/api_connexion/parameters.py#L99. It's a utility method for allowing users to pass sort parameters to the API, and it then adds the order_by string to the query. This is problematic for joined queries, since sqlalchemy uses aliases to reference the joins and subqueries. MSSQL has a different alias naming scheme than the other DBs.

My change addresses the two API functions that query on the permission models, and replaces the calls to apply_sorting() with localized code to handle the different sort arguments.

I've duplicated some code, which I could remove by modifying the original .apply_sorting() method, but to prevent scope creep I'd rather do that in a future PR.

This reverts commit 3e98280.

boring-cyborg bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues labels Oct 28, 2021

jhtimmins force-pushed the simplify-fab-has-access-lookup branch from e7e11e7 to 8f2dbef Compare October 30, 2021 00:17

jhtimmins marked this pull request as ready for review November 1, 2021 22:50

jhtimmins requested review from ashb, bbovenzi, ephraimbuddy, kaxil, mik-laj, ryanahamilton and XD-DENG as code owners November 1, 2021 22:50

jhtimmins force-pushed the simplify-fab-has-access-lookup branch from 45decd0 to da380c4 Compare November 2, 2021 16:10

andrewgodwin reviewed Nov 2, 2021

View reviewed changes

jhtimmins force-pushed the simplify-fab-has-access-lookup branch from da380c4 to a682403 Compare November 3, 2021 03:22

uranusjr reviewed Nov 3, 2021

View reviewed changes

airflow/api_connexion/endpoints/dag_run_endpoint.py Outdated Show resolved Hide resolved

airflow/www/fab_security/manager.py Show resolved Hide resolved

jhtimmins force-pushed the simplify-fab-has-access-lookup branch 4 times, most recently from ecb8ad1 to bf6f20c Compare November 9, 2021 06:40

uranusjr reviewed Nov 11, 2021

View reviewed changes

jhtimmins force-pushed the simplify-fab-has-access-lookup branch from bf6f20c to 50f4180 Compare December 7, 2021 02:23

ashb reviewed Dec 7, 2021

View reviewed changes

jhtimmins force-pushed the simplify-fab-has-access-lookup branch 3 times, most recently from 5975e47 to 7b0487b Compare January 19, 2022 04:40

jhtimmins force-pushed the simplify-fab-has-access-lookup branch 2 times, most recently from d3fac5c to ff89d19 Compare January 29, 2022 01:44

jhtimmins requested review from ashb and uranusjr January 29, 2022 01:54

jhtimmins added 17 commits February 3, 2022 11:17

Remove unused imports.

29224cc

Clarify query params.

d04ab9a

Modify sort logic so MSSQL passes.

ccfcc17

Add text modifier to order_by values.

74cc0b6

Remove calls to get_*_dags.

5f255ef

Add back execution_date

2af14cf

Add back comma to match rest of file.

3c143ac

Remove unused permission functions.

873da42

Fix failing tests.

75aa353

Pass user object to current_app.appbuilder.sm.has_all_dags_access.

00dfaf6

Remove attempts to fix query.

bc84d84

Update the api_connexion query builders.

7e07f6f

Add typing.

50d2d98

Apply sorts directly to model objects.

540fc98

Apply sorts directly to model objects.

d3c8d87

Standardize custom sort code.

8272a77

Fix merge conflicts.

5d22b20

jhtimmins requested a review from potiuk as a code owner February 3, 2022 21:05

jhtimmins added 2 commits February 3, 2022 15:17

Make sure joined fields prefetch.

44acd26

Dont use cached_property, since its only on > 3.8.

197d611

jhtimmins merged commit 3e98280 into apache:main Feb 4, 2022

jhtimmins deleted the simplify-fab-has-access-lookup branch February 4, 2022 17:09

potiuk mentioned this pull request Feb 9, 2022

AssertionError: Dependency rule tried to blank-out primary key column 'ab_permission.id' on instance ' #21407

Closed

2 tasks

potiuk added a commit to potiuk/airflow that referenced this pull request Feb 9, 2022

Revert "Simplify fab has access lookup (apache#19294)"

8768887

This reverts commit 3e98280.

ephraimbuddy added the type:improvement Changelog: Improvements label Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify fab has access lookup #19294

Simplify fab has access lookup #19294

jhtimmins commented Oct 28, 2021 •

edited

Loading

andrewgodwin Nov 2, 2021

jhtimmins Nov 3, 2021

jhtimmins Nov 3, 2021

uranusjr Nov 11, 2021

ashb Dec 7, 2021

ashb left a comment

ashb Dec 7, 2021

jhtimmins Jan 14, 2022

ashb Dec 7, 2021

jhtimmins Jan 14, 2022

ashb Dec 7, 2021

ashb Dec 7, 2021

jhtimmins commented Jan 29, 2022

	if not (g.user.is_anonymous or g.user.perms):
	if not g.user.is_anonymous and not g.user.perms:

	def get_permissions_from_roles(self, role_id: int) -> List[Permission]:
	def get_permissions_from_role(self, role_id: int) -> List[Permission]:

Simplify fab has access lookup #19294

Simplify fab has access lookup #19294

Conversation

jhtimmins commented Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhtimmins commented Jan 29, 2022

jhtimmins commented Oct 28, 2021 •

edited

Loading