diff --git a/server/api/login.go b/server/api/login.go index 2a674524ce..3dcdbbfa58 100644 --- a/server/api/login.go +++ b/server/api/login.go @@ -25,6 +25,7 @@ import ( "github.com/rs/zerolog/log" "go.woodpecker-ci.org/woodpecker/v2/server" + 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" "go.woodpecker-ci.org/woodpecker/v2/server/store/types" @@ -48,15 +49,20 @@ func HandleAuth(c *gin.Context) { // cannot, however, remember why, so need to revisit this line. c.Writer.Header().Del("Content-Type") - tmpuser, err := _forge.Login(c, c.Writer, c.Request) + tmpuser, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{ + Error: c.Request.FormValue("error"), + ErrorURI: c.Request.FormValue("error_uri"), + ErrorDescription: c.Request.FormValue("error_description"), + Code: c.Request.FormValue("code"), + }) if err != nil { log.Error().Err(err).Msg("cannot authenticate user") c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=oauth_error") return } - // this will happen when the forge redirects the user as - // part of the authorization workflow. + // The user is not authorized yet -> redirect if tmpuser == nil { + http.Redirect(c.Writer, c.Request, redirectURL, http.StatusSeeOther) return } diff --git a/server/forge/bitbucket/bitbucket.go b/server/forge/bitbucket/bitbucket.go index 13b0d43e52..095836be90 100644 --- a/server/forge/bitbucket/bitbucket.go +++ b/server/forge/bitbucket/bitbucket.go @@ -77,36 +77,35 @@ func (c *config) URL() string { // Login authenticates an account with Bitbucket using the oauth2 protocol. The // Bitbucket account details are returned when the user is successfully authenticated. -func (c *config) Login(ctx context.Context, w http.ResponseWriter, req *http.Request) (*model.User, error) { +func (c *config) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { config := c.newOAuth2Config() + redirectURL := config.AuthCodeURL("woodpecker") // get the OAuth errors - if err := req.FormValue("error"); err != "" { - return nil, &forge_types.AuthError{ - Err: err, - Description: req.FormValue("error_description"), - URI: req.FormValue("error_uri"), + if req.Error != "" { + return nil, redirectURL, &forge_types.AuthError{ + Err: req.Error, + Description: req.ErrorDescription, + URI: req.ErrorURI, } } - // get the OAuth code - code := req.FormValue("code") - if len(code) == 0 { - http.Redirect(w, req, config.AuthCodeURL("woodpecker"), http.StatusSeeOther) - return nil, nil + // check the OAuth code + if len(req.Code) == 0 { + return nil, redirectURL, nil } - token, err := config.Exchange(ctx, code) + token, err := config.Exchange(ctx, req.Code) if err != nil { - return nil, err + return nil, redirectURL, err } client := internal.NewClient(ctx, c.API, config.Client(ctx, token)) curr, err := client.FindCurrent() if err != nil { - return nil, err + return nil, redirectURL, err } - return convertUser(curr, token), nil + return convertUser(curr, token), redirectURL, nil } // Auth uses the Bitbucket oauth2 access token and refresh token to authenticate diff --git a/server/forge/bitbucket/bitbucket_test.go b/server/forge/bitbucket/bitbucket_test.go index 8488cc6aa2..0f3823e681 100644 --- a/server/forge/bitbucket/bitbucket_test.go +++ b/server/forge/bitbucket/bitbucket_test.go @@ -27,6 +27,7 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/server/forge/bitbucket/fixtures" "go.woodpecker-ci.org/woodpecker/v2/server/forge/bitbucket/internal" + "go.woodpecker-ci.org/woodpecker/v2/server/forge/types" "go.woodpecker-ci.org/woodpecker/v2/server/model" ) @@ -63,34 +64,35 @@ func Test_bitbucket(t *testing.T) { g.Describe("Given an authorization request", func() { g.It("Should redirect to authorize", func() { - w := httptest.NewRecorder() - r, _ := http.NewRequest("GET", "", nil) - _, err := c.Login(ctx, w, r) + user, _, err := c.Login(ctx, &types.OAuthRequest{}) g.Assert(err).IsNil() - g.Assert(w.Code).Equal(http.StatusSeeOther) + g.Assert(user).IsNil() }) g.It("Should return authenticated user", func() { - r, _ := http.NewRequest("GET", "?code=code", nil) - u, err := c.Login(ctx, nil, r) + u, _, err := c.Login(ctx, &types.OAuthRequest{ + Code: "code", + }) g.Assert(err).IsNil() g.Assert(u.Login).Equal(fakeUser.Login) g.Assert(u.Token).Equal("2YotnFZFEjr1zCsicMWpAA") g.Assert(u.Secret).Equal("tGzv3JOkF0XG5Qx2TlKWIA") }) g.It("Should handle failure to exchange code", func() { - w := httptest.NewRecorder() - r, _ := http.NewRequest("GET", "?code=code_bad_request", nil) - _, err := c.Login(ctx, w, r) + _, _, err := c.Login(ctx, &types.OAuthRequest{ + Code: "code_bad_request", + }) g.Assert(err).IsNotNil() }) g.It("Should handle failure to resolve user", func() { - r, _ := http.NewRequest("GET", "?code=code_user_not_found", nil) - _, err := c.Login(ctx, nil, r) + _, _, err := c.Login(ctx, &types.OAuthRequest{ + Code: "code_user_not_found", + }) g.Assert(err).IsNotNil() }) g.It("Should handle authentication errors", func() { - r, _ := http.NewRequest("GET", "?error=invalid_scope", nil) - _, err := c.Login(ctx, nil, r) + _, _, err := c.Login(ctx, &types.OAuthRequest{ + Error: "invalid_scope", + }) g.Assert(err).IsNotNil() }) }) diff --git a/server/forge/forge.go b/server/forge/forge.go index a6fb65e3b6..a823bc422f 100644 --- a/server/forge/forge.go +++ b/server/forge/forge.go @@ -36,8 +36,8 @@ type Forge interface { URL() string // Login authenticates the session and returns the - // forge user details. - Login(ctx context.Context, w http.ResponseWriter, r *http.Request) (*model.User, error) + // forge user details and the URL to redirect to if not authorized yet. + Login(ctx context.Context, r *types.OAuthRequest) (*model.User, string, error) // Auth authenticates the session and returns the forge user // login for the given token and secret diff --git a/server/forge/gitea/gitea.go b/server/forge/gitea/gitea.go index f378e97838..59907e9480 100644 --- a/server/forge/gitea/gitea.go +++ b/server/forge/gitea/gitea.go @@ -116,37 +116,36 @@ func (c *Gitea) oauth2Config(ctx context.Context) (*oauth2.Config, context.Conte // Login authenticates an account with Gitea using basic authentication. The // Gitea account details are returned when the user is successfully authenticated. -func (c *Gitea) Login(ctx context.Context, w http.ResponseWriter, req *http.Request) (*model.User, error) { +func (c *Gitea) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { config, oauth2Ctx := c.oauth2Config(ctx) - - // get the OAuth errors - if err := req.FormValue("error"); err != "" { - return nil, &forge_types.AuthError{ - Err: err, - Description: req.FormValue("error_description"), - URI: req.FormValue("error_uri"), + redirectURL := config.AuthCodeURL("woodpecker") + + // check the OAuth errors + if req.Error != "" { + return nil, redirectURL, &forge_types.AuthError{ + Err: req.Error, + Description: req.ErrorDescription, + URI: req.ErrorURI, } } - // get the OAuth code - code := req.FormValue("code") - if len(code) == 0 { - http.Redirect(w, req, config.AuthCodeURL("woodpecker"), http.StatusSeeOther) - return nil, nil + // check the OAuth code + if len(req.Code) == 0 { + return nil, redirectURL, nil } - token, err := config.Exchange(oauth2Ctx, code) + token, err := config.Exchange(oauth2Ctx, req.Code) if err != nil { - return nil, err + return nil, redirectURL, err } client, err := c.newClientToken(ctx, token.AccessToken) if err != nil { - return nil, err + return nil, redirectURL, err } account, _, err := client.GetMyUserInfo() if err != nil { - return nil, err + return nil, redirectURL, err } return &model.User{ @@ -157,7 +156,7 @@ func (c *Gitea) Login(ctx context.Context, w http.ResponseWriter, req *http.Requ Email: account.Email, ForgeRemoteID: model.ForgeRemoteID(fmt.Sprint(account.ID)), Avatar: expandAvatar(c.url, account.AvatarURL), - }, nil + }, redirectURL, nil } // Auth uses the Gitea oauth2 access token and refresh token to authenticate diff --git a/server/forge/github/github.go b/server/forge/github/github.go index a05e6b3ab9..751202dc91 100644 --- a/server/forge/github/github.go +++ b/server/forge/github/github.go @@ -92,46 +92,45 @@ func (c *client) URL() string { } // Login authenticates the session and returns the forge user details. -func (c *client) Login(ctx context.Context, res http.ResponseWriter, req *http.Request) (*model.User, error) { - config := c.newConfig(req) - - // get the OAuth errors - if err := req.FormValue("error"); err != "" { - return nil, &forge_types.AuthError{ - Err: err, - Description: req.FormValue("error_description"), - URI: req.FormValue("error_uri"), +func (c *client) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { + config := c.newConfig() + redirectURL := config.AuthCodeURL("woodpecker") + + // check the OAuth errors + if req.Error != "" { + return nil, redirectURL, &forge_types.AuthError{ + Err: req.Error, + Description: req.ErrorDescription, + URI: req.ErrorURI, } } - // get the OAuth code - code := req.FormValue("code") - if len(code) == 0 { + // check the OAuth code + if len(req.Code) == 0 { // TODO(bradrydzewski) we really should be using a random value here and // storing in a cookie for verification in the next stage of the workflow. - http.Redirect(res, req, config.AuthCodeURL("woodpecker"), http.StatusSeeOther) - return nil, nil + return nil, redirectURL, nil } - token, err := config.Exchange(c.newContext(ctx), code) + token, err := config.Exchange(c.newContext(ctx), req.Code) if err != nil { - return nil, err + return nil, redirectURL, err } client := c.newClientToken(ctx, token.AccessToken) user, _, err := client.Users.Get(ctx, "") if err != nil { - return nil, err + return nil, redirectURL, err } emails, _, err := client.Users.ListEmails(ctx, nil) if err != nil { - return nil, err + return nil, redirectURL, err } email := matchingEmail(emails, c.API) if email == nil { - return nil, fmt.Errorf("no verified Email address for GitHub account") + return nil, redirectURL, fmt.Errorf("no verified Email address for GitHub account") } return &model.User{ @@ -140,7 +139,7 @@ func (c *client) Login(ctx context.Context, res http.ResponseWriter, req *http.R Token: token.AccessToken, Avatar: user.GetAvatarURL(), ForgeRemoteID: model.ForgeRemoteID(fmt.Sprint(user.GetID())), - }, nil + }, redirectURL, nil } // Auth returns the GitHub user login for the given access token. @@ -405,16 +404,7 @@ func (c *client) newContext(ctx context.Context) context.Context { } // helper function to return the GitHub oauth2 config -func (c *client) newConfig(req *http.Request) *oauth2.Config { - var redirect string - - intendedURL := req.URL.Query()["url"] - if len(intendedURL) > 0 { - redirect = fmt.Sprintf("%s/authorize?url=%s", server.Config.Server.OAuthHost, intendedURL[0]) - } else { - redirect = fmt.Sprintf("%s/authorize", server.Config.Server.OAuthHost) - } - +func (c *client) newConfig() *oauth2.Config { return &oauth2.Config{ ClientID: c.Client, ClientSecret: c.Secret, @@ -423,7 +413,7 @@ func (c *client) newConfig(req *http.Request) *oauth2.Config { AuthURL: fmt.Sprintf("%s/login/oauth/authorize", c.url), TokenURL: fmt.Sprintf("%s/login/oauth/access_token", c.url), }, - RedirectURL: redirect, + RedirectURL: fmt.Sprintf("%s/authorize", server.Config.Server.OAuthHost), } } diff --git a/server/forge/gitlab/gitlab.go b/server/forge/gitlab/gitlab.go index 8a59c08b1f..008d037a19 100644 --- a/server/forge/gitlab/gitlab.go +++ b/server/forge/gitlab/gitlab.go @@ -105,38 +105,37 @@ func (g *GitLab) oauth2Config(ctx context.Context) (*oauth2.Config, context.Cont // Login authenticates the session and returns the // forge user details. -func (g *GitLab) Login(ctx context.Context, res http.ResponseWriter, req *http.Request) (*model.User, error) { +func (g *GitLab) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { config, oauth2Ctx := g.oauth2Config(ctx) - - // get the OAuth errors - if err := req.FormValue("error"); err != "" { - return nil, &forge_types.AuthError{ - Err: err, - Description: req.FormValue("error_description"), - URI: req.FormValue("error_uri"), + redirectURL := config.AuthCodeURL("woodpecker") + + // check the OAuth errors + if req.Error != "" { + return nil, redirectURL, &forge_types.AuthError{ + Err: req.Error, + Description: req.ErrorDescription, + URI: req.ErrorURI, } } - // get the OAuth code - code := req.FormValue("code") - if len(code) == 0 { - http.Redirect(res, req, config.AuthCodeURL("woodpecker"), http.StatusSeeOther) - return nil, nil + // check the OAuth code + if len(req.Code) == 0 { + return nil, redirectURL, nil } - token, err := config.Exchange(oauth2Ctx, code) + token, err := config.Exchange(oauth2Ctx, req.Code) if err != nil { - return nil, fmt.Errorf("error exchanging token: %w", err) + return nil, redirectURL, fmt.Errorf("error exchanging token: %w", err) } client, err := newClient(g.url, token.AccessToken, g.SkipVerify) if err != nil { - return nil, err + return nil, redirectURL, err } login, _, err := client.Users.CurrentUser(gitlab.WithContext(ctx)) if err != nil { - return nil, err + return nil, redirectURL, err } user := &model.User{ @@ -151,7 +150,7 @@ func (g *GitLab) Login(ctx context.Context, res http.ResponseWriter, req *http.R user.Avatar = g.url + "/" + login.AvatarURL } - return user, nil + return user, redirectURL, nil } // Refresh refreshes the Gitlab oauth2 access token. If the token is diff --git a/server/forge/mocks/forge.go b/server/forge/mocks/forge.go index 5d46271307..33e293ac43 100644 --- a/server/forge/mocks/forge.go +++ b/server/forge/mocks/forge.go @@ -242,34 +242,41 @@ func (_m *Forge) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model return r0, r1, r2 } -// Login provides a mock function with given fields: ctx, w, r -func (_m *Forge) Login(ctx context.Context, w http.ResponseWriter, r *http.Request) (*model.User, error) { - ret := _m.Called(ctx, w, r) +// Login provides a mock function with given fields: ctx, r +func (_m *Forge) Login(ctx context.Context, r *types.OAuthRequest) (*model.User, string, error) { + ret := _m.Called(ctx, r) if len(ret) == 0 { panic("no return value specified for Login") } var r0 *model.User - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, http.ResponseWriter, *http.Request) (*model.User, error)); ok { - return rf(ctx, w, r) + var r1 string + var r2 error + if rf, ok := ret.Get(0).(func(context.Context, *types.OAuthRequest) (*model.User, string, error)); ok { + return rf(ctx, r) } - if rf, ok := ret.Get(0).(func(context.Context, http.ResponseWriter, *http.Request) *model.User); ok { - r0 = rf(ctx, w, r) + if rf, ok := ret.Get(0).(func(context.Context, *types.OAuthRequest) *model.User); ok { + r0 = rf(ctx, r) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.User) } } - if rf, ok := ret.Get(1).(func(context.Context, http.ResponseWriter, *http.Request) error); ok { - r1 = rf(ctx, w, r) + if rf, ok := ret.Get(1).(func(context.Context, *types.OAuthRequest) string); ok { + r1 = rf(ctx, r) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(string) } - return r0, r1 + if rf, ok := ret.Get(2).(func(context.Context, *types.OAuthRequest) error); ok { + r2 = rf(ctx, r) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 } // Name provides a mock function with given fields: diff --git a/server/forge/types/oauth.go b/server/forge/types/oauth.go new file mode 100644 index 0000000000..fba870569c --- /dev/null +++ b/server/forge/types/oauth.go @@ -0,0 +1,22 @@ +// Copyright 2024 Woodpecker Authors +// +// Licensed 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 types + +type OAuthRequest struct { + Error string + ErrorURI string + ErrorDescription string + Code string +}