From d394cfc4dbd7ca0f25811eceb2f0a4b847e89e9b Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Wed, 18 Oct 2023 08:54:50 +0530 Subject: [PATCH] streams: Send user creation events on adding subscribers. This commit adds code to send user creation events to guests who gain access to new subscribers and to the new guest subscribers who gain access to existing stream subscribers. --- api_docs/changelog.md | 3 + zerver/actions/create_user.py | 45 +++++++++------ zerver/actions/streams.py | 95 ++++++++++++++++++++++++++++--- zerver/lib/stream_subscription.py | 15 +++++ zerver/openapi/zulip.yaml | 9 ++- zerver/tests/test_events.py | 33 +++++++++++ zerver/tests/test_invite.py | 2 +- 7 files changed, 174 insertions(+), 28 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index b09e7e9045..836cff7a47 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -38,6 +38,9 @@ format used by the Zulip server that they are interacting with. `CAN_ACCESS_ALL_USERS_GROUP_LIMITS_PRESENCE` server setting is set to `true`. +* [`GET /events`](/api/get-events): `realm_user` events with `op: "add"` + are now also sent when a guest user gains access to a user. + **Feature level 227** * [`PATCH /realm/user_settings_defaults`](/api/update-realm-user-settings-defaults), diff --git a/zerver/actions/create_user.py b/zerver/actions/create_user.py index 5f8b4bf4f5..51c4f54898 100644 --- a/zerver/actions/create_user.py +++ b/zerver/actions/create_user.py @@ -303,7 +303,7 @@ def process_new_human_user( send_initial_direct_message(user_profile) -def notify_created_user(user_profile: UserProfile) -> None: +def notify_created_user(user_profile: UserProfile, notify_user_ids: List[int]) -> None: user_row = user_profile_to_user_row(user_profile) format_user_row_kwargs: Dict[str, Any] = { @@ -324,23 +324,32 @@ def notify_created_user(user_profile: UserProfile) -> None: user_ids_without_access_to_created_user: List[int] = [] users_with_access_to_created_users: List[UserProfile] = [] - active_realm_users = list(user_profile.realm.get_active_users()) - # This call to user_access_restricted_in_realm results in - # one extra query in the user creation codepath to check - # "realm.can_access_all_users_group.name" because we do - # not prefetch realm and its related fields when fetching - # PreregistrationUser object. - if user_access_restricted_in_realm(user_profile): - for user in active_realm_users: - if user.is_guest: - # This logic assumes that can_access_all_users_group - # setting can only be set to EVERYONE or MEMBERS. - user_ids_without_access_to_created_user.append(user.id) - else: - users_with_access_to_created_users.append(user) + if notify_user_ids: + # This is currently used to send creation event when a guest + # gains access to a user, so we depend on the caller to make + # sure that only accessible users receive the user data. + users_with_access_to_created_users = list( + user_profile.realm.get_active_users().filter(id__in=notify_user_ids) + ) else: - users_with_access_to_created_users = active_realm_users + active_realm_users = list(user_profile.realm.get_active_users()) + + # This call to user_access_restricted_in_realm results in + # one extra query in the user creation codepath to check + # "realm.can_access_all_users_group.name" because we do + # not prefetch realm and its related fields when fetching + # PreregistrationUser object. + if user_access_restricted_in_realm(user_profile): + for user in active_realm_users: + if user.is_guest: + # This logic assumes that can_access_all_users_group + # setting can only be set to EVERYONE or MEMBERS. + user_ids_without_access_to_created_user.append(user.id) + else: + users_with_access_to_created_users.append(user) + else: + users_with_access_to_created_users = active_realm_users user_ids_with_real_email_access = [] user_ids_without_real_email_access = [] @@ -538,7 +547,7 @@ def do_create_user( # Note that for bots, the caller will send an additional event # with bot-specific info like services. - notify_created_user(user_profile) + notify_created_user(user_profile, []) do_send_user_group_members_update_event("add_members", system_user_group, [user_profile.id]) if user_profile.role == UserProfile.ROLE_MEMBER and not user_profile.is_provisional_member: @@ -617,7 +626,7 @@ def do_activate_mirror_dummy_user( if settings.BILLING_ENABLED: update_license_ledger_if_needed(user_profile.realm, event_time) - notify_created_user(user_profile) + notify_created_user(user_profile, []) @transaction.atomic(savepoint=False) diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index 869e593451..673405fcc0 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -34,6 +34,7 @@ from zerver.lib.stream_subscription import ( get_active_subscriptions_for_stream_id, get_bulk_stream_subscriber_info, get_used_colors_for_user_ids, + get_users_for_streams, ) from zerver.lib.stream_traffic import get_streams_traffic from zerver.lib.streams import ( @@ -47,6 +48,10 @@ from zerver.lib.streams import ( ) from zerver.lib.subscription_info import get_subscribers_query from zerver.lib.types import APISubscriptionDict +from zerver.lib.users import ( + get_subscribers_of_target_user_subscriptions, + get_users_involved_in_dms_with_target_users, +) from zerver.models import ( ArchivedAttachment, Attachment, @@ -58,6 +63,7 @@ from zerver.models import ( Recipient, Stream, Subscription, + SystemGroups, UserGroup, UserProfile, active_non_guest_user_ids, @@ -592,6 +598,65 @@ def send_peer_subscriber_events( send_event_on_commit(realm, event, peer_user_ids) +def send_user_creation_events_on_adding_subscriptions( + realm: Realm, + altered_user_dict: Dict[int, Set[int]], + altered_streams_dict: Dict[UserProfile, Set[int]], + subscribers_of_altered_user_subscriptions: Dict[int, Set[int]], +) -> None: + altered_users = list(altered_streams_dict.keys()) + non_guest_user_ids = active_non_guest_user_ids(realm.id) + + users_involved_in_dms = get_users_involved_in_dms_with_target_users(altered_users, realm) + + altered_stream_ids = altered_user_dict.keys() + subscribers_dict = get_users_for_streams(set(altered_stream_ids)) + + subscribers_user_id_map: Dict[int, UserProfile] = {} + subscriber_ids_dict: Dict[int, Set[int]] = defaultdict(set) + for stream_id, subscribers in subscribers_dict.items(): + for user in subscribers: + subscriber_ids_dict[stream_id].add(user.id) + subscribers_user_id_map[user.id] = user + + from zerver.actions.create_user import notify_created_user + + for user in altered_users: + streams_for_user = altered_streams_dict[user] + subscribers_in_altered_streams: Set[int] = set() + for stream_id in streams_for_user: + subscribers_in_altered_streams |= subscriber_ids_dict[stream_id] + + users_already_with_access_to_altered_user = ( + set(non_guest_user_ids) + | subscribers_of_altered_user_subscriptions[user.id] + | users_involved_in_dms[user.id] + | {user.id} + ) + + users_to_receive_creation_event = ( + subscribers_in_altered_streams - users_already_with_access_to_altered_user + ) + if users_to_receive_creation_event: + notify_created_user(user, list(users_to_receive_creation_event)) + + if user.is_guest: + # If the altered user is a guest, then the user may receive + # user creation events for subscribers of the new stream. + users_already_accessible_to_altered_user = ( + subscribers_of_altered_user_subscriptions[user.id] + | users_involved_in_dms[user.id] + | {user.id} + ) + + new_accessible_user_ids = ( + subscribers_in_altered_streams - users_already_accessible_to_altered_user + ) + for accessible_user_id in new_accessible_user_ids: + accessible_user = subscribers_user_id_map[accessible_user_id] + notify_created_user(accessible_user, [user.id]) + + SubT: TypeAlias = Tuple[List[SubInfo], List[SubInfo]] @@ -680,6 +745,21 @@ def bulk_add_subscriptions( # We can return early if users are already subscribed to all the streams. return ([], already_subscribed) + altered_user_dict: Dict[int, Set[int]] = defaultdict(set) + altered_guests: Set[int] = set() + altered_streams_dict: Dict[UserProfile, Set[int]] = defaultdict(set) + for sub_info in subs_to_add + subs_to_activate: + altered_user_dict[sub_info.stream.id].add(sub_info.user.id) + altered_streams_dict[sub_info.user].add(sub_info.stream.id) + if sub_info.user.is_guest: + altered_guests.add(sub_info.user.id) + + if realm.can_access_all_users_group.name != SystemGroups.EVERYONE: + altered_users = list(altered_streams_dict.keys()) + subscribers_of_altered_user_subscriptions = get_subscribers_of_target_user_subscriptions( + altered_users + ) + bulk_add_subs_to_db_with_logging( realm=realm, acting_user=acting_user, @@ -687,13 +767,6 @@ def bulk_add_subscriptions( subs_to_activate=subs_to_activate, ) - altered_user_dict: Dict[int, Set[int]] = defaultdict(set) - altered_guests: Set[int] = set() - for sub_info in subs_to_add + subs_to_activate: - altered_user_dict[sub_info.stream.id].add(sub_info.user.id) - if sub_info.user.is_guest: - altered_guests.add(sub_info.user.id) - stream_dict = {stream.id: stream for stream in streams} new_streams = [stream_dict[stream_id] for stream_id in altered_user_dict] @@ -721,6 +794,14 @@ def bulk_add_subscriptions( subscriber_dict=subscriber_peer_info.subscribed_ids, ) + if realm.can_access_all_users_group.name != SystemGroups.EVERYONE: + send_user_creation_events_on_adding_subscriptions( + realm, + altered_user_dict, + altered_streams_dict, + subscribers_of_altered_user_subscriptions, + ) + send_peer_subscriber_events( op="peer_add", realm=realm, diff --git a/zerver/lib/stream_subscription.py b/zerver/lib/stream_subscription.py index 1abba00036..da63a52117 100644 --- a/zerver/lib/stream_subscription.py +++ b/zerver/lib/stream_subscription.py @@ -163,6 +163,21 @@ def get_user_ids_for_streams(stream_ids: Set[int]) -> Dict[int, Set[int]]: return result +def get_users_for_streams(stream_ids: Set[int]) -> Dict[int, Set[UserProfile]]: + all_subs = ( + get_active_subscriptions_for_stream_ids(stream_ids) + .select_related("user_profile", "recipient") + .order_by("recipient__type_id") + ) + + result: Dict[int, Set[UserProfile]] = defaultdict(set) + for stream_id, rows in itertools.groupby(all_subs, key=lambda obj: obj.recipient.type_id): + users = {row.user_profile for row in rows} + result[stream_id] = users + + return result + + def bulk_get_subscriber_peer_info( realm: Realm, streams: Collection[Stream], diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index fb91d4010e..875bad29ff 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1036,8 +1036,13 @@ paths: - type: object description: | Event sent to all users in a Zulip organization when a new - user joins. Processing this event is important to being able - to display basic details on other users given only their ID. + user joins or when a guest user gains access to a user. + Processing this event is important to being able to display + basic details on other users given only their ID. + + **Changes**: Starting with Zulip 8.0 (feature level 228), + this event is also sent when a guest user gains access to + a user. properties: id: $ref: "#/components/schemas/EventIdSchema" diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index e8fe48b3b0..b758857eb3 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -4061,6 +4061,39 @@ class SubscribeActionTest(BaseAction): events = self.verify_action(action, include_subscribers=include_subscribers, num_events=1) check_subscription_remove("events[0]", events[0]) + def test_user_access_events_on_changing_subscriptions(self) -> None: + self.set_up_db_for_testing_user_access() + self.user_profile = self.example_user("polonius") + realm = self.user_profile.realm + stream = get_stream("test_stream1", realm) + othello = self.example_user("othello") + iago = self.example_user("iago") + + subscribe_action = lambda: bulk_add_subscriptions( + realm, [stream], [othello, iago], acting_user=None + ) + events = self.verify_action(subscribe_action, num_events=2) + check_realm_user_add("events[0]", events[0]) + self.assertEqual(events[0]["person"]["user_id"], othello.id) + check_subscription_peer_add("events[1]", events[1]) + self.assertEqual(set(events[1]["user_ids"]), {iago.id, othello.id}) + + def test_user_access_events_on_changing_subscriptions_for_guests(self) -> None: + self.set_up_db_for_testing_user_access() + polonius = self.example_user("polonius") + othello = self.example_user("othello") + self.user_profile = polonius + realm = self.user_profile.realm + stream = self.subscribe(self.example_user("othello"), "new_stream") + subscribe_action = lambda: bulk_add_subscriptions( + realm, [stream], [polonius, self.example_user("iago")], acting_user=None + ) + events = self.verify_action(subscribe_action, num_events=3) + check_stream_create("events[0]", events[0]) + check_subscription_add("events[1]", events[1]) + check_realm_user_add("events[2]", events[2]) + self.assertEqual(events[2]["person"]["user_id"], othello.id) + class DraftActionTest(BaseAction): def do_enable_drafts_synchronization(self, user_profile: UserProfile) -> None: diff --git a/zerver/tests/test_invite.py b/zerver/tests/test_invite.py index 22c328d1aa..26db37b1ef 100644 --- a/zerver/tests/test_invite.py +++ b/zerver/tests/test_invite.py @@ -108,7 +108,7 @@ class StreamSetupTest(ZulipTestCase): new_user = self.create_simple_new_user(realm, "alice@zulip.com") - with self.assert_database_query_count(11): + with self.assert_database_query_count(12): set_up_streams_for_new_human_user( user_profile=new_user, prereg_user=None,