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 | | --- | --- | --- | 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/plugins/plugin-security/src/explain-enforce-parity.test.ts b/packages/plugins/plugin-security/src/explain-enforce-parity.test.ts index b57ef532847..05b51fa168e 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,41 @@ 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', () => { + /** 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' }], + [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); + } + } +}); 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: 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