Skip to content

feat(AGX1-244): enforce parent-agent authz on event routes#257

Open
dm36 wants to merge 8 commits into
mainfrom
dhruv/agx1-244-events-route-fgac
Open

feat(AGX1-244): enforce parent-agent authz on event routes#257
dm36 wants to merge 8 commits into
mainfrom
dhruv/agx1-244-events-route-fgac

Conversation

@dm36
Copy link
Copy Markdown

@dm36 dm36 commented May 29, 2026

Linear

AGX1-244

Summary

Enforces SpiceDB authorization on the event routes. Per the ticket, events are view-only, are not a SpiceDB resource (no schema, no dual-write), and delegate authorization to the parent agent (not the parent task — that's the structural difference from the sibling messages PR #255).

Permission model:

  • GET /events/{event_id} -> agent.read on the parent agent
  • GET /events?task_id=...&agent_id=... -> task.read + agent.read (existing on this endpoint; unchanged here)

Changes

  • authorization_types.py: split event out of TaskChildResourceType (where it was misplaced) and introduce a new AgentChildResourceType enum containing event. State stays under TaskChildResourceType.
  • authorization_shortcuts.py:
    • Add _get_parent_agent_id(...) mirroring _get_parent_task_id(...). Resolves event_id -> agent_id via the event repository (the EventEntity carries agent_id directly, so no two-hop event -> task -> agent).
    • DAuthorizedId / DAuthorizedQuery now branch on AgentChildResourceType and check AgentexResource.agent(...).
    • Both child-resource branches now collapse AuthorizationError -> ItemDoesNotExist (404) so a denied check is indistinguishable from a non-existent resource (no 403/404 oracle for cross-tenant probing). This is the same shape Harvey introduced in feat(AGX1-277): enforce parent-task authz on GET /messages/{message_id} #255 for TaskChildResourceType.
    • _get_parent_task_id signature simplified: it no longer takes event_repository since events are no longer task-children.
  • routes/events.py: GET /events/{event_id} now uses DAuthorizedId(AgentChildResourceType.event, read). The list endpoint was already authz-protected per query param and is unchanged.
  • Tests: new tests/integration/api/events/test_events_authz_api.py mirroring Harvey's test_messages_authz_api.py:
    • Authorized GET /events/{event_id} -> 200; asserts exactly one /v1/authz/check on agent (not task) with operation read.
    • Denied GET -> 404 (no-leak guarantee).
    • Nonexistent event_id -> 404, with explicit assertion that no /v1/authz/check fires (404 came from the repo, not from a denied check).
    • Authorized list -> 200; asserts two checks, one on task and one on agent, both read.

Behavior change beyond the ticket

The ticket calls for permissioning on get / list. The list endpoint already had per-query-param authz on task_id and agent_id — left as-is. The event -> task mapping that lived in TaskChildResourceType was an existing miswiring (events would have inherited from the task, not the agent); this PR corrects it as part of the deliverable.

Out of scope

  • No SpiceDB schema for event (ticket: events are not a SpiceDB resource).
  • No register_resource / dual-write for events.
  • agent_identity bypass via AuthorizationService._bypass() is unchanged — agent-API-key callers still bypass per the AGX1-305 carve-out.

Coordination

Note on ticket wording

The ticket says "view permission on the parent agent." The wire-level operation in AuthorizedOperationType is read (which is what the agentex_agent SpiceDB schema calls it). "View" in the ticket maps to read here, matching how state / checkpoint / message authz is wired.

Test plan

  • Static: imports resolve, pytest collects all 33 tests in the touched directories with no errors.
  • uv run pytest agentex/tests/integration/api/events/ -q — not run locally (Docker daemon unavailable in this sandbox; testcontainers can't start Postgres). Will validate in CI.
  • Regression: uv run pytest agentex/tests/integration/api/messages/ and .../states/ — same, deferred to CI.

Greptile Summary

Enforces SpiceDB authorization on GET /events/{event_id} by delegating to the parent agent (agent.read) rather than the parent task, correcting a pre-existing misclassification of events under TaskChildResourceType. The list endpoint's existing per-query-param checks are left unchanged.

  • TaskChildResourceType.event is removed; events are now authorized via a new standalone helper check_agent_or_collapse_to_404 that collapses AuthorizationError to 404 to prevent cross-tenant existence probing.
  • authorization_shortcuts.py gains the same 404-collapse for the TaskChildResourceType branch in both DAuthorizedId and DAuthorizedQuery, and drops the now-unnecessary event_repository parameter from _get_parent_task_id.
  • A new integration test suite (test_events_authz_api.py) covers authorized GET (asserts exactly one agent.read check), denied GET (404), nonexistent event (404 with no authz call), authorized list (two checks), and denied-agent list (403).

Confidence Score: 5/5

Safe to merge; the authz logic is straightforward, the no-leak guarantee is preserved, and the new tests cover all four meaningful cases.

The route correctly fetches the event before checking authorization (necessary to resolve agent_id), both denial paths collapse to 404, and the test suite explicitly verifies the no-authz-call-on-404 invariant. The only finding is a missing type annotation on the authorization parameter in the new helper.

No files require special attention beyond the minor typing gap in agent_authorization.py.

Important Files Changed

Filename Overview
agentex/src/utils/agent_authorization.py New helper that checks agent authorization and collapses AuthorizationError to 404; correctly mirrors the task equivalent, but the authorization parameter is untyped.
agentex/src/api/routes/events.py Switches GET /events/{event_id} from DAuthorizedId(TaskChildResourceType.event) to a two-step fetch-then-check pattern; the list endpoint is unchanged.
agentex/src/api/schemas/authorization_types.py Removes misplaced event value from TaskChildResourceType; only state remains.
agentex/src/utils/authorization_shortcuts.py Removes event_repository from _get_parent_task_id and adds AuthorizationError→ItemDoesNotExist collapse to both DAuthorizedId and DAuthorizedQuery for TaskChildResourceType; the else (direct resource) branch is intentionally left as 403.
agentex/tests/integration/api/events/test_events_authz_api.py New integration test suite covering authorized GET, denied GET (404), nonexistent event (404 with no authz call), authorized list (two checks), and denied-agent list (403); all assertions match the actual implementation behavior.

Sequence Diagram

sequenceDiagram
    participant Client
    participant EventsRoute
    participant EventUseCase
    participant DB as Event DB
    participant AuthzHelper as check_agent_or_collapse_to_404
    participant SpiceDB

    Client->>EventsRoute: "GET /events/{event_id}"
    EventsRoute->>EventUseCase: get(event_id)
    EventUseCase->>DB: "SELECT WHERE id=event_id"
    alt event not found
        DB-->>EventUseCase: not found
        EventUseCase-->>EventsRoute: ItemDoesNotExist
        EventsRoute-->>Client: 404
    else event found
        DB-->>EventUseCase: EventEntity (carries agent_id)
        EventUseCase-->>EventsRoute: EventEntity
        EventsRoute->>AuthzHelper: check_agent_or_collapse_to_404(auth, agent_id, read)
        AuthzHelper->>SpiceDB: "POST /v1/authz/check {type:agent, selector:agent_id, op:read}"
        alt denied
            SpiceDB-->>AuthzHelper: AuthorizationError
            AuthzHelper-->>EventsRoute: ItemDoesNotExist (collapsed)
            EventsRoute-->>Client: 404
        else allowed
            SpiceDB-->>AuthzHelper: allowed
            AuthzHelper-->>EventsRoute: ok
            EventsRoute-->>Client: 200 Event
        end
    end
Loading

Fix All in Cursor Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
agentex/src/utils/agent_authorization.py:9-13
Missing type annotation on the `authorization` parameter. Every other helper in this module hierarchy (e.g. `authorization_shortcuts.py`) types the service as `DAuthorizationService` when receiving it as a dependency, or `AuthorizationService` when the resolved object is passed directly. Here the resolved `AuthorizationService` instance is passed in, so the parameter type should be `AuthorizationService`. Without it, static type checkers get no help and `authorization.check(...)` is unchecked.

```suggestion
async def check_agent_or_collapse_to_404(
    authorization: "AuthorizationService",
    agent_id: str,
    operation: AuthorizedOperationType,
) -> None:
```

Reviews (5): Last reviewed commit: "refactor(AGX1-244): re-add task-child 40..." | Re-trigger Greptile

dm36 and others added 2 commits May 29, 2026 13:02
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=<denied>. 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 <noreply@anthropic.com>
@dm36 dm36 marked this pull request as ready for review May 29, 2026 20:34
@dm36 dm36 requested a review from a team as a code owner May 29, 2026 20:34
Comment thread agentex/tests/integration/api/events/test_events_authz_api.py Outdated
…Id/Query

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 <noreply@anthropic.com>
raise ItemDoesNotExist(
f"Item with id '{resource_id}' does not exist."
) from None
else:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to be consistent here w/ my PR (#249) and @harvhan's (#255)

i think the version here is error-prone. in this case, this else is the path every direct-resource DAuthorizedId takes, not just events. Wrapping it in except AuthorizationError -> ItemDoesNotExist flips 403 to 404 for every direct-resource route in the service

#249 keeps else as 403 and only collapses to 404 for tasks/task-children (elif resource_type == AgentexResourceType.task). #255 leaves else as 403 too.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

raise ItemDoesNotExist(
f"Item with id '{resource_id}' does not exist."
) from None
else:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simillar comment as above

Copy link
Copy Markdown

@asherfink asherfink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall but want to clarify the comment / make consistent with other PRs here before stamping!

raise ItemDoesNotExist(
f"Item with id '{resource_id}' does not exist."
) from None
else:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

dm36 and others added 4 commits May 29, 2026 15:24
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 <noreply@anthropic.com>
…ceType

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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Comment thread agentex/src/utils/agent_authorization.py
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 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants