diff --git a/.changeset/29713-business-unit-head-manager-reads-refuse.md b/.changeset/29713-business-unit-head-manager-reads-refuse.md new file mode 100644 index 00000000000..3f85c88a98a --- /dev/null +++ b/.changeset/29713-business-unit-head-manager-reads-refuse.md @@ -0,0 +1,21 @@ +--- +'@objectstack/plugin-sharing': patch +--- + +fix(plugin-sharing)!: a failed business-unit head or standalone manager read refuses instead of answering no head or no manager + +Clause-②: no (narrowing) +Metadata-protocol: none + + + +**BREAKING** only where the read fails: `BusinessUnitGraphService.headOf` and the standalone `BusinessUnitGraphService.managerOf` used to answer `null` then, and now refuse. + +The member, seed and descent reads of `BusinessUnitGraphService` already refuse on a failed read. Two sibling lookups on the same service did not. + +- **`headOf(businessUnitId)`.** A failed `sys_business_unit` read now throws. Before, it answered `null` (no head) and kept that answer for the rest of the pass, so the head stayed missing after the read recovered. Nothing is cached after a failed read now: the next call reads again. +- **`managerOf(userId, organizationId)` without a `TeamGraphService`.** A failed `sys_user` read now throws. Before, it answered `null` (no manager). This fallback caches nothing, before or after. The organization screen keeps its fail-open posture, and a service built with a `teamGraph` still answers exactly what `TeamGraphService.managerOf` answers. +- **A missing object is still nobody.** A stack that never registers, or never creates, `sys_business_unit` or `sys_user` keeps answering `null`. That is judged by `isObjectAbsentError`; any other error stays loud. +- **No caller on this release changes behaviour.** No sharing rule, approval or ownership queue on this release calls either lookup. A caller of the exported class sees the refusal instead of `null`. + +**The remedy.** None is needed. A refusal means the business-unit or user table could not be read: fix that read, and the next lookup succeeds. diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index a27bf6e27fa..ffdef39d42d 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -4,8 +4,6 @@ import type { IBusinessUnitGraphService } from '@objectstack/spec/contracts'; import type { SharingEngine } from './sharing-service.js'; import { TeamGraphService, managerIsProvablyOutsideOrg, readMembershipToCompletion } from './team-graph.js'; -const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; - /** * [#29655] Rows per page of a child-unit read — the bound the read always * carried, so a parent with fewer children issues the one read it always did, @@ -261,21 +259,27 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { return Array.from(new Set(rows.map((r) => String(r?.user_id ?? '')).filter(Boolean))); } + /** + * The unit's head (`manager_user_id`), or `null` when the unit has none. + * + * [#29713] Read through the walk {@link seedIsUsable} uses + * ({@link readMembershipToCompletion}), so a read that FAILS throws and + * nothing is cached for it. It caught any error to `null` and cached that + * for the pass: a consumer naming a unit's head would have dropped the head + * on an outage, and kept dropping it after the read recovered. A unit that + * does not exist, sits in another organization or names no head answers + * `null`, and so does a stack that never materialises `sys_business_unit`. + */ async headOf(businessUnitId: string): Promise { if (!businessUnitId) return null; if (this.cache.head!.has(businessUnitId)) return this.cache.head!.get(businessUnitId) ?? null; - let row: any = null; - try { - const rows = await this.engine.find('sys_business_unit', { - where: this.orgScope({ id: businessUnitId }), - fields: ['id', 'manager_user_id'], - limit: 1, - context: SYSTEM_CTX, - }); - row = Array.isArray(rows) ? rows[0] : null; - } catch { - row = null; - } + const rows = await readMembershipToCompletion(this.engine, { + object: 'sys_business_unit', + where: this.orgScope({ id: businessUnitId }), + fields: ['manager_user_id'], + subject: `the head of business unit '${businessUnitId}'`, + }); + const row = rows?.[0]; const head = row?.manager_user_id ? String(row.manager_user_id) : null; this.cache.head!.set(businessUnitId, head); return head; @@ -291,29 +295,34 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { * 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. + * + * [#29713] The standalone fallback's `sys_user` read goes through + * {@link readMembershipToCompletion}: a read that FAILS throws, where it + * answered `null` ("no manager"); a stack that never materialises + * `sys_user` has no manager on file and answers `null`. The fallback caches + * nothing, before or after. The organization screen keeps its ruled + * fail-open posture ({@link managerIsProvablyOutsideOrg}), and the + * delegating limb is {@link TeamGraphService.managerOf}'s own answer, + * unchanged here. */ 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 }, - fields: ['id', 'manager_id'], - limit: 1, - context: SYSTEM_CTX, - }); - const row: any = Array.isArray(rows) ? rows[0] : 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; - } + const rows = await readMembershipToCompletion(this.engine, { + object: 'sys_user', + where: { id: userId }, + fields: ['manager_id'], + subject: `the manager of user '${userId}'`, + }); + const row = rows?.[0]; + 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; } /** @@ -358,7 +367,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { * ## Why the member rows are screened at all * * Both member reads used to carry no organization predicate whatever, under - * a {@link SYSTEM_CTX} that carries no tenant either — so the query was + * a system context that carries no tenant either — so the query was * completely unscoped by organization: any unit an org-stamped rule could * see handed back EVERY tenant's membership rows hanging off it, and tenant * A's rule materialised real `sys_record_share` rows for tenant B's users. diff --git a/packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts b/packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts new file mode 100644 index 00000000000..3a4bbbc0cb1 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts @@ -0,0 +1,208 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#29713] `BusinessUnitGraphService.headOf` and the standalone `managerOf` + * fallback answer the real head or manager, or refuse — never an invented + * "nobody" on a read that failed. #29606 and #29655 closed the member, seed + * and descent reads of the same service; these were the two left. + * + * 1. `headOf` caught any error from its `sys_business_unit` read to `null` + * and cached that `null` for the pass, so the head stayed missing after + * the read recovered. + * 2. The standalone `managerOf` (no `TeamGraphService` wired) caught any + * error from its `sys_user` read to `null`. It caches nothing. + * + * Absence is the one failure that IS an answer — the object is not + * registered, or its table was never created — judged by `isObjectAbsentError` + * through `readMembershipToCompletion`, the walk `seedIsUsable` reads with. + * + * Measured on a REAL engine (ObjectQL + SqlDriver over in-memory SQLite) + * carrying the shipped identity objects, so "absent" is what a real driver + * answers for a missing object or table, not what a double guesses. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SysBusinessUnit, SysMember, SysOrganization, SysUser } from '@objectstack/platform-objects/identity'; +import { BusinessUnitGraphService } from './business-unit-graph.js'; +import { TeamGraphService } from './team-graph.js'; + +const ORG = 'org_head'; +const UNIT = 'bu_sales'; +const HEADLESS = 'bu_headless'; +const HEAD = 'usr_head'; +const ALICE = 'usr_alice'; +const BOB = 'usr_bob'; + +interface Rig { + engine: ObjectQL; + driver: SqlDriver; + /** Fail the next `times` reads of `object`; every other read runs as before. */ + failReads: (object: 'sys_business_unit' | 'sys_user', times?: number) => void; +} + +const engines: ObjectQL[] = []; +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +/** UNIT headed by HEAD, HEADLESS with no head; ALICE managed by BOB, both members of ORG. */ +async function boot(opts: { omit?: string[] } = {}): Promise { + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never); + const engine = new ObjectQL(); + engine.registerDriver(driver as never, true); + await engine.init(); + const omit = new Set(opts.omit ?? []); + engine.registerApp({ + id: 'com.objectstack.qa.business-unit-head-manager-reads', + name: 'business unit head and manager reads', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [SysOrganization, SysBusinessUnit, SysUser, SysMember].filter((o) => !omit.has(o.name)), + } as never); + await engine.syncSchemas(); + engines.push(engine); + + const e = engine as any; + await e.insert('sys_organization', { id: ORG, name: ORG, slug: 'head' }, { context: { isSystem: true } }); + if (!omit.has('sys_business_unit')) { + await e.getDriverForObject('sys_business_unit').bulkCreate('sys_business_unit', [ + { id: UNIT, name: 'Sales', kind: 'department', active: true, manager_user_id: HEAD, organization_id: ORG }, + { id: HEADLESS, name: 'Headless', kind: 'department', active: true, organization_id: ORG }, + ]); + } + if (!omit.has('sys_user')) { + await e.getDriverForObject('sys_user').bulkCreate('sys_user', [ + { id: ALICE, name: 'Alice', email: 'alice@example.test', manager_id: BOB }, + { id: BOB, name: 'Bob', email: 'bob@example.test' }, + ]); + } + await e.getDriverForObject('sys_member').bulkCreate('sys_member', [ + { id: 'mem_alice', user_id: ALICE, organization_id: ORG, role: 'member' }, + { id: 'mem_bob', user_id: BOB, organization_id: ORG, role: 'member' }, + ]); + + // The driver's own `find`, taken once: a second fault wraps it again rather + // than wrapping the first fault's spy. + const d = e.getDriverForObject('sys_organization'); + const real = d.find.bind(d); + const failReads: Rig['failReads'] = (object, times = Number.POSITIVE_INFINITY) => { + let left = times; + vi.spyOn(d, 'find').mockImplementation(((obj: string, ...rest: unknown[]) => { + if (obj === object && left > 0) { + left -= 1; + return Promise.reject(Object.assign(new Error('SQLITE_IOERR: disk I/O error'), { code: 'SQLITE_IOERR' })); + } + return real(obj, ...rest); + }) as never); + }; + return { engine, driver, failReads }; +} + +const graph = (rig: Rig, extra: { teamGraph?: TeamGraphService } = {}) => + new BusinessUnitGraphService({ engine: rig.engine as never, organizationId: ORG, ...extra }); + +describe('[#29713] headOf: a failed unit read refuses, and nothing is cached for it', () => { + it('control — a healthy read answers the head, and a unit with no head answers null', async () => { + const g = graph(await boot()); + + expect(await g.headOf(UNIT)).toBe(HEAD); + expect(await g.headOf(HEADLESS)).toBeNull(); + }); + + it('⛔ a failed read refuses — it never answers an invented null', async () => { + const rig = await boot(); + rig.failReads('sys_business_unit'); + + await expect(graph(rig).headOf(UNIT)).rejects.toThrow( + /\[sharing\] the head of business unit 'bu_sales' could not be read: SQLITE_IOERR/, + ); + }); + + it('⛔ a failed read is not cached: the next read on the same pass answers the real head', async () => { + const rig = await boot(); + rig.failReads('sys_business_unit', 1); + const g = graph(rig); + + await expect(g.headOf(UNIT)).rejects.toThrow(/could not be read/); + expect(await g.headOf(UNIT)).toBe(HEAD); + }); + + it('absence — a composition that never registers sys_business_unit has no head: null', async () => { + const g = graph(await boot({ omit: ['sys_business_unit'] })); + + expect(await g.headOf(UNIT)).toBeNull(); + }); + + it('absence — a stack whose sys_business_unit table was never created has no head: null', async () => { + const rig = await boot(); + await (rig.driver as any).knex.schema.dropTable('sys_business_unit'); + + expect(await graph(rig).headOf(UNIT)).toBeNull(); + }); +}); + +describe('[#29713] standalone managerOf: a failed user read refuses', () => { + it('control — a healthy read answers the manager, and a user with none answers null', async () => { + const g = graph(await boot()); + + expect(await g.managerOf(ALICE, ORG)).toBe(BOB); + expect(await g.managerOf(BOB, ORG)).toBeNull(); + }); + + it('⛔ a failed read refuses — it never answers an invented null', async () => { + const rig = await boot(); + rig.failReads('sys_user'); + + await expect(graph(rig).managerOf(ALICE, ORG)).rejects.toThrow( + /\[sharing\] the manager of user 'usr_alice' could not be read: SQLITE_IOERR/, + ); + }); + + it('⛔ a failed read is not cached: the next read on the same pass answers the real manager', async () => { + const rig = await boot(); + rig.failReads('sys_user', 1); + const g = graph(rig); + + await expect(g.managerOf(ALICE, ORG)).rejects.toThrow(/could not be read/); + expect(await g.managerOf(ALICE, ORG)).toBe(BOB); + }); + + it('absence — a composition that never registers sys_user has no manager on file: null', async () => { + const g = graph(await boot({ omit: ['sys_user'] })); + + expect(await g.managerOf(ALICE, ORG)).toBeNull(); + }); + + it('absence — a stack whose sys_user table was never created has no manager on file: null', async () => { + const rig = await boot(); + await (rig.driver as any).knex.schema.dropTable('sys_user'); + + expect(await graph(rig).managerOf(ALICE, ORG)).toBeNull(); + }); +}); + +describe('[#29713] CONTROL — the wired TeamGraphService limb is unchanged', () => { + it('a wired managerOf answers exactly what TeamGraphService.managerOf answers, on a healthy and on a failed read', async () => { + const rig = await boot(); + const team = new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }); + const g = graph(rig, { teamGraph: team }); + + expect(await g.managerOf(ALICE, ORG)).toBe(BOB); + expect(await g.managerOf(ALICE, ORG)).toBe(await new TeamGraphService({ engine: rig.engine as never }).managerOf(ALICE, ORG)); + + // TeamGraphService's own failure posture, not this card's: it is passed + // through as it stands, and this card changes nothing on that limb. + rig.failReads('sys_user'); + const failed = graph(rig, { teamGraph: new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }) }); + expect(await failed.managerOf(ALICE, ORG)).toBeNull(); + expect(await failed.managerOf(ALICE, ORG)).toBe( + await new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }).managerOf(ALICE, ORG), + ); + }); +});