Opened 17 years ago

Closed 17 years ago

#4376 closed (fixed)

login_required doesn't work with bound methods

Reported by: steven.bethard@… Owned by: nobody
Component: Uncategorized Version: dev
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: no UI/UX: no

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@… 17 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@… 17 years ago.
bugfix for last patch
django.contrib.auth.decorators.3.diff (8.9 KB ) - added by anonymous 17 years ago.
updated version with tests
django.contrib.auth.decorators.4.diff (9.6 KB ) - added by steven.bethard@… 17 years ago.
Patch against revision 6364. This should now apply cleanly.

Download all attachments as: .zip

Change History (11)

by steven.bethard@…, 17 years ago

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

by steven.bethard@…, 17 years ago

bugfix for last patch

comment:1 by steven.bethard@…, 17 years ago

Has patch: set
Needs tests: set

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 by Gary Wilson <gary.wilson@…>, 17 years ago

have you seen the testing documentation?

by anonymous, 17 years ago

updated version with tests

comment:3 by steven.bethard@…, 17 years ago

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 by Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedReady for checkin

comment:5 by Malcolm Tredinnick, 17 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

by steven.bethard@…, 17 years ago

Patch against revision 6364. This should now apply cleanly.

comment:6 by scompt@…, 17 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady 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 by Luke Plant, 17 years ago

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.
Back to Top