From 3a0c9a842a5e93b5a481f778d13561bc0b1809de Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 07:08:17 +0200 Subject: [PATCH] fix(api): address independent review findings on RI-exchange scoping Follow-up to the executeExchange session-scope fix for issue #93: - Add TestExecuteExchange_SessionAccountScopeMatchReachesConstraintsCheck, the positive-path counterpart to the existing regression test: a session restricted to an allowed_accounts list that includes the deployment's own cloud account must clear the new scope gate and reach the pre-existing SEC-01/#1141 constraints check, proving the fix does not over-deny an in-scope request. - Add TestExecuteExchange_SessionScopeLookupErrorFailsClosed: a failure resolving the session's allowed_accounts must propagate as a plain error, not collapse into a 403 that misreports a lookup failure as an out-of-scope denial. - Document, in the existing regression test, why HasPermissionForConstraintsAPI is deliberately left unstubbed rather than stubbed-and-asserted-not-called. - Note reshapeCloudAccountInScope's doc comment now lists executeExchange (via requireReshapeAccountScope) among its callers. - Trim the requireReshapeAccountScope and resolveReshapeCloudAccountIDOrUnattributed doc comments to their essentials; the issue history and gocyclo rationale live in the PR description instead. --- internal/api/handler_ri_exchange.go | 33 +++------ internal/api/handler_ri_exchange_test.go | 93 +++++++++++++++++++++++- 2 files changed, 103 insertions(+), 23 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 7ee582f30..07a60e34a 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1281,7 +1281,8 @@ func (h *Handler) loadAWSConfigWithRegion(ctx context.Context, region string) (a // (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. +// to eliminate duplicated account-scoping blocks, and by executeExchange (via +// requireReshapeAccountScope) to enforce the same scope on the mutating path. func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Session) (bool, error) { allowed, aErr := h.getAccountScope(ctx, session) if aErr != nil { @@ -1298,20 +1299,11 @@ 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. +// requireReshapeAccountScope enforces the session's allowed_accounts scope for +// executeExchange (issue #93), returning 403 instead of an empty response +// since exchange execution is a mutating, financially irreversible action. +// See the PR description for why this is a separate gate from +// requirePermissionConstraints and why it is its own function. func (h *Handler) requireReshapeAccountScope(ctx context.Context, session *Session) error { inScope, err := h.reshapeCloudAccountInScope(ctx, session) if err != nil { @@ -1324,13 +1316,10 @@ func (h *Handler) requireReshapeAccountScope(ctx context.Context, session *Sessi } // 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. +// account ID, substituting the unattributedAccountConstraint sentinel when the +// deployment maps to no registered account (SEC-01, issue #1141: an +// AccountIDs-constrained permission must deny rather than match an +// empty/absent constraint). func (h *Handler) resolveReshapeCloudAccountIDOrUnattributed(ctx context.Context) (string, error) { cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) if err != nil { diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 85734f4ae..1987b8c6a 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1699,7 +1699,12 @@ func TestExecuteExchange_SessionAccountScopeMismatchReturns403(t *testing.T) { // 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. + // session-scope gate above must refuse the request first. Left + // deliberately unstubbed rather than stubbed-and-asserted-not-called: on + // the pre-fix handler this call happens and testify panics on the + // unstubbed mock, which fails the test just as surely (and for the same + // reason) as an assertion would; the AssertNotCalled below still proves + // the post-fix ordering. mockStore := new(MockConfigStore) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { @@ -1726,6 +1731,92 @@ func TestExecuteExchange_SessionAccountScopeMismatchReturns403(t *testing.T) { mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } +// TestExecuteExchange_SessionAccountScopeMatchReachesConstraintsCheck is the +// positive-path counterpart to TestExecuteExchange_SessionAccountScopeMismatchReturns403: +// a session restricted to allowed_accounts that DOES include the deployment's +// cloud account must clear the new session-scope gate and reach the existing +// SEC-01/#1141 permission-constraints check, proving the fix does not +// over-deny an in-scope request. HasPermissionForConstraintsAPI is stubbed to +// deny rather than mocking the downstream AWS call; the "constraints" (not +// "allowed accounts") substring in the resulting error distinguishes which +// gate produced it. +func TestExecuteExchange_SessionAccountScopeMatchReachesConstraintsCheck(t *testing.T) { + ctx := context.Background() + const deploymentAccountID = "11111111-2222-3333-4444-555555555555" + + 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, but its allowed list includes the + // deployment's own account, so the scope gate must let this through. + mockAuth.On("GetAllowedAccountsAPI", ctx, userSession.UserID).Return([]string{deploymentAccountID}, nil) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "ri-exchange", mock.Anything). + Return(false, nil) + + 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(), "constraints", "must be refused by the constraints check, not the session-scope gate") +} + +// TestExecuteExchange_SessionScopeLookupErrorFailsClosed pins the fail-closed +// behavior of the new session-scope gate itself: a failure resolving the +// session's allowed_accounts (auth store outage, etc.) must propagate as an +// error, not collapse into a 403 that would misreport a lookup failure as an +// out-of-scope denial, and must abort before the constraints check runs. +func TestExecuteExchange_SessionScopeLookupErrorFailsClosed(t *testing.T) { + ctx := context.Background() + + 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) + mockAuth.On("GetAllowedAccountsAPI", ctx, userSession.UserID). + Return(nil, fmt.Errorf("auth store unavailable")) + // No HasPermissionForConstraintsAPI expectation: the scope-lookup failure + // must abort before the constraints check is ever reached. + + h := &Handler{auth: mockAuth} + _, 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) + _, ok := IsClientError(err) + assert.False(t, ok, "a scope-lookup failure must not be reported as a ClientError, got: %v", err) + assert.Contains(t, err.Error(), "failed to get 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