PermissionRequiredMixin doesn't allow custom permissions/object permissions #67

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@chancez
Contributor
chancez commented Aug 8, 2013

The PermissionRequiredMixin does not allow for checking custom object permissions provided by libraries like django object permissions or django-guardian.

I was thinking of updating it to be similar to https://github.com/lukaszb/django-guardian/blob/master/guardian/mixins.py#L52

Except stripping out the parts where it does object specific actions or guardian specific things, and allow overriding to implement said functionality.

@kennethlove
Member

The PermissionRequiredMixin is meant to be an analog to the @permission_required decorator, so, no, it doesn't take into account per-object permissions.

Extending it to do that is a neat idea, but I wonder if it shouldn't be a new mixin.

@chrisjones-brack3t
Member

I was going to say the same thing. I think it's a good idea but I believe a separate mixin makes more sense. Most of the mixins are meant to work with standard Django. Anything custom, I would think, should be named accordingly. Maybe CustomPermissionRequiredMixin or something along those lines. That way it's easily understood that it's not typical Django behavior.

@chancez
Contributor
chancez commented Aug 9, 2013

A CustomPermissionRequireMixin makes perfect sense. Still what about adding a few additional methods to the existing PermissionRequiredMixin such that its extendable, even for the default Django permissions. Providing an interface to extra permissions checks like checking user.is_staff or user.is_superuser?
These could go into something like check_permissions()?

@kennethlove
Member

I'm OK with a check_permissions method that's overridable by the user provided its default state maintains the current functionality. Users shouldn't have to change all of their PermissionRequiredMixin usages.

As for checking is_staff or is_superuser, there are two other mixins that currently handle that functionality. If the changes to PermissionRequiredMixin make these mixins redundant or vastly simplified, I don't have a problem with that, either, provided we keep them around for ease-of-use.

Do you think you can have your changes done this week, @ecnahc515? We'd like to get 1.3 out soon.

@chancez
Contributor
chancez commented Aug 12, 2013

@kennethlove Yeah, I think I could do that. I'll try working on it today and tomorrow. Shouldn't be hard to add.

@chancez
Contributor
chancez commented Aug 13, 2013

I've made some base changes to the two PermissionRequiredMixins that separate the logic into methods for obtaining permissions, checking permissions, and what to do when the user has no permissions. I think for simplicity this is all that's really necessary, let me know if I've overdone it, or some changes are needed.

If you like it thus far, I could update some of the other access related mixins to work this way too.

@kennethlove kennethlove commented on an outdated diff Aug 14, 2013
braces/views.py
@@ -126,22 +131,43 @@ def dispatch(self, request, *args, **kwargs):
"'PermissionRequiredMixin' requires "
"'permission_required' attribute to be set.")
+ return self.permission_required
+
+ def check_permissions(self, request):
+ "Returns whether or not the user has permissions"
@kennethlove
kennethlove Aug 14, 2013 Member

blockcomments start and end with """, not "

@chancez
Contributor
chancez commented Aug 15, 2013

@kennethlove anything else that needs updating?

@kennethlove
Member

Wow, sorry this has been so long. The only thing I'm seeing missing from this is testing.

@chancez
Contributor
chancez commented Jan 7, 2014

It's okay, things happen. I'm actually not sure how to go about testing these changes, I see you've got a generic testcase for the access mixins that covers most of the cases, and functionally this operates how it did prior to changes.

So I really just need to test the new methods and flow, but I am kinda unsure how to test these methods since they're mostly meant to be overridden. How should I go about the tests?

@kennethlove
Member

Produce a view(s) that overrides the methods and then test that view? That's how the other mixins are tested.

@chancez
Contributor
chancez commented Jan 7, 2014

Gotcha. Will do.

@chancez
Contributor
chancez commented Jan 8, 2014

@kennethlove I added tests, and the coverage is 100%. Let me know if I'm missing anything.

@chancez
Contributor
chancez commented Jan 14, 2014

@kennethlove Ping. Anything else this needs?

@kennethlove
Member

@ecnahc515 that looks pretty good. I'll run the tests when I prepare the next release and it should make it in. Thanks for your work.

@chancez
Contributor
chancez commented Jan 14, 2014

Sweet, let me know if there is anything else needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment