From 3e58ae081e6b74bd84bbeb1799b124c5423518a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 01:01:17 +0000 Subject: [PATCH] =?UTF-8?q?fix(approvals):=20delete=20the=20two=20`session?= =?UTF-8?q?.roles`=20admin=20exemptions=20=E2=80=94=20record=20lock=20and?= =?UTF-8?q?=20delegation=20guard=20(#4839)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lifecycle-hooks.ts` carried two admin bypasses, both reading `ctx.session.roles`: the approval **record lock** (`bindApprovalLockHook`) and the `sys_approval_delegation` write guard (`bindDelegationWriteGuard`). Both are removed. Neither ever ran. `session.roles` has no producer anywhere in the platform — ObjectQL's `buildSession()` builds its session field by field and never writes it — so both branches were dead on every real engine path: the record lock has always applied to admins, and delegation has always been self-managed. The spec declares `roles` on the HookContext session, two consumers read it, nothing fills it: declared != enforced. They were also a second privilege dialect. Privilege here is judged by the ADR-0095 vocabulary (`permissions` / `positions` / derived posture) — ADR-0090 D3 bans the `role` spelling outright — and the sibling `ApprovalService.isOverrideActor` already does exactly that. Per the maintainer's ruling both sites are DELETED rather than rewired to the correct predicate: - Record lock: the sanctioned admin rescue path already exists (#3424 — recall / reject / reassign, gated by `isOverrideActor`, audited via `via_override`). Finalising the request is what releases the lock, so a record is never edited under a live approval. - Delegation: evidence-first, and the evidence says coverage exists. A delegation is consulted only at slate-resolution time (`applyOooDelegation` inside `resolveApproverSpec`), so forging one for an already-unavailable approver never moved their pending work; `reassign` / `recall` / reject do, and are gated by `isOverrideActor`. "Delegation is self-managed" is now the final semantic. Tests: `admin-exemption-retired.test.ts` turns that evidence into executable assertions and adds a source-level pin — no non-test source in this package may name a `roles` identifier or compare against the string 'admin'. The existing "allows an admin override" tests become "does NOT exempt ..." pins over both the retired dialect and every live ADR-0095 admin shape, on the by-id and predicate paths. The spec-side retirement of `session.roles` (now with zero consumers) is a separate protocol change under ADR-0049 enforce-or-remove — not in this PR. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX --- .../approvals-admin-exemption-retired.md | 37 ++ .../src/admin-exemption-retired.test.ts | 447 ++++++++++++++++++ .../src/approval-service.test.ts | 95 +++- .../plugin-approvals/src/lifecycle-hooks.ts | 68 ++- 4 files changed, 624 insertions(+), 23 deletions(-) create mode 100644 .changeset/approvals-admin-exemption-retired.md create mode 100644 packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts diff --git a/.changeset/approvals-admin-exemption-retired.md b/.changeset/approvals-admin-exemption-retired.md new file mode 100644 index 0000000000..9deabe72d9 --- /dev/null +++ b/.changeset/approvals-admin-exemption-retired.md @@ -0,0 +1,37 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): 删除两处读 `session.roles` 的 admin 豁免 —— 记录锁与委托守卫回到单一权限词汇 (#4839) + +`plugin-approvals` 的 `lifecycle-hooks.ts` 里有两处 admin 豁免,都读 +`ctx.session.roles`:审批**记录锁**的 `bindApprovalLockHook`,以及 +`sys_approval_delegation` 的 `bindDelegationWriteGuard`。两处都已删除。 + +**这不是行为变更。** `session.roles` 在整个平台没有生产者 —— ObjectQL 的 +`buildSession()` 逐字段构造 session,从不写 `roles` —— 所以两个分支在任何真实引擎 +路径上都是死代码,记录锁一直就对 admin 生效,委托一直就只能本人管理。删除让代码 +说出运行时本来就在做的事(spec 的 `HookContext` 声明了 `roles`,消费方在读,生产方 +从不写:典型的 declared ≠ enforced)。 + +**为什么不是「改用正确判据」而是删除。** `roles.includes('admin')` 还是第二套权限 +方言:本仓库的权限一律由 ADR-0095 词汇裁决(能力授予 `permissions`、任职 +`positions`、由其派生的 posture),ADR-0090 D3 更是直接禁掉 `role` 这个拼法。同包的 +`ApprovalService.isOverrideActor` 已经这么做了。维护者裁定两处都取「删除」而非改判据: + +- **记录锁**:admin 释放锁定记录的正规路径已经存在(#3424 —— `recall` / + `decideNode` 驳回 / `reassign`,全部由 `isOverrideActor` 把关并留痕 + `via_override`)。让审批终结来释放锁,记录就永远不会在审批在途时被改写 —— 这正是 + 合规场景购买记录锁所要的保证。 +- **委托**:最终语义确定为**仅本人管理**(`delegator_id` 必须等于写入者;只有 system + 上下文旁路)。审批人临时不可用时,替他处置**在途**审批用的是 + `reassign`(把该审批人的名额交给替代人,连 per_group 分组归属一起带过去)/ + `recall` / 驳回。反过来,「替别人建一条委托」本来也做不到这件事:委托只在请求 + **开启**时(`resolveApproverSpec` 内的 `applyOooDelegation`)被查询,对已经挂在该 + 审批人名下的在途审批毫无作用。 + +新增 `admin-exemption-retired.test.ts`,把上述证据变成可执行断言,并加了一道源码级 +pin:本包非测试源码中不得再出现 `roles` 标识符或与字符串 `'admin'` 的比较。 + +spec 侧 `session.roles` 的退役(至此零消费方)按 ADR-0049 enforce-or-remove 另立协议 +单处理,不在本次改动内。 diff --git a/packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts b/packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts new file mode 100644 index 0000000000..df1304abd9 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts @@ -0,0 +1,447 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The two `session.roles.includes('admin')` exemptions are gone (#4839) — and + * this file is the evidence that nothing was stranded by removing them. + * + * ## What was wrong + * + * `lifecycle-hooks.ts` carried two admin bypasses, both reading + * `ctx.session.roles`: one on the approval **record lock**, one on the + * **delegation write-guard**. `roles` has no producer anywhere in the platform — + * ObjectQL's `buildSession()` builds its session field by field and never writes + * it — so both branches were dead on every real engine path. Classic + * declared ≠ enforced: the spec's `HookContext` session declares + * `roles: z.array(z.string()).optional()`, two consumers read it, and no + * producer ever fills it. + * + * They also spoke a **second privilege dialect**. This codebase judges privilege + * by the ADR-0095 vocabulary — capability grants (`permissions`), placements + * (`positions`) and the posture derived from them — and ADR-0090 D3 bans the + * `role` spelling outright. `roles.includes('admin')` is exactly the string + * comparison those decisions exist to keep out. The sibling code in this very + * package already does it right: `ApprovalService.isOverrideActor`. + * + * ## Why they were DELETED rather than rewired to `isOverrideActor` + * + * The maintainer ruled (2026-08-04) that the record lock gets no admin override + * at all — "an admin edits a record while its approval is live" is a liability + * for compliance-minded tenants, and the sanctioned rescue path already exists. + * The delegation half was conditional on evidence, and the evidence is below, + * executed rather than asserted in prose: + * + * 1. {@link describe} "a delegation cannot rescue an in-flight approval" — a + * delegation is consulted only while a request is being OPENED, so an admin + * forging one for an approver who has *already* gone unavailable would not + * have moved that approver's pending work anyway. The exemption could not + * do the job it was written for. + * 2. {@link describe} "the sanctioned admin path covers an unavailable + * approver" — `reassign` / `recall` / `decideNode` reject, each gated by + * `isOverrideActor`, do cover it, including handing one named approver's + * slot to a substitute, which is precisely the "delegate for someone else" + * operation. + * + * Coverage exists, so delegation is now SELF-MANAGED as its final semantic. + * + * ## And the dialect must not come back + * + * The last describe is a source-level pin: no non-test source in this package + * may name a `roles` identifier or compare a value against the string + * `'admin'`. It is deliberately crude, because the thing it guards against is + * crude — someone re-adding a plausible-looking admin shortcut next to a guard. + * The spec-side retirement of `session.roles` is a separate protocol change + * (ADR-0049 enforce-or-remove); this pin is what lets it start from a clean fact + * base: zero consumers in this package. + */ + +import { readdirSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { describe, it, expect, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; + +import { ApprovalService } from './approval-service.js'; +import { bindApprovalLockHook, bindDelegationWriteGuard } from './lifecycle-hooks.js'; + +interface FakeRow { [k: string]: any } + +/** The same minimal engine shape `approval-service.test.ts` uses. */ +function makeFakeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + const hooks: Record any; object?: string | string[]; packageId?: string }>> = {}; + + function matches(row: FakeRow, filter: any): boolean { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (k === '$or') { + if (!(v as any[]).some(sub => matches(row, sub))) return false; + continue; + } + if (k === '$and') { + if (!(v as any[]).every(sub => matches(row, sub))) return false; + continue; + } + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (rv !== v) return false; + } + return true; + } + + return { + _tables: tables, + async find(object: string, options?: any) { + const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); + if (options?.orderBy?.[0]) { + const { field, order } = options.orderBy[0]; + rows.sort((a, b) => { + const av = a[field]; const bv = b[field]; + if (av === bv) return 0; + const cmp = av > bv ? 1 : -1; + return order === 'desc' ? -cmp : cmp; + }); + } + const start = options?.offset ?? 0; + return rows.slice(start, start + (options?.limit ?? 1000)); + }, + async insert(object: string, data: any) { + ensure(object).push({ ...data }); + return { ...data }; + }, + async update(object: string, idOrData: any, _opts?: any) { + const data = typeof idOrData === 'object' ? idOrData : _opts; + const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + const table = ensure(object); + const i = table.findIndex(r => r.id === id); + if (i >= 0) table[i] = { ...table[i], ...data }; + return table[i]; + }, + async delete(object: string, options?: any) { + // [#4550] Pinned to ObjectQL.delete's OWN dispatch predicate. A double + // looser than the engine it stands in for is how #4434 shipped a REST + // route that answered 500 to every caller with its suite green. + const dispatch = assertEngineDeleteDispatch(options); + const table = ensure(object); + if (dispatch.kind === 'multi') { + const survivors = table.filter(r => !matches(r, options?.where)); + const deleted = table.length - survivors.length; + table.splice(0, table.length, ...survivors); + return { deleted }; + } + const i = table.findIndex(r => r.id === dispatch.id); + if (i >= 0) table.splice(i, 1); + return { id: dispatch.id }; + }, + registerHook(event: string, handler: (ctx: any) => any, options?: any) { + (hooks[event] ??= []).push({ handler, object: options?.object, packageId: options?.packageId }); + }, + unregisterHooksByPackage(packageId: string): number { + let n = 0; + for (const ev of Object.keys(hooks)) { + const before = hooks[ev].length; + hooks[ev] = hooks[ev].filter(h => h.packageId !== packageId); + n += before - hooks[ev].length; + } + return n; + }, + async fire(event: string, ctx: any) { + for (const h of hooks[event] ?? []) { + if (h.object) { + const objs = Array.isArray(h.object) ? h.object : [h.object]; + if (!objs.includes(ctx.object)) continue; + } + await h.handler(ctx); + } + }, + }; +} + +/** The submitter who opens every request below. */ +const SUBMITTER = { userId: 'u1', tenantId: 't1', positions: [], permissions: [] } as any; +const SYS = { isSystem: true, positions: [], permissions: [] } as any; +/** An ADR-0095 platform admin: an unscoped `admin_full_access` capability grant. */ +const PLATFORM_ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any; +/** The substitute who takes the unavailable approver's work. */ +const SUBSTITUTE = { userId: 'u7', tenantId: 't1', positions: [], permissions: [] } as any; + +/** + * A properly STAFFED approval: one named, real approver (`u9`) — the person who + * is about to go on sick leave. Not the #3424 "unstaffed position" shape; the + * whole question here is what happens when a REAL approver becomes unavailable. + */ +const staffedInput = (approver = 'u9') => ({ + object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'approve_step', + flowName: 'deal_approval', + config: { + approvers: [{ type: 'user' as const, value: approver }], + behavior: 'first_response' as const, + lockRecord: true, + }, + record: { id: 'opp1', amount: 100 }, +}); + +describe('#4839 evidence — a delegation cannot rescue an IN-FLIGHT approval', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + }); + + /** Declare an out-of-office delegation directly (bypassing the write guard). */ + const seedDelegation = (delegatorId: string, delegateId: string) => { + engine._tables['sys_approval_delegation'] = [{ + id: 'del1', delegator_id: delegatorId, delegate_id: delegateId, + valid_from: null, valid_until: null, reason: 'sick leave', organization_id: null, + }]; + }; + + it('a delegation declared AFTER the request opened does not move the pending slot', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + expect(req.pending_approvers).toEqual(['u9']); + + // u9 falls ill. Someone declares the delegation now — the very thing the + // deleted admin exemption would have let an admin do on u9's behalf. + seedDelegation('u9', 'u7'); + + const after = await svc.getRequest(req.id, SYS); + expect(after!.status).toBe('pending'); + // Still routed to u9. `applyOooDelegation` runs inside `resolveApproverSpec`, + // i.e. at slate-resolution time, and this request's slate was resolved and + // snapshotted when it opened. So the exemption could not have delivered the + // outcome ("hand u9's pending approvals to u7") it was written for. + expect(after!.pending_approvers).toEqual(['u9']); + }); + + it('the delegation DOES apply to a request opened afterwards — it is an open-time rule, nothing more', async () => { + seedDelegation('u9', 'u7'); + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + // Guard the guard: the delegation machinery is wired and working in this + // fixture, so the assertion above is "does not apply to in-flight work", + // not "delegation is broken here". + expect(req.pending_approvers).toEqual(['u7']); + }); +}); + +describe('#4839 evidence — the sanctioned admin path covers an unavailable approver', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + bindApprovalLockHook(engine as any); + engine._tables['opportunity'] = [{ id: 'opp1', amount: 100, stage: 'new' }]; + }); + + /** An ordinary member's edit of the approval's target record. */ + const memberEdit = () => engine.fire('beforeUpdate', { + object: 'opportunity', + input: { id: 'opp1', data: { amount: 200 } }, + session: { isSystem: false, positions: [], permissions: [], userId: 'u1' }, + }); + + it('REASSIGN: an admin hands the unavailable approver’s slot to a substitute, who decides normally', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + expect(req.pending_approvers).toEqual(['u9']); + + // This is the "delegate on someone else's behalf" operation, in the one + // vocabulary the codebase has: `isOverrideActor` admits the admin even + // though they hold no slot, and the hand-off is audited. + const out = await svc.reassign(req.id, { actorId: 'root', to: 'u7', from: 'u9', comment: 'u9 on sick leave' }, PLATFORM_ADMIN); + expect(out.request.pending_approvers).toEqual(['u7']); + + const decided = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u7' }, SUBSTITUTE); + expect(decided.finalized).toBe(true); + expect(decided.request.status).toBe('approved'); + }); + + it('the reassign is audited as an override — the admin is never spoofed as the approver', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.reassign(req.id, { actorId: 'root', to: 'u7', from: 'u9' }, PLATFORM_ADMIN); + const acts = await svc.listActions(req.id, SYS); + expect(acts.at(-1)).toMatchObject({ + action: 'reassign', actor_id: 'root', reassign_from: 'u9', reassign_to: 'u7', via_override: true, + }); + }); + + it('RECALL: an admin withdraws the request, and THAT is what releases the record lock', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + // While the approval is live the record is locked — for everyone, admins + // included (see `approval-service.test.ts`, "does NOT exempt ..."). + await expect(memberEdit()).rejects.toThrow(/RECORD_LOCKED/); + + const out = await svc.recall(req.id, { actorId: 'root', comment: 'approver unreachable' }, PLATFORM_ADMIN); + expect(out.request.status).toBe('recalled'); + expect(out.request.pending_approvers).toEqual([]); + + // The lock keys on PENDING status, so finalising the request is the release. + // The record was never edited under a live approval — the property the + // deleted record-lock exemption would have broken. + await expect(memberEdit()).resolves.toBeUndefined(); + }); + + it('REJECT: an admin decides the request instead, which also releases the lock', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + const out = await svc.decideNode(req.id, { decision: 'reject', actorId: 'root', comment: 'approver left' }, PLATFORM_ADMIN); + expect(out.finalized).toBe(true); + expect(out.request.status).toBe('rejected'); + await expect(memberEdit()).resolves.toBeUndefined(); + }); + + it('a NON-privileged member gets none of this — the override is a real gate, not an open door', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + const member = { userId: 'nobody', tenantId: 't1', positions: [], permissions: [] } as any; + await expect(svc.reassign(req.id, { actorId: 'nobody', to: 'u7', from: 'u9' }, member)).rejects.toThrow(/FORBIDDEN/); + await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'nobody' }, member)).rejects.toThrow(/FORBIDDEN/); + }); + + it('a `roles: [admin]` session gets none of it either — the retired dialect grants nothing anywhere', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + const fakeAdmin = { userId: 'pretender', tenantId: 't1', positions: [], permissions: [], roles: ['admin'] } as any; + await expect(svc.reassign(req.id, { actorId: 'pretender', to: 'u7', from: 'u9' }, fakeAdmin)).rejects.toThrow(/FORBIDDEN/); + await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'pretender' }, fakeAdmin)).rejects.toThrow(/FORBIDDEN/); + }); +}); + +describe('#4839 — the delegation guard has no admin exemption left', () => { + const DEL = 'sys_approval_delegation'; + let engine: ReturnType; + + beforeEach(() => { + engine = makeFakeEngine(); + bindDelegationWriteGuard(engine as any); + }); + + const fireInsert = (data: any, session: any) => + engine.fire('beforeInsert', { object: DEL, input: { data }, session }); + + it('an ADR-0095 platform admin may not declare a delegation for someone else', async () => { + await expect(fireInsert({ delegator_id: 'u9', delegate_id: 'u7' }, PLATFORM_ADMIN)) + .rejects.toThrow(/FORBIDDEN/); + }); + + it('the system context still bypasses — service / seed / import writes are not user writes', async () => { + await expect(fireInsert({ delegator_id: 'u9', delegate_id: 'u7' }, SYS)).resolves.toBeUndefined(); + }); +}); + +// ── The dialect must not come back ─────────────────────────────────── + +/** + * Strip `//` and block comments, keeping string and template literals intact. + * + * Comments are stripped because this file's own explanation — and the ones now + * standing where the exemptions were — necessarily *name* the retired dialect. + * A pin that could not tell an explanation from an implementation would force + * the rationale out of the code, which is the failure ADR anchoring exists to + * prevent (AGENTS.md Prime Directive #13). + */ +function stripComments(src: string): string { + let out = ''; + let i = 0; + const n = src.length; + while (i < n) { + const c = src[i]; + const d = src[i + 1]; + if (c === '/' && d === '/') { + while (i < n && src[i] !== '\n') i++; + continue; + } + if (c === '/' && d === '*') { + i += 2; + while (i < n && !(src[i] === '*' && src[i + 1] === '/')) i++; + i += 2; + continue; + } + if (c === '"' || c === "'" || c === '`') { + out += c; i++; + while (i < n && src[i] !== c) { + if (src[i] === '\\' && i + 1 < n) { out += src[i] + src[i + 1]; i += 2; continue; } + out += src[i]; i++; + } + if (i < n) { out += src[i]; i++; } + continue; + } + out += c; i++; + } + return out; +} + +/** Every shape of the retired admin dialect, as source patterns. */ +const DIALECT_PATTERNS: Array<[string, RegExp]> = [ + ['a `roles` identifier (the session field ObjectQL never populates)', /\broles\b/], + ["a membership test against the string 'admin'", /\.includes\(\s*(['"])admin\1/], + ["an equality test against the string 'admin'", /[=!]==?\s*(['"])admin\1|(['"])admin\2\s*[=!]==?/], +]; + +function collectSources(dir: string, acc: string[] = []): string[] { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = join(dir, entry.name); + if (entry.isDirectory()) { collectSources(full, acc); continue; } + if (!entry.name.endsWith('.ts')) continue; + if (entry.name.endsWith('.test.ts')) continue; // fixtures legitimately build the shape + acc.push(full); + } + return acc; +} + +describe('#4839 pin — `session.roles` has zero readers left in plugin-approvals', () => { + const SRC = fileURLToPath(new URL('.', import.meta.url)); + const sources = collectSources(SRC); + + it('the scan actually reads this package’s sources', () => { + // Guard the guard: a broken path or a too-eager filter would make every + // assertion below pass over an empty list. + expect(sources.length).toBeGreaterThan(10); + expect(sources.some(f => f.endsWith('lifecycle-hooks.ts'))).toBe(true); + expect(sources.some(f => f.endsWith('approval-service.ts'))).toBe(true); + expect(sources.every(f => !f.endsWith('.test.ts'))).toBe(true); + }); + + it('the comment stripper keeps code and strings, and drops only comments', () => { + expect(stripComments("const a = 1; // roles.includes('admin')\nconst b = 2;")) + .toBe('const a = 1; \nconst b = 2;'); + expect(stripComments("/* roles */ const c = 'admin';")).toBe(" const c = 'admin';"); + // A `//` inside a string is not a comment — a stripper that thought so would + // silently blind the scan from that point on. + expect(stripComments("const u = 'https://x'; const r = roles;")).toContain('roles'); + }); + + it.each(DIALECT_PATTERNS)('no non-test source contains %s', (_label, pattern) => { + const offenders = sources + .filter(file => pattern.test(stripComments(readFileSync(file, 'utf8')))) + .map(file => file.slice(SRC.length)); + expect(offenders, [ + `The retired admin dialect (#4839) reappeared in: ${offenders.join(', ')}.`, + 'Privilege in this codebase is judged by the ADR-0095 vocabulary — capability', + 'grants (`permissions`), placements (`positions`) and the derived posture —', + 'never by a session field named `roles` or a comparison against the string', + "'admin' (ADR-0090 D3 bans the `role` spelling outright). `session.roles` has", + 'no producer at all: ObjectQL\'s `buildSession()` never writes it, so a guard', + 'reading it is dead code that merely LOOKS like an authorization decision.', + 'Reuse `ApprovalService.isOverrideActor` (or its exact predicate) instead.', + ].join('\n')).toEqual([]); + }); + + it('detects the dialect when it IS present — the patterns are not vacuous', () => { + const reintroduced = "const roles = ctx.session.roles ?? []; if (roles.includes('admin')) return;"; + const stripped = stripComments(reintroduced); + for (const [, pattern] of DIALECT_PATTERNS.slice(0, 2)) { + expect(pattern.test(stripped)).toBe(true); + } + expect(DIALECT_PATTERNS[2][1].test("if (r === 'admin') return;")).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 58a91d8a7a..f845124f5f 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -128,6 +128,36 @@ function makeFakeEngine() { const CTX = { userId: 'u1', tenantId: 't1', positions: [], permissions: [] } as any; const SYS = { isSystem: true, positions: [], permissions: [] } as any; +/** + * Every session shape that could plausibly be read as "this caller is an admin" + * — pinned as NOT exempt by the guards in `lifecycle-hooks.ts` (#4839). + * + * Two families, and both matter: + * + * - the **retired dialect** (`roles: ['admin']`), which is what the deleted + * branches actually tested. It has no producer at all — ObjectQL's + * `buildSession()` never writes `roles` — so a test that only proved this + * shape is refused would pass against a guard that had simply been rewired + * to the live vocabulary instead; + * - the **live ADR-0095 vocabulary** (`permissions` / `positions` / derived + * `posture`), i.e. the exact signals `ApprovalService.isOverrideActor` + * reads. These DO resolve on a real request, which is why they are the + * shapes the pin is really for: the maintainer's ruling is that the record + * lock and the delegation guard have no admin override, not that the + * override moved to a better spelling. + */ +const ADMIN_SESSIONS: Array<[string, any]> = [ + ['the retired `roles: [admin]` dialect', { isSystem: false, roles: ['admin'], userId: 'root' }], + ['an ADR-0095 platform admin (admin_full_access)', + { isSystem: false, userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] }], + ['an ADR-0095 platform admin (platform_admin position)', + { isSystem: false, userId: 'root', tenantId: 't1', positions: ['platform_admin'], permissions: [] }], + ['an ADR-0095 tenant admin (org_admin position)', + { isSystem: false, userId: 'root', tenantId: 't1', positions: ['org_admin'], permissions: [] }], + ['an ADR-0095 derived posture (PLATFORM_ADMIN)', + { isSystem: false, userId: 'root', tenantId: 't1', positions: [], permissions: [], posture: 'PLATFORM_ADMIN' }], +]; + /** * The signed-in caller, when it is someone other than {@link CTX}'s `u1`. * An approval action is recorded against the AUTHENTICATED caller (#3800), so a @@ -1834,14 +1864,24 @@ describe('record-lock hook (node era)', () => { ).resolves.toBeUndefined(); }); - it('allows an admin override', async () => { + // ── #4839: there is NO admin exemption on the record lock ───────── + // + // The hook used to bypass on `session.roles.includes('admin')`. `roles` has no + // producer (ObjectQL's `buildSession()` never writes it), so the branch was + // dead and the lock has always applied to admins; the maintainer's ruling + // deletes it rather than reviving it under the ADR-0095 vocabulary. An admin + // releases a locked record through the audited #3424 rescue path + // (recall / reject / reassign), never by editing the record under a live + // approval. Both shapes below are pinned: the dead dialect must not come back, + // and the live privilege vocabulary must not be wired in here instead. + it.each(ADMIN_SESSIONS)('does NOT exempt %s from the record lock', async (_label, session) => { await expect( engine.fire('beforeUpdate', { object: 'opportunity', input: { id: 'opp1', data: { amount: 200 } }, - session: { isSystem: false, roles: ['admin'] }, + session, }), - ).resolves.toBeUndefined(); + ).rejects.toThrow(/RECORD_LOCKED/); }); it('does not lock records without a pending request', async () => { @@ -2067,10 +2107,15 @@ describe('record-lock hook — predicate (multi) updates (#4778)', () => { ).resolves.toBeUndefined(); }); - it.each(SHAPES)('allows an admin override via %s', async (_label, where) => { - await expect( - predicateUpdate(where, { amount: 999 }, { session: { isSystem: false, roles: ['admin'] } }), - ).resolves.toBeUndefined(); + // #4839 — the deny half moves with the guard too: no admin shape is exempt on + // the predicate path either, so a `multi: true` rewrite cannot become the + // admin bypass the by-id path no longer has. + it.each( + SHAPES.flatMap(([sLabel, where]) => + ADMIN_SESSIONS.map(([aLabel, session]) => [`${aLabel} via ${sLabel}`, where, session] as const), + ), + )('does NOT exempt %s', async (_label, where, session) => { + await expect(predicateUpdate(where, { amount: 999 }, { session })).rejects.toThrow(/RECORD_LOCKED/); }); it.each(SHAPES)('allows a status-mirror write via %s', async (_label, where) => { @@ -2427,9 +2472,10 @@ describe('ApprovalService — out-of-office delegation (#1322)', () => { // // sys_approval_delegation is apiEnabled CRUD; a member must not be able to // forge a delegation for someone else (delegator_id = victim) and reroute the -// victim's approvals. The guard forces delegator_id == acting user for normal -// writes; system/admin contexts bypass. Row-ownership on update/delete is the -// platform's created_by RLS (not exercised here). +// victim's approvals. The guard forces delegator_id == acting user for EVERY +// non-system write — since #4839 there is no admin exemption, only the system +// context bypasses. Row-ownership on update/delete is the platform's created_by +// RLS (not exercised here). describe('sys_approval_delegation write guard (#1322)', () => { const DEL = 'sys_approval_delegation'; let engine: ReturnType; @@ -2467,8 +2513,33 @@ describe('sys_approval_delegation write guard (#1322)', () => { await expect(fireInsert({ delegator_id: 'victim', delegate_id: 'u1' }, { isSystem: true })).resolves.toBeUndefined(); }); - it('lets an admin set the delegator to anyone', async () => { - await expect(fireInsert({ delegator_id: 'victim', delegate_id: 'u2' }, { isSystem: false, roles: ['admin'], userId: 'admin1' })).resolves.toBeUndefined(); + // ── #4839: delegation is SELF-MANAGED — no admin exemption ──────── + // + // The guard used to bypass on `roles.includes('admin')` so an admin could + // declare a delegation on someone else's behalf. Evidence decided this + // (maintainer's ruling, point 2): a delegation is consulted only while a + // request is being OPENED (`applyOooDelegation` inside `resolveApproverSpec`), + // so it could never have handled the approvals an unavailable approver is + // ALREADY holding — and for those the sanctioned, `isOverrideActor`-gated + // path exists (`reassign` / `recall` / `decideNode` reject). See + // `admin-exemption-retired.test.ts` for that coverage, executed. + it.each(ADMIN_SESSIONS)('does NOT let %s forge a delegation for someone else', async (_label, session) => { + await expect( + fireInsert({ delegator_id: 'victim', delegate_id: 'u2' }, { ...session, userId: 'admin1' }), + ).rejects.toThrow(/FORBIDDEN/); + }); + + it.each(ADMIN_SESSIONS)('does NOT let %s relabel an existing delegation on update', async (_label, session) => { + await expect( + fireUpdate({ id: 'd1', delegator_id: 'victim' }, { ...session, userId: 'admin1' }), + ).rejects.toThrow(/FORBIDDEN/); + }); + + it('still lets an admin declare their OWN delegation — the guard is about the delegator, not the caller', async () => { + await expect( + fireInsert({ delegator_id: 'admin1', delegate_id: 'u2' }, + { isSystem: false, userId: 'admin1', permissions: ['admin_full_access'], positions: [] }), + ).resolves.toBeUndefined(); }); it('rejects a member relabelling delegator on update', async () => { diff --git a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts index 80d839147d..84eb7f93d0 100644 --- a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts +++ b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts @@ -17,9 +17,28 @@ * 3. Reads the lock policy from each request's `node_config_json` snapshot: * - `lockRecord === false` → allow. * - otherwise block, EXCEPT when the only changed field is the configured - * `approvalStatusField` (so the status mirror is never blocked), the - * caller is an `admin`, or the writer is the run that opened the request - * (`flowRunId`, #3456 / #3712). + * `approvalStatusField` (so the status mirror is never blocked) or the + * writer is the run that opened the request (`flowRunId`, #3456 / #3712). + * + * ## There is no admin exemption — by design (#4839) + * + * The lock applies to a platform/tenant admin exactly as it applies to anyone + * else. An admin who must release a locked record does it through the + * **sanctioned rescue path** (#3424): `ApprovalService.recall` / `decideNode` + * (reject) / `reassign`, all gated by `isOverrideActor` and all audited + * (`via_override`, #4466). Finalising the request is what releases the lock — + * the record is never edited *under* a live approval, which is precisely the + * guarantee a compliance-minded tenant buys the lock for. + * + * This is also not a behaviour change. The hook used to open with + * `if (session.roles?.includes('admin')) return`, but `roles` has no producer: + * ObjectQL's `buildSession()` never writes it, so the branch was dead on every + * real engine path and the lock has always applied to admins. Deleting it makes + * the code say what the runtime does — and closes a second, forbidden admin + * dialect: privilege in this codebase is judged by the ADR-0095 vocabulary + * (`permissions` / `positions` / derived posture), never by a string comparison + * against `'admin'` (ADR-0090 D3 bans the `role` spelling outright). See + * {@link file://./approval-service.ts} `isOverrideActor` for the one predicate. * * ## Step 2 is per-row, and it covers PREDICATE writes (#4778) * @@ -30,8 +49,8 @@ * resolved"* as *"there is nothing to authorize"* when the truth was *"nothing * was ever queried"* — the same fail-open reasoning as #4757 (`sys_attachment`) * and #4630 (`sys_comment`). Rewriting the very same edit as `multi: true` then - * bypassed the lock with **no privilege at all**: no admin role, no `isSystem`, - * no `lockRecord: false`, no whitelisted field. + * bypassed the lock with **no privilege at all**: no `isSystem`, no + * `lockRecord: false`, no whitelisted field. * * So the hook now resolves the row set the way the attachment/comment guards * do, with one difference the record lock forces: it is a **per-row** guard @@ -305,9 +324,10 @@ export function bindApprovalLockHook(engine: MinimalEngine, logger?: MinimalLogg // Allow engine self-writes (status mirror from the approvals service, etc). if ((ctx?.session as any)?.isSystem) return; - // Allow admin override. - const roles = (ctx?.session?.roles ?? []) as string[]; - if (Array.isArray(roles) && roles.includes('admin')) return; + // NOTE (#4839): there is deliberately NO admin exemption here. A privileged + // admin releases a locked record through the audited #3424 rescue path + // (recall / reject / reassign, gated by `isOverrideActor`), not by editing + // the record while its approval is live. See the module docstring. // ── Which rows does this write touch, and which of them are locked? const gating = await gatingRequests(engine, ctx, object); @@ -371,10 +391,34 @@ export const DELEGATION_OBJECT = 'sys_approval_delegation'; * themselves as the delegator: * * - **system** context (service / seed / import) → bypass; - * - **admin** (`roles` includes `'admin'`) → may set `delegator_id` to anyone; * - otherwise `delegator_id` must equal the acting user — an absent delegator * on insert is stamped to the caller, a foreign delegator is rejected. * + * ## Delegation is SELF-MANAGED — there is no admin exemption (#4839) + * + * The guard used to bypass for `session.roles?.includes('admin')`, so an admin + * could declare an out-of-office delegation *on behalf of* another user. That + * branch never ran (`roles` has no producer — ObjectQL's `buildSession()` never + * writes it), and it is not being restored, for two reasons: + * + * 1. **It could not do the job it was written for.** A delegation is consulted + * at *slate-resolution* time only: `applyOooDelegation` runs inside + * `resolveApproverSpec` while a request is being OPENED. Declaring a + * delegation for an approver who has *already* gone unavailable does not + * touch the approvals pending against them — it only redirects future ones. + * 2. **The in-flight case already has a sanctioned path.** For the approvals + * an unavailable approver is *currently* holding, a privileged admin uses + * `ApprovalService.reassign` (hand that approver's slot to a substitute — + * it even carries the slot's per_group membership over), `recall`, or + * `decideNode` with `reject`; all three are gated by `isOverrideActor`'s + * ADR-0095 criteria and audited via `via_override` (#4466). That is the + * operation an admin actually needs, and it exists. + * + * So the semantic is final: **a delegation names its own author as delegator**; + * acting for someone else is done through the approval-level override, in the + * one privilege vocabulary this codebase has (ADR-0090 D3 / ADR-0095 D3) — never + * by comparing a session field against the string `'admin'`. + * * Row-level ownership on update/delete (you can only touch a delegation you * created) is already enforced by `member_default`'s wildcard * `created_by == current_user.id` RLS; this guard adds the delegator-identity @@ -386,8 +430,10 @@ export function bindDelegationWriteGuard(engine: MinimalEngine, logger?: Minimal const makeGuard = (isInsert: boolean) => async (ctx: any) => { const session = (ctx?.session ?? {}) as any; if (session.isSystem) return; // service / seed / import - const roles = (session.roles ?? []) as unknown[]; - if (Array.isArray(roles) && roles.includes('admin')) return; // admin may act for anyone + // NOTE (#4839): no admin exemption — a delegation names its own author as + // delegator. Acting on another user's in-flight approvals is the approval + // service's audited override (reassign / recall / reject), not a forged + // delegation row. See the docstring above. const userId = session.userId != null ? String(session.userId) : ''; const data = ctx?.input?.data; const rows = Array.isArray(data) ? data : (data && typeof data === 'object' ? [data] : []);