diff --git a/internal/api/handler.go b/internal/api/handler.go index edf546b04..1bcb457f8 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -79,9 +79,11 @@ type Handler struct { targetOfferingsEC2Factory func(aws.Config) targetOfferingsEC2Client // Optional Azure exchange client factory injected by tests. When nil - // (the production default), buildAzureExchangeClient uses - // azidentity.NewDefaultAzureCredential to construct a real - // armreservations-backed client. + // (the production default), buildAzureExchangeClient resolves the + // registered CloudAccount's per-subscription credentials and constructs + // a real armreservations-backed client. The azureExchangeClient + // interface covers listing exchangeable reservations plus pricing + // (CalculateExchange) and committing (ExecuteExchange) an exchange. azureExchangeFactory func(subscriptionID string) azureExchangeClient // Optional marketplace EC2 client factory injected by tests. When nil @@ -460,6 +462,15 @@ func (h *Handler) authorizeAPIKeyAny(ctx context.Context, apiKey string, verbs [ // AccountIDs constraint still matches via the empty-permission-side rule. const unattributedAccountConstraint = "unattributed" +// requirePermissionConstraintsAction is the action every current caller of +// requirePermissionConstraints checks (execute:purchases, execute:ri-exchange +// for both AWS and Azure). Every constraint-gated operation today is an +// irreversible execute; hardcoded rather than threaded as a parameter since +// a parameter with only one real value across all call sites is dead +// flexibility (a genuinely new action should add a real parameter back, +// not resurrect an unused one). +const requirePermissionConstraintsAction = "execute" + // requirePermissionConstraints re-checks an already-authenticated session // against request-derived permission constraint sets, so the Constraints // (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) configured on @@ -477,7 +488,7 @@ const unattributedAccountConstraint = "unattributed" // prevents a CI key with MaxPurchaseAmount=$100 from spending up to the // owning user's full group limit by inheriting the broader group permissions // (adversarial-review F2). -func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Session, action, resource string, constraintSets []auth.PermissionConstraints) error { +func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Session, resource string, constraintSets []auth.PermissionConstraints) error { if session == nil { return fmt.Errorf("internal error: nil session passed to requirePermissionConstraints") } @@ -490,21 +501,21 @@ func (h *Handler) requirePermissionConstraints(ctx context.Context, session *Ses // User API key: evaluate constraints against the key's effective permissions, // not the owning user's full group permissions. if session.UserAPIKeyID != "" { - has, err := h.auth.HasAPIKeyPermissionForConstraintsAPI(ctx, session.UserAPIKeyID, session.UserID, action, resource, constraintSets) + has, err := h.auth.HasAPIKeyPermissionForConstraintsAPI(ctx, session.UserAPIKeyID, session.UserID, requirePermissionConstraintsAction, resource, constraintSets) if err != nil { return fmt.Errorf("permission constraint check failed: %w", err) } if !has { - return NewClientError(403, fmt.Sprintf("permission denied: this request exceeds the constraints configured on your %s permission for %s", action, resource)) + return NewClientError(403, fmt.Sprintf("permission denied: this request exceeds the constraints configured on your %s permission for %s", requirePermissionConstraintsAction, resource)) } return nil } - has, err := h.auth.HasPermissionForConstraintsAPI(ctx, session.UserID, action, resource, constraintSets) + has, err := h.auth.HasPermissionForConstraintsAPI(ctx, session.UserID, requirePermissionConstraintsAction, resource, constraintSets) if err != nil { return fmt.Errorf("permission constraint check failed: %w", err) } if !has { - return NewClientError(403, fmt.Sprintf("permission denied: this request exceeds the constraints configured on your %s permission for %s", action, resource)) + return NewClientError(403, fmt.Sprintf("permission denied: this request exceeds the constraints configured on your %s permission for %s", requirePermissionConstraintsAction, resource)) } return nil } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4b9e4efca..7553b2cd9 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2098,7 +2098,7 @@ func (h *Handler) enforcePurchaseConstraints(ctx context.Context, session *Sessi if err := requireNonZeroCommitment(constraintSets); err != nil { return err } - return h.requirePermissionConstraints(ctx, session, "execute", "purchases", constraintSets) + return h.requirePermissionConstraints(ctx, session, "purchases", constraintSets) } // purchaseConstraintSets builds one auth.PermissionConstraints per diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 5ac566ba5..265859ea4 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -10,8 +10,10 @@ import ( "math/big" "regexp" "strconv" + "strings" "time" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -163,12 +165,21 @@ func (h *Handler) listTargetOfferings(ctx context.Context, req *events.LambdaFun return &TargetOfferingsResponse{Offerings: offerings}, nil } -// azureExchangeClient is the narrow interface that listExchangeableAzureRIs -// needs from the Azure compute client. Satisfied by -// *azurecompute.ComputeClient; a stub can be injected via -// Handler.azureExchangeFactory for tests. +// azureExchangeClient is the narrow interface that listExchangeableAzureRIs, +// getAzureCompatibleOfferings, and executeAzureExchange need from the Azure +// compute client. Satisfied by *azurecompute.ComputeClient; a stub can be +// injected via Handler.azureExchangeFactory for tests. type azureExchangeClient interface { ListExchangeableReservations(ctx context.Context) ([]azurecompute.ExchangeableReservation, error) + + // CalculateExchange prices a proposed exchange without committing it. + // Both getAzureCompatibleOfferings (read-only quote) and + // executeAzureExchange (server-side re-quote before commit) call this; + // only executeAzureExchange ever calls ExecuteExchange, and only with + // the SessionID this same call just returned -- see executeAzureExchange's + // doc comment for why a client-supplied session is never trusted. + CalculateExchange(ctx context.Context, sources []azurecompute.ExchangeableReservation, targets []azurecompute.ExchangeTarget) (*azurecompute.ExchangePreview, []azurecompute.CompatibleOffering, error) + ExecuteExchange(ctx context.Context, sessionID string) (*azurecompute.ExchangeResult, error) } // buildAzureExchangeClient returns the injected factory result when one has @@ -255,6 +266,805 @@ func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.Lamb return &ExchangeableAzureRIsResponse{Reservations: reservations}, nil } +// maxAzureExchangeItems caps the number of sources/targets accepted per +// Azure exchange request, guarding against an oversized request fanning out +// into an enormous CalculateExchange payload. +const maxAzureExchangeItems = 50 + +// AzureExchangeSourceBody is one source reservation entry in an Azure +// compatible-offerings or execute request body. +type AzureExchangeSourceBody struct { + ReservationID string `json:"reservation_id"` + Quantity int32 `json:"quantity"` +} + +// AzureExchangeTargetBody is one target entry. Term is the ISO 8601 +// reservation term string ("P1Y", "P3Y", ...); azureReservationTermFromString +// validates it against the SDK's typed enum rather than accepting anything +// the caller sends. +type AzureExchangeTargetBody struct { + SKU string `json:"sku"` + Location string `json:"location"` + Term string `json:"term"` + Quantity int32 `json:"quantity"` + + // BillingScopeID is optional and is NOT the scope that gets charged: + // the handler always derives that from the request's authorized + // subscription_id (azureBillingScopeID), matching every other Azure + // reservation purchase path in this repo. When supplied it must match + // the derived scope, so a caller cannot direct the charge at a + // different subscription than the one their permission constraints + // were evaluated against. + BillingScopeID string `json:"billing_scope_id,omitempty"` +} + +// AzureCompatibleOfferingsRequestBody is the request body for the +// compatible-offerings endpoint. +type AzureCompatibleOfferingsRequestBody struct { + SubscriptionID string `json:"subscription_id"` + Sources []AzureExchangeSourceBody `json:"sources"` + Targets []AzureExchangeTargetBody `json:"targets"` +} + +// AzureCompatibleOfferingsResponse is the response for the +// compatible-offerings endpoint: the priced candidate offerings plus the +// preview (including the SessionID a subsequent execute call would need, +// though execute never trusts a client-supplied session -- see +// executeAzureExchange). +type AzureCompatibleOfferingsResponse struct { + Offerings []azurecompute.CompatibleOffering `json:"offerings"` + Preview *azurecompute.ExchangePreview `json:"preview"` +} + +// AzureExecuteExchangeRequestBody is the request body for the execute +// endpoint. MaxPaymentDue + Currency are mandatory safety guardrails: the +// handler refuses to execute an exchange whose fresh quote exceeds the cap +// or is denominated in a different currency. +type AzureExecuteExchangeRequestBody struct { + SubscriptionID string `json:"subscription_id"` + Sources []AzureExchangeSourceBody `json:"sources"` + Targets []AzureExchangeTargetBody `json:"targets"` + MaxPaymentDue string `json:"max_payment_due"` + Currency string `json:"currency"` +} + +// AzureExecuteExchangeResponse is the response from a successfully executed +// Azure exchange. +type AzureExecuteExchangeResponse struct { + SessionID string `json:"session_id"` + Status string `json:"status"` + NetPayable *float64 `json:"net_payable"` + NetPayableCurrency string `json:"net_payable_currency,omitempty"` + RefundsTotal *float64 `json:"refunds_total"` + PurchasesTotal *float64 `json:"purchases_total"` +} + +// azureReservationTermFromString converts the HTTP-layer term string to the +// typed SDK enum, rejecting anything outside armreservations' +// PossibleReservationTermValues(). No fallback: an unrecognized term is a +// 400, never silently coerced to a default term (feedback_sdk_enum_string_literals). +func azureReservationTermFromString(s string) (armreservations.ReservationTerm, error) { + term := armreservations.ReservationTerm(s) + for _, t := range armreservations.PossibleReservationTermValues() { + if t == term { + return term, nil + } + } + return "", fmt.Errorf("unsupported term %q", s) +} + +// validateAzureExchangeSources checks the shared sources[] shape for both +// the offerings and execute request bodies. +func validateAzureExchangeSources(sources []AzureExchangeSourceBody) error { + if len(sources) == 0 { + return NewClientError(400, "sources is required") + } + if len(sources) > maxAzureExchangeItems { + return NewClientError(400, fmt.Sprintf("sources exceeds the maximum of %d items", maxAzureExchangeItems)) + } + for i, s := range sources { + if s.ReservationID == "" { + return NewClientError(400, fmt.Sprintf("sources[%d].reservation_id is required", i)) + } + if s.Quantity < 1 { + return NewClientError(400, fmt.Sprintf("sources[%d].quantity must be >= 1", i)) + } + } + return nil +} + +// azureBillingScopeID returns the ARM billing scope that a purchase against +// subscriptionID is charged to. +// +// The billing scope is always derived from the request's subscription_id, +// never accepted from the caller. Every other Azure reservation purchase +// path in this repo does the same (ComputeClient.buildReservationBody and +// the database / cache / search / cosmosdb / synapse / managedredis +// clients all build "/subscriptions/{their own subscriptionID}"). It also +// keeps the charge inside the scope authorization actually checked: the +// execute:ri-exchange AccountIDs constraint is evaluated against the +// CloudAccount registered for subscription_id, so letting a caller name a +// different billing scope would move the money outside the account whose +// constraints were verified. +func azureBillingScopeID(subscriptionID string) string { + return "/subscriptions/" + subscriptionID +} + +// validateAzureExchangeTargets checks the shared targets[] shape for both +// the offerings and execute request bodies. subscriptionID is the already- +// validated request subscription; a target may omit billing_scope_id +// entirely, but may not name a scope other than that subscription's. +func validateAzureExchangeTargets(targets []AzureExchangeTargetBody, subscriptionID string) error { + if len(targets) == 0 { + return NewClientError(400, "targets is required") + } + if len(targets) > maxAzureExchangeItems { + return NewClientError(400, fmt.Sprintf("targets exceeds the maximum of %d items", maxAzureExchangeItems)) + } + scope := azureBillingScopeID(subscriptionID) + for i, t := range targets { + if t.SKU == "" { + return NewClientError(400, fmt.Sprintf("targets[%d].sku is required", i)) + } + // Blank-but-not-empty is rejected too: targetLocations trims before + // it builds the Regions constraint, so a whitespace-only location + // would otherwise reach the permission check as "" -- a region no + // permission can name. + if strings.TrimSpace(t.Location) == "" { + return NewClientError(400, fmt.Sprintf("targets[%d].location is required", i)) + } + if t.BillingScopeID != "" && !strings.EqualFold(t.BillingScopeID, scope) { + return NewClientError(400, fmt.Sprintf( + "targets[%d].billing_scope_id %q is not the billing scope of subscription %q; omit it to charge the subscription's own scope", + i, t.BillingScopeID, subscriptionID)) + } + if t.Quantity < 1 { + return NewClientError(400, fmt.Sprintf("targets[%d].quantity must be >= 1", i)) + } + if _, err := azureReservationTermFromString(t.Term); err != nil { + return NewClientError(400, fmt.Sprintf("targets[%d].term: %v", i, err)) + } + } + return nil +} + +// validateAzureOfferingsBody validates the compatible-offerings request +// body. Extracted so getAzureCompatibleOfferings and +// validateAzureExecuteBody share the same check without exceeding the +// gocyclo threshold (mirrors validateExecuteExchangeBody's precedent for +// the AWS execute handler). +func validateAzureOfferingsBody(body AzureCompatibleOfferingsRequestBody) error { + if body.SubscriptionID == "" { + return NewClientError(400, "subscription_id is required") + } + if err := validateAzureExchangeSources(body.Sources); err != nil { + return err + } + return validateAzureExchangeTargets(body.Targets, body.SubscriptionID) +} + +// validateAzureExecuteBody validates the execute request body: the shared +// offerings validation plus the mandatory spend-cap and currency guardrails. +func validateAzureExecuteBody(body AzureExecuteExchangeRequestBody) error { + if err := validateAzureOfferingsBody(AzureCompatibleOfferingsRequestBody{ + SubscriptionID: body.SubscriptionID, + Sources: body.Sources, + Targets: body.Targets, + }); err != nil { + return err + } + if body.MaxPaymentDue == "" { + return NewClientError(400, "max_payment_due is required as a safety guardrail") + } + if body.Currency == "" { + return NewClientError(400, "currency is required") + } + return nil +} + +// toAzureExchangeSources converts the HTTP-shaped sources into the +// provider-layer shape. Pure field mapping; validateAzureExchangeSources +// must be called first. +func toAzureExchangeSources(sources []AzureExchangeSourceBody) []azurecompute.ExchangeableReservation { + out := make([]azurecompute.ExchangeableReservation, len(sources)) + for i, s := range sources { + out[i] = azurecompute.ExchangeableReservation{ReservationID: s.ReservationID, Quantity: s.Quantity} + } + return out +} + +// toAzureExchangeTargets converts the HTTP-shaped targets into the +// provider-layer shape, re-parsing the term string and deriving each +// target's billing scope from subscriptionID rather than from the request +// body (see azureBillingScopeID). validateAzureExchangeTargets must be +// called first; a term error here indicates an internal invariant break +// rather than a fresh client mistake. +func toAzureExchangeTargets(targets []AzureExchangeTargetBody, subscriptionID string) ([]azurecompute.ExchangeTarget, error) { + out := make([]azurecompute.ExchangeTarget, len(targets)) + scope := azureBillingScopeID(subscriptionID) + for i, t := range targets { + term, err := azureReservationTermFromString(t.Term) + if err != nil { + return nil, fmt.Errorf("targets[%d]: %w", i, err) + } + out[i] = azurecompute.ExchangeTarget{ + SKU: t.SKU, + Location: t.Location, + Term: term, + Quantity: t.Quantity, + BillingScopeID: scope, + } + } + return out, nil +} + +// targetLocations returns the de-duplicated, canonically lower-cased set of +// target locations, used to populate the Regions dimension of the +// execute:ri-exchange constraint check. Callers must have already validated +// that every target has a non-blank Location. +// +// Azure treats location names case-insensitively and its own APIs return the +// lower-case form, so lower case is the canonical spelling. Normalizing +// before the dedup collapses "EastUS" -- the casing the Azure portal +// displays -- and "eastus" into the single region they actually are, so the +// constraint set names each target region exactly once rather than demanding +// a permission for two spellings of one place. +// +// auth.matchAllRegionsConstraint compares case-insensitively as well, so a +// permission stored in either casing matches either way. Normalizing here +// keeps the two layers agreeing on what a region is instead of leaving the +// constraint set's accuracy resting on the matcher's leniency. +// +// toAzureExchangeTargets sends Azure the raw, un-normalized Location. That +// is not a gap: Azure resolves either casing to the same region, so the +// region authorized here is the region the exchange lands in. +func targetLocations(targets []AzureExchangeTargetBody) []string { + locations := make([]string, len(targets)) + for i, t := range targets { + locations[i] = t.Location + } + return normalizeRegions(locations) +} + +// normalizeRegions canonically lower-cases and trims region names and +// de-duplicates them, preserving first-seen order. Shared by targetLocations +// and exchangeRegions so both sides of an exchange are spelled the same way +// before they reach auth.matchAllRegionsConstraint. +func normalizeRegions(regions []string) []string { + seen := make(map[string]bool, len(regions)) + out := make([]string, 0, len(regions)) + for _, r := range regions { + region := strings.ToLower(strings.TrimSpace(r)) + if !seen[region] { + seen[region] = true + out = append(out, region) + } + } + return out +} + +// unknownRegionConstraint is the Regions entry substituted for a source +// reservation whose region Azure did not report (ExchangeableReservation.Region +// "may be empty for reservations with AppliedScopeType == Shared"), or that is +// absent from the tenant listing altogether. +// +// It is deliberately not a region name: no Azure location is spelled this way, +// so a permission carrying ANY Regions constraint cannot name it and the +// exchange is denied. A permission with NO Regions constraint is unaffected +// (auth.matchAllRegionsConstraint treats an empty permission list as "no +// restriction"), so callers who were never region-scoped are not penalized for +// a reservation Azure described incompletely. +// +// The alternative -- dropping an unreported region from the set -- would make +// "Azure did not tell us where this is" mean "unconstrained", which is exactly +// the fail-open shape of the empty-region defect fixed in PR #1495. Same +// posture as unattributedAccountConstraint on the AccountIDs dimension. +const unknownRegionConstraint = "unknown-region" + +// exchangeRegions returns every region an Azure exchange touches: each +// target's location AND each source reservation's own region, normalized and +// de-duplicated into one set. +// +// Both halves belong in the Regions dimension because an exchange mutates +// both. The sources are handed back to Azure and their commitment value is +// consumed; the targets are acquired. Constraining only the targets (the +// pre-fix behavior) let a caller permitted solely in eastus name a +// westeurope reservation as the source of an eastus-targeted exchange: the +// Regions dimension saw only "eastus", requireAzureSourceOwnership keys on the +// reservation's BillingScopeID rather than its region, and no other gate +// consults a source's region at all. The westeurope commitment -- which the +// caller was never authorized to touch -- was consumed and relocated, +// irreversibly. +// +// Source regions come from the tenant listing (owned) rather than the request +// body, because the request names only reservation IDs; Azure is the authority +// on where a reservation lives. A source missing from the listing, or one +// whose Region Azure left empty, contributes unknownRegionConstraint rather +// than nothing -- see that constant for why. +// +// On the execute path requireAzureSourceOwnership has already refused any +// source absent from the listing by the time this runs, so the sentinel there +// means specifically "owned, but Azure reported no region". The missing-source +// branch is kept as a fail-closed default for any future caller that reaches +// this function without that guarantee. +func exchangeRegions(targets []AzureExchangeTargetBody, sources []AzureExchangeSourceBody, owned []azurecompute.ExchangeableReservation) []string { + regionByID := make(map[string]string, len(owned)) + for i := range owned { + regionByID[strings.ToLower(owned[i].ReservationID)] = owned[i].Region + } + + regions := targetLocations(targets) + for _, s := range sources { + region := strings.ToLower(strings.TrimSpace(regionByID[strings.ToLower(s.ReservationID)])) + if region == "" { + region = unknownRegionConstraint + } + regions = append(regions, region) + } + return normalizeRegions(regions) +} + +// requireAzureSubscriptionScope enforces the session's allowed_accounts +// scope (issue #1030) against the CloudAccount registered for +// subscriptionID, the same per-account gate the sibling /ri-exchange +// endpoints apply. Without it, subscription_id is a caller-controlled +// pointer at any subscription in the tenant: a user scoped to one account +// could price, and with an otherwise-unconstrained execute:ri-exchange +// permission execute, an exchange against another account's subscription. +// The per-permission Constraints check does not cover this -- it only +// consults the permission's own AccountIDs, never the user's +// allowed_accounts. +// +// Returns errNotFound (404, not 403) when a scoped session names a +// subscription outside its scope, including one with no registered account +// at all, matching requireAccountAccess: a user must not be able to probe +// which subscriptions exist outside their scope. +// +// Unrestricted / admin sessions short-circuit before the account fetch, +// mirroring requireExecutionAccess. +func (h *Handler) requireAzureSubscriptionScope(ctx context.Context, session *Session, subscriptionID string) error { + allowed, err := h.getAllowedAccounts(ctx, session) + if err != nil { + return fmt.Errorf("failed to get allowed accounts: %w", err) + } + if auth.IsUnrestrictedAccess(allowed) { + return nil + } + account, err := h.config.GetCloudAccountByExternalID(ctx, "azure", subscriptionID) + if err != nil { + return fmt.Errorf("failed to resolve cloud account scope: %w", err) + } + if account == nil || !auth.MatchesAccount(allowed, account.ID, account.Name) { + return errNotFound + } + return nil +} + +// requireAzureSourceOwnership refuses any source reservation that is not +// paid for by the authorized subscription (issue #1527). +// +// Every other gate on these endpoints constrains the DESTINATION of an +// exchange: requireAzureSubscriptionScope and the execute:ri-exchange +// AccountIDs constraint both key off subscription_id, and +// toAzureExchangeTargets forces each target's billing scope to that same +// subscription. The sources were unconstrained. That matters because Azure +// reservation orders are tenant-scoped, not subscription-scoped -- +// ListExchangeableReservations enumerates the whole tenant precisely +// because "the Azure Capacity exchange API operates on reservation order +// IDs which span subscriptions". So a caller authorized for subscription A +// could name subscription B's reservation IDs and hand B's commitments +// back, with the replacement purchased into A's billing scope. Azure RBAC +// on the reservation order was the only backstop. +// +// The check uses each reservation's own BillingScopeID -- the subscription +// Azure charges for it, and the scope an exchange refunds it to. That is +// the correct discriminator even for AppliedScopeType == Shared, which +// governs which subscriptions receive the discount rather than which one +// paid; an AppliedScopes-based check would pass for nearly every +// reservation and be security theater. +// +// Fails closed on every uncertainty: a reservation absent from the listing, +// or one Azure reports without a billing scope, is refused rather than +// allowed. Denials deliberately do not distinguish "does not exist" from +// "belongs to someone else", so this cannot be used to enumerate another +// subscription's reservation IDs (same posture as +// requireAzureSubscriptionScope). +func requireAzureSourceOwnership(owned []azurecompute.ExchangeableReservation, sources []AzureExchangeSourceBody, subscriptionID string) error { + scope := azureBillingScopeID(subscriptionID) + byID := make(map[string]string, len(owned)) + for i := range owned { + byID[strings.ToLower(owned[i].ReservationID)] = owned[i].BillingScopeID + } + for i, s := range sources { + billingScope, found := byID[strings.ToLower(s.ReservationID)] + if !found || billingScope == "" || !strings.EqualFold(billingScope, scope) { + return NewClientError(403, fmt.Sprintf( + "sources[%d].reservation_id is not a reservation billed to subscription %q; an exchange may only hand back reservations that subscription paid for", + i, subscriptionID)) + } + } + return nil +} + +// checkAzureSourceOwnership fetches the caller's visible reservations and +// applies requireAzureSourceOwnership. Split from the pure check so the +// authorization rule itself is testable without a client, and so both the +// pricing and execute endpoints share one code path. +func checkAzureSourceOwnership(ctx context.Context, client azureExchangeClient, sources []AzureExchangeSourceBody, subscriptionID string) error { + owned, err := listOwnedAzureReservations(ctx, client) + if err != nil { + return err + } + return requireAzureSourceOwnership(owned, sources, subscriptionID) +} + +// listOwnedAzureReservations fetches the tenant-wide reservation listing that +// both source-side gates consult: requireAzureSourceOwnership (which +// subscription paid for each source) and exchangeRegions (where each source +// lives). Extracted so the execute path can fetch it once and feed both, +// rather than listing twice and risking the two gates disagreeing. +// +// Fails closed: without the listing we can establish neither ownership nor +// region, and permitting the exchange would restore the very gaps those +// checks exist to close. 502 rather than 500 because the upstream dependency, +// not this service, is what failed. +func listOwnedAzureReservations(ctx context.Context, client azureExchangeClient) ([]azurecompute.ExchangeableReservation, error) { + owned, err := client.ListExchangeableReservations(ctx) + if err != nil { + logging.Errorf("azure exchange source ownership lookup failed: %v", err) + return nil, NewClientError(502, "could not verify which subscription owns the requested reservations; refusing to proceed") + } + return owned, nil +} + +// getAzureCompatibleOfferings prices a proposed Azure RI exchange and +// returns the compatible offerings Azure is willing to accept plus the cost +// preview, without committing anything. Requires "view:purchases" permission +// plus allowed_accounts scope over the requested subscription, mirroring the +// AWS quote endpoint. +// +// POST /api/ri-exchange/azure-instances/compatible-offerings. +func (h *Handler) getAzureCompatibleOfferings(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + session, err := h.requirePermission(ctx, req, "view", "purchases") + if err != nil { + return nil, err + } + + var body AzureCompatibleOfferingsRequestBody + if err = json.Unmarshal([]byte(req.Body), &body); err != nil { + return nil, NewClientError(400, "invalid request body") + } + if validateErr := validateAzureOfferingsBody(body); validateErr != nil { + return nil, validateErr + } + if scopeErr := h.requireAzureSubscriptionScope(ctx, session, body.SubscriptionID); scopeErr != nil { + return nil, scopeErr + } + + client, err := h.buildAzureExchangeClient(ctx, body.SubscriptionID) + if err != nil { + return nil, fmt.Errorf("failed to build Azure exchange client: %w", err) + } + if client == nil { + return nil, NewClientError(404, fmt.Sprintf("no Azure account registered for subscription %q", body.SubscriptionID)) + } + + if ownErr := checkAzureSourceOwnership(ctx, client, body.Sources, body.SubscriptionID); ownErr != nil { + return nil, ownErr + } + + targets, err := toAzureExchangeTargets(body.Targets, body.SubscriptionID) + if err != nil { + return nil, err + } + + preview, offerings, err := client.CalculateExchange(ctx, toAzureExchangeSources(body.Sources), targets) + if err != nil { + logging.Errorf("azure compatible offerings failed: %v", err) + return nil, mapAzureExchangeError("failed to find compatible offerings", err) + } + + return &AzureCompatibleOfferingsResponse{Offerings: offerings, Preview: preview}, nil +} + +// azureMaxPurchaseAmountCurrency is the currency the execute:ri-exchange +// permission's MaxPurchaseAmount constraint is denominated in, matching the +// AWS execute:ri-exchange precedent (which takes max_payment_due_usd). There +// is no FX conversion available here, so a non-USD exchange's raw amount can +// never be safely compared against a USD-denominated cap -- see +// checkAzureExecuteConstraints. +const azureMaxPurchaseAmountCurrency = "USD" + +// authorizeAzureExchangeExecution builds the Azure exchange client for the +// request's subscription, refuses sources the subscription does not own +// (issue #1527), and enforces the per-permission Constraints configured on +// execute:ri-exchange (SEC-01, issue #1141). Extracted from +// executeAzureExchange to keep that function under the gocyclo limit. +// +// Order matters, and all three gates run before any pricing or commit call: +// +// - The tenant listing is fetched before the constraint check because the +// Regions dimension cannot be assembled without knowing where the sources +// live (exchangeRegions). An unavailable listing therefore refuses with +// 502 ahead of any constraint denial. The caller has already cleared +// requirePermission("execute", "ri-exchange") and the allowed_accounts +// scope for this subscription by then, so the read-only listing call is +// within what they are authorized to trigger. +// +// - Ownership is checked before the constraint check so the two cannot form +// an enumeration oracle. If the constraint check ran first, a caller +// scoped to subscription A and permitted only in eastus would get +// distinguishable answers for a reservation id they do not own: an id +// that exists in eastus (owned by subscription B) would clear the Regions +// dimension and be refused by the ownership gate, while an id that does +// not exist -- or lives in an unpermitted region -- would be refused by +// the constraint check with a different message. That difference confirms +// "this reservation id exists, in one of my permitted regions, in a +// subscription I am not scoped to". requireAzureSourceOwnership +// deliberately makes its own denials indistinguishable; running it first +// keeps the whole path that way. +// +// It also sharpens exchangeRegions: every source reaching it is +// known-owned, so unknownRegionConstraint means only "owned, but Azure +// reported no region" rather than doubling as "not yours" or "not real". +func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session *Session, body AzureExecuteExchangeRequestBody, maxRat *big.Rat) (azureExchangeClient, error) { + // Scope check MUST precede building the client (mirrors + // getAzureCompatibleOfferings): otherwise an unregistered subscription + // (distinguishable 404: "no Azure account registered...") and a + // registered-but-out-of-scope one (generic errNotFound) would leak an + // enumeration signal to a scoped caller about which subscriptions exist, + // and credentials for an out-of-scope account could be resolved before + // the denial. + if scopeErr := h.requireAzureSubscriptionScope(ctx, session, body.SubscriptionID); scopeErr != nil { + return nil, scopeErr + } + + client, err := h.buildAzureExchangeClient(ctx, body.SubscriptionID) + if err != nil { + return nil, fmt.Errorf("failed to build Azure exchange client: %w", err) + } + if client == nil { + return nil, NewClientError(404, fmt.Sprintf("no Azure account registered for subscription %q", body.SubscriptionID)) + } + + owned, err := listOwnedAzureReservations(ctx, client) + if err != nil { + return nil, err + } + + if ownErr := requireAzureSourceOwnership(owned, body.Sources, body.SubscriptionID); ownErr != nil { + return nil, ownErr + } + + accountID, err := h.resolveAzureExchangeAccountID(ctx, body.SubscriptionID) + if err != nil { + return nil, err + } + + if err := h.checkAzureExecuteConstraints(ctx, session, body, accountID, maxRat, exchangeRegions(body.Targets, body.Sources, owned)); err != nil { + return nil, err + } + return client, nil +} + +// resolveAzureExchangeAccountID looks up the CloudAccount registered for +// subscriptionID and returns its ID, or unattributedAccountConstraint when +// no account is registered (so an AccountIDs-constrained permission still +// fails closed against an unattributed request). Extracted from +// authorizeAzureExchangeExecution to keep that function under the gocyclo +// limit. +func (h *Handler) resolveAzureExchangeAccountID(ctx context.Context, subscriptionID string) (string, error) { + account, err := h.config.GetCloudAccountByExternalID(ctx, "azure", subscriptionID) + if err != nil { + return "", fmt.Errorf("failed to resolve cloud account scope: %w", err) + } + if account != nil { + return account.ID, nil + } + return unattributedAccountConstraint, nil +} + +// checkAzureExecuteConstraints enforces the execute:ri-exchange permission +// Constraints (SEC-01, issue #1141): AccountIDs from the resolved +// CloudAccount, Providers/Services fixed to azure/compute, Regions covering +// every region the exchange touches (see exchangeRegions -- both the target +// locations and the source reservations' own regions), and MaxPurchaseAmount +// from the caller's cap. +// +// MaxPurchaseAmount is USD-denominated (azureMaxPurchaseAmountCurrency) with +// no FX conversion available. A non-USD request's raw amount is therefore +// never compared directly against the cap -- doing so would let a large +// non-USD amount (e.g. 1000 KWD, worth far more than 1000 USD) clear a cap +// meant to bound USD spend. Instead, a non-USD request is checked with an +// unmatchable sentinel amount (math.MaxFloat64): this denies the request if +// the granting permission carries ANY MaxPurchaseAmount constraint (fail +// closed on a cap this code cannot safely evaluate) while still allowing it +// through when the permission has no amount constraint at all -- callers +// without a spend cap are not penalized for using a non-USD subscription. +// +// When the sentinel check fails, a second call with the amount dimension +// neutralized (MaxPurchaseAmount: 0, which matchPurchaseAmountConstraint +// always treats as satisfied) disambiguates the cause: if that second call +// still fails, some other dimension (account/provider/service/region) is +// the real reason and its error is returned unchanged; otherwise the amount +// constraint was specifically the blocker and a currency-specific 403 is +// returned instead of the generic constraint-denied message. +func (h *Handler) checkAzureExecuteConstraints(ctx context.Context, session *Session, body AzureExecuteExchangeRequestBody, accountID string, maxRat *big.Rat, regions []string) error { + base := auth.PermissionConstraints{ + AccountIDs: []string{accountID}, + Providers: []string{string(common.ProviderAzure)}, + Services: []string{string(common.ServiceCompute)}, + Regions: regions, + } + + isUSD := strings.EqualFold(body.Currency, azureMaxPurchaseAmountCurrency) + attempt := base + if isUSD { + maxPayment, _ := maxRat.Float64() + attempt.MaxPurchaseAmount = maxPayment + } else { + attempt.MaxPurchaseAmount = math.MaxFloat64 + } + + err := h.requirePermissionConstraints(ctx, session, "ri-exchange", []auth.PermissionConstraints{attempt}) + if err == nil || isUSD { + return err + } + + // Non-USD and denied: isolate whether the amount dimension was + // specifically the cause. + withoutAmount := base + withoutAmount.MaxPurchaseAmount = 0 + if otherErr := h.requirePermissionConstraints(ctx, session, "ri-exchange", []auth.PermissionConstraints{withoutAmount}); otherErr != nil { + return otherErr + } + return NewClientError(403, fmt.Sprintf( + "your execute:ri-exchange permission has a spend-cap (MaxPurchaseAmount) constraint, which is USD-denominated and cannot be safely enforced against a %s exchange; use a USD-denominated request or ask an administrator to remove the constraint", + body.Currency)) +} + +// checkAzureExchangeMoneyGuardrails enforces the money-path guardrails +// against a freshly-obtained CalculateExchange preview, before its +// SessionID is allowed to reach ExecuteExchange: non-empty policy errors +// block execution, a nil NetPayable is refused rather than treated as free, +// a currency mismatch blocks execution, and NetPayable exceeding the cap +// blocks execution. Extracted from executeAzureExchange to keep that +// function under the gocyclo limit. +// +// A nil preview is itself refused rather than dereferenced: the current +// azureExchangeClient.CalculateExchange contract never returns (nil, nil, +// nil), but a future implementation of the interface making that mistake +// must not panic this handler. +func checkAzureExchangeMoneyGuardrails(preview *azurecompute.ExchangePreview, maxRat *big.Rat, currency string) error { + if preview == nil { + return fmt.Errorf("internal error: CalculateExchange returned a nil preview") + } + if len(preview.PolicyErrors) > 0 { + return NewClientError(422, fmt.Sprintf("Azure rejected this exchange: %s", strings.Join(preview.PolicyErrors, "; "))) + } + if preview.NetPayable == nil { + return NewClientError(422, "Azure did not return a net payable amount; refusing to execute") + } + // Case-insensitive to match the isUSD test in checkAzureExecuteConstraints: + // a request of "usd" must not clear the USD-denominated cap check there + // and then be rejected here as a mismatch against Azure's "USD". + if !strings.EqualFold(preview.NetPayableCurrency, currency) { + return NewClientError(422, fmt.Sprintf("quoted currency %q does not match requested currency %q", preview.NetPayableCurrency, currency)) + } + netPayableRat := new(big.Rat).SetFloat64(*preview.NetPayable) + if netPayableRat == nil { + return fmt.Errorf("internal error: quoted net payable %v is not a finite number", *preview.NetPayable) + } + if netPayableRat.Cmp(maxRat) > 0 { + return NewClientError(422, fmt.Sprintf("quoted net payable %s %s exceeds max_payment_due %s %s", + netPayableRat.FloatString(2), currency, maxRat.FloatString(2), currency)) + } + return nil +} + +// parseAzureExecuteRequest decodes and validates an execute request body and +// parses its spend cap into an exact rational. +// +// Extracted from executeAzureExchange purely to keep that function within +// the project's gocyclo limit once the issue #1527 source-ownership gate was +// added; it makes no decisions of its own beyond returning the same errors +// inline code did. +func parseAzureExecuteRequest(rawBody string) (AzureExecuteExchangeRequestBody, *big.Rat, error) { + var body AzureExecuteExchangeRequestBody + if err := json.Unmarshal([]byte(rawBody), &body); err != nil { + return body, nil, NewClientError(400, "invalid request body") + } + if err := validateAzureExecuteBody(body); err != nil { + return body, nil, err + } + maxRat, err := exchange.ParseDecimalRat(body.MaxPaymentDue) + if err != nil { + return body, nil, NewClientError(400, fmt.Sprintf("invalid max_payment_due: %v", err)) + } + return body, maxRat, nil +} + +// executeAzureExchange executes an Azure RI exchange with mandatory +// spend-cap and currency guardrails. Requires "execute:ri-exchange" +// (deliberately separate from execute:purchases), mirroring the AWS +// executeExchange handler: RI exchanges are financially irreversible once +// submitted. +// +// Unlike a design that executes a client-supplied session_id, this handler +// never trusts the caller's own pricing: it re-runs CalculateExchange itself +// against the caller's sources/targets, validates the FRESH quote against +// every guardrail in checkAzureExchangeMoneyGuardrails, and only then calls +// ExecuteExchange with the SessionID *that fresh call returned*. A +// client-supplied or stale session would bypass every guardrail below, so +// the server always re-quotes immediately before committing. +// +// POST /api/ri-exchange/azure-instances/exchange. +func (h *Handler) executeAzureExchange(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + session, err := h.requirePermission(ctx, req, "execute", "ri-exchange") + if err != nil { + return nil, err + } + + body, maxRat, err := parseAzureExecuteRequest(req.Body) + if err != nil { + return nil, err + } + + // authorizeAzureExchangeExecution applies every gate: allowed_accounts + // scope, source ownership (issue #1527) and the execute:ri-exchange + // Constraints, all before the pricing call below. + client, err := h.authorizeAzureExchangeExecution(ctx, session, body, maxRat) + if err != nil { + return nil, err + } + + targets, err := toAzureExchangeTargets(body.Targets, body.SubscriptionID) + if err != nil { + return nil, err + } + + preview, _, err := client.CalculateExchange(ctx, toAzureExchangeSources(body.Sources), targets) + if err != nil { + logging.Errorf("azure exchange re-quote failed: %v", err) + return nil, mapAzureExchangeError("failed to price the exchange before execution", err) + } + + err = checkAzureExchangeMoneyGuardrails(preview, maxRat, body.Currency) + if err != nil { + return nil, err + } + + result, err := client.ExecuteExchange(ctx, preview.SessionID) + if err != nil { + logging.Errorf("azure exchange execution failed: %v", err) + return nil, mapAzureExchangeError("exchange execution failed", err) + } + + logging.Infof("azure ri-exchange executed: subscription=%s session=%s status=%s", body.SubscriptionID, result.SessionID, result.Status) + + return &AzureExecuteExchangeResponse{ + SessionID: result.SessionID, + Status: result.Status, + NetPayable: result.NetPayable, + NetPayableCurrency: result.NetPayableCurrency, + RefundsTotal: preview.RefundsTotal, + PurchasesTotal: preview.PurchasesTotal, + }, nil +} + +// mapAzureExchangeError converts an error from an Azure RI exchange +// client-layer call to a ClientError with the appropriate HTTP status. +// Azure 4xx client faults (via isAzureClientError) produce a 400 with the +// Azure error message preserved; any other error produces a 500 using the +// opMsg fallback -- the same contract mapAWSExchangeError applies to the +// AWS exchange endpoints. +func mapAzureExchangeError(opMsg string, err error) error { + if isAzureClientError(err) { + return NewClientError(400, err.Error()) + } + return NewClientError(500, opMsg) +} + // getBaseAWSConfig returns the cached base AWS config, loading it once via sync.Once. func (h *Handler) getBaseAWSConfig(ctx context.Context) (aws.Config, error) { h.awsCfgOnce.Do(func() { @@ -771,7 +1581,7 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio if cloudAccountID == "" { cloudAccountID = unattributedAccountConstraint } - err = h.requirePermissionConstraints(ctx, session, "execute", "ri-exchange", []auth.PermissionConstraints{{ + err = h.requirePermissionConstraints(ctx, session, "ri-exchange", []auth.PermissionConstraints{{ AccountIDs: []string{cloudAccountID}, Providers: []string{string(common.ProviderAWS)}, Services: []string{string(common.ServiceEC2)}, diff --git a/internal/api/handler_ri_exchange_azure_test.go b/internal/api/handler_ri_exchange_azure_test.go new file mode 100644 index 000000000..42ba171b2 --- /dev/null +++ b/internal/api/handler_ri_exchange_azure_test.go @@ -0,0 +1,2225 @@ +package api + +import ( + "context" + "fmt" + "math" + "math/big" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/LeanerCloud/CUDly/internal/config" + azurecompute "github.com/LeanerCloud/CUDly/providers/azure/services/compute" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --- Azure compatible-offerings / execute exchange tests (issue #596) --- + +// mockAzureExchangeOpsClient is a testify mock implementing the widened +// azureExchangeClient interface. Used by the compatible-offerings and +// execute handler tests below to control exactly what Azure "returns" +// without any live credentials or network calls. +type mockAzureExchangeOpsClient struct { + mock.Mock +} + +func (m *mockAzureExchangeOpsClient) ListExchangeableReservations(ctx context.Context) ([]azurecompute.ExchangeableReservation, error) { + args := m.Called(ctx) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]azurecompute.ExchangeableReservation), args.Error(1) +} + +func (m *mockAzureExchangeOpsClient) CalculateExchange(ctx context.Context, sources []azurecompute.ExchangeableReservation, targets []azurecompute.ExchangeTarget) (*azurecompute.ExchangePreview, []azurecompute.CompatibleOffering, error) { + args := m.Called(ctx, sources, targets) + preview, _ := args.Get(0).(*azurecompute.ExchangePreview) + offerings, _ := args.Get(1).([]azurecompute.CompatibleOffering) + return preview, offerings, args.Error(2) +} + +func (m *mockAzureExchangeOpsClient) ExecuteExchange(ctx context.Context, sessionID string) (*azurecompute.ExchangeResult, error) { + args := m.Called(ctx, sessionID) + result, _ := args.Get(0).(*azurecompute.ExchangeResult) + return result, args.Error(1) +} + +// ownsAzureSource stubs the tenant-wide reservation listing that the +// source-side gates consult, reporting "res-1" (the source every valid +// request body below names) as billed to subscription "sub-1" and located in +// eastus -- the same region those bodies target, i.e. an ordinary same-region +// exchange. The region matters because exchangeRegions folds each source's +// region into the Regions constraint dimension; leaving it empty would put +// unknownRegionConstraint into every downstream test's constraint set. +// +// Registered on tests whose subject lies downstream of the gates, so they +// reach the behavior they actually assert. Maybe() because tests that are +// refused earlier -- by validation or allowed_accounts scope -- never get +// this far, and must not be required to. Tests whose subject IS a source-side +// gate register their own listing instead. +func ownsAzureSource(m *mockAzureExchangeOpsClient) { + m.On("ListExchangeableReservations", mock.Anything).Return([]azurecompute.ExchangeableReservation{ + {ReservationID: "res-1", BillingScopeID: "/subscriptions/sub-1", Region: "eastus", Quantity: 1}, + }, nil).Maybe() +} + +// allowAnyAccountScope stubs the allowed_accounts lookup as unrestricted +// (the "*" / Administrators-group shape), so tests whose subject is +// something other than requireAzureSubscriptionScope reach the behavior +// they actually assert. Maybe() because the validation-only tests return +// before the scope check runs. Tests that DO exercise the scope gate +// register their own restricted GetAllowedAccountsAPI expectation instead. +func allowAnyAccountScope(m *MockAuthService) { + m.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return([]string(nil), nil).Maybe() +} + +// validAzureOfferingsBody is a request body satisfying every field +// validateAzureOfferingsBody checks. Individual tests below build on it. +const validAzureOfferingsBody = `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-1", "quantity": 1}], + "targets": [{"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1, "billing_scope_id": "/subscriptions/sub-1"}] +}` + +// validAzureExecuteBody additionally satisfies the execute endpoint's +// mandatory spend-cap and currency guardrails. +const validAzureExecuteBody = `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-1", "quantity": 1}], + "targets": [{"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1, "billing_scope_id": "/subscriptions/sub-1"}], + "max_payment_due": "100.00", + "currency": "USD" +}` + +func azureOfferingsSource() AzureExchangeSourceBody { + return AzureExchangeSourceBody{ReservationID: "res-1", Quantity: 1} +} + +func azureOfferingsTarget() AzureExchangeTargetBody { + return AzureExchangeTargetBody{SKU: "Standard_D4s_v3", Location: "eastus", Term: "P1Y", Quantity: 1, BillingScopeID: "/subscriptions/sub-1"} +} + +// --- validateAzureOfferingsBody / validateAzureExecuteBody --- + +func TestValidateAzureOfferingsBody(t *testing.T) { + manySources := make([]AzureExchangeSourceBody, maxAzureExchangeItems+1) + for i := range manySources { + manySources[i] = azureOfferingsSource() + } + manyTargets := make([]AzureExchangeTargetBody, maxAzureExchangeItems+1) + for i := range manyTargets { + manyTargets[i] = azureOfferingsTarget() + } + + missingReservationID := azureOfferingsSource() + missingReservationID.ReservationID = "" + zeroSourceQty := azureOfferingsSource() + zeroSourceQty.Quantity = 0 + + missingSKU := azureOfferingsTarget() + missingSKU.SKU = "" + missingLocation := azureOfferingsTarget() + missingLocation.Location = "" + blankLocation := azureOfferingsTarget() + blankLocation.Location = " " + foreignBillingScope := azureOfferingsTarget() + foreignBillingScope.BillingScopeID = "/subscriptions/someone-elses-sub" + zeroTargetQty := azureOfferingsTarget() + zeroTargetQty.Quantity = 0 + unknownTerm := azureOfferingsTarget() + unknownTerm.Term = "P2Y" + + tests := []struct { + name string + body AzureCompatibleOfferingsRequestBody + wantErr string + }{ + { + "missing subscription_id", + AzureCompatibleOfferingsRequestBody{Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{azureOfferingsTarget()}}, + "subscription_id is required", + }, + { + "empty sources", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Targets: []AzureExchangeTargetBody{azureOfferingsTarget()}}, + "sources is required", + }, + { + "empty targets", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}}, + "targets is required", + }, + { + "too many sources", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: manySources, Targets: []AzureExchangeTargetBody{azureOfferingsTarget()}}, + "sources exceeds the maximum", + }, + { + "too many targets", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: manyTargets}, + "targets exceeds the maximum", + }, + { + "source missing reservation_id", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{missingReservationID}, Targets: []AzureExchangeTargetBody{azureOfferingsTarget()}}, + "sources[0].reservation_id is required", + }, + { + "source quantity zero", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{zeroSourceQty}, Targets: []AzureExchangeTargetBody{azureOfferingsTarget()}}, + "sources[0].quantity must be >= 1", + }, + { + "target missing sku", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{missingSKU}}, + "targets[0].sku is required", + }, + { + "target missing location", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{missingLocation}}, + "targets[0].location is required", + }, + { + // Whitespace-only survives the empty check but trims to "" in + // targetLocations, i.e. a Regions constraint entry no permission + // can name. Reject it at the boundary instead. + "target location is whitespace only", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{blankLocation}}, + "targets[0].location is required", + }, + { + "target names a foreign billing scope", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{foreignBillingScope}}, + `targets[0].billing_scope_id "/subscriptions/someone-elses-sub" is not the billing scope of subscription "sub-1"`, + }, + { + "target quantity zero", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{zeroTargetQty}}, + "targets[0].quantity must be >= 1", + }, + { + "target unknown term", + AzureCompatibleOfferingsRequestBody{SubscriptionID: "sub-1", Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, Targets: []AzureExchangeTargetBody{unknownTerm}}, + `targets[0].term: unsupported term "P2Y"`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateAzureOfferingsBody(tt.body) + 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, err.Error(), tt.wantErr) + }) + } +} + +func TestValidateAzureExecuteBody_RequiresCapAndCurrency(t *testing.T) { + base := AzureExecuteExchangeRequestBody{ + SubscriptionID: "sub-1", + Sources: []AzureExchangeSourceBody{azureOfferingsSource()}, + Targets: []AzureExchangeTargetBody{azureOfferingsTarget()}, + } + + missingCap := base + missingCap.Currency = "USD" + err := validateAzureExecuteBody(missingCap) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "a missing guardrail is a client fault, not a 500") + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "max_payment_due is required") + + missingCurrency := base + missingCurrency.MaxPaymentDue = "100.00" + err = validateAzureExecuteBody(missingCurrency) + require.Error(t, err) + ce, ok = IsClientError(err) + require.True(t, ok, "a missing guardrail is a client fault, not a 500") + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "currency is required") +} + +// TestValidateAzureExchangeTargets_BillingScopeCaseInsensitive pins the +// EqualFold comparison in validateAzureExchangeTargets. ARM returns resource +// IDs in mixed casing, so a client echoing back the scope Azure gave it must +// not be 400'd; an exact-match comparison would reject every such request +// while the rest of the suite (which only sends exact-case or deliberately +// foreign scopes) stayed green. +func TestValidateAzureExchangeTargets_BillingScopeCaseInsensitive(t *testing.T) { + caseVariant := azureOfferingsTarget() + caseVariant.BillingScopeID = "/SUBSCRIPTIONS/Sub-1" + require.NoError(t, validateAzureExchangeTargets([]AzureExchangeTargetBody{caseVariant}, "sub-1")) +} + +// --- getAzureCompatibleOfferings --- + +func TestGetAzureCompatibleOfferings_NoAuth(t *testing.T) { + h := &Handler{} + _, err := h.getAzureCompatibleOfferings(context.Background(), &events.LambdaFunctionURLRequest{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "authentication") +} + +func TestGetAzureCompatibleOfferings_InvalidJSON(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "view", "purchases").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth} + _, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: "not json", + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} + +// TestGetAzureCompatibleOfferings_UnregisteredSubscription mirrors +// TestListExchangeableAzureRIs_NoAzureAccountRegistered but asserts the +// stricter D5 contract for the offerings/execute endpoints: an +// unregistered subscription is a 404, not a graceful empty state (unlike +// the list endpoint, these endpoints cannot silently do nothing -- the +// caller asked to price a specific exchange). +func TestGetAzureCompatibleOfferings_UnregisteredSubscription(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "view", "purchases").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider) + require.Equal(t, "sub-1", externalID) + return nil, nil + } + + h := &Handler{auth: mockAuth, config: mockStore} + _, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 404, ce.code) + assert.Contains(t, err.Error(), `no Azure account registered for subscription "sub-1"`) +} + +func TestGetAzureCompatibleOfferings_AzureClientFault(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "view", "purchases").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything). + Return(nil, nil, &azcore.ResponseError{StatusCode: 400, ErrorCode: "ReservationNotFound"}) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: mockAuth, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} + +func TestGetAzureCompatibleOfferings_TransientErrorIs500(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "view", "purchases").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything). + Return(nil, nil, fmt.Errorf("azure: CalculateExchange: transport timeout")) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: mockAuth, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 500, ce.code, "a non-Azure-client-fault error must map to 500, not 400") +} + +// TestGetAzureCompatibleOfferings_HappyPath asserts the response carries a +// nil (not zero-coerced) NetPayable when Azure omits it, alongside a +// populated offering, proving the pointer money-field plumbing end to end. +func TestGetAzureCompatibleOfferings_HappyPath(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "view", "purchases").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + preview := &azurecompute.ExchangePreview{SessionID: "sess-preview-1"} // NetPayable intentionally nil + offerings := []azurecompute.CompatibleOffering{ + {SKU: "Standard_D4s_v3", Location: "eastus", Term: "P1Y", Quantity: 1, BillingCurrencyTotal: toPtr(42.5), CurrencyCode: "USD"}, + } + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return(preview, offerings, nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: mockAuth, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + res, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, + }) + require.NoError(t, err) + resp, ok := res.(*AzureCompatibleOfferingsResponse) + require.True(t, ok) + require.NotNil(t, resp.Preview) + assert.Nil(t, resp.Preview.NetPayable, "an omitted Azure NetPayable must surface as nil, never coerced to 0") + require.Len(t, resp.Offerings, 1) + require.NotNil(t, resp.Offerings[0].BillingCurrencyTotal) + assert.InDelta(t, 42.5, *resp.Offerings[0].BillingCurrencyTotal, 0.0001) +} + +// --- billing scope derivation (the charged scope is never client-chosen) --- + +// TestToAzureExchangeTargets_DerivesBillingScope pins that the scope Azure +// is told to charge comes from the authorized subscription, not from the +// request body. A body-supplied scope reaching the provider layer would +// move the charge outside the CloudAccount that checkAzureExecuteConstraints +// evaluated the caller's AccountIDs constraint against. +func TestToAzureExchangeTargets_DerivesBillingScope(t *testing.T) { + targets, err := toAzureExchangeTargets([]AzureExchangeTargetBody{ + {SKU: "Standard_D4s_v3", Location: "eastus", Term: "P1Y", Quantity: 1}, + {SKU: "Standard_D8s_v3", Location: "westus", Term: "P3Y", Quantity: 2, BillingScopeID: "/SUBSCRIPTIONS/SUB-1"}, + }, "sub-1") + require.NoError(t, err) + require.Len(t, targets, 2) + for i, tgt := range targets { + assert.Equal(t, "/subscriptions/sub-1", tgt.BillingScopeID, + "targets[%d] must be charged to the request's own subscription scope", i) + } +} + +// TestGetAzureCompatibleOfferings_ForeignBillingScopeRejected asserts a +// target naming another subscription's billing scope is refused before any +// Azure call. No CalculateExchange expectation is registered, so the mock +// panics (failing the test) the instant the guard stops rejecting. +func TestGetAzureCompatibleOfferings_ForeignBillingScopeRejected(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "view", "purchases").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: `{"subscription_id":"sub-1","sources":[{"reservation_id":"res-1","quantity":1}],` + + `"targets":[{"sku":"Standard_D4s_v3","location":"eastus","term":"P1Y","quantity":1,` + + `"billing_scope_id":"/subscriptions/victim-sub"}]}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "is not the billing scope of subscription") +} + +// TestExecuteAzureExchange_ForeignBillingScopeRejected is the same guard on +// the money-committing endpoint: the caller's AccountIDs constraint is +// checked against subscription_id, so a differing billing_scope_id would +// charge an account the check never looked at. +func TestExecuteAzureExchange_ForeignBillingScopeRejected(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + // Deliberately not newAzureExecuteMoneyPathHandler: the rejection must + // happen at body validation, before any permission-constraint or Azure + // call, so no expectation for those is registered here. + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: `{"subscription_id":"sub-1","sources":[{"reservation_id":"res-1","quantity":1}],` + + `"targets":[{"sku":"Standard_D4s_v3","location":"eastus","term":"P1Y","quantity":1,` + + `"billing_scope_id":"/subscriptions/victim-sub"}],"max_payment_due":"100.00","currency":"USD"}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "is not the billing scope of subscription") +} + +// --- allowed_accounts scoping (issue #1030) --- + +// scopedAzureAuth builds an auth mock for a user restricted to a single +// cloud account, used by the out-of-scope tests below. +func scopedAzureAuth(t *testing.T, action, resource string, allowed []string) *MockAuthService { + t.Helper() + ctx := context.Background() + m := new(MockAuthService) + m.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + m.On("HasPermissionAPI", ctx, "user-1", action, resource).Return(true, nil) + m.On("GetAllowedAccountsAPI", ctx, "user-1").Return(allowed, nil) + t.Cleanup(func() { m.AssertExpectations(t) }) + return m +} + +// scopedAzureStore returns a config store whose only registered Azure +// account is acct-other / "Other Team", i.e. not the one the scoped session +// is allowed to see. +func scopedAzureStore() *MockConfigStore { + s := &MockConfigStore{} + s.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-other", Name: "Other Team"}, nil + } + return s +} + +// TestGetAzureCompatibleOfferings_OutOfScopeSubscription asserts a user +// scoped to one account cannot price an exchange for another account's +// subscription. subscription_id is caller-controlled and the permission +// Constraints check only consults the permission's own AccountIDs, so +// without this gate the endpoint leaks another account's reservation +// pricing. +func TestGetAzureCompatibleOfferings_OutOfScopeSubscription(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: scopedAzureAuth(t, "view", "purchases", []string{"acct-mine"}), + config: scopedAzureStore(), + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, + }) + require.Error(t, err, "a scoped session must not price an out-of-scope subscription") + assert.ErrorIs(t, err, errNotFound) +} + +// TestExecuteAzureExchange_OutOfScopeSubscription is the money-path half: +// an out-of-scope subscription must never reach CalculateExchange or +// ExecuteExchange. Neither is registered on the mock, so any call panics. +func TestExecuteAzureExchange_OutOfScopeSubscription(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: scopedAzureAuth(t, "execute", "ri-exchange", []string{"acct-mine"}), + config: scopedAzureStore(), + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err, "a scoped session must not execute against an out-of-scope subscription") + assert.ErrorIs(t, err, errNotFound) +} + +// TestExecuteAzureExchange_InScopeSubscriptionAllowed is the other side of +// the gate: a scoped session whose allowed_accounts DO cover the resolved +// account still gets through to execution. +func TestExecuteAzureExchange_InScopeSubscriptionAllowed(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-fresh", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-fresh").Return( + &azurecompute.ExchangeResult{SessionID: "sess-fresh", Status: "Succeeded"}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + mockAuth := scopedAzureAuth(t, "execute", "ri-exchange", []string{"acct-mine"}) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", mock.Anything).Return(true, nil) + store := &MockConfigStore{} + store.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-mine", Name: "My Team"}, nil + } + + h := &Handler{auth: mockAuth, config: store, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + res, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.NoError(t, err) + resp, ok := res.(*AzureExecuteExchangeResponse) + require.True(t, ok) + assert.Equal(t, "sess-fresh", resp.SessionID) +} + +// TestExecuteAzureExchange_ScopeCheckPrecedesClientBuild_Unregistered proves +// the fix for the authz-ordering finding: requireAzureSubscriptionScope must +// run BEFORE buildAzureExchangeClient, exactly like getAzureCompatibleOfferings. +// Without that ordering, an unregistered subscription_id short-circuits in +// buildAzureExchangeClient with a distinguishable 404 ("no Azure account +// registered for subscription %q") before the scope check ever runs -- an +// enumeration oracle letting a scoped caller learn which subscription IDs +// are registered at all. This test exercises the REAL buildAzureExchangeClient +// path (no azureExchangeFactory), so the distinguishable message is only +// avoided when the scope check genuinely runs first. +func TestExecuteAzureExchange_ScopeCheckPrecedesClientBuild_Unregistered(t *testing.T) { + ctx := context.Background() + mockAuth := scopedAzureAuth(t, "execute", "ri-exchange", []string{"acct-mine"}) + + store := &MockConfigStore{} + store.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider) + require.Equal(t, "sub-1", externalID) + return nil, nil // unregistered: no account for this subscription at all + } + + // No azureExchangeFactory: the real buildAzureExchangeClient path runs, + // so a pre-fix reorder would reach its distinguishable 404 message. + h := &Handler{auth: mockAuth, config: store} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err, "a scoped session must not learn whether an unregistered subscription exists") + assert.ErrorIs(t, err, errNotFound, "must be the generic scope-check 404, not buildAzureExchangeClient's subscription-specific message") + assert.NotContains(t, err.Error(), "sub-1", "the error must not echo the subscription id back (that itself would be an enumeration signal)") +} + +// TestExecuteAzureExchange_ScopeCheckPrecedesClientBuild_RegisteredOutOfScope +// is the other half of the same finding: a registered-but-out-of-scope +// subscription must produce the IDENTICAL generic errNotFound as the +// unregistered case above, with no credential resolution attempted first. +// The account here uses client_secret auth mode with no stored secret +// (MockCredentialStore.LoadRaw always returns nil), so if the client were +// built before the scope check, credential resolution would fail with a +// DIFFERENT (non-404) error -- that divergence is exactly the signal this +// test catches if the ordering regresses. +func TestExecuteAzureExchange_ScopeCheckPrecedesClientBuild_RegisteredOutOfScope(t *testing.T) { + ctx := context.Background() + mockAuth := scopedAzureAuth(t, "execute", "ri-exchange", []string{"acct-mine"}) + + store := &MockConfigStore{} + store.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider) + require.Equal(t, "sub-1", externalID) + return &config.CloudAccount{ + ID: "acct-other", + Name: "Other Team", + Provider: "azure", + ExternalID: externalID, + AzureSubscriptionID: externalID, + AzureTenantID: "tenant-other", + AzureClientID: "client-other", + AzureAuthMode: "client_secret", + Enabled: true, + }, nil + } + + // No azureExchangeFactory, and a credential store that always fails + // client_secret resolution: if buildAzureExchangeClient ran before the + // scope check, this would surface as a credential-resolution error + // instead of the generic scope-denial. + h := &Handler{auth: mockAuth, config: store, credStore: &MockCredentialStore{}} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err, "a scoped session must not execute against an out-of-scope subscription") + assert.ErrorIs(t, err, errNotFound, "must be the generic scope-check 404, not a credential-resolution error from building the client first") +} + +// TestExecuteAzureExchange_ScopeCheckDenialsAreIndistinguishable directly +// compares the two scenarios above: the unregistered and the +// registered-but-out-of-scope subscription must produce the EXACT same +// error (same sentinel, same message), so a scoped caller cannot tell them +// apart by probing subscription IDs. +func TestExecuteAzureExchange_ScopeCheckDenialsAreIndistinguishable(t *testing.T) { + ctx := context.Background() + + unregisteredAuth := scopedAzureAuth(t, "execute", "ri-exchange", []string{"acct-mine"}) + unregisteredStore := &MockConfigStore{} + unregisteredStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return nil, nil + } + hUnregistered := &Handler{auth: unregisteredAuth, config: unregisteredStore} + _, unregisteredErr := hUnregistered.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + + outOfScopeAuth := scopedAzureAuth(t, "execute", "ri-exchange", []string{"acct-mine"}) + outOfScopeStore := scopedAzureStore() + outOfScopeOpsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(outOfScopeOpsClient) + t.Cleanup(func() { outOfScopeOpsClient.AssertExpectations(t) }) // no expectations: must never be called + hOutOfScope := &Handler{auth: outOfScopeAuth, config: outOfScopeStore, azureExchangeFactory: func(_ string) azureExchangeClient { return outOfScopeOpsClient }} + _, outOfScopeErr := hOutOfScope.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + + require.Error(t, unregisteredErr) + require.Error(t, outOfScopeErr) + assert.Equal(t, unregisteredErr.Error(), outOfScopeErr.Error(), "an unregistered subscription and a registered-but-out-of-scope one must be indistinguishable to the caller") + assert.ErrorIs(t, unregisteredErr, errNotFound) + assert.ErrorIs(t, outOfScopeErr, errNotFound) +} + +// --- executeAzureExchange: auth fail-closed --- + +func TestExecuteAzureExchange_NoAuth(t *testing.T) { + h := &Handler{} + _, err := h.executeAzureExchange(context.Background(), &events.LambdaFunctionURLRequest{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "authentication") +} + +func TestExecuteAzureExchange_MissingExecutePermission(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(false, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) +} + +// TestExecuteAzureExchange_ConstraintExceeded proves the fail-closed +// requirePermissionConstraints gate blocks execution BEFORE any pricing +// call: CalculateExchange must not be invoked when the constraint check +// denies the request. +func TestExecuteAzureExchange_ConstraintExceeded(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", mock.Anything).Return(false, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) // no CalculateExchange/ExecuteExchange expectations set + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: mockAuth, + config: mockStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "constraints") +} + +// validAzureExecuteBodyKWD mirrors validAzureExecuteBody but requests a +// non-USD currency, for the currency-blind-cap guardrail tests below. +const validAzureExecuteBodyKWD = `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-1", "quantity": 1}], + "targets": [{"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1, "billing_scope_id": "/subscriptions/sub-1"}], + "max_payment_due": "1000.00", + "currency": "KWD" +}` + +// TestExecuteAzureExchange_NonUSDCurrencyBlindCapRejected proves the fix for +// the currency-blind MaxPurchaseAmount finding: MaxPurchaseAmount is +// USD-denominated (matching the AWS precedent), so a raw float comparison +// against a non-USD amount would let e.g. 1000 KWD (worth far more than +// 1000 USD) clear a cap meant to bound USD spend. A non-USD request against +// a permission that DOES carry a MaxPurchaseAmount constraint must be +// refused with 403 rather than silently compared. +// +// The mock simulates a constrained permission across the two calls +// checkAzureExecuteConstraints makes: the sentinel-amount call is denied +// (the permission's real cap rejects the enormous sentinel), then the +// amount-neutralized disambiguation call is granted (every other dimension +// is fine), isolating the amount constraint as the specific cause. +func TestExecuteAzureExchange_NonUSDCurrencyBlindCapRejected(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && sets[0].MaxPurchaseAmount == math.MaxFloat64 + })).Return(false, nil) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && sets[0].MaxPurchaseAmount == 0 + })).Return(true, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) // no CalculateExchange/ExecuteExchange expectations set + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: mockAuth, + config: mockStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBodyKWD, + }) + require.Error(t, err, "a non-USD request against a MaxPurchaseAmount-constrained permission must be refused, not silently compared as if the amount were USD") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "USD-denominated") + assert.Contains(t, err.Error(), "KWD") +} + +// TestExecuteAzureExchange_USDCurrencyCapStillEnforced proves the fix does +// not regress the common case: a USD request is checked directly (single +// call, the real requested amount) and proceeds when within the cap. +func TestExecuteAzureExchange_USDCurrencyCapStillEnforced(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && sets[0].MaxPurchaseAmount == 100.00 + })).Return(true, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-usd-ok", NetPayable: toPtr(50.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-usd-ok").Return(&azurecompute.ExchangeResult{ + SessionID: "sess-usd-ok", NetPayable: toPtr(50.00), NetPayableCurrency: "USD", Status: "Succeeded", + }, nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: mockAuth, + config: mockStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, // USD, max_payment_due 100.00 + }) + require.NoError(t, err) +} + +// TestExecuteAzureExchange_NonUSDWithoutCapStillAllowed proves the fix does +// not over-reject: a non-USD request against a permission with NO +// MaxPurchaseAmount constraint must still be allowed through (only one +// constraint call is made, since the sentinel amount already passes when +// the permission has no cap). +func TestExecuteAzureExchange_NonUSDWithoutCapStillAllowed(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && sets[0].MaxPurchaseAmount == math.MaxFloat64 + })).Return(true, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-kwd-ok", NetPayable: toPtr(500.00), NetPayableCurrency: "KWD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-kwd-ok").Return(&azurecompute.ExchangeResult{ + SessionID: "sess-kwd-ok", NetPayable: toPtr(500.00), NetPayableCurrency: "KWD", Status: "Succeeded", + }, nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: mockAuth, + config: mockStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBodyKWD, + }) + require.NoError(t, err, "a non-USD request must still be allowed when the granting permission carries no MaxPurchaseAmount constraint") +} + +// TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive pins the +// currency comparison to the same case-insensitive rule +// checkAzureExecuteConstraints uses for its isUSD test. With an exact-match +// comparison a request of "usd" took the USD cap path there and was then +// always rejected here as a mismatch against Azure's "USD", making a +// well-formed request permanently unexecutable. +func TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive(t *testing.T) { + preview := &azurecompute.ExchangePreview{SessionID: "sess-1", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"} + require.NoError(t, checkAzureExchangeMoneyGuardrails(preview, big.NewRat(100, 1), "usd")) +} + +// TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused pins the +// defensive nil check: a future azureExchangeClient implementation that +// mistakenly returns (nil, nil, nil) from CalculateExchange must not panic +// this handler. +func TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused(t *testing.T) { + err := checkAzureExchangeMoneyGuardrails(nil, big.NewRat(100, 1), "USD") + require.Error(t, err, "a nil preview must be refused, not dereferenced") + assert.Contains(t, err.Error(), "nil preview") +} + +// --- executeAzureExchange: validation --- + +func TestExecuteAzureExchange_MalformedJSON(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: "not json", + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} + +func TestExecuteAzureExchange_MissingMaxPaymentDue(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, // no max_payment_due / currency + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "max_payment_due is required") +} + +func TestExecuteAzureExchange_InvalidMaxPaymentDue(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: `{"subscription_id":"sub-1","sources":[{"reservation_id":"res-1","quantity":1}],"targets":[{"sku":"Standard_D4s_v3","location":"eastus","term":"P1Y","quantity":1,"billing_scope_id":"/subscriptions/sub-1"}],"max_payment_due":"not-a-number","currency":"USD"}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "invalid max_payment_due") +} + +// --- executeAzureExchange: money-path guardrails --- +// +// Each test below asserts BOTH the response status and that +// client.ExecuteExchange was never called (no matching mock expectation is +// registered, so testify panics -- failing the test -- the instant a code +// change removes the guard and lets execution reach ExecuteExchange). + +// newAzureExecuteMoneyPathHandler builds a Handler wired for the money-path +// guardrail tests: auth grants execute:ri-exchange and passes the +// constraint check unconditionally, and the Azure client factory returns +// opsClient. Shared by every guardrail test below so each one only sets up +// the CalculateExchange response under test. +func newAzureExecuteMoneyPathHandler(t *testing.T, opsClient azureExchangeClient) *Handler { + t.Helper() + return newAzureExecuteHandler(t, opsClient, true) +} + +// newAzureExecuteSourceGateHandler is the same handler with the constraint +// check marked optional, for tests whose subject is a gate that legitimately +// runs BEFORE it: source ownership, and the tenant listing that feeds both it +// and the Regions dimension. Those tests must not require a call the handler +// correctly never makes. +func newAzureExecuteSourceGateHandler(t *testing.T, opsClient azureExchangeClient) *Handler { + t.Helper() + return newAzureExecuteHandler(t, opsClient, false) +} + +func newAzureExecuteHandler(t *testing.T, opsClient azureExchangeClient, requireConstraintCheck bool) *Handler { + t.Helper() + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + constraints := mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", mock.Anything).Return(true, nil) + if !requireConstraintCheck { + constraints.Maybe() + } + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + return &Handler{ + auth: mockAuth, + config: mockStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } +} + +func TestExecuteAzureExchange_CapExceeded(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-fresh", NetPayable: toPtr(500.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteMoneyPathHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, // max_payment_due: "100.00" + }) + require.Error(t, err, "a quoted net payable above the cap must refuse to execute") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 422, ce.code) + assert.Contains(t, err.Error(), "exceeds max_payment_due") +} + +func TestExecuteAzureExchange_PolicyErrorsBlockExecution(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{ + SessionID: "sess-fresh", + NetPayable: toPtr(10.00), + NetPayableCurrency: "USD", + PolicyErrors: []string{"reservations must share a billing account"}, + }, + []azurecompute.CompatibleOffering{}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteMoneyPathHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err, "a non-empty PolicyErrors preview must refuse to execute") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 422, ce.code) + assert.Contains(t, err.Error(), "reservations must share a billing account") +} + +func TestExecuteAzureExchange_CurrencyMismatchBlocksExecution(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-fresh", NetPayable: toPtr(10.00), NetPayableCurrency: "EUR"}, + []azurecompute.CompatibleOffering{}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteMoneyPathHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, // requests "currency":"USD" + }) + require.Error(t, err, "a quoted currency that does not match the requested currency must refuse to execute") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 422, ce.code) + assert.Contains(t, err.Error(), `quoted currency "EUR" does not match requested currency "USD"`) +} + +func TestExecuteAzureExchange_NilNetPayableRefused(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-fresh"}, // NetPayable intentionally nil + []azurecompute.CompatibleOffering{}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteMoneyPathHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err, "a nil NetPayable must never be treated as a free exchange") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 422, ce.code) + assert.Contains(t, err.Error(), "did not return a net payable amount") +} + +// TestExecuteAzureExchange_HappyPath is the central proof of the D2 +// server-re-quote design: ExecuteExchange must receive EXACTLY the +// SessionID this test's CalculateExchange mock returned, never a +// client-supplied value (the request body carries none). +func TestExecuteAzureExchange_HappyPath(t *testing.T) { + ctx := context.Background() + const freshSessionID = "sess-server-issued-99" + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{ + SessionID: freshSessionID, + NetPayable: toPtr(75.00), + NetPayableCurrency: "USD", + RefundsTotal: toPtr(20.00), + PurchasesTotal: toPtr(95.00), + }, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, mock.MatchedBy(func(sessionID string) bool { + return sessionID == freshSessionID + })).Return(&azurecompute.ExchangeResult{ + SessionID: freshSessionID, + NetPayable: toPtr(75.00), + NetPayableCurrency: "USD", + Status: "Succeeded", + }, nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteMoneyPathHandler(t, opsClient) + res, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, // cap "100.00" USD >= quoted 75.00 USD + }) + require.NoError(t, err) + resp, ok := res.(*AzureExecuteExchangeResponse) + require.True(t, ok) + assert.Equal(t, freshSessionID, resp.SessionID) + assert.Equal(t, "Succeeded", resp.Status) + require.NotNil(t, resp.NetPayable) + assert.InDelta(t, 75.00, *resp.NetPayable, 0.0001) + assert.Equal(t, "USD", resp.NetPayableCurrency) + require.NotNil(t, resp.RefundsTotal) + assert.InDelta(t, 20.00, *resp.RefundsTotal, 0.0001) + require.NotNil(t, resp.PurchasesTotal) + assert.InDelta(t, 95.00, *resp.PurchasesTotal, 0.0001) +} + +// --- executeAzureExchange: the SEC-01 constraint set's own contents --- + +// validAzureExecuteBodyMultiRegion targets two locations plus a duplicate, +// so the Regions dimension asserted below also pins targetLocations' dedup. +const validAzureExecuteBodyMultiRegion = `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-1", "quantity": 1}], + "targets": [ + {"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1}, + {"sku": "Standard_D8s_v3", "location": "westeurope", "term": "P3Y", "quantity": 2}, + {"sku": "Standard_D2s_v3", "location": "eastus", "term": "P1Y", "quantity": 1} + ], + "max_payment_due": "100.00", + "currency": "USD" +}` + +// TestExecuteAzureExchange_ConstraintSetPinsAllDimensions asserts the FULL +// constraint set checkAzureExecuteConstraints submits, not just its amount +// dimension. +// +// The other four dimensions are what confine an irreversible exchange to +// the caller's authorized blast radius: AccountIDs to the CloudAccount +// resolved from the request's subscription_id (the subscription the money +// actually lands in), Providers/Services to azure/compute, and Regions to +// every target location. Every other execute test matches the constraint +// argument with mock.Anything or an amount-only MatchedBy, so dropping +// Regions -- letting a permission scoped to one region commit an exchange +// into another -- or pointing AccountIDs at the wrong account would leave +// the whole suite green. +func TestExecuteAzureExchange_ConstraintSetPinsAllDimensions(t *testing.T) { + ctx := context.Background() + var captured []auth.PermissionConstraints + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + captured = sets + return true + })).Return(true, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + assert.Equal(t, "azure", provider) + assert.Equal(t, "sub-1", externalID, "the account gating the exchange must be resolved from the request's own subscription_id") + return &config.CloudAccount{ID: "acct-sub-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-dims", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-dims").Return( + &azurecompute.ExchangeResult{SessionID: "sess-dims", Status: "Succeeded"}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth, config: mockStore, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBodyMultiRegion, + }) + require.NoError(t, err) + + require.Len(t, captured, 1) + c := captured[0] + assert.Equal(t, []string{"acct-sub-1"}, c.AccountIDs, "AccountIDs must name the CloudAccount the exchange is billed to") + assert.Equal(t, []string{"azure"}, c.Providers) + assert.Equal(t, []string{"compute"}, c.Services) + assert.Equal(t, []string{"eastus", "westeurope"}, c.Regions, + "Regions must cover every target location plus every source region, de-duplicated in first-seen order (the source here is in eastus, which the targets already name)") + assert.InDelta(t, 100.00, c.MaxPurchaseAmount, 0.0001) +} + +// TestTargetLocations_CanonicalizesCase pins the normalization targetLocations +// applies before its output becomes the Regions dimension of the +// execute:ri-exchange constraint check. +// +// The permission side compares region names literally, so an un-normalized +// "EastUS" -- the spelling the Azure portal shows -- would not match a +// permission stored as "eastus", and the caller would be 403'd out of a +// region they legitimately hold. Normalizing before the dedup additionally +// collapses "EastUS" and "eastus" into the single region they actually are, +// instead of submitting two entries that must both be permitted. +func TestTargetLocations_CanonicalizesCase(t *testing.T) { + tests := []struct { + name string + targets []AzureExchangeTargetBody + want []string + }{ + { + name: "no targets yields an empty set", + targets: nil, + want: []string{}, + }, + { + name: "already-canonical locations pass through in first-seen order", + targets: []AzureExchangeTargetBody{{Location: "westeurope"}, {Location: "eastus"}}, + want: []string{"westeurope", "eastus"}, + }, + { + name: "mixed-case location is lower-cased", + targets: []AzureExchangeTargetBody{{Location: "EastUS"}}, + want: []string{"eastus"}, + }, + { + name: "case variants of one region collapse into a single entry", + targets: []AzureExchangeTargetBody{{Location: "EastUS"}, {Location: "eastus"}, {Location: "EASTUS"}}, + want: []string{"eastus"}, + }, + { + name: "surrounding whitespace is trimmed before dedup", + targets: []AzureExchangeTargetBody{{Location: " EastUS "}, {Location: "eastus"}}, + want: []string{"eastus"}, + }, + { + name: "distinct regions survive while their case variants collapse", + targets: []AzureExchangeTargetBody{ + {Location: "EastUS"}, + {Location: "WestEurope"}, + {Location: "eastus"}, + }, + want: []string{"eastus", "westeurope"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, targetLocations(tt.targets)) + }) + } +} + +// TestExecuteAzureExchange_ConstraintRegionsAreLowerCased drives the same +// normalization through the handler, so the constraint set actually submitted +// for a portal-cased request is pinned and not merely the helper's return +// value. Without it, a body naming "EastUS" and "eastus" would submit two +// Regions entries in the portal's casing and the permission check would +// compare them literally against the lower-case names permissions store. +func TestExecuteAzureExchange_ConstraintRegionsAreLowerCased(t *testing.T) { + ctx := context.Background() + var captured []auth.PermissionConstraints + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + captured = sets + return true + })).Return(true, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-sub-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-case", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-case").Return( + &azurecompute.ExchangeResult{SessionID: "sess-case", Status: "Succeeded"}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth, config: mockStore, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-1", "quantity": 1}], + "targets": [ + {"sku": "Standard_D4s_v3", "location": "EastUS", "term": "P1Y", "quantity": 1}, + {"sku": "Standard_D8s_v3", "location": "WestEurope", "term": "P3Y", "quantity": 2}, + {"sku": "Standard_D2s_v3", "location": "eastus", "term": "P1Y", "quantity": 1} + ], + "max_payment_due": "100.00", + "currency": "USD" + }`, + }) + require.NoError(t, err) + + require.Len(t, captured, 1) + assert.Equal(t, []string{"eastus", "westeurope"}, captured[0].Regions, + "portal-cased target locations must reach the permission check lower-cased and de-duplicated") +} + +// TestExecuteAzureExchange_LowercaseUSDTakesUSDCapPath closes the other half +// of the currency case-insensitivity pairing. +// +// TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive calls the +// guardrail helper directly, so it never exercises the isUSD test in +// checkAzureExecuteConstraints. With an exact-match isUSD, a "usd" request +// would take the non-USD sentinel path and be 403'd against any +// MaxPurchaseAmount-carrying permission. Asserting the constraint check +// receives the REAL 100.00 cap (not math.MaxFloat64) pins the USD path. +func TestExecuteAzureExchange_LowercaseUSDTakesUSDCapPath(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && sets[0].MaxPurchaseAmount == 100.00 + })).Return(true, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-lower-usd", NetPayable: toPtr(50.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-lower-usd").Return( + &azurecompute.ExchangeResult{SessionID: "sess-lower-usd", Status: "Succeeded"}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth, config: mockStore, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-1", "quantity": 1}], + "targets": [{"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1}], + "max_payment_due": "100.00", + "currency": "usd" + }`, + }) + require.NoError(t, err, `a lowercase "usd" request must take the USD cap path in both the constraint check and the money guardrails`) +} + +// TestExecuteAzureExchange_NonUSDDeniedOnOtherDimension pins the other half +// of checkAzureExecuteConstraints' disambiguation branch: when the +// amount-neutralized retry ALSO denies, some other dimension +// (account/provider/service/region) is the real cause and its generic error +// must be returned unchanged. Reporting the currency-specific 403 there +// would send an operator chasing an FX problem that does not exist. +func TestExecuteAzureExchange_NonUSDDeniedOnOtherDimension(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + // Both the sentinel-amount call and the amount-neutralized retry deny. + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", mock.Anything).Return(false, nil).Twice() + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + opsClient := new(mockAzureExchangeOpsClient) // neither pricing nor execution may be reached + ownsAzureSource(opsClient) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth, config: mockStore, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBodyKWD, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "constraints") + assert.NotContains(t, err.Error(), "USD-denominated", + "a denial caused by another dimension must not be reported as a currency problem") +} + +// --- executeAzureExchange: failure classification on the money path --- + +// TestExecuteAzureExchange_RequoteFailureAbortsBeforeCommit pins the +// invariant that a failed server-side re-quote aborts before ExecuteExchange +// is ever reached. No ExecuteExchange expectation is registered, so testify +// fails the test the instant a code change lets execution proceed on an +// unpriced exchange. +func TestExecuteAzureExchange_RequoteFailureAbortsBeforeCommit(t *testing.T) { + tests := []struct { + name string + quoteErr error + wantCode int + }{ + {"azure client fault", &azcore.ResponseError{StatusCode: 400, ErrorCode: "ReservationNotFound"}, 400}, + {"transient failure", fmt.Errorf("azure: CalculateExchange: transport timeout"), 500}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return(nil, nil, tt.quoteErr) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteMoneyPathHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, tt.wantCode, ce.code) + opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) + }) + } +} + +// TestExecuteAzureExchange_CommitFailureClassification covers the status +// mapping on the irreversible commit call itself, which every other execute +// test resolves successfully. An Azure 4xx must surface as a 400 carrying +// Azure's own message (the caller's input was wrong); anything else must +// stay a 500 with the generic operation message, so a transient failure is +// never mislabelled as a permanent client fault the caller should not retry. +func TestExecuteAzureExchange_CommitFailureClassification(t *testing.T) { + tests := []struct { + name string + commitErr error + wantCode int + wantDetail string + }{ + {"azure client fault", &azcore.ResponseError{StatusCode: 409, ErrorCode: "ReservationAlreadyExchanged"}, 400, "ReservationAlreadyExchanged"}, + {"transient failure", fmt.Errorf("azure: ExecuteExchange: transport timeout"), 500, "exchange execution failed"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-commit-fail", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-commit-fail").Return(nil, tt.commitErr) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteMoneyPathHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, tt.wantCode, ce.code) + assert.Contains(t, err.Error(), tt.wantDetail) + }) + } +} + +// --- checkAzureExchangeMoneyGuardrails: cap boundary and refunds --- + +// TestCheckAzureExchangeMoneyGuardrails_CapBoundaryAndRefunds pins the cap +// comparison at the two points the existing 75-vs-100 / 500-vs-100 tests +// leave open: a quote landing exactly ON the cap must be allowed (a `>=` +// comparison would reject every exactly-budgeted exchange), and a negative +// NetPayable -- the refund side of a downgrade, and a common Azure exchange +// outcome -- must never be treated as exceeding a positive cap. +func TestCheckAzureExchangeMoneyGuardrails_CapBoundaryAndRefunds(t *testing.T) { + tests := []struct { + name string + netPayable float64 + wantErr bool + }{ + {"exactly at cap", 100.00, false}, + {"one cent over cap", 100.01, true}, + {"negative net payable is a refund", -250.00, false}, + {"zero net payable", 0, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + preview := &azurecompute.ExchangePreview{ + SessionID: "sess-1", + NetPayable: toPtr(tt.netPayable), + NetPayableCurrency: "USD", + } + err := checkAzureExchangeMoneyGuardrails(preview, big.NewRat(100, 1), "USD") + if tt.wantErr { + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 422, ce.code) + return + } + require.NoError(t, err) + }) + } +} + +// TestGetAzureCompatibleOfferings_ScopeCheckPrecedesClientBuild mirrors the +// execute endpoint's ordering tests on the read-only quote endpoint, which +// had none. +// +// TestGetAzureCompatibleOfferings_OutOfScopeSubscription injects an +// azureExchangeFactory, so the client build always succeeds there and a +// reordering of requireAzureSubscriptionScope past buildAzureExchangeClient +// would still surface errNotFound -- leaving the same enumeration oracle the +// execute endpoint is explicitly guarded against. These two exercise the +// REAL buildAzureExchangeClient path so the distinguishable 404 ("no Azure +// account registered for subscription %q") is only avoided when the scope +// check genuinely runs first, and require both denials to be identical. +func TestGetAzureCompatibleOfferings_ScopeCheckPrecedesClientBuild(t *testing.T) { + ctx := context.Background() + + unregisteredStore := &MockConfigStore{} + unregisteredStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider) + require.Equal(t, "sub-1", externalID) + return nil, nil // unregistered: no account for this subscription at all + } + hUnregistered := &Handler{auth: scopedAzureAuth(t, "view", "purchases", []string{"acct-mine"}), config: unregisteredStore} + _, unregisteredErr := hUnregistered.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, + }) + require.Error(t, unregisteredErr, "a scoped session must not learn whether an unregistered subscription exists") + assert.ErrorIs(t, unregisteredErr, errNotFound) + assert.NotContains(t, unregisteredErr.Error(), "sub-1", + "the error must not echo the subscription id back (that itself would be an enumeration signal)") + + // Registered but out of scope, with a credential store that always fails + // client_secret resolution: building the client first would surface a + // credential-resolution error instead of the generic scope denial. + outOfScopeStore := &MockConfigStore{} + outOfScopeStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, externalID string) (*config.CloudAccount, error) { + return &config.CloudAccount{ + ID: "acct-other", + Name: "Other Team", + Provider: "azure", + ExternalID: externalID, + AzureSubscriptionID: externalID, + AzureTenantID: "tenant-other", + AzureClientID: "client-other", + AzureAuthMode: "client_secret", + Enabled: true, + }, nil + } + hOutOfScope := &Handler{ + auth: scopedAzureAuth(t, "view", "purchases", []string{"acct-mine"}), + config: outOfScopeStore, + credStore: &MockCredentialStore{}, + } + _, outOfScopeErr := hOutOfScope.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureOfferingsBody, + }) + require.Error(t, outOfScopeErr, "a scoped session must not price an out-of-scope subscription") + assert.ErrorIs(t, outOfScopeErr, errNotFound, + "must be the generic scope-check 404, not a credential-resolution error from building the client first") + + assert.Equal(t, unregisteredErr.Error(), outOfScopeErr.Error(), + "both denials must be byte-identical so a scoped caller cannot tell registered from unregistered subscriptions") +} + +// --- issue #1527: source reservations must be owned by the authorized subscription --- + +// foreignSourceBody names a source reservation that exists in the tenant but +// is billed to a DIFFERENT subscription than the authorized one. This is the +// cross-subscription attack shape: every destination gate (allowed_accounts +// scope, the execute:ri-exchange AccountIDs constraint, the derived target +// billing scope) is satisfied for sub-1, and only the source belongs to +// someone else. +const foreignSourceBody = `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-belongs-to-other", "quantity": 1}], + "targets": [{"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1}], + "max_payment_due": "100.00", + "currency": "USD" +}` + +// tenantListingWithForeignReservation is what a tenant-wide +// ListExchangeableReservations returns: the caller's own reservation AND +// another subscription's, because the Azure Capacity API enumerates +// reservation orders across the whole tenant. +func tenantListingWithForeignReservation() []azurecompute.ExchangeableReservation { + return []azurecompute.ExchangeableReservation{ + {ReservationID: "res-1", BillingScopeID: "/subscriptions/sub-1", Quantity: 1}, + {ReservationID: "res-belongs-to-other", BillingScopeID: "/subscriptions/other-sub", Quantity: 1}, + } +} + +// TestExecuteAzureExchange_ForeignSourceReservationRefused is the issue #1527 +// regression test: it reproduces the real cross-subscription scenario end to +// end through the handler. +// +// Pre-fix, sources were validated only for a non-empty reservation_id and +// quantity >= 1, so this request reached CalculateExchange and then +// ExecuteExchange, handing back another subscription's commitment and buying +// the replacement into the caller's own billing scope. Azure RBAC on the +// reservation order was the only thing standing in the way. +// +// No CalculateExchange or ExecuteExchange expectation is registered, so +// testify fails this test the instant a regression lets execution past the +// gate -- the money path must not be reached at all. +func TestExecuteAzureExchange_ForeignSourceReservationRefused(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + opsClient.On("ListExchangeableReservations", mock.Anything).Return(tenantListingWithForeignReservation(), nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteSourceGateHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: foreignSourceBody, + }) + require.Error(t, err, "a source billed to another subscription must never be exchanged") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "sources[0].reservation_id") + opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything) + opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) +} + +// TestGetAzureCompatibleOfferings_ForeignSourceReservationRefused is the +// read-side half: pricing another subscription's reservation leaks its +// commitment value, so the same gate applies to the quote endpoint. +func TestGetAzureCompatibleOfferings_ForeignSourceReservationRefused(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "view", "purchases").Return(true, nil) + allowAnyAccountScope(mockAuth) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + opsClient := new(mockAzureExchangeOpsClient) + opsClient.On("ListExchangeableReservations", mock.Anything).Return(tenantListingWithForeignReservation(), nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{auth: mockAuth, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }} + _, err := h.getAzureCompatibleOfferings(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: foreignSourceBody, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything) +} + +// TestExecuteAzureExchange_SourceOwnershipLookupFailureRefuses pins the +// fail-closed posture on the lookup itself: if the listing call fails we +// cannot establish ownership, and permitting the exchange would restore the +// exact gap the gate exists to close. 502 rather than 500 because the +// upstream dependency, not this service, is what failed. +// +// It uses newAzureExecuteSourceGateHandler because the listing precedes the +// constraint check -- the Regions dimension cannot be assembled without +// knowing where the sources live -- so a listing failure legitimately returns +// before HasPermissionForConstraintsAPI is ever called. +func TestExecuteAzureExchange_SourceOwnershipLookupFailureRefuses(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + opsClient.On("ListExchangeableReservations", mock.Anything). + Return(nil, fmt.Errorf("azure: list reservations: transport timeout")) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureExecuteSourceGateHandler(t, opsClient) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.Error(t, err, "unverifiable ownership must refuse, never fall through to permitting") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 502, ce.code) + opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything) + opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) +} + +// TestRequireAzureSourceOwnership covers the authorization rule directly, +// including the fail-closed branches that are awkward to drive through the +// whole handler. +func TestRequireAzureSourceOwnership(t *testing.T) { + mine := azurecompute.ExchangeableReservation{ReservationID: "res-1", BillingScopeID: "/subscriptions/sub-1"} + theirs := azurecompute.ExchangeableReservation{ReservationID: "res-2", BillingScopeID: "/subscriptions/sub-2"} + ownerless := azurecompute.ExchangeableReservation{ReservationID: "res-3", BillingScopeID: ""} + + tests := []struct { + name string + owned []azurecompute.ExchangeableReservation + sources []AzureExchangeSourceBody + wantErr bool + reason string + }{ + { + name: "own reservation is allowed", + owned: []azurecompute.ExchangeableReservation{mine, theirs}, + sources: []AzureExchangeSourceBody{{ReservationID: "res-1", Quantity: 1}}, + }, + { + name: "ARM id and scope casing must not matter", + owned: []azurecompute.ExchangeableReservation{{ReservationID: "RES-1", BillingScopeID: "/SUBSCRIPTIONS/SUB-1"}}, + sources: []AzureExchangeSourceBody{{ReservationID: "res-1", Quantity: 1}}, + reason: "ARM returns resource ids in mixed casing; a case-sensitive compare would refuse legitimate requests", + }, + { + name: "another subscription's reservation is refused", + owned: []azurecompute.ExchangeableReservation{mine, theirs}, + sources: []AzureExchangeSourceBody{{ReservationID: "res-2", Quantity: 1}}, + wantErr: true, + }, + { + name: "reservation absent from the tenant listing is refused", + owned: []azurecompute.ExchangeableReservation{mine}, + sources: []AzureExchangeSourceBody{{ReservationID: "res-does-not-exist", Quantity: 1}}, + wantErr: true, + reason: "fail closed: an id we cannot resolve has unknown ownership", + }, + { + name: "reservation with no reported billing scope is refused", + owned: []azurecompute.ExchangeableReservation{ownerless}, + sources: []AzureExchangeSourceBody{{ReservationID: "res-3", Quantity: 1}}, + wantErr: true, + reason: "fail closed: absent scope means ownership unknown, not unrestricted", + }, + { + name: "one foreign source among several taints the whole request", + owned: []azurecompute.ExchangeableReservation{mine, theirs}, + sources: []AzureExchangeSourceBody{{ReservationID: "res-1", Quantity: 1}, {ReservationID: "res-2", Quantity: 1}}, + wantErr: true, + reason: "an exchange is all-or-nothing; a single unauthorized source must sink it", + }, + { + name: "empty listing refuses everything", + owned: nil, + sources: []AzureExchangeSourceBody{{ReservationID: "res-1", Quantity: 1}}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := requireAzureSourceOwnership(tt.owned, tt.sources, "sub-1") + if !tt.wantErr { + require.NoError(t, err, tt.reason) + return + } + require.Error(t, err, tt.reason) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + }) + } +} + +// TestRequireAzureSourceOwnership_DenialsAreIndistinguishable pins the +// anti-enumeration property: refusing a reservation that belongs to another +// subscription must be byte-identical to refusing one that does not exist. +// Otherwise the gate itself becomes an oracle for probing which reservation +// ids are real elsewhere in the tenant -- the same failure this PR already +// guards against for subscription ids. +func TestRequireAzureSourceOwnership_DenialsAreIndistinguishable(t *testing.T) { + owned := []azurecompute.ExchangeableReservation{ + {ReservationID: "res-2", BillingScopeID: "/subscriptions/sub-2"}, + } + foreign := requireAzureSourceOwnership(owned, []AzureExchangeSourceBody{{ReservationID: "res-2", Quantity: 1}}, "sub-1") + missing := requireAzureSourceOwnership(owned, []AzureExchangeSourceBody{{ReservationID: "res-nope", Quantity: 1}}, "sub-1") + + require.Error(t, foreign) + require.Error(t, missing) + assert.Equal(t, foreign.Error(), missing.Error(), + "a caller must not be able to tell an existing foreign reservation from a nonexistent one") + assert.NotContains(t, foreign.Error(), "sub-2", + "the owning subscription must never be echoed back") +} + +// --- the Regions constraint must bound the SOURCES too, not only the targets --- + +// newAzureRegionScopedHandler builds a handler whose caller holds +// execute:ri-exchange constrained to exactly one region, emulating +// auth.matchAllRegionsConstraint's all-match rule: the permission grants the +// request only when EVERY region in the submitted constraint set is the +// permitted one. Comparison is a plain equality test because the constraint +// set reaches the auth layer canonically lower-cased. +// +// captured receives the constraint set the handler submitted, so a test can +// assert which regions the handler thought the operation touches rather than +// only that it was denied. +func newAzureRegionScopedHandler(t *testing.T, ctx context.Context, opsClient azureExchangeClient, permittedRegion string, captured *[]auth.PermissionConstraints) *Handler { + t.Helper() + permits := func(sets []auth.PermissionConstraints) bool { + *captured = sets + for _, s := range sets { + for _, r := range s.Regions { + if r != permittedRegion { + return false + } + } + } + return true + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-1", "execute", "ri-exchange").Return(true, nil) + allowAnyAccountScope(mockAuth) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(permits)).Return(true, nil).Maybe() + mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { return !permits(sets) })).Return(false, nil).Maybe() + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: "acct-1"}, nil + } + + return &Handler{ + auth: mockAuth, + config: mockStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } +} + +// foreignRegionSourceBody names a source reservation the caller's own +// subscription DOES pay for, but which lives in a region the caller is not +// permitted in, while every target stays inside the permitted region. +const foreignRegionSourceBody = `{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": "res-west", "quantity": 1}], + "targets": [{"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1}], + "max_payment_due": "100.00", + "currency": "USD" +}` + +// TestExecuteAzureExchange_SourceRegionOutsidePermissionRefused asserts the +// security property, not the shape of the constraint set: a caller permitted +// only in eastus must be DENIED an exchange whose source reservation lives in +// westeurope, even though every target is in eastus. +// +// Pre-fix the Regions dimension was built from targetLocations alone, so this +// request submitted Regions{"eastus"}, the permission granted it, and the +// exchange committed: a westeurope commitment the caller was never authorized +// to touch is handed back to Azure and its value relocated to eastus, +// irreversibly. Nothing else covered the gap -- requireAzureSourceOwnership +// keys on the reservation's BillingScopeID (which legitimately is sub-1 here), +// and no other gate reads a source's region at all. The AWS analog is safe +// only because AWS exchanges are same-region; cross-region is precisely what +// this endpoint exists to do. +// +// TestExecuteAzureExchange_ConstraintSetPinsAllDimensions cannot catch this: +// it asserts the constraint set as built, so it stayed green with the gap +// present. +// +// No CalculateExchange or ExecuteExchange expectation is registered, so +// testify fails this test the instant a regression lets execution past the +// gate -- the money path must not be reached at all. +func TestExecuteAzureExchange_SourceRegionOutsidePermissionRefused(t *testing.T) { + ctx := context.Background() + var captured []auth.PermissionConstraints + + opsClient := new(mockAzureExchangeOpsClient) + opsClient.On("ListExchangeableReservations", mock.Anything).Return([]azurecompute.ExchangeableReservation{ + {ReservationID: "res-west", BillingScopeID: "/subscriptions/sub-1", Region: "westeurope", Quantity: 1}, + }, nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureRegionScopedHandler(t, ctx, opsClient, "eastus", &captured) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: foreignRegionSourceBody, + }) + + require.Error(t, err, "an eastus-only caller must not be able to consume a westeurope reservation") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + require.Len(t, captured, 1) + assert.Contains(t, captured[0].Regions, "westeurope", + "the source's own region must reach the permission check; the exchange mutates it too") + opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything) + opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) +} + +// TestExecuteAzureExchange_UnreportedSourceRegionRefused covers the +// empty-region trap. ExchangeableReservation.Region "may be empty for +// reservations with AppliedScopeType == Shared", so the fold must not simply +// skip a source Azure described without a region -- that would make "we don't +// know where this is" mean "no region restriction applies", the same fail-open +// shape PR #1495 fixed. The unknownRegionConstraint sentinel makes it deny +// instead. +func TestExecuteAzureExchange_UnreportedSourceRegionRefused(t *testing.T) { + ctx := context.Background() + var captured []auth.PermissionConstraints + + opsClient := new(mockAzureExchangeOpsClient) + opsClient.On("ListExchangeableReservations", mock.Anything).Return([]azurecompute.ExchangeableReservation{ + // Billed to the caller's own subscription, but Azure reported no region. + {ReservationID: "res-1", BillingScopeID: "/subscriptions/sub-1", Region: "", Quantity: 1}, + }, nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureRegionScopedHandler(t, ctx, opsClient, "eastus", &captured) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, // sources: res-1, targets: eastus + }) + + require.Error(t, err, "a source whose region Azure did not report must deny, never fall through as unconstrained") + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + require.Len(t, captured, 1) + assert.Contains(t, captured[0].Regions, unknownRegionConstraint, + "an unreported source region must surface as a sentinel no permission can name, not be dropped") + opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything) + opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) +} + +// TestExecuteAzureExchange_UnreportedSourceRegionAllowedWithoutRegionScope is +// the other half of the sentinel's contract: it denies a region-scoped +// permission, but must NOT penalize a caller who has no Regions constraint at +// all (auth.matchAllRegionsConstraint treats an empty permission list as "no +// restriction"). Otherwise every Shared-scope reservation would become +// unexchangeable for everyone. +func TestExecuteAzureExchange_UnreportedSourceRegionAllowedWithoutRegionScope(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + opsClient.On("ListExchangeableReservations", mock.Anything).Return([]azurecompute.ExchangeableReservation{ + {ReservationID: "res-1", BillingScopeID: "/subscriptions/sub-1", Region: "", Quantity: 1}, + }, nil) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-shared", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", ctx, "sess-shared").Return( + &azurecompute.ExchangeResult{SessionID: "sess-shared", Status: "Succeeded"}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + // newAzureExecuteMoneyPathHandler models an unconstrained permission. + h := newAzureExecuteMoneyPathHandler(t, opsClient) + resp, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, + }) + require.NoError(t, err, "a caller with no Regions constraint must not be blocked by the unknown-region sentinel") + require.NotNil(t, resp) +} + +// TestExecuteAzureExchange_UnownedSourceDenialsAreIndistinguishable pins the +// anti-enumeration property at the HANDLER level, across the ownership gate +// and the constraint check together. +// +// TestRequireAzureSourceOwnership_DenialsAreIndistinguishable calls the gate +// directly, so it stays green even when the surrounding path leaks: it proves +// the gate's two denials match, not that a caller receives the same answer. +// The pipeline was the leak. With the constraint check running first, a caller +// scoped to sub-1 and permitted only in eastus got two different 403s for a +// reservation they do not own: +// +// - an id that exists in eastus but is billed to sub-2 -- exchangeRegions +// yields ["eastus"], the constraint check passes, and the ownership gate +// refuses with "sources[0].reservation_id is not a reservation billed +// to..."; +// - an id that does not exist (or lives in westeurope) -- the sentinel or +// the foreign region joins the set and the constraint check refuses first, +// with its own message. +// +// The difference confirms "this reservation id exists, in one of my permitted +// regions, in a subscription I am not scoped to" -- exactly the oracle the +// ownership gate's identical denials were written to deny. Checking ownership +// before the constraints closes it. +func TestExecuteAzureExchange_UnownedSourceDenialsAreIndistinguishable(t *testing.T) { + ctx := context.Background() + + // One tenant listing for both probes: a reservation that exists, sits in + // the caller's permitted region, and belongs to someone else. + tenantListing := []azurecompute.ExchangeableReservation{ + {ReservationID: "res-eastus-other", BillingScopeID: "/subscriptions/sub-2", Region: "eastus", Quantity: 1}, + } + bodyFor := func(reservationID string) string { + return fmt.Sprintf(`{ + "subscription_id": "sub-1", + "sources": [{"reservation_id": %q, "quantity": 1}], + "targets": [{"sku": "Standard_D4s_v3", "location": "eastus", "term": "P1Y", "quantity": 1}], + "max_payment_due": "100.00", + "currency": "USD" + }`, reservationID) + } + + probe := func(reservationID string) error { + var captured []auth.PermissionConstraints + opsClient := new(mockAzureExchangeOpsClient) + opsClient.On("ListExchangeableReservations", mock.Anything).Return(tenantListing, nil) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := newAzureRegionScopedHandler(t, ctx, opsClient, "eastus", &captured) + _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: bodyFor(reservationID), + }) + opsClient.AssertNotCalled(t, "CalculateExchange", mock.Anything, mock.Anything, mock.Anything) + opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) + return err + } + + // Exists, in a permitted region, owned by another subscription. + existsElsewhere := probe("res-eastus-other") + // Does not exist anywhere in the tenant listing. + doesNotExist := probe("res-nowhere") + + require.Error(t, existsElsewhere) + require.Error(t, doesNotExist) + + existsCE, ok := IsClientError(existsElsewhere) + require.True(t, ok) + missingCE, ok := IsClientError(doesNotExist) + require.True(t, ok) + + assert.Equal(t, existsCE.code, missingCE.code, + "a caller must not be able to tell a real foreign reservation from a nonexistent one by status code") + assert.Equal(t, existsElsewhere.Error(), doesNotExist.Error(), + "...nor by message: differing denials confirm the id exists in a subscription the caller is not scoped to") +} + +// TestExchangeRegions covers the fold itself, including the branches that are +// awkward to drive through the whole handler. +func TestExchangeRegions(t *testing.T) { + owned := []azurecompute.ExchangeableReservation{ + {ReservationID: "res-east", Region: "eastus"}, + {ReservationID: "res-west", Region: "WestEurope"}, + {ReservationID: "res-blank", Region: " "}, + {ReservationID: "res-none"}, + } + target := func(location string) AzureExchangeTargetBody { + return AzureExchangeTargetBody{Location: location} + } + source := func(id string) AzureExchangeSourceBody { + return AzureExchangeSourceBody{ReservationID: id, Quantity: 1} + } + + tests := []struct { + name string + targets []AzureExchangeTargetBody + sources []AzureExchangeSourceBody + want []string + }{ + { + name: "a source region outside the targets is added to the set", + targets: []AzureExchangeTargetBody{target("eastus")}, + sources: []AzureExchangeSourceBody{source("res-west")}, + want: []string{"eastus", "westeurope"}, + }, + { + name: "a same-region exchange collapses to one entry", + targets: []AzureExchangeTargetBody{target("EastUS")}, + sources: []AzureExchangeSourceBody{source("res-east")}, + want: []string{"eastus"}, + }, + { + name: "source regions are matched case-insensitively by reservation id", + targets: []AzureExchangeTargetBody{target("eastus")}, + sources: []AzureExchangeSourceBody{source("RES-WEST")}, + want: []string{"eastus", "westeurope"}, + }, + { + name: "a blank source region yields the sentinel, never nothing", + targets: []AzureExchangeTargetBody{target("eastus")}, + sources: []AzureExchangeSourceBody{source("res-blank")}, + want: []string{"eastus", unknownRegionConstraint}, + }, + { + name: "a source Azure reported without a region yields the sentinel", + targets: []AzureExchangeTargetBody{target("eastus")}, + sources: []AzureExchangeSourceBody{source("res-none")}, + want: []string{"eastus", unknownRegionConstraint}, + }, + { + name: "a source absent from the tenant listing yields the sentinel", + targets: []AzureExchangeTargetBody{target("eastus")}, + sources: []AzureExchangeSourceBody{source("res-unheard-of")}, + want: []string{"eastus", unknownRegionConstraint}, + }, + { + name: "several unknown sources collapse into a single sentinel entry", + targets: []AzureExchangeTargetBody{target("eastus")}, + sources: []AzureExchangeSourceBody{source("res-none"), source("res-blank")}, + want: []string{"eastus", unknownRegionConstraint}, + }, + { + name: "every distinct region on both sides survives", + targets: []AzureExchangeTargetBody{target("eastus"), target("northeurope")}, + sources: []AzureExchangeSourceBody{source("res-west"), source("res-east")}, + want: []string{"eastus", "northeurope", "westeurope"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, exchangeRegions(tt.targets, tt.sources, owned)) + }) + } +} diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index db29a4efd..c272eb5d2 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -704,7 +704,10 @@ var _ = config.RIExchangeRecord{} // --- Azure exchangeable RI tests --- // stubAzureExchangeClient is a minimal implementation of azureExchangeClient -// for unit tests. +// for unit tests. It only exercises ListExchangeableReservations; the +// CalculateExchange/ExecuteExchange methods exist solely to satisfy the +// widened interface and are not used by the listing tests below (the +// dedicated mockAzureExchangeOpsClient in this file covers those). type stubAzureExchangeClient struct { err error reservations []azurecompute.ExchangeableReservation @@ -714,6 +717,14 @@ func (s *stubAzureExchangeClient) ListExchangeableReservations(_ context.Context return s.reservations, s.err } +func (s *stubAzureExchangeClient) CalculateExchange(_ context.Context, _ []azurecompute.ExchangeableReservation, _ []azurecompute.ExchangeTarget) (*azurecompute.ExchangePreview, []azurecompute.CompatibleOffering, error) { + return nil, nil, fmt.Errorf("stubAzureExchangeClient.CalculateExchange not implemented") +} + +func (s *stubAzureExchangeClient) ExecuteExchange(_ context.Context, _ string) (*azurecompute.ExchangeResult, error) { + return nil, fmt.Errorf("stubAzureExchangeClient.ExecuteExchange not implemented") +} + func TestListExchangeableAzureRIs_RequiresPermission(t *testing.T) { h := &Handler{} // no auth configured _, err := h.listExchangeableAzureRIs(context.Background(), &events.LambdaFunctionURLRequest{}) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 04a9bfcee..ce638b42f 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -699,6 +699,130 @@ paths: '403': $ref: '#/components/responses/Forbidden' + /api/ri-exchange/azure-instances/compatible-offerings: + post: + operationId: getAzureCompatibleOfferings + tags: [RIExchange] + summary: Price a proposed Azure RI exchange and list compatible offerings + description: > + Requires `view:purchases` permission and `allowed_accounts` scope + over the cloud account registered for `subscription_id`. Runs Azure + CalculateExchange against the given source reservations and target + SKUs and returns the priced candidate offerings plus a cost preview, + without committing any financial transaction. + + Each target's billing scope is always derived from `subscription_id`. + A target may omit `billing_scope_id`; supplying one that names a + different subscription is a 400. + parameters: + - $ref: '#/components/parameters/CSRFToken' + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [subscription_id, sources, targets] + properties: + subscription_id: + type: string + sources: + type: array + minItems: 1 + maxItems: 50 + items: + $ref: '#/components/schemas/AzureExchangeSource' + targets: + type: array + minItems: 1 + maxItems: 50 + items: + $ref: '#/components/schemas/AzureExchangeTarget' + responses: + '200': + description: Compatible offerings and cost preview + content: + application/json: + schema: + type: object + '400': + $ref: '#/components/responses/BadRequest' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + '404': + $ref: '#/components/responses/NotFound' + '422': + $ref: '#/components/responses/UnprocessableEntity' + + /api/ri-exchange/azure-instances/exchange: + post: + operationId: executeAzureExchange + tags: [RIExchange] + summary: Execute an Azure RI exchange (irreversible) + description: > + Requires `execute:ri-exchange` permission (deliberately separate from + `execute:purchases`, mirroring the AWS execute endpoint) plus + `allowed_accounts` scope over the cloud account registered for + `subscription_id`. Each target's billing scope is derived from + `subscription_id`, so the charge always lands on the account whose + permission constraints were checked; a `billing_scope_id` naming a + different subscription is a 400. The server + re-runs CalculateExchange itself against the given sources/targets + and only executes the fresh session that call returns -- a + client-supplied session is never trusted. Execution is refused when + the fresh quote carries policy errors, omits a net payable amount, + is denominated in a different currency than requested, or exceeds + `max_payment_due`. + parameters: + - $ref: '#/components/parameters/CSRFToken' + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [subscription_id, sources, targets, max_payment_due, currency] + properties: + subscription_id: + type: string + sources: + type: array + minItems: 1 + maxItems: 50 + items: + $ref: '#/components/schemas/AzureExchangeSource' + targets: + type: array + minItems: 1 + maxItems: 50 + items: + $ref: '#/components/schemas/AzureExchangeTarget' + max_payment_due: + type: string + description: Spend-cap guardrail (decimal string, e.g. "1000.00") + currency: + type: string + description: ISO 4217 currency code the cap is denominated in + responses: + '200': + description: Exchange executed + content: + application/json: + schema: + type: object + '400': + $ref: '#/components/responses/BadRequest' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + '404': + $ref: '#/components/responses/NotFound' + '422': + $ref: '#/components/responses/UnprocessableEntity' + /api/ri-exchange/target-offerings: get: operationId: listTargetOfferings @@ -1728,6 +1852,12 @@ components: application/json: schema: $ref: '#/components/schemas/Error' + UnprocessableEntity: + description: Request understood but rejected by a business-rule guardrail + content: + application/json: + schema: + $ref: '#/components/schemas/Error' RateLimited: description: Rate limit exceeded content: @@ -1737,6 +1867,60 @@ components: # ---- Schemas ------------------------------------------------------------ schemas: + # -- Azure RI exchange -------------------------------------------------- + AzureExchangeSource: + type: object + description: One Azure VM reservation to hand back in an exchange. + required: [reservation_id, quantity] + properties: + reservation_id: + type: string + description: > + Full ARM resource id of the reservation item, as returned by + GET /api/ri-exchange/azure-instances, e.g. + /providers/Microsoft.Capacity/reservationOrders/{orderId}/reservations/{reservationId} + quantity: + type: integer + format: int32 + minimum: 1 + description: Number of instances to hand back. + + AzureExchangeTarget: + type: object + description: One Azure VM reservation to acquire in an exchange. + required: [sku, location, term, quantity] + properties: + sku: + type: string + description: VM size to purchase, e.g. Standard_D4s_v3. + location: + type: string + description: Azure region, e.g. eastus. + term: + type: string + description: > + Reservation term, validated against the Azure SDK's own term + enum; any other value is a 400. Which of these Azure actually + accepts depends on the resource type (VM reservations are sold + for one and three years), so a term Azure does not offer for + this SKU comes back as a policy error from the pricing call + rather than being rejected here. + enum: [P1Y, P3Y, P5Y] + quantity: + type: integer + format: int32 + minimum: 1 + description: Number of instances to reserve. + billing_scope_id: + type: string + description: > + Optional, and NOT the scope that gets charged: the billing scope + is always derived from subscription_id. Supplying a value that + names a different subscription is a 400, so a caller cannot + direct the charge away from the subscription their permission + constraints were evaluated against. Omit it to charge the + subscription's own scope. + # -- Generic ------------------------------------------------------------ Error: type: object diff --git a/internal/api/router.go b/internal/api/router.go index d5400ade3..11d0d6a85 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -301,6 +301,8 @@ func (r *Router) registerRoutes() { // each handler calls requirePermission so the per-handler check is // the real gate. approve/reject stay AuthPublic (token-based). {ExactPath: "/api/ri-exchange/azure-instances", Method: "GET", Handler: r.listExchangeableAzureRIsHandler, Auth: AuthUser}, + {ExactPath: "/api/ri-exchange/azure-instances/compatible-offerings", Method: "POST", Handler: r.getAzureCompatibleOfferingsHandler, Auth: AuthUser}, + {ExactPath: "/api/ri-exchange/azure-instances/exchange", Method: "POST", Handler: r.executeAzureExchangeHandler, Auth: AuthUser}, {ExactPath: "/api/ri-exchange/instances", Method: "GET", Handler: r.listConvertibleRIsHandler, Auth: AuthUser}, {ExactPath: "/api/ri-exchange/target-offerings", Method: "GET", Handler: r.listTargetOfferingsHandler, Auth: AuthUser}, {ExactPath: "/api/ri-exchange/utilization", Method: "GET", Handler: r.getRIUtilizationHandler, Auth: AuthUser}, @@ -818,6 +820,14 @@ func (r *Router) listExchangeableAzureRIsHandler(ctx context.Context, req *event return r.h.listExchangeableAzureRIs(ctx, req) } +func (r *Router) getAzureCompatibleOfferingsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.getAzureCompatibleOfferings(ctx, req) +} + +func (r *Router) executeAzureExchangeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.executeAzureExchange(ctx, req) +} + func (r *Router) listConvertibleRIsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.listConvertibleRIs(ctx, req) } diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 6930e5ea6..ce23b49ff 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "time" "github.com/google/uuid" @@ -241,7 +242,7 @@ func (s *Service) matchConstraints(permConstraints, reqConstraints *PermissionCo return s.matchStringListConstraints(permConstraints.AccountIDs, reqConstraints.AccountIDs) && s.matchStringListConstraints(permConstraints.Providers, reqConstraints.Providers) && s.matchStringListConstraints(permConstraints.Services, reqConstraints.Services) && - s.matchStringListConstraints(permConstraints.Regions, reqConstraints.Regions) && + s.matchAllRegionsConstraint(permConstraints.Regions, reqConstraints.Regions) && s.matchPurchaseAmountConstraint(permConstraints.MaxPurchaseAmount, reqConstraints.MaxPurchaseAmount) } @@ -265,6 +266,43 @@ func (s *Service) matchStringListConstraints(permList, reqList []string) bool { return true } +// matchAllRegionsConstraint is the Regions dimension's matcher: EVERY +// requested region must be permitted, not merely one of them. +// +// Regions is the one dimension where a single request legitimately spans +// several values: an Azure RI exchange takes a list of targets, and +// handler_ri_exchange.go's targetLocations feeds all of their locations in +// at once. Under the generic containsAny rule that made the region +// constraint bypassable -- a caller permitted only in eastus could attach a +// westus target, containsAny would find eastus in the permitted set, return +// true, and the exchange would execute for BOTH regions. "Regions limits to +// specific regions" (types.go) cannot mean "limits to requests that mention +// at least one permitted region". +// +// Every other dimension keeps containsAny: a request names one provider, +// one service, one account, so for them any-match and all-match coincide. +// +// Comparison is case-insensitive so a permission stored as "EastUS" still +// matches the canonical lower-case form callers are normalized to; the +// empty-list semantics are unchanged from matchStringListConstraints (an +// unconstrained permission, or a request that does not name a region, still +// matches). +func (s *Service) matchAllRegionsConstraint(permRegions, reqRegions []string) bool { + if len(permRegions) == 0 || len(reqRegions) == 0 { + return true + } + permitted := make(map[string]bool, len(permRegions)) + for _, r := range permRegions { + permitted[strings.ToLower(strings.TrimSpace(r))] = true + } + for _, r := range reqRegions { + if !permitted[strings.ToLower(strings.TrimSpace(r))] { + return false + } + } + return true +} + // matchPurchaseAmountConstraint checks if requested amount is within permitted limit. func (s *Service) matchPurchaseAmountConstraint(permMax, reqMax float64) bool { if permMax > 0 && reqMax > permMax { diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index 374bfc0f0..cbc3cf3d2 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -1429,3 +1429,142 @@ func TestMatchConstraints(t *testing.T) { assert.False(t, service.matchConstraints(permConstraints, reqConstraints)) }) } + +// TestMatchConstraints_RegionsRequireEveryRequestedRegion pins the Regions +// dimension's ALL-match rule. +// +// Regions is the only dimension a single request legitimately spans several +// values on: an Azure RI exchange submits every target location at once +// (api.targetLocations). Under the generic containsAny rule the other +// dimensions use, a caller permitted only in eastus could attach a westus +// target -- containsAny finds eastus in the permitted set, returns true, and +// the irreversible exchange executes in BOTH regions. Every case below is +// asserted through matchConstraints, not matchAllRegionsConstraint directly, +// so re-wiring the Regions dimension back to matchStringListConstraints +// fails these tests rather than leaving them vacuously green. +func TestMatchConstraints_RegionsRequireEveryRequestedRegion(t *testing.T) { + service := &Service{} + + tests := []struct { + name string + permRegions []string + reqRegions []string + want bool + }{ + { + name: "partially permitted multi-region request is denied", + permRegions: []string{"eastus"}, + reqRegions: []string{"eastus", "westus"}, + want: false, + }, + { + name: "every requested region permitted", + permRegions: []string{"eastus", "westus", "westeurope"}, + reqRegions: []string{"eastus", "westus"}, + want: true, + }, + { + name: "single permitted region", + permRegions: []string{"eastus", "westus"}, + reqRegions: []string{"westus"}, + want: true, + }, + { + name: "single unpermitted region is denied", + permRegions: []string{"eastus"}, + reqRegions: []string{"westus"}, + want: false, + }, + { + name: "permission casing does not matter", + permRegions: []string{"EastUS"}, + reqRegions: []string{"eastus"}, + want: true, + }, + { + name: "request casing does not matter", + permRegions: []string{"eastus"}, + reqRegions: []string{"EastUS"}, + want: true, + }, + { + name: "surrounding whitespace does not matter", + permRegions: []string{" eastus "}, + reqRegions: []string{"eastus"}, + want: true, + }, + { + name: "casing does not smuggle in an unpermitted region", + permRegions: []string{"EastUS"}, + reqRegions: []string{"eastus", "WestUS"}, + want: false, + }, + { + name: "unconstrained permission matches any request", + permRegions: nil, + reqRegions: []string{"eastus", "westus"}, + want: true, + }, + { + name: "request naming no region matches a constrained permission", + permRegions: []string{"eastus"}, + reqRegions: nil, + want: true, + }, + { + name: "AWS-style region names behave identically", + permRegions: []string{"us-east-1"}, + reqRegions: []string{"us-east-1", "eu-west-1"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := service.matchConstraints( + &PermissionConstraints{Regions: tt.permRegions}, + &PermissionConstraints{Regions: tt.reqRegions}, + ) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestHasPermission_RegionConstraintDeniesUnpermittedTargetRegion drives the +// same ALL-match rule through the real HasPermission entry point the API +// handlers call, so the fix is proven end to end and not just at the matcher. +// +// The scenario is the Azure RI exchange bypass: a group permitted to execute +// ri-exchange only in eastus, and a request whose target list spans eastus +// and westus. +func TestHasPermission_RegionConstraintDeniesUnpermittedTargetRegion(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + svc := createTestService(mockStore, mockEmail) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + user := &User{ID: "user-eastus", GroupIDs: []string{"group-eastus"}, Active: true} + group := &Group{ + ID: "group-eastus", + Name: "EastUS Exchangers", + Permissions: []Permission{{ + Action: ActionExecute, + Resource: ResourceRIExchange, + Constraints: &PermissionConstraints{Regions: []string{"eastus"}}, + }}, + } + mockStore.On("GetUserByID", ctx, "user-eastus").Return(user, nil) + mockStore.On("GetGroup", ctx, "group-eastus").Return(group, nil) + + has, err := svc.HasPermission(ctx, "user-eastus", ActionExecute, ResourceRIExchange, + &PermissionConstraints{Regions: []string{"eastus", "westus"}}) + require.NoError(t, err) + assert.False(t, has, + "a caller permitted only in eastus must not execute an exchange that also targets westus") + + has, err = svc.HasPermission(ctx, "user-eastus", ActionExecute, ResourceRIExchange, + &PermissionConstraints{Regions: []string{"eastus"}}) + require.NoError(t, err) + assert.True(t, has, "the permitted region alone must still be allowed") +} diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index fd447817b..6a8f27a4b 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -110,6 +110,14 @@ type ComputeClient struct { // armreservations.ReservationClient. Tests inject a stub to run // hermetically without Azure credentials. exchangeablePager ExchangeableReservationPager + + // Optional injected LRO callers for CalculateExchange and + // ExecuteExchange. When nil (the production default) the methods + // construct real armreservations SDK clients. Tests inject stubs via + // SetCalculateExchangeCaller and SetDoExchangeCaller to run + // hermetically and make the LRO synchronous. + calculateExchangeCaller CalculateExchangeCallerFunc + doExchangeCaller DoExchangeCallerFunc } // NewClient creates a new Azure Compute client diff --git a/providers/azure/services/compute/exchange.go b/providers/azure/services/compute/exchange.go index 18f2f9e37..a4e87586b 100644 --- a/providers/azure/services/compute/exchange.go +++ b/providers/azure/services/compute/exchange.go @@ -35,6 +35,23 @@ type ExchangeableReservation struct { // Used as the source identifier in CalculateExchange.ReservationsToExchange. ReservationID string `json:"reservation_id"` + // BillingScopeID is the ARM scope that paid for this reservation, e.g. + // "/subscriptions/{subscriptionID}". Azure documents the underlying + // field as "Subscription that will be charged for purchasing + // Reservation", so it identifies the owning subscription even for a + // reservation whose AppliedScopeType is Shared (Shared controls which + // subscriptions receive the DISCOUNT; exactly one scope is CHARGED). + // + // This is the only ownership signal available on a tenant-wide listing, + // and an exchange refunds each source reservation to its own billing + // scope. Callers authorizing an exchange MUST require this to match the + // subscription they authorized, or a caller scoped to one subscription + // can hand back another's commitments (issue #1527). + // + // Empty when Azure did not report one. Callers must treat that as + // "ownership unknown" and refuse, never as "no restriction". + BillingScopeID string `json:"billing_scope_id,omitempty"` + // SKU is the VM size (e.g. "Standard_D2s_v3"). SKU string `json:"sku"` @@ -45,7 +62,10 @@ type ExchangeableReservation struct { // reservations with AppliedScopeType == Shared. Region string `json:"region,omitempty"` - // Term is the reservation term in ISO 8601 duration format ("P1Y" or "P3Y"). + // Term is the reservation term in ISO 8601 duration format, stringified + // from armreservations.PossibleReservationTermValues() ("P1Y", "P3Y" or + // "P5Y"). Consumers must not treat a term outside the one/three-year + // pair as unsupported. Term string `json:"term,omitempty"` // ExpiryDate is when the reservation expires. Zero if not set by Azure. @@ -195,9 +215,18 @@ func convertToExchangeableReservation(item *armreservations.ReservationResponse) // in the inventory view, but callers MUST filter out empty-order-ID entries // before initiating an exchange operation -- the Azure exchange API requires a // non-empty reservationOrderId. + // Read directly rather than through extractReservationFields, which + // already returns the maximum number of positional results that stays + // readable. Absent stays "" so callers can distinguish "Azure did not + // report an owner" from any real scope. + var billingScopeID string + if item.Properties.BillingScopeID != nil { + billingScopeID = *item.Properties.BillingScopeID + } return &ExchangeableReservation{ ReservationOrderID: orderID, ReservationID: id, + BillingScopeID: billingScopeID, SKU: sku, Quantity: quantity, Region: region, diff --git a/providers/azure/services/compute/exchange_operations.go b/providers/azure/services/compute/exchange_operations.go new file mode 100644 index 000000000..1eaa1c0d5 --- /dev/null +++ b/providers/azure/services/compute/exchange_operations.go @@ -0,0 +1,560 @@ +// This file implements the "compatible offerings" and "execute exchange" +// steps of Azure Convertible RI exchange parity with AWS EC2 (refs #473, +// closes #596). +// +// Flow: +// 1. CalculateExchange -- calls armreservations.CalculateExchangeClient.BeginPost +// with the source reservations and caller-supplied target slots. Azure prices +// the proposed combination and returns a session ID, the candidate offerings +// it is willing to accept, and any policy errors -- without committing anything. +// 2. ExecuteExchange -- calls armreservations.ExchangeClient.BeginPost with the +// session ID from a CalculateExchange call, committing the swap. +// +// Both SDK operations are async LROs; PollUntilDone blocks until Azure completes +// or ctx is canceled. Context cancellation is treated as terminal and propagated +// immediately rather than folded into a generic error (feedback_ctx_cancel_terminal). +// +// Money-path note: this file only prices and executes exactly what it is told. +// The caller (internal/api handler) is responsible for never executing a +// session ID it did not just obtain from a CalculateExchange call made against +// the caller's own guardrail-checked inputs -- see the handler's doc comment +// for the full server-re-quote design. +package compute + +import ( + "context" + "errors" + "fmt" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" +) + +// CompatibleOffering describes one candidate target SKU that Azure priced as +// an exchange destination for the source reservations. +type CompatibleOffering struct { + // SKU is the VM size (e.g. "Standard_D4s_v3"). + SKU string `json:"sku"` + + // Location is the Azure region (e.g. "eastus"). + Location string `json:"location"` + + // Term is the reservation term Azure priced this offering at, in ISO + // 8601 duration format, stringified from + // armreservations.PossibleReservationTermValues() ("P1Y", "P3Y" or + // "P5Y"). Consumers must not treat a term outside the one/three-year + // pair as unsupported. + Term string `json:"term"` + + // Quantity is the number of instances that would be purchased. + Quantity int32 `json:"quantity"` + + // BillingCurrencyTotal is the net amount the customer would pay in their + // billing currency for this offering. Nil when Azure did not report an + // amount (never coerced to 0 -- absent is not the same as free). + BillingCurrencyTotal *float64 `json:"billing_currency_total"` + + // CurrencyCode is the ISO 4217 code for BillingCurrencyTotal (e.g. "USD"). + CurrencyCode string `json:"currency_code,omitempty"` +} + +// ExchangePreview holds the priced result of a CalculateExchange call: what +// the proposed exchange would cost if executed with this exact SessionID. +type ExchangePreview struct { + // SessionID must be passed verbatim to ExecuteExchange to commit this + // exact priced combination. It is single-use and has a short server-side + // TTL (typically 10 minutes). + SessionID string `json:"session_id"` + + // NetPayable is the net amount the customer would pay, in the billing + // currency. Positive: additional charge; negative: refund. Nil when + // Azure did not report an amount -- callers must refuse to execute + // rather than treat a nil NetPayable as "free" (feedback_nullable_not_zero). + NetPayable *float64 `json:"net_payable"` + + // NetPayableCurrency is the ISO 4217 code for NetPayable. + NetPayableCurrency string `json:"net_payable_currency,omitempty"` + + // RefundsTotal is the total refund value for the returned reservations. + RefundsTotal *float64 `json:"refunds_total"` + + // PurchasesTotal is the total cost of the acquired reservations. + PurchasesTotal *float64 `json:"purchases_total"` + + // PolicyErrors is non-empty when Azure's exchange policy blocks this + // combination (e.g. cross-billing-account, expired RIs). Each entry is + // a human-readable policy violation message, rendered by + // policyErrorMessage so that one entry here always corresponds to + // exactly one violation Azure reported. Callers must refuse to + // execute when this is non-empty. + PolicyErrors []string `json:"policy_errors,omitempty"` +} + +// ExchangeResult holds the outcome of a completed exchange. +type ExchangeResult struct { + // SessionID echoes the session identifier used for the exchange. + SessionID string `json:"session_id"` + + // NetPayable mirrors the final net payment amount. Nil when Azure did + // not report one. + NetPayable *float64 `json:"net_payable"` + + // NetPayableCurrency is the ISO 4217 code for NetPayable. + NetPayableCurrency string `json:"net_payable_currency,omitempty"` + + // Status is the typed ExchangeOperationResultStatus Azure returned + // (e.g. "Succeeded", "PendingPurchases"), stringified. + Status string `json:"status,omitempty"` +} + +// ExchangeTarget describes one reservation to acquire in an exchange. +type ExchangeTarget struct { + // SKU is the VM size to purchase (e.g. "Standard_D4s_v3"). Required. + SKU string + + // Location is the Azure region (e.g. "eastus"). Required. + Location string + + // Term is the reservation term. Required: must be one of + // armreservations.PossibleReservationTermValues(). There is no default + // -- an unset or unrecognized term is a validation error rather than a + // silent P1Y fallback. Which of those terms Azure actually sells for a + // given resource type is Azure's call, surfaced as a policy error from + // CalculateExchange rather than second-guessed here. + Term armreservations.ReservationTerm + + // Quantity is the number of instances to reserve. Required: must be >= 1. + Quantity int32 + + // BillingScopeID is the subscription or billing account that will be + // charged. Required by the Azure exchange API. + BillingScopeID string + + // AppliedScopeType controls whether the discount applies to a single + // subscription or all subscriptions ("Shared"). Optional: Azure's + // documented default of Shared is used when nil. + AppliedScopeType *armreservations.AppliedScopeType +} + +// CalculateExchangeCallerFunc is the narrow LRO-invoker interface that +// CalculateExchange needs from the SDK client. Satisfied by wrapping +// (*armreservations.CalculateExchangeClient).BeginPost + PollUntilDone; a +// stub can be injected for tests via SetCalculateExchangeCaller. +type CalculateExchangeCallerFunc func( + ctx context.Context, + body armreservations.CalculateExchangeRequest, +) (armreservations.CalculateExchangeOperationResultResponse, error) + +// DoExchangeCallerFunc is the narrow LRO-invoker interface for +// ExecuteExchange. Satisfied by wrapping +// (*armreservations.ExchangeClient).BeginPost + PollUntilDone; a stub can be +// injected via SetDoExchangeCaller. +type DoExchangeCallerFunc func( + ctx context.Context, + sessionID string, +) (armreservations.ExchangeOperationResultResponse, error) + +// SetCalculateExchangeCaller injects a test-only override for the +// CalculateExchange LRO. Tests use this to avoid real Azure API calls and to +// make the LRO synchronous (no time.Sleep / real polling needed). +func (c *ComputeClient) SetCalculateExchangeCaller(fn CalculateExchangeCallerFunc) { + c.calculateExchangeCaller = fn +} + +// SetDoExchangeCaller injects a test-only override for the Exchange LRO. +func (c *ComputeClient) SetDoExchangeCaller(fn DoExchangeCallerFunc) { + c.doExchangeCaller = fn +} + +// buildCalculateExchangeCaller returns the injected test stub when set, or +// constructs a real armreservations.CalculateExchangeClient wrapper. +func (c *ComputeClient) buildCalculateExchangeCaller() (CalculateExchangeCallerFunc, error) { + if c.calculateExchangeCaller != nil { + return c.calculateExchangeCaller, nil + } + client, err := armreservations.NewCalculateExchangeClient(c.cred, nil) + if err != nil { + return nil, fmt.Errorf("azure: create CalculateExchange client: %w", err) + } + return func(ctx context.Context, body armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + poller, err := client.BeginPost(ctx, body, nil) + if err != nil { + return armreservations.CalculateExchangeOperationResultResponse{}, fmt.Errorf("azure: CalculateExchange begin: %w", err) + } + resp, err := poller.PollUntilDone(ctx, nil) + if err != nil { + return armreservations.CalculateExchangeOperationResultResponse{}, fmt.Errorf("azure: CalculateExchange poll: %w", err) + } + return resp.CalculateExchangeOperationResultResponse, nil + }, nil +} + +// buildDoExchangeCaller returns the injected test stub when set, or +// constructs a real armreservations.ExchangeClient wrapper. +func (c *ComputeClient) buildDoExchangeCaller() (DoExchangeCallerFunc, error) { + if c.doExchangeCaller != nil { + return c.doExchangeCaller, nil + } + client, err := armreservations.NewExchangeClient(c.cred, nil) + if err != nil { + return nil, fmt.Errorf("azure: create Exchange client: %w", err) + } + return func(ctx context.Context, sessionID string) (armreservations.ExchangeOperationResultResponse, error) { + poller, err := client.BeginPost(ctx, armreservations.ExchangeRequest{ + Properties: &armreservations.ExchangeRequestProperties{ + SessionID: to.Ptr(sessionID), + }, + }, nil) + if err != nil { + return armreservations.ExchangeOperationResultResponse{}, fmt.Errorf("azure: Exchange begin: %w", err) + } + resp, err := poller.PollUntilDone(ctx, nil) + if err != nil { + return armreservations.ExchangeOperationResultResponse{}, fmt.Errorf("azure: Exchange poll: %w", err) + } + return resp.ExchangeOperationResultResponse, nil + }, nil +} + +// CalculateExchange prices a proposed exchange of sources for targets without +// committing it. Returns the priced preview (including the SessionID needed +// to execute) and the per-target compatible-offering breakdown. +// +// Every source must have a non-empty ReservationID and Quantity >= 1; every +// target must have a non-empty SKU/Location/BillingScopeID, Quantity >= 1, +// and a Term from PossibleReservationTermValues(). There is no coercion of +// invalid values (no clamping quantity to 1, no defaulting an unrecognized +// term) -- a caller mistake here is a validation error, not a silently +// different exchange than the one requested. +// +// Returns an error only when validation or the API call itself fails; a +// priced-but-policy-rejected combination is a successful call whose +// ExchangePreview.PolicyErrors is non-empty -- callers must check that +// before treating the preview as executable. +func (c *ComputeClient) CalculateExchange( + ctx context.Context, + sources []ExchangeableReservation, + targets []ExchangeTarget, +) (*ExchangePreview, []CompatibleOffering, error) { + if err := validateExchangeSources(sources); err != nil { + return nil, nil, err + } + if err := validateExchangeTargets(targets); err != nil { + return nil, nil, err + } + + caller, err := c.buildCalculateExchangeCaller() + if err != nil { + return nil, nil, err + } + + result, err := caller(ctx, buildCalculateExchangeRequest(sources, targets)) + if err != nil { + if isTerminalCtxErr(err) { + return nil, nil, err + } + return nil, nil, fmt.Errorf("azure: CalculateExchange: %w", err) + } + + props, err := checkCalculateExchangeResult(result) + if err != nil { + return nil, nil, err + } + return extractExchangePreview(props), extractCompatibleOfferings(props), nil +} + +// isTerminalCtxErr reports whether err is a context cancellation or deadline +// expiry, which callers must propagate as-is rather than fold into a +// generic wrapped error (feedback_ctx_cancel_terminal). +func isTerminalCtxErr(err error) bool { + return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) +} + +// checkCalculateExchangeResult validates that the raw LRO result represents +// a genuinely priced exchange -- no operation-level failure, a terminal +// status of Succeeded, and a non-empty SessionID actually present -- before +// the caller extracts a preview from it. A nil-Properties or +// empty-SessionID response is an explicit error rather than a fabricated +// empty preview. +// +// The Status check is not redundant with the Error check. Azure's contract +// documents Error as "required if status == failed or status == canceled", +// but a response that violates that contract (Failed/Cancelled with a nil +// Error) would otherwise yield a preview the execute handler immediately +// commits. Status is only asserted when Azure populated it: an absent +// status leaves the SessionID check as the guard, rather than inventing a +// failure Azure never reported. +func checkCalculateExchangeResult(result armreservations.CalculateExchangeOperationResultResponse) (armreservations.CalculateExchangeResponseProperties, error) { + if result.Error != nil { + return armreservations.CalculateExchangeResponseProperties{}, fmt.Errorf("azure: CalculateExchange operation failed: %s", operationErrorMessage(result.Error)) + } + if result.Status != nil && *result.Status != armreservations.CalculateExchangeOperationResultStatusSucceeded { + return armreservations.CalculateExchangeResponseProperties{}, fmt.Errorf("azure: CalculateExchange did not succeed: terminal status %q", string(*result.Status)) + } + if result.Properties == nil || result.Properties.SessionID == nil || *result.Properties.SessionID == "" { + return armreservations.CalculateExchangeResponseProperties{}, fmt.Errorf("azure: CalculateExchange returned no session id") + } + return *result.Properties, nil +} + +// ExecuteExchange commits a previously-calculated Azure RI exchange using the +// session ID returned by CalculateExchange. +// +// sessionID must be non-empty. Azure's CalculateExchange session ID is the +// idempotency mechanism for this call: replaying the same session ID after +// the exchange completes has no further effect server-side. +func (c *ComputeClient) ExecuteExchange(ctx context.Context, sessionID string) (*ExchangeResult, error) { + if sessionID == "" { + return nil, fmt.Errorf("azure: ExecuteExchange: session_id is required (obtain from CalculateExchange)") + } + + caller, err := c.buildDoExchangeCaller() + if err != nil { + return nil, err + } + + result, err := caller(ctx, sessionID) + if err != nil { + if isTerminalCtxErr(err) { + return nil, err + } + return nil, fmt.Errorf("azure: ExecuteExchange: %w", err) + } + if result.Error != nil { + return nil, fmt.Errorf("azure: ExecuteExchange operation failed: %s", operationErrorMessage(result.Error)) + } + if result.Status != nil && !exchangeStatusAccepted(*result.Status) { + return nil, fmt.Errorf( + "azure: ExecuteExchange returned terminal status %q with no error detail; verify the reservation state in the Azure portal before retrying", + string(*result.Status)) + } + if result.Properties == nil { + return nil, fmt.Errorf("azure: ExecuteExchange returned no properties") + } + + netPayable, netPayableCurrency := extractPrice(result.Properties.NetPayable) + res := &ExchangeResult{SessionID: sessionID, NetPayable: netPayable, NetPayableCurrency: netPayableCurrency} + if result.Status != nil { + res.Status = string(*result.Status) + } + return res, nil +} + +// --- internal helpers --- + +// exchangeStatusAccepted reports whether an ExchangeOperationResultStatus +// means Azure accepted and is carrying out the exchange. Succeeded is fully +// settled; PendingRefunds/PendingPurchases mean the swap was committed and +// Azure is still settling one leg, which the caller surfaces as-is. +// +// Everything else -- Failed, Cancelled, and any status a future API version +// adds that this SDK does not know -- is refused rather than reported as a +// successful exchange. An unrecognized status after a commit attempt is +// genuinely ambiguous, so the error tells the operator to check the portal +// instead of blindly retrying into a possible double exchange. +func exchangeStatusAccepted(s armreservations.ExchangeOperationResultStatus) bool { + switch s { + case armreservations.ExchangeOperationResultStatusSucceeded, + armreservations.ExchangeOperationResultStatusPendingPurchases, + armreservations.ExchangeOperationResultStatusPendingRefunds: + return true + default: + return false + } +} + +// operationErrorMessage extracts a human-readable message from an Azure LRO +// error result, falling back to a generic label when Azure omits the message +// field (still an explicit error, never silently swallowed). +func operationErrorMessage(opErr *armreservations.OperationResultError) string { + if opErr != nil && opErr.Message != nil { + return *opErr.Message + } + return "no error message returned" +} + +// validateExchangeSources fails loud on any source that would otherwise be +// silently coerced into something Azure did not actually ask to exchange. +func validateExchangeSources(sources []ExchangeableReservation) error { + if len(sources) == 0 { + return fmt.Errorf("azure: CalculateExchange: at least one source reservation is required") + } + for i := range sources { + s := &sources[i] + if s.ReservationID == "" { + return fmt.Errorf("azure: CalculateExchange: sources[%d].reservation_id is required", i) + } + if s.Quantity < 1 { + return fmt.Errorf("azure: CalculateExchange: sources[%d].quantity must be >= 1, got %d", i, s.Quantity) + } + } + return nil +} + +// validateExchangeTargets fails loud on any target field that would +// otherwise be silently coerced or defaulted to a different commitment +// than what the caller asked for. +func validateExchangeTargets(targets []ExchangeTarget) error { + if len(targets) == 0 { + return fmt.Errorf("azure: CalculateExchange: at least one target is required") + } + for i, t := range targets { + if t.SKU == "" { + return fmt.Errorf("azure: CalculateExchange: targets[%d].sku is required", i) + } + if t.Location == "" { + return fmt.Errorf("azure: CalculateExchange: targets[%d].location is required", i) + } + if t.BillingScopeID == "" { + return fmt.Errorf("azure: CalculateExchange: targets[%d].billing_scope_id is required", i) + } + if t.Quantity < 1 { + return fmt.Errorf("azure: CalculateExchange: targets[%d].quantity must be >= 1, got %d", i, t.Quantity) + } + if !isValidReservationTerm(t.Term) { + return fmt.Errorf("azure: CalculateExchange: targets[%d].term %q is not a supported reservation term", i, t.Term) + } + } + return nil +} + +// isValidReservationTerm reports whether term is one of the SDK's typed +// enum values, rather than accepting any string the caller happens to pass. +func isValidReservationTerm(term armreservations.ReservationTerm) bool { + for _, t := range armreservations.PossibleReservationTermValues() { + if t == term { + return true + } + } + return false +} + +// buildCalculateExchangeRequest converts validated sources/targets into the +// SDK request shape, using typed SDK enum constants throughout +// (feedback_sdk_enum_string_literals) rather than raw strings. +func buildCalculateExchangeRequest(sources []ExchangeableReservation, targets []ExchangeTarget) armreservations.CalculateExchangeRequest { + toReturn := make([]*armreservations.ReservationToReturn, 0, len(sources)) + for i := range sources { + src := sources[i] + toReturn = append(toReturn, &armreservations.ReservationToReturn{ + Quantity: to.Ptr(src.Quantity), + ReservationID: to.Ptr(src.ReservationID), + }) + } + + toPurchase := make([]*armreservations.PurchaseRequest, 0, len(targets)) + for i := range targets { + tgt := targets[i] + scopeType := armreservations.AppliedScopeTypeShared + if tgt.AppliedScopeType != nil { + scopeType = *tgt.AppliedScopeType + } + toPurchase = append(toPurchase, &armreservations.PurchaseRequest{ + Location: to.Ptr(tgt.Location), + SKU: &armreservations.SKUName{Name: to.Ptr(tgt.SKU)}, + Properties: &armreservations.PurchaseRequestProperties{ + AppliedScopeType: to.Ptr(scopeType), + BillingPlan: to.Ptr(armreservations.ReservationBillingPlanUpfront), + BillingScopeID: to.Ptr(tgt.BillingScopeID), + Quantity: to.Ptr(tgt.Quantity), + Renew: to.Ptr(false), + ReservedResourceType: to.Ptr(armreservations.ReservedResourceTypeVirtualMachines), + Term: to.Ptr(tgt.Term), + ReservedResourceProperties: &armreservations.PurchaseRequestPropertiesReservedResourceProperties{ + InstanceFlexibility: to.Ptr(armreservations.InstanceFlexibilityOn), + }, + }, + }) + } + + return armreservations.CalculateExchangeRequest{ + Properties: &armreservations.CalculateExchangeRequestProperties{ + ReservationsToExchange: toReturn, + ReservationsToPurchase: toPurchase, + }, + } +} + +// extractPrice reads the optional Amount/CurrencyCode pointer fields from an +// armreservations.Price, returning a nil amount (never a fabricated 0) when +// Azure did not report one. +func extractPrice(p *armreservations.Price) (amount *float64, currency string) { + if p == nil { + return nil, "" + } + if p.Amount != nil { + v := *p.Amount + amount = &v + } + if p.CurrencyCode != nil { + currency = *p.CurrencyCode + } + return amount, currency +} + +func extractExchangePreview(props armreservations.CalculateExchangeResponseProperties) *ExchangePreview { + preview := &ExchangePreview{SessionID: *props.SessionID} + preview.NetPayable, preview.NetPayableCurrency = extractPrice(props.NetPayable) + preview.RefundsTotal, _ = extractPrice(props.RefundsTotal) + preview.PurchasesTotal, _ = extractPrice(props.PurchasesTotal) + if props.PolicyResult != nil { + for _, e := range props.PolicyResult.PolicyErrors { + preview.PolicyErrors = append(preview.PolicyErrors, policyErrorMessage(e)) + } + } + return preview +} + +// policyErrorMessage renders one Azure exchange policy violation as a +// non-empty string. +// +// Both fields of armreservations.ExchangePolicyError are optional pointers, +// so an entry may carry only a Code, or (in a contract violation) neither. +// Every entry must still produce a message: callers gate execution on +// len(ExchangePreview.PolicyErrors) > 0, so dropping a Message-less entry +// would empty the slice and let a policy-rejected exchange be committed. +func policyErrorMessage(e *armreservations.ExchangePolicyError) string { + if e == nil { + return "azure reported an unspecified exchange policy violation" + } + switch { + case e.Message != nil && *e.Message != "" && e.Code != nil && *e.Code != "": + return fmt.Sprintf("%s: %s", *e.Code, *e.Message) + case e.Message != nil && *e.Message != "": + return *e.Message + case e.Code != nil && *e.Code != "": + return *e.Code + default: + return "azure reported an unspecified exchange policy violation" + } +} + +func extractCompatibleOfferings(props armreservations.CalculateExchangeResponseProperties) []CompatibleOffering { + out := make([]CompatibleOffering, 0, len(props.ReservationsToPurchase)) + for _, item := range props.ReservationsToPurchase { + if item == nil { + continue + } + o := CompatibleOffering{} + if item.Properties != nil { + pp := item.Properties + if pp.Location != nil { + o.Location = *pp.Location + } + if pp.SKU != nil && pp.SKU.Name != nil { + o.SKU = *pp.SKU.Name + } + if pp.Properties != nil { + if pp.Properties.Quantity != nil { + o.Quantity = *pp.Properties.Quantity + } + if pp.Properties.Term != nil { + o.Term = string(*pp.Properties.Term) + } + } + } + o.BillingCurrencyTotal, o.CurrencyCode = extractPrice(item.BillingCurrencyTotal) + out = append(out, o) + } + return out +} diff --git a/providers/azure/services/compute/exchange_operations_test.go b/providers/azure/services/compute/exchange_operations_test.go new file mode 100644 index 000000000..22935544c --- /dev/null +++ b/providers/azure/services/compute/exchange_operations_test.go @@ -0,0 +1,537 @@ +package compute_test + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/CUDly/providers/azure/services/compute" +) + +func validSource() compute.ExchangeableReservation { + return compute.ExchangeableReservation{ + ReservationID: "/providers/Microsoft.Capacity/reservationOrders/order-1/reservations/res-1", + Quantity: 2, + } +} + +func validTarget() compute.ExchangeTarget { + return compute.ExchangeTarget{ + SKU: "Standard_D4s_v3", + Location: "eastus", + Term: armreservations.ReservationTermP1Y, + Quantity: 1, + BillingScopeID: "/subscriptions/sub-1", + } +} + +func succeededResult(props *armreservations.CalculateExchangeResponseProperties) armreservations.CalculateExchangeOperationResultResponse { + return armreservations.CalculateExchangeOperationResultResponse{ + Status: to.Ptr(armreservations.CalculateExchangeOperationResultStatusSucceeded), + Properties: props, + } +} + +// --- CalculateExchange validation --- + +func TestCalculateExchange_ValidationSources(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + callerInvoked := false + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + callerInvoked = true + return armreservations.CalculateExchangeOperationResultResponse{}, nil + }) + + tests := []struct { + name string + sources []compute.ExchangeableReservation + wantErr string + }{ + {"no sources", nil, "at least one source reservation is required"}, + {"empty reservation id", []compute.ExchangeableReservation{{ReservationID: "", Quantity: 1}}, "sources[0].reservation_id is required"}, + {"zero quantity", []compute.ExchangeableReservation{{ReservationID: "res-1", Quantity: 0}}, "sources[0].quantity must be >= 1"}, + {"negative quantity", []compute.ExchangeableReservation{{ReservationID: "res-1", Quantity: -1}}, "sources[0].quantity must be >= 1"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + preview, offerings, err := c.CalculateExchange(context.Background(), tt.sources, []compute.ExchangeTarget{validTarget()}) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Nil(t, preview) + assert.Nil(t, offerings) + assert.False(t, callerInvoked, "caller must not be invoked when source validation fails") + }) + } +} + +func TestCalculateExchange_ValidationTargets(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + callerInvoked := false + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + callerInvoked = true + return armreservations.CalculateExchangeOperationResultResponse{}, nil + }) + + base := validTarget() + withSKU := base + withSKU.SKU = "" + withLocation := base + withLocation.Location = "" + withScope := base + withScope.BillingScopeID = "" + withQty := base + withQty.Quantity = 0 + withTerm := base + withTerm.Term = armreservations.ReservationTerm("P2Y") + + tests := []struct { + name string + targets []compute.ExchangeTarget + wantErr string + }{ + {"no targets", nil, "at least one target is required"}, + {"missing sku", []compute.ExchangeTarget{withSKU}, "targets[0].sku is required"}, + {"missing location", []compute.ExchangeTarget{withLocation}, "targets[0].location is required"}, + {"missing billing scope", []compute.ExchangeTarget{withScope}, "targets[0].billing_scope_id is required"}, + {"zero quantity", []compute.ExchangeTarget{withQty}, "targets[0].quantity must be >= 1"}, + {"unsupported term", []compute.ExchangeTarget{withTerm}, `targets[0].term "P2Y" is not a supported reservation term`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + preview, offerings, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, tt.targets) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Nil(t, preview) + assert.Nil(t, offerings) + assert.False(t, callerInvoked, "caller must not be invoked when target validation fails") + }) + } +} + +// --- CalculateExchange request-builder golden assertions --- + +func TestCalculateExchange_RequestBuilderWiring(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + var captured armreservations.CalculateExchangeRequest + c.SetCalculateExchangeCaller(func(_ context.Context, body armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + captured = body + return succeededResult(&armreservations.CalculateExchangeResponseProperties{ + SessionID: to.Ptr("session-abc"), + }), nil + }) + + source := validSource() + target := validTarget() + _, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{source}, []compute.ExchangeTarget{target}) + require.NoError(t, err) + + require.NotNil(t, captured.Properties) + require.Len(t, captured.Properties.ReservationsToExchange, 1) + toReturn := captured.Properties.ReservationsToExchange[0] + assert.Equal(t, source.ReservationID, *toReturn.ReservationID) + assert.Equal(t, source.Quantity, *toReturn.Quantity) + + require.Len(t, captured.Properties.ReservationsToPurchase, 1) + toPurchase := captured.Properties.ReservationsToPurchase[0] + assert.Equal(t, target.Location, *toPurchase.Location) + assert.Equal(t, target.SKU, *toPurchase.SKU.Name) + require.NotNil(t, toPurchase.Properties) + assert.Equal(t, armreservations.AppliedScopeTypeShared, *toPurchase.Properties.AppliedScopeType, "AppliedScopeType must default to Shared when unset") + assert.Equal(t, armreservations.ReservationBillingPlanUpfront, *toPurchase.Properties.BillingPlan) + assert.Equal(t, target.BillingScopeID, *toPurchase.Properties.BillingScopeID) + assert.Equal(t, target.Quantity, *toPurchase.Properties.Quantity) + assert.False(t, *toPurchase.Properties.Renew) + assert.Equal(t, armreservations.ReservedResourceTypeVirtualMachines, *toPurchase.Properties.ReservedResourceType) + assert.Equal(t, target.Term, *toPurchase.Properties.Term) + require.NotNil(t, toPurchase.Properties.ReservedResourceProperties) + assert.Equal(t, armreservations.InstanceFlexibilityOn, *toPurchase.Properties.ReservedResourceProperties.InstanceFlexibility) +} + +func TestCalculateExchange_AppliedScopeTypeOverride(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + var captured armreservations.CalculateExchangeRequest + c.SetCalculateExchangeCaller(func(_ context.Context, body armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + captured = body + return succeededResult(&armreservations.CalculateExchangeResponseProperties{SessionID: to.Ptr("session-abc")}), nil + }) + + single := armreservations.AppliedScopeTypeSingle + target := validTarget() + target.AppliedScopeType = &single + + _, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{target}) + require.NoError(t, err) + assert.Equal(t, armreservations.AppliedScopeTypeSingle, *captured.Properties.ReservationsToPurchase[0].Properties.AppliedScopeType) +} + +// --- CalculateExchange response handling --- + +func TestCalculateExchange_NilPropertiesError(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return succeededResult(nil), nil + }) + + preview, offerings, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.Error(t, err, "a nil-properties response must not be fabricated into an empty success") + assert.Contains(t, err.Error(), "no session id") + assert.Nil(t, preview) + assert.Nil(t, offerings) +} + +func TestCalculateExchange_EmptySessionIDError(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return succeededResult(&armreservations.CalculateExchangeResponseProperties{SessionID: to.Ptr("")}), nil + }) + + _, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.Error(t, err) + assert.Contains(t, err.Error(), "no session id") +} + +// TestCalculateExchange_NonSucceededStatusIsRefused pins the same invariant +// on the quote side. A Failed/Cancelled quote that still carries a +// SessionID would otherwise be handed to executeAzureExchange, which +// commits whatever session its fresh quote returned. +func TestCalculateExchange_NonSucceededStatusIsRefused(t *testing.T) { + for _, status := range []armreservations.CalculateExchangeOperationResultStatus{ + armreservations.CalculateExchangeOperationResultStatusFailed, + armreservations.CalculateExchangeOperationResultStatusCancelled, + armreservations.CalculateExchangeOperationResultStatusPending, + } { + t.Run(string(status), func(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return armreservations.CalculateExchangeOperationResultResponse{ + Status: to.Ptr(status), + Properties: &armreservations.CalculateExchangeResponseProperties{ + SessionID: to.Ptr("session-abc"), + }, + }, nil + }) + + preview, _, err := c.CalculateExchange(context.Background(), + []compute.ExchangeableReservation{validSource()}, + []compute.ExchangeTarget{validTarget()}) + require.Error(t, err, "a %s quote must not yield an executable preview", status) + assert.Contains(t, err.Error(), string(status)) + assert.Nil(t, preview) + }) + } +} + +func TestCalculateExchange_OperationError(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return armreservations.CalculateExchangeOperationResultResponse{ + Status: to.Ptr(armreservations.CalculateExchangeOperationResultStatusFailed), + Error: &armreservations.OperationResultError{Message: to.Ptr("cross billing account exchange not allowed")}, + }, nil + }) + + _, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.Error(t, err) + assert.Contains(t, err.Error(), "cross billing account exchange not allowed") +} + +func TestCalculateExchange_PolicyErrorsExtraction(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return succeededResult(&armreservations.CalculateExchangeResponseProperties{ + SessionID: to.Ptr("session-with-policy-errors"), + PolicyResult: &armreservations.ExchangePolicyErrors{ + PolicyErrors: []*armreservations.ExchangePolicyError{ + {Code: to.Ptr("CrossBillingAccount"), Message: to.Ptr("reservations must share a billing account")}, + }, + }, + }), nil + }) + + preview, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.NoError(t, err, "a policy-rejected combination is still a successful priced call") + require.NotNil(t, preview) + require.Len(t, preview.PolicyErrors, 1) + assert.Equal(t, "CrossBillingAccount: reservations must share a billing account", preview.PolicyErrors[0]) +} + +// TestCalculateExchange_PolicyErrorsWithoutMessageStillSurface pins the +// money-path invariant that every policy violation Azure reports produces an +// entry in ExchangePreview.PolicyErrors. +// +// armreservations.ExchangePolicyError has two optional pointer fields, so +// Azure may report a violation as a bare Code. The execute handler gates +// solely on len(PolicyErrors) > 0, so dropping such an entry would empty the +// slice and let a policy-rejected exchange be committed. Pre-fix, +// extractExchangePreview skipped every entry whose Message was nil and this +// test failed with 0 entries. +func TestCalculateExchange_PolicyErrorsWithoutMessageStillSurface(t *testing.T) { + tests := []struct { + name string + policy *armreservations.ExchangePolicyError + want string + }{ + { + name: "code only", + policy: &armreservations.ExchangePolicyError{Code: to.Ptr("ExchangeNotSupported")}, + want: "ExchangeNotSupported", + }, + { + name: "empty message falls back to code", + policy: &armreservations.ExchangePolicyError{Code: to.Ptr("ExchangeNotSupported"), Message: to.Ptr("")}, + want: "ExchangeNotSupported", + }, + { + name: "neither code nor message", + policy: &armreservations.ExchangePolicyError{}, + want: "azure reported an unspecified exchange policy violation", + }, + { + name: "nil entry", + policy: nil, + want: "azure reported an unspecified exchange policy violation", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return succeededResult(&armreservations.CalculateExchangeResponseProperties{ + SessionID: to.Ptr("session-with-messageless-policy-error"), + PolicyResult: &armreservations.ExchangePolicyErrors{ + PolicyErrors: []*armreservations.ExchangePolicyError{tt.policy}, + }, + }), nil + }) + + preview, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.NoError(t, err) + require.NotNil(t, preview) + require.Len(t, preview.PolicyErrors, 1, "a policy violation must never be dropped: the execute handler gates on this slice being non-empty") + assert.Equal(t, tt.want, preview.PolicyErrors[0]) + }) + } +} + +func TestCalculateExchange_NilVsZeroMoneyFields(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return succeededResult(&armreservations.CalculateExchangeResponseProperties{ + SessionID: to.Ptr("session-no-net-payable"), + // NetPayable intentionally omitted -- Azure did not report one. + }), nil + }) + + preview, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.NoError(t, err) + assert.Nil(t, preview.NetPayable, "absent NetPayable must stay nil, never coerced to 0") + + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return succeededResult(&armreservations.CalculateExchangeResponseProperties{ + SessionID: to.Ptr("session-zero-net-payable"), + NetPayable: &armreservations.Price{Amount: to.Ptr(0.0), CurrencyCode: to.Ptr("USD")}, + }), nil + }) + preview, _, err = c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.NoError(t, err) + require.NotNil(t, preview.NetPayable, "an explicit 0.0 amount must be preserved, not treated the same as absent") + assert.InDelta(t, 0.0, *preview.NetPayable, 0.0001) + assert.Equal(t, "USD", preview.NetPayableCurrency) +} + +func TestCalculateExchange_OfferingsExtraction(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return succeededResult(&armreservations.CalculateExchangeResponseProperties{ + SessionID: to.Ptr("session-offerings"), + ReservationsToPurchase: []*armreservations.ReservationToPurchaseCalculateExchange{ + { + BillingCurrencyTotal: &armreservations.Price{Amount: to.Ptr(123.45), CurrencyCode: to.Ptr("EUR")}, + Properties: &armreservations.PurchaseRequest{ + Location: to.Ptr("westeurope"), + SKU: &armreservations.SKUName{Name: to.Ptr("Standard_D4s_v3")}, + Properties: &armreservations.PurchaseRequestProperties{ + Quantity: to.Ptr(int32(3)), + Term: to.Ptr(armreservations.ReservationTermP3Y), + }, + }, + }, + nil, // defensive: a nil entry in the SDK slice must not panic the extractor + }, + }), nil + }) + + _, offerings, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.NoError(t, err) + require.Len(t, offerings, 1) + o := offerings[0] + assert.Equal(t, "westeurope", o.Location) + assert.Equal(t, "Standard_D4s_v3", o.SKU) + assert.Equal(t, int32(3), o.Quantity) + assert.Equal(t, "P3Y", o.Term) + require.NotNil(t, o.BillingCurrencyTotal) + assert.InDelta(t, 123.45, *o.BillingCurrencyTotal, 0.0001) + assert.Equal(t, "EUR", o.CurrencyCode) +} + +func TestCalculateExchange_CtxCancelPassthrough(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return armreservations.CalculateExchangeOperationResultResponse{}, context.Canceled + }) + + _, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled, "context cancellation must propagate unwrapped, not be folded into a generic error string") +} + +func TestCalculateExchange_CallerAPIError(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetCalculateExchangeCaller(func(_ context.Context, _ armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + return armreservations.CalculateExchangeOperationResultResponse{}, errors.New("azure: CalculateExchange begin: transport error") + }) + + _, _, err := c.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, []compute.ExchangeTarget{validTarget()}) + require.Error(t, err) + assert.Contains(t, err.Error(), "transport error") +} + +// --- ExecuteExchange --- + +func TestExecuteExchange_EmptySessionID(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetDoExchangeCaller(func(_ context.Context, _ string) (armreservations.ExchangeOperationResultResponse, error) { + t.Fatal("caller must not be invoked when session_id is empty") + return armreservations.ExchangeOperationResultResponse{}, nil + }) + + res, err := c.ExecuteExchange(context.Background(), "") + require.Error(t, err) + assert.Contains(t, err.Error(), "session_id is required") + assert.Nil(t, res) +} + +func TestExecuteExchange_NilPropertiesError(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetDoExchangeCaller(func(_ context.Context, _ string) (armreservations.ExchangeOperationResultResponse, error) { + return armreservations.ExchangeOperationResultResponse{ + Status: to.Ptr(armreservations.ExchangeOperationResultStatusSucceeded), + }, nil + }) + + res, err := c.ExecuteExchange(context.Background(), "session-abc") + require.Error(t, err) + assert.Contains(t, err.Error(), "no properties") + assert.Nil(t, res) +} + +func TestExecuteExchange_OperationError(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetDoExchangeCaller(func(_ context.Context, _ string) (armreservations.ExchangeOperationResultResponse, error) { + return armreservations.ExchangeOperationResultResponse{ + Status: to.Ptr(armreservations.ExchangeOperationResultStatusFailed), + Error: &armreservations.OperationResultError{Message: to.Ptr("session expired")}, + }, nil + }) + + res, err := c.ExecuteExchange(context.Background(), "session-abc") + require.Error(t, err) + assert.Contains(t, err.Error(), "session expired") + assert.Nil(t, res) +} + +// TestExecuteExchange_FailedStatusWithoutErrorIsRefused pins the money-path +// invariant that a non-accepted terminal status is an error even when Azure +// violates its own contract and omits the Error field ("required if status +// == failed or status == canceled"). Without the status check the handler +// returns HTTP 200 with status "Failed" and logs "exchange executed", +// telling the caller a failed exchange succeeded. +func TestExecuteExchange_FailedStatusWithoutErrorIsRefused(t *testing.T) { + for _, status := range []armreservations.ExchangeOperationResultStatus{ + armreservations.ExchangeOperationResultStatusFailed, + armreservations.ExchangeOperationResultStatusCancelled, + } { + t.Run(string(status), func(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetDoExchangeCaller(func(_ context.Context, _ string) (armreservations.ExchangeOperationResultResponse, error) { + return armreservations.ExchangeOperationResultResponse{ + Status: to.Ptr(status), + Properties: &armreservations.ExchangeResponseProperties{ + NetPayable: &armreservations.Price{Amount: to.Ptr(0.0), CurrencyCode: to.Ptr("USD")}, + }, + }, nil + }) + + res, err := c.ExecuteExchange(context.Background(), "session-abc") + require.Error(t, err, "a %s exchange must not be reported as a success", status) + assert.Contains(t, err.Error(), string(status)) + assert.Nil(t, res) + }) + } +} + +// TestExecuteExchange_PendingStatusesAccepted guards the other side of the +// allow-list: PendingPurchases/PendingRefunds mean Azure committed the swap +// and is still settling one leg, so they must NOT be turned into errors. +func TestExecuteExchange_PendingStatusesAccepted(t *testing.T) { + for _, status := range []armreservations.ExchangeOperationResultStatus{ + armreservations.ExchangeOperationResultStatusPendingPurchases, + armreservations.ExchangeOperationResultStatusPendingRefunds, + } { + t.Run(string(status), func(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetDoExchangeCaller(func(_ context.Context, _ string) (armreservations.ExchangeOperationResultResponse, error) { + return armreservations.ExchangeOperationResultResponse{ + Status: to.Ptr(status), + Properties: &armreservations.ExchangeResponseProperties{}, + }, nil + }) + + res, err := c.ExecuteExchange(context.Background(), "session-abc") + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, string(status), res.Status) + }) + } +} + +func TestExecuteExchange_CtxCancelPassthrough(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + c.SetDoExchangeCaller(func(_ context.Context, _ string) (armreservations.ExchangeOperationResultResponse, error) { + return armreservations.ExchangeOperationResultResponse{}, context.DeadlineExceeded + }) + + _, err := c.ExecuteExchange(context.Background(), "session-abc") + require.Error(t, err) + assert.ErrorIs(t, err, context.DeadlineExceeded) +} + +func TestExecuteExchange_HappyPath(t *testing.T) { + c := compute.NewClient(nil, "sub-1", "") + var capturedSessionID string + c.SetDoExchangeCaller(func(_ context.Context, sessionID string) (armreservations.ExchangeOperationResultResponse, error) { + capturedSessionID = sessionID + return armreservations.ExchangeOperationResultResponse{ + Status: to.Ptr(armreservations.ExchangeOperationResultStatusSucceeded), + Properties: &armreservations.ExchangeResponseProperties{ + NetPayable: &armreservations.Price{Amount: to.Ptr(42.5), CurrencyCode: to.Ptr("USD")}, + }, + }, nil + }) + + res, err := c.ExecuteExchange(context.Background(), "session-xyz") + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, "session-xyz", capturedSessionID, "the exact session id passed in must be the one sent to Azure") + assert.Equal(t, "session-xyz", res.SessionID) + require.NotNil(t, res.NetPayable) + assert.InDelta(t, 42.5, *res.NetPayable, 0.0001) + assert.Equal(t, "USD", res.NetPayableCurrency) + assert.Equal(t, string(armreservations.ExchangeOperationResultStatusSucceeded), res.Status) +} diff --git a/providers/azure/services/compute/exchange_test.go b/providers/azure/services/compute/exchange_test.go index 0556d7fb5..9542343b4 100644 --- a/providers/azure/services/compute/exchange_test.go +++ b/providers/azure/services/compute/exchange_test.go @@ -264,3 +264,36 @@ func TestListExchangeableReservations_PagerError(t *testing.T) { require.Error(t, err) assert.ErrorContains(t, err, "azure api error") } + +// TestListExchangeableReservations_BillingScopeIDExtracted pins the issue +// #1527 ownership signal. +// +// The API layer refuses to exchange a source reservation whose +// BillingScopeID does not match the authorized subscription. If this field +// were silently dropped here, every reservation would arrive with an empty +// scope, the gate would refuse everything, and the natural "fix" would be to +// relax the gate -- reopening the cross-subscription hole. Absent must stay +// empty rather than be coerced, so the gate can tell "Azure did not report +// an owner" from any real scope. +func TestListExchangeableReservations_BillingScopeIDExtracted(t *testing.T) { + t.Parallel() + withScope := makeReservation(vmID1, "Standard_D2s_v3", 2, provStateSuc(), resTypeVM(), ifOn()) + withScope.Properties.BillingScopeID = to.Ptr("/subscriptions/sub-owner") + + withoutScope := makeReservation(vmID2, "Standard_F4s_v2", 1, provStateSuc(), resTypeVM(), ifOn()) + withoutScope.Properties.BillingScopeID = nil + + c := newClient() + c.SetExchangeablePager(&staticExchangeablePager{ + pages: []*armreservations.ListResult{ + {Value: []*armreservations.ReservationResponse{withScope, withoutScope}}, + }, + }) + + result, err := c.ListExchangeableReservations(context.Background()) + require.NoError(t, err) + require.Len(t, result, 2) + assert.Equal(t, "/subscriptions/sub-owner", result[0].BillingScopeID) + assert.Empty(t, result[1].BillingScopeID, + "an unreported billing scope must stay empty so callers can fail closed on unknown ownership") +}