Skip to content

fix(identity): make admin remove-user atomic, cascade sys_member, and map DELETE_RESTRICTED to a 409 - #7879

Merged
huangyiirene merged 4 commits into
mainfrom
claude/issue-7724-remove-user-atomicity
Aug 12, 2026
Merged

fix(identity): make admin remove-user atomic, cascade sys_member, and map DELETE_RESTRICTED to a 409#7879
huangyiirene merged 4 commits into
mainfrom
claude/issue-7724-remove-user-atomicity

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Fixes#7724

All three compounding problems, plus the status-code leak. The envelope fix is deliberately not allowed to stand in for the atomicity fix — per the recorded scope, "a structured 409 on an operation that can never succeed is still an operation that can never succeed".

1. The referential veto — sys_member.user_id now declares deleteBehavior: 'cascade'

A lookup defaults to set_null, and the engine escalates a defaultedset_null on a REQUIRED foreign key to restrict (packages/objectql/src/engine.ts, cascadeDeleteRelations). user_id is required and nothing declared a behaviour, so the membership the reconciler creates for every user at sign-up vetoed every sys_user delete — remove-user could never succeed on any deployment.

The audit the card asked for, before adding the cascade: nothing depends on the restrict. It is specifically not an accidental last-administrator guard. That invariant is enforced by a beforeDelete hook registered on sys_member itself (last-admin-guard.ts, ADR-0024 D5.2), and the engine's cascade recurses through the publicdelete() precisely so the child's own hooks and events fire. The guard therefore still refuses a cascade that would take the last administrator's standing away — it just refuses it one row deeper.

Per #7796 this also removes memberships that invitation acceptance adopted, which is correct and is covered by a dedicated test.

2. The non-atomicity — the harmful half

Verified at source rather than inferred: better-auth 1.7.0-rc.2's internalAdapter.deleteUser (dist/db/internal-adapter.mjs) deletes sessions, then accounts, then the user, in three separate adapter calls. The string transaction does not occur in that file at all. So anything refusing the third call leaves the first two committed — credential rows gone, sys_user row still on the org roster, that identity permanently un-authenticatable.

Subject-erasure requests now run inside one engine transaction (ObjectQL.transaction, ADR-0034), opened at AuthManager.handleRequest — a seam the platform owns. Re-implementing better-auth's route was rejected for the reason #7725 gives: duplicated security checks are where bypasses live. This wrapper reads no bodies and makes no authorization decision.

One subtlety worth flagging for review: better-auth returns its faults as a Response rather than throwing, so a plain try/catch unit of work would commit on exactly the path it exists to undo. The rollback is therefore triggered from the response status and the response carried back out through a private sentinel.

The route set is the existing exported SESSION_ERASURE_PATHS, not a new list.

Two declared limits, both inherited rather than introduced, and both documented at the call site: a driver with no beginTransaction degrades with the engine's existing warning (failing closed was considered and rejected — it would make removal impossible on those datasources, the very defect being fixed); and non-datasource side effects are not transactional.

3. The bodyless 500

The PM's hypothesis was correct and is confirmed — the fix belongs at the adapter, not at the REST mapping. rethrowAsBetterAuthError in objectql-adapter.ts mapped engine validation errors and policy refusals, but a referential DELETE_RESTRICTED carries neither signature and fell through to throw err, which better-auth's router renders as a 500 with an empty body. A third arm maps it to APIError('CONFLICT') carrying developerMessage / dependentObject / dependentCount.

packages/rest/src/rest-server.ts is untouched. Its mapDataError already handles this code correctly for the generic data routes; the two transports map the one engine error independently, exactly as they already did for the other two arms.

Verification — predicted before mutating, then measured

Every pin's ablation direction was written down before the mutation was run.

AblationPredictedMeasured
drop deleteBehavior: 'cascade'tests 1, 2 and the declaration pin go red with 409 DELETE_RESTRICTEDmatched exactly, plus one unpredicted red (see below)
drop the DELETE_RESTRICTED arm3 adapter unit tests red; end-to-end red as a bodyless 500matched exactly — expected 500 to be 409, and the response body is empty
bypass the transaction wrapperrollback test red, credential rows stay deletedmatched — expected [] to deeply equal [ '7a34…' ]

Two honest deviations, reported rather than smoothed over:

  • The cascade ablation also turned the "structured body" test red with expected 409 to be 403. Not a false pin: with the restrict live, the membership veto fires before that test's own injected refusal, so a different — still structured — refusal wins.
  • The transaction ablation also turned the 409 test red on its trailing "refused AND clean" assertion. That is the card's central point in one line: a correct 409 that still leaves the credential rows deleted has not fixed this.

The ablation also incidentally confirmed the fake is faithful: with the declaration removed the field reads back set_null, which the fake escalates to restrict exactly as the engine does.

The fake engine is driven by the real declaration, not by a constant. It reads SysMember.fields.user_id and reproduces the engine's own arithmetic over it, so reverting the object file changes the fake's behaviour. Its transaction really snapshots and restores. Both write verbs are pinned to assertEngineDeleteDispatch / assertEngineUpdateDispatch.

Local gates run: @objectstack/plugin-auth 1048/1048, @objectstack/platform-objects 339/339, both typecheck clean, check:nul-bytes, check:docs-audit-scope, check:engine-double-contract, check:error-code-casing, and eslint --no-inline-config on every changed file. Consumer sweep in the prefix direction (--filter '...@objectstack/platform-objects' --filter '...@objectstack/plugin-auth' — 51 packages, the ones that DEPEND ON these): build and typecheck clean, tests exit 0 with 19,135 passed.

typecheck caught a real defect the green suite could not see: the composed scope helpers are typed Promise< Promise< Response > >, which the previous single call site flattened with its await.


Generated by Claude Code

…E_RESTRICTED
Three compounding problems on the better-auth admin remove-user path (#7724).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D6Qi9sYxhaRwj7TYiD5MWg
@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 1:49am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/platform-objects, @objectstack/plugin-auth.

9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/deployment/cli.mdx(via @objectstack/plugin-auth)
  • content/docs/deployment/production-readiness.mdx(via @objectstack/plugin-auth)
  • content/docs/kernel/contracts/cache-service.mdx(via @objectstack/plugin-auth)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/plugin-auth)
  • content/docs/permissions/authentication.mdx(via @objectstack/plugin-auth)
  • content/docs/permissions/sso.mdx(via @objectstack/plugin-auth)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-auth)
  • content/docs/plugins/packages.mdx(via @objectstack/platform-objects, @objectstack/plugin-auth)
  • content/docs/ui/setup-app.mdx(via @objectstack/platform-objects)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-auth)
  • content/docs/releases/v9.mdx(via @objectstack/plugin-auth)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 12, 2026
@huangyiirene
huangyiirene marked this pull request as ready for review August 12, 2026 08:28
@huangyiirene
huangyiirene added this pull request to the merge queueAug 12, 2026
Merged via the queue into main with commit e7a7506Aug 12, 2026
26 checks passed
@huangyiirene
huangyiirene deleted the claude/issue-7724-remove-user-atomicity branch August 12, 2026 08:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

admin-lifecycle: remove-user is non-atomic and can never succeed — 409 leaks as a bodyless HTTP 500, credential rows deleted without rollback

2 participants

@huangyiirene@claude