From bd8862f1a41d11c8fd1ae6a8fd956bb89fb1da3d Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sat, 3 Apr 2021 15:08:27 +0530 Subject: [PATCH 1/9] fix: Apply only if creator perm irrespective of user --- frappe/permissions.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/frappe/permissions.py b/frappe/permissions.py index abb1f6653a..b6f22ec782 100644 --- a/frappe/permissions.py +++ b/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) From 8460ca851f6cd8cfffa01f7618d3130adb993b3a Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sat, 3 Apr 2021 16:12:26 +0530 Subject: [PATCH 2/9] fix: filter list view based on owner --- frappe/model/db_query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index b29e143759..1c863a1577 100644 --- a/frappe/model/db_query.py +++ b/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 From cb27653da1b5db7c572781dea6f0e55a5bbd409d Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sat, 10 Apr 2021 22:34:12 +0530 Subject: [PATCH 3/9] fix: Test case for if_owner perm with getdoc --- frappe/tests/test_permissions.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index 6897d500c9..a0913b7867 100644 --- a/frappe/tests/test_permissions.py +++ b/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,8 @@ class TestPermissions(unittest.TestCase): user = frappe.get_doc("User", "test3@example.com") user.add_roles("Sales User") + user.add_roles("Blogger") + frappe.flags.permission_user_setup_done = True reset('Blogger') @@ -464,6 +467,28 @@ 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("test3@example.com") + + doc = frappe.get_doc({ + "doctype": "Blog Post", + "blog_category": "_Test Blog Category", + "blogger": "_Test Blogger 1", + "title": "_Test Blog Post Title", + "content": "_Test Blog Post Content" + }) + + doc.insert() + + frappe.set_user("test2@example.com") + self.assertRaises(frappe.PermissionError, getdoc, 'Blog Post', doc.name) + def test_clear_user_permissions(self): current_user = frappe.session.user frappe.set_user('Administrator') From 7b929fa72c86dd65afc3fd40f871d22dbb733842 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sat, 10 Apr 2021 23:05:08 +0530 Subject: [PATCH 4/9] fix: Update blog category --- frappe/tests/test_permissions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index a0913b7867..9ea58f7a78 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -478,7 +478,7 @@ class TestPermissions(unittest.TestCase): doc = frappe.get_doc({ "doctype": "Blog Post", - "blog_category": "_Test Blog Category", + "blog_category": "-test-blog-category", "blogger": "_Test Blogger 1", "title": "_Test Blog Post Title", "content": "_Test Blog Post Content" From a4c973a5496496a5ddfc743c90087827853730ba Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sun, 11 Apr 2021 10:26:07 +0530 Subject: [PATCH 5/9] fix: Update users for test --- frappe/tests/test_permissions.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index 9ea58f7a78..83aa4c1617 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -31,7 +31,6 @@ class TestPermissions(unittest.TestCase): user = frappe.get_doc("User", "test3@example.com") user.add_roles("Sales User") - user.add_roles("Blogger") frappe.flags.permission_user_setup_done = True @@ -474,7 +473,7 @@ class TestPermissions(unittest.TestCase): update('Blog Post', 'Blogger', 0, 'delete', 1) frappe.clear_cache(doctype="Blog Post") - frappe.set_user("test3@example.com") + frappe.set_user("test1@example.com") doc = frappe.get_doc({ "doctype": "Blog Post", @@ -489,6 +488,10 @@ class TestPermissions(unittest.TestCase): frappe.set_user("test2@example.com") self.assertRaises(frappe.PermissionError, getdoc, 'Blog Post', doc.name) + # delete the created doc + frappe.delete_doc('Blog Post', '-test-blog-post-title') + reset('Blog Post') + def test_clear_user_permissions(self): current_user = frappe.session.user frappe.set_user('Administrator') From dce4f62ff3382b51ce4992a03f2538216f233581 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sun, 11 Apr 2021 10:27:44 +0530 Subject: [PATCH 6/9] fix: Do not reset blog post perm --- frappe/tests/test_permissions.py | 1 - 1 file changed, 1 deletion(-) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index 83aa4c1617..cc2881b7d2 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -490,7 +490,6 @@ class TestPermissions(unittest.TestCase): # delete the created doc frappe.delete_doc('Blog Post', '-test-blog-post-title') - reset('Blog Post') def test_clear_user_permissions(self): current_user = frappe.session.user From d3c363a8f4cfc57ee7589d56e440f6eeccd9c867 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sun, 11 Apr 2021 12:42:37 +0530 Subject: [PATCH 7/9] fix: Do not delete blog post --- frappe/tests/test_permissions.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index cc2881b7d2..2c246885a7 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -488,9 +488,6 @@ class TestPermissions(unittest.TestCase): frappe.set_user("test2@example.com") self.assertRaises(frappe.PermissionError, getdoc, 'Blog Post', doc.name) - # delete the created doc - frappe.delete_doc('Blog Post', '-test-blog-post-title') - def test_clear_user_permissions(self): current_user = frappe.session.user frappe.set_user('Administrator') From 14c67cb2448568db64de63951dc7fc2ee6dbe944 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Mon, 12 Apr 2021 19:48:47 +0530 Subject: [PATCH 8/9] fix: Test to check if_owner perm on delete --- frappe/tests/test_permissions.py | 47 +++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index 2c246885a7..59cde2193e 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -32,6 +32,9 @@ 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') @@ -479,15 +482,57 @@ class TestPermissions(unittest.TestCase): "doctype": "Blog Post", "blog_category": "-test-blog-category", "blogger": "_Test Blogger 1", - "title": "_Test Blog Post Title", + "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) + def test_clear_user_permissions(self): current_user = frappe.session.user frappe.set_user('Administrator') From a7c8b93430fe77a227e3fa7db27a6965b6f0b20e Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Mon, 12 Apr 2021 22:05:09 +0530 Subject: [PATCH 9/9] fix: Delete doc afer use --- frappe/tests/test_permissions.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index 59cde2193e..82f0ee920a 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -525,14 +525,18 @@ class TestPermissions(unittest.TestCase): frappe.set_user("testperm@example.com") - ## Website Manager able to read + # 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 + # 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')