diff --git a/.changeset/24647-explain-authored-row-write.md b/.changeset/24647-explain-authored-row-write.md new file mode 100644 index 00000000000..06f31e590f9 --- /dev/null +++ b/.changeset/24647-explain-authored-row-write.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(plugin-security): `security/explain`'s record-grained `update`, `delete` and `transfer` verdicts compose the by-id write gate's authored-row-write deferral, so a row an app-authored row-level write policy admits is reported writable, as its `PATCH` / `DELETE` answers + +Clause-②: no (no accept set or public surface moves: enforcement is unchanged, and explain's record verdict now gives the answer the by-id write already gives) + +- **What was wrong.** The sharing middleware's by-id write gate does not stop at record sharing's refusal: it asks the security service whether an app-authored row-level policy admits the row for that verb (`checkAuthoredRowWrite`), and on `admit` hands the row to the pre-image gate, which admits it. Explain's record-grained write verdict asked record sharing's per-record gate alone, so on a private object, for a row owned by someone else that only such a policy lets the caller write, the `PATCH` / `DELETE` landed while explain answered `record.visible: false`, decided by `sharing`. +- **What explain answers now.** The same deferral, on the same condition: only when the per-record update or delete gate refuses the row, asked through the sharing service's own probe with the context that gate is asked with. A `transfer` is deferred as an `update`, since its door is the `PATCH` that writes `owner_id`. Only the literal `admit` lifts the refusal; `abstain`, any other answer and a failed probe leave it standing. The record verdict is then `visible: true`, `decidedBy: 'rls'`, the layer that last admitted the row; the `sharing` layer still reports `excluded`, and its detail names the deferral. A sharing service without the probe defers nothing, as before. +- **What does not move.** No enforcement answer changes. The `sys_attachment` and `sys_comment` parent gates ask record sharing first and refuse on its `deny` without asking explain, so a row this deferral admits is still refused there at the record-sharing step. diff --git a/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts b/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts index 92c13a3a1f5..b57ef532847 100644 --- a/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts +++ b/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts @@ -559,15 +559,26 @@ const SHARING_READER = { userId: 'u_reader', positions: ['qa_pos'], permissions: const SHARING_WRITER = { userId: 'u_writer', positions: ['qa_pos'], permissions: ['qa_line_writer'], posture: 'MEMBER' }; /** Read and write depth `own`: the control. */ const SHARING_OWN = { userId: 'u_own', positions: ['qa_pos'], permissions: ['qa_line_own'], posture: 'MEMBER' }; +/** + * Read depth `org`, write depth `own`, and an app-authored row-level `update` + * and `delete` policy admitting every line not named `unnamed` + * ({@link bootSharing}'s `authored` rows): record sharing refuses this writer + * every line owned by someone else, and the by-id write gate defers that + * refusal to the policy. + */ +const AUTHORED_WRITER = { userId: 'u_authored', positions: ['qa_pos'], permissions: ['qa_line_authored'], posture: 'MEMBER' }; /** * A `private`-OWD object with an `owner_id`, the real `SharingService` as the * kernel's `sharing` service and its middleware on the engine after this * plugin's, as the platform boots them. Row `l_other` is owned by nobody the * rig explains. `faultReadFilter` makes the sharing service's read filter - * reject, as a share store that cannot be read does. + * reject, as a share store that cannot be read does. `authored` adds + * {@link AUTHORED_WRITER}'s set and three lines: `l_theirs` (owned by + * someone else, admitted by the policy), `l_unnamed` (owned by someone else, + * not admitted) and `l_mine` (owned by the writer). */ -async function bootSharing(opts: { faultReadFilter?: boolean } = {}) { +async function bootSharing(opts: { faultReadFilter?: boolean; authored?: boolean } = {}) { const LINE = `qa_parity_line_${next()}`; const engine = new ObjectQL(); engine.registerDriver( @@ -601,6 +612,13 @@ async function bootSharing(opts: { faultReadFilter?: boolean } = {}) { { id: 'l_other', name: 'owned by someone else', owner_id: 'u_somebody' }, { id: 'l_own', name: 'owned by the control', owner_id: 'u_own' }, ], SYS); + if (opts.authored) { + await engine.insert(LINE, [ + { id: 'l_theirs', name: 'theirs', owner_id: 'u_somebody' }, + { id: 'l_unnamed', name: 'unnamed', owner_id: 'u_somebody' }, + { id: 'l_mine', name: 'mine', owner_id: AUTHORED_WRITER.userId }, + ], SYS); + } const sets = [ MEMBER_DEFAULT, @@ -613,6 +631,18 @@ async function bootSharing(opts: { faultReadFilter?: boolean } = {}) { name: 'qa_line_own', objects: { [LINE]: { allowRead: true, allowEdit: true, readScope: 'own', writeScope: 'own' } }, }), + ...(opts.authored + ? [PermissionSetSchema.parse({ + name: 'qa_line_authored', + objects: { + [LINE]: { allowRead: true, allowEdit: true, allowDelete: true, allowTransfer: true, readScope: 'org', writeScope: 'own' }, + }, + rowLevelSecurity: [ + { name: 'qa_line_named_updates', object: LINE, operation: 'update', using: "name != 'unnamed'" }, + { name: 'qa_line_named_deletes', object: LINE, operation: 'delete', using: "name != 'unnamed'" }, + ], + })] + : []), ]; let security: unknown; let sharing: SharingService | undefined; @@ -645,11 +675,21 @@ async function bootSharing(opts: { faultReadFilter?: boolean } = {}) { engine.registerMiddleware(buildSharingMiddleware(sharing, ctx.logger) as never, { object: '*' }); return { - explain: (caller: object, operation: 'read' | 'update', recordId: string) => + explain: (caller: object, operation: 'read' | 'update' | 'delete' | 'transfer', recordId: string) => explained(plugin.explainAccessForCaller({ object: LINE, operation, recordId }, { ...caller })), find: (caller: object, id: string) => rowsOf(engine.find(LINE, { where: { id }, context: { ...caller } } as never)), update: (caller: object, id: string) => landed(engine.update(LINE, { name: 'renamed' }, { where: { id }, context: { ...caller } } as never)), + remove: (caller: object, id: string) => landed(engine.delete(LINE, { where: { id }, context: { ...caller } } as never)), + /** A transfer is a by-id update of `owner_id`, gated by the transfer grant. */ + transfer: (caller: object, id: string) => + landed(engine.update(LINE, { owner_id: 'u_recipient' }, { where: { id }, context: { ...caller } } as never)), + /** + * Record sharing's tri-state update verdict, asked as the `sys_attachment` + * and `sys_comment` parent gates ask it before anything else: the caller's + * envelope with no operation-private key. + */ + checkEdit: (caller: object, id: string) => sharing!.checkEdit(LINE, id, { ...caller } as never), teardown: async () => { try { await engine.destroy(); } catch { /* noop */ } releaseAsyncContexts(engine, plugin); @@ -700,7 +740,7 @@ const withRls = (predicate: string, f: (rig: RlsRig) => Promise, opts: try { return { ...(await f(rig)), teardown: rig.teardown }; } catch (e) { await rig.teardown(); throw e; } }; const withSharing = ( - opts: { faultReadFilter?: boolean }, + opts: { faultReadFilter?: boolean; authored?: boolean }, f: (rig: Awaited>) => Promise, ): RowRun => async () => { const rig = await bootSharing(opts); @@ -935,6 +975,49 @@ const TABLE: Row[] = [ explain: await r.explain(SHARING_OWN, 'update', 'l_other'), enforce: await r.update(SHARING_OWN, 'l_other'), })), }, + // #24647: the by-id write gate defers record sharing's refusal to an + // app-authored row-level write policy that admits the row (ADR-0105 D3). + { + card: '#24647', shape: 'private OWD, an `own` writer whose authored update policy admits a row owned by someone else, update', + position: 'record.visible', + enforced: 'admitted', + run: withSharing({ authored: true }, async (r) => ({ + explain: await r.explain(AUTHORED_WRITER, 'update', 'l_theirs'), enforce: await r.update(AUTHORED_WRITER, 'l_theirs'), + })), + }, + { + card: '#24647', shape: 'private OWD, an `own` writer whose authored delete policy admits a row owned by someone else, delete', + position: 'record.visible', + enforced: 'admitted', + run: withSharing({ authored: true }, async (r) => ({ + explain: await r.explain(AUTHORED_WRITER, 'delete', 'l_theirs'), enforce: await r.remove(AUTHORED_WRITER, 'l_theirs'), + })), + }, + { + card: '#24647 control', shape: 'private OWD, the same writer, a row owned by someone else that no authored policy admits, update', + position: 'record.visible', + enforced: REFUSED_DENIED, + run: withSharing({ authored: true }, async (r) => ({ + explain: await r.explain(AUTHORED_WRITER, 'update', 'l_unnamed'), enforce: await r.update(AUTHORED_WRITER, 'l_unnamed'), + })), + }, + { + card: '#24647 control', shape: 'private OWD, the same writer, a row owned by someone else that no authored policy admits, delete', + position: 'record.visible', + enforced: REFUSED_DENIED, + run: withSharing({ authored: true }, async (r) => ({ + explain: await r.explain(AUTHORED_WRITER, 'delete', 'l_unnamed'), enforce: await r.remove(AUTHORED_WRITER, 'l_unnamed'), + })), + }, + { + // A transfer's door is the by-id update of `owner_id`, deferred as an update. + card: '#24647', shape: 'private OWD, an `own` writer holding the transfer grant whose authored update policy admits a row owned by someone else, transfer', + position: 'record.visible', + enforced: 'admitted', + run: withSharing({ authored: true }, async (r) => ({ + explain: await r.explain(AUTHORED_WRITER, 'transfer', 'l_theirs'), enforce: await r.transfer(AUTHORED_WRITER, 'l_theirs'), + })), + }, // #20002: a dependency enforcement shares with explain throws. { card: '#20002', shape: 'the sharing read filter throws, the caller\'s own row, read', position: 'record.visible', @@ -999,3 +1082,65 @@ describe('security.explain answers what enforcement does — the enumeration', ( }, 60_000); } }); + +/** + * #24647's row, beyond the verdict the table compares: what the record story + * names, and what the `sys_attachment` and `sys_comment` parent gates answer. + * + * Both gates ask record sharing's `checkEdit` FIRST, with the caller's envelope + * and no operation-private key, refuse on its `deny` without asking anything + * further, and ask explain's record-grained `update` verdict only once that + * step admits (each gate's own parity table pins it: its `sharing deny` row + * refuses with explain never called, though the double would admit). So on a + * row the deferral admits, record sharing's own `deny` still decides both + * gates: explain admitting it cannot widen either one. That holds for today's + * gate composition only, and the case below says which card changes it. + */ +describe('an app-authored row-level write policy the by-id write gate defers to (ADR-0105 D3)', () => { + it('the update story names the deferral: record sharing excludes the row, row-level security admits it', async () => { + const rig = await bootSharing({ authored: true }); + try { + const x = await rig.explain(AUTHORED_WRITER, 'update', 'l_theirs'); + expect(x.kind, describeExplained(x)).toBe('decision'); + if (x.kind !== 'decision') return; + expect(x.decision.record).toEqual({ recordId: 'l_theirs', visible: true, decidedBy: 'rls' }); + const sharingRecord = x.decision.layers.find((l) => l.layer === 'sharing')?.record; + expect(sharingRecord?.outcome).toBe('excluded'); + expect(sharingRecord?.detail).toMatch(/app-authored row-level update policy admits this record/); + expect(x.decision.layers.find((l) => l.layer === 'rls')?.record?.outcome).toBe('admitted'); + } finally { + await rig.teardown(); + } + }, 60_000); + + it("the parent gates' first question, record sharing, still refuses the row explain now admits", async () => { + const rig = await bootSharing({ authored: true }); + try { + // ⚠️ A sibling card flips this pin: #24646 (ruling B, comment 6279880675) + // drops both gates' own-depth record-sharing step and their master step, + // so they ask only explain's record-grained by-id `update` verdict. The + // gate then admits exactly what the parent's by-id update admits; for + // this scenario, with #24647's deferral, that is ADMITTED. Until then the + // gates' first question below is record sharing's, and it refuses. + expect(await rig.checkEdit(AUTHORED_WRITER, 'l_theirs')).toBe('deny'); + const x = await rig.explain(AUTHORED_WRITER, 'update', 'l_theirs'); + expect(x.kind === 'decision' ? x.decision.record?.visible : x.kind, describeExplained(x)).toBe(true); + expect(await rig.update(AUTHORED_WRITER, 'l_theirs')).toEqual({ kind: 'admitted' }); + } finally { + await rig.teardown(); + } + }, 60_000); + + it('the positive control: a row record sharing admits is admitted by the first question, by explain and by the door', async () => { + const rig = await bootSharing({ authored: true }); + try { + expect(await rig.checkEdit(AUTHORED_WRITER, 'l_mine')).toBe('allow'); + const x = await rig.explain(AUTHORED_WRITER, 'update', 'l_mine'); + expect(x.kind === 'decision' ? x.decision.record : x.kind, describeExplained(x)) + .toEqual({ recordId: 'l_mine', visible: true, decidedBy: 'rls' }); + expect(await rig.update(AUTHORED_WRITER, 'l_mine')).toEqual({ kind: 'admitted' }); + } finally { + await rig.teardown(); + } + }, 60_000); +}); diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index 08118f7e662..bd7700857b5 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -36,6 +36,8 @@ import { } from '@objectstack/formula'; import { ORGANIZATION_ADMIN_GRANTS } from '@objectstack/spec'; import type { + AuthoredRowWriteOperation, + AuthoredRowWriteVerdict, ControlledByParentWriteDenialLeg, ControlledByParentWriteOutcome, ControlledByParentWriteUnresolvedReason, @@ -380,6 +382,33 @@ export interface ExplainEngineDeps { * explanation for a `delete` must consult this rather than the update gate. */ canDeleteRecord?: (object: string, recordId: string, context: any) => Promise; + /** + * [#5493 / ADR-0105 D3] The by-id write gate's DEFERRAL: whether an + * APP-AUTHORED row-level policy admits the row for the refusing gate's verb + * (`ISecurityService.checkAuthoredRowWrite`, reached through the sharing + * service's own probe). The sharing middleware asks it only after its + * per-record gate refused the row, and only the literal `admit` lifts that + * refusal: the row then goes on to the pre-image gate, which is the + * row-level security this report's `rls` layer explains. + * + * Asked on the same condition here: for a by-id `update` or `delete` whose + * gate above ({@link canEditRecord} / {@link canDeleteRecord}) answered + * `false`, with that gate's verb and the context being EXPLAINED, and for a + * `transfer` as an `update` (its door is the by-id update of `owner_id`, + * which the update gate judges). Every + * other answer leaves the refusal standing, and so does a rejection: the + * door's probe catches its own failure and keeps its refusal, so a failing + * probe is not a failing request and is not reported as one. + * + * Optional: a deps bag wired without it reports the gate's refusal as the + * write verdict, as before. + */ + probeAuthoredRowWrite?: ( + object: string, + recordId: string, + operation: AuthoredRowWriteOperation, + context: any, + ) => Promise; /** * [ADR-0055] The master-detail write check's own answer for a by-id UPDATE * of `recordId` by `context`: `ISecurityService.checkControlledByParentWrite`, @@ -1558,6 +1587,21 @@ async function applyRecordAttribution( : undefined; const writeGateFault = canEditOrFault === DEPENDENCY_FAULT; const canEdit = canEditOrFault === DEPENDENCY_FAULT ? undefined : canEditOrFault; + // [#5493 / ADR-0105 D3] The by-id write gate DEFERS before it refuses: when + // its per-record gate refuses an `update` or a `delete`, the sharing + // middleware asks whether an app-authored row-level policy admits the row + // for that verb, and only `admit` lifts the refusal, handing the row to the + // pre-image gate (the `rls` layer below). Asked on that condition alone, so + // a row the gate admits, abstains on or faulted on is explained as before; + // every answer but `admit`, and a rejection, keeps the refusal, as at the door. + // A transfer's door is the by-id update of `owner_id`, so it is deferred as + // an update; restore and purge have no by-id door (the object gate refuses + // both), so nothing defers them. + const authoredDeferralVerb: AuthoredRowWriteOperation | undefined = + engineOp === 'update' || engineOp === 'transfer' ? 'update' : engineOp === 'delete' ? 'delete' : undefined; + const authoredRowWriteAdmits = canEdit === false && authoredDeferralVerb && deps.probeAuthoredRowWrite + ? (await deps.probeAuthoredRowWrite(object, recordId, authoredDeferralVerb, context).catch(() => 'abstain')) === 'admit' + : false; // [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 @@ -1697,7 +1741,11 @@ async function applyRecordAttribution( ? `The write gate admits this record via the Modify All Data bypass [${vamaSets.join(', ')}], ` + 'not ownership or a share (see the vama_bypass layer).' : 'The sharing service grants write on this record (ownership or an edit/full share).') - : 'No ownership and no edit/full share grants write on this record.') + : 'No ownership and no edit/full share grants write on this record.' + + (authoredRowWriteAdmits + ? ` The by-id ${engineOp} does not stop at that refusal: an app-authored row-level ${authoredDeferralVerb} ` + + 'policy admits this record, so the write gate defers to row-level security (see the rls layer).' + : '')) : depthAdmitsRow ? `${shareRules.length} share(s) attached; none grants the caller access to this record — read depth ` + `'${depthScope}' already admits it, so sharing adds nothing here (see the depth layer).` @@ -1865,7 +1913,7 @@ async function applyRecordAttribution( const businessRowAdmits = isRead ? owd.effect !== 'private' || ownerIsMe || sharingOutcome === 'admitted' || depthAdmitsRow || vamaAdmitsRow : canEdit !== undefined - ? canEdit + ? canEdit || authoredRowWriteAdmits : owd.effect === 'public' || ownerIsMe || sharingOutcome === 'admitted' || vamaAdmitsRow; // [#4647] Was the bypass DECISIVE? Only where the baseline and the concrete @@ -1893,6 +1941,9 @@ async function applyRecordAttribution( // the same layer. else if (masterLegRefusal) { visible = false; decidedBy = 'sharing'; } else if (!businessRowAdmits) { visible = false; decidedBy = owd.effect === 'private' ? 'sharing' : 'owd_baseline'; } + // [#5493 / ADR-0105 D3] A row the write gate refused and its deferral + // admitted was last admitted by the app-authored row-level policy. + else if (authoredRowWriteAdmits) { visible = true; decidedBy = 'rls'; } else { visible = true; // `depth` ranks with `sharing`, the other layer that widens a private diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index d31b3ba9c4d..56d547d5ed1 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -4964,6 +4964,9 @@ export class SecurityPlugin implements Plugin { // - 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. + // - [#5493 / ADR-0105 D3] A refusal of that gate is deferred as the + // middleware defers it: to an app-authored row-level policy that admits + // the row for the gate's verb (`probeAuthoredRowWrite`, wired below). // [#19986] The READ verdict's twin: plugin-sharing's read filter is asked // with `__readScope` stamped as step 2.6 stamps it for a find, so // `record.visible` for `read` is what the caller's find returns. @@ -5071,6 +5074,18 @@ export class SecurityPlugin implements Plugin { ...(sharing && typeof sharing.canDelete === 'function' ? { canDeleteRecord: async (o: string, rid: string, c: any) => sharing.canDelete(o, rid, await withWriteScope(o, c)) } : {}), + // [#5493 / ADR-0105 D3] The by-id write gate's deferral, asked through + // the sharing service's own probe, the member its middleware asks + // after the gate above refuses, with the context that gate is asked + // with. The probe passes through to {@link checkAuthoredRowWrite} and + // abstains on every failure; a sharing service without it defers + // nothing, so explain reports the gate's refusal as before. + ...(sharing && typeof sharing.probeAuthoredRowWrite === 'function' + ? { + probeAuthoredRowWrite: async (o: string, rid: string, op: AuthoredRowWriteOperation, c: any) => + sharing.probeAuthoredRowWrite(o, rid, op, await withWriteScope(o, c)), + } + : {}), // [#21771] The by-id write path's read question, asked the same way: // the caller-context by-id read, every data middleware included. recordAbsentToCaller: (o: string, rid: string, c: any) =>