fix(api): 404 for missing users/groups/API keys, 400 for bad read filters (#312)

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.

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)

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) {

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

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
 	}

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
 }

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)
+}