Fix Respect can_read_model permission in DjangoModelPermissions
#8009
Conversation
|
Hey @tomchristie interested in your opinion on this implementation! 😄 |
There was a problem hiding this comment.
I think we'll need to update the docs here as well: https://github.com/encode/django-rest-framework/blob/master/docs/api-guide/permissions.md#djangomodelpermissions.
That section now suggests adding view permissions, but I think we should say view permissions are supported and any user having either view or change permissions on a model will be able to view.
rest_framework/permissions.py
Outdated
|
|
||
| user = request.user | ||
| if request.method == 'GET': | ||
| if user.has_perms(perms) or user.has_perms(change_perm): |
There was a problem hiding this comment.
@ManishShah120 please update this block as we did in openwisp/openwisp-users#251
There was a problem hiding this comment.
Updated the docs and simplified the code.
I stressed that we maintain backward compatibility since the view permission is a relatively new thing (although has been around for a couple of years now). If old apps are not using view permissions, change permission will be used to determine if a user they can view. Examples: User with change permission but not view permission: can view and change @tomchristie I hope this clarifies. |
|
@tomchristie ping 🖖 |
|
BTW we are using this code in https://github.com/openwisp/openwisp-users/#djangomodelpermissions. Just to clarify that we are using this code and is not just some random untested code we're sending here 😊. |
|
Why this hasn't it been approved and merged yet? This issue has been opened for so long. |
@hericlesbitencourt I think if we get more people to test and use it the maintainers will be more confident in merging. |
actually the main maintainer lost his motivation to work on this project |
|
I think this PR is very useful and it is very necessary to be merged. |
|
It is also recommended to add a There will be no compatibility issues, because users of the old version can easily replace. In this way, three permissions are exist in total: |
|
|
||
| user = request.user | ||
| if request.method == 'GET': | ||
| return user.has_perms(perms) or user.has_perms(change_perm) |
There was a problem hiding this comment.
I don't see the motivation for this additional set of checks against view_* or change_* permissions.
Just seems inconsistent to me.
There was a problem hiding this comment.
This maintains backward compatibility Tom. If the DB was not updated to grant the users of an application view permissions, the API endpoints will not allow read operations and will break because users have no view permissions assigned, only change permissions which was how django worked before, we can't be sure all databases have been updated to use view permissions properly.
If we only check view permissions, that's how we would introduce a backward incompatible change.
Thanks man, indeed is highly necessary. When I was learning for the first time I took days to understand why a simple "get" wouldn't work with DjangoModelPermissions, for me makes no sense. |
| 'OPTIONS': [], | ||
| 'HEAD': [], | ||
| 'HEAD': ['%(app_label)s.view_%(model_name)s'], |
There was a problem hiding this comment.
Presumably we'd also want to make the same change on DjangoObjectPermissions, right?
There was a problem hiding this comment.
I honestly don't know, if you say this should be done we'll look into it, but we should clear any doubt regarding https://github.com/encode/django-rest-framework/pull/8009/files/dd4de8e420f3fe6e069cfb176a3e64218a10eda0#r709556976.
|
So the biggest thing here is the impact of behaviour changes in the project at this stage in its lifetime. Do we support Should we switch to requiring them by default? Well... possibly. It'll be more in line with what some users will want by default, but it'll also absolutely 100% undoubtedly break other projects that upgrade. Is the cost of breakage in exchange for switching the behaviour around a good trade-off? Maybe. Maybe not. Personally I'm almost always pushing back on almost any behavioural changes in REST framework at this point, because the negative impact the changes cause to upgrading projects probably doesn't seem worth it. Perhaps we ought to make an exception here, because "view" permissions have been around in Django for quite some time now (they didn't exist at the time we first introduced this permission class), but if I'm hesitant, it's because there's very good reasons to default to "leave things as they are" at this point. |
|
Having said all that, I do still think that this particular case is potentially worthy of a behavioural change, so long as we document it loudly enough. It's just that it's really not an easy trade-off to choose at this point. |
|
This should definitely be merged imo. In fact it should have been this way from the moment this was added to Django. Having a mode to use Django permissions that actually ignores half of Django permissions is a major flaw. Even though it is technically breaking, it is a safe change in the sense that, from a security standpoint, it's more restrictive than before (it won't give access to things that didn't have access before). |
|
I don't understand how this solution can break anything really because it has been thought with backward compatibility in mind. I think that whoever didn't implement anything custom with view permissions will get the same behavior as without this patch. |
|
When this issue will be merged? |
|
If I understand correctly, this will be a problem specifically for people who are using DjangoModelPermissions with users who are supposed to have read-only permissions, but don't have view permissions because it defaults to viewable. The special cases, such as making the detail endpoint viewable without the view permission, aren't very intuitive, but are unlikely to cause problems. I'm not sure why this patch is preferable to adding a new Django2ModelPermissions class that respects view permissions completely, without any backwards compatibility considerations or special cases. Anyone who needs backwards compatibility can continue to use DjangoModelPermissions and get the exact same behavior. Anyone who wants to switch can--without worrying about special cases. |
|
I think this should be added. I understand the concerns about backward compatibility, but I think backward compatibility is only desirable not mandatory. Could this break some projects that upgrade? Probably, but the good practice that all of us should use in our projects is: use fixed package versions, and only upgrade them once we have tested in a development environment ensuring that it won't break anything. Anyways, it's 2022 and this feature was firstly proposed in 2018. Maybe it's time for make a decission:
I like the propose of @kxbin of having For the 2nd option the name |
Sounds good, the alternative could be the opposite, implement a backward incompatible |
|
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. |
|
👀 |
|
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. |
|
i would love to see the minor conflicts resolved in the doc |
Sure, working on it. |
ManishShah120
force-pushed
the
fix-read-model-permission
branch
from
dd4de8e
to
6d4d048
Compare
|
I am going to accept this. as it is mostly a django compatibilty issue and will be released in a major version. if we need additional fixes we can handle those in another PR |
DRF verson 3.15 Fix Respect can_read_model permission in DjangoModelPermissions. encode/django-rest-framework#8009
DRF verson 3.15 Fix Respect can_read_model permission in DjangoModelPermissions. encode/django-rest-framework#8009
DRF verson 3.15 Fix Respect can_read_model permission in DjangoModelPermissions. encode/django-rest-framework#8009
DRF verson 3.15 Fix Respect can_read_model permission in DjangoModelPermissions. encode/django-rest-framework#8009
DRF verson 3.15 Fix Respect can_read_model permission in DjangoModelPermissions. encode/django-rest-framework#8009
DRF verson 3.15 Fix Respect can_read_model permission in DjangoModelPermissions. encode/django-rest-framework#8009
As DRF encode/django-rest-framework#8009 has been reverted in 3.15.1, the related hack is no longer needed.
As DRF encode/django-rest-framework#8009 has been reverted in 3.15.1, the related hack is no longer needed.
As DRF encode/django-rest-framework#8009 has been reverted in 3.15.1, the related hack is no longer needed.
Description
This PR implements the
viewpermission inDjangoModelPemissionsclass. The old PR #6325 tried to fix it, but it contains a flaw i.e., even if an object haschange,add,deletepermission the detail page of any objects returns"detail": "Not found.". which is not expected. As any objects which haschangepermission should also allowGETrequests as well.FIXES: #6324
Ref:-
can_read_modelin DjangoModelPermissions #6325 (comment)can_read_modelin DjangoModelPermissions #6325 (review)