fix(plugin-security): getReadFilter 补上 controlled_by_parent 主档收窄 —— analytics 读面与 middleware 同一作用域 (#5815)

.changeset/getreadfilter-controlled-by-parent.md

@@ -0,0 +1,42 @@
+---
+"@objectstack/plugin-security": patch
+---
+
+fix(plugin-security): `getReadFilter` applies the `controlled_by_parent` derivation — the analytics read scope was missing the master half entirely
+
+`getReadFilter` is the read-scope provider bound by the analytics / raw-SQL
+path: the one read surface that bypasses the engine and therefore has no other
+source of scope. Its contract is that it returns **the same filter the engine
+middleware ANDs into every find**. That middleware injects three things — the
+RLS filter, the ADR-0055 `controlled_by_parent` derivation (`masterFK IN
+(accessible master ids)`), and plugin-sharing's OWD / record-share filter.
+`getReadFilter` composed only the first and third; `computeControlledByParentFilter`
+was never called on that path at all.
+
+For an object whose `sharingModel` is `controlled_by_parent` that is not a
+partial gap but a total one, because the two layers it *did* compose both stand
+down on exactly that object by design: such an object carries no authored RLS
+(the whole point of the model is that access is derived rather than authored),
+and it maps to `public` in plugin-sharing's `effectiveSharingModel`, so
+`buildReadFilter` returns `null`. Both halves returned `null`, the composition
+returned `undefined`, and the analytics path ran with **no predicate**. A caller
+who could not read a single master row through `/data` could still `COUNT(*)`
+and `GROUP BY` its detail rows through `/analytics` — and line-item objects are
+the usual shape here, so the grouped values are per-line prices and discounts.
+
+The derivation is now composed into the same AND on that path, resolved from the
+permission sets `getReadFilter` had already resolved (no second resolution), so
+the two read surfaces enforce identical scoping — which is why
+`computeControlledByParentFilter` was extracted and shared in the first place.
+Failures deny: the derivation is internally fail-closed, and a throw propagates
+to the method's existing fail-closed handler rather than widening the read. The
+delegated (`onBehalfOf`) branch already denied outright on this path (#2852) and
+is unchanged.
+
+This is the same failure shape #4467 fixed for the OWD/sharing layer of this
+method, one layer over; #5386 fixed *which inputs* the derivation folds in, not
+*whether it runs* on this surface.
+
+**Impact.** A deployment with `controlled_by_parent` objects and an analytics /
+raw-SQL consumer will see those queries return fewer rows — the rows the caller
+was never entitled to aggregate. No authoring change is required.

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

@@ -229,6 +229,18 @@ async function boot(options: BootOptions = {}) {
       .map((r) => String(r.id));
   };
 
+  /**
+   * [#5815] The ANALYTICS read face: the scope `getReadFilter` hands the
+   * raw-SQL path, applied to the same rows. No middleware runs here — this
+   * method IS the whole enforcement on that surface.
+   */
+  const analyticsVisibleContacts = async (context?: any): Promise<string[]> => {
+    const filter = await plugin.getReadFilter('crm_contact', context ?? repContext());
+    return (store.rows.crm_contact ?? [])
+      .filter((r) => matchesFilterCondition(r, (filter ?? null) as any))
+      .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 = {
@@ -254,7 +266,16 @@ async function boot(options: BootOptions = {}) {
     return out;
   };
 
-  return { store, ctx, visibleContacts, updateContact, writableContacts };
+  return {
+    store,
+    ctx,
+    plugin,
+    repContext,
+    visibleContacts,
+    analyticsVisibleContacts,
+    updateContact,
+    writableContacts,
+  };
 }
 
 describe('[#5386] controlled_by_parent folds the master\'s ownership and share grants in', () => {
@@ -353,3 +374,96 @@ describe('[#5386] controlled_by_parent folds the master\'s ownership and share g
     await expect(h.updateContact('ct_us')).rejects.toThrow(/record sharing/);
   });
 });
+
+// ---------------------------------------------------------------------------
+
+/**
+ * [#5815] The THIRD face of the same contract — `getReadFilter`, the read-scope
+ * provider bound by the analytics / raw-SQL path.
+ *
+ * The suite above pins the engine middleware and the by-id write gate against
+ * one fixture precisely so a third, quieter answer cannot hide between them.
+ * There WAS one: `getReadFilter` composed the RLS layer and the sharing layer
+ * and never called the controlled_by_parent derivation at all. On this exact
+ * fixture both of those layers legitimately return `null` — the app authors no
+ * RLS, and plugin-sharing maps `controlled_by_parent` to `public` — so the
+ * composed scope came back `undefined`: NO predicate. A caller who could not
+ * read `acct_eu` could still `COUNT(*)` and `GROUP BY` its contacts.
+ *
+ * These cases therefore assert AGREEMENT, not a filter shape. The two read
+ * surfaces may compose their layers in any order and produce different ASTs;
+ * what they may never do is disagree about which rows are visible.
+ */
+describe('[#5815] getReadFilter enforces the same read scope as the engine middleware', () => {
+  it('PARITY: the analytics read scope and the middleware-injected `ast.where` see the same rows', async () => {
+    const h = await boot({ shareLevel: 'edit' });
+    const viaMiddleware = await h.visibleContacts();
+    const viaReadFilter = await h.analyticsVisibleContacts();
+    expect(viaReadFilter.sort()).toEqual(viaMiddleware.sort());
+    // …and the agreement carries information: one row is excluded on BOTH
+    // surfaces. Without this, "identical" would be satisfied by two surfaces
+    // that each return everything — which is exactly the broken state.
+    expect(viaReadFilter).not.toContain('ct_eu');
+    expect(viaReadFilter).toHaveLength(2);
+  });
+
+  it('the returned scope is a real predicate, not `undefined` — the defect measured', async () => {
+    const h = await boot({ shareLevel: 'edit' });
+    const filter = await h.plugin.getReadFilter('crm_contact', h.repContext());
+    // Before the fix this was `undefined` (RLS null AND sharing null), which
+    // the raw-SQL path spreads into a WHERE that constrains nothing.
+    expect(filter).toBeDefined();
+    expect(JSON.stringify(filter)).toContain('acct_own');
+    expect(JSON.stringify(filter)).not.toContain('acct_eu');
+  });
+
+  it('PARITY with no grant at all: both surfaces narrow to the caller-owned master', async () => {
+    const h = await boot({ shareLevel: null });
+    expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts());
+    expect(await h.analyticsVisibleContacts()).toEqual(['ct_own']);
+  });
+
+  it('PARITY on a READ-level grant: the read share widens the analytics face too', async () => {
+    const h = await boot({ shareLevel: 'read' });
+    expect(await h.analyticsVisibleContacts()).toEqual(['ct_us', 'ct_own']);
+    expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts());
+  });
+
+  it('PARITY without plugin-sharing: neither surface narrows, for the same reason', async () => {
+    // Both faces stand down together — there is no owner scope and there are no
+    // grants anywhere in the deployment, so a direct find of the master returns
+    // every row. Parity must hold in the WIDE direction as well, or the pin
+    // would merely be asserting that the analytics face is always narrower.
+    const h = await boot({ shareLevel: 'edit', sharing: 'none' });
+    expect(await h.analyticsVisibleContacts()).toEqual(['ct_us', 'ct_eu', 'ct_own']);
+    expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts());
+  });
+
+  it('fail-closed: a sharing service that throws denies the analytics face too', async () => {
+    const h = await boot({ shareLevel: 'edit', sharing: 'throws' });
+    expect(await h.analyticsVisibleContacts()).toEqual([]);
+    expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts());
+  });
+
+  it('fail-closed: the derivation resolves the master ONCE per call, never re-entering the detail', async () => {
+    const h = await boot({ shareLevel: 'edit' });
+    h.store.find.mockClear();
+    await h.analyticsVisibleContacts();
+    const reads = h.store.find.mock.calls.map((c: any[]) => String(c[0]));
+    // Direct evidence the derivation (and its sharing half) ran on THIS path
+    // rather than a filter merely coming back from somewhere.
+    expect(reads).toContain('sys_record_share');
+    expect(reads.filter((o) => o === 'crm_account')).toHaveLength(1);
+    expect(reads).not.toContain('crm_contact');
+  });
+
+  it('a delegated (on-behalf-of) read still denies outright — unchanged by the added layer', async () => {
+    // [#2852] The D10 delegator intersection is not implemented on this path,
+    // so it fails closed BEFORE any layer is composed. Pinned here because the
+    // added layer must not become a reason to compute a scope for a context
+    // this method refuses to scope at all.
+    const h = await boot({ shareLevel: 'edit' });
+    const delegated = { ...h.repContext(), onBehalfOf: { userId: OTHER } };
+    expect(await h.analyticsVisibleContacts(delegated)).toEqual([]);
+  });
+});

packages/plugins/plugin-security/src/security-plugin.ts

@@ -2398,6 +2398,33 @@ export class SecurityPlugin implements Plugin {
     }
   }
 
+  /**
+   * The read scope for `object` under `context` — the filter the analytics /
+   * raw-SQL path ANDs into its query, being the one surface that bypasses the
+   * engine and so has no other source of scope.
+   *
+   * Its contract is agreement: this must be **the same filter the engine
+   * middleware ANDs into every find**. That chain injects THREE things, and
+   * this method composes the same three:
+   *
+   *   1. {@link computeRlsFilter} — tenant Layer 0 + RLS policies;
+   *   2. {@link computeControlledByParentFilter} — ADR-0055, `masterFK IN
+   *      (accessible master ids)`;
+   *   3. {@link resolveSharingReadFilter} — plugin-sharing's OWD / record-share
+   *      visibility filter, contributed by the sibling middleware.
+   *
+   * Each layer has been missing here at some point, and each absence had the
+   * same shape: a caller who cannot read a row through `/data` could still
+   * `COUNT(*)` / `GROUP BY` it through `/analytics`. Layer 3 was #4467; layer 2
+   * was #5815 — for a `controlled_by_parent` object, halves 1 and 3 BOTH
+   * commonly return `null` by design (such an object carries no authored RLS,
+   * and maps to `public` in plugin-sharing's `effectiveSharingModel`), so the
+   * composed scope was `undefined` — no predicate at all over what are usually
+   * line-item rows.
+   *
+   * Fails CLOSED on any resolution failure: a dropped predicate here is the
+   * leak, so an unresolvable layer denies (zero rows) rather than widening.
+   */
   async getReadFilter(
     object: string,
     context?: any,
@@ -2427,6 +2454,11 @@ export class SecurityPlugin implements Plugin {
     // already 401s without a token). Mirrors the middleware's early `return next()`
     // — which is the RLS middleware's early exit only, so the sharing predicate
     // resolved above still applies.
+    // [#5815] The controlled_by_parent derivation is deliberately NOT resolved
+    // ahead of this branch the way the sharing predicate is: it stands down
+    // without a `userId` (`computeControlledByParentFilter` returns null), and
+    // this branch is reached only when there is none — so the middleware adds
+    // nothing here either. Agreement holds by both sides standing down.
     if (positions.length === 0 && explicit.length === 0 && !context?.userId) {
       return sharingFilter ?? undefined;
     }
@@ -2454,10 +2486,23 @@ export class SecurityPlugin implements Plugin {
     try {
       const permissionSets = await this.resolvePermissionSetsForContext(context);
       const filter = await this.computeRlsFilter(permissionSets, object, 'find', context);
-      // [#4467] RLS AND sharing — the same AND-composition the two middlewares
-      // achieve by both writing into `ast.where`. Either half may be absent;
-      // `andComposeLayers` returns the other, or null when neither constrains.
-      return andComposeLayers(filter, sharingFilter) ?? undefined;
+      // [#5815] ADR-0055 — the SECOND thing the middleware ANDs into `ast.where`
+      // for a read. Resolved from the sets already resolved above, exactly as
+      // the middleware feeds it its own, so the derived master id set is
+      // computed for this identity once and never re-resolved. It stands down
+      // (null) for any object that is not controlled_by_parent, and it is
+      // internally fail-closed; a THROW propagates to the catch below, which
+      // denies — the same posture as the surrounding layers.
+      const cbpFilter = await this.computeControlledByParentFilter(
+        permissionSets,
+        object,
+        context,
+      );
+      // [#4467] RLS AND controlled-by-parent AND sharing — the same
+      // AND-composition the middlewares achieve by all writing into `ast.where`.
+      // Any layer may be absent; `andComposeLayers` returns the others, or null
+      // when none constrains.
+      return andComposeLayers(andComposeLayers(filter, cbpFilter), sharingFilter) ?? undefined;
     } catch (e) {
       // Fail CLOSED — a resolution failure must deny (zero rows), never expose
       // every tenant's data through the raw-SQL analytics path.