Fix LDAP sync when Username Attribute is empty

services/auth/source/ldap/source_sync.go

@@ -6,7 +6,6 @@ package ldap
 import (
 	"context"
 	"fmt"
-	"sort"
 	"strings"
 
 	asymkey_model "code.gitea.io/gitea/models/asymkey"
@@ -24,7 +23,6 @@ import (
 func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 	log.Trace("Doing: SyncExternalUsers[%s]", source.authSource.Name)
 
-	var existingUsers []int
 	isAttributeSSHPublicKeySet := len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0
 	var sshKeysNeedUpdate bool
 
@@ -41,9 +39,14 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 	default:
 	}
 
-	sort.Slice(users, func(i, j int) bool {
-		return users[i].LowerName < users[j].LowerName
-	})
+	usernameUsers := make(map[string]*user_model.User, len(users))
+	mailUsers := make(map[string]*user_model.User, len(users))
+	keepActiveUsers := make(map[int64]struct{})
+
+	for _, u := range users {
+		usernameUsers[u.LowerName] = u
+		mailUsers[strings.ToLower(u.Email)] = u
+	}
 
 	sr, err := source.SearchEntries()
 	if err != nil {
@@ -59,11 +62,6 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 		log.Warn("LDAP search found no entries but did not report an error. All users will be deactivated as per settings")
 	}
 
-	sort.Slice(sr, func(i, j int) bool {
-		return sr[i].LowerName < sr[j].LowerName
-	})
-
-	userPos := 0
 	orgCache := make(map[string]*organization.Organization)
 	teamCache := make(map[string]*organization.Team)
 
@@ -86,21 +84,27 @@ 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 {
+		if len(su.Username) == 0 && len(su.Mail) == 0 {
 			continue
 		}
 
-		if len(su.Mail) == 0 {
-			su.Mail = fmt.Sprintf("%s@localhost", su.Username)
+		var usr *user_model.User
+		if len(su.Username) > 0 {
+			usr = usernameUsers[su.LowerName]
+		}
+		if usr == nil && len(su.Mail) > 0 {
+			usr = mailUsers[strings.ToLower(su.Mail)]
 		}
 
-		var usr *user_model.User
-		for userPos < len(users) && users[userPos].LowerName < su.LowerName {
-			userPos++
+		if usr != nil {
+			keepActiveUsers[usr.ID] = struct{}{}
+		} else if len(su.Username) == 0 {
+			// we cannot create the user if su.Username is empty
+			continue
 		}
-		if userPos < len(users) && users[userPos].LowerName == su.LowerName {
-			usr = users[userPos]
-			existingUsers = append(existingUsers, userPos)
+
+		if len(su.Mail) == 0 {
+			su.Mail = fmt.Sprintf("%s@localhost", su.Username)
 		}
 
 		fullName := composeFullName(su.Name, su.Surname, su.Username)
@@ -203,19 +207,17 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
 
 	// Deactivate users not present in LDAP
 	if updateExisting {
-		existPos := 0
-		for i, usr := range users {
-			for existPos < len(existingUsers) && i > existingUsers[existPos] {
-				existPos++
+		for _, usr := range users {
+			if _, ok := keepActiveUsers[usr.ID]; ok {
+				continue
 			}
-			if usr.IsActive && (existPos >= len(existingUsers) || i < existingUsers[existPos]) {
-				log.Trace("SyncExternalUsers[%s]: Deactivating user %s", source.authSource.Name, usr.Name)
 
-				usr.IsActive = false
-				err = user_model.UpdateUserCols(ctx, usr, "is_active")
-				if err != nil {
-					log.Error("SyncExternalUsers[%s]: Error deactivating user %s: %v", source.authSource.Name, usr.Name, err)
-				}
+			log.Trace("SyncExternalUsers[%s]: Deactivating user %s", source.authSource.Name, usr.Name)
+
+			usr.IsActive = false
+			err = user_model.UpdateUserCols(ctx, usr, "is_active")
+			if err != nil {
+				log.Error("SyncExternalUsers[%s]: Error deactivating user %s: %v", source.authSource.Name, usr.Name, err)
 			}
 		}
 	}

tests/integration/auth_ldap_test.go

@@ -268,6 +268,57 @@ func TestLDAPUserSync(t *testing.T) {
 	}
 }
 
+func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) {
+	if skipLDAPTests() {
+		t.Skip()
+		return
+	}
+	defer tests.PrepareTestEnv(t)()
+
+	session := loginUser(t, "user1")
+	csrf := GetCSRF(t, session, "/admin/auths/new")
+	payload := buildAuthSourceLDAPPayload(csrf, "", "", "", "")
+	payload["attribute_username"] = ""
+	req := NewRequestWithValues(t, "POST", "/admin/auths/new", payload)
+	session.MakeRequest(t, req, http.StatusSeeOther)
+
+	for _, u := range gitLDAPUsers {
+		req := NewRequest(t, "GET", "/admin/users?q="+u.UserName)
+		resp := session.MakeRequest(t, req, http.StatusOK)
+
+		htmlDoc := NewHTMLParser(t, resp.Body)
+
+		tr := htmlDoc.doc.Find("table.table tbody tr")
+		assert.True(t, tr.Length() == 0)
+	}
+
+	for _, u := range gitLDAPUsers {
+		req := NewRequestWithValues(t, "POST", "/user/login", map[string]string{
+			"_csrf":     csrf,
+			"user_name": u.UserName,
+			"password":  u.Password,
+		})
+		MakeRequest(t, req, http.StatusSeeOther)
+	}
+
+	auth.SyncExternalUsers(context.Background(), true)
+
+	authSource := unittest.AssertExistsAndLoadBean(t, &auth_model.Source{
+		Name: payload["name"],
+	})
+	unittest.AssertCount(t, &user_model.User{
+		LoginType:   auth_model.LDAP,
+		LoginSource: authSource.ID,
+	}, len(gitLDAPUsers))
+
+	for _, u := range gitLDAPUsers {
+		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{
+			Name: u.UserName,
+		})
+		assert.True(t, user.IsActive)
+	}
+}
+
 func TestLDAPUserSyncWithGroupFilter(t *testing.T) {
 	if skipLDAPTests() {
 		t.Skip()