diff --git a/agentex/src/api/routes/agent_api_keys.py b/agentex/src/api/routes/agent_api_keys.py index e041531f..527fc9c1 100644 --- a/agentex/src/api/routes/agent_api_keys.py +++ b/agentex/src/api/routes/agent_api_keys.py @@ -7,10 +7,20 @@ CreateAPIKeyRequest, CreateAPIKeyResponse, ) +from src.api.schemas.authorization_types import ( + AgentexResource, + AgentexResourceType, + AuthorizedOperationType, +) from src.domain.entities.agent_api_keys import AgentAPIKeyType from src.domain.services.authorization_service import DAuthorizationService from src.domain.use_cases.agent_api_keys_use_case import DAgentAPIKeysUseCase from src.domain.use_cases.agents_use_case import DAgentsUseCase +from src.utils.agent_api_key_authorization import _check_api_key_or_collapse_to_404 +from src.utils.authorization_shortcuts import ( + DAuthorizedId, + DAuthorizedResourceIds, +) from src.utils.logging import make_logger logger = make_logger(__name__) @@ -42,6 +52,16 @@ async def create_api_key( detail="Only one of 'agent_id' or 'agent_name' should be provided to create an agent api_key.", ) agent = await agent_use_case.get(id=request.agent_id, name=request.agent_name) + + # Parent-agent FGAC: only callers with ``update`` on the parent agent may + # mint a new api_key under it. The new api_key has no resource row yet, so + # this is the only enforcement surface at create time. SpiceDB cannot + # transitively gate on a not-yet-created child. + await authorization_service.check( + resource=AgentexResource.agent(agent.id), + operation=AuthorizedOperationType.update, + ) + # Check if external agent API key already exists for this name and agent ID existing_api_key = await agent_api_key_use_case.get_by_agent_id_and_name( agent_id=agent.id, @@ -81,6 +101,9 @@ async def create_api_key( async def list_agent_api_keys( agent_api_key_use_case: DAgentAPIKeysUseCase, agent_use_case: DAgentsUseCase, + authorized_api_key_ids: DAuthorizedResourceIds( + AgentexResourceType.api_key, AuthorizedOperationType.read + ), agent_id: str | None = None, agent_name: str | None = None, limit: int = Query(default=50, ge=1, le=1000), @@ -100,6 +123,11 @@ async def list_agent_api_keys( agent_api_key_entities = await agent_api_key_use_case.list( agent_id=agent.id, limit=limit, page_number=page_number ) + # Filter to the set of api_keys the caller can read. ``None`` means the + # authz backend declined to enumerate (e.g. bypass) — pass through. + if authorized_api_key_ids is not None: + allowed = set(authorized_api_key_ids) + agent_api_key_entities = [e for e in agent_api_key_entities if e.id in allowed] return [ AgentAPIKey.model_validate(agent_api_key_entity) for agent_api_key_entity in agent_api_key_entities @@ -116,6 +144,7 @@ async def get_agent_api_key_by_name( name: str, agent_api_key_use_case: DAgentAPIKeysUseCase, agent_use_case: DAgentsUseCase, + authorization_service: DAuthorizationService, agent_id: str | None = None, agent_name: str | None = None, api_key_type: AgentAPIKeyType = AgentAPIKeyType.EXTERNAL, @@ -139,6 +168,15 @@ async def get_agent_api_key_by_name( status_code=404, detail=f"Agent api_key '{name}' not found for agent ID {agent.id}", ) + # Name routes for api_key don't fit ``DAuthorizedName`` (the lookup key is + # ``(agent_id, name, api_key_type)``, not a single globally-unique name + # path param). Apply the collapse helper explicitly so present-but-denied + # surfaces as 404, mirroring tasks' name route in PR #249. + await _check_api_key_or_collapse_to_404( + authorization_service, + agent_api_key_entity.id, + AuthorizedOperationType.read, + ) return AgentAPIKey.model_validate(agent_api_key_entity) @@ -151,6 +189,11 @@ async def get_agent_api_key_by_name( async def get_agent_api_key( id: str, agent_api_key_use_case: DAgentAPIKeysUseCase, + _authorized_id: DAuthorizedId( + AgentexResourceType.api_key, + AuthorizedOperationType.read, + param_name="id", + ), ) -> AgentAPIKey: agent_api_key_entity = await agent_api_key_use_case.get(id=id) return AgentAPIKey.model_validate(agent_api_key_entity) @@ -166,7 +209,18 @@ async def delete_agent_api_key( id: str, agent_api_key_use_case: DAgentAPIKeysUseCase, authorization_service: DAuthorizationService, + _authorized_id: DAuthorizedId( + AgentexResourceType.api_key, + AuthorizedOperationType.delete, + param_name="id", + ), ) -> str: + # Two-factor mutation: SpiceDB's ``api_key.delete`` permission depends + # transitively on ``parent_agent->update`` per the schema, so the + # ``DAuthorizedId(..., delete)`` dep above enforces both api_key.delete + # and the parent-agent.update factor. No explicit second + # ``authorization_service.check`` on the parent agent is needed (matches + # Asher's PR #249 approach for tasks). account_id = getattr(authorization_service.principal_context, "account_id", None) await agent_api_key_use_case.delete(id=id, account_id=account_id) return f"Agent API key with ID {id} deleted" @@ -198,6 +252,25 @@ async def delete_agent_api_key_by_name( detail="Only one of 'agent_id' or 'agent_name' should be provided to delete an agent api_key.", ) agent = await agent_use_case.get(id=agent_id, name=agent_name) + + # Resolve name -> id, then run the collapse-wrapped delete check before + # mutating. Two-factor (api_key.delete & parent_agent->update) is + # transitively enforced by the SpiceDB schema definition of + # ``api_key.delete``; we don't repeat a parent check here. + existing = await agent_api_key_use_case.get_by_agent_id_and_name( + agent_id=agent.id, name=api_key_name, api_key_type=api_key_type + ) + if not existing: + raise HTTPException( + status_code=404, + detail=f"Agent api_key '{api_key_name}' not found for agent ID {agent.id}", + ) + await _check_api_key_or_collapse_to_404( + authorization_service, + existing.id, + AuthorizedOperationType.delete, + ) + account_id = getattr(authorization_service.principal_context, "account_id", None) await agent_api_key_use_case.delete_by_agent_id_and_key_name( agent_id=agent.id, diff --git a/agentex/src/utils/agent_api_key_authorization.py b/agentex/src/utils/agent_api_key_authorization.py new file mode 100644 index 00000000..17d0b3d0 --- /dev/null +++ b/agentex/src/utils/agent_api_key_authorization.py @@ -0,0 +1,42 @@ +from src.adapters.authorization.exceptions import AuthorizationError +from src.adapters.crud_store.exceptions import ItemDoesNotExist +from src.api.schemas.authorization_types import ( + AgentexResource, + AuthorizedOperationType, +) + + +async def _check_api_key_or_collapse_to_404( + authorization, + api_key_id: str, + operation: AuthorizedOperationType, +) -> None: + """Issue a check on an api_key resource. On any denial, surface 404 — even + when the api_key exists. + + Same rationale as :func:`src.utils.task_authorization._check_task_or_collapse_to_404`: + the deny-reason discriminator needed to safely return 403 (in-tenant but + lacking the operation) vs 404 (cross-tenant or absent) is not available + here. ``api_key.name`` is unique only per (agent_id, name, api_key_type) — + not globally — but the existence-leak risk via the name routes is still + real: a caller probing ``GET /agent_api_keys/name/{name}?agent_id=...`` + against another tenant's agent would otherwise be able to distinguish + "agent has a key with that name (403)" from "no such key (404)". + + Until api_keys carry tenant scope at the data layer (or agentex-auth's + deny distinguishes "cross-tenant" from "in-tenant lacking permission"), + the safer default is to collapse both into 404. Trade-off: a user with + ``read`` but not ``delete`` permission on an in-tenant api_key sees 404 + on delete attempts instead of 403. UX regression for in-tenant permission + granularity, but eliminates the cross-tenant existence leak. + + TODO(AGX1-290): Restore the 403/404 split for same-tenant calls once + api_keys carry tenant/account_id scope at the data layer. + """ + try: + await authorization.check( + resource=AgentexResource.api_key(api_key_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist(f"Item with id '{api_key_id}' does not exist.") from None diff --git a/agentex/src/utils/authorization_shortcuts.py b/agentex/src/utils/authorization_shortcuts.py index 2f8296d6..fe1633e8 100644 --- a/agentex/src/utils/authorization_shortcuts.py +++ b/agentex/src/utils/authorization_shortcuts.py @@ -13,6 +13,7 @@ from src.domain.repositories.task_repository import DTaskRepository from src.domain.repositories.task_state_repository import DTaskStateRepository from src.domain.services.authorization_service import DAuthorizationService +from src.utils.agent_api_key_authorization import _check_api_key_or_collapse_to_404 async def _get_parent_task_id( @@ -55,6 +56,12 @@ async def _ensure_authorized_id( resource=AgentexResource.task(task_id), operation=operation, ) + elif resource_type == AgentexResourceType.api_key: + # Collapse api_key denials to 404 so name/id probes can't + # distinguish "present in another tenant" from "absent". + await _check_api_key_or_collapse_to_404( + authorization, resource_id, operation + ) else: # For direct resources, check directly await authorization.check( diff --git a/agentex/tests/unit/api/test_agent_api_keys_authz.py b/agentex/tests/unit/api/test_agent_api_keys_authz.py new file mode 100644 index 00000000..a58b0c36 --- /dev/null +++ b/agentex/tests/unit/api/test_agent_api_keys_authz.py @@ -0,0 +1,382 @@ +"""Tests for AGX1-263 — agent_api_keys route migration to Spark AuthZ. + +Mirrors the structure of ``test_tasks_authz.py`` (AGX1-275, PR #249). Covers: + + 1. The ``_check_api_key_or_collapse_to_404`` helper (allow + denied-collapses). + 2. ``DAuthorizedId`` for ``api_key`` routes the check through the collapse + wrap so denied id-path calls return 404, not 403. + 3. The name-route handlers (``get_agent_api_key_by_name`` / + ``delete_agent_api_key_by_name``) call the collapse helper explicitly so + present-but-denied surfaces as 404 (the critical no-existence-leak path). + 4. ``list_agent_api_keys`` filters to the set returned by + ``DAuthorizedResourceIds``. + 5. ``create_api_key`` enforces parent ``agent.update`` (the only route where + no api_key resource exists yet). + +Cross-tenant / two-factor-via-SpiceDB checks belong in an end-to-end suite +gated on a live spark-authz cluster (the schema's ``api_key.delete = +internal_effective_editor & parent_agent->update & internal_tenant_gate`` +expansion is owned by spark-authz, not this repo). Here we only assert that +the route layer issues the correct ``check`` call with the correct operation +and surfaces denials as 404. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock + +import pytest +from src.adapters.authorization.exceptions import AuthorizationError +from src.adapters.crud_store.exceptions import ItemDoesNotExist +from src.api.schemas.authorization_types import ( + AgentexResource, + AgentexResourceType, + AuthorizedOperationType, +) +from src.utils.agent_api_key_authorization import _check_api_key_or_collapse_to_404 +from src.utils.authorization_shortcuts import DAuthorizedId + + +def _dep_callable(annotation): + """Pull the inner FastAPI dependency function out of an ``Annotated[str, Depends(...)]``.""" + return annotation.__metadata__[0].dependency + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestCheckApiKeyOrCollapseTo404: + """The api_key-resource authz wrap collapses every denial to 404 so callers + can't distinguish "present in another tenant" from "absent" via the name or + id routes.""" + + async def test_allowed_check_returns_normally(self): + authorization = MagicMock() + authorization.check = AsyncMock(return_value=True) + + await _check_api_key_or_collapse_to_404( + authorization, + "api-key-1", + AuthorizedOperationType.read, + ) + + authorization.check.assert_awaited_once() + called_kwargs = authorization.check.await_args.kwargs + assert called_kwargs["resource"] == AgentexResource.api_key("api-key-1") + assert called_kwargs["operation"] == AuthorizedOperationType.read + + async def test_denied_collapses_to_not_found_regardless_of_existence(self): + """Both "denied + api_key exists" and "denied + api_key missing" + surface as ``ItemDoesNotExist`` (→ 404). The wrap doesn't consult any + repository — collapsing avoids the cross-tenant existence leak.""" + authorization = MagicMock() + authorization.check = AsyncMock(side_effect=AuthorizationError("denied")) + + with pytest.raises(ItemDoesNotExist): + await _check_api_key_or_collapse_to_404( + authorization, + "api-key-1", + AuthorizedOperationType.delete, + ) + + async def test_uses_delete_operation_on_delete_routes(self): + """Sanity-check that the helper forwards the operation verbatim — the + two-factor SpiceDB expansion for ``delete`` is what bundles in the + ``parent_agent->update`` factor.""" + authorization = MagicMock() + authorization.check = AsyncMock(return_value=True) + + await _check_api_key_or_collapse_to_404( + authorization, + "api-key-2", + AuthorizedOperationType.delete, + ) + + called_kwargs = authorization.check.await_args.kwargs + assert called_kwargs["operation"] == AuthorizedOperationType.delete + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestDAuthorizedIdApiKeyWrap: + """``DAuthorizedId`` for ``AgentexResourceType.api_key`` must route the + check through the collapse wrap so denied id-path calls surface as 404, + matching the name-route behavior.""" + + async def test_api_key_id_routes_through_wrap_on_denial(self): + annotation = DAuthorizedId( + AgentexResourceType.api_key, + AuthorizedOperationType.read, + param_name="id", + ) + dep = _dep_callable(annotation) + + authorization = MagicMock() + authorization.check = AsyncMock(side_effect=AuthorizationError("denied")) + event_repository = MagicMock() + state_repository = MagicMock() + + with pytest.raises(ItemDoesNotExist): + await dep( + authorization, + event_repository, + state_repository, + "api-key-7", + ) + + async def test_api_key_id_returns_resource_id_when_allowed(self): + annotation = DAuthorizedId( + AgentexResourceType.api_key, + AuthorizedOperationType.read, + param_name="id", + ) + dep = _dep_callable(annotation) + + authorization = MagicMock() + authorization.check = AsyncMock(return_value=True) + + result = await dep(authorization, MagicMock(), MagicMock(), "api-key-9") + + assert result == "api-key-9" + called_kwargs = authorization.check.await_args.kwargs + assert called_kwargs["resource"] == AgentexResource.api_key("api-key-9") + + async def test_api_key_delete_op_propagated_to_check(self): + """Two-factor mutations rely on SpiceDB's transitive expansion of + ``api_key.delete`` (which includes ``parent_agent->update``), so the + route layer just needs to forward the ``delete`` operation correctly.""" + annotation = DAuthorizedId( + AgentexResourceType.api_key, + AuthorizedOperationType.delete, + param_name="id", + ) + dep = _dep_callable(annotation) + + authorization = MagicMock() + authorization.check = AsyncMock(return_value=True) + + await dep(authorization, MagicMock(), MagicMock(), "api-key-del") + + called_kwargs = authorization.check.await_args.kwargs + assert called_kwargs["operation"] == AuthorizedOperationType.delete + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestNameRouteCollapse: + """The name-route handlers don't fit ``DAuthorizedName`` (the lookup is + ``(agent_id, name, api_key_type)``), so they call the collapse helper + inline. Tests verify that direct call collapses denials to 404.""" + + async def test_get_by_name_handler_collapses_denial_to_404(self): + from src.api.routes.agent_api_keys import get_agent_api_key_by_name + + agent_use_case = MagicMock() + agent_use_case.get = AsyncMock(return_value=MagicMock(id="agent-1")) + api_key_use_case = MagicMock() + api_key_use_case.get_by_agent_id_and_name = AsyncMock( + return_value=MagicMock(id="api-key-named", name="prod-key") + ) + authorization = MagicMock() + authorization.check = AsyncMock(side_effect=AuthorizationError("denied")) + + with pytest.raises(ItemDoesNotExist): + await get_agent_api_key_by_name( + name="prod-key", + agent_api_key_use_case=api_key_use_case, + agent_use_case=agent_use_case, + authorization_service=authorization, + agent_id="agent-1", + agent_name=None, + ) + + # The lookup happens BEFORE the authz check — name → id is resolved, + # then the wrap intercepts the denial. + api_key_use_case.get_by_agent_id_and_name.assert_awaited_once() + authorization.check.assert_awaited_once() + called_kwargs = authorization.check.await_args.kwargs + assert called_kwargs["resource"] == AgentexResource.api_key("api-key-named") + + async def test_delete_by_name_handler_collapses_denial_to_404(self): + from src.api.routes.agent_api_keys import delete_agent_api_key_by_name + + agent_use_case = MagicMock() + agent_use_case.get = AsyncMock(return_value=MagicMock(id="agent-1")) + api_key_use_case = MagicMock() + api_key_use_case.get_by_agent_id_and_name = AsyncMock( + return_value=MagicMock(id="api-key-named") + ) + api_key_use_case.delete_by_agent_id_and_key_name = AsyncMock() + authorization = MagicMock() + authorization.check = AsyncMock(side_effect=AuthorizationError("denied")) + authorization.principal_context = MagicMock(account_id="acct-1") + + with pytest.raises(ItemDoesNotExist): + await delete_agent_api_key_by_name( + api_key_name="prod-key", + agent_api_key_use_case=api_key_use_case, + agent_use_case=agent_use_case, + authorization_service=authorization, + agent_id="agent-1", + agent_name=None, + ) + + # Crucially: the delete is NOT invoked when the check fails. + api_key_use_case.delete_by_agent_id_and_key_name.assert_not_called() + called_kwargs = authorization.check.await_args.kwargs + assert called_kwargs["operation"] == AuthorizedOperationType.delete + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestListFiltering: + """``list_agent_api_keys`` must filter rows to those the caller has + ``read`` on, per the ``DAuthorizedResourceIds`` enumeration.""" + + async def test_filters_to_authorized_subset(self): + from src.api.routes.agent_api_keys import list_agent_api_keys + + agent_use_case = MagicMock() + agent_use_case.get = AsyncMock(return_value=MagicMock(id="agent-1")) + api_key_use_case = MagicMock() + api_key_use_case.list = AsyncMock( + return_value=[ + MagicMock(id="api-key-a"), + MagicMock(id="api-key-b"), + MagicMock(id="api-key-c"), + ] + ) + + # Patch model_validate to a passthrough so we can assert on ids. + import src.api.routes.agent_api_keys as routes_mod + + original = routes_mod.AgentAPIKey.model_validate + routes_mod.AgentAPIKey.model_validate = staticmethod(lambda e: e) + try: + result = await list_agent_api_keys( + agent_api_key_use_case=api_key_use_case, + agent_use_case=agent_use_case, + authorized_api_key_ids=["api-key-a", "api-key-c"], + agent_id="agent-1", + agent_name=None, + limit=50, + page_number=1, + ) + finally: + routes_mod.AgentAPIKey.model_validate = original + + result_ids = [r.id for r in result] + assert result_ids == ["api-key-a", "api-key-c"] + + async def test_none_authorized_ids_passes_through(self): + """``None`` from the authz backend = "couldn't enumerate" (e.g. bypass + mode). Must pass through unfiltered — not collapse to empty list.""" + from src.api.routes.agent_api_keys import list_agent_api_keys + + agent_use_case = MagicMock() + agent_use_case.get = AsyncMock(return_value=MagicMock(id="agent-1")) + api_key_use_case = MagicMock() + api_key_use_case.list = AsyncMock( + return_value=[MagicMock(id="api-key-a"), MagicMock(id="api-key-b")] + ) + + import src.api.routes.agent_api_keys as routes_mod + + original = routes_mod.AgentAPIKey.model_validate + routes_mod.AgentAPIKey.model_validate = staticmethod(lambda e: e) + try: + result = await list_agent_api_keys( + agent_api_key_use_case=api_key_use_case, + agent_use_case=agent_use_case, + authorized_api_key_ids=None, + agent_id="agent-1", + agent_name=None, + limit=50, + page_number=1, + ) + finally: + routes_mod.AgentAPIKey.model_validate = original + + assert len(result) == 2 + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestCreateParentAgentCheck: + """``create_api_key`` is the only route where no api_key resource exists + yet, so SpiceDB cannot transitively gate on it. The route MUST explicitly + check ``agent.update`` on the parent.""" + + async def test_create_checks_parent_agent_update(self): + from src.api.routes.agent_api_keys import create_api_key + from src.api.schemas.agent_api_keys import CreateAPIKeyRequest + from src.domain.entities.agent_api_keys import AgentAPIKeyType + + agent_use_case = MagicMock() + agent_use_case.get = AsyncMock(return_value=MagicMock(id="agent-1")) + api_key_use_case = MagicMock() + api_key_use_case.get_by_agent_id_and_name = AsyncMock(return_value=None) + from datetime import datetime + + created_entity = MagicMock() + created_entity.id = "new-api-key" + created_entity.agent_id = "agent-1" + created_entity.created_at = datetime(2026, 1, 1) + created_entity.name = "prod-key" + created_entity.api_key_type = AgentAPIKeyType.EXTERNAL + api_key_use_case.create = AsyncMock(return_value=created_entity) + authorization = MagicMock() + authorization.check = AsyncMock(return_value=True) + authorization.principal_context = MagicMock(account_id="acct-1") + + request = CreateAPIKeyRequest( + agent_id="agent-1", + agent_name=None, + name="prod-key", + api_key_type=AgentAPIKeyType.EXTERNAL, + api_key="secret-key-value", + ) + + await create_api_key( + request=request, + agent_api_key_use_case=api_key_use_case, + agent_use_case=agent_use_case, + authorization_service=authorization, + ) + + authorization.check.assert_awaited_once() + called_kwargs = authorization.check.await_args.kwargs + assert called_kwargs["resource"] == AgentexResource.agent("agent-1") + assert called_kwargs["operation"] == AuthorizedOperationType.update + + async def test_create_denied_on_parent_agent_propagates_403(self): + """Create-time denial bubbles out as ``AuthorizationError`` (→ 403), + NOT collapsed to 404 — there is no api_key resource yet whose + existence could be leaked. Mirrors agent-side denial UX.""" + from src.api.routes.agent_api_keys import create_api_key + from src.api.schemas.agent_api_keys import CreateAPIKeyRequest + from src.domain.entities.agent_api_keys import AgentAPIKeyType + + agent_use_case = MagicMock() + agent_use_case.get = AsyncMock(return_value=MagicMock(id="agent-1")) + api_key_use_case = MagicMock() + api_key_use_case.create = AsyncMock() + authorization = MagicMock() + authorization.check = AsyncMock(side_effect=AuthorizationError("denied")) + + request = CreateAPIKeyRequest( + agent_id="agent-1", + agent_name=None, + name="prod-key", + api_key_type=AgentAPIKeyType.EXTERNAL, + api_key="x", + ) + + with pytest.raises(AuthorizationError): + await create_api_key( + request=request, + agent_api_key_use_case=api_key_use_case, + agent_use_case=agent_use_case, + authorization_service=authorization, + ) + # Create is NOT invoked when parent check fails. + api_key_use_case.create.assert_not_called()