diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index d3c2561b..7a99e063 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -90,7 +90,10 @@ func (h *Handler) listAccounts(ctx context.Context, req *events.LambdaFunctionUR return nil, err } - filter := buildAccountFilter(req.QueryStringParameters) + filter, err := buildAccountFilter(req.QueryStringParameters) + if err != nil { + return nil, err + } accts, err := h.config.ListCloudAccounts(ctx, filter) if err != nil { @@ -156,7 +159,10 @@ func (h *Handler) listAccountsMinimal(ctx context.Context, req *events.LambdaFun return nil, err } - filter := buildAccountFilter(req.QueryStringParameters) + filter, err := buildAccountFilter(req.QueryStringParameters) + if err != nil { + return nil, err + } accts, err := h.config.ListCloudAccounts(ctx, filter) if err != nil { @@ -218,29 +224,35 @@ func annotateAccountNotices(acct *config.CloudAccount, hostPartition string) { } // buildAccountFilter constructs a CloudAccountFilter from query parameters. -func buildAccountFilter(params map[string]string) config.CloudAccountFilter { +// An unknown provider or an enabled value other than "true"/"false" is a 400 +// instead of being silently ignored (issue #312); an empty value or +// provider "all" means no filter. +func buildAccountFilter(params map[string]string) (config.CloudAccountFilter, error) { var filter config.CloudAccountFilter - if p, ok := params["provider"]; ok && p != "" { - filter.Provider = &p + provider, err := providerFilterParam(params["provider"]) + if err != nil { + return filter, err } - - if e, ok := params["enabled"]; ok { - switch e { - case "true": - t := true - filter.Enabled = &t - case "false": - f := false - filter.Enabled = &f - } + if provider != "" { + filter.Provider = &provider } - if s, ok := params["search"]; ok { - filter.Search = s + switch params["enabled"] { + case "": + case "true": + t := true + filter.Enabled = &t + case "false": + f := false + filter.Enabled = &f + default: + return filter, NewClientError(400, "invalid enabled: must be true or false") } - return filter + filter.Search = params["search"] + + return filter, nil } // createSelfAccount handles POST /api/accounts/self. diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index bf6dd62d..df0a2f69 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -1872,31 +1872,38 @@ func (f *fakeCredStore) DecryptPayload(_ string) ([]byte, error) // --- buildAccountFilter --- func TestBuildAccountFilter(t *testing.T) { + build := func(t *testing.T, params map[string]string) config.CloudAccountFilter { + t.Helper() + f, err := buildAccountFilter(params) + require.NoError(t, err) + return f + } + t.Run("provider filter", func(t *testing.T) { - f := buildAccountFilter(map[string]string{"provider": "aws"}) + f := build(t, map[string]string{"provider": "aws"}) require.NotNil(t, f.Provider) assert.Equal(t, "aws", *f.Provider) }) t.Run("enabled true", func(t *testing.T) { - f := buildAccountFilter(map[string]string{"enabled": "true"}) + f := build(t, map[string]string{"enabled": "true"}) require.NotNil(t, f.Enabled) assert.True(t, *f.Enabled) }) t.Run("enabled false", func(t *testing.T) { - f := buildAccountFilter(map[string]string{"enabled": "false"}) + f := build(t, map[string]string{"enabled": "false"}) require.NotNil(t, f.Enabled) assert.False(t, *f.Enabled) }) t.Run("search filter", func(t *testing.T) { - f := buildAccountFilter(map[string]string{"search": "production"}) + f := build(t, map[string]string{"search": "production"}) assert.Equal(t, "production", f.Search) }) t.Run("empty params", func(t *testing.T) { - f := buildAccountFilter(map[string]string{}) + f := build(t, map[string]string{}) assert.Nil(t, f.Provider) assert.Nil(t, f.Enabled) assert.Empty(t, f.Search) diff --git a/internal/api/handler_apikeys.go b/internal/api/handler_apikeys.go index 7406e9cd..51c504f4 100644 --- a/internal/api/handler_apikeys.go +++ b/internal/api/handler_apikeys.go @@ -122,7 +122,7 @@ func (h *Handler) deleteAPIKey(ctx context.Context, req *events.LambdaFunctionUR } if err := h.auth.DeleteAPIKeyAPI(ctx, session.UserID, keyID); err != nil { - return nil, fmt.Errorf("failed to delete API key: %w", err) + return nil, mapAPIKeyAccessError(err, "delete") } return map[string]string{"status": "deleted"}, nil @@ -142,12 +142,24 @@ func (h *Handler) revokeAPIKey(ctx context.Context, req *events.LambdaFunctionUR } if err := h.auth.RevokeAPIKeyAPI(ctx, session.UserID, keyID); err != nil { - return nil, fmt.Errorf("failed to revoke API key: %w", err) + return nil, mapAPIKeyAccessError(err, "revoke") } return map[string]string{"status": "revoked"}, nil } +// mapAPIKeyAccessError maps a delete/revoke failure to its response. A key +// that does not exist and another user's key (for a non-admin caller) both +// answer 404 with the same text, as openapi.yaml documents, so the response +// does not confirm that someone else's key ID exists (issue #312). Anything +// else stays a 500. +func mapAPIKeyAccessError(err error, action string) error { + if errors.Is(err, auth.ErrAPIKeyNotFound) || errors.Is(err, auth.ErrAPIKeyNotOwned) { + return NewClientError(404, "API key not found") + } + return fmt.Errorf("failed to %s API key: %w", action, err) +} + // apiKeyIDFromPath extracts the key ID from /api/api-keys/{id} or // /api/api-keys/{id}/revoke depending on `hasTrailingAction`. func apiKeyIDFromPath(path string, hasTrailingAction bool) (string, error) { diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index c2399940..29d1b9b3 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -22,6 +22,10 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun if err != nil { return nil, err } + provider, err := providerFilterParam(params["provider"]) + if err != nil { + return nil, err + } accountUUIDs, accountExternalIDsByProvider, err := h.resolveDashboardAccountScope(ctx, session, params) if err != nil { @@ -36,7 +40,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun } recommendations, err := h.scheduler.ListRecommendations(recommendationsReadContext(ctx, scope), config.RecommendationFilter{ - Provider: params["provider"], + Provider: provider, }) if err != nil { return nil, fmt.Errorf("failed to get recommendations: %w", err) @@ -56,7 +60,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey) targetCoverage := h.resolveTargetCoverage(ctx) - activeCommitments, committedMonthly, ytdSavings, currentSavingsByService, err := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider) + activeCommitments, committedMonthly, ytdSavings, currentSavingsByService, err := h.calculateCommitmentMetrics(ctx, provider, accountUUIDs, accountExternalIDsByProvider) if err != nil { // Issue #72: purchase history is a hard dependency of the commitment // KPIs, same as recommendations are of the savings KPIs above. Fail diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index 13d2c2ca..8be7d4ad 100644 --- a/internal/api/handler_groups.go +++ b/internal/api/handler_groups.go @@ -88,6 +88,9 @@ func (h *Handler) getGroup(ctx context.Context, req *events.LambdaFunctionURLReq } group, err := h.auth.GetGroupAPI(ctx, groupID) + if errors.Is(err, auth.ErrGroupNotFound) { + return nil, NewClientError(404, "group not found") + } if err != nil { return nil, err } diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 1a482cce..2e78133c 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -105,6 +105,10 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF // `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) { + provider, err := providerFilterParam(params["provider"]) + if err != nil { + return nil, err + } filterUUIDs, filterExternalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, params["account_id"]) uuids, externalIDsByProvider, err := h.intersectAccountFilterScope(ctx, session, filterUUIDs, filterExternalIDsByProvider) if err != nil { @@ -118,10 +122,10 @@ func (h *Handler) fetchCommitmentRecords(ctx context.Context, asOf time.Time, se return nil, err } - // Apply provider filter in-memory. An absent or empty param means - // "all providers". Case-sensitive match — providers are always - // lowercase in the store (aws, azure, gcp). - if provider := params["provider"]; provider != "" { + // Apply provider filter in-memory. An absent param or "all" means + // "all providers"; providerFilterParam refused anything but the + // lowercase names the store holds (aws, azure, gcp). + if provider != "" { filtered := rows[:0] for _rvc := range rows { r := rows[_rvc] @@ -181,6 +185,10 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu if err != nil { return nil, err } + provider, err := providerFilterParam(params["provider"]) + if err != nil { + return nil, err + } // --- covered: active commitments ---------------------------------------- now := time.Now() @@ -219,7 +227,7 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu if err != nil { return nil, err } - onDemandByKey := aggregateOnDemandByKey(recs, params["provider"]) + onDemandByKey := aggregateOnDemandByKey(recs, provider) return buildCoverageBreakdown(coveredByKey, onDemandByKey), nil } diff --git a/internal/api/handler_not_found_mapping_test.go b/internal/api/handler_not_found_mapping_test.go new file mode 100644 index 00000000..fbe6e335 --- /dev/null +++ b/internal/api/handler_not_found_mapping_test.go @@ -0,0 +1,152 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #312 (audit A02-012): a missing user or group, and a missing or +// someone else's API key, answered 500 "Internal server error" (and logged at +// error level) where openapi.yaml documents 404. The auth service now returns +// exported sentinels; these tests pin the handler mapping. Each error below is +// the exact shape the real service returns. + +const notFoundTestID = "11111111-1111-1111-1111-111111111111" + +func notFoundAdminReq(ctx context.Context, path string) (*MockAuthService, *events.LambdaFunctionURLRequest) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + }, nil) + mockAuth.grantAdmin() + return mockAuth, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Path: path}, + }, + } +} + +func requireNotFound(t *testing.T, err error, msg string) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "must be a client error, not a 500: %v", err) + assert.Equal(t, 404, ce.code) + assert.Equal(t, msg, ce.message) +} + +func requireServerError(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + _, ok := IsClientError(err) + assert.False(t, ok, "a store failure must stay a 500, got client error %v", err) +} + +func TestHandler_getUser_MissingUserIs404(t *testing.T) { + ctx := context.Background() + mockAuth, req := notFoundAdminReq(ctx, "/api/users/"+notFoundTestID) + mockAuth.On("GetUser", ctx, notFoundTestID). + Return(nil, fmt.Errorf("%w: %w", auth.ErrUserNotFound, pgx.ErrNoRows)) + h := &Handler{auth: mockAuth} + + _, err := h.getUser(ctx, req, notFoundTestID) + + requireNotFound(t, err, "user not found") +} + +func TestHandler_getUser_StoreFailureStays500(t *testing.T) { + ctx := context.Background() + mockAuth, req := notFoundAdminReq(ctx, "/api/users/"+notFoundTestID) + mockAuth.On("GetUser", ctx, notFoundTestID).Return(nil, errors.New("connection refused")) + h := &Handler{auth: mockAuth} + + _, err := h.getUser(ctx, req, notFoundTestID) + + requireServerError(t, err) +} + +func TestHandler_getGroup_MissingGroupIs404(t *testing.T) { + ctx := context.Background() + for name, storeErr := range map[string]error{ + "no rows": fmt.Errorf("%w: %w", auth.ErrGroupNotFound, pgx.ErrNoRows), + "nil group": auth.ErrGroupNotFound, + } { + t.Run(name, func(t *testing.T) { + mockAuth, req := notFoundAdminReq(ctx, "/api/groups/"+notFoundTestID) + mockAuth.On("GetGroupAPI", ctx, notFoundTestID).Return(nil, storeErr) + h := &Handler{auth: mockAuth} + + _, err := h.getGroup(ctx, req, notFoundTestID) + + requireNotFound(t, err, "group not found") + }) + } +} + +func TestHandler_getGroup_StoreFailureStays500(t *testing.T) { + ctx := context.Background() + mockAuth, req := notFoundAdminReq(ctx, "/api/groups/"+notFoundTestID) + mockAuth.On("GetGroupAPI", ctx, notFoundTestID).Return(nil, errors.New("connection refused")) + h := &Handler{auth: mockAuth} + + _, err := h.getGroup(ctx, req, notFoundTestID) + + requireServerError(t, err) +} + +// The not-owned case gets the same 404 and message as a missing key, so a +// caller cannot tell another user's key ID from one that does not exist. +func TestHandler_APIKeyDeleteRevoke_MissingOrNotOwnedIs404(t *testing.T) { + ctx := context.Background() + serviceErrs := map[string]func(action string) error{ + "missing": func(string) error { return auth.ErrAPIKeyNotFound }, + "not owned": func(action string) error { + return fmt.Errorf("%w: cannot %s another user's API key", auth.ErrAPIKeyNotOwned, action) + }, + } + for name, serviceErr := range serviceErrs { + t.Run("delete "+name, func(t *testing.T) { + mockAuth, req := notFoundAdminReq(ctx, "/api/api-keys/"+notFoundTestID) + mockAuth.On("DeleteAPIKeyAPI", ctx, "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", notFoundTestID).Return(serviceErr("delete")) + h := &Handler{auth: mockAuth} + + _, err := h.deleteAPIKey(ctx, req) + + requireNotFound(t, err, "API key not found") + }) + t.Run("revoke "+name, func(t *testing.T) { + mockAuth, req := notFoundAdminReq(ctx, "/api/api-keys/"+notFoundTestID+"/revoke") + mockAuth.On("RevokeAPIKeyAPI", ctx, "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", notFoundTestID).Return(serviceErr("revoke")) + h := &Handler{auth: mockAuth} + + _, err := h.revokeAPIKey(ctx, req) + + requireNotFound(t, err, "API key not found") + }) + } +} + +func TestHandler_APIKeyDeleteRevoke_StoreFailureStays500(t *testing.T) { + ctx := context.Background() + storeErr := errors.New("failed to get API key: connection refused") + + mockAuth, req := notFoundAdminReq(ctx, "/api/api-keys/"+notFoundTestID) + mockAuth.On("DeleteAPIKeyAPI", ctx, "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", notFoundTestID).Return(storeErr) + _, err := (&Handler{auth: mockAuth}).deleteAPIKey(ctx, req) + requireServerError(t, err) + + mockAuth, req = notFoundAdminReq(ctx, "/api/api-keys/"+notFoundTestID+"/revoke") + mockAuth.On("RevokeAPIKeyAPI", ctx, "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", notFoundTestID).Return(storeErr) + _, err = (&Handler{auth: mockAuth}).revokeAPIKey(ctx, req) + requireServerError(t, err) +} diff --git a/internal/api/handler_query_param_validation_test.go b/internal/api/handler_query_param_validation_test.go new file mode 100644 index 00000000..23249e88 --- /dev/null +++ b/internal/api/handler_query_param_validation_test.go @@ -0,0 +1,235 @@ +package api + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #312 (audit A02-021): the dashboard summary, the inventory reads and +// the account lists accepted any provider (and the account lists any +// "enabled") and answered 200 with an empty or unfiltered result, where the +// recommendations, history, analytics and config endpoints answer 400 for the +// same parameter. "all" is the documented "every provider" value of +// validateProvider, so these reads now treat it like an absent provider +// instead of filtering on the literal string "all". + +func requireBadRequest(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "must be a 400 client error, got: %v", err) + assert.Equal(t, 400, ce.code) +} + +func TestHandler_getDashboardSummary_RejectsUnknownProvider(t *testing.T) { + ctx := context.Background() + for _, provider := range []string{"AWS", "ibm", "aws "} { + t.Run(provider, func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + mockAuth, req := adminDashboardReq(ctx) + h := &Handler{auth: mockAuth, scheduler: mockScheduler, config: mockStore} + + _, err := h.getDashboardSummary(ctx, req, map[string]string{"provider": provider}) + + requireBadRequest(t, err) + mockScheduler.AssertNotCalled(t, "ListRecommendations", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetActivePurchaseHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} + +func TestHandler_getDashboardSummary_AllProviderMeansNoFilter(t *testing.T) { + ctx := context.Background() + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + + mockScheduler.On("ListRecommendations", mock.Anything, config.RecommendationFilter{Provider: ""}). + Return([]config.RecommendationRecord{{Provider: "aws", Service: "ec2", ResourceType: "m5.large", Savings: 40}}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80}, nil) + mockStore.On("GetServiceConfig", ctx, mock.Anything, mock.Anything).Return(nil, config.ErrNotFound) + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything). + Return([]config.PurchaseHistoryRecord{{ + Provider: "azure", Service: "compute", AccountID: "acc-1", PurchaseID: "p-1", + Term: 1, Count: 1, Timestamp: time.Now().AddDate(0, -1, 0), MonthlyCost: float64Ptr(10), + UpfrontCost: new(float64), + }}, nil) + + mockAuth, req := adminDashboardReq(ctx) + h := &Handler{auth: mockAuth, scheduler: mockScheduler, config: mockStore} + + result, err := h.getDashboardSummary(ctx, req, map[string]string{"provider": "all"}) + + require.NoError(t, err) + assert.Equal(t, 1, result.TotalRecommendations) + assert.Equal(t, 1, result.ActiveCommitments, "provider=all must not drop every commitment") + mockScheduler.AssertExpectations(t) +} + +func inventoryRowsForProviderTests() []config.PurchaseHistoryRecord { + now := time.Now() + return []config.PurchaseHistoryRecord{ + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-aws", Provider: "aws", Service: "ec2", + Timestamp: now.AddDate(0, -3, 0), Term: 1, Count: 1, MonthlyCost: float64Ptr(80)}, + {UpfrontCost: new(float64), AccountID: "acc-1", PurchaseID: "p-azure", Provider: "azure", Service: "compute", + Timestamp: now.AddDate(0, -3, 0), Term: 1, Count: 1, MonthlyCost: float64Ptr(120)}, + } +} + +func TestHandler_inventory_RejectsUnknownProvider(t *testing.T) { + ctx := context.Background() + reads := map[string]func(*Handler, map[string]string) error{ + "commitments": func(h *Handler, p map[string]string) error { + _, req := adminInventoryReq(ctx) + _, err := h.listActiveCommitments(ctx, req, p) + return err + }, + "coverage": func(h *Handler, p map[string]string) error { + _, req := adminInventoryReq(ctx) + _, err := h.getCoverageBreakdown(ctx, req, p) + return err + }, + } + for name, read := range reads { + t.Run(name, func(t *testing.T) { + mockStore := new(MockConfigStore) + mockAuth, _ := adminInventoryReq(ctx) + h := &Handler{auth: mockAuth, config: mockStore, scheduler: new(MockScheduler)} + + err := read(h, map[string]string{"provider": "AWS"}) + + requireBadRequest(t, err) + mockStore.AssertNotCalled(t, "GetActivePurchaseHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} + +func TestHandler_listActiveCommitments_AllProviderMeansNoFilter(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)). + Return(inventoryRowsForProviderTests(), nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: "acc-1", Name: "Account One"}}, nil + } + mockAuth, req := adminInventoryReq(ctx) + h := &Handler{auth: mockAuth, config: mockStore} + + result, err := h.listActiveCommitments(ctx, req, map[string]string{"provider": "all"}) + + require.NoError(t, err) + assert.Len(t, result.(InventoryCommitmentsResponse).Commitments, 2) +} + +func TestHandler_getCoverageBreakdown_AllProviderMeansNoFilter(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string(nil)). + Return(inventoryRowsForProviderTests(), nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: "acc-1", Name: "Account One"}}, nil + } + mockScheduler := new(MockScheduler) + mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything). + Return([]config.RecommendationRecord{{Provider: "aws", Service: "ec2", Savings: 20}}, nil) + mockAuth, req := adminInventoryReq(ctx) + h := &Handler{auth: mockAuth, config: mockStore, scheduler: mockScheduler} + + result, err := h.getCoverageBreakdown(ctx, req, map[string]string{"provider": "all"}) + require.NoError(t, err) + + want, err := h.getCoverageBreakdown(ctx, req, map[string]string{}) + require.NoError(t, err) + assert.Equal(t, want, result, "provider=all must read the same as no provider") +} + +func TestListAccounts_RejectsUnknownEnumParams(t *testing.T) { + ctx := context.Background() + cases := map[string]map[string]string{ + "enabled=1": {"enabled": "1"}, + "enabled=yes": {"enabled": "yes"}, + "enabled=TRUE": {"enabled": "TRUE"}, + "provider=AWS": {"provider": "AWS"}, + "provider=ibm": {"provider": "ibm"}, + "both invalid": {"provider": "x", "enabled": "x"}, + "provider=aws ": {"provider": "aws "}, + } + lists := map[string]func(*Handler, map[string]string) error{ + "full": func(h *Handler, p map[string]string) error { + req := adminRequest("") + req.QueryStringParameters = p + _, err := h.listAccounts(ctx, req) + return err + }, + "minimal": func(h *Handler, p map[string]string) error { + req := adminRequest("") + req.QueryStringParameters = p + _, err := h.listAccountsMinimal(ctx, req) + return err + }, + } + for listName, list := range lists { + for name, params := range cases { + t.Run(listName+" "+name, func(t *testing.T) { + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + listed := false + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { + listed = true + return nil, nil + } + h := &Handler{auth: mockAuth, config: store} + h.sourceIdentityOnce.Do(func() {}) // no STS lookup from markSelfAccount + + requireBadRequest(t, list(h, params)) + assert.False(t, listed, "an invalid filter must be refused before the store is read") + }) + } + } +} + +func TestListAccounts_AcceptedFilterValues(t *testing.T) { + ctx := context.Background() + listWith := func(t *testing.T, params map[string]string) config.CloudAccountFilter { + t.Helper() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + var got config.CloudAccountFilter + store.ListCloudAccountsFn = func(_ context.Context, f config.CloudAccountFilter) ([]config.CloudAccount, error) { + got = f + return nil, nil + } + req := adminRequest("") + req.QueryStringParameters = params + h := &Handler{auth: mockAuth, config: store} + h.sourceIdentityOnce.Do(func() {}) // no STS lookup from markSelfAccount + _, err := h.listAccounts(ctx, req) + require.NoError(t, err) + return got + } + + t.Run("provider=all is no provider filter", func(t *testing.T) { + assert.Nil(t, listWith(t, map[string]string{"provider": "all"}).Provider) + }) + + t.Run("empty enabled is no enabled filter", func(t *testing.T) { + assert.Nil(t, listWith(t, map[string]string{"enabled": ""}).Enabled) + }) + + t.Run("valid provider and enabled pass through", func(t *testing.T) { + f := listWith(t, map[string]string{"provider": "gcp", "enabled": "false", "search": "prod"}) + require.NotNil(t, f.Provider) + assert.Equal(t, "gcp", *f.Provider) + require.NotNil(t, f.Enabled) + assert.False(t, *f.Enabled) + assert.Equal(t, "prod", f.Search) + }) +} diff --git a/internal/api/handler_users.go b/internal/api/handler_users.go index a2bb62f2..f5548c98 100644 --- a/internal/api/handler_users.go +++ b/internal/api/handler_users.go @@ -97,6 +97,9 @@ func mapAuthError(err error) error { errors.Is(err, auth.ErrLastAdmin), errors.Is(err, auth.ErrUserChanged): return NewClientError(409, err.Error()) + case errors.Is(err, auth.ErrUserNotFound): + // Fixed text: the wrapped error carries the store's "no rows" detail. + return NewClientError(404, "user not found") } return err } @@ -114,7 +117,7 @@ func (h *Handler) getUser(ctx context.Context, req *events.LambdaFunctionURLRequ user, err := h.auth.GetUser(ctx, userID) if err != nil { - return nil, err + return nil, mapAuthError(err) } return user, nil diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 4b9a7f79..60a542f9 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -126,10 +126,12 @@ paths: parameters: - name: provider in: query - description: Filter by cloud provider (aws, azure, gcp) + description: > + Filter by cloud provider. "all" or no value means every provider. + Any other value is a 400. schema: type: string - enum: [aws, azure, gcp] + enum: [aws, azure, gcp, all] - name: account_ids in: query description: > @@ -146,6 +148,8 @@ paths: application/json: schema: $ref: '#/components/schemas/DashboardSummary' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': @@ -3885,12 +3889,14 @@ paths: parameters: - name: provider in: query - description: Only return accounts of this provider (aws, azure or gcp). + description: > + Only return accounts of this provider (aws, azure or gcp). "all" + means every provider. Any other value is a 400. schema: type: string - name: enabled in: query - description: Only return enabled (true) or disabled (false) accounts. Other values are ignored. + description: Only return enabled (true) or disabled (false) accounts. Any other value is a 400. schema: type: boolean - name: search @@ -3907,6 +3913,8 @@ paths: type: array items: $ref: '#/components/schemas/AccountSummary' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': @@ -3923,12 +3931,14 @@ paths: parameters: - name: provider in: query - description: Only return accounts of this provider (aws, azure or gcp). + description: > + Only return accounts of this provider (aws, azure or gcp). "all" + means every provider. Any other value is a 400. schema: type: string - name: enabled in: query - description: Only return enabled (true) or disabled (false) accounts. Other values are ignored. + description: Only return enabled (true) or disabled (false) accounts. Any other value is a 400. schema: type: boolean - name: search @@ -3945,6 +3955,8 @@ paths: type: array items: $ref: '#/components/schemas/CloudAccount' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': @@ -4626,7 +4638,9 @@ paths: type: string - name: provider in: query - description: Exact, case-sensitive provider match. Not validated. + description: > + Only return commitments of this provider (aws, azure or gcp). + "all" means every provider. Any other value is a 400. schema: type: string responses: @@ -4636,6 +4650,8 @@ paths: application/json: schema: $ref: '#/components/schemas/InventoryCommitmentsResponse' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': @@ -4661,8 +4677,9 @@ paths: - name: provider in: query description: > - Exact provider match. Not validated. Other providers are still - listed, with no services. + Only fill in this provider (aws, azure or gcp); the other + providers are still listed, with no services. "all" means every + provider. Any other value is a 400. schema: type: string responses: @@ -4672,6 +4689,8 @@ paths: application/json: schema: $ref: '#/components/schemas/CoverageBreakdownResponse' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': diff --git a/internal/api/types.go b/internal/api/types.go index 3f098e9f..b3f7e9ab 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -753,15 +753,16 @@ type UpcomingPurchaseResponse struct { // to its parent PurchasePlan for display. // // The dashboard's Cancel button targets ExecutionID via -// DELETE /api/purchases/planned/{id} (api.deletePlannedPurchase) so the -// operator removes just THIS scheduled instance and leaves the plan -// template intact — the next scheduler tick re-creates the next instance -// for the plan. PlanID is exposed as context (e.g. for linking to the -// plan's settings) and is NOT what destructive action endpoints should -// target. PR #207 + #213 history: an earlier iteration routed Cancel to -// api.deletePlan(planID) which deleted the entire plan; that was too -// aggressive — operators usually want "skip this scheduled run", not -// "nuke the recurring template". +// DELETE /api/purchases/planned/{id} (api.deletePlannedPurchase). That +// cancels THIS scheduled instance and also DISABLES its plan (issue #774; +// DeletePlannedPurchaseResponse.PlanDisabled reports it): the plan row is +// kept, but the scheduler creates no further instances until someone +// re-enables it on the Plans page (issue #312 corrected this comment, which +// used to say the plan stays active). PlanID is exposed as context (e.g. for +// linking to the plan's settings) and is NOT what destructive action +// endpoints should target. PR #207 + #213 history: an earlier iteration +// routed Cancel to api.deletePlan(planID) which deleted the entire plan; +// that was too aggressive. type UpcomingPurchase struct { // CreatedByUserID propagates the underlying execution's // created_by_user_id so the dashboard widget can apply the same diff --git a/internal/api/validation.go b/internal/api/validation.go index 20476d47..b665337b 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -129,6 +129,20 @@ func validateProvider(provider string) error { return nil } +// providerFilterParam validates a read endpoint's provider query parameter +// and returns the value to filter on: "" (every provider) for an absent +// value or "all", else the provider. Anything validateProvider refuses is a +// 400, as on the recommendations and history reads (issue #312). +func providerFilterParam(provider string) (string, error) { + if err := validateProvider(provider); err != nil { + return "", err + } + if provider == "all" { + return "", nil + } + return provider, nil +} + // validateEmailFormat returns a 400 error when email is non-empty but does not // parse as an RFC 5322 address or lacks a TLD (e.g. name@hostonly). Empty // strings are accepted (contact email is optional on cloud accounts). diff --git a/internal/auth/errors.go b/internal/auth/errors.go index ec702f1c..b9e6f444 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -104,6 +104,19 @@ var ( // caller read it (issue #474). Mapped to 409. ErrUserChanged = errors.New("account changed concurrently; reload and retry") + // Lookup sentinels for the admin read and key-management endpoints + // (issue #312). Mapped to 404, as openapi.yaml documents, instead of + // surfacing the store's raw pgx.ErrNoRows as a 500. ErrUserNotFound and + // ErrGroupNotFound wrap pgx.ErrNoRows when the store returned it, so + // errors.Is(err, pgx.ErrNoRows) still holds for existing callers. + // ErrAPIKeyNotOwned: a non-admin acting on another user's key; the + // handler answers it with the same 404 as a missing key so the response + // does not confirm that the key exists. + ErrUserNotFound = errors.New("user not found") + ErrGroupNotFound = errors.New("group not found") + ErrAPIKeyNotFound = errors.New("API key not found") + ErrAPIKeyNotOwned = errors.New("unauthorized") + // MFA login-gate sentinels — used by the login API handler to map // to machine-readable response codes (mfa_required / // invalid_mfa_code) so the frontend can branch on the error class diff --git a/internal/auth/not_found_sentinels_test.go b/internal/auth/not_found_sentinels_test.go new file mode 100644 index 00000000..fdd30a19 --- /dev/null +++ b/internal/auth/not_found_sentinels_test.go @@ -0,0 +1,100 @@ +package auth + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #312 (audit A02-012): a missing user, group or API key reached the +// API layer as the store's raw pgx.ErrNoRows or as an untyped error, so the +// handlers could not tell it from a database failure and answered 500. The +// service now returns exported sentinels the handlers map to 404. + +func TestService_GetUser_MissingUserIsErrUserNotFound(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + mockStore.On("GetUserByID", ctx, "missing").Return(nil, pgx.ErrNoRows).Once() + + user, err := service.GetUser(ctx, "missing") + + require.Error(t, err) + assert.Nil(t, user) + assert.ErrorIs(t, err, ErrUserNotFound) + // Existing callers that test for the store's sentinel keep working. + assert.ErrorIs(t, err, pgx.ErrNoRows) +} + +func TestService_GetGroupAPI_MissingGroupIsErrGroupNotFound(t *testing.T) { + ctx := context.Background() + + t.Run("store returns ErrNoRows", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + mockStore.On("GetGroup", ctx, "missing").Return(nil, pgx.ErrNoRows).Once() + + _, err := service.GetGroupAPI(ctx, "missing") + + assert.ErrorIs(t, err, ErrGroupNotFound) + assert.ErrorIs(t, err, pgx.ErrNoRows) + }) + + t.Run("store returns nil group", func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + mockStore.On("GetGroup", ctx, "missing").Return(nil, nil).Once() + + _, err := service.GetGroupAPI(ctx, "missing") + + assert.ErrorIs(t, err, ErrGroupNotFound) + }) +} + +func TestService_APIKeyAccess_NotFoundAndNotOwnedSentinels(t *testing.T) { + ctx := context.Background() + actions := map[string]func(*Service) error{ + "revoke": func(s *Service) error { return s.RevokeAPIKeyAPI(ctx, "user-123", "key-1") }, + "delete": func(s *Service) error { return s.DeleteAPIKeyAPI(ctx, "user-123", "key-1") }, + } + + for name, act := range actions { + t.Run(name+" missing key", func(t *testing.T) { + mockStore := new(MockStore) + mockStore.On("GetAPIKeyByID", ctx, "key-1").Return(nil, pgx.ErrNoRows).Once() + + err := act(&Service{store: mockStore}) + + assert.ErrorIs(t, err, ErrAPIKeyNotFound) + }) + + t.Run(name+" nil key", func(t *testing.T) { + mockStore := new(MockStore) + mockStore.On("GetAPIKeyByID", ctx, "key-1").Return(nil, nil).Once() + + err := act(&Service{store: mockStore}) + + assert.ErrorIs(t, err, ErrAPIKeyNotFound) + }) + + t.Run(name+" another user's key", func(t *testing.T) { + mockStore := new(MockStore) + mockStore.On("GetAPIKeyByID", ctx, "key-1"). + Return(&UserAPIKey{ID: "key-1", UserID: "user-456", IsActive: true}, nil) + mockStore.On("GetUserByID", ctx, "user-123").Return(&User{ID: "user-123"}, nil) + + err := act(&Service{store: mockStore}) + + assert.ErrorIs(t, err, ErrAPIKeyNotOwned) + assert.NotErrorIs(t, err, ErrAPIKeyNotFound) + // The service-level message is unchanged. + assert.Contains(t, err.Error(), "unauthorized: cannot "+name+" another user's API key") + mockStore.AssertNotCalled(t, "UpdateAPIKey", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "DeleteAPIKey", mock.Anything, mock.Anything) + }) + } +} diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 0504f83a..4bc7c1eb 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -3,8 +3,11 @@ package auth import ( "context" "encoding/json" + "errors" "fmt" "time" + + "github.com/jackc/pgx/v5" ) // API adapter types - these match the types in internal/api/handler.go @@ -430,11 +433,14 @@ func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID strin // GetGroupAPI returns a group by ID via the API. func (s *Service) GetGroupAPI(ctx context.Context, groupID string) (any, error) { group, err := s.GetGroup(ctx, groupID) + if errors.Is(err, pgx.ErrNoRows) { + return nil, fmt.Errorf("%w: %w", ErrGroupNotFound, err) + } if err != nil { return nil, err } if group == nil { - return nil, fmt.Errorf("group not found") + return nil, ErrGroupNotFound } return groupToAPIGroup(group), nil } diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index d402bac4..509034a3 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -250,12 +250,12 @@ func (s *Service) authorizeAPIKeyAccess(ctx context.Context, userID, keyID, acti key, err := s.store.GetAPIKeyByID(ctx, keyID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - return nil, fmt.Errorf("API key not found") + return nil, ErrAPIKeyNotFound } return nil, fmt.Errorf("failed to get API key: %w", err) } if key == nil { - return nil, fmt.Errorf("API key not found") + return nil, ErrAPIKeyNotFound } if _, err := s.loadUser(ctx, userID); err != nil { @@ -268,7 +268,7 @@ func (s *Service) authorizeAPIKeyAccess(ctx context.Context, userID, keyID, acti return nil, fmt.Errorf("failed to check admin capability: %w", err) } if !isAdmin { - return nil, fmt.Errorf("unauthorized: cannot %s another user's API key", action) + return nil, fmt.Errorf("%w: cannot %s another user's API key", ErrAPIKeyNotOwned, action) } } diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index c9c29979..ac4455f7 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -777,9 +777,14 @@ func (s *Service) DeleteUser(ctx context.Context, userID string) error { return nil } -// GetUser returns user info. Returns (nil, pgx.ErrNoRows) if the user does not exist. +// GetUser returns user info. A user that does not exist returns an error +// matching both ErrUserNotFound and pgx.ErrNoRows (issue #312). func (s *Service) GetUser(ctx context.Context, userID string) (*User, error) { - return s.store.GetUserByID(ctx, userID) + user, err := s.store.GetUserByID(ctx, userID) + if errors.Is(err, pgx.ErrNoRows) { + return nil, fmt.Errorf("%w: %w", ErrUserNotFound, err) + } + return user, err } // UpdateUserProfile allows a user to update their own email and password. diff --git a/internal/server/adapter_not_found_test.go b/internal/server/adapter_not_found_test.go new file mode 100644 index 00000000..da293446 --- /dev/null +++ b/internal/server/adapter_not_found_test.go @@ -0,0 +1,35 @@ +package server + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" +) + +// Issue #312 (audit A02-012): GET /api/users/{id} answers 404 only if the +// adapter hands the handler auth.ErrUserNotFound for a missing user, whether +// the store reports it as pgx.ErrNoRows or as a nil user. +func TestAuthServiceAdapter_GetUser_MissingUserIsErrUserNotFound(t *testing.T) { + ctx := context.Background() + + t.Run("store returns ErrNoRows", func(t *testing.T) { + adapter, mockStore := createMockAuthService(t) + mockStore.On("GetUserByID", ctx, "missing").Return(nil, pgx.ErrNoRows) + + _, err := adapter.GetUser(ctx, "missing") + + assert.ErrorIs(t, err, auth.ErrUserNotFound) + }) + + t.Run("store returns nil user", func(t *testing.T) { + adapter, mockStore := createMockAuthService(t) + mockStore.On("GetUserByID", ctx, "missing").Return(nil, nil) + + _, err := adapter.GetUser(ctx, "missing") + + assert.ErrorIs(t, err, auth.ErrUserNotFound) + }) +} diff --git a/internal/server/app.go b/internal/server/app.go index 64f8076c..6a18524b 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1166,7 +1166,7 @@ func (a *authServiceAdapter) GetUser(ctx context.Context, userID string) (*api.U return nil, err } if user == nil { - return nil, fmt.Errorf("user not found") + return nil, auth.ErrUserNotFound } return &api.User{ ID: user.ID,