From c416dc7a4838b99cdb1bf973b89af9598d60c0a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 13:58:59 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat(plugin-auth):=20break-glass=20?= =?UTF-8?q?=E5=AE=88=E5=8D=AB=E7=9A=84=20delete=20=E5=8D=8A=E8=BE=B9=20?= =?UTF-8?q?=E2=80=94=E2=80=94=20=E6=9C=80=E5=90=8E=E4=B8=80=E4=B8=AA?= =?UTF-8?q?=E7=AE=A1=E7=90=86=E5=91=98=E4=B9=9F=E5=88=A0=E4=B8=8D=E6=8E=89?= =?UTF-8?q?=20(#5941)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #5892 / PR #5939 只守住了 ban(`sys_user.banned = true`,`beforeUpdate`)。 删除这条路径今天没有任何一段代码判断「管理员还剩几个」:`auth-manager.ts` 那段 HTTP 守卫判的是「最后一个本地 credential 持有人」,目标不持本地密码 (enforced SSO 下 IdP 托管 / SCIM JIT provision 出来的管理员)时整段跳过, 于是 SCIM `DELETE /Users/{id}` / `/admin/remove-user` 可以把环境最后一个 管理员的行删掉,环境从此无人可管。 本次把同一条不变量装到 `sys_user` 的 `beforeDelete` 上,与 ban 半边同形状、 同一份管理员枚举:装在写上而不是端点上(SCIM adapter、admin remove-user、 导入、脚本都覆盖),by-id 与谓词/multi(含无谓词的清表 multi)都守,对 `isSystem` 同样生效(真正会锁死的正是 system 那条),并且 fail-closed —— 枚举读不出来或超出 maxScan 一律拒绝。 实测(写进了模块头注释):`beforeDelete` 的 by-id 派发给 `input.id`, 谓词/multi 派发把行域谓词放在 `input.options.where`;`ctx.previous` (引擎 #5272 预取 + objectql 的 `sys_fetch_previous_delete` 内建)只在 by-id 形状上绑定,批量派发上是 undefined ——守卫因此完全不消费它,只解析目标 id 集。 拒绝走 `PERMISSION_DENIED` + 403,信息按调用方实际执行的动作措辞 (「Refusing to delete '…'」),并给出 ADR-0024 D5.2 依据与修复办法; `withValidationErrorMapping` 逐方法包装,#5939 加的 403 arm 原样覆盖 `delete`。 模块随之更名为 `last-admin-guard.ts` / `registerLastAdminGuard`(原 `last-admin-ban-guard.ts`,同一个未发布周期内加入):它现在注册两个钩子, 旧名字会低估它装了什么。 ⛔ 未动 `auth-manager.ts:1239-1300` 的 fail-open HTTP 守卫段(分诊明令, 它继续守自己的 credential 版不变量,两层并存)。撤销管理员「身份」的第三条 写法(改/删 `sys_member` 行、撤 `admin_full_access` 授权)另开 #5978。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- .changeset/last-admin-delete-break-glass.md | 58 +++ .../plugins/plugin-auth/src/auth-plugin.ts | 23 +- packages/plugins/plugin-auth/src/index.ts | 14 +- ...guard.test.ts => last-admin-guard.test.ts} | 393 +++++++++++++++++- ...admin-ban-guard.ts => last-admin-guard.ts} | 272 ++++++++---- 5 files changed, 645 insertions(+), 115 deletions(-) create mode 100644 .changeset/last-admin-delete-break-glass.md rename packages/plugins/plugin-auth/src/{last-admin-ban-guard.test.ts => last-admin-guard.test.ts} (54%) rename packages/plugins/plugin-auth/src/{last-admin-ban-guard.ts => last-admin-guard.ts} (53%) diff --git a/.changeset/last-admin-delete-break-glass.md b/.changeset/last-admin-delete-break-glass.md new file mode 100644 index 0000000000..7027bf7649 --- /dev/null +++ b/.changeset/last-admin-delete-break-glass.md @@ -0,0 +1,58 @@ +--- +"@objectstack/plugin-auth": minor +--- + +feat(plugin-auth): break-glass — the last administrator cannot be DELETED either (#5941) + +#5892 closed the *ban* half of ADR-0024 D5.2's break-glass invariant. The +**delete** half was still open, and it was reachable end to end: in an enforced +SSO environment the last administrator is typically IdP-managed and holds no +local password, so when the IdP drops them from the admin group the resulting +SCIM `DELETE /Users/{id}` (or `/admin/remove-user`, or `/delete-user`) removed +the row and **left the environment with nobody able to administer it** — quite +possibly with a password-holding non-admin still able to sign in and change +nothing. There is no recovery path from inside the product once that happens. + +The pre-existing HTTP guard on those three endpoints did not cover it: it +protects the last holder of a local `credential` account, so it skips the +credential-less (IdP-managed) target entirely. It is unchanged and keeps +enforcing its own invariant. + +**What changed.** The guard module now enforces one invariant on *both* writes +that can take the last administrator away, off one administrator enumeration: + +| write | hook | +|:--|:--| +| `sys_user.banned = true` | `beforeUpdate` (#5892) | +| deleting the `sys_user` row | `beforeDelete` (**new**) | + +The delete half is the ban half's twin in every property that matters: it sits +on the **write**, so it holds for the SCIM adapter delete, better-auth's admin +remove-user, an import and a script alike; it covers by-id **and** +predicate/`multi` deletes (including the unpredicated `multi` that would empty +the table); it applies to **every** context, `isSystem` included, because the +deprovision path that actually locks organizations out is the system one; and it +**fails closed** — an administrator population that cannot be read, or is too +large to enumerate, refuses the delete rather than guessing. + +The refusal is a **403** carrying `PERMISSION_DENIED` and names the operation +the caller actually attempted ("Refusing to delete 'usr_…'"), the invariant +(ADR-0024 D5.2), and the fix — grant someone else `admin_full_access` or an +owner/admin membership first, and if an IdP drove it, the SCIM deprovision is +too broad. On the auth pipeline it surfaces as an `APIError`, not an opaque 500. + +Untouched: deleting anyone who is not an administrator, deleting an +administrator while another unbanned one remains, and deleting an administrator +who is already banned (that account could not sign in either way). + +**Rename.** The module is now `last-admin-guard.ts` and the exported registration +function is `registerLastAdminGuard` (was `last-admin-ban-guard.ts` / +`registerLastAdminBanGuard`, added in the same unreleased cycle) — it registers +both hooks, so the old name would have understated what it installs. Hosts that +wire the guard onto their own ObjectQL engine rename the import; there is no +other change to its signature or behaviour. + +Not covered, tracked separately (#5978): revoking the *standing* that makes +someone an administrator — deleting or downgrading their `sys_member` row, +removing the `admin_full_access` grant — leaves the user row in place and writes +a different table, so neither hook sees it. diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 6723bda63d..865c24563c 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -41,7 +41,7 @@ import { registerManagedUpdateWhitelist, type SecondaryStorageLike, } from './identity-write-guard.js'; -import { registerLastAdminBanGuard } from './last-admin-ban-guard.js'; +import { registerLastAdminGuard } from './last-admin-guard.js'; import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js'; import { MANAGED_EXTENSION_EDITABLE_FIELDS } from './managed-extension-fields.js'; import { runSetInitialPassword } from './set-initial-password.js'; @@ -988,17 +988,18 @@ export class AuthPlugin implements Plugin { this.effectiveSecondaryStorage as SecondaryStorageLike | undefined, }); // [cloud ADR-0024 D5.2] Break-glass — the SAME `sys_user` write - // chokepoint, guarding a different question: not "may this caller + // chokepoints, guarding a different question: not "may this caller // write identity tables" (above, and system writes bypass it by - // design) but "may this VALUE be written at all". A `banned = true` - // that would leave the environment with no administrator able to sign - // in is refused for EVERY context, `isSystem` included — because the - // path that actually locks an org out is the system one (better-auth's - // admin ban, driven by a SCIM `active: false`). Registered at - // priority 20 so the ADR-0092 strip above (10) still answers first for - // user-context callers. See last-admin-ban-guard.ts. - registerLastAdminBanGuard(engine, { - packageId: 'com.objectstack.plugin-auth.last-admin-ban-guard', + // design) but "may this WRITE happen at all". A `banned = true` (#5892) + // or a row DELETE (#5941) that would leave the environment with no + // administrator able to sign in is refused for EVERY context, + // `isSystem` included — because the paths that actually lock an org out + // are the system ones (better-auth's admin ban and remove-user, driven + // by a SCIM `active: false` / `DELETE /Users/{id}`). Registered at + // priority 20 so the ADR-0092 checks above (10) still answer first for + // user-context callers. See last-admin-guard.ts. + registerLastAdminGuard(engine, { + packageId: 'com.objectstack.plugin-auth.last-admin-guard', logger: ctx.logger, }); } catch { diff --git a/packages/plugins/plugin-auth/src/index.ts b/packages/plugins/plugin-auth/src/index.ts index ec8612e1e4..eca49e068c 100644 --- a/packages/plugins/plugin-auth/src/index.ts +++ b/packages/plugins/plugin-auth/src/index.ts @@ -20,12 +20,14 @@ export * from './admin-user-endpoints.js'; export * from './placeholder-email.js'; export * from './admin-import-users.js'; export * from './identity-write-guard.js'; -// [cloud ADR-0024 D5.2 / #5892] The break-glass ban guard. Exported for the -// same reason its ADR-0092 neighbour above is: a host that stands up its own -// ObjectQL engine (the cloud control plane, an embedding that skips this -// plugin's `kernel:ready` wiring) has to be able to register the invariant -// itself rather than ship an environment that can ban its last administrator. -export * from './last-admin-ban-guard.js'; +// [cloud ADR-0024 D5.2 / #5892 / #5941] The break-glass guard — one invariant +// on both writes that can take the last administrator away (`banned = true` +// and deleting the `sys_user` row). Exported for the same reason its ADR-0092 +// neighbour above is: a host that stands up its own ObjectQL engine (the cloud +// control plane, an embedding that skips this plugin's `kernel:ready` wiring) +// has to be able to register the invariant itself rather than ship an +// environment that can ban or delete its last administrator. +export * from './last-admin-guard.js'; export * from './sys-user-writable-fields.js'; export * from './otp-send-guard.js'; // ADR-0069 D2 / #4772 — the cross-node rate-limit counter store (kernel cache, diff --git a/packages/plugins/plugin-auth/src/last-admin-ban-guard.test.ts b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts similarity index 54% rename from packages/plugins/plugin-auth/src/last-admin-ban-guard.test.ts rename to packages/plugins/plugin-auth/src/last-admin-guard.test.ts index b32dd62cc7..d5290e6eb8 100644 --- a/packages/plugins/plugin-auth/src/last-admin-ban-guard.test.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts @@ -1,7 +1,9 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * [#5892 / cloud ADR-0024 D5.2] The break-glass ban guard. + * [#5892 + #5941 / cloud ADR-0024 D5.2] The break-glass guard — both halves of + * one invariant: a `banned = true` write and a `sys_user` row DELETE may each + * only proceed while an administrator who can sign in is left behind. * * ## Why there is no fake engine here * @@ -22,18 +24,22 @@ * * ## The two faces * - * 1. **The engine write** — `engine.update('sys_user', …)`, by-id and - * predicate/multi, which is every path that reaches the column. - * 2. **The SCIM / admin-ban path** — the same refusal driven through + * 1. **The engine write** — `engine.update('sys_user', …)` / + * `engine.delete('sys_user', …)`, by-id and predicate/multi, which is every + * path that reaches the row. + * 2. **The SCIM / admin path** — the same refusals driven through * `createObjectQLAdapterFactory`, the adapter `@better-auth/scim`'s - * `active: false` → admin ban actually writes through, asserting it - * surfaces as a 403 `APIError` and not an opaque 500. + * `active: false` → admin ban and its `DELETE /Users/{id}` → remove-user + * actually write through, asserting each surfaces as a 403 `APIError` and + * not an opaque 500. * * Reverse verification (recorded because the direction is not obvious): with - * `registerLastAdminBanGuard` NOT called, the "last administrator" cases below - * are GREEN-as-in-the-ban-succeeds — `engine.update` resolves and the row comes - * back `banned = 1`. That is the pre-#5892 behaviour, and it is what every - * `rejects.toThrow` here is measured against. + * `registerLastAdminGuard` NOT called, the "last administrator" cases below are + * GREEN-as-in-the-write-succeeds — `engine.update` resolves and the row comes + * back `banned = 1`; `engine.delete` resolves and the row is GONE. That is the + * pre-#5892 / pre-#5941 behaviour, and it is what every `rejects.toThrow` here + * is measured against; the two `unguarded: true` cases at the bottom of the + * file re-run it on the same fixtures rather than describing it. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; @@ -42,7 +48,7 @@ import { SqlDriver } from '@objectstack/driver-sql'; import { isAPIError } from 'better-auth/api'; import { ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; import { SystemUserId } from '@objectstack/spec/system'; -import { registerLastAdminBanGuard, type LastAdminBanGuardEngine } from './last-admin-ban-guard.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 { createObjectQLAdapterFactory } from './objectql-adapter.js'; @@ -56,7 +62,7 @@ import { admin } from 'better-auth/plugins/admin'; const sysUser = { name: 'sys_user', label: 'User', - // The ADR-0092 guard keys off this; the ban guard deliberately does not. + // The ADR-0092 guard keys off this; the break-glass guard deliberately does not. managedBy: 'better-auth', fields: { id: { name: 'id', type: 'text' as const, primaryKey: true }, @@ -88,6 +94,24 @@ const sysPermissionSet = { }, }; +/** + * [#5941] Only the delete cases use this one, and the guard reads NOTHING from + * it — that is the point. `auth-manager.ts`'s HTTP break-glass guard judges the + * last holder of a local `credential` account, so an IdP-managed administrator + * (no `credential` row) walks straight past it. Seeding the accounts makes the + * fixture the environment the issue describes instead of a paraphrase of it. + */ +const sysAccount = { + name: 'sys_account', + label: 'Account', + managedBy: 'better-auth', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + user_id: { name: 'user_id', type: 'text' as const }, + provider_id: { name: 'provider_id', type: 'text' as const }, + }, +}; + const sysUserPermissionSet = { name: 'sys_user_permission_set', label: 'User Permission Set', @@ -110,18 +134,28 @@ async function ban(engine: ObjectQL, id: string): Promise { return engine.update('sys_user', { id, banned: true }, SYSTEM); } +/** …and so is every removal: better-auth's adapter deletes by resolved id. */ +async function removeUser(engine: ObjectQL, id: string): Promise { + return engine.delete('sys_user', { where: { id }, ...SYSTEM }); +} + async function bannedFlag(engine: ObjectQL, id: string): Promise { const row = await engine.findOne('sys_user', { where: { id }, fields: ['id', 'banned'] }, SYSTEM); return row?.banned; } +async function userExists(engine: ObjectQL, id: string): Promise { + const row = await engine.findOne('sys_user', { where: { id }, fields: ['id'] }, SYSTEM); + return Boolean(row); +} + interface BootOptions { /** Overrides the engine the GUARD reads through (hook stays on the real one). */ - readThrough?: (engine: ObjectQL) => LastAdminBanGuardEngine; + readThrough?: (engine: ObjectQL) => LastAdminGuardEngine; maxScan?: number; /** Register the ADR-0092 identity write guard alongside, at its own priority. */ withIdentityWriteGuard?: boolean; - /** Skip registration entirely — the pre-#5892 engine, for reverse verification. */ + /** Skip registration entirely — the pre-guard engine, for reverse verification. */ unguarded?: boolean; } @@ -149,7 +183,7 @@ async function boot(opts: BootOptions = {}): Promise { true, ); await engine.init(); - for (const o of [sysUser, sysMember, sysPermissionSet, sysUserPermissionSet]) { + for (const o of [sysUser, sysMember, sysPermissionSet, sysUserPermissionSet, sysAccount]) { engine.registry.registerObject(o as never); } await engine.syncSchemas(); @@ -159,8 +193,8 @@ async function boot(opts: BootOptions = {}): Promise { registerIdentityWriteGuard(engine, { packageId: 'test.identity-write-guard' }); } if (!opts.unguarded) { - registerLastAdminBanGuard(opts.readThrough?.(engine) ?? (engine as unknown as LastAdminBanGuardEngine), { - packageId: 'test.last-admin-ban-guard', + registerLastAdminGuard(opts.readThrough?.(engine) ?? (engine as unknown as LastAdminGuardEngine), { + packageId: 'test.last-admin-guard', ...(opts.maxScan !== undefined ? { maxScan: opts.maxScan } : {}), }); } @@ -171,13 +205,27 @@ async function boot(opts: BootOptions = {}): Promise { async function seedUser( engine: ObjectQL, id: string, - extra: { role?: string; platformAdmin?: boolean; banned?: boolean; grant?: Record } = {}, + extra: { + role?: string; + platformAdmin?: boolean; + banned?: boolean; + grant?: Record; + /** `credential` = holds a local password; anything else = IdP-managed. */ + accountProvider?: string; + } = {}, ): Promise { await engine.insert( 'sys_user', { id, name: id, email: `${id}@example.com`, banned: extra.banned ?? false }, SYSTEM, ); + if (extra.accountProvider) { + await engine.insert( + 'sys_account', + { id: `acc_${id}`, user_id: id, provider_id: extra.accountProvider }, + SYSTEM, + ); + } if (extra.role) { await engine.insert( 'sys_member', @@ -495,11 +543,305 @@ describe('[#5892] the SCIM / admin-ban path: refused as a 403, not an opaque 500 }); }); +// --------------------------------------------------------------------------- +// [#5941] The delete half — same invariant, the other write +// --------------------------------------------------------------------------- + +describe('[#5941] break-glass: the last unbanned administrator cannot be DELETED', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + }); + + it('two org admins: deleting the first is allowed, deleting the last is refused', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + await seedUser(engine, 'usr_member', { role: 'member' }); + + // One of two — the environment keeps an administrator, so this proceeds. + await expect(removeUser(engine, 'usr_admin')).resolves.toBeDefined(); + expect(await userExists(engine, 'usr_admin')).toBe(false); + + // The last one — refused, and the row is still there. + await expect(removeUser(engine, 'usr_owner')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + object: 'sys_user', + }); + expect(await userExists(engine, 'usr_owner')).toBe(true); + }); + + it('the refusal explains itself: the operation, which user, why, and the fix', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + // The verb is the caller's own: an operator who ran a SCIM + // `DELETE /Users/{id}` must not be told a "ban" was refused. + await expect(removeUser(engine, 'usr_owner')).rejects.toThrow(/Refusing to delete 'usr_owner'/); + await expect(removeUser(engine, 'usr_owner')).rejects.toThrow(/last administrator/i); + await expect(removeUser(engine, 'usr_owner')).rejects.toThrow(/deleting that account/); + await expect(removeUser(engine, 'usr_owner')).rejects.toThrow(/ADR-0024 D5\.2/); + await expect(removeUser(engine, 'usr_owner')).rejects.toThrow(new RegExp(ADMIN_FULL_ACCESS)); + await expect(removeUser(engine, 'usr_owner')).rejects.toThrow(/SCIM deprovision is too broad/); + }); + + it( + 'the reachable chain: an IdP-managed last admin (no local credential) removed by a SYSTEM ' + + 'caller is refused', + async () => { + // Exactly #5941's environment. `usr_idp_owner` is SCIM JIT-provisioned: + // its only account is the IdP one, so `auth-manager.ts`'s HTTP guard — + // which fires only when the TARGET holds a local `credential` account — + // skips this delete entirely. `usr_escape` is the password-holding + // non-admin the issue describes: able to sign in, unable to administer + // anything, which is the state ADR-0024 D5.2 exists to prevent. + await seedUser(engine, 'usr_idp_owner', { role: 'owner', accountProvider: 'oidc' }); + await seedUser(engine, 'usr_escape', { role: 'member', accountProvider: 'credential' }); + + // A SCIM deprovision runs as system — the context that bypasses ADR-0092 + // by design, and the one this guard therefore also covers. + await expect(removeUser(engine, 'usr_idp_owner')).rejects.toThrow(/last administrator/i); + expect(await userExists(engine, 'usr_idp_owner')).toBe(true); + + // The credential-less-ness of the target changed nothing: the guard + // counts administrators, never password holders. + expect( + await engine.findOne( + 'sys_account', + { where: { user_id: 'usr_idp_owner', provider_id: 'credential' } }, + SYSTEM, + ), + ).toBeFalsy(); + }, + ); + + it('deleting a non-administrator is untouched, even when exactly one admin exists', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_member', { role: 'member', accountProvider: 'credential' }); + await seedUser(engine, 'usr_nobody'); + + await expect(removeUser(engine, 'usr_member')).resolves.toBeDefined(); + await expect(removeUser(engine, 'usr_nobody')).resolves.toBeDefined(); + expect(await userExists(engine, 'usr_member')).toBe(false); + expect(await userExists(engine, 'usr_nobody')).toBe(false); + }); + + it('a platform admin (unscoped admin_full_access) counts, a SCOPED grant does not', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + await seedUser(engine, 'usr_scoped', { grant: { organization_id: ORG } }); + + await expect(removeUser(engine, 'usr_scoped')).resolves.toBeDefined(); + await expect(removeUser(engine, 'usr_platform')).rejects.toThrow(/last administrator/i); + }); + + it('`delegated_admin` and `usr_system` are never counted as the survivor', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_delegate', { role: 'delegated_admin' }); + await seedUser(engine, SystemUserId.SYSTEM, { platformAdmin: true }); + + await expect(removeUser(engine, 'usr_owner')).rejects.toThrow(/last administrator/i); + }); + + it('an ALREADY-banned administrator can be deleted (nothing is being taken away)', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner', banned: true }); + + await expect(removeUser(engine, 'usr_owner')).resolves.toBeDefined(); + expect(await userExists(engine, 'usr_owner')).toBe(false); + }); + + it('deleting a row on another object is not this guard\'s business', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + // The membership row (the thing that MAKES usr_owner an administrator) is + // a different write shape on a different table — filed as #5978, and + // deliberately not half-guarded from here. Pinned so the day it IS guarded, + // this expectation is the one that has to be changed on purpose. + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_owner' }, ...SYSTEM }), + ).resolves.toBeDefined(); + }); +}); + +describe('[#5941] the delete guard holds on predicate (multi) deletes, not only by-id', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + await seedUser(engine, 'usr_member', { role: 'member' }); + }); + + it('a predicate that would sweep every administrator is refused', async () => { + // `input.id` is unbound on this dispatch and `ctx.previous` is never + // fetched for it — the guard reads the predicate off `input.options.where` + // and resolves the doomed rows itself. + await expect( + engine.delete('sys_user', { multi: true, where: { banned: false }, ...SYSTEM }), + ).rejects.toThrow(/last administrator/i); + expect(await userExists(engine, 'usr_owner')).toBe(true); + expect(await userExists(engine, 'usr_admin')).toBe(true); + expect(await userExists(engine, 'usr_member')).toBe(true); + }); + + it('an `$in` predicate naming both admins is refused — a scalar-id read would have missed it', async () => { + await expect( + engine.delete('sys_user', { + multi: true, + where: { id: { $in: ['usr_owner', 'usr_admin'] } }, + ...SYSTEM, + }), + ).rejects.toThrow(/last administrators/i); + expect(await userExists(engine, 'usr_owner')).toBe(true); + }); + + it('an unpredicated `multi` delete — the one that empties the table — is refused', async () => { + await expect(engine.delete('sys_user', { multi: true, ...SYSTEM })).rejects.toThrow( + /last administrators/i, + ); + expect(await userExists(engine, 'usr_owner')).toBe(true); + }); + + it('a predicate that spares one administrator proceeds', async () => { + await expect( + engine.delete('sys_user', { + multi: true, + where: { id: { $in: ['usr_admin', 'usr_member'] } }, + ...SYSTEM, + }), + ).resolves.toBeDefined(); + expect(await userExists(engine, 'usr_admin')).toBe(false); + expect(await userExists(engine, 'usr_owner')).toBe(true); + }); +}); + +describe('[#5941] the delete guard fails CLOSED too', () => { + it('a failing identity read refuses the delete and names the reason', async () => { + const engine = await boot({ + readThrough: (real) => ({ + registerHook: (event, handler, options) => real.registerHook(event, handler, options), + find: async () => { + throw new Error('sys_member is unreadable'); + }, + }), + }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + + // Two admins exist — this delete WOULD be legal. It is refused anyway, + // because the guard could not prove it. + await expect(removeUser(engine, 'usr_admin')).rejects.toThrow(/Refusing this delete/); + await expect(removeUser(engine, 'usr_admin')).rejects.toThrow(/could not be verified/i); + await expect(removeUser(engine, 'usr_admin')).rejects.toThrow(/sys_member is unreadable/); + await expect(removeUser(engine, 'usr_admin')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + }); + expect(await userExists(engine, 'usr_admin')).toBe(true); + }); + + it('a population larger than the guard can enumerate refuses the delete', async () => { + const engine = await boot({ maxScan: 1 }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + + await expect(removeUser(engine, 'usr_admin')).rejects.toThrow(/more than 1 rows/); + expect(await userExists(engine, 'usr_admin')).toBe(true); + }); + + it('an environment with no administrator at all is not blocked (nothing to protect)', async () => { + const engine = await boot(); + await seedUser(engine, 'usr_a'); + await seedUser(engine, 'usr_b'); + + await expect(removeUser(engine, 'usr_a')).resolves.toBeDefined(); + }); +}); + +describe('[#5941] a USER-CONTEXT delete still gets the ADR-0092 answer, not this one', () => { + it('the identity write guard (priority 10) answers first for a data-API caller', async () => { + const engine = await boot({ withIdentityWriteGuard: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + // ADR-0092 refuses identity deletes through the data API outright, so a + // user-context caller is told THAT — the accurate answer for that surface — + // rather than the break-glass one. + await expect( + engine.delete('sys_user', { + where: { id: 'usr_owner' }, + context: { isSystem: false, userId: 'usr_caller', positions: [], permissions: [] }, + }), + ).rejects.toThrow(/managed by better-auth/); + expect(await userExists(engine, 'usr_owner')).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// [#5941] Face 2 — the SCIM `DELETE /Users/{id}` / admin remove-user path +// --------------------------------------------------------------------------- + +describe('[#5941] the SCIM / admin remove-user path: refused as a 403, not an opaque 500', () => { + let engine: ObjectQL; + let adapter: { + delete: (args: { model: string; where: unknown[] }) => Promise; + }; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + // The IdP-managed pair again, this time driven through the production + // adapter factory exactly as `AuthManager` builds it. + await seedUser(engine, 'usr_owner', { role: 'owner', accountProvider: 'oidc' }); + await seedUser(engine, 'usr_admin', { role: 'admin', accountProvider: 'oidc' }); + adapter = (createObjectQLAdapterFactory(engine) as unknown as (o: unknown) => typeof adapter)({ + plugins: [admin({ schema: buildAdminPluginSchema() })], + }); + }); + + /** What `@better-auth/scim`'s `DELETE /Users/{id}` ultimately writes. */ + const removeThroughAdapter = (userId: string) => + adapter.delete({ + model: 'user', + where: [{ field: 'id', value: userId, operator: 'eq', connector: 'AND' }], + }); + + it('removing the second-to-last administrator succeeds', async () => { + await expect(removeThroughAdapter('usr_admin')).resolves.toBeUndefined(); + expect(await userExists(engine, 'usr_admin')).toBe(false); + }); + + it('removing the LAST administrator is refused with a 403 APIError', async () => { + await removeThroughAdapter('usr_admin'); + + let caught: unknown; + try { + await removeThroughAdapter('usr_owner'); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + // Same requirement as the ban half: a raw engine error would reach the IdP + // as a 500 with no explanation. `withValidationErrorMapping` wraps every + // adapter method, so the 403 arm #5939 added covers `delete` unchanged. + expect(isAPIError(caught)).toBe(true); + const api = caught as { statusCode: number; body: { code?: string; message?: string } }; + expect(api.statusCode).toBe(403); + expect(api.body.code).toBe('PERMISSION_DENIED'); + expect(api.body.message).toMatch(/last administrator/i); + expect(await userExists(engine, 'usr_owner')).toBe(true); + }); +}); + // --------------------------------------------------------------------------- // Reverse verification — the same fixtures with the guard NOT registered // --------------------------------------------------------------------------- -describe('[#5892] reverse verification: without the guard, the lockout goes through', () => { +describe('[#5892 / #5941] reverse verification: without the guard, the lockout goes through', () => { it('the pre-#5892 engine bans the last administrator and reports success', async () => { const engine = await boot({ unguarded: true }); await seedAdminPermissionSet(engine); @@ -508,4 +850,17 @@ describe('[#5892] reverse verification: without the guard, the lockout goes thro await expect(ban(engine, 'usr_owner')).resolves.toBeTruthy(); expect(await bannedFlag(engine, 'usr_owner')).toBeTruthy(); }); + + it('the pre-#5941 engine DELETES the last administrator and the row is gone', async () => { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner', accountProvider: 'oidc' }); + await seedUser(engine, 'usr_escape', { role: 'member', accountProvider: 'credential' }); + + await expect(removeUser(engine, 'usr_owner')).resolves.toBeDefined(); + expect(await userExists(engine, 'usr_owner')).toBe(false); + // …and what is left is the issue's end state: a password holder who can + // sign in and administer nothing. + expect(await userExists(engine, 'usr_escape')).toBe(true); + }); }); diff --git a/packages/plugins/plugin-auth/src/last-admin-ban-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts similarity index 53% rename from packages/plugins/plugin-auth/src/last-admin-ban-guard.ts rename to packages/plugins/plugin-auth/src/last-admin-guard.ts index 0121692777..ca709d728f 100644 --- a/packages/plugins/plugin-auth/src/last-admin-ban-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -4,20 +4,28 @@ * [cloud ADR-0024 D5.2] Break-glass — a write may never leave this environment * with ZERO administrators able to sign in. * - * `sys_user.banned = true` is how EVERY deprovision lands: the better-auth - * admin plugin's ban endpoint writes it, and `@better-auth/scim` maps a SCIM - * `active: false` onto that same admin ban (which is why SCIM forces the admin - * plugin on — ADR-0071). SCIM writes are driven by an EXTERNAL system: nobody - * reads the payload before it commits, so one mis-scoped IdP group or one - * over-broad deprovision run is enough for an organization to ban its own last + * TWO writes can take the last administrator away, and this guard holds on + * both — they are one invariant, not two policies: + * + * 1. **`sys_user.banned = true`** (#5892) — how every *disable* lands: the + * better-auth admin plugin's ban endpoint writes it, and + * `@better-auth/scim` maps a SCIM `active: false` onto that same admin ban + * (which is why SCIM forces the admin plugin on — ADR-0071). + * 2. **deleting the `sys_user` row** (#5941) — how every *remove* lands: SCIM + * `DELETE /Users/{id}`, better-auth's `/admin/remove-user` and + * `/delete-user`, an import, a script. + * + * In the case that matters both are driven by an EXTERNAL system: nobody reads + * the payload before it commits, so one mis-scoped IdP group or one over-broad + * deprovision run is enough for an organization to remove its own last * administrator and lock itself out of its environment permanently. There is no * recovery path from inside the product once that happens. * - * So the invariant is enforced at the WRITE, on the one chokepoint every path - * goes through — `beforeUpdate` on `sys_user` — rather than at any individual - * endpoint. HTTP-level guards protect only the endpoint they are attached to; - * this one holds for the admin ban endpoint, the SCIM adapter write, an import, - * a script, and anything added later. + * So the invariant is enforced at the WRITE, on the two chokepoints every path + * goes through — `beforeUpdate` and `beforeDelete` on `sys_user` — rather than + * at any individual endpoint. HTTP-level guards protect only the endpoints they + * are attached to; these hold for the admin ban / remove endpoints, the SCIM + * adapter writes, an import, a script, and anything added later. * * ## What counts as an administrator * @@ -40,16 +48,52 @@ * service account is not loginable, so it can never be the escape hatch (the * same exclusion the first-admin bootstrap makes). * + * The population both halves protect is the administrators who can sign in + * TODAY — the ones whose row is not already banned. A ban that takes the last + * of them is refused; so is a delete. Conversely a write aimed at an + * administrator who is ALREADY banned takes nothing away (that account cannot + * sign in either way), so it is not this guard's business. + * * ## Fail-closed * - * Every lookup this guard makes is part of a SAFETY proof: a ban is permitted - * only when at least one other unbanned administrator is provably left. A - * lookup that fails, or a population too large to enumerate, proves nothing — - * so the ban is REFUSED, loudly, with the reason. That is the opposite of the - * fail-OPEN posture the neighbouring last-local-credential guard takes in - * `auth-manager.ts` (an HTTP-level convenience check whose failure mode is a - * blocked legitimate op); here the failure mode is a permanent lockout, so the - * two directions are chosen deliberately and are not a drift. + * Every lookup this guard makes is part of a SAFETY proof: the write is + * permitted only when at least one other unbanned administrator is provably + * left. A lookup that fails, or a population too large to enumerate, proves + * nothing — so the write is REFUSED, loudly, with the reason. That is the + * opposite of the fail-OPEN posture the neighbouring last-local-credential + * guard takes in `auth-manager.ts`; the two directions are chosen deliberately + * and are not a drift. + * + * ## Relationship to the `auth-manager.ts` break-glass HTTP guard + * + * `auth-manager.ts` already guards `/delete-user`, `/admin/remove-user` and + * `/admin/ban-user` — but it answers a DIFFERENT question: "is the target the + * last holder of a local `credential` account", i.e. the password escape hatch + * that survives an IdP outage. When the target holds no local credential it + * skips entirely, which is exactly the shape #5941 reported: under enforced SSO + * the last administrator is IdP-managed (SCIM JIT-provisioned, no password), so + * that guard never fires and the row is removed. It is also fail-OPEN by + * design, because its failure mode is a blocked legitimate operation rather + * than a lockout. Both properties are right FOR IT, so it is left untouched and + * keeps enforcing its own invariant; this hook is the fail-closed one that + * counts *administrators*, and it covers every write path rather than three + * endpoints. + * + * ## What a `beforeDelete` can see (measured on this engine, #5929 included) + * + * - **by-id** (`delete(obj, { where: { id } })` — what better-auth's adapter + * emits, and what every cascade recursion re-enters with): `input.id` + * carries the scalar id. + * - **predicate / `multi`**: `input.id` is unbound and the row-scoping + * predicate rides on `input.options.where` — the same shape #5273 pinned + * for update. + * - `ctx.previous` (the engine's #5272 pre-image, and objectql's + * `sys_fetch_previous_delete` builtin — `object: '*'`, priority 5) is bound + * for the by-id shape ONLY; a batch dispatch names no single row, so it + * stays undefined there. The guard therefore never consumes it: it needs the + * target IDS, not a pre-image, and a `previous`-based implementation would + * be correct by-id and blind on exactly the bulk path that can sweep every + * administrator at once. * * ## Scope: the ENVIRONMENT, not each organization * @@ -59,18 +103,26 @@ * policy with its own product decisions (what happens to an org whose only * owner leaves the company); it is deliberately not invented here. * + * Scope in the other direction: this guard watches the two writes that take the + * administrator away WITH THEIR ROW. Revoking the standing that MAKES someone + * an administrator — deleting their `sys_member` row, downgrading its role, + * removing the `admin_full_access` grant — leaves the user in place and writes + * a different table, so neither hook here sees it. Same end state, third write + * shape; filed as #5978 rather than half-guarded from this file. + * * ## Relationship to the ADR-0092 identity write guard * * `identity-write-guard.ts` answers "may this CALLER write identity tables * through the generic data path" and bypasses system-context writes by design — * better-auth's own adapter is exactly what it must let through. This guard - * answers a different question, "may this VALUE be written at all", and - * therefore applies to EVERY context, `isSystem` included: the ban path that - * actually causes lockouts is the system one. The two are registered together - * (`auth-plugin.ts`, `kernel:ready`) and ordered so the ADR-0092 strip runs - * first (priority 10 → 20): a user-context caller keeps getting the ADR-0092 - * message ("`banned` is not editable via the data API"), and only the writes - * that legitimately carry `banned` reach this guard. + * answers a different question, "may this WRITE happen at all", and therefore + * applies to EVERY context, `isSystem` included: the deprovision path that + * actually locks organizations out is the system one. The two are registered + * together (`auth-plugin.ts`, `kernel:ready`) and ordered so the ADR-0092 + * checks run first (priority 10 → 20): a user-context caller keeps getting the + * ADR-0092 answer (`banned` is not editable through the data API; an identity + * row is not deletable through it at all), and only the writes that legitimately + * reach the identity tables reach this guard. */ import type { BaseEngineOptions, EngineQueryOptions } from '@objectstack/spec/data'; @@ -99,7 +151,7 @@ type LoggerLike = { * reads. Structural rather than `IObjectQLEngine` so the guard can be driven * directly in tests without standing up an engine. */ -export interface LastAdminBanGuardEngine { +export interface LastAdminGuardEngine { registerHook( event: string, handler: (ctx: unknown) => Promise, @@ -112,13 +164,13 @@ export interface LastAdminBanGuardEngine { ): Promise>>; } -export interface LastAdminBanGuardOptions { +export interface LastAdminGuardOptions { packageId: string; logger?: LoggerLike; /** * Largest row count any one enumeration read may return before the guard * gives up and refuses (fail-closed). The administrator population of an - * environment is tiny; this exists so a pathological predicate ban — or a + * environment is tiny; this exists so a pathological predicate write — or a * `sys_member` table with tens of thousands of non-plain-member rows — * cannot be silently under-counted into a lockout. Default 1000. */ @@ -130,6 +182,19 @@ const DEFAULT_MAX_SCAN = 1000; /** Reads run as system: this is a safety proof, never RLS-scoped to a caller. */ const SYSTEM_READ: BaseEngineOptions = { context: { isSystem: true } }; +/** + * The two writes this guard judges. Carried into every message so a refusal + * describes the operation the caller actually attempted — an operator reading + * "refusing this ban" after a SCIM `DELETE /Users/{id}` would go looking in the + * wrong place. + */ +type GuardedOp = 'ban' | 'delete'; + +const OP_WORDS: Record = { + ban: { noun: 'ban', verb: 'ban', gerund: 'banning', Verb: 'Ban' }, + delete: { noun: 'delete', verb: 'delete', gerund: 'deleting', Verb: 'Delete' }, +}; + /** * Boolean columns arrive spelled by whichever driver / transport wrote them: * better-auth's adapter is configured `supportsBooleans: false` (so it hands @@ -170,49 +235,54 @@ function toId(value: unknown): string | undefined { } /** - * Register the last-administrator ban guard on an ObjectQL engine. + * Register the last-administrator guard on an ObjectQL engine: the ban half + * (`beforeUpdate`) and the delete half (`beforeDelete`) of ONE invariant, off + * one administrator enumeration. * * Idempotent per package the same way the identity write guard is: a caller * re-binding after a hot reload runs `engine.unregisterHooksByPackage(packageId)` * first. */ -export function registerLastAdminBanGuard( - engine: LastAdminBanGuardEngine, - opts: LastAdminBanGuardOptions, +export function registerLastAdminGuard( + engine: LastAdminGuardEngine, + opts: LastAdminGuardOptions, ): void { const { packageId, logger } = opts; const maxScan = opts.maxScan ?? DEFAULT_MAX_SCAN; /** Enumerate `object` under a hard ceiling; overflow proves nothing → refuse. */ const scan = async ( + op: GuardedOp, object: string, query: EngineQueryOptions, ): Promise>> => { const rows = await engine.find(object, { ...query, limit: maxScan + 1 }, SYSTEM_READ); const list = Array.isArray(rows) ? rows : []; if (list.length > maxScan) { + const words = OP_WORDS[op]; throw refuse( - `Refusing this ban: '${object}' returned more than ${maxScan} rows, so the remaining ` + - `administrators could not be verified (${BREAK_GLASS_CITATION}). Ban a narrower set of ` + - 'users, or raise the guard\'s maxScan if this environment really is that large.', + `Refusing this ${words.noun}: '${object}' returned more than ${maxScan} rows, so the ` + + `remaining administrators could not be verified (${BREAK_GLASS_CITATION}). ` + + `${words.Verb} a narrower set of users, or raise the guard's maxScan if this ` + + 'environment really is that large.', ); } return list; }; /** Every user this environment currently recognises as an administrator. */ - const resolveAdminUserIds = async (): Promise> => { + const resolveAdminUserIds = async (op: GuardedOp): Promise> => { const ids = new Set(); const now = Date.now(); // 1) Platform admins — unscoped, in-window `admin_full_access` grants. - const sets = await scan(SystemObjectName.PERMISSION_SET, { + const sets = await scan(op, SystemObjectName.PERMISSION_SET, { where: { name: ADMIN_FULL_ACCESS }, fields: ['id', 'name'], }); const adminSetIds = sets.map((r) => toId(r.id)).filter((v): v is string => Boolean(v)); if (adminSetIds.length > 0) { - const links = await scan(USER_PERMISSION_SET, { + const links = await scan(op, USER_PERMISSION_SET, { where: { permission_set_id: { $in: adminSetIds } }, }); for (const link of links) { @@ -233,7 +303,7 @@ export function registerLastAdminBanGuard( // only owner for an ordinary member. Narrowed to non-plain-member rows // so a large membership table is not read wholesale; the grade test // itself still runs in memory, over every row that narrowing kept. - const members = await scan(SystemObjectName.MEMBER, { + const members = await scan(op, SystemObjectName.MEMBER, { where: { role: { $ne: MEMBERSHIP_ROLE_MEMBER } }, }); for (const m of members) { @@ -249,8 +319,11 @@ export function registerLastAdminBanGuard( }; /** Of `adminIds`, those whose `sys_user` row is present and not banned. */ - const resolveUnbannedAdmins = async (adminIds: Set): Promise> => { - const rows = await scan(SystemObjectName.USER, { + const resolveUnbannedAdmins = async ( + op: GuardedOp, + adminIds: Set, + ): Promise> => { + const rows = await scan(op, SystemObjectName.USER, { where: { id: { $in: [...adminIds] } }, fields: ['id', 'banned'], }); @@ -264,18 +337,23 @@ export function registerLastAdminBanGuard( return out; }; - /** Which `sys_user` rows this one update writes to. */ + /** + * Which `sys_user` rows this one write addresses — the same answer for both + * halves. A scalar id when the engine dispatched by id (an update payload + * also carries it in `data.id`; a delete's `input` has no `data` at all), + * and otherwise the row-scoping predicate on `input.options.where`: the + * shape #5273 pinned for update and the one measured on `beforeDelete`. + */ const resolveTargetIds = async ( + op: GuardedOp, id: unknown, - data: Record, options: { where?: unknown } | undefined, + data?: Record, ): Promise> => { - const single = toId(id) ?? toId(data.id); + const single = toId(id) ?? toId(data?.id); if (single) return new Set([single]); - // Predicate / multi update: `input.id` is unbound and the row-scoping - // predicate rides on `input.options.where` (#5273 pinned that shape). const where = options?.where as EngineQueryOptions['where']; - const rows = await scan(SystemObjectName.USER, { + const rows = await scan(op, SystemObjectName.USER, { ...(where !== undefined ? { where } : {}), fields: ['id'], }); @@ -287,77 +365,113 @@ export function registerLastAdminBanGuard( return out; }; - const guardBan = async (rawCtx: unknown): Promise => { - const ctx = (rawCtx ?? {}) as { - object?: string; - input?: { id?: unknown; data?: Record; options?: { where?: unknown } }; - }; - if (ctx.object !== SystemObjectName.USER) return; - - const data = (ctx.input?.data ?? {}) as Record; - // Only a write that TURNS the ban on is interesting. An unban, an - // unrelated profile write, or a payload the ADR-0092 strip already emptied - // of `banned` can never reduce the administrator population. - if (!('banned' in data) || !isTrueFlag(data.banned)) return; - + /** + * The verdict, shared by both halves: refuse when this write takes away every + * administrator who can still sign in. Fail-closed — any lookup that throws + * becomes a refusal naming the reason. + */ + const enforce = async ( + op: GuardedOp, + input: + | { id?: unknown; data?: Record; options?: { where?: unknown } } + | undefined, + ): Promise => { + const words = OP_WORDS[op]; try { - const admins = await resolveAdminUserIds(); + const admins = await resolveAdminUserIds(op); // Nothing recognised as an administrator: there is no break-glass account - // to protect and refusing every ban would be a guard inventing a policy + // to protect and refusing every write would be a guard inventing a policy // out of an empty measurement. (A deployment reaches this only before the // first admin is bootstrapped.) if (admins.size === 0) return; - const unbanned = await resolveUnbannedAdmins(admins); - const targets = await resolveTargetIds(ctx.input?.id, data, ctx.input?.options); + const unbanned = await resolveUnbannedAdmins(op, admins); + const targets = await resolveTargetIds(op, input?.id, input?.options, input?.data); const losing = [...unbanned].filter((id) => targets.has(id)); // No administrator that could still sign in is affected → not our case. - // This is also what makes re-banning an already-banned admin a no-op - // rather than a refusal: nothing is being taken away. + // This is also what makes re-banning — or removing — an already-banned + // admin a no-op rather than a refusal: nothing is being taken away. if (losing.length === 0) return; const remaining = [...unbanned].filter((id) => !targets.has(id)); if (remaining.length > 0) return; logger?.warn( - `[LastAdminBanGuard] refused a ban that would have left this environment with no ` + + `[LastAdminGuard] refused a ${words.noun} that would have left this environment with no ` + `unbanned administrator (target: ${losing.join(', ')})`, ); const many = losing.length > 1; throw refuse( - `Refusing to ban ${losing.map((id) => `'${id}'`).join(', ')}: ` + + `Refusing to ${words.verb} ${losing.map((id) => `'${id}'`).join(', ')}: ` + `${many ? 'those are the last administrators' : 'that is the last administrator'} this ` + - `environment has that ${many ? 'are' : 'is'} not already banned, and banning ` + + `environment has that ${many ? 'are' : 'is'} not already banned, and ${words.gerund} ` + `${many ? 'them' : 'that account'} would leave nobody able to administer the ` + `environment or restore anyone's access (${BREAK_GLASS_CITATION}). Grant another user ` + `the '${ADMIN_FULL_ACCESS}' permission set or an organization ` + `'${MEMBERSHIP_ROLE_OWNER}'/'${MEMBERSHIP_ROLE_ADMIN}' membership first, then retry. ` + - 'If the ban came from an identity provider, the SCIM deprovision is too broad — fix the ' + - 'IdP group, not this guard.', + `If the ${words.noun} came from an identity provider, the SCIM deprovision is too ` + + 'broad — fix the IdP group, not this guard.', ); } catch (err) { if (isRefusal(err)) throw err; // Fail CLOSED: the guard could not prove another administrator survives, // and the cost of guessing wrong is a permanently locked-out environment. const reason = (err as Error)?.message ?? String(err); - logger?.warn(`[LastAdminBanGuard] administrator lookup failed — ban refused: ${reason}`); + logger?.warn( + `[LastAdminGuard] administrator lookup failed — ${words.noun} refused: ${reason}`, + ); throw refuse( - 'Refusing this ban: the remaining administrators could not be verified ' + - `(${reason}). This guard fails closed — a ban is only permitted when at least one other ` + - `unbanned administrator is provably left (${BREAK_GLASS_CITATION}). Retry once the ` + - 'identity tables are readable again.', + `Refusing this ${words.noun}: the remaining administrators could not be verified ` + + `(${reason}). This guard fails closed — a ${words.noun} is only permitted when at ` + + `least one other unbanned administrator is provably left (${BREAK_GLASS_CITATION}). ` + + 'Retry once the identity tables are readable again.', ); } }; - // Priority 20: AFTER the ADR-0092 identity write guard's strip (10), before + const guardBan = async (rawCtx: unknown): Promise => { + const ctx = (rawCtx ?? {}) as { + object?: string; + input?: { id?: unknown; data?: Record; options?: { where?: unknown } }; + }; + if (ctx.object !== SystemObjectName.USER) return; + + const data = (ctx.input?.data ?? {}) as Record; + // Only a write that TURNS the ban on is interesting. An unban, an + // unrelated profile write, or a payload the ADR-0092 strip already emptied + // of `banned` can never reduce the administrator population. + if (!('banned' in data) || !isTrueFlag(data.banned)) return; + + await enforce('ban', { ...ctx.input, data }); + }; + + const guardDelete = async (rawCtx: unknown): Promise => { + const ctx = (rawCtx ?? {}) as { + object?: string; + input?: { id?: unknown; options?: { where?: unknown } }; + }; + if (ctx.object !== SystemObjectName.USER) return; + + // Unlike a ban there is no payload to pre-filter on: EVERY delete of a + // `sys_user` row removes whatever standing that row had, so every one of + // them is judged. The population reads are a handful of small indexed + // queries, and deleting a user is a rare, deliberate operation. + await enforce('delete', ctx.input); + }; + + // Priority 20: AFTER the ADR-0092 identity write guard's checks (10), before // default-priority hooks (100) spend work on a write this may refuse. engine.registerHook('beforeUpdate', guardBan, { object: SystemObjectName.USER, priority: 20, packageId, }); + engine.registerHook('beforeDelete', guardDelete, { + object: SystemObjectName.USER, + priority: 20, + packageId, + }); - logger?.info('[LastAdminBanGuard] last-administrator ban guard registered (ADR-0024 D5.2)'); + logger?.info('[LastAdminGuard] last-administrator ban + delete guard registered (ADR-0024 D5.2)'); } From ddbb76ccc62ae2dd3ff34965ae1388f72d3fe318 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 14:08:36 +0000 Subject: [PATCH 2/2] =?UTF-8?q?docs(plugin-auth):=20=E7=B2=BE=E7=A1=AE?= =?UTF-8?q?=E5=8C=96=E5=AE=88=E5=8D=AB=E8=AF=BB=E5=8F=96=20`input.options.?= =?UTF-8?q?where`=20=E4=B8=8E=20HookContext=20=E5=A5=91=E7=BA=A6=E8=A1=A8?= =?UTF-8?q?=E7=9A=84=E5=85=B3=E7=B3=BB?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #5964 刚把 `HookEvent` 的枚举注释对齐到契约表的「谓词不在 `input` 上」。 两句都对,但要分清:钩子拿不到的是 composed `ast`(生效谓词);`before*` 期间 `input.options` 仍是调用方那只 engine options 包(engine.ts 5516→5517 / 6137→6152 之后才重建成 DriverOptions),`where` 与 `multi` 都在 —— 守卫读的 正是它。中间件只收窄不放宽,所以把调用方谓词当目标集是上界近似,对 fail-closed 守卫恰是安全方向。契约表那两处 `before` 行的措辞另记为 #5997。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- .../plugin-auth/src/last-admin-guard.ts | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index ca709d728f..09d402284c 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -84,9 +84,17 @@ * - **by-id** (`delete(obj, { where: { id } })` — what better-auth's adapter * emits, and what every cascade recursion re-enters with): `input.id` * carries the scalar id. - * - **predicate / `multi`**: `input.id` is unbound and the row-scoping - * predicate rides on `input.options.where` — the same shape #5273 pinned - * for update. + * - **predicate / `multi`**: `input.id` is present-but-undefined, and the + * CALLER's own options bag is still on `input.options`, predicate included + * — `delete()` only rebuilds that slot into `DriverOptions` *after* the + * `before*` hooks return. That is the same slot the ban half reads, and it + * does not contradict the `HookContextSchema.input` contract table + * (#5273 / #5899): what is unreachable from `input` is the composed + * `ast` — the *effective* predicate, onto which the filters middleware may + * add RLS / sharing scoping. Middleware can only NARROW it, so treating the + * caller's predicate as the doomed set over-approximates it, which is the + * fail-closed direction: this guard may refuse a delete that would have + * removed fewer rows, and can never miss one that removes more. * - `ctx.previous` (the engine's #5272 pre-image, and objectql's * `sys_fetch_previous_delete` builtin — `object: '*'`, priority 5) is bound * for the by-id shape ONLY; a batch dispatch names no single row, so it @@ -341,8 +349,10 @@ export function registerLastAdminGuard( * Which `sys_user` rows this one write addresses — the same answer for both * halves. A scalar id when the engine dispatched by id (an update payload * also carries it in `data.id`; a delete's `input` has no `data` at all), - * and otherwise the row-scoping predicate on `input.options.where`: the - * shape #5273 pinned for update and the one measured on `beforeDelete`. + * and otherwise the caller's predicate, still on `input.options.where` while + * `before*` runs (see the header: the composed `ast` is the part hooks + * cannot read, and middleware may only narrow it — so this set is an + * over-approximation, the safe direction). */ const resolveTargetIds = async ( op: GuardedOp,