From bd7f728796efb18f6ac95c0b4c442aea8137fffd Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Fri, 14 Oct 2022 15:48:37 +0530 Subject: [PATCH] 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 | 4 + version.py | 2 +- zerver/actions/message_edit.py | 108 ++++++++++++++- zerver/lib/exceptions.py | 26 ++++ zerver/openapi/zulip.yaml | 98 +++++++++++--- zerver/tests/test_message_edit.py | 215 +++++++++++++++++++++++++++++- 6 files changed, 428 insertions(+), 25 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index c2a8d8ef69..74e39a6c8b 100644 --- a/api_docs/changelog.md +++ b/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**: diff --git a/version.py b/version.py index 3f3e95df58..57368e89b5 100644 --- a/version.py +++ b/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 diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 1f6935a603..64b1c5c070 100644 --- a/zerver/actions/message_edit.py +++ b/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, diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index 7abcdbae03..089d7c5762 100644 --- a/zerver/lib/exceptions.py +++ b/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." + ) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index b2f407b4ae..2ee5076eea 100644 --- a/zerver/openapi/zulip.yaml +++ b/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 diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 4b97fd21df..5a5df40e35 100644 --- a/zerver/tests/test_message_edit.py +++ b/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",