Add updated-name wrappers for built-in FAB methods. #16077

jhtimmins · May 26, 2021

This is a re-implementation of one piece of #15398, which uses the "resource" and "action" naming scheme.

The original PR has proved difficult to get reviewed and through CI, as it touches a lot of different files that change frequently. This PR introduces wrapper methods around default FAB methods, this time using the updated naming scheme.

The methods are not in use yet; the next PR will update method calls throughout Airflow to use the new methods instead of the default FAB methods.

jedcunningham

Overall LGTM. Minor nit, might be nice to having typing on these.

jedcunningham · May 26, 2021

airflow/www/security.py

+        :return: Action record, if it exists
+        :rtype: Permission
+        """
+        return super().find_permission(name)


Why is this super() vs self?

And should we change parameter to action_name? for consistency ?

Generally, I like to add only as much explanation as is necessary based on context. In this case it seems reasonable to infer that name refers to the name of the action being retrieved.

potiuk · May 26, 2021

airflow/www/security.py

@@ -409,6 +474,20 @@ def has_access(self, permission, resource, user=None) -> bool:

        return has_access

+    def _has_access(self, user, action, resource) -> bool:


I think "action_name" and "resource_name" should be here as well.

potiuk · May 26, 2021

airflow/www/security.py

@@ -639,6 +750,16 @@ def sync_perm_for_dag(self, dag_id, access_control=None):
        if access_control:
            self._sync_dag_view_permissions(dag_resource_name, access_control)

+    def get_resource_permissions(self, resource):


Just double-checking - resource here is different than "resource_name" (looks like - from the type).

Yeah, resource should apply to the actual object

potiuk

Few consistency nits.

Would you also care to add Type annotations for parameters? That would make mypy verify types of the parameters in most cases - especially when there was action vs. action_name or resource vs. resource_name.

kaxil · May 26, 2021

airflow/www/security.py

+        Returns a resource record by name, if it exists.
+        :param name: Name of resource


Sphinx needs one blank line between description and params

Suggested change

Returns a resource record by name, if it exists.

:param name: Name of resource

Returns a resource record by name, if it exists.

:param name: Name of resource

airflow/www/security.py

kaxil · May 26, 2021

airflow/www/security.py

+        Gets an existing action record.
+        :param name: name


Suggested change

Gets an existing action record.

:param name: name

Gets an existing action record.

:param name: name

kaxil · May 26, 2021

airflow/www/security.py

+        Gets a permission made with the given action->resource pair, if the permission already exists.
+        :param action_name: Name of action


Suggested change

Gets a permission made with the given action->resource pair, if the permission already exists.

:param action_name: Name of action

Gets a permission made with the given action->resource pair, if the permission already exists.

:param action_name: Name of action

kaxil · May 26, 2021

airflow/www/security.py

+        Creates a permission linking an action and resource.
+        :param action_name: Name of existing action


Suggested change

Creates a permission linking an action and resource.

:param action_name: Name of existing action

Creates a permission linking an action and resource.

:param action_name: Name of existing action

kaxil · May 26, 2021

airflow/www/security.py

+        Add an existing permission pair to a role.
+        :param role: The role about to get a new permission.


Suggested change

Add an existing permission pair to a role.

:param role: The role about to get a new permission.

Add an existing permission pair to a role.

:param role: The role about to get a new permission.

kaxil · May 26, 2021

airflow/www/security.py

+        Remove a permission pair from a role.
+        :param role: User role containing permissions.


Suggested change

Remove a permission pair from a role.

:param role: User role containing permissions.

Remove a permission pair from a role.

:param role: User role containing permissions.

kaxil · May 26, 2021

airflow/www/security.py

+        Deletes a permission action.
+        :param name: Name of action to delete (e.g. can_read).


Suggested change

Deletes a permission action.

:param name: Name of action to delete (e.g. can_read).

Deletes a permission action.

:param name: Name of action to delete (e.g. can_read).

kaxil · May 26, 2021

airflow/www/security.py

+        Retrieve permission pairs associated with a specific resource object.
+        :param resource: Object representing a single resource.


Suggested change

Retrieve permission pairs associated with a specific resource object.

:param resource: Object representing a single resource.

Retrieve permission pairs associated with a specific resource object.

:param resource: Object representing a single resource.

kaxil · May 26, 2021

airflow/www/security.py

+        then re-syncs them.
+        :return: None


Suggested change

then re-syncs them.

:return: None

then re-syncs them.

:return: None

jhtimmins · May 27, 2021

@potiuk can you take another look? I've added typing and either updated or explained the naming choices.

github-actions · May 27, 2021

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

potiuk

LGTM. Thanks for the explanations. Makes sense.

jhtimmins · May 29, 2021

Thanks @potiuk. Is the failing MSSQL test related to that other PR about improving MSSQL compatibility?

potiuk · May 30, 2021

Yep. We already have a fix for that. We looked for the problem for last few days and it turned out it is caused by ..... Tmpfs backing our docker engine for self-hosted runners (they are optimized for speed). Fix is coming in #16159

boring-cyborg bot added the area:webserver Webserver related Issues label May 26, 2021

jhtimmins requested review from ashb, jedcunningham and kaxil May 26, 2021 02:56

jhtimmins force-pushed the add-fab-wrapper-functions branch from cb60630 to 6345285 Compare May 26, 2021 03:04

jedcunningham reviewed May 26, 2021

View reviewed changes

potiuk reviewed May 26, 2021

View reviewed changes

potiuk requested changes May 26, 2021

View reviewed changes

jhtimmins force-pushed the add-fab-wrapper-functions branch from 6b815c9 to 129ebf1 Compare May 26, 2021 20:57

jhtimmins requested review from potiuk and jedcunningham May 26, 2021 20:59

jedcunningham approved these changes May 26, 2021

View reviewed changes

kaxil reviewed May 26, 2021

View reviewed changes

jhtimmins force-pushed the add-fab-wrapper-functions branch from 129ebf1 to c05946a Compare May 27, 2021 01:21

jhtimmins requested a review from kaxil May 27, 2021 01:25

kaxil approved these changes May 27, 2021

View reviewed changes

jhtimmins requested review from mik-laj, turbaszek, vikramkoka and XD-DENG as code owners May 27, 2021 18:52

jhtimmins added 4 commits May 27, 2021 11:55

Add updated-name wrappers for built-in FAB methods.

d2389ce

Update typing.

b24de1c

Add spacing.

2c2e892

Add updated-name wrappers for built-in FAB methods.

1768355

jhtimmins force-pushed the add-fab-wrapper-functions branch from 08e46ad to 1768355 Compare May 27, 2021 19:02

potiuk approved these changes May 27, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label May 27, 2021

potiuk approved these changes May 29, 2021

View reviewed changes

potiuk merged commit a27e930 into apache:master May 29, 2021

jhtimmins mentioned this pull request Jun 1, 2021

call resource based fab methods. #16190

Merged

		Returns a resource record by name, if it exists.
		:param name: Name of resource

		Gets a permission made with the given action->resource pair, if the permission already exists.
		:param action_name: Name of action

		Creates a permission linking an action and resource.
		:param action_name: Name of existing action

		Add an existing permission pair to a role.
		:param role: The role about to get a new permission.

		Remove a permission pair from a role.
		:param role: User role containing permissions.

		Deletes a permission action.
		:param name: Name of action to delete (e.g. can_read).

		Retrieve permission pairs associated with a specific resource object.
		:param resource: Object representing a single resource.

Search code, repositories, users, issues, pull requests...

Add updated-name wrappers for built-in FAB methods. #16077

Add updated-name wrappers for built-in FAB methods. #16077

Uh oh!

Conversation

jhtimmins commented May 26, 2021

Uh oh!

jedcunningham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhtimmins commented May 27, 2021

Uh oh!

github-actions bot commented May 27, 2021

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

jhtimmins commented May 29, 2021

Uh oh!

potiuk commented May 30, 2021

Uh oh!

Uh oh!