From 859026d4c46d2b1a4aeb1856e31c02a890bd1c41 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:02:20 -0700 Subject: [PATCH 1/3] sec(api): apply allowed_accounts scope to listConvertibleRIs, getRIUtilization, getReshapeRecommendations Three RI-exchange listing handlers gated on view:purchases but did not apply the caller's allowed_accounts filter, so a restricted user could see the full convertible-RI inventory and utilization for accounts outside their scope. getRIExchangeHistory already filtered by allowed_accounts; these three siblings now match that behaviour exactly (closes #1030). - Extract session from requirePermission in all three handlers. - Gate on getAllowedAccounts + auth.IsUnrestrictedAccess before any AWS SDK call; resolve the deployment CloudAccount UUID via resolveAWSCloudAccountID (test-injectable via reshapeAccountResolver). - Return an empty typed response when the deployment account is outside the session's allowed_accounts, identical to the getRIExchangeHistory pattern. - Add six regression tests in handler_per_account_perms_test.go covering the out-of-scope (returns empty) and admin/in-scope (passes gate) paths for all three handlers. --- .../api/handler_per_account_perms_test.go | 294 +++++++++++++++++- internal/api/handler_ri_exchange.go | 65 +++- 2 files changed, 346 insertions(+), 13 deletions(-) diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 7ba7eddbc..36021848b 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -10,16 +10,19 @@ package api // minimal: just enough to confirm the filter passes when accounts match so we // know the test would fail for the right reason if the enforcement were removed. // -// Endpoints covered (9 total): -// 1. GET /recommendations (filterRecommendationsByAllowedAccounts) -// 2. GET /recommendations/:id (getRecommendationDetail — cross-account rejection) -// 3. GET /history (filterPurchaseHistoryByAllowedAccounts) -// 4. GET /history/analytics (validateAnalyticsAccountScope — cross-account rejection) -// 5. GET /history/breakdown (validateAnalyticsAccountScope — cross-account rejection) -// 6. GET /dashboard/summary (filterDashboardRecommendations — aggregate subset) -// 7. POST /purchases/execute (validatePurchaseRecommendationScope — 403) -// 8. GET /purchases/planned list (requireExecutionAccess / requirePlanAccess — 404) -// 9. GET /inventory/coverage (filterRecommendationsByAllowedAccounts on recs leg) +// Endpoints covered (12 total): +// 1. GET /recommendations (filterRecommendationsByAllowedAccounts) +// 2. GET /recommendations/:id (getRecommendationDetail — cross-account rejection) +// 3. GET /history (filterPurchaseHistoryByAllowedAccounts) +// 4. GET /history/analytics (validateAnalyticsAccountScope — cross-account rejection) +// 5. GET /history/breakdown (validateAnalyticsAccountScope — cross-account rejection) +// 6. GET /dashboard/summary (filterDashboardRecommendations — aggregate subset) +// 7. POST /purchases/execute (validatePurchaseRecommendationScope — 403) +// 8. GET /purchases/planned list (requireExecutionAccess / requirePlanAccess — 404) +// 9. GET /inventory/coverage (filterRecommendationsByAllowedAccounts on recs leg) +// 10. GET /ri-exchange/instances (getAllowedAccounts scope, issue #1030) +// 11. GET /ri-exchange/utilization (getAllowedAccounts scope, issue #1030) +// 12. GET /ri-exchange/reshape (getAllowedAccounts scope, issue #1030) import ( "context" @@ -28,7 +31,10 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/providers/aws/recommendations" + ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" "github.com/aws/aws-lambda-go/events" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -981,3 +987,271 @@ func TestPerAccountPerms_PlannedPurchase_AllowedAccountPlanSucceeds(t *testing.T assert.Equal(t, "paused", result.Status, "result must reflect the paused status") mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, executionID, mock.Anything, "paused") } + +// ─── 10. GET /ri-exchange/instances ────────────────────────────────────────── + +// TestPerAccountPerms_ListConvertibleRIs_ScopedOutsideAllowedReturnsEmpty +// asserts that a restricted user (allowed_accounts: [permsAccA]) receives an +// empty instance list when the deployment's registered CloudAccount UUID is +// permsAccB (outside their scope). +// +// The handler resolves the deployment CloudAccount ID via reshapeAccountResolver +// and checks it against the session's allowed_accounts with auth.MatchesAccount. +// When they don't match, no AWS SDK call is made and an empty list is returned. +// +// Regression: removing the getAllowedAccounts guard from listConvertibleRIs +// causes the handler to proceed to the AWS SDK call and the assertion fails +// because the scope check never fires. +func TestPerAccountPerms_ListConvertibleRIs_ScopedOutsideAllowedReturnsEmpty(t *testing.T) { + ctx := context.Background() + + mockStore := new(MockConfigStore) + // resolveAccountNamesByID calls ListCloudAccounts so MatchesAccount can + // resolve display names. Return both accounts so the name lookup is realistic. + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + + handler := &Handler{ + auth: scopedAuthMock(ctx), + config: mockStore, + // Inject the deployment CloudAccount UUID as permsAccB — outside + // the scoped user's allowed set ([permsAccA]). + reshapeAccountResolver: func(_ context.Context) (string, error) { + return permsAccB, nil + }, + } + + resp, err := handler.listConvertibleRIs(ctx, scopedReq()) + require.NoError(t, err, "scope mismatch must return empty list, not error") + typed, ok := resp.(*ConvertibleRIsResponse) + require.True(t, ok, "expected *ConvertibleRIsResponse, got %T", resp) + assert.Empty(t, typed.Instances, + "scoped user must receive an empty RI list when the deployment account is outside their allowed_accounts") +} + +// TestPerAccountPerms_ListConvertibleRIs_AdminSeesAll verifies that an admin +// (unrestricted) session bypasses the allowed_accounts gate entirely. The +// handler will attempt the AWS SDK call, which may fail without credentials +// in a test environment, but the scope check itself must not block the admin. +// +// This is the positive path through IsUnrestrictedAccess — if it were missing, +// the admin session would also go through the account check. +func TestPerAccountPerms_ListConvertibleRIs_AdminSeesAll(t *testing.T) { + ctx := context.Background() + + // Admin auth — ValidateSession returns Role:"admin" so getAllowedAccounts + // returns nil (unrestricted) and the scope filter is skipped entirely. + mockAuth, req := adminDashboardReq(ctx) + + // reshapeAccountResolver is NOT set: if the scope check incorrectly runs for + // admins it would call h.resolveAWSCloudAccountID which needs STS, causing a + // config-load error. The test confirms the resolver is never called by NOT + // injecting one — any call to it would panic. + mockStore := new(MockConfigStore) + + handler := &Handler{ + auth: mockAuth, + config: mockStore, + // Pre-seal awsCfgOnce with a zero Config so loadAWSConfigWithRegion + // returns immediately without hitting the AWS SDK. The EC2 client + // will fail (no real credentials), but we only care that the scope + // check is skipped — the subsequent error from AWS is expected. + // + // We can't inject an EC2 factory into listConvertibleRIs (it calls + // awsprovider.NewEC2ClientDirect directly), so we leave this as a + // known limitation: the admin path is confirmed scope-check-free, + // and the subsequent AWS failure is not part of this assertion. + } + // Pre-seal awsCfgOnce so loadAWSConfigWithRegion returns without loading. + handler.awsCfgOnce.Do(func() {}) + + // The call will fail at the AWS SDK level (no credentials in test), but + // critically it must NOT fail with a scope/allowed_accounts error. The + // scope check must be skipped for admins. + _, err := handler.listConvertibleRIs(ctx, req) + // If err is non-nil it must be an AWS/SDK error, not a scope error. + if err != nil { + assert.NotContains(t, err.Error(), "allowed accounts", + "admin must bypass the allowed_accounts scope check; error must be AWS-level, not scope-level") + assert.NotContains(t, err.Error(), "cloud account scope", + "admin must bypass the cloud account scope check") + } +} + +// ─── 11. GET /ri-exchange/utilization ──────────────────────────────────────── + +// TestPerAccountPerms_GetRIUtilization_ScopedOutsideAllowedReturnsEmpty +// asserts that a restricted user receives an empty utilization list when the +// deployment's registered CloudAccount UUID is outside their allowed_accounts. +// +// Regression: removing the getAllowedAccounts guard from getRIUtilization +// causes the handler to proceed to the AWS SDK call (Cost Explorer) and the +// empty-list assertion fails because the scope check never fires. +func TestPerAccountPerms_GetRIUtilization_ScopedOutsideAllowedReturnsEmpty(t *testing.T) { + ctx := context.Background() + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + + handler := &Handler{ + auth: scopedAuthMock(ctx), + config: mockStore, + // Deployment account is permsAccB — outside the scoped user's allowed set. + reshapeAccountResolver: func(_ context.Context) (string, error) { + return permsAccB, nil + }, + } + + resp, err := handler.getRIUtilization(ctx, scopedReq()) + require.NoError(t, err, "scope mismatch must return empty list, not error") + typed, ok := resp.(*RIUtilizationResponse) + require.True(t, ok, "expected *RIUtilizationResponse, got %T", resp) + assert.Empty(t, typed.Utilization, + "scoped user must receive empty utilization when the deployment account is outside their allowed_accounts") +} + +// TestPerAccountPerms_GetRIUtilization_ScopedWithinAllowedProceedsToAWS +// confirms that a scoped user whose allowed_accounts DOES include the +// deployment CloudAccount UUID is not blocked by the scope check. The handler +// will then proceed to the AWS SDK (and fail without credentials), but the +// scope filter itself must be transparent for an in-scope user. +// +// Regression: an overly-broad scope check that blocks ALL scoped users +// (not just out-of-scope ones) would cause this test to return empty prematurely. +func TestPerAccountPerms_GetRIUtilization_ScopedWithinAllowedProceedsToAWS(t *testing.T) { + ctx := context.Background() + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + + handler := &Handler{ + auth: scopedAuthMock(ctx), + config: mockStore, + // Deployment account is permsAccA — WITHIN the scoped user's allowed set. + reshapeAccountResolver: func(_ context.Context) (string, error) { + return permsAccA, nil + }, + } + // Pre-seal awsCfgOnce so the handler doesn't stall on AWS SDK load. + handler.awsCfgOnce.Do(func() {}) + + _, err := handler.getRIUtilization(ctx, scopedReq()) + // The scope check passes; any error from here is AWS-level, not scope-level. + if err != nil { + assert.NotContains(t, err.Error(), "allowed accounts", + "in-scope user must not be blocked by allowed_accounts check") + assert.NotContains(t, err.Error(), "cloud account scope", + "in-scope user must not be blocked by cloud account scope check") + } +} + +// ─── 12. GET /ri-exchange/reshape-recommendations ──────────────────────────── + +// TestPerAccountPerms_GetReshapeRecommendations_ScopedOutsideAllowedReturnsEmpty +// asserts that a restricted user receives an empty recommendations list when +// the deployment's registered CloudAccount UUID is outside their allowed_accounts. +// No AWS EC2 or Cost Explorer calls should be made. +// +// Regression: removing the getAllowedAccounts guard from getReshapeRecommendations +// causes the handler to proceed to the AWS SDK calls and the empty-list +// assertion fails because the scope check never fires. +func TestPerAccountPerms_GetReshapeRecommendations_ScopedOutsideAllowedReturnsEmpty(t *testing.T) { + ctx := context.Background() + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + + handler := &Handler{ + auth: scopedAuthMock(ctx), + config: mockStore, + // Deployment account is permsAccB — outside the scoped user's allowed set. + reshapeAccountResolver: func(_ context.Context) (string, error) { + return permsAccB, nil + }, + } + + resp, err := handler.getReshapeRecommendations(ctx, scopedReq()) + require.NoError(t, err, "scope mismatch must return empty recommendations, not error") + typed, ok := resp.(*ReshapeRecommendationsResponse) + require.True(t, ok, "expected *ReshapeRecommendationsResponse, got %T", resp) + assert.Empty(t, typed.Recommendations, + "scoped user must receive empty reshape recommendations when the deployment account is outside their allowed_accounts") +} + +// TestPerAccountPerms_GetReshapeRecommendations_AdminSeesAll verifies that an +// admin (unrestricted) session bypasses the allowed_accounts gate and reaches +// the actual reshape logic. The handler then proceeds to the AWS SDK and recs +// lookup, using factory injection so the test runs without real AWS credentials. +// +// Note: reshapeAccountResolver is injected here not because admins need it for +// the scope check (they bypass it via IsUnrestrictedAccess), but because the +// reshape handler independently calls resolveAccount later for the recs-lookup +// scope filter. Without injection it would invoke STS which fails in the test +// environment with a 403. This is a test-plumbing concern, NOT a regression +// for the scope-check logic. +// +// Regression: if IsUnrestrictedAccess is not checked (or is inverted), the +// admin would incorrectly fall into the scope-check block. We confirm this +// does NOT happen by ensuring the response is a valid *ReshapeRecommendationsResponse +// (not an error and not an empty stub from the scope-reject branch). +func TestPerAccountPerms_GetReshapeRecommendations_AdminSeesAll(t *testing.T) { + ctx := context.Background() + + mockStore := new(MockConfigStore) + // ListStoredRecommendations is called by the recs lookup closure inside + // AnalyzeReshapingWithRecs. Return empty so the test finishes without + // surfacing unrelated recs-lookup errors. + mockStore.On("ListStoredRecommendations", mock.Anything, mock.Anything). + Return([]config.RecommendationRecord(nil), nil) + mockStore.On("GetRecommendationsFreshness", mock.Anything). + Return(&config.RecommendationsFreshness{}, nil) + + mockAuth, req := adminDashboardReq(ctx) + + handler := &Handler{ + auth: mockAuth, + config: mockStore, + // Inject EC2 + recs stubs so no real AWS calls are needed. + reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { + return &fakeReshapeEC2Stub{ + instances: []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: "USD"}, + }, + } + }, + reshapeRecsFactory: func(_ aws.Config) reshapeRecsClient { + return &fakeReshapeRecsStub{ + utilization: []recommendations.RIUtilization{ + {ReservedInstanceID: "ri-1", UtilizationPercent: 50.0}, + }, + } + }, + // reshapeAccountResolver is injected to bypass the STS call that + // getReshapeRecommendations makes for the recs-lookup scope filter + // (a separate, pre-existing call unrelated to the allowed_accounts + // scope check). Returning permsAccA is arbitrary — for an admin the + // allowed_accounts block is never reached so this value only affects + // which account's recs are queried, not the scope decision. + reshapeAccountResolver: func(_ context.Context) (string, error) { + return permsAccA, nil + }, + } + handler.awsCfgOnce.Do(func() { + handler.awsCfg = aws.Config{Region: "us-east-1"} + }) + + resp, err := handler.getReshapeRecommendations(ctx, req) + require.NoError(t, err, "admin must reach the reshape logic without being blocked by scope check") + typed, ok := resp.(*ReshapeRecommendationsResponse) + require.True(t, ok, "expected *ReshapeRecommendationsResponse, got %T", resp) + // We only assert no error and correct type — proving the admin path + // proceeded past the scope gate into the actual reshape logic. + _ = typed +} diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index d4afa20b4..bd6a405b9 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -286,10 +286,31 @@ func (h *Handler) loadAWSConfigWithRegion(ctx context.Context, region string) (a // closed (returns an error) instead of silently leaking the ambient account's // RIs under another account's filter. func (h *Handler) listConvertibleRIs(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 } + // Apply the session's allowed_accounts scope: a restricted user must + // only see RIs for the deployment's registered AWS account. Mirrors + // the same guard in getRIExchangeHistory. + if allowed, aErr := h.getAllowedAccounts(ctx, session); aErr != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", aErr) + } else if !auth.IsUnrestrictedAccess(allowed) { + resolveAccount := h.resolveAWSCloudAccountID + if h.reshapeAccountResolver != nil { + resolveAccount = h.reshapeAccountResolver + } + cloudAccountID, aErr := resolveAccount(ctx) + if aErr != nil { + return nil, fmt.Errorf("failed to resolve cloud account scope for RI listing: %w", aErr) + } + nameByID := h.resolveAccountNamesByID(ctx) + if !auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]) { + return &ConvertibleRIsResponse{Instances: []ec2svc.ConvertibleRI{}}, nil + } + } + if accountID := req.QueryStringParameters["account_id"]; accountID != "" { resolve := h.resolveAWSAccountID if h.riInstancesAccountResolver != nil { @@ -321,10 +342,29 @@ func (h *Handler) listConvertibleRIs(ctx context.Context, req *events.LambdaFunc // getRIUtilization returns per-RI utilization from Cost Explorer. func (h *Handler) getRIUtilization(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 } + // Apply the session's allowed_accounts scope. Mirrors getRIExchangeHistory. + if allowed, aErr := h.getAllowedAccounts(ctx, session); aErr != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", aErr) + } else if !auth.IsUnrestrictedAccess(allowed) { + resolveAccount := h.resolveAWSCloudAccountID + if h.reshapeAccountResolver != nil { + resolveAccount = h.reshapeAccountResolver + } + cloudAccountID, aErr := resolveAccount(ctx) + if aErr != nil { + return nil, fmt.Errorf("failed to resolve cloud account scope for RI utilization: %w", aErr) + } + nameByID := h.resolveAccountNamesByID(ctx) + if !auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]) { + return &RIUtilizationResponse{Utilization: []recommendations.RIUtilization{}}, nil + } + } + lookbackDays, err := parseLookbackDaysParam(req.QueryStringParameters) if err != nil { return nil, err @@ -429,10 +469,29 @@ func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recomme // getReshapeRecommendations orchestrates fetching convertible RIs + utilization // and returns reshape recommendations. func (h *Handler) getReshapeRecommendations(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 } + // Apply the session's allowed_accounts scope. Mirrors getRIExchangeHistory. + if allowed, aErr := h.getAllowedAccounts(ctx, session); aErr != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", aErr) + } else if !auth.IsUnrestrictedAccess(allowed) { + resolveAccount := h.resolveAWSCloudAccountID + if h.reshapeAccountResolver != nil { + resolveAccount = h.reshapeAccountResolver + } + cloudAccountID, aErr := resolveAccount(ctx) + if aErr != nil { + return nil, fmt.Errorf("failed to resolve cloud account scope for reshape recommendations: %w", aErr) + } + nameByID := h.resolveAccountNamesByID(ctx) + if !auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]) { + return &ReshapeRecommendationsResponse{Recommendations: []exchange.ReshapeRecommendation{}}, nil + } + } + threshold, err := parseThresholdParam(req.QueryStringParameters) if err != nil { return nil, err From 2d6868dbb4d79ef9f0c52d36202cedafd0e859dc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:19:51 -0700 Subject: [PATCH 2/3] fix(api): require explicit region on execute, resolve from SDK on quote (closes #1060) getExchangeQuote now resolves the region from the AWS SDK configuration chain when the caller omits it, matching getReshapeRecommendations instead of hardcoding us-east-1. executeExchange now returns 400 when region is absent. RI exchanges are financially irreversible; silently defaulting to us-east-1 would execute the exchange in the wrong region for deployments elsewhere. executeApprovedExchange now fails the exchange via failExchange when record.Region is empty instead of silently defaulting to us-east-1. A stored record without a region stamp should not proceed; the region must have been captured at record-creation time. Three regression tests pin each path: - TestExecuteExchange_EmptyRegionReturns400 - TestGetExchangeQuote_EmptyRegionResolvesFromSDK - TestExecuteApprovedExchange_EmptyRecordRegionFails Finding: 01-L4 from report 01-api-handlers (FOLD-1042). --- internal/api/handler_ri_exchange.go | 25 +++++-- internal/api/handler_ri_exchange_test.go | 95 +++++++++++++++++++++++- 2 files changed, 111 insertions(+), 9 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index bd6a405b9..4d403c6d1 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -633,10 +633,14 @@ func (h *Handler) getExchangeQuote(ctx context.Context, req *events.LambdaFuncti return nil, err } - region := body.Region - if region == "" { - region = "us-east-1" + // Resolve region from the AWS SDK chain when the caller omits it, + // matching getReshapeRecommendations. Hardcoding us-east-1 would + // return an incorrect quote for deployments in other regions. + cfg, err := h.loadAWSConfigWithRegion(ctx, body.Region) + if err != nil { + return nil, fmt.Errorf("failed to load AWS config: %w", err) } + region := cfg.Region quote, err := exchange.GetExchangeQuote(ctx, exchange.ExchangeQuoteRequest{ Region: region, @@ -671,6 +675,13 @@ func validateExecuteExchangeBody(body ExchangeExecuteRequestBody) error { if body.MaxPaymentDueUSD == "" { return NewClientError(400, "max_payment_due_usd is required as a safety guardrail") } + // Region is required on execute: RI exchanges are region-scoped and + // financially irreversible. Silently defaulting to us-east-1 would + // execute the exchange in the wrong region for deployments hosted + // elsewhere, with no way to undo the operation. + if body.Region == "" { + return NewClientError(400, "region is required for execute; omitting it risks exchanging RIs in the wrong region") + } return nil } @@ -696,9 +707,6 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio } region := body.Region - if region == "" { - region = "us-east-1" - } exchangeID, quote, err := exchange.ExecuteExchange(ctx, exchange.ExchangeExecuteRequest{ Region: region, @@ -1212,7 +1220,10 @@ func (h *Handler) executeApprovedExchange(ctx context.Context, id string, record region := record.Region if region == "" { - region = "us-east-1" + // Region is a required field captured at record-creation time. + // Defaulting to us-east-1 here would execute a financial mutation + // in the wrong region for records created without a region stamp. + return h.failExchange(ctx, id, "exchange record has no region; cannot execute safely") } if globalCfg.RIExchangeMaxPerExchangeUSD == 0 { diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 88dd3175d..b93074866 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -220,10 +220,10 @@ func TestExecuteExchange_Validation(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "max_payment_due_usd is required") - // Invalid max_payment_due_usd + // Invalid max_payment_due_usd -- region provided so validation reaches the ParseDecimalRat check. _, err = h.executeExchange(context.Background(), &events.LambdaFunctionURLRequest{ Headers: map[string]string{"authorization": "Bearer test-token"}, - Body: `{"ri_ids":["ri-123"],"target_offering_id":"offering-1","target_count":1,"max_payment_due_usd":"abc"}`, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"offering-1","target_count":1,"max_payment_due_usd":"abc","region":"us-east-1"}`, }) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid max_payment_due_usd") @@ -1270,6 +1270,97 @@ func TestMapAWSExchangeError_NonAWSError(t *testing.T) { assert.Equal(t, 500, ce.code) } +// TestExecuteExchange_EmptyRegionReturns400 pins finding 01-L4: +// executeExchange must reject a request with no region rather than +// silently routing the exchange to us-east-1, where RIs may not live. +// A wrong-region execute is a financially irreversible mistake. +func TestExecuteExchange_EmptyRegionReturns400(t *testing.T) { + h := &Handler{auth: &mockAuthForExchange{}} + + _, err := h.executeExchange(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"off-1","max_payment_due_usd":"10.00"}`, + }) + 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(), "region is required") +} + +// TestGetExchangeQuote_EmptyRegionResolvesFromSDK pins finding 01-L4: +// getExchangeQuote must resolve the region from the AWS SDK chain when +// the caller omits it, matching getReshapeRecommendations, instead of +// hardcoding us-east-1. We pre-seal awsCfgOnce with a non-default region +// and verify the quote call receives that region (the call fails because +// there is no real AWS SDK, but the region check happens first via body +// validation passing; here we verify no 400 for missing region). +func TestGetExchangeQuote_EmptyRegionResolvesFromSDK(t *testing.T) { + const cfgRegion = "eu-west-1" + h := &Handler{auth: &mockAuthForExchange{}} + // Pre-seal awsCfgOnce so loadAWSConfigWithRegion returns cfgRegion + // without making a real SDK call. + h.awsCfgOnce.Do(func() { + h.awsCfg = aws.Config{Region: cfgRegion} + }) + + // The quote call will fail (no real AWS endpoint), but it must NOT fail + // with a 400 "region is required" — region resolution from the SDK is + // the accepted path for quote (non-mutation). The error will be a 500 + // from exchange.GetExchangeQuote failing against no real AWS. + _, err := h.getExchangeQuote(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"off-1"}`, + }) + // We expect either no error or a 500 from the AWS call — never a 400 + // complaining about a missing region. + if err != nil { + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got: %v", err) + assert.NotEqual(t, 400, ce.code, + "getExchangeQuote must not return 400 for a missing region; it must resolve from the SDK chain") + } +} + +// TestExecuteApprovedExchange_EmptyRecordRegionFails pins finding 01-L4: +// executeApprovedExchange must fail the exchange (not default to us-east-1) +// when the stored record has no region. A hardcoded fallback would execute +// a financially irreversible operation in the wrong region. +func TestExecuteApprovedExchange_EmptyRecordRegionFails(t *testing.T) { + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-000000000001" + + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + h := &Handler{config: mockStore} + + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil) + // Record has no region — the handler must call failExchange, not proceed. + mockStore.On("FailRIExchange", ctx, id, mock.MatchedBy(func(reason string) bool { + return len(reason) > 0 + })).Return(nil) + + record := &config.RIExchangeRecord{ + ID: id, + Region: "", // intentionally empty + SourceRIIDs: []string{"ri-1"}, + TargetOfferingID: "offering-1", + TargetCount: 1, + PaymentDue: "50.00", + } + + resp, err := h.executeApprovedExchange(ctx, id, record) + require.NoError(t, err, "executeApprovedExchange surfaces the failure via failExchange, not via error return") + respMap, ok := resp.(map[string]any) + require.True(t, ok) + assert.Equal(t, "failed", respMap["status"]) + assert.Contains(t, respMap["reason"], "no region") +} + // TestClassifyRecsAge pins the staleness classification thresholds for // the reshape freshness banner. The three transitions are: // From aef730751b60d93ba8d6f1438a5f83690fd5ccb5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 06:28:12 -0700 Subject: [PATCH 3/3] refactor(api): reduce gocyclo complexity in RI exchange handlers Extract reshapeCloudAccountInScope, resolveReshapeCloudAccountID, checkListRIsAccountIDParam, and parseReshapeParams helpers to bring getReshapeRecommendations (15->10) and listConvertibleRIs (13->7) within the gocyclo-10 threshold enforced by pre-commit. Also apply gofmt to pkg/exchange/reshape_crossfamily_test.go which the CI hook auto-fixed in-place but whose correction was not committed. --- internal/api/handler_ri_exchange.go | 187 +++++++++++++---------- pkg/exchange/reshape_crossfamily_test.go | 16 +- 2 files changed, 115 insertions(+), 88 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 4d403c6d1..99dc800c9 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -274,6 +274,60 @@ func (h *Handler) loadAWSConfigWithRegion(ctx context.Context, region string) (a return cfg, nil } +// reshapeCloudAccountInScope checks whether the session's allowed_accounts +// permit access to the deployment's registered AWS cloud account. It returns +// (true, nil) when the session is unrestricted or the cloud account matches, +// (false, nil) when the cloud account is outside the session's scope (caller +// should return an empty response), and (false, err) on any lookup failure. +// Used by listConvertibleRIs, getRIUtilization, and getReshapeRecommendations +// to eliminate duplicated account-scoping blocks. +func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Session) (bool, error) { + allowed, aErr := h.getAllowedAccounts(ctx, session) + if aErr != nil { + return false, fmt.Errorf("failed to get allowed accounts: %w", aErr) + } + if auth.IsUnrestrictedAccess(allowed) { + return true, nil + } + cloudAccountID, aErr := h.resolveReshapeCloudAccountID(ctx) + if aErr != nil { + return false, fmt.Errorf("failed to resolve cloud account scope: %w", aErr) + } + nameByID := h.resolveAccountNamesByID(ctx) + return auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]), nil +} + +// resolveReshapeCloudAccountID returns the cloud account ID for the running +// deployment, using the test-injected reshapeAccountResolver when set and +// falling back to the production resolveAWSCloudAccountID (STS-backed). +func (h *Handler) resolveReshapeCloudAccountID(ctx context.Context) (string, error) { + if h.reshapeAccountResolver != nil { + return h.reshapeAccountResolver(ctx) + } + return h.resolveAWSCloudAccountID(ctx) +} + +// checkListRIsAccountIDParam enforces the ?account_id= chip filter for +// listConvertibleRIs. Returns (true, nil) when the running AWS account matches +// the requested account (or when no account_id param is given), (false, nil) +// when it does not match (caller should return an empty list), and +// (false, err) on STS failure. +func (h *Handler) checkListRIsAccountIDParam(ctx context.Context, params map[string]string) (bool, error) { + accountID := params["account_id"] + if accountID == "" { + return true, nil + } + resolve := h.resolveAWSAccountID + if h.riInstancesAccountResolver != nil { + resolve = h.riInstancesAccountResolver + } + runningAccountID, err := resolve(ctx) + if err != nil { + return false, fmt.Errorf("failed to resolve running AWS account for RI scope: %w", err) + } + return runningAccountID == accountID, nil +} + // listConvertibleRIs returns all active convertible Reserved Instances for // the running AWS account. // @@ -292,37 +346,17 @@ func (h *Handler) listConvertibleRIs(ctx context.Context, req *events.LambdaFunc } // Apply the session's allowed_accounts scope: a restricted user must - // only see RIs for the deployment's registered AWS account. Mirrors - // the same guard in getRIExchangeHistory. - if allowed, aErr := h.getAllowedAccounts(ctx, session); aErr != nil { - return nil, fmt.Errorf("failed to get allowed accounts: %w", aErr) - } else if !auth.IsUnrestrictedAccess(allowed) { - resolveAccount := h.resolveAWSCloudAccountID - if h.reshapeAccountResolver != nil { - resolveAccount = h.reshapeAccountResolver - } - cloudAccountID, aErr := resolveAccount(ctx) - if aErr != nil { - return nil, fmt.Errorf("failed to resolve cloud account scope for RI listing: %w", aErr) - } - nameByID := h.resolveAccountNamesByID(ctx) - if !auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]) { - return &ConvertibleRIsResponse{Instances: []ec2svc.ConvertibleRI{}}, nil - } + // only see RIs for the deployment's registered AWS account. + if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { + return nil, sErr + } else if !inScope { + return &ConvertibleRIsResponse{Instances: []ec2svc.ConvertibleRI{}}, nil } - if accountID := req.QueryStringParameters["account_id"]; accountID != "" { - resolve := h.resolveAWSAccountID - if h.riInstancesAccountResolver != nil { - resolve = h.riInstancesAccountResolver - } - runningAccountID, err := resolve(ctx) - if err != nil { - return nil, fmt.Errorf("failed to resolve running AWS account for RI scope: %w", err) - } - if runningAccountID != accountID { - return &ConvertibleRIsResponse{Instances: []ec2svc.ConvertibleRI{}}, nil - } + if inScope, sErr := h.checkListRIsAccountIDParam(ctx, req.QueryStringParameters); sErr != nil { + return nil, sErr + } else if !inScope { + return &ConvertibleRIsResponse{Instances: []ec2svc.ConvertibleRI{}}, nil } region := req.QueryStringParameters["region"] @@ -347,22 +381,11 @@ func (h *Handler) getRIUtilization(ctx context.Context, req *events.LambdaFuncti return nil, err } - // Apply the session's allowed_accounts scope. Mirrors getRIExchangeHistory. - if allowed, aErr := h.getAllowedAccounts(ctx, session); aErr != nil { - return nil, fmt.Errorf("failed to get allowed accounts: %w", aErr) - } else if !auth.IsUnrestrictedAccess(allowed) { - resolveAccount := h.resolveAWSCloudAccountID - if h.reshapeAccountResolver != nil { - resolveAccount = h.reshapeAccountResolver - } - cloudAccountID, aErr := resolveAccount(ctx) - if aErr != nil { - return nil, fmt.Errorf("failed to resolve cloud account scope for RI utilization: %w", aErr) - } - nameByID := h.resolveAccountNamesByID(ctx) - if !auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]) { - return &RIUtilizationResponse{Utilization: []recommendations.RIUtilization{}}, nil - } + // Apply the session's allowed_accounts scope. + if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { + return nil, sErr + } else if !inScope { + return &RIUtilizationResponse{Utilization: []recommendations.RIUtilization{}}, nil } lookbackDays, err := parseLookbackDaysParam(req.QueryStringParameters) @@ -466,6 +489,33 @@ func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recomme return riInfos, utilInfos } +// reshapeRequestParams groups parsed query parameters for getReshapeRecommendations. +type reshapeRequestParams struct { + threshold float64 + lookbackDays int + region string +} + +// parseReshapeParams parses the threshold, lookback_days, and region query +// parameters for getReshapeRecommendations in a single call, reducing the +// number of error-check branches in the handler to keep it within the +// gocyclo limit. +func parseReshapeParams(params map[string]string) (reshapeRequestParams, error) { + threshold, err := parseThresholdParam(params) + if err != nil { + return reshapeRequestParams{}, err + } + lookbackDays, err := parseLookbackDaysParam(params) + if err != nil { + return reshapeRequestParams{}, err + } + return reshapeRequestParams{ + threshold: threshold, + lookbackDays: lookbackDays, + region: params["region"], + }, nil +} + // getReshapeRecommendations orchestrates fetching convertible RIs + utilization // and returns reshape recommendations. func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { @@ -474,36 +524,19 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam return nil, err } - // Apply the session's allowed_accounts scope. Mirrors getRIExchangeHistory. - if allowed, aErr := h.getAllowedAccounts(ctx, session); aErr != nil { - return nil, fmt.Errorf("failed to get allowed accounts: %w", aErr) - } else if !auth.IsUnrestrictedAccess(allowed) { - resolveAccount := h.resolveAWSCloudAccountID - if h.reshapeAccountResolver != nil { - resolveAccount = h.reshapeAccountResolver - } - cloudAccountID, aErr := resolveAccount(ctx) - if aErr != nil { - return nil, fmt.Errorf("failed to resolve cloud account scope for reshape recommendations: %w", aErr) - } - nameByID := h.resolveAccountNamesByID(ctx) - if !auth.MatchesAccount(allowed, cloudAccountID, nameByID[cloudAccountID]) { - return &ReshapeRecommendationsResponse{Recommendations: []exchange.ReshapeRecommendation{}}, nil - } - } - - threshold, err := parseThresholdParam(req.QueryStringParameters) - if err != nil { - return nil, err + // Apply the session's allowed_accounts scope. + if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { + return nil, sErr + } else if !inScope { + return &ReshapeRecommendationsResponse{Recommendations: []exchange.ReshapeRecommendation{}}, nil } - lookbackDays, err := parseLookbackDaysParam(req.QueryStringParameters) + p, err := parseReshapeParams(req.QueryStringParameters) if err != nil { return nil, err } - region := req.QueryStringParameters["region"] - cfg, err := h.loadAWSConfigWithRegion(ctx, region) + cfg, err := h.loadAWSConfigWithRegion(ctx, p.region) if err != nil { return nil, fmt.Errorf("failed to load AWS config: %w", err) } @@ -513,8 +546,8 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam // unscoped, leaking alternatives from other regions onto the reshape page. // Adopt the resolved region so every downstream consumer sees the same // value the AWS clients are actually talking to. - if region == "" { - region = cfg.Region + if p.region == "" { + p.region = cfg.Region } ec2Client := h.buildReshapeEC2Client(cfg) @@ -524,7 +557,7 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam } recsAdapter := h.buildReshapeRecsClient(cfg) - utilData, err := h.getRIUtilizationCache().getOrFetch(ctx, region, lookbackDays, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, recsAdapter.GetRIUtilization) + utilData, err := h.getRIUtilizationCache().getOrFetch(ctx, p.region, p.lookbackDays, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, recsAdapter.GetRIUtilization) if err != nil { return nil, fmt.Errorf("failed to get RI utilization: %w", err) } @@ -540,19 +573,13 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam // CloudAccount registration hasn't happened yet; a real ListCloudAccounts // error aborts the request instead of silently falling through to an // unscoped query that could match the wrong tenant's recs. - resolveAccount := h.resolveAWSCloudAccountID - if h.reshapeAccountResolver != nil { - // Test injection — bypasses sts.GetCallerIdentity so the - // integration suite runs without AWS credentials. - resolveAccount = h.reshapeAccountResolver - } - cloudAccountID, err := resolveAccount(ctx) + cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) if err != nil { return nil, fmt.Errorf("failed to resolve cloud account scope for reshape: %w", err) } currencyCode := firstNonEmptyCurrency(instances) lookup := purchaseRecLookupFromStore(h.config, cloudAccountID) - recs := exchange.AnalyzeReshapingWithRecs(ctx, riInfos, utilInfos, threshold, region, currencyCode, lookup) + recs := exchange.AnalyzeReshapingWithRecs(ctx, riInfos, utilInfos, p.threshold, p.region, currencyCode, lookup) resp := &ReshapeRecommendationsResponse{Recommendations: recs} h.attachReshapeStaleness(ctx, resp) diff --git a/pkg/exchange/reshape_crossfamily_test.go b/pkg/exchange/reshape_crossfamily_test.go index 7d4eb7f60..30f6d372d 100644 --- a/pkg/exchange/reshape_crossfamily_test.go +++ b/pkg/exchange/reshape_crossfamily_test.go @@ -465,7 +465,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // m6i.xlarge: same "m" prefix (gen jump bonus), exact NF, high confidence. nearPerfect := OfferingOption{ - InstanceType: "m6i.xlarge", + InstanceType: "m6i.xlarge", EffectiveMonthlyCost: 85, // slightly more expensive NormalizationFactor: 8, SavingsAbs: floatPtr(220), @@ -473,7 +473,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // r5.xlarge: different prefix (no family gen bonus), same NF, no confidence. crossFamily := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 80, // same cost as source -- better raw price NormalizationFactor: 8, } @@ -497,13 +497,13 @@ func TestCompositeScore_SameArchOutranksCrossArch(t *testing.T) { } // c6i: same "c" prefix (family gen bonus), Intel x86 (same arch). sameArch := OfferingOption{ - InstanceType: "c6i.xlarge", + InstanceType: "c6i.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } // c6g: same "c" prefix (family gen bonus), Graviton ARM (cross arch). crossArch := OfferingOption{ - InstanceType: "c6g.xlarge", + InstanceType: "c6g.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } @@ -522,14 +522,14 @@ func TestCompositeScore_HighConfidenceOutranksLow(t *testing.T) { MonthlyCost: 70, } highConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(250), RecommendationCount: 4, } lowConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(10), @@ -550,13 +550,13 @@ func TestCompositeScore_AbsentSavingsIsNeutral(t *testing.T) { MonthlyCost: 70, } absentSavings := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: nil, // not supplied } lowConf := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(5), // explicitly low confidence