Cleanup of the login flow and tests (#3810)

server/api/login.go

@@ -28,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"
@@ -55,12 +56,13 @@ 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()
 	if err != nil {
-		_ = c.AbortWithError(http.StatusInternalServerError, err)
+		log.Error().Err(err).Msg("Cannot get main forge")
+		c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
 		return
 	}
-	forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
 
 	userFromForge, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{
 		Code:  c.Request.FormValue("code"),
@@ -77,14 +79,26 @@ func HandleAuth(c *gin.Context) {
 		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)
@@ -92,17 +106,6 @@ func HandleAuth(c *gin.Context) {
 			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,
@@ -123,21 +126,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,
@@ -148,19 +157,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")
+			}
 		}
 	}
 
@@ -173,17 +182,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")
@@ -200,16 +198,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 {
@@ -223,15 +233,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) {

server/api/login_test.go

@@ -0,0 +1,339 @@
+package api_test
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+	"time"
+
+	"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_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"
+)
+
+func TestHandleAuth(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+
+	g := goblin.Goblin(t)
+	g.Describe("Login", func() {
+		user := &model.User{
+			ID:            1,
+			OrgID:         1,
+			ForgeID:       1,
+			ForgeRemoteID: "remote-id-1",
+			Login:         "test",
+			Email:         "test@example.com",
+			Admin:         false,
+		}
+		org := &model.Org{
+			ID:   1,
+			Name: user.Login,
+		}
+
+		server.Config.Server.SessionExpires = time.Hour
+
+		g.It("should handle errors from the callback", func() {
+			w := httptest.NewRecorder()
+			c, _ := gin.CreateTestContext(w)
+
+			query := url.Values{}
+			query.Set("error", "invalid_scope")
+			query.Set("error_description", "The requested scope is invalid, unknown, or malformed")
+			query.Set("error_uri", "https://developer.atlassian.com/cloud/jira/platform/rest/#api-group-OAuth2-ErrorHandling")
+
+			c.Request = &http.Request{
+				Header: make(http.Header),
+				Method: http.MethodGet,
+				URL: &url.URL{
+					Scheme:   "https",
+					Path:     "/authorize",
+					RawQuery: query.Encode(),
+				},
+			}
+
+			api.HandleAuth(c)
+
+			assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+			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
+		})
+
+		g.It("should redirect to forge login page", func() {
+			_manager := mocks_services.NewManager(t)
+			_forge := mocks_forge.NewForge(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)
+			c.Request = &http.Request{
+				Header: make(http.Header),
+				URL: &url.URL{
+					Scheme: "https",
+				},
+			}
+
+			forgeRedirectURL := "https://my-awesome-forge.com/oauth/authorize?client_id=client-id"
+
+			_manager.On("ForgeMain").Return(_forge, nil)
+			_forge.On("Login", mock.Anything, mock.Anything).Return(nil, forgeRedirectURL, nil)
+
+			api.HandleAuth(c)
+
+			assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+			assert.Equal(g, forgeRedirectURL, c.Writer.Header().Get("Location"))
+		})
+
+		g.It("should register a new user", func() {
+			_manager := mocks_services.NewManager(t)
+			_forge := mocks_forge.NewForge(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)
+			c.Request = &http.Request{
+				Header: make(http.Header),
+				URL: &url.URL{
+					Scheme: "https",
+				},
+			}
+
+			_manager.On("ForgeMain").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)
+			_store.On("OrgFindByName", user.Login).Return(nil, nil)
+			_store.On("OrgCreate", mock.Anything).Return(nil)
+			_store.On("UpdateUser", mock.Anything).Return(nil)
+			_forge.On("Repos", mock.Anything, mock.Anything).Return(nil, nil)
+
+			api.HandleAuth(c)
+
+			assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+			assert.Equal(g, "/", c.Writer.Header().Get("Location"))
+			assert.NotEmpty(g, c.Writer.Header().Get("Set-Cookie"))
+		})
+
+		g.It("should login an existing user", func() {
+			_manager := mocks_services.NewManager(t)
+			_forge := mocks_forge.NewForge(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)
+			c.Request = &http.Request{
+				Header: make(http.Header),
+				URL: &url.URL{
+					Scheme: "https",
+				},
+			}
+
+			_manager.On("ForgeMain").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)
+			_store.On("UpdateUser", mock.Anything).Return(nil)
+			_forge.On("Repos", mock.Anything, mock.Anything).Return(nil, nil)
+
+			api.HandleAuth(c)
+
+			assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+			assert.Equal(g, "/", c.Writer.Header().Get("Location"))
+			assert.NotEmpty(g, c.Writer.Header().Get("Set-Cookie"))
+		})
+
+		g.It("should deny a new user if registration is closed", func() {
+			_manager := mocks_services.NewManager(t)
+			_forge := mocks_forge.NewForge(t)
+			_store := mocks_store.NewStore(t)
+			server.Config.Services.Manager = _manager
+			server.Config.Permissions.Open = false
+			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)
+			c.Request = &http.Request{
+				Header: make(http.Header),
+				URL: &url.URL{
+					Scheme: "https",
+				},
+			}
+
+			_manager.On("ForgeMain").Return(_forge, nil)
+			_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
+			_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(nil, types.RecordNotExist)
+
+			api.HandleAuth(c)
+
+			assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+			assert.Equal(g, "/login?error=registration_closed", c.Writer.Header().Get("Location"))
+		})
+
+		g.It("should deny a user with missing org access", func() {
+			_manager := mocks_services.NewManager(t)
+			_forge := mocks_forge.NewForge(t)
+			_store := mocks_store.NewStore(t)
+			server.Config.Services.Manager = _manager
+			server.Config.Permissions.Open = true
+			server.Config.Permissions.Orgs = permissions.NewOrgs([]string{"org1"})
+			server.Config.Permissions.Admins = permissions.NewAdmins(nil)
+			w := httptest.NewRecorder()
+			c, _ := gin.CreateTestContext(w)
+			c.Set("store", _store)
+			c.Request = &http.Request{
+				Header: make(http.Header),
+				URL: &url.URL{
+					Scheme: "https",
+				},
+			}
+
+			_manager.On("ForgeMain").Return(_forge, nil)
+			_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
+			_forge.On("Teams", mock.Anything, user).Return([]*model.Team{
+				{
+					Login: "org2",
+				},
+			}, nil)
+
+			api.HandleAuth(c)
+
+			assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+			assert.Equal(g, "/login?error=org_access_denied", c.Writer.Header().Get("Location"))
+		})
+
+		g.Describe("User org", func() {
+			g.It("should be created if it does not exists", func() {
+				_manager := mocks_services.NewManager(t)
+				_forge := mocks_forge.NewForge(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)
+				c.Request = &http.Request{
+					Header: make(http.Header),
+					URL: &url.URL{
+						Scheme: "https",
+					},
+				}
+				user.OrgID = 0
+
+				_manager.On("ForgeMain").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)
+				_store.On("OrgCreate", mock.Anything).Return(nil)
+				_store.On("UpdateUser", mock.Anything).Return(nil)
+				_forge.On("Repos", mock.Anything, mock.Anything).Return(nil, nil)
+
+				api.HandleAuth(c)
+
+				assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+				assert.Equal(g, "/", c.Writer.Header().Get("Location"))
+				assert.NotEmpty(g, c.Writer.Header().Get("Set-Cookie"))
+			})
+
+			g.It("should be linked if it has the same name as the user", func() {
+				_manager := mocks_services.NewManager(t)
+				_forge := mocks_forge.NewForge(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)
+				c.Request = &http.Request{
+					Header: make(http.Header),
+					URL: &url.URL{
+						Scheme: "https",
+					},
+				}
+				user.OrgID = 0
+
+				_manager.On("ForgeMain").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)
+				_store.On("OrgUpdate", mock.Anything).Return(nil)
+				_store.On("UpdateUser", mock.Anything).Return(nil)
+				_forge.On("Repos", mock.Anything, mock.Anything).Return(nil, nil)
+
+				api.HandleAuth(c)
+
+				assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+				assert.Equal(g, "/", c.Writer.Header().Get("Location"))
+				assert.NotEmpty(g, c.Writer.Header().Get("Set-Cookie"))
+			})
+
+			g.It("should be updated if the user name was changed", func() {
+				_manager := mocks_services.NewManager(t)
+				_forge := mocks_forge.NewForge(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)
+				c.Request = &http.Request{
+					Header: make(http.Header),
+					URL: &url.URL{
+						Scheme: "https",
+					},
+				}
+				org.Name = "not-the-user-name"
+
+				_manager.On("ForgeMain").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)
+				_store.On("OrgUpdate", mock.Anything).Return(nil)
+				_store.On("UpdateUser", mock.Anything).Return(nil)
+				_forge.On("Repos", mock.Anything, mock.Anything).Return(nil, nil)
+
+				api.HandleAuth(c)
+
+				assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+				assert.Equal(g, "/", c.Writer.Header().Get("Location"))
+				assert.NotEmpty(g, c.Writer.Header().Get("Set-Cookie"))
+			})
+		})
+	})
+}