From a88b52d7d9072269f8eea2965aa1bd2b3d6d8226 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 01:59:21 +0000 Subject: [PATCH] fix(plugin-security): an organization administrator's update by id is refused before the row is looked up, whatever the id names Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj Co-authored-by: Claude --- ...-org-admin-update-refused-before-lookup.md | 12 +++++++ .../src/delegated-admin-gate.ts | 26 +++----------- ...-admin-assignment-boundary.posture.test.ts | 36 ++++++++++++------- ...ent-removal-unreadable-row.dogfood.test.ts | 18 ++++++---- 4 files changed, 52 insertions(+), 40 deletions(-) create mode 100644 .changeset/30198-org-admin-update-refused-before-lookup.md diff --git a/.changeset/30198-org-admin-update-refused-before-lookup.md b/.changeset/30198-org-admin-update-refused-before-lookup.md new file mode 100644 index 00000000000..c41e9e9778f --- /dev/null +++ b/.changeset/30198-org-admin-update-refused-before-lookup.md @@ -0,0 +1,12 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(plugin-security): an organization administrator's update by id is refused before the row is looked up, whatever the id names + +Clause-②: no +Metadata-protocol: none + +An organization administrator (`organization_admin` or `organization_admin_no_bypass`, holding no `adminScope`) updates an assignment row (`sys_user_position` or `sys_user_permission_set`) by id. An edit in place is never part of that administrator's right, so the write is refused before the row is looked up: `403 PERMISSION_DENIED`, with the same sentence and details for a row, for a row the administrator cannot read, and for an id that names no row. + +Unchanged: a removal by id of a row the administrator cannot read, or of an id that names no row, answers the read door's `404 RECORD_NOT_FOUND`. An administrator who also holds an `adminScope` is judged by that scope. diff --git a/packages/plugins/plugin-security/src/delegated-admin-gate.ts b/packages/plugins/plugin-security/src/delegated-admin-gate.ts index cb514e84ecc..e025d9846fc 100644 --- a/packages/plugins/plugin-security/src/delegated-admin-gate.ts +++ b/packages/plugins/plugin-security/src/delegated-admin-gate.ts @@ -849,11 +849,11 @@ export class DelegatedAdminGate { * * The row a removal targets is judged as the CALLER reads it * ({@link organizationTargets}), so a row the caller cannot read is answered - * exactly as an id naming no row is (ruling of record 5985885287). A by-id - * edit, which the right never admits, is asked the same question first: an - * edit of a row the caller cannot read, or of an id naming none, is refused - * as no row, not as an edit — `unreadable`, which the security middleware - * answers with the read door's not-found. + * exactly as an id naming no row is (ruling of record 5985885287). An edit + * in place is a verb the right never holds, so it is refused before any row + * is looked up, whatever the id names: a row and no row get the same + * refusal (the update-by-id refusal order). The not-found is the answer + * only for a verb the caller holds, judged on the row. */ private async organizationAdminFailure( opCtx: any, @@ -863,12 +863,6 @@ export class DelegatedAdminGate { if (!organizationId) { return { reason: 'the request carries no active organization to administer', meta: {} }; } - if (op === 'update') { - const id = extractSingleId(opCtx); - if (id != null && (await this.hiddenFromCaller(opCtx, id))) { - return { reason: 'the assignment row it addresses could not be read', meta: {}, unreadable: true }; - } - } if (op !== 'insert' && op !== 'delete') { return { reason: `'${op}' is not part of the right — an assignment row is created and removed, never edited in place`, @@ -1669,16 +1663,6 @@ export class DelegatedAdminGate { return [{ next: null, prev, wrote: new Set(), reads }]; } - /** - * Does the caller's own read answer NO row for `id` ({@link callerRead})? - * A declared 4xx refusal of the read is not a hidden row (`false`); a store - * fault propagates. - */ - private async hiddenFromCaller(opCtx: any, id: string | number): Promise { - const read = await this.callerReadUnlessRefused(opCtx, id); - return !read.refused && read.row === null; - } - /** * The caller's own read of row `id` ({@link callerRead}), or `refused` when * the read door refuses it (a declared 4xx). A store fault propagates. diff --git a/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts b/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts index 90c50fdbcef..6826e982464 100644 --- a/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts +++ b/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts @@ -17,10 +17,12 @@ * the failed condition. Every admission reads the stored row back. * * At the foot: under the walled postures, an organization administrator's - * update and delete of a row it cannot read, and of an id naming no row, - * answer the read door's not-found for that id (ruling of record 5985885287) - * — whole, and equal to what step 2.7 answers the tenant-level administrator - * for the same id; a row it reads keeps the boundary guidance. The same rule + * delete of a row it cannot read, and of an id naming no row, answers the + * read door's not-found for that id (ruling of record 5985885287) — whole, and + * equal to what step 2.7 answers the tenant-level administrator for the same + * id; its update, a verb the right never holds, is refused before any row is + * looked up, one refusal whatever the id names; a row it reads keeps the + * boundary guidance. The same rule * for the delegated branch follows: a delegate holding an `adminScope` and an * organization administrator who holds one too are answered the not-found, * and a delegate whose read of the two tables is refused outright is answered @@ -397,10 +399,10 @@ const wholeWrite = (h: Harness, table: Table, op: 'update' | 'delete', id: strin const readDoorNotFound = (table: Table, id: string) => wholeAnswerOf(async () => { throw recordNotFoundError(table, id); }); -describe("[ADR-0090 D12; ruling of record 5985885287] an organization administrator's assignment write to a row it cannot read, or to an id naming none, answers the read door's not-found", () => { +describe("[ADR-0090 D12; ruling of record 5985885287] an organization administrator's assignment removal of a row it cannot read, or of an id naming none, answers the read door's not-found; its edit in place is refused before any row is looked up", () => { for (const posture of ['group', 'isolated'] as const) { describe(`posture '${posture}'`, () => { - it("update and delete of a row the caller cannot read, and of an id naming no row, answer the read door's not-found for that id, whole, as step 2.7 answers the tenant-level administrator — and leave the row", async () => { + it("delete of a row the caller cannot read, and of an id naming no row, answers the read door's not-found for that id, whole, as step 2.7 answers the tenant-level administrator; update, a verb the right never holds, gets one refusal whatever the id names — and the row stays", async () => { const h = await boot(posture); for (const table of Object.keys(TABLES) as Table[]) { const id = `unreadable_${table}`; @@ -417,7 +419,16 @@ describe("[ADR-0090 D12; ruling of record 5985885287] an organization administra for (const caller of ORG_ADMINS) { // The precondition, read off the read door: this caller reads no such row. expect(await readAs(h, table, target, caller), `${caller} reads ${table} ${target}`).toBeFalsy(); - expect(await wholeWrite(h, table, op, target, CALLERS[caller]), `${caller} ${op} ${table} ${target}`).toEqual(notFound); + const answer = await wholeWrite(h, table, op, target, CALLERS[caller]); + if (op === 'delete') { + expect(answer, `${caller} ${op} ${table} ${target}`).toEqual(notFound); + } else { + // An edit in place is never part of the right: refused before the + // row is looked up, so a row and no row answer alike. + expect([answer.status, answer.code], `${caller} ${op} ${table} ${target}: ${answer.message}`).toEqual([403, 'PERMISSION_DENIED']); + expect(answer, `${caller} ${op} ${table} ${target}: the same refusal as for no row`) + .toEqual(await wholeWrite(h, table, op, NOWHERE, CALLERS[caller])); + } } } } @@ -460,7 +471,7 @@ describe("[ADR-0090 D12; ruling of record 5985885287] an organization administra }); } - it("control, 'single': a row of another organization that the caller reads keeps the guidance that names its organization; an id naming no row answers the read door's not-found", async () => { + it("control, 'single': a row of another organization that the caller reads keeps the guidance that names its organization; a removal of an id naming no row answers the read door's not-found, and an edit gets one refusal whatever the id names", async () => { const h = await boot('single'); for (const table of Object.keys(TABLES) as Table[]) { const id = `readable_${table}`; @@ -471,10 +482,11 @@ describe("[ADR-0090 D12; ruling of record 5985885287] an organization administra const row = await write(h, table, 'delete', id, context); expect([row.status, row.code], `${caller} ${table}: ${row.message}`).toEqual([403, 'PERMISSION_DENIED']); expect(row.message).toMatch(/belongs to organization 'org_b'/); - for (const op of ['update', 'delete'] as const) { - expect(await wholeWrite(h, table, op, NOWHERE, context), `${caller} ${op} ${table}, no row`) - .toEqual(await readDoorNotFound(table, NOWHERE)); - } + expect(await wholeWrite(h, table, 'delete', NOWHERE, context), `${caller} delete ${table}, no row`) + .toEqual(await readDoorNotFound(table, NOWHERE)); + const edit = await wholeWrite(h, table, 'update', id, context); + expect([edit.status, edit.code], `${caller} update ${table}: ${edit.message}`).toEqual([403, 'PERMISSION_DENIED']); + expect(await wholeWrite(h, table, 'update', NOWHERE, context), `${caller} update ${table}, no row`).toEqual(edit); } expect(await stored(h, table, { id }), `${table}: refused means untouched`).toHaveLength(1); } diff --git a/packages/qa/dogfood/test/org-admin-assignment-removal-unreadable-row.dogfood.test.ts b/packages/qa/dogfood/test/org-admin-assignment-removal-unreadable-row.dogfood.test.ts index 908cb871d9f..40bb3f011e8 100644 --- a/packages/qa/dogfood/test/org-admin-assignment-removal-unreadable-row.dogfood.test.ts +++ b/packages/qa/dogfood/test/org-admin-assignment-removal-unreadable-row.dogfood.test.ts @@ -12,9 +12,11 @@ // membership earns). Two assignment rows of organization B — B's owner's own // administrator grant (`sys_user_permission_set`) and a position B holds // (`sys_user_position`) — sit behind the wall, so A's owner reads neither. -// For each, and for an id naming no row, `PATCH` and `DELETE` by id answer -// exactly what a `GET` of that id answers — the read door's `404 -// RECORD_NOT_FOUND`, status and body. Both rows stay. +// For each, and for an id naming no row, `DELETE` by id answers exactly what +// a `GET` of that id answers — the read door's `404 RECORD_NOT_FOUND`, status +// and body. `PATCH` is an edit in place, which the right never holds, so it is +// refused before the row is looked up: one `403` for the row and for no row. +// Both rows stay. // // Controls, so a green run is not "every removal is answered alike": // @@ -135,17 +137,19 @@ describe("behind the organization wall, an organization administrator's assignme expect(own.status, `A's own grant — ${JSON.stringify(own.body)}`).toBe(200); }); - it("PATCH and DELETE of a row it cannot read, and of an id naming no row, answer exactly what a GET of that id answers — the read door's not-found — and leave the row", async () => { + it("DELETE of a row it cannot read, and of an id naming no row, answers exactly what a GET of that id answers — the read door's not-found; PATCH, an edit the right never holds, gets one refusal for the row and for no row — and the row stays", async () => { for (const [object, id] of [['sys_user_permission_set', grantB], ['sys_user_position', positionB]]) { const before = await stored(object, id); for (const target of [id, MISSING]) { const readDoor = await call('GET', `/data/${object}/${target}`); expect({ status: readDoor.status, code: codeOf(readDoor) }, `GET ${object} ${target} — ${JSON.stringify(readDoor.body)}`) .toEqual({ status: 404, code: 'RECORD_NOT_FOUND' }); - for (const method of ['PATCH', 'DELETE'] as const) { - expect(await write(method, object, target), `${method} ${object} ${target}: the read door's answer`).toEqual(readDoor); - } + expect(await write('DELETE', object, target), `DELETE ${object} ${target}: the read door's answer`).toEqual(readDoor); } + const hit = await write('PATCH', object, id); + const miss = await write('PATCH', object, MISSING); + expect({ status: hit.status, code: codeOf(hit) }, `PATCH ${object} — ${JSON.stringify(hit.body)}`).toEqual({ status: 403, code: 'PERMISSION_DENIED' }); + expect(withoutId(hit, id), `PATCH ${object}: the row answers as no row`).toEqual(withoutId(miss, MISSING)); expect(await stored(object, id), `${object}: refused writes leave the row as it was`).toEqual(before); } });