diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 9c30ad7521..68b2d6ed05 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -3272,7 +3272,17 @@ def do_claim_attachments(message): is_message_realm_public = Stream.objects.get(id=message.recipient.type_id).is_public() if not validate_attachment_request(user_profile, path_id): - logging.warning("User %s does not have permission to access upload %s" % (user_profile.id, path_id,)) + # Technically, there are 2 cases here: + # * The user put something in their message that has the form + # of an upload, but doesn't correspond to a file that doesn't + # exist. validate_attachment_request will return None. + # * The user is trying to send a link to a file they don't have permission to + # access themselves. validate_attachment_request will return False. + # + # Either case is unusual and suggests a UI bug that got + # the user in this situation, so we log in these cases. + logging.warning("User %s tried to share upload %s in message %s, but lacks permission" % ( + user_profile.id, path_id, message.id)) continue claim_attachment(user_profile, path_id, message, is_message_realm_public) diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index e9a394a229..b7fb152967 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -282,6 +282,13 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): self.assertTrue(not Attachment.objects.filter(path_id = d2_path_id).exists()) self.assertTrue(not delete_message_image(d2_path_id)) + def test_attachment_url_without_upload(self): + # type: () -> None + self.login("hamlet@zulip.com") + body = "Test message ...[zulip.txt](http://localhost:9991/user_uploads/1/64/fake_path_id.txt)" + self.send_message("hamlet@zulip.com", "Denmark", Recipient.STREAM, body, "test") + self.assertFalse(Attachment.objects.filter(path_id = "1/64/fake_path_id.txt").exists()) + def test_multiple_claim_attachments(self): # type: () -> None """