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)