From d2848a8331241167749c5e9088782e5dd5ab5cb2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 05:11:11 +0000 Subject: [PATCH] fix(api): 404 for missing users/groups/API keys, 400 for bad read filters Three of the four low findings in the 2026-09-02 audit issue, re-verified on current main: - A02-012: GET /api/users/{id}, GET /api/groups/{id} and DELETE / revoke of an API key answered 500 for a missing row (raw pgx.ErrNoRows or an untyped error) where openapi.yaml documents 404. internal/auth now returns ErrUserNotFound, ErrGroupNotFound (both still match pgx.ErrNoRows), ErrAPIKeyNotFound and ErrAPIKeyNotOwned, and the handlers map them to 404. A non-admin acting on another user's key gets the same 404 as a missing key. - A02-021: the dashboard summary, both inventory reads and both account lists accepted any provider (and any "enabled") and answered 200 with an empty or unfiltered result. They now validate with validateProvider (400, as on recommendations/history) and treat provider "all" as no filter; enabled must be true or false. - A01-021: the UpcomingPurchase doc said canceling one upcoming purchase leaves its plan active, but since #774 the cancel also disables the plan. The Go contract comment now says so; the dashboard confirm copy is left to the UI owner. A02-007 (approveRegistration ordering) is left to the in-flight fix on fix/issue-1102 (#1141), which rewrites the same lines. Refs #312 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_accounts.go | 48 ++-- internal/api/handler_accounts_test.go | 17 +- internal/api/handler_apikeys.go | 16 +- internal/api/handler_dashboard.go | 8 +- internal/api/handler_groups.go | 3 + internal/api/handler_inventory.go | 18 +- .../api/handler_not_found_mapping_test.go | 152 +++++++++++ .../handler_query_param_validation_test.go | 235 ++++++++++++++++++ internal/api/handler_users.go | 5 +- internal/api/openapi.yaml | 2 + internal/api/types.go | 19 +- internal/api/validation.go | 14 ++ internal/auth/errors.go | 13 + internal/auth/not_found_sentinels_test.go | 100 ++++++++ internal/auth/service_api.go | 8 +- internal/auth/service_apikeys.go | 6 +- internal/auth/service_user.go | 9 +- internal/server/adapter_not_found_test.go | 35 +++ internal/server/app.go | 2 +- 19 files changed, 661 insertions(+), 49 deletions(-) create mode 100644 internal/api/handler_not_found_mapping_test.go create mode 100644 internal/api/handler_query_param_validation_test.go create mode 100644 internal/auth/not_found_sentinels_test.go create mode 100644 internal/server/adapter_not_found_test.go diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index c1d5baadd..c7b9a938a 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 { @@ -155,7 +158,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 { @@ -201,29 +207,35 @@ func (h *Handler) markSelfAccount(ctx context.Context, accts []config.CloudAccou } // 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 bf6dd62dd..df0a2f693 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 36933bc6d..a16f82d16 100644 --- a/internal/api/handler_apikeys.go +++ b/internal/api/handler_apikeys.go @@ -120,7 +120,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 @@ -140,12 +140,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 b174c3ae5..990e3d616 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 := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider) + activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, provider, accountUUIDs, accountExternalIDsByProvider) // Populate CurrentSavings on each per-service bucket from actual // purchase_history commitments. Services with recommendations but no diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index 13d2c2ca7..8be7d4ad5 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 1a482ccef..2e78133c2 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 000000000..fbe6e3359 --- /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 000000000..23249e88a --- /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 746fdbac3..64f75c490 100644 --- a/internal/api/handler_users.go +++ b/internal/api/handler_users.go @@ -96,6 +96,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 } @@ -113,7 +116,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 ba01d7f22..4117bf006 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -133,6 +133,8 @@ paths: application/json: schema: $ref: '#/components/schemas/DashboardSummary' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': diff --git a/internal/api/types.go b/internal/api/types.go index 9ff2a91f1..d5346db6e 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -741,15 +741,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 dd6575dbb..fb4dfd4a9 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -128,6 +128,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 af2753395..567339629 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -97,6 +97,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 000000000..fdd30a195 --- /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 bcdfa753b..5563bc6fd 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 @@ -444,11 +447,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 bd5585dc6..663e92d0b 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -278,12 +278,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 } user, err := s.store.GetUserByID(ctx, userID) @@ -303,7 +303,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 61e865d21..fa616e213 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -745,9 +745,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 000000000..da2934464 --- /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 fd92d96bf..28a2fb587 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1158,7 +1158,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,