feat(api,frontend): 4-eyes approval mode — creator cannot self-approve (closes #1005)

[cristim]

Summary

Implements the 4-eyes approval mode from #1005 end-to-end (backend + frontend). When require_different_approver is set in global_config, the creator of a scheduled execution cannot approve it themselves — a second admin/purchaser must approve. Admin wildcard is NOT exempt (that's the whole point). NULL-creator legacy rows fail 403 with an admin-directed remediation message.

Backend

• Migration 000092: adds require_different_approver BOOLEAN NOT NULL DEFAULT false to global_config.
• Config plumbing: GlobalConfig.RequireDifferentApprover; GetGlobalConfig/SaveGlobalConfig read/write it; pgxmock fixtures updated.
• Handler enforcement (requireDifferentApprover helper): wired into both approval paths — approvePurchaseViaSession (session cookie path) and the email-token path in approvePurchase. Nil session with mode on fails 500 (fail-closed per feedback_fail_closed_middleware).
• 7 new backend tests: mode-off allow; mode-on same-user deny; mode-on different-user allow; admin-self-deny (wildcard not exempt); NULL-creator deny with admin-directed message; nil-auth 500; email-token path enforcement. Plus 3 existing tests updated to mock the new GetGlobalConfig call in the session path.

Frontend

• Settings > Purchasing "Require different approver" checkbox wired to saveGlobalSettings / loadGlobalSettings, round-trips through updateConfig / getConfig.
• Inline banner on the Purchases tab when the mode is on, so users understand why an Approve action they'd normally see is unavailable on their own rows.
• canApproveUnder4Eyes gate in history.ts: when RBAC would show Approve but 4-eyes blocks it (same user is creator, mode on), an inline "Awaiting different approver" badge replaces the button rather than silently hiding the action.
• New four-eyes-approval.test.ts (5 cases) + round-trip tests in settings.test.ts (2 cases) covering the settings toggle load + save; getConfig mock added to 8 sibling history-* test files whose jest.mock('../api', ...) blocks needed the new call.

Test coverage

• Backend: TestApprovePurchase_FourEyesMode_* × 7 in handler_purchases_test.go; TestApprovePurchaseViaSession_* updated for GetGlobalConfig mock; store_postgres_pgxmock_test.go fixtures include the new column.
• Frontend: four-eyes-approval.test.ts (235 lines, 5 cases); settings.test.ts (+41 lines, 2 round-trip cases); sibling history-* mocks updated.

Reconciliation note

This PR absorbed uncommitted frontend work from a prior agent session. Follow-up issue #1501 (originally opened when the PR was backend-only) is superseded and will be closed.

Closes #1005.

Summary by CodeRabbit

• New Features

  • Added optional 4-eyes approval mode requiring purchases to be approved by someone other than the creator.
  • Added an Admin setting to enable or disable this mode.
  • Added clear approval-status messaging when self-approval is blocked.
  • Enforced the rule across web, token, API-key, and automated approval flows.

• Tests

  • Added comprehensive coverage for self-approval prevention, alternate approvers, legacy records, and configuration behavior.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 33ea8aaa-0fa8-4ad5-bc28-d6d5131d3e3a

📥 Commits

Reviewing files that changed from the base of the PR and between 70c9a7b and 97662ec.

📒 Files selected for processing (33)

• frontend/src/__tests__/allowed-accounts.test.ts
• frontend/src/__tests__/four-eyes-approval.test.ts
• frontend/src/__tests__/history-approval-queue.test.ts
• frontend/src/__tests__/history-approve-button.test.ts
• frontend/src/__tests__/history-cancel-button.test.ts
• frontend/src/__tests__/history-cancel-permissions.test.ts
• frontend/src/__tests__/history-marketplace-sell-button.test.ts
• frontend/src/__tests__/history-retry-button.test.ts
• frontend/src/__tests__/history-revoke-button.test.ts
• frontend/src/__tests__/history.test.ts
• frontend/src/__tests__/settings.test.ts
• frontend/src/__tests__/xss-provider-class.test.ts
• frontend/src/api/types.ts
• frontend/src/history.ts
• frontend/src/index.html
• frontend/src/settings.ts
• frontend/src/types.ts
• internal/analytics/collector_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/middleware_test.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_coverage_test.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/types.go
• internal/database/postgres/migrations/000092_require_different_approver.down.sql
• internal/database/postgres/migrations/000092_require_different_approver.up.sql
• internal/mocks/stores.go
• internal/purchase/approvals.go
• internal/purchase/approvals_test.go
• internal/purchase/coverage_extra_test.go
• internal/server/test_helpers_test.go

---

📝 Walkthrough

Walkthrough

Adds an optional require_different_approver setting, persists it through configuration storage, enforces it across approval paths, and updates frontend settings and purchase-history controls with corresponding tests.

Changes

Four-eyes approval mode

Layer / File(s)	Summary
Frontend configuration and approval controls frontend/src/api/types.ts, frontend/src/types.ts, frontend/src/history.ts, frontend/src/index.html, frontend/src/settings.ts, frontend/src/__tests__/*	Adds the setting to frontend types and settings UI, loads/saves/resets it, displays the enforcement banner, and gates approval buttons and badges based on creator identity.
Global configuration persistence internal/config/*, internal/database/postgres/migrations/*, internal/mocks/stores.go, internal/server/test_helpers_test.go, internal/analytics/collector_test.go	Adds the database column, configuration field, PostgreSQL read/write support, user-email lookup, mocks, and storage regression updates.
Approval-path enforcement internal/api/handler_purchases.go, internal/purchase/approvals.go	Applies different-approver checks to session, token, direct-execute, and manager approval paths, including normalized API-key identity handling and fail-closed legacy-row behavior.
Four-eyes regression coverage internal/api/*_test.go, internal/purchase/*_test.go	Covers enabled and disabled modes, self-approval, different approvers, admin and API-key identities, token/SQS flows, direct execution, null creators, and configuration-dependent mocks.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• LeanerCloud/CUDly#299: Modifies the same purchase approval UI and session-authenticated approval flow.
• LeanerCloud/CUDly#373: Intersects with the ApproveAndExecute purchase approval execution path.
• LeanerCloud/CUDly#1456: Changes the same approval handlers with additional approval-time gating.

Suggested labels: type/feat

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/1005-four-eyes-approval

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 50 minutes.

[cristim]

Adversarial review follow-up: 4-eyes bypasses closed at the manager layer

A follow-up adversarial (Opus) review of this PR found that requireDifferentApprover was wired into only 2 of the 4 real approve/execute entry points, leaving two bypasses of the dual-control guarantee this PR advertises ("no single employee can request AND authorize; applies to all incl. admins"):

• HIGH: execute_mode="direct" (executePurchase -> directExecutePurchase) was gated only by authorizeSessionExecuteDirect (execute-own/execute-any RBAC), never by 4-eyes. Since the creator is always the direct-executor by construction on this path, this was unconditional self-authorization for anyone holding execute-own or execute-any.
• MEDIUM: the SQS async approve worker (handleApproveMessage -> ApproveExecution) enforced token + actor_email + per-account contact_email matching, but never compared the actor against the execution's creator.

Fix

Moved enforcement into purchase.Manager.ApproveAndExecute, the single choke point all four entry points (session approve, direct-execute, email-token approve, SQS approve) funnel through, instead of relying on per-handler wiring. New enforceFourEyesPolicy runs before any state mutation and fails closed. The handler-level requireDifferentApprover checks stay in place as defense-in-depth for the two paths that already had it.

Identity comparison is two-tier for precision:

1. UUID tier: when the caller supplies the actor's own UUID (transitionedBy, populated for the two session-based callers), compare it directly against CreatedByUserID.
2. Email tier: falls back to resolving the creator's email (new config.StoreInterface.GetUserEmailByID) and comparing case-insensitively against the actor's email, used only when no actor UUID is available (the token/SQS callers, which always pass transitionedBy=nil).

The two-tier design closes a second gap an independent Opus reviewer caught in this same fix during review: a per-user API key session carries a real user UUID but an empty Session.Email (unlike a normal login session), so an email-only comparison could never match it against the creator's real email and would have silently allowed self-direct-execute for that auth class. Comparing UUIDs first, when available, sidesteps that entirely.

Tests

New end-to-end regression tests drive the real production paths (not mocked-out shortcuts), each confirmed to fail on the pre-fix code and pass after:

• internal/purchase/approvals_test.go / coverage_extra_test.go: manager-level gate tests plus a true ProcessMessage SQS end-to-end test (self-approve denied, different approver succeeds).
• internal/api/handler_purchases_test.go: wires a real *purchase.Manager (not the usual MockPurchaseManager) into the Handler so the manager-layer gate is genuinely exercised through the full HTTP dispatch for both execute_mode="direct" and session-approve, plus the per-user-API-key edge case.

Full repo: go build ./..., go vet ./... clean; go test ./... = 5940 passed across 39 packages; gocyclo -over 10 clean on all changed files; golangci-lint (CI-pinned v2.10.1) = 0 issues. Migration 000092 (require_different_approver) is unchanged and does not collide with main.

@coderabbitai review

[coderabbitai]

@cristim: Reviewing the manager-layer enforcement and the newly covered direct-execute, SQS, and per-user API-key paths.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai full review

Context: the previous review passes on this PR were all throttled under CodeRabbit's Fair Usage limit (the 13:38 UTC full review returned a "review limit is currently reached" warning, and the 14:58 incremental command was a no-op that does not re-review already-reviewed commits). No walkthrough or inline review has actually landed for the current HEAD (97662ec), which added the manager-layer 4-eyes enforcement in internal/purchase/approvals.go and the new config.StoreInterface.GetUserEmailByID. Requesting a full review now that the rate-limit window has cleared so the security-critical enforcement change gets a real pass.

[coderabbitai]

@cristim: Starting a full review of the current HEAD, with particular focus on the manager-level four-eyes gate in internal/purchase/approvals.go, the GetUserEmailByID contract and implementations, and all approval/execute entry points.

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 21 minutes.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 8 seconds.

[cristim]

Merging. Verified the adversarial-review follow-up is fully implemented in code, not just described:

• enforceFourEyesPolicy runs at the Manager.ApproveAndExecute choke point before any state mutation, and all four entry points genuinely funnel through it: session approve, execute_mode="direct" (the HIGH bypass), email-token approve, and the SQS async worker (handleApproveMessage -> ApproveExecution -> ApproveAndExecute, the MEDIUM bypass). Both previously-open bypasses are closed at the shared layer.
• Fails closed on every gap: NULL creator (legacy row), empty actor identity, creator-email resolution failure, and both UUID and email self-match.
• Two-tier identity is real: authoritative UUID comparison first (transitionedBy vs CreatedByUserID), email fallback via the new GetUserEmailByID only when no actor UUID is available. The per-user-API-key edge (real UUID, empty Session.Email) is correctly handled UUID-first.
• Handler-level requireDifferentApprover retained as defense-in-depth on the two paths that already had it.
• Migration 000092 does not collide with main.

22 checks green, MERGEABLE/CLEAN. Merging.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.