[Django] #25142: Refactor dispatch method in PermissionRequiredMixin to allow customisation
Django
--------------------------------------+---------------------
Reporter: dfunckt | Owner: dfunckt
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+---------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: dfunckt | Owner: dfunckt
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by dfunckt):
* status: new => assigned
* cc: akiskesoglou@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:1>
Django
Reporter: dfunckt | Owner: dfunckt
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by dfunckt):
Here's the PR: https://github.com/django/django/pull/5020
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:2>
Django
Reporter: dfunckt | Owner: dfunckt
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by MarkusH):
* needs_docs: 0 => 1
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:3>
Django
Reporter: dfunckt | Owner: dfunckt
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by dfunckt):
{{{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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:4>
Django
Reporter: dfunckt | Owner: dfunckt
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by dfunckt):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:5>
Django
Reporter: dfunckt | Owner: dfunckt
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by dfunckt):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: dfunckt | Owner: dfunckt
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Description changed by dfunckt:
Old description:
> 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.
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:7>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by dfunckt):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:8>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
Comment:
I'm not sure that we need tests, but should we document the
`has_permission()` method?
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:9>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 1 | UI/UX: 0
Comment (by dfunckt):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:10>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 1 | UI/UX: 0
Comment (by timgraham):
`AccessMixin`
[https://github.com/django/django/blob/03aec35a12ea522ab9a60d6229b53e3ec3a871a3/docs/topics/auth/default.txt#L721-L753
is documented]. I thought the point of this change was to improve
extensibility or was it just meant as an internal cleanup?
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:11>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 1 | UI/UX: 0
Comment (by dfunckt):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:12>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 1 | UI/UX: 0
Comment (by MarkusH):
Yes, pleas document the `has_permission` method.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:13>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 1 | UI/UX: 0
Changes (by dfunckt):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
Comment:
Unfortunately turns out we can't refactor all mixins to implement
{{{AccessMixin.has_permission}}}, because it would forbid stacking them --
[https://github.com/django/django/blob/master/docs/topics/auth/default.txt#L621
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:14>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by timgraham):
Thanks for the report. Fixed that doc warning in
e3d1f2422c49d7f65d017b8caa35215afe18f835.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:15>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:16>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by MarkusH):
I looked for the documentation of things like `get_slug_field()` but must
have missed them, that's why I didn't mentioned them.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:17>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
Comment:
Okay, I reopened #24914 to add those docs. dfunckt, could you please
document the new method here.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:18>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by dfunckt):
* needs_docs: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:19>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"29465d438e00660221e1e0ac5e2929a719c56113" 29465d43]:
{{{
#!CommitTicketReference repository=""
revision="29465d438e00660221e1e0ac5e2929a719c56113"
Fixed #25142 -- Added PermissionRequiredMixin.has_permission() to allow
customization.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:20>
Django
--------------------------------------+------------------------------------
Reporter: dfunckt | Owner: dfunckt
Type: Cleanup/optimization | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25142#comment:21>