From bca81032376f3292a85f865a9332311dbc13933f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:30:09 +0000 Subject: [PATCH 1/9] test(dogfood): pin the sys_attachment and sys_comment parent gates on a controlled_by_parent parent Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- ...t-attachment-comment-gates.dogfood.test.ts | 318 ++++++++++++++++++ .../test/fixtures/cbp-parent-gates-fixture.ts | 130 +++++++ 2 files changed, 448 insertions(+) create mode 100644 packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts create mode 100644 packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts diff --git a/packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts b/packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts new file mode 100644 index 00000000000..06fabbfe0da --- /dev/null +++ b/packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts @@ -0,0 +1,318 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The `sys_attachment` and `sys_comment` parent gates judge a +// `controlled_by_parent` parent (ADR-0055) through its MASTER — the answer the +// parent's own `PATCH` gets — driven end-to-end through the REAL surfaces: +// better-auth sign-up members, the presigned upload, the generic `/data` path, +// plugin-sharing, plugin-security, service-storage and plugin-audit. +// +// ## What this suite pins +// +// Both gates ask the sharing service whether the caller may EDIT the parent. +// `effectiveSharingModel` maps `controlled_by_parent` to `public`, so +// `checkEdit` ABSTAINS on every such parent, and the gates used to read +// `canEdit`, which folds that abstention into `true`. A member holding the +// `sys_attachment` create or delete bit (or the `sys_comment` delete bit) then +// wrote on every master-detail child record of the org, including records whose +// own `PATCH` refuses them and records they cannot even read. +// +// The gates now read `checkEdit`, and on an abstention they ask the security +// service's master-detail write check (`checkControlledByParentWrite`), the +// check a by-id update of the parent runs. So each refusal below is pinned +// beside the `PATCH` of the same record by the same caller, which is the parity +// the fix promises, and beside the security service's own answer. +// +// ## Why the boot is org-bound +// +// The platform's wildcard delete floor binds `org_member` principals, and the +// alternate matches that service-storage and the `sys_comment_moderation` +// policy contribute are what let a delete reach the parent gates at all. An +// org-less boot would measure the gates in the one posture where no floor +// applies; `assertArmed` refuses it. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync } from 'node:fs'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { resolveAuthzContext } from '@objectstack/core'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { StorageServicePlugin } from '@objectstack/service-storage'; +import { AuditPlugin } from '@objectstack/plugin-audit'; +import { cpgStack, cpgSecurity } from './fixtures/cbp-parent-gates-fixture.js'; +import { assertArmed, principalArmed } from './armed.js'; + +const SYS = { isSystem: true } as const; + +const DELETE_FLOOR = + "the platform's wildcard row-level delete floor (`owner_only_deletes`, positions ['org_member']) " + + 'and the alternate matches that let a delete reach the attachment and comment parent gates'; +const DELETE_FLOOR_DISARM = + "an org-less harness: a fresh sign-up then holds only ['everyone'], so no floor applies and the " + + 'deletes below measure the gates in a posture no org-bound deployment has. `orgContext: true` arms it.'; + +async function createdId(res: Response): Promise { + const j = (await res.json()) as any; + const id = j.id ?? j.record?.id ?? j.data?.id; + if (!id) throw new Error(`create response carried no id: ${JSON.stringify(j)}`); + return String(id); +} + +/** Status plus the error code, for an assertion message that shows the whole answer. */ +async function answer(res: Response): Promise<{ status: number; code: string | undefined; body: string }> { + const body = await res.text(); + let code: string | undefined; + try { + const j = JSON.parse(body) as any; + code = j?.code ?? j?.error?.code; + } catch { + code = undefined; + } + return { status: res.status, code, body }; +} + +/** Drive the REAL presigned three-step upload; returns the fileId. */ +async function uploadFile(stack: VerifyStack, token: string): Promise { + const auth: Record = { Authorization: `Bearer ${token}` }; + const presignRes = await stack.api('/storage/upload/presigned', { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...auth }, + body: JSON.stringify({ filename: 'quote.pdf', mimeType: 'text/plain', size: 5, scope: 'attachments' }), + }); + expect(presignRes.status, 'presign').toBe(200); + const { data } = (await presignRes.json()) as any; + const putPath = String(data.uploadUrl).replace(/^https?:\/\/[^/]+/, ''); + const putRes = await stack.raw(putPath, { + method: 'PUT', + headers: data.headers ?? { 'content-type': 'text/plain' }, + body: 'hello', + }); + expect(putRes.status, 'raw PUT').toBeLessThan(300); + const completeRes = await stack.api('/storage/upload/complete', { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...auth }, + body: JSON.stringify({ fileId: data.fileId }), + }); + expect(completeRes.status, 'complete').toBe(200); + return data.fileId as string; +} + +describe('controlled_by_parent parent gates: sys_attachment and sys_comment judge the master (ADR-0055)', () => { + let stack: VerifyStack; + let rootDir: string; + let ql: any; + let adminTok: string; + let memberTok: string; + let adminId: string; + let memberId: string; + /** Admin-owned `public_read` master: the member reads it and may not edit it. */ + let adminAccountId: string; + /** A child of it: the member reads it and its own PATCH refuses them. */ + let contractId: string; + /** Member-owned master and its child: the member may edit both. */ + let memberContractId: string; + /** A child of an admin-owned PRIVATE master: the member cannot read it at all. */ + let vaultItemId: string; + /** The control: a `public_read_write` parent. */ + let boardId: string; + + const uid = async (email: string) => (await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id; + + const attach = (token: string, parentObject: string, parentId: string, fileId: string) => + stack.apiAs(token, 'POST', '/data/sys_attachment', { + parent_object: parentObject, + parent_id: parentId, + file_id: fileId, + file_name: 'quote.pdf', + mime_type: 'text/plain', + size: 5, + }); + + /** The security service's own master-detail answer for this caller, read through the real request path. */ + const memberAnswer = async (token: string, object: string, recordId: string) => { + const authService = await stack.kernel.getServiceAsync('auth'); + let api: any = authService?.api; + if (!api && typeof authService?.getApi === 'function') api = await authService.getApi(); + const context = await resolveAuthzContext({ + ql, + headers: new Headers({ authorization: `Bearer ${token}` }), + getSession: async (h: any) => api?.getSession?.({ headers: h }), + }); + const security = await stack.kernel.getServiceAsync('security'); + expect(typeof security?.checkControlledByParentWrite, 'the security service serves the master-detail write check').toBe( + 'function', + ); + return security.checkControlledByParentWrite(object, recordId, context); + }; + + beforeAll(async () => { + rootDir = mkdtempSync(join(tmpdir(), 'cpg-dogfood-')); + stack = await bootStack(cpgStack as never, { + security: cpgSecurity(), + orgContext: true, + extraPlugins: [ + new StorageServicePlugin({ adapter: 'local', local: { rootDir }, bindToSettings: false }), + new AuditPlugin(), + ], + }); + adminTok = await stack.signIn(); + memberTok = await stack.signUp('cpg-member@verify.test'); + ql = await stack.kernel.getServiceAsync('objectql'); + adminId = await uid('admin@objectos.ai'); + memberId = await uid('cpg-member@verify.test'); + + const domainSet = await ql.findOne('sys_permission_set', { where: { name: 'cpg_files_and_threads' }, context: SYS }); + expect(domainSet?.id, 'fixture permission set seeded').toBeTruthy(); + await ql.insert('sys_user_permission_set', { user_id: memberId, permission_set_id: domainSet.id }, { context: { ...SYS } }); + + await assertArmed([ + principalArmed({ + stack, + token: memberTok, + who: 'the member (attachment and comment manager, not the master owner)', + positions: ['org_member'], + permissions: ['cpg_files_and_threads'], + control: DELETE_FLOOR, + disarmedBy: DELETE_FLOOR_DISARM, + }), + ]); + + adminAccountId = (await ql.insert('cpg_account', { name: 'admin account', owner_id: adminId }, { context: { ...SYS } })).id; + contractId = (await ql.insert('cpg_contract', { name: 'executed contract', account: adminAccountId }, { context: { ...SYS } })).id; + const memberAccountId = (await ql.insert('cpg_account', { name: 'member account', owner_id: memberId }, { context: { ...SYS } })).id; + memberContractId = ( + await ql.insert('cpg_contract', { name: 'member contract', account: memberAccountId }, { context: { ...SYS } }) + ).id; + const vaultId = (await ql.insert('cpg_vault', { name: 'admin vault', owner_id: adminId }, { context: { ...SYS } })).id; + vaultItemId = (await ql.insert('cpg_vault_item', { name: 'vault item', vault: vaultId }, { context: { ...SYS } })).id; + const boardRes = await stack.apiAs(adminTok, 'POST', '/data/cpg_board', { name: 'open board' }); + expect(boardRes.status, 'board create').toBeLessThan(300); + boardId = await createdId(boardRes); + }, 120_000); + + afterAll(async () => { + await stack?.stop(); + if (rootDir) await fs.rm(rootDir, { recursive: true, force: true }); + }); + + it('precondition: the member reads the master and the child, and the PATCH of either refuses them', async () => { + expect((await stack.apiAs(memberTok, 'GET', `/data/cpg_account/${adminAccountId}`)).status, 'member reads the master').toBe(200); + expect((await stack.apiAs(memberTok, 'GET', `/data/cpg_contract/${contractId}`)).status, 'member reads the child').toBe(200); + const patchMaster = await answer(await stack.apiAs(memberTok, 'PATCH', `/data/cpg_account/${adminAccountId}`, { name: 'x' })); + expect(patchMaster.status, `member PATCH of the master: ${patchMaster.body}`).toBe(403); + const patchChild = await answer(await stack.apiAs(memberTok, 'PATCH', `/data/cpg_contract/${contractId}`, { name: 'x' })); + expect(patchChild.status, `member PATCH of the child: ${patchChild.body}`).toBe(403); + expect(patchChild.code).toBe('PERMISSION_DENIED'); + }); + + it('PATCH parity: the security service answers the child as its PATCH does, for the member and for the owner', async () => { + expect(await memberAnswer(memberTok, 'cpg_contract', contractId)).toEqual({ outcome: 'deny', leg: 'record_sharing' }); + expect(await memberAnswer(adminTok, 'cpg_contract', contractId)).toEqual({ outcome: 'allow' }); + expect(await memberAnswer(memberTok, 'cpg_contract', memberContractId)).toEqual({ outcome: 'allow' }); + expect(await memberAnswer(memberTok, 'cpg_board', boardId)).toEqual({ outcome: 'not_applicable' }); + + const ownerPatch = await answer(await stack.apiAs(adminTok, 'PATCH', `/data/cpg_contract/${contractId}`, { name: 'executed contract' })); + expect(ownerPatch.status, `owner PATCH of the child: ${ownerPatch.body}`).toBe(200); + const memberOwnPatch = await answer( + await stack.apiAs(memberTok, 'PATCH', `/data/cpg_contract/${memberContractId}`, { name: 'member contract' }), + ); + expect(memberOwnPatch.status, `member PATCH of a child under their own master: ${memberOwnPatch.body}`).toBe(200); + }); + + it('attach on a child whose master the member cannot edit is REFUSED (403 ATTACHMENT_PARENT_ACCESS), and nothing is attached', async () => { + const fileId = await uploadFile(stack, memberTok); + const res = await answer(await attach(memberTok, 'cpg_contract', contractId, fileId)); + expect(res.status, `attach on the child: ${res.body}`).toBe(403); + expect(res.code).toBe('ATTACHMENT_PARENT_ACCESS'); + expect(await ql.findOne('sys_attachment', { where: { file_id: fileId }, context: SYS })).toBeNull(); + }); + + it('attach on a child the member cannot even read is REFUSED (403 ATTACHMENT_PARENT_ACCESS)', async () => { + const read = await stack.apiAs(memberTok, 'GET', `/data/cpg_vault_item/${vaultItemId}`); + expect(read.status, 'precondition: the member cannot read the vault item').toBe(404); + const fileId = await uploadFile(stack, memberTok); + const res = await answer(await attach(memberTok, 'cpg_vault_item', vaultItemId, fileId)); + expect(res.status, `attach on the unreadable child: ${res.body}`).toBe(403); + expect(res.code).toBe('ATTACHMENT_PARENT_ACCESS'); + expect(await ql.findOne('sys_attachment', { where: { file_id: fileId }, context: SYS })).toBeNull(); + }); + + it('control: the master owner attaches to the child, and the member attaches to a child of their own master', async () => { + const ownerFile = await uploadFile(stack, adminTok); + const owner = await answer(await attach(adminTok, 'cpg_contract', contractId, ownerFile)); + expect(owner.status, `owner attach: ${owner.body}`).toBe(201); + const memberFile = await uploadFile(stack, memberTok); + const member = await answer(await attach(memberTok, 'cpg_contract', memberContractId, memberFile)); + expect(member.status, `member attach under their own master: ${member.body}`).toBe(201); + }); + + it('control: a public_read_write parent still admits the member (abstention there is permission)', async () => { + const fileId = await uploadFile(stack, memberTok); + const res = await answer(await attach(memberTok, 'cpg_board', boardId, fileId)); + expect(res.status, `attach on the public_read_write board: ${res.body}`).toBe(201); + }); + + it("delete of another user's file on the child is REFUSED (403 ATTACHMENT_DELETE_DENIED), and the file stays", async () => { + const ownerFile = await uploadFile(stack, adminTok); + const attached = await answer(await attach(adminTok, 'cpg_contract', contractId, ownerFile)); + expect(attached.status, `owner attach: ${attached.body}`).toBe(201); + const row = await ql.findOne('sys_attachment', { where: { file_id: ownerFile }, context: SYS }); + expect(row?.id).toBeTruthy(); + + const res = await answer(await stack.apiAs(memberTok, 'DELETE', `/data/sys_attachment/${row.id}`)); + expect(res.status, `member delete of the owner's file: ${res.body}`).toBe(403); + expect(res.code).toBe('ATTACHMENT_DELETE_DENIED'); + expect(await ql.findOne('sys_attachment', { where: { id: row.id }, context: SYS }), 'the file survives').not.toBeNull(); + }); + + it('control: the uploader deletes their own file on that child', async () => { + const fileId = await uploadFile(stack, memberTok); + const own = await ql.insert( + 'sys_attachment', + { + parent_object: 'cpg_contract', + parent_id: contractId, + file_id: fileId, + file_name: 'quote.pdf', + mime_type: 'text/plain', + size: 5, + uploaded_by: memberId, + }, + { context: { ...SYS } }, + ); + const res = await answer(await stack.apiAs(memberTok, 'DELETE', `/data/sys_attachment/${own.id}`)); + expect(res.status, `uploader delete: ${res.body}`).toBe(200); + }); + + it("comment delete of another user's comment on the child is REFUSED (403 RECORD_NOT_ACCESSIBLE), and the comment stays", async () => { + const posted = await answer( + await stack.apiAs(adminTok, 'POST', '/data/sys_comment', { thread_id: `cpg_contract:${contractId}`, body: 'executed by legal' }), + ); + expect(posted.status, `owner comment: ${posted.body}`).toBeLessThan(300); + const row = await ql.findOne('sys_comment', { where: { body: 'executed by legal' }, context: SYS }); + expect(row?.id).toBeTruthy(); + + const res = await answer(await stack.apiAs(memberTok, 'DELETE', `/data/sys_comment/${row.id}`)); + expect(res.status, `member delete of the owner's comment: ${res.body}`).toBe(403); + expect(res.code).toBe('RECORD_NOT_ACCESSIBLE'); + expect(await ql.findOne('sys_comment', { where: { id: row.id }, context: SYS }), 'the comment survives').not.toBeNull(); + }); + + it('control: the author deletes their own comment on that child, and the master owner moderates it', async () => { + const mine = await answer( + await stack.apiAs(memberTok, 'POST', '/data/sys_comment', { thread_id: `cpg_contract:${contractId}`, body: 'member note one' }), + ); + expect(mine.status, `member comment: ${mine.body}`).toBeLessThan(300); + const own = await ql.findOne('sys_comment', { where: { body: 'member note one' }, context: SYS }); + const authorDelete = await answer(await stack.apiAs(memberTok, 'DELETE', `/data/sys_comment/${own.id}`)); + expect(authorDelete.status, `author delete: ${authorDelete.body}`).toBe(200); + + const second = await answer( + await stack.apiAs(memberTok, 'POST', '/data/sys_comment', { thread_id: `cpg_contract:${contractId}`, body: 'member note two' }), + ); + expect(second.status, `member comment: ${second.body}`).toBeLessThan(300); + const moderated = await ql.findOne('sys_comment', { where: { body: 'member note two' }, context: SYS }); + const ownerDelete = await answer(await stack.apiAs(adminTok, 'DELETE', `/data/sys_comment/${moderated.id}`)); + expect(ownerDelete.status, `master owner moderates: ${ownerDelete.body}`).toBe(200); + }); +}); diff --git a/packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts b/packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts new file mode 100644 index 00000000000..5ae9fc737c3 --- /dev/null +++ b/packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts @@ -0,0 +1,130 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The `sys_attachment` and `sys_comment` parent gates on a `controlled_by_parent` +// parent (ADR-0055) — the fixture behind +// `cbp-parent-attachment-comment-gates.dogfood.test.ts`. +// +// A `controlled_by_parent` object authors no access of its own: plugin-sharing's +// `effectiveSharingModel` maps it to `public`, so `checkEdit` ABSTAINS on every +// one of its rows, and the security plugin's master-detail write check +// (`ISecurityService.checkControlledByParentWrite`) is what judges an edit of +// it. The objects below put that check on both sides of the gates: +// +// cpg_account — the MASTER. `public_read` with an `owner_id`: every member +// READS it, only its owner EDITS it (record sharing). +// cpg_contract — a `controlled_by_parent` DETAIL of `cpg_account`, with files +// and feeds on. A member reads it (its master is readable) and +// may edit it only where they may edit its master. +// cpg_vault — a second MASTER, `private` (the default) with an `owner_id`: +// a non-owner member cannot even read it. +// cpg_vault_item— a `controlled_by_parent` DETAIL of `cpg_vault`, files on. A +// non-owner member cannot read it either (its master is hidden). +// cpg_board — the CONTROL: `public_read_write`. `checkEdit` abstains here +// too, and here abstention IS permission: no master exists. +// +// Every member falls back to the fixture's permissive baseline (read, create and +// edit on every object), so every refusal the suite measures is a RECORD-level +// answer, never the object gate. The domain set adds the delete bit on +// `sys_attachment` and `sys_comment`, the grant an app ships when it lets members +// manage files and moderate threads. + +import { defineStack } from '@objectstack/spec'; +import { ObjectSchema, Field } from '@objectstack/spec/data'; +import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/security'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; + +export const CpgAccount = ObjectSchema.create({ + name: 'cpg_account', + label: 'Gate Account', + pluralLabel: 'Gate Accounts', + sharingModel: 'public_read', + fields: { + name: Field.text({ label: 'Name', required: true }), + owner_id: Field.text({ label: 'Owner' }), + }, +}); + +export const CpgContract = ObjectSchema.create({ + name: 'cpg_contract', + label: 'Gate Contract', + pluralLabel: 'Gate Contracts', + sharingModel: 'controlled_by_parent', + enable: { files: true, feeds: true }, + fields: { + name: Field.text({ label: 'Name', required: true }), + account: Field.masterDetail('cpg_account', { label: 'Account', required: true }), + }, +}); + +export const CpgVault = ObjectSchema.create({ + name: 'cpg_vault', + label: 'Gate Vault', + pluralLabel: 'Gate Vaults', + // sharingModel omitted — a custom object defaults to PRIVATE (ADR-0090). + fields: { + name: Field.text({ label: 'Name', required: true }), + owner_id: Field.text({ label: 'Owner' }), + }, +}); + +export const CpgVaultItem = ObjectSchema.create({ + name: 'cpg_vault_item', + label: 'Gate Vault Item', + pluralLabel: 'Gate Vault Items', + sharingModel: 'controlled_by_parent', + enable: { files: true, feeds: true }, + fields: { + name: Field.text({ label: 'Name', required: true }), + vault: Field.masterDetail('cpg_vault', { label: 'Vault', required: true }), + }, +}); + +export const CpgBoard = ObjectSchema.create({ + name: 'cpg_board', + label: 'Gate Board', + pluralLabel: 'Gate Boards', + sharingModel: 'public_read_write', + enable: { files: true, feeds: true }, + fields: { + name: Field.text({ label: 'Name', required: true }), + }, +}); + +/** The permissive baseline every member falls back to (read, create, edit everywhere; no delete). */ +export const cpgBaselineSet: PermissionSet = PermissionSetSchema.parse({ + name: 'cpg_fixture_baseline', + label: 'Parent-gate fixture baseline — record-level answers only', + objects: { + '*': { allowRead: true, allowCreate: true, allowEdit: true }, + }, +}); + +/** The domain grant: manage attachments and moderate comments. */ +export const cpgFilesAndThreadsSet: PermissionSet = PermissionSetSchema.parse({ + name: 'cpg_files_and_threads', + label: 'Parent-gate fixture — attachment and comment manager', + objects: { + sys_attachment: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + sys_comment: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, +}); + +export function cpgSecurity(): SecurityPlugin { + return new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, cpgBaselineSet, cpgFilesAndThreadsSet], + fallbackPermissionSet: cpgBaselineSet.name, + }); +} + +export const cpgStack = defineStack({ + manifest: { + id: 'com.dogfood.cbp-parent-gates-fixture', + namespace: 'cpg', + version: '0.0.0', + type: 'app', + name: 'Controlled-by-Parent Parent Gates Fixture', + description: + 'Master-detail app exercising the sys_attachment and sys_comment parent gates on a controlled_by_parent parent.', + }, + objects: [CpgAccount, CpgContract, CpgVault, CpgVaultItem, CpgBoard], +}); From 0e90d106c1e29605166c269333e470da1d4b2dd6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:34:30 +0000 Subject: [PATCH 2/9] feat(plugin-security): serve ISecurityService.checkControlledByParentWrite from the write path's own composition Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- .../plugin-security/src/security-plugin.ts | 327 +++++++++++++----- 1 file changed, 248 insertions(+), 79 deletions(-) diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index a218573f496..8b1ac155b33 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -134,6 +134,8 @@ import { type SharingWriteVerdict, type AuthoredRowWriteVerdict, type AuthoredRowWriteOperation, + type ControlledByParentWriteDenialLeg, + type ControlledByParentWriteOutcome, type DelegationNarrowing, SEED_SETTLEMENT_SERVICE, type ISeedSettlementService, @@ -510,6 +512,43 @@ interface RlsFilterOptions { */ const CBP_MAX_CHAIN_DEPTH = 8; +/** + * [ADR-0055] Which leg of the master-detail write check a refusal came from, + * keyed by the refusal itself. Written at the one place a refusal is built + * (`masterEditDenied`, inside {@link SecurityPlugin.assertControlledByParentWrite}) + * and read only by {@link controlledByParentWriteOutcomeOf}, so the served + * member ({@link SecurityPlugin.checkControlledByParentWrite}) names the leg + * without parsing the refusal's prose, and the refusal the write path throws + * keeps its envelope byte for byte: a weak map adds no property to the error, + * so nothing new can reach a response body or a log line. + */ +const MASTER_EDIT_DENIAL_LEG = new WeakMap(); + +/** + * [ADR-0055] The master-detail write check's own outcome, read off what + * {@link SecurityPlugin.assertControlledByParentWrite} threw. `null` when the + * throw is not one of its outcomes: a store fault, or a refusal of the CONTEXT + * rather than of the master. Those are rejections, and the caller rethrows them + * unchanged, so a datasource outage keeps its `503` and is never reported as a + * verdict about the record (`ControlledByParentWriteOutcome`). + * + * The three non-verdict classes map one to one onto the `unresolvable` reasons: + * each is thrown only for the record the caller addressed, never for a master + * above it (an unresolvable ancestor is the `master_chain` leg, a refusal). + */ +function controlledByParentWriteOutcomeOf(e: unknown): ControlledByParentWriteOutcome | null { + if (e instanceof PermissionDeniedError) { + const leg = MASTER_EDIT_DENIAL_LEG.get(e); + return leg ? { outcome: 'deny', leg } : null; + } + if (e instanceof MasterDetailRelationMissingError) { + return { outcome: 'unresolvable', reason: 'master_detail_relation_missing' }; + } + if (e instanceof DetailRecordNotFoundError) return { outcome: 'unresolvable', reason: 'record_not_found' }; + if (e instanceof MasterReferenceMissingError) return { outcome: 'unresolvable', reason: 'master_reference_missing' }; + return null; +} + interface CbpRelation { /** The detail's master reference field key. */ fk: string; @@ -2269,6 +2308,14 @@ export class SecurityPlugin implements Plugin { operation: AuthoredRowWriteOperation, context?: any, ) => this.checkAuthoredRowWrite(object, recordId, operation, context), + // [ADR-0055] The master-detail write check's answer for a by-id update + // of one record — the write path's own composition, served for a gate + // outside it (the `sys_attachment` and `sys_comment` parent gates) + // that must judge a `controlled_by_parent` parent as its own update is + // judged. Served on the typed literal, so the compiler holds it to the + // contract's signature and outcome type. + checkControlledByParentWrite: (object: string, recordId: string, context?: any) => + this.checkControlledByParentWrite(object, recordId, context), // [ADR-0046 §6.7] Effective permission-set NAMES for a caller — the // primitive the REST read layer needs to evaluate a permission-set- // gated book/doc audience ({ permissionSet: '…' }). Same resolution @@ -2388,7 +2435,14 @@ export class SecurityPlugin implements Plugin { }, }); ctx.registerService('security', registeredSecurityService); - ctx.logger.info('[security] registered "security" service (getReadFilter, canReadObject, getReadableFields, getWritableFields, getQueryableFields, getMetadataReadableFields, canExport, checkAuthoredRowWrite, resolvePermissionSetNames, resolvePermissionSetsForContext, explain, audience-binding suggestions, discardPermissionSetOverlay, contributeOwnershipFloorAlternates) — ADR-0021 D-C / ADR-0090 D5/D6/D9 / ADR-0094 / ADR-0106 D7'); + // The member list is READ OFF the registered object, never typed by hand: + // a hand-kept list fell behind the served surface (five served members + // never appeared in it), and a boot log that is silent about a member + // reads as that member's absence. + ctx.logger.info( + `[security] registered "security" service (${Object.keys(registeredSecurityService).sort().join(', ')}) ` + + '— ADR-0021 D-C / ADR-0055 / ADR-0090 D5/D6/D9 / ADR-0094 / ADR-0106 D7', + ); } catch (e) { ctx.logger.warn?.('[security] failed to register "security" service', { error: (e as Error).message, @@ -2646,62 +2700,18 @@ export class SecurityPlugin implements Plugin { const positions = opCtx.context?.positions ?? []; const explicitPermissionSets = opCtx.context?.permissions ?? []; - // [#21908] ADR-0096 D5 strict mode. No position, no explicit permission - // set and no user id, on a context that is not a system one: REFUSED, - // for every verb, before anything resolves and before the operation - // runs. This used to be the hand-off (`return next()`, ADR-0096 E1): no - // CRUD gate, no RLS, no FLS mask and no tenant Layer 0. The gates above - // still run first and keep their own refusals; the object-admission - // probes and the row scope read the same predicate - // ({@link isPrincipalLessContext}). - if (isPrincipalLessContext(opCtx.context)) { - throw principalLessDenial(opCtx.operation, opCtx.object); - } - - // 1. Resolve permission sets from BOTH role names and explicit - // permission set names attached to the execution context. The - // resolution (incl. the implicit + post-resolution baseline - // fallback) is shared with the public getReadFilter service via - // resolvePermissionSetsForContext — keeping the find-path RLS and - // the analytics raw-SQL RLS provably in lock-step. - let permissionSets: PermissionSet[] = []; - try { - permissionSets = await this.resolvePermissionSetsForContext(opCtx.context); - } catch (e) { - // Fail CLOSED. A permission-resolution failure must DENY the request, - // never bypass the checks (that would let a degraded metadata service - // expose every tenant's data). System/bootstrap operations already - // short-circuited above (`opCtx.context?.isSystem`), so reaching here - // means an authenticated user request whose RBAC/RLS could not be - // resolved — deny it and alert. - ctx.logger.error( - `[security] permission resolution failed for operation '${opCtx.operation}' on ` + - `object '${opCtx.object}' (user ${opCtx.context?.userId ?? 'unknown'}) — ` + - `denying request (fail-closed)`, - e instanceof Error ? e : new Error(String(e)), - ); - throw new PermissionDeniedError( - `[Security] Access denied: permission subsystem unavailable for ` + - `operation '${opCtx.operation}' on object '${opCtx.object}'`, - ); - } - - // [ADR-0090 D10 — agent intersection] When this principal acts ON BEHALF - // OF a user (an AI agent or a service), its effective permission is the - // INTERSECTION of its own grants and the delegator's grants — never the - // union (confused-deputy prevention). Resolve the delegator's effective - // permission sets ONCE here; every gate below AND-composes the two lists - // so the tighter of the two wins at each axis (CRUD, capabilities, FLS, - // depth, row-level using/check, VAMA). The whole thing is gated on the - // presence of the delegation LINK (not the `principalKind` label — a - // service acting for a user is the identical risk): on the ordinary - // non-delegated path `delegatorSets` stays null, every combine reduces to - // today's expression, no extra `ql` read happens, and behaviour is - // byte-identical. A dangling link (delegator deleted) fails CLOSED — see - // resolveDelegatorContext for why "empty sets" would be wrong (the - // additive baseline would resurrect access for a non-existent user). - let delegatorSets: PermissionSet[] | null = null; - let delegatorContext: any = null; + // [#21908] ADR-0096 D5 strict mode, 1. permission-set resolution, and + // [ADR-0090 D10] the delegator's sets: the CONTEXT prologue, refused or + // resolved before any gate below runs. One method, because the security + // service's master-detail write member (`checkControlledByParentWrite`) + // must refuse and resolve a context exactly as this path does — see + // {@link SecurityPlugin.resolveOperationPrincipals}, which carries each + // step's reasoning. + const { permissionSets, delegatorSets, delegatorContext } = await this.resolveOperationPrincipals( + opCtx.operation, + opCtx.object, + opCtx.context, + ); // [ADR-0058 D4] What the by-id pre-image gate (step 2.7) decided about the // platform ownership floor for THIS write. The post-image check (step // 3.6) reads it so that a USING-only policy set is held, on the new row, @@ -2712,20 +2722,6 @@ export class SecurityPlugin implements Plugin { // floor on exactly the terms its `where` scope does: step 3 composes the // bulk scope with no floor options either. let preImageFloorOpts: RlsFilterOptions | null = null; - if (permissionSets.length > 0 && opCtx.context?.onBehalfOf?.userId) { - const del = await resolveDelegatorContext(this.ql, opCtx.context); - if (del.kind === 'missing') { - throw new PermissionDeniedError( - `[Security] Access denied: on-behalf-of principal names delegator ` + - `'${del.userId}', who does not exist — refusing to act (ADR-0090 D10 fail-closed)`, - { operation: opCtx.operation, object: opCtx.object }, - ); - } - if (del.kind === 'resolved') { - delegatorContext = del.context; - delegatorSets = await this.resolvePermissionSetsForContext(delegatorContext); - } - } // [ADR-0066 D2/D3] Resolve the object's security posture (private flag, // platform-global flag, capability contract) once for the checks below. @@ -9026,6 +9022,160 @@ export class SecurityPlugin implements Plugin { return { [rel.fk]: { $in: masterIds } }; } + /** + * The CONTEXT prologue of the engine middleware: a context the operation may + * not run under is refused here, before any gate, and every other context is + * resolved to the permission sets every gate below composes with — the + * principal's, and on a delegated context the delegator's too. + * + * Two callers, one prologue: the middleware itself, for every operation, and + * {@link checkControlledByParentWrite}, which must refuse and resolve a + * context exactly as the by-id update it answers for does. + * + * - [#21908] ADR-0096 D5 strict mode. No position, no explicit permission set + * and no user id, on a context that is not a system one: REFUSED, for every + * verb, before anything resolves and before the operation runs. This used + * to be the hand-off (`return next()`, ADR-0096 E1): no CRUD gate, no RLS, + * no FLS mask and no tenant Layer 0. The middleware's earlier gates still + * run first and keep their own refusals; the object-admission probes and + * the row scope read the same predicate ({@link isPrincipalLessContext}). + * - Permission sets resolve from BOTH role names and explicit permission set + * names attached to the execution context. The resolution (incl. the + * implicit + post-resolution baseline fallback) is shared with the public + * getReadFilter service via resolvePermissionSetsForContext — keeping the + * find-path RLS and the analytics raw-SQL RLS provably in lock-step. A + * resolution failure fails CLOSED: it must DENY the request, never bypass + * the checks (that would let a degraded metadata service expose every + * tenant's data). System/bootstrap operations short-circuit before this is + * asked (`context.isSystem`), so reaching that branch means an + * authenticated user request whose RBAC/RLS could not be resolved — deny it + * and alert. + * - [ADR-0090 D10 — agent intersection] When this principal acts ON BEHALF OF + * a user (an AI agent or a service), its effective permission is the + * INTERSECTION of its own grants and the delegator's grants — never the + * union (confused-deputy prevention). The delegator's effective permission + * sets resolve ONCE here; every gate AND-composes the two lists so the + * tighter of the two wins at each axis (CRUD, capabilities, FLS, depth, + * row-level using/check, VAMA). The whole thing is gated on the presence of + * the delegation LINK (not the `principalKind` label — a service acting for + * a user is the identical risk): on the ordinary non-delegated path + * `delegatorSets` stays null, every combine reduces to today's expression, + * no extra `ql` read happens, and behaviour is byte-identical. A dangling + * link (delegator deleted) fails CLOSED — see resolveDelegatorContext for + * why "empty sets" would be wrong (the additive baseline would resurrect + * access for a non-existent user). + */ + private async resolveOperationPrincipals( + operation: string, + object: string, + context: any, + ): Promise<{ permissionSets: PermissionSet[]; delegatorSets: PermissionSet[] | null; delegatorContext: any }> { + if (isPrincipalLessContext(context)) { + throw principalLessDenial(operation, object); + } + + let permissionSets: PermissionSet[] = []; + try { + permissionSets = await this.resolvePermissionSetsForContext(context); + } catch (e) { + this.logger.error?.( + `[security] permission resolution failed for operation '${operation}' on ` + + `object '${object}' (user ${context?.userId ?? 'unknown'}) — ` + + `denying request (fail-closed)`, + e instanceof Error ? e : new Error(String(e)), + ); + throw new PermissionDeniedError( + `[Security] Access denied: permission subsystem unavailable for ` + + `operation '${operation}' on object '${object}'`, + ); + } + + let delegatorSets: PermissionSet[] | null = null; + let delegatorContext: any = null; + if (permissionSets.length > 0 && context?.onBehalfOf?.userId) { + const del = await resolveDelegatorContext(this.ql, context); + if (del.kind === 'missing') { + throw new PermissionDeniedError( + `[Security] Access denied: on-behalf-of principal names delegator ` + + `'${del.userId}', who does not exist — refusing to act (ADR-0090 D10 fail-closed)`, + { operation, object }, + ); + } + if (del.kind === 'resolved') { + delegatorContext = del.context; + delegatorSets = await this.resolvePermissionSetsForContext(delegatorContext); + } + } + return { permissionSets, delegatorSets, delegatorContext }; + } + + /** + * [ADR-0055] `ISecurityService.checkControlledByParentWrite` — what the + * master-detail write check answers for a by-id UPDATE of `recordId` on + * `object` by `context`, for a gate outside the write path (the + * `sys_attachment` and `sys_comment` parent gates) that must judge a record as + * its own update is judged. + * + * It is the write path's composition, run, never re-derived: the context is + * refused or resolved by {@link resolveOperationPrincipals} (the middleware's + * own prologue), and the check is {@link assertControlledByParentWrite}, run + * for the principal's sets and then, on a delegated context, for the + * delegator's — step 2.8's two calls, in step 2.8's order, so the first + * refusal is the answer. Its outcome is read off what that check throws + * ({@link controlledByParentWriteOutcomeOf}): a leg-tagged refusal is `deny`, + * the three non-verdict errors are `unresolvable`, and anything else — a store + * fault, a refusal of the context — rejects unchanged. + * + * Two places where it does not mirror step 2.8's GUARD, both deliberate: + * + * - `permissionSets.length > 0`. A principal whose sets resolve to none is + * judged over that empty list, as the contract declares (ADR-0056 D2): it + * holds no `update` grant on any master, so it answers `deny` on + * `object_permission`. On the write path the same principal never reaches + * step 2.8 because the object-level CRUD gate refuses it first. + * - `!!context.userId`. A principal with positions or sets and no user id + * (the guest envelope) skips step 2.8 on the write path; here the check runs + * for it. Mirroring the guard would answer `allow` for a master nothing + * measured, which the contract's `allow` does not cover (it is a system + * context or every leg passing), and it would hand a REFUSING gate an + * abstention dressed as a pass. Not mirroring it cannot widen anything: the + * member's every non-`allow` outcome refuses, and the record's own update + * already refuses that principal at the object-level gate (guest bindings + * refuse `allowEdit`), so the parity of the overall answer holds. + * + * A system context answers `allow` (the write path's very first exit), and an + * object that does not declare `controlled_by_parent` answers + * `not_applicable` once the context has been admitted. + */ + private async checkControlledByParentWrite( + object: string, + recordId: string, + context?: any, + ): Promise { + if (context?.isSystem) return { outcome: 'allow' }; + const { permissionSets, delegatorSets, delegatorContext } = await this.resolveOperationPrincipals( + 'update', + object, + context, + ); + if (!this.declaresControlledByParent(object)) return { outcome: 'not_applicable' }; + // A by-id update as step 2.8 sees it: one id, addressed by `where`, under the + // caller's own context (the delegation link, when present, rides on it — the + // master's floor exclusion reads it there on both passes). + const opCtx = { object, operation: 'update', options: { where: { id: recordId } }, context }; + try { + await this.assertControlledByParentWrite(permissionSets, object, 'update', opCtx, context); + if (delegatorSets) { + await this.assertControlledByParentWrite(delegatorSets, object, 'update', opCtx, delegatorContext); + } + } catch (e) { + const outcome = controlledByParentWriteOutcomeOf(e); + if (outcome) return outcome; + throw e; + } + return { outcome: 'allow' }; + } + /** * ADR-0055 — master-detail "controlled by parent" WRITE enforcement. * @@ -9132,13 +9282,25 @@ export class SecurityPlugin implements Plugin { // that returns `never` — the same reason the `!rel` branch below throws // directly rather than routing through the helper — and the chain walk needs // real narrowing after each of its refusals. - const masterEditDenied = (reason: string, recordId?: unknown): PermissionDeniedError => - new PermissionDeniedError( + // + // [ADR-0055] Each refusal also names its LEG — the closed vocabulary the + // served member (`checkControlledByParentWrite`) reports. It is recorded + // beside the error ({@link MASTER_EDIT_DENIAL_LEG}), never on it, so the + // envelope this path throws is unchanged. + const masterEditDenied = ( + leg: ControlledByParentWriteDenialLeg, + reason: string, + recordId?: unknown, + ): PermissionDeniedError => { + const denial = new PermissionDeniedError( `[Security] Access denied: ${operation} on '${object}' requires edit access to its master record (${reason})`, { operation, object, recordId }, ); - const denyMasterEdit = (reason: string, recordId?: unknown): never => { - throw masterEditDenied(reason, recordId); + MASTER_EDIT_DENIAL_LEG.set(denial, leg); + return denial; + }; + const denyMasterEdit = (leg: ControlledByParentWriteDenialLeg, reason: string, recordId?: unknown): never => { + throw masterEditDenied(leg, reason, recordId); }; const rel = this.resolveCbpRelation(object); @@ -9317,6 +9479,7 @@ export class SecurityPlugin implements Plugin { if (visited.has(masterObject)) { throw masterEditDenied( + 'master_chain', `the controlled_by_parent chain from '${object}' re-enters '${masterObject}' (metadata cycle)`, hopMasterId, ); @@ -9324,6 +9487,7 @@ export class SecurityPlugin implements Plugin { visited.add(masterObject); if (++hops >= CBP_MAX_CHAIN_DEPTH) { throw masterEditDenied( + 'master_chain', `the controlled_by_parent chain from '${object}' exceeds the depth bound of ${CBP_MAX_CHAIN_DEPTH}`, hopMasterId, ); @@ -9332,6 +9496,7 @@ export class SecurityPlugin implements Plugin { const nextRel = this.resolveCbpRelation(masterObject); if (!nextRel) { throw masterEditDenied( + 'master_chain', `master '${masterObject}' declares controlled_by_parent with no relation to derive edit access from`, hopMasterId, ); @@ -9342,6 +9507,7 @@ export class SecurityPlugin implements Plugin { const masterRow = await this.readRowById(masterObject, hopMasterId, { isSystem: true }); if (!masterRow) { throw masterEditDenied( + 'master_chain', `master '${masterObject}' record '${String(hopMasterId)}' is not present, so its own master ` + `access cannot be derived`, hopMasterId, @@ -9350,6 +9516,7 @@ export class SecurityPlugin implements Plugin { const nextMasterId = masterRow[nextRel.fk]; if (nextMasterId == null) { throw masterEditDenied( + 'master_chain', `master '${masterObject}' has no '${nextRel.fk}' master reference to derive edit access from`, hopMasterId, ); @@ -9380,12 +9547,12 @@ export class SecurityPlugin implements Plugin { masterId: unknown, context: any, opCtx: any, - denyMasterEdit: (reason: string, recordId?: unknown) => never, + denyMasterEdit: (leg: ControlledByParentWriteDenialLeg, reason: string, recordId?: unknown) => never, ): Promise { // Master edit access = CRUD update on the master AND the master row reachable // under BOTH halves of its own write gate (write RLS + record sharing). if (!this.permissionEvaluator.checkObjectPermission('update', rel.master, permissionSets)) { - denyMasterEdit(`no edit permission on master '${rel.master}'`, masterId); + denyMasterEdit('object_permission', `no edit permission on master '${rel.master}'`, masterId); } // [commit 498f4e884] The master's own write RLS — composed with the SAME ownership // authority the by-id write pre-image gate (step 2.7) composes with, which @@ -9476,7 +9643,9 @@ export class SecurityPlugin implements Plugin { } catch { visible = null; } - if (!visible) denyMasterEdit(`master '${rel.master}' not editable by this user (row-level security)`, masterId); + if (!visible) { + denyMasterEdit('row_level_security', `master '${rel.master}' not editable by this user (row-level security)`, masterId); + } } // [#5386] The OWD / record-share half — asked UNCONDITIONALLY, because the // RLS half above is skipped whole when the master authors no write policy, @@ -9520,7 +9689,7 @@ export class SecurityPlugin implements Plugin { context, ); if (authored !== 'admit') { - denyMasterEdit(`master '${rel.master}' not editable by this user (record sharing)`, masterId); + denyMasterEdit('record_sharing', `master '${rel.master}' not editable by this user (record sharing)`, masterId); } } } From 5ae012549fc315a2a1ff1fd72efd29917c10472a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:38:11 +0000 Subject: [PATCH 3/9] test(plugin-security): pin checkControlledByParentWrite against the by-id update, and pin that it stays served Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- .../controlled-by-parent-write-member.test.ts | 377 ++++++++++++++++++ ...tered-security-service-members.pin.test.ts | 27 ++ 2 files changed, 404 insertions(+) create mode 100644 packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts new file mode 100644 index 00000000000..8e21ffcad06 --- /dev/null +++ b/packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts @@ -0,0 +1,377 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [ADR-0055] `ISecurityService.checkControlledByParentWrite`, as this plugin +// serves it, answers what a by-id UPDATE of the same record gets from the +// master-detail write check — pinned beside that update, run through the REAL +// engine middleware over the same store, for the same caller. +// +// ## Why parity is pinned rather than assumed +// +// The member exists for gates outside the write path (the `sys_attachment` and +// `sys_comment` parent gates) that must judge a `controlled_by_parent` parent +// as its own update is judged. Its whole promise is that it is the write path's +// composition, run, never re-derived. So every case below asks BOTH faces: +// +// - `patch(caller, object, id)` — the middleware, on a by-id update, the way +// the REST door runs it; +// - `member(caller, object, id)` — the registered service's member. +// +// Where step 2.8 is what refuses the update (the record is readable, its own +// gates pass), the refusal's reason names the same leg the member reports. Where +// a gate the update meets EARLIER refuses it (the record is hidden by the read +// derivation, so step 2.7 answers not-found first), both faces still refuse — +// the member answers the master check alone, which the contract says. +// +// ## The fixture +// +// cbpm_account public_read + owner_id — read by all, edited by its owner +// cbpm_contact controlled_by_parent → cbpm_account +// cbpm_region public_read_write, no owner — edit gated by an authored +// write RLS policy (`region == 'us'`) for the rep +// cbpm_region_note controlled_by_parent → cbpm_region +// cbpm_quote controlled_by_parent → cbpm_account +// cbpm_line controlled_by_parent → cbpm_quote (a two-hop chain) +// cbpm_orphan controlled_by_parent with NO master_detail relation +// +// The rep holds full CRUD on every object (so every refusal is record-level); +// the viewer — the fallback set every user resolves — holds full CRUD on the +// details and READ only on the masters (the `object_permission` leg). + +import { describe, it, expect, vi } from 'vitest'; +import { SecurityPlugin } from './security-plugin.js'; +import { SharingService, type SharingEngine } from '@objectstack/plugin-sharing'; +import { matchesFilterCondition } from '@objectstack/formula'; +import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/security'; +import type { ControlledByParentWriteOutcome, ISecurityService } from '@objectstack/spec/contracts'; +import { assertEngineFindOnePredicate } from '@objectstack/metadata-core'; + +const REP = 'usr_rep'; +const OTHER = 'usr_other'; +const VIEWER = 'usr_viewer'; + +const text = (name: string) => ({ name, type: 'text' }); +const owner = { name: 'owner_id', type: 'lookup', reference: 'sys_user' }; +const masterDetail = (name: string, reference: string) => ({ name, type: 'master_detail', required: true, reference }); + +const SCHEMAS: Record = { + cbpm_account: { name: 'cbpm_account', sharingModel: 'public_read', fields: { id: text('id'), name: text('name'), owner_id: owner } }, + cbpm_contact: { + name: 'cbpm_contact', + sharingModel: 'controlled_by_parent', + fields: { id: text('id'), name: text('name'), account: masterDetail('account', 'cbpm_account') }, + }, + cbpm_region: { name: 'cbpm_region', sharingModel: 'public_read_write', fields: { id: text('id'), region: text('region') } }, + cbpm_region_note: { + name: 'cbpm_region_note', + sharingModel: 'controlled_by_parent', + fields: { id: text('id'), name: text('name'), region_ref: masterDetail('region_ref', 'cbpm_region') }, + }, + cbpm_quote: { + name: 'cbpm_quote', + sharingModel: 'controlled_by_parent', + fields: { id: text('id'), name: text('name'), account: masterDetail('account', 'cbpm_account') }, + }, + cbpm_line: { + name: 'cbpm_line', + sharingModel: 'controlled_by_parent', + fields: { id: text('id'), name: text('name'), quote: masterDetail('quote', 'cbpm_quote') }, + }, + // Authored wrong: `controlled_by_parent` with an OPTIONAL lookup, so there is + // no relation to derive access from (`resolveCbpRelation` finds none). + cbpm_orphan: { + name: 'cbpm_orphan', + sharingModel: 'controlled_by_parent', + fields: { id: text('id'), name: text('name'), account: { name: 'account', type: 'lookup', reference: 'cbpm_account' } }, + }, + sys_record_share: { + name: 'sys_record_share', + isSystem: true, + fields: { + id: text('id'), + object_name: text('object_name'), + record_id: text('record_id'), + recipient_type: text('recipient_type'), + recipient_id: text('recipient_id'), + access_level: text('access_level'), + }, + }, + sys_user: { name: 'sys_user', isSystem: true, fields: { id: text('id'), name: text('name') } }, +}; + +const CRUD = { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }; +const READ = { allowRead: true }; +const DETAILS = ['cbpm_contact', 'cbpm_region_note', 'cbpm_quote', 'cbpm_line', 'cbpm_orphan']; + +const REP_SET: PermissionSet = PermissionSetSchema.parse({ + name: 'cbpm_rep', + objects: Object.fromEntries([...DETAILS, 'cbpm_account', 'cbpm_region'].map((o) => [o, CRUD])), + rowLevelSecurity: [{ name: 'cbpm_region_us_edits', object: 'cbpm_region', operation: 'update', using: "region == 'us'" }], +}); + +const VIEWER_SET: PermissionSet = PermissionSetSchema.parse({ + name: 'cbpm_viewer', + objects: { + ...Object.fromEntries(DETAILS.map((o) => [o, CRUD])), + cbpm_account: READ, + cbpm_region: READ, + }, +}); + +type Row = Record; + +function fixtureRows(): Record { + return { + cbpm_account: [ + { id: 'acct_own', name: 'Rep owns', owner_id: REP }, + { id: 'acct_other', name: 'Other owns, not shared', owner_id: OTHER }, + { id: 'acct_shared', name: 'Other owns, shared to the rep at edit', owner_id: OTHER }, + ], + cbpm_contact: [ + { id: 'c_own', name: 'under acct_own', account: 'acct_own' }, + { id: 'c_other', name: 'under acct_other', account: 'acct_other' }, + { id: 'c_shared', name: 'under acct_shared', account: 'acct_shared' }, + { id: 'c_dangling', name: 'no master reference', account: null }, + ], + cbpm_region: [ + { id: 'reg_us', region: 'us' }, + { id: 'reg_eu', region: 'eu' }, + ], + cbpm_region_note: [ + { id: 'rn_us', name: 'under reg_us', region_ref: 'reg_us' }, + { id: 'rn_eu', name: 'under reg_eu', region_ref: 'reg_eu' }, + ], + cbpm_quote: [ + { id: 'q_own', name: 'under acct_own', account: 'acct_own' }, + { id: 'q_dangling', name: 'no master reference', account: null }, + ], + cbpm_line: [ + { id: 'line_ok', name: 'under q_own', quote: 'q_own' }, + { id: 'line_above_dangling', name: 'under q_dangling', quote: 'q_dangling' }, + ], + cbpm_orphan: [{ id: 'orphan_1', name: 'no relation', account: 'acct_own' }], + sys_record_share: [ + { + id: 'shr_1', + object_name: 'cbpm_account', + record_id: 'acct_shared', + recipient_type: 'user', + recipient_id: REP, + access_level: 'edit', + }, + ], + sys_user: [ + { id: REP, name: 'Rep' }, + { id: OTHER, name: 'Other' }, + { id: VIEWER, name: 'Viewer' }, + ], + }; +} + +/** The store fault this suite injects: the datasource is down for one record. */ +class DatasourceUnavailable extends Error { + readonly code = 'ERR_DATASOURCE_UNAVAILABLE'; + readonly status = 503; + readonly statusCode = 503; +} + +/** + * READ-surface-only engine double over `matchesFilterCondition`, the evaluator + * the plugin itself uses. `faultOn` makes any read naming that id throw a 503, + * the shape a datasource outage takes. + */ +function makeStore(rows: Record, faultOn?: string) { + const fault = (options: any) => { + if (faultOn !== undefined && JSON.stringify(options?.where ?? {}).includes(`"${faultOn}"`)) { + throw new DatasourceUnavailable(`datasource unavailable reading '${faultOn}'`); + } + }; + return { + rows, + getSchema: (object: string) => SCHEMAS[object], + find: vi.fn(async (object: string, options: any = {}) => { + fault(options); + const hits = (rows[object] ?? []).filter((r) => matchesFilterCondition(r, options?.where ?? null)); + return typeof options?.limit === 'number' ? hits.slice(0, options.limit) : hits; + }), + findOne: vi.fn(async (object: string, options: any = {}) => { + assertEngineFindOnePredicate(object, options); + fault(options); + return (rows[object] ?? []).find((r) => matchesFilterCondition(r, options?.where ?? null)) ?? null; + }), + }; +} + +async function boot(options: { faultOn?: string } = {}) { + const store = makeStore(fixtureRows(), options.faultOn); + const sets = [VIEWER_SET, REP_SET]; + let middleware: any; + const ql = { + registerMiddleware: (mw: any) => { + if (!middleware) middleware = mw; + }, + getSchema: store.getSchema, + find: store.find, + findOne: store.findOne, + }; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: ql, + metadata: { get: async (n: string) => store.getSchema(n), list: async () => sets }, + sharing: new SharingService({ engine: store as unknown as SharingEngine }), + }; + const registerService = vi.fn(); + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ defaultPermissionSets: sets, fallbackPermissionSet: 'cbpm_viewer' }); + await plugin.init(ctx); + await plugin.start(ctx); + const security = registerService.mock.calls.find((c: unknown[]) => c[0] === 'security')?.[1] as ISecurityService; + if (typeof security?.checkControlledByParentWrite !== 'function') { + throw new Error('the registered security service does not serve checkControlledByParentWrite'); + } + const serve = security.checkControlledByParentWrite.bind(security); + + /** A by-id update through the engine middleware. Resolves `'admitted'` or the refusal it threw. */ + const patch = async (context: any, object: string, id: string): Promise<'admitted' | any> => { + const opCtx: any = { object, operation: 'update', data: { id, name: 'edited' }, options: { where: { id } }, context }; + try { + await middleware(opCtx, async () => {}); + return 'admitted'; + } catch (e) { + return e; + } + }; + const member = (context: any, object: string, id: string) => serve(object, id, context); + return { store, ctx, patch, member }; +} + +const rep = () => ({ userId: REP, tenantId: 'org-1', positions: [], permissions: ['cbpm_rep'] }); +const viewer = () => ({ userId: VIEWER, tenantId: 'org-1', positions: [], permissions: [] }); + +/** The reason phrase step 2.8 writes for each leg — how a PATCH refusal names its leg. */ +const LEG_REASON: Record = { + object_permission: /requires edit access to its master record \(no edit permission on master/, + row_level_security: /requires edit access to its master record \(master '[a-z_]+' not editable by this user \(row-level security\)\)/, + record_sharing: /requires edit access to its master record \(master '[a-z_]+' not editable by this user \(record sharing\)\)/, +}; + +describe('[ADR-0055] checkControlledByParentWrite answers what the by-id update gets from the master check', () => { + it('allow ⇔ the update is admitted: an owned master, a master shared at edit, an RLS-admitted master, a two-hop chain', async () => { + const h = await boot(); + for (const [object, id] of [ + ['cbpm_contact', 'c_own'], + ['cbpm_contact', 'c_shared'], + ['cbpm_region_note', 'rn_us'], + ['cbpm_line', 'line_ok'], + ] as const) { + expect(await h.patch(rep(), object, id), `PATCH ${object}/${id}`).toBe('admitted'); + expect(await h.member(rep(), object, id), `member ${object}/${id}`).toEqual({ outcome: 'allow' }); + } + }); + + it('deny on the leg step 2.8 refuses on, for a record the caller can read', async () => { + const h = await boot(); + const cases: Array<[any, string, string, ControlledByParentWriteOutcome]> = [ + [rep(), 'cbpm_contact', 'c_other', { outcome: 'deny', leg: 'record_sharing' }], + [rep(), 'cbpm_region_note', 'rn_eu', { outcome: 'deny', leg: 'row_level_security' }], + [viewer(), 'cbpm_contact', 'c_own', { outcome: 'deny', leg: 'object_permission' }], + ]; + for (const [context, object, id, expected] of cases) { + const refused = await h.patch(context, object, id); + expect(refused, `PATCH ${object}/${id} by ${context.userId}`).not.toBe('admitted'); + expect(refused).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(String(refused.message)).toMatch(LEG_REASON[(expected as any).leg]); + expect(await h.member(context, object, id), `member ${object}/${id} by ${context.userId}`).toEqual(expected); + } + }); + + it('the ADR-0090 D10 delegator leg: the principal may edit the master, the delegator may not ⇒ deny on the delegator\'s leg', async () => { + const h = await boot(); + const delegated = { ...rep(), onBehalfOf: { userId: VIEWER } }; + // The rep alone may edit c_own's master; on behalf of the viewer, the + // viewer's pass refuses (no update grant on the master) — first refusal wins. + expect(await h.member(rep(), 'cbpm_contact', 'c_own')).toEqual({ outcome: 'allow' }); + const refused = await h.patch(delegated, 'cbpm_contact', 'c_own'); + expect(refused).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(String(refused.message)).toMatch(LEG_REASON.object_permission); + expect(await h.member(delegated, 'cbpm_contact', 'c_own')).toEqual({ outcome: 'deny', leg: 'object_permission' }); + }); + + it('master_chain: a master above the record\'s own master with no master reference ⇒ deny, and the update refuses', async () => { + const h = await boot(); + expect(await h.patch(rep(), 'cbpm_line', 'line_above_dangling')).not.toBe('admitted'); + expect(await h.member(rep(), 'cbpm_line', 'line_above_dangling')).toEqual({ outcome: 'deny', leg: 'master_chain' }); + }); + + it('unresolvable for the three non-verdicts, and the update refuses each', async () => { + const h = await boot(); + const cases: Array<[string, string, ControlledByParentWriteOutcome]> = [ + ['cbpm_contact', 'c_missing', { outcome: 'unresolvable', reason: 'record_not_found' }], + ['cbpm_contact', 'c_dangling', { outcome: 'unresolvable', reason: 'master_reference_missing' }], + ['cbpm_orphan', 'orphan_1', { outcome: 'unresolvable', reason: 'master_detail_relation_missing' }], + ]; + for (const [object, id, expected] of cases) { + expect(await h.patch(rep(), object, id), `PATCH ${object}/${id}`).not.toBe('admitted'); + expect(await h.member(rep(), object, id), `member ${object}/${id}`).toEqual(expected); + } + }); + + it('not_applicable for an object that derives nothing from a master; a system context answers allow', async () => { + const h = await boot(); + expect(await h.member(rep(), 'cbpm_account', 'acct_own')).toEqual({ outcome: 'not_applicable' }); + expect(await h.member(rep(), 'cbpm_region', 'reg_eu')).toEqual({ outcome: 'not_applicable' }); + expect(await h.member({ isSystem: true }, 'cbpm_contact', 'c_other')).toEqual({ outcome: 'allow' }); + expect(await h.patch({ isSystem: true }, 'cbpm_contact', 'c_other')).toBe('admitted'); + }); + + it('a principal with no user id is judged, not passed: deny where the write path skips step 2.8', async () => { + // Step 2.8 is guarded by `!!context.userId` on the write path; the member + // does not mirror the guard (see its docblock). The positions resolve no + // set, so the principal holds no update grant on the master. + const h = await boot(); + const guest = { positions: ['guest_like'], permissions: [] }; + expect(await h.patch(guest, 'cbpm_contact', 'c_own')).not.toBe('admitted'); + expect(await h.member(guest, 'cbpm_contact', 'c_own')).toEqual({ outcome: 'deny', leg: 'object_permission' }); + }); +}); + +describe('[ADR-0055] checkControlledByParentWrite rejects where the write path refuses the request, never resolving a verdict', () => { + it('a principal-less context rejects with the write path\'s own refusal', async () => { + const h = await boot(); + const refused = await h.patch({}, 'cbpm_contact', 'c_own'); + expect(refused).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + await expect(h.member({}, 'cbpm_contact', 'c_own')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + message: refused.message, + }); + }); + + it('an on-behalf-of link naming a delegator who does not exist rejects with the write path\'s own refusal', async () => { + const h = await boot(); + const dangling = { ...rep(), onBehalfOf: { userId: 'usr_gone' } }; + const refused = await h.patch(dangling, 'cbpm_contact', 'c_own'); + expect(refused).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + await expect(h.member(dangling, 'cbpm_contact', 'c_own')).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + message: refused.message, + }); + }); + + it('a store fault rejects with the engine\'s own error, unchanged — its 503 is never reported as a verdict', async () => { + const h = await boot({ faultOn: 'c_own' }); + const rejection = await h.member(rep(), 'cbpm_contact', 'c_own').then( + (outcome) => ({ resolved: outcome }), + (e) => e, + ); + expect(rejection).toBeInstanceOf(DatasourceUnavailable); + expect(rejection).toMatchObject({ code: 'ERR_DATASOURCE_UNAVAILABLE', status: 503 }); + }); +}); diff --git a/packages/plugins/plugin-security/src/registered-security-service-members.pin.test.ts b/packages/plugins/plugin-security/src/registered-security-service-members.pin.test.ts index c41c8bec13d..6f32d114b85 100644 --- a/packages/plugins/plugin-security/src/registered-security-service-members.pin.test.ts +++ b/packages/plugins/plugin-security/src/registered-security-service-members.pin.test.ts @@ -38,6 +38,9 @@ * direction is used only as this pin's non-vacuity control: every REQUIRED * member must be among the enumerated members, which fails if the boot stopped * reaching the registration or the enumeration stopped seeing the members. + * The one exception is `MUST_SERVE_OPTIONAL`: an optional member whose absence + * its consumers read as a fallback that admits, which this plugin therefore + * must keep serving. */ import { describe, it, expect, vi } from 'vitest'; @@ -87,6 +90,21 @@ const DECLARED_MEMBERS = { */ const SERVED_NOT_DECLARED: Readonly> = {}; +/** + * OPTIONAL members this plugin must SERVE, each with the reason its absence is + * not harmless here. An optional member's absence is legal for the contract, + * and its consumers read it as a declared fallback — so a plugin that stopped + * serving one would not fail anywhere: every consumer would quietly take that + * fallback. Listed here, a member that is no longer served turns this pin red + * by name. + */ +const MUST_SERVE_OPTIONAL: Readonly>> = { + checkControlledByParentWrite: + 'the sys_attachment and sys_comment parent gates read its absence as "no master-detail check composed" ' + + 'and ADMIT on a controlled_by_parent parent; this plugin runs that check on every by-id write, so it ' + + 'must serve it, or those gates write past the master the record\'s own update is refused by', +}; + /** * Every member name the object exposes, along its whole prototype chain up to * (not including) `Object.prototype`, enumerable or not, symbols included. A @@ -169,6 +187,15 @@ describe('the registered `security` service serves only what ISecurityService de } }); + it('every optional member this plugin must serve is served, and is still declared optional', async () => { + const served = servedMembers(await registeredSecurityService()); + for (const [name, reason] of Object.entries(MUST_SERVE_OPTIONAL)) { + expect(DECLARED_MEMBERS[name as keyof typeof DECLARED_MEMBERS], `'${name}' is not declared optional`).toBe('optional'); + expect(reason!.trim().length, `'${name}' carries no reason`).toBeGreaterThan(0); + expect(served, `'${name}' is no longer served: ${reason}`).toContain(name); + } + }); + it('the two extension members are served with the signatures the contract declares (compile-time)', () => { // The `Object.assign` half is not typed against the contract, so these two // witnesses are: each is the contract's member type, implemented by From 0589a5cbe6873f7bf07e2554ade150c00845a752 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:41:59 +0000 Subject: [PATCH 4/9] fix(service-storage): the attachment parent gate judges a controlled_by_parent parent through its master Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- .../src/attachment-access-hooks.test.ts | 164 ++++++++++++++- .../src/attachment-access-hooks.ts | 190 ++++++++++++------ .../src/attachment-delete-floor-alternate.ts | 4 +- .../services/service-storage/src/index.ts | 2 +- .../src/storage-service-plugin.ts | 12 ++ 5 files changed, 302 insertions(+), 70 deletions(-) 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 a4351a61114..d7f29c65c37 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -3,7 +3,11 @@ import { describe, it, expect, vi } from 'vitest'; import { ObjectQL, assertEngineFindOnePredicate } from '@objectstack/objectql'; import { renderOperationMessage } from '@objectstack/spec/system'; -import { installAttachmentAccessHooks, type AttachmentSharingLike } from './attachment-access-hooks.js'; +import { + installAttachmentAccessHooks, + type AttachmentSecurityLike, + type AttachmentSharingLike, +} from './attachment-access-hooks.js'; import type { AttachmentLifecycleEngine } from './attachment-lifecycle.js'; const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); @@ -31,10 +35,33 @@ function readableParentFind(readable: string[], reads: Array<{ object: string; o }; } +/** + * A two-state sharing double, as the suites below were written against: `true` + * (may edit) or `false`. The gate reads the TRI-STATE `checkEdit` since the + * `controlled_by_parent` fix, and these doubles stand for its two verdicts + * whose meaning did not move — `true` is `allow` (admits), `false` is `deny` + * (refuses). `abstain` never comes out of one; the suite that pins it (end of + * file) hands the gate a tri-state double directly. + */ +type TwoStateSharingDouble = { + canEdit: (object: string, recordId: string, context: any) => Promise; +}; + +function asTriState(sharing: AttachmentSharingLike | TwoStateSharingDouble | null | undefined) { + if (!sharing || !('canEdit' in sharing)) return sharing; + const { canEdit } = sharing; + return { + checkEdit: async (object: string, recordId: string, context: any) => + (await canEdit(object, recordId, context)) ? ('allow' as const) : ('deny' as const), + } satisfies AttachmentSharingLike; +} + /** Capture the three registered hooks so tests can drive them directly. */ function install(opts: { attachments?: Array>; - sharing?: AttachmentSharingLike | null; + sharing?: AttachmentSharingLike | TwoStateSharingDouble | null; + /** The security service, for its master-detail write check (#22455). */ + security?: AttachmentSecurityLike | null; /** Parents the CALLER can read through the engine, keyed `object/id`. */ readable?: string[]; messageTranslator?: () => ((key: string, locale: string, params?: Record) => string) | undefined; @@ -59,7 +86,8 @@ function install(opts: { ) ?? null; }, update: async () => ({}), }; - installAttachmentAccessHooks(engine, () => opts.sharing, silentLogger(), opts.messageTranslator); + const sharing = asTriState(opts.sharing); + installAttachmentAccessHooks(engine, () => sharing, silentLogger(), opts.messageTranslator, () => opts.security); return { beforeInsert: hooks.get('beforeInsert')!, beforeUpdate: hooks.get('beforeUpdate')!, @@ -544,7 +572,7 @@ function makeWiredDriver() { async function bootWired(opts: { attachments?: Array>; - sharing?: AttachmentSharingLike | null; + sharing?: AttachmentSharingLike | TwoStateSharingDouble | null; /** `att_secret` rows that EXIST — and, with no security middleware on this * rig, are therefore readable by every caller. Absent ⇒ no parent is. */ parents?: string[]; @@ -556,7 +584,8 @@ async function bootWired(opts: { ql.registry.registerObject(sysAttachmentObject as any, 'app:test'); ql.registry.registerObject(attSecretObject as any, 'app:test'); // `engine as any` mirrors the production wiring in storage-service-plugin.ts. - installAttachmentAccessHooks(ql as any, () => opts.sharing ?? null, silentLogger()); + const sharing = asTriState(opts.sharing) ?? null; + installAttachmentAccessHooks(ql as any, () => sharing, silentLogger()); if (!driver.stores.get('sys_attachment')) driver.stores.set('sys_attachment', new Map()); for (const row of opts.attachments ?? []) { driver.stores.get('sys_attachment')!.set(String(row.id), { ...row }); @@ -1375,3 +1404,128 @@ describe('a refusal on a parent the caller cannot read names nothing', () => { expect(seen.remaining()).toBe(1); }); }); + +// ── [ADR-0055] a controlled_by_parent parent is judged through its master ── +// +// `checkEdit` ABSTAINS on every `controlled_by_parent` parent (plugin-sharing +// maps the model to `public`), and the gate used to read `canEdit`, which folds +// that abstention into `true`. These pin the composition the gate now runs on +// all four limbs: `allow` admits and `deny` refuses without asking anything +// else; an abstention asks the security service's master-detail write check, +// whose `allow` and `not_applicable` admit and whose `deny` and `unresolvable` +// refuse; a rejection propagates unchanged; and a kernel without that member +// keeps the abstention's admit. +describe('[ADR-0055] parent EDIT on a controlled_by_parent parent — checkEdit, then the master-detail write check', () => { + type Verdict = 'allow' | 'abstain' | 'deny'; + type Outcome = Awaited>>; + + const triState = (verdict: Verdict) => { + const checkEdit = vi.fn(async (_o: string, _r: string, _c: any) => verdict); + return { checkEdit, sharing: { checkEdit } satisfies AttachmentSharingLike }; + }; + const masterCheck = (answer: Outcome | Error) => { + const checkControlledByParentWrite = vi.fn(async (_o: string, _r: string, _c?: any): Promise => { + if (answer instanceof Error) throw answer; + return answer; + }); + return { checkControlledByParentWrite, security: { checkControlledByParentWrite } satisfies AttachmentSecurityLike }; + }; + const attachCtx = () => insertCtx({ parent_object: 'cbp_contract', parent_id: 'k1', file_id: 'f1' }, { userId: 'member' }); + + // The truth table, on the attach limb. `null` for the member column = never asked. + const TABLE: Array<{ verdict: Verdict; member: Outcome | null; admits: boolean }> = [ + { verdict: 'allow', member: null, admits: true }, + { verdict: 'deny', member: null, admits: false }, + { verdict: 'abstain', member: { outcome: 'allow' }, admits: true }, + { verdict: 'abstain', member: { outcome: 'not_applicable' }, admits: true }, + { verdict: 'abstain', member: { outcome: 'deny', leg: 'record_sharing' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'deny', leg: 'object_permission' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'unresolvable', reason: 'master_detail_relation_missing' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'unresolvable', reason: 'record_not_found' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'unresolvable', reason: 'master_reference_missing' }, admits: false }, + ]; + for (const row of TABLE) { + const label = `checkEdit ${row.verdict}${row.member ? ` × member ${JSON.stringify(row.member)}` : ''}`; + it(`attach: ${label} → ${row.admits ? 'admits' : 'refuses 403 ATTACHMENT_PARENT_ACCESS'}`, async () => { + const { sharing } = triState(row.verdict); + const { security, checkControlledByParentWrite } = masterCheck(row.member ?? { outcome: 'deny', leg: 'record_sharing' }); + const { beforeInsert } = install({ 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: 'cbp_contract' }); + // The master check is asked on an abstention, and only then. + expect(checkControlledByParentWrite).toHaveBeenCalledTimes(row.member ? 1 : 0); + }); + } + + it('the member is asked about the parent, with the caller envelope the sharing gate gets', async () => { + const { sharing, checkEdit } = triState('abstain'); + const { security, checkControlledByParentWrite } = masterCheck({ outcome: 'allow' }); + const { beforeInsert } = install({ sharing, security }); + await beforeInsert(attachCtx()); + expect(checkControlledByParentWrite).toHaveBeenCalledWith('cbp_contract', 'k1', checkEdit.mock.calls[0]![2]); + expect(checkControlledByParentWrite.mock.calls[0]![2]).toMatchObject({ userId: 'member' }); + }); + + it('a store fault from the master check propagates unchanged — its 503 is never an admit or a 403', async () => { + const outage = Object.assign(new Error('datasource unavailable'), { code: 'ERR_DATASOURCE_UNAVAILABLE', status: 503 }); + const { sharing } = triState('abstain'); + const { security } = masterCheck(outage); + const { beforeInsert } = install({ sharing, security }); + await expect(beforeInsert(attachCtx())).rejects.toBe(outage); + }); + + it('a kernel without the member keeps the abstention\'s admit — no security service, or one that does not serve it', async () => { + const { sharing } = triState('abstain'); + await expect(install({ sharing, security: null }).beforeInsert(attachCtx())).resolves.toBeUndefined(); + await expect( + install({ sharing, security: {} as AttachmentSecurityLike }).beforeInsert(attachCtx()), + ).resolves.toBeUndefined(); + }); + + it("delete: another user's file on a child whose master the caller cannot edit is refused; the uploader's own is not asked", async () => { + const row = { id: 'a1', file_id: 'f1', parent_object: 'cbp_contract', parent_id: 'k1', uploaded_by: 'owner' }; + const { sharing } = triState('abstain'); + const { security, checkControlledByParentWrite } = masterCheck({ outcome: 'deny', leg: 'record_sharing' }); + const refused = install({ attachments: [row], sharing, security, readable: ['cbp_contract/k1'] }); + await expect(refused.beforeDelete(deleteCtx({ id: 'a1' }, { userId: 'member' }))).rejects.toMatchObject({ + code: 'ATTACHMENT_DELETE_DENIED', + status: 403, + object: 'cbp_contract', + }); + expect(checkControlledByParentWrite).toHaveBeenCalledTimes(1); + + const own = install({ attachments: [{ ...row, uploaded_by: 'member' }], sharing, security }); + await expect(own.beforeDelete(deleteCtx({ id: 'a1' }, { userId: 'member' }))).resolves.toBeUndefined(); + expect(checkControlledByParentWrite).toHaveBeenCalledTimes(1); // not asked again + }); + + it('delete: a caller who cannot read the parent gets the not-visible refusal, as on any other parent (#21755)', async () => { + const row = { id: 'a1', file_id: 'f1', parent_object: 'cbp_contract', parent_id: 'k1', uploaded_by: 'owner' }; + const { sharing } = triState('abstain'); + const { security } = masterCheck({ outcome: 'deny', leg: 'record_sharing' }); + const { beforeDelete } = install({ attachments: [row], sharing, security, readable: [] }); + await expect(beforeDelete(deleteCtx({ id: 'a1' }, { userId: 'member' }))).rejects.toMatchObject({ + code: 'PERMISSION_DENIED', + status: 403, + }); + }); + + it('update and re-point: the same composition refuses the row rule and the attach rule on the new parent', async () => { + const row = { id: 'a1', file_id: 'f1', parent_object: 'cbp_contract', parent_id: 'k1', uploaded_by: 'owner' }; + const { sharing } = triState('abstain'); + const denied = masterCheck({ outcome: 'deny', leg: 'record_sharing' }); + const rowRule = install({ attachments: [row], sharing, security: denied.security, readable: ['cbp_contract/k1'] }); + await expect( + rowRule.beforeUpdate(updateCtx({ id: 'a1' }, { description: 'x' }, { userId: 'member' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + + // The uploader may edit their own row, but moving it onto a child whose + // master they cannot edit is an attach there — refused. + const repoint = install({ attachments: [{ ...row, uploaded_by: 'member' }], sharing, security: denied.security }); + await expect( + repoint.beforeUpdate(updateCtx({ id: 'a1' }, { parent_id: 'k2' }, { userId: 'member' })), + ).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403, object: 'cbp_contract' }); + expect(denied.checkControlledByParentWrite).toHaveBeenLastCalledWith('cbp_contract', 'k2', expect.objectContaining({ userId: 'member' })); + }); +}); diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 3e932d379cf..c078a57acb1 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -2,6 +2,7 @@ import { withoutOperationPrivateKeys } from '@objectstack/core'; import type { StandardErrorCode } from '@objectstack/spec/api'; +import type { ISecurityService, ISharingService } from '@objectstack/spec/contracts'; import type { ExecutionContext } from '@objectstack/spec/kernel'; import { renderOperationMessage, type ValidationMessageTranslator } from '@objectstack/spec/system'; @@ -21,12 +22,11 @@ import type { * semantics (ContentDocumentLink): an attachment's access is derived from * its PARENT record. * - * - beforeInsert: the caller must be able to READ the parent record — - * verified with a caller-scoped findOne so RLS/OWD/sharing apply. - * Fail-closed 403 `ATTACHMENT_PARENT_ACCESS`. (Salesforce requires edit - * on the parent; v1 enforces read visibility — strictly better than - * nothing, edit-parity is a tracked follow-up.) `uploaded_by` is - * server-stamped from the session — a client-supplied value never wins. + * - beforeInsert: the caller must be able to EDIT the parent record (see + * "Parent EDIT" below; Salesforce parity, #2970 item 3 — v1 asked read + * visibility, which is now only the degraded mode). Fail-closed 403 + * `ATTACHMENT_PARENT_ACCESS`. `uploaded_by` is server-stamped from the + * session — a client-supplied value never wins. * - beforeUpdate (commit da891e0ef): the caller must be the uploader OR hold edit on * the parent record — the delete rule, applied to the verb that could * otherwise rewrite the other two gates away: an ungated update let any @@ -46,8 +46,8 @@ import type { * {@link refuseNotVisible}. The named refusals below are what a caller who * can read the parent, but may not edit it, receives. * - beforeDelete: the caller must be the uploader OR hold edit on the - * parent record (sharing service's `canEdit`; public-model parents are - * editable by design). Fail-closed 403 `ATTACHMENT_DELETE_DENIED`; a + * parent record (see "Parent EDIT" below). Fail-closed 403 + * `ATTACHMENT_DELETE_DENIED`; a * multi-delete requires EVERY matched row to pass, and one carrying * NEITHER an id NOR a `where` is refused outright (#4757) — the engine * would hand `deleteMany` an AST over the whole table, and a gate that @@ -56,6 +56,38 @@ import type { * whole-operation dispatch its registration declares (#9719) — the * per-row dispatch alone can never deliver the shape it refuses. * + * ## Parent EDIT — one question, four limbs + * + * 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: + * + * - 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. + * + * ⛔ This is why 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 + * the org — including records whose own update refuses them. + * * System-context operations (engine self-writes, seeds, lifecycle sweeps) * bypass all three gates, as do context-less programmatic calls on bare * kernels (no principal to authorize — REST always carries a context). @@ -65,10 +97,20 @@ import type { * order is not load-bearing. */ -/** Minimal surface of plugin-sharing's service this gate consults. */ -export interface AttachmentSharingLike { - canEdit(object: string, recordId: string, context: Record): Promise; -} +/** + * 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. + */ +export type AttachmentSharingLike = Pick; + +/** + * The one member 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. + */ +export type AttachmentSecurityLike = Pick; const PACKAGE_ID = 'com.objectstack.service.storage'; const SYSTEM_CTX = { isSystem: true } as const; @@ -316,13 +358,77 @@ function callerContext(ctx: any): ExecutionContext { * refusal, because the i18n service is contributed by another plugin that may * start after this one. Absent, the built-in catalog still renders the * 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. */ export function installAttachmentAccessHooks( engine: AttachmentLifecycleEngine, getSharing: () => AttachmentSharingLike | null | undefined, logger: AttachmentLifecycleLogger, messageTranslator?: () => ValidationMessageTranslator | undefined, + getSecurity?: () => AttachmentSecurityLike | null | undefined, ): void { + /** + * May the caller EDIT the parent record `(object, recordId)`? The one + * question the attach rule, the row rule's parent-editor limb and the + * re-point's attach rule all ask — see "Parent EDIT" in the module header for + * the composition and why each outcome lands where it does. + * + * `recordId` is the value the limb holds (the degraded read probe keeps the + * shape each limb always queried with); the sharing and security services + * are handed its string form, as `canEdit` always was. + */ + const mayEditParent = async ( + ctx: any, + object: string, + recordId: unknown, + callerCtx: ExecutionContext, + limb: string, + ): Promise => { + const sharing = getSharing(); + 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 ctx.api.object(object).findOne({ where: { id: recordId } })); + } catch { + return false; + } + } + const verdict = await sharing.checkEdit(object, String(recordId), callerCtx); + if (verdict === 'allow') return true; + // `deny`, and anything that is not one of the three verdicts, refuses. + if (verdict !== 'abstain') return false; + + // Record sharing does not enforce on this parent. Whether that is + // permission is the master-detail check's question (ADR-0055): it answers + // `not_applicable` for a parent that derives nothing from a master. + let security: AttachmentSecurityLike | null | undefined; + try { + security = getSecurity?.(); + } catch { + security = undefined; + } + if (!security || typeof security.checkControlledByParentWrite !== 'function') { + // No master-detail check is composed in this kernel, and the parent's own + // update meets none either. ⛔ Not "the master was judged editable" — + // nothing measured it. + logger.debug?.( + `[storage] attachment access: record sharing abstains on ${object} and the security service composes no ` + + `master-detail write check — ${limb} admitted on the abstention, as the parent's own update would be`, + ); + return true; + } + // A rejection (a datasource fault, a context the write path refuses) is a + // refusal of the request, and propagates as it is. + const answer = await security.checkControlledByParentWrite(object, String(recordId), callerCtx); + return answer.outcome === 'allow' || answer.outcome === 'not_applicable'; + }; + /** Resolve every sys_attachment row a write matches, under SYSTEM context * — the caller may legitimately be unable to READ rows they are allowed to * touch (and the read-visibility middleware below must not narrow the @@ -387,9 +493,8 @@ export function installAttachmentAccessHooks( }; /** Uploader-or-parent-editor over every matched row. Parent-edit verdicts - * are memoized per (object, id) — a multi-row write usually targets one - * record. Degrades to caller-scoped parent READ visibility when no sharing - * service is present — still strictly tighter than no gate. */ + * ({@link mayEditParent}) are memoized per (object, id) — a multi-row write + * usually targets one record. */ const authorizeRows = async ( ctx: any, rows: Array>, @@ -397,7 +502,6 @@ export function installAttachmentAccessHooks( denyCode: string, ): Promise => { const userId = ctx.session.userId as string | undefined; - const sharing = getSharing(); const callerCtx = callerContext(ctx); const canEditCache = new Map(); /** Parent READ verdicts, asked only for a row about to be refused. */ @@ -410,20 +514,7 @@ export function installAttachmentAccessHooks( const cacheKey = `${parentObject}\u0000${parentId}`; let allowed = canEditCache.get(cacheKey); if (allowed === undefined) { - if (sharing && typeof sharing.canEdit === 'function') { - allowed = await sharing.canEdit(parentObject, parentId, callerCtx); - } else { - // Degraded mode (no sharing service): fall back to caller-scoped - // parent READ visibility — still strictly tighter than no gate. - try { - allowed = !!(await ctx.api.object(parentObject).findOne({ where: { id: parentId } })); - } catch { - allowed = false; - } - logger.debug?.( - `[storage] attachment access: sharing service absent — ${verb} gated on parent read visibility`, - ); - } + allowed = await mayEditParent(ctx, parentObject, parentId, callerCtx, verb); canEditCache.set(cacheKey, allowed); } if (!allowed) { @@ -477,24 +568,10 @@ export function installAttachmentAccessHooks( if (parentId === undefined || parentId === null || parentId === '') return; // Salesforce parity (#2970 item 3): attaching to a record requires EDIT - // access to it, not merely read. Public-model parents return canEdit - // true for any member (so the common case is unchanged); private, - // owner-scoped parents require the caller to own/edit them. Degrades to - // caller-scoped READ visibility when no sharing service is present. - const sharing = getSharing(); - let allowed = false; - if (sharing && typeof sharing.canEdit === 'function') { - allowed = await sharing.canEdit(parentObject, String(parentId), callerContext(ctx)); - } else { - try { - allowed = !!(await ctx.api.object(parentObject).findOne({ where: { id: parentId } })); - } catch { - allowed = false; - } - logger.debug?.( - '[storage] attachment access: sharing service absent — attach gated on parent read visibility', - ); - } + // access to it, not merely read — answered as "Parent EDIT" in the module + // header sets out, so a `controlled_by_parent` parent is judged through + // its master, as its own update is. + const allowed = await mayEditParent(ctx, parentObject, parentId, callerContext(ctx), 'attach'); if (!allowed) { forbid( 'ATTACHMENT_PARENT_ACCESS', @@ -535,8 +612,7 @@ export function installAttachmentAccessHooks( // is closed by the row rule itself, not by stamping.) // Re-pointing an attachment is an INSERT into the new parent's files - // panel: the NEW parent must satisfy the attach rule (EDIT via the - // sharing service, degrading to caller-scoped read visibility — the + // panel: the NEW parent must satisfy the attach rule (parent EDIT, the // beforeInsert gate above), or an authorized edit of your own // attachment would be a way to plant files on records you cannot see. // Mirrors the comment kit's thread re-point rule (#4630). @@ -544,7 +620,6 @@ export function installAttachmentAccessHooks( if (!data || typeof data !== 'object') return; if (data.parent_object === undefined && data.parent_id === undefined) return; - const sharing = getSharing(); /** Attach-rule verdicts memoized per effective (object, id) target. */ const canAttachCache = new Map(); for (const row of rows) { @@ -570,18 +645,7 @@ export function installAttachmentAccessHooks( const cacheKey = `${nextObject}\u0000${String(nextId)}`; let allowed = canAttachCache.get(cacheKey); if (allowed === undefined) { - if (sharing && typeof sharing.canEdit === 'function') { - allowed = await sharing.canEdit(nextObject, String(nextId), callerContext(ctx)); - } else { - try { - allowed = !!(await ctx.api.object(nextObject).findOne({ where: { id: nextId } })); - } catch { - allowed = false; - } - logger.debug?.( - '[storage] attachment access: sharing service absent — re-point gated on parent read visibility', - ); - } + allowed = await mayEditParent(ctx, nextObject, nextId, callerContext(ctx), 're-point'); canAttachCache.set(cacheKey, allowed); } if (!allowed) { diff --git a/packages/services/service-storage/src/attachment-delete-floor-alternate.ts b/packages/services/service-storage/src/attachment-delete-floor-alternate.ts index 455e5628c18..a352d9d742e 100644 --- a/packages/services/service-storage/src/attachment-delete-floor-alternate.ts +++ b/packages/services/service-storage/src/attachment-delete-floor-alternate.ts @@ -31,7 +31,9 @@ import type { AttachmentLifecycleLogger } from './attachment-lifecycle.js'; * - `id != null` is every row of `sys_attachment`, said plainly. The * parent-editor rule is not expressible as a row predicate (the authority * lives on the PARENT record, resolved through the sharing service's - * `canEdit`, and RLS has no join), so the policy does not re-implement the + * `checkEdit` and, on a `controlled_by_parent` parent, the security + * service's master-detail write check, and RLS has no join), so the + * policy does not re-implement the * rule — it stops the floor answering for this object on this limb, and the * gate keeps deciding. A non-uploader who cannot edit the parent is still * refused, by the gate, with `ATTACHMENT_DELETE_DENIED`. diff --git a/packages/services/service-storage/src/index.ts b/packages/services/service-storage/src/index.ts index d5a44cee59d..629eb88c1ea 100644 --- a/packages/services/service-storage/src/index.ts +++ b/packages/services/service-storage/src/index.ts @@ -93,7 +93,7 @@ export type { BackfillReport, } from './backfill-file-references.js'; export { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js'; -export type { AttachmentSharingLike } from './attachment-access-hooks.js'; +export type { AttachmentSharingLike, AttachmentSecurityLike } from './attachment-access-hooks.js'; export { runFilesToReferencesMigration } from './files-to-references-migration.js'; export type { FilesToReferencesEngine, diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index b85d1c3f6fe..0246a5bfe45 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -17,6 +17,7 @@ import type { IHttpServer, IDataEngine, II18nService, + ISecurityService, IStorageService, IFileAccessDelegate, } from '@objectstack/spec/contracts'; @@ -398,6 +399,17 @@ export class StorageServicePlugin implements Plugin { if (typeof t !== 'function') return undefined; return (key: string, loc: string, params?: Record) => t.call(i18n, key, loc, params); }, + // [ADR-0055] The security service, for its master-detail write check: + // a `controlled_by_parent` parent is judged through its master, as its + // own update is. Resolved per check (the security plugin may start + // after this one); a kernel without it composes no master check. + () => { + try { + return ctx.getService('security'); + } catch { + return null; + } + }, ); // [#21729] The gate's parent-editor DELETE limb, made reachable: the // alternate match that stops the platform's `created_by` delete floor From f9d081f2de87db17e2cf495c8cdc8c6f2162b64c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:44:16 +0000 Subject: [PATCH 5/9] fix(plugin-audit): the comment moderation gate judges a controlled_by_parent parent through its master Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- .../plugins/plugin-audit/src/audit-plugin.ts | 15 +- .../src/comment-access-hooks.test.ts | 137 +++++++++++++++++- .../plugin-audit/src/comment-access-hooks.ts | 97 ++++++++++--- packages/plugins/plugin-audit/src/index.ts | 1 + 4 files changed, 225 insertions(+), 25 deletions(-) diff --git a/packages/plugins/plugin-audit/src/audit-plugin.ts b/packages/plugins/plugin-audit/src/audit-plugin.ts index 0288822c7df..3b41f97049a 100644 --- a/packages/plugins/plugin-audit/src/audit-plugin.ts +++ b/packages/plugins/plugin-audit/src/audit-plugin.ts @@ -2,7 +2,7 @@ import type { Plugin, PluginContext } from '@objectstack/core'; import { resolveLocalizationContext } from '@objectstack/core'; -import type { IDataEngine, II18nService, ISharingService } from '@objectstack/spec/contracts'; +import type { IDataEngine, II18nService, ISecurityService, ISharingService } from '@objectstack/spec/contracts'; import { SysAuditLog, SysActivity, SysComment } from './objects/index.js'; // `sys_notification` was parked here "until that [ADR-0030] migration lands". // It has landed, so the contribution moved to @objectstack/service-messaging — @@ -355,7 +355,7 @@ export class AuditPlugin implements Plugin { () => { try { // Typed with the slot's contract (#4251): the gate consults - // `canEdit` only, but it consults the REAL interface. + // `checkEdit` only, but it consults the REAL interface. return ctx.getService('sharing'); } catch { return null; @@ -372,6 +372,17 @@ export class AuditPlugin implements Plugin { if (typeof t !== 'function') return undefined; return (key: string, loc: string, params?: Record) => t.call(i18n, key, loc, params); }, + // [ADR-0055] The security service, for its master-detail write check: + // a `controlled_by_parent` parent is judged through its master, as its + // own update is. Resolved per check (the security plugin may start + // after this one); a kernel without it composes no master check. + () => { + try { + return ctx.getService('security'); + } catch { + return null; + } + }, ); if (typeof (engine as any).registerMiddleware === 'function') { installCommentReadVisibility(engine as any, ctx.logger); diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts index db6824aa170..d94369c6e6b 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts @@ -7,6 +7,7 @@ import { installCommentAccessHooks, parseCommentThreadId, type CommentAccessEngine, + type CommentSecurityLike, type CommentSharingLike, } from './comment-access-hooks.js'; @@ -35,10 +36,33 @@ function readableParentFind(readable: string[], reads: Array<{ object: string; o }; } +/** + * A two-state sharing double, as the suites below were written against: `true` + * (may edit) or `false`. The gate reads the TRI-STATE `checkEdit` since the + * `controlled_by_parent` fix, and these doubles stand for its two verdicts + * whose meaning did not move — `true` is `allow` (admits), `false` is `deny` + * (refuses). `abstain` never comes out of one; the suite that pins it (end of + * file) hands the gate a tri-state double directly. + */ +type TwoStateSharingDouble = { + canEdit: (object: string, recordId: string, context: any) => Promise; +}; + +function asTriState(sharing: CommentSharingLike | TwoStateSharingDouble | null | undefined) { + if (!sharing || !('canEdit' in sharing)) return sharing; + const { canEdit } = sharing; + return { + checkEdit: async (object: string, recordId: string, context: any) => + (await canEdit(object, recordId, context)) ? ('allow' as const) : ('deny' as const), + } satisfies CommentSharingLike; +} + /** Capture the three registered hooks so tests can drive them directly. */ function install(opts: { comments?: Array>; - sharing?: CommentSharingLike | null; + sharing?: CommentSharingLike | TwoStateSharingDouble | null; + /** The security service, for its master-detail write check (#22455). */ + security?: CommentSecurityLike | null; /** Parents the CALLER can read through the engine, keyed `object/id`. */ readable?: string[]; messageTranslator?: () => ((key: string, locale: string, params?: Record) => string) | undefined; @@ -62,7 +86,8 @@ function install(opts: { Object.entries(options?.where ?? {}).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return r[k] === v; }), ) ?? null, }; - installCommentAccessHooks(engine, () => opts.sharing, silentLogger(), opts.messageTranslator); + const sharing = asTriState(opts.sharing); + installCommentAccessHooks(engine, () => sharing, silentLogger(), opts.messageTranslator, () => opts.security); return { beforeInsert: hooks.get('beforeInsert')!, beforeUpdate: hooks.get('beforeUpdate')!, @@ -704,7 +729,7 @@ function makeWiredDriver() { async function bootWired(opts: { comments?: Array>; opportunities?: Array>; - sharing?: CommentSharingLike | null; + sharing?: CommentSharingLike | TwoStateSharingDouble | null; } = {}) { const ql = new ObjectQL(); const driver = makeWiredDriver(); @@ -713,7 +738,8 @@ async function bootWired(opts: { ql.registry.registerObject(sysCommentObject as any, 'app:test'); ql.registry.registerObject(crmOpportunityObject as any, 'app:test'); // `engine as any` mirrors the production wiring in audit-plugin.ts. - installCommentAccessHooks(ql as any, () => opts.sharing ?? null, silentLogger()); + const sharing = asTriState(opts.sharing) ?? null; + installCommentAccessHooks(ql as any, () => sharing, silentLogger()); if (!driver.stores.get('sys_comment')) driver.stores.set('sys_comment', new Map()); for (const r of opts.comments ?? []) driver.stores.get('sys_comment')!.set(String(r.id), { ...r }); for (const r of opts.opportunities ?? []) { @@ -1261,3 +1287,106 @@ describe('a refusal on a parent the caller cannot read names nothing', () => { expect(seen.remaining()).toBe(1); }); }); + +// ── [ADR-0055] a controlled_by_parent parent is judged through its master ── +// +// `checkEdit` ABSTAINS on every `controlled_by_parent` parent (plugin-sharing +// maps the model to `public`), and the moderation limb used to read `canEdit`, +// which folds that abstention into `true`: any member holding the `sys_comment` +// delete bit removed other people's comments from every master-detail child +// record. These pin the composition the limb now runs — the attachment kit's. +describe('[ADR-0055] moderation on a controlled_by_parent parent — checkEdit, then the master-detail write check', () => { + type Verdict = 'allow' | 'abstain' | 'deny'; + type Outcome = Awaited>>; + + const triState = (verdict: Verdict) => { + const checkEdit = vi.fn(async (_o: string, _r: string, _c: any) => verdict); + return { checkEdit, sharing: { checkEdit } satisfies CommentSharingLike }; + }; + const masterCheck = (answer: Outcome | Error) => { + const checkControlledByParentWrite = vi.fn(async (_o: string, _r: string, _c?: any): Promise => { + if (answer instanceof Error) throw answer; + return answer; + }); + return { checkControlledByParentWrite, security: { checkControlledByParentWrite } satisfies CommentSecurityLike }; + }; + const ownersComment = { id: 'c1', thread_id: 'cbp_contract:k1', author_id: 'owner', body: 'executed by legal' }; + const deleteByMember = () => writeCtx('beforeDelete', { id: 'c1' }, { userId: 'member' }); + + // The truth table, on the delete limb. `null` for the member column = never asked. + const TABLE: Array<{ verdict: Verdict; member: Outcome | null; admits: boolean }> = [ + { verdict: 'allow', member: null, admits: true }, + { verdict: 'deny', member: null, admits: false }, + { verdict: 'abstain', member: { outcome: 'allow' }, admits: true }, + { verdict: 'abstain', member: { outcome: 'not_applicable' }, admits: true }, + { verdict: 'abstain', member: { outcome: 'deny', leg: 'record_sharing' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'deny', leg: 'master_chain' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'unresolvable', reason: 'master_detail_relation_missing' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'unresolvable', reason: 'record_not_found' }, admits: false }, + { verdict: 'abstain', member: { outcome: 'unresolvable', reason: 'master_reference_missing' }, admits: false }, + ]; + for (const row of TABLE) { + const label = `checkEdit ${row.verdict}${row.member ? ` × member ${JSON.stringify(row.member)}` : ''}`; + it(`delete of another user's comment: ${label} → ${row.admits ? 'admits' : 'refuses 403 RECORD_NOT_ACCESSIBLE'}`, async () => { + const { sharing } = triState(row.verdict); + const { security, checkControlledByParentWrite } = masterCheck(row.member ?? { outcome: 'deny', leg: 'record_sharing' }); + const { beforeDelete } = install({ comments: [ownersComment], sharing, security, readable: ['cbp_contract/k1'] }); + const run = beforeDelete(deleteByMember()); + if (row.admits) await expect(run).resolves.toBeUndefined(); + else await expect(run).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, object: 'cbp_contract' }); + expect(checkControlledByParentWrite).toHaveBeenCalledTimes(row.member ? 1 : 0); + }); + } + + it('the member is asked about the parent the thread names, with the caller envelope the sharing gate gets', async () => { + const { sharing, checkEdit } = triState('abstain'); + const { security, checkControlledByParentWrite } = masterCheck({ outcome: 'allow' }); + const { beforeDelete } = install({ comments: [ownersComment], sharing, security }); + await beforeDelete(deleteByMember()); + expect(checkControlledByParentWrite).toHaveBeenCalledWith('cbp_contract', 'k1', checkEdit.mock.calls[0]![2]); + expect(checkControlledByParentWrite.mock.calls[0]![2]).toMatchObject({ userId: 'member' }); + }); + + it('the author deletes their own comment without the parent being asked', async () => { + const { sharing, checkEdit } = triState('abstain'); + const { security, checkControlledByParentWrite } = masterCheck({ outcome: 'deny', leg: 'record_sharing' }); + const { beforeDelete } = install({ comments: [{ ...ownersComment, author_id: 'member' }], sharing, security }); + await expect(beforeDelete(deleteByMember())).resolves.toBeUndefined(); + expect(checkEdit).not.toHaveBeenCalled(); + expect(checkControlledByParentWrite).not.toHaveBeenCalled(); + }); + + it('a caller who cannot read the parent gets the not-visible refusal, as on any other parent (#21755)', async () => { + const { sharing } = triState('abstain'); + const { security } = masterCheck({ outcome: 'deny', leg: 'record_sharing' }); + const { beforeDelete } = install({ comments: [ownersComment], sharing, security, readable: [] }); + await expect(beforeDelete(deleteByMember())).rejects.toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + }); + + it('the update limb runs the same rule (the same function)', async () => { + const { sharing } = triState('abstain'); + const { security } = masterCheck({ outcome: 'deny', leg: 'record_sharing' }); + const { beforeUpdate } = install({ comments: [ownersComment], sharing, security, readable: ['cbp_contract/k1'] }); + await expect( + beforeUpdate(writeCtx('beforeUpdate', { id: 'c1', data: { body: 'rewritten' } }, { userId: 'member' })), + ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); + }); + + it('a store fault from the master check propagates unchanged — its 503 is never an admit or a 403', async () => { + const outage = Object.assign(new Error('datasource unavailable'), { code: 'ERR_DATASOURCE_UNAVAILABLE', status: 503 }); + const { sharing } = triState('abstain'); + const { security } = masterCheck(outage); + const { beforeDelete } = install({ comments: [ownersComment], sharing, security, readable: ['cbp_contract/k1'] }); + await expect(beforeDelete(deleteByMember())).rejects.toBe(outage); + }); + + it("a kernel without the member keeps the abstention's admit — no security service, or one that does not serve it", async () => { + const { sharing } = triState('abstain'); + await expect( + install({ comments: [ownersComment], sharing, security: null }).beforeDelete(deleteByMember()), + ).resolves.toBeUndefined(); + await expect( + install({ comments: [ownersComment], sharing, security: {} as CommentSecurityLike }).beforeDelete(deleteByMember()), + ).resolves.toBeUndefined(); + }); +}); diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index b9af9e19816..7718798ea3e 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -24,9 +24,9 @@ * OWD/sharing, RLS and object-level CRUD all apply. `author_id` is * server-stamped from the session; a client-supplied value never wins. * * `beforeUpdate` / `beforeDelete`: the caller must be the comment's - * AUTHOR, or hold EDIT on the parent record (sharing's `canEdit`; - * public-model parents are editable by design) — the attachment kit's - * uploader-or-parent-editor rule. A multi-row write requires EVERY + * AUTHOR, or hold EDIT on the parent record (see "Parent EDIT" below) — + * the attachment kit's uploader-or-parent-editor rule. A multi-row write + * requires EVERY * matched row to pass, and an update that re-points `thread_id` must * additionally satisfy the insert rule on the NEW thread. A write * carrying NEITHER an id NOR a `where` is refused outright rather than @@ -45,10 +45,27 @@ * * Why read gating requires EDIT on write but only READ on insert: posting a * comment is collaboration — a user who may READ a record may discuss it (the - * attachment kit's stricter `canEdit` on insert exists because attaching a + * attachment kit's stricter parent-EDIT rule on insert exists because attaching a * FILE mutates the record's content surface). Rewriting or removing someone * else's words is moderation, hence the tighter author-or-parent-editor rule. * + * ## 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. ⛔ 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 * `sys_comment` itself) is REFUSED on write and EXCLUDED on read: an @@ -67,7 +84,7 @@ import { withoutOperationPrivateKeys } from '@objectstack/core'; import type { StandardErrorCode } from '@objectstack/spec/api'; -import type { ISharingService } from '@objectstack/spec/contracts'; +import type { ISecurityService, ISharingService } from '@objectstack/spec/contracts'; import type { ExecutionContext } from '@objectstack/spec/kernel'; import { renderOperationMessage, type ValidationMessageTranslator } from '@objectstack/spec/system'; @@ -111,12 +128,20 @@ export interface CommentReadMiddlewareCtx { } /** - * The single method of plugin-sharing's contract this gate consults. 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). + * 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 + * 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). + */ +export type CommentSharingLike = Pick; + +/** + * The single member 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. */ -export type CommentSharingLike = Pick; +export type CommentSecurityLike = Pick; export interface CommentAccessLogger { info(msg: string, meta?: unknown): void; @@ -369,25 +394,59 @@ async function callerCanRead(ctx: any, target: CommentThreadTarget): Promise CommentSharingLike | null | undefined, logger: CommentAccessLogger, messageTranslator?: () => ValidationMessageTranslator | undefined, + getSecurity?: () => CommentSecurityLike | null | undefined, ): void { - /** May the caller EDIT the parent record behind `target`? Sharing's - * `canEdit` when the service is present, else caller-scoped parent read - * visibility (degraded mode). */ + /** May the caller EDIT the parent record behind `target`? The composition + * "Parent EDIT" in the module header sets out; caller-scoped parent read + * visibility when no sharing service is present (degraded mode). */ const canEditParent = async (ctx: any, target: CommentThreadTarget, verb: string): Promise => { const sharing = getSharing(); - if (sharing && typeof sharing.canEdit === 'function') { - return sharing.canEdit(target.object, target.recordId, callerContext(ctx)); + if (!sharing || typeof sharing.checkEdit !== 'function') { + logger.debug?.( + `[audit] comment access: sharing service absent — ${verb} gated on parent read visibility`, + ); + return callerCanRead(ctx, target); } - logger.debug?.( - `[audit] comment access: sharing service absent — ${verb} gated on parent read visibility`, - ); - return callerCanRead(ctx, target); + const callerCtx = callerContext(ctx); + const verdict = await sharing.checkEdit(target.object, target.recordId, callerCtx); + if (verdict === 'allow') return true; + // `deny`, and anything that is not one of the three verdicts, refuses. + if (verdict !== 'abstain') return false; + + // Record sharing does not enforce on this parent. Whether that is + // permission is the master-detail check's question (ADR-0055): it answers + // `not_applicable` for a parent that derives nothing from a master. + let security: CommentSecurityLike | null | undefined; + try { + security = getSecurity?.(); + } catch { + security = undefined; + } + if (!security || typeof security.checkControlledByParentWrite !== 'function') { + // No master-detail check is composed in this kernel, and the parent's own + // update meets none either. ⛔ Not "the master was judged editable" — + // nothing measured it. + logger.debug?.( + `[audit] comment access: record sharing abstains on ${target.object} and the security service composes no ` + + `master-detail write check — ${verb} admitted on the abstention, as the parent's own update would be`, + ); + return true; + } + // A rejection (a datasource fault, a context the write path refuses) is a + // refusal of the request, and propagates as it is. + const answer = await security.checkControlledByParentWrite(target.object, target.recordId, callerCtx); + return answer.outcome === 'allow' || answer.outcome === 'not_applicable'; }; // ── Create: parent-record READ access + author_id stamping ────────── diff --git a/packages/plugins/plugin-audit/src/index.ts b/packages/plugins/plugin-audit/src/index.ts index c4e3ea7ca16..b7108e8d8bf 100644 --- a/packages/plugins/plugin-audit/src/index.ts +++ b/packages/plugins/plugin-audit/src/index.ts @@ -56,6 +56,7 @@ export type { CommentAccessEngine, CommentAccessLogger, CommentReadMiddlewareCtx, + CommentSecurityLike, CommentSharingLike, CommentThreadTarget, } from './comment-access-hooks.js'; From c08e917dbb1eb8f86248b4df30091a5874feae27 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:48:11 +0000 Subject: [PATCH 6/9] test(dogfood): the real stack names the master's row-level security leg for a floor-bound member Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- ...bp-parent-attachment-comment-gates.dogfood.test.ts | 11 ++++++++++- .../dogfood/test/fixtures/cbp-parent-gates-fixture.ts | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts b/packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts index 06fabbfe0da..80f8b127b80 100644 --- a/packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts +++ b/packages/qa/dogfood/test/cbp-parent-attachment-comment-gates.dogfood.test.ts @@ -206,7 +206,16 @@ describe('controlled_by_parent parent gates: sys_attachment and sys_comment judg }); it('PATCH parity: the security service answers the child as its PATCH does, for the member and for the owner', async () => { - expect(await memberAnswer(memberTok, 'cpg_contract', contractId)).toEqual({ outcome: 'deny', leg: 'record_sharing' }); + // The member's PATCH of the child is refused by the master check, on the + // leg the security service names: the platform's ownership floor on the + // master (`created_by`, org_member) stands, because record sharing gives + // the member no basis to lift it, so the master's write row-level security + // excludes the row. + const patchChild = await answer(await stack.apiAs(memberTok, 'PATCH', `/data/cpg_contract/${contractId}`, { name: 'x' })); + expect(patchChild.status, `member PATCH of the child: ${patchChild.body}`).toBe(403); + expect(patchChild.body).toContain('requires edit access to its master record'); + expect(patchChild.body).toContain('(row-level security)'); + expect(await memberAnswer(memberTok, 'cpg_contract', contractId)).toEqual({ outcome: 'deny', leg: 'row_level_security' }); expect(await memberAnswer(adminTok, 'cpg_contract', contractId)).toEqual({ outcome: 'allow' }); expect(await memberAnswer(memberTok, 'cpg_contract', memberContractId)).toEqual({ outcome: 'allow' }); expect(await memberAnswer(memberTok, 'cpg_board', boardId)).toEqual({ outcome: 'not_applicable' }); diff --git a/packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts b/packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts index 5ae9fc737c3..ed9950a7419 100644 --- a/packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/cbp-parent-gates-fixture.ts @@ -11,7 +11,7 @@ // it. The objects below put that check on both sides of the gates: // // cpg_account — the MASTER. `public_read` with an `owner_id`: every member -// READS it, only its owner EDITS it (record sharing). +// READS it, only its owner EDITS it. // cpg_contract — a `controlled_by_parent` DETAIL of `cpg_account`, with files // and feeds on. A member reads it (its master is readable) and // may edit it only where they may edit its master. From 182f11fb60596ca9b6e7b3c3c91e4698ec09420f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:56:39 +0000 Subject: [PATCH 7/9] chore(changeset): the controlled_by_parent parent gates and the served master-detail write check Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- .../22455-cbp-parent-gates-judge-master.md | 22 +++++++++++++++++++ ...urity-serves-controlled-by-parent-write.md | 12 ++++++++++ 2 files changed, 34 insertions(+) create mode 100644 .changeset/22455-cbp-parent-gates-judge-master.md create mode 100644 .changeset/22455-security-serves-controlled-by-parent-write.md diff --git a/.changeset/22455-cbp-parent-gates-judge-master.md b/.changeset/22455-cbp-parent-gates-judge-master.md new file mode 100644 index 00000000000..e6db95bbf0c --- /dev/null +++ b/.changeset/22455-cbp-parent-gates-judge-master.md @@ -0,0 +1,22 @@ +--- +'@objectstack/service-storage': minor +'@objectstack/plugin-audit': minor +--- + +fix(service-storage,plugin-audit)!: the `sys_attachment` and `sys_comment` parent gates judge a `controlled_by_parent` parent through its master, so a member can no longer attach files to, or delete other people's files or comments from, a child record they may not edit + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes. It follows ADR-0055. + +**What stops being accepted.** + +- A `controlled_by_parent` record takes its access from its MASTER. Record sharing does not enforce on it, so the sharing service's edit verdict abstains on every such record, and both gates used to take that abstention as permission. A member holding the `sys_attachment` create or delete bit, or the `sys_comment` delete bit, could therefore attach files to, and delete other people's files and comments from, every master-detail child record of the organization. That included records whose own `PATCH` refuses the member, and records the member cannot read. +- Both gates now ask the sharing service's tri-state `checkEdit`. `allow` admits and `deny` refuses, as before. On an abstention they ask the security service's master-detail write check (`ISecurityService.checkControlledByParentWrite`), which is the answer a by-id update of the parent gets. Its `allow` and `not_applicable` admit. Its `deny` refuses, and so does an `unresolvable` outcome: a broken master-detail declaration, a record that does not exist, or an empty master reference. A datasource fault propagates with its `503`. +- The refusals keep today's envelopes. An attach or a re-point answers `403 ATTACHMENT_PARENT_ACCESS`. A delete of another user's file answers `403 ATTACHMENT_DELETE_DENIED`, and an update of it answers `403 RECORD_NOT_ACCESSIBLE`. A delete or update of another user's comment answers `403 RECORD_NOT_ACCESSIBLE`. A caller who cannot read the parent still gets the not-visible `403 PERMISSION_DENIED`. + +**What is unchanged.** An uploader still deletes their own file, and an author still deletes their own comment. Posting a comment still needs only read access to the record. A parent that is not `controlled_by_parent` is judged exactly as before, and a `public_read_write` parent still admits every member. A kernel whose security service does not serve the master-detail write check composes no master check, and there the gates admit on the abstention as before, which is also what the parent's own update gets. + +**What changes for you.** A member who was attaching or moderating on a child record through this gap now gets the refusal above. To give them that access, grant them edit access on the MASTER record: ownership of it, an `edit`-level record share, the `modifyAllRecords` bypass on the master object, or an app-authored `update` row-level security policy on the master that admits the row. Then their `PATCH` of the child and their attachment and moderation rights on it agree. The port types `AttachmentSharingLike` and `CommentSharingLike` now pick `checkEdit` rather than `canEdit`. The registered sharing service serves both, so a caller passing it is unaffected. `installAttachmentAccessHooks` and `installCommentAccessHooks` take an optional trailing resolver for the security service (new port types `AttachmentSecurityLike` and `CommentSecurityLike`). The two plugins pass it themselves. diff --git a/.changeset/22455-security-serves-controlled-by-parent-write.md b/.changeset/22455-security-serves-controlled-by-parent-write.md new file mode 100644 index 00000000000..cd18a4d27f3 --- /dev/null +++ b/.changeset/22455-security-serves-controlled-by-parent-write.md @@ -0,0 +1,12 @@ +--- +'@objectstack/plugin-security': minor +--- + +The registered `security` service serves `checkControlledByParentWrite(object, recordId, context)`, the ADR-0055 master-detail write check that `ISecurityService` declares as optional. + +- **What it answers.** It answers what a by-id update of the record gets from the master-detail write check. It runs the write path's own composition and never re-derives it: the engine middleware's context prologue (a principal-less context, permission sets that cannot be resolved and a missing on-behalf-of delegator are refused with the same `403 PERMISSION_DENIED`), and then the check for the principal's permission sets and, on a delegated context, for the delegator's (ADR-0090 D10). The first refusal is the answer. A refusal resolves `deny` with the leg it came from, the three non-verdicts resolve `unresolvable` with their reason, and an object that is not `controlled_by_parent` resolves `not_applicable`. A system context resolves `allow`. A store fault rejects with the engine's own error, so a datasource outage keeps its `503`. +- **Where it differs from the write path's guard.** A principal whose permission sets resolve to none is judged over that empty list, so it answers `deny` on `object_permission` as the contract declares. A principal with positions or sets and no user id is also judged, where the write path skips the check. It answers `deny` when it holds no `update` grant on the master, and its own update is refused by the object-level gate anyway. +- **Who asks it.** The `sys_attachment` parent gate in `@objectstack/service-storage` and the `sys_comment` moderation gate in `@objectstack/plugin-audit` ask it, so they judge a `controlled_by_parent` parent through its master. +- The boot line that announces the `security` service now lists the members the registered service actually serves, read from the service object. The hand-kept list it replaced had fallen behind by five members. + +Nothing an author writes changes, and no by-id write is judged differently. The middleware's context prologue moved into one method that the middleware and the member both call. Its refusals, their order and their wording are unchanged. From b7d78930f01d8f9aa93200c07d8ba82b56cb8c9f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 16:20:38 +0000 Subject: [PATCH 8/9] chore(engine-double-contract): record the master-detail write member suite's pinned findOne double Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- scripts/engine-double-contract.pinned.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 7a14a635986..55e84d66a21 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -3146,6 +3146,11 @@ "verb": "findOne", "pinned": 1 }, + { + "file": "packages/plugins/plugin-security/src/controlled-by-parent-write-member.test.ts", + "verb": "findOne", + "pinned": 1 + }, { "file": "packages/plugins/plugin-security/src/declared-permission-reload-projection.test.ts", "verb": "findOne", From 15d05d2b697153d2e126285e037a9d49885fcd44 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 16:55:13 +0000 Subject: [PATCH 9/9] docs(permissions): the system-context census row for the served master-detail write check's system exit Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude --- content/docs/permissions/system-context.mdx | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index b67fef25a87..ea43e755307 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -10,7 +10,7 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a service self-write, a migration. This page is **the authority** for what that flag actually does. It exists -because the flag is not one concept: it is a single boolean read at **121 +because the flag is not one concept: it is a single boolean read at **122 distinct sites across 20 packages**, and knowing three of those behaviours gives no hint that the other hundred-and-four exist. Every documented app-side bug traced to `isSystem` had the same shape — the metadata was complete and correct, @@ -106,6 +106,7 @@ that silently does not happen. | 7 | Write bypass = `true`, effective write scope = `org` | plugin-security | Get: widest write scope without holding any capability | `packages/plugins/plugin-security/src/security-plugin.ts#start` | | 8 | Metadata-plane schema masking exempt (ADR-0106 D4) | metadata-core | Get: unmasked object schema. Note: the exemption is a **caller** property — it short-circuits before the security service is consulted | `packages/metadata-core/src/object-schema-fls.ts#isObjectSchemaMaskExempt` | | 9 | `explain()` may target a principal other than the caller | plugin-security | Get: no `manage_users` / delegated-admin check | `packages/plugins/plugin-security/src/security-plugin.ts#explainAccessForCaller` | +| 9b | The master-detail write check the `security` service serves (`checkControlledByParentWrite`, ADR-0055) answers `allow` | plugin-security | Get: `allow` for any record, with no master judged — the write path's own first exit, so a gate composing it admits exactly what the record's own system-context update would. Lose: nothing a caller reaches today: its two consumers, the `sys_attachment` and `sys_comment` parent gates, return before they ask for a system session | `packages/plugins/plugin-security/src/security-plugin.ts#checkControlledByParentWrite` | | 10 | Anonymous-deny treats the caller as authenticated | core | Get: passes the 401 seam with no `userId` | `packages/core/src/security/anonymous-deny.ts#shouldDenyAnonymous` | | 11 | Permission-set projection middleware skipped | plugin-security | Lose: projection of permission-set-derived columns | `packages/plugins/plugin-security/src/permission-set-projection.ts#createPermissionSetWriteThrough` | | 11b | Position write-through skipped — a `sys_position` write is not mirrored into the environment ledger | plugin-security | Get: the seeders and the package door write rows for definitions that already have their home, untouched. Lose: a system-written row has no environment definition, so the security catalog read does not resolve it until a data-door edit, or the one-time row-only position backfill, gives it one | `packages/plugins/plugin-security/src/position-write-through.ts#createPositionWriteThrough` | @@ -142,7 +143,7 @@ that silently does not happen. ### 3. Sharing (`plugin-sharing`) -The largest single consumer — **17 of the 121 sites**. +The largest single consumer — **17 of the 122 sites**. | # | Behaviour when `isSystem` | What you get / what you lose | Anchor | |:--|:---|:---|:---| @@ -285,7 +286,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are independent decisions, and a seed loader plausibly wants the first two but not the third. The concept is nevertheless **staying as one boolean**: -- **Shipped semantics.** `isSystem` is a published contract with 121 read sites +- **Shipped semantics.** `isSystem` is a published contract with 122 read sites in 20 packages. Splitting it is a breaking contract change across all of them. (The ruling was taken when the census read 80 sites in 18 packages; the count has grown, which strengthens rather than weakens the argument.) @@ -359,16 +360,16 @@ still holds equal to the census on every pull request: | Appearances of the bare identifier `isSystem` in non-test sources | 813 | — | | — parsed as a declaration | 28 | ✅ | | — parsed as an object-literal / type key (producers and option objects) | 310 | — | -| — parsed as a property **read** | 127 | ✅ | +| — parsed as a property **read** | 128 | ✅ | | — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ | | — the remainder: text inside comments and string literals | 358 | — | | Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ | -| Of those reads: reads of `ExecutionContext.isSystem` | **121** | ✅ | -| — behaviour-bearing (rows 1–61 above) | 118 | ✅ | +| Of those reads: reads of `ExecutionContext.isSystem` | **122** | ✅ | +| — behaviour-bearing (rows 1–61 above) | 119 | ✅ | | — carry the flag onward only (rows 62–64 above) | 3 | ✅ | | Packages containing at least one elevation read | **20** | ✅ | | Files containing at least one elevation read | 57 | ✅ | -| — the distinct symbols those reads live in — what this page anchors | 103 | ✅ | +| — the distinct symbols those reads live in — what this page anchors | 104 | ✅ | | — of those files, the ones holding more than one read in one symbol | 8 | ✅ | The six rows marked — are a **dated decomposition, not a live claim**: they were