diff --git a/CHANGELOG.md b/CHANGELOG.md index 01744a46..5b5eeb0a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/api/apikey_constraints_roundtrip_test.go b/internal/api/apikey_constraints_roundtrip_test.go index b1154602..64d76571 100644 --- a/internal/api/apikey_constraints_roundtrip_test.go +++ b/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) } diff --git a/internal/api/apikey_explicit_zero_cap_test.go b/internal/api/apikey_explicit_zero_cap_test.go new file mode 100644 index 00000000..eb02da7d --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/api/handler_apikeys.go b/internal/api/handler_apikeys.go index 36933bc6..7406e9cd 100644 --- a/internal/api/handler_apikeys.go +++ b/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") diff --git a/internal/api/handler_apikeys_test.go b/internal/api/handler_apikeys_test.go index 962d4c41..2757e0cc 100644 --- a/internal/api/handler_apikeys_test.go +++ b/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} diff --git a/internal/api/handler_coverage_test.go b/internal/api/handler_coverage_test.go index 9dd7077d..ee4c945b 100644 --- a/internal/api/handler_coverage_test.go +++ b/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) diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 51d463a8..81e4288f 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/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) { diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 2851eb04..7927beee 100644 --- a/internal/api/mocks_test.go +++ b/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) } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ba01d7f2..4adf571e 100644 --- a/internal/api/openapi.yaml +++ b/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: diff --git a/internal/api/types.go b/internal/api/types.go index 9ff2a91f..0b57694e 100644 --- a/internal/api/types.go +++ b/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 diff --git a/internal/api/types_apikeys.go b/internal/api/types_apikeys.go index 25889c51..1a92c056 100644 --- a/internal/api/types_apikeys.go +++ b/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"` diff --git a/internal/auth/explicit_zero_cap_test.go b/internal/auth/explicit_zero_cap_test.go index 654555e1..2491894d 100644 --- a/internal/auth/explicit_zero_cap_test.go +++ b/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( diff --git a/internal/auth/purchase_amount_cap.go b/internal/auth/purchase_amount_cap.go index 2d7509a0..84bdead5 100644 --- a/internal/auth/purchase_amount_cap.go +++ b/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 diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 20fa2131..8857fc9e 100644 --- a/internal/auth/service_apikeys_api.go +++ b/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 diff --git a/internal/auth/service_apikeys_api_test.go b/internal/auth/service_apikeys_api_test.go index bb4bc74b..762db2ca 100644 --- a/internal/auth/service_apikeys_api_test.go +++ b/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() diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index a1dfd4fe..b33d1c28 100644 --- a/internal/server/adapter_test.go +++ b/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, }, diff --git a/internal/server/apikey_admin_constraints_test.go b/internal/server/apikey_admin_constraints_test.go index 1e115b45..10e6a54b 100644 --- a/internal/server/apikey_admin_constraints_test.go +++ b/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) diff --git a/internal/server/app.go b/internal/server/app.go index fd92d96b..255b3519 100644 --- a/internal/server/app.go +++ b/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) }