From 33cd74ad70591c36dfd1d14bd75f3eb1c2f28906 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Tue, 20 Jun 2023 11:04:13 +0800 Subject: [PATCH] Fix LDAP sync when Username Attribute is empty (#25278) 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. --- services/auth/source/ldap/source_sync.go | 64 ++++++++++++------------ tests/integration/auth_ldap_test.go | 51 +++++++++++++++++++ 2 files changed, 84 insertions(+), 31 deletions(-) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 4571ff6540..3e0f47a37e 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/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,7 +84,22 @@ 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 + } + + 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)] + } + + if usr != nil { + keepActiveUsers[usr.ID] = struct{}{} + } else if len(su.Username) == 0 { + // we cannot create the user if su.Username is empty continue } @@ -94,15 +107,6 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { su.Mail = fmt.Sprintf("%s@localhost", su.Username) } - var usr *user_model.User - for userPos < len(users) && users[userPos].LowerName < su.LowerName { - userPos++ - } - if userPos < len(users) && users[userPos].LowerName == su.LowerName { - usr = users[userPos] - existingUsers = append(existingUsers, userPos) - } - fullName := composeFullName(su.Name, su.Surname, su.Username) // If no existing user found, create one if usr == nil { @@ -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) } } } diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index 81f8d9ddd7..d2160a3dbe 100644 --- a/tests/integration/auth_ldap_test.go +++ b/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()