From dfefe860456ffc94cac25d4670a3ddbeb3c18d35 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 20 Jun 2023 01:11:22 -0400 Subject: [PATCH] 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 --- 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()