Merge pull request #17681 from resilient-tech/fix-child-perm

refactor: improved child table permission check

frappe/__init__.py

@@ -912,15 +912,26 @@ def only_has_select_perm(doctype, user=None, ignore_permissions=False):
 
 
 def has_permission(
-	doctype=None, ptype="read", doc=None, user=None, verbose=False, throw=False, parent_doctype=None
+	doctype=None,
+	ptype="read",
+	doc=None,
+	user=None,
+	verbose=False,
+	throw=False,
+	*,
+	parent_doctype=None,
 ):
-	"""Raises `frappe.PermissionError` if not permitted.
+	"""
+	Returns True if the user has permission `ptype` for given `doctype` or `doc`
+	Raises `frappe.PermissionError` if user isn't permitted and `throw` is truthy
 
 	:param doctype: DocType for which permission is to be check.
 	:param ptype: Permission type (`read`, `write`, `create`, `submit`, `cancel`, `amend`). Default: `read`.
 	:param doc: [optional] Checks User permissions for given doc.
 	:param user: [optional] Check for given user. Default: current user.
-	:param parent_doctype: Required when checking permission for a child DocType (unless doc is specified)."""
+	:param verbose: DEPRECATED, will be removed in a future release.
+	:param parent_doctype: Required when checking permission for a child DocType (unless doc is specified).
+	"""
 	import frappe.permissions
 
 	if not doctype and doc:
@@ -930,7 +941,6 @@ def has_permission(
 		doctype,
 		ptype,
 		doc=doc,
-		verbose=verbose,
 		user=user,
 		raise_exception=throw,
 		parent_doctype=parent_doctype,

frappe/model/db_query.py

@@ -97,6 +97,7 @@ class DatabaseQuery:
 		strict=True,
 		pluck=None,
 		ignore_ddl=False,
+		*,
 		parent_doctype=None,
 	) -> list:
 

frappe/model/document.py

@@ -194,15 +194,19 @@ class Document(BaseDocument):
 			self.raise_no_permission_to(permlevel or permtype)
 
 	def has_permission(self, permtype="read", verbose=False) -> bool:
-		"""Call `frappe.has_permission` if `self.flags.ignore_permissions`
-		is not set.
+		"""
+		Call `frappe.permissions.has_permission` if `ignore_permissions` flag isn't truthy
 
-		:param permtype: one of `read`, `write`, `submit`, `cancel`, `delete`"""
-		import frappe.permissions
+		:param permtype: `read`, `write`, `submit`, `cancel`, `delete`, etc.
+		:param verbose: DEPRECATED, will be removed in a future release.
+		"""
 
 		if self.flags.ignore_permissions:
 			return True
-		return frappe.permissions.has_permission(self.doctype, permtype, self, verbose=verbose)
+
+		import frappe.permissions
+
+		return frappe.permissions.has_permission(self.doctype, permtype, self)
 
 	def raise_no_permission_to(self, perm_type):
 		"""Raise `frappe.PermissionError`."""

frappe/model/meta.py

@@ -539,9 +539,9 @@ class Meta(Document):
 
 		return self.high_permlevel_fields
 
-	def get_permlevel_access(self, permission_type="read", parenttype=None):
+	def get_permlevel_access(self, permission_type="read", parenttype=None, *, user=None):
 		has_access_to = []
-		roles = frappe.get_roles()
+		roles = frappe.get_roles(user)
 		for perm in self.get_permissions(parenttype):
 			if perm.role in roles and perm.get(permission_type):
 				if perm.permlevel not in has_access_to:

frappe/permissions.py

@@ -68,28 +68,39 @@ def has_permission(
 	verbose=False,
 	user=None,
 	raise_exception=True,
+	*,
 	parent_doctype=None,
 ):
 	"""Returns True if user has permission `ptype` for given `doctype`.
 	If `doc` is passed, it also checks user, share and owner permissions.
 
-	Note: if Table DocType is passed, it always returns True.
+	:param doctype: DocType to check permission for
+	:param ptype: Permission Type to check
+	:param doc: Check User Permissions for specified document.
+	:param verbose: DEPRECATED, will be removed in a future release.
+	:param user: User to check permission for. Defaults to current user.
+	:param raise_exception:
+	        DOES NOT raise an exception.
+	        If not False, will display a message using frappe.msgprint
+	                which explains why the permission check failed.
+
+	:param parent_doctype:
+	        Required when checking permission for a child DocType (unless doc is specified)
 	"""
+
 	if not user:
 		user = frappe.session.user
 
+	if user == "Administrator":
+		return True
+
 	if not doc and hasattr(doctype, "doctype"):
 		# first argument can be doc or doctype
 		doc = doctype
 		doctype = doc.doctype
 
-	if user == "Administrator":
-		return True
-
 	if frappe.is_table(doctype):
-		return has_child_table_permission(
-			doctype, ptype, doc, verbose, user, raise_exception, parent_doctype
-		)
+		return has_child_permission(doctype, ptype, doc, user, raise_exception, parent_doctype)
 
 	meta = frappe.get_meta(doctype)
 
@@ -667,52 +678,57 @@ def push_perm_check_log(log):
 	frappe.flags.get("has_permission_check_logs").append(_(log))
 
 
-def has_child_table_permission(
+def has_child_permission(
 	child_doctype,
 	ptype="read",
 	child_doc=None,
-	verbose=False,
 	user=None,
 	raise_exception=True,
 	parent_doctype=None,
 ):
-	parent_doc = None
+	if isinstance(child_doc, str):
+		child_doc = frappe.db.get_value(
+			child_doctype,
+			child_doc,
+			("parent", "parenttype", "parentfield"),
+			as_dict=True,
+		)
 
 	if child_doc:
-		parent_doctype = child_doc.get("parenttype")
-		parent_doc = frappe.get_cached_doc(
-			{"doctype": parent_doctype, "docname": child_doc.get("parent")}
-		)
+		parent_doctype = child_doc.parenttype
 
-	if parent_doctype:
-		if not is_parent_valid(child_doctype, parent_doctype):
-			frappe.throw(
-				_("{0} is not a valid parent DocType for {1}").format(
-					frappe.bold(parent_doctype), frappe.bold(child_doctype)
-				),
-				title=_("Invalid Parent DocType"),
-			)
-	else:
-		frappe.throw(
-			_("Please specify a valid parent DocType for {0}").format(frappe.bold(child_doctype)),
-			title=_("Parent DocType Required"),
+	if not parent_doctype:
+		push_perm_check_log(
+			_("Please specify a valid parent DocType for {0}").format(frappe.bold(child_doctype))
 		)
+		return False
+
+	parent_meta = frappe.get_meta(parent_doctype)
+
+	if parent_meta.istable or all(
+		df.options != child_doctype for df in parent_meta.get_table_fields()
+	):
+		push_perm_check_log(
+			_("{0} is not a valid parent DocType for {1}").format(
+				frappe.bold(parent_doctype), frappe.bold(child_doctype)
+			)
+		)
+		return False
+
+	if (
+		child_doc
+		and (permlevel := parent_meta.get_field(child_doc.parentfield).permlevel) > 0
+		and permlevel not in parent_meta.get_permlevel_access(ptype, user=user)
+	):
+		push_perm_check_log(
+			_("Insufficient Permission Level for {0}").format(frappe.bold(parent_doctype))
+		)
+		return False
 
 	return has_permission(
 		parent_doctype,
 		ptype=ptype,
-		doc=parent_doc,
-		verbose=verbose,
+		doc=child_doc and getattr(child_doc, "parent_doc", child_doc.parent),
 		user=user,
 		raise_exception=raise_exception,
 	)
-
-
-def is_parent_valid(child_doctype, parent_doctype):
-	from frappe.core.utils import find
-
-	parent_meta = frappe.get_meta(parent_doctype)
-	child_table_field_exists = find(
-		parent_meta.get_table_fields(), lambda d: d.options == child_doctype
-	)
-	return not parent_meta.istable and child_table_field_exists

frappe/tests/test_permissions.py

@@ -649,30 +649,31 @@ class TestPermissions(FrappeTestCase):
 		# reset the user
 		frappe.set_user(current_user)
 
-	def test_child_table_permissions(self):
-		frappe.set_user("test@example.com")
-		self.assertIsInstance(frappe.get_list("Has Role", parent_doctype="User", limit=1), list)
-		self.assertRaisesRegex(
-			frappe.exceptions.ValidationError,
-			".* is not a valid parent DocType for .*",
-			frappe.get_list,
-			doctype="Has Role",
-			parent_doctype="ToDo",
-		)
-		self.assertRaisesRegex(
-			frappe.exceptions.ValidationError,
-			"Please specify a valid parent DocType for .*",
-			frappe.get_list,
-			"Has Role",
-		)
-		self.assertRaisesRegex(
-			frappe.exceptions.ValidationError,
-			".* is not a valid parent DocType for .*",
-			frappe.get_list,
-			doctype="Has Role",
-			parent_doctype="Has Role",
+	def test_child_permissions(self):
+		frappe.set_user("test3@example.com")
+		self.assertIsInstance(frappe.get_list("DefaultValue", parent_doctype="User", limit=1), list)
+
+		# frappe.get_list
+		self.assertRaises(frappe.PermissionError, frappe.get_list, "DefaultValue")
+		self.assertRaises(frappe.PermissionError, frappe.get_list, "DefaultValue", parent_doctype="ToDo")
+		self.assertRaises(
+			frappe.PermissionError, frappe.get_list, "DefaultValue", parent_doctype="DefaultValue"
 		)
 
+		# frappe.get_doc
+		user = frappe.get_doc("User", frappe.session.user)
+		doc = user.append("defaults")
+		doc.check_permission()
+
+		# false by permlevel
+		doc = user.append("roles")
+		self.assertRaises(frappe.PermissionError, doc.check_permission)
+
+		# false by user permission
+		user = frappe.get_doc("User", "Administrator")
+		doc = user.append("defaults")
+		self.assertRaises(frappe.PermissionError, doc.check_permission)
+
 	def test_select_user(self):
 		"""If test3@example.com is restricted by a User Permission to see only
 		users linked to a certain doctype (in this case: Gender "Female"), he