From 7f94e158ac9317c88dccdf517dd27007604c6f67 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 15 Nov 2022 14:38:56 +0530 Subject: [PATCH 01/11] fix: Consistency in `get_role_permissions` return value - Return value contains `if_owner` propert in object same as py - Elaborate code documentation --- frappe/public/js/frappe/model/perm.js | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index 6931a2e2e7..f06c95194f 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -117,20 +117,38 @@ $.extend(frappe.perm, { }, get_role_permissions: (meta) => { + /** Returns a `dict` of evaluated Role Permissions like: + { + "read": 1, + "write": 0, + "if_owner": [if "if_owner" is enabled] + { + "read": 1, + "write": 0 + } + } */ let perm = [{ read: 0, permlevel: 0 }]; - // Returns a `dict` of evaluated Role Permissions + (meta.permissions || []).forEach((p) => { - // if user has this role let permlevel = cint(p.permlevel); if (!perm[permlevel]) { perm[permlevel] = {}; perm[permlevel]["permlevel"] = permlevel; } + // if user has this role if (frappe.user_roles.includes(p.role)) { frappe.perm.rights.forEach((right) => { let value = perm[permlevel][right] || p[right] || 0; - if (value) { + + if (p.if_owner && value) { + // if_owner is enabled for perm, + // construct perm object inside "if_owner" property + if (!perm[permlevel]["if_owner"]) { + perm[permlevel]["if_owner"] = {} + } + perm[permlevel]["if_owner"][right] = value; + } else if (value) { perm[permlevel][right] = value; } }); From 92d9e7d61162b0186485c2c5684bc2e48d4b23ec Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 15 Nov 2022 17:57:49 +0530 Subject: [PATCH 02/11] fix: Don't assign document level perms to doctype level permission store - If `doc` is passed to `has_perm`, checking for pre-stored doctype level perms is wrong - It gives back stale values that don't consider document level permissions and only change on reload (window property) - Get freshly evaluate perms at the doc level - If no `doc` is involved, doctype level custom window property can be used --- frappe/public/js/frappe/model/perm.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index f06c95194f..b9112174e4 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -37,11 +37,13 @@ $.extend(frappe.perm, { has_perm: (doctype, permlevel, ptype, doc) => { if (!permlevel) permlevel = 0; - if (!frappe.perm.doctype_perm[doctype]) { - frappe.perm.doctype_perm[doctype] = frappe.perm.get_perm(doctype, doc); + if (!frappe.perm.doctype_perm[doctype] && !doc) { + frappe.perm.doctype_perm[doctype] = frappe.perm.get_perm(doctype); } - let perms = frappe.perm.doctype_perm[doctype]; + // if document object is passed, get fresh doc based perms + // (with ownership and user perms applied) else stale doctype perms + let perms = doc ? frappe.perm.get_perm(doctype, doc) : frappe.perm.doctype_perm[doctype]; if (!perms || !perms[permlevel]) return false; From ff1c9be33b0dc2846e3eb6d461cfd137999bad4f Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 18 Nov 2022 00:52:38 +0530 Subject: [PATCH 03/11] fix: correct logic for `if_owner` and refactor --- frappe/public/js/frappe/model/perm.js | 100 +++++++++++--------------- 1 file changed, 43 insertions(+), 57 deletions(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index b9112174e4..524a0f5b1a 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -37,17 +37,14 @@ $.extend(frappe.perm, { has_perm: (doctype, permlevel, ptype, doc) => { if (!permlevel) permlevel = 0; - if (!frappe.perm.doctype_perm[doctype] && !doc) { - frappe.perm.doctype_perm[doctype] = frappe.perm.get_perm(doctype); - } // if document object is passed, get fresh doc based perms - // (with ownership and user perms applied) else stale doctype perms - let perms = doc ? frappe.perm.get_perm(doctype, doc) : frappe.perm.doctype_perm[doctype]; + // (with ownership and user perms applied) else cached doctype perms + const perms = doc + ? frappe.perm.get_perm(doctype, doc) + : (frappe.perm.doctype_perm[doctype] ??= frappe.perm.get_perm(doctype)); - if (!perms || !perms[permlevel]) return false; - - return !!perms[permlevel][ptype]; + return !!perms?.[permlevel]?.[ptype]; }, get_perm: (doctype, doc) => { @@ -63,56 +60,50 @@ $.extend(frappe.perm, { if (!meta) return perm; perm = frappe.perm.get_role_permissions(meta); + const base_perm = perm[0]; if (doc) { // apply user permissions via docinfo (which is processed server-side) let docinfo = frappe.model.get_docinfo(doctype, doc.name); if (docinfo && docinfo.permissions) { Object.keys(docinfo.permissions).forEach((ptype) => { - perm[0][ptype] = docinfo.permissions[ptype]; + base_perm[ptype] = docinfo.permissions[ptype]; }); } // if owner - if (!$.isEmptyObject(perm[0].if_owner)) { - if (doc.owner === user) { - $.extend(perm[0], perm[0].if_owner); - } else { - // not owner, remove permissions - $.each(perm[0].if_owner, (ptype) => { - if (perm[0].if_owner[ptype]) { - perm[0][ptype] = 0; - } - }); + if (doc.owner !== user) { + for (const right of frappe.perm.rights) { + if (base_perm[right] && !base_perm.rights_without_if_owner.has(right)) { + base_perm[right] = 0; + } } } // apply permissions from shared if (docinfo && docinfo.shared) { - for (let i = 0; i < docinfo.shared.length; i++) { - let s = docinfo.shared[i]; - if (s.user === user) { - perm[0]["read"] = perm[0]["read"] || s.read; - perm[0]["write"] = perm[0]["write"] || s.write; - perm[0]["submit"] = perm[0]["submit"] || s.submit; - perm[0]["share"] = perm[0]["share"] || s.share; + for (const s of docinfo.shared) { + if (s.user !== user) continue; - if (s.read) { - // also give print, email permissions if read - // and these permissions exist at level [0] - perm[0].email = - frappe.boot.user.can_email.indexOf(doctype) !== -1 ? 1 : 0; - perm[0].print = - frappe.boot.user.can_print.indexOf(doctype) !== -1 ? 1 : 0; - } + for (const right of ["read", "write", "submit", "share"]) { + if (!base_perm[right]) base_perm[right] = s[right]; + } + + if (s.read) { + // also give print, email permissions if read + // and these permissions exist at level [0] + base_perm.email = + frappe.boot.user.can_email.indexOf(doctype) !== -1 ? 1 : 0; + base_perm.print = + frappe.boot.user.can_print.indexOf(doctype) !== -1 ? 1 : 0; } } } } - if (frappe.model.can_read(doctype) && !perm[0].read) { + if (frappe.model.can_read(doctype) && !base_perm.read) { // read via sharing - perm[0].read = 1; + base_perm.read = 1; } return perm; @@ -123,35 +114,29 @@ $.extend(frappe.perm, { { "read": 1, "write": 0, - "if_owner": [if "if_owner" is enabled] - { - "read": 1, - "write": 0 - } - } */ + "rights_without_if_owner": {"read", "write"} // for permlevel 0 + } + */ + let perm = [{ read: 0, permlevel: 0 }]; (meta.permissions || []).forEach((p) => { - let permlevel = cint(p.permlevel); - if (!perm[permlevel]) { - perm[permlevel] = {}; - perm[permlevel]["permlevel"] = permlevel; + const permlevel = cint(p.permlevel); + const current_perm = (perm[permlevel] ??= { permlevel }); + + if (permlevel === 0) { + current_perm.rights_without_if_owner ??= new Set(); } // if user has this role if (frappe.user_roles.includes(p.role)) { frappe.perm.rights.forEach((right) => { - let value = perm[permlevel][right] || p[right] || 0; + if (!p[right]) return; - if (p.if_owner && value) { - // if_owner is enabled for perm, - // construct perm object inside "if_owner" property - if (!perm[permlevel]["if_owner"]) { - perm[permlevel]["if_owner"] = {} - } - perm[permlevel]["if_owner"][right] = value; - } else if (value) { - perm[permlevel][right] = value; + current_perm[right] = 1; + + if (permlevel === 0 && !p.if_owner) { + current_perm.rights_without_if_owner.add(right); } }); } @@ -189,7 +174,8 @@ $.extend(frappe.perm, { } } - if (perm[0].if_owner && perm[0].read) { + const base_perm = perm[0]; + if (base_perm.read && !base_perm.rights_without_if_owner.has("read")) { match_rules.push({ Owner: frappe.session.user }); } return match_rules; From ed2804d1c5a30c23c17356df02d6753548878b1e Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 21 Nov 2022 13:28:53 +0530 Subject: [PATCH 04/11] test: Cypress test to check basic `has_perm` and `get_perm` JS APIs - Test removes System Manager role for user and expects certain perms - On Kanban Board DocType, only System Manager is allowed to print/export/email/report/share - DocType level test only --- cypress/integration/permissions.js | 57 ++++++++++++++++++++++++++++++ frappe/tests/ui_test_helpers.py | 9 +++++ 2 files changed, 66 insertions(+) create mode 100644 cypress/integration/permissions.js diff --git a/cypress/integration/permissions.js b/cypress/integration/permissions.js new file mode 100644 index 0000000000..7a13239771 --- /dev/null +++ b/cypress/integration/permissions.js @@ -0,0 +1,57 @@ +context("Permissions API", () => { + before(() => { + cy.visit("/login"); + + cy.login("Administrator"); + cy.call("frappe.tests.ui_test_helpers.add_remove_role", { + action: "remove", + user: "frappe@example.com", + role: "System Manager", + }); + cy.call("logout"); + + cy.login("frappe@example.com"); + cy.visit("/app"); + }); + + it("Checks permissions via `has_perm` for Kanban Board DocType", () => { + cy.visit("/app/kanban-board/view/list"); + cy.window() + .its("frappe") + .then((frappe) => { + frappe.model.with_doctype("Kanban Board", function () { + // needed to make sure doc meta is loaded + expect(frappe.perm.has_perm("Kanban Board", 0, "read")).to.equal(true); + expect(frappe.perm.has_perm("Kanban Board", 0, "write")).to.equal(true); + expect(frappe.perm.has_perm("Kanban Board", 0, "print")).to.equal(false); + }); + }); + }); + + it("Checks permissions via `get_perm` for Kanban Board DocType", () => { + cy.visit("/app/kanban-board/view/list"); + cy.window() + .its("frappe") + .then((frappe) => { + frappe.model.with_doctype("Kanban Board", function () { + // needed to make sure doc meta is loaded + const perms = frappe.perm.get_perm("Kanban Board"); + expect(perms.read).to.equal(true); + expect(perms.write).to.equal(true); + expect(perms.rights_without_if_owner).to.include("read"); + }); + }); + }); + + after(() => { + cy.call("logout"); + + cy.login("Administrator"); + cy.call("frappe.tests.ui_test_helpers.add_remove_role", { + action: "add", + user: "frappe@example.com", + role: "System Manager", + }); + cy.call("logout"); + }); +}); diff --git a/frappe/tests/ui_test_helpers.py b/frappe/tests/ui_test_helpers.py index ecb6b4da97..9d32be229e 100644 --- a/frappe/tests/ui_test_helpers.py +++ b/frappe/tests/ui_test_helpers.py @@ -578,3 +578,12 @@ def create_kanban(): ], } ).insert() + + +@frappe.whitelist() +def add_remove_role(action, user, role): + user_doc = frappe.get_doc("User", user) + if action == "remove": + user_doc.remove_roles(role) + else: + user_doc.add_roles(role) From d529b2a64e3301f522e4a9b0b568b76af11b7e19 Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 21 Nov 2022 15:32:39 +0530 Subject: [PATCH 05/11] chore: Cache Document level perms as well in `has_perm` - Cache documet level output of `get_perm` in `has_perm` as well --- frappe/public/js/frappe/model/perm.js | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index 524a0f5b1a..66bdb3e02b 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -33,16 +33,26 @@ $.extend(frappe.perm, { "set_user_permissions", ], - doctype_perm: {}, + doc_perm: {}, has_perm: (doctype, permlevel, ptype, doc) => { if (!permlevel) permlevel = 0; - // if document object is passed, get fresh doc based perms - // (with ownership and user perms applied) else cached doctype perms - const perms = doc - ? frappe.perm.get_perm(doctype, doc) - : (frappe.perm.doctype_perm[doctype] ??= frappe.perm.get_perm(doctype)); + /** frappe.perm.doc_perm structure: + * { + * DocType: { + * "doctype_perm": {...perms}, + * : {...document perms}, + * : {...document perms}, + * } + * } + */ + // Cache doctype perms and document perms (with user perms and ownership) if absent + const doctype_perm = (frappe.perm.doc_perm[doctype] ??= {}); + const perms = (doctype_perm[doc ? doc.name : "doctype_perm"] ??= frappe.perm.get_perm( + doctype, + doc + )); return !!perms?.[permlevel]?.[ptype]; }, From 0937c962a700b500125ef27f625e58bcb2f67e3b Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 21 Nov 2022 16:18:58 +0530 Subject: [PATCH 06/11] chore: `web_form.py` format via pre-commit --- frappe/website/doctype/web_form/web_form.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/website/doctype/web_form/web_form.py b/frappe/website/doctype/web_form/web_form.py index 2bfa7e7133..d102ac2fd8 100644 --- a/frappe/website/doctype/web_form/web_form.py +++ b/frappe/website/doctype/web_form/web_form.py @@ -609,4 +609,6 @@ def get_link_options(web_form_name, doctype, allow_read_on_all_link_options=Fals return "\n".join([doc.value for doc in link_options]) else: - raise frappe.PermissionError(_("You don't have permission to access the {0} DocType.").format(doctype)) + raise frappe.PermissionError( + _("You don't have permission to access the {0} DocType.").format(doctype) + ) From 4cd80bd27928e39bdc3569c22e3f5d6e69168c90 Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 21 Nov 2022 16:41:37 +0530 Subject: [PATCH 07/11] fix: Avoid caching unsaved documents and secure whitelist decorator - Avoid caching documents like 'new-item-1', default to doctype perms instead - Use secure `whitelist_for_tests` decorator instead --- frappe/public/js/frappe/model/perm.js | 8 ++++---- frappe/tests/ui_test_helpers.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index 66bdb3e02b..2cd553a43d 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -49,10 +49,10 @@ $.extend(frappe.perm, { */ // Cache doctype perms and document perms (with user perms and ownership) if absent const doctype_perm = (frappe.perm.doc_perm[doctype] ??= {}); - const perms = (doctype_perm[doc ? doc.name : "doctype_perm"] ??= frappe.perm.get_perm( - doctype, - doc - )); + const doc_exists = doc && !doc.__islocal; + + const perms = (doctype_perm[doc_exists ? doc.name : "doctype_perm"] ??= + frappe.perm.get_perm(doctype, doc)); return !!perms?.[permlevel]?.[ptype]; }, diff --git a/frappe/tests/ui_test_helpers.py b/frappe/tests/ui_test_helpers.py index aabcd28c8b..158f8988c7 100644 --- a/frappe/tests/ui_test_helpers.py +++ b/frappe/tests/ui_test_helpers.py @@ -581,7 +581,7 @@ def create_kanban(): ).insert() -@frappe.whitelist() +@whitelist_for_tests() def add_remove_role(action, user, role): user_doc = frappe.get_doc("User", user) if action == "remove": From 8003b907c132372516082c4611d2d109d89c129c Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Mon, 21 Nov 2022 17:11:34 +0530 Subject: [PATCH 08/11] chore: cache `role_perms` instead --- frappe/public/js/frappe/model/perm.js | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index 2cd553a43d..eb2c24983f 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -33,27 +33,12 @@ $.extend(frappe.perm, { "set_user_permissions", ], - doc_perm: {}, + role_perms: {}, has_perm: (doctype, permlevel, ptype, doc) => { if (!permlevel) permlevel = 0; - /** frappe.perm.doc_perm structure: - * { - * DocType: { - * "doctype_perm": {...perms}, - * : {...document perms}, - * : {...document perms}, - * } - * } - */ - // Cache doctype perms and document perms (with user perms and ownership) if absent - const doctype_perm = (frappe.perm.doc_perm[doctype] ??= {}); - const doc_exists = doc && !doc.__islocal; - - const perms = (doctype_perm[doc_exists ? doc.name : "doctype_perm"] ??= - frappe.perm.get_perm(doctype, doc)); - + const perms = frappe.perm.get_perm(doctype, doc); return !!perms?.[permlevel]?.[ptype]; }, @@ -69,7 +54,7 @@ $.extend(frappe.perm, { if (!meta) return perm; - perm = frappe.perm.get_role_permissions(meta); + perm = frappe.perm.role_perms[doctype] ??= frappe.perm.get_role_permissions(meta); const base_perm = perm[0]; if (doc) { From 6e896aa412f777fddf436447d74a89ac2335bc07 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Mon, 21 Nov 2022 17:17:18 +0530 Subject: [PATCH 09/11] fix: decorator usage --- frappe/tests/ui_test_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/tests/ui_test_helpers.py b/frappe/tests/ui_test_helpers.py index 158f8988c7..05d3dedad6 100644 --- a/frappe/tests/ui_test_helpers.py +++ b/frappe/tests/ui_test_helpers.py @@ -581,7 +581,7 @@ def create_kanban(): ).insert() -@whitelist_for_tests() +@whitelist_for_tests def add_remove_role(action, user, role): user_doc = frappe.get_doc("User", user) if action == "remove": From e10a3d0b8dba86d6d1ae0dba16568f79c7f8e58b Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Mon, 21 Nov 2022 17:30:32 +0530 Subject: [PATCH 10/11] chore: move cheaper condition first --- frappe/public/js/frappe/model/perm.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index eb2c24983f..1951a9d841 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -96,7 +96,7 @@ $.extend(frappe.perm, { } } - if (frappe.model.can_read(doctype) && !base_perm.read) { + if (!base_perm.read && frappe.model.can_read(doctype)) { // read via sharing base_perm.read = 1; } From b8e7deea40927d94f6e4d8e5c214e5a7c2b0db0b Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Mon, 21 Nov 2022 18:06:08 +0530 Subject: [PATCH 11/11] chore: revert to `doctype_perm` cache for now --- frappe/public/js/frappe/model/perm.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/frappe/public/js/frappe/model/perm.js b/frappe/public/js/frappe/model/perm.js index 1951a9d841..fdd915ebfc 100644 --- a/frappe/public/js/frappe/model/perm.js +++ b/frappe/public/js/frappe/model/perm.js @@ -33,7 +33,7 @@ $.extend(frappe.perm, { "set_user_permissions", ], - role_perms: {}, + doctype_perm: {}, has_perm: (doctype, permlevel, ptype, doc) => { if (!permlevel) permlevel = 0; @@ -43,6 +43,17 @@ $.extend(frappe.perm, { }, get_perm: (doctype, doc) => { + // if document object is passed, get fresh doc based perms + // (with ownership and user perms applied) else cached doctype perms + + if (doc && !doc.__islocal) { + return frappe.perm._get_perm(doctype, doc); + } + + return (frappe.perm.doctype_perm[doctype] ??= frappe.perm._get_perm(doctype)); + }, + + _get_perm: (doctype, doc) => { let perm = [{ read: 0, permlevel: 0 }]; let meta = frappe.get_doc("DocType", doctype); @@ -54,7 +65,7 @@ $.extend(frappe.perm, { if (!meta) return perm; - perm = frappe.perm.role_perms[doctype] ??= frappe.perm.get_role_permissions(meta); + perm = frappe.perm.get_role_permissions(meta); const base_perm = perm[0]; if (doc) {