Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement is_authorized() in auth manager #33213

Merged
merged 27 commits into from
Sep 25, 2023

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Aug 8, 2023

Resolves #32204.

Implement the method is_authorized() in auth manager. The purpose of this method is to be the user authorization decision maker. The purpose of this method is to check whether the current user (or any user) is authorized to make an action on a given resource. In this PR I use it only in the decorator auth.has_access() but I aim to use it anywhere we need to check user permissions. For more informations on the signature of is_authorized, please look at AIP-56 section "Authorization flow".


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

airflow/auth/managers/base_auth_manager.py Outdated Show resolved Hide resolved
airflow/auth/managers/base_auth_manager.py Outdated Show resolved Hide resolved
airflow/auth/managers/fab/fab_auth_manager.py Outdated Show resolved Hide resolved
airflow/auth/managers/fab/fab_auth_manager.py Outdated Show resolved Hide resolved
airflow/auth/managers/models/base_user.py Show resolved Hide resolved
@vincbeck
Copy link
Contributor Author

@potiuk You might be interested into this one if you have time. It is pretty important/impactful since it is defining and updating the authorization model used to check whether a user has permissions to make a specific action on a given resource.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass ...done.

@vincbeck
Copy link
Contributor Author

First pass ...done.

Thank you! 🥳

@vincbeck vincbeck added the use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) label Sep 7, 2023
@vincbeck vincbeck closed this Sep 7, 2023
@vincbeck vincbeck reopened this Sep 7, 2023
@vincbeck
Copy link
Contributor Author

Any comment @potiuk @uranusjr ?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. It also very nicely simplifies the decorators used for views.

@potiuk
Copy link
Member

potiuk commented Sep 22, 2023

Needs resolve conflicts. @uranusjr ?

@vincbeck vincbeck merged commit 556791b into apache:main Sep 25, 2023
43 checks passed
@vincbeck vincbeck deleted the vincbeck/is_authorized branch September 25, 2023 14:13
@@ -35,16 +44,100 @@ def get_access_denied_message():
return conf.get("webserver", "access_denied_message")


def has_access(permissions: Sequence[tuple[str, str]] | None = None) -> Callable[[T], T]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincbeck This is a breaking change for us, we have a plugin that uses this method. Can we please keep this method and work around it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly this is really impossible to keep all the code in case someone uses that code in their own code (DAGs, plugins, ...). This goes back to definition of public interface in Airflow and the security manager is not part of it. However, there is an option in Airflow where you can bring your own security manager using the constant SECURITY_MANAGER_CLASS, even though this is not part of the public interface we did no want to break user experience in case they are using a custom security manager so we handled it. But in your case, I feel like you need to update your code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has_acess has been in use by plugins since Airflow 1.10 - way before we had any formal definition of what read public or not.

In short: this is a breaking change and needs fixing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, in our plugins we also use airflow.www.auth.has_access - if there is any alternative we could migrate before but maybe there are a lot of other plugins and users "in the wild" relying on this. So if the function at least is kept as a proxy to the new implementation it would be good. I also do not know any other alternative for the current main version how to check authentication for plugins if not using this method.

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-56 Extensible user management area:dev-tools area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AIP-56 - FAB AM - Implement is_authorized()
8 participants