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 Raphael Michel, 9 years ago

Development of the pull request is happening on https://github.com/raphaelm/django/tree/ticket_24914

comment:2 by Raphael Michel, 9 years ago

Needs documentation: set
Needs tests: set
Owner: changed from nobody to Raphael Michel
Status: newassigned

comment:3 by Joachim Jablon, 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 Ana Balica, 9 years ago

Triage Stage: UnreviewedAccepted

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 Raphael Michel, 9 years ago

Summary: Include a LoginRequiredMixin to DjangoInclude basic permission mixins into Django

comment:6 by Raphael Michel, 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 Raphael Michel, 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 Markus Holtermann, 9 years ago

Cc: Markus Holtermann added
Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:9 by Raphael Michel, 9 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:11 by Markus Holtermann, 9 years ago

Owner: changed from Raphael Michel to Markus Holtermann
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 Thomas Randle, 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.

comment:13 by Markus Holtermann, 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 Raphael Michel, 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:15 by Markus Holtermann, 9 years ago

I just added some tests to check for the mixin stacking.

comment:16 by Markus Holtermann, 9 years ago

Here's a PR that adopts django-braces' mixins: https://github.com/django/django/pull/4844

comment:17 by Markus Holtermann, 9 years ago

Patch needs improvement: unset

in reply to:  13 comment:18 by Jayson Reis, 9 years ago

Replying to MarkusH:

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.

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:19 by Jayson Reis, 9 years ago

Never mind I just saw that there is a pull request already :)

comment:20 by Markus Holtermann, 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:21 by Markus Holtermann <info@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In e5cb4e1:

Fixed #24914 -- Added authentication mixins for CBVs

Added the mixins LoginRequiredMixin, PermissionRequiredMixin and
UserPassesTestMixin to contrib.auth as counterparts to the respective
view decorators.

The authentication mixins UserPassesTestMixin, LoginRequiredMixin and
PermissionRequiredMixin have been inspired by django-braces
<https://github.com/brack3t/django-braces/>

Thanks Raphael Michel for the initial patch, tests and docs on the PR
and Ana Balica, Kenneth Love, Marc Tamlyn, and Tim Graham for the
review.

comment:22 by Tim Graham, 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 Tim Graham, 9 years ago

Resolution: fixed
Status: closednew

comment:24 by Tim Graham, 9 years ago

Has patch: set

PR for additional documentation.

comment:25 by Tim Graham <timograham@…>, 9 years ago

In 6c6eb8a6:

Refs #24914 -- Added docs for more auth mixin methods.

comment:26 by Tim Graham, 9 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top