fix(plugin-security): an organization administrator's update by id is refused before the row is looked up, whatever the id names

.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.

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<string>(), 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<boolean> {
-    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.

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);
     }

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);
     }
   });