diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 6c46b260d..7ee582f30 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1298,6 +1298,50 @@ func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Sessi return allowed.Allows(cloudAccountID, nameByID[cloudAccountID]), nil } +// requireReshapeAccountScope enforces the session's allowed_accounts scope +// for executeExchange (issue #93). The three sibling read endpoints +// (listConvertibleRIs, getRIUtilization, getReshapeRecommendations) all call +// reshapeCloudAccountInScope and return an empty list when the deployment's +// cloud account falls outside the session's scope; executeExchange is the +// one mutating, financially irreversible endpoint among the four, so it must +// fail closed with a 403 ClientError instead of an empty response. This is +// independent of the requirePermissionConstraints check executeExchange also +// runs, which evaluates the granting permission's own Constraints (SEC-01, +// issue #1141): a permission with no AccountIDs constraint satisfies that +// check unconditionally, so it does not substitute for the session-level +// scope enforced here. Split out as its own function (rather than inlined in +// executeExchange) to keep that handler's cyclomatic complexity under the +// project's gate. +func (h *Handler) requireReshapeAccountScope(ctx context.Context, session *Session) error { + inScope, err := h.reshapeCloudAccountInScope(ctx, session) + if err != nil { + return err + } + if !inScope { + return NewClientError(403, "permission denied: this deployment's cloud account is not covered by your session's allowed accounts") + } + return nil +} + +// resolveReshapeCloudAccountIDOrUnattributed resolves the deployment's cloud +// account ID, substituting the unattributedAccountConstraint sentinel when +// the deployment maps to no registered account. Used by executeExchange to +// build the AccountIDs dimension of its permission-constraints check +// (SEC-01, issue #1141): an AccountIDs-constrained permission must deny +// rather than silently matching an empty/absent constraint. Split out from +// executeExchange to keep that handler's cyclomatic complexity under the +// project's gate. +func (h *Handler) resolveReshapeCloudAccountIDOrUnattributed(ctx context.Context) (string, error) { + cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) + if err != nil { + return "", err + } + if cloudAccountID == "" { + return unattributedAccountConstraint, nil + } + return 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). @@ -1774,6 +1818,13 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio return nil, err } + // Apply the session's allowed_accounts scope (issue #93). See + // requireReshapeAccountScope for what this guards against. + err = h.requireReshapeAccountScope(ctx, session) + if err != nil { + return nil, err + } + var body ExchangeExecuteRequestBody err = json.Unmarshal([]byte(req.Body), &body) if err != nil { @@ -1802,13 +1853,10 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio // max_payment_due_usd guardrail, which ExecuteExchange independently // enforces against the actual quoted payment due. maxPayment, _ := maxRat.Float64() - cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) + cloudAccountID, err := h.resolveReshapeCloudAccountIDOrUnattributed(ctx) if err != nil { return nil, fmt.Errorf("failed to resolve cloud account scope: %w", err) } - if cloudAccountID == "" { - cloudAccountID = unattributedAccountConstraint - } err = h.requirePermissionConstraints(ctx, session, "ri-exchange", []auth.PermissionConstraints{{ AccountIDs: []string{cloudAccountID}, Providers: []string{string(common.ProviderAWS)}, diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index f281ec2a5..85734f4ae 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1551,6 +1551,10 @@ func TestExecuteExchange_PermissionConstraintsDenied(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // The session-scope gate (reshapeCloudAccountInScope, issue #93) runs + // before the permission-constraints check this test targets; unrestrict + // it so the request reaches the constraints check under test. + allowAnyAccountScope(mockAuth) mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "ri-exchange", mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { if len(sets) != 1 { @@ -1598,6 +1602,10 @@ func TestExecuteExchange_AccountResolutionErrorFailsClosed(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) short-circuits without needing the resolver, and this + // test's own resolver failure is the one that fails closed below. + allowAnyAccountScope(mockAuth) h := &Handler{ auth: mockAuth, @@ -1630,6 +1638,10 @@ func TestExecuteExchange_UnattributedAccountStillConstrained(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) is satisfied without consulting the resolver, leaving this + // test's own single resolver call (for the constraint set below) intact. + allowAnyAccountScope(mockAuth) mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "ri-exchange", mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { return len(sets) == 1 && @@ -1652,6 +1664,68 @@ func TestExecuteExchange_UnattributedAccountStillConstrained(t *testing.T) { assert.Equal(t, 403, ce.code) } +// TestExecuteExchange_SessionAccountScopeMismatchReturns403 is the regression +// test for issue #93: listConvertibleRIs, getRIUtilization and +// getReshapeRecommendations each call reshapeCloudAccountInScope and return an +// empty list when the deployment's registered AWS cloud account falls outside +// the session's allowed_accounts, but executeExchange did not, even though it +// is the only one of the four RI-exchange endpoints that is financially +// irreversible. +// +// This replays the issue's exact scenario: a session scoped to +// allowed_accounts=[acct-A] holding an execute:ri-exchange grant with NO +// AccountIDs constraint (so the SEC-01/#1141 requirePermissionConstraints +// check below would allow it unconditionally) attempts to execute against a +// deployment whose ambient AWS account resolves to acct-B. Before the fix +// this reached requirePermissionConstraints and beyond; it must instead be +// refused with 403 by the session-scope gate before either the constraints +// check or the AWS call. +func TestExecuteExchange_SessionAccountScopeMismatchReturns403(t *testing.T) { + ctx := context.Background() + const allowedAccountID = "11111111-2222-3333-4444-555555555555" // acct-A: in the session's scope + const deploymentAccountID = "99999999-8888-7777-6666-555555555555" // acct-B: the deployment's ambient account + + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + userSession := &Session{ + UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee", + Email: "exchanger@example.com", + } + mockAuth.On("ValidateSession", ctx, "exchange-token").Return(userSession, nil) + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "ri-exchange").Return(true, nil) + // The session is restricted to acct-A only. + mockAuth.On("GetAllowedAccountsAPI", ctx, userSession.UserID).Return([]string{allowedAccountID}, nil) + // No HasPermissionForConstraintsAPI expectation: the permission carries no + // AccountIDs constraint (matching the issue's failure scenario) and would + // satisfy that check unconditionally, so it must never be reached -- the + // session-scope gate above must refuse the request first. + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return nil, nil + } + + h := &Handler{ + auth: mockAuth, + config: mockStore, + reshapeAccountResolver: func(_ context.Context) (string, error) { + return deploymentAccountID, nil + }, + } + _, err := h.executeExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer exchange-token"}, + Body: `{"ri_ids":["ri-123"],"target_offering_id":"off-1","target_count":1,"max_payment_due_usd":"250.50","region":"eu-central-1"}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "allowed accounts") + mockAuth.AssertNotCalled(t, "HasPermissionForConstraintsAPI", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + // 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 diff --git a/internal/api/ri_exchange_idempotency_guards_test.go b/internal/api/ri_exchange_idempotency_guards_test.go index 8918a5dac..5a57829d2 100644 --- a/internal/api/ri_exchange_idempotency_guards_test.go +++ b/internal/api/ri_exchange_idempotency_guards_test.go @@ -87,6 +87,10 @@ func TestExecuteExchange_InvalidBodyNeverClaims(t *testing.T) { 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) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93), which now runs before body validation, does not block the + // request before reaching the validation failure this test targets. + allowAnyAccountScope(mockAuth) // Maybe(): the validation below must return before this is reached. It is // registered anyway so that a regression letting the request through fails // on the assertions rather than panicking on an unexpected mock call. diff --git a/internal/api/ri_exchange_idempotency_test.go b/internal/api/ri_exchange_idempotency_test.go index b50152a6d..ad0efd192 100644 --- a/internal/api/ri_exchange_idempotency_test.go +++ b/internal/api/ri_exchange_idempotency_test.go @@ -226,6 +226,9 @@ func TestExecuteExchange_DuplicateSubmitRefusedBeforeAWS(t *testing.T) { 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) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) does not interfere with the claim-ledger behavior under test. + allowAnyAccountScope(mockAuth) mockAuth.On("HasPermissionForConstraintsAPI", ctx, "user-1", "execute", "ri-exchange", mock.Anything).Return(true, nil) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) diff --git a/internal/api/router_660_permission_flips_test.go b/internal/api/router_660_permission_flips_test.go index 3c075921f..8cc4f32e6 100644 --- a/internal/api/router_660_permission_flips_test.go +++ b/internal/api/router_660_permission_flips_test.go @@ -457,6 +457,9 @@ func TestExecuteExchange_PermissionGate(t *testing.T) { // AWS SDK call will fail with a non-403 (connection refused / 500). t.Run("user with execute:ri-exchange clears 403 gate", func(t *testing.T) { mockAuth := authForUserWith(ctx, t, userID, "execute", "ri-exchange", true) + // Unrestricted session scope so the reshapeCloudAccountInScope gate + // (issue #93) does not itself produce the 403 this sub-test must not see. + allowAnyAccountScope(mockAuth) h := &Handler{auth: mockAuth} body := `{"ri_ids":["ri-abc"],"targets":[{"offering_id":"of-1"}],"max_payment_due_usd":"1000"}` _, err := h.executeExchange(ctx, reqWithBearerAndBody("user-token", body))