feat(gateway): let a session select its tenant among several memberships (CHOO-2723)

[wojtyniakAQ]

Stacked on #444

Base is work/split-operator-and-workspace-roles, not main. Review #444 first; this only makes sense on top of it.

Summary

Sign-in resolves the caller's single tenant_members row and returns 403 for anyone holding two — a deliberate Phase 1 guard, and the thing that makes "pick a workspace" impossible. This replaces it.

• The session carries the choice. The gateway JWT gains a tenant claim, and all the mint sites carry it. /auth/refresh is the one that matters: it re-minted from the user alone, so without this a refresh would silently drop the selection and drop a multi-workspace person into the choose-one path mid-session.
• _resolve_tenant_id becomes the five ordered cases from the design: claim with a membership binds it; claim without one is 403; no claim and one membership binds it, exactly as today; no claim and several is the choose-one response; no memberships is 403. The 403 deliberately does not clear the cookie — it is lax, so a cross-site navigation reaches that path, and clearing there would let any page reset someone's selection.
• GET /tenants and POST /tenants/{id}/switch. Switching verifies membership before re-minting; nobody can select a workspace they do not belong to.
• sole_tenant_id and TenantMembershipError are gone. The tests asserting "two memberships is an error" were inverted rather than deleted — that is precisely the behaviour this removes, so it is what the suite should now prove is gone.

Nothing changes for anyone until we say so

The choose-one response is a breaking change for every client at once, and Console's workspace switching is not in this phase. So it is behind GATEWAY_TENANT_CHOICE_ENABLED, default false — with the flag off, several memberships behave exactly as they do today. Documented in .env.example.

The listing constraint, which is the interesting part

GET /tenants needs each workspace's name and the caller's role, from two tables a bound session can read for exactly one tenant. The obvious implementation loops bound sessions inside the handler — and the request session already holds its connection, so that needs a second one concurrently. TestARequestHoldsOneConnection pins one per request with a single-connection pool precisely to stop that creeping in.

So the listing authenticates without binding a tenant (a new dependency that verifies the JWT and the user and stops there), reads the tenant ids once, then reads each workspace in its own short bound session, sequentially. One connection at any moment. The same routine produces the choose-one body, because it is the same question asked at a different moment.

Test plan

• just check, just typecheck — pass. Full suite: 3121 passed, 6 skipped, 1 xfailed, 0 failed.
• Every new behaviour proved to bite, by reverting the production change and watching the test fail:
  • Forcing the claim to None: test_a_claim_binds_even_with_several_memberships got 403 instead of 200, and test_a_claim_naming_no_membership_is_refused got 200 instead of 403.
  • Reverting refresh to mint without the claim: the re-minted token's tenant came back None.
  • Rewriting the listing to hold every tenant's session concurrently: sqlalchemy.exc.TimeoutError: QueuePool limit of size 1 overflow 0 reached, connection timed out.
• That last one was verified twice, because the first attempt at breaking it did not actually hold two connections and the test passed against code that should have failed. A test that cannot fail is worse than no test, so it was restructured until it did.

[petr-sandbox]

Nothing blocking.

1. _resolve_tenant_id's docstring says one round trip; case 4 makes two.

▎ tenants_of_user is read once and used for every case below it, rather than once per case, so this is one round trip to the exemption regardless of which case answers.

The 409 branch then calls list_tenant_memberships, which opens with for tenant_id in await tenants_of_user(session_factory, user_id). So the choose-one path reads the exemption twice. Either pass the ids down or correct the sentence — but this is the function that gates every authenticated request, and its comment is the sort this repo treats as load-bearing.

2. Case 2's 403 tells the caller the wrong remedy — and member removal is what makes it common.

▎ This account is not a member of the selected tenant; ask an administrator to check its membership.

The caller can fix this themselves: GET /tenants and POST /tenants/{id}/switch both still work, because neither goes through get_current_user. Signing in again also clears it, since login mints a null claim. So the message sends someone to an administrator for a problem they can resolve in one click.

That matters more than it sounds, because removing a member — which ships in this phase — puts everyone it touches into exactly this state on their next request. The wording is a leftover from case 5, where "ask an administrator" genuinely is the answer; the two cases now need different sentences.

3. The 409 body has no discriminator, and this is the moment to add one.

detail is a bare list of tenants. FastAPI's own validation failures are also {"detail": [...]}, so a client distinguishing them has only the status code to go on, and 409 is a plausible status for a future conflict elsewhere in the API. The description makes the case itself — this is "a breaking change for every client at once" — so shaping the body now ({"error": "tenant_selection_required", "tenants": [...]}) costs nothing and avoids doing it twice. Same for giving case 2's 403 a machine-readable code, which is what the SPA in point 2 would branch on.

4. get_authenticated_user_id is a new way to authenticate without binding a tenant, and it has no guard test.

Its two siblings both do: get_system_session has test_the_routes_with_no_tenant_bound_are_exactly_these_three, and the lookup exemption has test_tenant_exemption_allowlist, whose docstring in this very diff argues that reachability is what matters, not current use. get_authenticated_user_id is precisely a reachable way to skip the tenant check. Today it is used by exactly the two routes in tenants.py, which is correct; nothing pins that. It wants the same three-line exact-match test as get_system_session — and by the allowlist docstring's own reasoning, more than either of them.

5. Nothing pins that login mints a null claim.

test_refresh_carries_the_selected_tenant_forward covers the mint that carries a tenant. The two that pass a literal None — password login and the OIDC callback — have no test on the claim. That None is the recovery path from a stale selection (point 2), so if someone later made login "helpfully" carry the previous tenant forward, the suite would stay green and a removed member would be 403'd until their cookie expired. Two asserts.

6. Two dangling references to the removed symbol. db/tenant_lookup.py:142 and docs/old/multi-tenancy-phase1-db.md:1243 both cite get_sole_tenant_id — which was never even the name, so they were already stale by one rename. The doc is a historical record and can arguably stay; the source comment should go.

[petr-sandbox]

Looks good.

[petr-sandbox]

Rebased onto main (now that #444 has merged) and pushed the review fixes. Base retargeted from work/split-operator-and-workspace-roles to main, so the diff is this PR's own work again.

1. The docstring's "one round trip" is now true. Split _describe_memberships out of list_tenant_memberships: it takes the tenant ids rather than looking them up, so case 4 reuses what _resolve_tenant_id already read instead of asking the exemption a second time. GET /tenants still calls list_tenant_memberships, which does the lookup and delegates — the exemption's import surface is unchanged, which is why the ids are passed down rather than the lookup moved into tenants.py. Pinned by test_the_choose_one_body_costs_no_second_lookup, which counts the calls.

2 & 3. All three refusals are discriminated, and case 2 stops misdirecting the caller.

	status	error
claim names no membership	403	tenant_selection_invalid
several memberships, none selected	409	tenant_selection_required (choices under tenants)
no memberships, or several with the flag off	403	tenant_membership_unresolved

Exported as constants from gateway/auth.py so the tests reference the contract rather than re-spelling it.

Case 2's message is now "The workspace selected on this session is not one you belong to. Choose another, or sign in again." — you were right that this is the common 403 once member removal ships, and that the caller can resolve it in one click. The test asserts the word "administrator" is absent, so the old wording cannot drift back.

Nothing in gateway/ or console/ reads these bodies yet, so reshaping now costs nothing — which was your point.

4. get_authenticated_user_id gets the guard. test_the_routes_skipping_tenant_resolution_are_exactly_these_two, same exact-match shape as test_the_routes_with_no_tenant_bound_are_exactly_these_three. Added a second one too: test_no_route_both_resolves_a_tenant_and_skips_resolving_one — a route taking both would run the membership check and then be free to ignore its answer.

5. Login and the OIDC callback are pinned to a null claim. TestSigningInSelectsNoWorkspace::test_password_login_mints_no_tenant_claim and an assert on the existing OIDC callback test. Both docstrings say why: that null is the recovery path out of a stale selection.

6. tenant_lookup.py's get_sole_tenant_id reference is gone. Left the doc alone as a historical record, per your read.

Verification. just check, just typecheck (243 files) clean; full suite 3131 passed, 6 skipped, 11 deselected, 1 xfailed. Every new assertion was confirmed to fail against the unfixed code — login and the OIDC callback minting "tenant-0" instead of None, a third route taking get_authenticated_user_id alongside get_current_user, and restoring the second tenants_of_user call (assert 2 == 1).

One note for the stack: #446 and #447 are still based on the pre-rebase work/split-operator-and-workspace-roles, so their diffs currently show #444's changeset too until they get the same treatment.

🤖 Generated with Claude Code