diff --git a/internal/api/claimed_row_check.go b/internal/api/claimed_row_check.go new file mode 100644 index 000000000..25cea8e28 --- /dev/null +++ b/internal/api/claimed_row_check.go @@ -0,0 +1,80 @@ +package api + +import ( + "context" + "fmt" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" +) + +// Post-claim Constraints re-checks (issue #415). +// +// Every synchronous execute entry point checks the caller's permission +// Constraints (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) +// against a row read by GetExecutionByID, then calls the purchase funnel, +// which buys the row its claim CAS returns. A repricing, an edit or a plan +// step recommendation attach landing between that read and the CAS would be +// bought unchecked. Each check below re-runs the same Constraints the entry +// point already ran, on the claimed row, before any provider call. The +// pre-CAS check stays: it refuses an over-cap request on an unchanged row +// without touching the row. A post-claim refusal ends the row "failed" with +// no_submission set (see purchase.ClaimedRowCheck). + +// runNowClaimCheck is runPlannedPurchase's execute:purchases Constraints +// check, against the claimed row. +func (h *Handler) runNowClaimCheck(session *Session) purchase.ClaimedRowCheck { + return func(ctx context.Context, claimed *config.PurchaseExecution) error { + return h.enforcePurchaseConstraints(ctx, session, "execute", claimed.Recommendations) + } +} + +// sessionApproveClaimCheck is approvePurchaseViaSession's approve-any / +// approve-own check, against the claimed row. +func (h *Handler) sessionApproveClaimCheck(session *Session) purchase.ClaimedRowCheck { + return func(ctx context.Context, claimed *config.PurchaseExecution) error { + return h.authorizeSessionApprove(ctx, session, claimed) + } +} + +// tokenApproveClaimCheck is approveViaToken's Constraints check (including the +// plan step cap for an approver with no approve verb), against the claimed +// row. session may be nil, as on the pre-CAS check. +func (h *Handler) tokenApproveClaimCheck(session *Session) purchase.ClaimedRowCheck { + return func(ctx context.Context, claimed *config.PurchaseExecution) error { + return h.approveConstraintsForSession(ctx, session, claimed) + } +} + +// directExecuteClaimCheck is the direct-execute path's execute:purchases and +// execute-any / execute-own Constraints checks, against the claimed row. The +// creator comes from the claimed row: an execute-own session must still be +// its creator. +func (h *Handler) directExecuteClaimCheck(session *Session) purchase.ClaimedRowCheck { + return func(ctx context.Context, claimed *config.PurchaseExecution) error { + if err := h.enforcePurchaseConstraints(ctx, session, "execute", claimed.Recommendations); err != nil { + return err + } + return h.authorizeSessionExecuteDirect(ctx, session, derefStringOrEmpty(claimed.CreatedByUserID), claimed.Recommendations) + } +} + +// claimedRowRefusalError maps a purchase.ErrClaimedRowRefused failure. A +// Constraints denial keeps its status code (403, or 400 for a zero-commitment +// batch) and says the row was not bought; a failure to evaluate the check +// (an auth backend error) stays a server error. Either way the row already +// ended failed and nothing was bought. +func claimedRowRefusalError(final *config.PurchaseExecution, err error) error { + ce, ok := IsClientError(err) + if !ok { + return fmt.Errorf("purchase refused after claim, nothing was bought: %w", err) + } + details := map[string]any{} + if final != nil { + details["execution_id"] = final.ExecutionID + details["status"] = final.Status + } + return NewClientErrorWithDetails(ce.code, + "the purchase changed after it was authorized and no longer passes your permission checks, so nothing was bought and it is now marked failed: "+ce.message, + details) +} diff --git a/internal/api/claimed_row_check_test.go b/internal/api/claimed_row_check_test.go new file mode 100644 index 000000000..abe82788d --- /dev/null +++ b/internal/api/claimed_row_check_test.go @@ -0,0 +1,245 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #415, handler side: each execute entry point checks Constraints on the +// row GetExecutionByID returned, then hands the purchase funnel a +// ClaimedRowCheck that re-runs them on the row the claim CAS returns. +// MockPurchaseManager.ClaimedRow plays that CAS-returned row (see its doc); +// the Constraints are decided by the real auth matcher (grantPermissions). +// The funnel side (provider never called, row failed) is pinned in +// internal/purchase/claimed_row_check_test.go and, against real Postgres, in +// handler_purchases_claimed_row_integration_test.go. + +const ( + claimCap = 1000.0 + claimUnder = 400.0 // what the handler's pre-CAS check saw + claimRepriced = 5000.0 // what the CAS returned +) + +func claimRecs(upfront float64) []config.RecommendationRecord { + return []config.RecommendationRecord{{ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: upfront}} +} + +func cappedPurchasePerm(action string) auth.Permission { + return auth.Permission{Action: action, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: claimCap}} +} + +// repriced returns a copy of exec whose recommendations cost upfront. +func repriced(exec *config.PurchaseExecution, upfront float64) *config.PurchaseExecution { + c := *exec + c.Recommendations = claimRecs(upfront) + return &c +} + +// requireClaimedRowRefused asserts the 403 a post-claim refusal maps to and +// that the handler handed the funnel exactly one non-nil check. +func requireClaimedRowRefused(t *testing.T, mp *MockPurchaseManager, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.message, "nothing was bought") + assert.Contains(t, ce.message, "constraints") + assert.Equal(t, "failed", ce.Details()["status"]) + requireOneCheck(t, mp) +} + +func requireOneCheck(t *testing.T, mp *MockPurchaseManager) { + t.Helper() + require.Len(t, mp.Checks, 1) + require.NotNil(t, mp.Checks[0], "every session entry point must pass a ClaimedRowCheck") +} + +// --- Run now --------------------------------------------------------------- + +func runNowClaimHarness(t *testing.T, mp *MockPurchaseManager, claimedUpfront float64) error { + t.Helper() + ctx := context.Background() + execID := "11111111-1111-1111-1111-111111111111" + snapshot := &config.PurchaseExecution{ExecutionID: execID, Status: "pending", Recommendations: claimRecs(claimUnder)} + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(snapshot, nil) + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "capped-token").Return(&Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "capped@example.com"}, nil) + mockAuth.grantPermissions([]auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + cappedPurchasePerm(auth.ActionExecute), + }) + mp.ClaimedRow = repriced(snapshot, claimedUpfront) + mp.On("RunPlannedPurchaseNow", ctx, execID, mock.Anything, mock.Anything). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "completed"}, "", nil).Maybe() + handler := &Handler{purchase: mp, config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer capped-token"}} + _, err := handler.runPlannedPurchase(ctx, req, execID) + return err +} + +func TestClaimedRowCheck_RunNow_RepricedRowRefused(t *testing.T) { + mp := &MockPurchaseManager{} + err := runNowClaimHarness(t, mp, claimRepriced) + requireClaimedRowRefused(t, mp, err) + mp.AssertNotCalled(t, "RunPlannedPurchaseNow", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestClaimedRowCheck_RunNow_UnmodifiedRowRuns(t *testing.T) { + mp := &MockPurchaseManager{} + err := runNowClaimHarness(t, mp, claimUnder) + require.NoError(t, err) + requireOneCheck(t, mp) + mp.AssertNumberOfCalls(t, "RunPlannedPurchaseNow", 1) +} + +// --- Session approve ------------------------------------------------------- + +func sessionApproveClaimHarness(t *testing.T, mp *MockPurchaseManager, claimedUpfront float64) error { + t.Helper() + ctx := context.Background() + execID := "abcdefab-cdef-abcd-efab-cdefabcdef01" + approver := "approver@example.com" + snapshot := &config.PurchaseExecution{ExecutionID: execID, Status: "pending", Recommendations: claimRecs(claimUnder)} + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", ctx, execID).Return(snapshot, nil) + mockConfig.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", Email: approver}, nil) + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + mockAuth.grantPermissions([]auth.Permission{cappedPurchasePerm(auth.ActionApproveAny)}) + mp.ClaimedRow = repriced(snapshot, claimedUpfront) + mp.On("ApproveAndExecute", ctx, execID, approver, mock.Anything).Return(nil, "", nil).Maybe() + handler := &Handler{purchase: mp, config: mockConfig, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}} + _, err := handler.approvePurchase(ctx, req, execID, "") + return err +} + +func TestClaimedRowCheck_SessionApprove_RepricedRowRefused(t *testing.T) { + mp := &MockPurchaseManager{} + err := sessionApproveClaimHarness(t, mp, claimRepriced) + requireClaimedRowRefused(t, mp, err) + mp.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestClaimedRowCheck_SessionApprove_UnmodifiedRowRuns(t *testing.T) { + mp := &MockPurchaseManager{} + err := sessionApproveClaimHarness(t, mp, claimUnder) + require.NoError(t, err) + requireOneCheck(t, mp) + mp.AssertNumberOfCalls(t, "ApproveAndExecute", 1) +} + +// --- Token (email-link) approve --------------------------------------------- + +// The approver is the account's contact_email holding a capped approve-own on +// a plan-step row; the plan cap variant holds no verb at all. +func TestClaimedRowCheck_TokenApprove(t *testing.T) { + exec, total := planStepExecution(t) + overCap := func(e *config.PurchaseExecution) *config.PurchaseExecution { + c := *e + c.Recommendations = append([]config.RecommendationRecord(nil), e.Recommendations...) + c.Recommendations[0].Count *= 10 + c.Recommendations[0].UpfrontCost = total * 10 + return &c + } + for _, tc := range []struct { + name string + perms []auth.Permission + plan *config.PurchasePlan + }{ + {"approve-own cap", []auth.Permission{cappedPerm(auth.ActionApproveOwn, total)}, autoPlan(nil)}, + {"approve-any cap", []auth.Permission{cappedPerm(auth.ActionApproveAny, total)}, autoPlan(nil)}, + {"no verb, plan step cap", nil, autoPlan(&total)}, + } { + t.Run(tc.name+"/repriced row refused", func(t *testing.T) { + f := newTokenApprove(t, tc.perms, tc.plan) + f.purchase.ClaimedRow = overCap(exec) + requireClaimedRowRefusedAnyBody(t, f.purchase, f.approve(exec)) + assert.Zero(t, approveCalls(f.purchase), "the purchase must not run") + }) + t.Run(tc.name+"/unmodified row runs", func(t *testing.T) { + f := newTokenApprove(t, tc.perms, tc.plan) + f.purchase.ClaimedRow = exec + requireApproveReached(t, f.purchase, f.approve(exec)) + requireOneCheck(t, f.purchase) + }) + } +} + +// requireClaimedRowRefusedAnyBody is requireClaimedRowRefused for the token +// path, whose error carries no row (ApproveExecution returns none) and whose +// plan-cap refusal names the step cap rather than "constraints". +func requireClaimedRowRefusedAnyBody(t *testing.T, mp *MockPurchaseManager, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.message, "nothing was bought") + requireOneCheck(t, mp) +} + +// --- Direct execute ---------------------------------------------------------- + +func directExecuteClaimHarness(t *testing.T, mp *MockPurchaseManager, claimedUpfront float64) error { + t.Helper() + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "direct@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantPermissions([]auth.Permission{ + cappedPurchasePerm(auth.ActionExecute), + cappedPurchasePerm(auth.ActionExecuteAny), + }) + setupDirectExecMocks(ctx, mockStore) + // directExecRecBody's rec-1 is priced from the stored set at 500, under the cap. + mp.ClaimedRow = &config.PurchaseExecution{ + Status: "pending", + Recommendations: []config.RecommendationRecord{{ + ID: "rec-1", Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: claimedUpfront, + }}, + } + mp.On("DirectExecute", ctx, mock.AnythingOfType("string"), session.Email, mock.Anything).Return(nil, "", nil).Maybe() + handler := &Handler{config: mockStore, auth: mockAuth, purchase: mp} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: directExecRecBody} + _, err := handler.executePurchase(ctx, req) + return err +} + +func TestClaimedRowCheck_DirectExecute_RepricedRowRefused(t *testing.T) { + mp := &MockPurchaseManager{} + err := directExecuteClaimHarness(t, mp, claimRepriced) + requireClaimedRowRefused(t, mp, err) + mp.AssertNotCalled(t, "DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestClaimedRowCheck_DirectExecute_UnmodifiedRowRuns(t *testing.T) { + mp := &MockPurchaseManager{} + err := directExecuteClaimHarness(t, mp, 500) + require.NoError(t, err) + requireOneCheck(t, mp) + mp.AssertNumberOfCalls(t, "DirectExecute", 1) +} + +// --- Error mapping ------------------------------------------------------------- + +// A check that could not be evaluated (no clientError inside) is a server +// error, not a 403 or a 502: the row failed closed and nothing was bought. +func TestClaimedRowRefusalError_NonClientCauseIsServerError(t *testing.T) { + err := claimedRowRefusalError(nil, assert.AnError) + _, ok := IsClientError(err) + assert.False(t, ok) + assert.ErrorIs(t, err, assert.AnError) +} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 8543980c8..a08a4165b 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -366,7 +366,9 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc return nil, constraintErr } - final, revocationToken, runErr := h.purchase.RunPlannedPurchaseNow(ctx, executionID, fourEyesActorIdentity(session), resolveCreatorUserID(session)) + // The check above read a snapshot; the funnel re-runs it on the row its + // claim returns, the row it buys (issue #415). + final, revocationToken, runErr := h.purchase.RunPlannedPurchaseNow(ctx, executionID, fourEyesActorIdentity(session), resolveCreatorUserID(session), h.runNowClaimCheck(session)) if runErr != nil { return nil, executeFailureError(executionID, "started", final, runErr) } @@ -394,6 +396,8 @@ func executeFailureError(executionID, action string, final *config.PurchaseExecu 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)) + case errors.Is(err, purchase.ErrClaimedRowRefused): + return claimedRowRefusalError(final, err) case errors.Is(err, purchase.ErrStaleAzurePricing): return NewClientError(409, err.Error()) default: @@ -825,6 +829,8 @@ func tokenActionError(err error) error { switch { case errors.Is(err, config.ErrAuditLoss): return err + case errors.Is(err, purchase.ErrClaimedRowRefused): + return claimedRowRefusalError(nil, err) case errors.Is(err, purchase.ErrStaleAzurePricing): return NewClientError(409, err.Error()) case errors.Is(err, purchase.ErrInvalidApprovalToken): @@ -900,7 +906,9 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio // sourced from this return value, never from a DB re-read, since // only the token hash is stored and a re-read can only ever yield // the hash. - revocationToken, approveErr := h.purchase.ApproveExecution(ctx, execution.ExecutionID, token, actor) + // 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)) if approveErr != nil { return nil, tokenActionError(approveErr) } @@ -990,7 +998,9 @@ 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. - final, revocationToken, approveErr := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, fourEyesActorIdentity(session), actor) + // authorizeApproveSession above read a snapshot; the funnel re-runs the + // approve-any/approve-own check on the row its claim returns (issue #415). + final, revocationToken, approveErr := h.purchase.ApproveAndExecute(ctx, execution.ExecutionID, fourEyesActorIdentity(session), actor, h.sessionApproveClaimCheck(session)) if approveErr != nil { logging.Errorf("purchase[%s]: approvePurchaseViaSession failed after %s: %v", execution.ExecutionID, time.Since(t0), approveErr) @@ -3460,7 +3470,9 @@ func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaF // 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. - final, revocationToken, err := h.purchase.DirectExecute(ctx, executionID, fourEyesActorIdentity(session), validUUIDPtrOrNil(&session.UserID)) + // The direct-execute Constraints ran on the request's recommendations; the + // funnel re-runs them on the row its claim returns (issue #415). + final, revocationToken, err := h.purchase.DirectExecute(ctx, executionID, fourEyesActorIdentity(session), validUUIDPtrOrNil(&session.UserID), h.directExecuteClaimCheck(session)) if err != nil { logging.Errorf("purchase[%s]: directExecutePurchase failed after %s: %v", executionID, time.Since(t0), err) diff --git a/internal/api/handler_purchases_claimed_row_integration_test.go b/internal/api/handler_purchases_claimed_row_integration_test.go new file mode 100644 index 000000000..b3f77ff2a --- /dev/null +++ b/internal/api/handler_purchases_claimed_row_integration_test.go @@ -0,0 +1,135 @@ +//go:build integration + +package api + +import ( + "encoding/json" + "sync" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #415 against real PostgreSQL: the handler's Constraints check passes on +// the row it read, a second writer reprices the row over the cap before the +// claim CAS, and the funnel must refuse the row the CAS returned. Real store, +// real handler and purchase manager, counting fake provider (runNowService); +// the Constraints are decided by the real auth matcher. Nothing here reaches a +// cloud API. + +const ( + claimRaceCap = 1000.0 + claimRaceRepriced = 5000.0 // fanoutRecs() costs 300 +) + +// repriceDuringCheck answers the Constraints check with the real matcher and, +// on the first check that sees the original price, reprices the row with a +// separate UPDATE: the repricing lands after the handler's check and before +// the CAS. reprice=false leaves the row alone (the unmodified control). +func repriceDuringCheck(t *testing.T, f *pauseClaimFixture, perms []auth.Permission, reprice bool) *MockAuthService { + t.Helper() + var once sync.Once + mockAuth := new(MockAuthService) + session := &Session{UserID: *f.execution.CreatedByUserID, Email: "creator@example.com"} + mockAuth.On("ValidateSession", mock.Anything, "admin-token").Return(session, nil).Maybe() + mockAuth.On("ValidateCSRFToken", mock.Anything, "admin-token", "").Return(nil).Maybe() + authCtx := &auth.AuthContext{Permissions: perms} + mockAuth.On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(func(action, resource string) bool { return authCtx.HasPermission(action, resource) }, nil).Maybe() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(constraintDecisionFunc(func(action, resource string, sets []auth.PermissionConstraints) (bool, error) { + allowed, err := auth.PermissionsAllowForConstraintSets(perms, action, resource, sets) + if reprice && allowed && len(sets) > 0 && sets[0].MaxPurchaseAmount < claimRaceCap { + once.Do(func() { repriceRow(t, f, claimRaceRepriced) }) + } + return allowed, err + }), nil).Maybe() + return mockAuth +} + +// repriceRow is the concurrent writer: a raw UPDATE on its own pool +// connection, as a repricing or an edit would be. +func repriceRow(t *testing.T, f *pauseClaimFixture, upfront float64) { + t.Helper() + recs := fanoutRecs() + for i := range recs { + recs[i].CloudAccountID = f.execution.CloudAccountID + recs[i].UpfrontCost = upfront + } + payload, err := json.Marshal(recs) + require.NoError(t, err) + tag, err := f.pool.Exec(f.ctx, `UPDATE purchase_executions SET recommendations = $2 WHERE execution_id = $1`, f.execution.ExecutionID, payload) + require.NoError(t, err) + require.EqualValues(t, 1, tag.RowsAffected()) +} + +func claimRacePerm(action string) auth.Permission { + return auth.Permission{Action: action, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: claimRaceCap}} +} + +type claimRaceEntry struct { + name string + perms []auth.Permission + call func(f *pauseClaimFixture) error +} + +func claimRaceEntries() []claimRaceEntry { + return []claimRaceEntry{ + {"run-now", []auth.Permission{claimRacePerm(auth.ActionExecute)}, func(f *pauseClaimFixture) error { + _, err := f.runNow() + return err + }}, + {"session approve", []auth.Permission{claimRacePerm(auth.ActionApproveAny)}, func(f *pauseClaimFixture) error { + _, err := f.handler.approvePurchase(f.ctx, f.request, f.execution.ExecutionID, "") + return err + }}, + } +} + +func TestClaimedRowCheck_Postgres_RepricedBetweenCheckAndClaimIsRefused(t *testing.T) { + for _, e := range claimRaceEntries() { + t.Run(e.name, func(t *testing.T) { + f, service := newRunNowFixture(t, 0, fanoutRecs()) + f.handler.auth = repriceDuringCheck(t, f, e.perms, true) + + err := e.call(f) + + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client-facing error, got %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.message, "nothing was bought") + assert.Zero(t, service.calls.Load(), "the repriced row must never reach the provider") + row, getErr := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, getErr) + assert.Equal(t, claimRaceRepriced, row.Recommendations[0].UpfrontCost, "the race must have landed") + assert.Equal(t, "failed", row.Status) + assert.True(t, row.NoSubmission, "refused before any provider call") + assert.Nil(t, row.ApprovedBy) + assert.Nil(t, row.ExecutedAt) + assert.Contains(t, row.Error, "constraints") + var histories int + require.NoError(t, f.pool.QueryRow(f.ctx, "SELECT count(*) FROM purchase_history WHERE plan_id=$1", f.planID).Scan(&histories)) + assert.Zero(t, histories) + }) + } +} + +func TestClaimedRowCheck_Postgres_UnmodifiedRowPurchases(t *testing.T) { + for _, e := range claimRaceEntries() { + t.Run(e.name, func(t *testing.T) { + f, service := newRunNowFixture(t, 0, fanoutRecs()) + f.handler.auth = repriceDuringCheck(t, f, e.perms, false) + + require.NoError(t, e.call(f)) + + assert.EqualValues(t, 1, service.calls.Load()) + row, err := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, err) + assert.Equal(t, "completed", row.Status) + }) + } +} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index bfade3be4..1159d9a72 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -6721,7 +6721,10 @@ func TestHandler_approvePurchaseViaSession_FourEyesOn_DifferentApproverSucceeds( } // The approved row has no plan: a plan step with no recommendations is // refused at execution (#609), and this test is about the 4-eyes check. - approved := &config.PurchaseExecution{ExecutionID: execID, Status: "approved"} + // It carries the recommendations the approver was checked against: the + // funnel re-runs that check on the claimed row (issue #415), and an + // empty claimed row fails it closed. + approved := &config.PurchaseExecution{ExecutionID: execID, Status: "running", Recommendations: exec.Recommendations} mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockConfig.On("GetGlobalConfig", mock.Anything).Return(fourEyesCfgOn(), nil) mockConfig.On("TransitionExecutionStatus", ctx, execID, []string{"pending", "notified"}, "running", &approverID).Return(approved, nil) diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 7012a152d..31a00a167 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -2,12 +2,14 @@ package api import ( "context" + "fmt" "sync" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/LeanerCloud/cloud-commitments-platform/internal/mocks" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/LeanerCloud/cloud-commitments-platform/internal/scheduler" "github.com/stretchr/testify/mock" ) @@ -44,28 +46,67 @@ func (m *MockCredentialStore) DecryptPayload(ciphertext string) ([]byte, error) } // MockPurchaseManager is a mock implementation of purchase.Manager. +// +// Expectations match (ctx, execID, actor/token, ...) without the +// ClaimedRowCheck argument; every check the handler passes is recorded in +// Checks. When ClaimedRow is set, each execute method first runs the check +// against a copy of it, emulating the funnel's post-claim re-check (issue +// #415): a refusal returns that copy marked failed and an error wrapping +// purchase.ErrClaimedRowRefused, without consuming the expectation (nothing +// is bought). type MockPurchaseManager struct { mock.Mock + ClaimedRow *config.PurchaseExecution + Checks []purchase.ClaimedRowCheck +} + +// claimedRowRefusal records check and, when ClaimedRow is set, runs it the +// way transitionApproveAndExecute does after winning the claim. +func (m *MockPurchaseManager) claimedRowRefusal(ctx context.Context, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, error) { + m.Checks = append(m.Checks, check) + if m.ClaimedRow == nil || check == nil { + return nil, nil + } + claimed := *m.ClaimedRow + claimed.Status = "running" + if err := check(ctx, &claimed); err != nil { + claimed.Status = "failed" + claimed.NoSubmission = true + return &claimed, fmt.Errorf("%w: %w", purchase.ErrClaimedRowRefused, err) + } + return nil, nil } -func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string) (string, error) { +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 + } args := m.Called(ctx, execID, token, actor) return args.String(0), args.Error(1) } -func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { +func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *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, actor, transitionedBy) 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) { +func (m *MockPurchaseManager) DirectExecute(ctx context.Context, execID, actor string, transitionedBy *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, 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) { +func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *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, actor, transitionedBy) exec, _ := args.Get(0).(*config.PurchaseExecution) return exec, args.String(1), args.Error(2) diff --git a/internal/api/types.go b/internal/api/types.go index 9570dca8d..46261d7b0 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -12,6 +12,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/LeanerCloud/cloud-commitments-platform/internal/email" "github.com/LeanerCloud/cloud-commitments-platform/internal/oidc" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/LeanerCloud/cloud-commitments-platform/internal/scheduler" ) @@ -140,17 +141,23 @@ type BreakdownValue struct { // 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. +// +// `check` (every execute entry point) re-runs the caller's permission +// Constraints against the row the claim CAS returned, the row that is +// actually bought, before any provider call (issue #415). The handler's own +// 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) (string, error) - ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (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. - DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*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 // nothing else consumes (issue #218). - RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) CancelExecution(ctx context.Context, execID, token, actor string) error } diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 11bcbf8f0..1056b172d 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -44,12 +44,15 @@ const directExecuteSkipReason = "direct-execute permission" // NULL — the column is nullable TEXT and we don't want to claim "approved // by nobody". // +// 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 // otherwise-successful approve. -func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, actor string) (string, error) { +func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, actor string, check ClaimedRowCheck) (string, error) { t0 := time.Now() logging.Infof("purchase[%s]: ApproveExecution entry (auth=token actor=%q)", executionID, maskActor(actor)) @@ -76,7 +79,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, check) if err != nil { logging.Errorf("purchase[%s]: ApproveExecution (token path) failed after %s: %v", executionID, time.Since(t0), err) @@ -319,17 +322,18 @@ func (m *Manager) checkDifferentApprover(ctx context.Context, executionID string // per-account in parallel via executeMultiAccount. // // 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"}, "") +// error; it is nil when the run never started. check runs against the claimed +// row before any provider call; see ClaimedRowCheck. +func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string, check ClaimedRowCheck) (*config.PurchaseExecution, string, error) { + return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified"}, "", check) } // 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) +func (m *Manager) DirectExecute(ctx context.Context, executionID, actor string, transitionedBy *string, check ClaimedRowCheck) (*config.PurchaseExecution, string, error) { + return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified"}, directExecuteSkipReason, check) } // RunPlannedPurchaseNow lets an operator force a scheduled purchase (pending @@ -350,9 +354,10 @@ func (m *Manager) DirectExecute(ctx context.Context, executionID, actor string, // error rather than executing the purchase a second time. // // The returned row carries the run-now audit stamp and its final status, also -// on an execution error; it is nil when the run never started. -func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { - return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified", "paused"}, runNowSkipReason) +// on an execution error; it is nil when the run never started. check runs +// against the claimed row before any provider call; see ClaimedRowCheck. +func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor string, transitionedBy *string, check ClaimedRowCheck) (*config.PurchaseExecution, string, error) { + return m.transitionApproveAndExecute(ctx, executionID, actor, transitionedBy, []string{"pending", "notified", "paused"}, runNowSkipReason, check) } // transitionApproveAndExecute is the shared body behind ApproveAndExecute @@ -370,7 +375,13 @@ func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor // // A non-empty skipReason stamps the executed_* audit fields once the CAS wins. // The returned row is nil when the CAS was never won. -func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string, fromStatuses []string, skipReason string) (*config.PurchaseExecution, string, error) { +// +// check runs against the row the CAS returned, which is the row +// executeAndFinalize buys, before any audit stamp or provider call (issue +// #415). A refusal fails that row before submission (status "failed", +// no_submission set, a plan row's suppressions released) and returns it with +// an error satisfying errors.Is(err, ErrClaimedRowRefused). +func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, actor string, transitionedBy *string, fromStatuses []string, skipReason string, check ClaimedRowCheck) (*config.PurchaseExecution, string, error) { t0 := time.Now() logging.Infof("purchase[%s]: transitionApproveAndExecute starting (actor=%q, from=%v)", executionID, maskActor(actor), fromStatuses) @@ -393,11 +404,9 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, // executor (SQS, cron, re-drive, scheduler) can run this row, and there is // no window where the row sits "approved" with a stale copy in flight // (issue #829). A lost CAS surfaces ErrExecutionNotInExpectedStatus. - updated, err := m.claimRunning(ctx, executionID, fromStatuses, transitionedBy) + updated, err := m.claimAndCheck(ctx, executionID, fromStatuses, transitionedBy, check, t0) if err != nil { - logging.Errorf("purchase[%s]: transitionApproveAndExecute status transition failed after %s: %v", - executionID, time.Since(t0), err) - return nil, "", fmt.Errorf("approve: %w", err) + return updated, "", err } logging.Infof("purchase[%s]: approved and claimed running in %s", executionID, time.Since(t0)) diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index 26754db25..15e290659 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -102,7 +102,7 @@ func TestManager_ApproveExecution_Success(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "") + _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) sender.AssertExpectations(t) @@ -131,7 +131,7 @@ func TestManager_ApproveExecution_StampsApprovedBy(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "operator@example.com") + _, 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 +166,7 @@ func TestManager_ApproveExecution_NotifiedStatus(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-456") - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "") + _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) sender.AssertExpectations(t) @@ -185,7 +185,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", "") + _, 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 +207,7 @@ func TestManager_ApproveExecution_EmptyToken(t *testing.T) { } store.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) - _, err := manager.ApproveExecution(ctx, "exec-123", "", "") + _, err := manager.ApproveExecution(ctx, "exec-123", "", "", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid approval token") store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) @@ -219,7 +219,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", "") + _, 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 +231,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", "") + _, 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,7 +255,7 @@ func TestManager_ApproveExecution_TransitionFails(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-123", approveFromStatuses, "running", (*string)(nil)). Return(nil, errors.New(`execution exec-123 cannot transition from "canceled" to "approved"`)) - _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "") + _, err := manager.ApproveExecution(ctx, "exec-123", "valid-token", "", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "cannot transition") // Execute chain must not run after a failed transition. @@ -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, 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, nil) require.NoError(t, err) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, "session-user@example.com", *updated.ApprovedBy) @@ -363,7 +363,7 @@ func TestManager_RunPlannedPurchaseNow_ExecutesFromPaused(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-run-now", []string{"pending", "notified", "paused"}, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-run-now") - final, revocationToken, err := manager.RunPlannedPurchaseNow(ctx, "exec-run-now", "operator@example.com", nil) + final, revocationToken, err := manager.RunPlannedPurchaseNow(ctx, "exec-run-now", "operator@example.com", nil, nil) require.NoError(t, err) // Run-now shares the execute funnel's revocation-token rotation (#103). require.NotEmpty(t, revocationToken) @@ -390,7 +390,7 @@ func TestManager_RunPlannedPurchaseNow_LostCASReturnsError(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-race", []string{"pending", "notified", "paused"}, "running", (*string)(nil)). Return(nil, config.ErrExecutionNotInExpectedStatus) - final, _, err := manager.RunPlannedPurchaseNow(ctx, "exec-race", "operator@example.com", nil) + final, _, err := manager.RunPlannedPurchaseNow(ctx, "exec-race", "operator@example.com", nil, nil) require.Error(t, err) assert.Nil(t, final) assert.ErrorIs(t, err, config.ErrExecutionNotInExpectedStatus) @@ -416,7 +416,7 @@ func TestManager_DirectExecute_StampsAuditFieldsAfterClaim(t *testing.T) { 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) + final, _, err := manager.DirectExecute(ctx, "exec-direct-stamp", "session-user@example.com", &actorUUID, nil) require.NoError(t, err) assert.Same(t, updated, final) require.NotNil(t, final.ExecutedAt) @@ -436,7 +436,7 @@ func TestManager_DirectExecute_RefusedLeavesNoExecutedAt(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-direct-lost", approveFromStatuses, "running", (*string)(nil)). Return(nil, config.ErrExecutionNotInExpectedStatus) - final, _, err := manager.DirectExecute(ctx, "exec-direct-lost", "operator@example.com", nil) + final, _, err := manager.DirectExecute(ctx, "exec-direct-lost", "operator@example.com", nil, nil) require.ErrorIs(t, err, config.ErrExecutionNotInExpectedStatus) assert.Nil(t, final) store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) @@ -460,7 +460,7 @@ func TestManager_ApproveAndExecute_ReturnsRowOnExecutionFailure(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-fail-after-claim", approveFromStatuses, "running", (*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) + final, token, err := manager.ApproveAndExecute(ctx, "exec-fail-after-claim", "operator@example.com", nil, nil) require.Error(t, err) assert.Empty(t, token) require.NotNil(t, final) @@ -516,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, nil) require.Error(t, err) assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") store.AssertNotCalled(t, "TransitionExecutionStatus", @@ -551,7 +551,7 @@ func TestManager_ApproveAndExecute_FourEyesOn_AllowsDifferentApprover(t *testing store.On("TransitionExecutionStatus", ctx, "exec-direct-diff", approveFromStatuses, "running", &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, nil) require.NoError(t, err) require.NotNil(t, updated.ApprovedBy) assert.Equal(t, approverEmail, *updated.ApprovedBy) @@ -585,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, nil) require.Error(t, err) assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") store.AssertNotCalled(t, "TransitionExecutionStatus", @@ -606,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, nil) require.Error(t, err) assert.Contains(t, err.Error(), "predates the dual-control feature") store.AssertExpectations(t) @@ -624,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, nil) require.Error(t, err) assert.Contains(t, err.Error(), "no approver identity could be determined") store.AssertExpectations(t) @@ -648,7 +648,7 @@ func TestManager_ApproveAndExecute_FourEyesOff_AllowsSelfApprove(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-mode-off", approveFromStatuses, "running", &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, nil) require.NoError(t, err) store.AssertNotCalled(t, "GetExecutionByID", mock.Anything, mock.Anything) store.AssertNotCalled(t, "GetUserEmailByID", mock.Anything, mock.Anything) @@ -681,7 +681,7 @@ func TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied(t *tes store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) store.On("GetUserEmailByID", ctx, creatorID).Return(creatorEmail, nil) - _, err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", creatorEmail) + _, err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", creatorEmail, nil) require.Error(t, err) assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") store.AssertNotCalled(t, "TransitionExecutionStatus", @@ -719,7 +719,7 @@ func TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed(t *testing.T store.On("TransitionExecutionStatus", ctx, "exec-sqs-diff", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-fourEyes") - _, err := manager.ApproveExecution(ctx, "exec-sqs-diff", "valid-token", "approver@example.com") + _, 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) @@ -760,7 +760,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") + _, 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, "TransitionExecutionStatus", @@ -777,7 +777,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") + _, 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, "TransitionExecutionStatus", @@ -1027,7 +1027,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", "") + _, 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 @@ -1062,7 +1062,7 @@ func TestManager_ApproveExecution_ValidTokenWithinTTL(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-live", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-live") - _, err := manager.ApproveExecution(ctx, "exec-live", "valid-token", "") + _, err := manager.ApproveExecution(ctx, "exec-live", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) } @@ -1093,7 +1093,7 @@ func TestManager_ApproveExecution_NilExpiresAt_LegacyRow(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-legacy", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-legacy") - _, err := manager.ApproveExecution(ctx, "exec-legacy", "valid-token", "") + _, err := manager.ApproveExecution(ctx, "exec-legacy", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) } @@ -1119,7 +1119,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", "") + _, 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") @@ -1155,7 +1155,7 @@ func TestManager_ApproveExecution_AWSOrphanFallsThrough(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-aws-ambient", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-aws") - _, err := manager.ApproveExecution(ctx, "exec-aws-ambient", "valid-token", "") + _, err := manager.ApproveExecution(ctx, "exec-aws-ambient", "valid-token", "", nil) require.NoError(t, err) store.AssertExpectations(t) sender.AssertExpectations(t) @@ -1275,7 +1275,7 @@ func TestApproveExecution_MintsRevocationToken(t *testing.T) { store.On("TransitionExecutionStatus", ctx, "exec-rotate", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) stubExecuteChain(t, store, sender, "plan-rotate") - _, err := manager.ApproveExecution(ctx, "exec-rotate", "pre-rotate-token", "") + _, 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/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index 193ef5afb..9647e24f7 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"}) - _, _, err := mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) + _, _, err := mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil, 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/purchase/aws_no_token_redrive_test.go b/internal/purchase/aws_no_token_redrive_test.go index 07760b125..4acddfa7c 100644 --- a/internal/purchase/aws_no_token_redrive_test.go +++ b/internal/purchase/aws_no_token_redrive_test.go @@ -181,11 +181,11 @@ func TestApproveAndExecuteRefusesAWSNoTokenRetrySuccessor(t *testing.T) { approve func(m *Manager, id string) error }{ {"token", nil, func(m *Manager, id string) error { - _, err := m.ApproveExecution(context.Background(), id, rawToken, "approver@example.com") + _, err := m.ApproveExecution(context.Background(), id, rawToken, "approver@example.com", nil) return err }}, {"session", &sessionUser, func(m *Manager, id string) error { - _, _, err := m.ApproveAndExecute(context.Background(), id, "approver@example.com", &sessionUser) + _, _, err := m.ApproveAndExecute(context.Background(), id, "approver@example.com", &sessionUser, nil) return err }}, } diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index f9d3b6924..15ffc67e2 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -67,7 +67,7 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { saved = © return nil } - _, _, approveErr := mgr.ApproveAndExecute(ctx, exec.ExecutionID, "operator@example.com", nil) + _, _, approveErr := mgr.ApproveAndExecute(ctx, exec.ExecutionID, "operator@example.com", nil, nil) require.Equal(t, purchases, recorder.count()) require.NotNil(t, saved) if purchases == 1 { diff --git a/internal/purchase/azure_pricing_test.go b/internal/purchase/azure_pricing_test.go index 85b913db4..0abd597d2 100644 --- a/internal/purchase/azure_pricing_test.go +++ b/internal/purchase/azure_pricing_test.go @@ -98,9 +98,9 @@ func TestAzurePricingRefusalEntryPoints(t *testing.T) { expectClaim(store, exec, []string{"pending", "notified"}) var err error if entry == "approve" { - _, _, err = mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) + _, _, err = mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil, nil) } else { - _, _, err = mgr.DirectExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) + _, _, err = mgr.DirectExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil, nil) } assert.Zero(t, recorder.count()) require.Error(t, err) diff --git a/internal/purchase/claim.go b/internal/purchase/claim.go index bad5ab083..70d6a2216 100644 --- a/internal/purchase/claim.go +++ b/internal/purchase/claim.go @@ -2,7 +2,11 @@ package purchase import ( "context" + "errors" + "fmt" + "time" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -11,6 +15,21 @@ import ( // claim and not after the reaper, a cancel or another writer moved it on. var executorOwnedStatuses = []string{"running"} +// ErrClaimedRowRefused marks a run refused by the caller's ClaimedRowCheck +// after the claim was won and before any provider call (issue #415). The row +// ends "failed" with no_submission stamped; the check's own error is wrapped +// alongside it so callers can still map it (for example to a 403). +var ErrClaimedRowRefused = errors.New("purchase refused after claim: the claimed execution no longer passes the authorization checks") + +// ClaimedRowCheck re-runs a caller's authorization checks (permission +// Constraints such as MaxPurchaseAmount) against the row the claim CAS +// returned, which is exactly the row executeAndFinalize buys. A caller's +// earlier check ran on a snapshot read before the CAS, so a repricing or a +// recommendation attach landing in between would otherwise be bought +// unchecked (issue #415). A nil check skips the re-check; only callers with +// no pre-CAS Constraints check (the SQS approve worker) pass nil. +type ClaimedRowCheck func(ctx context.Context, claimed *config.PurchaseExecution) error + // claimRunning is the single claim primitive: an atomic CAS from one of from // to "running". Winning it is the right to call the provider; every executor // (approve, scheduler, SQS, cron, re-drive) claims before any provider call. @@ -20,3 +39,23 @@ var executorOwnedStatuses = []string{"running"} func (m *Manager) claimRunning(ctx context.Context, executionID string, from []string, actor *string) (*config.PurchaseExecution, error) { return m.config.TransitionExecutionStatus(ctx, executionID, from, "running", actor) } + +// claimAndCheck claims the row for transitionApproveAndExecute and then runs +// check against the claimed row. A lost claim returns (nil, err). A refused +// check fails the claimed row before submission and returns (row, err), with +// err satisfying errors.Is(err, ErrClaimedRowRefused); nothing is bought. +func (m *Manager) claimAndCheck(ctx context.Context, executionID string, from []string, actor *string, check ClaimedRowCheck, t0 time.Time) (*config.PurchaseExecution, error) { + updated, err := m.claimRunning(ctx, executionID, from, actor) + if err != nil { + logging.Errorf("purchase[%s]: transitionApproveAndExecute status transition failed after %s: %v", + executionID, time.Since(t0), err) + return nil, fmt.Errorf("approve: %w", err) + } + if check == nil { + return updated, nil + } + if checkErr := check(ctx, updated); checkErr != nil { + return updated, m.failClaimedBeforeSubmission(ctx, updated, fmt.Errorf("%w: %w", ErrClaimedRowRefused, checkErr)) + } + return updated, nil +} diff --git a/internal/purchase/claim_guard_test.go b/internal/purchase/claim_guard_test.go index 1a5014ebc..560b5798e 100644 --- a/internal/purchase/claim_guard_test.go +++ b/internal/purchase/claim_guard_test.go @@ -108,7 +108,7 @@ func TestApprove_ClaimsRunningAndStampsGuarded(t *testing.T) { store.On("SaveExecutionIfStatus", ctx, mock.Anything, []string{"running"}).Return(nil).Once() store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, []string{"running"}).Return(nil).Once() - _, _, err := manager.ApproveAndExecute(ctx, "exec-g", "approver@example.invalid", nil) + _, _, err := manager.ApproveAndExecute(ctx, "exec-g", "approver@example.invalid", nil, nil) require.NoError(t, err) store.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-g", approveFromStatuses, "running", (*string)(nil)) // Two guarded writes (audit stamp, terminal save); the only unguarded one is diff --git a/internal/purchase/claimed_row_check_test.go b/internal/purchase/claimed_row_check_test.go new file mode 100644 index 000000000..fd1a723fd --- /dev/null +++ b/internal/purchase/claimed_row_check_test.go @@ -0,0 +1,236 @@ +package purchase + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #415: the handler checks permission Constraints against a row read +// before the claim CAS, but the funnel buys the row the CAS returns. These +// tests reprice the row between the two (the CAS mock returns the repriced +// copy) and assert, per entry point, that the ClaimedRowCheck sees the +// claimed row, refuses it before any provider call, and leaves it failed with +// no_submission set. Mock store and recording provider only; nothing here +// reaches a cloud API. + +const ( + claimCheckCap = 50000.0 + claimCheckPrice = 30000.0 // under the cap: what the handler checked + claimCheckRepriced = 90000.0 // over the cap: what the CAS returned + claimCheckToken = "claimed-row-check-token" +) + +var errClaimCheckOverCap = errors.New("permission denied: over MaxPurchaseAmount") + +// capCheck is a ClaimedRowCheck modeling a MaxPurchaseAmount cap. It records +// every row it was handed so a test can prove it saw the claimed row. +type capCheck struct { + max float64 + err error // returned instead of the cap decision when set + seen []config.PurchaseExecution +} + +func (c *capCheck) check(_ context.Context, claimed *config.PurchaseExecution) error { + c.seen = append(c.seen, *claimed) + if c.err != nil { + return c.err + } + var total float64 + for i := range claimed.Recommendations { + total += claimed.Recommendations[i].TotalCommitment() + } + if total > c.max { + return fmt.Errorf("%w: %.2f > %.2f", errClaimCheckOverCap, total, c.max) + } + return nil +} + +// claimEntryPoint drives one funnel entry point; row is nil where the entry +// point returns none (the token approve). +type claimEntryPoint struct { + name string + from []string + run func(m *Manager, id string, check ClaimedRowCheck) (*config.PurchaseExecution, error) +} + +func claimEntryPoints() []claimEntryPoint { + ctx := context.Background() + actor := "operator@example.com" + return []claimEntryPoint{ + {"run-now", runNowFrom, func(m *Manager, id string, c ClaimedRowCheck) (*config.PurchaseExecution, error) { + row, _, err := m.RunPlannedPurchaseNow(ctx, id, actor, nil, c) + return row, err + }}, + {"session approve", []string{"pending", "notified"}, func(m *Manager, id string, c ClaimedRowCheck) (*config.PurchaseExecution, error) { + row, _, err := m.ApproveAndExecute(ctx, id, actor, nil, c) + return row, err + }}, + {"direct execute", []string{"pending", "notified"}, func(m *Manager, id string, c ClaimedRowCheck) (*config.PurchaseExecution, error) { + row, _, err := m.DirectExecute(ctx, id, actor, nil, c) + 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) + return nil, err + }}, + } +} + +// claimCheckHarness wires a pending AWS RDS row (no plan) that GetExecutionByID +// returns at claimCheckPrice and the claim CAS returns at claimedPrice. saved +// collects every persisted copy of the row. +func claimCheckHarness(t *testing.T, ep claimEntryPoint, claimedPrice float64) (*Manager, *MockConfigStore, *armedPurchaseRecorder, *config.PurchaseExecution, *[]config.PurchaseExecution) { + t.Helper() + snapshot := armedExecution("aws", "rds", 0, "pending") + snapshot.PlanID = "" + snapshot.Recommendations[0].UpfrontCost = claimCheckPrice + snapshot.ApprovalToken = config.HashApprovalToken(claimCheckToken) + mgr, store, rec := armedHarness(t, common.ServiceRDS) + saved := &[]config.PurchaseExecution{} + store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + *saved = append(*saved, *e) + return nil + } + store.On("GetExecutionByID", mock.Anything, snapshot.ExecutionID).Return(snapshot, nil).Maybe() + claimed := *snapshot + claimed.Recommendations = append([]config.RecommendationRecord(nil), snapshot.Recommendations...) + claimed.Recommendations[0].UpfrontCost = claimedPrice + claimed.Status = "running" + store.On("TransitionExecutionStatus", mock.Anything, snapshot.ExecutionID, ep.from, "running", (*string)(nil)). + Return(&claimed, nil).Once() + return mgr, store, rec, snapshot, saved +} + +func lastSaved(t *testing.T, saved []config.PurchaseExecution) config.PurchaseExecution { + t.Helper() + require.NotEmpty(t, saved, "the refused row must be persisted, not left running for the reaper") + return saved[len(saved)-1] +} + +// The regression: the row is repriced over the cap between the handler's +// check and the CAS. Before the fix the funnel bought the repriced row. +func TestClaimedRowCheck_RepricedBetweenCheckAndClaimIsRefused(t *testing.T) { + for _, ep := range claimEntryPoints() { + t.Run(ep.name, func(t *testing.T) { + mgr, _, rec, snapshot, saved := claimCheckHarness(t, ep, claimCheckRepriced) + cc := &capCheck{max: claimCheckCap} + + row, err := ep.run(mgr, snapshot.ExecutionID, cc.check) + + assert.Zero(t, rec.count(), "the repriced row must not reach the provider") + require.ErrorIs(t, err, ErrClaimedRowRefused) + require.ErrorIs(t, err, errClaimCheckOverCap, "the check's own error must stay reachable for the HTTP mapping") + require.Len(t, cc.seen, 1) + assert.Equal(t, claimCheckRepriced, cc.seen[0].Recommendations[0].UpfrontCost, + "the check must run on the row the CAS returned, not on the pre-CAS snapshot") + got := lastSaved(t, *saved) + assert.Equal(t, "failed", got.Status) + assert.True(t, got.NoSubmission, "a refusal before any provider call is proven not submitted") + assert.Contains(t, got.Error, ErrClaimedRowRefused.Error()) + assert.Nil(t, got.ApprovedBy, "a refused run is not an approval") + assert.Nil(t, got.ExecutedAt, "a refused run never executed") + assert.Nil(t, got.PreApprovalSkipReason) + if row != nil { + assert.Equal(t, "failed", row.Status) + } + }) + } +} + +// Negative control: an unmodified row passes the same check and is bought +// once. A check that refused everything would fail here. +func TestClaimedRowCheck_UnmodifiedRowStillPurchases(t *testing.T) { + for _, ep := range claimEntryPoints() { + t.Run(ep.name, func(t *testing.T) { + mgr, _, rec, snapshot, saved := claimCheckHarness(t, ep, claimCheckPrice) + cc := &capCheck{max: claimCheckCap} + + _, err := ep.run(mgr, snapshot.ExecutionID, cc.check) + + require.NoError(t, err) + assert.Equal(t, 1, rec.count()) + require.Len(t, cc.seen, 1, "the check runs exactly once, after the claim") + assert.Equal(t, "running", cc.seen[0].Status, "the check runs on the claimed row") + assert.Equal(t, "completed", lastSaved(t, *saved).Status) + }) + } +} + +// A check that cannot be evaluated (an auth backend error) fails closed. +func TestClaimedRowCheck_CheckErrorFailsClosed(t *testing.T) { + ep := claimEntryPoints()[0] + mgr, _, rec, snapshot, saved := claimCheckHarness(t, ep, claimCheckPrice) + backendErr := errors.New("permission check failed: auth store unavailable") + cc := &capCheck{max: claimCheckCap, err: backendErr} + + _, err := ep.run(mgr, snapshot.ExecutionID, cc.check) + + assert.Zero(t, rec.count()) + require.ErrorIs(t, err, ErrClaimedRowRefused) + require.ErrorIs(t, err, backendErr) + got := lastSaved(t, *saved) + assert.Equal(t, "failed", got.Status) + assert.True(t, got.NoSubmission) +} + +// When the failed row cannot be saved, the refusal still holds and the caller +// learns the row is stranded "running" for the reaper. +func TestClaimedRowCheck_TerminalSaveFailureIsAuditLoss(t *testing.T) { + ep := claimEntryPoints()[0] + mgr, store, rec, snapshot, _ := claimCheckHarness(t, ep, claimCheckRepriced) + store.On("SaveExecutionIfStatus", mock.Anything, mock.Anything, executorOwnedStatuses).Return(errors.New("db down")) + cc := &capCheck{max: claimCheckCap} + + _, err := ep.run(mgr, snapshot.ExecutionID, cc.check) + + assert.Zero(t, rec.count()) + require.ErrorIs(t, err, config.ErrAuditLoss) + require.ErrorIs(t, err, ErrClaimedRowRefused) +} + +// A plan row refused after the claim releases its suppressions with the +// failed save, like every other before-submission refusal. +func TestClaimedRowCheck_PlanRowReleasesSuppressions(t *testing.T) { + ep := claimEntryPoints()[0] + mgr, store, rec, snapshot, saved := claimCheckHarness(t, ep, claimCheckRepriced) + // Re-register the claim for a plan row: same row, PlanID set. + planRow := *snapshot + planRow.PlanID = armedPlanID + store.ExpectedCalls = nil + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("GetExecutionByID", mock.Anything, planRow.ExecutionID).Return(&planRow, nil).Maybe() + claimed := planRow + claimed.Recommendations = append([]config.RecommendationRecord(nil), planRow.Recommendations...) + claimed.Recommendations[0].UpfrontCost = claimCheckRepriced + claimed.Status = "running" + store.On("TransitionExecutionStatus", mock.Anything, planRow.ExecutionID, ep.from, "running", (*string)(nil)). + Return(&claimed, nil).Once() + cc := &capCheck{max: claimCheckCap} + + _, err := ep.run(mgr, planRow.ExecutionID, cc.check) + + require.ErrorIs(t, err, ErrClaimedRowRefused) + assert.Zero(t, rec.count()) + assert.Equal(t, "failed", lastSaved(t, *saved).Status) + store.AssertCalled(t, "DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, planRow.ExecutionID) +} + +// A nil check (the SQS approve worker, which has no session Constraints) +// keeps the previous behavior. +func TestClaimedRowCheck_NilCheckSkipsRecheck(t *testing.T) { + ep := claimEntryPoints()[3] + mgr, _, rec, snapshot, _ := claimCheckHarness(t, ep, claimCheckRepriced) + + _, err := ep.run(mgr, snapshot.ExecutionID, nil) + + require.NoError(t, err) + assert.Equal(t, 1, rec.count()) +} diff --git a/internal/purchase/ladder_execution_recheck_test.go b/internal/purchase/ladder_execution_recheck_test.go index a047db7ae..5d3683ad7 100644 --- a/internal/purchase/ladder_execution_recheck_test.go +++ b/internal/purchase/ladder_execution_recheck_test.go @@ -152,7 +152,7 @@ func TestLadderRecheck_EveryExecutorEntryRefuses(t *testing.T) { mgr, store, rec, exec := ladderHarness(t, ladderTrancheKey, openGate(), nil, off, nil) exec.Status = "pending" expectClaim(store, exec, []string{"pending", "notified"}) - _, _, err := mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) + _, _, err := mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil, nil) require.Error(t, err) assert.Equal(t, 0, rec.count()) }) diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go index 14d7585ac..eaf051854 100644 --- a/internal/purchase/messages.go +++ b/internal/purchase/messages.go @@ -177,8 +177,10 @@ func (m *Manager) handleApproveMessage(ctx context.Context, msg *AsyncMessage) e return err } // The SQS worker sends no email itself, so the minted revocation token - // (issue #103) has no consumer here; only the error matters. - _, err := m.ApproveExecution(ctx, msg.ExecutionID, msg.Token, msg.ActorEmail) + // (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) return err } diff --git a/internal/purchase/notified_run_now_test.go b/internal/purchase/notified_run_now_test.go index 35b017dee..9d724fe55 100644 --- a/internal/purchase/notified_run_now_test.go +++ b/internal/purchase/notified_run_now_test.go @@ -27,7 +27,7 @@ func TestRunNow_NotifiedRowReachesArmedRedriveGuard(t *testing.T) { store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, runNowFrom) - _, _, err := mgr.RunPlannedPurchaseNow(context.Background(), exec.ExecutionID, "operator@example.com", nil) + _, _, err := mgr.RunPlannedPurchaseNow(context.Background(), exec.ExecutionID, "operator@example.com", nil, nil) assert.Zero(t, rec.count()) require.Error(t, err) @@ -45,7 +45,7 @@ func TestRunNow_NotifiedRowReachesStaleAzurePricingRefusal(t *testing.T) { store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, runNowFrom) - _, _, err := mgr.RunPlannedPurchaseNow(context.Background(), exec.ExecutionID, "operator@example.com", nil) + _, _, err := mgr.RunPlannedPurchaseNow(context.Background(), exec.ExecutionID, "operator@example.com", nil, nil) assert.Zero(t, rec.count()) require.ErrorIs(t, err, ErrStaleAzurePricing) diff --git a/internal/purchase/single_claim_integration_test.go b/internal/purchase/single_claim_integration_test.go index 447463260..a73cd91b4 100644 --- a/internal/purchase/single_claim_integration_test.go +++ b/internal/purchase/single_claim_integration_test.go @@ -54,7 +54,7 @@ func TestSingleClaim_BothOrderingsBuyOnce(t *testing.T) { claimed, execErr := f.manager.claimAndExecute(ctx, rowStatusCopy(row)) require.NoError(t, execErr) require.True(t, claimed) - _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "approver@example.invalid", nil) + _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "approver@example.invalid", nil, nil) require.ErrorIs(t, err, config.ErrExecutionNotInExpectedStatus) assert.Equal(t, 1, f.purchaseCount()-before) assert.Equal(t, "completed", rowStatus(ctx, t, f, row.ExecutionID).Status) @@ -63,7 +63,7 @@ func TestSingleClaim_BothOrderingsBuyOnce(t *testing.T) { t.Run("approve then claimAndExecute", func(t *testing.T) { row := claimRaceRow(ctx, t, f, "notified") before := f.purchaseCount() - _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "approver@example.invalid", nil) + _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "approver@example.invalid", nil, nil) require.NoError(t, err) claimed, execErr := f.manager.claimAndExecute(ctx, rowStatusCopy(row)) require.NoError(t, execErr) @@ -85,10 +85,10 @@ func TestSingleClaim_ConcurrentExecutorsBuyOnce(t *testing.T) { const n = 4 cases := map[string][]func(row *config.PurchaseExecution){} approve := func(row *config.PurchaseExecution) { - _, _, _ = f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", nil) + _, _, _ = f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", nil, nil) } direct := func(row *config.PurchaseExecution) { - _, _, _ = f.manager.DirectExecute(ctx, row.ExecutionID, "a@example.invalid", nil) + _, _, _ = f.manager.DirectExecute(ctx, row.ExecutionID, "a@example.invalid", nil, nil) } cron := func(row *config.PurchaseExecution) { _, _ = f.manager.claimAndExecute(ctx, rowStatusCopy(row)) } redrive := func(row *config.PurchaseExecution) { _, _ = f.manager.claimAndRedrive(ctx, rowStatusCopy(row)) } @@ -170,7 +170,7 @@ func TestSingleClaim_ReaperInterleavingRecoversPurchasedRow(t *testing.T) { require.Eventually(t, func() bool { return rowStatus(ctx, t, f, row.ExecutionID).Status == "failed" }, 5*time.Second, 10*time.Millisecond) } - _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", nil) + _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", nil, nil) require.NoError(t, err) close(rs.release) <-reaped @@ -203,7 +203,7 @@ func TestSingleClaim_ExecutorArrivingMidApproveCannotBuyAgain(t *testing.T) { c.Status = "approved" midClaimed, _ = f.manager.claimAndExecute(ctx, c) } - _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", nil) + _, _, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "a@example.invalid", nil, nil) require.NoError(t, err) assert.False(t, midClaimed, "the row is running, so a second executor loses the claim") assert.Equal(t, 1, f.purchaseCount()) diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index 0f1780b2f..ebdbb78f6 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -28,17 +28,18 @@ 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. - ApproveExecution(ctx context.Context, execID, token, actor string) (string, error) - ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + // 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) + 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. - DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*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 // nothing else consumes (issue #218). - RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) + RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) CancelExecution(ctx context.Context, execID, token, actor string) error // ReapStuckExecutions sweeps purchase_executions stuck in // approved/running longer than reapAfter and flips them to "failed" diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index d26782975..bf6559c14 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -42,10 +42,10 @@ 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) (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) + ApproveExecutionFunc func(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (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) CancelExecutionFunc func(ctx context.Context, execID, token, actor string) error ReapStuckExecutionsFunc func(ctx context.Context, reapAfter time.Duration) (*purchase.ReapResult, error) FireScheduledDelayedPurchasesFunc func(ctx context.Context) (*purchase.FireResult, error) @@ -73,30 +73,30 @@ func (m *MockPurchaseManager) ProcessMessage(ctx context.Context, body string) e return nil } -func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string) (string, error) { +func (m *MockPurchaseManager) ApproveExecution(ctx context.Context, execID, token, actor string, check purchase.ClaimedRowCheck) (string, error) { if m.ApproveExecutionFunc != nil { - return m.ApproveExecutionFunc(ctx, execID, token, actor) + return m.ApproveExecutionFunc(ctx, execID, token, actor, check) } return "", nil } -func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { +func (m *MockPurchaseManager) ApproveAndExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) { if m.ApproveAndExecuteFunc != nil { - return m.ApproveAndExecuteFunc(ctx, execID, actor, transitionedBy) + return m.ApproveAndExecuteFunc(ctx, execID, actor, transitionedBy, check) } return nil, "", nil } -func (m *MockPurchaseManager) DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { +func (m *MockPurchaseManager) DirectExecute(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) { if m.DirectExecuteFunc != nil { - return m.DirectExecuteFunc(ctx, execID, actor, transitionedBy) + return m.DirectExecuteFunc(ctx, execID, actor, transitionedBy, check) } return nil, "", nil } -func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string) (*config.PurchaseExecution, string, error) { +func (m *MockPurchaseManager) RunPlannedPurchaseNow(ctx context.Context, execID, actor string, transitionedBy *string, check purchase.ClaimedRowCheck) (*config.PurchaseExecution, string, error) { if m.RunPlannedPurchaseNowFunc != nil { - return m.RunPlannedPurchaseNowFunc(ctx, execID, actor, transitionedBy) + return m.RunPlannedPurchaseNowFunc(ctx, execID, actor, transitionedBy, check) } return nil, "", nil }