feat(history): inline Cancel button + cancel-any/own RBAC verbs#145
Merged
cristim merged 4 commits intofeat/multicloud-web-frontendfrom Apr 27, 2026
Merged
Conversation
Adds a session-authed Cancel button to pending Purchase History rows so users can abort an in-flight approval from the dashboard instead of hunting through their inbox for the email-token link. The token-only escape hatch stays unchanged. Decision (issue #46, Option C): - cancel-any:purchases — granted via RoleAdmin/ResourceAll. Lets an admin cancel ANY user's pending execution. The new constant exists so a future non-admin operator role can be granted broad cancel rights without escalating to admin. - cancel-own:purchases — added to DefaultUserPermissions(). Lets an authenticated user cancel pending executions whose creator UUID matches their own session UserID. RBAC matrix (session-authed branch — token == ""): | role / verb | own row | someone else's | legacy NULL row | |---------------------|---------|----------------|-----------------| | admin | allow | allow | allow | | user (cancel-own) | allow | 403 | 403 | | user (no verb) | 403 | 403 | 403 | Pre-flight findings: - Permissions are {action, resource} tuples in internal/auth/types.go. Admin gets a synthetic {admin, *} via DefaultAdminPermissions(); the user role gets six entries from DefaultUserPermissions(). The issue's "cancel:any_execution" / "cancel:own_executions" map cleanly onto Action="cancel-any"/"cancel-own" + Resource=purchases. - purchase_executions had NO created_by_user_id column. Migration 41 adds it as a nullable UUID FK to users(id) ON DELETE SET NULL. NULL is the legitimate state for scheduler-driven executions and any row written before the migration. The cancel handler treats NULL as "not the current user", so legacy rows fall through to cancel-any (admins) or the existing email token in the inbox; we never lock a user out of a pending approval they could already act on. - executePurchase now stamps the session UserID onto the row when it's a real UUID (not the "admin-api-key" sentinel from the API-key path), populating future rows for the cancel-own check. - cancelPurchase keeps a two-mode dispatch: token != "" runs the legacy email-token flow unchanged; token == "" enters the new session-authed branch which enforces the matrix above and stamps session.Email onto CancelledBy for the audit trail. Frontend: - HistoryPurchase carries created_by_user_id; the Cancel button renders only when canCancelPendingRow() returns true (mirrors the backend RBAC matrix — the backend remains authoritative). - Click → confirmDialog → POST /api/purchases/cancel/:id (no token) → loadHistory(). Errors surface via toast and re-enable the button. Testing: - 7 backend tests cover all four matrix cells, the cancellable-state 409, the legacy NULL-creator rejection for non-admins, the missing-session 401, and the cancel-any allow path for non-admin roles. - 7 frontend tests cover button visibility (admin vs user vs no user vs non-pending), declined confirm, accepted confirm, and the error-surface-and-re-enable flow. Out of scope: - Retry button (issue #47). - Cancel for non-pending statuses (e.g. undo a cancellation). Closes #46
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds session-authenticated cancellation for pending/notified purchase executions, exposes a row-level Cancel button in History, records execution creators in the DB/models, introduces RBAC verbs Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend UI
participant API as Purchases API
participant Auth as Auth Service
participant DB as Database
User->>Frontend: Click Cancel on pending/notified row
Frontend->>Frontend: Show confirmDialog
User->>Frontend: Confirm
Frontend->>API: POST /api/purchases/cancel (session token, execution_id)
API->>Auth: Validate session & RBAC (cancel-own / cancel-any)
Auth-->>API: Allow / Deny
alt Denied
API-->>Frontend: 403/401
Frontend->>User: show error toast
else Allowed
API->>DB: GetExecutionByID (check status, CreatedByUserID)
alt Status not cancellable
API-->>Frontend: 400/409
Frontend->>User: show error toast
else Cancellable
API->>DB: Transaction: set status=cancelled, clear suppressions, set CancelledBy
DB-->>API: OK
API-->>Frontend: 200 OK
Frontend->>Frontend: show success toast
Frontend->>API: GET /api/purchases/history (reload)
API->>DB: Query history (includes created_by_user_id)
DB-->>API: History
API-->>Frontend: Updated history
Frontend->>User: re-render list
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
frontend/src/types.ts (1)
171-176: Clarify the field contract.The backend now carries
created_by_user_idon all synthetic execution history rows, not just pending/notified ones. Consider widening the comment so the frontend contract stays accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/types.ts` around lines 171 - 176, Update the comment for the created_by_user_id field to reflect the new backend contract: mention that created_by_user_id (on the type created_by_user_id?: string) is now populated on all synthetic execution history rows synthesized from purchase_executions (not only pending/notified rows), and keep the note about how the History UI uses it to decide visibility of the inline Cancel button for non-admins (i.e., non-admins only see Cancel on rows they created) so the frontend contract remains accurate.internal/api/handler_purchases_test.go (1)
1168-1175: Assert theCancelledByaudit stamp in the happy-path tests.These cases currently prove the status transition, but not the new audit behavior. If
cancelPurchaseViaSessionstops persistingCancelledBy = session.Email, this suite still passes becauseSavePurchaseExecutionis only stubbed, not inspected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler_purchases_test.go` around lines 1168 - 1175, Update the happy-path test to assert the audit stamp by capturing the execution passed to SavePurchaseExecution: when building the handler with buildSessionCancelHandler, change the mockConfig.SavePurchaseExecution stub to use mock.Run (or an argument capture) to inspect the saved execution and assert its CancelledBy equals session.Email (or set expectation that SavePurchaseExecution is called with an execution whose CancelledBy == session.Email); keep the existing TransitionExecutionStatus and result assertions but add this capture/assertion against the saved execution for cancelExecID in the cancelPurchase call path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/history.ts`:
- Around line 291-309: canCancelPendingRow currently only allows admins or the
original creator, but the backend authorizeSessionCancel also permits non-admin
users with the "cancel-any:purchases" permission; update canCancelPendingRow to
mirror that by checking the current user's permissions (from getCurrentUser())
for "cancel-any:purchases" and returning true if present (in addition to the
existing admin check), while preserving the existing pending/notified and
created_by_user_id creator logic; reference canCancelPendingRow and
authorizeSessionCancel when making the change.
- Around line 416-439: The try block currently wraps both api.cancelPurchase(id)
and loadHistory(), causing a failed refresh to be treated as a cancel failure;
refactor the click handler attached in
container.querySelectorAll<HTMLButtonElement>('.history-cancel-btn[data-cancel-id]')
so you only await api.cancelPurchase(id) inside the try and, on success,
immediately call showToast({ message: 'Purchase cancelled', ... }) and set
btn.disabled = true (or leave it disabled), then run loadHistory() in a separate
try/catch so any refresh errors are handled separately (e.g. show a different
toast or console.error) and do NOT re-enable the button in the refresh error
path; keep references to api.cancelPurchase, loadHistory, showToast, and
btn.disabled to locate and update the relevant logic.
In `@internal/auth/types.go`:
- Around line 278-293: The comment incorrectly states that cancel-own:purchases
is granted to "every authenticated user role by default"; update the comment to
accurately reflect that ActionCancelOwn (and ActionCancelAny) are defined here
but cancel-own:purchases is only added by DefaultUserPermissions(), while
DefaultReadOnlyPermissions() continues to return view-only permissions, so not
all authenticated roles get cancel-own by default — mention
DefaultUserPermissions and DefaultReadOnlyPermissions and clarify the actual
scope (user roles via DefaultUserPermissions, admins via
ActionCancelAny/ActionAdmin or ResourceAll).
---
Nitpick comments:
In `@frontend/src/types.ts`:
- Around line 171-176: Update the comment for the created_by_user_id field to
reflect the new backend contract: mention that created_by_user_id (on the type
created_by_user_id?: string) is now populated on all synthetic execution history
rows synthesized from purchase_executions (not only pending/notified rows), and
keep the note about how the History UI uses it to decide visibility of the
inline Cancel button for non-admins (i.e., non-admins only see Cancel on rows
they created) so the frontend contract remains accurate.
In `@internal/api/handler_purchases_test.go`:
- Around line 1168-1175: Update the happy-path test to assert the audit stamp by
capturing the execution passed to SavePurchaseExecution: when building the
handler with buildSessionCancelHandler, change the
mockConfig.SavePurchaseExecution stub to use mock.Run (or an argument capture)
to inspect the saved execution and assert its CancelledBy equals session.Email
(or set expectation that SavePurchaseExecution is called with an execution whose
CancelledBy == session.Email); keep the existing TransitionExecutionStatus and
result assertions but add this capture/assertion against the saved execution for
cancelExecID in the cancelPurchase call path.
🪄 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: 63988262-8812-41b3-a724-5dad9a5443fd
📒 Files selected for processing (17)
frontend/src/__tests__/history-cancel-button.test.tsfrontend/src/history.tsfrontend/src/types.tsinternal/api/coverage_extras_test.gointernal/api/handler_history.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/auth/service_group_test.gointernal/auth/service_test.gointernal/auth/types.gointernal/auth/types_test.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.down.sqlinternal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.up.sqlknown_issues/30_history_pending_cancel_ui.md
💤 Files with no reviewable changes (1)
- known_issues/30_history_pending_cancel_ui.md
Comment on lines
+291
to
+309
| // canCancelPendingRow returns true when the current session is permitted | ||
| // to cancel the given pending/notified history row via the session-authed | ||
| // Cancel button (issue #46). Mirror of the backend RBAC matrix in | ||
| // internal/api/handler_purchases.go::authorizeSessionCancel — keep both | ||
| // sides in sync. The backend remains authoritative; this helper is a UX | ||
| // gate (don't show buttons users can't use) and never the security | ||
| // boundary. | ||
| function canCancelPendingRow(p: HistoryPurchase): boolean { | ||
| const status = (p.status || '').toLowerCase(); | ||
| if (status !== 'pending' && status !== 'notified') return false; | ||
| const user = getCurrentUser(); | ||
| if (!user) return false; | ||
| if (user.role === 'admin') return true; | ||
| // Non-admin: only the original creator. Legacy rows with no | ||
| // created_by_user_id can't be cancelled via this UI; the email-token | ||
| // path remains the escape hatch. | ||
| if (!p.created_by_user_id) return false; | ||
| return p.created_by_user_id === user.id; | ||
| } |
There was a problem hiding this comment.
canCancelPendingRow is not actually mirroring backend auth.
authorizeSessionCancel also permits non-admin users who hold cancel-any:purchases, but this helper only allows admins or creators. Any operator/user role granted cancel-any will be allowed by the API and still never see the inline action here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/history.ts` around lines 291 - 309, canCancelPendingRow
currently only allows admins or the original creator, but the backend
authorizeSessionCancel also permits non-admin users with the
"cancel-any:purchases" permission; update canCancelPendingRow to mirror that by
checking the current user's permissions (from getCurrentUser()) for
"cancel-any:purchases" and returning true if present (in addition to the
existing admin check), while preserving the existing pending/notified and
created_by_user_id creator logic; reference canCancelPendingRow and
authorizeSessionCancel when making the change.
Member
Author
There was a problem hiding this comment.
Verified against current code. The intentional UX trade-off is documented inline at lines 303-310 of frontend/src/history.ts: surfacing the inline Cancel for non-admin custom-operator roles holding cancel-any:purchases would require fetching /me/permissions and extending the frontend User type to carry permissions. No default role grants cancel-any (it's reserved for future operator roles), so the practical impact today is zero. The backend authorizeSessionCancel remains the security boundary; if a custom role with cancel-any is added in the future, the inline comment explicitly flags broadening this helper as the follow-up. Skipping here to keep the PR scoped to RBAC + the inline button.
There was a problem hiding this comment.
@cristim, understood — the rationale is well-reasoned and the inline comment (the "Caveat" block in canCancelPendingRow) already documents the divergence clearly for future maintainers, including the explicit pointer to extend User to carry permissions if an operator role with cancel-any:purchases is ever introduced. Happy to leave it as-is.
✏️ Learnings added
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 145
File: frontend/src/history.ts:291-322
Timestamp: 2026-04-27T21:44:58.439Z
Learning: In `frontend/src/history.ts`, `canCancelPendingRow` intentionally does NOT mirror the backend `authorizeSessionCancel` for non-admin users holding `cancel-any:purchases`. This is a deliberate UX trade-off: no default role grants `cancel-any` (it is reserved for future operator roles), so the practical impact is zero today. Adding the check would require a `/me/permissions` round-trip and extending the `User` type to carry permissions — deemed out of scope. The backend `authorizeSessionCancel` in `internal/api/handler_purchases.go` remains the security boundary. If an operator role with `cancel-any:purchases` is introduced in the future, `canCancelPendingRow` and the `User` type should be extended accordingly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Four review findings, all addressed in the same changeset:
1. Major — `cancelPurchaseViaSession` skipped suppression cleanup.
The token-flow `purchase.Manager.CancelExecution` deletes
`purchase_suppressions` rows in the same tx as the status flip so
cancelled executions stop hiding capacity from recommendations.
The new session-flow only ran `TransitionExecutionStatus`, leaving
suppressions in place until the grace window expired. Refactor to
`WithTx { SavePurchaseExecutionTx; DeleteSuppressionsByExecutionTx }`
matching the email-token path. Status guard moves into
`SavePurchaseExecutionTx`'s natural ON CONFLICT semantics +
the explicit pending/notified state check before the tx.
2. Major — frontend cancel/refresh shared one try/catch. A successful
POST followed by a failed `loadHistory()` rendered a misleading
"Failed to cancel" toast even though the purchase WAS cancelled.
Split into two try blocks: cancel result drives the success/error
toast and button re-enable; reload failure logs but doesn't
override the success surface. Adds a regression test
(`successful cancel + failed reload still surfaces success toast`).
3. Major — `canCancelPendingRow` didn't mirror `cancel-any` for non-
admin operator roles. The frontend can't reach the user's
permission list today (User type doesn't carry it), so the
heuristic stays admin-or-creator. Documented the divergence
honestly: this is a UX gate; the backend is authoritative; if a
non-admin operator role is ever granted cancel-any, extend User
to carry permissions and broaden this check. The backend will
surface 403 if the heuristic is wrong-positive, which the click
handler already handles via toast.
4. Minor — comment on ActionCancelOwn/Any overstated default scope.
Said "every authenticated user role" but RoleReadOnly does NOT
get cancel-own. Rewrote the comment to enumerate role-by-role
defaults explicitly.
Plus the type-comment widening in `frontend/src/types.ts`
(CodeRabbit nitpick): the field is populated on every synthetic
purchase_executions row the History endpoint returns, not just
pending/notified ones. Comment now matches.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
The session-authed cancel matrix in TestHandler_cancelPurchase_Session_* only verified the status flip through SavePurchaseExecution being called with mock.Anything. If cancelPurchase ever stopped stamping session.Email onto CancelledBy (or stamped the wrong value) the matrix would still pass because the saved execution was never inspected. Capture the *config.PurchaseExecution pointer the mock receives via mock.Run, then assert Status=="cancelled" and (when session.Email is set) CancelledBy==session.Email. The History UI's annotateCancelled helper relies on this audit-stamp for attribution, so locking it behind a test prevents a silent regression. Addresses CodeRabbit nitpick on PR #145.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/handler_purchases_test.go (1)
1137-1149: Tighten mock expectation assertions to reduce false positives.
buildSessionCancelHandlerwires auth expectations, butrunSessionCancelAlloweddoes not assert they were actually consumed. Adding expectation assertions will better protect the session-auth security boundary in regression scenarios.♻️ Proposed test-hardening diff
func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, session *Session, hasAny, hasOwn bool) { t.Helper() - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, hasAny, hasOwn) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, hasAny, hasOwn) var saved *config.PurchaseExecution mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }). Return(nil) result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.NoError(t, err) assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + mockAuth.AssertCalled(t, "ValidateSession", mock.Anything, "sess-tok") + mockAuth.AssertExpectations(t) mockConfig.AssertCalled(t, "SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")) require.NotNil(t, saved, "SavePurchaseExecution should have captured the execution") assert.Equal(t, "cancelled", saved.Status) if session != nil && session.Email != "" { require.NotNil(t, saved.CancelledBy, "CancelledBy must be stamped when session has an email") assert.Equal(t, session.Email, *saved.CancelledBy, "CancelledBy must equal session.Email for audit attribution") } }Also applies to: 1171-1196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler_purchases_test.go` around lines 1137 - 1149, The test helper buildSessionCancelHandler sets up MockAuthService expectations (mockAuth.On for ValidateSession, HasPermissionAPI) but the tests (e.g., runSessionCancelAllowed and other cases around the 1171-1196 block) never assert those expectations were met; update the tests to call AssertExpectations on the mocks after invoking the handler to tighten assertions — specifically call mockAuth.AssertExpectations(t) and mockConfig.AssertExpectations(t) (or AssertCalled/AssertNumberOfCalls as appropriate) at the end of runSessionCancelAllowed and the other session-cancel test flows so the ValidateSession and HasPermissionAPI expectations set in buildSessionCancelHandler are actually verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/handler_purchases_test.go`:
- Around line 1137-1149: The test helper buildSessionCancelHandler sets up
MockAuthService expectations (mockAuth.On for ValidateSession, HasPermissionAPI)
but the tests (e.g., runSessionCancelAllowed and other cases around the
1171-1196 block) never assert those expectations were met; update the tests to
call AssertExpectations on the mocks after invoking the handler to tighten
assertions — specifically call mockAuth.AssertExpectations(t) and
mockConfig.AssertExpectations(t) (or AssertCalled/AssertNumberOfCalls as
appropriate) at the end of runSessionCancelAllowed and the other session-cancel
test flows so the ValidateSession and HasPermissionAPI expectations set in
buildSessionCancelHandler are actually verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9307b493-51b3-4e14-b1a6-6e515453518b
📒 Files selected for processing (1)
internal/api/handler_purchases_test.go
…sion-cancel branch CodeRabbit nitpick: `buildSessionCancelHandler` wires `ValidateSession` and `HasPermissionAPI` expectations on `MockAuthService`, but no test asserted they were actually consumed. A regression that bypassed `ValidateSession` (or stopped consulting `HasPermissionAPI` for non-admins) would silently still pass the status / audit assertions. Capture `mockAuth` (previously discarded with `_`) in `runSessionCancelAllowed` and the four reject-path tests (`CancelOwn_RejectsNonCreator`, `NoVerb_Rejects`, `RejectsTerminalStatus`, `LegacyNullCreator_NonAdminRejected`), then call `mockAuth.AssertExpectations(t)` at the end of each. `HasPermissionAPI` mocks are registered with `.Maybe()` already, so admin-role tests (which never reach the verb check) still pass. go test ./internal/api/... clean.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced Apr 27, 2026
cristim
added a commit
that referenced
this pull request
Apr 27, 2026
…token-entropy PR #145 (`feat(history): inline Cancel button + cancel-any/own RBAC verbs`) landed on the base while #101 was in flight. Both touch `internal/api/handler_purchases.go`'s pending-execution construction in `executePurchase`: - #101 hardens the path by extracting `newPendingExecution(req, total, savings)` so the approval token comes from `crypto/rand` via `common.GenerateApprovalToken` instead of the previous inline `uuid.New().String()`. - #145 adds `CreatedByUserID: resolveCreatorUserID(session)` to every pending execution so the cancel-own RBAC path (issue #46) can identify the creator on later cancellation. The two changes are semantically independent — keeping both. Resolution: keep #101's `newPendingExecution(...)` call, then set `execution.CreatedByUserID = resolveCreatorUserID(session)` immediately after construction. The helper stays signature-stable (no session parameter to plumb) and the cancel-own stamp is in place for every code path that goes through `executePurchase`. Other files in the merge auto-resolved cleanly (`internal/api/coverage_extras_test.go`, `internal/api/handler_purchases_test.go`, `internal/api/handler_test.go`). `go build ./...`, `go test ./...`, `go test ./...` from `pkg/` all clean.
8 tasks
cristim
added
triaged
Merged
5 tasks
cristim
added a commit
that referenced
this pull request
Apr 29, 2026
… threshold + ops-hint (closes #47) (#168) * feat(history): inline Retry button + retry-any/own RBAC + linkage + threshold + ops-hint Closes #47. Salvages a previously-stalled agent attempt: the work was implemented but never committed; rebased onto current feat/multicloud-web-frontend tip which now includes PR #145 (cancel button + cancel-{any,own} RBAC + migration 000041_purchase_executions_created_by_user_id). The retry plumbing mirrors the cancel pattern row-for-row: same session-validation gate, same authorize-{any,own} matrix, same legacy-NULL-creator semantics for non-admins. Backend ======= * New POST /api/purchases/retry/{id} (AuthUser). Creates a new pending execution from a *failed* row's stored Recommendations and links the predecessor with retry_execution_id. Three writes happen in one tx (successor INSERT, predecessor UPSERT, suppression INSERT) so a crash mid-flow leaves either everything or nothing. * RBAC: ActionRetryOwn / ActionRetryAny in internal/auth/types.go, retry-own:purchases added to DefaultUserPermissions(). Admins short-circuit, retry-any holders may retry any row, retry-own holders may retry only their own. Test matrix mirrors cancel. * Already-retried guard: a row whose retry_execution_id is already set rejects with 409 + retry_execution_id detail so a stale-cache double-tap can't silently overwrite the lineage pointer. * Persistent-failure block (Q3): handler resolves an ops_hint from a small map of known-persistent failure substrings (FROM_EMAIL not configured, SES sandbox, etc.) and returns 409 with the hint in structured detail fields. The History row also surfaces the same hint inline, replacing the Retry button entirely. * Threshold soft-block (Q2): retry_attempt_n on the new row is predecessor.retry_attempt_n + 1; an attempt that would land at >= 5 is rejected with 409 + retry_attempt_n / threshold details. The frontend confirms-with-warning then re-POSTs with ?force=true to override. * Migration 000042 adds nullable retry_execution_id (self-FK ON DELETE SET NULL) and retry_attempt_n INT NOT NULL DEFAULT 0, plus a partial index on the FK for the rare populated case. store_postgres.go INSERT/UPSERT/SELECT cover the new columns; retry_execution_id is updateable via ON CONFLICT (the retry handler re-saves the predecessor to set the pointer), but created_by_user_id and retry_attempt_n stay INSERT-only so the scheduler's status-only UPSERTs don't rewrite provenance. * clientError grew an optional details map so ops_hint / retry_attempt_n / threshold can reach the frontend as structured JSON fields instead of substring-matched message text. Frontend ======== * History column doubles as the per-row action surface (renderActionCell): Cancel on cancellable pending rows (#46), Retry on retryable failed rows (this change), ops-hint badge on persistent failures, and inline lineage cross-links on retried rows. Status-driven mutually-exclusive decision tree. * canRetryFailedRow gates the button: status=failed AND no ops_hint AND no retry_execution_id (already retried) AND admin OR matching created_by_user_id. UX gate only — backend remains authoritative. * Threshold-reached rows render an "⚠ Retried 5× — click to override" button; the click confirms then POSTs with force=true. * RetryPurchaseResult exported from frontend/src/api so callers can consume execution_id / retry_attempt_n. Slice-aliasing fix ================== The retry handler's new execution borrowed failedExec.Recommendations by slice header. Mutations downstream (internal/purchase/execution.go writes exec.Recommendations[i].{Error,Purchased,PurchaseID}) would have seen through the alias and corrupted the historical failed row's in-memory representation visible to any caller still holding the failedExec pointer. Fixed via defensive copy (append([]RecommendationRecord(nil), failedExec.Recommendations...)) with a comment explaining the in-process aliasing footgun. Tests ===== * 14 new backend table tests covering admin/retry-any/retry-own paths, non-failed rejection, legacy-null-creator non-admin rejection, persistent-failure block + no-match, threshold block + force override, just-under-threshold allow, already-retried 409, missing-session rejection. * 11 new frontend jest tests covering button visibility per role, ops-hint replacement, threshold-button text + force=true wiring, lineage link rendering, retry_attempt_n badge, declined dialog skips API, accepted posts + reloads + toasts, API failure surfaces toast and re-enables button. * Auth permission-count tests bumped 7→8 (default user) and 9→10 (user + 2 groups) to account for the new retry-own grant. * fix(history-retry): address CodeRabbit feedback on PR #168 Three actionable findings from CR's first review (PR #168): 1. **Major / security**: authorize before the already-retried guard. The `failedExec.RetryExecutionID` 409 ran ahead of `authorizeSessionRetry`, so any signed-in user who could guess a failed-row UUID would learn it had been retried AND get the descendant execution ID — a cross-user info leak. Reordered the gate sequence so RBAC runs first; the descendant UUID is now surfaced only after the caller has proven they're allowed to act on the row. New regression test `TestHandler_retryPurchase_AlreadyRetried_RBACBeforeLeak` asserts the 403 path doesn't carry retry_execution_id in the details map. 2. **Major / correctness**: preserve PlanID + StepNumber on retry successors. Without this, retrying a failed planned execution dropped the new row out of plan-scoped history and lost its ramp-step attribution. Both fields now propagate from the predecessor; for ad-hoc executions PlanID="" and StepNumber=0 so propagation is a no-op. New regression test `TestHandler_retryPurchase_PreservesPlanMetadata` asserts the propagation. 3. **Minor / UX**: surface structured retry hints in the failure toast. The catch path used to discard `ops_hint` / `retry_attempt_n` / `threshold` from a 4xx body and toast only `err.message`, leaving the user with a generic failure instead of the operator-actionable hint or override guidance the backend already returns. To wire this up: - `ApiError` gained an optional `details: Record<string, unknown>` field (frontend/src/api/types.ts). - `apiRequest` now extracts ALL non-`error` keys from a 4xx body into `error.details` (frontend/src/api/client.ts) so callers can branch on machine-readable hints without parsing message text. - The retry catch in history.ts inspects `err.details.ops_hint` first, then falls back to a "already retried n× (threshold k) — confirm the override prompt to force" message synthesised from `retry_attempt_n` + `threshold`, then to `err.message`. Plus two nits from the same review: 4. **Defensive copy of clientError details map**. The previous shape stored the constructor's input map by reference; a caller mutating the map after construction would reach into the error's payload. `cloneDetails` shallow-copies on both construction and `Details()` read so the error is sealed. 5. **Explicit retry-field assertions in pgxmock GetExecutionByID tests**. Two new assertions (one for the NULL/0 legacy-row case, one for a populated chain-position-2 case) guard against scan- order regressions in the new `retry_execution_id` / `retry_attempt_n` columns. A column reorder upstream would scan into the wrong destination and fail at least one of these. All 1337 frontend tests + ~16 backend retry/auth/config tests pass. gocyclo / gofmt / go vet pre-commit hooks pass.
This was referenced Apr 30, 2026
4 tasks
cristim
added a commit
that referenced
this pull request
May 5, 2026
… RBAC (#286) (#299) * feat(api,history): in-dashboard purchase approval + approve-{any,own} 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. * fix(history,api): disable both row-actions during click + tighten empty-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.
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.
Summary
Closes #46. Adds an inline Cancel button on pending Purchase History rows so a user can abort an in-flight approval from the dashboard instead of digging through the inbox for the email-token link. The token-only escape hatch stays unchanged.
RBAC decision (Option C)
Two new permission verbs in
internal/auth/types.go:cancel-any+purchases— admin-only by default (covered byRoleAdmin's implicit{admin, *}). Lets an admin cancel ANY user's pending execution. The verb exists as a distinct constant so a future non-admin operator role can be granted broad cancel rights without escalating to admin.cancel-own+purchases— added toDefaultUserPermissions(). Lets an authenticated user cancel pending executions whosecreated_by_user_idmatches their session UserID.Authorization matrix (session-authed branch —
?token=empty)cancel-own)Legacy rows with NULL creator stay reachable by admins (cancel-any) or via the email token already in the user's inbox — we never lock anyone out.
Pre-flight findings
internal/auth/types.goexposesAction+Resourcetuples. Admin gets a synthetic{admin, *}fromDefaultAdminPermissions(); the user role gets six entries fromDefaultUserPermissions(). The issue'scancel:any_execution/cancel:own_executionsmap cleanly ontocancel-any/cancel-own×purchases.purchase_executionshad nocreated_by_user_idcolumn. Migration000041adds it as nullableUUID REFERENCES users(id) ON DELETE SET NULL. NULL is the legitimate state for scheduler-driven executions and any pre-migration row. The cancel handler treats NULL as "not the current user".cancelPurchasewas AuthPublic + token-gated (?token=…). Extended in-place to a two-mode dispatch:token != ""runs the legacy email-token flow unchanged;token == ""enters the new session-authed branch which enforces the matrix above and stampssession.EmailontoCancelledByso History renders "cancelled by <email>".Backend changes
000041_purchase_executions_created_by_user_id(up + down).ActionCancelAny/ActionCancelOwnand inclusion inDefaultUserPermissions().PurchaseExecution.CreatedByUserID *string+ persistence inSavePurchaseExecutionTx(INSERT-only — never overwritten by ON CONFLICT).executePurchasestamps the session UserID onto new rows when the UserID is a real UUID (resolveCreatorUserIDhelper).cancelPurchasenow dispatches token vs session paths; newcancelPurchaseViaSession,authorizeSessionCancel,requireSession.created_by_user_idon synthesised pending rows so the frontend can render the button conditionally.Frontend changes
HistoryPurchase.created_by_user_id?: string.canCancelPendingRow()inhistory.tsmirrors the backend matrix as a UX gate (backend remains authoritative).Cancelbutton in the Plan column of pending/notified rows. Click →confirmDialog→api.cancelPurchase(id)→loadHistory()→ success/error toast.Test plan
handler_purchases_test.gocovering all four matrix cells, the cancellable-state 409, the legacy NULL-creator rejection for non-admins, the missing-session 401, and the cancel-any-as-non-admin path. ExistingTestHandler_cancelPurchasehappy-path test still covers the legacy email-token flow.history-cancel-button.test.tscovering button visibility (admin, user, no-user, non-pending), declined confirm, accepted confirm + reload + toast, and the API-failure-toast-and-re-enable flow.go build ./...,go vet ./...,go test ./...clean.npx tsc --noEmit,npm run build, full Jest suite (1303 tests) clean.Out of scope
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores