Merge pull request #12791 from deepeshgarg007/if_owner_per_check_url

fix: Apply only if creator perm irrespective of user

frappe/model/db_query.py

@@ -589,7 +589,7 @@ class DatabaseQuery(object):
 
 		else:
 			#if has if_owner permission skip user perm check
-			if role_permissions.get("if_owner", {}).get("read"):
+			if role_permissions.get("has_if_owner_enabled") and role_permissions.get("if_owner", {}):
 				self.match_conditions.append("`tab{0}`.`owner` = {1}".format(self.doctype,
 					frappe.db.escape(self.user, percent=False)))
 			# add user permission only if role has read perm

frappe/permissions.py

@@ -108,11 +108,18 @@ def get_doc_permissions(doc, user=None, ptype=None):
 
 	meta = frappe.get_meta(doc.doctype)
 
+	def is_user_owner():
+		doc_owner = doc.get('owner') or ''
+		doc_owner = doc_owner.lower()
+		session_user = frappe.session.user.lower()
+		return doc_owner == session_user
+
+
 	if has_controller_permissions(doc, ptype, user=user) == False :
 		push_perm_check_log('Not allowed via controller permission check')
 		return {ptype: 0}
 
-	permissions = copy.deepcopy(get_role_permissions(meta, user=user))
+	permissions = copy.deepcopy(get_role_permissions(meta, user=user, is_owner=is_user_owner()))
 
 	if not cint(meta.is_submittable):
 		permissions["submit"] = 0
@@ -120,13 +127,8 @@ def get_doc_permissions(doc, user=None, ptype=None):
 	if not cint(meta.allow_import):
 		permissions["import"] = 0
 
-	def is_user_owner():
-		doc_owner = doc.get('owner') or ''
-		doc_owner = doc_owner.lower()
-		session_user = frappe.session.user.lower()
-		return doc_owner == session_user
-
-	if is_user_owner():
+	# Override with `if_owner` perms irrespective of user
+	if permissions.get('has_if_owner_enabled'):
 		# apply owner permissions on top of existing permissions
 		# some access might be only for the owner
 		# eg. everyone might have read access but only owner can delete
@@ -143,7 +145,7 @@ def get_doc_permissions(doc, user=None, ptype=None):
 
 	return permissions
 
-def get_role_permissions(doctype_meta, user=None):
+def get_role_permissions(doctype_meta, user=None, is_owner=None):
 	"""
 	Returns dict of evaluated role permissions like
 		{
@@ -183,6 +185,8 @@ def get_role_permissions(doctype_meta, user=None):
 		applicable_permissions = list(filter(is_perm_applicable, getattr(doctype_meta, 'permissions', [])))
 		has_if_owner_enabled = any(p.get('if_owner', 0) for p in applicable_permissions)
 
+		perms['has_if_owner_enabled'] = has_if_owner_enabled
+
 		for ptype in rights:
 			pvalue = any(p.get(ptype, 0) for p in applicable_permissions)
 			# check if any perm object allows perm type
@@ -191,7 +195,7 @@ def get_role_permissions(doctype_meta, user=None):
 				and has_if_owner_enabled
 				and not has_permission_without_if_owner_enabled(ptype)
 				and ptype != 'create'):
-				perms['if_owner'][ptype] = 1
+				perms['if_owner'][ptype] = cint(pvalue and is_owner)
 				# has no access if not owner
 				# only provide select or read access so that user is able to at-least access list
 				# (and the documents will be filtered based on owner sin further checks)

frappe/tests/test_permissions.py

@@ -13,6 +13,7 @@ from frappe.permissions import (add_user_permission, remove_user_permission,
 from frappe.core.page.permission_manager.permission_manager import update, reset
 from frappe.test_runner import make_test_records_for_doctype
 from frappe.core.doctype.user_permission.user_permission import clear_user_permissions
+from frappe.desk.form.load import getdoc
 
 test_dependencies = ['Blogger', 'Blog Post', "User", "Contact", "Salutation"]
 
@@ -30,6 +31,10 @@ class TestPermissions(unittest.TestCase):
 
 			user = frappe.get_doc("User", "test3@example.com")
 			user.add_roles("Sales User")
+
+			user = frappe.get_doc("User", "testperm@example.com")
+			user.add_roles("Website Manager")
+
 			frappe.flags.permission_user_setup_done = True
 
 		reset('Blogger')
@@ -464,6 +469,74 @@ class TestPermissions(unittest.TestCase):
 		# delete the created doc
 		frappe.delete_doc('Blog Post', '-test-blog-post-title')
 
+	def test_if_owner_permission_on_getdoc(self):
+		update('Blog Post', 'Blogger', 0, 'if_owner', 1)
+		update('Blog Post', 'Blogger', 0, 'read', 1)
+		update('Blog Post', 'Blogger', 0, 'write', 1)
+		update('Blog Post', 'Blogger', 0, 'delete', 1)
+		frappe.clear_cache(doctype="Blog Post")
+
+		frappe.set_user("test1@example.com")
+
+		doc = frappe.get_doc({
+			"doctype": "Blog Post",
+			"blog_category": "-test-blog-category",
+			"blogger": "_Test Blogger 1",
+			"title": "_Test Blog Post Title New",
+			"content": "_Test Blog Post Content"
+		})
+
+		doc.insert()
+
+		getdoc('Blog Post', doc.name)
+		doclist = [d.name for d in frappe.response.docs]
+		self.assertTrue(doc.name in doclist)
+
+		frappe.set_user("test2@example.com")
+		self.assertRaises(frappe.PermissionError, getdoc, 'Blog Post', doc.name)
+
+	def test_if_owner_permission_on_delete(self):
+		update('Blog Post', 'Blogger', 0, 'if_owner', 1)
+		update('Blog Post', 'Blogger', 0, 'read', 1)
+		update('Blog Post', 'Blogger', 0, 'write', 1)
+		update('Blog Post', 'Blogger', 0, 'delete', 1)
+
+		# Remove delete perm
+		update('Blog Post', 'Website Manager', 0, 'delete', 0)
+
+
+		frappe.clear_cache(doctype="Blog Post")
+
+		frappe.set_user("test2@example.com")
+
+		doc = frappe.get_doc({
+			"doctype": "Blog Post",
+			"blog_category": "-test-blog-category",
+			"blogger": "_Test Blogger 1",
+			"title": "_Test Blog Post Title New 1",
+			"content": "_Test Blog Post Content"
+		})
+
+		doc.insert()
+
+		getdoc('Blog Post', doc.name)
+		doclist = [d.name for d in frappe.response.docs]
+		self.assertTrue(doc.name in doclist)
+
+		frappe.set_user("testperm@example.com")
+
+		# Website Manager able to read
+		getdoc('Blog Post', doc.name)
+		doclist = [d.name for d in frappe.response.docs]
+		self.assertTrue(doc.name in doclist)
+
+		# Website Manager should not be able to delete
+		self.assertRaises(frappe.PermissionError, frappe.delete_doc, 'Blog Post', doc.name)
+
+		frappe.set_user("test2@example.com")
+		frappe.delete_doc('Blog Post', '-test-blog-post-title-new-1')
+		update('Blog Post', 'Website Manager', 0, 'delete', 1)
+
 	def test_clear_user_permissions(self):
 		current_user = frappe.session.user
 		frappe.set_user('Administrator')