From 683f1f306039abbc317eab21b05eb4835a8d63f2 Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 13:02:28 -0700 Subject: [PATCH 1/8] feat(AGX1-244): enforce parent-agent authz on event routes --- agentex/src/api/routes/events.py | 4 +- .../src/api/schemas/authorization_types.py | 8 +- agentex/src/utils/authorization_shortcuts.py | 86 ++++-- .../api/events/test_events_authz_api.py | 251 ++++++++++++++++++ 4 files changed, 331 insertions(+), 18 deletions(-) create mode 100644 agentex/tests/integration/api/events/test_events_authz_api.py diff --git a/agentex/src/api/routes/events.py b/agentex/src/api/routes/events.py index 6da4999f..48c0d8e5 100644 --- a/agentex/src/api/routes/events.py +++ b/agentex/src/api/routes/events.py @@ -1,9 +1,9 @@ from fastapi import APIRouter, Query from src.api.schemas.authorization_types import ( + AgentChildResourceType, AgentexResourceType, AuthorizedOperationType, - TaskChildResourceType, ) from src.api.schemas.events import Event from src.domain.use_cases.events_use_case import DEventUseCase @@ -20,7 +20,7 @@ response_model=Event, ) async def get_event( - event_id: DAuthorizedId(TaskChildResourceType.event, AuthorizedOperationType.read), + event_id: DAuthorizedId(AgentChildResourceType.event, AuthorizedOperationType.read), event_use_case: DEventUseCase, ) -> Event: event_entity = await event_use_case.get(event_id) diff --git a/agentex/src/api/schemas/authorization_types.py b/agentex/src/api/schemas/authorization_types.py index 585fa3a3..45c97be8 100644 --- a/agentex/src/api/schemas/authorization_types.py +++ b/agentex/src/api/schemas/authorization_types.py @@ -20,10 +20,16 @@ class AgentexResourceType(StrEnum): class TaskChildResourceType(StrEnum): """Resources that inherit permissions from their parent task.""" - event = "event" state = "state" +# Resources that inherit permissions from their parent agent +class AgentChildResourceType(StrEnum): + """Resources that inherit permissions from their parent agent.""" + + event = "event" + + class AgentexResource(BaseModel): type: AgentexResourceType selector: str diff --git a/agentex/src/utils/authorization_shortcuts.py b/agentex/src/utils/authorization_shortcuts.py index 2f8296d6..1f0b518a 100644 --- a/agentex/src/utils/authorization_shortcuts.py +++ b/agentex/src/utils/authorization_shortcuts.py @@ -2,7 +2,10 @@ from fastapi import Depends, Path, Query, Request +from src.adapters.authorization.exceptions import AuthorizationError +from src.adapters.crud_store.exceptions import ItemDoesNotExist from src.api.schemas.authorization_types import ( + AgentChildResourceType, AgentexResource, AgentexResourceType, AuthorizedOperationType, @@ -18,13 +21,11 @@ async def _get_parent_task_id( resource_type: TaskChildResourceType, resource_id: str, - event_repository: DEventRepository, state_repository: DTaskStateRepository, ) -> str: - """Get the parent task ID for a child resource.""" + """Get the parent task ID for a task-child resource.""" registry = { TaskChildResourceType.state: state_repository, - TaskChildResourceType.event: event_repository, } repository = registry[resource_type] @@ -32,8 +33,23 @@ async def _get_parent_task_id( return resource.task_id +async def _get_parent_agent_id( + resource_type: AgentChildResourceType, + resource_id: str, + event_repository: DEventRepository, +) -> str: + """Get the parent agent ID for an agent-child resource.""" + registry = { + AgentChildResourceType.event: event_repository, + } + + repository = registry[resource_type] + resource = await repository.get(id=resource_id) + return resource.agent_id + + def DAuthorizedId( - resource_type: AgentexResourceType | TaskChildResourceType, + resource_type: AgentexResourceType | TaskChildResourceType | AgentChildResourceType, operation: AuthorizedOperationType = AuthorizedOperationType.read, param_name: str = None, ): @@ -46,15 +62,35 @@ async def _ensure_authorized_id( state_repository: DTaskStateRepository, resource_id: str = Path(..., alias=param_name), ) -> str: - # For child resources, check the parent task + # For child resources, check the parent. Collapse a denied check + # into 404 so callers cannot use 403 vs 404 to probe whether a resource + # exists in another tenant. if isinstance(resource_type, TaskChildResourceType): task_id = await _get_parent_task_id( - resource_type, resource_id, event_repository, state_repository + resource_type, resource_id, state_repository ) - await authorization.check( - resource=AgentexResource.task(task_id), - operation=operation, + try: + await authorization.check( + resource=AgentexResource.task(task_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None + elif isinstance(resource_type, AgentChildResourceType): + agent_id = await _get_parent_agent_id( + resource_type, resource_id, event_repository ) + try: + await authorization.check( + resource=AgentexResource.agent(agent_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None else: # For direct resources, check directly await authorization.check( @@ -67,7 +103,7 @@ async def _ensure_authorized_id( def DAuthorizedQuery( - resource_type: AgentexResourceType | TaskChildResourceType, + resource_type: AgentexResourceType | TaskChildResourceType | AgentChildResourceType, operation: AuthorizedOperationType = AuthorizedOperationType.read, param_name: str = None, description: str = None, @@ -83,15 +119,35 @@ async def _ensure_authorized_query( state_repository: DTaskStateRepository, resource_id: str = Query(..., alias=param_name, description=description), ) -> str: - # For child resources, check the parent task + # For child resources, check the parent. Collapse a denied check + # into 404 so callers cannot use 403 vs 404 to probe whether a resource + # exists in another tenant. if isinstance(resource_type, TaskChildResourceType): task_id = await _get_parent_task_id( - resource_type, resource_id, event_repository, state_repository + resource_type, resource_id, state_repository ) - await authorization.check( - resource=AgentexResource.task(task_id), - operation=operation, + try: + await authorization.check( + resource=AgentexResource.task(task_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None + elif isinstance(resource_type, AgentChildResourceType): + agent_id = await _get_parent_agent_id( + resource_type, resource_id, event_repository ) + try: + await authorization.check( + resource=AgentexResource.agent(agent_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None else: # For direct resources, check directly await authorization.check( diff --git a/agentex/tests/integration/api/events/test_events_authz_api.py b/agentex/tests/integration/api/events/test_events_authz_api.py new file mode 100644 index 00000000..6b8a0ea8 --- /dev/null +++ b/agentex/tests/integration/api/events/test_events_authz_api.py @@ -0,0 +1,251 @@ +""" +Integration tests for event-route authorization. + +Covers the AGX1-244 deliverable: list/get events enforce ``agent.read`` on +the parent agent (events delegate to the parent agent, not the task), with +denied checks collapsing to 404 (not 403) so callers cannot probe +cross-tenant existence by comparing response codes. +""" + +from typing import Any +from unittest.mock import patch + +import pytest +import pytest_asyncio +from src.adapters.authorization.exceptions import AuthorizationError +from src.api.schemas.authorization_types import AgentexResourceType +from src.domain.entities.agents import ACPType, AgentEntity +from src.domain.entities.task_messages import TextContentEntity +from src.domain.entities.tasks import TaskEntity, TaskStatus +from src.utils.ids import orm_id + +MOCK_PRINCIPAL_CONTEXT = { + "account_id": "test-account-id", + "user_id": "test-user-id", +} + + +def _mock_post_factory( + *, + deny_agent_ids: set[str] | None = None, + deny_task_ids: set[str] | None = None, +): + """Return a side_effect that allows authn + authz, except for agents/tasks + listed in ``deny_*_ids`` for which ``/v1/authz/check`` raises + AuthorizationError. + """ + deny_agent_ids = deny_agent_ids or set() + deny_task_ids = deny_task_ids or set() + + async def _side_effect( + base_url: str = "http://test.com", + path: str = "/test", + *, + json: dict | None = None, + headers: dict[str, str] | None = None, + ) -> dict[str, Any]: + if path == "/v1/authn": + return MOCK_PRINCIPAL_CONTEXT + if path == "/v1/authz/check": + assert json is not None + resource = json.get("resource", {}) + if ( + resource.get("type") == AgentexResourceType.agent.value + and resource.get("selector") in deny_agent_ids + ): + raise AuthorizationError("Denied by mock") + if ( + resource.get("type") == AgentexResourceType.task.value + and resource.get("selector") in deny_task_ids + ): + raise AuthorizationError("Denied by mock") + return {"allowed": True} + if path == "/v1/authz/search": + return {"items": []} + raise Exception(f"Unknown path: {path}") + + return _side_effect + + +@pytest.mark.integration +class TestEventsAuthzAPIIntegration: + """End-to-end integration tests for event-route authorization.""" + + @pytest_asyncio.fixture + async def test_agent(self, isolated_repositories): + agent_repo = isolated_repositories["agent_repository"] + agent = AgentEntity( + id=orm_id(), + name="test-authz-agent", + description="Agent for event-authz tests", + acp_url="http://test-acp:8000", + acp_type=ACPType.SYNC, + ) + return await agent_repo.create(agent) + + @pytest_asyncio.fixture + async def test_task(self, isolated_repositories, test_agent): + task_repo = isolated_repositories["task_repository"] + task = TaskEntity( + id=orm_id(), + name="test-authz-task", + status=TaskStatus.RUNNING, + status_reason="Task for event-authz tests", + ) + return await task_repo.create(agent_id=test_agent.id, task=task) + + @pytest_asyncio.fixture + async def test_event(self, isolated_repositories, test_agent, test_task): + event_repo = isolated_repositories["event_repository"] + content = TextContentEntity(type="text", author="user", content="hello") + return await event_repo.create( + id=orm_id(), + task_id=test_task.id, + agent_id=test_agent.id, + content=content, + ) + + @pytest.mark.asyncio + @patch( + "src.api.authentication_middleware.AgentexAuthMiddleware.is_enabled", + return_value=True, + ) + @patch( + "src.domain.services.authorization_service.AuthorizationService.is_enabled", + return_value=True, + ) + @patch( + "src.utils.http_request_handler.HttpRequestHandler.post_with_error_handling", + side_effect=_mock_post_factory(), + ) + async def test_get_event_authorized_returns_200( + self, + post_with_error_handling_mock, + is_enabled_authorization_mock, + is_enabled_mock, + isolated_client, + test_event, + test_agent, + ): + response = await isolated_client.get(f"/events/{test_event.id}") + assert response.status_code == 200 + assert response.json()["id"] == test_event.id + + # Exactly one /v1/authz/check, on the parent agent with the read + # operation. Events delegate to the parent agent, not the parent task. + check_calls = [ + call + for call in post_with_error_handling_mock.call_args_list + if call[0][1] == "/v1/authz/check" + ] + assert len(check_calls) == 1 + authz_data = check_calls[0][1]["json"] + assert authz_data["resource"]["type"] == AgentexResourceType.agent.value + assert authz_data["resource"]["selector"] == test_agent.id + assert authz_data["operation"] == "read" + assert authz_data["principal"] == MOCK_PRINCIPAL_CONTEXT + + @pytest.mark.asyncio + @patch( + "src.api.authentication_middleware.AgentexAuthMiddleware.is_enabled", + return_value=True, + ) + @patch( + "src.domain.services.authorization_service.AuthorizationService.is_enabled", + return_value=True, + ) + async def test_get_event_unauthorized_returns_404( + self, + is_enabled_authorization_mock, + is_enabled_mock, + isolated_client, + test_event, + test_agent, + ): + with patch( + "src.utils.http_request_handler.HttpRequestHandler.post_with_error_handling", + side_effect=_mock_post_factory(deny_agent_ids={test_agent.id}), + ): + response = await isolated_client.get(f"/events/{test_event.id}") + # Denial on the parent agent must collapse to 404, not surface as 403. + assert response.status_code == 404 + + @pytest.mark.asyncio + @patch( + "src.api.authentication_middleware.AgentexAuthMiddleware.is_enabled", + return_value=True, + ) + @patch( + "src.domain.services.authorization_service.AuthorizationService.is_enabled", + return_value=True, + ) + @patch( + "src.utils.http_request_handler.HttpRequestHandler.post_with_error_handling", + side_effect=_mock_post_factory(), + ) + async def test_get_event_nonexistent_returns_404( + self, + post_with_error_handling_mock, + is_enabled_authorization_mock, + is_enabled_mock, + isolated_client, + ): + response = await isolated_client.get(f"/events/{orm_id()}") + assert response.status_code == 404 + # The parent-agent lookup raises ItemDoesNotExist before any authz + # check fires, so /v1/authz/check should never be called. + assert not any( + call[0][1] == "/v1/authz/check" + for call in post_with_error_handling_mock.call_args_list + ) + + @pytest.mark.asyncio + @patch( + "src.api.authentication_middleware.AgentexAuthMiddleware.is_enabled", + return_value=True, + ) + @patch( + "src.domain.services.authorization_service.AuthorizationService.is_enabled", + return_value=True, + ) + @patch( + "src.utils.http_request_handler.HttpRequestHandler.post_with_error_handling", + side_effect=_mock_post_factory(), + ) + async def test_list_events_authorized_returns_200( + self, + post_with_error_handling_mock, + is_enabled_authorization_mock, + is_enabled_mock, + isolated_client, + test_event, + test_agent, + test_task, + ): + response = await isolated_client.get( + f"/events?task_id={test_task.id}&agent_id={test_agent.id}" + ) + assert response.status_code == 200 + body = response.json() + assert any(e["id"] == test_event.id for e in body) + + # The list endpoint takes both task_id and agent_id as query params and + # each has its own DAuthorizedQuery — so we expect two authz checks: + # one for the task (direct) and one for the agent (direct). + check_calls = [ + call + for call in post_with_error_handling_mock.call_args_list + if call[0][1] == "/v1/authz/check" + ] + assert len(check_calls) == 2 + checked = { + ( + call[1]["json"]["resource"]["type"], + call[1]["json"]["resource"]["selector"], + ) + for call in check_calls + } + assert (AgentexResourceType.task.value, test_task.id) in checked + assert (AgentexResourceType.agent.value, test_agent.id) in checked + for call in check_calls: + assert call[1]["json"]["operation"] == "read" From 0a9eef1073c8b3fb962855ba32042132f749c90f Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 13:24:14 -0700 Subject: [PATCH 2/8] test(AGX1-244): add denied-list case to events authz suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per the ticket's list-events deliverable ("user without agent view gets ... an empty/filtered list"), our query-param-gated list endpoint collapses denied agent queries to 404 via DAuthorizedQuery. The list endpoint requires both task_id and agent_id query params, so denying view on the requested agent_id means the list call itself returns 404 — there's no broader unfiltered list shape to filter via list_accessible_resources. Test asserts: caller with deny_agent_ids={test_agent.id} gets 404 on GET /events?task_id=...&agent_id=. Closes the test matrix — authorized-get + denied-get + nonexistent-get + authorized-list + denied-list now all covered (5/5). Collect-only: 5 tests collected (was 4). Full run requires docker. Co-Authored-By: Claude Opus 4.7 --- .../api/events/test_events_authz_api.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/agentex/tests/integration/api/events/test_events_authz_api.py b/agentex/tests/integration/api/events/test_events_authz_api.py index 6b8a0ea8..71aa6229 100644 --- a/agentex/tests/integration/api/events/test_events_authz_api.py +++ b/agentex/tests/integration/api/events/test_events_authz_api.py @@ -249,3 +249,33 @@ async def test_list_events_authorized_returns_200( assert (AgentexResourceType.agent.value, test_agent.id) in checked for call in check_calls: assert call[1]["json"]["operation"] == "read" + + @pytest.mark.asyncio + @patch( + "src.api.authentication_middleware.AgentexAuthMiddleware.is_enabled", + return_value=True, + ) + @patch( + "src.domain.services.authorization_service.AuthorizationService.is_enabled", + return_value=True, + ) + async def test_list_events_unauthorized_agent_returns_404( + self, + is_enabled_authorization_mock, + is_enabled_mock, + isolated_client, + test_event, + test_agent, + test_task, + ): + """Caller without view on the queried agent_id gets 404, not 403 — the + DAuthorizedQuery on agent_id catches the denial and collapses it. No + leak of agent existence across tenants.""" + with patch( + "src.utils.http_request_handler.HttpRequestHandler.post_with_error_handling", + side_effect=_mock_post_factory(deny_agent_ids={test_agent.id}), + ): + response = await isolated_client.get( + f"/events?task_id={test_task.id}&agent_id={test_agent.id}" + ) + assert response.status_code == 404 From 5deda18360e7a160bab19f5c135452cc6ae021e4 Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 13:40:51 -0700 Subject: [PATCH 3/8] fix(AGX1-244): collapse direct-resource denials to 404 in DAuthorizedId/Query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI flagged my denied-list test: it expected 404 but got 403, because the direct-resource branch of _ensure_authorized_id / _ensure_authorized_query didn't have the AuthorizationError -> ItemDoesNotExist collapse that the child-resource branches got. Inconsistent — the events GET endpoint returns 404 on denial (via AgentChildResourceType branch) while the events LIST endpoint returns 403 (via direct-resource branch on agent_id). Same security argument as for child resources: a 403/404 split on a direct resource leaks existence of that resource across tenants. Aligns with Harvey's #255 direction. Risk audit: only one existing test asserts 403, and it's about a missing api-key header (authentication, not authorization). All existing 4xx assertions on direct-resource routes are already 404 (no-row-exists), not 403. So no test regressions. Co-Authored-By: Claude Opus 4.7 --- agentex/src/utils/authorization_shortcuts.py | 32 ++++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/agentex/src/utils/authorization_shortcuts.py b/agentex/src/utils/authorization_shortcuts.py index 1f0b518a..2d8fdfca 100644 --- a/agentex/src/utils/authorization_shortcuts.py +++ b/agentex/src/utils/authorization_shortcuts.py @@ -92,11 +92,17 @@ async def _ensure_authorized_id( f"Item with id '{resource_id}' does not exist." ) from None else: - # For direct resources, check directly - await authorization.check( - resource=AgentexResource(type=resource_type, selector=resource_id), - operation=operation, - ) + # Direct resources — same 404 collapse so get/list behave + # consistently and we don't leak existence via 403 vs 404. + try: + await authorization.check( + resource=AgentexResource(type=resource_type, selector=resource_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None return resource_id return Annotated[str, Depends(_ensure_authorized_id)] @@ -149,11 +155,17 @@ async def _ensure_authorized_query( f"Item with id '{resource_id}' does not exist." ) from None else: - # For direct resources, check directly - await authorization.check( - resource=AgentexResource(type=resource_type, selector=resource_id), - operation=operation, - ) + # Direct resources — same 404 collapse so get/list behave + # consistently and we don't leak existence via 403 vs 404. + try: + await authorization.check( + resource=AgentexResource(type=resource_type, selector=resource_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None return resource_id return Annotated[str, Depends(_ensure_authorized_query)] From 05f7442226d84deb270e0595038338d61bbc53ce Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 15:24:23 -0700 Subject: [PATCH 4/8] fix(AGX1-244): revert broad direct-resource 404 collapse Per Asher's review: the previous commit (5deda18) wrapped every direct-resource DAuthorizedId/DAuthorizedQuery check in AuthorizationError -> ItemDoesNotExist, which flipped 403 to 404 for every direct-resource route service-wide. That diverges from #249 (tasks) and #255 (task-children) which keep direct-resource denials as 403. Restore the else branch to a bare authorization.check() in both helpers. Only child-resource branches (TaskChildResourceType, AgentChildResourceType) collapse to 404, which is the intended scope: the parent-resolution path already loads the child to find its parent, so 404 is the natural shape. Update the events list test to assert 403 for an unauthorized agent_id (consistent with the direct-resource convention). Co-Authored-By: Claude Opus 4.7 --- agentex/src/utils/authorization_shortcuts.py | 30 +++++-------------- .../api/events/test_events_authz_api.py | 11 +++---- 2 files changed, 14 insertions(+), 27 deletions(-) diff --git a/agentex/src/utils/authorization_shortcuts.py b/agentex/src/utils/authorization_shortcuts.py index 2d8fdfca..e0891ed0 100644 --- a/agentex/src/utils/authorization_shortcuts.py +++ b/agentex/src/utils/authorization_shortcuts.py @@ -92,17 +92,10 @@ async def _ensure_authorized_id( f"Item with id '{resource_id}' does not exist." ) from None else: - # Direct resources — same 404 collapse so get/list behave - # consistently and we don't leak existence via 403 vs 404. - try: - await authorization.check( - resource=AgentexResource(type=resource_type, selector=resource_id), - operation=operation, - ) - except AuthorizationError: - raise ItemDoesNotExist( - f"Item with id '{resource_id}' does not exist." - ) from None + await authorization.check( + resource=AgentexResource(type=resource_type, selector=resource_id), + operation=operation, + ) return resource_id return Annotated[str, Depends(_ensure_authorized_id)] @@ -155,17 +148,10 @@ async def _ensure_authorized_query( f"Item with id '{resource_id}' does not exist." ) from None else: - # Direct resources — same 404 collapse so get/list behave - # consistently and we don't leak existence via 403 vs 404. - try: - await authorization.check( - resource=AgentexResource(type=resource_type, selector=resource_id), - operation=operation, - ) - except AuthorizationError: - raise ItemDoesNotExist( - f"Item with id '{resource_id}' does not exist." - ) from None + await authorization.check( + resource=AgentexResource(type=resource_type, selector=resource_id), + operation=operation, + ) return resource_id return Annotated[str, Depends(_ensure_authorized_query)] diff --git a/agentex/tests/integration/api/events/test_events_authz_api.py b/agentex/tests/integration/api/events/test_events_authz_api.py index 71aa6229..59557362 100644 --- a/agentex/tests/integration/api/events/test_events_authz_api.py +++ b/agentex/tests/integration/api/events/test_events_authz_api.py @@ -259,7 +259,7 @@ async def test_list_events_authorized_returns_200( "src.domain.services.authorization_service.AuthorizationService.is_enabled", return_value=True, ) - async def test_list_events_unauthorized_agent_returns_404( + async def test_list_events_unauthorized_agent_returns_403( self, is_enabled_authorization_mock, is_enabled_mock, @@ -268,9 +268,10 @@ async def test_list_events_unauthorized_agent_returns_404( test_agent, test_task, ): - """Caller without view on the queried agent_id gets 404, not 403 — the - DAuthorizedQuery on agent_id catches the denial and collapses it. No - leak of agent existence across tenants.""" + """Caller without view on the queried agent_id gets 403. Matches the + direct-resource convention used in #249/#255 — only task-children and + agent-children collapse to 404 (via the parent-resolution path); direct + agent/task denials surface as 403.""" with patch( "src.utils.http_request_handler.HttpRequestHandler.post_with_error_handling", side_effect=_mock_post_factory(deny_agent_ids={test_agent.id}), @@ -278,4 +279,4 @@ async def test_list_events_unauthorized_agent_returns_404( response = await isolated_client.get( f"/events?task_id={test_task.id}&agent_id={test_agent.id}" ) - assert response.status_code == 404 + assert response.status_code == 403 From 7090604280c4ecdb73924d4573bdfc053d45ba77 Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 15:29:09 -0700 Subject: [PATCH 5/8] refactor(AGX1-244): mirror #249 helper pattern; drop AgentChildResourceType Per Asher's review: the AgentChildResourceType branch in DAuthorizedId/ DAuthorizedQuery added an extra parent-resolution hop and a new resource- type family compared to #249 (tasks) and #255 (task-children), both of which only special-case task/task-children. Mirror #249's shape exactly: - Add agent_authorization.check_agent_or_collapse_to_404, parallel to task_authorization.check_task_or_collapse_to_404 from #249. Same docstring rationale (no tenant column on AgentORM, denial reason not discriminable, so collapse to 404 to prevent cross-tenant existence leak; TODO references AGX1-290). - Delete AgentChildResourceType enum. - Delete _get_parent_agent_id and the elif branch in both shortcuts. - Drop DEventRepository injection from the shortcuts (was only used by the deleted branch). - Rewrite get_event to do event_use_case.get(...) -> agent check inline, using the helper. Identical wire behavior (single /v1/authz/check on the parent agent, denial -> 404) but no new abstraction. Net: shortcuts file is now structurally identical to main; events.py follows the same pattern Harvey/Asher established for direct/child task routes. Co-Authored-By: Claude Opus 4.7 --- agentex/src/api/routes/events.py | 14 ++++-- .../src/api/schemas/authorization_types.py | 7 --- agentex/src/utils/agent_authorization.py | 40 +++++++++++++++ agentex/src/utils/authorization_shortcuts.py | 49 +------------------ 4 files changed, 53 insertions(+), 57 deletions(-) create mode 100644 agentex/src/utils/agent_authorization.py diff --git a/agentex/src/api/routes/events.py b/agentex/src/api/routes/events.py index 48c0d8e5..53ccef44 100644 --- a/agentex/src/api/routes/events.py +++ b/agentex/src/api/routes/events.py @@ -1,13 +1,14 @@ from fastapi import APIRouter, Query from src.api.schemas.authorization_types import ( - AgentChildResourceType, AgentexResourceType, AuthorizedOperationType, ) from src.api.schemas.events import Event +from src.domain.services.authorization_service import DAuthorizationService from src.domain.use_cases.events_use_case import DEventUseCase -from src.utils.authorization_shortcuts import DAuthorizedId, DAuthorizedQuery +from src.utils.agent_authorization import check_agent_or_collapse_to_404 +from src.utils.authorization_shortcuts import DAuthorizedQuery from src.utils.logging import make_logger logger = make_logger(__name__) @@ -20,10 +21,17 @@ response_model=Event, ) async def get_event( - event_id: DAuthorizedId(AgentChildResourceType.event, AuthorizedOperationType.read), + event_id: str, event_use_case: DEventUseCase, + authorization: DAuthorizationService, ) -> Event: + # Events delegate to their parent agent. Load the event first (404 if it + # doesn't exist), then check `read` on the parent agent and collapse a + # denial to 404 — see check_agent_or_collapse_to_404 for the rationale. event_entity = await event_use_case.get(event_id) + await check_agent_or_collapse_to_404( + authorization, event_entity.agent_id, AuthorizedOperationType.read + ) return Event.model_validate(event_entity) diff --git a/agentex/src/api/schemas/authorization_types.py b/agentex/src/api/schemas/authorization_types.py index 45c97be8..0fcee539 100644 --- a/agentex/src/api/schemas/authorization_types.py +++ b/agentex/src/api/schemas/authorization_types.py @@ -23,13 +23,6 @@ class TaskChildResourceType(StrEnum): state = "state" -# Resources that inherit permissions from their parent agent -class AgentChildResourceType(StrEnum): - """Resources that inherit permissions from their parent agent.""" - - event = "event" - - class AgentexResource(BaseModel): type: AgentexResourceType selector: str diff --git a/agentex/src/utils/agent_authorization.py b/agentex/src/utils/agent_authorization.py new file mode 100644 index 00000000..ee7c8b72 --- /dev/null +++ b/agentex/src/utils/agent_authorization.py @@ -0,0 +1,40 @@ +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_agent_or_collapse_to_404( + authorization, + agent_id: str, + operation: AuthorizedOperationType, +) -> None: + """Issue a check on an agent resource. On any denial, surface 404 — even + when the agent exists. + + Mirrors ``check_task_or_collapse_to_404`` (see ``task_authorization.py``). + The 403/404 split cannot be done safely here: ``AgentORM`` has no tenant + column, ``agent_repository.get`` does an unfiltered primary-key lookup, + and ``AuthorizationError`` carries no deny-reason discriminator. Returning + 403 when the row exists and 404 when it doesn't leaks cross-tenant + existence (caller probes "does agent X exist?" and gets distinguishable + responses). + + Until agents carry tenant scope (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 + ``update`` permission on an in-tenant agent sees 404 on update attempts + instead of 403. + + TODO(AGX1-290): Restore the 403/404 split for same-tenant calls once + agents carry tenant/account_id scope at the data layer. + """ + try: + await authorization.check( + resource=AgentexResource.agent(agent_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist(f"Item with id '{agent_id}' does not exist.") from None diff --git a/agentex/src/utils/authorization_shortcuts.py b/agentex/src/utils/authorization_shortcuts.py index e0891ed0..cd49ab9a 100644 --- a/agentex/src/utils/authorization_shortcuts.py +++ b/agentex/src/utils/authorization_shortcuts.py @@ -5,14 +5,12 @@ from src.adapters.authorization.exceptions import AuthorizationError from src.adapters.crud_store.exceptions import ItemDoesNotExist from src.api.schemas.authorization_types import ( - AgentChildResourceType, AgentexResource, AgentexResourceType, AuthorizedOperationType, TaskChildResourceType, ) from src.domain.repositories.agent_repository import DAgentRepository -from src.domain.repositories.event_repository import DEventRepository 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 @@ -33,23 +31,8 @@ async def _get_parent_task_id( return resource.task_id -async def _get_parent_agent_id( - resource_type: AgentChildResourceType, - resource_id: str, - event_repository: DEventRepository, -) -> str: - """Get the parent agent ID for an agent-child resource.""" - registry = { - AgentChildResourceType.event: event_repository, - } - - repository = registry[resource_type] - resource = await repository.get(id=resource_id) - return resource.agent_id - - def DAuthorizedId( - resource_type: AgentexResourceType | TaskChildResourceType | AgentChildResourceType, + resource_type: AgentexResourceType | TaskChildResourceType, operation: AuthorizedOperationType = AuthorizedOperationType.read, param_name: str = None, ): @@ -58,7 +41,6 @@ def DAuthorizedId( async def _ensure_authorized_id( authorization: DAuthorizationService, - event_repository: DEventRepository, state_repository: DTaskStateRepository, resource_id: str = Path(..., alias=param_name), ) -> str: @@ -78,19 +60,6 @@ async def _ensure_authorized_id( raise ItemDoesNotExist( f"Item with id '{resource_id}' does not exist." ) from None - elif isinstance(resource_type, AgentChildResourceType): - agent_id = await _get_parent_agent_id( - resource_type, resource_id, event_repository - ) - try: - await authorization.check( - resource=AgentexResource.agent(agent_id), - operation=operation, - ) - except AuthorizationError: - raise ItemDoesNotExist( - f"Item with id '{resource_id}' does not exist." - ) from None else: await authorization.check( resource=AgentexResource(type=resource_type, selector=resource_id), @@ -102,7 +71,7 @@ async def _ensure_authorized_id( def DAuthorizedQuery( - resource_type: AgentexResourceType | TaskChildResourceType | AgentChildResourceType, + resource_type: AgentexResourceType | TaskChildResourceType, operation: AuthorizedOperationType = AuthorizedOperationType.read, param_name: str = None, description: str = None, @@ -114,7 +83,6 @@ def DAuthorizedQuery( async def _ensure_authorized_query( authorization: DAuthorizationService, - event_repository: DEventRepository, state_repository: DTaskStateRepository, resource_id: str = Query(..., alias=param_name, description=description), ) -> str: @@ -134,19 +102,6 @@ async def _ensure_authorized_query( raise ItemDoesNotExist( f"Item with id '{resource_id}' does not exist." ) from None - elif isinstance(resource_type, AgentChildResourceType): - agent_id = await _get_parent_agent_id( - resource_type, resource_id, event_repository - ) - try: - await authorization.check( - resource=AgentexResource.agent(agent_id), - operation=operation, - ) - except AuthorizationError: - raise ItemDoesNotExist( - f"Item with id '{resource_id}' does not exist." - ) from None else: await authorization.check( resource=AgentexResource(type=resource_type, selector=resource_id), From d1caa921bda1480e06e9a76cf2e1dba2c2668614 Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 15:32:10 -0700 Subject: [PATCH 6/8] docs(AGX1-244): tighten comments in events PR Trim verbose comments and docstrings added in this PR: - events route: collapse 3-line preamble to one line. - agent_authorization: defer the long rationale to task_authorization's docstring instead of duplicating it. - events authz tests: shorten the module docstring and inline comments; fix a stale reference to the (now removed) AgentChildResourceType in the denied-list test docstring. Co-Authored-By: Claude Opus 4.7 --- agentex/src/api/routes/events.py | 4 +-- agentex/src/utils/agent_authorization.py | 22 +++------------- .../api/events/test_events_authz_api.py | 26 +++++-------------- 3 files changed, 11 insertions(+), 41 deletions(-) diff --git a/agentex/src/api/routes/events.py b/agentex/src/api/routes/events.py index 53ccef44..cbda563f 100644 --- a/agentex/src/api/routes/events.py +++ b/agentex/src/api/routes/events.py @@ -25,9 +25,7 @@ async def get_event( event_use_case: DEventUseCase, authorization: DAuthorizationService, ) -> Event: - # Events delegate to their parent agent. Load the event first (404 if it - # doesn't exist), then check `read` on the parent agent and collapse a - # denial to 404 — see check_agent_or_collapse_to_404 for the rationale. + # Events delegate authz to the parent agent. event_entity = await event_use_case.get(event_id) await check_agent_or_collapse_to_404( authorization, event_entity.agent_id, AuthorizedOperationType.read diff --git a/agentex/src/utils/agent_authorization.py b/agentex/src/utils/agent_authorization.py index ee7c8b72..7e2e2adc 100644 --- a/agentex/src/utils/agent_authorization.py +++ b/agentex/src/utils/agent_authorization.py @@ -11,25 +11,11 @@ async def check_agent_or_collapse_to_404( agent_id: str, operation: AuthorizedOperationType, ) -> None: - """Issue a check on an agent resource. On any denial, surface 404 — even - when the agent exists. + """Check an agent resource; collapse any denial to 404 to avoid leaking + cross-tenant existence. Mirrors ``check_task_or_collapse_to_404`` in + ``task_authorization.py`` — see that docstring for the full rationale. - Mirrors ``check_task_or_collapse_to_404`` (see ``task_authorization.py``). - The 403/404 split cannot be done safely here: ``AgentORM`` has no tenant - column, ``agent_repository.get`` does an unfiltered primary-key lookup, - and ``AuthorizationError`` carries no deny-reason discriminator. Returning - 403 when the row exists and 404 when it doesn't leaks cross-tenant - existence (caller probes "does agent X exist?" and gets distinguishable - responses). - - Until agents carry tenant scope (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 - ``update`` permission on an in-tenant agent sees 404 on update attempts - instead of 403. - - TODO(AGX1-290): Restore the 403/404 split for same-tenant calls once - agents carry tenant/account_id scope at the data layer. + TODO(AGX1-290): restore 403/404 split once agents carry tenant scope. """ try: await authorization.check( diff --git a/agentex/tests/integration/api/events/test_events_authz_api.py b/agentex/tests/integration/api/events/test_events_authz_api.py index 59557362..bd181d97 100644 --- a/agentex/tests/integration/api/events/test_events_authz_api.py +++ b/agentex/tests/integration/api/events/test_events_authz_api.py @@ -1,11 +1,4 @@ -""" -Integration tests for event-route authorization. - -Covers the AGX1-244 deliverable: list/get events enforce ``agent.read`` on -the parent agent (events delegate to the parent agent, not the task), with -denied checks collapsing to 404 (not 403) so callers cannot probe -cross-tenant existence by comparing response codes. -""" +"""AGX1-244: event routes delegate authz to the parent agent.""" from typing import Any from unittest.mock import patch @@ -131,8 +124,7 @@ async def test_get_event_authorized_returns_200( assert response.status_code == 200 assert response.json()["id"] == test_event.id - # Exactly one /v1/authz/check, on the parent agent with the read - # operation. Events delegate to the parent agent, not the parent task. + # One check, on the parent agent (not the task). check_calls = [ call for call in post_with_error_handling_mock.call_args_list @@ -167,7 +159,7 @@ async def test_get_event_unauthorized_returns_404( side_effect=_mock_post_factory(deny_agent_ids={test_agent.id}), ): response = await isolated_client.get(f"/events/{test_event.id}") - # Denial on the parent agent must collapse to 404, not surface as 403. + # Parent-agent denial collapses to 404. assert response.status_code == 404 @pytest.mark.asyncio @@ -192,8 +184,7 @@ async def test_get_event_nonexistent_returns_404( ): response = await isolated_client.get(f"/events/{orm_id()}") assert response.status_code == 404 - # The parent-agent lookup raises ItemDoesNotExist before any authz - # check fires, so /v1/authz/check should never be called. + # Event lookup 404s before any authz call fires. assert not any( call[0][1] == "/v1/authz/check" for call in post_with_error_handling_mock.call_args_list @@ -229,9 +220,7 @@ async def test_list_events_authorized_returns_200( body = response.json() assert any(e["id"] == test_event.id for e in body) - # The list endpoint takes both task_id and agent_id as query params and - # each has its own DAuthorizedQuery — so we expect two authz checks: - # one for the task (direct) and one for the agent (direct). + # Two checks: one on the task, one on the agent. check_calls = [ call for call in post_with_error_handling_mock.call_args_list @@ -268,10 +257,7 @@ async def test_list_events_unauthorized_agent_returns_403( test_agent, test_task, ): - """Caller without view on the queried agent_id gets 403. Matches the - direct-resource convention used in #249/#255 — only task-children and - agent-children collapse to 404 (via the parent-resolution path); direct - agent/task denials surface as 403.""" + """Direct-resource denials surface as 403 (convention from #249/#255).""" with patch( "src.utils.http_request_handler.HttpRequestHandler.post_with_error_handling", side_effect=_mock_post_factory(deny_agent_ids={test_agent.id}), From cf2fde8a9cb35f9dd46e328a0036816c47b772a4 Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 15:33:30 -0700 Subject: [PATCH 7/8] fix(AGX1-244): revert task-child collapse; let #249 own it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Asher's review on the task-child branch: same point as the direct- resource else — this PR shouldn't be modifying the task-child collapse behavior either, since #249 owns that change (via check_task_or_collapse_to_404). Revert the task-child branch in DAuthorizedId/DAuthorizedQuery back to main's shape: bare authorization.check(), no inline try/except. Now this PR's diff against main is purely subtractive on authorization_shortcuts.py: remove event from TaskChildResourceType (it no longer fits — events delegate to the parent agent, not the parent task) and the cascading cleanup of DEventRepository injection. No behavioral changes to the task-child path. #249 will apply on top cleanly. Co-Authored-By: Claude Opus 4.7 --- agentex/src/utils/authorization_shortcuts.py | 38 +++++++------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/agentex/src/utils/authorization_shortcuts.py b/agentex/src/utils/authorization_shortcuts.py index cd49ab9a..bb93d8c4 100644 --- a/agentex/src/utils/authorization_shortcuts.py +++ b/agentex/src/utils/authorization_shortcuts.py @@ -2,8 +2,6 @@ from fastapi import Depends, Path, Query, Request -from src.adapters.authorization.exceptions import AuthorizationError -from src.adapters.crud_store.exceptions import ItemDoesNotExist from src.api.schemas.authorization_types import ( AgentexResource, AgentexResourceType, @@ -44,23 +42,17 @@ async def _ensure_authorized_id( state_repository: DTaskStateRepository, resource_id: str = Path(..., alias=param_name), ) -> str: - # For child resources, check the parent. Collapse a denied check - # into 404 so callers cannot use 403 vs 404 to probe whether a resource - # exists in another tenant. + # For child resources, check the parent task if isinstance(resource_type, TaskChildResourceType): task_id = await _get_parent_task_id( resource_type, resource_id, state_repository ) - try: - await authorization.check( - resource=AgentexResource.task(task_id), - operation=operation, - ) - except AuthorizationError: - raise ItemDoesNotExist( - f"Item with id '{resource_id}' does not exist." - ) from None + await authorization.check( + resource=AgentexResource.task(task_id), + operation=operation, + ) else: + # For direct resources, check directly await authorization.check( resource=AgentexResource(type=resource_type, selector=resource_id), operation=operation, @@ -86,23 +78,17 @@ async def _ensure_authorized_query( state_repository: DTaskStateRepository, resource_id: str = Query(..., alias=param_name, description=description), ) -> str: - # For child resources, check the parent. Collapse a denied check - # into 404 so callers cannot use 403 vs 404 to probe whether a resource - # exists in another tenant. + # For child resources, check the parent task if isinstance(resource_type, TaskChildResourceType): task_id = await _get_parent_task_id( resource_type, resource_id, state_repository ) - try: - await authorization.check( - resource=AgentexResource.task(task_id), - operation=operation, - ) - except AuthorizationError: - raise ItemDoesNotExist( - f"Item with id '{resource_id}' does not exist." - ) from None + await authorization.check( + resource=AgentexResource.task(task_id), + operation=operation, + ) else: + # For direct resources, check directly await authorization.check( resource=AgentexResource(type=resource_type, selector=resource_id), operation=operation, From 4b68ad41a29c1fa1a17ce765fb3dfb67af9c68cb Mon Sep 17 00:00:00 2001 From: Dhruv Madhok Date: Fri, 29 May 2026 15:38:56 -0700 Subject: [PATCH 8/8] refactor(AGX1-244): re-add task-child 404 collapse to match #255 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirror Harvey's #255 inline pattern exactly in the task-child branch of DAuthorizedId/DAuthorizedQuery — same imports, same comment, same try/except shape. Direct-resource else branch stays as plain check (403), matching both #249 and #255. Net diff vs main is consistent with #255's shape: collapse for task-children, plain check for direct resources. Only delta-from-#255 is this PR's own scope (removing event from TaskChildResourceType and DEventRepository injection, since events delegate to the parent agent instead). Co-Authored-By: Claude Opus 4.7 --- agentex/src/utils/authorization_shortcuts.py | 36 ++++++++++++++------ 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/agentex/src/utils/authorization_shortcuts.py b/agentex/src/utils/authorization_shortcuts.py index bb93d8c4..e4e0e7f8 100644 --- a/agentex/src/utils/authorization_shortcuts.py +++ b/agentex/src/utils/authorization_shortcuts.py @@ -2,6 +2,8 @@ from fastapi import Depends, Path, Query, Request +from src.adapters.authorization.exceptions import AuthorizationError +from src.adapters.crud_store.exceptions import ItemDoesNotExist from src.api.schemas.authorization_types import ( AgentexResource, AgentexResourceType, @@ -42,15 +44,22 @@ async def _ensure_authorized_id( state_repository: DTaskStateRepository, resource_id: str = Path(..., alias=param_name), ) -> str: - # For child resources, check the parent task + # For child resources, check the parent task. Collapse a denied check + # into 404 so callers cannot use 403 vs 404 to probe whether a resource + # exists in another tenant. if isinstance(resource_type, TaskChildResourceType): task_id = await _get_parent_task_id( resource_type, resource_id, state_repository ) - await authorization.check( - resource=AgentexResource.task(task_id), - operation=operation, - ) + try: + await authorization.check( + resource=AgentexResource.task(task_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None else: # For direct resources, check directly await authorization.check( @@ -78,15 +87,22 @@ async def _ensure_authorized_query( state_repository: DTaskStateRepository, resource_id: str = Query(..., alias=param_name, description=description), ) -> str: - # For child resources, check the parent task + # For child resources, check the parent task. Collapse a denied check + # into 404 so callers cannot use 403 vs 404 to probe whether a resource + # exists in another tenant. if isinstance(resource_type, TaskChildResourceType): task_id = await _get_parent_task_id( resource_type, resource_id, state_repository ) - await authorization.check( - resource=AgentexResource.task(task_id), - operation=operation, - ) + try: + await authorization.check( + resource=AgentexResource.task(task_id), + operation=operation, + ) + except AuthorizationError: + raise ItemDoesNotExist( + f"Item with id '{resource_id}' does not exist." + ) from None else: # For direct resources, check directly await authorization.check(