diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index e41a538fa..1cd3ce8b2 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -100,6 +100,15 @@ func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunc return nil, fmt.Errorf("failed to get recommendations: %w", err) } + // Issue #1409: drop recs for providers disabled in Admin > General Settings. + // Runs for ALL users regardless of view:config permission, fixing the + // regression where non-admin users saw all providers' recs because the + // client-side filter silently fell through when GET /api/config returned 403. + recommendations, err = h.filterRecommendationsByEnabledProviders(ctx, recommendations) + if err != nil { + return nil, err + } + // Filter by allowed accounts if the user has restricted access recommendations, err = h.filterRecommendationsByAllowedAccounts(ctx, session, recommendations) if err != nil { @@ -148,6 +157,43 @@ func (h *Handler) filterRecommendationsByAllowedAccounts(ctx context.Context, se return filtered, nil } +// filterRecommendationsByEnabledProviders removes recommendations for providers +// that are absent from the global config's EnabledProviders list. +// +// Semantics: +// - Empty EnabledProviders (the admin-facing default): permissive — all recs +// pass through unchanged. This preserves backward compatibility when the +// setting has never been configured. +// - Non-empty EnabledProviders: only recs whose Provider field matches an +// entry in the list pass through. +// - Config read error: fail-loud — the error is returned to the caller so a +// broken config layer cannot silently bypass the enabled-providers gate. +// - Nil config store (only possible in unit tests that omit config setup): +// skip filtering rather than panic. +func (h *Handler) filterRecommendationsByEnabledProviders(ctx context.Context, recs []config.RecommendationRecord) ([]config.RecommendationRecord, error) { + if h.config == nil { + return recs, nil + } + globalCfg, err := h.config.GetGlobalConfig(ctx) + if err != nil { + return nil, fmt.Errorf("failed to read global config for enabled-providers filter: %w", err) + } + if len(globalCfg.EnabledProviders) == 0 { + return recs, nil // permissive default: no filter configured + } + enabledSet := make(map[string]struct{}, len(globalCfg.EnabledProviders)) + for _, p := range globalCfg.EnabledProviders { + enabledSet[p] = struct{}{} + } + filtered := make([]config.RecommendationRecord, 0, len(recs)) + for i := range recs { + if _, ok := enabledSet[recs[i].Provider]; ok { + filtered = append(filtered, recs[i]) + } + } + return filtered, nil +} + // buildRecommendationsResponse calculates summary statistics and collects // unique regions from a set of recommendations. func buildRecommendationsResponse(recommendations []config.RecommendationRecord) *RecommendationsResponse { diff --git a/internal/api/handler_recommendations_test.go b/internal/api/handler_recommendations_test.go index ea9335813..1d82e3ee8 100644 --- a/internal/api/handler_recommendations_test.go +++ b/internal/api/handler_recommendations_test.go @@ -356,6 +356,103 @@ func TestGetRecommendations_AccountIDFilter(t *testing.T) { } } +// TestGetRecommendations_EnabledProvidersFilter is the regression test for +// issue #1409: when enabled_providers is set in Admin > General Settings, +// the Opportunities list must exclude disabled providers' recs for ALL users, +// not just Administrators (the only group that has view:config permission). +// +// Fail-before: prior to this fix getRecommendations did not consult +// enabled_providers; all providers' recs were returned regardless of the +// setting, and non-admin users saw them all because the client-side filter +// silently fell through when GET /api/config returned 403. +// Pass-after: only enabled providers' recs appear in the response. +func TestGetRecommendations_EnabledProvidersFilter(t *testing.T) { + ctx := context.Background() + + awsRec := config.RecommendationRecord{ + ID: "rec-aws", Provider: "aws", Service: "ec2", Region: "us-east-1", Savings: 100, + } + azureRec := config.RecommendationRecord{ + ID: "rec-azure", Provider: "azure", Service: "vm", Region: "eastus", Savings: 50, + } + gcpRec := config.RecommendationRecord{ + ID: "rec-gcp", Provider: "gcp", Service: "gce", Region: "us-central1", Savings: 75, + } + + t.Run("disabled provider recs are excluded", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", ctx, mock.Anything). + Return([]config.RecommendationRecord{awsRec, azureRec, gcpRec}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", mock.Anything). + Return(&config.GlobalConfig{EnabledProviders: []string{"aws"}}, nil) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{scheduler: mockScheduler, config: mockStore, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + + result, err := handler.getRecommendations(ctx, req, map[string]string{}) + require.NoError(t, err) + require.NotNil(t, result) + + gotIDs := make([]string, len(result.Recommendations)) + for i, r := range result.Recommendations { + gotIDs[i] = r.ID + } + assert.ElementsMatch(t, []string{"rec-aws"}, gotIDs, + "only aws recs should appear when azure and gcp are disabled") + }) + + t.Run("empty enabled_providers passes all recs through (permissive default)", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", ctx, mock.Anything). + Return([]config.RecommendationRecord{awsRec, azureRec}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", mock.Anything). + Return(&config.GlobalConfig{EnabledProviders: nil}, nil) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{scheduler: mockScheduler, config: mockStore, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + + result, err := handler.getRecommendations(ctx, req, map[string]string{}) + require.NoError(t, err) + require.NotNil(t, result) + assert.Len(t, result.Recommendations, 2, + "all recs pass through when no enabled_providers filter is configured") + }) + + t.Run("config read error is surfaced (fail loud)", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", ctx, mock.Anything). + Return([]config.RecommendationRecord{awsRec}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", mock.Anything). + Return((*config.GlobalConfig)(nil), errors.New("db down")) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{scheduler: mockScheduler, config: mockStore, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + + result, err := handler.getRecommendations(ctx, req, map[string]string{}) + require.Error(t, err, "config read error must surface, not silently bypass the filter") + assert.Nil(t, result) + assert.Contains(t, err.Error(), "failed to read global config") + }) +} + // TestBuildRecommendationsResponse_CapacityFields is the #219 regression // test. It replicates the REAL API shape: VCPU/MemoryGB live nested inside // the opaque Details blob (a marshaled common.ComputeDetails), exactly as the