fix(security): controlled_by_parent 折入主档的归属与共享授权 —— 声明的收窄真正被兑现 (#5386)

.changeset/controlled-by-parent-master-sharing.md

@@ -0,0 +1,41 @@
+---
+"@objectstack/plugin-security": patch
+---
+
+fix(security): `controlled_by_parent` 现在真的跟随主档访问 —— 折入主档的归属与共享授权 (#5386)
+
+**这是一次安全收紧。** 升级后,此前被越权看到 / 写到的明细行会读不到、写不了 —— 那正是
+声明本来就要求的边界。
+
+ADR-0055 的 `controlled_by_parent` 对作者的承诺是「子记录跟随父记录的访问」。实现只兑现了
+一半:派生用的主档 id 集来自 `computeRlsFilter(master, 'find')`,即只有 Layer 0(租户)与
+Layer 1(`rowLevelSecurity` 策略)。归属(owner scope)与 `sys_record_share` 授权由**另一个
+插件** `plugin-sharing` 的 `buildReadFilter` 贡献,而它对「有效共享模型不是 `private`」的对象
+返回 `null` —— `controlled_by_parent` 在那边恰好映射为 `public`。于是记录级访问的两半在派生
+对象上从未相遇。
+
+后果比文档里那句「sharing grants 未折入」读起来严重得多:
+
+- 主档上**没有写任何 `rowLevelSecurity`** 的应用,得到的是一个**不受限的主档 id 集**,派生
+ 过滤器等于什么都没收窄 —— 只要持有对象级 read,全部明细行可读。行项目类对象(报价行、
+ 发票行)是这个形状的常客,而它们携带逐行定价与折扣。
+- 在主档上补写 RLS 也不是绕法:RLS 与 sharing 过滤器是 **AND**,补写会连同被共享进来的行
+ 一起切掉。
+- 写这半有同样的洞,而且是从另一侧来的:`assertControlledByParentWrite` 只在主档的写 RLS
+ 编译出非空过滤器时才检查主档行,主档没写 RLS 时**整段跳过** —— 持有 `allowEdit` 的调用者
+ 可以改自己根本看不到的父记录下的明细。
+
+**修复**:主档可达性改走与「直接读 / 直接写主档」完全相同的路径,复用既有合成点,不在
+plugin-security 里重刻一份 sharing 语义。
+
+- 读:`computeControlledByParentFilter` 现在把主档的读 RLS 与 `resolveSharingReadFilter`
+ (`getReadFilter` 已经在用的那个 OWD/共享半边)AND 起来再解析主档 id 集。哪一半生效由
+ **主档自己的有效共享模型**决定,因此派生出的可见集与直接 find 主档逐点一致。
+- 写:`assertControlledByParentWrite` 在原有的 CRUD `update` + 写 RLS 之外,**无条件**追问
+ plugin-sharing 的单记录写闸 `canEdit`(归属按写深度放宽、`edit` 级共享、
+ `modifyAllRecords` 旁路)—— 无条件,正因为写 RLS 那一半在常见情形下会被整段跳过。
+- 两侧解析失败一律**fail closed**(主档 id 集为空 / 拒绝写),而不是悄悄放宽回全员可见。
+
+未变更的部分:v1 的**单层**语义 —— 主档自身的 `controlled_by_parent` 仍不递归下钻;没有装
+`plugin-sharing` 的部署行为不变(那种部署里主档本身也没有归属与共享可言,派生集依旧与直接
+读主档相等);`read` 级共享仍然只开读不开写,与直接访问主档的逐动词答案一致。

packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts

@@ -0,0 +1,355 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+//
+// [#5386] `controlled_by_parent` (ADR-0055) must resolve the MASTER's
+// accessibility through the SAME path a direct read / write of that master
+// takes — owner scope and `sys_record_share` grants folded in, not the master's
+// RLS policies alone.
+//
+// ## What was measured before this suite existed
+//
+// The derivation ran `computeRlsFilter(master, 'find')` and nothing else, under
+// a system context. Owner scope and record shares are contributed by a SIBLING
+// plugin (`plugin-sharing`), whose `buildReadFilter` returns `null` for any
+// object whose effective sharing model is not `private` — and
+// `controlled_by_parent` maps to `public` there. So the two halves of
+// record-level access never met on a derived object:
+//
+// • an app that authors NO `rowLevelSecurity` on the master got an
+// UNRESTRICTED master id set, i.e. the declared narrowing narrowed nothing
+// and every detail row was readable by any holder of object-level read;
+// • authoring RLS on the master was not a workaround, because RLS is ANDed
+// with the sharing filter rather than OR-ed into it, so it also cut off the
+// rows a grant had shared in;
+// • the write half had the same hole from the other side — its master row
+// check was SKIPPED WHOLE when the master's write RLS compiled to `null`.
+//
+// ## Why one fixture drives both faces
+//
+// The read filter and the write assertion are two implementation faces of ONE
+// contract ("a child follows its parent's access"). A suite that exercised them
+// on separate fixtures could not see them disagree, which is the failure this
+// family produces: a third, quieter answer where one face allows what the other
+// refuses. Every case below runs on the SAME three-account fixture — a master
+// owned by someone else and shared to the caller, a master owned by someone
+// else and NOT shared (the excluded row, without which "consistent" would prove
+// nothing), and a master the caller owns.
+
+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 type { PermissionSet } from '@objectstack/spec/security';
+
+const REP = 'usr_rep';
+const OTHER = 'usr_other';
+
+/** The MASTER — owner-scoped by OWD (`private`), with no authored RLS at all. */
+const ACCOUNT_SCHEMA = {
+ name: 'crm_account',
+ sharingModel: 'private',
+ fields: {
+ id: { name: 'id', type: 'text' },
+ name: { name: 'name', type: 'text' },
+ owner_id: { name: 'owner_id', type: 'lookup', reference: 'sys_user' },
+ },
+};
+
+/** The DETAIL — access derived from `crm_account` through the master_detail FK. */
+const CONTACT_SCHEMA = {
+ name: 'crm_contact',
+ sharingModel: 'controlled_by_parent',
+ fields: {
+ id: { name: 'id', type: 'text' },
+ name: { name: 'name', type: 'text' },
+ account: { name: 'account', type: 'master_detail', required: true, reference: 'crm_account' },
+ },
+};
+
+const SHARE_SCHEMA = {
+ name: 'sys_record_share',
+ isSystem: true,
+ fields: {
+ id: { name: 'id', type: 'text' },
+ object_name: { name: 'object_name', type: 'text' },
+ record_id: { name: 'record_id', type: 'text' },
+ recipient_type: { name: 'recipient_type', type: 'text' },
+ recipient_id: { name: 'recipient_id', type: 'text' },
+ access_level: { name: 'access_level', type: 'text' },
+ owner_id: { name: 'owner_id', type: 'lookup', reference: 'sys_user' },
+ },
+};
+
+/**
+ * The app's permission set: full CRUD on both objects (so requests reach the
+ * record layer instead of being refused by RBAC) and — deliberately — NO
+ * `rowLevelSecurity` anywhere. That is the shape the issue measured: the app
+ * expresses its record boundary entirely through OWD + sharing, and expects
+ * `controlled_by_parent` to follow it.
+ */
+const REP_SET: PermissionSet = {
+ name: 'crm_rep',
+ label: 'CRM Rep',
+ objects: {
+ crm_account: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true },
+ crm_contact: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true },
+ },
+} as unknown as PermissionSet;
+
+type Row = Record<string, unknown>;
+
+/**
+ * In-memory store standing in for the engine's READ surface only — `find`,
+ * `findOne`, `getSchema`. It declares NO write verb: nothing under test writes
+ * through it, and a double that does not have a verb cannot be looser than the
+ * engine on that verb (the `check:engine-double-contract` family, #4434/#5480).
+ * Filtering runs through `matchesFilterCondition`, the same evaluator the
+ * security plugin itself uses, so a filter this suite asserts on is a filter
+ * that was really applied rather than one merely inspected.
+ */
+function makeStore(rows: Record<string, Row[]>) {
+ const schemas: Record<string, unknown> = {
+ crm_account: ACCOUNT_SCHEMA,
+ crm_contact: CONTACT_SCHEMA,
+ sys_record_share: SHARE_SCHEMA,
+ };
+ return {
+ rows,
+ getSchema: (object: string) => schemas[object],
+ find: vi.fn(async (object: string, options: any = {}) => {
+ const all = rows[object] ?? [];
+ const hits = all.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 = {}) => {
+ const all = rows[object] ?? [];
+ return all.find((r) => matchesFilterCondition(r, options?.where ?? null)) ?? null;
+ }),
+ };
+}
+
+/** The fixture rows — identical for every case; only the grant level varies. */
+function fixtureRows(shareLevel: 'read' | 'edit' | null): Record<string, Row[]> {
+ return {
+ crm_account: [
+ { id: 'acct_us', name: 'US Corp', owner_id: OTHER }, // shared to the rep
+ { id: 'acct_eu', name: 'EU Corp', owner_id: OTHER }, // NOT shared — the excluded row
+ { id: 'acct_own', name: 'Own Corp', owner_id: REP }, // the rep's own
+ ],
+ crm_contact: [
+ { id: 'ct_us', name: 'US contact', account: 'acct_us' },
+ { id: 'ct_eu', name: 'EU contact', account: 'acct_eu' },
+ { id: 'ct_own', name: 'Own contact', account: 'acct_own' },
+ ],
+ sys_record_share: shareLevel
+ ? [
+ {
+ id: 'shr_1',
+ object_name: 'crm_account',
+ record_id: 'acct_us',
+ recipient_type: 'user',
+ recipient_id: REP,
+ access_level: shareLevel,
+ },
+ ]
+ : [],
+ };
+}
+
+interface BootOptions {
+ /** The single grant's level, or `null` for a fixture with no grant at all. */
+ shareLevel?: 'read' | 'edit' | null;
+ /** `'none'` boots a deployment WITHOUT plugin-sharing; `'throws'` a broken one. */
+ sharing?: 'real' | 'none' | 'throws';
+}
+
+async function boot(options: BootOptions = {}) {
+ const shareLevel = options.shareLevel === undefined ? 'edit' : options.shareLevel;
+ const store = makeStore(fixtureRows(shareLevel));
+
+ let middleware: any;
+ const ql = {
+ registerMiddleware: (mw: any) => {
+ if (!middleware) middleware = mw;
+ },
+ getSchema: store.getSchema,
+ find: store.find,
+ findOne: store.findOne,
+ };
+
+ const services: Record<string, unknown> = {
+ manifest: { register: vi.fn() },
+ objectql: ql,
+ metadata: { get: async (n: string) => store.getSchema(n), list: async () => [REP_SET] },
+ };
+ if ((options.sharing ?? 'real') === 'real') {
+ // The REAL sharing service over the same store — the point of the fix is
+ // that the derivation reuses this exact producer instead of re-deriving
+ // owner/share semantics inside plugin-security.
+ services.sharing = new SharingService({ engine: store as unknown as SharingEngine });
+ } else if (options.sharing === 'throws') {
+ services.sharing = {
+ buildReadFilter: async () => {
+ throw new Error('sharing store unreachable');
+ },
+ canEdit: async () => {
+ throw new Error('sharing store unreachable');
+ },
+ };
+ }
+
+ const ctx: any = {
+ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() },
+ registerService: vi.fn(),
+ getService: (name: string) => {
+ if (!(name in services)) throw new Error(`service not registered: ${name}`);
+ return services[name];
+ },
+ };
+ const plugin = new SecurityPlugin({
+ defaultPermissionSets: [REP_SET],
+ fallbackPermissionSet: 'crm_rep',
+ });
+ await plugin.init(ctx);
+ await plugin.start(ctx);
+
+ const repContext = () => ({ userId: REP, tenantId: 'org-1', positions: [], permissions: [] });
+
+ /** The READ face: run the middleware, then apply the filter it injected. */
+ const visibleContacts = async (): Promise<string[]> => {
+ const opCtx: any = {
+ object: 'crm_contact',
+ operation: 'find',
+ ast: {},
+ options: {},
+ context: repContext(),
+ };
+ await middleware(opCtx, async () => {});
+ return (store.rows.crm_contact ?? [])
+ .filter((r) => matchesFilterCondition(r, opCtx.ast.where ?? null))
+ .map((r) => String(r.id));
+ };
+
+ /** The WRITE face: a by-id update of one detail row. Resolves or throws. */
+ const updateContact = async (id: string): Promise<void> => {
+ const opCtx: any = {
+ object: 'crm_contact',
+ operation: 'update',
+ data: { id, name: 'renamed' },
+ options: { where: { id } },
+ context: repContext(),
+ };
+ await middleware(opCtx, async () => {});
+ };
+
+ const writableContacts = async (): Promise<string[]> => {
+ const out: string[] = [];
+ for (const row of store.rows.crm_contact ?? []) {
+ try {
+ await updateContact(String(row.id));
+ out.push(String(row.id));
+ } catch {
+ /* denied */
+ }
+ }
+ return out;
+ };
+
+ return { store, ctx, visibleContacts, updateContact, writableContacts };
+}
+
+describe('[#5386] controlled_by_parent folds the master\'s ownership and share grants in', () => {
+ // ── READ face ────────────────────────────────────────────────────────────
+ it('READ: only children of masters the caller can actually read are visible', async () => {
+ const h = await boot({ shareLevel: 'edit' });
+ // acct_own is reachable by OWNERSHIP, acct_us by the single GRANT,
+ // acct_eu by neither — so its contact must not appear.
+ expect(await h.visibleContacts()).toEqual(['ct_us', 'ct_own']);
+ });
+
+ it('READ: with no grant at all, only the caller-owned master\'s children survive', async () => {
+ const h = await boot({ shareLevel: null });
+ expect(await h.visibleContacts()).toEqual(['ct_own']);
+ });
+
+ it('READ: the derived master id set equals what a direct find of the master returns', async () => {
+ const h = await boot({ shareLevel: 'edit' });
+ // The master half, resolved exactly as a direct read would: owner-match
+ // OR-ed with the caller's grants. This is the set the child filter must
+ // quantify over — asserted here so a future change that widens the derived
+ // set without widening the master's own read is caught at the seam.
+ const sharing = new SharingService({ engine: h.store as unknown as SharingEngine });
+ const masterFilter = await sharing.buildReadFilter('crm_account', {
+ userId: REP,
+ tenantId: 'org-1',
+ } as any);
+ const directlyReadable = (h.store.rows.crm_account ?? [])
+ .filter((r) => matchesFilterCondition(r, masterFilter as any))
+ .map((r) => String(r.id));
+ expect(directlyReadable).toEqual(['acct_us', 'acct_own']);
+ });
+
+ // ── WRITE face ───────────────────────────────────────────────────────────
+ it('WRITE: a by-id update of a child under an unreachable master is denied', async () => {
+ const h = await boot({ shareLevel: 'edit' });
+ await expect(h.updateContact('ct_eu')).rejects.toThrow(/requires edit access to its master/);
+ });
+
+ it('WRITE: children of an owned master and of an edit-shared master stay writable', async () => {
+ const h = await boot({ shareLevel: 'edit' });
+ await expect(h.updateContact('ct_own')).resolves.toBeUndefined();
+ await expect(h.updateContact('ct_us')).resolves.toBeUndefined();
+ });
+
+ // ── the invariant the two faces owe each other ───────────────────────────
+ it('INVARIANT: read-visible and by-id-writable agree on the same fixture', async () => {
+ const h = await boot({ shareLevel: 'edit' });
+ const readable = await h.visibleContacts();
+ const writable = await h.writableContacts();
+ expect(readable.sort()).toEqual(writable.sort());
+ // …and the agreement is not the empty agreement: exactly one row is
+ // excluded, so "consistent" carries information.
+ expect(readable).not.toContain('ct_eu');
+ expect(readable).toHaveLength(2);
+ });
+
+ it('a READ-level grant opens the child for reading but NOT for writing — the master\'s own verb boundary', async () => {
+ const h = await boot({ shareLevel: 'read' });
+ // Same asymmetry a DIRECT access of the master has: a `read` share widens
+ // rows for reads only; `canEdit` refuses it. The derived faces must track
+ // that per-verb answer rather than collapsing to one of them.
+ expect(await h.visibleContacts()).toEqual(['ct_us', 'ct_own']);
+ expect(await h.writableContacts()).toEqual(['ct_own']);
+ await expect(h.updateContact('ct_us')).rejects.toThrow(/record sharing/);
+ });
+
+ // ── boundary conditions ──────────────────────────────────────────────────
+ it('the fold reads the grant table, resolves the master ONCE, and never re-enters the detail', async () => {
+ const h = await boot({ shareLevel: 'edit' });
+ h.store.find.mockClear();
+ await h.visibleContacts();
+ const reads = h.store.find.mock.calls.map((c: any[]) => String(c[0]));
+ // The grant table was consulted — direct evidence the sharing half ran and
+ // not merely that some filter came back.
+ expect(reads).toContain('sys_record_share');
+ // The master id set is resolved by ONE system-context read. More than one
+ // would mean the derivation re-entered the middleware (v1 is single-level,
+ // ADR-0055 — the master's own controlled_by_parent is not walked).
+ expect(reads.filter((o) => o === 'crm_account')).toHaveLength(1);
+ expect(reads).not.toContain('crm_contact');
+ });
+
+ it('a deployment WITHOUT plugin-sharing is unchanged — RLS-only derivation, nothing to fold', async () => {
+ // No sharing service means no owner scope and no grants anywhere in the
+ // deployment, so a direct find of the master returns every row too: the
+ // derived set stays point-for-point equal to it.
+ const h = await boot({ shareLevel: 'edit', sharing: 'none' });
+ expect(await h.visibleContacts()).toEqual(['ct_us', 'ct_eu', 'ct_own']);
+ await expect(h.updateContact('ct_eu')).resolves.toBeUndefined();
+ });
+
+ it('fail-closed: a sharing service that throws denies BOTH faces', async () => {
+ const h = await boot({ shareLevel: 'edit', sharing: 'throws' });
+ expect(await h.visibleContacts()).toEqual([]);
+ await expect(h.updateContact('ct_us')).rejects.toThrow(/record sharing/);
+ });
+});