Skip to content

feat(AGX1-301): authorize states via parent task#262

Open
deepthi-rao-scale wants to merge 5 commits into
mainfrom
deepthirao/agx1-301-fgac-state-route-migration-enforce-via-parent-task
Open

feat(AGX1-301): authorize states via parent task#262
deepthi-rao-scale wants to merge 5 commits into
mainfrom
deepthirao/agx1-301-fgac-state-route-migration-enforce-via-parent-task

Conversation

@deepthi-rao-scale
Copy link
Copy Markdown

@deepthi-rao-scale deepthi-rao-scale commented Jun 2, 2026

Summary: Authorizes state list/get/create/update through the parent task instead of any state AuthZ resource. GET /states now filters to tasks the caller can read via list_resources(task, read); GET /states/{id} continues to resolve the state parent task and checks read with 404 collapse from the shared task-child path; state create/update now use task.execute as the current owner-level placeholder, with a TODO for AGX1-237 if agentex-auth adds a dedicated state create/upsert task permission. Tests: added integration coverage for view-not-owner create denial, no-view list/get denial, accessible-task list filtering, owner create, and no state register_resource call. Local verification: python3.14 compileall on changed files passes; git diff --check passes. Blocked: pytest and ruff via Poetry cannot run locally because this shell has Python 3.11/3.13/3.14 but the project requires >=3.12,<3.13.

Greptile Summary

This PR moves state authorization away from a dedicated state auth resource and instead gates all state reads/writes through the parent task. GET /states with an explicit task_id now does a direct check() on the task, the no-task_id path uses list_resources to filter to accessible tasks, and create/update both require the new manage_access task operation added to the enum.

  • Routes (states.py): filter_states injects DAuthorizationService directly and branches on whether task_id is provided, calling check_task_or_collapse_to_404 for the explicit-id path and list_resources for the wildcard path; create and update use _STATE_WRITE_OPERATION = manage_access via the existing DAuthorizedBodyId/DAuthorizedId shortcuts.
  • Use case (states_use_case.py): list() gains authorized_task_ids; when populated it applies a $in filter and short-circuits to [] on an empty set, preserving the previous direct task_id filter when that is supplied.
  • Tests (test_states_authz_api.py): New integration suite covering create/update denial for view-only callers, list filtering to accessible tasks, direct-check for explicit task_id, and the owner-create happy path including an assertion that /v1/authz/register is never called for states.

Confidence Score: 5/5

Safe to merge; the new task-scoped auth logic is consistent across all five state endpoints and the bypass path is handled correctly in all branches.

All read/write state endpoints now consistently resolve auth through the parent task, the bypass path is handled correctly, and the new test suite covers the key scenarios including the previously flagged direct-check and update-denial paths.

No files require special attention.

Important Files Changed

Filename Overview
agentex/src/api/routes/states.py Rewires state auth to use parent-task checks: inline task-read guard for list, manage_access for create/update; imports DAuthorizationService and check_task_or_collapse_to_404.
agentex/src/api/schemas/authorization_types.py Adds manage_access to AuthorizedOperationType enum; straightforward addition used as the owner-level state write permission.
agentex/src/domain/use_cases/states_use_case.py Adds authorized_task_ids param to list(); applies $in filter when provided and short-circuits to [] on empty set; fixes task_id None-check.
agentex/tests/integration/api/states/test_states_authz_api.py New integration test suite for state authorization; covers create/update denial, list filtering, and owner-create happy path, but asserts operation names as raw strings rather than enum values.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["GET /states"] --> B{task_id provided?}
    B -->|Yes| C["check_task_or_collapse_to_404\n(task, read)"]
    C -->|Denied| D["→ 404"]
    C -->|Allowed| E["list states\nfiltered by task_id"]
    B -->|No| F["list_resources\n(task, read)"]
    F -->|None bypass| G["list all states\n(auth disabled)"]
    F -->|Empty set| H["→ []"]
    F -->|IDs returned| I["list states\nfiltered by $in task_ids"]

    J["POST /states"] --> K["DAuthorizedBodyId\n(task, manage_access)"]
    K -->|Denied| D
    K -->|Allowed| L["states_use_case.create"]

    M["PUT /states/{id}"] --> N["DAuthorizedId\n(state → parent task, manage_access)"]
    N -->|Denied| D
    N -->|Allowed| O["states_use_case.update"]

    P["GET /states/{id}"] --> Q["DAuthorizedId\n(state → parent task, read)"]
    Q -->|Denied| D
    Q -->|Allowed| R["states_use_case.get"]

    S["DELETE /states/{id}"] --> T["DAuthorizedId\n(state → parent task, delete)"]
    T -->|Denied| D
    T -->|Allowed| U["states_use_case.delete"]
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/tests/integration/api/states/test_states_authz_api.py:66
**Hardcoded operation strings instead of enum values**

Several assertions across this file use raw string literals (`"manage_access"`, `"read"`, `"delete"`) rather than `AuthorizedOperationType.X.value`. This pattern appears in `_mock_post_factory` (line 66), and in the operation assertions of the create-denial, list-with-task-id, update-denial, delete-denial, and owner-create tests. If an operation string is renamed in the enum, these assertions would silently pass against the wrong value.

Reviews (2): Last reviewed commit: "fix(AGX1-301): reuse task auth collapse ..." | Re-trigger Greptile

@deepthi-rao-scale deepthi-rao-scale requested a review from a team as a code owner June 2, 2026 16:58
@deepthi-rao-scale deepthi-rao-scale marked this pull request as draft June 2, 2026 17:00
Comment thread agentex/src/api/routes/states.py
Comment thread agentex/src/domain/use_cases/states_use_case.py Outdated
Comment thread agentex/tests/integration/api/states/test_states_authz_api.py
…gac-state-route-migration-enforce-via-parent-task

# Conflicts:
#	agentex/src/utils/authorization_shortcuts.py
Comment thread agentex/src/api/routes/states.py Outdated
@deepthi-rao-scale deepthi-rao-scale marked this pull request as ready for review June 3, 2026 14:50
read = "read"
update = "update"
delete = "delete"
manage_access = "manage_access"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we need to add this to agentex-auth, otherwise this will fail

state_id: DAuthorizedId(
TaskChildResourceType.state, AuthorizedOperationType.update
),
state_id: DAuthorizedId(TaskChildResourceType.state, _STATE_WRITE_OPERATION),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are you sure manage_access is the right operation here? maybe something new like create_child would be better

assert authz_data["resource"]["type"] == AgentexResourceType.task.value
assert authz_data["resource"]["selector"] == test_task.id
# AGX1-237 exposes manage_access as an owner-only task permission.
assert authz_data["operation"] == "manage_access"
Copy link
Copy Markdown
Contributor

@asherfink asherfink Jun 3, 2026

Choose a reason for hiding this comment

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

nit: use enum value rather than string, looks like greptile also flagged

Copy link
Copy Markdown
Contributor

@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.

just a few comments

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