From 2396e6858f2a716de9c4341c47935526fd5f1ff5 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Wed, 25 May 2022 16:43:31 +0530 Subject: [PATCH] users: Always pass "delivery_email" field in user objects. Previously, user objects contained delivery_email field only when user had access to real email. Also, delivery_email was not present if visibility setting is set to "everyone" as email field was itself set to real email. This commit changes the code to pass "delivery_email" field always in the user objects with its value being "None" if user does not have access to real email and real email otherwise. The "delivery_email" field value is None for logged-out users. For bots, the "delivery_email" is always set to real email irrespective of email_address_visibility setting. Also, since user has access to real email if visibility is set to "everyone", "delivery_email" field is passed in that case too. There is no change in email field and it is same as before. This commit also adds code to send event to update delivery_email field when email_address_visibility setting changes to all the users whose access to emails changes and also changes the code to send event on changing delivery_email to users who have access to email. --- api_docs/changelog.md | 13 ++++++ frontend_tests/node_tests/lib/events.js | 1 + zerver/actions/create_user.py | 4 +- zerver/actions/realm_settings.py | 12 ++++- zerver/actions/user_settings.py | 60 ++++++++++++++++++++++--- zerver/lib/event_schema.py | 3 +- zerver/lib/users.py | 30 ++++++++++++- zerver/openapi/zulip.yaml | 54 +++++++++++++++++----- zerver/tests/test_bots.py | 8 ++-- zerver/tests/test_event_system.py | 23 ++++------ zerver/tests/test_events.py | 43 ++++++++++++++++-- zerver/tests/test_example.py | 1 + zerver/tests/test_users.py | 6 +-- 13 files changed, 210 insertions(+), 48 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index decdb197a6..69b739360e 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,19 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 7.0 +**Feature level 163** + +* [`GET /users`](/api/get-users), [`GET /users/{user_id}`](/api/get-user), + [`GET /users/{email}`](/api/get-user-by-email), + [`GET /users/me`](/api/get-own-user) and [`GET /events`](/api/get-events): + The `delivery_email` field is always present in user objects, including the case + when `email_address_visibility` is set to `EMAIL_ADDRESS_VISIBILITY_EVERYONE,` + with the value being `None` if the requestor does not have access to the user's + real email. For bot users, the `delivery_email` field is always set to the real email. +* [`GET /events`](/api/get-events): Event for updating `delivery_email` is now sent to + all users who have access to it and is also sent when `email_address_visibility` setting + changes. + **Feature level 162** * [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events), diff --git a/frontend_tests/node_tests/lib/events.js b/frontend_tests/node_tests/lib/events.js index cf1f90f762..cbdf7baddf 100644 --- a/frontend_tests/node_tests/lib/events.js +++ b/frontend_tests/node_tests/lib/events.js @@ -547,6 +547,7 @@ exports.fixtures = { profile_data: {}, timezone: "America/New_York", date_joined: "2020-01-01", + delivery_email: "test-delivery@example.com", }, }, diff --git a/zerver/actions/create_user.py b/zerver/actions/create_user.py index d8ea6a27b7..1f9d2cabe3 100644 --- a/zerver/actions/create_user.py +++ b/zerver/actions/create_user.py @@ -306,7 +306,7 @@ def notify_created_user(user_profile: UserProfile) -> None: user_ids_without_real_email_access = [] for user in active_users: if can_access_delivery_email( - user, user_profile.id, user_profile.realm.email_address_visibility + user, user_profile.id, user_profile.realm.email_address_visibility, user_row["is_bot"] ): user_ids_with_real_email_access.append(user.id) else: @@ -322,7 +322,7 @@ def notify_created_user(user_profile: UserProfile) -> None: ) if user_ids_without_real_email_access: - del person["delivery_email"] + person["delivery_email"] = None event = dict(type="realm_user", op="add", person=person) transaction.on_commit( lambda event=event: send_event( diff --git a/zerver/actions/realm_settings.py b/zerver/actions/realm_settings.py index 2e7b87f329..968b537e70 100644 --- a/zerver/actions/realm_settings.py +++ b/zerver/actions/realm_settings.py @@ -12,7 +12,11 @@ from confirmation.models import Confirmation, create_confirmation_link, generate from zerver.actions.custom_profile_fields import do_remove_realm_custom_profile_fields from zerver.actions.message_delete import do_delete_messages_by_sender from zerver.actions.user_groups import update_users_in_full_members_system_group -from zerver.actions.user_settings import do_delete_avatar_image, send_user_email_update_event +from zerver.actions.user_settings import ( + do_delete_avatar_image, + send_delivery_email_update_events, + send_user_email_update_event, +) from zerver.lib.cache import flush_user_profile from zerver.lib.create_user import get_display_email_address from zerver.lib.message import parse_message_time_limit_setting, update_first_visible_message_id @@ -95,6 +99,11 @@ def do_set_realm_property( ) if name == "email_address_visibility": + user_profiles = UserProfile.objects.filter(realm=realm, is_bot=False) + + for user_profile in user_profiles: + send_delivery_email_update_events(user_profile, old_value, value) + if Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE not in [old_value, value]: # We use real email addresses on UserProfile.email only if # EMAIL_ADDRESS_VISIBILITY_EVERYONE is configured, so @@ -102,7 +111,6 @@ def do_set_realm_property( # that field, so we can save work and return here. return - user_profiles = UserProfile.objects.filter(realm=realm, is_bot=False) for user_profile in user_profiles: user_profile.email = get_display_email_address(user_profile) UserProfile.objects.bulk_update(user_profiles, ["email"]) diff --git a/zerver/actions/user_settings.py b/zerver/actions/user_settings.py index 12a06eda5a..736d5340a8 100644 --- a/zerver/actions/user_settings.py +++ b/zerver/actions/user_settings.py @@ -20,7 +20,12 @@ from zerver.lib.queue import queue_json_publish from zerver.lib.send_email import FromAddress, clear_scheduled_emails, send_email from zerver.lib.timezone import canonicalize_timezone from zerver.lib.upload import delete_avatar_image -from zerver.lib.users import check_bot_name_available, check_full_name +from zerver.lib.users import ( + can_access_delivery_email, + check_bot_name_available, + check_full_name, + get_users_with_access_to_real_email, +) from zerver.lib.utils import generate_api_key from zerver.models import ( Draft, @@ -50,6 +55,49 @@ def send_user_email_update_event(user_profile: UserProfile) -> None: ) +def send_delivery_email_update_events( + user_profile: UserProfile, old_visibility_setting: int, visibility_setting: int +) -> None: + active_users = user_profile.realm.get_active_users() + delivery_email_now_visible_user_ids = [] + delivery_email_now_invisible_user_ids = [] + + for active_user in active_users: + could_access_delivery_email_previously = can_access_delivery_email( + active_user, user_profile.id, old_visibility_setting, user_profile.is_bot + ) + can_access_delivery_email_now = can_access_delivery_email( + active_user, user_profile.id, visibility_setting, user_profile.is_bot + ) + + if could_access_delivery_email_previously != can_access_delivery_email_now: + if can_access_delivery_email_now: + delivery_email_now_visible_user_ids.append(active_user.id) + else: + delivery_email_now_invisible_user_ids.append(active_user.id) + + if delivery_email_now_visible_user_ids: + person = dict(user_id=user_profile.id, delivery_email=user_profile.delivery_email) + event = dict(type="realm_user", op="update", person=person) + transaction.on_commit( + lambda event=event: send_event( + user_profile.realm, + event, + delivery_email_now_visible_user_ids, + ) + ) + if delivery_email_now_invisible_user_ids: + person = dict(user_id=user_profile.id, delivery_email=None) + event = dict(type="realm_user", op="update", person=person) + transaction.on_commit( + lambda event=event: send_event( + user_profile.realm, + event, + delivery_email_now_invisible_user_ids, + ) + ) + + @transaction.atomic(savepoint=False) def do_change_user_delivery_email(user_profile: UserProfile, new_email: str) -> None: delete_user_profile_caches([user_profile]) @@ -61,12 +109,14 @@ def do_change_user_delivery_email(user_profile: UserProfile, new_email: str) -> else: user_profile.save(update_fields=["delivery_email"]) - # We notify just the target user (and eventually org admins, only - # when email_address_visibility=EMAIL_ADDRESS_VISIBILITY_ADMINS) - # about their new delivery email, since that field is private. + # We notify all the users who have access to delivery email. payload = dict(user_id=user_profile.id, delivery_email=new_email) event = dict(type="realm_user", op="update", person=payload) - transaction.on_commit(lambda: send_event(user_profile.realm, event, [user_profile.id])) + delivery_email_visible_user_ids = get_users_with_access_to_real_email(user_profile) + + transaction.on_commit( + lambda: send_event(user_profile.realm, event, delivery_email_visible_user_ids) + ) if user_profile.avatar_source == UserProfile.AVATAR_FROM_GRAVATAR: # If the user is using Gravatar to manage their email address, diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 878cb9512e..d16c75d9b2 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -1085,6 +1085,7 @@ realm_user_type = DictType( ("profile_data", StringDictType(dict)), ("timezone", str), ("date_joined", str), + ("delivery_email", OptionalType(str)), ] ) @@ -1157,7 +1158,7 @@ realm_user_person_types = dict( required_keys=[ # vertical formatting ("user_id", int), - ("delivery_email", str), + ("delivery_email", OptionalType(str)), ], ), email=DictType( diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 16d6192d24..193dfe1085 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -396,11 +396,20 @@ def validate_user_custom_profile_data( def can_access_delivery_email( - user_profile: UserProfile, target_user_id: int, email_address_visibility: int + user_profile: UserProfile, + target_user_id: int, + email_address_visibility: int, + target_user_is_bot: bool, ) -> bool: if target_user_id == user_profile.id: return True + if target_user_is_bot: + return True + + if email_address_visibility == Realm.EMAIL_ADDRESS_VISIBILITY_EVERYONE: + return True + if email_address_visibility == Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS: return user_profile.is_realm_admin @@ -483,9 +492,11 @@ def format_user_row( ) if acting_user is not None and can_access_delivery_email( - acting_user, row["id"], realm.email_address_visibility + acting_user, row["id"], realm.email_address_visibility, row["is_bot"] ): result["delivery_email"] = row["delivery_email"] + else: + result["delivery_email"] = None if is_bot: result["bot_type"] = row["bot_type"] @@ -635,3 +646,18 @@ def is_2fa_verified(user: UserProfile) -> bool: # is True before calling `is_2fa_verified`. assert settings.TWO_FACTOR_AUTHENTICATION_ENABLED return is_verified(user) + + +def get_users_with_access_to_real_email(user_profile: UserProfile) -> List[int]: + active_users = user_profile.realm.get_active_users() + user_ids_with_real_email_access = [] + for user in active_users: + if can_access_delivery_email( + user, + user_profile.id, + user_profile.realm.email_address_visibility, + user_profile.is_bot, + ): + user_ids_with_real_email_access.append(user.id) + + return user_ids_with_real_email_access diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index bdd32299c5..1187393309 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -504,9 +504,13 @@ paths: - type: object additionalProperties: false description: | - When the delivery email of a user changes. + When the value of the user's delivery email as visible to you changes, + either due to the email address changing or your access to the user's + email via their `email_address_visibility` policy changing. - Note: This event is only visible to admins. + **Changes**: Prior to Zulip 7.0 (feature level 163), this event was + sent only to the affected user, and this event would only be triggered + by changing the affected user's delivery email. properties: user_id: type: integer @@ -514,8 +518,17 @@ paths: The ID of the user affected by this change. delivery_email: type: string + nullable: true description: | The new delivery email of the user. + + This value can be `None` if user changes the + `email_address_visibility` value such that + you cannot access user's real email. + + **Changes**: As of Zulip 7.0 (feature level 163), + `None` is a possible value as this event is also + sent on changing `email_address_visibility` setting. - type: object additionalProperties: false description: | @@ -7225,11 +7238,20 @@ paths: example: 1 delivery_email: type: string + nullable: true description: | - The user's real email address. This field is present only if - [email address visibility](/help/restrict-visibility-of-email-addresses) is - limited and you are an administrator with access to real email addresses - under the configured policy. + The user's real email address. This value will be `None` if you cannot + access this user's real email address. For bot users, this field is + always set to the real email of the bot, since everyone can access the + bot's email irrespective of `email_address_visibility` setting. + + **Changes**: Prior to Zulip 7.0 (feature level 163), this field was present + only when `email_address_visibility` was restricted and you had access to + the user's real email. Now, this field is always present, including the case + when `email_address_visibility` is set to `EMAIL_ADDRESS_VISIBILITY_EVERYONE`, + with the value being `None` if you don't have access to the user's real email. + For bot users, this field is now always set to the real email irrespective of + `email_address_visibility` setting. profile_data: $ref: "#/components/schemas/profile_data" example: @@ -16814,7 +16836,8 @@ components: - additionalProperties: false properties: user_id: {} - delivery_email: {} + delivery_email: + nullable: true email: {} full_name: {} date_joined: {} @@ -16845,11 +16868,20 @@ components: The unique ID of the user. delivery_email: type: string + nullable: true description: | - The user's real email address. This field is present only if - [email address visibility](/help/restrict-visibility-of-email-addresses) is - limited and you are an administrator with access to real email addresses - under the configured policy. + The user's real email address. This value will be `None` if you cannot + access user's real email address. For bot users, this field is always + set to the real email of the bot, since everyone can access the bot's + email irrespective of `email_address_visibility` setting. + + **Changes**: Prior to Zulip 7.0 (feature level 163), this field was present + only when `email_address_visibility` was restricted and you had access to the + user's real email. Now, this field is always present, including the case when + `email_address_visibility` is set to `EMAIL_ADDRESS_VISIBILITY_EVERYONE`, + with the value being `None` if you don't have access the user's real email. + For bot users, this field is now always set to the real email irrespective of + `email_address_visibility` setting. email: type: string description: | diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py index 88fcffcd96..970c40def1 100644 --- a/zerver/tests/test_bots.py +++ b/zerver/tests/test_bots.py @@ -163,7 +163,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=5): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot() self.assert_num_bots_equal(1) @@ -329,7 +329,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login_user(user) self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=5): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot() self.assert_num_bots_equal(1) @@ -427,7 +427,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=5): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot(default_sending_stream="Denmark") self.assert_num_bots_equal(1) self.assertEqual(result["default_sending_stream"], "Denmark") @@ -511,7 +511,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] - with self.tornado_redirected_to_list(events, expected_num_events=5): + with self.tornado_redirected_to_list(events, expected_num_events=4): result = self.create_bot(default_events_register_stream="Denmark") self.assert_num_bots_equal(1) self.assertEqual(result["default_events_register_stream"], "Denmark") diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index a4f1f749f8..e14f5b298d 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -606,6 +606,7 @@ class FetchInitialStateDataTest(ZulipTestCase): def test_delivery_email_presence_for_non_admins(self) -> None: user_profile = self.example_user("aaron") self.assertFalse(user_profile.is_realm_admin) + hamlet = self.example_user("hamlet") do_set_realm_property( user_profile.realm, @@ -615,11 +616,8 @@ class FetchInitialStateDataTest(ZulipTestCase): ) result = fetch_initial_state_data(user_profile) - for key, value in result["raw_users"].items(): - if key == user_profile.id: - self.assertEqual(value["delivery_email"], user_profile.delivery_email) - else: - self.assertNotIn("delivery_email", value) + (hamlet_obj,) = (value for key, value in result["raw_users"].items() if key == hamlet.id) + self.assertEqual(hamlet_obj["delivery_email"], hamlet.delivery_email) do_set_realm_property( user_profile.realm, @@ -629,15 +627,13 @@ class FetchInitialStateDataTest(ZulipTestCase): ) result = fetch_initial_state_data(user_profile) - for key, value in result["raw_users"].items(): - if key == user_profile.id: - self.assertEqual(value["delivery_email"], user_profile.delivery_email) - else: - self.assertNotIn("delivery_email", value) + (hamlet_obj,) = (value for key, value in result["raw_users"].items() if key == hamlet.id) + self.assertIsNone(hamlet_obj["delivery_email"]) def test_delivery_email_presence_for_admins(self) -> None: user_profile = self.example_user("iago") self.assertTrue(user_profile.is_realm_admin) + hamlet = self.example_user("hamlet") do_set_realm_property( user_profile.realm, @@ -647,11 +643,8 @@ class FetchInitialStateDataTest(ZulipTestCase): ) result = fetch_initial_state_data(user_profile) - for key, value in result["raw_users"].items(): - if key == user_profile.id: - self.assertEqual(value["delivery_email"], user_profile.delivery_email) - else: - self.assertNotIn("delivery_email", value) + (hamlet_obj,) = (value for key, value in result["raw_users"].items() if key == hamlet.id) + self.assertEqual(hamlet_obj["delivery_email"], hamlet.delivery_email) do_set_realm_property( user_profile.realm, diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 7e57e4b4ff..00fe86cc3e 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -2635,8 +2635,11 @@ class RealmPropertyActionTest(BaseAction): old_value, val, ]: - # email update event is sent for each user. - num_events = 11 + # one event for updating "email_address_visibility" setting + + # one delivery_email update event is sent for 9 users (all human + # users except the user themselves) + one email update event for + # each of the 10 human users in the realm. + num_events = 20 events = self.verify_action( lambda: do_set_realm_property( @@ -2676,7 +2679,8 @@ class RealmPropertyActionTest(BaseAction): old_value, val, ]: - check_realm_user_update("events[1]", events[1], "email") + check_realm_user_update("events[1]", events[1], "delivery_email") + check_realm_user_update("events[10]", events[10], "email") def test_change_realm_property(self) -> None: for prop in Realm.property_types: @@ -2832,6 +2836,39 @@ class UserDisplayActionTest(BaseAction): check_update_display_settings("events[1]", events[1]) check_realm_user_update("events[2]", events[2], "timezone") + def test_delivery_email_events_on_changing_email_address_visibility(self) -> None: + do_change_user_role(self.user_profile, UserProfile.ROLE_MODERATOR, acting_user=None) + do_set_realm_property( + self.user_profile.realm, + "email_address_visibility", + Realm.EMAIL_ADDRESS_VISIBILITY_MODERATORS, + acting_user=None, + ) + + events = self.verify_action( + lambda: do_set_realm_property( + self.user_profile.realm, + "email_address_visibility", + Realm.EMAIL_ADDRESS_VISIBILITY_ADMINS, + acting_user=self.user_profile, + ), + num_events=10, + ) + check_realm_user_update("events[1]", events[1], "delivery_email") + self.assertIsNone(events[1]["person"]["delivery_email"]) + + events = self.verify_action( + lambda: do_set_realm_property( + self.user_profile.realm, + "email_address_visibility", + Realm.EMAIL_ADDRESS_VISIBILITY_MODERATORS, + acting_user=self.user_profile, + ), + num_events=10, + ) + check_realm_user_update("events[1]", events[1], "delivery_email") + self.assertIsNotNone(events[1]["person"]["delivery_email"]) + class SubscribeActionTest(BaseAction): def test_subscribe_events(self) -> None: diff --git a/zerver/tests/test_example.py b/zerver/tests/test_example.py index ae1e36c8f9..3e26fc540a 100644 --- a/zerver/tests/test_example.py +++ b/zerver/tests/test_example.py @@ -128,6 +128,7 @@ class TestFullStack(ZulipTestCase): avatar_url=content["user"]["avatar_url"], avatar_version=1, date_joined=content["user"]["date_joined"], + delivery_email=None, email=cordelia.email, full_name=cordelia.full_name, is_active=True, diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 896b8e5b4e..5b1b88683a 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -2211,17 +2211,17 @@ class GetProfileTest(ZulipTestCase): # is set to EMAIL_ADDRESS_VISIBILITY_EVERYONE. self.login("shiva") result = orjson.loads(self.client_get(f"/json/users/{hamlet.id}").content) - self.assertEqual(result["user"].get("delivery_email"), None) + self.assertEqual(result["user"].get("delivery_email"), hamlet.delivery_email) self.assertEqual(result["user"].get("email"), hamlet.delivery_email) self.login("cordelia") result = orjson.loads(self.client_get(f"/json/users/{hamlet.id}").content) - self.assertEqual(result["user"].get("delivery_email"), None) + self.assertEqual(result["user"].get("delivery_email"), hamlet.delivery_email) self.assertEqual(result["user"].get("email"), hamlet.delivery_email) self.login("polonius") result = orjson.loads(self.client_get(f"/json/users/{hamlet.id}").content) - self.assertEqual(result["user"].get("delivery_email"), None) + self.assertEqual(result["user"].get("delivery_email"), hamlet.delivery_email) self.assertEqual(result["user"].get("email"), hamlet.delivery_email)