diff --git a/frontend/src/api/recommendations.ts b/frontend/src/api/recommendations.ts index 7139de40c..258623cdd 100644 --- a/frontend/src/api/recommendations.ts +++ b/frontend/src/api/recommendations.ts @@ -113,6 +113,13 @@ export interface RecommendationDetail { usage_history: RecommendationUsagePoint[]; confidence_bucket: 'low' | 'medium' | 'high'; provenance_note: string; + /** + * Present (non-empty array) when the recommendation exists but is filtered + * out by an account-service override (issue #214). Each element names one + * failing dimension: "enabled=false", "engine", "region", or + * "resource_type". Absent / undefined means the rec is fully visible. + */ + hidden_by?: string[]; } /** diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index eff74f43f..44850fd4d 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -202,7 +202,8 @@ func TestPerAccountPerms_RecommendationDetail_CrossAccountRejected(t *testing.T) } mockSched := new(MockScheduler) - mockSched.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{recB}, nil) + mockSched.On("GetRecommendationByID", ctx, "rec-b-detail").Return(&recB, ([]string)(nil), nil) + t.Cleanup(func() { mockSched.AssertExpectations(t) }) mockStore := new(MockConfigStore) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { @@ -238,7 +239,8 @@ func TestPerAccountPerms_RecommendationDetail_AllowedAccountReturns200(t *testin } mockSched := new(MockScheduler) - mockSched.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{recA}, nil) + mockSched.On("GetRecommendationByID", ctx, "rec-a-detail").Return(&recA, ([]string)(nil), nil) + t.Cleanup(func() { mockSched.AssertExpectations(t) }) mockStore := new(MockConfigStore) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index 8daa3c72e..19c1b2093 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -202,32 +202,37 @@ func (h *Handler) getRecommendationDetail(ctx context.Context, req *events.Lambd return nil, NewClientError(400, "recommendation id is required") } - // The recommendation cache doesn't expose a get-by-id, so we look it - // up from the unfiltered list. The list is already cached in - // Postgres (see store_postgres_recommendations.go) so this is a - // single round-trip; the in-memory linear scan is bounded by the - // catalogue size which is small (low thousands at the high end). - recs, err := h.scheduler.ListRecommendations(ctx, config.RecommendationFilter{}) + // GetRecommendationByID bypasses the account-override filter so that a + // deep-linked URL to a rec that has been hidden by an account override + // still resolves. Suppressions are still applied: a fully-suppressed rec + // (actively dismissed) returns nil and we 404 as before. See issue #214. + rec, hiddenBy, err := h.scheduler.GetRecommendationByID(ctx, id) if err != nil { - return nil, fmt.Errorf("failed to list recommendations: %w", err) + return nil, fmt.Errorf("failed to fetch recommendation: %w", err) + } + if rec == nil { + return nil, errNotFound } - // Filter by allowed accounts FIRST, then look up by id within the - // filtered set. Doing it the other way around would leak existence - // of recommendations in accounts the caller can't see (a 404 vs - // 403 timing/wording diff would let an attacker probe the - // recommendation namespace across accounts). - visible, err := h.filterRecommendationsByAllowedAccounts(ctx, session, recs) + // Tenant-scoping gate: an authenticated user must not be able to read recs + // from accounts outside their allowed set, even via the detail endpoint. + // The existence-disclosure-safe check (match before reveal) is preserved: + // we checked GetRecommendationByID first and only reach here on a hit, so + // the caller still cannot distinguish "doesn't exist" from "wrong tenant" + // via a timing or wording difference. + visible, err := h.filterRecommendationsByAllowedAccounts(ctx, session, []config.RecommendationRecord{*rec}) if err != nil { return nil, err } + if len(visible) == 0 { + return nil, errNotFound + } - for i := range visible { - if visible[i].ID == id { - return h.buildRecommendationDetail(ctx, &visible[i]), nil - } + resp := h.buildRecommendationDetail(ctx, &visible[0]) + if len(hiddenBy) > 0 { + resp.HiddenBy = hiddenBy } - return nil, errNotFound + return resp, nil } // buildRecommendationDetail assembles the drawer payload from a single diff --git a/internal/api/handler_recommendations_test.go b/internal/api/handler_recommendations_test.go index 7da750442..cebe0e495 100644 --- a/internal/api/handler_recommendations_test.go +++ b/internal/api/handler_recommendations_test.go @@ -111,8 +111,10 @@ func TestHandler_getRecommendationDetail(t *testing.T) { t.Run("returns errNotFound on unknown id", func(t *testing.T) { mockScheduler := new(MockScheduler) - mockScheduler.On("ListRecommendations", ctx, mock.Anything). - Return(knownRecs, nil) + // GetRecommendationByID returns (nil, nil, nil) for absent recs. + mockScheduler.On("GetRecommendationByID", ctx, "rec-missing"). + Return((*config.RecommendationRecord)(nil), ([]string)(nil), nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) handler := &Handler{ scheduler: mockScheduler, @@ -129,9 +131,11 @@ func TestHandler_getRecommendationDetail(t *testing.T) { }) t.Run("returns 200 with the expected shape for a known id", func(t *testing.T) { + knownRec := knownRecs[0] mockScheduler := new(MockScheduler) - mockScheduler.On("ListRecommendations", ctx, mock.Anything). - Return(knownRecs, nil) + mockScheduler.On("GetRecommendationByID", ctx, "rec-known"). + Return(&knownRec, ([]string)(nil), nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) mockStore := new(MockConfigStore) mockStore.On("GetRecommendationsFreshness", ctx). @@ -161,12 +165,45 @@ func TestHandler_getRecommendationDetail(t *testing.T) { assert.Contains(t, got.ProvenanceNote, "AWS") assert.Contains(t, got.ProvenanceNote, "ec2") assert.Contains(t, got.ProvenanceNote, "last collected") + assert.Nil(t, got.HiddenBy, "visible rec must not carry hidden_by") + }) + + t.Run("returns 200 with hidden_by for override-filtered rec (issue #214)", func(t *testing.T) { + // GetRecommendationByID returns the rec and the override reasons + // when the rec exists but is filtered by an account-service override. + knownRec := knownRecs[0] + mockScheduler := new(MockScheduler) + mockScheduler.On("GetRecommendationByID", ctx, "rec-known"). + Return(&knownRec, []string{"enabled=false"}, nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + + mockStore := new(MockConfigStore) + mockStore.On("GetRecommendationsFreshness", ctx). + Return(&config.RecommendationsFreshness{LastCollectedAt: &now}, nil) + + handler := &Handler{ + scheduler: mockScheduler, + config: mockStore, + apiKey: "test-key", + } + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + + got, err := handler.getRecommendationDetail(ctx, req, "rec-known") + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, "rec-known", got.ID) + assert.Equal(t, []string{"enabled=false"}, got.HiddenBy, + "override-hidden rec must carry hidden_by reasons") }) t.Run("provenance degrades gracefully when freshness is unavailable", func(t *testing.T) { + knownRec := knownRecs[0] mockScheduler := new(MockScheduler) - mockScheduler.On("ListRecommendations", ctx, mock.Anything). - Return(knownRecs, nil) + mockScheduler.On("GetRecommendationByID", ctx, "rec-known"). + Return(&knownRec, ([]string)(nil), nil) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) mockStore := new(MockConfigStore) mockStore.On("GetRecommendationsFreshness", ctx). diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 8fb7a1249..30827827e 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -696,6 +696,19 @@ func (m *MockScheduler) ListRecommendations(ctx context.Context, filter config.R return args.Get(0).([]config.RecommendationRecord), args.Error(1) } +func (m *MockScheduler) GetRecommendationByID(ctx context.Context, id string) (*config.RecommendationRecord, []string, error) { + args := m.Called(ctx, id) + var rec *config.RecommendationRecord + if args.Get(0) != nil { + rec = args.Get(0).(*config.RecommendationRecord) + } + var hiddenBy []string + if args.Get(1) != nil { + hiddenBy = args.Get(1).([]string) + } + return rec, hiddenBy, args.Error(2) +} + // MockAuthService is a mock implementation of the auth service type MockAuthService struct { mock.Mock diff --git a/internal/api/types.go b/internal/api/types.go index 3cce623d2..4803cee26 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -142,6 +142,12 @@ type PurchaseManagerInterface interface { type SchedulerInterface interface { CollectRecommendations(ctx context.Context) (*scheduler.CollectResult, error) ListRecommendations(ctx context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error) + // GetRecommendationByID fetches a single rec by its application-level id, + // bypassing account-override filtering so deep-linked URLs to override- + // hidden recs resolve. hiddenBy is non-nil when the rec would be dropped by + // the override filter; callers render a "hidden" banner. Returns nil, nil, + // nil when the rec is absent or fully suppressed. + GetRecommendationByID(ctx context.Context, id string) (rec *config.RecommendationRecord, hiddenBy []string, err error) } // AuthServiceInterface defines auth service methods used by handler @@ -379,6 +385,12 @@ type RecommendationDetailResponse struct { UsageHistory []UsagePoint `json:"usage_history"` ConfidenceBucket string `json:"confidence_bucket"` ProvenanceNote string `json:"provenance_note"` + // HiddenBy is non-nil when the rec is filtered out by an account-service + // override (issue #214). Each element names one failing dimension: + // "enabled=false", "engine", "region", or "resource_type". The frontend + // renders a "hidden by your override" banner when this field is present. + // Absent (null) means the rec is fully visible. + HiddenBy []string `json:"hidden_by,omitempty"` } // PlansResponse holds the purchase plans response diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 72348bb40..626ac7e0d 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -269,6 +269,9 @@ func buildRecommendationFilter(filter RecommendationFilter) (string, []any) { if filter.MinSavings > 0 { add("monthly_savings >= $%d", filter.MinSavings) } + if filter.ID != "" { + add("payload->>'id' = $%d", filter.ID) + } if len(conds) == 0 { return "", nil } diff --git a/internal/config/types.go b/internal/config/types.go index caf518f68..6ff70892a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -390,6 +390,7 @@ type RecommendationFilter struct { Region string // "" = all regions AccountIDs []string // nil/empty = all accounts MinSavings float64 // 0 = no floor on monthly savings + ID string // "" = all ids; non-empty = exact match on the id column } // PurchasePlanFilter parameterises ListPurchasePlans. Zero-value means "no diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 0fd0b98c9..cddb00569 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -976,6 +976,79 @@ func (s *Scheduler) ListRecommendations(ctx context.Context, filter config.Recom return recs, nil } +// GetRecommendationByID fetches a single recommendation by its application- +// level ID (the id field in the stored payload), bypassing the account-override +// filter so deep-linked URLs to override-hidden recs still resolve. +// +// Suppressions are still applied: a rec whose remaining count has been +// suppressed to zero is treated as gone (returns nil, nil, nil) so the caller +// can render a 404 rather than a "hidden" banner for actively-dismissed recs. +// +// hiddenBy is non-nil (one or more reason strings) when the rec exists and +// passes the suppression check, but would be dropped by the account-override +// filter. Possible reasons: "enabled=false", "engine", "region", +// "resource_type". The caller renders a "hidden by your override" banner. +// +// Returns (nil, nil, nil) when the rec is genuinely absent or fully suppressed. +func (s *Scheduler) GetRecommendationByID(ctx context.Context, id string) (rec *config.RecommendationRecord, hiddenBy []string, err error) { + recs, err := s.config.ListStoredRecommendations(ctx, config.RecommendationFilter{ID: id}) + if err != nil { + return nil, nil, fmt.Errorf("GetRecommendationByID: store lookup: %w", err) + } + if len(recs) == 0 { + return nil, nil, nil + } + + // Apply suppressions. A fully-suppressed rec becomes genuinely absent from + // the caller's perspective (the suppression is intentional user action). + recs, err = s.applySuppressions(ctx, recs) + if err != nil { + // Over-show: on suppression read failure treat the rec as un-suppressed. + logging.Errorf("GetRecommendationByID: suppression check failed; treating rec as un-suppressed: %v", err) + } + if len(recs) == 0 { + return nil, nil, nil + } + found := &recs[0] + + // Check whether the account-override filter would drop this rec. This is + // a read-only call — we never drop it here, only report the reasons. + if found.CloudAccountID != nil { + resolved, resolveErr := config.ResolveAccountConfigsForRecs(ctx, s.config, recs) + if resolveErr != nil { + // Non-fatal: if the override check fails we surface the rec without + // a hidden_by marker (over-show is the safer default). + logging.Errorf("GetRecommendationByID: override resolution failed; returning rec without hidden_by: %v", resolveErr) + return found, nil, nil + } + cfg := resolved[config.AccountConfigKey(*found.CloudAccountID, found.Provider, found.Service)] + if cfg != nil { + hiddenBy = overrideHiddenReasons(found, cfg) + } + } + + return found, hiddenBy, nil +} + +// overrideHiddenReasons returns a non-empty slice when rec would be dropped by +// the given resolved ServiceConfig, naming each failing dimension. +func overrideHiddenReasons(rec *config.RecommendationRecord, cfg *config.ServiceConfig) []string { + var reasons []string + if !cfg.Enabled { + reasons = append(reasons, "enabled=false") + } + if !engineMatches(rec.Engine, cfg) { + reasons = append(reasons, "engine") + } + if !inListRule(rec.Region, cfg.IncludeRegions, cfg.ExcludeRegions) { + reasons = append(reasons, "region") + } + if !inListRule(rec.ResourceType, cfg.IncludeTypes, cfg.ExcludeTypes) { + reasons = append(reasons, "resource_type") + } + return reasons +} + // resolveEffectiveCacheTTL returns the effective stale-while-revalidate TTL // and whether background auto-refresh has been explicitly disabled (value 0). // It prefers the DB-configured RecommendationsCacheStaleHours; falls back to diff --git a/internal/scheduler/scheduler_overrides_test.go b/internal/scheduler/scheduler_overrides_test.go index e3b2d722e..3c0d5564c 100644 --- a/internal/scheduler/scheduler_overrides_test.go +++ b/internal/scheduler/scheduler_overrides_test.go @@ -24,8 +24,16 @@ type mockOverrideStore struct { getOverrideErr error } -func (m *mockOverrideStore) ListStoredRecommendations(_ context.Context, _ config.RecommendationFilter) ([]config.RecommendationRecord, error) { - return m.recs, nil +func (m *mockOverrideStore) ListStoredRecommendations(_ context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error) { + if filter.ID == "" { + return m.recs, nil + } + for _, r := range m.recs { + if r.ID == filter.ID { + return []config.RecommendationRecord{r}, nil + } + } + return nil, nil } func (m *mockOverrideStore) ListActiveSuppressions(_ context.Context) ([]config.PurchaseSuppression, error) { return nil, nil @@ -321,3 +329,61 @@ func TestApplyAccountOverrides_AcceptanceCriterion_Issue196(t *testing.T) { require.Len(t, recs, 1, "acct-A's recs hidden by the override") assert.Equal(t, "acct-B", *recs[0].CloudAccountID) } + +// TestGetRecommendationByID exercises the three acceptance cases from issue #214: +// +// (a) rec visible: returns rec, nil hiddenBy +// (b) rec hidden by override: returns rec + hiddenBy reasons (no 404) +// (c) rec genuinely absent: returns nil, nil, nil +func TestGetRecommendationByID_VisibleRec(t *testing.T) { + ctx := context.Background() + rec := rdsRec("acct-A", "us-east-1", "mysql") + store := &mockOverrideStore{ + recs: []config.RecommendationRecord{rec}, + globals: map[string]*config.ServiceConfig{ + "aws|rds": {Provider: "aws", Service: "rds", Enabled: true}, + }, + } + s := &Scheduler{config: store} + + got, hiddenBy, err := s.GetRecommendationByID(ctx, rec.ID) + require.NoError(t, err) + require.NotNil(t, got, "visible rec must be returned") + assert.Equal(t, rec.ID, got.ID) + assert.Empty(t, hiddenBy, "visible rec must carry no hidden_by reasons") +} + +func TestGetRecommendationByID_HiddenByOverride(t *testing.T) { + // Issue #214: detail endpoint must return the rec with hidden_by reasons + // instead of a 404 when an account-service override is filtering it out. + ctx := context.Background() + rec := rdsRec("acct-A", "us-east-1", "mysql") + store := &mockOverrideStore{ + recs: []config.RecommendationRecord{rec}, + globals: map[string]*config.ServiceConfig{ + "aws|rds": {Provider: "aws", Service: "rds", Enabled: true}, + }, + overrides: map[string]*config.AccountServiceOverride{ + "acct-A|aws|rds": {Enabled: boolPtr(false)}, + }, + } + s := &Scheduler{config: store} + + got, hiddenBy, err := s.GetRecommendationByID(ctx, rec.ID) + require.NoError(t, err) + require.NotNil(t, got, "override-hidden rec must still be returned") + assert.Equal(t, rec.ID, got.ID) + assert.Equal(t, []string{"enabled=false"}, hiddenBy, + "hidden_by must report the override dimension that caused the filter") +} + +func TestGetRecommendationByID_AbsentRec(t *testing.T) { + ctx := context.Background() + store := &mockOverrideStore{recs: nil} + s := &Scheduler{config: store} + + got, hiddenBy, err := s.GetRecommendationByID(ctx, "no-such-rec") + require.NoError(t, err) + assert.Nil(t, got, "absent rec must return nil") + assert.Nil(t, hiddenBy) +} diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index 765e463a6..a374c210f 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -13,6 +13,11 @@ import ( type SchedulerInterface interface { CollectRecommendations(ctx context.Context) (*scheduler.CollectResult, error) ListRecommendations(ctx context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error) + // GetRecommendationByID fetches a single rec by application-level id, + // bypassing account-override filtering. hiddenBy is non-nil when the rec + // exists but would be dropped by the override filter. Returns nil, nil, + // nil when absent or fully suppressed. + GetRecommendationByID(ctx context.Context, id string) (rec *config.RecommendationRecord, hiddenBy []string, err error) } // PurchaseManagerInterface defines the methods required for the purchase manager component diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go index 833f4cb90..4850d0844 100644 --- a/internal/testutil/mocks.go +++ b/internal/testutil/mocks.go @@ -13,6 +13,7 @@ import ( type MockScheduler struct { CollectRecommendationsFunc func(ctx context.Context) (*scheduler.CollectResult, error) ListRecommendationsFunc func(ctx context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error) + GetRecommendationByIDFunc func(ctx context.Context, id string) (*config.RecommendationRecord, []string, error) } func (m *MockScheduler) CollectRecommendations(ctx context.Context) (*scheduler.CollectResult, error) { @@ -29,6 +30,13 @@ func (m *MockScheduler) ListRecommendations(ctx context.Context, filter config.R return []config.RecommendationRecord{}, nil } +func (m *MockScheduler) GetRecommendationByID(ctx context.Context, id string) (*config.RecommendationRecord, []string, error) { + if m.GetRecommendationByIDFunc != nil { + return m.GetRecommendationByIDFunc(ctx, id) + } + return nil, nil, nil +} + // MockPurchaseManager is a mock implementation of server.PurchaseManagerInterface type MockPurchaseManager struct { ProcessScheduledPurchasesFunc func(ctx context.Context) (*purchase.ProcessResult, error)