diff --git a/.changeset/22514-explain-cbp-update-master-check.md b/.changeset/22514-explain-cbp-update-master-check.md new file mode 100644 index 00000000000..6b3a78958e0 --- /dev/null +++ b/.changeset/22514-explain-cbp-update-master-check.md @@ -0,0 +1,10 @@ +--- +'@objectstack/plugin-security': minor +--- + +`security/explain` answers an update of a `controlled_by_parent` record the way that record's own by-id update is answered: its record verdict now comes from the master-detail write check (ADR-0055), which it never asked. + +- **What was wrong.** On such a record the sharing service's per-record gate abstains, because the effective model of `controlled_by_parent` is `public`, and explain read the abstention as "writable". It also kept the platform ownership floor (`owner_only_writes`) on the record, which the write path hands over to the master-detail write check. So `record.visible` for an update was decided by that floor (`decidedBy: 'rls'`) for every principal who did not create the record. That was refused, on the wrong layer and with no word about the master, for a principal who may not edit the master. It was also refused for a principal who may edit the master, beside a `PATCH` that answers `200`. +- **What it does now.** For every update of a record that exists, explain asks the security service's `checkControlledByParentWrite` with the context it is explaining (another user's, when `userId` names one). A `deny` or `unresolvable` outcome makes the record not writable, decided by the `sharing` layer, whose record attribution names the refusing leg (`object_permission`, `row_level_security`, `record_sharing` or `master_chain`) or the reason the check reached no verdict. A rejection of the check is reported fail-closed, as a dependency that throws already is. `allow` and `not_applicable` leave the report exactly as before. The update's row-level composition carries the write path's own master-gate coverage vouch, so the ownership floor is handed over to that check 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, which answers whether the principal may update the object at all. `delete` is unchanged too: explain asks no master check for it and keeps its floor. +- **Public surface.** `ExplainEngineDeps` gains the optional `checkControlledByParentWrite` dependency. A deps bag wired without it explains an update as before, and claims nothing about a master. 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 8e21ffcad06..9a72cc78d82 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 @@ -248,7 +248,14 @@ async function boot(options: { faultOn?: string } = {}) { } }; const member = (context: any, object: string, id: string) => serve(object, id, context); - return { store, ctx, patch, member }; + /** + * [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 explainUpdate = (callerContext: any, object: string, recordId: string, userId?: string) => + security.explain({ object, operation: 'update', recordId, ...(userId ? { userId } : {}) }, callerContext); + return { store, ctx, patch, member, explainUpdate }; } const rep = () => ({ userId: REP, tenantId: 'org-1', positions: [], permissions: ['cbpm_rep'] }); @@ -375,3 +382,40 @@ describe('[ADR-0055] checkControlledByParentWrite rejects where the write path r expect(rejection).toMatchObject({ code: 'ERR_DATASOURCE_UNAVAILABLE', status: 503 }); }); }); + +describe('[ADR-0055] security/explain answers an update as the PATCH does, from this same check', () => { + const sharingDetailOf = (d: any): string => String(d.layers.find((l: any) => l.layer === 'sharing')?.record?.detail ?? ''); + + it('a refused update is explained refused on the leg the PATCH names; an admitted one is explained writable', 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) { + expect(await h.patch(context, object, id), `PATCH ${object}/${id} by ${context.userId}`).not.toBe('admitted'); + const d = await h.explainUpdate(context, object, id); + expect(d.record, `explain ${object}/${id} by ${context.userId}`).toEqual({ recordId: id, visible: false, decidedBy: 'sharing' }); + expect(sharingDetailOf(d)).toContain(`refuses this update 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) { + expect(await h.patch(rep(), object, id), `PATCH ${object}/${id}`).toBe('admitted'); + const d = await h.explainUpdate(rep(), object, id); + expect(d.record, `explain ${object}/${id}`).toMatchObject({ recordId: id, visible: true }); + expect(sharingDetailOf(d)).not.toContain('master-detail write check'); + } + }); + + it('explaining ANOTHER user asks the check for THAT user: the system caller would be admitted, the viewer it explains is not', async () => { + const h = await boot(); + // Asked with the CALLER's context, the check would admit: a system context answers `allow`. + expect(await h.member({ isSystem: true }, 'cbpm_contact', 'c_own')).toEqual({ outcome: 'allow' }); + // The viewer, explained by user id, resolves to the fallback set — READ only on the master. + expect(await h.patch(viewer(), 'cbpm_contact', 'c_own')).not.toBe('admitted'); + const d = await h.explainUpdate({ isSystem: true }, 'cbpm_contact', 'c_own', VIEWER); + expect(d.principal.userId).toBe(VIEWER); + expect(d.record).toEqual({ recordId: 'c_own', visible: false, decidedBy: 'sharing' }); + expect(sharingDetailOf(d)).toContain("refuses this update on its 'object_permission' leg"); + }); +}); 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 new file mode 100644 index 00000000000..67346e5d874 --- /dev/null +++ b/packages/plugins/plugin-security/src/explain-controlled-by-parent-write.test.ts @@ -0,0 +1,205 @@ +// 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 +// (`ISecurityService.checkControlledByParentWrite`), the composition step 2.8 +// of the write path runs, 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 +// `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 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 +// gives; `deny` and `unresolvable` refuse, on the `sharing` layer (the +// 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. +import { describe, it, expect, vi } from 'vitest'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { ExplainDecision, ExplainOperation } from '@objectstack/spec/security'; +import type { + ControlledByParentWriteDenialLeg, + ControlledByParentWriteOutcome, + ControlledByParentWriteUnresolvedReason, +} from '@objectstack/spec/contracts'; +import { PermissionEvaluator } from './permission-evaluator.js'; +import { explainAccess, type ExplainEngineDeps } from './explain-engine.js'; + +const OBJECT = 'cbx_contract'; +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 } }, +}); + +/** The principal being EXPLAINED. Kept as one object so a cell can assert the check received this very context. */ +const EXPLAINED = { userId: 'u_member', tenantId: 'org1', positions: ['org_member'], permissions: ['cbx_editor'] }; +const RECORD = { id: 'r1', organization_id: 'org1', owner_id: 'u_somebody' }; + +type Master = ExplainEngineDeps['checkControlledByParentWrite']; + +function deps(opts: { + schema?: Record; + master?: Master; + canEdit?: boolean; + record?: Record | null; + layer1?: Record | null; +} = {}): ExplainEngineDeps { + return { + ql: { getSchema: () => opts.schema ?? CBP_SCHEMA }, + resolveSets: async () => [EDITOR], + evaluator: new PermissionEvaluator(), + getObjectSecurityMeta: async () => ({ + isPrivate: false, + requiredPermissions: { all: [], read: [], create: [], update: [], delete: [] }, + fieldRequiredPermissions: {}, + }), + requiredCaps: () => [], + computeRlsFilter: async () => null, + getFieldMask: () => ({}), + getPartialMaskRules: async () => ({}), + baselinePermissionSets: ['member_default'], + computeLayeredRlsFilter: async () => ({ layer0: { organization_id: 'org1' }, layer1: opts.layer1 ?? null }), + fetchRecord: async () => (opts.record !== undefined ? opts.record : { ...RECORD }), + sharingReadFilter: async () => null, + listRecordShares: async () => [], + // plugin-sharing on a `controlled_by_parent` record: it abstains, and the abstention reads as `true`. + canEditRecord: async () => opts.canEdit ?? true, + canDeleteRecord: async () => opts.canEdit ?? true, + ...(opts.master ? { checkControlledByParentWrite: opts.master } : {}), + }; +} + +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 sharingRecordOf = (d: ExplainDecision) => d.layers.find((l) => l.layer === 'sharing')?.record; + +const LEGS: ControlledByParentWriteDenialLeg[] = ['object_permission', 'row_level_security', 'record_sharing', 'master_chain']; +const REASONS: Array<[ControlledByParentWriteUnresolvedReason, string]> = [ + ['master_detail_relation_missing', '422 INVALID_METADATA'], + ['record_not_found', '404 RECORD_NOT_FOUND'], + ['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'); + }); + } + + 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 }) })); + 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); + }); + } + + 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("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' }); + }); +}); + +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 [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('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'); + }); +}); + +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); + }); + + for (const operation of ['read', 'create', 'delete', 'transfer', 'export'] as ExplainOperation[]) { + it(`a record-grained ${operation} does not ask it`, async () => { + const master = answering({ outcome: 'deny', leg: 'object_permission' }); + await explainAccess(deps({ master }), { object: OBJECT, operation, context: EXPLAINED, recordId: 'r1' }); + 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 () => { + 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 }); + expect(master).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index 7387f4ea385..eadebfe59c3 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -34,6 +34,11 @@ import { type MatchesFilterOptions, } from '@objectstack/formula'; import { ORGANIZATION_ADMIN_GRANTS } from '@objectstack/spec'; +import type { + ControlledByParentWriteDenialLeg, + ControlledByParentWriteOutcome, + ControlledByParentWriteUnresolvedReason, +} from '@objectstack/spec/contracts'; import type { FieldMaskingRule } from '@objectstack/spec/data'; import type { PermissionSet } from '@objectstack/spec/security'; import type { @@ -365,6 +370,29 @@ export interface ExplainEngineDeps { * explanation for a `delete` must consult this rather than the update gate. */ canDeleteRecord?: (object: string, recordId: string, context: any) => Promise; + /** + * [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 + * 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. + * + * Optional: a deps bag wired without it explains the update from the sharing + * gate alone, as before, and claims nothing about a master. + */ + checkControlledByParentWrite?: ( + object: string, + recordId: string, + context: any, + ) => Promise; /** * [#21771] Enforcement's own read question for an addressed by-id write: * would the read door, asked by this principal, NOT return the record? @@ -869,6 +897,70 @@ function describeOwd(schema: any): { model: string; declared: boolean; effect: ' return { model: `${String(m)} (unknown → private, fail-closed)`, declared: true, effect: 'private' }; } +/** [ADR-0055] What each refusing leg of the master-detail write check means, for the report's prose. */ +const MASTER_DENIAL_LEG_DETAIL: Record = { + object_permission: 'the principal holds no object-level update grant on a master', + row_level_security: "a master row is outside the principal's write row-level security on that master", + record_sharing: + 'record sharing does not let the principal edit a master row, and no app-authored update policy on that ' + + 'master admits it', + 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", +}; + +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 + * `controlled_by_parent` record — or `undefined` where it does not refuse. + * + * The mapping is the door's: the update 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 + * master on `allow`: the member answers `allow` for a system context on ANY + * object (the write path's first exit), so `allow` is not evidence that the + * record's access derives from a master at all. + */ +function masterWriteCheckRefusal( + outcome: ControlledByParentWriteOutcome, +): { outcome: ExplainRecordAttribution['outcome']; detail: string } | undefined { + switch (outcome?.outcome) { + case 'allow': + case 'not_applicable': + return undefined; + case 'deny': + return { + outcome: 'excluded', + detail: `${MASTER_DERIVED}: the master-detail write check the by-id update runs refuses this update on its ` + + `'${outcome.leg}' leg — ${MASTER_DENIAL_LEG_DETAIL[outcome.leg] ?? 'a leg this report does not describe'}. ` + + 'The update answers 403 PERMISSION_DENIED.', + }; + case 'unresolvable': + 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.', + }; + default: + return { + outcome: 'not_evaluated', + detail: `${MASTER_DERIVED}, and the master-detail write check answered an outcome this report does not ` + + 'recognise. The record is reported NOT writable (fail closed), never admitted.', + }; + } +} + /** * [#20431] The object's declared columns, handed to the record matcher so it * applies the spec's cross-field comparison class (`crossFieldComparisonVerdict`) @@ -1339,6 +1431,21 @@ 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 + // 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, + // 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 + ? await settle(deps.checkControlledByParentWrite(object, recordId, context)) + : undefined; + const masterFault = masterOrFault === DEPENDENCY_FAULT; + const masterRefusal = masterOrFault === undefined || masterOrFault === DEPENDENCY_FAULT + ? undefined + : masterWriteCheckRefusal(masterOrFault); // 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 @@ -1353,7 +1460,11 @@ async function applyRecordAttribution( ? `The sharing service's per-record ${engineOp === 'delete' ? 'delete' : 'update'} gate could not be ` + 'evaluated: it threw, and the by-id write fails on the same call — so the record is reported NOT ' + 'writable (fail closed), never admitted.' - : undefined); + : 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 ' + + 'record is reported NOT writable (fail closed), never admitted.' + : undefined); const anyShareAdmits = shareRules.some((r) => r.effect === 'admits'); // ── read depth: did the depth-widened owner-match admit THIS row? ──────── // The sharing read filter is ONE predicate carrying two grants: the @@ -1379,6 +1490,8 @@ async function applyRecordAttribution( sharingOutcome = 'not_evaluated'; } else if (sharingFaultDetail) { sharingOutcome = 'not_evaluated'; + } else if (masterRefusal) { + sharingOutcome = masterRefusal.outcome; } else if (owd.effect !== 'private') { sharingOutcome = 'not_evaluated'; // baseline already grants the rows sharing would add } else if (canEdit !== undefined) { @@ -1401,8 +1514,9 @@ async function applyRecordAttribution( rowFilter: sharingFaultDetail || sharingFilter === undefined ? undefined : sharingFilter, matchesRecord: sharingFaultDetail ? undefined : sharingMatches, rules: shareRules, - // A fault is only ever recorded against an existing record. - detail: sharingFaultDetail ?? (!recordExists + // A fault is only ever recorded against an existing record, and the + // master check [ADR-0055] is only ever asked of one. + detail: sharingFaultDetail ?? masterRefusal?.detail ?? (!recordExists ? 'Record not found; sharing not evaluated.' : owd.effect !== 'private' ? 'Baseline is not private — sharing adds nothing beyond it for this record.' @@ -1592,6 +1706,10 @@ async function applyRecordAttribution( // pipeline calls it: the RLS composition runs before the sharing middleware. else if (layeredFault) { visible = false; decidedBy = 'rls'; } else if (sharingFaultDetail) { visible = false; decidedBy = 'sharing'; } + // [ADR-0055] The master check refuses after the record's own row-level + // security (step 2.8 runs below the pre-image gate), and names its leg on + // the sharing layer, the record's write gate. + else if (masterRefusal) { visible = false; decidedBy = 'sharing'; } else if (!businessRowAdmits) { visible = false; decidedBy = owd.effect === 'private' ? 'sharing' : 'owd_baseline'; } else { visible = true; diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 8b1ac155b33..c9f353de6ca 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -468,7 +468,12 @@ interface RlsFilterOptions { * master", which no gate had authorized; * • `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. + * 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. * * `false`/absent leaves the floor exactly where it is. Never affects Layer 0 * (the tenant wall) or any app-authored policy. @@ -5425,9 +5430,15 @@ export class SecurityPlugin implements Plugin { // path's own inputs, so `record.visible` for `update` / `delete` is the // answer the by-id PATCH / DELETE gives: // - Layer 1 carries the pre-image gate's floor decision (step 2.7, - // {@link resolvePreImageFloorDrop}). Not `masterGateCoversThisWrite`: - // that knob VOUCHES that ADR-0055's master gate runs after the filter, - // and explain runs no master gate, so it has nothing to vouch with. + // {@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. // - 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. @@ -5451,7 +5462,8 @@ export class SecurityPlugin implements Plugin { const dropPlatformOwnershipFloor = await this.resolvePreImageFloorDrop( engineOp, o, recordId, c, sets, actsOnBehalfOf(c), ).catch(() => false); - return { dropPlatformOwnershipFloor }; + const masterGateCoversThisWrite = engineOp === 'update' && !actsOnBehalfOf(c); + return { dropPlatformOwnershipFloor, masterGateCoversThisWrite }; }; const withWriteScope = async (o: string, c: any): Promise => actsOnBehalfOf(c) ? c : { ...c, __writeScope: await this.resolveWriteScopeForSharing(o, c) }; @@ -5534,6 +5546,12 @@ 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. + checkControlledByParentWrite: (o: string, rid: string, c: any) => + this.checkControlledByParentWrite(o, rid, c), }, { object, operation, context: targetContext, recordId }, ); 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 new file mode 100644 index 00000000000..a2bb5a3915d --- /dev/null +++ b/packages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts @@ -0,0 +1,172 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// `POST /security/explain` answers an UPDATE of a `controlled_by_parent` +// record (ADR-0055) the way that record's own `PATCH` is answered, driven +// end-to-end through the REAL surfaces: better-auth sign-up members, the REST +// explain route, the generic `/data` path, plugin-sharing and plugin-security. +// +// ## What this suite pins +// +// A by-id update of a `controlled_by_parent` record meets two steps of the +// write path that the explanation used to model differently: +// +// - step 2.7 hands the record's platform ownership floor (`owner_only_writes`, +// bound to `org_member`) over to the master-detail write check, vouching +// that the check runs on the same write. explain kept the floor, so a +// member who may edit the master, but did not create the child, was +// explained as refused (`decidedBy: 'rls'`) beside a PATCH answering 200; +// - step 2.8 runs that master-detail write check. explain never asked it, +// so a member who may NOT edit the master was explained as refused only +// by the floor, on the wrong layer and with no word about the master. +// +// explain now asks the security service's own check +// (`checkControlledByParentWrite`) for the EXPLAINED principal, refuses on its +// refusal and names the leg, and vouches for the floor as step 2.7 does. Each +// verdict below sits beside the PATCH of the same record by the same +// principal. +// +// `record.visible` is the record's bottom line. `allowed` answers the object +// question (may this principal update `cpg_contract` at all), which is `true` +// for every member here; this suite does not measure it. +// +// ## Why the boot is org-bound +// +// The ownership floor binds `org_member` principals. An org-less boot measures +// the editor control in the one posture where no floor applies, where it passes +// with the floor left standing; `assertArmed` refuses it. + +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 { assertArmed, principalArmed } from './armed.js'; + +const SYS = { isSystem: true } as const; + +const OWNERSHIP_FLOOR = + "the platform's wildcard row-level update floor (`owner_only_writes`, positions ['org_member']), which a " + + 'by-id update of a controlled_by_parent record hands over to the master-detail write check'; +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.'; + +/** 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(); + let code: string | undefined; + try { + const j = JSON.parse(body) as any; + code = j?.code ?? j?.error?.code; + } catch { + code = undefined; + } + return { status: res.status, code, body }; +} + +describe('security/explain answers a controlled_by_parent update as its PATCH does (ADR-0055)', () => { + let stack: VerifyStack; + let ql: any; + let adminTok: string; + let memberTok: string; + let memberId: string; + /** A child of an admin-owned `public_read` master: the member reads it and its PATCH refuses them. */ + let contractId: string; + /** A child of a member-owned master, created by nobody the suite signs in as. */ + let memberContractId: string; + /** The control: a `public_read_write` record. */ + let boardId: string; + + const uid = async (email: string) => (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + + const explainUpdate = async (token: string, object: string, recordId: string, userId?: string) => { + const res = await stack.apiAs(token, 'POST', '/security/explain', { + object, + operation: 'update', + recordId, + ...(userId ? { userId } : {}), + }); + const text = await res.text(); + expect(res.status, `explain update ${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 }); + 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'); + memberId = await uid('cpe-member@verify.test'); + + await assertArmed([ + principalArmed({ + stack, + token: memberTok, + who: 'the member (edits their own master, not the admin\'s)', + positions: ['org_member'], + control: OWNERSHIP_FLOOR, + disarmedBy: OWNERSHIP_FLOOR_DISARM, + }), + ]); + + const adminAccountId = (await ql.insert('cpg_account', { name: 'admin account', owner_id: adminId }, { context: { ...SYS } })).id; + contractId = (await ql.insert('cpg_contract', { name: 'executed contract', account: adminAccountId }, { context: { ...SYS } })).id; + const memberAccountId = (await ql.insert('cpg_account', { name: 'member account', owner_id: memberId }, { context: { ...SYS } })).id; + memberContractId = ( + await ql.insert('cpg_contract', { name: 'member contract', account: memberAccountId }, { 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; + boardId = String(board.id ?? board.record?.id ?? board.data?.id); + }, 120_000); + + afterAll(async () => { + await stack?.stop(); + }); + + it('a child whose master the member cannot edit: explain refuses on the master leg the PATCH refuses on', async () => { + const refused = await patch(memberTok, 'cpg_contract', contractId, 'executed contract'); + expect(refused.status, `member PATCH of the child: ${refused.body}`).toBe(403); + expect(refused.code).toBe('PERMISSION_DENIED'); + expect(refused.body).toContain('requires edit access to its master record'); + expect(refused.body).toContain('(row-level security)'); + + const explained = await explainUpdate(memberTok, 'cpg_contract', contractId); + expect(explained.record).toEqual({ recordId: contractId, visible: false, decidedBy: 'sharing' }); + expect(explained.sharing?.outcome).toBe('excluded'); + expect(explained.sharing?.detail).toContain("refuses this update on its 'row_level_security' leg"); + }); + + it('explaining that member as an administrator answers the member\'s verdict, not the administrator\'s', async () => { + const forMember = await explainUpdate(adminTok, 'cpg_contract', contractId, memberId); + expect(forMember.record).toEqual({ recordId: contractId, visible: false, decidedBy: 'sharing' }); + expect(forMember.sharing?.detail).toContain("refuses this update on its 'row_level_security' leg"); + + // The administrator owns the master: their own verdict, and their own PATCH, admit. + const forSelf = await explainUpdate(adminTok, 'cpg_contract', contractId); + expect(forSelf.record).toMatchObject({ recordId: contractId, visible: true }); + const admitted = await patch(adminTok, 'cpg_contract', contractId, 'executed contract'); + expect(admitted.status, `admin PATCH of the child: ${admitted.body}`).toBe(200); + }); + + it('control: a child of the member\'s own master, which the member did not create, is explained writable and the PATCH answers 200', async () => { + const explained = await explainUpdate(memberTok, 'cpg_contract', memberContractId); + expect(explained.record).toMatchObject({ recordId: memberContractId, visible: true }); + expect(explained.sharing?.detail ?? '').not.toContain('master-detail write check'); + const admitted = await patch(memberTok, 'cpg_contract', memberContractId, 'member contract'); + expect(admitted.status, `member PATCH of a child under their own master: ${admitted.body}`).toBe(200); + }); + + it('control: a public_read_write record is explained writable and the PATCH answers 200', async () => { + const explained = await explainUpdate(memberTok, 'cpg_board', boardId); + expect(explained.record).toMatchObject({ recordId: boardId, visible: true }); + expect(explained.sharing?.detail ?? '').not.toContain('master-detail write check'); + const admitted = await patch(memberTok, 'cpg_board', boardId, 'open board'); + expect(admitted.status, `member PATCH of the board: ${admitted.body}`).toBe(200); + }); +});