Skip to content

Commit 8dcf607

Browse files
baozhoutaoclaude
andauthored
feat(plugin-auth): break-glass 守卫的 delete 半边 —— 最后一个管理员也删不掉 (#5941) (#5993)
* feat(plugin-auth): break-glass 守卫的 delete 半边 —— 最后一个管理员也删不掉 (#5941) #5892 / PR #5939 只守住了 ban(`sys_user.banned = true`,`beforeUpdate`)。 删除这条路径今天没有任何一段代码判断「管理员还剩几个」:`auth-manager.ts` 那段 HTTP 守卫判的是「最后一个本地 credential 持有人」,目标不持本地密码 (enforced SSO 下 IdP 托管 / SCIM JIT provision 出来的管理员)时整段跳过, 于是 SCIM `DELETE /Users/{id}` / `/admin/remove-user` 可以把环境最后一个 管理员的行删掉,环境从此无人可管。 本次把同一条不变量装到 `sys_user` 的 `beforeDelete` 上,与 ban 半边同形状、 同一份管理员枚举:装在写上而不是端点上(SCIM adapter、admin remove-user、 导入、脚本都覆盖),by-id 与谓词/multi(含无谓词的清表 multi)都守,对 `isSystem` 同样生效(真正会锁死的正是 system 那条),并且 fail-closed —— 枚举读不出来或超出 maxScan 一律拒绝。 实测(写进了模块头注释):`beforeDelete` 的 by-id 派发给 `input.id`, 谓词/multi 派发把行域谓词放在 `input.options.where`;`ctx.previous` (引擎 #5272 预取 + objectql 的 `sys_fetch_previous_delete` 内建)只在 by-id 形状上绑定,批量派发上是 undefined ——守卫因此完全不消费它,只解析目标 id 集。 拒绝走 `PERMISSION_DENIED` + 403,信息按调用方实际执行的动作措辞 (「Refusing to delete '…'」),并给出 ADR-0024 D5.2 依据与修复办法; `withValidationErrorMapping` 逐方法包装,#5939 加的 403 arm 原样覆盖 `delete`。 模块随之更名为 `last-admin-guard.ts` / `registerLastAdminGuard`(原 `last-admin-ban-guard.ts`,同一个未发布周期内加入):它现在注册两个钩子, 旧名字会低估它装了什么。 ⛔ 未动 `auth-manager.ts:1239-1300` 的 fail-open HTTP 守卫段(分诊明令, 它继续守自己的 credential 版不变量,两层并存)。撤销管理员「身份」的第三条 写法(改/删 `sys_member` 行、撤 `admin_full_access` 授权)另开 #5978。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv * docs(plugin-auth): 精确化守卫读取 `input.options.where` 与 HookContext 契约表的关系 #5964 刚把 `HookEvent` 的枚举注释对齐到契约表的「谓词不在 `input` 上」。 两句都对,但要分清:钩子拿不到的是 composed `ast`(生效谓词);`before*` 期间 `input.options` 仍是调用方那只 engine options 包(engine.ts 5516→5517 / 6137→6152 之后才重建成 DriverOptions),`where` 与 `multi` 都在 —— 守卫读的 正是它。中间件只收窄不放宽,所以把调用方谓词当目标集是上界近似,对 fail-closed 守卫恰是安全方向。契约表那两处 `before` 行的措辞另记为 #5997。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f192981 commit 8dcf607

5 files changed

Lines changed: 655 additions & 115 deletions

File tree

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
---
2+
"@objectstack/plugin-auth": minor
3+
---
4+
5+
feat(plugin-auth): break-glass — the last administrator cannot be DELETED either (#5941)
6+
7+
#5892 closed the *ban* half of ADR-0024 D5.2's break-glass invariant. The
8+
**delete** half was still open, and it was reachable end to end: in an enforced
9+
SSO environment the last administrator is typically IdP-managed and holds no
10+
local password, so when the IdP drops them from the admin group the resulting
11+
SCIM `DELETE /Users/{id}` (or `/admin/remove-user`, or `/delete-user`) removed
12+
the row and **left the environment with nobody able to administer it** — quite
13+
possibly with a password-holding non-admin still able to sign in and change
14+
nothing. There is no recovery path from inside the product once that happens.
15+
16+
The pre-existing HTTP guard on those three endpoints did not cover it: it
17+
protects the last holder of a local `credential` account, so it skips the
18+
credential-less (IdP-managed) target entirely. It is unchanged and keeps
19+
enforcing its own invariant.
20+
21+
**What changed.** The guard module now enforces one invariant on *both* writes
22+
that can take the last administrator away, off one administrator enumeration:
23+
24+
| write | hook |
25+
|:--|:--|
26+
| `sys_user.banned = true` | `beforeUpdate` (#5892) |
27+
| deleting the `sys_user` row | `beforeDelete` (**new**) |
28+
29+
The delete half is the ban half's twin in every property that matters: it sits
30+
on the **write**, so it holds for the SCIM adapter delete, better-auth's admin
31+
remove-user, an import and a script alike; it covers by-id **and**
32+
predicate/`multi` deletes (including the unpredicated `multi` that would empty
33+
the table); it applies to **every** context, `isSystem` included, because the
34+
deprovision path that actually locks organizations out is the system one; and it
35+
**fails closed** — an administrator population that cannot be read, or is too
36+
large to enumerate, refuses the delete rather than guessing.
37+
38+
The refusal is a **403** carrying `PERMISSION_DENIED` and names the operation
39+
the caller actually attempted ("Refusing to delete 'usr_…'"), the invariant
40+
(ADR-0024 D5.2), and the fix — grant someone else `admin_full_access` or an
41+
owner/admin membership first, and if an IdP drove it, the SCIM deprovision is
42+
too broad. On the auth pipeline it surfaces as an `APIError`, not an opaque 500.
43+
44+
Untouched: deleting anyone who is not an administrator, deleting an
45+
administrator while another unbanned one remains, and deleting an administrator
46+
who is already banned (that account could not sign in either way).
47+
48+
**Rename.** The module is now `last-admin-guard.ts` and the exported registration
49+
function is `registerLastAdminGuard` (was `last-admin-ban-guard.ts` /
50+
`registerLastAdminBanGuard`, added in the same unreleased cycle) — it registers
51+
both hooks, so the old name would have understated what it installs. Hosts that
52+
wire the guard onto their own ObjectQL engine rename the import; there is no
53+
other change to its signature or behaviour.
54+
55+
Not covered, tracked separately (#5978): revoking the *standing* that makes
56+
someone an administrator — deleting or downgrading their `sys_member` row,
57+
removing the `admin_full_access` grant — leaves the user row in place and writes
58+
a different table, so neither hook sees it.

packages/plugins/plugin-auth/src/auth-plugin.ts

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ import {
4141
registerManagedUpdateWhitelist,
4242
type SecondaryStorageLike,
4343
} from './identity-write-guard.js';
44-
import { registerLastAdminBanGuard } from './last-admin-ban-guard.js';
44+
import { registerLastAdminGuard } from './last-admin-guard.js';
4545
import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js';
4646
import { MANAGED_EXTENSION_EDITABLE_FIELDS } from './managed-extension-fields.js';
4747
import { runSetInitialPassword } from './set-initial-password.js';
@@ -988,17 +988,18 @@ export class AuthPlugin implements Plugin {
988988
this.effectiveSecondaryStorage as SecondaryStorageLike | undefined,
989989
});
990990
// [cloud ADR-0024 D5.2] Break-glass — the SAME `sys_user` write
991-
// chokepoint, guarding a different question: not "may this caller
991+
// chokepoints, guarding a different question: not "may this caller
992992
// write identity tables" (above, and system writes bypass it by
993-
// design) but "may this VALUE be written at all". A `banned = true`
994-
// that would leave the environment with no administrator able to sign
995-
// in is refused for EVERY context, `isSystem` included — because the
996-
// path that actually locks an org out is the system one (better-auth's
997-
// admin ban, driven by a SCIM `active: false`). Registered at
998-
// priority 20 so the ADR-0092 strip above (10) still answers first for
999-
// user-context callers. See last-admin-ban-guard.ts.
1000-
registerLastAdminBanGuard(engine, {
1001-
packageId: 'com.objectstack.plugin-auth.last-admin-ban-guard',
993+
// design) but "may this WRITE happen at all". A `banned = true` (#5892)
994+
// or a row DELETE (#5941) that would leave the environment with no
995+
// administrator able to sign in is refused for EVERY context,
996+
// `isSystem` included — because the paths that actually lock an org out
997+
// are the system ones (better-auth's admin ban and remove-user, driven
998+
// by a SCIM `active: false` / `DELETE /Users/{id}`). Registered at
999+
// priority 20 so the ADR-0092 checks above (10) still answer first for
1000+
// user-context callers. See last-admin-guard.ts.
1001+
registerLastAdminGuard(engine, {
1002+
packageId: 'com.objectstack.plugin-auth.last-admin-guard',
10021003
logger: ctx.logger,
10031004
});
10041005
} catch {

packages/plugins/plugin-auth/src/index.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,14 @@ export * from './admin-user-endpoints.js';
2020
export * from './placeholder-email.js';
2121
export * from './admin-import-users.js';
2222
export * from './identity-write-guard.js';
23-
// [cloud ADR-0024 D5.2 / #5892] The break-glass ban guard. Exported for the
24-
// same reason its ADR-0092 neighbour above is: a host that stands up its own
25-
// ObjectQL engine (the cloud control plane, an embedding that skips this
26-
// plugin's `kernel:ready` wiring) has to be able to register the invariant
27-
// itself rather than ship an environment that can ban its last administrator.
28-
export * from './last-admin-ban-guard.js';
23+
// [cloud ADR-0024 D5.2 / #5892 / #5941] The break-glass guard — one invariant
24+
// on both writes that can take the last administrator away (`banned = true`
25+
// and deleting the `sys_user` row). Exported for the same reason its ADR-0092
26+
// neighbour above is: a host that stands up its own ObjectQL engine (the cloud
27+
// control plane, an embedding that skips this plugin's `kernel:ready` wiring)
28+
// has to be able to register the invariant itself rather than ship an
29+
// environment that can ban or delete its last administrator.
30+
export * from './last-admin-guard.js';
2931
export * from './sys-user-writable-fields.js';
3032
export * from './otp-send-guard.js';
3133
// ADR-0069 D2 / #4772 — the cross-node rate-limit counter store (kernel cache,

0 commit comments

Comments
 (0)