Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added view permission checks on DjangoAuthorization #1658

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rensieeee
Copy link

DjangoAuthorization docs are outdated, specifically on the READ_PERM_CODE definition. The link pointing to Django 1.9 no longer exists, but the latest link to Django 4.2 references the following:

https://docs.djangoproject.com/en/4.2/topics/auth/default/#permissions-and-authorization

Access to view objects is limited to users with the “view” or “change” permission for that type of object.

This PR allows GET calls by users with view permissions, but without change permissions. PATCH/PUT calls still require change permissions, and GET calls do still work with change permissions. Should be fully backwards compatible.

@rensieeee
Copy link
Author

rensieeee commented Jun 7, 2023

In the meantime (temporary solution), you can implement something like this:

class CustomAuthorization(DjangoAuthorization):
    READ_PERM_CODE = 'view'

This is not backwards compatible (will restrict access to GET requests with change permissions). However, I'd argue that it's even better, because it restricts the influence and access of change permissions, which can still be overridden/fixed by giving the user the extra view permissions.

@coveralls
Copy link

Coverage Status

coverage: 94.261% (+0.02%) from 94.244% when pulling 3a29cca on rensieeee:master into ddca032 on django-tastypie:master.

@georgedorn
Copy link
Contributor

Does anything change about TastyPie's authorization docs? Or is this just a compatibility fix that was previously missed?

(Specifically, is https://github.com/django-tastypie/django-tastypie/blob/master/docs/authorization.rst still accurate or does it need updating?)

@rensieeee
Copy link
Author

First look seems to be that those docs need updating as well.

https://github.com/django-tastypie/django-tastypie/blob/master/docs/authorization.rst#djangoauthorization

This part in particular, but I'll read through the whole thing tomorrow. I'll update the PR with updates.

rensieeee and others added 2 commits June 8, 2023 00:30
Updated DjangoAuthorization descriptions according to Django 4.2 authorization methods.
@rensieeee
Copy link
Author

I updated the docs, everything is up-to-date now.

Was thinking, maybe it's an idea to keep the READ_PERM_CODE implementation in another way? I guess a lot of implementations use this to exclude GET calls to view permissions, and this change will allow change permissions access as well. Maybe a check on this value to see if it's been set and if so, don't check on change permissions?

It will require a deprecation flag as well, but it'd be less breaking for existing implementations. But in this state it works too. Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants