Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#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 Akis Kesoglou)

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 Akis Kesoglou, 9 years ago

Cc: akiskesoglou@… added
Has patch: set
Status: newassigned

comment:3 by Markus Holtermann, 9 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 Akis Kesoglou, 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 Akis Kesoglou, 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 Akis Kesoglou, 9 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:7 by Akis Kesoglou, 9 years ago

Description: modified (diff)
Summary: Refactor dispatch method in PermissionRequiredMixin to allow customisationRefactor access mixins to allow customisation

comment:8 by Akis Kesoglou, 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 Tim Graham, 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 Akis Kesoglou, 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 Tim Graham, 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 Akis Kesoglou, 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:13 by Markus Holtermann, 9 years ago

Yes, pleas document the has_permission method.

comment:14 by Akis Kesoglou, 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.

I reverted to the initial patch which allows PermissionRequiredMixin to be used as a base class to easily implement an object permissions mixin.

Version 0, edited 9 years ago by Akis Kesoglou (next)

comment:15 by Tim Graham, 9 years ago

Thanks for the report. Fixed that doc warning in e3d1f2422c49d7f65d017b8caa35215afe18f835.

comment:16 by Tim Graham, 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 Markus Holtermann, 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 Tim Graham, 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 Akis Kesoglou, 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:20 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 29465d43:

Fixed #25142 -- Added PermissionRequiredMixin.has_permission() to allow customization.

comment:21 by Tim Graham, 9 years ago

Summary: Refactor access mixins to allow customisationAdd PermissionRequiredMixin.has_permission() to allow customization
Note: See TracTickets for help on using tickets.
Back to Top