From 04857438c984865daa7385f4d889da127cc140ba Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 11:49:41 +0100 Subject: [PATCH] fix(auth): keep accounts and max_amount on API-key create and list POST /api/api-keys decodes permissions in the documented shape (constraints.accounts / constraints.max_amount), but CreateAPIKeyAPI JSON-round-tripped the request into a struct whose permissions used the storage shape (account_ids / max_purchase_amount). The unmarshal ignored the two mismatched fields, so a key meant to be limited to certain accounts or a purchase cap was stored without those limits. The same storage shape leaked into the create and list responses. APICreateAPIKeyRequest now carries []APIPermission and is mapped to the storage type with apiPermissionsToPermissions, as the group endpoints already do. APIKeyInfo.Permissions is returned as []APIPermission, so the responses match openapi.yaml. Because the converted permissions now carry the requested cap, the negative spend-cap check from #83 is also reachable over HTTP. Closes #951 Co-Authored-By: Claude Opus 5.5 --- .../api/apikey_constraints_roundtrip_test.go | 223 ++++++++++++++++++ internal/auth/service_apikeys_api.go | 41 ++-- internal/auth/service_apikeys_api_test.go | 6 +- internal/server/adapter_test.go | 2 +- 4 files changed, 254 insertions(+), 18 deletions(-) create mode 100644 internal/api/apikey_constraints_roundtrip_test.go diff --git a/internal/api/apikey_constraints_roundtrip_test.go b/internal/api/apikey_constraints_roundtrip_test.go new file mode 100644 index 000000000..b8bb94b64 --- /dev/null +++ b/internal/api/apikey_constraints_roundtrip_test.go @@ -0,0 +1,223 @@ +package api //nolint:revive // var-naming: package name "api" is intentional for handler package + +import ( + "context" + "encoding/base64" + "encoding/json" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #951: POST /api/api-keys decodes permissions in the documented +// OpenAPI shape (constraints.accounts / constraints.max_amount). These tests +// drive the real HTTP handler into the real auth.Service (backed by +// auth.MockStore, so no database) and check that the account and amount +// constraints reach the stored key, and that the create and list responses +// return them in the same documented shape. + +const ( + roundtripUserID = "user-951" + roundtripToken = "session-951" + roundtripCSRF = "csrf-951" + roundtripAccount = "2f1c8e40-0000-4000-8000-000000000951" + roundtripOwnerPw = "Owner-951-pass" +) + +// realAPIKeyAuth answers session and permission checks from MockAuthService +// but sends the API-key create and list calls to a real auth.Service, so the +// handler-to-service conversion under test is the production one. +type realAPIKeyAuth struct { + *MockAuthService + service *auth.Service +} + +func (a *realAPIKeyAuth) CreateAPIKeyAPI(ctx context.Context, userID string, req any) (any, error) { + return a.service.CreateAPIKeyAPI(ctx, userID, req) +} + +func (a *realAPIKeyAuth) ListUserAPIKeysAPI(ctx context.Context, userID string) (any, error) { + return a.service.ListUserAPIKeysAPI(ctx, userID) +} + +// newAPIKeyRoundtripHandler returns a handler wired to a real auth.Service, +// the store behind it, and a pointer that receives the key the service hands +// to the store. +func newAPIKeyRoundtripHandler(t *testing.T) (*Handler, *auth.MockStore, **auth.UserAPIKey) { + t.Helper() + store := new(auth.MockStore) + user := &auth.User{ + ID: roundtripUserID, + Email: "owner@example.com", + Active: true, + PasswordHash: auth.TestPasswordHash(t, roundtripOwnerPw), + GroupIDs: []string{auth.DefaultAdminGroupID}, + } + store.On("GetUserByID", mock.Anything, roundtripUserID).Return(user, nil) + store.On("GetGroup", mock.Anything, auth.DefaultAdminGroupID).Return(&auth.Group{ + ID: auth.DefaultAdminGroupID, + Permissions: []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, + }, nil) + + stored := new(*auth.UserAPIKey) + store.On("CreateAPIKey", mock.Anything, mock.AnythingOfType("*auth.UserAPIKey")). + Run(func(args mock.Arguments) { *stored = args.Get(1).(*auth.UserAPIKey) }). + Return(nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, roundtripToken). + Return(&Session{UserID: roundtripUserID, Email: user.Email}, nil) + mockAuth.On("ValidateCSRFToken", mock.Anything, roundtripToken, roundtripCSRF).Return(nil).Maybe() + mockAuth.grantAdmin() + + service := auth.NewService(auth.ServiceConfig{Store: store, CSRFKey: auth.TestCSRFKey()}) + return &Handler{auth: &realAPIKeyAuth{MockAuthService: mockAuth, service: service}}, store, stored +} + +func apiKeyRoundtripRequest(t *testing.T, h *Handler, method, body string) *events.LambdaFunctionURLResponse { + t.Helper() + resp, err := h.HandleRequest(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "Authorization": "Bearer " + roundtripToken, + "Content-Type": "application/json", + "X-CSRF-Token": roundtripCSRF, + }, + Body: body, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: method, Path: "/api/api-keys"}, + }, + }) + require.NoError(t, err) + require.NotNil(t, resp) + return resp +} + +func constrainedAPIKeyBody(t *testing.T, maxAmount float64) string { + t.Helper() + body, err := json.Marshal(map[string]any{ + "name": "scoped key", + "password": base64.StdEncoding.EncodeToString([]byte(roundtripOwnerPw)), + "expires_at": time.Now().Add(24 * time.Hour).UTC().Format(time.RFC3339), + "permissions": []map[string]any{{ + "action": auth.ActionView, + "resource": auth.ResourceRecommendations, + "constraints": map[string]any{ + "accounts": []string{roundtripAccount}, + "providers": []string{"aws"}, + "services": []string{"ec2"}, + "regions": []string{"us-east-1"}, + "max_amount": maxAmount, + }, + }}, + }) + require.NoError(t, err) + return string(body) +} + +// requireDocumentedConstraints asserts a serialized constraints object uses +// the OpenAPI field names and carries every dimension that was requested. +func requireDocumentedConstraints(t *testing.T, constraints map[string]any) { + t.Helper() + assert.Equal(t, []any{roundtripAccount}, constraints["accounts"]) + assert.Equal(t, 500.5, constraints["max_amount"]) + assert.Equal(t, []any{"aws"}, constraints["providers"]) + assert.Equal(t, []any{"ec2"}, constraints["services"]) + assert.Equal(t, []any{"us-east-1"}, constraints["regions"]) + assert.NotContains(t, constraints, "account_ids", "response must use the documented field name") + assert.NotContains(t, constraints, "max_purchase_amount", "response must use the documented field name") +} + +func firstPermissionConstraints(t *testing.T, perms any) map[string]any { + t.Helper() + list, ok := perms.([]any) + require.True(t, ok, "permissions = %#v", perms) + require.Len(t, list, 1) + perm, ok := list[0].(map[string]any) + require.True(t, ok) + constraints, ok := perm["constraints"].(map[string]any) + require.True(t, ok, "constraints missing from %#v", perm) + return constraints +} + +func TestHandleRequest_CreateAPIKey_KeepsAccountAndAmountConstraints(t *testing.T) { + h, _, stored := newAPIKeyRoundtripHandler(t) + + resp := apiKeyRoundtripRequest(t, h, "POST", constrainedAPIKeyBody(t, 500.5)) + require.Equal(t, 200, resp.StatusCode, "body: %s", resp.Body) + + // The key handed to the store is what authorization reads later. + require.NotNil(t, *stored, "the service never reached the store") + require.Len(t, (*stored).Permissions, 1) + got := (*stored).Permissions[0].Constraints + require.NotNil(t, got, "constraints were dropped entirely") + assert.Equal(t, []string{roundtripAccount}, got.AccountIDs, "accounts constraint was dropped") + assert.InDelta(t, 500.5, got.MaxPurchaseAmount, 0, "max_amount constraint was dropped") + assert.Equal(t, []string{"aws"}, got.Providers) + assert.Equal(t, []string{"ec2"}, got.Services) + assert.Equal(t, []string{"us-east-1"}, got.Regions) + + var created struct { + Info struct { + Permissions any `json:"permissions"` + } `json:"info"` + } + require.NoError(t, json.Unmarshal([]byte(resp.Body), &created)) + requireDocumentedConstraints(t, firstPermissionConstraints(t, created.Info.Permissions)) +} + +func TestHandleRequest_ListAPIKeys_ReturnsDocumentedConstraintShape(t *testing.T) { + h, store, _ := newAPIKeyRoundtripHandler(t) + + // Seeded directly in the stored (auth) shape so this checks the listing + // response independently of the create path. + store.On("ListAPIKeysByUser", mock.Anything, roundtripUserID).Return([]*auth.UserAPIKey{{ + ID: "key-951", + UserID: roundtripUserID, + Name: "scoped key", + IsActive: true, + Permissions: []auth.Permission{{ + Action: auth.ActionView, + Resource: auth.ResourceRecommendations, + Constraints: &auth.PermissionConstraints{ + AccountIDs: []string{roundtripAccount}, + Providers: []string{"aws"}, + Services: []string{"ec2"}, + Regions: []string{"us-east-1"}, + MaxPurchaseAmount: 500.5, + }, + }}, + }}, nil) + + resp := apiKeyRoundtripRequest(t, h, "GET", "") + require.Equal(t, 200, resp.StatusCode, "body: %s", resp.Body) + + var listed struct { + APIKeys []struct { + Permissions any `json:"permissions"` + } `json:"api_keys"` + } + require.NoError(t, json.Unmarshal([]byte(resp.Body), &listed)) + require.Len(t, listed.APIKeys, 1) + requireDocumentedConstraints(t, firstPermissionConstraints(t, listed.APIKeys[0].Permissions)) +} + +// The negative spend-cap check from issue #83 runs on the converted +// permissions, so it must now be reachable over HTTP. Before the #951 fix the +// negative max_amount was dropped during decoding and the key was minted with +// no cap at all. +func TestHandleRequest_CreateAPIKey_RejectsNegativeMaxAmount(t *testing.T) { + h, _, stored := newAPIKeyRoundtripHandler(t) + + resp := apiKeyRoundtripRequest(t, h, "POST", constrainedAPIKeyBody(t, -1)) + + assert.Equal(t, 400, resp.StatusCode, "body: %s", resp.Body) + assert.Contains(t, resp.Body, "invalid max purchase amount") + // stored is set by the store's CreateAPIKey mock, so nil means the + // service never wrote the key. + assert.Nil(t, *stored, "a key with a negative cap must not be stored") +} diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 1d3ae91fd..4bb22ddce 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -72,30 +72,37 @@ func effectiveLifetimeUsage(key *UserAPIKey) *int64 { return &total } -// normalizeAPIKeyPermissions returns perms unchanged, or []Permission{} when -// nil. CreateAPIKey now rejects new keys with zero permissions (issue #61), +// normalizeAPIKeyPermissions converts a stored key's permissions to the +// documented wire shape (APIPermission: constraints.accounts / +// constraints.max_amount, issue #951), returning []APIPermission{} when perms +// is nil. CreateAPIKey now rejects new keys with zero permissions (issue #61), // but a legacy key minted before that fix can still have a NULL permissions // column, which the JSONB scan surfaces as a nil slice. Since // APIKeyInfo.Permissions is no longer omitempty (issue #61), a raw nil would // serialize as "permissions":null instead of the intended "permissions":[] // -- both mean "no scope", but a client that expects the field to always be // an array (never null) would otherwise have to special-case it. -func normalizeAPIKeyPermissions(perms []Permission) []Permission { - if perms == nil { - return []Permission{} +func normalizeAPIKeyPermissions(perms []Permission) []APIPermission { + out := make([]APIPermission, len(perms)) + for i, p := range perms { + out[i] = permissionToAPIPermission(p) } - return perms + return out } // API wrapper methods for API key operations // These methods return API-friendly types and handle type conversions // APICreateAPIKeyRequest represents the API request to create an API key. +// Permissions use the documented wire shape (APIPermission, whose +// constraints carry accounts / max_amount) rather than the storage shape +// (Permission: account_ids / max_purchase_amount), so the JSON re-encoding in +// CreateAPIKeyAPI keeps every constraint the caller sent (issue #951). type APICreateAPIKeyRequest 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 -- decoded by the handler and verified against the caller's own stored hash, never re-stored - Permissions []Permission `json:"permissions,omitempty"` + ExpiresAt *time.Time `json:"expires_at,omitempty"` + Name string `json:"name"` + Password string `json:"password"` //nolint:gosec // G117: intentional credential field in request struct -- decoded by the handler and verified against the caller's own stored hash, never re-stored + Permissions []APIPermission `json:"permissions,omitempty"` } // APIKeyInfo represents public API key information (without sensitive data). @@ -118,8 +125,11 @@ type APIKeyInfo struct { // simply has no info populated. Hiding it behind omitempty made a // zero-scope key indistinguishable from a fully-scoped one in // listAPIKeys, which was itself part of the audit gap the issue raised. - Permissions []Permission `json:"permissions"` - IsActive bool `json:"is_active"` + // It uses the documented wire shape (constraints.accounts / + // constraints.max_amount), the same one the create request accepts + // (issue #951). + Permissions []APIPermission `json:"permissions"` + IsActive bool `json:"is_active"` // Usage counters (issue #340/#344 deferred sub-task). RequestCountWindow // is a fixed/tumbling window count, not a true rolling 24h total, and is // zero once that window has closed -- see effectiveWindowUsage, @@ -187,7 +197,9 @@ type APIListAPIKeysResponse struct { // in a different package (api) and passes api.CreateAPIKeyRequest, which // carries the same JSON fields but is a distinct Go type. A direct type // assertion would always fail and return a 500; JSON re-encoding handles any -// caller-package type transparently (issue #1440). +// caller-package type transparently (issue #1440). The permissions are then +// mapped field by field from the wire shape to the storage shape, as the +// group endpoints do, so accounts and max_amount survive (issue #951). func (s *Service) CreateAPIKeyAPI(ctx context.Context, userID string, req any) (any, error) { var createReq APICreateAPIKeyRequest switch r := req.(type) { @@ -208,7 +220,8 @@ func (s *Service) CreateAPIKeyAPI(ctx context.Context, userID string, req any) ( } // Create the API key - apiKey, keyInfo, err := s.CreateAPIKey(ctx, userID, createReq.Name, createReq.Password, createReq.Permissions, createReq.ExpiresAt) + permissions := apiPermissionsToPermissions(createReq.Permissions) + apiKey, keyInfo, err := s.CreateAPIKey(ctx, userID, createReq.Name, createReq.Password, permissions, createReq.ExpiresAt) 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 7c5af2cb1..231f5b34f 100644 --- a/internal/auth/service_apikeys_api_test.go +++ b/internal/auth/service_apikeys_api_test.go @@ -27,7 +27,7 @@ func TestService_CreateAPIKeyAPI(t *testing.T) { GroupIDs: []string{DefaultAdminGroupID}, } - permissions := []Permission{ + permissions := []APIPermission{ {Action: ActionView, Resource: ResourceRecommendations}, } @@ -74,7 +74,7 @@ func TestService_CreateAPIKeyAPI(t *testing.T) { req := APICreateAPIKeyRequest{ Name: "Test API Key", Password: testAPIKeyPassword, - Permissions: []Permission{{Action: ActionView, Resource: ResourceRecommendations}}, + Permissions: []APIPermission{{Action: ActionView, Resource: ResourceRecommendations}}, ExpiresAt: &expiresAt, } @@ -106,7 +106,7 @@ func TestService_CreateAPIKeyAPI(t *testing.T) { req := APICreateAPIKeyRequest{ Name: "Test API Key", - Permissions: []Permission{}, + Permissions: []APIPermission{}, } mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index 12bb76e58..54bda1c5c 100644 --- a/internal/server/adapter_test.go +++ b/internal/server/adapter_test.go @@ -420,7 +420,7 @@ func TestAuthServiceAdapter_CreateAPIKeyAPI(t *testing.T) { result, err := adapter.CreateAPIKeyAPI(ctx, "user-1", auth.APICreateAPIKeyRequest{ Name: "my-key", Password: password, - Permissions: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, + Permissions: []auth.APIPermission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, ExpiresAt: &expiresAt, }) require.NoError(t, err)