diff --git a/internal/api/claimed_row_check_test.go b/internal/api/claimed_row_check_test.go index abe82788d..fe06bc22f 100644 --- a/internal/api/claimed_row_check_test.go +++ b/internal/api/claimed_row_check_test.go @@ -28,7 +28,7 @@ const ( ) func claimRecs(upfront float64) []config.RecommendationRecord { - return []config.RecommendationRecord{{ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: upfront}} + return []config.RecommendationRecord{{ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: upfront, CommitmentCurrency: "USD"}} } func cappedPurchasePerm(action string) auth.Permission { @@ -209,6 +209,7 @@ func directExecuteClaimHarness(t *testing.T, mp *MockPurchaseManager, claimedUpf 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, + CommitmentCurrency: "USD", }}, } mp.On("DirectExecute", ctx, mock.AnythingOfType("string"), session.Email, mock.Anything).Return(nil, "", nil).Maybe() diff --git a/internal/api/combined_plan_purchase_integration_test.go b/internal/api/combined_plan_purchase_integration_test.go index 7d94a6e90..c35a15cec 100644 --- a/internal/api/combined_plan_purchase_integration_test.go +++ b/internal/api/combined_plan_purchase_integration_test.go @@ -118,6 +118,13 @@ func combinedRoute(t *testing.T, h *Handler, method, path string, body any) even } func newCombinedPlanFixture(t *testing.T, automatic bool, capAmount *float64) *combinedPlanFixture { + t.Helper() + return newCombinedPlanFixtureIn(t, automatic, capAmount, "USD") +} + +// newCombinedPlanFixtureIn is newCombinedPlanFixture with the stored +// recommendation priced in currency (issue #361). +func newCombinedPlanFixtureIn(t *testing.T, automatic bool, capAmount *float64, currency string) *combinedPlanFixture { t.Helper() ctx := context.Background() f := &combinedPlanFixture{createConcurrencyFixture: newCreateConcurrencyFixture(ctx, t)} @@ -160,6 +167,7 @@ func newCombinedPlanFixture(t *testing.T, automatic bool, capAmount *float64) *c require.NoError(t, f.store.UpsertRecommendations(ctx, time.Now().UTC(), []config.RecommendationRecord{{ ID: "combined-azure-rec", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", Count: 100, Term: 1, Payment: "monthly", MonthlyCost: &monthly, Savings: 500, CloudAccountID: &f.account, + CommitmentCurrency: currency, }}, nil)) f.service = &combinedService{store: f.store} factory := combinedFactory{provider: &combinedProvider{service: f.service}} @@ -317,6 +325,32 @@ func TestCombinedPlanPurchase_ManualOffRoutedApproval(t *testing.T) { f.assertCompleted(t) } +// TestCombinedPlanPurchase_NonUSDApprovalRefusedUnderCap (issue #361): the +// stored recommendation is priced at 2,400 EUR. The currency must survive the +// recommendations payload, the plan-step attach and the execution row, and a +// USD approve-any cap of 2,400 must refuse it instead of comparing +// 2400 <= 2400. A grant with no amount cap still approves it. +func TestCombinedPlanPurchase_NonUSDApprovalRefusedUnderCap(t *testing.T) { + f := newCombinedPlanFixtureIn(t, false, nil, "EUR") + f.assertAttached(t) + stored, err := f.store.GetExecutionByID(context.Background(), f.root.ExecutionID) + require.NoError(t, err) + require.Len(t, stored.Recommendations, 1) + assert.Equal(t, "EUR", stored.Recommendations[0].CommitmentCurrency, "the currency must round-trip to the execution row") + approver := uuid.NewString() + _, err = f.pool.Exec(context.Background(), `INSERT INTO users (id,email,password_hash,salt,active,group_ids,created_at,updated_at) SELECT $1,'approver@example.invalid','x','y',true,ARRAY[g.id],now(),now() FROM groups g ORDER BY g.name LIMIT 1`, approver) + require.NoError(t, err) + for _, capAmount := range []float64{2400, 1_000_000} { + response := f.approveAs(t, approver, capAmount) + assert.Equal(t, 403, response.StatusCode, response.Body) + assert.Contains(t, response.Body, "EUR") + } + assert.Empty(t, f.service.recorded()) + response := f.approveAs(t, approver, 0) + require.Equal(t, 200, response.StatusCode, response.Body) + f.assertCompleted(t) +} + func TestCombinedPlanPurchase_AutomaticOnScheduledExecution(t *testing.T) { capAmount := 2400.0 f := newCombinedPlanFixture(t, true, &capAmount) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ce5212ded..d52e266a9 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -3001,12 +3001,17 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // runPlannedPurchase, "execute-any"/"execute-own" for direct-execute, // "approve-any"/"approve-own" for session approve. recs carry store-derived // or persisted-at-submit-time costs depending on the caller; none of these -// call sites ever price from client-supplied numbers. +// call sites ever price from client-supplied numbers. MaxPurchaseAmount is +// USD, so a batch with any rec not provably priced in USD is checked by +// requireUncappedPurchaseGrant instead of by its raw amount (issue #361). func (h *Handler) enforcePurchaseConstraints(ctx context.Context, session *Session, action string, recs []config.RecommendationRecord) error { constraintSets := purchaseConstraintSets(recs) if err := requireNonZeroCommitment(constraintSets); err != nil { return err } + if currency := nonUSDPurchaseCurrency(recs); currency != "" { + return h.requireUncappedPurchaseGrant(ctx, session, action, constraintSets, currency) + } return h.requirePermissionConstraints(ctx, session, action, "purchases", constraintSets) } diff --git a/internal/api/handler_purchases_plan_step_approve_test.go b/internal/api/handler_purchases_plan_step_approve_test.go index 418a4ec08..85e33210d 100644 --- a/internal/api/handler_purchases_plan_step_approve_test.go +++ b/internal/api/handler_purchases_plan_step_approve_test.go @@ -51,6 +51,7 @@ func planStepExecution(t *testing.T) (*config.PurchaseExecution, float64) { ID: "azure|acct-step|compute|westeurope|Standard_D2s_v3||1|monthly", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", Term: 1, Payment: "monthly", Count: 20, UpfrontCost: 0, MonthlyCost: &monthly, CloudAccountID: &acct, + CommitmentCurrency: "USD", } plan := &config.PurchasePlan{ ID: stepPlanID, diff --git a/internal/api/handler_purchases_retry_fanout_test.go b/internal/api/handler_purchases_retry_fanout_test.go index dfe371e2c..4e6d2f6ff 100644 --- a/internal/api/handler_purchases_retry_fanout_test.go +++ b/internal/api/handler_purchases_retry_fanout_test.go @@ -59,6 +59,7 @@ func fanoutRecs() []config.RecommendationRecord { Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, Term: 1, UpfrontCost: 300, Selected: true, + CommitmentCurrency: "USD", }, } } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 2bda5e715..d533ad32b 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -454,7 +454,7 @@ func TestHandler_approvePurchase_SessionApproveAny_PermissionConstraintsDenied(t Status: "pending", CreatedByUserID: &otherCreator, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000}, + {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CommitmentCurrency: "USD"}, }, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) @@ -528,7 +528,7 @@ func TestHandler_approvePurchase_SessionApproveAny_TokenPresent_ConstraintsDenie Status: "pending", CreatedByUserID: &otherCreator, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CloudAccountID: &accountID}, + {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CloudAccountID: &accountID, CommitmentCurrency: "USD"}, }, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) @@ -581,7 +581,7 @@ func TestHandler_authorizeSessionApprove_ApproveOwn_ConstraintsDenied(t *testing ExecutionID: "exec-1", CreatedByUserID: &userID, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000}, + {Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CommitmentCurrency: "USD"}, }, } @@ -677,7 +677,7 @@ func TestHandler_approveViaToken_SessionApproveAny_ConstraintsDenied(t *testing. ApprovalToken: "valid-token", Status: "pending", Recommendations: []config.RecommendationRecord{ - {ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CloudAccountID: &accountID}, + {ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CloudAccountID: &accountID, CommitmentCurrency: "USD"}, }, } // capped's own email is the account's registered contact_email, so @@ -1711,7 +1711,7 @@ func TestHandler_runPlannedPurchase_ConstraintDenied(t *testing.T) { Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 100}, }, - []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000}}, + []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CommitmentCurrency: "USD"}}, ) require.Error(t, err) ce, ok := IsClientError(err) @@ -4257,7 +4257,7 @@ func TestHandler_retryPurchase_PermissionConstraintsDenied(t *testing.T) { Error: "send failed: transient SES throttle", CreatedByUserID: &creator, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "rds", Region: "us-east-1", Term: 1, UpfrontCost: 5000}, + {Provider: "aws", Service: "rds", Region: "us-east-1", Term: 1, UpfrontCost: 5000, CommitmentCurrency: "USD"}, }, } session := &Session{UserID: retryCallerID, Email: "capped@example.com"} @@ -4485,6 +4485,7 @@ func setupDirectExecMocks(ctx context.Context, store *MockConfigStore) { // #1905: directExecRecBody's rec-1 is now priced from the stored set. expectStoredRecs(store, config.RecommendationRecord{ ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100, + CommitmentCurrency: "USD", }) } @@ -4581,8 +4582,8 @@ func TestHandler_executePurchase_PermissionConstraintsDenied(t *testing.T) { })).Return(false, nil) // #1905: constraint sets are now built from the stored costs. expectStoredRecs(mockStore, - config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 3000, Savings: 50}, - config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "eu-west-1", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 2500, Savings: 100}, + config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 3000, Savings: 50, CommitmentCurrency: "USD"}, + config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "eu-west-1", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 2500, Savings: 100, CommitmentCurrency: "USD"}, ) handler := &Handler{config: mockStore, auth: mockAuth} @@ -4645,6 +4646,7 @@ func TestHandler_executePurchase_NoUpfrontBatch_TotalCommitmentEnforced(t *testi expectStoredRecs(mockStore, config.RecommendationRecord{ Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 3, Payment: "no-upfront", UpfrontCost: 0, MonthlyCost: float64Ptr(600), Savings: 50, + CommitmentCurrency: "USD", }) handler := &Handler{config: mockStore, auth: mockAuth} @@ -4707,6 +4709,7 @@ func TestHandler_executePurchase_UserAPIKeyConstraintsDenied(t *testing.T) { // #1905: the constraint set is now built from the stored cost. expectStoredRecs(mockStore, config.RecommendationRecord{ Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 50, + CommitmentCurrency: "USD", }) handler := &Handler{config: mockStore, auth: mockAuth} @@ -5130,7 +5133,7 @@ func TestHandler_authorizeSessionExecuteDirect_AdminGroupViaExecuteAny(t *testin adminUserID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" creatorID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" session := &Session{UserID: adminUserID} - recs := []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 500}} + recs := []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 500, CommitmentCurrency: "USD"}} // Administrators-group wildcard {admin,*} covers execute-any. mockAuth.On("HasPermissionAPI", ctx, adminUserID, "execute-any", "purchases").Return(true, nil) @@ -5157,7 +5160,7 @@ func TestHandler_authorizeSessionExecuteDirect_ExecuteAny_ConstraintsDenied(t *t userID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" creatorID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" session := &Session{UserID: userID} - recs := []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000}} + recs := []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CommitmentCurrency: "USD"}} mockAuth.On("HasPermissionAPI", ctx, userID, "execute-any", "purchases").Return(true, nil) mockAuth.On("HasPermissionForConstraintsAPI", ctx, userID, "execute-any", "purchases", @@ -5183,7 +5186,7 @@ func TestHandler_authorizeSessionExecuteDirect_ExecuteOwn_ConstraintsDenied(t *t userID := "cccccccc-cccc-cccc-cccc-cccccccccccc" session := &Session{UserID: userID} - recs := []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000}} + recs := []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 50000, CommitmentCurrency: "USD"}} mockAuth.On("HasPermissionAPI", ctx, userID, "execute-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "execute-own", "purchases").Return(true, nil) diff --git a/internal/api/negative_purchase_cap_test.go b/internal/api/negative_purchase_cap_test.go index 875f94485..f1f78df75 100644 --- a/internal/api/negative_purchase_cap_test.go +++ b/internal/api/negative_purchase_cap_test.go @@ -21,6 +21,7 @@ func capRec(upfront float64) config.RecommendationRecord { return config.RecommendationRecord{ ID: "rec-1", Provider: "aws", Service: "ec2", Region: "us-east-1", CloudAccountID: &acct, Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: upfront, + CommitmentCurrency: "USD", } } diff --git a/internal/api/purchase_currency.go b/internal/api/purchase_currency.go new file mode 100644 index 000000000..99f23f9bf --- /dev/null +++ b/internal/api/purchase_currency.go @@ -0,0 +1,79 @@ +package api + +import ( + "context" + "fmt" + "math" + "strings" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// purchaseCapCurrency is the currency every purchases MaxPurchaseAmount +// (group permission and API-key max_amount) is denominated in. The cap has +// no currency field of its own (PR #1515): it is USD by construction, and +// there is no FX conversion here. +const purchaseCapCurrency = "USD" + +// nonUSDPurchaseCurrency returns "" when every rec's CommitmentCurrency is +// USD, so its total commitment can be compared against a USD cap. Otherwise +// it returns the first rec's currency that is not USD, or "unknown" when a +// rec carries none (issue #361). +func nonUSDPurchaseCurrency(recs []config.RecommendationRecord) string { + for i := range recs { + currency := strings.TrimSpace(recs[i].CommitmentCurrency) + if strings.EqualFold(currency, purchaseCapCurrency) { + continue + } + if currency == "" { + return "unknown" + } + return currency + } + return "" +} + +// requireUncappedPurchaseGrant enforces sets for a batch whose amounts are +// not provably USD. Comparing the raw amount against a USD cap is wrong in +// whichever direction the exchange rate points: 900 KWD (about 2,900 USD) +// clears a 1,000 USD cap, and 5,000 CNY (about 700 USD) is refused by it. +// So the amount is replaced with an unmatchable sentinel +// (math.MaxFloat64): a grant with ANY MaxPurchaseAmount refuses it, and a +// grant with no amount cap still allows it. This mirrors +// checkAzureExecuteConstraints on the Azure RI exchange path. +// +// When the sentinel check is refused, a second check with the amount +// dimension neutralized (0, which a well-formed cap always satisfies) +// tells the two causes apart: if it is refused too, another dimension +// (account, provider, service, region) is the reason and that error is +// returned unchanged; otherwise the spend cap is, and a 403 naming the +// currency is returned. +func (h *Handler) requireUncappedPurchaseGrant(ctx context.Context, session *Session, action string, sets []auth.PermissionConstraints, currency string) error { + err := h.requirePermissionConstraints(ctx, session, action, "purchases", withPurchaseAmount(sets, math.MaxFloat64)) + if err == nil { + return nil + } + if ce, ok := IsClientError(err); !ok || ce.code != 403 { + return err + } + if otherErr := h.requirePermissionConstraints(ctx, session, action, "purchases", withPurchaseAmount(sets, 0)); otherErr != nil { + return otherErr + } + return NewClientError(403, fmt.Sprintf( + "permission denied: the constraints configured on your %s permission for purchases include a USD spend cap (MaxPurchaseAmount), "+ + "and this purchase is priced in %s currency, so the cap cannot be enforced against it; "+ + "refresh the recommendations so they carry a USD price, or ask an administrator to remove the cap", + action, currency)) +} + +// withPurchaseAmount returns a copy of sets with every MaxPurchaseAmount set +// to amount, leaving the caller's slice untouched. +func withPurchaseAmount(sets []auth.PermissionConstraints, amount float64) []auth.PermissionConstraints { + out := make([]auth.PermissionConstraints, len(sets)) + for i := range sets { + out[i] = sets[i] + out[i].MaxPurchaseAmount = amount + } + return out +} diff --git a/internal/api/purchase_currency_test.go b/internal/api/purchase_currency_test.go new file mode 100644 index 000000000..fbae84bf3 --- /dev/null +++ b/internal/api/purchase_currency_test.go @@ -0,0 +1,151 @@ +package api + +// The purchases MaxPurchaseAmount cap is USD, so a purchase whose amounts are +// not provably USD is never compared against it (issue #361). The mock auth +// service answers constraint checks with the real +// auth.PermissionsAllowForConstraintSets, so these exercise the production +// matcher. Nothing here reaches a purchase manager or provider. + +import ( + "context" + "errors" + "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" +) + +func currencyRec(upfront float64, currency string) config.RecommendationRecord { + rec := capRec(upfront) + rec.CommitmentCurrency = currency + return rec +} + +func requireCurrencyRefusal(t *testing.T, err error, currency string) { + 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.Error(), "USD spend cap") + assert.Contains(t, ce.Error(), "priced in "+currency+" currency") +} + +// TestEnforcePurchaseConstraints_NonUSDUnderCapRefused fails on main: there a +// 900 EUR (or 900 of an unknown currency) purchase cleared a 1,000 USD cap +// because 900 <= 1000. +func TestEnforcePurchaseConstraints_NonUSDUnderCapRefused(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + for _, action := range []string{auth.ActionExecute, auth.ActionExecuteAny, auth.ActionApproveAny, auth.ActionApproveOwn} { + for _, tc := range []struct{ currency, want string }{ + {"EUR", "EUR"}, {"JPY", "JPY"}, {"", "unknown"}, {" ", "unknown"}, + } { + t.Run(action+"/"+tc.want, func(t *testing.T) { + h := handlerWithCap(action, 1000) + err := h.enforcePurchaseConstraints(ctx, session, action, []config.RecommendationRecord{currencyRec(900, tc.currency)}) + requireCurrencyRefusal(t, err, tc.want) + }) + } + } +} + +func TestEnforcePurchaseConstraints_MixedBatchRefused(t *testing.T) { + h := handlerWithCap(auth.ActionExecute, 1000) + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + err := h.enforcePurchaseConstraints(context.Background(), session, auth.ActionExecute, + []config.RecommendationRecord{currencyRec(100, "USD"), currencyRec(100, "CNY")}) + requireCurrencyRefusal(t, err, "CNY") +} + +func TestEnforcePurchaseConstraints_USDStillComparedAgainstCap(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + h := handlerWithCap(auth.ActionExecute, 1000) + for _, currency := range []string{"USD", "usd", " USD "} { + require.NoError(t, h.enforcePurchaseConstraints(ctx, session, auth.ActionExecute, []config.RecommendationRecord{currencyRec(900, currency)})) + } + err := h.enforcePurchaseConstraints(ctx, session, auth.ActionExecute, []config.RecommendationRecord{currencyRec(1100, "USD")}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "exceeds the constraints") +} + +// A grant with no amount cap has nothing to compare, so a non-USD purchase +// is not penalized. +func TestEnforcePurchaseConstraints_NonUSDWithoutCapAllowed(t *testing.T) { + h := handlerWithCap(auth.ActionExecute, 0) + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + for _, currency := range []string{"EUR", ""} { + require.NoError(t, h.enforcePurchaseConstraints(context.Background(), session, auth.ActionExecute, + []config.RecommendationRecord{currencyRec(1_000_000, currency)})) + } +} + +// When another dimension refuses the request too, that dimension's error is +// returned, not the currency one. +func TestEnforcePurchaseConstraints_NonUSDOtherDimensionKeepsItsError(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.grantPermissions([]auth.Permission{{ + Action: auth.ActionExecute, Resource: auth.ResourcePurchases, + Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000, Regions: []string{"eu-west-1"}}, + }}) + h := &Handler{auth: mockAuth} + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + err := h.enforcePurchaseConstraints(context.Background(), session, auth.ActionExecute, + []config.RecommendationRecord{currencyRec(900, "EUR")}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "exceeds the constraints") + assert.NotContains(t, ce.Error(), "USD spend cap") +} + +// TestHandler_executePurchase_NonUSDStoredPriceUnderCapRefused drives the web +// execute path: the currency comes from the stored row, never from the +// request, so a client stating "USD" cannot lift the refusal. +func TestHandler_executePurchase_NonUSDStoredPriceUnderCapRefused(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + authService := new(MockAuthService) + authService.On("ValidateSession", ctx, "eur-cap").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "capped@example.com"}, nil) + authService.grantPermissions([]auth.Permission{{Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000}}}) + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + expectStoredRecs(store, config.RecommendationRecord{ + ID: "eur-rec", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", + Count: 1, Term: 3, Payment: "upfront", UpfrontCost: 900, Savings: 50, CommitmentCurrency: "EUR", + }) + h := &Handler{config: store, auth: authService} + _, err := h.executePurchase(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer eur-cap"}, + Body: `{"recommendations":[{"id":"eur-rec","provider":"azure","service":"compute","region":"westeurope","resource_type":"Standard_D2s_v3","count":1,"term":3,"payment":"all-upfront","upfront_cost":900,"savings":50,"commitment_currency":"USD"}]}`, + }) + requireCurrencyRefusal(t, err, "EUR") + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +// An auth backend failure is returned as is: it is not a denial, so it is +// neither retried nor reported as a currency refusal. +func TestEnforcePurchaseConstraints_NonUSDBackendErrorPropagates(t *testing.T) { + boom := errors.New("auth store unavailable") + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, auth.ActionExecute, auth.ResourcePurchases, mock.Anything). + Return(false, boom).Once() + h := &Handler{auth: mockAuth} + session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"} + err := h.enforcePurchaseConstraints(context.Background(), session, auth.ActionExecute, + []config.RecommendationRecord{currencyRec(900, "EUR")}) + require.ErrorIs(t, err, boom) + _, isClient := IsClientError(err) + assert.False(t, isClient) + mockAuth.AssertNumberOfCalls(t, "HasPermissionForConstraintsAPI", 1) +} diff --git a/internal/api/purchase_pricing_test.go b/internal/api/purchase_pricing_test.go index 0ace2944a..599944151 100644 --- a/internal/api/purchase_pricing_test.go +++ b/internal/api/purchase_pricing_test.go @@ -105,7 +105,7 @@ func TestHandler_executePurchase_StoredPriceUnderCapProceeds(t *testing.T) { expectStoredRecs(mockStore, config.RecommendationRecord{ ID: "aws|123456789012|ec2|us-east-1|m5.24xlarge||3|all-upfront", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.24xlarge", - Count: 1, Term: 3, Payment: "all-upfront", UpfrontCost: 900, Savings: 30, + Count: 1, Term: 3, Payment: "all-upfront", UpfrontCost: 900, Savings: 30, CommitmentCurrency: "USD", }) mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil) @@ -419,12 +419,12 @@ func TestHandler_executePurchase_PaymentChangePricedFromStoredVariant(t *testing config.RecommendationRecord{ ID: "aws|acct|ec2|us-east-1|m5.large||1|all-upfront", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", - Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 3000, Savings: 10, + Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 3000, Savings: 10, CommitmentCurrency: "USD", }, config.RecommendationRecord{ ID: "aws|acct|ec2|us-east-1|m5.large||1|no-upfront", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", - Count: 1, Term: 1, Payment: "no-upfront", UpfrontCost: 0, MonthlyCost: float64Ptr(100), Savings: 10, + Count: 1, Term: 1, Payment: "no-upfront", UpfrontCost: 0, MonthlyCost: float64Ptr(100), Savings: 10, CommitmentCurrency: "USD", }, ) @@ -878,13 +878,13 @@ func TestHandler_executePurchase_MatchingTenancyTermChangeSucceeds(t *testing.T) config.RecommendationRecord{ ID: "aws|acct|ec2|us-east-1|m5.large||3|all-upfront", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", - Count: 1, Term: 3, Payment: "all-upfront", UpfrontCost: 3000, Savings: 300, + Count: 1, Term: 3, Payment: "all-upfront", UpfrontCost: 3000, Savings: 300, CommitmentCurrency: "USD", Details: json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"regional"}`), }, config.RecommendationRecord{ ID: "aws|acct|ec2|us-east-1|m5.large||1|all-upfront", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", - Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 1200, Savings: 100, + Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 1200, Savings: 100, CommitmentCurrency: "USD", Details: json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"regional"}`), }, ) @@ -977,7 +977,7 @@ func emptyIDExecuteHarness(t *testing.T, details string) (*config.PurchaseExecut expectStoredRecs(mockStore, config.RecommendationRecord{ ID: "aws|acct|ec2|us-east-1|m5.large||1|all-upfront", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", - Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 1200, Savings: 100, + Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 1200, Savings: 100, CommitmentCurrency: "USD", Details: json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"dedicated","scope":"regional"}`), }) @@ -1117,7 +1117,7 @@ func TestHandler_executePurchase_EmptyIDIgnoresNonDiscriminatorDetails(t *testin const stored = `{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"dedicated","scope":"regional"}` row := config.RecommendationRecord{ ID: "ec2-row", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", - Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 1200, Savings: 100, + Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 1200, Savings: 100, CommitmentCurrency: "USD", Details: json.RawMessage(stored), } saved, err := emptyIDExecuteAgainst(t, row, @@ -1186,7 +1186,7 @@ func TestAzureCorrectedPriceExceedsOldUnitsCap(t *testing.T) { authService.grantPermissions([]auth.Permission{{Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000}}}) store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() store.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() - expectStoredRecs(store, config.RecommendationRecord{ID: "azure-corrected", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", Count: 1, Term: 3, Payment: "upfront", UpfrontCost: 3600, Savings: 50}) + expectStoredRecs(store, config.RecommendationRecord{ID: "azure-corrected", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", Count: 1, Term: 3, Payment: "upfront", UpfrontCost: 3600, Savings: 50, CommitmentCurrency: "USD"}) h := &Handler{config: store, auth: authService} _, err := h.executePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer azure-cap"}, Body: `{"recommendations":[{"id":"azure-corrected","provider":"azure","service":"compute","region":"westeurope","resource_type":"Standard_D2s_v3","count":1,"term":3,"payment":"all-upfront","upfront_cost":70,"savings":50}]}`}) ce, ok := IsClientError(err) diff --git a/internal/config/types.go b/internal/config/types.go index 4b43d6a0f..4bde942b1 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -657,6 +657,15 @@ type RecommendationRecord struct { // collapses the denominator) don't inflate the displayed effective // savings %. See #274. OnDemandCost *float64 `json:"on_demand_cost,omitempty" dynamodbav:"on_demand_cost,omitempty"` + // CommitmentCurrency is the ISO 4217 code UpfrontCost and MonthlyCost are + // denominated in, copied from common.Recommendation.CommitmentCurrency at + // collection time. Empty means unknown: the provider parser could not + // prove the currency of every payment leg, or the row predates the field. + // The purchase spend caps are USD, so a rec that is not provably USD is + // never compared against one (issue #361). Persisted via the + // recommendations row's JSONB `payload` column, and with the execution's + // recommendations JSONB; no DDL needed. + CommitmentCurrency string `json:"commitment_currency,omitempty" dynamodbav:"commitment_currency,omitempty"` // SavingsPercentage is the provider-authoritative effective savings % // reported directly by the cloud provider (AWS Cost Explorer // `EstimatedMonthlySavingsPercentage`, Azure / GCP converters' computed diff --git a/internal/planstep/resolve.go b/internal/planstep/resolve.go index 80ebda6dd..6db87b0cb 100644 --- a/internal/planstep/resolve.go +++ b/internal/planstep/resolve.go @@ -419,6 +419,8 @@ func size(shown, stored *config.RecommendationRecord, product float64, n int, sh out.Savings = stored.Savings * ratio out.MonthlyCost = scaled(stored.MonthlyCost, ratio) out.OnDemandCost = scaled(stored.OnDemandCost, ratio) + // The costs are stored's, so their currency is too (issue #361). + out.CommitmentCurrency = stored.CommitmentCurrency if out.UpfrontCost < 0 || (out.MonthlyCost != nil && *out.MonthlyCost < 0) || out.UpfrontCost+monthlyTotal(&out) <= 0 { return out, false, errors.New("stored row carries no usable price") diff --git a/internal/planstep/resolve_test.go b/internal/planstep/resolve_test.go index 10c88faa1..f6c2bcf6b 100644 --- a/internal/planstep/resolve_test.go +++ b/internal/planstep/resolve_test.go @@ -90,6 +90,22 @@ func TestResolveSizesAndPricesFromUnsuppressedRow(t *testing.T) { assert.Equal(t, *stored.OnDemandCost*ratio, *got[0].OnDemandCost) } +// The costs come from the stored row, so the currency must too: a shown row +// that says USD cannot relabel a stored EUR price, and an unknown stored +// currency stays unknown (issue #361). +func TestResolveTakesCurrencyFromStoredRow(t *testing.T) { + for _, tc := range []struct{ stored, shown string }{{"EUR", "USD"}, {"", "USD"}, {"USD", ""}} { + stored := azureRec(20, 1000) + stored.CommitmentCurrency = tc.stored + shown := stored + shown.CommitmentCurrency = tc.shown + got, err := resolveStep1(t, fakeSource{shown: []config.RecommendationRecord{shown}}, fakeStore{rows: []config.RecommendationRecord{stored}}, azurePlan(80, 25), accts) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, tc.stored, got[0].CommitmentCurrency) + } +} + func TestResolveFloorsProductFirst(t *testing.T) { stored := azureRec(100, 1000) // 0.7*0.1 as a ratio is 0.06999999999999999, which floors 7 to 6. diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index c7514c959..494094de0 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1748,6 +1748,11 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide Selected: true, // Default to selected Purchased: false, } + // Keep the currency the provider parser proved for the commitment + // legs; empty stays empty (unknown), never a defaulted "USD". The + // purchase spend-cap check refuses a capped purchase of a rec that + // is not provably USD (issue #361). + record.CommitmentCurrency = rec.CommitmentCurrency id, err := recommendationID(record) if err != nil { logging.Errorf("convertRecommendations: dropping %s rec (account=%s service=%s sku=%s region=%s): %v", diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 9fcf5169c..82262b4b0 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1699,6 +1699,26 @@ func TestScheduler_ConvertRecommendations_SavingsPercentage(t *testing.T) { "SavingsPercentage=0 from the provider must round-trip as nil, not as a pointer to 0.0") } +// TestScheduler_ConvertRecommendations_CommitmentCurrency: the currency the +// provider parser proved for the commitment legs is kept on the record, and +// an unknown one stays empty rather than being defaulted to USD. The purchase +// spend cap refuses a capped purchase of a rec that is not provably USD +// (issue #361), so dropping it here would refuse every capped purchase. +func TestScheduler_ConvertRecommendations_CommitmentCurrency(t *testing.T) { + scheduler := &Scheduler{} + rec := func(currency string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceEC2, Region: "cn-north-1", ResourceType: "m5.large", + Count: 1, Term: "1yr", PaymentOption: "all-upfront", CommitmentCost: 5000, CommitmentCurrency: currency, + } + } + records := scheduler.convertRecommendations([]common.Recommendation{rec("CNY"), rec("USD"), rec("")}, "aws") + require.Len(t, records, 3) + assert.Equal(t, "CNY", records[0].CommitmentCurrency) + assert.Equal(t, "USD", records[1].CommitmentCurrency) + assert.Empty(t, records[2].CommitmentCurrency) +} + // TestScheduler_ConvertRecommendations_IDUniqueness pins issue #187 + // #188: the rec ID must include term and engine, not just // (provider, service, region, resource_type, payment) — otherwise