fix(api): scope Azure exchangeable-RI listing to session's allowed accounts

internal/api/handler_ri_exchange.go

@@ -194,8 +194,10 @@ type azureExchangeClient interface {
 // wired OIDC signer so managed_identity / client_secret / WIF all work.
 //
 // Graceful empty-state rules (returns nil client, nil error):
-//   - subscriptionID is empty AND no Azure accounts are registered at all:
-//     Azure is not configured; the caller returns an empty reservations list.
+//   - subscriptionID is empty: GetCloudAccountByExternalID looks up an exact
+//     match on external_id, and no registered account has an empty
+//     external_id, so this unconditionally misses and the caller returns an
+//     empty reservations list.
 //   - subscriptionID is provided but no matching CloudAccount is found:
 //     treated as "Azure not configured for this subscription".
 //
@@ -232,21 +234,67 @@ func (h *Handler) buildAzureExchangeClient(ctx context.Context, subscriptionID s
 	return azurecompute.NewClient(cred, subscriptionID, ""), nil
 }
 
-// listExchangeableAzureRIs returns all active Azure VM reservations that are
+// listExchangeableAzureRIs returns active Azure VM reservations that are
 // eligible for the cross-SKU/cross-region exchange flow (InstanceFlexibility
 // == On, ProvisioningState == Succeeded). Requires "view:purchases" permission.
 //
-// The optional ?subscription_id= query parameter scopes the credential lookup
-// to the matching registered CloudAccount. When no Azure account is configured
-// for the requested subscription (or no subscription is specified and none are
-// registered), the handler returns an empty reservations list rather than a 500.
+// ListExchangeableReservations is tenant-wide by design (see its doc
+// comment: "the Azure Capacity exchange API operates on reservation order
+// IDs which span subscriptions"), and that breadth does not change just
+// because ?subscription_id= was supplied -- the parameter only picks which
+// registered CloudAccount's credentials authenticate the call, not which
+// rows come back. Left unfiltered, every caller with "view:purchases" saw
+// every reservation the deployment's Azure credential could read, including
+// subscriptions they are not scoped to (issue #1656). Both POST siblings on
+// this resource deliberately withhold that same breadth (requireAzureSubscriptionScope
+// + requireAzureSourceOwnership, both indistinguishable-denial), so this GET
+// narrows the same way:
+//
+//   - ?subscription_id= supplied: requireAzureSubscriptionScope gates
+//     whether the session may use that subscription at all (errNotFound,
+//     not 403, matching the POST siblings so a scoped caller cannot probe
+//     which subscriptions exist). The listing is then narrowed to that
+//     subscription's own rows via BillingScopeID -- the same discriminator
+//     requireAzureSourceOwnership uses -- since a reservation billed
+//     elsewhere could never be named as a valid exchange source against
+//     this subscription anyway.
+//   - No subscription_id: the listing is narrowed to whatever the session's
+//     allowed_accounts scope covers (filterAzureReservationsByScope).
+//     Unrestricted/admin sessions see everything, matching every other
+//     listing endpoint in this package. In production this branch is
+//     defense-in-depth rather than the live path today: buildAzureExchangeClient
+//     resolves subscriptionID via an exact external_id match, so an empty
+//     subscriptionID never matches a registered account and the graceful
+//     empty-state branch below returns before ListExchangeableReservations
+//     is ever called. It becomes the live path the moment a caller can reach
+//     ListExchangeableReservations without a subscription_id -- the
+//     azureExchangeFactory test seam already exercises exactly that -- which
+//     is why the filter stays rather than being deleted as unreachable.
+//
+// Rows that cannot be attributed to a registered CloudAccount (no
+// BillingScopeID, or one that matches no CloudAccount) are dropped rather
+// than kept, so an unresolvable scope never degrades to "show everything".
+// That also keeps the two empty-result cases indistinguishable: a scoped
+// caller whose own subscriptions hold zero exchangeable reservations sees
+// the same empty list as one whose rows could not be attributed.
+//
+// When no Azure account is configured for the requested subscription (or no
+// subscription is specified and none are registered), the handler returns an
+// empty reservations list rather than a 500.
 func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) {
-	if _, err := h.requirePermission(ctx, req, "view", "purchases"); err != nil {
+	session, err := h.requirePermission(ctx, req, "view", "purchases")
+	if err != nil {
 		return nil, err
 	}
 
 	subscriptionID := req.QueryStringParameters["subscription_id"]
 
+	if subscriptionID != "" {
+		if scopeErr := h.requireAzureSubscriptionScope(ctx, session, subscriptionID); scopeErr != nil {
+			return nil, scopeErr
+		}
+	}
+
 	client, err := h.buildAzureExchangeClient(ctx, subscriptionID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to build Azure exchange client: %w", err)
@@ -263,9 +311,130 @@ func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.Lamb
 		return nil, fmt.Errorf("failed to list exchangeable Azure reservations: %w", err)
 	}
 
+	if subscriptionID != "" {
+		reservations = filterAzureReservationsBySubscription(reservations, subscriptionID)
+	} else {
+		reservations, err = h.filterAzureReservationsByScope(ctx, session, reservations)
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	return &ExchangeableAzureRIsResponse{Reservations: reservations}, nil
 }
 
+// filterAzureReservationsBySubscription narrows a tenant-wide reservation
+// listing down to the rows billed to subscriptionID, using the same
+// BillingScopeID discriminator requireAzureSourceOwnership keys on (issue
+// #1527). Reservations with no BillingScopeID, or one billed to a different
+// subscription, are dropped.
+//
+// Called only after requireAzureSubscriptionScope has cleared the caller for
+// subscriptionID; a reservation billed elsewhere could never be named as a
+// valid exchange source against this subscription anyway
+// (requireAzureSourceOwnership refuses cross-subscription sources), so
+// returning the wider tenant listing here would just be the enumeration leak
+// issue #1656 describes.
+func filterAzureReservationsBySubscription(reservations []azurecompute.ExchangeableReservation, subscriptionID string) []azurecompute.ExchangeableReservation {
+	scope := azureBillingScopeID(subscriptionID)
+	filtered := make([]azurecompute.ExchangeableReservation, 0, len(reservations))
+	for i := range reservations {
+		if strings.EqualFold(reservations[i].BillingScopeID, scope) {
+			filtered = append(filtered, reservations[i])
+		}
+	}
+	return filtered
+}
+
+// filterAzureReservationsByScope narrows a tenant-wide Azure reservation
+// listing down to the rows billed to a subscription the session's
+// allowed_accounts scope covers. Mirrors filterDashboardRecommendations
+// (handler_dashboard.go): unrestricted/admin sessions pass through
+// unchanged; a restricted session keeps only the rows it can resolve back to
+// one of its allowed CloudAccounts.
+//
+// Each reservation's BillingScopeID ("/subscriptions/{subscriptionID}") is
+// the same ownership signal requireAzureSourceOwnership keys on (issue
+// #1527) -- the subscription actually charged, which is the correct
+// discriminator even for AppliedScopeType == Shared.
+//
+// Fails closed: a reservation with no BillingScopeID, or one that matches no
+// registered Azure CloudAccount, is dropped rather than kept. Counts, not
+// the dropped reservation/billing-scope identifiers, are safe to log --
+// logging the identifiers would just relocate the disclosure this filter
+// exists to prevent.
+func (h *Handler) filterAzureReservationsByScope(ctx context.Context, session *Session, reservations []azurecompute.ExchangeableReservation) ([]azurecompute.ExchangeableReservation, error) {
+	allowed, err := h.getAllowedAccounts(ctx, session)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get allowed accounts: %w", err)
+	}
+	if auth.IsUnrestrictedAccess(allowed) {
+		// client.ListExchangeableReservations may return a nil slice for zero
+		// results; every other path here returns a non-nil empty slice, so
+		// normalize here too rather than letting an admin session alone see
+		// {"reservations": null}.
+		if reservations == nil {
+			reservations = []azurecompute.ExchangeableReservation{}
+		}
+		return reservations, nil
+	}
+
+	provider := "azure"
+	accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{Provider: &provider})
+	if err != nil {
+		return nil, fmt.Errorf("failed to list cloud accounts: %w", err)
+	}
+
+	filtered := filterReservationsByScopeIndex(reservations, azureScopeIndex(accounts), allowed)
+	if dropped := len(reservations) - len(filtered); dropped > 0 {
+		logging.Debugf("azure exchange listing: filtered %d of %d tenant-wide reservations outside session scope", dropped, len(reservations))
+	}
+	return filtered, nil
+}
+
+// azureScopeIndex builds a lookup from ARM billing scope (lower-cased) to the
+// registered Azure CloudAccount that owns it, so filterAzureReservationsByScope
+// can resolve each reservation's BillingScopeID straight to its CloudAccount
+// without re-deriving azureBillingScopeID per row. Accounts with no
+// ExternalID are skipped -- they cannot own any BillingScopeID.
+//
+// Split out of filterAzureReservationsByScope, together with
+// filterReservationsByScopeIndex below, to stay under the project's gocyclo
+// threshold (mirrors validateAzureOfferingsBody's precedent).
+func azureScopeIndex(accounts []config.CloudAccount) map[string]config.CloudAccount {
+	index := make(map[string]config.CloudAccount, len(accounts))
+	for i := range accounts {
+		if accounts[i].ExternalID == "" {
+			continue
+		}
+		index[strings.ToLower(azureBillingScopeID(accounts[i].ExternalID))] = accounts[i]
+	}
+	return index
+}
+
+// filterReservationsByScopeIndex narrows reservations down to the rows whose
+// BillingScopeID resolves, via scopeToAccount, to a CloudAccount the allowed
+// list covers (auth.MatchesAccount). Fails closed: a reservation with no
+// BillingScopeID, or one that resolves to no registered CloudAccount, is
+// dropped rather than kept.
+func filterReservationsByScopeIndex(reservations []azurecompute.ExchangeableReservation, scopeToAccount map[string]config.CloudAccount, allowed []string) []azurecompute.ExchangeableReservation {
+	filtered := make([]azurecompute.ExchangeableReservation, 0, len(reservations))
+	for i := range reservations {
+		r := reservations[i]
+		if r.BillingScopeID == "" {
+			continue
+		}
+		account, ok := scopeToAccount[strings.ToLower(r.BillingScopeID)]
+		if !ok {
+			continue
+		}
+		if auth.MatchesAccount(allowed, account.ID, account.Name) {
+			filtered = append(filtered, r)
+		}
+	}
+	return filtered
+}
+
 // maxAzureExchangeItems caps the number of sources/targets accepted per
 // Azure exchange request, guarding against an oversized request fanning out
 // into an enormous CalculateExchange payload.