Skip to content

Commit e560b4d

Browse files
os-salesclaude
andauthored
fix(plugin-sharing): a seeded business unit is a usable sharing-rule recipient, and its members are tenant-screened (#14949)
* wip(#14547): tenant screens on the business-unit graph Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * fix(plugin-sharing): a seeded business unit is a usable sharing-rule recipient, and its members are tenant-screened (#14547) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * chore(#14547): re-anchor the system-context census rows onto the shifted read sites Repaired with the gate's own `--fix` path, never by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ca3fd4b commit e560b4d

7 files changed

Lines changed: 684 additions & 51 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@objectstack/plugin-sharing': patch
3+
---
4+
5+
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.
6+
7+
`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.
8+
9+
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.
10+
11+
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.

content/docs/permissions/system-context.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ The largest single consumer — **17 of the 106 sites**.
137137
| 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1088` |
138138
| 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link **creation** while the policy is off — resolution is **not** bypassed since #14033 (`publicSharing.enabled` is a standing policy held at every redemption): a link minted this way does not resolve until the block is enabled | `plugin-sharing/src/share-link-service.ts:449`, `:503`, `:507`, `:580`, `:610` |
139139
| 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` |
140-
| 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` |
140+
| 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` |
141141

142142
### 4. Approvals, reports, attachments, comments, knowledge
143143

packages/plugins/plugin-sharing/src/business-unit-graph.test.ts

Lines changed: 228 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,45 @@
11
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
22

33
/**
4-
* BusinessUnitGraphService — org scoping of the unit tree.
4+
* BusinessUnitGraphService — the TWO tenant screens, pinned as a pair.
55
*
6-
* These pin the ORG-SCOPE behaviour specifically, because it is the exact
7-
* shape that broke approvals in #3807: `orgScope()` AND-composes a strict
8-
* `organization_id = <rule org>` equality, so a unit written with no
9-
* organization at all (a seeded / file-layer / bootstrap row — a seed cannot
10-
* know the org id the runtime mints at boot) matches nothing, the seed check
11-
* fails, and the expansion returns zero members. In approvals that produced a
12-
* dead `department:<id>` approver slot; here it would produce a sharing rule
13-
* that silently grants nobody.
6+
* ## What this file used to say, and why it changed
147
*
15-
* It is NOT reachable today: every materialized `sys_sharing_rule` row carries
16-
* `organization_id = null` (verified on a live showcase stack), so
17-
* `expandRecipient` passes `null` and `orgScope` is skipped entirely. The
18-
* moment rules start carrying an org — a multi-tenant deployment — a BU
19-
* subtree rule against a seeded unit stops granting, and the symptom is
20-
* "the right people cannot see the record", which is far quieter than a stuck
21-
* approval.
8+
* Until #14547 `orgScope()` AND-composed a strict `organization_id = <rule
9+
* org>` equality onto the UNIT read. A unit written with no organization at
10+
* all (a seeded / file-layer / bootstrap row — a seed cannot know the org id
11+
* the runtime mints at boot) therefore matched nothing, the seed check failed,
12+
* and BOTH widths expanded to zero members. #3807 had already fixed exactly
13+
* that on the approvals side; this file recorded the sharing side's divergence
14+
* as deliberate on the grounds that it was unreachable, because every
15+
* materialized `sys_sharing_rule` row carried `organization_id = null`.
2216
*
23-
* So this file locks BOTH sides down:
24-
* - the reachable paths (null-org rule) keep working, and
25-
* - the divergence from approvals is written down as an executable fact
26-
* rather than a comment, so flipping it is a deliberate edit to a named
27-
* test and never a silent behaviour change.
17+
* It was reachable. #14547 is the external report: an org admin creating a
18+
* rule at runtime gets an org-stamped rule, the seeded unit carries none, and
19+
* the rule is accepted, stays active, materialises zero `sys_record_share`
20+
* rows and logs nothing. The `[divergence]` test that pinned the old posture
21+
* is gone — replaced, not merely flipped, because an assertion that keeps
22+
* passing while the mechanism under it changes is worse than no assertion.
2823
*
29-
* If the platform decides null-org means "env-wide, visible to every org" for
30-
* sharing too — the way `plugin-approvals` and `sys_metadata` already read it —
31-
* the test named `[divergence]` below is the one to flip, and `orgScope` grows
32-
* the same `$or: [{ organization_id }, { organization_id: null }]` predicate.
24+
* ## The pair this file now pins
25+
*
26+
* The fix is ASYMMETRIC and both halves have to be pinned, because each one
27+
* alone is a defect:
28+
*
29+
* - the UNIT screen (`orgScope`) is NULL-INCLUSIVE — the platform's own
30+
* `(organization_id = ? OR organization_id IS NULL)`, the predicate
31+
* `SqlDriver.applyTenantScope` writes and `plugin-approvals` already
32+
* applies to these very rows;
33+
* - the MEMBER screen (`memberScope`) is STRICT. Both member reads used to
34+
* carry no organization predicate at all, and the strict unit screen was
35+
* the only thing holding an org-stamped rule away from that unscoped
36+
* query. Widening the unit screen ALONE turns a silent under-grant into a
37+
* silent CROSS-TENANT OVER-GRANT, since a seeded unit id exists
38+
* identically in every tenant.
39+
*
40+
* So the security half is pinned separately from the functional half below: a
41+
* change that expands the right members while also expanding another
42+
* organization's members satisfies the functional pin completely.
3343
*/
3444

3545
import { describe, it, expect } from 'vitest';
@@ -40,8 +50,9 @@ interface UnitRow {
4050
parent_business_unit_id?: string | null;
4151
organization_id?: string | null;
4252
active?: boolean;
53+
manager_user_id?: string | null;
4354
}
44-
interface MemberRow { business_unit_id: string; user_id: string }
55+
interface MemberRow { business_unit_id: string; user_id: string; organization_id?: string | null }
4556

4657
/**
4758
* Minimal engine over `sys_business_unit` + `sys_business_unit_member`.
@@ -168,13 +179,33 @@ describe('BusinessUnitGraphService — the two widths are actually two widths (#
168179
});
169180

170181
it('the narrow width is org-predicated exactly like the wide one', async () => {
182+
// [#14547] Same fixture, new mechanism — and the mechanism is spelled out
183+
// because the ASSERTION did not move. `DIV_MEMBERS` carry no organization,
184+
// so before #14547 this returned `[]` because the strict UNIT screen hid
185+
// the seeded unit, and after it returns `[]` because the strict MEMBER
186+
// screen refuses membership rows of unknown tenancy. An unchanged
187+
// expectation over a changed cause is exactly the kind of pin that stops
188+
// guarding anything, so the two causes are separated below: the unit is
189+
// now visible (`descendants` sees the whole seeded tree), and it is the
190+
// members that are refused.
171191
const g = new BusinessUnitGraphService({
172192
engine: makeEngine(DIV_UNITS, DIV_MEMBERS),
173193
organizationId: 'org_a',
174194
});
175-
// Seeded (null-org) units are not visible to an org-scoped rule — the
176-
// same `[divergence]` posture the wide width holds below.
177195
expect(await g.expandUnitMembers('bu_div')).toEqual([]);
196+
expect((await g.descendants('bu_div')).sort()).toEqual(['bu_dept', 'bu_div', 'bu_office']);
197+
});
198+
199+
it('[#14547] both widths reach org-stamped members of a SEEDED unit tree', async () => {
200+
// The one change that flips the outcome: the membership rows are stamped,
201+
// exactly as a REST/session write stamps them. The units stay seeded.
202+
const members: MemberRow[] = DIV_MEMBERS.map((m) => ({ ...m, organization_id: 'org_a' }));
203+
const g = new BusinessUnitGraphService({
204+
engine: makeEngine(DIV_UNITS, members),
205+
organizationId: 'org_a',
206+
});
207+
expect(await g.expandUnitMembers('bu_div')).toEqual(['u_div']);
208+
expect((await g.expandUsers('bu_div')).sort()).toEqual(['u_dept', 'u_div', 'u_office']);
178209
});
179210

180211
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)', () => {
208239
{ id: 'bu_root', organization_id: 'org_a', active: true },
209240
{ id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_a', active: true },
210241
];
242+
// [#14547] The membership rows are stamped now. They used to be org-less
243+
// here and still expanded, because the member read carried no organization
244+
// predicate whatever — the gap #14547 closed. Units created through the
245+
// API by org_a have memberships created the same way, so this is the
246+
// fixture becoming faithful, not the assertion being relaxed.
247+
const members: MemberRow[] = SEEDED_MEMBERS.map((m) => ({ ...m, organization_id: 'org_a' }));
211248
const g = new BusinessUnitGraphService({
212-
engine: makeEngine(units, SEEDED_MEMBERS),
249+
engine: makeEngine(units, members),
213250
organizationId: 'org_a',
214251
});
215252
expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']);
@@ -224,17 +261,171 @@ describe('BusinessUnitGraphService — org scoping (#3807)', () => {
224261
expect(await g.expandUsers('bu_root')).toEqual([]);
225262
});
226263

227-
it('[divergence] an org-scoped rule does NOT see an env-wide (null-org) unit — approvals does (#3807)', async () => {
228-
// Same inputs that #3807 fixed on the approvals side. Sharing still reads
229-
// a null-org unit as "belongs to no org, therefore not mine" and grants
230-
// nobody. Unreachable today (rules are null-org), deliberate until the
231-
// platform rules on null-org semantics for AUTHORIZATION paths — widening
232-
// who can SEE a record is not a change to make on a defect that cannot
233-
// currently fire.
264+
it('an org-scoped rule never reaches another org’s MEMBER of a unit it can see', async () => {
265+
// [#14547] The unit is org_a's and visible; the membership row is org_b's.
266+
// The member screen is the only thing that answers here, so this fails if
267+
// `memberScope` is dropped even while every unit-level assertion passes.
268+
const units: UnitRow[] = [{ id: 'bu_root', organization_id: 'org_a', active: true }];
269+
const members: MemberRow[] = [
270+
{ business_unit_id: 'bu_root', user_id: 'u_a', organization_id: 'org_a' },
271+
{ business_unit_id: 'bu_root', user_id: 'u_b', organization_id: 'org_b' },
272+
];
234273
const g = new BusinessUnitGraphService({
235-
engine: makeEngine(SEEDED_UNITS, SEEDED_MEMBERS),
274+
engine: makeEngine(units, members),
275+
organizationId: 'org_a',
276+
});
277+
expect(await g.expandUsers('bu_root')).toEqual(['u_a']);
278+
expect(await g.expandUnitMembers('bu_root')).toEqual(['u_a']);
279+
});
280+
});
281+
282+
/**
283+
* [#14547] The UNIT screen is null-inclusive — the divergence from
284+
* `plugin-approvals` (#3807) is CLOSED.
285+
*
286+
* The `[divergence]` test that used to live in the block above pinned the
287+
* opposite posture on the grounds that it could not fire. It fired: the
288+
* external report is an org admin creating a rule at runtime against a unit
289+
* the app seeded.
290+
*/
291+
describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => {
292+
const STAMPED_MEMBERS: MemberRow[] = SEEDED_MEMBERS.map((m) => ({
293+
...m,
294+
organization_id: 'org_a',
295+
}));
296+
297+
it('an org-scoped rule DOES see an env-wide (null-org) seeded unit', async () => {
298+
const g = new BusinessUnitGraphService({
299+
engine: makeEngine(SEEDED_UNITS, STAMPED_MEMBERS),
300+
organizationId: 'org_a',
301+
});
302+
expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']);
303+
expect(await g.expandUnitMembers('bu_root')).toEqual(['u_root']);
304+
});
305+
306+
it('the seed check and the subtree walk BOTH admit the seeded rows', async () => {
307+
// `seedIsUsable` and the `descendants` BFS are two separate reads through
308+
// the same screen; a widening applied to one and not the other would still
309+
// answer `[]` for the subtree width.
310+
const g = new BusinessUnitGraphService({
311+
engine: makeEngine(SEEDED_UNITS, STAMPED_MEMBERS),
312+
organizationId: 'org_a',
313+
});
314+
expect((await g.descendants('bu_root')).sort()).toEqual(['bu_child', 'bu_root']);
315+
});
316+
317+
it('`headOf` resolves the manager of a seeded unit too', async () => {
318+
const units: UnitRow[] = [
319+
{ id: 'bu_root', organization_id: null, active: true, manager_user_id: 'u_head' },
320+
];
321+
const g = new BusinessUnitGraphService({
322+
engine: makeEngine(units, []),
323+
organizationId: 'org_a',
324+
});
325+
expect(await g.headOf('bu_root')).toBe('u_head');
326+
});
327+
328+
it('ONLY the NULL arm widened — another org’s unit is still invisible', async () => {
329+
// The control that separates "null-inclusive" from "unscoped". Without it
330+
// a screen that had simply been deleted would pass every assertion above.
331+
const units: UnitRow[] = [
332+
{ id: 'bu_root', organization_id: 'org_b', active: true },
333+
{ id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_b', active: true },
334+
];
335+
const g = new BusinessUnitGraphService({
336+
engine: makeEngine(units, STAMPED_MEMBERS),
236337
organizationId: 'org_a',
237338
});
238339
expect(await g.expandUsers('bu_root')).toEqual([]);
340+
expect(await g.expandUnitMembers('bu_root')).toEqual([]);
341+
expect(await g.descendants('bu_root')).toEqual([]);
342+
expect(await g.headOf('bu_root')).toBeNull();
343+
});
344+
345+
it('an INACTIVE seeded unit still contributes nobody', async () => {
346+
const units: UnitRow[] = SEEDED_UNITS.map((u) =>
347+
u.id === 'bu_root' ? { ...u, active: false } : u,
348+
);
349+
const g = new BusinessUnitGraphService({
350+
engine: makeEngine(units, STAMPED_MEMBERS),
351+
organizationId: 'org_a',
352+
});
353+
expect(await g.expandUsers('bu_root')).toEqual([]);
354+
expect(await g.expandUnitMembers('bu_root')).toEqual([]);
355+
});
356+
});
357+
358+
/**
359+
* [#14547] The MEMBER screen is STRICT — the leak the unit widening would
360+
* otherwise have opened.
361+
*
362+
* ⚠️ These are the SECURITY half and they are pinned apart from the functional
363+
* half on purpose: a change that expands the right members while also
364+
* expanding another organization's members passes every assertion in the block
365+
* above.
366+
*/
367+
describe('BusinessUnitGraphService — the MEMBER screen (#14547)', () => {
368+
/**
369+
* One SEEDED unit id with two tenants' memberships hanging off it — the
370+
* shape that exists on every deployment whose org chart came from a seed,
371+
* and the one the widened unit screen makes reachable.
372+
*/
373+
const SHARED_SEED_UNITS: UnitRow[] = [
374+
{ id: 'bu_market', organization_id: null, active: true },
375+
{ id: 'bu_market_west', parent_business_unit_id: 'bu_market', organization_id: null, active: true },
376+
];
377+
const TWO_TENANT_MEMBERS: MemberRow[] = [
378+
{ business_unit_id: 'bu_market', user_id: 'u_a', organization_id: 'org_a' },
379+
{ business_unit_id: 'bu_market', user_id: 'u_b', organization_id: 'org_b' },
380+
{ business_unit_id: 'bu_market_west', user_id: 'u_a_west', organization_id: 'org_a' },
381+
{ business_unit_id: 'bu_market_west', user_id: 'u_b_west', organization_id: 'org_b' },
382+
];
383+
384+
it('WIDE — a subtree expansion never crosses into another organization', async () => {
385+
const g = new BusinessUnitGraphService({
386+
engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS),
387+
organizationId: 'org_a',
388+
});
389+
const users = await g.expandUsers('bu_market');
390+
expect(users.sort()).toEqual(['u_a', 'u_a_west']);
391+
expect(users).not.toContain('u_b');
392+
expect(users).not.toContain('u_b_west');
393+
});
394+
395+
it('NARROW — the single-unit expansion does not cross either', async () => {
396+
const g = new BusinessUnitGraphService({
397+
engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS),
398+
organizationId: 'org_a',
399+
});
400+
expect(await g.expandUnitMembers('bu_market')).toEqual(['u_a']);
401+
});
402+
403+
it('an org-LESS membership row is NOT a member of an org-scoped rule', async () => {
404+
// Unknown tenancy, not platform-global: `sys_business_unit_member` is not
405+
// organization-stamped by seed replay or by an elevated system write, so a
406+
// NULL here cannot be read the way a NULL on the UNIT row is read. The
407+
// grant fails closed, and `SharingRuleService` warns rather than staying
408+
// silent about it.
409+
const members: MemberRow[] = [
410+
{ business_unit_id: 'bu_market', user_id: 'u_seeded', organization_id: null },
411+
];
412+
const g = new BusinessUnitGraphService({
413+
engine: makeEngine(SHARED_SEED_UNITS, members),
414+
organizationId: 'org_a',
415+
});
416+
expect(await g.expandUnitMembers('bu_market')).toEqual([]);
417+
expect(await g.expandUsers('bu_market')).toEqual([]);
418+
});
419+
420+
it('an org-LESS rule is unmoved — both screens stay no-ops', async () => {
421+
// The dominant shape today (declared rules bootstrap org-less). #14547
422+
// must not change what they expand to, in either direction.
423+
const g = new BusinessUnitGraphService({
424+
engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS),
425+
organizationId: null,
426+
});
427+
expect((await g.expandUsers('bu_market')).sort()).toEqual([
428+
'u_a', 'u_a_west', 'u_b', 'u_b_west',
429+
]);
239430
});
240431
});

0 commit comments

Comments
 (0)