From 650e55fef89278b0d55e614eeb10979fa7b05d24 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Wed, 11 Oct 2023 13:04:26 +0530 Subject: [PATCH] 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 | 5 ++ zerver/actions/custom_profile_fields.py | 3 +- zerver/actions/user_settings.py | 26 ++++-- zerver/actions/users.py | 11 +-- zerver/lib/users.py | 97 ++++++++++++++++++---- zerver/openapi/zulip.yaml | 7 +- zerver/tests/test_events.py | 106 ++++++++++++++++++++++++ 7 files changed, 224 insertions(+), 31 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 520090c494..7ee43c40be 100644 --- a/api_docs/changelog.md +++ b/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), diff --git a/zerver/actions/custom_profile_fields.py b/zerver/actions/custom_profile_fields.py index 5d83892c4c..8f2c60fdf3 100644 --- a/zerver/actions/custom_profile_fields.py +++ b/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( diff --git a/zerver/actions/user_settings.py b/zerver/actions/user_settings.py index 0125ef07d9..57ebbd37b8 100644 --- a/zerver/actions/user_settings.py +++ b/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": diff --git a/zerver/actions/users.py b/zerver/actions/users.py index 86bdcdd989..8efd52aae4 100644 --- a/zerver/actions/users.py +++ b/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: diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 8fc97eb672..f7d44c3dea 100644 --- a/zerver/lib/users.py +++ b/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( - Q(recipient__type=Recipient.STREAM, is_user_active=True) - | Q(recipient__type=Recipient.HUDDLE) - ) + if include_deactivated_users_for_huddles: + subs_in_target_user_subscriptions_query = subs_in_target_user_subscriptions_query.filter( + 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( - realm=realm, - recipient_id__in=personal_recipient_ids_for_target_users, - recipient__type=Recipient.PERSONAL, - ) - .order_by("recipient__type_id") + direct_message_senders_query.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 diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index f5e17477cd..165d83df11 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -379,8 +379,11 @@ paths: } - type: object description: | - Event sent generally to all users in an organization for changes - in the set of users or those users metadata. + Event sent generally to all users who can access the modified + 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" diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index b1ce19f427..e6328d0a2f 100644 --- a/zerver/tests/test_events.py +++ b/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")