diff --git a/zerver/actions/users.py b/zerver/actions/users.py index 7b5abc3f05..b85971a694 100644 --- a/zerver/actions/users.py +++ b/zerver/actions/users.py @@ -99,8 +99,8 @@ def do_delete_user(user_profile: UserProfile, *, acting_user: UserProfile | None # Recipient objects don't get deleted through CASCADE, so we need to handle # the user's personal recipient manually. This will also delete all Messages pointing # to this recipient (all direct messages sent to the user). - assert personal_recipient is not None - personal_recipient.delete() + if personal_recipient is not None: + personal_recipient.delete() replacement_user = create_user( force_id=user_id, email=Address( diff --git a/zerver/lib/export.py b/zerver/lib/export.py index cee1380607..ebddd2afab 100644 --- a/zerver/lib/export.py +++ b/zerver/lib/export.py @@ -20,7 +20,6 @@ from contextvars import ContextVar from dataclasses import dataclass from datetime import datetime from email.headerregistry import Address -from functools import cache from itertools import chain, islice from typing import TYPE_CHECKING, Any, Optional, TypeAlias, TypedDict, TypeVar, cast from urllib.parse import urlsplit @@ -39,6 +38,7 @@ import zerver.lib.upload from analytics.models import RealmCount, StreamCount, UserCount from version import ZULIP_VERSION from zerver.lib.avatar_hash import user_avatar_base_path_from_ids +from zerver.lib.display_recipient import get_display_recipient from zerver.lib.migration_status import MigrationStatusJson, parse_migration_status from zerver.lib.parallel import run_parallel, run_parallel_queue from zerver.lib.pysa import mark_sanitized @@ -93,7 +93,7 @@ from zerver.models.presence import PresenceSequence from zerver.models.realm_audit_logs import AuditLogEventType from zerver.models.realms import get_fake_email_domain, get_realm from zerver.models.saved_snippets import SavedSnippet -from zerver.models.users import ExternalAuthID, get_system_bot +from zerver.models.users import ExternalAuthID, get_system_bot, is_cross_realm_bot_email if TYPE_CHECKING: from mypy_boto3_s3.service_resource import Bucket, Object @@ -1452,7 +1452,11 @@ def custom_fetch_user_profile_cross_realm(response: TableData, context: Context) bot_default_email = bot_name_to_default_email[bot_name] bot_user_id = get_system_bot(bot_email, internal_realm.id).id - recipient_id = Recipient.objects.get(type_id=bot_user_id, type=Recipient.PERSONAL).id + try: + recipient_id = Recipient.objects.get(type_id=bot_user_id, type=Recipient.PERSONAL).id + except Recipient.DoesNotExist: + recipient_id = None + crossrealm_bots.append( dict( email=bot_default_email, @@ -1537,6 +1541,7 @@ def custom_fetch_direct_message_groups(response: TableData, context: Context) -> r["id"] for r in list(response["zerver_userprofile"]) + list(response["zerver_userprofile_mirrordummy"]) + + list(response["zerver_userprofile_crossrealm"]) } recipient_filter = Q() @@ -1573,17 +1578,21 @@ def custom_fetch_direct_message_groups(response: TableData, context: Context) -> for sub in Subscription.objects.select_related("user_profile").filter( recipient__in=realm_direct_message_group_recipient_ids ): - if sub.user_profile.realm_id != realm.id: + if sub.user_profile.realm_id != realm.id and not is_cross_realm_bot_email( + sub.user_profile.delivery_email + ): # In almost every case the other realm will be zulip.com unsafe_direct_message_group_recipient_ids.add(sub.recipient_id) # Now filter down to just those direct message groups that are # entirely within the realm. # - # This is important for ensuring that the User objects needed - # to import it on the other end exist (since we're only - # exporting the users from this realm), at the cost of losing - # some of these cross-realm messages. + # This is important for ensuring that the User objects needed to + # import it on the other end exist (since we're only exporting the + # users from this realm), at the cost of losing any true + # cross-realm messages. (As of 2025, true cross-realm messages, + # not involving system bots cannot exist without a bug or fork of + # Zulip). direct_message_group_subs = [ sub for sub in realm_direct_message_group_subs @@ -2821,22 +2830,18 @@ def batched(iterable: Iterable[T], n: int) -> Iterable[tuple[T, ...]]: def export_messages_single_user( user_profile: UserProfile, *, output_dir: Path, reaction_message_ids: set[int] ) -> None: - @cache - def get_recipient(recipient_id: int) -> str: - recipient = Recipient.objects.get(id=recipient_id) - + def get_recipient(recipient: Recipient, sender: UserProfile) -> str: if recipient.type == Recipient.STREAM: stream = Stream.objects.values("name").get(id=recipient.type_id) return stream["name"] - user_names = ( - UserProfile.objects.filter( - subscription__recipient_id=recipient.id, - ) - .order_by("full_name") - .values_list("full_name", flat=True) - ) + display_recipients = get_display_recipient(recipient) + if len(display_recipients) == 2: + other_user = next(user for user in display_recipients if user["id"] != sender.id) + return other_user["full_name"] + + user_names = [user["full_name"] for user in display_recipients] return ", ".join(user_names) messages_from_me = Message.objects.filter( @@ -2878,7 +2883,9 @@ def export_messages_single_user( item["flags_mask"] = user_message.flags.mask # Add a few nice, human-readable details item["sending_client_name"] = user_message.message.sending_client.name - item["recipient_name"] = get_recipient(user_message.message.recipient_id) + item["recipient_name"] = get_recipient( + user_message.message.recipient, user_message.message.sender + ) return floatify_datetime_fields(item, "zerver_message") message_filename = os.path.join(output_dir, f"messages-{dump_file_id:06}.json") diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 2eb5652577..033742ee46 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -1244,8 +1244,16 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int = 1) -> Rea new_user_id = get_system_bot(item["email"], internal_realm.id).id update_id_map(table="user_profile", old_id=item["id"], new_id=new_user_id) crossrealm_user_ids.add(new_user_id) - new_recipient_id = Recipient.objects.get(type=Recipient.PERSONAL, type_id=new_user_id).id - update_id_map(table="recipient", old_id=item["recipient_id"], new_id=new_recipient_id) + try: + new_recipient_id = Recipient.objects.get( + type=Recipient.PERSONAL, type_id=new_user_id + ).id + update_id_map(table="recipient", old_id=item["recipient_id"], new_id=new_recipient_id) + except Recipient.DoesNotExist: + # This can happen if the pre-import server used DirectMessageGroup + # for cross-realm bots exactly when the post-import server does. + # The personal recipients shouldn't exist in both cases. + assert item["recipient_id"] is None # We first do a pass of updating model IDs for the cluster of # major models that have foreign keys into each other. diff --git a/zerver/lib/message.py b/zerver/lib/message.py index f865d3ca92..d08cba1ed5 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -1399,7 +1399,9 @@ def get_recent_conversations_recipient_id( return recipient_id -def get_recent_private_conversations(user_profile: UserProfile) -> dict[int, dict[str, Any]]: +def _get_recent_conversations_via_legacy_personal_recipient( + user_profile_id: int, recipient_id: int +) -> list[tuple[int, int]]: """This function uses some carefully optimized SQL queries, designed to use the UserMessage index on private_messages. It is somewhat complicated by the fact that for 1:1 direct @@ -1412,17 +1414,9 @@ def get_recent_private_conversations(user_profile: UserProfile) -> dict[int, dic It may be possible to write this query directly in Django, however it is made much easier by using CTEs, which Django does not natively support. - - We return a dictionary structure for convenient modification - below; this structure is converted into its final form by - post_process. - """ RECENT_CONVERSATIONS_LIMIT = 1000 - recipient_map = {} - my_recipient_id = user_profile.recipient_id - query = SQL( """ WITH personals AS ( @@ -1435,12 +1429,12 @@ def get_recent_private_conversations(user_profile: UserProfile) -> dict[int, dic message AS ( SELECT message_id, CASE - WHEN m.recipient_id = %(my_recipient_id)s + WHEN m.recipient_id = %(recipient_id)s THEN m.sender_id ELSE NULL END AS sender_id, CASE - WHEN m.recipient_id <> %(my_recipient_id)s + WHEN m.recipient_id <> %(recipient_id)s THEN m.recipient_id ELSE NULL END AS outgoing_recipient_id @@ -1466,29 +1460,64 @@ def get_recent_private_conversations(user_profile: UserProfile) -> dict[int, dic cursor.execute( query, { - "user_profile_id": user_profile.id, + "user_profile_id": user_profile_id, "conversation_limit": RECENT_CONVERSATIONS_LIMIT, - "my_recipient_id": my_recipient_id, + "recipient_id": recipient_id, }, ) - rows = cursor.fetchall() + return cursor.fetchall() - # The resulting rows will be (recipient_id, max_message_id) - # objects for all parties we've had recent (group?) private - # message conversations with, including direct messages with - # yourself (those will generate an empty list of user_ids). - for recipient_id, max_message_id in rows: - recipient_map[recipient_id] = dict( - max_message_id=max_message_id, - user_ids=[], + +def _get_recent_conversations_via_direct_message_group( + user_profile_id: int, +) -> list[tuple[int, int]]: + """ + This functions fetches the most recent conversations given that all + private messages of this user are through direct message groups. + """ + RECENT_CONVERSATIONS_LIMIT = 1000 + + recent_pm_message_ids = ( + UserMessage.objects.filter(user_profile_id=user_profile_id) + .extra(where=[UserMessage.where_flag_is_present(UserMessage.flags.is_private)]) # noqa: S610 + .order_by("-message_id") + .values_list("message_id", flat=True)[:RECENT_CONVERSATIONS_LIMIT] + ) + + return list( + Message.objects.filter(id__in=recent_pm_message_ids) + .values("recipient_id") + .annotate(max_message_id=Max("id")) + .values_list("recipient_id", "max_message_id") + ) + + +def get_recent_private_conversations(user_profile: UserProfile) -> dict[int, dict[str, Any]]: + """ + We return a dictionary structure for convenient modification + below; this structure is converted into its final form by + post_process. + """ + # Step 1: Collect recent message info + if user_profile.recipient_id is not None: + recent_conversations = _get_recent_conversations_via_legacy_personal_recipient( + user_profile.id, user_profile.recipient_id ) + else: + recent_conversations = _get_recent_conversations_via_direct_message_group(user_profile.id) + + recipient_map: dict[int, dict[str, Any]] = { + recipient_id: {"max_message_id": max_message_id, "user_ids": []} + for recipient_id, max_message_id in recent_conversations + } # Now we need to map all the recipient_id objects to lists of user IDs - for recipient_id, user_profile_id in ( + subscriptions = ( Subscription.objects.filter(recipient_id__in=recipient_map.keys()) .exclude(user_profile_id=user_profile.id) .values_list("recipient_id", "user_profile_id") - ): + ) + for recipient_id, user_profile_id in subscriptions: recipient_map[recipient_id]["user_ids"].append(user_profile_id) # Sort to prevent test flakes and client bugs. diff --git a/zerver/lib/narrow.py b/zerver/lib/narrow.py index be956eccfc..a8fb4b968e 100644 --- a/zerver/lib/narrow.py +++ b/zerver/lib/narrow.py @@ -654,13 +654,17 @@ class NarrowBuilder: narrow_user_profile ) + private_flag = column("flags", Integer).op("&")(UserMessage.flags.is_private.mask) != 0 + realm_match = column("realm_id", Integer) == self.realm.id + direct_message_group_cond = column("recipient_id", Integer).in_( + direct_message_group_recipient_ids + ) + self_recipient_id = self.user_profile.recipient_id - # See note above in `by_dm` about needing bidirectional messages - # for direct messages with another person. - cond = and_( - column("flags", Integer).op("&")(UserMessage.flags.is_private.mask) != 0, - column("realm_id", Integer) == self.realm.id, - or_( + if self_recipient_id is not None and narrow_user_profile.recipient_id is not None: + # See note above in `by_dm` about needing bidirectional messages + # for direct messages with another person. + dm_conditions = or_( and_( column("sender_id", Integer) == narrow_user_profile.id, column("recipient_id", Integer) == self_recipient_id, @@ -669,11 +673,13 @@ class NarrowBuilder: column("sender_id", Integer) == self.user_profile.id, column("recipient_id", Integer) == narrow_user_profile.recipient_id, ), - and_( - column("recipient_id", Integer).in_(direct_message_group_recipient_ids), - ), - ), - ) + direct_message_group_cond, + ) + cond = and_(private_flag, realm_match, dm_conditions) + else: + # If the user does not have a recipient_id, then they only rely on + # direct group message for their conversations. + cond = and_(private_flag, realm_match, direct_message_group_cond) return query.where(maybe_negate(cond)) def by_group_pm_with( diff --git a/zerver/lib/recipient_users.py b/zerver/lib/recipient_users.py index 2677ac5249..5b5ec70cd1 100644 --- a/zerver/lib/recipient_users.py +++ b/zerver/lib/recipient_users.py @@ -58,19 +58,23 @@ def get_recipient_from_user_profiles( type_id=direct_message_group.id, ) - # If no direct message group recipient exists, or if the setting - # is disabled, we fall back to using personal recipients. - del recipient_profiles_map[sender.id] - if len(recipient_profiles_map) == 1: - [recipient_user_profile] = recipient_profiles_map.values() - else: - recipient_user_profile = sender - return Recipient( - id=recipient_user_profile.recipient_id, - type=Recipient.PERSONAL, - type_id=recipient_user_profile.id, + # Making sure we have personal recipients for all users, + # otherwise, fall back to the direct message group. + has_personal_recipient = all( + user_profile.recipient_id is not None + for user_profile in recipient_profiles_map.values() ) - + if has_personal_recipient: + del recipient_profiles_map[sender.id] + if len(recipient_profiles_map) == 1: + [recipient_user_profile] = recipient_profiles_map.values() + else: + recipient_user_profile = sender + return Recipient( + id=recipient_user_profile.recipient_id, + type=Recipient.PERSONAL, + type_id=recipient_user_profile.id, + ) if create: direct_message_group = get_or_create_direct_message_group(user_ids) else: diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 55039c47d2..3c0ac6b29b 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -757,8 +757,10 @@ def check_can_access_user( ).exists(): return True - assert user_profile.recipient_id is not None - assert target_user.recipient_id is not None + if user_profile.recipient_id is None or target_user.recipient_id is None: + # If either user does not have a recipient_id, they rely on + # direct message groups for 1:1 or self DMs. + return False # Querying the "Message" table is expensive so we do this last. direct_message_query = Message.objects.filter( @@ -810,6 +812,11 @@ def get_inaccessible_user_ids( if not possible_inaccessible_user_ids: return set() + if not acting_user.recipient_id: + # If the acting user does not have a recipient_id, they only rely on + # direct message groups for 1:1 or self DMs. + return possible_inaccessible_user_ids + target_user_recipient_ids = UserProfile.objects.filter( id__in=possible_inaccessible_user_ids ).values_list("recipient_id", flat=True) @@ -948,7 +955,9 @@ def get_users_involved_in_dms_with_target_users( direct_message_participants_dict[sender_id] = recipient_user_ids - personal_recipient_ids_for_target_users = [user.recipient_id for user in target_users] + personal_recipient_ids_for_target_users = [ + user.recipient_id for user in target_users if user.recipient_id is not None + ] direct_message_senders_query = Message.objects.filter( realm=realm, recipient_id__in=personal_recipient_ids_for_target_users, diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index 59fdf6eab4..b041f66c48 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -14,6 +14,7 @@ from django.core.exceptions import ValidationError from django.core.management.base import CommandError from django.db.models import Q, QuerySet from django.forms.models import model_to_dict +from django.test import override_settings from django.utils.timezone import now as timezone_now from typing_extensions import override @@ -112,7 +113,10 @@ from zerver.models.messages import ImageAttachment from zerver.models.presence import PresenceSequence from zerver.models.realm_audit_logs import AuditLogEventType from zerver.models.realms import get_realm -from zerver.models.recipients import get_direct_message_group_hash +from zerver.models.recipients import ( + get_direct_message_group_hash, + get_or_create_direct_message_group, +) from zerver.models.streams import get_active_streams, get_stream from zerver.models.users import ExternalAuthID, get_system_bot, get_user_by_delivery_email @@ -1015,6 +1019,12 @@ class RealmImportExportTest(ExportFile): self.assertEqual(get_consented_user_ids(realm), consented_user_ids) + # Remove the recipient of the welcome bot to test exporting bots without personal recipients. + internal_realm = get_realm(settings.SYSTEM_BOT_REALM) + welcome_bot = get_system_bot(settings.WELCOME_BOT, internal_realm.id) + welcome_bot.recipient = None + welcome_bot.save(update_fields=["recipient"]) + self.export_realm_and_create_auditlog( realm, export_type=RealmExport.EXPORT_FULL_WITH_CONSENT, @@ -2992,6 +3002,55 @@ class SingleUserExportTest(ExportFile): ], ) + @override_settings(PREFER_DIRECT_MESSAGE_GROUP=True) + def test_1_to_1_message_data_using_direct_message_group(self) -> None: + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + othello = self.example_user("othello") + bot = self.create_test_bot("test-bot", hamlet) + + get_or_create_direct_message_group([hamlet.id, cordelia.id]) + get_or_create_direct_message_group([hamlet.id, bot.id]) + get_or_create_direct_message_group([hamlet.id]) + + hi_hamlet_message_id = self.send_personal_message(othello, hamlet, "hi hamlet") + hi_cordelia_message_id = self.send_personal_message(hamlet, cordelia, "hi cordelia") + bye_hamlet_message_id = self.send_personal_message(cordelia, hamlet, "bye hamlet") + test_bot_message_id = self.send_personal_message(hamlet, bot, "test bot message") + self.send_personal_message(othello, cordelia, "an irrelevant message") + bye_peeps_message_id = self.send_group_direct_message( + othello, [cordelia, hamlet], "bye peeps" + ) + self_message_id = self.send_personal_message(hamlet, hamlet, "hi myself") + + output_dir = make_export_output_dir() + hamlet = self.example_user("hamlet") + + with self.assertLogs(level="INFO"): + do_export_user(hamlet, output_dir) + + messages = read_json("messages-000001.json") + + excerpt = [ + (rec["id"], rec["content"], rec["recipient_name"]) + for rec in messages["zerver_message"][-6:] + ] + self.assertEqual( + excerpt, + [ + (hi_hamlet_message_id, "hi hamlet", hamlet.full_name), + (hi_cordelia_message_id, "hi cordelia", cordelia.full_name), + (bye_hamlet_message_id, "bye hamlet", hamlet.full_name), + (test_bot_message_id, "test bot message", bot.full_name), + ( + bye_peeps_message_id, + "bye peeps", + f"{cordelia.full_name}, {hamlet.full_name}, {othello.full_name}", + ), + (self_message_id, "hi myself", hamlet.full_name), + ], + ) + def test_user_data(self) -> None: # We register checkers during test setup, and then we call them at the end. checkers = {} diff --git a/zerver/tests/test_message_fetch.py b/zerver/tests/test_message_fetch.py index ef10302244..5200a3f47b 100644 --- a/zerver/tests/test_message_fetch.py +++ b/zerver/tests/test_message_fetch.py @@ -562,6 +562,18 @@ class NarrowBuilderTest(ZulipTestCase): "WHERE NOT ((flags & %(flags_1)s) != %(param_1)s AND realm_id = %(realm_id_1)s AND (sender_id = %(sender_id_1)s AND recipient_id = %(recipient_id_1)s OR sender_id = %(sender_id_2)s AND recipient_id = %(recipient_id_2)s OR recipient_id IN (__[POSTCOMPILE_recipient_id_3])))", ) + def test_add_term_using_dm_including_operator_without_personal_recipient(self) -> None: + # Dropping the personal recipient for Othello + othello = self.example_user("othello") + othello.recipient = None + othello.save() + + term = NarrowParameter(operator="dm-including", operand=self.othello_email) + self._do_add_term_test( + term, + "WHERE (flags & %(flags_1)s) != %(param_1)s AND realm_id = %(realm_id_1)s AND recipient_id IN (__[POSTCOMPILE_recipient_id_1])", + ) + def test_add_term_using_id_operator_integer(self) -> None: term = NarrowParameter(operator="id", operand=555) self._do_add_term_test(term, "WHERE id = %(param_1)s") diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index 7b700b4c6a..1eba3a4d70 100644 --- a/zerver/tests/test_message_send.py +++ b/zerver/tests/test_message_send.py @@ -2449,6 +2449,28 @@ class StreamMessagesTest(ZulipTestCase): ) self.assertEqual(recent_conversation["max_message_id"], message2_id) + def test_get_raw_unread_data_for_1_to_1_dms_using_group_direct_message(self) -> None: + sender = self.example_user("hamlet") + receiver = self.example_user("cordelia") + receiver.recipient = None + receiver.save() + + message1_id = self.send_personal_message(sender, receiver, "test content 1") + message2_id = self.send_personal_message(sender, receiver, "test content 2") + + msg_data = get_raw_unread_data(receiver) + + self.assert_length(msg_data["pm_dict"].keys(), 2) + self.assert_length(msg_data["huddle_dict"].keys(), 0) + + self.assertIn(message1_id, msg_data["pm_dict"].keys()) + self.assertIn(message2_id, msg_data["pm_dict"].keys()) + + recent_conversations = get_recent_private_conversations(receiver) + [recent_conversation] = recent_conversations.values() + self.assertEqual(set(recent_conversation["user_ids"]), {sender.id}) + self.assertEqual(recent_conversation["max_message_id"], message2_id) + def test_stream_becomes_active_on_message_send(self) -> None: # Mark a stream as inactive stream = self.make_stream("inactive_stream") @@ -2544,10 +2566,11 @@ class PersonalMessageSendTest(ZulipTestCase): self.assertEqual(message_stream_count(receiver), receiver_messages + 1) direct_message_group = get_direct_message_group([sender.id, receiver.id]) - if direct_message_group and settings.PREFER_DIRECT_MESSAGE_GROUP: - recipient = Recipient.objects.get( - type_id=direct_message_group.id, type=Recipient.DIRECT_MESSAGE_GROUP - ) + has_none_recipient = receiver.recipient is None or sender.recipient is None + if has_none_recipient: + recipient = get_or_create_direct_message_group([sender.id, receiver.id]).recipient + elif settings.PREFER_DIRECT_MESSAGE_GROUP and direct_message_group: + recipient = direct_message_group.recipient else: recipient = Recipient.objects.get(type_id=receiver.id, type=Recipient.PERSONAL) @@ -2582,6 +2605,26 @@ class PersonalMessageSendTest(ZulipTestCase): receiver=receiver, ) + def test_personal_when_personal_recipient_is_none(self) -> None: + """ + If you send a personal using direct_message_group, only you and the recipient see it. + """ + sender = self.example_user("hamlet") + receiver = self.example_user("othello") + + # Removing the personal recipient to ensure a new direct message group is created. + receiver.recipient = None + receiver.save() + + self.login("hamlet") + self.assert_personal( + sender=sender, + receiver=receiver, + ) + + message = most_recent_message(sender) + self.assertEqual(message.recipient.type, Recipient.DIRECT_MESSAGE_GROUP) + def test_direct_message_initiator_group_setting(self) -> None: """ Tests that direct_message_initiator_group_setting works correctly. diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 94835d4f9c..41f620b32e 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -18,7 +18,10 @@ from zerver.actions.create_user import do_create_user, do_reactivate_user from zerver.actions.invites import do_create_multiuse_invite_link, do_invite_users from zerver.actions.message_send import RecipientInfoResult, get_recipient_info from zerver.actions.muted_users import do_mute_user -from zerver.actions.realm_settings import do_set_realm_property +from zerver.actions.realm_settings import ( + do_change_realm_permission_group_setting, + do_set_realm_property, +) from zerver.actions.user_settings import bulk_regenerate_api_keys, do_change_user_setting from zerver.actions.user_topics import do_set_user_topic_visibility_policy from zerver.actions.users import ( @@ -76,7 +79,7 @@ from zerver.models import ( ) from zerver.models.clients import get_client from zerver.models.custom_profile_fields import check_valid_user_ids -from zerver.models.groups import SystemGroups +from zerver.models.groups import NamedUserGroup, SystemGroups from zerver.models.prereg_users import filter_to_valid_prereg_users from zerver.models.realm_audit_logs import AuditLogEventType from zerver.models.realms import InvalidFakeEmailDomainError, get_fake_email_domain, get_realm @@ -590,6 +593,29 @@ class PermissionTest(ZulipTestCase): self.example_user("cordelia"), self.example_user("aaron").id, for_admin=False ) + def test_access_user_by_id_when_personal_recipient_is_none(self) -> None: + self.set_up_db_for_testing_user_access() + polonius = self.example_user("polonius") + + # Removing the personal recipient to ensure a new direct message group is used for 1:1 dms. + polonius.recipient = None + polonius.save() + + # Restricting the "Members" system group to not allow access to all users. + realm = get_realm("zulip") + members_system_group = NamedUserGroup.objects.get(name=SystemGroups.MEMBERS, realm=realm) + do_change_realm_permission_group_setting( + realm, "can_access_all_users_group", members_system_group, acting_user=None + ) + + aaron = self.example_user("aaron") + target_user = access_user_by_id(polonius, aaron.id, for_admin=False) + self.assertEqual(target_user, aaron) + + othello = self.example_user("othello") + with self.assertRaises(JsonableError): + access_user_by_id(polonius, othello.id, for_admin=False) + def check_property_for_role(self, user_profile: UserProfile, role: int) -> bool: if role == UserProfile.ROLE_REALM_ADMINISTRATOR: return ( @@ -3206,6 +3232,26 @@ class GetProfileTest(ZulipTestCase): ) self.assertEqual(inaccessible_user_ids, {othello.id}) + def test_get_inaccessible_user_ids_when_personal_recipient_is_none(self) -> None: + polonius = self.example_user("polonius") + + # Removing the personal recipient to ensure we use direct message group. + polonius.recipient = None + polonius.save() + + bot = self.example_user("default_bot") + hamlet = self.example_user("hamlet") + othello = self.example_user("othello") + + inaccessible_user_ids = get_inaccessible_user_ids([bot.id, hamlet.id, othello.id], polonius) + self.assert_length(inaccessible_user_ids, 0) + + self.set_up_db_for_testing_user_access() + polonius = self.example_user("polonius") + + inaccessible_user_ids = get_inaccessible_user_ids([bot.id, hamlet.id, othello.id], polonius) + self.assertEqual(inaccessible_user_ids, {othello.id}) + def test_get_users_for_spectators(self) -> None: # Checks that spectators can fetch users data. hamlet = self.example_user("hamlet")