avatars: Fix gravatar URLs with EMAIL_ADDRESS_VISIBILITY_ADMINS.

Previously, we were using user_profile.email rather than
user_profile.delivery_email in all calculations involving Gravatar
URLs, which meant that all organizations with the new
EMAIL_ADDRESS_VISIBILITY_ADMINS setting enabled had useless gravatars
not based on the `user15@host.domain` type fake email addresses we
generate for the API to refer to users.

The fix is to convert these calculations to use the user's
delivery_email.  Some refactoring is required to ensure the data is
passed through to the parts of the codebase that do the check;
fortunately, our automated tests of schemas are effective in verifying
that the new `sender_delivery_email` field isn't visible to the API.

Fixes #13369.

zerver/lib/actions.py

@@ -884,6 +884,12 @@ def do_change_user_delivery_email(user_profile: UserProfile, new_email: str) ->
     event = dict(type='realm_user', op='update', person=payload)
     send_event(user_profile.realm, event, [user_profile.id])
 
+    if user_profile.avatar_source == UserProfile.AVATAR_FROM_GRAVATAR:
+        # If the user is using Gravatar to manage their email address,
+        # their Gravatar just changed, and we need to notify other
+        # clients.
+        notify_avatar_url_change(user_profile)
+
     if user_profile.email_address_is_realm_public():
         # Additionally, if we're also changing the publicly visible
         # email, we send a new_email event as well.
@@ -3330,16 +3336,7 @@ def do_regenerate_api_key(user_profile: UserProfile, acting_user: UserProfile) -
 
     return new_api_key
 
-def do_change_avatar_fields(user_profile: UserProfile, avatar_source: str) -> None:
+def notify_avatar_url_change(user_profile: UserProfile) -> None:
-    user_profile.avatar_source = avatar_source
-    user_profile.avatar_version += 1
-    user_profile.save(update_fields=["avatar_source", "avatar_version"])
-    event_time = timezone_now()
-    RealmAuditLog.objects.create(realm=user_profile.realm, modified_user=user_profile,
-                                 event_type=RealmAuditLog.USER_AVATAR_SOURCE_CHANGED,
-                                 extra_data={'avatar_source': avatar_source},
-                                 event_time=event_time)
-
     if user_profile.is_bot:
         send_event(user_profile.realm,
                    dict(type='realm_bot',
@@ -3364,6 +3361,18 @@ def do_change_avatar_fields(user_profile: UserProfile, avatar_source: str) -> No
                     person=payload),
                active_user_ids(user_profile.realm_id))
 
+def do_change_avatar_fields(user_profile: UserProfile, avatar_source: str) -> None:
+    user_profile.avatar_source = avatar_source
+    user_profile.avatar_version += 1
+    user_profile.save(update_fields=["avatar_source", "avatar_version"])
+    event_time = timezone_now()
+    RealmAuditLog.objects.create(realm=user_profile.realm, modified_user=user_profile,
+                                 event_type=RealmAuditLog.USER_AVATAR_SOURCE_CHANGED,
+                                 extra_data={'avatar_source': avatar_source},
+                                 event_time=event_time)
+
+    notify_avatar_url_change(user_profile)
+
 def do_delete_avatar_image(user: UserProfile) -> None:
     do_change_avatar_fields(user, UserProfile.AVATAR_FROM_GRAVATAR)
     delete_avatar_image(user)

zerver/lib/avatar.py

@@ -12,7 +12,7 @@ def avatar_url(user_profile: UserProfile, medium: bool=False, client_gravatar: b
     return get_avatar_field(
         user_id=user_profile.id,
         realm_id=user_profile.realm_id,
-        email=user_profile.email,
+        email=user_profile.delivery_email,
         avatar_source=user_profile.avatar_source,
         avatar_version=user_profile.avatar_version,
         medium=medium,

zerver/lib/events.py

@@ -86,7 +86,7 @@ def get_raw_user_data(realm: Realm, user_profile: UserProfile, client_gravatar:
         avatar_url = get_avatar_field(
             user_id=row['id'],
             realm_id=realm.id,
-            email=row['email'],
+            email=row['delivery_email'],
             avatar_source=row['avatar_source'],
             avatar_version=row['avatar_version'],
             medium=False,

zerver/lib/message.py

@@ -212,6 +212,7 @@ class MessageDict:
             del obj['rendered_content']
         del obj['sender_realm_id']
         del obj['sender_avatar_source']
+        del obj['sender_delivery_email']
         del obj['sender_avatar_version']
 
         del obj['recipient_type']
@@ -403,6 +404,7 @@ class MessageDict:
             'id',
             'full_name',
             'short_name',
+            'delivery_email',
             'email',
             'realm__string_id',
             'avatar_source',
@@ -423,6 +425,7 @@ class MessageDict:
             obj['sender_full_name'] = user_row['full_name']
             obj['sender_short_name'] = user_row['short_name']
             obj['sender_email'] = user_row['email']
+            obj['sender_delivery_email'] = user_row['delivery_email']
             obj['sender_realm_str'] = user_row['realm__string_id']
             obj['sender_avatar_source'] = user_row['avatar_source']
             obj['sender_avatar_version'] = user_row['avatar_version']
@@ -488,14 +491,14 @@ class MessageDict:
     def set_sender_avatar(obj: Dict[str, Any], client_gravatar: bool) -> None:
         sender_id = obj['sender_id']
         sender_realm_id = obj['sender_realm_id']
-        sender_email = obj['sender_email']
+        sender_delivery_email = obj['sender_delivery_email']
         sender_avatar_source = obj['sender_avatar_source']
         sender_avatar_version = obj['sender_avatar_version']
 
         obj['avatar_url'] = get_avatar_field(
             user_id=sender_id,
             realm_id=sender_realm_id,
-            email=sender_email,
+            email=sender_delivery_email,
             avatar_source=sender_avatar_source,
             avatar_version=sender_avatar_version,
             medium=False,

zerver/tests/test_events.py

@@ -1566,6 +1566,17 @@ class EventsRegisterTest(ZulipTestCase):
                 ('user_id', check_int),
             ])),
         ])
+        avatar_schema_checker = self.check_events_dict([
+            ('type', equals('realm_user')),
+            ('op', equals('update')),
+            ('person', check_dict_only([
+                ('email', check_string),
+                ('user_id', check_int),
+                ('avatar_url', check_string),
+                ('avatar_url_medium', check_string),
+                ('avatar_source', check_string),
+            ])),
+        ])
         do_set_realm_property(self.user_profile.realm, "email_address_visibility",
                               Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS)
         # Important: We need to refresh from the database here so that
@@ -1573,9 +1584,11 @@ class EventsRegisterTest(ZulipTestCase):
         # for email being passed into this next function.
         self.user_profile.refresh_from_db()
         action = lambda: do_change_user_delivery_email(self.user_profile, 'newhamlet@zulip.com')
-        events = self.do_test(action, num_events=1)
+        events = self.do_test(action, num_events=2, client_gravatar=False)
         error = schema_checker('events[0]', events[0])
         self.assert_on_error(error)
+        error = avatar_schema_checker('events[1]', events[1])
+        self.assert_on_error(error)
 
     def do_set_realm_property_test(self, name: str) -> None:
         bool_tests = [True, False, True]  # type: List[bool]
@@ -3378,6 +3391,7 @@ class ClientDescriptorsTest(ZulipTestCase):
                 #       will be useful when we let clients specify
                 #       that they can compute their own gravatar URLs.
                 sender_email=sender.email,
+                sender_delivery_email=sender.delivery_email,
                 sender_realm_id=sender.realm_id,
                 sender_avatar_source=UserProfile.AVATAR_FROM_GRAVATAR,
                 sender_avatar_version=1,

zerver/tests/test_users.py

@@ -194,7 +194,10 @@ class PermissionTest(ZulipTestCase):
         members = result.json()['members']
         hamlet = find_dict(members, 'user_id', user.id)
         self.assertEqual(hamlet['email'], "user%s@zulip.testserver" % (user.id,))
-        self.assertEqual(hamlet['avatar_url'], get_gravatar_url(hamlet['email'], 1))
+        # Note that the Gravatar URL should still be computed from the
+        # `delivery_email`; otherwise, we won't be able to serve the
+        # user's Gravatar.
+        self.assertEqual(hamlet['avatar_url'], get_gravatar_url(user.delivery_email, 1))
         self.assertNotIn('delivery_email', hamlet)
 
         # Also verify the /events code path.  This is a bit hacky, but
@@ -219,7 +222,7 @@ class PermissionTest(ZulipTestCase):
         members = result.json()['members']
         hamlet = find_dict(members, 'user_id', user.id)
         self.assertEqual(hamlet['email'], "user%s@zulip.testserver" % (user.id,))
-        self.assertEqual(hamlet['avatar_url'], get_gravatar_url(hamlet['email'], 1))
+        self.assertEqual(hamlet['avatar_url'], get_gravatar_url(user.email, 1))
         self.assertEqual(hamlet['delivery_email'], self.example_email("hamlet"))
 
     def test_user_cannot_promote_to_admin(self) -> None: