From dbcc9ea82633bd493a3d02ba0597036db56821bd Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Tue, 17 Oct 2023 16:26:39 +0530 Subject: [PATCH] users: Update presence and user status code to support restricted users. The presence and user status update events are only sent to accessible users, i.e. guests do not receive presence and user status updates for users they cannot access. --- api_docs/changelog.md | 13 +++++++ zerver/actions/presence.py | 7 +++- zerver/actions/user_status.py | 5 +-- zerver/lib/events.py | 8 +++-- zerver/lib/presence.py | 36 ++++++++++++------- zerver/lib/user_status.py | 47 +++++++++++++------------ zerver/lib/users.py | 3 +- zerver/openapi/zulip.yaml | 18 ++++++++-- zerver/tests/test_events.py | 49 ++++++++++++++++++++++++++ zerver/tests/test_presence.py | 60 ++++++++++++++++++++++++++++---- zerver/tests/test_user_status.py | 32 ++++++++++++++--- zerver/views/presence.py | 6 ++++ zproject/default_settings.py | 7 ++++ 13 files changed, 236 insertions(+), 55 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 7ee43c40be..b09e7e9045 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -25,6 +25,19 @@ format used by the Zulip server that they are interacting with. * [`GET /events`](/api/get-events): `realm_user` events with `op: "update"` are now only sent to users who can access the modified user. +* [`GET /events`](/api/get-events): `presence` events are now only sent to + users who can access the user who comes back online if the + `CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE` server setting is set + to `true`. + +* [`GET /events`](/api/get-events): `user_status` events are now only + sent to users who can access the modified user. + +* [`GET /realm/presence`](/api/get-presence): The endpoint now returns + presence information of accessible users only if the + `CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE` server setting is set + to `true`. + **Feature level 227** * [`PATCH /realm/user_settings_defaults`](/api/update-realm-user-settings-defaults), diff --git a/zerver/actions/presence.py b/zerver/actions/presence.py index 006148f11b..a7204d5719 100644 --- a/zerver/actions/presence.py +++ b/zerver/actions/presence.py @@ -11,6 +11,7 @@ from zerver.lib.presence import ( ) from zerver.lib.queue import queue_json_publish from zerver.lib.timestamp import datetime_to_timestamp +from zerver.lib.users import get_user_ids_who_can_access_user from zerver.models import Client, UserPresence, UserProfile, active_user_ids, get_client from zerver.tornado.django_api import send_event @@ -27,7 +28,11 @@ def send_presence_changed( # # See https://zulip.readthedocs.io/en/latest/subsystems/presence.html for # internals documentation on presence. - user_ids = active_user_ids(user_profile.realm_id) + if settings.CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE: + user_ids = get_user_ids_who_can_access_user(user_profile) + else: + user_ids = active_user_ids(user_profile.realm_id) + if ( len(user_ids) > settings.USER_LIMIT_FOR_SENDING_PRESENCE_UPDATE_EVENTS and not force_send_update diff --git a/zerver/actions/user_status.py b/zerver/actions/user_status.py index 0493eb9bd3..dedaf26fd4 100644 --- a/zerver/actions/user_status.py +++ b/zerver/actions/user_status.py @@ -2,7 +2,8 @@ from typing import Optional from zerver.actions.user_settings import do_change_user_setting from zerver.lib.user_status import update_user_status -from zerver.models import UserProfile, active_user_ids +from zerver.lib.users import get_user_ids_who_can_access_user +from zerver.models import UserProfile from zerver.tornado.django_api import send_event @@ -50,4 +51,4 @@ def do_update_user_status( event["emoji_name"] = emoji_name event["emoji_code"] = emoji_code event["reaction_type"] = reaction_type - send_event(realm, event, active_user_ids(realm.id)) + send_event(realm, event, get_user_ids_who_can_access_user(user_profile)) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 8dde483537..f5413cd63c 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -231,7 +231,9 @@ def fetch_initial_state_data( if want("presence"): state["presences"] = ( - {} if user_profile is None else get_presences_for_realm(realm, slim_presence) + {} + if user_profile is None + else get_presences_for_realm(realm, slim_presence, user_profile) ) # Send server_timestamp, to match the format of `GET /presence` requests. state["server_timestamp"] = time.time() @@ -652,7 +654,9 @@ def fetch_initial_state_data( if want("user_status"): # We require creating an account to access statuses. state["user_status"] = ( - {} if user_profile is None else get_user_status_dict(realm_id=realm.id) + {} + if user_profile is None + else get_user_status_dict(realm=realm, user_profile=user_profile) ) if want("user_topic"): diff --git a/zerver/lib/presence.py b/zerver/lib/presence.py index c3f01b884a..1cd8ae74d4 100644 --- a/zerver/lib/presence.py +++ b/zerver/lib/presence.py @@ -7,6 +7,7 @@ from django.conf import settings from django.utils.timezone import now as timezone_now from zerver.lib.timestamp import datetime_to_timestamp +from zerver.lib.users import check_user_can_access_all_users, get_accessible_user_ids from zerver.models import PushDeviceToken, Realm, UserPresence, UserProfile, query_for_ids @@ -151,24 +152,33 @@ def get_presence_for_user( def get_presence_dict_by_realm( - realm_id: int, slim_presence: bool = False + realm: Realm, slim_presence: bool = False, requesting_user_profile: Optional[UserProfile] = None ) -> Dict[str, Dict[str, Any]]: two_weeks_ago = timezone_now() - datetime.timedelta(weeks=2) query = UserPresence.objects.filter( - realm_id=realm_id, + realm_id=realm.id, last_connected_time__gte=two_weeks_ago, user_profile__is_active=True, user_profile__is_bot=False, - ).values( - "last_active_time", - "last_connected_time", - "user_profile__email", - "user_profile_id", - "user_profile__enable_offline_push_notifications", - "user_profile__date_joined", ) - presence_rows = list(query) + if settings.CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE and not check_user_can_access_all_users( + requesting_user_profile + ): + assert requesting_user_profile is not None + accessible_user_ids = get_accessible_user_ids(realm, requesting_user_profile) + query = query.filter(user_profile_id__in=accessible_user_ids) + + presence_rows = list( + query.values( + "last_active_time", + "last_connected_time", + "user_profile__email", + "user_profile_id", + "user_profile__enable_offline_push_notifications", + "user_profile__date_joined", + ) + ) mobile_query = PushDeviceToken.objects.distinct("user_id").values_list( "user_id", @@ -196,13 +206,13 @@ def get_presence_dict_by_realm( def get_presences_for_realm( - realm: Realm, slim_presence: bool + realm: Realm, slim_presence: bool, requesting_user_profile: UserProfile ) -> Dict[str, Dict[str, Dict[str, Any]]]: if realm.presence_disabled: # Return an empty dict if presence is disabled in this realm return defaultdict(dict) - return get_presence_dict_by_realm(realm.id, slim_presence) + return get_presence_dict_by_realm(realm, slim_presence, requesting_user_profile) def get_presence_response( @@ -210,5 +220,5 @@ def get_presence_response( ) -> Dict[str, Any]: realm = requesting_user_profile.realm server_timestamp = time.time() - presences = get_presences_for_realm(realm, slim_presence) + presences = get_presences_for_realm(realm, slim_presence, requesting_user_profile) return dict(presences=presences, server_timestamp=server_timestamp) diff --git a/zerver/lib/user_status.py b/zerver/lib/user_status.py index cd127af157..b6d1f414fc 100644 --- a/zerver/lib/user_status.py +++ b/zerver/lib/user_status.py @@ -3,7 +3,8 @@ from typing import Dict, Optional, TypedDict from django.db.models import Q from django.utils.timezone import now as timezone_now -from zerver.models import UserStatus +from zerver.lib.users import check_user_can_access_all_users, get_accessible_user_ids +from zerver.models import Realm, UserProfile, UserStatus class UserInfoDict(TypedDict, total=False): @@ -48,27 +49,29 @@ def format_user_status(row: RawUserInfoDict) -> UserInfoDict: return dct -def get_user_status_dict(realm_id: int) -> Dict[str, UserInfoDict]: - rows = ( - UserStatus.objects.filter( - user_profile__realm_id=realm_id, - user_profile__is_active=True, - ) - .exclude( - Q(user_profile__presence_enabled=True) - & Q(status_text="") - & Q(emoji_name="") - & Q(emoji_code="") - & Q(reaction_type=UserStatus.UNICODE_EMOJI), - ) - .values( - "user_profile_id", - "user_profile__presence_enabled", - "status_text", - "emoji_name", - "emoji_code", - "reaction_type", - ) +def get_user_status_dict(realm: Realm, user_profile: UserProfile) -> Dict[str, UserInfoDict]: + query = UserStatus.objects.filter( + user_profile__realm_id=realm.id, + user_profile__is_active=True, + ).exclude( + Q(user_profile__presence_enabled=True) + & Q(status_text="") + & Q(emoji_name="") + & Q(emoji_code="") + & Q(reaction_type=UserStatus.UNICODE_EMOJI), + ) + + if not check_user_can_access_all_users(user_profile): + accessible_user_ids = get_accessible_user_ids(realm, user_profile) + query = query.filter(user_profile_id__in=accessible_user_ids) + + rows = query.values( + "user_profile_id", + "user_profile__presence_enabled", + "status_text", + "emoji_name", + "emoji_code", + "reaction_type", ) user_dict: Dict[str, UserInfoDict] = {} diff --git a/zerver/lib/users.py b/zerver/lib/users.py index f7d44c3dea..b2c9fc5e57 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -591,7 +591,8 @@ def check_can_access_user( 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. + # this function is used to get users to send events and to + # send presence update. realm = target_user.realm if not user_access_restricted_in_realm(target_user): return active_user_ids(realm.id) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 165d83df11..fb91d4010e 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1133,6 +1133,13 @@ paths: confusing users when someone comes online and immediately sends a message (one wouldn't want them to still appear offline at that point!). + + If the `CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE` server-level + setting is set to `true`, then the event is only sent to users + who can access the user who came back online. + + **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" @@ -1702,8 +1709,11 @@ paths: - type: object additionalProperties: false description: | - Event sent to all users in a Zulip organization when the - status of a user changes. + Event sent to all users who can access the modified + user when the status of a user changes. + + **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" @@ -9761,6 +9771,10 @@ paths: description: | Get the presence information of all the users in an organization. + If the `CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE` server-level + setting is set to `true`, presence information of only accessible + users are returned. + See [Zulip's developer documentation][subsystems-presence] for details on the data model for presence in Zulip. diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index e6328d0a2f..e8fe48b3b0 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1645,6 +1645,55 @@ class NormalActionsTest(BaseAction): check_user_status("events[0]", events[0], {"status_text"}) + self.set_up_db_for_testing_user_access() + cordelia = self.example_user("cordelia") + self.user_profile = self.example_user("polonius") + + # Set the date_joined for cordelia here like we did at + # the start of this test. + cordelia.date_joined = timezone_now() - datetime.timedelta(days=15) + cordelia.save() + + away_val = False + with self.settings(CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE=True): + events = self.verify_action( + lambda: do_update_user_status( + user_profile=cordelia, + away=away_val, + status_text="out to lunch", + emoji_name="car", + emoji_code="1f697", + reaction_type=UserStatus.UNICODE_EMOJI, + client_id=client.id, + ), + num_events=0, + state_change_expected=False, + ) + + away_val = True + events = self.verify_action( + lambda: do_update_user_status( + user_profile=cordelia, + away=away_val, + status_text="at the beach", + emoji_name=None, + emoji_code=None, + reaction_type=None, + client_id=client.id, + ), + num_events=1, + state_change_expected=True, + ) + check_presence( + "events[0]", + events[0], + has_email=True, + # We no longer store information about the client and we simply + # set the field to 'website' for backwards compatibility. + presence_key="website", + status="idle", + ) + def test_user_group_events(self) -> None: othello = self.example_user("othello") events = self.verify_action( diff --git a/zerver/tests/test_presence.py b/zerver/tests/test_presence.py index ff838cbbf8..1c24c7d40b 100644 --- a/zerver/tests/test_presence.py +++ b/zerver/tests/test_presence.py @@ -37,7 +37,7 @@ class UserPresenceModelTests(ZulipTestCase): user_profile = self.example_user("hamlet") email = user_profile.email - presence_dct = get_presence_dict_by_realm(user_profile.realm_id) + presence_dct = get_presence_dict_by_realm(user_profile.realm) self.assert_length(presence_dct, 0) self.login_user(user_profile) @@ -45,12 +45,12 @@ class UserPresenceModelTests(ZulipTestCase): self.assert_json_success(result) slim_presence = False - presence_dct = get_presence_dict_by_realm(user_profile.realm_id, slim_presence) + presence_dct = get_presence_dict_by_realm(user_profile.realm, slim_presence) self.assert_length(presence_dct, 1) self.assertEqual(presence_dct[email]["website"]["status"], "active") slim_presence = True - presence_dct = get_presence_dict_by_realm(user_profile.realm_id, slim_presence) + presence_dct = get_presence_dict_by_realm(user_profile.realm, slim_presence) self.assert_length(presence_dct, 1) info = presence_dct[str(user_profile.id)] self.assertEqual(set(info.keys()), {"active_timestamp", "idle_timestamp"}) @@ -64,19 +64,19 @@ class UserPresenceModelTests(ZulipTestCase): # Simulate the presence being a week old first. Nothing should change. back_date(num_weeks=1) - presence_dct = get_presence_dict_by_realm(user_profile.realm_id) + presence_dct = get_presence_dict_by_realm(user_profile.realm) self.assert_length(presence_dct, 1) # If the UserPresence row is three weeks old, we ignore it. back_date(num_weeks=3) - presence_dct = get_presence_dict_by_realm(user_profile.realm_id) + presence_dct = get_presence_dict_by_realm(user_profile.realm) self.assert_length(presence_dct, 0) # If the values are set to "never", ignore it just like for sufficiently old presence rows. UserPresence.objects.filter(id=user_profile.id).update( last_active_time=None, last_connected_time=None ) - presence_dct = get_presence_dict_by_realm(user_profile.realm_id) + presence_dct = get_presence_dict_by_realm(user_profile.realm) self.assert_length(presence_dct, 0) def test_pushable_always_false(self) -> None: @@ -92,7 +92,7 @@ class UserPresenceModelTests(ZulipTestCase): self.assert_json_success(result) def pushable() -> bool: - presence_dct = get_presence_dict_by_realm(user_profile.realm_id) + presence_dct = get_presence_dict_by_realm(user_profile.realm) self.assert_length(presence_dct, 1) return presence_dct[email]["website"]["pushable"] @@ -491,6 +491,17 @@ class SingleUserPresenceTests(ZulipTestCase): result = self.client_get(f"/json/users/{othello.id}/presence", subdomain="zephyr") self.assert_json_error(result, "No such user") + self.set_up_db_for_testing_user_access() + self.login("polonius") + with self.settings(CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE=True): + result = self.client_get(f"/json/users/{othello.id}/presence") + self.assert_json_error(result, "Insufficient permission") + + result = self.client_get(f"/json/users/{othello.id}/presence") + result_dict = self.assert_json_success(result) + self.assertEqual(set(result_dict["presence"].keys()), {"website", "aggregated"}) + self.assertEqual(set(result_dict["presence"]["website"].keys()), {"status", "timestamp"}) + # Then, we check everything works self.login("hamlet") result = self.client_get("/json/users/othello@zulip.com/presence") @@ -658,6 +669,7 @@ class GetRealmStatusesTest(ZulipTestCase): # Set up the test by simulating users reporting their presence data. othello = self.example_user("othello") hamlet = self.example_user("hamlet") + self.example_user("cordelia") result = self.api_post( othello, @@ -689,6 +701,40 @@ class GetRealmStatusesTest(ZulipTestCase): json = self.assert_json_success(result) self.assertEqual(set(json["presences"].keys()), {hamlet.email, othello.email}) + # Check that polonius cannot fetch presence data for inaccessible user Othello + # if CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE is set to True. + self.set_up_db_for_testing_user_access() + polonius = self.example_user("polonius") + with self.settings(CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE=True): + result = self.api_get(polonius, "/api/v1/realm/presence") + json = self.assert_json_success(result) + self.assertEqual(set(json["presences"].keys()), {hamlet.email}) + + result = self.api_get(polonius, "/api/v1/realm/presence") + json = self.assert_json_success(result) + self.assertEqual(set(json["presences"].keys()), {hamlet.email, othello.email}) + + with self.settings(CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE=True): + result = self.api_post( + polonius, + "/api/v1/users/me/presence", + dict(status="idle"), + HTTP_USER_AGENT="ZulipDesktop/1.0", + ) + json = self.assert_json_success(result) + self.assertEqual(set(json["presences"].keys()), {hamlet.email, polonius.email}) + + result = self.api_post( + polonius, + "/api/v1/users/me/presence", + dict(status="idle"), + HTTP_USER_AGENT="ZulipDesktop/1.0", + ) + json = self.assert_json_success(result) + self.assertEqual( + set(json["presences"].keys()), {hamlet.email, polonius.email, othello.email} + ) + def test_presence_disabled(self) -> None: # Disable presence status and test whether the presence # is reported or not. diff --git a/zerver/tests/test_user_status.py b/zerver/tests/test_user_status.py index ef1a729768..90658c7b35 100644 --- a/zerver/tests/test_user_status.py +++ b/zerver/tests/test_user_status.py @@ -1,4 +1,4 @@ -from typing import Any, Dict +from typing import Any, Dict, Optional import orjson @@ -7,8 +7,10 @@ from zerver.lib.user_status import UserInfoDict, get_user_status_dict, update_us from zerver.models import UserProfile, UserStatus, get_client -def user_status_info(user: UserProfile) -> UserInfoDict: - user_dict = get_user_status_dict(user.realm_id) +def user_status_info(user: UserProfile, acting_user: Optional[UserProfile] = None) -> UserInfoDict: + if acting_user is None: + acting_user = user + user_dict = get_user_status_dict(user.realm, acting_user) return user_dict.get(str(user.id), {}) @@ -115,6 +117,26 @@ class UserStatusTest(ZulipTestCase): dict(status_text="in a meeting"), ) + # Test user status for inaccessible users. + self.set_up_db_for_testing_user_access() + cordelia = self.example_user("cordelia") + update_user_status( + user_profile_id=cordelia.id, + status_text="on vacation", + emoji_name=None, + emoji_code=None, + reaction_type=None, + client_id=client2.id, + ) + self.assertEqual( + user_status_info(hamlet, self.example_user("polonius")), + dict(status_text="in a meeting"), + ) + self.assertEqual( + user_status_info(cordelia, self.example_user("polonius")), + {}, + ) + def update_status_and_assert_event( self, payload: Dict[str, Any], expected_event: Dict[str, Any], num_events: int = 1 ) -> None: @@ -125,7 +147,7 @@ class UserStatusTest(ZulipTestCase): def test_endpoints(self) -> None: hamlet = self.example_user("hamlet") - realm_id = hamlet.realm_id + realm = hamlet.realm self.login_user(hamlet) @@ -263,7 +285,7 @@ class UserStatusTest(ZulipTestCase): expected_event=dict(type="user_status", user_id=hamlet.id, status_text=""), ) self.assertEqual( - get_user_status_dict(realm_id=realm_id), + get_user_status_dict(realm=realm, user_profile=hamlet), {}, ) diff --git a/zerver/views/presence.py b/zerver/views/presence.py index 34bd8f29ef..2cfc5d7a32 100644 --- a/zerver/views/presence.py +++ b/zerver/views/presence.py @@ -18,6 +18,7 @@ from zerver.lib.request import RequestNotes from zerver.lib.response import json_success from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.typed_endpoint import ApiParamConfig, typed_endpoint +from zerver.lib.users import check_can_access_user from zerver.models import ( UserActivity, UserPresence, @@ -48,6 +49,11 @@ def get_presence_backend( if target.is_bot: raise JsonableError(_("Presence is not supported for bot users.")) + if settings.CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE and not check_can_access_user( + target, user_profile + ): + raise JsonableError(_("Insufficient permission")) + presence_dict = get_presence_for_user(target.id) if len(presence_dict) == 0: raise JsonableError( diff --git a/zproject/default_settings.py b/zproject/default_settings.py index 69d01f23f6..d54a841eed 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -601,3 +601,10 @@ TYPING_STARTED_WAIT_PERIOD_MILLISECONDS = 10000 # notifications enabled. Default is set to avoid excessive Tornado # load in large organizations. MAX_STREAM_SIZE_FOR_TYPING_NOTIFICATIONS = 100 + +# Limiting guest access to other users via the +# can_access_all_users_group setting makes presence queries much more +# expensive. This can be a significant performance problem for +# installations with thousands of users with many guests limited in +# this way, pending further optimization of the relevant code paths. +CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE = False