diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index dd36143f10..64029e8b6a 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -2703,6 +2703,37 @@ def check_schedule_message( return do_schedule_messages([send_request])[0] +def validate_message_edit_payload( + message: Message, + stream_id: Optional[int], + topic_name: Optional[str], + propagate_mode: Optional[str], + content: Optional[str], +) -> None: + """ + Checks that the data sent is well-formed. Does not handle editability, permissions etc. + """ + if topic_name is None and content is None and stream_id is None: + raise JsonableError(_("Nothing to change")) + + if not message.is_stream_message(): + if stream_id is not None: + raise JsonableError(_("Private messages cannot be moved to streams.")) + + if propagate_mode != "change_one" and topic_name is None and stream_id is None: + raise JsonableError(_("Invalid propagate_mode without topic edit")) + + if topic_name == "": + raise JsonableError(_("Topic can't be empty")) + + if stream_id is not None and content is not None: + raise JsonableError(_("Cannot change message content while changing stream")) + + # Right now, we prevent users from editing widgets. + if content is not None and is_widget_message(message): + raise JsonableError(_("Widgets cannot be edited.")) + + def check_update_message( user_profile: UserProfile, message_id: int, @@ -2718,13 +2749,19 @@ def check_update_message( and raises a JsonableError if otherwise. It returns the number changed. """ + message, ignored_user_message = access_message(user_profile, message_id) + if not user_profile.realm.allow_message_editing: raise JsonableError(_("Your organization has turned off message editing")) - if propagate_mode != "change_one" and topic_name is None and stream_id is None: - raise JsonableError(_("Invalid propagate_mode without topic edit")) + # The zerver/views/message_edit.py callpoint already strips this + # via REQ_topic; so we can delete this line if we arrange a + # contract where future callers in the embedded bots system strip + # use REQ_topic as well (or otherwise are guaranteed to strip input). + if topic_name is not None: + topic_name = topic_name.strip() + validate_message_edit_payload(message, stream_id, topic_name, propagate_mode, content) - message, ignored_user_message = access_message(user_profile, message_id) is_no_topic_msg = message.topic_name() == "(no topic)" # You only have permission to edit a message if: @@ -2744,10 +2781,6 @@ def check_update_message( else: raise JsonableError(_("You don't have permission to edit this message")) - # Right now, we prevent users from editing widgets. - if content is not None and is_widget_message(message): - raise JsonableError(_("Widgets cannot be edited.")) - # If there is a change to the content, check that it hasn't been too long # Allow an extra 20 seconds since we potentially allow editing 15 seconds # past the limit, and in case there are network issues, etc. The 15 comes @@ -2773,12 +2806,6 @@ def check_update_message( if (timezone_now() - message.date_sent) > datetime.timedelta(seconds=deadline_seconds): raise JsonableError(_("The time limit for editing this message has passed")) - if topic_name is None and content is None and stream_id is None: - raise JsonableError(_("Nothing to change")) - if topic_name is not None: - topic_name = topic_name.strip() - if topic_name == "": - raise JsonableError(_("Topic can't be empty")) rendered_content = None links_for_embed: Set[str] = set() prior_mention_user_ids: Set[int] = set() @@ -2815,8 +2842,7 @@ def check_update_message( number_changed = 0 if stream_id is not None: - if not message.is_stream_message(): - raise JsonableError(_("Message must be a stream message")) + assert message.is_stream_message() if not user_profile.can_move_messages_between_streams(): raise JsonableError(_("You don't have permission to move this message")) try: @@ -2827,8 +2853,6 @@ def check_update_message( "You don't have permission to move this message due to missing access to its stream" ) ) - if content is not None: - raise JsonableError(_("Cannot change message content while changing stream")) new_stream = access_stream_by_id(user_profile, stream_id, require_active=True)[0] check_stream_access_based_on_stream_post_policy(user_profile, new_stream) diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 27dd21a759..c931e5c776 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -95,6 +95,117 @@ class EditMessageTestCase(ZulipTestCase): return (user_profile, stream, new_stream, msg_id, msg_id_lt) +class EditMessagePayloadTest(EditMessageTestCase): + def test_edit_message_no_changes(self) -> None: + self.login("hamlet") + msg_id = self.send_stream_message( + self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit" + ) + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + }, + ) + self.assert_json_error(result, "Nothing to change") + + def test_move_message_cant_move_private_message(self) -> None: + hamlet = self.example_user("hamlet") + self.login("hamlet") + cordelia = self.example_user("cordelia") + msg_id = self.send_personal_message(hamlet, cordelia) + + verona = get_stream("Verona", hamlet.realm) + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "stream_id": verona.id, + }, + ) + + self.assert_json_error(result, "Private messages cannot be moved to streams.") + + def test_propagate_invalid(self) -> None: + self.login("hamlet") + id1 = self.send_stream_message(self.example_user("hamlet"), "Scotland", topic_name="topic1") + + result = self.client_patch( + "/json/messages/" + str(id1), + { + "topic": "edited", + "propagate_mode": "invalid", + }, + ) + self.assert_json_error(result, "Invalid propagate_mode") + self.check_topic(id1, topic_name="topic1") + + result = self.client_patch( + "/json/messages/" + str(id1), + { + "content": "edited", + "propagate_mode": "change_all", + }, + ) + self.assert_json_error(result, "Invalid propagate_mode without topic edit") + self.check_topic(id1, topic_name="topic1") + + def test_edit_message_no_topic(self) -> None: + self.login("hamlet") + msg_id = self.send_stream_message( + self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit" + ) + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "topic": " ", + }, + ) + self.assert_json_error(result, "Topic can't be empty") + + def test_move_message_to_stream_with_content(self) -> None: + (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( + "iago", "test move stream", "new stream", "test" + ) + + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "stream_id": new_stream.id, + "propagate_mode": "change_all", + "content": "Not allowed", + }, + ) + self.assert_json_error(result, "Cannot change message content while changing stream") + + messages = get_topic_messages(user_profile, old_stream, "test") + self.assert_length(messages, 3) + + messages = get_topic_messages(user_profile, new_stream, "test") + self.assert_length(messages, 0) + + # Right now, we prevent users from editing widgets. + def test_edit_submessage(self) -> None: + self.login("hamlet") + msg_id = self.send_stream_message( + self.example_user("hamlet"), + "Scotland", + topic_name="editing", + content="/poll Games?\nYES\nNO", + ) + result = self.client_patch( + "/json/messages/" + str(msg_id), + { + "message_id": msg_id, + "content": "/poll Games?\nYES\nNO\nMaybe", + }, + ) + self.assert_json_error(result, "Widgets cannot be edited.") + + class EditMessageTest(EditMessageTestCase): def test_query_count_on_to_dict_uncached(self) -> None: # `to_dict_uncached` method is used by the mechanisms @@ -211,24 +322,6 @@ class EditMessageTest(EditMessageTestCase): result = self.client_get("/json/messages/" + str(msg_id)) self.assert_json_error(result, "Invalid message(s)") - # Right now, we prevent users from editing widgets. - def test_edit_submessage(self) -> None: - self.login("hamlet") - msg_id = self.send_stream_message( - self.example_user("hamlet"), - "Scotland", - topic_name="editing", - content="/poll Games?\nYES\nNO", - ) - result = self.client_patch( - "/json/messages/" + str(msg_id), - { - "message_id": msg_id, - "content": "/poll Games?\nYES\nNO\nMaybe", - }, - ) - self.assert_json_error(result, "Widgets cannot be edited.") - def test_edit_message_no_permission(self) -> None: self.login("hamlet") msg_id = self.send_stream_message( @@ -243,33 +336,6 @@ class EditMessageTest(EditMessageTestCase): ) self.assert_json_error(result, "You don't have permission to edit this message") - def test_edit_message_no_changes(self) -> None: - self.login("hamlet") - msg_id = self.send_stream_message( - self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit" - ) - result = self.client_patch( - "/json/messages/" + str(msg_id), - { - "message_id": msg_id, - }, - ) - self.assert_json_error(result, "Nothing to change") - - def test_edit_message_no_topic(self) -> None: - self.login("hamlet") - msg_id = self.send_stream_message( - self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit" - ) - result = self.client_patch( - "/json/messages/" + str(msg_id), - { - "message_id": msg_id, - "topic": " ", - }, - ) - self.assert_json_error(result, "Topic can't be empty") - def test_edit_message_no_content(self) -> None: self.login("hamlet") msg_id = self.send_stream_message( @@ -1094,30 +1160,6 @@ class EditMessageTest(EditMessageTestCase): self.check_topic(id3, topic_name="topiC1") self.check_topic(id4, topic_name="edited") - def test_propagate_invalid(self) -> None: - self.login("hamlet") - id1 = self.send_stream_message(self.example_user("hamlet"), "Scotland", topic_name="topic1") - - result = self.client_patch( - "/json/messages/" + str(id1), - { - "topic": "edited", - "propagate_mode": "invalid", - }, - ) - self.assert_json_error(result, "Invalid propagate_mode") - self.check_topic(id1, topic_name="topic1") - - result = self.client_patch( - "/json/messages/" + str(id1), - { - "content": "edited", - "propagate_mode": "change_all", - }, - ) - self.assert_json_error(result, "Invalid propagate_mode without topic edit") - self.check_topic(id1, topic_name="topic1") - def test_move_message_to_stream(self) -> None: (user_profile, old_stream, new_stream, msg_id, msg_id_lt) = self.prepare_move_topics( "iago", "test move stream", "new stream", "test" @@ -1270,29 +1312,6 @@ class EditMessageTest(EditMessageTestCase): private_stream.recipient_id, ) - def test_move_message_cant_move_private_message( - self, - ) -> None: - user_profile = self.example_user("iago") - self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR) - self.login("iago") - - hamlet = self.example_user("hamlet") - msg_id = self.send_personal_message(user_profile, hamlet) - - verona = get_stream("Verona", user_profile.realm) - - result = self.client_patch( - "/json/messages/" + str(msg_id), - { - "message_id": msg_id, - "stream_id": verona.id, - "propagate_mode": "change_all", - }, - ) - - self.assert_json_error(result, "Message must be a stream message") - def test_move_message_to_stream_change_later(self) -> None: (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( "iago", "test move stream", "new stream", "test" @@ -1493,28 +1512,6 @@ class EditMessageTest(EditMessageTestCase): ) check_move_message_to_stream(UserProfile.ROLE_MEMBER) - def test_move_message_to_stream_with_content(self) -> None: - (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( - "iago", "test move stream", "new stream", "test" - ) - - result = self.client_patch( - "/json/messages/" + str(msg_id), - { - "message_id": msg_id, - "stream_id": new_stream.id, - "propagate_mode": "change_all", - "content": "Not allowed", - }, - ) - self.assert_json_error(result, "Cannot change message content while changing stream") - - messages = get_topic_messages(user_profile, old_stream, "test") - self.assert_length(messages, 3) - - messages = get_topic_messages(user_profile, new_stream, "test") - self.assert_length(messages, 0) - def test_move_message_to_stream_and_topic(self) -> None: (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( "iago", "test move stream", "new stream", "test"