From fda1209d0ebaa5dedb53814dda8680143cbc0c24 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 22:55:12 -0700 Subject: [PATCH] fix(api): enforce user API key scoped permissions in requirePermission User API keys arrive in x-api-key with no bearer token, so every requirePermission-gated endpoint returned 401 for them, and the per-key scoped permissions persisted at key-creation time were never enforced: ComputeEffectivePermissions had no caller on the request path (SEC-02). Wire a real user-API-key authorization path into requirePermission: - Add auth.Service.HasAPIKeyPermissionAPI, which validates the key and checks the requested action/resource against the key's effective permissions (intersection of key scope and the owner's group-derived permissions, via ComputeEffectivePermissions). Fail closed on any validation or lookup error. - Expose it on AuthServiceInterface and the server adapter. - In requirePermission, authorize a non-admin x-api-key against the key's effective permissions: in-scope returns a session for the owning user, out-of-scope returns 403, and a key that fails validation falls through to bearer-token auth (matching resolveAuthenticatedUserID). Permission matching reuses AuthContext.HasPermission, so the admin:* wildcard money-spending carve-outs (issue #923) apply to API keys too. Regression tests replicate the real failing scenario (x-api-key header, no bearer token) at both layers and were confirmed to fail against the pre-fix code: a scoped key cannot exceed its scope, an in-scope key is authorized as its owner, and GET recommendations works end to end with a view:recommendations-scoped key. Closes #1142 --- internal/api/handler.go | 30 +++++- internal/api/handler_apikeys_test.go | 132 +++++++++++++++++++++++ internal/api/handler_ri_exchange_test.go | 3 + internal/api/mocks_test.go | 5 + internal/api/types.go | 9 ++ internal/auth/service_apikeys_api.go | 28 +++++ internal/auth/service_apikeys_test.go | 99 +++++++++++++++++ internal/server/app.go | 5 + 8 files changed, 306 insertions(+), 5 deletions(-) diff --git a/internal/api/handler.go b/internal/api/handler.go index 88f733a6c..41e673366 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -207,11 +207,15 @@ const apiKeyAdminUserID = "admin-api-key" // requirePermission validates authentication and checks if the user holds the // specified permission. The stateless admin API key bypasses the per-user -// permission lookup (it is a full-access infrastructure credential). Every -// other caller is checked against their group-derived permissions: a member of -// the Administrators group holds {admin, *} and passes any check, while a user -// with no groups holds no permissions and is denied (fail closed). Returns the -// session on success so callers can read session.UserID for account filtering. +// permission lookup (it is a full-access infrastructure credential). A user +// API key is checked against its effective permissions: the intersection of +// the key's scoped permissions with the owning user's group-derived +// permissions (issue #1142 - previously user API keys 401'd here and per-key +// scoping was never enforced). Every other caller is checked against their +// group-derived permissions: a member of the Administrators group holds +// {admin, *} and passes any check, while a user with no groups holds no +// permissions and is denied (fail closed). Returns the session on success so +// callers can read session.UserID for account filtering. func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunctionURLRequest, action, resource string) (*Session, error) { apiKey := extractAPIKey(req) if h.checkAdminAPIKey(apiKey) { @@ -222,6 +226,22 @@ func (h *Handler) requirePermission(ctx context.Context, req *events.LambdaFunct return nil, fmt.Errorf("authentication service not configured") } + // User API key: authorize against the key's effective permissions. A + // valid key that lacks the permission is denied (fail closed). A key + // that fails validation falls through to bearer-token auth, matching + // resolveAuthenticatedUserID, so a stale x-api-key header cannot lock + // out a caller that also presents a valid session. + if apiKey != "" { + userID, has, err := h.auth.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) + if err == nil { + if !has { + return nil, NewClientError(403, fmt.Sprintf("permission denied: requires %s on %s", action, resource)) + } + return &Session{UserID: userID}, nil + } + logging.Debugf("User API key permission check failed: %v", err) + } + token := h.extractBearerToken(req) if token == "" { return nil, NewClientError(401, "no authorization token provided") diff --git a/internal/api/handler_apikeys_test.go b/internal/api/handler_apikeys_test.go index 766821503..2bcb893ca 100644 --- a/internal/api/handler_apikeys_test.go +++ b/internal/api/handler_apikeys_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -649,3 +650,134 @@ func TestFormatTimePtr(t *testing.T) { assert.Contains(t, result, "2024-06-15T10:30:00") }) } + +// TestRequirePermission_UserAPIKey is the regression test for issue #1142: +// user API keys previously hit a hard 401 in requirePermission (only the +// admin API key and bearer-token sessions were recognized), so the per-key +// scoped permissions persisted at key-creation time were never enforced. +// requirePermission now authorizes user API keys against their effective +// permissions via HasAPIKeyPermissionAPI. +func TestRequirePermission_UserAPIKey(t *testing.T) { + ctx := context.Background() + + // The exact failing request shape: x-api-key header carrying a user API + // key, no bearer token (Authorization / X-Authorization absent). + newReq := func() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "cudly-user-key"}, + } + } + + t.Run("regression #1142: key with in-scope permission is authorized as its owner", func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations"). + Return("user-123", true, nil) + + handler := &Handler{auth: mockAuth} + session, err := handler.requirePermission(ctx, newReq(), "view", "recommendations") + require.NoError(t, err) + require.NotNil(t, session) + assert.Equal(t, "user-123", session.UserID) + }) + + t.Run("regression #1142: scoped key is denied an out-of-scope permission with 403", func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "execute", "purchases"). + Return("user-123", false, nil) + + handler := &Handler{auth: mockAuth} + session, err := handler.requirePermission(ctx, newReq(), "execute", "purchases") + require.Error(t, err) + assert.Nil(t, session) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "permission denied") + }) + + t.Run("invalid key with no bearer token is rejected with 401", func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations"). + Return("", false, errors.New("invalid API key")) + + handler := &Handler{auth: mockAuth} + session, err := handler.requirePermission(ctx, newReq(), "view", "recommendations") + require.Error(t, err) + assert.Nil(t, session) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 401, ce.code) + }) + + t.Run("invalid key falls through to a valid bearer session", func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "stale-key", "view", "recommendations"). + Return("", false, errors.New("invalid API key")) + mockAuth.On("ValidateSession", ctx, "session-token"). + Return(&Session{UserID: "user-456"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "user-456", "view", "recommendations"). + Return(true, nil) + + handler := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "x-api-key": "stale-key", + "Authorization": "Bearer session-token", + }, + } + session, err := handler.requirePermission(ctx, req, "view", "recommendations") + require.NoError(t, err) + require.NotNil(t, session) + assert.Equal(t, "user-456", session.UserID) + }) + + t.Run("admin API key still bypasses the per-key permission lookup", func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + handler := &Handler{auth: mockAuth, apiKey: "admin-infra-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "admin-infra-key"}, + } + session, err := handler.requirePermission(ctx, req, "view", "recommendations") + require.NoError(t, err) + require.NotNil(t, session) + assert.Equal(t, apiKeyAdminUserID, session.UserID) + mockAuth.AssertNotCalled(t, "HasAPIKeyPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) +} + +// TestGetRecommendations_UserAPIKey exercises the real failing scenario from +// issue #1142 end to end at the handler layer: GET /api/recommendations with +// only an x-api-key user key returned 401 before the fix even when the key +// was scoped to view:recommendations. +func TestGetRecommendations_UserAPIKey(t *testing.T) { + ctx := context.Background() + + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations"). + Return("user-123", true, nil) + // Account scoping resolves against the key's owning user. + mockAuth.On("GetAllowedAccountsAPI", ctx, "user-123"). + Return([]string(nil), nil) + + mockScheduler := new(MockScheduler) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + mockScheduler.On("ListRecommendations", ctx, mock.Anything). + Return([]config.RecommendationRecord{}, nil) + + handler := &Handler{auth: mockAuth, scheduler: mockScheduler} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "cudly-user-key"}, + } + + result, err := handler.getRecommendations(ctx, req, map[string]string{}) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, 0, result.Summary.TotalCount) +} diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 3b0bda6ad..957c33ce3 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1035,6 +1035,9 @@ func (m *mockAuthForExchange) RevokeAPIKeyAPI(_ context.Context, _, _ string) er func (m *mockAuthForExchange) ValidateUserAPIKeyAPI(_ context.Context, _ string) (any, any, error) { return nil, nil, nil } +func (m *mockAuthForExchange) HasAPIKeyPermissionAPI(_ context.Context, _, _, _ string) (string, bool, error) { + return "admin", true, nil +} func (m *mockAuthForExchange) GetAllowedAccountsAPI(_ context.Context, _ string) ([]string, error) { return nil, nil } diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index d57e86e6e..981708dd8 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -327,3 +327,8 @@ func (m *MockAuthService) ValidateUserAPIKeyAPI(ctx context.Context, apiKey stri args := m.Called(ctx, apiKey) return args.Get(0), args.Get(1), args.Error(2) } + +func (m *MockAuthService) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (string, bool, error) { + args := m.Called(ctx, apiKey, action, resource) + return args.String(0), args.Bool(1), args.Error(2) +} diff --git a/internal/api/types.go b/internal/api/types.go index 5f8d68267..3ebaac1b8 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -211,6 +211,15 @@ type AuthServiceInterface interface { DeleteAPIKeyAPI(ctx context.Context, userID, keyID string) error RevokeAPIKeyAPI(ctx context.Context, userID, keyID string) error ValidateUserAPIKeyAPI(ctx context.Context, apiKey string) (any, any, error) + // HasAPIKeyPermissionAPI validates a user API key and checks the + // requested action/resource against the key's effective permissions + // (the intersection of the key's scoped permissions with the owning + // user's group-derived permissions). Returns the owning user's ID and + // whether the permission is held; a non-nil error means the key did + // not validate or the lookup failed, and callers must deny (fail + // closed). Wired into requirePermission so per-key scoping is + // enforced at request time (issue #1142). + HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (string, bool, error) } // Auth request/response types (to avoid import cycle with auth package). diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index a65b06070..22948bf35 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -113,3 +113,31 @@ func (s *Service) RevokeAPIKeyAPI(ctx context.Context, userID, keyID string) err func (s *Service) ValidateUserAPIKeyAPI(ctx context.Context, apiKey string) (*UserAPIKey, *User, error) { return s.ValidateUserAPIKey(ctx, apiKey) } + +// HasAPIKeyPermissionAPI validates a user API key and checks the requested +// 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 and whether the permission is held. +// +// Fail closed: any validation failure (unknown, revoked, or expired key, +// inactive owner) or permission-lookup error returns a non-nil error and +// callers must deny access. +func (s *Service) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID string, allowed bool, err error) { + key, user, err := s.ValidateUserAPIKey(ctx, apiKey) + if err != nil { + return "", false, err + } + + perms, err := s.ComputeEffectivePermissions(ctx, key, user) + if err != nil { + return "", false, fmt.Errorf("computing effective API key permissions: %w", err) + } + + // AuthContext.HasPermission applies the same matching semantics as + // session-based checks, including the admin:* wildcard with its + // money-spending carve-outs (issue #923). + effective := &AuthContext{User: user, Permissions: perms} + return user.ID, effective.HasPermission(action, resource), nil +} diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index b43639584..401e718d3 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -797,6 +797,105 @@ func TestService_ComputeEffectivePermissions(t *testing.T) { }) } +// TestService_HasAPIKeyPermissionAPI is the regression test for issue #1142: +// per-key scoped permissions were persisted and UI-exposed but never enforced +// at request time (ComputeEffectivePermissions had no request-path caller). +// HasAPIKeyPermissionAPI is the request-path enforcement point wired into +// requirePermission. +func TestService_HasAPIKeyPermissionAPI(t *testing.T) { + ctx := context.Background() + + rawKey := "test-api-key-123456" + hash := sha256.Sum256([]byte(rawKey)) + keyHash := base64.RawURLEncoding.EncodeToString(hash[:]) + + userGrpID := "00000000-0000-5000-8000-000000000005" + userGrp := func() *Group { + return &Group{ID: userGrpID, Permissions: []Permission{ + {Action: ActionView, Resource: ResourceRecommendations}, + {Action: ActionCreate, Resource: ResourcePlans}, + }} + } + + // setup returns a service whose store resolves rawKey to a key scoped to + // keyPerms, owned by an active user whose group grants view:recommendations + // and create:plans. This is the exact data shape of a scoped key created + // through POST /api/auth/api-keys. + setup := func(t *testing.T, keyPerms []Permission) *Service { + t.Helper() + mockStore := new(MockStore) + service := &Service{store: mockStore} + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + user := &User{ID: "user-123", Email: "test@example.com", Active: true, GroupIDs: []string{userGrpID}} + keyRecord := &UserAPIKey{ + ID: "key-1", + UserID: "user-123", + KeyHash: keyHash, + IsActive: true, + Permissions: keyPerms, + } + + mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(keyRecord, nil) + mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) + mockStore.On("UpdateAPIKeyLastUsed", mock.Anything, "key-1").Return(nil).Maybe() + mockStore.On("GetGroup", ctx, userGrpID).Return(userGrp(), nil) + return service + } + + t.Run("scoped key grants its in-scope permission", func(t *testing.T) { + service := setup(t, []Permission{{Action: ActionView, Resource: ResourceRecommendations}}) + + userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionView, ResourceRecommendations) + require.NoError(t, err) + assert.Equal(t, "user-123", userID) + assert.True(t, has) + }) + + t.Run("regression #1142: scoped key cannot exceed its scope even when the user holds the permission", func(t *testing.T) { + // The key is scoped to view:recommendations only; the owning user's + // group also grants create:plans. The key must NOT inherit it. + service := setup(t, []Permission{{Action: ActionView, Resource: ResourceRecommendations}}) + + userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionCreate, ResourcePlans) + require.NoError(t, err) + assert.Equal(t, "user-123", userID) + assert.False(t, has, "scoped key must not grant permissions outside its scope") + }) + + t.Run("key permission the user lacks is denied (intersection)", func(t *testing.T) { + // The key claims admin:users but the user's group never granted it. + service := setup(t, []Permission{{Action: ActionAdmin, Resource: ResourceUsers}}) + + userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionAdmin, ResourceUsers) + require.NoError(t, err) + assert.Equal(t, "user-123", userID) + 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) { + 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) + }) + + t.Run("invalid key fails closed with an error", func(t *testing.T) { + mockStore := new(MockStore) + service := &Service{store: mockStore} + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(nil, nil) + + userID, has, err := service.HasAPIKeyPermissionAPI(ctx, rawKey, ActionView, ResourceRecommendations) + assert.Error(t, err) + assert.Empty(t, userID) + assert.False(t, has) + }) +} + func TestService_validateAPIKeyPermissions(t *testing.T) { ctx := context.Background() diff --git a/internal/server/app.go b/internal/server/app.go index 3949d8576..45f56c92d 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1167,3 +1167,8 @@ func (a *authServiceAdapter) RevokeAPIKeyAPI(ctx context.Context, userID, keyID func (a *authServiceAdapter) ValidateUserAPIKeyAPI(ctx context.Context, apiKey string) (any, any, error) { //nolint:gocritic // unnamedResult: return names would conflict with body locals return a.service.ValidateUserAPIKeyAPI(ctx, apiKey) } + +func (a *authServiceAdapter) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID string, allowed bool, err error) { + userID, allowed, err = a.service.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) + return +}