From 48e239fe4dbcf05abb977e61c06b597499abfd92 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 22:30:14 +0000 Subject: [PATCH 1/2] test(plugin-security): explain's verdict per write verb on a controlled_by_parent record, beside each verb's door Enumerates every operation security/explain answers against step 2.8 of the write path, at three layers: the engine over a deps bag, the registered service beside the middleware's by-id write of each verb, and the REST explain route beside each verb's REST door (PATCH, DELETE, and the PATCH that writes owner_id) for the master's editor and a non-editor. Red on this commit for delete and transfer; the fix follows. Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- .../controlled-by-parent-write-member.test.ts | 115 +++++++- ...explain-controlled-by-parent-write.test.ts | 253 +++++++++++------- .../cbp-explain-master-write.dogfood.test.ts | 172 +++++++++++- 3 files changed, 436 insertions(+), 104 deletions(-) diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts index 9a72cc78d82..7f5093bf8f8 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts @@ -22,6 +22,12 @@ // derivation, so step 2.7 answers not-found first), both faces still refuse — // the member answers the master check alone, which the contract says. // +// The last block runs `security/explain` for EVERY by-id write verb step 2.8 +// runs on (update, delete, transfer) beside that verb's own door through the +// same middleware: explain asks this one member for each, because the check +// judges EDIT of the master whatever the record's verb. A verb whose door's +// master check stopped matching the member's update answer turns it red. +// // ## The fixture // // cbpm_account public_read + owner_id — read by all, edited by its owner @@ -41,7 +47,7 @@ import { describe, it, expect, vi } from 'vitest'; import { SecurityPlugin } from './security-plugin.js'; import { SharingService, type SharingEngine } from '@objectstack/plugin-sharing'; import { matchesFilterCondition } from '@objectstack/formula'; -import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/security'; +import { ExplainOperationSchema, PermissionSetSchema, type ExplainOperation, type PermissionSet } from '@objectstack/spec/security'; import type { ControlledByParentWriteOutcome, ISecurityService } from '@objectstack/spec/contracts'; import { assertEngineFindOnePredicate } from '@objectstack/metadata-core'; @@ -98,7 +104,9 @@ const SCHEMAS: Record = { sys_user: { name: 'sys_user', isSystem: true, fields: { id: text('id'), name: text('name') } }, }; -const CRUD = { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }; +// `allowTransfer` lets a transfer through the object gate, so its record-level +// gates (step 2.7, step 2.8) are what answer it. +const CRUD = { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, allowTransfer: true }; const READ = { allowRead: true }; const DETAILS = ['cbpm_contact', 'cbpm_region_note', 'cbpm_quote', 'cbpm_line', 'cbpm_orphan']; @@ -247,15 +255,37 @@ async function boot(options: { faultOn?: string } = {}) { return e; } }; + /** + * A by-id write of any verb through the engine middleware, shaped as the + * write path builds it (a delete or a purge carries no data). Resolves + * `'admitted'` or the refusal it threw. + */ + const write = async (operation: string, context: any, object: string, id: string): Promise<'admitted' | any> => { + const opCtx: any = { + object, + operation, + ...(operation === 'delete' || operation === 'purge' ? {} : { data: { id, name: 'edited' } }), + options: { where: { id } }, + context, + }; + try { + await middleware(opCtx, async () => {}); + return 'admitted'; + } catch (e) { + return e; + } + }; const member = (context: any, object: string, id: string) => serve(object, id, context); /** * [ADR-0055] `security/explain`'s record-grained update verdict through the * registered service (`explainAccessForCaller`). `userId` explains another * user, as the REST route does; the caller is the second argument. */ + const explainAs = (operation: ExplainOperation, callerContext: any, object: string, recordId: string, userId?: string) => + security.explain({ object, operation, recordId, ...(userId ? { userId } : {}) }, callerContext); const explainUpdate = (callerContext: any, object: string, recordId: string, userId?: string) => - security.explain({ object, operation: 'update', recordId, ...(userId ? { userId } : {}) }, callerContext); - return { store, ctx, patch, member, explainUpdate }; + explainAs('update', callerContext, object, recordId, userId); + return { store, ctx, patch, write, member, explainAs, explainUpdate }; } const rep = () => ({ userId: REP, tenantId: 'org-1', positions: [], permissions: ['cbpm_rep'] }); @@ -419,3 +449,80 @@ describe('[ADR-0055] security/explain answers an update as the PATCH does, from expect(sharingDetailOf(d)).toContain("refuses this update on its 'object_permission' leg"); }); }); + +/** + * Every operation `security/explain` answers, classified against step 2.8 of + * the write path. Total by construction, and checked against the spec's + * vocabulary below, so a verb added there without a row here fails. + * + * - `by_id_master_checked`: a by-id write the object gate lets through for a + * principal holding the grant, on which step 2.8 runs the master check. + * - `object_gate_refused`: step 2.8 lists it, but the object gate refuses it + * to every principal (its grant is retired until the lifecycle batch). + * - `no_by_id_write`: a read, or an insert, whose master step 2.8 reads off + * the request body an explanation does not carry. + */ +const VERB_ROWS: Record = { + update: 'by_id_master_checked', + delete: 'by_id_master_checked', + transfer: 'by_id_master_checked', + restore: 'object_gate_refused', + purge: 'object_gate_refused', + create: 'no_by_id_write', + read: 'no_by_id_write', + export: 'no_by_id_write', +}; +const verbsOf = (row: (typeof VERB_ROWS)[ExplainOperation]) => + (Object.keys(VERB_ROWS) as ExplainOperation[]).filter((v) => VERB_ROWS[v] === row); + +describe('[ADR-0055] security/explain answers every by-id write verb as its door does, from this same check', () => { + const sharingDetailOf = (d: any): string => String(d.layers.find((l: any) => l.layer === 'sharing')?.record?.detail ?? ''); + + it('the classification is total: every operation explain answers has a row', () => { + expect([...ExplainOperationSchema.options].sort()).toEqual(Object.keys(VERB_ROWS).sort()); + }); + + for (const verb of verbsOf('by_id_master_checked')) { + it(`${verb}: refused on the leg its door refuses on; admitted where its door admits`, async () => { + const h = await boot(); + for (const [context, object, id, leg] of [ + [rep(), 'cbpm_contact', 'c_other', 'record_sharing'], + [rep(), 'cbpm_region_note', 'rn_eu', 'row_level_security'], + [viewer(), 'cbpm_contact', 'c_own', 'object_permission'], + [rep(), 'cbpm_line', 'line_above_dangling', 'master_chain'], + ] as const) { + const refused = await h.write(verb, context, object, id); + expect(refused, `${verb} ${object}/${id} by ${context.userId}`).not.toBe('admitted'); + expect(refused).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(String(refused.message)).toMatch(/requires edit access to its master record/); + if (LEG_REASON[leg]) expect(String(refused.message)).toMatch(LEG_REASON[leg]); + const d = await h.explainAs(verb, context, object, id); + expect(d.record, `explain ${verb} ${object}/${id} by ${context.userId}`).toEqual({ + recordId: id, + visible: false, + decidedBy: 'sharing', + }); + expect(sharingDetailOf(d)).toContain(`refuses this ${verb} on its '${leg}' leg`); + } + for (const [object, id] of [['cbpm_contact', 'c_own'], ['cbpm_contact', 'c_shared'], ['cbpm_region_note', 'rn_us'], ['cbpm_line', 'line_ok']] as const) { + const d = await h.explainAs(verb, rep(), object, id); + expect(d.record, `explain ${verb} ${object}/${id}`).toMatchObject({ recordId: id, visible: true }); + expect(sharingDetailOf(d)).not.toContain('master-detail write check'); + expect(await h.write(verb, rep(), object, id), `${verb} ${object}/${id}`).toBe('admitted'); + } + }); + } + + for (const verb of verbsOf('object_gate_refused')) { + it(`${verb}: the object gate refuses its door for every principal, and explain says the object gate decided`, async () => { + const h = await boot(); + for (const [context, object, id] of [[rep(), 'cbpm_contact', 'c_own'], [rep(), 'cbpm_contact', 'c_other']] as const) { + const refused = await h.write(verb, context, object, id); + expect(refused, `${verb} ${object}/${id}`).not.toBe('admitted'); + expect(String(refused.message)).not.toMatch(/requires edit access to its master record/); + const d = await h.explainAs(verb, context, object, id); + expect(d.record, `explain ${verb} ${object}/${id}`).toEqual({ recordId: id, visible: false, decidedBy: 'object_crud' }); + } + }); + } +}); diff --git a/packages/plugins/plugin-security/src/explain-controlled-by-parent-write.test.ts b/packages/plugins/plugin-security/src/explain-controlled-by-parent-write.test.ts index 67346e5d874..84e7c26d39f 100644 --- a/packages/plugins/plugin-security/src/explain-controlled-by-parent-write.test.ts +++ b/packages/plugins/plugin-security/src/explain-controlled-by-parent-write.test.ts @@ -1,18 +1,21 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. // -// [ADR-0055] `security/explain`'s record-grained UPDATE verdict on a -// `controlled_by_parent` record comes from the master-detail write check +// [ADR-0055] `security/explain`'s record-grained verdict for every by-id WRITE +// of a `controlled_by_parent` record (an update, a delete, a transfer) comes +// from the master-detail write check // (`ISecurityService.checkControlledByParentWrite`), the composition step 2.8 -// of the write path runs, and every outcome maps the way the door maps it. +// of the write path runs on each of them, and every outcome maps the way the +// door maps it. // -// The per-record sharing gate (`canEditRecord`) cannot decide such an update: -// plugin-sharing maps `controlled_by_parent` to `public`, so it abstains, and -// its abstention reads as `true`. This file pins the engine's half — which -// outcome refuses, what the report names, when the check is asked and with -// which context — over a deps bag, so each cell isolates one outcome. The -// served member's own parity with the PATCH is pinned in +// The per-record sharing gates (`canEditRecord`, `canDeleteRecord`) cannot +// decide such a write: plugin-sharing maps `controlled_by_parent` to `public`, +// so they abstain, and an abstention reads as `true`. This file pins the +// engine's half — which outcome refuses, what the report names, which verbs +// ask the check and with which context — over a deps bag, so each cell isolates +// one outcome. The served member's own parity with each by-id write is pinned in // `controlled-by-parent-write-member.test.ts`, and the whole REST answer beside -// the REST PATCH in `packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts`. +// the REST door of each verb in +// `packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts`. // // The mapping (the spec's own consumer rule): `allow` and `not_applicable` // proceed, so the report is exactly the one a deps bag without the check @@ -22,7 +25,7 @@ // refuses too. import { describe, it, expect, vi } from 'vitest'; import { PermissionSetSchema } from '@objectstack/spec/security'; -import type { ExplainDecision, ExplainOperation } from '@objectstack/spec/security'; +import { ExplainOperationSchema, type ExplainDecision, type ExplainOperation } from '@objectstack/spec/security'; import type { ControlledByParentWriteDenialLeg, ControlledByParentWriteOutcome, @@ -36,7 +39,7 @@ const CBP_SCHEMA = { name: OBJECT, sharingModel: 'controlled_by_parent' }; const EDITOR = PermissionSetSchema.parse({ name: 'cbx_editor', - objects: { [OBJECT]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + objects: { [OBJECT]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, allowTransfer: true } }, }); /** The principal being EXPLAINED. Kept as one object so a cell can assert the check received this very context. */ @@ -80,8 +83,37 @@ function deps(opts: { const answering = (outcome: ControlledByParentWriteOutcome): Master => vi.fn(async () => outcome); /** `null` asks the object-level question (no `recordId`). */ -const explainUpdate = (d: ExplainEngineDeps, recordId: string | null = 'r1') => - explainAccess(d, { object: OBJECT, operation: 'update', context: EXPLAINED, ...(recordId !== null ? { recordId } : {}) }); +const explainAs = (d: ExplainEngineDeps, operation: ExplainOperation, recordId: string | null = 'r1') => + explainAccess(d, { object: OBJECT, operation, context: EXPLAINED, ...(recordId !== null ? { recordId } : {}) }); + +/** + * Every operation `explain` answers, classified against step 2.8 of the write + * path. The classification is total: a verb added to the explain vocabulary + * without a row here fails the completeness case below. + * + * - `master_checked`: a by-id write step 2.8 runs the master-detail write + * check on, which the EXPLAINED principal's grants let through the object + * gate (`EDITOR` holds update, delete and transfer), so the check decides + * the record. + * - `master_checked_object_gate_first`: a by-id write step 2.8 lists, which + * the object gate refuses for every principal (the restore and purge grants + * are retired until the lifecycle batch returns them). The check is still + * asked, so the report stays the door's on the day the verb is grantable, + * and the object-level CRUD layer decides first. + * - `not_asked`: a read (the check guards writes), and `create`, whose master + * comes from the request body an explanation does not carry. + */ +const VERB_CLASS: Record = { + update: 'master_checked', + delete: 'master_checked', + transfer: 'master_checked', + restore: 'master_checked_object_gate_first', + purge: 'master_checked_object_gate_first', + read: 'not_asked', + export: 'not_asked', + create: 'not_asked', +}; +const MASTER_CHECKED = (Object.keys(VERB_CLASS) as ExplainOperation[]).filter((v) => VERB_CLASS[v] === 'master_checked'); const sharingRecordOf = (d: ExplainDecision) => d.layers.find((l) => l.layer === 'sharing')?.record; @@ -92,114 +124,141 @@ const REASONS: Array<[ControlledByParentWriteUnresolvedReason, string]> = [ ['master_reference_missing', '422 MISSING_REQUIRED_FIELD'], ]; -describe('[ADR-0055] explain refuses an update the master-detail write check refuses', () => { - for (const leg of LEGS) { - it(`deny on '${leg}': the record is not writable, decided by the sharing layer, which names the leg`, async () => { - const d = await explainUpdate(deps({ master: answering({ outcome: 'deny', leg }) })); - expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); - const sharing = sharingRecordOf(d); - expect(sharing?.outcome).toBe('excluded'); - expect(sharing?.detail).toContain(`refuses this update on its '${leg}' leg`); - expect(sharing?.detail).toContain('403 PERMISSION_DENIED'); - }); - } +describe('[ADR-0055] explain refuses a by-id write the master-detail write check refuses, for every verb it decides', () => { + for (const verb of MASTER_CHECKED) { + for (const leg of LEGS) { + it(`${verb}, deny on '${leg}': the record is not writable, decided by the sharing layer, which names the leg`, async () => { + const d = await explainAs(deps({ master: answering({ outcome: 'deny', leg }) }), verb); + expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); + const sharing = sharingRecordOf(d); + expect(sharing?.outcome).toBe('excluded'); + expect(sharing?.detail).toContain(`the by-id ${verb} runs refuses this ${verb} on its '${leg}' leg`); + expect(sharing?.detail).toContain(`The ${verb} answers 403 PERMISSION_DENIED`); + }); + } + + for (const [reason, answered] of REASONS) { + it(`${verb}, unresolvable '${reason}': not writable (fail closed), the sharing layer names the reason and the ${verb}'s answer`, async () => { + const d = await explainAs(deps({ master: answering({ outcome: 'unresolvable', reason }) }), verb); + expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); + const sharing = sharingRecordOf(d); + expect(sharing?.outcome).toBe('not_evaluated'); + expect(sharing?.detail).toContain(`reaches no verdict ('${reason}')`); + expect(sharing?.detail).toContain(`the ${verb} answers ${answered}`); + }); + } - for (const [reason, answered] of REASONS) { - it(`unresolvable '${reason}': not writable (fail closed), the sharing layer names the reason and the update's answer`, async () => { - const d = await explainUpdate(deps({ master: answering({ outcome: 'unresolvable', reason }) })); + it(`${verb}: a rejection (a refused context, a store fault) fails the request, so it is reported fail-closed with no predicate`, async () => { + const fault = Object.assign(new Error('datasource unavailable'), { code: 'ERR_DATASOURCE_UNAVAILABLE', status: 503 }); + const master: Master = vi.fn(async () => { throw fault; }); + const d = await explainAs(deps({ master }), verb); expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); const sharing = sharingRecordOf(d); expect(sharing?.outcome).toBe('not_evaluated'); - expect(sharing?.detail).toContain(`reaches no verdict ('${reason}')`); - expect(sharing?.detail).toContain(answered); + expect(sharing?.rowFilter).toBeUndefined(); + expect(sharing?.matchesRecord).toBeUndefined(); + expect(sharing?.detail).toContain('master-detail write check (controlled_by_parent, ADR-0055) could not be evaluated'); + expect(sharing?.detail).toContain(`the by-id ${verb} fails on the same call`); }); - } - it('a rejection (a refused context, a store fault) fails the request, so it is reported fail-closed with no predicate', async () => { - const fault = Object.assign(new Error('datasource unavailable'), { code: 'ERR_DATASOURCE_UNAVAILABLE', status: 503 }); - const master: Master = vi.fn(async () => { throw fault; }); - const d = await explainUpdate(deps({ master })); - expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); - const sharing = sharingRecordOf(d); - expect(sharing?.outcome).toBe('not_evaluated'); - expect(sharing?.rowFilter).toBeUndefined(); - expect(sharing?.matchesRecord).toBeUndefined(); - expect(sharing?.detail).toContain('master-detail write check (controlled_by_parent, ADR-0055) could not be evaluated'); - }); - - it('an outcome outside the vocabulary refuses (fail closed), never reads as a pass', async () => { - const d = await explainUpdate(deps({ master: answering({ outcome: 'maybe' } as unknown as ControlledByParentWriteOutcome) })); - expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); - expect(sharingRecordOf(d)?.outcome).toBe('not_evaluated'); - }); + it(`${verb}: an outcome outside the vocabulary refuses (fail closed), never reads as a pass`, async () => { + const d = await explainAs(deps({ master: answering({ outcome: 'maybe' } as unknown as ControlledByParentWriteOutcome) }), verb); + expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); + expect(sharingRecordOf(d)?.outcome).toBe('not_evaluated'); + }); - it("the record's own row-level security still decides first (step 2.7 runs above step 2.8)", async () => { - const d = await explainUpdate(deps({ - master: answering({ outcome: 'deny', leg: 'record_sharing' }), - layer1: { id: 'not_this_record' }, - })); - expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'rls' }); - }); + it(`${verb}: the record's own row-level security still decides first (step 2.7 runs above step 2.8)`, async () => { + const d = await explainAs(deps({ + master: answering({ outcome: 'deny', leg: 'record_sharing' }), + layer1: { id: 'not_this_record' }, + }), verb); + expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'rls' }); + }); + } }); describe('[ADR-0055] a proceeding outcome changes nothing in the report', () => { - it('allow: the report is exactly the one a deps bag without the check gives — the master is not named', async () => { - const without = await explainUpdate(deps()); - const withAllow = await explainUpdate(deps({ master: answering({ outcome: 'allow' }) })); - expect(withAllow).toEqual(without); - expect(withAllow.record).toMatchObject({ visible: true }); - expect(sharingRecordOf(withAllow)?.detail).not.toContain('master'); - }); - - it('allow does not lift a refusal the rest of the pipeline makes (the sharing gate refuses)', async () => { - const without = await explainUpdate(deps({ canEdit: false })); - const withAllow = await explainUpdate(deps({ canEdit: false, master: answering({ outcome: 'allow' }) })); - expect(withAllow).toEqual(without); - expect(withAllow.record).toMatchObject({ visible: false }); - }); + for (const verb of MASTER_CHECKED) { + it(`${verb}, allow: the report is exactly the one a deps bag without the check gives — the master is not named`, async () => { + const without = await explainAs(deps(), verb); + const withAllow = await explainAs(deps({ master: answering({ outcome: 'allow' }) }), verb); + expect(withAllow).toEqual(without); + expect(withAllow.record).toMatchObject({ visible: true }); + expect(sharingRecordOf(withAllow)?.detail).not.toContain('master'); + }); - for (const [label, schema, canEdit] of [ - ['public_read_write', { name: OBJECT, sharingModel: 'public_read_write' }, true], - ['private, the gate admits', { name: OBJECT, sharingModel: 'private' }, true], - ['private, the gate refuses', { name: OBJECT, sharingModel: 'private' }, false], - ] as const) { - it(`not_applicable on a ${label} object: byte-identical to the report without the check`, async () => { - const without = await explainUpdate(deps({ schema, canEdit })); - const withCheck = await explainUpdate(deps({ schema, canEdit, master: answering({ outcome: 'not_applicable' }) })); - expect(withCheck).toEqual(without); + it(`${verb}, allow does not lift a refusal the rest of the pipeline makes (the sharing gate refuses)`, async () => { + const without = await explainAs(deps({ canEdit: false }), verb); + const withAllow = await explainAs(deps({ canEdit: false, master: answering({ outcome: 'allow' }) }), verb); + expect(withAllow).toEqual(without); + expect(withAllow.record).toMatchObject({ visible: false }); }); + + for (const [label, schema, canEdit] of [ + ['public_read_write', { name: OBJECT, sharingModel: 'public_read_write' }, true], + ['private, the gate admits', { name: OBJECT, sharingModel: 'private' }, true], + ['private, the gate refuses', { name: OBJECT, sharingModel: 'private' }, false], + ] as const) { + it(`${verb}, not_applicable on a ${label} object: byte-identical to the report without the check`, async () => { + const without = await explainAs(deps({ schema, canEdit }), verb); + const withCheck = await explainAs(deps({ schema, canEdit, master: answering({ outcome: 'not_applicable' }) }), verb); + expect(withCheck).toEqual(without); + }); + } } - it('absent check (a kernel without the member): the update keeps the sharing gate\'s answer and claims nothing about a master', async () => { - const d = await explainUpdate(deps()); - expect(d.record).toMatchObject({ recordId: 'r1', visible: true }); - expect(sharingRecordOf(d)?.detail).not.toContain('master'); + it('absent check (a kernel without the member): each write keeps the sharing gate\'s answer and claims nothing about a master', async () => { + for (const verb of MASTER_CHECKED) { + const d = await explainAs(deps(), verb); + expect(d.record, verb).toMatchObject({ recordId: 'r1', visible: true }); + expect(sharingRecordOf(d)?.detail, verb).not.toContain('master'); + } }); }); -describe('[ADR-0055] when the check is asked, and for whom', () => { - it('an update of a record that exists asks it once, for that record, with the EXPLAINED context', async () => { - const master = answering({ outcome: 'allow' }); - await explainUpdate(deps({ master })); - expect(master).toHaveBeenCalledTimes(1); - expect(master).toHaveBeenCalledWith(OBJECT, 'r1', EXPLAINED); - expect((master as ReturnType).mock.calls[0][2]).toBe(EXPLAINED); +describe('[ADR-0055] which verbs ask the check, and for whom', () => { + it('the classification is total: every operation explain answers is classified against step 2.8', () => { + // A verb added to the explain vocabulary lands here unclassified and turns + // this red, so its parity with the door is decided rather than inherited. + expect([...ExplainOperationSchema.options].sort()).toEqual(Object.keys(VERB_CLASS).sort()); }); - for (const operation of ['read', 'create', 'delete', 'transfer', 'export'] as ExplainOperation[]) { - it(`a record-grained ${operation} does not ask it`, async () => { + for (const verb of MASTER_CHECKED) { + it(`a ${verb} of a record that exists asks it once, for that record, with the EXPLAINED context`, async () => { + const master = answering({ outcome: 'allow' }); + await explainAs(deps({ master }), verb); + expect(master).toHaveBeenCalledTimes(1); + expect(master).toHaveBeenCalledWith(OBJECT, 'r1', EXPLAINED); + expect((master as ReturnType).mock.calls[0][2]).toBe(EXPLAINED); + }); + } + + for (const verb of (Object.keys(VERB_CLASS) as ExplainOperation[]).filter((v) => VERB_CLASS[v] === 'master_checked_object_gate_first')) { + it(`a ${verb} asks it too, and the object gate, which refuses ${verb} to every principal, decides first`, async () => { + const master = answering({ outcome: 'deny', leg: 'object_permission' }); + const d = await explainAs(deps({ master }), verb); + expect(master).toHaveBeenCalledTimes(1); + expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'object_crud' }); + expect(sharingRecordOf(d)?.detail).toContain(`refuses this ${verb} on its 'object_permission' leg`); + }); + } + + for (const verb of (Object.keys(VERB_CLASS) as ExplainOperation[]).filter((v) => VERB_CLASS[v] === 'not_asked')) { + it(`a record-grained ${verb} does not ask it`, async () => { const master = answering({ outcome: 'deny', leg: 'object_permission' }); - await explainAccess(deps({ master }), { object: OBJECT, operation, context: EXPLAINED, recordId: 'r1' }); + await explainAs(deps({ master }), verb); expect(master).not.toHaveBeenCalled(); }); } - it('an object-level update (no recordId) does not ask it, and a record that does not exist is not asked about', async () => { + it('an object-level write (no recordId) does not ask it, and a record that does not exist is not asked about', async () => { const master = answering({ outcome: 'deny', leg: 'object_permission' }); - const objectLevel = await explainUpdate(deps({ master }), null); - expect(objectLevel.record).toBeUndefined(); - const missing = await explainUpdate(deps({ master, record: null }), 'r_missing'); - expect(missing.record).toEqual({ recordId: 'r_missing', visible: false }); + for (const verb of MASTER_CHECKED) { + const objectLevel = await explainAs(deps({ master }), verb, null); + expect(objectLevel.record, verb).toBeUndefined(); + const missing = await explainAs(deps({ master, record: null }), verb, 'r_missing'); + expect(missing.record, verb).toEqual({ recordId: 'r_missing', visible: false }); + } expect(master).not.toHaveBeenCalled(); }); }); diff --git a/packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts b/packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts index a2bb5a3915d..5bae6f9e4e1 100644 --- a/packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts +++ b/packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts @@ -29,6 +29,17 @@ // question (may this principal update `cpg_contract` at all), which is `true` // for every member here; this suite does not measure it. // +// ## Every write verb, not only update +// +// The data door runs the same master-detail write check (step 2.8) on a by-id +// DELETE, and on a transfer (the `PATCH` that writes `owner_id`, step 3.5's +// transfer door) too, and hands each one's ownership floor over to it the same +// way. The second block below enumerates every operation explain answers, +// classified against that door, and pins explain's verdict beside the door's +// answer for the master's editor and for a non-editor, per verb. Its +// classification is total, so a verb added to the explain vocabulary without a +// door parity row fails it. +// // ## Why the boot is org-bound // // The ownership floor binds `org_member` principals. An org-less boot measures @@ -37,7 +48,9 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { bootStack, type VerifyStack } from '@objectstack/verify'; -import { cpgStack, cpgSecurity } from './fixtures/cbp-parent-gates-fixture.js'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { ExplainOperationSchema, PermissionSetSchema, type ExplainOperation, type PermissionSet } from '@objectstack/spec/security'; +import { cpgStack, cpgBaselineSet, cpgFilesAndThreadsSet } from './fixtures/cbp-parent-gates-fixture.js'; import { assertArmed, principalArmed } from './armed.js'; const SYS = { isSystem: true } as const; @@ -49,6 +62,31 @@ const OWNERSHIP_FLOOR_DISARM = "an org-less harness: a fresh sign-up then holds only ['everyone'], so no floor applies and the editor " + 'control below passes whether or not explain hands the floor over. `orgContext: true` arms it.'; +/** + * The steward's grant: every write verb on the detail, so the object gate lets + * each verb through and the record-level gates answer it. `allowTransfer` is the + * grant the transfer door (a `PATCH` that writes `owner_id`) asks for. + */ +const contractStewardSet: PermissionSet = PermissionSetSchema.parse({ + name: 'cpe_contract_steward', + label: 'explain parity fixture — every write verb on the detail', + objects: { + cpg_contract: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, allowTransfer: true }, + }, +}); + +/** The fixture's own security composition, plus the steward's grant. */ +function cpeSecurity(): SecurityPlugin { + return new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, cpgBaselineSet, cpgFilesAndThreadsSet, contractStewardSet], + fallbackPermissionSet: cpgBaselineSet.name, + }); +} + +const OWNERSHIP_WRITE_AND_DELETE_FLOORS = + "the platform's wildcard row-level floors (`owner_only_writes` and `owner_only_deletes`, positions " + + "['org_member']), which a by-id write of a controlled_by_parent record hands over to the master-detail write check"; + /** Status plus the error code, for an assertion message that shows the whole answer. */ async function answer(res: Response): Promise<{ status: number; code: string | undefined; body: string }> { const body = await res.text(); @@ -74,6 +112,54 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do let memberContractId: string; /** The control: a `public_read_write` record. */ let boardId: string; + let adminId: string; + /** The steward: every write verb on `cpg_contract` (see `contractStewardSet`), the master's editor or not. */ + let stewardTok: string; + let stewardId: string; + /** Per verb with a by-id door: a child of the admin's master, which the steward may not edit. */ + const underAdmin: Partial> = {}; + /** Per verb with a by-id door: a child of the steward's own master, which the steward did not create. */ + const underSteward: Partial> = {}; + + /** + * Every operation explain answers, classified against the data door. Total + * by construction (`Record`) and checked against the + * spec's vocabulary below, so a verb added there without a row here fails. + * + * - `by_id_door`: the door a principal reaches by id, which runs the + * master-detail write check (step 2.8) and hands the record's ownership + * floor over to it (step 2.7). `transfer`'s door is the `PATCH` that + * writes `owner_id`: the engine dispatches no `transfer` operation, and + * that write is where the transfer grant is enforced (step 3.5). + * - `object_gate_refused`: step 2.8 lists the verb, but the object gate + * refuses it to every principal (its grant is retired until the lifecycle + * batch returns it) and no route dispatches it. + * - `no_by_id_write`: not a write the master check judges by id — a read, + * or an insert, whose master step 2.8 reads off the request body. + */ + type DoorRow = + | { kind: 'by_id_door'; door: (token: string, id: string) => Promise } + | { kind: 'object_gate_refused' } + | { kind: 'no_by_id_write' }; + const DOORS: Record = { + update: { kind: 'by_id_door', door: (t, id) => stack.apiAs(t, 'PATCH', `/data/cpg_contract/${id}`, { name: 'stewarded' }) }, + delete: { kind: 'by_id_door', door: (t, id) => stack.apiAs(t, 'DELETE', `/data/cpg_contract/${id}`) }, + transfer: { + kind: 'by_id_door', + door: (t, id) => stack.apiAs(t, 'PATCH', `/data/cpg_contract/${id}`, { owner_id: adminId }), + }, + restore: { kind: 'object_gate_refused' }, + purge: { kind: 'object_gate_refused' }, + create: { kind: 'no_by_id_write' }, + read: { kind: 'no_by_id_write' }, + export: { kind: 'no_by_id_write' }, + }; + const verbsOf = (kind: DoorRow['kind']) => (Object.keys(DOORS) as ExplainOperation[]).filter((v) => DOORS[v].kind === kind); + const doorOf = (verb: ExplainOperation) => { + const row = DOORS[verb]; + if (row.kind !== 'by_id_door') throw new Error(`${verb} has no by-id door`); + return row.door; + }; const uid = async (email: string) => (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; @@ -91,16 +177,31 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do return { record: decision.record, sharing: sharing?.record as { outcome?: string; detail?: string } | undefined }; }; + /** explain for any verb, as the REST route answers it; the sharing layer's record attribution beside the record verdict. */ + const explainAs = async (token: string, operation: ExplainOperation, object: string, recordId: string) => { + const res = await stack.apiAs(token, 'POST', '/security/explain', { object, operation, recordId }); + const text = await res.text(); + expect(res.status, `explain ${operation} ${object}/${recordId}: ${text}`).toBe(200); + const decision = JSON.parse(text) as any; + const sharing = (decision.layers ?? []).find((l: any) => l.layer === 'sharing'); + return { record: decision.record, sharing: sharing?.record as { outcome?: string; detail?: string } | undefined }; + }; + const patch = async (token: string, object: string, id: string, name: string) => answer(await stack.apiAs(token, 'PATCH', `/data/${object}/${id}`, { name })); beforeAll(async () => { - stack = await bootStack(cpgStack as never, { security: cpgSecurity(), orgContext: true }); + stack = await bootStack(cpgStack as never, { security: cpeSecurity(), orgContext: true }); adminTok = await stack.signIn(); memberTok = await stack.signUp('cpe-member@verify.test'); ql = await stack.kernel.getServiceAsync('objectql'); - const adminId = await uid('admin@objectos.ai'); + adminId = await uid('admin@objectos.ai'); memberId = await uid('cpe-member@verify.test'); + stewardTok = await stack.signUp('cpe-steward@verify.test'); + stewardId = await uid('cpe-steward@verify.test'); + const stewardSet = await ql.findOne('sys_permission_set', { where: { name: contractStewardSet.name }, context: SYS }); + expect(stewardSet?.id, 'fixture permission set seeded').toBeTruthy(); + await ql.insert('sys_user_permission_set', { user_id: stewardId, permission_set_id: stewardSet.id }, { context: { ...SYS } }); await assertArmed([ principalArmed({ @@ -111,6 +212,17 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do control: OWNERSHIP_FLOOR, disarmedBy: OWNERSHIP_FLOOR_DISARM, }), + principalArmed({ + stack, + token: stewardTok, + who: 'the steward (every write verb on the detail; edits their own master, not the admin\'s)', + positions: ['org_member'], + permissions: [contractStewardSet.name], + control: OWNERSHIP_WRITE_AND_DELETE_FLOORS, + disarmedBy: + OWNERSHIP_FLOOR_DISARM + + ' And without the steward grant every delete and transfer below is refused at the object gate, before any record-level gate answers.', + }), ]); const adminAccountId = (await ql.insert('cpg_account', { name: 'admin account', owner_id: adminId }, { context: { ...SYS } })).id; @@ -119,6 +231,20 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do memberContractId = ( await ql.insert('cpg_contract', { name: 'member contract', account: memberAccountId }, { context: { ...SYS } }) ).id; + // One pair of children per verb with a by-id door, inserted as the system, so + // the steward created neither: the ownership floor binds both until a door + // hands it over to the master check. + const stewardAccountId = ( + await ql.insert('cpg_account', { name: 'steward account', owner_id: stewardId }, { context: { ...SYS } }) + ).id; + for (const verb of verbsOf('by_id_door')) { + underAdmin[verb] = ( + await ql.insert('cpg_contract', { name: `${verb}: admin's master`, account: adminAccountId }, { context: { ...SYS } }) + ).id; + underSteward[verb] = ( + await ql.insert('cpg_contract', { name: `${verb}: steward's master`, account: stewardAccountId }, { context: { ...SYS } }) + ).id; + } const boardRes = await stack.apiAs(adminTok, 'POST', '/data/cpg_board', { name: 'open board' }); expect(boardRes.status, 'board create').toBeLessThan(300); const board = (await boardRes.json()) as any; @@ -169,4 +295,44 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do const admitted = await patch(memberTok, 'cpg_board', boardId, 'open board'); expect(admitted.status, `member PATCH of the board: ${admitted.body}`).toBe(200); }); + describe('every write verb explain answers: its verdict is the door\'s, for the master\'s editor and for a non-editor', () => { + it('the classification is total: every operation explain answers has a door row', () => { + expect([...ExplainOperationSchema.options].sort()).toEqual(Object.keys(DOORS).sort()); + }); + + for (const verb of verbsOf('by_id_door')) { + it(`${verb}: the master's non-editor is explained refused on the master leg the door refuses on`, async () => { + const id = underAdmin[verb]!; + const explained = await explainAs(stewardTok, verb, 'cpg_contract', id); + const door = await answer(await doorOf(verb)(stewardTok, id)); + expect(door.status, `${verb} door, the master's non-editor: ${door.body}`).toBe(403); + expect(door.code).toBe('PERMISSION_DENIED'); + expect(door.body).toContain('requires edit access to its master record'); + expect(door.body).toContain('(row-level security)'); + + expect(explained.record, `explain ${verb}`).toEqual({ recordId: id, visible: false, decidedBy: 'sharing' }); + expect(explained.sharing?.outcome).toBe('excluded'); + expect(explained.sharing?.detail).toContain(`refuses this ${verb} on its 'row_level_security' leg`); + }); + + it(`${verb}: the master's editor, who did not create the child, is explained writable and the door answers 200`, async () => { + const id = underSteward[verb]!; + const explained = await explainAs(stewardTok, verb, 'cpg_contract', id); + const door = await answer(await doorOf(verb)(stewardTok, id)); + expect(door.status, `${verb} door, the master's editor: ${door.body}`).toBe(200); + + expect(explained.record, `explain ${verb}`).toMatchObject({ recordId: id, visible: true }); + expect(explained.sharing?.detail ?? '').not.toContain('master-detail write check'); + }); + } + + for (const verb of verbsOf('object_gate_refused')) { + it(`${verb}: refused at the object gate, for the master's editor and for a non-editor alike`, async () => { + for (const id of [underAdmin.update!, underSteward.update!]) { + const explained = await explainAs(stewardTok, verb, 'cpg_contract', id); + expect(explained.record, `explain ${verb} ${id}`).toEqual({ recordId: id, visible: false, decidedBy: 'object_crud' }); + } + }); + } + }); }); From 20f71f01f8686bc77a45ce1ff9def88f9dc77090 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 22:31:19 +0000 Subject: [PATCH 2/2] fix(plugin-security): explain's delete and transfer verdicts on a controlled_by_parent record come from the master-detail write check The write path runs the master-detail write check (step 2.8) on every by-id write of a controlled_by_parent record and hands the record's ownership floor over to it (step 2.7). explain asked the check for an update only, so a delete kept its owner_only_deletes floor and a transfer read the sharing gate's abstention as writable. explain now asks the served member for every by-id write step 2.8 runs on. The check judges edit access to the master whatever the record's verb, so the update answer it computes is each verb's answer. The explain wiring carries step 2.7's coverage vouch for a delete as it did for an update. No second copy of the check, no spec change. Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- .../22530-explain-cbp-write-verb-parity.md | 11 ++ .../plugin-security/src/explain-engine.ts | 100 +++++++++++++----- .../plugin-security/src/security-plugin.ts | 42 ++++---- 3 files changed, 106 insertions(+), 47 deletions(-) create mode 100644 .changeset/22530-explain-cbp-write-verb-parity.md diff --git a/.changeset/22530-explain-cbp-write-verb-parity.md b/.changeset/22530-explain-cbp-write-verb-parity.md new file mode 100644 index 00000000000..ddc77a8d614 --- /dev/null +++ b/.changeset/22530-explain-cbp-write-verb-parity.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-security': patch +--- + +`security/explain` answers a delete and a transfer of a `controlled_by_parent` record the way each verb's own door answers it: their record verdicts now come from the master-detail write check (ADR-0055), as the update's already did. + +- **What was wrong.** The write path runs the master-detail write check on every by-id write of such a record (an update, a delete, a transfer), and hands the record's platform ownership floor over to that check. explain asked the check for an update only. + - A delete kept its `owner_only_deletes` floor. A principal who may edit the master but did not create the record was explained as refused (`decidedBy: 'rls'`) beside a `DELETE` that answers `200`. A principal who may not edit the master was refused on that floor, with no word about the master, beside a `DELETE` whose `403` names the master. + - A transfer asked only the sharing service's per-record gate, which abstains on such a record. A principal who may not edit the master was explained as allowed to transfer it, beside the `PATCH` that writes `owner_id` and answers `403`. +- **What it does now.** For an update, a delete or a transfer of a record that exists (and a restore or a purge, which the object gate still refuses to everyone), explain asks the security service's `checkControlledByParentWrite` with the context it is explaining. The check judges edit access to the master whatever the record's own verb, so its answer is each verb's answer. A `deny` or `unresolvable` outcome makes the record not writable, decided by the `sharing` layer, whose record attribution names the verb and the refusing leg (or the reason no verdict was reached). A delete's row-level composition carries the write path's master-gate coverage vouch, as an update's does, so the ownership floor is handed over as the write path hands it, and never on behalf of a delegator. +- **What is unchanged.** Every object that is not `controlled_by_parent`. Object-level reports (no `recordId`). The `allowed` field. A `create` asks no master check: the write path reads an insert's master from the request body, which an explanation does not carry. The `ExplainEngineDeps` type: the `checkControlledByParentWrite` dependency keeps its signature and is now asked for every by-id write, not only an update. diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index eadebfe59c3..0990c5fc41d 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -374,19 +374,24 @@ export interface ExplainEngineDeps { * [ADR-0055] The master-detail write check's own answer for a by-id UPDATE * of `recordId` by `context`: `ISecurityService.checkControlledByParentWrite`, * the composition step 2.8 of the write path runs. On a - * `controlled_by_parent` record the per-record sharing gate above abstains - * (the object's effective model is `public`), so it cannot be the update's + * `controlled_by_parent` record the per-record sharing gates above abstain + * (the object's effective model is `public`), so they cannot be a write's * verdict there: this check is, and the door refuses on every outcome but * `allow` and `not_applicable`. * - * Asked for every `update` of a record that exists, with the context being - * EXPLAINED. The member holds the `controlled_by_parent` predicate, so the - * engine keeps no second copy of it: `not_applicable` leaves the report - * exactly as it was. A rejection (a context the write path refuses, or a - * store fault) is a dependency fault, reported fail-closed. + * Asked for every by-id write step 2.8 runs the check on + * ({@link MASTER_CHECKED_BY_ID_WRITES}: `update`, `delete`, `transfer`, + * `restore`, `purge`) of a record that exists, with the context being + * EXPLAINED. The update's answer is each of those writes' answer: the check + * judges EDIT access to the master whatever the record's own verb, and reads + * the verb only to pick its `insert` branch. The member holds the + * `controlled_by_parent` predicate, so the engine keeps no second copy of it: + * `not_applicable` leaves the report exactly as it was. A rejection (a + * context the write path refuses, or a store fault) is a dependency fault, + * reported fail-closed. * - * Optional: a deps bag wired without it explains the update from the sharing - * gate alone, as before, and claims nothing about a master. + * Optional: a deps bag wired without it explains each write from the sharing + * gates alone, as before, and claims nothing about a master. */ checkControlledByParentWrite?: ( object: string, @@ -907,23 +912,56 @@ const MASTER_DENIAL_LEG_DETAIL: Record master_chain: 'the controlled_by_parent chain cannot be walked to a master that governs its own rows', }; -/** [ADR-0055] Why the master-detail write check reached no verdict, and what the update answers instead. */ -const MASTER_UNRESOLVED_DETAIL: Record = { - master_detail_relation_missing: - 'the object declares controlled_by_parent with no master_detail relation to derive access from, and the ' + - 'update answers 422 INVALID_METADATA', - record_not_found: 'the record does not exist, and the update answers 404 RECORD_NOT_FOUND', - master_reference_missing: "the record's master reference is empty, and the update answers 422 MISSING_REQUIRED_FIELD", +/** + * [ADR-0055] Why the master-detail write check reached no verdict, and what the + * write answers instead. The answer is the same for every by-id write the check + * runs on: each condition throws one error class whatever the verb. + */ +const MASTER_UNRESOLVED_DETAIL: Record = { + master_detail_relation_missing: { + condition: 'the object declares controlled_by_parent with no master_detail relation to derive access from', + answer: '422 INVALID_METADATA', + }, + record_not_found: { condition: 'the record does not exist', answer: '404 RECORD_NOT_FOUND' }, + master_reference_missing: { condition: "the record's master reference is empty", answer: '422 MISSING_REQUIRED_FIELD' }, }; const MASTER_DERIVED = 'Write access to this record derives from its master (controlled_by_parent, ADR-0055)'; /** - * [ADR-0055] The sharing layer's record attribution for an update the - * master-detail write check REFUSES — the check that decides an update of a + * [ADR-0055] The record-grained writes whose verdict on a `controlled_by_parent` + * record the master-detail write check decides: every write step 2.8 of the + * write path runs that check on (the guard of step 2.8 in `security-plugin.ts`) + * that addresses one existing record by id. + * + * `insert` is the one verb step 2.8 runs on that is not here. Its master is read + * off the request BODY, which an explanation does not carry, and it addresses + * no existing record, so a record-grained `create` has no master check to ask. + * + * One answer serves every verb here. Whatever the record's own verb, the check + * judges EDIT access to the master (the object-level `update` grant, the + * master's write row-level security and record sharing, on every hop up the + * chain) and reads the verb only to pick its `insert` branch. + * `masterGateCoversOperation` states the same fact for the ownership floor's + * hand-over: deleting a child does not delete its master. So the answer the + * served member computes for an update is the answer step 2.8 gives each of + * these writes, and the engine asks it for each. + * + * `restore` and `purge` are refused at the object gate for every principal + * until their grants return, so the object-level CRUD layer decides their + * record verdict first. They are listed because step 2.8 lists them, so the + * report stays the door's on the day they can be granted. + */ +const MASTER_CHECKED_BY_ID_WRITES: ReadonlySet = new Set(['update', 'delete', 'transfer', 'restore', 'purge']); + +/** + * [ADR-0055] The sharing layer's record attribution for a by-id write the + * master-detail write check REFUSES — the check that decides such a write of a * `controlled_by_parent` record — or `undefined` where it does not refuse. + * `verb` is the write being explained (one of + * {@link MASTER_CHECKED_BY_ID_WRITES}), named in the prose. * - * The mapping is the door's: the update proceeds on `allow` and on + * The mapping is the door's: the write proceeds on `allow` and on * `not_applicable`, and is refused on every other outcome, one outside the * vocabulary included (fail closed). A proceeding outcome leaves the report * exactly as the rest of the pipeline computes it. ⛔ It does not name the @@ -933,6 +971,7 @@ const MASTER_DERIVED = 'Write access to this record derives from its master (con */ function masterWriteCheckRefusal( outcome: ControlledByParentWriteOutcome, + verb: string, ): { outcome: ExplainRecordAttribution['outcome']; detail: string } | undefined { switch (outcome?.outcome) { case 'allow': @@ -941,17 +980,19 @@ function masterWriteCheckRefusal( case 'deny': return { outcome: 'excluded', - detail: `${MASTER_DERIVED}: the master-detail write check the by-id update runs refuses this update on its ` + + detail: `${MASTER_DERIVED}: the master-detail write check the by-id ${verb} runs refuses this ${verb} on its ` + `'${outcome.leg}' leg — ${MASTER_DENIAL_LEG_DETAIL[outcome.leg] ?? 'a leg this report does not describe'}. ` + - 'The update answers 403 PERMISSION_DENIED.', + `The ${verb} answers 403 PERMISSION_DENIED.`, }; - case 'unresolvable': + case 'unresolvable': { + const unresolved = MASTER_UNRESOLVED_DETAIL[outcome.reason]; return { outcome: 'not_evaluated', detail: `${MASTER_DERIVED}, and the master-detail write check reaches no verdict ('${outcome.reason}'): ` + - `${MASTER_UNRESOLVED_DETAIL[outcome.reason] ?? 'a reason this report does not describe'}. The record is ` + - 'reported NOT writable (fail closed), never admitted.', + `${unresolved ? `${unresolved.condition}, and the ${verb} answers ${unresolved.answer}` : 'a reason this report does not describe'}. ` + + 'The record is reported NOT writable (fail closed), never admitted.', }; + } default: return { outcome: 'not_evaluated', @@ -1431,21 +1472,22 @@ async function applyRecordAttribution( : undefined; const writeGateFault = canEditOrFault === DEPENDENCY_FAULT; const canEdit = canEditOrFault === DEPENDENCY_FAULT ? undefined : canEditOrFault; - // [ADR-0055] An update of a `controlled_by_parent` record is judged by the + // [ADR-0055] A by-id write of a `controlled_by_parent` record (an update, a + // delete, a transfer: {@link MASTER_CHECKED_BY_ID_WRITES}) is judged by the // master-detail write check (step 2.8), which the sharing gate above cannot // stand in for: it abstains on such a record, and its abstention reads as - // `true`. So the check is asked for every update of a record that exists, + // `true`. So the check is asked for every such write of a record that exists, // with the explained context, and it decides whether it applies: `allow` and // `not_applicable` change nothing, every other outcome refuses // ({@link masterWriteCheckRefusal}). A rejection is the request failing, // exactly as for the gate. - const masterOrFault = engineOp === 'update' && recordExists && deps.checkControlledByParentWrite + const masterOrFault = MASTER_CHECKED_BY_ID_WRITES.has(engineOp) && recordExists && deps.checkControlledByParentWrite ? await settle(deps.checkControlledByParentWrite(object, recordId, context)) : undefined; const masterFault = masterOrFault === DEPENDENCY_FAULT; const masterRefusal = masterOrFault === undefined || masterOrFault === DEPENDENCY_FAULT ? undefined - : masterWriteCheckRefusal(masterOrFault); + : masterWriteCheckRefusal(masterOrFault, engineOp); // The sharing call THIS operation's row verdict rests on: the read filter for // a read (the sharing middleware ANDs it into every find), the per-record gate // for a write (the middleware gates every by-id write on it). Either one @@ -1462,7 +1504,7 @@ async function applyRecordAttribution( 'writable (fail closed), never admitted.' : masterFault ? 'The master-detail write check (controlled_by_parent, ADR-0055) could not be evaluated: it rejected — ' + - 'a refusal of the context or a store fault — and the by-id update fails on the same call, so the ' + + `a refusal of the context or a store fault — and the by-id ${engineOp} fails on the same call, so the ` + 'record is reported NOT writable (fail closed), never admitted.' : undefined); const anyShareAdmits = shareRules.some((r) => r.effect === 'admits'); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index c9f353de6ca..597bd3407f7 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -469,11 +469,11 @@ interface RlsFilterOptions { * • `getReadFilter` and `checkAuthoredRowWrite` do not, and need not: the * floor is `update`/`delete`-only so the read path never carries it, and * the authored-write probe already removes it by provenance; - * • `security/explain`'s record-grained UPDATE verdict sets it on step - * 2.7's condition, because explain asks the master gate for that same - * update ({@link SecurityPlugin.checkControlledByParentWrite}) and - * refuses on its refusal; its `delete` verdict asks no master gate and - * does not. + * • `security/explain`'s record-grained UPDATE and DELETE verdicts set it + * on step 2.7's condition, because explain asks the master gate for + * every by-id write it explains + * ({@link SecurityPlugin.checkControlledByParentWrite}) and refuses on + * its refusal. * * `false`/absent leaves the floor exactly where it is. Never affects Layer 0 * (the tenant wall) or any app-authored policy. @@ -5431,14 +5431,14 @@ export class SecurityPlugin implements Plugin { // answer the by-id PATCH / DELETE gives: // - Layer 1 carries the pre-image gate's floor decision (step 2.7, // {@link resolvePreImageFloorDrop}). - // - [ADR-0055] For an `update` it also carries step 2.7's coverage vouch, - // `masterGateCoversThisWrite`, on the same condition (not on behalf of - // anyone): explain asks the master gate for every update of a record - // that exists ({@link checkControlledByParentWrite}, wired below), so a - // `controlled_by_parent` record's ownership floor is handed over to that - // check here exactly as the write path hands it over. `delete` keeps - // the floor: explain asks no master gate for it, so it has nothing to - // vouch with. + // - [ADR-0055] It also carries step 2.7's coverage vouch, + // `masterGateCoversThisWrite`, on step 2.7's own condition (not on behalf + // of anyone), for an `update` and a `delete` alike: explain asks the + // master gate for every by-id write of a record that exists + // ({@link checkControlledByParentWrite}, wired below), so a + // `controlled_by_parent` record's ownership floor (`owner_only_writes`, + // `owner_only_deletes`) is handed over to that check here exactly as the + // write path hands it over. // - plugin-sharing's per-record gate is asked with `__writeScope` stamped // as the middleware stamps it (step 2.6), always overwritten, so an // `org` / unit writer is not judged owner-only. @@ -5462,7 +5462,10 @@ export class SecurityPlugin implements Plugin { const dropPlatformOwnershipFloor = await this.resolvePreImageFloorDrop( engineOp, o, recordId, c, sets, actsOnBehalfOf(c), ).catch(() => false); - const masterGateCoversThisWrite = engineOp === 'update' && !actsOnBehalfOf(c); + // Step 2.7's vouch is `!delegatorSets` for every verb it runs on; the + // floor itself comes off only where `masterGateCoversOperation` says the + // master gate covers the verb (`update`, `delete`). + const masterGateCoversThisWrite = !actsOnBehalfOf(c); return { dropPlatformOwnershipFloor, masterGateCoversThisWrite }; }; const withWriteScope = async (o: string, c: any): Promise => @@ -5546,10 +5549,13 @@ export class SecurityPlugin implements Plugin { // the caller-context by-id read, every data middleware included. recordAbsentToCaller: (o: string, rid: string, c: any) => absentUnderCallerRead(() => this.readRowById(o, rid, c)), - // [ADR-0055] The master-detail write check an update meets at step - // 2.8, served as `ISecurityService.checkControlledByParentWrite` and - // asked with the EXPLAINED context. Its presence is what licenses the - // update's `masterGateCoversThisWrite` vouch above: the two go together. + // [ADR-0055] The master-detail write check a by-id update, delete or + // transfer meets at step 2.8, served as + // `ISecurityService.checkControlledByParentWrite` and asked with the + // EXPLAINED context. The update's answer is each of those verbs' answer + // (the check judges EDIT of the master whatever the record's verb). Its + // presence is what licenses the `masterGateCoversThisWrite` vouch + // above: the two go together. checkControlledByParentWrite: (o: string, rid: string, c: any) => this.checkControlledByParentWrite(o, rid, c), },