feat(api,history): in-dashboard purchase approval + approve-{any,own} RBAC (#286)#299
Merged
cristim merged 2 commits intofeat/multicloud-web-frontendfrom May 5, 2026
Conversation
… RBAC (closes #286) Adds an inline Approve button on pending Purchase History rows + the session-authed backend dispatch that lets admins (and `approve-own` holders for their own pending rows) approve without round-tripping to the SES email. Mirrors the same dual-auth shape as the Cancel button (PR #145) and Retry button (PR #168). Backend ------- * `internal/auth/types.go`: new constants `ActionApproveOwn` / `ActionApproveAny` next to the existing cancel-/retry- block. `DefaultUserPermissions()` adds `approve-own:purchases` so every authenticated user can approve pending executions they themselves created. `approve-any` exists for future operator roles; no default non-admin grant. * `internal/api/handler_purchases.go::approvePurchase` refactored into the same three-mode dispatch as `cancelPurchase`: 1. Session present + RBAC matches → session-authed approve via `approvePurchaseViaSession` regardless of token. 2. token != "" → legacy email-link path, unchanged. 3. token == "" → session-authed dashboard branch. Permission-denied (403) on the session check falls through to the token branch so a logged-in non-admin who is the per-account contact_email recipient can still approve via the email link. * New `approvePurchaseViaSession` mirrors `cancelPurchaseViaSession` minus the suppression-cleanup tx (approve doesn't drop suppressions — those persist until cancel/expiry). Status flip + `ApprovedBy = session.Email` stamp + SavePurchaseExecution. * New `authorizeSessionApprove` mirror of `authorizeSessionCancel`: admin → ok; else `approve-any` → ok; else `approve-own` AND `created_by_user_id == session.UserID` → ok; else 403. No new migration: existing `purchase_executions.approved_by` (TEXT, migration 000035) and `created_by_user_id` (UUID FK, migration 000041) are sufficient — the email field stamps the actor and the UUID drives the own-ness check. Frontend -------- * `frontend/src/api/purchases.ts`: new `approvePurchase(executionId)` caller wrapping `POST /purchases/approve/{id}` with bearer-session auth (no token in URL — backend's session-first dispatch picks the auth path). * `frontend/src/api/index.ts`: re-export the new function from the api barrel. * `frontend/src/history.ts`: - new `canApprovePendingRow` predicate mirroring `canCancelPendingRow` (admin → yes; non-admin → only own pending rows; legacy null-creator rows out of reach via this UI). - `renderActionCell` now renders Approve + Cancel side-by-side for pending rows where the session qualifies for both. Each predicate is checked independently so a custom role with only one verb renders just that button. - new click handler on `.history-approve-btn` mirroring the cancel-button pattern: confirmDialog (non-destructive) → `api.approvePurchase` → reload history + success toast. Failed approve surfaces an error toast + re-enables the button. RI exchange ----------- `approveRIExchange` has the same shape gap (token-only auth) but mutates a state-machine and triggers downstream cloud-side exchange execution — bigger surface than fits in this PR. Filing as a follow-up issue tracking the symmetric work. Tests ----- Backend * `internal/auth/types_test.go` + `service_test.go` + `service_group_test.go`: bumped permission-count assertions (8→9, 10→11) and added `ActionApproveOwn:ResourcePurchases` membership check in `DefaultUserPermissions returns user access`. * `internal/api/handler_purchases_test.go`: added `.Maybe()` HasPermissionAPI mocks (returning false for both verbs) on five existing approve-purchase tests so the new session-first dispatch falls through to the token branch they exercise. * `internal/api/coverage_extras_test.go`: `TestHandler_approvePurchase_EmptyToken` now asserts the empty-token + zero-handler case returns SOME error (recovered from panic via `defer recover`) — the empty-token path is no longer a pre-flight 400, since after #286 it's the dispatch into the session-authed branch. * `internal/api/handler_test.go::TestHandler_HandleRequest_ApprovePurchase`: same `.Maybe()` mocks for the end-to-end approve route test. Frontend * New `frontend/src/__tests__/history-approve-button.test.ts`: 8 tests covering admin/regular/anonymous render branches, non-pending-row absence, side-by-side Approve+Cancel render, declined confirmDialog, accepted confirm + reload + toast, and API-failure error toast + button re-enable. Verification: - `go test ./...` clean (4265 passed, 0 failed, 6 skipped). - `npm test` clean (1469 passed, 0 failed). - `npm run build` + `npx tsc --noEmit` clean. Token-only email-link path is unchanged — backwards-compat mandatory; existing pre-#286 tests on that path continue to pass.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements session-authenticated purchase approval with RBAC gating. It adds ChangesPurchase Approval with RBAC and Dual-Auth Dispatch
Sequence DiagramsequenceDiagram
participant User as Admin/User
participant History as History UI
participant API as Frontend API
participant Handler as Backend Handler
participant Auth as Auth Service
participant DB as Purchase DB
User->>History: Click Approve button
activate History
History->>History: Show confirm dialog
User->>History: Confirm approval
History->>API: approvePurchase(executionId)
deactivate History
activate API
API->>Handler: POST /purchases/approve/{id}
deactivate API
activate Handler
Handler->>Handler: Check session present
alt Session + RBAC Path
Handler->>Auth: HasPermissionAPI(approve-any)
Auth-->>Handler: false/true
alt User has approve-any or approve-own
Handler->>Handler: Validate status pending/notified
Handler->>Handler: Check ownership (approve-own)
Handler->>DB: SavePurchaseExecution(approved)
DB-->>Handler: Saved
Handler-->>API: 200 {status: approved}
else Unauthorized
Handler-->>API: 403 ClientError
end
else No Session or Permissions Denied
Handler->>Handler: Check token present
alt Token Present (Legacy)
Handler->>Handler: authorizeApprovalAction(token)
Handler->>DB: SavePurchaseExecution(approved)
DB-->>Handler: Saved
Handler-->>API: 200 {status: approved}
else No Token
Handler-->>API: Error
end
end
deactivate Handler
activate API
alt Success
API->>History: Show success toast
History->>API: getHistory() [reload]
API-->>History: Updated rows
else Failure
API->>History: Show error toast
end
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
cristim
added
priority/p2
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/history.ts`:
- Around line 595-603: The approve and cancel handlers only disable the clicked
button (btn) so users can trigger conflicting requests; update the approve
handler (the block around api.approvePurchase(id)) to find the row container
(e.g., using btn.closest(...) or a row element already in scope) and disable
both buttons in that row by selecting '.history-approve-btn' and
'.history-cancel-btn' before the await, then on error re-enable both; apply the
same change to the cancel handler (the block around api.cancelPurchase / related
code) so both buttons are disabled during the in-flight request and re-enabled
on failure.
In `@internal/api/coverage_extras_test.go`:
- Around line 42-52: The test currently defers a recover() which swallows any
panic from h.approvePurchase and can produce false-green results; remove the
defer func() { _ = recover() } so that panics in approvePurchase surface as test
failures (or replace it with a defer that checks and re-panics) and keep the
existing assertion that err must not be nil after calling h.approvePurchase(ctx,
nil, execID, "").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94bdd393-01d5-4450-b8c2-da831ef8d27d
📒 Files selected for processing (12)
frontend/src/__tests__/history-approve-button.test.tsfrontend/src/api/index.tsfrontend/src/api/purchases.tsfrontend/src/history.tsinternal/api/coverage_extras_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/api/handler_test.gointernal/auth/service_group_test.gointernal/auth/service_test.gointernal/auth/types.gointernal/auth/types_test.go
Comment on lines
+595
to
+603
| btn.disabled = true; | ||
| try { | ||
| await api.approvePurchase(id); | ||
| } catch (approveError) { | ||
| console.error('Failed to approve pending purchase:', approveError); | ||
| const err = approveError as Error; | ||
| showToast({ message: `Failed to approve: ${err.message || 'unknown error'}`, kind: 'error' }); | ||
| btn.disabled = false; | ||
| return; |
There was a problem hiding this comment.
Disable both row-level actions once one is confirmed.
Line 595 and Line 625 disable only the clicked button. Since Approve and Cancel can now render together, users can fire conflicting requests on the same row before reload completes. Disable both .history-approve-btn and .history-cancel-btn for that row during the in-flight request (re-enable both on failure).
Suggested patch
- btn.disabled = true;
+ const cell = btn.closest('td') || btn.parentElement;
+ const siblingActions = cell
+ ? Array.from(cell.querySelectorAll<HTMLButtonElement>('.history-approve-btn, .history-cancel-btn'))
+ : [btn];
+ siblingActions.forEach(b => { b.disabled = true; });
try {
await api.approvePurchase(id);
} catch (approveError) {
console.error('Failed to approve pending purchase:', approveError);
const err = approveError as Error;
showToast({ message: `Failed to approve: ${err.message || 'unknown error'}`, kind: 'error' });
- btn.disabled = false;
+ siblingActions.forEach(b => { b.disabled = false; });
return;
}Also applies to: 625-633
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/history.ts` around lines 595 - 603, The approve and cancel
handlers only disable the clicked button (btn) so users can trigger conflicting
requests; update the approve handler (the block around api.approvePurchase(id))
to find the row container (e.g., using btn.closest(...) or a row element already
in scope) and disable both buttons in that row by selecting
'.history-approve-btn' and '.history-cancel-btn' before the await, then on error
re-enable both; apply the same change to the cancel handler (the block around
api.cancelPurchase / related code) so both buttons are disabled during the
in-flight request and re-enabled on failure.
Comment on lines
+42
to
+52
| defer func() { | ||
| // h.config is nil so GetExecutionByID may panic before the | ||
| // dispatch returns; that's still a non-success outcome and is | ||
| // what we want to assert (empty token + no session does not | ||
| // silently succeed). | ||
| _ = recover() | ||
| }() | ||
| _, err := h.approvePurchase(context.Background(), nil, execID, "") | ||
| if err == nil { | ||
| t.Fatal("approvePurchase with empty token + zero handler must not return nil error") | ||
| } |
There was a problem hiding this comment.
Don’t swallow panic in this test; it can create false-green results.
With the current recover() pattern, a panic in approvePurchase can bypass the err assertion and still pass the test. This masks real regressions in the empty-token path.
Suggested patch
- defer func() {
- // h.config is nil so GetExecutionByID may panic before the
- // dispatch returns; that's still a non-success outcome and is
- // what we want to assert (empty token + no session does not
- // silently succeed).
- _ = recover()
- }()
- _, err := h.approvePurchase(context.Background(), nil, execID, "")
- if err == nil {
- t.Fatal("approvePurchase with empty token + zero handler must not return nil error")
- }
+ defer func() {
+ if r := recover(); r != nil {
+ t.Fatalf("approvePurchase should return an error, not panic: %v", r)
+ }
+ }()
+ _, err := h.approvePurchase(context.Background(), nil, execID, "")
+ require.Error(t, err, "approvePurchase with empty token + zero handler must fail")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/coverage_extras_test.go` around lines 42 - 52, The test
currently defers a recover() which swallows any panic from h.approvePurchase and
can produce false-green results; remove the defer func() { _ = recover() } so
that panics in approvePurchase surface as test failures (or replace it with a
defer that checks and re-panics) and keep the existing assertion that err must
not be nil after calling h.approvePurchase(ctx, nil, execID, "").
…ty-token test (CR pass on PR #299) Addresses both actionable items from CodeRabbit's first review on PR #299: 1. **history.ts:603 — disable BOTH Approve and Cancel during a click** (Minor / Quick win): after #286 the two buttons render side-by-side on pending rows, but the click handlers were disabling only the clicked button. A quick double-click could fire conflicting requests on the same row before the reload completes. Extracted a small `sameRowActions(btn)` helper that returns every row-action button (`.history-approve-btn`, `.history-cancel-btn`) in the same `<td>` as the clicked button (falls back to `[btn]` when no parent cell exists — test fixtures may not wrap in a table). Both Approve and Cancel handlers now disable the full set while the API is in flight and re-enable the full set on failure. Successful requests trigger a full history reload that re-renders the row, so the row-action sibling state doesn't matter on the happy path. 2. **coverage_extras_test.go:52 — fail loudly on panic instead of swallowing** (Major / Quick win): the prior `recover()` swallow could let a panic in any new dispatch branch pass the test without ever asserting on `err`, masking real regressions on the empty-token path. The test now (a) `t.Fatalf`s on any panic from `approvePurchase` so the panic path is itself a failure signal, and (b) wires a minimal `MockConfigStore` returning a clean "execution not found" error from `GetExecutionByID` so the dispatch reaches a proper `NewClientError(404)` rather than nil-deref'ing on `h.config`. `require.Error(...)` then asserts the contract: empty token + no session must NOT silently succeed. Tests (regression coverage for #1): * `history-approve-button.test.ts` gains two new cases: - "approve click disables BOTH Approve and Cancel for the row while in flight (CR pass)" — uses an unresolved promise from the api mock to capture the in-flight state and asserts both buttons are disabled until the promise resolves. - "approve API failure re-enables BOTH Approve and Cancel for the row (CR pass)" — asserts both buttons are re-enabled after the API rejection path runs. Verification: - `go test ./...` clean (4265 passed / 0 failed / 6 skipped). - `npm test` clean (1471 passed / 0 failed). - `npx tsc --noEmit` clean.
Member
Author
|
Pass 2: addressed both CR actionables.
Verification: @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review of the updated commits now. [review] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #286.
Why
Today every approve action requires the SES email link. Admins logged into the dashboard cannot approve from History; they have to context-switch to email even though they're already authenticated and authorised. PR #145 closed the same gap for Cancel; PR #168 closed it for Retry. This PR closes it for Approve, with the same dual-auth shape.
What changed
Backend
internal/auth/types.go):ActionApproveOwn/ActionApproveAny.DefaultUserPermissions()addsapprove-own:purchases.approve-anyexists for future operator roles; no default non-admin grant.approvePurchase(mirror ofcancelPurchase):approvePurchaseViaSession.Permission-denied (403) on the session check falls through to the token branch so a logged-in non-admin who is the per-account contact_email recipient can still approve via the email link.
approvePurchaseViaSessionmirrorscancelPurchaseViaSessionminus the suppression-cleanup tx. Status flip +ApprovedBy = session.Emailstamp + SavePurchaseExecution.authorizeSessionApprove: admin → ok; elseapprove-any→ ok; elseapprove-ownANDcreated_by_user_id == session.UserID→ ok; else 403.No new migration: existing
purchase_executions.approved_by(TEXT, mig 000035) andcreated_by_user_id(UUID FK, mig 000041) suffice.Frontend
api.approvePurchase(executionId)caller (no token in URL — bearer-session auth onapiRequest).canApprovePendingRowpredicate mirroringcanCancelPendingRow.renderActionCellrenders Approve + Cancel side-by-side for pending rows the session qualifies for. Each predicate independent so a custom role with only one verb renders just that button..history-approve-btn: confirmDialog (non-destructive) →api.approvePurchase→ reload history + success toast. API failure → error toast + button re-enable.Test plan
go test ./...clean — 4265 passed / 0 failed / 6 skipped (37 packages).npm testclean — 1469 passed / 0 failed.npx tsc --noEmit+npm run buildclean.history-approve-button.test.tscovering admin/regular/anonymous render branches, non-pending-row absence, side-by-side Approve+Cancel render, declined confirmDialog, accepted confirm + reload + toast, and API-failure path..Maybe()HasPermissionAPI mocks (returning false for both verbs) so the legacy token-branch tests continue to exercise the legacy path. Backwards-compat is mandatory — verified.Out of scope (filed as follow-up)
approveRIExchangesymmetric work (issue feat(api,history): add Approve button + approve-{any,own} RBAC for in-dashboard purchase approval #286 referenced this in its body): the RI-exchange approve has the same shape gap (token-only auth) but mutates a state-machine viaTransitionRIExchangeStatus+ triggers downstream cloud-sideexecuteApprovedExchange. That's a substantially larger surface than fits in this PR. Will file a sibling follow-up tracking that work specifically.Notes for reviewers
approve-ownsecurity boundary — the predicate is the backend'sauthorizeSessionApproveathandler_purchases.go. The frontend predicate is purely UX (don't render unusable buttons); a false-positive here surfaces as a 403 toast on click rather than an actual approval.go build ./...andgo test ./...are both clean..Maybe()HasPermissionAPI returns since the new dispatch consults the verb matrix before falling to the token branch. Cancel had the same mocks already; this PR brings approve tests into shape.Summary by CodeRabbit