message: Don't allow moving messages that have passed the time limit.

We previously allowed moving messages that have passed the time limit
using "change_all" value for "propagate_mode" parameter. This commit
changes the behavior to not allow moving messages (both stream and
topic edit) that have passed the time limit for non-admin and
non-moderator users.

api_docs/changelog.md

@@ -24,6 +24,10 @@ format used by the Zulip server that they are interacting with.
 
 * [`PATCH /messages/{message_id}`](/api/update-message): Topic editing
   restrictions now apply to messages without a topic as well.
+* [`PATCH /messages/{message_id}`](/api/update-message): The endpoint
+  now returns an error when users, other than organization administrators
+  and moderators, try to move messages that have passed the time limit
+  using `change_all` value for `propagate_mode` parameter.
 
 **Feature level 171**:
 

version.py

@@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3"
 # Changes should be accompanied by documentation explaining what the
 # new level means in api_docs/changelog.md, as well as "**Changes**"
 # entries in the endpoint's documentation in `zulip.yaml`.
-API_FEATURE_LEVEL = 171
+API_FEATURE_LEVEL = 172
 
 # Bump the minor PROVISION_VERSION to indicate that folks should provision
 # only when going from an old version of the code to a newer version. Bump

zerver/actions/message_edit.py

@@ -21,7 +21,7 @@ from zerver.actions.message_send import (
 )
 from zerver.actions.uploads import check_attachment_reference_change
 from zerver.actions.user_topics import bulk_do_set_user_topic_visibility_policy
-from zerver.lib.exceptions import JsonableError
+from zerver.lib.exceptions import JsonableError, MessageMoveError
 from zerver.lib.markdown import MessageRenderingResult, topic_links
 from zerver.lib.markdown import version as markdown_version
 from zerver.lib.mention import MentionBackend, MentionData, silent_mention_syntax_for_user
@@ -36,7 +36,11 @@ from zerver.lib.message import (
 from zerver.lib.queue import queue_json_publish
 from zerver.lib.stream_subscription import get_active_subscriptions_for_stream_id
 from zerver.lib.stream_topic import StreamTopicTarget
-from zerver.lib.streams import access_stream_by_id, check_stream_access_based_on_stream_post_policy
+from zerver.lib.streams import (
+    access_stream_by_id,
+    can_access_stream_history,
+    check_stream_access_based_on_stream_post_policy,
+)
 from zerver.lib.string_validation import check_stream_topic
 from zerver.lib.timestamp import datetime_to_timestamp
 from zerver.lib.topic import (
@@ -941,6 +945,98 @@ def do_update_message(
     return len(changed_messages)
 
 
+def check_time_limit_for_change_all_propagate_mode(
+    message: Message,
+    user_profile: UserProfile,
+    topic_name: Optional[str] = None,
+    stream_id: Optional[int] = None,
+) -> None:
+    realm = user_profile.realm
+    message_move_limit_buffer = 20
+
+    topic_edit_deadline_seconds = None
+    if topic_name is not None and realm.move_messages_within_stream_limit_seconds is not None:
+        # We set topic_edit_deadline_seconds only if topic is actually
+        # changed and there is some time limit to edit topic.
+        topic_edit_deadline_seconds = (
+            realm.move_messages_within_stream_limit_seconds + message_move_limit_buffer
+        )
+
+    stream_edit_deadline_seconds = None
+    if stream_id is not None and realm.move_messages_between_streams_limit_seconds is not None:
+        # We set stream_edit_deadline_seconds only if stream is
+        # actually changed and there is some time limit to edit
+        # stream.
+        stream_edit_deadline_seconds = (
+            realm.move_messages_between_streams_limit_seconds + message_move_limit_buffer
+        )
+
+    # Calculate whichever of the applicable topic and stream moving
+    # limits is stricter, and use that.
+    if topic_edit_deadline_seconds is not None and stream_edit_deadline_seconds is not None:
+        # When both stream and topic are changed, we consider the
+        # minimum of the two limits to make sure that we raise the
+        # error even when user cannot change one of topic or stream.
+        message_move_deadline_seconds = min(
+            topic_edit_deadline_seconds, stream_edit_deadline_seconds
+        )
+    elif topic_edit_deadline_seconds is not None:
+        message_move_deadline_seconds = topic_edit_deadline_seconds
+    elif stream_edit_deadline_seconds is not None:
+        message_move_deadline_seconds = stream_edit_deadline_seconds
+    else:
+        # There is no applicable time limit for this move request, so
+        # approve it.
+        return
+
+    stream = get_stream_by_id_in_realm(message.recipient.type_id, realm)
+
+    if not can_access_stream_history(user_profile, stream):
+        # If the user doesn't have full access to the stream's
+        # history, check if the user can move the entire portion that
+        # they do have access to.
+        accessible_messages_in_topic = UserMessage.objects.filter(
+            user_profile=user_profile,
+            message__recipient_id=message.recipient_id,
+            message__subject__iexact=message.topic_name(),
+        ).values_list("message_id", flat=True)
+        messages_allowed_to_move: List[int] = list(
+            Message.objects.filter(
+                id__in=accessible_messages_in_topic,
+                date_sent__gt=timezone_now()
+                - datetime.timedelta(seconds=message_move_deadline_seconds),
+            )
+            .order_by("date_sent")
+            .values_list("id", flat=True)
+        )
+        total_messages_requested_to_move = len(accessible_messages_in_topic)
+    else:
+        all_messages_in_topic = (
+            messages_for_topic(message.recipient_id, message.topic_name())
+            .order_by("id")
+            .values_list("id", "date_sent")
+        )
+        oldest_allowed_message_date = timezone_now() - datetime.timedelta(
+            seconds=message_move_deadline_seconds
+        )
+        messages_allowed_to_move = [
+            message[0]
+            for message in all_messages_in_topic
+            if message[1] > oldest_allowed_message_date
+        ]
+        total_messages_requested_to_move = len(all_messages_in_topic)
+
+    if total_messages_requested_to_move == len(messages_allowed_to_move):
+        # We return if all messages are allowed to move.
+        return
+
+    raise MessageMoveError(
+        first_message_id_allowed_to_move=messages_allowed_to_move[0],
+        total_messages_in_topic=total_messages_requested_to_move,
+        total_messages_allowed_to_move=len(messages_allowed_to_move),
+    )
+
+
 def check_update_message(
     user_profile: UserProfile,
     message_id: int,
@@ -1075,6 +1171,14 @@ def check_update_message(
                     _("The time limit for editing this message's stream has passed")
                 )
 
+    if (
+        propagate_mode == "change_all"
+        and not user_profile.is_realm_admin
+        and not user_profile.is_moderator
+        and (topic_name is not None or stream_id is not None)
+    ):
+        check_time_limit_for_change_all_propagate_mode(message, user_profile, topic_name, stream_id)
+
     number_changed = do_update_message(
         user_profile,
         message,

zerver/lib/exceptions.py

@@ -39,6 +39,7 @@ class ErrorCode(Enum):
     AUTHENTICATION_FAILED = auto()
     UNAUTHORIZED = auto()
     REQUEST_TIMEOUT = auto()
+    MOVE_MESSAGES_TIME_LIMIT_EXCEEDED = auto()
 
 
 class JsonableError(Exception):
@@ -474,3 +475,28 @@ class ValidationFailureError(JsonableError):
     def __init__(self, error: ValidationError) -> None:
         super().__init__(error.messages[0])
         self.errors = error.message_dict
+
+
+class MessageMoveError(JsonableError):
+    code = ErrorCode.MOVE_MESSAGES_TIME_LIMIT_EXCEEDED
+    data_fields = [
+        "first_message_id_allowed_to_move",
+        "total_messages_in_topic",
+        "total_messages_allowed_to_move",
+    ]
+
+    def __init__(
+        self,
+        first_message_id_allowed_to_move: int,
+        total_messages_in_topic: int,
+        total_messages_allowed_to_move: int,
+    ) -> None:
+        self.first_message_id_allowed_to_move = first_message_id_allowed_to_move
+        self.total_messages_in_topic = total_messages_in_topic
+        self.total_messages_allowed_to_move = total_messages_allowed_to_move
+
+    @staticmethod
+    def msg_format() -> str:
+        return _(
+            "You only have permission to move the {total_messages_allowed_to_move}/{total_messages_in_topic} most recent messages in this topic."
+        )

zerver/openapi/zulip.yaml

@@ -6525,6 +6525,12 @@ paths:
         permissions. Now topic editing restrictions apply to messages without a
         topic as well.
 
+        Before Zulip 7.0 (feature level 172), users were allowed to move messages
+        that had passed the time limit by using `change_all` value for `propagate_mode`
+        parameter. Now, the server returns an error if users, other than organization
+        administrators and moderators, try to move messages that have passed the time
+        limit.
+
         [config-message-editing]: /help/restrict-message-editing-and-deletion
       x-curl-examples-parameters:
         oneOf:
@@ -6627,25 +6633,79 @@ paths:
           content:
             application/json:
               schema:
-                allOf:
-                  - $ref: "#/components/schemas/CodedError"
-                  - properties:
-                      msg:
-                        enum:
-                          - Your organization has turned off message editing
-                          - You don't have permission to edit this message
-                          - The time limit for editing this message has past
-                          - Nothing to change
-                          - Topic can't be empty
-                    example:
-                      {
-                        "code": "BAD_REQUEST",
-                        "msg": "You don't have permission to edit this message",
-                        "result": "error",
-                      }
-                    description: |
-                      A typical JSON response for when one doesn't have the permission to
-                      edit a particular message:
+                oneOf:
+                  - allOf:
+                      - $ref: "#/components/schemas/CodedError"
+                      - properties:
+                          msg:
+                            enum:
+                              - Your organization has turned off message editing
+                              - You don't have permission to edit this message
+                              - The time limit for editing this message has past
+                              - Nothing to change
+                              - Topic can't be empty
+                        example:
+                          {
+                            "code": "BAD_REQUEST",
+                            "msg": "You don't have permission to edit this message",
+                            "result": "error",
+                          }
+                        description: |
+                          A typical JSON response for when one doesn't have the permission to
+                          edit a particular message:
+                  - allOf:
+                      - $ref: "#/components/schemas/CodedError"
+                      - example:
+                          {
+                            "code": "MOVE_MESSAGES_TIME_LIMIT_EXCEEDED",
+                            "first_message_id_allowed_to_move": 123,
+                            "msg": "You only have permission to move the 2/5 most recent messages in this topic.",
+                            "result": "error",
+                            "total_messages_allowed_to_move": 2,
+                            "total_messages_in_topic": 5,
+                          }
+                        description: |
+                          A special failed JSON response for when the user has permission to move
+                          the target message, but asked to move all messages in a topic
+                          (`change_all`) and the user does not have permission to move the entire
+                          topic.
+
+                          This happens when the topic contains some messages that are older than an
+                          applicable time limit for the requested topic move
+                          (`move_messages_within_stream_limit_seconds` and/or
+                          `move_messages_between_streams_limit_seconds`).
+
+                          This response contains data on which portion of this topic the user has
+                          permission to move; `first_message_id_allowed_to_move` is the oldest
+                          message ID in this topic that the user has permission to move; the user
+                          has permission to move the most recent `total_messages_allowed_to_move`
+                          messages, but `total_messages_in_topic` exist in the topic.
+
+                          A client is recommended to either just present the error to the user, or
+                          if `first_message_id_allowed_to_move` is not `null`, prompt the user with
+                          this information. Clients should support this error code with
+                          `first_message_id_allowed_to_move=null` for forwards compatibility
+                          with future server versions that may use this error code in other
+                          propagation modes where it is possible that the user does not have
+                          permission to move any messages, or where the server did not calculate the
+                          other fields.
+
+                          If clients choose to present a prompt for this error code, they should
+                          recommend the option of cancelling and (manually) asking a moderator to
+                          move the entire topic, since that's often a better experience than
+                          partially moving a topic. This API supports a client that wishes to allow
+                          the user to repeat the request with a `change_later` propagation mode and
+                          a target message ID of `first_message_id_allowed_to_move`, if the user
+                          desires to move only the portion of the topic that they can.
+
+                          Note that in a stream with protected history, the Zulip security model
+                          requires that the above calculations only include messages the acting user
+                          has access to. So in the rare case of a user attempting to move a topic
+                          that started before the user joined a private stream with protected
+                          history, this API endpoint might move only portion of a topic that they
+                          have access to, without this error or any confirmation dialog.
+
+                          **Changes**: New in Zulip 7.0 (feature level 172).
     delete:
       operationId: delete-message
       summary: Delete a message

zerver/tests/test_message_edit.py

@@ -1354,9 +1354,9 @@ class EditMessageTest(EditMessageTestCase):
             users_to_be_notified_via_muted_topics_event.append(user_topic.user_profile_id)
 
         change_all_topic_name = "Topic 1 edited"
-        # This code path adds 17 (10 + 4/visibility_policy + 1/user + 1) to
+        # This code path adds 19 (12 + 4/visibility_policy + 1/user + 1) to
         # the number of database queries for moving a topic.
-        with self.assert_database_query_count(17):
+        with self.assert_database_query_count(19):
             check_update_message(
                 user_profile=hamlet,
                 message_id=message_id,
@@ -1647,7 +1647,7 @@ class EditMessageTest(EditMessageTestCase):
             users_to_be_notified_via_muted_topics_event.append(user_topic.user_profile_id)
 
         change_all_topic_name = "Topic 1 edited"
-        with self.assert_database_query_count(22):
+        with self.assert_database_query_count(24):
             check_update_message(
                 user_profile=hamlet,
                 message_id=message_id,
@@ -1966,6 +1966,215 @@ class EditMessageTest(EditMessageTestCase):
         self.check_topic(id3, topic_name="topiC1")
         self.check_topic(id4, topic_name="edited")
 
+    def test_change_all_propagate_mode_for_moving_old_messages(self) -> None:
+        user_profile = self.example_user("hamlet")
+        id1 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1")
+        id2 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1")
+        id3 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1")
+        id4 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1")
+        self.send_stream_message(user_profile, "Denmark", topic_name="topic1")
+
+        do_set_realm_property(
+            user_profile.realm,
+            "move_messages_between_streams_policy",
+            Realm.POLICY_MEMBERS_ONLY,
+            acting_user=None,
+        )
+
+        message = Message.objects.get(id=id1)
+        message.date_sent = message.date_sent - datetime.timedelta(days=10)
+        message.save()
+
+        message = Message.objects.get(id=id2)
+        message.date_sent = message.date_sent - datetime.timedelta(days=8)
+        message.save()
+
+        message = Message.objects.get(id=id3)
+        message.date_sent = message.date_sent - datetime.timedelta(days=5)
+        message.save()
+
+        verona = get_stream("Verona", user_profile.realm)
+        denmark = get_stream("Denmark", user_profile.realm)
+        old_topic = "topic1"
+        old_stream = denmark
+
+        def test_moving_all_topic_messages(
+            new_topic: Optional[str] = None, new_stream: Optional[Stream] = None
+        ) -> None:
+            self.login("hamlet")
+            params_dict: Dict[str, Union[str, int]] = {
+                "propagate_mode": "change_all",
+                "send_notification_to_new_thread": "false",
+            }
+
+            if new_topic is not None:
+                params_dict["topic"] = new_topic
+            else:
+                new_topic = old_topic
+
+            if new_stream is not None:
+                params_dict["stream_id"] = new_stream.id
+            else:
+                new_stream = old_stream
+
+            result = self.client_patch(
+                f"/json/messages/{id4}",
+                params_dict,
+            )
+            self.assert_json_error(
+                result,
+                "You only have permission to move the 3/5 most recent messages in this topic.",
+            )
+            # Check message count in old topic and/or stream.
+            messages = get_topic_messages(user_profile, old_stream, old_topic)
+            self.assert_length(messages, 5)
+
+            # Check message count in new topic and/or stream.
+            messages = get_topic_messages(user_profile, new_stream, new_topic)
+            self.assert_length(messages, 0)
+
+            json = orjson.loads(result.content)
+            first_message_id_allowed_to_move = json["first_message_id_allowed_to_move"]
+
+            params_dict["propagate_mode"] = "change_later"
+            result = self.client_patch(
+                f"/json/messages/{first_message_id_allowed_to_move}",
+                params_dict,
+            )
+            self.assert_json_success(result)
+
+            # Check message count in old topic and/or stream.
+            messages = get_topic_messages(user_profile, old_stream, old_topic)
+            self.assert_length(messages, 2)
+
+            # Check message count in new topic and/or stream.
+            messages = get_topic_messages(user_profile, new_stream, new_topic)
+            self.assert_length(messages, 3)
+
+            self.login("shiva")
+            # Move these messages to the original topic and stream, to test the case
+            # when user is moderator.
+            result = self.client_patch(
+                f"/json/messages/{id4}",
+                {
+                    "topic": old_topic,
+                    "stream_id": old_stream.id,
+                    "propagate_mode": "change_all",
+                    "send_notification_to_new_thread": "false",
+                },
+            )
+
+            params_dict["propagate_mode"] = "change_all"
+            result = self.client_patch(
+                f"/json/messages/{id4}",
+                params_dict,
+            )
+            self.assert_json_success(result)
+
+            # Check message count in old topic and/or stream.
+            messages = get_topic_messages(user_profile, old_stream, old_topic)
+            self.assert_length(messages, 0)
+
+            # Check message count in new topic and/or stream.
+            messages = get_topic_messages(user_profile, new_stream, new_topic)
+            self.assert_length(messages, 5)
+
+        # Test only topic editing case.
+        test_moving_all_topic_messages(new_topic="topic edited")
+
+        # Move these messages to the original topic to test the next case.
+        self.client_patch(
+            f"/json/messages/{id4}",
+            {
+                "topic": old_topic,
+                "propagate_mode": "change_all",
+                "send_notification_to_new_thread": "false",
+            },
+        )
+
+        # Test only stream editing case
+        test_moving_all_topic_messages(new_stream=verona)
+
+        # Move these messages to the original stream to test the next case.
+        self.client_patch(
+            f"/json/messages/{id4}",
+            {
+                "stream_id": denmark.id,
+                "propagate_mode": "change_all",
+                "send_notification_to_new_thread": "false",
+            },
+        )
+
+        # Set time limit for moving messages between streams to 2 weeks.
+        do_set_realm_property(
+            user_profile.realm,
+            "move_messages_between_streams_limit_seconds",
+            604800 * 2,
+            acting_user=None,
+        )
+
+        # Test editing both topic and stream together.
+        test_moving_all_topic_messages(new_topic="edited", new_stream=verona)
+
+    def test_change_all_propagate_mode_for_moving_from_stream_with_restricted_history(self) -> None:
+        self.make_stream("privatestream", invite_only=True, history_public_to_subscribers=False)
+        iago = self.example_user("iago")
+        cordelia = self.example_user("cordelia")
+        self.subscribe(iago, "privatestream")
+        self.subscribe(cordelia, "privatestream")
+        id1 = self.send_stream_message(iago, "privatestream", topic_name="topic1")
+        id2 = self.send_stream_message(iago, "privatestream", topic_name="topic1")
+
+        hamlet = self.example_user("hamlet")
+        self.subscribe(hamlet, "privatestream")
+        id3 = self.send_stream_message(iago, "privatestream", topic_name="topic1")
+        id4 = self.send_stream_message(hamlet, "privatestream", topic_name="topic1")
+        self.send_stream_message(hamlet, "privatestream", topic_name="topic1")
+
+        message = Message.objects.get(id=id1)
+        message.date_sent = message.date_sent - datetime.timedelta(days=10)
+        message.save()
+
+        message = Message.objects.get(id=id2)
+        message.date_sent = message.date_sent - datetime.timedelta(days=9)
+        message.save()
+
+        message = Message.objects.get(id=id3)
+        message.date_sent = message.date_sent - datetime.timedelta(days=8)
+        message.save()
+
+        message = Message.objects.get(id=id4)
+        message.date_sent = message.date_sent - datetime.timedelta(days=6)
+        message.save()
+
+        self.login("hamlet")
+        result = self.client_patch(
+            f"/json/messages/{id4}",
+            {
+                "topic": "edited",
+                "propagate_mode": "change_all",
+                "send_notification_to_new_thread": "false",
+            },
+        )
+        self.assert_json_error(
+            result,
+            "You only have permission to move the 2/3 most recent messages in this topic.",
+        )
+
+        self.login("cordelia")
+        result = self.client_patch(
+            f"/json/messages/{id4}",
+            {
+                "topic": "edited",
+                "propagate_mode": "change_all",
+                "send_notification_to_new_thread": "false",
+            },
+        )
+        self.assert_json_error(
+            result,
+            "You only have permission to move the 2/5 most recent messages in this topic.",
+        )
+
     def test_move_message_to_stream(self) -> None:
         (user_profile, old_stream, new_stream, msg_id, msg_id_lt) = self.prepare_move_topics(
             "iago",