From e72191a56ffdac96ba4bbf048926a59df57a4de8 Mon Sep 17 00:00:00 2001 From: anandbaburajan Date: Sat, 1 Jul 2023 00:45:35 +0530 Subject: [PATCH 1/4] fix: attach unattached guest files on doc update --- frappe/core/doctype/file/utils.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/frappe/core/doctype/file/utils.py b/frappe/core/doctype/file/utils.py index 1d0d145303..5d8c4503bb 100644 --- a/frappe/core/doctype/file/utils.py +++ b/frappe/core/doctype/file/utils.py @@ -320,6 +320,28 @@ def attach_files_to_document(doc: "File", event) -> None: ): return + unattached_file = frappe.db.exists( + "File", + { + "file_url": value, + "attached_to_name": None, + "attached_to_doctype": None, + "attached_to_field": None, + }, + ) + + if unattached_file: + frappe.db.set_value( + "File", + unattached_file, + field={ + "attached_to_name": doc.name, + "attached_to_doctype": doc.doctype, + "attached_to_field": df.fieldname, + }, + ) + return + file: "File" = frappe.get_doc( doctype="File", file_url=value, From d4c30c29f693f4df7ce7bdf1768e261129b3d2d3 Mon Sep 17 00:00:00 2001 From: anandbaburajan Date: Sat, 1 Jul 2023 00:50:59 +0530 Subject: [PATCH 2/4] fix: don't show guest's private files to other guests --- frappe/core/doctype/file/file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 8778826b56..359425d58c 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -724,7 +724,7 @@ def has_permission(doc, ptype=None, user=None): if ptype == "create": return frappe.has_permission("File", "create", user=user) - if not doc.is_private or doc.owner == user or user == "Administrator": + if not doc.is_private or (user != "Guest" and doc.owner == user) or user == "Administrator": return True if doc.attached_to_doctype and doc.attached_to_name: From d36765457d56705bdf7bacf070f8578092920987 Mon Sep 17 00:00:00 2001 From: anandbaburajan Date: Sat, 1 Jul 2023 19:41:22 +0530 Subject: [PATCH 3/4] test: add tests for guest files --- frappe/core/doctype/file/test_file.py | 78 +++++++++++++++++++++++++++ frappe/core/doctype/file/utils.py | 2 +- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 1e7e698062..e442a12b6d 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -785,3 +785,81 @@ class TestFileOptimization(FrappeTestCase): file_content = f.read() self.assertEqual(get_extension("", None, file_content), "jpg") + + +class TestGuestFileAndAttachments(FrappeTestCase): + def setUp(self) -> None: + frappe.db.delete("File", {"is_folder": 0}) + frappe.get_doc( + doctype="DocType", + name="Test For Attachment", + module="Custom", + custom=1, + fields=[ + {"label": "Title", "fieldname": "title", "fieldtype": "Data"}, + {"label": "Attachment", "fieldname": "attachment", "fieldtype": "Attach"}, + ], + ).insert(ignore_if_duplicate=True) + + def tearDown(self) -> None: + frappe.set_user("Administrator") + frappe.db.rollback() + frappe.delete_doc("DocType", "Test For Attachment") + + def test_attach_unattached_guest_file(self): + frappe.set_user("Guest") + + f = frappe.new_doc( + "File", + file_name="test_private_guest_attachment.txt", + content="Guest Home", + is_private=1, + ).insert() + + d = frappe.new_doc("Test For Attachment") + d.title = "Test for attachment on update" + d.attachment = f.file_url + d.assigned_by = frappe.session.user + d.save() + + self.assertTrue( + frappe.db.exists( + "File", + { + "file_name": "test_private_guest_attachment.txt", + "file_url": f.file_url, + "attached_to_doctype": "Test For Attachment", + "attached_to_name": d.name, + "attached_to_field": "attachment", + }, + ) + ) + + def test_list_private_guest_single_file(self): + """Ensure that guests are not able to list private standalone guest files.""" + frappe.set_user("Guest") + frappe.new_doc( + "File", + file_name="test_private_guest_single_txt", + content="Private single File", + is_private=1, + ).insert() + + files = [file.file_name for file in get_files_in_folder("Home")["files"]] + self.assertNotIn("test_private_guest_single_txt.txt", files) + + def test_list_private_guest_attachment(self): + """Ensure that guests are not able to list private guest attachments.""" + frappe.set_user("Guest") + self.attached_to_doctype, self.attached_to_docname = make_test_doc() + frappe.new_doc( + "File", + file_name="test_private_guest_attachment.txt", + attached_to_doctype=self.attached_to_doctype, + attached_to_name=self.attached_to_docname, + content="Private Attachment", + is_private=1, + ).insert() + + files = [file.file_name for file in get_files_in_folder("Home/Attachments")["files"]] + self.assertNotIn("test_private_guest_attachment.txt", files) diff --git a/frappe/core/doctype/file/utils.py b/frappe/core/doctype/file/utils.py index 5d8c4503bb..f1637c1906 100644 --- a/frappe/core/doctype/file/utils.py +++ b/frappe/core/doctype/file/utils.py @@ -294,7 +294,7 @@ def update_existing_file_docs(doc: "File") -> None: ).run() -def attach_files_to_document(doc: "File", event) -> None: +def attach_files_to_document(doc: "Document", event) -> None: """Runs on on_update hook of all documents. Goes through every Attach and Attach Image field and attaches the file url to the document if it is not already attached. From 2481b7035e8fe5c2d2885aeb82d53f65ed323694 Mon Sep 17 00:00:00 2001 From: anandbaburajan Date: Sat, 1 Jul 2023 22:32:34 +0530 Subject: [PATCH 4/4] chore: fix permission issues in tests and update docstring of attach_files_to_document --- frappe/core/doctype/file/test_file.py | 32 +++++++++++++-------------- frappe/core/doctype/file/utils.py | 5 +++-- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index e442a12b6d..ef3a7a5f2b 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -29,11 +29,11 @@ test_content1 = "Hello" test_content2 = "Hello World" -def make_test_doc(): +def make_test_doc(ignore_permissions=False): d = frappe.new_doc("ToDo") d.description = "Test" d.assigned_by = frappe.session.user - d.save() + d.save(ignore_permissions) return d.doctype, d.name @@ -807,14 +807,13 @@ class TestGuestFileAndAttachments(FrappeTestCase): frappe.delete_doc("DocType", "Test For Attachment") def test_attach_unattached_guest_file(self): - frappe.set_user("Guest") - + """Ensure that unattached files are attached on doc update.""" f = frappe.new_doc( "File", file_name="test_private_guest_attachment.txt", content="Guest Home", is_private=1, - ).insert() + ).insert(ignore_permissions=True) d = frappe.new_doc("Test For Attachment") d.title = "Test for attachment on update" @@ -836,30 +835,31 @@ class TestGuestFileAndAttachments(FrappeTestCase): ) def test_list_private_guest_single_file(self): - """Ensure that guests are not able to list private standalone guest files.""" + """Ensure that guests are not able to read private standalone guest files.""" frappe.set_user("Guest") - frappe.new_doc( + + file = frappe.new_doc( "File", file_name="test_private_guest_single_txt", content="Private single File", is_private=1, - ).insert() + ).insert(ignore_permissions=True) - files = [file.file_name for file in get_files_in_folder("Home")["files"]] - self.assertNotIn("test_private_guest_single_txt.txt", files) + self.assertFalse(file.is_downloadable()) def test_list_private_guest_attachment(self): - """Ensure that guests are not able to list private guest attachments.""" + """Ensure that guests are not able to read private guest attachments.""" frappe.set_user("Guest") - self.attached_to_doctype, self.attached_to_docname = make_test_doc() - frappe.new_doc( + + self.attached_to_doctype, self.attached_to_docname = make_test_doc(ignore_permissions=True) + + file = frappe.new_doc( "File", file_name="test_private_guest_attachment.txt", attached_to_doctype=self.attached_to_doctype, attached_to_name=self.attached_to_docname, content="Private Attachment", is_private=1, - ).insert() + ).insert(ignore_permissions=True) - files = [file.file_name for file in get_files_in_folder("Home/Attachments")["files"]] - self.assertNotIn("test_private_guest_attachment.txt", files) + self.assertFalse(file.is_downloadable()) diff --git a/frappe/core/doctype/file/utils.py b/frappe/core/doctype/file/utils.py index f1637c1906..932fce9a03 100644 --- a/frappe/core/doctype/file/utils.py +++ b/frappe/core/doctype/file/utils.py @@ -296,8 +296,9 @@ def update_existing_file_docs(doc: "File") -> None: def attach_files_to_document(doc: "Document", event) -> None: """Runs on on_update hook of all documents. - Goes through every Attach and Attach Image field and attaches - the file url to the document if it is not already attached. + Goes through every file linked with the Attach and Attach Image field and attaches + the file to the document if not already attached. If no file is found, a new file + is created. """ attach_fields = doc.meta.get("fields", {"fieldtype": ["in", ["Attach", "Attach Image"]]})