fix(api): keep an explicit max_amount of 0 on API-key create so it is rejected

CHANGELOG.md

@@ -32,7 +32,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
-- Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit. API-key creation over HTTP is not covered yet (#980) (#133)
+- API-key creation over HTTP rejects an explicit `max_amount: 0` spend cap with a 400 instead of minting the key with no cap, matching group writes (#980)
+- Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit (#133)
 - Remove debug console.log from frontend recommendation handler
 - Align pre-commit gocyclo threshold (10) with CI pipeline
 - Pin tool versions in GitHub Actions for reproducible builds

internal/api/apikey_constraints_roundtrip_test.go

@@ -37,7 +37,7 @@ type realAPIKeyAuth struct {
 	service *auth.Service
 }
 
-func (a *realAPIKeyAuth) CreateAPIKeyAPI(ctx context.Context, userID string, req any) (any, error) {
+func (a *realAPIKeyAuth) CreateAPIKeyAPI(ctx context.Context, userID string, req auth.APICreateAPIKeyRequest) (any, error) {
 	return a.service.CreateAPIKeyAPI(ctx, userID, req)
 }
 

internal/api/apikey_explicit_zero_cap_test.go

@@ -0,0 +1,67 @@
+package api //nolint:revive // var-naming: package name "api" is intentional for handler package
+
+// Issue #980: POST /api/api-keys must reject an explicit max_amount of 0 with
+// a 400, as group writes do since #133. The handler used to decode into a
+// local type whose max_amount was a plain float64 with omitempty, so the
+// explicit 0 was erased before the auth layer could tell it from an absent
+// field, and the key was minted with no cap. These tests drive the real
+// handler into a real auth.Service (MockStore, no database).
+
+import (
+	"encoding/base64"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// apiKeyBodyWithConstraints builds a create body whose constraints object is
+// the raw JSON given, so literals such as 0.0 and -0 reach the decoder as
+// written (json.Marshal would normalize them).
+func apiKeyBodyWithConstraints(constraints string) string {
+	return `{"name":"capped key","password":"` +
+		base64.StdEncoding.EncodeToString([]byte(roundtripOwnerPw)) +
+		`","expires_at":"` + time.Now().Add(24*time.Hour).UTC().Format(time.RFC3339) +
+		`","permissions":[{"action":"view","resource":"recommendations","constraints":` +
+		constraints + `}]}`
+}
+
+func TestHandleRequest_CreateAPIKey_RejectsExplicitZeroMaxAmount(t *testing.T) {
+	for _, literal := range []string{`0`, `0.0`, `-0`, `0e5`} {
+		t.Run(literal, func(t *testing.T) {
+			h, _, stored := newAPIKeyRoundtripHandler(t)
+
+			resp := apiKeyRoundtripRequest(t, h, "POST",
+				apiKeyBodyWithConstraints(`{"regions":["us-east-1"],"max_amount":`+literal+`}`))
+
+			assert.Equal(t, 400, resp.StatusCode, "body: %s", resp.Body)
+			assert.Contains(t, resp.Body, "max_amount must be greater than 0")
+			assert.Nil(t, *stored, "a key with an explicit zero cap must not be stored")
+		})
+	}
+}
+
+// Absent and null keep meaning "no cap" (stored as 0), and a positive cap is
+// stored as sent, so the zero check does not over-reject.
+func TestHandleRequest_CreateAPIKey_AcceptsAbsentNullAndPositiveMaxAmount(t *testing.T) {
+	cases := map[string]float64{
+		`{"regions":["us-east-1"]}`:                   0,
+		`{"regions":["us-east-1"],"max_amount":null}`: 0,
+		`{"regions":["us-east-1"],"max_amount":5000}`: 5000,
+	}
+	for constraints, want := range cases {
+		t.Run(constraints, func(t *testing.T) {
+			h, _, stored := newAPIKeyRoundtripHandler(t)
+
+			resp := apiKeyRoundtripRequest(t, h, "POST", apiKeyBodyWithConstraints(constraints))
+
+			require.Equal(t, 200, resp.StatusCode, "body: %s", resp.Body)
+			require.NotNil(t, *stored)
+			require.Len(t, (*stored).Permissions, 1)
+			require.NotNil(t, (*stored).Permissions[0].Constraints)
+			assert.Equal(t, want, (*stored).Permissions[0].Constraints.MaxPurchaseAmount)
+			assert.Equal(t, []string{"us-east-1"}, (*stored).Permissions[0].Constraints.Regions)
+		})
+	}
+}

internal/api/handler_apikeys.go

@@ -57,7 +57,9 @@ func (h *Handler) createAPIKey(ctx context.Context, req *events.LambdaFunctionUR
 		}
 	}
 
-	var createReq CreateAPIKeyRequest
+	// Decode straight into the auth wire type so an explicit max_amount of 0
+	// reaches the explicit-zero check instead of being erased (issue #980).
+	var createReq auth.APICreateAPIKeyRequest
 	err = json.Unmarshal([]byte(req.Body), &createReq)
 	if err != nil {
 		return nil, NewClientError(400, "invalid request body")

internal/api/handler_apikeys_test.go

@@ -332,9 +332,8 @@ func TestHandler_createAPIKey_PasswordDecodedBeforeForwarding(t *testing.T) {
 	mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil)
 	mockAuth.grantAdmin()
 	mockRateLimiter.On("AllowWithUser", ctx, "user-123", "admin").Return(true, nil)
-	mockAuth.On("CreateAPIKeyAPI", ctx, "user-123", mock.MatchedBy(func(req any) bool {
-		ckReq, ok := req.(CreateAPIKeyRequest)
-		return ok && ckReq.Password == "correct-horse"
+	mockAuth.On("CreateAPIKeyAPI", ctx, "user-123", mock.MatchedBy(func(req auth.APICreateAPIKeyRequest) bool {
+		return req.Password == "correct-horse"
 	})).Return(map[string]string{"api_key": "new-key-value", "key_id": "key-123"}, nil)
 
 	handler := &Handler{auth: mockAuth, rateLimiter: mockRateLimiter}

internal/api/handler_coverage_test.go

@@ -397,7 +397,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) {
 		mockAuth.On("ValidateSession", ctx, "test-token").Return(&Session{UserID: "user-1"}, nil)
 		mockAuth.grantAdmin()
 		created := func() map[string]string { return map[string]string{"key_id": "key-1"} }
-		mockAuth.On("CreateAPIKeyAPI", ctx, "user-1", CreateAPIKeyRequest{Name: "My Key", Password: "pass123"}).Return(created(), nil)
+		mockAuth.On("CreateAPIKeyAPI", ctx, "user-1", auth.APICreateAPIKeyRequest{Name: "My Key", Password: "pass123"}).Return(created(), nil)
 
 		h := &Handler{auth: mockAuth}
 		router := NewRouter(h)

internal/api/handler_ri_exchange_test.go

@@ -1642,7 +1642,7 @@ func (m *mockAuthForExchange) HasPermissionForConstraintsAPI(_ context.Context,
 func (m *mockAuthForExchange) GetUserPermissionsAPI(_ context.Context, _ string) (any, error) {
 	return nil, nil
 }
-func (m *mockAuthForExchange) CreateAPIKeyAPI(_ context.Context, _ string, _ any) (any, error) {
+func (m *mockAuthForExchange) CreateAPIKeyAPI(_ context.Context, _ string, _ auth.APICreateAPIKeyRequest) (any, error) {
 	return nil, nil
 }
 func (m *mockAuthForExchange) ListUserAPIKeysAPI(_ context.Context, _ string) (any, error) {

internal/api/mocks_test.go

@@ -525,7 +525,7 @@ func (m *MockAuthService) GetAllowedAccountsAPI(ctx context.Context, userID stri
 }
 
 // API Key management mock methods.
-func (m *MockAuthService) CreateAPIKeyAPI(ctx context.Context, userID string, req interface{}) (interface{}, error) {
+func (m *MockAuthService) CreateAPIKeyAPI(ctx context.Context, userID string, req auth.APICreateAPIKeyRequest) (interface{}, error) {
 	args := m.Called(ctx, userID, req)
 	return args.Get(0), args.Error(1)
 }

internal/api/openapi.yaml

@@ -3163,8 +3163,8 @@ components:
           exclusiveMinimum: true
           description: >-
             Spend cap per purchase. Omit for no cap. An explicit 0 is rejected
-            with 400 on group writes (create and update); API-key creation
-            over HTTP does not enforce this yet.
+            with 400 on group writes (create and update) and on API-key
+            creation.
 
     # -- API Keys -----------------------------------------------------------
     CreateAPIKeyRequest:

internal/api/types.go

@@ -245,7 +245,9 @@ type AuthServiceInterface interface {
 	// Account access - returns the union of allowed_accounts from all user groups (empty = all access)
 	GetAllowedAccountsAPI(ctx context.Context, userID string) ([]string, error)
 	// API Key management
-	CreateAPIKeyAPI(ctx context.Context, userID string, req any) (any, error)
+	// CreateAPIKeyAPI takes the auth wire type directly: re-encoding a local
+	// copy erased an explicit max_amount of 0 (issue #980).
+	CreateAPIKeyAPI(ctx context.Context, userID string, req auth.APICreateAPIKeyRequest) (any, error)
 	ListUserAPIKeysAPI(ctx context.Context, userID string) (any, error)
 	// GetAPIKeysUsageStatsAPI returns the section-level usage summary
 	// (active count, fixed-window/lifetime request totals, top-3 most

internal/api/types_apikeys.go

@@ -1,15 +1,5 @@
 package api //nolint:revive // var-naming: package name "api" is intentional for this handler package
 
-import "time"
-
-// CreateAPIKeyRequest represents a request to create a new API key.
-type CreateAPIKeyRequest struct {
-	ExpiresAt   *time.Time   `json:"expires_at,omitempty"`
-	Name        string       `json:"name"`
-	Password    string       `json:"password"` //nolint:gosec // G117: intentional credential field in request struct -- base64-encoded like the login/MFA password fields, decoded by the handler and never re-stored
-	Permissions []Permission `json:"permissions,omitempty"`
-}
-
 // CreateAPIKeyResponse returns the newly created API key (only shown once).
 type CreateAPIKeyResponse struct {
 	Info   *APIKeyInfo `json:"info"`

internal/auth/explicit_zero_cap_test.go

@@ -61,8 +61,8 @@ func TestGroupWrite_RejectsExplicitZeroMaxAmount(t *testing.T) {
 	}
 }
 
-// The shared adapter also guards CreateAPIKeyAPI for a caller that decodes
-// auth.APICreateAPIKeyRequest itself. The HTTP key handler does not (#980).
+// The shared adapter also guards CreateAPIKeyAPI. The HTTP key handler decodes
+// auth.APICreateAPIKeyRequest itself, so this is the path it takes (#980).
 func TestCreateAPIKeyAPI_RejectsDecodedExplicitZeroMaxAmount(t *testing.T) {
 	var req APICreateAPIKeyRequest
 	require.NoError(t, json.Unmarshal([]byte(

internal/auth/purchase_amount_cap.go

@@ -66,8 +66,9 @@ func validateMaxPurchaseAmount(permIndex int, p Permission) error {
 // validateExplicitZeroCap refuses a permission whose JSON carried an explicit
 // max_amount of 0 (issue #133). Zero stays "no cap" when the field is absent
 // or null, but typing 0 into a cap field reads as "spend nothing" and silently
-// granting unlimited spend is the opposite. Applied to group writes; the HTTP
-// API-key path erases an explicit 0 before it gets here (#980).
+// granting unlimited spend is the opposite. Applied to group writes and to
+// API-key creation, whose handler decodes straight into APICreateAPIKeyRequest
+// so the explicit 0 survives to this check (#980).
 func validateExplicitZeroCap(permIndex int, p APIPermission) error {
 	if p.Constraints == nil || !p.Constraints.maxAmountSet || p.Constraints.MaxAmount != 0 {
 		return nil

internal/auth/service_apikeys_api.go

@@ -2,7 +2,6 @@ package auth
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"sort"
 	"time"
@@ -188,34 +187,16 @@ type APIListAPIKeysResponse struct {
 
 // CreateAPIKeyAPI creates a new API key and returns API-friendly response.
 //
-// req may be any struct whose JSON representation is compatible with
-// APICreateAPIKeyRequest (name, expires_at, permissions in the API wire shape:
-// APIPermission, with accounts/max_amount constraint tags, not the storage
-// shape). The handler lives in a different package (api) and passes
-// api.CreateAPIKeyRequest, which carries the same JSON fields but is a
-// distinct Go type. Permissions must stay APIPermission: decoding them into
-// Permission silently dropped the account and amount constraints. A direct type
-// assertion would always fail and return a 500; JSON re-encoding handles any
-// caller-package type transparently (issue #1440).
-func (s *Service) CreateAPIKeyAPI(ctx context.Context, userID string, req any) (any, error) {
-	var createReq APICreateAPIKeyRequest
-	switch r := req.(type) {
-	case APICreateAPIKeyRequest:
-		// Fast path: caller already supplies our concrete type (service tests,
-		// future callers that import this package directly).
-		createReq = r
-	default:
-		// Cross-package path: re-encode to JSON and back so any struct with
-		// compatible json tags (e.g. api.CreateAPIKeyRequest) is accepted.
-		b, err := json.Marshal(r)
-		if err != nil {
-			return nil, fmt.Errorf("invalid request: %w", err)
-		}
-		if err := json.Unmarshal(b, &createReq); err != nil {
-			return nil, fmt.Errorf("invalid request: %w", err)
-		}
-	}
-
+// createReq is the API wire shape (APIPermission, with accounts/max_amount
+// constraint tags), which the HTTP handler decodes the request body into
+// directly. It used to be `any` and was re-encoded through JSON so a handler
+// type with the same tags would be accepted (issue #1440). That re-encode
+// dropped constraint fields when the tags drifted (#950) and erased an
+// explicit max_amount of 0, which omitempty cannot tell from an absent field,
+// so the key was minted with no cap instead of being refused (#980). Taking
+// the concrete type keeps the decode-time explicit-zero signal that
+// apiPermissionsToPermissions checks.
+func (s *Service) CreateAPIKeyAPI(ctx context.Context, userID string, createReq APICreateAPIKeyRequest) (any, error) {
 	perms, err := apiPermissionsToPermissions(createReq.Permissions)
 	if err != nil {
 		return nil, err

internal/auth/service_apikeys_api_test.go

@@ -119,66 +119,6 @@ func TestService_CreateAPIKeyAPI(t *testing.T) {
 	})
 }
 
-// TestService_CreateAPIKeyAPI_CrossPackageType is the regression test for
-// issue #1440. The HTTP handler (internal/api) unmarshals the request body
-// into api.CreateAPIKeyRequest - a struct in a different package that has
-// the same json tags as APICreateAPIKeyRequest but is a distinct Go type.
-// The former type-assertion implementation always returned "invalid request
-// type", causing a 500. The fix uses JSON re-encoding so any struct with
-// compatible json fields is accepted.
-func TestService_CreateAPIKeyAPI_CrossPackageType(t *testing.T) {
-	ctx := context.Background()
-	mockStore := new(MockStore)
-	t.Cleanup(func() { mockStore.AssertExpectations(t) })
-	service := &Service{store: mockStore}
-
-	user := &User{
-		ID:           "user-123",
-		Email:        "test@example.com",
-		Active:       true,
-		PasswordHash: testAPIKeyPasswordHash,
-		GroupIDs:     []string{DefaultAdminGroupID},
-	}
-	mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
-	mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(&Group{
-		ID:          DefaultAdminGroupID,
-		Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}},
-	}, nil)
-	mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil)
-
-	// Simulate what the api.Handler does: an anonymous struct with the same
-	// json field names but a different Go type than APICreateAPIKeyRequest.
-	// This is exactly the value passed by the handler in production (issue #1440).
-	expiresAt := time.Now().Add(24 * time.Hour)
-	crossPkgReq := struct {
-		Name        string     `json:"name"`
-		Password    string     `json:"password"`
-		ExpiresAt   *time.Time `json:"expires_at,omitempty"`
-		Permissions []struct {
-			Action   string `json:"action"`
-			Resource string `json:"resource"`
-		} `json:"permissions,omitempty"`
-	}{
-		Name:      "My API Key",
-		Password:  testAPIKeyPassword,
-		ExpiresAt: &expiresAt,
-		Permissions: []struct {
-			Action   string `json:"action"`
-			Resource string `json:"resource"`
-		}{{Action: ActionView, Resource: ResourceRecommendations}},
-	}
-
-	result, err := service.CreateAPIKeyAPI(ctx, "user-123", crossPkgReq)
-
-	// Before the fix this returned "invalid request type" (500); now it must succeed.
-	require.NoError(t, err, "CreateAPIKeyAPI must accept cross-package types with compatible JSON fields (issue #1440)")
-	require.NotNil(t, result)
-	resp, ok := result.(*APICreateAPIKeyResponse)
-	require.True(t, ok)
-	assert.NotEmpty(t, resp.APIKey)
-	assert.Equal(t, "My API Key", resp.Info.Name)
-}
-
 func TestService_ListUserAPIKeysAPI(t *testing.T) {
 	ctx := context.Background()
 

internal/server/adapter_test.go

@@ -427,9 +427,9 @@ func TestAuthServiceAdapter_CreateAPIKeyAPI(t *testing.T) {
 	require.NotNil(t, result)
 }
 
-// The handler passes api.CreateAPIKeyRequest, whose constraint JSON tags
-// (accounts, max_amount) differ from auth.PermissionConstraints. Every
-// constraint field must reach the store; a dropped one widens the key's scope.
+// The wire constraint tags (accounts, max_amount) differ from
+// auth.PermissionConstraints. Every constraint field must reach the store; a
+// dropped one widens the key's scope.
 func TestAuthServiceAdapter_CreateAPIKeyAPI_KeepsPermissionConstraints(t *testing.T) {
 	adapter, mockStore := createMockAuthService(t)
 	ctx := context.Background()
@@ -450,13 +450,13 @@ func TestAuthServiceAdapter_CreateAPIKeyAPI_KeepsPermissionConstraints(t *testin
 		Run(func(args mock.Arguments) { stored = args.Get(1).(*auth.UserAPIKey) }).Return(nil)
 
 	expiresAt := time.Now().Add(24 * time.Hour)
-	_, err := adapter.CreateAPIKeyAPI(ctx, "user-1", api.CreateAPIKeyRequest{
+	_, err := adapter.CreateAPIKeyAPI(ctx, "user-1", auth.APICreateAPIKeyRequest{
 		Name:      "scoped",
 		Password:  password,
 		ExpiresAt: &expiresAt,
-		Permissions: []api.Permission{{
+		Permissions: []auth.APIPermission{{
 			Action: auth.ActionView, Resource: auth.ResourceRecommendations,
-			Constraints: &api.PermissionConstraint{
+			Constraints: &auth.APIPermissionConstraint{
 				Accounts: []string{"123"}, Providers: []string{"aws"}, Services: []string{"ec2"},
 				Regions: []string{"us-east-1"}, MaxAmount: 500,
 			},

internal/server/apikey_admin_constraints_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/LeanerCloud/cloud-commitments-platform/internal/api"
 	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -40,13 +39,13 @@ func TestAuthServiceAdapter_ConstrainedAdminKeyRefusedOutOfScope(t *testing.T) {
 		Run(func(args mock.Arguments) { stored = args.Get(1).(*auth.UserAPIKey) }).Return(nil)
 
 	expiresAt := time.Now().Add(24 * time.Hour)
-	_, err := adapter.CreateAPIKeyAPI(ctx, "user-1", api.CreateAPIKeyRequest{
+	_, err := adapter.CreateAPIKeyAPI(ctx, "user-1", auth.APICreateAPIKeyRequest{
 		Name:      "admin-scoped",
 		Password:  ownerPass,
 		ExpiresAt: &expiresAt,
-		Permissions: []api.Permission{{
+		Permissions: []auth.APIPermission{{
 			Action: auth.ActionAdmin, Resource: auth.ResourceAll,
-			Constraints: &api.PermissionConstraint{Accounts: []string{"acct-a"}, MaxAmount: 500},
+			Constraints: &auth.APIPermissionConstraint{Accounts: []string{"acct-a"}, MaxAmount: 500},
 		}},
 	})
 	require.NoError(t, err)

internal/server/app.go

@@ -1262,7 +1262,7 @@ func (a *authServiceAdapter) ValidateCSRFToken(ctx context.Context, sessionToken
 }
 
 // API Key management.
-func (a *authServiceAdapter) CreateAPIKeyAPI(ctx context.Context, userID string, req any) (any, error) {
+func (a *authServiceAdapter) CreateAPIKeyAPI(ctx context.Context, userID string, req auth.APICreateAPIKeyRequest) (any, error) {
 	return a.service.CreateAPIKeyAPI(ctx, userID, req)
 }