-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Fix logout route in Keycloak provider so a KeycloakostError doesn't lead to Internal Server Error in API server #59382
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
Fix logout route in Keycloak provider so a KeycloakostError doesn't lead to Internal Server Error in API server #59382
Conversation
…ostError doesn't propagate to API server also which leads to Internal Server Error
|
CI is unhappy :( |
|
I've put this PR in draft because I still need to add a test for the logout route |
Yes it's still WIP but I wanted to already start the PR so we know it's being handled ;-) |
…en refresh_token is being invoked in logout route
|
Test added |
…akAuthManagerUser
vincbeck
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.
One nit, overall, looks good
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py
Outdated
Show resolved
Hide resolved
…/keycloak_auth_manager.py Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
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.
NICE!
…ead to Internal Server Error in API server (apache#59382) * refactor: Fix logout route in Keycloak provider also so the KeycloakPostError doesn't propagate to API server also which leads to Internal Server Error * refactor: Fixed static checks * refactor: Fixed refresh_token invocations * refactor: Must call refresh_user in refresh route * refactor: refresh_token must always return a dict * refactor: Added test when keycloak client raises KeycloakPostError when refresh_token is being invoked in logout route * refactor: Fixed some additional static checks * refactor: Refactored refresh_user * refactor: Reformatted imports * refactor: Fixed mocking in refresh test * refactor: Removed unused mocking of keycloak client in test_refresh_token * refactor: Fixed mock get_auth_manager and added missing import KeycloakAuthManagerUser * refactor: Refresh token route calls refresh_user instead of refresh_token * refactor: Changed assert on refresh user * Update providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * refactor: Fixed calls to refresh_tokens instead of refresh_token --------- Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
…ead to Internal Server Error in API server (apache#59382) * refactor: Fix logout route in Keycloak provider also so the KeycloakPostError doesn't propagate to API server also which leads to Internal Server Error * refactor: Fixed static checks * refactor: Fixed refresh_token invocations * refactor: Must call refresh_user in refresh route * refactor: refresh_token must always return a dict * refactor: Added test when keycloak client raises KeycloakPostError when refresh_token is being invoked in logout route * refactor: Fixed some additional static checks * refactor: Refactored refresh_user * refactor: Reformatted imports * refactor: Fixed mocking in refresh test * refactor: Removed unused mocking of keycloak client in test_refresh_token * refactor: Fixed mock get_auth_manager and added missing import KeycloakAuthManagerUser * refactor: Refresh token route calls refresh_user instead of refresh_token * refactor: Changed assert on refresh user * Update providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * refactor: Fixed calls to refresh_tokens instead of refresh_token --------- Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
This PR is also related to issue #59359.
Problem is during the logout route defined in the Keycloak provider is that there the refresh_token is also called directly on the Keycloak client. When a KeycloakPostError is raised when the refresh token fails, the error will also be raised and this will lead to an HTTP 500 Internal Server Error in the API server.
So I extracted a refresh_token method from the refresh_user method in the KeycloakAuthManager so the refresh_token method is guarded and thus catches the KeycloakPostError, that way I can also re-use that method in the logout route so that when an exception occurs the API server doesn't return an HTTP 500 Internal Server Error.
^ 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.