Opened 9 years ago
Closed 9 years ago
#24914 closed New feature (fixed)
Include basic permission mixins into Django
Reported by: | Raphael Michel | Owned by: | Markus Holtermann |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | 1.9 |
Cc: | Markus Holtermann | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
Django does provide very convenient decorators in django.contrib.auth
, most notably the login_required
decorator. At the same time, Django encourages the use of class-based views, where this cannot be used directly. The de-facto standard to do this is a mixin, there even is an example on this mixin in the documentation: https://docs.djangoproject.com/en/1.8/topics/class-based-views/intro/#mixins-that-wrap-as-view
I'll now be working on a proposed pull request during djangocon.eu 2015 sprint.
It is to discuss whether the same is to be done with permission_required.
Change History (26)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Owner: | changed from | to
Status: | new → assigned |
comment:3 by , 9 years ago
It's worth noting that it's a feature that exists in http://django-braces.readthedocs.org/en/latest/index.html
comment:4 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
It is a good idea to provide a LoginRequiredMixin
, which basically will act the same way as the login_required
decorator. It might be implemented in a cleaner way by decorating dispatch
method (in contrast with decorating as_view
classmethod).
I would opt for getting started with LoginRequiredMixin
and afterwards providing mixins for user_passes_test
and permission_required
from django.contrib.auth.decorators
.
https://docs.djangoproject.com/en/dev/_modules/django/contrib/auth/decorators/
comment:5 by , 9 years ago
Summary: | Include a LoginRequiredMixin to Django → Include basic permission mixins into Django |
---|
comment:6 by , 9 years ago
Has patch: | set |
---|---|
Needs tests: | unset |
I created a pull request for this ticket. An initial version of implementation and tests is in the pull request, documentation is yet to be done: https://github.com/django/django/pull/4749
This proposal provides all the and only the functionality of all three authentication decorators from contrib.auth
.
While it is slightly differently implemented, it tries to stay compatible with the widely-used 3rd-party package django-braces
in terms of parameter and method names. django-braces
does have more functionality than the decorators have, but implementing this functionality in django itself is outside the scope of this ticket.
comment:7 by , 9 years ago
Needs documentation: | unset |
---|
I added documentation. As I'm not a native speaker, I'd love someone to review the language.
I decieded to drop a section in the documentation on class-based view on wrapping as_view with decorators, as there no longer seems to be any decorator in django core where this would make any sense.
comment:8 by , 9 years ago
Cc: | added |
---|---|
Needs documentation: | set |
Needs tests: | set |
Patch needs improvement: | set |
comment:9 by , 9 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:11 by , 9 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | set |
The current implementation has some drawbacks as pointed out on the PR. I'll fix the in aforementioned PR.
comment:12 by , 9 years ago
What would be the recommend approach for stacking different permission mixins on a CBV?
e.g. using LoginRequiredMixin and a PermissionRequiredMixin on the same view.
follow-up: 18 comment:13 by , 9 years ago
Thanks for mentioning that, Rundll. The current implementation does not support stacking them (braces' LoginRequired
and PermissionRequired
mixins do, though). However, neither our UserPassesTestMixin
nor braces' allows stacking inherited mixins, either.
That makes me wonder how one is supposed to combine different permission checks that are used separately and together:
class CheckOneMixin(UserPassesTestMixin): def test_func(self, user): return something_is_true_or_false class CheckTwoMixin(UserPassesTestMixin): def test_func(self, user): return something_else_is_true_or_false class MyView1(CheckOneMixin, View): pass class MyView2(CheckTwoMixin, View): pass class MyView3(CheckOneMixin, CheckTwoMixin, View): pass
Effectively, MyView3
will never bother about the value of something_else_is_true_or_false
as that function is never called.
comment:14 by , 9 years ago
I don't really see an easy and obvious way to support stacking of UserPassesTestMixin
. You could either do something like
class CheckOneMixin(UserPassesTestMixin): def test_func(self, user): return something_is_true_or_false and super().test_func()
or make the API more complex by allowing each subclass to set/append to a list of test functions in some way.
comment:16 by , 9 years ago
Here's a PR that adopts django-braces' mixins: https://github.com/django/django/pull/4844
comment:17 by , 9 years ago
Patch needs improvement: | unset |
---|
comment:18 by , 9 years ago
Replying to MarkusH:
Thanks for mentioning that, Rundll. The current implementation does not support stacking them (braces'
LoginRequired
andPermissionRequired
mixins do, though). However, neither ourUserPassesTestMixin
nor braces' allows stacking inherited mixins, either.
That makes me wonder how one is supposed to combine different permission checks that are used separately and together:
class CheckOneMixin(UserPassesTestMixin): def test_func(self, user): return something_is_true_or_false class CheckTwoMixin(UserPassesTestMixin): def test_func(self, user): return something_else_is_true_or_false class MyView1(CheckOneMixin, View): pass class MyView2(CheckTwoMixin, View): pass class MyView3(CheckOneMixin, CheckTwoMixin, View): passEffectively,
MyView3
will never bother about the value ofsomething_else_is_true_or_false
as that function is never called.
About ensuring at least one valid response, I thought something like this:
class CheckOneMixin: def test_func(self): print(self) return True class CheckTwoMixin: def test_func(self): print(self) return False class Dispatcher: def dispatch(self): validators = [b.test_func for b in self.__class__.__bases__ if hasattr(b, 'test_func')] for validator in validators: if validator(self): return 'Dispatch for real' return 'Invalid permissions' class MyView1(CheckOneMixin, Dispatcher): pass class MyView2(CheckTwoMixin, Dispatcher): pass class MyView3(CheckOneMixin, CheckTwoMixin, Dispatcher): pass for klass in (MyView1, MyView2, MyView3): print(klass().dispatch(), '\n')
In this case the class that act like the real dispatcher could call every function on View's bases.
comment:20 by , 9 years ago
Thanks jaysonsantos, I had something like that in an experimental version, but dropped it because I disliked iterating over __bases__
or mro()
.
comment:22 by , 9 years ago
Easy pickings: | set |
---|---|
Has patch: | unset |
Keywords: | 1.9 added |
Reopening to add documentation for the new methods like get_redirect_field_name()
, get_permission_denied_message()
, and get_permission_required()
. Anything in the changeset above that says " Override this method ..." should be documented.
comment:23 by , 9 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
comment:26 by , 9 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Development of the pull request is happening on https://github.com/raphaelm/django/tree/ticket_24914