fix(plugin-approvals)!: refuse a reassign target that resolves to no one

.changeset/25485-reassign-target-resolves.md

@@ -0,0 +1,21 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+fix(plugin-approvals): reassigning an approval to an address that resolves to no one is refused, instead of stranding the request on a slate nobody can decide
+
+Clause-②: no (narrowing)
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any metadata schema is removed, renamed or re-shaped, so there is nothing for `objectstack migrate meta` to rewrite and no tombstone. What changes is which `to` the runtime reassign door accepts in its request body. -->
+
+**BREAKING** for a caller of `POST /api/v1/approvals/requests/:id/reassign` (or `ApprovalService.reassign`) that names a `to` no one can take.
+
+- **What is refused.** FROM: any non-empty `to` that was not already pending was written onto the slate and answered 200, so `{"to":"usr_does_not_exist"}` left the request waiting on nobody until an override actor moved it again. TO: `to` must resolve in the request's organization, or the call answers `400 VALIDATION_FAILED` naming the address, and the slate, the audit trail and the notifications are unchanged:
+  - a user id or an email: a user account carries it (the email as the account stores it), and that user is not a member of other organizations only;
+  - `position:<name>`: someone holds that position in the request's organization;
+  - any other `type:value` address (`team:`, `department:`, `org_membership_level:`, a stored `role:`): never, because no approver acts under it.
+- **Who is affected.** Every caller of the door, the admin rescue of a request whose slate names no one included. A missing user and a user of another organization get the same refusal text, so the answer does not reveal who exists elsewhere.
+- **Unchanged.** A caller who may not move the slot is still refused `403 FORBIDDEN` first, before any directory read. An empty `to`, and a `to` that is already pending, are refused as before. The SLA escalation `reassign` action is not this door and is unchanged.
+
+**What a caller does.** Reassign to the user id or email of a person in the request's organization, or to `position:<name>` for a position someone in it holds.

packages/plugins/plugin-approvals/src/actor-lookups-hold-ids.integration.test.ts

@@ -75,6 +75,19 @@ const sysUser = {
     email: { name: 'email', label: 'Email', type: 'text' as const },
   },
 };
+/**
+ * Just enough of `sys_user_position` for a `position:` reassign target to have
+ * a holder: a reassign target must resolve to someone (#25485).
+ */
+const sysUserPosition = {
+  name: 'sys_user_position',
+  label: 'User Position',
+  fields: {
+    id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
+    user_id: { name: 'user_id', label: 'User', type: 'text' as const },
+    position: { name: 'position', label: 'Position', type: 'text' as const },
+  },
+};
 const deal = {
   name: 'crm_deal',
   label: 'Deal',
@@ -153,14 +166,16 @@ describe('ADR-0118 D1 — every sys_user lookup approvals writes holds an id or
     }), true);
     await engine.init();
     objects = await packageObjects();
-    for (const def of [sysUser, deal, ...objects]) {
+    for (const def of [sysUser, sysUserPosition, deal, ...objects]) {
       engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test');
     }
     await engine.syncSchemas();
     // A real `single` install's Default Organization: the system writes below
     // derive it (ADR-0131 D9), as a fresh NOT NULL tenant column requires (D1).
     await installDefaultOrganization(engine);
     for (const u of USERS) await engine.insert('sys_user', u, { context: SYSTEM } as any);
+    // The `position:finance` reassign target below has a holder.
+    await engine.insert('sys_user_position', { id: 'up_finance', user_id: 'u_boss', position: 'finance' }, { context: SYSTEM } as any);
 
     svc = new ApprovalService({
       engine: engine as any,

packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts

@@ -252,6 +252,8 @@ describe('#4839 evidence — the sanctioned admin path covers an unavailable app
     svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } });
     bindApprovalLockHook(engine as any);
     engine._tables['opportunity'] = [{ id: 'opp1', amount: 100, stage: 'new' }];
+    // The substitute has an account: a reassign target must name one (#25485).
+    engine._tables['sys_user'] = [{ id: 'u7' }];
   });
 
   /** An ordinary member's edit of the approval's target record. */

packages/plugins/plugin-approvals/src/approval-override-audit.test.ts

@@ -102,6 +102,8 @@ describe('approval override audit marker (#4466)', () => {
       engine: engine as any,
       clock: { now: () => new Date(baseTime + (n++) * 1000) },
     });
+    // The reassign target has an account: a reassign target must name one (#25485).
+    engine._tables['sys_user'] = [{ id: 'u7' }];
   });
 
   /** A request whose slate is properly STAFFED — one real, designated user. */

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

@@ -190,6 +190,16 @@ const ADMIN_SESSIONS: Array<[string, any]> = [
 const asUser = (userId: string) =>
   ({ userId, tenantId: 't1', positions: [], permissions: [] }) as any;
 
+/**
+ * Give each of `ids` a `sys_user` row. A reassign target must name an account
+ * (#25485), so a test that hands a slot to a user id staffs the directory with
+ * it first; the doubles start with an empty `sys_user`.
+ */
+function staffDirectory(engine: { _tables: Record<string, FakeRow[]> }, ...ids: string[]): void {
+  const users = (engine._tables['sys_user'] ??= []);
+  for (const id of ids) if (!users.some(u => u.id === id)) users.push({ id });
+}
+
 function nodeConfig(approvers: string[], extra: Record<string, any> = {}) {
   return {
     approvers: approvers.map(v => ({ type: 'user' as const, value: v })),
@@ -1407,6 +1417,7 @@ describe('ApprovalService (node era)', () => {
 
     it('reassign: names the record', async () => {
       const id = await openRequestId(openInput(['u9']), CTX);
+      staffDirectory(engine, 'u7');
       const out = await svc.reassign(id, { actorId: 'u9', to: 'u7' }, asUser('u9'));
       expect(named('approval_reassign', out)).toBe('Reassigned "Acme Renewal"');
     });
@@ -1509,6 +1520,7 @@ describe('ApprovalService (node era)', () => {
 
   it('reassign: hands the slot to a new approver and audits the move on structured fields (#4365)', async () => {
     const req = await svc.openNodeRequest(openInput(['u9', 'u2']), CTX);
+    staffDirectory(engine, 'u7');
     const out = await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, asUser('u9'));
     expect(out.request.pending_approvers).toEqual(['u7', 'u2']);
     const actions = await svc.listActions(req.id, SYS);
@@ -1521,6 +1533,7 @@ describe('ApprovalService (node era)', () => {
 
   it('reassign: a user comment is stored verbatim alongside the structured fields (#4365)', async () => {
     const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+    staffDirectory(engine, 'u7');
     await svc.reassign(req.id, { actorId: 'u9', to: 'u7', comment: 'On leave next week' }, asUser('u9'));
     const actions = await svc.listActions(req.id, SYS);
     expect(actions.at(-1)).toMatchObject({
@@ -1549,6 +1562,7 @@ describe('ApprovalService (node era)', () => {
     // approval-requested-notification.integration.test.ts); this test reads
     // only what the step under test sends after it.
     expect(emitted.splice(0).map(e => e.topic)).toEqual(['approval.requested']);
+    staffDirectory(engine, 'u7');
     await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, asUser('u9'));
     expect(emitted).toHaveLength(1);
     expect(emitted[0]).toMatchObject({ topic: 'approval.reassigned', audience: ['u7'] });
@@ -1560,6 +1574,148 @@ describe('ApprovalService (node era)', () => {
     await expect(svc.reassign(req.id, { actorId: 'u9', to: 'u2' }, asUser('u9'))).rejects.toThrow(/VALIDATION_FAILED/);
   });
 
+  // ── reassign target resolution (#25485) ─────────────────────────
+  //
+  // A `to` that resolves to no one used to be written onto the slate, so the
+  // request waited on nobody until an override actor moved it again. Each
+  // refusal pins all three halves: the named VALIDATION_FAILED, the slate and
+  // its index unchanged, and no audit row or notice for a move that did not
+  // happen. Each preservation pin reads what the slate actually became.
+  describe('reassign target resolution (#25485)', () => {
+    const PLATFORM_ADMIN_CTX = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any;
+    let emitted: any[];
+
+    beforeEach(() => {
+      emitted = [];
+      svc.attachMessaging({ async emit(input) { emitted.push(input); } });
+    });
+
+    /** Open on `slate` and hand back the request id, with the opening's notices cleared. */
+    async function openOn(slate: string[]): Promise<string> {
+      const { id } = (await svc.openNodeRequest(openInput(slate), CTX)) as { id: string };
+      // The request takes the acting context's organization here (no schema on
+      // the double), and every screen below is judged against it.
+      expect(engine._tables['sys_approval_request'].find(r => r.id === id)?.organization_id).toBe('t1');
+      emitted.splice(0);
+      return id;
+    }
+
+    async function expectRefusedUnchanged(id: string, move: Promise<unknown>, message: RegExp, slate: string[]) {
+      await expect(move).rejects.toThrow(message);
+      const row = engine._tables['sys_approval_request'].find(r => r.id === id)!;
+      expect(row.pending_approvers).toBe(slate.join(','));
+      const indexed = (engine._tables['sys_approval_approver'] ?? []).filter(r => r.request_id === id);
+      expect(indexed.map(r => r.approver).sort()).toEqual([...slate].sort());
+      const moves = (engine._tables['sys_approval_action'] ?? []).filter(r => r.request_id === id && r.action === 'reassign');
+      expect(moves).toEqual([]);
+      expect(emitted.filter(e => e.topic === 'approval.reassigned')).toEqual([]);
+    }
+
+    it('refuses a user id no account carries, as the slot holder and as a platform admin (the card\'s two measurements)', async () => {
+      const id = await openOn(['u9']);
+      await expectRefusedUnchanged(
+        id, svc.reassign(id, { actorId: 'u9', to: 'nobody-at-all' }, asUser('u9')),
+        /^VALIDATION_FAILED: cannot reassign to 'nobody-at-all' — it names no user in this request's organization\./, ['u9'],
+      );
+      await expectRefusedUnchanged(
+        id, svc.reassign(id, { actorId: 'root', to: 'usr_does_not_exist', from: 'u9' }, PLATFORM_ADMIN_CTX),
+        /^VALIDATION_FAILED: cannot reassign to 'usr_does_not_exist' — /, ['u9'],
+      );
+    });
+
+    it('refuses the admin rescue onto an unresolvable target too: the override does not exempt the new slot', async () => {
+      const id = await openOn(['position:ghost']);
+      await expectRefusedUnchanged(
+        id, svc.reassign(id, { actorId: 'root', to: 'usr_does_not_exist' }, PLATFORM_ADMIN_CTX),
+        /^VALIDATION_FAILED: cannot reassign to 'usr_does_not_exist' — /, ['position:ghost'],
+      );
+    });
+
+    it('refuses an email no account carries', async () => {
+      engine._tables['sys_user'] = [{ id: 'u7', email: 'ada@example.com' }];
+      const id = await openOn(['u9']);
+      await expectRefusedUnchanged(
+        id, svc.reassign(id, { actorId: 'u9', to: 'grace@example.com' }, asUser('u9')),
+        /^VALIDATION_FAILED: cannot reassign to 'grace@example.com' — it names no user/, ['u9'],
+      );
+    });
+
+    it('refuses a user who is provably a member of other organizations only, in the same words as a missing user', async () => {
+      engine._tables['sys_user'] = [{ id: 'u_other' }];
+      engine._tables['sys_member'] = [{ id: 'm1', user_id: 'u_other', organization_id: 't2' }];
+      const id = await openOn(['u9']);
+      const missing = await svc.reassign(id, { actorId: 'u9', to: 'u_missing' }, asUser('u9')).catch((e: Error) => e.message);
+      const elsewhere = await svc.reassign(id, { actorId: 'u9', to: 'u_other' }, asUser('u9')).catch((e: Error) => e.message);
+      // One text for both, so the answer is not a probe of other tenants' directories.
+      expect(elsewhere).toBe(String(missing).replace('u_missing', 'u_other'));
+      await expectRefusedUnchanged(
+        id, svc.reassign(id, { actorId: 'u9', to: 'u_other' }, asUser('u9')),
+        /^VALIDATION_FAILED: cannot reassign to 'u_other' — it names no user in this request's organization\./, ['u9'],
+      );
+    });
+
+    it('refuses a position no one holds in the request\'s organization, a holder in another one included', async () => {
+      engine._tables['sys_user_position'] = [{ id: 'up1', user_id: 'u_far', position: 'finance', organization_id: 't2' }];
+      const id = await openOn(['u9']);
+      await expectRefusedUnchanged(
+        id, svc.reassign(id, { actorId: 'u9', to: 'position:finance' }, asUser('u9')),
+        /^VALIDATION_FAILED: cannot reassign to 'position:finance' — no one holds position 'finance' in this request's organization\./,
+        ['u9'],
+      );
+    });
+
+    it('refuses every other type:value literal: no actor takes a team, department or membership-tier address', async () => {
+      engine._tables['sys_team'] = [{ id: 'ops', organization_id: 't1' }];
+      const id = await openOn(['u9']);
+      for (const literal of ['team:ops', 'department:sales', 'org_membership_level:admin', 'role:sales_manager']) {
+        await expectRefusedUnchanged(
+          id, svc.reassign(id, { actorId: 'u9', to: literal }, asUser('u9')),
+          new RegExp(`^VALIDATION_FAILED: cannot reassign to '${literal}' — it is a slot literal no approver acts under`),
+          ['u9'],
+        );
+      }
+    });
+
+    it('a caller who may not move the slot is refused FORBIDDEN before the target is resolved: no directory answer leaks', async () => {
+      const id = await openOn(['u9']);
+      const reads = engine._finds.length;
+      await expect(svc.reassign(id, { actorId: 'intruder', to: 'nobody-at-all' }, asUser('intruder')))
+        .rejects.toThrow(/^FORBIDDEN:/);
+      // The caller's OWN account is read (its email is an acting address);
+      // the target is never looked up.
+      expect(engine._finds.slice(reads).filter(f => JSON.stringify(f.options?.where ?? f.options?.filter ?? {})
+        .includes('nobody-at-all'))).toEqual([]);
+    });
+
+    it('moves the slot to a user id of a member here, and to one with no membership rows at all (no tenancy fact, as the sibling screens read it)', async () => {
+      engine._tables['sys_user'] = [{ id: 'u7' }, { id: 'u8' }];
+      engine._tables['sys_member'] = [{ id: 'm1', user_id: 'u7', organization_id: 't1' }, { id: 'm2', user_id: 'u7', organization_id: 't2' }];
+      const id = await openOn(['u9', 'u2']);
+      const first = await svc.reassign(id, { actorId: 'u9', to: 'u7' }, asUser('u9'));
+      expect(first.request.pending_approvers).toEqual(['u7', 'u2']);
+      const second = await svc.reassign(id, { actorId: 'u2', to: 'u8' }, asUser('u2'));
+      expect(second.request.pending_approvers).toEqual(['u7', 'u8']);
+      expect(emitted.filter(e => e.topic === 'approval.reassigned').map(e => e.audience)).toEqual([['u7'], ['u8']]);
+    });
+
+    it('moves the slot to an email an account carries, in the spelling the account stores', async () => {
+      engine._tables['sys_user'] = [{ id: 'u7', email: 'ada@example.com' }];
+      const id = await openOn(['u9']);
+      const out = await svc.reassign(id, { actorId: 'u9', to: 'ada@example.com' }, asUser('u9'));
+      expect(out.request.pending_approvers).toEqual(['ada@example.com']);
+    });
+
+    it('moves the slot to a position someone holds here, and its holder then takes it', async () => {
+      engine._tables['sys_user_position'] = [{ id: 'up1', user_id: 'u_fin', position: 'finance', organization_id: 't1' }];
+      const id = await openOn(['u9']);
+      const out = await svc.reassign(id, { actorId: 'u9', to: 'position:finance' }, asUser('u9'));
+      expect(out.request.pending_approvers).toEqual(['position:finance']);
+      const decided = await svc.decideNode(id, { decision: 'approve' } as any,
+        { userId: 'u_fin', tenantId: 't1', positions: ['finance'], permissions: [] } as any);
+      expect(decided.request.status).toBe('approved');
+    });
+  });
+
   it('remind: notifies pending approvers, audits, and throttles repeats', async () => {
     const emitted: any[] = [];
     svc.attachMessaging({ async emit(input) { emitted.push(input); } });
@@ -1674,6 +1830,7 @@ describe('ApprovalService (node era)', () => {
     expect(await svc.redeemActionToken(short.approve)).toMatchObject({ ok: false, reason: 'expired' });
 
     const live = await svc.issueActionTokens(req.id, 'u9');
+    staffDirectory(engine, 'u7');
     await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, asUser('u9'));
     expect(await svc.redeemActionToken(live.approve)).toMatchObject({ ok: false, reason: 'not_approver' });
 
@@ -1788,6 +1945,7 @@ describe('ApprovalService (node era)', () => {
     const req = await svc.openNodeRequest(
       openInput(['u9', 'u2'], {}, { escalation: { timeoutHours: 1, action: 'reassign', escalateTo: 'boss', notifySubmitter: false } }), CTX,
     );
+    staffDirectory(engine, 'u7');
     await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, SYS);
     expect(indexRows().map(r => r.approver).sort()).toEqual(['u2', 'u7']);
 
@@ -2187,6 +2345,7 @@ describe('ApprovalService — admin override (#3424)', () => {
 
   it('an admin can reassign a stuck request to a real approver, who then decides normally', async () => {
     const req = await svc.openNodeRequest(stuckInput(), CTX);
+    staffDirectory(engine, 'u7');
     const out = await svc.reassign(req.id, { actorId: 'owner', to: 'u7' }, TENANT_ADMIN);
     expect(out.request.pending_approvers).toEqual(['u7']);
     const decided = await svc.decideNode(
@@ -3414,6 +3573,7 @@ describe('ApprovalService — decision_progress & deep links (#2678 P1.5)', () =
     const emitted: any[] = [];
     svc.attachMessaging({ async emit(input) { emitted.push(input); } });
     const req = await svc.openNodeRequest(openInput(['u1', 'u2']), CTX);
+    staffDirectory(engine, 'u7');
     await svc.reassign(req.id, { actorId: 'u1', to: 'u7' }, SYS);
     const note = emitted.find(e => e.topic === 'approval.reassigned');
     expect(note.payload.actionUrl).toBe(`/system/approvals?request=${encodeURIComponent(req.id)}`);
@@ -4433,6 +4593,7 @@ describe('ApprovalService — every slot reader takes the acting addresses (#213
     await expect(svc.reassign(req.id, { to: 'u_next' } as any, BYSTANDER))
       .rejects.toThrow("FORBIDDEN: 'u_bystander' is not a pending approver on this request");
     // With `from` unnamed, the slot handed over is the one the holder takes.
+    staffDirectory(engine, 'u_next');
     const moved = await svc.reassign(req.id, { to: 'u_next' } as any, HOLDER);
     expect(moved.request.pending_approvers).toEqual(['u_next']);
     const [reassigned] = actionsOf(engine, req.id, 'reassign');
@@ -4654,6 +4815,7 @@ describe('ApprovalService — the `role:` arm retired; the override decides what
       const svc = svcFor(engine);
       const { id } = await plant(engine, svc);
       const [, admin] = RESCUERS[2];
+      staffDirectory(engine, 'u_holder');
       const moved = await svc.reassign(id, { actorId: admin.userId, to: 'u_holder' } as any, admin);
       expect(moved.request.pending_approvers).toEqual(['u_holder']);
       const out = await svc.decideNode(id, { decision: 'approve' } as any, HOLDER);