Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion internal/api/claimed_row_check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down
34 changes: 34 additions & 0 deletions internal/api/combined_plan_purchase_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)}
Expand Down Expand Up @@ -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}}
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion internal/api/handler_purchases.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
1 change: 1 addition & 0 deletions internal/api/handler_purchases_plan_step_approve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions internal/api/handler_purchases_retry_fanout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
}
}
Expand Down
25 changes: 14 additions & 11 deletions internal/api/handler_purchases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"},
},
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"}
Expand Down Expand Up @@ -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",
})
}

Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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)
Expand All @@ -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",
Expand All @@ -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)
Expand Down
1 change: 1 addition & 0 deletions internal/api/negative_purchase_cap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}
}

Expand Down
79 changes: 79 additions & 0 deletions internal/api/purchase_currency.go
Original file line number Diff line number Diff line change
@@ -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
}
Loading
Loading