#25142 closed Cleanup/optimization (fixed)
Add PermissionRequiredMixin.has_permission() to allow customization
Reported by: | Akis Kesoglou | Owned by: | Akis Kesoglou |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | akiskesoglou@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
Extending from PermissionRequiredMixin to implement a mixin for checking for *object* permissions means that you can't call super in .dispatch because PermissionRequiredMixin will do another check for model-level permissions.
I propose to delegate the check for permissions in PermissionRequiredMixin.dispatch to an instance method. This way, we are able to provide a hook for code to check for object permissions without having to re-implement the whole class.
PR coming shortly.
I have went ahead and generalised the ticket and patch to refactor all access mixins.
Change History (21)
comment:1 by , 9 years ago
Cc: | added |
---|---|
Has patch: | set |
Status: | new → assigned |
comment:2 by , 9 years ago
comment:3 by , 9 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Does this actually solve the issue? You haven't called get_object()
by the time dispatch()
is called. You would now query for the object twice, one during permission check and once sometime later (depending on the CBV).
Tentively accepting based on the general idea.
comment:4 by , 9 years ago
get_object()
will get called twice -- I don't see a way around it, unless itself caches the result somewhere, but I haven't investigated what happens then with form mixins. Another way would be to delay permission checks by wrapping the handler methods themselves, but this seems fragile. I personally consider this a fair compromise, but suggestions are welcome.
comment:5 by , 9 years ago
Just in case I was not clear in the ticket's description, the purpose is to allow subclassing the PermissionRequiredMixin to customise authorization. As it is now, PermissionRequiredMixin has model-level authorization hard-coded to dispatch -- there's no way to extend it -- one must implement a completely new mixin, duplicating most code from PermissionRequiredMixin.
comment:6 by , 9 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:7 by , 9 years ago
Description: | modified (diff) |
---|---|
Summary: | Refactor dispatch method in PermissionRequiredMixin to allow customisation → Refactor access mixins to allow customisation |
comment:8 by , 9 years ago
I have modified the AccessMixin base class to provide the dispatch method itself, and delegate the check for access to subclasses. This simplifies/cleans up the code, but most importantly, allows one to use the mixins as base classes to slightly customise the behaviour.
Ticket and PR updated.
comment:9 by , 9 years ago
Needs documentation: | set |
---|
I'm not sure that we need tests, but should we document the has_permission()
method?
comment:10 by , 9 years ago
Hi Tim, if by "document" you mean "public API" then it depends on whether these mixins are intended to be extended. There's no reason one would call the method unless wanting to customise the mixin somehow and UserPassesTestMixin
isn't enough. Apart from checking for object permissions, I can't think of other reasons to extend though, and that includes my experience with the respective function decorators -- access customisation is usually done with auth backends.
Regarding tests, the only meaningful test for these changes I guess would be to test that has_permission
raises an exception if not provided.
comment:11 by , 9 years ago
AccessMixin
is documented. I thought the point of this change was to improve extensibility or was it just meant as an internal cleanup?
comment:12 by , 9 years ago
The point is to allow extensibility, indeed -- my comment was more about asking for a decision to be made, placing a couple counter arguments along the way. Hadn't see that AccessMixin was already documented, so I think that also documenting has_permission is the way to go. I will update the PR later.
comment:14 by , 9 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Unfortunately turns out we can't refactor all mixins to implement AccessMixin.has_permission
, because it would forbid stacking them -- docs warn about this and explain why that would be the case (BTW this admonition is not rendered in djangoproject.com/docs).
I reverted to the initial patch which allows PermissionRequiredMixin to be used as a base class to easily implement an object permissions mixin.
comment:15 by , 9 years ago
Thanks for the report. Fixed that doc warning in e3d1f2422c49d7f65d017b8caa35215afe18f835.
comment:16 by , 9 years ago
Markus, was it intentional not to document methods like get_redirect_field_name()
, get_permission_denied_message()
, and get_permission_required()
? This seems to fall into a similar category.
comment:17 by , 9 years ago
I looked for the documentation of things like get_slug_field()
but must have missed them, that's why I didn't mentioned them.
comment:18 by , 9 years ago
Needs documentation: | set |
---|
Okay, I reopened #24914 to add those docs. dfunckt, could you please document the new method here.
comment:19 by , 9 years ago
Needs documentation: | unset |
---|
I updated the PR with docs. I think the description I give is a bit terse, so I'd appreciate suggestions for improvement if required.
comment:21 by , 9 years ago
Summary: | Refactor access mixins to allow customisation → Add PermissionRequiredMixin.has_permission() to allow customization |
---|
Here's the PR: https://github.com/django/django/pull/5020