diff --git a/.changeset/break-glass-standing-revocation-guard.md b/.changeset/break-glass-standing-revocation-guard.md new file mode 100644 index 0000000000..c36b6ffa42 --- /dev/null +++ b/.changeset/break-glass-standing-revocation-guard.md @@ -0,0 +1,41 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): break-glass 不变量补上第三条路径 —— 撤销「管理员身份」的写(`sys_member` 降级/删行、`admin_full_access` 授权删/改)同样被拒 (#5978) + +cloud ADR-0024 D5.2 的不变量是「环境永远至少留一个能登录的管理员」。此前它由两个引擎钩子守着, +**都装在 `sys_user` 上**:`banned = true`(#5892 / PR #5939)与删 `sys_user` 行(#5941 / PR #5993)。 + +但「谁是管理员」这件事根本不存在 `sys_user` 上 —— 它由另外两张表推导(`resolveAdminUserIds` +正是从这两张表反向枚举的)。于是第三条写法完全绕开两个守卫:**用户行原封不动,把他的管理员身份拿掉**。 + +- 把最后一个管理员的 `sys_member.role` 降到 admin 等级之下(better-auth 的 `updateMemberRole`、 + 一次 SCIM 组映射变更、导入、脚本),或直接删掉那条 `sys_member` 行; +- 删掉那条 `admin_full_access` 的 `sys_user_permission_set` 授权,或把它改到不再生效 + —— 改指向别的权限集、加上 `organization_id` 组织作用域、把 ADR-0091 有效期窗口改过去。 + +三者事后状态与「删掉最后一个管理员」完全等价:所有人都还在,没有任何人能管理任何东西, +产品内部无恢复路径。 + +**新增的拒写语义。** 守卫现在按同一形状扩到 `sys_member` 与 `sys_user_permission_set` 的 +`beforeUpdate` / `beforeDelete`(共六个钩子,同 `packageId`、同 priority 20)。判据就是 issue 的原话 +——**枚举、模拟、再枚举**:先枚举当前管理员,再把这次写落地后的行拿同一个枚举函数跑一遍, +若第二次为空而第一次不为空则拒写。两次枚举是同一份实现,「谁是管理员」不可能对写前问题和写后问题 +给出两个答案。 + +- **全覆盖,不是只拦自降级**:真正会发生的是 IdP 组映射改别人的角色,不是管理员给自己降级。 +- **谓词/批量写照判**:一次 `where` 命中多行的 update/delete 会先解析出整个匹配行集再做写后模拟, + 而不是一律拒绝;只有匹配集本身解析不出来(读失败,或超过 `maxScan`)才响亮拒写。 +- **fail-closed**:枚举失败或形状不确定一律拒写并点名 ADR-0024 D5.2,与既有两半同向。 +- 模拟是**单向**的 —— 只会拿走身份,不会授予身份(把 role 从 `member` 升到 `admin`、把授权改指向 + `admin_full_access` 这类写,模拟看不见新增的管理员),因此每一处取整都倒向「拒写」而非「放行」。 + +**不拦的**:降级到**另一个** admin 等级(`owner` → `admin`,或逗号拼写 `member,admin`)—— 等级未失; +已被 ban 的管理员的身份被撤(本来就不能登录,没有东西被拿走);非管理员的 membership/授权; +以及不触及 `role` / `user_id`(membership)或权限集/作用域/有效期(授权)的 payload —— 这类写 +静态可证不改变枚举结果,一次读都不做。 + +有效期语义按 `resolveAdminUserIds` 现有的 `isGrantActive`(ADR-0091 D2)**原样消费**,本次不新造 +(#5893 才是那个问题的归属单)。等级判定全程只问 `isOrgAdminGrade` 这把唯一的尺(#5939 / #5942), +守卫内没有任何手抄的 role 解析。 diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 865c24563c..8fa771c908 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -987,17 +987,19 @@ export class AuthPlugin implements Plugin { getSecondaryStorage: () => this.effectiveSecondaryStorage as SecondaryStorageLike | undefined, }); - // [cloud ADR-0024 D5.2] Break-glass — the SAME `sys_user` write + // [cloud ADR-0024 D5.2] Break-glass — the same identity write // chokepoints, guarding a different question: not "may this caller // write identity tables" (above, and system writes bypass it by // 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. + // or a row DELETE (#5941) on `sys_user`, and — since #5978 — any write + // to `sys_member` / `sys_user_permission_set` that revokes the last + // administrator's STANDING while leaving their user row untouched, is + // refused for EVERY context, `isSystem` included: the paths that + // actually lock an org out are the system ones (better-auth's admin + // ban, remove-user and updateMemberRole, driven by a SCIM + // `active: false` / `DELETE /Users/{id}` / group remap). All six hooks + // register 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, diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts index d5290e6eb8..8771df9f94 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts @@ -52,8 +52,9 @@ import { registerLastAdminGuard, type LastAdminGuardEngine } from './last-admin- import { registerIdentityWriteGuard, registerManagedUpdateWhitelist } from './identity-write-guard.js'; import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js'; import { createObjectQLAdapterFactory } from './objectql-adapter.js'; -import { buildAdminPluginSchema } from './auth-schema-config.js'; +import { buildAdminPluginSchema, buildOrganizationPluginSchema } from './auth-schema-config.js'; import { admin } from 'better-auth/plugins/admin'; +import { organization } from 'better-auth/plugins/organization'; // --------------------------------------------------------------------------- // Fixtures @@ -650,17 +651,30 @@ describe('[#5941] break-glass: the last unbanned administrator cannot be DELETED expect(await userExists(engine, 'usr_owner')).toBe(false); }); - it('deleting a row on another object is not this guard\'s business', async () => { + it('deleting an unrelated row on another object is not this guard\'s business', async () => { await seedUser(engine, 'usr_owner', { role: 'owner' }); + // `usr_member`'s membership carries no administrative grade, so removing it + // takes no standing away — the standing halves (#5978) below judge every + // `sys_member` delete, and this is what "judged and allowed" looks like. + await seedUser(engine, 'usr_member', { role: 'member' }); - // 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 }), + engine.delete('sys_member', { where: { id: 'mem_usr_member' }, ...SYSTEM }), + ).resolves.toBeDefined(); + // …and a table this guard reads but does not write-guard is untouched. + await expect( + engine.delete('sys_account', { where: { id: 'nope' }, ...SYSTEM }), ).resolves.toBeDefined(); }); + + // NOTE (#5978): the case that used to live here asserted the OPPOSITE — that + // `engine.delete('sys_member', { where: { id: 'mem_usr_owner' } })` resolves, + // pinning the third-path gap #5941 deliberately left open ("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"). + // Today is that day: the same write is now refused, and that inversion is + // the before-red anchor for this whole change — see + // `[#5978] path 2` below, which is the same fixture with the verdict flipped. }); describe('[#5941] the delete guard holds on predicate (multi) deletes, not only by-id', () => { @@ -864,3 +878,663 @@ describe('[#5892 / #5941] reverse verification: without the guard, the lockout g expect(await userExists(engine, 'usr_escape')).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// [#5978] The THIRD write shape — the `sys_user` row is never touched +// +// "Who is an administrator" is not stored on `sys_user`. It is derived from the +// two tables `resolveAdminUserIds` enumerates, so it can be taken away by +// writing THEM while every user row stays exactly as it was. The two halves +// #5892 / #5941 installed both filter on `object === 'sys_user'`, so they see +// none of it. +// +// Each path below pins the same five things the invariant needs: +// (1) the last administrator's standing cannot be revoked, +// (2) a non-last administrator's can, +// (3) a predicate/bulk write is judged over its whole matched set, +// (4) an unverifiable population refuses (fail CLOSED), +// (5) the path IS the third path — `sys_user` is untouched by the refused +// write, which is what makes it invisible to the first two halves. +// --------------------------------------------------------------------------- + +/** The `sys_member.role` a membership row currently carries. */ +async function memberRole(engine: ObjectQL, memberId: string): Promise { + const row = await engine.findOne('sys_member', { where: { id: memberId } }, SYSTEM); + return row?.role; +} + +async function rowExists(engine: ObjectQL, object: string, id: string): Promise { + const row = await engine.findOne(object, { where: { id }, fields: ['id'] }, SYSTEM); + return Boolean(row); +} + +/** + * The assertion that makes these the THIRD path rather than a restatement of + * the first two: the user row is present, unbanned, and was never a party to + * the write that got refused. + */ +async function expectUserRowUntouched(engine: ObjectQL, id: string): Promise { + expect(await userExists(engine, id)).toBe(true); + expect(await bannedFlag(engine, id)).toBeFalsy(); +} + +describe('[#5978] path 1 — downgrading the last administrator\'s sys_member role', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + }); + + /** What better-auth's `updateMemberRole` (and a SCIM group remap) writes. */ + const setRole = (memberId: string, role: string) => + engine.update('sys_member', { id: memberId, role }, SYSTEM); + + it('two org admins: downgrading the first is allowed, downgrading the last is refused', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + + await expect(setRole('mem_usr_admin', 'member')).resolves.toBeTruthy(); + expect(await memberRole(engine, 'mem_usr_admin')).toBe('member'); + + await expect(setRole('mem_usr_owner', 'member')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + object: 'sys_member', + }); + // Nothing was written: the standing survives the refusal. + expect(await memberRole(engine, 'mem_usr_owner')).toBe('owner'); + }); + + it('THE PATH ITSELF: the sys_user row is never touched, which is why #5892/#5941 miss it', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner', accountProvider: 'oidc' }); + await seedUser(engine, 'usr_escape', { role: 'member', accountProvider: 'credential' }); + + // No `banned` write, no `sys_user` delete — the two guarded chokepoints are + // not on this path at all. The write lands on `sys_member`, and it is still + // refused. + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow(/ADR-0024 D5\.2/); + await expectUserRowUntouched(engine, 'usr_owner'); + expect(await memberRole(engine, 'mem_usr_owner')).toBe('owner'); + }); + + it('the refusal explains itself: whose standing, which table, why, and the fix', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow( + /Refusing this membership change/, + ); + // The user who LOSES standing is named, not the membership row id — the + // operator needs to know which person is about to be locked out. + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow(/'usr_owner'/); + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow(/last administrator/i); + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow(/sys_member/); + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow(/ADR-0024 D5\.2/); + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow(new RegExp(ADMIN_FULL_ACCESS)); + // An IdP drove most of these, so the message points at the group mapping. + await expect(setRole('mem_usr_owner', 'member')).rejects.toThrow(/SCIM group mapping/); + }); + + it('a downgrade to ANOTHER administrative grade is allowed — the grade is not lost', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + // `owner` → `admin` is a demotion in the ladder, but both grades administer + // the org, so the environment keeps an administrator and the guard has no + // opinion. Nothing here is a role-governance policy. + await expect(setRole('mem_usr_owner', 'admin')).resolves.toBeTruthy(); + expect(await memberRole(engine, 'mem_usr_owner')).toBe('admin'); + // …and the reverse, back up the ladder, likewise. + await expect(setRole('mem_usr_owner', 'owner')).resolves.toBeTruthy(); + }); + + it('a comma-joined downgrade that KEEPS an administrative role is allowed', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + // The one grade ruler (`isOrgAdminGrade`, #5939/#5942) reads the whole + // comma-joined value, so `member,admin` still administers. A hand-copied + // `role === 'owner' || role === 'admin'` in the simulation would have + // refused this legal write. + await expect(setRole('mem_usr_owner', 'member,admin')).resolves.toBeTruthy(); + }); + + it('a downgrade to `delegated_admin` IS refused (ADR-0105 D8: reach, not authority)', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + await expect(setRole('mem_usr_owner', 'delegated_admin')).rejects.toThrow( + /last administrator/i, + ); + }); + + it('a payload that touches neither `role` nor `user_id` is never guarded', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + // The invariant is scoped to the ENVIRONMENT, so moving the last admin's + // membership to another organization cannot reduce the administrator + // population — and the guard proves that statically (MEMBER_STANDING_KEYS) + // rather than by running four reads on every membership write. + await expect( + engine.update('sys_member', { id: 'mem_usr_owner', organization_id: 'org_2' }, SYSTEM), + ).resolves.toBeTruthy(); + }); + + it('a platform admin elsewhere keeps the downgrade legal', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + // The survivor need not be an org admin: an unscoped `admin_full_access` + // grant is the other half of the same enumeration. + await expect(setRole('mem_usr_owner', 'member')).resolves.toBeTruthy(); + }); + + it('re-homing the membership onto a BANNED user is refused', async () => { + // The patch keeps the `owner` grade but moves it to someone who cannot sign + // in — set arithmetic on the doomed row would call this harmless. Only a + // real write-after simulation catches it: the after-set is `{usr_banned}`, + // and `resolveUnbannedAdmins` empties it. + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_banned', { banned: true }); + + await expect( + engine.update('sys_member', { id: 'mem_usr_owner', user_id: 'usr_banned' }, SYSTEM), + ).rejects.toThrow(/last administrator/i); + }); + + it('re-homing the membership onto an UNBANNED user is allowed', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_next'); + + await expect( + engine.update('sys_member', { id: 'mem_usr_owner', user_id: 'usr_next' }, SYSTEM), + ).resolves.toBeTruthy(); + }); +}); + +describe('[#5978] path 2 — deleting the last administrator\'s sys_member row', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + }); + + const removeMembership = (memberId: string) => + engine.delete('sys_member', { where: { id: memberId }, ...SYSTEM }); + + it('two org admins: removing the first is allowed, removing the last is refused', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + + await expect(removeMembership('mem_usr_admin')).resolves.toBeDefined(); + expect(await rowExists(engine, 'sys_member', 'mem_usr_admin')).toBe(false); + + await expect(removeMembership('mem_usr_owner')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + object: 'sys_member', + }); + expect(await rowExists(engine, 'sys_member', 'mem_usr_owner')).toBe(true); + }); + + it( + 'THE INVERTED PIN: the exact write #5941 recorded as "not this guard\'s business" ' + + 'is now refused, and the sys_user row is still untouched', + async () => { + // Byte-for-byte the fixture that used to assert `.resolves.toBeDefined()` + // a few describes up — same seed, same call, opposite verdict. This is + // the before-red anchor for the whole change. + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_owner' }, ...SYSTEM }), + ).rejects.toThrow(/last administrator/i); + await expectUserRowUntouched(engine, 'usr_owner'); + }, + ); + + it('the refusal names the removal, not a ban or a user delete', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + await expect(removeMembership('mem_usr_owner')).rejects.toThrow( + /Refusing this membership removal/, + ); + await expect(removeMembership('mem_usr_owner')).rejects.toThrow(/removing it/); + await expect(removeMembership('mem_usr_owner')).rejects.toThrow(/ADR-0024 D5\.2/); + }); + + it('removing a non-administrative membership is allowed even with exactly one admin', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_member', { role: 'member' }); + await seedUser(engine, 'usr_delegate', { role: 'delegated_admin' }); + + await expect(removeMembership('mem_usr_member')).resolves.toBeDefined(); + await expect(removeMembership('mem_usr_delegate')).resolves.toBeDefined(); + }); + + it('removing the membership of an ALREADY-banned admin is allowed (nothing is taken away)', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner', banned: true }); + + await expect(removeMembership('mem_usr_owner')).resolves.toBeDefined(); + }); +}); + +describe('[#5978] path 3 — revoking the last administrator\'s admin_full_access grant', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + }); + + const revoke = (grantId: string) => + engine.delete('sys_user_permission_set', { where: { id: grantId }, ...SYSTEM }); + + const editGrant = (grantId: string, patch: Record) => + engine.update('sys_user_permission_set', { id: grantId, ...patch }, SYSTEM); + + it('two platform admins: revoking the first is allowed, revoking the last is refused', async () => { + await seedUser(engine, 'usr_p1', { platformAdmin: true }); + await seedUser(engine, 'usr_p2', { platformAdmin: true }); + + await expect(revoke('ups_usr_p1')).resolves.toBeDefined(); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_p1')).toBe(false); + + await expect(revoke('ups_usr_p2')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + object: 'sys_user_permission_set', + }); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_p2')).toBe(true); + }); + + it('THE PATH ITSELF: the sys_user row is never touched', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true, accountProvider: 'oidc' }); + + await expect(revoke('ups_usr_platform')).rejects.toThrow(/Refusing this grant removal/); + await expectUserRowUntouched(engine, 'usr_platform'); + }); + + it('ORG-SCOPING the last grant is refused — a tenant admin is not a break-glass admin', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + // The row survives and still points at `admin_full_access`; it just stops + // being the UNSCOPED grant `resolveAuthzContext` derives `platform_admin` + // from. Same end state, no delete anywhere. + await expect(editGrant('ups_usr_platform', { organization_id: ORG })).rejects.toThrow( + /Refusing this grant change/, + ); + const row = await engine.findOne( + 'sys_user_permission_set', + { where: { id: 'ups_usr_platform' } }, + SYSTEM, + ); + expect(row?.organization_id).toBeFalsy(); + }); + + it('EXPIRING the last grant is refused (ADR-0091 window, consumed as-is)', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + const past = new Date(Date.now() - 86_400_000).toISOString(); + + await expect(editGrant('ups_usr_platform', { valid_until: past })).rejects.toThrow( + /last administrator/i, + ); + }); + + it('back-DATING `valid_from` past now is refused too (the other half of the window)', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + const future = new Date(Date.now() + 86_400_000).toISOString(); + + await expect(editGrant('ups_usr_platform', { valid_from: future })).rejects.toThrow( + /last administrator/i, + ); + }); + + it('RE-POINTING the last grant at another permission set is refused', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + // The row is neither deleted nor scoped nor expired — it simply stops + // granting `admin_full_access`. The simulation re-tests which set the grant + // points at rather than trusting the enumeration's own `where`. + await expect(editGrant('ups_usr_platform', { permission_set_id: 'ps_member' })).rejects.toThrow( + /last administrator/i, + ); + }); + + it('EXTENDING the window, or editing a grant while another admin exists, is allowed', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + const future = new Date(Date.now() + 86_400_000).toISOString(); + + // A standing key is touched, so the guard does run — and allows it. + await expect(editGrant('ups_usr_platform', { valid_until: future })).resolves.toBeTruthy(); + + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await expect(revoke('ups_usr_platform')).resolves.toBeDefined(); + }); + + it('revoking an ALREADY-org-scoped grant is untouched — it never conferred standing', async () => { + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_scoped', { grant: { organization_id: ORG } }); + + await expect(revoke('ups_usr_scoped')).resolves.toBeDefined(); + }); + + it('revoking an ALREADY-expired grant is untouched', async () => { + const past = new Date(Date.now() - 86_400_000).toISOString(); + await seedUser(engine, 'usr_expired', { grant: { valid_until: past } }); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + await expect(revoke('ups_usr_expired')).resolves.toBeDefined(); + }); + + it('the non-loginable `usr_system` grant is never counted as the survivor', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + await seedUser(engine, SystemUserId.SYSTEM, { platformAdmin: true }); + + await expect(revoke('ups_usr_platform')).rejects.toThrow(/last administrator/i); + }); +}); + +// --------------------------------------------------------------------------- +// [#5978] Predicate / bulk writes on the standing tables +// --------------------------------------------------------------------------- + +describe('[#5978] the standing halves hold on predicate (multi) writes, 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 downgrade that would sweep every administrative membership is refused', async () => { + // One `where`, every administrator: `input.id` is unbound on this dispatch, + // so the guard resolves the matched set itself and simulates the payload + // over all of it. + await expect( + engine.update( + 'sys_member', + { role: 'member' }, + { multi: true, where: { role: { $ne: 'member' } }, ...SYSTEM }, + ), + ).rejects.toThrow(/last administrators/i); + expect(await memberRole(engine, 'mem_usr_owner')).toBe('owner'); + expect(await memberRole(engine, 'mem_usr_admin')).toBe('admin'); + }); + + it('an unpredicated `multi` membership delete — the one that empties the table — is refused', async () => { + await expect(engine.delete('sys_member', { multi: true, ...SYSTEM })).rejects.toThrow( + /last administrators/i, + ); + expect(await rowExists(engine, 'sys_member', 'mem_usr_owner')).toBe(true); + }); + + it('an `$in` predicate naming both administrative memberships is refused', async () => { + await expect( + engine.delete('sys_member', { + multi: true, + where: { id: { $in: ['mem_usr_owner', 'mem_usr_admin'] } }, + ...SYSTEM, + }), + ).rejects.toThrow(/last administrators/i); + }); + + it('a predicate that spares one administrator proceeds', async () => { + await expect( + engine.update( + 'sys_member', + { role: 'member' }, + { multi: true, where: { id: { $in: ['mem_usr_admin', 'mem_usr_member'] } }, ...SYSTEM }, + ), + ).resolves.toBeDefined(); + expect(await memberRole(engine, 'mem_usr_admin')).toBe('member'); + expect(await memberRole(engine, 'mem_usr_owner')).toBe('owner'); + }); + + it('a predicate revoke that would sweep every admin_full_access grant is refused', async () => { + const grantEngine = await boot(); + await seedAdminPermissionSet(grantEngine); + await seedUser(grantEngine, 'usr_p1', { platformAdmin: true }); + await seedUser(grantEngine, 'usr_p2', { platformAdmin: true }); + + await expect( + grantEngine.delete('sys_user_permission_set', { + multi: true, + where: { permission_set_id: PS_ADMIN }, + ...SYSTEM, + }), + ).rejects.toThrow(/last administrators/i); + expect(await rowExists(grantEngine, 'sys_user_permission_set', 'ups_usr_p1')).toBe(true); + expect(await rowExists(grantEngine, 'sys_user_permission_set', 'ups_usr_p2')).toBe(true); + }); + + it('a predicate grant EDIT that would expire every admin grant at once is refused', async () => { + const grantEngine = await boot(); + await seedAdminPermissionSet(grantEngine); + await seedUser(grantEngine, 'usr_p1', { platformAdmin: true }); + await seedUser(grantEngine, 'usr_p2', { platformAdmin: true }); + const past = new Date(Date.now() - 86_400_000).toISOString(); + + await expect( + grantEngine.update( + 'sys_user_permission_set', + { valid_until: past }, + { multi: true, where: { permission_set_id: PS_ADMIN }, ...SYSTEM }, + ), + ).rejects.toThrow(/last administrators/i); + }); +}); + +// --------------------------------------------------------------------------- +// [#5978] Fail-closed, on the standing halves too +// --------------------------------------------------------------------------- + +describe('[#5978] the standing halves fail CLOSED', () => { + it('a failing identity read refuses the membership removal 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 removal WOULD be legal. It is refused anyway. + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_admin' }, ...SYSTEM }), + ).rejects.toThrow(/Refusing this membership removal/); + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_admin' }, ...SYSTEM }), + ).rejects.toThrow(/could not be verified/i); + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_admin' }, ...SYSTEM }), + ).rejects.toThrow(/sys_member is unreadable/); + expect(await rowExists(engine, 'sys_member', 'mem_usr_admin')).toBe(true); + }); + + it('a failing identity read refuses the role downgrade too', async () => { + const engine = await boot({ + readThrough: (real) => ({ + registerHook: (event, handler, options) => real.registerHook(event, handler, options), + find: async () => { + throw new Error('identity tables are unreadable'); + }, + }), + }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + + await expect( + engine.update('sys_member', { id: 'mem_usr_admin', role: 'member' }, SYSTEM), + ).rejects.toMatchObject({ code: 'PERMISSION_DENIED', object: 'sys_member' }); + expect(await memberRole(engine, 'mem_usr_admin')).toBe('admin'); + }); + + it('a failing identity read refuses the grant revoke too', async () => { + const engine = await boot({ + readThrough: (real) => ({ + registerHook: (event, handler, options) => real.registerHook(event, handler, options), + find: async () => { + throw new Error('identity tables are unreadable'); + }, + }), + }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_p1', { platformAdmin: true }); + await seedUser(engine, 'usr_p2', { platformAdmin: true }); + + await expect( + engine.delete('sys_user_permission_set', { where: { id: 'ups_usr_p1' }, ...SYSTEM }), + ).rejects.toThrow(/Refusing this grant removal/); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_p1')).toBe(true); + }); + + it('a population larger than the guard can enumerate refuses, in the op\'s own words', async () => { + const engine = await boot({ maxScan: 1 }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + await seedUser(engine, 'usr_admin', { role: 'admin' }); + + // The advice is about the table the caller wrote — "a narrower set of + // memberships", not "of users". + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_admin' }, ...SYSTEM }), + ).rejects.toThrow(/more than 1 rows/); + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_admin' }, ...SYSTEM }), + ).rejects.toThrow(/Remove a narrower set of memberships/); + expect(await rowExists(engine, 'sys_member', 'mem_usr_admin')).toBe(true); + }); + + it('an environment with no administrator at all is not blocked (nothing to protect)', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_a', { role: 'member' }); + + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_a' }, ...SYSTEM }), + ).resolves.toBeDefined(); + }); +}); + +// --------------------------------------------------------------------------- +// [#5978] Face 2 — the better-auth `updateMemberRole` / organization path +// --------------------------------------------------------------------------- + +describe('[#5978] the updateMemberRole path: refused as a 403, not an opaque 500', () => { + let engine: ObjectQL; + let adapter: { + update: (args: { model: string; where: unknown[]; update: Record }) => Promise; + }; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner', accountProvider: 'oidc' }); + await seedUser(engine, 'usr_admin', { role: 'admin', accountProvider: 'oidc' }); + // The organization plugin has to be in the options for the same reason the + // admin plugin does in the #5892 block: `createAdapterFactory` resolves + // `member` → `sys_member` off ITS schema (`buildOrganizationPluginSchema`, + // the mapping `AuthManager` passes), so without it the write would not + // reach the guarded table at all. + adapter = (createObjectQLAdapterFactory(engine) as unknown as (o: unknown) => typeof adapter)({ + plugins: [organization({ schema: buildOrganizationPluginSchema() })], + }); + }); + + /** What better-auth's `updateMemberRole` ultimately writes. */ + const changeRole = (memberId: string, role: string) => + adapter.update({ + model: 'member', + where: [{ field: 'id', value: memberId, operator: 'eq', connector: 'AND' }], + update: { role }, + }); + + it('downgrading the second-to-last administrator succeeds', async () => { + await expect(changeRole('mem_usr_admin', 'member')).resolves.toBeTruthy(); + expect(await memberRole(engine, 'mem_usr_admin')).toBe('member'); + }); + + it('downgrading the LAST administrator is refused with a 403 APIError', async () => { + await changeRole('mem_usr_admin', 'member'); + + let caught: unknown; + try { + await changeRole('mem_usr_owner', 'member'); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + 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 memberRole(engine, 'mem_usr_owner')).toBe('owner'); + }); +}); + +// --------------------------------------------------------------------------- +// [#5978] Reverse verification — the same fixtures with the guard NOT registered +// +// Direction, decided before running: RED, the usual one. With +// `registerLastAdminGuard` not called, every case in the three path blocks +// above is "the write succeeds and the standing is gone". These three re-run +// that on the same fixtures rather than describing it. +// --------------------------------------------------------------------------- + +describe('[#5978] reverse verification: without the guard, the third path locks the env out', () => { + it('the unguarded engine downgrades the last administrator and reports success', 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( + engine.update('sys_member', { id: 'mem_usr_owner', role: 'member' }, SYSTEM), + ).resolves.toBeTruthy(); + expect(await memberRole(engine, 'mem_usr_owner')).toBe('member'); + // The issue's end state, spelled out: every user row is present and + // unbanned — which is exactly why #5892 and #5941 see nothing wrong — and + // no row in either standing table grades as an administrator any more. + expect(await userExists(engine, 'usr_owner')).toBe(true); + expect(await bannedFlag(engine, 'usr_owner')).toBeFalsy(); + const admins = await engine.find( + 'sys_member', + { where: { role: { $ne: 'member' } } }, + SYSTEM, + ); + expect(admins).toHaveLength(0); + }); + + it('the unguarded engine DELETES the last administrative membership', async () => { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_owner' }, ...SYSTEM }), + ).resolves.toBeDefined(); + expect(await rowExists(engine, 'sys_member', 'mem_usr_owner')).toBe(false); + expect(await userExists(engine, 'usr_owner')).toBe(true); + }); + + it('the unguarded engine REVOKES the last admin_full_access grant', async () => { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect( + engine.delete('sys_user_permission_set', { where: { id: 'ups_usr_platform' }, ...SYSTEM }), + ).resolves.toBeDefined(); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_platform')).toBe(false); + expect(await userExists(engine, 'usr_platform')).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index 09d402284c..7796e43e70 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -4,8 +4,8 @@ * [cloud ADR-0024 D5.2] Break-glass — a write may never leave this environment * with ZERO administrators able to sign in. * - * TWO writes can take the last administrator away, and this guard holds on - * both — they are one invariant, not two policies: + * THREE write shapes can take the last administrator away, and this guard + * holds on all of them — they are one invariant, not three policies: * * 1. **`sys_user.banned = true`** (#5892) — how every *disable* lands: the * better-auth admin plugin's ban endpoint writes it, and @@ -14,6 +14,18 @@ * 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. + * 3. **revoking the STANDING, leaving the user row untouched** (#5978) — the + * shape neither of the first two can see, because "who is an + * administrator" is not a fact stored on `sys_user` at all. It lives in the + * two tables `resolveAdminUserIds` enumerates, so it is taken away by + * writing THEM: downgrading (or deleting) the `sys_member` row that carries + * the `owner`/`admin` grade — better-auth's `updateMemberRole`, a SCIM + * group-mapping change — or deleting the `admin_full_access` + * `sys_user_permission_set` grant, or editing it until it no longer counts + * (re-pointed at another set, scoped to an organization, moved outside its + * ADR-0091 validity window). The end state is identical to (2): everyone is + * still there, nobody can administer anything, and there is no recovery + * path from inside the product. * * 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 @@ -21,11 +33,42 @@ * 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 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. + * So the invariant is enforced at the WRITE, on the chokepoints every path goes + * through — `beforeUpdate` and `beforeDelete` on `sys_user`, `sys_member` and + * `sys_user_permission_set` — 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, `updateMemberRole`, the SCIM adapter + * writes, an import, a script, and anything added later. + * + * ## How the standing halves decide (#5978) + * + * The row halves can answer by set arithmetic — "is every unbanned + * administrator inside the doomed set of `sys_user` ids?". The standing halves + * cannot: the write does not name users at all, it edits the evidence the + * administrator set is DERIVED from. So they answer the way the issue framed + * it — **enumerate, simulate, enumerate again**: + * + * 1. enumerate the administrators as the tables read now; + * 2. replay the SAME enumeration over the rows as this write would leave them + * (`applyPending`: addressed rows are dropped for a delete, or `{...row, + * ...payload}` for an update); + * 3. refuse when step 2 leaves nobody who can sign in and step 1 did not. + * + * One enumeration function serves both readings, so the before-answer and the + * after-answer are the same code and cannot drift. The simulation is + * deliberately one-directional — it can only take standing away, never grant + * it (`applyPending`'s comment says why) — which keeps every rounding error + * pointing at "refuse", not at "allow". + * + * A predicate write (`multi`, one `where` matching many memberships or grants) + * is resolved to its matching row ids first and then simulated over that whole + * set, so bulk writes get a real answer rather than a blanket refusal; when the + * match set itself cannot be resolved — the read throws, or it overflows + * `maxScan` — the write is refused loudly instead of guessed at. + * + * The standing halves judge EVERY membership downgrade, not only a caller + * downgrading themselves. Narrowing to self-downgrade would miss the case that + * actually happens: an IdP group mapping that rewrites other people's roles. * * ## What counts as an administrator * @@ -111,12 +154,12 @@ * 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. + * Scope in the other direction: this guard watches writes to the three tables + * the administrator population is derived from. It does NOT watch + * `sys_permission_set` itself — deleting or renaming the row named + * `admin_full_access` would un-make every platform admin at once, which is a + * fourth write shape on a fourth table; filed as #6084 rather than + * half-guarded from here. * * ## Relationship to the ADR-0092 identity write guard * @@ -191,16 +234,86 @@ const DEFAULT_MAX_SCAN = 1000; const SYSTEM_READ: BaseEngineOptions = { context: { isSystem: true } }; /** - * The two writes this guard judges. Carried into every message so a refusal + * The six 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. + * wrong place, and one reading it after an `updateMemberRole` would go looking + * on the wrong TABLE. + * + * The first two take the administrator away with their `sys_user` row; the four + * `standing` ops (#5978) leave the row untouched and take away what MAKES them + * an administrator. */ -type GuardedOp = 'ban' | 'delete'; +type GuardedOp = + | 'ban' + | 'delete' + | 'member-update' + | 'member-delete' + | 'grant-update' + | 'grant-delete'; + +interface OpWords { + /** Reads after "Refusing this …". */ + noun: string; + verb: string; + gerund: string; + /** Sentence-initial imperative, for the "…a narrower set of X" advice. */ + Verb: string; + /** What a narrower set would be a set OF. */ + subject: string; + /** The table this op writes — what a refusal reports as `err.object`. */ + table: string; +} -const OP_WORDS: Record = { - ban: { noun: 'ban', verb: 'ban', gerund: 'banning', Verb: 'Ban' }, - delete: { noun: 'delete', verb: 'delete', gerund: 'deleting', Verb: 'Delete' }, +const OP_WORDS: Record = { + ban: { + noun: 'ban', + verb: 'ban', + gerund: 'banning', + Verb: 'Ban', + subject: 'users', + table: SystemObjectName.USER, + }, + delete: { + noun: 'delete', + verb: 'delete', + gerund: 'deleting', + Verb: 'Delete', + subject: 'users', + table: SystemObjectName.USER, + }, + 'member-update': { + noun: 'membership change', + verb: 'change', + gerund: 'changing', + Verb: 'Change', + subject: 'memberships', + table: SystemObjectName.MEMBER, + }, + 'member-delete': { + noun: 'membership removal', + verb: 'remove', + gerund: 'removing', + Verb: 'Remove', + subject: 'memberships', + table: SystemObjectName.MEMBER, + }, + 'grant-update': { + noun: 'grant change', + verb: 'change', + gerund: 'changing', + Verb: 'Change', + subject: 'permission-set grants', + table: USER_PERMISSION_SET, + }, + 'grant-delete': { + noun: 'grant removal', + verb: 'revoke', + gerund: 'revoking', + Verb: 'Revoke', + subject: 'permission-set grants', + table: USER_PERMISSION_SET, + }, }; /** @@ -214,8 +327,14 @@ function isTrueFlag(value: unknown): boolean { return value === true || value === 1 || value === '1' || value === 'true'; } -/** The refusal. `PERMISSION_DENIED` + 403 is what `mapDataError` already maps. */ -function refuse(message: string): Error { +/** + * The refusal. `PERMISSION_DENIED` + 403 is what `mapDataError` already maps. + * `object` names the table the CALLER was writing — `sys_user` for the two + * halves that take the row away, `sys_member` / `sys_user_permission_set` for + * the standing halves (#5978) — so the error points at the write that was + * refused rather than at the table the invariant is about. + */ +function refuse(message: string, object: string = SystemObjectName.USER): Error { const err = new Error(`PERMISSION_DENIED: ${message}`) as Error & { code?: string; status?: number; @@ -223,7 +342,7 @@ function refuse(message: string): Error { }; err.code = 'PERMISSION_DENIED'; err.status = 403; - err.object = SystemObjectName.USER; + err.object = object; return err; } @@ -242,6 +361,82 @@ function toId(value: unknown): string | undefined { return undefined; } +/** + * [#5978] The write the standing halves have to judge, described the way the + * enumeration can consume it: WHICH rows of the standing table this write + * addresses, and what it does to them. + * + * `patch: undefined` means the rows are being deleted; otherwise the rows are + * updated and `patch` is the caller's payload, applied over each row. + */ +interface PendingStandingWrite { + /** `sys_member` or `sys_user_permission_set`. */ + table: string; + /** Ids of the rows this one write addresses (by-id, or the predicate's matches). */ + ids: Set; + /** The update payload, or `undefined` for a delete. */ + patch?: Record; +} + +/** + * The row as it would read AFTER `pending` lands — `undefined` when the row + * would no longer exist. Rows this write does not address come back unchanged. + * + * Deliberately one-directional: a pending write can only take standing AWAY in + * this simulation, never add it. A payload that would *promote* someone (role + * `member` → `admin`, a grant re-pointed AT `admin_full_access`) writes a row + * the enumeration's narrowing `where` never selected, so the simulation does + * not see the new administrator and under-counts the survivors. That is the + * fail-closed direction: the guard may refuse a write that would in fact have + * left an administrator behind, and can never wave through one that leaves + * none. + */ +function applyPending( + row: Record, + pending: PendingStandingWrite | undefined, + table: string, +): Record | undefined { + if (!pending || pending.table !== table) return row; + const id = toId(row.id); + if (!id || !pending.ids.has(id)) return row; + if (!pending.patch) return undefined; // deleted + return { ...row, ...pending.patch }; +} + +/** + * Which keys of a `sys_member` payload can move the administrator enumeration. + * The `sys_member` half of `resolveAdminUserIds` reads exactly two columns — + * the graded `role` and the `user_id` the standing belongs to — so a payload + * touching neither provably produces the same enumeration and is skipped + * without any reads. (`organization_id` is NOT one of them: the invariant is + * scoped to the ENVIRONMENT, so which org a membership sits in never changes + * who administers this deployment.) + */ +const MEMBER_STANDING_KEYS = ['role', 'user_id', 'userId'] as const; + +/** + * Same, for `sys_user_permission_set`: which permission set the grant points + * at, whose it is, whether it is org-scoped, and its ADR-0091 validity window + * — every column the grant half of the enumeration consumes, in both the + * snake_case and camelCase spellings the readers already tolerate. + */ +const GRANT_STANDING_KEYS = [ + 'permission_set_id', + 'permissionSetId', + 'user_id', + 'userId', + 'organization_id', + 'organizationId', + 'valid_from', + 'validFrom', + 'valid_until', + 'validUntil', +] as const; + +function touchesAny(data: Record, keys: readonly string[]): boolean { + return keys.some((k) => k in data); +} + /** * Register the last-administrator guard on an ObjectQL engine: the ban half * (`beforeUpdate`) and the delete half (`beforeDelete`) of ONE invariant, off @@ -271,15 +466,26 @@ export function registerLastAdminGuard( throw refuse( `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.', + `${words.Verb} a narrower set of ${words.subject}, or raise the guard's maxScan if ` + + 'this environment really is that large.', + words.table, ); } return list; }; - /** Every user this environment currently recognises as an administrator. */ - const resolveAdminUserIds = async (op: GuardedOp): Promise> => { + /** + * Every user this environment recognises as an administrator. + * + * With `pending` (#5978) the SAME enumeration is replayed over the rows as + * they would read once that write lands — one code path answers both "who + * administers this environment now" and "who would administer it after", so + * the two answers can never drift apart the way two separate readers would. + */ + const resolveAdminUserIds = async ( + op: GuardedOp, + pending?: PendingStandingWrite, + ): Promise> => { const ids = new Set(); const now = Date.now(); @@ -293,11 +499,26 @@ export function registerLastAdminGuard( const links = await scan(op, USER_PERMISSION_SET, { where: { permission_set_id: { $in: adminSetIds } }, }); - for (const link of links) { + for (const raw of links) { + const link = applyPending(raw, pending, USER_PERMISSION_SET); + // Revoked outright by the pending write. + if (!link) continue; + // Re-pointed away from `admin_full_access` — the row survives, the + // standing does not. Re-tested rather than assumed, because the scan's + // own `where` only proved where the grant pointed BEFORE the write. + const setId = toId(link.permission_set_id ?? link.permissionSetId); + if (setId !== undefined && !adminSetIds.includes(setId)) continue; // An org-SCOPED grant makes a tenant admin, not the environment's // break-glass admin — the same distinction `resolveAuthzContext` draws - // when it derives `platform_admin` from the unscoped grant only. + // when it derives `platform_admin` from the unscoped grant only. This + // is also the "scope it to an org" revocation shape: a pending write + // that fills `organization_id` in lands here. if (link.organization_id ?? link.organizationId) continue; + // ADR-0091's ONE validity predicate, consumed exactly as it already was + // — the guard invents no expiry semantics of its own (#5893 owns that + // question, and is blocked on #5702). A pending write that moves + // `valid_until` into the past is judged by the same predicate that + // judges a stored one. if (!isGrantActive(link, now)) continue; const uid = toId(link.user_id ?? link.userId); if (uid) ids.add(uid); @@ -314,7 +535,9 @@ export function registerLastAdminGuard( const members = await scan(op, SystemObjectName.MEMBER, { where: { role: { $ne: MEMBERSHIP_ROLE_MEMBER } }, }); - for (const m of members) { + for (const raw of members) { + const m = applyPending(raw, pending, SystemObjectName.MEMBER); + if (!m) continue; if (!isOrgAdminGrade(m.role)) continue; const uid = toId(m.user_id ?? m.userId); if (uid) ids.add(uid); @@ -346,16 +569,24 @@ 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 + * Which rows of `object` this one write addresses — the same answer for all + * six 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 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). + * + * [#5978] A predicate write on a standing table is resolved to its matching + * row ids the same way — this is what lets the simulation be exact for a + * `where` that sweeps many memberships or grants at once, rather than the + * guard having to refuse every bulk write on principle. When the resolution + * itself cannot be completed (the read throws, or the match set overflows + * `maxScan`) `scan` refuses loudly instead of guessing. */ const resolveTargetIds = async ( op: GuardedOp, + object: string, id: unknown, options: { where?: unknown } | undefined, data?: Record, @@ -363,7 +594,7 @@ export function registerLastAdminGuard( const single = toId(id) ?? toId(data?.id); if (single) return new Set([single]); const where = options?.where as EngineQueryOptions['where']; - const rows = await scan(op, SystemObjectName.USER, { + const rows = await scan(op, object, { ...(where !== undefined ? { where } : {}), fields: ['id'], }); @@ -376,9 +607,39 @@ export function registerLastAdminGuard( }; /** - * 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. + * The fail-CLOSED envelope every half runs inside. A lookup that throws is + * not "probably fine": the guard could not prove another administrator + * survives, and the cost of guessing wrong is a permanently locked-out + * environment. A deliberate refusal from inside passes through unchanged. + */ + const failClosed = async (op: GuardedOp, judge: () => Promise): Promise => { + const words = OP_WORDS[op]; + try { + await judge(); + } catch (err) { + if (isRefusal(err)) throw err; + const reason = (err as Error)?.message ?? String(err); + logger?.warn( + `[LastAdminGuard] administrator lookup failed — ${words.noun} refused: ${reason}`, + ); + throw refuse( + `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.', + words.table, + ); + } + }; + + /** The one sentence every refusal ends with: how to make the write legal. */ + const REMEDY = + `Grant another user the '${ADMIN_FULL_ACCESS}' permission set or an organization ` + + `'${MEMBERSHIP_ROLE_OWNER}'/'${MEMBERSHIP_ROLE_ADMIN}' membership first, then retry.`; + + /** + * The verdict for the two `sys_user` halves: refuse when this write takes + * away every administrator who can still sign in. */ const enforce = async ( op: GuardedOp, @@ -387,7 +648,7 @@ export function registerLastAdminGuard( | undefined, ): Promise => { const words = OP_WORDS[op]; - try { + await failClosed(op, async () => { const admins = await resolveAdminUserIds(op); // Nothing recognised as an administrator: there is no break-glass account // to protect and refusing every write would be a guard inventing a policy @@ -396,7 +657,13 @@ export function registerLastAdminGuard( if (admins.size === 0) return; const unbanned = await resolveUnbannedAdmins(op, admins); - const targets = await resolveTargetIds(op, input?.id, input?.options, input?.data); + const targets = await resolveTargetIds( + op, + SystemObjectName.USER, + 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. @@ -417,27 +684,87 @@ export function registerLastAdminGuard( `${many ? 'those are the last administrators' : 'that is the last administrator'} this ` + `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. ` + + `environment or restore anyone's access (${BREAK_GLASS_CITATION}). ${REMEDY} ` + `If the ${words.noun} came from an identity provider, the SCIM deprovision is too ` + 'broad — fix the IdP group, not this guard.', + words.table, ); - } 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); + }); + }; + + /** + * [#5978] The verdict for the four STANDING halves — the third write shape, + * where the `sys_user` row is never touched and what is taken away is the + * thing that MADE the user an administrator. + * + * The criterion is the issue's, verbatim: enumerate the administrators, then + * enumerate them AGAIN over the rows as this write would leave them, and + * refuse if the second enumeration is empty while the first was not. Both + * enumerations are the same function, so "who administers this environment" + * has one implementation and cannot answer the before-question and the + * after-question differently. + */ + const enforceStanding = async ( + op: GuardedOp, + table: string, + input: + | { id?: unknown; data?: Record; options?: { where?: unknown } } + | undefined, + patch?: Record, + ): Promise => { + const words = OP_WORDS[op]; + await failClosed(op, async () => { + const before = await resolveAdminUserIds(op); + // Same bootstrap exemption the row halves make: with nobody recognised as + // an administrator there is no break-glass account to protect. + if (before.size === 0) return; + + const unbannedBefore = await resolveUnbannedAdmins(op, before); + // Every administrator is already banned — this write cannot take away an + // ability to sign in that nobody currently has. + if (unbannedBefore.size === 0) return; + + // Which rows of the standing table this write addresses. For a predicate + // write this resolves the whole matched set, so the simulation below is + // exact for bulk writes rather than being refused wholesale. + const ids = await resolveTargetIds(op, table, input?.id, input?.options, input?.data); + if (ids.size === 0) return; + + const after = await resolveAdminUserIds(op, { table, ids, ...(patch ? { patch } : {}) }); + // A patch that re-homes standing onto a DIFFERENT user (a `user_id` + // rewrite) can put someone in `after` who was not in `before`, so the + // survivors' ban state is re-read rather than intersected with the + // before-set. + const unbannedAfter: Set = + after.size > 0 ? await resolveUnbannedAdmins(op, after) : new Set(); + + const losing = [...unbannedBefore].filter((id) => !unbannedAfter.has(id)); + // The write leaves every administrator's standing intact → not our case. + // This is the common path for the vast majority of membership and grant + // writes, and it costs no refusal and no surprise. + if (losing.length === 0) return; + // Somebody can still administer the environment afterwards. + if (unbannedAfter.size > 0) return; + logger?.warn( - `[LastAdminGuard] administrator lookup failed — ${words.noun} refused: ${reason}`, + `[LastAdminGuard] refused a ${words.noun} on '${table}' that would have left this ` + + `environment with no unbanned administrator (losing: ${losing.join(', ')})`, ); + const many = losing.length > 1; throw refuse( - `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.', + `Refusing this ${words.noun}: it would revoke the administrator standing of ` + + `${losing.map((id) => `'${id}'`).join(', ')}, ` + + `${many ? 'who are the last administrators' : 'who is the last administrator'} this ` + + `environment has that ${many ? 'are' : 'is'} not already banned. The ` + + `'${table}' row is what MAKES ${many ? 'those accounts' : 'that account'} an ` + + `administrator, so ${words.gerund} it has the same end state as ${words.gerund} ` + + `${many ? 'the users themselves' : 'the user themselves'}: nobody would be able to ` + + `administer the environment or restore anyone's access (${BREAK_GLASS_CITATION}). ` + + `${REMEDY} If the ${words.noun} came from an identity provider, the SCIM group ` + + 'mapping is too broad — fix the IdP group, not this guard.', + words.table, ); - } + }); }; const guardBan = async (rawCtx: unknown): Promise => { @@ -470,8 +797,65 @@ export function registerLastAdminGuard( await enforce('delete', ctx.input); }; + /** + * [#5978] Standing halves. `ctxOf` is the same unwrap the two row halves do; + * `object` is re-checked here as well as in the registration filter, so a + * handler can never judge a table it was not written for. + */ + const ctxOf = (rawCtx: unknown) => + (rawCtx ?? {}) as { + object?: string; + input?: { id?: unknown; data?: Record; options?: { where?: unknown } }; + }; + + const guardMemberUpdate = async (rawCtx: unknown): Promise => { + const ctx = ctxOf(rawCtx); + if (ctx.object !== SystemObjectName.MEMBER) return; + const data = (ctx.input?.data ?? {}) as Record; + // The whole downgrade family lands here: better-auth's `updateMemberRole`, + // a SCIM group-mapping change, an import, a script. It is NOT narrowed to + // "the caller downgrading themselves" — an IdP writes these on everyone's + // behalf, which is exactly the case ADR-0024 D5.2 exists for. + // + // A payload touching neither `role` nor `user_id` provably cannot move the + // enumeration (see MEMBER_STANDING_KEYS), so it costs no reads at all. + if (!touchesAny(data, MEMBER_STANDING_KEYS)) return; + await enforceStanding('member-update', SystemObjectName.MEMBER, ctx.input, data); + }; + + const guardMemberDelete = async (rawCtx: unknown): Promise => { + const ctx = ctxOf(rawCtx); + if (ctx.object !== SystemObjectName.MEMBER) return; + // No payload to pre-filter on: removing a membership removes whatever + // administrative standing it carried, so every one of them is judged. + await enforceStanding('member-delete', SystemObjectName.MEMBER, ctx.input); + }; + + const guardGrantUpdate = async (rawCtx: unknown): Promise => { + const ctx = ctxOf(rawCtx); + if (ctx.object !== USER_PERMISSION_SET) return; + const data = (ctx.input?.data ?? {}) as Record; + // The three ways a grant stops counting without being deleted: re-pointed + // at another permission set, scoped to an organization, or moved outside + // its ADR-0091 validity window. All three are just columns, so the + // simulation reads them back through the same predicates the enumeration + // already uses rather than special-casing any of them here. + if (!touchesAny(data, GRANT_STANDING_KEYS)) return; + await enforceStanding('grant-update', USER_PERMISSION_SET, ctx.input, data); + }; + + const guardGrantDelete = async (rawCtx: unknown): Promise => { + const ctx = ctxOf(rawCtx); + if (ctx.object !== USER_PERMISSION_SET) return; + await enforceStanding('grant-delete', USER_PERMISSION_SET, 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. + // default-priority hooks (100) spend work on a write this may refuse. The + // four standing hooks (#5978) are registered in exactly the shape the two + // `sys_user` hooks established — same event names, same priority, same + // `packageId`, only the `object` filter differs — so the whole invariant + // binds and unbinds as one package. engine.registerHook('beforeUpdate', guardBan, { object: SystemObjectName.USER, priority: 20, @@ -482,6 +866,29 @@ export function registerLastAdminGuard( priority: 20, packageId, }); + engine.registerHook('beforeUpdate', guardMemberUpdate, { + object: SystemObjectName.MEMBER, + priority: 20, + packageId, + }); + engine.registerHook('beforeDelete', guardMemberDelete, { + object: SystemObjectName.MEMBER, + priority: 20, + packageId, + }); + engine.registerHook('beforeUpdate', guardGrantUpdate, { + object: USER_PERMISSION_SET, + priority: 20, + packageId, + }); + engine.registerHook('beforeDelete', guardGrantDelete, { + object: USER_PERMISSION_SET, + priority: 20, + packageId, + }); - logger?.info('[LastAdminGuard] last-administrator ban + delete guard registered (ADR-0024 D5.2)'); + logger?.info( + '[LastAdminGuard] last-administrator guard registered on sys_user (ban + delete), ' + + 'sys_member and sys_user_permission_set (standing revocation) — ADR-0024 D5.2', + ); }