From f9f111f950054dcf7d077dc09aa4d5e3d99e9e30 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Thu, 24 Mar 2022 15:51:31 -0700 Subject: [PATCH] message_edit: Only move muted topic records when moving whole topics. Our original implementation of moving muted topic records when a topic is moved took a shortcut of treating all change_later usage as something with intent to move the whole topic. This works OK when moving the whole topic via this interface, but not when moving a last off-topic message in the topic. Address this by changing the rule to match the existing moved_all_visible_messages variable. --- .../help/move-content-to-another-stream.md | 22 +++- .../help/move-content-to-another-topic.md | 4 +- zerver/lib/actions.py | 115 +++++++++--------- zerver/tests/test_message_edit.py | 28 ++++- 4 files changed, 106 insertions(+), 63 deletions(-) diff --git a/templates/zerver/help/move-content-to-another-stream.md b/templates/zerver/help/move-content-to-another-stream.md index 65142dd11c..c6845e256f 100644 --- a/templates/zerver/help/move-content-to-another-stream.md +++ b/templates/zerver/help/move-content-to-another-stream.md @@ -6,8 +6,8 @@ Organizations can [configure][move-permission-setting] which streams. To help others find moved content, you can have Zulip send automated -notification messages to the source topic, the destination topic, or both. These -notifications include: +notification messages to the source topic, the destination topic, or +both. These notifications include: * A link to the source or destination topic. * How many messages were moved, or whether the whole topic was moved. @@ -82,6 +82,24 @@ will only be accessible to users who both: * Were subscribed to the *original* stream when the content was *sent*. * Are subscribed to the *destination* stream when the content is *moved*. +## Moving content out of private streams + +In [private streams with protected history](/help/stream-permissions), +Zulip determines whether to treat the entire topic as moved using the +access permissions of the user requesting the topic move. This means +that the automated notifications will report that the entire topic was +moved if the requesting user moved every message in the topic that +they can access, regardless of whether older messages exist that +they cannot access. + +Similarly, [muted topics](/help/mute-a-topic) will be migrated to the +new stream and topic if the requesting user moved every message in the +topic that they can access. + +This model ensures that the topic editing feature cannot be abused to +determine any information about the existence of messages or topics +that one does not have permission to access. + ## Related articles * [Rename a topic](/help/rename-a-topic) diff --git a/templates/zerver/help/move-content-to-another-topic.md b/templates/zerver/help/move-content-to-another-topic.md index 8590f16a91..2ed07e5848 100644 --- a/templates/zerver/help/move-content-to-another-topic.md +++ b/templates/zerver/help/move-content-to-another-topic.md @@ -8,8 +8,8 @@ topic](/help/rename-a-topic). When messages are moved, Zulip's [permanent links to messages in context](/help/link-to-a-message-or-conversation#get-a-link-to-a-specific-message) will automatically redirect to the new location of the message. [Muted -topics](/help/mute-a-topic) are automatically migrated when all messages after a -certain point are moved, or an entire topic is moved. +topics](/help/mute-a-topic) are automatically migrated when an entire +topic is moved. Organizations can [configure](/help/configure-who-can-edit-topics) which [roles](/help/roles-and-permissions) have permission to modify topics. See the diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index a6866f2c22..65f05c59f6 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -7166,64 +7166,12 @@ def do_update_message( users_to_be_notified += list(map(subscriber_info, sorted(list(subscriber_ids)))) - # Migrate muted topic configuration in the following circumstances: - # - # * If propagate_mode is change_all, do so unconditionally. - # - # * If propagate_mode is change_later, it's likely that we want to - # move these only when it appears that the intent is to move - # most of the topic, not just the last 1-2 messages which may - # have been "off topic". At present we do so unconditionally. - # - # * Never move muted topic configuration with change_one. - # - # We may want more complex behavior in cases where one appears to - # be merging topics (E.g. there are existing messages in the - # target topic). - # - # Moving a topic to another stream is complicated in that we want - # to avoid creating a UserTopic row for the user in a stream that - # they don't have access to; doing so could leak information about - # the existence of a private stream to some users. See the - # moved_all_visible_messages below for related details. - # - # So for now, we require new_stream=None for this feature. - if propagate_mode != "change_one" and (topic_name is not None or new_stream is not None): + # UserTopic updates and the content of notifications depend on + # whether we've moved the entire topic, or just part of it. We + # make that determination here. + moved_all_visible_messages = False + if topic_name is not None or new_stream is not None: assert stream_being_edited is not None - for muting_user in get_users_muting_topic(stream_being_edited.id, orig_topic_name): - # TODO: Ideally, this would be a bulk update operation, - # because we are doing database operations in a loop here. - # - # This loop is only acceptable in production because it is - # rare for more than a few users to have muted an - # individual topic that is being moved; as of this - # writing, no individual topic in Zulip Cloud had been - # muted by more than 100 users. - - if new_stream is not None and muting_user.id in delete_event_notify_user_ids: - # If the messages are being moved to a stream the user - # cannot access, then we treat this as the - # messages/topic being deleted for this user. Unmute - # the topic for such users. - do_unmute_topic(muting_user, stream_being_edited, orig_topic_name) - else: - # Otherwise, we move the muted topic record for the user. - # We call remove_topic_mute rather than do_unmute_topic to - # avoid sending two events with new muted topics in - # immediate succession; this is correct only because - # muted_topics events always send the full set of topics. - remove_topic_mute(muting_user, stream_being_edited.id, orig_topic_name) - do_mute_topic( - muting_user, - new_stream if new_stream is not None else stream_being_edited, - topic_name if topic_name is not None else orig_topic_name, - ) - - send_event(user_profile.realm, event, users_to_be_notified) - - if len(changed_messages) > 0 and new_stream is not None and stream_being_edited is not None: - # Notify users that the topic was moved. - changed_messages_count = len(changed_messages) if propagate_mode == "change_all": moved_all_visible_messages = True @@ -7253,6 +7201,59 @@ def do_update_message( ) moved_all_visible_messages = len(visible_unmoved_messages) == 0 + # Migrate muted topic configuration in the following circumstances: + # + # * If propagate_mode is change_all, do so unconditionally. + # + # * If propagate_mode is change_later or change_one, do so when + # the acting user has moved the entire topic (as visible to them). + # + # This rule corresponds to checking moved_all_visible_messages. + # + # We may want more complex behavior in cases where one appears to + # be merging topics (E.g. there are existing messages in the + # target topic). + if moved_all_visible_messages: + assert stream_being_edited is not None + assert topic_name is not None or new_stream is not None + + for muting_user in get_users_muting_topic(stream_being_edited.id, orig_topic_name): + # TODO: Ideally, this would be a bulk update operation, + # because we are doing database operations in a loop here. + # + # This loop is only acceptable in production because it is + # rare for more than a few users to have muted an + # individual topic that is being moved; as of this + # writing, no individual topic in Zulip Cloud had been + # muted by more than 100 users. + + if new_stream is not None and muting_user.id in delete_event_notify_user_ids: + # If the messages are being moved to a stream the user + # cannot access, then we treat this as the + # messages/topic being deleted for this user. This is + # important for security reasons; we don't want to + # give users a UserTopic row in a stream they cannot + # access. Unmute the topic for such users. + do_unmute_topic(muting_user, stream_being_edited, orig_topic_name) + else: + # Otherwise, we move the muted topic record for the user. + # We call remove_topic_mute rather than do_unmute_topic to + # avoid sending two events with new muted topics in + # immediate succession; this is correct only because + # muted_topics events always send the full set of topics. + remove_topic_mute(muting_user, stream_being_edited.id, orig_topic_name) + do_mute_topic( + muting_user, + new_stream if new_stream is not None else stream_being_edited, + topic_name if topic_name is not None else orig_topic_name, + ) + + send_event(user_profile.realm, event, users_to_be_notified) + + if len(changed_messages) > 0 and new_stream is not None and stream_being_edited is not None: + # Notify users that the topic was moved. + changed_messages_count = len(changed_messages) + old_thread_notification_string = None if send_notification_to_old_thread: if moved_all_visible_messages: diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index c1daea3874..188ce7a166 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1342,8 +1342,8 @@ class EditMessageTest(EditMessageTestCase): send_notification_to_new_thread=False, content=None, ) - self.assertFalse(topic_is_muted(hamlet, stream.id, change_one_topic_name)) - self.assertTrue(topic_is_muted(hamlet, stream.id, change_later_topic_name)) + self.assertTrue(topic_is_muted(hamlet, stream.id, change_one_topic_name)) + self.assertFalse(topic_is_muted(hamlet, stream.id, change_later_topic_name)) # Move topic between two public streams. desdemona = self.example_user("desdemona") @@ -1445,6 +1445,30 @@ class EditMessageTest(EditMessageTestCase): self.assertTrue(topic_is_muted(cordelia, new_public_stream.id, "changed topic name")) self.assertFalse(topic_is_muted(aaron, new_public_stream.id, "changed topic name")) + # Moving only half the messages doesn't move MutedTopic records. + second_message_id = self.send_stream_message( + hamlet, stream_name, topic_name="changed topic name", content="Second message" + ) + with queries_captured() as queries: + check_update_message( + user_profile=desdemona, + message_id=second_message_id, + stream_id=new_public_stream.id, + topic_name="final topic name", + propagate_mode="change_later", + send_notification_to_old_thread=False, + send_notification_to_new_thread=False, + content=None, + ) + self.assert_length(queries, 25) + + self.assertTrue(topic_is_muted(desdemona, new_public_stream.id, "changed topic name")) + self.assertTrue(topic_is_muted(cordelia, new_public_stream.id, "changed topic name")) + self.assertFalse(topic_is_muted(aaron, new_public_stream.id, "changed topic name")) + self.assertFalse(topic_is_muted(desdemona, new_public_stream.id, "final topic name")) + self.assertFalse(topic_is_muted(cordelia, new_public_stream.id, "final topic name")) + self.assertFalse(topic_is_muted(aaron, new_public_stream.id, "final topic name")) + @mock.patch("zerver.lib.actions.send_event") def test_wildcard_mention(self, mock_send_event: mock.MagicMock) -> None: stream_name = "Macbeth"