From 1a39d57f71c523d9141e0a4d7c160a973ff4b2f4 Mon Sep 17 00:00:00 2001 From: Anbraten <6918444+anbraten@users.noreply.github.com> Date: Fri, 21 Jun 2024 09:55:30 +0200 Subject: [PATCH] Enhance authentication UX (#3807) --- server/api/login.go | 130 +++++++++--------- server/forge/bitbucket/bitbucket.go | 11 +- server/forge/bitbucket/bitbucket_test.go | 6 - .../bitbucketdatacenter.go | 12 +- server/forge/forgejo/forgejo.go | 11 +- server/forge/gitea/gitea.go | 11 +- server/forge/github/github.go | 14 +- server/forge/gitlab/gitlab.go | 11 +- server/forge/types/errors.go | 22 --- server/forge/types/oauth.go | 6 +- server/router/router.go | 3 +- web/src/assets/locales/en.json | 11 +- .../admin/settings/AdminInfoTab.vue | 1 + web/src/components/atomic/Error.vue | 4 +- web/src/components/atomic/Warning.vue | 4 +- web/src/compositions/useAuthentication.ts | 2 +- web/src/router.ts | 11 +- web/src/views/Login.vue | 33 +++-- 18 files changed, 116 insertions(+), 187 deletions(-) diff --git a/server/api/login.go b/server/api/login.go index c03de5db2..e610c3698 100644 --- a/server/api/login.go +++ b/server/api/login.go @@ -17,7 +17,9 @@ package api import ( "encoding/base32" "errors" + "fmt" "net/http" + "net/url" "strconv" "time" @@ -34,15 +36,24 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/shared/token" ) -func HandleLogin(c *gin.Context) { - if err := c.Request.FormValue("error"); err != "" { - c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login/error?code="+err) - } else { - c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/authorize") - } -} - func HandleAuth(c *gin.Context) { + // TODO: check if this is really needed + c.Writer.Header().Del("Content-Type") + + // redirect when getting oauth error from forge to login page + if err := c.Request.FormValue("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) _forge, err := server.Config.Services.Manager.ForgeMain() if err != nil { @@ -51,15 +62,9 @@ func HandleAuth(c *gin.Context) { } forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported - // 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") - - 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"), + userFromForge, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{ + Code: c.Request.FormValue("code"), + State: "woodpecker", // TODO: use proper state }) if err != nil { log.Error().Err(err).Msg("cannot authenticate user") @@ -67,13 +72,13 @@ func HandleAuth(c *gin.Context) { return } // The user is not authorized yet -> redirect - if tmpUser == nil { + if userFromForge == nil { http.Redirect(c.Writer, c.Request, redirectURL, http.StatusSeeOther) return } // get the user from the database - u, err := _store.GetUserRemoteID(tmpUser.ForgeRemoteID, tmpUser.Login) + user, err := _store.GetUserRemoteID(userFromForge.ForgeRemoteID, userFromForge.Login) if err != nil && !errors.Is(err, types.RecordNotExist) { _ = c.AbortWithError(http.StatusInternalServerError, err) return @@ -81,31 +86,31 @@ func HandleAuth(c *gin.Context) { if 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(tmpUser) { - log.Error().Msgf("cannot register %s. registration closed", tmpUser.Login) - c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=access_denied") + 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=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, tmpUser) + 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.", u.Login) + 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 - u = &model.User{ - Login: tmpUser.Login, - ForgeRemoteID: tmpUser.ForgeRemoteID, - Token: tmpUser.Token, - Secret: tmpUser.Secret, - Email: tmpUser.Email, - Avatar: tmpUser.Avatar, + user = &model.User{ + Login: userFromForge.Login, + ForgeRemoteID: userFromForge.ForgeRemoteID, + Token: userFromForge.Token, + Secret: userFromForge.Secret, + Email: userFromForge.Email, + Avatar: userFromForge.Avatar, ForgeID: forgeID, Hash: base32.StdEncoding.EncodeToString( securecookie.GenerateRandomKey(32), @@ -113,17 +118,17 @@ func HandleAuth(c *gin.Context) { } // insert the user into the database - if err := _store.CreateUser(u); err != nil { - log.Error().Err(err).Msgf("cannot insert %s", u.Login) + if err := _store.CreateUser(user); err != nil { + log.Error().Err(err).Msgf("cannot insert %s", user.Login) 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. - if org, err := _store.OrgFindByName(u.Login); err == nil && org != nil { + if org, err := _store.OrgFindByName(user.Login); err == nil && org != nil { org.IsUser = true - u.OrgID = org.ID + 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") } @@ -133,76 +138,76 @@ func HandleAuth(c *gin.Context) { return } org = &model.Org{ - Name: u.Login, + Name: user.Login, IsUser: true, Private: false, - ForgeID: u.ForgeID, + ForgeID: user.ForgeID, } if err := _store.OrgCreate(org); err != nil { log.Error().Err(err).Msgf("on user creation, could not create org for user") } - u.OrgID = org.ID + user.OrgID = org.ID } } // update org name - if u.Login != tmpUser.Login { - org, err := _store.OrgGet(u.OrgID) + if user.Login != userFromForge.Login { + org, err := _store.OrgGet(user.OrgID) if err != nil { - log.Error().Err(err).Msgf("cannot get org %s", u.Login) + 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 = u.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") } } // update the user meta data and authorization data. - u.Token = tmpUser.Token - u.Secret = tmpUser.Secret - u.Email = tmpUser.Email - u.Avatar = tmpUser.Avatar - u.ForgeRemoteID = tmpUser.ForgeRemoteID - u.Login = tmpUser.Login - u.Admin = u.Admin || server.Config.Permissions.Admins.IsAdmin(tmpUser) + user.Token = userFromForge.Token + user.Secret = userFromForge.Secret + user.Email = userFromForge.Email + user.Avatar = userFromForge.Avatar + user.ForgeRemoteID = userFromForge.ForgeRemoteID + 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, u) + 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", u.Login) + 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(u); err != nil { - log.Error().Err(err).Msgf("cannot update %s", u.Login) + 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") return } exp := time.Now().Add(server.Config.Server.SessionExpires).Unix() _token := token.New(token.SessToken) - _token.Set("user-id", strconv.FormatInt(u.ID, 10)) - tokenString, err := _token.SignExpires(u.Hash, exp) + _token.Set("user-id", strconv.FormatInt(user.ID, 10)) + tokenString, err := _token.SignExpires(user.Hash, exp) if err != nil { - log.Error().Msgf("cannot create token for %s", u.Login) + log.Error().Msgf("cannot create token for %s", user.Login) c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error") return } - repos, _ := _forge.Repos(c, u) + 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", u.Login) + log.Error().Err(err).Msgf("cannot list repos for %s", user.Login) c.Redirect(http.StatusSeeOther, "/login?error=internal_error") return } @@ -211,14 +216,14 @@ func HandleAuth(c *gin.Context) { continue } - log.Debug().Msgf("synced user permission for %s %s", u.Login, dbRepo.FullName) + log.Debug().Msgf("synced user permission for %s %s", user.Login, dbRepo.FullName) perm := forgeRepo.Perm perm.Repo = dbRepo perm.RepoID = dbRepo.ID - perm.UserID = u.ID + 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", u.Login) + log.Error().Err(err).Msgf("cannot update permissions for %s", user.Login) c.Redirect(http.StatusSeeOther, "/login?error=internal_error") return } @@ -235,7 +240,8 @@ func GetLogout(c *gin.Context) { c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/") } -func GetLoginToken(c *gin.Context) { +// TODO: remove in 3.0 +func DeprecatedGetLoginToken(c *gin.Context) { _store := store.FromContext(c) _forge, err := server.Config.Services.Manager.ForgeMain() // TODO: get selected forge from auth request diff --git a/server/forge/bitbucket/bitbucket.go b/server/forge/bitbucket/bitbucket.go index c87047788..2fc95a8d1 100644 --- a/server/forge/bitbucket/bitbucket.go +++ b/server/forge/bitbucket/bitbucket.go @@ -81,16 +81,7 @@ func (c *config) URL() string { // Bitbucket account details are returned when the user is successfully authenticated. 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 req.Error != "" { - return nil, redirectURL, &forge_types.AuthError{ - Err: req.Error, - Description: req.ErrorDescription, - URI: req.ErrorURI, - } - } + redirectURL := config.AuthCodeURL(req.State) // check the OAuth code if len(req.Code) == 0 { diff --git a/server/forge/bitbucket/bitbucket_test.go b/server/forge/bitbucket/bitbucket_test.go index 9cd83f7c9..4a9760819 100644 --- a/server/forge/bitbucket/bitbucket_test.go +++ b/server/forge/bitbucket/bitbucket_test.go @@ -90,12 +90,6 @@ func Test_bitbucket(t *testing.T) { }) g.Assert(err).IsNotNil() }) - g.It("Should handle authentication errors", func() { - _, _, err := c.Login(ctx, &types.OAuthRequest{ - Error: "invalid_scope", - }) - g.Assert(err).IsNotNil() - }) }) g.Describe("Given an access token", func() { diff --git a/server/forge/bitbucketdatacenter/bitbucketdatacenter.go b/server/forge/bitbucketdatacenter/bitbucketdatacenter.go index 573ee52e3..cd3b58bc4 100644 --- a/server/forge/bitbucketdatacenter/bitbucketdatacenter.go +++ b/server/forge/bitbucketdatacenter/bitbucketdatacenter.go @@ -96,16 +96,8 @@ func (c *client) URL() string { func (c *client) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { config := c.newOAuth2Config() - // TODO: Add proper state and pkce (https://oauth.net/2/pkce/) ... - redirectURL := config.AuthCodeURL("woodpecker") - - if req.Error != "" { - return nil, redirectURL, &forge_types.AuthError{ - Err: req.Error, - Description: req.ErrorDescription, - URI: req.ErrorURI, - } - } + // TODO: Use pkce flow (https://oauth.net/2/pkce/) ... + redirectURL := config.AuthCodeURL(req.State) if len(req.Code) == 0 { return nil, redirectURL, nil diff --git a/server/forge/forgejo/forgejo.go b/server/forge/forgejo/forgejo.go index 943502862..4185a04cc 100644 --- a/server/forge/forgejo/forgejo.go +++ b/server/forge/forgejo/forgejo.go @@ -111,16 +111,7 @@ func (c *Forgejo) oauth2Config(ctx context.Context) (*oauth2.Config, context.Con // Forgejo account details are returned when the user is successfully authenticated. func (c *Forgejo) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { config, oauth2Ctx := c.oauth2Config(ctx) - 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, - } - } + redirectURL := config.AuthCodeURL(req.State) // check the OAuth code if len(req.Code) == 0 { diff --git a/server/forge/gitea/gitea.go b/server/forge/gitea/gitea.go index 11e5c8892..a489e66a8 100644 --- a/server/forge/gitea/gitea.go +++ b/server/forge/gitea/gitea.go @@ -113,16 +113,7 @@ func (c *Gitea) oauth2Config(ctx context.Context) (*oauth2.Config, context.Conte // Gitea account details are returned when the user is successfully authenticated. func (c *Gitea) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { config, oauth2Ctx := c.oauth2Config(ctx) - 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, - } - } + redirectURL := config.AuthCodeURL(req.State) // check the OAuth code if len(req.Code) == 0 { diff --git a/server/forge/github/github.go b/server/forge/github/github.go index 0e62413cf..93ba8cbed 100644 --- a/server/forge/github/github.go +++ b/server/forge/github/github.go @@ -100,16 +100,7 @@ func (c *client) URL() string { // Login authenticates the session and returns the forge user details. 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, - } - } + redirectURL := config.AuthCodeURL(req.State) // check the OAuth code if len(req.Code) == 0 { @@ -662,9 +653,6 @@ func (c *client) getTagCommitSHA(ctx context.Context, repo *model.Repo, tagName } gh := c.newClientToken(ctx, user.Token) - if err != nil { - return "", err - } page := 1 var tag *github.RepositoryTag diff --git a/server/forge/gitlab/gitlab.go b/server/forge/gitlab/gitlab.go index 27cd50cb7..1929aef66 100644 --- a/server/forge/gitlab/gitlab.go +++ b/server/forge/gitlab/gitlab.go @@ -115,16 +115,7 @@ func (g *GitLab) oauth2Config(ctx context.Context) (*oauth2.Config, context.Cont // forge user details. func (g *GitLab) Login(ctx context.Context, req *forge_types.OAuthRequest) (*model.User, string, error) { config, oauth2Ctx := g.oauth2Config(ctx) - 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, - } - } + redirectURL := config.AuthCodeURL(req.State) // check the OAuth code if len(req.Code) == 0 { diff --git a/server/forge/types/errors.go b/server/forge/types/errors.go index 190f036b2..b155f6021 100644 --- a/server/forge/types/errors.go +++ b/server/forge/types/errors.go @@ -21,28 +21,6 @@ import ( "strings" ) -// AuthError represents forge authentication error. -type AuthError struct { - Err string - Description string - URI string -} - -// Error implements error interface. -func (ae *AuthError) Error() string { - err := ae.Err - if ae.Description != "" { - err += " " + ae.Description - } - if ae.URI != "" { - err += " " + ae.URI - } - return err -} - -// Check interface implementation at compile time. -var _ error = new(AuthError) - var ErrNotImplemented = errors.New("not implemented") type ErrIgnoreEvent struct { diff --git a/server/forge/types/oauth.go b/server/forge/types/oauth.go index fba870569..0bc9aee3f 100644 --- a/server/forge/types/oauth.go +++ b/server/forge/types/oauth.go @@ -15,8 +15,6 @@ package types type OAuthRequest struct { - Error string - ErrorURI string - ErrorDescription string - Code string + Code string + State string } diff --git a/server/router/router.go b/server/router/router.go index 317f0975d..89e1046f9 100644 --- a/server/router/router.go +++ b/server/router/router.go @@ -58,12 +58,11 @@ func Load(noRouteHandler http.HandlerFunc, middleware ...gin.HandlerFunc) http.H base.GET("/web-config.js", web.Config) base.GET("/logout", api.GetLogout) - base.GET("/login", api.HandleLogin) auth := base.Group("/authorize") { auth.GET("", api.HandleAuth) auth.POST("", api.HandleAuth) - auth.POST("/token", api.GetLoginToken) + auth.POST("/token", api.DeprecatedGetLoginToken) } base.GET("/metrics", metrics.PromHandler()) diff --git a/web/src/assets/locales/en.json b/web/src/assets/locales/en.json index a6586152e..41ba7dd17 100644 --- a/web/src/assets/locales/en.json +++ b/web/src/assets/locales/en.json @@ -409,10 +409,7 @@ "reset_token": "Reset token", "swagger_ui": "Swagger UI" } - }, - "oauth_error": "Error while authenticating against OAuth provider", - "internal_error": "Some internal error occurred", - "access_denied": "You are not allowed to login" + } }, "secrets": { "secrets": "Secrets", @@ -451,5 +448,9 @@ "cli_login_failed": "Login to CLI failed", "cli_login_denied": "Login to CLI denied", "return_to_cli": "You can now close this tab and return to the CLI.", - "settings": "Settings" + "settings": "Settings", + "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" } diff --git a/web/src/components/admin/settings/AdminInfoTab.vue b/web/src/components/admin/settings/AdminInfoTab.vue index 8f9182361..da965f7ee 100644 --- a/web/src/components/admin/settings/AdminInfoTab.vue +++ b/web/src/components/admin/settings/AdminInfoTab.vue @@ -29,6 +29,7 @@ diff --git a/web/src/components/atomic/Warning.vue b/web/src/components/atomic/Warning.vue index 418d8b525..f30885fac 100644 --- a/web/src/components/atomic/Warning.vue +++ b/web/src/components/atomic/Warning.vue @@ -4,7 +4,7 @@ > - {{ text }} + {{ text }} @@ -12,6 +12,6 @@ diff --git a/web/src/compositions/useAuthentication.ts b/web/src/compositions/useAuthentication.ts index bd7b6d0b1..d8bcb21fd 100644 --- a/web/src/compositions/useAuthentication.ts +++ b/web/src/compositions/useAuthentication.ts @@ -12,6 +12,6 @@ export default () => const config = useUserConfig(); config.setUserConfig('redirectUrl', url); } - window.location.href = `${useConfig().rootPath}/login`; + window.location.href = `${useConfig().rootPath}/authorize`; }, }) as const; diff --git a/web/src/router.ts b/web/src/router.ts index 21f97afac..441a0a30b 100644 --- a/web/src/router.ts +++ b/web/src/router.ts @@ -153,14 +153,7 @@ const routes: RouteRecordRaw[] = [ props: true, }, { - path: `${rootPath}/login/error`, - name: 'login-error', - component: (): Component => import('~/views/Login.vue'), - meta: { blank: true }, - props: true, - }, - { - path: `${rootPath}/do-login`, + path: `${rootPath}/login`, name: 'login', component: (): Component => import('~/views/Login.vue'), meta: { blank: true }, @@ -199,7 +192,7 @@ const router = createRouter({ router.beforeEach(async (to, _, next) => { const config = useUserConfig(); const { redirectUrl } = config.userConfig.value; - if (redirectUrl !== '') { + if (redirectUrl !== '' && to.name !== 'login') { config.setUserConfig('redirectUrl', ''); next(redirectUrl); } diff --git a/web/src/views/Login.vue b/web/src/views/Login.vue index d6fcec0b9..65b25e769 100644 --- a/web/src/views/Login.vue +++ b/web/src/views/Login.vue @@ -1,7 +1,17 @@