diff --git a/.gitallowed b/.gitallowed index 23d04a569..8769f9bb8 100644 --- a/.gitallowed +++ b/.gitallowed @@ -36,6 +36,9 @@ 111222333444 555666777888 999888777666 +# Descending-step fixture added in #956 tests (store_postgres_pgxmock_test.go, +# handler_analytics_test.go, handler_history_test.go, handler_dashboard_test.go). +999988887777 # UUID-shaped account ID used in handler_accounts_test.go (synthetic, not a # real subscription/account). diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 9068a43f1..38d8fc5ae 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -200,7 +200,7 @@ func (m *mockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return nil, nil } -func (m *mockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]config.PurchaseHistoryRecord, error) { +func (m *mockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { return nil, nil } diff --git a/internal/api/analytics_postgres.go b/internal/api/analytics_postgres.go index 1deed6401..0c9bb57f0 100644 --- a/internal/api/analytics_postgres.go +++ b/internal/api/analytics_postgres.go @@ -4,6 +4,8 @@ package api import ( "context" "fmt" + "sort" + "strings" "time" "github.com/LeanerCloud/CUDly/internal/database" @@ -74,13 +76,78 @@ func dimensionToColumn(dimension string) (string, error) { } } +// accountFilterClause builds the dual-column account WHERE fragment and the +// full positional arg list for the analytics aggregate queries. baseArgs holds +// the fixed leading binds ($1=start, $2=end); the account array binds (if any) +// are appended after them and the returned clause references them by the right +// positions. +// +// purchase_history carries two account identifiers, either of which may be the +// only one populated on a row: account_id (the cloud-provider external number, +// always populated) and cloud_account_id (the cloud_accounts UUID FK, NULL on +// every direct-execute/ambient/pre-000011 row). The top-bar chip emits the +// UUID, so matching only cloud_account_id silently dropped every NULL row +// (issue #701/#498) and matching only account_id dropped UUID-only rows (#866). +// +// The external-id half is grouped per provider so each external number only +// matches rows of its own provider: +// +// (cloud_account_id = ANY($u) +// OR (provider = $p AND account_id = ANY($extsForP)) OR ...) +// +// preserving the (provider, external_id) pairing so a filter for aws/123 never +// pulls azure/123 rows. The "" provider key (legacy raw external number, unknown +// provider) matches account_id with no provider gate. cloud_account_id is +// compared directly (no ::text cast) so the plain cloud_account_id UUID index +// (migration 000011) can back it. Providers are sorted for deterministic SQL. +// Both empty -> "TRUE" (no account filter). +func accountFilterClause(accountUUIDs []string, accountExternalIDsByProvider map[string][]string, baseArgs []any) (clause string, args []any) { + args = baseArgs + var ors []string + if len(accountUUIDs) > 0 { + args = append(args, accountUUIDs) + ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args))) + } + for _, provider := range sortedProviderKeys(accountExternalIDsByProvider) { + exts := accountExternalIDsByProvider[provider] + if len(exts) == 0 { + continue + } + if provider == "" { + args = append(args, exts) + ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args))) + continue + } + args = append(args, provider) + providerArg := len(args) + args = append(args, exts) + ors = append(ors, fmt.Sprintf("(provider = $%d AND account_id = ANY($%d))", providerArg, len(args))) + } + if len(ors) == 0 { + return "TRUE", args + } + return "(" + strings.Join(ors, " OR ") + ")", args +} + +// sortedProviderKeys returns the map keys in ascending order so the generated +// SQL (and its bind-arg ordering) is deterministic across calls and testable. +func sortedProviderKeys(m map[string][]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // QueryHistory aggregates purchase_history rows bucketed by interval. -// accountID == "" means "all accounts accessible to the caller"; scoping -// is enforced upstream in the handler. Returns data points in ascending -// order and a summary covering the full window. +// Empty accountUUIDs AND accountExternalIDsByProvider means "all accounts +// accessible to the caller"; scoping is enforced upstream in the handler. +// Returns data points in ascending order and a summary covering the full window. func (c *PostgresAnalyticsClient) QueryHistory( ctx context.Context, - accountID string, + accountUUIDs []string, + accountExternalIDsByProvider map[string][]string, start, end time.Time, interval string, ) ([]HistoryDataPoint, *HistorySummary, error) { @@ -91,16 +158,12 @@ func (c *PostgresAnalyticsClient) QueryHistory( // Single query grouped by (bucket, service, provider) so we can assemble // both the top-line bucket totals and the by_service / by_provider - // breakdowns without a second trip to the DB. - // - // account_id is the legacy VARCHAR(20) external-account identifier - // (e.g. a 12-digit AWS account number) written by older code paths. - // cloud_account_id is the UUID FK to cloud_accounts added in migration - // 000011 and written by all current purchase flows. The topbar Account - // chip sends the cloud_accounts.id UUID, so we match BOTH columns so - // that rows written by either path are included (issue #701). + // breakdowns without a second trip to the DB. The account predicate is the + // shared dual-column clause (see accountFilterClause). // - // #nosec G201 — `unit` is allowlisted by intervalToTruncUnit above. + // #nosec G201 — `unit` is allowlisted by intervalToTruncUnit above and the + // account clause is parameter-bound (no user input interpolated). + accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{start, end}) query := fmt.Sprintf(` SELECT date_trunc('%s', timestamp) AS bucket, service, @@ -111,12 +174,12 @@ func (c *PostgresAnalyticsClient) QueryHistory( FROM purchase_history WHERE timestamp >= $1 AND timestamp <= $2 - AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3) + AND %s GROUP BY bucket, service, provider ORDER BY bucket ASC - `, unit) + `, unit, accountClause) - rows, err := c.db.Query(ctx, query, start, end, accountID) + rows, err := c.db.Query(ctx, query, args...) if err != nil { return nil, nil, fmt.Errorf("query purchase_history: %w", err) } @@ -184,7 +247,8 @@ func (c *PostgresAnalyticsClient) QueryHistory( // bucket. func (c *PostgresAnalyticsClient) QueryBreakdown( ctx context.Context, - accountID string, + accountUUIDs []string, + accountExternalIDsByProvider map[string][]string, start, end time.Time, dimension string, ) (map[string]BreakdownValue, error) { @@ -193,10 +257,12 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( return nil, err } - // account_id / cloud_account_id dual-column match: see QueryHistory - // comment for rationale (issue #701). + // Dual-column account predicate: see accountFilterClause / QueryHistory for + // rationale (issue #701/#498/#866). // - // #nosec G201 — `column` is allowlisted by dimensionToColumn above. + // #nosec G201 — `column` is allowlisted by dimensionToColumn above and the + // account clause is parameter-bound (no user input interpolated). + accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{start, end}) query := fmt.Sprintf(` SELECT %s AS bucket, SUM(estimated_savings)::float8 AS savings, @@ -205,12 +271,12 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( FROM purchase_history WHERE timestamp >= $1 AND timestamp <= $2 - AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3) + AND %s GROUP BY bucket ORDER BY savings DESC - `, column) + `, column, accountClause) - rows, err := c.db.Query(ctx, query, start, end, accountID) + rows, err := c.db.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("query breakdown: %w", err) } diff --git a/internal/api/analytics_postgres_test.go b/internal/api/analytics_postgres_test.go index e05bef11e..6b4b98cd6 100644 --- a/internal/api/analytics_postgres_test.go +++ b/internal/api/analytics_postgres_test.go @@ -64,13 +64,15 @@ func TestQueryHistory_Success(t *testing.T) { AddRow(bucket1, "rds", "aws", 40.0, 10.0, 1). AddRow(bucket2, "ec2", "aws", 75.0, 0.0, 1) - mock.ExpectQuery(`SELECT date_trunc\('day', timestamp\)`). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "acct-1"). + // External-id-only filter: predicate is (account_id = ANY($3)), bound to the + // single external id. No cloud_account_id half since no UUIDs supplied. + mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(account_id = ANY\(\$3\)\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{"acct-1"}). WillReturnRows(rows) start := time.Date(2026, 4, 20, 0, 0, 0, 0, time.UTC) end := time.Date(2026, 4, 24, 0, 0, 0, 0, time.UTC) - points, summary, err := client.QueryHistory(ctx, "acct-1", start, end, "daily") + points, summary, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"acct-1"}}, start, end, "daily") require.NoError(t, err) require.Len(t, points, 2) @@ -101,16 +103,17 @@ func TestQueryHistory_Success(t *testing.T) { func TestQueryHistory_BadInterval(t *testing.T) { client, _ := newMockAnalyticsClient(t) - _, _, err := client.QueryHistory(context.Background(), "", time.Now(), time.Now(), "yearly") + _, _, err := client.QueryHistory(context.Background(), nil, nil, time.Now(), time.Now(), "yearly") assert.Error(t, err) } func TestQueryHistory_QueryError(t *testing.T) { client, mock := newMockAnalyticsClient(t) + // No account filter: predicate degrades to TRUE, only start/end bound. mock.ExpectQuery(`SELECT date_trunc`). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), ""). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnError(errors.New("db down")) - _, _, err := client.QueryHistory(context.Background(), "", time.Now(), time.Now(), "daily") + _, _, err := client.QueryHistory(context.Background(), nil, nil, time.Now(), time.Now(), "daily") assert.Error(t, err) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -122,10 +125,10 @@ func TestQueryBreakdown_Success(t *testing.T) { AddRow("rds", 100.0, 50.0, 2) mock.ExpectQuery(`SELECT service AS bucket`). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), ""). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnRows(rows) - out, err := client.QueryBreakdown(context.Background(), "", + out, err := client.QueryBreakdown(context.Background(), nil, nil, time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), time.Date(2026, 12, 31, 0, 0, 0, 0, time.UTC), "service") @@ -146,7 +149,7 @@ func TestQueryBreakdown_Success(t *testing.T) { func TestQueryBreakdown_BadDimension(t *testing.T) { client, _ := newMockAnalyticsClient(t) - _, err := client.QueryBreakdown(context.Background(), "", time.Now(), time.Now(), "team") + _, err := client.QueryBreakdown(context.Background(), nil, nil, time.Now(), time.Now(), "team") assert.Error(t, err) } @@ -156,36 +159,39 @@ func TestQueryBreakdown_ZeroTotalYieldsZeroPct(t *testing.T) { AddRow("ec2", 0.0, 0.0, 0) mock.ExpectQuery(`SELECT service AS bucket`). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), ""). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnRows(rows) - out, err := client.QueryBreakdown(context.Background(), "", time.Now(), time.Now(), "service") + out, err := client.QueryBreakdown(context.Background(), nil, nil, time.Now(), time.Now(), "service") require.NoError(t, err) assert.Equal(t, 0.0, out["ec2"].Percentage, "percentage must be 0 when total savings is 0") assert.NoError(t, mock.ExpectationsWereMet()) } -// TestQueryHistory_CloudAccountIDFilter verifies that QueryHistory accepts a -// cloud_accounts UUID as the accountID parameter (issue #701). The WHERE -// clause matches on BOTH account_id (legacy VARCHAR(20) external ID) and -// cloud_account_id::text (UUID FK), so rows written by either code path are -// included. pgxmock validates the SQL shape and arg binding. -func TestQueryHistory_CloudAccountIDFilter(t *testing.T) { +// TestQueryHistory_DualColumnFilter verifies the dual-column account predicate +// (issue #701/#498/#866): when a UUID and its resolved external id (grouped +// under its provider) are both supplied, the WHERE clause ORs cloud_account_id = +// ANY($3) with (provider = $4 AND account_id = ANY($5)), so a row that carries +// only one of the two account representations is still aggregated while the +// external-id half stays provider-scoped. pgxmock validates the SQL shape and +// the array arg binding (cloud_account_id is compared directly, no ::text cast). +func TestQueryHistory_DualColumnFilter(t *testing.T) { client, mock := newMockAnalyticsClient(t) ctx := context.Background() uuid := "aabbccdd-1234-5678-abcd-aabbccddee00" + external := "123456789012" bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}). AddRow(bucket, "ec2", "aws", 50.0, 20.0, 1) - mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). + mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(cloud_account_id = ANY\(\$3\) OR \(provider = \$4 AND account_id = ANY\(\$5\)\)\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{uuid}, "aws", []string{external}). WillReturnRows(rows) start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) - points, summary, err := client.QueryHistory(ctx, uuid, start, end, "daily") + points, summary, err := client.QueryHistory(ctx, []string{uuid}, map[string][]string{"aws": {external}}, start, end, "daily") require.NoError(t, err) require.Len(t, points, 1) assert.InDelta(t, 50.0, points[0].TotalSavings, 1e-9) @@ -194,21 +200,22 @@ func TestQueryHistory_CloudAccountIDFilter(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } -// TestQueryBreakdown_CloudAccountIDFilter verifies that QueryBreakdown accepts -// a UUID as the accountID (issue #701). Mirrors TestQueryHistory_CloudAccountIDFilter. -func TestQueryBreakdown_CloudAccountIDFilter(t *testing.T) { +// TestQueryBreakdown_DualColumnFilter mirrors TestQueryHistory_DualColumnFilter +// for the breakdown aggregate (issue #701/#498/#866). +func TestQueryBreakdown_DualColumnFilter(t *testing.T) { client, mock := newMockAnalyticsClient(t) ctx := context.Background() uuid := "aabbccdd-1234-5678-abcd-aabbccddee00" + external := "123456789012" rows := mock.NewRows([]string{"bucket", "savings", "upfront", "purchases"}). AddRow("rds", 200.0, 80.0, 3) - mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). + mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(cloud_account_id = ANY\(\$3\) OR \(provider = \$4 AND account_id = ANY\(\$5\)\)\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{uuid}, "aws", []string{external}). WillReturnRows(rows) - out, err := client.QueryBreakdown(ctx, uuid, + out, err := client.QueryBreakdown(ctx, []string{uuid}, map[string][]string{"aws": {external}}, time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC), time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC), "service") @@ -217,3 +224,50 @@ func TestQueryBreakdown_CloudAccountIDFilter(t *testing.T) { assert.InDelta(t, 200.0, out["rds"].TotalSavings, 1e-9) assert.NoError(t, mock.ExpectationsWereMet()) } + +// TestQueryHistory_CrossProviderScoped is the analytics regression for issue +// #956 CR finding #1: when the same external id "123" exists under two providers +// the WHERE clause emits a separate provider-gated branch per provider (sorted), +// so an external id only matches rows of its own provider and aws/123 never +// pulls azure/123 rows. The args are bound in sorted-provider order. +func TestQueryHistory_CrossProviderScoped(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + + bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}). + AddRow(bucket, "ec2", "aws", 10.0, 5.0, 1) + + mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(\(provider = \$3 AND account_id = ANY\(\$4\)\) OR \(provider = \$5 AND account_id = ANY\(\$6\)\)\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "aws", []string{"123"}, "azure", []string{"123"}). + WillReturnRows(rows) + + start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) + end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) + _, _, err := client.QueryHistory(ctx, nil, map[string][]string{"aws": {"123"}, "azure": {"123"}}, start, end, "daily") + require.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestQueryHistory_ExternalIDFilter verifies that when only external IDs are +// supplied, the WHERE clause emits account_id = ANY($3) bound to the flat slice, +// so a row that carries only account_id (cloud_account_id NULL) is still matched +// (issue #701/#498/#866). +func TestQueryHistory_ExternalIDFilter(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + + bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}). + AddRow(bucket, "ec2", "aws", 10.0, 5.0, 1) + + mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(account_id = ANY\(\$3\)\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{"123456789012"}). + WillReturnRows(rows) + + start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) + end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) + _, _, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"123456789012"}}, start, end, "daily") + require.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) +} diff --git a/internal/api/handler_analytics.go b/internal/api/handler_analytics.go index 57bba1b7c..a6bfccd08 100644 --- a/internal/api/handler_analytics.go +++ b/internal/api/handler_analytics.go @@ -67,8 +67,13 @@ func (h *Handler) getHistoryAnalytics(ctx context.Context, req *events.LambdaFun return nil, err } + // Resolve the requested account (a top-bar chip UUID, or "" for all) to the + // dual-column filter inputs so rows that carry only the external account_id + // (cloud_account_id NULL) are aggregated (issue #701/#498/#866). + accountUUIDs, accountExternalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, accountID) + // Aggregate history from the analytics client (Postgres-backed). - dataPoints, summary, err := h.analyticsClient.QueryHistory(ctx, accountID, start, end, interval) + dataPoints, summary, err := h.analyticsClient.QueryHistory(ctx, accountUUIDs, accountExternalIDsByProvider, start, end, interval) if err != nil { return nil, fmt.Errorf("failed to query analytics: %w", err) } @@ -109,8 +114,11 @@ func (h *Handler) getHistoryBreakdown(ctx context.Context, req *events.LambdaFun return nil, err } + // Resolve account UUID -> dual-column inputs; see getHistoryAnalytics. + accountUUIDs, accountExternalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, accountID) + // Fetch the breakdown from the analytics client (Postgres-backed). - data, err := h.analyticsClient.QueryBreakdown(ctx, accountID, start, end, dimension) + data, err := h.analyticsClient.QueryBreakdown(ctx, accountUUIDs, accountExternalIDsByProvider, start, end, dimension) if err != nil { return nil, fmt.Errorf("failed to query breakdown: %w", err) } diff --git a/internal/api/handler_analytics_test.go b/internal/api/handler_analytics_test.go index 06b0d720c..b24802405 100644 --- a/internal/api/handler_analytics_test.go +++ b/internal/api/handler_analytics_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -17,8 +18,8 @@ type MockAnalyticsClient struct { mock.Mock } -func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountID string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) { - args := m.Called(ctx, accountID, start, end, interval) +func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) { + args := m.Called(ctx, accountUUIDs, accountExternalIDsByProvider, start, end, interval) if args.Get(0) == nil { return nil, nil, args.Error(2) } @@ -29,14 +30,41 @@ func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountID string return args.Get(0).([]HistoryDataPoint), summary, args.Error(2) } -func (m *MockAnalyticsClient) QueryBreakdown(ctx context.Context, accountID string, start, end time.Time, dimension string) (map[string]BreakdownValue, error) { - args := m.Called(ctx, accountID, start, end, dimension) +func (m *MockAnalyticsClient) QueryBreakdown(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time, dimension string) (map[string]BreakdownValue, error) { + args := m.Called(ctx, accountUUIDs, accountExternalIDsByProvider, start, end, dimension) if args.Get(0) == nil { return nil, args.Error(1) } return args.Get(0).(map[string]BreakdownValue), args.Error(1) } +// TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount is the analytics +// keystone (issue #701/#498/#866): selecting an account by its cloud_accounts +// UUID resolves to (UUID, external_id) and both reach QueryHistory so rows that +// carry only the external account_id (cloud_account_id NULL) are aggregated. +func TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount(t *testing.T) { + ctx := context.Background() + accountUUID := "bbbbbbbb-1111-2222-3333-444444444444" + accountExternal := "999988887777" + + mockClient := new(MockAnalyticsClient) + mockClient.On("QueryHistory", ctx, []string{accountUUID}, map[string][]string{"aws": {accountExternal}}, mock.Anything, mock.Anything, "hourly"). + Return([]HistoryDataPoint{{TotalSavings: 50.0, PurchaseCount: 1}}, &HistorySummary{TotalPurchases: 1}, nil) + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: accountUUID, Name: "Account B", Provider: "aws", ExternalID: accountExternal}}, nil + } + + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: mockStore} + + result, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"account_id": accountUUID}) + require.NoError(t, err) + require.NotNil(t, result) + mockClient.AssertCalled(t, "QueryHistory", ctx, []string{accountUUID}, map[string][]string{"aws": {accountExternal}}, mock.Anything, mock.Anything, "hourly") +} + // MockAnalyticsCollector is a mock implementation of AnalyticsCollectorInterface type MockAnalyticsCollector struct { mock.Mock @@ -72,10 +100,13 @@ func TestHandler_getHistoryAnalytics_Success(t *testing.T) { } summary := &HistorySummary{TotalPurchases: 8, TotalMonthlySavings: 190.0} - mockClient.On("QueryHistory", ctx, "account-123", mock.Anything, mock.Anything, "hourly").Return(dataPoints, summary, nil) + // "account-123" is not a known UUID, so resolveSingleAccountFilterIDs + // treats it as an external account number: QueryHistory is called with no + // UUIDs and account-123 as the external id. + mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string{"": {"account-123"}}, mock.Anything, mock.Anything, "hourly").Return(dataPoints, summary, nil) mockAuth, req := adminAnalyticsReq(ctx) - handler := &Handler{auth: mockAuth, analyticsClient: mockClient} + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} params := map[string]string{ "account_id": "account-123", @@ -107,17 +138,18 @@ func TestHandler_getHistoryAnalytics_DefaultInterval(t *testing.T) { mockClient := new(MockAnalyticsClient) dataPoints := []HistoryDataPoint{} - mockClient.On("QueryHistory", ctx, "", mock.Anything, mock.Anything, "hourly").Return(dataPoints, (*HistorySummary)(nil), nil) + // Empty account_id: resolver returns no UUIDs and no external ids. + mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, "hourly").Return(dataPoints, (*HistorySummary)(nil), nil) mockAuth, req := adminAnalyticsReq(ctx) - handler := &Handler{auth: mockAuth, analyticsClient: mockClient} + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} params := map[string]string{} // No interval specified _, err := handler.getHistoryAnalytics(ctx, req, params) require.NoError(t, err) - mockClient.AssertCalled(t, "QueryHistory", ctx, "", mock.Anything, mock.Anything, "hourly") + mockClient.AssertCalled(t, "QueryHistory", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, "hourly") } func TestHandler_getHistoryAnalytics_InvalidDateRange(t *testing.T) { @@ -140,10 +172,10 @@ func TestHandler_getHistoryAnalytics_QueryError(t *testing.T) { ctx := context.Background() mockClient := new(MockAnalyticsClient) - mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, errors.New("query failed")) + mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, errors.New("query failed")) mockAuth, req := adminAnalyticsReq(ctx) - handler := &Handler{auth: mockAuth, analyticsClient: mockClient} + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} params := map[string]string{} @@ -185,10 +217,10 @@ func TestHandler_getHistoryBreakdown_Success(t *testing.T) { "lambda": {PurchaseCount: 5, TotalSavings: 100.0}, } - mockClient.On("QueryBreakdown", ctx, "account-123", mock.Anything, mock.Anything, "service").Return(breakdownData, nil) + mockClient.On("QueryBreakdown", ctx, []string(nil), map[string][]string{"": {"account-123"}}, mock.Anything, mock.Anything, "service").Return(breakdownData, nil) mockAuth, req := adminAnalyticsReq(ctx) - handler := &Handler{auth: mockAuth, analyticsClient: mockClient} + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} params := map[string]string{ "account_id": "account-123", @@ -220,17 +252,17 @@ func TestHandler_getHistoryBreakdown_DefaultDimension(t *testing.T) { mockClient := new(MockAnalyticsClient) breakdownData := map[string]BreakdownValue{} - mockClient.On("QueryBreakdown", ctx, "", mock.Anything, mock.Anything, "service").Return(breakdownData, nil) + mockClient.On("QueryBreakdown", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, "service").Return(breakdownData, nil) mockAuth, req := adminAnalyticsReq(ctx) - handler := &Handler{auth: mockAuth, analyticsClient: mockClient} + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} params := map[string]string{} // No dimension specified _, err := handler.getHistoryBreakdown(ctx, req, params) require.NoError(t, err) - mockClient.AssertCalled(t, "QueryBreakdown", ctx, "", mock.Anything, mock.Anything, "service") + mockClient.AssertCalled(t, "QueryBreakdown", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, "service") } func TestHandler_getHistoryBreakdown_InvalidDateRange(t *testing.T) { @@ -253,10 +285,10 @@ func TestHandler_getHistoryBreakdown_QueryError(t *testing.T) { ctx := context.Background() mockClient := new(MockAnalyticsClient) - mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("breakdown failed")) + mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("breakdown failed")) mockAuth, req := adminAnalyticsReq(ctx) - handler := &Handler{auth: mockAuth, analyticsClient: mockClient} + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} params := map[string]string{} diff --git a/internal/api/handler_coverage_test.go b/internal/api/handler_coverage_test.go index 28b867863..a8b70a8ef 100644 --- a/internal/api/handler_coverage_test.go +++ b/internal/api/handler_coverage_test.go @@ -599,7 +599,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { t.Run("getHistoryAnalyticsHandler", func(t *testing.T) { mockAuth, req := adminAnalyticsReq(ctx) mockClient := new(MockAnalyticsClient) - mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil) + mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil) h := &Handler{auth: mockAuth, analyticsClient: mockClient} router := NewRouter(h) @@ -612,7 +612,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { t.Run("getHistoryBreakdownHandler", func(t *testing.T) { mockAuth, req := adminAnalyticsReq(ctx) mockClient := new(MockAnalyticsClient) - mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(map[string]BreakdownValue{}, nil) + mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(map[string]BreakdownValue{}, nil) h := &Handler{auth: mockAuth, analyticsClient: mockClient} router := NewRouter(h) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index f8cfc5d60..2321355dd 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -22,11 +22,26 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun return nil, err } - legacyAccountID, cloudAccountUUIDs, err := resolveDashboardAccountScope(params) + accountUUIDs, accountExternalIDsByProvider, err := h.resolveDashboardAccountScope(ctx, params) if err != nil { return nil, err } + // Issue #956 (CR): when the session is account-restricted and no explicit + // account filter was supplied, scope the commitment metrics to the session's + // allowed_accounts instead of falling through to all-accounts history. The + // recommendations half is already gated by filterDashboardRecommendations; + // without this the commitment KPIs (ActiveCommitments / CommittedMonthly / + // CurrentCoverage / YTDSavings) would leak other accounts' data to a scoped + // user. Unrestricted / admin sessions resolve to an empty scope and keep the + // all-accounts behaviour. + if len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 { + accountUUIDs, accountExternalIDsByProvider, err = h.resolveAllowedAccountScope(ctx, session) + if err != nil { + return nil, err + } + } + recommendations, err := h.scheduler.ListRecommendations(ctx, config.RecommendationFilter{ Provider: params["provider"], }) @@ -51,7 +66,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey) targetCoverage := h.resolveTargetCoverage(ctx) - activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, legacyAccountID, cloudAccountUUIDs) + activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, accountUUIDs, accountExternalIDsByProvider) // Populate CurrentSavings on each per-service bucket so the Home page // chart can render the green "Current Savings" bars with real data. @@ -75,36 +90,73 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun }, nil } -// resolveDashboardAccountScope parses the account filter params and returns -// the two disjoint representations used by commitment-metrics fetching: -// -// - legacyAccountID: the singular `account_id` param (cloud-provider account -// number, e.g. "123456789012"), passed as-is to GetPurchaseHistory when -// no UUID-based filter is present. Empty string means "no legacy filter". +// resolveDashboardAccountScope parses the account filter params and resolves +// them to the dual-column purchase-history filter inputs: the cloud_accounts +// UUIDs and their cloud-provider external account numbers. +// Both are needed because purchase_history rows carry either identifier +// independently and the top-bar chip emits the UUID, so matching only +// cloud_account_id dropped every NULL-cloud_account_id row (issue #701/#498). // -// - cloudAccountUUIDs: UUIDs from the `account_ids` param, matched against -// purchase_history.cloud_account_id via GetPurchaseHistoryFiltered. Non-nil -// (even if empty) when `account_ids` was present and valid. +// - account_ids (plural, UUIDs): resolved to UUID + external ids via +// resolveAccountFilterIDs. Takes precedence over the legacy singular param. +// - account_id (singular legacy): a top-bar chip UUID for current callers or a +// raw external number for pre-UUID callers; resolved via +// resolveSingleAccountFilterIDs. // -// Exactly one of legacyAccountID or cloudAccountUUIDs is meaningful per call: -// when account_ids is supplied, legacyAccountID is set to "" so the caller -// uses the UUID path; when only account_id is supplied, cloudAccountUUIDs is -// nil so the caller uses the legacy path; when neither is supplied, both are -// zero-valued and the caller fetches all history. -func resolveDashboardAccountScope(params map[string]string) (legacyAccountID string, cloudAccountUUIDs []string, err error) { - uuids, parseErr := parseAccountIDs(params["account_ids"]) +// Both return values are nil when neither param is supplied, so the caller +// fetches across all accounts (or, for a restricted session, scopes to the +// session's allowed_accounts — see resolveAllowedAccountScope). +func (h *Handler) resolveDashboardAccountScope(ctx context.Context, params map[string]string) (uuids []string, externalIDsByProvider map[string][]string, err error) { + parsedUUIDs, parseErr := parseAccountIDs(params["account_ids"]) if parseErr != nil { - return "", nil, NewClientError(400, parseErr.Error()) + return nil, nil, NewClientError(400, parseErr.Error()) } - if len(uuids) > 0 { + if len(parsedUUIDs) > 0 { // UUID-based multi-account filter — takes precedence over legacy param. - return "", uuids, nil + uuids, externalIDsByProvider = h.resolveAccountFilterIDs(ctx, parsedUUIDs) + return uuids, externalIDsByProvider, nil } - // No UUID filter: fall back to the legacy singular cloud-provider account - // number. Empty string means "fetch all accounts" (no filter). - return params["account_id"], nil, nil + // No plural UUID filter: fall back to the legacy singular param. + uuids, externalIDsByProvider = h.resolveSingleAccountFilterIDs(ctx, params["account_id"]) + return uuids, externalIDsByProvider, nil +} + +// resolveAllowedAccountScope resolves a restricted session's allowed_accounts +// into the dual-column purchase-history filter inputs so dashboard commitment +// metrics never include accounts the session can't access (issue #956). It lists +// the cloud accounts, keeps those the session matches via auth.MatchesAccount, +// and resolves their UUIDs through resolveAccountFilterIDs (same code path as an +// explicit filter). +// +// Returns (nil, nil, nil) for unrestricted / admin sessions so the caller keeps +// the all-accounts behaviour. A restricted session that matches no account +// resolves to a non-nil-but-empty UUID set (a sentinel that selects no rows), +// so a scoped user with zero accessible accounts sees zeroed KPIs rather than +// everyone's data. +func (h *Handler) resolveAllowedAccountScope(ctx context.Context, session *Session) (uuids []string, externalIDsByProvider map[string][]string, err error) { + allowed, err := h.getAllowedAccounts(ctx, session) + if err != nil { + return nil, nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + if auth.IsUnrestrictedAccess(allowed) { + return nil, nil, nil + } + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) + if err != nil { + return nil, nil, fmt.Errorf("failed to list cloud accounts: %w", err) + } + // Non-nil empty slice: a sentinel meaning "scoped to zero accounts" so the + // dual-column predicate matches no rows (never falls back to all-accounts). + allowedUUIDs := []string{} + for _, a := range accounts { + if auth.MatchesAccount(allowed, a.ID, a.Name) { + allowedUUIDs = append(allowedUUIDs, a.ID) + } + } + uuids, externalIDsByProvider = h.resolveAccountFilterIDs(ctx, allowedUUIDs) + return uuids, externalIDsByProvider, nil } // filterDashboardRecommendations applies the session's allowed_accounts filter @@ -453,47 +505,68 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco return byService } -// calculateCommitmentMetrics aggregates active-commitment counts and monthly -// savings from purchase history. The fetch strategy depends on the filter: -// -// - cloudAccountUUIDs non-empty: GetPurchaseHistoryFiltered scoped to those -// cloud_account_id UUIDs (multi-account UUID filter from `account_ids`). -// - legacyAccountID non-empty: GetPurchaseHistory filtered by account_id -// (cloud-provider account number, e.g. "123456789012"; legacy single-account -// filter from the `account_id` param). -// - both empty: GetAllPurchaseHistory (no account filter). -// -// EstimatedSavings on purchase_history rows is always written in monthly units -// (populated from PurchaseExecution.EstimatedSavings which derives from -// recommendation monthly savings at purchase time, see SavePurchaseHistory -// and the purchase manager). No unit normalisation is needed. +// fetchCommitmentPurchases loads the purchase_history rows that calculateCommitmentMetrics +// aggregates, applying the pre-resolved account scope. Extracted to keep the +// parent under the cyclomatic limit (mirrors the appendAccountPredicate / +// accountMatchesFilters pattern). Returns ok=false on a store error or an +// explicit zero-account scope so the caller emits zeroed KPIs without querying. // -// The fourth return value is the per-service breakdown of active -// EstimatedSavings, derived from aggregateActiveCommitmentsPerService so both -// this KPI path (committedMonthly) and the per-service chart use exactly the -// same gate. -func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountID string, cloudAccountUUIDs []string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) { +// - non-nil but empty accountUUIDs (no external groups): explicit "scoped to +// zero accounts" sentinel (a restricted session whose allowed_accounts match +// nothing). Returns (nil, false) WITHOUT querying so a scoped user never sees +// all-accounts data (issue #956). A nil/empty filter sent to the store would +// match every row (no WHERE clause), so this must short-circuit. +// - either accountUUIDs or accountExternalIDsByProvider non-empty: +// GetPurchaseHistoryFiltered with the dual-column predicate. +// - both nil: GetAllPurchaseHistory (no account filter — unrestricted session). +func (h *Handler) fetchCommitmentPurchases(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) { const fetchLimit = 1000 + if accountUUIDs != nil && len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 { + return nil, false + } + var ( purchases []config.PurchaseHistoryRecord err error ) - - switch { - case len(cloudAccountUUIDs) > 0: - // Multi-account UUID filter: match purchase_history.cloud_account_id. - purchases, err = h.config.GetPurchaseHistoryFiltered(ctx, "", cloudAccountUUIDs, nil, nil, fetchLimit) - case legacyAccountID != "": - // Legacy single-account filter: match purchase_history.account_id. - purchases, err = h.config.GetPurchaseHistory(ctx, legacyAccountID, fetchLimit) - default: + if len(accountUUIDs) > 0 || len(accountExternalIDsByProvider) > 0 { + // Account filter: dual-column match so NULL-cloud_account_id rows are + // counted via their external id. + purchases, err = h.config.GetPurchaseHistoryFiltered(ctx, config.PurchaseHistoryFilter{ + AccountIDs: accountUUIDs, + ExternalIDsByProvider: accountExternalIDsByProvider, + Limit: fetchLimit, + }) + } else { // No account filter: fetch across all accounts. purchases, err = h.config.GetAllPurchaseHistory(ctx, fetchLimit) } - if err != nil { // Log error but don't fail the dashboard request. + return nil, false + } + return purchases, true +} + +// calculateCommitmentMetrics aggregates active-commitment counts and monthly +// savings from purchase history. The account scope arrives pre-resolved as the +// dual-column filter inputs (see resolveDashboardAccountScope); the +// scope-to-query mapping (including the zero-account short-circuit for issue +// #956) lives in fetchCommitmentPurchases. +// +// EstimatedSavings on purchase_history rows is always written in monthly units +// (populated from PurchaseExecution.EstimatedSavings which derives from +// recommendation monthly savings at purchase time, see SavePurchaseHistory +// and the purchase manager). No unit normalisation is needed. +// +// The fourth return value is the per-service breakdown of active +// EstimatedSavings, derived from aggregateActiveCommitmentsPerService so both +// this KPI path (committedMonthly) and the per-service chart use exactly the +// same gate. +func (h *Handler) calculateCommitmentMetrics(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) { + purchases, ok := h.fetchCommitmentPurchases(ctx, accountUUIDs, accountExternalIDsByProvider) + if !ok { return 0, 0, 0, nil } diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index c74bac8f7..ec13d43e9 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -685,11 +685,11 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { t.Run("no purchase history", func(t *testing.T) { mockStore := new(MockConfigStore) - mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return([]config.PurchaseHistoryRecord{}, nil) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil) + activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 0, activeCommitments) assert.Equal(t, 0.0, committedMonthly) @@ -699,11 +699,11 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { t.Run("purchase history error returns zeros", func(t *testing.T) { mockStore := new(MockConfigStore) - mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return(nil, errors.New("db error")) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(nil, errors.New("db error")) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil) + activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 0, activeCommitments) assert.Equal(t, 0.0, committedMonthly) @@ -725,11 +725,11 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { }, } - mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return(purchases, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil) + activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 100.0, committedMonthly) @@ -752,11 +752,11 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { }, } - mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return(purchases, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil) + activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}}) // Should skip expired commitments assert.Equal(t, 0, activeCommitments) @@ -779,11 +779,11 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { }, } - mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return(purchases, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil) + activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 50.0, committedMonthly) @@ -812,11 +812,11 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { // Status "" = completed DB row — must be counted }, } - mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return(purchases, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"account-123"}}, Limit: 1000}).Return(purchases, nil) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil) + activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}}) // Only the status="" row counts; the failed row must be excluded. assert.Equal(t, 1, activeCommitments, @@ -842,7 +842,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { {CloudAccountID: &accountBID, Timestamp: purchaseTime, Term: 1, EstimatedSavings: 150.0}, } uuids := []string{accountAID, accountBID} - mockStore.On("GetPurchaseHistoryFiltered", ctx, "", uuids, (*time.Time)(nil), (*time.Time)(nil), 1000). + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: uuids, Limit: 1000}). Return(purchasesAB, nil) // GetPurchaseHistory must not be called — no On registration so it // would panic if accidentally invoked. @@ -850,12 +850,40 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", uuids) + activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, uuids, nil) assert.Equal(t, 2, activeCommitments) assert.Equal(t, 250.0, committedMonthly, "only accounts A and B must contribute; account C rows must not appear") }) + + // Keystone (issue #701/#498/#866): an account's commitments may live in rows + // that carry only the external account_id (cloud_account_id NULL). When the + // dashboard scope resolves the selected UUID to its external id, the + // dual-column filter must include those rows so the KPIs aren't zero. + t.Run("external-id-only commitment rows are counted via the dual-column filter", func(t *testing.T) { + mockStore := new(MockConfigStore) + purchaseTime := time.Now().AddDate(0, -2, 0) + + // Row attributed by external number only (no CloudAccountID). + externalOnly := []config.PurchaseHistoryRecord{ + {AccountID: "999988887777", Timestamp: purchaseTime, Term: 1, EstimatedSavings: 175.0}, + } + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ + AccountIDs: []string{"bbbbbbbb-1111-2222-3333-444444444444"}, + ExternalIDsByProvider: map[string][]string{"aws": {"999988887777"}}, + Limit: 1000, + }).Return(externalOnly, nil) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{config: mockStore} + + activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics( + ctx, []string{"bbbbbbbb-1111-2222-3333-444444444444"}, map[string][]string{"aws": {"999988887777"}}) + + assert.Equal(t, 1, activeCommitments, "external-id-only commitment must be counted") + assert.Equal(t, 175.0, committedMonthly) + }) } // TestAggregateActiveCommitmentsPerService covers the core primitive used by diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 35d8bdb3f..e33fbac62 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -27,6 +27,14 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR return nil, err } + // Resolve the requested account identifiers to the dual-column filter + // inputs (UUIDs + their external account numbers) so both the SQL path and + // the in-memory execution path match rows that carry only one of the two + // representations (issue #701/#498/#866). The legacy singular account_id + // (a top-bar chip UUID, or a raw external number for pre-UUID callers) is + // folded in here so fetchPurchaseHistory sees a single unified account set. + h.resolveHistoryAccountFilter(ctx, &filters) + completed, err := h.fetchPurchaseHistory(ctx, filters) if err != nil { return nil, err @@ -536,10 +544,21 @@ type historyFilters struct { Provider string LegacyAccountID string AccountIDs []string - HasDate bool - Start time.Time - End time.Time - Limit int + // ExternalIDsByProvider are the cloud-provider external account numbers + // resolved from AccountIDs (the UUIDs) via Handler.resolveAccountFilterIDs, + // grouped by provider so the external-id match stays provider-scoped (a + // reused external number across providers cannot leak rows). Populated by + // the handler AFTER parse (the resolution needs a DB read), not by + // parseHistoryFilters. Both the SQL path (provider = $p AND account_id = + // ANY) and the in-memory matchesExecution use them so a row/execution that + // carries only the external id (cloud_account_id NULL) is still matched + // (issue #701/#498). The "" provider key means "provider unknown" and + // matches the external id regardless of provider (legacy behaviour). + ExternalIDsByProvider map[string][]string + HasDate bool + Start time.Time + End time.Time + Limit int } // parseHistoryFilters validates and normalises the /api/history query string. @@ -683,23 +702,8 @@ func (f historyFilters) matchesExecution(exec config.PurchaseExecution) bool { return false } } - if len(f.AccountIDs) > 0 { - // AccountIDs are UUIDs against cloud_account_id; legacy - // LegacyAccountID is intentionally NOT folded here because it is a - // different (VARCHAR(20)) cloud-provider account number that the - // fast path applies on the SQL side. - // - // Use the same two-level account resolution as executionToHistoryRow: - // exec.CloudAccountID first, then the rec-level fallback. This ensures - // web bulk-purchase executions (exec.CloudAccountID == nil) are not - // silently dropped when the caller filters by account. - var accountID string - if exec.CloudAccountID != nil { - accountID = *exec.CloudAccountID - } else { - accountID = collapseRecommendationAccount(exec.Recommendations) - } - if accountID == "" || !stringInSlice(accountID, f.AccountIDs) { + if len(f.AccountIDs) > 0 || len(f.ExternalIDsByProvider) > 0 { + if !accountMatchesFilters(exec, f.AccountIDs, f.ExternalIDsByProvider) { return false } } @@ -711,6 +715,61 @@ func (f historyFilters) matchesExecution(exec config.PurchaseExecution) bool { return true } +// accountMatchesFilters pulls the dual-id account-match logic out of +// matchesExecution to keep it under the cyclomatic limit. +// +// AccountIDs are cloud_accounts UUIDs; externalIDsByProvider are the +// cloud-provider external account numbers resolved from them, grouped by +// provider. An execution's effective account ID may be EITHER representation: +// exec.CloudAccountID and the per-rec CloudAccountID hold the UUID for +// UUID-attributed executions, while a legacy/ambient execution may only carry +// the external number. Match against both so an external-id-only pending row is +// not dropped (the mirror of the SQL dual-column predicate, issue #701/#498). +// +// The external-id match is provider-scoped: an external number matches only when +// it is listed under the execution's own provider, or under the "" key (unknown +// provider, legacy behaviour). This mirrors the SQL (provider = $p AND +// account_id = ANY(...)) and keeps a reused external number across providers +// (aws/123 vs azure/123) from matching the wrong execution. +// +// Uses the same two-level account resolution as executionToHistoryRow: +// exec.CloudAccountID first, then the rec-level fallback, so web bulk-purchase +// executions (exec.CloudAccountID == nil) are not silently dropped. +func accountMatchesFilters(exec config.PurchaseExecution, accountIDs []string, externalIDsByProvider map[string][]string) bool { + var accountID string + if exec.CloudAccountID != nil { + accountID = *exec.CloudAccountID + } else { + accountID = collapseRecommendationAccount(exec.Recommendations) + } + if accountID == "" { + return false + } + if stringInSlice(accountID, accountIDs) { + return true + } + // External-id half: only the execution's own provider group (plus the "" + // unknown-provider group) may match, so a reused external number across + // providers can't leak. + provider := executionProvider(exec) + if provider != "" && stringInSlice(accountID, externalIDsByProvider[provider]) { + return true + } + return stringInSlice(accountID, externalIDsByProvider[""]) +} + +// executionProvider returns the execution's provider, taken from its first +// recommendation (all recs in an execution share a provider in practice). +// Returns "" when no recommendation carries one. +func executionProvider(exec config.PurchaseExecution) string { + for _, r := range exec.Recommendations { + if r.Provider != "" { + return r.Provider + } + } + return "" +} + // executionHasProvider reports whether any of the execution's // recommendations carries the given provider value. func executionHasProvider(exec config.PurchaseExecution, provider string) bool { @@ -736,25 +795,24 @@ func stringInSlice(needle string, haystack []string) bool { } // fetchPurchaseHistory reads purchases from the store, applying the parsed -// filter set. The store-level filtered method (added with issue #701) -// pushes provider / cloud_account_id / timestamp range into SQL; the -// fast-path legacy methods (no-filter, single-account-only) are kept for -// the dashboard/inventory/analytics callers that don't speak the new shape. +// filter set. The store-level filtered method (issue #701) pushes provider / +// account / timestamp range into SQL. The account predicate is dual-column +// (cloud_account_id = ANY(AccountIDs) OR (provider = $p AND account_id = +// ANY(ExternalIDsByProvider[p]))) so a row that carries only one of the two +// account representations is still matched — the original #701/#498/#866 bug was +// that filtering on the sparse cloud_account_id UUID column alone dropped every +// direct-execute/ambient row. Grouping the external ids by provider keeps a +// reused external number across providers from leaking the wrong rows. // -// LegacyAccountID is honoured only on the fast path (it filters the -// VARCHAR(20) purchase_history.account_id column). When combined with any -// new filter it is ignored: the new filtered method's account predicate is -// on cloud_account_id (UUID), and silently coercing one column's identifier -// into the other would either match nothing or, worse, match the wrong -// rows. Callers using the new filter set should send `account_ids` -// (UUIDs); the legacy singular param is preserved for the dashboard's -// historical, single-cloud-account view. +// The no-filter case still uses GetAllPurchaseHistory as a fast path. The +// legacy singular account_id (LegacyAccountID) is resolved to the same dual- +// column inputs by the caller (getHistory) and arrives here folded into +// AccountIDs/ExternalIDsByProvider, so there is no longer a separate +// single-column legacy path to coerce identifiers through. func (h *Handler) fetchPurchaseHistory(ctx context.Context, filters historyFilters) ([]config.PurchaseHistoryRecord, error) { - noNewFilters := !filters.HasDate && filters.Provider == "" && len(filters.AccountIDs) == 0 - if noNewFilters { - if filters.LegacyAccountID != "" { - return h.config.GetPurchaseHistory(ctx, filters.LegacyAccountID, filters.Limit) - } + noFilters := !filters.HasDate && filters.Provider == "" && + len(filters.AccountIDs) == 0 && len(filters.ExternalIDsByProvider) == 0 + if noFilters { return h.config.GetAllPurchaseHistory(ctx, filters.Limit) } @@ -764,14 +822,54 @@ func (h *Handler) fetchPurchaseHistory(ctx context.Context, filters historyFilte startPtr = &s endPtr = &e } - return h.config.GetPurchaseHistoryFiltered( - ctx, - filters.Provider, - filters.AccountIDs, - startPtr, - endPtr, - filters.Limit, - ) + return h.config.GetPurchaseHistoryFiltered(ctx, config.PurchaseHistoryFilter{ + Provider: filters.Provider, + AccountIDs: filters.AccountIDs, + ExternalIDsByProvider: filters.ExternalIDsByProvider, + Start: startPtr, + End: endPtr, + Limit: filters.Limit, + }) +} + +// resolveHistoryAccountFilter populates filters.AccountIDs / +// filters.ExternalIDsByProvider from the parsed account params so the SQL and +// in-memory paths share one dual-column account set. The plural `account_ids` +// (UUIDs) are resolved to their per-provider external numbers via +// resolveAccountFilterIDs. The singular legacy `account_id` is folded in too: it +// is a top-bar chip UUID for current callers (resolved to UUID + external) or a +// raw external number for pre-UUID callers (grouped under the "" provider key). +// Best-effort: resolution failures leave the UUID-only set in place (no worse +// than the pre-fix behaviour), and the per-record allowed_accounts filter still +// enforces scoping downstream. +func (h *Handler) resolveHistoryAccountFilter(ctx context.Context, filters *historyFilters) { + uuids, externalsByProvider := h.resolveAccountFilterIDs(ctx, filters.AccountIDs) + + if filters.LegacyAccountID != "" { + lUUIDs, lExternalsByProvider := h.resolveSingleAccountFilterIDs(ctx, filters.LegacyAccountID) + uuids = appendMissing(uuids, lUUIDs...) + for provider, exts := range lExternalsByProvider { + for _, ext := range exts { + externalsByProvider = addExternalIDForProvider(externalsByProvider, provider, ext) + } + } + } + + filters.AccountIDs = uuids + filters.ExternalIDsByProvider = externalsByProvider +} + +// appendMissing appends each value to dst only if not already present, +// preserving order. Used to merge the legacy single-account ids into the +// plural sets without introducing duplicate ANY() bind values. +func appendMissing(dst []string, vals ...string) []string { + for _, v := range vals { + if v == "" || stringInSlice(v, dst) { + continue + } + dst = append(dst, v) + } + return dst } // filterPurchaseHistoryByAllowedAccounts drops records whose AccountID/Name diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 0ff679e9c..e5872a43c 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -37,7 +37,10 @@ func TestHandler_getHistory(t *testing.T) { {AccountID: "123456789012", PurchaseID: "purchase-1", UpfrontCost: 100.0, EstimatedSavings: 10.0}, } - mockStore.On("GetPurchaseHistory", ctx, "123456789012", 100).Return(history, nil) + // account_id "123456789012" is not a known cloud_accounts UUID, so it is + // folded into the dual-column filter as an external account number and the + // request routes through GetPurchaseHistoryFiltered (issue #701/#498/#866). + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"123456789012"}}, Limit: 100}).Return(history, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) mockAuth, req := adminHistoryReq(ctx) @@ -646,6 +649,12 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { mockStore := new(MockConfigStore) mockAuth, req := adminHistoryReq(ctx) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approver}, nil).Maybe() + // resolveHistoryAccountFilter loads cloud_accounts to resolve UUIDs to + // their external ids. Default to an empty list (no resolvable external + // ids) unless a sub-test overrides ListCloudAccountsFn. + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return nil, nil + } return mockStore, &Handler{auth: mockAuth, config: mockStore}, req, ctx } @@ -657,7 +666,7 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { filtered := []config.PurchaseHistoryRecord{ {AccountID: "acc-aws", PurchaseID: "p-aws", Provider: "aws", UpfrontCost: 100.0}, } - mockStore.On("GetPurchaseHistoryFiltered", ctx, "aws", []string(nil), (*time.Time)(nil), (*time.Time)(nil), config.DefaultListLimit). + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{Provider: "aws", Limit: config.DefaultListLimit}). Return(filtered, nil).Once() // Executions path: two execs, one aws-rec and one azure-rec. Only the @@ -687,7 +696,7 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { for _, p := range resp.Purchases { assert.NotEqual(t, "exec-azure", p.PurchaseID, "azure-only execution must be filtered out when provider=aws") } - mockStore.AssertCalled(t, "GetPurchaseHistoryFiltered", ctx, "aws", []string(nil), (*time.Time)(nil), (*time.Time)(nil), config.DefaultListLimit) + mockStore.AssertCalled(t, "GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{Provider: "aws", Limit: config.DefaultListLimit}) }) t.Run("provider=all is treated as no filter (legacy SQL path)", func(t *testing.T) { @@ -698,7 +707,7 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { _, err := handler.getHistory(ctx, req, map[string]string{"provider": "all"}) require.NoError(t, err) mockStore.AssertCalled(t, "GetAllPurchaseHistory", ctx, config.DefaultListLimit) - mockStore.AssertNotCalled(t, "GetPurchaseHistoryFiltered", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetPurchaseHistoryFiltered", mock.Anything, mock.Anything) }) t.Run("account_ids filter is pushed to SQL and prunes executions", func(t *testing.T) { @@ -707,7 +716,7 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { uuidA := "11111111-1111-1111-1111-111111111111" uuidB := "22222222-2222-2222-2222-222222222222" - mockStore.On("GetPurchaseHistoryFiltered", ctx, "", []string{uuidA, uuidB}, (*time.Time)(nil), (*time.Time)(nil), config.DefaultListLimit). + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{uuidA, uuidB}, Limit: config.DefaultListLimit}). Return([]config.PurchaseHistoryRecord{{AccountID: "acc-A", PurchaseID: "p-A"}}, nil).Once() execs := []config.PurchaseExecution{ @@ -750,18 +759,16 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { assert.False(t, ids["exec-nil-account"], "execution with NULL exec.CloudAccountID AND no rec-level CloudAccountID must still be excluded (effective account is empty)") }) - t.Run("legacy singular account_id is ignored when combined with new filters", func(t *testing.T) { - // account_id (singular) is the AWS-style VARCHAR(20) account number - // matched against purchase_history.account_id by the legacy - // GetPurchaseHistory; it is NOT a UUID and therefore must not be - // silently coerced into the cloud_account_id (UUID) WHERE clause of - // the new GetPurchaseHistoryFiltered method when other filters are - // also present. The frontend uses `account_ids` (plural, UUIDs) for - // the new shape; the singular param survives only for backward - // compatibility with the no-other-filters fast path. + t.Run("legacy singular account_id is folded into the dual-column filter", func(t *testing.T) { + // account_id (singular) is an AWS-style external account number (not a + // known cloud_accounts UUID in this fixture). resolveHistoryAccountFilter + // folds it into the ExternalIDs half of the dual-column predicate so it + // is matched against purchase_history.account_id alongside any other + // filters — fixing the prior bug where the legacy param was silently + // dropped when combined with provider/account_ids (issue #701/#498/#866). mockStore, handler, req, ctx := newHandler(t) - mockStore.On("GetPurchaseHistoryFiltered", ctx, "aws", []string(nil), (*time.Time)(nil), (*time.Time)(nil), config.DefaultListLimit). + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{Provider: "aws", ExternalIDsByProvider: map[string][]string{"": {"123456789012"}}, Limit: config.DefaultListLimit}). Return([]config.PurchaseHistoryRecord{}, nil).Once() mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) @@ -771,16 +778,19 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { mockStore.AssertNotCalled(t, "GetPurchaseHistory", mock.Anything, mock.Anything, mock.Anything) }) - t.Run("legacy singular account_id alone still hits the fast path", func(t *testing.T) { + t.Run("legacy singular account_id alone routes through the dual-column filter", func(t *testing.T) { + // A bare external account number is now matched on account_id via the + // dual-column filter rather than the single-column GetPurchaseHistory + // fast path, so its rows surface regardless of cloud_account_id. mockStore, handler, req, ctx := newHandler(t) - mockStore.On("GetPurchaseHistory", ctx, "123456789012", config.DefaultListLimit). + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ExternalIDsByProvider: map[string][]string{"": {"123456789012"}}, Limit: config.DefaultListLimit}). Return([]config.PurchaseHistoryRecord{{AccountID: "123456789012", PurchaseID: "p-legacy"}}, nil).Once() mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) _, err := handler.getHistory(ctx, req, map[string]string{"account_id": "123456789012"}) require.NoError(t, err) mockStore.AssertExpectations(t) - mockStore.AssertNotCalled(t, "GetPurchaseHistoryFiltered", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetPurchaseHistory", mock.Anything, mock.Anything, mock.Anything) }) t.Run("start/end date filter is pushed to SQL and prunes executions", func(t *testing.T) { @@ -796,11 +806,13 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { endDay := now.Format("2006-01-02") outOfRangeDay := now.AddDate(0, 0, -10) // older than the window AND outside the requested range - mockStore.On("GetPurchaseHistoryFiltered", - ctx, "", []string(nil), - mock.MatchedBy(func(p *time.Time) bool { return p != nil && p.Format("2006-01-02") == startDay }), - mock.MatchedBy(func(p *time.Time) bool { return p != nil && p.Format("2006-01-02") == endDay }), - config.DefaultListLimit, + mockStore.On("GetPurchaseHistoryFiltered", ctx, + mock.MatchedBy(func(f config.PurchaseHistoryFilter) bool { + return f.Provider == "" && len(f.AccountIDs) == 0 && len(f.ExternalIDsByProvider) == 0 && + f.Start != nil && f.Start.Format("2006-01-02") == startDay && + f.End != nil && f.End.Format("2006-01-02") == endDay && + f.Limit == config.DefaultListLimit + }), ).Return([]config.PurchaseHistoryRecord{{AccountID: "in-range", PurchaseID: "p-in-range"}}, nil).Once() execs := []config.PurchaseExecution{ @@ -833,11 +845,11 @@ func TestHandler_getHistory_FilterParams(t *testing.T) { mockStore, handler, req, ctx := newHandler(t) uuidA := "55555555-5555-5555-5555-555555555555" - mockStore.On("GetPurchaseHistoryFiltered", - ctx, "aws", []string{uuidA}, - mock.MatchedBy(func(p *time.Time) bool { return p != nil }), - mock.MatchedBy(func(p *time.Time) bool { return p != nil }), - config.DefaultListLimit, + mockStore.On("GetPurchaseHistoryFiltered", ctx, + mock.MatchedBy(func(f config.PurchaseHistoryFilter) bool { + return f.Provider == "aws" && len(f.AccountIDs) == 1 && f.AccountIDs[0] == uuidA && + f.Start != nil && f.End != nil && f.Limit == config.DefaultListLimit + }), ).Return([]config.PurchaseHistoryRecord{{AccountID: "match", PurchaseID: "p-match"}}, nil).Once() now := time.Now().UTC() @@ -958,7 +970,7 @@ func TestHandler_getHistory_FilterValidation(t *testing.T) { // Sanity guards: no store calls leaked through on a 400. mockStore.AssertNotCalled(t, "GetPurchaseHistory", mock.Anything, mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", mock.Anything, mock.Anything) - mockStore.AssertNotCalled(t, "GetPurchaseHistoryFiltered", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetPurchaseHistoryFiltered", mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "GetExecutionsByStatuses", mock.Anything, mock.Anything, mock.Anything) }) } @@ -974,7 +986,7 @@ func TestHandler_getHistory_DateRangeBoundary(t *testing.T) { mockStore := new(MockConfigStore) mockAuth, req := adminHistoryReq(ctx) handler := &Handler{auth: mockAuth, config: mockStore} - mockStore.On("GetPurchaseHistoryFiltered", ctx, "", []string(nil), mock.Anything, mock.Anything, config.DefaultListLimit). + mockStore.On("GetPurchaseHistoryFiltered", ctx, mock.Anything). Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) @@ -1289,7 +1301,7 @@ func TestHandler_getHistory_ApprovalQueueColumnsPopulated(t *testing.T) { // account_ids is set, so fetchPurchaseHistory routes to GetPurchaseHistoryFiltered // (not GetAllPurchaseHistory). No DB rows needed; we only care about the // execution path. - mockStore.On("GetPurchaseHistoryFiltered", ctx, "", []string{recAccountID}, (*time.Time)(nil), (*time.Time)(nil), 100). + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{recAccountID}, Limit: 100}). Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approverEmail}, nil) @@ -1306,6 +1318,82 @@ func TestHandler_getHistory_ApprovalQueueColumnsPopulated(t *testing.T) { }) } +// TestHandler_getHistory_ExternalIDOnlyAccount is the end-to-end keystone +// regression test for issue #701/#498/#866: an account whose purchase_history +// rows were written by a direct-execute/ambient/legacy path carry only the +// external account_id (cloud_account_id IS NULL). When the user selects that +// account by its cloud_accounts UUID (the top-bar chip value), the handler +// must resolve the UUID to its external id and pass BOTH to the store so the +// dual-column predicate matches the external-id-only rows. Before the fix the +// handler filtered cloud_account_id only and the user saw "No purchase history +// yet" for the account. +func TestHandler_getHistory_ExternalIDOnlyAccount(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + accountBUUID := "bbbbbbbb-1111-2222-3333-444444444444" + accountBExternal := "999988887777" + + // cloud_accounts knows account B's external id; the resolver maps the + // requested UUID to it. + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{ + {ID: accountBUUID, Name: "Account B", Provider: "aws", ExternalID: accountBExternal}, + }, nil + } + + // The store returns account B's row only when the dual-column filter carries + // BOTH the UUID and the resolved external id. The row itself has the external + // AccountID and (implicitly) a NULL cloud_account_id. + bRow := []config.PurchaseHistoryRecord{ + {AccountID: accountBExternal, PurchaseID: "p-B", Provider: "aws", Service: "ec2", UpfrontCost: 100.0, EstimatedSavings: 25.0}, + } + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ + AccountIDs: []string{accountBUUID}, + ExternalIDsByProvider: map[string][]string{"aws": {accountBExternal}}, + Limit: config.DefaultListLimit, + }).Return(bRow, nil).Once() + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.getHistory(ctx, req, map[string]string{"account_ids": accountBUUID}) + require.NoError(t, err) + resp := result.(HistoryResponse) + + require.Len(t, resp.Purchases, 1, "external-id-only row must surface when its account UUID is requested") + assert.Equal(t, accountBExternal, resp.Purchases[0].AccountID) +} + +// TestMatchesExecution_ExternalIDOnlyPending asserts the in-memory mirror of +// the dual-column filter: a pending execution attributed only by an external +// account number (no UUID anywhere) must survive when the request resolves to +// that external id, so external-id-only pending rows aren't dropped from the +// approval queue (issue #701/#498/#866). +func TestMatchesExecution_ExternalIDOnlyPending(t *testing.T) { + external := "999988887777" + uuid := "bbbbbbbb-1111-2222-3333-444444444444" + + // Execution carries the external number as its effective account id. + exec := config.PurchaseExecution{ + ExecutionID: "pend-external", + Status: "pending", + ScheduledDate: time.Now(), + CloudAccountID: strPtr(external), + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}}, + } + + // Filtering by the UUID alone drops it (no UUID match)... + uuidOnly := historyFilters{AccountIDs: []string{uuid}} + assert.False(t, uuidOnly.matchesExecution(exec), "UUID-only filter must not match an external-id-only execution") + + // ...but once the external id is resolved into the filter it is retained. + dual := historyFilters{AccountIDs: []string{uuid}, ExternalIDsByProvider: map[string][]string{"aws": {external}}} + assert.True(t, dual.matchesExecution(exec), "external-id-only pending execution must survive when its external id is in the filter") +} + // TestHandler_getHistory_CompletedExecutionNotDuplicated guards the dedup path. // The store loads "completed" executions now (so audit-gap rows can surface), // but a NORMAL completed execution (Error=="") is already represented by its diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 5f765df18..66c4432fb 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -76,15 +76,25 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF // drop expired rows before returning) so a high cap is appropriate; an // over-truncation here would silently hide rows the user is entitled to. // -// `provider` filtering is applied in-memory after the store read so that -// the existing single-account and all-accounts store paths remain -// unchanged; the record set is small enough that a post-read filter has -// negligible cost. +// The singular `account_id` (a top-bar chip cloud_accounts UUID for current +// callers, or a raw external number for legacy ones) is resolved to the +// dual-column filter inputs so a commitment row that carries only the external +// account_id (cloud_account_id NULL) is still returned — the original +// #701/#498/#866 bug was that passing the UUID to the single-column +// GetPurchaseHistory (account_id) matched nothing. +// +// `provider` filtering is applied in-memory after the store read so the +// record set is small enough that a post-read filter has negligible cost. func (h *Handler) fetchCommitmentRecords(ctx context.Context, params map[string]string) ([]config.PurchaseHistoryRecord, error) { var rows []config.PurchaseHistoryRecord var err error if accountID := params["account_id"]; accountID != "" { - rows, err = h.config.GetPurchaseHistory(ctx, accountID, config.MaxListLimit) + uuids, externalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, accountID) + rows, err = h.config.GetPurchaseHistoryFiltered(ctx, config.PurchaseHistoryFilter{ + AccountIDs: uuids, + ExternalIDsByProvider: externalIDsByProvider, + Limit: config.MaxListLimit, + }) } else { rows, err = h.config.GetAllPurchaseHistory(ctx, config.MaxListLimit) } diff --git a/internal/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index 92f93691a..95476f85d 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -131,8 +131,10 @@ func TestHandler_listActiveCommitments_FiltersExpired(t *testing.T) { } // TestHandler_listActiveCommitments_AccountFilter verifies the account_id -// query param routes through GetPurchaseHistory (single-account read) +// query param routes through the dual-column GetPurchaseHistoryFiltered // instead of GetAllPurchaseHistory, and the response respects the filter. +// "acc-1" is a known cloud_accounts UUID (no external id in this fixture), so +// it is matched on cloud_account_id only (issue #701/#498/#866). func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -152,9 +154,9 @@ func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { }, } - mockStore.On("GetPurchaseHistory", ctx, "acc-1", config.MaxListLimit).Return(purchases, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{"acc-1"}, Limit: config.MaxListLimit}).Return(purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { - return []config.CloudAccount{{ID: "22222222-0000-0000-0000-000000000001", ExternalID: "acc-1", Name: "Account One"}}, nil + return []config.CloudAccount{{ID: "acc-1", Name: "Account One"}}, nil } mockAuth, req := adminInventoryReq(ctx) @@ -167,8 +169,10 @@ func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { require.Len(t, resp.Commitments, 1) assert.Equal(t, "acc-1", resp.Commitments[0].AccountID) - // GetAllPurchaseHistory must NOT have been called when account_id is set. + // GetAllPurchaseHistory and the legacy single-column GetPurchaseHistory must + // NOT have been called when account_id is set. mockStore.AssertNotCalled(t, "GetAllPurchaseHistory") + mockStore.AssertNotCalled(t, "GetPurchaseHistory") } // TestHandler_listActiveCommitments_ProviderFilter verifies the provider @@ -489,9 +493,9 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { now := time.Now() // Only the acc-1 + aws commitment should reach the aws section. - // GetPurchaseHistory(acc-1) is the single-account read path; the - // account_id chip routes through it, so the store only returns acc-1 - // rows here. The provider chip is applied in-memory by + // GetPurchaseHistoryFiltered(AccountIDs:[acc-1]) is the per-account read + // path; the account_id chip routes through it, so the store only returns + // acc-1 rows here. The provider chip is applied in-memory by // fetchCommitmentRecords to drop the azure row. acc1Purchases := []config.PurchaseHistoryRecord{ { @@ -526,7 +530,7 @@ func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { {Provider: "azure", Service: "compute", Savings: 999.0, CloudAccountID: strPtr("acc-1")}, } - mockStore.On("GetPurchaseHistory", ctx, "acc-1", config.MaxListLimit).Return(acc1Purchases, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{"acc-1"}, Limit: config.MaxListLimit}).Return(acc1Purchases, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return []config.CloudAccount{ {ID: "acc-1", Name: "Account One"}, diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index c5bd405ef..7ba7eddbc 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -363,7 +363,10 @@ func TestPerAccountPerms_HistoryAnalytics_AllowedAccountSucceeds(t *testing.T) { start := now.Add(-7 * 24 * time.Hour) mockClient := new(MockAnalyticsClient) - mockClient.On("QueryHistory", ctx, permsAccA, mock.Anything, mock.Anything, mock.Anything). + // permsAccA is a known account UUID (no external id in the fixture), so it + // is matched on cloud_account_id only — the uuid set carries it, externals + // is nil. + mockClient.On("QueryHistory", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything). Return([]HistoryDataPoint{}, &HistorySummary{}, nil) mockStore := new(MockConfigStore) @@ -384,7 +387,7 @@ func TestPerAccountPerms_HistoryAnalytics_AllowedAccountSucceeds(t *testing.T) { require.NoError(t, err, "scoped user must be able to query analytics for account-A") require.NotNil(t, result) // Confirm the analytics backend was reached — not short-circuited. - mockClient.AssertCalled(t, "QueryHistory", ctx, permsAccA, mock.Anything, mock.Anything, mock.Anything) + mockClient.AssertCalled(t, "QueryHistory", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything) } // ─── 5. GET /history/breakdown ─────────────────────────────────────────────── @@ -400,7 +403,7 @@ func TestPerAccountPerms_HistoryBreakdown_AllowedAccountSucceeds(t *testing.T) { "ec2": {PurchaseCount: 3, TotalSavings: 150.0}, } mockClient := new(MockAnalyticsClient) - mockClient.On("QueryBreakdown", ctx, permsAccA, mock.Anything, mock.Anything, mock.Anything). + mockClient.On("QueryBreakdown", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything). Return(expectedData, nil) mockStore := new(MockConfigStore) @@ -420,7 +423,7 @@ func TestPerAccountPerms_HistoryBreakdown_AllowedAccountSucceeds(t *testing.T) { }) require.NoError(t, err, "scoped user must be able to query breakdown for account-A") require.NotNil(t, result) - mockClient.AssertCalled(t, "QueryBreakdown", ctx, permsAccA, mock.Anything, mock.Anything, mock.Anything) + mockClient.AssertCalled(t, "QueryBreakdown", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything) } // TestPerAccountPerms_HistoryBreakdown_CrossAccountRejected mirrors the @@ -492,8 +495,14 @@ func TestPerAccountPerms_DashboardSummary_AggregatesAllowedSubsetOnly(t *testing // Guard against future code paths that resolve service configs before filtering: // stub rds so an unexpected GetServiceConfig call doesn't panic the test. mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return((*config.ServiceConfig)(nil), nil) - // calculateCommitmentMetrics calls GetAllPurchaseHistory when no account filter is set. - mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + // Issue #956: a restricted session with no explicit account filter scopes the + // commitment metrics to its allowed_accounts (permsAccA), so the handler calls + // GetPurchaseHistoryFiltered scoped to account A — NOT GetAllPurchaseHistory. + // permsAccA has no ExternalID, so only the UUID half of the filter is set. + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ + AccountIDs: []string{permsAccA}, + Limit: 1000, + }).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil } @@ -512,6 +521,57 @@ func TestPerAccountPerms_DashboardSummary_AggregatesAllowedSubsetOnly(t *testing "dashboard must count only account-A recommendations for a scoped user") assert.Equal(t, 100.0, result.PotentialMonthlySavings, "dashboard savings must reflect only account-A (100); account-B savings (200) must not leak") + // Issue #956 regression: the all-accounts fast path must NOT be taken for a + // restricted session, or commitment KPIs would leak other accounts' data. + mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", mock.Anything, mock.Anything) +} + +// TestPerAccountPerms_DashboardSummary_CommitmentMetricsExcludeOtherAccounts is +// the issue #956 regression: a restricted session with no explicit account +// filter must see commitment KPIs (ActiveCommitments / CommittedMonthly) +// derived only from its allowed account's purchase history. Before the fix the +// handler fell through to GetAllPurchaseHistory, so a scoped user saw every +// account's commitments. Here the store is asked for account A only, and the +// account-B commitment never reaches the KPIs. +func TestPerAccountPerms_DashboardSummary_CommitmentMetricsExcludeOtherAccounts(t *testing.T) { + ctx := context.Background() + + mockSched := new(MockScheduler) + mockSched.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) + + purchaseTime := time.Now().AddDate(0, -2, 0) // active 1-year commitment + // Only account A's purchase history is returned by the scoped query; the + // account-B row (200/mo) is filtered out at the store and must not appear. + accountARows := []config.PurchaseHistoryRecord{ + {CloudAccountID: permsPtr(permsAccA), Timestamp: purchaseTime, Term: 1, Service: "ec2", EstimatedSavings: 100.0}, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.On("GetPurchaseHistoryFiltered", ctx, config.PurchaseHistoryFilter{ + AccountIDs: []string{permsAccA}, + Limit: 1000, + }).Return(accountARows, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return permsAccountList(), nil + } + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{ + auth: scopedAuthMock(ctx), + scheduler: mockSched, + config: mockStore, + } + + result, err := handler.getDashboardSummary(ctx, scopedReq(), map[string]string{}) + require.NoError(t, err) + require.NotNil(t, result) + + assert.Equal(t, 1, result.ActiveCommitments, + "only account-A's commitment may be counted for a scoped session") + assert.Equal(t, 100.0, result.CommittedMonthly, + "account-B commitment (200) must not leak into a scoped session's KPIs") + mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", mock.Anything, mock.Anything) } // ─── 7. POST /purchases/execute ────────────────────────────────────────────── diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index b2da0152a..c0603f761 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -204,8 +204,8 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) } -func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]config.PurchaseHistoryRecord, error) { - args := m.Called(ctx, providerFilter, accountIDs, start, end, limit) +func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, filter) if args.Get(0) == nil { return nil, args.Error(1) } diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 31580221b..41edd8936 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -143,7 +143,111 @@ func (h *Handler) requireExecutionAccess(ctx context.Context, session *Session, return h.requirePlanAccess(ctx, session, execution.PlanID) } -// resolveAccountNamesByID fetches the complete account list once and returns +// resolveAccountFilterIDs maps requested cloud_accounts UUIDs to the +// (uuid, provider, external_id) data needed for the dual-column purchase-history +// filter. purchase_history rows carry either the UUID FK (cloud_account_id) or +// the cloud-provider external number (account_id) and frequently only one of +// them, so a UUID-only WHERE silently drops the external-only rows. This +// resolver loads cloud_accounts once (reusing ListCloudAccounts, the same source +// as resolveAccountNamesByID), then returns the UUIDs unchanged plus the matched +// accounts' external ids grouped by provider. +// +// The external ids are grouped by provider so the downstream predicate compares +// each external number only against rows of its own provider +// ((provider = p AND account_id = ANY(...))). Without this, an external number +// reused across providers (aws/123 vs azure/123) would leak the wrong rows when +// the filter omits provider. +// +// Only UUIDs that resolve to a known cloud_accounts row contribute an external +// id, so a caller cannot inject an arbitrary external id. Unknown UUIDs are +// still returned in the uuid set so the cloud_account_id half of the predicate +// matches any rows that happen to carry them. +// +// Returns (uuids, nil) when uuids is empty or the account load fails — the +// dual-column predicate then degrades to UUID-only matching, no worse than the +// pre-fix behaviour. +func (h *Handler) resolveAccountFilterIDs(ctx context.Context, uuids []string) (resolvedUUIDs []string, externalIDsByProvider map[string][]string) { + if len(uuids) == 0 { + return uuids, nil + } + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) + if err != nil { + return uuids, nil + } + type provExt struct{ provider, externalID string } + byUUID := make(map[string]provExt, len(accounts)) + for _, a := range accounts { + if a.ExternalID != "" { + byUUID[a.ID] = provExt{provider: a.Provider, externalID: a.ExternalID} + } + } + for _, u := range uuids { + pe, ok := byUUID[u] + if !ok { + continue + } + externalIDsByProvider = addExternalIDForProvider(externalIDsByProvider, pe.provider, pe.externalID) + } + return uuids, externalIDsByProvider +} + +// addExternalIDForProvider appends externalID to the provider's slice in m, +// allocating m and the slice lazily and skipping duplicates so each provider's +// ANY() bind has no repeated values. Returns the (possibly newly-allocated) map. +func addExternalIDForProvider(m map[string][]string, provider, externalID string) map[string][]string { + if m == nil { + m = make(map[string][]string) + } + for _, e := range m[provider] { + if e == externalID { + return m + } + } + m[provider] = append(m[provider], externalID) + return m +} + +// resolveSingleAccountFilterIDs resolves a single account identifier (the +// legacy singular `account_id` param, which the frontend populates with the +// top-bar chip's cloud_accounts UUID) into the dual-column filter inputs: +// +// - Known account UUID: returned in the uuid set, plus its external id (if +// the account has one) grouped under the account's provider. Both columns +// are then matched, with the external half scoped to that provider. +// - Unknown value: treated as an opaque external account number (the pre-UUID +// call shape, e.g. a raw AWS account number) so legacy single-account +// callers keep working; it is NOT placed in the uuid set so a raw external +// number is never compared against cloud_account_id UUIDs. Its provider is +// unknown, so it is grouped under the "" key, which the predicate treats as +// an unconstrained-provider match (legacy behaviour preserved). +// +// Empty input returns nil maps (no account filter). A cloud_accounts load +// failure falls back to treating the value as an external id (no worse than the +// pre-fix behaviour); per-record allowed_accounts scoping still applies +// downstream. +func (h *Handler) resolveSingleAccountFilterIDs(ctx context.Context, accountID string) (uuids []string, externalIDsByProvider map[string][]string) { + if accountID == "" { + return nil, nil + } + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{}) + if err != nil { + return nil, map[string][]string{"": {accountID}} + } + for _, a := range accounts { + if a.ID == accountID { + // Known UUID: match cloud_account_id by UUID and, when present, + // account_id by the resolved external number scoped to its provider. + if a.ExternalID != "" { + return []string{accountID}, map[string][]string{a.Provider: {a.ExternalID}} + } + return []string{accountID}, nil + } + } + // Not a known UUID: treat the value as an external account number with an + // unknown provider ("" key = unconstrained provider match). + return nil, map[string][]string{"": {accountID}} +} + // a map from account identifier → display name. The map is keyed by BOTH // the internal UUID (CloudAccount.ID) and the cloud-provider external ID // (CloudAccount.ExternalID, e.g. an AWS account number or Azure subscription @@ -159,7 +263,7 @@ func (h *Handler) resolveAccountNamesByID(ctx context.Context) map[string]string if err != nil { return map[string]string{} } - // Allocate 2× capacity since each account contributes two keys. + // Allocate 2x capacity since each account contributes up to two keys. nameByID := make(map[string]string, len(accounts)*2) for _, a := range accounts { nameByID[a.ID] = a.Name diff --git a/internal/api/types.go b/internal/api/types.go index bd58fc38c..9a787087e 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -88,10 +88,20 @@ type CommitmentOptsInterface interface { Validate(ctx context.Context, provider, service string, term int, payment string) (bool, error) } -// AnalyticsClientInterface defines the interface for analytics queries +// AnalyticsClientInterface defines the interface for analytics queries. +// +// accountUUIDs / accountExternalIDsByProvider are the dual-column account +// filter: rows match when cloud_account_id = ANY(accountUUIDs) OR (provider = p +// AND account_id = ANY(accountExternalIDsByProvider[p])). Both nil/empty means +// "all accounts accessible to the caller" (scoping is enforced upstream in the +// handler). The handler resolves the requested account (a top-bar chip UUID) +// into both representations via resolveSingleAccountFilterIDs so rows that carry +// only the external account_id (cloud_account_id NULL) are still aggregated, and +// the external ids stay grouped by provider so a reused external number across +// providers cannot leak the wrong rows (issue #701/#498/#866). type AnalyticsClientInterface interface { - QueryHistory(ctx context.Context, accountID string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) - QueryBreakdown(ctx context.Context, accountID string, start, end time.Time, dimension string) (map[string]BreakdownValue, error) + QueryHistory(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) + QueryBreakdown(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time, dimension string) (map[string]BreakdownValue, error) } // AnalyticsCollectorInterface defines the interface for analytics collection diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 72c179e56..541a02c7a 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -94,23 +94,21 @@ type StoreInterface interface { GetPurchaseHistory(ctx context.Context, accountID string, limit int) ([]PurchaseHistoryRecord, error) GetAllPurchaseHistory(ctx context.Context, limit int) ([]PurchaseHistoryRecord, error) // GetPurchaseHistoryFiltered reads purchase_history rows matching the - // supplied filter set, newest-first, capped at limit. Each filter is - // applied independently and only when non-empty: - // - providerFilter: matches purchase_history.provider exactly. Empty - // skips the clause. - // - accountIDs: matches purchase_history.cloud_account_id (UUID) with - // ANY($). Empty/nil skips the clause; non-empty excludes legacy - // ambient rows whose cloud_account_id IS NULL (mirrors the - // recommendations filter semantics on issue #211). - // - start/end: bounds purchase_history.timestamp with a BETWEEN. nil - // for both skips the clause; nil for either sets that side open - // (caller is responsible for any range cap, see - // api.MaxHistoryDateRangeDays). - // Added for issue #701: the legacy GetPurchaseHistory / - // GetAllPurchaseHistory pair only accepted a single account_id and the - // limit, so the /api/history handler silently dropped the - // provider/account_ids/start/end query params the frontend was sending. - GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]PurchaseHistoryRecord, error) + // PurchaseHistoryFilter, newest-first, capped at filter.Limit. Each field is + // applied independently and only when populated (see PurchaseHistoryFilter). + // The account predicate matches BOTH identifier columns: + // (cloud_account_id = ANY(AccountIDs) + // OR (provider = $p AND account_id = ANY(ExternalIDsByProvider[p])) OR ...) + // because purchase_history rows carry the cloud_accounts UUID FK + // (cloud_account_id, NULL on direct-execute/ambient/pre-000011 rows) and the + // cloud-provider external number (account_id, always populated) independently. + // The top-bar Account chip emits the UUID; matching only one column silently + // dropped rows that carried only the other (issues #701/#498/#866). The caller + // resolves AccountIDs to their external account numbers grouped by provider + // and populates ExternalIDsByProvider so rows that carry only account_id are + // also matched, while the per-provider grouping keeps a reused external number + // across providers (aws/123 vs azure/123) from leaking the wrong rows. + GetPurchaseHistoryFiltered(ctx context.Context, filter PurchaseHistoryFilter) ([]PurchaseHistoryRecord, error) // RI Exchange history SaveRIExchangeRecord(ctx context.Context, record *RIExchangeRecord) error diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 4380c7af7..c09f2e9b9 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "sort" "strings" "time" @@ -1391,20 +1392,98 @@ func (s *PostgresStore) GetAllPurchaseHistory(ctx context.Context, limit int) ([ return s.queryPurchaseHistory(ctx, query, limit) } +// appendAccountPredicate pulls the dual-column account-predicate arg-building +// branch out of GetPurchaseHistoryFiltered to keep it under the cyclomatic limit. +// +// Either input alone is sufficient: a UUID-only filter still matches +// NULL-cloud_account_id rows via the resolved external IDs, and an +// external-only filter matches UUID-only rows once the caller resolves them. +// Both empty returns conds/args unchanged (no account clause -> all accounts). +// +// The external-id half is grouped per provider so each external number is only +// compared against rows of its own provider: +// +// (cloud_account_id = ANY($u) +// OR (provider = $p1 AND account_id = ANY($e1)) +// OR (provider = $p2 AND account_id = ANY($e2))) +// +// This preserves the (provider, external_id) pairing so a filter for aws/123 +// never matches azure/123 rows. The "" provider key (legacy raw external +// number, unknown provider) matches account_id with no provider gate. Providers +// are sorted for deterministic SQL. The OR is wrapped in parentheses so it +// composes with the surrounding AND chain. +func appendAccountPredicate(conds []string, args []any, accountIDs []string, externalIDsByProvider map[string][]string) ([]string, []any) { + if len(accountIDs) == 0 && len(externalIDsByProvider) == 0 { + return conds, args + } + var ors []string + if len(accountIDs) > 0 { + args = append(args, accountIDs) + ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args))) + } + for _, provider := range sortedProviderKeys(externalIDsByProvider) { + exts := externalIDsByProvider[provider] + if len(exts) == 0 { + continue + } + if provider == "" { + args = append(args, exts) + ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args))) + continue + } + args = append(args, provider) + providerArg := len(args) + args = append(args, exts) + ors = append(ors, fmt.Sprintf("(provider = $%d AND account_id = ANY($%d))", providerArg, len(args))) + } + if len(ors) == 0 { + return conds, args + } + conds = append(conds, "("+strings.Join(ors, " OR ")+")") + return conds, args +} + +// sortedProviderKeys returns the map keys in ascending order so the generated +// SQL (and its bind-arg ordering) is deterministic across calls and testable. +func sortedProviderKeys(m map[string][]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // GetPurchaseHistoryFiltered reads purchase_history rows matching the -// supplied filter set, newest-first, capped at limit. See the StoreInterface -// docstring for the per-filter semantics. Each WHERE clause is appended -// only when its filter is populated, so callers that pass an empty -// providerFilter / nil accountIDs / nil dates get the same plan-shape as -// GetAllPurchaseHistory. Implementation mirrors buildRecommendationFilter +// supplied filter set, newest-first, capped at filter.Limit. See the +// StoreInterface docstring and PurchaseHistoryFilter for the per-field +// semantics. Each WHERE clause is appended only when its field is populated, +// so an empty filter gets the same plan-shape as GetAllPurchaseHistory. +// Implementation mirrors buildRecommendationFilter // (store_postgres_recommendations.go). +// +// The account predicate matches BOTH identifier columns: +// +// (cloud_account_id = ANY($uuids) +// OR (provider = $p AND account_id = ANY($extsForP)) OR ...) +// +// purchase_history carries two account identifiers and either may be the only +// one populated on a given row: cloud_account_id (the cloud_accounts UUID FK, +// added in migration 000011 with no backfill, so NULL on every direct-execute / +// ambient / pre-000011 row) and account_id (the cloud-provider external number, +// e.g. an AWS account number, always populated). The top-bar Account chip emits +// the UUID, so a UUID-only predicate silently dropped every NULL-cloud_account_id +// row (issue #701/#498) while an external-only predicate dropped every row that +// only has the UUID (issue #866). Matching both columns includes rows written by +// either path. The caller resolves the requested UUIDs to their (provider, +// external_id) pairs scoped to the user's accessible accounts and groups the +// external ids by provider, so the external-id half stays provider-scoped and a +// reused external number (aws/123 vs azure/123) cannot leak the wrong rows. func (s *PostgresStore) GetPurchaseHistoryFiltered( ctx context.Context, - providerFilter string, - accountIDs []string, - start, end *time.Time, - limit int, + filter PurchaseHistoryFilter, ) ([]PurchaseHistoryRecord, error) { + limit := filter.Limit if limit <= 0 { limit = DefaultListLimit } @@ -1420,19 +1499,15 @@ func (s *PostgresStore) GetPurchaseHistoryFiltered( conds = append(conds, fmt.Sprintf(cond, len(args)+1)) args = append(args, val) } - if providerFilter != "" { - add("provider = $%d", providerFilter) - } - if len(accountIDs) > 0 { - // NULL cloud_account_id rows are excluded once a list is supplied, - // same as buildRecommendationFilter (issue #211 semantics). - add("cloud_account_id = ANY($%d)", accountIDs) + if filter.Provider != "" { + add("provider = $%d", filter.Provider) } - if start != nil { - add("timestamp >= $%d", *start) + conds, args = appendAccountPredicate(conds, args, filter.AccountIDs, filter.ExternalIDsByProvider) + if filter.Start != nil { + add("timestamp >= $%d", *filter.Start) } - if end != nil { - add("timestamp <= $%d", *end) + if filter.End != nil { + add("timestamp <= $%d", *filter.End) } where := "" diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index d522e29e9..ad4f97dc8 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -515,10 +515,12 @@ func purchaseHistoryRow(now time.Time, provider, acct string) []interface{} { } // TestPGXMock_GetPurchaseHistoryFiltered_AllFilters asserts the SQL emitted -// when every filter is set: WHERE provider = $1 AND cloud_account_id = -// ANY($2) AND timestamp >= $3 AND timestamp <= $4, ORDER BY timestamp DESC, -// LIMIT $5. Each filter's positional argument is bound in declaration order -// (provider, accountIDs, start, end, limit). +// when every filter is set: WHERE provider = $1 AND (cloud_account_id = +// ANY($2) OR (provider = $3 AND account_id = ANY($4))) AND timestamp >= $5 AND +// timestamp <= $6, ORDER BY timestamp DESC, LIMIT $7. The account predicate is +// dual-column with a provider-scoped external-id half so a row carrying only one +// identifier is still matched without leaking across providers (issue +// #701/#498/#866 + provider-coupling). func TestPGXMock_GetPurchaseHistoryFiltered_AllFilters(t *testing.T) { mock := newMock(t) store := storeWith(mock) @@ -530,21 +532,108 @@ func TestPGXMock_GetPurchaseHistoryFiltered_AllFilters(t *testing.T) { rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "acct-1")...) mock.ExpectQuery( - `SELECT account_id, purchase_id, timestamp, provider, service, region.*FROM purchase_history WHERE provider = \$1 AND cloud_account_id = ANY\(\$2\) AND timestamp >= \$3 AND timestamp <= \$4.*ORDER BY timestamp DESC.*LIMIT \$5`, - ).WithArgs("aws", []string{"acct-uuid-1"}, start, end, 50).WillReturnRows(rows) - - records, err := store.GetPurchaseHistoryFiltered(ctx, "aws", []string{"acct-uuid-1"}, &start, &end, 50) + `SELECT account_id, purchase_id, timestamp, provider, service, region.*FROM purchase_history WHERE provider = \$1 AND \(cloud_account_id = ANY\(\$2\) OR \(provider = \$3 AND account_id = ANY\(\$4\)\)\) AND timestamp >= \$5 AND timestamp <= \$6.*ORDER BY timestamp DESC.*LIMIT \$7`, + ).WithArgs("aws", []string{"acct-uuid-1"}, "aws", []string{"111122223333"}, start, end, 50).WillReturnRows(rows) + + records, err := store.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{ + Provider: "aws", + AccountIDs: []string{"acct-uuid-1"}, + ExternalIDsByProvider: map[string][]string{"aws": {"111122223333"}}, + Start: &start, + End: &end, + Limit: 50, + }) require.NoError(t, err) require.Len(t, records, 1) assert.Equal(t, "aws", records[0].Provider) assert.NoError(t, mock.ExpectationsWereMet()) } +// TestPGXMock_GetPurchaseHistoryFiltered_ExternalIDOnly is the keystone +// regression test for issue #701/#498/#866: a UUID-known account whose +// purchase_history rows carry cloud_account_id IS NULL + a populated +// account_id (external number) MUST still be returned when its UUID is +// requested. The handler resolves the UUID to its external id and passes BOTH; +// the dual-column predicate then matches via account_id. Before the fix the +// store filtered cloud_account_id only, so the row was silently dropped and the +// user saw "No purchase history yet" for that account. +func TestPGXMock_GetPurchaseHistoryFiltered_ExternalIDOnly(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + // The matched row has account_id="999988887777" and cloud_account_id NULL + // (purchaseHistoryRow already writes a NULL cloud_account_id). + rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "999988887777")...) + mock.ExpectQuery( + `FROM purchase_history WHERE \(cloud_account_id = ANY\(\$1\) OR \(provider = \$2 AND account_id = ANY\(\$3\)\)\).*ORDER BY timestamp DESC.*LIMIT \$4`, + ).WithArgs([]string{"acct-uuid-B"}, "aws", []string{"999988887777"}, 100).WillReturnRows(rows) + + records, err := store.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{ + AccountIDs: []string{"acct-uuid-B"}, + ExternalIDsByProvider: map[string][]string{"aws": {"999988887777"}}, + Limit: 100, + }) + require.NoError(t, err) + require.Len(t, records, 1, "external-id-only row must be returned when its account is requested") + assert.Equal(t, "999988887777", records[0].AccountID) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetPurchaseHistoryFiltered_MultiProviderExternalIDs asserts that +// when external ids are grouped across two providers, the predicate emits one +// provider-gated OR branch per provider (sorted) so an external id only matches +// rows of its own provider. This is the SQL-side guarantee that a reused +// external number (aws/123 vs azure/123) cannot leak across providers (issue +// #956 CR finding #1). Providers are emitted in sorted order: aws before azure. +func TestPGXMock_GetPurchaseHistoryFiltered_MultiProviderExternalIDs(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "123")...) + mock.ExpectQuery( + `FROM purchase_history WHERE \(\(provider = \$1 AND account_id = ANY\(\$2\)\) OR \(provider = \$3 AND account_id = ANY\(\$4\)\)\).*ORDER BY timestamp DESC.*LIMIT \$5`, + ).WithArgs("aws", []string{"123"}, "azure", []string{"123"}, 100).WillReturnRows(rows) + + records, err := store.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{ + ExternalIDsByProvider: map[string][]string{"aws": {"123"}, "azure": {"123"}}, + Limit: 100, + }) + require.NoError(t, err) + require.Len(t, records, 1) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetPurchaseHistoryFiltered_UUIDOnly asserts a UUID-only account +// filter (no resolvable external id) emits just the cloud_account_id half of +// the predicate, bound to the UUID set. +func TestPGXMock_GetPurchaseHistoryFiltered_UUIDOnly(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "acct-1")...) + mock.ExpectQuery( + `FROM purchase_history WHERE \(cloud_account_id = ANY\(\$1\)\).*ORDER BY timestamp DESC.*LIMIT \$2`, + ).WithArgs([]string{"acct-uuid-1"}, 100).WillReturnRows(rows) + + records, err := store.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{ + AccountIDs: []string{"acct-uuid-1"}, + Limit: 100, + }) + require.NoError(t, err) + require.Len(t, records, 1) + assert.NoError(t, mock.ExpectationsWereMet()) +} + // TestPGXMock_GetPurchaseHistoryFiltered_NoFilters asserts that an -// all-defaults call (empty provider, nil accountIDs, nil dates) emits a -// WHERE-less query identical in shape to GetAllPurchaseHistory. This is -// what proves the handler's fast-path call into the legacy methods stays a -// valid alternative — the filtered variant degrades gracefully. +// all-defaults call (empty filter) emits a WHERE-less query identical in shape +// to GetAllPurchaseHistory. This proves the filtered variant degrades +// gracefully when no fields are set. func TestPGXMock_GetPurchaseHistoryFiltered_NoFilters(t *testing.T) { mock := newMock(t) store := storeWith(mock) @@ -556,17 +645,16 @@ func TestPGXMock_GetPurchaseHistoryFiltered_NoFilters(t *testing.T) { mock.ExpectQuery(`SELECT.*FROM purchase_history\s+ORDER BY timestamp DESC.*LIMIT \$1`). WithArgs(100).WillReturnRows(rows) - records, err := store.GetPurchaseHistoryFiltered(ctx, "", nil, nil, nil, 100) + records, err := store.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{Limit: 100}) require.NoError(t, err) assert.Len(t, records, 1) assert.NoError(t, mock.ExpectationsWereMet()) } // TestPGXMock_GetPurchaseHistoryFiltered_PartialFilters asserts that -// supplying only a subset of filters (here: provider + start, no -// accountIDs, no end) emits exactly two AND clauses and binds the right -// positional arguments. Guards against an off-by-one in the placeholder -// counter (the bug shape mirroring buildRecommendationFilter's add helper). +// supplying only a subset of filters (here: provider + start, no account ids, +// no end) emits exactly two AND clauses and binds the right positional +// arguments. Guards against an off-by-one in the placeholder counter. func TestPGXMock_GetPurchaseHistoryFiltered_PartialFilters(t *testing.T) { mock := newMock(t) store := storeWith(mock) @@ -580,7 +668,11 @@ func TestPGXMock_GetPurchaseHistoryFiltered_PartialFilters(t *testing.T) { `FROM purchase_history WHERE provider = \$1 AND timestamp >= \$2.*ORDER BY timestamp DESC.*LIMIT \$3`, ).WithArgs("azure", start, 25).WillReturnRows(rows) - _, err := store.GetPurchaseHistoryFiltered(ctx, "azure", nil, &start, nil, 25) + _, err := store.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{ + Provider: "azure", + Start: &start, + Limit: 25, + }) require.NoError(t, err) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -599,7 +691,7 @@ func TestPGXMock_GetPurchaseHistoryFiltered_LimitClamp(t *testing.T) { mock.ExpectQuery(`FROM purchase_history\s+ORDER BY timestamp DESC.*LIMIT \$1`). WithArgs(DefaultListLimit).WillReturnRows(rows) - _, err := store.GetPurchaseHistoryFiltered(ctx, "", nil, nil, nil, -5) + _, err := store.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{Limit: -5}) require.NoError(t, err) assert.NoError(t, mock.ExpectationsWereMet()) @@ -610,7 +702,7 @@ func TestPGXMock_GetPurchaseHistoryFiltered_LimitClamp(t *testing.T) { mock2.ExpectQuery(`FROM purchase_history\s+ORDER BY timestamp DESC.*LIMIT \$1`). WithArgs(MaxListLimit).WillReturnRows(rows2) - _, err = store2.GetPurchaseHistoryFiltered(ctx, "", nil, nil, nil, MaxListLimit+1) + _, err = store2.GetPurchaseHistoryFiltered(ctx, PurchaseHistoryFilter{Limit: MaxListLimit + 1}) require.NoError(t, err) assert.NoError(t, mock2.ExpectationsWereMet()) } diff --git a/internal/config/types.go b/internal/config/types.go index eb3402064..ba5127595 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -468,6 +468,41 @@ type RIUtilizationCacheEntry struct { FetchedAt time.Time } +// PurchaseHistoryFilter is the filter set consumed by +// StoreInterface.GetPurchaseHistoryFiltered. Each field is optional; a +// zero-valued filter selects all rows (same plan-shape as +// GetAllPurchaseHistory). See the implementation docstring for the per-field +// semantics and the dual-column account predicate. +type PurchaseHistoryFilter struct { + // Provider matches purchase_history.provider exactly. Empty skips the clause. + Provider string + // AccountIDs matches purchase_history.cloud_account_id (the cloud_accounts + // UUID FK) with ANY($). Empty/nil skips this half of the account predicate. + AccountIDs []string + // ExternalIDsByProvider matches purchase_history.account_id (the + // cloud-provider external account number) scoped per provider. The caller + // resolves AccountIDs to their (provider, external_id) pairs and groups the + // external ids by provider, so the predicate matches each external id only + // against rows of its own provider: + // + // (provider = $p AND account_id = ANY($extsForP)) + // + // This keeps the (provider, external_id) pairing intact so a filter for + // aws/123 never pulls azure/123 rows that reuse the same external number. + // The "" provider key means "provider unknown" (legacy raw external number) + // and matches account_id without a provider gate. Empty/nil skips this half + // of the account predicate. + ExternalIDsByProvider map[string][]string + // Start/End bound purchase_history.timestamp. nil for both skips the clause; + // nil for either leaves that side open (caller owns any range cap, see + // api.MaxHistoryDateRangeDays). + Start *time.Time + End *time.Time + // Limit caps the row count; clamped to [1, MaxListLimit] with a + // DefaultListLimit fallback when <= 0. + Limit int +} + // PurchaseHistoryRecord is the response-layer representation for rows on the // /api/history page. DB-backed rows always describe *completed* purchases; the // handler additionally synthesises rows for pending executions so users can diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index fdbffafa9..ec6b017ba 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -190,8 +190,8 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) } // GetPurchaseHistoryFiltered mocks the GetPurchaseHistoryFiltered operation (issue #701). -func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]config.PurchaseHistoryRecord, error) { - args := m.Called(ctx, providerFilter, accountIDs, start, end, limit) +func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, filter) if args.Get(0) == nil { return nil, args.Error(1) } diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index b34ebfcdb..86cd27dec 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -352,8 +352,8 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) } -func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]config.PurchaseHistoryRecord, error) { - args := m.Called(ctx, providerFilter, accountIDs, start, end, limit) +func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, filter) if args.Get(0) == nil { return nil, args.Error(1) } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index a292ad516..dd3a2ac7e 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -177,8 +177,8 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) } -func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]config.PurchaseHistoryRecord, error) { - args := m.Called(ctx, providerFilter, accountIDs, start, end, limit) +func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, filter) if args.Get(0) == nil { return nil, args.Error(1) } diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index ddd51abe7..a33012bdc 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -99,7 +99,7 @@ func (m *mockConfigStoreForHealth) GetAllPurchaseHistory(ctx context.Context, li return nil, nil } -func (m *mockConfigStoreForHealth) GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]config.PurchaseHistoryRecord, error) { +func (m *mockConfigStoreForHealth) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { return nil, nil }