Has permission hook#3
Conversation
📝 WalkthroughWalkthroughThis PR refactors controller permission hooks to enforce that controllers operate on the evaluated permission map and can only deny permissions, not grant them. Additionally, the Delete button visibility is tightened to require explicit model-level and permission-level delete capability. ChangesPermission Tightening and Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude can check this |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@frappe/permissions.py`:
- Around line 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.
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20600239-0ae1-4bfe-991b-28820dab2f84
📒 Files selected for processing (2)
frappe/permissions.pyfrappe/public/js/frappe/form/toolbar.js
| 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 |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| 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.
1 participant
Summary by CodeRabbit