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

Add back decorator has_access #34786

Merged
merged 8 commits into from
Oct 12, 2023
Merged

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Oct 5, 2023

See comment thread.

has_access decorator seems to be used by many plugins. Hence, in order to avoid breaking them. Adding back this function.

@utkarsharma2, @ashb, @jens-scheffler-bosch


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

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Oct 5, 2023
airflow/www/auth.py Outdated Show resolved Hide resolved
@utkarsharma2
Copy link
Contributor

utkarsharma2 commented Oct 5, 2023

@vincbeck @ashb Should we also update the public interface document to include has_access? Maybe as a separate PR.

@vincbeck
Copy link
Contributor Author

vincbeck commented Oct 5, 2023

@vincbeck @ashb Should we also update the public interface document to include has_access? Maybe as a separate PR.

I would definitely say yes. I would do it in a separate PR though, mainly because I dont really know how to explain besides saying: "Many plugins are using this decorator historically" :D One of you want to take a chance at it?

@hussein-awala
Copy link
Member

@vincbeck @ashb Should we also update the public interface document to include has_access? Maybe as a separate PR.

I would definitely say yes. I would do it in a separate PR though, mainly because I dont really know how to explain besides saying: "Many plugins are using this decorator historically" :D One of you want to take a chance at it?

I don't have a strong opinion on this, but as I understood in the original discussion, this decorator should not be in the public interface, but just maintain it because it was used since Airflow 1 for plugins, and we kept it in Airflow 2.
Adding a deprecated method to the public interface doesn't make much sense for me.

@vincbeck
Copy link
Contributor Author

vincbeck commented Oct 6, 2023

@vincbeck @ashb Should we also update the public interface document to include has_access? Maybe as a separate PR.

I would definitely say yes. I would do it in a separate PR though, mainly because I dont really know how to explain besides saying: "Many plugins are using this decorator historically" :D One of you want to take a chance at it?

I don't have a strong opinion on this, but as I understood in the original discussion, this decorator should not be in the public interface, but just maintain it because it was used since Airflow 1 for plugins, and we kept it in Airflow 2. Adding a deprecated method to the public interface doesn't make much sense for me.

That's true too ... I guess we are in some kind of grey area here. But what you said makes sense to me

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Thanks for the PR adding this back. I assume just one pydoc issue. Otherwise code looks promising.

Regarding the public API my opinion is that the code properly adds a deprecation warning and points to the new API - which we then should declare as public. Agree that the previous API was a gray area but for the proposed new entry points at least we should make it better.

What I did not fully get with the new API methods are, these are specific to the UI areas. If I add a MyPlugin then I would need to add a has_access_my_plugin() decorator, the ones existing are specific to the existing UI.
So besides public API would be good to add some hints into the plugin authoring docs - If you don't want to raise a PR I could if you help me in understanding.

airflow/www/auth.py Outdated Show resolved Hide resolved
airflow/www/auth.py Outdated Show resolved Hide resolved
vincbeck and others added 3 commits October 10, 2023 12:50
Co-authored-by: Jens Scheffler <95105677+jens-scheffler-bosch@users.noreply.github.com>
@vincbeck
Copy link
Contributor Author

What I did not fully get with the new API methods are, these are specific to the UI areas. If I add a MyPlugin then I would need to add a has_access_my_plugin() decorator, the ones existing are specific to the existing UI.

Yes it can be an option. If you implement MyPlugin, then you can implement has_access_my_plugin() which checks whether the user has permissions to access the view provided by your plugin. In a following PR of #34317 (meaning: whenever #34317 is merged I'll be able to create another PR), I implement a mechanism to check custom permissions. By custom permissions I mean permissions defined outside of Airflow codebase (which is the case of plugins). As part of this PR I'll be able to implement also a has_access_custom() decorator which then can be used in such use case. Stay tuned :)

So besides public API would be good to add some hints into the plugin authoring docs - If you don't want to raise a PR I could if you help me in understanding.

Yes, happy to work together on this one. Please feel free to create a PR and tag me on it and I'll be able to review it. Or you can ping me on Slack if you want

@vincbeck vincbeck added use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) and removed use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) labels Oct 10, 2023
@jscheffl
Copy link
Contributor

So besides public API would be good to add some hints into the plugin authoring docs - If you don't want to raise a PR I could if you help me in understanding.

Yes, happy to work together on this one. Please feel free to create a PR and tag me on it and I'll be able to review it. Or you can ping me on Slack if you want

Promise to contribute this after merging this PR...

@vincbeck
Copy link
Contributor Author

@utkarsharma2, @ashb, @hussein-awala Any comments?

@utkarsharma2
Copy link
Contributor

@vincbeck Thanks for adding it back :) changes look good to me.

@potiuk potiuk merged commit 84941f8 into apache:main Oct 12, 2023
43 checks passed
@vincbeck vincbeck deleted the vincbeck/has_access branch October 23, 2023 18:01
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:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants