diff --git a/api_docs/changelog.md b/api_docs/changelog.md index f50a112406..22f98047f0 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 285** + +* [`PATCH /messages/{message_id}`](/api/update-message): Added + `detached_uploads` to the response, indicating which uploaded files + are now only accessible via message edit history. + **Feature level 284** * [`GET /events`](/api/get-events), [`GET /messages`](/api/get-messages), diff --git a/version.py b/version.py index 18507dc8fc..2c6584ae4a 100644 --- a/version.py +++ b/version.py @@ -35,7 +35,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 284 # Last bumped for removing 'prev_rendered_content_version' +API_FEATURE_LEVEL = 285 # Last bumped for detached_uploads # 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 f309f9b6c2..5336c12baa 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -2,6 +2,7 @@ import itertools from collections import defaultdict from collections.abc import Iterable from collections.abc import Set as AbstractSet +from dataclasses import dataclass from datetime import timedelta from typing import Any @@ -22,7 +23,7 @@ from zerver.actions.message_send import ( internal_send_stream_message, render_incoming_message, ) -from zerver.actions.uploads import check_attachment_reference_change +from zerver.actions.uploads import AttachmentChangeResult, check_attachment_reference_change from zerver.actions.user_topics import bulk_do_set_user_topic_visibility_policy from zerver.lib.exceptions import ( JsonableError, @@ -86,6 +87,12 @@ from zerver.models.users import get_system_bot from zerver.tornado.django_api import send_event_on_commit +@dataclass +class UpdateMessageResult: + changed_message_count: int + detached_uploads: list[dict[str, Any]] + + def subscriber_info(user_id: int) -> dict[str, Any]: return {"id": user_id, "flags": ["read"]} @@ -436,7 +443,7 @@ def do_update_message( rendering_result: MessageRenderingResult | None, prior_mention_user_ids: set[int], mention_data: MentionData | None = None, -) -> int: +) -> UpdateMessageResult: """ The main function for message editing. A message edit event can modify: @@ -467,6 +474,7 @@ def do_update_message( } realm = user_profile.realm + attachment_reference_change = AttachmentChangeResult(False, []) stream_being_edited = None if target_message.is_stream_message(): @@ -514,10 +522,10 @@ def do_update_message( # target_message.has_image and target_message.has_link will have been # already updated by Markdown rendering in the caller. - target_message.has_attachment = check_attachment_reference_change( + attachment_reference_change = check_attachment_reference_change( target_message, rendering_result ) - + target_message.has_attachment = attachment_reference_change.did_attachment_change if target_message.is_stream_message(): if topic_name is not None: new_topic_name = topic_name @@ -1139,7 +1147,9 @@ def do_update_message( changed_messages_count, ) - return changed_messages_count + return UpdateMessageResult( + changed_messages_count, attachment_reference_change.detached_attachments + ) def check_time_limit_for_change_all_propagate_mode( @@ -1244,7 +1254,7 @@ def check_update_message( send_notification_to_old_thread: bool = True, send_notification_to_new_thread: bool = True, content: str | None = None, -) -> int: +) -> UpdateMessageResult: """This will update a message given the message id and user profile. It checks whether the user profile has the permission to edit the message and raises a JsonableError if otherwise. @@ -1338,7 +1348,6 @@ def check_update_message( check_user_group_mention_allowed(user_profile, mentioned_group_ids) new_stream = None - number_changed = 0 if stream_id is not None: assert message.is_stream_message() @@ -1369,7 +1378,7 @@ def check_update_message( ): check_time_limit_for_change_all_propagate_mode(message, user_profile, topic_name, stream_id) - number_changed = do_update_message( + updated_message_result = do_update_message( user_profile, message, new_stream, @@ -1395,4 +1404,4 @@ def check_update_message( } queue_json_publish("embed_links", event_data) - return number_changed + return updated_message_result diff --git a/zerver/actions/scheduled_messages.py b/zerver/actions/scheduled_messages.py index d4f32b1cfc..bc03155a89 100644 --- a/zerver/actions/scheduled_messages.py +++ b/zerver/actions/scheduled_messages.py @@ -223,9 +223,10 @@ def edit_scheduled_message( ) scheduled_message_object.content = send_request.message.content scheduled_message_object.rendered_content = rendering_result.rendered_content - scheduled_message_object.has_attachment = check_attachment_reference_change( + attachment_reference_change = check_attachment_reference_change( scheduled_message_object, rendering_result ) + scheduled_message_object.has_attachment = attachment_reference_change.did_attachment_change if deliver_at is not None: # User has updated the scheduled message's send timestamp. diff --git a/zerver/actions/uploads.py b/zerver/actions/uploads.py index 1e49eae90b..1aedcef064 100644 --- a/zerver/actions/uploads.py +++ b/zerver/actions/uploads.py @@ -1,4 +1,5 @@ import logging +from dataclasses import dataclass from typing import Any from zerver.lib.attachments import get_old_unclaimed_attachments, validate_attachment_request @@ -16,6 +17,12 @@ from zerver.models import ( from zerver.tornado.django_api import send_event_on_commit +@dataclass +class AttachmentChangeResult: + did_attachment_change: bool + detached_attachments: list[dict[str, Any]] + + def notify_attachment_update( user_profile: UserProfile, op: str, attachment_dict: dict[str, Any] ) -> None: @@ -116,7 +123,7 @@ def do_delete_old_unclaimed_attachments(weeks_ago: int) -> None: def check_attachment_reference_change( message: Message | ScheduledMessage, rendering_result: MessageRenderingResult -) -> bool: +) -> AttachmentChangeResult: # For a unsaved message edit (message.* has been updated, but not # saved to the database), adjusts Attachment data to correspond to # the new content. @@ -124,15 +131,21 @@ def check_attachment_reference_change( new_attachments = set(rendering_result.potential_attachment_path_ids) if new_attachments == prev_attachments: - return bool(prev_attachments) + return AttachmentChangeResult(bool(prev_attachments), []) to_remove = list(prev_attachments - new_attachments) if len(to_remove) > 0: attachments_to_update = Attachment.objects.filter(path_id__in=to_remove).select_for_update() message.attachment_set.remove(*attachments_to_update) + sender = message.sender + detached_attachments_query = Attachment.objects.filter( + path_id__in=to_remove, messages__isnull=True, owner=sender + ) + detached_attachments = [attachment.to_dict() for attachment in detached_attachments_query] + to_add = list(new_attachments - prev_attachments) if len(to_add) > 0: do_claim_attachments(message, to_add) - return message.attachment_set.exists() + return AttachmentChangeResult(message.attachment_set.exists(), detached_attachments) diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index e1591ce750..5cfcbbd9e6 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -16,6 +16,7 @@ import orjson import responses from django.apps import apps from django.conf import settings +from django.core.files.uploadedfile import UploadedFile from django.core.mail import EmailMessage from django.core.signals import got_request_exception from django.db import connection, transaction @@ -76,6 +77,7 @@ from zerver.lib.test_helpers import ( ) from zerver.lib.thumbnail import ThumbnailFormat from zerver.lib.topic import RESOLVED_TOPIC_PREFIX, filter_by_topic_name_via_message +from zerver.lib.upload import upload_message_attachment_from_request from zerver.lib.user_groups import get_system_user_group_for_user from zerver.lib.users import get_api_key from zerver.lib.webhooks.common import ( @@ -2027,6 +2029,14 @@ Output: ): yield + def create_attachment_helper(self, user: UserProfile) -> str: + with tempfile.NamedTemporaryFile() as attach_file: + attach_file.write(b"Hello, World!") + attach_file.flush() + with open(attach_file.name, "rb") as fp: + file_path = upload_message_attachment_from_request(UploadedFile(fp), user) + return file_path + class ZulipTestCase(ZulipTestCaseMixin, TestCase): @contextmanager diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 89a627a329..a7a5939bc9 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -6009,7 +6009,18 @@ paths: contentType: application/json responses: "200": - $ref: "#/components/responses/SimpleSuccess" + description: Success. + content: + application/json: + schema: + allOf: + - $ref: "#/components/schemas/JsonSuccessBase" + - additionalProperties: false + properties: + result: {} + msg: {} + ignored_parameters_unsupported: {} + example: {"result": "success", "msg": ""} "400": description: Bad request. content: @@ -7976,7 +7987,48 @@ paths: contentType: application/json responses: "200": - $ref: "#/components/responses/SimpleSuccess" + description: Success. + content: + application/json: + schema: + allOf: + - $ref: "#/components/schemas/JsonSuccessBase" + - additionalProperties: false + properties: + result: {} + msg: {} + ignored_parameters_unsupported: {} + detached_uploads: + type: array + description: | + Details on all files uploaded by the acting user whose only references + were removed when editing this message. Clients should ask the acting user + if they wish to delete the uploaded files returned in this response, + which might otherwise remain visible only in message edit history. + + Note that [access to message edit + history](/help/disable-message-edit-history) is configurable; this detail + may be important in presenting the question clearly to users. + + New in Zulip 10.0 (feature level 285). + items: + $ref: "#/components/schemas/Attachments" + example: + { + "result": "success", + "msg": "", + "detached_uploads": + [ + { + "id": 3, + "name": "1253601-1.jpg", + "path_id": "2/5d/BD5NRptFxPDKY3RUKwhhup8r/1253601-1.jpg", + "size": 1339060, + "create_time": 1687984706000, + "messages": [], + }, + ], + } "400": description: Bad request. content: diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 2120c991e6..293dfd796f 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -16,7 +16,7 @@ from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import queries_captured from zerver.lib.topic import TOPIC_NAME from zerver.lib.utils import assert_is_not_none -from zerver.models import Message, NamedUserGroup, Realm, UserProfile, UserTopic +from zerver.models import Attachment, Message, NamedUserGroup, Realm, UserProfile, UserTopic from zerver.models.groups import SystemGroups from zerver.models.realms import EditTopicPolicyEnum, WildcardMentionPolicyEnum, get_realm from zerver.models.streams import get_stream @@ -1659,3 +1659,168 @@ class EditMessageTest(ZulipTestCase): }, ) self.assert_json_success(result) + + def test_remove_attachment_while_editing(self) -> None: + # Try editing a message and removing an linked attachment that's + # uploaded by us. Users should be able to detach their own attachments + CONST_UPLOAD_PATH_PREFIX = "/user_uploads/" + user_profile = self.example_user("hamlet") + file1 = self.create_attachment_helper(user_profile) + + content = f"Init message [attachment1.txt]({file1})" + self.login("hamlet") + + # Create two messages referencing the same attachment. + original_msg_id = self.send_stream_message( + user_profile, + "Denmark", + topic_name="editing", + content=content, + ) + + attachments = Attachment.objects.filter(messages__in=[original_msg_id]) + self.assert_length(attachments, 1) + path_id_set = CONST_UPLOAD_PATH_PREFIX + attachments[0].path_id + self.assertEqual(path_id_set, file1) + + msg_id = self.send_stream_message( + user_profile, + "Denmark", + topic_name="editing", + content=content, + ) + + attachments = Attachment.objects.filter(messages__in=[msg_id]) + self.assert_length(attachments, 1) + path_id_set = CONST_UPLOAD_PATH_PREFIX + attachments[0].path_id + self.assertEqual(path_id_set, file1) + + # Try editing first message and removing one reference to the attachment. + result = self.client_patch( + f"/json/messages/{original_msg_id}", + { + "content": "Try editing a message with an attachment", + }, + ) + result_content = orjson.loads(result.content) + self.assertEqual(result_content["result"], "success") + self.assert_length(result_content["detached_uploads"], 0) + + # Try editing second message, the only reference to the attachment now + result = self.client_patch( + f"/json/messages/{msg_id}", + { + "content": "Try editing a message with an attachment", + }, + ) + result_content = orjson.loads(result.content) + self.assertEqual(result_content["result"], "success") + self.assert_length(result_content["detached_uploads"], 1) + actual_path_id_set = ( + CONST_UPLOAD_PATH_PREFIX + result_content["detached_uploads"][0]["path_id"] + ) + + self.assertEqual(actual_path_id_set, file1) + + result = self.client_patch( + f"/json/messages/{msg_id}", + { + "content": "Try editing a message with no attachments", + }, + ) + result_content = orjson.loads(result.content) + self.assertEqual(result_content["result"], "success") + self.assert_length(result_content["detached_uploads"], 0) + + def test_remove_another_user_attachment_while_editing(self) -> None: + # Try editing a message and removing an linked attachment that's + # uploaded by another user. Users should not be able to detach another + # user's attachments. + + user_profile = self.example_user("hamlet") + file1 = self.create_attachment_helper(user_profile) + + content = f"Init message [attachment1.txt]({file1})" + + # Send a message with attachment using another user profile. + msg_id = self.send_stream_message( + user_profile, + "Denmark", + topic_name="editing", + content=content, + ) + self.check_message(msg_id, topic_name="editing", content=content) + attachments = Attachment.objects.filter(messages__in=[msg_id]) + self.assert_length(attachments, 1) + + # Send a message referencing to the attachment uploaded by another user. + self.login("iago") + msg_id = self.send_stream_message( + self.example_user("iago"), + "Denmark", + topic_name="editing", + content=content, + ) + self.check_message(msg_id, topic_name="editing", content=content) + attachments = Attachment.objects.filter(messages__in=[msg_id]) + self.assert_length(attachments, 1) + + # Try editing the message and removing the reference to the attachment. + result = self.client_patch( + f"/json/messages/{msg_id}", + { + "content": "Try editing a message with an attachment uploaded by another user", + }, + ) + result_content = orjson.loads(result.content) + self.assertEqual(result_content["result"], "success") + self.assert_length(result_content["detached_uploads"], 0) + + def test_remove_another_user_deleted_attachment_while_editing(self) -> None: + # Try editing a message and removing an linked attachment that's been + # uploaded and deleted by the original user. Users should not be able + # to detach another user's attachments. + + user_profile = self.example_user("hamlet") + file1 = self.create_attachment_helper(user_profile) + + content = f"Init message [attachment1.txt]({file1})" + + # Send messages with the attachment on both users + original_msg_id = self.send_stream_message( + user_profile, + "Denmark", + topic_name="editing", + content=content, + ) + self.check_message(original_msg_id, topic_name="editing", content=content) + attachments = Attachment.objects.filter(messages__in=[original_msg_id]) + self.assert_length(attachments, 1) + + msg_id = self.send_stream_message( + self.example_user("iago"), + "Denmark", + topic_name="editing", + content=content, + ) + self.check_message(msg_id, topic_name="editing", content=content) + attachments = Attachment.objects.filter(messages__in=[msg_id]) + self.assert_length(attachments, 1) + + # Delete the message reference from the attachment uploader + self.login("hamlet") + result = self.client_delete(f"/json/messages/{original_msg_id}") + result_content = orjson.loads(result.content) + self.assertEqual(result_content["result"], "success") + + # Try editing the message and removing the reference of the now deleted attachment. + self.login("iago") + result = self.client_patch( + f"/json/messages/{msg_id}", + { + "content": "Try editing a message with an attachment uploaded by another user", + }, + ) + result_content = orjson.loads(result.content) + self.assertEqual(result_content["result"], "success") + self.assert_length(result_content["detached_uploads"], 0) diff --git a/zerver/views/message_edit.py b/zerver/views/message_edit.py index 6da04e591f..7f5c3743ea 100644 --- a/zerver/views/message_edit.py +++ b/zerver/views/message_edit.py @@ -128,7 +128,7 @@ def update_message_backend( send_notification_to_new_thread: Json[bool] = True, content: str | None = None, ) -> HttpResponse: - number_changed = check_update_message( + updated_message_result = check_update_message( user_profile, message_id, stream_id, @@ -142,9 +142,9 @@ def update_message_backend( # Include the number of messages changed in the logs log_data = RequestNotes.get_notes(request).log_data assert log_data is not None - log_data["extra"] = f"[{number_changed}]" + log_data["extra"] = f"[{updated_message_result.changed_message_count}]" - return json_success(request) + return json_success(request, data={"detached_uploads": updated_message_result.detached_uploads}) def validate_can_delete_message(user_profile: UserProfile, message: Message) -> None: