diff --git a/.changeset/scim-active-false-reconcile-user.md b/.changeset/scim-active-false-reconcile-user.md new file mode 100644 index 0000000000..8c73bf4a0a --- /dev/null +++ b/.changeset/scim-active-false-reconcile-user.md @@ -0,0 +1,5 @@ +--- +"@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 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 2c992572ce..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 @@ -161,11 +164,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 +198,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..2ae509b93c 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 './user-ban-write.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,140 @@ 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. 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` + * 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`), 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 + * 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 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, + context: SCIMTransactionContext, + ): Promise { + const db = context.database; + const user = await db.findOne<{ banned?: unknown; banReason?: unknown; banExpires?: 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) { + 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, + }); + 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..8a4d445f4e --- /dev/null +++ b/packages/plugins/plugin-auth/src/scim-deactivation-reconcile-user.test.ts @@ -0,0 +1,675 @@ +// 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). + * (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'; +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 './user-ban-write.js'; +import { CREDENTIAL_ISSUER } from './backfill-account-issuer.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 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', { + schemas: [USER_SCHEMA], + userName: email, + name: { givenName: localPart, familyName: 'Example' }, + displayName: `${localPart} Example`, + emails: [{ value: email, primary: true, type: 'work' }], + 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(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 }; +} + +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. 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 hashed = await ctx.password.hash(PASSWORD); + await ctx.internalAdapter.createAccount({ + userId: user.userId, + providerId: 'credential', + issuer: CREDENTIAL_ISSUER, + accountId: user.userId, + password: hashed, + }); + 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/); + + // 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(); + 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 () => { + 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); + + 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); +}); + +// --------------------------------------------------------------------------- +// (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/); + }); +}); + +// --------------------------------------------------------------------------- +// (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 +// --------------------------------------------------------------------------- + +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); +}); 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(), + }); +}