{
return;
}
- if (route.query.code) {
- const code = route.query.code as keyof typeof authErrorMessages;
- errorMessage.value = authErrorMessages[code];
+ if (route.query.error) {
+ const error = route.query.error as keyof typeof authErrorMessages;
+ errorMessage.value = authErrorMessages[error] ?? i18n.t('unknown_auth_error', { error });
}
});
From 553776faddcc5a6b48da21a521934c44b77e715e Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 20 Jun 2024 15:41:20 +0200
Subject: [PATCH 04/15] enhance login flow
---
server/api/login.go | 15 ++++-----
server/api/login_test.go | 67 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+), 7 deletions(-)
create mode 100644 server/api/login_test.go
diff --git a/server/api/login.go b/server/api/login.go
index fc5f73ed58..800cdd0688 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -36,6 +36,14 @@ import (
)
func HandleAuth(c *gin.Context) {
+ // oauth flow
+ // 1. send user to forge: https://forge/oauth/authorize?client_id=...&redirect_uri=...&state=...
+ // 2. user comes back from forge
+ // 2a) with ?error=...: redirect to login?error=... (UI route. It will handle error and show message)
+ // 2b) with ?code=...: check state and continue to exchange code for token, following the flow
+ // 3. exchange code for token: POST https://forge/oauth/token?client_id=...&client_secret=...&code
+ // 4. redirect user to UI /
+
_store := store.FromContext(c)
forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
_forge, err := server.Config.Services.Manager.ForgeMain()
@@ -44,13 +52,6 @@ func HandleAuth(c *gin.Context) {
return
}
- // oauth flow
- // 1. send user to forge: https://forge/oauth/authorize?client_id=...&redirect_uri=...&state=...
- // 2. user comes back from forge
- // 2a) with ?error=...: redirect to login?error=... (UI route. It will handle error and show message)
- // 2b) with ?code=...: check state and continue to exchange code for token
- // 3. exchange code for token: POST https://forge/oauth/token?client_id=...&client_secret=...&code
-
// when dealing with redirects, we may need to adjust the content type. I
// cannot, however, remember why, so need to revisit this line.
c.Writer.Header().Del("Content-Type")
diff --git a/server/api/login_test.go b/server/api/login_test.go
new file mode 100644
index 0000000000..55ff344baf
--- /dev/null
+++ b/server/api/login_test.go
@@ -0,0 +1,67 @@
+package api_test
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "testing"
+
+ "github.com/franela/goblin"
+ "github.com/gin-gonic/gin"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/mock"
+
+ "go.woodpecker-ci.org/woodpecker/v2/server"
+ "go.woodpecker-ci.org/woodpecker/v2/server/api"
+ mocks_forge "go.woodpecker-ci.org/woodpecker/v2/server/forge/mocks"
+ "go.woodpecker-ci.org/woodpecker/v2/server/model"
+ mocks_manager "go.woodpecker-ci.org/woodpecker/v2/server/services/mocks"
+ mocks_store "go.woodpecker-ci.org/woodpecker/v2/server/store/mocks"
+)
+
+func TestHandleAuth(t *testing.T) {
+ gin.SetMode(gin.TestMode)
+
+ g := goblin.Goblin(t)
+ g.Describe("Login", func() {
+ g.It("should redirect to forge login", func() {
+ _manager := mocks_manager.NewManager(t)
+ _forge := mocks_forge.NewForge(t)
+ _store := mocks_store.NewStore(t)
+
+ _forge.On("Login", mock.Anything, mock.Anything).Return(&model.User{}, "", nil)
+ _manager.On("ForgeMain").Return(_forge, nil)
+ server.Config.Services.Manager = _manager
+
+ w := httptest.NewRecorder()
+ c, _ := gin.CreateTestContext(w)
+ c.Set("store", _store)
+
+ api.HandleAuth(c)
+
+ // mockStore.AssertCalled(t, "GetPipelineList", mock.Anything, mock.Anything, mock.Anything)
+ assert.Equal(t, http.StatusOK, c.Writer.Status())
+ })
+
+ g.It("should handle the error", func() {
+ _manager := mocks_manager.NewManager(t)
+ _forge := mocks_forge.NewForge(t)
+ _store := mocks_store.NewStore(t)
+
+ _forge.On("Login", mock.Anything, mock.Anything).Return(&model.User{}, "", nil)
+ _manager.On("ForgeMain").Return(_forge, nil)
+ server.Config.Services.Manager = _manager
+
+ w := httptest.NewRecorder()
+ c, _ := gin.CreateTestContext(w)
+ c.Set("store", _store)
+ c.Request.URL, _ = url.Parse("/authorize?error=access_denied")
+
+ api.HandleAuth(c)
+
+ // check if we get redirected to /login?error=access_denied
+ assert.Equal(t, http.StatusFound, c.Writer.Status())
+ assert.Equal(t, "/login?error=access_denied", c.Writer.Header().Get("Location"))
+ })
+ })
+}
From 46a16e81766ea7cfbea69ed3b28786b7bfc94dfe Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 20 Jun 2024 16:31:15 +0200
Subject: [PATCH 05/15] enhance
---
server/api/login.go | 95 ++++++++++++++++++++------------------
server/api/login_test.go | 4 +-
server/services/manager.go | 15 +++---
3 files changed, 61 insertions(+), 53 deletions(-)
diff --git a/server/api/login.go b/server/api/login.go
index 800cdd0688..ade3f5d98a 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -44,70 +44,71 @@ func HandleAuth(c *gin.Context) {
// 3. exchange code for token: POST https://forge/oauth/token?client_id=...&client_secret=...&code
// 4. redirect user to UI /
- _store := store.FromContext(c)
- forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
- _forge, err := server.Config.Services.Manager.ForgeMain()
- if err != nil {
- _ = c.AbortWithError(http.StatusInternalServerError, err)
+ // TODO: when dealing with redirects, we may need to adjust the content type. I
+ // cannot, however, remember why, so need to revisit this line.
+ c.Writer.Header().Del("Content-Type")
+
+ // check for OAuth errors and redirect to the login page and show error
+ if err := c.Request.FormValue("error"); err != "" {
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error="+err)
return
}
- // when dealing with redirects, we may need to adjust the content type. I
- // cannot, however, remember why, so need to revisit this line.
- c.Writer.Header().Del("Content-Type")
+ _store := store.FromContext(c)
+ forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
- // check the OAuth errors
- if c.Request.FormValue("error") != "" {
- redirectURL := _forge.GetRedirectURL(c, forgeID)
+ // if we have an oauth code we need to check the state token
+ oauthCode := c.Request.FormValue("code")
+ if oauthCode != "" {
+ state := c.Request.FormValue("state")
+ if len(state) == 0 {
+ log.Error().Msg("missing state token")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=missing_state")
+ return
+ }
- http.Redirect(c.Writer, c.Request, redirectURL, http.StatusSeeOther)
- return
+ // check the OAuth state token
+ stateToken, err := token.Parse(state, func(t *token.Token) (string, error) {
+ if t.Kind != token.OAuthStateToken {
+ return "", fmt.Errorf("invalid token type")
+ }
+ return server.Config.JWTSecret, nil
+ })
+ if err != nil {
+ log.Error().Err(err).Msg("cannot parse state token")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error")
+ return
+ }
+ forgeID, err = strconv.ParseInt(stateToken.Get("forge-id"), 10, 64)
+ if err != nil {
+ log.Error().Err(err).Msg("cannot parse forge id")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error")
+ return
+ }
}
- // check the OAuth code
- code := c.Request.FormValue("code")
- if len(code) == 0 {
+ state := ""
+ // if we don't have an oauth code yet we create a state token
+ if oauthCode == "" {
exp := time.Now().Add(time.Minute * 15).Unix()
stateToken := token.New(token.OAuthStateToken)
stateToken.Set("forge-id", fmt.Sprintf("%d", forgeID))
- state, err := stateToken.SignExpires(server.Config.JWTSecret, exp)
+ var err error
+ state, err = stateToken.SignExpires(server.Config.JWTSecret, exp)
if err != nil {
log.Error().Err(err).Msg("cannot create state token")
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
return
}
-
- redirectURL := _forge.GetRedirectURL(c, &forge_types.OAuthRequest{
- State: state,
- Code: c.Request.FormValue("code"),
- })
-
- http.Redirect(c.Writer, c.Request, redirectURL, http.StatusSeeOther)
- return
}
- // check the OAuth state
- state := c.Request.FormValue("state")
- if len(state) == 0 {
- log.Error().Msg("missing state token")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error")
- return
- }
-
- // check the OAuth state token
- _token, err := token.Parse(state, func(t *token.Token) (string, error) {
- if t.Kind != token.OAuthStateToken {
- return "", fmt.Errorf("invalid token type")
- }
- return server.Config.JWTSecret, nil
- })
+ _forge, err := server.Config.Services.Manager.ForgeMain()
if err != nil {
- log.Error().Err(err).Msg("cannot parse state token")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error")
+ _ = c.AbortWithError(http.StatusInternalServerError, err)
return
}
- userFromForge, err := _forge.Login(c, &forge_types.OAuthRequest{
+ userFromForge, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{
State: state,
Error: c.Request.FormValue("error"),
ErrorURI: c.Request.FormValue("error_uri"),
@@ -120,6 +121,12 @@ func HandleAuth(c *gin.Context) {
return
}
+ // The user is not authorized yet -> redirect
+ if userFromForge == nil {
+ http.Redirect(c.Writer, c.Request, redirectURL, http.StatusSeeOther)
+ return
+ }
+
// get the user from the database
user, err := _store.GetUserRemoteID(userFromForge.ForgeRemoteID, userFromForge.Login)
if err != nil && !errors.Is(err, types.RecordNotExist) {
@@ -234,7 +241,7 @@ func HandleAuth(c *gin.Context) {
}
exp := time.Now().Add(server.Config.Server.SessionExpires).Unix()
- _token = token.New(token.SessToken)
+ _token := token.New(token.SessToken)
_token.Set("user-id", strconv.FormatInt(user.ID, 10))
tokenString, err := _token.SignExpires(user.Hash, exp)
if err != nil {
diff --git a/server/api/login_test.go b/server/api/login_test.go
index 55ff344baf..d54e729da9 100644
--- a/server/api/login_test.go
+++ b/server/api/login_test.go
@@ -40,7 +40,7 @@ func TestHandleAuth(t *testing.T) {
api.HandleAuth(c)
// mockStore.AssertCalled(t, "GetPipelineList", mock.Anything, mock.Anything, mock.Anything)
- assert.Equal(t, http.StatusOK, c.Writer.Status())
+ assert.Equal(t, http.StatusSeeOther, c.Writer.Status())
})
g.It("should handle the error", func() {
@@ -60,7 +60,7 @@ func TestHandleAuth(t *testing.T) {
api.HandleAuth(c)
// check if we get redirected to /login?error=access_denied
- assert.Equal(t, http.StatusFound, c.Writer.Status())
+ assert.Equal(t, http.StatusSeeOther, c.Writer.Status())
assert.Equal(t, "/login?error=access_denied", c.Writer.Header().Get("Location"))
})
})
diff --git a/server/services/manager.go b/server/services/manager.go
index 079222a02d..0ee6344dc9 100644
--- a/server/services/manager.go
+++ b/server/services/manager.go
@@ -46,6 +46,7 @@ type Manager interface {
EnvironmentService() environment.Service
ForgeFromRepo(repo *model.Repo) (forge.Forge, error)
ForgeFromUser(user *model.User) (forge.Forge, error)
+ ForgeByID(id int64) (forge.Forge, error)
ForgeMain() (forge.Forge, error)
}
@@ -120,18 +121,14 @@ func (m *manager) EnvironmentService() environment.Service {
}
func (m *manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) {
- return m.getForgeByID(repo.ForgeID)
+ return m.ForgeByID(repo.ForgeID)
}
func (m *manager) ForgeFromUser(user *model.User) (forge.Forge, error) {
- return m.getForgeByID(user.ForgeID)
+ return m.ForgeByID(user.ForgeID)
}
-func (m *manager) ForgeMain() (forge.Forge, error) {
- return m.getForgeByID(1) // main forge is always 1 and is configured via environment variables
-}
-
-func (m *manager) getForgeByID(id int64) (forge.Forge, error) {
+func (m *manager) ForgeByID(id int64) (forge.Forge, error) {
item := m.forgeCache.Get(id)
if item != nil && !item.IsExpired() {
return item.Value(), nil
@@ -151,3 +148,7 @@ func (m *manager) getForgeByID(id int64) (forge.Forge, error) {
return forge, nil
}
+
+func (m *manager) ForgeMain() (forge.Forge, error) {
+ return m.ForgeByID(1) // main forge is always 1 and is configured via environment variables
+}
From ee776b88a84820dba245a75099b0d4c11b63bcde Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 20 Jun 2024 16:31:56 +0200
Subject: [PATCH 06/15] update mock
---
server/services/mocks/manager.go | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/server/services/mocks/manager.go b/server/services/mocks/manager.go
index 1d64c2af38..1ef09e4b48 100644
--- a/server/services/mocks/manager.go
+++ b/server/services/mocks/manager.go
@@ -65,6 +65,36 @@ func (_m *Manager) EnvironmentService() environment.Service {
return r0
}
+// ForgeByID provides a mock function with given fields: id
+func (_m *Manager) ForgeByID(id int64) (forge.Forge, error) {
+ ret := _m.Called(id)
+
+ if len(ret) == 0 {
+ panic("no return value specified for ForgeByID")
+ }
+
+ var r0 forge.Forge
+ var r1 error
+ if rf, ok := ret.Get(0).(func(int64) (forge.Forge, error)); ok {
+ return rf(id)
+ }
+ if rf, ok := ret.Get(0).(func(int64) forge.Forge); ok {
+ r0 = rf(id)
+ } else {
+ if ret.Get(0) != nil {
+ r0 = ret.Get(0).(forge.Forge)
+ }
+ }
+
+ if rf, ok := ret.Get(1).(func(int64) error); ok {
+ r1 = rf(id)
+ } else {
+ r1 = ret.Error(1)
+ }
+
+ return r0, r1
+}
+
// ForgeFromRepo provides a mock function with given fields: repo
func (_m *Manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) {
ret := _m.Called(repo)
From 19e2a27f37c1256a36e9e2112e787bcbb3e17549 Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 20 Jun 2024 17:07:39 +0200
Subject: [PATCH 07/15] improve tests
---
server/api/login_test.go | 70 ++++++++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 14 deletions(-)
diff --git a/server/api/login_test.go b/server/api/login_test.go
index d54e729da9..d245e5409c 100644
--- a/server/api/login_test.go
+++ b/server/api/login_test.go
@@ -13,9 +13,9 @@ import (
"go.woodpecker-ci.org/woodpecker/v2/server"
"go.woodpecker-ci.org/woodpecker/v2/server/api"
+
mocks_forge "go.woodpecker-ci.org/woodpecker/v2/server/forge/mocks"
- "go.woodpecker-ci.org/woodpecker/v2/server/model"
- mocks_manager "go.woodpecker-ci.org/woodpecker/v2/server/services/mocks"
+ mocks_services "go.woodpecker-ci.org/woodpecker/v2/server/services/mocks"
mocks_store "go.woodpecker-ci.org/woodpecker/v2/server/store/mocks"
)
@@ -24,44 +24,86 @@ func TestHandleAuth(t *testing.T) {
g := goblin.Goblin(t)
g.Describe("Login", func() {
- g.It("should redirect to forge login", func() {
- _manager := mocks_manager.NewManager(t)
+ g.It("should handle the error", func() {
+ w := httptest.NewRecorder()
+ c, _ := gin.CreateTestContext(w)
+ c.Request = &http.Request{
+ Header: make(http.Header),
+ Method: http.MethodGet,
+ URL: &url.URL{
+ Path: "/authorize",
+ RawQuery: "error=access_denied",
+ },
+ }
+
+ api.HandleAuth(c)
+
+ // check if we get redirected to /login?error=access_denied
+ assert.Equal(t, http.StatusSeeOther, c.Writer.Status())
+ assert.Equal(t, "/login?error=access_denied", c.Writer.Header().Get("Location"))
+ })
+
+ g.It("should fail if a code was provided, but no state", func() {
+ // TODO
+ })
+
+ g.It("should fail if a code was provided, but the state is wrong", func() {
+ // TODO
+ })
+
+ g.It("should fail if a code was provided, but the state is wrong", func() {
+ // TODO
+ })
+
+ g.It("should redirect to forge login page", func() {
+ _manager := mocks_services.NewManager(t)
_forge := mocks_forge.NewForge(t)
_store := mocks_store.NewStore(t)
- _forge.On("Login", mock.Anything, mock.Anything).Return(&model.User{}, "", nil)
+ forgeRedirectURL := "https://my-awesome-forge.com/oauth/authorize?client_id=client-id"
+
+ _forge.On("Login", mock.Anything, mock.Anything).Return(nil, forgeRedirectURL, nil)
_manager.On("ForgeMain").Return(_forge, nil)
server.Config.Services.Manager = _manager
w := httptest.NewRecorder()
c, _ := gin.CreateTestContext(w)
c.Set("store", _store)
+ c.Request = &http.Request{
+ Header: make(http.Header),
+ }
api.HandleAuth(c)
- // mockStore.AssertCalled(t, "GetPipelineList", mock.Anything, mock.Anything, mock.Anything)
assert.Equal(t, http.StatusSeeOther, c.Writer.Status())
+ assert.Equal(t, forgeRedirectURL, c.Writer.Header().Get("Location"))
})
- g.It("should handle the error", func() {
- _manager := mocks_manager.NewManager(t)
+ g.It("should handle the callback and register a new user", func() {
+ _manager := mocks_services.NewManager(t)
_forge := mocks_forge.NewForge(t)
_store := mocks_store.NewStore(t)
- _forge.On("Login", mock.Anything, mock.Anything).Return(&model.User{}, "", nil)
+ _forge.On("Callback", mock.Anything, mock.Anything).Return(nil, nil)
_manager.On("ForgeMain").Return(_forge, nil)
server.Config.Services.Manager = _manager
w := httptest.NewRecorder()
c, _ := gin.CreateTestContext(w)
c.Set("store", _store)
- c.Request.URL, _ = url.Parse("/authorize?error=access_denied")
+ c.Request = &http.Request{
+ Header: make(http.Header),
+ Method: http.MethodGet,
+ URL: &url.URL{},
+ }
+ })
- api.HandleAuth(c)
+ g.It("should handle the callback and login an existing user", func() {
+ // TODO: implement
+ })
- // check if we get redirected to /login?error=access_denied
- assert.Equal(t, http.StatusSeeOther, c.Writer.Status())
- assert.Equal(t, "/login?error=access_denied", c.Writer.Header().Get("Location"))
+ g.It("should handle the callback and deny a new user to register", func() {
+ // TODO: implement
})
})
}
From f12611259ccb5b83c2a9f9fd6b9681125f81d93f Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Wed, 26 Jun 2024 20:07:11 +0200
Subject: [PATCH 08/15] use state
---
cmd/server/setup.go | 23 ++----
server/api/login.go | 188 +++++++++++++++++++-------------------------
server/config.go | 4 +-
3 files changed, 91 insertions(+), 124 deletions(-)
diff --git a/cmd/server/setup.go b/cmd/server/setup.go
index 02b8a37079..343b337d9f 100644
--- a/cmd/server/setup.go
+++ b/cmd/server/setup.go
@@ -17,13 +17,13 @@ package main
import (
"context"
- "crypto/rand"
- "encoding/base64"
+ "encoding/base32"
"errors"
"fmt"
"os"
"time"
+ "github.com/gorilla/securecookie"
"github.com/prometheus/client_golang/prometheus"
prometheus_auto "github.com/prometheus/client_golang/prometheus/promauto"
"github.com/rs/zerolog/log"
@@ -172,28 +172,21 @@ func setupLogStore(c *cli.Context, s store.Store) (logService.Service, error) {
const jwtSecretID = "jwt-secret"
-func randToken() (string, error) {
- b := make([]byte, 32)
- if _, err := rand.Read(b); err != nil {
- return "", err
- }
- return base64.StdEncoding.EncodeToString(b), nil
-}
-
func setupJWTSecret(_store store.Store) (string, error) {
jwtSecret, err := _store.ServerConfigGet(jwtSecretID)
if errors.Is(err, types.RecordNotExist) {
- jwtSecret, err := randToken()
- if err != nil {
- return "", err
- }
+ jwtSecret := base32.StdEncoding.EncodeToString(
+ securecookie.GenerateRandomKey(32),
+ )
err = _store.ServerConfigSet(jwtSecretID, jwtSecret)
if err != nil {
return "", err
}
log.Debug().Msg("created jwt secret")
return jwtSecret, nil
- } else if err != nil {
+ }
+
+ if err != nil {
return "", err
}
diff --git a/server/api/login.go b/server/api/login.go
index ade3f5d98a..9f8fa4cbe8 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -19,6 +19,7 @@ import (
"errors"
"fmt"
"net/http"
+ "net/url"
"strconv"
"time"
@@ -27,6 +28,7 @@ import (
"github.com/rs/zerolog/log"
"go.woodpecker-ci.org/woodpecker/v2/server"
+ "go.woodpecker-ci.org/woodpecker/v2/server/forge"
forge_types "go.woodpecker-ci.org/woodpecker/v2/server/forge/types"
"go.woodpecker-ci.org/woodpecker/v2/server/model"
"go.woodpecker-ci.org/woodpecker/v2/server/store"
@@ -36,65 +38,43 @@ import (
)
func HandleAuth(c *gin.Context) {
- // oauth flow
- // 1. send user to forge: https://forge/oauth/authorize?client_id=...&redirect_uri=...&state=...
- // 2. user comes back from forge
- // 2a) with ?error=...: redirect to login?error=... (UI route. It will handle error and show message)
- // 2b) with ?code=...: check state and continue to exchange code for token, following the flow
- // 3. exchange code for token: POST https://forge/oauth/token?client_id=...&client_secret=...&code
- // 4. redirect user to UI /
-
- // TODO: when dealing with redirects, we may need to adjust the content type. I
- // cannot, however, remember why, so need to revisit this line.
+ // TODO: check if this is really needed
c.Writer.Header().Del("Content-Type")
- // check for OAuth errors and redirect to the login page and show error
+ // redirect when getting oauth error from forge to login page
if err := c.Request.FormValue("error"); err != "" {
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error="+err)
+ query := url.Values{}
+ query.Set("error", err)
+ if errorDescription := c.Request.FormValue("error_description"); errorDescription != "" {
+ query.Set("error_description", errorDescription)
+ }
+ if errorURI := c.Request.FormValue("error_uri"); errorURI != "" {
+ query.Set("error_uri", errorURI)
+ }
+ c.Redirect(http.StatusSeeOther, fmt.Sprintf("%s/login?%s", server.Config.Server.RootPath, query.Encode()))
return
}
_store := store.FromContext(c)
forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
-
- // if we have an oauth code we need to check the state token
- oauthCode := c.Request.FormValue("code")
- if oauthCode != "" {
- state := c.Request.FormValue("state")
- if len(state) == 0 {
- log.Error().Msg("missing state token")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=missing_state")
- return
- }
-
- // check the OAuth state token
- stateToken, err := token.Parse(state, func(t *token.Token) (string, error) {
- if t.Kind != token.OAuthStateToken {
- return "", fmt.Errorf("invalid token type")
- }
- return server.Config.JWTSecret, nil
- })
- if err != nil {
- log.Error().Err(err).Msg("cannot parse state token")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error")
- return
- }
- forgeID, err = strconv.ParseInt(stateToken.Get("forge-id"), 10, 64)
- if err != nil {
- log.Error().Err(err).Msg("cannot parse forge id")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error")
- return
- }
+ _forge, err := server.Config.Services.Manager.ForgeMain()
+ if err != nil {
+ log.Error().Err(err).Msg("Cannot get main forge")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
+ return
}
+ code := c.Request.FormValue("code")
state := ""
- // if we don't have an oauth code yet we create a state token
- if oauthCode == "" {
+ isCallback := code != ""
+
+ // only generate a state token if not a callback
+ if !isCallback {
+ jwtSecret := server.Config.Server.JWTSecret
exp := time.Now().Add(time.Minute * 15).Unix()
stateToken := token.New(token.OAuthStateToken)
- stateToken.Set("forge-id", fmt.Sprintf("%d", forgeID))
- var err error
- state, err = stateToken.SignExpires(server.Config.JWTSecret, exp)
+ stateToken.Set("forge-id", strconv.FormatInt(forgeID, 10))
+ state, err = stateToken.SignExpires(jwtSecret, exp)
if err != nil {
log.Error().Err(err).Msg("cannot create state token")
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
@@ -102,57 +82,48 @@ func HandleAuth(c *gin.Context) {
}
}
- _forge, err := server.Config.Services.Manager.ForgeMain()
- if err != nil {
- _ = c.AbortWithError(http.StatusInternalServerError, err)
- return
- }
-
userFromForge, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{
- State: state,
- Error: c.Request.FormValue("error"),
- ErrorURI: c.Request.FormValue("error_uri"),
- ErrorDescription: c.Request.FormValue("error_description"),
- Code: c.Request.FormValue("code"),
+ Code: c.Request.FormValue("code"),
+ State: state,
})
if err != nil {
log.Error().Err(err).Msg("cannot authenticate user")
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error")
return
}
-
// The user is not authorized yet -> redirect
if userFromForge == nil {
http.Redirect(c.Writer, c.Request, redirectURL, http.StatusSeeOther)
return
}
+ // if organization filter is enabled, we need to check if the user is a member of one
+ // of the configured organizations
+ if server.Config.Permissions.Orgs.IsConfigured {
+ teams, terr := _forge.Teams(c, userFromForge)
+ if terr != nil || !server.Config.Permissions.Orgs.IsMember(teams) {
+ log.Error().Err(terr).Msgf("cannot verify team membership for %s", userFromForge.Login)
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=org_access_denied")
+ return
+ }
+ }
+
// get the user from the database
user, err := _store.GetUserRemoteID(userFromForge.ForgeRemoteID, userFromForge.Login)
if err != nil && !errors.Is(err, types.RecordNotExist) {
- _ = c.AbortWithError(http.StatusInternalServerError, err)
+ log.Error().Err(err).Msgf("cannot get user %s", userFromForge.Login)
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
return
}
- if errors.Is(err, types.RecordNotExist) {
+ if user == nil || errors.Is(err, types.RecordNotExist) {
// if self-registration is disabled we should return a not authorized error
if !server.Config.Permissions.Open && !server.Config.Permissions.Admins.IsAdmin(userFromForge) {
log.Error().Msgf("cannot register %s. registration closed", userFromForge.Login)
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=access_denied")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=registration_closed")
return
}
- // if self-registration is enabled for allowed organizations we need to
- // check the user's organization membership.
- if server.Config.Permissions.Orgs.IsConfigured {
- teams, terr := _forge.Teams(c, userFromForge)
- if terr != nil || !server.Config.Permissions.Orgs.IsMember(teams) {
- log.Error().Err(terr).Msgf("cannot verify team membership for %s.", user.Login)
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=access_denied")
- return
- }
- }
-
// create the user account
user = &model.User{
Login: userFromForge.Login,
@@ -173,21 +144,27 @@ func HandleAuth(c *gin.Context) {
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
return
}
+ }
- // if another user already have activated repos on behave of that user,
- // the user was stored as org. now we adopt it to the user.
+ // create or set the user's organization if it isn't linked yet
+ if user.OrgID == 0 {
+ // check if an org with the same name exists already and assign it to the user if it does
if org, err := _store.OrgFindByName(user.Login); err == nil && org != nil {
org.IsUser = true
user.OrgID = org.ID
+
if err := _store.OrgUpdate(org); err != nil {
log.Error().Err(err).Msgf("on user creation, could not mark org as user")
}
- } else {
- if err != nil && !errors.Is(err, types.RecordNotExist) {
- _ = c.AbortWithError(http.StatusInternalServerError, err)
- return
- }
- org = &model.Org{
+ }
+ if err != nil && !errors.Is(err, types.RecordNotExist) {
+ log.Error().Err(err).Msgf("cannot get org %s", user.Login)
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
+ return
+ }
+
+ if user.OrgID == 0 {
+ org := &model.Org{
Name: user.Login,
IsUser: true,
Private: false,
@@ -198,19 +175,19 @@ func HandleAuth(c *gin.Context) {
}
user.OrgID = org.ID
}
- }
-
- // update org name
- if user.Login != userFromForge.Login {
+ } else {
+ // update org name if necessary
org, err := _store.OrgGet(user.OrgID)
if err != nil {
log.Error().Err(err).Msgf("cannot get org %s", user.Login)
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
return
}
- org.Name = user.Login
- if err := _store.OrgUpdate(org); err != nil {
- log.Error().Err(err).Msgf("on user creation, could not mark org as user")
+ if org != nil && org.Name != user.Login {
+ org.Name = user.Login
+ if err := _store.OrgUpdate(org); err != nil {
+ log.Error().Err(err).Msgf("on user creation, could not mark org as user")
+ }
}
}
@@ -223,17 +200,6 @@ func HandleAuth(c *gin.Context) {
user.Login = userFromForge.Login
user.Admin = user.Admin || server.Config.Permissions.Admins.IsAdmin(userFromForge)
- // if self-registration is enabled for allowed organizations we need to
- // check the user's organization membership.
- if server.Config.Permissions.Orgs.IsConfigured {
- teams, terr := _forge.Teams(c, user)
- if terr != nil || !server.Config.Permissions.Orgs.IsMember(teams) {
- log.Error().Err(terr).Msgf("cannot verify team membership for %s", user.Login)
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=access_denied")
- return
- }
- }
-
if err := _store.UpdateUser(user); err != nil {
log.Error().Err(err).Msgf("cannot update %s", user.Login)
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
@@ -250,16 +216,28 @@ func HandleAuth(c *gin.Context) {
return
}
+ err = updateRepoPermissions(c, user, _store, _forge)
+ if err != nil {
+ log.Error().Err(err).Msgf("cannot update repo permissions for %s", user.Login)
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
+ return
+ }
+
+ httputil.SetCookie(c.Writer, c.Request, "user_sess", tokenString)
+
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/")
+}
+
+func updateRepoPermissions(c *gin.Context, user *model.User, _store store.Store, _forge forge.Forge) error {
repos, _ := _forge.Repos(c, user)
+
for _, forgeRepo := range repos {
dbRepo, err := _store.GetRepoForgeID(forgeRepo.ForgeRemoteID)
if err != nil && errors.Is(err, types.RecordNotExist) {
continue
}
if err != nil {
- log.Error().Err(err).Msgf("cannot list repos for %s", user.Login)
- c.Redirect(http.StatusSeeOther, "/login?error=internal_error")
- return
+ return err
}
if !dbRepo.IsActive {
@@ -273,15 +251,11 @@ func HandleAuth(c *gin.Context) {
perm.UserID = user.ID
perm.Synced = time.Now().Unix()
if err := _store.PermUpsert(perm); err != nil {
- log.Error().Err(err).Msgf("cannot update permissions for %s", user.Login)
- c.Redirect(http.StatusSeeOther, "/login?error=internal_error")
- return
+ return err
}
}
- httputil.SetCookie(c.Writer, c.Request, "user_sess", tokenString)
-
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/")
+ return nil
}
func GetLogout(c *gin.Context) {
diff --git a/server/config.go b/server/config.go
index ffb0779cc1..bb5ba721e5 100644
--- a/server/config.go
+++ b/server/config.go
@@ -29,8 +29,7 @@ import (
)
var Config = struct {
- JWTSecret string
- Services struct {
+ Services struct {
Pubsub *pubsub.Publisher
Queue queue.Queue
Logs logging.Log
@@ -39,6 +38,7 @@ var Config = struct {
LogStore log.Service
}
Server struct {
+ JWTSecret string
Key string
Cert string
OAuthHost string
From 38a1232f806bb3988ecf50d2301f16448c719f50 Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Wed, 26 Jun 2024 20:11:25 +0200
Subject: [PATCH 09/15] cleanup
---
server/api/hook.go | 2 +-
server/api/login.go | 4 ++--
server/services/manager.go | 5 -----
3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/server/api/hook.go b/server/api/hook.go
index 40a93ef356..e5d0a63ba2 100644
--- a/server/api/hook.go
+++ b/server/api/hook.go
@@ -104,7 +104,7 @@ func BlockTilQueueHasRunningItem(c *gin.Context) {
func PostHook(c *gin.Context) {
_store := store.FromContext(c)
- _forge, err := server.Config.Services.Manager.ForgeMain() // TODO: get the forge for the specific repo somehow
+ _forge, err := server.Config.Services.Manager.ForgeByID(1) // TODO: get the forge for the specific repo somehow
if err != nil {
log.Error().Err(err).Msg("Cannot get main forge")
c.AbortWithStatus(http.StatusInternalServerError)
diff --git a/server/api/login.go b/server/api/login.go
index 9f8fa4cbe8..a384516c4a 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -57,7 +57,7 @@ func HandleAuth(c *gin.Context) {
_store := store.FromContext(c)
forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
- _forge, err := server.Config.Services.Manager.ForgeMain()
+ _forge, err := server.Config.Services.Manager.ForgeByID(forgeID)
if err != nil {
log.Error().Err(err).Msg("Cannot get main forge")
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
@@ -268,7 +268,7 @@ func GetLogout(c *gin.Context) {
func DeprecatedGetLoginToken(c *gin.Context) {
_store := store.FromContext(c)
- _forge, err := server.Config.Services.Manager.ForgeMain() // TODO: get selected forge from auth request
+ _forge, err := server.Config.Services.Manager.ForgeByID(1) // TODO: get selected forge from auth request
if err != nil {
log.Error().Err(err).Msg("Cannot get main forge")
c.AbortWithStatus(http.StatusInternalServerError)
diff --git a/server/services/manager.go b/server/services/manager.go
index be846c6e08..39ce07a488 100644
--- a/server/services/manager.go
+++ b/server/services/manager.go
@@ -47,7 +47,6 @@ type Manager interface {
ForgeFromRepo(repo *model.Repo) (forge.Forge, error)
ForgeFromUser(user *model.User) (forge.Forge, error)
ForgeByID(id int64) (forge.Forge, error)
- ForgeMain() (forge.Forge, error)
}
type manager struct {
@@ -148,7 +147,3 @@ func (m *manager) ForgeByID(id int64) (forge.Forge, error) {
return forge, nil
}
-
-func (m *manager) ForgeMain() (forge.Forge, error) {
- return m.ForgeByID(1) // main forge is always 1 and is configured via environment variables
-}
From 0902294bccd450fd44f8b6aab3c33c228cd9c2cb Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Wed, 26 Jun 2024 20:11:50 +0200
Subject: [PATCH 10/15] cleanup
---
server/services/mocks/manager.go | 30 ------------------------------
1 file changed, 30 deletions(-)
diff --git a/server/services/mocks/manager.go b/server/services/mocks/manager.go
index 7a4fc0ec2b..0df118f818 100644
--- a/server/services/mocks/manager.go
+++ b/server/services/mocks/manager.go
@@ -158,36 +158,6 @@ func (_m *Manager) ForgeFromUser(user *model.User) (forge.Forge, error) {
return r0, r1
}
-// ForgeMain provides a mock function with given fields:
-func (_m *Manager) ForgeMain() (forge.Forge, error) {
- ret := _m.Called()
-
- if len(ret) == 0 {
- panic("no return value specified for ForgeMain")
- }
-
- var r0 forge.Forge
- var r1 error
- if rf, ok := ret.Get(0).(func() (forge.Forge, error)); ok {
- return rf()
- }
- if rf, ok := ret.Get(0).(func() forge.Forge); ok {
- r0 = rf()
- } else {
- if ret.Get(0) != nil {
- r0 = ret.Get(0).(forge.Forge)
- }
- }
-
- if rf, ok := ret.Get(1).(func() error); ok {
- r1 = rf()
- } else {
- r1 = ret.Error(1)
- }
-
- return r0, r1
-}
-
// RegistryService provides a mock function with given fields:
func (_m *Manager) RegistryService() registry.Service {
ret := _m.Called()
From fa8751dfec2233731b42f2cdc835991e542bea78 Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 27 Jun 2024 09:40:51 +0200
Subject: [PATCH 11/15] fix test
---
.vscode/settings.json | 1 +
server/api/login.go | 51 +++++++++++++++++++++------
server/api/login_test.go | 63 ++++++++++++++++++++++++++--------
web/src/assets/locales/en.json | 3 +-
web/src/views/Login.vue | 1 +
5 files changed, 93 insertions(+), 26 deletions(-)
diff --git a/.vscode/settings.json b/.vscode/settings.json
index 90fbfbc2b6..daee9de8e0 100644
--- a/.vscode/settings.json
+++ b/.vscode/settings.json
@@ -8,6 +8,7 @@
},
"go.lintTool": "golangci-lint",
"go.lintFlags": ["--fast"],
+ "go.buildTags": "test",
"eslint.workingDirectories": ["./web"],
"prettier.ignorePath": "./web/.prettierignore",
// Enable the ESlint flat config support
diff --git a/server/api/login.go b/server/api/login.go
index a384516c4a..e9f62e0a9c 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -56,20 +56,44 @@ func HandleAuth(c *gin.Context) {
}
_store := store.FromContext(c)
- forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
- _forge, err := server.Config.Services.Manager.ForgeByID(forgeID)
- if err != nil {
- log.Error().Err(err).Msg("Cannot get main forge")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
- return
- }
code := c.Request.FormValue("code")
- state := ""
+ state := c.Request.FormValue("state")
isCallback := code != ""
+ forgeID := int64(-1) // we use -1 to fail if no forge-id was found
+
+ if isCallback { // validate the state token
+ stateToken, err := token.Parse([]token.Type{token.OAuthStateToken}, state, func(t *token.Token) (string, error) {
+ return server.Config.Server.JWTSecret, nil
+ })
+ if err != nil {
+ log.Error().Err(err).Msg("cannot verify state token")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state")
+ return
+ }
+
+ _forgeID := stateToken.Get("forge-id")
+ forgeID, err = strconv.ParseInt(_forgeID, 10, 64)
+ if err != nil {
+ log.Error().Err(err).Msg("forge-id of state token invalid")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state")
+ return
+ }
+ } else { // only generate a state token if not a callback
+ var err error
+
+ _forgeID := c.Request.FormValue("state")
+ if _forgeID == "" {
+ forgeID = 1 // fallback to main forge
+ } else {
+ forgeID, err = strconv.ParseInt(_forgeID, 10, 64)
+ if err != nil {
+ log.Error().Err(err).Msg("forge-id of state token invalid")
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state")
+ return
+ }
+ }
- // only generate a state token if not a callback
- if !isCallback {
jwtSecret := server.Config.Server.JWTSecret
exp := time.Now().Add(time.Minute * 15).Unix()
stateToken := token.New(token.OAuthStateToken)
@@ -82,6 +106,13 @@ func HandleAuth(c *gin.Context) {
}
}
+ _forge, err := server.Config.Services.Manager.ForgeByID(forgeID)
+ if err != nil {
+ log.Error().Err(err).Msgf("Cannot get forge by id %d", forgeID)
+ c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
+ return
+ }
+
userFromForge, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{
Code: c.Request.FormValue("code"),
State: state,
diff --git a/server/api/login_test.go b/server/api/login_test.go
index 5de2745098..0b8bbc46d7 100644
--- a/server/api/login_test.go
+++ b/server/api/login_test.go
@@ -1,6 +1,7 @@
package api_test
import (
+ "context"
"fmt"
"net/http"
"net/http/httptest"
@@ -16,11 +17,13 @@ import (
"go.woodpecker-ci.org/woodpecker/v2/server"
"go.woodpecker-ci.org/woodpecker/v2/server/api"
mocks_forge "go.woodpecker-ci.org/woodpecker/v2/server/forge/mocks"
+ forge_types "go.woodpecker-ci.org/woodpecker/v2/server/forge/types"
"go.woodpecker-ci.org/woodpecker/v2/server/model"
mocks_services "go.woodpecker-ci.org/woodpecker/v2/server/services/mocks"
"go.woodpecker-ci.org/woodpecker/v2/server/services/permissions"
mocks_store "go.woodpecker-ci.org/woodpecker/v2/server/store/mocks"
"go.woodpecker-ci.org/woodpecker/v2/server/store/types"
+ "go.woodpecker-ci.org/woodpecker/v2/shared/token"
)
func TestHandleAuth(t *testing.T) {
@@ -69,12 +72,37 @@ func TestHandleAuth(t *testing.T) {
assert.Equal(g, fmt.Sprintf("/login?%s", query.Encode()), c.Writer.Header().Get("Location"))
})
- g.It("should fail if a code was provided and no state", func() {
- // TODO: implement
- })
-
g.It("should fail if the state is wrong", func() {
- // TODO: implement
+ _manager := mocks_services.NewManager(t)
+ _store := mocks_store.NewStore(t)
+ server.Config.Services.Manager = _manager
+ server.Config.Permissions.Open = true
+ server.Config.Permissions.Orgs = permissions.NewOrgs(nil)
+ server.Config.Permissions.Admins = permissions.NewAdmins(nil)
+ w := httptest.NewRecorder()
+ c, _ := gin.CreateTestContext(w)
+ c.Set("store", _store)
+
+ query := url.Values{}
+ query.Set("code", "assumed_to_be_valid_code")
+
+ wrongToken := token.New(token.OAuthStateToken)
+ wrongToken.Set("forge_id", "1")
+ signedWrongToken, _ := wrongToken.Sign("wrong_secret")
+ query.Set("state", signedWrongToken)
+
+ c.Request = &http.Request{
+ Header: make(http.Header),
+ URL: &url.URL{
+ Scheme: "https",
+ RawQuery: query.Encode(),
+ },
+ }
+
+ api.HandleAuth(c)
+
+ assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+ assert.Equal(g, "/login?error=invalid_state", c.Writer.Header().Get("Location"))
})
g.It("should redirect to forge login page", func() {
@@ -95,10 +123,15 @@ func TestHandleAuth(t *testing.T) {
},
}
- forgeRedirectURL := "https://my-awesome-forge.com/oauth/authorize?client_id=client-id"
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
- _manager.On("ForgeMain").Return(_forge, nil)
- _forge.On("Login", mock.Anything, mock.Anything).Return(nil, forgeRedirectURL, nil)
+ forgeRedirectURL := ""
+ _forge.On("Login", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+ state := args.Get(1).(*forge_types.OAuthRequest)
+ forgeRedirectURL = fmt.Sprintf("https://my-awesome-forge.com/oauth/authorize?client_id=client-id&state=%s", state.State)
+ }).Return(nil, func(context.Context, *forge_types.OAuthRequest) string {
+ return forgeRedirectURL
+ }, nil)
api.HandleAuth(c)
@@ -124,7 +157,7 @@ func TestHandleAuth(t *testing.T) {
},
}
- _manager.On("ForgeMain").Return(_forge, nil)
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(nil, types.RecordNotExist)
_store.On("CreateUser", mock.Anything).Return(nil)
@@ -158,7 +191,7 @@ func TestHandleAuth(t *testing.T) {
},
}
- _manager.On("ForgeMain").Return(_forge, nil)
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
_store.On("OrgGet", org.ID).Return(org, nil)
@@ -190,7 +223,7 @@ func TestHandleAuth(t *testing.T) {
},
}
- _manager.On("ForgeMain").Return(_forge, nil)
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(nil, types.RecordNotExist)
@@ -218,7 +251,7 @@ func TestHandleAuth(t *testing.T) {
},
}
- _manager.On("ForgeMain").Return(_forge, nil)
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
_forge.On("Teams", mock.Anything, user).Return([]*model.Team{
{
@@ -252,7 +285,7 @@ func TestHandleAuth(t *testing.T) {
}
user.OrgID = 0
- _manager.On("ForgeMain").Return(_forge, nil)
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
_store.On("OrgFindByName", user.Login).Return(nil, types.RecordNotExist)
@@ -286,7 +319,7 @@ func TestHandleAuth(t *testing.T) {
}
user.OrgID = 0
- _manager.On("ForgeMain").Return(_forge, nil)
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
_store.On("OrgFindByName", user.Login).Return(org, nil)
@@ -320,7 +353,7 @@ func TestHandleAuth(t *testing.T) {
}
org.Name = "not-the-user-name"
- _manager.On("ForgeMain").Return(_forge, nil)
+ _manager.On("ForgeByID", int64(1)).Return(_forge, nil)
_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
_store.On("OrgGet", user.OrgID).Return(org, nil)
diff --git a/web/src/assets/locales/en.json b/web/src/assets/locales/en.json
index 41ba7dd178..646b965bce 100644
--- a/web/src/assets/locales/en.json
+++ b/web/src/assets/locales/en.json
@@ -452,5 +452,6 @@
"oauth_error": "Error while authenticating against OAuth provider",
"internal_error": "Some internal error occurred",
"registration_closed": "The registration is closed",
- "access_denied": "You are not allowed to access this instance"
+ "access_denied": "You are not allowed to access this instance",
+ "invalid_state": "The OAuth state is invalid"
}
diff --git a/web/src/views/Login.vue b/web/src/views/Login.vue
index 65b25e769b..9f60c192bf 100644
--- a/web/src/views/Login.vue
+++ b/web/src/views/Login.vue
@@ -52,6 +52,7 @@ const authErrorMessages = {
internal_error: i18n.t('internal_error'),
registration_closed: i18n.t('registration_closed'),
access_denied: i18n.t('access_denied'),
+ invalid_state: i18n.t('invalid_state'),
};
const errorMessage = ref();
From 07fa9164c2485ec0e2aecc690451eca17333cd7f Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 27 Jun 2024 09:44:36 +0200
Subject: [PATCH 12/15] enhance code
---
server/api/login.go | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/server/api/login.go b/server/api/login.go
index e9f62e0a9c..434d7b1a95 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -59,7 +59,7 @@ func HandleAuth(c *gin.Context) {
code := c.Request.FormValue("code")
state := c.Request.FormValue("state")
- isCallback := code != ""
+ isCallback := code != "" && state != ""
forgeID := int64(-1) // we use -1 to fail if no forge-id was found
if isCallback { // validate the state token
@@ -82,7 +82,7 @@ func HandleAuth(c *gin.Context) {
} else { // only generate a state token if not a callback
var err error
- _forgeID := c.Request.FormValue("state")
+ _forgeID := c.Request.FormValue("forge_id")
if _forgeID == "" {
forgeID = 1 // fallback to main forge
} else {
From abc18d90e197c816442c8cf5474da6d527f32202 Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 27 Jun 2024 09:47:23 +0200
Subject: [PATCH 13/15] rm unrelated
---
server/api/login.go | 25 ++-----------------------
1 file changed, 2 insertions(+), 23 deletions(-)
diff --git a/server/api/login.go b/server/api/login.go
index 434d7b1a95..f0e2bd4a29 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -60,10 +60,10 @@ func HandleAuth(c *gin.Context) {
code := c.Request.FormValue("code")
state := c.Request.FormValue("state")
isCallback := code != "" && state != ""
- forgeID := int64(-1) // we use -1 to fail if no forge-id was found
+ forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
if isCallback { // validate the state token
- stateToken, err := token.Parse([]token.Type{token.OAuthStateToken}, state, func(t *token.Token) (string, error) {
+ _, err := token.Parse([]token.Type{token.OAuthStateToken}, state, func(t *token.Token) (string, error) {
return server.Config.Server.JWTSecret, nil
})
if err != nil {
@@ -71,29 +71,8 @@ func HandleAuth(c *gin.Context) {
c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state")
return
}
-
- _forgeID := stateToken.Get("forge-id")
- forgeID, err = strconv.ParseInt(_forgeID, 10, 64)
- if err != nil {
- log.Error().Err(err).Msg("forge-id of state token invalid")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state")
- return
- }
} else { // only generate a state token if not a callback
var err error
-
- _forgeID := c.Request.FormValue("forge_id")
- if _forgeID == "" {
- forgeID = 1 // fallback to main forge
- } else {
- forgeID, err = strconv.ParseInt(_forgeID, 10, 64)
- if err != nil {
- log.Error().Err(err).Msg("forge-id of state token invalid")
- c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state")
- return
- }
- }
-
jwtSecret := server.Config.Server.JWTSecret
exp := time.Now().Add(time.Minute * 15).Unix()
stateToken := token.New(token.OAuthStateToken)
From 211235817ff252b8e3d8aeccd79a33be8e89b0b3 Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 27 Jun 2024 13:31:25 +0200
Subject: [PATCH 14/15] review, fix
---
cmd/server/server.go | 2 +-
server/api/login.go | 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/cmd/server/server.go b/cmd/server/server.go
index 47946848b7..524b9f0af6 100644
--- a/cmd/server/server.go
+++ b/cmd/server/server.go
@@ -267,7 +267,7 @@ func run(c *cli.Context) error {
func setupEvilGlobals(c *cli.Context, s store.Store) error {
// secrets
var err error
- server.Config.JWTSecret, err = setupJWTSecret(s)
+ server.Config.Server.JWTSecret, err = setupJWTSecret(s)
if err != nil {
return fmt.Errorf("could not setup jwt secret: %w", err)
}
diff --git a/server/api/login.go b/server/api/login.go
index f0e2bd4a29..0ca2917d6e 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -37,6 +37,8 @@ import (
"go.woodpecker-ci.org/woodpecker/v2/shared/token"
)
+const stateTokenDuration = time.Minute * 5
+
func HandleAuth(c *gin.Context) {
// TODO: check if this is really needed
c.Writer.Header().Del("Content-Type")
@@ -74,7 +76,7 @@ func HandleAuth(c *gin.Context) {
} else { // only generate a state token if not a callback
var err error
jwtSecret := server.Config.Server.JWTSecret
- exp := time.Now().Add(time.Minute * 15).Unix()
+ exp := time.Now().Add(stateTokenDuration).Unix()
stateToken := token.New(token.OAuthStateToken)
stateToken.Set("forge-id", strconv.FormatInt(forgeID, 10))
state, err = stateToken.SignExpires(jwtSecret, exp)
From f6e0afb3640417f50ae45532429c87940ef918af Mon Sep 17 00:00:00 2001
From: Anbraten <6918444+anbraten@users.noreply.github.com>
Date: Thu, 27 Jun 2024 15:52:14 +0200
Subject: [PATCH 15/15] fix lint
---
server/api/login.go | 2 +-
server/api/login_test.go | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/server/api/login.go b/server/api/login.go
index 0ca2917d6e..aa883b79d4 100644
--- a/server/api/login.go
+++ b/server/api/login.go
@@ -65,7 +65,7 @@ func HandleAuth(c *gin.Context) {
forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
if isCallback { // validate the state token
- _, err := token.Parse([]token.Type{token.OAuthStateToken}, state, func(t *token.Token) (string, error) {
+ _, err := token.Parse([]token.Type{token.OAuthStateToken}, state, func(_ *token.Token) (string, error) {
return server.Config.Server.JWTSecret, nil
})
if err != nil {
diff --git a/server/api/login_test.go b/server/api/login_test.go
index 0b8bbc46d7..f76dd721dd 100644
--- a/server/api/login_test.go
+++ b/server/api/login_test.go
@@ -127,8 +127,10 @@ func TestHandleAuth(t *testing.T) {
forgeRedirectURL := ""
_forge.On("Login", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
- state := args.Get(1).(*forge_types.OAuthRequest)
- forgeRedirectURL = fmt.Sprintf("https://my-awesome-forge.com/oauth/authorize?client_id=client-id&state=%s", state.State)
+ state, ok := args.Get(1).(*forge_types.OAuthRequest)
+ if ok {
+ forgeRedirectURL = fmt.Sprintf("https://my-awesome-forge.com/oauth/authorize?client_id=client-id&state=%s", state.State)
+ }
}).Return(nil, func(context.Context, *forge_types.OAuthRequest) string {
return forgeRedirectURL
}, nil)