Fix Respect `can_read_model` permission in DjangoModelPermissions

[ManishShah120]

trafficstars

Description

This PR implements the view permission in DjangoModelPemissions class. The old PR #6325 tried to fix it, but it contains a flaw i.e., even if an object has change, add, delete permission the detail page of any objects returns "detail": "Not found.". which is not expected. As any objects which has change permission should also allow GET requests as well.

FIXES: #6324

Ref:-

• https://github.com/encode/django-rest-framework/pull/6325#issuecomment-569997047
• https://github.com/encode/django-rest-framework/pull/6325#pullrequestreview-664894357

[atb00ker]

Hey @tomchristie interested in your opinion on this implementation! 😄

[nemesifier]

Code-wise this seems good yup. All very neat & cleanly done.

Seems like a possible issue with the change in behaviour. I guess there are plenty of existing apps out there that haven't been using "view" permissions, that would break if upgraded to this?

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. This avoids the situation in which a user with change permission cannot view because of the missing view permission (which doesn't make any sense).

Examples:

User with change permission but not view permission: can view and change User with no change permission nor view permission: cannot view nor change User with view permission but no change permission: can view but cannot change

@tomchristie I hope this clarifies.

[nemesifier]

@tomchristie ping :vulcan_salute:

[nemesifier]

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 :blush:.

[hericlesbitencourt]

Why this hasn't it been approved and merged yet? This issue has been opened for so long.

[nemesifier]

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. I think they are afraid of causing bugs and didn't have time to test.

[auvipy]

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. I think they are afraid of causing bugs and didn't have time to test.

actually the main maintainer lost his motivation to work on this project

[kxbin]

I think this PR is very useful and it is very necessary to be merged.

[kxbin]

It is also recommended to add a DjangoModelPerssionsOrReadOnly, which remains compatible with the old version DjangoModelPerssions.

There will be no compatibility issues, because users of the old version can easily replace.

In this way, three permissions are exist in total: DjangoModelPerssions DjangoModelPerssionsOrReadOnly DjangoModelPerssionsOrAnonReadOnly

[hericlesbitencourt]

I think this PR is very useful and it is very necessary to be merged.

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.

[lovelydinosaur]

So the biggest thing here is the impact of behaviour changes in the project at this stage in its lifetime.

Do we support view model permissions? Yup, we currently document what the existing behaviour is, and note that if you want them then you can do so by subclassing DjangoModelPermissions. That's a few lines of code for any project that does want this behaviour.

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.

[lovelydinosaur]

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.

[IgnacioFDM]

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).

[nemesifier]

I don't understand how this solution can break anything really because it has been thought with backward compatibility in mind. @tomchristie do you care about providing a real world example of how this change can break a system so that if there's something we can do now to avoid it we could try?

I think that whoever didn't implement anything custom with view permissions will get the same behavior as without this patch. Those who did implement something to handle view permissions may have some redundancy but unless they implemented it differently (eg: their implementation could grant a user to change an object but not view it, but that doensn't make sense and shouldn't be endorsed) it should not hurt them.

[ahmedalrifai]

When this issue will be merged?

[jonathan-golorry]

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.

[igonro]

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:

1. Add these changes
2. Leave DjangoModelPermissions as it is, and create a new DjangoModelWithViewPermissions (or something like that)

I like the propose of @kxbin of having DjangoModelPermissions, DjangoModelPermissionsOrReadOnly and DjangoModelPermissionsOrAnonReadOnly for the 1st option.

For the 2nd option the name DjangoModelWithViewPermissions feels a bit strange, but I would prefer having a strange name and having this functionality by default. This would be very useful for new people that are starting to use DRF (as it is my case).

[nemesifier]

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:

1. Add these changes
2. Leave DjangoModelPermissions as it is, and create a new DjangoModelWithViewPermissions (or something like that)

I like the propose of @kxbin of having DjangoModelPermissions, DjangoModelPermissionsOrReadOnly and DjangoModelPermissionsOrAnonReadOnly for the 1st option.

For the 2nd option the name DjangoModelWithViewPermissions feels a bit strange, but I would prefer having a strange name and having this functionality by default. This would be very useful for new people that are starting to use DRF (as it is my case).

Sounds good, the alternative could be the opposite, implement a backward incompatible DjangoModelPermissions class but provide DjangoModelPermissionsLegacy with the old behavior.

[stale[bot]]

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.

[igonro]

:eyes:

[stale[bot]]

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]

i would love to see the minor conflicts resolved in the doc

[ManishShah120]

i would love to see the minor conflicts resolved in the doc

Sure, working on it.

[auvipy]

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