From b7c5686b6380088f4243497513e191d927562f28 Mon Sep 17 00:00:00 2001 From: Joshua Jeon Date: Wed, 30 Sep 2026 08:55:57 -0700 Subject: [PATCH 1/4] fix(server): authorize binding IDs with stored targets [SAO-17527] Resolve caller identity before the namespace-scoped binding lookup, then authorize against the stored target. Preserve 401 responses and mask missing or target-denied bindings as 404. Add provider and endpoint regression coverage and refresh generated TypeScript route descriptions. --- .../funcs/control-bindings-delete.ts | 8 +- .../generated/funcs/control-bindings-get.ts | 11 +- .../funcs/control-bindings-update.ts | 8 +- .../src/generated/sdk/control-bindings.ts | 27 +- .../auth_framework/config.py | 6 +- .../auth_framework/core.py | 13 +- .../auth_framework/providers/header.py | 4 + .../auth_framework/providers/http_upstream.py | 78 +++- .../auth_framework/providers/local_jwt.py | 38 +- .../endpoints/control_bindings.py | 74 ++-- .../services/control_bindings.py | 33 +- server/tests/test_auth_framework.py | 161 ++++++++ .../tests/test_control_bindings_endpoints.py | 379 +++++++++++++++++- 13 files changed, 743 insertions(+), 97 deletions(-) diff --git a/sdks/typescript/src/generated/funcs/control-bindings-delete.ts b/sdks/typescript/src/generated/funcs/control-bindings-delete.ts index 9872a9b4..a8ff3f41 100644 --- a/sdks/typescript/src/generated/funcs/control-bindings-delete.ts +++ b/sdks/typescript/src/generated/funcs/control-bindings-delete.ts @@ -28,15 +28,13 @@ import { APICall, APIPromise } from "../types/async.js"; import { Result } from "../types/fp.js"; /** - * Delete a control binding (namespace-wide) + * Delete a control binding * * @remarks * Delete a control binding by surrogate ID. * - * See the GET-by-id docstring for the authorization scope: this route - * is namespace-wide because the target identifiers are not available - * before the binding is loaded. Use ``POST /by-key:delete`` for - * target-scoped detach that includes the target in the request context. + * Authorization uses the binding's stored target identifiers. The deletion + * remains scoped to the namespace resolved by the authorizer. */ export function controlBindingsDelete( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/control-bindings-get.ts b/sdks/typescript/src/generated/funcs/control-bindings-get.ts index 88b4e419..c83512aa 100644 --- a/sdks/typescript/src/generated/funcs/control-bindings-get.ts +++ b/sdks/typescript/src/generated/funcs/control-bindings-get.ts @@ -28,17 +28,14 @@ import { APICall, APIPromise } from "../types/async.js"; import { Result } from "../types/fp.js"; /** - * Get a control binding (namespace-wide) + * Get a control binding * * @remarks * Read a single control binding by surrogate ID. * - * Authorization is namespace-wide: the binding's target identifiers - * are not available until after the row is loaded. - * Callers whose authorization model requires per-target permissions - * should use the natural-key endpoints (``PUT /by-key``, - * ``POST /by-key:delete``) and the target-filtered list endpoint, all - * of which include ``(target_type, target_id)`` in the request context. + * Authorization uses the binding's stored target identifiers. After + * authorization succeeds, the row is loaded again using the namespace + * resolved by the authorizer before any binding data is returned. */ export function controlBindingsGet( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/control-bindings-update.ts b/sdks/typescript/src/generated/funcs/control-bindings-update.ts index b94520a2..c1bf2062 100644 --- a/sdks/typescript/src/generated/funcs/control-bindings-update.ts +++ b/sdks/typescript/src/generated/funcs/control-bindings-update.ts @@ -28,15 +28,13 @@ import { APICall, APIPromise } from "../types/async.js"; import { Result } from "../types/fp.js"; /** - * Update a control binding (namespace-wide) + * Update a control binding * * @remarks * Update the ``enabled`` flag on a control binding. * - * See the GET-by-id docstring for the authorization scope: this route - * is namespace-wide because the target identifiers are not available - * before the binding is loaded. Use ``PUT /by-key`` for target-scoped - * upserts that include the target in the request context. + * Authorization uses the binding's stored target identifiers. The mutation + * remains scoped to the namespace resolved by the authorizer. */ export function controlBindingsUpdate( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/sdk/control-bindings.ts b/sdks/typescript/src/generated/sdk/control-bindings.ts index a8708986..1f2ac5b7 100644 --- a/sdks/typescript/src/generated/sdk/control-bindings.ts +++ b/sdks/typescript/src/generated/sdk/control-bindings.ts @@ -118,15 +118,13 @@ export class ControlBindings extends ClientSDK { } /** - * Delete a control binding (namespace-wide) + * Delete a control binding * * @remarks * Delete a control binding by surrogate ID. * - * See the GET-by-id docstring for the authorization scope: this route - * is namespace-wide because the target identifiers are not available - * before the binding is loaded. Use ``POST /by-key:delete`` for - * target-scoped detach that includes the target in the request context. + * Authorization uses the binding's stored target identifiers. The deletion + * remains scoped to the namespace resolved by the authorizer. */ async delete( request: @@ -141,17 +139,14 @@ export class ControlBindings extends ClientSDK { } /** - * Get a control binding (namespace-wide) + * Get a control binding * * @remarks * Read a single control binding by surrogate ID. * - * Authorization is namespace-wide: the binding's target identifiers - * are not available until after the row is loaded. - * Callers whose authorization model requires per-target permissions - * should use the natural-key endpoints (``PUT /by-key``, - * ``POST /by-key:delete``) and the target-filtered list endpoint, all - * of which include ``(target_type, target_id)`` in the request context. + * Authorization uses the binding's stored target identifiers. After + * authorization succeeds, the row is loaded again using the namespace + * resolved by the authorizer before any binding data is returned. */ async get( request: @@ -166,15 +161,13 @@ export class ControlBindings extends ClientSDK { } /** - * Update a control binding (namespace-wide) + * Update a control binding * * @remarks * Update the ``enabled`` flag on a control binding. * - * See the GET-by-id docstring for the authorization scope: this route - * is namespace-wide because the target identifiers are not available - * before the binding is loaded. Use ``PUT /by-key`` for target-scoped - * upserts that include the target in the request context. + * Authorization uses the binding's stored target identifiers. The mutation + * remains scoped to the namespace resolved by the authorizer. */ async update( request: diff --git a/server/src/agent_control_server/auth_framework/config.py b/server/src/agent_control_server/auth_framework/config.py index 7830766b..6fc4c6ff 100644 --- a/server/src/agent_control_server/auth_framework/config.py +++ b/server/src/agent_control_server/auth_framework/config.py @@ -53,6 +53,7 @@ # Default flow. _MODE_ENV = "AGENT_CONTROL_AUTH_MODE" _UPSTREAM_URL_ENV = "AGENT_CONTROL_AUTH_UPSTREAM_URL" +_UPSTREAM_IDENTITY_URL_ENV = "AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL" _UPSTREAM_TIMEOUT_ENV = "AGENT_CONTROL_AUTH_UPSTREAM_TIMEOUT_SECONDS" _UPSTREAM_TOKEN_ENV = "AGENT_CONTROL_AUTH_UPSTREAM_SERVICE_TOKEN" _UPSTREAM_TOKEN_HEADER_ENV = "AGENT_CONTROL_AUTH_UPSTREAM_SERVICE_TOKEN_HEADER" @@ -110,7 +111,9 @@ def configure_auth_from_env() -> None: is unset, startup selects ``api_key`` only if local API-key validation is enabled; otherwise it selects ``none``. - ``AGENT_CONTROL_AUTH_MODE=http_upstream``: :class:`HttpUpstreamAuthProvider` - pointed at ``AGENT_CONTROL_AUTH_UPSTREAM_URL``. + pointed at ``AGENT_CONTROL_AUTH_UPSTREAM_URL``. By-ID binding routes also + use ``AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL`` when the identity URL + cannot be derived from Orbit's management authorization URL. Runtime flow: @@ -245,6 +248,7 @@ def _build_default_provider() -> RequestAuthorizer: try: upstream_config = HttpUpstreamConfig( url=url, + identity_url=(os.environ.get(_UPSTREAM_IDENTITY_URL_ENV) or "").strip() or None, timeout_seconds=timeout, service_token=token, service_token_header=token_header, diff --git a/server/src/agent_control_server/auth_framework/core.py b/server/src/agent_control_server/auth_framework/core.py index 011c62de..028987ca 100644 --- a/server/src/agent_control_server/auth_framework/core.py +++ b/server/src/agent_control_server/auth_framework/core.py @@ -26,7 +26,7 @@ from dataclasses import dataclass from datetime import datetime from enum import StrEnum -from typing import Any, Protocol +from typing import Any, Protocol, runtime_checkable from fastapi import Request @@ -115,6 +115,17 @@ async def authorize( ) -> Principal: ... +@runtime_checkable +class IdentityResolver(Protocol): + """Optional credential and namespace lookup for target-bound authorizers. + + It does not grant resource access. The route still calls ``authorize`` + with the stored target after the namespace-scoped lookup. + """ + + async def resolve_identity(self, request: Request, operation: Operation) -> Principal: ... + + _default_authorizer: RequestAuthorizer | None = None _operation_authorizers: dict[Operation, RequestAuthorizer] = {} diff --git a/server/src/agent_control_server/auth_framework/providers/header.py b/server/src/agent_control_server/auth_framework/providers/header.py index 2d917d91..5d6c49ad 100644 --- a/server/src/agent_control_server/auth_framework/providers/header.py +++ b/server/src/agent_control_server/auth_framework/providers/header.py @@ -75,6 +75,10 @@ def __init__( ) self._default_namespace_key = default_namespace_key + async def resolve_identity(self, request: Request, operation: Operation) -> Principal: + """Apply the local operation gate before a namespace-scoped lookup.""" + return await self.authorize(request, operation) + async def authorize( self, request: Request, diff --git a/server/src/agent_control_server/auth_framework/providers/http_upstream.py b/server/src/agent_control_server/auth_framework/providers/http_upstream.py index b61705b9..d4864661 100644 --- a/server/src/agent_control_server/auth_framework/providers/http_upstream.py +++ b/server/src/agent_control_server/auth_framework/providers/http_upstream.py @@ -46,6 +46,7 @@ from datetime import datetime from time import perf_counter from typing import Any +from urllib.parse import urlsplit, urlunsplit import httpx from agent_control_models.errors import ErrorCode, ErrorReason @@ -67,6 +68,9 @@ _logger = get_logger(__name__) _DEFAULT_FORWARDED_HEADERS = ("X-API-Key", "Authorization", "Cookie") +_ORBIT_MANAGEMENT_PATH = "/internal/auth/agent_control/check_management_access" +_ORBIT_IDENTITY_PATH = "/internal/auth/resolve_tenant_context" +_IDENTITY_OPERATION = "identity.resolve" _AUTH_UPSTREAM_ATTEMPTS = Counter( "agent_control_server_auth_upstream_attempts_total", @@ -180,6 +184,10 @@ class HttpUpstreamConfig: max_keepalive_connections: int = 20 """Maximum idle connections retained for the auth upstream.""" + identity_url: str | None = None + """URL for credential and namespace resolution. If omitted, the Orbit + identity URL is derived from the known management authorization path.""" + def __post_init__(self) -> None: if self.keepalive_expiry_seconds < 0: raise ValueError("keepalive_expiry_seconds must be greater than or equal to 0") @@ -213,6 +221,7 @@ def __init__( client: httpx.AsyncClient | None = None, ) -> None: self._config = config + self._identity_url = config.identity_url or _derive_orbit_identity_url(config.url) self._owns_client = client is None if client is not None: self._client = client @@ -245,19 +254,49 @@ async def authorize( if context: payload["context"] = context - response = await self._post_upstream(operation, payload, headers) - return self._handle_response(response, operation, context) + response = await self._post_upstream(operation.value, payload, headers) + return self._handle_response(response, operation.value, context) + + async def resolve_identity(self, request: Request, operation: Operation) -> Principal: + """Authenticate with the upstream before a namespace-scoped lookup.""" + del operation # Target-specific permission is checked by authorize afterward. + if self._identity_url is None: + raise APIError( + status_code=500, + error_code=ErrorCode.AUTH_MISCONFIGURED, + reason=ErrorReason.INTERNAL_ERROR, + detail="Authorization identity endpoint is not configured.", + hint="Set AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL.", + ) + response = await self._post_upstream( + _IDENTITY_OPERATION, + None, + self._forward_headers(request), + url=self._identity_url, + ) + if response.status_code == 404: + raise APIError( + status_code=502, + error_code=ErrorCode.AUTH_UPSTREAM_REJECTED, + reason=ErrorReason.INTERNAL_ERROR, + detail="Authorization identity endpoint was not found.", + hint="Check the configured authorization identity URL.", + ) + principal = self._handle_response(response, _IDENTITY_OPERATION, None) + return Principal(namespace_key=principal.namespace_key, caller_id=principal.caller_id) async def _post_upstream( self, - operation: Operation, - payload: dict[str, Any], + operation: str, + payload: dict[str, Any] | None, headers: dict[str, str], + *, + url: str | None = None, ) -> httpx.Response: started = perf_counter() try: response = await self._client.post( - self._config.url, + url or self._config.url, json=payload, headers=headers, ) @@ -270,7 +309,7 @@ async def _post_upstream( ) _logger.warning( "Auth upstream unreachable for operation %s: %s", - operation.value, + operation, exc, ) raise _authorization_service_unavailable_error() from exc @@ -301,7 +340,7 @@ def _forward_headers(self, request: Request) -> dict[str, str]: def _handle_response( self, response: httpx.Response, - operation: Operation, + operation: str, context: dict[str, Any] | None, ) -> Principal: status = response.status_code @@ -318,7 +357,7 @@ def _handle_response( if status == 403: raise ForbiddenError( error_code=ErrorCode.AUTH_INSUFFICIENT_PRIVILEGES, - detail=f"Not authorized to perform {operation.value!r}.", + detail=f"Not authorized to perform {operation!r}.", hint="Contact your administrator if you expected access.", ) if status == 404: @@ -343,7 +382,7 @@ def _handle_response( hint = f"{hint} Retry-After: {retry_after}." _logger.warning( "Upstream returned 429 for operation %s", - operation.value, + operation, ) raise APIError( status_code=503, @@ -355,7 +394,7 @@ def _handle_response( if 400 <= status < 500: _logger.warning( "Authorization upstream rejected operation %s with status %d", - operation.value, + operation, status, ) raise APIError( @@ -375,7 +414,7 @@ def _handle_response( _logger.warning( "Unexpected upstream status %d for operation %s", status, - operation.value, + operation, ) raise APIError( status_code=503, @@ -420,7 +459,7 @@ def _parse_principal(self, response: httpx.Response) -> Principal: def _observe_upstream_attempt( - operation: Operation, + operation: str, duration_seconds: float, *, outcome: str, @@ -428,13 +467,13 @@ def _observe_upstream_attempt( error: httpx.HTTPError | None = None, ) -> None: _AUTH_UPSTREAM_ATTEMPTS.labels( - operation=operation.value, + operation=operation, outcome=outcome, status_code=str(status_code) if status_code is not None else "none", error_type=type(error).__name__ if error is not None else "none", ).inc() _AUTH_UPSTREAM_ATTEMPT_DURATION.labels( - operation=operation.value, + operation=operation, outcome=outcome, ).observe(duration_seconds) @@ -449,6 +488,17 @@ def _authorization_service_unavailable_error() -> APIError: ) +def _derive_orbit_identity_url(url: str) -> str | None: + """Resolve Orbit's existing identity route from its management route.""" + parts = urlsplit(url) + if not parts.path.endswith(_ORBIT_MANAGEMENT_PATH): + return None + prefix = parts.path[: -len(_ORBIT_MANAGEMENT_PATH)] + return urlunsplit( + (parts.scheme, parts.netloc, prefix + _ORBIT_IDENTITY_PATH, parts.query, parts.fragment) + ) + + def _ensure_target_context_matches_grant( context: dict[str, Any] | None, principal: Principal, diff --git a/server/src/agent_control_server/auth_framework/providers/local_jwt.py b/server/src/agent_control_server/auth_framework/providers/local_jwt.py index 7cab77f4..6f068603 100644 --- a/server/src/agent_control_server/auth_framework/providers/local_jwt.py +++ b/server/src/agent_control_server/auth_framework/providers/local_jwt.py @@ -70,12 +70,8 @@ def __init__( # sync with AgentControlClient._runtime_token_use_bearer in the SDK. self._require_bearer = self._header_name.lower() == "authorization" - async def authorize( - self, - request: Request, - operation: Operation, - context: dict[str, Any] | None = None, - ) -> Principal: + async def resolve_identity(self, request: Request, operation: Operation) -> Principal: + """Verify the runtime token and return its bound identity.""" token = self._extract_bearer_token(request) try: claims = verify_runtime_token(token, self._secret) @@ -96,29 +92,39 @@ async def authorize( hint="Request a token with the required scope.", ) + return Principal( + namespace_key=claims.namespace_key, + caller_id=claims.actor_id, + target_type=claims.target_type, + target_id=claims.target_id, + scopes=claims.scopes, + grant_expires_at=claims.expires_at, + ) + + async def authorize( + self, + request: Request, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + principal = await self.resolve_identity(request, operation) + requested_target_type = context.get("target_type") if context is not None else None requested_target_id = context.get("target_id") if context is not None else None - if requested_target_type != claims.target_type: + if requested_target_type != principal.target_type: raise ForbiddenError( error_code=ErrorCode.AUTH_INSUFFICIENT_PRIVILEGES, detail="Runtime token target_type does not match the request.", hint="Re-exchange a token bound to the request target.", ) - if requested_target_id != claims.target_id: + if requested_target_id != principal.target_id: raise ForbiddenError( error_code=ErrorCode.AUTH_INSUFFICIENT_PRIVILEGES, detail="Runtime token target_id does not match the request.", hint="Re-exchange a token bound to the request target.", ) - return Principal( - namespace_key=claims.namespace_key, - caller_id=claims.actor_id, - target_type=claims.target_type, - target_id=claims.target_id, - scopes=claims.scopes, - grant_expires_at=claims.expires_at, - ) + return principal def _extract_bearer_token(self, request: Request) -> str: header = request.headers.get(self._header_name) diff --git a/server/src/agent_control_server/endpoints/control_bindings.py b/server/src/agent_control_server/endpoints/control_bindings.py index 279328c4..be06c2a6 100644 --- a/server/src/agent_control_server/endpoints/control_bindings.py +++ b/server/src/agent_control_server/endpoints/control_bindings.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Awaitable, Callable from typing import Any from agent_control_models.errors import ErrorCode @@ -23,9 +24,10 @@ from fastapi import APIRouter, Depends, Query, Request from sqlalchemy.ext.asyncio import AsyncSession -from ..auth_framework import Operation, Principal, require_operation -from ..db import get_async_db -from ..errors import BadRequestError +from ..auth_framework import Operation, Principal, get_authorizer, require_operation +from ..auth_framework.core import IdentityResolver +from ..db import AsyncSessionLocal, get_async_db +from ..errors import BadRequestError, ForbiddenError, NotFoundError from ..models import ControlBinding from ..services.control_bindings import ControlBindingsService @@ -72,6 +74,39 @@ async def _binding_list_context(request: Request) -> dict[str, Any]: return {"target_type": target_type, "target_id": target_id} +def _require_binding_operation( + operation: Operation, +) -> Callable[..., Awaitable[Principal]]: + """Authorize a by-ID route against the binding's stored target.""" + + async def dependency(request: Request, binding_id: int) -> Principal: + authorizer = get_authorizer(operation) + if isinstance(authorizer, IdentityResolver): + identity = await authorizer.resolve_identity(request, operation) + else: + # Legacy providers can still resolve namespace-wide operations. + # Target-bound providers should implement IdentityResolver. + identity = await authorizer.authorize(request, operation) + # Use a short-lived session so no database connection is held while the + # authorization provider performs a potentially remote request. + async with AsyncSessionLocal() as db: + target_type, target_id = await ControlBindingsService( + db + ).get_binding_target_for_authorization_or_404( + namespace_key=identity.namespace_key, binding_id=binding_id + ) + + context = {"target_type": target_type, "target_id": target_id} + try: + return await authorizer.authorize(request, operation, context) + except (ForbiddenError, NotFoundError) as exc: + # A caller cannot distinguish a binding it cannot access from a + # missing binding in its namespace. + raise ControlBindingsService.binding_not_found(binding_id) from exc + + return dependency + + def _to_response(binding: ControlBinding) -> GetControlBindingResponse: return GetControlBindingResponse( id=binding.id, @@ -191,22 +226,19 @@ async def list_control_bindings( @router.get( "/{binding_id}", response_model=GetControlBindingResponse, - summary="Get a control binding (namespace-wide)", + summary="Get a control binding", response_description="The requested binding", ) async def get_control_binding( binding_id: int, db: AsyncSession = Depends(get_async_db), - principal: Principal = Depends(require_operation(Operation.CONTROL_BINDINGS_READ)), + principal: Principal = Depends(_require_binding_operation(Operation.CONTROL_BINDINGS_READ)), ) -> GetControlBindingResponse: """Read a single control binding by surrogate ID. - Authorization is namespace-wide: the binding's target identifiers - are not available until after the row is loaded. - Callers whose authorization model requires per-target permissions - should use the natural-key endpoints (``PUT /by-key``, - ``POST /by-key:delete``) and the target-filtered list endpoint, all - of which include ``(target_type, target_id)`` in the request context. + Authorization uses the binding's stored target identifiers. After + authorization succeeds, the row is loaded again using the namespace + resolved by the authorizer before any binding data is returned. """ service = ControlBindingsService(db) binding = await service.get_binding_or_404( @@ -252,21 +284,19 @@ async def patch_control_binding_by_key( @router.patch( "/{binding_id}", response_model=PatchControlBindingResponse, - summary="Update a control binding (namespace-wide)", + summary="Update a control binding", response_description="Updated enabled flag", ) async def patch_control_binding( binding_id: int, request: PatchControlBindingRequest, db: AsyncSession = Depends(get_async_db), - principal: Principal = Depends(require_operation(Operation.CONTROL_BINDINGS_WRITE)), + principal: Principal = Depends(_require_binding_operation(Operation.CONTROL_BINDINGS_WRITE)), ) -> PatchControlBindingResponse: """Update the ``enabled`` flag on a control binding. - See the GET-by-id docstring for the authorization scope: this route - is namespace-wide because the target identifiers are not available - before the binding is loaded. Use ``PUT /by-key`` for target-scoped - upserts that include the target in the request context. + Authorization uses the binding's stored target identifiers. The mutation + remains scoped to the namespace resolved by the authorizer. """ service = ControlBindingsService(db) binding = await service.set_enabled( @@ -281,20 +311,18 @@ async def patch_control_binding( @router.delete( "/{binding_id}", response_model=DeleteControlBindingResponse, - summary="Delete a control binding (namespace-wide)", + summary="Delete a control binding", response_description="Deletion confirmation", ) async def delete_control_binding( binding_id: int, db: AsyncSession = Depends(get_async_db), - principal: Principal = Depends(require_operation(Operation.CONTROL_BINDINGS_WRITE)), + principal: Principal = Depends(_require_binding_operation(Operation.CONTROL_BINDINGS_WRITE)), ) -> DeleteControlBindingResponse: """Delete a control binding by surrogate ID. - See the GET-by-id docstring for the authorization scope: this route - is namespace-wide because the target identifiers are not available - before the binding is loaded. Use ``POST /by-key:delete`` for - target-scoped detach that includes the target in the request context. + Authorization uses the binding's stored target identifiers. The deletion + remains scoped to the namespace resolved by the authorizer. """ service = ControlBindingsService(db) await service.delete_binding(namespace_key=principal.namespace_key, binding_id=binding_id) diff --git a/server/src/agent_control_server/services/control_bindings.py b/server/src/agent_control_server/services/control_bindings.py index b4118bd6..e6dc35e5 100644 --- a/server/src/agent_control_server/services/control_bindings.py +++ b/server/src/agent_control_server/services/control_bindings.py @@ -238,15 +238,34 @@ async def get_binding_or_404(self, *, namespace_key: str, binding_id: int) -> Co result = await self._db.execute(stmt) binding = cast(ControlBinding | None, result.scalars().first()) if binding is None: - raise NotFoundError( - error_code=ErrorCode.CONTROL_BINDING_NOT_FOUND, - detail=f"Control binding with ID '{binding_id}' not found", - resource="ControlBinding", - resource_id=str(binding_id), - hint="Verify the binding ID and that it belongs to this namespace.", - ) + raise self.binding_not_found(binding_id) return binding + async def get_binding_target_for_authorization_or_404( + self, *, namespace_key: str, binding_id: int + ) -> tuple[str, str]: + """Load a binding's target within the authenticated namespace.""" + stmt = select(ControlBinding.target_type, ControlBinding.target_id).where( + ControlBinding.id == binding_id, + ControlBinding.namespace_key == namespace_key, + ) + result = await self._db.execute(stmt) + target = result.tuples().first() + if target is None: + raise self.binding_not_found(binding_id) + target_type, target_id = target + return target_type, target_id + + @staticmethod + def binding_not_found(binding_id: int) -> NotFoundError: + return NotFoundError( + error_code=ErrorCode.CONTROL_BINDING_NOT_FOUND, + detail=f"Control binding with ID '{binding_id}' not found", + resource="ControlBinding", + resource_id=str(binding_id), + hint="Verify the binding ID and that it belongs to this namespace.", + ) + async def list_bindings( self, *, diff --git a/server/tests/test_auth_framework.py b/server/tests/test_auth_framework.py index 289a1889..c2cd288f 100644 --- a/server/tests/test_auth_framework.py +++ b/server/tests/test_auth_framework.py @@ -279,6 +279,146 @@ def factory(request: httpx.Request) -> httpx.Response: assert captured["headers"]["x-api-key"] == "caller-key" +@pytest.mark.asyncio +async def test_http_upstream_resolves_identity_at_derived_orbit_url(): + captured: dict[str, Any] = {} + + def factory(request: httpx.Request) -> httpx.Response: + captured["url"] = str(request.url) + captured["method"] = request.method + captured["headers"] = dict(request.headers) + return httpx.Response( + 200, + json={ + "namespace_key": "org-7", + "caller_id": "user-42", + "is_admin": True, + "scopes": ["controls.read"], + }, + ) + + provider = _build_upstream( + factory, + config_overrides={ + "url": ("https://orbit.example/prefix/internal/auth/agent_control/check_management_access"), + "service_token": "service-secret", + "extra_forward_headers": ("X-Deployer-Auth",), + }, + ) + principal = await provider.resolve_identity( + _build_request( + headers={ + "X-API-Key": "caller-key", + "Authorization": "Bearer caller-token", + "Cookie": "session=caller-session", + "X-Deployer-Auth": "deployer-key", + } + ), + Operation.CONTROL_BINDINGS_WRITE, + ) + + assert captured["method"] == "POST" + assert captured["url"] == "https://orbit.example/prefix/internal/auth/resolve_tenant_context" + assert captured["headers"]["x-api-key"] == "caller-key" + assert captured["headers"]["authorization"] == "Bearer caller-token" + assert captured["headers"]["cookie"] == "session=caller-session" + assert captured["headers"]["x-deployer-auth"] == "deployer-key" + assert captured["headers"]["x-agent-control-service-token"] == "service-secret" + assert principal == Principal(namespace_key="org-7", caller_id="user-42") + + +@pytest.mark.asyncio +async def test_http_upstream_resolves_identity_at_explicit_url(): + captured: dict[str, str] = {} + + def factory(request: httpx.Request) -> httpx.Response: + captured["url"] = str(request.url) + return httpx.Response(200, json={"namespace_key": "tenant-1"}) + + provider = _build_upstream( + factory, + config_overrides={"identity_url": "https://identity.example/resolve"}, + ) + principal = await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) + + assert captured["url"] == "https://identity.example/resolve" + assert principal == Principal(namespace_key="tenant-1") + + +@pytest.mark.asyncio +async def test_http_upstream_identity_requires_configured_url(): + provider = _build_upstream(lambda request: pytest.fail("unexpected upstream call")) + + with pytest.raises(APIError) as exc_info: + await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) + + assert exc_info.value.status_code == 500 + assert exc_info.value.error_code == "AUTH_MISCONFIGURED" + + +@pytest.mark.asyncio +async def test_http_upstream_identity_preserves_401(): + provider = _build_upstream( + lambda request: httpx.Response(401), + config_overrides={"identity_url": "https://identity.example/resolve"}, + ) + + with pytest.raises(AuthenticationError) as exc_info: + await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) + + assert exc_info.value.status_code == 401 + + +@pytest.mark.asyncio +async def test_http_upstream_identity_treats_missing_route_as_upstream_error(): + provider = _build_upstream( + lambda request: httpx.Response(404), + config_overrides={"identity_url": "https://identity.example/resolve"}, + ) + + with pytest.raises(APIError) as exc_info: + await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) + + assert exc_info.value.status_code == 502 + assert exc_info.value.error_code == "AUTH_UPSTREAM_REJECTED" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("status", [429, 500, 503]) +async def test_http_upstream_identity_fails_closed_when_unavailable(status: int): + provider = _build_upstream( + lambda request: httpx.Response(status), + config_overrides={"identity_url": "https://identity.example/resolve"}, + ) + + with pytest.raises(APIError) as exc_info: + await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) + + assert exc_info.value.status_code == 503 + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "grant", + [ + {}, + {"namespace_key": ""}, + {"namespace_key": 42}, + {"namespace_key": "tenant-1", "is_admin": "true"}, + ], +) +async def test_http_upstream_identity_rejects_malformed_grant(grant: dict[str, Any]): + provider = _build_upstream( + lambda request: httpx.Response(200, json=grant), + config_overrides={"identity_url": "https://identity.example/resolve"}, + ) + + with pytest.raises(APIError) as exc_info: + await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) + + assert exc_info.value.status_code == 502 + + @pytest.mark.asyncio async def test_http_upstream_forwards_service_token(): captured: dict[str, Any] = {} @@ -1528,6 +1668,27 @@ async def test_configure_http_upstream_extra_forward_headers_env(monkeypatch): await auth_config.teardown_auth() +@pytest.mark.asyncio +async def test_configure_http_upstream_identity_url_env(monkeypatch): + from agent_control_server.auth_framework import config as auth_config + + clear_authorizers() + monkeypatch.setenv("AGENT_CONTROL_AUTH_MODE", "http_upstream") + monkeypatch.setenv("AGENT_CONTROL_AUTH_UPSTREAM_URL", "https://auth.example.test/check") + monkeypatch.setenv( + "AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL", + " https://identity.example.test/resolve ", + ) + + try: + auth_config.configure_auth_from_env() + provider = get_authorizer(Operation.CONTROL_BINDINGS_READ) + assert isinstance(provider, HttpUpstreamAuthProvider) + assert provider._config.identity_url == "https://identity.example.test/resolve" + finally: + await auth_config.teardown_auth() + + @pytest.mark.asyncio async def test_configure_http_upstream_ca_file_env(monkeypatch): from agent_control_server.auth_framework import config as auth_config diff --git a/server/tests/test_control_bindings_endpoints.py b/server/tests/test_control_bindings_endpoints.py index 8333bb95..116808e3 100644 --- a/server/tests/test_control_bindings_endpoints.py +++ b/server/tests/test_control_bindings_endpoints.py @@ -5,9 +5,14 @@ import uuid from typing import Any +import pytest +from agent_control_models.errors import ErrorCode, ErrorReason +from fastapi.testclient import TestClient +from httpx import Response + from agent_control_server.auth_framework import Operation, Principal, set_authorizer +from agent_control_server.errors import APIError, ForbiddenError, NotFoundError from agent_control_server.models import DEFAULT_NAMESPACE_KEY -from fastapi.testclient import TestClient from .utils import VALID_CONTROL_PAYLOAD @@ -100,6 +105,374 @@ def test_get_unknown_binding_returns_404(client: TestClient) -> None: assert resp.json()["error_code"] == "CONTROL_BINDING_NOT_FOUND" +def test_binding_id_routes_authorize_the_stored_target(client: TestClient) -> None: + # Given a binding whose opaque target is stored only in the database. + control_id = _create_control(client) + binding_id = _create_binding( + client, + control_id=control_id, + target_type="custom_target", + target_id="target-42", + )["binding_id"] + calls: list[tuple[str, Operation, dict[str, Any] | None]] = [] + + class RecordingAuthorizer: + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request + calls.append(("identity", operation, None)) + return Principal(namespace_key=DEFAULT_NAMESPACE_KEY, is_admin=True) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request + calls.append(("authorize", operation, context)) + return Principal(namespace_key=DEFAULT_NAMESPACE_KEY, is_admin=True) + + set_authorizer(RecordingAuthorizer()) + + # When each by-ID route is called without target data in the request. + get_resp = client.get(f"{_BINDINGS_URL}/{binding_id}") + patch_resp = client.patch(f"{_BINDINGS_URL}/{binding_id}", json={"enabled": False}) + delete_resp = client.delete(f"{_BINDINGS_URL}/{binding_id}") + + # Then every authorization call receives the target stored on the binding. + assert get_resp.status_code == 200, get_resp.text + assert patch_resp.status_code == 200, patch_resp.text + assert delete_resp.status_code == 200, delete_resp.text + target_context = {"target_type": "custom_target", "target_id": "target-42"} + assert calls == [ + ("identity", Operation.CONTROL_BINDINGS_READ, None), + ("authorize", Operation.CONTROL_BINDINGS_READ, target_context), + ("identity", Operation.CONTROL_BINDINGS_WRITE, None), + ("authorize", Operation.CONTROL_BINDINGS_WRITE, target_context), + ("identity", Operation.CONTROL_BINDINGS_WRITE, None), + ("authorize", Operation.CONTROL_BINDINGS_WRITE, target_context), + ] + + +@pytest.mark.parametrize("method", ["get", "patch", "delete"]) +@pytest.mark.parametrize("exists", [True, False]) +@pytest.mark.parametrize( + ("api_key", "expected_error_code"), + [(None, "AUTH_MISSING_KEY"), ("wrong-key", "AUTH_INVALID_KEY")], +) +def test_binding_id_routes_preserve_authentication_errors( + method: str, + exists: bool, + api_key: str | None, + expected_error_code: str, + client: TestClient, + app: object, +) -> None: + # Given: a caller with no valid credential and an existing or missing ID. + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + headers = {} if api_key is None else {"X-API-Key": api_key} + unauthorized_client = TestClient(app, raise_server_exceptions=True, headers=headers) + + # When: the caller accesses the binding by ID. + url = f"{_BINDINGS_URL}/{binding_id if exists else 999_999}" + if method == "get": + response = unauthorized_client.get(url) + elif method == "patch": + response = unauthorized_client.patch(url, json={"enabled": False}) + else: + response = unauthorized_client.delete(url) + + # Then: the route preserves the normal 401 challenge. + assert response.status_code == 401 + assert response.json()["error_code"] == expected_error_code + assert response.headers["WWW-Authenticate"] == "ApiKey" + + +@pytest.mark.parametrize("method", ["patch", "delete"]) +def test_binding_id_write_checks_local_access_before_lookup( + method: str, non_admin_client: TestClient +) -> None: + # Given: a valid non-admin key and a missing binding ID. + url = f"{_BINDINGS_URL}/999999" + + # When: the caller attempts an ID-based write. + if method == "patch": + response = non_admin_client.patch(url, json={"enabled": False}) + else: + response = non_admin_client.delete(url) + + # Then: the operation-wide access check preserves the normal 403. + assert response.status_code == 403 + + +@pytest.mark.parametrize("method", ["get", "patch", "delete"]) +def test_binding_id_lookup_is_scoped_before_target_authorization( + method: str, client: TestClient +) -> None: + class NamespaceAuthorizer: + namespace_key = "other-namespace" + + def __init__(self) -> None: + self.target_calls = 0 + + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request, operation + return Principal(namespace_key=self.namespace_key, is_admin=True) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request, operation, context + self.target_calls += 1 + return Principal(namespace_key=self.namespace_key, is_admin=True) + + # Given: a binding in another namespace and an identity in the default namespace. + authorizer = NamespaceAuthorizer() + set_authorizer(authorizer) + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + authorizer.namespace_key = DEFAULT_NAMESPACE_KEY + authorizer.target_calls = 0 + + # When: the caller tries the foreign ID and a missing ID. + def request_id(requested_id: int) -> Response: + url = f"{_BINDINGS_URL}/{requested_id}" + if method == "get": + return client.get(url) + if method == "patch": + return client.patch(url, json={"enabled": False}) + return client.delete(url) + + foreign = request_id(binding_id) + missing = request_id(999_999) + + # Then: neither ID reaches target authorization or mutates the binding. + assert foreign.status_code == missing.status_code == 404 + assert foreign.json()["error_code"] == missing.json()["error_code"] == ( + "CONTROL_BINDING_NOT_FOUND" + ) + assert authorizer.target_calls == 0 + authorizer.namespace_key = "other-namespace" + stored = client.get(f"{_BINDINGS_URL}/{binding_id}") + assert stored.status_code == 200 + assert stored.json()["enabled"] is True + + +def test_binding_id_identity_failure_does_not_depend_on_id( + client: TestClient, +) -> None: + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + + class UnavailableAuthorizer: + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request, operation + raise APIError( + status_code=503, + error_code=ErrorCode.AUTH_MISCONFIGURED, + reason=ErrorReason.SERVICE_UNAVAILABLE, + detail="Authorization service unavailable.", + ) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + raise AssertionError("Target authorization must not run after identity failure") + + # Given: the identity service is unavailable before any binding lookup. + set_authorizer(UnavailableAuthorizer()) + + # When: the caller requests an existing and a missing ID. + existing = client.get(f"{_BINDINGS_URL}/{binding_id}") + missing = client.get(f"{_BINDINGS_URL}/999999") + + # Then: both fail with the same upstream error. + assert existing.status_code == missing.status_code == 503 + assert existing.json()["error_code"] == missing.json()["error_code"] == "AUTH_MISCONFIGURED" + + +@pytest.mark.parametrize("denial", ["forbidden", "not_found"]) +def test_binding_id_target_denial_looks_like_missing_binding( + denial: str, client: TestClient +) -> None: + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + + class DenyingAuthorizer: + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request, operation + return Principal(namespace_key=DEFAULT_NAMESPACE_KEY) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request, operation, context + if denial == "forbidden": + raise ForbiddenError( + error_code=ErrorCode.AUTH_INSUFFICIENT_PRIVILEGES, + detail="Target access denied.", + ) + raise NotFoundError( + error_code=ErrorCode.AUTH_INVALID_KEY, + detail="Target not found.", + resource="Target", + ) + + # Given: an identity in the binding's namespace without target access. + set_authorizer(DenyingAuthorizer()) + + # When: the caller requests the denied binding and a missing ID. + denied = client.get(f"{_BINDINGS_URL}/{binding_id}") + missing = client.get(f"{_BINDINGS_URL}/999999") + + # Then: both use the binding-not-found response contract. + assert denied.status_code == missing.status_code == 404 + assert denied.json()["error_code"] == missing.json()["error_code"] == ( + "CONTROL_BINDING_NOT_FOUND" + ) + + +def test_binding_id_legacy_authorizer_preflights_before_lookup(client: TestClient) -> None: + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + contexts: list[dict[str, Any] | None] = [] + + class LegacyAuthorizer: + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request, operation + contexts.append(context) + return Principal(namespace_key=DEFAULT_NAMESPACE_KEY) + + # Given: a provider that implements the original authorize-only contract. + set_authorizer(LegacyAuthorizer()) + + # When: an ID-based binding read succeeds. + response = client.get(f"{_BINDINGS_URL}/{binding_id}") + + # Then: operation-wide auth runs before target auth. + assert response.status_code == 200 + assert contexts == [None, {"target_type": "env", "target_id": "prod"}] + + +def test_binding_id_delete_rechecks_the_authorized_namespace( + client: TestClient, +) -> None: + # Given a binding in the default namespace and an authorizer resolving another one. + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + authorized_namespace = "other-namespace" + + class NamespaceAuthorizer: + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request, operation + return Principal(namespace_key=authorized_namespace, is_admin=True) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request, operation, context + return Principal(namespace_key=authorized_namespace, is_admin=True) + + set_authorizer(NamespaceAuthorizer()) + + # When deletion is authorized for a namespace that does not own the row. + delete_resp = client.delete(f"{_BINDINGS_URL}/{binding_id}") + + # Then the scoped mutation is rejected and the original binding remains. + assert delete_resp.status_code == 404 + authorized_namespace = DEFAULT_NAMESPACE_KEY + get_resp = client.get(f"{_BINDINGS_URL}/{binding_id}") + assert get_resp.status_code == 200, get_resp.text + + +def test_binding_id_patch_rechecks_the_authorized_namespace( + client: TestClient, +) -> None: + # Given a binding in the default namespace and an authorizer resolving another one. + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + authorized_namespace = "other-namespace" + + class NamespaceAuthorizer: + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request, operation + return Principal(namespace_key=authorized_namespace, is_admin=True) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request, operation, context + return Principal(namespace_key=authorized_namespace, is_admin=True) + + set_authorizer(NamespaceAuthorizer()) + + # When an update is authorized for a namespace that does not own the row. + patch_resp = client.patch( + f"{_BINDINGS_URL}/{binding_id}", + json={"enabled": False}, + ) + + # Then the scoped mutation is rejected and the original binding remains enabled. + assert patch_resp.status_code == 404 + authorized_namespace = DEFAULT_NAMESPACE_KEY + get_resp = client.get(f"{_BINDINGS_URL}/{binding_id}") + assert get_resp.status_code == 200, get_resp.text + assert get_resp.json()["enabled"] is True + + +def test_binding_id_path_validation_runs_before_authorization( + client: TestClient, +) -> None: + # Given an authorizer that records every invocation. + calls: list[Operation] = [] + + class RecordingAuthorizer: + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request + calls.append(operation) + return Principal(namespace_key=DEFAULT_NAMESPACE_KEY, is_admin=True) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request, context + calls.append(operation) + return Principal(namespace_key=DEFAULT_NAMESPACE_KEY, is_admin=True) + + set_authorizer(RecordingAuthorizer()) + + # When a caller supplies a non-integer binding ID. + resp = client.get(f"{_BINDINGS_URL}/not-an-integer") + + # Then FastAPI reports validation without loading or authorizing a binding. + assert resp.status_code == 422 + assert calls == [] + + def test_list_bindings_returns_all(client: TestClient) -> None: control_id = _create_control(client) _create_binding( @@ -252,6 +625,10 @@ def test_non_admin_cannot_write(non_admin_client: TestClient, client: TestClient delete_resp = non_admin_client.delete(f"{_BINDINGS_URL}/{binding_id}") assert delete_resp.status_code == 403 + unchanged = client.get(f"{_BINDINGS_URL}/{binding_id}") + assert unchanged.status_code == 200, unchanged.text + assert unchanged.json()["enabled"] is True + def test_non_admin_can_read(non_admin_client: TestClient, client: TestClient) -> None: control_id = _create_control(client) From 7680e10e38cbd1d9677bfcaaa834834f9bcddc0b Mon Sep 17 00:00:00 2001 From: Joshua Jeon Date: Wed, 30 Sep 2026 11:47:07 -0700 Subject: [PATCH 2/4] fix(server): preserve legacy binding ID authorization --- .../funcs/control-bindings-delete.ts | 5 +- .../generated/funcs/control-bindings-get.ts | 6 +- .../funcs/control-bindings-update.ts | 5 +- .../src/generated/sdk/control-bindings.ts | 16 +- .../auth_framework/config.py | 7 +- .../auth_framework/core.py | 11 +- .../auth_framework/providers/header.py | 4 - .../auth_framework/providers/http_upstream.py | 5 + .../auth_framework/providers/local_jwt.py | 2 + .../endpoints/control_bindings.py | 34 +++-- server/tests/test_auth_framework.py | 5 +- .../tests/test_control_bindings_endpoints.py | 137 +++++++++++++++++- 12 files changed, 195 insertions(+), 42 deletions(-) diff --git a/sdks/typescript/src/generated/funcs/control-bindings-delete.ts b/sdks/typescript/src/generated/funcs/control-bindings-delete.ts index a8ff3f41..50f80201 100644 --- a/sdks/typescript/src/generated/funcs/control-bindings-delete.ts +++ b/sdks/typescript/src/generated/funcs/control-bindings-delete.ts @@ -33,8 +33,9 @@ import { Result } from "../types/fp.js"; * @remarks * Delete a control binding by surrogate ID. * - * Authorization uses the binding's stored target identifiers. The deletion - * remains scoped to the namespace resolved by the authorizer. + * Target-aware authorizers use the binding's stored target identifiers. + * Other authorizers retain namespace-wide authorization. The deletion + * remains scoped to the authorized namespace. */ export function controlBindingsDelete( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/control-bindings-get.ts b/sdks/typescript/src/generated/funcs/control-bindings-get.ts index c83512aa..306f21d3 100644 --- a/sdks/typescript/src/generated/funcs/control-bindings-get.ts +++ b/sdks/typescript/src/generated/funcs/control-bindings-get.ts @@ -33,9 +33,9 @@ import { Result } from "../types/fp.js"; * @remarks * Read a single control binding by surrogate ID. * - * Authorization uses the binding's stored target identifiers. After - * authorization succeeds, the row is loaded again using the namespace - * resolved by the authorizer before any binding data is returned. + * Target-aware authorizers use the binding's stored target identifiers. + * Other authorizers retain namespace-wide authorization. The row is loaded + * using the authorized namespace before any binding data is returned. */ export function controlBindingsGet( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/funcs/control-bindings-update.ts b/sdks/typescript/src/generated/funcs/control-bindings-update.ts index c1bf2062..f3874a38 100644 --- a/sdks/typescript/src/generated/funcs/control-bindings-update.ts +++ b/sdks/typescript/src/generated/funcs/control-bindings-update.ts @@ -33,8 +33,9 @@ import { Result } from "../types/fp.js"; * @remarks * Update the ``enabled`` flag on a control binding. * - * Authorization uses the binding's stored target identifiers. The mutation - * remains scoped to the namespace resolved by the authorizer. + * Target-aware authorizers use the binding's stored target identifiers. + * Other authorizers retain namespace-wide authorization. The mutation + * remains scoped to the authorized namespace. */ export function controlBindingsUpdate( client: AgentControlSDKCore, diff --git a/sdks/typescript/src/generated/sdk/control-bindings.ts b/sdks/typescript/src/generated/sdk/control-bindings.ts index 1f2ac5b7..6bb6caa1 100644 --- a/sdks/typescript/src/generated/sdk/control-bindings.ts +++ b/sdks/typescript/src/generated/sdk/control-bindings.ts @@ -123,8 +123,9 @@ export class ControlBindings extends ClientSDK { * @remarks * Delete a control binding by surrogate ID. * - * Authorization uses the binding's stored target identifiers. The deletion - * remains scoped to the namespace resolved by the authorizer. + * Target-aware authorizers use the binding's stored target identifiers. + * Other authorizers retain namespace-wide authorization. The deletion + * remains scoped to the authorized namespace. */ async delete( request: @@ -144,9 +145,9 @@ export class ControlBindings extends ClientSDK { * @remarks * Read a single control binding by surrogate ID. * - * Authorization uses the binding's stored target identifiers. After - * authorization succeeds, the row is loaded again using the namespace - * resolved by the authorizer before any binding data is returned. + * Target-aware authorizers use the binding's stored target identifiers. + * Other authorizers retain namespace-wide authorization. The row is loaded + * using the authorized namespace before any binding data is returned. */ async get( request: @@ -166,8 +167,9 @@ export class ControlBindings extends ClientSDK { * @remarks * Update the ``enabled`` flag on a control binding. * - * Authorization uses the binding's stored target identifiers. The mutation - * remains scoped to the namespace resolved by the authorizer. + * Target-aware authorizers use the binding's stored target identifiers. + * Other authorizers retain namespace-wide authorization. The mutation + * remains scoped to the authorized namespace. */ async update( request: diff --git a/server/src/agent_control_server/auth_framework/config.py b/server/src/agent_control_server/auth_framework/config.py index 6fc4c6ff..275c5066 100644 --- a/server/src/agent_control_server/auth_framework/config.py +++ b/server/src/agent_control_server/auth_framework/config.py @@ -111,9 +111,10 @@ def configure_auth_from_env() -> None: is unset, startup selects ``api_key`` only if local API-key validation is enabled; otherwise it selects ``none``. - ``AGENT_CONTROL_AUTH_MODE=http_upstream``: :class:`HttpUpstreamAuthProvider` - pointed at ``AGENT_CONTROL_AUTH_UPSTREAM_URL``. By-ID binding routes also - use ``AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL`` when the identity URL - cannot be derived from Orbit's management authorization URL. + pointed at ``AGENT_CONTROL_AUTH_UPSTREAM_URL``. By-ID binding routes use + stored-target authorization when an identity URL can be derived from + Orbit's management URL or ``AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL`` is + set. Other upstreams retain the prior namespace-wide authorization flow. Runtime flow: diff --git a/server/src/agent_control_server/auth_framework/core.py b/server/src/agent_control_server/auth_framework/core.py index 028987ca..c0c0a59f 100644 --- a/server/src/agent_control_server/auth_framework/core.py +++ b/server/src/agent_control_server/auth_framework/core.py @@ -117,12 +117,17 @@ async def authorize( @runtime_checkable class IdentityResolver(Protocol): - """Optional credential and namespace lookup for target-bound authorizers. + """Opt-in credential and namespace lookup for target-bound authorizers. - It does not grant resource access. The route still calls ``authorize`` - with the stored target after the namespace-scoped lookup. + Providers opt in only when they can resolve an identity without changing + the authorization behavior of existing namespace-wide binding ID routes. + The route then calls ``authorize`` with the stored target after the + namespace-scoped lookup. """ + @property + def binding_target_authorization(self) -> bool: ... + async def resolve_identity(self, request: Request, operation: Operation) -> Principal: ... diff --git a/server/src/agent_control_server/auth_framework/providers/header.py b/server/src/agent_control_server/auth_framework/providers/header.py index 5d6c49ad..2d917d91 100644 --- a/server/src/agent_control_server/auth_framework/providers/header.py +++ b/server/src/agent_control_server/auth_framework/providers/header.py @@ -75,10 +75,6 @@ def __init__( ) self._default_namespace_key = default_namespace_key - async def resolve_identity(self, request: Request, operation: Operation) -> Principal: - """Apply the local operation gate before a namespace-scoped lookup.""" - return await self.authorize(request, operation) - async def authorize( self, request: Request, diff --git a/server/src/agent_control_server/auth_framework/providers/http_upstream.py b/server/src/agent_control_server/auth_framework/providers/http_upstream.py index d4864661..8423250f 100644 --- a/server/src/agent_control_server/auth_framework/providers/http_upstream.py +++ b/server/src/agent_control_server/auth_framework/providers/http_upstream.py @@ -238,6 +238,11 @@ def __init__( client_kwargs["verify"] = ssl.create_default_context(cafile=config.ca_file) self._client = httpx.AsyncClient(**client_kwargs) + @property + def binding_target_authorization(self) -> bool: + """Use stored-target checks only when a separate identity URL exists.""" + return self._identity_url is not None + async def aclose(self) -> None: """Release the HTTP client if this provider created it.""" if self._owns_client: diff --git a/server/src/agent_control_server/auth_framework/providers/local_jwt.py b/server/src/agent_control_server/auth_framework/providers/local_jwt.py index 6f068603..dd208739 100644 --- a/server/src/agent_control_server/auth_framework/providers/local_jwt.py +++ b/server/src/agent_control_server/auth_framework/providers/local_jwt.py @@ -53,6 +53,8 @@ def validate_http_field_name(name: str) -> str: class LocalJwtVerifyProvider(RequestAuthorizer): """Verifies a runtime Bearer token and emits a target-bound :class:`Principal`.""" + binding_target_authorization = True + def __init__( self, *, diff --git a/server/src/agent_control_server/endpoints/control_bindings.py b/server/src/agent_control_server/endpoints/control_bindings.py index be06c2a6..09ba983c 100644 --- a/server/src/agent_control_server/endpoints/control_bindings.py +++ b/server/src/agent_control_server/endpoints/control_bindings.py @@ -77,16 +77,20 @@ async def _binding_list_context(request: Request) -> dict[str, Any]: def _require_binding_operation( operation: Operation, ) -> Callable[..., Awaitable[Principal]]: - """Authorize a by-ID route against the binding's stored target.""" + """Use stored-target authorization when the provider opts in.""" async def dependency(request: Request, binding_id: int) -> Principal: authorizer = get_authorizer(operation) - if isinstance(authorizer, IdentityResolver): - identity = await authorizer.resolve_identity(request, operation) - else: - # Legacy providers can still resolve namespace-wide operations. - # Target-bound providers should implement IdentityResolver. - identity = await authorizer.authorize(request, operation) + if ( + not isinstance(authorizer, IdentityResolver) + or not authorizer.binding_target_authorization + ): + # Preserve the existing single namespace-wide authorization call + # for providers without an independent identity lookup. Their + # operation contract may reject a new target context altogether. + return await authorizer.authorize(request, operation) + + identity = await authorizer.resolve_identity(request, operation) # Use a short-lived session so no database connection is held while the # authorization provider performs a potentially remote request. async with AsyncSessionLocal() as db: @@ -236,9 +240,9 @@ async def get_control_binding( ) -> GetControlBindingResponse: """Read a single control binding by surrogate ID. - Authorization uses the binding's stored target identifiers. After - authorization succeeds, the row is loaded again using the namespace - resolved by the authorizer before any binding data is returned. + Target-aware authorizers use the binding's stored target identifiers. + Other authorizers retain namespace-wide authorization. The row is loaded + using the authorized namespace before any binding data is returned. """ service = ControlBindingsService(db) binding = await service.get_binding_or_404( @@ -295,8 +299,9 @@ async def patch_control_binding( ) -> PatchControlBindingResponse: """Update the ``enabled`` flag on a control binding. - Authorization uses the binding's stored target identifiers. The mutation - remains scoped to the namespace resolved by the authorizer. + Target-aware authorizers use the binding's stored target identifiers. + Other authorizers retain namespace-wide authorization. The mutation + remains scoped to the authorized namespace. """ service = ControlBindingsService(db) binding = await service.set_enabled( @@ -321,8 +326,9 @@ async def delete_control_binding( ) -> DeleteControlBindingResponse: """Delete a control binding by surrogate ID. - Authorization uses the binding's stored target identifiers. The deletion - remains scoped to the namespace resolved by the authorizer. + Target-aware authorizers use the binding's stored target identifiers. + Other authorizers retain namespace-wide authorization. The deletion + remains scoped to the authorized namespace. """ service = ControlBindingsService(db) await service.delete_binding(namespace_key=principal.namespace_key, binding_id=binding_id) diff --git a/server/tests/test_auth_framework.py b/server/tests/test_auth_framework.py index c2cd288f..589e4a76 100644 --- a/server/tests/test_auth_framework.py +++ b/server/tests/test_auth_framework.py @@ -305,6 +305,7 @@ def factory(request: httpx.Request) -> httpx.Response: "extra_forward_headers": ("X-Deployer-Auth",), }, ) + assert provider.binding_target_authorization is True principal = await provider.resolve_identity( _build_request( headers={ @@ -339,6 +340,7 @@ def factory(request: httpx.Request) -> httpx.Response: factory, config_overrides={"identity_url": "https://identity.example/resolve"}, ) + assert provider.binding_target_authorization is True principal = await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) assert captured["url"] == "https://identity.example/resolve" @@ -346,8 +348,9 @@ def factory(request: httpx.Request) -> httpx.Response: @pytest.mark.asyncio -async def test_http_upstream_identity_requires_configured_url(): +async def test_http_upstream_direct_identity_lookup_requires_configured_url(): provider = _build_upstream(lambda request: pytest.fail("unexpected upstream call")) + assert provider.binding_target_authorization is False with pytest.raises(APIError) as exc_info: await provider.resolve_identity(_build_request(), Operation.CONTROL_BINDINGS_READ) diff --git a/server/tests/test_control_bindings_endpoints.py b/server/tests/test_control_bindings_endpoints.py index 116808e3..5220c88e 100644 --- a/server/tests/test_control_bindings_endpoints.py +++ b/server/tests/test_control_bindings_endpoints.py @@ -2,15 +2,22 @@ from __future__ import annotations +import json import uuid from typing import Any +import httpx import pytest from agent_control_models.errors import ErrorCode, ErrorReason from fastapi.testclient import TestClient from httpx import Response from agent_control_server.auth_framework import Operation, Principal, set_authorizer +from agent_control_server.auth_framework.providers import ( + HeaderAuthProvider, + HttpUpstreamAuthProvider, +) +from agent_control_server.auth_framework.providers.http_upstream import HttpUpstreamConfig from agent_control_server.errors import APIError, ForbiddenError, NotFoundError from agent_control_server.models import DEFAULT_NAMESPACE_KEY @@ -117,6 +124,8 @@ def test_binding_id_routes_authorize_the_stored_target(client: TestClient) -> No calls: list[tuple[str, Operation, dict[str, Any] | None]] = [] class RecordingAuthorizer: + binding_target_authorization = True + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: del request calls.append(("identity", operation, None)) @@ -211,6 +220,7 @@ def test_binding_id_lookup_is_scoped_before_target_authorization( method: str, client: TestClient ) -> None: class NamespaceAuthorizer: + binding_target_authorization = True namespace_key = "other-namespace" def __init__(self) -> None: @@ -269,6 +279,8 @@ def test_binding_id_identity_failure_does_not_depend_on_id( binding_id = _create_binding(client, control_id=control_id)["binding_id"] class UnavailableAuthorizer: + binding_target_authorization = True + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: del request, operation raise APIError( @@ -306,6 +318,8 @@ def test_binding_id_target_denial_looks_like_missing_binding( binding_id = _create_binding(client, control_id=control_id)["binding_id"] class DenyingAuthorizer: + binding_target_authorization = True + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: del request, operation return Principal(namespace_key=DEFAULT_NAMESPACE_KEY) @@ -342,7 +356,7 @@ async def authorize( ) -def test_binding_id_legacy_authorizer_preflights_before_lookup(client: TestClient) -> None: +def test_binding_id_legacy_authorizer_preserves_namespace_wide_check(client: TestClient) -> None: control_id = _create_control(client) binding_id = _create_binding(client, control_id=control_id)["binding_id"] contexts: list[dict[str, Any] | None] = [] @@ -364,9 +378,120 @@ async def authorize( # When: an ID-based binding read succeeds. response = client.get(f"{_BINDINGS_URL}/{binding_id}") - # Then: operation-wide auth runs before target auth. + # Then: the legacy provider receives the same single targetless check as before. assert response.status_code == 200 - assert contexts == [None, {"target_type": "env", "target_id": "prod"}] + assert contexts == [None] + + +@pytest.mark.parametrize( + ("method", "operation"), + [ + ("get", Operation.CONTROL_BINDINGS_READ), + ("patch", Operation.CONTROL_BINDINGS_WRITE), + ("delete", Operation.CONTROL_BINDINGS_WRITE), + ], +) +@pytest.mark.parametrize( + ("namespace_key", "expected_status"), + [(DEFAULT_NAMESPACE_KEY, 200), ("other-namespace", 404)], +) +def test_binding_id_non_orbit_http_upstream_preserves_targetless_check( + method: str, + operation: Operation, + namespace_key: str, + expected_status: int, + client: TestClient, +) -> None: + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + upstream_requests: list[httpx.Request] = [] + + def upstream_auth(request: httpx.Request) -> Response: + upstream_requests.append(request) + if "context" in json.loads(request.content): + # This upstream accepted the old namespace-wide contract only. + return Response(400, json={"detail": "Unexpected target context"}) + return Response(200, json={"namespace_key": namespace_key}) + + upstream_client = httpx.AsyncClient(transport=httpx.MockTransport(upstream_auth)) + set_authorizer( + HttpUpstreamAuthProvider( + HttpUpstreamConfig(url="https://custom.example/check"), + client=upstream_client, + ) + ) + + url = f"{_BINDINGS_URL}/{binding_id}" + if method == "get": + response = client.get(url) + elif method == "patch": + response = client.patch(url, json={"enabled": False}) + else: + response = client.delete(url) + + assert response.status_code == expected_status, response.text + assert len(upstream_requests) == 1 + assert str(upstream_requests[0].url) == "https://custom.example/check" + assert upstream_requests[0].method == "POST" + assert upstream_requests[0].headers["x-api-key"] == client.headers["x-api-key"] + assert json.loads(upstream_requests[0].content) == {"operation": operation.value} + if expected_status == 404: + assert response.json()["error_code"] == "CONTROL_BINDING_NOT_FOUND" + elif method == "get": + assert response.json()["namespace_key"] == DEFAULT_NAMESPACE_KEY + else: + assert response.json()["success"] is True + + set_authorizer(HeaderAuthProvider()) + stored = client.get(url) + if method == "delete" and expected_status == 200: + assert stored.status_code == 404 + else: + assert stored.status_code == 200 + assert stored.json()["enabled"] is (method != "patch" or expected_status != 200) + + +def test_binding_id_orbit_http_upstream_authorizes_stored_target(client: TestClient) -> None: + # Given: Orbit requires target context for management authorization. + control_id = _create_control(client) + target_id = str(uuid.uuid4()) + binding_id = _create_binding( + client, control_id=control_id, target_type="log_stream", target_id=target_id + )["binding_id"] + upstream_requests: list[httpx.Request] = [] + + def upstream_auth(request: httpx.Request) -> Response: + upstream_requests.append(request) + if request.url.path == "/internal/auth/resolve_tenant_context": + return Response(200, json={"namespace_key": DEFAULT_NAMESPACE_KEY}) + body = json.loads(request.content) + if body.get("context") != {"target_type": "log_stream", "target_id": target_id}: + return Response(400, json={"detail": "Target context required"}) + return Response(200, json={"namespace_key": DEFAULT_NAMESPACE_KEY}) + + upstream_client = httpx.AsyncClient(transport=httpx.MockTransport(upstream_auth)) + set_authorizer( + HttpUpstreamAuthProvider( + HttpUpstreamConfig( + url="https://orbit.example/internal/auth/agent_control/check_management_access" + ), + client=upstream_client, + ) + ) + + # When: an ID-based delete is requested without a target in the request. + response = client.delete(f"{_BINDINGS_URL}/{binding_id}") + + # Then: identity resolution precedes a target-bound management check. + assert response.status_code == 200, response.text + assert [request.url.path for request in upstream_requests] == [ + "/internal/auth/resolve_tenant_context", + "/internal/auth/agent_control/check_management_access", + ] + assert json.loads(upstream_requests[1].content) == { + "operation": Operation.CONTROL_BINDINGS_WRITE.value, + "context": {"target_type": "log_stream", "target_id": target_id}, + } def test_binding_id_delete_rechecks_the_authorized_namespace( @@ -378,6 +503,8 @@ def test_binding_id_delete_rechecks_the_authorized_namespace( authorized_namespace = "other-namespace" class NamespaceAuthorizer: + binding_target_authorization = True + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: del request, operation return Principal(namespace_key=authorized_namespace, is_admin=True) @@ -412,6 +539,8 @@ def test_binding_id_patch_rechecks_the_authorized_namespace( authorized_namespace = "other-namespace" class NamespaceAuthorizer: + binding_target_authorization = True + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: del request, operation return Principal(namespace_key=authorized_namespace, is_admin=True) @@ -448,6 +577,8 @@ def test_binding_id_path_validation_runs_before_authorization( calls: list[Operation] = [] class RecordingAuthorizer: + binding_target_authorization = True + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: del request calls.append(operation) From fe9b7d3670554b7e37c433fe87b828043970dc52 Mon Sep 17 00:00:00 2001 From: Joshua Jeon Date: Wed, 30 Sep 2026 15:47:24 -0700 Subject: [PATCH 3/4] refactor(server): isolate Orbit identity URL selection --- .../auth_framework/config.py | 20 +++++++- .../auth_framework/providers/http_upstream.py | 26 +++------- server/tests/test_auth_framework.py | 50 +++++++++++++++++-- .../tests/test_control_bindings_endpoints.py | 3 +- 4 files changed, 73 insertions(+), 26 deletions(-) diff --git a/server/src/agent_control_server/auth_framework/config.py b/server/src/agent_control_server/auth_framework/config.py index 275c5066..1d1260f6 100644 --- a/server/src/agent_control_server/auth_framework/config.py +++ b/server/src/agent_control_server/auth_framework/config.py @@ -32,6 +32,7 @@ import os import ssl from dataclasses import dataclass +from urllib.parse import urlsplit, urlunsplit from ..config import auth_settings from ..logging_utils import get_logger @@ -64,6 +65,8 @@ _UPSTREAM_MAX_KEEPALIVE_CONNECTIONS_ENV = ( "AGENT_CONTROL_AUTH_UPSTREAM_MAX_KEEPALIVE_CONNECTIONS" ) +_ORBIT_MANAGEMENT_PATH = "/internal/auth/agent_control/check_management_access" +_ORBIT_IDENTITY_PATH = "/internal/auth/resolve_tenant_context" # Runtime flow. _RUNTIME_MODE_ENV = "AGENT_CONTROL_RUNTIME_AUTH_MODE" @@ -246,10 +249,14 @@ def _build_default_provider() -> RequestAuthorizer: max_keepalive_connections=max_keepalive_connections, ) _logger.info("Default auth provider: http_upstream url=%s", url) + # Only the known Orbit management route has a predictable identity route. + # Other upstreams keep their existing namespace-wide authorization flow. + explicit_identity_url = (os.environ.get(_UPSTREAM_IDENTITY_URL_ENV) or "").strip() + identity_url = explicit_identity_url or _derive_orbit_identity_url(url) try: upstream_config = HttpUpstreamConfig( url=url, - identity_url=(os.environ.get(_UPSTREAM_IDENTITY_URL_ENV) or "").strip() or None, + identity_url=identity_url, timeout_seconds=timeout, service_token=token, service_token_header=token_header, @@ -277,6 +284,17 @@ def _build_default_provider() -> RequestAuthorizer: ) +def _derive_orbit_identity_url(url: str) -> str | None: + """Resolve Orbit's existing identity route from its management route.""" + parts = urlsplit(url) + if not parts.path.endswith(_ORBIT_MANAGEMENT_PATH): + return None + prefix = parts.path[: -len(_ORBIT_MANAGEMENT_PATH)] + return urlunsplit( + (parts.scheme, parts.netloc, prefix + _ORBIT_IDENTITY_PATH, parts.query, parts.fragment) + ) + + def _validate_local_api_key_mode(mode_env: str = _MODE_ENV) -> None: """Fail startup when local API-key mode has no local key validator.""" if not auth_settings.api_key_enabled: diff --git a/server/src/agent_control_server/auth_framework/providers/http_upstream.py b/server/src/agent_control_server/auth_framework/providers/http_upstream.py index 8423250f..c2a18b4f 100644 --- a/server/src/agent_control_server/auth_framework/providers/http_upstream.py +++ b/server/src/agent_control_server/auth_framework/providers/http_upstream.py @@ -46,7 +46,6 @@ from datetime import datetime from time import perf_counter from typing import Any -from urllib.parse import urlsplit, urlunsplit import httpx from agent_control_models.errors import ErrorCode, ErrorReason @@ -68,9 +67,8 @@ _logger = get_logger(__name__) _DEFAULT_FORWARDED_HEADERS = ("X-API-Key", "Authorization", "Cookie") -_ORBIT_MANAGEMENT_PATH = "/internal/auth/agent_control/check_management_access" -_ORBIT_IDENTITY_PATH = "/internal/auth/resolve_tenant_context" -_IDENTITY_OPERATION = "identity.resolve" +# Diagnostic label only; the identity POST has no operation payload. +_IDENTITY_LOOKUP_LABEL = "identity.resolve" _AUTH_UPSTREAM_ATTEMPTS = Counter( "agent_control_server_auth_upstream_attempts_total", @@ -185,8 +183,7 @@ class HttpUpstreamConfig: """Maximum idle connections retained for the auth upstream.""" identity_url: str | None = None - """URL for credential and namespace resolution. If omitted, the Orbit - identity URL is derived from the known management authorization path.""" + """Optional URL for credential and namespace resolution before target authorization.""" def __post_init__(self) -> None: if self.keepalive_expiry_seconds < 0: @@ -221,7 +218,7 @@ def __init__( client: httpx.AsyncClient | None = None, ) -> None: self._config = config - self._identity_url = config.identity_url or _derive_orbit_identity_url(config.url) + self._identity_url = config.identity_url self._owns_client = client is None if client is not None: self._client = client @@ -274,7 +271,7 @@ async def resolve_identity(self, request: Request, operation: Operation) -> Prin hint="Set AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL.", ) response = await self._post_upstream( - _IDENTITY_OPERATION, + _IDENTITY_LOOKUP_LABEL, None, self._forward_headers(request), url=self._identity_url, @@ -287,7 +284,7 @@ async def resolve_identity(self, request: Request, operation: Operation) -> Prin detail="Authorization identity endpoint was not found.", hint="Check the configured authorization identity URL.", ) - principal = self._handle_response(response, _IDENTITY_OPERATION, None) + principal = self._handle_response(response, _IDENTITY_LOOKUP_LABEL, None) return Principal(namespace_key=principal.namespace_key, caller_id=principal.caller_id) async def _post_upstream( @@ -493,17 +490,6 @@ def _authorization_service_unavailable_error() -> APIError: ) -def _derive_orbit_identity_url(url: str) -> str | None: - """Resolve Orbit's existing identity route from its management route.""" - parts = urlsplit(url) - if not parts.path.endswith(_ORBIT_MANAGEMENT_PATH): - return None - prefix = parts.path[: -len(_ORBIT_MANAGEMENT_PATH)] - return urlunsplit( - (parts.scheme, parts.netloc, prefix + _ORBIT_IDENTITY_PATH, parts.query, parts.fragment) - ) - - def _ensure_target_context_matches_grant( context: dict[str, Any] | None, principal: Principal, diff --git a/server/tests/test_auth_framework.py b/server/tests/test_auth_framework.py index 589e4a76..eee6789a 100644 --- a/server/tests/test_auth_framework.py +++ b/server/tests/test_auth_framework.py @@ -280,7 +280,7 @@ def factory(request: httpx.Request) -> httpx.Response: @pytest.mark.asyncio -async def test_http_upstream_resolves_identity_at_derived_orbit_url(): +async def test_http_upstream_resolves_identity_at_configured_url(): captured: dict[str, Any] = {} def factory(request: httpx.Request) -> httpx.Response: @@ -300,7 +300,10 @@ def factory(request: httpx.Request) -> httpx.Response: provider = _build_upstream( factory, config_overrides={ - "url": ("https://orbit.example/prefix/internal/auth/agent_control/check_management_access"), + "url": ( + "https://orbit.example/prefix/internal/auth/agent_control/check_management_access" + ), + "identity_url": "https://orbit.example/prefix/internal/auth/resolve_tenant_context", "service_token": "service-secret", "extra_forward_headers": ("X-Deployer-Auth",), }, @@ -1671,13 +1674,51 @@ async def test_configure_http_upstream_extra_forward_headers_env(monkeypatch): await auth_config.teardown_auth() +@pytest.mark.parametrize( + ("upstream_url", "expected_identity_url"), + [ + ( + "https://orbit.example/prefix/internal/auth/agent_control/check_management_access", + "https://orbit.example/prefix/internal/auth/resolve_tenant_context", + ), + ("https://auth.example.test/check", None), + ], +) @pytest.mark.asyncio -async def test_configure_http_upstream_identity_url_env(monkeypatch): +async def test_configure_http_upstream_selects_identity_url( + monkeypatch, upstream_url: str, expected_identity_url: str | None +): from agent_control_server.auth_framework import config as auth_config clear_authorizers() monkeypatch.setenv("AGENT_CONTROL_AUTH_MODE", "http_upstream") - monkeypatch.setenv("AGENT_CONTROL_AUTH_UPSTREAM_URL", "https://auth.example.test/check") + monkeypatch.setenv("AGENT_CONTROL_AUTH_UPSTREAM_URL", upstream_url) + monkeypatch.delenv("AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL", raising=False) + + try: + auth_config.configure_auth_from_env() + provider = get_authorizer(Operation.CONTROL_BINDINGS_READ) + assert isinstance(provider, HttpUpstreamAuthProvider) + assert provider._config.identity_url == expected_identity_url + assert provider.binding_target_authorization is (expected_identity_url is not None) + finally: + await auth_config.teardown_auth() + + +@pytest.mark.parametrize( + "upstream_url", + [ + "https://auth.example.test/check", + "https://orbit.example/internal/auth/agent_control/check_management_access", + ], +) +@pytest.mark.asyncio +async def test_configure_http_upstream_identity_url_env(monkeypatch, upstream_url: str): + from agent_control_server.auth_framework import config as auth_config + + clear_authorizers() + monkeypatch.setenv("AGENT_CONTROL_AUTH_MODE", "http_upstream") + monkeypatch.setenv("AGENT_CONTROL_AUTH_UPSTREAM_URL", upstream_url) monkeypatch.setenv( "AGENT_CONTROL_AUTH_UPSTREAM_IDENTITY_URL", " https://identity.example.test/resolve ", @@ -1688,6 +1729,7 @@ async def test_configure_http_upstream_identity_url_env(monkeypatch): provider = get_authorizer(Operation.CONTROL_BINDINGS_READ) assert isinstance(provider, HttpUpstreamAuthProvider) assert provider._config.identity_url == "https://identity.example.test/resolve" + assert provider.binding_target_authorization is True finally: await auth_config.teardown_auth() diff --git a/server/tests/test_control_bindings_endpoints.py b/server/tests/test_control_bindings_endpoints.py index 5220c88e..c990f68f 100644 --- a/server/tests/test_control_bindings_endpoints.py +++ b/server/tests/test_control_bindings_endpoints.py @@ -473,7 +473,8 @@ def upstream_auth(request: httpx.Request) -> Response: set_authorizer( HttpUpstreamAuthProvider( HttpUpstreamConfig( - url="https://orbit.example/internal/auth/agent_control/check_management_access" + url="https://orbit.example/internal/auth/agent_control/check_management_access", + identity_url="https://orbit.example/internal/auth/resolve_tenant_context", ), client=upstream_client, ) From 5fb016b16a9528e9a707b0d589aa52c2fc5ef85e Mon Sep 17 00:00:00 2001 From: Joshua Jeon Date: Wed, 30 Sep 2026 19:22:14 -0700 Subject: [PATCH 4/4] fix(server): reject binding auth namespace mismatches --- .../endpoints/control_bindings.py | 6 +- .../tests/test_control_bindings_endpoints.py | 55 ++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/server/src/agent_control_server/endpoints/control_bindings.py b/server/src/agent_control_server/endpoints/control_bindings.py index 09ba983c..93fc2d82 100644 --- a/server/src/agent_control_server/endpoints/control_bindings.py +++ b/server/src/agent_control_server/endpoints/control_bindings.py @@ -102,11 +102,15 @@ async def dependency(request: Request, binding_id: int) -> Principal: context = {"target_type": target_type, "target_id": target_id} try: - return await authorizer.authorize(request, operation, context) + principal = await authorizer.authorize(request, operation, context) except (ForbiddenError, NotFoundError) as exc: # A caller cannot distinguish a binding it cannot access from a # missing binding in its namespace. raise ControlBindingsService.binding_not_found(binding_id) from exc + if principal.namespace_key != identity.namespace_key: + # The target grant must apply to the namespace used for the lookup. + raise ControlBindingsService.binding_not_found(binding_id) + return principal return dependency diff --git a/server/tests/test_control_bindings_endpoints.py b/server/tests/test_control_bindings_endpoints.py index c990f68f..8ebb386f 100644 --- a/server/tests/test_control_bindings_endpoints.py +++ b/server/tests/test_control_bindings_endpoints.py @@ -9,9 +9,6 @@ import httpx import pytest from agent_control_models.errors import ErrorCode, ErrorReason -from fastapi.testclient import TestClient -from httpx import Response - from agent_control_server.auth_framework import Operation, Principal, set_authorizer from agent_control_server.auth_framework.providers import ( HeaderAuthProvider, @@ -20,6 +17,8 @@ from agent_control_server.auth_framework.providers.http_upstream import HttpUpstreamConfig from agent_control_server.errors import APIError, ForbiddenError, NotFoundError from agent_control_server.models import DEFAULT_NAMESPACE_KEY +from fastapi.testclient import TestClient +from httpx import Response from .utils import VALID_CONTROL_PAYLOAD @@ -356,6 +355,56 @@ async def authorize( ) +@pytest.mark.parametrize("method", ["get", "patch", "delete"]) +def test_binding_id_rejects_namespace_changed_by_target_authorization( + method: str, client: TestClient +) -> None: + # Given: identity resolves to the binding namespace, but target auth changes it. + control_id = _create_control(client) + binding_id = _create_binding(client, control_id=control_id)["binding_id"] + + class MismatchedNamespaceAuthorizer: + binding_target_authorization = True + authorization_namespace = "other-namespace" + + async def resolve_identity(self, request: Any, operation: Operation) -> Principal: + del request, operation + return Principal(namespace_key=DEFAULT_NAMESPACE_KEY) + + async def authorize( + self, + request: Any, + operation: Operation, + context: dict[str, Any] | None = None, + ) -> Principal: + del request, operation, context + return Principal(namespace_key=self.authorization_namespace) + + authorizer = MismatchedNamespaceAuthorizer() + set_authorizer(authorizer) + + # When: the caller reads or mutates the binding by ID. + url = f"{_BINDINGS_URL}/{binding_id}" + if method == "get": + response = client.get(url) + elif method == "patch": + response = client.patch(url, json={"enabled": False}) + else: + response = client.delete(url) + + # Then: it looks missing, and PATCH/DELETE leave the row untouched. + missing = client.get(f"{_BINDINGS_URL}/999999") + assert response.status_code == missing.status_code == 404 + assert response.json()["error_code"] == missing.json()["error_code"] == ( + "CONTROL_BINDING_NOT_FOUND" + ) + + authorizer.authorization_namespace = DEFAULT_NAMESPACE_KEY + stored = client.get(url) + assert stored.status_code == 200, stored.text + assert stored.json()["enabled"] is True + + def test_binding_id_legacy_authorizer_preserves_namespace_wide_check(client: TestClient) -> None: control_id = _create_control(client) binding_id = _create_binding(client, control_id=control_id)["binding_id"]