-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Update refresh token flow #55506
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
Update refresh token flow #55506
Conversation
5c0cfe3 to
20126d1
Compare
bugraoz93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like the approach. It is simple and extensible. It won't bring any complexity to create new auth managers. Thanks, Vincent!
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py
Show resolved
Hide resolved
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I like this approach too. 👍
20126d1 to
ce67f86
Compare
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the PR!
airflow-core/src/airflow/api_fastapi/auth/middlewares/refresh_token.py
Outdated
Show resolved
Hide resolved
b9ed97b to
fc13ede
Compare
fc13ede to
e8aff0c
Compare
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just one remark
8503714 to
22cba85
Compare
|
#56633 was merged so I rebased this PR to only include the refresh token change. |
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The refresh token flow gets updated to make it simpler. Auth managers that need to refresh token periodically (e.g. Keycloak), need to implement the method
refresh_token. This method is called before every request and check whether the token needs to be refreshed. If so, the token gets refreshed, is passed to the route implementation and saved as part of the response as cookie. This happens silently without the user noticing. Hence, we no longer need to have a refresh token public API.^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.