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) {