Add KeycloakJWTMiddleware to KeycloakAuthManager - #70800
Conversation
87764ef to
40f0199
Compare
|
Why do we need a config here? |
|
I thought I would make the new JWT management an opt-in feature initially, I can take out the config if you don't think its necessary |
|
Yeah I think it would be better without config. That creates complexity |
d34f246 to
526606a
Compare
ba60d88 to
468c907
Compare
potiuk
left a comment
There was a problem hiding this comment.
Reviewed the middleware end to end. The refresh flow reads well, the version-compat guard is used consistently in both source and tests, and the thirteen tests cover the cases I would want — refresh, expiry, missing Airflow JWT, missing Keycloak token, and root-cookie clearing.
I also checked the earlier discussion about the config: that has been handled. There is no provider.yaml change, no CONF_*_KEY, and no toggle wording left, so the opt-in has genuinely gone rather than moved somewhere else.
Three things below, one of which I would like resolved before merge.
Separately, and not inline-able: the PR description is still the unedited template. For a change of this size that renames session cookies in an auth manager, the reasoning behind the rename is exactly what a reviewer needs, and this body becomes the squash commit message and the raw material for the changelog. Worth a few lines.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
468c907 to
bc5806c
Compare
bc5806c to
822e24b
Compare
related: #70550 - adds
access_tokenandrefresh_tokento user correctlyAdd KeycloakJWTMiddleware to KeycloakAuthManager
KeycloakJWTMiddlewareto theKeycloakAuthManagerto extract Keycloak JWT tokens from the request cookies and attach them to the user during auth requests.KeycloakJWTMiddleware, skip refreshing user when called inJWTRefreshMiddlewarewithout theaccess_tokenandrefresh_tokenparametersRename Keycloak JWT session cookies from
access_tokenandrefresh_tokento_access_tokenand_refresh_tokenfor consistency. Since the JWT cookies change is not yet released, this is safe to change.Note: This change will logout any existing sessions for the
KeycloakAuthManager, which should then be handled gracefully by issuing a new Airflow JWT token from the Keycloak login route.Was generative AI tooling used to co-author this PR?