From 7da68682b143e257f692603f9d8e397d0cf899b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 03:02:29 +0000 Subject: [PATCH 1/5] fix(plugin-security): explain's on-behalf-of by-id write verdict composes the delegated write path The record-grained update/delete verdict for an on-behalf-of principal now asks plugin-sharing's per-record gate as the by-id write does (agent leg at its intersected depth, then the delegator at the delegator's depth), and ANDs the delegator's own write filter beside the principal's, as step 2.7 does. Measured on a real stack before the fix: 4 of 52 cells forked (both postures), in both directions; after: 0 of 52. Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude --- .../plugin-security/src/explain-engine.ts | 62 +++++++++++---- .../plugin-security/src/security-plugin.ts | 76 ++++++++++++++++--- 2 files changed, 116 insertions(+), 22 deletions(-) diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index bd7700857b5..69b8c4dfead 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -1404,6 +1404,14 @@ interface RecordAttributionContext { * object-level detail and its record block name one depth. */ depthScope: string; + /** + * [#24646 / ADR-0090 D10] The delegator behind an on-behalf-of principal — + * its resolved sets and its context, as the object-level pass resolved them + * — or `undefined` on the ordinary path. A by-id write's pre-image re-read + * (step 2.7) ANDs the delegator's own write filter beside the principal's, + * so the record verdict of an `update` or a `delete` composes it too. + */ + delegator?: { sets: PermissionSet[]; context: any }; } /** @@ -1425,7 +1433,7 @@ async function applyRecordAttribution( ): Promise<{ record: NonNullable; posture: AuthzPosture }> { const { deps, object, recordId, engineOp, rlsOp, context, sets, layers, owd, capsDeny, crudAllowed, vamaEffective, vamaSets, - declaredColumns, depthScope, + declaredColumns, depthScope, delegator, } = ra; const isRead = engineOp === 'find'; const posture = derivePosture(context); @@ -1468,6 +1476,23 @@ async function applyRecordAttribution( const layered = layeredOrFault === DEPENDENCY_FAULT ? undefined : layeredOrFault; const layer0 = layered?.layer0; const layer1 = layered?.layer1; + // [#24646 / ADR-0090 D10] An on-behalf-of by-id write may only touch a row the + // delegator could touch: step 2.7 composes the DELEGATOR's write filter (its + // own sets, its own context, no floor option) and ANDs it into the same + // pre-image re-read as the principal's. Composed here by that same call, for + // the verbs step 2.7 judges, and judged on the `rls` layer with the + // principal's Layer 1. A rejection fails the write, so it is reported fail + // closed, as a rejection of the principal's own composition is. + const delegatorWriteOrFault = delegator && recordExists && (rlsOp === 'update' || rlsOp === 'delete') + ? await settle(deps.computeRlsFilter(delegator.sets, object, rlsOp, delegator.context)) + : undefined; + const delegatorWriteFault = delegatorWriteOrFault === DEPENDENCY_FAULT; + const delegatorWriteFilter = delegatorWriteOrFault === DEPENDENCY_FAULT ? undefined : (delegatorWriteOrFault ?? undefined); + /** The business row filter this record's verdict is judged on: the principal's Layer 1, and the delegator's write filter beside it. */ + const recordLayer1 = delegatorWriteFilter + ? (layer1 != null ? { $and: [layer1, delegatorWriteFilter] } : delegatorWriteFilter) + : layer1; + const rlsFault = layeredFault || delegatorWriteFault; const layeredFaultDetail = (layerName: string): string => `${layerName} could not be evaluated: the row-level security composition threw, and enforcement ` + 'fails the request on the same call — so the record is reported NOT visible (fail closed), never admitted.'; @@ -1854,22 +1879,28 @@ async function applyRecordAttribution( if (rlsLayer) { if (!recordExists) { rlsLayer.record = { outcome: 'not_evaluated', rules: [], detail: 'Record not found; business RLS not evaluated.' }; - } else if (layeredFault) { - rlsLayer.record = { outcome: 'not_evaluated', rules: [], detail: layeredFaultDetail('Business RLS') }; - } else if (layered === undefined) { + } else if (rlsFault) { + rlsLayer.record = { + outcome: 'not_evaluated', + rules: [], + detail: layeredFault + ? layeredFaultDetail('Business RLS') + : layeredFaultDetail("The delegator's write filter (ADR-0090 D10)"), + }; + } else if (layered === undefined && !delegatorWriteFilter) { rlsLayer.record = { outcome: 'not_evaluated', rules: [], detail: 'Business RLS split is unavailable on this engine build.' }; - } else if (layer1 == null) { + } else if (recordLayer1 == null) { rlsLayer.record = { outcome: 'not_evaluated', rowFilter: null, rules: [], detail: 'No business RLS policy applies to this record.' }; } else { - const deny = isDenyAll(layer1); - const m = matches(layer1); + const deny = isDenyAll(recordLayer1); + const m = matches(recordLayer1); const excluded = deny || m === false; // [#24164 ruling E′] An object's default row scope is named as itself, // reached through the object's declaration rather than a held set. - const [objectDefault] = objectDefaultRowScopesOf([layer1]); + const [objectDefault] = objectDefaultRowScopesOf([layer1, delegatorWriteFilter].filter(Boolean) as Record[]); rlsLayer.record = { outcome: excluded ? 'excluded' : 'admitted', - rowFilter: layer1, + rowFilter: recordLayer1, matchesRecord: deny ? false : m, rules: [ objectDefault @@ -1877,16 +1908,20 @@ async function applyRecordAttribution( kind: 'rls_policy', name: objectDefault.name ?? '(unnamed)', via: 'object_default', - predicate: layer1, + predicate: recordLayer1, effect: excluded ? 'excludes' : 'admits', } - : { kind: 'rls_policy', name: 'business_rls', predicate: layer1, effect: excluded ? 'excludes' : 'admits' }, + : { kind: 'rls_policy', name: 'business_rls', predicate: recordLayer1, effect: excluded ? 'excludes' : 'admits' }, ], detail: (deny ? 'Business RLS composes to DENY ALL for this principal.' : excluded ? 'The record does not satisfy the business row-level predicate.' : 'The record satisfies the business row-level predicate.') + + (delegatorWriteFilter + ? ` On this on-behalf-of ${engineOp}, the predicate is the principal's business RLS AND the delegator's ` + + 'own write filter, as the by-id write composes them (ADR-0090 D10).' + : '') + (objectDefault ? objectDefaultRowScopeSource(object, [objectDefault]) : ''), }; } @@ -1932,7 +1967,7 @@ async function applyRecordAttribution( else if (rlsExcluded) { visible = false; decidedBy = 'rls'; } // A dependency that threw decides by failing the request, in the order the // pipeline calls it: the RLS composition runs before the sharing middleware. - else if (layeredFault) { visible = false; decidedBy = 'rls'; } + else if (rlsFault) { 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 @@ -1955,7 +1990,7 @@ async function applyRecordAttribution( ? 'sharing' : depthAdmitsRow ? 'depth' - : layer1 != null + : recordLayer1 != null ? 'rls' : owd.effect === 'private' && ownerIsMe ? 'owd_baseline' @@ -2652,6 +2687,7 @@ async function explainOnce(deps: ExplainEngineDeps, input: ExplainInput, asker: deps: askerCannotRead || principalCannotRead ? { ...deps, fetchRecord: async () => null } : deps, object, recordId, engineOp: dataOp, rlsOp, context, sets, layers, owd, capsDeny, crudAllowed, vamaEffective, vamaSets, declaredColumns, depthScope: scope, + ...(delegatorSets && delegatorContextForRls ? { delegator: { sets: delegatorSets, context: delegatorContextForRls } } : {}), }); recordVerdict = out.record; posture = out.posture; diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 56d547d5ed1..1b4450205f1 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -4970,10 +4970,14 @@ export class SecurityPlugin implements Plugin { // [#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. - // The on-behalf-of path keeps its previous inputs on every half. The gate - // itself excludes it from the floor decision, and the middleware's - // delegated depths (the agent-leg intersection, plus a second gate call or - // filter as the delegator) are not modelled on explain's record path. + // [#24646] On the on-behalf-of path the per-record WRITE gate is asked as + // the by-id write asks it ({@link delegatedWriteGate}): once for the + // delegated principal at its agent-leg depth, then, if that admits, once + // for the delegator at the delegator's own depth. The delegator's own + // write filter, which step 2.7 ANDs into the pre-image re-read, is composed + // by the engine. The floor decision still excludes this path, as the gate + // does. The delegated READ depths are not modelled on explain's record + // path: the read filter is asked with the delegated context as it stands. const actsOnBehalfOf = (c: any): boolean => !!c?.onBehalfOf?.userId; const recordWriteFloorOptions = async ( sets: PermissionSet[], @@ -4999,6 +5003,55 @@ export class SecurityPlugin implements Plugin { }; const withWriteScope = async (o: string, c: any): Promise => actsOnBehalfOf(c) ? c : { ...c, __writeScope: await this.resolveWriteScopeForSharing(o, c) }; + /** + * [#24646 / ADR-0090 D10] The two contexts the by-id write path hands + * plugin-sharing's per-record gate for an on-behalf-of principal, built as + * the write path builds them: + * + * - `agent`: the delegated context with step 2.6's two stamps, the + * agent-leg write depth ({@link intersectDelegatedScope} of the ceiling's + * DECLARED depth and the delegator's effective one) as `__writeScope`, and + * the delegator's effective depth as `__delegatorWriteScope`; + * - `delegator`: what the sharing middleware's second gate call hands the + * service, that same context as the delegator (`userId` the delegator's, + * no delegation link) at the delegator's depth. + * + * A delegation link naming no user (`missing`) answers `null`: the write + * path refuses such a principal before any gate runs, and so does every + * gate built on this. + */ + const delegatedWriteContexts = async (o: string, c: any): Promise<{ agent: any; delegator: any } | null> => { + const del = await resolveDelegatorContext(this.ql, c); + if (del.kind !== 'resolved') return null; + const [sets, delegatorSets, meta] = await Promise.all([ + this.resolvePermissionSetsForContext(c), + this.resolvePermissionSetsForContext(del.context), + this.getObjectSecurityMeta(o), + ]); + const delegatorScope = this.permissionEvaluator.getEffectiveScope('write', o, delegatorSets, { isPrivate: meta.isPrivate }); + const declared = this.permissionEvaluator.getDeclaredScope('write', o, sets, { isPrivate: meta.isPrivate }); + const agent = { + ...c, + __writeScope: intersectDelegatedScope(declared, delegatorScope).agentLegScope, + __delegatorWriteScope: delegatorScope, + }; + return { agent, delegator: { ...agent, userId: c.onBehalfOf.userId, onBehalfOf: undefined, __writeScope: delegatorScope } }; + }; + /** + * [#24646] A per-record write gate of the sharing service, asked as the + * by-id write asks it: for an ordinary principal once, at its own write + * depth; for an on-behalf-of principal as the agent and then, only if that + * admits, as the delegator — the sharing middleware's two calls, in its + * order ({@link delegatedWriteContexts}). + */ + const delegatedWriteGate = (gate: (o: string, rid: string, c: any) => Promise) => + async (o: string, rid: string, c: any): Promise => { + if (!actsOnBehalfOf(c)) return gate(o, rid, await withWriteScope(o, c)); + const legs = await delegatedWriteContexts(o, c); + if (!legs) return false; + if (!(await gate(o, rid, legs.agent))) return false; + return gate(o, rid, legs.delegator); + }; // {@link resolveSharingReadFilter} IS the read-depth stamp step 2.6 makes // (the same two inputs to the same evaluator call); the analytics path // already asks it. The caller's own key is cleared first, so the depth @@ -5067,23 +5120,28 @@ export class SecurityPlugin implements Plugin { ? { listRecordShares: (o: string, rid: string) => sharing.listShares(o, rid, { isSystem: true }) } : {}), ...(sharing && typeof sharing.canEdit === 'function' - ? { canEditRecord: async (o: string, rid: string, c: any) => sharing.canEdit(o, rid, await withWriteScope(o, c)) } + ? { canEditRecord: delegatedWriteGate((o, rid, c) => sharing.canEdit(o, rid, c)) } : {}), // [ADR-0111 D3] The narrower delete gate — an edit share opens update // but not delete, so a delete explanation must consult this. ...(sharing && typeof sharing.canDelete === 'function' - ? { canDeleteRecord: async (o: string, rid: string, c: any) => sharing.canDelete(o, rid, await withWriteScope(o, c)) } + ? { canDeleteRecord: delegatedWriteGate((o, rid, c) => sharing.canDelete(o, rid, 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. + // nothing, so explain reports the gate's refusal as before. [#24646] An + // on-behalf-of principal's probe carries the agent leg's stamps, as the + // middleware's own context does when it asks. ...(sharing && typeof sharing.probeAuthoredRowWrite === 'function' ? { - probeAuthoredRowWrite: async (o: string, rid: string, op: AuthoredRowWriteOperation, c: any) => - sharing.probeAuthoredRowWrite(o, rid, op, await withWriteScope(o, c)), + probeAuthoredRowWrite: async (o: string, rid: string, op: AuthoredRowWriteOperation, c: any) => { + if (!actsOnBehalfOf(c)) return sharing.probeAuthoredRowWrite(o, rid, op, await withWriteScope(o, c)); + const legs = await delegatedWriteContexts(o, c); + return legs ? sharing.probeAuthoredRowWrite(o, rid, op, legs.agent) : 'abstain'; + }, } : {}), // [#21771] The by-id write path's read question, asked the same way: From ba6fb4912cf0f98ca7451287fc60ae9de14c8084 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 03:08:48 +0000 Subject: [PATCH 2/5] feat(service-storage, plugin-audit): the parent gates ask the parent's by-id update judgement alone The sys_attachment parent gate and the sys_comment moderation gate drop their own-depth record-sharing step and their master-detail step wherever the security service serves explain: may-edit-the-parent is the record- grained by-id update verdict of the parent alone. A kernel whose security service serves no explain keeps the previous composition exactly. The ISharingService contract prose records the superseded own-depth rule. Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude --- .../plugin-audit/src/comment-access-hooks.ts | 177 +++++++++------- .../src/comment-parent-update-parity.test.ts | 144 ++++++++----- .../src/attachment-access-hooks.test.ts | 171 +++++++++------ .../src/attachment-access-hooks.ts | 195 ++++++++++-------- .../spec/src/contracts/sharing-service.ts | 59 +++--- 5 files changed, 452 insertions(+), 294 deletions(-) diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index eeccb78a99e..153158903b5 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -56,34 +56,41 @@ * ## Parent EDIT * * The moderation limb asks the question the attachment kit's four limbs ask — - * may the caller edit the parent record? — and answers it the same way: - * plugin-sharing's TRI-STATE `checkEdit` first (`allow` admits, `deny` - * refuses); on `abstain` (record sharing does not enforce on the parent), the - * security service's ADR-0055 master-detail write check - * (`ISecurityService.checkControlledByParentWrite`, the answer the parent's own - * by-id update gets), whose `allow` and `not_applicable` admit and whose `deny` - * and `unresolvable` refuse; a rejection propagates unchanged. A kernel whose - * security service does not serve that member composes no master check, so the - * abstention admits there, as the parent's own update would. + * may the caller edit the parent record? — and answers it the same way: with + * the answer a by-id UPDATE of the parent record, by the same caller, gets. * - * [#24549] Whatever those steps admit (the degraded read included), the - * parent's own by-id UPDATE must admit too, and it decides last: the security - * service's judgement of that update, `ISecurityService.explain`'s - * record-grained verdict for `update` on the parent — `allowed` and - * `record.visible` both. That verdict is the write path's own composition (the - * parent object's grant, its row-level security, its record sharing and its - * master), so the moderation limb gives the answer a by-id update of the - * parent gets, and the gate keeps no copy of that judgement — the same step - * the attachment kit takes. Asked only for a caller the steps above admit, it - * adds refusals and never admits one; record sharing's `deny`, the master - * check's refusals, the owner-only edit DEPTH (the #7144 ruling below) and a - * rejection keep their answers. A kernel with no security service meets no - * such judgement on the parent's update either, and a security service that - * serves no `explain` is not asked it. ⛔ Never `canEdit`: - * it folds `abstain` into `true`, and `effectiveSharingModel` maps every - * `controlled_by_parent` parent to `public`, so that fold let any member holding - * the `sys_comment` delete bit remove other people's comments from every - * master-detail child record of the org. + * [#24646] The security service's judgement of that update decides it alone: + * `ISecurityService.explain`'s record-grained verdict for `update` on the + * parent, which admits only when `allowed` and `record.visible` are both + * `true` — a decision that carries no record verdict did not judge the record, + * and refuses. That verdict is the write path's own composition: the parent + * object's grant, its row-level security, its record sharing at the caller's + * real write DEPTH, the deferral to an app-authored row-level write policy, + * the master of a `controlled_by_parent` parent (ADR-0055), and both legs of an + * on-behalf-of write (ADR-0090 D10). So a caller who can update the parent + * record by id can also moderate its comments; a caller who cannot, still + * cannot. A rejection propagates unchanged. The gate keeps no copy of that + * judgement: until #24646 it also asked record sharing at owner-only depth and + * then the master-detail check, ahead of the judgement — a narrower duplicate + * that refused a caller with `unit`, `unit_and_below` or `org` write depth on + * the parent, whom the CRUD path admits. The ruling that kept it (#7144) is + * superseded for these gates. With no sharing service, caller-scoped parent + * READ visibility is asked first (the degraded mode), then the judgement. + * + * A security service that serves no `explain`, or none at all, judges no by-id + * update here. There the gate keeps the composition it had before it could ask + * one: plugin-sharing's TRI-STATE `checkEdit` (`allow` admits, `deny` refuses); + * on `abstain` (record sharing does not enforce on the parent), the ADR-0055 + * master-detail write check (`ISecurityService.checkControlledByParentWrite`), + * whose `allow` and `not_applicable` admit and whose `deny` and `unresolvable` + * refuse, while a kernel that does not serve it lets the abstention admit, as + * the parent's own update would; with no sharing service, the degraded read + * alone. ⛔ Dropping those steps on such a kernel would admit every caller, so + * they stay there. ⛔ On that path, never `canEdit`: it folds `abstain` into + * `true`, and `effectiveSharingModel` maps every `controlled_by_parent` parent + * to `public`, so that fold let any member holding the `sys_comment` delete bit + * remove other people's comments from every master-detail child record of the + * org. * * Everything fails CLOSED. A `thread_id` that names no parseable parent (the * dangling `"crm_opportunity:"` of #4630, a free-form thread, a thread on @@ -184,7 +191,9 @@ export interface CommentReadMiddlewareCtx { /** * The single method of plugin-sharing's contract this gate consults — the - * TRI-STATE edit verdict (see "Parent EDIT" above). Taken as a `Pick` of the + * TRI-STATE edit verdict (see "Parent EDIT" above), asked only where the + * security service judges no by-id update; its presence alone also decides + * whether the degraded read is asked. Taken as a `Pick` of the * real `ISharingService` rather than re-declared locally: the narrow surface is * the point, a second hand-written shape for it is not (AGENTS.md PD #12 — one * contract, no dialects). @@ -192,17 +201,20 @@ export interface CommentReadMiddlewareCtx { export type CommentSharingLike = Pick; /** - * The two members of the security service's contract this gate consults: the - * ADR-0055 master-detail write check, for a `controlled_by_parent` parent - * (OPTIONAL on the contract, so the gate handles its absence by construction), - * and `explain`, whose record-grained `update` verdict is the parent's own - * by-id update judgement (see "Parent EDIT" above). `explain` is - * feature-detected like every member here, so it is optional on this view of - * the contract. + * The two members of the security service's contract this gate consults: + * `explain`, whose record-grained `update` verdict is the parent's own by-id + * update judgement and decides the question wherever it is served (see "Parent + * EDIT" above), and the ADR-0055 master-detail write check, asked only where no + * `explain` is served (OPTIONAL on the contract, so the gate handles its + * absence by construction). `explain` is feature-detected like every member + * here, so it is optional on this view of the contract. */ export type CommentSecurityLike = Pick & Partial>; +/** A security service known to serve `explain` — the one the parent's by-id update judgement is asked of. */ +type ExplainingSecurity = CommentSecurityLike & Pick; + export interface CommentAccessLogger { info(msg: string, meta?: unknown): void; warn(msg: string, meta?: unknown): void; @@ -637,18 +649,19 @@ function asIdList(id: unknown): Array | null { * now for the same reason — the envelope is the contract's unit. * * Note what deliberately did NOT change: no access DEPTH is synthesised for the - * parent object. Absent depth leaves the sharing owner-match at its narrowest - * (`own`) — the safe direction, and byte-for-byte the behaviour the projection - * produced. + * parent object, and none is carried across from `sys_comment`. * - * [#7144] That is now the RULED shape, not a pending question: the maintainer's - * ruling of 2026-08-10 keeps these gates at `own`, deliberately tighter than - * the parent's real edit authority. The reasoning — including the fail-open in - * `ISecurityService.resolveWriteScope` that makes the widening unsafe to wire - * today — is recorded once on the contract that owns this gate's meaning - * (`@objectstack/spec` — `ISharingService`, "Write DEPTH is an input the CALLER - * supplies"), because the `sys_attachment` kit reaches the same gate from - * another package; it is not restated here. */ + * [#24646] The parent's own depth is the security service's to resolve, and it + * does: the by-id update judgement this gate asks resolves it together with the + * parent object's grant, so the fail-open of a depth probe used alone + * (`ISecurityService.resolveWriteScope` answering `'org'` for an object no + * permission set mentions) never arises here. Where that judgement is not + * served, record sharing is asked with no depth, so its owner-match stays at + * its narrowest (`own`) — the safe direction. The contract that owns this + * gate's meaning records it once (`@objectstack/spec` — `ISharingService`, + * "Write DEPTH is an input the CALLER supplies"), because the `sys_attachment` + * kit reaches the same question from another package; it is not restated + * here. */ function callerContext(ctx: any): ExecutionContext { const exec = ctx?.input?.options?.context; if (exec && typeof exec === 'object') { @@ -690,9 +703,12 @@ async function callerCanRead(ctx: any, target: CommentThreadTarget): Promise CommentSecurityLike | null | undefined, ): void { /** May the caller EDIT the parent record behind `target`? The composition - * "Parent EDIT" in the module header sets out, in two steps: record sharing - * (or, with no sharing service, caller-scoped parent read visibility — the - * degraded mode) and the master check; then, only when that admits, the - * parent's own by-id update judgement. */ + * "Parent EDIT" in the module header sets out. [#24646] Where the security + * service serves `explain`, the parent's own by-id update judgement decides + * alone — after caller-scoped parent read visibility on a kernel with no + * sharing service (the degraded mode). Where it serves none, the composition + * the gate had before it could ask that judgement decides: record sharing + * (or the degraded read) and the master check. */ const canEditParent = async (ctx: any, target: CommentThreadTarget, verb: string): Promise => { - if (!(await sharingAndMasterAdmit(ctx, target, verb))) return false; - return parentUpdateAdmits(target, callerContext(ctx), verb); - }; - - /** [#24549] The parent's own by-id update judgement: the security service's - * record-grained `explain` verdict for `update` on the parent, which admits - * only when `allowed` and `record.visible` are both `true` — a decision that - * carries no record verdict did not judge the record, and refuses. See - * "Parent EDIT" in the module header. A rejection propagates unchanged. */ - const parentUpdateAdmits = async ( - target: CommentThreadTarget, - callerCtx: ExecutionContext, - verb: string, - ): Promise => { let security: CommentSecurityLike | null | undefined; try { security = getSecurity?.(); @@ -730,10 +734,29 @@ export function installCommentAccessHooks( if (!security || typeof security.explain !== 'function') { logger.debug?.( `[audit] comment access: the security service judges no by-id update of ${target.object} here — ${verb} ` + - 'decided by record sharing and the master-detail check alone', + 'decided by record sharing and the master-detail check', ); - return true; + return sharingAndMasterAdmit(ctx, target, verb); + } + const sharing = getSharing(); + if ((!sharing || typeof sharing.checkEdit !== 'function') && !(await degradedParentRead(ctx, target, verb))) { + return false; } + return parentUpdateAdmits(security as ExplainingSecurity, target, callerContext(ctx), verb); + }; + + /** [#24549, #24646] The parent's own by-id update judgement: the security + * service's record-grained `explain` verdict for `update` on the parent, + * which admits only when `allowed` and `record.visible` are both `true` — a + * decision that carries no record verdict did not judge the record, and + * refuses. See "Parent EDIT" in the module header. A rejection propagates + * unchanged. */ + const parentUpdateAdmits = async ( + security: ExplainingSecurity, + target: CommentThreadTarget, + callerCtx: ExecutionContext, + verb: string, + ): Promise => { const decision = await security.explain( { object: target.object, operation: 'update', recordId: target.recordId }, callerCtx, @@ -746,15 +769,19 @@ export function installCommentAccessHooks( return false; }; - /** The first step of {@link canEditParent}: record sharing (or the degraded - * read), then the master check. */ + /** The degraded mode (no sharing service): caller-scoped parent READ visibility. */ + const degradedParentRead = (ctx: any, target: CommentThreadTarget, verb: string): Promise => { + logger.debug?.(`[audit] comment access: sharing service absent — ${verb} gated on parent read visibility`); + return callerCanRead(ctx, target); + }; + + /** The composition {@link canEditParent} keeps where the security service + * judges no by-id update: record sharing (or the degraded read), then the + * master check. */ const sharingAndMasterAdmit = async (ctx: any, target: CommentThreadTarget, verb: string): Promise => { const sharing = getSharing(); if (!sharing || typeof sharing.checkEdit !== 'function') { - logger.debug?.( - `[audit] comment access: sharing service absent — ${verb} gated on parent read visibility`, - ); - return callerCanRead(ctx, target); + return degradedParentRead(ctx, target, verb); } const callerCtx = callerContext(ctx); const verdict = await sharing.checkEdit(target.object, target.recordId, callerCtx); diff --git a/packages/plugins/plugin-audit/src/comment-parent-update-parity.test.ts b/packages/plugins/plugin-audit/src/comment-parent-update-parity.test.ts index d00848bf8ce..d4fd27c544d 100644 --- a/packages/plugins/plugin-audit/src/comment-parent-update-parity.test.ts +++ b/packages/plugins/plugin-audit/src/comment-parent-update-parity.test.ts @@ -2,23 +2,26 @@ /** * The `sys_comment` moderation gate gives the answer a by-id update of the - * parent record gets. + * parent record gets — and, since #24646, asks nothing else. * * The limb that asks "may the caller edit the parent record?" — editing or - * removing another person's comment — admits only where the security service's - * judgement of that update admits: its record-grained `explain` verdict for - * `update` on `(parent object, parent id)`, `allowed` and `record.visible` - * both. That verdict is the write path's own composition (the object-level - * grant, the parent's own row-level security, its record sharing and, for a - * `controlled_by_parent` parent, its master), so the gate keeps no second copy - * of it. + * removing another person's comment — admits exactly where the security + * service's judgement of that update admits: its record-grained `explain` + * verdict for `update` on `(parent object, parent id)`, `allowed` and + * `record.visible` both. That verdict is the write path's own composition (the + * object-level grant, the parent's own row-level security, its record sharing + * at the caller's real write depth and, for a `controlled_by_parent` parent, + * its master), so the gate keeps no second copy of it: record sharing and the + * master-detail check are not asked where the judgement is served. A caller + * record sharing refuses at owner-only depth — a `unit` / `org` write-depth + * holder — moderates when the update admits them, and a caller the update + * refuses is refused whatever record sharing or the master check would have + * said. A store fault keeps its own status. Posting a comment stays a READ rule + * on the parent (the module header says why), so the update judgement is not + * asked there. * - * What the gate already composed keeps its answers: record sharing's `deny` - * still refuses without asking anything further, and so do the master-detail - * write check's refusals. A store fault keeps its own status. The update - * judgement can only add refusals; it never admits what the gate refused. - * Posting a comment stays a READ rule on the parent (the module header says - * why), so the update judgement is not asked there. + * Where the security service serves no `explain`, the gate keeps the + * composition it had before it could ask one (the last suite below). * * Refusals are asserted on the ADR-0112 envelope (`code` and `status`). */ @@ -136,75 +139,71 @@ const hookCtx = (event: string, input: Record, visible: string[ const othersComment = { id: 'c1', thread_id: THREAD, author_id: AUTHOR, body: 'hello' }; +/** What record sharing and the master-detail check WOULD answer — neither is asked where the judgement is served. */ interface Row { shape: string; sharing: SharingVerdict; master?: ControlledByParentWriteOutcome; - /** `null` = never asked. */ - judgement: UpdateJudgement | null; + judgement: UpdateJudgement; admits: boolean; } const ROWS: Row[] = [ - { shape: 'sharing allow, update judgement admits', sharing: 'allow', judgement: 'admits', admits: true }, - { shape: 'sharing allow, update judgement: record verdict refuses', sharing: 'allow', judgement: 'record_refuses', admits: false }, - { shape: 'sharing allow, update judgement: decision refuses', sharing: 'allow', judgement: 'decision_refuses', admits: false }, - { shape: 'sharing allow, update judgement carries no record verdict', sharing: 'allow', judgement: 'no_record_verdict', admits: false }, - { shape: 'sharing abstain, no master, update judgement admits', sharing: 'abstain', judgement: 'admits', admits: true }, - { shape: 'sharing abstain, no master, update judgement: record verdict refuses', sharing: 'abstain', judgement: 'record_refuses', admits: false }, + { shape: 'update judgement admits (record sharing allows)', sharing: 'allow', judgement: 'admits', admits: true }, + { shape: 'update judgement: record verdict refuses (record sharing allows)', sharing: 'allow', judgement: 'record_refuses', admits: false }, + { shape: 'update judgement: decision refuses (record sharing allows)', sharing: 'allow', judgement: 'decision_refuses', admits: false }, + { shape: 'update judgement carries no record verdict (record sharing allows)', sharing: 'allow', judgement: 'no_record_verdict', admits: false }, + // [#24646] The write-depth holder: record sharing at owner-only depth + // refuses, the parent's own update admits — and so does the gate. + { shape: 'update judgement admits, record sharing at own depth refuses (a unit / org write-depth holder)', sharing: 'deny', judgement: 'admits', admits: true }, + { shape: 'update judgement: record verdict refuses, record sharing refuses', sharing: 'deny', judgement: 'record_refuses', admits: false }, + { shape: 'update judgement admits (record sharing abstains)', sharing: 'abstain', judgement: 'admits', admits: true }, + // A controlled_by_parent parent follows its master THROUGH the judgement. { - shape: 'sharing abstain, no master, update judgement: decision refuses', + shape: 'update judgement admits, the master check alone would refuse', sharing: 'abstain', - judgement: 'decision_refuses', - admits: false, - }, - { - shape: 'sharing abstain, master admits, update judgement admits', - sharing: 'abstain', - master: { outcome: 'allow' }, + master: { outcome: 'deny', leg: 'object_permission' }, judgement: 'admits', admits: true, }, { - shape: 'sharing abstain, master admits, update judgement: decision refuses', + shape: 'update judgement: record verdict refuses, the master check alone would admit', sharing: 'abstain', master: { outcome: 'allow' }, - judgement: 'decision_refuses', + judgement: 'record_refuses', admits: false, }, { - shape: 'sharing abstain, master refuses', - sharing: 'abstain', - master: { outcome: 'deny', leg: 'object_permission' }, - judgement: null, - admits: false, - }, - { - shape: 'sharing abstain, master unresolvable', + shape: 'update judgement: decision refuses, the master check is unresolvable', sharing: 'abstain', master: { outcome: 'unresolvable', reason: 'master_reference_missing' }, - judgement: null, + judgement: 'decision_refuses', admits: false, }, - { shape: 'sharing deny', sharing: 'deny', judgement: null, admits: false }, ]; -const judgementOf = (row: Row): UpdateJudgement => row.judgement ?? 'admits'; +const rowDoubles = (row: Row) => { + const { security, explain, checkControlledByParentWrite } = securityDouble(row.judgement, row.master); + const sharing = sharingDouble(row.sharing); + return { security, explain, checkControlledByParentWrite, sharing, checkEdit: sharing.checkEdit as ReturnType }; +}; -describe('the comment moderation gate agrees with the parent by-id update', () => { +describe('the comment moderation gate asks the parent by-id update, and only it', () => { for (const row of ROWS) { it(`delete another person's comment — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 RECORD_NOT_ACCESSIBLE'}`, async () => { - const { security, explain } = securityDouble(judgementOf(row), row.master); - const { beforeDelete } = install({ sharing: sharingDouble(row.sharing), security, comments: [othersComment] }); + const { security, explain, checkControlledByParentWrite, sharing, checkEdit } = rowDoubles(row); + const { beforeDelete } = install({ sharing, security, comments: [othersComment] }); const run = beforeDelete(hookCtx('beforeDelete', { id: 'c1' })); if (row.admits) await expect(run).resolves.toBeUndefined(); else await expect(run).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: PARENT }); - expect(explain).toHaveBeenCalledTimes(row.judgement === null ? 0 : 1); + expect(explain).toHaveBeenCalledTimes(1); + expect(checkEdit).not.toHaveBeenCalled(); + expect(checkControlledByParentWrite).not.toHaveBeenCalled(); }); it(`edit another person's comment — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 RECORD_NOT_ACCESSIBLE'}`, async () => { - const { security } = securityDouble(judgementOf(row), row.master); - const { beforeUpdate } = install({ sharing: sharingDouble(row.sharing), security, comments: [othersComment] }); + const { security, sharing } = rowDoubles(row); + const { beforeUpdate } = install({ sharing, security, comments: [othersComment] }); const run = beforeUpdate(hookCtx('beforeUpdate', { id: 'c1', data: { body: 'edited' } })); if (row.admits) await expect(run).resolves.toBeUndefined(); else await expect(run).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: PARENT }); @@ -284,9 +283,50 @@ describe('the comment moderation gate asks the update judgement of the parent re await expect(beforeInsert(hookCtx('beforeInsert', { data: { thread_id: THREAD, body: 'hi' } }))).resolves.toBeUndefined(); expect(explain).not.toHaveBeenCalled(); }); +}); - it('a kernel with no security service composes no update judgement, as the parent update meets none', async () => { - const { beforeDelete } = install({ sharing: sharingDouble('abstain'), security: null, comments: [othersComment] }); - await expect(beforeDelete(hookCtx('beforeDelete', { id: 'c1' }))).resolves.toBeUndefined(); +/** + * [#24646] A kernel whose security service serves no `explain` — or no + * security service at all — judges no by-id update of the parent, and the gate + * keeps the composition it had before it could ask one: record sharing's + * tri-state `checkEdit` at owner-only depth, then, on an abstention, the + * master-detail check. Removing those steps there would admit every caller, so + * this suite holds them: each row is the answer the gate gave before #24646. + */ +describe('a kernel that judges no by-id update keeps the record-sharing and master-detail steps', () => { + const masterOnly = (outcome: ControlledByParentWriteOutcome): CommentSecurityLike => ({ + checkControlledByParentWrite: vi.fn(async (): Promise => outcome), + }); + const DEGRADED: Array<{ shape: string; sharing: SharingVerdict; master: ControlledByParentWriteOutcome; admits: boolean }> = [ + { shape: 'record sharing allows', sharing: 'allow', master: { outcome: 'deny', leg: 'record_sharing' }, admits: true }, + { shape: 'record sharing refuses at owner-only depth', sharing: 'deny', master: { outcome: 'allow' }, admits: false }, + { shape: 'record sharing abstains, the master check admits', sharing: 'abstain', master: { outcome: 'allow' }, admits: true }, + { shape: 'record sharing abstains, the master check refuses', sharing: 'abstain', master: { outcome: 'deny', leg: 'record_sharing' }, admits: false }, + ]; + for (const row of DEGRADED) { + it(`delete another person's comment on a security service serving no explain — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 RECORD_NOT_ACCESSIBLE'}`, async () => { + const { beforeDelete } = install({ sharing: sharingDouble(row.sharing), security: masterOnly(row.master), comments: [othersComment] }); + const run = beforeDelete(hookCtx('beforeDelete', { id: 'c1' })); + if (row.admits) await expect(run).resolves.toBeUndefined(); + else await expect(run).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: PARENT }); + }); + + it(`edit another person's comment with no security service — ${row.shape} → ${row.sharing === 'deny' ? 'refused 403 RECORD_NOT_ACCESSIBLE' : 'admitted'}`, async () => { + // No security service: an abstention admits, as the parent's own update meets no master check either. + const { beforeUpdate } = install({ sharing: sharingDouble(row.sharing), security: null, comments: [othersComment] }); + const run = beforeUpdate(hookCtx('beforeUpdate', { id: 'c1', data: { body: 'edited' } })); + if (row.sharing !== 'deny') await expect(run).resolves.toBeUndefined(); + else await expect(run).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: PARENT }); + }); + } + + it('with no sharing service either, the parent read alone decides — as before', async () => { + const shown = install({ sharing: null, security: null, comments: [othersComment] }); + await expect(shown.beforeDelete(hookCtx('beforeDelete', { id: 'c1' }))).resolves.toBeUndefined(); + const hidden = install({ sharing: null, security: null, comments: [othersComment] }); + await expect(hidden.beforeDelete(hookCtx('beforeDelete', { id: 'c1' }, []))).rejects.toMatchObject({ + code: 'RECORD_NOT_ACCESSIBLE', + status: 403, + }); }); }); diff --git a/packages/services/service-storage/src/attachment-access-hooks.test.ts b/packages/services/service-storage/src/attachment-access-hooks.test.ts index 1bc816580dd..ab29075cb5b 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -1533,29 +1533,32 @@ describe('[ADR-0055] parent EDIT on a controlled_by_parent parent — checkEdit, }); }); -// ── The parent gate agrees with the parent by-id update (#24549) ────────── +// ── The parent gate asks the parent by-id update, and only it (#24549, #24646) ── /** * The `sys_attachment` parent gate gives the answer a by-id update of the - * parent record gets. + * parent record gets — and, since #24646, asks nothing else. * * Every limb that asks "may the caller edit the parent record?" — the attach * rule, the row rule's parent-editor limb on update and delete, the attach rule * on a re-point, and the caller-scoped `mayEditAttachmentParent` the - * file-attachments service asks before it spends work on a file — admits only - * where the security service's judgement of that update admits: its + * file-attachments service asks before it spends work on a file — admits + * exactly where the security service's judgement of that update admits: its * record-grained `explain` verdict for `update` on `(parent object, parent id)`, * `allowed` and `record.visible` both. That verdict is the write path's own * composition (the object-level grant, the parent's own row-level security, its - * record sharing and, for a `controlled_by_parent` parent, its master), so the - * gate keeps no second copy of it. + * record sharing at the caller's real write depth and, for a + * `controlled_by_parent` parent, its master), so the gate keeps no second copy + * of it: record sharing and the master-detail check are not asked where the + * judgement is served. A caller record sharing refuses at owner-only depth — + * a `unit` / `org` write-depth holder — is admitted when the update admits + * them, and a caller the update refuses is refused whatever record sharing or + * the master check would have said. A store fault keeps its own status. * - * What the gate already composed keeps its answers: record sharing's `deny` - * still refuses without asking anything further, and so do the master-detail - * write check's refusals. A store fault keeps its own status. The update - * judgement can only add refusals; it never admits what the gate refused. + * Where the security service serves no `explain`, the gate keeps the + * composition it had before it could ask one (the last suite below). * - * Each row below names the shape of the answers the gate is handed and the + * Each row below names the shape of the answers the gate's doubles hold and the * answer it gives. Refusals are asserted on the ADR-0112 envelope (`code` and * `status`). */ @@ -1640,105 +1643,102 @@ const attachCtx = () => const othersRow = { id: 'a1', file_id: 'f1', parent_object: PARENT, parent_id: PARENT_ID, uploaded_by: UPLOADER }; /** - * One row per shape: what record sharing answers, what the master-detail check - * answers (only asked on an abstention), what the parent's update judgement - * answers (`null` = never asked), and the gate's answer. + * One row per shape: what record sharing and the master-detail check WOULD + * answer — neither is asked where the judgement is served — what the parent's + * update judgement answers, and the gate's answer. */ interface Row { shape: string; sharing: SharingVerdict; master?: ControlledByParentWriteOutcome; - judgement: UpdateJudgement | null; + judgement: UpdateJudgement; admits: boolean; } const ROWS: Row[] = [ - { shape: 'sharing allow, update judgement admits', sharing: 'allow', judgement: 'admits', admits: true }, - { shape: 'sharing allow, update judgement: record verdict refuses', sharing: 'allow', judgement: 'record_refuses', admits: false }, - { shape: 'sharing allow, update judgement: decision refuses', sharing: 'allow', judgement: 'decision_refuses', admits: false }, - { shape: 'sharing allow, update judgement carries no record verdict', sharing: 'allow', judgement: 'no_record_verdict', admits: false }, - { shape: 'sharing abstain, no master, update judgement admits', sharing: 'abstain', judgement: 'admits', admits: true }, - { shape: 'sharing abstain, no master, update judgement: record verdict refuses', sharing: 'abstain', judgement: 'record_refuses', admits: false }, + { shape: 'update judgement admits (record sharing allows)', sharing: 'allow', judgement: 'admits', admits: true }, + { shape: 'update judgement: record verdict refuses (record sharing allows)', sharing: 'allow', judgement: 'record_refuses', admits: false }, + { shape: 'update judgement: decision refuses (record sharing allows)', sharing: 'allow', judgement: 'decision_refuses', admits: false }, + { shape: 'update judgement carries no record verdict (record sharing allows)', sharing: 'allow', judgement: 'no_record_verdict', admits: false }, + // [#24646] The write-depth holder: record sharing at owner-only depth + // refuses, the parent's own update admits — and so does the gate. + { shape: 'update judgement admits, record sharing at own depth refuses (a unit / org write-depth holder)', sharing: 'deny', judgement: 'admits', admits: true }, + { shape: 'update judgement: record verdict refuses, record sharing refuses', sharing: 'deny', judgement: 'record_refuses', admits: false }, + { shape: 'update judgement admits (record sharing abstains)', sharing: 'abstain', judgement: 'admits', admits: true }, + // A controlled_by_parent parent follows its master THROUGH the judgement: + // the gate's own master step is gone in both directions. { - shape: 'sharing abstain, no master, update judgement: decision refuses', + shape: 'update judgement admits, the master check alone would refuse', sharing: 'abstain', - judgement: 'decision_refuses', - admits: false, - }, - { - shape: 'sharing abstain, master admits, update judgement admits', - sharing: 'abstain', - master: { outcome: 'allow' }, + master: { outcome: 'deny', leg: 'record_sharing' }, judgement: 'admits', admits: true, }, { - shape: 'sharing abstain, master admits, update judgement: decision refuses', + shape: 'update judgement: record verdict refuses, the master check alone would admit', sharing: 'abstain', master: { outcome: 'allow' }, - judgement: 'decision_refuses', - admits: false, - }, - { - shape: 'sharing abstain, master refuses', - sharing: 'abstain', - master: { outcome: 'deny', leg: 'record_sharing' }, - judgement: null, + judgement: 'record_refuses', admits: false, }, { - shape: 'sharing abstain, master unresolvable', + shape: 'update judgement: decision refuses, the master check is unresolvable', sharing: 'abstain', master: { outcome: 'unresolvable', reason: 'record_not_found' }, - judgement: null, + judgement: 'decision_refuses', admits: false, }, - { shape: 'sharing deny', sharing: 'deny', judgement: null, admits: false }, ]; -/** The judgement a row hands the double; a row whose judgement is never asked still gets one that admits. */ -const judgementOf = (row: Row): UpdateJudgement => row.judgement ?? 'admits'; +/** Doubles for one row: the judgement, and the two services the gate no longer asks beside it. */ +const rowDoubles = (row: Row) => { + const { security, explain, checkControlledByParentWrite } = securityDouble(row.judgement, row.master); + const sharing = sharingDouble(row.sharing); + return { security, explain, checkControlledByParentWrite, sharing, checkEdit: sharing.checkEdit as ReturnType }; +}; -describe('the attachment parent gate agrees with the parent by-id update', () => { +describe('the attachment parent gate asks the parent by-id update, and only it', () => { for (const row of ROWS) { it(`attach — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 ATTACHMENT_PARENT_ACCESS'}`, async () => { - const { security, explain } = securityDouble(judgementOf(row), row.master); - const { beforeInsert } = installForParity({ sharing: sharingDouble(row.sharing), security }); + const { security, explain, checkControlledByParentWrite, sharing, checkEdit } = rowDoubles(row); + const { beforeInsert } = installForParity({ sharing, security }); const run = beforeInsert(attachCtx()); if (row.admits) await expect(run).resolves.toBeUndefined(); else await expect(run).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403, object: PARENT }); - expect(explain).toHaveBeenCalledTimes(row.judgement === null ? 0 : 1); + expect(explain).toHaveBeenCalledTimes(1); + expect(checkEdit).not.toHaveBeenCalled(); + expect(checkControlledByParentWrite).not.toHaveBeenCalled(); }); it(`delete another user's file — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 ATTACHMENT_DELETE_DENIED'}`, async () => { - const { security } = securityDouble(judgementOf(row), row.master); - const { beforeDelete } = installForParity({ sharing: sharingDouble(row.sharing), security, attachments: [othersRow] }); + const { security, sharing } = rowDoubles(row); + const { beforeDelete } = installForParity({ sharing, security, attachments: [othersRow] }); const run = beforeDelete(hookCtx('beforeDelete', { id: 'a1' })); if (row.admits) await expect(run).resolves.toBeUndefined(); else await expect(run).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403, object: PARENT }); }); it(`update another user's file — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 RECORD_NOT_ACCESSIBLE'}`, async () => { - const { security } = securityDouble(judgementOf(row), row.master); - const { beforeUpdate } = installForParity({ sharing: sharingDouble(row.sharing), security, attachments: [othersRow] }); + const { security, sharing } = rowDoubles(row); + const { beforeUpdate } = installForParity({ sharing, security, attachments: [othersRow] }); const run = beforeUpdate(hookCtx('beforeUpdate', { id: 'a1', data: { file_name: 'renamed.pdf' } })); if (row.admits) await expect(run).resolves.toBeUndefined(); else await expect(run).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: PARENT }); }); it(`re-point the caller's own file — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 ATTACHMENT_PARENT_ACCESS'}`, async () => { - const { security } = securityDouble(judgementOf(row), row.master); + const { security, sharing } = rowDoubles(row); const own = { ...othersRow, uploaded_by: CALLER }; - const { beforeUpdate } = installForParity({ sharing: sharingDouble(row.sharing), security, attachments: [own] }); + const { beforeUpdate } = installForParity({ sharing, security, attachments: [own] }); const run = beforeUpdate(hookCtx('beforeUpdate', { id: 'a1', data: { parent_id: NEXT_ID } })); if (row.admits) await expect(run).resolves.toBeUndefined(); else await expect(run).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403, object: PARENT }); }); it(`mayEditAttachmentParent — ${row.shape} → ${row.admits}`, async () => { - const { security } = securityDouble(judgementOf(row), row.master); + const { security, sharing } = rowDoubles(row); const verdict = await mayEditAttachmentParent( - { getSharing: () => sharingDouble(row.sharing), getSecurity: () => security, logger: { debug: vi.fn() } }, + { getSharing: () => sharing, getSecurity: () => security, logger: { debug: vi.fn() } }, PARENT, PARENT_ID, callerEnvelope, @@ -1816,9 +1816,64 @@ describe('the attachment parent gate asks the update judgement of the parent rec }); expect(unread.explain).not.toHaveBeenCalled(); }); +}); - it('a kernel with no security service composes no update judgement, as the parent update meets none', async () => { - const { beforeInsert } = installForParity({ sharing: sharingDouble('abstain'), security: null }); - await expect(beforeInsert(attachCtx())).resolves.toBeUndefined(); +/** + * [#24646] A kernel whose security service serves no `explain` — or no + * security service at all — judges no by-id update of the parent, and the gate + * keeps the composition it had before it could ask one: record sharing's + * tri-state `checkEdit` at owner-only depth, then, on an abstention, the + * master-detail check. Removing those steps there would admit every caller, so + * this suite holds them: each row is the answer the gate gave before #24646. + */ +describe('a kernel that judges no by-id update keeps the record-sharing and master-detail steps', () => { + const masterOnly = (outcome: ControlledByParentWriteOutcome) => { + const checkControlledByParentWrite = vi.fn(async (): Promise => outcome); + return { security: { checkControlledByParentWrite } satisfies AttachmentSecurityLike, checkControlledByParentWrite }; + }; + const DEGRADED: Array<{ shape: string; sharing: SharingVerdict; master: ControlledByParentWriteOutcome; admits: boolean }> = [ + { shape: 'record sharing allows', sharing: 'allow', master: { outcome: 'deny', leg: 'record_sharing' }, admits: true }, + { shape: 'record sharing refuses at owner-only depth', sharing: 'deny', master: { outcome: 'allow' }, admits: false }, + { shape: 'record sharing abstains, the master check admits', sharing: 'abstain', master: { outcome: 'allow' }, admits: true }, + { shape: 'record sharing abstains, the master check refuses', sharing: 'abstain', master: { outcome: 'deny', leg: 'record_sharing' }, admits: false }, + ]; + for (const row of DEGRADED) { + it(`attach on a security service serving no explain — ${row.shape} → ${row.admits ? 'admitted' : 'refused 403 ATTACHMENT_PARENT_ACCESS'}`, async () => { + const { security } = masterOnly(row.master); + const { beforeInsert } = installForParity({ sharing: sharingDouble(row.sharing), security }); + const run = beforeInsert(attachCtx()); + if (row.admits) await expect(run).resolves.toBeUndefined(); + else await expect(run).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403, object: PARENT }); + }); + + it(`delete another user's file with no security service — ${row.shape} → ${row.sharing === 'deny' ? 'refused 403 ATTACHMENT_DELETE_DENIED' : 'admitted'}`, async () => { + // No security service: an abstention admits, as the parent's own update meets no master check either. + const { beforeDelete } = installForParity({ sharing: sharingDouble(row.sharing), security: null, attachments: [othersRow] }); + const run = beforeDelete(hookCtx('beforeDelete', { id: 'a1' })); + if (row.sharing !== 'deny') await expect(run).resolves.toBeUndefined(); + else await expect(run).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403, object: PARENT }); + }); + } + + it('mayEditAttachmentParent on a security service serving no explain refuses what record sharing refuses', async () => { + const { security } = masterOnly({ outcome: 'allow' }); + const verdict = await mayEditAttachmentParent( + { getSharing: () => sharingDouble('deny'), getSecurity: () => security, logger: { debug: vi.fn() } }, + PARENT, + PARENT_ID, + callerEnvelope, + 'attach', + async () => ({ id: PARENT_ID }), + ); + expect(verdict).toBe(false); + }); + + it('with no sharing service either, the parent read alone decides — as before', async () => { + await expect(installForParity({ sharing: null, security: null }).beforeInsert(attachCtx())).resolves.toBeUndefined(); + const ctx = { ...attachCtx(), api: apiReading([]) }; + await expect(installForParity({ sharing: null, security: null }).beforeInsert(ctx)).rejects.toMatchObject({ + code: 'ATTACHMENT_PARENT_ACCESS', + status: 403, + }); }); }); diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 609142e1486..f78d8f6896c 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -70,45 +70,42 @@ import { * * The attach rule, the row rule's parent-editor limb (update and delete) and * the attach rule on a re-point all ask ONE question — may the caller edit the - * parent record? — answered by ONE composition ({@link installAttachmentAccessHooks}'s - * `mayEditParent`), so the four cannot drift: + * parent record? — answered by ONE composition ({@link mayEditAttachmentParent}), + * so the four cannot drift. Its answer is the one a by-id UPDATE of the parent + * record, by the same caller, gets: * - * - plugin-sharing's `checkEdit` (`ISharingService`) answers first: `allow` - * admits and `deny` refuses (with each limb's own envelope). - * - `abstain` means record sharing does not enforce on the parent at all. For - * most parents that IS permission (a `public_read_write` object: nobody - * owns its rows). For a `controlled_by_parent` parent it is not: its access - * derives from its MASTER (ADR-0055), and `effectiveSharingModel` maps it to - * `public` precisely because the security plugin's master-detail check - * judges it instead. So an abstention asks that check — - * `ISecurityService.checkControlledByParentWrite`, the answer a by-id update - * of the parent gets — and only its `allow` and `not_applicable` admit. - * `deny` and `unresolvable` (a broken declaration, a missing record, a null - * master reference) refuse. A rejection — a datasource fault, a context the - * write path refuses — propagates unchanged, so an outage keeps its `503`. - * - A kernel whose security service does not serve that member composes no - * master check, and there the parent's own update meets none either: the - * abstention admits, as it always did. - * - No sharing service at all: caller-scoped parent READ visibility (degraded - * mode) — still strictly tighter than no gate. - * - [#24549] Whatever the steps above admit, the parent's own by-id UPDATE - * must admit too, and it decides last: the security service's judgement of - * that update, `ISecurityService.explain`'s record-grained verdict for - * `update` on the parent — `allowed` and `record.visible` both. That verdict - * is the write path's own composition (the parent object's grant, its - * row-level security, its record sharing and its master), so each limb - * gives the answer a by-id update of the parent gets, and the gate keeps no - * copy of that judgement. Asked only for a caller the steps above admit, it - * adds refusals and never admits one: record sharing's `deny`, the master - * check's refusals and a rejection keep their answers, and so does the - * owner-only edit DEPTH the sharing step is asked at (the #7144 ruling). A - * rejection of it propagates unchanged. A kernel with no security service - * meets no such judgement on the parent's update either, and a security - * service that serves no `explain` is not asked it — the same + * - [#24646] The security service's judgement of that update decides it + * alone: `ISecurityService.explain`'s record-grained verdict for `update` on + * the parent, which admits only when `allowed` and `record.visible` are both + * `true` — a decision that carries no record verdict did not judge the + * record, and refuses. That verdict is the write path's own composition: the + * parent object's grant, its row-level security, its record sharing at the + * caller's real write DEPTH, the deferral to an app-authored row-level write + * policy, the master of a `controlled_by_parent` parent (ADR-0055), and both + * legs of an on-behalf-of write (ADR-0090 D10). So a caller who can update + * the parent record by id can also attach files to it; a caller who cannot, + * still cannot. A rejection — a datasource fault, a context the write path + * refuses — propagates unchanged, so an outage keeps its `503`. + * - The gate keeps no copy of that judgement. Until #24646 it also asked + * record sharing at owner-only depth and then the master-detail check, ahead + * of the judgement: a narrower duplicate that refused a caller with `unit`, + * `unit_and_below` or `org` write depth on the parent, whom the CRUD path + * admits. The ruling that kept it (#7144) is superseded for these gates. + * - No sharing service: caller-scoped parent READ visibility is asked first + * (the degraded mode), then the judgement above. + * - A security service that serves no `explain`, or none at all, judges no + * by-id update here. There the gate keeps the composition it had before it + * could ask one ({@link sharingAndMasterAdmit}): plugin-sharing's TRI-STATE + * `checkEdit`, where `allow` admits and `deny` refuses, and an `abstain` + * asks the master-detail check, `ISecurityService.checkControlledByParentWrite`, + * whose `allow` and `not_applicable` alone admit (absent, the abstention + * admits, as the parent's own update meets no master check there either); + * with no sharing service, the degraded read alone. ⛔ Dropping those steps + * on such a kernel would admit every caller, so they stay there. The same * feature-detection `FileAttachmentService.assertMayAttach` applies to its * own `explain` question. * - * ⛔ This is why the gate reads `checkEdit`, never `canEdit`: `canEdit` is its + * ⛔ On that path the gate reads `checkEdit`, never `canEdit`: `canEdit` is its * two-state projection, folding `abstain` into `true`, and an attach gate that * took that fold as permission let any member holding the `sys_attachment` * create or delete bit write files onto every master-detail child record of @@ -126,22 +123,28 @@ import { /** * The one member of plugin-sharing's contract this gate consults — the * TRI-STATE edit verdict, because the gate must tell "sharing permits this" - * from "sharing does not enforce here" (see "Parent EDIT" above). A `Pick` of - * the real `ISharingService`, not a hand-written shape. + * from "sharing does not enforce here" (see "Parent EDIT" above). Asked only + * where the security service judges no by-id update; its presence alone also + * decides whether the degraded read is asked. A `Pick` of the real + * `ISharingService`, not a hand-written shape. */ export type AttachmentSharingLike = Pick; /** - * The two members of the security service's contract this gate consults: the - * ADR-0055 master-detail write check, for a `controlled_by_parent` parent - * (OPTIONAL on the contract, so the gate handles its absence by construction), - * and `explain`, whose record-grained `update` verdict is the parent's own - * by-id update judgement (see "Parent EDIT" above). `explain` is feature-detected - * like every member here, so it is optional on this view of the contract. + * The two members of the security service's contract this gate consults: + * `explain`, whose record-grained `update` verdict is the parent's own by-id + * update judgement and decides the question wherever it is served (see "Parent + * EDIT" above), and the ADR-0055 master-detail write check, asked only where + * no `explain` is served (OPTIONAL on the contract, so the gate handles its + * absence by construction). `explain` is feature-detected like every member + * here, so it is optional on this view of the contract. */ export type AttachmentSecurityLike = Pick & Partial>; +/** A security service known to serve `explain` — the one the parent's by-id update judgement is asked of. */ +type ExplainingSecurity = AttachmentSecurityLike & Pick; + const PACKAGE_ID = 'com.objectstack.service.storage'; const SYSTEM_CTX = { isSystem: true } as const; /** Bound on join rows authorized per multi-update/-delete; mirrors the @@ -309,10 +312,12 @@ function asIdList(id: unknown): Array | null { * now for the same reason — the envelope is the contract's unit. * * Note what deliberately did NOT change: no access DEPTH is synthesised for the - * parent object. Absent depth leaves the sharing owner-match at its narrowest - * (`own`) — the safe direction, and byte-for-byte the behaviour the projection - * produced. Resolving the parent's own depth would WIDEN this gate and is a - * separate decision, tracked as #7144. */ + * parent object, and none is carried across from `sys_attachment`. [#24646] The + * parent's own depth is the security service's to resolve, and it does: the + * by-id update judgement this gate asks resolves it together with the parent + * object's grant. Where that judgement is not served, record sharing is asked + * with no depth, so its owner-match stays at its narrowest (`own`) — the safe + * direction. */ function callerContext(ctx: any): ExecutionContext { const exec = ctx?.input?.options?.context; if (exec && typeof exec === 'object') { @@ -352,9 +357,11 @@ export interface AttachmentParentVerdictDeps { * are handed its string form, as `canEdit` always was. `readParent` is the * degraded mode's caller-scoped read of the parent. * - * Two steps, in order: record sharing (or the degraded read) and the master - * check, then the parent's own by-id update judgement - * ({@link parentUpdateAdmits}), asked only when the first step admits. + * [#24646] Where the security service serves `explain`, the parent's own by-id + * update judgement ({@link parentUpdateAdmits}) decides alone — after the + * degraded read, on a kernel with no sharing service. Where it serves none, + * the composition the gate had before it could ask that judgement decides + * ({@link sharingAndMasterAdmit}). */ export async function mayEditAttachmentParent( deps: AttachmentParentVerdictDeps, @@ -363,24 +370,6 @@ export async function mayEditAttachmentParent( callerCtx: ExecutionContext, limb: string, readParent: () => Promise, -): Promise { - if (!(await sharingAndMasterAdmit(deps, object, recordId, callerCtx, limb, readParent))) return false; - return parentUpdateAdmits(deps, object, recordId, callerCtx, limb); -} - -/** - * [#24549] The parent's own by-id update judgement: the security service's - * record-grained `explain` verdict for `update` on `(object, recordId)`, which - * admits only when `allowed` and `record.visible` are both `true` — a decision - * that carries no record verdict did not judge the record, and refuses. See - * "Parent EDIT" in the module header. A rejection propagates unchanged. - */ -async function parentUpdateAdmits( - deps: AttachmentParentVerdictDeps, - object: string, - recordId: unknown, - callerCtx: ExecutionContext, - limb: string, ): Promise { let security: AttachmentSecurityLike | null | undefined; try { @@ -391,10 +380,33 @@ async function parentUpdateAdmits( if (!security || typeof security.explain !== 'function') { deps.logger.debug?.( `[storage] attachment access: the security service judges no by-id update of ${object} here — ${limb} decided ` + - 'by record sharing and the master-detail check alone', + 'by record sharing and the master-detail check', ); - return true; + return sharingAndMasterAdmit(deps, object, recordId, callerCtx, limb, readParent); + } + const sharing = deps.getSharing(); + if ((!sharing || typeof sharing.checkEdit !== 'function') && !(await degradedParentRead(deps, limb, readParent))) { + return false; } + return parentUpdateAdmits(security as ExplainingSecurity, deps, object, recordId, callerCtx, limb); +} + +/** + * [#24549, #24646] The parent's own by-id update judgement: the security + * service's record-grained `explain` verdict for `update` on `(object, + * recordId)`, which admits only when `allowed` and `record.visible` are both + * `true` — a decision that carries no record verdict did not judge the record, + * and refuses. See "Parent EDIT" in the module header. A rejection propagates + * unchanged. + */ +async function parentUpdateAdmits( + security: ExplainingSecurity, + deps: AttachmentParentVerdictDeps, + object: string, + recordId: unknown, + callerCtx: ExecutionContext, + limb: string, +): Promise { const decision = await security.explain({ object, operation: 'update', recordId: String(recordId) }, callerCtx); if (decision?.allowed === true && decision.record?.visible === true) return true; deps.logger.debug?.( @@ -403,7 +415,25 @@ async function parentUpdateAdmits( return false; } -/** The first step of {@link mayEditAttachmentParent}: record sharing (or the degraded read), then the master check. */ +/** The degraded mode (no sharing service): caller-scoped parent READ visibility, a throw reading as "no". */ +async function degradedParentRead( + deps: AttachmentParentVerdictDeps, + limb: string, + readParent: () => Promise, +): Promise { + deps.logger.debug?.(`[storage] attachment access: sharing service absent — ${limb} gated on parent read visibility`); + try { + return !!(await readParent()); + } catch { + return false; + } +} + +/** + * The composition {@link mayEditAttachmentParent} keeps where the security + * service judges no by-id update: record sharing (or the degraded read), then + * the master check. + */ async function sharingAndMasterAdmit( deps: AttachmentParentVerdictDeps, object: string, @@ -417,12 +447,7 @@ async function sharingAndMasterAdmit( if (!sharing || typeof sharing.checkEdit !== 'function') { // Degraded mode (no sharing service): caller-scoped parent READ // visibility — still strictly tighter than no gate. - logger.debug?.(`[storage] attachment access: sharing service absent — ${limb} gated on parent read visibility`); - try { - return !!(await readParent()); - } catch { - return false; - } + return degradedParentRead(deps, limb, readParent); } const verdict = await sharing.checkEdit(object, String(recordId), callerCtx); if (verdict === 'allow') return true; @@ -464,9 +489,12 @@ async function sharingAndMasterAdmit( * caller's locale. * * `getSecurity` resolves the security service, lazily for the same reason, for - * its ADR-0055 master-detail write check (see "Parent EDIT" in the module - * header). Absent — or serving no such member — a `controlled_by_parent` - * parent meets no master check here, exactly as its own update meets none. + * its judgement of a by-id update of the parent (`explain`), which decides the + * parent-edit question wherever it is served (see "Parent EDIT" in the module + * header). Serving none, the service's ADR-0055 master-detail write check is + * asked on a record-sharing abstention instead; absent too, a + * `controlled_by_parent` parent meets no master check here, exactly as its own + * update meets none. */ export function installAttachmentAccessHooks( engine: AttachmentLifecycleEngine, @@ -649,9 +677,10 @@ export function installAttachmentAccessHooks( // its master, as its own update is. const allowed = await mayEditParent(ctx, parentObject, parentId, callerContext(ctx), 'attach'); if (!allowed) { - // [#22466] Every refusal leg of the attach rule arrives HERE — sharing - // `deny` or a non-verdict, the master-detail check's `deny` or - // `unresolvable`, the degraded read probe's miss — so this one call + // [#22466] Every refusal leg of the attach rule arrives HERE — the + // by-id update judgement's refusal, the degraded read probe's miss and, + // where no judgement is served, sharing `deny` or a non-verdict and the + // master-detail check's `deny` or `unresolvable` — so this one call // covers them all and no leg can drift. A REJECTION from either check // (an outage) never reaches this line: it is no verdict, and keeps // its own status. The tombstone is scheduled, never awaited, and is diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index 26a2e9669ed..6f0eee2ee11 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -217,34 +217,41 @@ export type SharingWriteVerdict = 'allow' | 'abstain' | 'deny'; * not that middleware hands over no depth, and the owner-match then runs at its * NARROWEST — owner-only, i.e. `own`. * - * **For the PARENT-record gates that is deliberate, not an oversight.** The - * `sys_comment` gates (`@objectstack/plugin-audit`) and the `sys_attachment` - * kit (`@objectstack/service-storage`) ask this service about the PARENT - * record's object while the operation in flight is on the CHILD, so the stamped - * depth was resolved for a different object and is dropped rather than carried - * across. Stated as behaviour: a caller whose write depth on the parent object - * is `unit` / `unit_and_below` / `org` can edit that parent directly through - * the CRUD path, and is REFUSED when editing or deleting a comment or - * attachment on it. The divergence runs in the RESTRICTIVE direction — - * refusals, never a leak — and the maintainer ruling of 2026-08-10 (#7144, - * split from #7141 / PR #7143) is that these gates STAY at `own`. + * **The PARENT-record gates no longer ask this service (#24646).** The + * `sys_comment` moderation gate (`@objectstack/plugin-audit`) and the + * `sys_attachment` parent gate (`@objectstack/service-storage`) ask about the + * PARENT record's object while the operation in flight is on the CHILD, so no + * depth stamped for the child could serve them. They used to ask + * {@link checkEdit} with no depth, at `own`, and then the ADR-0055 master + * check: a caller whose write depth on the parent object is `unit` / + * `unit_and_below` / `org` could edit that parent directly through the CRUD + * path, and was REFUSED when editing or deleting a comment or attachment on + * it. The maintainer ruling of 2026-08-10 (#7144, split from #7141 / PR #7143) + * kept them there; for the parent gates it is superseded by the ruling on + * #24646 (letter B). "May edit the parent" is the security service's + * record-grained by-id `update` judgement of the parent alone + * (`ISecurityService.explain`, admitting only `allowed === true` and + * `record.visible === true`), which resolves the caller's depth on the parent + * object as the CRUD path does. Stated as behaviour: a caller who can update + * the parent record by id can also attach files to it and moderate its + * comments; a caller who cannot, still cannot. Only a gate whose kernel serves + * no `explain` still asks this service, through {@link checkEdit} at `own` — + * the narrow direction. * - * **Why the obvious "fix" is off the table** — recorded here so the next reader - * can tell "deliberately tighter" from "nobody got round to it". Making these - * gates inherit the parent's real edit authority means resolving the parent's - * depth through the only tool a package outside `plugin-security` has, - * `ISecurityService.resolveWriteScope` — and that fails OPEN on one input: - * `getEffectiveScope` returns `'org'` when NO permission set mentions the - * object at all (`plugin-security/src/permission-evaluator.ts` — + * **Why the gates never resolve the parent's depth themselves** — recorded + * here so the next reader does not reach for it. The only depth tool a package + * outside `plugin-security` has, `ISecurityService.resolveWriteScope`, fails + * OPEN on one input: `getEffectiveScope` returns `'org'` when NO permission set + * mentions the object at all (`plugin-security/src/permission-evaluator.ts` — * `if (!matched) return 'org'`), which is byte-identical to what a genuine * `modifyAllRecords` holder gets. That is why the method's own doc block calls * `'org'` authoritative ONLY when paired with an explicit * `ISecurityService.hasWriteBypass` check. Handed to a write gate as the depth * it becomes fully authoritative on its own, and the owner-match short-exits * `true` for EVERY owned row of an unmatched object — a real widening, not a - * theoretical one. So inheriting the parent's depth starts with a depth - * primitive that can tell "org depth" from "nothing matched", never with these - * gates. + * theoretical one. The by-id update judgement does not meet it: it asks the + * object-level grant first, so an object no permission set mentions is refused + * before any depth is consulted. */ export interface ISharingService { /** @@ -298,11 +305,11 @@ export interface ISharingService { * * [#7144] The write DEPTH in that first sentence is an INPUT this service is * handed, never one it resolves — so a caller outside the CRUD middleware - * (the `sys_comment` and `sys_attachment` PARENT-record gates, which both - * reach this service through {@link checkEdit}) matches ownership at `own`. - * That is the ruled shape, and the fail-open that blocks widening it is - * written up once in "Write DEPTH is an input the CALLER supplies" on this - * interface. + * matches ownership at `own`. The `sys_comment` and `sys_attachment` + * PARENT-record gates no longer ask this service where the security service + * judges a by-id update (#24646); "Write DEPTH is an input the CALLER + * supplies" on this interface records what they ask instead, and the + * fail-open that keeps them from resolving a depth themselves. * * [#6428] The two-state PROJECTION of {@link checkEdit}: `true` for every * verdict that is not `deny`, i.e. `allow` and `abstain` alike. That From b29ab564a28d86e175192b01a7712f812133ebe7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 03:13:17 +0000 Subject: [PATCH 3/5] test(plugin-security): on-behalf-of explain-vs-door rows, the flipped parent-gate pin, and the real comment gate on both postures Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude --- .../src/explain-enforce-parity.test.ts | 386 +++++++++++++++++- 1 file changed, 371 insertions(+), 15 deletions(-) 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 b57ef532847..dac7bafa253 100644 --- a/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts +++ b/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts @@ -81,6 +81,7 @@ import { SharingService, buildSharingMiddleware, SysRecordShare } from '@objects import { PermissionSetSchema } from '@objectstack/spec/security'; import type { ExplainDecision, TenancyPosture } from '@objectstack/spec/security'; import { SysOrganization, SysUser, SysMember } from '@objectstack/platform-objects/identity'; +import { installCommentAccessHooks } from '@objectstack/plugin-audit'; import { SysPosition } from './__tests__/legacy-catalog-objects.testkit.js'; import { SysPermissionSet } from './__tests__/legacy-catalog-objects.testkit.js'; @@ -241,6 +242,48 @@ async function expectParity( } } +// ── the real parent gate ────────────────────────────────────────────────── + +/** + * [#24646] The REAL `sys_comment` moderation gate (`@objectstack/plugin-audit`, + * read from source by this package's vitest alias), wired to a rig's own + * security and sharing services and its engine. Only the comment table is held + * in memory: one comment by somebody else on the parent record. `moderate` + * deletes it as `caller` would and answers what the gate does — the question + * "may the caller edit the parent record?" asked by the gate itself, against + * the services enforcement runs on. + */ +function realCommentGate(engine: ObjectQL, getSharing: () => unknown, getSecurity: () => unknown) { + return async (caller: object, object: string, recordId: string): Promise => { + const comment = { id: `c_${next()}`, thread_id: `${object}:${recordId}`, author_id: 'u_comment_author', body: 'x' }; + const hooks = new Map Promise>(); + const e = engine as unknown as { + find: (o: string, opts: unknown) => Promise>>; + findOne: (o: string, opts: unknown) => Promise | null>; + }; + installCommentAccessHooks( + { + // The FIRST handler per event is the authorization gate. + registerHook: (event, handler) => { if (!hooks.has(event)) hooks.set(event, handler as never); }, + find: async (o, opts) => (o === 'sys_comment' ? [comment] : e.find(o, opts)), + findOne: async (o, opts) => + o === 'sys_comment' ? ((opts as { where?: { id?: unknown } })?.where?.id === comment.id ? comment : null) : e.findOne(o, opts), + }, + getSharing as never, + { info: vi.fn(), warn: vi.fn(), debug: vi.fn() }, + undefined, + getSecurity as never, + ); + const userId = (caller as { userId?: string }).userId; + return landed(hooks.get('beforeDelete')!({ + object: 'sys_comment', + event: 'beforeDelete', + input: { id: comment.id, options: { context: { ...caller } } }, + session: { userId }, + })); + }; +} + // ── rig 1: one row-level policy, the caller explaining themselves ──────────── let seq = 0; @@ -673,6 +716,7 @@ async function bootSharing(opts: { faultReadFilter?: boolean; authored?: boolean sharing.buildReadFilter = async () => { throw new Error('share store unavailable'); }; } engine.registerMiddleware(buildSharingMiddleware(sharing, ctx.logger) as never, { object: '*' }); + const moderate = realCommentGate(engine, () => sharing, () => security); return { explain: (caller: object, operation: 'read' | 'update' | 'delete' | 'transfer', recordId: string) => @@ -690,6 +734,208 @@ async function bootSharing(opts: { faultReadFilter?: boolean; authored?: boolean * envelope with no operation-private key. */ checkEdit: (caller: object, id: string) => sharing!.checkEdit(LINE, id, { ...caller } as never), + /** [#24646] The real `sys_comment` moderation gate, asked about a comment on `LINE/id` ({@link realCommentGate}). */ + moderate: (caller: object, id: string) => moderate(caller, LINE, id), + teardown: async () => { + try { await engine.destroy(); } catch { /* noop */ } + releaseAsyncContexts(engine, plugin); + }, + }; +} + +// ── rig 4: on-behalf-of principals, and the delegators they act for ────────── + +/** Write depth `org` on the parent, the master and the detail: the delegator ruling B is about. */ +const D_ORG = 'u_d_org'; +/** Write depth `own`: the control. */ +const D_OWN = 'u_d_own'; +/** Owns the rows nobody the rig explains owns. */ +const D_OTHER = 'u_d_other'; + +/** + * #24646 ruling item 3: for an ON-BEHALF-OF caller — an OAuth agent acting for + * a user, its context assembled by the real `assembleExecutionContext` with the + * `mcp_agent_data_write` ceiling — explain's record-grained `update` and + * `delete` verdicts are what the by-id PATCH and DELETE of the same record + * answer, under a walled (`isolated`) and a wall-less (`single`) posture. The + * same rig explains the delegators themselves as the controls. + * + * A `private`-OWD `PARENT` with an owner and a creator column (`created_by` is + * what the platform's ownership floor reads, and the delegated path keeps that + * floor on both legs), a `private` `MASTER` and a `controlled_by_parent` + * `DETAIL`. The rows separate owner from creator, so the delegated write's two + * legs — plugin-sharing's gate as the delegator at its write depth, and the + * delegator's own write filter beside the agent's — each decide a row: + * + * - `p_theirs_created_by_org`: owned by someone else, created by `D_ORG`; + * - `p_org_mine_created_by_other`: owned by `D_ORG`, created by someone else; + * - `p_theirs`, `p_org_mine`, `p_own_mine`: the controls; + * - `p_beta`: another organization's row, which only the wall-less posture lets through; + * - `p_del_*`: the same two shapes again, for the delete rows (a delete that lands removes its row). + * + * One rig per posture, shared by its rows: an admitted update writes only + * `note`, which no policy reads, and every delete row has its own record. + */ +async function bootDelegated(posture: TenancyPosture) { + const n = next(); + const PARENT = `qa_parity_parent_${n}`; + const MASTER = `qa_parity_master_${n}`; + const DETAIL = `qa_parity_detail_${n}`; + const engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }) as never, + true, + ); + await engine.init(); + const ownedFields = { + id: { name: 'id', type: 'text', primaryKey: true }, + name: { name: 'name', type: 'text' }, + note: { name: 'note', type: 'text' }, + owner_id: { name: 'owner_id', type: 'text' }, + created_by: { name: 'created_by', type: 'text' }, + }; + engine.registerApp({ + id: `com.objectstack.qa.explain-parity-delegated-${seq}`, + name: 'Explain parity: on-behalf-of principals', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [ + SysPosition, SysPermissionSet, SysPositionPermissionSet, SysUserPosition, SysUserPermissionSet, + SysOrganization, SysUser, SysMember, SysRecordShare, + { name: PARENT, label: 'Parent', sharingModel: 'private', fields: ownedFields }, + { name: MASTER, label: 'Master', sharingModel: 'private', fields: ownedFields }, + { + name: DETAIL, + label: 'Detail', + sharingModel: 'controlled_by_parent', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + note: { name: 'note', type: 'text' }, + created_by: { name: 'created_by', type: 'text' }, + master: { name: 'master', type: 'master_detail', required: true, reference: MASTER }, + }, + }, + ], + } as never); + await engine.syncSchemas(); + + const e = engine as unknown as { insert: (o: string, row: Record, opts: never) => Promise }; + for (const id of [ALPHA, BETA]) await e.insert('sys_organization', { id, name: id, slug: id }, SYS); + for (const id of [D_ORG, D_OWN, D_OTHER]) { + await e.insert('sys_user', { id, name: id, email: `${id}@example.test` }, SYS); + await e.insert('sys_member', { id: `mem_${id}_${n}`, user_id: id, organization_id: ALPHA, role: 'member' }, SYS); + } + const org = { allowRead: true, allowEdit: true, allowDelete: true, readScope: 'org', writeScope: 'org' } as const; + const own = { allowRead: true, allowEdit: true, allowDelete: true, readScope: 'own', writeScope: 'own' } as const; + const detail = { allowRead: true, allowEdit: true } as const; + const sets = [ + ...defaultPermissionSets, + PermissionSetSchema.parse({ name: 'qa_d_org', objects: { [PARENT]: org, [MASTER]: org, [DETAIL]: detail } }), + // Reads every master (so every detail), writes only its own: a detail under + // someone else's master is one it reads and may not write. + PermissionSetSchema.parse({ + name: 'qa_d_own', + objects: { [PARENT]: own, [MASTER]: { ...own, readScope: 'org' }, [DETAIL]: detail }, + }), + ]; + // A catalogue row with no organization, landed through the driver as in rig 2. + for (const name of ['qa_d_org', 'qa_d_own']) { + await (engine as any).getDriver('sys_permission_set').create('sys_permission_set', { + id: `ps_${name}_${n}`, name, label: name, organization_id: null, managed_by: 'admin', active: true, + object_permissions: JSON.stringify({}), system_permissions: JSON.stringify([]), + }); + } + for (const [user, set] of [[D_ORG, 'qa_d_org'], [D_OWN, 'qa_d_own']] as const) { + await e.insert('sys_user_permission_set', { + id: `ups_${user}_${n}`, user_id: user, permission_set: set, organization_id: ALPHA, + }, SYS); + } + const row = (id: string, owner: string, creator: string, organization = ALPHA) => + ({ id, name: id, note: '', owner_id: owner, created_by: creator, organization_id: organization }); + for (const r of [ + row('p_theirs', D_OTHER, D_OTHER), + row('p_theirs_created_by_org', D_OTHER, D_ORG), + row('p_org_mine', D_ORG, D_ORG), + row('p_org_mine_created_by_other', D_ORG, D_OTHER), + row('p_own_mine', D_OWN, D_OWN), + row('p_beta', D_ORG, D_ORG, BETA), + row('p_del_theirs_created_by_org', D_OTHER, D_ORG), + row('p_del_org_mine_created_by_other', D_ORG, D_OTHER), + ]) await e.insert(PARENT, r, SYS); + await e.insert(MASTER, row('m_org', D_ORG, D_ORG), SYS); + await e.insert(MASTER, row('m_theirs', D_OTHER, D_OTHER), SYS); + await e.insert(DETAIL, { id: 'd_under_org', note: '', created_by: D_ORG, master: 'm_org', organization_id: ALPHA }, SYS); + await e.insert(DETAIL, { id: 'd_under_theirs', note: '', created_by: D_ORG, master: 'm_theirs', organization_id: ALPHA }, SYS); + + let security: any; + let sharing: SharingService | undefined; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (type: string, name: string) => + type === 'permission' ? sets.find((s: { name?: string }) => s.name === name) ?? null : engine.getSchema(name) ?? null, + list: async () => sets, + }, + tenancy: { posture }, + get sharing() { return sharing; }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: (name: string, svc: unknown) => { if (name === 'security') security = svc; else services[name] = svc; }, + hook: () => undefined, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + sharing = new SharingService({ engine: engine as never, securityService: () => security as never }); + engine.registerMiddleware(buildSharingMiddleware(sharing, ctx.logger) as never, { object: '*' }); + // The pre-image gate logs the rows it refuses. + vi.spyOn((engine as unknown as { logger: { warn: () => void } }).logger, 'warn').mockImplementation(() => undefined); + + /** The caller's context as a request assembles it: the user's session, and for an agent its OAuth ceiling. */ + const contextOf = async (userId: string, onBehalf: boolean) => { + const authz = await resolveAuthzContext({ + userAuthGates: undefined, + apiKeyLifecycle: undefined, + ql: engine, + headers: {}, + getSession: async () => ({ + user: { id: userId }, + session: { id: `sess_${userId}_${n}`, userId, activeOrganizationId: ALPHA }, + }), + tenancyPosture: posture, + }); + return assembleExecutionContext({ + authz, + oauth: onBehalf + ? { userId, scopes: ['data:write'], clientId: 'qa_agent_client', scopePermissions: ['mcp_agent_data_write'], delegatesActions: false } + : undefined, + localization: undefined, + requestLocale: undefined, + accessToken: undefined, + } as never)!; + }; + const objects = { PARENT, MASTER, DETAIL } as const; + type Obj = keyof typeof objects; + const moderate = realCommentGate(engine, () => sharing, () => security); + + return { + /** The security service's `explain`, asked as the caller about its own by-id write — what the parent gates ask. */ + explain: async (userId: string, onBehalf: boolean, operation: 'update' | 'delete', object: Obj, recordId: string) => + explained(security.explain({ object: objects[object], operation, recordId }, { ...(await contextOf(userId, onBehalf)) })), + update: async (userId: string, onBehalf: boolean, object: Obj, id: string) => + landed(engine.update(objects[object], { note: `n_${next()}` }, { where: { id }, context: { ...(await contextOf(userId, onBehalf)) } } as never)), + remove: async (userId: string, onBehalf: boolean, object: Obj, id: string) => + landed(engine.delete(objects[object], { where: { id }, context: { ...(await contextOf(userId, onBehalf)) } } as never)), + /** The real `sys_comment` moderation gate, asked about a comment on `object/id` ({@link realCommentGate}). */ + moderate: async (userId: string, onBehalf: boolean, object: Obj, id: string) => + moderate({ ...(await contextOf(userId, onBehalf)) }, objects[object], id), teardown: async () => { try { await engine.destroy(); } catch { /* noop */ } releaseAsyncContexts(engine, plugin); @@ -773,6 +1019,25 @@ const withPrincipal = (source: PostureSource, f: (rig: PrincipalRig) => Promise< return { ...(await f(rig)), teardown: async () => undefined }; }; +type DelegatedRig = Awaited>; +/** One on-behalf-of rig per posture, booted by its first row (see {@link bootDelegated} for why its rows may share it). */ +const delegatedRigs = new Map>(); +const sharedDelegatedRig = (posture: TenancyPosture): Promise => { + let rig = delegatedRigs.get(posture); + if (!rig) { + rig = bootDelegated(posture); + delegatedRigs.set(posture, rig); + } + return rig; +}; +afterAll(async () => { + for (const rig of delegatedRigs.values()) await (await rig.catch(() => undefined))?.teardown(); +}); +const withDelegated = (posture: TenancyPosture, f: (rig: DelegatedRig) => Promise): RowRun => async () => { + const rig = await sharedDelegatedRig(posture); + return { ...(await f(rig)), teardown: async () => undefined }; +}; + const REFUSED_INVALID: Enforced = { kind: 'refused', ...INVALID }; const REFUSED_DENIED: Enforced = { kind: 'refused', ...DENIED }; /** [#21771] The read door's answer for an id it does not return, which a by-id write now gives for a row its caller cannot read. */ @@ -927,6 +1192,46 @@ function principalRows( const CLAIM_KEPT_UNDER_A_WALL = 'with no `tenancy` service, admission keeps a removed member\'s claim while Layer 0 walls at `isolated`'; +/** + * #24646 ruling item 3, as rows: an on-behalf-of caller's record-grained + * `update` / `delete` verdict against its by-id PATCH / DELETE, under one + * posture, with the delegators' own requests as the controls. `walled` names + * whether the posture walls Layer 0 (another organization's row is then one the + * caller cannot read). + */ +function delegatedRows(posture: TenancyPosture, label: string, walled: boolean): Row[] { + type Cell = [who: string, onBehalf: boolean, op: 'update' | 'delete', object: 'PARENT' | 'DETAIL', id: string, shape: string, enforced: Enforced | 'admitted']; + const cells: Cell[] = [ + // The delegated path's two legs, each deciding a row. + [D_ORG, true, 'update', 'PARENT', 'p_theirs_created_by_org', 'a row owned by someone else that it created', 'admitted'], + [D_ORG, true, 'update', 'PARENT', 'p_org_mine_created_by_other', 'its own row someone else created', REFUSED_DENIED], + [D_ORG, true, 'delete', 'PARENT', 'p_del_theirs_created_by_org', 'a row owned by someone else that it created', 'admitted'], + [D_ORG, true, 'delete', 'PARENT', 'p_del_org_mine_created_by_other', 'its own row someone else created', REFUSED_DENIED], + [D_ORG, true, 'update', 'PARENT', 'p_theirs', 'a row owned and created by someone else', REFUSED_DENIED], + [D_ORG, true, 'update', 'PARENT', 'p_org_mine', 'its own row', 'admitted'], + [D_ORG, true, 'update', 'PARENT', 'p_beta', "its own row in another organization", walled ? REFUSED_NOT_FOUND : 'admitted'], + [D_ORG, true, 'update', 'DETAIL', 'd_under_org', 'a detail under its own master', 'admitted'], + [D_ORG, true, 'update', 'DETAIL', 'd_under_theirs', "a detail under someone else's master", REFUSED_NOT_FOUND], + [D_OWN, true, 'update', 'PARENT', 'p_own_mine', 'its own row', 'admitted'], + [D_OWN, true, 'update', 'PARENT', 'p_theirs', 'a row it cannot read', REFUSED_NOT_FOUND], + // The delegators themselves. + [D_ORG, false, 'update', 'PARENT', 'p_theirs', 'a row owned and created by someone else', 'admitted'], + [D_ORG, false, 'update', 'DETAIL', 'd_under_theirs', "a detail under someone else's master", 'admitted'], + [D_ORG, false, 'update', 'PARENT', 'p_beta', 'its own row in another organization', walled ? REFUSED_NOT_FOUND : 'admitted'], + [D_OWN, false, 'update', 'DETAIL', 'd_under_org', "a detail it reads under someone else's master", REFUSED_DENIED], + ]; + return cells.map(([who, onBehalf, op, object, id, shape, enforced]) => ({ + card: '#24646 item 3', + shape: `${label}: ${onBehalf ? `an agent on behalf of the \`${who === D_ORG ? 'org' : 'own'}\` writer` : `the \`${who === D_ORG ? 'org' : 'own'}\` writer`}, ${shape}, ${op}`, + position: 'record.visible' as const, + enforced, + run: withDelegated(posture, async (r) => ({ + explain: await r.explain(who, onBehalf, op, object, id), + enforce: op === 'update' ? await r.update(who, onBehalf, object, id) : await r.remove(who, onBehalf, object, id), + })), + })); +} + const TABLE: Row[] = [ // #20604 position 1, and #20431's record-grained twin, over both orderings of one cross-class pair. ...rlsRows('#20604 P1 · #20431', 'cross-class `status != amount`', CROSS_CLASS, true), @@ -1018,6 +1323,9 @@ const TABLE: Row[] = [ explain: await r.explain(AUTHORED_WRITER, 'transfer', 'l_theirs'), enforce: await r.transfer(AUTHORED_WRITER, 'l_theirs'), })), }, + // #24646 ruling item 3: an on-behalf-of caller's by-id write verdict, under a walled and a wall-less posture. + ...delegatedRows('isolated', '`isolated`', true), + ...delegatedRows('single', '`single`', false), // #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', @@ -1087,14 +1395,16 @@ describe('security.explain answers what enforcement does — the enumeration', ( * #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. + * [#24646, ruling B] Both gates ask one question about the parent: explain's + * record-grained `update` verdict, which composes the by-id write gate's + * deferral to an app-authored row-level write policy (#24647). They no longer + * ask record sharing's `checkEdit` first — that step, at owner-only depth, is + * what used to refuse the row the deferral admits. So on such a row the gate + * admits exactly what the parent's by-id update admits, and on a row the policy + * does not admit it refuses exactly what that update refuses. The case below + * asks the REAL `sys_comment` moderation gate ({@link realCommentGate}) on the + * services enforcement runs on, so it fails the day either gate composition or + * explain's deferral moves. */ 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 () => { @@ -1113,19 +1423,29 @@ describe('an app-authored row-level write policy the by-id write gate defers to } }, 60_000); - it("the parent gates' first question, record sharing, still refuses the row explain now admits", async () => { + it("the parent gates' one question, the by-id update verdict, admits the row the by-id update 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. + // [#24646, ruling B] Flipped from "the gates' first question, record + // sharing, still refuses": record sharing alone still refuses this row at + // owner-only depth, and the gates no longer ask it. They ask explain's + // record-grained by-id `update` verdict alone, which admits the row as the + // by-id update does, and so the gate admits it. 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.moderate(AUTHORED_WRITER, 'l_theirs')).toEqual({ kind: 'admitted' }); expect(await rig.update(AUTHORED_WRITER, 'l_theirs')).toEqual({ kind: 'admitted' }); + + // The control: a row the policy does not admit is refused by the update, + // by its judgement and by the gate, the gate with its named refusal. + const refusedByUpdate = await rig.update(AUTHORED_WRITER, 'l_unnamed'); + expect(refusedByUpdate.kind).toBe('refused'); + const y = await rig.explain(AUTHORED_WRITER, 'update', 'l_unnamed'); + expect(y.kind === 'decision' ? y.decision.record?.visible : y.kind, describeExplained(y)).toBe(false); + const moderated = await rig.moderate(AUTHORED_WRITER, 'l_unnamed'); + expect(moderated.kind === 'refused' ? envelopeKeysOf(moderated) : moderated) + .toStrictEqual({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); } finally { await rig.teardown(); } @@ -1144,3 +1464,39 @@ describe('an app-authored row-level write policy the by-id write gate defers to } }, 60_000); }); + +/** + * [#24646] The REAL `sys_comment` moderation gate on rig 4's services: removing + * another person's comment on a parent record is admitted exactly where the + * by-id update of that parent is, under a walled and a wall-less posture — + * for the `org` writer ruling B admits, for a `controlled_by_parent` parent + * judged through its master, and for an on-behalf-of caller, whose by-id write + * the gate now judges by explain's delegated verdict (#24646 item 3). A + * refusal names the parent (`RECORD_NOT_ACCESSIBLE`) to a caller who reads it + * and answers the not-visible refusal (`PERMISSION_DENIED`) to one who does not. + */ +describe('the real comment moderation gate admits exactly what the parent by-id update admits', () => { + const NAMED: Enforced = { kind: 'refused', code: 'RECORD_NOT_ACCESSIBLE', status: 403 }; + const NOT_VISIBLE: Enforced = { kind: 'refused', code: 'PERMISSION_DENIED', status: 403 }; + type GateCell = [who: string, onBehalf: boolean, object: 'PARENT' | 'DETAIL', id: string, shape: string, gate: Enforced]; + const CELLS: GateCell[] = [ + [D_ORG, false, 'PARENT', 'p_theirs', 'the `org` writer, a row owned by someone else', { kind: 'admitted' }], + [D_OWN, false, 'PARENT', 'p_own_mine', 'the `own` writer, its own row', { kind: 'admitted' }], + [D_OWN, false, 'PARENT', 'p_theirs', 'the `own` writer, a row it cannot read', NOT_VISIBLE], + [D_ORG, false, 'DETAIL', 'd_under_theirs', "the `org` writer, a detail under someone else's master", { kind: 'admitted' }], + [D_OWN, false, 'DETAIL', 'd_under_org', "the `own` writer, a detail it reads under someone else's master", NAMED], + [D_ORG, true, 'PARENT', 'p_theirs_created_by_org', 'an agent on behalf of the `org` writer, a row owned by someone else that it created', { kind: 'admitted' }], + [D_ORG, true, 'PARENT', 'p_org_mine_created_by_other', 'an agent on behalf of the `org` writer, its own row someone else created', NAMED], + ]; + for (const posture of ['isolated', 'single'] as const) { + for (const [who, onBehalf, object, id, shape, gate] of CELLS) { + it(`\`${posture}\` · ${shape} → ${gate.kind === 'admitted' ? 'admitted' : `refused ${gate.code} / ${gate.status}`}`, async () => { + const rig = await sharedDelegatedRig(posture); + const moderated = await rig.moderate(who, onBehalf, object, id); + const updated = await rig.update(who, onBehalf, object, id); + expect(moderated.kind === 'refused' ? { kind: 'refused', ...envelopeKeysOf(moderated) } : moderated).toStrictEqual(gate); + expect(updated.kind, `the by-id update answered ${JSON.stringify(updated)}`).toBe(gate.kind); + }, 60_000); + } + } +}); From 84a2ff9adeeaee3475dca279794b7dc0ba48571b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 03:18:06 +0000 Subject: [PATCH 4/5] docs(changeset, permissions): the parent gates' one question, and explain's on-behalf-of write verdict Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude --- ...4646-explain-on-behalf-of-write-verdict.md | 12 +++++++ .changeset/24646-parent-gates-one-question.md | 16 +++++++++ .../docs/permissions/attachments-access.mdx | 36 +++++++++---------- 3 files changed, 46 insertions(+), 18 deletions(-) create mode 100644 .changeset/24646-explain-on-behalf-of-write-verdict.md create mode 100644 .changeset/24646-parent-gates-one-question.md diff --git a/.changeset/24646-explain-on-behalf-of-write-verdict.md b/.changeset/24646-explain-on-behalf-of-write-verdict.md new file mode 100644 index 00000000000..81c813b4a17 --- /dev/null +++ b/.changeset/24646-explain-on-behalf-of-write-verdict.md @@ -0,0 +1,12 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(plugin-security): `security.explain` answers an on-behalf-of caller's by-id update and delete the way the write itself does + +For a principal acting on behalf of a user (an OAuth agent), explain's record-grained `update` / `delete` verdict now composes what the by-id PATCH / DELETE composes (ADR-0090 D10): + +- record sharing's per-record gate is asked twice, as the write asks it: as the delegated principal at its agent-leg write depth, then as the delegator at the delegator's own write depth; +- the delegator's own row-level write filter is ANDed beside the principal's, as the write's pre-image check ANDs it. + +Before, explain asked the gate once at owner-only depth and left the delegator's write filter out, so it could refuse a row the write admits and admit a row the write refuses. Ordinary (non-delegated) callers are unaffected, and so is the delegated read verdict. diff --git a/.changeset/24646-parent-gates-one-question.md b/.changeset/24646-parent-gates-one-question.md new file mode 100644 index 00000000000..3ca12209284 --- /dev/null +++ b/.changeset/24646-parent-gates-one-question.md @@ -0,0 +1,16 @@ +--- +'@objectstack/service-storage': minor +'@objectstack/plugin-audit': minor +'@objectstack/spec': minor +--- + +feat(service-storage, plugin-audit): the attachment and comment gates judge "may edit the parent record" by the parent's own by-id update alone + +Clause-②: yes (widening) + +A caller who can update the parent record by id can also attach files to it and moderate its comments; a caller who cannot, still cannot. The gates only stop refusing what the CRUD path already admits. + +- **What widens.** The `sys_attachment` parent gate (`@objectstack/service-storage`: attaching, detaching or editing another user's attachment, re-pointing one, and `assertMayAttach`) and the `sys_comment` moderation gate (`@objectstack/plugin-audit`: editing or removing another person's comment) now ask one question about the parent record: the security service's record-grained judgement of a by-id `update` of it (`security.explain` for `update` on the parent, admitting only `allowed === true` and `record.visible === true`). They no longer ask record sharing at owner-only depth first, nor the master-detail check: both are part of that judgement already. So a caller whose write depth on the parent object is `unit`, `unit_and_below` or `org` may now attach and moderate wherever that depth lets them update the parent, and so may a caller an app-authored row-level update policy admits. +- **What does not move.** A caller the parent's by-id update refuses is refused, with the same codes as before (`ATTACHMENT_PARENT_ACCESS`, `ATTACHMENT_DELETE_DENIED`, `RECORD_NOT_ACCESSIBLE`, all 403), and a caller who cannot read the parent still gets the not-visible refusal. A `controlled_by_parent` parent is still judged through its master. Uploaders keep their own attachments and authors their own comments. Posting a comment stays a read rule on the parent. A store fault keeps its own status. +- **Kernels that judge no by-id update.** Where the security service serves no `explain` (or there is none), both gates keep exactly the composition they had: record sharing's tri-state edit verdict at owner-only depth, then the master-detail check. With no sharing service, a caller-scoped read of the parent is still asked first. +- **`@objectstack/spec`.** The `ISharingService` contract prose ("Write DEPTH is an input the CALLER supplies") now records that the parent-record gates ask the security service's by-id update judgement instead of this service, and why they never resolve the parent's depth themselves. No type, member or export changes. diff --git a/content/docs/permissions/attachments-access.mdx b/content/docs/permissions/attachments-access.mdx index 3892b5878a4..f6fac852bdb 100644 --- a/content/docs/permissions/attachments-access.mdx +++ b/content/docs/permissions/attachments-access.mdx @@ -43,30 +43,30 @@ independent of the record-level checks below. Creating a `sys_attachment` requires that the caller may **edit** the parent record — Salesforce edit-on-parent parity — and "may edit" has one meaning here: **a by-id update of that parent record, by the same caller, would be -admitted.** The gate asks three questions in order and admits only when all -three do: - -1. record sharing (`ISharingService.checkEdit`); -2. the master-detail check, for a `controlled_by_parent` parent - (`ISecurityService.checkControlledByParentWrite`); -3. the security service's judgement of that update — the record-grained verdict - `security.explain` reports for `update` on the parent record (`allowed` and - `record.visible` both). +admitted.** The gate asks exactly that question, of the security service: the +record-grained verdict `security.explain` reports for `update` on the parent +record (`allowed` and `record.visible` both). That verdict is the write path's +own composition — the parent object's grant, its row-level security, its record +sharing at the caller's write depth, the master of a `controlled_by_parent` +parent, and both legs of an on-behalf-of (agent) request — so a caller who can +update the parent record by id can also attach files to it, and a caller who +cannot, still cannot. A caller whose write depth on the parent object is +`unit`, `unit_and_below` or `org` attaches wherever that depth lets them update. The same rule governs every check that asks about the parent: attaching, detaching or editing another user's attachment, re-pointing an attachment to another record (judged on the new parent), and the file-attachments service's -caller-scoped `assertMayAttach`. It never admits more than the parent's update -does, and it can refuse more: record sharing is asked at the narrowest edit -depth for these gates (the ruling recorded on `ISharingService`, "Write DEPTH -is an input the CALLER supplies"). +caller-scoped `assertMayAttach`. When no sharing service is wired, a caller-scoped read of the parent (a -`findOne`) stands in for step 1. A kernel with no security service asks no -update judgement, as the parent's own update meets none there. A store fault in -step 2 or 3 keeps its own status (a `503` stays a `503`); it is never reported -as a refusal. `uploaded_by` is **server-stamped** from the session; a -client-supplied value is ignored. +`findOne`) is asked first. A security service that judges no by-id update (it +serves no `explain`, or there is none) leaves the gate on the composition it +had before: record sharing (`ISharingService.checkEdit`) at the narrowest edit +depth, then, for a `controlled_by_parent` parent, the master-detail check +(`ISecurityService.checkControlledByParentWrite`). A store fault keeps its own +status (a `503` stays a `503`); it is never reported as a refusal. +`uploaded_by` is **server-stamped** from the session; a client-supplied value +is ignored. | Code | Status | When | | --- | --- | --- | From d35ec05d557a6d9f0ac74ed9a1f691f7dca85865 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 03:34:48 +0000 Subject: [PATCH 5/5] test(plugin-security): type the real-gate cells' expected answer as admit-or-envelope Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude --- .../plugin-security/src/explain-enforce-parity.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 dac7bafa253..05b51fa168e 100644 --- a/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts +++ b/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts @@ -1476,9 +1476,11 @@ describe('an app-authored row-level write policy the by-id write gate defers to * and answers the not-visible refusal (`PERMISSION_DENIED`) to one who does not. */ describe('the real comment moderation gate admits exactly what the parent by-id update admits', () => { - const NAMED: Enforced = { kind: 'refused', code: 'RECORD_NOT_ACCESSIBLE', status: 403 }; - const NOT_VISIBLE: Enforced = { kind: 'refused', code: 'PERMISSION_DENIED', status: 403 }; - type GateCell = [who: string, onBehalf: boolean, object: 'PARENT' | 'DETAIL', id: string, shape: string, gate: Enforced]; + /** What the gate answers: it admits, or it refuses with an envelope. */ + type GateAnswer = { kind: 'admitted' } | ({ kind: 'refused' } & Envelope); + const NAMED: GateAnswer = { kind: 'refused', code: 'RECORD_NOT_ACCESSIBLE', status: 403 }; + const NOT_VISIBLE: GateAnswer = { kind: 'refused', code: 'PERMISSION_DENIED', status: 403 }; + type GateCell = [who: string, onBehalf: boolean, object: 'PARENT' | 'DETAIL', id: string, shape: string, gate: GateAnswer]; const CELLS: GateCell[] = [ [D_ORG, false, 'PARENT', 'p_theirs', 'the `org` writer, a row owned by someone else', { kind: 'admitted' }], [D_OWN, false, 'PARENT', 'p_own_mine', 'the `own` writer, its own row', { kind: 'admitted' }],