feat(authz): split the deployment-operator bit from tenant-scoped admin - #444
Conversation
|
Very nice.
The nine rows classify the explicit admin gates. But authz.can() short-circuits on is_admin before it looks at ownership or visibility, and can_manage() does the same. So the moment tenant_members.role feeds that bit, a workspace admin gains read, write and delete on every private room, reference, document and package in the workspace — including rooms they do not own — and management of every agent in it, including other people's. require_room_access is the widest of these and it is not one of the nine rows. Today this is invisible: in tenant zero, after the migration, the only owner/admin rows belong to deployment operators, who already had all of it. It becomes real the first time someone is invited as a workspace admin — which is the very next PR in this phase. I do not think it's wrong. I think it needs to be written down and decided deliberately, because the least obvious case is a private DM room between a colleague and their agent, and "workspace admins can read those" is a statement someone should make on purpose. Worth a paragraph in the phase 2 doc and a row in this table saying "all owned resources in the tenant, via authz.can".
The docstring defends it — "every gateway request binds one by the time this is reachable" — and for the gateway that's true. But this is also reachable from the agent bridge through _resolve_acting_identity, and #442 (merged two PRs ago, and specifically praised for this) makes the opposite call for the same condition: it raises rather than proceeding on an unbound tenant, and it found a real caller that was unbound (ConnectorCore.delete_agents()). CLAUDE.md's rule is the same one. The failure is restrictive rather than an escalation — a workspace owner becomes a non-admin, so the symptom is an unexplainable 403 rather than access they shouldn't have — which is why I'd raise it as a follow-up rather than a blocker. But it's the quiet kind of wrong: nothing in the logs would say why. Either raise, as #442 does, or at minimum logger.warning. There's also no test for this branch, so nothing pins whichever behaviour you choose.
across agents.py (7), packages.py (12), rooms.py:76, room_groups.py:119, templates.py:168, connectors.py:142, auth.py:302. In packages.py:137/159/235/310/405, agents.py:510 the handler already takes user_store: Annotated[UserStore, Depends(get_user_store)] and ignores it. This PR is internally inconsistent about it in three directions: documents.py takes Depends(get_tenant_is_admin), collaborations.py uses Depends(require_tenant_admin), references.py and rooms.py:251/409 use the injected user_store — and then 25 sites do none of those. get_tenant_is_admin was added by this PR for exactly this and is used five times. It's harmless today only because UserStore happens to be stateless. It stops being harmless the moment it isn't, and it already means app.dependency_overrides[get_user_store] does not reach these paths — test_change_password.py:58 overrides with a specific instance, so the pattern is one test away from a confusing miss. Against CLAUDE.md's "stores and services are injected, not global singletons". Mechanical to fix: Depends(get_tenant_is_admin) on the handlers, the injected user_store in the helpers. |
petr-sandbox
left a comment
petr-sandbox
left a comment
There was a problem hiding this comment.
Looks good. Just a few nitpicks.
`users.role == "admin"` was a global bypass and `tenant_members.role` was written at provisioning and never read. So making the creator of a workspace its owner would have granted nothing, and anything that set the global bit would have made that person an administrator of every workspace they later joined. The global bit now means what it honestly is — a deployment operator, never granted by anything self-service. `Principal.is_admin` becomes "may administer the tenant this request is bound to": the operator bypass, or an owner/admin membership row in that tenant. An agent still inherits exactly its owner's power, now in the bound tenant rather than everywhere. Each of the nine admin gates was classified as deployment-wide or workspace-scoped rather than converted uniformly; the two that enumerate or create deployment users stay operator-only. Design: docs/old/multi-tenancy-phase2-tenants.md section 2.
Three review points on the role split, plus the collision the rebase onto main exposed. **The grant is wider than a list of routes.** `authz.can` and `can_manage` short-circuit on `Principal.is_admin` before ownership or visibility, so the moment `tenant_members.role` feeds that bit a workspace owner or admin holds read, write and delete on every owned resource in the workspace — private rooms included, via `require_room_access`, which is not an admin-gated route at all. That is a decision about what an administrator may see, not a consequence of a route list, so it is written down in the phase 2 note: accepted for this phase, with the narrower "administers configuration" bit left to whichever phase takes on resource-level sharing. **`UserStore.administers` refuses an unbound tenant** rather than answering on the operator bit alone. Half the question is a membership row that cannot be read without a tenant; answering anyway silently demotes a workspace owner, and the 403 that follows names no cause. Same call #442 made on the bridge-identity path. Two tests pin it, including for an operator, whose bypass is the one answer that would have been right and so the worst one to give. **No handler builds its own `UserStore` any more.** Twenty-five did, six while a real one sat in their signature. Now route handlers take `is_admin: Annotated[bool, Depends(get_tenant_is_admin)]` and non-route helpers take a plain `bool`, so authz I/O leaves the helpers entirely and a route cannot authorize against a different bit than its own dependencies resolved. Six `user_store` dependencies that the rewrite left with no reader are gone with them. Rebasing first paid for itself: this branch's test fake still implemented `all_bridges()`, which #442 removed when it merged — green apart, red together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All three review comments are addressed. The work is on 1. What a workspace admin actually gets. The comment also asked for a row in the PR-description table. That is your PR body, so I have not edited it — proposed row: | Workspace admin ( 2. tenant_id = current_tenant_id()
if tenant_id is None:
raise RuntimeError(
"administers requires a bound tenant; whether someone may "
"administer a workspace is only answerable about a particular one"
)The fallback would quietly demote a workspace owner to a plain member, and the symptom — a 403 on their own workspace — says nothing about why. Two tests pin it ( This surfaced exactly the unbound caller you predicted: 3. The tenant-admin bit is resolved once, in the dependency. Touched: Tests call these coroutines directly, so ~60 call sites across 10 test files now pass the bit. They resolve it through the store ( Verification: Why a separate branch. This branch was still based on Those two make the rebased commit the only coherent unit; cherry-picking it onto the un-rebased tip conflicts. And updating this branch in place would mean a force-push, which would rebase one member of a four-deep stack and orphan the bases of #445–#447. That is the stack's merge strategy, which is yours to pick — so the branch is pushed alongside instead of over anything. Point this PR at it, cherry-pick 🤖 Generated with Claude Code |
4c4ce57 to
0fc9869
Compare
2 participants
Summary
users.role == "admin"was a global bypass — read byauthz.can(), byrequire_admin, and by everyPrincipalconstruction — whiletenant_members.rolewas written at provisioning and never read anywhere.That makes two things Phase 2 depends on impossible. Making the creator of a workspace its
ownerwould grant nothing, so a person could not administer the workspace they had just created. And anything that set the global bit would make that person an administrator of every workspace they later joined — which invitations and domain joining both make reachable.So: the global bit narrows to what it honestly is, a deployment operator, never granted by anything self-service.
Principal.is_adminnow means "may administer the tenant this request is bound to" — the operator bypass, or anowner/adminmembership row in that tenant.authz.administers_tenant()is the one place a role becomes an admin bit;UserStore.administers()composes it with the single membership read that can answer "in this tenant".An agent still inherits exactly its owner's power — now in the bound tenant rather than everywhere.
Nothing changes for anyone today: current administrators keep the operator bypass, and a person with one membership and no operator bit sees identical behaviour.
The classification, which is the part to argue with
Each admin gate is either a deployment-wide action or a workspace-scoped one. Converting them uniformly would have been wrong in one direction or the other: permissive is a privilege escalation, restrictive locks an operator out of their own deployment.
GET /usersuserscarries no tenantPOST /usersPOST /collaborationscollaboration_bridgesis tenant-scopedPOST /collaborations/{id}/defaultPATCH /collaborations/{id}DELETE /collaborations/{id}POST /collaborations/{id}/identitiesDELETE …/identities/{id}DELETE /connectors/{id}The two operator-only gates are the two that reach beyond a single workspace. Everything else acts on tenant-scoped rows and belongs to whoever administers that workspace.
Migration
8ef6d4038eccbackfills anownermembership in tenant zero for any user carrying the operator bit whose row is missing or stale. It is a label fix rather than a lockout fix — the operator bypass is unconditional either way — and it is idempotent.Test plan
just check,just typecheck— pass.administers_tenantto the oldreturn is_operatorfails five of them, includingtest_workspace_owner_can_delete_a_room_they_do_not_personally_own(assert can(Principal(id=…, is_admin=False), 'delete', room)) andtest_owner_has_no_power_in_a_tenant_they_only_belong_to_as_member. Restored, and they pass.Note
bridges/agent/auth.pyis deliberately untouched — #435 is mid-flight in that file.