dabla commented on code in PR #76353:
URL: https://github.com/apache/airflow/pull/76353#discussion_r4477811681
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -223,8 +223,10 @@ async def get_user_from_token(
# the cookie path those claims are covered by the Airflow JWT
signature, so they
# need no separate subject check. A browser token does not carry them
and can
# only be completed by KeycloakJWTMiddleware from the cookies; without
them
- # there is nothing to authorize against.
- return user if user.access_token else None
+ # there is nothing to authorize against, so the token is rejected.
+ if not user.access_token:
Review Comment:
You're right, thanks. The core `JWTRefreshMiddleware` resolves the bare
`_token` on every request without the Keycloak cookies, so raising there turned
every request into an invalidated session and cleared the cookie after the
first one.
So the `None` stays (with a comment saying why), and the fix moved to
`KeycloakJWTMiddleware`: once `_refresh_user` fails it already clears `_token`
on the response, and now it also removes `_token` from the request it passes
on. `get_user` then answers 401 instead of handing `None` to the authorization
checks, and the core middleware passes the request through. The new tests run
both middlewares as `init_middlewares` installs them: a session with its
Keycloak tokens keeps its cookie (that one fails on the previous commit), and
an expired refresh token or a missing access token cookie answers 401 and
clears it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
