From e28eaff7742ffb19cd1042df4b8c7acc17a0a1b4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 20:28:41 +0000 Subject: [PATCH] fix(purchase): return the row from token approve and answer post-claim failures 502 ApproveExecution (the email-link and SQS approve) discarded the row the approve funnel returned, so the token-mode handler could not tell a refusal before the claim from a provider failure after it: every post-claim failure fell through tokenActionError as a bare 500, and a 4-eyes denial from the funnel was a 500 too. ApproveExecution now returns the funnel's row, as ApproveAndExecute and RunPlannedPurchaseNow do: nil when the run never started, the final row otherwise. The token handler maps a failure with a row through the same executeFailure as session approve, direct execute and run-now (502 with execution_id and status for a failed or partially_completed run, 500 for an audit loss, the partial-run email sent from the returned row), and a 4-eyes denial to 403. Refusals before the claim keep tokenActionError. The SQS worker ignores the row. Nothing changes in what is bought or when. Closes #532 Co-Authored-By: Claude Opus 5.5 --- internal/api/authpublic_csrf_guard_test.go | 2 +- internal/api/coverage_extras_test.go | 2 +- .../api/executed_notification_flow_test.go | 4 +- .../api/executed_notification_partial_test.go | 78 +++++++++++++++---- internal/api/execution_scope_test.go | 2 +- internal/api/handler_purchases.go | 33 ++++---- ...andler_purchases_plan_step_approve_test.go | 2 +- internal/api/handler_purchases_test.go | 4 +- internal/api/handler_test.go | 2 +- internal/api/middleware_test.go | 2 +- internal/api/mocks_test.go | 9 ++- internal/api/openapi.yaml | 8 +- internal/api/types.go | 6 +- internal/purchase/approvals.go | 29 +++---- internal/purchase/approvals_test.go | 72 ++++++++++++----- .../purchase/aws_no_token_redrive_test.go | 2 +- internal/purchase/claimed_row_check_test.go | 2 +- internal/purchase/messages.go | 10 +-- ...partial_run_revocation_integration_test.go | 51 ++++++++++++ .../token_rotation_pin_integration_test.go | 4 +- internal/server/interfaces.go | 8 +- internal/testutil/mocks.go | 6 +- 22 files changed, 245 insertions(+), 93 deletions(-) diff --git a/internal/api/authpublic_csrf_guard_test.go b/internal/api/authpublic_csrf_guard_test.go index 2b6ab47c3..e1ffadfcf 100644 --- a/internal/api/authpublic_csrf_guard_test.go +++ b/internal/api/authpublic_csrf_guard_test.go @@ -267,7 +267,7 @@ func purchaseCSRFGuardFixture(action string, viaToken bool) func(t *testing.T) c pm := new(MockPurchaseManager) pm.On("ApproveAndExecute", mock.Anything, csrfGuardExecID, mock.Anything, mock.Anything).Return(exec, "", nil).Maybe() - pm.On("ApproveExecution", mock.Anything, csrfGuardExecID, csrfGuardRawToken, mock.Anything).Return("", nil).Maybe() + pm.On("ApproveExecution", mock.Anything, csrfGuardExecID, csrfGuardRawToken, mock.Anything).Return(nil, "", nil).Maybe() pm.On("CancelExecution", mock.Anything, csrfGuardExecID, csrfGuardRawToken, mock.Anything).Return(nil).Maybe() var m *MockAuthService diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index ee9e8f3dc..861bf98a6 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -101,7 +101,7 @@ func TestHandler_approvePurchase_PurchaseError(t *testing.T) { mockPurchase := new(MockPurchaseManager) mockPurchase.On("ApproveExecution", ctx, execID, "tok", approver). - Return("", errors.New("approval failed")) + Return(nil, "", errors.New("approval failed")) h := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index fb15aa3ef..1228f3cd3 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -114,7 +114,7 @@ func TestExecutedNotification_TokenApprovePath(t *testing.T) { mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(freshToken, nil) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(nil, freshToken, nil) notifier := &recordingExecutedNotifier{} handler := &Handler{ @@ -187,7 +187,7 @@ func TestExecutedNotification_TokenApprovePath_RefetchFailureStillEmailsToken(t mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(freshToken, nil) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(nil, freshToken, nil) notifier := &recordingExecutedNotifier{} handler := &Handler{ diff --git a/internal/api/executed_notification_partial_test.go b/internal/api/executed_notification_partial_test.go index 17ccde409..cf20a1816 100644 --- a/internal/api/executed_notification_partial_test.go +++ b/internal/api/executed_notification_partial_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -139,19 +140,15 @@ func TestExecutedNotification_PartialDirectExecuteEmailsRevocationToken(t *testi assertPartialRunEmailed(t, notifier, row) } -// The token approve returns no row yet, so a token alongside the error is what -// marks a committed partial run; the email comes from the re-fetched row. The -// status mapping of this path is unchanged here. -func TestExecutedNotification_PartialTokenApproveEmailsRevocationToken(t *testing.T) { - ctx := context.Background() - execID := "89012345-8901-8901-8901-89012345abcd" - contact := "contact@example.com" - row := partialRunRow(execID) - +// tokenApproveHandler wires the email-link approve: the caller is the +// account's contact mailbox with no approve-* permission, so approvePurchase +// takes the token branch, whose ApproveExecution returns final, token and +// runErr. The execution is read once, for the request itself: a failed run +// answers from the returned row, never from a re-fetch. +func tokenApproveHandler(ctx context.Context, execID, contact string, final *config.PurchaseExecution, token string, runErr error) (*Handler, *recordingExecutedNotifier, *MockConfigStore) { mockConfig := new(MockConfigStore) exec := approvalTestExec(execID, contact, mockConfig) mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil).Once() - mockConfig.On("GetExecutionByID", ctx, execID).Return(row, nil).Once() mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &contact}, nil) mockAuth := new(MockAuthService) mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() @@ -160,16 +157,69 @@ func TestExecutedNotification_PartialTokenApproveEmailsRevocationToken(t *testin mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(partialRunToken, errPartialRun) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(final, token, runErr) notifier := &recordingExecutedNotifier{} - handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth, emailNotifier: notifier} + return &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth, emailNotifier: notifier}, notifier, mockConfig +} + +// The token approve now gets the row back, so a partial run answers like the +// session approve: emailed from the returned row, then a 502 with its status. +func TestExecutedNotification_PartialTokenApproveEmailsRevocationToken(t *testing.T) { + ctx := context.Background() + execID := "89012345-8901-8901-8901-89012345abcd" + row := partialRunRow(execID) + handler, notifier, mockConfig := tokenApproveHandler(ctx, execID, "contact@example.com", row, partialRunToken, errPartialRun) _, err := handler.approvePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}}, execID, "valid-token") - require.ErrorIs(t, err, errPartialRun) + assertPartialRun502(t, err) assertPartialRunEmailed(t, notifier, row) mockConfig.AssertExpectations(t) } +// Issue #532 slice 3: a token approve maps its failures by cause, as the +// session approve does. Past the claim (a row came back) a provider failure +// is a 502 with the row's status and an audit loss a 500; before it, the +// 4-eyes denial is a 403 and the token and lost-claim refusals keep their +// 4xx. Nothing here committed, so nothing is emailed. +func TestTokenApprove_FailureMappedByCause(t *testing.T) { + ctx := context.Background() + execID := "a1234567-a123-a123-a123-a1234567abcd" + failed := partialRunRow(execID) + failed.Status = "failed" + cases := map[string]struct { + final *config.PurchaseExecution + err error + code int // 0: a plain (500) error, not a client error + status string + }{ + "provider failure after the claim": {failed, errors.New("AWS RI purchase failed"), 502, "failed"}, + "final status not saved": {partialRunRow(execID), fmt.Errorf("%w: terminal save failed; original execution error: %w", config.ErrAuditLoss, errPartialRun), 500, ""}, + "4-eyes denial before the claim": {nil, fmt.Errorf("%w: approver is the requester", purchase.ErrFourEyesDenied), 403, ""}, + "lost claim": {nil, fmt.Errorf("approve: %w", config.ErrExecutionNotInExpectedStatus), 409, ""}, + "token rotated before the claim": {nil, fmt.Errorf("approve: %w", config.ErrApprovalTokenChanged), 403, ""}, + "not started, store error": {nil, errors.New("failed to get execution: db down"), 0, ""}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + handler, notifier, _ := tokenApproveHandler(ctx, execID, "contact@example.com", tc.final, "", tc.err) + _, err := handler.approvePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}}, execID, "valid-token") + require.Error(t, err) + assert.Zero(t, notifier.calls) + ce, ok := IsClientError(err) + if tc.code == 0 { + assert.False(t, ok, "a failure before the run started stays a plain error, got %v", err) + return + } + require.True(t, ok, "expected a client error, got %v", err) + assert.Equal(t, tc.code, ce.code) + if tc.status != "" { + assert.Equal(t, execID, ce.Details()["execution_id"]) + assert.Equal(t, tc.status, ce.Details()["status"]) + } + }) + } +} + func TestExecutedNotification_TokenApproveFailureWithoutTokenSendsNoEmail(t *testing.T) { ctx := context.Background() execID := "90123456-9012-9012-9012-90123456abcd" @@ -186,7 +236,7 @@ func TestExecutedNotification_TokenApproveFailureWithoutTokenSendsNoEmail(t *tes mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return("", errors.New("AWS RI purchase failed")) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(nil, "", errors.New("AWS RI purchase failed")) notifier := &recordingExecutedNotifier{} handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth, emailNotifier: notifier} diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index 50607f08c..7186a2c15 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -389,7 +389,7 @@ func TestApprove_OutOfScopeSessionWithTokenUsesContactGate(t *testing.T) { store := new(MockConfigStore) mockPurchase := new(MockPurchaseManager) h := newScopeTestHandler(t, exec, []string{scopeTestProd.ID}, store, mockPurchase) - mockPurchase.On("ApproveExecution", mock.Anything, scopeTestExecID, "email-token", "operator@example.com").Return("revocation-token", nil) + mockPurchase.On("ApproveExecution", mock.Anything, scopeTestExecID, "email-token", "operator@example.com").Return(nil, "revocation-token", nil) store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) res, err := h.approvePurchase(context.Background(), scopeTestRequest(), scopeTestExecID, "email-token") diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 88f25f872..cd58a09d0 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1094,25 +1094,32 @@ func (h *Handler) executeTokenApprove(ctx context.Context, req *events.LambdaFun // the hash. // approveConstraintsForSession above read a snapshot; the funnel re-runs // it on the row its claim returns, the row it buys (issue #415). - revocationToken, approveErr := h.purchase.ApproveExecution(ctx, execution.ExecutionID, token, actor, h.tokenApproveClaimCheck(tokenSession)) - // A token returned with an error is a partially_completed run: part of it - // committed and is revocable, so it is emailed like a completed run - // (issue #532). Any other error has nothing to email. - if approveErr != nil && revocationToken == "" { - return nil, tokenActionError(approveErr) - } - h.emailTokenApproveOutcome(ctx, req, execution, revocationToken, actor) + final, revocationToken, approveErr := h.purchase.ApproveExecution(ctx, execution.ExecutionID, token, actor, h.tokenApproveClaimCheck(tokenSession)) if approveErr != nil { - return nil, tokenActionError(approveErr) + return nil, h.tokenApproveFailure(ctx, req, execution.ExecutionID, final, revocationToken, actor, approveErr) } + h.emailTokenApproveOutcome(ctx, req, execution, revocationToken, actor) return map[string]string{"status": "completed"}, nil } +// tokenApproveFailure answers a failed token approve (issue #532). Once the +// claim was won (a row came back) it answers like the session approve: a +// partially_completed run gets its executed email, a provider failure is a +// 502 carrying the row's status, an audit loss a 500. A 4-eyes denial is a +// 403 on every path. Any other refusal before the claim (bad, rotated or +// expired token, lost claim) keeps the tokenActionError mapping. +func (h *Handler) tokenApproveFailure(ctx context.Context, req *events.LambdaFunctionURLRequest, executionID string, final *config.PurchaseExecution, revocationToken, actor string, err error) error { + if final == nil && !errors.Is(err, purchase.ErrFourEyesDenied) { + return tokenActionError(err) + } + return h.executeFailure(ctx, req, executionID, "approved", final, revocationToken, actor, err) +} + // emailTokenApproveOutcome sends the executed email for a token approve that -// committed, in full or in part. It re-fetches the execution to pick up the -// FINAL state (status, completed_at, per-rec purchase results) -// ApproveAndExecute wrote -- the pre-approve `execution` struct is stale on -// those fields. Falling back to the stale struct on a re-fetch failure is a +// completed (tokenApproveFailure emails a partial run from the returned row). +// It re-fetches the execution to pick up the FINAL state (status, +// completed_at, per-rec purchase results) ApproveAndExecute wrote -- the +// pre-approve `execution` struct is stale on those fields. Falling back to the stale struct on a re-fetch failure is a // cosmetic degradation of the email content only: revocationToken is already // in hand either way, so the Revoke link is always valid. Best-effort (issue // #291): errors are logged inside sendPurchaseExecutedEmail and never diff --git a/internal/api/handler_purchases_plan_step_approve_test.go b/internal/api/handler_purchases_plan_step_approve_test.go index 85e33210d..72d3bbab7 100644 --- a/internal/api/handler_purchases_plan_step_approve_test.go +++ b/internal/api/handler_purchases_plan_step_approve_test.go @@ -97,7 +97,7 @@ func newTokenApprove(t *testing.T, perms []auth.Permission, plan *config.Purchas mp := new(MockPurchaseManager) // A reached ApproveExecution proves the approval was not refused up front; // it answers with a conflict so the test needs no email or re-fetch wiring. - mp.On("ApproveExecution", mock.Anything, stepExecID, "valid-token", stepContact).Return("", config.ErrExecutionNotInExpectedStatus).Maybe() + mp.On("ApproveExecution", mock.Anything, stepExecID, "valid-token", stepContact).Return(nil, "", config.ErrExecutionNotInExpectedStatus).Maybe() return &tokenApproveFixture{ handler: &Handler{purchase: mp, config: mockConfig, auth: mockAuth}, purchase: mp, diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 97b511147..cf4a52e4c 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -74,7 +74,7 @@ func TestHandler_approvePurchase(t *testing.T) { mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", approver).Return("", nil) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", approver).Return(nil, "", nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} @@ -291,7 +291,7 @@ func TestHandler_approvePurchase_AcceptsContactEmailSession(t *testing.T) { mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contactEmail).Return("", nil) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contactEmail).Return(nil, "", nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 9905c1304..e02495122 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -877,7 +877,7 @@ func TestHandler_HandleRequest_ApprovePurchase(t *testing.T) { mockAuth.On("HasPermissionAPI", mock.Anything, "", "approve-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", mock.Anything, execID, "token123", approver).Return("", nil) + mockPurchase.On("ApproveExecution", mock.Anything, execID, "token123", approver).Return(nil, "", nil) handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth} diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 65635ce4f..ed72eeaf7 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -726,7 +726,7 @@ func TestTokenApprove_SessionRequiresCSRF(t *testing.T) { t.Run("valid CSRF header", func(t *testing.T) { mockPurchase := new(MockPurchaseManager) handler := newHandler(new(MockConfigStore), mockPurchase) - mockPurchase.On("ApproveExecution", ctx, execID, "email-token", contactEmail).Return("", nil) + mockPurchase.On("ApproveExecution", ctx, execID, "email-token", contactEmail).Return(nil, "", nil) req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{"authorization": "Bearer sess-tok", "x-csrf-token": "csrf-ok"}, } diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 3cf4f38eb..b840c4c9c 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -78,12 +78,13 @@ func (m *MockPurchaseManager) claimedRowRefusal(ctx context.Context, check purch return nil, nil } -func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (string, error) { - if _, err := m.claimedRowRefusal(ctx, check); err != nil { - return "", err +func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) { + if refused, err := m.claimedRowRefusal(ctx, check); err != nil { + return refused, "", err } args := m.Called(ctx, execID, token, actor) - return args.String(0), args.Error(1) + exec, _ := args.Get(0).(*config.PurchaseExecution) + return exec, args.String(1), args.Error(2) } func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) { diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index d3611e134..be16dcd94 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -991,7 +991,7 @@ 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, and in session mode a + concurrent change answers the same way, and in either mode a lost claim answers 409 as well). content: application/json: @@ -1015,9 +1015,9 @@ paths: $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 + Session and token mode. 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. A partially_completed run is revocable: the executed email with its revocation link is sent as for a completed run. Check the execution in History before diff --git a/internal/api/types.go b/internal/api/types.go index 82de02f27..c6ed2974e 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -136,7 +136,9 @@ type BreakdownValue struct { // ApproveExecution, ApproveAndExecute and RunPlannedPurchaseNow return the // raw revocation token minted on a successful execute (issue #103: only its // hash is stored, so callers can no longer recover a raw, emailable token by -// re-reading the execution from the store). Empty on failure. +// re-reading the execution from the store). Empty on failure. All three +// also return the row with its final status, also on an execution error, and +// a nil row when the run never started (issue #532). // // `check` (every execute entry point) re-runs the caller's permission // Constraints against the row the claim CAS returned, the row that is @@ -144,7 +146,7 @@ type BreakdownValue struct { // pre-CAS check reads an earlier snapshot, which a repricing or a // recommendation attach can change before the CAS. type PurchaseManagerInterface interface { - ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (string, error) + ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) // DirectExecute is ApproveAndExecute for the direct-execute permission; it // stamps the executed_* audit fields only after the claim is won. diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 530760d2c..c38bf046b 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -48,28 +48,31 @@ const directExecuteSkipReason = "direct-execute permission" // check is the caller's ClaimedRowCheck, run against the claimed row before // any provider call (issue #415); the SQS approve worker passes nil. // -// Returns the raw revocation token minted by ApproveAndExecute on success -// (see that function's doc comment for why this must be a return value -// rather than something the caller re-fetches from the DB). Empty on -// failure, and best-effort empty if the mint itself failed after an +// Returns the same row and token as ApproveAndExecute (issue #532): the row +// carries the run's final status, also on an execution error, and is nil when +// the run never started (bad token, orphan, 4-eyes denial, lost claim), so the +// caller can tell a refusal from a provider failure after the claim. The raw +// revocation token minted on success is a return value rather than something +// the caller re-fetches from the DB (see transitionApproveAndExecute). It is +// empty on failure, and best-effort empty if the mint itself failed after an // otherwise-successful approve. A partially_completed run returns its token -// together with its error (issue #532), so a non-empty token with an error -// means part of the purchase committed and is revocable. -func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, actor string, check ClaimedRowCheck) (string, error) { +// together with its error, so a non-empty token with an error means part of +// the purchase committed and is revocable. +func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, actor string, check ClaimedRowCheck) (*config.PurchaseExecution, string, error) { t0 := time.Now() logging.Infof("purchase[%s]: ApproveExecution entry (auth=token actor=%q)", executionID, maskActor(actor)) execution, err := m.config.GetExecutionByID(ctx, executionID) if errors.Is(err, config.ErrNotFound) { - return "", fmt.Errorf("execution not found: %s", executionID) + return nil, "", fmt.Errorf("execution not found: %s", executionID) } if err != nil { - return "", fmt.Errorf("failed to get execution: %w", err) + return nil, "", fmt.Errorf("failed to get execution: %w", err) } // Validate token and TTL (Finding #4 + issue #397). if tokErr := ValidateApprovalToken(execution, token); tokErr != nil { - return "", tokErr + return nil, "", tokErr } // Preflight guard (issue #609): reject non-AWS orphan executions before @@ -77,7 +80,7 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto if checkErr := OrphanExecutionError(execution); checkErr != nil { logging.Errorf("purchase[%s]: ApproveExecution preflight rejected after %s: %v", executionID, time.Since(t0), checkErr) - return "", checkErr + return nil, "", checkErr } // Token/SQS path: no authenticated session UUID is available, so the @@ -85,7 +88,7 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto // The token check above read a snapshot; the claim pins the same token // inside its status CAS so a re-notification that rotated it since cannot // win the claim with the old one (issue #1190). - _, revocationToken, err := m.transitionApproveAndExecute(ctx, executionID, actor, nil, []string{"pending", "notified"}, "", token, check) + final, revocationToken, err := m.transitionApproveAndExecute(ctx, executionID, actor, nil, []string{"pending", "notified"}, "", token, check) if err != nil { logging.Errorf("purchase[%s]: ApproveExecution (token path) failed after %s: %v", executionID, time.Since(t0), err) @@ -93,7 +96,7 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto logging.Infof("purchase[%s]: ApproveExecution (token path) completed in %s", executionID, time.Since(t0)) } - return revocationToken, err + return final, revocationToken, err } // maskActor masks an actor email/username for safe log emission. diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index f69cbaa25..c59f45359 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -102,8 +102,10 @@ func TestManager_ApproveExecution_Success(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) + final, _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) require.NoError(t, err) + require.NotNil(t, final, "the token approve returns the row it ran (issue #532)") + assert.Equal(t, "completed", final.Status) store.AssertExpectations(t) sender.AssertExpectations(t) } @@ -131,7 +133,7 @@ func TestManager_ApproveExecution_StampsApprovedBy(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "operator@example.com", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "operator@example.com", nil) require.NoError(t, err) // Two SavePurchaseExecution calls land on the same pointer (the @@ -166,7 +168,7 @@ func TestManager_ApproveExecution_NotifiedStatus(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) sender.AssertExpectations(t) @@ -185,7 +187,7 @@ func TestManager_ApproveExecution_InvalidToken(t *testing.T) { store.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - _, err := manager.ApproveExecution(ctx, "exec-123", "invalid-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-123", "invalid-token", "", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid approval token") // Critically: TransitionExecutionStatus and SavePurchaseExecution must @@ -207,7 +209,7 @@ func TestManager_ApproveExecution_EmptyToken(t *testing.T) { } store.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - _, err := manager.ApproveExecution(ctx, "exec-123", "", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-123", "", "", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid approval token") store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) @@ -219,7 +221,7 @@ func TestManager_ApproveExecution_NotFound(t *testing.T) { store.On("GetExecutionByID", ctx, "exec-123").Return(nil, fmt.Errorf("%w: execution exec-123", config.ErrNotFound)) - _, err := manager.ApproveExecution(ctx, "exec-123", "token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-123", "token", "", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "execution not found") store.AssertExpectations(t) @@ -231,7 +233,7 @@ func TestManager_ApproveExecution_GetError(t *testing.T) { store.On("GetExecutionByID", ctx, "exec-123").Return(nil, errors.New("database error")) - _, err := manager.ApproveExecution(ctx, "exec-123", "token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-123", "token", "", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to get execution") store.AssertExpectations(t) @@ -255,9 +257,10 @@ func TestManager_ApproveExecution_TransitionFails(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil), "valid-token"). Return(nil, errors.New(`execution exec-123 cannot transition from "canceled" to "approved"`)) - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) + final, _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "cannot transition") + assert.Nil(t, final, "a lost claim never started a run, so no row comes back") // Execute chain must not run after a failed transition. store.AssertNotCalled(t, "GetPurchasePlan", mock.Anything, mock.Anything) store.AssertExpectations(t) @@ -469,6 +472,39 @@ func TestManager_ApproveAndExecute_ReturnsRowOnExecutionFailure(t *testing.T) { assert.Equal(t, "failed", final.Status) } +// TestManager_ApproveExecution_ReturnsRowOnExecutionFailure is the token-path +// twin (issue #532 slice 3): the email-link approve also returns the claimed +// row with its final status, so the handler answers 502, not a bare 500. +func TestManager_ApproveExecution_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: allowFailedNotification(new(MockEmailSender)), providerFactory: factory} + + execution := &config.PurchaseExecution{ + ExecutionID: "exec-token-fail-after-claim", + Status: "pending", + ApprovalToken: config.HashApprovalToken("valid-token"), + } + updated := &config.PurchaseExecution{ + ExecutionID: "exec-token-fail-after-claim", + Status: "running", + ApprovalToken: config.HashApprovalToken("valid-token"), + Recommendations: approvalTestRecs(), + } + store.On("GetExecutionByID", ctx, "exec-token-fail-after-claim").Return(execution, nil) + store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-token-fail-after-claim", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) + store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + + final, token, err := manager.ApproveExecution(ctx, "exec-token-fail-after-claim", "valid-token", "", nil) + require.Error(t, err) + assert.Empty(t, token) + require.NotNil(t, final) + assert.Equal(t, "exec-token-fail-after-claim", final.ExecutionID) + assert.Equal(t, "failed", final.Status) +} + // ─── enforceFourEyesPolicy at the ApproveAndExecute choke point ─────────────── // (issue #1005 / PR #1500 adversarial review) // @@ -702,7 +738,7 @@ func TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied(t *tes store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetUserEmailByID", ctx, creatorID).Return(tc.creatorEmail, nil) - _, err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", tc.actorEmail, nil) + _, _, err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", tc.actorEmail, nil) require.ErrorIs(t, err, ErrFourEyesDenied) assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", @@ -742,7 +778,7 @@ func TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed(t *testing.T store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-sqs-diff", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-fourEyes") - _, err := manager.ApproveExecution(ctx, "exec-sqs-diff", "valid-token", "approver@example.com", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-sqs-diff", "valid-token", "approver@example.com", nil) require.NoError(t, err) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, "approver@example.com", *updated.ApprovedBy) @@ -783,7 +819,7 @@ func TestManager_ApproveExecution_FourEyesOn_UnresolvableCreator_Denied(t *testi // pgx.ErrNoRows -- a deleted/nonexistent creator user row. store.On("GetUserEmailByID", ctx, creatorID).Return("", nil) - _, err := manager.ApproveExecution(ctx, "exec-sqs-unresolvable", "valid-token", "approver@example.com", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-sqs-unresolvable", "valid-token", "approver@example.com", nil) require.Error(t, err) assert.Contains(t, err.Error(), "creator's account could not be resolved") store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", @@ -800,7 +836,7 @@ func TestManager_ApproveExecution_FourEyesOn_UnresolvableCreator_Denied(t *testi store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetUserEmailByID", ctx, creatorID).Return("", lookupErr) - _, err := manager.ApproveExecution(ctx, "exec-sqs-unresolvable", "valid-token", "approver@example.com", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-sqs-unresolvable", "valid-token", "approver@example.com", nil) require.Error(t, err) assert.Contains(t, err.Error(), "failed to resolve creator identity") store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned", @@ -1050,7 +1086,7 @@ func TestManager_ApproveExecution_ExpiredToken(t *testing.T) { } store.On("GetExecutionByID", ctx, "exec-expired").Return(execution, nil) - _, err := manager.ApproveExecution(ctx, "exec-expired", "valid-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-expired", "valid-token", "", nil) require.Error(t, err) assert.Contains(t, err.Error(), "expired") // Token validation passes but the expiry check fires — TransitionExecutionStatus @@ -1085,7 +1121,7 @@ func TestManager_ApproveExecution_ValidTokenWithinTTL(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-live", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-live") - _, err := manager.ApproveExecution(ctx, "exec-live", "valid-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-live", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) } @@ -1116,7 +1152,7 @@ func TestManager_ApproveExecution_NilExpiresAt_LegacyRow(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-legacy", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-legacy") - _, err := manager.ApproveExecution(ctx, "exec-legacy", "valid-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-legacy", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) } @@ -1142,7 +1178,7 @@ func TestManager_ApproveExecution_NonAWSOrphanReturnsError(t *testing.T) { } store.On("GetExecutionByID", ctx, "exec-orphan").Return(execution, nil) - _, err := manager.ApproveExecution(ctx, "exec-orphan", "valid-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-orphan", "valid-token", "", nil) require.Error(t, err) assert.Contains(t, err.Error(), "no longer exists") assert.Contains(t, err.Error(), "azure") @@ -1178,7 +1214,7 @@ func TestManager_ApproveExecution_AWSOrphanFallsThrough(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-aws-ambient", approveFromStatuses, "running", (*string)(nil), "valid-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-aws") - _, err := manager.ApproveExecution(ctx, "exec-aws-ambient", "valid-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-aws-ambient", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) sender.AssertExpectations(t) @@ -1298,7 +1334,7 @@ func TestApproveExecution_MintsRevocationToken(t *testing.T) { store.On("TransitionExecutionStatusTokenPinned", ctx, "exec-rotate", approveFromStatuses, "running", (*string)(nil), "pre-rotate-token").Return(updated, nil) stubExecuteChain(t, store, sender, "plan-rotate") - _, err := manager.ApproveExecution(ctx, "exec-rotate", "pre-rotate-token", "", nil) + _, _, err := manager.ApproveExecution(ctx, "exec-rotate", "pre-rotate-token", "", nil) require.NoError(t, err) // After approve, rotateApprovalToken must have stamped a fresh token onto diff --git a/internal/purchase/aws_no_token_redrive_test.go b/internal/purchase/aws_no_token_redrive_test.go index 3193d7fbe..3b754a113 100644 --- a/internal/purchase/aws_no_token_redrive_test.go +++ b/internal/purchase/aws_no_token_redrive_test.go @@ -182,7 +182,7 @@ func TestApproveAndExecuteRefusesAWSNoTokenRetrySuccessor(t *testing.T) { approve func(m *Manager, id string) error }{ {"token", nil, rawToken, func(m *Manager, id string) error { - _, err := m.ApproveExecution(context.Background(), id, rawToken, "approver@example.com", nil) + _, _, err := m.ApproveExecution(context.Background(), id, rawToken, "approver@example.com", nil) return err }}, {"session", &sessionUser, "", func(m *Manager, id string) error { diff --git a/internal/purchase/claimed_row_check_test.go b/internal/purchase/claimed_row_check_test.go index 22c6c4ce6..6080712c1 100644 --- a/internal/purchase/claimed_row_check_test.go +++ b/internal/purchase/claimed_row_check_test.go @@ -81,7 +81,7 @@ func claimEntryPoints() []claimEntryPoint { return row, err }, ""}, {"token approve", []string{"pending", "notified"}, func(m *Manager, id string, c ClaimedRowCheck) (*config.PurchaseExecution, error) { - _, err := m.ApproveExecution(ctx, id, claimCheckToken, actor, c) + _, _, err := m.ApproveExecution(ctx, id, claimCheckToken, actor, c) return nil, err }, claimCheckToken}, } diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go index 7d0ca6b24..362f0480a 100644 --- a/internal/purchase/messages.go +++ b/internal/purchase/messages.go @@ -176,11 +176,11 @@ func (m *Manager) handleApproveMessage(ctx context.Context, msg *AsyncMessage) e if err := m.verifyAsyncApprovalActor(ctx, msg); err != nil { return err } - // The SQS worker sends no email itself, so the minted revocation token - // (issue #103) has no consumer here; only the error matters. It carries - // no session, so there are no permission Constraints to re-check against - // the claimed row (nil ClaimedRowCheck, issue #415). - _, err := m.ApproveExecution(ctx, msg.ExecutionID, msg.Token, msg.ActorEmail, nil) + // The SQS worker sends no email itself, so the final row and the minted + // revocation token (issue #103) have no consumer here; only the error + // matters. It carries no session, so there are no permission Constraints + // to re-check against the claimed row (nil ClaimedRowCheck, issue #415). + _, _, err := m.ApproveExecution(ctx, msg.ExecutionID, msg.Token, msg.ActorEmail, nil) return err } diff --git a/internal/purchase/partial_run_revocation_integration_test.go b/internal/purchase/partial_run_revocation_integration_test.go index f58777b52..bf509443d 100644 --- a/internal/purchase/partial_run_revocation_integration_test.go +++ b/internal/purchase/partial_run_revocation_integration_test.go @@ -65,3 +65,54 @@ func TestApproveAndExecute_FailedRunMintsNoToken(t *testing.T) { assert.Zero(t, f.purchaseCount()) assert.True(t, config.ApprovalTokenMatches(rowStatus(ctx, t, f, row.ExecutionID).ApprovalToken, "approval-tok")) } + +// Issue #532 slice 3: the token approve returns the funnel's row, so its +// caller can answer a failure after the claim with the row's status. A +// partial run returns the partially_completed row with its revocation token, +// a failed run the failed row, and a claim that was never won no row at all. +func TestApproveExecution_ReturnsTheFinalRow(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + allowFailedNotification(f.manager.email.(*MockEmailSender)) + + t.Run("partial run", func(t *testing.T) { + root := f.saveRootExecution(ctx, t, 3) + root.ApprovalToken = config.HashApprovalToken("approval-tok") + require.NoError(t, f.store.SavePurchaseExecution(ctx, root)) + + final, token, err := f.manager.ApproveExecution(ctx, root.ExecutionID, "approval-tok", "approver@example.invalid", nil) + require.Error(t, err) + require.NotNil(t, final, "a run that started returns its row") + assert.Equal(t, root.ExecutionID, final.ExecutionID) + assert.Equal(t, config.StatusPartiallyCompleted, final.Status) + assert.Equal(t, config.StatusPartiallyCompleted, rowStatus(ctx, t, f, root.ExecutionID).Status, "the returned status is the stored one") + require.NotEmpty(t, token) + assert.True(t, config.ApprovalTokenMatches(rowStatus(ctx, t, f, root.ExecutionID).ApprovalToken, token)) + }) + + t.Run("failed run, then a second approve that loses the claim", func(t *testing.T) { + broken := f.accounts[1].ID + row := &config.PurchaseExecution{ + PlanID: f.plan.ID, ExecutionID: uuid.New().String(), Status: "notified", StepNumber: 3, + ScheduledDate: time.Now(), CloudAccountID: &broken, ApprovalToken: config.HashApprovalToken("approval-tok"), + Recommendations: []config.RecommendationRecord{withAccount(rampStepRecommendation()[0], broken)}, + } + require.NoError(t, f.store.SavePurchaseExecution(ctx, row)) + bought := f.purchaseCount() + + final, token, err := f.manager.ApproveExecution(ctx, row.ExecutionID, "approval-tok", "approver@example.invalid", nil) + require.Error(t, err) + require.NotNil(t, final) + assert.Equal(t, "failed", final.Status) + assert.Equal(t, "failed", rowStatus(ctx, t, f, row.ExecutionID).Status) + assert.Empty(t, token) + + // Nothing was bought, so the approval token is unchanged and still + // validates; the claim CAS is what refuses the second approve. + again, token, err := f.manager.ApproveExecution(ctx, row.ExecutionID, "approval-tok", "approver@example.invalid", nil) + require.ErrorIs(t, err, config.ErrExecutionNotInExpectedStatus) + assert.Nil(t, again, "a lost claim never started a run, so no row comes back") + assert.Empty(t, token) + assert.Equal(t, bought, f.purchaseCount(), "neither approve bought anything") + }) +} diff --git a/internal/purchase/token_rotation_pin_integration_test.go b/internal/purchase/token_rotation_pin_integration_test.go index 8b148e034..10da13b91 100644 --- a/internal/purchase/token_rotation_pin_integration_test.go +++ b/internal/purchase/token_rotation_pin_integration_test.go @@ -82,7 +82,7 @@ func TestApproveExecution_TokenRotatedBeforeClaimIsRefused(t *testing.T) { stubUnreachableProvider(factory) manager := &Manager{config: &rotatingStore{PostgresStore: store, t: t}, email: quietSender(), providerFactory: factory} - revocationToken, err := manager.ApproveExecution(ctx, row.ExecutionID, rotOldToken, "approver@example.invalid", nil) + _, revocationToken, err := manager.ApproveExecution(ctx, row.ExecutionID, rotOldToken, "approver@example.invalid", nil) require.ErrorIs(t, err, config.ErrApprovalTokenChanged, "the revoked token must lose the claim") assert.Empty(t, revocationToken) @@ -99,7 +99,7 @@ func TestApproveExecution_TokenRotatedBeforeClaimIsRefused(t *testing.T) { stubUnreachableProvider(factory) manager := &Manager{config: store, email: quietSender(), providerFactory: factory} - _, err := manager.ApproveExecution(ctx, row.ExecutionID, rotOldToken, "approver@example.invalid", nil) + _, _, err := manager.ApproveExecution(ctx, row.ExecutionID, rotOldToken, "approver@example.invalid", nil) require.NotErrorIs(t, err, config.ErrApprovalTokenChanged) assert.NotEqual(t, "notified", statusInDB(ctx, t, store, row.ExecutionID).Status, "the claim was won and the run started") diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index 2a072b0f4..b771b9569 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -31,9 +31,11 @@ type PurchaseManagerInterface interface { // ApproveExecution, ApproveAndExecute and RunPlannedPurchaseNow return the // raw revocation token minted on a successful execute (issue #103: only // its hash is stored, so a re-read cannot yield an emailable token). - // Empty on failure. Each takes the caller's ClaimedRowCheck, run against - // the claimed row before any provider call (issue #415). - ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (string, error) + // Empty on failure. Each also returns the row with its final status, nil + // when the run never started (issue #532), and takes the caller's + // ClaimedRowCheck, run against the claimed row before any provider call + // (issue #415). + ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) // DirectExecute is ApproveAndExecute for the direct-execute permission; it // stamps the executed_* audit fields only after the claim is won. diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index 646a8155b..6f5c16e46 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -51,7 +51,7 @@ type MockPurchaseManager struct { ProcessScheduledPurchasesFunc func(ctx context.Context) (*purchase.ProcessResult, error) 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, check purchase.ClaimedRowCheck) (string, error) + ApproveExecutionFunc func(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) ApproveAndExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) DirectExecuteFunc func(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) RunPlannedPurchaseNowFunc func(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) @@ -82,11 +82,11 @@ func (m *MockPurchaseManager) ProcessMessage(ctx context.Context, body string) e return nil } -func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (string, error) { +func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) { if m.ApproveExecutionFunc != nil { return m.ApproveExecutionFunc(ctx, execID, token, actor, check) } - return "", nil + return nil, "", nil } func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) {