From 8a31d158b8ae1b569ab77bc40bcf1bb9a7edc561 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Wed, 6 May 2026 20:38:27 +0530 Subject: [PATCH 1/3] feat(server): migrate /controls + /control-templates onto auth framework Mirrors #204's bindings migration: replaces require_admin_key and router-level require_api_key with require_operation(CONTROLS_*) on every protected route on /controls and on /control-templates/render. Both routers now mount with the non-validating get_api_key_from_header so the framework owns authentication and authorization, with the extractor attached purely so the generated OpenAPI advertises X-API-Key. GET /controls/schema is intentionally left without a require_operation dependency: it returns a static model schema with no tenant state and routing it through the framework would force the upstream provider to handle a meta-only operation that has no permission semantics. POST /controls/validate and POST /control-templates/render are wired to CONTROLS_CREATE rather than CONTROLS_READ. Both exercise the authoring materialization path and exist to support the create / set- data flow; a caller who cannot create controls has no use for the result. Backwards-incompatible for OSS deployments that previously called these routes with non-admin keys; deployments that want the old behavior can override with HeaderAuthProvider(operation_access={...}). Storage namespace continues to come from get_namespace_key, matching the bindings migration in #204. The unified principal-derived cutover across /controls, /policies, /agents, and /evaluation is a follow-up. --- .../generated/funcs/controls-get-schema.ts | 6 + .../funcs/controls-render-template.ts | 4 + .../generated/funcs/controls-validate-data.ts | 5 + sdks/typescript/src/generated/sdk/controls.ts | 15 + .../endpoints/controls.py | 50 ++- server/src/agent_control_server/main.py | 13 +- server/tests/test_controls_auth.py | 365 ++++++++++++++++++ 7 files changed, 445 insertions(+), 13 deletions(-) create mode 100644 server/tests/test_controls_auth.py diff --git a/sdks/typescript/src/generated/funcs/controls-get-schema.ts b/sdks/typescript/src/generated/funcs/controls-get-schema.ts index ca5442bd..a6ea27cd 100644 --- a/sdks/typescript/src/generated/funcs/controls-get-schema.ts +++ b/sdks/typescript/src/generated/funcs/controls-get-schema.ts @@ -27,6 +27,12 @@ import { Result } from "../types/fp.js"; * * @remarks * Return the canonical JSON schema for ControlDefinition. + * + * Intentionally has no ``require_operation`` dependency: the schema is + * static metadata derived from the model class and exposes no tenant + * state. Routing it through the auth framework would force callers + * (and the upstream authorizer) to handle a meta-only operation that + * has no permission semantics. */ export function controlsGetSchema( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/controls-render-template.ts b/sdks/typescript/src/generated/funcs/controls-render-template.ts index a8998d0e..6f5d4d0e 100644 --- a/sdks/typescript/src/generated/funcs/controls-render-template.ts +++ b/sdks/typescript/src/generated/funcs/controls-render-template.ts @@ -31,6 +31,10 @@ import { Result } from "../types/fp.js"; * * @remarks * Render a template-backed control without persisting it. + * + * Authorized as ``controls.create``: rendering is part of the authoring + * flow (the result feeds the create / update endpoints), so a caller + * who cannot create controls has no use for the materialized output. */ export function controlsRenderTemplate( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/controls-validate-data.ts b/sdks/typescript/src/generated/funcs/controls-validate-data.ts index 70d9a1f0..f1084887 100644 --- a/sdks/typescript/src/generated/funcs/controls-validate-data.ts +++ b/sdks/typescript/src/generated/funcs/controls-validate-data.ts @@ -32,6 +32,11 @@ import { Result } from "../types/fp.js"; * @remarks * Validate control configuration data without saving it. * + * Authorized as ``controls.create`` rather than ``controls.read``: + * validation exercises the full create / update materialization path + * and exists to support authoring, so a caller who cannot create + * controls has no use for the result. + * * Args: * request: Control configuration data to validate * db: Database session (injected) diff --git a/sdks/typescript/src/generated/sdk/controls.ts b/sdks/typescript/src/generated/sdk/controls.ts index ed3cf8db..28edbda3 100644 --- a/sdks/typescript/src/generated/sdk/controls.ts +++ b/sdks/typescript/src/generated/sdk/controls.ts @@ -25,6 +25,10 @@ export class Controls extends ClientSDK { * * @remarks * Render a template-backed control without persisting it. + * + * Authorized as ``controls.create``: rendering is part of the authoring + * flow (the result feeds the create / update endpoints), so a caller + * who cannot create controls has no use for the materialized output. */ async renderTemplate( request: models.RenderControlTemplateRequest, @@ -110,6 +114,12 @@ export class Controls extends ClientSDK { * * @remarks * Return the canonical JSON schema for ControlDefinition. + * + * Intentionally has no ``require_operation`` dependency: the schema is + * static metadata derived from the model class and exposes no tenant + * state. Routing it through the auth framework would force callers + * (and the upstream authorizer) to handle a meta-only operation that + * has no permission semantics. */ async getSchema( options?: RequestOptions, @@ -126,6 +136,11 @@ export class Controls extends ClientSDK { * @remarks * Validate control configuration data without saving it. * + * Authorized as ``controls.create`` rather than ``controls.read``: + * validation exercises the full create / update materialization path + * and exists to support authoring, so a caller who cannot create + * controls has no use for the result. + * * Args: * request: Control configuration data to validate * db: Database session (injected) diff --git a/server/src/agent_control_server/endpoints/controls.py b/server/src/agent_control_server/endpoints/controls.py index 6208652b..a6509a2e 100644 --- a/server/src/agent_control_server/endpoints/controls.py +++ b/server/src/agent_control_server/endpoints/controls.py @@ -33,7 +33,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession -from ..auth import require_admin_key +from ..auth_framework import Operation, Principal, require_operation from ..db import get_async_db from ..errors import ( APIValidationError, @@ -446,8 +446,14 @@ async def _validate_control_definition( async def render_control_template( request: RenderControlTemplateRequest, db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_CREATE)), ) -> RenderControlTemplateResponse: - """Render a template-backed control without persisting it.""" + """Render a template-backed control without persisting it. + + Authorized as ``controls.create``: rendering is part of the authoring + flow (the result feeds the create / update endpoints), so a caller + who cannot create controls has no use for the materialized output. + """ control_def = await _render_and_validate_template_input( TemplateControlInput( template=request.template, @@ -461,13 +467,14 @@ async def render_control_template( @router.put( "", - dependencies=[Depends(require_admin_key)], response_model=CreateControlResponse, summary="Create a new control", response_description="Created control ID", ) async def create_control( - request: CreateControlRequest, db: AsyncSession = Depends(get_async_db) + request: CreateControlRequest, + db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_CREATE)), ) -> CreateControlResponse: """ Create a new control with a unique name. @@ -550,7 +557,14 @@ async def create_control( response_description="JSON schema for ControlDefinition", ) async def get_control_schema() -> GetControlSchemaResponse: - """Return the canonical JSON schema for ControlDefinition.""" + """Return the canonical JSON schema for ControlDefinition. + + Intentionally has no ``require_operation`` dependency: the schema is + static metadata derived from the model class and exposes no tenant + state. Routing it through the auth framework would force callers + (and the upstream authorizer) to handle a meta-only operation that + has no permission semantics. + """ return GetControlSchemaResponse( schema=ControlDefinition.model_json_schema(by_alias=True) ) @@ -563,7 +577,9 @@ async def get_control_schema() -> GetControlSchemaResponse: response_description="Control metadata and configuration", ) async def get_control( - control_id: int, db: AsyncSession = Depends(get_async_db) + control_id: int, + db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_READ)), ) -> GetControlResponse: """ Retrieve a control by ID including its name and configuration data. @@ -600,7 +616,9 @@ async def get_control( response_description="Control data payload", ) async def get_control_data( - control_id: int, db: AsyncSession = Depends(get_async_db) + control_id: int, + db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_READ)), ) -> GetControlDataResponse: """ Retrieve the configuration data for a control. @@ -640,6 +658,7 @@ async def list_control_versions( ), limit: int = Query(_DEFAULT_PAGINATION_LIMIT, ge=1, le=_MAX_PAGINATION_LIMIT), db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_READ)), ) -> ListControlVersionsResponse: """List control versions ordered newest-first using cursor-based pagination.""" page = await ControlService(db).list_versions(control_id, cursor=cursor, limit=limit) @@ -673,6 +692,7 @@ async def get_control_version( control_id: int, version_num: int, db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_READ)), ) -> GetControlVersionResponse: """Return a specific control version, including its raw persisted snapshot.""" version = await ControlService(db).get_version_or_404(control_id, version_num) @@ -687,7 +707,6 @@ async def get_control_version( @router.put( "/{control_id}/data", - dependencies=[Depends(require_admin_key)], response_model=SetControlDataResponse, summary="Update control configuration data", response_description="Success confirmation", @@ -696,6 +715,7 @@ async def set_control_data( control_id: int, request: SetControlDataRequest, db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_UPDATE)), ) -> SetControlDataResponse: """ Update the configuration data for a control. @@ -758,11 +778,18 @@ async def set_control_data( response_description="Validation result", ) async def validate_control_data( - request: ValidateControlDataRequest, db: AsyncSession = Depends(get_async_db) + request: ValidateControlDataRequest, + db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_CREATE)), ) -> ValidateControlDataResponse: """ Validate control configuration data without saving it. + Authorized as ``controls.create`` rather than ``controls.read``: + validation exercises the full create / update materialization path + and exists to support authoring, so a caller who cannot create + controls has no use for the result. + Args: request: Control configuration data to validate db: Database session (injected) @@ -798,6 +825,7 @@ async def list_controls( execution: str | None = Query(None, description="Filter by execution ('server' or 'sdk')"), tag: str | None = Query(None, description="Filter by tag"), db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_READ)), ) -> ListControlsResponse: """ List all controls with optional filtering and cursor-based pagination. @@ -884,7 +912,6 @@ async def list_controls( @router.delete( "/{control_id}", - dependencies=[Depends(require_admin_key)], response_model=DeleteControlResponse, summary="Delete a control", response_description="Deletion confirmation with dissociation info", @@ -897,6 +924,7 @@ async def delete_control( "If false, fail if control is associated with any policy or agent.", ), db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_DELETE)), ) -> DeleteControlResponse: """ Delete a control by ID. @@ -1035,7 +1063,6 @@ async def delete_control( @router.patch( "/{control_id}", - dependencies=[Depends(require_admin_key)], response_model=PatchControlResponse, summary="Update control metadata", response_description="Updated control information", @@ -1044,6 +1071,7 @@ async def patch_control( control_id: int, request: PatchControlRequest, db: AsyncSession = Depends(get_async_db), + _principal: Principal = Depends(require_operation(Operation.CONTROLS_UPDATE)), ) -> PatchControlResponse: """ Update control metadata (name and/or enabled status). diff --git a/server/src/agent_control_server/main.py b/server/src/agent_control_server/main.py index 76416e04..718d7b04 100644 --- a/server/src/agent_control_server/main.py +++ b/server/src/agent_control_server/main.py @@ -273,9 +273,15 @@ async def attach_version_header(request, call_next): # type: ignore[no-untyped- dependencies=[Depends(require_api_key)], ) app.include_router( + # ``/controls`` CRUD goes through the auth framework on each + # endpoint (``require_operation(Operation.CONTROLS_*)``); see the + # ``control_binding_router`` rationale below for the + # ``get_api_key_from_header`` mounting pattern. The single route on + # this router without ``require_operation`` is ``GET /controls/schema``, + # which is intentionally public meta — see its endpoint docstring. control_router, prefix=api_v1_prefix, - dependencies=[Depends(require_api_key)], + dependencies=[Depends(get_api_key_from_header)], ) app.include_router( # The auth framework on each endpoint owns authentication and @@ -300,9 +306,12 @@ async def attach_version_header(request, call_next): # type: ignore[no-untyped- dependencies=[Depends(get_api_key_from_header)], ) app.include_router( + # Control templates: ``/render`` is on the auth framework via + # ``require_operation(Operation.CONTROLS_CREATE)``; same mounting + # pattern as the controls and control-bindings routers. control_template_router, prefix=api_v1_prefix, - dependencies=[Depends(require_api_key)], + dependencies=[Depends(get_api_key_from_header)], ) app.include_router( evaluation_router, diff --git a/server/tests/test_controls_auth.py b/server/tests/test_controls_auth.py new file mode 100644 index 00000000..0357421f --- /dev/null +++ b/server/tests/test_controls_auth.py @@ -0,0 +1,365 @@ +"""HTTP-level coverage for the auth seam on ``/controls`` and +``/control-templates``. + +These tests exercise the wiring of ``require_operation`` on each route +through the default ``HeaderAuthProvider``: read operations require any +valid credential (``CONTROLS_READ`` -> ``AUTHENTICATED``), write +operations require an admin credential +(``CONTROLS_CREATE`` / ``CONTROLS_UPDATE`` / ``CONTROLS_DELETE`` -> +``ADMIN``), and ``GET /controls/schema`` is intentionally outside the +framework so it stays publicly reachable. + +The provider primitives themselves are exercised in +``tests/test_auth_framework.py``; this file focuses on each endpoint +calling the right ``Operation`` so a future change to the operation +mapping is caught at the route level. +""" + +from __future__ import annotations + +import uuid + +import pytest +from fastapi.testclient import TestClient + +from agent_control_server.config import auth_settings + +from .utils import VALID_CONTROL_PAYLOAD + + +_CONTROLS_URL = "/api/v1/controls" +_TEMPLATES_URL = "/api/v1/control-templates" + + +def _create_control(client: TestClient, name: str | None = None) -> int: + payload = { + "name": name or f"control-{uuid.uuid4().hex[:12]}", + "data": VALID_CONTROL_PAYLOAD, + } + resp = client.put(_CONTROLS_URL, json=payload) + assert resp.status_code == 200, resp.text + return int(resp.json()["control_id"]) + + +# --------------------------------------------------------------------------- +# /controls/schema is intentionally public meta — no require_operation. +# --------------------------------------------------------------------------- + + +def test_schema_endpoint_reachable_without_credentials( + unauthenticated_client: TestClient, +) -> None: + # Given: a client that never sends an API key + # When: the schema endpoint is fetched + resp = unauthenticated_client.get(f"{_CONTROLS_URL}/schema") + + # Then: the canonical ControlDefinition schema is returned + assert resp.status_code == 200, resp.text + body = resp.json() + assert "schema" in body + assert isinstance(body["schema"], dict) + + +def test_schema_endpoint_reachable_with_admin_key(client: TestClient) -> None: + # Given: an admin client + # When: the schema endpoint is fetched + resp = client.get(f"{_CONTROLS_URL}/schema") + + # Then: the schema is returned (header is ignored, route is public) + assert resp.status_code == 200, resp.text + + +def test_schema_endpoint_reachable_with_non_admin_key( + non_admin_client: TestClient, +) -> None: + # Given: a non-admin client + # When: the schema endpoint is fetched + resp = non_admin_client.get(f"{_CONTROLS_URL}/schema") + + # Then: the schema is returned + assert resp.status_code == 200, resp.text + + +# --------------------------------------------------------------------------- +# CONTROLS_READ operations: AUTHENTICATED suffices. +# --------------------------------------------------------------------------- + + +def test_non_admin_can_list_controls( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control + _create_control(client) + + # When: a non-admin lists controls + resp = non_admin_client.get(_CONTROLS_URL) + + # Then: the list is returned + assert resp.status_code == 200, resp.text + + +def test_non_admin_can_get_control( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control + control_id = _create_control(client) + + # When: a non-admin reads it + resp = non_admin_client.get(f"{_CONTROLS_URL}/{control_id}") + + # Then: the control is returned + assert resp.status_code == 200, resp.text + + +def test_non_admin_can_get_control_data( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control + control_id = _create_control(client) + + # When: a non-admin reads its data + resp = non_admin_client.get(f"{_CONTROLS_URL}/{control_id}/data") + + # Then: the data is returned + assert resp.status_code == 200, resp.text + + +def test_non_admin_can_list_versions( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control with at least one version (creation) + control_id = _create_control(client) + + # When: a non-admin lists versions + resp = non_admin_client.get(f"{_CONTROLS_URL}/{control_id}/versions") + + # Then: the version list is returned + assert resp.status_code == 200, resp.text + + +def test_non_admin_can_get_specific_version( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control (version 1 = "created") + control_id = _create_control(client) + + # When: a non-admin reads version 1 + resp = non_admin_client.get(f"{_CONTROLS_URL}/{control_id}/versions/1") + + # Then: the version snapshot is returned + assert resp.status_code == 200, resp.text + + +# --------------------------------------------------------------------------- +# CONTROLS_CREATE / UPDATE / DELETE: ADMIN required. +# --------------------------------------------------------------------------- + + +def test_non_admin_cannot_create_control(non_admin_client: TestClient) -> None: + # When: a non-admin attempts to create + resp = non_admin_client.put( + _CONTROLS_URL, + json={ + "name": f"control-{uuid.uuid4().hex[:12]}", + "data": VALID_CONTROL_PAYLOAD, + }, + ) + + # Then: the request is forbidden by the auth seam + assert resp.status_code == 403, resp.text + + +def test_non_admin_cannot_set_control_data( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control + control_id = _create_control(client) + + # When: a non-admin attempts to replace its data + resp = non_admin_client.put( + f"{_CONTROLS_URL}/{control_id}/data", + json={"data": VALID_CONTROL_PAYLOAD}, + ) + + # Then: the request is forbidden + assert resp.status_code == 403, resp.text + + +def test_non_admin_cannot_patch_control( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control + control_id = _create_control(client) + + # When: a non-admin attempts to rename it + resp = non_admin_client.patch( + f"{_CONTROLS_URL}/{control_id}", + json={"name": "renamed"}, + ) + + # Then: the request is forbidden + assert resp.status_code == 403, resp.text + + +def test_non_admin_cannot_delete_control( + non_admin_client: TestClient, client: TestClient +) -> None: + # Given: an existing control + control_id = _create_control(client) + + # When: a non-admin attempts to delete it + resp = non_admin_client.delete(f"{_CONTROLS_URL}/{control_id}") + + # Then: the request is forbidden + assert resp.status_code == 403, resp.text + + +def test_non_admin_cannot_validate_control_data( + non_admin_client: TestClient, +) -> None: + """``/controls/validate`` is wired to ``CONTROLS_CREATE`` rather than + ``CONTROLS_READ`` because validation exercises the create / update + materialization path; a caller who cannot create has no use for the + result. This pins that decision so it can't drift to ``READ`` + accidentally. + """ + # When: a non-admin attempts to validate a draft payload + resp = non_admin_client.post( + f"{_CONTROLS_URL}/validate", + json={"data": VALID_CONTROL_PAYLOAD}, + ) + + # Then: validation is admin-only + assert resp.status_code == 403, resp.text + + +def test_non_admin_cannot_render_template(non_admin_client: TestClient) -> None: + """``/control-templates/render`` is wired to ``CONTROLS_CREATE`` for + the same reason as ``/validate``: rendering is part of the authoring + flow. The 422 path is not exercised here — only the auth gate is + asserted, so the request shape need not validate. + """ + # When: a non-admin attempts to render a template + resp = non_admin_client.post( + f"{_TEMPLATES_URL}/render", + json={"template": {}, "template_values": {}}, + ) + + # Then: rendering is admin-only — the auth gate fires before body + # validation reaches the materialization path + assert resp.status_code == 403, resp.text + + +# --------------------------------------------------------------------------- +# Unauthenticated requests are rejected on every framework-protected route. +# --------------------------------------------------------------------------- + + +def test_unauthenticated_cannot_list_controls( + unauthenticated_client: TestClient, +) -> None: + # When: a client without credentials lists controls + resp = unauthenticated_client.get(_CONTROLS_URL) + + # Then: the request is rejected + assert resp.status_code == 401, resp.text + + +def test_unauthenticated_cannot_create_control( + unauthenticated_client: TestClient, +) -> None: + # When: a client without credentials attempts to create + resp = unauthenticated_client.put( + _CONTROLS_URL, + json={ + "name": f"control-{uuid.uuid4().hex[:12]}", + "data": VALID_CONTROL_PAYLOAD, + }, + ) + + # Then: the request is rejected + assert resp.status_code == 401, resp.text + + +def test_unauthenticated_cannot_validate( + unauthenticated_client: TestClient, +) -> None: + # When: a client without credentials attempts to validate + resp = unauthenticated_client.post( + f"{_CONTROLS_URL}/validate", + json={"data": VALID_CONTROL_PAYLOAD}, + ) + + # Then: the request is rejected + assert resp.status_code == 401, resp.text + + +def test_unauthenticated_cannot_render_template( + unauthenticated_client: TestClient, +) -> None: + # When: a client without credentials attempts to render + resp = unauthenticated_client.post( + f"{_TEMPLATES_URL}/render", + json={"template": {}, "template_values": {}}, + ) + + # Then: the request is rejected + assert resp.status_code == 401, resp.text + + +# --------------------------------------------------------------------------- +# No-auth deployment mode: api_key_enabled=False bypasses every gate. +# --------------------------------------------------------------------------- + + +def test_no_auth_mode_allows_writes_without_credentials( + unauthenticated_client: TestClient, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """When ``api_key_enabled`` is False, the ``HeaderAuthProvider`` + short-circuits to a non-admin ``Principal`` for every operation, + including admin-level writes. This pins the "no auth" deployment + path so a future refactor can't silently start enforcing. + """ + # Given: api_key_enabled is False (single-tenant OSS dev mode) + monkeypatch.setattr(auth_settings, "api_key_enabled", False) + + # When: an unauthenticated client creates a control + resp = unauthenticated_client.put( + _CONTROLS_URL, + json={ + "name": f"control-{uuid.uuid4().hex[:12]}", + "data": VALID_CONTROL_PAYLOAD, + }, + ) + + # Then: the create succeeds because auth is disabled at the provider + assert resp.status_code == 200, resp.text + assert "control_id" in resp.json() + + +# --------------------------------------------------------------------------- +# Project-scoped API key deny — pending header forwarding follow-up. +# --------------------------------------------------------------------------- + + +@pytest.mark.skip( + reason=( + "Requires the upstream auth provider to forward an additional " + "configurable credential header. The default forward set is " + "fixed to (X-API-Key, Authorization, Cookie); deployments that " + "use a different credential header can't surface a " + "project-scoped credential to upstream until that becomes " + "configurable. Re-enable when the follow-up PR adds an " + "operator-configurable extra forward list." + ) +) +def test_project_scoped_credential_denied_on_org_scoped_controls() -> None: + """Stub for the deny-test promised by the upstream provider's + response contract: a project-scoped credential calling an + org-scoped operation (``controls.*``) should resolve to a 403 from + the upstream. The end-to-end path is unreachable today because the + provider's credential-forward list is not configurable; tracked as + the next follow-up after this PR. + """ + pytest.fail("test stub — see skip reason") From 3a5b7e47d87fb9b7946c96711bfec264c13cef90 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Wed, 6 May 2026 21:19:03 +0530 Subject: [PATCH 2/3] fix(server): keep public docstrings API-level on migrated controls routes Move auth-framework rationale on /controls/schema, /controls/validate, and /control-templates/render from route docstrings into normal code comments. The docstrings flow into the generated TypeScript SDK as public API documentation, so internal terminology like ``require_operation`` and "upstream authorizer" should not appear there. Function-level comments preserve the rationale for readers of the source. Also remove the skipped placeholder test for the project-scoped credential deny scenario; that scenario depends on a deployment-side provider configuration that is not part of the OSS server, so tracking it as a permanent skipped test in this repo was the wrong home for it. Regenerate the TypeScript SDK to drop the leaked rationale lines. --- .../generated/funcs/controls-get-schema.ts | 6 -- .../funcs/controls-render-template.ts | 4 -- .../generated/funcs/controls-validate-data.ts | 5 -- sdks/typescript/src/generated/sdk/controls.ts | 15 ----- .../endpoints/controls.py | 24 ++----- server/src/agent_control_server/main.py | 11 +--- server/tests/test_controls_auth.py | 63 ++----------------- 7 files changed, 13 insertions(+), 115 deletions(-) diff --git a/sdks/typescript/src/generated/funcs/controls-get-schema.ts b/sdks/typescript/src/generated/funcs/controls-get-schema.ts index a6ea27cd..ca5442bd 100644 --- a/sdks/typescript/src/generated/funcs/controls-get-schema.ts +++ b/sdks/typescript/src/generated/funcs/controls-get-schema.ts @@ -27,12 +27,6 @@ import { Result } from "../types/fp.js"; * * @remarks * Return the canonical JSON schema for ControlDefinition. - * - * Intentionally has no ``require_operation`` dependency: the schema is - * static metadata derived from the model class and exposes no tenant - * state. Routing it through the auth framework would force callers - * (and the upstream authorizer) to handle a meta-only operation that - * has no permission semantics. */ export function controlsGetSchema( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/controls-render-template.ts b/sdks/typescript/src/generated/funcs/controls-render-template.ts index 6f5d4d0e..a8998d0e 100644 --- a/sdks/typescript/src/generated/funcs/controls-render-template.ts +++ b/sdks/typescript/src/generated/funcs/controls-render-template.ts @@ -31,10 +31,6 @@ import { Result } from "../types/fp.js"; * * @remarks * Render a template-backed control without persisting it. - * - * Authorized as ``controls.create``: rendering is part of the authoring - * flow (the result feeds the create / update endpoints), so a caller - * who cannot create controls has no use for the materialized output. */ export function controlsRenderTemplate( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/controls-validate-data.ts b/sdks/typescript/src/generated/funcs/controls-validate-data.ts index f1084887..70d9a1f0 100644 --- a/sdks/typescript/src/generated/funcs/controls-validate-data.ts +++ b/sdks/typescript/src/generated/funcs/controls-validate-data.ts @@ -32,11 +32,6 @@ import { Result } from "../types/fp.js"; * @remarks * Validate control configuration data without saving it. * - * Authorized as ``controls.create`` rather than ``controls.read``: - * validation exercises the full create / update materialization path - * and exists to support authoring, so a caller who cannot create - * controls has no use for the result. - * * Args: * request: Control configuration data to validate * db: Database session (injected) diff --git a/sdks/typescript/src/generated/sdk/controls.ts b/sdks/typescript/src/generated/sdk/controls.ts index 28edbda3..ed3cf8db 100644 --- a/sdks/typescript/src/generated/sdk/controls.ts +++ b/sdks/typescript/src/generated/sdk/controls.ts @@ -25,10 +25,6 @@ export class Controls extends ClientSDK { * * @remarks * Render a template-backed control without persisting it. - * - * Authorized as ``controls.create``: rendering is part of the authoring - * flow (the result feeds the create / update endpoints), so a caller - * who cannot create controls has no use for the materialized output. */ async renderTemplate( request: models.RenderControlTemplateRequest, @@ -114,12 +110,6 @@ export class Controls extends ClientSDK { * * @remarks * Return the canonical JSON schema for ControlDefinition. - * - * Intentionally has no ``require_operation`` dependency: the schema is - * static metadata derived from the model class and exposes no tenant - * state. Routing it through the auth framework would force callers - * (and the upstream authorizer) to handle a meta-only operation that - * has no permission semantics. */ async getSchema( options?: RequestOptions, @@ -136,11 +126,6 @@ export class Controls extends ClientSDK { * @remarks * Validate control configuration data without saving it. * - * Authorized as ``controls.create`` rather than ``controls.read``: - * validation exercises the full create / update materialization path - * and exists to support authoring, so a caller who cannot create - * controls has no use for the result. - * * Args: * request: Control configuration data to validate * db: Database session (injected) diff --git a/server/src/agent_control_server/endpoints/controls.py b/server/src/agent_control_server/endpoints/controls.py index a6509a2e..fcb7cb18 100644 --- a/server/src/agent_control_server/endpoints/controls.py +++ b/server/src/agent_control_server/endpoints/controls.py @@ -443,17 +443,13 @@ async def _validate_control_definition( summary="Render a control template preview", response_description="Rendered control preview", ) +# Rendering is part of the authoring flow, so require create access. async def render_control_template( request: RenderControlTemplateRequest, db: AsyncSession = Depends(get_async_db), _principal: Principal = Depends(require_operation(Operation.CONTROLS_CREATE)), ) -> RenderControlTemplateResponse: - """Render a template-backed control without persisting it. - - Authorized as ``controls.create``: rendering is part of the authoring - flow (the result feeds the create / update endpoints), so a caller - who cannot create controls has no use for the materialized output. - """ + """Render a template-backed control without persisting it.""" control_def = await _render_and_validate_template_input( TemplateControlInput( template=request.template, @@ -556,15 +552,9 @@ async def create_control( summary="Get control definition JSON schema", response_description="JSON schema for ControlDefinition", ) +# Public schema metadata: no tenant state, no auth operation. async def get_control_schema() -> GetControlSchemaResponse: - """Return the canonical JSON schema for ControlDefinition. - - Intentionally has no ``require_operation`` dependency: the schema is - static metadata derived from the model class and exposes no tenant - state. Routing it through the auth framework would force callers - (and the upstream authorizer) to handle a meta-only operation that - has no permission semantics. - """ + """Return the canonical JSON schema for ControlDefinition.""" return GetControlSchemaResponse( schema=ControlDefinition.model_json_schema(by_alias=True) ) @@ -777,6 +767,7 @@ async def set_control_data( summary="Validate control configuration", response_description="Validation result", ) +# Validation uses the authoring path, so require create access. async def validate_control_data( request: ValidateControlDataRequest, db: AsyncSession = Depends(get_async_db), @@ -785,11 +776,6 @@ async def validate_control_data( """ Validate control configuration data without saving it. - Authorized as ``controls.create`` rather than ``controls.read``: - validation exercises the full create / update materialization path - and exists to support authoring, so a caller who cannot create - controls has no use for the result. - Args: request: Control configuration data to validate db: Database session (injected) diff --git a/server/src/agent_control_server/main.py b/server/src/agent_control_server/main.py index 718d7b04..bc1bf04b 100644 --- a/server/src/agent_control_server/main.py +++ b/server/src/agent_control_server/main.py @@ -273,12 +273,7 @@ async def attach_version_header(request, call_next): # type: ignore[no-untyped- dependencies=[Depends(require_api_key)], ) app.include_router( - # ``/controls`` CRUD goes through the auth framework on each - # endpoint (``require_operation(Operation.CONTROLS_*)``); see the - # ``control_binding_router`` rationale below for the - # ``get_api_key_from_header`` mounting pattern. The single route on - # this router without ``require_operation`` is ``GET /controls/schema``, - # which is intentionally public meta — see its endpoint docstring. + # Endpoint dependencies handle auth; this advertises X-API-Key. control_router, prefix=api_v1_prefix, dependencies=[Depends(get_api_key_from_header)], @@ -306,9 +301,7 @@ async def attach_version_header(request, call_next): # type: ignore[no-untyped- dependencies=[Depends(get_api_key_from_header)], ) app.include_router( - # Control templates: ``/render`` is on the auth framework via - # ``require_operation(Operation.CONTROLS_CREATE)``; same mounting - # pattern as the controls and control-bindings routers. + # Endpoint dependencies handle auth; this advertises X-API-Key. control_template_router, prefix=api_v1_prefix, dependencies=[Depends(get_api_key_from_header)], diff --git a/server/tests/test_controls_auth.py b/server/tests/test_controls_auth.py index 0357421f..1a2af21f 100644 --- a/server/tests/test_controls_auth.py +++ b/server/tests/test_controls_auth.py @@ -1,19 +1,4 @@ -"""HTTP-level coverage for the auth seam on ``/controls`` and -``/control-templates``. - -These tests exercise the wiring of ``require_operation`` on each route -through the default ``HeaderAuthProvider``: read operations require any -valid credential (``CONTROLS_READ`` -> ``AUTHENTICATED``), write -operations require an admin credential -(``CONTROLS_CREATE`` / ``CONTROLS_UPDATE`` / ``CONTROLS_DELETE`` -> -``ADMIN``), and ``GET /controls/schema`` is intentionally outside the -framework so it stays publicly reachable. - -The provider primitives themselves are exercised in -``tests/test_auth_framework.py``; this file focuses on each endpoint -calling the right ``Operation`` so a future change to the operation -mapping is caught at the route level. -""" +"""HTTP-level auth coverage for ``/controls`` and ``/control-templates``.""" from __future__ import annotations @@ -42,7 +27,7 @@ def _create_control(client: TestClient, name: str | None = None) -> int: # --------------------------------------------------------------------------- -# /controls/schema is intentionally public meta — no require_operation. +# /controls/schema is intentionally public metadata. # --------------------------------------------------------------------------- @@ -165,7 +150,7 @@ def test_non_admin_cannot_create_control(non_admin_client: TestClient) -> None: }, ) - # Then: the request is forbidden by the auth seam + # Then: the request is forbidden assert resp.status_code == 403, resp.text @@ -217,12 +202,7 @@ def test_non_admin_cannot_delete_control( def test_non_admin_cannot_validate_control_data( non_admin_client: TestClient, ) -> None: - """``/controls/validate`` is wired to ``CONTROLS_CREATE`` rather than - ``CONTROLS_READ`` because validation exercises the create / update - materialization path; a caller who cannot create has no use for the - result. This pins that decision so it can't drift to ``READ`` - accidentally. - """ + """``/controls/validate`` requires ``CONTROLS_CREATE``.""" # When: a non-admin attempts to validate a draft payload resp = non_admin_client.post( f"{_CONTROLS_URL}/validate", @@ -234,19 +214,14 @@ def test_non_admin_cannot_validate_control_data( def test_non_admin_cannot_render_template(non_admin_client: TestClient) -> None: - """``/control-templates/render`` is wired to ``CONTROLS_CREATE`` for - the same reason as ``/validate``: rendering is part of the authoring - flow. The 422 path is not exercised here — only the auth gate is - asserted, so the request shape need not validate. - """ + """``/control-templates/render`` requires ``CONTROLS_CREATE``.""" # When: a non-admin attempts to render a template resp = non_admin_client.post( f"{_TEMPLATES_URL}/render", json={"template": {}, "template_values": {}}, ) - # Then: rendering is admin-only — the auth gate fires before body - # validation reaches the materialization path + # Then: rendering is admin-only assert resp.status_code == 403, resp.text @@ -337,29 +312,3 @@ def test_no_auth_mode_allows_writes_without_credentials( assert resp.status_code == 200, resp.text assert "control_id" in resp.json() - -# --------------------------------------------------------------------------- -# Project-scoped API key deny — pending header forwarding follow-up. -# --------------------------------------------------------------------------- - - -@pytest.mark.skip( - reason=( - "Requires the upstream auth provider to forward an additional " - "configurable credential header. The default forward set is " - "fixed to (X-API-Key, Authorization, Cookie); deployments that " - "use a different credential header can't surface a " - "project-scoped credential to upstream until that becomes " - "configurable. Re-enable when the follow-up PR adds an " - "operator-configurable extra forward list." - ) -) -def test_project_scoped_credential_denied_on_org_scoped_controls() -> None: - """Stub for the deny-test promised by the upstream provider's - response contract: a project-scoped credential calling an - org-scoped operation (``controls.*``) should resolve to a 403 from - the upstream. The end-to-end path is unreachable today because the - provider's credential-forward list is not configurable; tracked as - the next follow-up after this PR. - """ - pytest.fail("test stub — see skip reason") From 8312b99bd5bd8d242e385924641659f9d0ef2dbc Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 8 May 2026 22:23:50 +0530 Subject: [PATCH 3/3] docs(server): keep binding auth comments generic --- .../endpoints/control_bindings.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/server/src/agent_control_server/endpoints/control_bindings.py b/server/src/agent_control_server/endpoints/control_bindings.py index 18cb75b4..92798ae1 100644 --- a/server/src/agent_control_server/endpoints/control_bindings.py +++ b/server/src/agent_control_server/endpoints/control_bindings.py @@ -36,13 +36,11 @@ async def _binding_body_context(request: Request) -> dict[str, Any]: - """Surface ``(target_type, target_id)`` to the authorizer's context. + """Surface ``(target_type, target_id)`` to the authorization context. The body-bearing binding endpoints carry the target identifiers in - the request payload. Upstream authorizers that resolve the target's - owning project (e.g., Galileo's ``check_management_access``) need - those identifiers to make a project-level decision; without them the - upstream returns 400. + the request payload. Authorization providers can use those + identifiers when a request needs target-scoped access checks. FastAPI caches the parsed body, so the endpoint's own Pydantic request model still binds normally. @@ -60,13 +58,12 @@ async def _binding_body_context(request: Request) -> dict[str, Any]: async def _binding_list_context(request: Request) -> dict[str, Any]: - """Surface optional target query parameters to the authorizer. + """Surface optional target query parameters to authorization context. When the GET list endpoint is called with ``target_type`` and ``target_id`` query params, the request is target-scoped and the - upstream needs the identifiers to make a project-level decision. - When neither is present the request is namespace-wide and forwards - no target context (upstream may then reject if it requires one). + request context includes those identifiers. When neither is present + the request is namespace-wide and forwards no target context. """ target_type = request.query_params.get("target_type") target_id = request.query_params.get("target_id")