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.querySelectorAllLoading 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[]): PromiseCreate 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 @@
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..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
@@ -93,12 +108,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,
@@ -207,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,
@@ -237,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,
@@ -339,8 +360,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..7c5af2cb1 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)
@@ -220,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
diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go
index 51e4efcb2..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)
@@ -263,18 +266,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 +1067,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 +1083,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 +1230,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_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)
+}
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",