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>
This commit is contained in:
Giteabot 2023-06-20 01:11:22 -04:00 committed by GitHub
parent 10fcb55507
commit dfefe86045
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 84 additions and 31 deletions

View file

@ -6,7 +6,6 @@ package ldap
import ( import (
"context" "context"
"fmt" "fmt"
"sort"
"strings" "strings"
asymkey_model "code.gitea.io/gitea/models/asymkey" asymkey_model "code.gitea.io/gitea/models/asymkey"
@ -24,7 +23,6 @@ import (
func (source *Source) Sync(ctx context.Context, updateExisting bool) error { func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
log.Trace("Doing: SyncExternalUsers[%s]", source.authSource.Name) log.Trace("Doing: SyncExternalUsers[%s]", source.authSource.Name)
var existingUsers []int
isAttributeSSHPublicKeySet := len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0 isAttributeSSHPublicKeySet := len(strings.TrimSpace(source.AttributeSSHPublicKey)) > 0
var sshKeysNeedUpdate bool var sshKeysNeedUpdate bool
@ -41,9 +39,14 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
default: default:
} }
sort.Slice(users, func(i, j int) bool { usernameUsers := make(map[string]*user_model.User, len(users))
return users[i].LowerName < users[j].LowerName 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() sr, err := source.SearchEntries()
if err != nil { 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") 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) orgCache := make(map[string]*organization.Organization)
teamCache := make(map[string]*organization.Team) 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) return db.ErrCancelledf("During update of %s before completed update of users", source.authSource.Name)
default: 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 continue
} }
@ -94,15 +107,6 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
su.Mail = fmt.Sprintf("%s@localhost", su.Username) 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) fullName := composeFullName(su.Name, su.Surname, su.Username)
// If no existing user found, create one // If no existing user found, create one
if usr == nil { if usr == nil {
@ -203,19 +207,17 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error {
// Deactivate users not present in LDAP // Deactivate users not present in LDAP
if updateExisting { if updateExisting {
existPos := 0 for _, usr := range users {
for i, usr := range users { if _, ok := keepActiveUsers[usr.ID]; ok {
for existPos < len(existingUsers) && i > existingUsers[existPos] { continue
existPos++
} }
if usr.IsActive && (existPos >= len(existingUsers) || i < existingUsers[existPos]) {
log.Trace("SyncExternalUsers[%s]: Deactivating user %s", source.authSource.Name, usr.Name)
usr.IsActive = false log.Trace("SyncExternalUsers[%s]: Deactivating user %s", source.authSource.Name, usr.Name)
err = user_model.UpdateUserCols(ctx, usr, "is_active")
if err != nil { usr.IsActive = false
log.Error("SyncExternalUsers[%s]: Error deactivating user %s: %v", source.authSource.Name, usr.Name, err) 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)
} }
} }
} }

View file

@ -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) { func TestLDAPUserSyncWithGroupFilter(t *testing.T) {
if skipLDAPTests() { if skipLDAPTests() {
t.Skip() t.Skip()