diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 9dfdb8bcfb..00174bca73 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -13,6 +13,7 @@ from django.db import models, transaction from django.db.models.fields import BLANK_CHOICE_DASH from django.http import Http404, HttpResponse, HttpResponseRedirect from django.shortcuts import get_object_or_404, render_to_response +from django.utils.decorators import method_decorator from django.utils.datastructures import SortedDict from django.utils.functional import update_wrapper from django.utils.html import escape @@ -53,6 +54,7 @@ FORMFIELD_FOR_DBFIELD_DEFAULTS = { models.FileField: {'widget': widgets.AdminFileWidget}, } +csrf_protect_m = method_decorator(csrf_protect) class BaseModelAdmin(object): """Functionality common to both ModelAdmin and InlineAdmin.""" @@ -754,7 +756,7 @@ class ModelAdmin(BaseModelAdmin): msg = _("No action selected.") self.message_user(request, msg) - @csrf_protect + @csrf_protect_m @transaction.commit_on_success def add_view(self, request, form_url='', extra_context=None): "The 'add' admin view for this model." @@ -844,7 +846,7 @@ class ModelAdmin(BaseModelAdmin): context.update(extra_context or {}) return self.render_change_form(request, context, form_url=form_url, add=True) - @csrf_protect + @csrf_protect_m @transaction.commit_on_success def change_view(self, request, object_id, extra_context=None): "The 'change' admin view for this model." @@ -936,7 +938,7 @@ class ModelAdmin(BaseModelAdmin): context.update(extra_context or {}) return self.render_change_form(request, context, change=True, obj=obj) - @csrf_protect + @csrf_protect_m def changelist_view(self, request, extra_context=None): "The 'change list' admin view for this model." from django.contrib.admin.views.main import ERROR_FLAG @@ -1057,7 +1059,7 @@ class ModelAdmin(BaseModelAdmin): 'admin/change_list.html' ], context, context_instance=context_instance) - @csrf_protect + @csrf_protect_m def delete_view(self, request, object_id, extra_context=None): "The 'delete' admin view for this model." opts = self.model._meta diff --git a/django/contrib/auth/admin.py b/django/contrib/auth/admin.py index b444fa5bac..61bae28052 100644 --- a/django/contrib/auth/admin.py +++ b/django/contrib/auth/admin.py @@ -10,9 +10,12 @@ from django.http import HttpResponseRedirect, Http404 from django.shortcuts import render_to_response, get_object_or_404 from django.template import RequestContext from django.utils.html import escape +from django.utils.decorators import method_decorator from django.utils.translation import ugettext, ugettext_lazy as _ from django.views.decorators.csrf import csrf_protect +csrf_protect_m = method_decorator(csrf_protect) + class GroupAdmin(admin.ModelAdmin): search_fields = ('name',) ordering = ('name',) @@ -76,7 +79,7 @@ class UserAdmin(admin.ModelAdmin): (r'^(\d+)/password/$', self.admin_site.admin_view(self.user_change_password)) ) + super(UserAdmin, self).get_urls() - @csrf_protect + @csrf_protect_m @transaction.commit_on_success def add_view(self, request, form_url='', extra_context=None): # It's an error for a user to have add permission but NOT change diff --git a/django/contrib/auth/decorators.py b/django/contrib/auth/decorators.py index 798e596978..97db3bb1c9 100644 --- a/django/contrib/auth/decorators.py +++ b/django/contrib/auth/decorators.py @@ -6,7 +6,7 @@ except ImportError: from django.contrib.auth import REDIRECT_FIELD_NAME from django.http import HttpResponseRedirect from django.utils.http import urlquote -from django.utils.decorators import auto_adapt_to_methods + def user_passes_test(test_func, login_url=None, redirect_field_name=REDIRECT_FIELD_NAME): """ @@ -26,7 +26,8 @@ def user_passes_test(test_func, login_url=None, redirect_field_name=REDIRECT_FIE tup = login_url, redirect_field_name, path return HttpResponseRedirect('%s?%s=%s' % tup) return wraps(view_func)(_wrapped_view) - return auto_adapt_to_methods(decorator) + return decorator + def login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME): """ @@ -41,6 +42,7 @@ def login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME): return actual_decorator(function) return actual_decorator + def permission_required(perm, login_url=None): """ Decorator for views that checks whether a user has a particular permission diff --git a/django/contrib/formtools/wizard.py b/django/contrib/formtools/wizard.py index 4729e155b8..b97e090cfd 100644 --- a/django/contrib/formtools/wizard.py +++ b/django/contrib/formtools/wizard.py @@ -14,8 +14,10 @@ from django.template.context import RequestContext from django.utils.hashcompat import md5_constructor from django.utils.translation import ugettext_lazy as _ from django.contrib.formtools.utils import security_hash +from django.utils.decorators import method_decorator from django.views.decorators.csrf import csrf_protect + class FormWizard(object): # Dictionary of extra template context variables. extra_context = {} @@ -45,7 +47,7 @@ class FormWizard(object): # hook methods might alter self.form_list. return len(self.form_list) - @csrf_protect + @method_decorator(csrf_protect) def __call__(self, request, *args, **kwargs): """ Main method that does all the hard work, conforming to the Django view diff --git a/django/utils/decorators.py b/django/utils/decorators.py index e3f16e462e..810d863849 100644 --- a/django/utils/decorators.py +++ b/django/utils/decorators.py @@ -7,44 +7,24 @@ except ImportError: from django.utils.functional import wraps, update_wrapper # Python 2.3, 2.4 fallback. -# Licence for MethodDecoratorAdaptor and auto_adapt_to_methods -# -# This code is taken from stackoverflow.com [1], the code being supplied by -# users 'Ants Aasma' [2] and 'Silent Ghost' [3] with modifications. It is -# legally included here under the terms of the Creative Commons -# Attribution-Share Alike 2.5 Generic Licence [4] -# -# [1] http://stackoverflow.com/questions/1288498/using-the-same-decorator-with-arguments-with-functions-and-methods -# [2] http://stackoverflow.com/users/107366/ants-aasma -# [3] http://stackoverflow.com/users/12855/silentghost -# [4] http://creativecommons.org/licenses/by-sa/2.5/ +def method_decorator(decorator): + """ + Converts a function decorator into a method decorator + """ + def _dec(func): + def _wrapper(self, *args, **kwargs): + def bound_func(*args2, **kwargs2): + return func(self, *args2, **kwargs2) + # bound_func has the signature that 'decorator' expects i.e. no + # 'self' argument, but it is a closure over self so it can call + # 'func' correctly. + return decorator(bound_func)(*args, **kwargs) + return wraps(func)(_wrapper) + update_wrapper(_dec, decorator) + # Change the name to aid debugging. + _dec.__name__ = 'method_dec(%s)' % decorator.__name__ + return _dec -class MethodDecoratorAdaptor(object): - """ - Generic way of creating decorators that adapt to being - used on methods - """ - def __init__(self, decorator, func): - update_wrapper(self, func) - # NB: update the __dict__ first, *then* set - # our own .func and .decorator, in case 'func' is actually - # another MethodDecoratorAdaptor object, which has its - # 'func' and 'decorator' attributes in its own __dict__ - self.decorator = decorator - self.func = func - def __call__(self, *args, **kwargs): - return self.decorator(self.func)(*args, **kwargs) - def __get__(self, instance, owner): - return self.decorator(self.func.__get__(instance, owner)) - -def auto_adapt_to_methods(decorator): - """ - Takes a decorator function, and returns a decorator-like callable that can - be used on methods as well as functions. - """ - def adapt(func): - return MethodDecoratorAdaptor(decorator, func) - return wraps(decorator)(adapt) def decorator_from_middleware_with_args(middleware_class): """ @@ -61,6 +41,7 @@ def decorator_from_middleware_with_args(middleware_class): """ return make_middleware_decorator(middleware_class) + def decorator_from_middleware(middleware_class): """ Given a middleware class (not an instance), returns a view decorator. This @@ -69,6 +50,7 @@ def decorator_from_middleware(middleware_class): """ return make_middleware_decorator(middleware_class)() + def make_middleware_decorator(middleware_class): def _make_decorator(*m_args, **m_kwargs): middleware = middleware_class(*m_args, **m_kwargs) @@ -96,5 +78,5 @@ def make_middleware_decorator(middleware_class): return result return response return wraps(view_func)(_wrapped_view) - return auto_adapt_to_methods(_decorator) + return _decorator return _make_decorator diff --git a/django/views/decorators/cache.py b/django/views/decorators/cache.py index f051f096a0..303a41ef7d 100644 --- a/django/views/decorators/cache.py +++ b/django/views/decorators/cache.py @@ -16,10 +16,11 @@ try: except ImportError: from django.utils.functional import wraps # Python 2.3, 2.4 fallback. -from django.utils.decorators import decorator_from_middleware_with_args, auto_adapt_to_methods +from django.utils.decorators import decorator_from_middleware_with_args from django.utils.cache import patch_cache_control, add_never_cache_headers from django.middleware.cache import CacheMiddleware + def cache_page(*args, **kwargs): # We need backwards compatibility with code which spells it this way: # def my_view(): pass @@ -48,18 +49,16 @@ def cache_page(*args, **kwargs): else: return decorator_from_middleware_with_args(CacheMiddleware)(cache_timeout=args[0], key_prefix=key_prefix) + def cache_control(**kwargs): - def _cache_controller(viewfunc): - def _cache_controlled(request, *args, **kw): response = viewfunc(request, *args, **kw) patch_cache_control(response, **kwargs) return response - return wraps(viewfunc)(_cache_controlled) + return _cache_controller - return auto_adapt_to_methods(_cache_controller) def never_cache(view_func): """ @@ -71,4 +70,3 @@ def never_cache(view_func): add_never_cache_headers(response) return response return wraps(view_func)(_wrapped_view_func) -never_cache = auto_adapt_to_methods(never_cache) diff --git a/docs/releases/1.2.txt b/docs/releases/1.2.txt index f4826787c2..f8bb3c8bf3 100644 --- a/docs/releases/1.2.txt +++ b/docs/releases/1.2.txt @@ -251,6 +251,55 @@ incompatibilities, especially if you are storing comma or semi-colon in cookies and have javascript code that parses and manipulates cookie values client-side. +``user_passes_test``, ``login_required`` and ``permission_required`` +-------------------------------------------------------------------- + +``django.contrib.auth.decorators`` provides the decorators ``login_required``, +``permission_required`` and ``user_passes_test``. Previously it was possible to +use these decorators both on functions (where the first argument is 'request') +and on methods (where the first argument is 'self', and the second argument is +'request'). However, we have found that the trick which enabled this is +flawed. It only works in limited circumstances, and produces errors that are +very difficult to debug when it does not work. + +For this reason, the 'auto adapt' behaviour has been removed, and if you are +using these decorators on methods, you will need to manually apply +:func:`django.utils.decorators.method_decorator` to convert the decorator to one +that works with methods. You would change code from this:: + + class MyClass(object): + + @login_required + def my_view(self, request): + pass + +to this:: + + from django.utils.decorators import method_decorator + + class MyClass(object): + + @method_decorator(login_required) + def my_view(self, request): + pass + +or:: + + from django.utils.decorators import method_decorator + + login_required_m = method_decorator(login_required) + + class MyClass(object): + + @login_required_m + def my_view(self, request): + pass + +For those following trunk, this change also applies to other decorators +introduced since 1.1, including ``csrf_protect``, ``cache_control`` and anything +created using ``decorator_from_middleware``. + + .. _deprecated-features-1.2: Features deprecated in 1.2 diff --git a/tests/modeltests/test_client/views.py b/tests/modeltests/test_client/views.py index 111c72e7f5..68963fff75 100644 --- a/tests/modeltests/test_client/views.py +++ b/tests/modeltests/test_client/views.py @@ -7,6 +7,7 @@ from django.contrib.auth.decorators import login_required, permission_required from django.forms.forms import Form from django.forms import fields from django.shortcuts import render_to_response +from django.utils.decorators import method_decorator def get_view(request): "A simple view that expects a GET request, and returns a rendered template" @@ -147,14 +148,15 @@ def permission_protected_view(request): permission_protected_view = permission_required('modeltests.test_perm')(permission_protected_view) class _ViewManager(object): + @method_decorator(login_required) def login_protected_view(self, request): t = Template('This is a login protected test using a method. ' 'Username is {{ user.username }}.', name='Login Method Template') c = Context({'user': request.user}) return HttpResponse(t.render(c)) - login_protected_view = login_required(login_protected_view) + @method_decorator(permission_required('modeltests.test_perm')) def permission_protected_view(self, request): t = Template('This is a permission protected test using a method. ' 'Username is {{ user.username }}. ' @@ -162,7 +164,6 @@ class _ViewManager(object): name='Permissions Template') c = Context({'user': request.user}) return HttpResponse(t.render(c)) - permission_protected_view = permission_required('modeltests.test_perm')(permission_protected_view) _view_manager = _ViewManager() login_protected_method_view = _view_manager.login_protected_view diff --git a/tests/regressiontests/decorators/tests.py b/tests/regressiontests/decorators/tests.py index b81a5d665a..2330b0e073 100644 --- a/tests/regressiontests/decorators/tests.py +++ b/tests/regressiontests/decorators/tests.py @@ -10,7 +10,7 @@ from django.utils.functional import allow_lazy, lazy, memoize from django.views.decorators.http import require_http_methods, require_GET, require_POST from django.views.decorators.vary import vary_on_headers, vary_on_cookie from django.views.decorators.cache import cache_page, never_cache, cache_control -from django.utils.decorators import auto_adapt_to_methods +from django.utils.decorators import method_decorator from django.contrib.auth.decorators import login_required, permission_required, user_passes_test from django.contrib.admin.views.decorators import staff_member_required @@ -47,6 +47,7 @@ fully_decorated = memoize(fully_decorated, {}, 1) fully_decorated = allow_lazy(fully_decorated) fully_decorated = lazy(fully_decorated) + class DecoratorsTest(TestCase): def test_attributes(self): @@ -112,42 +113,25 @@ class DecoratorsTest(TestCase): my_view_cached2 = cache_page(my_view, 123, key_prefix="test") self.assertEqual(my_view_cached2(HttpRequest()), "response") -class MethodDecoratorAdapterTests(TestCase): - def test_auto_adapt_to_methods(self): - """ - Test that auto_adapt_to_methods actually works. - """ - # Need 2 decorators with auto_adapt_to_methods, - # to check it plays nicely with composing itself. - def my_decorator(func): - def wrapped(*args, **kwargs): - # need to ensure that the first arg isn't 'self' - self.assertEqual(args[0], "test") - return "my_decorator:" + func(*args, **kwargs) - wrapped.my_decorator_custom_attribute = True - return wraps(func)(wrapped) - my_decorator = auto_adapt_to_methods(my_decorator) +# For testing method_decorator, a decorator that assumes a single argument. +# We will get type arguments if there is a mismatch in the number of arguments. +def simple_dec(func): + def wrapper(arg): + return func("test:" + arg) + return wraps(func)(wrapper) - def my_decorator2(func): - def wrapped(*args, **kwargs): - # need to ensure that the first arg isn't 'self' - self.assertEqual(args[0], "test") - return "my_decorator2:" + func(*args, **kwargs) - wrapped.my_decorator2_custom_attribute = True - return wraps(func)(wrapped) - my_decorator2 = auto_adapt_to_methods(my_decorator2) +simple_dec_m = method_decorator(simple_dec) - class MyClass(object): - def my_method(self, *args, **kwargs): - return "my_method:%r %r" % (args, kwargs) - my_method = my_decorator2(my_decorator(my_method)) - obj = MyClass() - self.assertEqual(obj.my_method("test", 123, name='foo'), - "my_decorator2:my_decorator:my_method:('test', 123) {'name': 'foo'}") - self.assertEqual(obj.my_method.__name__, 'my_method') - self.assertEqual(getattr(obj.my_method, 'my_decorator_custom_attribute', False), - True) - self.assertEqual(getattr(obj.my_method, 'my_decorator2_custom_attribute', False), - True) +class MethodDecoratorTests(TestCase): + """ + Tests for method_decorator + """ + def test_method_decorator(self): + class Test(object): + @simple_dec_m + def say(self, arg): + return arg + + self.assertEqual("test:hello", Test().say("hello"))