Role-based authorization layer #356
Conversation
WalkthroughAdds a role-based authorization framework and applies it across multiple endpoints via a new decorator. Introduces authorization models/configuration, updates authentication token handling, extends conversation ownership checks, and adapts endpoints and tests to async with DI-based auth. Adds a small dependency and configuration accessor. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant EP as FastAPI Endpoint
participant MW as authorize(action)
participant AR as get_authorization_resolvers()
participant RR as RolesResolver
participant XR as AccessResolver
participant H as Handler Logic
Client->>EP: HTTP Request (+ auth via Depends)
EP->>MW: Wrapped call
MW->>AR: Resolve (RolesResolver, AccessResolver)
AR-->>MW: (RR, XR)
MW->>RR: resolve_roles(auth)
RR-->>MW: user_roles ∪ {"*"}
MW->>XR: check_access(action, user_roles)
alt authorized
MW->>XR: get_actions(user_roles)
XR-->>MW: authorized_actions
MW->>EP: set request.state.authorized_actions
MW->>H: proceed(auth, request, ...)
H-->>Client: Response
else forbidden
MW-->>Client: HTTP 403
end
sequenceDiagram
autonumber
actor Client
participant EP as GET /conversations/{id}
participant H as Handler
participant R as request.state.authorized_actions
participant U as validate_conversation_ownership
Client->>EP: Request (+auth)
EP->>H: execute
H->>R: check Action.READ_OTHERS_CONVERSATIONS
alt others allowed
H->>U: validate(id, user_id, others_allowed=True)
else self only
H->>U: validate(id, user_id, others_allowed=False)
end
U-->>H: UserConversation | None
H-->>Client: Response / 404
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
pyproject.toml (1)
39-39: Pin jsonpath-ng major range and consider declaring pydantic explicitly
- To reduce supply-chain drift, pin an upper bound for jsonpath-ng.
- Since src/authorization/models.py imports pydantic directly, consider declaring pydantic as a first-class runtime dependency instead of relying on FastAPI’s transitive dependency.
Apply this diff to constrain jsonpath-ng’s version:
- "jsonpath-ng>=1.6.1", + "jsonpath-ng>=1.6.1,<2.0.0",And add pydantic explicitly to [project.dependencies] (outside the selected lines). Example:
[project] dependencies = [ # ...existing deps... "pydantic>=2.6.0,<3.0.0", ]src/app/endpoints/query.py (1)
37-38: Fix linter warning: group imports from the same package (utils) together.CI reports Pylint C0412 (ungrouped-imports) at Line 39. The two
authorization.*imports split the adjacentutils.*imports. Move theauthorizationimports above or below theutils.*block.[pipeline]
Apply one of the following diffs to group imports (example: move authorization imports above the utils block):-from utils.endpoints import ( - check_configuration_loaded, - get_agent, - get_system_prompt, - validate_conversation_ownership, -) -from authorization.middleware import authorize -from authorization.models import Action -from utils.mcp_headers import mcp_headers_dependency, handle_mcp_headers_with_toolgroups +from authorization.middleware import authorize +from authorization.models import Action +from utils.endpoints import ( + check_configuration_loaded, + get_agent, + get_system_prompt, + validate_conversation_ownership, +) +from utils.mcp_headers import mcp_headers_dependency, handle_mcp_headers_with_toolgroupssrc/app/endpoints/models.py (1)
68-71: Remove trailing whitespace and blank line after docstring.There's trailing whitespace on line 68 and an unnecessary blank line after the docstring.
- - # Used only by the middleware - _ = auth - + # Used only by the middleware + _ = authtests/unit/authorization/test_engine.py (2)
28-34: Consider avoiding direct access to protected members in tests.While accessing
_access_lookupdirectly tests the internal state, it couples the test to implementation details. Consider using public methods to verify the engine state.Instead of directly accessing
_access_lookup, you could verify the behavior through the public API:- assert "employee" in engine._access_lookup # pylint: disable=protected-access - assert ( - Action.QUERY in engine._access_lookup["employee"] - ) # pylint: disable=protected-access - assert ( - Action.GET_MODELS in engine._access_lookup["employee"] - ) # pylint: disable=protected-access + # Verify through public API + assert engine.check_access(["employee"], Action.QUERY) is True + assert engine.check_access(["employee"], Action.GET_MODELS) is True + assert engine.get_allowed_actions(["employee"]) == {Action.QUERY, Action.GET_MODELS}
381-381: Add test for empty JWT claims and missing fields.Consider adding test cases for edge scenarios like empty JWT claims or missing expected fields to ensure robust error handling.
Add these test cases to improve coverage:
def test_extract_roles_empty_jwt(self): """Test role extraction with empty JWT claims.""" role_rules = [ RoleRule( jsonpath="$.realm_access.roles[*]", operator=JsonPathOperator.CONTAINS, value="redhat:employees", roles=["employee"], ) ] engine = AuthorizationEngine(role_rules, []) # Empty JWT roles = engine.extract_roles_from_jwt({}) assert not roles # None values jwt_claims = {"realm_access": None} roles = engine.extract_roles_from_jwt(jwt_claims) assert not roles def test_extract_roles_missing_nested_fields(self): """Test role extraction when nested fields are missing.""" role_rules = [ RoleRule( jsonpath="$.deeply.nested.field", operator=JsonPathOperator.EQUALS, value="test", roles=["test_role"], ) ] engine = AuthorizationEngine(role_rules, []) # Partially missing path jwt_claims = {"deeply": {"other_field": "value"}} roles = engine.extract_roles_from_jwt(jwt_claims) assert not rolessrc/authorization/middleware.py (2)
56-59: Improve auth parameter detection robustness.The current auth detection logic searches for
author_authkeys. Consider making this more robust by checking for the actual dependency injection pattern or documenting why both keys are needed.auth = None - for key, value in kwargs.items(): - if key in ["auth", "_auth"] and isinstance(value, tuple) and len(value) >= 3: - auth = value - break + # Look for auth parameter injected by FastAPI dependency + auth_param_names = ["auth", "_auth"] # Support both naming conventions + for param_name in auth_param_names: + if param_name in kwargs: + value = kwargs[param_name] + if isinstance(value, tuple) and len(value) >= 3: + auth = value + break
104-104: Fix docstring to use imperative mood.The docstring should start with an imperative verb according to PEP 257.
- """Decorator to check authorization for an endpoint (sync version).""" + """Check authorization for an endpoint (sync version)."""src/authorization/engine.py (3)
17-33: Precompile JSONPath expressions in init to avoid per-request parsingparse(rule.jsonpath) runs for every token evaluation. Precompiling once in init improves latency and avoids repeated parse failures, while retaining the same behavior.
Apply this diff:
class AuthorizationEngine: @@ def __init__(self, role_rules: list[RoleRule], access_rules: list[AccessRule]): @@ self.role_rules = role_rules self.access_rules = access_rules + # Precompile JSONPath expressions so we don't parse on every request + self._compiled_role_rules: list[tuple[RoleRule, Any]] = [] + for rule in self.role_rules: + try: + expr = parse(rule.jsonpath) + self._compiled_role_rules.append((rule, expr)) + except (JsonPathParserError, JsonPathLexerError) as e: + logger.warning("Invalid JSONPath expression '%s': %s", rule.jsonpath, e) + # Build a lookup table for access rules self._access_lookup: dict[str, set[Action]] = {} for rule in access_rules: if rule.role not in self._access_lookup: self._access_lookup[rule.role] = set() self._access_lookup[rule.role].update(rule.actions) @@ - for rule in self.role_rules: + for rule, jsonpath_expr in self._compiled_role_rules: try: - # Parse the JSONPath expression - jsonpath_expr = parse(rule.jsonpath) - # Find all matches in the JWT claims matches = [match.value for match in jsonpath_expr.find(jwt_claims)] @@ - except (ValueError, TypeError, AttributeError) as e: + except (ValueError, TypeError, AttributeError) as e: logger.error("Error evaluating rule %s: %s", rule.jsonpath, e)Also applies to: 45-56, 63-67
98-141: Refactor operator evaluation to reduce branches/returns (address Pylint R0911/R0912) and support setsCurrent _evaluate_operator triggers Pylint warnings and omits common containers like set/frozenset. A dispatch-based approach improves readability, testability, and lint compliance.
Apply this diff:
def _evaluate_operator( self, match: Any, operator: JsonPathOperator, value: Any ) -> bool: """Evaluate an operator against a match and value. @@ - try: - if operator == JsonPathOperator.EQUALS: - return match == value - if operator == JsonPathOperator.NOT_EQUALS: - return match != value - if operator == JsonPathOperator.CONTAINS: - if isinstance(match, str) and isinstance(value, str): - return value in match - if isinstance(match, (list, tuple)): - return value in match - return False - if operator == JsonPathOperator.NOT_CONTAINS: - if isinstance(match, str) and isinstance(value, str): - return value not in match - if isinstance(match, (list, tuple)): - return value not in match - return True - if operator == JsonPathOperator.IN: - if isinstance(value, (list, tuple)): - return match in value - return False - if operator == JsonPathOperator.NOT_IN: - if isinstance(value, (list, tuple)): - return match not in value - return True - - logger.warning("Unknown operator: %s", operator) - return False - except (ValueError, TypeError, AttributeError) as e: - logger.warning("Error evaluating operator %s: %s", operator, e) - return False + try: + def _contains(m: Any, v: Any) -> bool: + if isinstance(m, str) and isinstance(v, str): + return v in m + if isinstance(m, (list, tuple, set, frozenset)): + return v in m + return False + + def _not_contains(m: Any, v: Any) -> bool: + if isinstance(m, str) and isinstance(v, str): + return v not in m + if isinstance(m, (list, tuple, set, frozenset)): + return v not in m + return True + + def _in(m: Any, v: Any) -> bool: + if isinstance(v, (list, tuple, set, frozenset)): + return m in v + return False + + def _not_in(m: Any, v: Any) -> bool: + if isinstance(v, (list, tuple, set, frozenset)): + return m not in v + return True + + ops = { + JsonPathOperator.EQUALS: lambda m, v: m == v, + JsonPathOperator.NOT_EQUALS: lambda m, v: m != v, + JsonPathOperator.CONTAINS: _contains, + JsonPathOperator.NOT_CONTAINS: _not_contains, + JsonPathOperator.IN: _in, + JsonPathOperator.NOT_IN: _not_in, + } + + func = ops.get(operator) + if func is None: + logger.warning("Unknown operator: %s", operator) + return False + return func(match, value) + except (ValueError, TypeError, AttributeError) as e: + logger.warning("Error evaluating operator %s: %s", operator, e) + return FalseThis should resolve the linter warnings flagged in the pipeline and extends support to set/frozenset containers.
68-77: Simplify role de-duplication with dict.fromkeysA tiny simplification; behavior is identical and order-preserving.
Apply this diff:
- unique_roles = [] - seen = set() - for role in extracted_roles: - if role not in seen: - unique_roles.append(role) - seen.add(role) - - logger.debug("Extracted roles from JWT: %s", unique_roles) - return unique_roles + unique_roles = list(dict.fromkeys(extracted_roles)) + logger.debug("Extracted roles from JWT: %s", unique_roles) + return unique_roles
📜 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 ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
pyproject.toml(1 hunks)src/app/endpoints/config.py(2 hunks)src/app/endpoints/conversations.py(3 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/metrics.py(1 hunks)src/app/endpoints/models.py(3 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/auth/jwk_token.py(2 hunks)src/authorization/__init__.py(1 hunks)src/authorization/engine.py(1 hunks)src/authorization/middleware.py(1 hunks)src/authorization/models.py(1 hunks)src/models/config.py(2 hunks)tests/unit/authorization/__init__.py(1 hunks)tests/unit/authorization/test_engine.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
src/app/endpoints/streaming_query.py (2)
src/authorization/middleware.py (1)
authorize(117-136)src/authorization/models.py (1)
Action(28-38)
src/app/endpoints/metrics.py (3)
src/authorization/middleware.py (1)
authorize(117-136)src/authorization/models.py (1)
Action(28-38)src/metrics/utils.py (1)
setup_model_metrics(13-50)
tests/unit/authorization/test_engine.py (2)
src/authorization/engine.py (4)
AuthorizationEngine(14-179)extract_roles_from_jwt(34-77)check_access(143-163)get_allowed_actions(165-179)src/authorization/models.py (4)
RoleRule(19-25)AccessRule(41-45)JsonPathOperator(8-16)Action(28-38)
src/app/endpoints/query.py (2)
src/authorization/middleware.py (1)
authorize(117-136)src/authorization/models.py (1)
Action(28-38)
src/app/endpoints/feedback.py (2)
src/authorization/middleware.py (1)
authorize(117-136)src/authorization/models.py (1)
Action(28-38)
src/app/endpoints/conversations.py (2)
src/authorization/middleware.py (1)
authorize(117-136)src/authorization/models.py (1)
Action(28-38)
src/models/config.py (1)
src/authorization/models.py (1)
AuthorizationConfiguration(48-52)
src/authorization/middleware.py (4)
src/authorization/engine.py (3)
AuthorizationEngine(14-179)extract_roles_from_jwt(34-77)check_access(143-163)src/authorization/models.py (1)
Action(28-38)src/configuration.py (2)
configuration(55-60)authentication_configuration(95-105)src/models/config.py (2)
jwk_configuration(245-252)authorization_configuration(212-214)
src/authorization/engine.py (1)
src/authorization/models.py (4)
RoleRule(19-25)AccessRule(41-45)JsonPathOperator(8-16)Action(28-38)
src/app/endpoints/config.py (2)
src/authorization/middleware.py (1)
authorize(117-136)src/authorization/models.py (1)
Action(28-38)
src/app/endpoints/models.py (6)
src/configuration.py (1)
configuration(55-60)src/authorization/middleware.py (1)
authorize(117-136)src/authorization/models.py (1)
Action(28-38)src/models/responses.py (1)
ModelsResponse(8-34)src/utils/endpoints.py (1)
check_configuration_loaded(34-40)src/auth/__init__.py (1)
get_auth_dependency(14-43)
🪛 GitHub Actions: Python linter
tests/unit/authorization/test_engine.py
[warning] 30-30: Pylint: W0212 - Access to a protected member _access_lookup of a client class (protected-access).
[warning] 33-33: Pylint: W0212 - Access to a protected member _access_lookup of a client class (protected-access).
src/app/endpoints/query.py
[warning] 39-39: Pylint: C0412 - Imports from package utils are not grouped (ungrouped-imports).
src/app/endpoints/conversations.py
[warning] 24-24: Pylint: C0412 - Imports from package utils are not grouped (ungrouped-imports).
src/authorization/engine.py
[warning] 98-98: Pylint: R0911 - Too many return statements (too-many-return-statements).
[warning] 98-98: Pylint: R0912 - Too many branches (too-many-branches).
src/app/endpoints/models.py
[warning] 54-54: Pylint: C0301 - Line too long (115/100).
[warning] 68-68: Pylint: C0303 - Trailing whitespace (trailing-whitespace).
[warning] 8-8: Pylint: C0412 - Imports from package fastapi are not grouped (ungrouped-imports).
🪛 GitHub Actions: Pydocstyle
src/authorization/middleware.py
[warning] 104-104: D401: First line should be in imperative mood (perhaps 'Decorate', not 'Decorator')
src/app/endpoints/models.py
[warning] 55-55: D202: No blank lines allowed after function docstring (found 1)
🪛 GitHub Actions: Type checks
src/authorization/middleware.py
[error] 108-108: Step 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/' failed. Mypy: Function is missing a type annotation [no-untyped-def]
[error] 122-122: Step 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/' failed. Mypy: Function is missing a type annotation [no-untyped-def]
[error] 127-127: Step 'uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/' failed. Mypy: Function is missing a type annotation [no-untyped-def]
🪛 GitHub Actions: Black
src/app/endpoints/models.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted: src/app/endpoints/models.py. Run 'black .' to fix code style issues.
⏰ 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 (16)
tests/unit/authorization/__init__.py (1)
1-1: LGTMDocstring is fine; keeping init.py in tests is harmless and can aid discovery if needed.
src/authorization/__init__.py (1)
1-1: LGTMHelpful package-level docstring. No issues.
src/auth/jwk_token.py (1)
6-6: LGTMjson import is appropriate for serializing claims in the new return value.
src/app/endpoints/metrics.py (1)
10-11: LGTM: Authorization imports are correct and scoped.The added imports align with the new authorization framework and are used below.
src/app/endpoints/query.py (1)
151-153: LGTM: Route-level authorization added for /query.Decorator usage is correct for an async handler, and FastAPI dependencies remain intact due to
@wraps.src/app/endpoints/streaming_query.py (2)
22-23: LGTM: Authorization imports added.Imports are correct and used only for the decorator. No side effects.
389-390: LGTM: Route-level authorization added for /streaming_query.Decorator stacking order is correct (authorize wraps the function before route registration). Signature is preserved via
@wraps, so FastAPI DI should continue to work.src/app/endpoints/feedback.py (2)
12-13: LGTM: Authorization imports added.Imports are minimal and accurate for the decorator usage.
84-85: LGTM: Authorization enforced for feedback POST.Decorator placement is consistent with other endpoints. The existing
assert_feedback_enabledcheck still executes within the authorized context.src/models/config.py (2)
12-12: LGTM: Importing AuthorizationConfiguration for JWK config.This enables typed embedding of RBAC configuration in the JWK block.
209-215: LGTM: Optional authorization block and convenience property on JwkConfiguration.The
authorizationfield andauthorization_configurationproperty mirror the existingjwk_config/jwk_configurationpattern and provide a consistent API path (configuration.authentication.jwk_configuration.authorization_configuration).src/app/endpoints/config.py (1)
10-11: LGTM! Authorization integration is clean and follows the pattern.The authorization decorator is correctly applied to protect the
/configendpoint with the appropriateAction.GET_CONFIGpermission.src/app/endpoints/conversations.py (2)
205-206: LGTM! Authorization correctly applied to get_conversation endpoint.The
@authorize(Action.GET_CONVERSATION)decorator is properly positioned and uses the correct action enum value.
315-316: LGTM! Authorization correctly applied to delete_conversation endpoint.The
@authorize(Action.DELETE_CONVERSATION)decorator is properly positioned and uses the correct action enum value.tests/unit/authorization/test_engine.py (1)
65-66: LGTM! Comprehensive test coverage for role extraction.The test suite thoroughly covers various JSONPath operators, edge cases, and complex scenarios. The tests validate both positive and negative cases for each operator and handle invalid JSONPath expressions gracefully.
src/authorization/middleware.py (1)
19-19: No change needed — project requires Python >=3.12 so PEP 604X | Yis supportedpyproject.toml specifies: requires-python = ">=3.12,<3.14". The codebase already uses the
|union syntax in many places, for example:
- src/authorization/middleware.py: def get_authorization_engine() -> AuthorizationEngine | None
- src/app/database.py: engine: Engine | None = None
- src/utils/endpoints.py: ...) -> UserConversation | None:
- src/app/endpoints/query.py: user_conversation: UserConversation | None
- src/models/config.py: def config(self) -> SQLiteDatabaseConfiguration | PostgreSQLDatabaseConfiguration
Action: keep
AuthorizationEngine | Noneas-is.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/app/endpoints/streaming_query.py (2)
400-401: Avoid logging sensitive configuration (API keys, tokens)logger.info("LLama stack config: %s", llama_stack_config) risks leaking secrets (e.g., API keys).
Apply this diff to remove or redact:
- logger.info("LLama stack config: %s", llama_stack_config) + # Avoid logging sensitive configuration fields (e.g., API keys) + logger.info("LLama stack configuration loaded")
492-492: Set correct SSE media type for StreamingResponseWithout media_type="text/event-stream", browsers and proxies may not treat the payload as an event stream.
Apply this diff:
- return StreamingResponse(response_generator(response)) + return StreamingResponse( + response_generator(response), + media_type="text/event-stream", + )src/app/endpoints/feedback.py (1)
84-89: authorize decorator currently swallows the endpoint call — endpoint won’t runSame issue as in root: the authorize wrapper does not execute the wrapped function, so feedback_endpoint_handler will not run and tests will receive None.
Fix in src/authorization/middleware.py by calling and returning the wrapped function after the authorization check:
def authorize(action: Action) -> Callable: def decorator(func: Callable) -> Callable: @wraps(func) async def wrapper(*args: Any, **kwargs: Any) -> Any: await _perform_authorization_check(action, kwargs) return await func(*args, **kwargs) return wrapper return decoratortests/unit/app/endpoints/test_feedback.py (1)
28-39: Async tests missing @pytest.mark.asyncio — tests will not runBoth async tests (test_assert_feedback_enabled_disabled and test_assert_feedback_enabled) lack the asyncio marker.
Apply this diff:
-async def test_assert_feedback_enabled_disabled(mocker): +@pytest.mark.asyncio +async def test_assert_feedback_enabled_disabled(mocker): @@ -async def test_assert_feedback_enabled(mocker): +@pytest.mark.asyncio +async def test_assert_feedback_enabled(mocker):tests/unit/app/endpoints/test_models.py (3)
26-32: Conflicting configuration patching — simplify to simulate “not loaded”You patch app.endpoints.models.configuration twice (first to a callable mock, then to None). The first is redundant and confusing. Keep only the None patch.
Apply this diff:
- mocker.patch( - "app.endpoints.models.configuration", - return_value=mocker.Mock(), - ) - mocker.patch("app.endpoints.models.configuration", None) + mocker.patch("app.endpoints.models.configuration", None)
118-141: cfg is created but never used — patch the module configurationYou create cfg but never patch app.endpoints.models.configuration to reference it, leaving the handler to use real/global configuration. Patch it so the test is deterministic.
@@ cfg = AppConfig() cfg.init_from_dict(config_dict) + mocker.patch("app.endpoints.models.configuration", cfg)Also applies to: 144-151
262-271: Same as above: patch configuration with cfg for determinismAdd:
@@ cfg = AppConfig() cfg.init_from_dict(config_dict) + mocker.patch("app.endpoints.models.configuration", cfg)src/app/endpoints/conversations.py (1)
205-209: Critical:authorizedecorator missing handler invocation
Thewrapperinsrc/authorization/middleware.pyonly performs the auth check and never calls or returns the wrapped endpoint function—soget_conversation_endpoint_handler(and any decorated handler) will always returnNone.• Location:
src/authorization/middleware.py, insidedef authorize(action: Action)
• Lines to update: theasync def wrapper(*args, **kwargs)blockSuggested fix:
async def wrapper(*args: Any, **kwargs: Any) -> Any: - await _perform_authorization_check(action, kwargs) + await _perform_authorization_check(action, kwargs) + # call through to the original handler and return its result + return await func(*args, **kwargs)
♻️ Duplicate comments (7)
src/models/config.py (1)
274-278: Avoid mutable list defaults in Pydantic modelsBoth fields use [] as a default, which is a shared mutable value. Prefer Field(default_factory=list) to avoid cross-instance mutation bugs.
Apply this diff:
class AuthorizationConfiguration(BaseModel): """Authorization configuration.""" - access_rules: list[AccessRule] = [] # Rules for role-based access control + access_rules: list[AccessRule] = Field(default_factory=list) # Rules for role-based access controlclass JwtConfiguration(BaseModel): """JWT configuration.""" @@ - role_rules: list[JwtRoleRule] = [] # Rules for extracting roles from JWT + role_rules: list[JwtRoleRule] = Field(default_factory=list) # Rules for extracting roles from JWTAnd add the missing import for Field:
-from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl +from pydantic import BaseModel, Field, model_validator, FilePath, AnyHttpUrlAlso applies to: 285-285
tests/unit/app/endpoints/test_root.py (1)
13-21: Be aware: current authorize() wrapper never calls the endpointGiven the implementation snippet in authorization.middleware.authorize, the wrapper awaits the auth check but does not invoke the wrapped function, causing this test to receive None. See my detailed note and fix under src/app/endpoints/streaming_query.py.
tests/unit/app/endpoints/test_metrics.py (1)
30-31: AuthTuple ordering verificationSame note as in test_root: confirm tuple field order to match endpoint unpacking.
tests/unit/app/endpoints/test_info.py (1)
41-49: Be aware: authorize() wrapper currently swallows the callIf authorization.middleware.authorize doesn’t call the wrapped function, the handler returns None. See my detailed note and fix under src/app/endpoints/streaming_query.py.
src/app/endpoints/metrics.py (1)
24-27: Previous review addressed — auth DI added to satisfy middlewareThanks for adding
auth: Annotated[AuthTuple, Depends(...)]and the@authorize(Action.GET_METRICS)guard; this resolves the earlier “missing auth kwarg” issue on /metrics.src/app/endpoints/conversations.py (1)
12-17: Group utils imports together to satisfy import ordering lintReorder to keep
utilsimports adjacent (as previously requested).from client import AsyncLlamaStackClientHolder from configuration import configuration -from app.database import get_session -from auth import get_auth_dependency -from authorization.middleware import authorize -from models.config import Action -from models.database.conversations import UserConversation +from app.database import get_session +from auth import get_auth_dependency +from authorization.middleware import authorize +from models.config import Action +from models.database.conversations import UserConversation from models.responses import ( @@ -from utils.endpoints import check_configuration_loaded, validate_conversation_ownership -from utils.suid import check_suid +from utils.endpoints import check_configuration_loaded, validate_conversation_ownership +from utils.suid import check_suidsrc/authorization/middleware.py (1)
85-95: Improve typing of the decorator using ParamSpec/TypeVar (mypy-friendly).Make the decorator type-preserving to avoid mypy Any leaks.
Apply a stronger typing pattern:
-from typing import Any, Callable, Tuple +from typing import Any, Callable, Tuple, ParamSpec, TypeVar +P = ParamSpec("P") +T = TypeVar("T") @@ -def authorize(action: Action) -> Callable: +def authorize(action: Action) -> Callable[[Callable[P, T]], Callable[P, T]]: @@ - def decorator(func: Callable) -> Callable: + def decorator(func: Callable[P, T]) -> Callable[P, T]: @wraps(func) - async def wrapper(*args: Any, **kwargs: Any) -> Any: + async def wrapper(*args: P.args, **kwargs: P.kwargs) -> T: await _perform_authorization_check(action, kwargs) - result = func(*args, **kwargs) + result = func(*args, **kwargs) if inspect.isawaitable(result): - return await result - return result + return await result # type: ignore[return-value] + return result # type: ignore[return-value]Note: the
ignorecomments placate mypy about narrowing fromT|Awaitable[T].
🧹 Nitpick comments (39)
src/authorization/models.py (1)
1-1: Empty placeholder module: either remove or re-export intended typesThis module only contains a docstring while authorization models live under src/models/config.py. Keeping an empty module can confuse readers and linters.
- Option A: Remove the file until you intend to place code here.
- Option B: Re-export selected public types from src/models/config.py to provide a stable import path (e.g., from authorization.models import Action, AccessRule, AuthorizationConfiguration, JwtRoleRule).
I can draft the re-exports if you want to consolidate import paths.
src/models/config.py (3)
216-237: Scope validators to fields for clarity and better error messagesCurrent model-level validators work, but field-level validators are a tighter fit:
- Validate jsonpath via a field_validator("jsonpath")
- Validate roles via a field_validator("roles") ensuring non-empty and uniqueness
This will attribute validation errors directly to the specific field and simplify ordering interactions between validators.
267-272: Strengthen AccessRule with basic validationConsider validating that:
- role is a non-empty string
- actions is non-empty and contains unique entries
This prevents silent misconfigurations early.
199-205: Operator support and negate logic confirmed; add test coverage and type guardsThe
_evaluate_operatormethod insrc/authorization/engine.py(lines 109–121) correctly implements all threeJsonPathOperatorcases (EQUALS,CONTAINS,IN) and applies thenegateflag after evaluation.
To ensure robustness, please:
- Add unit tests in
tests/unit/authorization/test_engine.pyfor:
JsonPathOperator.EQUALSwith both scalar and list inputsJsonPathOperator.INwith both scalar and list inputs- (Optional) Introduce runtime type checks or documentation in
src/models/config.py/engine.pyto validate thatmatchandvaluetypes align with each operator’s expectations.tests/unit/models/test_config.py (1)
522-523: Expected JSON updated with 'authorization': None — aligned with model defaultsLooks correct given model_dump_json excludes_none=False by default.
Consider adding unit tests for the new validation paths:
- invalid JSONPath in JwtRoleRule.jsonpath raises ValueError
- empty or duplicate roles rejected
- AccessRule with empty/duplicate actions rejected (if you add that validation)
I can provide test scaffolding if helpful.
src/configuration.py (1)
108-120: Fix docstring: should read 'authorization configuration'Minor typo in the new accessor docstring.
Apply this diff:
@property def authorization_configuration(self) -> AuthorizationConfiguration: - """Return authentication configuration.""" + """Return authorization configuration."""src/app/endpoints/authorized.py (1)
4-4: Remove unused Any importAfter switching to Annotated[AuthTuple, Depends(...)] the Any import is unused.
Apply this diff:
-from typing import Annotated, Any +from typing import Annotatedtests/unit/app/endpoints/test_root.py (1)
28-28: Strengthen assertionConsider asserting key properties (e.g., status code or content) to catch regressions beyond “not None.”
tests/unit/app/endpoints/test_info.py (1)
56-58: Strengthen assertions and verify tuple order
- Prefer asserting exact values to catch regressions.
- Ensure the AuthTuple ordering matches the endpoint’s unpacking and your test data.
Apply this diff to tighten assertions:
- assert response is not None - assert response.name is not None - assert response.version is not None + assert response is not None + assert response.name == "foo" + from version import __version__ # local import to avoid global side effects + assert response.version == __version__Also confirm AuthTuple order as noted in other comments.
src/app/endpoints/streaming_query.py (4)
563-568: Header construction falls back to Bearer {token}; ensure token is strGiven the unpacking issue above, validate token is a non-empty string before constructing headers to avoid malformed Authorization values.
Apply this guard:
- if not mcp_headers and token: + if not mcp_headers and isinstance(token, str) and token: for mcp_server in configuration.mcp_servers: mcp_headers[mcp_server.url] = { "Authorization": f"Bearer {token}", }
533-534: Consider redacting or truncating large prompts in logsLogging the full system prompt can leak sensitive content and spam logs.
For example:
- logger.debug("Using system prompt: %s", system_prompt) + logger.debug("Using system prompt (len=%d)", len(system_prompt or ""))
424-505: Add a catch-all error path to surface unexpected failuresOnly APIConnectionError is handled. Consider a broad except Exception to increment failure metrics and return a 500, with sanitized message.
Happy to provide a patch if you want to include this now.
388-396: Add a negative authorization test for STREAMING_QUERYGiven this endpoint now gates on Action.STREAMING_QUERY, add a unit test that denies access (e.g., access_resolver.check_access returns False) and asserts the correct HTTPException is raised.
I can scaffold the test mirroring the pattern used in other endpoint tests. Want me to push that?
src/app/endpoints/info.py (1)
20-21: Consider lazy auth dependency resolutionauth_dependency = get_auth_dependency() runs at import time, which can read configuration before it’s loaded in some test setups. A small refactor can defer evaluation to request time.
Example:
# middleware expects a callable; wrap the factory to defer evaluation def _auth_dep(): return get_auth_dependency() # then in the signature: auth: Annotated[AuthTuple, Depends(_auth_dep)],src/app/endpoints/root.py (1)
17-17: Avoid import-time side effects when resolving auth dependencyCalling get_auth_dependency() at import time can create hard-to-test side effects. Prefer resolving it within Depends for consistency with other endpoints.
Apply this diff:
- auth_dependency = get_auth_dependency() + # Resolve at dependency-injection time to avoid import-time side effectsAnd update the parameter:
- auth: Annotated[AuthTuple, Depends(auth_dependency)], + auth: Annotated[AuthTuple, Depends(get_auth_dependency())],Also applies to: 783-784
tests/unit/app/endpoints/test_health.py (1)
16-28: DRY up authorization resolver mocks across testsThe same resolver-mocking boilerplate is repeated three times. Factor it into a fixture to improve readability and maintainability.
Example fixture:
# conftest.py or this module import pytest @pytest.fixture def allow_all_authorization(mocker): mock_resolvers = mocker.patch("authorization.middleware.get_authorization_resolvers") mock_role_resolver = mocker.AsyncMock() mock_access_resolver = mocker.AsyncMock() mock_role_resolver.resolve_roles.return_value = [] mock_access_resolver.check_access.return_value = True mock_resolvers.return_value = (mock_role_resolver, mock_access_resolver) return mock_role_resolver, mock_access_resolverThen use allow_all_authorization in tests.
Also applies to: 53-65, 96-108
src/app/endpoints/feedback.py (3)
27-27: Resolve auth dependency inside Depends for consistency and testabilityAvoid module-level get_auth_dependency() calls; resolve within Depends. This matches patterns in other endpoints and avoids import-time coupling.
Apply this diff:
-auth_dependency = get_auth_dependency() @@ - auth: Annotated[AuthTuple, Depends(auth_dependency)], + auth: Annotated[AuthTuple, Depends(get_auth_dependency())],Also applies to: 87-87
108-111: Avoid logging full feedback payload (risk of PII leakage)Logging the entire FeedbackRequest can leak sensitive content (user questions, responses). Log only non-sensitive metadata.
- logger.debug("Feedback received %s", str(feedback_request)) + logger.debug( + "Feedback received: conversation_id=%s, sentiment=%s, categories=%s", + getattr(feedback_request, "conversation_id", None), + getattr(feedback_request, "sentiment", None), + getattr(feedback_request, "categories", None), + )[security]
95-101: Docstring arg name mismatchThe docstring refers to ensure_feedback_enabled, but the parameter name is _ensure_feedback_enabled. Align for clarity.
- ensure_feedback_enabled: The feedback handler (FastAPI Depends) that + _ensure_feedback_enabled: The feedback handler (FastAPI Depends) thattests/unit/app/endpoints/test_feedback.py (2)
65-78: Extract repeated authorization mocks into a fixtureSame pattern is repeated twice. Factor into a shared fixture (see suggestion in health tests) to reduce duplication.
Also applies to: 98-110
88-92: Unify AuthTuple shape across testsElsewhere, auth is often ("user_id", "token", {}) while here it's ("user_id", "username", "token"). Since only user_id is used, tests pass, but the inconsistency is confusing. Standardize the tuple form project-wide (and/or provide a helper).
tests/unit/app/endpoints/test_models.py (1)
16-25: Factor repeated authorization mocks into a fixtureThe same resolver-mocking boilerplate is repeated across five tests. Extract into a shared fixture to reduce duplication.
See fixture example in the health tests comment.
Also applies to: 50-59, 106-117, 162-170, 219-228
src/app/endpoints/config.py (2)
18-18: Avoid initializing the auth dependency at import-time
auth_dependency = get_auth_dependency()runs during module import and may depend on configuration state. Prefer lazy init to reduce import-time coupling and ease testing.Option A (preferred): inject the factory directly.
- auth_dependency = get_auth_dependency() + # Defer resolution to request-time + # (get_auth_dependency() should be fast or internally cached)Then change the parameter to:
- auth: Annotated[AuthTuple, Depends(auth_dependency)], + auth: Annotated[AuthTuple, Depends(get_auth_dependency)],
78-80: Drop the no-op assignment by naming the parameter_authTiny cleanup to avoid an extra line.
-async def config_endpoint_handler( - auth: Annotated[AuthTuple, Depends(auth_dependency)], +async def config_endpoint_handler( + _auth: Annotated[AuthTuple, Depends(auth_dependency)], _request: Request, ) -> Configuration: @@ - # Used only for authorization - _ = auth + # Authorization handled by decorator; auth injected to satisfy middlewaresrc/app/endpoints/health.py (4)
93-95: Avoid the no-op assignment; rename parameter to_authUse a leading underscore parameter name and remove the extra assignment.
-async def readiness_probe_get_method( - auth: Annotated[AuthTuple, Depends(auth_dependency)], +async def readiness_probe_get_method( + _auth: Annotated[AuthTuple, Depends(auth_dependency)], response: Response, ) -> ReadinessResponse: @@ - # Used only for authorization - _ = auth + # Authorization is performed by the decorator
112-112: Return full provider statuses for observabilityYou’re returning only
unhealthy_providers. For better diagnostics (and to align typical readiness schemas), consider returning all provider statuses and keeping “reason” narrowed to unhealthy names.- return ReadinessResponse(ready=ready, reason=reason, providers=unhealthy_providers) + return ReadinessResponse(ready=ready, reason=reason, providers=provider_statuses)
127-131: Make liveness handler async for consistency with the async decoratorMinor consistency nit: the decorator returns an async wrapper; standardize the handler to async.
-@router.get("/liveness", responses=get_liveness_responses) -@authorize(Action.INFO) -def liveness_probe_get_method( - auth: Annotated[AuthTuple, Depends(auth_dependency)], -) -> LivenessResponse: +@router.get("/liveness", responses=get_liveness_responses) +@authorize(Action.INFO) +async def liveness_probe_get_method( + _auth: Annotated[AuthTuple, Depends(auth_dependency)], +) -> LivenessResponse: @@ - # Used only for authorization - _ = auth + # Authorization handled by decorator
28-29: Defer auth dependency resolution to request-timeAs in config.py, consider injecting the factory rather than creating the dependency at import time to reduce configuration coupling.
-auth_dependency = get_auth_dependency() +# Prefer lazy injection; FastAPI will call the factory at request-timeUpdate parameter sites to:
- auth: Annotated[AuthTuple, Depends(auth_dependency)], + auth: Annotated[AuthTuple, Depends(get_auth_dependency)],src/app/endpoints/metrics.py (3)
38-40: Rename the injected parameter to_authand drop no-op assignmentMinor cleanup.
-async def metrics_endpoint_handler( - auth: Annotated[AuthTuple, Depends(auth_dependency)], +async def metrics_endpoint_handler( + _auth: Annotated[AuthTuple, Depends(auth_dependency)], _request: Request, ) -> PlainTextResponse: @@ - # Used only for authorization - _ = auth + # Authorization handled by decorator
41-44: Avoid doing model metrics setup on every request
setup_model_metrics()appears to perform remote calls; running it each time can be costly. Gate it with a one-time flag or move to an app startup hook.Option A (per-endpoint lazy one-time init):
# At module top: import asyncio _metrics_ready = asyncio.Event() # In handler: if not _metrics_ready.is_set(): await setup_model_metrics() _metrics_ready.set()Option B (preferred): perform setup during app startup/lifespan to keep the hot path fast, then expose metrics.
19-19: Defer auth dependency resolution to request-timeSame note as other endpoints: consider injecting the factory rather than caching the dependency at import-time.
-auth_dependency = get_auth_dependency() +# Prefer lazy injection via Depends(get_auth_dependency)And update the parameter accordingly.
tests/unit/app/endpoints/test_config.py (2)
23-29: Redundant patching — choose one configuration overrideYou patch both
configuration._configuration = Noneand then the entireconfigurationsymbol toNone. The first is unnecessary once the second happens (and may mislead future readers).- mocker.patch( - "app.endpoints.config.configuration._configuration", - new=None, - ) - mocker.patch("app.endpoints.config.configuration", None) + mocker.patch("app.endpoints.config.configuration", None)
90-93: Add a negative authorization test (403) to cover the denial pathTo fully exercise the decorator path, add a test where
check_accessreturnsFalseand assert a 403/HTTPException from the wrapper.I can draft this test if you want it added to the suite.
src/app/endpoints/conversations.py (3)
151-154: Type the auth dependency as AuthTuple with AnnotatedTighten typing for clarity and editor support.
-async def get_conversations_list_endpoint_handler( - auth: Any = Depends(auth_dependency), +async def get_conversations_list_endpoint_handler( + auth: Annotated[tuple[str, str | None, dict], Depends(auth_dependency)], ) -> ConversationsListResponse:Or, if available:
-from typing import Any -from auth import get_auth_dependency +from typing import Annotated +from auth.interface import AuthTuple @@ - auth: Any = Depends(auth_dependency), + auth: Annotated[AuthTuple, Depends(auth_dependency)],
206-209: Type the auth dependency and/or rename to_authAlign with the rest of the codebase and avoid unused variable lines.
-async def get_conversation_endpoint_handler( - conversation_id: str, - auth: Any = Depends(auth_dependency), +async def get_conversation_endpoint_handler( + conversation_id: str, + _auth: Annotated[AuthTuple, Depends(auth_dependency)], ) -> ConversationResponse: @@ - user_id, _, _ = auth + user_id, _, _ = _auth
316-319: Same typing/rename suggestion for delete handler-async def delete_conversation_endpoint_handler( - conversation_id: str, - auth: Any = Depends(auth_dependency), +async def delete_conversation_endpoint_handler( + conversation_id: str, + _auth: Annotated[AuthTuple, Depends(auth_dependency)], ) -> ConversationDeleteResponse: @@ - user_id, _, _ = auth + user_id, _, _ = _authsrc/authorization/middleware.py (1)
6-6: Remove unused import.
asynciois unused after the changes.Apply:
-import asynciosrc/authorization/engine.py (2)
79-91: Optional: compile JSONPath expressions once per rule and cache.
parse(rule.jsonpath)on every evaluation is unnecessary work; compile once in__init__and reuse to reduce overhead and avoid re-parsing errors at runtime.Example approach:
- In
__init__, buildself._compiled_paths: list[Any] = [parse(r.jsonpath) for r in role_rules].- Make
evaluate_role_rulesan instance method (drop@staticmethod) and use the compiled expression at the corresponding index.- Alternatively, wrap
parse(...)withfunctools.lru_cachekeyed by the JSONPath string.If you want, I can provide a concrete patch consistent with your models’ validators.
58-66: Nit: docstring grammar.-class JwtRolesResolver(RolesResolver): # pylint: disable=too-few-public-methods - """Engine for extract roles from JWT claims using JSONPath rules.""" +class JwtRolesResolver(RolesResolver): # pylint: disable=too-few-public-methods + """Engine to extract roles from JWT claims using JSONPath rules."""
📜 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 (25)
src/app/endpoints/authorized.py(2 hunks)src/app/endpoints/config.py(3 hunks)src/app/endpoints/conversations.py(4 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/health.py(4 hunks)src/app/endpoints/info.py(3 hunks)src/app/endpoints/metrics.py(2 hunks)src/app/endpoints/models.py(3 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/root.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/authorization/engine.py(1 hunks)src/authorization/middleware.py(1 hunks)src/authorization/models.py(1 hunks)src/configuration.py(2 hunks)src/models/config.py(3 hunks)tests/unit/app/endpoints/test_config.py(3 hunks)tests/unit/app/endpoints/test_feedback.py(4 hunks)tests/unit/app/endpoints/test_health.py(5 hunks)tests/unit/app/endpoints/test_info.py(2 hunks)tests/unit/app/endpoints/test_metrics.py(2 hunks)tests/unit/app/endpoints/test_models.py(10 hunks)tests/unit/app/endpoints/test_root.py(1 hunks)tests/unit/authorization/test_engine.py(1 hunks)tests/unit/models/test_config.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/authorization/test_engine.py
- src/app/endpoints/query.py
- src/app/endpoints/models.py
🧰 Additional context used
🧬 Code Graph Analysis (18)
tests/unit/app/endpoints/test_config.py (3)
src/authorization/engine.py (6)
resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)check_access(23-24)check_access(30-34)check_access(146-163)src/app/endpoints/config.py (1)
config_endpoint_handler(65-84)src/configuration.py (2)
AppConfig(28-143)init_from_dict(51-53)
tests/unit/app/endpoints/test_root.py (2)
src/app/endpoints/root.py (1)
root_endpoint_handler(782-791)src/authorization/engine.py (6)
resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)check_access(23-24)check_access(30-34)check_access(146-163)
src/configuration.py (1)
src/models/config.py (1)
AuthorizationConfiguration(274-277)
src/app/endpoints/root.py (3)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(85-95)src/models/config.py (2)
config(93-99)Action(239-264)
tests/unit/app/endpoints/test_metrics.py (2)
src/app/endpoints/metrics.py (1)
metrics_endpoint_handler(24-44)src/authorization/engine.py (6)
resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)check_access(23-24)check_access(30-34)check_access(146-163)
tests/unit/app/endpoints/test_feedback.py (2)
src/authorization/engine.py (6)
resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)check_access(23-24)check_access(30-34)check_access(146-163)src/app/endpoints/feedback.py (1)
feedback_endpoint_handler(85-123)
tests/unit/app/endpoints/test_models.py (2)
src/authorization/engine.py (6)
resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)check_access(23-24)check_access(30-34)check_access(146-163)src/app/endpoints/models.py (1)
models_endpoint_handler(54-105)
tests/unit/app/endpoints/test_info.py (3)
src/app/endpoints/info.py (1)
info_endpoint_handler(33-49)src/configuration.py (3)
configuration(56-61)AppConfig(28-143)init_from_dict(51-53)src/authorization/engine.py (6)
resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)check_access(23-24)check_access(30-34)check_access(146-163)
src/app/endpoints/health.py (3)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(85-95)src/models/config.py (2)
config(93-99)Action(239-264)
tests/unit/app/endpoints/test_health.py (2)
src/authorization/engine.py (6)
resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)check_access(23-24)check_access(30-34)check_access(146-163)src/app/endpoints/health.py (2)
readiness_probe_get_method(82-112)liveness_probe_get_method(129-141)
src/app/endpoints/info.py (5)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(85-95)src/configuration.py (1)
configuration(56-61)src/models/config.py (2)
config(93-99)Action(239-264)src/models/responses.py (1)
InfoResponse(82-118)
src/app/endpoints/streaming_query.py (2)
src/authorization/middleware.py (1)
authorize(85-95)src/models/config.py (2)
config(93-99)Action(239-264)
src/authorization/engine.py (1)
src/models/config.py (5)
config(93-99)JwtRoleRule(207-236)AccessRule(267-271)JsonPathOperator(199-204)Action(239-264)
src/app/endpoints/metrics.py (4)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(85-95)src/models/config.py (2)
config(93-99)Action(239-264)src/metrics/utils.py (1)
setup_model_metrics(13-50)
src/app/endpoints/config.py (4)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(85-95)src/models/config.py (3)
config(93-99)Action(239-264)Configuration(371-388)src/utils/endpoints.py (1)
check_configuration_loaded(34-40)
src/app/endpoints/feedback.py (3)
src/authorization/middleware.py (1)
authorize(85-95)src/models/config.py (2)
config(93-99)Action(239-264)src/models/requests.py (1)
FeedbackRequest(239-381)
src/app/endpoints/conversations.py (5)
src/app/database.py (1)
get_session(34-40)src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(85-95)src/models/config.py (2)
config(93-99)Action(239-264)src/models/database/conversations.py (1)
UserConversation(11-36)
src/authorization/middleware.py (3)
src/authorization/engine.py (12)
AccessResolver(19-24)GenericAccessResolver(125-163)JwtRolesResolver(58-122)NoopAccessResolver(27-34)NoopRolesResolver(49-55)RolesResolver(41-46)check_access(23-24)check_access(30-34)check_access(146-163)resolve_roles(45-46)resolve_roles(52-55)resolve_roles(69-76)src/models/config.py (2)
config(93-99)Action(239-264)src/configuration.py (3)
configuration(56-61)authorization_configuration(109-119)authentication_configuration(96-106)
🔇 Additional comments (20)
src/models/config.py (3)
381-381: Top-level authorization field addition is appropriateOptional authorization on the Configuration model matches the new accessor and the test expectations. Serialization including "authorization": null is intentional and verified by tests.
7-8: No action needed:jsonpath-ngdependency is declaredThe
pyproject.tomlalready includes
- Line 39:
"jsonpath-ng>=1.6.1"So the imports in
src/models/config.pyand other modules will resolve at runtime.
239-265: ADMIN wildcard behavior verified in authorization engineThe authorization logic in src/authorization/engine.py already enforces that:
- Access rules cannot mix
Action.ADMINwith other actions (ValueError on lines 132–135).check_accessgrants any action when the user hasAction.ADMIN(lines 148–152).No further changes are needed.
tests/unit/models/test_config.py (1)
472-472: Test asserts presence of 'authorization' key — good coverage of serialization contractThis ensures backward compatibility of the JSON dump and validates the new surface area.
src/configuration.py (1)
12-12: Accessor import is correct and matches the new Configuration fieldImporting AuthorizationConfiguration here keeps the accessor typed and discoverable.
src/app/endpoints/authorized.py (1)
35-36: Type-annotated auth dependency improves clarity and toolingUsing Annotated[AuthTuple, Depends(...)] makes the contract explicit and aids static analysis. Handler logic remains unchanged.
tests/unit/app/endpoints/test_root.py (1)
9-11: Async test conversion looks goodUsing pytest.mark.asyncio and awaiting the handler aligns with the async endpoint changes.
tests/unit/app/endpoints/test_metrics.py (1)
12-21: Auth mocking pattern is consistentPatching get_authorization_resolvers with AsyncMocks matches the new auth flow.
tests/unit/app/endpoints/test_info.py (2)
32-34: Config additions are appropriate for exercising auth pathIncluding minimal authorization and authentication config keeps the test aligned with the new flow.
38-40: Mocking configuration access is correctPatching configuration.configuration to the prepared AppConfig instance is the right hook for the handler.
src/app/endpoints/streaming_query.py (2)
393-395: Auth and MCP headers DI is consistent with the new patternInjecting auth and mcp_headers via Depends follows the refactor across endpoints.
402-402: Auth tuple unpacking likely wrong (token ends up being claims dict)Tests pass auth as ("test_user", "token", {}), implying (user_id, token, claims). The current unpacking treats the second element as username and assigns the third (claims dict) to token, leading to Authorization: Bearer {}.
Apply this diff (assuming the intended order is (user_id, token, claims)):
- user_id, _user_name, token = auth + user_id, token, _claims = authIf the canonical order differs, standardize it in AuthTuple and update all call sites accordingly.
To verify canonical order, run the script in my earlier comment to find the AuthTuple definition and all unpacking sites.
src/app/endpoints/info.py (1)
49-49: LGTM: returns the expected InfoResponseReturning configuration.configuration.name and version is correct and matches the test’s expectations.
src/app/endpoints/root.py (1)
781-785: INFO action not granted in the provided RBAC config — likely 403 for everyonePer PR description, access_rules include query, streaming_query, get_models, feedback, but not info. With this guard, the root page will be inaccessible unless the user has ADMIN or a role that implicitly allows all.
Options:
- Add INFO to the allowed actions for redhat_employee (or a public role), or
- Remove @authorize(Action.INFO) from the root endpoint if it should remain public.
Please confirm intended behavior and adjust config or decorator accordingly.
tests/unit/app/endpoints/test_models.py (1)
87-91: Test intent vs setup mismatch for “improper Llama Stack configuration”You build cfg = AppConfig() with a seemingly valid llama_stack section, then patch configuration to None. The assertion expects "LLama stack is not configured" but the setup simulates "configuration not loaded".
Please clarify intent:
- If testing “not configured”, patch configuration to a config lacking llama_stack settings.
- If testing “not loaded”, assert the corresponding error message.
Example adjustment (if testing “not configured”):
- mocker.patch( - "app.endpoints.models.configuration", - return_value=None, - ) + # Provide a config missing llama_stack settings to trigger 'not configured' + bad_cfg = AppConfig() + bad_cfg.init_from_dict({ "name": "test", "service": {...} }) + mocker.patch("app.endpoints.models.configuration", bad_cfg)src/app/endpoints/config.py (1)
62-85: LGTM — endpoint now properly enforces GET_CONFIGAuth DI, decorator use, and return typing look consistent with the broader pattern.
tests/unit/app/endpoints/test_config.py (2)
10-22: Good: mocks isolate authorization while exercising endpoint codeMocking
get_authorization_resolvers()to allow access keeps these tests focused on endpoint behavior.
42-54: Good: positive-path test constructs a real AppConfig and injects itThis is a solid pattern that exercises the real model shapes and ensures the return value is the loaded configuration.
src/app/endpoints/conversations.py (1)
155-157: LGTM — ownership validation + user scoping is clearConfiguration check, ID validation, DB scoping by
user_id, and ownership validation provide a solid defense-in-depth layer.src/authorization/engine.py (1)
146-163: LGTM: access evaluation and admin override are clear.Lookup table and admin short-circuit look correct and efficient.
omertuc
force-pushed
the
auth
branch
11 times, most recently
from
76a0281 to
20b5d62
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
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/authorized.py (1)
47-48: Update Test AuthTuple Order and TypesThe code in
authorized.pynow unpacksauthas(user_id, user_name, token), so tests must construct the tuple in that same order and use a string for the token. Currently, many tests do:auth = ("test_user", "token", {})which assigns
user_name="token"and drops a dict intotoken. Please update all affected tests to:
- Construct
authas(user_id: str, user_name: str, token: str)- Replace the third-element
{}with a dummy string, e.g."token"Impacted test files:
- tests/unit/app/endpoints/test_config.py (lines 27, 75)
- tests/unit/app/endpoints/test_health.py (lines 35, 69, 85)
- tests/unit/app/endpoints/test_metrics.py (line 23)
- tests/unit/app/endpoints/test_info.py (line 49)
- tests/unit/app/endpoints/test_models.py (lines 32, 83, 127, 180, 232)
- tests/unit/app/endpoints/test_root.py (line 15)
Example correction:
- auth = ("test_user", "token", {}) + auth = ("test_user", "test_user_name", "token")Consider adding a shared test helper like:
def make_auth(user_id="test_user", user_name="test_user_name", token="token"): return (user_id, user_name, token)to keep tuple construction consistent across tests.
src/app/endpoints/streaming_query.py (1)
551-553: Honor QUERY_OTHERS_CONVERSATIONS in streaming path (parity with /query)./query allows continuing others’ conversations when Action.QUERY_OTHERS_CONVERSATIONS is present. The streaming variant should apply the same rule.
Apply this diff:
- user_conversation = validate_conversation_ownership( - user_id=user_id, conversation_id=query_request.conversation_id - ) + allowed = Action.QUERY_OTHERS_CONVERSATIONS in getattr( + request.state, "authorized_actions", set() + ) + user_conversation = validate_conversation_ownership( + user_id=user_id, + conversation_id=query_request.conversation_id, + others_allowed=allowed, + )src/app/endpoints/conversations.py (1)
323-395: DELETE endpoint: ownership check hook and Llama Stack delete are correct; fix docs mismatch
- Behavior when no sessions are found (return success) matches requirement to be idempotent. Good.
- The responses example (conversation_delete_responses) uses a “message” field, while the handler returns “response” in ConversationDeleteResponse. Align the documented example with the actual schema to avoid confusing clients/SDKs.
conversation_delete_responses: dict[int | str, dict[str, Any]] = { "conversation_id": "123e4567-e89b-12d3-a456-426614174000", "success": True, - "message": "Conversation deleted successfully", + "response": "Conversation deleted successfully", },
♻️ Duplicate comments (6)
src/auth/jwk_token.py (1)
192-192: Auth tuple now returns JSON claims as 3rd element — expose parsed claims on request.state for zero-cost reuseGood with the new representation, but many downstream components will want the dict, not the JSON string. To avoid repeated json.loads and ease future migrations, also stash the parsed claims on the request.
Apply this minimal diff:
logger.info("Successfully authenticated user %s (ID: %s)", username, user_id) - return user_id, username, json.dumps(claims) + # Expose parsed claims for downstream consumers (middleware, endpoints) + request.state.jwt_claims = dict(claims) + return user_id, username, json.dumps(claims)tests/unit/app/endpoints/test_root.py (1)
15-15: Fix AuthTuple order: (UserID, UserName, Token)The current tuple assigns user_name="token" and token={} (dict). Use a string token in the 3rd slot.
Apply:
- auth = ("test_user", "token", {}) + auth = ("test_user", "test_user_name", "token")tests/unit/app/endpoints/test_metrics.py (1)
15-17: Patch awaited function with AsyncMocksetup_model_metrics is awaited by the handler; patching with a plain MagicMock/return_value=None will cause “object is not awaitable”.
Apply:
- mock_setup_metrics = mocker.patch( - "app.endpoints.metrics.setup_model_metrics", return_value=None - ) + mock_setup_metrics = mocker.patch( + "app.endpoints.metrics.setup_model_metrics", + new=mocker.AsyncMock(return_value=None), + )src/authorization/middleware.py (3)
23-25: Cache invalidation on config reloadget_authorization_resolvers is cached; ensure config reloads call get_authorization_resolvers.cache_clear() to prevent stale rules.
26-28: Critical: configuration accessors aren’t called (runtime AttributeError)authorization_configuration and authentication_configuration are methods; add parentheses.
Apply:
- authorization_cfg = configuration.authorization_configuration - authentication_config = configuration.authentication_configuration + authorization_cfg = configuration.authorization_configuration() + authentication_config = configuration.authentication_configuration()
69-86: Return 401 on invalid/malformed tokens during role resolutionFailures in resolve_roles currently bubble to 500. Treat token/claims parsing issues as authentication errors.
Apply:
- user_roles = await role_resolver.resolve_roles(auth) | everyone_roles + try: + user_roles = await role_resolver.resolve_roles(auth) + except Exception as exc: # Narrow to a RoleResolutionError if available + logger.warning("Failed to resolve roles from auth token: %s", exc) + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid authentication token", + ) from exc + user_roles |= everyone_rolesIf you have a dedicated RoleResolutionError in authorization.resolvers, catch that specifically.
🧹 Nitpick comments (19)
tests/unit/auth/test_jwk_token.py (1)
176-180: Nit: make payload split more robust and add return type hintGuard against malformed tokens and clarify intent for readers.
Apply this diff:
-def get_claims(token: str): +def get_claims(token: str) -> dict: """Extract claims from a JWT token without validating it.""" - payload = token.split(".")[1] + parts = token.split(".", 2) + assert len(parts) == 3, "Malformed JWT used in tests" + payload = parts[1] padded = payload + "=" * (-len(payload) % 4) return json.loads(base64.urlsafe_b64decode(padded))src/utils/endpoints.py (2)
22-29: Clarify “others_allowed” semantics and provenanceThe flag meaning is clear, but it’s security-sensitive. Please add to the docstring that this must only be set from authorization decisions (e.g., request.state.authorized_actions) and never from user input.
Apply this diff:
def validate_conversation_ownership( user_id: str, conversation_id: str, others_allowed: bool = False ) -> UserConversation | None: - """Validate that the conversation belongs to the user. + """Validate that the conversation belongs to the user. Validates that the conversation with the given ID belongs to the user with the given ID. If `others_allowed` is True, it allows conversations that do not belong to the user, - which is useful for admin access. + which is useful for admin access. + + SECURITY: others_allowed must only be derived from server-side authorization + (e.g., request.state.authorized_actions) — never from client input. """
31-41: Minor simplification: collapse query constructionCurrent temp variables are fine, but this can be expressed more succinctly.
Apply this diff:
- conversation_query = session.query(UserConversation) - - filtered_conversation_query = ( - conversation_query.filter_by(id=conversation_id) - if others_allowed - else conversation_query.filter_by(id=conversation_id, user_id=user_id) - ) - - conversation: UserConversation | None = filtered_conversation_query.first() + query = session.query(UserConversation) + filters = {"id": conversation_id} if others_allowed else {"id": conversation_id, "user_id": user_id} + conversation: UserConversation | None = query.filter_by(**filters).first()src/models/config.py (2)
224-243: Pre-compile JSONPath once for each rule to avoid repeated parsing at runtimeIf resolvers re-parse self.jsonpath per request, it adds overhead. Store the compiled JSONPath object on the model during validation for reuse.
Apply this diff to materialize a compiled attribute:
class JwtRoleRule(BaseModel): """Rule for extracting roles from JWT claims.""" jsonpath: str # JSONPath expression to evaluate against the JWT payload operator: JsonPathOperator # Comparison operator negate: bool = False # If True, negate the rule value: Any # Value to compare against roles: list[str] # Roles to assign if rule matches + # Non-serialized field to cache compiled JSONPath + _compiled_jsonpath: Any = None # set in validator @model_validator(mode="after") def check_jsonpath(self) -> Self: """Verify that the JSONPath expression is valid.""" try: - jsonpath_ng.parse(self.jsonpath) - return self + self._compiled_jsonpath = jsonpath_ng.parse(self.jsonpath) + return self except JSONPathError as e: raise ValueError( f"Invalid JSONPath expression: {self.jsonpath}: {e}" ) from eNote: Mark _compiled_jsonpath as private to avoid being included in dumps; resolvers can use rule._compiled_jsonpath when evaluating.
245-260: Role normalization and validation improvements (optional)Consider normalizing role names (e.g., lowercasing and trimming) and validating against a convention to prevent duplicates that differ only by case or whitespace.
Apply this diff:
@model_validator(mode="after") def check_roles(self) -> Self: """Ensure that at least one role is specified.""" - if not self.roles: + if not self.roles: raise ValueError("At least one role must be specified in the rule") - if len(self.roles) != len(set(self.roles)): + # Normalize roles to avoid duplicates that differ only by case/whitespace + self.roles = [r.strip().lower() for r in self.roles] + if len(self.roles) != len(set(self.roles)): raise ValueError("Roles must be unique in the rule")src/app/endpoints/authorized.py (1)
33-36: Consider enforcing a non-privileged action on /authorizedIf you want request.state.authorized_actions populated consistently and to gate this endpoint behind “logged-in user” permissions, decorate with a low-friction action like Action.INFO.
Apply:
from fastapi import APIRouter, Depends +from authorization.middleware import authorize +from models.config import Action @router.post("/authorized", responses=authorized_responses) -async def authorized_endpoint_handler( +@authorize(Action.INFO) +async def authorized_endpoint_handler( auth: Annotated[AuthTuple, Depends(auth_dependency)], ) -> AuthorizedResponse:tests/unit/app/endpoints/test_metrics.py (1)
23-24: Fix AuthTuple order in this test as wellKeep consistency with (UserID, UserName, Token); use a string token.
Apply:
- auth = ("test_user", "token", {}) + auth = ("test_user", "test_user_name", "token")tests/unit/app/endpoints/test_models.py (1)
90-94: Optional: patch the imported symbol to avoid import-path ambiguityIn one test you patch client.AsyncLlamaStackClientHolder.get_client, and in another you patch app.endpoints.models.AsyncLlamaStackClientHolder. Prefer patching the symbol where it’s used (app.endpoints.models.AsyncLlamaStackClientHolder) consistently to avoid surprises if import semantics change.
src/authorization/middleware.py (1)
102-106: Docstring nit: decorator isn’t “async version”The wrapper adapts to async handlers but the decorator itself isn’t an “async version.”
Apply:
-def authorize(action: Action) -> Callable: - """Check authorization for an endpoint (async version).""" +def authorize(action: Action) -> Callable: + """Check authorization for an endpoint before invoking the handler."""src/app/endpoints/streaming_query.py (1)
653-653: Set correct SSE content type for clients.The response body is SSE-formatted strings (“data: …”), so advertise text/event-stream to avoid client-side parsing issues.
Apply this diff:
- return StreamingResponse(response_generator(response)) + return StreamingResponse( + response_generator(response), + media_type="text/event-stream", + )src/app/endpoints/config.py (1)
78-83: Intentional unused parameters: keep for middleware compatibility.Keeping auth/request ensures the authorization layer can extract context, even if the handler doesn’t use them. The explicit “unused” assignments are fine to satisfy linters.
If you prefer, you can annotate with a short comment to make the intent permanent:
“# kept for authorization middleware context.”tests/unit/app/endpoints/test_conversations.py (1)
591-695: Optional: factor out repetitive authorization mockingmock_authorization_resolvers(mocker) is invoked in most tests; consider an autouse fixture at module scope to reduce repetition and ensure consistency.
+@pytest.fixture(autouse=True) +def _auto_mock_authorization(mocker): + # Allow all actions across this module’s tests + from tests.unit.utils.auth_helpers import mock_authorization_resolvers + mock_authorization_resolvers(mocker)tests/unit/app/endpoints/test_query.py (2)
126-181: Helper-driven happy path covers metrics and transcript togglesThe helper ensures we hit model selection, response piping, and transcript toggling. Consider adding a variant with query_request.conversation_id to exercise ownership + authorized_actions path.
1123-1147: Connection error path correctly increments failure metricThe APIConnectionError branch and metric assertion are well covered. One nit: the first patch to configuration with return_value=mocker.Mock() is redundant since you override it to a Mock object anyway. Removing it will simplify the test.
- mocker.patch( - "app.endpoints.query.configuration", - return_value=mocker.Mock(), - ) - mocker.patch("app.endpoints.query.configuration", None) + mocker.patch("app.endpoints.query.configuration", None)src/app/endpoints/conversations.py (2)
151-171: LIST endpoint: filter logic correct; update docstring and error PII
- Logic: returning all conversations when LIST_OTHERS_CONVERSATIONS is present is correct.
- Docstring still says “retrieve all conversations for the authenticated user” which is now conditional; clarify it.
- Error detail returns the concrete user_id in “cause”. Consider avoiding echoing internal identifiers in client-facing errors.
-"""Handle request to retrieve all conversations for the authenticated user.""" +"""Retrieve conversations for the authenticated user, or all conversations if authorized.""" @@ - raise HTTPException( + raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail={ "response": "Unknown error", - "cause": f"Unknown error while getting conversations for user {user_id}", + # Avoid leaking specific user identifiers in error payloads + "cause": "Unknown error while getting conversations", }, ) from eAlso, the responses example for conversations_list_responses includes a 503 “Unable to connect to Llama Stack,” which the list endpoint does not produce. Consider replacing with a 500 “Unknown error” example for accuracy.
Also applies to: 173-209
264-289: Session retrieval path is coherent; minor resiliency suggestionIf agent_sessions is a large list, always selecting the first element is fine given current API, but consider guarding for malformed entries (missing session_id) with a 404 fallback.
- session_id = str(agent_sessions[0].get("session_id")) + first = agent_sessions[0] + session_id = first.get("session_id") + if not session_id: + logger.error("Malformed session entry for conversation %s: %s", conversation_id, first) + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "response": "Conversation not found", + "cause": f"Conversation {conversation_id} could not be retrieved.", + }, + ) + session_id = str(session_id)src/authorization/resolvers.py (3)
115-128: NoopAccessResolver.get_actions docstring contradicts behaviorDocstring says “empty set of actions”, implementation returns all actions except ADMIN (which is correct for “allow all”). Make the docstring reflect actual behavior. Also make get_actions synchronous per the base class fix.
-class NoopAccessResolver(AccessResolver): # pylint: disable=too-few-public-methods +class NoopAccessResolver(AccessResolver): # pylint: disable=too-few-public-methods @@ - async def get_actions(self, user_roles: UserRoles) -> set[Action]: - """Return an empty set of actions, indicating no specific actions are allowed.""" - _ = user_roles # We're noop, it doesn't matter, everyone is allowed - return set(Action) - {Action.ADMIN} + def get_actions(self, user_roles: UserRoles) -> set[Action]: + """Return all actions (except ADMIN), indicating full access in noop mode.""" + _ = user_roles # Everyone is allowed in noop mode + return set(Action) - {Action.ADMIN}
55-67: Micro: pre-parse JSONPath to avoid repeated parsing per requestIf JwtRoleRule is stable (coming from config), pre-compile JSONPath once. You can extend JwtRoleRule to hold a compiled expression or compile during resolver init.
class JwtRolesResolver(RolesResolver): # pylint: disable=too-few-public-methods @@ - def __init__(self, role_rules: list[JwtRoleRule]): + def __init__(self, role_rules: list[JwtRoleRule]): """Initialize the resolver with rules.""" - self.role_rules = role_rules + self.role_rules = role_rules + # Pre-compile paths for efficiency + self._compiled_paths = {id(rule): parse(rule.jsonpath) for rule in role_rules} @@ - matches = [m.value for m in parse(rule.jsonpath).find(jwt_claims)] + matches = [m.value for m in self._compiled_paths[id(rule)].find(jwt_claims)]If you prefer not to alter JwtRoleRule, this resolver-local cache is sufficient.
16-16: Type alias clarity (optional)UserRoles = set[str] works for type checkers, but to make intent explicit and avoid runtime shadowing, consider:
-from typing import Any +from typing import Any, TypeAlias @@ -UserRoles = set[str] +UserRoles: TypeAlias = set[str]
📜 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 ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
pyproject.toml(1 hunks)src/app/endpoints/authorized.py(2 hunks)src/app/endpoints/config.py(3 hunks)src/app/endpoints/conversations.py(7 hunks)src/app/endpoints/feedback.py(4 hunks)src/app/endpoints/health.py(4 hunks)src/app/endpoints/info.py(3 hunks)src/app/endpoints/metrics.py(2 hunks)src/app/endpoints/models.py(3 hunks)src/app/endpoints/query.py(3 hunks)src/app/endpoints/root.py(2 hunks)src/app/endpoints/streaming_query.py(3 hunks)src/auth/jwk_token.py(2 hunks)src/authorization/__init__.py(1 hunks)src/authorization/middleware.py(1 hunks)src/authorization/resolvers.py(1 hunks)src/configuration.py(2 hunks)src/models/config.py(3 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_config.py(3 hunks)tests/unit/app/endpoints/test_conversations.py(20 hunks)tests/unit/app/endpoints/test_feedback.py(5 hunks)tests/unit/app/endpoints/test_health.py(4 hunks)tests/unit/app/endpoints/test_info.py(2 hunks)tests/unit/app/endpoints/test_metrics.py(2 hunks)tests/unit/app/endpoints/test_models.py(10 hunks)tests/unit/app/endpoints/test_query.py(13 hunks)tests/unit/app/endpoints/test_root.py(1 hunks)tests/unit/auth/test_jwk_token.py(2 hunks)tests/unit/authorization/__init__.py(1 hunks)tests/unit/authorization/test_engine.py(1 hunks)tests/unit/models/test_config.py(2 hunks)tests/unit/utils/auth_helpers.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- pyproject.toml
- src/authorization/init.py
- tests/unit/authorization/init.py
- tests/unit/models/test_config.py
- tests/unit/app/endpoints/test_health.py
- src/app/endpoints/root.py
- src/app/endpoints/models.py
- src/app/endpoints/info.py
- src/app/endpoints/health.py
- tests/unit/app/endpoints/test_config.py
- src/app/endpoints/metrics.py
- tests/unit/authorization/test_engine.py
- src/configuration.py
- tests/unit/app/endpoints/test_info.py
🧰 Additional context used
🧬 Code graph analysis (15)
src/utils/endpoints.py (2)
src/models/database/conversations.py (1)
UserConversation(11-36)src/app/database.py (1)
get_session(34-40)
tests/unit/utils/auth_helpers.py (2)
src/models/config.py (2)
config(113-119)Action(262-299)src/authorization/resolvers.py (3)
get_actions(27-28)get_actions(124-127)get_actions(174-184)
tests/unit/app/endpoints/test_conversations.py (3)
src/models/config.py (2)
config(113-119)Action(262-299)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)src/app/endpoints/conversations.py (3)
get_conversation_endpoint_handler(213-320)delete_conversation_endpoint_handler(327-425)get_conversations_list_endpoint_handler(152-208)
src/authorization/middleware.py (3)
src/authorization/resolvers.py (15)
AccessResolver(19-28)GenericAccessResolver(130-184)JwtRolesResolver(52-112)NoopAccessResolver(115-127)NoopRolesResolver(43-49)RolesResolver(35-40)resolve_roles(39-40)resolve_roles(46-49)resolve_roles(59-66)check_access(23-24)check_access(118-122)check_access(155-172)get_actions(27-28)get_actions(124-127)get_actions(174-184)src/models/config.py (3)
config(113-119)Action(262-299)jwk_configuration(358-365)src/configuration.py (3)
configuration(56-61)authorization_configuration(109-118)authentication_configuration(96-106)
src/app/endpoints/feedback.py (3)
src/authorization/middleware.py (1)
authorize(102-113)src/models/config.py (2)
config(113-119)Action(262-299)src/models/requests.py (1)
FeedbackRequest(239-381)
src/app/endpoints/conversations.py (1)
src/client.py (1)
AsyncLlamaStackClientHolder(18-50)
tests/unit/app/endpoints/test_feedback.py (2)
tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)src/app/endpoints/feedback.py (1)
feedback_endpoint_handler(85-123)
tests/unit/app/endpoints/test_models.py (2)
tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)src/app/endpoints/models.py (1)
models_endpoint_handler(54-108)
src/app/endpoints/config.py (5)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(102-113)src/configuration.py (1)
configuration(56-61)src/models/config.py (3)
config(113-119)Action(262-299)Configuration(406-423)src/utils/endpoints.py (1)
check_configuration_loaded(44-50)
src/authorization/resolvers.py (1)
src/models/config.py (5)
config(113-119)JwtRoleRule(224-259)AccessRule(302-306)JsonPathOperator(216-221)Action(262-299)
tests/unit/app/endpoints/test_root.py (2)
src/app/endpoints/root.py (1)
root_endpoint_handler(782-794)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)
tests/unit/app/endpoints/test_metrics.py (2)
src/app/endpoints/metrics.py (1)
metrics_endpoint_handler(24-47)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)
src/app/endpoints/streaming_query.py (2)
src/authorization/middleware.py (1)
authorize(102-113)src/models/config.py (2)
config(113-119)Action(262-299)
src/app/endpoints/query.py (3)
src/authorization/middleware.py (1)
authorize(102-113)src/models/config.py (2)
config(113-119)Action(262-299)src/models/database/conversations.py (1)
UserConversation(11-36)
tests/unit/app/endpoints/test_query.py (2)
tests/unit/app/endpoints/test_conversations.py (1)
dummy_request(30-40)src/app/endpoints/query.py (1)
query_endpoint_handler(154-265)
🔇 Additional comments (21)
src/auth/jwk_token.py (1)
4-4: LGTM on import additionThe new json import is used and scoped appropriately.
tests/unit/utils/auth_helpers.py (1)
9-27: Ignore AsyncMock suggestion forget_actionsThe middleware in src/authorization/middleware.py invokes
access_resolver.get_actions(user_roles)synchronously (withoutawait), so in tests you must mock it to return a concreteset[Action], not a coroutine. Changing the mock to anAsyncMockwould makeget_actionsreturn a coroutine and break the test harness.• tests/unit/utils/auth_helpers.py (lines 9–27): keep
mock_access_resolver.get_actions = Mock(return_value=set(Action))• src/authorization/middleware.py (around line 92): note the missing
awaitonget_actionsUntil the middleware is updated to
await access_resolver.get_actions(...), the current synchronous mock is correct.Likely an incorrect or invalid review comment.
src/app/endpoints/authorized.py (1)
35-36: Type-accurate DI: good move to Annotated[AuthTuple, Depends(...)]The signature change improves FastAPI schema and enforces the expected tuple shape at call sites.
tests/unit/app/endpoints/test_root.py (1)
10-14: Async test + auth resolvers mocking looks solidTest shape matches the new DI and authorization flow.
tests/unit/app/endpoints/test_models.py (2)
17-18: Bypassing authorization in unit tests: goodUsing mock_authorization_resolvers keeps these tests focused on handler logic.
66-68: Config blocks include authz/authn: good coverageIncluding authorization/access_rules and authentication.module in test configs aligns with the new pipeline.
Also applies to: 115-117, 160-162, 210-212
src/app/endpoints/query.py (3)
19-29: Imports and decorator wiring look correct.Good fix on the Action import path and adding the authorize decorator import. No issues spotted here.
32-32: Response models import is aligned with the documented responses.Importing QueryResponse/UnauthorizedResponse/ForbiddenResponse here matches the responses dict below. Looks good.
153-159: Endpoint gated with @authorize(Action.QUERY) and DI shape is correct.The function now accepts Request and auth via Depends, matching the middleware’s expectations. Signature and return type are consistent with usage.
tests/unit/app/endpoints/test_feedback.py (2)
14-14: Auth bypass helper is correctly wired into tests.Using mock_authorization_resolvers keeps tests focused on endpoint behavior. Good pattern.
66-71: Async conversion and authorization mocking are correct.Migrating to async tests and invoking the resolver mocks ensures decorator flow executes. Looks good.
src/app/endpoints/streaming_query.py (1)
514-520: Authorization decorator and DI shape are correct.@authenticate(Action.STREAMING_QUERY) is applied and the handler receives Request and auth; consistent with the new framework.
src/app/endpoints/config.py (1)
64-69: Config endpoint is correctly gated with GET_CONFIG and converted to async.Signature, decorator usage, and type hints look good. The auth argument is present for the middleware.
tests/unit/app/endpoints/test_conversations.py (4)
29-41: Good test scaffolding: request.state.authorized_actions populatedSetting authorized_actions to the full set of Action values makes the tests independent of the authorization middleware and keeps intent explicit. Including Action.ADMIN here is fine because endpoints check for specific actions.
65-74: Robust DB mocking for both filtered and unfiltered pathsNice touch creating a nested mock_query and wiring both .all() and .filter_by(...).all(). This prevents accidental test brittleness if query paths diverge.
252-266: Config-not-loaded paths are exercised correctlyBoth GET and DELETE tests cover the configuration guard and assert the same error shape production returns. LGTM.
Also applies to: 427-441
566-589: Delete flow: assert against concrete call argsAsserting the delete call with agent_id and session_id derived from the list response ensures we won’t regress on session_id handling. Good coverage.
tests/unit/app/endpoints/test_query.py (1)
83-103: Config-not-loaded test is minimal and targetedGood: it asserts exact response text and code without coupling to unrelated behavior.
src/app/endpoints/conversations.py (3)
8-8: FastAPI import extension is fineAdding Depends here aligns with the new auth dependency usage.
12-16: Auth and authorization wiring looks correct
- get_session is used via context manager
- get_auth_dependency() captured in module scope
- authorize decorator imported and used below
212-256: Ownership enforcement gating via authorized_actions is solidothers_allowed tied to READ_OTHERS_CONVERSATIONS is straightforward and testable.
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)
tests/unit/app/endpoints/test_models.py (1)
167-173: Patch the symbol where it is used: patch app.endpoints.models.AsyncLlamaStackClientHolder, not client.AsyncLlamaStackClientHolderWhere-to-patch rule: patch in the module under test where the symbol is looked up. Otherwise, the real client may be used and the test can become flaky or perform network I/O.
- mock_lsc = mocker.patch("client.AsyncLlamaStackClientHolder.get_client") + mock_lsc = mocker.patch( + "app.endpoints.models.AsyncLlamaStackClientHolder.get_client" + )src/app/endpoints/feedback.py (2)
108-113: Avoid logging raw user feedback payload; scrub PII.
logger.debug("Feedback received %s", str(feedback_request))can leak user content (questions/responses) into logs. Log only non‑sensitive metadata (e.g., conversation_id, sentiment, categories).- logger.debug("Feedback received %s", str(feedback_request)) + logger.debug( + "Feedback received: conversation_id=%s sentiment=%s categories=%s", + feedback_request.conversation_id, + feedback_request.sentiment, + feedback_request.categories, + )
126-159: Disallow empty storage path; fail fast instead of writing to CWD.
Path(configuration.user_data_collection_configuration.feedback_storage or "")will resolve to the current working directory when unset, potentially scattering feedback files in unintended locations. Treat a missing path as a configuration error.- storage_path = Path( - configuration.user_data_collection_configuration.feedback_storage or "" - ) + storage_dir = configuration.user_data_collection_configuration.feedback_storage + if not storage_dir: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Feedback storage path is not configured", + ) + storage_path = Path(storage_dir)Optionally, enforce restrictive file permissions when writing feedback (e.g., 0o600) to minimize data exposure at rest.
# Outside selected range (illustrative): import os fd = os.open(feedback_file_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) with os.fdopen(fd, "w", encoding="utf-8") as feedback_file: json.dump(data_to_store, feedback_file)
♻️ Duplicate comments (17)
tests/unit/app/endpoints/test_info.py (1)
49-50: Standardize AuthTuple ordering: (user_id, user_name, token)Current tuple sets user_name="token" and token=dict. Align with the project-wide tuple contract to reduce future breakage.
- auth = ("test_user", "token", {}) + auth = ("test_user", "test_user_name", "token")tests/unit/app/endpoints/test_models.py (4)
180-182: AuthTuple orderingKeep (user_id, user_name, token) for consistency.
- auth = ("test_user", "token", {}) + auth = ("test_user", "test_user_name", "token")
83-86: Unreachable assertions inside pytest.raises (repeat) + AuthTuple ordering
- Move assertions outside the with-block as above.
- Fix tuple ordering to (user_id, user_name, token).
- auth = ("test_user", "token", {}) - with pytest.raises(HTTPException) as e: - await models_endpoint_handler(request=request, auth=auth) - assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR - assert e.detail["response"] == "LLama stack is not configured" + auth = ("test_user", "test_user_name", "token") + with pytest.raises(HTTPException) as e: + await models_endpoint_handler(request=request, auth=auth) + assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert e.value.detail["response"] == "LLama stack is not configured"
127-133: Unreachable assertions inside pytest.raises (repeat) + AuthTuple orderingSame fixes apply here.
- auth = ("test_user", "token", {}) - with pytest.raises(HTTPException) as e: - await models_endpoint_handler(request=request, auth=auth) - assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR - assert e.detail["response"] == "Unable to connect to Llama Stack" + auth = ("test_user", "test_user_name", "token") + with pytest.raises(HTTPException) as e: + await models_endpoint_handler(request=request, auth=auth) + assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert e.value.detail["response"] == "Unable to connect to Llama Stack"
232-236: Unreachable assertions inside pytest.raises (repeat) + AuthTuple orderingApply the same fixes.
- auth = ("test_user", "token", {}) - with pytest.raises(HTTPException) as e: - await models_endpoint_handler(request=request, auth=auth) - assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR - assert e.detail["response"] == "Unable to connect to Llama Stack" + auth = ("test_user", "test_user_name", "token") + with pytest.raises(HTTPException) as e: + await models_endpoint_handler(request=request, auth=auth) + assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert e.value.detail["response"] == "Unable to connect to Llama Stack"src/app/endpoints/root.py (1)
780-785: Authorization decorator usage looks correct; previous short‑circuit bug appears resolved.The handler is now async, gated with @authorize(Action.INFO), and the middleware’s wrapper calls the original function (per current middleware). This should restore normal endpoint execution.
src/authorization/middleware.py (5)
23-25: Add cache invalidation on configuration reload to avoid stale resolversget_authorization_resolvers() is lru_cached; after a config reload the resolvers will be stale unless cache_clear() is called.
Wire your configuration reload path to call:
from authorization.middleware import get_authorization_resolvers get_authorization_resolvers.cache_clear()Would you like me to open a follow-up PR to add a small hook in your config loader for this?
40-47: Fix role_rules “unset” check; current comparison to 0 is always FalseComparing a list to 0 never succeeds. Also, guard against None and keep the access_rules length check after fixing the accessor call.
- jwt_role_rules_unset = ( - authentication_config.jwk_configuration.jwt_configuration.role_rules - ) == 0 - - authz_access_rules_unset = len(authorization_cfg.access_rules) == 0 + jwt_cfg = authentication_config.jwk_configuration.jwt_configuration + rules = jwt_cfg.role_rules or [] + jwt_role_rules_unset = len(rules) == 0 + + authz_access_rules_unset = len(authorization_cfg.access_rules) == 0
84-90: Return 401 for malformed/invalid auth token; catch RoleResolutionErrorIf resolve_roles fails (e.g., bad/malformed JWT), we should not 500. Surface a 401 and log context. Adds import of RoleResolutionError.
- user_roles = await role_resolver.resolve_roles(auth) | everyone_roles - - if not await access_resolver.check_access(action, user_roles): + try: + user_roles = await role_resolver.resolve_roles(auth) + except RoleResolutionError as exc: + logger.warning("Failed to resolve roles from auth token: %s", exc) + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid authentication token", + ) from exc + user_roles |= everyone_roles + + if not await access_resolver.check_access(action, user_roles): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail=f"Insufficient permissions for action: {action}", )And update imports:
from authorization.resolvers import ( AccessResolver, GenericAccessResolver, JwtRolesResolver, NoopAccessResolver, NoopRolesResolver, - RolesResolver, + RolesResolver, + RoleResolutionError, )
92-99: Await get_actions; otherwise you store a coroutine on request.stateAccessResolver.get_actions is async in this PR; without await, authorized_actions becomes a coroutine and membership checks explode at runtime.
- authorized_actions = access_resolver.get_actions(user_roles) + authorized_actions = await access_resolver.get_actions(user_roles)
26-28: Call configuration accessors (they’re methods), otherwise AttributeError at runtimeBoth accessors are defined as methods in configuration.py; without parentheses you’ll get a bound method object and subsequent attribute access (e.g., .module/.access_rules) will fail.
- authorization_cfg = configuration.authorization_configuration - authentication_config = configuration.authentication_configuration + authorization_cfg = configuration.authorization_configuration() + authentication_config = configuration.authentication_configuration()src/authorization/resolvers.py (3)
82-93: JWT parsing assumes JSON string; support compact JWTs and dicts; raise RoleResolutionError on decode failuresjson.loads(token) will blow up for typical compact JWTs (header.payload.signature). Accept dicts, JSON strings, and compact JWTs (base64url decode payload).
def _get_claims(auth: AuthTuple) -> dict[str, Any]: """Get the JWT claims from the auth tuple.""" - _, _, token = auth - jwt_claims = json.loads(token) - - if not jwt_claims: - raise RoleResolutionError( - "Invalid authentication token: no JWT claims found" - ) - - return jwt_claims + _, _, token = auth + try: + # Already a dict of claims? + if isinstance(token, dict): + return token + # String token + if isinstance(token, str): + # Compact JWT: header.payload.signature + if token.count(".") == 2: + payload_b64 = token.split(".")[1] + padding = "=" * (-len(payload_b64) % 4) + payload = base64.urlsafe_b64decode(payload_b64 + padding) + claims = json.loads(payload.decode("utf-8")) + if not isinstance(claims, dict) or not claims: + raise RoleResolutionError("Invalid JWT payload: empty or non-object") + return claims + # Plain JSON string + claims = json.loads(token) + if not isinstance(claims, dict) or not claims: + raise RoleResolutionError("Invalid authentication token: empty or non-object") + return claims + raise RoleResolutionError("Unsupported token type for role resolution") + except json.JSONDecodeError as e: + raise RoleResolutionError(f"Invalid authentication token: {e}") from eAlso add:
import json +import base64
68-80: Operator semantics are wrong for JSONPath multi-matches; evaluate per element_evaluate_operator receives a list of matches; equals/contains/in should be computed element-wise with type-safe checks.
def evaluate_role_rules(rule: JwtRoleRule, jwt_claims: dict[str, Any]) -> UserRoles: """Get roles from a JWT role rule if it matches the claims.""" - return ( - set(rule.roles) - if JwtRolesResolver._evaluate_operator( - rule.negate, - [match.value for match in parse(rule.jsonpath).find(jwt_claims)], - rule.operator, - rule.value, - ) - else set() - ) + matches = [match.value for match in parse(rule.jsonpath).find(jwt_claims)] + return set(rule.roles) if JwtRolesResolver._evaluate_operator( + rule.negate, matches, rule.operator, rule.value + ) else set() @@ - def _evaluate_operator( - negate: bool, match: Any, operator: JsonPathOperator, value: Any + def _evaluate_operator( + negate: bool, matches: list[Any], operator: JsonPathOperator, value: Any ) -> bool: # pylint: disable=too-many-branches """Evaluate an operator against a match and value.""" - result = False - match operator: - case JsonPathOperator.EQUALS: - result = match == value - case JsonPathOperator.CONTAINS: - result = value in match - case JsonPathOperator.IN: - result = match in value + result = False + match operator: + case JsonPathOperator.EQUALS: + result = any(m == value for m in matches) + case JsonPathOperator.CONTAINS: + def contains(container: Any, needle: Any) -> bool: + if isinstance(container, dict): + return needle in container.keys() + if isinstance(container, (list, set, tuple)): + return needle in container + if isinstance(container, str): + return isinstance(needle, str) and needle in container + return False + result = any(contains(m, value) for m in matches) + case JsonPathOperator.IN: + if isinstance(value, dict): + result = any(m in value.keys() for m in matches) + elif isinstance(value, (list, set, tuple)): + result = any(m in value for m in matches) + elif isinstance(value, str): + result = any(isinstance(m, str) and m in value for m in matches) + else: + result = False if negate: result = not resultAlso applies to: 95-112
174-182: Avoid KeyError when aggregating actions for unknown rolesIndexing _access_lookup[role] raises KeyError for roles without explicit rules; use .get(role, set()).
- actions = { - action for role in user_roles for action in self._access_lookup[role] - } + actions: set[Action] = set() + for role in user_roles: + actions.update(self._access_lookup.get(role, set()))src/models/config.py (2)
309-313: Use Field(default_factory=list) to avoid shared mutable defaultsLists as defaults are shared across instances. Switch to Field(default_factory=list).
- access_rules: list[AccessRule] = [] # Rules for role-based access control + access_rules: list[AccessRule] = Field(default_factory=list) # Rules for role-based access controlAlso add the import:
from pydantic import Field # alongside existing pydantic imports
318-321: Same for JwtConfiguration.role_rules — avoid mutable default listPrevent cross-instance mutation by using Field(default_factory=list).
- role_rules: list[JwtRoleRule] = [] # Rules for extracting roles from JWT + role_rules: list[JwtRoleRule] = Field(default_factory=list) # Rules for extracting roles from JWTReminder: ensure Field is imported as noted above.
tests/unit/app/endpoints/test_query.py (1)
36-44: Populate request.state.authorized_actions in the dummy_request fixtureEndpoints consult request.state.authorized_actions; without it tests can fail or mis-evaluate “others” checks. Align with conversations test fixture.
@pytest.fixture def dummy_request() -> Request: """Dummy request fixture for testing.""" - return Request( - scope={ - "type": "http", - } - ) + req = Request(scope={"type": "http"}) + # Allow all actions in tests unless a test overrides it explicitly + from models.config import Action + req.state.authorized_actions = set(Action) + return req
🧹 Nitpick comments (22)
src/utils/endpoints.py (2)
22-29: API behavior change is reasonable; consider clarifying the contract in the docstringReturning Optional[UserConversation] (None when not found or not owned) aligns with the new authorization flow. To avoid ambiguity, explicitly document the Optional return and the ownership-bypass semantics when others_allowed=True.
Proposed docstring tweak:
-"""Validate that the conversation belongs to the user. - -Validates that the conversation with the given ID belongs to the user with the given ID. -If `others_allowed` is True, it allows conversations that do not belong to the user, -which is useful for admin access. -""" +"""Validate that the conversation belongs to the user and return it if authorized. + +Validates that the conversation with the given ID belongs to the user with the given ID. +If `others_allowed` is True, ownership is not enforced (useful for admin access). + +Returns: + UserConversation | None: The conversation if found and authorized; otherwise None. +"""
31-41: Use Session.get for primary-key lookups to avoid an extra query build and improve clarityWhen others_allowed is True, this is a pure PK fetch. session.get is more direct and can be marginally more efficient; keep the filtered query for the ownership-enforced path.
- conversation_query = session.query(UserConversation) - - filtered_conversation_query = ( - conversation_query.filter_by(id=conversation_id) - if others_allowed - else conversation_query.filter_by(id=conversation_id, user_id=user_id) - ) - - conversation: UserConversation | None = filtered_conversation_query.first() + if others_allowed: + # Primary-key fetch when ownership is not enforced + conversation: UserConversation | None = session.get( + UserConversation, conversation_id + ) + else: + # Enforce ownership + conversation = ( + session.query(UserConversation) + .filter_by(id=conversation_id, user_id=user_id) + .first() + )tests/unit/models/test_config.py (1)
623-624: Default to None is fine; consider adding a companion test for the AppConfig accessor behaviorAppConfig.authorization_configuration returns a default AuthorizationConfiguration when configuration.authorization is None. A small test that exercises this accessor would catch regressions in the future.
Would you like me to add that unit test?
src/app/endpoints/authorized.py (1)
35-36: Minor nit: ensure tests provide AuthTuple in (user_id, user_name, token) orderHandler unpacks as user_id, user_name, _ = auth, so please keep tests consistent with that ordering to avoid future surprises if token handling becomes stricter.
src/app/endpoints/root.py (2)
17-17: Minor: consider deferring resolver acquisition to FastAPI dependency system.Creating
auth_dependency = get_auth_dependency()at import time is fine and consistent with other files, but you could also inline Depends(get_auth_dependency()) directly in the param to avoid module‑level state. Not required.-async def root_endpoint_handler( - auth: Annotated[AuthTuple, Depends(auth_dependency)], +async def root_endpoint_handler( + auth: Annotated[AuthTuple, Depends(get_auth_dependency())],
787-792: No‑op assignments are acceptable; consider underscore‑prefixed params instead.You’re intentionally touching
authandrequestto appease linters. Alternatively, make them underscore‑prefixed (e.g.,_request) if you don’t needrequest.state.authorized_actionshere.src/app/endpoints/metrics.py (2)
44-47: Avoid repeated provider listing on every scrape; guard setup with an async one‑time initializer.
setup_model_metrics()pulls provider/model data and configures gauges. Doing this on each scrape can add latency and load on your LlamaStack client. Add a one‑time, process‑wide guard with an asyncio.Lock.Apply in this file:
+import asyncio @@ router = APIRouter(tags=["metrics"]) -auth_dependency = get_auth_dependency() +auth_dependency = get_auth_dependency() + +# One-time guard for metrics setup +_metrics_setup_done = False +_metrics_setup_lock = asyncio.Lock() @@ - # Setup the model metrics if not already done. This is a one-time setup - # and will not be run again on subsequent calls to this endpoint - await setup_model_metrics() + # One-time model metrics setup guarded by a lock + global _metrics_setup_done # noqa: PLW0603 + if not _metrics_setup_done: + async with _metrics_setup_lock: + if not _metrics_setup_done: + await setup_model_metrics() + _metrics_setup_done = True return PlainTextResponse(generate_latest(), media_type=CONTENT_TYPE_LATEST)If metrics must be refreshed dynamically (e.g., providers/models change at runtime), consider a TTL refresh instead.
38-43: Optional: remove no‑op assignments if not needed.Keeping
requestonly to be future‑proof forrequest.state.authorized_actionsis reasonable; if you don’t need it here, you can drop it and the no‑op assignments.src/app/endpoints/config.py (1)
78-83: No‑op touches are fine; consider underscore param for clarity.If you don’t rely on
request.state.authorized_actionshere, either droprequestor rename to_requestto avoid the extra assignments.-async def config_endpoint_handler( - auth: Annotated[AuthTuple, Depends(auth_dependency)], - request: Request, +async def config_endpoint_handler( + auth: Annotated[AuthTuple, Depends(auth_dependency)], + _request: Request, ) -> Configuration: @@ - # Nothing interesting in the request - _ = request + # Nothing interesting in the request + _ = _requesttests/unit/app/endpoints/test_health.py (1)
33-38: [Assuming the script above ran and displayed the patch decorator intest_health.py]It looks like the test is indeed patching the client class on its original import path:
- @patch("client.AsyncLlamaStackClientHolder.get_client") + @patch("app.endpoints.health.AsyncLlamaStackClientHolder.get_client")Why this matters
Patching at the point of use ensures you’re replacing exactly the reference your code under test is invoking. If you ever refactor the import (for example, moveAsyncLlamaStackClientHolderinto a submodule or alias it), a module-under-test patch will continue to work, whereas patching the original implementation module (client) may silently stop intercepting your calls.Next steps
• Intests/unit/app/endpoints/test_health.py, update your patch decorator (or context manager) to target
"app.endpoints.health.AsyncLlamaStackClientHolder.get_client"instead of"client.AsyncLlamaStackClientHolder.get_client".This change is a low-risk nitpick but improves the resilience of your tests against future refactors.
src/app/endpoints/feedback.py (1)
83-89: Add FastAPI Request parameter to enable authorized actions propagation.Most endpoints accept a
Requestso the middleware can populaterequest.state.authorized_actions. While not strictly required here, adding it keeps the pattern consistent and future‑proofs the handler if you later branch behavior by allowed actions.-async def feedback_endpoint_handler( - feedback_request: FeedbackRequest, - auth: Annotated[AuthTuple, Depends(auth_dependency)], - _ensure_feedback_enabled: Any = Depends(assert_feedback_enabled), -) -> FeedbackResponse: +async def feedback_endpoint_handler( + feedback_request: FeedbackRequest, + request: Request, + auth: Annotated[AuthTuple, Depends(auth_dependency)], + _ensure_feedback_enabled: Any = Depends(assert_feedback_enabled), +) -> FeedbackResponse: + # Used by middleware for request.state.authorized_actions + _ = requestsrc/authorization/middleware.py (1)
105-111: Decorator should support both async and sync handlersFastAPI allows sync endpoints. Awaiting func unconditionally breaks sync handlers. Prefer awaiting when the result is awaitable.
- @wraps(func) - async def wrapper(*args: Any, **kwargs: Any) -> Any: - await _perform_authorization_check(action, kwargs) - return await func(*args, **kwargs) + @wraps(func) + async def wrapper(*args: Any, **kwargs: Any) -> Any: + await _perform_authorization_check(action, kwargs) + result = func(*args, **kwargs) + if inspect.isawaitable(result): + return await result + return resultAdd import:
-from functools import wraps, lru_cache +from functools import wraps, lru_cache +import inspectsrc/app/endpoints/conversations.py (3)
167-174: Avoid AttributeError if middleware didn’t set authorized_actionsDefensively read request.state.authorized_actions with a default set() so membership tests don’t blow up when middleware is bypassed (e.g., unit tests, custom integrations).
- filtered_query = ( - query - if Action.LIST_OTHERS_CONVERSATIONS in request.state.authorized_actions - else query.filter_by(user_id=user_id) - ) + authorized = getattr(request.state, "authorized_actions", set()) + filtered_query = ( + query + if Action.LIST_OTHERS_CONVERSATIONS in authorized + else query.filter_by(user_id=user_id) + )
253-256: Same defensive access for READ_OTHERS_CONVERSATIONS checkPrevents crashes when state is missing.
- others_allowed=( - Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions - ), + others_allowed=( + Action.READ_OTHERS_CONVERSATIONS + in getattr(request.state, "authorized_actions", set()) + ),
361-364: Same defensive access for DELETE_OTHERS_CONVERSATIONS checkMirror the pattern to keep behavior consistent and robust.
- others_allowed=( - Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions - ), + others_allowed=( + Action.DELETE_OTHERS_CONVERSATIONS + in getattr(request.state, "authorized_actions", set()) + ),tests/unit/app/endpoints/test_conversations.py (7)
22-22: Reduce repetition: make authorization mocking autouseEvery test calls mock_authorization_resolvers(mocker). Convert it into an autouse fixture to DRY this file and ensure future tests inherit the default “allow all” behavior unless overridden.
Example to place in this test module or in tests/unit/conftest.py:
import pytest from tests.unit.utils.auth_helpers import mock_authorization_resolvers @pytest.fixture(autouse=True) def allow_all_authorization(mocker): """Default: allow all actions unless a test overrides the resolver behavior.""" mock_authorization_resolvers(mocker)
29-41: Make dummy_request configurable for negative/ownership-path testsHard-coding request.state.authorized_actions = set(Action) prevents testing denied/filtered branches (e.g., LIST_OTHERS_CONVERSATIONS absent). Keep this fixture for current tests, but add a small factory to create requests with custom action sets. This will unlock coverage for 403s and user-filtered queries.
Add alongside the current fixture:
import typing as t @pytest.fixture def dummy_request_factory(): """Factory to create Request with specific authorized actions.""" def _make(authorized: t.Iterable[Action] | None = None) -> Request: req = Request(scope={"type": "http"}) req.state.authorized_actions = set(authorized or []) return req return _make # Keep the existing default for “allow all” tests # (no changes to current tests needed)With this, you can write e.g.:
req = dummy_request_factory({Action.LIST_CONVERSATIONS}) # no LIST_OTHERS_CONVERSATIONS
69-73: Strengthen the SQLAlchemy query mock to support chained callsFuture code may chain .filter_by(...).order_by(...).all(). Returning the same mock from filter_by makes chaining safer without changing current behavior.
Apply this diff inside mock_database_session:
- mock_query = mocker.Mock() - mock_query.all.return_value = query_result - mock_query.filter_by.return_value.all.return_value = query_result + mock_query = mocker.Mock() + # Support both `query().all()` and chained variants like `query().filter_by(...).all()` + mock_query.all.return_value = query_result + mock_query.filter_by.return_value = mock_query mock_session.query.return_value = mock_query
381-413: Tighten assertions for the happy-path retrievalConsider asserting that retrieve() was invoked with the expected session ID and agent ID. This guards against regressions in parameter wiring. If the client expects keyword arguments, you can assert via call_args.kwargs to avoid coupling to exact names.
Example:
# After awaiting the handler mock_client.agents.session.retrieve.assert_called_once() kwargs = mock_client.agents.session.retrieve.call_args.kwargs assert kwargs.get("session_id") == VALID_CONVERSATION_ID # agent_id may be required as well depending on client API # assert kwargs.get("agent_id") == VALID_CONVERSATION_IDIf the client is positional-only or uses different names, adjust accordingly. Want me to push a follow-up with the exact assertion based on the actual client signature?
557-589: Add test for “no sessions found” treated as successful deletionThe endpoint is supposed to return success if no sessions exist. Add a test where session.list returns an empty dataset and assert a successful ConversationDeleteResponse. This ensures we don’t regress into 404s for benign deletes.
Example test to add:
@pytest.mark.asyncio async def test_delete_conversation_no_sessions_found(mocker, setup_configuration, dummy_request): mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) mocker.patch("app.endpoints.conversations.validate_conversation_ownership") mock_client = mocker.AsyncMock() mock_client.agents.session.list.return_value = mocker.Mock(data=[]) mock_client_holder = mocker.patch("app.endpoints.conversations.AsyncLlamaStackClientHolder") mock_client_holder.return_value.get_client.return_value = mock_client response = await delete_conversation_endpoint_handler( request=dummy_request, conversation_id=VALID_CONVERSATION_ID, auth=MOCK_AUTH ) assert isinstance(response, ConversationDeleteResponse) assert response.conversation_id == VALID_CONVERSATION_ID assert response.success is True
609-643: Also test the “filtered-by-user” branch and assert more fieldsCurrent test exercises the “list all” path due to set(Action). Add a companion test where authorized_actions lacks LIST_OTHERS_CONVERSATIONS so the endpoint filters by the current user_id. While here, optionally assert additional fields such as created_at/last_message_at and provider/model to lock the response shape.
Example (leverages the suggested dummy_request_factory):
@pytest.mark.asyncio async def test_conversations_list_filtered_by_user(mocker, setup_configuration, dummy_request_factory): mocker.patch("app.endpoints.conversations.configuration", setup_configuration) # Only allow listing own conversations req = dummy_request_factory({Action.LIST_CONVERSATIONS}) # Prepare a mock session and ensure filter_by(user_id=...) is used convs = [create_mock_conversation( mocker, "abc", "2024-01-01T00:00:00Z", "2024-01-01T00:05:00Z", 1, "model/x", "prov" )] session = mock_database_session(mocker, convs) # Call the handler resp = await get_conversations_list_endpoint_handler(auth=MOCK_AUTH, request=req) # Assertions assert len(resp.conversations) == 1 session.query.return_value.filter_by.assert_called_once_with(user_id=MOCK_AUTH[0])
29-41: Add explicit negative authorization tests (403) for decorator enforcementGiven the new @authorize(action) layer, include at least one test per endpoint where AccessResolver.check_access returns False for the required action to ensure a 403 is raised. Your helper returns “allow all”; you can override the returned mock within individual tests.
Example pattern inside a test:
# Arrange resolvers from authorization.middleware import get_authorization_resolvers as _gar # Grab the current mocks created by mock_authorization_resolvers and override check_access mock_role_resolver, mock_access_resolver = _gar() mock_access_resolver.check_access.return_value = False # Expect 403 when calling the handler with pytest.raises(HTTPException) as exc: await get_conversations_list_endpoint_handler(auth=MOCK_AUTH, request=dummy_request) assert exc.value.status_code == status.HTTP_403_FORBIDDENIf get_authorization_resolvers() isn’t directly accessible, expose it via the helper or return the mocks from mock_authorization_resolvers to customize per-test. I can refactor the helper for you if helpful.
Also applies to: 253-263, 595-605
📜 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 ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
pyproject.toml(1 hunks)src/app/endpoints/authorized.py(2 hunks)src/app/endpoints/config.py(3 hunks)src/app/endpoints/conversations.py(7 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/health.py(4 hunks)src/app/endpoints/info.py(3 hunks)src/app/endpoints/metrics.py(2 hunks)src/app/endpoints/models.py(3 hunks)src/app/endpoints/query.py(3 hunks)src/app/endpoints/root.py(2 hunks)src/app/endpoints/streaming_query.py(3 hunks)src/auth/jwk_token.py(2 hunks)src/authorization/__init__.py(1 hunks)src/authorization/middleware.py(1 hunks)src/authorization/resolvers.py(1 hunks)src/configuration.py(2 hunks)src/models/config.py(3 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_config.py(3 hunks)tests/unit/app/endpoints/test_conversations.py(20 hunks)tests/unit/app/endpoints/test_feedback.py(5 hunks)tests/unit/app/endpoints/test_health.py(4 hunks)tests/unit/app/endpoints/test_info.py(2 hunks)tests/unit/app/endpoints/test_metrics.py(2 hunks)tests/unit/app/endpoints/test_models.py(10 hunks)tests/unit/app/endpoints/test_query.py(13 hunks)tests/unit/app/endpoints/test_root.py(1 hunks)tests/unit/auth/test_jwk_token.py(2 hunks)tests/unit/authorization/__init__.py(1 hunks)tests/unit/authorization/test_engine.py(1 hunks)tests/unit/models/test_config.py(2 hunks)tests/unit/utils/auth_helpers.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/unit/authorization/init.py
- src/authorization/init.py
🚧 Files skipped from review as they are similar to previous changes (15)
- src/auth/jwk_token.py
- tests/unit/auth/test_jwk_token.py
- tests/unit/authorization/test_engine.py
- pyproject.toml
- src/configuration.py
- tests/unit/app/endpoints/test_root.py
- src/app/endpoints/models.py
- tests/unit/app/endpoints/test_config.py
- src/app/endpoints/streaming_query.py
- src/app/endpoints/query.py
- tests/unit/utils/auth_helpers.py
- src/app/endpoints/info.py
- tests/unit/app/endpoints/test_feedback.py
- src/app/endpoints/health.py
- tests/unit/app/endpoints/test_metrics.py
🧰 Additional context used
🧬 Code graph analysis (13)
src/app/endpoints/root.py (3)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(102-113)src/models/config.py (2)
config(113-119)Action(262-299)
src/utils/endpoints.py (2)
src/models/database/conversations.py (1)
UserConversation(11-36)src/app/database.py (1)
get_session(34-40)
tests/unit/app/endpoints/test_health.py (3)
src/app/endpoints/health.py (3)
readiness_probe_get_method(82-112)liveness_probe_get_method(129-141)get_providers_health_statuses(31-65)src/models/responses.py (2)
ProviderHealthStatus(121-141)ReadinessResponse(144-183)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)
tests/unit/app/endpoints/test_info.py (3)
src/app/endpoints/info.py (1)
info_endpoint_handler(33-52)src/configuration.py (3)
configuration(56-61)AppConfig(28-142)init_from_dict(51-53)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)
tests/unit/app/endpoints/test_models.py (2)
tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)src/app/endpoints/models.py (1)
models_endpoint_handler(54-108)
src/authorization/middleware.py (3)
src/authorization/resolvers.py (15)
AccessResolver(19-28)GenericAccessResolver(130-184)JwtRolesResolver(52-112)NoopAccessResolver(115-127)NoopRolesResolver(43-49)RolesResolver(35-40)resolve_roles(39-40)resolve_roles(46-49)resolve_roles(59-66)check_access(23-24)check_access(118-122)check_access(155-172)get_actions(27-28)get_actions(124-127)get_actions(174-184)src/models/config.py (3)
config(113-119)Action(262-299)jwk_configuration(358-365)src/configuration.py (3)
configuration(56-61)authorization_configuration(109-118)authentication_configuration(96-106)
src/authorization/resolvers.py (1)
src/models/config.py (5)
config(113-119)JwtRoleRule(224-259)AccessRule(302-306)JsonPathOperator(216-221)Action(262-299)
src/app/endpoints/conversations.py (5)
src/app/database.py (1)
get_session(34-40)src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(102-113)src/models/config.py (2)
config(113-119)Action(262-299)src/models/database/conversations.py (1)
UserConversation(11-36)
tests/unit/app/endpoints/test_query.py (3)
tests/unit/app/endpoints/test_conversations.py (1)
dummy_request(30-40)tests/unit/auth/test_jwk_token.py (1)
dummy_request(132-140)src/app/endpoints/query.py (1)
query_endpoint_handler(154-265)
src/app/endpoints/feedback.py (3)
src/authorization/middleware.py (1)
authorize(102-113)src/models/config.py (2)
config(113-119)Action(262-299)src/models/requests.py (1)
FeedbackRequest(239-381)
src/app/endpoints/config.py (5)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(102-113)src/configuration.py (1)
configuration(56-61)src/models/config.py (3)
config(113-119)Action(262-299)Configuration(406-423)src/utils/endpoints.py (1)
check_configuration_loaded(44-50)
tests/unit/app/endpoints/test_conversations.py (4)
src/models/config.py (2)
config(113-119)Action(262-299)src/models/responses.py (3)
ConversationResponse(360-407)ConversationDeleteResponse(410-443)ConversationsListResponse(476-529)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)src/app/endpoints/conversations.py (3)
get_conversation_endpoint_handler(213-320)delete_conversation_endpoint_handler(327-425)get_conversations_list_endpoint_handler(152-208)
src/app/endpoints/metrics.py (4)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(102-113)src/models/config.py (2)
config(113-119)Action(262-299)src/metrics/utils.py (1)
setup_model_metrics(13-50)
⏰ 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 (24)
tests/unit/models/test_config.py (1)
556-556: Good addition: assert the presence of the new top-level "authorization" sectionThis ensures the serialization contract includes the new field. Looks correct.
src/app/endpoints/authorized.py (1)
4-9: Type-annotating auth as Annotated[AuthTuple, Depends(...)] is aligned with the new auth/authorization pipelineThis keeps FastAPI DI explicit and consistent with the rest of the endpoints. Import placement and usage look good.
tests/unit/app/endpoints/test_info.py (1)
11-13: Async conversion and resolver mocking look goodThe test now mirrors the DI and authorization-aware path while keeping the endpoint assertions intact. Nice.
tests/unit/app/endpoints/test_models.py (1)
17-18: Good: centralized bypass via mock_authorization_resolversThis keeps tests focused on endpoint behavior without coupling to authorization policy.
src/app/endpoints/metrics.py (1)
23-27: Good: endpoint gated by RBAC and DI’d auth.The signature aligns with the middleware expectations (
authkwarg present), and the action chosen (GET_METRICS) is appropriate.src/app/endpoints/config.py (1)
64-68: RBAC gate and DI look consistent; handler remains simple and clear.The
@authorize(Action.GET_CONFIG)andauthinjection match the new authorization layer. Returning the Pydantic Configuration object unchanged preserves existing behavior.tests/unit/app/endpoints/test_health.py (2)
16-20: Tests correctly bypass authorization via mocks.Using
mock_authorization_resolvers(mocker)keeps these unit tests focused on health logic instead of auth plumbing. Good separation of concerns.
80-87: Liveness test is aligned with async handler.
liveness_probe_get_methodis async now; awaiting it here is correct. Assertions are minimal and sufficient.tests/unit/app/endpoints/test_conversations.py (16)
1-1: LGTM: keep pylint pragmaDisabling redefined-outer-name is justified for pytest fixtures/mocks in this module.
5-5: LGTM: Request import addedImporting Request is necessary for constructing the dummy Request fixture.
15-15: LGTM: Action enum importBringing Action into tests clarifies authorized_actions usage and improves readability.
253-263: LGTM: configuration-not-loaded path coveredThe test clearly asserts 500 with an explicit message when configuration is None.
269-287: LGTM: invalid ID path coveredGood verification of 400 and inclusion of the invalid ID in the cause.
289-316: LGTM: Llama Stack connection error surfaced as 503Accurately simulates APIConnectionError and asserts the correct response.
318-348: LGTM: NotFound propagated as 404Solid coverage of NotFoundError surface and messaging.
350-379: LGTM: generic retrieval failure -> 500Covers the catch-all error path and verifies the error detail/cause.
427-437: LGTM: delete endpoint handles configuration-not-loadedMirrors the GET endpoint behavior, consistent and correct.
443-461: LGTM: delete endpoint invalid IDGood 400 coverage and cause detail assertion.
463-485: LGTM: delete endpoint connection error -> 503Accurate error mapping and assertion.
491-521: LGTM: delete endpoint NotFound -> 404Good path coverage and messaging verification.
523-547: LGTM: delete endpoint generic failure -> 500Covers the catch-all and validates the cause string.
595-605: LGTM: list endpoint configuration-not-loadedConsistent 500 handling; matches other endpoints.
655-669: LGTM: empty list behaviorConfirms empty response shape without errors.
675-687: LGTM: database exception surfaced as 500Covers server error path and verifies cause.
This commit adds a second authorization layer that evaluates what
authenticated users are allowed to do, separate from whether they can
authenticate at all. The authorization system sits between endpoint
handlers and the existing auth dependency injection.
# Architecture
The system uses two resolver interfaces that can be swapped based on
auth module (although `AccessResolver` shouldn't change much across
different auth modules):
- `RolesResolver`: given an `AuthTuple`, determine what roles the user has
- `AccessResolver`: given roles and an action, determine if access is allowed
For JWT auth, roles are extracted by applying user-provided rules
containing JSONPath expressions to JWT claims . Each role rule specifies
a JSONPath, operator (equals/contains/in), a value, and the resulting
roles to apply if the rule matches the user claims.
This lets you map claim values like `department: "engineering"` to roles
like "developer". Roles are arbitrary except for the special `*` role
which applies to everyone automatically.
Access rules then map roles to permitted actions.
# Implementation
The `@authorize(action)` decorator wraps endpoint functions and performs the check:
1. Extract `AuthTuple` from endpoint dependencies
2. Resolve user roles from auth credentials using the `RolesResolver`
3. Check if those roles permit the requested action using the
`AccessResolver`
4. Raise 403 if denied, continue if allowed
All endpoints now declare what action they perform
(`READ_CONVERSATIONS`, `FEEDBACK`, etc.) through the `@authorize`
decorator. The middleware automatically handles the permission check
before the endpoint runs.
The middleware also populates the `request: Request` `state` property
with the `authorized_actions` set, which contains all actions the user
is allowed to perform based on their roles. Endpoints can then inspect
this property to dynamically adjust their behavior based on what the
user is allowed to do, for special actions such as listing others'
conversations (as opposed to listing only the user's own conversations)
or deleting conversations. These behaviors which are more "complicated"
than just whether the endpoint is accessible or not, as they depend on
the actual endpoint logic.
# Backwards compatibility
For non-JWT auth modules, we will default to no-op resolvers that allow
all access, maintaining current behavior.
# Technical notes
- All endpoints should accept `auth: Any =
Depends(get_auth_dependency())` to use the authorization system. The
parameter must be named `auth` to be recognized by the middleware.
# Config
Example configuration:
```yaml
authentication:
module: jwk-token
jwk_config:
url: ${SSO_BASE_URL}/protocol/openid-connect/certs
jwt_configuration:
user_id_claim: ${USER_ID_CLAIM}
username_claim: ${USERNAME_CLAIM}
role_rules:
- jsonpath: "$.realm_access.roles[*]"
operator: "contains"
value: "redhat:employees"
roles: ["redhat_employee"]
- jsonpath: "$.realm_access.roles[*]"
operator: "contains"
value: "candlepin_system_access_view_edit_all"
roles: ["read_only_admin"]
authorization:
access_rules:
- role: "admin"
actions:
- query_other_conversations
- delete_other_conversations
- role: "read_only_admin"
actions:
- list_other_conversations
- read_other_conversations
- role: "redhat_employee"
actions:
- get_models
- get_config
- role: "*"
actions:
- query
- streaming_query
- get_conversation
- list_conversations
- delete_conversation
- feedback
- get_metrics
- info
```
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/endpoints/feedback.py (1)
108-108: Avoid logging full user-submitted payloads (PII leakage risk)
feedback_requestcan include sensitive content. Log minimal metadata instead (IDs only).- logger.debug("Feedback received %s", str(feedback_request)) + logger.debug( + "Feedback received for conversation_id=%s sentiment=%s", + getattr(feedback_request, "conversation_id", None), + getattr(feedback_request, "sentiment", None), + )
♻️ Duplicate comments (6)
src/app/endpoints/root.py (1)
781-785: Fix validated — authorize wrapper now calls the endpoint.Prior review flagged that the decorator swallowed the call; current authorize() correctly awaits the check and then awaits/returns the wrapped handler. Endpoints will execute as expected.
src/app/endpoints/config.py (1)
64-68: Authorization decorator wiring looks correct; prior call-through concern is resolved
@authorize(Action.GET_CONFIG)is applied and, per the currentauthorization.middleware.authorizeimplementation, the wrapper awaits the original handler. This addresses the earlier review concern about not calling the wrapped function.src/app/endpoints/feedback.py (2)
83-89: Include a FastAPI Request parameter so the decorator can populate request.state.authorized_actionsOther endpoints pass
request: Request, enabling the authorization layer to store per-request allowed actions. Add it here for consistency and future-proofing.@router.post("", responses=feedback_response) @authorize(Action.FEEDBACK) async def feedback_endpoint_handler( - feedback_request: FeedbackRequest, + feedback_request: FeedbackRequest, + request: Request, auth: Annotated[AuthTuple, Depends(auth_dependency)], _ensure_feedback_enabled: Any = Depends(assert_feedback_enabled), ) -> FeedbackResponse:
85-101: Good: payload parameter renamed to avoid collision with FastAPI RequestRenaming to
feedback_requestand updating storage/logging addressed the earlier collision risk noted in prior reviews.src/models/config.py (2)
319-321: Good fix: safe default for access_rulesUsing
Field(default_factory=list)prevents shared mutable defaults.
329-331: Good fix: safe default for JWT role rules
Field(default_factory=list)forJwtConfiguration.role_rulesavoids cross-instance state.
🧹 Nitpick comments (25)
src/utils/endpoints.py (2)
31-41: Prefer PK lookup when bypassing ownership and use one_or_none() for uniqueness.Since id is the PK, use session.get() when others_allowed=True. For the ownership path, one_or_none() better reflects the uniqueness constraint than first().
Apply:
@@ - with get_session() as session: - conversation_query = session.query(UserConversation) - - filtered_conversation_query = ( - conversation_query.filter_by(id=conversation_id) - if others_allowed - else conversation_query.filter_by(id=conversation_id, user_id=user_id) - ) - - conversation: UserConversation | None = filtered_conversation_query.first() + with get_session() as session: + if others_allowed: + # PK lookup is fastest when ownership checks are bypassed + conversation: UserConversation | None = session.get( + UserConversation, conversation_id + ) + else: + conversation = ( + session.query(UserConversation) + .filter_by(id=conversation_id, user_id=user_id) + .one_or_none() + ) return conversationOptional audit line (only if allowed by policy):
+ if conversation and others_allowed and conversation.user_id != user_id: + logger.info( + "Cross-user conversation access: actor_user_id=%s target_conversation_id=%s owner_user_id=%s", + user_id, conversation_id, conversation.user_id + )
22-29: others_allowed gating verified; optional audit logging recommended
- Safety ✅: All invocations of
validate_conversation_ownershipin
•streaming_query.py(line 551)
•query.py(lines 184–189)
•conversations.py(lines 250–255, 358–363)
deriveothers_allowedexclusively from the presence of the correspondingAction.*_OTHERS_CONVERSATIONSinrequest.state.authorized_actions. No unintended code paths allow ownership bypass.- Observability 🔍 (optional): Currently, when
others_allowed=Trueand the conversation belongs to a different user, no audit record is emitted. To improve traceability, consider logging an audit-level entry—avoiding any PII—when this branch is exercised.Suggested snippet (in
validate_conversation_ownershipafter the ownership check):if others_allowed and conversation.user_id != user_id: logger.info( "Audit: user %s accessed conversation %s owned by %s with elevated rights", user_id, conversation_id, conversation.user_id, extra={"audit": True, "conversation_id": conversation_id}, )src/configuration.py (1)
108-119: Good defaulting behavior; add a debug log to surface when the no-op authorization config is used.Returning an empty AuthorizationConfiguration keeps callers simple. A small debug log will help operators notice when the service runs without explicit access rules.
Apply:
def authorization_configuration(self) -> AuthorizationConfiguration: """Return authorization configuration or default no-op configuration.""" assert ( self._configuration is not None ), "logic error: configuration is not loaded" if self._configuration.authorization is None: + logger.debug( + "No authorization configuration provided; using default no-op configuration." + ) return AuthorizationConfiguration() return self._configuration.authorizationtests/unit/app/endpoints/test_info.py (2)
39-43: Patch configuration before importing the endpoint to avoid import-time coupling.If any endpoint module constructs auth_dependency at import time, importing before patching the configuration can be brittle. Import inside the test after patching the configuration (or in a fixture).
Apply outside this hunk:
- from app.endpoints.info import info_endpoint_handler + # Defer import until after configuration is patched (see below)And inside the test, after the patch:
@@ - mocker.patch("configuration.configuration", cfg) + mocker.patch("configuration.configuration", cfg) + # Import after patch so any module-level dependencies resolve with this cfg + from app.endpoints.info import info_endpoint_handler # noqa: WPS433
51-53: Strengthen assertions to validate actual values, not just presence.Confirm the endpoint returns the configured name and a non-empty version.
Apply:
- assert response is not None - assert response.name is not None - assert response.version is not None + assert response is not None + assert response.name == "foo" + assert isinstance(response.version, str) and response.version.strip()src/app/endpoints/root.py (1)
787-792: Condense unused-variable suppression.Minor: combine the two no-op assignments for clarity.
Apply:
- # Used only for authorization - _ = auth - - # Nothing interesting in the request - _ = request + # Unused, kept for DI and authorization side-effects + _ = (auth, request)src/app/endpoints/config.py (1)
63-69: Expose a response_model for accurate OpenAPI schemaAdding
response_model=Configurationwill document the endpoint more precisely in Swagger/Redoc and validate return shapes.-@router.get("/config", responses=get_config_responses) +@router.get("/config", response_model=Configuration, responses=get_config_responses) @authorize(Action.GET_CONFIG) async def config_endpoint_handler( auth: Annotated[AuthTuple, Depends(auth_dependency)], request: Request, ) -> Configuration:tests/unit/authorization/test_resolvers.py (3)
35-38: Use json.dumps for JWT claims to avoid brittle string conversions
str(dict).replace("'", '"')can break on nested quotes/ordering. Preferjson.dumps.+import json @@ - auth = ("user", "token", str(jwt_claims).replace("'", '"')) + auth = ("user", "token", json.dumps(jwt_claims))
59-62: Same here — serialize JWT claims with json.dumps- auth = ("user", "token", str(jwt_claims).replace("'", '"')) + auth = ("user", "token", json.dumps(jwt_claims))
65-101: Async tests are supported via pytest-asyncio’s auto mode—conversion to sync is purely optionalThe project’s
pyproject.tomlenablesasyncio_mode = "auto"under[tool.pytest.ini_options], soasync deftests will be collected and run without needing an explicit@pytest.mark.asynciodecorator. No change is strictly required here.If you’d still prefer to keep all tests synchronous for consistency or clarity, you can refactor the three methods in
tests/unit/authorization/test_resolvers.pyfromasync defto plaindef, but this is purely an optional cleanup.• Location:
tests/unit/authorization/test_resolvers.py:65–101• Optional refactor (if desired):
class TestGenericAccessResolver: """Test cases for GenericAccessResolver.""" - async def test_check_access_with_valid_role(self): + def test_check_access_with_valid_role(self): """Test access check with valid role.""" … - async def test_check_access_with_invalid_role(self): + def test_check_access_with_invalid_role(self): … - async def test_check_access_with_no_roles(self): + def test_check_access_with_no_roles(self): …src/app/endpoints/feedback.py (1)
63-71: Align docstring param name with function signatureThe docstring refers to
request, but the param is_request. Either rename the param or adjust docs. Since the parameter is intentionally unused, keeping the underscore is fine—just sync the docstring.-async def assert_feedback_enabled(_request: Request) -> None: +async def assert_feedback_enabled(request: Request) -> None: @@ - Args: - request (Request): The FastAPI request object. + Args: + request (Request): The FastAPI request object.src/models/config.py (3)
324-341: Avoid defaulting to a shared JwtConfiguration instancePydantic models as direct defaults can leak state across instances. Use a factory.
class JwkConfiguration(BaseModel): """JWK configuration.""" url: AnyHttpUrl - jwt_configuration: JwtConfiguration = JwtConfiguration() + jwt_configuration: JwtConfiguration = Field(default_factory=JwtConfiguration)
417-429: Avoid mutable default list for mcp_serversUse a default factory to prevent shared state across Configuration instances.
class Configuration(BaseModel): @@ - mcp_servers: list[ModelContextProtocolServer] = [] + mcp_servers: list[ModelContextProtocolServer] = Field(default_factory=list)
231-267: JwtRoleRule validation is solid; consider one extra guardYou already enforce non-empty, uniqueness, and disallow "*". Optionally also validate that each role is non-empty and trimmed (no whitespace-only roles) to fail-fast on typos.
src/app/endpoints/conversations.py (2)
241-246: Error message says UUID but IDs are SUIDsFor consistency with
check_suid, update the message to avoid confusing users.- "cause": f"Conversation ID {conversation_id} is not a valid UUID", + "cause": f"Conversation ID {conversation_id} is not a valid SUID",
150-198: Consider pagination for conversation listingListing all conversations (especially with LIST_OTHERS_CONVERSATIONS) can be heavy. Add
limit/offset(or cursor) query params and DB indexes to keep response sizes and query times bounded.tests/unit/app/endpoints/test_conversations.py (9)
29-41: Solid dummy_request fixture; consider centralizing and enabling per-test overridesFixture correctly simulates middleware by setting request.state.authorized_actions. To reduce duplication (there’s a similar helper in tests/unit/app/endpoints/test_query.py) and promote reuse, consider moving this to a shared conftest.py and, if needed, add a helper to override authorized_actions in specific tests.
69-74: Mock session builder is good; add assertions to verify the filtered vs unfiltered pathsThe nested query mock supports both query().all() and query().filter_by().all() paths. To validate RBAC effects, add assertions in list-endpoint tests that filter_by was or was not called depending on authorized_actions.
182-247: Async markers on purely sync simplify_session_data tests are unnecessaryThese tests don’t await anything. Dropping @pytest.mark.asyncio and making them sync will run faster and simplify the event loop context. Not critical, just tidier.
381-421: Add assertion that session.retrieve was called with the resolved session_idVerifies that the handler uses the first session’s session_id when calling retrieve, matching production logic.
Apply this diff near the existing assertions:
assert response.chat_history == expected_chat_history mock_client.agents.session.list.assert_called_once_with( agent_id=VALID_CONVERSATION_ID ) + mock_client.agents.session.retrieve.assert_called_once_with( + agent_id=VALID_CONVERSATION_ID, session_id=VALID_CONVERSATION_ID + )
463-489: Stabilize the error-path by returning a concrete list() result before deleteCurrently, delete raises APIConnectionError, but the test relies on default Mock behavior for the preceding list() call. Make the flow deterministic by ensuring list() returns a session with a real session_id.
Apply this diff after setting the delete side effect and before invoking the handler:
mock_client = mocker.AsyncMock() mock_client.agents.session.delete.side_effect = APIConnectionError(request=None) mock_client_holder = mocker.patch( "app.endpoints.conversations.AsyncLlamaStackClientHolder" ) mock_client_holder.return_value.get_client.return_value = mock_client + mock_client.agents.session.list.return_value = mocker.Mock( + data=[{"session_id": VALID_CONVERSATION_ID}] + )
491-521: Same determinism for NotFoundError path: seed list() with a sessionWithout an explicit list() result, test behavior depends on Mock quirks. Seed a concrete session to guarantee the code reaches delete().
Apply this diff:
mock_client = mocker.AsyncMock() mock_client.agents.session.delete.side_effect = NotFoundError( message="Session not found", response=mocker.Mock(request=None), body=None ) mock_client_holder = mocker.patch( "app.endpoints.conversations.AsyncLlamaStackClientHolder" ) mock_client_holder.return_value.get_client.return_value = mock_client + mock_client.agents.session.list.return_value = mocker.Mock( + data=[{"session_id": VALID_CONVERSATION_ID}] + )
523-555: Same determinism for general-exception path: seed list() with a sessionEnsure the handler reaches delete() before raising the injected exception.
Apply this diff:
mock_client = mocker.AsyncMock() mock_client.agents.session.delete.side_effect = Exception( "Session deletion failed" ) mock_client_holder = mocker.patch( "app.endpoints.conversations.AsyncLlamaStackClientHolder" ) mock_client_holder.return_value.get_client.return_value = mock_client + mock_client.agents.session.list.return_value = mocker.Mock( + data=[{"session_id": VALID_CONVERSATION_ID}] + )
609-653: Strengthen list “happy-path” assertions to cover all serialized fieldsYou already check IDs. Verifying timestamps, counts, and metadata increases confidence that conversation summaries serialize as expected.
Apply this diff to add field-level checks:
assert ( response.conversations[1].conversation_id == "456e7890-e12b-34d5-a678-901234567890" ) + # Verify full serialization of the first conversation + assert response.conversations[0].created_at == "2024-01-01T00:00:00Z" + assert response.conversations[0].last_message_at == "2024-01-01T00:05:00Z" + assert response.conversations[0].message_count == 5 + assert response.conversations[0].last_used_model == "gemini/gemini-2.0-flash" + assert response.conversations[0].last_used_provider == "gemini"
595-605: Add coverage for RBAC gating and “no sessions” branchesTwo meaningful gaps given the new authorization layer:
- Verify filtering when LIST_OTHERS_CONVERSATIONS is absent (assert filter_by(user_id=...) was used).
- Exercise branches where Llama Stack returns no sessions for GET (404) and DELETE (idempotent success).
I can draft these tests for you. Examples:
@pytest.mark.asyncio async def test_list_endpoint_filters_without_list_others(mocker, setup_configuration, dummy_request): mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) # Only allow listing own conversations dummy_request.state.authorized_actions = {Action.LIST_CONVERSATIONS} mock_session = mock_database_session(mocker, []) await get_conversations_list_endpoint_handler(request=dummy_request, auth=MOCK_AUTH) mock_session.query.return_value.filter_by.assert_called_once_with(user_id=MOCK_AUTH[0]) @pytest.mark.asyncio async def test_get_conversation_no_sessions_returns_404(mocker, setup_configuration, dummy_request): mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) mocker.patch("app.endpoints.conversations.validate_conversation_ownership") mock_client = mocker.AsyncMock() mock_client.agents.session.list.return_value = mocker.Mock(data=[]) mocker.patch("app.endpoints.conversations.AsyncLlamaStackClientHolder").return_value.get_client.return_value = mock_client with pytest.raises(HTTPException) as exc_info: await get_conversation_endpoint_handler(request=dummy_request, conversation_id=VALID_CONVERSATION_ID, auth=MOCK_AUTH) assert exc_info.value.status_code == status.HTTP_404_NOT_FOUND @pytest.mark.asyncio async def test_delete_conversation_no_sessions_idempotent_success(mocker, setup_configuration, dummy_request): mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) mocker.patch("app.endpoints.conversations.validate_conversation_ownership") mock_client = mocker.AsyncMock() mock_client.agents.session.list.return_value = mocker.Mock(data=[]) mocker.patch("app.endpoints.conversations.AsyncLlamaStackClientHolder").return_value.get_client.return_value = mock_client resp = await delete_conversation_endpoint_handler(request=dummy_request, conversation_id=VALID_CONVERSATION_ID, auth=MOCK_AUTH) assert resp.success is TrueOptionally, add a forbidden scenario to prove others_allowed=False is honored:
@pytest.mark.asyncio async def test_get_conversation_forbidden_without_read_others(mocker, setup_configuration, dummy_request): mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) # Exclude READ_OTHERS_CONVERSATIONS so ownership is required dummy_request.state.authorized_actions = set() validate_mock = mocker.patch( "app.endpoints.conversations.validate_conversation_ownership", side_effect=HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail={"response": "Forbidden", "cause": "Not owner"}) ) with pytest.raises(HTTPException) as exc_info: await get_conversation_endpoint_handler(request=dummy_request, conversation_id=VALID_CONVERSATION_ID, auth=MOCK_AUTH) assert exc_info.value.status_code == status.HTTP_403_FORBIDDEN validate_mock.assert_called_once() _, kwargs = validate_mock.call_args assert kwargs["others_allowed"] is FalseAlso applies to: 640-642, 666-668, 685-687
📜 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 ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
pyproject.toml(1 hunks)src/app/endpoints/authorized.py(2 hunks)src/app/endpoints/config.py(3 hunks)src/app/endpoints/conversations.py(7 hunks)src/app/endpoints/feedback.py(2 hunks)src/app/endpoints/health.py(4 hunks)src/app/endpoints/info.py(3 hunks)src/app/endpoints/metrics.py(2 hunks)src/app/endpoints/models.py(3 hunks)src/app/endpoints/query.py(3 hunks)src/app/endpoints/root.py(2 hunks)src/app/endpoints/streaming_query.py(3 hunks)src/auth/jwk_token.py(2 hunks)src/authorization/__init__.py(1 hunks)src/authorization/middleware.py(1 hunks)src/authorization/resolvers.py(1 hunks)src/configuration.py(2 hunks)src/models/config.py(3 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_config.py(3 hunks)tests/unit/app/endpoints/test_conversations.py(20 hunks)tests/unit/app/endpoints/test_feedback.py(5 hunks)tests/unit/app/endpoints/test_health.py(4 hunks)tests/unit/app/endpoints/test_info.py(2 hunks)tests/unit/app/endpoints/test_metrics.py(2 hunks)tests/unit/app/endpoints/test_models.py(10 hunks)tests/unit/app/endpoints/test_query.py(13 hunks)tests/unit/app/endpoints/test_root.py(1 hunks)tests/unit/auth/test_jwk_token.py(2 hunks)tests/unit/authorization/__init__.py(1 hunks)tests/unit/authorization/test_resolvers.py(1 hunks)tests/unit/models/test_config.py(2 hunks)tests/unit/utils/auth_helpers.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/authorization/init.py
🚧 Files skipped from review as they are similar to previous changes (21)
- src/auth/jwk_token.py
- src/app/endpoints/query.py
- src/authorization/middleware.py
- src/app/endpoints/authorized.py
- tests/unit/models/test_config.py
- tests/unit/app/endpoints/test_config.py
- tests/unit/auth/test_jwk_token.py
- tests/unit/app/endpoints/test_query.py
- tests/unit/app/endpoints/test_models.py
- src/app/endpoints/metrics.py
- src/app/endpoints/streaming_query.py
- pyproject.toml
- tests/unit/utils/auth_helpers.py
- src/app/endpoints/info.py
- tests/unit/app/endpoints/test_metrics.py
- tests/unit/app/endpoints/test_root.py
- src/app/endpoints/health.py
- src/app/endpoints/models.py
- tests/unit/app/endpoints/test_feedback.py
- src/authorization/resolvers.py
- tests/unit/app/endpoints/test_health.py
🧰 Additional context used
🧬 Code graph analysis (9)
tests/unit/authorization/test_resolvers.py (2)
src/authorization/resolvers.py (8)
JwtRolesResolver(40-100)GenericAccessResolver(130-186)resolve_roles(27-28)resolve_roles(34-37)resolve_roles(47-54)check_access(107-108)check_access(118-122)check_access(155-172)src/models/config.py (5)
config(120-126)JwtRoleRule(231-266)AccessRule(309-313)JsonPathOperator(223-228)Action(269-306)
src/app/endpoints/config.py (4)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(104-115)src/models/config.py (3)
config(120-126)Action(269-306)Configuration(417-434)src/utils/endpoints.py (1)
check_configuration_loaded(44-50)
tests/unit/app/endpoints/test_info.py (3)
src/app/endpoints/info.py (1)
info_endpoint_handler(33-52)src/configuration.py (3)
configuration(56-61)AppConfig(28-142)init_from_dict(51-53)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)
src/utils/endpoints.py (2)
src/models/database/conversations.py (1)
UserConversation(11-36)src/app/database.py (1)
get_session(34-40)
src/app/endpoints/feedback.py (4)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(104-115)src/models/config.py (2)
config(120-126)Action(269-306)src/models/requests.py (1)
FeedbackRequest(239-381)
src/configuration.py (1)
src/models/config.py (1)
AuthorizationConfiguration(316-321)
src/app/endpoints/conversations.py (5)
src/app/database.py (1)
get_session(34-40)src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(104-115)src/models/config.py (2)
config(120-126)Action(269-306)src/models/database/conversations.py (1)
UserConversation(11-36)
tests/unit/app/endpoints/test_conversations.py (4)
src/models/config.py (2)
config(120-126)Action(269-306)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(9-27)tests/unit/app/endpoints/test_query.py (1)
dummy_request(37-46)src/app/endpoints/conversations.py (3)
get_conversation_endpoint_handler(213-320)delete_conversation_endpoint_handler(327-425)get_conversations_list_endpoint_handler(152-208)
src/app/endpoints/root.py (3)
src/auth/__init__.py (1)
get_auth_dependency(14-43)src/authorization/middleware.py (1)
authorize(104-115)src/models/config.py (2)
config(120-126)Action(269-306)
🔇 Additional comments (6)
src/authorization/__init__.py (1)
1-1: LGTM — clear package-level docstring.No issues. Docstring succinctly describes the module’s purpose.
src/configuration.py (1)
12-12: LGTM — import of AuthorizationConfiguration is correct and required.src/app/endpoints/root.py (1)
17-18: Deferget_auth_dependency()invocation out of module import and into startup or enforce import‐orderOur search confirmed that all endpoint modules initialize the auth dependency at import time, which risks triggering configuration‐loaded assertions if this package is ever imported before your
AppConfigis initialized. Affected files:
- src/app/endpoints/streaming_query.py:51
- src/app/endpoints/query.py:45
- src/app/endpoints/models.py:23
- src/app/endpoints/metrics.py:19
- src/app/endpoints/info.py:20
- src/app/endpoints/root.py:17
- src/app/endpoints/health.py:28
- src/app/endpoints/feedback.py:27
- src/app/endpoints/conversations.py:28
- src/app/endpoints/authorized.py:14
- src/app/endpoints/config.py:18
Two ways to address this:
- Document & enforce that all imports of
src/app/endpoints/**occur only afterAppConfig.init_from_dict/load_configuration()has completed.- Refactor to defer calling
get_auth_dependency()until application startup (e.g. in a FastAPI@app.on_event("startup")hook), then assign it to each router’s dependency at runtime instead of at import time.Please review the implementation of
get_auth_dependency()and your configuration bootstrap to confirm whether it truly depends on loaded config at import time, and then choose one of the above approaches to prevent unexpected assertion failures.src/app/endpoints/conversations.py (1)
150-156: RBAC integration on list endpoint looks goodDecorator, DI, and async signature align with the new authorization layer.
tests/unit/app/endpoints/test_conversations.py (2)
252-267: GET endpoint negative paths look comprehensive and correctly wire mocksConfiguration-not-loaded, invalid ID, connection, not-found, and unknown-error cases are all covered and assertions match the endpoint contract.
Also applies to: 269-287, 289-316, 318-348, 350-379
426-441: DELETE endpoint positive/negative paths are exercised; see notes above for deterministic setupCoverage looks good, especially the successful delete path asserting exact client call args. With the minor list() seeding tweak, the error paths will be robust as well.
Also applies to: 443-461, 556-589
| filtered_query = ( | ||
| query | ||
| if Action.LIST_OTHERS_CONVERSATIONS in request.state.authorized_actions | ||
| else query.filter_by(user_id=user_id) | ||
| ) | ||
|
|
||
| user_conversations = filtered_query.all() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing request.state.authorized_actions
If the decorator/middleware doesn’t populate authorized_actions (misconfiguration or legacy auth), this will raise AttributeError. Default to least privilege.
- filtered_query = (
- query
- if Action.LIST_OTHERS_CONVERSATIONS in request.state.authorized_actions
- else query.filter_by(user_id=user_id)
- )
+ allowed = getattr(request.state, "authorized_actions", set())
+ filtered_query = (
+ query
+ if Action.LIST_OTHERS_CONVERSATIONS in allowed
+ else query.filter_by(user_id=user_id)
+ )📝 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.
| filtered_query = ( | |
| query | |
| if Action.LIST_OTHERS_CONVERSATIONS in request.state.authorized_actions | |
| else query.filter_by(user_id=user_id) | |
| ) | |
| user_conversations = filtered_query.all() | |
| allowed = getattr(request.state, "authorized_actions", set()) | |
| filtered_query = ( | |
| query | |
| if Action.LIST_OTHERS_CONVERSATIONS in allowed | |
| else query.filter_by(user_id=user_id) | |
| ) | |
| user_conversations = filtered_query.all() |
🤖 Prompt for AI Agents
In src/app/endpoints/conversations.py around lines 167 to 174, the code assumes
request.state.authorized_actions exists and will raise AttributeError if
middleware/decorator didn't set it; change the check to safely read
authorized_actions with a default least-privilege value (e.g., actions =
getattr(request.state, "authorized_actions", set()) or actions or set()), then
use that actions set in the membership test so that missing or None
authorized_actions defaults to no privileges and the query is filtered by
user_id.
| others_allowed=( | ||
| Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same defensive default when checking READ_OTHERS_CONVERSATIONS
- others_allowed=(
- Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions
- ),
+ others_allowed=(
+ Action.READ_OTHERS_CONVERSATIONS
+ in getattr(request.state, "authorized_actions", set())
+ ),📝 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.
| others_allowed=( | |
| Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions | |
| ), | |
| ) | |
| others_allowed=( | |
| Action.READ_OTHERS_CONVERSATIONS | |
| in getattr(request.state, "authorized_actions", set()) | |
| ), |
🤖 Prompt for AI Agents
In src/app/endpoints/conversations.py around lines 253 to 256, the membership
check for Action.READ_OTHERS_CONVERSATIONS assumes
request.state.authorized_actions is always present and iterable; make it
defensive by using a safe default (e.g., getattr(request.state,
"authorized_actions", set()) or (request.state.authorized_actions or set()))
when performing the "in" check so it won't raise if authorized_actions is None
or missing.
| others_allowed=( | ||
| Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same defensive default when checking DELETE_OTHERS_CONVERSATIONS
- others_allowed=(
- Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions
- ),
+ others_allowed=(
+ Action.DELETE_OTHERS_CONVERSATIONS
+ in getattr(request.state, "authorized_actions", set())
+ ),📝 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.
| others_allowed=( | |
| Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions | |
| ), | |
| ) | |
| others_allowed=( | |
| Action.DELETE_OTHERS_CONVERSATIONS | |
| in getattr(request.state, "authorized_actions", set()) | |
| ), | |
| ) |
🤖 Prompt for AI Agents
In src/app/endpoints/conversations.py around lines 361 to 364, the membership
check for DELETE_OTHERS_CONVERSATIONS should defensively handle missing or None
authorized_actions; change the expression to check membership against a safe
default (e.g., getattr(request.state, "authorized_actions", set()) or an empty
iterable) so the code won't raise if authorized_actions is absent or null.
| async def test_resolve_roles_redhat_employee(self): | ||
| """Test role extraction for RedHat employee JWT.""" | ||
| role_rules = [ | ||
| JwtRoleRule( | ||
| jsonpath="$.realm_access.roles[*]", | ||
| operator=JsonPathOperator.CONTAINS, | ||
| value="redhat:employees", | ||
| roles=["employee"], | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Add test coverage for negate/EQUALS/IN operators and wildcard-role validation
Current tests exercise only CONTAINS. Add cases for:
- EQUALS with scalar equality
- IN semantics
- negate=True variants
- Validator errors (e.g., roles=[], duplicate roles, roles containing "*")
Also, there is likely a bug in IN evaluation: JwtRolesResolver._evaluate_operator receives a list of matches but checks match in value. That compares the entire list against the container. It should check any element membership.
Proposed resolver fix (outside this file):
--- a/src/authorization/resolvers.py
+++ b/src/authorization/resolvers.py
@@ class JwtRolesResolver(RolesResolver):
- return (
- set(rule.roles)
- if JwtRolesResolver._evaluate_operator(
- rule.negate,
- [match.value for match in parse(rule.jsonpath).find(jwt_claims)],
- rule.operator,
- rule.value,
- )
- else set()
- )
+ matches = [m.value for m in parse(rule.jsonpath).find(jwt_claims)]
+ return set(rule.roles) if JwtRolesResolver._evaluate_operator(
+ rule.negate, matches, rule.operator, rule.value
+ ) else set()
@@
- def _evaluate_operator(
- negate: bool, match: Any, operator: JsonPathOperator, value: Any
+ def _evaluate_operator(
+ negate: bool, matches: list[Any], operator: JsonPathOperator, value: Any
) -> bool: # pylint: disable=too-many-branches
"""Evaluate an operator against a match and value."""
result = False
match operator:
case JsonPathOperator.EQUALS:
- result = match == value
+ result = matches == [value] or any(m == value for m in matches)
case JsonPathOperator.CONTAINS:
- result = value in match
+ result = any(
+ (isinstance(m, (list, set, tuple, str)) and value in m)
+ for m in matches
+ )
case JsonPathOperator.IN:
- result = match in value
+ result = any(m in value for m in matches) if hasattr(value, "__contains__") else FalseHappy to submit companion tests for these cases if you want.
🤖 Prompt for AI Agents
In tests/unit/authorization/test_resolvers.py around lines 10 to 19, add unit
tests that cover the missing operator and validation cases: create test cases
for EQUALS (scalar equality), IN semantics (ensure any element of the jsonpath
matches membership), negate=True variants for CONTAINS/EQUALS/IN, and validator
error scenarios (roles=[], duplicate roles, and roles containing the wildcard
"*" which should raise). Also update assertions to reflect the expected role
list or raised validation errors. Separately, fix the resolver's IN evaluation
to iterate the list of matches and check if any element is in the provided
container (instead of comparing the whole matches list to the container).
| @router.post("", responses=feedback_response) | ||
| def feedback_endpoint_handler( | ||
| @authorize(Action.FEEDBACK) | ||
| async def feedback_endpoint_handler( |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @lru_cache(maxsize=1) |
There was a problem hiding this comment.
Does LRU actually help here?, seems like all this function is doing is a simple switch case
There was a problem hiding this comment.
The instantiation of the access resolver involves creating a lookup dictionary for quick resolution of roles and their allowed actions
There was a problem hiding this comment.
So perhaps the instantiation should use a singleton
There was a problem hiding this comment.
lru_cache1 is effectively a singleton without all the boilerplate
| Configuration: The loaded service configuration object. | ||
| """ | ||
| # Used only for authorization | ||
| _ = auth |
There was a problem hiding this comment.
why it's used like this? Can't you just rename the parameter to _auth and _request?
There was a problem hiding this comment.
No, see my commit message remark under "Technical notes"
| Returns: | ||
| InfoResponse: An object containing the service's name and version. | ||
| """ | ||
| # Used only for authorization |
|
Anything else needs to be done to get this merged? |
Bump lightspeed-stack submodule to include the authorization changes from lightspeed-core/lightspeed-stack#356 Use the new authorization features to restrict access to the assisted-chat service to, for now, Red Hat employees only. In the future we can open this up to all authenticated users.
Bump lightspeed-stack submodule to include the authorization changes from lightspeed-core/lightspeed-stack#356 Use the new authorization features to restrict access to the assisted-chat service to, for now, Red Hat employees only. In the future we can open this up to all authenticated users. Also bumped to a version that no longer requires the patching of llama-stack / lightspeed-stack to support postgres SSL, so removed the patching steps from the Containerfile.
3 participants
Description
This commit adds a second authorization layer that evaluates what authenticated users are allowed to do, separate from whether they can authenticate at all. The authorization system sits between endpoint handlers and the existing auth dependency injection.
Architecture
The system uses two resolver interfaces that can be swapped based on auth module (although
AccessResolvershouldn't change much across different auth modules):RolesResolver: given anAuthTuple, determine what roles the user hasAccessResolver: given roles and an action, determine if access is allowedFor JWT auth, roles are extracted by applying user-provided rules containing JSONPath expressions to JWT claims . Each role rule specifies a JSONPath, operator (equals/contains/in), a value, and the resulting roles to apply if the rule matches the user claims.
This lets you map claim values like
department: "engineering"to roles like "developer". Roles are arbitrary except for the special*role which applies to everyone automatically.Access rules then map roles to permitted actions.
Implementation
The
@authorize(action)decorator wraps endpoint functions and performs the check:AuthTuplefrom endpoint dependenciesRolesResolverAccessResolverAll endpoints now declare what action they perform (
READ_CONVERSATIONS,FEEDBACK, etc.) through the@authorizedecorator. The middleware automatically handles the permission check before the endpoint runs.The middleware also populates the
request: Requeststateproperty with theauthorized_actionsset, which contains all actions the user is allowed to perform based on their roles. Endpoints can then inspect this property to dynamically adjust their behavior based on what the user is allowed to do, for special actions such as listing others' conversations (as opposed to listing only the user's own conversations) or deleting conversations. These behaviors which are more "complicated" than just whether the endpoint is accessible or not, as they depend on the actual endpoint logic.Backwards compatibility
For non-JWT auth modules, we will default to no-op resolvers that allow all access, maintaining current behavior.
Technical notes
auth: Any = Depends(get_auth_dependency())to use the authorization system. The parameter must be namedauthto be recognized by the middleware.Config
Example configuration:
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit