Skip to content

Commit 7472a06

Browse files
committed
fix(approvals): admin override to recover an approval routed to an unstaffed position (#3424)
An `approval` node routed to a position/team with no holders resolved to only the unresolvable `position:<name>` literal in `pending_approvers`, so no concrete user was in the slate. Every normal `decide` / `reassign` / `recall` then returned FORBIDDEN and, with `lockRecord`, the target record stayed RECORD_LOCKED forever — a data-availability dead-end with no in-product recovery. Trivial to hit in fresh/demo orgs (positions seeded, holders not) and whenever a role is vacated. Let a platform or tenant admin act on any pending request to release it: approve, reject, reassign it to a real approver, or recall it. The override finalizes the request (which releases the record lock, keyed on a pending request); a tenant admin's authority is org-scoped, a platform admin's is not, and the action is audited under the admin's own id. An admin approval is authoritative — it finalizes the node even under unanimous/quorum/per_group rather than counting as one vote among the (empty) slate. - Add server-computed `sys_approval_request.viewer.can_override`; the approve/reject/reassign declared actions OR it into their `visible` gate so the console surfaces the recovery path with no hand-wired button. - Warn loudly when a node resolves to no concrete approver, so the misconfiguration is visible instead of silently locking the record. The literal-fallback behavior (15.x slot back-compat) is otherwise unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmQPXXbgomoqrXtoxr3CS2
1 parent 2ba560a commit 7472a06

6 files changed

Lines changed: 286 additions & 23 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/plugin-approvals": patch
3+
"@objectstack/spec": patch
4+
---
5+
6+
fix(approvals): admin override for a request routed to an unstaffed approver (#3424)
7+
8+
An `approval` node routed to a `position` (or `team`/`department`) with **no
9+
holders** resolved to only the unresolvable `position:<name>` literal in
10+
`pending_approvers` — no concrete user was in the slate. Every normal
11+
`decide` / `reassign` / `recall` then returned `FORBIDDEN` (not a pending
12+
approver) and, with `lockRecord`, the target record stayed `RECORD_LOCKED`
13+
forever: a data-availability dead-end with no in-product recovery (the only exit
14+
was editing the DB by hand). Very easy to hit in fresh/demo orgs (positions
15+
seeded, holders not) and whenever a role is vacated in production.
16+
17+
A **platform or tenant admin** — the same posture the engine's superuser bypass
18+
already trusts — may now act on any *pending* request to release it: **approve,
19+
reject, reassign** it to a real approver, or **recall** it. The override finalizes
20+
the request (which releases the record lock, keyed on a pending request); a
21+
tenant admin's authority is org-scoped, a platform admin's is not, and the
22+
decision is audited under the admin's own id. An admin approval is authoritative,
23+
finalizing the node even under `unanimous` / `quorum` / `per_group` rather than
24+
counting as one vote among the (empty) slate.
25+
26+
- `sys_approval_request.viewer` gains `can_override` (server-computed): true for a
27+
privileged admin on a pending request. The `approve` / `reject` / `reassign`
28+
declared actions OR it into their `visible` gate, so the console surfaces the
29+
recovery path without a hand-wired button. Existing approver/submitter gating is
30+
unchanged.
31+
- `openNodeRequest` now logs a loud warning when a node resolves to **no concrete
32+
approver**, so the misconfiguration is visible instead of silently locking the
33+
record. The literal-fallback behavior (kept for 15.x slot back-compat) is
34+
otherwise unchanged.

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

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -418,11 +418,11 @@ describe('ApprovalService (node era)', () => {
418418
it('getRequest: viewer.can_act is true for a pending approver, false for the submitter', async () => {
419419
const req = await svc.openNodeRequest(openInput(['u9']), CTX); // submitter u1, approver u9
420420
const asApprover = await svc.getRequest(req.id, { userId: 'u9', tenantId: 't1' } as any);
421-
expect(asApprover!.viewer).toEqual({ can_act: true, is_submitter: false });
421+
expect(asApprover!.viewer).toEqual({ can_act: true, is_submitter: false, can_override: false });
422422
const asSubmitter = await svc.getRequest(req.id, { userId: 'u1', tenantId: 't1' } as any);
423-
expect(asSubmitter!.viewer).toEqual({ can_act: false, is_submitter: true });
423+
expect(asSubmitter!.viewer).toEqual({ can_act: false, is_submitter: true, can_override: false });
424424
const asOther = await svc.getRequest(req.id, { userId: 'u_stranger', tenantId: 't1' } as any);
425-
expect(asOther!.viewer).toEqual({ can_act: false, is_submitter: false });
425+
expect(asOther!.viewer).toEqual({ can_act: false, is_submitter: false, can_override: false });
426426
});
427427

428428
it('getRequest: viewer.can_act drops to false once the request is finalized', async () => {
@@ -1034,6 +1034,117 @@ describe('ApprovalService (node era)', () => {
10341034
});
10351035
});
10361036

1037+
// ── Admin / privileged override (#3424) ──────────────────────────────
1038+
//
1039+
// An approval routed to a position/team with NO holders resolves to only the
1040+
// unresolvable `position:<name>` literal — no concrete user is in the slate, so
1041+
// every normal decision is FORBIDDEN and (with lockRecord) the record stays
1042+
// locked forever with no in-product recovery. A platform or tenant admin may
1043+
// act on the pending request to release it: approve, reject, reassign it to a
1044+
// real approver, or recall it. Privilege is org-scoped for tenant admins.
1045+
describe('ApprovalService — admin override (#3424)', () => {
1046+
let engine: ReturnType<typeof makeFakeEngine>;
1047+
let svc: ApprovalService;
1048+
let n = 0;
1049+
const baseTime = new Date('2026-01-15T10:00:00Z').getTime();
1050+
1051+
// Admin exec contexts, shaped like the resolved authz envelope (permissions
1052+
// carry the permission-set names the shared resolver aggregates).
1053+
const PLATFORM_ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any;
1054+
const TENANT_ADMIN = { userId: 'owner', tenantId: 't1', positions: [], permissions: ['organization_admin'] } as any;
1055+
const OTHER_TENANT_ADMIN = { userId: 'owner2', tenantId: 't2', positions: [], permissions: ['organization_admin'] } as any;
1056+
const MEMBER = { userId: 'nobody', tenantId: 't1', positions: [], permissions: [] } as any;
1057+
1058+
// A request routed to an UNSTAFFED position → `pending_approvers` falls back to
1059+
// the `position:sales_manager` literal, undecidable by any normal user.
1060+
const stuckInput = (extra: Record<string, any> = {}) => ({
1061+
object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'approve_step',
1062+
flowName: 'deal_approval',
1063+
config: { approvers: [{ type: 'position' as const, value: 'sales_manager' }], behavior: 'first_response' as const, lockRecord: true },
1064+
record: { id: 'opp1', amount: 100 },
1065+
...extra,
1066+
});
1067+
1068+
beforeEach(() => {
1069+
engine = makeFakeEngine();
1070+
n = 0;
1071+
svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } });
1072+
});
1073+
1074+
it('the stuck request is undecidable by any normal user (repro)', async () => {
1075+
const req = await svc.openNodeRequest(stuckInput(), CTX);
1076+
expect(req.pending_approvers).toEqual(['position:sales_manager']);
1077+
// Even the org owner-by-id is not in the resolved (empty) slate.
1078+
await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'nobody' }, MEMBER))
1079+
.rejects.toThrow(/FORBIDDEN/);
1080+
});
1081+
1082+
it('a tenant admin can approve a stuck request, finalizing it (which releases the lock)', async () => {
1083+
const req = await svc.openNodeRequest(stuckInput(), CTX);
1084+
const out = await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN);
1085+
expect(out.finalized).toBe(true);
1086+
expect(out.request.status).toBe('approved');
1087+
// No pending request remains → the record-lock hook no longer blocks edits.
1088+
const fresh = await svc.getRequest(req.id, SYS);
1089+
expect(fresh?.status).toBe('approved');
1090+
expect(fresh?.pending_approvers).toEqual([]);
1091+
// Audited under the admin's own id — never spoofed as an approver.
1092+
const acts = await svc.listActions(req.id, SYS);
1093+
expect(acts.at(-1)).toMatchObject({ action: 'approve', actor_id: 'owner' });
1094+
});
1095+
1096+
it('a platform admin can reject a stuck request', async () => {
1097+
const req = await svc.openNodeRequest(stuckInput(), CTX);
1098+
const out = await svc.decide(req.id, { decision: 'reject', actorId: 'root' }, PLATFORM_ADMIN);
1099+
expect(out.finalized).toBe(true);
1100+
expect(out.request.status).toBe('rejected');
1101+
});
1102+
1103+
it('an admin override finalizes even a unanimous request immediately (not one vote among the slate)', async () => {
1104+
const req = await svc.openNodeRequest(stuckInput({
1105+
config: { approvers: [{ type: 'position' as const, value: 'sales_manager' }], behavior: 'unanimous' as const, lockRecord: true },
1106+
}), CTX);
1107+
const out = await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN);
1108+
expect(out.finalized).toBe(true);
1109+
expect(out.request.status).toBe('approved');
1110+
});
1111+
1112+
it('an admin can reassign a stuck request to a real approver, who then decides normally', async () => {
1113+
const req = await svc.openNodeRequest(stuckInput(), CTX);
1114+
const out = await svc.reassign(req.id, { actorId: 'owner', to: 'u7' }, TENANT_ADMIN);
1115+
expect(out.request.pending_approvers).toEqual(['u7']);
1116+
const decided = await svc.decideNode(
1117+
req.id, { decision: 'approve', actorId: 'u7' },
1118+
{ userId: 'u7', tenantId: 't1', positions: [], permissions: [] } as any,
1119+
);
1120+
expect(decided.finalized).toBe(true);
1121+
});
1122+
1123+
it('an admin can recall (withdraw) a stuck request', async () => {
1124+
const req = await svc.openNodeRequest(stuckInput(), CTX);
1125+
const out = await svc.recall(req.id, { actorId: 'owner', comment: 'unstaffed role' }, TENANT_ADMIN);
1126+
expect(out.request.status).toBe('recalled');
1127+
expect(out.request.pending_approvers).toEqual([]);
1128+
});
1129+
1130+
it('a tenant admin of a DIFFERENT org cannot override (privilege is org-scoped)', async () => {
1131+
const req = await svc.openNodeRequest(stuckInput(), CTX); // organization_id = t1
1132+
await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'owner2' }, OTHER_TENANT_ADMIN))
1133+
.rejects.toThrow(/FORBIDDEN/);
1134+
});
1135+
1136+
it('viewer.can_override reflects the privilege, and drops once finalized', async () => {
1137+
const req = await svc.openNodeRequest(stuckInput(), CTX);
1138+
const asAdmin = await svc.getRequest(req.id, TENANT_ADMIN);
1139+
expect(asAdmin!.viewer).toMatchObject({ can_act: false, can_override: true });
1140+
const asMember = await svc.getRequest(req.id, MEMBER);
1141+
expect(asMember!.viewer!.can_override).toBe(false);
1142+
await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN);
1143+
const after = await svc.getRequest(req.id, TENANT_ADMIN);
1144+
expect(after!.viewer!.can_override).toBe(false);
1145+
});
1146+
});
1147+
10371148
describe('record-lock hook (node era)', () => {
10381149
let engine: ReturnType<typeof makeFakeEngine>;
10391150
let svc: ApprovalService;

0 commit comments

Comments
 (0)