From 4789de2e96ee9df9c2fc45831cc0faa699c07656 Mon Sep 17 00:00:00 2001 From: roanster007 Date: Wed, 12 Feb 2025 21:04:26 +0530 Subject: [PATCH] markdown: Fix #-mention of private channel's topic made by system bots. Previously when system bots used to `#-mention` a private channel's topics in cases like moving messages of a private channel, then the #-mentions were not rendered by the markdown. This was because the system bots did not have authorization to mention these channels. This is fixed by passing down an `acting_user` parameter in code paths involving sending these move message notifications so that permission of acting_user to mention the topic is verified for rendering the markdown, rather than that of the system bot. --- zerver/actions/message_edit.py | 3 +++ zerver/actions/message_send.py | 12 ++++++++++++ zerver/lib/markdown/__init__.py | 18 +++++++++++++----- zerver/lib/mention.py | 10 +++------- zerver/tests/test_markdown.py | 32 ++++++++++++++++++++++++++++++++ 5 files changed, 63 insertions(+), 12 deletions(-) diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 42a35fec28..5580ee78b3 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -226,6 +226,7 @@ def maybe_send_resolve_topic_notifications( ), message_type=Message.MessageType.RESOLVE_TOPIC_NOTIFICATION, limit_unread_user_ids=affected_participant_ids, + acting_user=user_profile, ) return resolved_topic_message_id, False @@ -293,6 +294,7 @@ def send_message_moved_breadcrumbs( user=user_mention, changed_messages_count=changed_messages_count, ), + acting_user=user_profile, ) if old_thread_notification_string is not None: @@ -307,6 +309,7 @@ def send_message_moved_breadcrumbs( new_location=new_topic_link, changed_messages_count=changed_messages_count, ), + acting_user=user_profile, ) diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py index 759a5fc57d..7a3474862c 100644 --- a/zerver/actions/message_send.py +++ b/zerver/actions/message_send.py @@ -156,6 +156,7 @@ def render_incoming_message( mention_data: MentionData | None = None, url_embed_data: dict[str, UrlEmbedData | None] | None = None, email_gateway: bool = False, + acting_user: UserProfile | None = None, ) -> MessageRenderingResult: realm_alert_words_automaton = get_alert_word_automaton(realm) try: @@ -167,6 +168,7 @@ def render_incoming_message( mention_data=mention_data, url_embed_data=url_embed_data, email_gateway=email_gateway, + acting_user=acting_user, ) except MarkdownRenderingError: raise JsonableError(_("Unable to render message")) @@ -576,6 +578,7 @@ def build_message_send_dict( limit_unread_user_ids: set[int] | None = None, disable_external_notifications: bool = False, recipients_for_user_creation_events: dict[UserProfile, set[int]] | None = None, + acting_user: UserProfile | None = None, ) -> SendMessageRequest: """Returns a dictionary that can be passed into do_send_messages. In production, this is always called by check_message, but some @@ -620,6 +623,7 @@ def build_message_send_dict( realm, mention_data=mention_data, email_gateway=email_gateway, + acting_user=acting_user, ) message.rendered_content = rendering_result.rendered_content message.rendered_content_version = markdown_version @@ -1707,6 +1711,7 @@ def check_message( limit_unread_user_ids: set[int] | None = None, disable_external_notifications: bool = False, archived_channel_notice: bool = False, + acting_user: UserProfile | None = None, ) -> SendMessageRequest: """See https://zulip.readthedocs.io/en/latest/subsystems/sending-messages.html @@ -1850,6 +1855,7 @@ def check_message( limit_unread_user_ids=limit_unread_user_ids, disable_external_notifications=disable_external_notifications, recipients_for_user_creation_events=recipients_for_user_creation_events, + acting_user=acting_user, ) if ( @@ -1888,6 +1894,7 @@ def _internal_prep_message( forged: bool = False, forged_timestamp: float | None = None, archived_channel_notice: bool = False, + acting_user: UserProfile | None = None, ) -> SendMessageRequest | None: """ Create a message object and checks it, but doesn't send it or save it to the database. @@ -1920,6 +1927,7 @@ def _internal_prep_message( forged=forged, forged_timestamp=forged_timestamp, archived_channel_notice=archived_channel_notice, + acting_user=acting_user, ) except JsonableError as e: logging.exception( @@ -1944,6 +1952,7 @@ def internal_prep_stream_message( forged: bool = False, forged_timestamp: float | None = None, archived_channel_notice: bool = False, + acting_user: UserProfile | None = None, ) -> SendMessageRequest | None: """ See _internal_prep_message for details of how this works. @@ -1962,6 +1971,7 @@ def internal_prep_stream_message( forged=forged, forged_timestamp=forged_timestamp, archived_channel_notice=archived_channel_notice, + acting_user=acting_user, ) @@ -2041,6 +2051,7 @@ def internal_send_stream_message( message_type: int = Message.MessageType.NORMAL, limit_unread_user_ids: set[int] | None = None, archived_channel_notice: bool = False, + acting_user: UserProfile | None = None, ) -> int | None: message = internal_prep_stream_message( sender, @@ -2051,6 +2062,7 @@ def internal_send_stream_message( message_type=message_type, limit_unread_user_ids=limit_unread_user_ids, archived_channel_notice=archived_channel_notice, + acting_user=acting_user, ) if message is None: diff --git a/zerver/lib/markdown/__init__.py b/zerver/lib/markdown/__init__.py index a7a8f3c089..5c246cda20 100644 --- a/zerver/lib/markdown/__init__.py +++ b/zerver/lib/markdown/__init__.py @@ -67,7 +67,7 @@ from zerver.lib.timezone import common_timezones from zerver.lib.types import LinkifierDict from zerver.lib.url_encoding import encode_stream, hash_util_encode from zerver.lib.url_preview.types import UrlEmbedData, UrlOEmbedData -from zerver.models import Message, Realm +from zerver.models import Message, Realm, UserProfile from zerver.models.linkifiers import linkifiers_for_realm from zerver.models.realm_emoji import EmojiInfo, get_name_keyed_dict_for_active_realm_emoji @@ -2697,6 +2697,7 @@ def do_convert( mention_data: MentionData | None = None, email_gateway: bool = False, no_previews: bool = False, + acting_user: UserProfile | None = None, ) -> MessageRenderingResult: """Convert Markdown to HTML, with Zulip-specific settings and hacks.""" # This logic is a bit convoluted, but the overall goal is to support a range of use cases: @@ -2772,13 +2773,16 @@ def do_convert( mention_backend = MentionBackend(message_realm.id) mention_data = MentionData(mention_backend, content, message_sender) + if acting_user is None: + acting_user = message_sender + stream_names = possible_linked_stream_names(content) - stream_name_info = mention_data.get_stream_name_map( - stream_names, acting_user=message_sender - ) + stream_name_info = mention_data.get_stream_name_map(stream_names, acting_user=acting_user) linked_stream_topic_data = possible_linked_topics(content) - topic_info = mention_data.get_topic_info_map(linked_stream_topic_data) + topic_info = mention_data.get_topic_info_map( + linked_stream_topic_data, acting_user=acting_user + ) if content_has_emoji_syntax(content): active_realm_emoji = get_name_keyed_dict_for_active_realm_emoji(message_realm.id) @@ -2877,6 +2881,7 @@ def markdown_convert( mention_data: MentionData | None = None, email_gateway: bool = False, no_previews: bool = False, + acting_user: UserProfile | None = None, ) -> MessageRenderingResult: markdown_stats_start() ret = do_convert( @@ -2890,6 +2895,7 @@ def markdown_convert( mention_data, email_gateway, no_previews=no_previews, + acting_user=acting_user, ) markdown_stats_finish() return ret @@ -2903,6 +2909,7 @@ def render_message_markdown( url_embed_data: dict[str, UrlEmbedData | None] | None = None, mention_data: MentionData | None = None, email_gateway: bool = False, + acting_user: UserProfile | None = None, ) -> MessageRenderingResult: """ This is basically just a wrapper for do_render_markdown. @@ -2925,6 +2932,7 @@ def render_message_markdown( url_embed_data=url_embed_data, mention_data=mention_data, email_gateway=email_gateway, + acting_user=acting_user, ) return rendering_result diff --git a/zerver/lib/mention.py b/zerver/lib/mention.py index fceaf778e8..a46f764ea2 100644 --- a/zerver/lib/mention.py +++ b/zerver/lib/mention.py @@ -423,16 +423,12 @@ class MentionData: def get_stream_name_map( self, stream_names: set[str], acting_user: UserProfile | None ) -> dict[str, int]: - return self.mention_backend.get_stream_name_map( - stream_names, acting_user=self.message_sender - ) + return self.mention_backend.get_stream_name_map(stream_names, acting_user=acting_user) def get_topic_info_map( - self, channel_topics: set[ChannelTopicInfo] + self, channel_topics: set[ChannelTopicInfo], acting_user: UserProfile | None ) -> dict[ChannelTopicInfo, int | None]: - return self.mention_backend.get_topic_info_map( - channel_topics, acting_user=self.message_sender - ) + return self.mention_backend.get_topic_info_map(channel_topics, acting_user=acting_user) def silent_mention_syntax_for_user(user_profile: UserProfile) -> str: diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index c4289f113a..f72edfa8d8 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -72,6 +72,7 @@ from zerver.models.groups import SystemGroups from zerver.models.linkifiers import linkifiers_for_realm from zerver.models.realms import get_realm from zerver.models.streams import get_stream +from zerver.models.users import get_system_bot class SimulatedFencedBlockPreprocessor(FencedBlockPreprocessor): @@ -3236,6 +3237,21 @@ class MarkdownStreamTopicMentionTests(ZulipTestCase): f'

#{core_stream.name} > testing

', ) + # Test permalinks generated when a user with no access to the channel + # sends a topic link on behalf of a user who has access to the channel. + notification_bot = get_system_bot(settings.NOTIFICATION_BOT, iago.realm_id) + + msg = Message( + sender=notification_bot, + sending_client=get_client("test"), + realm=realm, + ) + content = "#**core>testing**" + self.assertEqual( + render_message_markdown(msg, content, acting_user=iago).rendered_content, + f'

#{core_stream.name} > testing

', + ) + # Test newly subscribed user to a channel with protected history # won't have accessed to this message, and hence, the topic # link would not be a permalink. @@ -3252,6 +3268,22 @@ class MarkdownStreamTopicMentionTests(ZulipTestCase): f'

#{core_stream.name} > testing

', ) + # Test permalinks would not be generated when a user with no access to the + # channel sends a topic link on behalf of another user who has no access to + # the channel. + cordelia = self.example_user("cordelia") + + msg = Message( + sender=notification_bot, + sending_client=get_client("test"), + realm=realm, + ) + content = "#**core>testing**" + self.assertEqual( + render_message_markdown(msg, content, acting_user=cordelia).rendered_content, + "

#core>testing

", + ) + # Test when trying to render a topic link of a channel with protected # history, if message_sender is None, topic link is not permalink. content = "#**core>testing**"