diff --git a/.changeset/bu-tenant-screen-relanding.md b/.changeset/bu-tenant-screen-relanding.md new file mode 100644 index 0000000000..fd084e945e --- /dev/null +++ b/.changeset/bu-tenant-screen-relanding.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-sharing': patch +--- + +Fix: a sharing rule with a business-unit recipient granted nothing when the unit came from seed data — and tenant-screen the member reads that widening exposes. + +`BusinessUnitGraphService.orgScope` screened `sys_business_unit` with a strict `organization_id` equality, while the platform's own read-side chokepoint (`SqlDriver.applyTenantScope`) is null-inclusive: `(organization_id = ? OR organization_id IS NULL)`, because a NULL organization marks a platform/seeded row every tenant may see. 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, the same predicate `plugin-approvals` already applies to these very rows and `SharingRuleService.adminOrgScope` applies to the rule table. + +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 either, so 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. The sibling recipient widths already read their membership rows this way. + +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 membership rows 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 2e27ae7abd..6bb46d1afb 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:449`, `:503`, `:507`, `:580`, `:610` | | 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:165`, `:390` | ### 4. Approvals, reports, attachments, comments, knowledge 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..e52d156717 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts @@ -1,35 +1,45 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * BusinessUnitGraphService — org scoping of the unit tree. + * BusinessUnitGraphService — the TWO tenant screens, pinned as a pair. * - * These pin the ORG-SCOPE behaviour specifically, because it is the exact - * shape that broke approvals in #3807: `orgScope()` AND-composes a strict - * `organization_id = ` equality, so a unit written with no - * organization at all (a seeded / file-layer / bootstrap row — a seed cannot - * know the org id the runtime mints at boot) matches nothing, the seed check - * fails, and the expansion returns zero members. In approvals that produced a - * dead `department:` approver slot; here it would produce a sharing rule - * that silently grants nobody. + * ## What this file used to say, and why it changed * - * 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. + * Until #14547 `orgScope()` AND-composed a strict `organization_id = ` equality onto the UNIT read. A unit written with no organization at + * all (a seeded / file-layer / bootstrap row — a seed cannot know the org id + * the runtime mints at boot) therefore matched nothing, the seed check failed, + * and BOTH widths expanded to zero members. #3807 had already fixed exactly + * that on the approvals side; this file recorded the sharing side's divergence + * as deliberate on the grounds that it was unreachable, because every + * materialized `sys_sharing_rule` row carried `organization_id = null`. * - * 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. + * It was reachable. #14547 is the external report: an org admin creating a + * rule at runtime gets an org-stamped rule, the seeded unit carries none, and + * the rule is accepted, stays active, materialises zero `sys_record_share` + * rows and logs nothing. The `[divergence]` test that pinned the old posture + * is gone — replaced, not merely flipped, because an assertion that keeps + * passing while the mechanism under it changes is worse than no assertion. * - * 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 pair this file now pins + * + * The fix is ASYMMETRIC and both halves have to be pinned, because each one + * alone is a defect: + * + * - the UNIT screen (`orgScope`) is NULL-INCLUSIVE — the platform's own + * `(organization_id = ? OR organization_id IS NULL)`, the predicate + * `SqlDriver.applyTenantScope` writes and `plugin-approvals` already + * applies to these very rows; + * - the MEMBER screen (`memberScope`) is STRICT. Both member reads used to + * carry no organization predicate at all, and the strict unit screen was + * the only thing holding an org-stamped rule away from that unscoped + * query. Widening the unit screen ALONE turns a silent under-grant into a + * silent CROSS-TENANT OVER-GRANT, since a seeded unit id exists + * identically in every tenant. + * + * So the security half is pinned separately from the functional half below: a + * change that expands the right members while also expanding another + * organization's members satisfies the functional pin completely. */ import { describe, it, expect } from 'vitest'; @@ -40,8 +50,9 @@ interface UnitRow { parent_business_unit_id?: string | null; organization_id?: string | null; active?: boolean; + manager_user_id?: string | null; } -interface MemberRow { business_unit_id: string; user_id: string } +interface MemberRow { business_unit_id: string; user_id: string; organization_id?: string | null } /** * Minimal engine over `sys_business_unit` + `sys_business_unit_member`. @@ -168,13 +179,33 @@ describe('BusinessUnitGraphService — the two widths are actually two widths (# }); it('the narrow width is org-predicated exactly like the wide one', async () => { + // [#14547] Same fixture, new mechanism — and the mechanism is spelled out + // because the ASSERTION did not move. `DIV_MEMBERS` carry no organization, + // so before #14547 this returned `[]` because the strict UNIT screen hid + // the seeded unit, and after it returns `[]` because the strict MEMBER + // screen refuses membership rows of unknown tenancy. An unchanged + // expectation over a changed cause is exactly the kind of pin that stops + // guarding anything, so the two causes are separated below: the unit is + // now visible (`descendants` sees the whole seeded tree), and it is the + // members that are refused. 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. expect(await g.expandUnitMembers('bu_div')).toEqual([]); + expect((await g.descendants('bu_div')).sort()).toEqual(['bu_dept', 'bu_div', 'bu_office']); + }); + + it('[#14547] both widths reach org-stamped members of a SEEDED unit tree', async () => { + // The one change that flips the outcome: the membership rows are stamped, + // exactly as a REST/session write stamps them. The units stay seeded. + const members: MemberRow[] = DIV_MEMBERS.map((m) => ({ ...m, organization_id: 'org_a' })); + const g = new BusinessUnitGraphService({ + engine: makeEngine(DIV_UNITS, members), + organizationId: 'org_a', + }); + expect(await g.expandUnitMembers('bu_div')).toEqual(['u_div']); + expect((await g.expandUsers('bu_div')).sort()).toEqual(['u_dept', 'u_div', 'u_office']); }); it('the two widths do NOT share a cache entry for the same unit id', async () => { @@ -208,8 +239,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 membership rows are stamped now. They used to be org-less + // here and still expanded, because the member read carried no organization + // predicate whatever — the gap #14547 closed. Units created through the + // API by org_a have memberships created the same way, so this is the + // fixture becoming faithful, not the assertion being relaxed. + 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 +261,171 @@ 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('an org-scoped rule never reaches another org’s MEMBER of a unit it can see', async () => { + // [#14547] The unit is org_a's and visible; the membership row is org_b's. + // The member screen is the only thing that answers here, so this fails if + // `memberScope` is dropped even while every unit-level assertion passes. + const units: UnitRow[] = [{ id: 'bu_root', organization_id: 'org_a', active: true }]; + const members: MemberRow[] = [ + { business_unit_id: 'bu_root', user_id: 'u_a', organization_id: 'org_a' }, + { business_unit_id: 'bu_root', user_id: 'u_b', organization_id: 'org_b' }, + ]; const g = new BusinessUnitGraphService({ - engine: makeEngine(SEEDED_UNITS, SEEDED_MEMBERS), + engine: makeEngine(units, members), + organizationId: 'org_a', + }); + expect(await g.expandUsers('bu_root')).toEqual(['u_a']); + expect(await g.expandUnitMembers('bu_root')).toEqual(['u_a']); + }); +}); + +/** + * [#14547] The UNIT screen is null-inclusive — the divergence from + * `plugin-approvals` (#3807) is CLOSED. + * + * The `[divergence]` test that used to live in the block above pinned the + * opposite posture on the grounds that it could not fire. It fired: the + * external report is an org admin creating a rule at runtime against a unit + * the app seeded. + */ +describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => { + const STAMPED_MEMBERS: MemberRow[] = SEEDED_MEMBERS.map((m) => ({ + ...m, + organization_id: 'org_a', + })); + + it('an org-scoped rule DOES see an env-wide (null-org) seeded unit', async () => { + const g = new BusinessUnitGraphService({ + engine: makeEngine(SEEDED_UNITS, STAMPED_MEMBERS), + organizationId: 'org_a', + }); + expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']); + expect(await g.expandUnitMembers('bu_root')).toEqual(['u_root']); + }); + + it('the seed check and the subtree walk BOTH admit the seeded rows', async () => { + // `seedIsUsable` and the `descendants` BFS are two separate reads through + // the same screen; a widening applied to one and not the other would still + // answer `[]` for the subtree width. + const g = new BusinessUnitGraphService({ + engine: makeEngine(SEEDED_UNITS, STAMPED_MEMBERS), + organizationId: 'org_a', + }); + expect((await g.descendants('bu_root')).sort()).toEqual(['bu_child', 'bu_root']); + }); + + it('`headOf` resolves the manager of a seeded unit too', async () => { + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: null, active: true, manager_user_id: 'u_head' }, + ]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(units, []), + organizationId: 'org_a', + }); + expect(await g.headOf('bu_root')).toBe('u_head'); + }); + + it('ONLY the NULL arm widened — another org’s unit is still invisible', async () => { + // The control that separates "null-inclusive" from "unscoped". Without it + // a screen that had simply been deleted would pass every assertion above. + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: 'org_b', active: true }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_b', active: true }, + ]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(units, STAMPED_MEMBERS), organizationId: 'org_a', }); expect(await g.expandUsers('bu_root')).toEqual([]); + expect(await g.expandUnitMembers('bu_root')).toEqual([]); + expect(await g.descendants('bu_root')).toEqual([]); + expect(await g.headOf('bu_root')).toBeNull(); + }); + + it('an INACTIVE seeded unit still contributes nobody', async () => { + const units: UnitRow[] = SEEDED_UNITS.map((u) => + u.id === 'bu_root' ? { ...u, active: false } : u, + ); + const g = new BusinessUnitGraphService({ + engine: makeEngine(units, STAMPED_MEMBERS), + organizationId: 'org_a', + }); + expect(await g.expandUsers('bu_root')).toEqual([]); + expect(await g.expandUnitMembers('bu_root')).toEqual([]); + }); +}); + +/** + * [#14547] The MEMBER screen is STRICT — the leak the unit widening would + * otherwise have opened. + * + * ⚠️ These are the SECURITY half and they are pinned apart from the functional + * half on purpose: a change that expands the right members while also + * expanding another organization's members passes every assertion in the block + * above. + */ +describe('BusinessUnitGraphService — the MEMBER screen (#14547)', () => { + /** + * One SEEDED unit id with two tenants' memberships hanging off it — the + * shape that exists on every deployment whose org chart came from a seed, + * and the one the widened unit screen makes reachable. + */ + const SHARED_SEED_UNITS: UnitRow[] = [ + { id: 'bu_market', organization_id: null, active: true }, + { id: 'bu_market_west', parent_business_unit_id: 'bu_market', organization_id: null, active: true }, + ]; + const TWO_TENANT_MEMBERS: MemberRow[] = [ + { business_unit_id: 'bu_market', user_id: 'u_a', organization_id: 'org_a' }, + { business_unit_id: 'bu_market', user_id: 'u_b', organization_id: 'org_b' }, + { business_unit_id: 'bu_market_west', user_id: 'u_a_west', organization_id: 'org_a' }, + { business_unit_id: 'bu_market_west', user_id: 'u_b_west', organization_id: 'org_b' }, + ]; + + it('WIDE — a subtree expansion never crosses into another organization', async () => { + const g = new BusinessUnitGraphService({ + engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS), + organizationId: 'org_a', + }); + const users = await g.expandUsers('bu_market'); + expect(users.sort()).toEqual(['u_a', 'u_a_west']); + expect(users).not.toContain('u_b'); + expect(users).not.toContain('u_b_west'); + }); + + it('NARROW — the single-unit expansion does not cross either', async () => { + const g = new BusinessUnitGraphService({ + engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS), + organizationId: 'org_a', + }); + expect(await g.expandUnitMembers('bu_market')).toEqual(['u_a']); + }); + + it('an org-LESS membership row is NOT a member of an org-scoped rule', async () => { + // Unknown tenancy, not platform-global: `sys_business_unit_member` is not + // organization-stamped by seed replay or by an elevated system write, so a + // NULL here cannot be read the way a NULL on the UNIT row is read. The + // grant fails closed, and `SharingRuleService` warns rather than staying + // silent about it. + const members: MemberRow[] = [ + { business_unit_id: 'bu_market', user_id: 'u_seeded', organization_id: null }, + ]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(SHARED_SEED_UNITS, members), + organizationId: 'org_a', + }); + expect(await g.expandUnitMembers('bu_market')).toEqual([]); + expect(await g.expandUsers('bu_market')).toEqual([]); + }); + + it('an org-LESS rule is unmoved — both screens stay no-ops', async () => { + // The dominant shape today (declared rules bootstrap org-less). #14547 + // must not change what they expand to, in either direction. + const g = new BusinessUnitGraphService({ + engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS), + organizationId: null, + }); + expect((await g.expandUsers('bu_market')).sort()).toEqual([ + 'u_a', 'u_a_west', 'u_b', 'u_b_west', + ]); }); }); diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index b7ec0c9499..7d5c5b6afb 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -51,6 +51,16 @@ export interface BusinessUnitGraphOptions { * - {@link BusinessUnitGraphService.expandUnitMembers} — exactly that one * unit's members (drives the `business_unit` recipient). * + * Two DIFFERENT tenant screens live here too, and keeping THEM distinct is + * the point of #14547 — an asymmetric pair, not an oversight: + * - {@link BusinessUnitGraphService.orgScope} screens `sys_business_unit`, + * the ANCHOR the rule names, with the platform's NULL-INCLUSIVE predicate + * (a seeded unit belongs to no organization and every tenant may see it); + * - `memberScope` screens `sys_business_unit_member`, the SET BEING GRANTED, + * with a STRICT equality (a NULL organization there means unknown + * tenancy, and a grant fails closed on it). + * Each method carries the measurement it rests on. ⛔ They are not one method. + * * Reuses {@link TeamGraphService.managerOf} for user-level manager * lookup so callers can use this single service in approval / sharing * pipelines. @@ -170,7 +180,9 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { let rows: any[] = []; try { rows = await this.engine.find('sys_business_unit_member', { - where: { business_unit_id: businessUnitId }, + // [#14547] Tenant-screened — see {@link memberScope}, which is STRICT + // on purpose and is not {@link orgScope}. + where: this.memberScope({ business_unit_id: businessUnitId }), fields: ['user_id'], limit: 10000, context: SYSTEM_CTX, @@ -196,7 +208,12 @@ 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] Tenant-screened — see {@link memberScope}. The WIDE width + // needs the screen exactly as much as the narrow one: the reported + // reproduction used `unit_and_subordinates`, and a subtree walk that + // now admits org-less (seeded) units reaches MORE unscreened member + // rows than the single-unit read, not fewer. + where: this.memberScope({ business_unit_id: { $in: units } }), fields: ['user_id'], limit: 10000, context: SYSTEM_CTX, @@ -266,8 +283,116 @@ 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 single chokepoint for + * read-side tenant isolation — emits `(organization_id = ? OR + * organization_id IS NULL)`, and its own comment names business units among + * the populations that arm depends on: 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 recipient widths returned zero users, and the rule + * stayed `active: true` having materialised no `sys_record_share` row and + * logged nothing. A silent under-grant whose only symptom is "the right + * people cannot see the record". + * + * The same predicate, for the same rows, is already written twice in this + * codebase: `SharingRuleService.adminOrgScope` (#7676) for the rule table, + * and `ApprovalService.businessUnitOrgScope` (#3807) for `sys_business_unit` + * itself. This file was the outlier, and `sharing-rule-service.ts` names the + * mistake in prose while this file made it. + * + * The spelling is `$or` rather than threading `context.tenantId` on purpose. + * A tenant on the context would hand the SQL driver the same predicate, but + * these reads are elevated ({@link SYSTEM_CTX}) precisely so they can see + * rows no recipient could; more decisively, `driver-memory` and + * `driver-mongodb` implement no `applyTenantScope` layer 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 whole point of #14547, + * and both halves are pinned (`business-unit-graph.test.ts`, + * `recipient-width.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 + * completely unscoped by organization. That was survivable 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 ALONE would + * therefore have converted 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 and materialise real + * `sys_record_share` rows for them. The bug was moonlighting as the tenant + * guard, and removing a screen 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 the driver's NULL arm exists for. 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. Re-measured on this tree at `origin/main` for #14547: + * + * - REST / session writes ARE stamped — the engine threads the caller's + * `tenantId` into `DriverOptions` and `SqlDriver.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 absent from `PLATFORM_OBJECT_TENANCY` + * (`packages/objectql/src/tenancy/platform-object-tenancy.ts`), so it + * classifies `unclassified` and `Engine.resolveSystemInsertOrganization` + * returns early, stamping nothing. That residual classification gap is + * tracked separately (#14570) and is not closed here; + * - `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 same cross-tenant over-grant, arriving by the + * other door. A grant fails CLOSED: unknown tenancy is not a member here. + * The sibling recipient widths already read their membership rows this way — + * `TeamGraphService` screens `sys_team_member` and `PositionGraphService` + * screens `sys_user_position`, both with a strict equality. + * + * ⚠️ 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 serving 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/recipient-width.test.ts b/packages/plugins/plugin-sharing/src/recipient-width.test.ts index 419f1765b4..7eeb4752af 100644 --- a/packages/plugins/plugin-sharing/src/recipient-width.test.ts +++ b/packages/plugins/plugin-sharing/src/recipient-width.test.ts @@ -34,7 +34,7 @@ * grandchild. */ -import { describe, it, expect, beforeEach } from 'vitest'; +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'; @@ -263,3 +263,240 @@ describe('#7807 recipient width — business_unit vs unit_and_subordinates', () }); }); }); + +/** + * [#14547] The same two widths, now against a SEEDED business unit — driven + * end to end so the observables the report named are the observables asserted. + * + * `business-unit-graph.test.ts` pins the two screens at the graph service. + * This block 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 + * (201), it stayed `active: true`, it materialised zero grants, and nothing + * was logged. Asserting the graph's return value alone leaves every one of + * those facts unpinned. + * + * The fixture is the reported reproduction's shape, not an invented one, and + * its three rows carry three DIFFERENT tenancy facts — which is the whole + * reason the defect existed: + * + * - `sys_business_unit` comes from app SEED data and carries + * `organization_id = NULL` — a seed cannot know the id the runtime mints + * at boot; + * - `sys_business_unit_member` is POSTed through the REST data API and IS + * organization-stamped (the engine threads the caller's tenant and + * `SqlDriver.injectTenantOnInsert` fills the injected column); + * - `sys_sharing_rule` is created by an organization admin and is + * org-stamped too (an explicit `organization_id: null` in the payload is + * overridden). + * + * ⚠️ The security half lives in its own `describe` below and is NOT a + * corollary of the functional half: a change that expands the right members + * while also expanding another organization's members passes every functional + * assertion here. + */ +describe('#14547 — an org-stamped rule against a SEEDED business unit', () => { + let engine: ReturnType; + let rules: SharingRuleService; + let warn: ReturnType; + + const ORG_A = 'org_a'; + const ORG_B = 'org_b'; + const RULE = 'kpi_sheet_to_market_unit'; + + /** 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 empty-expansion warn calls this run emitted. */ + const emptyWarns = (): any[][] => + warn.mock.calls.filter((c) => String(c[0]).includes('expands to NO recipients')); + + /** 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', + }]); + }; + + beforeEach(() => { + engine = makeEngine(); + warn = vi.fn(); + const sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing, logger: { warn } as any }); + + // Seed data: units written before any organization existed. The column is + // spelled explicitly — a NULL column, not an absent key, is what the + // widened `$or` arm has to match. + 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' }]); + }); + + 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(emptyWarns()).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 unit widening would otherwise have opened', () => { + /** + * ONE seeded unit id, two tenants' memberships hanging off it — the shape + * that exists on any deployment whose org chart came from a seed, and the + * one the widened unit screen makes reachable for the first time. + */ + const twoTenantMembers = () => [ + { 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 }, + { id: 'bum_aw', business_unit_id: 'bu_market_west', user_id: 'u_a_west', organization_id: ORG_A }, + { id: 'bum_bw', business_unit_id: 'bu_market_west', user_id: 'u_b_west', organization_id: ORG_B }, + ]; + + it('WIDE — no `sys_record_share` row is ever materialised for another org’s member', async () => { + engine.seed('sys_business_unit_member', twoTenantMembers()); + seedRule('unit_and_subordinates', ORG_A); + await rules.evaluateRule(RULE, SYS); + expect(granteesOf('kpi_1')).toEqual(['u_a', 'u_a_west']); + expect(granteesOf('kpi_1')).not.toContain('u_b'); + expect(granteesOf('kpi_1')).not.toContain('u_b_west'); + }); + + it('NARROW — the single-unit width does not cross either', async () => { + engine.seed('sys_business_unit_member', twoTenantMembers()); + seedRule('business_unit', ORG_A); + await rules.evaluateRule(RULE, SYS); + expect(granteesOf('kpi_1')).toEqual(['u_a']); + }); + + it('an org-LESS membership row is not admitted to an org-stamped rule', async () => { + // Unknown tenancy, not platform-global: `sys_business_unit_member` is + // NOT organization-stamped by seed replay or by an elevated system + // write, so a NULL here cannot be read the way a NULL on the UNIT row + // is read. The grant fails closed. + engine.seed('sys_business_unit_member', [ + { id: 'bum_ok', business_unit_id: 'bu_market', user_id: 'u_ok', organization_id: ORG_A }, + { id: 'bum_seeded', business_unit_id: 'bu_market', user_id: 'u_seeded', organization_id: null }, + ]); + seedRule('unit_and_subordinates', ORG_A); + await rules.evaluateRule(RULE, SYS); + expect(granteesOf('kpi_1')).toEqual(['u_ok']); + }); + }); + + describe('an active rule that grants nobody is LOUD', () => { + it('warns naming the rule, the object, the recipient kind, the unit and the org', async () => { + // Unit AND memberships both seeded: the unit resolves now, but org-less + // membership rows are of unknown tenancy. This residual empty expansion + // is the case the warn exists for — it used to be completely silent. + engine.seed('sys_business_unit_member', [ + { id: 'bum_seeded', business_unit_id: 'bu_market', user_id: 'u_seeded', organization_id: null }, + ]); + seedRule('unit_and_subordinates'); + await rules.evaluateRule(RULE, SYS); + + expect(granteesOf('kpi_1')).toEqual([]); + const calls = emptyWarns(); + 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(emptyWarns()).toHaveLength(1); + expect(emptyWarns()[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(emptyWarns()).toHaveLength(1); + }); + + 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(emptyWarns()).toHaveLength(0); + }); + + it('an INACTIVE rule is not warned about — it is meant to grant nobody', async () => { + // `evaluateRule` short-circuits an inactive rule before `expandRecipient` + // runs at all; the guard inside the warn covers the reconciler paths that + // reach the expansion directly. Both roads lead here, so this asserts the + // observable rather than which of the two answered. + 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(emptyWarns()).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 () => { + // A platform-global rule threads no organization, so BOTH screens are + // no-ops for it, exactly as before #14547. Pinned in both directions so + // the change cannot silently retire the declared cross-tenant behaviour + // of a null-org rule. + 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: null }, + ]); + 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/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 42a94b415a..d65eeaa8fe 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -134,6 +134,14 @@ 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 — the + * same once-per-rule dedup {@link inertRuleSeen} carries, for the same + * reason: the reconcilers call {@link expandRecipient} on every matched + * write, so an undeduped warn would let one misconfigured rule dominate a + * deployment's log. + */ + private readonly emptyUnitExpansionSeen = new Set(); constructor(opts: SharingRuleServiceOptions) { this.engine = opts.engine; @@ -1092,7 +1100,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 @@ -1125,12 +1135,65 @@ 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. + * + * It also covers the case the screens deliberately leave empty: a rule + * stamped with an organization whose unit AND membership rows were both + * seeded expands to nobody, because an org-less membership row is of unknown + * tenancy and is not a member of an org-scoped rule + * ({@link BusinessUnitGraphService} → `memberScope`). Failing closed is the + * right answer; failing closed in silence is not. + * + * Both business-unit recipient kinds route here, and only they: a `user` + * recipient cannot be empty, and `team` / `position` / `queue` have their own + * reasons for an empty set that this issue did not measure. ⛔ Do not widen + * this 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 completely: the rule, the object, the recipient kind, the unit, the + * organization, 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, + }, + ); + } + /** * [#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..22d33a9ad0 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -170,11 +170,16 @@ 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] The membership rows carry the tenant too. They used to be + // org-less here and still expanded, because the member read carried no + // organization predicate at all — the gap #14547 closed. These units are + // org1's, created by org1's API calls, so their memberships are org1's: + // the fixture is becoming faithful, not the assertions being relaxed. 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 +252,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 });