From cf2cfe9f8da68bd7cd60af418feb50582621cb5f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 11:59:54 +0200 Subject: [PATCH] fix(api): scope plans list, inventory filter and RI quote to allowed accounts GET /api/plans never resolved the caller's account scope, so a scoped user listed every tenant's plans, and account_ids naming another tenant narrowed the list to that tenant. The store query also ORed in every plan with no plan_accounts row regardless of the filter, so unassigned plans leaked to any account-filtered caller. - listPlans intersects account_ids with allowed_accounts through intersectAccountFilterScope and returns an empty list when nothing is left. Unassigned plans stay visible only to unrestricted callers, matching requirePlanAccess. - PurchasePlanFilter.IncludeUnassigned gates the unassigned disjunct in buildListPlansQuery; only unrestricted callers set it. - fetchCommitmentRecords routes an explicit account_id through intersectAccountFilterScope, so an out-of-scope account is no longer read from the store (the callers' post-filter already hid the rows). - getExchangeQuote applies requireReshapeAccountScope, the gate executeExchange uses, so a scoped user cannot quote the deployment account's RIs when it is outside their scope. Closes #29 Closes #384 --- internal/api/handler_inventory.go | 42 ++-- internal/api/handler_plans.go | 29 ++- internal/api/handler_plans_test.go | 18 +- internal/api/handler_ri_exchange.go | 8 +- internal/api/tenant_scope_sweep_test.go | 198 ++++++++++++++++++ internal/config/store_postgres.go | 28 +-- .../config/store_postgres_pgxmock_test.go | 2 +- .../config/store_postgres_unassigned_test.go | 17 +- internal/config/types.go | 3 + 9 files changed, 292 insertions(+), 53 deletions(-) create mode 100644 internal/api/tenant_scope_sweep_test.go diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 59a6af2b7..7a2bba928 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -87,37 +87,27 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF // #701/#498/#866 bug was that passing the UUID to the single-column // GetPurchaseHistory (account_id) matched nothing. // -// When no explicit `account_id` is supplied, a restricted session's -// allowed_accounts scope is pushed into the SQL read via -// resolveAllowedAccountScope, the same contract as the dashboard KPI path -// (issue #956): without it the store would read every tenant's active -// commitments and only trim them in memory afterwards. A restricted session -// whose allowed_accounts match no account resolves to the non-nil-but-empty -// sentinel and short-circuits to an empty result without querying (an empty -// scope sent to the store would match ALL rows, not none). Unrestricted / -// admin sessions resolve to a nil scope and keep the all-accounts read. The +// The filter is then narrowed to the session's allowed_accounts by +// intersectAccountFilterScope, the same contract as the dashboard KPI path +// (issues #956, #384): a restricted session's scope is pushed into the SQL +// read, and an explicit account_id outside it is dropped rather than queried. +// An empty intersection resolves to the non-nil-but-empty sentinel and +// short-circuits to an empty result without querying (an empty scope sent to +// the store would match ALL rows, not none). Unrestricted / admin sessions +// keep the caller's filter, or the all-accounts read without one. The // in-memory filterPurchaseHistoryByAllowedAccounts in the callers still runs -// afterwards; it is what trims an explicit account_id outside the session's -// scope and serves as defense in depth for the no-param path. +// afterwards as defense in depth. // // `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, asOf time.Time, session *Session, params map[string]string) ([]config.PurchaseHistoryRecord, error) { - var uuids []string - var externalIDsByProvider map[string][]string - if accountID := params["account_id"]; accountID != "" { - uuids, externalIDsByProvider = h.resolveSingleAccountFilterIDs(ctx, accountID) - } else { - var err error - uuids, externalIDsByProvider, err = h.resolveAllowedAccountScope(ctx, session) - if err != nil { - return nil, err - } - // Non-nil empty scope: restricted session with zero accessible - // accounts. Querying with it would match every row, so short-circuit. - if uuids != nil && len(uuids) == 0 && len(externalIDsByProvider) == 0 { - return nil, nil - } + filterUUIDs, filterExternalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, params["account_id"]) + uuids, externalIDsByProvider, err := h.intersectAccountFilterScope(ctx, session, filterUUIDs, filterExternalIDsByProvider) + if err != nil { + return nil, err + } + if uuids != nil && len(uuids) == 0 && len(externalIDsByProvider) == 0 { + return nil, nil } rows, err := h.config.GetActivePurchaseHistory(ctx, asOf, uuids, externalIDsByProvider) if err != nil { diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 7cf5c09b0..711283999 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -18,8 +18,8 @@ import ( // Plans handlers. func (h *Handler) listPlans(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (*PlansResponse, error) { - // Require view:plans permission - if _, err := h.requirePermission(ctx, req, "view", "plans"); err != nil { + session, err := h.requirePermission(ctx, req, "view", "plans") + if err != nil { return nil, err } @@ -30,7 +30,13 @@ func (h *Handler) listPlans(ctx context.Context, req *events.LambdaFunctionURLRe return nil, NewClientError(400, err.Error()) } - filter := config.PurchasePlanFilter{AccountIDs: accountIDs} + filter, anyAccount, err := h.scopedPlanFilter(ctx, session, accountIDs) + if err != nil { + return nil, err + } + if !anyAccount { + return &PlansResponse{Plans: []PlanWithHealth{}}, nil + } plans, err := h.config.ListPurchasePlans(ctx, filter) if err != nil { return nil, err @@ -47,6 +53,23 @@ func (h *Handler) listPlans(ctx context.Context, req *events.LambdaFunctionURLRe return &PlansResponse{Plans: h.attachPlanHealth(ctx, plans, now)}, nil } +// scopedPlanFilter narrows account_ids to the session's allowed_accounts. Only +// unrestricted callers get unassigned plans, matching requirePlanAccess. +func (h *Handler) scopedPlanFilter(ctx context.Context, session *Session, accountIDs []string) (filter config.PurchasePlanFilter, anyAccount bool, err error) { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return filter, false, fmt.Errorf("failed to get allowed accounts: %w", err) + } + if scope.AllowsAll() { + return config.PurchasePlanFilter{AccountIDs: accountIDs, IncludeUnassigned: true}, true, nil + } + allowed, _, err := h.intersectAccountFilterScope(ctx, session, accountIDs, nil) + if err != nil { + return filter, false, err + } + return config.PurchasePlanFilter{AccountIDs: allowed}, len(allowed) > 0, nil +} + // attachPlanHealth computes the per-plan health-score badge (issue #340 // follow-up) for every plan in the list, from exact per-plan execution // counts over the trailing planHealthLookbackDays diff --git a/internal/api/handler_plans_test.go b/internal/api/handler_plans_test.go index b0e309bfe..067299728 100644 --- a/internal/api/handler_plans_test.go +++ b/internal/api/handler_plans_test.go @@ -33,7 +33,7 @@ func TestHandler_listPlans(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}).Return(plans, nil) // listPlans now also counts each plan's failed/canceled executions to // compute its health-score badge (issue #340 follow-up). mockStore.On("CountExecutionsByPlanAndStatus", ctx, planHealthExecutionStatuses, mock.AnythingOfType("time.Time")). @@ -73,7 +73,7 @@ func TestHandler_listPlans_AccountIDsFilter(t *testing.T) { } accountID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" - expectedFilter := config.PurchasePlanFilter{AccountIDs: []string{accountID}} + expectedFilter := config.PurchasePlanFilter{AccountIDs: []string{accountID}, IncludeUnassigned: true} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() @@ -117,7 +117,7 @@ func TestHandler_listPlans_HealthScoreReflectsFailedExecutions(t *testing.T) { {ID: troubledPlanID, Name: "Troubled Plan", Enabled: true}, {ID: otherPlanID, Name: "Other Plan", Enabled: true}, } - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}).Return(plans, nil) mockStore.On("CountExecutionsByPlanAndStatus", ctx, planHealthExecutionStatuses, mock.AnythingOfType("time.Time")). Return(map[string]config.ExecutionStatusCounts{ troubledPlanID: {"failed": 2}, @@ -169,7 +169,7 @@ func TestHandler_listPlans_HealthScoreUnknownOnCountsFetchError(t *testing.T) { plans := []config.PurchasePlan{ {ID: "11111111-1111-1111-1111-111111111111", Name: "Some Plan", Enabled: true}, } - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}).Return(plans, nil) mockStore.On("CountExecutionsByPlanAndStatus", ctx, planHealthExecutionStatuses, mock.AnythingOfType("time.Time")). Return(nil, errors.New("db unavailable")) @@ -206,7 +206,7 @@ func TestHandler_listPlans_HealthUsesExactCountsNotTruncatedPage(t *testing.T) { mockAuth.grantAdmin() planID := "11111111-1111-1111-1111-111111111111" - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}). + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}). Return([]config.PurchasePlan{{ID: planID, Name: "Old Failures", Enabled: true}}, nil) mockStore.On("CountExecutionsByPlanAndStatus", ctx, planHealthExecutionStatuses, mock.AnythingOfType("time.Time")). Return(map[string]config.ExecutionStatusCounts{planID: {"failed": 3}}, nil) @@ -239,7 +239,7 @@ func TestHandler_listPlans_HealthCountsUseTheLookbackWindow(t *testing.T) { adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}). + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}). Return([]config.PurchasePlan{{ID: "11111111-1111-1111-1111-111111111111", Enabled: true}}, nil) var gotSince time.Time @@ -273,7 +273,7 @@ func TestHandler_listPlans_EmptyListSkipsCountsQuery(t *testing.T) { adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}). + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}). Return([]config.PurchasePlan{}, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -1732,7 +1732,7 @@ func TestHandler_listPlans_StuckRampFetchFailureLeavesHealthUnknown(t *testing.T mockAuth.grantAdmin() planID := "11111111-1111-1111-1111-111111111111" - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}). + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}). Return([]config.PurchasePlan{{ID: planID, Name: "Ramping Plan", Enabled: true}}, nil) mockStore.On("CountExecutionsByPlanAndStatus", ctx, planHealthExecutionStatuses, mock.AnythingOfType("time.Time")). Return(map[string]config.ExecutionStatusCounts{}, nil) @@ -1768,7 +1768,7 @@ func TestHandler_listPlans_StuckRampProducesTheBlockedFactor(t *testing.T) { Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, CurrentStep: 2, TotalSteps: 4, StartDate: time.Now().AddDate(0, 0, -28), } - mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}). + mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{IncludeUnassigned: true}). Return([]config.PurchasePlan{ {ID: planID, Name: "Stuck Plan", Enabled: true, RampSchedule: ramp}, {ID: quietPlanID, Name: "Late Plan", Enabled: true, RampSchedule: ramp}, diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 198b5a4b3..0126dd9fb 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1715,7 +1715,13 @@ func validateExchangeRIIDs(riIDs []string) error { // getExchangeQuote gets a quote for an RI exchange. func (h *Handler) getExchangeQuote(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - if _, err := h.requirePermission(ctx, req, "view", "purchases"); err != nil { + session, permErr := h.requirePermission(ctx, req, "view", "purchases") + if permErr != nil { + return nil, permErr + } + // A quote reveals the deployment account's RI ids and prices, so it takes + // the same session-scope gate as executeExchange (issue #384). + if err := h.requireReshapeAccountScope(ctx, session); err != nil { return nil, err } diff --git a/internal/api/tenant_scope_sweep_test.go b/internal/api/tenant_scope_sweep_test.go new file mode 100644 index 000000000..635e0e8ca --- /dev/null +++ b/internal/api/tenant_scope_sweep_test.go @@ -0,0 +1,198 @@ +package api + +import ( + "context" + "errors" + "slices" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Account-scope regression tests for GET /api/plans, the inventory commitment +// read and the RI-exchange quote (issues #29, #384). + +const ( + planInScope = "a1111111-1111-4111-8111-111111111111" + planOutOfScope = "a2222222-2222-4222-8222-222222222222" + planUnassigned = "a3333333-3333-4333-8333-333333333333" + planBothScopes = "a4444444-4444-4444-8444-444444444444" +) + +// planListStore applies buildListPlansQuery's predicate, so a wrong filter +// fails by the plans returned rather than by an unstubbed-call panic. +type planListStore struct { + *MockConfigStore + plans []config.PurchasePlan + accounts map[string][]string + filters []config.PurchasePlanFilter +} + +func (s *planListStore) ListPurchasePlans(_ context.Context, f config.PurchasePlanFilter) ([]config.PurchasePlan, error) { + s.filters = append(s.filters, f) + out := []config.PurchasePlan{} + for _, p := range s.plans { + accts := s.accounts[p.ID] + matches := slices.ContainsFunc(accts, func(a string) bool { return slices.Contains(f.AccountIDs, a) }) + if len(f.AccountIDs) == 0 || matches || (f.IncludeUnassigned && len(accts) == 0) { + out = append(out, p) + } + } + return out, nil +} + +func newPlanListStore(base *MockConfigStore) *planListStore { + base.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}, nil + } + base.On("CountExecutionsByPlanAndStatus", mock.Anything, mock.Anything, mock.Anything). + Return(map[string]config.ExecutionStatusCounts{}, nil).Maybe() + return &planListStore{ + MockConfigStore: base, + plans: []config.PurchasePlan{ + {ID: planInScope, Name: "in scope"}, + {ID: planOutOfScope, Name: "other tenant"}, + {ID: planUnassigned, Name: "unassigned"}, + {ID: planBothScopes, Name: "shared"}, + }, + accounts: map[string][]string{ + planInScope: {scopedInAccount}, + planOutOfScope: {scopedOutAccount}, + planBothScopes: {scopedInAccount, scopedOutAccount}, + }, + } +} + +func listPlanIDs(t *testing.T, h *Handler, params map[string]string) []string { + t.Helper() + resp, err := h.listPlans(context.Background(), scopedRequest(""), params) + require.NoError(t, err) + ids := make([]string, 0, len(resp.Plans)) + for _, p := range resp.Plans { + ids = append(ids, p.ID) + } + slices.Sort(ids) + return ids +} + +func TestListPlans_ScopedCallerSeesOnlyInScopePlans(t *testing.T) { + h, base := scopedHandler(t, scopedInAccount) + h.config = newPlanListStore(base) + + assert.Equal(t, []string{planInScope, planBothScopes}, listPlanIDs(t, h, map[string]string{}), + "a scoped caller must see neither another tenant's plans nor unassigned plans") +} + +func TestListPlans_ScopedCallerOutOfScopeFilterReturnsNothing(t *testing.T) { + h, base := scopedHandler(t, scopedInAccount) + store := newPlanListStore(base) + h.config = store + + ids := listPlanIDs(t, h, map[string]string{"account_ids": scopedOutAccount}) + + assert.Empty(t, ids, "account_ids naming an out-of-scope account must not narrow the list to that tenant") + assert.Empty(t, store.filters, "an empty intersection must not reach the store, where no filter means all plans") +} + +func TestListPlans_UnrestrictedCallerStillSeesUnassignedPlans(t *testing.T) { + h, base := scopedHandler(t) + h.config = newPlanListStore(base) + + assert.Equal(t, []string{planInScope, planUnassigned, planBothScopes}, + listPlanIDs(t, h, map[string]string{"account_ids": scopedInAccount})) +} + +func TestListPlans_ScopeLookupErrorFailsClosed(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, scopedToken).Return(&Session{UserID: scopedUserID}, nil) + mockAuth.On("HasPermissionAPI", ctx, scopedUserID, "view", "plans").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, scopedUserID).Return(nil, errors.New("auth store unavailable")) + store := newPlanListStore(new(MockConfigStore)) + h := &Handler{auth: mockAuth, config: store} + + _, err := h.listPlans(ctx, scopedRequest(""), map[string]string{}) + + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a scope-lookup failure must surface as a server error, got %v", err) + assert.Empty(t, store.filters) +} + +func TestListActiveCommitments_ScopedCallerOutOfScopeAccountFilterSkipsStore(t *testing.T) { + h, store := scopedHandler(t, scopedInAccount) + store.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}, nil + } + store.On("GetActivePurchaseHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]config.PurchaseHistoryRecord{}, nil).Maybe() + + _, err := h.listActiveCommitments(context.Background(), scopedRequest(""), map[string]string{"account_id": scopedOutAccount}) + require.NoError(t, err) + + store.AssertNotCalled(t, "GetActivePurchaseHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestListActiveCommitments_ScopedCallerInScopeAccountFilterReachesStore(t *testing.T) { + h, store := scopedHandler(t, scopedInAccount) + store.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}, nil + } + store.On("GetActivePurchaseHistory", mock.Anything, mock.Anything, []string{scopedInAccount}, map[string][]string(nil)). + Return([]config.PurchaseHistoryRecord{}, nil).Once() + + _, err := h.listActiveCommitments(context.Background(), scopedRequest(""), map[string]string{"account_id": scopedInAccount}) + require.NoError(t, err) + store.AssertExpectations(t) +} + +// quoteHandler scopes the caller to scopedInAccount; the failing credentials +// keep a request that slips past the gate from calling EC2. +func quoteHandler(t *testing.T, deploymentAccount string) *Handler { + h, store := scopedHandler(t, scopedInAccount) + store.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}, nil + } + h.reshapeAccountResolver = func(_ context.Context) (string, error) { return deploymentAccount, nil } + h.awsCfgOnce.Do(func() { + h.awsCfg = aws.Config{ + Region: "eu-west-1", + Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { + return aws.Credentials{}, errors.New("test: no AWS credentials") + }), + } + }) + return h +} + +const quoteBody = `{"ri_ids":["ri-123"],"target_offering_id":"4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91","target_count":1}` + +func TestGetExchangeQuote_OutOfScopeDeploymentAccountReturns403(t *testing.T) { + h := quoteHandler(t, scopedOutAccount) + + _, err := h.getExchangeQuote(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: scopedRequest("").Headers, Body: quoteBody, + }) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "allowed accounts") +} + +func TestGetExchangeQuote_InScopeDeploymentAccountPassesGate(t *testing.T) { + h := quoteHandler(t, scopedInAccount) + + _, err := h.getExchangeQuote(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: scopedRequest("").Headers, Body: quoteBody, + }) + + require.Error(t, err, "the test AWS credentials fail, so the quote itself cannot succeed") + assert.Contains(t, err.Error(), "exchange quote failed", "an in-scope caller must reach the AWS quote call") +} diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b4f018cde..d1d4a8d1b 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -836,9 +836,11 @@ func (s *PostgresStore) DeletePurchasePlan(ctx context.Context, planID string) e // caller bucket the two groups without a separate query. // // The WHERE clause retains the original account-filter semantics for -// assigned plans and adds OR NOT EXISTS ... to include unassigned ones. -// DISTINCT prevents duplicates when a plan matches multiple account IDs. -func buildListPlansQuery(accountIDs []string) (query string, args []any) { +// assigned plans and adds OR NOT EXISTS ... to include unassigned ones only +// when includeUnassigned is set, so an account-scoped caller never receives +// plans no account owns (issue #29). DISTINCT prevents duplicates when a plan +// matches multiple account IDs. +func buildListPlansQuery(accountIDs []string, includeUnassigned bool) (query string, args []any) { if len(accountIDs) == 0 { return ` SELECT id, name, enabled, auto_purchase, notification_days_before, @@ -855,6 +857,10 @@ func buildListPlansQuery(accountIDs []string) (query string, args []any) { placeholders[i] = fmt.Sprintf("$%d", i+1) args[i] = id } + unassignedClause := "" + if includeUnassigned { + unassignedClause = " OR NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id)" + } query = fmt.Sprintf(` SELECT DISTINCT pp.id, pp.name, pp.enabled, pp.auto_purchase, pp.notification_days_before, pp.services, pp.ramp_schedule, pp.created_at, pp.updated_at, @@ -862,21 +868,19 @@ func buildListPlansQuery(accountIDs []string) (query string, args []any) { (NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id)) AS unassigned FROM purchase_plans pp LEFT JOIN plan_accounts pa ON pa.plan_id = pp.id - WHERE pa.account_id IN (%s) - OR NOT EXISTS (SELECT 1 FROM plan_accounts WHERE plan_id = pp.id) + WHERE pa.account_id IN (%s)%s ORDER BY pp.created_at DESC - `, strings.Join(placeholders, ", ")) + `, strings.Join(placeholders, ", "), unassignedClause) return query, args } // ListPurchasePlans lists purchase plans, optionally filtered by account IDs. -// When filter.AccountIDs is non-empty the result includes both plans that -// reference at least one of the given accounts AND legacy plans with zero -// plan_accounts rows (flagged with Unassigned=true). Plans that have at -// least one account row are returned with Unassigned=false. The no-filter -// case returns all plans with Unassigned=false. +// When filter.AccountIDs is non-empty the result includes plans that +// reference at least one of the given accounts and, with +// filter.IncludeUnassigned, plans with zero plan_accounts rows (flagged with +// Unassigned=true). The no-filter case returns all plans with Unassigned=false. func (s *PostgresStore) ListPurchasePlans(ctx context.Context, filter PurchasePlanFilter) ([]PurchasePlan, error) { - query, args := buildListPlansQuery(filter.AccountIDs) + query, args := buildListPlansQuery(filter.AccountIDs, filter.IncludeUnassigned) rows, err := s.db.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("failed to list purchase plans: %w", err) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index f7f7fc2a7..59f5dded5 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -499,7 +499,7 @@ func TestPGXMock_ListPurchasePlans_UnassignedIncluded(t *testing.T) { sql.NullTime{}, sql.NullTime{}, sql.NullTime{}, true) mock.ExpectQuery("SELECT").WithArgs("acc-uuid").WillReturnRows(rows) - plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{"acc-uuid"}}) + plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{"acc-uuid"}, IncludeUnassigned: true}) require.NoError(t, err) require.Len(t, plans, 2) diff --git a/internal/config/store_postgres_unassigned_test.go b/internal/config/store_postgres_unassigned_test.go index 02068ab2d..c4b0c6d3a 100644 --- a/internal/config/store_postgres_unassigned_test.go +++ b/internal/config/store_postgres_unassigned_test.go @@ -112,7 +112,7 @@ func TestPostgresStoreDB_ListPurchasePlans_UnassignedBucket(t *testing.T) { // Sub-test 1: account-filtered query (filter = [accountX]) // ==================================================================== t.Run("account-filtered result includes unassigned plan and excludes wrong-account plan", func(t *testing.T) { - plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{accountXID}}) + plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{accountXID}, IncludeUnassigned: true}) require.NoError(t, err) byID := make(map[string]PurchasePlan, len(plans)) @@ -137,6 +137,21 @@ func TestPostgresStoreDB_ListPurchasePlans_UnassignedBucket(t *testing.T) { assert.False(t, planCPresent, "planC (assigned to accountY only) must NOT appear when filtering by accountX") }) + // Sub-test 1b: a scoped caller (IncludeUnassigned unset) must not receive + // the unassigned plan, nor another account's plan (issue #29). + t.Run("account-filtered result without IncludeUnassigned excludes unassigned plan", func(t *testing.T) { + plans, err := store.ListPurchasePlans(ctx, PurchasePlanFilter{AccountIDs: []string{accountXID}}) + require.NoError(t, err) + + ids := make([]string, 0, len(plans)) + for _, p := range plans { + ids = append(ids, p.ID) + } + assert.Contains(t, ids, planA.ID, "planA (assigned to accountX) must be in result") + assert.NotContains(t, ids, planB.ID, "planB (unassigned) must not reach a scoped caller") + assert.NotContains(t, ids, planC.ID, "planC (assigned to accountY only) must not appear") + }) + // ==================================================================== // Sub-test 2: no-filter query returns all three plans // ==================================================================== diff --git a/internal/config/types.go b/internal/config/types.go index 37e5565b6..881f1cb87 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -674,6 +674,9 @@ type RecommendationFilter struct { // plan_accounts join table. type PurchasePlanFilter struct { AccountIDs []string // nil/empty = all plans + // IncludeUnassigned adds plans with zero plan_accounts rows to an + // AccountIDs-filtered result. Only unrestricted callers may set it. + IncludeUnassigned bool } // RecommendationsFreshness describes the cache staleness state surfaced to