From f7fb92ca9bfc1f064c90f46e90a1a7ac135c380c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 00:47:09 +0000 Subject: [PATCH 1/4] fix(identity): make remove-user atomic, cascade sys_member, map DELETE_RESTRICTED Three compounding problems on the better-auth admin remove-user path (#7724). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D6Qi9sYxhaRwj7TYiD5MWg --- .../src/identity/sys-member.object.ts | 21 +++ .../plugins/plugin-auth/src/auth-manager.ts | 121 +++++++++++++++++- .../plugin-auth/src/objectql-adapter.ts | 69 +++++++++- 3 files changed, 202 insertions(+), 9 deletions(-) diff --git a/packages/platform-objects/src/identity/sys-member.object.ts b/packages/platform-objects/src/identity/sys-member.object.ts index aee44eb20e..9373f1ae31 100644 --- a/packages/platform-objects/src/identity/sys-member.object.ts +++ b/packages/platform-objects/src/identity/sys-member.object.ts @@ -164,6 +164,27 @@ export const SysMember = ObjectSchema.create({ user_id: Field.lookup('sys_user', { label: 'User', required: true, + // [#7724] A membership without its user is meaningless, so deleting the + // user takes its memberships with it. This must be DECLARED: a `lookup` + // defaults to `set_null`, and the engine escalates a *defaulted* + // `set_null` on a REQUIRED foreign key to `restrict` (you cannot null a + // NOT NULL column). That escalation vetoed every `sys_user` delete on any + // deployment where the membership reconciler had run — i.e. all of them, + // since `reconcile-membership.ts` binds every user to the default org at + // sign-up, and (since #7796) invitation acceptance ADOPTS that same row. + // So `/admin/remove-user` could never succeed, and the operator could not + // clear the blocker by hand either: `enable.apiMethods` below is read-only. + // + // Audited before declaring it, because the engine's own error naming + // `deleteBehavior:'cascade'` is a suggestion, not an audit: nothing + // depends on the restrict. In particular it is NOT an accidental + // last-administrator guard — that invariant is enforced by a `beforeDelete` + // hook registered on `sys_member` itself (ADR-0024 D5.2, + // `last-admin-guard.ts`), and the engine's cascade recurses through the + // PUBLIC `delete()` 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 simply refuses it one row deeper. + deleteBehavior: 'cascade', }), // [ADR-0108 / #3723] The framework's four roles — the WHOLE list. Nothing diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 1962e22fb8..989a4531b3 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -22,6 +22,7 @@ import { postureEnforcesWall, type TenancyPosture } from '@objectstack/spec/secu import { MCP_OAUTH_SCOPES } from '@objectstack/spec/ai'; import { createObjectQLAdapterFactory, withSystemReadContext } from './objectql-adapter.js'; import { runWithAuthActorScope, setAuthActorResolver } from './auth-actor-attribution.js'; +import { SESSION_ERASURE_PATHS } from './session-tombstone.js'; import { invitationRoleCapFailure, isPlainMemberInvitation, @@ -165,6 +166,26 @@ function installWebContainerRequestStatePolyfill(): void { } } +/** + * [#7724] Carries better-auth's own error `Response` out through the engine + * transaction that must roll back because of it. + * + * better-auth's HTTP entrypoint CATCHES every fault and RETURNS a `Response` — + * it does not throw. A `try`/`catch`-shaped unit of work therefore sees a clean + * return on the exact path it exists to undo, commits, and the partial writes + * land anyway. So the failure signal has to be re-raised from the response + * status, and the response itself has to survive the throw that rolls the + * transaction back — that is the whole job of this class. It never escapes + * `runSubjectErasureAtomically`, which unwraps it back into the response the + * client was always going to get. + */ +class SubjectErasureRollback extends Error { + constructor(readonly response: Response) { + super(`subject-erasure unit of work rolled back (HTTP ${response.status})`); + this.name = 'SubjectErasureRollback'; + } +} + function readBooleanEnv(name: string, legacyName?: string): boolean | undefined { const env = (globalThis as any)?.process?.env as Record | undefined; const raw = env?.[name] ?? (legacyName ? env?.[legacyName] : undefined); @@ -3069,9 +3090,25 @@ export class AuthManager { // costs nothing: the scope starts empty, the before-hook drops a resolver // in, and the session is looked up only if some write asks. Attribution // only — the authorization subject of those writes is unchanged (system). - const response = await runWithAuthActorScope(() => - runWithRequestState(new WeakMap(), () => auth.handler(request)), - ); + const runHandler = (): Promise => + runWithAuthActorScope(() => + runWithRequestState(new WeakMap(), () => auth.handler(request)), + ); + + // [#7724] A subject-erasure request is ONE unit of work, and better-auth + // does not treat it as one: `internalAdapter.deleteUser` deletes the + // sessions, then the accounts, then the user, in three unrelated adapter + // calls with no transaction (verified in better-auth 1.7.0-rc.2 — + // `dist/db/internal-adapter.mjs` mentions no transaction at all). Anything + // that refuses the LAST of those three leaves the first two committed: the + // credential rows are gone, the `sys_user` row is not, and the deployment + // is left with an identity that still occupies the org roster and can no + // longer sign in. Nothing tells the operator, and there is no way back. + const endpointPath = this.betterAuthEndpointPath(request); + const response = + endpointPath !== undefined && SESSION_ERASURE_PATHS.has(endpointPath) + ? await this.runSubjectErasureAtomically(runHandler) + : await runHandler(); if (response.status >= 500) { try { @@ -3085,6 +3122,84 @@ export class AuthManager { return response; } + /** + * The better-auth endpoint path (`/admin/remove-user`) this request addresses, + * or `undefined` when it is not under the configured `basePath`. + * + * The same spelling better-auth's own `ctx.path` uses, so the sets keyed by it + * — `SESSION_ERASURE_PATHS`, the break-glass guard's path tests — are all + * talking about one thing. + */ + private betterAuthEndpointPath(request: Request): string | undefined { + let pathname: string; + try { + pathname = new URL(request.url).pathname; + } catch { + return undefined; + } + const configured = this.config.basePath || '/api/v1/auth'; + const base = (configured.startsWith('/') ? configured : `/${configured}`).replace(/\/+$/, ''); + if (!pathname.startsWith(base)) return undefined; + const endpoint = pathname.slice(base.length).replace(/\/+$/, ''); + return endpoint.startsWith('/') ? endpoint : undefined; + } + + /** + * [#7724] Run a subject-erasure request as ONE unit of work: every write it + * makes commits together, or none of them do. + * + * Placed at the REQUEST seam rather than inside better-auth's route, because + * re-implementing `/admin/remove-user` here would duplicate its permission + * check, its self-removal check and its not-found check — and a duplicated + * security check is where the two copies drift apart. This wrapper reads no + * bodies and makes no authorization decision; better-auth's handler runs + * exactly as before, and the only thing added is the transaction it runs in. + * + * The engine's `transaction()` (ADR-0034) publishes its handle into the + * ambient store, so every adapter write on the way down joins it without the + * adapter knowing — which is why this needs no change in `objectql-adapter.ts`. + * + * Two declared limits, both inherited rather than introduced: + * - a datasource whose driver has no `beginTransaction` runs the callback + * with no transaction and no rollback (ADR-0119 D1). The engine warns once + * per driver. Failing CLOSED instead (`{ require: true }`) was considered + * and rejected: it would make user removal impossible on those datasources, + * which is the very defect this card is fixing. + * - side effects outside the datasource (a sent email, secondary-storage + * session state) are not transactional and are not undone by a rollback. + * `/admin/remove-user` sends nothing, so no path here relies on it. + */ + private async runSubjectErasureAtomically( + run: () => Promise, + ): Promise { + const engine = this.config.dataEngine as + | (IDataEngine & { + transaction?: (callback: (trxCtx: any, info: any) => Promise) => Promise; + }) + | undefined; + // `transaction` is an ObjectQL capability, not an `IDataEngine` member — an + // engine without it (a test double, a foreign engine) keeps the previous + // behaviour rather than being refused. + if (typeof engine?.transaction !== 'function') return run(); + + try { + return await engine.transaction(async () => { + const response = await run(); + // better-auth RETURNS its faults; see `SubjectErasureRollback`. Any 4xx/5xx + // means the erasure did not complete, so whatever part of it already + // landed must not survive. 2xx commits; so does the 302 that + // `/delete-user/callback` answers with on success. + if (response.status >= 400) throw new SubjectErasureRollback(response); + return response; + }); + } catch (err) { + // The rollback has happened by the time this runs — hand the client the + // response better-auth composed, now with no partial writes behind it. + if (err instanceof SubjectErasureRollback) return err.response; + throw err; + } + } + /** * Get the better-auth API for programmatic access * Use this for server-side operations (e.g., creating users, checking sessions) diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index df2df55235..5f8d6f2e84 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -497,10 +497,51 @@ function isEnginePolicyRefusal(err: unknown): err is { code?: string; message?: return (err as { code?: unknown }).code === 'PERMISSION_DENIED'; } +/** + * [#7724] A REFERENTIAL refusal — the engine's `cascadeDeleteRelations` found + * dependent rows it may neither cascade nor null, so it vetoed the delete + * (`DELETE_RESTRICTED`, 409, ADR-0112). + * + * The third shape in this file, and the one that shows why the set had to be + * widened rather than left at two. The two arms above both map errors raised by + * code that *knows about better-auth* — the record validator and this package's + * own policy guards. A referential restrict is raised by the ENGINE, several + * layers below, and carries neither signature; `rethrowAsBetterAuthError` fell + * through to `throw err`, better-auth's router saw an unhandled fault, and the + * admin caller got a **500 with an empty body** for a refusal the engine had + * explained in full. The client is told nothing at all — not the status, not the + * dependent object, not the remedy. + * + * Mapped HERE, at the adapter, rather than at the REST transport: this is the + * seam where an engine error crosses into better-auth, so one arm covers every + * better-auth endpoint that deletes through the adapter. `rest-server.ts`'s + * `mapDataError` already maps the same code correctly for the generic data + * routes and is deliberately untouched — the two transports map the one engine + * error independently, exactly as they already do for the two arms above. + * + * The structured half of the envelope rides along unchanged (`developerMessage` + * / `dependentObject` / `dependentCount`), for the reason #7307 gives at the + * REST mapping: dropping the remedy at the transport moves the defect rather + * than fixing it, and the fields disclose nothing the envelope did not carry. + */ +function isReferentialDeleteRestriction( + err: unknown, +): err is { + code?: string; + message?: string; + developerMessage?: string; + dependentObject?: string; + dependentCount?: number; +} { + if (!err || typeof err !== 'object') return false; + return (err as { code?: unknown }).code === 'DELETE_RESTRICTED'; +} + /** * Re-throw `err` as a better-auth `APIError` when it is an ObjectQL validation - * failure or an engine policy refusal; otherwise re-throw it verbatim. Always - * throws — the return type is `never`. + * failure (400), an engine policy refusal (403) or a referential delete + * restriction (409); otherwise re-throw it verbatim. Always throws — the return + * type is `never`. */ async function rethrowAsBetterAuthError(err: unknown): Promise { if (isObjectQLValidationError(err)) { @@ -525,15 +566,31 @@ async function rethrowAsBetterAuthError(err: unknown): Promise { code: 'PERMISSION_DENIED', }); } + if (isReferentialDeleteRestriction(err)) { + const { APIError } = await import('better-auth/api'); + throw new APIError('CONFLICT', { + message: + typeof err.message === 'string' && err.message.trim() + ? err.message + : 'Cannot delete: dependent records exist', + code: 'DELETE_RESTRICTED', + ...(typeof err.developerMessage === 'string' && err.developerMessage.length > 0 + ? { developerMessage: err.developerMessage } + : {}), + ...(err.dependentObject ? { dependentObject: err.dependentObject } : {}), + ...(typeof err.dependentCount === 'number' ? { dependentCount: err.dependentCount } : {}), + }); + } throw err; } /** * Wrap every function-valued method of a better-auth adapter so an ObjectQL - * `ValidationError` (400) or an engine policy refusal (403) thrown from the - * underlying engine surfaces as a 4xx `APIError` instead of an opaque 500. - * Non-function properties pass through untouched, and every error that carries - * neither signature is re-thrown verbatim. + * `ValidationError` (400), an engine policy refusal (403) or a referential + * delete restriction (409, #7724) thrown from the underlying engine surfaces as + * a 4xx `APIError` instead of an opaque 500. Non-function properties pass + * through untouched, and every error that carries none of those signatures is + * re-thrown verbatim. */ export function withValidationErrorMapping>(adapter: A): A { const out: Record = {}; From 8c2258f742226a68ec3ebb8b65fde23fe7a94fcb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 00:56:03 +0000 Subject: [PATCH 2/4] test(plugin-auth): pin remove-user atomicity, cascade and 409 mapping Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D6Qi9sYxhaRwj7TYiD5MWg --- .../plugin-auth/src/objectql-adapter.test.ts | 86 ++++ .../src/remove-user-atomicity.test.ts | 465 ++++++++++++++++++ 2 files changed, 551 insertions(+) create mode 100644 packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.test.ts b/packages/plugins/plugin-auth/src/objectql-adapter.test.ts index 8f0f36d1c7..9e47f42510 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.test.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.test.ts @@ -460,6 +460,92 @@ describe('withValidationErrorMapping – ObjectQL ValidationError → better-aut await expect(adapter.update()).rejects.toBe(boom); }); + // [#7724] The third arm. A referential veto is raised by the ENGINE, well + // below the layers that know better-auth exists, so it carried neither the + // validation envelope nor the policy-refusal code and fell through to + // `throw err` — reaching an admin as a 500 with an EMPTY body for a refusal + // the engine had explained in full. + describe('a referential delete restriction (DELETE_RESTRICTED) → 409', () => { + // Faithful mimic of the engine's envelope (`packages/objectql/src/engine.ts`, + // ADR-0112 + #7307's message split). + const restricted = () => { + const err: any = new Error('Cannot delete User: 1 or more Member records still reference it.'); + err.code = 'DELETE_RESTRICTED'; + err.status = 409; + err.object = 'sys_user'; + err.dependentObject = 'sys_member'; + err.dependentCount = 3; + err.developerMessage = + 'Cannot delete sys_user: 3 dependent sys_member record(s) reference it via user_id ' + + "(user_id is required, so it cannot be cleared). Delete or reassign them first, " + + "or set deleteBehavior:'cascade' on sys_member.user_id."; + return err; + }; + + it('maps it to a 409 APIError instead of letting it escape as a bodyless 500', async () => { + const adapter = withValidationErrorMapping({ + delete: async () => { + throw restricted(); + }, + }); + + let caught: any; + try { + await adapter.delete(); + } catch (e) { + caught = e; + } + + // Both halves of the envelope, per ADR-0112: a throw alone cannot tell + // "refused with the wrong envelope" apart from "refused correctly" — + // the unfixed path throws too, it just throws something better-auth + // cannot render. + expect(isAPIError(caught)).toBe(true); + expect(caught.statusCode).toBe(409); + expect(caught.body).toMatchObject({ + code: 'DELETE_RESTRICTED', + message: 'Cannot delete User: 1 or more Member records still reference it.', + }); + }); + + it('carries the structured half through — the remedy stays reachable', async () => { + // #7307's reasoning at the REST mapping, applied to this transport: + // dropping `developerMessage` here would move the defect rather than fix + // it, and it discloses nothing `dependentObject` does not already. + const adapter = withValidationErrorMapping({ + delete: async () => { + throw restricted(); + }, + }); + + const caught: any = await adapter.delete().catch((e: unknown) => e); + expect(caught.body.dependentObject).toBe('sys_member'); + expect(caught.body.dependentCount).toBe(3); + expect(caught.body.developerMessage).toContain("deleteBehavior:'cascade'"); + }); + + it('omits the structured keys when the engine did not supply them', async () => { + // A bare `DELETE_RESTRICTED` must still map — the arm keys off `code`, + // not off the optional detail — and must not invent `dependentCount: 0`, + // which would read as "no dependents" on the error that exists to say + // there are some. + const bare: any = new Error('Cannot delete: dependent records exist'); + bare.code = 'DELETE_RESTRICTED'; + const adapter = withValidationErrorMapping({ + delete: async () => { + throw bare; + }, + }); + + const caught: any = await adapter.delete().catch((e: unknown) => e); + expect(caught.statusCode).toBe(409); + expect(caught.body.code).toBe('DELETE_RESTRICTED'); + expect(caught.body).not.toHaveProperty('dependentObject'); + expect(caught.body).not.toHaveProperty('dependentCount'); + expect(caught.body).not.toHaveProperty('developerMessage'); + }); + }); + it('passes successful results through untouched and leaves non-function props alone', async () => { const adapter = withValidationErrorMapping({ create: async (x: number) => x + 1, diff --git a/packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts b/packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts new file mode 100644 index 0000000000..60a326348d --- /dev/null +++ b/packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts @@ -0,0 +1,465 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7724] Regression suite for "`remove-user` is non-atomic and can never +// succeed — 409 leaks as a bodyless HTTP 500, credential rows deleted without +// rollback". +// +// Three compounding problems, three groups below, one end-to-end pipeline for +// all of them: the REAL better-auth admin plugin driven through a REAL +// `AuthManager`, the same shape as `accept-invitation-adopt-membership.test.ts` +// (#7725/#7796). Nothing on the removal path is stubbed. +// +// ## The two things the fake engine MUST do, or this whole file is theatre +// +// 1. **The referential rule has to come from the real declaration.** The defect +// is a `deleteBehavior` that `sys-member.object.ts` did not declare, so a +// fake that hard-codes "cascading works" would stay green with the fix +// reverted — it would be asserting its own constant. {@link referentialRule} +// therefore READS `SysMember.fields.user_id` and reproduces `ObjectQL`'s own +// `cascadeDeleteRelations` arithmetic over it, including the escalation of a +// `set_null` on a REQUIRED foreign key to `restrict` +// (`packages/objectql/src/engine.ts`). Delete the `deleteBehavior` line from +// the object and this fake starts vetoing again, exactly as the engine does. +// +// 2. **The transaction has to be able to actually roll back.** A fake whose +// `transaction()` merely calls the callback would make the atomicity group +// green without any rollback existing — the empty-reason pass. This one +// snapshots every table on entry and restores them on throw, which is the +// contract `ObjectQL.transaction` implements over a driver +// (begin/commit/rollback, ADR-0034). +// +// Both write verbs are pinned to the real engine's dispatch predicates +// (`assertEngineDeleteDispatch` / `assertEngineUpdateDispatch`, #4550/#5480) so +// the double cannot accept a call ObjectQL refuses. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { SysMember } from '@objectstack/platform-objects'; +import { AuthManager } from './auth-manager'; + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const BASE = 'http://localhost:3000'; +const DEFAULT_ORG = 'org_default'; +const PASSWORD = 'S3cure!Passw0rd-7724'; + +/** + * The engine's referential verdict for `sys_member.user_id`, derived from the + * DECLARATION rather than restated here. + * + * `ObjectQL.cascadeDeleteRelations` resolves a `lookup` to its configured + * `deleteBehavior` (default `set_null`) and then escalates a `set_null` on a + * REQUIRED foreign key to `restrict`, because a NOT NULL column cannot be + * cleared. That escalation is the whole defect: `user_id` is required, nothing + * declared a behaviour, so every `sys_user` delete was vetoed. + */ +const referentialRule = (): 'cascade' | 'restrict' | 'set_null' => { + const field = (SysMember.fields as Record).user_id; + const declared = (field?.deleteBehavior as string | undefined) ?? 'set_null'; + if (declared === 'set_null' && field?.required === true) return 'restrict'; + return declared as 'cascade' | 'restrict' | 'set_null'; +}; + +/** + * The engine's `DELETE_RESTRICTED` envelope, in the engine's own shape + * (`code` / `status` / `developerMessage` / `dependentObject` / + * `dependentCount`, ADR-0112 + #7307). plugin-auth duck-types the error by + * `code`, so a same-shape stand-in exercises the exact mapping arm. + */ +const deleteRestricted = (dependentCount: number) => { + const err: any = new Error('Cannot delete User: 1 or more Member records still reference it.'); + err.code = 'DELETE_RESTRICTED'; + err.status = 409; + err.object = 'sys_user'; + err.dependentObject = 'sys_member'; + err.dependentCount = dependentCount; + err.developerMessage = + `Cannot delete sys_user: ${dependentCount} dependent sys_member record(s) reference it via user_id ` + + `(user_id is required, so it cannot be cleared). ` + + `Delete or reassign them first, or set deleteBehavior:'cascade' on sys_member.user_id.`; + return err; +}; + +/** + * In-memory engine enforcing `sys_member`'s unique index, the declared + * referential rule, and real snapshot transactions. + * + * @param vetoUserDelete optional stand-in for ANY other refusal of the + * `sys_user` delete — a `beforeDelete` guard, a driver fault, a constraint on + * some other dependent table. The atomicity group needs one that survives the + * cascade fix, because otherwise "nothing was left behind" would be true for + * the empty reason that nothing failed. + */ +const createMemoryEngine = (vetoUserDelete?: () => Error | undefined) => { + const tables = new Map(); + const rows = (name: string) => { + if (!tables.has(name)) tables.set(name, []); + return tables.get(name)!; + }; + const eq = (a: any, b: any) => + a instanceof Date || b instanceof Date + ? new Date(a as any).getTime() === new Date(b as any).getTime() + : a === b; + const matches = (row: any, where: Record = {}) => + Object.entries(where).every(([k, v]) => { + const actual = row[k]; + if (v && typeof v === 'object' && !Array.isArray(v) && !(v instanceof Date)) { + if ('$ne' in v) return !eq(actual, v.$ne); + if ('$in' in v) return (v.$in as any[]).some((x) => eq(actual, x)); + if ('$gt' in v) return actual > v.$gt; + if ('$gte' in v) return actual >= v.$gte; + if ('$lt' in v) return actual < v.$lt; + if ('$lte' in v) return actual <= v.$lte; + if ('$regex' in v) return new RegExp(String(v.$regex)).test(String(actual ?? '')); + } + return eq(actual, v); + }); + + const assertMemberUnique = (name: string, candidate: any, ignoreId?: string) => { + if (name !== 'sys_member') return; + const clash = rows(name).some( + (r) => + r.id !== ignoreId && + eq(r.organization_id, candidate.organization_id) && + eq(r.user_id, candidate.user_id), + ); + if (clash) { + throw new Error( + 'insert into sys_member … UNIQUE constraint failed: ' + + 'sys_member.organization_id, sys_member.user_id', + ); + } + }; + + let seq = 0; + + const engine: any = { + tables, + async insert(name: string, data: any) { + const row = { id: data.id ?? `row_${++seq}`, ...data }; + assertMemberUnique(name, row); + rows(name).push(row); + return { ...row }; + }, + async findOne(name: string, q: any = {}) { + const found = rows(name).find((r) => matches(r, q.where)); + return found ? { ...found } : null; + }, + async find(name: string, q: any = {}) { + let out = rows(name).filter((r) => matches(r, q.where)); + const order = q.orderBy?.[0]; + if (order) { + out = [...out].sort( + (a, b) => (a[order.field] > b[order.field] ? 1 : -1) * (order.order === 'desc' ? -1 : 1), + ); + } + if (q.offset) out = out.slice(q.offset); + if (q.limit) out = out.slice(0, q.limit); + return out.map((r) => ({ ...r })); + }, + async count(name: string, q: any = {}) { + return rows(name).filter((r) => matches(r, q.where)).length; + }, + async update(name: string, patch: any, options?: any) { + assertEngineUpdateDispatch(patch, options); + const row = rows(name).find((r) => r.id === patch.id); + if (!row) return null; + assertMemberUnique(name, { ...row, ...patch }, row.id); + Object.assign(row, patch); + return { ...row }; + }, + async delete(name: string, q: any = {}) { + assertEngineDeleteDispatch(q); + + if (name === 'sys_user') { + const targets = rows(name).filter((r) => matches(r, q.where)); + for (const target of targets) { + // The engine resolves dependents BEFORE removing the parent row. + const dependents = rows('sys_member').filter((m) => eq(m.user_id, target.id)); + if (dependents.length > 0) { + const rule = referentialRule(); + if (rule === 'restrict') throw deleteRestricted(dependents.length); + for (const dep of dependents) { + // "Recurse via the public delete so the child's own cascade, + // hooks and events fire" — engine.ts. Re-entering keeps the + // dispatch contract honoured on the child too. + if (rule === 'cascade') await engine.delete('sys_member', { where: { id: dep.id } }); + else await engine.update('sys_member', { id: dep.id, user_id: null }); + } + } + const veto = vetoUserDelete?.(); + if (veto) throw veto; + } + } + + const table = rows(name); + const keep = table.filter((r) => !matches(r, q.where)); + tables.set(name, keep); + return table.length - keep.length; + }, + + /** + * `ObjectQL.transaction`'s contract over a driver that supports one + * (ADR-0034): run the callback, commit on return, roll everything back on + * throw. Snapshot/restore is how `driver-memory` implements it too. + */ + async transaction(callback: (trxCtx: any, info: any) => Promise): Promise { + const snapshot = new Map(); + for (const [name, table] of tables) snapshot.set(name, table.map((r) => ({ ...r }))); + try { + return await callback({ transaction: { id: 'tx_test' } }, { owned: true }); + } catch (err) { + tables.clear(); + for (const [name, table] of snapshot) tables.set(name, table); + throw err; + } + }, + }; + + return engine; +}; + +type MemoryEngine = ReturnType; + +const singleOrgTenancy = () => + ({ + posture: 'single', + requestedPosture: 'single', + isolationActive: false, + requested: false, + degraded: false, + defaultOrgId: async () => DEFAULT_ORG, + }) as any; + +const makeManager = (engine: MemoryEngine) => + new AuthManager({ + secret: SECRET, + baseUrl: BASE, + dataEngine: engine as any, + membershipPolicy: 'auto', + getTenancy: () => singleOrgTenancy(), + }); + +const cookieFrom = (response: Response): string => + (response.headers.getSetCookie?.() ?? [response.headers.get('set-cookie') ?? '']) + .map((c) => c.split(';')[0]) + .filter(Boolean) + .join('; '); + +const post = (manager: AuthManager, path: string, body: unknown, cookie?: string) => + manager.handleRequest( + new Request(`${BASE}/api/v1/auth${path}`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + ...(cookie ? { cookie } : {}), + }, + body: JSON.stringify(body), + }), + ); + +const signUp = async (manager: AuthManager, engine: MemoryEngine, email: string) => { + const res = await post(manager, '/sign-up/email', { email, password: PASSWORD, name: email }); + expect(res.status, await res.clone().text()).toBe(200); + const user = (engine.tables.get('sys_user') ?? []).find((u: any) => u.email === email); + expect(user, `sign-up did not create ${email}`).toBeDefined(); + return { cookie: cookieFrom(res), userId: String(user!.id) }; +}; + +const rowsOf = (engine: MemoryEngine, table: string) => engine.tables.get(table) ?? []; +const membersOf = (engine: MemoryEngine, userId: string) => + rowsOf(engine, 'sys_member').filter((m: any) => m.user_id === userId); +const accountsOf = (engine: MemoryEngine, userId: string) => + rowsOf(engine, 'sys_account').filter((a: any) => a.user_id === userId); +const userRow = (engine: MemoryEngine, userId: string) => + rowsOf(engine, 'sys_user').find((u: any) => String(u.id) === userId); + +const seedOrganizations = (engine: MemoryEngine) => { + engine.tables.set('sys_organization', [{ id: DEFAULT_ORG, name: 'Default', slug: 'default' }]); +}; + +/** + * A deployment with a platform admin who can drive `/admin/remove-user`. + * + * The admin keeps their own local credential, which also keeps the break-glass + * guard out of the way: it refuses only when the TARGET holds the last one + * (`auth-manager.ts`), and here two users hold credentials throughout. + */ +const bootWithAdmin = async (engine: MemoryEngine) => { + seedOrganizations(engine); + const manager = makeManager(engine); + const admin = await signUp(manager, engine, 'admin@example.com'); + // better-auth's admin plugin authorizes by `user.role` (default + // `adminRoles: ['admin']`). + userRow(engine, admin.userId)!.role = 'admin'; + return { manager, admin }; +}; + +describe('#7724 — removing a user is one unit of work that can actually complete', () => { + beforeEach(() => { + vi.spyOn(console, 'info').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + // ------------------------------------------------------------------------- + // 1. The reported bug — the auto-bound membership vetoed every removal + // ------------------------------------------------------------------------- + + it('the reported bug: an auto-bound user can be removed, and nothing is left behind', async () => { + const engine = createMemoryEngine(); + const { manager, admin } = await bootWithAdmin(engine); + + // The reconciler binds every user to the default org at sign-up — the + // dependent row that used to make this operation impossible. + const victim = await signUp(manager, engine, 'victim@example.com'); + expect(membersOf(engine, victim.userId)).toHaveLength(1); + expect(accountsOf(engine, victim.userId).length).toBeGreaterThan(0); + + const res = await post(manager, '/admin/remove-user', { userId: victim.userId }, admin.cookie); + + // Before the fix: HTTP 500 with an EMPTY body. + expect(res.status, await res.clone().text()).toBe(200); + expect(userRow(engine, victim.userId)).toBeUndefined(); + expect(membersOf(engine, victim.userId)).toHaveLength(0); + expect(accountsOf(engine, victim.userId)).toHaveLength(0); + }); + + // The card's required case: the membership must be gone whether the reconciler + // merely created it or invitation ACCEPTANCE adopted it (#7796). Adoption + // rewrites the same row rather than adding one, so a cascade keyed on + // `user_id` covers both — this pins that it does, rather than assuming it. + it('a user who ACCEPTED an invitation leaves no orphan membership and no orphan credentials', async () => { + const engine = createMemoryEngine(); + const { manager, admin } = await bootWithAdmin(engine); + // The admin is the organization owner, so they may issue invitations. + const adminMember = rowsOf(engine, 'sys_member').find((m: any) => m.user_id === admin.userId); + expect(adminMember, 'the reconciler did not bind the admin').toBeDefined(); + adminMember!.role = 'owner'; + + const invite = await post( + manager, + '/organization/invite-member', + { email: 'invitee@example.com', role: 'member', organizationId: DEFAULT_ORG }, + admin.cookie, + ); + expect(invite.status, await invite.clone().text()).toBe(200); + const invitation = rowsOf(engine, 'sys_invitation').find( + (i: any) => i.email === 'invitee@example.com', + ); + expect(invitation).toBeDefined(); + + const invitee = await signUp(manager, engine, 'invitee@example.com'); + const accept = await post( + manager, + '/organization/accept-invitation', + { invitationId: String(invitation!.id) }, + invitee.cookie, + ); + expect(accept.status, await accept.clone().text()).toBe(200); + // The row acceptance ADOPTED — one membership, not two. + expect(membersOf(engine, invitee.userId)).toHaveLength(1); + + const res = await post(manager, '/admin/remove-user', { userId: invitee.userId }, admin.cookie); + expect(res.status, await res.clone().text()).toBe(200); + + expect(userRow(engine, invitee.userId)).toBeUndefined(); + expect(membersOf(engine, invitee.userId)).toHaveLength(0); + expect(accountsOf(engine, invitee.userId)).toHaveLength(0); + expect(rowsOf(engine, 'sys_session').filter((s: any) => s.user_id === invitee.userId)).toHaveLength(0); + }); + + // ------------------------------------------------------------------------- + // 2. Atomicity — the harmful half, and the half that must survive the cascade + // ------------------------------------------------------------------------- + + describe('when something still refuses the sys_user delete', () => { + // A refusal that is NOT the membership restrict, so it survives problem 1's + // fix. This is the "some other failure path" the card names: the cascade + // makes the reported case succeed, and atomicity is what keeps the NEXT + // failure from leaving an un-authenticatable identity on the roster. + const guardRefusal = () => { + const err: any = new Error('Refused by a beforeDelete guard on sys_user.'); + err.code = 'PERMISSION_DENIED'; + err.status = 403; + return err; + }; + + it('rolls the credential and session deletes back — the identity stays usable', async () => { + const engine = createMemoryEngine(guardRefusal); + const { manager, admin } = await bootWithAdmin(engine); + const victim = await signUp(manager, engine, 'victim@example.com'); + + const accountsBefore = accountsOf(engine, victim.userId).map((a: any) => a.id); + expect(accountsBefore.length).toBeGreaterThan(0); + + const res = await post(manager, '/admin/remove-user', { userId: victim.userId }, admin.cookie); + expect(res.status).toBeGreaterThanOrEqual(400); + + // The reported harm, inverted: the user row survived the refusal, so its + // credentials must have survived it too. Without the transaction these + // are gone and that email can never sign in again (401) while still + // occupying the org roster. + expect(userRow(engine, victim.userId)).toBeDefined(); + expect(accountsOf(engine, victim.userId).map((a: any) => a.id)).toEqual(accountsBefore); + // The membership the cascade had already removed is back too — the whole + // unit of work, not just the part the reporter happened to notice. + expect(membersOf(engine, victim.userId)).toHaveLength(1); + }); + + it('the refusal still reaches the client as a structured body, not an empty one', async () => { + const engine = createMemoryEngine(guardRefusal); + const { manager, admin } = await bootWithAdmin(engine); + const victim = await signUp(manager, engine, 'victim@example.com'); + + const res = await post(manager, '/admin/remove-user', { userId: victim.userId }, admin.cookie); + const text = await res.clone().text(); + expect(text.length, 'the response body is empty — the fault leaked unmapped').toBeGreaterThan(0); + expect(res.status).toBe(403); + expect((await res.json()).code).toBe('PERMISSION_DENIED'); + }); + }); + + // ------------------------------------------------------------------------- + // 3. The status-code leak — a referential veto is a 409, never a bodyless 500 + // ------------------------------------------------------------------------- + + it('a DELETE_RESTRICTED veto surfaces as a 409 carrying the engine’s explanation', async () => { + // Reached through the same seam the membership restrict used to reach: any + // dependent table the engine may neither cascade nor null. Pinned here + // end-to-end because the unit-level arm cannot show that better-auth's + // router renders it instead of swallowing it. + const engine = createMemoryEngine(() => deleteRestricted(1)); + const { manager, admin } = await bootWithAdmin(engine); + const victim = await signUp(manager, engine, 'victim@example.com'); + + const res = await post(manager, '/admin/remove-user', { userId: victim.userId }, admin.cookie); + + expect(res.status).toBe(409); + const body: any = await res.json(); + expect(body.code).toBe('DELETE_RESTRICTED'); + expect(body.dependentObject).toBe('sys_member'); + expect(body.dependentCount).toBe(1); + expect(body.developerMessage).toContain("deleteBehavior:'cascade'"); + + // Same request, the other half of the card: refused AND clean. + expect(userRow(engine, victim.userId)).toBeDefined(); + expect(accountsOf(engine, victim.userId).length).toBeGreaterThan(0); + }); + + // ------------------------------------------------------------------------- + // 4. The declaration itself + // ------------------------------------------------------------------------- + + it('sys_member.user_id declares the cascade rather than inheriting the veto', () => { + // The rule the fake reads, asserted directly so a failure in the groups + // above can be told apart from the declaration simply being gone. + const field = (SysMember.fields as Record).user_id; + expect(field.required).toBe(true); + expect(field.deleteBehavior).toBe('cascade'); + expect(referentialRule()).toBe('cascade'); + }); +}); From 7e2beb4ce3df993ca82d34f734147821f7470ed2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 01:03:52 +0000 Subject: [PATCH 3/4] test(plugin-auth): enable the admin plugin in the #7724 harness Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D6Qi9sYxhaRwj7TYiD5MWg --- .../plugins/plugin-auth/src/remove-user-atomicity.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts b/packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts index 60a326348d..cfddec746f 100644 --- a/packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts +++ b/packages/plugins/plugin-auth/src/remove-user-atomicity.test.ts @@ -237,6 +237,11 @@ const makeManager = (engine: MemoryEngine) => dataEngine: engine as any, membershipPolicy: 'auto', getTenancy: () => singleOrgTenancy(), + // `/admin/remove-user` is the better-auth admin plugin's route, and the + // plugin is opt-in (`admin: pluginConfig.admin ?? scimEffective`). Without + // this the route 404s and every assertion below would be measuring the + // absence of an endpoint rather than the behaviour of one. + plugins: { admin: true }, }); const cookieFrom = (response: Response): string => From 125dbe5685d1eab91fa63a7ad675ccbcd0dc2830 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 01:48:34 +0000 Subject: [PATCH 4/4] fix(identity): changeset + type the composed handler runner Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D6Qi9sYxhaRwj7TYiD5MWg --- .changeset/olive-moons-shave.md | 12 ++++++++++++ packages/plugins/plugin-auth/src/auth-manager.ts | 7 +++++-- 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 .changeset/olive-moons-shave.md diff --git a/.changeset/olive-moons-shave.md b/.changeset/olive-moons-shave.md new file mode 100644 index 0000000000..75fd6084c1 --- /dev/null +++ b/.changeset/olive-moons-shave.md @@ -0,0 +1,12 @@ +--- +'@objectstack/platform-objects': patch +'@objectstack/plugin-auth': patch +--- + +Fix `POST /api/v1/auth/admin/remove-user`, which could never succeed and left the identity un-authenticatable when it failed. + +Three compounding problems on the better-auth admin removal path: + +- **`sys_member.user_id` declared no `deleteBehavior`.** A `lookup` defaults to `set_null`, and the engine escalates a defaulted `set_null` on a REQUIRED foreign key to `restrict` — so the membership every user gets at sign-up (and, since the invitation-adoption change, keeps after accepting an invitation) vetoed every `sys_user` delete. The field now declares `deleteBehavior: 'cascade'`. The last-administrator invariant is unaffected: it is enforced by a `beforeDelete` hook on `sys_member`, and the engine's cascade recurses through the public `delete()`, so that hook still runs. +- **The removal was not atomic.** better-auth deletes the sessions, then the accounts, then the user, in three calls with no transaction, so anything refusing the last one left the credential rows deleted and the user row behind — an identity still on the org roster that can no longer sign in. Subject-erasure requests now run inside one engine transaction and roll back as a unit. Datasources whose driver has no transaction support keep the previous behaviour and log the engine's existing warning. +- **A referential refusal reached the client as an HTTP 500 with an empty body.** The auth adapter mapped engine validation errors and policy refusals to better-auth `APIError`s but not referential ones, so a `DELETE_RESTRICTED` escaped unmapped. It now surfaces as a structured 409 carrying the dependent object, the dependent count and the remedy. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 989a4531b3..c04b6a6219 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -3090,8 +3090,11 @@ export class AuthManager { // costs nothing: the scope starts empty, the before-hook drops a resolver // in, and the session is looked up only if some write asks. Attribution // only — the authorization subject of those writes is unchanged (system). - const runHandler = (): Promise => - runWithAuthActorScope(() => + // `await`, not a bare return: both scope helpers are generic over their + // callback, so the composed call is typed `Promise< Promise< Response > >`. + // The previous single call site flattened it with the `await` below. + const runHandler = async (): Promise => + await runWithAuthActorScope(() => runWithRequestState(new WeakMap(), () => auth.handler(request)), );