MGMT-21338: allow disabling query model and provider #475
Conversation
WalkthroughAdds RBAC enforcement preventing callers from overriding model/provider in /v1/query and /v1/streaming_query unless they hold a new Action.MODEL_OVERRIDE permission; introduces validate_model_provider_override, extends authorization middleware to find Request in args or kwargs, updates Action enum/OpenAPI, adds tests and README docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant E as Endpoint (/v1/query or /v1/streaming_query)
participant A as Authorization Middleware
participant V as validate_model_provider_override
participant S as Service Logic
C->>E: Send QueryRequest (may include model/provider)
Note over E: check_configuration_loaded(configuration)
E->>A: authorize(..., request via args/kwargs)
A->>A: resolve roles/actions
A-->>E: set request.state.authorized_actions
E->>V: validate_model_provider_override(request, authorized_actions)
alt override present && no MODEL_OVERRIDE
V-->>E: raise HTTPException(403)
E-->>C: 403 Forbidden (remove model/provider)
else allowed or no override
V-->>E: OK
E->>S: proceed with normal request handling
S-->>E: response / stream
E-->>C: 200 OK / streaming response
end
sequenceDiagram
autonumber
participant D as @authorize Decorator
participant H as _perform_authorization_check
participant R as Request (args or kwargs)
D->>H: call with (action, args, kwargs)
alt Request found in kwargs
H->>R: read/set state.authorized_actions
else Request found in positional args
H->>R: locate and set state.authorized_actions
end
H-->>D: continue to wrapped handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
zszabo-rh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/app/endpoints/streaming_query.py (1)
32-37: Consolidate imports from utils.endpoints.Keep related imports grouped; fold the new import into the existing utils.endpoints import block.
-from utils.endpoints import check_configuration_loaded, get_agent, get_system_prompt +from utils.endpoints import ( + check_configuration_loaded, + get_agent, + get_system_prompt, + validate_model_provider_override, +) @@ -from utils.endpoints import validate_model_provider_overridetests/unit/utils/test_endpoints.py (2)
596-606: Rename fixture to reflect semantics (False = overrides allowed).The name “config_with_override_disabled” conflicts with its semantics (flag False => allowed). Rename to avoid confusion.
-@pytest.fixture(name="config_with_override_disabled") -def config_with_override_disabled_fixture(): - """Configuration where overriding model/provider is allowed (flag False).""" +@pytest.fixture(name="config_allow_override") +def config_allow_override_fixture(): + """Configuration where overriding model/provider is allowed (disable flag False).""" @@ -def test_validate_model_provider_override_allowed_when_flag_false( - config_with_override_disabled, +def test_validate_model_provider_override_allowed_when_flag_false( + config_allow_override, ): @@ - endpoints.validate_model_provider_override(query_request, config_with_override_disabled) + endpoints.validate_model_provider_override(query_request, config_allow_override)Also applies to: 620-627
608-618: Rename the “enabled” fixture; True means overrides disallowed.“config_with_override_enabled” suggests the opposite of what the flag enforces. Rename for clarity.
-@pytest.fixture(name="config_with_override_enabled") -def config_with_override_enabled_fixture(): - """Configuration where overriding model/provider is NOT allowed (flag True).""" +@pytest.fixture(name="config_disallow_override") +def config_disallow_override_fixture(): + """Configuration where overriding model/provider is NOT allowed (disable flag True).""" @@ -def test_validate_model_provider_override_rejected_when_flag_true( - config_with_override_enabled, +def test_validate_model_provider_override_rejected_when_flag_true( + config_disallow_override, ): @@ - with pytest.raises(HTTPException) as exc_info: - endpoints.validate_model_provider_override(query_request, config_with_override_enabled) + with pytest.raises(HTTPException) as exc_info: + endpoints.validate_model_provider_override(query_request, config_disallow_override) @@ -def test_validate_model_provider_override_no_override_with_flag_true( - config_with_override_enabled, +def test_validate_model_provider_override_no_override_with_flag_true( + config_disallow_override, ): @@ - endpoints.validate_model_provider_override(query_request, config_with_override_enabled) + endpoints.validate_model_provider_override(query_request, config_disallow_override)Also applies to: 629-637, 639-646
📜 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 (7)
src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)tests/unit/utils/test_endpoints.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/utils/endpoints.py (2)
src/models/requests.py (1)
QueryRequest(71-221)src/configuration.py (2)
AppConfig(32-135)customization(117-121)
tests/unit/app/endpoints/test_streaming_query.py (3)
src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(523-678)
src/app/endpoints/streaming_query.py (1)
src/utils/endpoints.py (1)
validate_model_provider_override(87-106)
tests/unit/utils/test_endpoints.py (3)
src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/utils/endpoints.py (1)
validate_model_provider_override(87-106)
tests/unit/app/endpoints/test_query.py (4)
tests/unit/app/endpoints/test_conversations.py (1)
dummy_request(30-40)src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/query.py (1)
query_endpoint_handler(155-271)
src/app/endpoints/query.py (2)
src/utils/endpoints.py (1)
validate_model_provider_override(87-106)src/configuration.py (1)
configuration(60-64)
🪛 GitHub Actions: Black
src/utils/endpoints.py
[error] 1-1: Black formatting check failed. This file would be reformatted by Black (3 files would be reformatted in total). Run 'black --write' to format. Step: 'uv tool run black --check .'
tests/unit/app/endpoints/test_streaming_query.py
[error] 1-1: Black formatting check failed. This file would be reformatted by Black (3 files would be reformatted in total). Run 'black --write' to format. Step: 'uv tool run black --check .'
tests/unit/utils/test_endpoints.py
[error] 1-1: Black formatting check failed. This file would be reformatted by Black (3 files would be reformatted in total). Run 'black --write' to format. Step: 'uv tool run black --check .'
🪛 GitHub Actions: Python linter
src/utils/endpoints.py
[error] 102-102: Pylint: Line too long (C0301) - exceeds 100 characters (107/100). [Step: uv run pylint src tests]
tests/unit/app/endpoints/test_streaming_query.py
[error] 1522-1522: Pylint: Line too long (C0301) - exceeds 100 characters (119/100). [Step: uv run pylint src tests]
[error] 1566-1566: Pylint: Line too long (C0301) - exceeds 100 characters (201/100). [Step: uv run pylint src tests]
tests/unit/app/endpoints/test_query.py
[error] 1516-1516: Pylint: Line too long (C0301) - exceeds 100 characters (119/100). [Step: uv run pylint src tests]
[error] 1556-1556: Pylint: Line too long (C0301) - exceeds 100 characters (201/100). [Step: uv run pylint src tests]
⏰ 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 (6)
src/models/config.py (1)
391-391: New flag looks correct and non-breakingField name, default (False), and placement in Customization align with existing patterns. No functional concerns.
src/app/endpoints/query.py (2)
38-39: Import placement is fineImporting validate_model_provider_override alongside other endpoint utils keeps cohesion.
178-180: Right place for the guardCalling the validator immediately after checking configuration ensures fast fail and minimal work before rejecting overrides.
src/app/endpoints/streaming_query.py (1)
552-554: Confirmed: bothquery.pyandstreaming_query.pyinvokevalidate_model_provider_overrideimmediately after configuration. Approving code changes.tests/unit/utils/test_endpoints.py (2)
620-646: Tests cover allowed, rejected, and no-override paths. LGTM.Assertions match the utility’s contract and new config flag behavior.
1-1: Reformat code with Black: install theuvCLI locally (e.g.pip install uv), then runmake format(oruv run black .) to fix the formatting errors CI reported, and commit the updated files.
zszabo-rh
force-pushed
the
disable_query_model
branch
from
7d24dd4 to
ec79cb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/endpoints.py (1)
100-109: Deduplicate the exact error message via a named constant and reuse it in testsThis string is replicated across tests; a single source reduces drift.
Apply:
@@ -logger = logging.getLogger("utils.endpoints") +logger = logging.getLogger("utils.endpoints") + +# Public constant to keep error messaging consistent across endpoints and tests. +MODEL_PROVIDER_OVERRIDE_DISABLED_MSG = ( + "This instance does not support overriding model/provider in the query request " + "(disable_query_model_provider_override is set). Please remove the " + "model and provider fields from your request." +) @@ - raise HTTPException( + raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail={ - "response": ( - "This instance does not support overriding model/provider in the query request " - "(disable_query_model_provider_override is set). Please remove the model and " - "provider fields from your request." - ) + "response": MODEL_PROVIDER_OVERRIDE_DISABLED_MSG }, )Optionally import and assert this constant in the endpoint tests instead of rebuilding the string.
📜 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 (7)
src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)tests/unit/utils/test_endpoints.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/models/config.py
- src/app/endpoints/query.py
- tests/unit/utils/test_endpoints.py
- src/app/endpoints/streaming_query.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/app/endpoints/test_query.py (4)
tests/unit/app/endpoints/test_conversations.py (1)
dummy_request(30-40)src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/query.py (1)
query_endpoint_handler(155-271)
tests/unit/app/endpoints/test_streaming_query.py (3)
src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(523-678)
src/utils/endpoints.py (2)
src/models/requests.py (1)
QueryRequest(71-221)src/configuration.py (2)
AppConfig(32-135)customization(117-121)
⏰ 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 (3)
src/utils/endpoints.py (1)
87-110: LGTM: correct 422 guard when overrides are disabledBehavior matches the PR objective and the message is wrapped to satisfy linters. Nice.
tests/unit/app/endpoints/test_query.py (1)
1512-1564: LGTM: negative-path test is precise and lint-safeAsserts correct 422 and message; line-length issue handled via a variable.
tests/unit/app/endpoints/test_streaming_query.py (1)
1520-1576: LGTM: streaming endpoint mirrors the non-streaming validationGood parity test; message wrapped to avoid C0301.
|
Should this simply be a |
zszabo-rh
force-pushed
the
disable_query_model
branch
from
ec79cb9 to
df50b99
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/unit/utils/test_endpoints.py (4)
620-629: Align test with fixture renameIf you accept the rename above, update the parameter and reference in this test.
-def test_validate_model_provider_override_allowed_when_flag_false( - config_with_override_disabled, +def test_validate_model_provider_override_allowed_when_flag_false( + config_with_override_allowed, ): @@ - endpoints.validate_model_provider_override( - query_request, config_with_override_disabled - ) + endpoints.validate_model_provider_override( + query_request, config_with_override_allowed + )
631-641: Also assert error payload to lock the contractYou already assert 422. Add minimal checks on
detailshape and key text so accidental changes won’t silently degrade clients depending on message shape.with pytest.raises(HTTPException) as exc_info: endpoints.validate_model_provider_override( query_request, config_with_override_enabled ) assert exc_info.value.status_code == 422 + # Guardrail: ensure message contract remains stable. + detail = exc_info.value.detail + assert isinstance(detail, dict) and "response" in detail + assert "does not support overriding model/provider" in detail["response"]
643-651: Add coverage for customization=None and empty-string edge caseTwo edge cases worth pinning:
- When
customizationisNone, overrides should be allowed (regression guard).- When model/provider are empty strings, Pydantic validators treat them as falsy; your validator uses
is not None, which correctly flags them. Add a test to prove it.@@ def test_validate_model_provider_override_no_override_with_flag_true( config_with_override_enabled, ): @@ endpoints.validate_model_provider_override( query_request, config_with_override_enabled ) + +def test_validate_model_provider_override_allows_when_customization_none(): + """Overrides allowed when customization is None (flag absent).""" + test_config = config_dict.copy() + test_config["customization"] = None + cfg = AppConfig() + cfg.init_from_dict(test_config) + query_request = QueryRequest(query="q", model="m", provider="p") + # Should not raise + endpoints.validate_model_provider_override(query_request, cfg) + +def test_validate_model_provider_override_rejected_for_empty_strings_when_flag_true( + config_with_override_enabled, +): + """Even empty strings count as override intent and must be rejected when disabled.""" + query_request = QueryRequest(query="q", model="", provider="") + with pytest.raises(HTTPException) as exc_info: + endpoints.validate_model_provider_override( + query_request, config_with_override_enabled + ) + assert exc_info.value.status_code == 422
596-606: Rename fixture to reflect allowed overrides
The fixture namedconfig_with_override_disabledsetsdisable_query_model_override=False(overrides allowed), so rename both the decorator and function toconfig_with_override_allowed/config_with_override_allowed_fixtureand update its usages accordingly.-@pytest.fixture(name="config_with_override_disabled") -def config_with_override_disabled_fixture(): +@pytest.fixture(name="config_with_override_allowed") +def config_with_override_allowed_fixture():Verified no
disable_query_model_provider_overridereferences remain across the repo.
📜 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 (7)
src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)tests/unit/utils/test_endpoints.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/models/config.py
- src/utils/endpoints.py
- src/app/endpoints/streaming_query.py
- tests/unit/app/endpoints/test_query.py
- tests/unit/app/endpoints/test_streaming_query.py
- src/app/endpoints/query.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/utils/test_endpoints.py (3)
src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/utils/endpoints.py (1)
validate_model_provider_override(87-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (1)
tests/unit/utils/test_endpoints.py (1)
608-618: LGTM: “override disabled” fixture covers the True pathThis fixture correctly sets
disable_query_model_override=Trueto exercise the rejection path. No issues.
WDYT? |
zszabo-rh
force-pushed
the
disable_query_model
branch
2 times, most recently
from
9b3f8ba to
bbb2988
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/generate_openapi_schema.py (1)
43-55: Harden version fallback: add timeout, text mode, and explicit ImportError handlingPrevents hangs when PDM is present but unresponsive and makes the fallback failure obvious.
def read_version_from_pyproject(): - """Read version from pyproject.toml file.""" + """Get project version via PDM, with fallback to version.__version__.""" @@ - try: - completed = subprocess.run( # noqa: S603 - ["pdm", "show", "--version"], # noqa: S607 - capture_output=True, - check=True, - ) - return completed.stdout.decode("utf-8").strip() - except (FileNotFoundError, subprocess.CalledProcessError): - # Fallback: import version from src/version.py - from version import __version__ - - return __version__ + try: + completed = subprocess.run( # noqa: S603 + ["pdm", "show", "--version"], # noqa: S607 + capture_output=True, + check=True, + text=True, + timeout=5, + ) + return completed.stdout.strip() + except (FileNotFoundError, subprocess.CalledProcessError, subprocess.TimeoutExpired): + # Fallback: import version from version.py + try: + from version import __version__ + except Exception as e: # noqa: BLE001 + raise RuntimeError( + "Unable to determine project version via PDM or version module" + ) from e + return __version__docs/openapi.json (1)
1255-1259: Document the new flag’s effect for clientsAdd a short description so API consumers know what happens when this flag is true.
"disable_query_model_override": { "type": "boolean", "title": "Disable Query Model Override", + "description": "If true, server rejects requests that set QueryRequest.model or QueryRequest.provider for /v1/query and /v1/streaming_query.", "default": false },tests/unit/app/endpoints/test_streaming_query.py (1)
1520-1575: Parametrize to cover single-field overrides (model-only/provider-only)Catches edge cases if validation triggers on either field independently.
-@pytest.mark.asyncio -async def test_streaming_query_endpoint_rejects_model_provider_override_when_disabled( - mocker, -): +@pytest.mark.asyncio +@pytest.mark.parametrize("overrides", [{"model": "m"}, {"provider": "p"}, {"model": "m", "provider": "p"}]) +async def test_streaming_query_endpoint_rejects_model_provider_override_when_disabled( + mocker, overrides +): @@ - query_request = QueryRequest(query="What?", model="m", provider="p") + query_request = QueryRequest(query="What?", **overrides)
📜 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 (10)
README.md(1 hunks)docs/openapi.json(1 hunks)scripts/generate_openapi_schema.py(1 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)tests/unit/utils/test_endpoints.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/endpoints/streaming_query.py
- src/models/config.py
- src/utils/endpoints.py
- src/app/endpoints/query.py
- tests/unit/app/endpoints/test_query.py
- tests/unit/utils/test_endpoints.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/app/endpoints/test_streaming_query.py (2)
src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(523-678)
⏰ 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 (1)
tests/unit/app/endpoints/test_streaming_query.py (1)
1520-1575: Nice coverage for the disabled override pathTest clearly asserts 422 and stable message; string is split to satisfy linters. LGTM.
zszabo-rh
force-pushed
the
disable_query_model
branch
from
bbb2988 to
1c7ee47
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_streaming_query.py (1)
1566-1575: Strengthen: assert early-exit by verifying no downstream calls occurMock the Llama client creation and assert it’s not called; this guards against regressions that might perform work after the validation failure.
@@ - with pytest.raises(HTTPException) as exc_info: + # Ensure we don't reach client/model selection on validation failure + mock_get_client = mocker.patch("client.AsyncLlamaStackClientHolder.get_client") + with pytest.raises(HTTPException) as exc_info: await streaming_query_endpoint_handler(request, query_request, auth=MOCK_AUTH) @@ assert exc_info.value.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert exc_info.value.detail["response"] == expected_msg + mock_get_client.assert_not_called()tests/unit/app/endpoints/test_query.py (1)
1552-1563: Also verify no side effects when validation failsAs in the streaming test, assert that no client/model selection happens once validation trips.
@@ - with pytest.raises(HTTPException) as exc_info: + mock_get_client = mocker.patch("client.AsyncLlamaStackClientHolder.get_client") + with pytest.raises(HTTPException) as exc_info: await query_endpoint_handler( request=dummy_request, query_request=query_request, auth=MOCK_AUTH ) @@ assert exc_info.value.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert exc_info.value.detail["response"] == expected_msg + mock_get_client.assert_not_called()
📜 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 (10)
README.md(1 hunks)docs/openapi.json(1 hunks)docs/openapi.md(1 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)tests/unit/utils/test_endpoints.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/openapi.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/utils/endpoints.py
- src/models/config.py
- src/app/endpoints/streaming_query.py
- src/app/endpoints/query.py
- docs/openapi.json
- tests/unit/utils/test_endpoints.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/app/endpoints/test_streaming_query.py (3)
src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(523-678)
tests/unit/app/endpoints/test_query.py (5)
tests/unit/auth/test_jwk_token.py (1)
dummy_request(131-139)tests/unit/app/endpoints/test_conversations.py (1)
dummy_request(30-40)src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/query.py (1)
query_endpoint_handler(155-271)
⏰ 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 (2)
tests/unit/app/endpoints/test_streaming_query.py (1)
1520-1575: LGTM: correct 422 behavior and stable assertion textTest cleanly validates the override-block path and uses a wrapped expected_msg to satisfy linters. Message matches the helper’s contract.
tests/unit/app/endpoints/test_query.py (1)
1512-1563: LGTM: negative test covers disabled model/provider overridesSolid coverage for the query endpoint; exact error text and status are asserted and lines are wrapped to avoid C0301.
Unsure if that is correct, looking at the query endpoint implementation, I don't see where we enforce |
I don't understand. I'm suggesting that this should be an action you can grant to a role rather than a global config for the entire deployment |
I definitely see the pros here, though I also kinda like the simplicity of the current global config–based approach. The key question is whether the benefits of introducing RBAC would truly justify the added complexity.. do we expect strong need for it? My concern is mainly about implementing something that might add overhead without being widely adopted. |
My suggestion has less complexity, since it doesn't require adding any new config. Simply a new enum value to models.Action and an if statement in the query endpoint. Similar to LIST_OTHERS_CONVERSATIONS |
|
I don't mind one way or the other. |
zszabo-rh
force-pushed
the
disable_query_model
branch
from
1c7ee47 to
7b517ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/app/endpoints/streaming_query.py (3)
36-36: Co-locate validate_model_provider_override import with existing utils.endpoints importsKeep related imports grouped to avoid ungrouped-imports warnings and improve readability.
-from utils.endpoints import validate_model_provider_overrideApply within the earlier utils.endpoints import block (Line 32) as shown in the next comment.
32-37: Group utils.endpoints imports (minor style/lint)Fold the separate import into the existing one.
-from utils.endpoints import check_configuration_loaded, get_agent, get_system_prompt +from utils.endpoints import ( + check_configuration_loaded, + get_agent, + get_system_prompt, + validate_model_provider_override, +)
552-554: Harden permission check against missing request.state.authorized_actionsIf middleware doesn’t attach authorized_actions (e.g., refactor or misuse), this will raise AttributeError. Fail closed with a default.
-# Enforce RBAC: optionally disallow overriding model/provider in requests -validate_model_provider_override(query_request, request.state.authorized_actions) +# Enforce RBAC: optionally disallow overriding model/provider in requests +authorized = getattr(request.state, "authorized_actions", frozenset()) +validate_model_provider_override(query_request, authorized)Also, confirm the RBAC pivot matches product intent (PR summary mentions a global config + 422 vs. code uses RBAC + 403). If config-based behavior must remain, we can combine both (config denies outright; otherwise RBAC decides). WDYT?
tests/unit/app/endpoints/test_streaming_query.py (2)
46-47: Move NoopRolesResolver import to module top to fix Pylint C0415Importing inside the test function triggers import-outside-toplevel. Bring it up here.
-from models.config import ModelContextProtocolServer, Action +from models.config import ModelContextProtocolServer, Action +from authorization.resolvers import NoopRolesResolver
1521-1578: Fix docstring (422 → 403) and remove in-function importThe test asserts 403; align the docstring. Also remove the in-function import to satisfy Pylint C0415.
-async def test_streaming_query_endpoint_rejects_model_provider_override_without_permission( +async def test_streaming_query_endpoint_rejects_model_provider_override_without_permission( mocker, ): - """Assert 422 when request includes model/provider without MODEL_OVERRIDE.""" + """Assert 403 when request includes model/provider without MODEL_OVERRIDE.""" @@ - # Patch authorization to exclude MODEL_OVERRIDE from authorized actions - from authorization.resolvers import NoopRolesResolver - + # Patch authorization to exclude MODEL_OVERRIDE from authorized actionstests/unit/utils/test_endpoints.py (2)
11-16: Group models. imports to silence C0412 (ungrouped-imports)*Keep imports from the same package adjacent.
-from models.requests import QueryRequest -from utils import endpoints -from utils.endpoints import get_agent -from models.config import Action +from models.requests import QueryRequest +from models.config import Action +from utils import endpoints +from utils.endpoints import get_agent
604-611: Fix test docstring (422 → 403) to match implementationThe helper raises 403 (Forbidden) when permission is missing.
-def test_validate_model_provider_override_rejected_without_action(): - """Ensure HTTP 422 when request includes model/provider and caller lacks permission.""" +def test_validate_model_provider_override_rejected_without_action(): + """Ensure HTTP 403 when request includes model/provider and caller lacks permission."""src/utils/endpoints.py (2)
9-17: Group models. imports and keep third-party/first-party sections tidy*Address Pylint C0412 by grouping models.* imports together.
-from models.requests import QueryRequest -from models.database.conversations import UserConversation -from app.database import get_session -from configuration import AppConfig -from utils.suid import get_suid -from utils.types import GraniteToolParser -from models.config import Action +from models.requests import QueryRequest +from models.config import Action +from models.database.conversations import UserConversation +from app.database import get_session +from configuration import AppConfig +from utils.suid import get_suid +from utils.types import GraniteToolParser
88-109: Correct docstring: function raises 403, not 422Align docs with behavior to prevent confusion.
def validate_model_provider_override( query_request: QueryRequest, authorized_actions: set[Action] | frozenset[Action] ) -> None: """Validate whether model/provider overrides are allowed by RBAC. - Raises HTTP 422 if the request includes model or provider and the caller - lacks Action.MODEL_OVERRIDE permission. + Raises HTTP 403 if the request includes model or provider and the caller + lacks Action.MODEL_OVERRIDE permission. """
📜 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 (10)
README.md(1 hunks)docs/openapi.json(1 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/authorization/middleware.py(4 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(2 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)tests/unit/utils/test_endpoints.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/app/endpoints/test_query.py
- src/models/config.py
- src/app/endpoints/query.py
- docs/openapi.json
🧰 Additional context used
🧬 Code graph analysis (5)
src/app/endpoints/streaming_query.py (1)
src/utils/endpoints.py (1)
validate_model_provider_override(88-108)
src/authorization/middleware.py (1)
src/models/config.py (1)
Action(276-315)
tests/unit/app/endpoints/test_streaming_query.py (5)
src/models/config.py (3)
config(127-133)ModelContextProtocolServer(156-161)Action(276-315)src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/authorization/resolvers.py (1)
NoopRolesResolver(33-39)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(523-678)
tests/unit/utils/test_endpoints.py (3)
src/models/config.py (2)
config(127-133)Action(276-315)src/models/requests.py (1)
QueryRequest(71-221)src/utils/endpoints.py (1)
validate_model_provider_override(88-108)
src/utils/endpoints.py (2)
src/models/config.py (2)
config(127-133)Action(276-315)src/models/requests.py (1)
QueryRequest(71-221)
🪛 GitHub Actions: Python linter
tests/unit/app/endpoints/test_streaming_query.py
[warning] 1549-1549: C0415: Import outside toplevel (authorization.resolvers.NoopRolesResolver) (import-outside-toplevel)
tests/unit/utils/test_endpoints.py
[warning] 14-14: C0412: Imports from package models are not grouped (ungrouped-imports)
src/utils/endpoints.py
[warning] 16-16: C0412: Imports from package models are not grouped (ungrouped-imports)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (1)
src/authorization/middleware.py (1)
68-71: Good: robust Request discovery and propagation of authorized_actionsScanning both kwargs and positional args to locate Request fixes earlier brittleness; passing args to _perform_authorization_check preserves this behavior across endpoints.
Also applies to: 99-109, 117-117
zszabo-rh
force-pushed
the
disable_query_model
branch
from
7b517ff to
22f10e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/app/endpoints/test_streaming_query.py (1)
37-48: Add NoopRolesResolver import at module scope (complements C0415 fix).
Place the import with the other top-level imports.Apply this diff:
from models.requests import QueryRequest, Attachment -from models.config import ModelContextProtocolServer, Action +from models.config import ModelContextProtocolServer, Action +from authorization.resolvers import NoopRolesResolvertests/unit/app/endpoints/test_query.py (1)
28-31: Add NoopRolesResolver import at module scope (complements C0415 fix).
Keep it with other imports.Apply this diff:
from models.requests import QueryRequest, Attachment from models.config import Action, ModelContextProtocolServer +from authorization.resolvers import NoopRolesResolver
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_streaming_query.py (1)
1548-1557: Optional: remove unused resolver patching.
The endpoint does not call get_authorization_resolvers; setting request.state.authorized_actions is sufficient. Keeping this patch adds noise.tests/unit/app/endpoints/test_query.py (1)
1512-1567: Optional: drop unused resolver patching.
The handler reads request.state.authorized_actions directly; patching get_authorization_resolvers doesn’t influence this unit test and can be removed for clarity.
📜 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 (10)
README.md(1 hunks)docs/openapi.json(1 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/authorization/middleware.py(4 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(2 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)tests/unit/utils/test_endpoints.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/app/endpoints/query.py
- src/authorization/middleware.py
- src/models/config.py
- src/utils/endpoints.py
- docs/openapi.json
- src/app/endpoints/streaming_query.py
- README.md
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/app/endpoints/test_query.py (5)
src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)src/authorization/resolvers.py (7)
NoopRolesResolver(33-39)check_access(124-125)check_access(135-139)check_access(172-189)get_actions(128-129)get_actions(141-144)get_actions(191-203)src/models/config.py (1)
Action(276-315)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/query.py (1)
query_endpoint_handler(155-271)
tests/unit/utils/test_endpoints.py (3)
src/models/config.py (2)
config(127-133)Action(276-315)src/models/requests.py (1)
QueryRequest(71-221)src/utils/endpoints.py (1)
validate_model_provider_override(88-108)
tests/unit/app/endpoints/test_streaming_query.py (4)
src/models/config.py (3)
config(127-133)ModelContextProtocolServer(156-161)Action(276-315)src/authorization/resolvers.py (1)
NoopRolesResolver(33-39)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(523-678)
🪛 GitHub Actions: Python linter
tests/unit/app/endpoints/test_query.py
[error] 1542-1542: C0415: Import outside toplevel (authorization.resolvers.NoopRolesResolver) (import-outside-toplevel)
tests/unit/utils/test_endpoints.py
[error] 14-14: C0412: Imports from package models are not grouped (ungrouped-imports)
tests/unit/app/endpoints/test_streaming_query.py
[error] 1549-1549: C0415: Import outside toplevel (authorization.resolvers.NoopRolesResolver) (import-outside-toplevel)
⏰ 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 (2)
tests/unit/app/endpoints/test_streaming_query.py (1)
46-46: Import addition looks good.
Action is now used in tests; import is appropriate.tests/unit/utils/test_endpoints.py (1)
597-617: RBAC helper tests look correct.
Covers allow, reject, and no-override cases succinctly.
| mocker.patch( | ||
| "authorization.middleware.get_authorization_resolvers", | ||
| return_value=(NoopRolesResolver(), access_resolver), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Ensure the test uses restricted authorized_actions.
dummy_request fixture sets all actions; override it here so validate_model_provider_override takes the 403 path.
Apply this diff:
mocker.patch(
"authorization.middleware.get_authorization_resolvers",
return_value=(NoopRolesResolver(), access_resolver),
)
+ # Simulate middleware: restrict request actions for this test
+ dummy_request.state.authorized_actions = set(Action) - {Action.MODEL_OVERRIDE}📝 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.
| mocker.patch( | |
| "authorization.middleware.get_authorization_resolvers", | |
| return_value=(NoopRolesResolver(), access_resolver), | |
| ) | |
| mocker.patch( | |
| "authorization.middleware.get_authorization_resolvers", | |
| return_value=(NoopRolesResolver(), access_resolver), | |
| ) | |
| # Simulate middleware: restrict request actions for this test | |
| dummy_request.state.authorized_actions = set(Action) - {Action.MODEL_OVERRIDE} |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_query.py around lines 1547 to 1551, the test
currently inherits the dummy_request fixture which grants all actions; to force
validate_model_provider_override to hit the 403 branch override the request's
authorized_actions to a restricted set (e.g., empty set or a set that does not
include the required action for this endpoint). Update the test to either set
dummy_request.authorized_actions = set() (or the minimal disallowed set) before
invoking the endpoint, or use mocker.patch to return a resolver whose
authorized_actions lacks the needed permission, so the call follows the 403
path.
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| } | ||
| ) | ||
|
|
||
| with pytest.raises(HTTPException) as exc_info: |
There was a problem hiding this comment.
Set request.state.authorized_actions in the test to avoid AttributeError and ensure 403 path.
The handler reads request.state.authorized_actions; when calling the endpoint directly, middleware isn’t invoked. Without setting this, the test can fail or miss the intended 403 branch.
Apply this diff after creating the Request:
request = Request(
scope={
"type": "http",
}
)
+ # Simulate middleware-provided actions for this test
+ request.state.authorized_actions = set(Action) - {Action.MODEL_OVERRIDE}📝 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.
| request = Request( | |
| scope={ | |
| "type": "http", | |
| } | |
| ) | |
| with pytest.raises(HTTPException) as exc_info: | |
| request = Request( | |
| scope={ | |
| "type": "http", | |
| } | |
| ) | |
| # Simulate middleware-provided actions for this test | |
| request.state.authorized_actions = set(Action) - {Action.MODEL_OVERRIDE} | |
| with pytest.raises(HTTPException) as exc_info: |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_streaming_query.py around lines 1562-1568, the
test constructs a Request but doesn’t set request.state.authorized_actions so
the handler raises AttributeError or skips the 403 branch; after creating the
Request, set request.state.authorized_actions to the appropriate
empty/unauthorized value used by the app (e.g., an empty set or list) so the
attribute exists and the endpoint takes the 403 path.
22f10e3 to
0ef9f3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit/app/endpoints/test_streaming_query.py (1)
1561-1569: Test will raise AttributeError unless request.state.authorized_actions is set.The handler reads request.state.authorized_actions but middleware isn’t invoked in this unit test. Set it on the Request to exercise the 403 path.
Apply this diff right after constructing the Request:
request = Request( scope={ "type": "http", } ) + + # Simulate middleware-provided authorized actions (missing MODEL_OVERRIDE) + request.state.authorized_actions = set(Action) - {Action.MODEL_OVERRIDE}
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_streaming_query.py (2)
1549-1556: Mocking get_authorization_resolvers is unnecessary here.Since the middleware isn’t executed when calling the handler directly, this patch has no effect. Prefer setting request.state.authorized_actions and drop this mock to reduce noise. If you keep the mock, it still won’t influence the test outcome.
Apply this diff to remove the unused mock (and drop NoopRolesResolver import if unused elsewhere):
- access_resolver = mocker.Mock() - access_resolver.check_access.return_value = True - access_resolver.get_actions.return_value = set(Action) - {Action.MODEL_OVERRIDE} - mocker.patch( - "authorization.middleware.get_authorization_resolvers", - return_value=(NoopRolesResolver(), access_resolver), - )
1570-1576: Optional: make the assertion less brittle.Exact-string matching for detail text couples the test to message wording. Consider asserting status code and a key substring (e.g., "MODEL_OVERRIDE") to preserve intent across wording tweaks.
Example:
- assert exc_info.value.detail["response"] == expected_msg + assert exc_info.value.detail["response"].endswith("fields from your request.") + assert "MODEL_OVERRIDE" in exc_info.value.detail["response"]
📜 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 (10)
README.md(1 hunks)docs/openapi.json(1 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/authorization/middleware.py(4 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(2 hunks)tests/unit/app/endpoints/test_query.py(2 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)tests/unit/utils/test_endpoints.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/unit/utils/test_endpoints.py
- README.md
- docs/openapi.json
- src/models/config.py
- tests/unit/app/endpoints/test_query.py
- src/utils/endpoints.py
- src/authorization/middleware.py
- src/app/endpoints/streaming_query.py
- src/app/endpoints/query.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/app/endpoints/test_streaming_query.py (4)
src/models/config.py (2)
config(127-133)Action(276-315)src/authorization/resolvers.py (1)
NoopRolesResolver(33-39)src/models/requests.py (1)
QueryRequest(71-221)src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(523-678)
⏰ 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 (1)
tests/unit/app/endpoints/test_streaming_query.py (1)
46-48: Top-level imports look good and address prior linter warning.Moving NoopRolesResolver and Action imports to the module scope resolves C0415 and keeps tests clean.
| if "request" in kwargs and isinstance(kwargs["request"], Request): | ||
| req = kwargs["request"] | ||
| else: | ||
| for arg in args: | ||
| if isinstance(arg, Request): | ||
| req = arg | ||
| break |
zszabo-rh
force-pushed
the
disable_query_model
branch
from
0ef9f3b to
3371c97
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/utils/test_endpoints.py (2)
605-611: Assert error detail to prevent message regressions.Check the exception detail contains a stable hint (e.g., the action name).
- assert exc_info.value.status_code == 403 + assert exc_info.value.status_code == 403 + assert "MODEL_OVERRIDE" in str(exc_info.value.detail)
597-617: Option: collapse to a parametrized matrix test.Keeps the same coverage with less duplication.
- def test_validate_model_provider_override_allowed_with_action(): - """Ensure no exception when caller has MODEL_OVERRIDE and request includes model/provider.""" - query_request = QueryRequest(query="q", model="m", provider="p") - authorized_actions = {Action.MODEL_OVERRIDE} - endpoints.validate_model_provider_override(query_request, authorized_actions) - - def test_validate_model_provider_override_rejected_without_action(): - """Ensure HTTP 403 when request includes model/provider and caller lacks permission.""" - query_request = QueryRequest(query="q", model="m", provider="p") - authorized_actions: set[Action] = set() - with pytest.raises(HTTPException) as exc_info: - endpoints.validate_model_provider_override(query_request, authorized_actions) - assert exc_info.value.status_code == 403 - - def test_validate_model_provider_override_no_override_without_action(): - """No exception when request does not include model/provider regardless of permission.""" - query_request = QueryRequest(query="q") - endpoints.validate_model_provider_override(query_request, set()) + @pytest.mark.parametrize( + "has_override, includes_override, expect_forbidden", + [(True, True, False), (False, True, True), (False, False, False)], + ) + def test_validate_model_provider_override_matrix( + has_override: bool, includes_override: bool, expect_forbidden: bool + ): + req = {"query": "q"} + if includes_override: + req |= {"model": "m", "provider": "p"} + query_request = QueryRequest(**req) + authorized = {Action.MODEL_OVERRIDE} if has_override else set() + if expect_forbidden: + with pytest.raises(HTTPException) as exc_info: + endpoints.validate_model_provider_override(query_request, authorized) + assert exc_info.value.status_code == 403 + else: + endpoints.validate_model_provider_override(query_request, authorized)
📜 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 (10)
README.md(1 hunks)docs/openapi.json(1 hunks)src/app/endpoints/query.py(2 hunks)src/app/endpoints/streaming_query.py(2 hunks)src/authorization/middleware.py(4 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(2 hunks)tests/unit/app/endpoints/test_query.py(2 hunks)tests/unit/app/endpoints/test_streaming_query.py(2 hunks)tests/unit/utils/test_endpoints.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/openapi.json
- tests/unit/app/endpoints/test_query.py
- README.md
- src/models/config.py
- src/utils/endpoints.py
- tests/unit/app/endpoints/test_streaming_query.py
- src/app/endpoints/query.py
- src/authorization/middleware.py
- src/app/endpoints/streaming_query.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/utils/test_endpoints.py (1)
src/models/requests.py (1)
QueryRequest(71-221)
⏰ 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 (2)
tests/unit/utils/test_endpoints.py (2)
12-12: Import grouping fix looks good.models.* imports are now grouped; satisfies Pylint C0412.
597-617: Solid RBAC coverage for model/provider overrides.Three cases (allowed, forbidden, no override) are covered and map cleanly to the contract.
4 participants
Description
Use fine‑grained RBAC to control model/provider overrides for
/queryand/streaming_queryendpoints, enforced via a new Action.Requests that include model/provider are now allowed only for callers granted MODEL_OVERRIDE, otherwise they receive HTTP 403.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests