From 958c7b301ead79974db8edd5b9c6588a10a28ae7 Mon Sep 17 00:00:00 2001 From: Carlton Gibson Date: Mon, 18 Jun 2018 21:07:29 +0200 Subject: [PATCH] Fixed #29419 -- Allowed permissioning of admin actions. --- django/contrib/admin/actions.py | 5 +-- django/contrib/admin/checks.py | 27 ++++++++++++++ django/contrib/admin/options.py | 40 +++++++++++++++------ docs/ref/checks.txt | 2 ++ docs/ref/contrib/admin/actions.txt | 49 +++++++++++++++++++++++++ docs/releases/2.1.txt | 3 ++ tests/admin_views/test_actions.py | 7 ++-- tests/modeladmin/test_actions.py | 57 ++++++++++++++++++++++++++++++ tests/modeladmin/test_checks.py | 19 ++++++++++ 9 files changed, 192 insertions(+), 17 deletions(-) create mode 100644 tests/modeladmin/test_actions.py diff --git a/django/contrib/admin/actions.py b/django/contrib/admin/actions.py index f64b89205e..1e1c3bd384 100644 --- a/django/contrib/admin/actions.py +++ b/django/contrib/admin/actions.py @@ -23,10 +23,6 @@ def delete_selected(modeladmin, request, queryset): opts = modeladmin.model._meta app_label = opts.app_label - # Check that the user has delete permission for the actual model - if not modeladmin.has_delete_permission(request): - raise PermissionDenied - # Populate deletable_objects, a data structure of all related objects that # will also be deleted. deletable_objects, model_count, perms_needed, protected = modeladmin.get_deleted_objects(queryset, request) @@ -79,4 +75,5 @@ def delete_selected(modeladmin, request, queryset): ], context) +delete_selected.allowed_permissions = ('delete',) delete_selected.short_description = gettext_lazy("Delete selected %(verbose_name_plural)s") diff --git a/django/contrib/admin/checks.py b/django/contrib/admin/checks.py index cf5b312369..8dd5a72256 100644 --- a/django/contrib/admin/checks.py +++ b/django/contrib/admin/checks.py @@ -579,6 +579,7 @@ class ModelAdminChecks(BaseModelAdminChecks): *self._check_list_editable(admin_obj), *self._check_search_fields(admin_obj), *self._check_date_hierarchy(admin_obj), + *self._check_action_permission_methods(admin_obj), ] def _check_save_as(self, obj): @@ -891,6 +892,32 @@ class ModelAdminChecks(BaseModelAdminChecks): else: return [] + def _check_action_permission_methods(self, obj): + """ + Actions with an allowed_permission attribute require the ModelAdmin to + implement a has__permission() method for each permission. + """ + actions = obj._get_base_actions() + errors = [] + for func, name, _ in actions: + if not hasattr(func, 'allowed_permissions'): + continue + for permission in func.allowed_permissions: + method_name = 'has_%s_permission' % permission + if not hasattr(obj, method_name): + errors.append( + checks.Error( + '%s must define a %s() method for the %s action.' % ( + obj.__class__.__name__, + method_name, + func.__name__, + ), + obj=obj.__class__, + id='admin.E129', + ) + ) + return errors + class InlineModelAdminChecks(BaseModelAdminChecks): diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 62c881eb74..0f4dd93cb3 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -852,16 +852,8 @@ class ModelAdmin(BaseModelAdmin): return helpers.checkbox.render(helpers.ACTION_CHECKBOX_NAME, str(obj.pk)) action_checkbox.short_description = mark_safe('') - def get_actions(self, request): - """ - Return a dictionary mapping the names of all actions for this - ModelAdmin to a tuple of (callable, name, description) for each action. - """ - # If self.actions is explicitly set to None that means that we don't - # want *any* actions enabled on this page. - if self.actions is None or IS_POPUP_VAR in request.GET: - return OrderedDict() - + def _get_base_actions(self): + """Return the list of actions, prior to any request-based filtering.""" actions = [] # Gather actions from the admin site first @@ -876,8 +868,34 @@ class ModelAdmin(BaseModelAdmin): actions.extend(self.get_action(action) for action in class_actions) # get_action might have returned None, so filter any of those out. - actions = filter(None, actions) + return filter(None, actions) + def _filter_actions_by_permissions(self, request, actions): + """Filter out any actions that the user doesn't have access to.""" + filtered_actions = [] + for action in actions: + callable = action[0] + if not hasattr(callable, 'allowed_permissions'): + filtered_actions.append(action) + continue + permission_checks = ( + getattr(self, 'has_%s_permission' % permission) + for permission in callable.allowed_permissions + ) + if any(has_permission(request) for has_permission in permission_checks): + filtered_actions.append(action) + return filtered_actions + + def get_actions(self, request): + """ + Return a dictionary mapping the names of all actions for this + ModelAdmin to a tuple of (callable, name, description) for each action. + """ + # If self.actions is set to None that means actions are disabled on + # this page. + if self.actions is None or IS_POPUP_VAR in request.GET: + return OrderedDict() + actions = self._filter_actions_by_permissions(request, self._get_base_actions()) # Convert the actions into an OrderedDict keyed by name. return OrderedDict( (name, (func, name, desc)) diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index a85c6b4e6e..b0d7dfc066 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -579,6 +579,8 @@ with the admin site: which does not refer to a Field. * **admin.E128**: The value of ``date_hierarchy`` must be a ``DateField`` or ``DateTimeField``. +* **admin.E129**: ```` must define a ``has__permission()`` + method for the ```` action. ``InlineModelAdmin`` ~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/ref/contrib/admin/actions.txt b/docs/ref/contrib/admin/actions.txt index 0eb6de5b11..82ce1e6f92 100644 --- a/docs/ref/contrib/admin/actions.txt +++ b/docs/ref/contrib/admin/actions.txt @@ -357,3 +357,52 @@ Conditionally enabling or disabling actions if 'delete_selected' in actions: del actions['delete_selected'] return actions + +.. _admin-action-permissions: + +Setting permissions for actions +------------------------------- + +.. versionadded:: 2.1 + +Actions may limit their availability to users with specific permissions by +setting an ``allowed_permissions`` attribute on the action function:: + + def make_published(modeladmin, request, queryset): + queryset.update(status='p') + make_published.allowed_permissions = ('change',) + +The ``make_published()`` action will only be available to users that pass the +:meth:`.ModelAdmin.has_change_permission` check. + +If ``allowed_permissions`` has more than one permission, the action will be +available as long as the user passes at least one of the checks. + +Available values for ``allowed_permissions`` and the corresponding method +checks are: + +- ``'add'``: :meth:`.ModelAdmin.has_add_permission` +- ``'change'``: :meth:`.ModelAdmin.has_change_permission` +- ``'delete'``: :meth:`.ModelAdmin.has_delete_permission` +- ``'view'``: :meth:`.ModelAdmin.has_view_permission` + +You can specify any other value as long as you implement a corresponding +``has__permission(self, request)`` method on the ``ModelAdmin``. + +For example:: + + from django.contrib import admin + from django.contrib.auth import get_permission_codename + + class ArticleAdmin(admin.ModelAdmin): + actions = ['make_published'] + + def make_published(self, request, queryset): + queryset.update(status='p') + make_published.allowed_permissions = ('publish',) + + def has_publish_permission(self, request): + """Does the user have the publish permission?""" + opts = self.opts + codename = get_permission_codename('publish', opts) + return request.user.has_perm('%s.%s' % (opts.app_label, codename)) diff --git a/docs/releases/2.1.txt b/docs/releases/2.1.txt index 81f112b464..c68f673d84 100644 --- a/docs/releases/2.1.txt +++ b/docs/releases/2.1.txt @@ -82,6 +82,9 @@ Minor features * :meth:`.InlineModelAdmin.has_add_permission` is now passed the parent object as the second positional argument, ``obj``. +* Admin actions may now :ref:`specify permissions ` + to limit their availability to certain users. + :mod:`django.contrib.auth` ~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/admin_views/test_actions.py b/tests/admin_views/test_actions.py index 423c2f927f..c0ea30fcf7 100644 --- a/tests/admin_views/test_actions.py +++ b/tests/admin_views/test_actions.py @@ -429,8 +429,11 @@ class AdminActionsPermissionTests(TestCase): ACTION_CHECKBOX_NAME: [self.s1.pk], 'action': 'delete_selected', } - response = self.client.post(reverse('admin:admin_views_subscriber_changelist'), action_data) - self.assertEqual(response.status_code, 403) + url = reverse('admin:admin_views_subscriber_changelist') + response = self.client.post(url, action_data) + self.assertRedirects(response, url, fetch_redirect_response=False) + response = self.client.get(response.url) + self.assertContains(response, 'No action selected.') def test_model_admin_no_delete_permission_externalsubscriber(self): """ diff --git a/tests/modeladmin/test_actions.py b/tests/modeladmin/test_actions.py new file mode 100644 index 0000000000..17b3c324d2 --- /dev/null +++ b/tests/modeladmin/test_actions.py @@ -0,0 +1,57 @@ +from django.contrib import admin +from django.contrib.auth.models import Permission, User +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase + +from .models import Band + + +class AdminActionsTests(TestCase): + + @classmethod + def setUpTestData(cls): + cls.superuser = User.objects.create_superuser(username='super', password='secret', email='super@example.com') + content_type = ContentType.objects.get_for_model(Band) + Permission.objects.create(name='custom', codename='custom_band', content_type=content_type) + for user_type in ('view', 'add', 'change', 'delete', 'custom'): + username = '%suser' % user_type + user = User.objects.create_user(username=username, password='secret', is_staff=True) + permission = Permission.objects.get(codename='%s_band' % user_type, content_type=content_type) + user.user_permissions.add(permission) + setattr(cls, username, user) + + def test_get_actions_respects_permissions(self): + class MockRequest: + pass + + class BandAdmin(admin.ModelAdmin): + actions = ['custom_action'] + + def custom_action(modeladmin, request, queryset): + pass + + def has_custom_permission(self, request): + return request.user.has_perm('%s.custom_band' % self.opts.app_label) + + ma = BandAdmin(Band, admin.AdminSite()) + mock_request = MockRequest() + mock_request.GET = {} + cases = [ + (None, self.viewuser, ['custom_action']), + ('view', self.superuser, ['delete_selected', 'custom_action']), + ('view', self.viewuser, ['custom_action']), + ('add', self.adduser, ['custom_action']), + ('change', self.changeuser, ['custom_action']), + ('delete', self.deleteuser, ['delete_selected', 'custom_action']), + ('custom', self.customuser, ['custom_action']), + ] + for permission, user, expected in cases: + with self.subTest(permission=permission, user=user): + if permission is None: + if hasattr(BandAdmin.custom_action, 'allowed_permissions'): + del BandAdmin.custom_action.allowed_permissions + else: + BandAdmin.custom_action.allowed_permissions = (permission,) + mock_request.user = user + actions = ma.get_actions(mock_request) + self.assertEqual(list(actions.keys()), expected) diff --git a/tests/modeladmin/test_checks.py b/tests/modeladmin/test_checks.py index 5a0433deb4..6a10441471 100644 --- a/tests/modeladmin/test_checks.py +++ b/tests/modeladmin/test_checks.py @@ -1290,3 +1290,22 @@ class AutocompleteFieldsTests(CheckTestCase): site = AdminSite() site.register(User, UserAdmin) self.assertIsValid(Admin, ValidationTestModel, admin_site=site) + + +class ActionsCheckTests(CheckTestCase): + + def test_custom_permissions_require_matching_has_method(self): + def custom_permission_action(modeladmin, request, queryset): + pass + + custom_permission_action.allowed_permissions = ('custom',) + + class BandAdmin(ModelAdmin): + actions = (custom_permission_action,) + + self.assertIsInvalid( + BandAdmin, Band, + 'BandAdmin must define a has_custom_permission() method for the ' + 'custom_permission_action action.', + id='admin.E129', + )