Merge pull request #12374 from shariquerik/todo-access-fix

fix: All todos not accessible to all users

frappe/core/doctype/user/test_records.json

@@ -38,6 +38,13 @@
   "new_password": "Eastern_43A1W",
   "enabled": 1
  },
+ {
+  "doctype": "User",
+  "email": "test4@example.com",
+  "first_name": "_Test4",
+  "new_password": "Eastern_43A1W",
+  "enabled": 1
+ },
  {
   "doctype": "User",
   "email": "testperm@example.com",

frappe/desk/doctype/todo/test_todo.py

@@ -5,8 +5,12 @@ from __future__ import unicode_literals
 
 import frappe
 import unittest
+from frappe.model.db_query import DatabaseQuery
+from frappe.permissions import add_permission, reset_perms
+from frappe.core.doctype.doctype.doctype import clear_permissions_cache
 
 # test_records = frappe.get_test_records('ToDo')
+test_user_records = frappe.get_test_records('User')
 
 class TestToDo(unittest.TestCase):
 	def test_delete(self):
@@ -47,6 +51,62 @@ class TestToDo(unittest.TestCase):
 		self.assertEqual(todo.assigned_by_full_name,
 			frappe.db.get_value('User', todo.assigned_by, 'full_name'))
 
+	def test_todo_list_access(self):
+		create_new_todo('Test1', 'testperm@example.com')
+
+		frappe.set_user('test4@example.com')
+		create_new_todo('Test2', 'test4@example.com')
+		test_user_data = DatabaseQuery('ToDo').execute()
+
+		frappe.set_user('testperm@example.com')
+		system_manager_data = DatabaseQuery('ToDo').execute()
+
+		self.assertNotEqual(test_user_data, system_manager_data)
+
+		frappe.set_user('Administrator')
+		frappe.db.rollback()
+
+	def test_doc_read_access(self):
+		#owner and assigned_by is testperm
+		todo1 = create_new_todo('Test1', 'testperm@example.com')
+		test_user = frappe.get_doc('User', 'test4@example.com')
+
+		#owner is testperm, but assigned_by is test4
+		todo2 = create_new_todo('Test2', 'test4@example.com')
+
+		frappe.set_user('test4@example.com')
+		#owner and assigned_by is test4
+		todo3 = create_new_todo('Test3', 'test4@example.com')
+		
+		# user without any role to read or write todo document
+		self.assertFalse(todo1.has_permission("read"))
+		self.assertFalse(todo1.has_permission("write"))
+
+		# user without any role but he/she is assigned_by of that todo document
+		self.assertTrue(todo2.has_permission("read"))
+		self.assertTrue(todo2.has_permission("write"))
+
+		# user is the owner and assigned_by of the todo document
+		self.assertTrue(todo3.has_permission("read"))
+		self.assertTrue(todo3.has_permission("write"))
+
+		frappe.set_user('Administrator')
+
+		test_user.add_roles('Blogger')
+		add_permission('ToDo', 'Blogger')
+
+		frappe.set_user('test4@example.com')
+
+		# user with only read access to todo document, not an owner or assigned_by
+		self.assertTrue(todo1.has_permission("read"))
+		self.assertFalse(todo1.has_permission("write"))
+
+		frappe.set_user('Administrator')
+		test_user.remove_roles('Blogger')
+		reset_perms('ToDo')
+		clear_permissions_cache('ToDo')
+		frappe.db.rollback()
+
 def test_fetch_if_empty(self):
 		frappe.db.sql('delete from tabToDo')
 
@@ -74,3 +134,11 @@ def test_fetch_if_empty(self):
 
 		self.assertEqual(todo.assigned_by_full_name,
 			frappe.db.get_value('User', todo.assigned_by, 'full_name'))
+
+def create_new_todo(description, assigned_by):
+	todo = {
+		'doctype': 'ToDo',
+		'description': description,
+		'assigned_by': assigned_by
+	}
+	return frappe.get_doc(todo).insert()

frappe/desk/doctype/todo/todo.py

@@ -85,21 +85,30 @@ class ToDo(Document):
 			else:
 				raise
 
-# NOTE: todo is viewable if either owner or assigned_to or System Manager in roles
+# NOTE: todo is viewable if a user is an owner, or set as assigned_to value, or has any role that is allowed to access ToDo doctype.
 def on_doctype_update():
 	frappe.db.add_index("ToDo", ["reference_type", "reference_name"])
 
 def get_permission_query_conditions(user):
 	if not user: user = frappe.session.user
 
-	if "System Manager" in frappe.get_roles(user):
+	todo_roles = frappe.permissions.get_doctype_roles('ToDo')
+	if 'All' in todo_roles: 
+		todo_roles.remove('All')
+
+	if any(check in todo_roles for check in frappe.get_roles(user)):
 		return None
 	else:
 		return """(`tabToDo`.owner = {user} or `tabToDo`.assigned_by = {user})"""\
 			.format(user=frappe.db.escape(user))
 
-def has_permission(doc, user):
-	if "System Manager" in frappe.get_roles(user):
+def has_permission(doc, ptype="read", user=None):
+	user = user or frappe.session.user
+	todo_roles = frappe.permissions.get_doctype_roles('ToDo', ptype)
+	if 'All' in todo_roles: 
+		todo_roles.remove('All')
+
+	if any(check in todo_roles for check in frappe.get_roles(user)):
 		return True
 	else:
 		return doc.owner==user or doc.assigned_by==user

frappe/permissions.py

@@ -362,6 +362,11 @@ def get_roles(user=None, with_standard=True):
 
 	return roles
 
+def get_doctype_roles(doctype, access_type="read"):
+	"""Returns a list of roles that are allowed to access passed doctype."""
+	meta = frappe.get_meta(doctype)
+	return [d.role for d in meta.get("permissions") if d.get(access_type)]
+
 def get_perms_for(roles, perm_doctype='DocPerm'):
 	'''Get perms for given roles'''
 	filters = {