[Backport 5.0.x] [Fixes #14217 #14219] Permissons diffing and fix to permissions removal#14225
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a structured mechanism for diffing and applying permission changes through the new PermSpecCompactDiff class and a diff method in PermSpecCompact. The API's resource_service_permissions view now supports PATCH requests by merging proposed changes with current permissions and correctly filtering restricted group updates based on user permissions. Additionally, the patch_perms utility was refactored to utilize this new diffing logic, and comprehensive tests were added to ensure correctness. Feedback was provided to ensure that the apply method receives a dictionary as expected by the PermSpecCompact constructor to avoid potential type errors.
| entries are appended (replacing any pre-existing entry with the same | ||
| id so the operation is idempotent against stale diffs). | ||
| """ | ||
| result = PermSpecCompact(current_perms_compact, resource) |
There was a problem hiding this comment.
The PermSpecCompact constructor expects a dictionary as its first argument. Ensure that current_perms_compact passed to apply is indeed a dictionary (e.g., the result of PermSpec.compact). If it's already a PermSpecCompact instance, this call might fail or lead to unexpected behavior because _bind_json expects a dict.
References
- Defensive programming: ensure appropriate type checks or guards exist before object property accesses or constructor calls.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 5.0.x #14225 +/- ##
========================================
Coverage ? 74.41%
========================================
Files ? 940
Lines ? 56566
Branches ? 7665
========================================
Hits ? 42093
Misses ? 12794
Partials ? 1679 🚀 New features to boost your workflow:
|
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.