From c46539f9a6c3672e4bd3b8c55ceb5ea1eba75f78 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 15:38:35 +0200 Subject: [PATCH 1/5] fix(purchases): gate scheduled-purchase management on creator-scope ownership (closes #950) A standard user holding the default update:purchases verb could pause, resume, run, or delete scheduled purchases created by OTHER users: the planned-purchase management handlers checked only the verb and account scope, never per-record ownership. This is an integrity / separation-of- duties hole (QA row 552, P1 / severity-high). Backend: - Add auth.ActionUpdateAny ("update-any:purchases"), the privileged escape that lets a holder manage any user's scheduled purchase. Not in adminCarvedOuts, so admins pass via {admin, *}. - Add Handler.authorizeExecutionManagement enforcing the creator-scope model (mirrors authorizeSessionCancel / approve / retry): allow when the caller is the stateless admin key, holds update-any, or is the row's creator (non-nil CreatedByUserID == non-empty session.UserID). Otherwise 403; nil auth is 500 (fail closed). Legacy NULL-creator rows are out of reach for non-update-any users. - Wire the gate into pause / resume / run / delete after the existing verb and account-scope checks (run/delete keep their stricter execute/delete verb gates). - Surface created_by_user_id on the PlannedPurchase DTO so the client can gate ownership. Frontend: - Add created_by_user_id to the PlannedPurchase type and the 'update-any' Action. - Add canManageScheduledPurchase() and AND it into the Scheduled Purchases row-button gates: a non-creator without update-any sees no action buttons, matching the backend. Tests (fail pre-fix, pass post-fix): - Backend: user A pausing/resuming/deleting/running user B's execution -> 403 with no status transition; owner manages own -> success; update-any holder manages any -> success; nil-auth 500; legacy NULL-creator denied. Updated the #660 permission-flip tests to assert the new ownership model. - Frontend: non-creator with the same verbs renders no row buttons; creator and update-any holder do; legacy NULL-creator row shows none. --- .../src/__tests__/plans-ownership-950.test.ts | 170 ++++++++++++++++++ frontend/src/api/types.ts | 5 + frontend/src/permissions.ts | 4 + frontend/src/plans.ts | 31 +++- internal/api/handler_purchases.go | 68 +++++++ internal/api/handler_purchases_test.go | 140 +++++++++++++++ .../api/router_660_permission_flips_test.go | 72 +++++++- internal/api/types.go | 5 + internal/auth/types.go | 22 +++ 9 files changed, 510 insertions(+), 7 deletions(-) create mode 100644 frontend/src/__tests__/plans-ownership-950.test.ts diff --git a/frontend/src/__tests__/plans-ownership-950.test.ts b/frontend/src/__tests__/plans-ownership-950.test.ts new file mode 100644 index 000000000..de30fd01f --- /dev/null +++ b/frontend/src/__tests__/plans-ownership-950.test.ts @@ -0,0 +1,170 @@ +/** + * Issue #950: creator-scope ownership gating on Scheduled (Planned) Purchase + * row action buttons. + * + * The pre-fix behaviour gated the Run / Pause / Resume / Edit / Disable + * buttons purely on the plan-management verbs (update:plans / delete:plans), + * so a Standard user with those verbs saw actionable buttons on scheduled + * purchases created by OTHER users. The fix ANDs in + * canManageScheduledPurchase(): a non-creator without update-any:purchases + * sees NO action buttons. + * + * These tests drive the real loadPlans() render path. They set + * effectivePermissions on the mock user so the #365 verb gate passes, + * isolating the new ownership gate as the deciding factor. + */ +import { loadPlans } from '../plans'; + +jest.mock('../api', () => ({ + getPlans: jest.fn(), + getPlannedPurchases: jest.fn(), + listPlanAccounts: jest.fn().mockResolvedValue([]), +})); + +jest.mock('../state', () => ({ + getRecommendations: jest.fn().mockReturnValue([]), + getSelectedRecommendationIDs: jest.fn().mockReturnValue(new Set()), + getVisibleRecommendations: jest.fn().mockReturnValue([]), + setVisibleRecommendations: jest.fn(), + getCurrentProvider: jest.fn().mockReturnValue(''), + setCurrentProvider: jest.fn(), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + setCurrentAccountIDs: jest.fn(), + subscribeProvider: jest.fn().mockReturnValue(() => {}), + subscribeAccount: jest.fn().mockReturnValue(() => {}), + getCurrentUser: jest.fn(), + getPlansColumnFilters: jest.fn().mockReturnValue({}), + setPlansColumnFilter: jest.fn(), + clearAllPlansColumnFilters: jest.fn(), +})); + +jest.mock('../history', () => ({ viewPlanHistory: jest.fn() })); + +import * as api from '../api'; +import * as state from '../state'; + +const CREATOR_ID = 'creator-aaaa'; +const OTHER_ID = 'other-bbbb'; + +const samplePlan = { + id: 'plan-1', + name: 'Sample Plan', + enabled: true, + auto_purchase: true, + services: { + ec2: { provider: 'aws', service: 'ec2', enabled: true, term: 1, payment: 'all-upfront', coverage: 80 }, + }, + ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 1, total_steps: 4 }, +}; + +// A scheduled purchase created by CREATOR_ID. +const ownedPurchase = { + id: 'pp-1', + plan_id: 'plan-1', + plan_name: 'Sample Plan', + scheduled_date: '2026-06-01T00:00:00Z', + provider: 'aws', + service: 'ec2', + resource_type: 't3.medium', + region: 'us-east-1', + count: 5, + term: 1, + payment: 'all-upfront', + upfront_cost: 100, + estimated_savings: 50, + step_number: 1, + total_steps: 4, + status: 'pending', + created_by_user_id: CREATOR_ID, +}; + +// Same row but created by someone else. +const othersPurchase = { ...ownedPurchase, created_by_user_id: OTHER_ID }; + +// Legacy row with no creator (pre-migration NULL). +const legacyPurchase = { ...ownedPurchase, created_by_user_id: undefined as string | undefined }; + +// A user holding the plan-management verbs + update:purchases (so the #365 +// verb gate passes), optionally update-any:purchases. id identifies the +// session user for the ownership comparison. +const setUser = (id: string, opts: { updateAny?: boolean } = {}) => { + const effectivePermissions = [ + { action: 'update', resource: 'plans' }, + { action: 'delete', resource: 'plans' }, + { action: 'update', resource: 'purchases' }, + ]; + if (opts.updateAny) { + effectivePermissions.push({ action: 'update-any', resource: 'purchases' }); + } + (state.getCurrentUser as jest.Mock).mockReturnValue({ + id, + email: `${id}@example.com`, + groups: [], + effectivePermissions, + }); +}; + +const ppHtml = (): string => (document.getElementById('planned-purchases-list') as HTMLElement).innerHTML; + +const setupDom = () => { + const btn = document.createElement('button'); + btn.id = 'new-plan-btn'; + const list = document.createElement('div'); + list.id = 'plans-list'; + const planned = document.createElement('div'); + planned.id = 'planned-purchases-list'; + document.body.replaceChildren(btn, list, planned); +}; + +const ACTIONS = ['run', 'pause', 'resume', 'edit', 'disable']; + +describe('Scheduled-purchase ownership gating (issue #950)', () => { + beforeEach(() => { + jest.clearAllMocks(); + setupDom(); + (api.getPlans as jest.Mock).mockResolvedValue({ plans: [samplePlan] }); + }); + + test("creator sees action buttons on their own scheduled purchase", async () => { + setUser(CREATOR_ID); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadPlans(); + const html = ppHtml(); + // run/pause are status-dependent (pending -> run+pause shown). + expect(html).toContain('data-action="run"'); + expect(html).toContain('data-action="pause"'); + expect(html).toContain('data-action="edit"'); + expect(html).toContain('data-action="disable"'); + }); + + test("non-creator with the same verbs sees NO action buttons (the bug)", async () => { + // The deciding factor is ownership: this user holds update:plans / + // delete:plans / update:purchases but did NOT create the row. + setUser(OTHER_ID); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadPlans(); + const html = ppHtml(); + ACTIONS.forEach((act) => expect(html).not.toContain(`data-action="${act}"`)); + // The row itself still renders (status badge visible), just no buttons. + expect(html).toContain('Sample Plan'); + }); + + test("update-any holder sees buttons on another user's scheduled purchase", async () => { + setUser(OTHER_ID, { updateAny: true }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [othersPurchase] }); + await loadPlans(); + const html = ppHtml(); + expect(html).toContain('data-action="run"'); + expect(html).toContain('data-action="pause"'); + expect(html).toContain('data-action="edit"'); + expect(html).toContain('data-action="disable"'); + }); + + test("legacy NULL-creator row shows no buttons for a non-update-any user", async () => { + setUser(CREATOR_ID); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [legacyPurchase] }); + await loadPlans(); + const html = ppHtml(); + ACTIONS.forEach((act) => expect(html).not.toContain(`data-action="${act}"`)); + }); +}); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 77ebab5cf..fe32e9224 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -370,6 +370,11 @@ export interface PlannedPurchase { status: 'pending' | 'paused' | 'running' | 'completed' | 'failed'; step_number: number; total_steps: number; + // created_by_user_id is the UUID of the user who scheduled the purchase. + // The row action buttons are gated on creator-scope ownership (issue #950): + // a non-creator without update-any:purchases sees no actionable buttons. + // Omitted (undefined) for legacy rows with a NULL creator. + created_by_user_id?: string; } // User Management Types diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index fecf06d46..b3473f4b7 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -49,6 +49,10 @@ export type Action = | 'approve-any' | 'execute-own' | 'execute-any' + // update-any:purchases lets a holder manage (pause/resume/run/delete) + // ANY user's scheduled purchase, bypassing the creator-scope ownership + // check (issue #950). Mirrors cancel-any/approve-any on History rows. + | 'update-any' | 'admin'; // Resource names. Closed enum for the same reason. diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 8834ad6cb..e4e5315c7 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -652,6 +652,26 @@ function rerenderPlannedPurchases(): void { renderPlannedPurchasesInternal(); } +// canManageScheduledPurchase returns true when the current session is +// permitted to act on the given scheduled purchase's row buttons (issue #950). +// UX gate only -- the backend authorizeExecutionManagement in +// internal/api/handler_purchases.go remains the security boundary; a +// false-positive here surfaces as a 403 toast on click rather than a +// successful mutation. +// +// Heuristic (mirrors the creator-scope model on History rows): +// * admin (admin:* wildcard) or update-any:purchases -> manage anyone's row; +// * otherwise the row's created_by_user_id must match the current user; +// * legacy rows with a NULL created_by_user_id -> no buttons for non- +// privileged users (out of reach without update-any). +function canManageScheduledPurchase(purchase: PlannedPurchase): boolean { + if (canAccess('admin', '*') || canAccess('update-any', 'purchases')) return true; + const user = state.getCurrentUser(); + if (!user) return false; + if (!purchase.created_by_user_id) return false; + return purchase.created_by_user_id === user.id; +} + /** * Render a single planned purchase row */ @@ -674,8 +694,15 @@ function renderPlannedPurchaseRow(purchase: PlannedPurchase): string { // a click on each button would require. Readonly users see no buttons // (status badge only); user role sees Run/Pause/Resume/Edit but not // Disable; admins see everything. - const canManagePlan = canAccess('update', 'plans'); - const canDisablePlan = canAccess('delete', 'plans'); + // + // Issue #950: AND in creator-scope ownership. A non-creator who lacks + // update-any:purchases (a standard user looking at someone else's row) + // sees NO action buttons, mirroring the backend ownership gate. This is + // a UX gate; the backend authorizeExecutionManagement is the real + // boundary. + const isOwner = canManageScheduledPurchase(purchase); + const canManagePlan = canAccess('update', 'plans') && isOwner; + const canDisablePlan = canAccess('delete', 'plans') && isOwner; return ` diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 6df17dea0..ffffa4ac6 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -188,9 +188,65 @@ func buildPlannedPurchase(plan *config.PurchasePlan, exec *config.PurchaseExecut Status: exec.Status, StepNumber: exec.StepNumber, TotalSteps: plan.RampSchedule.TotalSteps, + CreatedByUserID: exec.CreatedByUserID, } } +// authorizeExecutionManagement enforces creator-scope ownership on the +// scheduled-purchase management handlers (pause / resume / run / delete), +// closing the authz hole in issue #950 where any holder of update:purchases +// could act on another user's scheduled purchase. It runs AFTER the +// per-handler verb check (update / execute / delete) and the account-scope +// check (requireExecutionAccess); those still apply unchanged. +// +// Gate logic (mirrors authorizeSessionCancel / authorizeSessionApprove): +// - stateless admin API key: always permitted (apiKeyAdminUserID sentinel). +// - update-any:purchases: permitted regardless of creator. Administrators- +// group users pass here because {admin, *} matches ActionUpdateAny +// (update-any is not in adminCarvedOuts). +// - otherwise: permitted only when the execution's CreatedByUserID is +// non-nil and equals a non-empty session.UserID (the caller created it). +// Legacy rows with a NULL creator are out of reach for non-update-any +// users, matching the cancel-own / approve-own / retry-own model. +// - any other case: 403 fail-closed. A nil auth component is a 500 per +// feedback_fail_closed_middleware.md. +// +// Only fetches the execution on the creator-match path: admin and update-any +// callers are authorised without a store round-trip (and admin sessions have +// unrestricted access, so requireExecutionAccess skipped the fetch too). +func (h *Handler) authorizeExecutionManagement(ctx context.Context, session *Session, executionID string) error { + if session.UserID == apiKeyAdminUserID { + return nil + } + if h.auth == nil { + return NewClientError(500, "authentication service not configured") + } + + hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionUpdateAny, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if hasAny { + return nil + } + + execution, err := h.config.GetExecutionByID(ctx, executionID) + if err != nil { + return fmt.Errorf("failed to get execution: %w", err) + } + if execution == nil { + return errNotFound + } + + // Creator match: both IDs must be non-empty and equal. An empty-string + // collision (legacy NULL creator + missing session UserID) must not + // grant access. + if session.UserID == "" || execution.CreatedByUserID == nil || *execution.CreatedByUserID != session.UserID { + return NewClientError(403, "permission denied: cannot manage another user's scheduled purchase") + } + return nil +} + func (h *Handler) pausePlannedPurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, executionID string) (*StatusResponse, error) { if err := validateUUID(executionID); err != nil { return nil, err @@ -203,6 +259,9 @@ func (h *Handler) pausePlannedPurchase(ctx context.Context, req *events.LambdaFu if err := h.requireExecutionAccess(ctx, session, executionID); err != nil { return nil, err } + if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil { + return nil, err + } // Atomically transition to paused if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "running"}, "paused"); err != nil { @@ -224,6 +283,9 @@ func (h *Handler) resumePlannedPurchase(ctx context.Context, req *events.LambdaF if err := h.requireExecutionAccess(ctx, session, executionID); err != nil { return nil, err } + if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil { + return nil, err + } // Atomically transition from paused back to pending if _, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"paused"}, "pending"); err != nil { @@ -245,6 +307,9 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc if err := h.requireExecutionAccess(ctx, session, executionID); err != nil { return nil, err } + if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil { + return nil, err + } // Atomically transition to running — only one concurrent caller can succeed. // TransitionExecutionStatus handles not-found and wrong-status cases. @@ -271,6 +336,9 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF if err := h.requireExecutionAccess(ctx, session, executionID); err != nil { return nil, err } + if err := h.authorizeExecutionManagement(ctx, session, executionID); err != nil { + return nil, err + } // Cancel the scheduled execution. The RETURNING clause gives us the // parent plan_id so we can disable the plan in the same handler call. diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index bebd33576..39c3b652d 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3317,3 +3317,143 @@ func TestHandler_authorizeSessionExecuteDirect_NilAuth(t *testing.T) { require.True(t, ok, "expected a clientError") assert.Equal(t, 500, ce.code) } + +// --- Issue #950: creator-scope ownership gate on scheduled-purchase mgmt --- +// +// These tests replicate the QA scenario in issue #950: a Standard user with +// update:purchases (and account access) must NOT be able to pause/resume/ +// cancel a scheduled purchase created by ANOTHER user. They drive the real +// handlers end-to-end (ValidateSession -> requirePermission -> account scope +// -> authorizeExecutionManagement -> transition) and FAIL against the pre-fix +// handler, which honoured the request because only update:purchases was +// checked. + +const ownExecID = "12121212-1212-1212-1212-121212121212" +const ownUserA = "aaaa1111-1111-1111-1111-111111111111" // creator of P1 +const ownUserB = "bbbb2222-2222-2222-2222-222222222222" // creator of P2 + +// buildManageHandler wires a non-admin "user-token" session for userID with +// account access (empty allowed_accounts -> all accessible) and the given +// update-any grant. The stored execution is created by creatorID. +func buildManageHandler(userID, creatorID string, hasUpdateAny bool) (*Handler, *MockConfigStore, *MockAuthService) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "user-token").Return(&Session{UserID: userID}, nil) + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "update", "purchases").Return(true, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "execute", "purchases").Return(true, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "delete", "purchases").Return(true, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, userID, "update-any", "purchases").Return(hasUpdateAny, nil).Maybe() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, userID).Return([]string{}, nil).Maybe() + + creator := creatorID + exec := &config.PurchaseExecution{ExecutionID: ownExecID, Status: "pending", CreatedByUserID: &creator} + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, ownExecID).Return(exec, nil) + + return &Handler{config: mockConfig, auth: mockAuth}, mockConfig, mockAuth +} + +func manageReq() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer user-token"}, + } +} + +// TestHandler_pausePlannedPurchase_NonOwner_Rejected is the core #950 +// regression: user A pauses a scheduled purchase created by user B -> 403, +// and the status transition never runs. +func TestHandler_pausePlannedPurchase_NonOwner_Rejected(t *testing.T) { + handler, mockConfig, mockAuth := buildManageHandler(ownUserA, ownUserB, false) + + _, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.message, "another user's scheduled purchase") + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockAuth.AssertExpectations(t) +} + +func TestHandler_resumePlannedPurchase_NonOwner_Rejected(t *testing.T) { + handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, false) + + _, err := handler.resumePlannedPurchase(context.Background(), manageReq(), ownExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestHandler_deletePlannedPurchase_NonOwner_Rejected(t *testing.T) { + handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, false) + + _, err := handler.deletePlannedPurchase(context.Background(), manageReq(), ownExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestHandler_runPlannedPurchase_NonOwner_Rejected(t *testing.T) { + handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, false) + + _, err := handler.runPlannedPurchase(context.Background(), manageReq(), ownExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + mockConfig.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +// TestHandler_pausePlannedPurchase_Owner_Allowed: user A manages their OWN P1. +func TestHandler_pausePlannedPurchase_Owner_Allowed(t *testing.T) { + handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserA, false) + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused"). + Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil) + + res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID) + require.NoError(t, err) + assert.Equal(t, "paused", res.Status) + mockConfig.AssertCalled(t, "TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused") +} + +// TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny: a privileged user with +// update-any:purchases manages P2 created by user B. +func TestHandler_pausePlannedPurchase_UpdateAny_AllowsAny(t *testing.T) { + handler, mockConfig, _ := buildManageHandler(ownUserA, ownUserB, true) + mockConfig.On("TransitionExecutionStatus", mock.Anything, ownExecID, []string{"pending", "running"}, "paused"). + Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "paused"}, nil) + + res, err := handler.pausePlannedPurchase(context.Background(), manageReq(), ownExecID) + require.NoError(t, err) + assert.Equal(t, "paused", res.Status) +} + +// TestHandler_authorizeExecutionManagement_NilAuth: fail-closed 500. +func TestHandler_authorizeExecutionManagement_NilAuth(t *testing.T) { + handler := &Handler{auth: nil} + err := handler.authorizeExecutionManagement(context.Background(), &Session{UserID: ownUserA}, ownExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 500, ce.code) +} + +// TestHandler_authorizeExecutionManagement_LegacyNullCreator: a legacy row +// with a NULL creator is unreachable for a non-update-any user. +func TestHandler_authorizeExecutionManagement_LegacyNullCreator(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("HasPermissionAPI", mock.Anything, ownUserA, "update-any", "purchases").Return(false, nil) + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, ownExecID). + Return(&config.PurchaseExecution{ExecutionID: ownExecID, Status: "pending", CreatedByUserID: nil}, nil) + handler := &Handler{config: mockConfig, auth: mockAuth} + + err := handler.authorizeExecutionManagement(context.Background(), &Session{UserID: ownUserA}, ownExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) +} diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index b435fd985..50f30e1b9 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -194,13 +194,18 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { const userID = "33333333-3333-3333-3333-333333333333" const execID = "cccccccc-cccc-cccc-cccc-cccccccccccc" - t.Run("user with update:purchases can pause a planned purchase", func(t *testing.T) { + t.Run("creator with update:purchases can pause their own planned purchase", func(t *testing.T) { + // Issue #950: a standard user manages only the scheduled purchases + // they created. update-any is false; the creator match authorises. mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true) mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + creator := userID mockStore := new(MockConfigStore) - // requireExecutionAccess calls GetExecutionByID; stub a minimal row. + // requireExecutionAccess + authorizeExecutionManagement both call + // GetExecutionByID; stub a row created by this user. mockStore.On("GetExecutionByID", ctx, execID). - Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil) + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) // TransitionExecutionStatus is called next; stub it. mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused"). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) @@ -210,6 +215,44 @@ func TestPausePlannedPurchase_PermissionGate(t *testing.T) { assertNotForbidden(t, err) }) + t.Run("non-creator with update:purchases is rejected with 403 (issue #950)", func(t *testing.T) { + // The user holds update:purchases (and account access) but did NOT + // create the execution and lacks update-any -> 403. This is the + // regression guard for the pre-fix authz hole. + mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + otherCreator := "99999999-9999-9999-9999-999999999999" + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &otherCreator}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.pausePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assert403(t, err) + // The status transition must never run for a non-owner. + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("update-any holder can pause another user's planned purchase (issue #950)", func(t *testing.T) { + // An operator role with update-any:purchases bypasses the creator + // check, mirroring cancel-any/approve-any on History. + mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", true) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(true, nil) + mockStore := new(MockConfigStore) + // update-any short-circuits the ownership fetch in + // authorizeExecutionManagement; only requireExecutionAccess fetches. + mockStore.On("GetExecutionByID", ctx, execID). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "running"}, "paused"). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "paused"}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.pausePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assertNotForbidden(t, err) + }) + t.Run("user without update:purchases is rejected with 403", func(t *testing.T) { mockAuth := authForUserWith(ctx, t, userID, "update", "purchases", false) h := &Handler{auth: mockAuth, config: new(MockConfigStore)} @@ -239,12 +282,16 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { const userID = "44444444-4444-4444-4444-444444444444" const execID = "dddddddd-dddd-dddd-dddd-dddddddddddd" - t.Run("user with delete:purchases can delete a planned purchase", func(t *testing.T) { + t.Run("creator with delete:purchases can delete their own planned purchase", func(t *testing.T) { + // Issue #950: ownership gate also applies to delete; a creator with + // delete:purchases (no update-any) is authorised by the creator match. mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", true) mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + creator := userID mockStore := new(MockConfigStore) mockStore.On("GetExecutionByID", ctx, execID). - Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending"}, nil) + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &creator}, nil) mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled"). Return(&config.PurchaseExecution{ExecutionID: execID, Status: "cancelled"}, nil) @@ -253,6 +300,21 @@ func TestDeletePlannedPurchase_PermissionGate(t *testing.T) { assertNotForbidden(t, err) }) + t.Run("non-creator with delete:purchases is rejected with 403 (issue #950)", func(t *testing.T) { + mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", true) + mockAuth.On("GetAllowedAccountsAPI", ctx, userID).Return([]string{}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "update-any", "purchases").Return(false, nil) + otherCreator := "99999999-9999-9999-9999-999999999999" + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "pending", CreatedByUserID: &otherCreator}, nil) + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.deletePlannedPurchase(ctx, reqWithBearer("user-token"), execID) + assert403(t, err) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + t.Run("user without delete:purchases is rejected with 403", func(t *testing.T) { mockAuth := authForUserWith(ctx, t, userID, "delete", "purchases", false) h := &Handler{auth: mockAuth, config: new(MockConfigStore)} diff --git a/internal/api/types.go b/internal/api/types.go index 40b979d7e..302aa5fd0 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -649,6 +649,11 @@ type PlannedPurchase struct { Status string `json:"status"` StepNumber int `json:"step_number"` TotalSteps int `json:"total_steps"` + // CreatedByUserID is the UUID of the user who created the scheduled + // purchase, mirroring PurchaseHistoryRecord.CreatedByUserID. The + // frontend gates the row action buttons on creator-scope ownership + // (issue #950); omitted for legacy rows with a NULL creator. + CreatedByUserID *string `json:"created_by_user_id,omitempty"` } // PlanRequest represents the API request format for creating/updating plans diff --git a/internal/auth/types.go b/internal/auth/types.go index 4958182d7..398ddc93c 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -426,6 +426,28 @@ const ( // check. No default non-admin grant; add to a custom operator group. ActionExecuteOwn = "execute-own" ActionExecuteAny = "execute-any" + // ActionUpdateAny is the privileged escape that lets a holder manage + // (pause / resume / run / delete) a SCHEDULED purchase execution + // regardless of who created it (issue #950). It complements the base + // update:purchases verb every authenticated user already holds: that + // base verb authorises managing only your OWN scheduled purchases + // (created_by_user_id == session.UserID), while update-any drops the + // per-record ownership check. + // + // * RoleAdmin — implicit via {ActionAdmin, ResourceAll}; update-any is + // NOT in adminCarvedOuts, so admins manage every scheduled purchase. + // * RoleUser — NO default grant. A standard user manages only the + // scheduled purchases they created (base update:purchases + creator + // match). Legacy rows with NULL created_by_user_id are out of reach + // for non-admins (they hold neither update-any nor a creator match). + // * Custom operator groups — add update-any:purchases to let a role + // manage everyone's scheduled purchases without escalating to admin. + // + // There is no separate update-own verb: the existing update:purchases + // grant already plays that role, mirroring how cancel-own/approve-own + // gate History rows. The creator match is enforced in the handler + // (authorizeExecutionManagement), not in HasPermission. + ActionUpdateAny = "update-any" ) // Predefined resources From dd2cc1c47d72a848e6f5757abec13e03cc18844a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 07:52:37 +0200 Subject: [PATCH 2/5] refactor: extract cancelOrRecoverExecution helper to clear gocyclo (#995) The new authorize/disable wiring in deletePlannedPurchase nudged it from complexity 10 to 11, failing pre-commit's gocyclo > 10 ceiling. Split the cancel-or-recover idempotency block (TransitionExecutionStatus plus the post-conflict GetExecutionByID fallback) into a focused helper. The main function now reads top-to-bottom as validate -> authorize -> cancel-or-recover -> disable-plan, and the helper carries the four decision points that previously lived inline. Behaviour unchanged; 1475 tests pass in internal/api. --- internal/api/handler_purchases.go | 47 ++++++++++++++++--------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ffffa4ac6..76954e12a 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -340,29 +340,9 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF return nil, err } - // Cancel the scheduled execution. The RETURNING clause gives us the - // parent plan_id so we can disable the plan in the same handler call. - // - // Idempotency: if TransitionExecutionStatus returns - // ErrExecutionNotInExpectedStatus the row is already in a terminal state - // (most likely "cancelled" from a previous attempt). In that case we - // fetch the execution to recover the PlanID and still attempt to disable - // the plan, so a retry never leaves plan.enabled=true. - cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled") + cancelled, err := h.cancelOrRecoverExecution(ctx, executionID) if err != nil { - if !errors.Is(err, config.ErrExecutionNotInExpectedStatus) { - return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, err)) - } - // The cancel already landed (e.g. a prior request succeeded and was - // retried). Recover the execution so we can still disable the plan. - existing, getErr := h.config.GetExecutionByID(ctx, executionID) - if getErr != nil { - return nil, fmt.Errorf("disable plan: failed to get execution %s after conflict: %w", executionID, getErr) - } - if existing == nil { - return nil, NewClientError(404, fmt.Sprintf("execution %s not found", executionID)) - } - cancelled = existing + return nil, err } // Set the parent plan's enabled flag to false so the Plans page toggle @@ -378,6 +358,29 @@ func (h *Handler) deletePlannedPurchase(ctx context.Context, req *events.LambdaF return &StatusResponse{Status: "cancelled"}, nil } +// cancelOrRecoverExecution transitions the execution to "cancelled" if it is +// still in {pending, paused}. If a prior attempt already cancelled it +// (ErrExecutionNotInExpectedStatus), it fetches the row instead so the caller +// can still drive the plan-disable side-effect, keeping the operation +// idempotent across retries. +func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID string) (*config.PurchaseExecution, error) { + cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled") + if err == nil { + return cancelled, nil + } + if !errors.Is(err, config.ErrExecutionNotInExpectedStatus) { + return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, err)) + } + existing, getErr := h.config.GetExecutionByID(ctx, executionID) + if getErr != nil { + return nil, fmt.Errorf("disable plan: failed to get execution %s after conflict: %w", executionID, getErr) + } + if existing == nil { + return nil, NewClientError(404, fmt.Sprintf("execution %s not found", executionID)) + } + return existing, nil +} + // disablePlan fetches the plan identified by planID and sets Enabled=false if // it is currently true. It is idempotent: calling it against an already- // disabled plan is a no-op. Returns a 404 ClientError when the plan does not From 1f66f71d20cfcc93620e169ae7d2e7739c5d5502 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:14:24 +0200 Subject: [PATCH 3/5] fix(purchases): address CR pass-1 findings on PR #995 - cancelOrRecoverExecution: only treat fetched row as recovered when status is actually "cancelled"; return 409 with actual status otherwise, catching races to running/completed/failed. Regression test added (ConflictRetryRunningReturns409). - plans.ts row buttons: gate on the verbs the backend actually checks (execute/update/delete:purchases), keeping Edit on update:plans. Update test fixtures to include execute:purchases and delete:purchases verbs and PURCHASER_GROUP_ID where needed for the run/disable buttons. - plans-ownership-950.test.ts: use ownedPurchase (created by CREATOR_ID) in the update-any bypass test so OTHER_ID session is actually not the creator, properly exercising the bypass; remove now-unused othersPurchase. --- .../src/__tests__/plans-ownership-950.test.ts | 7 ++- .../src/__tests__/plans-permissions.test.ts | 4 +- frontend/src/__tests__/plans.test.ts | 6 ++- frontend/src/plans.ts | 16 +++--- internal/api/handler_purchases.go | 5 ++ internal/api/handler_purchases_test.go | 50 +++++++++++++++++++ 6 files changed, 74 insertions(+), 14 deletions(-) diff --git a/frontend/src/__tests__/plans-ownership-950.test.ts b/frontend/src/__tests__/plans-ownership-950.test.ts index de30fd01f..33a9195e8 100644 --- a/frontend/src/__tests__/plans-ownership-950.test.ts +++ b/frontend/src/__tests__/plans-ownership-950.test.ts @@ -78,9 +78,6 @@ const ownedPurchase = { created_by_user_id: CREATOR_ID, }; -// Same row but created by someone else. -const othersPurchase = { ...ownedPurchase, created_by_user_id: OTHER_ID }; - // Legacy row with no creator (pre-migration NULL). const legacyPurchase = { ...ownedPurchase, created_by_user_id: undefined as string | undefined }; @@ -91,7 +88,9 @@ const setUser = (id: string, opts: { updateAny?: boolean } = {}) => { const effectivePermissions = [ { action: 'update', resource: 'plans' }, { action: 'delete', resource: 'plans' }, + { action: 'execute', resource: 'purchases' }, { action: 'update', resource: 'purchases' }, + { action: 'delete', resource: 'purchases' }, ]; if (opts.updateAny) { effectivePermissions.push({ action: 'update-any', resource: 'purchases' }); @@ -151,7 +150,7 @@ describe('Scheduled-purchase ownership gating (issue #950)', () => { test("update-any holder sees buttons on another user's scheduled purchase", async () => { setUser(OTHER_ID, { updateAny: true }); - (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [othersPurchase] }); + (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); await loadPlans(); const html = ppHtml(); expect(html).toContain('data-action="run"'); diff --git a/frontend/src/__tests__/plans-permissions.test.ts b/frontend/src/__tests__/plans-permissions.test.ts index 89168d7c5..7e97929dd 100644 --- a/frontend/src/__tests__/plans-permissions.test.ts +++ b/frontend/src/__tests__/plans-permissions.test.ts @@ -44,7 +44,7 @@ jest.mock('../history', () => ({ viewPlanHistory: jest.fn() })); import * as api from '../api'; import * as state from '../state'; -import { ADMINISTRATORS_GROUP_ID } from '../permissions'; +import { ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID } from '../permissions'; const samplePlan = { id: 'plan-1', @@ -78,7 +78,7 @@ const samplePlannedPurchase = { const mockUser = (role: string | null) => { (state.getCurrentUser as jest.Mock).mockReturnValue( - role === null ? null : { id: 'u', email: 'u@example.com', groups: role === 'admin' ? [ADMINISTRATORS_GROUP_ID] : [] }, + role === null ? null : { id: 'u', email: 'u@example.com', groups: role === 'admin' ? [ADMINISTRATORS_GROUP_ID, PURCHASER_GROUP_ID] : [] }, ); }; diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index e297cc445..95d5ea285 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -57,7 +57,11 @@ jest.mock('../state', () => ({ // factories are hoisted before imports; jest.requireActual also fails here // because permissions.ts has a top-level import of ./state which is the very // module being mocked (circular init). permissions.test.ts pins the value. - getCurrentUser: jest.fn().mockReturnValue({ id: 'u-admin', email: 'admin@example.com', groups: ['00000000-0000-5000-8000-000000000001'] }), + // Include PURCHASER_GROUP_ID literal alongside ADMINISTRATORS_GROUP_ID so + // execute:purchases (carved out of admin:*) resolves true for the default + // admin session. Both are literals to avoid circular-init issues (see + // comment above). Matches PURCHASER_GROUP_ID from permissions.ts. + getCurrentUser: jest.fn().mockReturnValue({ id: 'u-admin', email: 'admin@example.com', groups: ['00000000-0000-5000-8000-000000000001', '00000000-0000-5000-8000-000000000007'] }), // Issue #166 follow-up: plans.ts now reads per-column filter state via // these accessors. Default to "no filters" so legacy assertions keep // passing; tests that exercise the popover override per-case. diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index e4e5315c7..f1632b5c3 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -700,9 +700,11 @@ function renderPlannedPurchaseRow(purchase: PlannedPurchase): string { // sees NO action buttons, mirroring the backend ownership gate. This is // a UX gate; the backend authorizeExecutionManagement is the real // boundary. - const isOwner = canManageScheduledPurchase(purchase); - const canManagePlan = canAccess('update', 'plans') && isOwner; - const canDisablePlan = canAccess('delete', 'plans') && isOwner; + const canManagePurchase = canManageScheduledPurchase(purchase); + const canRunPurchase = canManagePurchase && canAccess('execute', 'purchases') && canRun; + const canPauseOrResumePurchase = canManagePurchase && canAccess('update', 'purchases'); + const canEditPlan = canManagePurchase && canAccess('update', 'plans'); + const canDisablePlan = canManagePurchase && canAccess('delete', 'purchases'); return ` @@ -720,10 +722,10 @@ function renderPlannedPurchaseRow(purchase: PlannedPurchase): string { ${formatCurrency(purchase.estimated_savings)}/mo ${escapeHtml(purchase.status)} - ${canManagePlan && canRun ? `` : ''} - ${canManagePlan && isPending ? `` : ''} - ${canManagePlan && isPaused ? `` : ''} - ${canManagePlan ? `` : ''} + ${canRunPurchase ? `` : ''} + ${canPauseOrResumePurchase && isPending ? `` : ''} + ${canPauseOrResumePurchase && isPaused ? `` : ''} + ${canEditPlan ? `` : ''} ${canDisablePlan ? `` : ''} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 76954e12a..397d6db75 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -378,6 +378,11 @@ func (h *Handler) cancelOrRecoverExecution(ctx context.Context, executionID stri if existing == nil { return nil, NewClientError(404, fmt.Sprintf("execution %s not found", executionID)) } + if existing.Status != "cancelled" { + return nil, NewClientError(409, fmt.Sprintf( + "execution %s cannot be cancelled (status=%s)", + executionID, existing.Status)) + } return existing, nil } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 39c3b652d..44f9ec8ae 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1319,6 +1319,56 @@ func TestHandler_deletePlannedPurchase_ConflictRetryAlreadyDisabled(t *testing.T assert.Equal(t, "cancelled", result.Status) } +// TestHandler_deletePlannedPurchase_ConflictRetryRunningReturns409 is a +// regression test for CR #995 Finding 1: when TransitionExecutionStatus +// returns ErrExecutionNotInExpectedStatus but the fetched row is NOT +// "cancelled" (e.g. the execution raced to "running"), cancelOrRecoverExecution +// must return a 409 and must NOT call disablePlan (no GetPurchasePlan call). +func TestHandler_deletePlannedPurchase_ConflictRetryRunningReturns409(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + + execID := "abababab-abab-abab-abab-abababababab" + planID := "cdcdcdcd-cdcd-cdcd-cdcd-cdcdcdcdcdcd" + + conflictErr := fmt.Errorf("%w: execution %s cannot transition", config.ErrExecutionNotInExpectedStatus, execID) + + // The execution raced to "running" — not "cancelled". + runningExec := &config.PurchaseExecution{ + ExecutionID: execID, + PlanID: planID, + Status: "running", + } + + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + mockStore.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "paused"}, "cancelled").Return(nil, conflictErr) + mockStore.On("GetExecutionByID", ctx, execID).Return(runningExec, nil) + // GetPurchasePlan must NOT be called — AssertExpectations verifies this. + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + } + result, err := handler.deletePlannedPurchase(ctx, req, execID) + require.Error(t, err, "racing-to-running execution must fail") + assert.Nil(t, result) + + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 409, ce.code, "status mismatch must return 409") + assert.Contains(t, ce.message, "cannot be cancelled", "error must name the action") + assert.Contains(t, ce.message, "running", "error must include actual status") +} + func TestHandler_pausePlannedPurchase_NilExecution(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) From 9ef1b4352d38fdedf92133afa33fcaec39021b0e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:35:13 +0200 Subject: [PATCH 4/5] fix(purchases): stamp creator on scheduled purchases from plan endpoint (closes #950 followup) Adversarial review of the PR #995 ownership-gate fix uncovered that POST /api/plans/{id}/purchases (createPlannedPurchases) never wrote CreatedByUserID onto the rows it inserted. Every freshly scheduled purchase therefore shipped a NULL creator, which the new per-row ownership gate (authorizeExecutionManagement) treats as a legacy unattributed row -- reachable only by admin / update-any holders. Net effect pre-fix: a standard user clicks "Create planned purchases" on their own plan and immediately loses the ability to pause / resume / run / delete the rows they just scheduled. The user-facing 403 is indistinguishable from the original #950 bypass to a confused operator. The scheduler-tick path in purchase/notifications.go legitimately stays NULL-creator (no human session, by migration-000041 design); only the session-driven explicit-create endpoint stamps the actor here. Wire resolveCreatorUserID(session) through createPurchaseExecutionsTx so each batch row carries the session UUID. Admin-API-key sessions resolve to nil (the UserID sentinel is not a UUID), matching the executePurchase / retry paths. Regression tests: - TestHandler_createPlannedPurchases_StampsCreator: captures every saved row's CreatedByUserID and asserts each one is the session user's UUID. Fails pre-fix on the first nil pointer. - TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil: the admin-API-key path stays NULL so the FK to users remains valid; locks in the resolveCreatorUserID contract. --- internal/api/handler_plans.go | 35 ++++++--- internal/api/handler_plans_test.go | 111 +++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 8 deletions(-) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 15f7cdb58..eed686d06 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -289,9 +289,20 @@ func (h *Handler) createPlannedPurchases(ctx context.Context, httpReq *events.La // pointer). A retry would then duplicate rows 1-3. WithTx makes // both classes of corruption impossible — the caller can safely // retry on transient errors knowing nothing was committed. + // + // Issue #950: stamp the session user onto each new execution's + // created_by_user_id so the per-row creator-scope ownership gate + // (authorizeExecutionManagement) recognises the actor who scheduled + // the purchases as their owner. Without this the rows ship NULL and + // are unreachable for pause / resume / run / delete by anyone except + // admins / update-any holders, including the user who just clicked + // "Create planned purchases" for their own plan. Admin-API-key and + // non-UUID sessions resolve to nil, matching the executePurchase / + // retry paths and the migration-000041 fail-closed policy. + creator := resolveCreatorUserID(session) created := 0 if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { - n, txErr := h.createPurchaseExecutionsTx(ctx, tx, plan, planID, req.Count, startDate) + n, txErr := h.createPurchaseExecutionsTx(ctx, tx, plan, planID, req.Count, startDate, creator) if txErr != nil { return txErr } @@ -344,7 +355,14 @@ func (h *Handler) getPlanForPurchaseCreation(ctx context.Context, planID string) // Returns the number of rows that would have been committed had the // loop completed — used for the user-visible response on success; // undefined (and unused) on error since the rollback voids them all. -func (h *Handler) createPurchaseExecutionsTx(ctx context.Context, tx pgx.Tx, plan *config.PurchasePlan, planID string, count int, startDate time.Time) (int, error) { +// +// creator carries the session user's UUID (or nil for the admin-API-key / +// non-UUID-session paths) and is stamped onto every inserted row's +// created_by_user_id so the issue-#950 ownership gate downstream can +// recognise the actor as the rightful manager. A nil value mirrors the +// migration-000041 fail-closed semantics: legacy / unattributed rows are +// reachable only by admin / update-any holders. +func (h *Handler) createPurchaseExecutionsTx(ctx context.Context, tx pgx.Tx, plan *config.PurchasePlan, planID string, count int, startDate time.Time, creator *string) (int, error) { intervalDays := plan.RampSchedule.StepIntervalDays if intervalDays == 0 { intervalDays = 7 // Default to weekly if not set @@ -359,12 +377,13 @@ func (h *Handler) createPurchaseExecutionsTx(ctx context.Context, tx pgx.Tx, pla return created, fmt.Errorf("failed to generate approval token (row %d/%d): %w", created+1, count, err) } execution := &config.PurchaseExecution{ - PlanID: planID, - ExecutionID: uuid.New().String(), - Status: "pending", - StepNumber: plan.RampSchedule.CurrentStep + i + 1, - ScheduledDate: scheduledDate, - ApprovalToken: approvalToken, + PlanID: planID, + ExecutionID: uuid.New().String(), + Status: "pending", + StepNumber: plan.RampSchedule.CurrentStep + i + 1, + ScheduledDate: scheduledDate, + ApprovalToken: approvalToken, + CreatedByUserID: creator, } if err := h.config.SavePurchaseExecutionTx(ctx, tx, execution); err != nil { diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index 40bdf2387..3f7e3e29b 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -445,6 +445,117 @@ func TestHandler_createPlannedPurchases(t *testing.T) { assert.Equal(t, 3, result.Created) } +// TestHandler_createPlannedPurchases_StampsCreator is the issue-#950 regression +// guard: every execution row written through POST /api/plans/{id}/purchases +// MUST carry the session user's UUID in CreatedByUserID, otherwise the +// per-row ownership gate (authorizeExecutionManagement in +// handler_purchases.go) downstream cannot recognise the actor as the +// rightful manager and the user who just scheduled the purchases is +// locked out of pause / resume / run / delete until an admin steps in. +// +// Pre-fix the field shipped zero-valued (nil pointer), making every +// freshly scheduled row look like a legacy unattributed entry. +func TestHandler_createPlannedPurchases_StampsCreator(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + const userID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + userSession := &Session{UserID: userID, Email: "u@example.com"} + + plan := &config.PurchasePlan{ + ID: "11111111-1111-1111-1111-111111111111", + Name: "Test Plan", + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 0, + }, + } + + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) + mockAuth.grantAdmin() + mockStore.On("GetPurchasePlan", ctx, "11111111-1111-1111-1111-111111111111").Return(plan, nil) + + // Capture every saved execution's CreatedByUserID so we can assert + // the field is stamped on each row in the batch (not just the first). + var savedCreators []*string + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + exec := args.Get(1).(*config.PurchaseExecution) + savedCreators = append(savedCreators, exec.CreatedByUserID) + }). + Return(nil).Times(3) + mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + + body := `{"count": 3, "start_date": "2024-12-01"}` + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + Body: body, + } + result, err := handler.createPlannedPurchases(ctx, req, "11111111-1111-1111-1111-111111111111") + require.NoError(t, err) + assert.Equal(t, 3, result.Created) + + require.Len(t, savedCreators, 3, "expected 3 saved executions") + for i, c := range savedCreators { + require.NotNil(t, c, "execution %d shipped a nil CreatedByUserID (issue #950 regression)", i) + assert.Equal(t, userID, *c, "execution %d shipped the wrong CreatedByUserID", i) + } +} + +// TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil locks in that +// the stateless admin-API-key path (UserID == apiKeyAdminUserID, not a UUID) +// stamps NULL rather than the literal sentinel. resolveCreatorUserID rejects +// non-UUID UserIDs so the FK to users stays valid; the row falls through to +// the admin / update-any management path exactly like a legacy scheduler- +// created row would. +func TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + + apiKeySession := &Session{UserID: apiKeyAdminUserID, Email: "admin-api-key"} + + plan := &config.PurchasePlan{ + ID: "11111111-1111-1111-1111-111111111111", + Name: "Test Plan", + RampSchedule: config.RampSchedule{ + StepIntervalDays: 7, + CurrentStep: 0, + }, + } + + mockAuth.On("ValidateSession", ctx, "api-key").Return(apiKeySession, nil) + mockAuth.grantAdmin() + mockStore.On("GetPurchasePlan", ctx, "11111111-1111-1111-1111-111111111111").Return(plan, nil) + + var savedCreators []*string + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + exec := args.Get(1).(*config.PurchaseExecution) + savedCreators = append(savedCreators, exec.CreatedByUserID) + }). + Return(nil).Times(2) + mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + + body := `{"count": 2, "start_date": "2024-12-01"}` + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer api-key"}, + Body: body, + } + _, err := handler.createPlannedPurchases(ctx, req, "11111111-1111-1111-1111-111111111111") + require.NoError(t, err) + + require.Len(t, savedCreators, 2) + for i, c := range savedCreators { + assert.Nil(t, c, "execution %d should ship a nil CreatedByUserID for the admin-API-key path", i) + } +} + // TestHandler_createPlannedPurchases_MidLoopFailureRollsBack verifies // the partial-failure regression CodeRabbit flagged: a save failure on // row N must NOT leave rows 1..N-1 persisted (they would be retried as From 94326f6b92c35b7ac01f3e8ece41a243fd772cd9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:41:29 +0200 Subject: [PATCH 5/5] fix(dashboard): gate upcoming-purchase Cancel on creator-scope ownership (closes #950 followup) Adversarial review of PR #995's frontend permission walk uncovered a second site that bypassed the new ownership gate: the dashboard "Upcoming purchases" widget. Both the card-level Cancel button and the modal Cancel Purchase button in dashboard.ts called api.deletePlannedPurchase(executionId) unconditionally, while the backend now (correctly) 403s for non-owners after PR #995's authorizeExecutionManagement gate. The Plans page hides those buttons via canManageScheduledPurchase; the Dashboard widget rendered them for everyone, so a standard user looking at another user's scheduled purchase saw an actionable Cancel button and got a confusing 403 toast on click. The UpcomingPurchase backend struct also lacked CreatedByUserID, so the frontend had no way to apply the gate even if it wanted to. Fix spans backend (struct + projection) and frontend (types + render). Backend: - Add CreatedByUserID *string with omitempty json tag to the internal/api/types.UpcomingPurchase response struct. - Populate from exec.CreatedByUserID in upcomingFromExecution (handler_dashboard.go). Frontend: - Add created_by_user_id?: string to UpcomingPurchase in both frontend/src/api/types.ts and frontend/src/types.ts (the latter is the local dashboard-side view). - New canCancelUpcomingPurchase() helper in dashboard.ts mirrors plans.ts's canManageScheduledPurchase: admin / update-any sees Cancel on any row; otherwise created_by_user_id must match the current user; legacy NULL-creator rows hide for non-privileged users. Additionally requires the base delete:purchases verb so a read-only user never sees the button even on their own row. - Gate the card-level Cancel button (renderUpcomingPurchases) and the modal Cancel Purchase button (buildUpcomingDetailsModal) on the new helper. View Details stays unrestricted -- only the destructive action needs the gate. Regression tests: - frontend/__tests__/dashboard-ownership-950.test.ts (new): drives loadDashboard end-to-end across the {owner, non-owner, update-any, admin, legacy-NULL, no-delete-verb} matrix. - TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID: locks in the backend struct propagation -- owned rows ship the UUID, scheduler-tick NULL rows stay nil (omitempty so the JSON wire shape is unchanged on legacy rows). - Existing dashboard.test.ts cancel-flow tests now mock state.getCurrentUser as an admin so the click path stays exercised end-to-end; the dedicated ownership tests above cover the gate logic on its own. --- .../__tests__/dashboard-ownership-950.test.ts | 198 ++++++++++++++++++ frontend/src/__tests__/dashboard.test.ts | 11 + frontend/src/api/types.ts | 7 + frontend/src/dashboard.ts | 97 ++++++--- frontend/src/types.ts | 5 + internal/api/handler_dashboard.go | 1 + internal/api/handler_dashboard_test.go | 60 ++++++ internal/api/types.go | 9 + 8 files changed, 359 insertions(+), 29 deletions(-) create mode 100644 frontend/src/__tests__/dashboard-ownership-950.test.ts diff --git a/frontend/src/__tests__/dashboard-ownership-950.test.ts b/frontend/src/__tests__/dashboard-ownership-950.test.ts new file mode 100644 index 000000000..335d1ae97 --- /dev/null +++ b/frontend/src/__tests__/dashboard-ownership-950.test.ts @@ -0,0 +1,198 @@ +/** + * Issue #950 follow-up: creator-scope ownership gating on the dashboard + * upcoming-purchases widget's Cancel buttons. + * + * The pre-fix dashboard widget rendered a "Cancel" button on every + * upcoming-purchase card (and a "Cancel Purchase" button in the + * View Details modal) regardless of who created the underlying + * execution. Clicking it called DELETE /api/purchases/planned/{id}, + * which the backend now (correctly) 403s for non-owners after PR #995. + * The result was a UX hole: the operator sees the button, clicks it, + * and gets a confusing failure toast. + * + * The fix gates both buttons on canCancelUpcomingPurchase(), which + * mirrors plans.ts's canManageScheduledPurchase: admin / update-any + * see Cancel on any row; otherwise the row's created_by_user_id must + * match the current user (legacy NULL-creator rows are out of reach + * for non-privileged users). + * + * These tests drive the real renderUpcomingPurchases pipe via + * loadDashboard() so the production gate is what's exercised, not a + * unit shim around the helper. + */ + +// Chart.js + recommendations + freshness need to be mocked before the +// dashboard import the same way dashboard.test.ts does. +const mockShowToast = jest.fn<{ dismiss: () => void }, [unknown]>(() => ({ dismiss: jest.fn() })); +jest.mock('../toast', () => ({ + showToast: (opts: unknown) => mockShowToast(opts), +})); +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn(() => Promise.resolve(true)), +})); +jest.mock('chart.js', () => { + const MockChart = jest.fn().mockImplementation(() => ({ destroy: jest.fn() })); + (MockChart as unknown as { register: jest.Mock }).register = jest.fn(); + return { Chart: MockChart, registerables: [] }; +}); +jest.mock('../recommendations', () => ({ + groupRecsByCell: jest.fn(() => new Map()), + pageLevelRange: jest.fn(() => ({ savingsMin: 0, savingsMax: 0, cellCount: 0 })), + formatSavingsRange: jest.fn((min: number, max: number) => `$${min}-$${max}`), + triggerAutoRefreshIfStale: jest.fn(() => Promise.resolve()), +})); + +jest.mock('../api', () => ({ + getDashboardSummary: jest.fn().mockResolvedValue({ potential_monthly_savings: 0, by_service: {} }), + getUpcomingPurchases: jest.fn(), + getPurchaseDetails: jest.fn(), + cancelPurchase: jest.fn(), + deletePlannedPurchase: jest.fn().mockResolvedValue({}), + deletePlan: jest.fn(), + listAccounts: jest.fn().mockResolvedValue([]), + getSavingsAnalytics: jest.fn().mockResolvedValue({ data_points: [] }), + getRecommendations: jest.fn().mockResolvedValue([]), +})); + +jest.mock('../state', () => ({ + getCurrentProvider: jest.fn().mockReturnValue(''), + setCurrentProvider: jest.fn(), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + setCurrentAccountIDs: jest.fn(), + getSavingsChart: jest.fn().mockReturnValue(null), + setSavingsChart: jest.fn(), + subscribeProvider: jest.fn().mockReturnValue(() => {}), + subscribeAccount: jest.fn().mockReturnValue(() => {}), + getCurrentUser: jest.fn(), +})); + +jest.mock('../utils', () => ({ + formatCurrency: jest.fn((val) => `$${val || 0}`), + getDateParts: jest.fn(() => ({ day: 15, month: 'Jan' })), + escapeHtml: jest.fn((str) => str || ''), + populateAccountFilter: jest.fn(() => Promise.resolve()), +})); + +import { loadDashboard } from '../dashboard'; +import * as api from '../api'; +import * as state from '../state'; + +const CREATOR_ID = 'creator-aaaa'; +const OTHER_ID = 'other-bbbb'; +const ADMIN_GROUP = '00000000-0000-5000-8000-000000000001'; + +const ownedPurchase = { + execution_id: 'exec-1', + plan_id: 'plan-1', + plan_name: 'Owned Plan', + scheduled_date: '2026-06-01', + provider: 'aws', + service: 'ec2', + step_number: 1, + total_steps: 4, + estimated_savings: 100, + created_by_user_id: CREATOR_ID, +}; + +const legacyPurchase = { + ...ownedPurchase, + execution_id: 'exec-legacy', + created_by_user_id: undefined as string | undefined, +}; + +type StubUserOpts = { updateAny?: boolean; admin?: boolean; deletePurchases?: boolean }; +const setUser = (id: string, opts: StubUserOpts = {}) => { + const effectivePermissions: Array<{ action: string; resource: string }> = []; + if (opts.admin) { + effectivePermissions.push({ action: 'admin', resource: '*' }); + } else if (opts.deletePurchases !== false) { + // Standard user holds delete:purchases (PR #660 default). + effectivePermissions.push({ action: 'delete', resource: 'purchases' }); + } + if (opts.updateAny) { + effectivePermissions.push({ action: 'update-any', resource: 'purchases' }); + } + (state.getCurrentUser as jest.Mock).mockReturnValue({ + id, + email: `${id}@example.com`, + groups: opts.admin ? [ADMIN_GROUP] : [], + effectivePermissions, + }); +}; + +const setupDom = () => { + document.body.innerHTML = ` +
+
+ + +
+
+ `; +}; + +const cancelBtns = () => + document.querySelectorAll('[data-action="cancel-purchase"]'); + +describe('Dashboard upcoming-purchase ownership gating (issue #950)', () => { + beforeEach(() => { + jest.clearAllMocks(); + setupDom(); + }); + + test('creator sees the Cancel button on their own scheduled purchase', async () => { + setUser(CREATOR_ID); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(1); + }); + + test('non-creator with the same verbs sees NO Cancel button (the bug)', async () => { + setUser(OTHER_ID); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(0); + // The card still renders -- the operator sees the row and can click + // "View Details", which is intentionally unrestricted. + const viewBtns = document.querySelectorAll('[data-action="view-purchase"]'); + expect(viewBtns).toHaveLength(1); + }); + + test('update-any holder sees Cancel on another user\'s scheduled purchase', async () => { + setUser(OTHER_ID, { updateAny: true }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(1); + }); + + test('admin sees Cancel on every row (admin:* covers delete:purchases)', async () => { + setUser(OTHER_ID, { admin: true }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ + purchases: [ownedPurchase, { ...ownedPurchase, execution_id: 'exec-2' }], + }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(2); + }); + + test('legacy NULL-creator row shows no Cancel for a non-update-any user', async () => { + setUser(CREATOR_ID); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [legacyPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(0); + }); + + test('user without delete:purchases sees no Cancel even on their own row', async () => { + // Read-only style: holds neither delete:purchases nor admin nor + // update-any. Even on their own row the button must stay hidden + // because the backend would reject the click on verb grounds. + (state.getCurrentUser as jest.Mock).mockReturnValue({ + id: CREATOR_ID, + email: 'ro@example.com', + groups: [], + effectivePermissions: [{ action: 'view', resource: 'purchases' }], + }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [ownedPurchase] }); + await loadDashboard(); + expect(cancelBtns()).toHaveLength(0); + }); +}); diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index 7b35dd964..91a6bd7d3 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -87,6 +87,17 @@ jest.mock('../state', () => ({ // during setup to register its reload callback. subscribeProvider: jest.fn().mockReturnValue(() => {}), subscribeAccount: jest.fn().mockReturnValue(() => {}), + // Issue #950: the dashboard upcoming-purchase widget now gates the + // Cancel button on creator-scope ownership (canCancelUpcomingPurchase + // -> canAccess + getCurrentUser). Default the session to an admin so + // the pre-#950 cancel-flow tests below keep exercising the click path; + // the dedicated ownership tests override this per-test. + getCurrentUser: jest.fn().mockReturnValue({ + id: 'admin-user', + email: 'admin@example.com', + groups: ['00000000-0000-5000-8000-000000000001'], + effectivePermissions: [{ action: 'admin', resource: '*' }], + }), })); // Mock utils diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index fe32e9224..f56a98f56 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -107,6 +107,13 @@ export interface UpcomingPurchase { step_number: number; total_steps: number; estimated_savings: number; + // created_by_user_id is the UUID of the user who scheduled the + // execution, propagated by the backend so the dashboard's "Cancel" + // button can apply the same creator-scope ownership gate the Plans + // page uses (issue #950). Optional because legacy / scheduler-tick + // rows ship NULL here; the gate treats undefined as "not the current + // user" and hides the button for non-update-any callers. + created_by_user_id?: string; } // Recommendation types diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index e3bab9fc7..6c03c6dc6 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -10,6 +10,7 @@ import type { DashboardSummary, UpcomingPurchase, ServiceSavings, LocalRecommend import type { SavingsDataPoint } from './api'; import { showToast } from './toast'; import { confirmDialog } from './confirmDialog'; +import { canAccess } from './permissions'; import { groupRecsByCell, pageLevelRange, formatSavingsRange, triggerAutoRefreshIfStale } from './recommendations'; import { showSkeletonTiles, showSkeletonBlock, teardownSkeleton } from './lib/skeleton'; @@ -373,6 +374,31 @@ function attachSparkline(key: string, values: readonly number[]): void { export const __test__ = { sparklinePoints, attachSparkline, computeServiceStats }; +// canCancelUpcomingPurchase returns true when the current session is +// permitted to cancel the given upcoming purchase via the Dashboard +// widget (issue #950). UX gate only -- the backend +// authorizeExecutionManagement in internal/api/handler_purchases.go +// remains the security boundary; a false-positive surfaces as a 403 +// toast rather than a successful mutation. +// +// Mirrors canManageScheduledPurchase in plans.ts so the Plans page and +// the Dashboard widget agree on which Cancel buttons appear: +// * admin (admin:*) or update-any:purchases -> can cancel any row; +// * otherwise the row's created_by_user_id must match the current user; +// * legacy / scheduler-tick rows with undefined created_by_user_id -> +// no Cancel button for non-privileged users (out of reach without +// update-any). +// Additionally requires the base delete:purchases verb the backend +// handler asks for, mirroring the gate on the Plans page disable button. +export function canCancelUpcomingPurchase(purchase: UpcomingPurchase): boolean { + if (!canAccess('delete', 'purchases')) return false; + if (canAccess('admin', '*') || canAccess('update-any', 'purchases')) return true; + const user = state.getCurrentUser(); + if (!user) return false; + if (!purchase.created_by_user_id) return false; + return purchase.created_by_user_id === user.id; +} + function renderUpcomingPurchases(purchases: UpcomingPurchase[]): void { const container = document.getElementById('upcoming-list'); if (!container) return; @@ -447,13 +473,20 @@ function renderUpcomingPurchases(purchases: UpcomingPurchase[]): void { viewBtn.dataset['action'] = 'view-purchase'; viewBtn.dataset['id'] = String(p.execution_id); viewBtn.textContent = 'View Details'; - const cancelBtn = document.createElement('button'); - cancelBtn.dataset['action'] = 'cancel-purchase'; - cancelBtn.dataset['id'] = String(p.execution_id); - cancelBtn.className = 'danger'; - cancelBtn.textContent = 'Cancel'; actions.appendChild(viewBtn); - actions.appendChild(cancelBtn); + // Issue #950: Cancel routes to DELETE /api/purchases/planned/{id}, + // which the backend now gates on creator-scope ownership. Hide the + // button when the current session is not authorised so the operator + // doesn't get a 403 toast on click. Plans page applies the same gate + // via canManageScheduledPurchase + canAccess('delete','purchases'). + if (canCancelUpcomingPurchase(p)) { + const cancelBtn = document.createElement('button'); + cancelBtn.dataset['action'] = 'cancel-purchase'; + cancelBtn.dataset['id'] = String(p.execution_id); + cancelBtn.className = 'danger'; + cancelBtn.textContent = 'Cancel'; + actions.appendChild(cancelBtn); + } card.appendChild(info); card.appendChild(savings); @@ -545,30 +578,36 @@ function buildUpcomingDetailsModal(p: UpcomingPurchase, executionId: string): HT btnRow.className = 'modal-buttons'; content.appendChild(btnRow); - const cancelBtn = document.createElement('button'); - cancelBtn.type = 'button'; - cancelBtn.id = 'cancel-purchase-detail-btn'; - cancelBtn.className = 'danger'; - cancelBtn.textContent = 'Cancel Purchase'; - cancelBtn.addEventListener('click', async () => { - const ok = await confirmDialog({ - title: 'Cancel this scheduled purchase?', - body: 'Cancelling a scheduled purchase cannot be undone. Any upfront cost already committed will not be refunded.', - confirmLabel: 'Cancel purchase', - destructive: true, + // Issue #950: gate the Cancel button on the same ownership check as + // the card-level button above. Users land here from "View Details", + // which is visible to everyone; only the destructive action needs the + // creator-scope gate. + if (canCancelUpcomingPurchase(p)) { + const cancelBtn = document.createElement('button'); + cancelBtn.type = 'button'; + cancelBtn.id = 'cancel-purchase-detail-btn'; + cancelBtn.className = 'danger'; + cancelBtn.textContent = 'Cancel Purchase'; + cancelBtn.addEventListener('click', async () => { + const ok = await confirmDialog({ + title: 'Cancel this scheduled purchase?', + body: 'Cancelling a scheduled purchase cannot be undone. Any upfront cost already committed will not be refunded.', + confirmLabel: 'Cancel purchase', + destructive: true, + }); + if (!ok) return; + try { + await api.deletePlannedPurchase(executionId); + modal.remove(); + await loadDashboard(); + showToast({ message: 'Purchase cancelled successfully', kind: 'success', timeout: 5_000 }); + } catch (cancelError) { + console.error('Failed to cancel purchase:', cancelError); + showToast({ message: 'Failed to cancel purchase', kind: 'error' }); + } }); - if (!ok) return; - try { - await api.deletePlannedPurchase(executionId); - modal.remove(); - await loadDashboard(); - showToast({ message: 'Purchase cancelled successfully', kind: 'success', timeout: 5_000 }); - } catch (cancelError) { - console.error('Failed to cancel purchase:', cancelError); - showToast({ message: 'Failed to cancel purchase', kind: 'error' }); - } - }); - btnRow.appendChild(cancelBtn); + btnRow.appendChild(cancelBtn); + } const closeBtn = document.createElement('button'); closeBtn.type = 'button'; diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 5b37e7fe3..0413014d1 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -47,6 +47,11 @@ export interface UpcomingPurchase { step_number: number; total_steps: number; estimated_savings: number; + // created_by_user_id mirrors the field on api.UpcomingPurchase so the + // dashboard widget can apply the issue-#950 creator-scope ownership + // gate on the Cancel button. Optional: legacy / scheduler-tick rows + // ship NULL here. + created_by_user_id?: string; } // Recommendations types diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 2321355dd..56a897f8a 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -394,6 +394,7 @@ func upcomingFromExecution(plan *config.PurchasePlan, exec *config.PurchaseExecu StepNumber: exec.StepNumber, TotalSteps: plan.RampSchedule.TotalSteps, EstimatedSavings: exec.EstimatedSavings, + CreatedByUserID: exec.CreatedByUserID, } } diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index ec13d43e9..181b75f63 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -429,6 +429,66 @@ func TestHandler_getUpcomingPurchases(t *testing.T) { assert.Equal(t, 2, second.StepNumber) } +// TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID is the +// issue-#950 follow-up regression: the dashboard widget on the frontend +// applies a creator-scope ownership gate on the Cancel button (mirrors +// the Plans page); it can only do so if the backend ships +// created_by_user_id on every row. Pre-fix the field was absent, so the +// widget defaulted to "no owner known" and either showed Cancel for +// everyone (when ungated) or for nobody (when gated) -- both wrong. +func TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + scheduled := time.Now().AddDate(0, 0, 7) + plan := config.PurchasePlan{ + ID: "11111111-1111-1111-1111-111111111111", + Name: "Owned Plan", + Enabled: true, + Services: map[string]config.ServiceConfig{ + "aws/ec2": {Provider: "aws", Service: "ec2"}, + }, + RampSchedule: config.RampSchedule{TotalSteps: 4}, + } + creator := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + pending := []config.PurchaseExecution{ + { + ExecutionID: "11112222-3333-4444-5555-666677778888", + PlanID: plan.ID, + Status: "pending", + ScheduledDate: scheduled, + StepNumber: 1, + CreatedByUserID: &creator, + }, + { + // Legacy / scheduler-tick row: NULL creator. Must serialise as + // no created_by_user_id field (omitempty on the JSON tag) so + // the frontend treats it as out-of-reach for non-update-any + // users -- the documented #950 behaviour. + ExecutionID: "99998888-7777-6666-5555-444433332222", + PlanID: plan.ID, + Status: "pending", + ScheduledDate: scheduled.AddDate(0, 0, 7), + StepNumber: 2, + CreatedByUserID: nil, + }, + } + + mockStore.On("GetPendingExecutions", ctx).Return(pending, nil) + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return([]config.PurchasePlan{plan}, nil) + + mockAuth, req := adminDashboardReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.getUpcomingPurchases(ctx, req) + require.NoError(t, err) + require.Len(t, result.Purchases, 2) + + require.NotNil(t, result.Purchases[0].CreatedByUserID, "owned-row CreatedByUserID must propagate") + assert.Equal(t, creator, *result.Purchases[0].CreatedByUserID) + assert.Nil(t, result.Purchases[1].CreatedByUserID, "legacy NULL-creator row must stay nil") +} + // TestHandler_getUpcomingPurchases_OrphanExecutionSkipped guards against the // "execution row with deleted parent plan" cleanup-gap edge case: rather // than crash, the widget hides the orphan. Cleanup is a separate concern. diff --git a/internal/api/types.go b/internal/api/types.go index 302aa5fd0..64aff5d71 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -624,6 +624,15 @@ type UpcomingPurchase struct { StepNumber int `json:"step_number"` TotalSteps int `json:"total_steps"` EstimatedSavings float64 `json:"estimated_savings"` + // CreatedByUserID propagates the underlying execution's + // created_by_user_id so the dashboard widget can apply the same + // creator-scope ownership gate the Plans page uses (issue #950). + // Without it the widget renders a "Cancel" button on every row + // while the backend now 403s for non-owners -- a UX hole that + // surfaces as a confusing toast on click. Mirrors the field on + // PlannedPurchase / PurchaseHistoryEntry. omitempty so legacy + // NULL-creator rows keep the JSON shape they had pre-fix. + CreatedByUserID *string `json:"created_by_user_id,omitempty"` } // PlannedPurchasesResponse holds the list of planned purchases