diff options
| -rw-r--r-- | services/auth/source/ldap/source_sync.go | 62 | ||||
| -rw-r--r-- | tests/integration/auth_ldap_test.go | 51 |
2 files changed, 83 insertions, 30 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,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) } } } 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() |