Skip to content

Commit be1c52c

Browse files
authored
fix(approvals): admin override to recover an approval routed to an unstaffed position (#3451)
Platform/tenant admins can approve/reject/reassign/recall any pending request to release the record lock; adds viewer.can_override, a runtime warning, an advisory lint rule, and staffs the showcase `exec` position. Fixes #3424.
1 parent 5ac93d4 commit be1c52c

11 files changed

Lines changed: 416 additions & 29 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.

content/docs/automation/approvals.mdx

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -281,20 +281,38 @@ service attaches to every request it returns:
281281

282282
```jsonc
283283
// getRequest / listRequests responses carry, per the calling user:
284-
"viewer": { "can_act": true, "is_submitter": false }
284+
"viewer": { "can_act": true, "is_submitter": false, "can_override": false }
285285
```
286286

287287
- `can_act` — the caller is a **current pending approver** (their id is in the
288288
resolved `pending_approvers` while the request is `pending`). This is the same
289289
check the decision routes authorize with, so it already reflects
290290
position/team/manager resolution.
291291
- `is_submitter` — the caller submitted the request.
292+
- `can_override` — the caller is a **platform or tenant admin** who may act on a
293+
`pending` request despite holding no slot (see the admin-override callout
294+
below).
292295

293296
Approver actions gate on `record.viewer.can_act`, submitter levers
294-
(remind/recall/resubmit) on `record.viewer.is_submitter`. So a submitter viewing
297+
(remind/recall/resubmit) on `record.viewer.is_submitter`, and Approve/Reject/
298+
Reassign additionally OR in `record.viewer.can_override`. So a submitter viewing
295299
their own pending request never sees Approve/Reject/Reassign (buttons the server
296-
would 403 anyway), and a position-addressed approver is never wrongly hidden.
297-
The service stays the sole authority — the predicate only trims the UI.
300+
would 403 anyway), a position-addressed approver is never wrongly hidden, and an
301+
admin can rescue a stuck request. The service stays the sole authority — the
302+
predicate only trims the UI.
303+
304+
<Callout type="info">
305+
**Admin override — recovering a stuck request.** An approval routed to a
306+
`position` / `team` / `department` with **no holders** resolves to only an
307+
unresolvable `position:<name>` literal in `pending_approvers`: no concrete user
308+
can act, and (with `lockRecord`) the record stays locked. A **platform admin**
309+
(`admin_full_access`) or **tenant admin** (`organization_admin`, org-scoped) may
310+
act on any `pending` request — **approve, reject, reassign** it to a real
311+
approver, or **recall** it — releasing the lock. An admin decision is
312+
authoritative: it finalizes the node even under `unanimous`/`quorum`/`per_group`,
313+
and is audited under the admin's own id. Prefer a guaranteed-staffed fallback
314+
approver so the set is never empty in the first place.
315+
</Callout>
298316

299317
### Progress and notification deep links
300318

examples/app-showcase/src/security/seed-approval-demo.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,16 @@ const SYS = { isSystem: true } as const;
4141

4242
const ADMIN_EMAIL = 'admin@objectos.ai';
4343

44-
/** Positions the admin is granted so they resolve as an approver on the demos. */
45-
const ADMIN_APPROVAL_POSITIONS = ['manager', 'finance', 'legal'] as const;
44+
/**
45+
* Positions the admin is granted so they resolve as an approver on the demos.
46+
* Covers every `{ type: 'position' }` the showcase approval flows route to and
47+
* expect the admin to hold — including `exec`, the SECOND tier of
48+
* `showcase_budget_approval` (manager → exec). Without `exec` a budget approval
49+
* that a user drives to step 2 routes to an unstaffed position and dead-ends
50+
* (framework#3424): undecidable request, record locked. Keep this in sync with
51+
* the position values authored in `automation/flows`.
52+
*/
53+
const ADMIN_APPROVAL_POSITIONS = ['manager', 'finance', 'legal', 'exec'] as const;
4654

4755
/** A phone-based demo persona (§6 "phone sign-in surfaces"). */
4856
const PHONE_DEMO_USER = {

packages/lint/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ export {
106106
APPROVAL_APPROVER_TYPE_DEPRECATED,
107107
APPROVAL_APPROVER_TYPE_UNKNOWN,
108108
APPROVAL_ESCALATION_REASSIGN_NO_TARGET,
109+
APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY,
109110
} from './validate-approval-approvers.js';
110111
export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js';
111112

packages/lint/src/validate-approval-approvers.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
APPROVAL_APPROVER_TYPE_DEPRECATED,
88
APPROVAL_APPROVER_TYPE_UNKNOWN,
99
APPROVAL_ESCALATION_REASSIGN_NO_TARGET,
10+
APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY,
1011
} from './validate-approval-approvers.js';
1112

1213
function stackWithApprovers(approvers: unknown[]): Record<string, unknown> {
@@ -120,6 +121,56 @@ describe('validateApprovalApprovers', () => {
120121
expect(validateApprovalApprovers(stack)).toEqual([]);
121122
});
122123

124+
// ── empty-slate dead-end (#3424) ─────────────────────────────────────────
125+
126+
it('flags a node routed only to a single group (unstaffed-position dead-end)', () => {
127+
const findings = validateApprovalApprovers(stackWithApprovers([
128+
{ type: 'position', value: 'exec' },
129+
]));
130+
expect(findings).toHaveLength(1);
131+
expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY);
132+
expect(findings[0].severity).toBe('info');
133+
expect(findings[0].path).toBe('flows[0].nodes[1].config.approvers');
134+
expect(findings[0].message).toContain('locked'); // lockRecord defaults true
135+
expect(findings[0].hint).toContain("org_membership_level', value: 'owner'");
136+
});
137+
138+
it('flags a node routed only to groups (all position/team/department)', () => {
139+
const findings = validateApprovalApprovers(stackWithApprovers([
140+
{ type: 'position', value: 'finance' },
141+
{ type: 'team', value: 't1' },
142+
{ type: 'department', value: 'bu_sales' },
143+
]));
144+
expect(findings.filter(f => f.rule === APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY)).toHaveLength(1);
145+
});
146+
147+
it('does NOT flag when a guaranteed-staffed or individual fallback is present', () => {
148+
// position + owner tier — the prescribed fallback.
149+
expect(validateApprovalApprovers(stackWithApprovers([
150+
{ type: 'position', value: 'exec' },
151+
{ type: 'org_membership_level', value: 'owner' },
152+
]))).toEqual([]);
153+
// position + a specific user.
154+
expect(validateApprovalApprovers(stackWithApprovers([
155+
{ type: 'position', value: 'exec' },
156+
{ type: 'user', value: 'u1' },
157+
]))).toEqual([]);
158+
// position + the submitter's manager.
159+
expect(validateApprovalApprovers(stackWithApprovers([
160+
{ type: 'position', value: 'exec' },
161+
{ type: 'manager' },
162+
]))).toEqual([]);
163+
});
164+
165+
it('drops the record-lock clause when lockRecord is false', () => {
166+
const stack = stackWithApprovers([{ type: 'position', value: 'exec' }]);
167+
(stack.flows as any)[0].nodes[1].config.lockRecord = false;
168+
const findings = validateApprovalApprovers(stack);
169+
expect(findings).toHaveLength(1);
170+
expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY);
171+
expect(findings[0].message).not.toContain('locked');
172+
});
173+
123174
it('only scans approval nodes and tolerates malformed shapes', () => {
124175
const findings = validateApprovalApprovers({
125176
flows: [{

packages/lint/src/validate-approval-approvers.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
* | approval-approver-type-deprecated | warning | ADR-0090 D3 (#3133) |
2121
* | approval-approver-type-unknown | warning | contract-first (PD #12) |
2222
* | approval-escalation-reassign-no-target | warning | silent notify degradation |
23+
* | approval-approvers-may-resolve-empty | info | empty-position dead-end (#3424) |
2324
*
2425
* The first two are mutually exclusive by construction — a bad *value* wins,
2526
* because its fix (`position`) differs from the deprecation's fix
@@ -44,6 +45,19 @@ export const APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER = 'approval-approver-not-memb
4445
export const APPROVAL_APPROVER_TYPE_DEPRECATED = 'approval-approver-type-deprecated';
4546
export const APPROVAL_APPROVER_TYPE_UNKNOWN = 'approval-approver-type-unknown';
4647
export const APPROVAL_ESCALATION_REASSIGN_NO_TARGET = 'approval-escalation-reassign-no-target';
48+
export const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY = 'approval-approvers-may-resolve-empty';
49+
50+
/**
51+
* Approver types that route to a GROUP whose membership is runtime data and can
52+
* be empty (an unstaffed position, an empty team/department). When EVERY
53+
* approver on a node is one of these, the node can resolve to an empty slate at
54+
* runtime — the framework#3424 dead-end. Individually-routed types
55+
* (`user`/`field`/`manager`), the guaranteed-staffed `org_membership_level`
56+
* tiers, and the opaque `queue` are deliberately excluded: any of them present
57+
* signals the author has a non-group route, so the node isn't purely
58+
* group-gated.
59+
*/
60+
const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']);
4761

4862
export type ApprovalApproverSeverity = 'error' | 'warning' | 'info';
4963

@@ -171,6 +185,38 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin
171185
}
172186
}
173187

188+
// Empty-slate dead-end (#3424): when EVERY approver on the node routes to
189+
// a group whose membership can be empty (an unstaffed position, an empty
190+
// team/department), the request can resolve to an empty `pending_approvers`
191+
// at runtime — no concrete user can act, and with `lockRecord` the record
192+
// stays locked with no recovery except a platform/tenant admin override.
193+
// Advisory (`info`): staffing is runtime data a linter can't see, so this
194+
// flags the risky SHAPE and prescribes a guaranteed-staffed fallback.
195+
const routable = approvers.filter(
196+
(a) => a && typeof a === 'object' && typeof (a as AnyRec).type === 'string',
197+
);
198+
if (
199+
routable.length > 0 &&
200+
routable.every((a) => GROUP_ROUTED_TYPES.has(canonicalApproverType(String((a as AnyRec).type))))
201+
) {
202+
const locks = (cfg as AnyRec).lockRecord !== false; // default true
203+
findings.push({
204+
severity: 'info',
205+
rule: APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY,
206+
where,
207+
path: `flows[${fi}].nodes[${ni}].config.approvers`,
208+
message:
209+
`every approver on this node routes to a group (position/team/department) whose ` +
210+
`members are runtime data — if none is staffed, the request resolves to an empty ` +
211+
`slate and waits forever` +
212+
(locks ? `, and (lockRecord) the record stays locked with no in-product recovery.` : `.`),
213+
hint:
214+
`Make sure at least one target is always staffed, or add a guaranteed-staffed ` +
215+
`fallback approver, e.g. { type: 'org_membership_level', value: 'owner' }. A request ` +
216+
`that still lands empty is recoverable only by a platform/tenant admin override (#3424).`,
217+
});
218+
}
219+
174220
// escalation.action 'reassign' with no escalateTo silently degrades to a
175221
// plain SLA-breach notification at runtime — the hand-off the author
176222
// asked for never happens.

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)