feat(checkpoints): FGAC checkpoint routes, enforce via parent task pe…#260
feat(checkpoints): FGAC checkpoint routes, enforce via parent task pe…#260jenniechung wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if path == "/v1/authz/search": | ||
| return {"items": []} | ||
| raise Exception(f"Unknown path: {path}") | ||
|
|
There was a problem hiding this comment.
put-writes test coverage
The put-writes endpoint had the same execute → update operation change as put, but has no tests at all — neither an authorized path (verifying the update operation is sent to the authz service) nor a denied path (verifying the 404 collapse). The PR test plan lists 7 tests and explicitly names the other four routes, but put-writes is absent. A regression that re-introduced execute on this route would go undetected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: agentex/tests/integration/api/checkpoints/test_checkpoints_authz_api.py
Line: 61
Comment:
**Missing `put-writes` test coverage**
The `put-writes` endpoint had the same `execute → update` operation change as `put`, but has no tests at all — neither an authorized path (verifying the `update` operation is sent to the authz service) nor a denied path (verifying the 404 collapse). The PR test plan lists 7 tests and explicitly names the other four routes, but `put-writes` is absent. A regression that re-introduced `execute` on this route would go undetected.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "metadata": {"step": 1}, | ||
| "blobs": [], | ||
| }, | ||
| ) | ||
| assert response.status_code == 404 | ||
|
|
||
| # ── delete-thread ── | ||
|
|
||
| @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_delete_thread_authorized_returns_204( | ||
| self, | ||
| post_with_error_handling_mock, | ||
| is_enabled_authorization_mock, | ||
| is_enabled_mock, | ||
| isolated_client, | ||
| test_checkpoint, | ||
| test_task, | ||
| ): | ||
| response = await isolated_client.post( | ||
| "/checkpoints/delete-thread", json={"thread_id": test_task.id} | ||
| ) | ||
| assert response.status_code == 204 | ||
|
|
||
| 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.task.value | ||
| assert authz_data["resource"]["selector"] == test_task.id | ||
| assert authz_data["operation"] == "delete" |
There was a problem hiding this comment.
delete-thread
Every other route with a denied case has a corresponding _unauthorized_returns_404 test (get-tuple, list, put), but delete-thread only has the authorized 204 path. Because delete maps to the owner role only — the most restrictive permission — validating that a denied check collapses to 404 rather than 403 is particularly important for this route to preserve the cross-tenant opacity guarantee.
Prompt To Fix With AI
This is a comment left during a code review.
Path: agentex/tests/integration/api/checkpoints/test_checkpoints_authz_api.py
Line: 325-369
Comment:
**No denied-path test for `delete-thread`**
Every other route with a denied case has a corresponding `_unauthorized_returns_404` test (get-tuple, list, put), but `delete-thread` only has the authorized 204 path. Because `delete` maps to the owner role only — the most restrictive permission — validating that a denied check collapses to 404 rather than 403 is particularly important for this route to preserve the cross-tenant opacity guarantee.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Enforce Spark AuthZ on the checkpoint routes by delegating to the owning task. Checkpoints have no SpiceDB type of their own —
thread_idis the owning task id (LangGraph checkpointer convention; the SDK templates setconfigurable.thread_id = task.id), so each route checks the parenttaskdirectly viaDAuthorizedBodyId(task, …, field_name="thread_id"). Noregister_resource, no dual-write — the authz request only ever carries resource typetask.Operation mapping (matches the task SpiceDB schema):
get-tuple,listreadviewer + editor + ownerput,put-writesupdateeditor + ownerdelete-threaddeleteownerA denied
taskcheck collapses to 404 (not 403) so callers cannot probe cross-tenant existence by comparing response codes.Switches
put/put-writesfromexecute→update:executemaps to no task permission in the schema (it would lock everyone out), whereastask.update = (editor + owner) & internal_tenant_gatelets the editor role write checkpoints without owner.Changes
src/utils/authorization_shortcuts.py—DAuthorizedBodyIdcollapses a deniedtaskcheck into 404 (AuthorizationError → ItemDoesNotExist); non-task resources keep surfacing 403. Carries aTODOto converge on the canonicalcheck_task_or_collapse_to_404helper from AGX1-275 / feat(AGX1-275): per-RPC task permission rewire and 404/403 wrap #249.src/api/routes/checkpoints.py—put/put-writes:execute → update. (get-tuple/listalreadyread;delete-threadalreadydelete.)tests/integration/api/checkpoints/test_checkpoints_authz_api.py(new) — integration tests covering the auth matrix.Behavior change beyond the ticket
The 404 collapse lives in
DAuthorizedBodyId'staskbranch, so it also applies to the other body-id task routes (messagePOST/PUT, state create) — those now collapse a denied check to 404 as well. Same cross-tenant-leak rationale, and aligns with the canonical wrap landing in AGX1-275 (#249).Out of scope
request.state.agent_identity) still bypasses authz viaAuthorizationService._bypass()— same caveat as the message-route migration.task.update.Coordination
AGX1-275 (#249) introduces the shared
check_task_or_collapse_to_404helper insrc/utils/task_authorization.pyand the sameexecute → updateswap acrosscheckpoints.py/messages.py/states.py. This PR's inlineDAuthorizedBodyIdtask-wrap (flagged with aTODO) converges on that helper when the two land together — trivial merge regardless of order.Pre-merge wire dependency: the
execute → updateswap changes the operation literal hitting agentex-auth.updateis already in SGP'sAgentexOperationenum on master, so no additional gating is needed for this PR.Test Plan
tests/integration/api/checkpoints/test_checkpoints_authz_api.py— 7/7 passing:get-tuple/listauthorized → 200 (onetask/readcheck); denied → 404 via collapse.putauthorized → 200 (onetask/updatecheck); denied → 404.delete-threadauthorized → 204 (onetask/deletecheck).tests/integration/api/{messages,states}/, since the sharedDAuthorizedBodyIdtask-wrap changes their write-route deny code from 403 → 404. Both suites pass; no regressions from the DAuthorizedBodyId task-wrap change.Linear Issue
resolves https://linear.app/scale-epd/issue/AGX1-302, Parent: AGX1-299.
Greptile Summary
This PR enforces FGAC authorization on the five checkpoint routes by delegating to the parent task (
thread_idis the task ID), and fixesput/put-writesfrom the non-functionalexecuteoperation totask.update. A new task-specific branch inDAuthorizedBodyIdcollapsesAuthorizationError→ItemDoesNotExist(404) to prevent cross-tenant probing, with a TODO tracking convergence on the sharedcheck_task_or_collapse_to_404helper from AGX1-275.checkpoints.py:putandput-writesnow checktask.update(editor + owner) instead oftask.execute(unmapped, would lock everyone out).get-tuple/listremainread;delete-threadremainsdelete.authorization_shortcuts.py:DAuthorizedBodyIdgains a task branch that re-raisesAuthorizationErroras 404, keeping gateway/service errors (502/503) unchanged. The change also silently affects other body-id task routes (message POST/PUT, state create) as documented in the PR.test_checkpoints_authz_api.py: Seven new integration tests cover the auth matrix;put-writesis not tested (authorized or denied), and thedelete-threaddenied path is absent.Confidence Score: 4/5
Safe to merge with minor test gaps; the authorization logic itself is correct and well-scoped.
The core changes in checkpoints.py and authorization_shortcuts.py are correct. The test file omits put-writes entirely and skips the delete-thread denied path, leaving two gaps in the authorization coverage the PR set out to verify.
agentex/tests/integration/api/checkpoints/test_checkpoints_authz_api.py is missing put-writes tests and the delete-thread denied path.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant CheckpointRoute participant DAuthorizedBodyId participant AuthzService participant CheckpointsUseCase Client->>CheckpointRoute: "POST /checkpoints/{route}" CheckpointRoute->>DAuthorizedBodyId: "resolve dependency field_name=thread_id" DAuthorizedBodyId->>DAuthorizedBodyId: "body[thread_id] = field_value" alt "resource_type == task" DAuthorizedBodyId->>AuthzService: check(task, field_value, operation) alt Authorized AuthzService-->>DAuthorizedBodyId: OK DAuthorizedBodyId-->>CheckpointRoute: field_value CheckpointRoute->>CheckpointsUseCase: execute business logic CheckpointsUseCase-->>Client: 200 or 204 else AuthorizationError AuthzService-->>DAuthorizedBodyId: AuthorizationError DAuthorizedBodyId->>DAuthorizedBodyId: raise ItemDoesNotExist DAuthorizedBodyId-->>Client: 404 else Gateway or ServiceError AuthzService-->>DAuthorizedBodyId: 502 or 503 DAuthorizedBodyId-->>Client: 502 or 503 end else other resource_type DAuthorizedBodyId->>AuthzService: check(resource, operation) AuthzService-->>Client: 403 on denial endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "move back imports" | Re-trigger Greptile