From 2ef5e6bd1d2bba43de3c2b9ecb02838db88cd596 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Tue, 2 May 2023 17:40:09 +0200 Subject: [PATCH 1/3] fix: file permissions --- frappe/core/doctype/file/file.json | 8 +----- frappe/core/doctype/file/file.py | 44 +++++++++++++++--------------- frappe/hooks.py | 1 + 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/frappe/core/doctype/file/file.json b/frappe/core/doctype/file/file.json index d6c4a99bc3..6c64bfe274 100644 --- a/frappe/core/doctype/file/file.json +++ b/frappe/core/doctype/file/file.json @@ -174,7 +174,7 @@ "icon": "fa fa-file", "idx": 1, "links": [], - "modified": "2022-09-13 15:50:15.508251", + "modified": "2023-05-02 15:42:14.274901", "modified_by": "Administrator", "module": "Core", "name": "File", @@ -196,14 +196,8 @@ { "create": 1, "delete": 1, - "email": 1, - "export": 1, - "if_owner": 1, - "print": 1, "read": 1, - "report": 1, "role": "All", - "share": 1, "write": 1 } ], diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 1323359030..2e88591f94 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -16,6 +16,7 @@ import frappe from frappe import _ from frappe.database.schema import SPECIAL_CHAR_PATTERN from frappe.model.document import Document +from frappe.permissions import get_doctypes_with_read from frappe.utils import call_hook_method, cint, get_files_path, get_hook_method, get_url from frappe.utils.file_manager import is_safe_path from frappe.utils.image import optimize_image, strip_exif_data @@ -703,40 +704,39 @@ def on_doctype_update(): def has_permission(doc, ptype=None, user=None): - has_access = False user = user or frappe.session.user if ptype == "create": - has_access = frappe.has_permission("File", "create", user=user) + return frappe.has_permission("File", "create", user=user) - if not doc.is_private or doc.owner in [user, "Guest"] or user == "Administrator": - has_access = True + if not doc.is_private or doc.owner == user or user == "Administrator": + return True if doc.attached_to_doctype and doc.attached_to_name: attached_to_doctype = doc.attached_to_doctype attached_to_name = doc.attached_to_name - try: - ref_doc = frappe.get_doc(attached_to_doctype, attached_to_name) + ref_doc = frappe.get_doc(attached_to_doctype, attached_to_name) - if ptype in ["write", "create", "delete"]: - has_access = ref_doc.has_permission("write") + if ptype in ["write", "create", "delete"]: + return ref_doc.has_permission("write") + else: + return ref_doc.has_permission("read") - if ptype == "delete" and not has_access: - frappe.throw( - _( - "Cannot delete file as it belongs to {0} {1} for which you do not have permissions" - ).format(doc.attached_to_doctype, doc.attached_to_name), - frappe.PermissionError, - ) - else: - has_access = ref_doc.has_permission("read") - except frappe.DoesNotExistError: - # if parent doc is not created before file is created - # we cannot check its permission so we will use file's permission - pass + return False - return has_access + +def get_permission_query_conditions(user: str = None) -> str: + user = user or frappe.session.user + if user == "Administrator": + return "" + + readable_doctypes = ", ".join(repr(dt) for dt in get_doctypes_with_read()) + return f""" + (`tabFile`.`is_private` = 0) + OR (`tabFile`.`attached_to_doctype` IS NULL AND `tabFile`.`owner` = {user !r}) + OR (`tabFile`.`attached_to_doctype` IN ({readable_doctypes})) + """ # Note: kept at the end to not cause circular, partial imports & maintain backwards compatibility diff --git a/frappe/hooks.py b/frappe/hooks.py index 5967486824..e30b300a58 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -108,6 +108,7 @@ permission_query_conditions = { "Communication": "frappe.core.doctype.communication.communication.get_permission_query_conditions_for_communication", "Workflow Action": "frappe.workflow.doctype.workflow_action.workflow_action.get_permission_query_conditions", "Prepared Report": "frappe.core.doctype.prepared_report.prepared_report.get_permission_query_condition", + "File": "frappe.core.doctype.file.file.get_permission_query_conditions", } has_permission = { From 9c785569f9d4d6cb3d8b03739d6c149f683ae947 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Wed, 3 May 2023 11:28:20 +0200 Subject: [PATCH 2/3] test: file permissions --- frappe/core/doctype/file/test_file.py | 66 +++++++++++++++++++++------ 1 file changed, 53 insertions(+), 13 deletions(-) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 51e065f710..dbab111257 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -601,25 +601,27 @@ class TestAttachmentsAccess(FrappeTestCase): def setUp(self) -> None: frappe.db.delete("File", {"is_folder": 0}) - def test_attachments_access(self): + def test_list_private_attachments(self): frappe.set_user("test4@example.com") self.attached_to_doctype, self.attached_to_docname = make_test_doc() frappe.get_doc( { "doctype": "File", - "file_name": "test_user.txt", + "file_name": "test_user_attachment.txt", "attached_to_doctype": self.attached_to_doctype, "attached_to_name": self.attached_to_docname, "content": "Testing User", + "is_private": 1, } ).insert() frappe.get_doc( { "doctype": "File", - "file_name": "test_user_home.txt", + "file_name": "test_user_standalone.txt", "content": "User Home", + "is_private": 1, } ).insert() @@ -628,18 +630,20 @@ class TestAttachmentsAccess(FrappeTestCase): frappe.get_doc( { "doctype": "File", - "file_name": "test_system_manager.txt", + "file_name": "test_sm_attachment.txt", "attached_to_doctype": self.attached_to_doctype, "attached_to_name": self.attached_to_docname, "content": "Testing System Manager", + "is_private": 1, } ).insert() frappe.get_doc( { "doctype": "File", - "file_name": "test_sm_home.txt", + "file_name": "test_sm_standalone.txt", "content": "System Manager Home", + "is_private": 1, } ).insert() @@ -654,15 +658,51 @@ class TestAttachmentsAccess(FrappeTestCase): file.file_name for file in get_files_in_folder("Home/Attachments")["files"] ] - self.assertIn("test_sm_home.txt", system_manager_files) - self.assertNotIn("test_sm_home.txt", user_files) - self.assertIn("test_user_home.txt", system_manager_files) - self.assertIn("test_user_home.txt", user_files) + self.assertIn("test_sm_standalone.txt", system_manager_files) + self.assertNotIn("test_sm_standalone.txt", user_files) - self.assertIn("test_system_manager.txt", system_manager_attachments_files) - self.assertNotIn("test_system_manager.txt", user_attachments_files) - self.assertIn("test_user.txt", system_manager_attachments_files) - self.assertIn("test_user.txt", user_attachments_files) + self.assertIn("test_user_standalone.txt", user_files) + self.assertNotIn("test_user_standalone.txt", system_manager_files) + + self.assertIn("test_sm_attachment.txt", system_manager_attachments_files) + self.assertIn("test_sm_attachment.txt", user_attachments_files) + self.assertIn("test_user_attachment.txt", system_manager_attachments_files) + self.assertIn("test_user_attachment.txt", user_attachments_files) + + def test_list_public_single_file(self): + """Ensure that users are able to list public standalone files.""" + frappe.set_user("test@example.com") + frappe.get_doc( + { + "doctype": "File", + "file_name": "test_public_single.txt", + "content": "Public single File", + "is_private": 0, + } + ).insert() + + frappe.set_user("test4@example.com") + files = [file.file_name for file in get_files_in_folder("Home")["files"]] + self.assertIn("test_public_single.txt", files) + + def test_list_public_attachment(self): + """Ensure that users are able to list public attachments.""" + frappe.set_user("test@example.com") + self.attached_to_doctype, self.attached_to_docname = make_test_doc() + frappe.get_doc( + { + "doctype": "File", + "file_name": "test_public_attachment.txt", + "attached_to_doctype": self.attached_to_doctype, + "attached_to_name": self.attached_to_docname, + "content": "Public Attachment", + "is_private": 0, + } + ).insert() + + frappe.set_user("test4@example.com") + files = [file.file_name for file in get_files_in_folder("Home/Attachments")["files"]] + self.assertIn("test_public_attachment.txt", files) def tearDown(self) -> None: frappe.set_user("Administrator") From 4b046f0b1116830ba2326ba36fea06465782c6fb Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Mon, 26 Jun 2023 15:53:15 +0200 Subject: [PATCH 3/3] refactor: use new_doc instead of get_doc --- frappe/core/doctype/file/test_file.py | 84 ++++++++++++--------------- 1 file changed, 36 insertions(+), 48 deletions(-) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index b07e344dc0..1e7e698062 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -615,46 +615,38 @@ class TestAttachmentsAccess(FrappeTestCase): frappe.set_user("test4@example.com") self.attached_to_doctype, self.attached_to_docname = make_test_doc() - frappe.get_doc( - { - "doctype": "File", - "file_name": "test_user_attachment.txt", - "attached_to_doctype": self.attached_to_doctype, - "attached_to_name": self.attached_to_docname, - "content": "Testing User", - "is_private": 1, - } + frappe.new_doc( + "File", + file_name="test_user_attachment.txt", + attached_to_doctype=self.attached_to_doctype, + attached_to_name=self.attached_to_docname, + content="Testing User", + is_private=1, ).insert() - frappe.get_doc( - { - "doctype": "File", - "file_name": "test_user_standalone.txt", - "content": "User Home", - "is_private": 1, - } + frappe.new_doc( + "File", + file_name="test_user_standalone.txt", + content="User Home", + is_private=1, ).insert() frappe.set_user("test@example.com") - frappe.get_doc( - { - "doctype": "File", - "file_name": "test_sm_attachment.txt", - "attached_to_doctype": self.attached_to_doctype, - "attached_to_name": self.attached_to_docname, - "content": "Testing System Manager", - "is_private": 1, - } + frappe.new_doc( + "File", + file_name="test_sm_attachment.txt", + attached_to_doctype=self.attached_to_doctype, + attached_to_name=self.attached_to_docname, + content="Testing System Manager", + is_private=1, ).insert() - frappe.get_doc( - { - "doctype": "File", - "file_name": "test_sm_standalone.txt", - "content": "System Manager Home", - "is_private": 1, - } + frappe.new_doc( + "File", + file_name="test_sm_standalone.txt", + content="System Manager Home", + is_private=1, ).insert() system_manager_files = [file.file_name for file in get_files_in_folder("Home")["files"]] @@ -682,13 +674,11 @@ class TestAttachmentsAccess(FrappeTestCase): def test_list_public_single_file(self): """Ensure that users are able to list public standalone files.""" frappe.set_user("test@example.com") - frappe.get_doc( - { - "doctype": "File", - "file_name": "test_public_single.txt", - "content": "Public single File", - "is_private": 0, - } + frappe.new_doc( + "File", + file_name="test_public_single.txt", + content="Public single File", + is_private=0, ).insert() frappe.set_user("test4@example.com") @@ -699,15 +689,13 @@ class TestAttachmentsAccess(FrappeTestCase): """Ensure that users are able to list public attachments.""" frappe.set_user("test@example.com") self.attached_to_doctype, self.attached_to_docname = make_test_doc() - frappe.get_doc( - { - "doctype": "File", - "file_name": "test_public_attachment.txt", - "attached_to_doctype": self.attached_to_doctype, - "attached_to_name": self.attached_to_docname, - "content": "Public Attachment", - "is_private": 0, - } + frappe.new_doc( + "File", + file_name="test_public_attachment.txt", + attached_to_doctype=self.attached_to_doctype, + attached_to_name=self.attached_to_docname, + content="Public Attachment", + is_private=0, ).insert() frappe.set_user("test4@example.com")