Skip to content

fix(storage): cascade DeleteUser to all user-keyed tables - #749

Merged
lakhansamani merged 1 commit into
mainfrom
fix/747-delete-user-cascade
Aug 7, 2026
Merged

fix(storage): cascade DeleteUser to all user-keyed tables#749
lakhansamani merged 1 commit into
mainfrom
fix/747-delete-user-cascade

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Closes #747

Problem

StorageProvider.DeleteUser cascaded 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: jitProvisionFederatedUser resolves 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, so AddFederatedIdentity collides. Deleting the row by hand was the only fix.

Couchbase was worse: its DeleteUser deleted no children at all, not even sessions.

Fix

schemas.UserOwnedCollections is 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.

  • SQL keeps a parallel userOwnedModels (GORM deletes by model, not table name), all inside the existing transaction.
  • The five non-transactional backends delete children first and the user row last. They previously deleted the user first; with a partial failure that ordering manufactures exactly the orphan this issue is about, with no retry path. Children-first leaves the user intact and a retried delete completes the job.
  • Admin _delete_user also purges the user's FGA tuples (internal/service/fga.go). Tuples live outside StorageProvider, 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. A ponytail: comment names the ceiling and the upgrade path (enumerate the model's types via ReadModel, one filtered Read per type) if the store ever grows big enough to matter.

Not changed

Soft deletes do not cascade. DeactivateAccount and revoke-access only stamp RevokedTimestamp and never call DeleteUser — the account is meant to come back. This holds by construction, since the cascade lives entirely inside DeleteUser.

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

  • Cross-backend testDeleteUserCascade in 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.
  • Static parity check (TestDeleteUserCascadeIsUniform), mirroring TestNotFoundContractIsUniform: AST-asserts each backend's DeleteUser cascades 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.
  • SQL: userOwnedModelsUserOwnedCollections table-name alignment (via gorm.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.
  • Integration: end-to-end _delete_user with an FGA grant, asserting the tuple is gone.

Both halves were confirmed to actually catch the bug — dropping FederatedIdentity from the list fails the lockout regression; removing the purge call fails the tuple assertion.

Verification

go build, go vet, make test and make lint all clean.

Per AGENTS.md, the storage suite was run live against all six backends, not SQLite alone:

Backend Result
SQLite pass
ArangoDB pass
MongoDB pass
DynamoDB pass
ScyllaDB pass
Couchbase pass

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
lakhansamani merged commit 9c3ac81 into main Aug 7, 2026
4 checks passed
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DeleteUser cascade is incomplete: orphaned FGA tuples, org memberships, authenticators, passkeys and federated identities (SSO lockout)

1 participant