diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 9a690e08e..26a25deb2 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2297,11 +2297,11 @@ func normalizeCapacityPercent(execReq *ExecutePurchaseRequest) error { } // validateExecutePurchaseRecommendations runs the per-rec #643 boundary -// validation over every rec in a web execute request, returning the first -// failure. On success it also returns the payment-option coercions that -// occurred (nil-free, in rec order) so the response can surface them to the -// caller (#1503 follow-up). Extracted so validateExecutePurchaseRequest stays -// under the gocyclo threshold. +// validation over every rec in a web execute request, then the one-account- +// per-batch rule (#1902), returning the first failure. On success it also +// returns the payment-option coercions that occurred (nil-free, in rec order) +// so the response can surface them to the caller (#1503 follow-up). Extracted +// so validateExecutePurchaseRequest stays under the gocyclo threshold. func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) ([]PaymentAdjustment, error) { var adjustments []PaymentAdjustment for i := range recs { @@ -2313,6 +2313,13 @@ func validateExecutePurchaseRecommendations(recs []config.RecommendationRecord) adjustments = append(adjustments, *adjustment) } } + // One plan-less execution targets exactly one cloud account (#1902). + // Same rule the executor enforces in resolveSingleAccountProvider, applied + // here so the batch is refused before an execution row exists, an + // approval email goes out, or a History row lands as failed. + if _, err := purchase.SingleCloudAccountIDFromRecs(recs); err != nil { + return nil, NewClientError(400, err.Error()) + } return adjustments, nil } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index bc3075325..e4a1a80e4 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4010,6 +4010,65 @@ func TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner(t *testing.T) { assert.Equal(t, true, resultMap["direct_execute"]) } +// TestHandler_executePurchase_MultiAccountBatch_Rejected is the #1902 boundary +// guard: a web execute request whose recommendations span two cloud accounts +// is refused with a 400 naming both accounts before an execution row is +// persisted, in both submit modes. The executor enforces the same rule +// (purchase.SingleCloudAccountIDFromRecs); this test pins the early refusal so +// a multi-account batch never becomes a pending row that fails at approval. +func TestHandler_executePurchase_MultiAccountBatch_Rejected(t *testing.T) { + const multiAccountBody = `{ + "recommendations": [ + {"id": "rec-a", "provider": "aws", "service": "ec2", "region": "us-east-1", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 500.0, "savings": 100.0, "selected": true, "cloud_account_id": "acct-a"}, + {"id": "rec-b", "provider": "aws", "service": "ec2", "region": "us-east-1", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 700.0, "savings": 120.0, "selected": true, "cloud_account_id": "acct-b"} + ]%s +}` + cases := []struct { + name string + modeField string + }{ + {name: "direct mode", modeField: `, "execute_mode": "direct"`}, + {name: "approval mode", modeField: ``}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockPurchase := new(MockPurchaseManager) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.On("HasPermissionAPI", ctx, session.UserID, "execute", "purchases").Return(true, nil) + mockAuth.allowConstraintChecks() + mockAuth.On("GetAllowedAccountsAPI", ctx, session.UserID).Return([]string{}, nil) + // Registered as optional so a pre-fix run (which persists the row, + // then direct-executes or emails) fails on the assertions below + // rather than on an unexpected-call panic. + mockAuth.On("HasPermissionAPI", ctx, session.UserID, "execute-any", "purchases").Return(true, nil).Maybe() + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil).Maybe() + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil).Maybe() + mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth, purchase: mockPurchase} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: fmt.Sprintf(multiAccountBody, tc.modeField), + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.Error(), "2 cloud accounts (acct-a, acct-b)") + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} + // TestHandler_executePurchase_DirectExec_FourEyesOn_DeniesSelfExecute is the // true end-to-end regression test for the HIGH finding on PR #1500's // adversarial review: execute_mode="direct" bypassed 4-eyes mode entirely diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 32210f746..e6273a8d4 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strconv" "strings" "time" @@ -26,8 +27,11 @@ import ( // When the plan has associated cloud accounts and a credential store is configured, // it fans out execution in parallel — one goroutine per account, each with its own // PurchaseExecution record tagged with cloud_account_id. -// If no accounts are configured or no credential store is available, it falls back -// to single-account execution using ambient credentials. +// If no accounts are configured or no credential store is available, it takes the +// single-account path, which resolves credentials from the recommendations' own +// cloud account and refuses a batch spanning more than one (see #1902). Only a +// batch whose selected recommendations carry no account at all falls back to +// ambient credentials. // executePurchase runs the purchase for a single execution. When the plan has // associated cloud accounts it fans out via executeMultiAccount (which saves its // own per-account records); otherwise it runs the single-account path. The root @@ -39,9 +43,10 @@ func (m *Manager) executePurchase(ctx context.Context, exec *config.PurchaseExec // with no associated plan. PlanID is empty and the Postgres UUID // column rejects "" with SQLSTATE 22P02, so skip the plan/accounts // fetch entirely and synthesize a placeholder plan whose Name is the - // only field downstream history/notification code reads. By - // definition direct-execute purchases target a single account, so - // fall straight through to the legacy single-account path. + // only field downstream history/notification code reads. Direct-execute + // purchases target exactly one account: resolveSingleAccountProvider + // refuses a batch whose selected recs span accounts (#1902), so fall + // straight through to the single-account path. var plan *config.PurchasePlan if exec.PlanID == "" { plan = &config.PurchasePlan{Name: "Direct purchase"} @@ -351,9 +356,12 @@ func applyAccountOutcome(acctExec *config.PurchaseExecution, purchaseErrors []st // caller to fall back to the ambient AWS STS identity. // // The account is taken from exec.CloudAccountID when set (plan-with-single- -// account executions), or derived from the shared cloud_account_id on the -// recommendations when all selected recs agree on exactly one account (direct- -// execute purchases where PlanID is empty and exec.CloudAccountID is nil). +// account executions), or derived from the shared cloud_account_id of the +// SELECTED recommendations (direct-execute purchases where PlanID is empty and +// exec.CloudAccountID is nil). When the selected recs span more than one +// account, or mix attributed and unattributed recs, this returns +// errAmbiguousAccountScope: the single-account path cannot honor such a +// batch and must never fall back to ambient credentials for it (#1902). // // A non-nil error means a target account was identified but could not be // resolved (lookup failed, the account does not exist, or credentials could @@ -370,7 +378,11 @@ func (m *Manager) resolveSingleAccountProvider(ctx context.Context, exec *config cloudAccountID := exec.CloudAccountID if cloudAccountID == nil { - cloudAccountID = singleCloudAccountIDFromRecs(exec.Recommendations) + var scopeErr error + cloudAccountID, scopeErr = SingleCloudAccountIDFromRecs(exec.Recommendations) + if scopeErr != nil { + return nil, "", fmt.Errorf("execution %s: %w", exec.ExecutionID, scopeErr) + } } if cloudAccountID == nil { return nil, "", nil @@ -863,26 +875,49 @@ func indexKeys(idx []int) []string { return out } -// singleCloudAccountIDFromRecs returns the shared cloud_account_id when -// all recommendations in the slice agree on exactly one non-empty account ID. -// Returns nil when the slice is empty, all IDs are absent, or more than one -// distinct ID is present (the caller falls back to ambient credentials in the -// first two cases and to fan-out in the last). -func singleCloudAccountIDFromRecs(recs []config.RecommendationRecord) *string { +// errAmbiguousAccountScope is returned when the selected recommendations of a +// plan-less execution do not resolve to exactly one cloud account. The +// single-account path cannot honor such a batch: buying it under ambient +// credentials (the pre-#1902 behavior) purchases every commitment in the +// CUDly host account and stamps the ambient identity on history (#646). +var errAmbiguousAccountScope = errors.New("selected recommendations do not resolve to a single cloud account") + +// SingleCloudAccountIDFromRecs returns the cloud_account_id shared by every +// SELECTED recommendation, nil when no selected recommendation carries one +// (the ambient single-account deployment), and errAmbiguousAccountScope when +// the selected recs span more than one account or mix attributed and +// unattributed entries (#1902). Only selected recs count: they are the recs +// the money moves for (processPurchaseRecommendations buys selectedIndices). +// Exported so the API boundary (validateExecutePurchaseRecommendations) +// applies the identical rule before an execution row exists. +func SingleCloudAccountIDFromRecs(recs []config.RecommendationRecord) (*string, error) { + var ids []string var found *string + selected, unattributed := 0, 0 for i := range recs { - id := recs[i].CloudAccountID - if id == nil || *id == "" { + rec := &recs[i] + if !rec.Selected { continue } - if found == nil { - found = id - } else if *found != *id { - // More than one distinct account — not the single-account path. - return nil + selected++ + if rec.CloudAccountID == nil || *rec.CloudAccountID == "" { + unattributed++ + continue + } + if !slices.Contains(ids, *rec.CloudAccountID) { + ids = append(ids, *rec.CloudAccountID) + found = rec.CloudAccountID } } - return found + switch { + case len(ids) > 1: + return nil, fmt.Errorf("%w: %d selected recommendation(s) target %d cloud accounts (%s); submit one purchase per account", + errAmbiguousAccountScope, selected, len(ids), strings.Join(ids, ", ")) + case len(ids) == 1 && unattributed > 0: + return nil, fmt.Errorf("%w: %d selected recommendation(s) carry no cloud_account_id while %d target account %s", + errAmbiguousAccountScope, unattributed, selected-unattributed, ids[0]) + } + return found, nil } // savePurchaseHistory persists one purchase_history row for a successful diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 021e48bb5..1d91e702d 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1579,15 +1579,20 @@ func TestExecutePurchase_SingleAccount_AccountNotFound(t *testing.T) { } // TestSingleCloudAccountIDFromRecs covers the helper that derives a shared -// cloud_account_id from a recommendation slice. +// cloud_account_id from the SELECTED recommendations of a plan-less +// execution, and the #1902 ambiguous-scope errors it returns for the +// multi-account and mixed attributed/unattributed shapes. func TestSingleCloudAccountIDFromRecs(t *testing.T) { aid1 := "acct-1" aid2 := "acct-2" + aid3 := "acct-3" tests := []struct { - want *string - name string - recs []config.RecommendationRecord + want *string + name string + recs []config.RecommendationRecord + wantErr bool + wantMsg string }{ { name: "empty slice returns nil", @@ -1595,50 +1600,83 @@ func TestSingleCloudAccountIDFromRecs(t *testing.T) { want: nil, }, { - name: "all nil account IDs returns nil", + name: "all unattributed returns nil", recs: []config.RecommendationRecord{ - {CloudAccountID: nil}, - {CloudAccountID: nil}, + {Selected: true, CloudAccountID: nil}, + {Selected: true, CloudAccountID: nil}, }, want: nil, }, { - name: "single rec with account ID returns it", + name: "single attributed rec returns it", recs: []config.RecommendationRecord{ - {CloudAccountID: &aid1}, + {Selected: true, CloudAccountID: &aid1}, }, want: &aid1, }, { - name: "all recs share same account ID returns it", + name: "all recs share one account", recs: []config.RecommendationRecord{ - {CloudAccountID: &aid1}, - {CloudAccountID: &aid1}, + {Selected: true, CloudAccountID: &aid1}, + {Selected: true, CloudAccountID: &aid1}, }, want: &aid1, }, { - name: "mixed nil and same non-nil returns the non-nil ID", + name: "unselected rec from another account is ignored", recs: []config.RecommendationRecord{ - {CloudAccountID: nil}, - {CloudAccountID: &aid1}, - {CloudAccountID: &aid1}, + {Selected: true, CloudAccountID: &aid1}, + {Selected: false, CloudAccountID: &aid2}, }, want: &aid1, }, { - name: "two distinct account IDs returns nil (multi-account, not this path)", + name: "only unselected recs returns nil", recs: []config.RecommendationRecord{ - {CloudAccountID: &aid1}, - {CloudAccountID: &aid2}, + {Selected: false, CloudAccountID: &aid1}, }, want: nil, }, + { + name: "two distinct accounts is an error", + recs: []config.RecommendationRecord{ + {Selected: true, CloudAccountID: &aid1}, + {Selected: true, CloudAccountID: &aid2}, + }, + wantErr: true, + wantMsg: "2 cloud accounts (acct-1, acct-2)", + }, + { + name: "mixed attributed and unattributed is an error", + recs: []config.RecommendationRecord{ + {Selected: true, CloudAccountID: nil}, + {Selected: true, CloudAccountID: &aid1}, + }, + wantErr: true, + wantMsg: "carry no cloud_account_id", + }, + { + name: "three distinct accounts lists all three in order", + recs: []config.RecommendationRecord{ + {Selected: true, CloudAccountID: &aid1}, + {Selected: true, CloudAccountID: &aid2}, + {Selected: true, CloudAccountID: &aid3}, + }, + wantErr: true, + wantMsg: "(acct-1, acct-2, acct-3)", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := singleCloudAccountIDFromRecs(tc.recs) + got, err := SingleCloudAccountIDFromRecs(tc.recs) + if tc.wantErr { + require.ErrorIs(t, err, errAmbiguousAccountScope) + assert.Contains(t, err.Error(), tc.wantMsg) + assert.Nil(t, got) + return + } + require.NoError(t, err) if tc.want == nil { assert.Nil(t, got) } else { diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index efe02beac..24a407ae8 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -9,6 +9,8 @@ import ( "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -606,3 +608,101 @@ func TestRootKeyStillFansOutThroughExecutePurchase(t *testing.T) { "acct-B": "root-lineage:acct-B", }, perAccountKey, "every plan account must still be purchased under its own per-account lineage key") } + +// TestPlanLessExecutionRefusesAmbiguousAccountScope is the #1902 (audit +// A05-001 / A05-002) regression guard. A plan-less execution whose SELECTED +// recommendations span two cloud accounts, or mix attributed and unattributed +// recs, must fail before any provider client is built. Pre-fix the resolver +// returned a nil provider config for both shapes and the factory built a +// client from the host's ambient credentials, so every commitment was bought +// in the CUDly host account and history was stamped with the ambient STS +// identity. Every collaborator below is registered with .Maybe() so a pre-fix +// run reaches the assertions instead of panicking inside the rec fan-out +// goroutines; the AssertNotCalled lines are what fail pre-fix. +func TestPlanLessExecutionRefusesAmbiguousAccountScope(t *testing.T) { + const hostAccount = "999999999999" // ambient STS identity; must never reach history + acctA, acctB := "acct-a", "acct-b" + + cases := []struct { + name string + recs []config.RecommendationRecord + wantInError []string + }{ + { + name: "two distinct accounts", + recs: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 100, Savings: 20, Selected: true, CloudAccountID: &acctA}, + {Provider: "aws", Service: "ec2", ResourceType: "m5.xlarge", Region: "us-east-1", Count: 1, UpfrontCost: 200, Savings: 40, Selected: true, CloudAccountID: &acctB}, + }, + wantInError: []string{"do not resolve to a single cloud account", "2 cloud accounts (acct-a, acct-b)"}, + }, + { + name: "attributed and unattributed mixed", + recs: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 100, Savings: 20, Selected: true, CloudAccountID: &acctA}, + {Provider: "aws", Service: "ec2", ResourceType: "m5.xlarge", Region: "us-east-1", Count: 1, UpfrontCost: 200, Savings: 40, Selected: true}, + }, + wantInError: []string{"do not resolve to a single cloud account", "1 selected recommendation(s) carry no cloud_account_id while 1 target account acct-a"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockSTS := new(MockSTSClient) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + + exec := &config.PurchaseExecution{ + ExecutionID: "exec-1902-" + tc.name, + PlanID: "", // plan-less: direct execute / approval of a web submission + Source: common.PurchaseSourceWeb, + Recommendations: tc.recs, + } + + // Everything a pre-fix run would touch, all optional, so the run + // completes and the negative assertions below carry the failure. + mockStore.On("GetCloudAccount", ctx, mock.AnythingOfType("string")). + Return(&config.CloudAccount{ID: acctA, Provider: "aws", AWSAuthMode: "access_keys", ExternalID: "111111111111"}, nil).Maybe() + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Maybe() + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil).Maybe() + mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")). + Return(&sts.GetCallerIdentityOutput{Account: aws.String(hostAccount)}, nil).Maybe() + mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil).Maybe() + mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil).Maybe() + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")). + Return(common.PurchaseResult{Success: true, CommitmentID: "ri-1902"}, nil).Maybe() + + manager := &Manager{ + config: mockStore, + email: mockEmail, + stsClient: mockSTS, + providerFactory: mockFactory, + credStore: awsAccessKeyCredStore(), + dashboardURL: "https://dashboard.example.com", + } + + err := manager.executePurchase(ctx, exec) + + require.Error(t, err, "a plan-less batch that does not resolve to one account must be refused") + for _, want := range tc.wantInError { + assert.Contains(t, err.Error(), want) + } + // The money-path facts: no provider client of any kind was built + // (ambient or per-account), nothing was bought, nothing was stamped + // on history, no confirmation went out, no account was even looked up. + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) + mockServiceClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "SavePurchaseHistory", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetCloudAccount", mock.Anything, mock.Anything) + mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything) + for i := range exec.Recommendations { + assert.False(t, exec.Recommendations[i].Purchased, "rec %d must not be marked purchased", i) + assert.Empty(t, exec.Recommendations[i].PurchaseID) + } + }) + } +}