LCORE-1210: Missing JWK auth header correctly returns 401 error#1344
Merged
tisnik merged 1 commit intoMar 18, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAuthorization handling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
asimurka
force-pushed
the
fix_jwk_missing_auth_header_returns_403
branch
from
ca9f102 to
d92f6db
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/authentication/jwk_token.py (2)
19-19:⚠️ Potential issue | 🟠 MajorRemove unused
NO_AUTH_TUPLEimport to unblock CI.Ruff/Pylint are failing because
NO_AUTH_TUPLEis no longer used after switching to exception-based handling for missing Authorization headers.Suggested fix
-from authentication.interface import NO_AUTH_TUPLE, AuthInterface, AuthTuple +from authentication.interface import AuthInterface, AuthTuple🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/jwk_token.py` at line 19, Remove the unused import NO_AUTH_TUPLE from the import statement in jwk_token.py (currently importing NO_AUTH_TUPLE, AuthInterface, AuthTuple); update the import to only import AuthInterface and AuthTuple so the module no longer imports an unused symbol and linter/CI errors are resolved.
166-180:⚠️ Potential issue | 🟡 MinorUpdate
__call__docstring to match the new missing-header behavior.The docstring still says missing Authorization returns
NO_AUTH_TUPLE, but Line 183-184 now raisesHTTPException(401).Suggested fix
- If the Authorization header is missing, returns NO_AUTH_TUPLE. On token + If the Authorization header is missing, raises HTTPException(401). On token verification or validation failures this function raises HTTPException with appropriate HTTP status codes: @@ - AuthTuple: A tuple (user_id, username, skip_userid_check, token) - extracted from the validated token, or NO_AUTH_TUPLE when no - Authorization header is present. + AuthTuple: A tuple (user_id, username, skip_userid_check, token) + extracted from the validated token. + + Raises: + HTTPException: 401 when Authorization header is missing or token is invalid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/authentication/jwk_token.py` around lines 166 - 180, The docstring for __call__ is out of sync: it still states that a missing Authorization header returns NO_AUTH_TUPLE but the implementation now raises an HTTPException(401). Update the __call__ docstring (parameters/behavior/returns) to state that when the Authorization header is missing an HTTPException with status 401 is raised, and update the Returns section to reflect that NO_AUTH_TUPLE is no longer returned on missing header (or clarify when NO_AUTH_TUPLE is returned if still applicable); reference the __call__ method and the HTTPException(401) behavior so the documentation matches the code paths for missing header, verification failures, and other error statuses.tests/unit/authentication/test_jwk_token.py (1)
16-16:⚠️ Potential issue | 🟠 MajorRemove unused constants import to fix lint failures.
DEFAULT_USER_NAME,DEFAULT_USER_UID, andNO_USER_TOKENare no longer used and currently fail Ruff/Pylint.Suggested fix
-from constants import DEFAULT_USER_NAME, DEFAULT_USER_UID, NO_USER_TOKEN🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/authentication/test_jwk_token.py` at line 16, The test_jwk_token.py file imports three unused constants (DEFAULT_USER_NAME, DEFAULT_USER_UID, NO_USER_TOKEN) which cause lint failures; remove those symbols from the import statement so only actually used names are imported (or remove the entire import if none are used) and run the linter to confirm; specifically edit the import line that currently references DEFAULT_USER_NAME, DEFAULT_USER_UID, and NO_USER_TOKEN to drop them.
🧹 Nitpick comments (1)
tests/unit/authentication/test_jwk_token.py (1)
443-447: Prefer structured detail assertions over substring matching.Line 447 currently checks a stringified dict; asserting exact
detail["cause"]/detail["response"]is more stable and clearer.Suggested refactor
with pytest.raises(HTTPException) as exc_info: await dependency(no_token_request) assert exc_info.value.status_code == 401 - assert "No Authorization header found" in str(exc_info.value.detail) + detail = cast(dict[str, str], exc_info.value.detail) + assert detail["response"] == "Missing or invalid credentials provided by client" + assert detail["cause"] == "No Authorization header found"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/authentication/test_jwk_token.py` around lines 443 - 447, Replace the substring assertion on the stringified HTTPException detail with structured assertions: after raising via await dependency(no_token_request) and capturing exc_info, assert exc_info.value.status_code == 401, then treat exc_info.value.detail as a dict and assert exact values for the relevant keys (e.g., detail["cause"] == "<expected cause>" and detail["response"] == "<expected response>") rather than using "in str(...)" so tests on dependency and no_token_request validate concrete detail["cause"]/detail["response"] values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/authentication/jwk_token.py`:
- Line 19: Remove the unused import NO_AUTH_TUPLE from the import statement in
jwk_token.py (currently importing NO_AUTH_TUPLE, AuthInterface, AuthTuple);
update the import to only import AuthInterface and AuthTuple so the module no
longer imports an unused symbol and linter/CI errors are resolved.
- Around line 166-180: The docstring for __call__ is out of sync: it still
states that a missing Authorization header returns NO_AUTH_TUPLE but the
implementation now raises an HTTPException(401). Update the __call__ docstring
(parameters/behavior/returns) to state that when the Authorization header is
missing an HTTPException with status 401 is raised, and update the Returns
section to reflect that NO_AUTH_TUPLE is no longer returned on missing header
(or clarify when NO_AUTH_TUPLE is returned if still applicable); reference the
__call__ method and the HTTPException(401) behavior so the documentation matches
the code paths for missing header, verification failures, and other error
statuses.
In `@tests/unit/authentication/test_jwk_token.py`:
- Line 16: The test_jwk_token.py file imports three unused constants
(DEFAULT_USER_NAME, DEFAULT_USER_UID, NO_USER_TOKEN) which cause lint failures;
remove those symbols from the import statement so only actually used names are
imported (or remove the entire import if none are used) and run the linter to
confirm; specifically edit the import line that currently references
DEFAULT_USER_NAME, DEFAULT_USER_UID, and NO_USER_TOKEN to drop them.
---
Nitpick comments:
In `@tests/unit/authentication/test_jwk_token.py`:
- Around line 443-447: Replace the substring assertion on the stringified
HTTPException detail with structured assertions: after raising via await
dependency(no_token_request) and capturing exc_info, assert
exc_info.value.status_code == 401, then treat exc_info.value.detail as a dict
and assert exact values for the relevant keys (e.g., detail["cause"] ==
"<expected cause>" and detail["response"] == "<expected response>") rather than
using "in str(...)" so tests on dependency and no_token_request validate
concrete detail["cause"]/detail["response"] values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed7f9638-a3e8-4cd6-b8cf-7405607a2531
📒 Files selected for processing (3)
src/authentication/jwk_token.pytests/e2e/features/rbac.featuretests/unit/authentication/test_jwk_token.py
asimurka
force-pushed
the
fix_jwk_missing_auth_header_returns_403
branch
from
d92f6db to
659c6e9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/authentication/test_jwk_token.py (1)
437-446: Assert the structured 401 payload explicitly, not via string matching.
str(exc_info.value.detail)is a loose check. This test should validate the response contract fields directly (responseandcause) to prevent silent regressions.✅ Suggested test assertion tightening
with pytest.raises(HTTPException) as exc_info: await dependency(no_token_request) assert exc_info.value.status_code == 401 - assert "No Authorization header found" in str(exc_info.value.detail) + detail = cast(dict[str, str], exc_info.value.detail) + assert detail["response"] == "Missing or invalid credentials provided by client" + assert detail["cause"] == "No Authorization header found"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/authentication/test_jwk_token.py` around lines 437 - 446, Replace the loose string check on str(exc_info.value.detail) with explicit assertions against the structured error payload returned by JwkTokenAuthDependency: after asserting status_code == 401, assert exc_info.value.detail is a dict (or has attributes) and then check exc_info.value.detail["response"] equals the expected message (e.g., "No Authorization header found") and exc_info.value.detail["cause"] equals the expected cause (or is None/empty as appropriate); reference the JwkTokenAuthDependency, default_jwk_configuration, and no_token_request variables to locate the failing case and tighten the assertions on the response contract rather than using string matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/authentication/jwk_token.py`:
- Around line 183-184: The __call__ method's docstring in jwk_token.py is
outdated: it claims the function returns NO_AUTH_TUPLE but the code now raises
HTTPException via UnauthorizedResponse when the Authorization header is missing;
update the __call__ docstring to describe the new behavior (raise HTTPException
with UnauthorizedResponse details on missing/invalid Authorization header) and
mention any returned types when successful, referencing the __call__ method name
and the UnauthorizedResponse/HTTPException behavior so the docstring matches the
implementation.
---
Nitpick comments:
In `@tests/unit/authentication/test_jwk_token.py`:
- Around line 437-446: Replace the loose string check on
str(exc_info.value.detail) with explicit assertions against the structured error
payload returned by JwkTokenAuthDependency: after asserting status_code == 401,
assert exc_info.value.detail is a dict (or has attributes) and then check
exc_info.value.detail["response"] equals the expected message (e.g., "No
Authorization header found") and exc_info.value.detail["cause"] equals the
expected cause (or is None/empty as appropriate); reference the
JwkTokenAuthDependency, default_jwk_configuration, and no_token_request
variables to locate the failing case and tighten the assertions on the response
contract rather than using string matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d632282f-a0dc-4661-9142-a84900c602d3
📒 Files selected for processing (3)
src/authentication/jwk_token.pytests/e2e/features/rbac.featuretests/unit/authentication/test_jwk_token.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/features/rbac.feature
asimurka
force-pushed
the
fix_jwk_missing_auth_header_returns_403
branch
from
659c6e9 to
e942022
Compare
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
3 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.
Description
This PR fixes bug in JWK auth that previously returned 403 Forbidden error instead of 401 Unauthorized.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit