diff --git a/.changeset/25485-reassign-target-resolves.md b/.changeset/25485-reassign-target-resolves.md new file mode 100644 index 00000000000..ace51d0d1f3 --- /dev/null +++ b/.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 + + + +**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:`: 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:` for a position someone in it holds. diff --git a/packages/plugins/plugin-approvals/src/actor-lookups-hold-ids.integration.test.ts b/packages/plugins/plugin-approvals/src/actor-lookups-hold-ids.integration.test.ts index 47e86114010..9493f2f9982 100644 --- a/packages/plugins/plugin-approvals/src/actor-lookups-hold-ids.integration.test.ts +++ b/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,7 +166,7 @@ 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(); @@ -161,6 +174,8 @@ describe('ADR-0118 D1 — every sys_user lookup approvals writes holds an id or // 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, diff --git a/packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts b/packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts index b5822cc94b9..918165da754 100644 --- a/packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts +++ b/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. */ diff --git a/packages/plugins/plugin-approvals/src/approval-override-audit.test.ts b/packages/plugins/plugin-approvals/src/approval-override-audit.test.ts index 2cdec9fd0df..d53c44a07ed 100644 --- a/packages/plugins/plugin-approvals/src/approval-override-audit.test.ts +++ b/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. */ diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 6a4dc75720d..3ff38f622c0 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/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 }, ...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 = {}) { 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 { + 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, 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); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index aae41b2e56b..6f30ae6100b 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -86,6 +86,7 @@ import { // decision methods' slot test and `can_act` alike (`approver-address.ts`). import { actingAddresses, + addressedPosition, equivalentApproverAddresses, heldSlot, positionAddresses, @@ -5513,6 +5514,10 @@ export class ApprovalService implements IApprovalService { } else { throw new Error(`FORBIDDEN: '${from}' is not a pending approver on this request`); } + // #25485 — the new slot must name someone who can take it. Asked only + // AFTER the authorization branches above, so a caller who may not move + // this slot learns nothing about the directory from the answer. + await this.assertReassignTargetResolves(to, raw.organization_id ?? null); const now = this.clock.now().toISOString(); // Audit first, then mutate — mirrors decideNode(), so a failed audit // write can never leave a moved slot without a trail. @@ -5570,6 +5575,69 @@ export class ApprovalService implements IApprovalService { return { request: fresh }; } + /** + * Refuse a reassign target that resolves to no one (#25485): a slot nobody + * can take strands the request, waiting on no one until an override actor + * moves it again. `to` is a slot address in the spellings a slate holds, and + * it resolves when: + * + * - a user id or an email — a `sys_user` row carries it (`id`, or `email` + * as stored: the spelling a default actor takes the slot under, + * {@link actingAddresses}), and that user is not PROVABLY a member of + * other organizations only. "Provably" is the posture of the + * manager and team screens ({@link managerIsProvablyOutsideOrg}, + * {@link dropMembersProvablyOutsideOrg}), on the same `sys_member` + * column: membership rows that name other organizations and not this one + * refuse; no membership rows at all, or a request with no organization, + * leave the target as it is; + * - `position:

` — someone holds `p` in the request's organization + * ({@link expandPositionUsers}, the routing read a position approver + * opens on); + * - any other `type:value` literal — never: no actor takes a `team:` / + * `department:` / `org_membership_level:` / stored `role:` address + * (`approver-address.ts`), so such a slot is the stranded slate itself. + * + * One refusal text for a missing user and a user of another organization, + * so the answer is not a probe of who exists in other tenants. A failed + * `sys_user` read propagates — the caller is told, and nothing moved — while + * an unreadable `sys_member` is an absent tenancy fact, as in the sibling + * screens. + */ + private async assertReassignTargetResolves(to: string, organizationId: string | null): Promise { + const refuse = (why: string): never => { + throw new Error( + `VALIDATION_FAILED: cannot reassign to '${to}' — ${why} The request and its approvers are unchanged. ` + + `Reassign to the user id or email of a person in this request's organization, or to ` + + `'position:' for a position someone in it holds.`, + ); + }; + const position = addressedPosition(to); + if (position !== undefined) { + if ((await this.expandPositionUsers(position, organizationId)).length) return; + refuse(`no one holds position '${position}' in this request's organization.`); + } + if (to.includes(':')) { + refuse('it is a slot literal no approver acts under, so no one could ever decide the request.'); + } + const rows = await this.engine.find('sys_user', { + where: to.includes('@') ? { email: to } : { id: to }, + fields: ['id'], limit: 1, context: SYSTEM_CTX, + }); + const userId = Array.isArray(rows) && rows[0]?.id != null ? String(rows[0].id) : ''; + if (!userId) refuse('it names no user in this request\'s organization.'); + if (!organizationId) return; + let memberships: any[] = []; + try { + memberships = await this.engine.find('sys_member', { + where: { user_id: userId }, fields: ['organization_id'], limit: 1000, context: SYSTEM_CTX, + }); + } catch { return; } // membership unreadable ⇒ no tenancy fact, as in the sibling screens + const orgs = (memberships ?? []).map((r: any) => String(r?.organization_id ?? '')).filter(Boolean); + if (orgs.length && !orgs.includes(String(organizationId))) { + refuse('it names no user in this request\'s organization.'); + } + } + /** * Submitter nudge — notify every pending approver. Throttled to one * reminder per {@link REMIND_COOLDOWN_MS} per request. diff --git a/packages/plugins/plugin-approvals/src/approver-address.ts b/packages/plugins/plugin-approvals/src/approver-address.ts index 260b5e61af2..40429526041 100644 --- a/packages/plugins/plugin-approvals/src/approver-address.ts +++ b/packages/plugins/plugin-approvals/src/approver-address.ts @@ -87,6 +87,20 @@ export function equivalentApproverAddresses(address: string): string[] { return [address]; } +/** + * The position `address` names — `p` for `position:

` — or `undefined` when + * it names none (a user id, an email, any other `type:value` literal, a stored + * `role:` slot). The inverse of {@link positionAddresses}, read where a slot + * address is RESOLVED to the people who could take it (`reassign`'s target + * check), never compared with a caller. + */ +export function addressedPosition(address: string): string | undefined { + for (const prefix of POSITION_ADDRESS_PREFIXES) { + if (address.startsWith(prefix)) return address.slice(prefix.length); + } + return undefined; +} + /** * The caller's identity as the SERVER resolved it — never a client-supplied * value: a body or query parameter that names an address is an ASK, judged diff --git a/packages/qa/dogfood/test/approval-reassign-target-resolves.dogfood.test.ts b/packages/qa/dogfood/test/approval-reassign-target-resolves.dogfood.test.ts new file mode 100644 index 00000000000..a264774ede4 --- /dev/null +++ b/packages/qa/dogfood/test/approval-reassign-target-resolves.dogfood.test.ts @@ -0,0 +1,126 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #25485 — the reassign door accepted a `to` that resolves to no one. +// +// `POST /approvals/requests/:id/reassign {"to":"usr_does_not_exist"}` answered +// 200 and wrote the address onto the slate, so the request waited on nobody +// until an override actor moved it again — as the slot holder and as an admin +// (measured on stock showcase). Now `to` must resolve in the request's +// organization, on a real boot, through the door the console calls: +// +// ⭐ the slot holder's reassign to a user id no account carries, and to a +// bare word, are refused `400 VALIDATION_FAILED` naming the address; +// ⭐ so is the platform admin's (the override does not exempt the new slot), +// and a `type:value` literal no approver acts under; +// ⭐ none of them moved anything: the slate is unchanged and no `reassign` +// action was recorded; +// ⭐ CONTROL: a reassign to a member of the organization moves the slot, and +// the member then finds the request in their "My Pending". +// +// The fixture is #22824's: its staffed object routes one request to a named +// user by email, which is the slot the holder hands over here. + +import { describe, it, expect } from 'vitest'; +import { bootStack } from '@objectstack/verify'; +import { ApprovalsServicePlugin } from '@objectstack/plugin-approvals'; +import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; +import { MessagingServicePlugin } from '@objectstack/service-messaging'; +import { + adminRescueStack, + adminRescueSecurity, + STAFFED_APPROVER_EMAIL, +} from './fixtures/admin-rescue-slate-fixture.js'; + +const SYS = { context: { isSystem: true } } as const; +const SUBMITTER = 'reassign-target-submitter@example.com'; +const MEMBER = 'reassign-target-member@example.com'; + +interface Row { + id: string; + object_name?: string; + pending_approvers?: string[]; +} + +describe('the reassign door refuses a target that resolves to no one (#25485)', () => { + it( + 'refuses an unknown user id, a bare word and a slot literal, as holder and as admin, moves nothing, and still moves a slot to a member', + async () => { + const stack = await bootStack(adminRescueStack as unknown as Parameters[0], { + automation: true, + security: adminRescueSecurity(), + extraPlugins: [ + new MessagingServicePlugin({ reliableDelivery: false }), + new RecordChangeTriggerPlugin(), + new ApprovalsServicePlugin(), + ], + }); + try { + const ql: any = await stack.kernel.getServiceAsync('objectql'); + const idOf = async (email: string): Promise => { + const u = await ql.findOne('sys_user', { where: { email }, context: SYS.context }); + return String(u?.id ?? ''); + }; + // The dev admin first, so the sign-ups below are plain members. + const adminToken = await stack.signIn(); + const submitterToken = await stack.signUp(SUBMITTER); + const approverToken = await stack.signUp(STAFFED_APPROVER_EMAIL); + const memberToken = await stack.signUp(MEMBER); + const memberId = await idOf(MEMBER); + expect(memberId).not.toBe(''); + + const created = await stack.apiAs(submitterToken, 'POST', '/data/admin_rescue_staffed', { name: 'reassign me' }); + expect(created.status).toBe(201); + const pendingRes = await stack.apiAs(adminToken, 'GET', '/approvals/requests?status=pending'); + expect(pendingRes.status).toBe(200); + const open = ((await pendingRes.json()) as { data: Row[] }).data + .find((r) => r.object_name === 'admin_rescue_staffed'); + expect(open?.pending_approvers, 'precondition: the request is open on the named approver').toEqual([STAFFED_APPROVER_EMAIL]); + const requestId = open!.id; + + const reassign = (token: string, body: Record) => + stack.apiAs(token, 'POST', `/approvals/requests/${requestId}/reassign`, body); + const slate = async (): Promise => { + const row = await ql.findOne('sys_approval_request', { where: { id: requestId }, context: SYS.context }); + return String(row?.pending_approvers ?? ''); + }; + const moves = async (): Promise => + (await ql.find('sys_approval_action', { where: { request_id: requestId }, context: SYS.context })) + .filter((a: any) => a.action === 'reassign'); + + // ⭐ 1–2 — every unresolvable target is refused, naming the address. + const refusals: Array<[string, string, Record]> = [ + ['the slot holder, an unknown user id', approverToken, { to: 'usr_does_not_exist' }], + ['the slot holder, a bare word', approverToken, { to: 'nobody-at-all' }], + ['the slot holder, a team literal', approverToken, { to: 'team:nobody' }], + ['the platform admin, an unknown user id', adminToken, { to: 'usr_does_not_exist', from: STAFFED_APPROVER_EMAIL }], + ]; + for (const [who, token, body] of refusals) { + const res = await reassign(token, body); + expect(res.status, who).toBe(400); + const answer = (await res.json()) as { code?: string; error?: string }; + expect(answer.code, who).toBe('VALIDATION_FAILED'); + expect(answer.error, who).toContain(`cannot reassign to '${String(body.to)}'`); + } + + // ⭐ 3 — none of them moved anything. + expect(await slate()).toBe(STAFFED_APPROVER_EMAIL); + expect(await moves()).toEqual([]); + + // ⭐ 4 — CONTROL: a member of the organization takes the slot. + const moved = await reassign(approverToken, { to: memberId }); + expect(moved.status).toBe(200); + expect(((await moved.json()) as { request?: Row }).request?.pending_approvers).toEqual([memberId]); + expect(await moves()).toHaveLength(1); + const mine = await stack.apiAs( + memberToken, 'GET', + `/approvals/requests?status=pending&approverId=${encodeURIComponent(memberId)},${encodeURIComponent(MEMBER)}&limit=50`, + ); + expect(mine.status).toBe(200); + expect(((await mine.json()) as { data: Row[] }).data.map((r) => r.id)).toEqual([requestId]); + } finally { + await stack.stop(); + } + }, + 120_000, + ); +});