Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions docs/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,17 @@
}
},
"400": {
"description": "Missing or invalid credentials provided by client",
"description": "Missing or invalid credentials provided by client for the noop and noop-with-token authentication modules",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/UnauthorizedResponse"
}
}
}
},
"401": {
"description": "Missing or invalid credentials provided by client for the k8s authentication module",
"content": {
"application/json": {
"schema": {
Expand Down Expand Up @@ -922,19 +932,27 @@
"John Doe",
"Adam Smith"
]
},
"skip_userid_check": {
"type": "boolean",
"title": "Skip User Id Check",
"description": "Whether to skip the user ID check",
"examples": [true, false]
}
},
"type": "object",
"required": [
"user_id",
"username"
"username",
"skip_userid_check"
],
"title": "AuthorizedResponse",
"description": "Model representing a response to an authorization request.\n\nAttributes:\n user_id: The ID of the logged in user.\n username: The name of the logged in user.",
"examples": [
{
"user_id": "123e4567-e89b-12d3-a456-426614174000",
"username": "user1"
"username": "user1",
"skip_userid_check": false
}
]
},
Expand Down
4 changes: 3 additions & 1 deletion docs/openapi.md
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,8 @@ Returns:
| Status Code | Description | Component |
|-------------|-------------|-----------|
| 200 | The user is logged-in and authorized to access OLS | [AuthorizedResponse](#authorizedresponse) |
| 400 | Missing or invalid credentials provided by client | [UnauthorizedResponse](#unauthorizedresponse) |
| 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) |
| 403 | User is not authorized | [ForbiddenResponse](#forbiddenresponse) |
## GET `/metrics`

Expand Down Expand Up @@ -515,6 +516,7 @@ Attributes:
|-------|------|-------------|
| user_id | string | User ID, for example UUID |
| username | string | User name |
| skip_userid_check | bool | Whether to skip user_id check |


## CORSConfiguration
Expand Down
5 changes: 4 additions & 1 deletion docs/output.md
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,8 @@ Returns:
| Status Code | Description | Component |
|-------------|-------------|-----------|
| 200 | The user is logged-in and authorized to access OLS | [AuthorizedResponse](#authorizedresponse) |
| 400 | Missing or invalid credentials provided by client | [UnauthorizedResponse](#unauthorizedresponse) |
| 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) |
| 403 | User is not authorized | [ForbiddenResponse](#forbiddenresponse) |
## GET `/metrics`

Expand Down Expand Up @@ -509,12 +510,14 @@ Model representing a response to an authorization request.
Attributes:
user_id: The ID of the logged in user.
username: The name of the logged in user.
skip_userid_check: Whether to skip user_id check


| Field | Type | Description |
|-------|------|-------------|
| user_id | string | User ID, for example UUID |
| username | string | User name |
| skip_userid_check | bool | skip user_id check |


## CORSConfiguration
Expand Down
14 changes: 11 additions & 3 deletions src/app/endpoints/authorized.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@
"model": AuthorizedResponse,
},
400: {
"description": "Missing or invalid credentials provided by client",
"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,
},
Comment on lines +23 to 31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
"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.

403: {
Expand All @@ -44,5 +50,7 @@ async def authorized_endpoint_handler(
AuthorizedResponse: Contains the user ID and username of the authenticated user.
"""
# Ignore the user token, we should not return it in the response
user_id, user_name, _ = auth
return AuthorizedResponse(user_id=user_id, username=user_name)
user_id, user_name, skip_userid_check, _ = auth
return AuthorizedResponse(
user_id=user_id, username=user_name, skip_userid_check=skip_userid_check
)
4 changes: 2 additions & 2 deletions src/app/endpoints/feedback.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ async def feedback_endpoint_handler(
"""
logger.debug("Feedback received %s", str(feedback_request))

user_id, _, _ = auth
user_id, _, _, _ = auth
try:
store_feedback(user_id, feedback_request.model_dump(exclude={"model_config"}))
except Exception as e:
Expand Down Expand Up @@ -195,7 +195,7 @@ async def update_feedback_status(
Returns:
FeedbackStatusUpdateResponse: Indicates whether feedback is enabled.
"""
user_id, _, _ = auth
user_id, _, _, _ = auth
requested_status = feedback_update_request.get_value()

with feedback_status_lock:
Expand Down
2 changes: 1 addition & 1 deletion src/app/endpoints/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ async def query_endpoint_handler(
# log Llama Stack configuration
logger.info("Llama stack config: %s", configuration.llama_stack_configuration)

user_id, _, token = auth
user_id, _, _, token = auth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 = auth

Run 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 = auth

Apply 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).


user_conversation: UserConversation | None = None
if query_request.conversation_id:
Expand Down
2 changes: 1 addition & 1 deletion src/app/endpoints/streaming_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ async def streaming_query_endpoint_handler( # pylint: disable=too-many-locals
# log Llama Stack configuration
logger.info("Llama stack config: %s", configuration.llama_stack_configuration)

user_id, _user_name, token = auth
user_id, _user_name, _skip_userid_check, token = auth

user_conversation: UserConversation | None = None
if query_request.conversation_id:
Expand Down
17 changes: 14 additions & 3 deletions src/auth/interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,26 @@

from fastapi import Request

from constants import DEFAULT_USER_NAME, DEFAULT_USER_UID, NO_USER_TOKEN
from constants import (
DEFAULT_USER_NAME,
DEFAULT_SKIP_USER_ID_CHECK,
DEFAULT_USER_UID,
NO_USER_TOKEN,
)

UserID = str
UserName = str
SkipUserIdCheck = bool
Token = str

AuthTuple = tuple[UserID, UserName, Token]
AuthTuple = tuple[UserID, UserName, SkipUserIdCheck, Token]

NO_AUTH_TUPLE: AuthTuple = (DEFAULT_USER_UID, DEFAULT_USER_NAME, NO_USER_TOKEN)
NO_AUTH_TUPLE: AuthTuple = (
DEFAULT_USER_UID,
DEFAULT_USER_NAME,
DEFAULT_SKIP_USER_ID_CHECK,
NO_USER_TOKEN,
)


class AuthInterface(ABC): # pylint: disable=too-few-public-methods
Expand Down
3 changes: 2 additions & 1 deletion src/auth/jwk_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ def __init__(
"""Initialize the required allowed paths for authorization checks."""
self.virtual_path: str = virtual_path
self.config: JwkConfiguration = config
self.skip_userid_check = False

async def __call__(self, request: Request) -> AuthTuple:
"""Authenticate the JWT in the headers against the keys from the JWK url."""
Expand Down Expand Up @@ -190,4 +191,4 @@ async def __call__(self, request: Request) -> AuthTuple:

logger.info("Successfully authenticated user %s (ID: %s)", username, user_id)

return user_id, username, user_token
return user_id, username, self.skip_userid_check, user_token
29 changes: 24 additions & 5 deletions src/auth/k8s.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from kubernetes.config import ConfigException

from configuration import configuration
from auth.utils import extract_user_token
from auth.interface import AuthInterface
from constants import DEFAULT_VIRTUAL_PATH

Expand Down Expand Up @@ -227,18 +226,33 @@ class K8SAuthDependency(AuthInterface): # pylint: disable=too-few-public-method
def __init__(self, virtual_path: str = DEFAULT_VIRTUAL_PATH) -> None:
"""Initialize the required allowed paths for authorization checks."""
self.virtual_path = virtual_path
self.skip_userid_check = False

async def __call__(self, request: Request) -> tuple[str, str, str]:
async def __call__(self, request: Request) -> tuple[str, str, bool, str]:
"""Validate FastAPI Requests for authentication and authorization.

Args:
request: The FastAPI request object.

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
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
"""
token = extract_user_token(request.headers)
authorization_header = request.headers.get("Authorization")
if not authorization_header:
raise HTTPException(
status_code=401, detail="Unauthorized: No auth header found"
)

token = _extract_bearer_token(authorization_header)
if not token:
raise HTTPException(
status_code=401,
detail="Unauthorized: Bearer token not found or invalid",
)

user_info = get_user_info(token)
if user_info is None:
raise HTTPException(
Expand Down Expand Up @@ -267,4 +281,9 @@ async def __call__(self, request: Request) -> tuple[str, str, str]:
logger.error("API exception during SubjectAccessReview: %s", e)
raise HTTPException(status_code=403, detail="Internal server error") from e

Comment on lines 281 to 283
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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").

return user_info.user.uid, user_info.user.username, token
return (
user_info.user.uid,
user_info.user.username,
self.skip_userid_check,
token,
)
Comment on lines +284 to +289
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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?

5 changes: 3 additions & 2 deletions src/auth/noop.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ class NoopAuthDependency(AuthInterface): # pylint: disable=too-few-public-metho
def __init__(self, virtual_path: str = DEFAULT_VIRTUAL_PATH) -> None:
"""Initialize the required allowed paths for authorization checks."""
self.virtual_path = virtual_path
self.skip_userid_check = True

async def __call__(self, request: Request) -> tuple[str, str, str]:
async def __call__(self, request: Request) -> tuple[str, str, bool, str]:
"""Validate FastAPI Requests for authentication and authorization.

Args:
Expand All @@ -39,4 +40,4 @@ async def __call__(self, request: Request) -> tuple[str, str, str]:
# try to extract user ID from request
user_id = request.query_params.get("user_id", DEFAULT_USER_UID)
logger.debug("Retrieved user ID: %s", user_id)
return user_id, DEFAULT_USER_NAME, NO_USER_TOKEN
return user_id, DEFAULT_USER_NAME, self.skip_userid_check, NO_USER_TOKEN
5 changes: 3 additions & 2 deletions src/auth/noop_with_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ class NoopWithTokenAuthDependency(
def __init__(self, virtual_path: str = DEFAULT_VIRTUAL_PATH) -> None:
"""Initialize the required allowed paths for authorization checks."""
self.virtual_path = virtual_path
self.skip_userid_check = True

async def __call__(self, request: Request) -> tuple[str, str, str]:
async def __call__(self, request: Request) -> tuple[str, str, bool, str]:
"""Validate FastAPI Requests for authentication and authorization.

Args:
Expand All @@ -52,4 +53,4 @@ async def __call__(self, request: Request) -> tuple[str, str, str]:
# try to extract user ID from request
user_id = request.query_params.get("user_id", DEFAULT_USER_UID)
logger.debug("Retrieved user ID: %s", user_id)
return user_id, DEFAULT_USER_NAME, user_token
return user_id, DEFAULT_USER_NAME, self.skip_userid_check, user_token
2 changes: 1 addition & 1 deletion src/authorization/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def evaluate_role_rules(rule: JwtRoleRule, jwt_claims: dict[str, Any]) -> UserRo
@staticmethod
def _get_claims(auth: AuthTuple) -> dict[str, Any]:
"""Get the JWT claims from the auth tuple."""
_, _, token = auth
_, _, _, token = auth
if token == constants.NO_USER_TOKEN:
# No claims for guests
return {}
Expand Down
1 change: 1 addition & 0 deletions src/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
# Authentication constants
DEFAULT_VIRTUAL_PATH = "/ls-access"
DEFAULT_USER_NAME = "lightspeed-user"
DEFAULT_SKIP_USER_ID_CHECK = True
DEFAULT_USER_UID = "00000000-0000-0000-0000-000"
# default value for token when no token is provided
NO_USER_TOKEN = ""
Expand Down
7 changes: 7 additions & 0 deletions src/models/responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ class AuthorizedResponse(BaseModel):
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.
"""

user_id: str = Field(
Expand All @@ -319,6 +320,11 @@ class AuthorizedResponse(BaseModel):
description="User name",
examples=["John Doe", "Adam Smith"],
)
skip_userid_check: bool = Field(
...,
description="Whether to skip the user ID check",
examples=[True, False],
)
Comment on lines +323 to +327
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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_check to the AuthorizedResponse constructor.
  • In tests/unit/models/test_responses.py (lines 55, 67, 70), update each AuthorizedResponse(...) call to include skip_userid_check.
  • Refactor any 3-element auth tuple 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.


# provides examples for /docs endpoint
model_config = {
Expand All @@ -327,6 +333,7 @@ class AuthorizedResponse(BaseModel):
{
"user_id": "123e4567-e89b-12d3-a456-426614174000",
"username": "user1",
"skip_userid_check": False,
}
]
}
Expand Down
10 changes: 5 additions & 5 deletions tests/e2e/features/authorized_noop.feature
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Feature: Authorized endpoint API tests for the noop authentication module
Then The status code of the response is 200
And The body of the response is the following
"""
{"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user"}
{"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user","skip_userid_check": true}
"""

Scenario: Check if the authorized endpoint works when auth token is not provided
Expand All @@ -24,7 +24,7 @@ Feature: Authorized endpoint API tests for the noop authentication module
Then The status code of the response is 200
And The body of the response is the following
"""
{"user_id": "test_user","username": "lightspeed-user"}
{"user_id": "test_user","username": "lightspeed-user","skip_userid_check": true}
"""

Scenario: Check if the authorized endpoint works when user_id is not provided
Expand All @@ -34,7 +34,7 @@ Feature: Authorized endpoint API tests for the noop authentication module
Then The status code of the response is 200
And The body of the response is the following
"""
{"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user"}
{"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user","skip_userid_check": true}
"""

Scenario: Check if the authorized endpoint works when providing empty user_id
Expand All @@ -44,7 +44,7 @@ Feature: Authorized endpoint API tests for the noop authentication module
Then The status code of the response is 200
And The body of the response is the following
"""
{"user_id": "","username": "lightspeed-user"}
{"user_id": "","username": "lightspeed-user","skip_userid_check": true}
"""

Scenario: Check if the authorized endpoint works when providing proper user_id
Expand All @@ -54,5 +54,5 @@ Feature: Authorized endpoint API tests for the noop authentication module
Then The status code of the response is 200
And The body of the response is the following
"""
{"user_id": "test_user","username": "lightspeed-user"}
{"user_id": "test_user","username": "lightspeed-user","skip_userid_check": true}
"""
Loading