diff --git a/.changeset/department-approver-env-wide-business-unit.md b/.changeset/department-approver-env-wide-business-unit.md new file mode 100644 index 0000000000..3535ba8c46 --- /dev/null +++ b/.changeset/department-approver-env-wide-business-unit.md @@ -0,0 +1,36 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): a `department` approver resolves against env-wide business units (#3807) + +`expandBusinessUnitUsers` scoped its `sys_business_unit` reads with a strict +`organization_id = ` equality, so a unit whose `organization_id` +is `null` was invisible: the seed check found no row, the expansion returned +`[]`, and the approver fell back to the dead `department:` literal that +routes to nobody. + +That is the normal case, not an edge case. An app's org tree is seeded, and a +seed cannot know the organization id the runtime mints at boot, so every seeded +unit carries `organization_id = null` — while an approval request always +carries an org. Every business unit a flow author could pick therefore resolved +to nobody, silently: the request opens, the slate is empty, and (with +`lockRecord`) the record stays locked with no one able to act (#3424 is the +downstream shape of the same dead end). Verified against a live showcase stack: +a `{ type: 'department', value: 'bu_hq_finance' }` approver produced +`pending_approvers: "department:bu_hq_finance"` while the unit's member sat +right there in `sys_business_unit_member`. + +Both the seed check and the subtree descent now scope to **this org ∪ +env-wide** — `$or: [{ organization_id: }, { organization_id: null }]` — +the same predicate `sys_metadata`'s pending-draft listing settled on for the +identical reason (a strict equality silently dropping env-wide rows). The wall +between two organizations is unchanged: another org's unit still fails the +match, and a null-org parent does not drag another org's child unit into the +subtree. + +Note the same strict-equality scope exists in `plugin-sharing`'s +`BusinessUnitGraphService.orgScope`. It is not reachable today — every +materialized `sys_sharing_rule` row carries `organization_id = null`, so the +filter is skipped — and is left alone here rather than widen an +access-granting path on a defect that cannot currently fire. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index a56a547a80..d8bad71f5a 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -616,6 +616,69 @@ describe('ApprovalService (node era)', () => { expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); }); + // #3807 — an app's org tree is normally SEEDED, and a seed cannot know the + // organization id the runtime mints at boot, so those rows carry + // `organization_id = null` while every approval request carries an org. The + // old strict equality made each of them invisible and every `department` + // approver resolved to the dead `department:` literal. + const departmentInput = (value: string) => positionInput({ + config: { + approvers: [{ type: 'department' as const, value }], + behavior: 'first_response' as const, + lockRecord: true, + }, + }); + + it('department approver: an env-wide (null-org) business unit still resolves (#3807)', async () => { + engine._tables['sys_business_unit'] = [ + { id: 'bu_seeded', organization_id: null, active: true }, + { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: null, active: true }, + ]; + engine._tables['sys_business_unit_member'] = [ + { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5' }, + { id: 'bm2', business_unit_id: 'bu_seeded_child', user_id: 'u6' }, + ]; + const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX); + // Both the seed check AND the subtree descent must see the null-org rows. + expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); + }); + + it('department approver: another organization’s unit stays invisible (#3807 keeps the wall)', async () => { + engine._tables['sys_business_unit'] = [ + { id: 'bu_other', organization_id: 't2', active: true }, + { id: 'bu_other_child', parent_business_unit_id: 'bu_other', organization_id: 't2', active: true }, + ]; + engine._tables['sys_business_unit_member'] = [ + { id: 'bm1', business_unit_id: 'bu_other', user_id: 'intruder' }, + { id: 'bm2', business_unit_id: 'bu_other_child', user_id: 'intruder2' }, + ]; + const req = await svc.openNodeRequest(departmentInput('bu_other'), CTX); + expect(req.pending_approvers).toEqual(['department:bu_other']); + }); + + it('department approver: a null-org subtree does not drag in another org’s child unit (#3807)', async () => { + engine._tables['sys_business_unit'] = [ + { id: 'bu_seeded', organization_id: null, active: true }, + { id: 'bu_mine', parent_business_unit_id: 'bu_seeded', organization_id: 't1', active: true }, + { id: 'bu_theirs', parent_business_unit_id: 'bu_seeded', organization_id: 't2', active: true }, + ]; + engine._tables['sys_business_unit_member'] = [ + { id: 'bm1', business_unit_id: 'bu_mine', user_id: 'u5' }, + { id: 'bm2', business_unit_id: 'bu_theirs', user_id: 'intruder' }, + ]; + const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX); + expect(req.pending_approvers).toEqual(['u5']); + }); + + it('department approver: an inactive env-wide unit still contributes nobody (#3807)', async () => { + engine._tables['sys_business_unit'] = [{ id: 'bu_seeded', organization_id: null, active: false }]; + engine._tables['sys_business_unit_member'] = [ + { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5' }, + ]; + const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX); + expect(req.pending_approvers).toEqual(['department:bu_seeded']); + }); + // ── decideNode ────────────────────────────────────────────────── it('decideNode: first_response approve finalizes immediately', async () => { diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 3732d5d3f8..7f677c5f11 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -802,15 +802,41 @@ export class ApprovalService implements IApprovalService { return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean))); } + /** + * Tenant scope for a `sys_business_unit` read that may legitimately be + * env-wide (#3807). + * + * `organization_id = null` on a platform object means "owned by no + * organization" — a row written by a seed, the file layer, or bootstrap, + * i.e. before (or outside) any org exists. A strict + * `organization_id = ` equality made every such row invisible: + * the seed check below found nothing, the whole expansion returned `[]`, and + * the approver fell back to the dead `department:` literal that routes to + * nobody. That is not an edge case — an app's org tree is normally seeded + * (a seed cannot know the org id the runtime mints at boot) while the + * approval request always carries one, so EVERY department approver a + * designer could pick resolved to nobody. + * + * Widen to "this org ∪ env-wide", the same predicate `sys_metadata`'s + * pending-draft listing settled on for the identical reason. Another org's + * unit still fails the match, so the wall between two organizations is + * unchanged — only rows belonging to no org at all become visible. + */ + private businessUnitOrgScope( + filter: Record, + organizationId?: string | null, + ): Record { + if (!organizationId) return filter; + return { ...filter, $or: [{ organization_id: organizationId }, { organization_id: null }] }; + } + /** Recursive department — walks `sys_business_unit.parent_business_unit_id`. */ private async expandBusinessUnitUsers(businessUnitId: string, organizationId?: string | null): Promise { if (!businessUnitId) return []; // Seed sanity check: skip if dept doesn't exist or is inactive within tenant. try { const seed = await this.engine.find('sys_business_unit', { - filter: organizationId - ? { id: businessUnitId, organization_id: organizationId } - : { id: businessUnitId }, + filter: this.businessUnitOrgScope({ id: businessUnitId }, organizationId), fields: ['id', 'active'], limit: 1, context: SYSTEM_CTX, @@ -825,8 +851,10 @@ export class ApprovalService implements IApprovalService { const parent = queue.shift()!; let kids: any[] = []; try { - const filter: any = { parent_business_unit_id: parent, active: { $ne: false } }; - if (organizationId) filter.organization_id = organizationId; + const filter = this.businessUnitOrgScope( + { parent_business_unit_id: parent, active: { $ne: false } }, + organizationId, + ); kids = await this.engine.find('sys_business_unit', { filter, fields: ['id'], limit: 1000, context: SYSTEM_CTX } as any); } catch { kids = []; } for (const k of kids ?? []) {