diff --git a/.changeset/bu-graph-tenant-screen.md b/.changeset/bu-graph-tenant-screen.md new file mode 100644 index 0000000000..47a27e00c0 --- /dev/null +++ b/.changeset/bu-graph-tenant-screen.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-sharing': patch +--- + +Sharing rules with a business-unit recipient no longer grant nothing when the unit was created by seed data. + +`BusinessUnitGraphService.orgScope` screened `sys_business_unit` with a strict `organization_id` equality, while the platform's own tenant screen (`SqlDriver.applyTenantScope`) is null-inclusive: `(organization_id = ? OR organization_id IS NULL)`. A sharing rule always carries the caller's organization, but a business unit written by seed data carries none — a seed cannot know the id the runtime mints at boot — so the two never matched. The seed check read the unit as "does not exist", both recipient widths (`business_unit` and `unit_and_subordinates`) expanded to zero users, and the rule stayed active having materialised no `sys_record_share` row and logged nothing. `orgScope` now applies the platform's null-inclusive screen, matching what `plugin-approvals` already did for the same rows. + +The member reads are now tenant-screened, which they were not before. Both `expandUnitMembers` and `expandUsers` queried `sys_business_unit_member` with no organization predicate at all, under a system context that carries no tenant; the strict unit screen was the only thing keeping an org-stamped rule away from that unscoped query. Widening the unit screen alone would have turned a silent under-grant into a silent cross-tenant over-grant, since a seeded unit id exists identically in every tenant. The member screen is strict rather than null-inclusive on purpose: seed replay and elevated system writes both leave `sys_business_unit_member.organization_id` NULL, so a NULL there means unknown tenancy rather than platform-global, and an org-scoped rule does not grant to it. + +An active business-unit rule that expands to no recipients now warns once per rule per process, naming the rule, the object, the recipient kind, the unit and the organization. That case — a rule whose unit and memberships were both seeded — is the one combination that still grants nobody, and it is no longer silent. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 682fd4c272..5d6bb89b3d 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -137,7 +137,7 @@ The largest single consumer — **20 of the 109 sites**. | 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1077` | | 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link creation/resolution while the policy is off | `plugin-sharing/src/share-link-service.ts:440`, `:494`, `:498`, `:571`, `:601` | | 38 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `sharing-rule-provenance.ts:47` | -| 39 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, and the platform-global-rule delete guard | `sharing-rule-service.ts:157`, `:382` | +| 39 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, and the platform-global-rule delete guard | `sharing-rule-service.ts:164`, `:389` | ### 4. Approvals, reports, attachments, comments, knowledge diff --git a/packages/plugins/plugin-sharing/src/bu-rule-tenant-screen.test.ts b/packages/plugins/plugin-sharing/src/bu-rule-tenant-screen.test.ts new file mode 100644 index 0000000000..aeea19229a --- /dev/null +++ b/packages/plugins/plugin-sharing/src/bu-rule-tenant-screen.test.ts @@ -0,0 +1,281 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14547] A business-unit sharing rule against a SEEDED unit, end to end. + * + * `business-unit-graph.test.ts` pins the two screens at the graph service. + * This file drives the whole rule path — `evaluateRule` → `expandRecipient` → + * `reconcile` → `sys_record_share` — because that is the layer the defect was + * reported at and the layer at which it was silent: the rule was accepted, it + * stayed `active: true`, it materialised zero grants, and nothing was logged. + * Asserting the graph's return value alone would leave every one of those + * observable facts unpinned. + * + * The fixture is the reported reproduction's shape, not an invented one: + * + * - `sys_business_unit` rows come from app SEED data and carry + * `organization_id = NULL` — a seed cannot know the id the runtime mints + * at boot; + * - `sys_business_unit_member` rows are POSTed through the REST data API and + * ARE organization-stamped (the engine threads the caller's tenant and the + * SQL driver stamps the injected column); + * - the `sys_sharing_rule` row is created by an organization admin and is + * org-stamped too (an explicit `organization_id: null` in the payload is + * overridden). + * + * Two of those three carry an organization and one does not, which is exactly + * the combination the strict unit screen turned into zero grants. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { SharingService } from './sharing-service.js'; +import { SharingRuleService } from './sharing-rule-service.js'; + +interface Row { [k: string]: any } + +const SYS = { isSystem: true, positions: [], permissions: [] } as any; +const ORG_A = 'org_a'; +const ORG_B = 'org_b'; + +/** + * Filter matcher over the operators this path actually emits. + * + * `organization_id: null` must match a row that OMITS the column, because that + * is what a NULL column reads back as and the whole `$or` arm exists for it. A + * fake that answered otherwise would report the widened screen as still broken + * — or, worse, report a screen that never widened as fixed. + */ +function matches(row: Row, f: any): boolean { + if (!f || typeof f !== 'object') return true; + for (const [k, v] of Object.entries(f)) { + if (k === '$or') { + if (!(v as any[]).some((sub) => matches(row, sub))) return false; + continue; + } + if (k === '$and') { + if (!(v as any[]).every((sub) => matches(row, sub))) return false; + continue; + } + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + const rv = row[k]; + if (v === null) { + if (rv != null) return false; + continue; + } + if (v != null && typeof v === 'object' && !Array.isArray(v)) { + const op: any = v; + if ('$in' in op) { if (!op.$in.includes(rv)) return false; continue; } + // `descendants()` filters children with `active: { $ne: false }`, so an + // undefined `active` must PASS — the graph treats absent as active. + if ('$ne' in op) { if (rv === op.$ne) return false; continue; } + if ('$gte' in op) { if (!(rv >= op.$gte)) return false; continue; } + } + if (rv !== v) return false; + } + return true; +} + +function makeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + let seq = 0; + return { + _tables: tables, + getSchema() { return undefined; }, + seed(object: string, rows: Row[]) { ensure(object).push(...rows.map((r) => ({ ...r }))); }, + async find(o: string, opts?: any) { + const f = opts?.filter ?? opts?.where; + return ensure(o).filter((r) => matches(r, f)).slice(0, opts?.limit ?? 10000); + }, + async insert(o: string, data: any) { + const row = { id: data.id ?? `${o}_${++seq}`, ...data }; + ensure(o).push(row); + return row; + }, + // The PRODUCER's own dispatch predicates, so a fixture that drifts to a + // call shape `ObjectQL` would refuse fails here instead of going green. + async update(o: string, data: any, options?: any) { + const verdict = assertEngineUpdateDispatch(data, options); + const t = ensure(o); + const targets = verdict.kind === 'by-id' + ? t.filter((r) => r.id === verdict.id) + : t.filter((r) => matches(r, options?.where)); + for (const r of targets) Object.assign(r, data); + return verdict.kind === 'by-id' ? (targets[0] ?? null) : targets.length; + }, + async delete(o: string, opts?: any) { + assertEngineDeleteDispatch(opts); + const t = ensure(o); + const where = opts?.where ?? (opts?.id != null ? { id: opts.id } : {}); + for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); + return { ok: true }; + }, + }; +} + +const RULE = 'kpi_sheet_to_market_unit'; + +describe('#14547 — an org-stamped rule against a SEEDED business unit', () => { + let engine: ReturnType; + let rules: SharingRuleService; + let warn: ReturnType void>>; + + /** Who currently holds a rule-materialised grant on `recordId`. */ + const granteesOf = (recordId: string): string[] => + (engine._tables.sys_record_share ?? []) + .filter((r) => r.record_id === recordId && r.source === 'rule') + .map((r) => String(r.recipient_id)) + .sort(); + + /** The warn lines this run emitted, as one searchable string each. */ + const warnLines = (): string[] => warn.mock.calls.map((c) => String(c[0])); + const emptyExpansionWarns = (): any[][] => + warn.mock.calls.filter((c) => String(c[0]).includes('expands to NO recipients')); + + beforeEach(() => { + engine = makeEngine(); + warn = vi.fn(); + const sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing, logger: { warn } }); + + // Seed data: units written before any organization existed. + engine.seed('sys_business_unit', [ + { id: 'bu_market', name: 'Market', parent_business_unit_id: null, organization_id: null, active: true }, + { id: 'bu_market_west', name: 'Market West', parent_business_unit_id: 'bu_market', organization_id: null, active: true }, + ]); + engine.seed('kpi_entry_sheet', [{ id: 'kpi_1', subject: 'bu_market', owner_id: 'author' }]); + }); + + /** Create the rule the reproduction created, org-stamped like a real one. */ + const seedRule = (recipientType: 'business_unit' | 'unit_and_subordinates', organizationId: string | null = ORG_A) => { + engine.seed('sys_sharing_rule', [{ + id: 'srule_kpi', organization_id: organizationId, name: RULE, + label: 'KPI sheet → Market', object_name: 'kpi_entry_sheet', + criteria_json: JSON.stringify({ subject: 'bu_market' }), + recipient_type: recipientType, recipient_id: 'bu_market', + access_level: 'edit', active: true, managed_by: 'package', + }]); + }; + + describe('the reported defect: 201, active, zero shares, no log', () => { + it('WIDE — `unit_and_subordinates` now materialises the grants', async () => { + engine.seed('sys_business_unit_member', [ + { id: 'bum_1', business_unit_id: 'bu_market', user_id: 'u_1', organization_id: ORG_A }, + { id: 'bum_2', business_unit_id: 'bu_market_west', user_id: 'u_2', organization_id: ORG_A }, + ]); + seedRule('unit_and_subordinates'); + const result = await rules.evaluateRule(RULE, SYS); + expect(granteesOf('kpi_1')).toEqual(['u_1', 'u_2']); + expect(result.expandedUsers).toBe(2); + // …and it did so QUIETLY: the new warn is for the empty case only. + expect(emptyExpansionWarns()).toHaveLength(0); + }); + + it('NARROW — `business_unit` materialises the anchor unit only', async () => { + engine.seed('sys_business_unit_member', [ + { id: 'bum_1', business_unit_id: 'bu_market', user_id: 'u_1', organization_id: ORG_A }, + { id: 'bum_2', business_unit_id: 'bu_market_west', user_id: 'u_2', organization_id: ORG_A }, + ]); + seedRule('business_unit'); + await rules.evaluateRule(RULE, SYS); + // The two widths stay two widths (#7807) — the tenant screen moved, the + // subtree boundary did not. + expect(granteesOf('kpi_1')).toEqual(['u_1']); + }); + }); + + describe('the leak the same change would have opened', () => { + it('another organization’s members are never granted through the shared seeded unit', async () => { + // ONE seeded unit id, two tenants' memberships hanging off it — the + // shape that exists on any deployment whose org chart came from a seed. + engine.seed('sys_business_unit_member', [ + { id: 'bum_a', business_unit_id: 'bu_market', user_id: 'u_a', organization_id: ORG_A }, + { id: 'bum_b', business_unit_id: 'bu_market', user_id: 'u_b', organization_id: ORG_B }, + ]); + seedRule('unit_and_subordinates', ORG_A); + await rules.evaluateRule(RULE, SYS); + expect(granteesOf('kpi_1')).toEqual(['u_a']); + expect(granteesOf('kpi_1')).not.toContain('u_b'); + }); + }); + + describe('an active rule that grants nobody is LOUD', () => { + it('warns naming the rule, the recipient kind and the unit', async () => { + // Unit and memberships BOTH seeded: the unit resolves now, but org-less + // membership rows are of unknown tenancy and are not members of an + // org-stamped rule. The residual empty expansion is the case this warn + // exists for. + engine.seed('sys_business_unit_member', [ + { id: 'bum_seeded', business_unit_id: 'bu_market', user_id: 'u_seeded' }, + ]); + seedRule('unit_and_subordinates'); + await rules.evaluateRule(RULE, SYS); + + expect(granteesOf('kpi_1')).toEqual([]); + const calls = emptyExpansionWarns(); + expect(calls).toHaveLength(1); + expect(String(calls[0][0])).toContain('organization_id'); + expect(calls[0][1]).toMatchObject({ + rule: RULE, + object: 'kpi_entry_sheet', + recipientType: 'unit_and_subordinates', + businessUnit: 'bu_market', + organization: ORG_A, + }); + }); + + it('warns for the NARROW width too', async () => { + seedRule('business_unit'); + await rules.evaluateRule(RULE, SYS); + expect(emptyExpansionWarns()).toHaveLength(1); + expect(emptyExpansionWarns()[0][1]).toMatchObject({ recipientType: 'business_unit' }); + }); + + it('warns ONCE per rule per process, not once per evaluation', async () => { + // The reconcilers call `expandRecipient` on every matched write. Without + // the dedup one misconfigured rule dominates the deployment's log — + // the same reasoning the inert-criteria warn already carries. + seedRule('unit_and_subordinates'); + await rules.evaluateRule(RULE, SYS); + await rules.evaluateRule(RULE, SYS); + await rules.evaluateRule(RULE, SYS); + expect(emptyExpansionWarns()).toHaveLength(1); + expect(rules.emptyUnitExpansionRuleKeys).toEqual(['srule_kpi::bu_market']); + }); + + it('says nothing when the rule grants somebody', async () => { + engine.seed('sys_business_unit_member', [ + { id: 'bum_1', business_unit_id: 'bu_market', user_id: 'u_1', organization_id: ORG_A }, + ]); + seedRule('unit_and_subordinates'); + await rules.evaluateRule(RULE, SYS); + expect(warnLines().join('\n')).not.toContain('expands to NO recipients'); + expect(rules.emptyUnitExpansionRuleKeys).toEqual([]); + }); + + it('an INACTIVE rule is not warned about — it is meant to grant nobody', async () => { + engine.seed('sys_sharing_rule', [{ + id: 'srule_off', organization_id: ORG_A, name: 'off_rule', + label: 'Off', object_name: 'kpi_entry_sheet', + criteria_json: JSON.stringify({ subject: 'bu_market' }), + recipient_type: 'unit_and_subordinates', recipient_id: 'bu_market', + access_level: 'edit', active: false, managed_by: 'package', + }]); + await rules.evaluateRule('off_rule', SYS); + expect(emptyExpansionWarns()).toHaveLength(0); + }); + }); + + describe('the org-less rule — the dominant shape today — is unmoved', () => { + it('still expands every member of the seeded tree, stamped or not', async () => { + engine.seed('sys_business_unit_member', [ + { id: 'bum_1', business_unit_id: 'bu_market', user_id: 'u_1', organization_id: ORG_A }, + { id: 'bum_2', business_unit_id: 'bu_market_west', user_id: 'u_2' }, + ]); + seedRule('unit_and_subordinates', null); + await rules.evaluateRule(RULE, SYS); + expect(granteesOf('kpi_1')).toEqual(['u_1', 'u_2']); + }); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts index ca4ccd2145..918817fc4b 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts @@ -12,24 +12,34 @@ * dead `department:` approver slot; here it would produce a sharing rule * that silently grants nobody. * - * It is NOT reachable today: every materialized `sys_sharing_rule` row carries - * `organization_id = null` (verified on a live showcase stack), so - * `expandRecipient` passes `null` and `orgScope` is skipped entirely. The - * moment rules start carrying an org — a multi-tenant deployment — a BU - * subtree rule against a seeded unit stops granting, and the symptom is - * "the right people cannot see the record", which is far quieter than a stuck - * approval. + * [#14547] IT BECAME REACHABLE, and the divergence is now closed. A rule + * created through the REST data API by an organization admin IS org-stamped + * (the engine stamps it, and an explicit `organization_id: null` in the payload + * is overridden), so `expandRecipient` passes a real organization and the + * strict screen fired on exactly the seeded units the reproduction used. The + * predicate this file's earlier note nominated is the one that landed: + * `orgScope` now composes `$or: [{ organization_id }, { organization_id: null }]` + * — the platform's own null-inclusive tenant screen, the same one + * `SqlDriver.applyTenantScope` emits and the same reading `plugin-approvals` + * already had. * - * So this file locks BOTH sides down: - * - the reachable paths (null-org rule) keep working, and - * - the divergence from approvals is written down as an executable fact - * rather than a comment, so flipping it is a deliberate edit to a named - * test and never a silent behaviour change. + * ⚠️ Widening that screen alone would have been a LEAK, which is why this file + * now pins a PAIR of asymmetric screens rather than one: * - * If the platform decides null-org means "env-wide, visible to every org" for - * sharing too — the way `plugin-approvals` and `sys_metadata` already read it — - * the test named `[divergence]` below is the one to flip, and `orgScope` grows - * the same `$or: [{ organization_id }, { organization_id: null }]` predicate. + * - the UNIT screen is null-inclusive (a seeded unit is usable), while + * - the MEMBER screen is STRICT (`memberScope`) — the member reads used to + * carry no organization predicate at all, and the old strict unit screen + * was the only thing keeping an org-stamped rule away from that unscoped + * query. A seeded unit id exists identically in every tenant, so widening + * the unit screen without screening the members turns a silent under-grant + * into a silent CROSS-TENANT over-grant. + * + * A NULL organization means different things on the two rows, which is what + * makes the asymmetry principled rather than convenient: on the unit it is the + * documented platform/seeded class, and on a member row it is UNKNOWN TENANCY + * (measured: seed replay and elevated system writes both leave + * `sys_business_unit_member.organization_id` NULL). A grant fails closed on + * unknown tenancy. */ import { describe, it, expect } from 'vitest'; @@ -41,7 +51,12 @@ interface UnitRow { organization_id?: string | null; active?: boolean; } -interface MemberRow { business_unit_id: string; user_id: string } +interface MemberRow { + business_unit_id: string; + user_id: string; + /** [#14547] Absent = the org-less row seed replay and system writes produce. */ + organization_id?: string | null; +} /** * Minimal engine over `sys_business_unit` + `sys_business_unit_member`. @@ -167,13 +182,15 @@ describe('BusinessUnitGraphService — the two widths are actually two widths (# expect(await graph().expandUnitMembers('')).toEqual([]); }); - it('the narrow width is org-predicated exactly like the wide one', async () => { + it('[#14547] the narrow width is org-predicated exactly like the wide one', async () => { const g = new BusinessUnitGraphService({ engine: makeEngine(DIV_UNITS, DIV_MEMBERS), organizationId: 'org_a', }); - // Seeded (null-org) units are not visible to an org-scoped rule — the - // same `[divergence]` posture the wide width holds below. + // The seeded (null-org) UNIT is now usable — but these members carry no + // organization either, and an org-stamped rule does not grant to rows of + // unknown tenancy. Empty, for the member reason rather than the unit one; + // the org-stamped-member control is in the `#14547` block below. expect(await g.expandUnitMembers('bu_div')).toEqual([]); }); @@ -208,8 +225,14 @@ describe('BusinessUnitGraphService — org scoping (#3807)', () => { { id: 'bu_root', organization_id: 'org_a', active: true }, { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_a', active: true }, ]; + // [#14547] The MEMBER rows now carry the organization too. This fixture + // used to leave them org-less and pass anyway, because the member read was + // unscoped — the very gap #14547 closed. Stamping them is not a workaround + // for the new screen: an org-scoped unit whose members were written by the + // same org-scoped API calls is what this test was always describing. + const members: MemberRow[] = SEEDED_MEMBERS.map((m) => ({ ...m, organization_id: 'org_a' })); const g = new BusinessUnitGraphService({ - engine: makeEngine(units, SEEDED_MEMBERS), + engine: makeEngine(units, members), organizationId: 'org_a', }); expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']); @@ -224,17 +247,125 @@ describe('BusinessUnitGraphService — org scoping (#3807)', () => { expect(await g.expandUsers('bu_root')).toEqual([]); }); - it('[divergence] an org-scoped rule does NOT see an env-wide (null-org) unit — approvals does (#3807)', async () => { - // Same inputs that #3807 fixed on the approvals side. Sharing still reads - // a null-org unit as "belongs to no org, therefore not mine" and grants - // nobody. Unreachable today (rules are null-org), deliberate until the - // platform rules on null-org semantics for AUTHORIZATION paths — widening - // who can SEE a record is not a change to make on a defect that cannot - // currently fire. + it('[#14547, was divergence] an org-scoped rule DOES see an env-wide (null-org) unit — as approvals does (#3807)', async () => { + // The former `[divergence]` pin, flipped. Same inputs #3807 fixed on the + // approvals side; sharing now reads a null-org unit the same way, so the + // unit resolves and its ORG-STAMPED members are granted. The unit rows + // stay seeded (null-org) — that is the whole point — and only the + // membership rows carry the rule's organization, which is exactly what the + // reported reproduction had (units from app seed data, memberships POSTed + // through the REST data API). + const members: MemberRow[] = SEEDED_MEMBERS.map((m) => ({ ...m, organization_id: 'org_a' })); const g = new BusinessUnitGraphService({ - engine: makeEngine(SEEDED_UNITS, SEEDED_MEMBERS), + engine: makeEngine(SEEDED_UNITS, members), + organizationId: 'org_a', + }); + expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']); + }); +}); + +/** + * [#14547] The pair of screens, asserted as a pair. + * + * Every case below runs the SAME seeded (null-org) unit tree, because that is + * the tree the reproduction had and the one the unit screen was widened for. + * What varies is the MEMBER rows' organization, which is the only tenancy fact + * a membership carries — so each assertion isolates the member screen from the + * unit screen instead of moving both at once. + */ +describe('BusinessUnitGraphService — the unit screen widened, the member screen did not (#14547)', () => { + /** Two tenants' members inside ONE seeded, org-less unit tree. */ + const MIXED_MEMBERS: MemberRow[] = [ + { business_unit_id: 'bu_root', user_id: 'u_a_root', organization_id: 'org_a' }, + { business_unit_id: 'bu_child', user_id: 'u_a_child', organization_id: 'org_a' }, + { business_unit_id: 'bu_root', user_id: 'u_b_root', organization_id: 'org_b' }, + { business_unit_id: 'bu_child', user_id: 'u_b_child', organization_id: 'org_b' }, + { business_unit_id: 'bu_root', user_id: 'u_unstamped' }, + ]; + + const graph = (organizationId: string | null, members: MemberRow[] = MIXED_MEMBERS) => + new BusinessUnitGraphService({ engine: makeEngine(SEEDED_UNITS, members), organizationId }); + + it('(a) an org-NULL unit is USABLE for an org-stamped rule — both widths', async () => { + // The defect verbatim: `seedIsUsable` read the seeded unit as "does not + // exist", so both widths returned zero users and the rule granted nobody. + expect((await graph('org_a').expandUsers('bu_root')).sort()).toEqual(['u_a_child', 'u_a_root']); + expect(await graph('org_a').expandUnitMembers('bu_root')).toEqual(['u_a_root']); + }); + + it('(b) members of ANOTHER organization are never expanded — both widths', async () => { + // The leak that widening the unit screen alone would have opened: one + // seeded unit id, reachable from every tenant, over an unscoped member + // query. Asserted on the WIDE width too — it is the recipient kind the + // reported reproduction used. + const wide = await graph('org_a').expandUsers('bu_root'); + expect(wide).not.toContain('u_b_root'); + expect(wide).not.toContain('u_b_child'); + const narrow = await graph('org_a').expandUnitMembers('bu_root'); + expect(narrow).not.toContain('u_b_root'); + // …and the mirror image, so neither answer is right by accident. + expect((await graph('org_b').expandUsers('bu_root')).sort()).toEqual(['u_b_child', 'u_b_root']); + }); + + it('(c) a NULL-org member row is NOT a member of an org-stamped rule', async () => { + // Measured on this tree: seed replay (`seed-loader.ts` withholds its + // single-org fallback from every `sys_` object) and elevated system writes + // (`sys_business_unit_member` is `unclassified` in + // `PLATFORM_OBJECT_TENANCY`) both leave the column NULL. So NULL here is + // UNKNOWN TENANCY, not "platform-global", and a grant fails closed on it. + expect(await graph('org_a').expandUsers('bu_root')).not.toContain('u_unstamped'); + expect(await graph('org_b').expandUsers('bu_root')).not.toContain('u_unstamped'); + }); + + it('(c control) an org-LESS rule still expands every member, stamped or not', async () => { + // The dominant path in practice, and the one that must not move: with no + // organization on the rule there is nothing to screen against, so the + // member read stays exactly as unscoped as it was before #14547. + expect((await graph(null).expandUsers('bu_root')).sort()).toEqual( + ['u_a_child', 'u_a_root', 'u_b_child', 'u_b_root', 'u_unstamped'], + ); + }); + + it('an org-stamped rule still never reaches ANOTHER org’s unit', async () => { + // The widening admits NULL, and only NULL. A unit belonging to org_b is + // still invisible to an org_a rule — the null-inclusive screen must not be + // read as "no screen". + const units: UnitRow[] = [{ id: 'bu_root', organization_id: 'org_b', active: true }]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(units, [{ business_unit_id: 'bu_root', user_id: 'u_b', organization_id: 'org_b' }]), + organizationId: 'org_a', + }); + expect(await g.expandUsers('bu_root')).toEqual([]); + expect(await g.expandUnitMembers('bu_root')).toEqual([]); + }); + + it('the descent into a seeded subtree is null-inclusive too, not just the seed', async () => { + // `descendants()` runs `orgScope` on the CHILD query as well. A screen + // applied to the seed alone would find the root and then walk into an + // empty child set — the same zero-grant, one query later. + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: 'org_a', active: true }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: null, active: true }, + ]; + const members: MemberRow[] = [ + { business_unit_id: 'bu_child', user_id: 'u_child', organization_id: 'org_a' }, + ]; + const g = new BusinessUnitGraphService({ engine: makeEngine(units, members), organizationId: 'org_a' }); + expect(await g.expandUsers('bu_root')).toEqual(['u_child']); + }); + + it('an inactive seeded unit still contributes nobody', async () => { + // The widening is about TENANCY only. `active: false` remains a hard stop, + // on the seed and on the descent. + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: null, active: false }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: null, active: true }, + ]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(units, MIXED_MEMBERS), organizationId: 'org_a', }); expect(await g.expandUsers('bu_root')).toEqual([]); + expect(await g.expandUnitMembers('bu_root')).toEqual([]); }); }); diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index b7ec0c9499..770a13fd4e 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -170,7 +170,8 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { let rows: any[] = []; try { rows = await this.engine.find('sys_business_unit_member', { - where: { business_unit_id: businessUnitId }, + // [#14547] Screened — see {@link memberScope}. + where: this.memberScope({ business_unit_id: businessUnitId }), fields: ['user_id'], limit: 10000, context: SYSTEM_CTX, @@ -196,7 +197,10 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { let rows: any[] = []; try { rows = await this.engine.find('sys_business_unit_member', { - where: { business_unit_id: { $in: units } }, + // [#14547] Screened — see {@link memberScope}. The WIDE width needs it + // exactly as much as the narrow one: `unit_and_subordinates` is the + // recipient kind the reported repro used. + where: this.memberScope({ business_unit_id: { $in: units } }), fields: ['user_id'], limit: 10000, context: SYSTEM_CTX, @@ -266,8 +270,102 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { } } + /** + * [#14547] The UNIT screen — the platform's own NULL-INCLUSIVE tenant + * predicate, not a strict equality. + * + * `SqlDriver.applyTenantScope` — the platform's one chokepoint for tenant + * scoping on a read — emits `(organization_id = ? OR organization_id IS + * NULL)`, because a NULL organization marks a PLATFORM/seeded row that every + * tenant may see (#2734). This method used to AND a bare + * `organization_id = ` instead, dropping that NULL arm. A + * `sys_business_unit` row written by seed data carries no organization — a + * seed cannot know the id the runtime mints at boot — so an org-stamped rule + * naming a seeded unit matched nothing: {@link seedIsUsable} read the unit as + * "does not exist", both widths returned zero users, and the rule stayed + * `active: true` having materialised no `sys_record_share` row and logged + * nothing. Silent under-grant, and the symptom operators see is "the right + * people cannot see the record". + * + * The spelling is `$or` rather than `context.tenantId` on purpose. Threading + * a tenant would hand the driver the same predicate, but the graph's reads + * are elevated ({@link SYSTEM_CTX}) precisely so they can see rows no + * recipient could, and the two axes are resolved separately + * (`ObjectQLEngine.buildDriverOptions`); more decisively, `driver-memory` and + * `driver-mongodb` implement NO tenant scoping at all, so a screen that + * existed only inside the SQL family would be no screen. The predicate is + * written where the decision is, and it is the same one the driver writes. + * + * ⛔ This is NOT the screen the MEMBER rows get — see {@link memberScope}, + * which is strict on purpose. The asymmetry is the point of #14547 and both + * halves are pinned in `business-unit-graph.test.ts`. + */ private orgScope(filter: Record): Record { - if (this.organizationId) return { ...filter, organization_id: this.organizationId }; - return filter; + if (!this.organizationId) return filter; + return { + ...filter, + $or: [{ organization_id: this.organizationId }, { organization_id: null }], + }; + } + + /** + * [#14547] The MEMBER screen — STRICT equality, deliberately not + * {@link orgScope}. + * + * ## Why the member rows are screened at all + * + * Both member reads used to carry no organization predicate whatever, under + * a {@link SYSTEM_CTX} that carries no tenant either — so the query was + * unscoped by organization. That was invisible only because the strict + * equality {@link orgScope} used to apply kept an org-stamped rule from ever + * reaching a seeded unit. Widening the unit screen without this one would + * have turned a silent UNDER-grant into a silent CROSS-TENANT OVER-grant: a + * seeded unit id exists identically in every tenant, so tenant A's rule would + * expand to tenant B's members. A screen removed from the only place it was + * being enforced is not a fix. + * + * ## Why STRICT, when the unit screen is null-inclusive + * + * The two rows answer different questions. The unit is the ANCHOR the rule's + * author named by id, and a NULL organization on it is the documented + * platform/seeded class. A member row is part of the SET BEING GRANTED, + * enumerated by the platform rather than named by anyone, and its + * organization is the only tenancy fact it carries. + * + * `sys_business_unit_member` rows are NOT organization-stamped on every write + * path (measured on this tree for #14547): + * + * - REST / session writes ARE stamped — the engine threads the caller's + * `tenantId` into `DriverOptions` and the SQL driver's + * `injectTenantOnInsert` fills the injected `organization_id` column; + * - SEED replay is NOT — `seed-loader.ts` withholds its single-org + * `fallbackOrgId` from every `sys_` / `cloud_` / `ai_` object, so a + * seeded membership lands org-less unless the replay pinned an + * organization or the record spelled the column itself; + * - ELEVATED (system-context) writes are NOT — `sys_business_unit_member` + * is `unclassified` in `PLATFORM_OBJECT_TENANCY` + * (`packages/objectql/src/tenancy/platform-object-tenancy.ts`), so + * `Engine.resolveSystemInsertOrganization` returns early and stamps + * nothing; + * - `driver-memory` / `driver-mongodb` never stamp a tenant column at all + * (both refuse to boot multi-tenant, which is what makes that safe). + * + * So a NULL organization on a member row does not mean "platform-global", it + * means UNKNOWN TENANCY — and admitting an identity of unknown tenancy into + * an org-stamped grant is the cross-tenant over-grant above, arriving by the + * other door. A grant fails CLOSED: unknown tenancy is not a member here. + * + * ⚠️ The cost is declared, not hidden: a rule stamped with an organization + * whose unit AND memberships were both seeded still expands to nobody. That + * outcome is now LOUD — `SharingRuleService.expandRecipient` warns once per + * rule naming the rule and the unit — where before it was silent, and the + * repair is to stamp the membership rows rather than to widen this screen. + * + * ⛔ Do not "unify" this with {@link orgScope}. One method for both screens + * re-opens whichever half it does not implement. + */ + private memberScope(filter: Record): Record { + if (!this.organizationId) return filter; + return { ...filter, organization_id: this.organizationId }; } } diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 80028917ab..13b26f833e 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -134,6 +134,13 @@ export class SharingRuleService implements ISharingRuleService { * matches NOTHING); only the repetition is gone. */ private readonly inertRuleSeen = new Set(); + /** + * [#14547] Business-unit rules seen expanding to NOBODY this process, for the + * same once-per-rule dedup {@link inertRuleSeen} carries and for the same + * reason: the reconcilers call `expandRecipient` on every matched write, so + * an undeduped warn would let one misconfigured rule dominate the log. + */ + private readonly emptyUnitExpansionSeen = new Set(); constructor(opts: SharingRuleServiceOptions) { this.engine = opts.engine; @@ -1082,7 +1089,9 @@ export class SharingRuleService implements ISharingRuleService { organizationId: rule.organization_id ?? null, teamGraph: team, }); - return dept.expandUnitMembers(rule.recipient_id); + const members = await dept.expandUnitMembers(rule.recipient_id); + this.warnOnEmptyUnitExpansion(rule, members); + return members; } if (rule.recipient_type === 'position') { // [#8710] A DEACTIVATED position confers NOTHING — checked before the @@ -1115,12 +1124,63 @@ export class SharingRuleService implements ISharingRuleService { organizationId: rule.organization_id ?? null, teamGraph: team, }); - return dept.expandUsers(rule.recipient_id); + const members = await dept.expandUsers(rule.recipient_id); + this.warnOnEmptyUnitExpansion(rule, members); + return members; } // queue — v1 stores literal; treat as no-op until queue impl lands. return []; } + /** + * [#14547] An ACTIVE business-unit rule that expands to NOBODY says so. + * + * This is the half of #14547 that is independent of any screen: the reported + * failure was not merely that the expansion was empty, it was that nothing + * anywhere recorded it. The rule was accepted (201), stayed `active: true`, + * materialised zero `sys_record_share` rows and logged nothing, so the only + * observable was "the right people cannot see the record" — arbitrarily far + * from the cause, and indistinguishable from a criteria mistake, a + * permission-set mistake or a UI bug. + * + * Both business-unit recipient kinds route here, and only they: a rule whose + * recipient is a `user` cannot be empty, and `team` / `position` / `queue` + * have their own reasons for an empty set that this issue did not measure. + * ⛔ Do not widen it into "warn whenever any recipient expands to zero" + * without measuring those — `queue` expands to `[]` by construction today, + * so a blanket warn would fire on every pass of every queue rule. + * + * Named loudly and completely: the rule, the recipient kind, the unit id and + * the two causes worth checking first. Once per rule per process + * ({@link emptyUnitExpansionSeen}). + */ + private warnOnEmptyUnitExpansion(rule: SharingRuleRow, users: readonly string[]): void { + if (users.length > 0) return; + if (rule.active === false) return; + const key = `${String(rule.id ?? rule.name)}::${String(rule.recipient_id ?? '')}`; + if (this.emptyUnitExpansionSeen.has(key)) return; + this.emptyUnitExpansionSeen.add(key); + this.logger?.warn?.( + '[sharing-rule] active business-unit rule expands to NO recipients — it grants nobody and ' + + 'will materialise no shares (logged once per rule per process). Check that the business ' + + 'unit exists and is active, and that its `sys_business_unit_member` rows carry the same ' + + 'organization_id as the rule — membership rows written by seed replay or by an elevated ' + + 'system write are not organization-stamped, and are not members of an org-scoped rule', + { + rule: rule.name ?? rule.id, + object: rule.object_name, + recipientType: rule.recipient_type, + businessUnit: rule.recipient_id, + organization: rule.organization_id ?? null, + }, + ); + } + + /** Rules seen expanding to no business-unit recipients — for tests and boot reports. */ + get emptyUnitExpansionRuleKeys(): readonly string[] { + return [...this.emptyUnitExpansionSeen]; + } + /** * [#8710] Does `positionName` still CONFER access in this rule's * organization? Memoised for the pass; the extra read is accepted. diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index ca1216c836..cbfc923b31 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -170,11 +170,15 @@ describe('BusinessUnitGraphService (recursive sys_business_unit)', () => { // Foreign tenant — must not leak { id: 'foreign', name: 'Foreign', parent_business_unit_id: 'emea', organization_id: 'org2', active: true }, ]; + // [#14547] Membership rows carry the tenant too. They used to be org-less + // here and still expanded, because the member read had no organization + // predicate at all — the gap #14547 closed. These units are org1's, written + // by org1's API calls, so their memberships are org1's. engine._tables.sys_business_unit_member = [ - { id: 'dm1', business_unit_id: 'emea_sales_uk', user_id: 'alice' }, - { id: 'dm2', business_unit_id: 'emea_sales', user_id: 'bob' }, - { id: 'dm3', business_unit_id: 'emea_marketing', user_id: 'carol' }, - { id: 'dm4', business_unit_id: 'emea_legacy', user_id: 'ghost' }, + { id: 'dm1', business_unit_id: 'emea_sales_uk', user_id: 'alice', organization_id: 'org1' }, + { id: 'dm2', business_unit_id: 'emea_sales', user_id: 'bob', organization_id: 'org1' }, + { id: 'dm3', business_unit_id: 'emea_marketing', user_id: 'carol', organization_id: 'org1' }, + { id: 'dm4', business_unit_id: 'emea_legacy', user_id: 'ghost', organization_id: 'org1' }, ]; }); @@ -247,9 +251,10 @@ describe('SharingRuleService', () => { { id: 'emea_sales', name: 'EMEA Sales', parent_business_unit_id: null, organization_id: 'org1', active: true }, { id: 'emea_sales_uk', name: 'EMEA Sales UK', parent_business_unit_id: 'emea_sales', organization_id: 'org1', active: true }, ]; + // [#14547] Org-stamped for the same reason as the fixture above. engine._tables.sys_business_unit_member = [ - { id: 'dm1', business_unit_id: 'emea_sales', user_id: 'alice' }, - { id: 'dm2', business_unit_id: 'emea_sales_uk', user_id: 'bob' }, + { id: 'dm1', business_unit_id: 'emea_sales', user_id: 'alice', organization_id: 'org1' }, + { id: 'dm2', business_unit_id: 'emea_sales_uk', user_id: 'bob', organization_id: 'org1' }, ]; sharing = new SharingService({ engine: engine as any }); rules = new SharingRuleService({ engine: engine as any, sharing }); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 3ca0671909..f9329273ac 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2696,6 +2696,16 @@ "verb": "delete", "pinned": 1 }, + { + "file": "packages/plugins/plugin-sharing/src/bu-rule-tenant-screen.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-sharing/src/bu-rule-tenant-screen.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts", "verb": "delete",