From a939f1bc134bbe343005941c6fb4be9c450111ab Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 09:46:09 +0000 Subject: [PATCH 1/3] fix(plugin-auth): route @better-auth/scim's identity.reconcileUser to the platform ban write SCIM active:false revoked sessions and wrote nothing on stable @better-auth/scim (the vendor's ban write left the package in 1.7.0); sys_user.banned was never set and a local-password user signed straight back in. Wire identity.reconcileUser into the scim() options and route it to the shared ban/unban write in admin-ban-endpoints.ts, inside the SCIM transaction; the engine-level last-administrator guard judges it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../scim-active-false-reconcile-user.md | 7 + .../plugin-auth/src/admin-ban-endpoints.ts | 80 ++- .../plugins/plugin-auth/src/auth-manager.ts | 118 ++++ .../scim-deactivation-reconcile-user.test.ts | 595 ++++++++++++++++++ 4 files changed, 791 insertions(+), 9 deletions(-) create mode 100644 .changeset/scim-active-false-reconcile-user.md create mode 100644 packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts diff --git a/.changeset/scim-active-false-reconcile-user.md b/.changeset/scim-active-false-reconcile-user.md new file mode 100644 index 0000000000..130319895f --- /dev/null +++ b/.changeset/scim-active-false-reconcile-user.md @@ -0,0 +1,7 @@ +--- +"@objectstack/plugin-auth": minor +--- + +SCIM `active: false` disables the account again. Stable `@better-auth/scim` (1.7.0+) no longer writes the admin plugin's `banned` column itself — it hands the aggregate lifecycle state to an optional host callback, `identity.reconcileUser`, and only revokes sessions. `plugin-auth` passed no `identity` member, so an identity provider deactivating a user revoked sessions and wrote nothing: `sys_user.banned` stayed false and a user holding a local password signed straight back in. `AuthManager` now implements the callback and routes it to the platform's own ban write: `active: false` bans the user (reason `Deactivated via SCIM`, no expiry) and the vendor's `BANNED_USER` sign-in refusal applies; `active: true` lifts a ban that carries that reason — an administrator's ban (any other reason) is not the identity provider's to lift, so an attribute sync never re-admits a user banned for cause. The break-glass last-administrator guard (ADR-0024 D5.2) judges the write at the engine, so deactivating the last administrator through SCIM is refused with a 403 SCIM error and the account stays active. A SCIM `DELETE /Users/{id}` — which on 1.7.2 tombstones the source rather than deleting the user — now leaves that account disabled too. + +New exports from `@objectstack/plugin-auth` (the shared write the SCIM hook and the `/admin/ban-user` mount both call): `applyUserBan`, `applyUserUnban`, `SCIM_DEACTIVATION_BAN_REASON`, and the `UserBanWriter` / `UserBanFields` types. diff --git a/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts index 2c992572ce..86dceaeebf 100644 --- a/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts +++ b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts @@ -90,6 +90,75 @@ export interface AdminBanEndpointDeps { getAuthContext(): Promise; } +/** + * [#14360] The `sys_user` ban write, as ONE callable both ban paths share. + * + * Two callers land a disable on this platform: the `/admin/ban-user` mount + * below, and — since `@better-auth/scim` 1.7.0 stopped writing `banned` + * itself and handed the host an `identity.reconcileUser` callback instead — + * the SCIM `active: false` deprovisioning hook in `auth-manager.ts` + * (`reconcileScimUserLifecycle`). Field for field this is the write + * better-auth's own `banUser` handler makes (`banned` / `banReason` / + * `banExpires` / `updatedAt`), so the vendor's `session.create` hook + * (`BANNED_USER`) enforces both halves identically. The break-glass + * last-administrator guard (ADR-0024 D5.2, `last-admin-guard.ts`) judges the + * write at the ENGINE, so it holds on both callers by construction — neither + * can reach the column without passing it. + * + * `UserBanWriter` is the narrowest surface the write needs, and both callers + * already hold one: better-auth's `internalAdapter` satisfies it directly; the + * SCIM hook adapts the vendor's transaction-bound `DBTransactionAdapter`, so + * its write commits — or rolls back — with the SCIM mutation it belongs to. + * + * Session revocation is deliberately NOT part of the write: the admin mount + * revokes explicitly (below), and the SCIM vendor revokes after its callback + * returns (`deleteUserSessions` on `active: false`, measured on 1.7.2). + */ +export interface UserBanWriter { + updateUser(id: string, data: Record): Promise; +} + +/** + * The reason the SCIM deactivation path stamps on the row — the exact string + * `@better-auth/scim` wrote itself through 1.6.x + * (`resolveSCIMActiveDeactivation`, `dist/index.mjs` at 1.6.30), kept verbatim + * so a row the vendor banned before the 1.7.0 decoupling and a row the host + * hook bans are the same fact. It is also how the reactivation half + * recognises its OWN ban: an `active: true` from the IdP lifts a ban carrying + * this reason and leaves an administrator's ban (any other reason) in place. + */ +export const SCIM_DEACTIVATION_BAN_REASON = 'Deactivated via SCIM'; + +export interface UserBanFields { + banReason: string; + /** `undefined` leaves the column untouched; `null` clears a prior expiry. */ + banExpires?: Date | null; +} + +/** Disable `userId` — the vendor-shaped ban write, on whichever writer the caller is inside. */ +export async function applyUserBan( + writer: UserBanWriter, + userId: string, + ban: UserBanFields, +): Promise { + await writer.updateUser(userId, { + banned: true, + banReason: ban.banReason, + ...(ban.banExpires !== undefined ? { banExpires: ban.banExpires } : {}), + updatedAt: new Date(), + }); +} + +/** Re-enable `userId` — clears the three ban columns, exactly as the vendor's `unbanUser` does. */ +export async function applyUserUnban(writer: UserBanWriter, userId: string): Promise { + await writer.updateUser(userId, { + banned: false, + banReason: null, + banExpires: null, + updatedAt: new Date(), + }); +} + const invalid = (message: string): EndpointResult => ({ status: 400, body: { success: false, error: { code: 'INVALID_REQUEST', message } }, @@ -161,11 +230,9 @@ export async function runAdminBanUser( }; } - await ctx.internalAdapter.updateUser(userId, { - banned: true, + await applyUserBan(ctx.internalAdapter, userId, { banReason, ...(banExpires ? { banExpires } : {}), - updatedAt: new Date(), }); // Sign the banned user out everywhere, exactly as the vendor handler does. await ctx.internalAdapter.deleteUserSessions(userId); @@ -197,12 +264,7 @@ export async function runAdminUnbanUser( const ctx = await deps.getAuthContext(); if (!(await ctx.internalAdapter.findUserById(userId))) return notFound(); - await ctx.internalAdapter.updateUser(userId, { - banned: false, - banReason: null, - banExpires: null, - updatedAt: new Date(), - }); + await applyUserUnban(ctx.internalAdapter, userId); return { status: 200, body: { success: true, data: { userId, banned: false } } }; } diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 7f729659d8..28d0aa19c1 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { Auth, BetterAuthOptions } from 'better-auth'; +import type { SCIMIdentityState, SCIMTransactionContext } from '@better-auth/scim'; // better-auth value imports (betterAuth + plugins) are deferred via dynamic // import() in getOrCreateAuth() / buildPluginList() so that disabled plugins // never get loaded into the process. See Stage 2F (RSS investigation). @@ -101,6 +102,11 @@ import { LAST_LOCAL_CREDENTIAL_CODE, LAST_LOCAL_CREDENTIAL_MESSAGE, } from './last-local-credential.js'; +import { + applyUserBan, + applyUserUnban, + SCIM_DEACTIVATION_BAN_REASON, +} from './admin-ban-endpoints.js'; import { PHONE_SMS_TOPICS, builtinPhoneSmsBody, @@ -3276,6 +3282,16 @@ export class AuthManager { return verifyScimBearerToken(engine as never, secret, input.token); }, }, + // [#14360] The host half of `active`: stable @better-auth/scim + // writes no `banned` itself any more (the 1.6.x coupling left the + // package in 1.7.0) — it hands the aggregate lifecycle state to + // this callback inside the SCIM transaction and only revokes + // sessions. Routed to the platform's own ban write; the break-glass + // last-administrator guard judges it at the engine. See + // `reconcileScimUserLifecycle` for the contract and the measurement. + identity: { + reconcileUser: (state, context) => this.reconcileScimUserLifecycle(state, context), + }, }); }); } @@ -4840,6 +4856,108 @@ export class AuthManager { return auth.api; } + /** + * [#14360] `identity.reconcileUser` — the host half of SCIM `active`. + * + * `@better-auth/scim` 1.7.0 removed its own `banned` write (1.6.30 mapped + * `active` onto the admin plugin's ban and refused a deactivation without + * that plugin; on the installed 1.7.2 the substring `ban` occurs zero times + * in the package) and replaced it with this optional callback: the vendor + * computes the user's AGGREGATE lifecycle state — `active` is true while + * any participating SCIM source says so — inside the request's + * transaction, calls the host, and then revokes the user's sessions when + * the state is inactive (`dist/index.mjs`, the identity facade's + * `reconcileUser`). Without a host implementation an IdP's `active: false` + * revoked sessions and wrote nothing: `sys_user.banned` stayed false and a + * local-password user signed straight back in, while ADR-0071, the + * generated docs and the #13816 refusal all asserted the ban. + * + * This method restores declared = enforced by routing the state to the + * platform's OWN ban write (`admin-ban-endpoints.ts`): + * + * - `active: false` on a row that is not banned ⇒ `applyUserBan` with + * `SCIM_DEACTIVATION_BAN_REASON` and no expiry. The vendor's + * `session.create` hook (`BANNED_USER`) then refuses sign-in — the same + * enforcement the admin ban has, because it is the same write. + * - `active: true` on a row banned WITH that reason ⇒ `applyUserUnban`. + * A ban carrying any other reason was placed by an administrator and is + * not the IdP's to lift: an attribute sync (every SCIM PUT carries + * `active: true`) must not silently re-admit a user banned for cause. + * - Anything else is a no-op. The callback is contractually idempotent + * ("Implementations must be idempotent") and the vendor invokes it on + * EVERY user mutation, so a PATCH that changes only `displayName` + * touches no ban column. + * + * A consequence worth stating: on 1.7.2 a SCIM `DELETE /Users/{id}` no + * longer deletes the better-auth user (the vendor tombstones the source); + * it leaves the user with no active source, so this callback disables the + * account. Re-provisioning through the tombstone re-links the same user, + * the state turns active, and the SCIM ban is lifted by the second bullet. + * + * The break-glass last-administrator guard (ADR-0024 D5.2, #5892) is an + * ENGINE `beforeUpdate` hook on `sys_user`, so it judges this write exactly + * as it judges the admin mount's: deactivating the last administrator + * throws its 403 `PERMISSION_DENIED`, the adapter rethrows it as an + * `APIError`, the vendor re-throws `APIError`s unchanged out of this + * callback (`runSCIMApplicationCallback`, measured on 1.7.2 — any other + * throw becomes a SCIM 500 "SCIM identity reconciliation failed" carrying + * the original as `cause`), the transaction rolls back, and the IdP + * receives a SCIM error with `status: "403"` and the guard's own + * explanation. The account stays active; nothing is skipped silently. + * + * Deliberately NOT applied here: the last-LOCAL-credential guard the admin + * mount re-runs (`isLastLocalCredentialHolder`). That guard protects the + * password escape hatch from an administrator's click; on this path the + * identity provider is the authority for the user it deprovisions, and + * keeping a departed user's password alive because it happened to be the + * last one is the wrong direction for a deprovisioning contract. 1.6.x + * never applied it on the SCIM path either — the vendor wrote the column + * straight through the adapter. + * + * Every read and write goes through `context.database` — the vendor's + * transaction-bound adapter — never through an `internalAdapter` resolved + * outside it, so the ban commits or rolls back with the SCIM mutation it + * belongs to. + */ + private async reconcileScimUserLifecycle( + state: SCIMIdentityState, + context: SCIMTransactionContext, + ): Promise { + const db = context.database; + const user = await db.findOne<{ banned?: unknown; banReason?: unknown }>({ + model: 'user', + where: [{ field: 'id', value: state.userId }], + }); + if (!user) { + // The vendor holds a `scimSubject` for this user inside the same + // transaction, so a missing row is an invariant break, not a state to + // reconcile. Thrown, not logged: the vendor turns it into a SCIM 500 + // and rolls the mutation back — a deactivation that cannot find its + // account must not report success. + throw new Error( + `[auth] SCIM identity reconciliation: better-auth user '${state.userId}' has no sys_user row`, + ); + } + const writer = { + updateUser: (id: string, data: Record) => + db.update({ model: 'user', where: [{ field: 'id', value: id }], update: data }), + }; + const banned = user.banned === true; + if (!state.active) { + // Already disabled — by an earlier SCIM pass or by an administrator. + if (banned) return; + await applyUserBan(writer, state.userId, { + banReason: SCIM_DEACTIVATION_BAN_REASON, + banExpires: null, + }); + return; + } + if (!banned) return; + // An administrator's ban is not the IdP's to lift. + if (user.banReason !== SCIM_DEACTIVATION_BAN_REASON) return; + await applyUserUnban(writer, state.userId); + } + /** * Get the underlying better-auth context for low-level operations such as * `internalAdapter.createAccount` / `password.hash`. diff --git a/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts b/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts new file mode 100644 index 0000000000..0c8a0e94ed --- /dev/null +++ b/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts @@ -0,0 +1,595 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14360] SCIM `active: false` disables the account again — end to end, + * through `@better-auth/scim` itself. + * + * ## The defect + * + * `@better-auth/scim` 1.6.x mapped a SCIM `active: false` onto the admin + * plugin's `banned` column. 1.7.0 removed that write (the substring `ban` + * occurs zero times in the installed 1.7.2 package) and replaced it with an + * OPTIONAL host callback, `identity.reconcileUser`, followed by the vendor's + * own `deleteUserSessions`. `plugin-auth` passed no `identity` member, so an + * identity provider deactivating a user revoked sessions and wrote nothing: + * `sys_user.banned` stayed false, and a user holding a local password signed + * straight back in. `auth-manager.ts` now passes the callback and routes it + * to the platform's own ban write (`admin-ban-endpoints.ts`). + * + * ## Why every case drives the vendor and none simulates the write + * + * `last-admin-guard.test.ts`'s `deprovision()` helper writes `banned: true` + * through the adapter in the vendor's NAME — and it stayed green across the + * very release that stopped the vendor writing it (triage on #14360: ⛔ do not + * extend it). So every case below goes through `AuthManager.handleRequest()` + * with a real SCIM bearer: the `scim({ identity: { reconcileUser } })` wiring + * in `auth-manager.ts` is inside the system under test, and so are the + * vendor's transaction, its session revocation and its error mapping. + * + * ## Backend + * + * A real `ObjectQL` over `@objectstack/driver-sql` + better-sqlite3 + * `:memory:` — the backend `credential-at-rest-posture.test.ts` already boots + * SCIM on. The break-glass last-administrator guard (ADR-0024 D5.2) and the + * ADR-0092 identity write guard are registered on the engine the way + * `auth-plugin.ts` registers them at `kernel:ready`, so the refusal in (c) is + * the production hook, not a stand-in. + * + * ## Faces + * + * (a) `active: false` ⇒ `sys_user.banned`, sessions revoked, sign-in refused + * with the vendor's `BANNED_USER` (status AND code, never a bare throw). + * (b) `active: true` ⇒ unbanned, sign-in accepted again. + * (c) the last administrator: refused THROUGH SCIM as a 403 SCIM error, the + * account stays active — plus the positive control (a second + * administrator makes the same request succeed) that proves the guard + * was the thing refusing. + * (d) negative controls: a PATCH that does not change `active` touches no + * ban column and revokes nothing; an administrator's ban survives an + * IdP attribute sync and an explicit `active: true`. + * (e) a host that declines the admin plugin beside SCIM is still refused at + * construction (#13816 — unchanged by this card). + * (f) `DELETE /Users/{id}` leaves the tombstoned account disabled (the + * vendor no longer deletes the better-auth user on 1.7.2). + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import { + SysUser, + SysSession, + SysAccount, + SysVerification, + SysOrganization, + SysMember, + SysInvitation, + SysTeam, + SysTeamMember, + SysScimConnectionBinding, + SysScimConnectionCredential, + SysScimGroup, + SysScimGroupMember, + SysScimIdentityTombstone, + SysScimProjectionGrant, + SysScimSubject, + SysScimUser, + SysJwks, +} from '@objectstack/platform-objects'; +import { AuthManager } from './auth-manager.js'; +import { createTenancyService } from './tenancy-service.js'; +import { mintScimConnectionCredential } from './scim-connection-service.js'; +import { registerLastAdminGuard, type LastAdminGuardEngine } from './last-admin-guard.js'; +import { registerIdentityWriteGuard, registerManagedUpdateWhitelist } from './identity-write-guard.js'; +import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js'; +import { SCIM_DEACTIVATION_BAN_REASON } from './admin-ban-endpoints.js'; + +const BASE = 'http://localhost:3000'; +const AUTH = `${BASE}/api/v1/auth`; +const SECRET = 'test-secret-at-least-32-chars-long-14360'; +const PASSWORD = 'correct-horse-battery-staple-14360'; + +const USER_SCHEMA = 'urn:ietf:params:scim:schemas:core:2.0:User'; +const PATCH_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:PatchOp'; +const SCIM_ERROR_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:Error'; + +/** Every read below is a safety-proof read, never RLS-scoped to a caller. */ +const SYSTEM = { context: { isSystem: true } } as const; + +/** The identity surface the org + admin (forced by SCIM) + scim plugins touch. */ +const AUTH_OBJECTS = [ + SysUser, + SysSession, + SysAccount, + SysVerification, + SysOrganization, + SysMember, + SysInvitation, + SysTeam, + SysTeamMember, + SysScimConnectionBinding, + SysScimConnectionCredential, + SysScimGroup, + SysScimGroupMember, + SysScimIdentityTombstone, + SysScimProjectionGrant, + SysScimSubject, + SysScimUser, + SysJwks, +]; + +/** + * The two tables the break-glass guard enumerates platform administrators + * from, declared down to the columns it reads — the same minimal fixtures + * `last-admin-guard.test.ts` uses, so the guard's enumeration runs against + * real tables rather than a hand-written `where` matcher. + */ +const sysPermissionSet = { + name: 'sys_permission_set', + label: 'Permission Set', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + label: { name: 'label', type: 'text' as const }, + active: { name: 'active', type: 'boolean' as const }, + }, +}; + +const sysUserPermissionSet = { + name: 'sys_user_permission_set', + label: 'User Permission Set', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + user_id: { name: 'user_id', type: 'text' as const }, + permission_set_id: { name: 'permission_set_id', type: 'text' as const }, + organization_id: { name: 'organization_id', type: 'text' as const }, + valid_from: { name: 'valid_from', type: 'datetime' as const }, + valid_until: { name: 'valid_until', type: 'datetime' as const }, + }, +}; + +const PS_ADMIN = 'ps_admin_full_access'; + +const engines: ObjectQL[] = []; +afterEach(async () => { + while (engines.length) { + const e = engines.pop(); + try { + await (e as unknown as { destroy?(): Promise })?.destroy?.(); + } catch { + /* noop */ + } + } +}); + +interface Harness { + engine: ObjectQL; + manager: AuthManager; + token: string; + send: (request: Request) => Promise; +} + +/** + * The manager under test — built the way a deployment with SCIM turned on + * builds it (`plugins.scim: true` forces the admin plugin on, which is what + * supplies the `banned` column and the `BANNED_USER` sign-in refusal). The + * `identity.reconcileUser` wiring comes from `AuthManager.buildPluginList()`; + * nothing here names it. + */ +async function boot(): Promise { + const engine = new ObjectQL(); + engines.push(engine); + engine.registerDriver( + new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), + true, + ); + await engine.init(); + for (const object of AUTH_OBJECTS) { + engine.registry.registerObject(object as never, '@objectstack/plugin-auth'); + } + for (const object of [sysPermissionSet, sysUserPermissionSet]) { + engine.registry.registerObject(object as never, '@objectstack/plugin-auth'); + } + await engine.syncSchemas(); + + // The two engine guards `auth-plugin.ts` registers at `kernel:ready`, in + // the same order (ADR-0092 at priority 10, break-glass at 20). + registerManagedUpdateWhitelist('sys_user', SYS_USER_PROFILE_EDIT_FIELDS); + registerIdentityWriteGuard(engine, { packageId: 'test.identity-write-guard' }); + registerLastAdminGuard(engine as unknown as LastAdminGuardEngine, { + packageId: 'test.last-admin-guard', + }); + + const manager = new AuthManager({ + secret: SECRET, + baseUrl: BASE, + dataEngine: engine as never, + getTenancy: () => createTenancyService({ requested: 'isolated', probeIsolation: () => true }), + plugins: { scim: true, organization: true }, + } as never); + + const { token } = await mintScimConnectionCredential(engine as never, SECRET, { + connectionId: 'okta-14360', + }); + + return { engine, manager, token, send: (request) => manager.handleRequest(request) }; +} + +// --------------------------------------------------------------------------- +// SCIM 2.0 requests — the shapes an identity provider actually sends +// --------------------------------------------------------------------------- + +function scimRequest(h: Harness, method: string, path: string, body?: unknown): Request { + return new Request(`${AUTH}/scim/v2${path}`, { + method, + headers: { + origin: BASE, + authorization: `Bearer ${h.token}`, + ...(body !== undefined ? { 'content-type': 'application/scim+json' } : {}), + }, + ...(body !== undefined ? { body: JSON.stringify(body) } : {}), + }); +} + +interface Provisioned { + /** The SCIM resource id (what the IdP addresses). */ + scimId: string; + /** The better-auth / `sys_user` id. */ + userId: string; + email: string; +} + +/** `POST /Users` — an IdP provisioning an active user. */ +async function provision(h: Harness, localPart: string): Promise { + const email = `${localPart}@example.com`; + const res = await h.send( + scimRequest(h, 'POST', '/Users', { + schemas: [USER_SCHEMA], + userName: email, + name: { givenName: localPart, familyName: 'Example' }, + displayName: `${localPart} Example`, + emails: [{ value: email, primary: true, type: 'work' }], + active: true, + }), + ); + expect(res.status, `SCIM POST /Users failed: ${await res.clone().text()}`).toBe(201); + const body = (await res.json()) as { id: string; active?: boolean }; + expect(body.active).toBe(true); + const row = await userRow(h, email); + expect(row, 'the SCIM create must have landed a sys_user row').toBeTruthy(); + return { scimId: body.id, userId: String(row!.id), email }; +} + +async function patchUser( + h: Harness, + scimId: string, + operations: Array<{ op: string; path?: string; value?: unknown }>, +): Promise { + return h.send( + scimRequest(h, 'PATCH', `/Users/${scimId}`, { schemas: [PATCH_SCHEMA], Operations: operations }), + ); +} + +const setActive = (h: Harness, scimId: string, active: boolean) => + patchUser(h, scimId, [{ op: 'replace', path: 'active', value: active }]); + +const setDisplayName = (h: Harness, scimId: string, displayName: string) => + patchUser(h, scimId, [{ op: 'replace', path: 'displayName', value: displayName }]); + +async function scimActive(h: Harness, scimId: string): Promise { + const res = await h.send(scimRequest(h, 'GET', `/Users/${scimId}`)); + expect(res.status, `SCIM GET /Users/{id} failed: ${await res.clone().text()}`).toBe(200); + return ((await res.json()) as { active?: boolean }).active; +} + +// --------------------------------------------------------------------------- +// The platform side — rows read as system, the vendor's sign-in driven for real +// --------------------------------------------------------------------------- + +async function userRow(h: Harness, email: string): Promise | null> { + return h.engine.findOne( + 'sys_user', + { where: { email }, fields: ['id', 'email', 'banned', 'ban_reason', 'ban_expires'] }, + SYSTEM, + ) as Promise | null>; +} + +/** sqlite hands the boolean back as 0/1; anything else is NOT a ban. */ +const isBanned = (row: Record | null): boolean => + row?.banned === true || row?.banned === 1; + +async function sessionCount(h: Harness, userId: string): Promise { + const rows = await h.engine.find('sys_session', { where: { user_id: userId } }, SYSTEM); + return Array.isArray(rows) ? rows.length : 0; +} + +/** + * The "local-password user" the card describes: an IdP-provisioned account an + * administrator later gave a password (the `set-user-password` shape). Written + * through better-auth's own context, the same seam `set-initial-password` + * uses; the address is marked verified because the IdP asserted it. + */ +async function attachPassword(h: Harness, user: Provisioned): Promise { + const ctx = await h.manager.getAuthContext(); + const hash = await ctx.password.hash(PASSWORD); + await ctx.internalAdapter.linkAccount({ + userId: user.userId, + providerId: 'credential', + accountId: user.userId, + password: hash, + }); + await h.engine.update('sys_user', { id: user.userId, email_verified: true }, SYSTEM); +} + +async function signIn(h: Harness, email: string): Promise { + return h.send( + new Request(`${AUTH}/sign-in/email`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE }, + body: JSON.stringify({ email, password: PASSWORD }), + }), + ); +} + +async function expectSignInAccepted(h: Harness, email: string): Promise { + const res = await signIn(h, email); + expect(res.status, `sign-in refused: ${await res.clone().text()}`).toBeLessThan(300); +} + +/** + * The vendor's refusal, pinned as status AND code: better-auth's admin + * plugin throws `FORBIDDEN` / `BANNED_USER` from its `session.create` hook. + * A bare "not 2xx" would also be satisfied by a wrong password or a broken + * transport — neither is the fact under test. + */ +async function expectSignInBanned(h: Harness, email: string): Promise { + const res = await signIn(h, email); + const body = (await res.json()) as { code?: string; message?: string }; + expect(res.status, `expected BANNED_USER, got ${res.status} ${JSON.stringify(body)}`).toBe(403); + expect(body.code).toBe('BANNED_USER'); +} + +/** Give `userId` unscoped `admin_full_access` — a platform administrator. */ +async function makePlatformAdmin(h: Harness, userId: string): Promise { + const existing = await h.engine.findOne( + 'sys_permission_set', + { where: { id: PS_ADMIN }, fields: ['id'] }, + SYSTEM, + ); + if (!existing) { + await h.engine.insert('sys_permission_set', { id: PS_ADMIN, name: ADMIN_FULL_ACCESS }, SYSTEM); + } + await h.engine.insert( + 'sys_user_permission_set', + { id: `ups_${userId}`, user_id: userId, permission_set_id: PS_ADMIN }, + SYSTEM, + ); +} + +// --------------------------------------------------------------------------- +// (a) + (b) — the contract: deactivation disables, reactivation re-enables +// --------------------------------------------------------------------------- + +describe('[#14360] SCIM active:false disables the account through the platform ban write', () => { + it('(a) active:false ⇒ banned with the SCIM reason, sessions revoked, sign-in refused as BANNED_USER', async () => { + const h = await boot(); + const dana = await provision(h, 'dana'); + await attachPassword(h, dana); + + // Control: before the IdP deactivates, the local password works and + // leaves a session behind — the row this card says must stop working. + await expectSignInAccepted(h, dana.email); + expect(await sessionCount(h, dana.userId)).toBeGreaterThan(0); + expect(isBanned(await userRow(h, dana.email))).toBe(false); + + const res = await setActive(h, dana.scimId, false); + expect(res.status, `SCIM PATCH active:false failed: ${await res.clone().text()}`).toBe(200); + expect(((await res.json()) as { active?: boolean }).active).toBe(false); + + const row = await userRow(h, dana.email); + expect(isBanned(row)).toBe(true); + expect(row?.ban_reason).toBe(SCIM_DEACTIVATION_BAN_REASON); + expect(row?.ban_expires ?? null).toBeNull(); + // The vendor's own revocation, after the callback returned. + expect(await sessionCount(h, dana.userId)).toBe(0); + // And the enforcement: the vendor's session hook reads `banned`. + await expectSignInBanned(h, dana.email); + }, 60_000); + + it('(b) active:true ⇒ the SCIM ban is lifted and sign-in is accepted again', async () => { + const h = await boot(); + const dana = await provision(h, 'dana'); + await attachPassword(h, dana); + + expect((await setActive(h, dana.scimId, false)).status).toBe(200); + await expectSignInBanned(h, dana.email); + + const res = await setActive(h, dana.scimId, true); + expect(res.status, `SCIM PATCH active:true failed: ${await res.clone().text()}`).toBe(200); + + const row = await userRow(h, dana.email); + expect(isBanned(row)).toBe(false); + expect(row?.ban_reason ?? null).toBeNull(); + expect(row?.ban_expires ?? null).toBeNull(); + await expectSignInAccepted(h, dana.email); + }, 60_000); + + it('(a)+(b) are idempotent: repeating the same state writes nothing new and changes nothing', async () => { + const h = await boot(); + const dana = await provision(h, 'dana'); + + expect((await setActive(h, dana.scimId, false)).status).toBe(200); + // The vendor reports "no change" for an identical PATCH; the callback + // still runs on the vendor's terms and must leave the row as it is. + expect((await setActive(h, dana.scimId, false)).status).toBe(200); + const banned = await userRow(h, dana.email); + expect(isBanned(banned)).toBe(true); + expect(banned?.ban_reason).toBe(SCIM_DEACTIVATION_BAN_REASON); + + expect((await setActive(h, dana.scimId, true)).status).toBe(200); + expect((await setActive(h, dana.scimId, true)).status).toBe(200); + expect(isBanned(await userRow(h, dana.email))).toBe(false); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// (c) — the break-glass guard holds on the REAL SCIM path +// --------------------------------------------------------------------------- + +describe('[#14360] deactivating the last administrator is refused through SCIM, loudly', () => { + it('(c) the IdP gets a 403 SCIM error naming the invariant, and the account stays active', async () => { + const h = await boot(); + const owner = await provision(h, 'owner'); + await attachPassword(h, owner); + await makePlatformAdmin(h, owner.userId); + await expectSignInAccepted(h, owner.email); + + const res = await setActive(h, owner.scimId, false); + const body = (await res.json()) as { schemas?: string[]; status?: string; detail?: string }; + // The SCIM 2.0 error envelope, status AND detail — never an opaque 500: + // the guard's whole product is the explanation the IdP operator reads. + expect(res.status, `expected the guard's 403, got ${res.status} ${JSON.stringify(body)}`).toBe(403); + expect(body.schemas ?? []).toContain(SCIM_ERROR_SCHEMA); + expect(String(body.status)).toBe('403'); + expect(body.detail).toMatch(/last administrator/i); + expect(body.detail).toMatch(/ADR-0024 D5\.2/); + expect(body.detail).toMatch(/SCIM deprovision is too broad/); + + // Nothing landed — not the ban, and not the vendor's own `active` write + // either: the throw aborted the SCIM transaction as a whole. + const row = await userRow(h, owner.email); + expect(isBanned(row)).toBe(false); + expect(row?.ban_reason ?? null).toBeNull(); + expect(await scimActive(h, owner.scimId)).toBe(true); + await expectSignInAccepted(h, owner.email); + }, 60_000); + + it('(c) positive control: with a second administrator left behind, the same request succeeds', async () => { + const h = await boot(); + const owner = await provision(h, 'owner'); + await makePlatformAdmin(h, owner.userId); + // The survivor — provisioned by the same IdP, holding the same standing. + const deputy = await provision(h, 'deputy'); + await makePlatformAdmin(h, deputy.userId); + + const res = await setActive(h, owner.scimId, false); + expect(res.status, `expected the deactivation to proceed: ${await res.clone().text()}`).toBe(200); + expect(isBanned(await userRow(h, owner.email))).toBe(true); + expect(await scimActive(h, owner.scimId)).toBe(false); + + // …and the deputy is now the last one: refused. + const last = await setActive(h, deputy.scimId, false); + expect(last.status).toBe(403); + expect(isBanned(await userRow(h, deputy.email))).toBe(false); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// (d) — negative controls +// --------------------------------------------------------------------------- + +describe('[#14360] a SCIM update that does not change `active` touches no ban column', () => { + it('(d) an attribute-only PATCH on an active user leaves `banned` alone and revokes nothing', async () => { + const h = await boot(); + const erin = await provision(h, 'erin'); + await attachPassword(h, erin); + await expectSignInAccepted(h, erin.email); + const sessionsBefore = await sessionCount(h, erin.userId); + expect(sessionsBefore).toBeGreaterThan(0); + + const res = await setDisplayName(h, erin.scimId, 'Erin Renamed'); + expect(res.status, `SCIM PATCH displayName failed: ${await res.clone().text()}`).toBe(200); + + const row = await userRow(h, erin.email); + expect(isBanned(row)).toBe(false); + expect(row?.ban_reason ?? null).toBeNull(); + // Still active ⇒ the vendor revoked nothing either. + expect(await sessionCount(h, erin.userId)).toBe(sessionsBefore); + await expectSignInAccepted(h, erin.email); + }, 60_000); + + it("(d) an administrator's ban survives an IdP attribute sync AND an explicit active:true", async () => { + const h = await boot(); + const frank = await provision(h, 'frank'); + await attachPassword(h, frank); + + // The ban an administrator placed for cause — the platform ban write's + // effect, with a reason that is not the IdP's. + await h.engine.update( + 'sys_user', + { id: frank.userId, banned: true, ban_reason: 'Policy violation' }, + SYSTEM, + ); + await expectSignInBanned(h, frank.email); + + // Every IdP PUT carries `active: true`; an attribute sync must not + // silently re-admit a user banned for cause. + expect((await setDisplayName(h, frank.scimId, 'Frank Renamed')).status).toBe(200); + let row = await userRow(h, frank.email); + expect(isBanned(row)).toBe(true); + expect(row?.ban_reason).toBe('Policy violation'); + + // Nor may an explicit reactivation: the reason is not SCIM's, so the ban + // is not SCIM's to lift. + expect((await setActive(h, frank.scimId, true)).status).toBe(200); + row = await userRow(h, frank.email); + expect(isBanned(row)).toBe(true); + expect(row?.ban_reason).toBe('Policy violation'); + await expectSignInBanned(h, frank.email); + + // And a deactivation on an already-banned row overwrites nothing: the + // administrator's reason is the record of why. + expect((await setActive(h, frank.scimId, false)).status).toBe(200); + row = await userRow(h, frank.email); + expect(isBanned(row)).toBe(true); + expect(row?.ban_reason).toBe('Policy violation'); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// (e) — the #13816 construction-time refusal is untouched +// --------------------------------------------------------------------------- + +describe('[#14360] a host that declines the admin plugin beside SCIM is still refused at construction', () => { + it('(e) plugins.admin:false + plugins.scim:true throws the #13816 conflict', () => { + expect( + () => + new AuthManager({ + secret: SECRET, + baseUrl: BASE, + plugins: { scim: true, admin: false }, + } as never), + ).toThrow(/conflicting auth plugin configuration/); + }); +}); + +// --------------------------------------------------------------------------- +// (f) — DELETE: the tombstoned account is disabled, not left signing in +// --------------------------------------------------------------------------- + +describe('[#14360] DELETE /Users/{id} leaves the tombstoned account disabled', () => { + it('(f) the sys_user row survives the vendor tombstone, banned with the SCIM reason, sign-in refused', async () => { + const h = await boot(); + const gus = await provision(h, 'gus'); + await attachPassword(h, gus); + await expectSignInAccepted(h, gus.email); + + const res = await h.send(scimRequest(h, 'DELETE', `/Users/${gus.scimId}`)); + expect(res.status, `SCIM DELETE /Users/{id} failed: ${await res.clone().text()}`).toBe(204); + + // 1.7.2 no longer deletes the better-auth user — it tombstones the SCIM + // source and reports the aggregate state as inactive. Without this card + // that account kept its password and kept signing in. + const row = await userRow(h, gus.email); + expect(row, 'the better-auth user row is tombstoned, not deleted, on 1.7.2').toBeTruthy(); + expect(isBanned(row)).toBe(true); + expect(row?.ban_reason).toBe(SCIM_DEACTIVATION_BAN_REASON); + expect(await sessionCount(h, gus.userId)).toBe(0); + await expectSignInBanned(h, gus.email); + }, 60_000); +}); From 51000d482e74210cbaca49aaee7eaf3c798cdf93 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 10:18:12 +0000 Subject: [PATCH 2/3] test(plugin-auth): pin the refused-deactivation residual the dead SCIM transaction scope leaves (#14522) The (c) face keeps the account enabled and the SCIM 403 shape; the vendor's own scimUser.active write surviving the refusal is pinned as observed and attributed to the adapter's SCIM transaction scoping never engaging on 1.7.2, so the fix for that seam flips the pin deliberately. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../plugin-auth/src/admin-ban-endpoints.ts | 5 +-- .../plugins/plugin-auth/src/auth-manager.ts | 25 +++++++++++---- .../scim-deactivation-reconcile-user.test.ts | 32 +++++++++++++------ 3 files changed, 44 insertions(+), 18 deletions(-) diff --git a/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts index 86dceaeebf..f60c53e84b 100644 --- a/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts +++ b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts @@ -107,8 +107,9 @@ export interface AdminBanEndpointDeps { * * `UserBanWriter` is the narrowest surface the write needs, and both callers * already hold one: better-auth's `internalAdapter` satisfies it directly; the - * SCIM hook adapts the vendor's transaction-bound `DBTransactionAdapter`, so - * its write commits — or rolls back — with the SCIM mutation it belongs to. + * SCIM hook adapts the `DBTransactionAdapter` the vendor bound to its + * transaction, so the write commits — or rolls back — with the SCIM mutation + * it belongs to once that transaction is real on this adapter (#14522). * * Session revocation is deliberately NOT part of the write: the admin mount * revokes explicitly (below), and the SCIM vendor revokes after its callback diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 28d0aa19c1..e9d5c5ebdf 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -4901,9 +4901,20 @@ export class AuthManager { * `APIError`, the vendor re-throws `APIError`s unchanged out of this * callback (`runSCIMApplicationCallback`, measured on 1.7.2 — any other * throw becomes a SCIM 500 "SCIM identity reconciliation failed" carrying - * the original as `cause`), the transaction rolls back, and the IdP - * receives a SCIM error with `status: "403"` and the guard's own - * explanation. The account stays active; nothing is skipped silently. + * the original as `cause`), and the IdP receives a SCIM error with + * `status: "403"` and the guard's own explanation. The ban is ONE write, + * so it never half-lands: the account stays enabled and nothing is + * skipped silently. + * + * ⚠️ What does NOT roll back today: the vendor runs this callback inside + * `runWithTransaction`, which on this adapter is a real engine transaction + * only while `scimRequestScope` is set — and that scope, stamped inside + * `verifyBearerToken`, is not observed at write time on 1.7.2 (measured: + * zero `engine.transaction` calls across a SCIM POST + PATCH; #14522). So + * the vendor's own `scimUser.active = false` write, made before this + * callback, survives a refusal and the SCIM resource reads inactive while + * the account is enabled. #14522 owns that seam; the #14360 suite pins the + * residual so its fix flips the pin deliberately. * * Deliberately NOT applied here: the last-LOCAL-credential guard the admin * mount re-runs (`isLastLocalCredentialHolder`). That guard protects the @@ -4914,10 +4925,10 @@ export class AuthManager { * never applied it on the SCIM path either — the vendor wrote the column * straight through the adapter. * - * Every read and write goes through `context.database` — the vendor's - * transaction-bound adapter — never through an `internalAdapter` resolved - * outside it, so the ban commits or rolls back with the SCIM mutation it - * belongs to. + * Every read and write goes through `context.database` — the adapter the + * vendor bound to its transaction — never through an `internalAdapter` + * resolved outside it, so the moment #14522 makes that transaction real, + * the ban commits or rolls back with the SCIM mutation it belongs to. */ private async reconcileScimUserLifecycle( state: SCIMIdentityState, diff --git a/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts b/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts index 0c8a0e94ed..7c18279b42 100644 --- a/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts +++ b/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts @@ -84,6 +84,7 @@ import { registerLastAdminGuard, type LastAdminGuardEngine } from './last-admin- import { registerIdentityWriteGuard, registerManagedUpdateWhitelist } from './identity-write-guard.js'; import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js'; import { SCIM_DEACTIVATION_BAN_REASON } from './admin-ban-endpoints.js'; +import { CREDENTIAL_ISSUER } from './backfill-account-issuer.js'; const BASE = 'http://localhost:3000'; const AUTH = `${BASE}/api/v1/auth`; @@ -310,18 +311,21 @@ async function sessionCount(h: Harness, userId: string): Promise { /** * The "local-password user" the card describes: an IdP-provisioned account an - * administrator later gave a password (the `set-user-password` shape). Written - * through better-auth's own context, the same seam `set-initial-password` - * uses; the address is marked verified because the IdP asserted it. + * administrator later gave a password. Written exactly as the platform's own + * `/admin/set-user-password` mount writes it for an SSO/invite-onboarded user + * (`admin-user-endpoints.ts`: hash, then `createAccount` with the local + * credential issuer — 1.7.2's sign-in accepts no other credential row). The + * address is marked verified because the IdP asserted it. */ async function attachPassword(h: Harness, user: Provisioned): Promise { const ctx = await h.manager.getAuthContext(); - const hash = await ctx.password.hash(PASSWORD); - await ctx.internalAdapter.linkAccount({ + const hashed = await ctx.password.hash(PASSWORD); + await ctx.internalAdapter.createAccount({ userId: user.userId, providerId: 'credential', + issuer: CREDENTIAL_ISSUER, accountId: user.userId, - password: hash, + password: hashed, }); await h.engine.update('sys_user', { id: user.userId, email_verified: true }, SYSTEM); } @@ -460,13 +464,23 @@ describe('[#14360] deactivating the last administrator is refused through SCIM, expect(body.detail).toMatch(/ADR-0024 D5\.2/); expect(body.detail).toMatch(/SCIM deprovision is too broad/); - // Nothing landed — not the ban, and not the vendor's own `active` write - // either: the throw aborted the SCIM transaction as a whole. + // The ban did not land and the administrator still signs in — the + // invariant the guard exists for. const row = await userRow(h, owner.email); expect(isBanned(row)).toBe(false); expect(row?.ban_reason ?? null).toBeNull(); - expect(await scimActive(h, owner.scimId)).toBe(true); await expectSignInAccepted(h, owner.email); + + // RESIDUAL — pinned as observed, filed as #14522, ⛔ not this card's to + // fix: the vendor's own `scimUser.active = false` write, made BEFORE the + // callback inside what it believes is a transaction, survives the + // refusal, because the adapter's #3653 SCIM transaction scoping never + // opens an engine transaction on 1.7.2 (measured: 0 `engine.transaction` + // and 0 `driver.beginTransaction` calls across POST + PATCH /Users). So + // the SCIM resource reports `active: false` while the account is still + // enabled. When #14522 lands, this line flips to `true` DELIBERATELY — + // that is the whole reason it is asserted rather than left unread. + expect(await scimActive(h, owner.scimId)).toBe(false); }, 60_000); it('(c) positive control: with a second administrator left behind, the same request succeeds', async () => { From 21c7dbe76b7d44df32fbd9f6497db6372b9f2d0f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 11:02:49 +0000 Subject: [PATCH 3/3] fix(plugin-auth): make a deactivated principal's expiring ban permanent; move the shared ban write off the barrel Contract review round 1 (#14360): a SCIM active:false over an administrator's TIMED ban left the expiry in place, and the vendor's session hook auto-lifts an expired ban - so the principal was re-admitted while the IdP still held them deactivated. The hook now clears banExpires on that row (reason and banned untouched). The shared write moves to the package-internal user-ban-write.ts (not re-exported from index.ts), so no new public symbol ships and the changeset drops to patch. Two faces added: the expiring-ban overlap and POST /Users with active:false. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../scim-active-false-reconcile-user.md | 6 +- .../plugin-auth/src/admin-ban-endpoints.ts | 75 +----------------- .../plugins/plugin-auth/src/auth-manager.ts | 31 ++++++-- .../scim-deactivation-reconcile-user.test.ts | 76 ++++++++++++++++-- .../plugins/plugin-auth/src/user-ban-write.ts | 77 +++++++++++++++++++ 5 files changed, 180 insertions(+), 85 deletions(-) create mode 100644 packages/plugins/plugin-auth/src/user-ban-write.ts diff --git a/.changeset/scim-active-false-reconcile-user.md b/.changeset/scim-active-false-reconcile-user.md index 130319895f..8c73bf4a0a 100644 --- a/.changeset/scim-active-false-reconcile-user.md +++ b/.changeset/scim-active-false-reconcile-user.md @@ -1,7 +1,5 @@ --- -"@objectstack/plugin-auth": minor +"@objectstack/plugin-auth": patch --- -SCIM `active: false` disables the account again. Stable `@better-auth/scim` (1.7.0+) no longer writes the admin plugin's `banned` column itself — it hands the aggregate lifecycle state to an optional host callback, `identity.reconcileUser`, and only revokes sessions. `plugin-auth` passed no `identity` member, so an identity provider deactivating a user revoked sessions and wrote nothing: `sys_user.banned` stayed false and a user holding a local password signed straight back in. `AuthManager` now implements the callback and routes it to the platform's own ban write: `active: false` bans the user (reason `Deactivated via SCIM`, no expiry) and the vendor's `BANNED_USER` sign-in refusal applies; `active: true` lifts a ban that carries that reason — an administrator's ban (any other reason) is not the identity provider's to lift, so an attribute sync never re-admits a user banned for cause. The break-glass last-administrator guard (ADR-0024 D5.2) judges the write at the engine, so deactivating the last administrator through SCIM is refused with a 403 SCIM error and the account stays active. A SCIM `DELETE /Users/{id}` — which on 1.7.2 tombstones the source rather than deleting the user — now leaves that account disabled too. - -New exports from `@objectstack/plugin-auth` (the shared write the SCIM hook and the `/admin/ban-user` mount both call): `applyUserBan`, `applyUserUnban`, `SCIM_DEACTIVATION_BAN_REASON`, and the `UserBanWriter` / `UserBanFields` types. +SCIM `active: false` disables the account again. Stable `@better-auth/scim` (1.7.0+) no longer writes the admin plugin's `banned` column itself — it hands the aggregate lifecycle state to an optional host callback, `identity.reconcileUser`, and only revokes sessions. `plugin-auth` passed no `identity` member, so an identity provider deactivating a user revoked sessions and wrote nothing: `sys_user.banned` stayed false and a user holding a local password signed straight back in. `AuthManager` now implements the callback and routes it to the platform's own ban write: `active: false` bans the user (reason `Deactivated via SCIM`, no expiry) — and makes an administrator's existing EXPIRING ban permanent (`banExpires` cleared, the administrator's reason kept), because the vendor's session hook auto-lifts an expired ban and would otherwise admit a principal the identity provider still holds deactivated — and the vendor's `BANNED_USER` sign-in refusal applies; `POST /Users` with `active: false` provisions the account disabled. `active: true` lifts a ban that carries that reason — an administrator's ban (any other reason) is not the identity provider's to lift, so an attribute sync never re-admits a user banned for cause; the one documented collision is an administrator who types the reason `Deactivated via SCIM` themselves, which produces a ban the identity provider can lift. The last-LOCAL-credential guard the `/admin/ban-user` mount re-runs is deliberately not applied on the SCIM path: an identity-provider deprovision can disable the last password-holding account while non-administrator SSO users remain. The break-glass last-administrator guard (ADR-0024 D5.2) judges the write at the engine, so deactivating the last administrator through SCIM is refused with a 403 SCIM error and the account stays active. A SCIM `DELETE /Users/{id}` — which on 1.7.2 tombstones the source rather than deleting the user — now leaves that account disabled too. No new public symbol: the shared write lives in a package-internal module. diff --git a/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts index f60c53e84b..caa962d632 100644 --- a/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts +++ b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts @@ -51,7 +51,9 @@ * The writes mirror better-auth's own handlers field for field — `banned` / * `banReason` / `banExpires` / `updatedAt`, then `deleteUserSessions` — so a * banned user is signed out and refused at sign-in by the vendor's OWN session - * hook (`BANNED_USER`), which is untouched. The default ban reason is + * hook (`BANNED_USER`), which is untouched. The write itself lives in the + * package-internal `user-ban-write.ts` (#14360), shared with the SCIM + * deprovisioning hook in `auth-manager.ts` — one write, two callers. The default ban reason is * `'No reason'` because ObjectStack configures no `defaultBanReason`. * * ⚠️ Shadowing a vendor route detaches every better-auth hook keyed on its @@ -72,6 +74,7 @@ import { type CredentialAccountAdapter, } from './last-local-credential.js'; import type { AdminActor, EndpointResult } from './admin-user-endpoints.js'; +import { applyUserBan, applyUserUnban } from './user-ban-write.js'; /** * Minimal better-auth `$context` surface these two routes touch. Mirrors what @@ -90,76 +93,6 @@ export interface AdminBanEndpointDeps { getAuthContext(): Promise; } -/** - * [#14360] The `sys_user` ban write, as ONE callable both ban paths share. - * - * Two callers land a disable on this platform: the `/admin/ban-user` mount - * below, and — since `@better-auth/scim` 1.7.0 stopped writing `banned` - * itself and handed the host an `identity.reconcileUser` callback instead — - * the SCIM `active: false` deprovisioning hook in `auth-manager.ts` - * (`reconcileScimUserLifecycle`). Field for field this is the write - * better-auth's own `banUser` handler makes (`banned` / `banReason` / - * `banExpires` / `updatedAt`), so the vendor's `session.create` hook - * (`BANNED_USER`) enforces both halves identically. The break-glass - * last-administrator guard (ADR-0024 D5.2, `last-admin-guard.ts`) judges the - * write at the ENGINE, so it holds on both callers by construction — neither - * can reach the column without passing it. - * - * `UserBanWriter` is the narrowest surface the write needs, and both callers - * already hold one: better-auth's `internalAdapter` satisfies it directly; the - * SCIM hook adapts the `DBTransactionAdapter` the vendor bound to its - * transaction, so the write commits — or rolls back — with the SCIM mutation - * it belongs to once that transaction is real on this adapter (#14522). - * - * Session revocation is deliberately NOT part of the write: the admin mount - * revokes explicitly (below), and the SCIM vendor revokes after its callback - * returns (`deleteUserSessions` on `active: false`, measured on 1.7.2). - */ -export interface UserBanWriter { - updateUser(id: string, data: Record): Promise; -} - -/** - * The reason the SCIM deactivation path stamps on the row — the exact string - * `@better-auth/scim` wrote itself through 1.6.x - * (`resolveSCIMActiveDeactivation`, `dist/index.mjs` at 1.6.30), kept verbatim - * so a row the vendor banned before the 1.7.0 decoupling and a row the host - * hook bans are the same fact. It is also how the reactivation half - * recognises its OWN ban: an `active: true` from the IdP lifts a ban carrying - * this reason and leaves an administrator's ban (any other reason) in place. - */ -export const SCIM_DEACTIVATION_BAN_REASON = 'Deactivated via SCIM'; - -export interface UserBanFields { - banReason: string; - /** `undefined` leaves the column untouched; `null` clears a prior expiry. */ - banExpires?: Date | null; -} - -/** Disable `userId` — the vendor-shaped ban write, on whichever writer the caller is inside. */ -export async function applyUserBan( - writer: UserBanWriter, - userId: string, - ban: UserBanFields, -): Promise { - await writer.updateUser(userId, { - banned: true, - banReason: ban.banReason, - ...(ban.banExpires !== undefined ? { banExpires: ban.banExpires } : {}), - updatedAt: new Date(), - }); -} - -/** Re-enable `userId` — clears the three ban columns, exactly as the vendor's `unbanUser` does. */ -export async function applyUserUnban(writer: UserBanWriter, userId: string): Promise { - await writer.updateUser(userId, { - banned: false, - banReason: null, - banExpires: null, - updatedAt: new Date(), - }); -} - const invalid = (message: string): EndpointResult => ({ status: 400, body: { success: false, error: { code: 'INVALID_REQUEST', message } }, diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index e9d5c5ebdf..2ae509b93c 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -106,7 +106,7 @@ import { applyUserBan, applyUserUnban, SCIM_DEACTIVATION_BAN_REASON, -} from './admin-ban-endpoints.js'; +} from './user-ban-write.js'; import { PHONE_SMS_TOPICS, builtinPhoneSmsBody, @@ -4878,11 +4878,22 @@ export class AuthManager { * - `active: false` on a row that is not banned ⇒ `applyUserBan` with * `SCIM_DEACTIVATION_BAN_REASON` and no expiry. The vendor's * `session.create` hook (`BANNED_USER`) then refuses sign-in — the same - * enforcement the admin ban has, because it is the same write. + * enforcement the admin ban has, because it is the same write. On a row + * that is ALREADY banned with an expiry (an administrator's timed ban), + * the deactivation makes that ban permanent — `banExpires` is cleared, + * `banned` and the administrator's reason are left untouched — because + * the vendor's session hook auto-lifts an expired ban and would admit a + * principal the IdP still holds deactivated, and this callback is not + * re-invoked until the IdP mutates that user again. * - `active: true` on a row banned WITH that reason ⇒ `applyUserUnban`. * A ban carrying any other reason was placed by an administrator and is * not the IdP's to lift: an attribute sync (every SCIM PUT carries * `active: true`) must not silently re-admit a user banned for cause. + * Known collision, documented rather than reserved: an administrator + * who types the reason `Deactivated via SCIM` on the admin mount + * produces a ban this rule reads as the IdP's, so an `active: true` + * lifts it. Reserving the string on the admin mount would change that + * surface, which is not this hook's to do. * - Anything else is a no-op. The callback is contractually idempotent * ("Implementations must be idempotent") and the vendor invokes it on * EVERY user mutation, so a PATCH that changes only `displayName` @@ -4935,7 +4946,7 @@ export class AuthManager { context: SCIMTransactionContext, ): Promise { const db = context.database; - const user = await db.findOne<{ banned?: unknown; banReason?: unknown }>({ + const user = await db.findOne<{ banned?: unknown; banReason?: unknown; banExpires?: unknown }>({ model: 'user', where: [{ field: 'id', value: state.userId }], }); @@ -4955,8 +4966,18 @@ export class AuthManager { }; const banned = user.banned === true; if (!state.active) { - // Already disabled — by an earlier SCIM pass or by an administrator. - if (banned) return; + if (banned) { + // Already disabled — by an earlier SCIM pass or by an administrator. + // An administrator's TIMED ban is made permanent: the vendor's session + // hook auto-lifts an expired ban, and nothing re-invokes this callback + // until the IdP mutates the user again — so left alone, the expiry + // would re-admit a principal the IdP still holds deactivated. The + // reason stays the administrator's; only the expiry goes. + if (user.banExpires !== null && user.banExpires !== undefined) { + await writer.updateUser(state.userId, { banExpires: null, updatedAt: new Date() }); + } + return; + } await applyUserBan(writer, state.userId, { banReason: SCIM_DEACTIVATION_BAN_REASON, banExpires: null, diff --git a/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts b/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts index 7c18279b42..8a4d445f4e 100644 --- a/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts +++ b/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts @@ -51,6 +51,10 @@ * construction (#13816 — unchanged by this card). * (f) `DELETE /Users/{id}` leaves the tombstoned account disabled (the * vendor no longer deletes the better-auth user on 1.7.2). + * (g) `POST /Users` with `active: false` provisions the account disabled. + * And, under (d): a deactivation makes an administrator's EXPIRING ban + * permanent, so the vendor's auto-lift cannot re-admit a deactivated + * principal. */ import { describe, it, expect, afterEach } from 'vitest'; @@ -83,7 +87,7 @@ import { mintScimConnectionCredential } from './scim-connection-service.js'; import { registerLastAdminGuard, type LastAdminGuardEngine } from './last-admin-guard.js'; import { registerIdentityWriteGuard, registerManagedUpdateWhitelist } from './identity-write-guard.js'; import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js'; -import { SCIM_DEACTIVATION_BAN_REASON } from './admin-ban-endpoints.js'; +import { SCIM_DEACTIVATION_BAN_REASON } from './user-ban-write.js'; import { CREDENTIAL_ISSUER } from './backfill-account-issuer.js'; const BASE = 'http://localhost:3000'; @@ -245,8 +249,8 @@ interface Provisioned { email: string; } -/** `POST /Users` — an IdP provisioning an active user. */ -async function provision(h: Harness, localPart: string): Promise { +/** `POST /Users` — an IdP provisioning a user, active unless the case says otherwise. */ +async function provision(h: Harness, localPart: string, active = true): Promise { const email = `${localPart}@example.com`; const res = await h.send( scimRequest(h, 'POST', '/Users', { @@ -255,12 +259,12 @@ async function provision(h: Harness, localPart: string): Promise { name: { givenName: localPart, familyName: 'Example' }, displayName: `${localPart} Example`, emails: [{ value: email, primary: true, type: 'work' }], - active: true, + active, }), ); expect(res.status, `SCIM POST /Users failed: ${await res.clone().text()}`).toBe(201); const body = (await res.json()) as { id: string; active?: boolean }; - expect(body.active).toBe(true); + expect(body.active).toBe(active); const row = await userRow(h, email); expect(row, 'the SCIM create must have landed a sys_user row').toBeTruthy(); return { scimId: body.id, userId: String(row!.id), email }; @@ -563,6 +567,44 @@ describe('[#14360] a SCIM update that does not change `active` touches no ban co expect(isBanned(row)).toBe(true); expect(row?.ban_reason).toBe('Policy violation'); }, 60_000); + + it("(d) a deactivation makes an administrator's EXPIRING ban permanent — the expiry cannot re-admit a deactivated principal", async () => { + const h = await boot(); + const hana = await provision(h, 'hana'); + await attachPassword(h, hana); + + // A timed administrator ban: the vendor's session hook auto-lifts it the + // moment `banExpires` is in the past, and nothing re-invokes the SCIM + // callback until the IdP mutates the user again — so an expiry left in + // place would ADMIT a principal the IdP still holds deactivated. + const expiresAt = new Date(Date.now() + 1_500); + await h.engine.update( + 'sys_user', + { id: hana.userId, banned: true, ban_reason: 'Policy violation', ban_expires: expiresAt }, + SYSTEM, + ); + expect((await userRow(h, hana.email))?.ban_expires ?? null).not.toBeNull(); + await expectSignInBanned(h, hana.email); + + const res = await setActive(h, hana.scimId, false); + expect(res.status, `SCIM PATCH active:false failed: ${await res.clone().text()}`).toBe(200); + let row = await userRow(h, hana.email); + expect(isBanned(row)).toBe(true); + // The administrator's reason is kept — the ban stays theirs to lift. + expect(row?.ban_reason).toBe('Policy violation'); + // …and only the expiry is gone. + expect(row?.ban_expires ?? null).toBeNull(); + + // Let the administrator's expiry pass, then prove the refusal still holds + // (status AND code): without the clearing above the vendor would have + // auto-unbanned here and answered 2xx. + await new Promise((resolve) => setTimeout(resolve, 2_000)); + expect(Date.now()).toBeGreaterThan(expiresAt.getTime()); + await expectSignInBanned(h, hana.email); + row = await userRow(h, hana.email); + expect(isBanned(row)).toBe(true); + expect(row?.ban_reason).toBe('Policy violation'); + }, 60_000); }); // --------------------------------------------------------------------------- @@ -582,6 +624,30 @@ describe('[#14360] a host that declines the admin plugin beside SCIM is still re }); }); +// --------------------------------------------------------------------------- +// (g) — POST with active:false: provisioned disabled from the first write +// --------------------------------------------------------------------------- + +describe('[#14360] POST /Users with active:false provisions the account disabled', () => { + it('(g) the created user is banned with the SCIM reason and refused at sign-in', async () => { + const h = await boot(); + // The vendor invokes the callback on create too; 1.6.x banned at + // creation as well, so this is restored behaviour, pinned so it is + // declared rather than incidental. + const ivan = await provision(h, 'ivan', false); + + const row = await userRow(h, ivan.email); + expect(isBanned(row)).toBe(true); + expect(row?.ban_reason).toBe(SCIM_DEACTIVATION_BAN_REASON); + expect(row?.ban_expires ?? null).toBeNull(); + + // Even with a local password attached afterwards, the refusal holds — + // status AND code. + await attachPassword(h, ivan); + await expectSignInBanned(h, ivan.email); + }, 60_000); +}); + // --------------------------------------------------------------------------- // (f) — DELETE: the tombstoned account is disabled, not left signing in // --------------------------------------------------------------------------- diff --git a/packages/plugins/plugin-auth/src/user-ban-write.ts b/packages/plugins/plugin-auth/src/user-ban-write.ts new file mode 100644 index 0000000000..4df1ffbe68 --- /dev/null +++ b/packages/plugins/plugin-auth/src/user-ban-write.ts @@ -0,0 +1,77 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14360] The `sys_user` ban write, as ONE callable both ban paths share — + * PACKAGE-INTERNAL by design. This module is deliberately NOT re-exported from + * `index.ts` (the same posture as `scim-connection-service.ts` and the other + * off-barrel plugin-auth modules): nothing here is a published symbol, so the + * two callers can share the write without the package growing a public API it + * would then owe forever. + * + * Two callers land a disable on this platform: the `/admin/ban-user` mount in + * `admin-ban-endpoints.ts`, and — since `@better-auth/scim` 1.7.0 stopped + * writing `banned` itself and handed the host an `identity.reconcileUser` + * callback instead — the SCIM `active: false` deprovisioning hook in + * `auth-manager.ts` (`reconcileScimUserLifecycle`). Field for field this is the + * write better-auth's own `banUser` handler makes (`banned` / `banReason` / + * `banExpires` / `updatedAt`), so the vendor's `session.create` hook + * (`BANNED_USER`) enforces both halves identically. The break-glass + * last-administrator guard (ADR-0024 D5.2, `last-admin-guard.ts`) judges the + * write at the ENGINE, so it holds on both callers by construction — neither + * can reach the column without passing it. + * + * `UserBanWriter` is the narrowest surface the write needs, and both callers + * already hold one: better-auth's `internalAdapter` satisfies it directly; the + * SCIM hook adapts the `DBTransactionAdapter` the vendor bound to its + * transaction, so the write commits — or rolls back — with the SCIM mutation + * it belongs to once that transaction is real on this adapter (#14522). + * + * Session revocation is deliberately NOT part of the write: the admin mount + * revokes explicitly, and the SCIM vendor revokes after its callback returns + * (`deleteUserSessions` on `active: false`, measured on 1.7.2). + */ + +export interface UserBanWriter { + updateUser(id: string, data: Record): Promise; +} + +/** + * The reason the SCIM deactivation path stamps on the row — the exact string + * `@better-auth/scim` wrote itself through 1.6.x + * (`resolveSCIMActiveDeactivation`, `dist/index.mjs` at 1.6.30), kept verbatim + * so a row the vendor banned before the 1.7.0 decoupling and a row the host + * hook bans are the same fact. It is also how the reactivation half + * recognises its OWN ban: an `active: true` from the IdP lifts a ban carrying + * this reason and leaves an administrator's ban (any other reason) in place. + */ +export const SCIM_DEACTIVATION_BAN_REASON = 'Deactivated via SCIM'; + +export interface UserBanFields { + banReason: string; + /** `undefined` leaves the column untouched; `null` clears a prior expiry. */ + banExpires?: Date | null; +} + +/** Disable `userId` — the vendor-shaped ban write, on whichever writer the caller is inside. */ +export async function applyUserBan( + writer: UserBanWriter, + userId: string, + ban: UserBanFields, +): Promise { + await writer.updateUser(userId, { + banned: true, + banReason: ban.banReason, + ...(ban.banExpires !== undefined ? { banExpires: ban.banExpires } : {}), + updatedAt: new Date(), + }); +} + +/** Re-enable `userId` — clears the three ban columns, exactly as the vendor's `unbanUser` does. */ +export async function applyUserUnban(writer: UserBanWriter, userId: string): Promise { + await writer.updateUser(userId, { + banned: false, + banReason: null, + banExpires: null, + updatedAt: new Date(), + }); +}