fix(auth): agent-config and self-service identity routes are human-only, with a route census (#2996) - #3044
Conversation
Records the rule in security.md §20.10 ahead of the change: POST /api/mcp/keys and POST /api/mcp/keys/ensure-default take a signed-in (JWT) session, and ensure-default writes the same key_create audit row as the create route.
…person, require_interactive Two allowlist rules over mcp_scope, both fail-closed on a principal without one: PERSON (a JWT session or the person's own user-scoped key, via is_person_principal) and INTERACTIVE (a JWT session only). The Depends forms make a route's rule a one-line signature change an AST guard can see. Both also refuse a principal carrying vouched_source_agent. The ent#611 ask-endings detail and predicate are unchanged, and pinned so.
POST /api/mcp/keys (every scope) and POST /api/mcp/keys/ensure-default now take Depends(require_interactive): a credential minter is at least as strict as the principal it produces. The existing per-scope checks stay as they were. ensure-default also writes the same key_create audit row as the create route, so every created key is attributable. Tests go through the real get_current_user with seeded key rows and assert the mcp_api_keys row count is unchanged on every refusal.
…t registry mcp-api-keys.md: POST /keys and ensure-default are signed-in-session only; ensure-default is audited as key_create. Adds the feature-flows index row, a learnings fragment and the two new test files to tests/registry.json.
…he route census requirements/auth.md §2.8 (and the §2.7 helper table): the PERSON and INTERACTIVE rules, which routes take which, the primitives, and the route census with its shrink-only baseline. architecture/security.md §5 and §6, and one sentence on Invariant #8. Refs #2996, trinity-enterprise#711
PUT /api/agents/{name}/autonomy takes Depends(require_person): a JWT
session or the person's own user-scoped key. Agent-scoped keys (on
their own agent or any other), the system key and every other scope get
a 403 with the human-only detail, before the owner check, so the
refusal discloses nothing about whether the agent exists.
Autonomy decides whether an agent's cron schedules fire unattended; it
is a grant, so the agent's own key must not be able to switch it on.
Tests go through the real get_current_user with seeded key rows and
assert the stored autonomy flag before and after.
Refs #2996
…2996) PUT api-key-setting, read-only, resources, capabilities, capacity, timeout, public-channel-model and guardrails take Depends(require_person), the same rule as autonomy: the same file, the same owner configuration surface, and no agent or system caller. Each write is tested from its own non-ambient stored value: machine keys through the real get_current_user leave it unchanged, a person changes it. A router-level check pins that these and autonomy are every PUT in the file. Refs #2996
…a signed-in session PUT /api/users/me/email and PUT/DELETE /api/users/me/github-pat take Depends(require_interactive). The email is the account's sign-in identity and the PAT is the credential future agent creations inherit; both are credential/identity bindings, so no MCP key may change them, the person's own user key included. Tests: every key kind through the real get_current_user, with the stored email and encrypted PAT checked before and after; the session path (400 on a bad shape, 409 on a taken address, set and clear) unchanged; and a record that email sign-in resolves the account by the email column alone. Refs trinity-enterprise#711
…2996) Every OSS backend route, every method, must resolve to exactly one class: gated in code (interactive / person / admin tier / widened admin / portal), listed with a reason (agent-callable / own-auth / delegated), or in the frozen baseline, which only shrinks. A new route that is neither gated nor classified fails the build, so the next human-only route inherits the rule instead of relying on someone remembering it. The AST side is import-free (rglob over src/backend, minus the private submodule). The runtime side imports main in a subprocess and checks every stored METHOD /path against the live table; it fails, never skips.
The human-only rule covers the autonomy grant, not the schedule tools it bounds. Pins create/enable/disable with the agent's own key through the real principal resolution, asserting the stored state moves.
Autonomy, read-only, resources, capabilities, capacity, guardrails, public-channel model and the API-key setting flows note the person-only rule on their writes; email-authentication, first-time-setup and github-sync note the session-only rule on the sign-in email and the personal GitHub PAT. Index rows, a learnings fragment, and the schedule regression test in auth.md §2.8. Refs #2996, trinity-enterprise#711
…tes call (#2996) The read-only route imports its logic lazily from sys.modules, and a sibling unit file evicts that entry at import, so under some orders the route ran an unpatched copy and 404'd. The fixture now resolves and pins the real module, and patches the bound logic through the globals of the functions the router holds.
Drop the sign-in resolution test class and its registry note: they pinned behaviour outside this change. Reword the census and MCP-boundary notes neutrally; the baseline is a to-do marker, not a judgement.
… only (#2996) An inherited PYTHONPATH (the verify-local unit stage sets one) let the subprocess resolve `main` from another tree, so the fail-loud check on a missing app could not fail.
dolho
left a comment
dolho
left a comment
There was a problem hiding this comment.
Approving. I reviewed only this PR's own commits (pr-3043...pr-3044). The primitives it builds on are covered in the #3043 review.
Verification. The 5 new test files: 194 passed. The runtime census takes about 10s and can't skip.
What I checked
- All 9 agent-config writes now use
Depends(require_person).PUT /me/emailandPUT/DELETE /me/github-patnow userequire_interactive. set_autonomy_status_logicis the only caller ofdb.set_autonomy_enabled, so there's no side door to toggle autonomy.- No gated route is called by the MCP server, the agent base image, the scheduler, the CLI or the
trinity-systemtemplate. The UI calls them with a JWT. The canary-fleet runbook'scapacity/timeoutcalls still work with a JWT oruserkey. - These routes are pinned as
agent_callable: heartbeat, result callback, reports, notifications, and schedule create/enable/disable. - The 403 depends only on who the caller is, so it's enumeration-safe (Invariant #8 / #186).
- The tests are strong: real keys resolved against the stored value, self-tests that plant broken source files to show each ratchet rule catches them, a check of the live route table, and a pin that an agent key can still manage its own schedules.
Findings (minor or nits)
-
Minor. These routes are grants of the same kind this PR fences, but they stay agent-callable in the baseline:
routers/git.py:652(PUT /api/agents/{name}/github-pat)routers/agents.py:1095(circuit-breaker)routers/agents.py:1247(mcp-exposed)
The per-agent PAT bind is the sharpest. It is the per-agent version of
/users/me/github-pat, which this PR makes interactive-only, and it has noreject_agent_principalat all. Suggest it goes first in the follow-up. -
Minor:
tests/unit/_route_census.py:626andtests/unit/fixtures/README.md. The exact-count, shrink-only baseline has no path for a pure rename or move, such as an Invariant #1 package split. The only way out is--freeze, which quietly re-baselines everything. Please document a rename rule (the sameMETHOD /pathwith a new key allowed in the same diff), or assert that a re-freeze never adds aMETHOD /paththat wasn't already in the baseline. -
Nit:
_route_census.py:~507-612. Every new route now edits one shared table, so expect merge conflicts. Make the ratchet's failure message name the exact table to add the route to. -
Nit. Handlers already guarded by the imperative
reject_agent_principal(for exampleagent_files.py::set_agent_permissionsandconnector.py::regenerate_connector_key) still count asunclassified_at_freeze. That's correct, since it's a denylist, but the fixtures README should say so, or readers may assume those routes are open.
|
merge-train (non-blocking, for a follow-up): |
|
merge-train: ejected from train #3070 — rides the next train once fixed.
Also heads-up: #3028 (native asks) is riding this train and adds Classifying these (gate vs AGENT_CALLABLE / OWN_AUTH / DELEGATED with a reason) is a grant-vs-use judgement, so it's left to you rather than done mechanically. The code itself validated READY; merge order after #3043 still stands. |
|
Resolve by running |
|
Resolve by merging |
…e-2996 # Conflicts: # tests/registry.json
…sus (#2996) Routes that landed on dev after the branch was cut were unclassified. Skill-set assign/unassign are the USE of the ent#596 skills-manage capability (fenced by get_skill_managed_agent_by_name; the grant is the admin+interactive /skill-manager route), the two skill-set reads are open reads, and raise_my_ask is MCP ask_operator, self-only via get_self_acting_agent. All five go in AGENT_CALLABLE with a reason.
An agent key can no longer PUT /api/agents/{name}/timeout, so the cap
refusal on schedule and loop timeouts now says the owner sets it.
…e-2996 # Conflicts: # tests/registry.json
|
merge-train: merged |
…e-2996 # Conflicts: # tests/registry.json
3 participants
Fixes #2996
Refs trinity-enterprise#711
Summary
Owner-tier grant routes and self-service identity routes are now human-only, and a route census makes every new route pick a side.
Depends(require_person): a JWT or the person's ownuserkey):PUT /api/agents/{name}/autonomyand the other agent-config writes (api-key-setting,read-only,resources,capabilities,capacity,timeout,public-channel-model,guardrails).Depends(require_interactive): JWT only):PUT /api/users/me/email, andPUT/DELETE /api/users/me/github-pat.tests/unit/test_2996_human_only_routes.py. Every OSS HTTP route, every method, resolves to one policy class, or sits in a shrink-only baseline (tests/unit/fixtures/human_only_route_baseline.json, exact count).test_2996_route_census_runtime.pychecks it against the real app.Why per route and not inside the owner gates: unlike the admin tier (#1890), agents legitimately use owner-gated routes (the MCP schedule tools, file writes).
test_2996_agent_schedule_tools_unchanged.pypins that an agent key still manages its own schedules.Merge order: M → A → B
This PR includes PR M's commits (the first 5) because it is stacked on it. Merge M first; I'll rebase this PR onto
devafterwards so only its own commits remain. PR B (ent#712) follows this one and updates the census baseline. See the PR comment for why these are three PRs.Journey Impact: none: the UI and CLI call these routes with a JWT, and agent-used routes are unchanged
Type of Change
Testing
test_1310_*,test_293_*,test_186_*,test_1854_*) the run is green on 3 seeds.systemadded to the person scopes 11.tests/unitshows the same 50 failures on this branch as ondev, with none unique to the branch./reviewand/cso --diffboth ran; their findings are fixed or filed privately.backend-unit-test.yml(every PR to dev/main, not label-gated). None of the new tests is markedslow.After merge
trinity-enterprise#711is cross-repo, so close it by hand at the release cut.🤖 Generated with Claude Code