evaluate history model permissions explicitly #1017
Conversation
…Y_ENFORCE_HISTORY_MODEL_PERMISSIONS
erikvw
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1017 +/- ##
==========================================
+ Coverage 97.24% 97.30% +0.06%
==========================================
Files 23 23
Lines 1234 1263 +29
Branches 200 205 +5
==========================================
+ Hits 1200 1229 +29
Misses 16 16
Partials 18 18 ☔ View full report in Codecov by Sentry. |
|
The current implicit permissions based on concrete model change permission is confusing. This change would be very useful. Any reason it's not merged yet? |
We can't know that the model's `HistoryRecords` field is named `history` (it's simply a convention), so using `get_history_model_for_model()` is more robust.
It should now be easier to see the differences between the assertions the tests make, and less code is now duplicated. Diff notes: * Merged `test_history_view___view_only_permissions_and_revert_enabled()` and `test_history_view__view_change_permissions_and_revert_enabled()` and renamed the resulting method to `test_history_view_response_text__revert_enabled()` * Renamed `test_history_view_with_view_only_permissions_and_norevert()` to `test_history_view_response_text__revert_disabled()` and added the same logic as in the previous `test_history_view__view_change_permissions_and_revert_enabled()` by calling the new `_test_history_view_response_text_with_revert_disabled()`
Also corrected the overridden value of the `SIMPLE_HISTORY_REVERT_DISABLED` for `test_history_view__enforce_history_permissions_and_revert_enabled()`, to match with the name of the method.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Thank you, this is absolutely a necessary change, in my opinion!
Assuming the changes I added are still in line with what you had in mind, @erikvw, I'll merge in a couple weeks' time if no remarks have been made - if it's not already been merged by you in the meantime 🙂
| def has_view_permission(self, request, obj=None): | ||
| return super().has_view_permission(request, obj) | ||
|
|
||
| def has_change_permission(self, request, obj=None): | ||
| return super().has_change_permission(request, obj) | ||
|
|
||
| def has_view_or_change_permission(self, request, obj=None): | ||
| return self.has_view_permission(request, obj) or self.has_change_permission( | ||
| request, obj | ||
| ) |
There was a problem hiding this comment.
I'm curious as to why you added these methods when they provide exactly the same implementation as the super class. Is it to communicate that this can be a good starting point for future changes, should the need ever arise? In that case, it might be a good idea to add some comments 🙂
(This doesn't affect my approval in any way, though)
Add a feature to evaluate history model permissions explicitly when settings.SIMPLE_HISTORY_ENFORCE_HISTORY_MODEL_PERMISSIONS is set to True.
Description
With the example of the
Pollmodel, by default granting view and change permissions to thePollmodelimplicitly grants view and change permissions to the
Pollhistory model. When enabled, this feature expects explicit permissions to be assigned to a user wanting to access a history model. This allows for more granular permission management of the history models.The new settings attribute
SIMPLE_HISTORY_ENFORCE_HISTORY_MODEL_PERMISSIONStoggles the feature on. By default the attribute is False and there is no change in behaviour when False.The feature is built into the
admin.py. The methodshas_view_permissionsandhas_change_permissionsare no longer shared between the model and the history model. The methodshistory_viewand thehistory_form_viewinstead access the new methodshas_view_history_permissionsandhas_change_history_permissionswhere permissions for the history model are evaluated instead of the permissions for the model. If the feature is not enabled, both of the new methods just returnhas_view_permissionsorhas_change_permissions, respectively.Regardless of the permissions a user has to the model, if the user has only view permissions to the history model, the revert button is hidden (Even if
SIMPLE_HISTORY_REVERT_DISABLED=False).If the user has no permissions to the model but does have, say, view permissions to the history model, the
app_indexandchangelistare not accessible, as access to these are controlled by the model's permissions only. I think this is as it should be. If this is how an app manager wants to assign user permissions; that is, to only the history models, then the app can provide a direct link to the history model instead of navigating through the app_index->changelist->history.It makes more sense to use this feature with
SIMPLE_HISTORY_REVERT_DISABLED = False.Related Issue
#631, #704, #529, #475Motivation and Context
Our application has an auditor role. We have been disabling the REVERT button using the settings attribute, but it recently seemed to stop working as expected. I believe this is because the
viewpermission had been somewhat muddled with thechangepermission of the model. More importantly though, I want to be able to allow access to some history records and not others based on a user role / group.How Has This Been Tested?
Screenshots (if appropriate):
None
Types of changes
Checklist:
pre-commit runcommand to format and lint.AUTHORS.rstCHANGES.rst