From 35139ac559cc535446293f04ee654de981af075a Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 12 May 2020 11:28:53 -0700 Subject: [PATCH] api: Remove email field from realm_user and realm_bot events. The `email` field for identifying the user being modified in these events was not used by either the webapp or other official Zulip clients. Instead, it was legacy data from before we switched years ago to sending user_id fields as the correct way to uniquely identify a user. --- templates/zerver/api/changelog.md | 7 +++++ zerver/lib/actions.py | 48 ++++++++++++------------------- zerver/lib/events.py | 8 +++--- zerver/tests/test_events.py | 15 ++-------- zerver/tests/test_users.py | 16 +++++------ 5 files changed, 39 insertions(+), 55 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index c8755c4df1..68bce39bab 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,13 @@ below features are supported. ## Changes in Zulip 2.2 +**Feature level 7** +* [`GET /events`](/api/get-events-from-queue): `realm_user` and + `realm_bot` events no longer contain an `email` field to identify + the user; use the `user_id` field instead. Previously, some (but + not all) events of these types contained an `email` key in addition to + to `user_id`) for identifying the modified user. + **Feature level 6** * [`GET /events`](/api/get-events-from-queue): `realm_user` events to update a user's avatar now include the `avatar_version` field, which diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index b9ece499d0..98585a05ba 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -799,15 +799,13 @@ def do_deactivate_user(user_profile: UserProfile, update_license_ledger_if_needed(user_profile.realm, event_time) event = dict(type="realm_user", op="remove", - person=dict(email=user_profile.email, - user_id=user_profile.id, + person=dict(user_id=user_profile.id, full_name=user_profile.full_name)) send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) if user_profile.is_bot: event = dict(type="realm_bot", op="remove", - bot=dict(email=user_profile.email, - user_id=user_profile.id, + bot=dict(user_id=user_profile.id, full_name=user_profile.full_name)) send_event(user_profile.realm, event, bot_owner_user_ids(user_profile)) @@ -3117,8 +3115,7 @@ def do_change_full_name(user_profile: UserProfile, full_name: str, RealmAuditLog.objects.create(realm=user_profile.realm, acting_user=acting_user, modified_user=user_profile, event_type=RealmAuditLog.USER_FULL_NAME_CHANGED, event_time=event_time, extra_data=old_name) - payload = dict(email=user_profile.email, - user_id=user_profile.id, + payload = dict(user_id=user_profile.id, full_name=user_profile.full_name) send_event(user_profile.realm, dict(type='realm_user', op='update', person=payload), @@ -3175,9 +3172,9 @@ def do_change_bot_owner(user_profile: UserProfile, bot_owner: UserProfile, send_event(user_profile.realm, dict(type='realm_bot', op="delete", - bot=dict(email=user_profile.email, - user_id=user_profile.id, - )), + bot=dict( + user_id=user_profile.id, + )), {previous_owner.id, }) # Do not send update event for previous bot owner. update_users = update_users - {previous_owner.id, } @@ -3192,8 +3189,7 @@ def do_change_bot_owner(user_profile: UserProfile, bot_owner: UserProfile, send_event(user_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=user_profile.email, - user_id=user_profile.id, + bot=dict(user_id=user_profile.id, owner_id=user_profile.bot_owner.id, )), update_users) @@ -3239,8 +3235,7 @@ def do_regenerate_api_key(user_profile: UserProfile, acting_user: UserProfile) - send_event(user_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=user_profile.email, - user_id=user_profile.id, + bot=dict(user_id=user_profile.id, api_key=new_api_key, )), bot_owner_user_ids(user_profile)) @@ -3256,8 +3251,7 @@ def notify_avatar_url_change(user_profile: UserProfile) -> None: send_event(user_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=user_profile.email, - user_id=user_profile.id, + bot=dict(user_id=user_profile.id, avatar_url=avatar_url(user_profile), )), bot_owner_user_ids(user_profile)) @@ -3267,7 +3261,8 @@ def notify_avatar_url_change(user_profile: UserProfile) -> None: avatar_url=avatar_url(user_profile), avatar_url_medium=avatar_url(user_profile, medium=True), avatar_version=user_profile.avatar_version, - email=user_profile.email, + # Even clients using client_gravatar don't need the email, + # since we're sending the URL anyway. user_id=user_profile.id ) @@ -3382,8 +3377,7 @@ def do_change_default_sending_stream(user_profile: UserProfile, stream: Optional send_event(user_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=user_profile.email, - user_id=user_profile.id, + bot=dict(user_id=user_profile.id, default_sending_stream=stream_name, )), bot_owner_user_ids(user_profile)) @@ -3405,8 +3399,7 @@ def do_change_default_events_register_stream(user_profile: UserProfile, send_event(user_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=user_profile.email, - user_id=user_profile.id, + bot=dict(user_id=user_profile.id, default_events_register_stream=stream_name, )), bot_owner_user_ids(user_profile)) @@ -3423,8 +3416,7 @@ def do_change_default_all_public_streams(user_profile: UserProfile, value: bool, send_event(user_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=user_profile.email, - user_id=user_profile.id, + bot=dict(user_id=user_profile.id, default_all_public_streams=user_profile.default_all_public_streams, )), bot_owner_user_ids(user_profile)) @@ -3456,8 +3448,7 @@ def do_change_is_admin(user_profile: UserProfile, value: bool, RealmAuditLog.ROLE_COUNT: realm_user_count_by_role(user_profile.realm), })) event = dict(type="realm_user", op="update", - person=dict(email=user_profile.email, - user_id=user_profile.id, + person=dict(user_id=user_profile.id, is_admin=value)) send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) @@ -3480,8 +3471,7 @@ def do_change_is_guest(user_profile: UserProfile, value: bool) -> None: RealmAuditLog.ROLE_COUNT: realm_user_count_by_role(user_profile.realm), })) event = dict(type="realm_user", op="update", - person=dict(email=user_profile.email, - user_id=user_profile.id, + person=dict(user_id=user_profile.id, is_guest=value)) send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) @@ -5640,8 +5630,7 @@ def do_update_outgoing_webhook_service(bot_profile: UserProfile, send_event(bot_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=bot_profile.email, - user_id=bot_profile.id, + bot=dict(user_id=bot_profile.id, services = [dict(base_url=service.base_url, interface=service.interface, token=service.token,)], @@ -5657,8 +5646,7 @@ def do_update_bot_config_data(bot_profile: UserProfile, send_event(bot_profile.realm, dict(type='realm_bot', op='update', - bot=dict(email=bot_profile.email, - user_id=bot_profile.id, + bot=dict(user_id=bot_profile.id, services = [dict(config_data=updated_config_data)], ), ), diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 49d25dfa41..0d31c51ae6 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -467,18 +467,18 @@ def apply_event(state: Dict[str, Any], state['realm_bots'].append(event['bot']) if event['op'] == 'remove': - email = event['bot']['email'] + user_id = event['bot']['user_id'] for bot in state['realm_bots']: - if bot['email'] == email: + if bot['user_id'] == user_id: bot['is_active'] = False if event['op'] == 'delete': state['realm_bots'] = [item for item - in state['realm_bots'] if item['email'] != event['bot']['email']] + in state['realm_bots'] if item['user_id'] != event['bot']['user_id']] if event['op'] == 'update': for bot in state['realm_bots']: - if bot['email'] == event['bot']['email']: + if bot['user_id'] == event['bot']['user_id']: if 'owner_id' in event['bot']: bot_owner_id = event['bot']['owner_id'] bot['owner_id'] = bot_owner_id diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index de57989fb7..6d3399add6 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -474,7 +474,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_bot')), ('op', equals('update')), ('bot', check_dict_only([ - ('email', check_string), ('user_id', check_int), (field_name, check), ])), @@ -1532,12 +1531,11 @@ class EventsRegisterTest(ZulipTestCase): ('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_version', check_int), ('avatar_source', check_string), + ('user_id', check_int), ])), ]) events = self.do_test( @@ -1554,7 +1552,6 @@ class EventsRegisterTest(ZulipTestCase): ('avatar_url', check_none_or(check_string)), ('avatar_url_medium', check_none_or(check_string)), ('avatar_version', check_int), - ('email', check_string), ('user_id', check_int), ])), ]) @@ -1569,7 +1566,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_user')), ('op', equals('update')), ('person', check_dict_only([ - ('email', check_string), ('full_name', check_string), ('user_id', check_int), ])), @@ -1591,12 +1587,11 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_user')), ('op', equals('update')), ('person', check_dict_only([ - ('email', check_string), - ('user_id', check_int), ('avatar_source', check_string), ('avatar_url', check_string), ('avatar_url_medium', check_string), ('avatar_version', check_int), + ('user_id', check_int), ])), ]) do_set_realm_property(self.user_profile.realm, "email_address_visibility", @@ -1836,7 +1831,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_user')), ('op', equals('update')), ('person', check_dict_only([ - ('email', check_string), ('is_admin', check_bool), ('user_id', check_int), ])), @@ -1907,7 +1901,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_user')), ('op', equals('update')), ('person', check_dict_only([ - ('email', check_string), ('user_id', check_int), ('timezone', check_string), ])), @@ -2259,7 +2252,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_bot')), ('op', equals('update')), ('bot', check_dict_only([ - ('email', check_string), ('user_id', check_int), ('owner_id', check_int), ])), @@ -2278,7 +2270,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_bot')), ('op', equals('delete')), ('bot', check_dict_only([ - ('email', check_string), ('user_id', check_int), ])), ]) @@ -2326,7 +2317,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_bot')), ('op', equals('update')), ('bot', check_dict_only([ - ('email', check_string), ('user_id', check_int), ('services', check_list(check_dict_only([ ('base_url', check_url), @@ -2352,7 +2342,6 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('realm_bot')), ('op', equals('remove')), ('bot', check_dict_only([ - ('email', check_string), ('full_name', check_string), ('user_id', check_int), ])), diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index ac96a8abaa..db8f480f7e 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -157,7 +157,7 @@ class PermissionTest(ZulipTestCase): admin_users = realm.get_human_admin_users() self.assertTrue(othello in admin_users) person = events[0]['event']['person'] - self.assertEqual(person['email'], othello.email) + self.assertEqual(person['user_id'], othello.id) self.assertEqual(person['is_admin'], True) # Taketh away @@ -169,7 +169,7 @@ class PermissionTest(ZulipTestCase): admin_users = realm.get_human_admin_users() self.assertFalse(othello in admin_users) person = events[0]['event']['person'] - self.assertEqual(person['email'], othello.email) + self.assertEqual(person['user_id'], othello.id) self.assertEqual(person['is_admin'], False) # Cannot take away from last admin @@ -182,7 +182,7 @@ class PermissionTest(ZulipTestCase): admin_users = realm.get_human_admin_users() self.assertFalse(hamlet in admin_users) person = events[0]['event']['person'] - self.assertEqual(person['email'], hamlet.email) + self.assertEqual(person['user_id'], hamlet.id) self.assertEqual(person['is_admin'], False) with tornado_redirected_to_list([]): result = self.client_patch('/json/users/{}'.format(iago.id), req) @@ -383,7 +383,7 @@ class PermissionTest(ZulipTestCase): self.assertTrue(hamlet.is_guest) self.assertFalse(hamlet.can_access_all_realm_members()) person = events[0]['event']['person'] - self.assertEqual(person['email'], hamlet.email) + self.assertEqual(person['user_id'], hamlet.id) self.assertTrue(person['is_guest']) def test_change_guest_to_regular_member(self) -> None: @@ -401,7 +401,7 @@ class PermissionTest(ZulipTestCase): polonius = self.example_user("polonius") self.assertFalse(polonius.is_guest) person = events[0]['event']['person'] - self.assertEqual(person['email'], polonius.email) + self.assertEqual(person['user_id'], polonius.id) self.assertFalse(person['is_guest']) def test_change_admin_to_guest(self) -> None: @@ -431,11 +431,11 @@ class PermissionTest(ZulipTestCase): self.assertFalse(hamlet.is_realm_admin) person = events[0]['event']['person'] - self.assertEqual(person['email'], hamlet.email) + self.assertEqual(person['user_id'], hamlet.id) self.assertFalse(person['is_admin']) person = events[1]['event']['person'] - self.assertEqual(person['email'], hamlet.email) + self.assertEqual(person['user_id'], hamlet.id) self.assertTrue(person['is_guest']) def test_change_guest_to_admin(self) -> None: @@ -464,7 +464,7 @@ class PermissionTest(ZulipTestCase): self.assertTrue(polonius.is_realm_admin) person = events[0]['event']['person'] - self.assertEqual(person['email'], polonius.email) + self.assertEqual(person['user_id'], polonius.id) self.assertTrue(person['is_admin']) def test_admin_user_can_change_profile_data(self) -> None: