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