From b0a20d2cae3e5701fa42e76399ff173b5989c2c7 Mon Sep 17 00:00:00 2001 From: joseph Date: Sun, 28 Jul 2024 01:21:33 +0000 Subject: [PATCH] attachments: Return a list of removed attachments while editing. Currently, we want to ask users if they would like to delete their attachments after they have removed the attachments while editing. These changes are preparatory changes on the backend to return a list of removed attachments after the user has removed attachments while editing. Fixes part of #25525. --- api_docs/changelog.md | 6 + version.py | 2 +- zerver/actions/message_edit.py | 27 +++-- zerver/actions/scheduled_messages.py | 3 +- zerver/actions/uploads.py | 19 ++- zerver/lib/test_classes.py | 10 ++ zerver/openapi/zulip.yaml | 56 ++++++++- zerver/tests/test_message_edit.py | 167 ++++++++++++++++++++++++++- zerver/views/message_edit.py | 6 +- 9 files changed, 276 insertions(+), 20 deletions(-) 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: