From 244a8dcd65bddccd5417f871c08c3dfaef824c6c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 04:47:20 +0200 Subject: [PATCH 1/4] sec(auth): unscoped API key fails closed instead of inheriting owner permissions Summary: An API key created without explicit scopes silently inherited the owning user's full permission set at request time (computeEffectivePermissionsFromAuthCtx). "Leave scope blank" was the most permissive option in the UI, and a long-lived unscoped key became admin-capable the moment its owner joined the Administrators group. Root cause: computeEffectivePermissionsFromAuthCtx returned authCtx.Permissions (the owner's full set) when the key carried zero permissions, instead of treating "no scopes granted" as "no permissions granted". Fix (defense in depth, per the issue's proposed fix): 1. computeEffectivePermissionsFromAuthCtx now returns []Permission{} for a zero-permission key. An unscoped key still authenticates its owner but authorizes nothing. 2. CreateAPIKey rejects len(permissions) == 0 with the new ErrEmptyAPIKeyPermissions sentinel, mapped to 400 in the createAPIKey handler. Scoping is now a deliberate choice at creation time, not a default. 3. APIKeyInfo.Permissions in listAPIKeys/create responses is no longer omitempty, so a legacy zero-scope key (minted before this fix) is visible in listings instead of looking identical to a fully-scoped one. Extracted loadActiveUserForAPIKey out of CreateAPIKey to keep its cyclomatic complexity under the project's gocyclo gate (10 -> 11 with the new check -> 8 after the extraction). Co-Authored-By: claude-flow --- internal/api/handler_apikeys.go | 6 +- internal/api/openapi.yaml | 8 +- internal/auth/errors.go | 7 ++ internal/auth/service_apikeys.go | 78 +++++++++++++------- internal/auth/service_apikeys_api.go | 25 +++++-- internal/auth/service_apikeys_api_test.go | 4 + internal/auth/service_apikeys_test.go | 71 ++++++++++++++++-- internal/server/adapter_test.go | 7 +- internal/server/apikey_usage_booking_test.go | 17 +++-- 9 files changed, 171 insertions(+), 52 deletions(-) diff --git a/internal/api/handler_apikeys.go b/internal/api/handler_apikeys.go index cef22d56e..8486a7125 100644 --- a/internal/api/handler_apikeys.go +++ b/internal/api/handler_apikeys.go @@ -85,6 +85,9 @@ func (h *Handler) createAPIKey(ctx context.Context, req *events.LambdaFunctionUR // - ErrAPIKeyInvalidPassword -> 401: the acting user is re-verifying their // own credential, so a precise message is safe (same treatment as // ErrCurrentPasswordIncorrect). +// - ErrEmptyAPIKeyPermissions -> 400: malformed/insufficient input, not a +// server failure -- surface the specific reason instead of a generic +// 500 (issue #61). // - ErrAPIKeyExpiresAtRequired / ErrAPIKeyExpiresAtTooFar / ErrAPIKeyExpiresAtInPast // -> 400: caller-correctable request validation. // - All other errors pass through unchanged for handleRequestError to @@ -93,7 +96,8 @@ func mapCreateAPIKeyError(err error) error { switch { case errors.Is(err, auth.ErrAPIKeyInvalidPassword): return NewClientError(401, err.Error()) - case errors.Is(err, auth.ErrAPIKeyExpiresAtRequired), + case errors.Is(err, auth.ErrEmptyAPIKeyPermissions), + errors.Is(err, auth.ErrAPIKeyExpiresAtRequired), errors.Is(err, auth.ErrAPIKeyExpiresAtTooFar), errors.Is(err, auth.ErrAPIKeyExpiresAtInPast): return NewClientError(400, err.Error()) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 76581421a..f8cdf8545 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2290,7 +2290,7 @@ components: # -- API Keys ----------------------------------------------------------- CreateAPIKeyRequest: type: object - required: [name, password, expires_at] + required: [name, password, permissions, expires_at] properties: name: type: string @@ -2301,6 +2301,12 @@ components: type: array items: $ref: '#/components/schemas/Permission' + minItems: 1 + description: | + At least one permission is required. An unscoped key would + authenticate its owner but authorize nothing (issue #61), so the + API rejects an empty or missing list with 400 rather than + accepting a key that can never be used. expires_at: type: string format: date-time diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 5d3fc72c7..ff75609f7 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -69,6 +69,13 @@ var ( // credential, so a precise message is safe -- issue #929). ErrCurrentPasswordIncorrect = errors.New("current password is incorrect") + // ErrEmptyAPIKeyPermissions is returned by CreateAPIKey when the request + // carries zero permissions. An unscoped key used to silently inherit the + // owner's full permission set at request time; requiring at least one + // explicit permission at creation forces scoping to be deliberate rather + // than the most-permissive default. Mapped to 400 (issue #61). + ErrEmptyAPIKeyPermissions = errors.New("at least one permission is required") + // 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/service_apikeys.go b/internal/auth/service_apikeys.go index 06cc7022c..90bd00523 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -45,29 +45,40 @@ const ( MaxAPIKeyLifetime = 365 * 24 * time.Hour ) -// CreateAPIKey creates a new user API key with scoped permissions. The -// caller's current password must be supplied and is re-verified against the -// stored hash before minting a key: a stolen session token alone must not be -// enough to create a durable, potentially wildcard-scoped credential, the -// same defense-in-depth MFASetup/MFADisable already apply (issue #102). -// Returns the full API key (shown only once), key info, and error. -func (s *Service) CreateAPIKey(ctx context.Context, userID, name, password string, permissions []Permission, expiresAt *time.Time) (string, *UserAPIKey, error) { - // Validate user exists and is active +// loadActiveUserForAPIKey resolves userID and ensures the account exists and +// is active. Pulled out of CreateAPIKey to keep its cyclomatic complexity +// under the project's gocyclo gate as new checks (e.g. issue #61's empty- +// permissions rejection, issue #102's password/expiry validation) are added. +func (s *Service) loadActiveUserForAPIKey(ctx context.Context, userID string) (*User, error) { user, err := s.store.GetUserByID(ctx, userID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - return "", nil, fmt.Errorf("user not found") + return nil, fmt.Errorf("user not found") } - return "", nil, fmt.Errorf("failed to get user: %w", err) + return nil, fmt.Errorf("failed to get user: %w", err) } if user == nil { - return "", nil, fmt.Errorf("user not found") + return nil, fmt.Errorf("user not found") } if !user.Active { - return "", nil, fmt.Errorf("user account is not active") + return nil, fmt.Errorf("user account is not active") } + return user, nil +} - if err := s.validateCreateAPIKeyRequest(user, password, name, expiresAt); err != nil { +// CreateAPIKey creates a new user API key with scoped permissions. The +// caller's current password must be supplied and is re-verified against the +// stored hash before minting a key: a stolen session token alone must not be +// enough to create a durable, potentially wildcard-scoped credential, the +// same defense-in-depth MFASetup/MFADisable already apply (issue #102). +// Returns the full API key (shown only once), key info, and error. +func (s *Service) CreateAPIKey(ctx context.Context, userID, name, password string, permissions []Permission, expiresAt *time.Time) (string, *UserAPIKey, error) { + user, err := s.loadActiveUserForAPIKey(ctx, userID) + if err != nil { + return "", nil, err + } + + if err := s.validateCreateAPIKeyRequest(user, password, name, permissions, expiresAt); err != nil { return "", nil, err } @@ -125,16 +136,25 @@ func (s *Service) CreateAPIKey(ctx context.Context, userID, name, password strin } // validateCreateAPIKeyRequest checks the caller-supplied fields of a -// CreateAPIKey call: password re-verification, key name, and the mandatory, -// capped expiry (issue #102). Extracted out of CreateAPIKey to keep its -// cyclomatic complexity under the repo's gocyclo gate. -func (s *Service) validateCreateAPIKeyRequest(user *User, password, name string, expiresAt *time.Time) error { +// CreateAPIKey call: password re-verification, key name, the required +// permissions scope, and the mandatory, capped expiry (issues #61, #102). +// Extracted out of CreateAPIKey to keep its cyclomatic complexity under the +// repo's gocyclo gate. +func (s *Service) validateCreateAPIKeyRequest(user *User, password, name string, permissions []Permission, expiresAt *time.Time) error { if !s.verifyPassword(password, user.PasswordHash) { return fmt.Errorf("%w", ErrAPIKeyInvalidPassword) } if name == "" { return fmt.Errorf("API key name is required") } + // Reject unscoped keys outright: an unscoped key used to inherit the + // owner's full permission set (computeEffectivePermissionsFromAuthCtx), + // which made "leave scope blank" the most permissive option in the UI. + // Requiring at least one explicit permission forces scoping to be a + // deliberate choice (issue #61). + if len(permissions) == 0 { + return ErrEmptyAPIKeyPermissions + } // ExpiresAt is mandatory and capped: a nil value used to mean "never // expires", which let a single leaked key outlive any credential rotation. if expiresAt == nil { @@ -518,13 +538,17 @@ func (s *Service) RecordUsage(ctx context.Context, keyID string, delta int64) er // computeEffectivePermissionsFromAuthCtx returns the subset of key permissions // that the owner's authCtx also grants at the action/resource level, keeping -// the key's own constraint limits. If the key has no specific permissions the -// owner's full permission set is returned (key inherits owner). The result -// carries the key's constraints, not the owner's; callers that need both -// sources must check ownerAuthCtx.Permissions independently. +// the key's own constraint limits. If the key has no specific permissions it +// authorizes nothing: an unscoped key authenticates the owner but grants no +// permissions (deny-by-default, issue #61), rather than inheriting the +// owner's full permission set. That inheritance was a privilege-escalation +// trap -- a long-lived unscoped key silently became admin-capable the moment +// its owner joined the Administrators group. The result carries the key's +// constraints, not the owner's; callers that need both sources must check +// ownerAuthCtx.Permissions independently. func computeEffectivePermissionsFromAuthCtx(key *UserAPIKey, authCtx *AuthContext) []Permission { if len(key.Permissions) == 0 { - return authCtx.Permissions + return []Permission{} } effectivePerms := make([]Permission, 0, len(key.Permissions)) for _, keyPerm := range key.Permissions { @@ -538,11 +562,11 @@ func computeEffectivePermissionsFromAuthCtx(key *UserAPIKey, authCtx *AuthContex // ComputeEffectivePermissions computes the intersection of API key permissions and user permissions. // This ensures an API key cannot grant more permissions than the user has. // -// Administrators-group members carry {admin, *}: with no key-specific -// permissions their full {admin, *} context is returned, and a scoped admin -// key's permissions all pass the HasPermission intersection below, so the -// group-derived path preserves the previous role == admin behavior without a -// special case. +// An unscoped key (no key-specific permissions) grants nothing, regardless of +// the owner's group; see computeEffectivePermissionsFromAuthCtx. A scoped +// admin key's permissions all pass the HasPermission intersection below, so +// the group-derived path still lets a deliberately-admin-scoped key act with +// the owner's {admin, *} authority. func (s *Service) ComputeEffectivePermissions(ctx context.Context, apiKey *UserAPIKey, user *User) ([]Permission, error) { authCtx, err := s.GetAuthContext(ctx, user.ID) if err != nil { diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 8e9336f16..4ce66ed55 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -93,12 +93,18 @@ type APIKeyInfo struct { // has never recorded a request or because its last window has closed. // Exposed so consumers know exactly which period RequestCountWindow // covers instead of assuming a true trailing "last 24h". - RequestCountWindowStart *time.Time `json:"request_count_window_start,omitempty"` - ID string `json:"id"` - Name string `json:"name"` - KeyPrefix string `json:"key_prefix"` - Permissions []Permission `json:"permissions,omitempty"` - IsActive bool `json:"is_active"` + RequestCountWindowStart *time.Time `json:"request_count_window_start,omitempty"` + ID string `json:"id"` + Name string `json:"name"` + KeyPrefix string `json:"key_prefix"` + // Permissions is never omitempty: CreateAPIKey now rejects zero-length + // permissions outright (issue #61), so an empty array in a listing means + // a legacy key minted before that fix, distinguishable from a key that + // 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"` // 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, @@ -339,8 +345,11 @@ func (s *Service) ValidateUserAPIKeyAPI(ctx context.Context, apiKey string) (*Us // action/resource against the key's effective permissions: the intersection // of the key's scoped permissions with the owning user's group-derived // permissions (ComputeEffectivePermissions). A key created without explicit -// permissions inherits the owner's full permission set. Returns the owning -// user's ID, the key's database ID, and whether the permission is held. +// permissions grants nothing (deny-by-default, issue #61); CreateAPIKey now +// rejects new unscoped keys outright, but a legacy zero-scope key minted +// before that fix still authenticates its owner while authorizing no +// action. Returns the owning user's ID, the key's database ID, and whether +// the permission is held. // The key ID is threaded to callers so they can pass it to // HasAPIKeyPermissionForConstraintsAPI without a redundant DB lookup. // diff --git a/internal/auth/service_apikeys_api_test.go b/internal/auth/service_apikeys_api_test.go index c7dd2322a..a3c421e22 100644 --- a/internal/auth/service_apikeys_api_test.go +++ b/internal/auth/service_apikeys_api_test.go @@ -162,6 +162,10 @@ func TestService_CreateAPIKeyAPI_CrossPackageType(t *testing.T) { 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) diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index 51e4efcb2..ea211ece9 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -263,18 +263,76 @@ func TestService_CreateAPIKey(t *testing.T) { Email: "test@example.com", Active: true, PasswordHash: testAPIKeyPasswordHash, + GroupIDs: []string{DefaultAdminGroupID}, + } + permissions := []Permission{ + {Action: ActionView, Resource: ResourceRecommendations}, } 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(assert.AnError) - apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &validExpiry) + apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, permissions, &validExpiry) assert.Error(t, err) assert.Empty(t, apiKey) assert.Nil(t, keyInfo) mockStore.AssertExpectations(t) }) + + t.Run("fail when permissions is empty", func(t *testing.T) { + // Regression test for issue #61: an unscoped key used to be accepted + // and would silently inherit the owner's full permission set at + // request time. CreateAPIKey must now reject it outright so scoping + // is a deliberate choice, not a default. + mockStore := new(MockStore) + service := &Service{store: mockStore} + + user := &User{ + ID: "user-123", + Email: "test@example.com", + Active: true, + PasswordHash: testAPIKeyPasswordHash, + } + + mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) + + apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &validExpiry) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrEmptyAPIKeyPermissions) + assert.Empty(t, apiKey) + assert.Nil(t, keyInfo) + mockStore.AssertNotCalled(t, "CreateAPIKey", mock.Anything, mock.Anything) + mockStore.AssertExpectations(t) + }) + + t.Run("fail when permissions is nil", func(t *testing.T) { + mockStore := new(MockStore) + service := &Service{store: mockStore} + + user := &User{ + ID: "user-123", + Email: "test@example.com", + Active: true, + PasswordHash: testAPIKeyPasswordHash, + } + + mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) + + apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, nil, &validExpiry) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrEmptyAPIKeyPermissions) + assert.Empty(t, apiKey) + assert.Nil(t, keyInfo) + mockStore.AssertNotCalled(t, "CreateAPIKey", mock.Anything, mock.Anything) + mockStore.AssertExpectations(t) + }) } func TestService_ListUserAPIKeys(t *testing.T) { @@ -1006,7 +1064,10 @@ func TestService_ComputeEffectivePermissions(t *testing.T) { assert.Equal(t, scoped, permissions) }) - t.Run("admin with unscoped key returns full group permissions", func(t *testing.T) { + t.Run("admin with unscoped key returns no permissions (fail closed)", func(t *testing.T) { + // Regression test for issue #61: an unscoped key used to inherit the + // owner's full permission set, so an admin's unscoped key was + // silently admin-capable. It must now authorize nothing. mockStore := new(MockStore) service := &Service{store: mockStore} t.Cleanup(func() { mockStore.AssertExpectations(t) }) @@ -1019,7 +1080,7 @@ func TestService_ComputeEffectivePermissions(t *testing.T) { permissions, err := service.ComputeEffectivePermissions(ctx, apiKey, user) require.NoError(t, err) - assert.Equal(t, []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, permissions) + assert.Empty(t, permissions) }) t.Run("zero-group user with unscoped key returns no permissions (fail closed)", func(t *testing.T) { @@ -1166,13 +1227,13 @@ func TestService_HasAPIKeyPermissionAPI(t *testing.T) { assert.False(t, has, "key must not grant permissions the owning user does not hold") }) - t.Run("unscoped key inherits the owner's group permissions", func(t *testing.T) { + t.Run("regression #61: unscoped key is denied (fail closed), not inherited from the owner", func(t *testing.T) { service := setup(t, nil) userID, _, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionCreate, ResourcePlans) require.NoError(t, err) assert.Equal(t, "user-123", userID) - assert.True(t, has) + assert.False(t, has, "unscoped key must not inherit the owner's group permissions") }) t.Run("invalid key fails closed with an error", func(t *testing.T) { diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index a6c9913ea..cb7ead618 100644 --- a/internal/server/adapter_test.go +++ b/internal/server/adapter_test.go @@ -399,9 +399,10 @@ func TestAuthServiceAdapter_CreateAPIKeyAPI(t *testing.T) { expiresAt := time.Now().Add(24 * time.Hour) result, err := adapter.CreateAPIKeyAPI(ctx, "user-1", auth.APICreateAPIKeyRequest{ - Name: "my-key", - Password: password, - ExpiresAt: &expiresAt, + Name: "my-key", + Password: password, + Permissions: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, + ExpiresAt: &expiresAt, }) require.NoError(t, err) require.NotNil(t, result) diff --git a/internal/server/apikey_usage_booking_test.go b/internal/server/apikey_usage_booking_test.go index ade8281a6..c17c4fec6 100644 --- a/internal/server/apikey_usage_booking_test.go +++ b/internal/server/apikey_usage_booking_test.go @@ -61,8 +61,10 @@ type usageBookingHarness struct { } // newUsageBookingHarness builds the harness. groupPermissions are granted to -// the key owner's group; the API key itself carries no explicit permissions, -// so it inherits the owner's set. +// both the key owner's group and the key itself: an unscoped key now +// authorizes nothing (issue #61's fail-closed fix), so the harness scopes the +// key explicitly to the same permissions to exercise the booking path rather +// than the now-denied inheritance path. func newUsageBookingHarness(t *testing.T, groupPermissions []auth.Permission) *usageBookingHarness { t.Helper() @@ -74,11 +76,12 @@ func newUsageBookingHarness(t *testing.T, groupPermissions []auth.Permission) *u keyHash := usageBookingKeyHash() key := &auth.UserAPIKey{ - ID: "key-1", - UserID: "user-1", - Name: "usage booking harness", - KeyHash: keyHash, - IsActive: true, + ID: "key-1", + UserID: "user-1", + Name: "usage booking harness", + KeyHash: keyHash, + IsActive: true, + Permissions: groupPermissions, } user := &auth.User{ ID: "user-1", From a24a97769e9f6547f2a96f071d1b5857844a9c0d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 05:02:45 +0200 Subject: [PATCH 2/4] test(auth): end-to-end regression test for unscoped API key denial Adds a real request-path regression test for issue #61, driving a real auth.Service behind a real api.Handler through HandleRequest with an admin-owned, zero-permission API key hitting a protected endpoint. Confirms the request is now denied with 403 rather than succeeding. Verified this fails on the pre-fix service_apikeys.go (the request proceeds past requirePermission and panics on an unmocked store call inside the handler, proving it was not denied) and passes with the fix restored. Co-Authored-By: claude-flow --- .../server/apikey_unscoped_permission_test.go | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 internal/server/apikey_unscoped_permission_test.go diff --git a/internal/server/apikey_unscoped_permission_test.go b/internal/server/apikey_unscoped_permission_test.go new file mode 100644 index 000000000..e6bb0a140 --- /dev/null +++ b/internal/server/apikey_unscoped_permission_test.go @@ -0,0 +1,94 @@ +package server + +import ( + "context" + "crypto/sha256" + "encoding/base64" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/api" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// TestHandleRequest_UnscopedAPIKey_DeniedOnAdminOwner is the regression test +// for issue #61: an API key created with zero explicit permissions used to +// inherit its owner's full permission set at request time +// (computeEffectivePermissionsFromAuthCtx), so an admin's unscoped key was +// silently admin-capable on every endpoint. It must now be denied. +// +// This drives the real, wired-together stack -- a real auth.Service over a +// mock store, behind a real api.Handler -- through HandleRequest so the +// assertion covers the actual request path (API-key auth middleware -> +// requirePermission -> ComputeEffectivePermissions), not just the service +// method in isolation. See apikey_usage_booking_test.go for why package +// server is required for this: api's mock auth service stubs the permission +// check itself, which would make the regression structurally invisible. +func TestHandleRequest_UnscopedAPIKey_DeniedOnAdminOwner(t *testing.T) { + rawKey := "unscoped-key-owned-by-admin" // #nosec G101 -- test fixture, not a real credential + hash := sha256.Sum256([]byte(rawKey)) + keyHash := base64.RawURLEncoding.EncodeToString(hash[:]) + + key := &auth.UserAPIKey{ + ID: "key-unscoped-1", + UserID: "admin-1", + Name: "unscoped key", + KeyHash: keyHash, + IsActive: true, + // No Permissions set: this is the exact "leave scope blank" shape + // the issue describes. CreateAPIKey now rejects minting one, but a + // key minted before that fix (or inserted directly, e.g. by a + // migration) can still exist and authenticate. + } + user := &auth.User{ + ID: "admin-1", + Email: "admin@example.com", + Active: true, + GroupIDs: []string{auth.DefaultAdminGroupID}, + } + adminGroup := &auth.Group{ + ID: auth.DefaultAdminGroupID, + Name: "Administrators", + Permissions: []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, + } + + mockStore := new(auth.MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + mockStore.On("GetAPIKeyByHash", mock.Anything, keyHash).Return(key, nil) + mockStore.On("GetUserByID", mock.Anything, "admin-1").Return(user, nil) + mockStore.On("GetGroup", mock.Anything, auth.DefaultAdminGroupID).Return(adminGroup, nil) + mockStore.On("RecordAPIKeyUsage", mock.Anything, "key-unscoped-1", mock.Anything).Return(nil).Maybe() + + service := auth.NewService(auth.ServiceConfig{ + Store: mockStore, + SessionDuration: time.Hour, + CSRFKey: auth.TestCSRFKey(), + }) + handler := api.NewHandler(api.HandlerConfig{ + AuthService: newAuthServiceAdapter(service), + CORSAllowedOrigin: "https://dashboard.example.com", + }) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": rawKey}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "GET", + Path: "/api/api-keys", + }, + }, + } + + resp, err := handler.HandleRequest(context.Background(), req) + + require.NoError(t, err) + // Pre-fix this returned 200: the key's owner is an Administrators-group + // member, and an unscoped key inherited {admin, *} wholesale even though + // the key itself was never granted view:api-keys. + require.Equal(t, 403, resp.StatusCode, + "an unscoped API key must not inherit its admin owner's permissions; body: %s", resp.Body) +} From ba273d1ff67286d33a3827f6dc46ce103e0da975 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 10:41:51 +0200 Subject: [PATCH 3/4] fix(frontend): add permission picker to create-key modal, fix 400 regression Summary: CreateAPIKey now rejects an empty permissions array (PR #375's fix for issue #61), but frontend/src/apikeys.ts always called createApiKey(name, undefined, expiresAt) and the create-key modal had no way to select permissions. Every dashboard-created API key would now 400. Fix: - frontend/src/index.html: add a permissions picker container to the create-key modal form, and give the submit button an id so it can be disabled/enabled. - frontend/src/apikeys.ts: showCreateKeyModal fetches the caller's own effective permissions from GET /api/auth/me/permissions on every open and renders one checkbox per entry the owner actually holds (never a wider catalog). The Create button stays disabled until at least one is checked; handleCreateApiKey also re-validates a non-empty selection before calling the API and sends it as `permissions` in the request body. All interpolated values are escaped before the innerHTML write. - internal/auth/service_apikeys_api.go: normalizeAPIKeyPermissions returns []Permission{} instead of nil so a legacy key minted before CreateAPIKey started rejecting empty permissions serializes as "permissions":[] rather than "permissions":null in the create/list API responses (APIKeyInfo.Permissions is no longer omitempty as of the earlier commit in this PR). Regression tests: - frontend/src/__tests__/apikeys.test.ts: fetches and renders the owner's permissions as unchecked checkboxes; labels the admin wildcard entry; escapes a hostile action/resource value (asserts no element is created and the checkbox's dataset round-trips the exact hostile string); shows a message when the owner holds none; shows an inline error when the fetch fails; Create button enables/disables on checkbox change; handleCreateApiKey sends the checked permissions in the request body (asserts a non-empty `permissions` array); rejects submission with an error and never calls the API when nothing is checked; sends every checked permission when more than one is selected. - internal/auth/service_apikeys_api_test.go: a legacy key with Permissions: nil normalizes to an empty (non-nil) slice through ListUserAPIKeysAPI. Verification: - go build ./..., go vet ./... -- clean (GOTOOLCHAIN=go1.26.6) - go test -race -short ./internal/auth/... -- pass (803 subtests) - gocyclo -over 10 -ignore "_test\.go" . -- empty - go mod tidy -diff -- empty - npx tsc --noEmit -- clean - npm test -- 91 suites, 2974 passed, 1 pre-existing skip - npm run lint -- 0 errors (pre-existing warnings only, none in touched files) - npm run build -- succeeds (pre-existing bundle-size warning only) --- frontend/src/__tests__/apikeys.test.ts | 218 +++++++++++++++++++++- frontend/src/apikeys.ts | 131 +++++++++++-- frontend/src/index.html | 7 +- internal/auth/service_apikeys_api.go | 19 +- internal/auth/service_apikeys_api_test.go | 28 +++ 5 files changed, 379 insertions(+), 24 deletions(-) diff --git a/frontend/src/__tests__/apikeys.test.ts b/frontend/src/__tests__/apikeys.test.ts index a349278c1..771768ec2 100644 --- a/frontend/src/__tests__/apikeys.test.ts +++ b/frontend/src/__tests__/apikeys.test.ts @@ -21,6 +21,7 @@ jest.mock('../api', () => ({ createApiKey: jest.fn(), revokeApiKey: jest.fn(), deleteApiKey: jest.fn(), + getUserPermissions: jest.fn(), // Real implementation (not mocked): createApiKey encodes the password // field with this before building the request, so tests need the real // encoding to assert on the resulting request shape. @@ -48,9 +49,16 @@ describe('API Keys Module', () => {
+
+ +
- +
@@ -72,6 +80,13 @@ describe('API Keys Module', () => { total_requests_lifetime: 0, top_keys: [], }); + // Default the owner's effective-permissions fetch (issue #61's + // frontend follow-up) so showCreateKeyModal tests that don't care + // about the picker's own behavior aren't forced to stub it too. + (api.getUserPermissions as jest.Mock).mockResolvedValue({ + permissions: [{ action: 'view', resource: 'recommendations' }], + is_admin: false, + }); }); describe('loadApiKeys', () => { @@ -404,6 +419,108 @@ describe('API Keys Module', () => { // Should not throw expect(() => showCreateKeyModal()).not.toThrow(); }); + + // Issue #61 frontend follow-up: the create-key modal now fetches the + // caller's own effective permissions and renders one checkbox per + // entry, since the backend rejects an empty permissions array. + describe('permission picker (issue #61)', () => { + test('fetches and renders the owner\'s effective permissions as unchecked checkboxes', async () => { + (api.getUserPermissions as jest.Mock).mockResolvedValue({ + permissions: [ + { action: 'view', resource: 'recommendations' }, + { action: 'create', resource: 'plans' }, + ], + is_admin: false, + }); + + await showCreateKeyModal(); + + expect(api.getUserPermissions).toHaveBeenCalled(); + const container = document.getElementById('apikey-permissions-list'); + const checkboxes = container?.querySelectorAll('.apikey-permission-checkbox'); + expect(checkboxes?.length).toBe(2); + checkboxes?.forEach(cb => expect(cb.checked).toBe(false)); + expect(container?.textContent).toContain('view:recommendations'); + expect(container?.textContent).toContain('create:plans'); + }); + + test('labels the admin wildcard entry in plain language', async () => { + (api.getUserPermissions as jest.Mock).mockResolvedValue({ + permissions: [{ action: 'admin', resource: '*' }], + is_admin: true, + }); + + await showCreateKeyModal(); + + const container = document.getElementById('apikey-permissions-list'); + expect(container?.textContent).toContain('Admin (all actions, all resources)'); + }); + + test('escapes hostile action/resource values instead of injecting markup', async () => { + const hostile = '">'; + (api.getUserPermissions as jest.Mock).mockResolvedValue({ + permissions: [{ action: hostile, resource: 'plans' }], + is_admin: false, + }); + + await showCreateKeyModal(); + + const container = document.getElementById('apikey-permissions-list'); + // No element was injected -- the quote-breakout attempt stayed + // inside the data-action attribute's value rather than closing it + // and opening a real tag. + expect(container?.querySelector('img')).toBeNull(); + // The checkbox's dataset round-trips the exact hostile string, + // proving escapeHtml neutralized it as data rather than mangling + // or truncating it. + const checkbox = container?.querySelector('.apikey-permission-checkbox'); + expect(checkbox?.dataset['action']).toBe(hostile); + // The visible label text is HTML-escaped, not raw markup. + expect(container?.textContent).toContain(`${hostile}:plans`); + }); + + test('shows a message and no checkboxes when the owner holds no permissions', async () => { + (api.getUserPermissions as jest.Mock).mockResolvedValue({ permissions: [], is_admin: false }); + + await showCreateKeyModal(); + + const container = document.getElementById('apikey-permissions-list'); + expect(container?.querySelectorAll('.apikey-permission-checkbox').length).toBe(0); + expect(container?.textContent).toContain('No permissions available'); + }); + + test('shows an inline error when the permissions fetch fails', async () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); + (api.getUserPermissions as jest.Mock).mockRejectedValue(new Error('boom')); + + await showCreateKeyModal(); + + const container = document.getElementById('apikey-permissions-list'); + expect(container?.querySelector('.error')?.textContent).toContain('Failed to load your permissions'); + consoleError.mockRestore(); + }); + + test('Create button starts disabled and enables only once a permission is checked', async () => { + (api.getUserPermissions as jest.Mock).mockResolvedValue({ + permissions: [{ action: 'view', resource: 'recommendations' }], + is_admin: false, + }); + + await showCreateKeyModal(); + + const submitBtn = document.getElementById('create-apikey-submit-btn') as HTMLButtonElement; + expect(submitBtn.disabled).toBe(true); + + const checkbox = document.querySelector('.apikey-permission-checkbox') as HTMLInputElement; + checkbox.checked = true; + checkbox.dispatchEvent(new Event('change')); + expect(submitBtn.disabled).toBe(false); + + checkbox.checked = false; + checkbox.dispatchEvent(new Event('change')); + expect(submitBtn.disabled).toBe(true); + }); + }); }); describe('closeCreateKeyModal', () => { @@ -426,8 +543,12 @@ describe('API Keys Module', () => { describe('createApiKey', () => { const expiresAt = new Date('2025-12-31T00:00:00Z'); + // permissions is required (issue #61): the backend rejects an empty + // array with 400, so every real caller (handleCreateApiKey) always has + // a non-empty selection by the time it calls this wrapper. + const permissions = [{ action: 'view', resource: 'recommendations' }]; - test('creates API key with name, password and expiration', async () => { + test('creates API key with name, password, expiration and permissions', async () => { const mockResponse = { api_key: 'full-api-key-value', key_id: 'key-1', @@ -436,17 +557,18 @@ describe('API Keys Module', () => { (api.createApiKey as jest.Mock).mockResolvedValue(mockResponse); - const result = await createApiKey('Test Key', 'my-password', expiresAt); + const result = await createApiKey('Test Key', 'my-password', expiresAt, permissions); expect(api.createApiKey).toHaveBeenCalledWith({ name: 'Test Key', password: Buffer.from('my-password', 'binary').toString('base64'), - expires_at: expiresAt.toISOString() + expires_at: expiresAt.toISOString(), + permissions, }); expect(result.api_key).toBe('full-api-key-value'); }); - test('creates API key with permissions', async () => { + test('sends every permission passed in', async () => { const mockResponse = { api_key: 'full-api-key-value', key_id: 'key-1', @@ -455,14 +577,17 @@ describe('API Keys Module', () => { (api.createApiKey as jest.Mock).mockResolvedValue(mockResponse); - const permissions = [{ action: 'read', resource: '*' }]; - await createApiKey('Test Key', 'my-password', expiresAt, permissions); + const multiplePermissions = [ + { action: 'view', resource: 'recommendations' }, + { action: 'create', resource: 'plans' }, + ]; + await createApiKey('Test Key', 'my-password', expiresAt, multiplePermissions); expect(api.createApiKey).toHaveBeenCalledWith({ name: 'Test Key', password: Buffer.from('my-password', 'binary').toString('base64'), expires_at: expiresAt.toISOString(), - permissions: permissions + permissions: multiplePermissions }); }); @@ -470,7 +595,7 @@ describe('API Keys Module', () => { const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); (api.createApiKey as jest.Mock).mockRejectedValue(new Error('Create failed')); - await expect(createApiKey('Test Key', 'my-password', expiresAt)).rejects.toThrow('Create failed'); + await expect(createApiKey('Test Key', 'my-password', expiresAt, permissions)).rejects.toThrow('Create failed'); expect(consoleError).toHaveBeenCalled(); consoleError.mockRestore(); }); @@ -592,6 +717,81 @@ describe('API Keys Module', () => { const errorEl = document.getElementById('create-apikey-error'); expect(errorEl?.textContent).toContain('Failed to create API key'); }); + + // Issue #61: the backend now 400s on an empty/missing permissions + // array, so the request body sent from the form must always carry the + // checked selection. + test('sends the checked permissions in the request body', async () => { + const futureDate = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000); + (api.createApiKey as jest.Mock).mockResolvedValue({ + api_key: 'test-key', + key_id: 'key-1', + key: { id: 'key-1', name: 'Test', key_prefix: 'abc' } + }); + (api.getApiKeys as jest.Mock).mockResolvedValue({ api_keys: [] }); + + (document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key'; + (document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password'; + (document.getElementById('apikey-expires-at') as HTMLInputElement).value = futureDate.toISOString().split('T')[0] || ""; + // The default fixture ships one pre-checked view:recommendations box. + + const event = { preventDefault: jest.fn() } as unknown as Event; + await handleCreateApiKey(event); + + expect(api.createApiKey).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'Test Key', + permissions: [{ action: 'view', resource: 'recommendations' }], + }) + ); + const [request] = (api.createApiKey as jest.Mock).mock.calls[0] as [{ permissions: unknown[] }]; + expect(request.permissions.length).toBeGreaterThan(0); + }); + + test('rejects submission with an error, and never calls the API, when no permission is checked', async () => { + const futureDate = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000); + (document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key'; + (document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password'; + (document.getElementById('apikey-expires-at') as HTMLInputElement).value = futureDate.toISOString().split('T')[0] || ""; + const checkbox = document.querySelector('.apikey-permission-checkbox') as HTMLInputElement; + checkbox.checked = false; + + const event = { preventDefault: jest.fn() } as unknown as Event; + await handleCreateApiKey(event); + + const errorEl = document.getElementById('create-apikey-error'); + expect(errorEl?.textContent).toBe('Select at least one permission'); + expect(api.createApiKey).not.toHaveBeenCalled(); + }); + + test('sends every checked permission when more than one is selected', async () => { + const futureDate = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000); + document.getElementById('apikey-permissions-list')!.innerHTML = ` + + + `; + (api.createApiKey as jest.Mock).mockResolvedValue({ + api_key: 'test-key', + key_id: 'key-1', + key: { id: 'key-1', name: 'Test', key_prefix: 'abc' } + }); + (api.getApiKeys as jest.Mock).mockResolvedValue({ api_keys: [] }); + (document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key'; + (document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password'; + (document.getElementById('apikey-expires-at') as HTMLInputElement).value = futureDate.toISOString().split('T')[0] || ""; + + const event = { preventDefault: jest.fn() } as unknown as Event; + await handleCreateApiKey(event); + + expect(api.createApiKey).toHaveBeenCalledWith( + expect.objectContaining({ + permissions: [ + { action: 'view', resource: 'recommendations' }, + { action: 'create', resource: 'plans' }, + ], + }) + ); + }); }); describe('showKeyCreatedModal', () => { diff --git a/frontend/src/apikeys.ts b/frontend/src/apikeys.ts index 9b6b7683d..d8891c7ff 100644 --- a/frontend/src/apikeys.ts +++ b/frontend/src/apikeys.ts @@ -167,17 +167,100 @@ export function renderApiKeysList(): void { } /** - * Show create API key modal + * Format an (action, resource) permission entry for display in the + * create-key picker. No catalog of human labels is maintained here -- + * the values are the closed vocabulary the backend already validates + * (internal/auth/types.go), so the raw "action:resource" pair is + * unambiguous and never goes stale against a separate label list. */ -export function showCreateKeyModal(): void { +function formatPermissionLabel(entry: api.PermissionEntry): string { + if (entry.action === 'admin' && entry.resource === '*') { + return 'Admin (all actions, all resources)'; + } + return `${entry.action}:${entry.resource}`; +} + +/** + * Render the create-key permission picker as one checkbox per entry the + * calling user actually holds (issue #61's frontend follow-up): after the + * backend fix, CreateAPIKey rejects an empty permissions array, so a + * picker limited to the owner's own effective permissions is both the + * fix for the resulting "every key creation 400s" regression and the + * least-privilege UI the per-key scoping feature always intended. + * + * Every interpolated value is escaped even though action/resource come + * from the closed backend vocabulary -- defense in depth for an innerHTML + * write. + */ +function renderApiKeyPermissionPicker(entries: api.PermissionEntry[]): void { + const container = document.getElementById('apikey-permissions-list'); + if (!container) return; + + if (entries.length === 0) { + container.innerHTML = '

No permissions available on your account -- an API key cannot be created.

'; + return; + } + + container.innerHTML = entries.map((entry, i) => ` + + `).join(''); + + container.querySelectorAll('.apikey-permission-checkbox').forEach(cb => { + cb.addEventListener('change', updateCreateKeySubmitState); + }); +} + +/** + * Read the permissions currently checked in the picker. + */ +function collectSelectedApiKeyPermissions(): api.Permission[] { + const container = document.getElementById('apikey-permissions-list'); + if (!container) return []; + + const permissions: api.Permission[] = []; + container.querySelectorAll('.apikey-permission-checkbox:checked').forEach(cb => { + const action = cb.dataset['action']; + const resource = cb.dataset['resource']; + if (action && resource) permissions.push({ action, resource }); + }); + return permissions; +} + +/** + * Enable the Create button only while at least one permission is + * selected. An unscoped key is now rejected server-side (400), and + * disabling here catches it before the round-trip rather than after. + */ +function updateCreateKeySubmitState(): void { + const submitBtn = document.getElementById('create-apikey-submit-btn') as HTMLButtonElement | null; + if (!submitBtn) return; + submitBtn.disabled = collectSelectedApiKeyPermissions().length === 0; +} + +/** + * Show create API key modal. + * + * Fetches the caller's own effective permissions fresh on every open + * (rather than reusing a cached snapshot from login) so the picker + * reflects the account's CURRENT grants -- a group change since login + * shows up immediately instead of offering a permission the key would + * then fail to obtain from the intersection check server-side. + */ +export async function showCreateKeyModal(): Promise { const modal = document.getElementById('create-apikey-modal'); const form = document.getElementById('create-apikey-form') as HTMLFormElement; const errorEl = document.getElementById('create-apikey-error'); + const submitBtn = document.getElementById('create-apikey-submit-btn') as HTMLButtonElement | null; if (!modal || !form) return; form.reset(); if (errorEl) errorEl.classList.add('hidden'); + if (submitBtn) submitBtn.disabled = true; // Expiration is required (issue #102: no more "never expires"). Prefill // 90 days out so the field never starts empty; the actual cap is @@ -189,7 +272,23 @@ export function showCreateKeyModal(): void { expiresAtInput.value = toLocalDateInputValue(defaultDate); } + const permissionsContainer = document.getElementById('apikey-permissions-list'); + if (permissionsContainer) permissionsContainer.innerHTML = '

Loading your permissions…

'; + + // Open the modal before the permissions fetch resolves so the dialog + // never sits closed while waiting on the network. openModal(modal); + + try { + const { permissions } = await api.getUserPermissions(); + renderApiKeyPermissionPicker(permissions); + } catch (error) { + console.error('Failed to load permissions for API key creation:', error); + if (permissionsContainer) { + permissionsContainer.innerHTML = '

Failed to load your permissions. Close and reopen this dialog to retry.

'; + } + } + updateCreateKeySubmitState(); } /** @@ -224,22 +323,20 @@ export function closeCreateKeyModal(): void { } /** - * Create new API key. password and expiresAt are required server-side - * (issue #102): creation re-verifies the caller's password, and a key can - * no longer be minted with no expiration. + * Create new API key. password, expiresAt and permissions are all required + * server-side: creation re-verifies the caller's password (issue #102), a + * key can no longer be minted with no expiration (issue #102), and an + * empty permissions array is rejected outright (issue #61). */ -export async function createApiKey(name: string, password: string, expiresAt: Date, permissions?: api.Permission[]): Promise { +export async function createApiKey(name: string, password: string, expiresAt: Date, permissions: api.Permission[]): Promise { try { const request: api.CreateAPIKeyRequest = { name, password: api.base64Encode(password), expires_at: expiresAt.toISOString(), + permissions, }; - if (permissions && permissions.length > 0) { - request.permissions = permissions; - } - const response = await api.createApiKey(request); return response; } catch (error) { @@ -274,6 +371,16 @@ export async function handleCreateApiKey(e: Event): Promise { return; } + // Belt-and-suspenders alongside the disabled submit button: the backend + // now rejects an empty permissions array with 400 (issue #61), so an + // unscoped key can never be minted. Catching it here avoids the round + // trip and gives a clearer message than the raw 400 body. + const permissions = collectSelectedApiKeyPermissions(); + if (permissions.length === 0) { + showError('Select at least one permission'); + return; + } + const expiresAt = parseLocalDateInputValue(expiresAtInput); if (expiresAt <= new Date()) { showError('Expiration date must be in the future'); @@ -281,7 +388,7 @@ export async function handleCreateApiKey(e: Event): Promise { } try { - const response = await createApiKey(name, password, expiresAt); + const response = await createApiKey(name, password, expiresAt, permissions); closeCreateKeyModal(); showKeyCreatedModal(response.api_key); await loadApiKeys(); @@ -413,7 +520,7 @@ export function initApiKeys(): void { // Setup create key button const createKeyBtn = document.getElementById('create-apikey-btn'); if (createKeyBtn) { - createKeyBtn.addEventListener('click', () => showCreateKeyModal()); + createKeyBtn.addEventListener('click', () => void showCreateKeyModal()); } // Setup close modal button diff --git a/frontend/src/index.html b/frontend/src/index.html index 899f57f89..bef9561e5 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -1025,6 +1025,11 @@

Create API Key

Confirm your password to create this API key +
+ +
+ Only the permissions your own account holds are offered. At least one is required -- an unscoped key would authenticate you but authorize nothing. +
@@ -1033,7 +1038,7 @@

Create API Key

- +
diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 4ce66ed55..1d3ae91fd 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -72,6 +72,21 @@ 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), +// 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{} + } + return perms +} + // API wrapper methods for API key operations // These methods return API-friendly types and handle type conversions @@ -213,7 +228,7 @@ func (s *Service) CreateAPIKeyAPI(ctx context.Context, userID string, req any) ( ID: keyInfo.ID, Name: keyInfo.Name, KeyPrefix: keyInfo.KeyPrefix, - Permissions: keyInfo.Permissions, + Permissions: normalizeAPIKeyPermissions(keyInfo.Permissions), ExpiresAt: keyInfo.ExpiresAt, CreatedAt: keyInfo.CreatedAt, LastUsedAt: keyInfo.LastUsedAt, @@ -243,7 +258,7 @@ func (s *Service) ListUserAPIKeysAPI(ctx context.Context, userID string) (any, e ID: key.ID, Name: key.Name, KeyPrefix: key.KeyPrefix, - Permissions: key.Permissions, + Permissions: normalizeAPIKeyPermissions(key.Permissions), ExpiresAt: key.ExpiresAt, CreatedAt: key.CreatedAt, LastUsedAt: key.LastUsedAt, diff --git a/internal/auth/service_apikeys_api_test.go b/internal/auth/service_apikeys_api_test.go index a3c421e22..7c5af2cb1 100644 --- a/internal/auth/service_apikeys_api_test.go +++ b/internal/auth/service_apikeys_api_test.go @@ -224,6 +224,34 @@ func TestService_ListUserAPIKeysAPI(t *testing.T) { mockStore.AssertExpectations(t) }) + // Regression for issue #61's follow-up: a key minted before CreateAPIKey + // started rejecting zero-length permissions can still have a NULL + // permissions column, which the store surfaces as a nil slice. + // APIKeyInfo.Permissions is no longer omitempty, so a raw nil would + // serialize as "permissions":null; normalizeAPIKeyPermissions makes it + // "permissions":[] like every post-fix key instead. + t.Run("normalizes a legacy nil-permissions key to an empty slice, not null", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + service := &Service{store: mockStore} + + keys := []*UserAPIKey{ + {ID: "legacy-key", UserID: "user-123", Name: "pre-fix key", Permissions: nil}, + } + user := &User{ID: "user-123", Email: "test@example.com", Active: true} + mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) + mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return(keys, nil) + + result, err := service.ListUserAPIKeysAPI(ctx, "user-123") + require.NoError(t, err) + + resp, ok := result.(*APIListAPIKeysResponse) + require.True(t, ok) + require.Len(t, resp.APIKeys, 1) + assert.NotNil(t, resp.APIKeys[0].Permissions, "a legacy nil-permissions key must serialize as [] not null") + assert.Empty(t, resp.APIKeys[0].Permissions) + }) + // Regression: the per-row "Requests (window)" cell must not show a count // left over from a window that has already closed. The stored column // keeps its value until the key's next request, so the read path is what From f87a069b0be6e5a7a5943d55d1084231ec909a5f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 16:34:48 +0200 Subject: [PATCH 4/4] fix(auth): give the expiresAt validation tests a non-empty permissions set Root cause: after rebasing PR #375 onto main (which merged #392's password/expiry validation for CreateAPIKey), validateCreateAPIKeyRequest now checks permissions before expiresAt. The three expiresAt-focused tests (nil, too-far, in-the-past) still passed []Permission{}, so they now hit ErrEmptyAPIKeyPermissions before ever reaching the expiresAt check they exist to exercise. Give them a real permission so they reach the assertion they're actually testing. --- internal/auth/service_apikeys_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index ea211ece9..097b875ac 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -162,7 +162,8 @@ func TestService_CreateAPIKey(t *testing.T) { mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) - apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, nil) + apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, + []Permission{{Action: ActionView, Resource: ResourceRecommendations}}, nil) assert.Error(t, err) assert.ErrorIs(t, err, ErrAPIKeyExpiresAtRequired) @@ -185,7 +186,8 @@ func TestService_CreateAPIKey(t *testing.T) { mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) - apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &tooFar) + apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, + []Permission{{Action: ActionView, Resource: ResourceRecommendations}}, &tooFar) assert.Error(t, err) assert.ErrorIs(t, err, ErrAPIKeyExpiresAtTooFar) @@ -211,7 +213,8 @@ func TestService_CreateAPIKey(t *testing.T) { mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) - apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &pastExpiry) + apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, + []Permission{{Action: ActionView, Resource: ResourceRecommendations}}, &pastExpiry) assert.Error(t, err) assert.ErrorIs(t, err, ErrAPIKeyExpiresAtInPast)