fix(storage): cascade DeleteUser to all user-keyed tables - #749
Merged
Conversation
DeleteUser cascaded to sessions only, so a hard delete left orphans in six tables. An orphaned authorizer_federated_identities row is a permanent SSO lockout: jitProvisionFederatedUser resolves a returning principal through it, fails closed once the user id is dead, and the unique (org_id, issuer, subject) triple blocks re-provisioning. - schemas.UserOwnedCollections is the single source of truth; all six backends cascade over it. Couchbase deleted no children at all. - Non-transactional backends delete children first and the user row last, so a partial failure leaves the user retryable, not orphaned. - Admin _delete_user also purges the user's FGA tuples. OpenFGA's Read rejects a user-only filter, so it pages the store and matches client-side; synchronous, best-effort, logged. - Soft deletes (deactivate_account, revoke access) do not cascade. Verified live on all six backends, not SQLite alone. Closes #747
lakhansamani
added a commit
that referenced
this pull request
Aug 7, 2026
…essions Pre-release audit remediation for 2.4.0: findings F1-F5, F11, F14. (Prefixed F- rather than written bare: a `#1` renders as a link to an unrelated issue.) F1/F2 nOAuth account takeover via social login Federated logins resolved a local account from a provider-asserted email with no attestation behind it. Entra v2 tokens carry no email_verified at all and `email` is a mutable directory attribute, so a free attacker-owned tenant could assert a victim's address and land in their session. Every provider now reports its own real signal, Microsoft tokens are pinned to a trusted tenant (or xms_edov), and an unattested address may not select a local account. --oauth-allow-unverified-provider-email is a narrowed compatibility ramp: it still cannot cross into an account another credential owns. F3 verification-token purpose confusion Magic-link, signup, invite and forgot-password tokens shared one table keyed by token alone, and no consumer checked purpose. A leaked magic link was redeemable at ResetPassword for durable account takeover. Gated at all three consumers, not just the GraphQL pair: GET /verify_email is a separate implementation of the same flow and is the URL the mails actually point at, so gating only the mutations would have left a forgot-password token redeemable there for a full session. F4/F14 admin session and secret The admin cookie was bcrypt(AdminSecret): no expiry, no revocation, logout could not invalidate a captured copy. Now an opaque server-side handle. Both admin-secret comparison paths share one throttled gate. It counts FAILED attempts only — it runs on every x-authorizer-admin-secret request, so counting successes would 401 a concurrent admin API client presenting the correct secret. MetaFromGRPC now falls back to the gRPC peer address, so the counter is keyed per caller instead of collapsing to one bucket shared by every gRPC client (where a handful of wrong guesses locked out all of them). F5 per-account login lockout, F11 single-use TOTP passcodes (RFC 6238 5.2). Also bounds the pre-hijack delete to accounts holding no state. The cascade (#749) destroys those rows rather than orphaning them, but an unauthenticated callback still must not take out an account's org memberships, MFA enrollment or FGA grants to resolve an email collision, and FGA tuples are outside the cascade entirely. Makes resend_verify_email able to mint a fresh request, and makes email verification with no SMTP a fatal misconfiguration.
lakhansamani
added a commit
that referenced
this pull request
Aug 7, 2026
…essions (#748) Pre-release audit remediation for 2.4.0: findings F1-F5, F11, F14. (Prefixed F- rather than written bare: a `#1` renders as a link to an unrelated issue.) F1/F2 nOAuth account takeover via social login Federated logins resolved a local account from a provider-asserted email with no attestation behind it. Entra v2 tokens carry no email_verified at all and `email` is a mutable directory attribute, so a free attacker-owned tenant could assert a victim's address and land in their session. Every provider now reports its own real signal, Microsoft tokens are pinned to a trusted tenant (or xms_edov), and an unattested address may not select a local account. --oauth-allow-unverified-provider-email is a narrowed compatibility ramp: it still cannot cross into an account another credential owns. F3 verification-token purpose confusion Magic-link, signup, invite and forgot-password tokens shared one table keyed by token alone, and no consumer checked purpose. A leaked magic link was redeemable at ResetPassword for durable account takeover. Gated at all three consumers, not just the GraphQL pair: GET /verify_email is a separate implementation of the same flow and is the URL the mails actually point at, so gating only the mutations would have left a forgot-password token redeemable there for a full session. F4/F14 admin session and secret The admin cookie was bcrypt(AdminSecret): no expiry, no revocation, logout could not invalidate a captured copy. Now an opaque server-side handle. Both admin-secret comparison paths share one throttled gate. It counts FAILED attempts only — it runs on every x-authorizer-admin-secret request, so counting successes would 401 a concurrent admin API client presenting the correct secret. MetaFromGRPC now falls back to the gRPC peer address, so the counter is keyed per caller instead of collapsing to one bucket shared by every gRPC client (where a handful of wrong guesses locked out all of them). F5 per-account login lockout, F11 single-use TOTP passcodes (RFC 6238 5.2). Also bounds the pre-hijack delete to accounts holding no state. The cascade (#749) destroys those rows rather than orphaning them, but an unauthenticated callback still must not take out an account's org memberships, MFA enrollment or FGA grants to resolve an email collision, and FGA tuples are outside the cascade entirely. Makes resend_verify_email able to mint a fresh request, and makes email verification with no SMTP a fatal misconfiguration.
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.
Closes #747
Problem
StorageProvider.DeleteUsercascaded to sessions only. Every other row keyed on the user id survived, so a hard delete left orphans in six tables.The federated-identity orphan is a permanent SSO lockout, not just untidiness:
jitProvisionFederatedUserresolves a returning principal through the(org_id, issuer, subject)row, finds it points at a user id that no longer exists, and fails closed on every subsequent login. Re-provisioning cannot recover it either — the triple is unique, soAddFederatedIdentitycollides. Deleting the row by hand was the only fix.Couchbase was worse: its
DeleteUserdeleted no children at all, not even sessions.Fix
schemas.UserOwnedCollectionsis the single source of truth — sessions, federated identities, org memberships, authenticators, passkeys, session tokens, MFA sessions. All six backends cascade over it, so a new user-keyed table is covered everywhere by adding one line.userOwnedModels(GORM deletes by model, not table name), all inside the existing transaction._delete_useralso purges the user's FGA tuples (internal/service/fga.go). Tuples live outsideStorageProvider, so the storage cascade cannot reach them.Two deliberate calls on the FGA purge
Synchronous, not
asyncutil.Go. It stays best-effort and logged as the issue proposed — a tuple-store failure does not fail a delete that already happened — but it runs inline. A deleted user must not be observably still holding grants, and async made the regression test racy.It pages the whole tuple store and matches client-side. OpenFGA's Read API rejects a user-only filter — it requires at least an object type (
pkg/server/commands/read.go: "the object type field is required") — and we do not know which types a user appears under. Aponytail:comment names the ceiling and the upgrade path (enumerate the model's types viaReadModel, one filtered Read per type) if the store ever grows big enough to matter.Not changed
Soft deletes do not cascade.
DeactivateAccountand revoke-access only stampRevokedTimestampand never callDeleteUser— the account is meant to come back. This holds by construction, since the cascade lives entirely insideDeleteUser.accountHasState(oauth_account_state.go) is left as-is. It exists because this cascade was incomplete, but relaxing a security bound is a separate decision — refusing is recoverable, deleting is not.Tests
testDeleteUserCascadein the storage suite: create a user with a membership, TOTP authenticator, passkey, federated identity, session token and MFA session; delete; assert every row is gone. Includes the lockout regression — the same(org, issuer, subject)triple must be free for re-provisioning.TestDeleteUserCascadeIsUniform), mirroringTestNotFoundContractIsUniform: AST-asserts each backend'sDeleteUsercascades over the shared list. This is the check that would have caught the Couchbase gap — CI runs SQLite only, so a cascade covering six tables on SQL and none on Couchbase stays green forever.userOwnedModels↔UserOwnedCollectionstable-name alignment (viagorm.Statement.Parse, not hand-guessed snake-casing), plus an atomic-rollback test proving a failed cascade step does not leave the user row deleted on its own._delete_userwith an FGA grant, asserting the tuple is gone.Both halves were confirmed to actually catch the bug — dropping
FederatedIdentityfrom the list fails the lockout regression; removing the purge call fails the tuple assertion.Verification
go build,go vet,make testandmake lintall clean.Per AGENTS.md, the storage suite was run live against all six backends, not SQLite alone: