Code

Opened 7 years ago

Closed 6 years ago

#4376 closed (fixed)

login_required doesn't work with bound methods

Reported by: steven.bethard@… Owned by: nobody
Component: Uncategorized Version: master
Severity: Keywords: auth login_required decorator
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The django.contrib.auth.decorators.login_required decorator doesn't work correctly with bound methods. If I have code like:

class ViewManager(object):
    @login_required
    def get_main_view(self, request):
        return HttpResponse('hello')

view_manager = ViewManager()
urlpatterns = patterns('',
    (r'^$', view_manager.get_main_view),
)

I'll get an error saying:

AttributeError at /
'ViewManager' object has no attribute 'user'

I believe this is because __get__ is being invoked on the _checklogin function returned by login_required and so the ViewManager instance is being bound as the request argument of _checklogin instead of being bound as the self argument of get_main_view as it should be. To fix this, I had to replace the _dec function in django.contrib.auth.decorators.user_passes_test like so::

def user_passes_test(test_func, login_url=None):
    if not login_url:
        from django.conf import settings
        login_url = settings.LOGIN_URL
    class CheckLogin(object):
        def __init__(self, func):
            self.func = func
        def __get__(self, obj, cls=None):
            return CheckLogin(self.func.__get__(obj, cls))
        def __call__(self, request, *args, **kwargs):
            if test_func(request.user):
                return view_func(request, *args, **kwargs)
            return HttpResponseRedirect('%s?%s=%s' % (login_url, REDIRECT_FIELD_NAME, quote(request.get_full_path())))
    return CheckLogin

This simply redirects the __get__ call to the wrapped function, instead of allowing it to be called on the _checklogin function.

Attachments (4)

django.contrib.auth.decorators.diff (2.7 KB) - added by steven.bethard@… 7 years ago.
rewrite of django.contrib.auth.decorators to handle bound methods properly
django.contrib.auth.decorators.2.diff (2.7 KB) - added by steven.bethard@… 7 years ago.
bugfix for last patch
django.contrib.auth.decorators.3.diff (8.9 KB) - added by anonymous 7 years ago.
updated version with tests
django.contrib.auth.decorators.4.diff (9.6 KB) - added by steven.bethard@… 7 years ago.
Patch against revision 6364. This should now apply cleanly.

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by steven.bethard@…

rewrite of django.contrib.auth.decorators to handle bound methods properly

Changed 7 years ago by steven.bethard@…

bugfix for last patch

comment:1 Changed 7 years ago by steven.bethard@…

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset

I don't know how to write unit tests for Django, but if someone can point me in the right direction, I can make some tests for this. I need to know know how to programatically visit, say, '/' and verify that it returns the appropriate output.

comment:2 Changed 7 years ago by Gary Wilson <gary.wilson@…>

have you seen the testing documentation?

Changed 7 years ago by anonymous

updated version with tests

comment:3 Changed 7 years ago by steven.bethard@…

  • Needs tests unset

Thanks for the pointer. I think I figured out how to modify modeltests.test_client where the only existing django.contrib.auth.decorators test I could find was. The patch I just uploaded rewrites the decorators module to handle methods properly, and adds three new tests: test_view_with_method_login, test_view_with_permissions and test_view_with_method_permissions.

Note that I didn't see the @permission_required decorator tested anywhere, so I added both a regular function test and a method test for that one. Both of these tests have TODO comments on them - I was able to test that when the permissions are wrong, they redirect to a login page, but I wasn't sure how to get a user with the right permissions to complete the tests. Of course, if @permission_required really wasn't tested at all before, half-way there is better than nothing, right? ;-)

comment:4 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:5 Changed 7 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

I like this patch. Code looks code and everything.

Unfortunately, due to the changes during the sprint over the last few days, in particular, it no longer cleanly applies and at least one of the changes (django.contrib.auth.decorators) requires a bit of reworking of the code.

If somebody could update the patch, it's then ready to go in.

Changed 7 years ago by steven.bethard@…

Patch against revision 6364. This should now apply cleanly.

comment:6 Changed 6 years ago by scompt@…

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

I think this patch may have fallen through the cracks. I've just applied it against revision 6652 and it still applies cleanly, so I'm going to tweak this ticket so somebody sees it. I hope that's cool with the powers that be.

comment:7 Changed 6 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

(In [6658]) Fixed #4376 -- login_required now works with bound methods. Thanks, Steven Bethard.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.