Fixed #8936 -- Added view permissions and a read-only admin #5297

PetrDlouhy · 2015-09-16T16:20:24Z

This is implementation of ticket 8936.
It adds 'view' permission to Django models options besides existing 'add', 'delete' and 'edit' permissions. It also changes the behaviour of admin if that permission is set. It completes the CRUD pattern for Django permissions.

The implementation is quite straightforward. Still, there few weak spots, that I am feeling that should be discussed with someone more experienced:

Model situation: User has defined ModelAdmin with custom has_change_permission() such that it disallows to change (consequently also view) anything.
- Then adding view permissions allows superuser to view that ModelAdmin.
- I had to change RowLevelChangePermissionModelAdmin in tests for this.
How should change RelatedFieldWidgetWrapper

PetrDlouhy · 2015-09-16T16:20:53Z

This was previously PR #5108

PetrDlouhy · 2015-09-16T16:35:12Z

@timgraham: I made simple application (https://github.com/PetrDlouhy/view_permissions_test) and made some testing there.

I have fixed some bugs and done some functionality drawbacks:

fixed behaviour with add permission
adjusted the save buttons behaviour (now it shows them, when there are some inlines present)
inlines without change permissions are not displayed at all, because it is not possible to get coherent behaviour without fixing this ticket: https://code.djangoproject.com/ticket/15602

I have also added some migration notes to the docs.

collinanderson · 2015-09-16T16:35:18Z

Also, just checking, does this work properly with list_editable? (as in people with view permission can see but not edit?).

PetrDlouhy · 2015-09-16T16:47:19Z

@collinanderson: Thanks for pointing this out. I didn't know about this feature until now.

According to my tests: It doesn't allow user to save the changes (403 forbidden), but it shows the page with editable inputs and save button.

I will get to fix this tomorrow.

PetrDlouhy · 2015-09-17T11:56:37Z

@collinanderson Now it is fixed. Without change permissions the list page will show without the edit elements.

PetrDlouhy · 2015-09-17T15:55:18Z

I realized few more problems with inlines and that it requires quite complex change. So I drew back this functionality for inlines completely.
Now view permissions work only for base model and inline functionality remain the same as in previous Django versions.

timgraham · 2015-10-22T12:30:33Z

docs/releases/1.9.txt

This documentation is looking better, but the bulk of it should be moved into the admin docs so that users of future versions of Django don't have to refer back to the release notes to see how it works. We need to target 1.10 now that 1.9 is feature frozen.

timgraham · 2015-10-24T22:24:00Z

I think view permissions for inlines really need to be implemented as part of this. It's going to cause a lot of confusion if not (e.g. to see the headings and no objects).

PetrDlouhy · 2015-10-24T23:41:23Z

@timgraham: OK, I made this compromise in belief, that it could catch 1.9 freeze deadline.

I will try to make the inlines support. It will very probably bloat to much bigger change. According to my explorations go, I will need to make changes in BaseInlineFormSet and it's factory to make possible to either only allow to add new related object or only allow to change existing objects. I didn't want to go so deep, because I was expecting compatibility issues.

For the view permissions could be enough to only add something like can_save_existing and can_save_new parameters, which would work generally for whole formset.

Though more systematic would be implementing some mechanism which allowed to run has_xxx_permission functions for all the saved objects. The problem is, that these functions has the request parameter, which is not available in BaseInlineFormSet. I didn't manage to solve this issue nicely.

I will continue my investigation, how it could be done. Although some more help with this would be welcome.

timgraham · 2015-10-24T23:51:02Z

Can you describe at a high level what the problem is with inlines? It seems to me a combination of "get_uneditable_fields" and inline.max_num=0 should be roughly enough for a read-only view, but I could be missing something.

PetrDlouhy · 2015-11-24T15:30:03Z

@timgraham: The edge case is, when user does have only view and add permission. We need to allow saving of the change form, but changing of the related values shouldn't be permitted (even if the POST request is planted).

With the current code, the inlines seem visually OK, but saving of the inline formset is not treated correctly - it will pass the planted data.

The permission should be somehow checked during saving of the inline formset. Checking readonly/uneditable fields is not enough, because we should treat differently new and changed objects.

PS: I will merge the commits into one, write the documentation, correct and complete tests and finish other things once the inlines are solved.

PetrDlouhy · 2015-11-25T23:14:34Z

I finally managed to get this working somehow by modifying DeleteProtectedModelForm. Please comment, if you consider this approach to be meaningful.

I am planning to clean the code up.

timgraham · 2015-11-25T23:57:49Z

I'll try to take a look at this soon, though I have a lot of review in the backlog. Just wanted to ask you to please be mindful that each commit you push triggers a build on Jenkins, so try to minimize that where possible. Thanks!

PetrDlouhy · 2015-11-26T17:14:40Z

Sorry for the unnecesarry commits.

I have made cleanup, updated the docs and made some testing. There is sitll work, I am planning to do, but you can try to review now.

PetrDlouhy · 2016-01-25T12:47:21Z

Hi @timgraham, do you still have this PR in mind? :-)
Thanks.

timgraham · 2016-01-25T13:04:47Z

Not really -- since "Patch needs improvement" is checked on the ticket, it doesn't appear in the review queue.

PetrDlouhy · 2016-01-27T01:35:06Z

OK, sorry. I unchecked it now.

SafetyDog · 2016-03-03T14:59:22Z

@PetrDlouhy thank you for attempting this! I noticed that some of the default admin .html templates label individual page titles and/or actions on them as "Change" or "Add", but didn't see any changes in your code to add "View" as part of the page titles or action links. Was this work you are leaving for after the initial review? I'm curious because this feature will be a huge help for me and I can't wait for it to be part of the main Django codebase. Thanks again.

olivierdalang · 2016-04-11T09:00:39Z

Hi!

Awesome work, it will be extremely useful here!!
Is this going to land in master soon?

Thanks

olivierdalang · 2016-05-29T14:03:44Z

@PetrDlouhy @timgraham
I'm also extremely eager to have this merged ;) Is there anything we can do to make this happen ? (fixing the conflicts, testing...)

olivierdalang · 2016-06-03T23:01:50Z

Hi,

I took some time today to look at this. The merge conflicts were easy to solve, I did it here :
https://github.com/olivierdalang/django/tree/view_permission_master

I ran the tests and got only one unrelated error (something with GDAL), so I guess the merge was successful.

What should I do with this ? Make another PR ? Or @PetrDlouhy can you update you PR accordingly ?

Many thanks to you, Petr, this patch is awesome. The ability to show a model but allow some users to edit inlines only is really a killer feature. That will be extremely useful here.

collinanderson · 2016-06-06T13:39:10Z

@olivierdalang Feel free to open a new PR. (And unsetting "patch needs improvement" was perfect. It's a good way to mark the ticket as requesting feedback)

olivierdalang · 2016-06-06T16:06:14Z

@collinanderson Ok I did as you said : #6734

Please let me know if I can do anything else on this issue.

olivierdalang · 2016-06-17T01:59:23Z

django/contrib/admin/helpers.py

-        yield InlineAdminForm(self.formset, self.formset.empty_form,
-            self.fieldsets, self.prepopulated_fields, None,
-            self.readonly_fields, model_admin=self.opts)
+        if self.has_add_permission:


Hi,

I'm trying to do the rebase on 1.11 in another PR (#6734), but I am having a little trouble understanding why you had to add this condition.

I don't see how adding the view permission requires to add a check on the add permission for the inlines.

With this change, I had some strange problems (the last object in the inline got hidden because it gets the empty-form class). Removing the condition leads to an exception which looks pretty low level, so I guess there was a good reason.

I could fix it (I think) with this commit :
olivierdalang@061a2cb

But I'm not sure of the fix at all, since I don't get why the change was made in the first place.

Can you explain it to me ?

Thanks again for the great work !!

(besides, I see you have a 1.9.x backport branch for this, so do I, as I'll be needing this feature soon on 1.9.x, check it out if you're still working on this)

I suppose, this condition is there because I had to change the behaviour of empty fields on the end of Inline formset. I You don't have add permissions and only view permissions You shouldn't be able to add anything. Before it was easy, because without change permissions, the inlines was not displayed at all, but with view permissions You have to display inlines in read only way.

BTW, can you please manage to add me to be one of the authors of the read-permissions commit?

Ok I didn't think this through, I assumed that one could add inlines with the add permission.

Of course for the author. If you like I can also withdraw my PR to django's repo and do the PR to you branch, so you keep control on this (you did all the work, and you're obviously more experienced than me in this, I just thought you may have no more time to spend on this).

Also, I'm willing to backport this patch to 1.9 (or 1.10, but can't wait for 1.11 to use it in my projects). I see you have a 1.9 backport branch too, we may want to share efforts on this. My backport branch isn't in production yet, but will probably be in the two or three weeks. It seems to work well for now.

I might have more time for this now and during summer.

If I would do any changes, I would pull it from you PR and propagate to mine, but if not, I think, that your PR is OK.

I have the backport branch in my repository. It might be needed to update 1.9's security updates, which I don't expect to be much work. That branch is not pure backport, because I have one minor change about memcached's record size there also.

abusquets · 2016-07-20T20:29:42Z

When an ModelAdmin has inlines, the "save and continue editing" and "save" are visible and is possible to save.

timgraham · 2016-10-06T00:08:27Z

Continued in #6734.

Tomfox91 · 2017-01-21T18:31:09Z

django/contrib/admin/options.py

+            return flatten_fieldsets(self.declared_fieldsets)
+        else:
+            return [field.name for field in self.opts.local_fields] + \
+                [field.name for field in self.opts.local_many_to_many]


This code ignores the value of readonly_fields. I noticed this because it breaks an inline with a field which is defined as a method in the inline itself (and, thus, needs to be in readonly_fields). I'm currently using this workaround (there might be a more clever way to solve it):

return [field.name for field in self.opts.local_fields] + \ [field.name for field in self.opts.local_many_to_many] + \ list(self.get_readonly_fields(request, obj))

PetrDlouhy mentioned this pull request Sep 16, 2015

Fixed #8936 -- Added view permissions and a read-only admin #5108

Closed

PetrDlouhy force-pushed the view_permission_master branch 3 times, most recently from 97dcdf7 to 02488b3 Compare September 17, 2015 08:59

PetrDlouhy mentioned this pull request Sep 17, 2015

Fixed #8936 -- Added view permissions and a read-only admin #5296

Closed

PetrDlouhy force-pushed the view_permission_master branch 2 times, most recently from df3fe4b to a156071 Compare September 17, 2015 10:36

PetrDlouhy force-pushed the view_permission_master branch 2 times, most recently from 059ea3e to eae9bdb Compare September 17, 2015 15:37

timgraham reviewed Oct 22, 2015
View reviewed changes

PetrDlouhy force-pushed the view_permission_master branch 2 times, most recently from fbbe929 to 9032cbf Compare November 19, 2015 12:35

PetrDlouhy force-pushed the view_permission_master branch from 69b1156 to d1bdabc Compare November 25, 2015 23:01

PetrDlouhy force-pushed the view_permission_master branch from 6e2f007 to 48f0c84 Compare November 25, 2015 23:39

Fixed django#8936 -- Added view permissions and a read-only admin

d4129c6

PetrDlouhy force-pushed the view_permission_master branch from 1b2b445 to d4129c6 Compare November 27, 2015 16:55

miraculixx mentioned this pull request Mar 25, 2016

Need a much safer default: DjangoAuthorization should not allow read access to all model object django-tastypie/django-tastypie#1407

Closed

olivierdalang mentioned this pull request Jun 6, 2016

Fixed #8936 -- Added view permissions and a read-only admin [rebased] #6734

Merged

olivierdalang reviewed Jun 17, 2016
View reviewed changes

timgraham closed this Oct 6, 2016

Tomfox91 reviewed Jan 21, 2017

View reviewed changes

Uh oh!

Fixed #8936 -- Added view permissions and a read-only admin #5297

Fixed #8936 -- Added view permissions and a read-only admin #5297

Uh oh!

Conversation

PetrDlouhy commented Sep 16, 2015

Uh oh!

PetrDlouhy commented Sep 16, 2015

Uh oh!

PetrDlouhy commented Sep 16, 2015

Uh oh!

collinanderson commented Sep 16, 2015

Uh oh!

PetrDlouhy commented Sep 16, 2015

Uh oh!

PetrDlouhy commented Sep 17, 2015

Uh oh!

PetrDlouhy commented Sep 17, 2015

Uh oh!

timgraham Oct 22, 2015

Choose a reason for hiding this comment

Uh oh!

timgraham commented Oct 24, 2015

Uh oh!

PetrDlouhy commented Oct 24, 2015

Uh oh!

timgraham commented Oct 24, 2015

Uh oh!

PetrDlouhy commented Nov 24, 2015

Uh oh!

PetrDlouhy commented Nov 25, 2015

Uh oh!

timgraham commented Nov 25, 2015

Uh oh!

PetrDlouhy commented Nov 26, 2015

Uh oh!

PetrDlouhy commented Jan 25, 2016

Uh oh!

timgraham commented Jan 25, 2016

Uh oh!

PetrDlouhy commented Jan 27, 2016

Uh oh!

SafetyDog commented Mar 3, 2016

Uh oh!

olivierdalang commented Apr 11, 2016

Uh oh!

olivierdalang commented May 29, 2016

Uh oh!

olivierdalang commented Jun 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

collinanderson commented Jun 6, 2016

Uh oh!

olivierdalang commented Jun 6, 2016

Uh oh!

olivierdalang Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

PetrDlouhy Jun 18, 2016

Choose a reason for hiding this comment

Uh oh!

olivierdalang Jun 18, 2016

Choose a reason for hiding this comment

Uh oh!

PetrDlouhy Jun 19, 2016

Choose a reason for hiding this comment

Uh oh!

abusquets commented Jul 20, 2016

Uh oh!

timgraham commented Oct 6, 2016

Uh oh!

Tomfox91 Jan 21, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

olivierdalang commented Jun 3, 2016 •

edited

Loading