Add PermissionRequiredMixin.has_permission() to allow customization

[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.

[Akis Kesoglou]

Here's the PR: ​https://github.com/django/django/pull/5020

[Markus Holtermann]

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.

[Akis Kesoglou]

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.

[Akis Kesoglou]

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.

[Akis Kesoglou]

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.

[Tim Graham]

I'm not sure that we need tests, but should we document the has_permission() method?

[Akis Kesoglou]

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.

[Tim Graham]

AccessMixin ​is documented. I thought the point of this change was to improve extensibility or was it just meant as an internal cleanup?

[Akis Kesoglou]

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.

[Markus Holtermann]

Yes, pleas document the has_permission method.

[Akis Kesoglou]

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.

[Tim Graham]

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

[Tim Graham]

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.

[Markus Holtermann]

I looked for the documentation of things like get_slug_field() but must have missed them, that's why I didn't mentioned them.

[Tim Graham]

Okay, I reopened #24914 to add those docs. dfunckt, could you please document the new method here.

[Akis Kesoglou]

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.

[Tim Graham <timograham@…>]

In 29465d43:

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