Refactor some LDAP code (#32849)

services/auth/source/ldap/source.go

@@ -56,8 +56,7 @@ type Source struct {
 	UserUID               string // User Attribute listed in Group
 	SkipLocalTwoFA        bool   `json:",omitempty"` // Skip Local 2fa for users authenticated with this source
 
-	// reference to the authSource
+	authSource *auth.Source // reference to the authSource
-	authSource *auth.Source
 }
 
 // FromDB fills up a LDAPConfig from serialized format.
@@ -107,7 +106,7 @@ func (source *Source) UseTLS() bool {
 
 // ProvidesSSHKeys returns if this source provides SSH Keys
 func (source *Source) ProvidesSSHKeys() bool {
-	return len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0
+	return strings.TrimSpace(source.AttributeSSHPublicKey) != ""
 }
 
 // SetAuthSource sets the related AuthSource

services/auth/source/ldap/source_authenticate.go

@@ -31,13 +31,13 @@ func (source *Source) Authenticate(ctx context.Context, user *user_model.User, u
 		return nil, user_model.ErrUserNotExist{Name: loginName}
 	}
 	// Fallback.
-	if len(sr.Username) == 0 {
+	if sr.Username == "" {
 		sr.Username = userName
 	}
-	if len(sr.Mail) == 0 {
+	if sr.Mail == "" {
 		sr.Mail = fmt.Sprintf("%s@localhost.local", sr.Username)
 	}
-	isAttributeSSHPublicKeySet := len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0
+	isAttributeSSHPublicKeySet := strings.TrimSpace(source.AttributeSSHPublicKey) != ""
 
 	// Update User admin flag if exist
 	if isExist, err := user_model.IsUserExist(ctx, 0, sr.Username); err != nil {
@@ -51,11 +51,11 @@ func (source *Source) Authenticate(ctx context.Context, user *user_model.User, u
 		}
 		if user != nil && !user.ProhibitLogin {
 			opts := &user_service.UpdateOptions{}
-			if len(source.AdminFilter) > 0 && user.IsAdmin != sr.IsAdmin {
+			if source.AdminFilter != "" && user.IsAdmin != sr.IsAdmin {
 				// Change existing admin flag only if AdminFilter option is set
 				opts.IsAdmin = optional.Some(sr.IsAdmin)
 			}
-			if !sr.IsAdmin && len(source.RestrictedFilter) > 0 && user.IsRestricted != sr.IsRestricted {
+			if !sr.IsAdmin && source.RestrictedFilter != "" && user.IsRestricted != sr.IsRestricted {
 				// Change existing restricted flag only if RestrictedFilter option is set
 				opts.IsRestricted = optional.Some(sr.IsRestricted)
 			}
@@ -99,7 +99,7 @@ func (source *Source) Authenticate(ctx context.Context, user *user_model.User, u
 				return user, err
 			}
 		}
-		if len(source.AttributeAvatar) > 0 {
+		if source.AttributeAvatar != "" {
 			if err := user_service.UploadAvatar(ctx, user, sr.Avatar); err != nil {
 				return user, err
 			}

services/auth/source/ldap/source_search.go

@@ -147,7 +147,7 @@ func bindUser(l *ldap.Conn, userDN, passwd string) error {
 }
 
 func checkAdmin(l *ldap.Conn, ls *Source, userDN string) bool {
-	if len(ls.AdminFilter) == 0 {
+	if ls.AdminFilter == "" {
 		return false
 	}
 	log.Trace("Checking admin with filter %s and base %s", ls.AdminFilter, userDN)
@@ -169,7 +169,7 @@ func checkAdmin(l *ldap.Conn, ls *Source, userDN string) bool {
 }
 
 func checkRestricted(l *ldap.Conn, ls *Source, userDN string) bool {
-	if len(ls.RestrictedFilter) == 0 {
+	if ls.RestrictedFilter == "" {
 		return false
 	}
 	if ls.RestrictedFilter == "*" {
@@ -250,8 +250,17 @@ func (source *Source) getUserAttributeListedInGroup(entry *ldap.Entry) string {
 
 // SearchEntry : search an LDAP source if an entry (name, passwd) is valid and in the specific filter
 func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchResult {
+	if MockedSearchEntry != nil {
+		return MockedSearchEntry(source, name, passwd, directBind)
+	}
+	return realSearchEntry(source, name, passwd, directBind)
+}
+
+var MockedSearchEntry func(source *Source, name, passwd string, directBind bool) *SearchResult
+
+func realSearchEntry(source *Source, name, passwd string, directBind bool) *SearchResult {
 	// See https://tools.ietf.org/search/rfc4513#section-5.1.2
-	if len(passwd) == 0 {
+	if passwd == "" {
 		log.Debug("Auth. failed for %s, password cannot be empty", name)
 		return nil
 	}
@@ -323,17 +332,17 @@ func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchR
 		return nil
 	}
 
-	isAttributeSSHPublicKeySet := len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0
+	isAttributeSSHPublicKeySet := strings.TrimSpace(source.AttributeSSHPublicKey) != ""
-	isAtributeAvatarSet := len(strings.TrimSpace(source.AttributeAvatar)) > 0
+	isAttributeAvatarSet := strings.TrimSpace(source.AttributeAvatar) != ""
 
 	attribs := []string{source.AttributeUsername, source.AttributeName, source.AttributeSurname, source.AttributeMail}
-	if len(strings.TrimSpace(source.UserUID)) > 0 {
+	if strings.TrimSpace(source.UserUID) != "" {
 		attribs = append(attribs, source.UserUID)
 	}
 	if isAttributeSSHPublicKeySet {
 		attribs = append(attribs, source.AttributeSSHPublicKey)
 	}
-	if isAtributeAvatarSet {
+	if isAttributeAvatarSet {
 		attribs = append(attribs, source.AttributeAvatar)
 	}
 
@@ -375,7 +384,7 @@ func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchR
 		isRestricted = checkRestricted(l, source, userDN)
 	}
 
-	if isAtributeAvatarSet {
+	if isAttributeAvatarSet {
 		Avatar = sr.Entries[0].GetRawAttributeValue(source.AttributeAvatar)
 	}
 
@@ -440,14 +449,14 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) {
 
 	userFilter := fmt.Sprintf(source.Filter, "*")
 
-	isAttributeSSHPublicKeySet := len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0
+	isAttributeSSHPublicKeySet := strings.TrimSpace(source.AttributeSSHPublicKey) != ""
-	isAtributeAvatarSet := len(strings.TrimSpace(source.AttributeAvatar)) > 0
+	isAttributeAvatarSet := strings.TrimSpace(source.AttributeAvatar) != ""
 
 	attribs := []string{source.AttributeUsername, source.AttributeName, source.AttributeSurname, source.AttributeMail, source.UserUID}
 	if isAttributeSSHPublicKeySet {
 		attribs = append(attribs, source.AttributeSSHPublicKey)
 	}
-	if isAtributeAvatarSet {
+	if isAttributeAvatarSet {
 		attribs = append(attribs, source.AttributeAvatar)
 	}
 
@@ -503,7 +512,7 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) {
 			user.SSHPublicKey = v.GetAttributeValues(source.AttributeSSHPublicKey)
 		}
 
-		if isAtributeAvatarSet {
+		if isAttributeAvatarSet {
 			user.Avatar = v.GetRawAttributeValue(source.AttributeAvatar)
 		}
 

services/auth/source/ldap/source_sync.go

@@ -25,7 +25,7 @@ import (
 func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 	log.Trace("Doing: SyncExternalUsers[%s]", source.authSource.Name)
 
-	isAttributeSSHPublicKeySet := len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0
+	isAttributeSSHPublicKeySet := strings.TrimSpace(source.AttributeSSHPublicKey) != ""
 	var sshKeysNeedUpdate bool
 
 	// Find all users with this login type - FIXME: Should this be an iterator?
@@ -86,26 +86,26 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 			return db.ErrCancelledf("During update of %s before completed update of users", source.authSource.Name)
 		default:
 		}
-		if len(su.Username) == 0 && len(su.Mail) == 0 {
+		if su.Username == "" && su.Mail == "" {
 			continue
 		}
 
 		var usr *user_model.User
-		if len(su.Username) > 0 {
+		if su.Username != "" {
 			usr = usernameUsers[su.LowerName]
 		}
-		if usr == nil && len(su.Mail) > 0 {
+		if usr == nil && su.Mail != "" {
 			usr = mailUsers[strings.ToLower(su.Mail)]
 		}
 
 		if usr != nil {
 			keepActiveUsers.Add(usr.ID)
-		} else if len(su.Username) == 0 {
+		} else if su.Username == "" {
 			// we cannot create the user if su.Username is empty
 			continue
 		}
 
-		if len(su.Mail) == 0 {
+		if su.Mail == "" {
 			su.Mail = fmt.Sprintf("%s@localhost.local", su.Username)
 		}
 
@@ -141,7 +141,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 				}
 			}
 
-			if err == nil && len(source.AttributeAvatar) > 0 {
+			if err == nil && source.AttributeAvatar != "" {
 				_ = user_service.UploadAvatar(ctx, usr, su.Avatar)
 			}
 		} else if updateExisting {
@@ -151,8 +151,8 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 			}
 
 			// Check if user data has changed
-			if (len(source.AdminFilter) > 0 && usr.IsAdmin != su.IsAdmin) ||
+			if (source.AdminFilter != "" && usr.IsAdmin != su.IsAdmin) ||
-				(len(source.RestrictedFilter) > 0 && usr.IsRestricted != su.IsRestricted) ||
+				(source.RestrictedFilter != "" && usr.IsRestricted != su.IsRestricted) ||
 				!strings.EqualFold(usr.Email, su.Mail) ||
 				usr.FullName != fullName ||
 				!usr.IsActive {
@@ -180,7 +180,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 			}
 
 			if usr.IsUploadAvatarChanged(su.Avatar) {
-				if err == nil && len(source.AttributeAvatar) > 0 {
+				if err == nil && source.AttributeAvatar != "" {
 					_ = user_service.UploadAvatar(ctx, usr, su.Avatar)
 				}
 			}

services/auth/source/ldap/util.go

@@ -6,11 +6,11 @@ package ldap
 // composeFullName composes a firstname surname or username
 func composeFullName(firstname, surname, username string) string {
 	switch {
-	case len(firstname) == 0 && len(surname) == 0:
+	case firstname == "" && surname == "":
 		return username
-	case len(firstname) == 0:
+	case firstname == "":
 		return surname
-	case len(surname) == 0:
+	case surname == "":
 		return firstname
 	default:
 		return firstname + " " + surname

tests/integration/auth_ldap_test.go

@@ -15,13 +15,17 @@ import (
 	"code.gitea.io/gitea/models/organization"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/optional"
+	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/modules/translation"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/auth"
 	"code.gitea.io/gitea/services/auth/source/ldap"
 	org_service "code.gitea.io/gitea/services/org"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 type ldapUser struct {
@@ -35,7 +39,20 @@ type ldapUser struct {
 	SSHKeys      []string
 }
 
-var gitLDAPUsers = []ldapUser{
+type ldapTestEnv struct {
+	gitLDAPUsers   []ldapUser
+	otherLDAPUsers []ldapUser
+	serverHost     string
+	serverPort     string
+}
+
+func prepareLdapTestEnv(t *testing.T) *ldapTestEnv {
+	if os.Getenv("TEST_LDAP") != "1" {
+		t.Skip()
+		return nil
+	}
+
+	gitLDAPUsers := []ldapUser{
 		{
 			UserName:    "professor",
 			Password:    "professor",
@@ -77,7 +94,7 @@ var gitLDAPUsers = []ldapUser{
 		},
 	}
 
-var otherLDAPUsers = []ldapUser{
+	otherLDAPUsers := []ldapUser{
 		{
 			UserName: "zoidberg",
 			Password: "zoidberg",
@@ -92,30 +109,27 @@ var otherLDAPUsers = []ldapUser{
 		},
 	}
 
-func skipLDAPTests() bool {
+	return &ldapTestEnv{
-	return os.Getenv("TEST_LDAP") != "1"
+		gitLDAPUsers:   gitLDAPUsers,
+		otherLDAPUsers: otherLDAPUsers,
+		serverHost:     util.IfZero(os.Getenv("TEST_LDAP_HOST"), "ldap"),
+		serverPort:     util.IfZero(os.Getenv("TEST_LDAP_PORT"), "389"),
+	}
 }
 
-func getLDAPServerHost() string {
+type ldapAuthOptions struct {
-	host := os.Getenv("TEST_LDAP_HOST")
+	attributeUID          optional.Option[string] // defaults to "uid"
-	if len(host) == 0 {
+	attributeSSHPublicKey string
-		host = "ldap"
+	groupFilter           string
-	}
+	groupTeamMap          string
-	return host
+	groupTeamMapRemoval   string
 }
 
-func getLDAPServerPort() string {
+func (te *ldapTestEnv) buildAuthSourcePayload(csrf string, opts ...ldapAuthOptions) map[string]string {
-	port := os.Getenv("TEST_LDAP_PORT")
+	opt := util.OptionalArg(opts)
-	if len(port) == 0 {
-		port = "389"
-	}
-	return port
-}
-
-func buildAuthSourceLDAPPayload(csrf, sshKeyAttribute, groupFilter, groupTeamMap, groupTeamMapRemoval string) map[string]string {
 	// Modify user filter to test group filter explicitly
 	userFilter := "(&(objectClass=inetOrgPerson)(memberOf=cn=git,ou=people,dc=planetexpress,dc=com)(uid=%s))"
-	if groupFilter != "" {
+	if opt.groupFilter != "" {
 		userFilter = "(&(objectClass=inetOrgPerson)(uid=%s))"
 	}
 
@@ -123,53 +137,47 @@ func buildAuthSourceLDAPPayload(csrf, sshKeyAttribute, groupFilter, groupTeamMap
 		"_csrf":                    csrf,
 		"type":                     "2",
 		"name":                     "ldap",
-		"host":                     getLDAPServerHost(),
+		"host":                     te.serverHost,
-		"port":                     getLDAPServerPort(),
+		"port":                     te.serverPort,
 		"bind_dn":                  "uid=gitea,ou=service,dc=planetexpress,dc=com",
 		"bind_password":            "password",
 		"user_base":                "ou=people,dc=planetexpress,dc=com",
 		"filter":                   userFilter,
 		"admin_filter":             "(memberOf=cn=admin_staff,ou=people,dc=planetexpress,dc=com)",
 		"restricted_filter":        "(uid=leela)",
-		"attribute_username":       "uid",
+		"attribute_username":       util.Iif(opt.attributeUID.Has(), opt.attributeUID.Value(), "uid"),
 		"attribute_name":           "givenName",
 		"attribute_surname":        "sn",
 		"attribute_mail":           "mail",
-		"attribute_ssh_public_key": sshKeyAttribute,
+		"attribute_ssh_public_key": opt.attributeSSHPublicKey,
 		"is_sync_enabled":          "on",
 		"is_active":                "on",
 		"groups_enabled":           "on",
 		"group_dn":                 "ou=people,dc=planetexpress,dc=com",
 		"group_member_uid":         "member",
-		"group_filter":             groupFilter,
+		"group_filter":             opt.groupFilter,
-		"group_team_map":           groupTeamMap,
+		"group_team_map":           opt.groupTeamMap,
-		"group_team_map_removal":   groupTeamMapRemoval,
+		"group_team_map_removal":   opt.groupTeamMapRemoval,
 		"user_uid":                 "DN",
 	}
 }
 
-func addAuthSourceLDAP(t *testing.T, sshKeyAttribute, groupFilter string, groupMapParams ...string) {
+func (te *ldapTestEnv) addAuthSource(t *testing.T, opts ...ldapAuthOptions) {
-	groupTeamMapRemoval := "off"
-	groupTeamMap := ""
-	if len(groupMapParams) == 2 {
-		groupTeamMapRemoval = groupMapParams[0]
-		groupTeamMap = groupMapParams[1]
-	}
 	session := loginUser(t, "user1")
 	csrf := GetUserCSRFToken(t, session)
-	req := NewRequestWithValues(t, "POST", "/-/admin/auths/new", buildAuthSourceLDAPPayload(csrf, sshKeyAttribute, groupFilter, groupTeamMap, groupTeamMapRemoval))
+	req := NewRequestWithValues(t, "POST", "/-/admin/auths/new", te.buildAuthSourcePayload(csrf, opts...))
 	session.MakeRequest(t, req, http.StatusSeeOther)
 }
 
 func TestLDAPUserSignin(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
 	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "", "")
+	te.addAuthSource(t)
 
-	u := gitLDAPUsers[0]
+	u := te.gitLDAPUsers[0]
 
 	session := loginUserWithPassword(t, u.UserName, u.Password)
 	req := NewRequest(t, "GET", "/user/settings")
@@ -183,8 +191,13 @@ func TestLDAPUserSignin(t *testing.T) {
 }
 
 func TestLDAPAuthChange(t *testing.T) {
+	te := prepareLdapTestEnv(t)
+	if te == nil {
+		return
+	}
+
 	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "", "")
+	te.addAuthSource(t)
 
 	session := loginUser(t, "user1")
 	req := NewRequest(t, "GET", "/-/admin/auths")
@@ -201,34 +214,35 @@ func TestLDAPAuthChange(t *testing.T) {
 	doc = NewHTMLParser(t, resp.Body)
 	csrf := doc.GetCSRF()
 	host, _ := doc.Find(`input[name="host"]`).Attr("value")
-	assert.Equal(t, host, getLDAPServerHost())
+	assert.Equal(t, te.serverHost, host)
 	binddn, _ := doc.Find(`input[name="bind_dn"]`).Attr("value")
 	assert.Equal(t, "uid=gitea,ou=service,dc=planetexpress,dc=com", binddn)
 
-	req = NewRequestWithValues(t, "POST", href, buildAuthSourceLDAPPayload(csrf, "", "", "", "off"))
+	req = NewRequestWithValues(t, "POST", href, te.buildAuthSourcePayload(csrf, ldapAuthOptions{groupTeamMapRemoval: "off"}))
 	session.MakeRequest(t, req, http.StatusSeeOther)
 
 	req = NewRequest(t, "GET", href)
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	doc = NewHTMLParser(t, resp.Body)
 	host, _ = doc.Find(`input[name="host"]`).Attr("value")
-	assert.Equal(t, host, getLDAPServerHost())
+	assert.Equal(t, te.serverHost, host)
 	binddn, _ = doc.Find(`input[name="bind_dn"]`).Attr("value")
 	assert.Equal(t, "uid=gitea,ou=service,dc=planetexpress,dc=com", binddn)
 }
 
 func TestLDAPUserSync(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
+
 	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "", "")
+	te.addAuthSource(t)
 	err := auth.SyncExternalUsers(context.Background(), true)
 	assert.NoError(t, err)
 
 	// Check if users exists
-	for _, gitLDAPUser := range gitLDAPUsers {
+	for _, gitLDAPUser := range te.gitLDAPUsers {
 		dbUser, err := user_model.GetUserByName(db.DefaultContext, gitLDAPUser.UserName)
 		assert.NoError(t, err)
 		assert.Equal(t, gitLDAPUser.UserName, dbUser.Name)
@@ -238,27 +252,28 @@ func TestLDAPUserSync(t *testing.T) {
 	}
 
 	// Check if no users exist
-	for _, otherLDAPUser := range otherLDAPUsers {
+	for _, otherLDAPUser := range te.otherLDAPUsers {
 		_, err := user_model.GetUserByName(db.DefaultContext, otherLDAPUser.UserName)
 		assert.True(t, user_model.IsErrUserNotExist(err))
 	}
 }
 
 func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
+
 	defer tests.PrepareTestEnv(t)()
 
 	session := loginUser(t, "user1")
 	csrf := GetUserCSRFToken(t, session)
-	payload := buildAuthSourceLDAPPayload(csrf, "", "", "", "")
+	payload := te.buildAuthSourcePayload(csrf)
 	payload["attribute_username"] = ""
 	req := NewRequestWithValues(t, "POST", "/-/admin/auths/new", payload)
 	session.MakeRequest(t, req, http.StatusSeeOther)
 
-	for _, u := range gitLDAPUsers {
+	for _, u := range te.gitLDAPUsers {
 		req := NewRequest(t, "GET", "/-/admin/users?q="+u.UserName)
 		resp := session.MakeRequest(t, req, http.StatusOK)
 
@@ -268,7 +283,7 @@ func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) {
 		assert.Equal(t, 0, tr.Length())
 	}
 
-	for _, u := range gitLDAPUsers {
+	for _, u := range te.gitLDAPUsers {
 		req := NewRequestWithValues(t, "POST", "/user/login", map[string]string{
 			"_csrf":     csrf,
 			"user_name": u.UserName,
@@ -277,7 +292,7 @@ func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) {
 		MakeRequest(t, req, http.StatusSeeOther)
 	}
 
-	auth.SyncExternalUsers(context.Background(), true)
+	require.NoError(t, auth.SyncExternalUsers(context.Background(), true))
 
 	authSource := unittest.AssertExistsAndLoadBean(t, &auth_model.Source{
 		Name: payload["name"],
@@ -285,9 +300,9 @@ func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) {
 	unittest.AssertCount(t, &user_model.User{
 		LoginType:   auth_model.LDAP,
 		LoginSource: authSource.ID,
-	}, len(gitLDAPUsers))
+	}, len(te.gitLDAPUsers))
 
-	for _, u := range gitLDAPUsers {
+	for _, u := range te.gitLDAPUsers {
 		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{
 			Name: u.UserName,
 		})
@@ -296,12 +311,13 @@ func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) {
 }
 
 func TestLDAPUserSyncWithGroupFilter(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
+
 	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "", "(cn=git)")
+	te.addAuthSource(t, ldapAuthOptions{groupFilter: "(cn=git)"})
 
 	// Assert a user not a member of the LDAP group "cn=git" cannot login
 	// This test may look like TestLDAPUserSigninFailed but it is not.
@@ -309,20 +325,20 @@ func TestLDAPUserSyncWithGroupFilter(t *testing.T) {
 	// This test is for the case when LDAP user records may not be linked with
 	// all groups the user is a member of, the user filter is modified accordingly inside
 	// the addAuthSourceLDAP based on the value of the groupFilter
-	u := otherLDAPUsers[0]
+	u := te.otherLDAPUsers[0]
 	testLoginFailed(t, u.UserName, u.Password, translation.NewLocale("en-US").TrString("form.username_password_incorrect"))
 
-	auth.SyncExternalUsers(context.Background(), true)
+	require.NoError(t, auth.SyncExternalUsers(context.Background(), true))
 
 	// Assert members of LDAP group "cn=git" are added
-	for _, gitLDAPUser := range gitLDAPUsers {
+	for _, gitLDAPUser := range te.gitLDAPUsers {
 		unittest.BeanExists(t, &user_model.User{
 			Name: gitLDAPUser.UserName,
 		})
 	}
 
 	// Assert everyone else is not added
-	for _, gitLDAPUser := range otherLDAPUsers {
+	for _, gitLDAPUser := range te.otherLDAPUsers {
 		unittest.AssertNotExistsBean(t, &user_model.User{
 			Name: gitLDAPUser.UserName,
 		})
@@ -333,11 +349,11 @@ func TestLDAPUserSyncWithGroupFilter(t *testing.T) {
 	})
 	ldapConfig := ldapSource.Cfg.(*ldap.Source)
 	ldapConfig.GroupFilter = "(cn=ship_crew)"
-	auth_model.UpdateSource(db.DefaultContext, ldapSource)
+	require.NoError(t, auth_model.UpdateSource(db.DefaultContext, ldapSource))
 
-	auth.SyncExternalUsers(context.Background(), true)
+	require.NoError(t, auth.SyncExternalUsers(context.Background(), true))
 
-	for _, gitLDAPUser := range gitLDAPUsers {
+	for _, gitLDAPUser := range te.gitLDAPUsers {
 		if gitLDAPUser.UserName == "fry" || gitLDAPUser.UserName == "leela" || gitLDAPUser.UserName == "bender" {
 			// Assert members of the LDAP group "cn-ship_crew" are still active
 			user := unittest.AssertExistsAndLoadBean(t, &user_model.User{
@@ -355,29 +371,31 @@ func TestLDAPUserSyncWithGroupFilter(t *testing.T) {
 }
 
 func TestLDAPUserSigninFailed(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
-	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "", "")
 
-	u := otherLDAPUsers[0]
+	defer tests.PrepareTestEnv(t)()
+	te.addAuthSource(t)
+
+	u := te.otherLDAPUsers[0]
 	testLoginFailed(t, u.UserName, u.Password, translation.NewLocale("en-US").TrString("form.username_password_incorrect"))
 }
 
 func TestLDAPUserSSHKeySync(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
-	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "sshPublicKey", "")
 
-	auth.SyncExternalUsers(context.Background(), true)
+	defer tests.PrepareTestEnv(t)()
+	te.addAuthSource(t, ldapAuthOptions{attributeSSHPublicKey: "sshPublicKey"})
+
+	require.NoError(t, auth.SyncExternalUsers(context.Background(), true))
 
 	// Check if users has SSH keys synced
-	for _, u := range gitLDAPUsers {
+	for _, u := range te.gitLDAPUsers {
 		if len(u.SSHKeys) == 0 {
 			continue
 		}
@@ -400,18 +418,22 @@ func TestLDAPUserSSHKeySync(t *testing.T) {
 }
 
 func TestLDAPGroupTeamSyncAddMember(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
+
 	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "", "", "on", `{"cn=ship_crew,ou=people,dc=planetexpress,dc=com":{"org26": ["team11"]},"cn=admin_staff,ou=people,dc=planetexpress,dc=com": {"non-existent": ["non-existent"]}}`)
+	te.addAuthSource(t, ldapAuthOptions{
+		groupTeamMap:        `{"cn=ship_crew,ou=people,dc=planetexpress,dc=com":{"org26": ["team11"]},"cn=admin_staff,ou=people,dc=planetexpress,dc=com": {"non-existent": ["non-existent"]}}`,
+		groupTeamMapRemoval: "on",
+	})
 	org, err := organization.GetOrgByName(db.DefaultContext, "org26")
 	assert.NoError(t, err)
 	team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11")
 	assert.NoError(t, err)
-	auth.SyncExternalUsers(context.Background(), true)
+	require.NoError(t, auth.SyncExternalUsers(context.Background(), true))
-	for _, gitLDAPUser := range gitLDAPUsers {
+	for _, gitLDAPUser := range te.gitLDAPUsers {
 		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{
 			Name: gitLDAPUser.UserName,
 		})
@@ -445,19 +467,22 @@ func TestLDAPGroupTeamSyncAddMember(t *testing.T) {
 }
 
 func TestLDAPGroupTeamSyncRemoveMember(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
 	defer tests.PrepareTestEnv(t)()
-	addAuthSourceLDAP(t, "", "", "on", `{"cn=dispatch,ou=people,dc=planetexpress,dc=com": {"org26": ["team11"]}}`)
+	te.addAuthSource(t, ldapAuthOptions{
+		groupTeamMap:        `{"cn=dispatch,ou=people,dc=planetexpress,dc=com": {"org26": ["team11"]}}`,
+		groupTeamMapRemoval: "on",
+	})
 	org, err := organization.GetOrgByName(db.DefaultContext, "org26")
 	assert.NoError(t, err)
 	team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11")
 	assert.NoError(t, err)
-	loginUserWithPassword(t, gitLDAPUsers[0].UserName, gitLDAPUsers[0].Password)
+	loginUserWithPassword(t, te.gitLDAPUsers[0].UserName, te.gitLDAPUsers[0].Password)
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{
-		Name: gitLDAPUsers[0].UserName,
+		Name: te.gitLDAPUsers[0].UserName,
 	})
 	err = organization.AddOrgUser(db.DefaultContext, org.ID, user.ID)
 	assert.NoError(t, err)
@@ -470,7 +495,7 @@ func TestLDAPGroupTeamSyncRemoveMember(t *testing.T) {
 	assert.NoError(t, err)
 	assert.True(t, isMember, "User should be member of this team")
 	// assert team member "professor" gets removed from org26 team11
-	loginUserWithPassword(t, gitLDAPUsers[0].UserName, gitLDAPUsers[0].Password)
+	loginUserWithPassword(t, te.gitLDAPUsers[0].UserName, te.gitLDAPUsers[0].Password)
 	isMember, err = organization.IsOrganizationMember(db.DefaultContext, org.ID, user.ID)
 	assert.NoError(t, err)
 	assert.False(t, isMember, "User membership should have been removed from organization")
@@ -480,14 +505,67 @@ func TestLDAPGroupTeamSyncRemoveMember(t *testing.T) {
 }
 
 func TestLDAPPreventInvalidGroupTeamMap(t *testing.T) {
-	if skipLDAPTests() {
+	te := prepareLdapTestEnv(t)
-		t.Skip()
+	if te == nil {
 		return
 	}
 	defer tests.PrepareTestEnv(t)()
 
 	session := loginUser(t, "user1")
 	csrf := GetUserCSRFToken(t, session)
-	req := NewRequestWithValues(t, "POST", "/-/admin/auths/new", buildAuthSourceLDAPPayload(csrf, "", "", `{"NOT_A_VALID_JSON"["MISSING_DOUBLE_POINT"]}`, "off"))
+	payload := te.buildAuthSourcePayload(csrf, ldapAuthOptions{groupTeamMap: `{"NOT_A_VALID_JSON"["MISSING_DOUBLE_POINT"]}`, groupTeamMapRemoval: "off"})
+	req := NewRequestWithValues(t, "POST", "/-/admin/auths/new", payload)
 	session.MakeRequest(t, req, http.StatusOK) // StatusOK = failed, StatusSeeOther = ok
 }
+
+func TestLDAPEmailSignin(t *testing.T) {
+	te := ldapTestEnv{
+		gitLDAPUsers: []ldapUser{
+			{
+				UserName: "u1",
+				Password: "xx",
+				FullName: "user 1",
+				Email:    "u1@gitea.com",
+			},
+		},
+		serverHost: "mock-host",
+		serverPort: "mock-port",
+	}
+	defer test.MockVariableValue(&ldap.MockedSearchEntry, func(source *ldap.Source, name, passwd string, directBind bool) *ldap.SearchResult {
+		var u *ldapUser
+		for _, user := range te.gitLDAPUsers {
+			if user.Email == name && user.Password == passwd {
+				u = &user
+				break
+			}
+		}
+		if u == nil {
+			return nil
+		}
+		result := &ldap.SearchResult{
+			Username:  u.UserName,
+			Mail:      u.Email,
+			LowerName: strings.ToLower(u.UserName),
+		}
+		nameFields := strings.Split(u.FullName, " ")
+		result.Name = nameFields[0]
+		if len(nameFields) > 1 {
+			result.Surname = nameFields[1]
+		}
+		return result
+	})()
+	defer tests.PrepareTestEnv(t)()
+	te.addAuthSource(t)
+
+	u := te.gitLDAPUsers[0]
+
+	session := loginUserWithPassword(t, u.Email, u.Password)
+	req := NewRequest(t, "GET", "/user/settings")
+	resp := session.MakeRequest(t, req, http.StatusOK)
+
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	assert.Equal(t, u.UserName, htmlDoc.GetInputValueByName("name"))
+	assert.Equal(t, u.FullName, htmlDoc.GetInputValueByName("full_name"))
+	assert.Equal(t, u.Email, htmlDoc.Find("#signed-user-email").Text())
+}