feat(gateway): workspace, invitation and member routes (CHOO-2722)

[wojtyniakAQ]

Rebased onto main

#444, #445 and #446 are all merged, so this is no longer stacked — base is main and the diff is this PR's own work.

Summary

The rest of the tenant API: create a workspace, mint / list / revoke / accept invitations, list members, change a role, remove a member.

• POST /tenants goes through the existing ClientLifecycleService.create_tenant seam rather than a raw insert, so a new workspace gets the admin client every tenant needs. A taken slug is a 409, never a silent suffix.
• Accepting an invitation runs in its own session. The request is already stamped with the caller's current workspace; inserting a membership for a different one on it is refused by the policy and rebinding raises. So POST /invitations/accept resolves the token's tenant through the lookup and does its work in a tenant_session of its own. Revocation, expiry, remaining uses and the bound address are all checked.
• Who owns a workspace is decided by its owners, not its admins — see below.
• A workspace can never lose its last owner — removal and demotion both refused.
• Removing a member revokes their personal keys in the same transaction as the membership row, then invalidates the cache.

The privilege escalation the review found

An admin could take a workspace from its owner in three requests: promote self to owner, demote the founder, remove them. The last-owner guard never fired, because there genuinely were two owners at every step — each request was individually safe and the sequence was not.

The fix is a second, narrower bit beside administers_tenant: authz.owns_tenant. An admin runs the workspace; only an owner moves the ownership set. Granting owner, changing an existing owner's role, removing an owner, and minting an owner invitation all answer to it, on top of the owner/admin gate that lets the request in at all. test_an_admin_cannot_take_the_workspace_from_its_owner runs the three requests and asserts both memberships are untouched afterwards.

The rest of the review, fixed

• Two concurrent accepts of a single-use invitation both succeeded. Accept now goes through InvitationStore.consume's conditional UPDATE (feat(db): invitations table and the lookup that resolves one to a tenant (CHOO-2722) #446) rather than the read-then-write copy this branch was carrying. Worth naming how that got here: the rebase onto main textually merged a second consume into the store, shadowing the safe one — a clean auto-merge that silently replaced the correct implementation. The test run caught it; git did not.
• A use is spent only when a membership is actually granted. consume had been running unconditionally, so a double-clicked shared link burned a seat for nothing. It now sits inside the existing_role is None branch.
• The token moved from the path to the body. It is a bearer credential, and a URL segment is written to proxy logs, browser history and Referer.
• expires_in_hours is bounded above. 1000000000 passed gt=0 and then raised OverflowError inside timedelta — a 500 for what should be a 422.
• The "every route that opens a session is authenticated" guard had a second door. It keyed on get_system_session, which the routes taking get_session_factory and opening their own sessions walk straight past. It now keys on the absence of get_current_user, with an explicit allowlist of the ten routes that resolve no tenant, so the next door is a failing test rather than a route nobody counted.
• The 409 on removing a member who owns agents no longer offers reassignment. Nothing in this phase reassigns an agent's owner, so the message was sending admins looking for a button that does not exist.

The one I did not fold, disclosed rather than hidden

POST /tenants commits the tenant row and then writes the owner membership in a second transaction. It is not one transaction because the tenant has to be committed before ensure_system_client can provision an admin client against it, and that call is inside the seam; folding it means moving membership into ClientLifecycleService and threading a UserStore through nine construction sites, which is a refactor rather than a fix.

What the gap gets instead is a logger.error naming the workspace id, the slug and the user who should have owned it, so an orphan is findable and repairable rather than silent. Say if you'd rather have the refactor.

The one decision worth arguing with (unchanged)

An earlier version of this deleted every agent the removed member owned, because there is no way to disable an agent and its bearer key cannot be freed while the agent still references it. That was rejected. An agent is not a private possession: it sits in rooms with other members, it may be the only copy of a working configuration, and removing someone from a workspace is a routine administrative act that gets done by mistake. A deleted API key is reminted; a deleted agent is not. So removal refuses, naming the agents, and changes nothing.

ProtocolService.delete_agent is deliberately not used anywhere in this path — it spans several sessions and commits and calls out to the live client and the bridges, none of which folds into one transaction.

Known gap, disclosed rather than hidden

Removal does not tear down a live session an agent already holds. Its next call fails authentication because the credential is gone, but one in flight continues. Judged acceptable; say if it should block instead.

Test plan

• just check, just typecheck — pass. Full suite: 3188 passed, 6 skipped, 1 xfailed, 0 failed.
• The three new guards each proved to bite by reverting the production code: with _require_owner neutered the escalation succeeds (201/200 where 403 is asserted); with the le= bound removed the absurd expiry reaches OverflowError: date value out of range rather than a 422; with consume moved back outside the branch the re-accept test finds uses_remaining == 0 instead of 1.
• The seven guards from the first pass still bite, unchanged.
• The route-coverage test was failing on arrival after the rebase — feat(gateway): workspace, invitation and member routes (CHOO-2722) #447 adds routes that resolve no tenant, which is exactly what it exists to notice.
• The refusal path is asserted to change nothing: membership, agent, the agent's key and an unrelated personal key are all still there afterwards.

Two repo invariants caught real mistakes in the first pass

test_there_is_one_way_to_write_a_membership caught membership rows being constructed directly in handlers, and test_tenant_exemption_allowlist caught the privileged tenant lookup being reached from the wrong module. Both fixed properly rather than by weakening the tests.

🤖 Generated with Claude Code

[petr-sandbox]

1. An admin can take a workspace away from its owner in three requests. Reproduced.

update_member_role is gated by require_tenant_admin, and beyond the last-owner check it places no limit on which role an admin may grant or to whom — including themselves. I added a test:

PATCH /tenants/A/members/{self}        {"role": "owner"}   → 200
PATCH /tenants/A/members/{real_owner}  {"role": "member"}  → 200   (count_owners is 2 by now)
DELETE /tenants/A/members/{real_owner}                     → 200

Final state: ADMIN NOW owner, ORIGINAL OWNER NOW REMOVED. The last-owner guard never fires, because at every step there genuinely are two owners — the admin just made sure of it first.

This makes owner and admin the same role with extra steps, and it hollows out the invariant #439 states as a guarantee ("a workspace must always have an owner", §5): the workspace still has an owner, just not the one it started with. Granting or removing owner should require the caller to be an owner, not merely an admin. Same argument for create_invitation — an admin can currently mint an owner invitation.

2. Two concurrent accepts of a single-use invitation both succeed. Reproduced, first attempt, not flaky.

InvitationStore.consume is a Python read-modify-write:

invitation.uses_remaining -= 1

Two POST /invitations/{token}/accept in an asyncio.gather, uses_remaining=1:

RESULT 200 {...,"role":"member"}
RESULT 200 {...,"role":"member"}
ADMITTED BY ONE-USE INVITE: 2
USES REMAINING NOW: 0

Note the last line, because it changes the fix I suggested on #446: the counter landed on 0, not −1. The ORM writes the absolute value from each session's own snapshot, so both issued SET uses_remaining = 0 — a CHECK (uses_remaining >= 0) would not have caught this either. The database has to arbitrate the decrement itself:

UPDATE invitations SET uses_remaining = uses_remaining - 1
WHERE id = :id AND uses_remaining > 0 RETURNING uses_remaining

with a zero rowcount meaning "already used"; or, if you'd rather keep the checks where they are, with_for_update() on the get_by_token_hash in the accept path so the whole validate-and-consume serialises. The first is better — it puts the invariant in the store, which is where consume's docstring already implies it lives.

This is not a theoretical window. A shareable link with uses_remaining=N is precisely the object someone pastes into a channel where several people click it within the same second.

For what it's worth I probed the last-owner guard the same way — two concurrent demotions of a two-owner workspace — and could not break it; one got its 409. I'm not claiming it's proven safe, only that it isn't the same easy target.

3. The "no tenant bound" guard now has a second door it cannot see.

test_session_requires_authentication.py keys every one of its four assertions on get_system_session, and says so deliberately: "get_system_session exists so that adding a fourth is a visible act, not a signature nobody reads." But GET /tenants, POST /tenants and POST /invitations/{token}/accept don't take get_system_session — they take get_session_factory and open sessions themselves. None of the three appears in _ROUTES_WITH_NO_TENANT_BOUND, and none is visible to the test.

accept is the one that matters: it opens a tenant_session bound to a tenant chosen by the submitted token and writes a membership row into it. That is the most consequential unbound-session route in the codebase, and the test written to make such routes conspicuous does not know it exists. Either add get_session_factory to the discovery and declare all three, or flip the predicate to "takes neither get_session nor get_current_user", which catches the next one too.

4. The invitation token travels in the URL path.

POST /invitations/{token}/accept. It is a bearer credential that grants workspace membership, and a path segment ends up in load-balancer and reverse-proxy access logs, in browser history, and in the Referer header of whatever the accept page loads next. #446 went to real trouble to keep only a hash so the plaintext exists in exactly one place; putting it in the path puts it back in several. POST /invitations/accept with {"token": …} in the body costs nothing.

5. The 409 on member removal tells the admin to do something no route can do.

I agree with the decision — refusing beats deleting someone's agents, for exactly the reasons in the description. But the message says "reassign or delete them first", and nothing in the codebase reassigns an agent's owner. There is no gateway route that writes agents.owner_id (the only writes are owner_id=user.id at registration), and update_agent_detail on the MCP side is owner-only and doesn't touch it either. So the admin's only actual option is to delete the agents — the irreversible act the refusal exists to prevent — or to leave the member in the workspace indefinitely.

That doesn't change the decision, but it does mean the reassign route has to ship alongside it, or the 409 is a dead end. At minimum the message should not offer an action that doesn't exist.

6. A user-supplied expires_in_hours can 500. expires_in_hours=1000000000 passes gt=0, then datetime.now(UTC) + timedelta(hours=…) raises OverflowError (verified). An le= on the field.

7. Accepting an invitation you already hold still burns a use. The existing_role is None check skips the membership insert but consume runs unconditionally, so a double-click on a shared link costs the invite a slot. Move the consume inside the branch.

8. create_tenant can leave an ownerless workspace. ClientLifecycleService.create_tenant commits the tenant row and provisions the admin client; the membership is a separate session and a separate commit. If that second commit fails, the workspace exists with no members — absent from GET /tenants, unreachable by anyone, and with no deletion route until Phase 5. If folding them isn't practical, a logger.error naming the tenant id at least makes it findable.

[petr-sandbox]

Rebased onto main (base retargeted; #444–#446 are all merged) and all eight findings are addressed. af22ad1f is the fix commit; full suite is 3188 passed, 0 failed.

1 — privilege escalation. Fixed as you'd expect: authz.owns_tenant beside administers_tenant, a get_tenant_is_owner dependency, and a _require_owner helper applied to granting owner, changing an existing owner's role, removing an owner, and minting an owner invitation. The last one matters as much as the other three — otherwise the same escalation is the same three steps with a link in the middle. test_an_admin_cannot_take_the_workspace_from_its_owner fires all three requests and asserts both memberships are untouched; test_an_owner_can_do_all_of_it asserts the guard is narrower authority rather than a locked door.

2 — the double accept. Took your first option. Accept now calls main's consume(session, invitation.id) and maps InvitationNotUsableError to 403.

Worth flagging how the unsafe version got here at all: the rebase textually auto-merged a second consume into InvitationStore, appended after revoke, shadowing the conditional-UPDATE one #446 added. A clean merge with no conflict that silently replaced the safe implementation with the racy one. #446's own store tests caught it on the first run after the rebase; git had nothing to say.

_require_invitation_usable stays, but its docstring now says what it is for: the message, not the decision. Three of its four gates are re-checked inside the UPDATE that actually settles the race; the fourth — the addressed email — is the one gate only it applies, because the store has no idea who is asking.

3 — the second door. Took your preferred option. The predicate is now the absence of get_current_user, with an explicit ten-route allowlist, rather than the presence of any particular alternative — because the alternatives keep arriving: keying on get_system_session missed the get_session_factory routes, and keying on get_authenticated_user_id missed get_authenticated_caller. There's a second test asserting that those two dependencies are the only authenticated way into the set, so a route that invents a third is caught rather than inheriting the exemption quietly.

4 — the token in the URL. POST /invitations/accept with the token in the body, via a new InvitationAcceptRequest. Design note updated too.

5 — the 409 that promises reassignment. Reworded to "delete them first", and the docstring now says explicitly that there is no reassignment route in this phase, so the next person to read it doesn't re-add the promise.

6 — expires_in_hours. le=8760 on the field. Verified the old behaviour: without the bound it is OverflowError: date value out of range, i.e. a 500.

7 — a use burned on re-acceptance. consume moved inside if existing_role is None:. test_re_accepting_an_invitation_you_already_hold_costs_nothing mints a two-use invitation, accepts twice from the same caller, and asserts uses_remaining == 1.

8 — the ownerless-workspace window. This is the one I did not fold, and I'd like a second opinion. One transaction means moving the membership write into ClientLifecycleService.create_tenant, which means threading a UserStore through its constructor and nine call sites — and the tenant row has to be committed before ensure_system_client can provision against it regardless, so the seam does not currently have a "before commit" to hook into. That is a refactor, not a fix, and it argues against the service's own docstring ("it provisions the tenant, not any particular person's place in it").

So I took your fallback, slightly widened: the membership write is wrapped, and a failure logs an error naming the tenant id, the slug and the user who should have owned it, then re-raises. The orphan is findable and repairable instead of silent. Happy to do the refactor in a follow-up if you'd rather not carry the window.

Also: owns_tenant has unit tests in test_authz.py including the one that states the point — an admin administers but does not own — and the design note now records the owner-only rule and why the last-owner guard cannot substitute for it.

Each new guard was verified to bite by reverting the production code: escalation succeeds with _require_owner neutered, the expiry 500s without the bound, and the re-accept test reads uses_remaining == 0 with consume back outside the branch.

[petr-sandbox]

Looks good.