Skip to content

Commit ab47816

Browse files
claude[bot]claude
andauthored
revert(plugin-sharing): drop the NULL-inclusive business-unit screen before 17.3; keep the strict member screen (#15078)
* revert(plugin-sharing): drop the NULL-inclusive business-unit screen before 17.3 Restore `BusinessUnitGraphService.orgScope` to the strict `organization_id = <rule org>` equality 17.2.0 ships, byte-identical to its form before e560b4d (#14949), and delete the docblock that argued for the NULL-inclusive shape. Maintainer ruling, ADR-0131 D8: that shape re-implements the predicate `SqlDriver.applyTenantScope` already owns, a second time in a second place, which is the duplication ADR-0131 exists to retire (#10103 cause 1) — and it had not shipped, so reverting costs nothing while releasing it would owe v18 a breaking change and a migration. KEPT, untouched: `memberScope` (byte-identical to origin/main) and `SharingRuleService.warnOnEmptyUnitExpansion`. Both member reads previously carried no organization predicate at all; 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. #14547 therefore remains open and behaves as in 17.2.0. It is fixed structurally on the v18 line by ADR-0131 C1. Tests: the cases that pinned the NULL-inclusive unit screen now pin the restored behaviour and name #14547 as the open defect they reproduce. The member-screen pins are re-anchored from a seeded (org-less) unit onto an org-stamped one — with the unit screen strict, an org-less unit is invisible to an org-stamped rule, so those assertions would otherwise pass with `memberScope` deleted. See the PR body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * docs(plugin-sharing): state #14547's tracker status accurately in the reverted screens #14547 is CLOSED as completed — closed by #14949, whose unit half this branch reverts — so calling it "the OPEN defect" in the pins would have been wrong on a checkable fact. The tests name #14547 as the defect they reproduce and say plainly that its tracker state no longer matches the 17.x runtime, leaving the reopen decision to the maintainer rather than asserting it. Comment-only; no assertion, fixture or method body moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * docs(changeset): correct #14949's unreleased entry to describe only what 17.3 ships `.changeset/bu-tenant-screen-relanding.md` is still on main and unconsumed, and it announces as shipped fact the half this branch reverts: "orgScope now applies the platform's null-inclusive screen", under a headline claiming the seed-data under-grant is fixed. Both changesets compile into the same 17.3 notes, so leaving it would ship two contradictory patch entries for one package and tell users a tenancy fix landed that did not. Corrected surgically: the headline and the orgScope paragraph now state that the unit half was reverted and that the seed-data symptom remains as in 17.2.0, fixed structurally in v18 by ADR-0131 C1. The member-screen and warn paragraphs are kept — those DO ship — with one sentence rewritten, since the leak they close does not depend on the unit screen having been widened. ⛔ No code from #14949 outside this card's scope is touched: sharing-rule-service.ts and sharing-rule.test.ts are untouched, and this is not `git revert e560b4d`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 * docs: carry the v18 decision citation as PR #14976, not as an ADR-0131 anchor `check-adr-anchors` is red on the previous head: this branch cited `ADR-0131` 22 times and `docs/adr/0131-*.md` does not exist — ADR files stop at 0130 and PR #14976 (which would author it) is open and draft. The gate treats an unresolvable ADR id as a citation squat: whoever later writes a real ADR-0131 would retroactively falsify every one of those citations at once (#6634). No legitimate forward-reference form exists. The gate's only citation exemption is CROSS_REPO_QUALIFIERS (objectui / object-ui / cloud), so a repo qualifier is skipped but `ADR-0131 (PR #14976)` is not — the qualifier must be a sibling repo, not a PR. Its allowlist is shrink-only and documented to stay empty. ⛔ Deliberately NOT done: no allowlist entry, and no `docs/adr/0131-*.md` created here — that file belongs to PR #14976 and `docs/adr/**` is governed. Every citation now reads "the v18 org-ownership decision (PR #14976)", keeping the decision letters (D8 / C1) that carry the actual meaning. Issue and PR references (#14547, #14949, #10103, #13564) are untouched — they resolve. Substance is unchanged; the ADR id can be restored once #14976 lands. check-adr-anchors now exits 0: "OK (53 anchored file(s) … 31871 citation(s) across 4202 file(s) resolve)". 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 6665c5c commit ab47816

5 files changed

Lines changed: 328 additions & 177 deletions

File tree

.changeset/bu-tenant-screen-relanding.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22
'@objectstack/plugin-sharing': patch
33
---
44

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.
5+
Tenant-screen the `sys_business_unit_member` reads, and warn when an active business-unit rule grants nobody.
66

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.
7+
⚠️ 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.
88

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.
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. 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.
1010

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.
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 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".
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
"@objectstack/plugin-sharing": patch
3+
---
4+
5+
revert(plugin-sharing): drop the NULL-inclusive business-unit screen added after 17.2.0
6+
7+
17.3 does not ship the NULL-inclusive business-unit screen added after 17.2.0;
8+
#14547 remains as in 17.2.0 and is fixed structurally in v18 (the v18 org-ownership decision (PR #14976), C1: the
9+
Default Organization exists before application seed datasets load, and the seed
10+
loader stamps `sys_business_unit` seeds).
11+
12+
`BusinessUnitGraphService.orgScope` briefly read
13+
`$or: [{ organization_id: <rule org> }, { organization_id: null }]` so that an
14+
org-stamped sharing rule could name a seeded (org-less) `sys_business_unit`
15+
row. It is restored to the strict `organization_id = <rule org>` equality
16+
17.2.0 ships. That shape re-implemented, a second time and in a second place,
17+
the predicate `SqlDriver.applyTenantScope` already owns — the duplication
18+
the v18 org-ownership decision (PR #14976) exists to retire (#10103 cause 1) — and it had not been released, so
19+
reverting costs nothing while shipping it would have owed v18 a breaking change
20+
and a migration.
21+
22+
The other half of the same change is KEPT and is not touched:
23+
`BusinessUnitGraphService.memberScope` still screens both
24+
`sys_business_unit_member` reads with a strict equality. Those reads previously
25+
carried no organization predicate at all, so an org-stamped rule reaching any
26+
visible unit collected every tenant's membership rows hanging off it; a strict
27+
unit screen narrows which units are reachable but does not close that, because
28+
other organizations' member rows sit on org-stamped units too.
29+
`SharingRuleService.warnOnEmptyUnitExpansion` is also kept: it is what keeps the
30+
remaining #14547 symptom loud instead of silent.

0 commit comments

Comments
 (0)