diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 65626050b..a6a0379f8 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -509,7 +509,7 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return nil, err } - if execution.Status != "pending" && execution.Status != "notified" { + if !execution.IsCancelable() { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled (status=%s)", execution.ExecutionID, execution.Status)) } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 14e1f5262..4fc522c6a 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1721,6 +1721,57 @@ func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { mockAuth.AssertExpectations(t) } +// TestHandler_cancelPurchase_Session_RejectsEachNonCancelableStatus is the +// session-path companion to the token-path #645 regression guard: every +// status outside pending/notified must be rejected with a 409 and no write, +// for parity with purchase.Manager.CancelExecution. The admin session keeps +// the focus on the status guard (which fires before authorizeSessionCancel) +// rather than the RBAC matrix, already covered by the matrix tests above. +func TestHandler_cancelPurchase_Session_RejectsEachNonCancelableStatus(t *testing.T) { + rejected := []string{"approved", "running", "paused", "failed", "expired", "completed", "cancelled"} + for _, status := range rejected { + t.Run(status, func(t *testing.T) { + creator := cancelCallerID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: status, + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "admin"} + + handler, mockConfig, mockAuth := 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(), status) + mockConfig.AssertNotCalled(t, "WithTx") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) + }) + } +} + +// TestHandler_cancelPurchase_Session_AllowsEachCancelableStatus confirms the +// inverse: pending and notified rows remain cancelable on the session path, +// guarding against an over-restriction that would break the dashboard cancel +// of a row awaiting approval. +func TestHandler_cancelPurchase_Session_AllowsEachCancelableStatus(t *testing.T) { + allowed := []string{"pending", "notified"} + for _, status := range allowed { + t.Run(status, func(t *testing.T) { + creator := cancelCallerID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: status, + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} + runSessionCancelAllowed(t, exec, session, false, false) + }) + } +} + 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 diff --git a/internal/config/types.go b/internal/config/types.go index 66edb632d..b89603cc6 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -266,6 +266,18 @@ type PurchaseExecution struct { ApprovalTokenExpiresAt *time.Time `json:"approval_token_expires_at,omitempty" dynamodbav:"approval_token_expires_at,omitempty"` } +// IsCancelable reports whether an execution may still be cancelled. Only the +// pre-purchase states ("pending"/"notified") qualify: once a row reaches +// "approved" or "running" the AWS commitment is being or has been created, so +// cancelling would leave the DB and the cloud out of sync; "cancelled", +// "completed", "failed", "expired", and "paused" are likewise non-cancelable. +// Both cancel paths (purchase.Manager.CancelExecution on the email-token flow +// and the session-authed cancelPurchaseViaSession) call this single predicate +// so the policy can never drift between them (issue #645). +func (e *PurchaseExecution) IsCancelable() bool { + return e.Status == "pending" || e.Status == "notified" +} + // RecommendationRecord stores a recommendation with purchase status type RecommendationRecord struct { ID string `json:"id" dynamodbav:"id"` diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 998592629..2891993a5 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -188,7 +188,17 @@ func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, toke return nil, fmt.Errorf("approval token has expired") } - if execution.Status == "completed" || execution.Status == "cancelled" { + // Only pending/notified rows are cancelable — shares the single + // PurchaseExecution.IsCancelable predicate with the session path in + // cancelPurchaseViaSession so the policy can never drift between the two + // flows (issue #645). The previous predicate rejected only + // completed/cancelled, which let an email-link holder cancel an + // approved/running/paused/failed/expired execution that the dashboard + // user cannot. Restricting to the pre-purchase states is also the + // in-flight guard: approved/running rows are mid-execution (the AWS + // commitment is being or has been created), so cancelling them would + // leave the DB and the cloud out of sync. + if !execution.IsCancelable() { return nil, fmt.Errorf("execution cannot be cancelled, current status: %s", execution.Status) } return execution, nil diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index bc04bb308..44f20acc0 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -374,6 +374,92 @@ func TestManager_CancelExecution_AlreadyCompleted(t *testing.T) { mockStore.AssertExpectations(t) } +// TestManager_CancelExecution_RejectsNonCancelableStatus is the regression +// guard for issue #645: the token/email cancel path previously rejected only +// completed/cancelled, so an email-link holder could cancel an +// approved/running/paused/failed/expired execution that the dashboard +// (session) path refuses. Each non-pending/notified status must now be +// rejected with no write to the store — approved/running rows in particular +// are mid-execution and cancelling them would desync the DB from the cloud. +func TestManager_CancelExecution_RejectsNonCancelableStatus(t *testing.T) { + rejected := []string{"approved", "running", "paused", "failed", "expired", "completed", "cancelled"} + for _, status := range rejected { + t.Run(status, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + execution := &config.PurchaseExecution{ + ExecutionID: "exec-123", + PlanID: "plan-456", + Status: status, + ApprovalToken: "valid-token", + } + mockStore.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.CancelExecution(ctx, "exec-123", "valid-token", status) + require.Error(t, err) + assert.Contains(t, err.Error(), "execution cannot be cancelled") + assert.Contains(t, err.Error(), status) + // Status guard must fire before any persistence — a rejected + // cancel must not flip the row or drop suppressions. + // SavePurchaseExecutionTx forwards to SavePurchaseExecution in + // the mock, so this single assertion covers both the tx and + // non-tx write paths. + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockStore.AssertExpectations(t) + }) + } +} + +// TestManager_CancelExecution_AllowsCancelableStatus confirms the inverse of +// the #645 guard: genuinely-pending and notified rows are still cancelable on +// the token path, and the cancel commits (status flip + suppression cleanup) +// in a single tx. Without this the alignment fix could silently over-restrict +// and break the legitimate email-link cancel of a row awaiting approval. +func TestManager_CancelExecution_AllowsCancelableStatus(t *testing.T) { + allowed := []string{"pending", "notified"} + for _, status := range allowed { + t.Run(status, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + execution := &config.PurchaseExecution{ + ExecutionID: "exec-123", + PlanID: "plan-456", + Status: status, + ApprovalToken: "valid-token", + } + mockStore.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + saved = args.Get(1).(*config.PurchaseExecution) + }). + Return(nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.CancelExecution(ctx, "exec-123", "valid-token", "") + require.NoError(t, err) + require.NotNil(t, saved, "cancel should persist the execution") + assert.Equal(t, "cancelled", saved.Status) + mockStore.AssertExpectations(t) + }) + } +} + func TestManager_CancelExecution_NotFound(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore)