From 912ac6c9651ed5b256eb510c22b31731239095bc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:11:50 +0000 Subject: [PATCH 1/2] fix(sharing): honour the declared organizationId in managerOf (#10231) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ITeamGraphService.managerOf declares an organization parameter that TeamGraphService.managerOf spelled _organizationId and discarded, while expandRoleUsers on the same class applied organization_id to its read. The BusinessUnitGraphService standalone fallback read sys_user the same unscreened way, so the answer depended on whether a teamGraph was passed to the ctor. Both limbs now apply the screen #10153 landed for the identical column on the approvals side: a manager provably outside the caller's organization is dropped. The read is sys_member because sys_user carries no organization_id — it is the global better-auth identity table — so filtering the sys_user read would match nothing and silently return null for every lookup. Fail-open on an absent tenancy fact, and no query at all when no organization is in play. The manager cache key is now organization-qualified. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 --- .changeset/manager-of-org-screen.md | 27 ++ .../plugin-sharing/src/business-unit-graph.ts | 21 +- .../src/manager-org-screen.test.ts | 230 ++++++++++++++++++ .../plugins/plugin-sharing/src/team-graph.ts | 99 +++++++- 4 files changed, 371 insertions(+), 6 deletions(-) create mode 100644 .changeset/manager-of-org-screen.md create mode 100644 packages/plugins/plugin-sharing/src/manager-org-screen.test.ts diff --git a/.changeset/manager-of-org-screen.md b/.changeset/manager-of-org-screen.md new file mode 100644 index 0000000000..5de7ee32bc --- /dev/null +++ b/.changeset/manager-of-org-screen.md @@ -0,0 +1,27 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(sharing): honour the declared `organizationId` in `managerOf` (#10231) + +`ITeamGraphService.managerOf(userId, organizationId?)` declares an organization +parameter. `TeamGraphService.managerOf` spelled it `_organizationId` and +discarded it, and the `BusinessUnitGraphService` standalone fallback read +`sys_user` the same unscreened way — a declared-but-unenforced parameter on a +security seam, while `expandRoleUsers` on the same class applied +`organization_id` to its own read. + +Both now apply the screen #10153 landed for the identical column +(`sys_user.manager_id`) on the approvals side: a manager who is **provably** +outside the caller's organization — membership rows exist for him, none of +them in that organization — is dropped. The read is `sys_member`, because +`sys_user` is the global better-auth identity table and carries no +`organization_id` at all; filtering the `sys_user` read on a column that does +not exist would match nothing and silently return `null` for every lookup. + +The screen is fail-open on an ABSENT tenancy fact (no membership rows, or the +membership read failed) and issues no query at all when no organization is in +play, so callers that pass nothing — which is how the parameter is used today — +are byte-identical to before. The manager cache key is now organization- +qualified; a user-keyed cache would have served one screened `null` to every +unscoped reader behind it. diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index 65ffa41a24..b7ec0c9499 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -2,7 +2,7 @@ import type { IBusinessUnitGraphService } from '@objectstack/spec/contracts'; import type { SharingEngine } from './sharing-service.js'; -import { TeamGraphService } from './team-graph.js'; +import { TeamGraphService, managerIsProvablyOutsideOrg } from './team-graph.js'; const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; @@ -231,10 +231,22 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { return head; } + /** + * [#10231] Honours the declared `organizationId` on BOTH limbs. + * + * The delegating limb always did, by construction — it hands the argument to + * {@link TeamGraphService.managerOf}, which now screens. The STANDALONE + * fallback below did not, and that gap was the whole risk: the fallback is + * reached exactly when no `teamGraph` was supplied, so a caller could get the + * unscreened answer from the same method name purely by how the service + * happened to be constructed. A screen that depends on a constructor option + * is not a screen. + */ async managerOf(userId: string, organizationId?: string): Promise { if (this.teamGraph) return this.teamGraph.managerOf(userId, organizationId); // Standalone fallback: read sys_user.manager_id directly. if (!userId) return null; + const org = organizationId ?? this.organizationId; try { const rows = await this.engine.find('sys_user', { where: { id: userId }, @@ -243,7 +255,12 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { context: SYSTEM_CTX, }); const row: any = Array.isArray(rows) ? rows[0] : null; - return row?.manager_id ? String(row.manager_id) : null; + const managerId = row?.manager_id ? String(row.manager_id) : null; + if (!managerId) return null; + // The SAME screen the delegating limb applies — one implementation, + // imported rather than restated, so the two limbs cannot drift. + if (await managerIsProvablyOutsideOrg(this.engine, managerId, org)) return null; + return managerId; } catch { return null; } diff --git a/packages/plugins/plugin-sharing/src/manager-org-screen.test.ts b/packages/plugins/plugin-sharing/src/manager-org-screen.test.ts new file mode 100644 index 0000000000..78a1f253e0 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/manager-org-screen.test.ts @@ -0,0 +1,230 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10231] `managerOf` honours its declared `organizationId`. + * + * `ITeamGraphService.managerOf(userId, organizationId?)` declares the + * organization parameter; the implementation used to spell it `_organizationId` + * and drop it, while `expandRoleUsers` on the SAME class applied + * `organization_id` to its read. + * + * ## Why both directions are pinned, and why the positive half is the longer one + * + * This is a security seam, so over-screening is a defect of the same rank as + * under-screening: a `managerOf` that returned nothing would silently empty + * every approver slate and every `manager` sharing recipient, and the + * surrounding `catch` blocks would make that look like "no manager on file" + * rather than like a fault. The screen therefore reads `sys_member` (the ONLY + * table carrying a tenancy fact for a user — `sys_user` has no + * `organization_id`), and it is fail-open on an ABSENT fact. + * + * Note the fixture below mirrors that: no `sys_user` row carries an + * `organization_id`, because no `sys_user` row can. A fixture that invented one + * would let a `where: { id, organization_id }` implementation pass here and + * return null against every real driver. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { TeamGraphService } from './team-graph.js'; +import { BusinessUnitGraphService } from './business-unit-graph.js'; + +interface Row { [k: string]: any } + +/** + * Minimal engine faithful to the two predicate shapes these paths use. Records + * every read so a test can assert that the no-organization path issues NO + * membership query at all — "unchanged when absent" is a claim about the reads + * as much as about the return value. + */ +function makeEngine() { + const tables: Record = {}; + const reads: string[] = []; + let throwOnMember = false; + function matches(row: Row, f: any): boolean { + if (!f || typeof f !== 'object') return true; + for (const [k, v] of Object.entries(f)) { + if (row[k] !== v) return false; + } + return true; + } + return { + _tables: tables, + _reads: reads, + _throwOnMember: (v: boolean) => { throwOnMember = v; }, + async find(object: string, options?: any): Promise { + reads.push(object); + if (object === 'sys_member' && throwOnMember) throw new Error('membership store unavailable'); + const predicate = options?.where ?? options?.filter ?? {}; + return (tables[object] ?? []).filter((r) => matches(r, predicate)); + }, + async insert() { return {}; }, + async update() { return {}; }, + async delete() { return {}; }, + }; +} + +function seed(engine: ReturnType) { + // ⛔ No `organization_id` on any row here — sys_user is the global + // better-auth identity table and has no such column (ADR-0010 section 3.7). + engine._tables.sys_user = [ + { id: 'alice', manager_id: 'bob' }, // bob: member of org1 + { id: 'dave', manager_id: 'eve' }, // eve: member of org2 ONLY + { id: 'frank', manager_id: 'ghost' }, // ghost: no membership rows at all + { id: 'heidi', manager_id: 'ivan' }, // ivan: member of BOTH org1 and org2 + { id: 'carol', manager_id: null }, + ]; + engine._tables.sys_member = [ + { id: 'm1', user_id: 'bob', organization_id: 'org1', role: 'sales_rep' }, + { id: 'm2', user_id: 'eve', organization_id: 'org2', role: 'sales_rep' }, + { id: 'm3', user_id: 'ivan', organization_id: 'org1', role: 'sales_rep' }, + { id: 'm4', user_id: 'ivan', organization_id: 'org2', role: 'sales_rep' }, + ]; +} + +describe('[#10231] TeamGraphService.managerOf — POSITIVE direction (over-screening guard)', () => { + let engine: ReturnType; + beforeEach(() => { engine = makeEngine(); seed(engine); }); + + it('returns a manager who IS a member of the caller organization', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await g.managerOf('alice', 'org1')).toEqual('bob'); + }); + + it('returns a manager with NO membership rows at all (absent fact => fail open)', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await g.managerOf('frank', 'org1')).toEqual('ghost'); + }); + + it('returns a manager who holds membership in the caller org AND elsewhere', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await g.managerOf('heidi', 'org1')).toEqual('ivan'); + }); + + it('returns the manager when the membership read FAILS (infrastructure => fail open)', async () => { + engine._throwOnMember(true); + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + // dave's manager is provably outside org1 — but the fact is unreadable, so + // routing must be left exactly as it was rather than emptied on a hiccup. + expect(await g.managerOf('dave', 'org1')).toEqual('eve'); + }); + + it('still returns null for a user with no manager, without inventing a screen', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await g.managerOf('carol', 'org1')).toBeNull(); + expect(engine._reads.filter((r) => r === 'sys_member')).toEqual([]); + }); + + it('returns null for an empty user id', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await g.managerOf('', 'org1')).toBeNull(); + }); +}); + +describe('[#10231] TeamGraphService.managerOf — NEGATIVE direction (the screen)', () => { + let engine: ReturnType; + beforeEach(() => { engine = makeEngine(); seed(engine); }); + + it('screens out a manager provably outside the caller organization', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await g.managerOf('dave', 'org1')).toBeNull(); + }); + + it('screens using the INSTANCE organization when the argument is omitted (expandRoleUsers parity)', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await g.managerOf('dave')).toBeNull(); + }); + + it('the explicit argument WINS over the instance organization', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + // eve is a member of org2, so asking as org2 must return her even though + // the instance is scoped to org1. + expect(await g.managerOf('dave', 'org2')).toEqual('eve'); + }); +}); + +describe('[#10231] TeamGraphService.managerOf — ABSENT organization is UNCHANGED', () => { + let engine: ReturnType; + beforeEach(() => { engine = makeEngine(); seed(engine); }); + + it('returns the cross-organization manager unchanged when no organization is in play', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: null }); + expect(await g.managerOf('dave')).toEqual('eve'); + expect(await g.managerOf('alice')).toEqual('bob'); + }); + + it('issues NO membership read at all when no organization is in play', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: null }); + await g.managerOf('dave'); + expect(engine._reads).toEqual(['sys_user']); + expect(engine._reads).not.toContain('sys_member'); + }); + + it('an undefined argument on an unscoped instance does not screen', async () => { + const g = new TeamGraphService({ engine: engine as any }); + expect(await g.managerOf('dave', undefined)).toEqual('eve'); + }); +}); + +describe('[#10231] TeamGraphService.managerOf — the cache is organization-qualified', () => { + let engine: ReturnType; + beforeEach(() => { engine = makeEngine(); seed(engine); }); + + it('a screened null for one organization does not leak to an unscoped read', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: null }); + expect(await g.managerOf('dave', 'org1')).toBeNull(); // screened + // A user-keyed cache would serve that null here and turn one screened + // caller into a permanent outage for every unscoped reader behind it. + expect(await g.managerOf('dave')).toEqual('eve'); + }); + + it('answers for two different organizations do not overwrite each other', async () => { + const g = new TeamGraphService({ engine: engine as any, organizationId: null }); + expect(await g.managerOf('dave', 'org2')).toEqual('eve'); + expect(await g.managerOf('dave', 'org1')).toBeNull(); + expect(await g.managerOf('dave', 'org2')).toEqual('eve'); + }); +}); + +describe('[#10231] BusinessUnitGraphService.managerOf — standalone fallback', () => { + let engine: ReturnType; + beforeEach(() => { engine = makeEngine(); seed(engine); }); + + it('screens on the standalone fallback (no teamGraph supplied)', async () => { + const d = new BusinessUnitGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await d.managerOf('dave', 'org1')).toBeNull(); + }); + + it('returns an in-organization manager on the standalone fallback', async () => { + const d = new BusinessUnitGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await d.managerOf('alice', 'org1')).toEqual('bob'); + }); + + it('fails open on the standalone fallback when no membership fact exists', async () => { + const d = new BusinessUnitGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await d.managerOf('frank', 'org1')).toEqual('ghost'); + }); + + it('leaves the standalone fallback unchanged when no organization is in play', async () => { + const d = new BusinessUnitGraphService({ engine: engine as any, organizationId: null }); + expect(await d.managerOf('dave')).toEqual('eve'); + expect(engine._reads).not.toContain('sys_member'); + }); + + it('screens using the INSTANCE organization on the standalone fallback', async () => { + const d = new BusinessUnitGraphService({ engine: engine as any, organizationId: 'org1' }); + expect(await d.managerOf('dave')).toBeNull(); + }); + + it('the delegating limb screens identically to the standalone one', async () => { + const team = new TeamGraphService({ engine: engine as any, organizationId: 'org1' }); + const delegating = new BusinessUnitGraphService({ + engine: engine as any, organizationId: 'org1', teamGraph: team, + }); + const standalone = new BusinessUnitGraphService({ engine: engine as any, organizationId: 'org1' }); + // Same method name, same inputs — the answer must not depend on whether a + // teamGraph happened to be passed to the constructor. + expect(await delegating.managerOf('dave', 'org1')).toEqual(await standalone.managerOf('dave', 'org1')); + expect(await delegating.managerOf('dave', 'org1')).toBeNull(); + expect(await delegating.managerOf('alice', 'org1')).toEqual('bob'); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/team-graph.ts b/packages/plugins/plugin-sharing/src/team-graph.ts index 29747a7515..9d7edec4b2 100644 --- a/packages/plugins/plugin-sharing/src/team-graph.ts +++ b/packages/plugins/plugin-sharing/src/team-graph.ts @@ -8,6 +8,14 @@ const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; type Cache = { expandUsers?: Map; expandRole?: Map; + /** + * [#10231] Keyed `org::userId`, NOT bare `userId`. The answer is now + * org-dependent (a manager across a tenant boundary is screened out), so a + * user-only key would serve one caller's screened `null` to the next caller + * that asked without an organization — turning a screen into a permanent + * outage for every unscoped reader that happened to run second. Mirrors the + * composite key {@link Cache.expandRole} has carried for the same reason. + */ manager?: Map; }; @@ -32,6 +40,73 @@ export interface TeamGraphOptions { * metadata; callers (sharing rule evaluator, approval engine) own their * own enforcement. */ +/** + * Is `managerId` PROVABLY a member of other organizations and not of + * `organizationId`? (#10231) + * + * This is the screen `ITeamGraphService.managerOf`'s declared `organizationId` + * parameter asks for. It is deliberately the SAME shape #10153 landed for the + * identical column on the approvals side (`managerIsProvablyOutsideOrg` in + * `plugin-approvals`): both are consumers of one fact — `sys_user.manager_id` — + * and a screen that differed between them would route an approval one way and + * share a record the other. + * + * ## Why this reads `sys_member` instead of filtering the `sys_user` read + * + * `sys_user` carries NO `organization_id`. It is the global better-auth + * identity table (`managedBy: 'better-auth'`, `protection.lock: 'full'`, + * ADR-0010 section 3.7), so a membership row is the only tenancy fact that + * exists for a user. Adding `organization_id` to the `sys_user` predicate is + * not a tighter version of this screen, it is a broken one: the column does + * not exist, so the predicate matches NOTHING and every manager lookup given + * an organization returns null. That failure is SILENT — the surrounding + * `catch` swallows the driver's complaint — which is exactly why the screen is + * a second, explicit read rather than one more key in the first one. + * + * ## Why "provably outside" rather than "must prove membership" + * + * - membership rows exist for this user, none in the caller's org + * => the tenancy fact is present and NEGATIVE => screen him out; + * - no membership rows at all, or the read failed + * => the tenancy fact is ABSENT => leave the answer exactly as it was. + * + * The fail-open half is the ruled posture for this column (#10153), not + * timidity, and it is load-bearing: a stack that stamps an organization on its + * callers but never materializes `sys_member` rows would otherwise lose every + * manager at once — a far bigger behaviour change than the hole being closed. + * + * ⛔ Not exported from the package index. This is the screen's ONE + * implementation, shared with {@link BusinessUnitGraphService}'s standalone + * fallback so the two cannot drift; it is not a public affordance. + */ +export async function managerIsProvablyOutsideOrg( + engine: SharingEngine, + managerId: string, + organizationId?: string | null, +): Promise { + const callerOrg = organizationId ? String(organizationId) : ''; + // No organization in play => nothing to screen against, and NO read. The + // ordinary single-organization / embedded stack pays nothing here, and the + // absent-organization path stays byte-identical to its pre-#10231 shape. + if (!callerOrg) return false; + let rows: any[] = []; + try { + rows = await engine.find('sys_member', { + where: { user_id: managerId }, + fields: ['user_id', 'organization_id'], + limit: 1000, + context: SYSTEM_CTX, + }); + } catch { + return false; // membership unreadable — see the fail-open note above + } + const orgs = (rows ?? []) + .map((r: any) => String(r?.organization_id ?? '')) + .filter(Boolean); + if (!orgs.length) return false; // no tenancy fact recorded for this user + return !orgs.includes(callerOrg); // member here => route as before +} + export class TeamGraphService implements ITeamGraphService { private readonly engine: SharingEngine; private readonly organizationId: string | null; @@ -91,9 +166,24 @@ export class TeamGraphService implements ITeamGraphService { return users; } - async managerOf(userId: string, _organizationId?: string): Promise { + /** + * [#10231] The declared `organizationId` is HONOURED, not discarded. + * + * It used to be spelled `_organizationId` — accepted and dropped — while the + * sibling {@link TeamGraphService.expandRoleUsers} on this same class applied + * `organization_id` to its read. That asymmetry was not a considered posture: + * the two methods differ only in which table backs them, and `sys_user` + * (unlike `sys_member`) has no organization column to filter on. The tenancy + * fact lives one table over; see {@link managerIsProvablyOutsideOrg}. + * + * Resolution order matches `expandRoleUsers` exactly — the argument, else the + * instance's `organizationId`, else no screen at all. + */ + async managerOf(userId: string, organizationId?: string): Promise { if (!userId) return null; - if (this.cache.manager!.has(userId)) return this.cache.manager!.get(userId) ?? null; + const org = organizationId ?? this.organizationId; + const key = `${org ?? '*'}::${userId}`; + if (this.cache.manager!.has(key)) return this.cache.manager!.get(key) ?? null; let row: any = null; try { const rows = await this.engine.find('sys_user', { @@ -106,8 +196,9 @@ export class TeamGraphService implements ITeamGraphService { } catch { row = null; } - const mgr = row?.manager_id ? String(row.manager_id) : null; - this.cache.manager!.set(userId, mgr); + let mgr = row?.manager_id ? String(row.manager_id) : null; + if (mgr && (await managerIsProvablyOutsideOrg(this.engine, mgr, org))) mgr = null; + this.cache.manager!.set(key, mgr); return mgr; } } From a620ecdf806788d0c390d404c40470f3bb559e6e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:15:08 +0000 Subject: [PATCH 2/2] test(sharing): keep the #10231 engine double read-only and its matcher honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The double declared unused insert/update/delete members, enrolling it in the engine-double dispatch contract for methods managerOf never reaches, and its WHERE matcher walked Object.entries unconditionally — it would have read a combinator key as a field name and answered "no match" instead of refusing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 --- .../src/manager-org-screen.test.ts | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/plugins/plugin-sharing/src/manager-org-screen.test.ts b/packages/plugins/plugin-sharing/src/manager-org-screen.test.ts index 78a1f253e0..fe2cbc76d4 100644 --- a/packages/plugins/plugin-sharing/src/manager-org-screen.test.ts +++ b/packages/plugins/plugin-sharing/src/manager-org-screen.test.ts @@ -40,13 +40,33 @@ function makeEngine() { const tables: Record = {}; const reads: string[] = []; let throwOnMember = false; + /** + * Plain-equality matcher, and it REFUSES anything else rather than guessing. + * + * Both predicates on these paths are flat equality (`{ id }` and + * `{ user_id }`). A matcher that walked `Object.entries` unconditionally + * would read a combinator key like `$or` as a FIELD NAME, compare it against + * `row['$or']`, and quietly answer "no match" — a fake looser (or here, + * blinder) than the drivers it stands in for, which is how a green suite + * ships a broken filter. Refusing is the honest failure: if one of these + * reads ever grows a combinator, this throws instead of returning a + * confidently wrong row set. + */ function matches(row: Row, f: any): boolean { if (!f || typeof f !== 'object') return true; for (const [k, v] of Object.entries(f)) { + if (k.startsWith('$')) throw new Error(`unsupported combinator '${k}' in this fake`); + if (v !== null && typeof v === 'object') { + throw new Error(`unsupported operator object on field '${k}' in this fake`); + } if (row[k] !== v) return false; } return true; } + // ⛔ Read-only double on purpose: these paths call `find` and nothing else, + // so no `insert`/`update`/`delete` member is declared. Declaring unused + // write members would enrol this double in the engine-double dispatch + // contract for methods the code under test never reaches. return { _tables: tables, _reads: reads, @@ -57,9 +77,6 @@ function makeEngine() { const predicate = options?.where ?? options?.filter ?? {}; return (tables[object] ?? []).filter((r) => matches(r, predicate)); }, - async insert() { return {}; }, - async update() { return {}; }, - async delete() { return {}; }, }; }