From 03a3e0a53be522b8b18cc6e26bf664e3caea57d3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 09:41:34 +0100 Subject: [PATCH 1/2] fix(history): apply the plan_id filter the plan-history view sends The plan-history view requests /api/history?plan_id=, but parseHistoryFilters never read it, so the view listed and totaled every in-scope purchase instead of the plan's own. Parse plan_id (400 on a non-UUID) and apply it to the purchase_history row listings (purchase_history.plan_id), the summary aggregate's purchase and execution halves (purchase_executions.plan_id), and the in-memory execution filter. The predicate is parameterised and ANDed with the account and provider filters, so it narrows and never widens the caller's scope. Also split the limit parsing out of parseHistoryFilters to keep it under the gocyclo limit. Closes #1404 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history.go | 60 ++++++++-- .../api/handler_history_plan_filter_test.go | 82 +++++++++++++ internal/config/store_postgres.go | 3 + ...es_history_plan_filter_integration_test.go | 113 ++++++++++++++++++ .../config/store_postgres_history_summary.go | 4 + internal/config/types.go | 3 + 6 files changed, 252 insertions(+), 13 deletions(-) create mode 100644 internal/api/handler_history_plan_filter_test.go create mode 100644 internal/config/store_postgres_history_plan_filter_integration_test.go diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index b4371daa5..feeba2d0c 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -721,9 +721,12 @@ type historyFilters struct { ExternalIDsByProvider map[string][]string Provider string LegacyAccountID string - AccountIDs []string - Limit int - HasDate bool + // PlanID narrows the history to one purchase plan's purchases and + // executions (the plan-history view's plan_id); empty means no plan filter. + PlanID string + AccountIDs []string + Limit int + HasDate bool // Cursor is the stream position the page starts after (issue #674); nil // for the first page. Cursor *config.HistoryPageKey @@ -752,6 +755,12 @@ func parseHistoryFilters(params map[string]string) (historyFilters, error) { f.AccountIDs = accountIDs f.LegacyAccountID = params["account_id"] + planID, err := parseHistoryPlanID(params["plan_id"]) + if err != nil { + return f, err + } + f.PlanID = planID + start, end, hasDate, err := parseHistoryDateRange(params["start"], params["end"]) if err != nil { return f, err @@ -760,11 +769,29 @@ func parseHistoryFilters(params map[string]string) (historyFilters, error) { f.Start = start f.End = end + limit, err := parseHistoryLimit(params["limit"]) + if err != nil { + return f, err + } + f.Limit = limit + + cursor, pagingErr := parseHistoryPaging(params) + if pagingErr != nil { + return f, pagingErr + } + f.Cursor = cursor + + return f, nil +} + +// parseHistoryLimit parses the optional limit, clamped to [1, MaxListLimit] +// with DefaultListLimit for empty or non-positive values. +func parseHistoryLimit(raw string) (int, error) { limit := config.DefaultListLimit - if s := params["limit"]; s != "" { - n, err := strconv.Atoi(s) + if raw != "" { + n, err := strconv.Atoi(raw) if err != nil { - return f, NewClientError(400, "invalid limit: must be a positive integer") + return 0, NewClientError(400, "invalid limit: must be a positive integer") } limit = n } @@ -774,15 +801,18 @@ func parseHistoryFilters(params map[string]string) (historyFilters, error) { if limit > config.MaxListLimit { limit = config.MaxListLimit } - f.Limit = limit + return limit, nil +} - cursor, pagingErr := parseHistoryPaging(params) - if pagingErr != nil { - return f, pagingErr +// parseHistoryPlanID validates the optional plan_id filter; empty means none. +func parseHistoryPlanID(raw string) (string, error) { + if raw == "" { + return "", nil } - f.Cursor = cursor - - return f, nil + if err := validateUUID(raw); err != nil { + return "", NewClientError(400, "invalid plan_id: must be a UUID") + } + return raw, nil } // parseHistoryDateRange parses optional YYYY-MM-DD start/end strings. Both @@ -878,6 +908,9 @@ func (f historyFilters) matchesExecution(exec config.PurchaseExecution) bool { if !f.matchesExecutionProviderAndAccount(exec) { return false } + if f.PlanID != "" && exec.PlanID != f.PlanID { + return false + } if f.HasDate { if exec.ScheduledDate.Before(f.Start) || exec.ScheduledDate.After(f.End) { return false @@ -1016,6 +1049,7 @@ func (h *Handler) fetchPurchaseHistory(ctx context.Context, filters *historyFilt func (f historyFilters) purchaseHistoryFilter() config.PurchaseHistoryFilter { out := config.PurchaseHistoryFilter{ Provider: f.Provider, + PlanID: f.PlanID, AccountIDs: f.AccountIDs, ExternalIDsByProvider: f.ExternalIDsByProvider, Limit: f.Limit, diff --git a/internal/api/handler_history_plan_filter_test.go b/internal/api/handler_history_plan_filter_test.go new file mode 100644 index 000000000..91ee5826f --- /dev/null +++ b/internal/api/handler_history_plan_filter_test.go @@ -0,0 +1,82 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const planFilterUUID = "11111111-1111-4111-8111-111111111111" + +func TestParseHistoryFilters_PlanID(t *testing.T) { + f, err := parseHistoryFilters(map[string]string{"plan_id": planFilterUUID}) + require.NoError(t, err) + assert.Equal(t, planFilterUUID, f.PlanID) + + f, err = parseHistoryFilters(map[string]string{}) + require.NoError(t, err) + assert.Empty(t, f.PlanID, "absent plan_id means no plan filter") + + for _, bad := range []string{"not-a-uuid", "' OR 1=1 --", "11111111-1111-4111-8111-11111111111"} { + _, err = parseHistoryFilters(map[string]string{"plan_id": bad}) + require.Error(t, err, bad) + ce, ok := IsClientError(err) + require.True(t, ok, "got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "invalid plan_id") + } +} + +func TestHistoryFilters_PlanIDMatchesExecution(t *testing.T) { + f := historyFilters{PlanID: planFilterUUID} + assert.True(t, f.matchesExecution(config.PurchaseExecution{PlanID: planFilterUUID})) + assert.False(t, f.matchesExecution(config.PurchaseExecution{PlanID: "22222222-2222-4222-8222-222222222222"})) + assert.False(t, f.matchesExecution(config.PurchaseExecution{}), "a planless execution is not in any plan's history") + assert.True(t, historyFilters{}.matchesExecution(config.PurchaseExecution{}), "no plan filter keeps every execution") +} + +// TestGetHistory_PlanIDReachesStoreFilters checks that plan_id reaches both the +// row fetch and the summary aggregate, and that an absent plan_id sets neither. +func TestGetHistory_PlanIDReachesStoreFilters(t *testing.T) { + for _, tc := range []struct{ name, planID string }{{"with plan", planFilterUUID}, {"without plan", ""}} { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + var rowFilter config.PurchaseHistoryFilter + mockStore.On("ListPurchaseHistoryPage", ctx, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { rowFilter = args.Get(1).(config.PurchaseHistoryFilter) }). + Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil) + var aggFilter config.HistorySummaryFilter + mockStore.On("HistorySummaryAggregate", ctx, mock.Anything). + Run(func(args mock.Arguments) { aggFilter = args.Get(1).(config.HistorySummaryFilter) }). + Return(&config.HistorySummaryGroups{}, nil) + + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + params := map[string]string{} + if tc.planID != "" { + params["plan_id"] = tc.planID + } + _, err := handler.getHistory(ctx, req, params) + require.NoError(t, err) + assert.Equal(t, tc.planID, rowFilter.PlanID) + assert.Equal(t, tc.planID, aggFilter.History.PlanID) + }) + } +} + +func TestGetHistory_BadPlanIDIs400(t *testing.T) { + ctx := context.Background() + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: new(MockConfigStore)} + _, err := handler.getHistory(ctx, req, map[string]string{"plan_id": "garbage"}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b94c317d6..e31f7c9c8 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -2680,6 +2680,9 @@ func purchaseHistoryFilterWhere(filter PurchaseHistoryFilter) (where string, arg if filter.Provider != "" { add("provider = $%d", filter.Provider) } + if filter.PlanID != "" { + add("plan_id = $%d", filter.PlanID) + } conds, args = appendAccountPredicate(conds, args, filter.AccountIDs, filter.ExternalIDsByProvider) if filter.Start != nil { add("timestamp >= $%d", *filter.Start) diff --git a/internal/config/store_postgres_history_plan_filter_integration_test.go b/internal/config/store_postgres_history_plan_filter_integration_test.go new file mode 100644 index 000000000..5fe1cbfd8 --- /dev/null +++ b/internal/config/store_postgres_history_plan_filter_integration_test.go @@ -0,0 +1,113 @@ +//go:build integration + +package config + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func createHistoryTestPlan(ctx context.Context, t *testing.T, store *PostgresStore, name string) string { + t.Helper() + next := time.Now().Add(time.Hour).Truncate(time.Microsecond) + plan := &PurchasePlan{ + Name: name, Enabled: true, + Services: map[string]ServiceConfig{"ec2": {Provider: "aws", Service: "ec2", Enabled: true}}, + RampSchedule: RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -7)}, + NextExecutionDate: &next, + } + require.NoError(t, plan.Validate()) + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + return plan.ID +} + +// TestIntegration_HistoryPlanIDFilter pins the plan-history view's plan_id +// filter (issue #1404): the row listings and the summary aggregate return only +// that plan's purchases and executions, it narrows (never widens) the account +// filter, and no plan filter returns everything including planless rows. +func TestIntegration_HistoryPlanIDFilter(t *testing.T) { + ctx := context.Background() + f := newHistorySummaryFixture(ctx, t) + planA := createHistoryTestPlan(ctx, t, f.store, "history-plan-a") + planB := createHistoryTestPlan(ctx, t, f.store, "history-plan-b") + a, b := f.accountA, f.accountB + + savePlanPurchase := func(day int, planID, provider, extID string, cloudAccountID *string) string { + rec := &PurchaseHistoryRecord{ + AccountID: extID, PurchaseID: uuid.NewString(), Timestamp: f.base.Add(time.Duration(day) * 24 * time.Hour), + Provider: provider, Service: "ec2", Region: "us-east-1", ResourceType: "t3.micro", Count: 1, Term: 1, + Payment: "no-upfront", EstimatedSavings: 1, PlanID: planID, CloudAccountID: cloudAccountID, + } + require.NoError(t, f.store.SavePurchaseHistory(ctx, rec)) + return rec.PurchaseID + } + pa1 := savePlanPurchase(1, planA, "aws", "ext-sum-a", &a) + pa2 := savePlanPurchase(2, planA, "azure", "ext-sum-b", &b) + savePlanPurchase(3, planB, "aws", "ext-sum-a", &a) + savePlanPurchase(4, "", "aws", "ext-sum-a", &a) // not tied to any plan + + saveExec := func(planID, status string) { + exec := &PurchaseExecution{ + PlanID: planID, ExecutionID: uuid.NewString(), IdempotencyKey: uuid.NewString(), Status: status, + Error: "boom", ScheduledDate: f.base.Add(time.Hour), CloudAccountID: &a, + } + require.NoError(t, f.store.SavePurchaseExecution(ctx, exec)) + } + saveExec(planA, "failed") + saveExec(planB, "failed") + saveExec(planB, "failed") + + purchaseIDs := func(filter PurchaseHistoryFilter) []string { + t.Helper() + filter.Limit = MaxListLimit + page, err := f.store.ListPurchaseHistoryPage(ctx, filter, nil) + require.NoError(t, err) + listed, err := f.store.GetPurchaseHistoryFiltered(ctx, filter) + require.NoError(t, err) + require.Len(t, listed, len(page), "the two listings share one predicate") + ids := make([]string, 0, len(page)) + for i := range page { + ids = append(ids, page[i].PurchaseID) + } + return ids + } + totals := func(filter PurchaseHistoryFilter) (purchases, executions int) { + t.Helper() + agg, err := f.store.HistorySummaryAggregate(ctx, HistorySummaryFilter{History: filter, ExecutionStatuses: []string{"failed"}}) + require.NoError(t, err) + for i := range agg.Purchases { + purchases += agg.Purchases[i].Count + } + for i := range agg.Executions { + executions += agg.Executions[i].Count + } + return purchases, executions + } + + assert.ElementsMatch(t, []string{pa1, pa2}, purchaseIDs(PurchaseHistoryFilter{PlanID: planA})) + p, e := totals(PurchaseHistoryFilter{PlanID: planA}) + assert.Equal(t, 2, p) + assert.Equal(t, 1, e) + p, e = totals(PurchaseHistoryFilter{PlanID: planB}) + assert.Equal(t, 1, p) + assert.Equal(t, 2, e) + + // The plan filter narrows the account filter instead of replacing it. + assert.Equal(t, []string{pa1}, purchaseIDs(PurchaseHistoryFilter{PlanID: planA, AccountIDs: []string{a}})) + p, _ = totals(PurchaseHistoryFilter{PlanID: planA, AccountIDs: []string{a}}) + assert.Equal(t, 1, p) + + // A plan with no rows matches nothing; no plan filter matches everything. + assert.Empty(t, purchaseIDs(PurchaseHistoryFilter{PlanID: uuid.NewString()})) + p, e = totals(PurchaseHistoryFilter{PlanID: uuid.NewString()}) + assert.Zero(t, p+e) + assert.Len(t, purchaseIDs(PurchaseHistoryFilter{}), 4) + p, e = totals(PurchaseHistoryFilter{}) + assert.Equal(t, 4, p) + assert.Equal(t, 3, e) +} diff --git a/internal/config/store_postgres_history_summary.go b/internal/config/store_postgres_history_summary.go index aa7571fc3..275baed74 100644 --- a/internal/config/store_postgres_history_summary.go +++ b/internal/config/store_postgres_history_summary.go @@ -105,6 +105,10 @@ const executionSummaryRecsKey = `CASE WHEN jsonb_typeof(recommendations) = 'arra func executionSummaryQuery(filter HistorySummaryFilter) (query string, args []any) { where := historyExecutionsPredicate args = []any{filter.ExecutionStatuses} + if filter.History.PlanID != "" { + args = append(args, filter.History.PlanID) + where += fmt.Sprintf(" AND plan_id = $%d", len(args)) + } if filter.ExecutionStart != nil { args = append(args, *filter.ExecutionStart) where += fmt.Sprintf(" AND scheduled_date >= $%d", len(args)) diff --git a/internal/config/types.go b/internal/config/types.go index 06b931371..5cbcb2d8e 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -955,6 +955,9 @@ type RIUtilizationCacheEntry struct { type PurchaseHistoryFilter struct { // Provider matches purchase_history.provider exactly. Empty skips the clause. Provider string + // PlanID matches purchase_history.plan_id exactly; HistorySummaryAggregate + // also applies it to purchase_executions.plan_id. Empty skips the clause. + PlanID 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 From e3ae7e145d6265e5fd2297e69fc6f77c425949d9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 10:29:26 +0100 Subject: [PATCH 2/2] fix(history): canonicalise plan_id and filter execution pages in SQL Trim and lowercase plan_id so the SQL predicate and the in-memory execution check compare the same text. Push the plan into the execution page queries (terminal stream and the actionable class) as a parameterised plan_id predicate, so a plan view does not walk every batch and other plans' actionable rows over MaxListLimit cannot drop the plan's own pending row. The Go check stays as a guard for stores that do not filter. Add real-Postgres tests for canonical plan_id, the actionable cap and a cross-tenant plan_id (indistinguishable from a random one), and make the no-filter unit case use an execution that carries a PlanID. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history.go | 25 ++- internal/api/handler_history_page.go | 6 +- ...er_history_plan_filter_integration_test.go | 176 ++++++++++++++++++ .../api/handler_history_plan_filter_test.go | 7 +- .../config/store_postgres_history_page.go | 6 + 5 files changed, 210 insertions(+), 10 deletions(-) create mode 100644 internal/api/handler_history_plan_filter_integration_test.go diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index feeba2d0c..a05c2214d 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -6,6 +6,7 @@ import ( "fmt" "sort" "strconv" + "strings" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" @@ -192,7 +193,7 @@ const approvalExpiryWindow = 7 * 24 * time.Hour // the class reached its MaxListLimit cap, so older executions may have been // dropped before the filters ran, or its listing failed (issue #686). func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyFilters) ([]config.PurchaseHistoryRecord, []config.PurchaseExecution, bool) { - executions, ok := h.fetchExecutionsByClass(ctx, actionableExecutionStatuses, config.MaxListLimit) + executions, ok := h.fetchExecutionsByClass(ctx, actionableExecutionStatuses, filters.PlanID, config.MaxListLimit) incomplete := !ok || len(executions) >= config.MaxListLimit if len(executions) == 0 { return nil, nil, incomplete @@ -222,8 +223,17 @@ func (h *Handler) fetchExecutionsAsHistory(ctx context.Context, filters historyF // the class skipped (ok=false) so the other class and completed history still // render; the caller flags the response as incomplete so the hidden rows are // not silent (issue #686). -func (h *Handler) fetchExecutionsByClass(ctx context.Context, statuses []string, limit int) ([]config.PurchaseExecution, bool) { - rows, err := h.config.GetExecutionsByStatuses(ctx, statuses, limit) +// +// A non-empty planID selects only that plan's executions in SQL, so a plan view +// is not crowded out by other plans' rows under the limit. +func (h *Handler) fetchExecutionsByClass(ctx context.Context, statuses []string, planID string, limit int) ([]config.PurchaseExecution, bool) { + var rows []config.PurchaseExecution + var err error + if planID != "" { + rows, err = h.config.ListHistoryExecutionsPage(ctx, config.HistoryExecutionsPageQuery{Statuses: statuses, PlanID: planID, Limit: limit}) + } else { + rows, err = h.config.GetExecutionsByStatuses(ctx, statuses, limit) + } if err != nil { logging.Warnf("history: failed to load executions with statuses %v: %v", statuses, err) return nil, false @@ -805,14 +815,17 @@ func parseHistoryLimit(raw string) (int, error) { } // parseHistoryPlanID validates the optional plan_id filter; empty means none. +// The value is trimmed and lowercased so the SQL predicate and the in-memory +// execution check compare the same canonical text. func parseHistoryPlanID(raw string) (string, error) { - if raw == "" { + id := strings.TrimSpace(raw) + if id == "" { return "", nil } - if err := validateUUID(raw); err != nil { + if err := validateUUID(id); err != nil { return "", NewClientError(400, "invalid plan_id: must be a UUID") } - return raw, nil + return strings.ToLower(id), nil } // parseHistoryDateRange parses optional YYYY-MM-DD start/end strings. Both diff --git a/internal/api/handler_history_page.go b/internal/api/handler_history_page.go index ae4076943..f5671d1d6 100644 --- a/internal/api/handler_history_page.go +++ b/internal/api/handler_history_page.go @@ -245,10 +245,10 @@ func (st *historyStream) fetchPurchases(ctx context.Context, after *config.Histo // fetchTerminalExecutions reads a batch of terminal executions and projects // the ones matching the request's filters into History rows. The date bound -// is pushed into SQL as well as checked by matchesExecution; provider and -// account are checked in Go only, like the actionable class. +// is pushed into SQL as well as checked by matchesExecution; the plan is pushed into +// SQL too; provider and account are checked in Go only, like the actionable class. func (st *historyStream) fetchTerminalExecutions(ctx context.Context, after *config.HistoryPageKey) ([]historyStreamRow, error) { - q := config.HistoryExecutionsPageQuery{Statuses: terminalExecutionStatuses, After: after, Limit: st.executions.batch} + q := config.HistoryExecutionsPageQuery{Statuses: terminalExecutionStatuses, PlanID: st.filters.PlanID, After: after, Limit: st.executions.batch} if st.filters.HasDate { start, end := st.filters.Start, st.filters.End q.Start, q.End = &start, &end diff --git a/internal/api/handler_history_plan_filter_integration_test.go b/internal/api/handler_history_plan_filter_integration_test.go new file mode 100644 index 000000000..1af99fabf --- /dev/null +++ b/internal/api/handler_history_plan_filter_integration_test.go @@ -0,0 +1,176 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +type planFilterEnv struct { + *historySummaryEnv + container *testhelpers.PostgresContainer + planA, planB string + admin, scoped *Handler +} + +func (e *planFilterEnv) createPlan(ctx context.Context, t *testing.T, name string) string { + t.Helper() + next := time.Now().Add(time.Hour).Truncate(time.Microsecond) + plan := &config.PurchasePlan{ + Name: name, Enabled: true, + Services: map[string]config.ServiceConfig{"ec2": {Provider: "aws", Service: "ec2", Enabled: true}}, + RampSchedule: config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -7)}, + NextExecutionDate: &next, + } + require.NoError(t, plan.Validate()) + require.NoError(t, e.store.CreatePurchasePlan(ctx, plan)) + return plan.ID +} + +func (e *planFilterEnv) addPlanPurchase(ctx context.Context, t *testing.T, tenant, planID string, at time.Time) { + t.Helper() + acct := e.account(tenant) + up := 10.0 + require.NoError(t, e.store.SavePurchaseHistory(ctx, &config.PurchaseHistoryRecord{ + AccountID: acct.ExternalID, PurchaseID: uuid.NewString(), Timestamp: at, Provider: "aws", Service: "ec2", + Region: "us-east-1", ResourceType: "t3.micro", Count: 1, Term: 1, Payment: "no-upfront", + UpfrontCost: &up, EstimatedSavings: 1, PlanID: planID, CloudAccountID: &acct.ID, + })) +} + +func (e *planFilterEnv) addPlanExecution(ctx context.Context, t *testing.T, tenant, planID, status, execErr string, at time.Time) { + t.Helper() + exec := &config.PurchaseExecution{ + PlanID: planID, ExecutionID: uuid.NewString(), IdempotencyKey: uuid.NewString(), Status: status, Error: execErr, + ScheduledDate: at, TotalUpfrontCost: 500, EstimatedSavings: 50, + } + if tenant != "" { + id := e.account(tenant).ID + exec.CloudAccountID = &id + exec.Recommendations = []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1", CloudAccountID: &id}} + } + require.NoError(t, e.store.SavePurchaseExecution(ctx, exec)) +} + +// newPlanFilterEnv seeds plan A (tenant A) and plan B (tenant B), each with two +// purchases, a failed and a pending execution, plus tenant A rows tied to no +// plan. The pending rows are recent, so the stale-expiry sweep leaves them. +func newPlanFilterEnv(ctx context.Context, t *testing.T) *planFilterEnv { + t.Helper() + base, container := setupHistorySummaryEnv(ctx, t) + e := &planFilterEnv{historySummaryEnv: base, container: container} + e.planA, e.planB = e.createPlan(ctx, t, "plan-a"), e.createPlan(ctx, t, "plan-b") + at := time.Now().UTC().Add(-3 * 24 * time.Hour).Truncate(time.Second) + for i, p := range []struct{ tenant, plan string }{{"a", e.planA}, {"b", e.planB}, {"a", ""}} { + o := time.Duration(i) * time.Minute + e.addPlanPurchase(ctx, t, p.tenant, p.plan, at.Add(o)) + e.addPlanPurchase(ctx, t, p.tenant, p.plan, at.Add(o+time.Hour)) + e.addPlanExecution(ctx, t, p.tenant, p.plan, "failed", "boom", at.Add(o+2*time.Hour)) + e.addPlanExecution(ctx, t, p.tenant, p.plan, "pending", "", at.Add(o+3*time.Hour)) + } + scopedAuth := new(MockAuthService) + scopedAuth.On("ValidateSession", mock.Anything, scopedToken).Return(&Session{UserID: scopedUserID}, nil).Maybe() + scopedAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() + scopedAuth.grantScoped(e.tenantA.Name) + adminAuth := new(MockAuthService) + adminAuth.On("GetUser", mock.Anything, mock.Anything).Return(&User{Email: "creator@example.com"}, nil).Maybe() + adminAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"}, nil).Maybe() + adminAuth.grantAdmin() + e.admin = &Handler{config: e.store, auth: adminAuth} + e.scoped = &Handler{config: e.store, auth: scopedAuth} + return e +} + +func historyBody(ctx context.Context, t *testing.T, h *Handler, token string, params map[string]string) (HistoryResponse, string) { + t.Helper() + resp, _ := getHistoryJSON(ctx, t, h, token, params) + raw, err := json.Marshal(resp) + require.NoError(t, err) + return resp, string(raw) +} + +func assertAllPlan(t *testing.T, resp HistoryResponse, plan string) { + t.Helper() + for i := range resp.Purchases { + assert.Equal(t, plan, resp.Purchases[i].PlanID, "row %s", resp.Purchases[i].PurchaseID) + } +} + +// TestIntegration_HistoryPlanFilter_Canonicalised: an uppercase or padded +// plan_id selects the same purchases and executions as the lowercase one, so +// the SQL predicate and the in-memory execution check agree. +func TestIntegration_HistoryPlanFilter_Canonicalised(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + e := newPlanFilterEnv(ctx, t) + + resp, want := historyBody(ctx, t, e.admin, "admin-token", map[string]string{"plan_id": e.planA}) + require.Len(t, resp.Purchases, 4, "2 purchases + failed + pending execution of plan A") + assertAllPlan(t, resp, e.planA) + assert.Equal(t, 4, resp.Summary.TotalPurchases) + for _, variant := range []string{strings.ToUpper(e.planA), " " + e.planA + " "} { + _, got := historyBody(ctx, t, e.admin, "admin-token", map[string]string{"plan_id": variant}) + assert.Equal(t, want, got, variant) + } + all, _ := historyBody(ctx, t, e.admin, "admin-token", map[string]string{}) + assert.Len(t, all.Purchases, 12, "no plan filter returns every plan and planless rows") +} + +// TestIntegration_HistoryPlanFilter_CrossTenant: a user scoped to tenant A who +// passes tenant B's plan_id gets exactly what a random plan_id gets, and +// tenant A's own plan returns only tenant A's rows. +func TestIntegration_HistoryPlanFilter_CrossTenant(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + e := newPlanFilterEnv(ctx, t) + + adminB, _ := historyBody(ctx, t, e.admin, "admin-token", map[string]string{"plan_id": e.planB}) + require.Len(t, adminB.Purchases, 4, "tenant B's plan is not empty for an unrestricted viewer") + + foreign, foreignBody := historyBody(ctx, t, e.scoped, scopedToken, map[string]string{"plan_id": e.planB}) + _, randomBody := historyBody(ctx, t, e.scoped, scopedToken, map[string]string{"plan_id": uuid.NewString()}) + assert.Empty(t, foreign.Purchases) + assertSummaryEqual(t, HistorySummary{TotalUpfront: new(float64)}, foreign.Summary) + assert.Equal(t, randomBody, foreignBody, "another tenant's plan must be indistinguishable from a missing one") + + own, _ := historyBody(ctx, t, e.scoped, scopedToken, map[string]string{"plan_id": e.planA}) + require.Len(t, own.Purchases, 4) + assertAllPlan(t, own, e.planA) + assert.Equal(t, 4, own.Summary.TotalPurchases) +} + +// TestIntegration_HistoryPlanFilter_ActionableOverCap: more than MaxListLimit +// other-plan actionable executions, all newer, must not push the plan's own +// pending execution out of the listing. +func TestIntegration_HistoryPlanFilter_ActionableOverCap(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + e := newPlanFilterEnv(ctx, t) + _, err := e.container.DB.Pool().Exec(ctx, ` + INSERT INTO purchase_executions (plan_id, status, scheduled_date, idempotency_key) + SELECT $1, 'approved', now() - interval '1 hour' - g * interval '1 second', gen_random_uuid()::text + FROM generate_series(1, $2) g`, e.planB, config.MaxListLimit+5) + require.NoError(t, err) + + resp, _ := historyBody(ctx, t, e.admin, "admin-token", map[string]string{"plan_id": e.planA}) + require.Len(t, resp.Purchases, 4) + assertAllPlan(t, resp, e.planA) + pending := 0 + for i := range resp.Purchases { + if resp.Purchases[i].Status == "pending" { + pending++ + } + } + assert.Equal(t, 1, pending, "the plan's pending execution survives the other plan's rows") +} diff --git a/internal/api/handler_history_plan_filter_test.go b/internal/api/handler_history_plan_filter_test.go index 91ee5826f..76328b00b 100644 --- a/internal/api/handler_history_plan_filter_test.go +++ b/internal/api/handler_history_plan_filter_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -21,6 +22,10 @@ func TestParseHistoryFilters_PlanID(t *testing.T) { require.NoError(t, err) assert.Empty(t, f.PlanID, "absent plan_id means no plan filter") + f, err = parseHistoryFilters(map[string]string{"plan_id": " " + strings.ToUpper(planFilterUUID) + "\t"}) + require.NoError(t, err) + assert.Equal(t, planFilterUUID, f.PlanID, "padded and uppercase plan_id is canonicalised") + for _, bad := range []string{"not-a-uuid", "' OR 1=1 --", "11111111-1111-4111-8111-11111111111"} { _, err = parseHistoryFilters(map[string]string{"plan_id": bad}) require.Error(t, err, bad) @@ -36,7 +41,7 @@ func TestHistoryFilters_PlanIDMatchesExecution(t *testing.T) { assert.True(t, f.matchesExecution(config.PurchaseExecution{PlanID: planFilterUUID})) assert.False(t, f.matchesExecution(config.PurchaseExecution{PlanID: "22222222-2222-4222-8222-222222222222"})) assert.False(t, f.matchesExecution(config.PurchaseExecution{}), "a planless execution is not in any plan's history") - assert.True(t, historyFilters{}.matchesExecution(config.PurchaseExecution{}), "no plan filter keeps every execution") + assert.True(t, historyFilters{}.matchesExecution(config.PurchaseExecution{PlanID: planFilterUUID}), "no plan filter keeps a plan's execution") } // TestGetHistory_PlanIDReachesStoreFilters checks that plan_id reaches both the diff --git a/internal/config/store_postgres_history_page.go b/internal/config/store_postgres_history_page.go index 2f5895534..fa5092a24 100644 --- a/internal/config/store_postgres_history_page.go +++ b/internal/config/store_postgres_history_page.go @@ -68,6 +68,8 @@ type HistoryExecutionsPageQuery struct { // Statuses the rows must have. Clean completed rows (status completed, // empty error) are always excluded, as in GetExecutionsByStatuses. Statuses []string + // PlanID matches purchase_executions.plan_id exactly; empty skips the clause. + PlanID string // Start and End bound scheduled_date, inclusive; nil leaves a side open. Start *time.Time End *time.Time @@ -194,6 +196,10 @@ func (s *PostgresStore) ListHistoryExecutionsPage(ctx context.Context, q History // HistorySummaryAggregate, so the stream and the summary select the same // executions (issue #675). conds := []string{historyExecutionsPredicate} + if q.PlanID != "" { + args = append(args, q.PlanID) + conds = append(conds, fmt.Sprintf("plan_id = $%d", len(args))) + } if q.Start != nil { args = append(args, *q.Start) conds = append(conds, fmt.Sprintf("scheduled_date >= $%d", len(args)))