From 0acf821f21240dea95a80b8a085fb3d3c0718e64 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 02:06:34 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(plugin-auth):=20break-glass=20=E5=AE=88?= =?UTF-8?q?=E5=8D=AB=E6=89=A9=E5=88=B0=20sys=5Fpermission=5Fset,=E5=B9=B6?= =?UTF-8?q?=E5=88=86=E8=BE=A8=E3=80=8C=E9=9B=B6=E7=AE=A1=E7=90=86=E5=91=98?= =?UTF-8?q?=E3=80=8D=E4=B8=8E=E5=BC=95=E5=AF=BC=E6=9C=9F=20(#6084)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 第四条写法绕开 #5892/#5941/#5978 三条守卫:「谁是 platform admin」按名字解析,删掉 或改名 `admin_full_access` 那条 `sys_permission_set` 行,授权行/`sys_user`/`sys_member` 一个都没动,而所有 platform admin 同时不再是管理员。 放大缺陷:守卫的引导期豁免(零管理员 ⇒ 放行)此后把「刚被清空」读成「引导期」, 连带解除其余三条路径的守卫。 ① `sys_permission_set` 的 beforeUpdate(payload 触及 `name` 才判)+ beforeDelete, 复用 `enforceStanding`/`applyPending`;枚举第一段 scan 现在也对 pending 做模拟并 重测 `name`。 ② 「零管理员」拆成真引导期(照旧放行)与刚被清空(存在悬空的无组织范围、有效期内授权行 ⇒ fail-closed 拒写)。悬空授权行在正常路径写不出来,全新环境的可写性按构造不变。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- .../last-admin-guard-permission-set-row.md | 46 ++ .../plugins/plugin-auth/src/auth-plugin.ts | 10 +- .../plugin-auth/src/last-admin-guard.test.ts | 419 +++++++++++++++++- .../plugin-auth/src/last-admin-guard.ts | 331 ++++++++++++-- 4 files changed, 769 insertions(+), 37 deletions(-) create mode 100644 .changeset/last-admin-guard-permission-set-row.md diff --git a/.changeset/last-admin-guard-permission-set-row.md b/.changeset/last-admin-guard-permission-set-row.md new file mode 100644 index 0000000000..c71b311626 --- /dev/null +++ b/.changeset/last-admin-guard-permission-set-row.md @@ -0,0 +1,46 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): break-glass 守卫扩到 `sys_permission_set`,并把「零管理员」从引导期豁免里分辨出来 (#6084) + +break-glass 不变量(cloud ADR-0024 D5.2)此前守三张表:`sys_user`(ban/删行,#5892/#5941)与 +`sys_member`/`sys_user_permission_set`(撤销 standing,#5978)。**第四条写法绕开全部三条**: +「谁是 platform admin」是**按名字**解析的——`resolveAdminUserIds` 先 +`where: { name: 'admin_full_access' }` 取那条 `sys_permission_set` 行,再去读指向它 id 的授权行。 +删掉那一行、或把它改个名字,授权行、`sys_user` 行、`sys_member` 行**一个都没动**,而所有 +platform admin 同时不再是管理员。 + +## 放大缺陷:这一条写法还会顺手关掉守卫本身 + +两个判据都以「这个环境有管理员吗?没有就放行」开场——引导期本就没有 break-glass 账号可保护, +在那个窗口里拒绝一切身份写会是守卫拿一个空测量值自造政策。可是 `admin_full_access` 行没了的环境 +**读起来正是零管理员**,于是豁免生效,ban / 删用户 / 降级 / 撤授权**一并放行**。所以这一条写法 +不只是锁死环境,还在锁死的路上把 #5892 / #5941 / #5978 三条守卫一起解除。 + +## 两处改动 + +**① 同形状扩到第四张表。** `sys_permission_set` 的 `beforeUpdate` + `beforeDelete`,复用 #5978 的 +`enforceStanding` / `applyPending`,`PendingStandingWrite` 多认一张表;枚举的第一段 scan 现在也对 +pending 做模拟并**重测 `name`**——与 grant 半边重测 `permission_set_id` 同理,scan 自己的 `where` +只证明了写**之前**那行叫什么。静态跳过键只有 `name` 一个:枚举只读这一列,所以每一次 projection +回填、每一次 `os meta resync`、每一次 Setup 里编辑权限集(写的是 `label`/`description`/权限 JSON) +一次读都不花。数据门自己已经拒绝改名(ADR-0094),这道守卫覆盖的是不经数据门的引擎级与 +system-context 写。 + +**② 收紧引导期豁免。** 「零管理员」拆成它本来混在一起的两种状态: + +- **真引导期**——没有任何证据说这里曾经有过 platform admin。照旧放行。 +- **刚被清空**——仍存在无组织范围、有效期内的 `sys_user_permission_set` 授权行,而它指向的 + `sys_permission_set` 行已经不在了。fail-closed 拒写,并在报文里点名那些悬空授权行。 + +判据选的是**悬空授权行**,因为它在正常路径上根本写不出来:每一个生产者都先插权限集、再读回 id 写 +授权行(`bootstrapPlatformAdmin` 第 1 步 seed 权限集、第 2 步才提拔第一个用户,权限集缺席时返回 +`admin_permission_set_missing` 而不是发授权),所以**全新环境的可写性按构造不变**——测试里有一条 +「真引导期照常放行」的钉专门量这一点。改名不留下悬空授权行,这条判据看不见它;那条路径改由 ① 在 +写入处拦下,残留因此只剩一种状态:守卫尚未注册时落下的改名。曾考虑把判据放宽成「不存在 +`admin_full_access` 行 且 存在无组织范围授权行」,被否掉——它会改变「seed 顺序先写授权行」的全新 +环境的答案,而不改变全新环境的答案正是这条判据唯一不能碰的红线。 + +`sys_permission_set` 的拒绝报文结尾不走 SCIM 那句:IdP 不写这张表,写它的是元数据删除、 +`os meta` 与包卸载,报文点名的是这些门。 diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 8fa771c908..73c0ed3c00 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -997,9 +997,13 @@ export class AuthPlugin implements Plugin { // 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. + // `active: false` / `DELETE /Users/{id}` / group remap). Since #6084 it + // also covers `sys_permission_set`: deleting or renaming the + // `admin_full_access` row un-makes every platform admin at once, and + // the zero-admin bootstrap exemption then switched the guard off for + // all of the above. All eight 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 8771df9f94..fe50a80981 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts @@ -1,10 +1,19 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * [#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 + * [#5892 + #5941 + #5978 + #6084 / cloud ADR-0024 D5.2] The break-glass guard — + * every half of ONE invariant: a `banned = true` write, a `sys_user` row DELETE, + * a standing revocation on `sys_member` / `sys_user_permission_set`, and a + * delete-or-rename of the `admin_full_access` `sys_permission_set` row may each * only proceed while an administrator who can sign in is left behind. * + * #6084 adds one thing the earlier three did not need: the guard must still be + * ON afterwards. Its write shape empties the administrator population, and the + * zero-administrator BOOTSTRAP exemption then read that emptiness as "nothing to + * protect" and waved every other path through — so the last two blocks in this + * file pin the fourth write's refusal AND that a zero-administrator reading is + * no longer, by itself, a licence to write. + * * ## Why there is no fake engine here * * The guard's whole job is to read the identity tables and decide whether an @@ -92,6 +101,11 @@ const sysPermissionSet = { fields: { id: { name: 'id', type: 'text' as const, primaryKey: true }, name: { name: 'name', type: 'text' as const }, + // [#6084] Not read by the guard — that is its job here. `label` is what + // every projection pass and every Setup edit writes + // (`permissionSetRowFields`), so it is the column the "costs no reads" + // pin drives. + label: { name: 'label', type: 'text' as const }, }, }; @@ -1538,3 +1552,404 @@ describe('[#5978] reverse verification: without the guard, the third path locks expect(await userExists(engine, 'usr_platform')).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// [#6084] The FOURTH write shape — the one table that is not an identity table +// +// "Who is a platform admin" is resolved BY NAME: `resolveAdminUserIds` looks +// the permission set up as `where: { name: 'admin_full_access' }` and only then +// reads the grants pointing at its id. So the row named `admin_full_access` is +// itself part of the administrator evidence, and deleting it — or calling it +// something else — un-makes every platform admin in one write while `sys_user`, +// `sys_member` and `sys_user_permission_set` all stay exactly as they were. +// +// The block pins the same five things each earlier path did, plus the one this +// path adds: the guard must NOT go quiet on every other path afterwards. +// --------------------------------------------------------------------------- + +describe('[#6084] path 4 — deleting or renaming the admin_full_access permission-set row', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = await boot(); + await seedAdminPermissionSet(engine); + }); + + /** What a metadata delete (`retirePermissionSetRecord`) ultimately writes. */ + const deleteSet = (id: string) => + engine.delete('sys_permission_set', { where: { id }, ...SYSTEM }); + const renameSet = (id: string, name: string) => + engine.update('sys_permission_set', { id, name }, SYSTEM); + + const setName = async (id: string): Promise => { + const row = await engine.findOne('sys_permission_set', { where: { id } }, SYSTEM); + return row?.name; + }; + + it('THE REPRODUCTION from the issue: deleting the row is refused and the row survives', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(deleteSet(PS_ADMIN)).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + object: 'sys_permission_set', + }); + expect(await rowExists(engine, 'sys_permission_set', PS_ADMIN)).toBe(true); + }); + + it('RENAMING it is refused too — the row would survive, the standing would not', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(renameSet(PS_ADMIN, 'admin_full_access_old')).rejects.toThrow( + /last administrator/i, + ); + // Nothing was written: the enumeration still finds the row by its name. + expect(await setName(PS_ADMIN)).toBe(ADMIN_FULL_ACCESS); + }); + + it('THE PATH ITSELF: no identity table is touched, which is why the first three halves miss it', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true, accountProvider: 'oidc' }); + + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(/ADR-0024 D5\.2/); + // The user row is present and unbanned (#5892 / #5941 see nothing), and the + // grant row is untouched as well (#5978 sees nothing) — the write lands on + // a fourth table entirely, and is still refused. + await expectUserRowUntouched(engine, 'usr_platform'); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_platform')).toBe(true); + }); + + it('the refusal explains itself: who loses standing, which table, why, and the fix', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(/Refusing this permission-set removal/); + // The USER about to be locked out is named, not the permission-set row id. + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(/'usr_platform'/); + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(/last administrator/i); + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(/sys_permission_set/); + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(/ADR-0024 D5\.2/); + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(new RegExp(ADMIN_FULL_ACCESS)); + // …and the closing advice names the doors that actually write this table. + await expect(deleteSet(PS_ADMIN)).rejects.toThrow(/package uninstall/); + + // NOT the SCIM sentence the other standing halves end with: nothing in an + // IdP writes `sys_permission_set`, so pointing this operator at a group + // mapping would send them into a system they may not even run. + let message = ''; + try { + await deleteSet(PS_ADMIN); + } catch (e) { + message = (e as Error).message; + } + expect(message).not.toMatch(/SCIM group mapping/); + }); + + it('a rename to a DIFFERENT name is what is refused — the payload is simulated, not pattern-matched', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + // Re-writing the same name changes nothing the enumeration reads, so the + // simulation finds the administrator still there and the write proceeds. + await expect(renameSet(PS_ADMIN, ADMIN_FULL_ACCESS)).resolves.toBeTruthy(); + await expect(renameSet(PS_ADMIN, 'something_else')).rejects.toThrow(/last administrator/i); + }); + + // ── not over-tightened ──────────────────────────────────────────────────── + + it('an org admin elsewhere keeps the removal legal — the invariant is the ENVIRONMENT\'s', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + await seedUser(engine, 'usr_owner', { role: 'owner' }); + + // The platform-admin half of the enumeration goes empty and the guard has + // no opinion, because the environment still has an administrator. A rule + // that protected the platform-admin POPULATION rather than the environment + // would have refused this. + await expect(deleteSet(PS_ADMIN)).resolves.toBeDefined(); + expect(await rowExists(engine, 'sys_permission_set', PS_ADMIN)).toBe(false); + }); + + it('a payload that does not touch `name` costs no reads at all', async () => { + const quiet = await boot({ + readThrough: (real) => ({ + registerHook: (event, handler, options) => real.registerHook(event, handler, options), + find: async () => { + throw new Error('the guard must not read anything for a non-name payload'); + }, + }), + }); + await seedAdminPermissionSet(quiet); + await seedUser(quiet, 'usr_platform', { platformAdmin: true }); + + // Every read this guard makes runs inside the fail-CLOSED envelope, so a + // payload that provoked ANY read here would come back as a refusal. It + // resolving is the proof that PERMISSION_SET_STANDING_KEYS skipped it + // statically — which is the shape of every projection pass, every + // `os meta resync` and every Setup edit of a permission set. + await expect( + quiet.update('sys_permission_set', { id: PS_ADMIN, label: 'Full Access (edited)' }, SYSTEM), + ).resolves.toBeTruthy(); + }); + + it('deleting or renaming ANOTHER permission set is unaffected', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(renameSet('ps_member', 'member_default_v2')).resolves.toBeTruthy(); + await expect(deleteSet('ps_member')).resolves.toBeDefined(); + expect(await rowExists(engine, 'sys_permission_set', PS_ADMIN)).toBe(true); + }); + + it('an ALREADY-banned platform admin is not protected — nothing is being taken away', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true, banned: true }); + + await expect(deleteSet(PS_ADMIN)).resolves.toBeDefined(); + }); + + it('an ORG-SCOPED grant holder never was a break-glass admin, so the row stays removable', async () => { + await seedUser(engine, 'usr_scoped', { grant: { organization_id: ORG } }); + + await expect(deleteSet(PS_ADMIN)).resolves.toBeDefined(); + }); + + // ── predicate / bulk, and fail-closed ───────────────────────────────────── + + it('an unpredicated `multi` delete — the one that empties the table — is refused', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect(engine.delete('sys_permission_set', { multi: true, ...SYSTEM })).rejects.toThrow( + /last administrator/i, + ); + expect(await rowExists(engine, 'sys_permission_set', PS_ADMIN)).toBe(true); + }); + + it('a predicate rename that sweeps every permission set at once is refused', async () => { + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await expect( + engine.update( + 'sys_permission_set', + { name: 'retired' }, + { multi: true, where: { id: { $in: [PS_ADMIN, 'ps_member'] } }, ...SYSTEM }, + ), + ).rejects.toThrow(/last administrator/i); + expect(await setName(PS_ADMIN)).toBe(ADMIN_FULL_ACCESS); + }); + + it('fails CLOSED: an unreadable table refuses a removal that would have been legal', async () => { + const broken = await boot({ + readThrough: (real) => ({ + registerHook: (event, handler, options) => real.registerHook(event, handler, options), + find: async () => { + throw new Error('sys_permission_set is unreadable'); + }, + }), + }); + await seedAdminPermissionSet(broken); + await seedUser(broken, 'usr_platform', { platformAdmin: true }); + await seedUser(broken, 'usr_owner', { role: 'owner' }); + + // Two administrators exist — this removal WOULD be legal. Refused anyway. + await expect( + broken.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }), + ).rejects.toThrow(/Refusing this permission-set removal/); + await expect( + broken.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }), + ).rejects.toThrow(/sys_permission_set is unreadable/); + expect(await rowExists(broken, 'sys_permission_set', PS_ADMIN)).toBe(true); + }); + + it('a population larger than the guard can enumerate refuses, in the op\'s own words', async () => { + const tiny = await boot({ maxScan: 1 }); + await seedAdminPermissionSet(tiny); + await seedUser(tiny, 'usr_p1', { platformAdmin: true }); + await seedUser(tiny, 'usr_p2', { platformAdmin: true }); + + await expect( + tiny.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }), + ).rejects.toThrow(/Remove a narrower set of permission sets/); + expect(await rowExists(tiny, 'sys_permission_set', PS_ADMIN)).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// [#6084] The AMPLIFICATION — the half that matters more than the fourth hook +// +// Both verdicts open with "no administrator here, nothing to protect, proceed". +// An environment whose `admin_full_access` row is gone reads exactly that way, +// so before this change ONE write did not merely lock the environment out — it +// switched the guard off for #5892, #5941 and #5978 as well. +// +// So "zero administrators" is now split into the two states it was conflating: +// a genuinely fresh environment (permitted, unchanged) and one that was emptied +// (refused). The evidence is a DANGLING unscoped in-window grant, which no +// producer can write — every one of them inserts the permission set first and +// reads its id back — so the bootstrap window's behaviour is unchanged by +// construction, and the tests below measure that in both directions. +// --------------------------------------------------------------------------- + +describe('[#6084] a zero-administrator reading is no longer automatically the bootstrap window', () => { + /** + * The environment the fourth path leaves behind, built the only way it can + * still be reached now that the write itself is guarded: the delete lands + * while the guard is NOT registered — a pre-#6084 deployment, a migration, a + * restore, a direct database edit — and the platform then boots with the + * guard on, which is when `registerLastAdminGuard` runs for real. + */ + async function wipedEnvironment(): Promise { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true, accountProvider: 'oidc' }); + await engine.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }); + registerLastAdminGuard(engine as unknown as LastAdminGuardEngine, { + packageId: 'test.last-admin-guard', + }); + return engine; + } + + it('THE REGRESSION PIN: the emptied environment refuses the ban the exemption used to wave through', async () => { + const engine = await wipedEnvironment(); + + await expect(ban(engine, 'usr_platform')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + }); + expect(await bannedFlag(engine, 'usr_platform')).toBeFalsy(); + }); + + it('…and the user delete and the grant revoke with it — all three halves stay on', async () => { + const engine = await wipedEnvironment(); + + await expect(removeUser(engine, 'usr_platform')).rejects.toThrow(/#6084/); + await expect( + engine.delete('sys_user_permission_set', { where: { id: 'ups_usr_platform' }, ...SYSTEM }), + ).rejects.toThrow(/#6084/); + expect(await userExists(engine, 'usr_platform')).toBe(true); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_platform')).toBe(true); + }); + + it('the refusal names the evidence, the cause and the way back', async () => { + const engine = await wipedEnvironment(); + + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/recognises NO administrator/); + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/not the bootstrap window/i); + // Holder and target are both quoted, so an operator can go find the row. + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/'usr_platform'/); + await expect(ban(engine, 'usr_platform')).rejects.toThrow(new RegExp(PS_ADMIN)); + await expect(ban(engine, 'usr_platform')).rejects.toThrow(new RegExp(ADMIN_FULL_ACCESS)); + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/ADR-0024 D5\.2/); + }); + + it('after the fourth path is REFUSED, the other three guards still answer in that environment', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + // The write that would have disabled everything is refused… + await expect( + engine.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }), + ).rejects.toThrow(/last administrator/i); + // …and in the SAME environment the three earlier paths still refuse with + // the ordinary last-administrator verdict rather than the bootstrap + // exemption — which is what "the fourth write does not disarm the other + // three" means, measured instead of argued. + await expect(ban(engine, 'usr_platform')).rejects.toThrow(/last administrator/i); + await expect(removeUser(engine, 'usr_platform')).rejects.toThrow(/last administrator/i); + await expect( + engine.delete('sys_user_permission_set', { where: { id: 'ups_usr_platform' }, ...SYSTEM }), + ).rejects.toThrow(/last administrator/i); + }); + + // ── the bootstrap window itself, unchanged ──────────────────────────────── + + it('a genuinely fresh environment stays writable — every bootstrap write still lands', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_first', { role: 'member' }); + + // No administrator, and no evidence there ever was one: this IS the + // bootstrap window, and it behaves exactly as it did before #6084. + await expect( + engine.delete('sys_member', { where: { id: 'mem_usr_first' }, ...SYSTEM }), + ).resolves.toBeDefined(); + await expect(ban(engine, 'usr_first')).resolves.toBeTruthy(); + await expect(removeUser(engine, 'usr_first')).resolves.toBeDefined(); + // …including writes to the fourth table: an environment with no + // administrator to lose can still rename and retire the set row. + await expect( + engine.update('sys_permission_set', { id: PS_ADMIN, name: 'admin_full_access_v2' }, SYSTEM), + ).resolves.toBeTruthy(); + await expect( + engine.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }), + ).resolves.toBeDefined(); + }); + + it('an unscoped grant whose permission set still EXISTS is not evidence of a wipe', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + // An ordinary pre-first-admin state: somebody holds `member_default` + // unscoped, and nobody is an administrator yet. + await seedUser(engine, 'usr_a', { grant: { permission_set_id: 'ps_member' } }); + + await expect(ban(engine, 'usr_a')).resolves.toBeTruthy(); + }); + + it('an ORG-SCOPED dangling grant is not evidence — it never conferred platform standing', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_a', { + grant: { permission_set_id: 'ps_gone', organization_id: ORG }, + }); + + await expect(ban(engine, 'usr_a')).resolves.toBeTruthy(); + }); + + it('an EXPIRED dangling grant is not evidence either (ADR-0091, the same predicate)', async () => { + const engine = await boot(); + await seedAdminPermissionSet(engine); + const past = new Date(Date.now() - 86_400_000).toISOString(); + await seedUser(engine, 'usr_a', { + grant: { permission_set_id: 'ps_gone', valid_until: past }, + }); + + await expect(ban(engine, 'usr_a')).resolves.toBeTruthy(); + }); +}); + +// --------------------------------------------------------------------------- +// [#6084] Reverse verification — the same fixtures with the guard NOT registered +// +// Direction, decided before running: RED, the usual one. Without +// `registerLastAdminGuard` the fourth write succeeds and takes the whole +// platform-admin population with it, and the ban that follows succeeds too — +// the amplification, on the engine as it behaved before this change. +// +// The second half of that pair is the `wipedEnvironment()` block above: same +// wipe, guard registered afterwards, and the ban is refused. Together the two +// isolate what the bootstrap predicate contributes, which neither can do alone. +// --------------------------------------------------------------------------- + +describe('[#6084] reverse verification: one unguarded write takes the admins AND the guard', () => { + it('the unguarded engine deletes the admin_full_access row and every platform admin evaporates', async () => { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true, accountProvider: 'oidc' }); + + await expect( + engine.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }), + ).resolves.toBeDefined(); + // The issue's end state: the user row, its account and its grant are all + // exactly as they were — which is precisely why the first three shapes see + // nothing wrong — and no `admin_full_access` row is left to resolve. + expect(await userExists(engine, 'usr_platform')).toBe(true); + expect(await bannedFlag(engine, 'usr_platform')).toBeFalsy(); + expect(await rowExists(engine, 'sys_user_permission_set', 'ups_usr_platform')).toBe(true); + expect(await rowExists(engine, 'sys_permission_set', PS_ADMIN)).toBe(false); + }); + + it('THE AMPLIFICATION: on that same engine the ban of the last administrator then succeeds', async () => { + const engine = await boot({ unguarded: true }); + await seedAdminPermissionSet(engine); + await seedUser(engine, 'usr_platform', { platformAdmin: true }); + + await engine.delete('sys_permission_set', { where: { id: PS_ADMIN }, ...SYSTEM }); + await expect(ban(engine, 'usr_platform')).resolves.toBeTruthy(); + expect(await bannedFlag(engine, 'usr_platform')).toBeTruthy(); + }); +}); diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index 7796e43e70..804b382fc3 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. * - * THREE write shapes can take the last administrator away, and this guard - * holds on all of them — they are one invariant, not three policies: + * FOUR write shapes can take the last administrator away, and this guard + * holds on all of them — they are one invariant, not four policies: * * 1. **`sys_user.banned = true`** (#5892) — how every *disable* lands: the * better-auth admin plugin's ban endpoint writes it, and @@ -26,6 +26,18 @@ * 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. + * 4. **deleting — or RENAMING — the `admin_full_access` `sys_permission_set` + * row** (#6084) — the one table the enumeration reads that is not itself an + * identity table. "Who is a platform admin" is resolved by NAME: the first + * step of `resolveAdminUserIds` looks the permission set up as + * `where: { name: 'admin_full_access' }` and only then reads the grants + * pointing at its id. Remove that row, or call it something else, and every + * grant, every `sys_user` row and every `sys_member` row survives untouched + * while nobody is a platform admin any more — one write, the whole + * platform-admin population. Unlike (3) this one is not driven by an IdP at + * all: it is written by a metadata delete, an `os meta` run or a package + * uninstall, which is why it needs its own two hooks rather than a wider + * filter on the three tables above. * * 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 @@ -34,8 +46,9 @@ * recovery path from inside the product once that happens. * * 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. + * through — `beforeUpdate` and `beforeDelete` on `sys_user`, `sys_member`, + * `sys_user_permission_set` and `sys_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. @@ -45,8 +58,11 @@ * 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**: + * administrator set is DERIVED from. (#6084's two permission-set halves are + * standing halves in exactly this sense, and reuse the same three steps — the + * `sys_permission_set` row is a step further from the user than a grant is, not + * a different kind of evidence.) 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 @@ -107,6 +123,49 @@ * guard takes in `auth-manager.ts`; the two directions are chosen deliberately * and are not a drift. * + * ## The bootstrap window, and the state that only LOOKS like one (#6084) + * + * Both verdicts open by asking "does this environment recognise any + * administrator at all", and answer "no" by PERMITTING the write: before the + * first admin is bootstrapped there is no break-glass account to protect, and + * refusing every identity write in that window would be a guard inventing a + * policy out of an empty measurement. + * + * Write shape (4) turns that exemption into an AMPLIFIER, which is the half of + * #6084 that matters more than the fourth pair of hooks: an environment whose + * `admin_full_access` row is gone reads as ZERO administrators, so the exemption + * fires and every OTHER path — ban, delete, downgrade, revoke — is waved through + * as well. One write does not just lock the environment out, it disables the + * whole guard on the way. + * + * So "zero administrators" is resolved into the two states it was conflating, + * and only one of them is the bootstrap window: + * + * - **a genuinely fresh environment** — no evidence anybody was ever a platform + * admin here. Permitted, exactly as before. + * - **an environment that was EMPTIED** — an unscoped, in-window + * `sys_user_permission_set` grant still points at a `sys_permission_set` row + * that no longer exists. Refused, loudly, naming the dangling grants. + * + * The evidence is chosen so the FRESH-INSTALL answer cannot change: a dangling + * grant is unreachable on the happy path in either direction. Every writer + * inserts the permission set first and reads its id back to write the grant — + * `bootstrapPlatformAdmin` seeds the set rows in step 1 and only then promotes + * the first user in step 2, returning `admin_permission_set_missing` rather than + * granting when the set is absent — so no ordering of a fresh boot produces one. + * It is produced by exactly one thing: DELETING a permission set that grants + * already point at. + * + * A RENAME leaves no dangling grant, so this predicate cannot see it: the row is + * still there and every grant still resolves. That path is closed at the WRITE + * instead (`guardPermissionSetUpdate`), which narrows the residual to one state + * — an environment renamed away from `admin_full_access` by a write that landed + * while this guard was not registered. Widening the predicate to cover it ("no + * `admin_full_access` row exists AND some unscoped grant does") was considered + * and rejected: it changes the answer for a fresh environment that writes an + * unscoped grant before the admin set exists, and not changing the fresh-install + * answer is the one thing this predicate may not do. + * * ## Relationship to the `auth-manager.ts` break-glass HTTP guard * * `auth-manager.ts` already guards `/delete-user`, `/admin/remove-user` and @@ -154,12 +213,14 @@ * 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 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. + * Scope in the other direction: this guard watches writes to the four tables + * the administrator population is derived from, and stops there. It has no + * opinion on what a permission set CONTAINS — emptying `admin_full_access`'s + * `system_permissions` would leave administrators who administer nothing, but + * that is a capability question (ADR-0086), and answering it here would make + * every permission-set edit in Setup a break-glass decision. "Who is an + * administrator" is resolved by NAME in this codebase (`resolveAuthzContext`, + * ADR-0068 D2), so the guard reads exactly the name. * * ## Relationship to the ADR-0092 identity write guard * @@ -234,7 +295,7 @@ const DEFAULT_MAX_SCAN = 1000; const SYSTEM_READ: BaseEngineOptions = { context: { isSystem: true } }; /** - * The six writes this guard judges. Carried into every message so a refusal + * The eight 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, and one reading it after an `updateMemberRole` would go looking @@ -242,7 +303,8 @@ const SYSTEM_READ: BaseEngineOptions = { context: { isSystem: true } }; * * 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. + * an administrator; the last two (#6084) take away the `sys_permission_set` row + * that the platform-admin half of the enumeration resolves BY NAME. */ type GuardedOp = | 'ban' @@ -250,7 +312,9 @@ type GuardedOp = | 'member-update' | 'member-delete' | 'grant-update' - | 'grant-delete'; + | 'grant-delete' + | 'permission-set-update' + | 'permission-set-delete'; interface OpWords { /** Reads after "Refusing this …". */ @@ -314,8 +378,49 @@ const OP_WORDS: Record = { subject: 'permission-set grants', table: USER_PERMISSION_SET, }, + 'permission-set-update': { + noun: 'permission-set rename', + verb: 'rename', + gerund: 'renaming', + Verb: 'Rename', + subject: 'permission sets', + table: SystemObjectName.PERMISSION_SET, + }, + 'permission-set-delete': { + noun: 'permission-set removal', + verb: 'remove', + gerund: 'removing', + Verb: 'Remove', + subject: 'permission sets', + table: SystemObjectName.PERMISSION_SET, + }, }; +/** + * [#6084] The closing sentence of a standing refusal: where a write of THIS + * shape actually comes from, so the operator goes and fixes the source instead + * of this guard. + * + * The two identity tables are written by IdP integrations, so their refusals + * point at the SCIM group mapping. `sys_permission_set` is not — nothing in + * SCIM or better-auth writes it; a metadata delete, an `os meta` run or a + * package uninstall does. Sending THAT operator to look at an IdP group would + * send them into a system they may not even run. + */ +function standingOrigin(table: string, noun: string): string { + if (table === SystemObjectName.PERMISSION_SET) { + return ( + `If the ${noun} came from a metadata delete, an 'os meta' run or a package uninstall, ` + + `revoke the '${ADMIN_FULL_ACCESS}' grants first — the permission-set row every platform ` + + 'admin is derived from is the last thing an environment gives up, not the first.' + ); + } + return ( + `If the ${noun} came from an identity provider, the SCIM group mapping is too broad — fix ` + + 'the IdP group, not this guard.' + ); +} + /** * Boolean columns arrive spelled by whichever driver / transport wrote them: * better-auth's adapter is configured `supportsBooleans: false` (so it hands @@ -370,7 +475,7 @@ function toId(value: unknown): string | undefined { * updated and `patch` is the caller's payload, applied over each row. */ interface PendingStandingWrite { - /** `sys_member` or `sys_user_permission_set`. */ + /** `sys_member`, `sys_user_permission_set` or `sys_permission_set` (#6084). */ table: string; /** Ids of the rows this one write addresses (by-id, or the predicate's matches). */ ids: Set; @@ -384,7 +489,8 @@ interface PendingStandingWrite { * * 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 + * `member` → `admin`, a grant re-pointed AT `admin_full_access`, or — since + * #6084 — another permission set RENAMED INTO `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 @@ -433,6 +539,23 @@ const GRANT_STANDING_KEYS = [ 'validUntil', ] as const; +/** + * [#6084] Same, for `sys_permission_set` — and it is a one-element list, + * because the platform-admin half of the enumeration reads exactly one column + * of that table: the `name` it looks the set up by. Everything else a + * permission-set write touches (`label`, `description`, the four permission + * JSON blobs, `active`, provenance) is invisible to "who is an administrator", + * so those writes — which is every projection pass and every Setup edit — cost + * this guard no reads at all. + * + * `id` is deliberately NOT here even though the enumeration reads it. On this + * engine `data.id` on an update ADDRESSES the row (it is what + * `resolveTargetIds` resolves the target from) rather than proposing a new + * primary key, so a key rewrite is not expressible through this write path; the + * two standing key lists above exclude `id` for the same reason. + */ +const PERMISSION_SET_STANDING_KEYS = ['name'] as const; + function touchesAny(data: Record, keys: readonly string[]): boolean { return keys.some((k) => k in data); } @@ -490,11 +613,25 @@ export function registerLastAdminGuard( const now = Date.now(); // 1) Platform admins — unscoped, in-window `admin_full_access` grants. + // + // [#6084] The set row is simulated exactly like the grant rows below it: a + // pending write on `sys_permission_set` can DELETE this row (it drops out of + // `adminSetIds`, and with it every grant that pointed at it) or RENAME it, + // and the name is RE-TESTED for the same reason the grant's + // `permission_set_id` is — the scan's own `where` only proved what the row + // was called BEFORE the write. 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)); + const adminSetIds: string[] = []; + for (const rawSet of sets) { + const set = applyPending(rawSet, pending, SystemObjectName.PERMISSION_SET); + if (!set) continue; // removed outright by the pending write + if (set.name !== ADMIN_FULL_ACCESS) continue; // renamed away — the row survives, the meaning does not + const sid = toId(set.id); + if (sid) adminSetIds.push(sid); + } if (adminSetIds.length > 0) { const links = await scan(op, USER_PERMISSION_SET, { where: { permission_set_id: { $in: adminSetIds } }, @@ -568,9 +705,83 @@ export function registerLastAdminGuard( return out; }; + /** + * [#6084] Called at the ONE place both verdicts read zero administrators: + * decide whether that emptiness is the bootstrap window or an environment + * that has just been emptied, and refuse in the second case. + * + * The evidence is a DANGLING platform-admin-shaped grant: an unscoped, + * in-window `sys_user_permission_set` row whose `permission_set_id` names a + * `sys_permission_set` row that is not there any more. Nobody writes such a + * row — every producer inserts the set first and reads its id back — so it + * can only be left behind by deleting a permission set that grants already + * pointed at, which is write shape (4) landing on `admin_full_access`. A + * fresh environment has no grants at all, or grants whose sets exist; either + * way this returns without refusing and the bootstrap window is untouched. + * + * Deliberately an OVER-approximation of "the `admin_full_access` row was + * deleted": a dangling unscoped grant to some other set trips it too. The + * guard cannot tell the two apart (the name it would compare went away with + * the row), and refusing in an environment that has zero administrators AND a + * grant pointing into nowhere is the fail-closed direction. + */ + const refuseIfEmptiedRatherThanFresh = async (op: GuardedOp): Promise => { + const sets = await scan(op, SystemObjectName.PERMISSION_SET, { fields: ['id'] }); + const known = new Set(); + for (const row of sets) { + const sid = toId(row.id); + if (sid) known.add(sid); + } + // With no permission set at all every grant is dangling, and `$nin: []` is + // not a predicate every driver agrees on — so that case reads unfiltered + // and lets the in-memory re-test below do the work. + const candidates = await scan(op, USER_PERMISSION_SET, { + ...(known.size > 0 ? { where: { permission_set_id: { $nin: [...known] } } } : {}), + }); + + const now = Date.now(); + const dangling: string[] = []; + for (const link of candidates) { + const setId = toId(link.permission_set_id ?? link.permissionSetId); + // A grant that points at nothing names no deleted set. + if (!setId) continue; + // `$nin` is NULL-safe on this engine (#5298) and the drivers differ on + // the edges, so danglingness is re-tested in memory rather than trusted + // from the `where` — the same discipline the enumeration applies to + // `permission_set_id` and `name`. + if (known.has(setId)) continue; + // An org-SCOPED grant never conferred the environment's break-glass + // standing, and an out-of-window one never conferred it either, so + // neither is evidence that this environment once had a platform admin. + if (link.organization_id ?? link.organizationId) continue; + if (!isGrantActive(link, now)) continue; + const uid = toId(link.user_id ?? link.userId); + dangling.push(uid ? `'${uid}' → '${setId}'` : `'${setId}'`); + } + if (dangling.length === 0) return; // a genuinely fresh environment + + const words = OP_WORDS[op]; + logger?.warn( + `[LastAdminGuard] refused a ${words.noun} in an environment with no administrator that is ` + + `NOT a fresh install — ${dangling.length} dangling unscoped grant(s): ${dangling.join(', ')}`, + ); + throw refuse( + `Refusing this ${words.noun}: this environment recognises NO administrator, and this is not ` + + `the bootstrap window — ${dangling.length} unscoped, in-window '${USER_PERMISSION_SET}' ` + + `grant(s) still point at a '${SystemObjectName.PERMISSION_SET}' row that no longer exists ` + + `(${dangling.join(', ')}). That is the state a DELETED '${ADMIN_FULL_ACCESS}' ` + + 'permission-set row leaves behind (#6084): it un-makes every platform admin at once, and ' + + 'reading the resulting emptiness as "no administrator to protect" would switch this guard ' + + `off for every other write too (${BREAK_GLASS_CITATION}). Restore the ` + + `'${ADMIN_FULL_ACCESS}' permission set — the grants naming it are still there — before ` + + 'writing the identity tables again.', + words.table, + ); + }; + /** * 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 + * eight 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 @@ -652,9 +863,14 @@ export function registerLastAdminGuard( 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 - // out of an empty measurement. (A deployment reaches this only before the - // first admin is bootstrapped.) - if (admins.size === 0) return; + // out of an empty measurement. [#6084] …provided the emptiness really IS + // the bootstrap window, and not an environment whose `admin_full_access` + // row was just deleted out from under it — reading THAT as "nothing to + // protect" is what switches this guard off wholesale. + if (admins.size === 0) { + await refuseIfEmptiedRatherThanFresh(op); + return; + } const unbanned = await resolveUnbannedAdmins(op, admins); const targets = await resolveTargetIds( @@ -715,9 +931,14 @@ export function registerLastAdminGuard( 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; + // Same bootstrap exemption the row halves make, with the same #6084 + // qualification: with nobody recognised as an administrator there is no + // break-glass account to protect — unless the environment got there by + // being emptied rather than by being new. + if (before.size === 0) { + await refuseIfEmptiedRatherThanFresh(op); + return; + } const unbannedBefore = await resolveUnbannedAdmins(op, before); // Every administrator is already banned — this write cannot take away an @@ -760,8 +981,7 @@ export function registerLastAdminGuard( `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.', + `${REMEDY} ${standingOrigin(table, words.noun)}`, words.table, ); }); @@ -850,12 +1070,48 @@ export function registerLastAdminGuard( await enforceStanding('grant-delete', USER_PERMISSION_SET, ctx.input); }; + /** + * [#6084] The fourth table. `resolveAdminUserIds` resolves "who is a platform + * admin" by looking the permission set up BY NAME, so renaming that row takes + * the standing away from everyone holding a grant to it, in one write, with + * no identity table touched at all. + * + * Only a payload that touches `name` can move the enumeration + * (PERMISSION_SET_STANDING_KEYS), which is what keeps every projection pass, + * every `os meta resync` and every Setup edit — none of which write `name` — + * free of any read. The data door refuses renames on its own (ADR-0094), and + * that is not this guard's coverage: this one holds for the engine-level and + * system-context writes that never pass the data door. + */ + const guardPermissionSetUpdate = async (rawCtx: unknown): Promise => { + const ctx = ctxOf(rawCtx); + if (ctx.object !== SystemObjectName.PERMISSION_SET) return; + const data = (ctx.input?.data ?? {}) as Record; + if (!touchesAny(data, PERMISSION_SET_STANDING_KEYS)) return; + await enforceStanding( + 'permission-set-update', + SystemObjectName.PERMISSION_SET, + ctx.input, + data, + ); + }; + + const guardPermissionSetDelete = async (rawCtx: unknown): Promise => { + const ctx = ctxOf(rawCtx); + if (ctx.object !== SystemObjectName.PERMISSION_SET) return; + // No payload to pre-filter on, and the same reasoning as the other delete + // halves: removing ANY permission-set row is judged, and the simulation + // answers "not `admin_full_access`, nothing changes" without a refusal for + // every row that is not the one the enumeration reads. + await enforceStanding('permission-set-delete', SystemObjectName.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. 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. + // four standing hooks (#5978) and the two permission-set hooks (#6084) 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, @@ -886,9 +1142,20 @@ export function registerLastAdminGuard( priority: 20, packageId, }); + engine.registerHook('beforeUpdate', guardPermissionSetUpdate, { + object: SystemObjectName.PERMISSION_SET, + priority: 20, + packageId, + }); + engine.registerHook('beforeDelete', guardPermissionSetDelete, { + object: SystemObjectName.PERMISSION_SET, + priority: 20, + packageId, + }); 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', + 'sys_member and sys_user_permission_set (standing revocation), and sys_permission_set ' + + '(the admin_full_access row every platform admin is derived from) — ADR-0024 D5.2', ); } From 65fe2c9a78cf4d395e7a36b4243935d668181bb0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 02:09:55 +0000 Subject: [PATCH 2/2] =?UTF-8?q?docs(plugin-auth):=20=E7=94=A8=E5=AE=9E?= =?UTF-8?q?=E6=B5=8B=E7=BB=93=E8=AE=BA=E6=9B=BF=E6=8D=A2=E3=80=8C=E6=B8=85?= =?UTF-8?q?=E7=A9=BA=E8=83=BD=E5=8A=9B=3D=E7=AE=A1=E7=90=86=E5=91=98?= =?UTF-8?q?=E5=A4=B1=E6=95=88=E3=80=8D=E7=9A=84=E6=8E=A8=E6=96=AD=20(#6084?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveAuthzContext` 的 `hasPlatformAdminGrant` 只看 `ps.name === 'admin_full_access'` (resolve-authz-context.ts:390),`derivePosture` 就凭这个布尔返回 PLATFORM_ADMIN —— 清空 `system_permissions` 并不会让 platform admin 失效,posture 与 superuser bypass 都挂在**名字**上。原注释把它写成一处 break-glass 缺口是错的;改为记录实测结论,顺带 把「只读 name」这条静态跳过键的依据说实。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- .../plugin-auth/src/last-admin-guard.ts | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index 804b382fc3..8a75ac5939 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -215,12 +215,17 @@ * * Scope in the other direction: this guard watches writes to the four tables * the administrator population is derived from, and stops there. It has no - * opinion on what a permission set CONTAINS — emptying `admin_full_access`'s - * `system_permissions` would leave administrators who administer nothing, but - * that is a capability question (ADR-0086), and answering it here would make - * every permission-set edit in Setup a break-glass decision. "Who is an - * administrator" is resolved by NAME in this codebase (`resolveAuthzContext`, - * ADR-0068 D2), so the guard reads exactly the name. + * opinion on what a permission set CONTAINS, and that is not a gap being left + * open — it was measured. `resolveAuthzContext` sets `hasPlatformAdminGrant` + * from `ps.name === 'admin_full_access'` alone and `derivePosture` returns + * `PLATFORM_ADMIN` off that boolean, so emptying the set's + * `system_permissions` does NOT un-make a platform admin: the posture rung and + * the superuser bypass ride on the NAME (ADR-0068 D2 / ADR-0095 D3). Such an + * edit costs the holder `setup.access` / `studio.access` — Setup and Studio go + * invisible — while the data plane still answers, so it is recoverable from + * inside the product and is a capability question (ADR-0086), not a break-glass + * one. The name is the whole of what makes an administrator here, so the name + * is the whole of what this guard reads. * * ## Relationship to the ADR-0092 identity write guard * @@ -544,9 +549,10 @@ const GRANT_STANDING_KEYS = [ * because the platform-admin half of the enumeration reads exactly one column * of that table: the `name` it looks the set up by. Everything else a * permission-set write touches (`label`, `description`, the four permission - * JSON blobs, `active`, provenance) is invisible to "who is an administrator", - * so those writes — which is every projection pass and every Setup edit — cost - * this guard no reads at all. + * JSON blobs, `active`, provenance) is invisible to "who is an administrator" — + * `resolveAuthzContext` derives `platform_admin` from the NAME, not from the + * capabilities the set carries — so those writes, which is every projection + * pass and every Setup edit, cost this guard no reads at all. * * `id` is deliberately NOT here even though the enumeration reads it. On this * engine `data.id` on an update ADDRESSES the row (it is what