diff --git a/.changeset/22792-explain-read-master-leg.md b/.changeset/22792-explain-read-master-leg.md new file mode 100644 index 00000000000..bd24f936118 --- /dev/null +++ b/.changeset/22792-explain-read-master-leg.md @@ -0,0 +1,8 @@ +--- +'@objectstack/plugin-security': patch +--- + +`security/explain`'s record-grained `read` verdict for a `controlled_by_parent` record now equals the read door's answer on the master leg (ADR-0055). + +- **What changed.** The read door scopes such a record by its master as well as by its own row-level security. explain's read branch modelled the record's own row-level security only. It now asks the read door's own by-id read, with the context it is explaining, wherever every other leg of the read's row story admits the record. A record the door does not return is reported not visible, decided by the `sharing` layer. A record the door returns is reported exactly as before. A rejection of that read is reported fail-closed, as a dependency that throws already is. An `export` explanation, which streams the same find, answers the same way. +- **What is unchanged.** Every object that is not `controlled_by_parent`. Object-level reports (no `recordId`). The `allowed` field. Every write verb. The answer for an id no row carries. The `ExplainEngineDeps` type: the `recordAbsentToCaller` dependency keeps its signature and is now asked for such a read too. 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 84e7c26d39f..eae34392e9a 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 @@ -23,6 +23,15 @@ // record's write gate), naming the leg or the reason; a rejection fails the // request, so it is reported fail-closed; an outcome outside the vocabulary // refuses too. +// +// A READ of such a record meets a master leg as well: the read door scopes the +// find by the master ids the principal can read. The write check does not +// answer a read, so explain asks the read door's own by-id read +// (`recordAbsentToCaller`) where every leg it models admits the record, and +// takes its answer. The last block pins that half: a record the door withholds +// is not visible on the `sharing` layer, a record the door returns leaves the +// report exactly as before, and a rejection is reported fail-closed. The REST +// answer beside the REST read door is pinned in the same dogfood suite. import { describe, it, expect, vi } from 'vitest'; import { PermissionSetSchema } from '@objectstack/spec/security'; import { ExplainOperationSchema, type ExplainDecision, type ExplainOperation } from '@objectstack/spec/security'; @@ -39,7 +48,9 @@ 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, allowTransfer: true } }, + objects: { + [OBJECT]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, allowTransfer: true, allowExport: true }, + }, }); /** The principal being EXPLAINED. Kept as one object so a cell can assert the check received this very context. */ @@ -47,6 +58,7 @@ const EXPLAINED = { userId: 'u_member', tenantId: 'org1', positions: ['org_membe const RECORD = { id: 'r1', organization_id: 'org1', owner_id: 'u_somebody' }; type Master = ExplainEngineDeps['checkControlledByParentWrite']; +type ReadDoor = ExplainEngineDeps['recordAbsentToCaller']; function deps(opts: { schema?: Record; @@ -54,6 +66,8 @@ function deps(opts: { canEdit?: boolean; record?: Record | null; layer1?: Record | null; + /** The read door's own by-id read: does it withhold the record from the explained principal? */ + readDoor?: ReadDoor; } = {}): ExplainEngineDeps { return { ql: { getSchema: () => opts.schema ?? CBP_SCHEMA }, @@ -77,6 +91,7 @@ function deps(opts: { canEditRecord: async () => opts.canEdit ?? true, canDeleteRecord: async () => opts.canEdit ?? true, ...(opts.master ? { checkControlledByParentWrite: opts.master } : {}), + ...(opts.readDoor ? { recordAbsentToCaller: opts.readDoor } : {}), }; } @@ -100,17 +115,23 @@ const explainAs = (d: ExplainEngineDeps, operation: ExplainOperation, recordId: * 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. + * - `read_door_asked`: a read, and an export, which streams the same find. + * The check guards writes and is not asked; the read door's master leg is, + * through the read door's own by-id read (the last block below). + * - `not_asked`: `create`, whose master comes from the request body an + * explanation does not carry. */ -const VERB_CLASS: Record = { +const VERB_CLASS: Record< + ExplainOperation, + 'master_checked' | 'master_checked_object_gate_first' | 'read_door_asked' | 'not_asked' +> = { 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', + read: 'read_door_asked', + export: 'read_door_asked', create: 'not_asked', }; const MASTER_CHECKED = (Object.keys(VERB_CLASS) as ExplainOperation[]).filter((v) => VERB_CLASS[v] === 'master_checked'); @@ -243,7 +264,9 @@ describe('[ADR-0055] which verbs ask the check, and for whom', () => { }); } - for (const verb of (Object.keys(VERB_CLASS) as ExplainOperation[]).filter((v) => VERB_CLASS[v] === 'not_asked')) { + for (const verb of (Object.keys(VERB_CLASS) as ExplainOperation[]).filter( + (v) => VERB_CLASS[v] === 'not_asked' || VERB_CLASS[v] === 'read_door_asked', + )) { it(`a record-grained ${verb} does not ask it`, async () => { const master = answering({ outcome: 'deny', leg: 'object_permission' }); await explainAs(deps({ master }), verb); @@ -262,3 +285,83 @@ describe('[ADR-0055] which verbs ask the check, and for whom', () => { expect(master).not.toHaveBeenCalled(); }); }); + +const READ_DOOR_ASKED = (Object.keys(VERB_CLASS) as ExplainOperation[]).filter((v) => VERB_CLASS[v] === 'read_door_asked'); + +/** The read door's answer: `true` withholds the record from the explained principal, `false` returns it. */ +const readDoorAnswering = (absent: boolean): ReadDoor => vi.fn(async () => absent); + +describe('[ADR-0055] a read of a controlled_by_parent record takes the read door\'s answer on the master leg', () => { + for (const verb of READ_DOOR_ASKED) { + it(`${verb}: a record the read door withholds is not visible, decided by the sharing layer`, async () => { + const d = await explainAs(deps({ readDoor: readDoorAnswering(true) }), verb); + expect(d.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'sharing' }); + expect(sharingRecordOf(d)?.outcome).toBe('excluded'); + }); + + it(`${verb}: a record the read door returns: byte-identical to the report without the question`, async () => { + const without = await explainAs(deps(), verb); + const withDoor = await explainAs(deps({ readDoor: readDoorAnswering(false) }), verb); + expect(withDoor).toEqual(without); + expect(withDoor.record).toMatchObject({ recordId: 'r1', visible: true }); + }); + + it(`${verb}: a rejection fails the find, 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 readDoor: ReadDoor = vi.fn(async () => { throw fault; }); + const d = await explainAs(deps({ readDoor }), verb); + 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(); + }); + + it(`${verb}: asked once, for that record, with the EXPLAINED context, and the write check is not asked`, async () => { + const readDoor = readDoorAnswering(false); + const master = answering({ outcome: 'deny', leg: 'object_permission' }); + await explainAs(deps({ readDoor, master }), verb); + expect(readDoor).toHaveBeenCalledTimes(1); + expect(readDoor).toHaveBeenCalledWith(OBJECT, 'r1', EXPLAINED); + expect((readDoor as ReturnType).mock.calls[0][2]).toBe(EXPLAINED); + expect(master).not.toHaveBeenCalled(); + }); + + it(`${verb}: a leg this report models that withholds the record decides, and the read door is not asked`, async () => { + const readDoor = readDoorAnswering(true); + const byRls = await explainAs(deps({ readDoor, layer1: { id: 'not_this_record' } }), verb); + expect(byRls.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'rls' }); + const byTenant = await explainAs(deps({ readDoor, record: { ...RECORD, organization_id: 'org2' } }), verb); + expect(byTenant.record).toEqual({ recordId: 'r1', visible: false, decidedBy: 'tenant_isolation' }); + expect(readDoor).not.toHaveBeenCalled(); + }); + + for (const [label, schema] of [ + ['public_read_write', { name: OBJECT, sharingModel: 'public_read_write' }], + ['private', { name: OBJECT, sharingModel: 'private' }], + ] as const) { + it(`${verb}: not asked on a ${label} object, which derives nothing from a master: the report is unchanged`, async () => { + const readDoor = readDoorAnswering(true); + const without = await explainAs(deps({ schema }), verb); + const withDoor = await explainAs(deps({ schema, readDoor }), verb); + expect(withDoor).toEqual(without); + expect(readDoor).not.toHaveBeenCalled(); + }); + } + + it(`${verb}: an object-level request, and a record that does not exist, are not asked about`, async () => { + const readDoor = readDoorAnswering(true); + const objectLevel = await explainAs(deps({ readDoor }), verb, null); + expect(objectLevel.record).toBeUndefined(); + const missing = await explainAs(deps({ readDoor, record: null }), verb, 'r_missing'); + expect(missing.record).toEqual({ recordId: 'r_missing', visible: false }); + expect(readDoor).not.toHaveBeenCalled(); + }); + } + + it('a create, which addresses no stored record, is not asked about', async () => { + const readDoor = readDoorAnswering(true); + await explainAs(deps({ readDoor }), 'create'); + expect(readDoor).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index f0f52a3c00d..9ded7bf31dc 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -419,6 +419,12 @@ export interface ExplainEngineDeps { * delete, read through {@link rlsOperationForVerb}: `update`, `delete`, and * the lifecycle verbs that map onto them (`transfer` and `restore` onto * update, `purge` onto delete). + * + * [ADR-0055] Asked for a record-grained read too, of a + * `controlled_by_parent` record the rest of the read's row story admits: the + * read door also scopes such a record by its master, a leg no other + * dependency here answers, so the door's own answer is what the read verdict + * takes ({@link applyRecordAttribution}). */ recordAbsentToCaller?: (object: string, recordId: string, context: any) => Promise; } @@ -942,6 +948,7 @@ const MASTER_UNRESOLVED_DETAIL: Record> = {}; /** Per verb with a by-id door: a child of the steward's own master, which the steward did not create. */ const underSteward: Partial> = {}; + /** Per verb with a by-id read door: a child of the admin's private master, which the steward cannot read. */ + const underUnreadable: Partial> = {}; + /** Per verb with a by-id read door: a child of the steward's own private master, which the steward reads. */ + const underReadable: Partial> = {}; /** * Every operation explain answers, classified against the data door. Total @@ -164,14 +174,20 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do * 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). + * - `by_id_read_door`: the by-id `GET`, which scopes a `controlled_by_parent` + * record by the master ids the principal can READ (ADR-0055). Asked of + * `cpg_vault_item`, whose master is private, so a principal who cannot + * read the master cannot read the record either. * - `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. + * - `no_by_id_write`: neither — an insert, whose master step 2.8 reads off + * the request body, or an export, which has no by-id door (it streams the + * read's find, and explain answers it as it answers the read). */ type DoorRow = | { kind: 'by_id_door'; door: (token: string, id: string) => Promise } + | { kind: 'by_id_read_door'; object: string; door: (token: string, id: string) => Promise } | { kind: 'object_gate_refused' } | { kind: 'no_by_id_write' }; const DOORS: Record = { @@ -184,7 +200,11 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do restore: { kind: 'object_gate_refused' }, purge: { kind: 'object_gate_refused' }, create: { kind: 'no_by_id_write' }, - read: { kind: 'no_by_id_write' }, + read: { + kind: 'by_id_read_door', + object: 'cpg_vault_item', + door: (t, id) => stack.apiAs(t, 'GET', `/data/cpg_vault_item/${id}`), + }, export: { kind: 'no_by_id_write' }, }; const verbsOf = (kind: DoorRow['kind']) => (Object.keys(DOORS) as ExplainOperation[]).filter((v) => DOORS[v].kind === kind); @@ -193,6 +213,11 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do if (row.kind !== 'by_id_door') throw new Error(`${verb} has no by-id door`); return row.door; }; + const readDoorOf = (verb: ExplainOperation) => { + const row = DOORS[verb]; + if (row.kind !== 'by_id_read_door') throw new Error(`${verb} has no by-id read door`); + return row; + }; const uid = async (email: string) => (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; @@ -270,6 +295,18 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do const stewardAccountId = ( await ql.insert('cpg_account', { name: 'steward account', owner_id: stewardId }, { context: { ...SYS } }) ).id; + const adminVaultId = (await ql.insert('cpg_vault', { name: 'admin vault', owner_id: adminId }, { context: { ...SYS } })).id; + const stewardVaultId = ( + await ql.insert('cpg_vault', { name: 'steward vault', owner_id: stewardId }, { context: { ...SYS } }) + ).id; + for (const verb of verbsOf('by_id_read_door')) { + underUnreadable[verb] = ( + await ql.insert('cpg_vault_item', { name: `${verb}: admin's vault`, vault: adminVaultId }, { context: { ...SYS } }) + ).id; + underReadable[verb] = ( + await ql.insert('cpg_vault_item', { name: `${verb}: steward's vault`, vault: stewardVaultId }, { 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 } }) @@ -328,7 +365,7 @@ 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', () => { + describe('every verb explain answers by id: its verdict is the door\'s, for the master\'s editor or reader and for a principal who is not', () => { it('the classification is total: every operation explain answers has a door row', () => { expect([...ExplainOperationSchema.options].sort()).toEqual(Object.keys(DOORS).sort()); }); @@ -359,6 +396,42 @@ describe('security/explain answers a controlled_by_parent update as its PATCH do }); } + for (const verb of verbsOf('by_id_read_door')) { + it(`${verb}: a record under a master the principal cannot read is explained not visible, as its door answers`, async () => { + const { object, door } = readDoorOf(verb); + const id = underUnreadable[verb]!; + const explained = await explainAs(stewardTok, verb, object, id); + const answered = await answer(await door(stewardTok, id)); + expect(answered.status, `${verb} door, the master's non-reader: ${answered.body}`).toBe(404); + expect(answered.code).toBe('RECORD_NOT_FOUND'); + + expect(explained.record, `explain ${verb}`).toEqual({ recordId: id, visible: false, decidedBy: 'sharing' }); + expect(explained.sharing?.outcome).toBe('excluded'); + }); + + it(`${verb}: control, a record under a master the principal reads is explained visible and its door answers 200`, async () => { + const { object, door } = readDoorOf(verb); + const id = underReadable[verb]!; + const explained = await explainAs(stewardTok, verb, object, id); + const answered = await answer(await door(stewardTok, id)); + expect(answered.status, `${verb} door, the master's reader: ${answered.body}`).toBe(200); + + expect(explained.record, `explain ${verb}`).toMatchObject({ recordId: id, visible: true }); + expect(explained.sharing?.outcome).not.toBe('excluded'); + }); + + it(`${verb}: control, an id no row carries is explained as it was, beside its door's 404`, async () => { + const { object, door } = readDoorOf(verb); + const id = `${verb}-no-such-record`; + const explained = await explainAs(stewardTok, verb, object, id); + const answered = await answer(await door(stewardTok, id)); + expect(answered.status, `${verb} door, an id no row carries: ${answered.body}`).toBe(404); + expect(answered.code).toBe('RECORD_NOT_FOUND'); + + expect(explained.record, `explain ${verb}`).toEqual({ recordId: id, visible: false }); + }); + } + 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!]) {