Skip to content

Commit 42e1211

Browse files
fix(plugin-security): a delegated administrator's refused assignment write answers the same for a row it cannot read as for a missing one (#29950)
Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:委派管理员(持有 `adminScope` 的调用者,包括同时持有 `adminScope` 的组织管理员)按 id 修改或删除授权行(`sys_user_position`、`sys_user_permission_set`)被拒时,对它读不到的行和根本不存在的 id 给出完全相同的回答(状态码、错误码、文案、details 全同);委派管理闸门先以调用者自己的读取取得这一行,读得到的行仍按存储行判定。 - 为什么改:落实 ruling 5985885287(调用者读不到的行与不存在的 id 回答一致,对所有主体类别都适用);组织管理员分支已由 #29932 落地,本 PR 把这条规则落到委派分支,以及“组织管理员兼持 adminScope”这一类调用者。 - 风险与回滚:只改委派分支判断“有没有这一行”的读取来源;读得到的行判定与提示不变,原本放行的写入仍放行(含对该表无读权限的委派者),插入、租户级管理员、不持 adminScope 的组织管理员都不经此改动;唯一可见变化是不存在的 id 的删除由引擎的 404 改为闸门的 403(一种拒绝换成另一种)。回滚即撤销本 PR。 - 先看:`packages/plugins/plugin-security/src/delegated-admin-gate.ts` 的 `delegatedTargets` 与 `hiddenFromCaller`(风险 hunk:读取被拒绝不算“读不到”,仍按存储行判);`security-plugin.ts` 中 `readAsCaller` 的接线;`org-admin-assignment-boundary.posture.test.ts` 末尾新增的 describe。 - 两分钟验证:`pnpm --filter @objectstack/plugin-security exec vitest run src/org-admin-assignment-boundary.posture.test.ts`,预期 70/70 通过(含 `group` / `isolated` 下三类委派调用者的新用例与 `single` 对照)。 - 需要你做的:无,席位落地。 ## What changes - **The delegated walk asks the caller first.** `DelegatedAdminGate` (ADR-0090 D12) judges a delegate's by-id update or delete of an assignment row (`sys_user_position`, `sys_user_permission_set`) only once the caller's own by-id read of that row — the read door's answer to this caller, every read visibility and wall included — has returned it. The read is #29932's organization-branch read, now one method (`callerRead`) both branches use. - **No row is one answer, given at the gate.** When the caller's read returns no row — the id names no row, or the caller cannot read it — the write is refused there: `403 PERMISSION_DENIED`, "the assignment row it addresses could not be read", details `{ operation, object }`. Neither case reaches the engine, so a removal of an id that names no row now answers that refusal instead of `404 RECORD_NOT_FOUND`. - **A row the caller reads is judged on the stored row, as before**, so its verdict and its guidance are unchanged. The caller's own read is served through its field-level security, and a judgement made on what a field rule hid would be made on less than the row; the posture rig pins this. - **A read the read door refuses outright (a declared 4xx) hides no row.** The stored row is judged as before, and a refusal the gate then answers for the by-id write the caller addressed is answered with that read refusal by the security middleware (`readDoorAnswerForGateRefusal`), for every id alike — the same reading the by-id write door gives such a caller. A store fault in either read propagates as raised. - **One caller read per operation.** The security plugin passes the gate its per-operation pre-image read (`readAsCaller` → `getCallerPreImage`), so the organization branch, the delegated walk and the read-door questions after them share one read of the row. - Changeset: `@objectstack/plugin-security` patch. ## Tests - **Engine level, real `ObjectQL` + SQL driver + `SecurityPlugin`** (`org-admin-assignment-boundary.posture.test.ts`, new top-level describe; the harness now also registers the two business-unit objects): - under `group` and `isolated`, for three callers — a delegate, an organization administrator who also holds the scope, and a delegate refused the read of both tables — and both tables, the update and the delete of a row the caller cannot read answer exactly what the same write answers for an id that names no row, compared whole (status, code, sentence, details); the no-row answer is the gate's 403, never the engine's; the row stays; - control: a row inside the scope that the caller reads is admitted; a row of its own organization outside the scope keeps the guidance naming its unit or its holder and is not answered as no row, after one read of the row as the caller; an assignment whose position a field rule hides from the caller is still held to the allowlist; - control, `single`: a row the delegate reads keeps its guidance, and every in-scope update and delete lands — the delegate refused the read included — reading the row as the caller once. - **HTTP door** (`packages/qa/dogfood/test/delegated-admin-assignment-write-unreadable-row.dogfood.test.ts`, showcase behind a real `isolated` wall, the showcase's own `showcase_field_ops_delegate` set): for a delegate and for an organization owner who also holds the set, `PATCH` and `DELETE` of an assignment row the caller cannot read answer exactly what an id naming no row answers; the row stays. Controls: the wall is real, each caller reads no such row; a row inside the scope that the delegate wrote is admitted; a row outside it keeps the guidance naming its unit. - **One-shot ablations**, not kept (each restored blob == HEAD blob, `git diff HEAD` empty): - engine, the caller-visibility question removed (`node scripts/ablation-replace.mjs`, anchor hit 1 time, blob `78e3ddaf` → `47eb74de`): posture rig 2 failed | 68 passed — the two walled "answers exactly what an id naming no row answers" cases; every control green; - engine, the shared-read wiring removed (blob `4e1045f1` → `e13d4015`): 3 failed | 67 passed — the three cases that count the caller's reads of the row (2, not 1); - HTTP, the visibility question made to answer "readable" always (blob `78e3ddaf` → `35c1210c`), plugin-security rebuilt, `node scripts/ablation-dist-preflight.mjs @objectstack/plugin-security PLANTED_TEXT` exit 0 (hit `dist/index.js`, `dist/index.mjs`): dogfood pin 1 failed | 2 passed — the unreadable-row case; restored, rebuilt, preflight `--absent` exit 0, 3/3. ## Acceptance notes - **Why `Clause-②: no`.** The writes this newly refuses are by-id updates and deletes whose row the caller's read does not return; each was refused before too. An id naming no row was refused by the gate or answered `404` by the engine. A row the caller cannot read was refused by the gate or answered `404 RECORD_NOT_FOUND` by the by-id write door's own read question — measured on the real engine under `group` and `isolated` for rows whose stored image is inside the scope (before: 404, row untouched; after: the gate's 403, row untouched). Every admission stays: in-scope rows the caller reads are admitted under all three postures, and a delegate refused the read keeps its in-scope writes under `single` (pinned). - The posture rig's `test-budget: 20` header was written at 60 cases; the file now holds 70, and its tests took 8.9 s here on a shared box, inside the budget. - `delegated-admin-gate.test.ts`'s harness reads through a fake `ql` that ignores context, so its by-id cases stand for a caller who reads the row; their expectations are unchanged. ## Verification <details><summary>Final head d57b423 (main merged at 098261a, clean)</summary> - Every command `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derives (5 paths vs merge base 21510bb): 65 commands, each exit 0, captured before any pipe; `--ran` reconciliation: 65 derived, 65 run, 0 NOT-MEASURED (derived from the recorded exit codes). `check:dual-build-cjs-loads` and `check:type-check-debt` ran with the lock held and the dist they read built. - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 src/org-admin-assignment-boundary.posture.test.ts`: 70/70. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0 (tsc, scripts tsc, test layer 0 errors). - `pnpm --filter @objectstack/dogfood test` (full battery, once, under the verify lock): 415 files passed, 3 skipped; 3430 tests passed, 28 skipped; command-exit 0. </details> <details><summary>At 098261a (the main merge; the one later commit types two test reads)</summary> - `pnpm turbo run build --filter=@objectstack/dogfood --concurrency=2`: exit 0, 68 tasks. - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2` (full suite): 242 files, 5266 passed, 45 skipped. - `pnpm --filter @objectstack/plugin-security typecheck`, `pnpm --filter @objectstack/dogfood typecheck`: exit 0 each. - `pnpm --filter @objectstack/dogfood exec vitest run --project isolated` on this PR's pin and the organization-branch pin beside it: 2 files, 6/6. - First gate pass here: `check:query-options-erasure` exit 1 (test surface 236 to 238, the two new reads cast to any; typed in d57b423, then exit 0) and `check:dual-build-cjs-loads` exit 3 (prerequisite: seven packages outside the dogfood closure unbuilt; built, then exit 0). The whole derived set was then re-run on d57b423 (above). </details> Authored in session `session_01QSeKYn5CmmdShCJifm6WYj` (security#1 dispatch, no card). --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 82f60eb commit 42e1211

5 files changed

Lines changed: 513 additions & 21 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
fix(plugin-security): a delegated administrator's refused assignment write answers the same for a row it cannot read as for a missing one
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
A delegated administrator — a caller holding an `adminScope`, an organization administrator who holds one included — updates or removes an assignment row (`sys_user_position` or `sys_user_permission_set`) by id. The delegated-administration gate (ADR-0090 D12) now asks first whether the caller itself reads that row, in its own context.
11+
12+
- **A row the caller cannot read** is refused with exactly the answer an id that names no row gets: `403 PERMISSION_DENIED`, "the assignment row it addresses could not be read", with the same details. Both are answered by the gate; an id that names no row is no longer handed on, so a removal of one answers that refusal instead of `404 RECORD_NOT_FOUND`.
13+
- **A row the caller reads** keeps the verdict and the guidance it had: the gate judges the stored row, as before.
14+
- **A caller the read door refuses outright** (no read grant on the table) is answered as before: the read refusal, for every id the gate refuses, and the writes it was admitted are still admitted.
15+
- **Unchanged.** Inserts, tenant-level administrators, and organization administrators who hold no scope are judged as before.

‎packages/plugins/plugin-security/src/delegated-admin-gate.ts‎

Lines changed: 86 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
*/
5757

5858
import { isGrantActive, securityCatalogReaderOf, type SecurityCatalogReader } from '@objectstack/core';
59+
import { declaredHttpStatus } from '@objectstack/types';
5960
import type { AdminScope, AdminScopeParsed, PermissionSet } from '@objectstack/spec/security';
6061
import {
6162
ADMIN_FULL_ACCESS,
@@ -224,6 +225,14 @@ export interface DelegatedAdminGateDeps {
224225
ql: any;
225226
/** Shared permission-set resolution (same path as the CRUD middleware). */
226227
resolveSets: (context: any) => Promise<PermissionSet[]>;
228+
/**
229+
* [ruling of record 5985885287] The caller's own by-id read of the row an
230+
* update or delete addresses: the read door's answer to this caller. The
231+
* security plugin passes its per-operation pre-image read, so the gate and
232+
* the write gates after it read the row once between them. Absent, the gate
233+
* reads through `ql` under the caller's context.
234+
*/
235+
readAsCaller?: (opCtx: any, id: string | number) => Promise<Record<string, unknown> | null>;
227236
logger?: { warn?: (msg: string, meta?: any) => void };
228237
/** [ADR-0091 D3] Clock for delegation validity/ceiling checks (tests inject). */
229238
now?: () => number;
@@ -843,11 +852,26 @@ export class DelegatedAdminGate {
843852
private async organizationTargets(opCtx: any): Promise<Array<{ next: any | null; prev: any | null }>> {
844853
if (opCtx.operation === 'insert') return rowsOf(opCtx).map((r) => ({ next: r, prev: null }));
845854
const id = extractSingleId(opCtx);
855+
return [{ next: null, prev: id != null ? await this.callerRead(opCtx, id) : null }];
856+
}
857+
858+
/**
859+
* [ruling of record 5985885287] The caller's own by-id read of row `id` of
860+
* `opCtx.object`: the read door's answer to this caller, every read
861+
* visibility and wall included — through
862+
* {@link DelegatedAdminGateDeps.readAsCaller} when the host shares its
863+
* per-operation pre-image, else through the engine under the caller's
864+
* context. `null` for no row. A read refusal or a store fault propagates as
865+
* raised.
866+
*/
867+
private async callerRead(opCtx: any, id: string | number): Promise<any | null> {
846868
const ql = this.deps.ql;
847-
const prev = id != null && ql?.findOne
848-
? await ql.findOne(opCtx.object, { where: { id }, context: opCtx.context })
849-
: null;
850-
return [{ next: null, prev: prev && typeof prev === 'object' ? prev : null }];
869+
const row = this.deps.readAsCaller
870+
? await this.deps.readAsCaller(opCtx, id)
871+
: ql?.findOne
872+
? await ql.findOne(opCtx.object, { where: { id }, context: opCtx.context })
873+
: null;
874+
return row && typeof row === 'object' ? row : null;
851875
}
852876

853877
/**
@@ -1169,7 +1193,7 @@ export class DelegatedAdminGate {
11691193
// ── sys_user_position: user ↔ position assignments ──────────────────
11701194

11711195
private async assertAssignmentWrite(opCtx: any, held: HeldScope[]): Promise<void> {
1172-
const targets = await this.materializeTargets(opCtx, 'sys_user_position');
1196+
const targets = await this.delegatedTargets(opCtx);
11731197
for (const t of targets) {
11741198
const buId = t.next?.business_unit_id ?? null;
11751199
const positionName = String(t.next?.position ?? t.prev?.position ?? '');
@@ -1213,7 +1237,7 @@ export class DelegatedAdminGate {
12131237
// ── sys_user_permission_set: direct user ↔ set grants ───────────────
12141238

12151239
private async assertDirectGrantWrite(opCtx: any, held: HeldScope[]): Promise<void> {
1216-
const targets = await this.materializeTargets(opCtx, 'sys_user_permission_set');
1240+
const targets = await this.delegatedTargets(opCtx);
12171241
for (const t of targets) {
12181242
const row = t.next ?? t.prev ?? {};
12191243
const { setRow, setName, unresolved } = await this.directGrantSet(t, row);
@@ -1457,30 +1481,76 @@ export class DelegatedAdminGate {
14571481
return allowed;
14581482
}
14591483

1460-
/** Rows targeted by this write: `{ next, prev }` per row (prev = pre-image on update/delete). */
1461-
private async materializeTargets(opCtx: any, object: string): Promise<Array<{ next: any | null; prev: any | null }>> {
1484+
/**
1485+
* [ADR-0090 D12; ruling of record 5985885287] The rows a delegate's write
1486+
* targets: `{ next, prev }` per row — an insert's payload rows, or the one row
1487+
* a by-id write addresses (`authorize` refused a write naming no single row
1488+
* before the walk), `prev` its pre-image and an update's `next` its payload
1489+
* over that pre-image.
1490+
*
1491+
* Whether there is a row to judge is asked of the CALLER's own read
1492+
* ({@link callerRead}): a row the caller cannot read answers exactly what an
1493+
* id naming no row answers — one refusal, here at the gate, never a
1494+
* fall-through to the engine. A row the caller reads is judged on the stored
1495+
* row, as before, never on the caller's read of it: that read is served
1496+
* through the caller's field-level security, and a judgement made on what a
1497+
* field rule hid would be made on less than the row. So a row the caller
1498+
* reads keeps its verdict and its guidance.
1499+
*
1500+
* A read the read door REFUSES (a declared 4xx — the object's read grant
1501+
* withheld) hides no row: the stored row is judged as before, and a refusal
1502+
* the gate then answers for the by-id write the caller addressed is answered
1503+
* with that read refusal by the security middleware
1504+
* (`readDoorAnswerForGateRefusal`), for every id alike. A store fault, in
1505+
* either read, propagates as raised: an outage is neither an answer nor a
1506+
* refusal.
1507+
*/
1508+
private async delegatedTargets(opCtx: any): Promise<Array<{ next: any | null; prev: any | null }>> {
14621509
const op = opCtx.operation;
14631510
const payload = rowsOf(opCtx);
14641511
if (op === 'insert') return payload.map((r) => ({ next: r, prev: null }));
14651512

1466-
const id = extractSingleId(opCtx);
1467-
let prev: any = null;
1468-
if (id != null && this.deps.ql?.findOne) {
1469-
prev = await this.deps.ql
1470-
.findOne(object, { where: { id }, context: SYSTEM_CTX })
1471-
.catch(() => null);
1513+
const id = extractSingleId(opCtx) as string | number;
1514+
const prev = (await this.hiddenFromCaller(opCtx, id)) ? null : await this.storedRow(opCtx.object, id);
1515+
if (!prev) {
1516+
throw new PermissionDeniedError(
1517+
`[Security] Access denied: delegated '${op}' on ${opCtx.object} rejected — the assignment row it addresses could not be read.`,
1518+
{ operation: op, object: opCtx.object },
1519+
);
14721520
}
14731521
if (op === 'update') {
14741522
const next = payload.length > 0 ? payload[0] : null;
14751523
// Merge unchanged pre-image values so boundary checks see the full row
14761524
// (an update payload may carry only the changed columns).
1477-
const merged = next && prev ? { ...prev, ...next } : next ?? prev;
1478-
return [{ next: merged, prev }];
1525+
return [{ next: next ? { ...prev, ...next } : prev, prev }];
14791526
}
14801527
// delete / transfer / restore / purge — judged on the pre-image alone.
14811528
return [{ next: null, prev }];
14821529
}
14831530

1531+
/**
1532+
* Does the caller's own read answer NO row for `id` ({@link callerRead})?
1533+
* A declared 4xx refusal of the read is not a hidden row (`false`); a store
1534+
* fault propagates.
1535+
*/
1536+
private async hiddenFromCaller(opCtx: any, id: string | number): Promise<boolean> {
1537+
try {
1538+
return (await this.callerRead(opCtx, id)) === null;
1539+
} catch (e) {
1540+
const status = declaredHttpStatus(e);
1541+
if (status !== undefined && status < 500) return false;
1542+
throw e;
1543+
}
1544+
}
1545+
1546+
/** The stored row `id` of `object`, read by the system; `null` for no row. A store fault propagates. */
1547+
private async storedRow(object: string, id: string | number): Promise<any | null> {
1548+
const ql = this.deps.ql;
1549+
if (!ql?.findOne) return null;
1550+
const row = await ql.findOne(object, { where: { id }, context: SYSTEM_CTX });
1551+
return row && typeof row === 'object' ? row : null;
1552+
}
1553+
14841554
/**
14851555
* [ADR-0131 D3/D4, ADR-0090 D12 as amended 2026-10-17] The permission sets a
14861556
* position distributes, as the delegated walk judges them: every set an

0 commit comments

Comments
 (0)