Respect `can_read_model` in DjangoModelPermissions #6325

paultiplady · 2018-11-18T20:35:28Z

Django version 2.1 introduced the can_read_model permission
to support read-only ModelAdmin views. Add support for this
permission to a DjangoModelPermissions subclass.

(A subclass is created in order to preserve
backwards-compatibility with versions of Django that don't
support this flag).

FIXES: #6324

TODO: design, tests.

paultiplady · 2018-11-18T20:41:10Z

Two design options:

Break backwards compatibility by updating the DjangoModelPermissions GET check to require the new permission. This has the advantage of being more secure by default, but the major disadvantage of breaking backwards compatibility.
Create a new subclass with the stricter permissions check. Advantage is it maintains compatibility, disadvantage is more auth classes to document/test/maintain.

As a user that expected the default behaviour to be to not grant read access to all users, I prefer 1, but I'm not sure how painful you consider a back-incompatible change to be.

tomchristie · 2018-11-19T10:57:36Z

I'd suggest (1) but only merging it with the next major version change.

Django version 2.1 introduced the `can_read_model` permission to support read-only ModelAdmin views. Add support for this permission to a DjangoModelPermissions subclass. (A subclass is created in order to preserve backwards-compatibility with versions of Django that don't support this flag).

paultiplady · 2018-11-19T17:06:41Z

Sounds good. I've updated the code with this approach. The tests are currently failing because this permission doesn't exist on older versions by default; wonder what the approach should be for testing this? Is there precedent elsewhere that I could compare/copy?

Might be able to do something like adding the view permission only in the new has_view_permission test, and skipping these tests if version < nextversion?

rpkilby · 2018-11-19T22:42:42Z

We plan on dropping Django 1.11 in April when the 2.2 LTS release is cut. This also coincides with the EOL for Django 2.0, meaning that the version differences won't matter since 2.1 will be the minimum supported version.

Basically, this PR will need to hang around until we're ready to prepare the next major DRF release.

rpkilby

This will also need updated docs.

rpkilby

Blocked until we drop Django 1.11 & 2.0 in ~April.

paultiplady · 2018-11-19T23:54:17Z

Ah gotcha, that makes sense. Will hang tight on this changeset then.

gregschmit · 2018-12-16T00:14:20Z

This is a welcome change.

rpkilby · 2019-07-01T18:06:30Z

Moving this to 3.11, since we're not dropping Django 1.11 support until at least December.

idoash4 · 2019-12-31T22:02:43Z

I am using this change in my project(by overriding DjangoObjectPermissions and setting the .perms_map property).
It is working as intended when listing objects using ModelViewSet. However when trying to retrieve a single item from the same view I am getting "detail": "Not found." error. This doesn't happen when the user making the request is a superuser. Adding all permission to the non-superuser testing user hasn't solved this as well.
@paultiplady have you tested this case?
I have spent a few hours on it and can't find anything wrong with my environment.

marcosox · 2020-07-03T15:07:12Z

Hello, any update on the status of this feature?

namoshizun · 2020-08-11T08:53:30Z

Hoping to see progress on this MR... I am already using it in my code but still would be nice to see it goes into the DRF codebase.

emmceemoore · 2020-08-13T04:26:20Z

rest_framework/permissions.py

@@ -151,7 +151,7 @@ class DjangoModelPermissions(BasePermission):
    # Override this if you need to also provide 'view' permissions,
    # or if you want to provide custom permission codes.
    perms_map = {
-        'GET': [],
+        'GET': ['%(app_label)s.view_%(model_name)s'],
        'OPTIONS': [],
        'HEAD': [],


I think the view_* permission check should be applied to HEAD requests as well.

Sure, the body isn't returned but having access to content-length could leak information about the page. Also, having GET and HEAD return different status codes seems weird to me.

Agreed. We'd want the same permission on HEAD requests, yup.

Also the "Override this if you need to also provide 'view' permissions" part of the comment can be dropped.

nemesifier · 2021-05-12T21:37:31Z

Hey there, can anyone give an update on what is missing from this PR so that some of us who are using DRF in @openwisp may help out in case it's doable for us? We will implement our own solution for this anyway so maybe it's worth to help here instead.

tomchristie · 2021-05-13T14:39:00Z

@nemesisdesign Have added a comment above on a small tweak that'd be needed. Happy to take that either on this PR, on against a new one.

nemesifier · 2021-05-14T16:01:36Z

@nemesisdesign Have added a comment above on a small tweak that'd be needed. Happy to take that either on this PR, on against a new one.

Cool, what about the OPTIONS method?

namoshizun · 2021-05-18T05:16:56Z

It is indeed a very useful addition.... hope it gets merged into the next release. We have been using this implementation for a long while, but it feels awkward each time when I have to explain why DRF doesn't include this solution..

nemesifier

I guess that if a user has the change permissions but not the view permissions, they should still be able to view, for backward compatibility reasons, since it's not guaranteed that old apps have view permissions created and assigned to users.

ManishShah120 · 2021-05-26T12:27:41Z

I am using this change in my project(by overriding DjangoObjectPermissions and setting the .perms_map property).
It is working as intended when listing objects using ModelViewSet. However when trying to retrieve a single item from the same view I am getting "detail": "Not found." error. This doesn't happen when the user making the request is a superuser. Adding all permission to the non-superuser testing user hasn't solved this as well.
@paultiplady have you tested this case?
I have spent a few hours on it and can't find anything wrong with my environment.

Hi, @BlackXnt Yes there's an issue with this PR, I have opened #8009 to deal with this case, you can do a similar thing and see if it resolves your problem. 👍

nemesifier · 2021-06-01T16:32:39Z

#8009 is based on work we're doing in OpenWISP: openwisp/openwisp-users#251. Would be great to see this handled directly in DRF. Let us know if it needs improvement. We'll put effort in helping out on this (me, @ManishShah120 and @atb00ker).

stale · 2022-04-16T22:57:08Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

auvipy

closing in favor of #6325

paultiplady force-pushed the django-permissions-read-only branch 3 times, most recently from 8bd001d to e879e8c Compare November 19, 2018 17:00

paultiplady force-pushed the django-permissions-read-only branch from e879e8c to b085754 Compare November 19, 2018 17:02

rpkilby suggested changes Nov 19, 2018

View reviewed changes

carltongibson added this to the 3.10 Release milestone Dec 19, 2018

rpkilby modified the milestones: 3.10 Release, 3.11 Release Jul 1, 2019

rpkilby mentioned this pull request Jul 18, 2019

DjangoObjectPermissions behaviour #6596

Closed

rpkilby modified the milestones: 3.11 Release, 3.12 Release Dec 6, 2019

emmceemoore reviewed Aug 13, 2020

View reviewed changes

tomchristie modified the milestones: 3.12 Release, 3.13 Release Sep 28, 2020

atb00ker mentioned this pull request May 15, 2021

[API][config] Functional testing current status openwisp/openwisp-controller#459

Closed

nemesifier reviewed May 20, 2021

View reviewed changes

ManishShah120 mentioned this pull request May 26, 2021

Fix Respect can_read_model permission in DjangoModelPermissions #8009

Merged

tomchristie modified the milestones: 3.13 Release, 3.14 Jan 10, 2022

stale bot added the stale label Apr 16, 2022

auvipy removed the stale label Jan 12, 2023

auvipy suggested changes Jan 12, 2023

View reviewed changes

auvipy removed this from the 3.15 milestone Jan 12, 2023

auvipy closed this Jan 12, 2023

Uh oh!

Respect can_read_model in DjangoModelPermissions #6325

Respect can_read_model in DjangoModelPermissions #6325

Uh oh!

Conversation

paultiplady commented Nov 18, 2018

Uh oh!

paultiplady commented Nov 18, 2018

Uh oh!

tomchristie commented Nov 19, 2018

Uh oh!

paultiplady commented Nov 19, 2018

Uh oh!

rpkilby commented Nov 19, 2018

Uh oh!

rpkilby left a comment

Choose a reason for hiding this comment

Uh oh!

rpkilby left a comment

Choose a reason for hiding this comment

Uh oh!

paultiplady commented Nov 19, 2018

Uh oh!

gregschmit commented Dec 16, 2018

Uh oh!

rpkilby commented Jul 1, 2019

Uh oh!

idoash4 commented Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcosox commented Jul 3, 2020

Uh oh!

namoshizun commented Aug 11, 2020

Uh oh!

emmceemoore Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

tomchristie May 13, 2021

Choose a reason for hiding this comment

Uh oh!

nemesifier commented May 12, 2021

Uh oh!

tomchristie commented May 13, 2021

Uh oh!

nemesifier commented May 14, 2021

Uh oh!

namoshizun commented May 18, 2021

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

ManishShah120 commented May 26, 2021

Uh oh!

nemesifier commented Jun 1, 2021

Uh oh!

stale bot commented Apr 16, 2022

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Respect `can_read_model` in DjangoModelPermissions #6325

Respect `can_read_model` in DjangoModelPermissions #6325

idoash4 commented Dec 31, 2019 •

edited

Loading