diff --git a/.changeset/bu-tenant-screen-relanding.md b/.changeset/bu-tenant-screen-relanding.md index fd084e945e..bc68bc3425 100644 --- a/.changeset/bu-tenant-screen-relanding.md +++ b/.changeset/bu-tenant-screen-relanding.md @@ -2,10 +2,10 @@ '@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. +Tenant-screen the `sys_business_unit_member` reads, and warn when an active business-unit rule grants nobody. -`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. +⚠️ This entry originally also announced that `BusinessUnitGraphService.orgScope` had been widened to the platform's null-inclusive `(organization_id = ? OR organization_id IS NULL)` screen, fixing the case where a sharing rule naming a **seed-written** business unit granted nobody. That half was **reverted before 17.3 was cut** and does not ship: it re-implemented, a second time and in a second place, the predicate `SqlDriver.applyTenantScope` already owns — the duplication the v18 org-ownership decision (PR #14976) exists to retire — and it had not been released. `orgScope` keeps the strict `organization_id` equality 17.2.0 ships, so a rule naming a seeded unit still expands to nobody. That defect is fixed structurally on the v18 line by the v18 org-ownership decision (PR #14976), C1 (the Default Organization exists before application seed datasets load, and the seed loader stamps `sys_business_unit` seeds). What follows is the part of this change that DOES ship. -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. +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. This matters independently of the unit screen: other organizations' member rows sit on org-stamped — visible — units too, so a strict unit screen narrows which units are reachable but does not close the unscoped member read. 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. +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 is what keeps the remaining seed-data symptom observable at the moment it happens, rather than surfacing only as "the right people cannot see the record". diff --git a/.changeset/revert-null-inclusive-business-unit-screen.md b/.changeset/revert-null-inclusive-business-unit-screen.md new file mode 100644 index 0000000000..bd7a8219af --- /dev/null +++ b/.changeset/revert-null-inclusive-business-unit-screen.md @@ -0,0 +1,30 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +revert(plugin-sharing): drop the NULL-inclusive business-unit screen added after 17.2.0 + +17.3 does not ship the NULL-inclusive business-unit screen added after 17.2.0; +#14547 remains as in 17.2.0 and is fixed structurally in v18 (the v18 org-ownership decision (PR #14976), C1: the +Default Organization exists before application seed datasets load, and the seed +loader stamps `sys_business_unit` seeds). + +`BusinessUnitGraphService.orgScope` briefly read +`$or: [{ organization_id: }, { organization_id: null }]` so that an +org-stamped sharing rule could name a seeded (org-less) `sys_business_unit` +row. It is restored to the strict `organization_id = ` equality +17.2.0 ships. That shape re-implemented, a second time and in a second place, +the predicate `SqlDriver.applyTenantScope` already owns — the duplication +the v18 org-ownership decision (PR #14976) exists to retire (#10103 cause 1) — and it had not been released, so +reverting costs nothing while shipping it would have owed v18 a breaking change +and a migration. + +The other half of the same change is KEPT and is not touched: +`BusinessUnitGraphService.memberScope` still screens both +`sys_business_unit_member` reads with a strict equality. Those reads previously +carried no organization predicate at all, so an org-stamped rule reaching any +visible unit collected every tenant's membership rows hanging off it; a strict +unit screen narrows which units are reachable but does not close that, because +other organizations' member rows sit on org-stamped units too. +`SharingRuleService.warnOnEmptyUnitExpansion` is also kept: it is what keeps the +remaining #14547 symptom loud instead of silent. 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 e52d156717..2f8514faa6 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts @@ -3,43 +3,51 @@ /** * BusinessUnitGraphService — the TWO tenant screens, pinned as a pair. * - * ## What this file used to say, and why it changed + * ## Both screens are STRICT equalities, and that is the shipped posture * - * 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`. + * - the UNIT screen (`orgScope`) AND-composes `organization_id = ` + * onto the `sys_business_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 matches nothing for an + * org-stamped rule, the seed check fails, and BOTH widths expand to zero + * members; + * - the MEMBER screen (`memberScope`) AND-composes the same equality onto + * the `sys_business_unit_member` reads. Both member reads used to carry no + * organization predicate at all — completely unscoped by organization — + * which is the cross-tenant hole #14949 closed and this file still pins. * - * 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. + * ## #14547's symptom STANDS in 17.x, and these tests reproduce it * - * ## The pair this file now pins + * #14547 is the external report of the unit half: an org admin creating a rule + * at runtime gets an org-stamped rule, the seeded unit carries no + * organization, and the rule is accepted, stays active and materialises zero + * `sys_record_share` rows. #14949 briefly closed it by giving the UNIT screen + * the platform's NULL-inclusive `$or` arm. That was reverted before 17.3 was + * cut (the v18 org-ownership decision (PR #14976), D8): it re-implements the predicate + * `SqlDriver.applyTenantScope` already owns, a second time in a second place — + * the duplication the v18 org-ownership decision (PR #14976) exists to retire — and it had not shipped. * - * The fix is ASYMMETRIC and both halves have to be pinned, because each one - * alone is a defect: + * So 17.3 behaves exactly as 17.2.0 does here, and the cases below name + * **#14547** as the defect they reproduce. ⚠️ #14547 is CLOSED as completed — + * closed by #14949, whose unit half this reverts — so its GitHub state no + * longer matches the 17.x runtime. Whether it is reopened is the maintainer's + * call, not this file's; what the tests assert is the behaviour, which is + * 17.2.0's. It is fixed structurally on + * the v18 line by the v18 org-ownership decision (PR #14976), C1 (the Default Organization exists before + * application seed datasets load, and the seed loader stamps + * `sys_business_unit` seeds), so the row this screen reads carries an + * organization and no screen has to special-case it. What #14949 left behind — + * and what keeps the 17.x symptom LOUD rather than silent — is + * `SharingRuleService.warnOnEmptyUnitExpansion`. * - * - 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. + * ## Why the member screen is pinned on ORG-STAMPED units * - * 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. + * ⚠️ The member pins below deliberately anchor on a unit stamped with the + * rule's own organization. With the unit screen strict, a seeded (org-less) + * unit is invisible to an org-stamped rule, so a member pin written on one + * would pass no matter what `memberScope` did — the unit screen would answer + * first and the assertion would never reach the member read. Anchoring on a + * visible unit is what keeps these assertions about the MEMBER screen. */ import { describe, it, expect } from 'vitest'; @@ -179,33 +187,36 @@ 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. + // [#14547] `DIV_UNITS` are seeded (org-less) and `DIV_MEMBERS` carry no + // organization either, so BOTH strict screens refuse this fixture. The + // unit screen answers FIRST and is the one being pinned here: the subtree + // walk cannot see the seeded tree at all, which is why the expansion is + // empty. The member screen's own refusal is pinned separately, on a unit + // the rule can actually see, in the MEMBER screen block below. const g = new BusinessUnitGraphService({ engine: makeEngine(DIV_UNITS, DIV_MEMBERS), organizationId: 'org_a', }); expect(await g.expandUnitMembers('bu_div')).toEqual([]); - expect((await g.descendants('bu_div')).sort()).toEqual(['bu_dept', 'bu_div', 'bu_office']); + expect(await g.descendants('bu_div')).toEqual([]); }); - 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. + it('[#14547] NEITHER width reaches a SEEDED unit tree, even with stamped members', async () => { + // ⚠️ This reproduces the defect #14547 reports — it does not assert a fix. + // The membership rows are stamped exactly as a REST/session write stamps + // them, so the MEMBER screen would admit every one of them; the units stay + // seeded, so the strict UNIT screen hides the tree before the member read + // is ever issued. Both widths therefore answer nobody, which is precisely + // the 17.2.0 symptom: rule accepted, active, zero shares. + // Fixed structurally in v18 by the v18 org-ownership decision (PR #14976), C1 (seed loader stamps + // `sys_business_unit`), NOT by widening this screen. 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']); + expect(await g.expandUnitMembers('bu_div')).toEqual([]); + expect(await g.expandUsers('bu_div')).toEqual([]); }); it('the two widths do NOT share a cache entry for the same unit id', async () => { @@ -280,13 +291,21 @@ describe('BusinessUnitGraphService — org scoping (#3807)', () => { }); /** - * [#14547] The UNIT screen is null-inclusive — the divergence from - * `plugin-approvals` (#3807) is CLOSED. + * [#14547] The UNIT screen is STRICT — the divergence from `plugin-approvals` + * (#3807) STANDS, and 17.3 ships it exactly as 17.2.0 did. + * + * ⚠️ Every case in this block reproduces the defect **#14547** reports, which + * 17.x still has. None of them asserts a fix. #14949 closed the defect by giving this screen the + * platform's NULL-inclusive arm; that was reverted before the 17.3 tag + * (the v18 org-ownership decision (PR #14976), D8) because it wrote `SqlDriver.applyTenantScope`'s own predicate + * a second time, in a second place, and had not shipped. The structural fix is + * the v18 org-ownership decision (PR #14976), C1 on the v18 line: the Default Organization exists before + * application seed datasets load and the seed loader stamps + * `sys_business_unit` seeds, so these rows arrive already carrying an + * organization and this screen needs no NULL arm to find them. * - * 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. + * ⛔ If a future change makes any assertion here fail, that is a NULL arm + * coming back — re-read the v18 org-ownership decision (PR #14976), D8 before "fixing" the test. */ describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => { const STAMPED_MEMBERS: MemberRow[] = SEEDED_MEMBERS.map((m) => ({ @@ -294,27 +313,32 @@ describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => { organization_id: 'org_a', })); - it('an org-scoped rule DOES see an env-wide (null-org) seeded unit', async () => { + it('an org-scoped rule does NOT see an env-wide (null-org) seeded unit', async () => { + // The reported reproduction, pinned as the behaviour 17.x ships: the + // members are stamped and would pass the member screen, so the empty + // answer is the UNIT screen's alone. 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']); + expect(await g.expandUsers('bu_root')).toEqual([]); + expect(await g.expandUnitMembers('bu_root')).toEqual([]); }); - it('the seed check and the subtree walk BOTH admit the seeded rows', async () => { + it('the seed check and the subtree walk BOTH refuse 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. + // the same screen. Pinning the BFS separately keeps a widening applied to + // only one of them from passing as "unchanged". 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']); + expect(await g.descendants('bu_root')).toEqual([]); }); - it('`headOf` resolves the manager of a seeded unit too', async () => { + it('`headOf` does not resolve the manager of a seeded unit either', async () => { + // The third read through `orgScope`, pinned so the screen cannot be + // widened at one call site while the other two stay strict. const units: UnitRow[] = [ { id: 'bu_root', organization_id: null, active: true, manager_user_id: 'u_head' }, ]; @@ -322,12 +346,28 @@ describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => { engine: makeEngine(units, []), organizationId: 'org_a', }); + expect(await g.headOf('bu_root')).toBeNull(); + }); + + it('an org-stamped unit IS visible — the screen is strict, not broken', async () => { + // The control that separates "strict" from "refuses everything". Same + // fixture shape as the seeded case above; only the unit's organization + // moves, and that single change flips every read to visible. + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: 'org_a', active: true, manager_user_id: 'u_head' }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_a', active: true }, + ]; + const g = new BusinessUnitGraphService({ + engine: makeEngine(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']); + expect((await g.descendants('bu_root')).sort()).toEqual(['bu_child', 'bu_root']); 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. + it('another org’s unit is invisible too', async () => { 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 }, @@ -342,10 +382,14 @@ describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => { 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, - ); + it('an INACTIVE org-stamped unit still contributes nobody', async () => { + // `active: false` is a hard filter independent of either screen — pinned + // on a VISIBLE unit so the empty answer is the active flag's doing and + // not the tenant screen answering first. + const units: UnitRow[] = [ + { id: 'bu_root', organization_id: 'org_a', active: false }, + { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_a', active: true }, + ]; const g = new BusinessUnitGraphService({ engine: makeEngine(units, STAMPED_MEMBERS), organizationId: 'org_a', @@ -356,23 +400,32 @@ describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => { }); /** - * [#14547] The MEMBER screen is STRICT — the leak the unit widening would - * otherwise have opened. + * [#14949, KEPT] The MEMBER screen is STRICT. + * + * ⚠️ This is the SECURITY half of #14949 and it is NOT part of the the v18 org-ownership decision (PR #14976), D8 + * revert. Both `sys_business_unit_member` reads used to carry no organization + * predicate whatever — completely unscoped by organization — so an org-stamped + * rule reaching any visible unit collected every tenant's membership rows off + * it. `memberScope` closed that, and it stays closed. * - * ⚠️ 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. + * ⚠️ The anchor unit here is stamped with the rule's OWN organization, not + * seeded. That is load-bearing: with the unit screen strict again, a seeded + * (org-less) unit is invisible to an org-stamped rule, so a member assertion + * written on one would be answered by the UNIT screen before the member read + * ever ran — it would pass with `memberScope` deleted. Anchoring on a visible + * unit is what keeps every assertion below a pin on the MEMBER screen. */ 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. + * One unit tree the rule's organization owns, with two tenants' membership + * rows hanging off it — the shape an unscoped member read turns into a + * cross-tenant over-grant. `sys_business_unit_member` is not + * organization-stamped on every write path, so mixed tenancy on one visible + * unit is a real deployment shape, not a contrived one. */ 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 }, + { id: 'bu_market', organization_id: 'org_a', active: true }, + { id: 'bu_market_west', parent_business_unit_id: 'bu_market', organization_id: 'org_a', active: true }, ]; const TWO_TENANT_MEMBERS: MemberRow[] = [ { business_unit_id: 'bu_market', user_id: 'u_a', organization_id: 'org_a' }, @@ -403,9 +456,9 @@ describe('BusinessUnitGraphService — the MEMBER screen (#14547)', () => { 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. + // NULL here fails CLOSED. The anchor unit is visible to the rule, so this + // empty answer is the member screen's own — `SharingRuleService` warns + // rather than staying silent about it. const members: MemberRow[] = [ { business_unit_id: 'bu_market', user_id: 'u_seeded', organization_id: null }, ]; @@ -418,8 +471,9 @@ describe('BusinessUnitGraphService — the MEMBER screen (#14547)', () => { }); 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. + // The dominant shape today (declared rules bootstrap org-less). Neither + // #14949 nor the the v18 org-ownership decision (PR #14976), D8 revert of its unit half may change what a + // platform-global rule expands to, in either direction. const g = new BusinessUnitGraphService({ engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS), organizationId: null, diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index 7d5c5b6afb..6a20250614 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -51,15 +51,14 @@ 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: + * Two tenant screens live here, and BOTH are strict equalities: * - {@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); + * the ANCHOR the rule names; * - `memberScope` screens `sys_business_unit_member`, the SET BEING GRANTED, - * with a STRICT equality (a NULL organization there means unknown + * added by #14949 and kept (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. + * Each method carries the measurement it rests on. ⛔ They are not one method, + * and ⛔ neither one gets a NULL arm — see {@link BusinessUnitGraphService.orgScope}. * * Reuses {@link TeamGraphService.managerOf} for user-level manager * lookup so callers can use this single service in approval / sharing @@ -284,76 +283,61 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { } /** - * [#14547] The UNIT screen — the platform's own NULL-INCLUSIVE tenant - * predicate, not a strict equality. + * [v18 org-ownership decision · PR #14976 · D8] The UNIT screen — STRICT equality, as 17.2.0 ships it. * - * `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. + * ⛔ Do NOT re-add a NULL arm here. #14949 briefly made this `$or` the rule's + * own organization together with a second arm matching an unset + * `organization_id`, so that an org-stamped rule could name a seeded + * (org-less) `sys_business_unit` row — the shape + * `SqlDriver.applyTenantScope` writes. That was reverted before 17.3 was cut on + * the maintainer's ruling: it re-implements, a second time and in a second + * place, the very predicate `SqlDriver.applyTenantScope` already owns — the + * duplication the v18 org-ownership decision (PR #14976) exists to retire (#10103 cause 1) — and it had not + * shipped, so reverting cost nothing while shipping it would have owed v18 a + * breaking change plus a migration. * - * 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`). + * #14547 therefore REMAINS as in 17.2.0: an org-stamped rule naming a seeded + * unit expands to nobody. That is a real defect and it is not fixed here. + * (#14547 is CLOSED as completed — by #14949, whose unit half this reverts — + * so its tracker state no longer matches the runtime; reopening it is the + * maintainer's call.) + * Its root cause is the seed loader's `sys_` exemption plus first-boot + * ordering, and it is fixed STRUCTURALLY on the v18 line by the v18 org-ownership decision (PR #14976), C1 — + * the Default Organization exists before application seed datasets load, and + * the seed loader stamps `sys_business_unit` seeds — so the row this screen + * reads carries an organization and no screen has to special-case it. + * `SharingRuleService.warnOnEmptyUnitExpansion` (#14949's other half, kept) + * is what keeps the 17.x symptom LOUD instead of silent. */ private orgScope(filter: Record): Record { - if (!this.organizationId) return filter; - return { - ...filter, - $or: [{ organization_id: this.organizationId }, { organization_id: null }], - }; + if (this.organizationId) return { ...filter, organization_id: this.organizationId }; + return filter; } /** - * [#14547] The MEMBER screen — STRICT equality, deliberately not - * {@link orgScope}. + * [#14949, KEPT] The MEMBER screen — STRICT equality. + * + * ⚠️ This half is NOT part of the the v18 org-ownership decision (PR #14976), D8 revert that restored + * {@link orgScope} to a strict equality. It is kept, and it is load-bearing + * on its own. * * ## 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. + * completely unscoped by organization: any unit an org-stamped rule could + * see handed back EVERY tenant's membership rows hanging off it, and tenant + * A's rule materialised real `sys_record_share` rows for tenant B's users. + * A strict {@link orgScope} narrows which units are reachable but does not + * close that: `sys_business_unit_member` rows of another organization sit on + * org-stamped units too, and those units are exactly the visible ones. This + * screen is the only thing that answers there. * - * ## Why STRICT, when the unit screen is null-inclusive + * ## Why STRICT * - * 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. + * 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: @@ -383,8 +367,8 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { * 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 + * whose memberships were seeded expands to nobody even on a unit it can see. + * That outcome is 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. * diff --git a/packages/plugins/plugin-sharing/src/recipient-width.test.ts b/packages/plugins/plugin-sharing/src/recipient-width.test.ts index 7eeb4752af..8b9a12f9ce 100644 --- a/packages/plugins/plugin-sharing/src/recipient-width.test.ts +++ b/packages/plugins/plugin-sharing/src/recipient-width.test.ts @@ -329,24 +329,88 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => }]); }; + /** + * Units written before any organization existed — the `organization_id` + * column spelled explicitly, because a NULL column and an absent key are + * different things to a tenant screen. This is the shape #14547 is about. + */ + const seedOrglessUnits = () => + 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 }, + ]); + + /** + * The same tree, created through the API by `org_a` and therefore stamped. + * ⚠️ Required by every MEMBER-screen assertion below: with the unit screen + * strict, an org-less unit is invisible to an org-stamped rule, so a member + * assertion anchored on one would be answered by the UNIT screen before the + * member read ever ran — it would pass with `memberScope` deleted. + */ + const seedOrgStampedUnits = () => + engine.seed('sys_business_unit', [ + { id: 'bu_market', name: 'Market', parent_business_unit_id: null, organization_id: ORG_A, active: true }, + { id: 'bu_market_west', name: 'Market West', parent_business_unit_id: 'bu_market', organization_id: ORG_A, active: true }, + ]); + 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 () => { + /** + * ⚠️ #14547's symptom stands in 17.x and these two cases REPRODUCE it — they + * do not assert a fix. (#14547 itself is CLOSED as completed, closed by + * #14949 whose unit half this reverts; reopening it is the maintainer's + * call. These tests assert the behaviour, which is 17.2.0's.) #14949 closed it by widening the unit screen with the + * platform's NULL arm; that was reverted before the 17.3 tag (the v18 org-ownership decision (PR #14976), D8) + * because it re-implemented `SqlDriver.applyTenantScope`'s own predicate a + * second time in a second place, and had not shipped. The structural fix is + * the v18 org-ownership decision (PR #14976), C1 on the v18 line: the Default Organization exists before + * application seed datasets load and the seed loader stamps + * `sys_business_unit` seeds. + * + * What DID survive #14949 is the observability half — the rule no longer + * grants nobody in silence. + */ + describe('the reported defect — 201, active, zero shares, now LOUD (#14547)', () => { + it('WIDE — `unit_and_subordinates` still materialises NOTHING, and says so', async () => { + seedOrglessUnits(); + 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); + // The membership rows are stamped and would pass the member screen; the + // units are seeded, so the strict UNIT screen answers first. + expect(granteesOf('kpi_1')).toEqual([]); + expect(result.expandedUsers).toBe(0); + // The one thing 17.3 improves on 17.2.0 for this defect: it is no + // longer silent. + expect(emptyWarns()).toHaveLength(1); + }); + + it('NARROW — `business_unit` materialises nothing either', async () => { + seedOrglessUnits(); + 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); + expect(granteesOf('kpi_1')).toEqual([]); + expect(emptyWarns()).toHaveLength(1); + }); + + it('the SAME rule against an org-stamped tree grants normally — the control', async () => { + // Separates "#14547 stands" from "business-unit rules are broken". Only + // the units' organization moves; every other input is identical to the + // WIDE case above, and the grant materialises. + seedOrgStampedUnits(); 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 }, @@ -355,28 +419,36 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => 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 () => { + it('the two widths stay two widths on that tree (#7807)', async () => { + seedOrgStampedUnits(); 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', () => { + /** + * [#14949, KEPT] The MEMBER screen — NOT part of the the v18 org-ownership decision (PR #14976), D8 revert. + * + * ⚠️ Anchored on an ORG-STAMPED unit on purpose. With the unit screen strict + * again, an org-less unit is invisible to an org-stamped rule, so every + * assertion here would pass with `memberScope` deleted — the unit screen + * would answer before the member read ran. A visible unit is what makes + * these assertions pins on the MEMBER screen. + */ + describe('⚠️ the cross-tenant leak `memberScope` closes', () => { /** - * 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. + * ONE unit the rule's own organization owns, two tenants' membership rows + * hanging off it. `sys_business_unit_member` is not organization-stamped + * on every write path, so mixed tenancy on a visible unit is a real + * deployment shape. */ const twoTenantMembers = () => [ { id: 'bum_a', business_unit_id: 'bu_market', user_id: 'u_a', organization_id: ORG_A }, @@ -386,6 +458,7 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => ]; it('WIDE — no `sys_record_share` row is ever materialised for another org’s member', async () => { + seedOrgStampedUnits(); engine.seed('sys_business_unit_member', twoTenantMembers()); seedRule('unit_and_subordinates', ORG_A); await rules.evaluateRule(RULE, SYS); @@ -395,6 +468,7 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => }); it('NARROW — the single-unit width does not cross either', async () => { + seedOrgStampedUnits(); engine.seed('sys_business_unit_member', twoTenantMembers()); seedRule('business_unit', ORG_A); await rules.evaluateRule(RULE, SYS); @@ -404,8 +478,9 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => 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. + // write, so a NULL here fails CLOSED. The anchor unit is visible to the + // rule, so the exclusion below is the MEMBER screen's own doing. + seedOrgStampedUnits(); 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 }, @@ -418,9 +493,11 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => 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. + // The unit is visible and the membership row is org-less, so the empty + // expansion here is the MEMBER screen's — the residual case the warn + // exists for, and one the v18 org-ownership decision (PR #14976), C1 does not remove. It used to be + // completely silent. + seedOrgStampedUnits(); engine.seed('sys_business_unit_member', [ { id: 'bum_seeded', business_unit_id: 'bu_market', user_id: 'u_seeded', organization_id: null }, ]); @@ -441,6 +518,7 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => }); it('warns for the NARROW width too', async () => { + seedOrgStampedUnits(); seedRule('business_unit'); await rules.evaluateRule(RULE, SYS); expect(emptyWarns()).toHaveLength(1); @@ -451,6 +529,7 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => // 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. + seedOrgStampedUnits(); seedRule('unit_and_subordinates'); await rules.evaluateRule(RULE, SYS); await rules.evaluateRule(RULE, SYS); @@ -459,6 +538,7 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => }); it('says nothing when the rule grants somebody', async () => { + seedOrgStampedUnits(); engine.seed('sys_business_unit_member', [ { id: 'bum_1', business_unit_id: 'bu_market', user_id: 'u_1', organization_id: ORG_A }, ]); @@ -472,6 +552,7 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => // 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. + seedOrgStampedUnits(); engine.seed('sys_sharing_rule', [{ id: 'srule_off', organization_id: ORG_A, name: 'off_rule', label: 'Off', object_name: 'kpi_entry_sheet', @@ -487,9 +568,11 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => 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. + // no-ops for it — unmoved by #14949 and unmoved by the the v18 org-ownership decision (PR #14976), D8 + // revert of its unit half. Pinned in both directions so neither change + // can silently retire the declared cross-tenant behaviour of a + // null-org rule. + seedOrglessUnits(); 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 },