users: Send events only to users who can access the modified user.

This commit adds code to make sure that update events for changing
a user's role, email, etc. are not sent to guests who cannot access
the modified user.

api_docs/changelog.md

@@ -20,6 +20,11 @@ format used by the Zulip server that they are interacting with.
 
 ## Changes in Zulip 8.0
 
+**Feature level 228**
+
+* [`GET /events`](/api/get-events): `realm_user` events with `op: "update"`
+  are now only sent to users who can access the modified user.
+
 **Feature level 227**
 
 * [`PATCH /realm/user_settings_defaults`](/api/update-realm-user-settings-defaults),

zerver/actions/custom_profile_fields.py

@@ -8,6 +8,7 @@ from zerver.lib.exceptions import JsonableError
 from zerver.lib.external_accounts import DEFAULT_EXTERNAL_ACCOUNTS
 from zerver.lib.streams import render_stream_description
 from zerver.lib.types import ProfileDataElementUpdateDict, ProfileFieldData
+from zerver.lib.users import get_user_ids_who_can_access_user
 from zerver.models import (
     CustomProfileField,
     CustomProfileFieldValue,
@@ -139,7 +140,7 @@ def notify_user_update_custom_profile_data(
         data["rendered_value"] = field["rendered_value"]
     payload = dict(user_id=user_profile.id, custom_profile_field=data)
     event = dict(type="realm_user", op="update", person=payload)
-    send_event(user_profile.realm, event, active_user_ids(user_profile.realm.id))
+    send_event(user_profile.realm, event, get_user_ids_who_can_access_user(user_profile))
 
 
 def do_update_user_custom_profile_data_if_changed(

zerver/actions/user_settings.py

@@ -25,7 +25,9 @@ from zerver.lib.users import (
     can_access_delivery_email,
     check_bot_name_available,
     check_full_name,
+    get_user_ids_who_can_access_user,
     get_users_with_access_to_real_email,
+    user_access_restricted_in_realm,
 )
 from zerver.lib.utils import generate_api_key
 from zerver.models import (
@@ -36,7 +38,6 @@ from zerver.models import (
     ScheduledMessageNotificationEmail,
     UserPresence,
     UserProfile,
-    active_user_ids,
     bot_owner_user_ids,
     get_client,
     get_user_profile_by_id,
@@ -50,14 +51,27 @@ def send_user_email_update_event(user_profile: UserProfile) -> None:
     send_event_on_commit(
         user_profile.realm,
         event,
-        active_user_ids(user_profile.realm_id),
+        get_user_ids_who_can_access_user(user_profile),
     )
 
 
 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()
+    if not user_access_restricted_in_realm(user_profile):
+        active_users = user_profile.realm.get_active_users()
+    else:
+        # The get_user_ids_who_can_access_user returns user IDs and not
+        # user objects and we instead do one more query for UserProfile
+        # objects. We need complete UserProfile objects only for a couple
+        # of cases and it is not worth to query the whole UserProfile
+        # objects in all the cases and it is fine to do the extra query
+        # wherever needed.
+        user_ids_who_can_access_user = get_user_ids_who_can_access_user(user_profile)
+        active_users = UserProfile.objects.filter(
+            id__in=user_ids_who_can_access_user, is_active=True
+        )
+
     delivery_email_now_visible_user_ids = []
     delivery_email_now_invisible_user_ids = []
 
@@ -213,7 +227,7 @@ def do_change_full_name(
     send_event(
         user_profile.realm,
         dict(type="realm_user", op="update", person=payload),
-        active_user_ids(user_profile.realm_id),
+        get_user_ids_who_can_access_user(user_profile),
     )
     if user_profile.is_bot:
         send_event(
@@ -343,7 +357,7 @@ def notify_avatar_url_change(user_profile: UserProfile) -> None:
     send_event_on_commit(
         user_profile.realm,
         event,
-        active_user_ids(user_profile.realm_id),
+        get_user_ids_who_can_access_user(user_profile),
     )
 
 
@@ -488,7 +502,7 @@ def do_change_user_setting(
         send_event_on_commit(
             user_profile.realm,
             timezone_event,
-            active_user_ids(user_profile.realm_id),
+            get_user_ids_who_can_access_user(user_profile),
         )
 
     if setting_name == "email_address_visibility":

zerver/actions/users.py

@@ -24,7 +24,7 @@ from zerver.lib.stream_traffic import get_streams_traffic
 from zerver.lib.streams import get_streams_for_user, stream_to_dict
 from zerver.lib.user_counts import realm_user_count_by_role
 from zerver.lib.user_groups import get_system_user_group_for_user
-from zerver.lib.users import get_active_bots_owned_by_user
+from zerver.lib.users import get_active_bots_owned_by_user, get_user_ids_who_can_access_user
 from zerver.models import (
     Message,
     Realm,
@@ -35,7 +35,6 @@ from zerver.models import (
     Subscription,
     UserGroupMembership,
     UserProfile,
-    active_user_ids,
     bot_owner_user_ids,
     get_bot_dicts_in_realm,
     get_bot_services,
@@ -305,7 +304,9 @@ def do_deactivate_user(
             person=dict(user_id=user_profile.id, is_active=False),
         )
         send_event_on_commit(
-            user_profile.realm, event_deactivate_user, active_user_ids(user_profile.realm_id)
+            user_profile.realm,
+            event_deactivate_user,
+            get_user_ids_who_can_access_user(user_profile),
         )
 
         if user_profile.is_bot:
@@ -402,7 +403,7 @@ def do_change_user_role(
     event = dict(
         type="realm_user", op="update", person=dict(user_id=user_profile.id, role=user_profile.role)
     )
-    send_event_on_commit(user_profile.realm, event, active_user_ids(user_profile.realm_id))
+    send_event_on_commit(user_profile.realm, event, get_user_ids_who_can_access_user(user_profile))
 
     UserGroupMembership.objects.filter(
         user_profile=user_profile, user_group=old_system_group
@@ -450,7 +451,7 @@ def do_change_is_billing_admin(user_profile: UserProfile, value: bool) -> None:
     event = dict(
         type="realm_user", op="update", person=dict(user_id=user_profile.id, is_billing_admin=value)
     )
-    send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id))
+    send_event(user_profile.realm, event, get_user_ids_who_can_access_user(user_profile))
 
 
 def do_change_can_forge_sender(user_profile: UserProfile, value: bool) -> None:

zerver/lib/users.py

@@ -37,6 +37,8 @@ from zerver.models import (
     SystemGroups,
     UserMessage,
     UserProfile,
+    active_non_guest_user_ids,
+    active_user_ids,
     get_fake_email_domain,
     get_realm_user_dicts,
     get_user,
@@ -587,8 +589,31 @@ def check_can_access_user(
     return False
 
 
+def get_user_ids_who_can_access_user(target_user: UserProfile) -> List[int]:
+    # We assume that caller only needs active users here, since
+    # this function is used to get users to send events.
+    realm = target_user.realm
+    if not user_access_restricted_in_realm(target_user):
+        return active_user_ids(realm.id)
+
+    active_non_guest_user_ids_in_realm = active_non_guest_user_ids(realm.id)
+
+    users_in_subscribed_streams_or_huddles_dict = get_subscribers_of_target_user_subscriptions(
+        [target_user]
+    )
+    users_involved_in_dms_dict = get_users_involved_in_dms_with_target_users([target_user], realm)
+
+    user_ids_who_can_access_target_user = (
+        {target_user.id}
+        | set(active_non_guest_user_ids_in_realm)
+        | users_in_subscribed_streams_or_huddles_dict[target_user.id]
+        | users_involved_in_dms_dict[target_user.id]
+    )
+    return list(user_ids_who_can_access_target_user)
+
+
 def get_subscribers_of_target_user_subscriptions(
-    target_users: List[UserProfile],
+    target_users: List[UserProfile], include_deactivated_users_for_huddles: bool = False
 ) -> Dict[int, Set[int]]:
     target_user_ids = [user.id for user in target_users]
     target_user_subscriptions = (
@@ -616,10 +641,15 @@ def get_subscribers_of_target_user_subscriptions(
         active=True,
     )
 
-    subs_in_target_user_subscriptions_query = subs_in_target_user_subscriptions_query.filter(
+    if include_deactivated_users_for_huddles:
-        Q(recipient__type=Recipient.STREAM, is_user_active=True)
+        subs_in_target_user_subscriptions_query = subs_in_target_user_subscriptions_query.filter(
-        | Q(recipient__type=Recipient.HUDDLE)
+            Q(recipient__type=Recipient.STREAM, is_user_active=True)
-    )
+            | Q(recipient__type=Recipient.HUDDLE)
+        )
+    else:
+        subs_in_target_user_subscriptions_query = subs_in_target_user_subscriptions_query.filter(
+            recipient__type__in=[Recipient.STREAM, Recipient.HUDDLE], is_user_active=True
+        )
 
     subs_in_target_user_subscriptions = subs_in_target_user_subscriptions_query.order_by(
         "recipient_id"
@@ -644,7 +674,7 @@ def get_subscribers_of_target_user_subscriptions(
 
 
 def get_users_involved_in_dms_with_target_users(
-    target_users: List[UserProfile], realm: Realm
+    target_users: List[UserProfile], realm: Realm, include_deactivated_users: bool = False
 ) -> Dict[int, Set[int]]:
     target_user_ids = [user.id for user in target_users]
 
@@ -657,21 +687,35 @@ def get_users_involved_in_dms_with_target_users(
         .values("sender_id", "recipient__type_id")
     )
 
+    direct_messages_recipient_users_set = {
+        obj["recipient__type_id"] for obj in direct_messages_recipient_users
+    }
+    active_direct_messages_recipient_user_ids = UserProfile.objects.filter(
+        id__in=list(direct_messages_recipient_users_set), is_active=True
+    ).values_list("id", flat=True)
+
     direct_message_participants_dict: Dict[int, Set[int]] = defaultdict(set)
     for sender_id, message_rows in itertools.groupby(
         direct_messages_recipient_users, itemgetter("sender_id")
     ):
         recipient_user_ids = {row["recipient__type_id"] for row in message_rows}
+        if not include_deactivated_users:
+            recipient_user_ids &= set(active_direct_messages_recipient_user_ids)
+
         direct_message_participants_dict[sender_id] = recipient_user_ids
 
     personal_recipient_ids_for_target_users = [user.recipient_id for user in target_users]
+    direct_message_senders_query = Message.objects.filter(
+        realm=realm,
+        recipient_id__in=personal_recipient_ids_for_target_users,
+        recipient__type=Recipient.PERSONAL,
+    )
+
+    if not include_deactivated_users:
+        direct_message_senders_query = direct_message_senders_query.filter(sender__is_active=True)
+
     direct_messages_senders = (
-        Message.objects.filter(
+        direct_message_senders_query.order_by("recipient__type_id")
-            realm=realm,
-            recipient_id__in=personal_recipient_ids_for_target_users,
-            recipient__type=Recipient.PERSONAL,
-        )
-        .order_by("recipient__type_id")
         .distinct("sender_id", "recipient__type_id")
         .values("sender_id", "recipient__type_id")
     )
@@ -759,11 +803,15 @@ def get_data_for_inaccessible_user(realm: Realm, user_id: int) -> APIUserDict:
     return user_dict
 
 
-def get_accessible_user_ids(realm: Realm, user_profile: UserProfile) -> List[int]:
+def get_accessible_user_ids(
+    realm: Realm, user_profile: UserProfile, include_deactivated_users: bool = False
+) -> List[int]:
     subscribers_dict_of_target_user_subscriptions = get_subscribers_of_target_user_subscriptions(
-        [user_profile]
+        [user_profile], include_deactivated_users_for_huddles=include_deactivated_users
+    )
+    users_involved_in_dms_dict = get_users_involved_in_dms_with_target_users(
+        [user_profile], realm, include_deactivated_users=include_deactivated_users
     )
-    users_involved_in_dms_dict = get_users_involved_in_dms_with_target_users([user_profile], realm)
 
     # This does not include bots, because either the caller
     # wants only human users or it handles bots separately.
@@ -787,7 +835,9 @@ def get_user_dicts_in_realm(
         return (all_user_dicts, [])
 
     assert user_profile is not None
-    accessible_user_ids = get_accessible_user_ids(realm, user_profile)
+    accessible_user_ids = get_accessible_user_ids(
+        realm, user_profile, include_deactivated_users=True
+    )
 
     accessible_user_dicts: List[RawUserDict] = []
     inaccessible_user_dicts: List[APIUserDict] = []
@@ -896,7 +946,20 @@ def is_2fa_verified(user: UserProfile) -> bool:
 
 
 def get_users_with_access_to_real_email(user_profile: UserProfile) -> List[int]:
-    active_users = user_profile.realm.get_active_users()
+    if not user_access_restricted_in_realm(user_profile):
+        active_users = user_profile.realm.get_active_users()
+    else:
+        # The get_user_ids_who_can_access_user returns user IDs and not
+        # user objects and we instead do one more query for UserProfile
+        # objects. We need complete UserProfile objects only for a couple
+        # of cases and it is not worth to query the whole UserProfile
+        # objects in all the cases and it is fine to do the extra query
+        # wherever needed.
+        user_ids_who_can_access_user = get_user_ids_who_can_access_user(user_profile)
+        active_users = UserProfile.objects.filter(
+            id__in=user_ids_who_can_access_user, is_active=True
+        )
+
     return [
         user.id
         for user in active_users

zerver/openapi/zulip.yaml

@@ -379,8 +379,11 @@ paths:
                                 }
                             - type: object
                               description: |
-                                Event sent generally to all users in an organization for changes
+                                Event sent generally to all users who can access the modified
-                                in the set of users or those users metadata.
+                                user for changes in the set of users or those users metadata.
+
+                                **Changes**: Prior to Zulip 8.0 (feature level 228), this event
+                                was sent to all users in the organization.
                               properties:
                                 id:
                                   $ref: "#/components/schemas/EventIdSchema"

zerver/tests/test_events.py

@@ -1356,6 +1356,27 @@ class NormalActionsTest(BaseAction):
         check_realm_user_update("events[0]", events[0], "custom_profile_field")
         self.assertEqual(events[0]["person"]["custom_profile_field"].keys(), {"id", "value"})
 
+        # Test event for updating custom profile data for guests.
+        self.set_up_db_for_testing_user_access()
+        self.user_profile = self.example_user("polonius")
+        field = {
+            "id": field_id,
+            "value": "New value",
+        }
+        cordelia = self.example_user("cordelia")
+        events = self.verify_action(
+            lambda: do_update_user_custom_profile_data_if_changed(cordelia, [field]),
+            num_events=0,
+            state_change_expected=False,
+        )
+
+        hamlet = self.example_user("hamlet")
+        events = self.verify_action(
+            lambda: do_update_user_custom_profile_data_if_changed(hamlet, [field])
+        )
+        check_realm_user_update("events[0]", events[0], "custom_profile_field")
+        self.assertEqual(events[0]["person"]["custom_profile_field"].keys(), {"id", "value"})
+
     def test_presence_events(self) -> None:
         events = self.verify_action(
             lambda: do_update_user_presence(
@@ -1870,12 +1891,32 @@ class NormalActionsTest(BaseAction):
         self.assertEqual(events[0]["person"]["avatar_url"], None)
         self.assertEqual(events[0]["person"]["avatar_url_medium"], None)
 
+        self.set_up_db_for_testing_user_access()
+        self.user_profile = self.example_user("polonius")
+        cordelia = self.example_user("cordelia")
+        events = self.verify_action(
+            lambda: do_change_avatar_fields(
+                cordelia, UserProfile.AVATAR_FROM_GRAVATAR, acting_user=cordelia
+            ),
+            num_events=0,
+            state_change_expected=False,
+        )
+
     def test_change_full_name(self) -> None:
         events = self.verify_action(
             lambda: do_change_full_name(self.user_profile, "Sir Hamlet", self.user_profile)
         )
         check_realm_user_update("events[0]", events[0], "full_name")
 
+        self.set_up_db_for_testing_user_access()
+        cordelia = self.example_user("cordelia")
+        self.user_profile = self.example_user("polonius")
+        self.verify_action(
+            lambda: do_change_full_name(cordelia, "Cordelia", self.user_profile),
+            num_events=0,
+            state_change_expected=False,
+        )
+
     def test_change_user_delivery_email_email_address_visibility_admins(self) -> None:
         do_change_user_setting(
             self.user_profile,
@@ -1915,6 +1956,25 @@ class NormalActionsTest(BaseAction):
         assert isinstance(events[1]["person"]["avatar_url"], str)
         assert isinstance(events[1]["person"]["avatar_url_medium"], str)
 
+        # Reset hamlet's email to original email.
+        do_change_user_delivery_email(self.user_profile, "hamlet@zulip.com")
+
+        self.set_up_db_for_testing_user_access()
+        cordelia = self.example_user("cordelia")
+        do_change_user_setting(
+            cordelia,
+            "email_address_visibility",
+            UserProfile.EMAIL_ADDRESS_VISIBILITY_EVERYONE,
+            acting_user=None,
+        )
+        self.user_profile = self.example_user("polonius")
+        action = lambda: do_change_user_delivery_email(cordelia, "newcordelia@zulip.com")
+        self.verify_action(
+            action,
+            num_events=0,
+            state_change_expected=False,
+        )
+
     def test_change_realm_authentication_methods(self) -> None:
         def fake_backends() -> Any:
             backends = (
@@ -2253,6 +2313,37 @@ class NormalActionsTest(BaseAction):
                 check_subscription_peer_add("events[5]", events[5])
                 check_subscription_peer_add("events[6]", events[6])
 
+    def test_change_user_role_for_restricted_users(self) -> None:
+        self.set_up_db_for_testing_user_access()
+        self.user_profile = self.example_user("polonius")
+
+        for role in [
+            UserProfile.ROLE_REALM_OWNER,
+            UserProfile.ROLE_REALM_ADMINISTRATOR,
+            UserProfile.ROLE_MODERATOR,
+            UserProfile.ROLE_MEMBER,
+            UserProfile.ROLE_GUEST,
+        ]:
+            cordelia = self.example_user("cordelia")
+            old_role = cordelia.role
+
+            num_events = 2
+            if UserProfile.ROLE_MEMBER in [old_role, role]:
+                num_events = 3
+
+            events = self.verify_action(
+                partial(do_change_user_role, cordelia, role, acting_user=None),
+                num_events=num_events,
+            )
+
+            check_user_group_remove_members("events[0]", events[0])
+            check_user_group_add_members("events[1]", events[1])
+
+            if old_role == UserProfile.ROLE_MEMBER:
+                check_user_group_remove_members("events[2]", events[2])
+            elif role == UserProfile.ROLE_MEMBER:
+                check_user_group_add_members("events[2]", events[2])
+
     def test_change_notification_settings(self) -> None:
         for notification_setting in self.user_profile.notification_setting_types:
             if notification_setting in [
@@ -2774,6 +2865,21 @@ class NormalActionsTest(BaseAction):
         events = self.verify_action(action, num_events=1)
         check_realm_user_update("events[0]", events[0], "is_active")
 
+        do_reactivate_user(user_profile, acting_user=None)
+        self.set_up_db_for_testing_user_access()
+
+        # Test that guest users receive event only
+        # if they can access the deactivated user.
+        user_profile = self.example_user("cordelia")
+        self.user_profile = self.example_user("polonius")
+        action = lambda: do_deactivate_user(user_profile, acting_user=None)
+        events = self.verify_action(action, num_events=0, state_change_expected=False)
+
+        user_profile = self.example_user("shiva")
+        action = lambda: do_deactivate_user(user_profile, acting_user=None)
+        events = self.verify_action(action, num_events=1)
+        check_realm_user_update("events[0]", events[0], "is_active")
+
     def test_do_reactivate_user(self) -> None:
         bot = self.create_bot("test")
         self.subscribe(bot, "Denmark")