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,