Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions internal/api/handler_recommendations.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
97 changes: 97 additions & 0 deletions internal/api/handler_recommendations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading