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

@@ -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),

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

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

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.

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)

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

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:

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)

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: