From e54546aa77ce43e2176003ecb194155d06eff2d9 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Thu, 15 Jun 2023 14:36:18 +0800 Subject: [PATCH 1/8] fix ldap sync --- services/auth/source/ldap/source_sync.go | 62 ++++++++++++------------ 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 4571ff6540c3e..2060800c2667c 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,15 @@ 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)) + existingUsers := make(map[int64]struct{}) + + for i := range users { + u := users[i] + usernameUsers[u.LowerName] = u + mailUsers[u.Email] = u + } sr, err := source.SearchEntries() if err != nil { @@ -59,11 +63,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 +85,23 @@ 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 - for userPos < len(users) && users[userPos].LowerName < su.LowerName { - userPos++ + if len(su.Username) > 0 { + usr = usernameUsers[su.LowerName] + } + if usr == nil && len(su.Mail) > 0 { + usr = mailUsers[su.Mail] } - if userPos < len(users) && users[userPos].LowerName == su.LowerName { - usr = users[userPos] - existingUsers = append(existingUsers, userPos) + if usr != nil { + existingUsers[usr.ID] = struct{}{} + } + + if len(su.Mail) == 0 { + su.Mail = fmt.Sprintf("%s@localhost", su.Username) } fullName := composeFullName(su.Name, su.Surname, su.Username) @@ -203,19 +204,18 @@ 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 i := range users { + usr := users[i] + if _, ok := existingUsers[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) } } } From a7980a0089f1b4f8c0b8c585d56e1f70e5c5e7f6 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Thu, 15 Jun 2023 18:30:35 +0800 Subject: [PATCH 2/8] add TestLDAPUserSyncWithEmptyUsernameAttribute(wip) --- services/auth/source/ldap/source_sync.go | 2 +- tests/integration/auth_ldap_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 2060800c2667c..44d222a9e751d 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -106,7 +106,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { fullName := composeFullName(su.Name, su.Surname, su.Username) // If no existing user found, create one - if usr == nil { + if usr == nil && len(su.Username) > 0 { log.Trace("SyncExternalUsers[%s]: Creating user %s", source.authSource.Name, su.Username) usr = &user_model.User{ diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index 81f8d9ddd7503..828695a63c76d 100644 --- a/tests/integration/auth_ldap_test.go +++ b/tests/integration/auth_ldap_test.go @@ -268,6 +268,30 @@ func TestLDAPUserSync(t *testing.T) { } } +func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) { + if skipLDAPTests() { + t.Skip() + return + } + + 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) + } +} + func TestLDAPUserSyncWithGroupFilter(t *testing.T) { if skipLDAPTests() { t.Skip() From 22bea98933b1bc160097472c729937343958c20f Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Fri, 16 Jun 2023 10:25:56 +0800 Subject: [PATCH 3/8] impl TestLDAPUserSyncWithEmptyUsernameAttribute --- tests/integration/auth_ldap_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index 828695a63c76d..20335583f6d5c 100644 --- a/tests/integration/auth_ldap_test.go +++ b/tests/integration/auth_ldap_test.go @@ -273,6 +273,7 @@ func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) { t.Skip() return } + defer tests.PrepareTestEnv(t)() session := loginUser(t, "user1") csrf := GetCSRF(t, session, "/admin/auths/new") @@ -290,6 +291,24 @@ func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) { 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) + + for _, u := range gitLDAPUsers { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ + Name: u.UserName, + }) + assert.True(t, user.IsActive) + } } func TestLDAPUserSyncWithGroupFilter(t *testing.T) { From c8e6607d54013999a3de3ac46686f331e44382a8 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Fri, 16 Jun 2023 14:47:23 +0800 Subject: [PATCH 4/8] add count assert --- tests/integration/auth_ldap_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index 20335583f6d5c..d2160a3dbea2c 100644 --- a/tests/integration/auth_ldap_test.go +++ b/tests/integration/auth_ldap_test.go @@ -303,6 +303,14 @@ func TestLDAPUserSyncWithEmptyUsernameAttribute(t *testing.T) { 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, From 003ea013cc019eed323473f604f8da4050f44442 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Fri, 16 Jun 2023 16:32:11 +0800 Subject: [PATCH 5/8] convert email to lowercase and fix existingUsers --- services/auth/source/ldap/source_sync.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 44d222a9e751d..3104efc8b0fdc 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -41,12 +41,12 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { usernameUsers := make(map[string]*user_model.User, len(users)) mailUsers := make(map[string]*user_model.User, len(users)) - existingUsers := make(map[int64]struct{}) + keepActiveUsers := make(map[int64]struct{}) for i := range users { u := users[i] usernameUsers[u.LowerName] = u - mailUsers[u.Email] = u + mailUsers[strings.ToLower(u.Email)] = u } sr, err := source.SearchEntries() @@ -94,19 +94,22 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { usr = usernameUsers[su.LowerName] } if usr == nil && len(su.Mail) > 0 { - usr = mailUsers[su.Mail] + usr = mailUsers[strings.ToLower(su.Mail)] } - if usr != nil { - existingUsers[usr.ID] = struct{}{} + if usr == nil && len(su.Username) == 0 { + // we cannot create the user if su.Username is empty + continue } + keepActiveUsers[usr.ID] = struct{}{} + if len(su.Mail) == 0 { su.Mail = fmt.Sprintf("%s@localhost", su.Username) } fullName := composeFullName(su.Name, su.Surname, su.Username) // If no existing user found, create one - if usr == nil && len(su.Username) > 0 { + if usr == nil { log.Trace("SyncExternalUsers[%s]: Creating user %s", source.authSource.Name, su.Username) usr = &user_model.User{ @@ -206,7 +209,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { if updateExisting { for i := range users { usr := users[i] - if _, ok := existingUsers[usr.ID]; ok { + if _, ok := keepActiveUsers[usr.ID]; ok { continue } From 0d25bae69ced1a2a4aa9736c0f87a574b66bbe43 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Fri, 16 Jun 2023 16:37:14 +0800 Subject: [PATCH 6/8] fix update keepActiveUsers --- services/auth/source/ldap/source_sync.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 3104efc8b0fdc..dbb4208bc8092 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -96,13 +96,14 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { if usr == nil && len(su.Mail) > 0 { usr = mailUsers[strings.ToLower(su.Mail)] } - if usr == nil && len(su.Username) == 0 { + + if usr != nil { + keepActiveUsers[usr.ID] = struct{}{} + } else if len(su.Username) == 0 { // we cannot create the user if su.Username is empty continue } - keepActiveUsers[usr.ID] = struct{}{} - if len(su.Mail) == 0 { su.Mail = fmt.Sprintf("%s@localhost", su.Username) } From e50f6d0c3880937ae5ff4745872402740e73724c Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 19 Jun 2023 11:48:12 +0800 Subject: [PATCH 7/8] improve for range loop --- services/auth/source/ldap/source_sync.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index dbb4208bc8092..82211b38daa4d 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -43,8 +43,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { mailUsers := make(map[string]*user_model.User, len(users)) keepActiveUsers := make(map[int64]struct{}) - for i := range users { - u := users[i] + for _,u := range users { usernameUsers[u.LowerName] = u mailUsers[strings.ToLower(u.Email)] = u } From 99ee376ec49f92e239c4aed491443ba2e613c6ab Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 19 Jun 2023 11:50:00 +0800 Subject: [PATCH 8/8] improve for range loop --- services/auth/source/ldap/source_sync.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 82211b38daa4d..3e0f47a37e296 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -43,7 +43,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { mailUsers := make(map[string]*user_model.User, len(users)) keepActiveUsers := make(map[int64]struct{}) - for _,u := range users { + for _, u := range users { usernameUsers[u.LowerName] = u mailUsers[strings.ToLower(u.Email)] = u } @@ -207,8 +207,7 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { // Deactivate users not present in LDAP if updateExisting { - for i := range users { - usr := users[i] + for _, usr := range users { if _, ok := keepActiveUsers[usr.ID]; ok { continue }