fix: get_doctypes_with_custom_docperms and get_doctypes_with_read #26018
Conversation
…ocperms because unused in frappe and erpnext
casesolved-co-uk
requested review from
a team and
ankush
and removed request for
a team
github-actions
bot
added
the
add-test-cases
ankush
changed the title
| @@ -461,8 +461,8 @@ def get_valid_perms(doctype=None, user=None): | |||
| perms = get_perms_for(roles) | |||
| custom_perms = get_perms_for(roles, "Custom DocPerm") | |||
|
|
|||
| doctypes_with_custom_perms = get_doctypes_with_custom_docperms() | |||
| for p in perms: | |||
| doctypes_with_custom_perms = list(set(p.parent for p in custom_perms if p.parent)) | |||
There was a problem hiding this comment.
This isn't correct.
If ANY custom perms exist then only custom perms should be used. The code is bit convoluted here because it's supposed to work with specific doctype or all doctypes.
This change will break use case like this:
- Standard role perms allow read to role X but not Y.
- Custom role perms don't allow X to read.
- Now custom docperms will be empty for role X.
As per your change standard role perms will be appended and user has no way to remove it?
There was a problem hiding this comment.
I disagree.
Consider this set of permissions:
| Type | DocType | Role | Perm |
|---|---|---|---|
| Standard | Project | Projects User | proj1 |
| Standard | Project | Projects Manager | proj2 |
| Standard | Timesheet | Projects Manager | timesheet1 |
| Custom | Project | Projects User | proj3 |
| Custom | Project | Projects Manager | proj4 |
| Custom | Timesheet | Employee | timesheet2 |
If User only has Roles [Projects User, Projects Manager]:
Existing permissions results:
perms = proj1, proj2, timesheet1
initial custom_perms = proj3, proj4
doctypes_with_custom_perms = Project, Timesheet
result custom_perms = proj3, proj4
In other words standard permissions for Timesheet for all Users are removed simply by adding a Custom DocPerm to Timesheet for any role
My permission results:
perms = proj1, proj2, timesheet1
initial custom_perms = proj3, proj4
doctypes_with_custom_perms = Project
result custom_perms = proj3, proj4, timesheet1
Standard timesheet permissions are preserved.
The only way the current code works is if all permissions for a doctype are copied from standard when a custom docperm for that doctype is created.
My code also saves a database query.
closes frappe#26015 Extracted from frappe#26018
closes frappe#26015 Extracted from frappe#26018
|
Added failing test case here: #26037 |
ankush
added
backport version-14-hotfix
Your added test case doesn't answer the docstring statement, because at no point does it add a Custom DocPerm def test_overrides_work_as_expected(self):
"""custom docperms should completely override standard ones""" |
|
Please read the code again. |
…26040) * fix: filter select perm in get_doctypes_with_read closes #26015 Extracted from #26018 (cherry picked from commit a1bb734) * ci: ruff only fix imports (cherry picked from commit 229fc71) # Conflicts: # .pre-commit-config.yaml * test: add test for custom docperm behaviour (cherry picked from commit 3f707f1) --------- Co-authored-by: Ankush Menat <ankush@frappe.io>
…26039) * fix: filter select perm in get_doctypes_with_read closes #26015 Extracted from #26018 (cherry picked from commit a1bb734) # Conflicts: # frappe/tests/test_permissions.py * test: add test for custom docperm behaviour (cherry picked from commit 3f707f1) # Conflicts: # frappe/tests/test_permissions.py * chore: conflicts --------- Co-authored-by: Ankush Menat <ankush@frappe.io>
fix: #26017 and #26015. Remove get_doctypes_with_custom_docperms because unused in frappe and erpnext
fixes: #26017 and #26015
version-14-hotfix
version-15-hotfix