From 46068d850d8debd3611ed6499d48b9907bf07ef6 Mon Sep 17 00:00:00 2001 From: Thomas Tanner Date: Wed, 31 Dec 2014 22:25:00 +0100 Subject: [PATCH] Fixed #22295 -- Replaced permission check for displaying admin user-tools --- django/contrib/admin/actions.py | 2 +- django/contrib/admin/options.py | 8 ++--- django/contrib/admin/sites.py | 15 ++++---- .../contrib/admin/templates/admin/base.html | 12 ++++--- .../contrib/admin/templates/admin/login.html | 2 ++ django/contrib/admindocs/views.py | 8 ++--- django/contrib/auth/admin.py | 2 +- docs/ref/contrib/admin/index.txt | 18 ++++++++++ docs/releases/1.8.txt | 17 +++++++++ .../custom_has_permission_admin.py | 33 +++++++++++++++++ .../fixtures/admin-views-users.xml | 14 ++++++++ tests/admin_views/tests.py | 36 +++++++++++++++++++ tests/admin_views/urls.py | 3 +- 13 files changed, 148 insertions(+), 22 deletions(-) create mode 100644 tests/admin_views/custom_has_permission_admin.py diff --git a/django/contrib/admin/actions.py b/django/contrib/admin/actions.py index bd14057ffd..2dafd3ea3a 100644 --- a/django/contrib/admin/actions.py +++ b/django/contrib/admin/actions.py @@ -64,7 +64,7 @@ def delete_selected(modeladmin, request, queryset): title = _("Are you sure?") context = dict( - modeladmin.admin_site.each_context(), + modeladmin.admin_site.each_context(request), title=title, objects_name=objects_name, deletable_objects=[deletable_objects], diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index d3f4ad16fc..d950de7ffd 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -1461,7 +1461,7 @@ class ModelAdmin(BaseModelAdmin): for inline_formset in inline_formsets: media = media + inline_formset.media - context = dict(self.admin_site.each_context(), + context = dict(self.admin_site.each_context(request), title=(_('Add %s') if add else _('Change %s')) % force_text(opts.verbose_name), adminform=adminForm, object_id=object_id, @@ -1617,7 +1617,7 @@ class ModelAdmin(BaseModelAdmin): 'All %(total_count)s selected', cl.result_count) context = dict( - self.admin_site.each_context(), + self.admin_site.each_context(request), module_name=force_text(opts.verbose_name_plural), selection_note=_('0 of %(cnt)s selected') % {'cnt': len(cl.result_list)}, selection_note_all=selection_note_all % {'total_count': cl.result_count}, @@ -1686,7 +1686,7 @@ class ModelAdmin(BaseModelAdmin): title = _("Are you sure?") context = dict( - self.admin_site.each_context(), + self.admin_site.each_context(request), title=title, object_name=object_name, object=obj, @@ -1720,7 +1720,7 @@ class ModelAdmin(BaseModelAdmin): content_type=get_content_type_for_model(model) ).select_related().order_by('action_time') - context = dict(self.admin_site.each_context(), + context = dict(self.admin_site.each_context(request), title=_('Change history: %s') % force_text(obj), action_list=action_list, module_name=capfirst(force_text(opts.verbose_name_plural)), diff --git a/django/contrib/admin/sites.py b/django/contrib/admin/sites.py index e6a7f641c8..6c6f35f813 100644 --- a/django/contrib/admin/sites.py +++ b/django/contrib/admin/sites.py @@ -272,7 +272,7 @@ class AdminSite(object): def urls(self): return self.get_urls(), 'admin', self.name - def each_context(self): + def each_context(self, request): """ Returns a dictionary of variables to put in the template context for *every* page in the admin site. @@ -281,6 +281,7 @@ class AdminSite(object): 'site_title': self.site_title, 'site_header': self.site_header, 'site_url': self.site_url, + 'has_permission': self.has_permission(request), } def password_change(self, request, extra_context=None): @@ -294,7 +295,7 @@ class AdminSite(object): 'current_app': self.name, 'password_change_form': AdminPasswordChangeForm, 'post_change_redirect': url, - 'extra_context': dict(self.each_context(), **(extra_context or {})), + 'extra_context': dict(self.each_context(request), **(extra_context or {})), } if self.password_change_template is not None: defaults['template_name'] = self.password_change_template @@ -307,7 +308,7 @@ class AdminSite(object): from django.contrib.auth.views import password_change_done defaults = { 'current_app': self.name, - 'extra_context': dict(self.each_context(), **(extra_context or {})), + 'extra_context': dict(self.each_context(request), **(extra_context or {})), } if self.password_change_done_template is not None: defaults['template_name'] = self.password_change_done_template @@ -336,7 +337,7 @@ class AdminSite(object): from django.contrib.auth.views import logout defaults = { 'current_app': self.name, - 'extra_context': dict(self.each_context(), **(extra_context or {})), + 'extra_context': dict(self.each_context(request), **(extra_context or {})), } if self.logout_template is not None: defaults['template_name'] = self.logout_template @@ -357,7 +358,7 @@ class AdminSite(object): # it cannot import models from other applications at the module level, # and django.contrib.admin.forms eventually imports User. from django.contrib.admin.forms import AdminAuthenticationForm - context = dict(self.each_context(), + context = dict(self.each_context(request), title=_('Log in'), app_path=request.get_full_path(), ) @@ -431,7 +432,7 @@ class AdminSite(object): app['models'].sort(key=lambda x: x['name']) context = dict( - self.each_context(), + self.each_context(request), title=self.index_title, app_list=app_list, ) @@ -489,7 +490,7 @@ class AdminSite(object): raise Http404('The requested admin page does not exist.') # Sort the models alphabetically within each app. app_dict['models'].sort(key=lambda x: x['name']) - context = dict(self.each_context(), + context = dict(self.each_context(request), title=_('%(app)s administration') % {'app': app_name}, app_list=[app_dict], app_label=app_label, diff --git a/django/contrib/admin/templates/admin/base.html b/django/contrib/admin/templates/admin/base.html index fca6b349d2..e76f15c540 100644 --- a/django/contrib/admin/templates/admin/base.html +++ b/django/contrib/admin/templates/admin/base.html @@ -24,7 +24,8 @@
{% block branding %}{% endblock %}
- {% if user.is_active and user.is_staff %} + {% block usertools %} + {% if has_permission %}
{% block welcome-msg %} {% trans 'Welcome,' %} @@ -34,9 +35,11 @@ {% if site_url %} {% trans 'View site' %} / {% endif %} - {% url 'django-admindocs-docroot' as docsroot %} - {% if docsroot %} - {% trans 'Documentation' %} / + {% if user.is_active and user.is_staff %} + {% url 'django-admindocs-docroot' as docsroot %} + {% if docsroot %} + {% trans 'Documentation' %} / + {% endif %} {% endif %} {% if user.has_usable_password %} {% trans 'Change password' %} / @@ -45,6 +48,7 @@ {% endblock %}
{% endif %} + {% endblock %} {% block nav-global %}{% endblock %} diff --git a/django/contrib/admin/templates/admin/login.html b/django/contrib/admin/templates/admin/login.html index ce7ac30927..1268297e4b 100644 --- a/django/contrib/admin/templates/admin/login.html +++ b/django/contrib/admin/templates/admin/login.html @@ -5,6 +5,8 @@ {% block bodyclass %}{{ block.super }} login{% endblock %} +{% block usertools %}{% endblock %} + {% block nav-global %}{% endblock %} {% block content_title %}{% endblock %} diff --git a/django/contrib/admindocs/views.py b/django/contrib/admindocs/views.py index 2c77a24f63..2b45301f3a 100644 --- a/django/contrib/admindocs/views.py +++ b/django/contrib/admindocs/views.py @@ -30,16 +30,16 @@ class BaseAdminDocsView(TemplateView): Base view for admindocs views. """ @method_decorator(staff_member_required) - def dispatch(self, *args, **kwargs): + def dispatch(self, request, *args, **kwargs): if not utils.docutils_is_available: # Display an error message for people without docutils self.template_name = 'admin_doc/missing_docutils.html' - return self.render_to_response(admin.site.each_context()) - return super(BaseAdminDocsView, self).dispatch(*args, **kwargs) + return self.render_to_response(admin.site.each_context(request)) + return super(BaseAdminDocsView, self).dispatch(request, *args, **kwargs) def get_context_data(self, **kwargs): kwargs.update({'root_path': urlresolvers.reverse('admin:index')}) - kwargs.update(admin.site.each_context()) + kwargs.update(admin.site.each_context(self.request)) return super(BaseAdminDocsView, self).get_context_data(**kwargs) diff --git a/django/contrib/auth/admin.py b/django/contrib/auth/admin.py index 8b7f80ee39..90394c8f09 100644 --- a/django/contrib/auth/admin.py +++ b/django/contrib/auth/admin.py @@ -157,7 +157,7 @@ class UserAdmin(admin.ModelAdmin): 'save_as': False, 'show_save': True, } - context.update(admin.site.each_context()) + context.update(admin.site.each_context(request)) request.current_app = self.admin_site.name diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt index 12a2c0cc28..70348e5d47 100644 --- a/docs/ref/contrib/admin/index.txt +++ b/docs/ref/contrib/admin/index.txt @@ -2520,6 +2520,24 @@ Templates can override or extend base admin templates as described in ``AdminSite`` methods --------------------- +.. method:: AdminSite.each_context(request): + + .. versionadded:: 1.7 + + Returns a dictionary of variables to put in the template context for + every page in the admin site. + + Includes the following variables and values by default: + + * ``site_header``: :attr:`AdminSite.site_header` + * ``site_title``: :attr:`AdminSite.site_title` + * ``site_url``: :attr:`AdminSite.site_url` + * ``has_permission``: :meth:`AdminSite.has_permission` + + .. versionchanged:: 1.8 + + The ``request`` argument and the ``has_permission`` variable were added. + .. method:: AdminSite.has_permission(request) Returns ``True`` if the user for the given ``HttpRequest`` has permission diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt index c869cbc5b7..02fae70c82 100644 --- a/docs/releases/1.8.txt +++ b/docs/releases/1.8.txt @@ -136,6 +136,15 @@ Minor features * The ``AdminSite.password_change()`` method now has an ``extra_context`` parameter. +* You can now control who may login to the admin site by overriding only + :meth:`AdminSite.has_permission() + ` and + :attr:`AdminSite.login_form `. + The ``base.html`` template has a new block ``usertools`` which contains the + user-specific header. A new context variable ``has_permission``, which gets + its value from :meth:`~django.contrib.admin.AdminSite.has_permission`, + indicates whether the user may access the site. + :mod:`django.contrib.admindocs` ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -899,6 +908,14 @@ Miscellaneous opposed to the instance name which you can still customize using ``AdminSite(name="...")``. +* The block ``usertools`` in the ``base.html`` template of + :mod:`django.contrib.admin` now requires the ``has_permission`` context + variable to be set. If you have any custom admin views that use this + template, update them to pass :meth:`AdminSite.has_permission() + ` as this new variable's + value or simply include :meth:`AdminSite.each_context(request) + ` in the context. + * Internal changes were made to the :class:`~django.forms.ClearableFileInput` widget to allow more customization. The undocumented ``url_markup_template`` attribute was removed in favor of ``template_with_initial``. diff --git a/tests/admin_views/custom_has_permission_admin.py b/tests/admin_views/custom_has_permission_admin.py new file mode 100644 index 0000000000..6c15a9805a --- /dev/null +++ b/tests/admin_views/custom_has_permission_admin.py @@ -0,0 +1,33 @@ +""" +A custom AdminSite for AdminViewPermissionsTest.test_login_has_permission(). +""" +from __future__ import unicode_literals + +from django.contrib import admin +from django.contrib.auth import get_permission_codename +from django.contrib.auth.forms import AuthenticationForm + +from . import models, admin as base_admin + +PERMISSION_NAME = 'admin_views.%s' % get_permission_codename('change', models.Article._meta) + + +class PermissionAdminAuthenticationForm(AuthenticationForm): + def confirm_login_allowed(self, user): + from django import forms + if not user.is_active or not (user.is_staff or user.has_perm(PERMISSION_NAME)): + raise forms.ValidationError('permission denied') + + +class HasPermissionAdmin(admin.AdminSite): + login_form = PermissionAdminAuthenticationForm + + def has_permission(self, request): + return ( + request.user.is_active and + (request.user.is_staff or request.user.has_perm(PERMISSION_NAME)) + ) + + +site = HasPermissionAdmin(name="has_permission_admin") +site.register(models.Article, base_admin.ArticleAdmin) diff --git a/tests/admin_views/fixtures/admin-views-users.xml b/tests/admin_views/fixtures/admin-views-users.xml index 1c85e1c909..0b951b4e28 100644 --- a/tests/admin_views/fixtures/admin-views-users.xml +++ b/tests/admin_views/fixtures/admin-views-users.xml @@ -70,6 +70,20 @@ + + nostaff + No + Staff + nostaff@example.com + sha1$995a3$6011485ea3834267d719b4c801409b8b1ddd0158 + False + True + False + 2007-05-30 13:20:10 + 2007-05-30 13:20:10 + + + Test section diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 9a36c9afe8..fda1531139 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -1091,6 +1091,9 @@ class AdminViewPermissionsTest(TestCase): change_user = User.objects.get(username='changeuser') change_user.user_permissions.add(get_perm(Article, get_permission_codename('change', opts))) + change_user2 = User.objects.get(username='nostaff') + change_user2.user_permissions.add(get_perm(Article, + get_permission_codename('change', opts))) # User who can delete Articles delete_user = User.objects.get(username='deleteuser') @@ -1131,6 +1134,11 @@ class AdminViewPermissionsTest(TestCase): 'username': 'deleteuser', 'password': 'secret', } + self.nostaff_login = { + REDIRECT_FIELD_NAME: '/test_admin/has_permission_admin/', + 'username': 'nostaff', + 'password': 'secret', + } self.joepublic_login = { REDIRECT_FIELD_NAME: '/test_admin/admin/', 'username': 'joepublic', @@ -1211,6 +1219,34 @@ class AdminViewPermissionsTest(TestCase): form = login.context[0].get('form') self.assertEqual(form.errors['username'][0], 'This field is required.') + def test_login_has_permission(self): + # Regular User should not be able to login. + response = self.client.get('/test_admin/has_permission_admin/') + self.assertEqual(response.status_code, 302) + login = self.client.post('/test_admin/has_permission_admin/login/', self.joepublic_login) + self.assertEqual(login.status_code, 200) + self.assertContains(login, 'permission denied') + + # User with permissions should be able to login. + response = self.client.get('/test_admin/has_permission_admin/') + self.assertEqual(response.status_code, 302) + login = self.client.post('/test_admin/has_permission_admin/login/', self.nostaff_login) + self.assertRedirects(login, '/test_admin/has_permission_admin/') + self.assertFalse(login.context) + self.client.get('/test_admin/has_permission_admin/logout/') + + # Staff should be able to login. + response = self.client.get('/test_admin/has_permission_admin/') + self.assertEqual(response.status_code, 302) + login = self.client.post('/test_admin/has_permission_admin/login/', { + REDIRECT_FIELD_NAME: '/test_admin/has_permission_admin/', + 'username': 'deleteuser', + 'password': 'secret', + }) + self.assertRedirects(login, '/test_admin/has_permission_admin/') + self.assertFalse(login.context) + self.client.get('/test_admin/has_permission_admin/logout/') + def test_login_successfully_redirects_to_original_URL(self): response = self.client.get('/test_admin/admin/') self.assertEqual(response.status_code, 302) diff --git a/tests/admin_views/urls.py b/tests/admin_views/urls.py index a9fa71040c..6c2442d520 100644 --- a/tests/admin_views/urls.py +++ b/tests/admin_views/urls.py @@ -1,6 +1,6 @@ from django.conf.urls import include, url -from . import views, customadmin, admin +from . import views, customadmin, custom_has_permission_admin, admin urlpatterns = [ @@ -12,4 +12,5 @@ urlpatterns = [ url(r'^test_admin/admin4/', include(customadmin.simple_site.urls)), url(r'^test_admin/admin5/', include(admin.site2.urls)), url(r'^test_admin/admin7/', include(admin.site7.urls)), + url(r'^test_admin/has_permission_admin/', include(custom_has_permission_admin.site.urls)), ]