Skip to content

Has permission hook #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
elshafei-developer wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

Has permission hook #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
cdeb5dc
Update permissions.py
elshafei-developer May 26, 2025
00a7058
Update toolbar.js
elshafei-developer May 26, 2025
c85fcc6
Update permissions.py
elshafei-developer May 27, 2025
3a62de3
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Jun 25, 2025
dc4618f
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Jul 2, 2025
56f80c1
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Jul 29, 2025
819a27d
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Aug 28, 2025
d8a2560
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Sep 7, 2025
3fe923b
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Oct 11, 2025
ae1a930
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Oct 12, 2025
f10c008
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Oct 27, 2025
91bce13
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Nov 2, 2025
fe3e767
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Nov 17, 2025
ecc52d2
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Dec 4, 2025
d196275
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Dec 25, 2025
174f48a
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer Jan 1, 2026
ccd627f
Merge branch 'frappe:develop' into has_permission-hook
elshafei-developer May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions frappe/permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,6 @@ def get_doc_permissions(doc, user=None, ptype=None, debug=False):
def is_user_owner():
return (doc.get("owner") or "").lower() == user.lower()

if not has_controller_permissions(doc, ptype, user=user, debug=debug):
push_perm_check_log(_("Not allowed via controller permission check"), debug=debug)
return {ptype: 0}

permissions = copy.deepcopy(get_role_permissions(meta, user=user, is_owner=is_user_owner(), debug=debug))

debug and _debug_log(
Expand Down Expand Up @@ -272,6 +268,10 @@ def is_user_owner():
debug and _debug_log("User has no permissions because of User Permissions")
permissions = {}

if not has_controller_permissions(doc, ptype, permissions, user=user, debug=debug):
push_perm_check_log(_("Not allowed via controller permission check"), debug=debug)
return {ptype: 0}

debug and _debug_log(
"Final applicable permissions after evaluating user permissions: "
+ frappe.as_json(permissions, indent=8)
Expand Down Expand Up @@ -478,7 +478,7 @@ def check_user_permission_on_link_fields(d):
return True


def has_controller_permissions(doc, ptype, user=None, debug=False) -> bool:
def has_controller_permissions(doc, ptype, permissions, user=None, debug=False) -> bool:
"""Return controller permissions if denied, True if not defined.

Controllers can only deny permission, they can not explicitly grant any permission that wasn't
Expand All @@ -488,9 +488,16 @@ def has_controller_permissions(doc, ptype, user=None, debug=False) -> bool:

hooks = frappe.get_hooks("has_permission")
methods = hooks.get(doc.doctype, []) + hooks.get("*", [])
org_permissions = copy.deepcopy(permissions)

for method in reversed(methods):
controller_permission = frappe.call(method, doc=doc, ptype=ptype, user=user, debug=debug)
controller_permission = frappe.call(
method, doc=doc, ptype=ptype, permissions=permissions, user=user, debug=debug
)
if permissions != org_permissions:
for key, value in permissions.items():
if org_permissions.get(key) == 0 and value == 1:
permissions[key] = 0
Comment on lines +491 to +500

@coderabbitai coderabbitai Bot May 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Preserve deny-only semantics across every hook.

org_permissions is captured only once, so a later hook can restore a right that an earlier hook already denied. It also treats missing keys as non-denied, so after Line 269 collapses the map to {}, a hook can inject permissions[ptype] = 1 and bypass the earlier denial. That reintroduces permission escalation through the controller chain.

Proposed fix 
-	org_permissions = copy.deepcopy(permissions)
-
 	for method in reversed(methods):
+		previous_permissions = copy.deepcopy(permissions)
 		controller_permission = frappe.call(
 			method, doc=doc, ptype=ptype, permissions=permissions, user=user, debug=debug
 		)
-		if permissions != org_permissions:
-			for key, value in permissions.items():
-				if org_permissions.get(key) == 0 and value == 1:
-					permissions[key] = 0
+		for key in set(previous_permissions) | set(permissions):
+			if permissions.get(key, 0) and not previous_permissions.get(key, 0):
+				permissions[key] = 0
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frappe/permissions.py` around lines 491 - 500, The loop captures
org_permissions only once and treats missing keys as non-denied, allowing later
hooks to re-enable rights; fix by computing and maintaining a persistent
denied_keys set from the initial permissions (e.g., denied_keys = {k for k,v in
permissions.items() if v == 0}) before iterating over methods, and after each
controller call (the block around controller_permission = frappe.call(...))
enforce that any key in denied_keys remains 0 in permissions, then extend
denied_keys with any new keys set to 0 by the current hook so once-denied rights
cannot be re-enabled by later hooks.

debug and _debug_log(f"Controller permission check from {method}: {controller_permission}")
if not controller_permission:
return bool(controller_permission)
Comment on lines +494 to 503

@coderabbitai coderabbitai Bot May 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Treat None as “no opinion”, not denial.

Line 502 still turns any falsy return into a hard deny. With the new permission-map contract, a hook that only mutates permissions and falls through with None will now block access, which contradicts the local "True if not defined" behavior.

Proposed fix 
-		if not controller_permission:
-			return bool(controller_permission)
+		if controller_permission is False:
+			return False
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
controller_permission = frappe.call(
method, doc=doc, ptype=ptype, permissions=permissions, user=user, debug=debug
)
if permissions != org_permissions:
for key, value in permissions.items():
if org_permissions.get(key) == 0 and value == 1:
permissions[key] = 0
debug and _debug_log(f"Controller permission check from {method}: {controller_permission}")
if not controller_permission:
return bool(controller_permission)
controller_permission = frappe.call(
method, doc=doc, ptype=ptype, permissions=permissions, user=user, debug=debug
)
if permissions != org_permissions:
for key, value in permissions.items():
if org_permissions.get(key) == 0 and value == 1:
permissions[key] = 0
debug and _debug_log(f"Controller permission check from {method}: {controller_permission}")
if controller_permission is False:
return False
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frappe/permissions.py` around lines 494 - 503, The current logic treats any
falsy controller_permission as a hard deny; change the check after the
frappe.call to only deny when the controller explicitly returns False (i.e.,
test `controller_permission is False`) so that None is treated as "no opinion"
and does not block access; keep the debug log and return False only on an
explicit False from the hook, otherwise allow the existing permission-map
mutations to take effect.

Expand Down
3 changes: 2 additions & 1 deletion frappe/public/js/frappe/form/toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,8 @@ frappe.ui.form.Toolbar = class Toolbar {
cint(this.frm.doc.docstatus) != 1 &&
!this.frm.doc.__islocal &&
!frappe.model.is_single(this.frm.doctype) &&
frappe.model.can_delete(this.frm.doctype)
frappe.model.can_delete(this.frm.doctype) &&
this.frm.perm[0].delete
) {
this.page.add_menu_item(
__("Delete"),
Expand Down