Skip to content

DjangoModelPermissions should perform auth check before accessing the view's queryset #5376

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

Merged
merged 5 commits into from
Sep 4, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Aug 30, 2017

The below will replicate the old behavior where MethodNotAllowed preceded the authentication check:

class DjangoModelPermissions(permissions.DjangoModelPermissions):

    def has_permission(self, request, view):
        if request.method not in self.perms_map:
            raise exceptions.MethodNotAllowed(request.method)
        return super().has_permission(request, view)

This is an updated version of #5367 that includes a regression test.

Note:
This does slightly change the behavior for when the request has no authentication. Previously, if a user made an unauthenticated request, they would receive a 405. With the PR, users would receive the 401 first, then 405 once authenticated.

I'd argue that the change is more correct, given the general ordering of authentication, authorization, then method allowed checks. Either way, the old behavior can be replicated by moving the 405 check out of get_required_permissions() and into has_permission() before the request.user check.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

I'm happy with the change here.

I think we need to milestone for 3.7 (and being thinking about rolling that.

Can I ask you to just add a note on the breaking change (and the workaround) to the beginning of some release notes?

(Ta!)

@carltongibson carltongibson added this to the 3.7.0 Release milestone Aug 31, 2017
@rpkilby
Copy link
Member Author

rpkilby commented Aug 31, 2017

to the beginning of some release notes?

Do you mean to go ahead and update docs/release-notes.md?

@carltongibson
Copy link
Collaborator

I do. 😃

Mostly it’s just a once per release but items that need thinking about can be done as we go (otherwise I’ll forget to call it out.)

@rpkilby rpkilby force-pushed the django-perms-queryset branch from e5eb315 to 23b2d80 Compare September 1, 2017 17:56
@rpkilby
Copy link
Member Author

rpkilby commented Sep 1, 2017

Okay, release note has been added.

I've also extracted the queryset handling into a private method, as the duplicate code in the model / object permission has gotten slightly out of sync.

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.

3 participants