Fix LDAP sync when Username Attribute is empty (#25278) (#25379)

Backport #25278 by @Zettat123

Fix #21072


![image](https://github.com/go-gitea/gitea/assets/15528715/96b30beb-7f88-4a60-baae-2e5ad8049555)

Username Attribute is not a required item when creating an
authentication source. If Username Attribute is empty, the username
value of LDAP user cannot be read, so all users from LDAP will be marked
as inactive by mistake when synchronizing external users.

This PR improves the sync logic, if username is empty, the email address
will be used to find user.

Co-authored-by: Zettat123 <zettat123@gmail.com>

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()