Skip to content

Commit 0b7286f

Browse files
fix(plugin-sharing): re-evaluate position and team sharing rules for the user whose membership changes (#24790)
Security fix (maintainer-ordered; no public card; detail withheld under the disclosure ruling). Clause-②: no ## What changed `@objectstack/plugin-sharing` now re-evaluates position-recipient and team-recipient sharing rules for the affected user when the membership behind the recipient changes. - **New hooks, `recipient-membership-recompute.ts`.** `afterInsert`, `afterUpdate` and `afterDelete` on `sys_user_position` and `sys_team_member`, bound under their own package id so a rule rebind cannot remove them. The plugin binds them next to the business-unit recompute. - **Which writes trigger it.** Each insert and each delete triggers it. An update triggers it only when its patch touches a column the expansion reads: the user, the position or team, the organization, or the validity window. For each such write, every rule whose recipient is the position or team the row names (before or after the write) is re-evaluated for the user the row names. - **Organization scope.** An organization's rule is re-evaluated only for rows of its own organization. A platform-global rule is re-evaluated for every row. Team expansion has no organization scope, so neither does its trigger. - **`SharingRuleService.reconcileRuleForUser(rule, userId)`.** The user-scoped pass. Its outcome equals the whole-rule pass restricted to that user. If the rule reaches the user, the user ends up with one grant per matched record at the rule's level. If it does not reach the user (inactive rule, switched-off position, or the user no longer holds the recipient), the user ends up with none. Withdrawal goes through `SharingService.revoke`, one grant at a time, the same way the whole-rule pass withdraws. - **User-scoped holder checks.** `PositionGraphService.holdsPosition` and `teamHasMember` / `holdsRole` in `team-graph.ts` read the same rows the expansions read, narrowed to one user. The expansions and the checks build their predicates from one shared function per table, so the two cannot drift apart. - **Docs.** `content/docs/permissions/sharing-rules.mdx` now lists the new reconcile moment in its "Switching a rule off" table. The sentence in `permissions-matrix.mdx` about when rules are re-evaluated now names it too. ## The invariant restored When a position is assigned to a user or removed from one, every sharing rule whose recipient is that position is re-evaluated for that user in the same write path. A new holder gains the rule's grants. A removed holder loses them before the write returns, and nobody has to re-evaluate the rule by hand. Team membership follows the same rule. Other users' grants are not touched, and neither are rules that name other recipients. ## Cost The business-unit recompute does not recompute every rule on every graph write, and it queues its grant half. The reason is that a BU write can move many recipients at once. This change addresses the same cost concern by narrowing the scope instead of deferring the work. A membership row moves exactly one user. Per written row the pass costs: - one rule listing; - for each rule that names the recipient: an indexed holder check (never the recipient's whole member list), that user's grants of the rule, and, only when the rule reaches the user, the rule's criteria read plus the grants the user lacks. A bulk import of N rows therefore costs N user-scoped passes and never runs a whole-rule pass. ## Tests - `src/recipient-membership-recompute.test.ts`: a real `ObjectQL` engine over `SqlDriver` (better-sqlite3), so the engine itself supplies each hook context (inserted row, update and delete pre-image, per-row dispatch of a batch insert). Pins: - a new holder gains the rule's grants; - a removed holder loses them, and both read (`buildReadFilter`) and edit (`checkEdit` → `deny`) are refused; - moving an assignment to another user, or to another position, moves the grants with it; - a validity window that closes counts as a removal; - a second assignment of the same position, or a membership role naming it, keeps the grants; - an unrelated position's grants are untouched and its rule is never re-evaluated; - other holders' grants are untouched; - an assignment in another organization does not re-evaluate an organization's rule; - an update that cannot change who holds the position re-evaluates nothing; - a bulk import of 40 assignments makes 20 user-scoped passes (for the named position), no whole-rule pass, and only user-scoped assignment reads; - after the user-scoped passes, a whole-rule `evaluateRule` has nothing left to create, update or revoke; - team: a new member gains, a removed member loses; - binding, write classification and organization pinning. - `src/position-graph.test.ts`: `holdsPosition` answers exactly `expandPositionUsers(...).includes(user)` across users, positions, organization scopes, validity windows and the membership-role source, and it reads one user only. - **Ablations**, each run once with `scripts/ablation-replace.mjs` and restored to the HEAD blob (`git diff HEAD` empty): 1. No membership hook bound: 13 of 18 red. 2. Organization pin removed: 2 of 18 red. 3. Holder check replaced by the full expansion: the bulk pin red. 4. The user-scoped withdrawal revokes nothing: 7 of 18 red (every removal, move and team-removal pin). Readings at `99997e91e`: - `pnpm --filter @objectstack/plugin-sharing test`: 51 files, 1192 tests passed. - `pnpm --filter @objectstack/plugin-sharing typecheck`: exit 0. The test layer compiles, and `test-typecheck-debt.json` holds its 3 existing pinned signatures with no change. - The new and changed files on their own: 26 tests passed. - Ablations 1 to 3 ran at `6d46619c7`; the lines they mutate are unchanged since then. Ablation 4 ran at `99997e91e`. ## Gates All gate families that `node scripts/pm/dispatch-gates.mjs --commands` derives for this change set: 99 families, run locally at `99997e91e`. - 98 families exit 0. `check:type-check-debt` ran under the verify lock: exit 0, 26 raw tsc errors, none above its recorded number. - `node scripts/check-plugin-teardown-shape.mjs --self-test` exits 3 (PREREQUISITE NOT MEASURED). Its pinned positive control is not reachable from this shallow clone. CI runs it. - `dispatch-gates --ran` reconciliation: 99 derived, 99 run, 0 unrun. - `pnpm --filter @objectstack/spec build` and `check:generated` ran after merging `origin/main`: exit 0, all 14 generated artifacts up to date. The full test battery and the type-check lanes are CI's. --- _Generated by [Claude Code](https://claude.ai/code/session_01KqkTQNLcxVc588eZpJmp1r)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent faead58 commit 0b7286f

11 files changed

Lines changed: 1130 additions & 13 deletions
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/plugin-sharing': patch
3+
---
4+
5+
fix(plugin-sharing): position- and team-recipient sharing rules are re-evaluated for the affected user when the membership behind the recipient changes
6+
7+
Clause-②: no
8+
9+
A sharing rule's grants are materialised per user. When a position assignment (`sys_user_position`) or a team membership (`sys_team_member`) is inserted, deleted, or updated in a column the recipient expansion reads (the user, the position or team, the organization, the validity window), every sharing rule whose recipient the row names is now re-evaluated for the user the row names, before the write returns. A new holder gains the rule's grants and a former holder loses them. Other users' grants, and rules naming other recipients, are not touched.
10+
11+
The re-evaluation is scoped to that one user and to the rules naming the recipient: an indexed holder check, that user's grants of the rule, and the rule's criteria read only when the rule reaches the user. A bulk assignment import therefore costs one user-scoped pass per row and never a whole-rule pass. An organization's rule is re-evaluated only for rows of its own organization; a platform-global rule for every row.
12+
13+
`SharingRuleService` gains `reconcileRuleForUser(rule, userId)`, the user-scoped pass these hooks run. No spec key, endpoint or configuration changes.

‎content/docs/permissions/permissions-matrix.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ Sharing rules extend access beyond ownership and the depth axis. The declarative
173173
</Callout>
174174

175175
<Callout type="info">
176-
**Beyond declarative rules:** Two other sharing mechanisms exist but are **not** `SharingRule` types. **Manual sharing** is a runtime grant — `sys_record_share` rows created with `source: 'manual'` (see `packages/plugins/plugin-sharing/src/sharing-service.ts`). **Matrix / territory-shaped access** is served today by multi-position assignment anchored on business units, plus pre-resolved membership sets: a registered `rls-membership-resolver` (ADR-0105 D11) stages e.g. `territory_account_ids` into `ExecutionContext.rlsMembership`, and a policy references it as `account_id IN (current_user.territory_account_ids)`. The aspirational `TerritorySchema` was removed in ADR-0105 D11 — it had no runtime object, stack field or resolver; a generalized dimension-security module will arrive with its own ADR. Criteria rules are re-evaluated on record insert/update via internal sharing rule hooks, on every write to the rule itself, and once per boot — so deactivating or deleting a rule withdraws the grants it materialized (see [Sharing Rules](/docs/permissions/sharing-rules#switching-a-rule-off-withdraws-the-access-it-granted)).
176+
**Beyond declarative rules:** Two other sharing mechanisms exist but are **not** `SharingRule` types. **Manual sharing** is a runtime grant — `sys_record_share` rows created with `source: 'manual'` (see `packages/plugins/plugin-sharing/src/sharing-service.ts`). **Matrix / territory-shaped access** is served today by multi-position assignment anchored on business units, plus pre-resolved membership sets: a registered `rls-membership-resolver` (ADR-0105 D11) stages e.g. `territory_account_ids` into `ExecutionContext.rlsMembership`, and a policy references it as `account_id IN (current_user.territory_account_ids)`. The aspirational `TerritorySchema` was removed in ADR-0105 D11 — it had no runtime object, stack field or resolver; a generalized dimension-security module will arrive with its own ADR. Criteria rules are re-evaluated on record insert/update via internal sharing rule hooks, on every write to the rule itself, when the business-unit graph or the position or team membership behind a rule's recipient changes, and once per boot — so deactivating or deleting a rule withdraws the grants it materialized (see [Sharing Rules](/docs/permissions/sharing-rules#switching-a-rule-off-withdraws-the-access-it-granted)).
177177
</Callout>
178178

179179
### Configuration Example

‎content/docs/permissions/sharing-rules.mdx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,13 +213,14 @@ default organization is bootstrapped (cloud ADR-0081 D1).
213213
A sharing rule's grants are **materialized** — evaluating a rule writes real
214214
`sys_record_share` rows with `source: 'rule'` and `source_id` set to the rule.
215215
Because those rows outlive the evaluation that produced them, withdrawal has to
216-
be an explicit act, and it happens at **four** moments:
216+
be an explicit act, and it happens at **five** moments:
217217

218218
| When | What is reconciled |
219219
|:--|:--|
220220
| **The write that deactivates or edits the rule** | That rule's grants, immediately — deactivating with `active: false` (or `POST {basePath}/sharing/rules` with the same name) revokes them before the call returns |
221221
| **The next insert/update of a matching record** | That record's grants for every rule on the object — an inactive rule desires nothing, so its rows are revoked |
222222
| **A `sys_business_unit` or `sys_business_unit_member` write** | The grants of rules whose recipients read the BU graph — `unit_and_subordinates` and `business_unit`, and only those two. Recipients the rule no longer reaches are revoked **before the write returns**; the grant direction (a unit moved *into* a shared subtree) is queued and coalesced per rule (objectstack#7729) |
223+
| **A `sys_user_position` or `sys_team_member` write** | The grants of rules whose recipient is the position or team the row names, re-evaluated for the user the row names — inserts, deletes, and updates to the user, the recipient, the organization or the validity window. A new holder gains the rule's grants and a former holder loses them **before the write returns**; other users' grants and rules naming other recipients are not touched |
223224
| **Every boot** | All rules, plus a sweep of `source: 'rule'` rows whose `source_id` no longer resolves to any rule |
224225

225226
Deleting a rule withdraws its grants too, whether you delete it through

‎packages/plugins/plugin-sharing/src/bu-tree-recompute.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,9 @@
5151
* Everything else is deliberately NOT recomputed. That exclusion is a
5252
* requirement, not an optimisation: a fix that recomputed every rule on every
5353
* BU write would close the security hole and replace it with a load problem —
54-
* see {@link BU_TREE_RECIPIENT_TYPES}.
54+
* see {@link BU_TREE_RECIPIENT_TYPES}. The `position` and `team` kinds are
55+
* recomputed by writes to THEIR membership tables instead, per affected user —
56+
* `recipient-membership-recompute.ts`.
5557
*
5658
* ## The design: synchronous revoke, asynchronous re-grant
5759
*

‎packages/plugins/plugin-sharing/src/position-graph.test.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,3 +96,64 @@ describe('PositionGraphService (ADR-0090 D3 — flat expansion)', () => {
9696
expect(calls).toBe(before); // second hit served from cache
9797
});
9898
});
99+
100+
describe('PositionGraphService.holdsPosition — the user-scoped twin of the expansion', () => {
101+
const past = '2000-01-01T00:00:00Z';
102+
const future = '2999-01-01T00:00:00Z';
103+
const ASSIGN = [
104+
{ position: 'rep', user_id: 'u_a', organization_id: 'org_a' },
105+
{ position: 'rep', user_id: 'u_b', organization_id: 'org_b' },
106+
{ position: 'rep', user_id: 'u_global', organization_id: null },
107+
{ position: 'rep', user_id: 'u_expired', organization_id: 'org_a', valid_until: past },
108+
{ position: 'rep', user_id: 'u_pending', organization_id: 'org_a', valid_from: future },
109+
{ position: 'rep', user_id: 'u_mixed', organization_id: 'org_a', valid_until: past },
110+
{ position: 'rep', user_id: 'u_mixed', organization_id: 'org_a', valid_from: past, valid_until: future },
111+
{ position: 'mgr', user_id: 'u_a', organization_id: 'org_a' },
112+
];
113+
const MEMBER = [
114+
{ role: 'rep', user_id: 'u_role_a', organization_id: 'org_a' },
115+
{ role: 'rep', user_id: 'u_role_b', organization_id: 'org_b' },
116+
];
117+
/** Honours every predicate key both reads send: position / role, organization_id, user_id. */
118+
const engine = {
119+
async find(object: string, options: any) {
120+
const f = options?.filter ?? options?.where ?? {};
121+
const rows: any[] = object === 'sys_user_position' ? ASSIGN : object === 'sys_member' ? MEMBER : [];
122+
const matches = (row: any, where: any): boolean => {
123+
for (const [k, v] of Object.entries(where)) {
124+
if (k.startsWith('$')) throw new Error('test double: unsupported logical operator ' + k);
125+
if (row[k] !== v) return false;
126+
}
127+
return true;
128+
};
129+
return rows.filter((r) => matches(r, f)).slice(0, options?.limit ?? rows.length);
130+
},
131+
} as any;
132+
const USERS = ['u_a', 'u_b', 'u_global', 'u_expired', 'u_pending', 'u_mixed', 'u_role_a', 'u_role_b', 'u_none'];
133+
134+
it('answers exactly expandPositionUsers(...).includes(user) for every user, position and organization scope', async () => {
135+
const verdicts = { holds: 0, not: 0 };
136+
for (const org of [undefined, 'org_a', 'org_b']) {
137+
for (const position of ['rep', 'mgr', 'nope']) {
138+
const expanded = await new PositionGraphService({ engine, organizationId: org ?? null }).expandPositionUsers(position, org);
139+
for (const user of USERS) {
140+
const g = new PositionGraphService({ engine, organizationId: org ?? null });
141+
const holds = await g.holdsPosition(user, position, org);
142+
expect({ org, position, user, holds }).toEqual({ org, position, user, holds: expanded.includes(user) });
143+
verdicts[holds ? 'holds' : 'not'] += 1;
144+
}
145+
}
146+
}
147+
// Both answers occur, so the matrix cannot pass by answering one of them.
148+
expect(verdicts.holds).toBeGreaterThan(5);
149+
expect(verdicts.not).toBeGreaterThan(5);
150+
});
151+
152+
it('reads one user, never the position\'s whole holder list', async () => {
153+
const seen: any[] = [];
154+
const spy = { async find(object: string, options: any) { seen.push({ object, f: options?.filter ?? options?.where }); return engine.find(object, options); } } as any;
155+
await new PositionGraphService({ engine: spy, organizationId: 'org_a' }).holdsPosition('u_role_a', 'rep', 'org_a');
156+
expect(seen.map((s) => s.object)).toEqual(['sys_user_position', 'sys_member']);
157+
expect(seen.every((s) => s.f.user_id === 'u_role_a')).toBe(true);
158+
});
159+
});

‎packages/plugins/plugin-sharing/src/position-graph.ts‎

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,26 @@
33
import { isGrantActive } from '@objectstack/core';
44

55
import type { SharingEngine } from './sharing-service.js';
6-
import { TeamGraphService } from './team-graph.js';
6+
import { TeamGraphService, holdsRole } from './team-graph.js';
77

88
const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const;
99

10+
/**
11+
* The `sys_user_position` predicate "assignments of `positionName`", scoped to
12+
* `scopeOrg` when one is given — ONE spelling for the rule-wide expansion
13+
* ({@link PositionGraphService.expandPositionUsers}) and the user-scoped check
14+
* ({@link PositionGraphService.holdsPosition}), so the two can never disagree
15+
* about which assignment rows count.
16+
*/
17+
function positionAssignmentPredicate(positionName: string, scopeOrg: string | null | undefined): Record<string, unknown> {
18+
const filter: Record<string, unknown> = { position: positionName };
19+
if (scopeOrg) filter.organization_id = scopeOrg;
20+
return filter;
21+
}
22+
23+
/** The columns both reads project: the holder and the ADR-0091 D2 validity window. */
24+
const ASSIGNMENT_FIELDS = ['user_id', 'valid_from', 'valid_until'];
25+
1026
type PositionCache = {
1127
expand?: Map<string, string[]>;
1228
};
@@ -73,16 +89,14 @@ export class PositionGraphService {
7389
const users = new Set<string>();
7490

7591
// 1) Platform assignment table (source of truth).
76-
const filter: Record<string, unknown> = { position: positionName };
7792
const scopeOrg = organizationId ?? this.organizationId;
78-
if (scopeOrg) filter.organization_id = scopeOrg;
7993
try {
8094
// valid_from / valid_until ride the projection so the ADR-0091 D2
8195
// validity filter below sees them — expired holders stop receiving
8296
// position-recipient shares at resolution time, fail-closed.
8397
const rows = await this.engine.find('sys_user_position', {
84-
filter,
85-
fields: ['user_id', 'valid_from', 'valid_until'],
98+
filter: positionAssignmentPredicate(positionName, scopeOrg),
99+
fields: ASSIGNMENT_FIELDS,
86100
limit: 10000,
87101
context: SYSTEM_CTX,
88102
});
@@ -105,4 +119,42 @@ export class PositionGraphService {
105119
this.cache.expand!.set(key, result);
106120
return result;
107121
}
122+
123+
/**
124+
* Does `userId` hold `positionName`? The user-scoped twin of
125+
* {@link expandPositionUsers}: the same two sources, the same organization
126+
* scope and the same ADR-0091 D2 validity filter, each narrowed to one user,
127+
* so the answer is `expandPositionUsers(positionName, organizationId)
128+
* .includes(userId)` — pinned in `position-graph.test.ts` — read in two
129+
* indexed lookups instead of the position's whole holder list.
130+
*
131+
* Exists for the recipient-membership recompute
132+
* (`recipient-membership-recompute.ts`), which re-evaluates a position rule
133+
* for the ONE user whose assignment was written. That runs on the write path,
134+
* once per written row, so a full expansion there would make a bulk
135+
* assignment import quadratic in the position's holder count.
136+
*
137+
* Uncached on purpose: the answer is wanted right after a write that may
138+
* have changed it. A failed assignment read falls through to the transition
139+
* source, as the expansion's does.
140+
*/
141+
async holdsPosition(userId: string, positionName: string, organizationId?: string): Promise<boolean> {
142+
if (!userId || !positionName) return false;
143+
const scopeOrg = organizationId ?? this.organizationId;
144+
try {
145+
const rows = await this.engine.find('sys_user_position', {
146+
filter: { ...positionAssignmentPredicate(positionName, scopeOrg), user_id: userId },
147+
fields: ASSIGNMENT_FIELDS,
148+
limit: 1000,
149+
context: SYSTEM_CTX,
150+
});
151+
const nowMs = Date.now();
152+
for (const r of (rows ?? []) as any[]) {
153+
if (String(r.user_id ?? '') === userId && isGrantActive(r, nowMs)) return true;
154+
}
155+
} catch {
156+
/* table may not exist on minimal stacks — the transition source below still applies */
157+
}
158+
return holdsRole(this.engine, positionName, userId, scopeOrg);
159+
}
108160
}

0 commit comments

Comments
 (0)