LCORE-287: update documentation for authorized endpoint to align with openshift #505
Conversation
WalkthroughExpands AuthTuple to include a boolean skip_userid_check and propagates it across auth dependencies, endpoints, models, and tests. Updates K8s token extraction (Bearer header) and OpenAPI/docs to require and document AuthorizedResponse.skip_userid_check and refine 400/401 descriptions for credential-related endpoints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as /authorized Endpoint
participant Auth as Auth Dependency
participant Models as AuthorizedResponse
Client->>API: POST /authorized
API->>Auth: __call__(request)
alt Auth success
Auth-->>API: (user_id, username, skip_userid_check, token)
API->>Models: Build AuthorizedResponse(user_id, username, skip_userid_check)
Models-->>API: AuthorizedResponse JSON
API-->>Client: 200 AuthorizedResponse
else Invalid credentials (noop / noop-with-token)
API-->>Client: 400 UnauthorizedResponse (JSON)
else Unauthorized (k8s)
API-->>Client: 401 UnauthorizedResponse (JSON)
end
sequenceDiagram
autonumber
participant Client
participant K8s as K8SAuthDependency
participant K8API as Kubernetes API
Client->>K8s: Request (Authorization: Bearer <token>)
alt Missing/invalid header or token
K8s-->>Client: 401 Unauthorized
else Valid token
K8s->>K8API: TokenReview(token)
K8API-->>K8s: user(uid, username)
opt username == "kube:admin"
K8s->>K8API: get cluster id
K8API-->>K8s: cluster_id
K8s->>K8s: uid = cluster_id
end
K8s->>K8API: SubjectAccessReview
K8API-->>K8s: allowed/denied
K8s-->>Client: (uid, username, skip_userid_check=false, token)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/endpoints/query.py (1)
311-321: Bug: wrong tuple returned when falling back to the first available LLM.This returns
(model_id, model_id, provider_id)instead of(llama_stack_model_id, model_id, provider_id), breaking downstream expectations.- logger.info("Selected model: %s", model) - return model_id, model_id, provider_id + logger.info("Selected model: %s", model) + llama_stack_model_id = f"{provider_id}/{model_id}" + return llama_stack_model_id, model_id, provider_idsrc/auth/noop_with_token.py (1)
5-10: Docstring is outdated: return shape still shows 3-tuplePlease update to reflect the 4-tuple and the skip_userid_check semantics.
Apply:
- - Returns a tuple: (user_id, DEFAULT_USER_NAME, user_token). + - Returns an AuthTuple: (user_id, DEFAULT_USER_NAME, True, user_token), + where the third element (skip_userid_check) indicates user_id validation + was intentionally skipped in this no-op auth flow.src/auth/k8s.py (1)
167-185: Fix type annotation and guard against missing status in TokenReviewThe function returns response.status (V1TokenReviewStatus), not V1TokenReview. Also guard when status is None to avoid AttributeError.
-def get_user_info(token: str) -> Optional[kubernetes.client.V1TokenReview]: +def get_user_info(token: str) -> Optional[kubernetes.client.V1TokenReviewStatus]: @@ - Returns: - The user information if the token is valid, None otherwise. + Returns: + The user information (V1TokenReviewStatus) if the token is valid and authenticated, else None. @@ - response = auth_api.create_token_review(token_review) - if response.status.authenticated: - return response.status - return None + response = auth_api.create_token_review(token_review) + status = getattr(response, "status", None) + if status and getattr(status, "authenticated", False): + return status + return None
🧹 Nitpick comments (33)
src/constants.py (1)
34-34: Confirm default semantics for skip_userid_check (security-impacting).Setting DEFAULT_SKIP_USER_ID_CHECK = True means user_id verification is skipped by default. Please confirm this is intentional for all default flows (e.g., noop) and that k8s/JWK paths explicitly override to False. Consider in-line documentation to avoid misuse.
Apply this small doc tweak:
-DEFAULT_SKIP_USER_ID_CHECK = True +DEFAULT_SKIP_USER_ID_CHECK = True # True => service skips enforcing user_id verification (noop defaults). k8s/JWK should set False.docs/output.md (2)
400-401: Tighten phrasing of 400/401 messages.Use consistent, compact wording across docs:
-| 400 | Missing or invalid credentials provided by client for noop and noop-with-token | [UnauthorizedResponse](#unauthorizedresponse) | -| 401 | Missing or invalid credentials provided by client for k8s | [UnauthorizedResponse](#unauthorizedresponse) | +| 400 | Missing or invalid client credentials (noop, noop-with-token) | [UnauthorizedResponse](#unauthorizedresponse) | +| 401 | Missing or invalid client credentials (k8s) | [UnauthorizedResponse](#unauthorizedresponse) |
513-521: Clarify wording and type consistency for skip_userid_check.
- Prefer “Whether to skip the user_id check.”
- Use “boolean” to match the rest of the doc.
- skip_userid_check: Whether to skip user_id check + skip_userid_check: Whether to skip the user_id check ... -| skip_userid_check | bool | skip user_id check | +| skip_userid_check | boolean | Whether to skip the user_id check |docs/openapi.md (2)
400-401: Match 400/401 wording with output.md for consistency.-| 400 | Missing or invalid credentials provided by client for noop and noop-with-token | [UnauthorizedResponse](#unauthorizedresponse) | -| 401 | Missing or invalid credentials provided by client for k8s | [UnauthorizedResponse](#unauthorizedresponse) | +| 400 | Missing or invalid client credentials (noop, noop-with-token) | [UnauthorizedResponse](#unauthorizedresponse) | +| 401 | Missing or invalid client credentials (k8s) | [UnauthorizedResponse](#unauthorizedresponse) |
510-520: Add skip_userid_check to the Attributes block (currently missing).The fields table includes skip_userid_check, but the Attributes list omits it. Please add:
Attributes: user_id: The ID of the logged in user. username: The name of the logged in user. skip_userid_check: Whether to skip the user_id check.docs/openapi.json (2)
692-703: Make 400/401 descriptions concise and consistent with Markdown docs.- "description": "Missing or invalid credentials provided by client for the noop and noop-with-token authentication modules", + "description": "Missing or invalid client credentials (noop, noop-with-token)", ... - "description": "Missing or invalid credentials provided by client for the k8s authentication module", + "description": "Missing or invalid client credentials (k8s)",
935-956: Polish property title/copy and ensure model description mentions the new field.
- Title casing: “ID” (not “Id”).
- Description: include definitive article “the”.
- The AuthorizedResponse “description” block still lists only user_id and username; please add skip_userid_check there (likely via src/models/responses.py docstring).
- "title": "Skip User Id Check", - "description": "Whether to skip the user ID check", + "title": "Skip User ID Check", + "description": "Whether to skip the user_id check",Follow-up: Update the model docstring so the generated description includes skip_userid_check in “Attributes”. I can open a PR snippet for src/models/responses.py if helpful.
tests/unit/app/endpoints/test_authorized.py (1)
21-21: Add a False-case to cover non-noop providersConsider an extra test where auth has skip_userid_check=False to cover JWK/K8s paths.
Example (add outside this block):
@pytest.mark.asyncio async def test_authorized_endpoint_skip_false(): resp = await authorized_endpoint_handler(auth=("id", "user", False, "token")) assert resp.model_dump()["skip_userid_check"] is Falsetests/unit/app/endpoints/test_feedback.py (1)
85-86: Deduplicate auth tuple setupIntroduce a module-level AUTH_TEST_TUPLE or a pytest fixture to avoid repetition and ease future shape changes.
Example (add near imports):
AUTH_TEST_TUPLE = ("test_user_id", "test_username", False, "test_token")Then replace the four call sites with
auth=AUTH_TEST_TUPLE.Also applies to: 112-113, 207-208, 224-225
tests/unit/authorization/test_resolvers.py (1)
53-54: Fix tuple element ordering; second element should be username (not the string 'token')Align tests with AuthTuple = (user_id, username, skip_userid_check, token) to prevent confusion and accidental pattern copy.
Apply this diff:
- auth = ("user", "token", False, claims_to_token(jwt_claims)) + auth = ("user", "username", False, claims_to_token(jwt_claims)) @@ - auth = ("user", "token", False, claims_to_token(jwt_claims)) + auth = ("user", "username", False, claims_to_token(jwt_claims)) @@ - auth = ("user", "token", False, claims_to_token(jwt_claims)) + auth = ("user", "username", False, claims_to_token(jwt_claims)) @@ - auth = ("user", "token", False, claims_to_token(jwt_claims)) + auth = ("user", "username", False, claims_to_token(jwt_claims)) @@ - auth = ("user", "token", False, claims_to_token(jwt_claims)) + auth = ("user", "username", False, claims_to_token(jwt_claims))Additionally, update the nearby comments that still say “third element” to “fourth element” to match the new layout.
Optional: consider a small helper or NamedTuple/TypedDict for AuthTuple in tests to make ordering explicit and reduce mistakes.
Also applies to: 77-78, 100-101, 123-124, 169-170
tests/e2e/features/authorized_noop.feature (1)
18-19: Use JSON booleans (true/false) instead of Python-style TrueAvoid relying on a tolerant parser in step helpers; keep expected bodies strictly JSON.
Apply this diff:
- {"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user","skip_userid_check": True} + {"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user","skip_userid_check": true} @@ - {"user_id": "test_user","username": "lightspeed-user","skip_userid_check": True} + {"user_id": "test_user","username": "lightspeed-user","skip_userid_check": true} @@ - {"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user","skip_userid_check": True} + {"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user","skip_userid_check": true} @@ - {"user_id": "","username": "lightspeed-user","skip_userid_check": True} + {"user_id": "","username": "lightspeed-user","skip_userid_check": true} @@ - {"user_id": "test_user","username": "lightspeed-user","skip_userid_check": True} + {"user_id": "test_user","username": "lightspeed-user","skip_userid_check": true}If the step implementation already normalizes Python booleans, this is a style cleanup; if not, it prevents brittle comparisons.
Also applies to: 27-28, 37-38, 47-48, 57-58
src/app/endpoints/feedback.py (2)
113-113: AuthTuple unpack matches 4-tuple — prefer named throwaways for clarity.- user_id, _, _, _ = auth + user_id, _username, _skip_userid_check, _token = auth
198-198: Same here: make ignored elements explicit.- user_id, _, _, _ = auth + user_id, _username, _skip_userid_check, _token = authtests/unit/auth/test_noop.py (1)
16-39: Tests aligned with 4-tuple — consider parametrizing to reduce duplication.Example:
@@ -async def test_noop_auth_dependency(): - ... - user_id, username, skip_userid_check, user_token = await dependency(request) - ... - -async def test_noop_auth_dependency_custom_user_id(): - ... - user_id, username, skip_userid_check, user_token = await dependency(request) - ... +import pytest + +@pytest.mark.parametrize( + "query,expected_user_id", + [(b"", DEFAULT_USER_UID), (b"user_id=test-user", "test-user")], +) +async def test_noop_auth_dependency_variants(query, expected_user_id): + dependency = NoopAuthDependency() + request = Request(scope={"type": "http", "query_string": query}) + user_id, username, skip_userid_check, user_token = await dependency(request) + assert user_id == expected_user_id + assert username == DEFAULT_USER_NAME + assert skip_userid_check is True + assert user_token == NO_USER_TOKENsrc/authorization/resolvers.py (1)
84-96: Unpack fix LGTM — add guard and error handling for malformed/empty tokens.Prevents crashes on unexpected token shapes and surfaces a clear error.
- _, _, _, token = auth - if token == constants.NO_USER_TOKEN: + _, _, _, token = auth + if not token or token == constants.NO_USER_TOKEN: # No claims for guests return {} - - jwt_claims = unsafe_get_claims(token) + try: + jwt_claims = unsafe_get_claims(token) + except Exception as exc: + raise RoleResolutionError("Invalid authentication token") from excsrc/auth/noop_with_token.py (2)
35-35: Use shared constant for skip flag; note: virtual_path unusedPrefer the repo-wide constant over a literal for consistency; also consider documenting or removing virtual_path if it’s not used.
Apply:
- self.skip_userid_check = True + self.skip_userid_check = DEFAULT_SKIP_USER_ID_CHECKAlso add to imports (outside this hunk):
from constants import ( DEFAULT_USER_NAME, DEFAULT_USER_UID, DEFAULT_VIRTUAL_PATH, DEFAULT_SKIP_USER_ID_CHECK, # new )
37-46: Annotate with AuthTuple and fix return docstringKeep type annotations consistent with the interface alias and clarify the return docs.
Apply:
- async def __call__(self, request: Request) -> tuple[str, str, bool, str]: + async def __call__(self, request: Request) -> AuthTuple: @@ - Returns: - The user's UID and username if authentication and authorization succeed - user_id check is skipped with noop auth to allow consumers provide user_id + Returns: + AuthTuple: (user_id, username, skip_userid_check, token). + For this dependency, skip_userid_check is True to allow clients to provide user_id.And update imports (outside this hunk):
from auth.interface import AuthInterface, AuthTupletests/unit/auth/test_noop_with_token.py (1)
25-31: LGTM: asserts align with new 4-tupleUnpacking and asserting skip_userid_check=True look correct.
You can parametrize the default vs custom user_id cases to reduce duplication.
tests/unit/models/test_responses.py (1)
58-63: LGTM: model updated to include skip_userid_checkConsider also exercising the default value path if the field is optional.
Example:
ar = AuthorizedResponse(user_id="u", username="n") assert isinstance(ar.skip_userid_check, bool)src/app/endpoints/streaming_query.py (1)
559-559: LGTM: unpack updated AuthTupleMinor nit: since skip flag is unused here,
user_id, _, _, token = authwould also suffice.src/auth/jwk_token.py (1)
121-121: Annotate the new attribute for clarity.Add a type annotation to make intent explicit.
- self.skip_userid_check = False + self.skip_userid_check: bool = Falsetests/unit/app/endpoints/test_streaming_query.py (1)
50-50: 4‑element AuthTuple adoption and token propagation verified — LGTM.Consider centralizing MOCK_AUTH in a shared test util to avoid duplication across endpoint tests, and parameterize skip_userid_check (True/False) to assert it doesn’t affect endpoint behavior.
Also applies to: 1321-1321
tests/unit/app/endpoints/test_query.py (1)
34-34: AuthTuple migration (4 elements) across tests — LGTM.Same note as streaming tests: centralize MOCK_AUTH and consider parametrizing skip_userid_check for coverage symmetry.
Also applies to: 111-112, 1217-1218
tests/unit/auth/test_k8s.py (1)
106-136: Add missing-header test case
Add a unit test for K8SAuthDependency that verifies when the Authorization header is absent, it raises an HTTPException with status code 401, matching the documented behavior.src/auth/interface.py (1)
5-6: Docstring still mentions 3‑tuple; update to 4‑tuple.-where `AuthTuple = (UserID, UserName, Token)`. +where `AuthTuple = (UserID, UserName, SkipUserIdCheck, Token)`.src/auth/noop.py (2)
24-24: Prefer constant over literal for skip flagUse DEFAULT_SKIP_USER_ID_CHECK to avoid drift with project-wide defaults.
- self.skip_userid_check = True + self.skip_userid_check = DEFAULT_SKIP_USER_ID_CHECKAlso update the import list accordingly:
from constants import ( DEFAULT_USER_NAME, DEFAULT_USER_UID, NO_USER_TOKEN, DEFAULT_VIRTUAL_PATH, DEFAULT_SKIP_USER_ID_CHECK, )
26-35: Tighten return docstring to reflect 4‑tuple contractClarify tuple shape and semantics for downstream readers/tools.
Returns: - The user's UID and username if authentication and authorization succeed - user_id check is skipped with noop auth to allow consumers provide user_id + tuple[str, str, bool, str]: (user_id, username, skip_userid_check, token). + For noop auth, skip_userid_check is True (allowing clients to provide user_id). + token is always NO_USER_TOKEN.src/auth/k8s.py (3)
231-242: Polish docstring for clarity about return and skip semanticsMake the return contract explicit and remove ambiguity.
- Returns: - The user's UID and username if authentication and authorization succeed - user_id check should never be skipped with K8s authentication - If user_id check should be skipped - always return False for k8s - User's token + Returns: + tuple[str, str, bool, str]: (user_id, username, skip_userid_check, token). + With k8s auth, skip_userid_check is always False.
243-255: Return 401 with WWW-Authenticate header for better client interoperabilityRFC 6750 recommends sending WWW-Authenticate: Bearer on 401. Helps CLIs/browsers.
- if not authorization_header: - raise HTTPException( - status_code=401, detail="Unauthorized: No auth header found" - ) + if not authorization_header: + raise HTTPException( + status_code=401, + detail="Unauthorized: No auth header found", + headers={"WWW-Authenticate": "Bearer"}, + ) ... - if not token: - raise HTTPException( - status_code=401, - detail="Unauthorized: Bearer token not found or invalid", - ) + if not token: + raise HTTPException( + status_code=401, + detail="Unauthorized: Bearer token not found or invalid", + headers={"WWW-Authenticate": "Bearer"}, + )
196-210: Harden bearer token parsingTrim whitespace and split on any whitespace; avoid accepting malformed headers.
-def _extract_bearer_token(header: str) -> str: +def _extract_bearer_token(header: str) -> str: @@ - try: - scheme, token = header.split(" ", 1) - return token if scheme.lower() == "bearer" else "" + header = header.strip() + if not header: + return "" + try: + scheme, token = header.split(None, 1) # split on any whitespace + if scheme.lower() != "bearer": + return "" + return token.strip() except ValueError: return ""src/app/endpoints/authorized.py (2)
28-31: Optional: use parentheses for 401 description too (consistency/readability)Keeps style consistent with the 400 block.
- 401: { - "description": "Missing or invalid credentials provided by client for the" - "k8s authentication module", - "model": UnauthorizedResponse, - }, + 401: { + "description": ( + "Missing or invalid credentials provided by client for the " + "k8s authentication module" + ), + "model": UnauthorizedResponse, + },
49-51: Update docstring to include skip_userid_check in responseReflect the new field in the endpoint documentation.
- Returns: - AuthorizedResponse: Contains the user ID and username of the authenticated user. + Returns: + AuthorizedResponse: Contains the user ID, username, and skip_userid_check flag.tests/e2e/features/steps/common_http.py (1)
14-26: Harden JSON parsing: avoid blind True/False replacement; handle None safely
Apply this diff:def _safe_json_loads(json_str): - """Safely parse JSON string, handling Python boolean literals. - - Handles both standard JSON boolean literals (true/false) and - Python boolean literals (True/False) for robust parsing. - """ + """Safely parse JSON string, tolerating Python literals (True/False/None). + First try strict JSON; on failure, fall back to Python-literal parsing. + """ try: return json.loads(json_str) except json.JSONDecodeError: - # Handle Python boolean literals in JSON - normalized_str = json_str.replace("True", "true").replace("False", "false") - return json.loads(normalized_str) + # Fall back to Python-literal parsing (e.g., True/False/None, single quotes) + import ast + return ast.literal_eval(json_str)Optional: move this helper to
tests/e2e/utils/for reuse across steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (29)
docs/openapi.json(2 hunks)docs/openapi.md(2 hunks)docs/output.md(2 hunks)src/app/endpoints/authorized.py(2 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/query.py(1 hunks)src/app/endpoints/streaming_query.py(1 hunks)src/auth/interface.py(1 hunks)src/auth/jwk_token.py(2 hunks)src/auth/k8s.py(2 hunks)src/auth/noop.py(2 hunks)src/auth/noop_with_token.py(2 hunks)src/authorization/resolvers.py(1 hunks)src/constants.py(1 hunks)src/models/responses.py(3 hunks)tests/e2e/features/authorized_noop.feature(5 hunks)tests/e2e/features/authorized_noop_token.feature(4 hunks)tests/e2e/features/steps/common_http.py(6 hunks)tests/integration/test_openapi_json.py(1 hunks)tests/unit/app/endpoints/test_authorized.py(2 hunks)tests/unit/app/endpoints/test_feedback.py(4 hunks)tests/unit/app/endpoints/test_query.py(3 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)tests/unit/auth/test_jwk_token.py(2 hunks)tests/unit/auth/test_k8s.py(2 hunks)tests/unit/auth/test_noop.py(2 hunks)tests/unit/auth/test_noop_with_token.py(2 hunks)tests/unit/authorization/test_resolvers.py(5 hunks)tests/unit/models/test_responses.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/features/authorized_noop.featuretests/e2e/features/authorized_noop_token.featuretests/unit/auth/test_noop_with_token.py
📚 Learning: 2025-09-02T11:15:02.411Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Applied to files:
tests/e2e/features/authorized_noop.featuretests/e2e/features/authorized_noop_token.feature
🪛 LanguageTool
docs/openapi.md
[grammar] ~400-~400: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 401 | Missing or invalid credentials p...
(QB_NEW_EN)
[grammar] ~401-~401: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 403 | User is not authorized | [Forbid...
(QB_NEW_EN)
docs/output.md
[grammar] ~400-~400: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 401 | Missing or invalid credentials p...
(QB_NEW_EN)
[grammar] ~401-~401: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 403 | User is not authorized | [Forbid...
(QB_NEW_EN)
[grammar] ~516-~516: There might be a mistake here.
Context: ...d check | Field | Type | Description | |-------|------|-------------| | user_id...
(QB_NEW_EN)
[grammar] ~517-~517: There might be a mistake here.
Context: ...ription | |-------|------|-------------| | user_id | string | User ID, for exampl...
(QB_NEW_EN)
[grammar] ~518-~518: There might be a mistake here.
Context: ...d | string | User ID, for example UUID | | username | string | User name | | skip...
(QB_NEW_EN)
[style] ~519-~519: It’s more common nowadays to write this noun as one word.
Context: ...or example UUID | | username | string | User name | | skip_userid_check | bool | skip use...
(RECOMMENDED_COMPOUNDS)
[grammar] ~519-~519: There might be a mistake here.
Context: ...UUID | | username | string | User name | | skip_userid_check | bool | skip user_i...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (21)
tests/integration/test_openapi_json.py (1)
74-75: LGTM: test updated to include 401 for /authorized.This aligns with the new k8s behavior.
tests/unit/app/endpoints/test_authorized.py (1)
10-10: MOCK_AUTH updated to 4-tuple — LGTMTuple shape matches the new AuthTuple (user_id, username, skip_userid_check, token).
src/models/responses.py (1)
310-311: AuthorizedResponse: new skip_userid_check field — LGTMField is required, docstring and examples are consistent.
Also applies to: 323-327, 334-337
tests/unit/app/endpoints/test_feedback.py (1)
85-86: Auth tuple expanded in tests — LGTMCalls now pass (user_id, username, skip_userid_check, token) consistently.
Also applies to: 112-113, 207-208, 224-225
tests/e2e/features/authorized_noop_token.feature (1)
2-2: Title update looks good.src/auth/noop_with_token.py (1)
56-56: LGTM: 4-tuple order matches AuthTuple contractOrder is (user_id, username, skip_userid_check, token) as expected.
tests/unit/auth/test_noop_with_token.py (1)
50-56: LGTM: custom user_id path coveredCovers query param override and preserves skip flag semantics.
tests/unit/auth/test_jwk_token.py (1)
263-271: LGTM: no-auth-header behavior capturedFallback to defaults with skip_userid_check=True and NO_USER_TOKEN is verified.
src/auth/jwk_token.py (1)
194-194: 4‑tuple return ordering matches AuthTuple — LGTM.tests/unit/auth/test_k8s.py (1)
97-104: K8S auth now returns 4‑tuple; assertions cover skip_userid_check=False — LGTM.Also applies to: 169-176
src/auth/interface.py (1)
12-31: Auth dependencies return correct 4-tuple
All AuthDependency call methods consistently return (user_id, username, skip_userid_check, token), as verified across noop, noop_with_token, jwk_token, and k8s implementations. Tests cover this pattern.src/auth/noop.py (1)
26-26: Signature aligns with new AuthTuple (4‑tuple)Returning (uid, username, skip_userid_check, token) looks correct.
src/auth/k8s.py (2)
229-229: Explicitly set skip_userid_check to False for k8sGood: k8s should never skip the user_id check.
6-6: typing.Self supported by Python 3.12 requirement
pyproject.toml declaresrequires-python = ">=3.12,<3.14", and typing.Self is available since Python 3.11, so importing Self from typing is valid.src/app/endpoints/authorized.py (2)
53-56: Unpacking 4‑tuple and propagating skip_userid_check looks correctEndpoint correctly ignores the token and returns the new field.
23-26: Syntax error in responses dict (missing comma after concatenated string)The 400 description value needs a trailing comma; wrap in parentheses for safe literal concatenation.
- 400: { - "description": "Missing or invalid credentials provided by client for the noop and" - "noop-with-token authentication modules" - "model": UnauthorizedResponse, - }, + 400: { + "description": ( + "Missing or invalid credentials provided by client for the noop and " + "noop-with-token authentication modules" + ), + "model": UnauthorizedResponse, + },Likely an incorrect or invalid review comment.
tests/e2e/features/steps/common_http.py (5)
64-64: LGTM: unified payload parsing via helperConsistently using
_safe_json_loadshere improves test robustness.
157-157: LGTM: schema parsing via helperThis will tolerate Python booleans in inline schemas.
182-182: LGTM: expected body parsing via helperKeeps comparisons resilient to Python-literal booleans.
201-201: LGTM: ignore-field comparator uses helperShallow copy after safe parse is appropriate.
272-272: LGTM: POST payload parsing via helperEnsures POSTed JSON is parsed consistently with other steps.
| logger.info("Llama stack config: %s", configuration.llama_stack_configuration) | ||
|
|
||
| user_id, _, token = auth | ||
| user_id, _, _, token = auth |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
AuthTuple unpack updated to 4-tuple — consider self-documenting placeholders.
Using named throwaways improves readability when the tuple grows.
- user_id, _, _, token = auth
+ user_id, _username, _skip_userid_check, token = authRun to catch any remaining 3-item unpacking of auth across the repo:
🏁 Script executed:
#!/bin/bash
# 3-item unpacks of `auth`
rg -nP --type=py '\b([A-Za-z_][A-Za-z0-9_]*\s*,\s*){2}[A-Za-z_][A-Za-z0-9_]*\s*=\s*auth\b' src/
# 4-item unpacks of `auth` (expected)
rg -nP --type=py '\b([A-Za-z_][A-Za-z0-9_]*\s*,\s*){3}[A-Za-z_][A-Za-z0-9_]*\s*=\s*auth\b' src/Length of output: 1261
Unpack auth into all four elements with descriptive placeholders
Update the three-element unpack in conversations endpoints to the new 4-tuple shape and use self-documenting throwaways for unused fields. For example:
user_id, _user_name, _skip_userid_check, token = authApply this change at:
- src/app/endpoints/conversations.py:163
- src/app/endpoints/conversations.py:252
- src/app/endpoints/conversations.py:374
🤖 Prompt for AI Agents
In src/app/endpoints/query.py around line 183, the auth tuple should be unpacked
into four named elements with self-documenting throwaway placeholders; replace
the current generic unpack with one that assigns user_id, a descriptive unused
username placeholder, a descriptive unused skip_userid_check placeholder, and
token (for example: user_id, _user_name, _skip_userid_check, token = auth) so
callers and future readers know the unused positions and the token position;
also apply the same change to the three locations noted in the review for
src/app/endpoints/conversations.py (lines ~163, ~252, ~374).
| logger.error("API exception during SubjectAccessReview: %s", e) | ||
| raise HTTPException(status_code=403, detail="Internal server error") from e | ||
|
|
There was a problem hiding this comment.
403 with “Internal server error” message is inconsistent; use 500
On SAR ApiException, respond 500 to reflect server failure.
- raise HTTPException(status_code=403, detail="Internal server error") from e
+ raise HTTPException(status_code=500, detail="Internal server error") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.error("API exception during SubjectAccessReview: %s", e) | |
| raise HTTPException(status_code=403, detail="Internal server error") from e | |
| logger.error("API exception during SubjectAccessReview: %s", e) | |
| raise HTTPException(status_code=500, detail="Internal server error") from e |
🤖 Prompt for AI Agents
In src/auth/k8s.py around lines 281 to 283, the handler currently raises
HTTPException(status_code=403, detail="Internal server error") on an
ApiException from SubjectAccessReview; change the response to
HTTPException(status_code=500, detail="Internal server error") so the HTTP
status reflects a server-side failure (keep the existing logger.error call and
preserve the exception chaining "from e").
| skip_userid_check: bool = Field( | ||
| ..., | ||
| description="Whether to skip the user ID check", | ||
| examples=[True, False], | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all producers now set skip_userid_check
Guard against partial updates by scanning for missing field in AuthorizedResponse constructors and lingering 3-tuple auth usage.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Calls to AuthorizedResponse potentially missing skip_userid_check:"
# List all constructor calls and filter out those that already mention skip_userid_check
rg -n 'AuthorizedResponse\s*\(' | rg -v 'skip_userid_check'
echo
echo "Possible 3-tuple auth destructuring or construction:"
# Common patterns of 3-elem destructure from auth
rg -nP '\b([\w_]+\s*,\s*[\w_]+\s*,\s*[\w_]+)\s*=\s*auth\b' -g '!**/node_modules/**'
# 3-elem tuples passed as auth=(...)
rg -nP r'auth\s*=\s*\([^,()]+,\s*[^,()]+,\s*[^,()]+\)' -g '!**/node_modules/**'Length of output: 1526
Include skip_userid_check in all AuthorizedResponse usages
- In src/app/endpoints/authorized.py (line 54), add
skip_userid_check=skip_userid_checkto theAuthorizedResponseconstructor. - In tests/unit/models/test_responses.py (lines 55, 67, 70), update each
AuthorizedResponse(...)call to includeskip_userid_check. - Refactor any 3-element
authtuple unpacking (e.g. in resolvers.py:84; feedback.py:113, 198; query.py:183; conversations.py:163, 252, 374) to unpack four items(user_id, user_name, skip_userid_check, token)so the new flag isn’t dropped.
🤖 Prompt for AI Agents
In src/models/responses.py around lines 323-327, the new Field skip_userid_check
was added but not propagated; update usages: in src/app/endpoints/authorized.py
line 54 pass skip_userid_check=skip_userid_check into the AuthorizedResponse
constructor; update tests in tests/unit/models/test_responses.py at lines 55,
67, and 70 to include skip_userid_check in each AuthorizedResponse(...) call;
and refactor any 3-element auth tuple unpacking (resolvers.py:84;
feedback.py:113,198; query.py:183; conversations.py:163,252,374) to unpack four
items as (user_id, user_name, skip_userid_check, token) so the new flag is
preserved and threaded into AuthorizedResponse creations.
| user_id, username, skip_userid_check, token = auth_tuple | ||
| assert user_id == TEST_USER_ID | ||
| assert username == TEST_USER_NAME | ||
| assert skip_userid_check is False | ||
| assert token == expected_token |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: helper enforces new tuple shape
Asserting skip_userid_check is False for valid JWK flow is correct.
Run a quick scan for any leftover 3-tuple unpacks of auth:
🏁 Script executed:
#!/bin/bash
# Potential legacy 3-var unpacks from `auth`
rg -nP --type=py -C2 '^\s*\w+\s*,\s*\w+\s*,\s*\w+\s*=\s*auth\b' | rg -v 'skip_userid_check|_skip'
# Old 3-elt Tuple types
rg -nP --type=py 'Tuple\s*\[\s*str\s*,\s*str\s*,\s*str\s*\]'
# Docstrings mentioning 3-tuple returns
rg -n --type=py -S 'Returns a tuple: .*user_token'Length of output: 1223
Update all legacy 3-variable auth unpack and docstring to the new 4-element tuple shape
- src/app/endpoints/conversations.py (lines 163, 252, 374): change
to
user_id, _, _ = auth
user_id, username, skip_userid_check, token = auth
- src/auth/noop_with_token.py (line 9): update the docstring to reflect four returned values, e.g.
Returns a tuple: (user_id, DEFAULT_USER_NAME, False, user_token).
🤖 Prompt for AI Agents
In tests/unit/auth/test_jwk_token.py around lines 177 to 181, the test still
unpacks the auth tuple into three variables but the new auth API returns four
elements; update the unpack to user_id, username, skip_userid_check, token =
auth and assert username and skip_userid_check against TEST_USER_NAME and False
respectively (keep existing user_id and token assertions), and scan the repo for
other legacy 3-variable unpack patterns to update similarly and adjust any
related docstrings (e.g., src/auth/noop_with_token.py) to document the 4-tuple
return shape.
| auth = ("user", "token", False, claims_to_token(jwt_claims)) | ||
| roles = await jwt_resolver.resolve_roles(auth) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Quick repo sanity sweep for 3-tuple remnants
Automate a check for lingering 3-elem usage and incorrect tuple unpacking.
Also applies to: 77-78, 100-101, 123-124, 169-170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "3-element destructures from auth (should be 4 now):"
rg -nP '\b([\w_]+\s*,\s*[\w_]+\s*,\s*[\w_]+)\s*=\s*auth\b' -g '!**/node_modules/**' -S
echo
echo "Auth tuple constructions with only 3 elements:"
rg -nP r'auth\s*=\s*\([^,()]+,\s*[^,()]+,\s*[^,()]+\)' -g '!**/node_modules/**' -SLength of output: 1046
Replace all 3-variable auth unpacking in conversations.py
- In src/app/endpoints/conversations.py (lines 163, 252, 374), change
user_id, _, _ = authto unpack all four elements (e.g.user_id, user_name, skip_userid_check, token = auth).
🤖 Prompt for AI Agents
In src/app/endpoints/conversations.py around lines 163, 252, and 374, the code
currently unpacks auth as three variables (user_id, _, _) but the auth tuple
actually contains four elements; update each unpack to capture all four values
(for example: user_id, user_name, skip_userid_check, token = auth) and then use
the newly captured variables where needed (replace discarded placeholders with
user_name or token or skip_userid_check as appropriate) so there are no ignored
elements and downstream checks/reference use the correct variables.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM
(OT: will create task to refactor those large tuples to value classes in auth handling)
radofuchs
force-pushed
the
LCORE_287_authorized_openshift_compatibility
branch
from
8af6087 to
9f21060
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/endpoints/streaming_query.py (1)
765-770: Gate MCP header token injection behind an explicit opt-inCurrently: if no specific MCP headers are provided and
tokenexists, you setAuthorization: Bearer {token}for all MCP servers. Make this opt-in to avoid credential exfiltration.Apply this minimal change:
- if not mcp_headers and token: + # Only forward user tokens to MCP servers if explicitly allowed + if not mcp_headers and token and getattr(configuration.authentication_configuration, "forward_mcp_user_token", False): for mcp_server in configuration.mcp_servers: mcp_headers[mcp_server.url] = { "Authorization": f"Bearer {token}", }If
forward_mcp_user_tokendoesn’t exist in your config, I can add it in a separate PR and default it to False.src/auth/k8s.py (1)
185-193: Distinguish K8s API failures (500) from invalid tokens (403)
get_user_inforeturns None onApiException, which leads to a 403 above. Treat API errors as 500 to reflect server-side failure.Apply:
- except ApiException as e: - logger.error("API exception during TokenReview: %s", e) - return None + except ApiException as e: + logger.error("API exception during TokenReview: %s", e) + raise HTTPException( + status_code=500, + detail={"response": "Internal server error: Unable to review token", "cause": str(e)}, + ) from edocs/openapi.md (1)
505-520: Add skip_userid_check to Attributes and unify its type naming
- Insert in the Attributes list:
skip_userid_check: Whether to skip the user ID check.- In the Fields table, change the type from
booltobooleanand update the description:Attributes: user_id: The ID of the logged in user. username: The name of the logged in user. + skip_userid_check: Whether to skip the user ID check. @@ -| skip_userid_check | bool | Whether to skip user_id check | +| skip_userid_check | boolean | Whether to skip the user ID check |
♻️ Duplicate comments (1)
src/auth/k8s.py (1)
281-283: Use 500 on SAR ApiException (server failure), not 403This mirrors prior feedback and keeps semantics consistent.
Apply:
- raise HTTPException(status_code=403, detail="Internal server error") from e + raise HTTPException(status_code=500, detail="Internal server error") from e
🧹 Nitpick comments (10)
src/auth/noop.py (3)
24-24: Use the shared constant for consistencyInitialize with
DEFAULT_SKIP_USER_ID_CHECK(added in this PR) rather than a literal.Apply:
-from constants import ( +from constants import ( DEFAULT_USER_NAME, DEFAULT_USER_UID, NO_USER_TOKEN, DEFAULT_VIRTUAL_PATH, + DEFAULT_SKIP_USER_ID_CHECK, ) @@ - self.skip_userid_check = True + self.skip_userid_check = DEFAULT_SKIP_USER_ID_CHECK
26-26: Annotate return type with theAuthTuplealiasAligns implementations and improves readability/static checks.
Apply:
-from auth.interface import AuthInterface +from auth.interface import AuthInterface, AuthTuple @@ - async def __call__(self, request: Request) -> tuple[str, str, bool, str]: + async def __call__(self, request: Request) -> AuthTuple:
32-35: Docstring: clarify the full 4-tuple contractList all fields returned, including the token, to match the new interface.
Suggested snippet:
- Returns:
- tuple[str, str, bool, str]: (user_id, user_name, skip_userid_check=True, token="").
- With Noop auth, user_id checks are skipped to allow clients to provide user_id; no token is provided.
src/auth/k8s.py (3)
196-210: Harden Bearer token parsingTrim the token and tolerate extra spaces; avoid passing a token with leading whitespace.
Apply:
def _extract_bearer_token(header: str) -> str: @@ - try: - scheme, token = header.split(" ", 1) - return token if scheme.lower() == "bearer" else "" + try: + scheme, token = header.split(" ", 1) + if scheme.lower().strip() == "bearer": + return token.strip() + return ""
231-242: Return type: useAuthTuplealiasKeep dependencies consistent with the shared type.
Apply:
-from auth.interface import AuthInterface +from auth.interface import AuthInterface, AuthTuple @@ - async def __call__(self, request: Request) -> tuple[str, str, bool, str]: + async def __call__(self, request: Request) -> AuthTuple:
243-255: 401 responses should includeWWW-Authenticate: BearerBetter interoperability with clients expecting RFC 6750 semantics.
Apply:
- if not authorization_header: - raise HTTPException( - status_code=401, detail="Unauthorized: No auth header found" - ) + if not authorization_header: + raise HTTPException( + status_code=401, + detail="Unauthorized: No auth header found", + headers={"WWW-Authenticate": "Bearer"}, + ) @@ - if not token: - raise HTTPException( - status_code=401, - detail="Unauthorized: Bearer token not found or invalid", - ) + if not token: + raise HTTPException( + status_code=401, + detail="Unauthorized: Bearer token not found or invalid", + headers={"WWW-Authenticate": "Bearer"}, + )docs/openapi.md (1)
400-401: Clarify module context and fix grammar (“the client”; format module names).Suggested rewrite for consistency and readability.
-| 400 | Missing or invalid credentials provided by client for noop and noop-with-token | [UnauthorizedResponse](#unauthorizedresponse) | -| 401 | Missing or invalid credentials provided by client for k8s | [UnauthorizedResponse](#unauthorizedresponse) | +| 400 | Missing or invalid credentials provided by the client for the `noop` and `noop-with-token` modules | [UnauthorizedResponse](#unauthorizedresponse) | +| 401 | Missing or invalid credentials provided by the client for the `k8s` module | [UnauthorizedResponse](#unauthorizedresponse) |docs/output.md (3)
400-401: Same grammar/format fix as openapi.md for 400/401 descriptions.-| 400 | Missing or invalid credentials provided by client for noop and noop-with-token | [UnauthorizedResponse](#unauthorizedresponse) | -| 401 | Missing or invalid credentials provided by client for k8s | [UnauthorizedResponse](#unauthorizedresponse) | +| 400 | Missing or invalid credentials provided by the client for the `noop` and `noop-with-token` modules | [UnauthorizedResponse](#unauthorizedresponse) | +| 401 | Missing or invalid credentials provided by the client for the `k8s` module | [UnauthorizedResponse](#unauthorizedresponse) |
510-514: Polish wording for the new attribute.Add article and use “user ID”.
- skip_userid_check: Whether to skip user_id check + skip_userid_check: Whether to skip the user ID check.
516-521: Standardize skip_userid_check field type and description
- In docs/output.md (line 520) and docs/openapi.md (line 519), change the type from
booltobooleanand update the description to “Whether to skip the user ID check”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
docs/openapi.json(2 hunks)docs/openapi.md(2 hunks)docs/output.md(2 hunks)src/app/endpoints/authorized.py(2 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/query.py(1 hunks)src/app/endpoints/streaming_query.py(1 hunks)src/auth/interface.py(1 hunks)src/auth/jwk_token.py(2 hunks)src/auth/k8s.py(2 hunks)src/auth/noop.py(2 hunks)src/auth/noop_with_token.py(2 hunks)src/authorization/resolvers.py(1 hunks)src/constants.py(1 hunks)src/models/responses.py(3 hunks)tests/e2e/features/authorized_noop.feature(5 hunks)tests/e2e/features/authorized_noop_token.feature(4 hunks)tests/integration/test_openapi_json.py(1 hunks)tests/unit/app/endpoints/test_authorized.py(2 hunks)tests/unit/app/endpoints/test_feedback.py(4 hunks)tests/unit/app/endpoints/test_query.py(3 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)tests/unit/auth/test_jwk_token.py(2 hunks)tests/unit/auth/test_k8s.py(2 hunks)tests/unit/auth/test_noop.py(2 hunks)tests/unit/auth/test_noop_with_token.py(2 hunks)tests/unit/authorization/test_resolvers.py(5 hunks)tests/unit/models/test_responses.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- src/constants.py
- tests/unit/app/endpoints/test_authorized.py
- src/app/endpoints/query.py
- src/authorization/resolvers.py
- src/auth/jwk_token.py
- src/models/responses.py
- tests/unit/app/endpoints/test_query.py
- tests/unit/auth/test_noop.py
- src/app/endpoints/feedback.py
- tests/unit/app/endpoints/test_streaming_query.py
- tests/unit/authorization/test_resolvers.py
- src/auth/noop_with_token.py
- tests/unit/app/endpoints/test_feedback.py
- tests/unit/auth/test_jwk_token.py
- tests/unit/auth/test_noop_with_token.py
- tests/e2e/features/authorized_noop.feature
- tests/integration/test_openapi_json.py
- src/auth/interface.py
- tests/unit/auth/test_k8s.py
- tests/e2e/features/authorized_noop_token.feature
- docs/openapi.json
- src/app/endpoints/authorized.py
- tests/unit/models/test_responses.py
🧰 Additional context used
🪛 LanguageTool
docs/openapi.md
[grammar] ~400-~400: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 401 | Missing or invalid credentials p...
(QB_NEW_EN)
[grammar] ~401-~401: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 403 | User is not authorized | [Forbid...
(QB_NEW_EN)
docs/output.md
[grammar] ~400-~400: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 401 | Missing or invalid credentials p...
(QB_NEW_EN)
[grammar] ~401-~401: There might be a mistake here.
Context: ...orizedResponse](#unauthorizedresponse) | | 403 | User is not authorized | [Forbid...
(QB_NEW_EN)
[grammar] ~516-~516: There might be a mistake here.
Context: ...d check | Field | Type | Description | |-------|------|-------------| | user_id...
(QB_NEW_EN)
[grammar] ~517-~517: There might be a mistake here.
Context: ...ription | |-------|------|-------------| | user_id | string | User ID, for exampl...
(QB_NEW_EN)
[grammar] ~518-~518: There might be a mistake here.
Context: ...d | string | User ID, for example UUID | | username | string | User name | | skip...
(QB_NEW_EN)
[style] ~519-~519: It’s more common nowadays to write this noun as one word.
Context: ...or example UUID | | username | string | User name | | skip_userid_check | bool | skip use...
(RECOMMENDED_COMPOUNDS)
[grammar] ~519-~519: There might be a mistake here.
Context: ...UUID | | username | string | User name | | skip_userid_check | bool | skip user_i...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (1)
src/auth/noop.py (1)
43-43: LGTM on 4-tuple returnReturn order matches the new
AuthTuplecontract. No further action.
| return ( | ||
| user_info.user.uid, | ||
| user_info.user.username, | ||
| self.skip_userid_check, | ||
| token, | ||
| ) |
There was a problem hiding this comment.
Reconsider returning the raw K8s token to downstream
The 4th tuple element is now the bearer token. Downstream code currently forwards it to MCP servers by default when no mcp_headers are provided, which can leak cluster/user tokens.
Options:
- Return an empty token from K8s auth (preferred, safest), or
- Add a config flag (default False) to allow forwarding user tokens to MCP and gate on it, or
- Mark tokens with an origin/scope and forward only those explicitly intended for MCP.
Would you like me to draft a small design doc for this?
radofuchs
force-pushed
the
LCORE_287_authorized_openshift_compatibility
branch
from
ad2eff8 to
c5f7049
Compare
…authorized endpoint. Added skip_userid_check as returned bool value to describe whether user_id check was performed. Aligned k8s missing header status code with openshift to 401.
radofuchs
force-pushed
the
LCORE_287_authorized_openshift_compatibility
branch
from
29b8a94 to
86bf3fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/app/endpoints/test_streaming_query.py (1)
146-149: Fix pytest.raises misuse and update expected error message
- Move the
assertstatements out of thewith pytest.raises(HTTPException)block and reference the exception viae.value.- In
test_streaming_query_endpoint_on_connection_error(lines 178–182), change the expected response from"Configuration is not loaded"to"Unable to connect to Llama Stack"to match the handler’sdetail["response"].
🧹 Nitpick comments (9)
tests/unit/models/responses/test_authorized_response.py (2)
15-21: Add assertion for skip_userid_check to prevent silent regressions.Assert the new boolean to fully exercise the model.
ar = AuthorizedResponse( user_id="123e4567-e89b-12d3-a456-426614174000", username="testuser", skip_userid_check=False, ) assert ar.user_id == "123e4567-e89b-12d3-a456-426614174000" assert ar.username == "testuser" +assert ar.skip_userid_check is False
23-35: Add a “missing skip_userid_check” validation case.Since skip_userid_check is required, add a dedicated negative test.
with pytest.raises(ValidationError): # missing username parameter _ = AuthorizedResponse( user_id="123e4567-e89b-12d3-a456-426614174000" ) # pyright: ignore +with pytest.raises(ValidationError): + # missing skip_userid_check parameter + _ = AuthorizedResponse( + user_id="123e4567-e89b-12d3-a456-426614174000", + username="testuser", + ) # pyright: ignoretests/unit/app/endpoints/test_query.py (1)
34-34: Type the MOCK_AUTH to AuthTuple (and/or centralize via fixture).Helps catch tuple-shape regressions across tests.
+from auth.interface import AuthTuple -MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") +MOCK_AUTH: AuthTuple = ("mock_user_id", "mock_username", False, "mock_token")Optionally, expose a pytest fixture returning this tuple to DRY repeated literals.
tests/unit/models/test_responses.py (2)
1-1: Tiny grammar nit in module docstring.-"""Tests for QueryResponse. StatusResponse, AuthorizedResponse, and UnauthorizedResponse models.""" +"""Tests for QueryResponse, StatusResponse, AuthorizedResponse, and UnauthorizedResponse models."""
85-88: Narrow the expected exception to ValidationError.Avoids masking unexpected errors.
-with pytest.raises(Exception): +with pytest.raises(ValidationError): UnauthorizedResponse() # pyright: ignoresrc/app/endpoints/authorized.py (1)
49-51: Update docstring to mention skip_userid_check in the return description.- AuthorizedResponse: Contains the user ID and username of the authenticated user. + AuthorizedResponse: Contains the user ID, username, and skip_userid_check flag.tests/unit/app/endpoints/test_streaming_query.py (3)
50-50: Prefer a typed Auth container over a raw 4-tupleThe 4-tuple is correct, but positional unpacking is brittle. Use a NamedTuple (or dataclass) to make fields explicit and reduce future breakage.
Apply near this constant:
+from typing import NamedTuple + +class Auth(NamedTuple): + user_id: str + username: str + skip_userid_check: bool + token: str + -MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") +MOCK_AUTH = Auth("mock_user_id", "mock_username", False, "mock_token")
1109-1123: Compare parsed JSON, not raw dumps, to avoid key-order flakinessThese tests compare a JSON-serialized string for equality. Dict key order can change; parse and compare objects instead.
Example:
-assert mock_agent.extra_headers == expected_extra_headers +actual = json.loads(mock_agent.extra_headers["X-LlamaStack-Provider-Data"]) +expected = json.loads(expected_extra_headers["X-LlamaStack-Provider-Data"]) +assert actual == expectedAlso applies to: 1265-1268, 1511-1519
197-203: Stabilize async iteration in mocksAssigning a list or
iter([])to__aiter__.return_valuecan be fragile. Provide a real async iterator to better mirror runtime.Add helper once in this module:
def _async_iter(items): async def gen(): for x in items: yield x return gen()Then:
-mock_streaming_response.__aiter__.return_value = [ +mock_streaming_response.__aiter__.return_value = _async_iter([ ...chunks... -] +])and
-mock_streaming_response.__aiter__.return_value = iter([]) +mock_streaming_response.__aiter__.return_value = _async_iter([])Also applies to: 1297-1299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
docs/openapi.json(2 hunks)docs/openapi.md(2 hunks)docs/output.md(2 hunks)src/app/endpoints/authorized.py(2 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/query.py(1 hunks)src/app/endpoints/streaming_query.py(1 hunks)src/auth/interface.py(1 hunks)src/auth/jwk_token.py(2 hunks)src/auth/k8s.py(2 hunks)src/auth/noop.py(2 hunks)src/auth/noop_with_token.py(2 hunks)src/authorization/resolvers.py(1 hunks)src/constants.py(1 hunks)src/models/responses.py(3 hunks)tests/e2e/features/authorized_noop.feature(5 hunks)tests/e2e/features/authorized_noop_token.feature(4 hunks)tests/integration/test_openapi_json.py(1 hunks)tests/unit/app/endpoints/test_authorized.py(2 hunks)tests/unit/app/endpoints/test_feedback.py(4 hunks)tests/unit/app/endpoints/test_query.py(3 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)tests/unit/auth/test_jwk_token.py(2 hunks)tests/unit/auth/test_k8s.py(2 hunks)tests/unit/auth/test_noop.py(2 hunks)tests/unit/auth/test_noop_with_token.py(2 hunks)tests/unit/authorization/test_resolvers.py(5 hunks)tests/unit/models/responses/test_authorized_response.py(1 hunks)tests/unit/models/test_responses.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- tests/unit/auth/test_noop.py
- docs/output.md
- tests/unit/auth/test_jwk_token.py
- src/auth/noop.py
- src/app/endpoints/query.py
- src/app/endpoints/feedback.py
- tests/e2e/features/authorized_noop.feature
- src/auth/jwk_token.py
- tests/integration/test_openapi_json.py
- tests/unit/app/endpoints/test_feedback.py
- src/models/responses.py
- tests/unit/authorization/test_resolvers.py
- src/app/endpoints/streaming_query.py
- tests/unit/auth/test_k8s.py
- src/auth/interface.py
- docs/openapi.json
- src/authorization/resolvers.py
- src/auth/k8s.py
- src/auth/noop_with_token.py
- tests/e2e/features/authorized_noop_token.feature
- src/constants.py
- docs/openapi.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/endpoints/authorized.py (1)
src/models/responses.py (2)
UnauthorizedResponse(343-357)AuthorizedResponse(304-340)
tests/unit/models/test_responses.py (1)
src/models/responses.py (4)
QueryResponse(48-79)StatusResponse(272-301)AuthorizedResponse(304-340)UnauthorizedResponse(343-357)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-pr
🔇 Additional comments (9)
tests/unit/auth/test_noop_with_token.py (2)
25-31: Tuple expansion and new assertion look correct.Unpacking the 4-tuple and asserting skip_userid_check=True matches the new AuthTuple contract.
50-56: LGTM on custom user_id path.Covers query param override and keeps the new boolean assertion.
tests/unit/app/endpoints/test_query.py (2)
111-112: Correct update to 4-tuple usage.Passing (user, name, skip, token) maintains handler expectations.
1217-1218: Auth tuple shape consistently applied.Unpacking and downstream usage of token remains correct.
tests/unit/models/test_responses.py (1)
50-63: Good coverage of the new boolean field.Constructor and boolean assertion for skip_userid_check are spot on.
tests/unit/app/endpoints/test_authorized.py (2)
10-10: Auth tuple update looks correct.Matches new (user_id, username, skip_userid_check, token) signature.
18-22: Response shape assertion updated appropriately.Verifies the new field and omits token as intended.
src/app/endpoints/authorized.py (1)
53-56: Unpacking and response mapping look good.Correctly ignores token and propagates skip_userid_check.
tests/unit/app/endpoints/test_streaming_query.py (1)
1319-1326: LGTM: verifies token pass-through from 4th auth elementGood assertion that the handler forwards the 4th tuple element as the token.
| "description": "Missing or invalid credentials provided by client for the noop and" | ||
| "noop-with-token authentication modules", | ||
| "model": UnauthorizedResponse, | ||
| }, | ||
| 401: { | ||
| "description": "Missing or invalid credentials provided by client for the" | ||
| "k8s authentication module", | ||
| "model": UnauthorizedResponse, | ||
| }, |
There was a problem hiding this comment.
Fix missing spaces in concatenated doc strings (user-facing text).
Adjacent string literals concatenate without spaces; current text renders “noop andnoop-with-token” and “thek8s”.
- "description": "Missing or invalid credentials provided by client for the noop and"
- "noop-with-token authentication modules",
+ "description": "Missing or invalid credentials provided by client for the noop and "
+ "noop-with-token authentication modules",
@@
- "description": "Missing or invalid credentials provided by client for the"
- "k8s authentication module",
+ "description": "Missing or invalid credentials provided by client for the "
+ "k8s authentication module",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "description": "Missing or invalid credentials provided by client for the noop and" | |
| "noop-with-token authentication modules", | |
| "model": UnauthorizedResponse, | |
| }, | |
| 401: { | |
| "description": "Missing or invalid credentials provided by client for the" | |
| "k8s authentication module", | |
| "model": UnauthorizedResponse, | |
| }, | |
| "description": "Missing or invalid credentials provided by client for the noop and " | |
| "noop-with-token authentication modules", | |
| "model": UnauthorizedResponse, | |
| }, | |
| 401: { | |
| "description": "Missing or invalid credentials provided by client for the " | |
| "k8s authentication module", | |
| "model": UnauthorizedResponse, | |
| }, |
🤖 Prompt for AI Agents
In src/app/endpoints/authorized.py around lines 23 to 31 the adjacent string
literals for the response descriptions are concatenating without spaces
(producing "noop andnoop-with-token" and "thek8s"); fix by adding a space either
at the end of the first literal or at the start of the second literal so the
joined strings read "...noop and noop-with-token..." and "...the k8s..."; update
both description pairs accordingly and run formatter/checks.
2 participants
Description
update documentation for authorized endpoint to align with openshift authorized endpoint. Added skip_userid_check as returned bool value to describe whether user_id check was performed. Aligned k8s missing header status code with openshift to 401.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation