From 0c41872cd474f8213a1f8c7f2aab4dfee8d792bf Mon Sep 17 00:00:00 2001 From: Elias Schneider Date: Mon, 23 Feb 2026 20:34:25 +0100 Subject: [PATCH] fix: disallow API key renewal and creation with API key authentication (#1334) --- backend/internal/common/errors.go | 7 ++ .../internal/controller/api_key_controller.go | 9 +- .../internal/middleware/auth_middleware.go | 28 +++++ .../middleware/auth_middleware_test.go | 104 ++++++++++++++++++ backend/internal/service/api_key_service.go | 3 + 5 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 backend/internal/middleware/auth_middleware_test.go diff --git a/backend/internal/common/errors.go b/backend/internal/common/errors.go index 13fb9a13..b97f2af0 100644 --- a/backend/internal/common/errors.go +++ b/backend/internal/common/errors.go @@ -280,6 +280,13 @@ func (e *APIKeyExpirationDateError) Error() string { } func (e *APIKeyExpirationDateError) HttpStatusCode() int { return http.StatusBadRequest } +type APIKeyAuthNotAllowedError struct{} + +func (e *APIKeyAuthNotAllowedError) Error() string { + return "API key authentication is not allowed for this endpoint" +} +func (e *APIKeyAuthNotAllowedError) HttpStatusCode() int { return http.StatusForbidden } + type OidcInvalidRefreshTokenError struct{} func (e *OidcInvalidRefreshTokenError) Error() string { diff --git a/backend/internal/controller/api_key_controller.go b/backend/internal/controller/api_key_controller.go index 3b364682..0162b96b 100644 --- a/backend/internal/controller/api_key_controller.go +++ b/backend/internal/controller/api_key_controller.go @@ -26,12 +26,11 @@ func NewApiKeyController(group *gin.RouterGroup, authMiddleware *middleware.Auth uc := &ApiKeyController{apiKeyService: apiKeyService} apiKeyGroup := group.Group("/api-keys") - apiKeyGroup.Use(authMiddleware.WithAdminNotRequired().Add()) { - apiKeyGroup.GET("", uc.listApiKeysHandler) - apiKeyGroup.POST("", uc.createApiKeyHandler) - apiKeyGroup.POST("/:id/renew", uc.renewApiKeyHandler) - apiKeyGroup.DELETE("/:id", uc.revokeApiKeyHandler) + apiKeyGroup.GET("", authMiddleware.WithAdminNotRequired().Add(), uc.listApiKeysHandler) + apiKeyGroup.POST("", authMiddleware.WithAdminNotRequired().WithApiKeyAuthDisabled().Add(), uc.createApiKeyHandler) + apiKeyGroup.POST("/:id/renew", authMiddleware.WithAdminNotRequired().WithApiKeyAuthDisabled().Add(), uc.renewApiKeyHandler) + apiKeyGroup.DELETE("/:id", authMiddleware.WithAdminNotRequired().Add(), uc.revokeApiKeyHandler) } } diff --git a/backend/internal/middleware/auth_middleware.go b/backend/internal/middleware/auth_middleware.go index c66b54f1..3af9ce17 100644 --- a/backend/internal/middleware/auth_middleware.go +++ b/backend/internal/middleware/auth_middleware.go @@ -18,6 +18,7 @@ type AuthMiddleware struct { type AuthOptions struct { AdminRequired bool SuccessOptional bool + AllowApiKeyAuth bool } func NewAuthMiddleware( @@ -31,6 +32,7 @@ func NewAuthMiddleware( options: AuthOptions{ AdminRequired: true, SuccessOptional: false, + AllowApiKeyAuth: true, }, } } @@ -59,6 +61,17 @@ func (m *AuthMiddleware) WithSuccessOptional() *AuthMiddleware { return clone } +// WithApiKeyAuthDisabled disables API key authentication fallback and requires JWT auth. +func (m *AuthMiddleware) WithApiKeyAuthDisabled() *AuthMiddleware { + clone := &AuthMiddleware{ + apiKeyMiddleware: m.apiKeyMiddleware, + jwtMiddleware: m.jwtMiddleware, + options: m.options, + } + clone.options.AllowApiKeyAuth = false + return clone +} + func (m *AuthMiddleware) Add() gin.HandlerFunc { return func(c *gin.Context) { userID, isAdmin, err := m.jwtMiddleware.Verify(c, m.options.AdminRequired) @@ -79,6 +92,21 @@ func (m *AuthMiddleware) Add() gin.HandlerFunc { return } + if !m.options.AllowApiKeyAuth { + if m.options.SuccessOptional { + c.Next() + return + } + + c.Abort() + if c.GetHeader("X-API-Key") != "" { + _ = c.Error(&common.APIKeyAuthNotAllowedError{}) + return + } + _ = c.Error(err) + return + } + // JWT auth failed, try API key auth userID, isAdmin, err = m.apiKeyMiddleware.Verify(c, m.options.AdminRequired) if err == nil { diff --git a/backend/internal/middleware/auth_middleware_test.go b/backend/internal/middleware/auth_middleware_test.go new file mode 100644 index 00000000..e3fb0ee6 --- /dev/null +++ b/backend/internal/middleware/auth_middleware_test.go @@ -0,0 +1,104 @@ +package middleware + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + + "github.com/pocket-id/pocket-id/backend/internal/common" + "github.com/pocket-id/pocket-id/backend/internal/dto" + "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" + "github.com/pocket-id/pocket-id/backend/internal/service" + testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" +) + +func TestWithApiKeyAuthDisabled(t *testing.T) { + gin.SetMode(gin.TestMode) + + originalEnvConfig := common.EnvConfig + defer func() { + common.EnvConfig = originalEnvConfig + }() + common.EnvConfig.AppURL = "https://test.example.com" + common.EnvConfig.EncryptionKey = []byte("0123456789abcdef0123456789abcdef") + + db := testutils.NewDatabaseForTest(t) + + appConfigService, err := service.NewAppConfigService(t.Context(), db) + require.NoError(t, err) + + jwtService, err := service.NewJwtService(t.Context(), db, appConfigService) + require.NoError(t, err) + + userService := service.NewUserService(db, jwtService, nil, nil, appConfigService, nil, nil, nil, nil) + apiKeyService, err := service.NewApiKeyService(t.Context(), db, nil) + require.NoError(t, err) + + authMiddleware := NewAuthMiddleware(apiKeyService, userService, jwtService) + + user := createUserForAuthMiddlewareTest(t, db) + jwtToken, err := jwtService.GenerateAccessToken(user) + require.NoError(t, err) + + _, apiKeyToken, err := apiKeyService.CreateApiKey(t.Context(), user.ID, dto.ApiKeyCreateDto{ + Name: "Middleware API Key", + ExpiresAt: datatype.DateTime(time.Now().Add(24 * time.Hour)), + }) + require.NoError(t, err) + + router := gin.New() + router.Use(NewErrorHandlerMiddleware().Add()) + router.GET("/api/protected", authMiddleware.WithAdminNotRequired().WithApiKeyAuthDisabled().Add(), func(c *gin.Context) { + c.Status(http.StatusNoContent) + }) + + t.Run("rejects API key auth when API key auth is disabled", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/protected", nil) + req.Header.Set("X-API-Key", apiKeyToken) + recorder := httptest.NewRecorder() + + router.ServeHTTP(recorder, req) + + require.Equal(t, http.StatusForbidden, recorder.Code) + + var body map[string]string + err := json.Unmarshal(recorder.Body.Bytes(), &body) + require.NoError(t, err) + require.Equal(t, "API key authentication is not allowed for this endpoint", body["error"]) + }) + + t.Run("allows JWT auth when API key auth is disabled", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/protected", nil) + req.Header.Set("Authorization", "Bearer "+jwtToken) + recorder := httptest.NewRecorder() + + router.ServeHTTP(recorder, req) + + require.Equal(t, http.StatusNoContent, recorder.Code) + }) +} + +func createUserForAuthMiddlewareTest(t *testing.T, db *gorm.DB) model.User { + t.Helper() + + email := "auth@example.com" + user := model.User{ + Username: "auth-user", + Email: &email, + FirstName: "Auth", + LastName: "User", + DisplayName: "Auth User", + } + + err := db.Create(&user).Error + require.NoError(t, err) + + return user +} diff --git a/backend/internal/service/api_key_service.go b/backend/internal/service/api_key_service.go index 0d175a92..3c29d6e9 100644 --- a/backend/internal/service/api_key_service.go +++ b/backend/internal/service/api_key_service.go @@ -77,6 +77,9 @@ func (s *ApiKeyService) CreateApiKey(ctx context.Context, userID string, input d Create(&apiKey). Error if err != nil { + if errors.Is(err, gorm.ErrDuplicatedKey) { + return model.ApiKey{}, "", &common.AlreadyInUseError{Property: "API key name"} + } return model.ApiKey{}, "", err }