fix: file permissions (backport #20877) (#21503)

* fix: file permissions (cherry picked from commit 2ef5e6b) * test: file permissions (cherry picked from commit 9c78556) * refactor: use new_doc instead of get_doc (cherry picked from commit 4b046f0) * fix: attach unattached guest files on doc update * fix: don't show guest's private files to other guests * test: add tests for guest files * chore: fix permission issues in tests and update docstring of attach_files_to_document * Revert "refactor: use new_doc instead of get_doc" This reverts commit 876444f. * refactor: use old style get_doc For consistency with the rest of the file --------- Co-authored-by: barredterra <14891507+barredterra@users.noreply.github.com> Co-authored-by: anandbaburajan <anandbaburajan@gmail.com>
frappe · Aug 16, 2023 · f38b6a0 · f38b6a0
1 parent bc764aa
commit f38b6a0
Show file tree

Hide file tree

Showing 5 changed files with 195 additions and 49 deletions.
diff --git 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
@@ -16,6 +16,7 @@
 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
@@ -707,40 +708,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 (user != "Guest" and 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)
-
-			if ptype in ["write", "create", "delete"]:
-				has_access = ref_doc.has_permission("write")
-
-				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
+		ref_doc = frappe.get_doc(attached_to_doctype, attached_to_name)
+
+		if ptype in ["write", "create", "delete"]:
+			return ref_doc.has_permission("write")
+		else:
+			return ref_doc.has_permission("read")
+
+	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/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py
@@ -28,11 +28,11 @@
 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
 
 
@@ -600,25 +600,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()
 
@@ -627,18 +629,20 @@ def test_attachments_access(self):
 		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()
 
@@ -653,15 +657,51 @@ def test_attachments_access(self):
 			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_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()
 
-		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)
+		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")
@@ -739,3 +779,91 @@ def test_revert_optimized_file_on_rollback(self):
 			size_after_rollback = os.stat(image_path).st_size
 
 			self.assertEqual(size_before_optimization, size_after_rollback)
+
+
+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):
+		"""Ensure that unattached files are attached on doc update."""
+		f = frappe.get_doc(
+			{
+				"doctype": "File",
+				"file_name": "test_private_guest_attachment.txt",
+				"content": "Guest Home",
+				"is_private": 1,
+			}
+		).insert(ignore_permissions=True)
+
+		d = frappe.get_doc(
+			{
+				"doctype": "Test For Attachment",
+				"title": "Test for attachment on update",
+				"attachment": f.file_url,
+				"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 read private standalone guest files."""
+		frappe.set_user("Guest")
+
+		file = frappe.get_doc(
+			{
+				"doctype": "File",
+				"file_name": "test_private_guest_single_txt",
+				"content": "Private single File",
+				"is_private": 1,
+			}
+		).insert(ignore_permissions=True)
+
+		self.assertFalse(file.is_downloadable())
+
+	def test_list_private_guest_attachment(self):
+		"""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(ignore_permissions=True)
+
+		file = frappe.get_doc(
+			{
+				"doctype": "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(ignore_permissions=True)
+
+		self.assertFalse(file.is_downloadable())
diff --git a/frappe/core/doctype/file/utils.py b/frappe/core/doctype/file/utils.py
@@ -292,10 +292,11 @@ 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.
+	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"]]})
@@ -318,6 +319,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,

diff --git a/frappe/hooks.py b/frappe/hooks.py
@@ -103,6 +103,7 @@
 	"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 = {