ldap: Fix LDAP avatar synchronization to check if avatar has changed.

When "manage.py sync_ldap_user_data" is run, user avatars are now only
updated if they have changed in LDAP.

Fixes #12381.

zerver/lib/avatar.py

@@ -2,7 +2,7 @@
 
 from typing import Any, Dict, Optional
 
-from zerver.lib.avatar_hash import gravatar_hash, user_avatar_path_from_ids
+from zerver.lib.avatar_hash import gravatar_hash, user_avatar_path_from_ids, user_avatar_content_hash
 from zerver.lib.upload import upload_backend, MEDIUM_AVATAR_SIZE
 from zerver.models import UserProfile
 import urllib
@@ -117,3 +117,14 @@ def absolute_avatar_url(user_profile: UserProfile) -> str:
     # avatar_url can return None if client_gravatar=True, however here we use the default value of False
     assert avatar is not None
     return urllib.parse.urljoin(user_profile.realm.uri, avatar)
+
+def is_avatar_new(ldap_avatar: bytes, user_profile: UserProfile) -> bool:
+    new_avatar_hash = user_avatar_content_hash(ldap_avatar)
+
+    if user_profile.avatar_hash:
+        if user_profile.avatar_hash == new_avatar_hash:
+            # If an avatar exists and is the same as the new avatar,
+            # then, no need to change the avatar.
+            return False
+
+    return True

zerver/lib/avatar_hash.py

@@ -36,3 +36,6 @@ def user_avatar_path(user_profile: UserProfile) -> str:
 def user_avatar_path_from_ids(user_profile_id: int, realm_id: int) -> str:
     user_id_hash = user_avatar_hash(str(user_profile_id))
     return '%s/%s' % (str(realm_id), user_id_hash)
+
+def user_avatar_content_hash(ldap_avatar: bytes) -> str:
+    return hashlib.sha256(ldap_avatar).hexdigest()

zerver/migrations/0233_userprofile_avatar_hash.py

@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.20 on 2019-06-28 22:38
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zerver', '0232_make_archive_transaction_field_not_nullable'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='userprofile',
+            name='avatar_hash',
+            field=models.CharField(max_length=64, null=True),
+        ),
+    ]

zerver/models.py

@@ -932,6 +932,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin):
     )
     avatar_source = models.CharField(default=AVATAR_FROM_GRAVATAR, choices=AVATAR_SOURCES, max_length=1)  # type: str
     avatar_version = models.PositiveSmallIntegerField(default=1)  # type: int
+    avatar_hash = models.CharField(null=True, max_length=64)  # type: Optional[str]
 
     TUTORIAL_WAITING  = u'W'
     TUTORIAL_STARTED  = u'S'

zerver/tests/test_auth_backends.py

@@ -3037,6 +3037,28 @@ def test_update_user_avatar(self) -> None:
             hamlet = self.example_user('hamlet')
             self.assertEqual(hamlet.avatar_source, UserProfile.AVATAR_FROM_USER)
 
+            # Verify that the next time we do an LDAP sync, we don't
+            # end up updating this user's avatar again if the LDAP
+            # data hasn't changed.
+            self.perform_ldap_sync(self.example_user('hamlet'))
+            fn.assert_called_once()
+
+        # Now verify that if we do change the thumbnailPhoto image, we
+        # will upload a new avatar.
+        self.mock_ldap.directory = {
+            'uid=hamlet,ou=users,dc=zulip,dc=com': {
+                'cn': ['King Hamlet', ],
+                'thumbnailPhoto': [open(os.path.join(settings.STATIC_ROOT, "images/logo/zulip-icon-512x512.png"), "rb").read()]
+            }
+        }
+        with mock.patch('zerver.lib.upload.upload_avatar_image') as fn, \
+            self.settings(AUTH_LDAP_USER_ATTR_MAP={'full_name': 'cn',
+                                                   'avatar': 'thumbnailPhoto'}):
+            self.perform_ldap_sync(self.example_user('hamlet'))
+            fn.assert_called_once()
+            hamlet = self.example_user('hamlet')
+            self.assertEqual(hamlet.avatar_source, UserProfile.AVATAR_FROM_USER)
+
     @use_s3_backend
     def test_update_user_avatar_for_s3(self) -> None:
         bucket = create_s3_buckets(settings.S3_AVATAR_BUCKET)[0]

zproject/backends.py

@@ -37,6 +37,8 @@
 
 from zerver.lib.actions import do_create_user, do_reactivate_user, do_deactivate_user, \
     do_update_user_custom_profile_data, validate_email_for_realm
+from zerver.lib.avatar import is_avatar_new
+from zerver.lib.avatar_hash import user_avatar_content_hash
 from zerver.lib.dev_ldap_directory import init_fakeldap
 from zerver.lib.request import JsonableError
 from zerver.lib.users import check_full_name, validate_user_custom_profile_field
@@ -348,14 +350,24 @@ def sync_avatar_from_ldap(self, user: UserProfile, ldap_user: _LDAPUser) -> None
                 # thumbnailPhoto set in LDAP, just skip that user.
                 return
 
-            io = BytesIO(ldap_user.attrs[avatar_attr_name][0])
+            ldap_avatar = ldap_user.attrs[avatar_attr_name][0]
+
+            avatar_changed = is_avatar_new(ldap_avatar, user)
+            if not avatar_changed:
+                # Don't do work to replace the avatar with itself.
+                return
+
+            io = BytesIO(ldap_avatar)
             # Structurally, to make the S3 backend happy, we need to
             # provide a Content-Type; since that isn't specified in
             # any metadata, we auto-detect it.
             content_type = magic.from_buffer(copy.deepcopy(io).read()[0:1024], mime=True)
             if content_type.startswith("image/"):
                 upload_avatar_image(io, user, user, content_type=content_type)
                 do_change_avatar_fields(user, UserProfile.AVATAR_FROM_USER)
+                # Update avatar hash.
+                user.avatar_hash = user_avatar_content_hash(ldap_avatar)
+                user.save(update_fields=["avatar_hash"])
             else:
                 logging.warning("Could not parse %s field for user %s" %
                                 (avatar_attr_name, user.id))