feat(AGX1-263): agent_api_keys route migration to FGAC (404 collapse + two-factor)

[dm36]

Summary

Wires Spark AuthZ into all six agent_api_keys routes. Mirrors Asher's task-route pattern from #249.

• All denials on the api_key resource collapse to 404 so callers can't probe cross-tenant existence.
• GET list filters to api_keys the caller can read (id filter pushed into the repo for correct pagination).
• POST does an explicit agent.update check on the parent — only enforcement surface at create time, since no api_key resource exists yet.
• Mutations (delete) rely on SpiceDB's transitive api_key.delete = editor & parent_agent->update & tenant_gate expansion. The parent_agent edge is populated by #248's register_resource(parent=agent).

Stack

Repo	PR	State
scaleapi/scaleapi	#144657 — sgp-authz 0.7.0	✅ Merged
scaleapi/agentex	#354 — api_key mapping	✅ Merged
scaleapi/scale-agentex	#248 — dual-write	✅ Merged
scaleapi/scale-agentex	this PR — route-layer FGAC	Ready

Linear: AGX1-263.

Tests

• tests/unit/api/test_agent_api_keys_authz.py — 12 tests covering the collapse helper, DAuthorizedId routing, name-route inline collapse, list filtering, and create-parent-check.

Out of scope

• Restoring the 403/404 split once api_keys carry tenant scope (AGX1-290).

Greptile Summary

This PR wires Spark AuthZ (FGAC) into all six agent_api_keys routes, mirroring the task-route pattern from #249. All authorization denials collapse to 404 to prevent cross-tenant key-name probing, and the delete-by-name route now resolves name→id before checking auth (closing the TOCTOU window flagged in a prior review).

• All six routes gated: POST checks agent.update on the parent agent (no api_key resource exists yet); GET /{id} and DELETE /{id} use DAuthorizedId; GET /name/{name} and DELETE /name/{name} use the inline _check_api_key_or_collapse_to_404 helper; GET / uses DAuthorizedResourceIds with the id list pushed into the SQL filter for correct pagination.
• 404 collapse: Both "row absent" and "row present but denied" paths on name routes now raise ItemDoesNotExist(API_KEY_NOT_FOUND_MESSAGE), making the bodies byte-for-byte identical and closing the existence-leak vector.
• AgentAPIKeysUseCase.list gains an optional id filter with an explicit empty-list short-circuit to prevent the base repo from converting id=[] into an unfiltered query.

Confidence Score: 5/5

The FGAC wiring is correct across all six routes; the 404-collapse and TOCTOU fixes flagged in prior reviews have been addressed.

The authorization logic is sound: denials are consistently collapsed to 404 with identical bodies, delete-by-name now deletes by resolved ID rather than by name, and the empty-list short-circuit prevents an accidental unfiltered query. The only findings are a misleading (but vacuous) test assertion and an untyped parameter — neither affects runtime behavior.

The test assertion in test_delete_by_name_handler_collapses_denial_to_404 checks a removed method rather than the one now used for deletion; worth a quick fix before the tests are relied upon for regression coverage.

Important Files Changed

Filename	Overview
agentex/src/api/routes/agent_api_keys.py	All six routes now gated by FGAC; denials collapse to 404; delete-by-name resolves name→id before auth check, closing the TOCTOU window; absent and denied 404 bodies are byte-for-byte identical via API_KEY_NOT_FOUND_MESSAGE.
agentex/src/utils/agent_api_key_authorization.py	New collapse helper correctly catches AuthorizationError and raises ItemDoesNotExist with a fixed message; minor: authorization parameter is untyped.
agentex/src/utils/authorization_shortcuts.py	DAuthorizedId gains an api_key branch that delegates to the collapse helper; formatting-only change to DAuthorizedBodyId; no logic issues.
agentex/src/domain/use_cases/agent_api_keys_use_case.py	list() gains optional id filter pushed to repo layer; empty-list short-circuit correctly prevents unfiltered query when caller has no authorized keys.
agentex/tests/unit/api/test_agent_api_keys_authz.py	12 tests covering all major FGAC paths; test_delete_by_name_handler_collapses_denial_to_404 asserts the removed delete_by_agent_id_and_key_name is not called rather than the new delete method, making the delete-not-invoked assertion vacuous.
agentex/src/api/schemas/authorization_types.py	Formatting-only change to the schedule() method signature; no logic changes.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Route as agent_api_keys route
    participant SpiceDB as SpiceDB (AuthZ)
    participant DB as Postgres (repo)

    Note over Route,DB: GET /{id} / DELETE /{id} — DAuthorizedId dep
    Client->>Route: "GET /agent_api_keys/{id}"
    Route->>SpiceDB: "check(api_key:{id}, read)"
    alt denied
        SpiceDB-->>Route: AuthorizationError
        Route-->>Client: 404 ItemDoesNotExist
    else allowed
        SpiceDB-->>Route: OK
        Route->>DB: get(id)
        DB-->>Route: entity
        Route-->>Client: 200 AgentAPIKey
    end

    Note over Route,DB: GET /name/{name} — inline collapse
    Client->>Route: "GET /agent_api_keys/name/{name}"
    Route->>DB: get_by_agent_id_and_name(...)
    alt absent
        DB-->>Route: None
        Route-->>Client: 404 Agent api_key not found.
    else present
        DB-->>Route: entity (with id)
        Route->>SpiceDB: "check(api_key:{id}, read)"
        alt denied
            SpiceDB-->>Route: AuthorizationError
            Route-->>Client: 404 Agent api_key not found.
        else allowed
            SpiceDB-->>Route: OK
            Route-->>Client: 200 AgentAPIKey
        end
    end

    Note over Route,DB: DELETE /name/{name} — resolve→check→delete
    Client->>Route: "DELETE /agent_api_keys/name/{name}"
    Route->>DB: get_by_agent_id_and_name(...)
    alt absent
        DB-->>Route: None
        Route-->>Client: 404 Agent api_key not found.
    else present
        DB-->>Route: entity (with id)
        Route->>SpiceDB: "check(api_key:{id}, delete)"
        alt denied
            SpiceDB-->>Route: AuthorizationError
            Route-->>Client: 404 Agent api_key not found.
        else allowed
            SpiceDB-->>Route: OK
            Route->>DB: "delete(id=entity.id)"
            Route-->>Client: 200 deleted
        end
    end

    Note over Route,DB: POST / — parent agent.update check
    Client->>Route: POST /agent_api_keys
    Route->>DB: agent_use_case.get(...)
    Route->>SpiceDB: "check(agent:{id}, update)"
    alt denied
        SpiceDB-->>Route: AuthorizationError 403
        Route-->>Client: 403
    else allowed
        SpiceDB-->>Route: OK
        Route->>DB: create api_key
        Route-->>Client: 201 CreateAPIKeyResponse
    end

Loading

Comments Outside Diff (2)

1. agentex/src/api/routes/agent_api_keys.py, line 163-179 (link)

   Distinguishable 404 bodies defeat the existence-leak collapse

   The name routes produce two structurally different 404 responses depending on whether the key exists in the database. If the key is absent, HTTPException is raised with "Agent api_key '{name}' not found for agent ID {agent.id}". If the key exists but the caller is denied, ItemDoesNotExist is raised with "Item with id '{api_key_id}' does not exist.". Both return HTTP 404, but a cross-tenant caller who can resolve an agent_id can probe for key-name existence by comparing the response body — exactly the information leak the collapse is meant to prevent. The same issue exists in delete_agent_api_key_by_name (line ~264). To close it, both "not found" paths should produce an identical detail string (e.g. both raise ItemDoesNotExist with the same generic message).

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: agentex/src/api/routes/agent_api_keys.py
   Line: 163-179
   
   Comment:
   **Distinguishable 404 bodies defeat the existence-leak collapse**
   
   The name routes produce two structurally different 404 responses depending on whether the key exists in the database. If the key is absent, `HTTPException` is raised with `"Agent api_key '{name}' not found for agent ID {agent.id}"`. If the key exists but the caller is denied, `ItemDoesNotExist` is raised with `"Item with id '{api_key_id}' does not exist."`. Both return HTTP 404, but a cross-tenant caller who can resolve an `agent_id` can probe for key-name existence by comparing the response body — exactly the information leak the collapse is meant to prevent. The same issue exists in `delete_agent_api_key_by_name` (line ~264). To close it, both "not found" paths should produce an identical detail string (e.g. both raise `ItemDoesNotExist` with the same generic message).
   
   How can I resolve this? If you propose a fix, please make it concise.

   

2. agentex/src/api/routes/agent_api_keys.py, line 260-278 (link)

   TOCTOU window between auth check and delete-by-name

   The auth check resolves the key to an id and passes that ID to SpiceDB, but the subsequent delete is issued by name (delete_by_agent_id_and_key_name). If the key is deleted and a new key with the same name is created between the lookup and the delete, the auth check will have evaluated the old key's ID (which the caller may have legitimately had delete rights on) but the mutation will affect the new key. The practical risk is low, but it is a real race between a non-atomic read-auth-mutate sequence. Deleting by the resolved id instead of by name would close it.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: agentex/src/api/routes/agent_api_keys.py
   Line: 260-278
   
   Comment:
   **TOCTOU window between auth check and delete-by-name**
   
   The auth check resolves the key to an `id` and passes that ID to SpiceDB, but the subsequent delete is issued by name (`delete_by_agent_id_and_key_name`). If the key is deleted and a new key with the same name is created between the lookup and the delete, the auth check will have evaluated the old key's ID (which the caller may have legitimately had delete rights on) but the mutation will affect the new key. The practical risk is low, but it is a real race between a non-atomic read-auth-mutate sequence. Deleting by the resolved `id` instead of by name would close it.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
agentex/tests/unit/api/test_agent_api_keys_authz.py:195-213
**Vacuous assertion on removed method**

`api_key_use_case.delete_by_agent_id_and_key_name.assert_not_called()` is dead code — `delete_agent_api_key_by_name` no longer calls `delete_by_agent_id_and_key_name`; it calls `agent_api_key_use_case.delete(id=...)`. The comment says "the delete is NOT invoked when the check fails," but the assertion doesn't test that. If someone accidentally removed the `_check_api_key_or_collapse_to_404` guard, this assertion would still pass. Add `api_key_use_case.delete = AsyncMock()` in the setup and assert `api_key_use_case.delete.assert_not_called()` to test the intended safety property.

### Issue 2 of 2
agentex/src/utils/agent_api_key_authorization.py:13-17
Missing type annotation on `authorization` parameter — the other parameters are typed but this one isn't, so type-checkers won't catch callers passing the wrong type. The concrete type is `DAuthorizationService` (matching every call site in this PR).

```suggestion
async def _check_api_key_or_collapse_to_404(
    authorization: "DAuthorizationService",
    api_key_id: str,
    operation: AuthorizedOperationType,
) -> None:
```

_{Reviews (4): Last reviewed commit: "refactor(AGX1-263): close delete-by-name..." | Re-trigger Greptile}