Skip to content

Commit

Permalink
[fix] Admin actions require model permissions
Browse files Browse the repository at this point in the history
  • Loading branch information
pandafy committed Jul 2, 2024
1 parent bd0cad9 commit 202ec0c
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 11 deletions.
5 changes: 4 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ jobs:
run: pip install -e .[rest]

- name: Change Django Version for test
run: pip install -U ${{ matrix.django-version }}
run: |
pip install -U ${{ matrix.django-version }}
# TODO: Remove before merging
pip install --force-reinstall --no-cache-dir --no-deps --upgrade https://github.com/openwisp/openwisp-utils/tarball/admin-action-perm
- name: QA checks
run: |
Expand Down
16 changes: 8 additions & 8 deletions openwisp_users/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,9 @@ def wrapper(modeladmin, request, queryset):
wrapper.__name__ = func.__name__
return wrapper

@admin.action(
description=_('Flag selected users as inactive'), permissions=['change']
)
@require_confirmation
def make_inactive(self, request, queryset):
queryset.update(is_active=False)
Expand All @@ -253,8 +256,9 @@ def make_inactive(self, request, queryset):
messages.SUCCESS,
)

make_inactive.short_description = _('Flag selected users as inactive')

@admin.action(
description=_('Flag selected users as active'), permissions=['change']
)
@require_confirmation
def make_active(self, request, queryset):
queryset.update(is_active=True)
Expand All @@ -269,8 +273,6 @@ def make_active(self, request, queryset):
messages.SUCCESS,
)

make_active.short_description = _('Flag selected users as active')

def get_list_display(self, request):
"""
Hide `is_superuser` from column from operators
Expand Down Expand Up @@ -342,6 +344,7 @@ def get_actions(self, request):
del actions['delete_selected']
return actions

@admin.action(description=delete_selected.short_description, permissions=['delete'])
def delete_selected_overridden(self, request, queryset):
if not request.user.is_superuser:
users_pk = queryset.values_list('pk', flat=True)
Expand Down Expand Up @@ -376,8 +379,6 @@ def delete_selected_overridden(self, request, queryset):
queryset = excluded_owners_qs
return delete_selected(self, request, queryset)

delete_selected_overridden.short_description = delete_selected.short_description

def get_inline_instances(self, request, obj=None):
"""
1. Avoid displaying inline objects when adding a new user
Expand Down Expand Up @@ -581,6 +582,7 @@ def get_actions(self, request):
del actions['delete_selected']
return actions

@admin.action(description=delete_selected.short_description, permissions=['delete'])
def delete_selected_overridden(self, request, queryset):
count = 0
pks = []
Expand Down Expand Up @@ -616,8 +618,6 @@ def delete_selected_overridden(self, request, queryset):
# otherwise proceed but remove org users from the delete queryset
return delete_selected(self, request, queryset)

delete_selected_overridden.short_description = delete_selected.short_description


class OrganizationOwnerAdmin(
MultitenantAdminMixin, BaseOrganizationOwnerAdmin, BaseAdmin
Expand Down
100 changes: 98 additions & 2 deletions openwisp_users/tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from django.test import TestCase, override_settings
from django.urls import reverse
from django.utils.timezone import now, timedelta
from openwisp_utils.tests import capture_any_output
from openwisp_utils.tests import AdminActionPermTestMixin, capture_any_output
from swapper import load_model

from .. import settings as app_settings
Expand All @@ -34,7 +34,12 @@
Group = load_model('openwisp_users', 'Group')


class TestUsersAdmin(TestOrganizationMixin, TestUserAdditionalFieldsMixin, TestCase):
class TestUsersAdmin(
AdminActionPermTestMixin,
TestOrganizationMixin,
TestUserAdditionalFieldsMixin,
TestCase,
):
"""test admin site"""

app_label = 'openwisp_users'
Expand Down Expand Up @@ -1089,6 +1094,27 @@ def test_action_active(self):
response = self.client.post(path, post_data, follow=True)
self.assertEqual(response.status_code, 200)

def test_action_active_perms(self):
org = self._get_org()
org_user = self._create_org_user(
organization=org, is_admin=True, user=self._create_user(is_staff=True)
).user
user_obj = self._create_org_user(
organization=org,
user=self._create_user(
username='active-user', email='[email protected]'
),
).user
self._test_action_permission(
path=reverse(f'admin:{self.app_label}_user_changelist'),
action='make_active',
user=org_user,
obj=user_obj,
message='Successfully made 1 user active.',
required_perms=['change'],
extra_payload={'confirmation': True},
)

def test_action_inactive(self):
user = User.objects.create(
username='openwisp',
Expand All @@ -1109,6 +1135,27 @@ def test_action_inactive(self):
self.assertFalse(user.is_active)
self.assertEqual(response.status_code, 200)

def test_action_inactive_perms(self):
org = self._get_org()
org_user = self._create_org_user(
organization=org, is_admin=True, user=self._create_user(is_staff=True)
).user
user_obj = self._create_org_user(
organization=org,
user=self._create_user(
username='active-user', email='[email protected]'
),
).user
self._test_action_permission(
path=reverse(f'admin:{self.app_label}_user_changelist'),
action='make_inactive',
user=org_user,
obj=user_obj,
message='Successfully made 1 user inactive.',
required_perms=['change'],
extra_payload={'confirmation': True},
)

def test_action_confirmation_page(self):
user = User.objects.create(
username='openwisp',
Expand Down Expand Up @@ -1146,6 +1193,34 @@ def test_superuser_delete_operator(self):
self.assertEqual(user_qs.count(), 0)
self.assertEqual(org_user_qs.count(), 0)

def test_delete_selected_overridden_action_perms(self):
org = self._get_org()
org_user = self._create_org_user(
organization=org, is_admin=True, user=self._create_user(is_staff=True)
).user
# Deleting user would require deleting OrganizationUser.
# Thus, add permission to delete OrganizationUser
org_user.user_permissions.add(
Permission.objects.get(
codename=f'delete_{OrganizationUser._meta.model_name}'
)
)
user_obj = self._create_org_user(
organization=org,
user=self._create_user(
username='delete-user', email='[email protected]'
),
).user
self._test_action_permission(
path=reverse(f'admin:{self.app_label}_user_changelist'),
action='delete_selected_overridden',
user=org_user,
obj=user_obj,
message='Successfully deleted 1 user.',
required_perms=['delete'],
extra_payload={'post': 'yes'},
)

def test_staff_delete_staff(self):
org = self._create_org()
staff = self._create_user(
Expand Down Expand Up @@ -1552,6 +1627,27 @@ def test_delete_org_user(self):
self.assertEqual(qs.count(), 1)
self.assertEqual(qs.first().organization, org1)

def test_delete_selected_overridden_org_user_action_perms(self):
org = self._get_org()
user = self._create_org_user(
organization=org, is_admin=True, user=self._create_user(is_staff=True)
).user
org_user_obj = self._create_org_user(
organization=org,
user=self._create_user(
username='delete-user', email='[email protected]'
),
)
self._test_action_permission(
path=reverse(f'admin:{self.app_label}_organizationuser_changelist'),
action='delete_selected_overridden',
user=user,
obj=org_user_obj,
message='Successfully deleted 1 organization user.',
required_perms=['delete'],
extra_payload={'post': 'yes'},
)

@capture_any_output()
def test_admin_add_user_with_invalid_email(self):
admin = self._create_admin()
Expand Down

0 comments on commit 202ec0c

Please sign in to comment.