From 8024b1179a9b6d20ba0b64dfbb70cbd419c68204 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 23 Sep 2019 13:51:31 -0700 Subject: [PATCH] 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. --- .../migrations/0242_fix_bot_email_property.py | 25 +++++++++ zerver/models.py | 2 + zerver/tests/test_bots.py | 53 ++++++++++++++++++- 3 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 zerver/migrations/0242_fix_bot_email_property.py diff --git a/zerver/migrations/0242_fix_bot_email_property.py b/zerver/migrations/0242_fix_bot_email_property.py new file mode 100644 index 0000000000..a48227a7a1 --- /dev/null +++ b/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), + ] diff --git a/zerver/models.py b/zerver/models.py index 92aeef1d44..ebff88c992 100644 --- a/zerver/models.py +++ b/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: diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py index 1460ba40e7..24b17726f5 100644 --- a/zerver/tests/test_bots.py +++ b/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