diff --git a/go.mod b/go.mod index 5fb3a2f6..abcf4fb6 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b..83df52e6 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 4aeca1a1..9aeaa842 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -421,11 +421,20 @@ func (h *Handler) resolveTargetCoverage(ctx context.Context) float64 { // aggressive for "skip this scheduled run". This version emits actual // execution rows so Cancel can target just the planned purchase via // DELETE /api/purchases/planned/{id}. +// +// The optional provider, account_ids and legacy account_id query parameters +// narrow the rows like the summary route does (issue #713, see +// parseUpcomingFilter); they never widen the session's allowed_accounts. func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFunctionURLRequest) (*UpcomingPurchaseResponse, error) { session, err := h.requirePermission(ctx, req, "view", "purchases") if err != nil { return nil, err } + // Issue #713: the topbar provider/account chips, validated before any read. + filter, err := h.parseUpcomingFilter(ctx, session, req.QueryStringParameters) + if err != nil { + return nil, err + } executions, err := h.config.GetPendingExecutions(ctx) if err != nil { @@ -445,6 +454,17 @@ func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFu if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } + upcoming, err := h.upcomingInScope(ctx, scope, filter, executions, planMap) + if err != nil { + return nil, err + } + return &UpcomingPurchaseResponse{Purchases: upcoming}, nil +} + +// upcomingInScope builds the upcoming rows for the executions whose plan still +// exists, that the session scope can see, and that pass the caller's filter, +// in input order. +func (h *Handler) upcomingInScope(ctx context.Context, scope auth.AccountScope, filter upcomingFilter, executions []config.PurchaseExecution, planMap map[string]*config.PurchasePlan) ([]UpcomingPurchase, error) { // Memoises GetPlanAccounts, mirroring getPlannedPurchases. planAccounts := make(map[string][]config.CloudAccount) @@ -464,10 +484,16 @@ func (h *Handler) getUpcomingPurchases(ctx context.Context, req *events.LambdaFu if !ok { continue } - upcoming = append(upcoming, upcomingFromExecution(plan, &exec)) + row := upcomingFromExecution(plan, &exec) + ok, err = filter.matches(ctx, h, &row, &exec, planAccounts) + if err != nil { + return nil, err + } + if ok { + upcoming = append(upcoming, row) + } } - - return &UpcomingPurchaseResponse{Purchases: upcoming}, nil + return upcoming, nil } // firstServiceConfig returns the ServiceConfig for the lexicographically first diff --git a/internal/api/handler_dashboard_upcoming_filter.go b/internal/api/handler_dashboard_upcoming_filter.go new file mode 100644 index 00000000..35334c7c --- /dev/null +++ b/internal/api/handler_dashboard_upcoming_filter.go @@ -0,0 +1,99 @@ +package api + +import ( + "context" + "strings" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// upcomingFilter is the optional provider and account narrowing for +// GET /api/dashboard/upcoming (issue #713), so the Home widget can follow the +// same topbar chips as /api/dashboard/summary. The zero value keeps every row +// the session scope already allows. +type upcomingFilter struct { + // accounts is nil when the caller sent no account filter. Otherwise it + // holds the requested accounts already intersected with the session's + // allowed_accounts by resolveDashboardAccountScope, so an out-of-scope + // account is dropped (an empty set matches nothing) instead of widening + // the result. + accounts *dualColumnAccountSet + // provider is "" for every provider. + provider string +} + +// dualColumnAccountSet is a resolved account filter: cloud_accounts UUIDs plus +// external account numbers grouped by provider ("" = provider unknown, from a +// legacy account_id that is not a known UUID). +type dualColumnAccountSet struct { + externalIDsByProvider map[string][]string + uuids []string +} + +// parseUpcomingFilter reads the provider, account_ids and legacy account_id +// query parameters with the same validation and precedence as +// getDashboardSummary: an unknown provider or a malformed account_ids is a 400, +// and account_ids wins over account_id. +func (h *Handler) parseUpcomingFilter(ctx context.Context, session *Session, params map[string]string) (upcomingFilter, error) { + provider, err := providerFilterParam(params["provider"]) + if err != nil { + return upcomingFilter{}, err + } + f := upcomingFilter{provider: provider} + + requested, err := parseAccountIDs(params["account_ids"]) + if err != nil { + return upcomingFilter{}, NewClientError(400, err.Error()) + } + if len(requested) == 0 && params["account_id"] == "" { + // No account chip: leave the rows to the session scope alone. + return f, nil + } + uuids, externalIDsByProvider, err := h.resolveDashboardAccountScope(ctx, session, params) + if err != nil { + return upcomingFilter{}, err + } + f.accounts = &dualColumnAccountSet{uuids: uuids, externalIDsByProvider: externalIDsByProvider} + return f, nil +} + +// matches reports whether an upcoming row passes the filter. The provider is +// compared with the row's provider (its plan's first service, as shown in the +// widget). An execution matches the account filter when any account it can buy +// in is selected: a plan-level parent fans out to every plan account, so it is +// relevant to each of them, while a fan-out child carries its single account. +func (f upcomingFilter) matches(ctx context.Context, h *Handler, row *UpcomingPurchase, exec *config.PurchaseExecution, planAccounts map[string][]config.CloudAccount) (bool, error) { + if f.provider != "" && !strings.EqualFold(row.Provider, f.provider) { + return false, nil + } + if f.accounts == nil { + return true, nil + } + accounts, err := h.executionAccounts(ctx, exec, planAccounts) + if IsNotFoundError(err) { + return false, nil + } + if err != nil { + return false, err + } + return f.accounts.matchesAny(accounts), nil +} + +// matchesAny reports whether any account is in the set, by UUID or by external +// account number under its provider (or under "" for an unknown provider). +func (s *dualColumnAccountSet) matchesAny(accounts []config.CloudAccount) bool { + for i := range accounts { + a := &accounts[i] + if stringInSlice(a.ID, s.uuids) { + return true + } + if a.ExternalID == "" { + continue + } + if stringInSlice(a.ExternalID, s.externalIDsByProvider[a.Provider]) || + stringInSlice(a.ExternalID, s.externalIDsByProvider[""]) { + return true + } + } + return false +} diff --git a/internal/api/handler_dashboard_upcoming_filter_test.go b/internal/api/handler_dashboard_upcoming_filter_test.go new file mode 100644 index 00000000..827fab97 --- /dev/null +++ b/internal/api/handler_dashboard_upcoming_filter_test.go @@ -0,0 +1,260 @@ +package api + +import ( + "context" + "encoding/json" + "sort" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #713: GET /api/dashboard/upcoming took no filter parameters, so the +// Home widget could not follow the topbar provider and account chips. It now +// accepts provider and account_ids (plus the legacy account_id) like +// /api/dashboard/summary, with the account filter intersected with the +// session's allowed_accounts. + +const ( + upAcctProd = "aaaaaaaa-0000-4000-8000-000000000001" + upAcctStage = "aaaaaaaa-0000-4000-8000-000000000002" + upAcctGCP = "aaaaaaaa-0000-4000-8000-000000000003" + + upPlanProd = "bbbbbbbb-0000-4000-8000-000000000001" + upPlanStage = "bbbbbbbb-0000-4000-8000-000000000002" + upPlanGCP = "bbbbbbbb-0000-4000-8000-000000000003" + upPlanMulti = "bbbbbbbb-0000-4000-8000-000000000004" +) + +// Placeholder external IDs, named so request maps stay clear of the account-ID detector. +const ( + upExtProd = "123456789012" + upExtStage = "111111111111" +) + +var upcomingFilterAccounts = []config.CloudAccount{ + {ID: upAcctProd, Name: "Production", Provider: "aws", ExternalID: upExtProd}, + {ID: upAcctStage, Name: "Staging", Provider: "aws", ExternalID: upExtStage}, + {ID: upAcctGCP, Name: "Analytics", Provider: "gcp", ExternalID: "example-gcp-project"}, +} + +func upcomingFilterAccount(id string) config.CloudAccount { + for _, a := range upcomingFilterAccounts { + if a.ID == id { + return a + } + } + return config.CloudAccount{ID: id} +} + +// upcomingFilterStore wires one pending execution per plan, plus a fan-out +// child of the multi-account plan pinned to Staging: +// +// exec-prod aws plan, accounts [Production] +// exec-stage aws plan, accounts [Staging] +// exec-gcp gcp plan, accounts [Analytics] +// exec-multi aws plan-level parent, accounts [Production, Staging] +// exec-multi-stage aws fan-out child, cloud_account_id = Staging +func upcomingFilterStore() *MockConfigStore { + store := new(MockConfigStore) + scheduled := time.Now().AddDate(0, 0, 7) + plan := func(id, name, provider, service string) config.PurchasePlan { + return config.PurchasePlan{ + ID: id, Name: name, Enabled: true, + Services: map[string]config.ServiceConfig{provider + "/" + service: {Provider: provider, Service: service}}, + RampSchedule: config.RampSchedule{TotalSteps: 3}, + } + } + plans := []config.PurchasePlan{ + plan(upPlanProd, "Prod EC2", "aws", "ec2"), + plan(upPlanStage, "Stage RDS", "aws", "rds"), + plan(upPlanGCP, "GCP compute", "gcp", "compute"), + plan(upPlanMulti, "Multi EC2", "aws", "ec2"), + } + stage := upAcctStage + pending := []config.PurchaseExecution{ + {ExecutionID: "exec-prod", PlanID: upPlanProd, Status: "pending", ScheduledDate: scheduled, StepNumber: 1}, + {ExecutionID: "exec-stage", PlanID: upPlanStage, Status: "pending", ScheduledDate: scheduled, StepNumber: 1}, + {ExecutionID: "exec-gcp", PlanID: upPlanGCP, Status: "pending", ScheduledDate: scheduled, StepNumber: 1}, + {ExecutionID: "exec-multi", PlanID: upPlanMulti, Status: "pending", ScheduledDate: scheduled, StepNumber: 1}, + {ExecutionID: "exec-multi-stage", PlanID: upPlanMulti, CloudAccountID: &stage, Status: "pending", ScheduledDate: scheduled, StepNumber: 1}, + } + planAccounts := map[string][]config.CloudAccount{ + upPlanProd: {upcomingFilterAccount(upAcctProd)}, + upPlanStage: {upcomingFilterAccount(upAcctStage)}, + upPlanGCP: {upcomingFilterAccount(upAcctGCP)}, + upPlanMulti: {upcomingFilterAccount(upAcctProd), upcomingFilterAccount(upAcctStage)}, + } + + store.On("GetPendingExecutions", mock.Anything).Return(pending, nil) + store.On("ListPurchasePlans", mock.Anything, config.PurchasePlanFilter{}).Return(plans, nil) + store.GetPlanAccountsFn = func(_ context.Context, planID string) ([]config.CloudAccount, error) { + return planAccounts[planID], nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + a := upcomingFilterAccount(id) + return &a, nil + } + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { + return append([]config.CloudAccount(nil), upcomingFilterAccounts...), nil + } + return store +} + +// upcomingAsViewer calls getUpcomingPurchases as a session restricted to +// allowed (nil = admin, unrestricted) with the given query parameters. +func upcomingAsViewer(t *testing.T, allowed []string, params map[string]string) (*UpcomingPurchaseResponse, error) { + t.Helper() + ctx := context.Background() + store := upcomingFilterStore() + var mockAuth *MockAuthService + var req *events.LambdaFunctionURLRequest + if allowed == nil { + mockAuth, req = adminDashboardReq(ctx) + } else { + mockAuth = new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "viewer-token").Return(&Session{UserID: "viewer-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "viewer-1", "view", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "viewer-1").Return(allowed, nil) + req = &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer viewer-token"}} + } + req.QueryStringParameters = params + handler := &Handler{auth: mockAuth, config: store} + return handler.getUpcomingPurchases(ctx, req) +} + +// upcomingIDsFor returns the sorted execution IDs upcomingAsViewer lists. +func upcomingIDsFor(t *testing.T, allowed []string, params map[string]string) []string { + t.Helper() + resp, err := upcomingAsViewer(t, allowed, params) + require.NoError(t, err) + return upcomingIDs(resp) +} + +func upcomingIDs(resp *UpcomingPurchaseResponse) []string { + ids := make([]string, 0, len(resp.Purchases)) + for _, p := range resp.Purchases { + ids = append(ids, p.ExecutionID) + } + sort.Strings(ids) + return ids +} + +func TestGetUpcomingPurchases_NoFilterUnchanged(t *testing.T) { + ids := upcomingIDsFor(t, nil, nil) + assert.Equal(t, []string{"exec-gcp", "exec-multi", "exec-multi-stage", "exec-prod", "exec-stage"}, ids) + + ids = upcomingIDsFor(t, nil, map[string]string{"provider": "all", "account_ids": ""}) + assert.Equal(t, []string{"exec-gcp", "exec-multi", "exec-multi-stage", "exec-prod", "exec-stage"}, ids, + "provider=all and an empty account_ids mean no filter") +} + +func TestGetUpcomingPurchases_ProviderFilter(t *testing.T) { + ids := upcomingIDsFor(t, nil, map[string]string{"provider": "gcp"}) + assert.Equal(t, []string{"exec-gcp"}, ids) + + ids = upcomingIDsFor(t, nil, map[string]string{"provider": "aws"}) + assert.Equal(t, []string{"exec-multi", "exec-multi-stage", "exec-prod", "exec-stage"}, ids) + + ids = upcomingIDsFor(t, nil, map[string]string{"provider": "azure"}) + assert.Empty(t, ids) +} + +func TestGetUpcomingPurchases_InvalidFilterIs400(t *testing.T) { + for name, params := range map[string]map[string]string{ + "unknown provider": {"provider": "oracle"}, + "non-uuid account_ids": {"account_ids": "not-a-uuid"}, + } { + t.Run(name, func(t *testing.T) { + _, err := upcomingAsViewer(t, nil, params) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, 400, ce.code) + }) + } +} + +func TestGetUpcomingPurchases_AccountFilter(t *testing.T) { + // A plan-level parent buys in every plan account, so it matches when any + // of them is selected; the fan-out child matches only its own account. + ids := upcomingIDsFor(t, nil, map[string]string{"account_ids": upAcctProd}) + assert.Equal(t, []string{"exec-multi", "exec-prod"}, ids) + + ids = upcomingIDsFor(t, nil, map[string]string{"account_ids": upAcctStage + "," + upAcctGCP}) + assert.Equal(t, []string{"exec-gcp", "exec-multi", "exec-multi-stage", "exec-stage"}, ids) + + // Legacy singular account_id carrying the external account number. + ids = upcomingIDsFor(t, nil, map[string]string{"account_id": upExtStage}) + assert.Equal(t, []string{"exec-multi", "exec-multi-stage", "exec-stage"}, ids) + + // account_ids wins over account_id, as on the summary route. + ids = upcomingIDsFor(t, nil, map[string]string{"account_ids": upAcctGCP, "account_id": upAcctProd}) + assert.Equal(t, []string{"exec-gcp"}, ids) + + // An account no plan targets matches nothing rather than everything. + ids = upcomingIDsFor(t, nil, map[string]string{"account_ids": "cccccccc-0000-4000-8000-000000000009"}) + assert.Empty(t, ids) +} + +func TestGetUpcomingPurchases_ProviderAndAccountCombine(t *testing.T) { + ids := upcomingIDsFor(t, nil, map[string]string{"provider": "gcp", "account_ids": upAcctProd + "," + upAcctGCP}) + assert.Equal(t, []string{"exec-gcp"}, ids) + + ids = upcomingIDsFor(t, nil, map[string]string{"provider": "gcp", "account_ids": upAcctProd}) + assert.Empty(t, ids) +} + +// A restricted session naming an account outside its allowed_accounts gets +// that account dropped from the filter (an empty result, not someone else's +// rows and not an error), the same as /api/dashboard/summary. +func TestGetUpcomingPurchases_OutOfScopeAccount(t *testing.T) { + allowed := []string{upAcctProd, upAcctGCP} + + // Baseline: the session scope alone hides every row touching Staging. + ids := upcomingIDsFor(t, allowed, nil) + assert.Equal(t, []string{"exec-gcp", "exec-prod"}, ids) + + ids = upcomingIDsFor(t, allowed, map[string]string{"account_ids": upAcctStage}) + assert.Empty(t, ids, "an out-of-scope account must not widen or bypass the session scope") + + ids = upcomingIDsFor(t, allowed, map[string]string{"account_ids": upAcctStage + "," + upAcctProd}) + assert.Equal(t, []string{"exec-prod"}, ids, "the in-scope part of a mixed filter still applies") + + ids = upcomingIDsFor(t, allowed, map[string]string{"account_id": upExtStage}) + assert.Empty(t, ids, "a legacy external number for an out-of-scope account matches nothing") + + // An in-scope legacy external number is re-grouped under its real + // provider by the intersection and still matches. + ids = upcomingIDsFor(t, allowed, map[string]string{"account_id": upExtProd}) + assert.Equal(t, []string{"exec-prod"}, ids) + + ids = upcomingIDsFor(t, allowed, map[string]string{"account_ids": upAcctGCP}) + assert.Equal(t, []string{"exec-gcp"}, ids) +} + +// Through the real router: the query string reaches the handler. +func TestHandleRequest_DashboardUpcomingAppliesQueryFilters(t *testing.T) { + ctx := context.Background() + handler := &Handler{config: upcomingFilterStore(), corsAllowedOrigin: "*", apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": "test-key"}, + QueryStringParameters: map[string]string{"provider": "aws", "account_ids": upAcctStage}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "GET", Path: "/api/dashboard/upcoming"}, + }, + } + + resp, err := handler.HandleRequest(ctx, req) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode, resp.Body) + + var body UpcomingPurchaseResponse + require.NoError(t, json.Unmarshal([]byte(resp.Body), &body)) + assert.Equal(t, []string{"exec-multi", "exec-multi-stage", "exec-stage"}, upcomingIDs(&body)) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index a7782101..dc8ee46d 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -160,6 +160,30 @@ paths: operationId: getUpcomingPurchases tags: [Dashboard] summary: Get upcoming scheduled purchases + description: > + Pending purchase executions the caller's account scope can see. The + optional filters narrow that set and never widen it. + parameters: + - name: provider + in: query + description: > + Filter by the row's provider (its plan's first service). "all" or + no value means every provider. Any other value is a 400. + schema: + type: string + enum: [aws, azure, gcp, all] + - name: account_ids + in: query + description: > + Comma-separated cloud account UUIDs (at most 200). When present it + takes precedence over account_id. A row matches when any account + the execution can buy in is selected (a plan-level execution + covers every plan account; a per-account execution only its own). + The selection is narrowed to the caller's allowed accounts, so an + account outside them matches no rows. + schema: + type: string + - $ref: '#/components/parameters/AccountID' responses: '200': description: Upcoming purchases @@ -167,6 +191,8 @@ paths: application/json: schema: $ref: '#/components/schemas/UpcomingPurchaseResponse' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403':