fix(auth): one actor per human per org - #581
Merged
Merged
Conversation
An org-subdomain sign-in resolved the caller by `(org_id, external_id)`, then by email, and minted a fresh identity when both missed — without ever asking whether that human already held an actor in the org. A founder identity from `POST /v1/orgs` carries no `external_id` and the address the account had at signup, so an IdP reporting a different address today misses both keys and forks a second identity linked to the same `users` row. The person's agents, grants and audit trail split across the halves. Nothing then picked between them deterministically: `find_by_org_and_user` is a `fetch_optional`, which takes the first row and discards the rest, so the `sub` in the session JWT on org switch, the connect-gate account match, the OAuth consent target and billing ownership all became planner-order dependent. Resolve the account once, up front, and adopt by `user_id` before adopting by email — an existing actor outranks a pending invite for the new address, and membership is already settled, so the branch returns ahead of the admission gate rather than asking a member to be re-admitted. Then make the fork unrepresentable. `identities_org_user_unique` is the partial UNIQUE `docs/design/multi_org_auth.md` specifies, that `find_by_org_and_user`, `remove_user`'s detach-on-archive and migration 043's rationale all already assume — and that no migration ever created; 040 shipped only a plain index on `user_id`. It lands second on purpose: alone it would have converted the silent fork into a 500 at login. Migration 115 heals before it constrains, keeping the oldest live actor per (org, human) and detaching the rest with `user_id = NULL` — the same detach `remove_user` performs, so no subtree or audit history is destroyed. The heal is not optional: `overslash-prod-db` is private-IP only, so a bare CREATE UNIQUE INDEX would risk failing the deploy on data we cannot inspect from a workstation. Also drops `idx_identities_email_lookup`: 043 recreated the email index believing it had to replace a UNIQUE it could not ALTER, but the index it dropped was 013's `idx_identities_user_email` while 013's plain `idx_identities_email` had been there all along — two byte-identical indexes on `identities(email) WHERE email IS NOT NULL`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Code diff size+105 / −17 across 2 files (net +88)
Source files under |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Seer caught this on the adopt-by-user branch, and it was a real regression. Archiving a user identity revokes access — it revokes the identity's API keys, and `restore_identity` is sub-agent-only, so there is no way back for `kind = 'user'` — but only `remove_user` detaches `user_id`. A plain archive leaves the row linked, so the new account lookup found it and handed the same human a fresh session. Before this PR that case fell through to the admission gate and forked a second actor; now it would have resurrected the first. Neither is right. Guard both lookups that can surface an archived row — by IdP subject and by account. The subject fast-path had the same hole already, independent of this PR: `find_user_by_external_id_in_org` never filtered `archived_at` either, and leaving it unguarded would make admission depend on whether the row happened to carry an `external_id`. Adopt-by-email needs nothing: its query already filters `archived_at IS NULL` in SQL, which is the rule the other two were missing. Refuse rather than fall through: the archived row still holds the `(org, human)` slot, so minting a second actor would collide with `identities_org_user_unique` and answer 500 where 403 is the truth. Re-admission stays an admin action — remove the member and re-invite, which detaches the tombstone on the way out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What went wrong
overfolder-devon the dev deployment had three user identities carryingamanuelmartincanto@gmail.com. Two are legitimate (the founder identity fromPOST /v1/orgs, and the one the Overfolder backend provisions with its ownusers.idasexternal_id). The third was a fork: on 2026-05-11, between flippingallow_overslash_managed_signinon at 17:04 and off again at 17:17, a test login on the org subdomain minted a second identity at 17:06 —external_id = 113738577913051785749(the Googlesub), linked to the sameusersrow as the founder identity. It has zero audit rows since. It's now deleted from dev; this PR stops it recurring.The hole
provision_org_subdomainresolves the caller by(org_id, external_id), then by email, and mints a fresh identity when both miss — without ever asking whether that human already holds an actor in the org. A founder identity carries noexternal_idand the address the account had at signup, so an IdP reporting a different address today misses both keys and forks. #487's adopt-by-email converges the two only when the address still matches.Nothing then picked between the halves deterministically.
find_by_org_and_useris afetch_optional, which returns the first row and discards the rest (sqlx query_as.rs:192— no error on multi-row), so the fork silently made four things planner-order dependent:subin the session JWT on org switch (auth/session.rs:235)connect_gate.rs:118)oauth/consent.rs:379)billing/webhook.rs:154,192)The fix, in two layers
1. Adopt-by-user. Resolve the Overslash account once, up front, and check
find_by_org_and_userbefore adopt-by-email. Ordering is deliberate: if adopt-by-email ran first it could land on a pending invite for the new address and link that row to a human who already owns an identity here — the same fork by another route. Checking the account first makes the existing actor win, which is right; they hold the agents, the grants and the audit trail. Membership is already settled, so the branch returns ahead of the admission gate — an existing member whose email changed must not be asked to be re-admitted, or rejectednot_invited.2.
identities_org_user_unique. The partial UNIQUEdocs/design/multi_org_auth.mdspecifies, thatfind_by_org_and_user,remove_user's detach-on-archive and migration 043's rationale all already assume — and that no migration ever created. 040 shipped only a plain index onuser_id.The order matters: the index alone would have been a foot-gun. With the code fix stashed, the new regression test fails
500rather than303— the constraint converts the silent fork into a hard failure at login. Code first, constraint as the backstop.Migration 115
Heals before it constrains: keeps the oldest live actor per
(org, human)and detaches the rest withuser_id = NULL— the same detachremove_useralready performs on archive, so no subtree, grant or audit row is destroyed. The heal is not optional:overslash-prod-dbis private-IP only, so a bareCREATE UNIQUE INDEXrisks failing the deploy on data that can't be inspected from a workstation. Plain (notCONCURRENTLY) is correct here — the table is small and sqlx wraps each migration in a transaction, so heal + constrain are atomic.Dry-run on a purpose-built pre-115 database seeded with the
overfolder-devshape:user_id IS NULL)Down-migration reverts and re-applies cleanly.
Also drops
idx_identities_email_lookup. Migration 043 recreated the email index believing it had to replace a UNIQUE it couldn'tALTER— but the index it dropped was 013'sidx_identities_user_email, while 013's plainidx_identities_emailhad been there all along. Two byte-identical indexes onidentities(email) WHERE email IS NOT NULL, paid for on every insert.Testing
member_whose_idp_email_changed_is_adopted_not_forked— the fork, end to end through the Google mock. Fails500without the code fix.a_human_cannot_hold_two_actors_in_one_org— the constraint as an invariant, including what the partial predicate must still allow (unlinked invites, one actor per org for the same human).Not done here
SCHEMA.sqlturned out to be ~230 lines stale — missingorgs.require_invite_admissionandmanaged_signin_allowed_domainsfrom migration 092, several column comments, D56 where the comments now read D59. Regenerating it wholesale would bury a 4-line schema change, so this PR patches only theidentitiesindexes (in pg_dump's own sort position, so a later regen stays minimal) and writes the drift up inTECH_DEBT.md:make schemaplus a CI job that diffs a freshly migrated dump wants its own PR.🤖 Generated with Claude Code