[PR] Resolve the caller from the explicit credential, not the session cookie [airflow]

["potiuk (via GitHub)" <gi...@apache.org>]

potiuk opened a new pull request, #72225:
URL: https://github.com/apache/airflow/pull/72225

   `get_user()` codes an explicit precedence — bearer, then OAuth2, then the
   session cookie:
   
   ```python
   if bearer_credentials and bearer_credentials.scheme.lower() == "bearer":
       token_str = bearer_credentials.credentials
   elif oauth_token:
       token_str = oauth_token
   else:
       token_str = request.cookies.get(COOKIE_NAME_JWT_TOKEN)
   ```
   
   That block was never reached when a session cookie was present. `JWTRefreshMiddleware`
   runs first, resolves a user from the `_token` cookie alone, and stamps it on
   `request.state` together with the trust sentinel; `get_user()` returned that cached
   user up front, before looking at either explicit credential. The effective order on
   every core-API route was therefore **cookie over bearer** — the inverse of what the
   function reads as doing.
   
   So a request carrying both a cookie and an explicit `Authorization: Bearer` token ran
   as the cookie's principal. The token the client deliberately presented was ignored,
   and the request was recorded in the audit log under the wrong identity.
   
   ## The change
   
   The cached user is honoured only when the request carries no explicit credential —
   which is the case it exists for: a browser session whose token the middleware has just
   refreshed. When a bearer or OAuth2 token is present, that token is resolved instead.
   
   The trust-sentinel check is unchanged and still guards the cached-user path; it has
   simply moved inside the no-explicit-credential branch.
   
   ## Behaviour
   
   | Request carries | Resolved as |
   |---|---|
   | cookie only | cookie principal (refreshed by the middleware, as before) |
   | bearer only | bearer principal (as before) |
   | OAuth2 only | OAuth2 principal (as before) |
   | cookie **and** bearer | bearer principal (**changed** — was the cookie) |
   | cookie **and** OAuth2 | OAuth2 principal (**changed** — was the cookie) |
   
   Only the two mixed-credential rows change. A client that sends one credential is
   unaffected, and cookie-only browser sessions keep the refresh behaviour intact.
   
   ## Tests
   
   `test_get_user_explicit_credential_beats_cookie_user`, parametrised over bearer and
   OAuth2: a trusted cookie-derived user is stamped on `request.state` *and* an explicit
   credential is supplied; the explicit one must win. Both fail if the source change is
   reverted.
   
   The existing `test_get_user_with_trusted_request_state` still passes unmodified — it
   supplies no explicit credential, so it exercises the path that was deliberately kept.
   
   140 passed across `core_api/test_security.py` and `auth/middlewares/`; ruff clean.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Resolve the caller from the explicit credential, not the session cookie [airflow]

["pierrejeambrun (via GitHub)" <gi...@apache.org>]

pierrejeambrun commented on code in PR #72225:
URL: https://github.com/apache/airflow/pull/72225#discussion_r3934662153


##########
airflow-core/newsfragments/72225.significant.rst:
##########


Review Comment:
   Newsfragment doesn't mention: invalid bearer + valid cookie now fails loud where it used to silently fall back to cookie. Fine but worth a line.



##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -147,22 +147,26 @@ async def get_user(
     oauth_token: str | None = Depends(oauth2_scheme),
     bearer_credentials: HTTPAuthorizationCredentials | None = Depends(bearer_scheme),
 ) -> BaseUser:
-    # A user might have been already built by a trusted in-tree middleware (currently
-    # only `JWTRefreshMiddleware`); if so, it is stored in `request.state.user` AND
-    # `request.state.user_authenticated_via` is set to the trust sentinel above.
-    # Honour the cached user only when both are present, so a stray `state.user`
-    # assignment from unrelated middleware can't bypass JWT validation.
-    user: BaseUser | None = getattr(request.state, "user", None)
-    trust_marker = getattr(request.state, "user_authenticated_via", None)
-    if user and trust_marker is USER_INJECTED_BY_TRUSTED_MIDDLEWARE:
-        return user
-
+    # An explicitly supplied credential always wins over the ambient session cookie.
     token_str: str | None
     if bearer_credentials and bearer_credentials.scheme.lower() == "bearer":
         token_str = bearer_credentials.credentials
     elif oauth_token:
         token_str = oauth_token
     else:
+        token_str = None

Review Comment:
   This `token_str = None` looks weird.
   
   Early returns eveywhere read better, maybe:
   ```python
     async def get_user(request, oauth_token, bearer_credentials) -> BaseUser:
         if bearer_credentials and bearer_credentials.scheme.lower() == "bearer":
             return await resolve_user_from_token(bearer_credentials.credentials)
         if oauth_token:
             return await resolve_user_from_token(oauth_token)
     
         # No explicit credential — cached middleware user, else cookie.
         user = getattr(request.state, "user", None)
         trust_marker = getattr(request.state, "user_authenticated_via", None)
         if user and trust_marker is USER_INJECTED_BY_TRUSTED_MIDDLEWARE:
             return user
         return await resolve_user_from_token(request.cookies.get(COOKIE_NAME_JWT_TOKEN))
   ```



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Resolve the caller from the explicit credential, not the session cookie [airflow]

["potiuk (via GitHub)" <gi...@apache.org>]

potiuk merged PR #72225:
URL: https://github.com/apache/airflow/pull/72225


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org