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

Make parameter user mandatory for all methods in the auth manager interface #45986

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

vincbeck
Copy link
Contributor

In Airflow 3 we do not use the session to store the user. Hence, there is no notion of "current user". Today, in the auth manager interface, all is_authorized_* APIs have the parameter user optional. If not provided, the current user is used.

This PR makes the parameter user required in all APIs, I also added the parameter in some APIs where it was missing (batch_is_authorized_* APIs).

Just one API/method in the auth manager will need some updates on that front as well: def filter_permitted_menu_items(self, menu_items: list[MenuItem]) -> list[MenuItem]:. Actually, this API will need multiple updates because:

  1. It uses MenuItem that is a Flask object, we want to get rid of that
  2. User is not part of the list of parameters and it should

This API will be updated in a future PR because I need to sync with the front-end team @bbovenzi and @pierrejeambrun to agree on a signature.


^ 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 area:API Airflow's REST/HTTP API area:providers area:webserver Webserver related Issues provider:amazon AWS/Amazon - related issues provider:fab labels Jan 23, 2025
@vincbeck vincbeck added the legacy api Whether legacy API changes should be allowed in PR label Jan 23, 2025
@vincbeck vincbeck closed this Jan 23, 2025
@vincbeck vincbeck reopened this Jan 23, 2025
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Unrelated to this PR but, ideally we'll want to update the def get_user from the simple auth manager I think.

Currently it uses the flask session object so we cannot use it outside the flask context. We have to manually use the core_api/security.py get_user function that retrieve the user from the token instead, which is working but being able to use the auth_manager interface is maybe more convenient ?

@vincbeck
Copy link
Contributor Author

Unrelated to this PR but, ideally we'll want to update the def get_user from the simple auth manager I think.

Currently is uses the flask session object so we cannot use it in the flask context. We have to manually use the core_api/security.py get_user function that retrieve the user from the token instead, which is working but being able to use the auth_manager interface is maybe more convenient ?

The method get_user from the auth manager will be deleted. This is only used in AF2. We could, as you mentioned and similar to the function get_jwt_token in the auth manager interface, create a function get_user_from_token (or any other name) which basically does what the get_user in core_api/security.py does. It will be a thin wrapper around deserialize_user. I'll create a PR for it

@vincbeck vincbeck force-pushed the vincbeck/am_user_mandatory branch from 8caba96 to ab29ae5 Compare January 27, 2025 15:16
@vincbeck vincbeck merged commit d024cda into apache:main Jan 27, 2025
62 checks passed
@vincbeck vincbeck deleted the vincbeck/am_user_mandatory branch January 27, 2025 15:48
@vincbeck
Copy link
Contributor Author

Unrelated to this PR but, ideally we'll want to update the def get_user from the simple auth manager I think.
Currently is uses the flask session object so we cannot use it in the flask context. We have to manually use the core_api/security.py get_user function that retrieve the user from the token instead, which is working but being able to use the auth_manager interface is maybe more convenient ?

The method get_user from the auth manager will be deleted. This is only used in AF2. We could, as you mentioned and similar to the function get_jwt_token in the auth manager interface, create a function get_user_from_token (or any other name) which basically does what the get_user in core_api/security.py does. It will be a thin wrapper around deserialize_user. I'll create a PR for it

#46135

got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Jan 30, 2025
niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:providers area:webserver Webserver related Issues legacy api Whether legacy API changes should be allowed in PR provider:amazon AWS/Amazon - related issues provider:fab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants