fix(sharing): honour the declared organizationId in managerOf (#10231)

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

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<string | null> {
  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;
  }

packages/plugins/plugin-sharing/src/manager-org-screen.test.ts

@@ -0,0 +1,247 @@
+// 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<string, Row[]> = {};
+ 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,
+ _throwOnMember: (v: boolean) => { throwOnMember = v; },
+ async find(object: string, options?: any): Promise<any[]> {
+ 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));
+ },
+ };
+}
+
+function seed(engine: ReturnType<typeof makeEngine>) {
+ // ⛔ 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<typeof makeEngine>;
+ 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<typeof makeEngine>;
+ 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<typeof makeEngine>;
+ 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<typeof makeEngine>;
+ 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<typeof makeEngine>;
+ 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');
+ });
+});