diff --git a/backend/server/api/middlewares.go b/backend/server/api/middlewares.go index a988c41288a..7d4612fe064 100644 --- a/backend/server/api/middlewares.go +++ b/backend/server/api/middlewares.go @@ -33,6 +33,7 @@ import ( "github.com/apache/incubator-devlake/core/errors" "github.com/apache/incubator-devlake/core/models/common" "github.com/apache/incubator-devlake/helpers/apikeyhelper" + "github.com/apache/incubator-devlake/server/api/shared" "github.com/gin-gonic/gin" ) @@ -272,7 +273,7 @@ func CheckAuthorizationHeader(c *gin.Context, logger log.Logger, db dal.Dal, api logger.Info("redirect path: %s to: %s", c.Request.URL.Path, path) c.Request.URL.Path = path - c.Set(common.USER, &common.User{ + shared.SetUserOnRequest(c, &common.User{ Name: apiKey.Creator.Creator, Email: apiKey.Creator.CreatorEmail, }) diff --git a/backend/server/api/middlewares_authchain_test.go b/backend/server/api/middlewares_authchain_test.go new file mode 100644 index 00000000000..1a99bdd8bc5 --- /dev/null +++ b/backend/server/api/middlewares_authchain_test.go @@ -0,0 +1,161 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api + +import ( + stdctx "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + corectx "github.com/apache/incubator-devlake/core/context" + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + contextimpl "github.com/apache/incubator-devlake/impls/context" + "github.com/apache/incubator-devlake/impls/logruslog" + "github.com/apache/incubator-devlake/server/api/auth" + "github.com/gin-gonic/gin" + "github.com/spf13/viper" +) + +// authChainStubDal is a minimal dal.Dal sufficient to drive the auth chain +// without a real database. Only the three methods exercised by this test are +// implemented; any other call would (intentionally) panic via the embedded +// nil dal.Dal, surfacing an unexpected dependency. +type authChainStubDal struct { + dal.Dal + firstErr errors.Error // returned by First; nil means "found" +} + +// All backs the auth service's revocation-cache boot load. Returning no rows +// keeps the service from depending on a real DB. +func (s *authChainStubDal) All(dst interface{}, _ ...dal.Clause) errors.Error { return nil } + +// First backs apikeyhelper.GetApiKey. On success it leaves dst as the zero +// ApiKey, whose empty AllowedPath regex matches every path — enough to model +// "a valid, correctly-scoped key" without persisting one. +func (s *authChainStubDal) First(dst interface{}, _ ...dal.Clause) errors.Error { return s.firstErr } + +func (s *authChainStubDal) IsErrorNotFound(err error) bool { return err != nil } + +// newAuthChainEnv builds a router whose middleware chain mirrors production +// with AUTH_ENABLED=true and OIDC_ENABLED=false (the hardened-image default): +// +// RestAuthentication -> OIDCAuthentication -> RequireAuth +// +// firstErr controls how the stubbed API-key lookup resolves. +func newAuthChainEnv(t *testing.T, firstErr errors.Error) *gin.Engine { + t.Helper() + // apikeyhelper reads ENCRYPTION_SECRET from the global config (AutomaticEnv). + t.Setenv("ENCRYPTION_SECRET", strings.Repeat("a", 32)) + + cfg := viper.New() + cfg.Set("ENCRYPTION_SECRET", strings.Repeat("a", 32)) + cfg.Set("AUTH_ENABLED", true) + cfg.Set("OIDC_ENABLED", false) + + db := &authChainStubDal{firstErr: firstErr} + basicRes := contextimpl.NewDefaultBasicRes(cfg, logruslog.Global, db) + + ctx, cancel := stdctx.WithCancel(stdctx.Background()) + t.Cleanup(cancel) + svc, err := auth.NewService(ctx, basicRes) + if err != nil { + t.Fatalf("auth.NewService: %v", err) + } + + gin.SetMode(gin.TestMode) + router := gin.New() + router.Use(RestAuthentication(router, basicRes)) // API-key auth for /rest, then re-dispatches + router.Use(svc.OIDCAuthentication()) // session cookie -> sets user (no-op here) + router.Use(svc.RequireAuth()) // terminal gate: no user -> 401 + + // Open-API handler, reached only after RestAuthentication rewrites the + // /rest-prefixed path and re-dispatches through the chain. + router.POST("/plugins/webhook/connections/:id/deployments", func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"success": true}) + }) + // A normal protected route used to prove RequireAuth is actually active. + router.GET("/plugins/webhook/connections", func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"success": true}) + }) + return router +} + +// TestAuthChainHoldsForApiKeyWithOIDCEnabled is the regression test for the +// open-API 401 bug. With AUTH_ENABLED on and the OIDC RequireAuth gate live, a +// valid API key against a /rest endpoint must survive the internal +// HandleContext re-dispatch (which wipes gin Keys) and reach the handler. +func TestAuthChainHoldsForApiKeyWithOIDCEnabled(t *testing.T) { + const restPath = "/rest/plugins/webhook/connections/1/deployments" + + t.Run("valid key reaches open-api handler (200)", func(t *testing.T) { + router := newAuthChainEnv(t, nil) + resp := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, restPath, strings.NewReader(`{}`)) + req.Header.Set("Authorization", "Bearer valid-key") + router.ServeHTTP(resp, req) + + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, want %d (RequireAuth wiped the API-key user)", resp.Code, http.StatusOK) + } + }) + + t.Run("missing token (401)", func(t *testing.T) { + router := newAuthChainEnv(t, nil) + resp := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, restPath, strings.NewReader(`{}`)) + router.ServeHTTP(resp, req) + + if resp.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want %d", resp.Code, http.StatusUnauthorized) + } + if !strings.Contains(resp.Body.String(), "token is missing") { + t.Errorf("body = %q, want it to mention 'token is missing'", resp.Body.String()) + } + }) + + t.Run("invalid key (403)", func(t *testing.T) { + router := newAuthChainEnv(t, errors.NotFound.New("not found")) + resp := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, restPath, strings.NewReader(`{}`)) + req.Header.Set("Authorization", "Bearer wrong-key") + router.ServeHTTP(resp, req) + + if resp.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d", resp.Code, http.StatusForbidden) + } + if !strings.Contains(resp.Body.String(), "api key is invalid") { + t.Errorf("body = %q, want it to mention 'api key is invalid'", resp.Body.String()) + } + }) + + t.Run("protected route without user is gated (401)", func(t *testing.T) { + router := newAuthChainEnv(t, nil) + resp := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/plugins/webhook/connections", nil) + router.ServeHTTP(resp, req) + + if resp.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want %d (RequireAuth not active?)", resp.Code, http.StatusUnauthorized) + } + }) +} + +var _ corectx.BasicRes = (*contextimpl.DefaultBasicRes)(nil) diff --git a/backend/server/api/shared/gin_utils.go b/backend/server/api/shared/gin_utils.go index 892dc489ae6..8cbe144fbac 100644 --- a/backend/server/api/shared/gin_utils.go +++ b/backend/server/api/shared/gin_utils.go @@ -18,15 +18,40 @@ limitations under the License. package shared import ( + "context" + "github.com/apache/incubator-devlake/core/models/common" "github.com/gin-gonic/gin" ) +// userContextKey is a dedicated, unexported type for storing the +// authenticated user on the request's context.Context. A dedicated type +// avoids collisions with other packages and satisfies go vet (which flags +// basic types such as plain strings used as context keys). +type userContextKey struct{} + +// SetUserOnRequest stores the authenticated user on the request's +// context.Context so the identity survives gin's Engine.HandleContext +// re-dispatch, which calls Context.reset() and wipes gin's Keys (where +// c.Set stores values). Callers that re-dispatch (e.g. the /rest open-API +// path rewrite) must use this so the terminal RequireAuth gate can still +// see the user. It also mirrors the value into gin Keys via c.Set for the +// common, non-re-dispatched case. +func SetUserOnRequest(c *gin.Context, user *common.User) { + c.Set(common.USER, user) + c.Request = c.Request.WithContext(context.WithValue(c.Request.Context(), userContextKey{}, user)) +} + func GetUser(c *gin.Context) (*common.User, bool) { - userObj, exist := c.Get(common.USER) - if !exist { - return nil, false + if userObj, exist := c.Get(common.USER); exist { + if user, ok := userObj.(*common.User); ok { + return user, true + } + } + // Fall back to the request context, which survives Engine.HandleContext + // re-dispatch (unlike gin Keys, which Context.reset() clears). + if user, ok := c.Request.Context().Value(userContextKey{}).(*common.User); ok { + return user, true } - user := userObj.(*common.User) - return user, true + return nil, false } diff --git a/backend/server/api/shared/gin_utils_test.go b/backend/server/api/shared/gin_utils_test.go new file mode 100644 index 00000000000..09bfc7465f9 --- /dev/null +++ b/backend/server/api/shared/gin_utils_test.go @@ -0,0 +1,97 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package shared + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/apache/incubator-devlake/core/models/common" + "github.com/gin-gonic/gin" +) + +// TestGetUserReadsGinKeys covers the common, non-re-dispatched case where the +// user is set directly on the gin context. +func TestGetUserReadsGinKeys(t *testing.T) { + gin.SetMode(gin.TestMode) + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Request = httptest.NewRequest(http.MethodGet, "/", nil) + c.Set(common.USER, &common.User{Name: "alice"}) + + user, ok := GetUser(c) + if !ok || user == nil || user.Name != "alice" { + t.Fatalf("GetUser() = %+v, %v; want alice, true", user, ok) + } +} + +// TestSetUserOnRequestSurvivesHandleContext is the regression test for the +// open-API key 401 bug: gin's Engine.HandleContext re-dispatch calls +// Context.reset(), which sets c.Keys = nil and therefore drops anything set +// via c.Set. The authenticated user must instead ride on the request's +// context.Context so the terminal RequireAuth gate can still see it after the +// /rest path-rewrite re-dispatch. +func TestSetUserOnRequestSurvivesHandleContext(t *testing.T) { + gin.SetMode(gin.TestMode) + + var ( + ginKeysVisible bool + requestVisible bool + reachedTerminal bool + ) + + router := gin.New() + router.GET("/target", func(c *gin.Context) { + reachedTerminal = true + // gin Keys are wiped by reset() during HandleContext re-dispatch. + if _, ok := c.Get(common.USER); ok { + ginKeysVisible = true + } + // The user must survive on the request context. + if user, ok := GetUser(c); ok && user != nil && user.Name == "bob" { + requestVisible = true + } + c.Status(http.StatusOK) + }) + + // Entry handler mimics RestAuthentication: authenticate, stash the user, + // rewrite the path, and re-dispatch through the engine. + router.GET("/rest/target", func(c *gin.Context) { + SetUserOnRequest(c, &common.User{Name: "bob"}) + c.Request.URL.Path = "/target" + router.HandleContext(c) + c.Abort() + }) + + resp := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/rest/target", nil) + router.ServeHTTP(resp, req) + + if !reachedTerminal { + t.Fatal("re-dispatched request never reached the terminal handler") + } + if ginKeysVisible { + t.Error("gin Keys unexpectedly survived HandleContext; test no longer exercises the reset() bug") + } + if !requestVisible { + t.Error("user did not survive HandleContext re-dispatch via request context (regression)") + } + if resp.Code != http.StatusOK { + t.Errorf("status = %d, want %d", resp.Code, http.StatusOK) + } +}