From c176f1951e5ee3ac70ef070295c9a914ed60af61 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 16:56:34 +0200 Subject: [PATCH] fix(api/validation): provider-canonical payment-option whitelist (#717) Replace the hand-maintained purchasePaymentWhitelist var with purchasePaymentSet(), which derives the accepted token set from config.ValidPaymentOptionsByProvider -- the same source used by the plan-validator in #709, eliminating drift between the two boundaries. Apply config.NormalizePaymentOption before the whitelist check so legacy AWS-style tokens on Azure/GCP are coerced to canonical spellings (e.g. all-upfront -> upfront on Azure) before validation, preserving backward compatibility for existing callers. Tighten the rejection error to name the provider and list its valid options: `invalid payment option for azure service: "foo" (valid for azure: upfront, monthly)`. Add 15 new test cases covering the per-provider canonical sets, the cross-provider normalization paths, and the error message shape. --- internal/api/handler_purchases_guards_test.go | 53 ++++++++++++++- internal/api/validation.go | 68 +++++++++++-------- 2 files changed, 89 insertions(+), 32 deletions(-) diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index ddd9c10c5..56160a21d 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -43,15 +43,48 @@ func TestValidatePurchaseRecommendation(t *testing.T) { rec config.RecommendationRecord wantError bool }{ + // --- AWS canonical set --- {"valid aws all-upfront 3y", validRec(), false}, {"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false}, + {"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false}, + {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true}, + {"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true}, + // --- Azure canonical set --- + {"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false}, {"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false}, - {"valid gcp upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "upfront" }), false}, + // Legacy AWS-style tokens on Azure are normalized to Azure-canonical before validation. + {"azure accepts legacy all-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) { + r.Provider = "azure"; r.Payment = "all-upfront" + }), false}, + {"azure accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + r.Provider = "azure"; r.Payment = "no-upfront" + }), false}, + {"azure accepts legacy partial-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) { + r.Provider = "azure"; r.Payment = "partial-upfront" + }), false}, + {"azure rejects unknown token", mutate(func(r *config.RecommendationRecord) { + r.Provider = "azure"; r.Payment = "foo" + }), true}, + // --- GCP canonical set (monthly-only) --- + {"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false}, + // Legacy tokens on GCP are all normalized to monthly. + {"gcp accepts legacy upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + r.Provider = "gcp"; r.Payment = "upfront" + }), false}, + {"gcp accepts legacy all-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + r.Provider = "gcp"; r.Payment = "all-upfront" + }), false}, + {"gcp accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) { + r.Provider = "gcp"; r.Payment = "no-upfront" + }), false}, + {"gcp rejects unknown token", mutate(func(r *config.RecommendationRecord) { + r.Provider = "gcp"; r.Payment = "foo" + }), true}, + // --- General --- {"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false}, {"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true}, {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true}, {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true}, - {"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true}, {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true}, {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true}, {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true}, @@ -74,6 +107,22 @@ func TestValidatePurchaseRecommendation(t *testing.T) { } } +// TestValidatePurchaseRecommendation_ErrorMessage verifies that a mismatched +// (provider, payment-option) pair produces a 400 error whose message names the +// provider and lists the valid options, matching the plan-validator shape +// required by issue #717. +func TestValidatePurchaseRecommendation_ErrorMessage(t *testing.T) { + t.Parallel() + rec := validRec() + rec.Provider = "azure" + rec.Payment = "foo" // not in azure canonical set and has no normalization alias + err := validatePurchaseRecommendation(&rec, 0) + require.Error(t, err) + assert.Contains(t, err.Error(), "azure") + assert.Contains(t, err.Error(), "upfront") + assert.Contains(t, err.Error(), "monthly") +} + // The per-rec #643 validation is wired into the web execute boundary // (validateExecutePurchaseRequest), NOT the shared validateAndTotalRecommendations // which the retry path also calls with replayed recs. This test pins that diff --git a/internal/api/validation.go b/internal/api/validation.go index 92a41af84..2401884d4 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -463,32 +463,21 @@ func parseAccountIDs(raw string) ([]string, error) { return ids, nil } -// purchasePaymentWhitelist maps a concrete provider to the set of payment -// options that provider's purchase path accepts. AWS RIs/SPs support the -// three classic upfront tiers; Azure/GCP reservations additionally accept -// the "upfront"/"monthly" spellings their SDK clients switch on (see -// providers/{azure,gcp}/services/*/client.go). Any value outside the set -// for the rec's provider is rejected at the API boundary so a malformed -// Payment never reaches the cloud SDK with a silent default substituted -// (issue #643). -var purchasePaymentWhitelist = map[string]map[string]bool{ - "aws": { - "all-upfront": true, - "partial-upfront": true, - "no-upfront": true, - }, - "azure": { - "all-upfront": true, - "upfront": true, - "no-upfront": true, - "monthly": true, - }, - "gcp": { - "all-upfront": true, - "upfront": true, - "no-upfront": true, - "monthly": true, - }, +// purchasePaymentSet returns the provider-canonical set of accepted payment +// option tokens for the given (already-lowercased) provider. It derives the +// set from config.ValidPaymentOptionsByProvider so that this boundary and the +// plan-validator share a single source of truth and cannot drift (issue #717). +// Returns nil when provider is unknown. +func purchasePaymentSet(provider string) map[string]bool { + opts, ok := config.ValidPaymentOptionsByProvider[provider] + if !ok { + return nil + } + set := make(map[string]bool, len(opts)) + for _, o := range opts { + set[o] = true + } + return set } // purchaseTermWhitelist maps a concrete provider to the set of commitment @@ -506,11 +495,20 @@ var purchaseTermWhitelist = map[string]map[int]bool{ // query-time validateProvider (which permits ""/"all"), the execute path // requires a concrete provider because each rec triggers a real provider // call. idx is the rec's position in the request slice, surfaced in the -// error so the caller can point at the offending row. Closes issue #643. +// error so the caller can point at the offending row. Closes issues #643, +// #717. +// +// Payment-option handling follows the same two-step approach as the +// plan-validator in internal/config/validation.go: +// 1. NormalizePaymentOption coerces any legacy AWS-style token onto the +// provider-canonical spelling (e.g. "all-upfront" on Azure -> "upfront"). +// 2. The provider-canonical set (from config.ValidPaymentOptionsByProvider) +// rejects anything that has no canonical mapping, with an error that +// names the provider and lists the accepted tokens. func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) error { provider := strings.ToLower(strings.TrimSpace(rec.Provider)) - payments, providerOK := purchasePaymentWhitelist[provider] - if !providerOK { + payments := purchasePaymentSet(provider) + if payments == nil { return NewClientError(400, fmt.Sprintf("recommendation %d has invalid provider %q: must be one of aws, azure, gcp", idx, rec.Provider)) } rec.Provider = provider @@ -525,8 +523,18 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e return NewClientError(400, fmt.Sprintf("recommendation %d has invalid term %d for provider %s: must be 1 or 3", idx, rec.Term, provider)) } payment := strings.ToLower(strings.TrimSpace(rec.Payment)) + // Coerce any legacy/cross-provider alias before the whitelist check so + // that callers using old AWS-style tokens are transparently redirected to + // the canonical token for the target provider. + if normalized, ok := config.NormalizePaymentOption(provider, payment); ok { + payment = normalized + } if !payments[payment] { - return NewClientError(400, fmt.Sprintf("recommendation %d has invalid payment %q for provider %s", idx, rec.Payment, provider)) + return NewClientError(400, fmt.Sprintf( + "invalid payment option for %s service: %q (valid for %s: %s)", + provider, rec.Payment, provider, + strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "), + )) } rec.Payment = payment return nil