Integrate DJ2.1 "view" permissions for both model and historical model #529

erikvw · 2019-02-16T00:52:04Z

In some scenarios, it is not ideal to allow users to revert a model instance to a previous version.
Django 2.1 introduced view permissions which, if integrated into DSH, would give an administrator the ability to allow a user to add/change a model but not revert to a previous instance through the model's history.

Description

The SimpleHistoryAdmin reviews the historical model instance permissions instead of just the model instance by overriding the has_xxxx_permission methods in ModelAdmin.

enable this feature through settings (SIMPLE_HISTORY_PERMISSIONS_ENABLED);
honor superuser privileges;
user/group permissions granted for the historical model cannot exceed those of the model. That is, granting view and change permissions for the historical model but only view permission for the model means the user has view permission only for the historical model;
text on the changelist and form becomes context sensitive (change permission vs view permission);
setting SIMPLE_HISTORY_REVERT_DISABLED=True limits all users (other than superuser) to view access. (this works for all Django versions)
the [Revert] button is changed to a [Close] button if the user has:
-- if Django >= 2.1, feature is disabled, and has view on model;
-- if Django >= 2.1, feature is enable and has view model permission and view historical model permission;
-- if settings. SIMPLE_HISTORY_REVERT_DISABLED = True (regardless of Django version).

How Has This Been Tested?

See tests in test_admin_with_permissions.py

A few tests in test_admin.py required very minor modifications.

Screenshots (if appropriate):

For a user with "view" only permission on a historical object, the changelist looks like this:

and the form looks like this:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have run the make format command to format my code
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have added my name and/or github handle to AUTHORS.rst
I have added my change to CHANGES.rst
All new and existing tests passed.

…and settings, update tests

codecov-io · 2019-02-18T02:05:14Z

Codecov Report

Merging #529 into master will decrease coverage by 0.24%.
The diff coverage is 94.82%.

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
- Coverage    97.8%   97.55%   -0.25%     
==========================================
  Files          17       17              
  Lines         866      942      +76     
  Branches      122      131       +9     
==========================================
+ Hits          847      919      +72     
- Misses          8       12       +4     
  Partials       11       11

Impacted Files	Coverage Δ
simple_history/admin.py	`96.7% <94.82%> (-1.41%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80469cc...74d1b7f. Read the comment docs.

dwasyl · 2019-06-15T21:55:31Z

Since this wasn't fully merged in yet, I jus wanted to add in 2 cents and say that rather than just hiding the Revert button the 'read only' view of the historical record might want to follow the pattern that Django has for view permission which is to render all of the fields as if there are in the readonly_fields list. That way the widgets aren't rendered which could leave some users confused UI wise.

Edit: aren't instead of are.

rossmechanic · 2019-06-18T14:07:14Z

@erikvw I like the idea suggested by @dwasyl . Thoughts?

author erikvw <ew2789@gmail.com> 1550266705 -0600 committer erikvw <ew2789@gmail.com> 1560873977 -0500 add Close button conditionally show text move titles and show close to methods just use has_change_permission use user permissions instead of just overriding method add additional tests

SIMPLE_HISTORY_REVERT_ENABLED

refactor history_view reduce complexity ... add dj2.1 note further refactor history_view, allow superuser to override all perms and settings, update tests fix py2 incompatibles, fix revert logic with view/change revert_disabled must return false if superuser format minor format improve coverage, update docs improve coverage, update docs add test update test update CHANGES add doscstring update docs separate tests, refactor format revert runtests format change title to inline if statement minor edit to docs format, pep8

minor

format merge w/ master

erikvw · 2019-06-18T20:44:22Z

@rossmechanic and @dwasyl ... to want to follow the Django pattern of adding the fields to the "readonly_fields" is a good instinct. The difference, however, is that the options are no longer rendered (e.g. for a radio field, etc). Although a matter of opinion/preference, my experience has been that an auditor prefers not only to see the selected response but also to see what the available options were. So I would prefer to leave it at is. What do you think?

dwasyl · 2019-06-18T20:48:14Z

@erikvw @rossmechanic For what it's worth, I understand that and actually find it to be a problem with history records (since many of mine link to other models via ForeignKey or ManyToMany relationships which are never quite captured. The choices that you'll see in a historical record are coloured by the currently available relations or choices. Since it's not truly an accurate historical representation my thought is that displaying it as a widget makes it more confusing since an end-user could think those were the options at the time the record was made (which may or may not be true).

Flattening them to the readonly_fields view that Django is using for the View permission avoids all of that by saying here's what the historical record is and doesn't leave room for misinterpretation. But that's just my 2 cents.

erikvw · 2019-06-18T21:13:13Z

@rossmechanic @dwasyl You make a good case. Would you like me to explore this change?

rossmechanic · 2019-06-19T14:50:15Z

@erikvw I agree with @dwasyl . If it's an easy change, feel free to add it to this, but I'm also happy to get this merged and let @dwasyl put up a follow-on PR to make the fields read only. Up to you

erikvw · 2019-06-19T21:10:06Z

Would be nice to merge and treat @dwasyl 's suggestion as a separate PR.

rossmechanic · 2019-06-26T17:31:16Z

simple_history/admin.py

+        return self.fetch_history_list_display_callables(queryset)
+
+    @property
+    def history_manager(self):


We have a util that already does this. but fine with this for now so we can get this in

ThePumpingLemma · 2019-06-26T17:52:28Z

simple_history/templates/simple_history/object_history_form.html

    <a href="../">{% trans "History" %}</a> &rsaquo;
-    {% blocktrans with original_opts.verbose_name as verbose_name %}Revert {{verbose_name}}{% endblocktrans %}
+	{% if has_revert_permission %}
+    	{% blocktrans with original_opts.verbose_name as verbose_name %}Revert {{ verbose_name }}{% endblocktrans %}


Indentation looks off here.

rossmechanic

Hey @erikvw ! Finally got around to reviewing this. A few questions around how we structured perms. Also want to avoid some of the refactoring work that was done, so we can get this feature in and possibly look at refactoring work in a different PR. Thanks!

rossmechanic · 2019-06-26T17:37:52Z

simple_history/admin.py

+        historical_permission = request.user.has_perm(
+            "%s.%s" % (historical_opts.app_label, historical_codename)
+        )
+        return permission and historical_permission


so in this case, for view, the user has to succeed on the base model's has_view_permission function as well as have the app_label.view_historicalmodelname permission?

rossmechanic · 2019-06-26T17:41:07Z

simple_history/admin.py

+        else:
+            try:
+                view_permission = self.has_view_permission(request, obj)
+            except AttributeError:


Is this just checking that we're on version >= 2.1? Would rather have an explicit check of the django version

rossmechanic · 2019-06-26T17:43:45Z

simple_history/admin.py

+        else:
+            try:
+                has_permission = self.has_view_or_change_permission(request, obj)
+            except AttributeError:


Same as above. Would prefer to have an explicit check for django version. Makes it a lot easier to remove as we deprecate certain versions down the line

rossmechanic · 2019-06-26T17:49:50Z

simple_history/admin.py

+        historical_opts = self.history_manager.model._meta
+        historical_codename = get_permission_codename(action, historical_opts)
+        try:
+            func = getattr(self, "has_%s_permission" % action)


not sure that I necessarily want to check for this permission. For example, in the case of reverting:
Say you have Model and HistoricalModel,
if I need to have has_change_permission as well as change_historicalmodel permission, then what's the point of this change_historicalmodel permission? I think the only reason that permission would make sense would be if we wanted users who could revert objects, but not change them from scratch. In that case, you would want a user to be able to revert using only the change_historicalmodel permission. I think you shouldn't need both, you should only need the historical permission

rossmechanic · 2019-06-26T17:51:13Z

simple_history/admin.py

    object_history_template = "simple_history/object_history.html"
    object_history_form_template = "simple_history/object_history_form.html"

    def get_urls(self):
-        """Returns the additional urls used by the Reversion admin."""
+        """Returns the additional urls used by the Reversion admin.


why these changes?

rossmechanic · 2019-06-26T17:52:46Z

simple_history/admin.py

+        return obj
+
+    def get_object_history(self, object_id):
+        """Returns a queryset of historical instances.


Let's reformat these docstrings so if they can fit on one line then they're on one line

rossmechanic · 2019-06-26T17:55:42Z

simple_history/admin.py

@@ -42,53 +121,39 @@ def get_urls(self):
        return history_urls + urls

    def history_view(self, request, object_id, extra_context=None):
-        """The 'history' admin view for this model."""
+        """Overridden `history_view`.


I appreciate this refactor, but maybe let's do this in a separate PR? Hard to review in the context of the larger PR

rossmechanic · 2019-06-26T17:58:43Z

simple_history/admin.py

+    @property
+    def change_history(self):
+        try:
+            change_history = settings.SIMPLE_HISTORY_EDIT


can we change these try except AttributeErrors to getattr(settings, "SIMPLE_HISTORY_EDIT")

rossmechanic · 2019-06-26T17:59:48Z

simple_history/admin.py

@@ -207,6 +344,13 @@ def render_history_view(self, request, template, context, **kwargs):
        return render(request, template, context, **kwargs)

    def save_model(self, request, obj, form, change):
-        """Set special model attribute to user for reference after save"""
+        """Set special model attribute to user for reference after save.


same as above on docstrings

rossmechanic · 2019-07-15T20:38:17Z

Hey @erikvw any update on this?

rossmechanic · 2019-08-02T14:34:48Z

@erikvw lmk if you want me to take this over

erikvw · 2019-08-05T15:35:09Z

been very busy recently, ...I'll try to look at this either today or tomorrow. but failing that I may ask you to take over ...

…

On Fri, Aug 2, 2019 at 9:34 AM Ross Mechanic ***@***.***> wrote: @erikvw <https://github.com/erikvw> lmk if you want me to take this over — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#529>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD2TZH3JYXENSB5PSFASVDQCRAYRANCNFSM4GX2XGXQ> .

rossmechanic · 2019-08-05T16:50:24Z

Sounds good @erikvw ! Just lmk

… as a kwarg, format

jeking3 · 2021-09-17T16:32:25Z

This needs to be rebased.

erikvw added 13 commits February 15, 2019 12:33

override has_perms in modeladmin, check permissions in template

bacb1e1

historical perms cannot exceed model perms

76e7d69

add Close button

bb60daa

conditionally show text

369bc29

move titles and show close to methods

f590820

just use has_change_permission

3b25c87

use user permissions instead of just overriding method

6e46dec

format

e33fafa

add additional tests

9d31d50

add doc section on admin permissions

ec0e6e7

SIMPLE_HISTORY_REVERT_ENABLED

a06999c

refactor history_view, consider if superuser

9bc9d36

refactor history_view

2f8d52a

This was referenced Feb 16, 2019

Utilize has_view_permission to let users with view permissions see the history #475

Closed

Admin view needs has_view_permission in context for Django 2.1 #443

Closed

erikvw added 5 commits February 16, 2019 17:58

reduce complexity ...

7d06750

add dj2.1 note

4310b80

further refactor history_view, allow superuser to override all perms …

78b6a24

…and settings, update tests

fix py2 incompatibles, fix revert logic with view/change

65e7f05

revert_disabled must return false if superuser

48333b0

erikvw added 9 commits February 17, 2019 20:11

format

55f544c

minor

e9ed6ff

format

4dfc263

improve coverage, update docs

f09cdca

improve coverage, update docs

2df10d6

add test

876811e

update test

70970c6

update CHANGES

47640d5

add doscstring

81aeb03

erikvw added 8 commits June 18, 2019 10:55

override has_perms in modeladmin, check permissions in template

581e673

historical perms cannot exceed model perms

04b312c

add doc section on admin permissions

7a0ad5a

SIMPLE_HISTORY_REVERT_ENABLED

use get_model for contenttype to avoid import errors when subclassing

6967514

give user an opt out by showing close button next to revert button

a7a00db

minor

return none if both obj and object_history is none

2ea62ab

format merge w/ master

merge

8f8f4bc

rossmechanic reviewed Jun 26, 2019

View reviewed changes

ThePumpingLemma reviewed Jun 26, 2019

View reviewed changes

rossmechanic requested changes Jun 26, 2019

View reviewed changes

erikvw added 4 commits September 14, 2019 17:36

change method name to get_historical_permission, pass readonly_fields…

8f21054

… as a kwarg, format

format imports

8ef1e55

use new databases attr instead of multi_db

1619af2

fix merge conflicts

74d1b7f

erikvw mentioned this pull request Feb 16, 2020

Admin revert enabled/disabled, view only permissions #631

Closed

11 tasks

erikvw mentioned this pull request Aug 3, 2022

evaluate history model permissions explicitly #1017

Merged

11 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate DJ2.1 "view" permissions for both model and historical model #529

Integrate DJ2.1 "view" permissions for both model and historical model #529

erikvw commented Feb 16, 2019 •

edited

codecov-io commented Feb 18, 2019 •

edited

dwasyl commented Jun 15, 2019 •

edited

rossmechanic commented Jun 18, 2019

erikvw commented Jun 18, 2019

dwasyl commented Jun 18, 2019 •

edited

erikvw commented Jun 18, 2019

rossmechanic commented Jun 19, 2019

erikvw commented Jun 19, 2019

rossmechanic Jun 26, 2019

ThePumpingLemma Jun 26, 2019

rossmechanic left a comment

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic Jun 26, 2019

rossmechanic commented Jul 15, 2019

rossmechanic commented Aug 2, 2019

erikvw commented Aug 5, 2019 via email

rossmechanic commented Aug 5, 2019

jeking3 commented Sep 17, 2021

Integrate DJ2.1 "view" permissions for both model and historical model #529

Integrate DJ2.1 "view" permissions for both model and historical model #529

Conversation

erikvw commented Feb 16, 2019 • edited

Description

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov-io commented Feb 18, 2019 • edited

Codecov Report

dwasyl commented Jun 15, 2019 • edited

rossmechanic commented Jun 18, 2019

erikvw commented Jun 18, 2019

dwasyl commented Jun 18, 2019 • edited

erikvw commented Jun 18, 2019

rossmechanic commented Jun 19, 2019

erikvw commented Jun 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmechanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmechanic commented Jul 15, 2019

rossmechanic commented Aug 2, 2019

erikvw commented Aug 5, 2019 via email

rossmechanic commented Aug 5, 2019

jeking3 commented Sep 17, 2021

erikvw commented Feb 16, 2019 •

edited

codecov-io commented Feb 18, 2019 •

edited

dwasyl commented Jun 15, 2019 •

edited

dwasyl commented Jun 18, 2019 •

edited