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

olivierdalang · 2016-06-06T16:05:04Z

Hi,

This is just a rebase of #5297
The only difference (besides fixing the merge conflicts) is that ModelAdmin.declared_fieldsets doesn't exist anymore and was replaced by get_fieldsets.

Thanks,

Olivier

auvipy · 2016-06-07T11:29:26Z

docs/ref/contrib/admin/index.txt

timgraham · 2016-09-15T15:59:28Z

The test failures are:
admin_views.tests.UserAdminTest.test_user_fk_change_popup
admin_views.tests.UserAdminTest.test_user_fk_delete_popup

timgraham · 2016-09-16T21:59:49Z

Did you fix #5297 (comment)?

olivierdalang · 2016-10-03T15:29:04Z

Hi,

It seems these tests fail because of a newline in the template (to comply with flake) :

self.assertContains(response, 'class="related-widget-wrapper-link change-related" id="change_id_owner"')

AssertionError: False is not true : Couldn't find 'class="related-widget-wrapper-link change-related" id="change_id_owner"' in response

while this appears in the template :

        <a class="related-widget-wrapper-link change-related"
            id="change_id_owner"

An easy fix is to add the line break in the test, but this will fail again if the template gets changed. I see there's the html parameter to assertContains, but it doesn't seem to work with partial match (the test only tests for the class and the id attribute, it doesn't match since there are also a data-href-template and a title html attributes.

timgraham · 2016-10-03T15:54:04Z

django/contrib/admin/templates/admin/related_widget_wrapper.html

    {% block links %}
-        {% if can_change_related %}
-        <a class="related-widget-wrapper-link change-related" id="change_id_{{ name }}"
+        {% if can_change_related or can_view_related%}


missing space before "%}"

timgraham · 2016-10-03T15:54:56Z

I don't have a strong opinion about how to fix that.

olivierdalang · 2016-10-04T14:50:02Z

Hi,

I made the changes :

missing space & flake8
failing tests (I just added the line break in the tested string, it's not very elegant but I don't see how to do it without making the test case too complex)
Fixed #8936 -- Added view permissions and a read-only admin #5297 (comment) including another edge case when the inline has view and delete permissions

I intend to squash those latter commits if they look good to you.

I suggest closing #5297 to avoid having comments at different places.

I removed the patch needs improvement flag on the ticket. Let me know if I can do anything more for this !

olivierdalang · 2016-10-13T21:07:14Z

There's a failing build, but it looks like it's travis fault : FATAL: Could not checkout ea3c082f7160685ab0c8215745dd191e0a85424c
Any way to restart the tests ?

timgraham · 2016-10-13T21:10:26Z

Rebase so the patch merges cleanly and squash commits (in general, there's no need to push incremental commits for minor changes; just squash right away). Of course with a patch of this size, after someday reviews the patch the first time , it might be useful to push incremental changes so they don't have to recheck the entire patch again.

yigitguler · 2016-11-05T11:48:28Z

This will be a nice addition to the Django Admin. Can't wait to see it!

olivierdalang · 2016-11-07T13:41:29Z

Ok I think the PR is ready !

slurms · 2016-11-16T17:37:52Z

We've been testing this in our own admin site and found a couple of things:

using prepopulated_fields with view only permissions fails (tries to access a field which doesn't exist on the form due to excludes).
using custom admin forms with form fields that only exist on the form fails (label_for_field issue).

I'll try and write a couple of failing test cases.

hirokiky · 2016-12-03T02:36:47Z

Hi. It's awesome PR and I tried it with my existing web app and existing database..
My question is how can I migrate from existing database?
As the Options.default_permissions doc said here, old existing apps can't use this new feature.

I tried recreating the database and tried this feature, It worked with my app.

ngnpope · 2016-12-05T09:55:47Z

@hirokiky: I see no reason why old existing apps cannot use this feature - the note states that backwards compatibility is maintained by ensuring that having a change permission also implicitly means that the user has the view permission for the same model. This is to avoid loss of access to pages in the admin site when Django is upgraded because users would not have the new view permissions by default. After upgrade change permissions can be removed and view permissions added as required. (It also rarely makes sense to be able to change something but not view it!)

hirokiky · 2016-12-14T09:06:04Z

@pope1ni Yes. Admin pages can be seen for existing users.

But with migrated DB, I can't apply Can view <model> for users. there isn't any Can view <model> permissions at change users page on admin.
The reason is that there aren't any Can view <model> permission on Permission model.

anyway thank you. my comment was not enough.

carltongibson

Hi @olivierdalang.

A couple of issues from more complex manual testing of the admin:

When you have no permissions at all, the Admin homepage says:

You don't have permission to edit anything.

Arguably, this should be updated to say "view or edit" or such.

More serious: there is an issue with TabularInline.

If my user does not have the add permission for the inline model, TabularInline leads to an error when rendering the empty form. (KeyError on missing field.)

This works as expected with StackedInline. (The empty form simply isn't rendered.)
It's a regression from master, where TabularInline renders (as expected) without the empty form when user does not have the add permission.

Other than that, it's looking really good: combinations of permissions for different models in a inline etc all seem to behave as expected. Good stuff!

carltongibson

Hi @olivierdalang.

Right, final pass. Thank you again for your effort on this. (I'm exhausted. 😵)

Manual testing is great.
The non-test changes look fine to me.
Maybe others have a comment or two?

I pushed a couple of small edits. Please check. We can squash at the end.

I've left comments on the test cases to be addressed.

To highlight one, it's important that the case for the view only user is added to test_add_view and test_delete_view.
In each of those cases we check that the other permissions don't allow the action, and just as you've added it to test_change_view we need it in those other places.

Beyond that there's two possible refactorings:

Pull out the tests for the redirect behaviour on row level permissions.
The above mentioned changes in admin_changelist/tests.py (to create the authenticated request etc.)

Neither of these are blockers for this PR. (They can go in separate commits in this PR or in separate PRs, if at all.)

Beyond that there remains:

The release note changes as per the discussion above. (Nick is 100% right that you might have an existing view permission that might now allow an access to the admin.)
The crash using TabularInline without the add permission.

If we can get all this resolved I'll feel happy asking Tim to have a look.

Good stuff! 👍

carltongibson · 2018-04-25T09:05:55Z

tests/admin_views/tests.py

Please use """ for docstrings.

carltongibson · 2018-04-25T09:14:30Z

tests/admin_views/tests.py

I see the old tests do this, but (AFAICS) there's no reason to to store post_data on self. (It's not accessed outside this test case.)

The same post_data is used across other tests that use the same change form. If this is OK for you I'll leave it like this (more consistent with other tests)

carltongibson · 2018-04-25T09:15:04Z

tests/admin_views/tests.py

.all()[0] -> .first()

carltongibson · 2018-04-25T09:17:31Z

tests/admin_views/tests.py

What's this block testing? We just added the Widget. We don't have change permission so we can't update it. (That's testing in the block below) So what's the purpose of this POST?

Is it worth adding an assert against the location for the 302 responses?

Indeed this doesn't test much... I can't see why it's there neither. I'll remove it.

I think it's worth testing the 302 response, as it may be that the query is successful, but redirects to a wrong page (e.g. if you don't have view permission and were redirected to change page, you'd get a 403)

carltongibson · 2018-04-25T09:21:21Z

tests/admin_views/tests.py

No need to wrap this. 118 chars is fine.

carltongibson · 2018-04-25T10:22:59Z

tests/admin_views/tests.py

This can be part of test_login, directly above.

carltongibson · 2018-04-25T13:11:01Z

tests/admin_views/admin.py

As per comments on test_change_view and test_history_view, I don't think this is really behaving in the expected way, or really testing the desired behaviour.

(TODO: pull out the row-level permissions parts of those tests in a separate commit, and then this can be skipped.)

You mean a separate commit but still in the PR ? (or take it out of the PR altogether and leave a TODO note in the code ?)

carltongibson · 2018-04-25T13:14:12Z

tests/admin_filters/tests.py

alfred is part of the data set. I think a separate self.superuser would be more consistent with the other tests here.

(Otherwise we see request.user = self.alfred in each method and wonder "Who's alfred?")

Ok I'll do that, but this induce some changes in the expected results from the queries in the tests (the commit may be a bit confusing because of that). Let me know if you think it's worth the change.

carltongibson · 2018-04-25T13:27:10Z

django/contrib/admin/templatetags/admin_modify.py

This looks OK on manual testing.

I have a user with add and view permissions on X

I add an X, using the "Save and continue" button.

I'm redirected to the same page to view the object. Only option is "Close without saving".

carltongibson · 2018-04-25T13:32:01Z

django/contrib/admin/templatetags/admin_modify.py

Except the message displayed isn't quite right in this case:

The X "Great new X" was added successfully. You may edit it again below.

It's not true that it may be edited.

ngnpope

I've had another pass through this and picked up a number of issue by inspection of the code.

ngnpope · 2018-04-25T19:17:39Z

django/contrib/admin/helpers.py

Can we not just extend self.readonly_fields and not add the extra self.viewonly_fields?

Also, this line will likely result in duplicate column names - I'm not sure that necessarily matters here, but it's not very tidy...

We can't just extend it, because they are different depending on if we are in extra rows (with the add permission, where unless a field is explicitly readonly, you can still change it) or if we are in existing rows (where all fields are readonly if we don't have the change permission).

Still that's not very clear in the code isn't very clear and I'll refactor so it's more readable.

ngnpope · 2018-04-25T19:27:28Z

django/contrib/admin/options.py

Minor rephrasing: ... and the user does not have permission ...

Also, typo: exlcude → exclude

ngnpope · 2018-04-25T19:30:36Z

django/contrib/admin/options.py

It is not necessary to pass None here.

I think the comment is obsolete as the code is descriptive enough and this should probably just be rolled into the above if statement.

ngnpope · 2018-04-25T19:37:29Z

django/contrib/admin/options.py

Rename this variable to readonly_fields. Let's not pollute the code with three things that mean the same thing, i.e. the uneditable_fields here and the aforementioned viewonly_fields.

ngnpope · 2018-04-25T19:45:20Z

django/contrib/admin/options.py

Again, it shouldn't be necessary to provide None here? (I realise that the existing version of this line did so.)

Please fix all cases of this, but only for lines modified by this PR. Other cases can be dealt with by a separate PR some other time.

ngnpope · 2018-04-25T20:06:37Z

django/contrib/admin/static/admin/css/forms.css

Why is this added?

Did you intend to remove line-height (and other) properties with the same values from later rules?

It's just so that the close button (which is an unlike other buttons that are ) displays the same way than the rest of the buttons

You where right, it wasn't really needed... anyways I just refactored the css a little bit

ngnpope · 2018-04-25T20:11:40Z

django/contrib/admin/templates/admin/submit_line.html

Will this render as "Close without saving" if we only have view permissions and this cannot save anyway?

Yes, I think it's better to be clear that there's no saving going on despite clicking a button that's located where the close buttons usually are.

I think it stays quite clear for the user, because that button only appears if there's no editable form on the page anyways.

ngnpope · 2018-04-25T20:28:10Z

django/contrib/admin/templatetags/admin_modify.py

Thinking about this again, my point was that "Save and continue" doesn't make sense if you only have the add permission without view or change also.

Before you had to have the change permission. Now it ought to be change or view, but has been changed to use can_save which allows just having the add permission.

This makes no sense. Either we then have to redirect somewhere else other than the change/view view making the behaviour different, i.e. the same as the "Save" button, or we redirect and get a permission denied error.

I haven't tested these scenarios, but just by inspection of the code, it is clear we are changing the expected behaviour here.

ngnpope · 2018-04-25T20:32:54Z

tests/admin_changelist/test_date_hierarchy.py

Please rephrase the comment to avoid personal pronouns.

About the save and continue: I'll change it so that it only appears when the user can change, and to reword to "Save and view" if the user can only view (good catch !)

ngnpope · 2018-04-25T20:36:20Z

tests/admin_views/admin.py

The parentheses on this line are unnecessary.

Ok (that was copied from just above, I'm never sure if it's better to keep resemblance with existing code for consistency or do things right)

olivierdalang · 2018-04-25T21:40:44Z

Thanks for the detailed review !!

I'll try to find some time next week to deal with them.

olivierdalang · 2018-04-26T05:59:49Z

I made some progress on this.

I still need to address the refactoring of the tests cases, and to address #6734 (comment) but for the rest, it should be good.

I hope I didn't miss any comment, this whole PR is getting quite confusing :-)

I left everything as separated commits to facilitate review but of course will squash.

Thanks again for your excellent inputs !!

carltongibson · 2018-04-26T07:27:14Z

Hi @olivierdalang. Thanks for the effort.

... this whole PR is getting quite confusing :-)

Yes. And there are so many comments now that it's taking (me at least) multiple refreshes to load.
(Maybe we can move to one final PR once the current batch of comments are addressed.)

It looks to me as if there are a number of comments not yet addressed. Please take the time to make sure each one is handled. (I don't think we're far off now. 💃🏼)

olivierdalang · 2018-04-27T01:59:30Z

Another round done... I hope I didn't miss anything.

Those may need some additional review (not always sure of the amendments, mostly about test case style) :
#6734 (comment)
#6734 (comment)
#6734 (comment)
#6734 (comment)
#6734 (comment)

Hehe I'll make sure to have a bottle of champagne ready for when this gets merged (if it gets merged) !!

carltongibson · 2018-04-27T07:30:46Z

Hi @olivierdalang.

Thanks for the super effort here!

...when this gets merged (if it gets merged) !!

It should get merged. It works. And it's been a long-time on the wish list. We just have to cover all the bases.

Thank you for all your edits. I'd gone through. It looks like you've addressed each point. As such I've squashed and rebased the PR.

Of the outstanding points:

Don't worry about the refactorings, e.g. post_data vs self.post_data and the row-level permissions tests. We can address those separately. (They're not directly relevant.)
You're correct, the history_view is read-only, so removing the comment about changes is correct.
Given that it affected the query sets elsewhere, I reverted the alfred change. We'll keep using alfred, and the comment there.

@pope1ni: I looked at the add only permission case. Here from the create form, the instance is created and then you're redirected back to the app_index, where you can chose the Add button again. @olivierdalang updated the button text so I think your query is resolved. Can you confirm?

olivierdalang · 2018-04-30T21:44:15Z

@carltongibson @pope1ni
Thanks again for your involvement in this. Should I squash the commits already ?
Let me know if there's anything else I can do...

carltongibson · 2018-05-02T07:00:25Z

Hi @olivierdalang. Sorry for the delay: I was away.

I squashed and rebased last week, so nothing to do there.

I've marked this as Ready for checkin on the ticket. Tim will have a look next: it's a big patch, with potential security issues, so it justifies another review.

Tim, with fresh eyes, will likely make a few more suggestions. I think we've gone over the main scenarios well enough (POSTing data, accessing inlines etc) so hopefully these will just be minor. On that assumption we should be able to get it in. 💃🏼 (But lets await Tim's review.)

Thanks for your super effort here. This has been a long time on the wish-list!

olivierdalang · 2018-05-02T08:51:55Z

Great ! I just redid the commit so that it's properly attributed to @PetrDlouhy (original author) and me.
I see there are now failing tests, it looks like a git related glitch because there's no change in the code.

mahadi · 2018-05-14T17:51:46Z

Will this get accepted for the feature freeze (which seems to be today)?

collinanderson · 2018-05-15T16:49:17Z

Hi All. I tried this out and it seems to work as expected for me.

One point of feedback, feel free to ignore if you want: I think the button "Close without saving" is a little confusing. It's not like you would ever have any changes that will get discarded, right? Maybe we could say "back to list page" (or have no button at all)?

In any case. This is awesome. Thanks so much!

timgraham · 2018-05-15T16:52:37Z

I changed that to say "Close" in my edits that I haven't pushed yet.

…

Co-authored-by: Petr Dlouhy <[email protected]> Co-authored-by: Olivier Dalang <[email protected]>

olivierdalang · 2018-05-16T22:58:14Z

Yeaaah !!! Finally :D

Thank you all !!!

timgraham · 2018-05-16T23:06:32Z

Do you have time to add additional tests as described on the ticket?

…

On Wed, May 16, 2018 at 6:59 PM olivierdalang ***@***.***> wrote: Yeaaah !!! Finally :D Thank you all !!! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#6734 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZI3UoXP9AwxjAWfeK46a5dxmTR558-ks5tzK88gaJpZM4IvDfv> .

olivierdalang mentioned this pull request Jun 6, 2016

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

Closed

olivierdalang force-pushed the view_permission_master branch from 3d73f7a to 080b79b Compare June 7, 2016 10:54

auvipy reviewed Jun 7, 2016
View reviewed changes

docs/ref/contrib/admin/index.txt Outdated

Copy link

Contributor

auvipy Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.11

olivierdalang force-pushed the view_permission_master branch 2 times, most recently from 0069d56 to efcefb1 Compare June 7, 2016 12:05

olivierdalang force-pushed the view_permission_master branch 2 times, most recently from d3a2d76 to 52bbd3d Compare June 17, 2016 02:04

olivierdalang force-pushed the view_permission_master branch from 52bbd3d to f57acae Compare August 5, 2016 11:17

timgraham reviewed Oct 3, 2016

View reviewed changes

olivierdalang force-pushed the view_permission_master branch from f57acae to f6a70e5 Compare October 3, 2016 17:46

PetrDlouhy mentioned this pull request Oct 19, 2016

Update README.rst PetrDlouhy/django-view-permission#1

Merged

olivierdalang force-pushed the view_permission_master branch 4 times, most recently from 578a3de to 40b0d3c Compare November 7, 2016 13:12

carltongibson reviewed Apr 25, 2018

View reviewed changes

ngnpope suggested changes Apr 25, 2018

View reviewed changes

olivierdalang force-pushed the view_permission_master branch from 3ee7828 to 8211bc3 Compare April 27, 2018 01:52

olivierdalang force-pushed the view_permission_master branch from 8211bc3 to ab56fa3 Compare April 27, 2018 02:22

carltongibson force-pushed the view_permission_master branch 2 times, most recently from 5159d29 to 3d2e98d Compare April 27, 2018 07:23

olivierdalang force-pushed the view_permission_master branch from 3d2e98d to 326a76b Compare May 2, 2018 08:44

timgraham force-pushed the view_permission_master branch 3 times, most recently from d55c15d to 54ba557 Compare May 14, 2018 13:45

timgraham force-pushed the view_permission_master branch 2 times, most recently from 3ff0d40 to 1869ca2 Compare May 16, 2018 01:19

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

825f0be

Co-authored-by: Petr Dlouhy <[email protected]> Co-authored-by: Olivier Dalang <[email protected]>

timgraham force-pushed the view_permission_master branch from 1869ca2 to 825f0be Compare May 16, 2018 12:13

timgraham merged commit 825f0be into django:master May 16, 2018

peteruithoven mentioned this pull request Jul 12, 2019

Read only admin views VOKO-Utrecht/voko#75

Open

Uh oh!

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

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

Uh oh!

Conversation

olivierdalang commented Jun 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Sep 15, 2016

Uh oh!

timgraham commented Sep 16, 2016

Uh oh!

olivierdalang commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Oct 3, 2016

Uh oh!

olivierdalang commented Oct 4, 2016

Uh oh!

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

Uh oh!

Uh oh!

timgraham commented Oct 13, 2016

Uh oh!

yigitguler commented Nov 5, 2016

Uh oh!

olivierdalang commented Nov 7, 2016

Uh oh!

slurms commented Nov 16, 2016

Uh oh!

hirokiky commented Dec 3, 2016

Uh oh!

ngnpope commented Dec 5, 2016

Uh oh!

hirokiky commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

olivierdalang commented Oct 13, 2016 •

edited

Loading

hirokiky commented Dec 14, 2016 •

edited

Loading