fix(plugin-approvals): screen expanded business-unit members to the directory organization (#14946)

.changeset/approvals-bu-member-org-screen.md

@@ -0,0 +1,9 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+Fix: a `department` approver on a seeded business unit no longer routes the approval to another organization's members.
+
+`ApprovalService.expandBusinessUnitUsers` screened the `sys_business_unit` rows with the null-inclusive tenant predicate (#3807 — a seeded unit carries no organization and is admitted on purpose) but read `sys_business_unit_member` with no organization predicate at all, under a system context that carries no tenant either. A seeded unit id exists identically in every tenant, so a `department:<id>` approver on tenant A's request resolved the shared unit and then collected every tenant's membership rows hanging off it — approval authority over A's record, routed to B's users. The member read now carries a strict `organization_id` equality against the directory organization the approver resolves in: the same screen `plugin-sharing` applies to these rows, and the same posture this package already takes for `sys_team_member` and `sys_user_position`.
+
+The screen is strict rather than null-inclusive on purpose. `sys_business_unit_member.organization_id` is filled by REST/session writes but left NULL by seed replay and by elevated system-context writes (tracked in #14570), so a NULL on a membership row means unknown tenancy, not "platform-global", and routing fails closed on it. Declared cost: on a deployment whose membership rows (not merely its units) were seeded or system-written, a `department` approver on a request that carries an organization now expands to nobody — the slot falls to the `department:<id>` literal, the existing `expanded to nobody` warning (#3807) names it, and `onEmptyApprovers` governs the request as for any unstaffed target. The repair is to stamp those membership rows. A request that carries no organization is unchanged, and so is every unit-level screen.

content/docs/permissions/system-context.mdx

@@ -145,7 +145,7 @@ The largest single consumer — **17 of the 106 sites**.
 |:--|:---|:---|:---|:---|
 | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` |
 | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` |
-| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3248`, `:3396`, `:3564`, `:3635`, `:3824`, `:3864` |
+| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3305`, `:3453`, `:3621`, `:3692`, `:3881`, `:3921` |
 | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` |
 | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` |
 | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` |

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -429,10 +429,12 @@ describe('ApprovalService (node era)', () => {
       { id: 'd1', active: true, organization_id: 't1' },
       { id: 'd2', active: true, organization_id: 't1' },
     ];
+    // #14946: membership rows are stamped (the session write path fills
+    // `organization_id`); the member read is now strictly screened to it.
     engine._tables['sys_business_unit_member'] = [
-      { id: 'm1', business_unit_id: 'd1', user_id: 'u2' },
-      { id: 'm2', business_unit_id: 'd1', user_id: 'u3' },
-      { id: 'm3', business_unit_id: 'd2', user_id: 'u4' },
+      { id: 'm1', business_unit_id: 'd1', user_id: 'u2', organization_id: 't1' },
+      { id: 'm2', business_unit_id: 'd1', user_id: 'u3', organization_id: 't1' },
+      { id: 'm3', business_unit_id: 'd2', user_id: 'u4', organization_id: 't1' },
     ];
     const req = await svc.openNodeRequest(
       exprInput('vars.picked_departments', {
@@ -913,8 +915,8 @@ describe('ApprovalService (node era)', () => {
       { id: 'bu2', parent_business_unit_id: 'bu1', organization_id: 't1', active: true },
     ];
     engine._tables['sys_business_unit_member'] = [
-      { id: 'bm1', business_unit_id: 'bu1', user_id: 'u5' },
-      { id: 'bm2', business_unit_id: 'bu2', user_id: 'u6' },
+      { id: 'bm1', business_unit_id: 'bu1', user_id: 'u5', organization_id: 't1' },
+      { id: 'bm2', business_unit_id: 'bu2', user_id: 'u6', organization_id: 't1' },
     ];
     const req = await svc.openNodeRequest(positionInput({
       config: {
@@ -944,9 +946,13 @@ describe('ApprovalService (node era)', () => {
       { id: 'bu_seeded', organization_id: null, active: true },
       { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: null, active: true },
     ];
+    // #14946: the SEEDED rows are the units; the membership rows are stamped,
+    // as the session write path leaves them. An org-less membership row on a
+    // seeded unit is pinned on its own in
+    // `business-unit-member-org-screen.test.ts` (B3) — it does NOT route.
     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' },
+      { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5', organization_id: 't1' },
+      { id: 'bm2', business_unit_id: 'bu_seeded_child', user_id: 'u6', organization_id: 't1' },
     ];
     const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX);
     // Both the seed check AND the subtree descent must see the null-org rows.
@@ -973,8 +979,8 @@ describe('ApprovalService (node era)', () => {
       { 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' },
+      { id: 'bm1', business_unit_id: 'bu_mine', user_id: 'u5', organization_id: 't1' },
+      { id: 'bm2', business_unit_id: 'bu_theirs', user_id: 'intruder', organization_id: 't2' },
     ];
     const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX);
     expect(req.pending_approvers).toEqual(['u5']);

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -1734,7 +1734,60 @@ export class ApprovalService implements IApprovalService {
     return { ...filter, $or: [{ organization_id: organizationId }, { organization_id: null }] };
   }
 
-  /** Recursive department — walks `sys_business_unit.parent_business_unit_id`. */
+  /**
+   * Tenant scope for the `sys_business_unit_member` read — a STRICT equality,
+   * deliberately NOT {@link businessUnitOrgScope} (#14946).
+   *
+   * The two screens answer different questions. The UNIT is the anchor the
+   * approver NAMES, and a seeded unit carries `organization_id = null` by
+   * construction (a seed cannot know the id the runtime mints at boot), so
+   * #3807 admits the null there on purpose. The MEMBER rows are the SET BEING
+   * ROUTED TO — enumerated by the platform, never named by anyone — and a
+   * seeded unit id exists identically in every tenant. Before this screen the
+   * member read carried no organization predicate at all, under
+   * {@link SYSTEM_CTX} which carries no tenant either, so tenant A's request
+   * resolved the shared unit and then collected EVERY tenant's membership rows
+   * hanging off it: approval authority over A's record, routed to B's users.
+   *
+   * Why the null arm is NOT copied here — measured on this tree:
+   *   - `sys_business_unit_member` declares no `organization_id`; the column
+   *     is injected (`applySystemFields`) and the tenancy census lists it in;
+   *   - REST / session writes fill it (`SqlDriver.injectTenantOnInsert`);
+   *   - seed replay does NOT (`seed-loader.ts` withholds its `fallbackOrgId`
+   *     from every `sys_` object), and elevated system-context writes do NOT
+   *     (`unclassified` in `PLATFORM_OBJECT_TENANCY`, tracked as #14570).
+   * So a NULL on a member row means UNKNOWN tenancy, not "platform-global",
+   * and unknown tenancy is not a member of this organization. This is the
+   * ruling `plugin-sharing`'s `memberScope` already applies to the same rows
+   * (#14547 / #14949), and the posture this file already takes for
+   * `sys_team_member` and `sys_user_position`.
+   *
+   * The cost is declared, not hidden: an organization whose MEMBERSHIP rows
+   * were seeded or system-written expands to nobody even on a unit it can
+   * see. That is not silent — the graph-type fallback in `expandApprover`
+   * warns `expanded to nobody` (#3807) and `onEmptyApprovers` governs the
+   * request as for any unstaffed target — and the repair is to stamp the
+   * membership rows, never to widen this screen. ⛔ Do not "unify" the two
+   * screens: one method serving both re-opens whichever half it does not
+   * implement.
+   */
+  private businessUnitMemberScope(
+    filter: Record<string, unknown>,
+    organizationId?: string | null,
+  ): Record<string, unknown> {
+    if (!organizationId) return filter;
+    return { ...filter, organization_id: organizationId };
+  }
+
+  /**
+   * Recursive department — walks `sys_business_unit.parent_business_unit_id`.
+   *
+   * Two tenant screens, and they are different on purpose: the UNIT rows
+   * (seed check and descent) go through the null-inclusive
+   * {@link businessUnitOrgScope}; the MEMBER read goes through the strict
+   * {@link businessUnitMemberScope}. `organizationId` is the DIRECTORY
+   * organization the approver resolves in (ADR-0105 D9), for both.
+   */
   private async expandBusinessUnitUsers(businessUnitId: string, organizationId?: string | null): Promise<string[]> {
     if (!businessUnitId) return [];
     // Seed sanity check: skip if dept doesn't exist or is inactive within tenant.
@@ -1769,7 +1822,11 @@ export class ApprovalService implements IApprovalService {
     let rows: any[] = [];
     try {
       rows = await this.engine.find('sys_business_unit_member', {
-        where: { business_unit_id: { $in: Array.from(seen) } },
+        // #14946: tenant-screened — {@link businessUnitMemberScope} is STRICT
+        // on purpose and is not {@link businessUnitOrgScope}. The units above
+        // proved their tenancy (or are seeded); these rows have not, and the
+        // shared seeded unit id is exactly where other tenants' rows sit.
+        where: this.businessUnitMemberScope({ business_unit_id: { $in: Array.from(seen) } }, organizationId),
         fields: ['user_id'],
         limit: 10000,
         context: SYSTEM_CTX,