diff --git a/server/api/login.go b/server/api/login.go index e610c3698..0cabbe887 100644 --- a/server/api/login.go +++ b/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) { diff --git a/server/api/login_test.go b/server/api/login_test.go new file mode 100644 index 000000000..5de274509 --- /dev/null +++ b/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")) + }) + }) + }) +}