From 9a2f08866d3af32f2b9a932cd29cf0d0ed7b4c74 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Apr 2026 22:50:46 +0200 Subject: [PATCH 1/4] feat(history): inline Cancel button + cancel-any/own RBAC verbs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/history-cancel-button.test.ts | 244 ++++++++++++++++++ frontend/src/history.ts | 61 ++++- frontend/src/types.ts | 6 + internal/api/coverage_extras_test.go | 17 +- internal/api/handler_history.go | 5 + internal/api/handler_purchases.go | 141 +++++++++- internal/api/handler_purchases_test.go | 189 ++++++++++++++ internal/auth/service_group_test.go | 19 +- internal/auth/service_test.go | 5 +- internal/auth/types.go | 22 ++ internal/auth/types_test.go | 4 +- internal/config/store_postgres.go | 25 +- .../config/store_postgres_pgxmock_test.go | 4 + internal/config/types.go | 17 ++ ...ase_executions_created_by_user_id.down.sql | 2 + ...chase_executions_created_by_user_id.up.sql | 27 ++ known_issues/30_history_pending_cancel_ui.md | 43 --- 17 files changed, 759 insertions(+), 72 deletions(-) create mode 100644 frontend/src/__tests__/history-cancel-button.test.ts create mode 100644 internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.down.sql create mode 100644 internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.up.sql delete mode 100644 known_issues/30_history_pending_cancel_ui.md diff --git a/frontend/src/__tests__/history-cancel-button.test.ts b/frontend/src/__tests__/history-cancel-button.test.ts new file mode 100644 index 00000000..47ebc98a --- /dev/null +++ b/frontend/src/__tests__/history-cancel-button.test.ts @@ -0,0 +1,244 @@ +/** + * History inline Cancel button tests (issue #46). + * + * Mirror of the backend RBAC matrix in + * internal/api/handler_purchases.go::authorizeSessionCancel — keep both + * sides in sync. The backend remains authoritative; these tests only + * verify the UX gate (don't render buttons users can't use). + * + * Tested matrix: + * 1. admin sees Cancel on every pending row, regardless of creator. + * 2. regular user sees Cancel only on rows they themselves created. + * 3. Anonymous (no current user cached) sees no Cancel buttons. + * 4. Cancel button absent for non-pending rows (completed, cancelled, ...). + * 5. Click + decline confirmDialog → no API call. + * 6. Click + accept → cancelPurchase + reload + toast. + * 7. cancelPurchase rejects → toast.error + button re-enabled. + */ + +import { loadHistory } from '../history'; + +jest.mock('../api', () => ({ + getHistory: jest.fn(), + cancelPurchase: jest.fn(), +})); + +jest.mock('../navigation', () => ({ + switchTab: jest.fn(), +})); + +jest.mock('../utils', () => ({ + formatCurrency: jest.fn((val) => `$${val || 0}`), + formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), + formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), + escapeHtml: jest.fn((str) => str || ''), + populateAccountFilter: jest.fn(() => Promise.resolve()), +})); + +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn(), +})); + +jest.mock('../toast', () => ({ + showToast: jest.fn(), +})); + +jest.mock('../state', () => ({ + getCurrentUser: jest.fn(), +})); + +import * as api from '../api'; +import { confirmDialog } from '../confirmDialog'; +import { showToast } from '../toast'; +import { getCurrentUser } from '../state'; + +const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', role: 'admin' }; +const REG_USER = { id: 'user-uuid', email: 'user@example.com', role: 'user' }; +const OTHER_UUID = 'other-uuid'; + +function setupDOM(): void { + // Build the test fixture programmatically rather than via innerHTML so + // the lint hook stays happy. Element list mirrors the production + // history page's controls used by loadHistory(). + while (document.body.firstChild) document.body.removeChild(document.body.firstChild); + + const mkInput = (id: string): HTMLInputElement => { + const el = document.createElement('input'); + el.type = 'date'; + el.id = id; + return el; + }; + const mkSelect = (id: string): HTMLSelectElement => { + const el = document.createElement('select'); + el.id = id; + const opt = document.createElement('option'); + opt.value = ''; + opt.textContent = 'All'; + el.appendChild(opt); + return el; + }; + const mkDiv = (id: string): HTMLDivElement => { + const el = document.createElement('div'); + el.id = id; + return el; + }; + + document.body.appendChild(mkInput('history-start')); + document.body.appendChild(mkInput('history-end')); + document.body.appendChild(mkSelect('history-provider-filter')); + document.body.appendChild(mkSelect('history-account-filter')); + document.body.appendChild(mkDiv('history-summary')); + document.body.appendChild(mkDiv('history-list')); +} + +function makeRow(overrides: Record) { + return { + purchase_id: 'exec-1', + timestamp: '2024-01-15T00:00:00Z', + provider: 'aws', + service: 'ec2', + resource_type: 't3.medium', + region: 'us-east-1', + count: 1, + term: 1, + upfront_cost: 100, + estimated_savings: 50, + plan_name: '', + status: 'pending', + ...overrides, + }; +} + +describe('History inline Cancel button (issue #46)', () => { + beforeEach(() => { + setupDOM(); + jest.clearAllMocks(); + }); + + test('admin sees Cancel on every pending row, regardless of creator', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-mine', created_by_user_id: ADMIN_USER.id }), + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_UUID }), + makeRow({ purchase_id: 'exec-legacy', created_by_user_id: undefined }), + ], + }); + + await loadHistory(); + + const buttons = document.querySelectorAll('.history-cancel-btn'); + const ids = Array.from(buttons).map((b) => b.dataset['cancelId']); + expect(ids).toEqual(expect.arrayContaining(['exec-mine', 'exec-other', 'exec-legacy'])); + expect(ids).toHaveLength(3); + }); + + test('regular user sees Cancel only on rows they created', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(REG_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-mine', created_by_user_id: REG_USER.id }), + makeRow({ purchase_id: 'exec-other', created_by_user_id: OTHER_UUID }), + makeRow({ purchase_id: 'exec-legacy', created_by_user_id: undefined }), + ], + }); + + await loadHistory(); + + const buttons = document.querySelectorAll('.history-cancel-btn'); + const ids = Array.from(buttons).map((b) => b.dataset['cancelId']); + expect(ids).toEqual(['exec-mine']); + }); + + test('renders no Cancel buttons when no current user is cached', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(null); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ created_by_user_id: 'anything' })], + }); + + await loadHistory(); + + expect(document.querySelectorAll('.history-cancel-btn')).toHaveLength(0); + }); + + test('Cancel button absent on non-pending rows even for admin', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'exec-completed', status: 'completed', created_by_user_id: ADMIN_USER.id }), + makeRow({ purchase_id: 'exec-cancelled', status: 'cancelled', created_by_user_id: ADMIN_USER.id }), + makeRow({ purchase_id: 'exec-failed', status: 'failed', created_by_user_id: ADMIN_USER.id }), + // Sanity: notified is treated like pending and SHOULD render the button. + makeRow({ purchase_id: 'exec-notified', status: 'notified', created_by_user_id: ADMIN_USER.id }), + ], + }); + + await loadHistory(); + + const ids = Array.from(document.querySelectorAll('.history-cancel-btn')) + .map((b) => b.dataset['cancelId']); + expect(ids).toEqual(['exec-notified']); + }); + + test('declined confirm dialog skips API call', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(false); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-1', created_by_user_id: ADMIN_USER.id })], + }); + + await loadHistory(); + const btn = document.querySelector('.history-cancel-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(confirmDialog).toHaveBeenCalled(); + expect(api.cancelPurchase).not.toHaveBeenCalled(); + }); + + test('accepted confirm posts cancel + reloads + toasts', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + (api.cancelPurchase as jest.Mock).mockResolvedValue(undefined); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-1', created_by_user_id: ADMIN_USER.id })], + }); + + await loadHistory(); + expect(api.getHistory).toHaveBeenCalledTimes(1); + + const btn = document.querySelector('.history-cancel-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + expect(api.cancelPurchase).toHaveBeenCalledWith('exec-1'); + // Reload was triggered → second getHistory call. + expect(api.getHistory).toHaveBeenCalledTimes(2); + expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'success' })); + }); + + test('cancel API failure surfaces toast and re-enables the button', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + (api.cancelPurchase as jest.Mock).mockRejectedValue(new Error('boom')); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-1', created_by_user_id: ADMIN_USER.id })], + }); + console.error = jest.fn(); + + await loadHistory(); + const btn = document.querySelector('.history-cancel-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' })); + expect(btn?.disabled).toBe(false); + }); +}); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 3e75b2da..b0ec163a 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -6,6 +6,9 @@ import * as api from './api'; import { formatCurrency, formatDate, formatTerm, escapeHtml, populateAccountFilter } from './utils'; import type { HistoryResponse, HistorySummary, HistoryPurchase } from './types'; import { switchTab } from './navigation'; +import { confirmDialog } from './confirmDialog'; +import { showToast } from './toast'; +import { getCurrentUser } from './state'; const VALID_PROVIDERS: api.Provider[] = ['aws', 'azure', 'gcp']; @@ -285,6 +288,26 @@ function providerCell(p: HistoryPurchase): string { return `${p.provider.toUpperCase()}`; } +// 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; +} + function renderHistoryList(purchases: HistoryPurchase[]): void { const container = document.getElementById('history-list'); if (!container) return; @@ -328,6 +351,13 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { return badge; })(); const execIdAttr = p.purchase_id ? ` data-execution-id="${escapeHtml(p.purchase_id)}"` : ''; + // Inline Cancel button on pending/notified rows the current user is + // permitted to cancel. Renders in the Plan column so the table + // width stays the same; pending rows show their plan in + // StatusDescription, not the Plan column, so this is non-conflicting. + const planCellContent = canCancelPendingRow(p) && p.purchase_id + ? `` + : escapeHtml(p.plan_name || '-'); return ` ${statusCell} @@ -340,7 +370,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { ${formatTerm(p.term)} ${formatCurrency(p.upfront_cost)} ${formatCurrency(p.estimated_savings)} - ${escapeHtml(p.plan_name || '-')} + ${planCellContent} `; }).join(''); @@ -379,6 +409,35 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { }); }); + // Wire the inline Cancel button on pending/notified rows the current + // session may cancel (issue #46). confirmDialog → POST → reload. The + // backend remains the security boundary; this UX gate just hides the + // button when the call would 403. + container.querySelectorAll('.history-cancel-btn[data-cancel-id]').forEach(btn => { + btn.addEventListener('click', async () => { + const id = btn.dataset['cancelId']; + if (!id) return; + const ok = await confirmDialog({ + title: 'Cancel this pending purchase?', + body: 'This will permanently abort the approval flow. The pending email approval link will stop working. This action cannot be undone.', + confirmLabel: 'Cancel purchase', + destructive: true, + }); + if (!ok) return; + btn.disabled = true; + try { + await api.cancelPurchase(id); + showToast({ message: 'Purchase cancelled', kind: 'success', timeout: 5_000 }); + await loadHistory(); + } catch (cancelError) { + console.error('Failed to cancel pending purchase:', cancelError); + const err = cancelError as Error; + showToast({ message: `Failed to cancel: ${err.message || 'unknown error'}`, kind: 'error' }); + btn.disabled = false; + } + }); + }); + // Scroll + flash the deep-link target if the URL hash carries a // ?execution=. The suppression badge on the Recommendations // view links here so the user lands on the relevant row without diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 34c2fd4f..53d273cc 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -168,6 +168,12 @@ export interface HistoryPurchase { // non-ok rows. "failed" → backend's send-error message; "expired" → canned // 7-day-window reminder. Empty on completed / pending rows. status_description?: string; + // CreatedByUserID: UUID of the user who created the underlying execution. + // Set only on pending/notified rows (synthesised from purchase_executions); + // empty on completed history rows. The History UI uses it to decide + // whether the inline Cancel button (issue #46) is visible to the current + // session — non-admins only see it on rows they themselves created. + created_by_user_id?: string; } // Savings Analytics types diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index bd6d9c4b..a78558ee 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -71,11 +71,20 @@ func TestHandler_cancelPurchase_InvalidUUID(t *testing.T) { assert.Contains(t, err.Error(), "invalid ID format") } -func TestHandler_cancelPurchase_EmptyToken(t *testing.T) { - h := &Handler{} - _, err := h.cancelPurchase(context.Background(), nil, "11111111-1111-1111-1111-111111111111", "") +// TestHandler_cancelPurchase_EmptyToken_FallsThroughToSession asserts that +// the token-empty branch no longer short-circuits with "cancellation token +// is required" — the empty-token path is now the dispatch into the +// session-authed cancel flow (issue #46). Without an execution to load, +// GetExecutionByID is the first thing that runs; with no config wired, +// the call surfaces a downstream error rather than the legacy 400. +func TestHandler_cancelPurchase_EmptyToken_FallsThroughToSession(t *testing.T) { + execID := "11111111-1111-1111-1111-111111111111" + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, execID).Return(nil, errors.New("store error")) + h := &Handler{config: mockConfig} + _, err := h.cancelPurchase(context.Background(), nil, execID, "") assert.Error(t, err) - assert.Contains(t, err.Error(), "cancellation token is required") + assert.Contains(t, err.Error(), "failed to get execution") } func TestHandler_cancelPurchase_PurchaseError(t *testing.T) { diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index dd061157..60ccc6f5 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -156,6 +156,10 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver string) confi if exec.CloudAccountID != nil { accountID = *exec.CloudAccountID } + var createdBy string + if exec.CreatedByUserID != nil { + createdBy = *exec.CreatedByUserID + } row := config.PurchaseHistoryRecord{ AccountID: accountID, PurchaseID: exec.ExecutionID, @@ -168,6 +172,7 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver string) confi EstimatedSavings: exec.EstimatedSavings, PlanID: exec.PlanID, Status: exec.Status, + CreatedByUserID: createdBy, } annotateHistoryRowByStatus(&row, exec, approver) return row diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4da01ac7..6f284e38 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/email" "github.com/LeanerCloud/CUDly/pkg/common" @@ -294,9 +295,6 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction if err := validateUUID(execID); err != nil { return nil, err } - if token == "" { - return nil, NewClientError(400, "cancellation token is required") - } execution, err := h.config.GetExecutionByID(ctx, execID) if err != nil { @@ -305,18 +303,128 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction if execution == nil { return nil, NewClientError(404, "execution not found") } - actor, err := h.authorizeApprovalAction(ctx, req, execution) + + // Two-mode dispatch (issue #46): + // * token != "" → legacy email-link flow. Token possession proves + // intent; the handler still requires the session email (when one + // exists) to be on the authorised-approver list, then delegates + // to the purchase service which validates the token itself. + // * token == "" → session-authed dashboard Cancel button. The + // session must carry a permission allowing the cancel: + // - cancel-any:purchases (admin) → any pending execution; or + // - cancel-own:purchases (default user) AND the execution's + // created_by_user_id matches the session UserID. + // Legacy rows with NULL created_by_user_id are reachable only + // via cancel-any (admin) or via the email token already in the + // inbox — they do not become orphaned by this change. + if token != "" { + actor, err := h.authorizeApprovalAction(ctx, req, execution) + if err != nil { + return nil, err + } + if err := h.purchase.CancelExecution(ctx, execID, token, actor); err != nil { + return nil, err + } + return map[string]string{"status": "cancelled"}, nil + } + + return h.cancelPurchaseViaSession(ctx, req, execution) +} + +// cancelPurchaseViaSession is the session-authed branch of cancelPurchase. +// Enforces the cancel-any/cancel-own RBAC matrix, validates the execution +// is in a cancellable state (pending|notified), atomically transitions the +// row to "cancelled", and stamps the cancelling user's email onto +// CancelledBy. The History UI's annotateCancelled() helper renders that +// into "cancelled by " at read time — see handler_history.go. +func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (any, error) { + session, err := h.requireSession(ctx, req) if err != nil { return nil, err } - if err := h.purchase.CancelExecution(ctx, execID, token, actor); err != nil { + if execution.Status != "pending" && execution.Status != "notified" { + return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled (status=%s)", execution.ExecutionID, execution.Status)) + } + + if err := h.authorizeSessionCancel(ctx, session, execution); err != nil { return nil, err } + updated, err := h.config.TransitionExecutionStatus(ctx, execution.ExecutionID, []string{"pending", "notified"}, "cancelled") + if err != nil { + return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", execution.ExecutionID, err)) + } + + // Persist who cancelled the row so the History view can show + // "cancelled by " instead of falling back to the notification + // inbox. Best-effort — a write failure does not undo the status + // transition; the row is already cancelled. + if session.Email != "" { + actor := session.Email + updated.CancelledBy = &actor + if err := h.config.SavePurchaseExecution(ctx, updated); err != nil { + logging.Warnf("cancelPurchaseViaSession: failed to persist cancelled_by for %s: %v", execution.ExecutionID, err) + } + } + return map[string]string{"status": "cancelled"}, nil } +// authorizeSessionCancel returns nil when the session is permitted to cancel +// the given execution under the cancel-any / cancel-own RBAC rules added in +// issue #46. Returns a 403 ClientError otherwise. +func (h *Handler) authorizeSessionCancel(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { + if session.Role == "admin" { + return nil + } + if h.auth == nil { + return NewClientError(500, "authentication service not configured") + } + + hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionCancelAny, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if hasAny { + return nil + } + + hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionCancelOwn, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if !hasOwn { + return NewClientError(403, "permission denied: requires cancel-any or cancel-own on purchases") + } + + if execution.CreatedByUserID == nil || *execution.CreatedByUserID != session.UserID { + return NewClientError(403, "permission denied: cannot cancel another user's pending purchase") + } + return nil +} + +// requireSession validates the request's session token and returns the +// session, or a 401 ClientError when no/invalid session is present. Unlike +// requirePermission, it does NOT consult the admin-API-key shortcut — the +// session-authed cancel path needs an actual user UUID for the +// cancel-own check; falling through to the API-key admin role would let +// a key impersonate ownership we cannot verify. +func (h *Handler) requireSession(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) { + if h.auth == nil { + return nil, fmt.Errorf("authentication service not configured") + } + token := h.extractBearerToken(req) + if token == "" { + return nil, NewClientError(401, "no authorization token provided") + } + session, err := h.auth.ValidateSession(ctx, token) + if err != nil || session == nil { + return nil, NewClientError(401, "invalid session") + } + return session, nil +} + // tryResolveActorEmail returns the email of the session-authenticated user // who made the request, or "" when the request carries no valid session. // Best-effort: the approve/cancel routes are AuthPublic (token-only), so @@ -489,6 +597,27 @@ func validateAndTotalRecommendations(recs []config.RecommendationRecord) (upfron return upfront, savings, nil } +// resolveCreatorUserID returns a pointer to the session user's UUID for +// stamping onto purchase_executions.created_by_user_id, or nil for +// non-user sessions whose UserID isn't a real UUID. This keeps the +// cancel-own RBAC check (issue #46) honest: +// - Real human session → pointer to UUID; future cancel-own match works. +// - Admin-API-key session → UserID is the literal "admin-api-key" +// sentinel, which fails validateUUID; store NULL so the FK to users +// stays valid and the row is reachable only via cancel-any/email-token. +// - nil session (defensive — current callers gate on requirePermission +// so this shouldn't happen) → NULL. +func resolveCreatorUserID(session *Session) *string { + if session == nil { + return nil + } + if validateUUID(session.UserID) != nil { + return nil + } + uid := session.UserID + return &uid +} + // executePurchase handles direct purchase execution from recommendations func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { execReq, session, err := h.validateExecutePurchaseRequest(ctx, req) @@ -500,7 +629,6 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio if err != nil { return nil, err } - _ = session executionID := uuid.New().String() execution := &config.PurchaseExecution{ @@ -513,6 +641,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio ApprovalToken: uuid.New().String(), Source: common.PurchaseSourceWeb, CapacityPercent: execReq.CapacityPercent, + CreatedByUserID: resolveCreatorUserID(session), } // Load the grace-period config once before entering the tx so a diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index ff6a2456..cc81290d 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1111,3 +1111,192 @@ func TestHandler_pausePlannedPurchase_OutOfScope(t *testing.T) { assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) mockStore.AssertNotCalled(t, "TransitionExecutionStatus") } + +// ─── Session-authed Cancel (issue #46) ───────────────────────────────────── +// +// Covers the full cancel-any / cancel-own RBAC matrix on the +// session-authed branch of cancelPurchase (token == ""): +// +// 1. admin → allowed (any execution) +// 2. user with cancel-any (e.g. ops role) → allowed (any execution) +// 3. user with cancel-own + matching creator → allowed +// 4. user with cancel-own + different creator → 403 +// 5. user with neither verb → 403 +// 6. cancellable-state guard → 409 on non-pending status +// 7. legacy NULL creator + non-admin cancel-own → 403 (still reachable +// via the email-token path, which is exercised by the existing +// TestHandler_cancelPurchase happy-path test). + +const cancelExecID = "55555555-5555-5555-5555-555555555555" +const cancelCallerID = "66666666-6666-6666-6666-666666666666" +const cancelOtherID = "77777777-7777-7777-7777-777777777777" + +// buildSessionCancelHandler wires the handler with mocks the session-authed +// cancel tests share. Token is left empty by callers when invoking +// cancelPurchase to drive the new branch. +func buildSessionCancelHandler(exec *config.PurchaseExecution, session *Session, hasAny, hasOwn bool) (*Handler, *MockConfigStore, *MockAuthService) { + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil) + if session != nil && session.Role != "admin" { + mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-any", "purchases").Return(hasAny, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-own", "purchases").Return(hasOwn, nil).Maybe() + } + + return &Handler{config: mockConfig, auth: mockAuth}, mockConfig, mockAuth +} + +func sessionCancelReq() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } +} + +func TestHandler_cancelPurchase_Session_Admin_AllowsAny(t *testing.T) { + creator := cancelOtherID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "pending", + CreatedByUserID: &creator, + } + transitioned := *exec + transitioned.Status = "cancelled" + session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} + + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, false) + mockConfig.On("TransitionExecutionStatus", mock.Anything, cancelExecID, []string{"pending", "notified"}, "cancelled").Return(&transitioned, nil) + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.NoError(t, err) + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) +} + +func TestHandler_cancelPurchase_Session_CancelAny_AllowsAny(t *testing.T) { + // Non-admin operator role with cancel-any:purchases. Future use case + // (no role currently has it by default) but the verb exists today. + creator := cancelOtherID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "pending", + CreatedByUserID: &creator, + } + transitioned := *exec + transitioned.Status = "cancelled" + session := &Session{UserID: cancelCallerID, Role: "user", Email: "ops@example.com"} + + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, true, false) + mockConfig.On("TransitionExecutionStatus", mock.Anything, cancelExecID, []string{"pending", "notified"}, "cancelled").Return(&transitioned, nil) + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.NoError(t, err) + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) +} + +func TestHandler_cancelPurchase_Session_CancelOwn_AllowsCreator(t *testing.T) { + creator := cancelCallerID // execution created by the same user + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "notified", + CreatedByUserID: &creator, + } + transitioned := *exec + transitioned.Status = "cancelled" + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, true) + mockConfig.On("TransitionExecutionStatus", mock.Anything, cancelExecID, []string{"pending", "notified"}, "cancelled").Return(&transitioned, nil) + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.NoError(t, err) + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) +} + +func TestHandler_cancelPurchase_Session_CancelOwn_RejectsNonCreator(t *testing.T) { + creator := cancelOtherID // someone else created it + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "pending", + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, true) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "another user's pending purchase") + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") +} + +func TestHandler_cancelPurchase_Session_NoVerb_Rejects(t *testing.T) { + creator := cancelCallerID // even own row is rejected without the verb + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "pending", + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, false) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "cancel-any or cancel-own") + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") +} + +func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { + creator := cancelCallerID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "completed", // already done — cannot transition + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "admin"} + + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, false) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot be cancelled") + assert.Contains(t, err.Error(), "completed") + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") +} + +func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *testing.T) { + // Pre-migration row: created_by_user_id is NULL. cancel-own can't + // match a NULL creator, so a non-admin must be rejected. The email + // token in the inbox stays the escape hatch (covered by the existing + // TestHandler_cancelPurchase happy-path test). + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "pending", + CreatedByUserID: nil, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, true) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "another user's pending purchase") + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") +} + +func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) { + exec := &config.PurchaseExecution{ExecutionID: cancelExecID, Status: "pending"} + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil) + + handler := &Handler{config: mockConfig, auth: new(MockAuthService)} + // No Authorization header → 401, not 403. Tokenless + sessionless is + // the only state where the user can't reach either branch. + _, err := handler.cancelPurchase(context.Background(), &events.LambdaFunctionURLRequest{}, cancelExecID, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "no authorization token provided") +} diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index 0bfb8399..65683950 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -255,7 +255,8 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - assert.Len(t, permissions, 6) // 5 default user permissions + // 7 = 6 read/plan-author + cancel-own:purchases (issue #46). + assert.Len(t, permissions, 7) mockStore.AssertExpectations(t) }) @@ -312,8 +313,8 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // 6 user + 1 from group1 + 1 from group2 = 8 - assert.Len(t, permissions, 8) + // 7 user (incl. cancel-own:purchases from issue #46) + 1 group1 + 1 group2 + assert.Len(t, permissions, 9) mockStore.AssertExpectations(t) }) @@ -349,8 +350,9 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // Should have only user permissions, missing group is skipped - assert.Len(t, permissions, 6) + // Should have only user permissions, missing group is skipped. + // 7 = 6 read/plan-author + cancel-own:purchases (issue #46). + assert.Len(t, permissions, 7) mockStore.AssertExpectations(t) }) @@ -427,8 +429,8 @@ func TestService_BuildAuthContext(t *testing.T) { assert.Contains(t, authCtx.AllowedAccounts, "111111111111") assert.Contains(t, authCtx.AllowedAccounts, "222222222222") assert.Contains(t, authCtx.AllowedAccounts, "333333333333") - // 6 user perms + 1 from group1 + 1 from group2 - assert.Len(t, authCtx.Permissions, 8) + // 7 user perms (incl. cancel-own:purchases from issue #46) + 1 from group1 + 1 from group2 + assert.Len(t, authCtx.Permissions, 9) mockStore.AssertExpectations(t) }) @@ -450,7 +452,8 @@ func TestService_BuildAuthContext(t *testing.T) { require.NoError(t, err) assert.NotNil(t, authCtx) assert.Empty(t, authCtx.AllowedAccounts) - assert.Len(t, authCtx.Permissions, 6) // Only role-based permissions + // 6 read/plan-author + cancel-own:purchases (issue #46) = 7. Only role-based permissions. + assert.Len(t, authCtx.Permissions, 7) mockStore.AssertExpectations(t) }) diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 7fc874ec..5d2d32a7 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -660,8 +660,9 @@ func TestService_ErrorPaths(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // Should still return user permissions even if group fetch fails - assert.Len(t, permissions, 6) + // Should still return user permissions even if group fetch fails. + // 7 = 6 read/plan-author + cancel-own:purchases (issue #46). + assert.Len(t, permissions, 7) mockStore.AssertExpectations(t) }) diff --git a/internal/auth/types.go b/internal/auth/types.go index 9e5b9f00..02d79507 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -275,6 +275,22 @@ const ( ActionExecute = "execute" ActionApprove = "approve" ActionAdmin = "admin" + // ActionCancelOwn / ActionCancelAny gate the session-authed Cancel + // button on pending Purchase History rows (issue #46). + // * cancel-own + purchases — granted to every authenticated user + // role by default; allows cancelling pending executions whose + // created_by_user_id matches the session user. Legacy rows with + // a NULL creator are out of reach for non-admins via this verb; + // admins still cancel them via cancel-any (or implicit admin). + // * cancel-any + purchases — granted via ActionAdmin/ResourceAll + // for admin role; this constant exists so future non-admin + // operator roles can be granted broad cancel rights without + // escalating to admin. Allows cancelling pending executions + // regardless of creator. + // The legacy email-token cancel path stays unchanged as an escape + // hatch and is gated by token possession, not these verbs. + ActionCancelOwn = "cancel-own" + ActionCancelAny = "cancel-any" ) // Predefined resources @@ -307,6 +323,12 @@ func DefaultUserPermissions() []Permission { {Action: ActionView, Resource: ResourceHistory}, {Action: ActionCreate, Resource: ResourcePlans}, {Action: ActionUpdate, Resource: ResourcePlans}, + // cancel-own:purchases — every authenticated user can cancel + // pending purchase executions they created themselves (issue #46). + // The handler still requires the execution to be in a cancellable + // state (pending/notified) and the creator UUID to match the + // session UserID before honouring the request. + {Action: ActionCancelOwn, Resource: ResourcePurchases}, } } diff --git a/internal/auth/types_test.go b/internal/auth/types_test.go index f2cbd918..bfb2d054 100644 --- a/internal/auth/types_test.go +++ b/internal/auth/types_test.go @@ -16,7 +16,8 @@ func TestDefaultPermissions(t *testing.T) { t.Run("DefaultUserPermissions returns user access", func(t *testing.T) { perms := DefaultUserPermissions() - assert.Len(t, perms, 6) + // 6 read/plan-author perms + cancel-own:purchases (issue #46) = 7. + assert.Len(t, perms, 7) actions := make(map[string]bool) for _, p := range perms { @@ -29,6 +30,7 @@ func TestDefaultPermissions(t *testing.T) { assert.True(t, actions[ActionView+":"+ResourceHistory]) assert.True(t, actions[ActionCreate+":"+ResourcePlans]) assert.True(t, actions[ActionUpdate+":"+ResourcePlans]) + assert.True(t, actions[ActionCancelOwn+":"+ResourcePurchases]) }) t.Run("DefaultReadOnlyPermissions returns readonly access", func(t *testing.T) { diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 01eae447..162d2876 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -656,8 +656,9 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18) + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) ON CONFLICT (execution_id) DO UPDATE SET status = $3, notification_sent = $6, @@ -686,6 +687,9 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, planIDArg = execution.PlanID } + // created_by_user_id is INSERT-only: the original creator must not be + // rewritten by an ON CONFLICT update (e.g. the scheduler upserting status + // transitions). The column is omitted from the DO UPDATE SET clause above. _, err = tx.Exec(ctx, query, planIDArg, execution.ExecutionID, @@ -705,6 +709,7 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, execution.ApprovedBy, execution.CancelledBy, capacityPercent, + execution.CreatedByUserID, ) if err != nil { @@ -725,7 +730,8 @@ func (s *PostgresStore) TransitionExecutionStatus(ctx context.Context, execution RETURNING plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id ` records, err := s.queryExecutions(ctx, query, executionID, toStatus, fromStatuses) @@ -763,7 +769,8 @@ func (s *PostgresStore) GetExecutionsByStatuses(ctx context.Context, statuses [] SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id FROM purchase_executions WHERE status = ANY($1) ORDER BY scheduled_date DESC @@ -778,7 +785,8 @@ func (s *PostgresStore) GetPendingExecutions(ctx context.Context) ([]PurchaseExe SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id FROM purchase_executions WHERE status IN ('pending', 'notified') AND (expires_at IS NULL OR expires_at > NOW()) @@ -795,7 +803,8 @@ func (s *PostgresStore) GetExecutionByID(ctx context.Context, executionID string SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id FROM purchase_executions WHERE execution_id = $1 ` @@ -818,7 +827,8 @@ func (s *PostgresStore) GetExecutionByPlanAndDate(ctx context.Context, planID st SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, - cloud_account_id, source, approved_by, cancelled_by, capacity_percent + cloud_account_id, source, approved_by, cancelled_by, capacity_percent, + created_by_user_id FROM purchase_executions WHERE plan_id = $1 AND scheduled_date = $2 ` @@ -871,6 +881,7 @@ func (s *PostgresStore) queryExecutions(ctx context.Context, query string, args &exec.ApprovedBy, &exec.CancelledBy, &exec.CapacityPercent, + &exec.CreatedByUserID, ) if err != nil { return nil, fmt.Errorf("failed to scan execution: %w", err) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 51e2201c..39cb177f 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -370,12 +370,14 @@ func TestPGXMock_GetExecutionByID_Success(t *testing.T) { "notification_sent", "approval_token", "recommendations", "total_upfront_cost", "estimated_savings", "completed_at", "error", "expires_at", "cloud_account_id", "source", "approved_by", "cancelled_by", "capacity_percent", + "created_by_user_id", } rows := pgxmock.NewRows(cols).AddRow( "plan-1", "exec-1", "pending", 1, now, sql.NullTime{}, "tok-123", recsJSON, 100.0, 200.0, sql.NullTime{}, "", sql.NullTime{}, nil, "", nil, nil, 100, + nil, ) mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) @@ -399,6 +401,7 @@ func TestPGXMock_GetExecutionByID_WithTimestamps(t *testing.T) { "notification_sent", "approval_token", "recommendations", "total_upfront_cost", "estimated_savings", "completed_at", "error", "expires_at", "cloud_account_id", "source", "approved_by", "cancelled_by", "capacity_percent", + "created_by_user_id", } rows := pgxmock.NewRows(cols).AddRow( "plan-1", "exec-2", "completed", 1, now, @@ -406,6 +409,7 @@ func TestPGXMock_GetExecutionByID_WithTimestamps(t *testing.T) { 100.0, 200.0, sql.NullTime{Valid: true, Time: now}, "some error", sql.NullTime{Valid: true, Time: future}, nil, "cudly-web", nil, nil, 100, + nil, ) mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) diff --git a/internal/config/types.go b/internal/config/types.go index 46ba719c..f006f902 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -188,6 +188,15 @@ type PurchaseExecution struct { // accountable party in that case. Nullable TEXT in Postgres. ApprovedBy *string `json:"approved_by,omitempty" dynamodbav:"approved_by,omitempty"` CancelledBy *string `json:"cancelled_by,omitempty" dynamodbav:"cancelled_by,omitempty"` + // CreatedByUserID is the UUID of the session-authenticated user who + // triggered this execution (e.g. clicked Execute on the Recommendations + // page or submitted the bulk-purchase modal). NULL on rows created + // before the column was introduced (migration 000041) and on + // scheduler-driven executions where there is no human creator. Used + // by the session-authed cancel handler to enforce cancel:own_executions + // — a non-admin may cancel only executions they themselves created. + // NULL is treated as "not the current user". + CreatedByUserID *string `json:"created_by_user_id,omitempty" dynamodbav:"created_by_user_id,omitempty"` // CapacityPercent records what fraction of the originally-recommended // counts the user chose when the bulk Purchase flow submitted this // execution (1..100). Audit-only: the Recommendations slice already @@ -339,6 +348,14 @@ type PurchaseHistoryRecord struct { // the 7-day approval window elapsed. Empty on completed/pending rows — // those speak for themselves via Status alone. StatusDescription string `json:"status_description,omitempty" dynamodbav:"-"` + // CreatedByUserID propagates the originating execution's + // created_by_user_id so the History UI can decide whether to render + // the inline Cancel button (issue #46): a non-admin user only sees + // the button on their own pending rows. Set only for synthesised + // pending/notified rows (executions); empty on completed history + // rows (where the action has already completed). Excluded from DB + // persistence. + CreatedByUserID string `json:"created_by_user_id,omitempty" dynamodbav:"-"` } // RIExchangeRecord represents a record in the ri_exchange_history table diff --git a/internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.down.sql b/internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.down.sql new file mode 100644 index 00000000..bcfdc975 --- /dev/null +++ b/internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS created_by_user_id; diff --git a/internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.up.sql b/internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.up.sql new file mode 100644 index 00000000..3b81300e --- /dev/null +++ b/internal/database/postgres/migrations/000041_purchase_executions_created_by_user_id.up.sql @@ -0,0 +1,27 @@ +-- 000041: track who created a purchase execution +-- +-- Adds purchase_executions.created_by_user_id (nullable UUID, FK to users.id +-- with ON DELETE SET NULL) so the new session-authed Cancel button on +-- pending History rows can enforce the cancel:own_executions RBAC rule: +-- a non-admin may cancel a pending execution only when they themselves +-- created it. +-- +-- Nullable because: +-- * existing rows pre-date attribution and cannot be backfilled — the +-- direct-execute Recommendations flow ran without recording the +-- creator's UUID; +-- * scheduler-driven executions (ramp schedule, cron) have no human +-- creator and should record NULL; +-- * NULL is treated as "not the current user" by the cancel handler, +-- so legacy rows fall through to the cancel:any_execution path +-- (admins) or the existing email-token path. Either way they +-- remain reachable by the email token already in the inbox; we do +-- NOT lose access to existing pending approvals. +-- +-- ON DELETE SET NULL mirrors the approved_by / cancelled_by columns from +-- migration 000035 — deleting a user must not cascade-delete their audit +-- trail of executions. + +ALTER TABLE purchase_executions + ADD COLUMN created_by_user_id UUID + REFERENCES users(id) ON DELETE SET NULL; diff --git a/known_issues/30_history_pending_cancel_ui.md b/known_issues/30_history_pending_cancel_ui.md deleted file mode 100644 index 0018afe5..00000000 --- a/known_issues/30_history_pending_cancel_ui.md +++ /dev/null @@ -1,43 +0,0 @@ -# Purchase History — inline Cancel button for pending rows - -**Surfaced during:** 2026-04-23 follow-up audit after the Failed/Expired states landed (`b44283746`) -**Related commits:** `32f9e4ffc` (pending-in-history), `b44283746` (failed + expired states) -**Status:** deferred — pending rows surface the approver email but give no UI path to cancel. - -## Problem - -Once `/api/history` started merging pending executions (`32f9e4ffc`), users can -see their own in-flight approvals with the approver email rendered under the -Pending badge. What's missing is a way to **act** on them from History: - -- The approval-link cancel path (`POST /api/purchases/cancel/{id}?token=…`) is - gated by a one-time token that only lives in the email body. If the email - never went out the token is effectively orphaned; once `b3e17719b` ships - with the FROM_EMAIL wire-through the email will arrive, but users who - prefer to cancel from the dashboard rather than the inbox still can't. -- The planned-purchase endpoints (`pause` / `resume` / `delete`) are session- - authed but only accept plan-backed executions (`plan_id` populated). The - ad-hoc "Execute Purchase" flow from Recommendations creates plan-less - executions (`plan_id = ""`) so those endpoints reject them with - `execution not found` or `execution cannot transition`. - -## Proposed resolution - -Add a session-authed cancel endpoint (or broaden the existing -`cancelPurchase` to accept either `token=` or an authenticated admin -session) and wire a Cancel button on pending rows in -`frontend/src/history.ts:renderHistoryList`. The button should: - -- Show only for rows with `status = "pending"` (or `notified`). -- Require admin perms (same permission gate as Purchase - History rendering already uses). -- On click: confirm via `confirmDialog`, then `POST` to the new endpoint, - then reload history. - -## Why not now - -The token-authed cancel path is secure by design — the token in the email -is what proves intent. Adding a session-authed bypass widens the blast -radius; wants a permission-gate review (does `delete purchases` in the -RBAC table cover this, or do we add a new `cancel:own_executions` verb?). -Out of scope for the inbox-visibility milestone. From aabff3f911802afb0cc21dae6d1b5fef0ca22525 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Apr 2026 23:09:33 +0200 Subject: [PATCH 2/4] fix(history-cancel): address CodeRabbit feedback on PR #145 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../__tests__/history-cancel-button.test.ts | 34 ++++++++++ frontend/src/history.ts | 50 +++++++++++--- frontend/src/types.ts | 17 +++-- internal/api/handler_purchases.go | 43 +++++++----- internal/api/handler_purchases_test.go | 68 +++++++++---------- internal/auth/types.go | 26 ++++--- 6 files changed, 165 insertions(+), 73 deletions(-) diff --git a/frontend/src/__tests__/history-cancel-button.test.ts b/frontend/src/__tests__/history-cancel-button.test.ts index 47ebc98a..aa8b55bf 100644 --- a/frontend/src/__tests__/history-cancel-button.test.ts +++ b/frontend/src/__tests__/history-cancel-button.test.ts @@ -223,6 +223,40 @@ describe('History inline Cancel button (issue #46)', () => { expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'success' })); }); + test('successful cancel + failed reload still surfaces success toast', async () => { + // CR feedback (PR #145): the cancel POST and refresh must not share + // a try/catch — a failed reload can't be allowed to override the + // success of the cancel itself. The user already gave intent and + // the row IS cancelled on the backend; surfacing "Failed to cancel" + // would be wrong. + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + (api.cancelPurchase as jest.Mock).mockResolvedValue(undefined); + let getCallCount = 0; + (api.getHistory as jest.Mock).mockImplementation(() => { + getCallCount++; + if (getCallCount === 1) { + return Promise.resolve({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-1', created_by_user_id: ADMIN_USER.id })], + }); + } + return Promise.reject(new Error('refresh failed')); + }); + console.error = jest.fn(); + + await loadHistory(); + const btn = document.querySelector('.history-cancel-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + // Success toast lands BEFORE the reload error. + const successCalls = (showToast as jest.Mock).mock.calls.filter((c) => c[0].kind === 'success'); + const errorCalls = (showToast as jest.Mock).mock.calls.filter((c) => c[0].kind === 'error'); + expect(successCalls.length).toBeGreaterThan(0); + expect(errorCalls).toHaveLength(0); // the reload-failure path must NOT toast error + }); + test('cancel API failure surfaces toast and re-enables the button', async () => { (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); (confirmDialog as jest.Mock).mockResolvedValue(true); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index b0ec163a..ffaa69ef 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -290,11 +290,24 @@ function providerCell(p: HistoryPurchase): string { // 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. +// Cancel button (issue #46). UX gate only — the backend +// authorizeSessionCancel in internal/api/handler_purchases.go remains the +// security boundary; if this helper is wrong-positive the API surfaces +// 403 and the click handler turns that into a "Failed to cancel" toast. +// +// Heuristic: +// * admin → always yes; +// * non-admin matching the row's created_by_user_id → yes (cancel-own); +// * anyone else → no. +// +// Caveat: a non-admin role explicitly granted cancel-any:purchases (no +// such role exists by default; the verb is reserved for future operator +// roles) WILL be allowed by the backend but hidden by this helper. We +// don't surface that case because the frontend doesn't currently fetch +// the user's permission list, and adding a /me/permissions round-trip +// just to enable a button for a role nobody has is wasteful. If/when an +// operator role lands, extend User to carry permissions and broaden this +// check accordingly. function canCancelPendingRow(p: HistoryPurchase): boolean { const status = (p.status || '').toLowerCase(); if (status !== 'pending' && status !== 'notified') return false; @@ -411,8 +424,16 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { // Wire the inline Cancel button on pending/notified rows the current // session may cancel (issue #46). confirmDialog → POST → reload. The - // backend remains the security boundary; this UX gate just hides the - // button when the call would 403. + // backend remains the security boundary; the canCancelPendingRow + // helper above is a UX gate that hides the button when the call would + // 403, but a stale cache could still surface a 403 — handle it the + // same way as any other failure. + // + // The cancel POST and the follow-up reload are split into separate + // try/catch blocks: a successful cancel + failed reload must not show + // a "Failed to cancel" toast (the purchase IS cancelled), and the + // user should see success-toast first so they don't think their + // click was lost while we re-fetch the table. container.querySelectorAll('.history-cancel-btn[data-cancel-id]').forEach(btn => { btn.addEventListener('click', async () => { const id = btn.dataset['cancelId']; @@ -427,13 +448,24 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { btn.disabled = true; try { await api.cancelPurchase(id); - showToast({ message: 'Purchase cancelled', kind: 'success', timeout: 5_000 }); - await loadHistory(); } catch (cancelError) { console.error('Failed to cancel pending purchase:', cancelError); const err = cancelError as Error; showToast({ message: `Failed to cancel: ${err.message || 'unknown error'}`, kind: 'error' }); btn.disabled = false; + return; + } + // Cancel succeeded — surface success regardless of whether the + // refresh works. A reload failure leaves the row in its previous + // pending state on screen (stale-but-correct: the next manual + // reload corrects it). + showToast({ message: 'Purchase cancelled', kind: 'success', timeout: 5_000 }); + try { + await loadHistory(); + } catch (reloadError) { + console.error('Failed to reload history after cancel:', reloadError); + // Don't downgrade the success toast; loadHistory's own catch + // already paints an error message into the list area. } }); }); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 53d273cc..a5d14ecf 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -169,10 +169,19 @@ export interface HistoryPurchase { // 7-day-window reminder. Empty on completed / pending rows. status_description?: string; // CreatedByUserID: UUID of the user who created the underlying execution. - // Set only on pending/notified rows (synthesised from purchase_executions); - // empty on completed history rows. The History UI uses it to decide - // whether the inline Cancel button (issue #46) is visible to the current - // session — non-admins only see it on rows they themselves created. + // Populated on every synthesised purchase_executions row the History + // endpoint returns (pending, notified, failed, expired, cancelled — see + // historyExecutionStatuses in the backend). NOT populated on rows from + // the purchase_history table (completed purchases) since attribution + // there is via the older approver/cancelled_by fields. + // Empty when: + // * the row is a completed-purchase row from purchase_history, OR + // * the underlying execution pre-dates migration 000041 (legacy + // NULL creator), OR + // * the execution was scheduler-driven (no human creator). + // The History UI uses it to decide whether the inline Cancel button + // (issue #46) is visible — non-admins only see it on rows they + // themselves created. created_by_user_id?: string; } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 6f284e38..dbabc4ee 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -333,10 +333,18 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction // cancelPurchaseViaSession is the session-authed branch of cancelPurchase. // Enforces the cancel-any/cancel-own RBAC matrix, validates the execution -// is in a cancellable state (pending|notified), atomically transitions the -// row to "cancelled", and stamps the cancelling user's email onto -// CancelledBy. The History UI's annotateCancelled() helper renders that -// into "cancelled by " at read time — see handler_history.go. +// is in a cancellable state (pending|notified), atomically flips the row +// to "cancelled" AND drops its purchase_suppressions in the same +// transaction, and stamps session.Email onto CancelledBy. The History +// UI's annotateCancelled() helper renders CancelledBy as +// "cancelled by " at read time — see handler_history.go. +// +// The atomic suppression cleanup mirrors purchase.Manager.CancelExecution +// on the email-token path: an executePurchase upfront writes +// purchase_suppressions to hide the just-bought capacity from the +// recommendations list during the grace window, and cancel must drop +// those rows in the same commit so a crash between the two writes can't +// leave the rec list hiding capacity the user already cancelled. func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (any, error) { session, err := h.requireSession(ctx, req) if err != nil { @@ -351,21 +359,24 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return nil, err } - updated, err := h.config.TransitionExecutionStatus(ctx, execution.ExecutionID, []string{"pending", "notified"}, "cancelled") - if err != nil { - return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", execution.ExecutionID, err)) - } - - // Persist who cancelled the row so the History view can show - // "cancelled by " instead of falling back to the notification - // inbox. Best-effort — a write failure does not undo the status - // transition; the row is already cancelled. + // Flip status + clear suppressions + stamp CancelledBy in one tx. + // An optimistic-locking guard inside the tx (status IN + // ('pending','notified')) prevents a concurrent approval from + // landing on top of us — if the status drifted, the UPDATE 0-rows + // the row count and we 409 cleanly without rolling back the entire + // flow into an inconsistent state. + execution.Status = "cancelled" if session.Email != "" { actor := session.Email - updated.CancelledBy = &actor - if err := h.config.SavePurchaseExecution(ctx, updated); err != nil { - logging.Warnf("cancelPurchaseViaSession: failed to persist cancelled_by for %s: %v", execution.ExecutionID, err) + execution.CancelledBy = &actor + } + if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { + if err := h.config.SavePurchaseExecutionTx(ctx, tx, execution); err != nil { + return err } + return h.config.DeleteSuppressionsByExecutionTx(ctx, tx, execution.ExecutionID) + }); err != nil { + return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", execution.ExecutionID, err)) } return map[string]string{"status": "cancelled"}, nil diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index cc81290d..11670a01 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1154,6 +1154,29 @@ func sessionCancelReq() *events.LambdaFunctionURLRequest { } } +// runSessionCancelAllowed asserts the success path of the session-authed +// branch given a permission-matrix cell that should be allowed. The +// cancel commits in a single tx (SavePurchaseExecutionTx + +// DeleteSuppressionsByExecutionTx via WithTx); the mock store's WithTx +// default forwards fn(nil) and SavePurchaseExecutionTx default routes +// through SavePurchaseExecution, which we wire here. The suppression +// delete returns nil by default so we don't need to register it. +func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, session *Session, hasAny, hasOwn bool) { + t.Helper() + handler, mockConfig, _ := buildSessionCancelHandler(exec, session, hasAny, hasOwn) + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.NoError(t, err) + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + // Status flip + suppression cleanup are paired in one tx — the mock + // only sees the un-tx variants because of how MockConfigStore wires + // SavePurchaseExecutionTx → SavePurchaseExecution. Asserting the + // un-tx call ran is enough for the matrix tests; the atomicity + // itself is exercised by the live integration tests. + mockConfig.AssertCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + func TestHandler_cancelPurchase_Session_Admin_AllowsAny(t *testing.T) { creator := cancelOtherID exec := &config.PurchaseExecution{ @@ -1161,17 +1184,8 @@ func TestHandler_cancelPurchase_Session_Admin_AllowsAny(t *testing.T) { Status: "pending", CreatedByUserID: &creator, } - transitioned := *exec - transitioned.Status = "cancelled" session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} - - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, false) - mockConfig.On("TransitionExecutionStatus", mock.Anything, cancelExecID, []string{"pending", "notified"}, "cancelled").Return(&transitioned, nil) - mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) - - result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") - require.NoError(t, err) - assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + runSessionCancelAllowed(t, exec, session, false, false) } func TestHandler_cancelPurchase_Session_CancelAny_AllowsAny(t *testing.T) { @@ -1183,17 +1197,8 @@ func TestHandler_cancelPurchase_Session_CancelAny_AllowsAny(t *testing.T) { Status: "pending", CreatedByUserID: &creator, } - transitioned := *exec - transitioned.Status = "cancelled" session := &Session{UserID: cancelCallerID, Role: "user", Email: "ops@example.com"} - - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, true, false) - mockConfig.On("TransitionExecutionStatus", mock.Anything, cancelExecID, []string{"pending", "notified"}, "cancelled").Return(&transitioned, nil) - mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) - - result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") - require.NoError(t, err) - assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + runSessionCancelAllowed(t, exec, session, true, false) } func TestHandler_cancelPurchase_Session_CancelOwn_AllowsCreator(t *testing.T) { @@ -1203,17 +1208,8 @@ func TestHandler_cancelPurchase_Session_CancelOwn_AllowsCreator(t *testing.T) { Status: "notified", CreatedByUserID: &creator, } - transitioned := *exec - transitioned.Status = "cancelled" session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} - - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, true) - mockConfig.On("TransitionExecutionStatus", mock.Anything, cancelExecID, []string{"pending", "notified"}, "cancelled").Return(&transitioned, nil) - mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) - - result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") - require.NoError(t, err) - assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + runSessionCancelAllowed(t, exec, session, false, true) } func TestHandler_cancelPurchase_Session_CancelOwn_RejectsNonCreator(t *testing.T) { @@ -1230,7 +1226,8 @@ func TestHandler_cancelPurchase_Session_CancelOwn_RejectsNonCreator(t *testing.T _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) assert.Contains(t, err.Error(), "another user's pending purchase") - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") + mockConfig.AssertNotCalled(t, "WithTx") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") } func TestHandler_cancelPurchase_Session_NoVerb_Rejects(t *testing.T) { @@ -1247,7 +1244,8 @@ func TestHandler_cancelPurchase_Session_NoVerb_Rejects(t *testing.T) { _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) assert.Contains(t, err.Error(), "cancel-any or cancel-own") - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") + mockConfig.AssertNotCalled(t, "WithTx") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") } func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { @@ -1265,7 +1263,8 @@ func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "cannot be cancelled") assert.Contains(t, err.Error(), "completed") - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") + mockConfig.AssertNotCalled(t, "WithTx") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") } func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *testing.T) { @@ -1285,7 +1284,8 @@ func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *te _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) assert.Contains(t, err.Error(), "another user's pending purchase") - mockConfig.AssertNotCalled(t, "TransitionExecutionStatus") + mockConfig.AssertNotCalled(t, "WithTx") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") } func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) { diff --git a/internal/auth/types.go b/internal/auth/types.go index 02d79507..2121d872 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -277,16 +277,22 @@ const ( ActionAdmin = "admin" // ActionCancelOwn / ActionCancelAny gate the session-authed Cancel // button on pending Purchase History rows (issue #46). - // * cancel-own + purchases — granted to every authenticated user - // role by default; allows cancelling pending executions whose - // created_by_user_id matches the session user. Legacy rows with - // a NULL creator are out of reach for non-admins via this verb; - // admins still cancel them via cancel-any (or implicit admin). - // * cancel-any + purchases — granted via ActionAdmin/ResourceAll - // for admin role; this constant exists so future non-admin - // operator roles can be granted broad cancel rights without - // escalating to admin. Allows cancelling pending executions - // regardless of creator. + // + // Default grants: + // * RoleAdmin — implicit via {ActionAdmin, ResourceAll}; covers + // both verbs. + // * RoleUser — DefaultUserPermissions() adds cancel-own:purchases. + // Allows cancelling pending executions whose created_by_user_id + // matches the session user. Legacy rows with NULL creator are + // out of reach for non-admins via this verb; admins still cancel + // them via cancel-any. + // * RoleReadOnly — neither verb. Read-only users cannot cancel. + // + // cancel-any has no default non-admin grant; the constant exists so + // future operator roles can be granted broad cancel rights without + // escalating to admin. Add it to a custom group's Permissions to + // enable that path. + // // The legacy email-token cancel path stays unchanged as an escape // hatch and is gated by token possession, not these verbs. ActionCancelOwn = "cancel-own" From 71d1cf72ee75fe76decc9759ec5603ba1f216573 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Apr 2026 23:43:48 +0200 Subject: [PATCH 3/4] test(history-cancel): assert CancelledBy audit stamp in matrix tests 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. --- internal/api/handler_purchases_test.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 11670a01..537d0d02 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1161,10 +1161,22 @@ func sessionCancelReq() *events.LambdaFunctionURLRequest { // default forwards fn(nil) and SavePurchaseExecutionTx default routes // through SavePurchaseExecution, which we wire here. The suppression // delete returns nil by default so we don't need to register it. +// +// Captures the saved execution so the caller can assert the audit-stamp +// invariants — primarily that CancelledBy is set to session.Email when +// the session has a non-empty email. cancelPurchase relies on this stamp +// for History UI attribution; if SavePurchaseExecution stops being +// called with the email-bearing copy the matrix tests would otherwise +// silently regress. func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, session *Session, hasAny, hasOwn bool) { t.Helper() handler, mockConfig, _ := buildSessionCancelHandler(exec, session, hasAny, hasOwn) - mockConfig.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + 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) @@ -1174,7 +1186,13 @@ func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, sessi // SavePurchaseExecutionTx → SavePurchaseExecution. Asserting the // un-tx call ran is enough for the matrix tests; the atomicity // itself is exercised by the live integration tests. - mockConfig.AssertCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + 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") + } } func TestHandler_cancelPurchase_Session_Admin_AllowsAny(t *testing.T) { From f0666bd33a9ba48f813455861401bf8edb737e0e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Apr 2026 23:51:59 +0200 Subject: [PATCH 4/4] test(history-cancel): assert mockAuth.AssertExpectations on every session-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. --- internal/api/handler_purchases_test.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 537d0d02..daa8135d 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1170,7 +1170,7 @@ func sessionCancelReq() *events.LambdaFunctionURLRequest { // silently regress. 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) { @@ -1193,6 +1193,11 @@ func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, sessi 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") } + // Verify the session-auth boundary actually fired — without this a + // regression that bypassed ValidateSession (or stopped consulting + // HasPermissionAPI for non-admins) would silently still pass the + // status/audit assertions above. + mockAuth.AssertExpectations(t) } func TestHandler_cancelPurchase_Session_Admin_AllowsAny(t *testing.T) { @@ -1239,13 +1244,14 @@ func TestHandler_cancelPurchase_Session_CancelOwn_RejectsNonCreator(t *testing.T } session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, true) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, true) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) assert.Contains(t, err.Error(), "another user's pending purchase") mockConfig.AssertNotCalled(t, "WithTx") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) } func TestHandler_cancelPurchase_Session_NoVerb_Rejects(t *testing.T) { @@ -1257,13 +1263,14 @@ func TestHandler_cancelPurchase_Session_NoVerb_Rejects(t *testing.T) { } session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, false) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) assert.Contains(t, err.Error(), "cancel-any or cancel-own") mockConfig.AssertNotCalled(t, "WithTx") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) } func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { @@ -1275,7 +1282,7 @@ func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { } session := &Session{UserID: cancelCallerID, Role: "admin"} - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, false) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) @@ -1283,6 +1290,7 @@ func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { assert.Contains(t, err.Error(), "completed") mockConfig.AssertNotCalled(t, "WithTx") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) } func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *testing.T) { @@ -1297,13 +1305,14 @@ func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *te } session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} - handler, mockConfig, _ := buildSessionCancelHandler(exec, session, false, true) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, true) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) assert.Contains(t, err.Error(), "another user's pending purchase") mockConfig.AssertNotCalled(t, "WithTx") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) } func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) {