From baae966a030e91b7a9bb9e639ef377a4ee6b7903 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 23:52:27 +0100 Subject: [PATCH] test(api): answer region-scoped Azure exchange tests from the real matcher newAzureRegionScopedHandler hand-wrote auth.matchAllRegionsConstraint's all-match rule and returned its own verdict as the authorization answer, so the region-denial tests stayed green with the production matcher regressed to containsAny's any-match rule. Grant execute:ri-exchange constrained to eastus through grantPermissions instead, so auth.PermissionsAllowForConstraintSets decides on the constraint set the handler actually built. The submitted set is recovered from the mock's recorded calls, and a positive control proves the same grant allows an exchange that stays inside eastus. Closes #208 Co-Authored-By: Claude Opus 5.5 --- .../api/handler_ri_exchange_azure_test.go | 119 ++++++++++++------ 1 file changed, 83 insertions(+), 36 deletions(-) diff --git a/internal/api/handler_ri_exchange_azure_test.go b/internal/api/handler_ri_exchange_azure_test.go index e8f084f32..4a78a00d7 100644 --- a/internal/api/handler_ri_exchange_azure_test.go +++ b/internal/api/handler_ri_exchange_azure_test.go @@ -1898,38 +1898,33 @@ func TestRequireAzureSourceOwnership_DenialsAreIndistinguishable(t *testing.T) { // --- 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. +// execute:ri-exchange constrained to exactly one region, eastus. The grant goes +// through grantPermissions, so the constraint check is answered by +// auth.PermissionsAllowForConstraintSets: the production matcher (including +// matchAllRegionsConstraint's all-match rule) evaluates the constraint set the +// handler actually built. The test states only the permission; whether a +// request falls inside it is decided by the code that decides it in +// production. An earlier version hand-wrote the all-match rule here and fed +// its own verdict back as the answer, so these tests stayed green with the +// matcher regressed to containsAny's any-match rule (issue #208). // -// 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 { +// The returned mock records every call, so submittedConstraintSets can still +// recover which regions the handler thought the operation touches. +func newAzureRegionScopedHandler(t *testing.T, ctx context.Context, opsClient azureExchangeClient) (*Handler, *MockAuthService) { 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) }) + mockAuth.grantPermissions([]auth.Permission{{ + Action: auth.ActionExecute, + Resource: auth.ResourceRIExchange, + Constraints: &auth.PermissionConstraints{Regions: []string{"eastus"}}, + }}) + // grantPermissions registers the verb check as optional; every test built + // on this helper reaches it, so keep requiring it as the old helper did. + t.Cleanup(func() { + mockAuth.AssertExpectations(t) + mockAuth.AssertCalled(t, "HasPermissionAPI", ctx, "user-1", auth.ActionExecute, auth.ResourceRIExchange) + }) mockStore := &MockConfigStore{} mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, _, _ string) (*config.CloudAccount, error) { @@ -1940,7 +1935,28 @@ func newAzureRegionScopedHandler(t *testing.T, ctx context.Context, opsClient az auth: mockAuth, config: mockStore, azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + }, mockAuth +} + +// submittedConstraintSets returns the constraint sets the handler passed to +// HasPermissionForConstraintsAPI, failing the test unless it made exactly one +// such call, for user-1's execute:ri-exchange. +func submittedConstraintSets(t *testing.T, m *MockAuthService) []auth.PermissionConstraints { + t.Helper() + var submitted [][]auth.PermissionConstraints + for _, call := range m.Calls { + if call.Method != "HasPermissionForConstraintsAPI" { + continue + } + assert.Equal(t, "user-1", call.Arguments.String(1)) + assert.Equal(t, auth.ActionExecute, call.Arguments.String(2)) + assert.Equal(t, auth.ResourceRIExchange, call.Arguments.String(3)) + sets, ok := call.Arguments.Get(4).([]auth.PermissionConstraints) + require.True(t, ok, "constraint sets argument has type %T", call.Arguments.Get(4)) + submitted = append(submitted, sets) } + require.Len(t, submitted, 1, "the handler must make exactly one constraint check") + return submitted[0] } // foreignRegionSourceBody names a source reservation the caller's own @@ -1978,15 +1994,13 @@ const foreignRegionSourceBody = `{ // 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) + h, mockAuth := newAzureRegionScopedHandler(t, ctx, opsClient) _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ Headers: map[string]string{"authorization": "Bearer tok"}, Body: foreignRegionSourceBody, @@ -1996,6 +2010,7 @@ func TestExecuteAzureExchange_SourceRegionOutsidePermissionRefused(t *testing.T) ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 403, ce.code) + captured := submittedConstraintSets(t, mockAuth) 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") @@ -2003,6 +2018,40 @@ func TestExecuteAzureExchange_SourceRegionOutsidePermissionRefused(t *testing.T) opsClient.AssertNotCalled(t, "ExecuteExchange", mock.Anything, mock.Anything) } +// TestExecuteAzureExchange_RegionScopedCallerAllowedInsideRegion is the +// positive control for the region-scoped denials around it: the same eastus-only +// grant must ALLOW an exchange whose source and target both sit in eastus. +// Without it a grant that denied everything would satisfy every denial test +// vacuously, and so would a matcher that refused any non-empty Regions list. +func TestExecuteAzureExchange_RegionScopedCallerAllowedInsideRegion(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: "eastus", Quantity: 1}, + }, nil) + opsClient.On("CalculateExchange", ctx, mock.Anything, mock.Anything).Return( + &azurecompute.ExchangePreview{SessionID: "sess-east", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"}, + []azurecompute.CompatibleOffering{}, nil, + ) + opsClient.On("ExecuteExchange", mock.Anything, "sess-east").Return( + &azurecompute.ExchangeResult{SessionID: "sess-east", Status: "Succeeded"}, nil, + ) + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h, mockAuth := newAzureRegionScopedHandler(t, ctx, opsClient) + resp, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + Body: validAzureExecuteBody, // sources: res-1, targets: eastus + }) + + require.NoError(t, err, "an eastus-only caller must be allowed an exchange that stays inside eastus") + require.NotNil(t, resp) + captured := submittedConstraintSets(t, mockAuth) + require.Len(t, captured, 1) + assert.Equal(t, []string{"eastus"}, captured[0].Regions) +} + // TestExecuteAzureExchange_UnreportedSourceRegionRefused covers the // empty-region trap. ExchangeableReservation.Region "may be empty for // reservations with AppliedScopeType == Shared", so the fold must not simply @@ -2012,8 +2061,6 @@ func TestExecuteAzureExchange_SourceRegionOutsidePermissionRefused(t *testing.T) // 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. @@ -2021,7 +2068,7 @@ func TestExecuteAzureExchange_UnreportedSourceRegionRefused(t *testing.T) { }, nil) t.Cleanup(func() { opsClient.AssertExpectations(t) }) - h := newAzureRegionScopedHandler(t, ctx, opsClient, "eastus", &captured) + h, mockAuth := newAzureRegionScopedHandler(t, ctx, opsClient) _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ Headers: map[string]string{"authorization": "Bearer tok"}, Body: validAzureExecuteBody, // sources: res-1, targets: eastus @@ -2031,6 +2078,7 @@ func TestExecuteAzureExchange_UnreportedSourceRegionRefused(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 403, ce.code) + captured := submittedConstraintSets(t, mockAuth) 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") @@ -2111,12 +2159,11 @@ func TestExecuteAzureExchange_UnownedSourceDenialsAreIndistinguishable(t *testin } 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) + h, _ := newAzureRegionScopedHandler(t, ctx, opsClient) _, err := h.executeAzureExchange(ctx, &events.LambdaFunctionURLRequest{ Headers: map[string]string{"authorization": "Bearer tok"}, Body: bodyFor(reservationID),