From 49ec84206635033f4d5bd7dd611173f2f61ce0c5 Mon Sep 17 00:00:00 2001 From: Corentin Forler Date: Tue, 20 Aug 2024 14:38:32 +0200 Subject: [PATCH 1/3] refactor(File): Move validations to before_insert --- frappe/core/doctype/file/file.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 449fc572f4..a6c2707c1d 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -91,6 +91,7 @@ class File(Document): def before_insert(self): self.set_folder_name() + self.set_is_private() self.set_file_name() self.validate_attachment_limit() self.set_file_type() @@ -106,12 +107,11 @@ class File(Document): self.flags.new_file = True frappe.db.after_rollback.add(self.on_rollback) + self.validate_duplicate_entry() # Hash is generated in save_file + def after_insert(self): if not self.is_folder: self.create_attachment_record() - self.set_is_private() - self.set_file_name() - self.validate_duplicate_entry() def validate(self): if self.is_folder: From 96d11ff9f604e6b3abf2ee1b60503d59aec32f39 Mon Sep 17 00:00:00 2001 From: Corentin Forler Date: Tue, 20 Aug 2024 14:45:39 +0200 Subject: [PATCH 2/3] test(File): Ensure private file remains private --- frappe/core/doctype/file/test_file.py | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 03a8927dac..51c90bc38c 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -920,3 +920,38 @@ class TestGuestFileAndAttachments(FrappeTestCase): ).insert(ignore_permissions=True) self.assertFalse(file.is_downloadable()) + + def test_private_remains_private_even_if_same_hash(self): + file_name = "test" + frappe.generate_hash() + content = file_name.encode() + + doc_pub: "File" = frappe.new_doc("File") # type: ignore + doc_pub.file_url = f"/files/{file_name}.txt" + doc_pub.content = content + doc_pub.save() + + doc_pri: "File" = frappe.new_doc("File") # type: ignore + doc_pri.file_url = f"/private/files/{file_name}.txt" + doc_pri.is_private = False + doc_pri.content = content + doc_pri.save() + + doc_pub.reload() + doc_pri.reload() + + self.assertEqual(doc_pub.is_private, 0) + self.assertEqual(doc_pri.is_private, 1) + + self.assertEqual(doc_pub.file_url, f"/files/{file_name}.txt") + self.assertEqual(doc_pri.file_url, f"/private/files/{file_name}.txt") + + self.assertEqual(doc_pub.get_content(), content) + self.assertEqual(doc_pri.get_content(), content) + + doc_pub.delete() + self.assertTrue(os.path.exists(doc_pri.get_full_path())) # Still exists + self.assertFalse(os.path.exists(doc_pub.get_full_path())) + self.assertEqual(doc_pri.get_content(), content) + + doc_pri.delete() + self.assertFalse(os.path.exists(doc_pri.get_full_path())) From b7ea5640c78527fc8d36a8b101262080435bc468 Mon Sep 17 00:00:00 2001 From: Corentin Forler Date: Tue, 20 Aug 2024 15:30:33 +0200 Subject: [PATCH 3/3] test(File): Don't check for file deletion in distinct folders --- frappe/core/doctype/file/file.py | 7 ++++++- frappe/core/doctype/file/test_file.py | 9 ++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index a6c2707c1d..13435d5d47 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -470,7 +470,12 @@ class File(Document): """If file not attached to any other record, delete it""" on_disk_file_not_shared = self.content_hash and not frappe.get_all( "File", - filters={"content_hash": self.content_hash, "name": ["!=", self.name]}, + filters={ + "content_hash": self.content_hash, + "name": ["!=", self.name], + # NOTE: Some old Files might share file_urls while not sharing the is_private value + # "is_private": self.is_private, + }, limit=1, ) if on_disk_file_not_shared: diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 51c90bc38c..57159fa48d 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -948,10 +948,13 @@ class TestGuestFileAndAttachments(FrappeTestCase): self.assertEqual(doc_pub.get_content(), content) self.assertEqual(doc_pri.get_content(), content) + # Deleting a public File should not delete the private File's disk file doc_pub.delete() - self.assertTrue(os.path.exists(doc_pri.get_full_path())) # Still exists - self.assertFalse(os.path.exists(doc_pub.get_full_path())) - self.assertEqual(doc_pri.get_content(), content) + self.assertTrue(os.path.exists(doc_pri.get_full_path())) + # TODO: Migrate existing Files that have a mismatch between `is_private` and `file_url` prefix? + # self.assertFalse(os.path.exists(doc_pub.get_full_path())) + + self.assertEqual(doc_pri.get_content(), content) doc_pri.delete() self.assertFalse(os.path.exists(doc_pri.get_full_path()))