Fix AUTH_ROLE_PUBLIC returning 401 in FastAPI API server#69773
Open
lyric2249 wants to merge 3 commits into
Open
Fix AUTH_ROLE_PUBLIC returning 401 in FastAPI API server#69773lyric2249 wants to merge 3 commits into
lyric2249 wants to merge 3 commits into
Conversation
FabAuthRolePublicMiddleware only sets request.state.user for anonymous requests. Core's get_user() dependency also checks request.state.user_authenticated_via for the trusted-middleware sentinel before skipping JWT validation, so every anonymous request falls through to JWT validation, has no token, and is rejected with 401 regardless of AUTH_ROLE_PUBLIC (or [fab] auth_role_public) being configured correctly. Set the same trust marker JWTRefreshMiddleware already sets when it pre-authenticates a request, so both middlewares honor the same contract.
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
The FAB provider supports Airflow 3.0.2 and later, while USER_INJECTED_BY_TRUSTED_MIDDLEWARE is only available in newer core releases. Importing it unconditionally prevents the provider from loading and causes test collection to fail against supported versions such as Airflow 3.0.6. Preserve trusted middleware authentication on newer core versions without requiring older supported versions to expose the marker. r Airflow versions
SameerMesiah97
left a comment
SameerMesiah97
left a comment
Contributor
There was a problem hiding this comment.
I have left a comment.
| if public_user is not None: | ||
| request.state.user = public_user | ||
|
|
||
| if _USER_INJECTED_BY_TRUSTED_MIDDLEWARE is not None: |
Contributor
There was a problem hiding this comment.
I don't see the need for the _USER_INJECTED_BY_TRUSTED_MIDDLEWARE if it is only being used once. It would be better to in-line it like this:
user_injected = getattr(
core_api_security,
"USER_INJECTED_BY_TRUSTED_MIDDLEWARE",
None,
)
if user_injected is not None:
request.state.user_authenticated_via = user_injected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FabAuthRolePublicMiddleware(added in #65685) setsrequest.state.userfor anonymous requests whenAUTH_ROLE_PUBLIC/[fab] auth_role_publicis configured, but never setsrequest.state.user_authenticated_via. Core'sget_user()dependency requires both attributes before it will trust a pre-set user and skip JWT validation, so anonymous requests still fall through to JWT validation and get rejected with 401 even though the public role is configured correctly.This sets the same trust marker
JWTRefreshMiddlewarealready sets, so both middlewares satisfy the same contract thatget_user()checks.The existing
test_sets_public_user_when_no_auth_presenttest already exercises this exact code path, so it is extended with one assertion for the new attribute rather than adding a separate test; no existing assertions were changed or removed.related: #60897
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Sonnet 5) following the guidelines