From 5780b34bd7c4fbf202e5618b4b6d847f7330b903 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 22:58:02 +0200 Subject: [PATCH] fix(purchase): map session approve and direct execute failures by cause Session approve and direct execute answered 409 for every error, including failures after the claim where money may have moved, and direct execute stamped executed_at before the claim, so a refused run kept it. ApproveAndExecute now returns the row, and a new DirectExecute stamps the executed_* audit fields only after the claim is won. Both handlers map errors through executeFailureError (renamed from runNowError): 403 for a four-eyes denial, 409 for a lost claim, 500 before the claim, 502 with the row's status after it. Refs #532 --- .../api/executed_notification_flow_test.go | 6 +- internal/api/execution_scope_test.go | 2 +- internal/api/handler_purchases.go | 74 +++---- internal/api/handler_purchases_test.go | 185 +++++++++++++----- internal/api/middleware_test.go | 2 +- internal/api/mocks_test.go | 11 +- internal/api/openapi.yaml | 28 ++- internal/api/purchase_pricing_test.go | 10 +- internal/api/session_denial_final_test.go | 2 +- internal/api/types.go | 5 +- internal/purchase/approvals.go | 22 ++- internal/purchase/approvals_test.go | 86 +++++++- internal/purchase/armed_redrive_test.go | 2 +- internal/server/interfaces.go | 5 +- internal/testutil/mocks.go | 14 +- 15 files changed, 319 insertions(+), 135 deletions(-) diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index a2c37d934..6f89a9826 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -239,7 +239,7 @@ func TestExecutedNotification_SessionApprovePath(t *testing.T) { // execution's ApprovalToken (the pre-fix behavior) is no longer possible, // so ApproveAndExecute mints and returns one on every successful approve // regardless of which path triggered it. - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(freshToken, nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil, freshToken, nil) notifier := &recordingExecutedNotifier{} handler := &Handler{ @@ -299,7 +299,7 @@ func TestExecutedNotification_DirectExecutePath(t *testing.T) { // Direct-execute also mints its own fresh revocation token (issue #103), // replacing the pre-fix "reuse execution.ApprovalToken directly" behavior // -- see the SessionApprovePath test above for the full rationale. - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(freshToken, nil) + mockPurchase.On("DirectExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil, freshToken, nil) notifier := &recordingExecutedNotifier{} handler := &Handler{ @@ -345,7 +345,7 @@ func TestExecutedNotification_DirectExecute_NilNotifierNoPanic(t *testing.T) { mockConfig.On("SavePurchaseExecution", ctx, exec).Return(nil) mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return("", nil) + mockPurchase.On("DirectExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil, "", nil) handler := &Handler{ purchase: mockPurchase, diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index ca7a541fa..576870271 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -211,7 +211,7 @@ func newScopeTestHandler(t *testing.T, exec *config.PurchaseExecution, scope []s store.On("TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errScopeTestReached).Maybe() store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() store.On("CancelScheduledExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errScopeTestReached).Maybe() - mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", errScopeTestReached).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", errScopeTestReached).Maybe() mockPurchase.On("RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", errScopeTestReached).Maybe() mockPurchase.On("CancelExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 78cbc669e..fe0fc1a33 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -349,7 +349,7 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc final, revocationToken, runErr := h.purchase.RunPlannedPurchaseNow(ctx, executionID, fourEyesActorIdentity(session), resolveCreatorUserID(session)) if runErr != nil { - return nil, runNowError(executionID, final, runErr) + return nil, executeFailureError(executionID, "started", final, runErr) } // The shared execute funnel rotated the row's token into a revocation @@ -364,17 +364,19 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc }, nil } -// runNowError maps a RunPlannedPurchaseNow failure to its HTTP error. A non-nil +// executeFailureError maps a failure of the shared approve-and-execute funnel +// (run-now, session approve, direct execute) to its HTTP error. action names +// what the caller tried ("started", "approved", "direct-executed"). A non-nil // final means the purchase ran and money may have moved, so it is never a 409. // ErrNotFound and ErrExecutionNotInExpectedStatus are 409 only before a claim. -func runNowError(executionID string, final *config.PurchaseExecution, err error) error { +func executeFailureError(executionID, action string, final *config.PurchaseExecution, err error) error { switch { case errors.Is(err, purchase.ErrFourEyesDenied): - return NewClientError(403, fmt.Sprintf("execution %s cannot be started: %v", executionID, err)) + return NewClientError(403, fmt.Sprintf("execution %s cannot be %s: %v", executionID, action, err)) case final == nil && (errors.Is(err, config.ErrExecutionNotInExpectedStatus) || errors.Is(err, config.ErrNotFound)): - return NewClientError(409, fmt.Sprintf("execution %s cannot be started: %v", executionID, err)) + return NewClientError(409, fmt.Sprintf("execution %s cannot be %s: %v", executionID, action, err)) case final == nil: - return fmt.Errorf("execution %s could not be started: %w", executionID, err) + return fmt.Errorf("execution %s could not be %s: %w", executionID, action, err) case errors.Is(err, config.ErrAuditLoss): return NewClientError(500, fmt.Sprintf("execution %s ran but its final status could not be saved: %v", executionID, err)) default: @@ -839,16 +841,11 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam // approval (issue #103); only the token hash is stored, so this // return value is the only place a raw, emailable token exists after // the call returns. - revocationToken, approveErr := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, fourEyesActorIdentity(session), actor) + final, revocationToken, approveErr := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, fourEyesActorIdentity(session), actor) if approveErr != nil { - // ApproveAndExecute returns either a transition error (the row - // drifted out of pending/notified between our check and the UPDATE - // -- race with cancel/expire) or an execution error (AWS API failed, - // status is now "failed" on disk). Both surface as 409 to the - // caller; the History view shows the resulting row state. logging.Errorf("purchase[%s]: approvePurchaseViaSession failed after %s: %v", execution.ExecutionID, time.Since(t0), approveErr) - return nil, NewClientError(409, fmt.Sprintf("execution %s could not be approved: %v", execution.ExecutionID, approveErr)) + return nil, executeFailureError(execution.ExecutionID, "approved", final, approveErr) } logging.Infof("purchase[%s]: approvePurchaseViaSession completed in %s (auth=session)", @@ -3119,64 +3116,41 @@ func buildApprovalPendingResponse( // holds execute-any or execute-own on purchases. // // Steps: -// 1. Stamp the three audit fields (executed_by_user_id, executed_at, -// pre_approval_skip_reason) onto the in-memory execution so -// SavePurchaseExecution persists them in the next call inside -// ApproveAndExecute. -// 2. Delegate to purchase.Manager.ApproveAndExecute, which atomically -// transitions the row to "approved" and then runs the purchase -// synchronously. ApproveAndExecute already stamps ApprovedBy; we pass -// the session email as the actor so the approved_by column also records -// who direct-executed. -// 3. Send the best-effort post-execution notification email (issue #291). +// 1. Delegate to purchase.Manager.DirectExecute, which atomically +// transitions the row to "approved" (stamping ApprovedBy), stamps the +// three audit fields (executed_by_user_id, executed_at, +// pre_approval_skip_reason) only once that claim is won, and then runs the +// purchase synchronously. A refused or lost direct execute therefore +// leaves no executed_at on a row that never ran, and failures map through +// executeFailureError (403 four-eyes, 409 lost claim, 502 after the claim). +// 2. Send the best-effort post-execution notification email (issue #291). // This is the only email the direct-execute flow emits -- no approval // email precedes it -- so it is the path where the executed-notification // matters most. -// 4. Return a "completed" status to the caller, carrying any payment-option +// 3. Return a "completed" status to the caller, carrying any payment-option // coercion notices (paymentAdjustments, from the request validation) so // the direct-execute response surfaces them like the approval path does. // -// The audit fields are best-effort if ApproveAndExecute's SavePurchaseExecution -// races with our pre-call stamp -- but in practice ApproveAndExecute calls -// SavePurchaseExecution once after a successful TransitionExecutionStatus, at -// which point our pre-stamp is already on the row that was loaded by -// TransitionExecutionStatus. The critical audit invariant is that a non-nil -// executed_by_user_id always co-occurs with a non-nil pre_approval_skip_reason, -// and both are set atomically in the same SavePurchaseExecution call here. +// The audit invariant is that a non-nil executed_by_user_id always co-occurs +// with a non-nil pre_approval_skip_reason; both are stamped in the same save. func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, session *Session, paymentAdjustments []PaymentAdjustment) (any, error) { t0 := time.Now() executionID := execution.ExecutionID logging.Infof("purchase[%s]: directExecutePurchase entry (auth=session)", executionID) - // Stamp audit fields before the status transition so they are - // present on the row the reaper / history query reads. - if session.UserID != "" { - uid := session.UserID - execution.ExecutedByUserID = &uid - } - now := time.Now() - execution.ExecutedAt = &now - skipReason := "direct-execute permission" - execution.PreApprovalSkipReason = &skipReason - if err := h.config.SavePurchaseExecution(ctx, execution); err != nil { - // Audit-gap: stamp failed but don't block the purchase. Log at - // error level so a CloudWatch alarm can catch persistent failures. - logging.Errorf("AUDIT GAP: failed to stamp direct-execute audit fields on %s: %v", executionID, err) - } - // Human session direct-execute: stamp the session user's UUID onto // transitioned_by (FK-safe via validUUIDPtrOrNil) so the audit trail // records who flipped the row to "approved". // - // revocationToken is the raw token ApproveAndExecute mints for this + // revocationToken is the raw token DirectExecute mints for this // approval (issue #103); only the token hash is stored, so this // return value is the only place a raw, emailable token exists after // the call returns. - revocationToken, err := h.purchase.ApproveAndExecute(ctx, executionID, fourEyesActorIdentity(session), validUUIDPtrOrNil(&session.UserID)) + final, revocationToken, err := h.purchase.DirectExecute(ctx, executionID, fourEyesActorIdentity(session), validUUIDPtrOrNil(&session.UserID)) if err != nil { logging.Errorf("purchase[%s]: directExecutePurchase failed after %s: %v", executionID, time.Since(t0), err) - return nil, NewClientError(409, fmt.Sprintf("execution %s could not be direct-executed: %v", executionID, err)) + return nil, executeFailureError(executionID, "direct-executed", final, err) } logging.Infof("purchase[%s]: directExecutePurchase completed in %s", executionID, time.Since(t0)) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 930d206bb..e1eb8797c 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -337,7 +337,7 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T) // ApproveAndExecute, not ApproveExecution. The token-only path runs // ApproveExecution; the dashboard click runs ApproveAndExecute. Both // converge inside the Manager. - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return("", nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil, "", nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -352,52 +352,64 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T) mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } -// TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409 pins the -// failure shape: when ApproveAndExecute returns an error (e.g. the AWS -// purchase fails or the row drifted out of pending/notified mid-flight), -// the session handler surfaces it as a 409 instead of the optimistic -// "approved" the pre-fix flow returned. Mirrors the rationale in -// approvePurchaseViaSession. -func TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409(t *testing.T) { +// TestHandleRequest_approvePurchase_SessionFailureStatuses drives a session +// approve through the real router and pins the status class per failure: +// a four-eyes denial is 403, a lost claim is 409, a failure before the claim +// is 500, and a failure after the claim (money may have moved, including a +// partial run) is 502 carrying the row's actual status, never a 409 (#532). +func TestHandleRequest_approvePurchase_SessionFailureStatuses(t *testing.T) { ctx := context.Background() execID := "12345678-1234-1234-1234-123456789abc" adminEmail := "admin@example.com" - - mockConfig := new(MockConfigStore) - exec := &config.PurchaseExecution{ - ExecutionID: execID, - ApprovalToken: "valid-token", - Status: "pending", - // Non-zero UpfrontCost: see the comment on the sibling test above. - Recommendations: []config.RecommendationRecord{ - {ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 100}, - }, + cases := []struct { + name string + final *config.PurchaseExecution + err error + wantStatus int + wantRow string + }{ + {"four-eyes denial", nil, fmt.Errorf("%w: same approver", purchase.ErrFourEyesDenied), 403, ""}, + {"lost claim", nil, fmt.Errorf("approve: %w", config.ErrExecutionNotInExpectedStatus), 409, ""}, + {"store error before claim", nil, errors.New("connection reset"), 500, ""}, + {"provider failure after claim", &config.PurchaseExecution{Status: "failed"}, errors.New("AWS RI purchase failed"), 502, "failed"}, + {"partial multi-account run", &config.PurchaseExecution{Status: "partially_completed"}, errors.New("1 of 2 accounts failed"), 502, "partially_completed"}, } - mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) - // approvePurchaseViaSession checks PurchaseDelayHours (issue #291 wave-2) - // and requireDifferentApprover's mode-off default (issue #1005). - mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{PurchaseDelayHours: 0}, nil) - - mockAuth := new(MockAuthService) - mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) - mockAuth.grantAdminPurchaser() - // approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404). - mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mockConfig := new(MockConfigStore) + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "valid-token", + Status: "pending", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 100}, + }, + } + mockConfig.On("GetExecutionByID", mock.Anything, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{PurchaseDelayHours: 0}, nil) - mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return("", errors.New("AWS RI purchase failed")) + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: adminEmail}, nil) + mockAuth.grantAdminPurchaser() + mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil) - handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", mock.Anything, execID, adminEmail, (*string)(nil)).Return(tc.final, "", tc.err) - req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"authorization": "Bearer sess-tok"}, + handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth, corsAllowedOrigin: "*"} + resp, err := handler.HandleRequest(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST", Path: "/api/purchases/approve/" + execID}, + }, + }) + require.NoError(t, err) + assert.Equal(t, tc.wantStatus, resp.StatusCode, resp.Body) + if tc.wantRow != "" { + assert.Contains(t, resp.Body, tc.wantRow) + } + }) } - _, err := handler.approvePurchase(ctx, req, execID, "") - require.Error(t, err) - ce, ok := IsClientError(err) - require.True(t, ok, "expected a clientError") - assert.Equal(t, 409, ce.code) - assert.Contains(t, ce.Error(), "could not be approved") } // TestHandler_approvePurchase_SessionApproveAny_PermissionConstraintsDenied @@ -842,7 +854,7 @@ func TestHandler_approvePurchase_AWSOrphanFallsThrough(t *testing.T) { mockPurchase := new(MockPurchaseManager) // Guard does not fire; ApproveAndExecute is called normally. - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return("", nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil, "", nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -884,7 +896,7 @@ func TestHandler_approvePurchase_NonOrphanUnchanged(t *testing.T) { mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return("", nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil, "", nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -2201,7 +2213,7 @@ func TestHandler_runPlannedPurchase_NilExecution(t *testing.T) { assert.Nil(t, result) } -func TestRunNowError(t *testing.T) { +func TestExecuteFailureError(t *testing.T) { ran := &config.PurchaseExecution{Status: "failed"} cases := []struct { name string @@ -2220,7 +2232,7 @@ func TestRunNowError(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := runNowError("exec-1", tc.final, tc.err) + err := executeFailureError("exec-1", "started", tc.final, tc.err) ce, ok := IsClientError(err) if tc.code == 0 { assert.False(t, ok) @@ -4691,8 +4703,8 @@ func TestHandler_executePurchase_DirectExec_ExecuteAny(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", ctx, adminSession.UserID).Return([]string{}, nil) // Direct-execute is a human session action: the transitioned_by actor // must be the session user's UUID, not nil (issue #1009 audit objective). - mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), adminSession.Email, - mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == adminSession.UserID })).Return("", nil) + mockPurchase.On("DirectExecute", ctx, mock.AnythingOfType("string"), adminSession.Email, + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == adminSession.UserID })).Return(nil, "", nil) setupDirectExecMocks(ctx, mockStore) handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} @@ -4708,6 +4720,66 @@ func TestHandler_executePurchase_DirectExec_ExecuteAny(t *testing.T) { assert.Equal(t, 500.0, resultMap["total_upfront_cost"]) } +// TestHandler_executePurchase_DirectExec_FailureStatuses pins the status class +// per direct-execute failure (#532): four-eyes denial 403, lost claim 409, a +// failure before the claim 500, and a failure after the claim 502 with the +// row's status. The handler itself must never stamp executed_at: the manager +// does that once the claim is won, so a refused run leaves it unset. +func TestHandler_executePurchase_DirectExec_FailureStatuses(t *testing.T) { + cases := []struct { + name string + final *config.PurchaseExecution + err error + wantStatus int + }{ + {"four-eyes denial", nil, fmt.Errorf("%w: same approver", purchase.ErrFourEyesDenied), 403}, + {"lost claim", nil, fmt.Errorf("approve: %w", config.ErrExecutionNotInExpectedStatus), 409}, + {"store error before claim", nil, errors.New("connection reset"), 500}, + {"provider failure after claim", &config.PurchaseExecution{Status: "failed"}, errors.New("AWS RI purchase failed"), 502}, + {"partial run", &config.PurchaseExecution{Status: "partially_completed"}, errors.New("1 of 2 accounts failed"), 502}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockPurchase := new(MockPurchaseManager) + + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute", "purchases").Return(true, nil) + mockAuth.allowConstraintChecks() + mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute-any", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, adminSession.UserID).Return([]string{}, nil) + mockPurchase.On("DirectExecute", ctx, mock.AnythingOfType("string"), adminSession.Email, mock.Anything).Return(tc.final, "", tc.err) + setupDirectExecMocks(ctx, mockStore) + + handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} + _, err := handler.executePurchase(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: directExecRecBody, + }) + require.Error(t, err) + if tc.wantStatus == 500 { + _, isClient := IsClientError(err) + assert.False(t, isClient) + } else { + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, tc.wantStatus, ce.code) + } + for _, call := range mockStore.Calls { + if call.Method != "SavePurchaseExecution" { + continue + } + saved, ok := call.Arguments.Get(1).(*config.PurchaseExecution) + require.True(t, ok) + assert.Nil(t, saved.ExecutedAt, "handler must not stamp executed_at before the claim") + } + }) + } +} + // TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner verifies that a // session with only execute-own:purchases can direct-execute when the // execution's creator matches the session user. @@ -4733,8 +4805,8 @@ func TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", ctx, ownerID).Return([]string{}, nil) // Direct-execute is a human session action: the transitioned_by actor // must be the session user's UUID, not nil (issue #1009 audit objective). - mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), ownerSession.Email, - mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == ownerID })).Return("", nil) + mockPurchase.On("DirectExecute", ctx, mock.AnythingOfType("string"), ownerSession.Email, + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == ownerID })).Return(nil, "", nil) setupDirectExecMocks(ctx, mockStore) handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} @@ -4789,7 +4861,7 @@ func TestHandler_executePurchase_MultiAccountBatch_Rejected(t *testing.T) { mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil).Maybe() mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil).Maybe() mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil).Maybe() - mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", nil).Maybe() + mockPurchase.On("DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", nil).Maybe() handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} req := &events.LambdaFunctionURLRequest{ @@ -4869,11 +4941,21 @@ func TestHandler_executePurchase_DirectExec_FourEyesOn_DeniesSelfExecute(t *test require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok, "expected a clientError") - assert.Equal(t, 409, ce.code) + assert.Equal(t, 403, ce.code) assert.Contains(t, ce.Error(), "4-eyes mode requires a different approver") mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) + // A refused direct execute must leave no executed_at on the row, so only + // the creation save (no ExecutedAt) may have happened. + for _, call := range mockStore.Calls { + if call.Method != "SavePurchaseExecution" { + continue + } + saved, ok := call.Arguments.Get(1).(*config.PurchaseExecution) + require.True(t, ok) + assert.Nil(t, saved.ExecutedAt, "refused direct execute must not stamp executed_at") + } } // TestHandler_executePurchase_DirectExec_FourEyesOn_PerUserAPIKey_DeniesSelfExecute @@ -4901,7 +4983,6 @@ func TestHandler_executePurchase_DirectExec_FourEyesOn_PerUserAPIKey_DeniesSelfE ownerSession := &Session{UserID: ownerID, Email: "", UserAPIKeyID: "key-1"} mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil) - mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) execution := &config.PurchaseExecution{ ExecutionID: "exec-apikey-direct", @@ -4916,11 +4997,13 @@ func TestHandler_executePurchase_DirectExec_FourEyesOn_PerUserAPIKey_DeniesSelfE require.Error(t, err) ce, ok := IsClientError(err) require.True(t, ok, "expected a clientError") - assert.Equal(t, 409, ce.code) + assert.Equal(t, 403, ce.code) assert.Contains(t, ce.Error(), "4-eyes mode requires a different approver") mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) + // A refused direct execute must leave no executed_at on the row. + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } // TestHandler_executePurchase_DirectExec_ExecuteOwn_NonOwner verifies the diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index af8d83d3e..20baaed84 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -418,7 +418,7 @@ func TestApproveViaSession_PassesCSRF(t *testing.T) { t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return("", nil) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail, (*string)(nil)).Return(nil, "", nil) handler := &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase} diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 228fdcba0..7012a152d 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -53,9 +53,16 @@ func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, toke return args.String(0), args.Error(1) } -func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) { +func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { args := m.Called(ctx, execID, actor, transitionedBy) - return args.String(0), args.Error(1) + exec, _ := args.Get(0).(*config.PurchaseExecution) + return exec, args.String(1), args.Error(2) +} + +func (m *MockPurchaseManager) DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { + args := m.Called(ctx, execID, actor, transitionedBy) + exec, _ := args.Get(0).(*config.PurchaseExecution) + return exec, args.String(1), args.Error(2) } func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 591311ea9..7ae8d2691 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -492,7 +492,18 @@ paths: cause: for the idempotency case, verify the earlier submit's outcome with the provider before resubmitting; for a stored-recommendation conflict, refresh the recommendation set - and resubmit with the current data. + and resubmit with the current data. With execute_mode "direct", + a lost claim on the new execution also answers 409. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '502': + description: > + execute_mode "direct" only. The claim was won and the purchase + ran, but it failed or only partly completed. Money may have moved; + the body details carry execution_id and the row's actual status. + Check the execution in History before retrying. content: application/json: schema: @@ -585,8 +596,8 @@ paths: The execution cannot be approved: a non-AWS execution without a resolvable cloud account, a status other than pending or notified (in token mode the status check is the atomic transition, so a - concurrent change answers the same way), or (session mode) the - purchase itself failed during execution. + concurrent change answers the same way, and in session mode a + lost claim answers 409 as well). content: application/json: schema: @@ -607,6 +618,17 @@ paths: application/json: schema: $ref: '#/components/schemas/Error' + '502': + description: > + Session mode only. The claim was won and the purchase ran, but it + failed or only partly completed (a failed or partially_completed + run). Money may have moved; the body details carry execution_id + and the row's actual status. Check the execution in History + before retrying. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' /api/purchases/cancel/{id}: parameters: diff --git a/internal/api/purchase_pricing_test.go b/internal/api/purchase_pricing_test.go index ab99c3710..c07041d56 100644 --- a/internal/api/purchase_pricing_test.go +++ b/internal/api/purchase_pricing_test.go @@ -63,7 +63,7 @@ func TestHandler_executePurchase_CapUsesStoredPriceNotClientPrice(t *testing.T) mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() - mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", nil).Maybe() + mockPurchase.On("DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", nil).Maybe() handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} req := &events.LambdaFunctionURLRequest{ @@ -77,7 +77,7 @@ func TestHandler_executePurchase_CapUsesStoredPriceNotClientPrice(t *testing.T) assert.Equal(t, 403, ce.code) assert.Contains(t, ce.Error(), "constraints") mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) - mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_executePurchase_StoredPriceUnderCapProceeds is the no-over- @@ -108,7 +108,7 @@ func TestHandler_executePurchase_StoredPriceUnderCapProceeds(t *testing.T) { mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil) mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) - mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", nil) + mockPurchase.On("DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", nil) handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} req := &events.LambdaFunctionURLRequest{ @@ -261,7 +261,7 @@ func TestHandler_executePurchase_CrossAccountMismatchRefused(t *testing.T) { mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() mockStore.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() - mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", nil).Maybe() + mockPurchase.On("DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", nil).Maybe() accountA := "111111111111" expectStoredRecs(mockStore, config.RecommendationRecord{ @@ -281,7 +281,7 @@ func TestHandler_executePurchase_CrossAccountMismatchRefused(t *testing.T) { assert.Equal(t, 409, ce.code) assert.Contains(t, ce.Error(), "not in the current recommendation set") mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) - mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_executePurchase_StoredRecWithoutPriceRefused: a matched stored diff --git a/internal/api/session_denial_final_test.go b/internal/api/session_denial_final_test.go index ec2f09cfb..0f619702f 100644 --- a/internal/api/session_denial_final_test.go +++ b/internal/api/session_denial_final_test.go @@ -35,7 +35,7 @@ func newDenialTestHandler(t *testing.T, store *MockConfigStore, mockPurchase *Mo store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errDenialTestReached).Maybe() store.On("TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errDenialTestReached).Maybe() - mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", errDenialTestReached).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, "", errDenialTestReached).Maybe() ctx := contextWithPrincipal(context.Background(), &Principal{ Kind: PrincipalSession, diff --git a/internal/api/types.go b/internal/api/types.go index ab6bc31b8..5ec84b3d4 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -141,7 +141,10 @@ type BreakdownValue struct { // re-reading the execution from the store). Empty on failure. type PurchaseManagerInterface interface { ApproveExecution(ctx context.Context, execID, token, actor string) (string, error) - ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) + ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + // DirectExecute is ApproveAndExecute for the direct-execute permission; it + // stamps the executed_* audit fields only after the claim is won. + DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) // RunPlannedPurchaseNow forces a pending or paused scheduled purchase to // execute immediately (the "Run now" button), sharing ApproveAndExecute's // 4-eyes-gated, CAS-guarded funnel instead of a bare status flip that diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index bcdb48c15..9c8cb9d57 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -28,6 +28,10 @@ var ( // "Run now" stands in for the approval and the scheduled wait. const runNowSkipReason = "run-now" +// directExecuteSkipReason is stamped onto PreApprovalSkipReason when a session +// holding the direct-execute permission skips the approval round-trip. +const directExecuteSkipReason = "direct-execute permission" + // ApproveExecution is the token-authenticated approve entry point used by // the legacy email-link flow and the SQS approve worker. After validating // the approval token it hands off to ApproveAndExecute, which performs the @@ -72,7 +76,7 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto // Token/SQS path: no authenticated session UUID is available, so the // transition is recorded as system-initiated (transitioned_by = NULL). - revocationToken, err := m.ApproveAndExecute(ctx, executionID, actor, nil) + _, revocationToken, err := m.ApproveAndExecute(ctx, executionID, actor, nil) if err != nil { logging.Errorf("purchase[%s]: ApproveExecution (token path) failed after %s: %v", executionID, time.Since(t0), err) @@ -313,9 +317,19 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string // transition" error. Cross-execution concurrency is unaffected: each // approval drives its own executeAndFinalize, which already fans out // per-account in parallel via executeMultiAccount. -func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string) (string, error) { - _, revocationToken, err := m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified"}, "") - return revocationToken, err +// +// The returned row carries the approval's final status, also on an execution +// error; it is nil when the run never started. +func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { + return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified"}, "") +} + +// DirectExecute is ApproveAndExecute for a session with the direct-execute +// permission: the same 4-eyes-gated, CAS-guarded funnel, but it stamps the +// executed_* audit fields once the CAS has won, so a refused or lost direct +// execute leaves no executed_at on a row that never ran. +func (m *Manager) DirectExecute(ctx context.Context, executionID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { + return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified"}, directExecuteSkipReason) } // RunPlannedPurchaseNow lets an operator force a scheduled purchase (pending diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index a886cef9f..a4cb793fb 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -286,7 +286,7 @@ func TestManager_ApproveAndExecute_EmptyPlanID(t *testing.T) { sender.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil) store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) - _, err := manager.ApproveAndExecute(ctx, "exec-direct-1", "operator@example.com", nil) + _, _, err := manager.ApproveAndExecute(ctx, "exec-direct-1", "operator@example.com", nil) require.NoError(t, err) // Crucially, the empty PlanID must never reach the UUID-typed store columns. @@ -330,7 +330,7 @@ func TestManager_ApproveAndExecute_SkipsTokenCheck(t *testing.T) { mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == actorUUID })).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-789") - _, err := manager.ApproveAndExecute(ctx, "exec-456", "session-user@example.com", &actorUUID) + _, _, err := manager.ApproveAndExecute(ctx, "exec-456", "session-user@example.com", &actorUUID) require.NoError(t, err) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, "session-user@example.com", *updated.ApprovedBy) @@ -397,6 +397,76 @@ func TestManager_RunPlannedPurchaseNow_LostCASReturnsError(t *testing.T) { store.AssertExpectations(t) } +// TestManager_DirectExecute_StampsAuditFieldsAfterClaim pins #532: the +// executed_* audit fields land on the row the claim returned, together with +// the skip reason, and the row comes back to the caller. +func TestManager_DirectExecute_StampsAuditFieldsAfterClaim(t *testing.T) { + ctx := context.Background() + manager, store, sender := newApproveManager(t) + + actorUUID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + updated := &config.PurchaseExecution{ + ExecutionID: "exec-direct-stamp", + PlanID: "plan-direct-stamp", + Status: "approved", + StepNumber: 1, + Recommendations: approvalTestRecs(), + } + store.On("TransitionExecutionStatus", ctx, "exec-direct-stamp", approveFromStatuses, "approved", + mock.MatchedBy(func(actor *string) bool { return actor != nil && *actor == actorUUID })).Return(updated, nil) + stubExecuteChain(t, store, sender, "plan-direct-stamp") + + final, _, err := manager.DirectExecute(ctx, "exec-direct-stamp", "session-user@example.com", &actorUUID) + require.NoError(t, err) + assert.Same(t, updated, final) + require.NotNil(t, final.ExecutedAt) + require.NotNil(t, final.ExecutedByUserID) + assert.Equal(t, actorUUID, *final.ExecutedByUserID) + require.NotNil(t, final.PreApprovalSkipReason) + assert.Equal(t, directExecuteSkipReason, *final.PreApprovalSkipReason) +} + +// TestManager_DirectExecute_RefusedLeavesNoExecutedAt pins #532: a lost claim +// returns no row and saves nothing, so no executed_at can land on a row that +// never ran. +func TestManager_DirectExecute_RefusedLeavesNoExecutedAt(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + + store.On("TransitionExecutionStatus", ctx, "exec-direct-lost", approveFromStatuses, "approved", (*string)(nil)). + Return(nil, config.ErrExecutionNotInExpectedStatus) + + final, _, err := manager.DirectExecute(ctx, "exec-direct-lost", "operator@example.com", nil) + require.ErrorIs(t, err, config.ErrExecutionNotInExpectedStatus) + assert.Nil(t, final) + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// TestManager_ApproveAndExecute_ReturnsRowOnExecutionFailure pins #532: after +// the claim, an execution failure still returns the row (with its final +// status) so the HTTP layer can tell it from a lost claim. +func TestManager_ApproveAndExecute_ReturnsRowOnExecutionFailure(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + factory := new(MockProviderFactory) + factory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(nil, errors.New("credentials rejected")) + manager := &Manager{config: store, email: new(MockEmailSender), providerFactory: factory} + + updated := &config.PurchaseExecution{ + ExecutionID: "exec-fail-after-claim", + Status: "approved", + Recommendations: approvalTestRecs(), + } + store.On("TransitionExecutionStatus", ctx, "exec-fail-after-claim", approveFromStatuses, "approved", (*string)(nil)).Return(updated, nil) + store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + + final, token, err := manager.ApproveAndExecute(ctx, "exec-fail-after-claim", "operator@example.com", nil) + require.Error(t, err) + assert.Empty(t, token) + require.NotNil(t, final) + assert.Equal(t, "failed", final.Status) +} + // ─── enforceFourEyesPolicy at the ApproveAndExecute choke point ─────────────── // (issue #1005 / PR #1500 adversarial review) // @@ -446,7 +516,7 @@ func TestManager_ApproveAndExecute_FourEyesOn_DeniesSelfApprove(t *testing.T) { store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetExecutionByID", ctx, "exec-direct-self").Return(execution, nil) - _, err := manager.ApproveAndExecute(ctx, "exec-direct-self", creatorEmail, &creatorID) + _, _, err := manager.ApproveAndExecute(ctx, "exec-direct-self", creatorEmail, &creatorID) require.Error(t, err) assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") store.AssertNotCalled(t, "TransitionExecutionStatus", @@ -481,7 +551,7 @@ func TestManager_ApproveAndExecute_FourEyesOn_AllowsDifferentApprover(t *testing store.On("TransitionExecutionStatus", ctx, "exec-direct-diff", approveFromStatuses, "approved", &approverUUID).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-fourEyes") - _, err := manager.ApproveAndExecute(ctx, "exec-direct-diff", approverEmail, &approverUUID) + _, _, err := manager.ApproveAndExecute(ctx, "exec-direct-diff", approverEmail, &approverUUID) require.NoError(t, err) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, approverEmail, *updated.ApprovedBy) @@ -515,7 +585,7 @@ func TestManager_ApproveAndExecute_FourEyesOn_PerUserAPIKey_DeniesSelfExecute(t // what internal/api's fourEyesActorIdentity would pass for a per-user API // key session; transitionedBy is the same UUID, populated regardless of // Email by validUUIDPtrOrNil(&session.UserID). - _, err := manager.ApproveAndExecute(ctx, "exec-apikey-self", creatorID, &creatorID) + _, _, err := manager.ApproveAndExecute(ctx, "exec-apikey-self", creatorID, &creatorID) require.Error(t, err) assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") store.AssertNotCalled(t, "TransitionExecutionStatus", @@ -536,7 +606,7 @@ func TestManager_ApproveAndExecute_FourEyesOn_NullCreatorDenied(t *testing.T) { store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetExecutionByID", ctx, "exec-legacy").Return(execution, nil) - _, err := manager.ApproveAndExecute(ctx, "exec-legacy", "someone@example.com", nil) + _, _, err := manager.ApproveAndExecute(ctx, "exec-legacy", "someone@example.com", nil) require.Error(t, err) assert.Contains(t, err.Error(), "predates the dual-control feature") store.AssertExpectations(t) @@ -554,7 +624,7 @@ func TestManager_ApproveAndExecute_FourEyesOn_EmptyActorDenied(t *testing.T) { store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetExecutionByID", ctx, "exec-no-actor").Return(execution, nil) - _, err := manager.ApproveAndExecute(ctx, "exec-no-actor", "", nil) + _, _, err := manager.ApproveAndExecute(ctx, "exec-no-actor", "", nil) require.Error(t, err) assert.Contains(t, err.Error(), "no approver identity could be determined") store.AssertExpectations(t) @@ -578,7 +648,7 @@ func TestManager_ApproveAndExecute_FourEyesOff_AllowsSelfApprove(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-mode-off", approveFromStatuses, "approved", &creatorID).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-fourEyes") - _, err := manager.ApproveAndExecute(ctx, "exec-mode-off", "creator@example.com", &creatorID) + _, _, err := manager.ApproveAndExecute(ctx, "exec-mode-off", "creator@example.com", &creatorID) require.NoError(t, err) store.AssertNotCalled(t, "GetExecutionByID", mock.Anything, mock.Anything) store.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) diff --git a/internal/purchase/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index fc8172cee..12d9af2f3 100644 --- a/internal/purchase/armed_redrive_test.go +++ b/internal/purchase/armed_redrive_test.go @@ -144,7 +144,7 @@ func TestApproveAndExecuteRefusesArmedAzureSavingsPlanRetry(t *testing.T) { store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"pending", "notified"}, "approved") - _, err := mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) + _, _, err := mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) assert.Equal(t, 0, rec.count(), "approving an armed Azure savings-plans retry successor must buy NOTHING; one call here is a second, non-cancelable savings plan (issue #1718)") diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index 1df5c8a42..6880ca7d0 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -30,7 +30,10 @@ type PurchaseManagerInterface interface { // its hash is stored, so a re-read cannot yield an emailable token). // Empty on failure. ApproveExecution(ctx context.Context, execID, token, actor string) (string, error) - ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) + ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + // DirectExecute is ApproveAndExecute for the direct-execute permission; it + // stamps the executed_* audit fields only after the claim is won. + DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) // RunPlannedPurchaseNow forces a pending or paused scheduled purchase to // execute immediately (the "Run now" button), sharing ApproveAndExecute's // 4-eyes-gated, CAS-guarded funnel instead of a bare status flip that diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index df97421c8..d26782975 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -43,7 +43,8 @@ type MockPurchaseManager struct { SendUpcomingPurchaseNotificationsFunc func(ctx context.Context) (*purchase.NotificationResult, error) ProcessMessageFunc func(ctx context.Context, body string) error ApproveExecutionFunc func(ctx context.Context, execID, token, actor string) (string, error) - ApproveAndExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) + ApproveAndExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + DirectExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) RunPlannedPurchaseNowFunc func(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) CancelExecutionFunc func(ctx context.Context, execID, token, actor string) error ReapStuckExecutionsFunc func(ctx context.Context, reapAfter time.Duration) (*purchase.ReapResult, error) @@ -79,11 +80,18 @@ func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, toke return "", nil } -func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (string, error) { +func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { if m.ApproveAndExecuteFunc != nil { return m.ApproveAndExecuteFunc(ctx, execID, actor, transitionedBy) } - return "", nil + return nil, "", nil +} + +func (m *MockPurchaseManager) DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { + if m.DirectExecuteFunc != nil { + return m.DirectExecuteFunc(ctx, execID, actor, transitionedBy) + } + return nil, "", nil } func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) {