From 5d6c5ce71abf6352cb5651cc7e9361b5a89cf84b Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 3 Jun 2025 05:19:55 +0800 Subject: [PATCH] Fix/improve avatar sync from LDAP (#34573) (#34587) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backport #34573 by @raucao This fixes 3 issues I encountered when debugging problems with our LDAP sync: 1. The comparison of the hashed image data in `IsUploadAvatarChanged` is wrong. It seems to be from before avatar hashing was changed and unified in #22289. This results in the function always returning `true` for any avatars, even if they weren't changed. 2. Even if there's no avatar to upload (i.e. no avatar available for the LDAP entry), the upload function would still be called for every single user, only to then fail, because the data isn't valid. This is unnecessary. 3. Another small issue is that the comparison function (and thus hashing of data) is called for every user, even if there is no avatar attribute configured at all for the LDAP source. Thus, I switched the condition nesting, so that no cycles are wasted when avatar sync isn't configured in the first place. I also added a trace log for when there is actually a new avatar being uploaded for an existing user, which is now only shown when that is actually the case. Co-authored-by: Râu Cao <842+raucao@users.noreply.github.com> Co-authored-by: wxiaoguang --- models/user/avatar.go | 3 +-- services/auth/source/ldap/source_sync.go | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/models/user/avatar.go b/models/user/avatar.go index 3d9fc4452f..542bd93b98 100644 --- a/models/user/avatar.go +++ b/models/user/avatar.go @@ -5,7 +5,6 @@ package user import ( "context" - "crypto/md5" "fmt" "image/png" "io" @@ -106,7 +105,7 @@ func (u *User) IsUploadAvatarChanged(data []byte) bool { if !u.UseCustomAvatar || len(u.Avatar) == 0 { return true } - avatarID := fmt.Sprintf("%x", md5.Sum([]byte(fmt.Sprintf("%d-%x", u.ID, md5.Sum(data))))) + avatarID := avatar.HashAvatar(u.ID, data) return u.Avatar != avatarID } diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 678b6b2b62..416b67dd46 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -178,8 +178,9 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { } } - if usr.IsUploadAvatarChanged(su.Avatar) { - if err == nil && source.AttributeAvatar != "" { + if source.AttributeAvatar != "" { + if len(su.Avatar) > 0 && usr.IsUploadAvatarChanged(su.Avatar) { + log.Trace("SyncExternalUsers[%s]: Uploading new avatar for %s", source.AuthSource.Name, usr.Name) _ = user_service.UploadAvatar(ctx, usr, su.Avatar) } }