bots: Fix bot email addresses with EMAIL_ADDRESS_VISIBILITY_ADMINS.

When using our EMAIL_ADDRESS_VISIBILITY_ADMINS feature, we were
apparently creating bot users with different email and delivery_email
properties, due to effectively an oversight in how the code was
written (the initial migration handled bots correctly, but not bots
created after the transition).

Following the refactor in the last commit, the fix for this is just
adding the missing conditional, a test, and a database migration to
fix any incorrectly created bots leaked previously.

zerver/migrations/0242_fix_bot_email_property.py

@@ -0,0 +1,25 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.24 on 2019-09-23 20:39
+from __future__ import unicode_literals
+
+from django.db import migrations
+from django.db.backends.postgresql_psycopg2.schema import DatabaseSchemaEditor
+from django.db.migrations.state import StateApps
+
+def fix_bot_email_property(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None:
+    UserProfile = apps.get_model('zerver', 'UserProfile')
+    for user_profile in UserProfile.objects.filter(is_bot=True):
+        if user_profile.email != user_profile.delivery_email:
+            user_profile.email = user_profile.delivery_email
+            user_profile.save(update_fields=["email"])
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zerver', '0241_usermessage_bigint_id_migration_finalize'),
+    ]
+
+    operations = [
+        migrations.RunPython(fix_bot_email_property,
+                             reverse_code=migrations.RunPython.noop),
+    ]

zerver/models.py

@@ -1042,6 +1042,8 @@ class UserProfile(AbstractBaseUser, PermissionsMixin):
     def email_address_is_realm_public(self) -> bool:
         if self.realm.email_address_visibility == Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE:
             return True
+        if self.is_bot:
+            return True
         return False
 
     def can_create_streams(self) -> bool:

zerver/tests/test_bots.py

@@ -7,7 +7,8 @@ from django.test import override_settings
 from mock import patch, MagicMock
 from typing import Any, Dict, List, Mapping, Optional
 
-from zerver.lib.actions import do_change_stream_invite_only, do_deactivate_user
+from zerver.lib.actions import do_change_stream_invite_only, do_deactivate_user, \
+    do_set_realm_property
 from zerver.lib.bot_config import get_bot_config, ConfigError
 from zerver.models import get_realm, get_stream, \
     Realm, UserProfile, get_user, get_bot_services, Service, \
@@ -269,6 +270,56 @@ class BotTest(ZulipTestCase, UploadSerializeMixin):
         assert(profile.default_sending_stream is not None)
         self.assertEqual(profile.default_sending_stream.name, 'Rome')
 
+    def test_add_bot_email_address_visibility(self) -> None:
+        # Test that we don't mangle the email field with
+        # email_address_visiblity limited to admins
+        user = self.example_user("hamlet")
+        do_set_realm_property(user.realm, "email_address_visibility",
+                              Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS)
+        user.refresh_from_db()
+
+        self.login(user.delivery_email)
+        self.assert_num_bots_equal(0)
+        events = []  # type: List[Mapping[str, Any]]
+        with tornado_redirected_to_list(events):
+            result = self.create_bot()
+        self.assert_num_bots_equal(1)
+
+        email = 'hambot-bot@zulip.testserver'
+        bot = self.get_bot_user(email)
+
+        event = [e for e in events if e['event']['type'] == 'realm_bot'][0]
+        self.assertEqual(
+            dict(
+                type='realm_bot',
+                op='add',
+                bot=dict(email='hambot-bot@zulip.testserver',
+                         user_id=bot.id,
+                         bot_type=bot.bot_type,
+                         full_name='The Bot of Hamlet',
+                         is_active=True,
+                         api_key=result['api_key'],
+                         avatar_url=result['avatar_url'],
+                         default_sending_stream=None,
+                         default_events_register_stream=None,
+                         default_all_public_streams=False,
+                         services=[],
+                         # Important: This is the product-facing
+                         # email, not the delivery email, for this
+                         # user.  TODO: Migrate this to an integer ID.
+                         owner=user.email)
+            ),
+            event['event']
+        )
+
+        users_result = self.client_get('/json/users')
+        members = ujson.loads(users_result.content)['members']
+        bots = [m for m in members if m['email'] == 'hambot-bot@zulip.testserver']
+        self.assertEqual(len(bots), 1)
+        bot = bots[0]
+        self.assertEqual(bot['bot_owner'], user.email)
+        self.assertEqual(bot['user_id'], self.get_bot_user(email).id)
+
     def test_bot_add_subscription(self) -> None:
         """
         Calling POST /json/users/me/subscriptions should successfully add