From c09a7fc1ae8b4ae24e79046ddbaa1c2b745ce8cb Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Tue, 21 Aug 2018 14:54:16 +0200 Subject: [PATCH] Allow users with Admin:Advanced to delete users with relations to other models Note that the behaviour depends on which model is affected - some are kept, others are deleted because of UserProfile.delete_or_disable_related_content(). --- src/olympia/addons/admin.py | 4 ++ src/olympia/constants/permissions.py | 10 +++++ src/olympia/users/models.py | 2 + src/olympia/users/tests/test_admin.py | 63 ++++++++++++++++++++++++++- 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/olympia/addons/admin.py b/src/olympia/addons/admin.py index 60791bcf5a..fc7d6801c9 100644 --- a/src/olympia/addons/admin.py +++ b/src/olympia/addons/admin.py @@ -73,6 +73,10 @@ class AddonAdmin(admin.ModelAdmin): status_with_admin_manage_link.short_description = _(u'Status') +class AddonUserAdmin(admin.ModelAdmin): + raw_id_fields = ('addon', 'user') + + class FeatureAdmin(admin.ModelAdmin): raw_id_fields = ('addon',) list_filter = ('application', 'locale') diff --git a/src/olympia/constants/permissions.py b/src/olympia/constants/permissions.py index aafe9c6df3..74400841ba 100644 --- a/src/olympia/constants/permissions.py +++ b/src/olympia/constants/permissions.py @@ -84,7 +84,14 @@ PERMISSIONS_LIST = [ DJANGO_PERMISSIONS_MAPPING = defaultdict(lambda: SUPERPOWERS) DJANGO_PERMISSIONS_MAPPING.update({ + 'abuse.delete_abusereport': ADMIN_ADVANCED, + # Note that ActivityLog's ModelAdmin actually forbids deletion entirely. + # This is just here to allow deletion of users, because django checks + # foreign keys even though users are only soft-deleted and related objects + # will be kept. + 'activity.delete_activitylog': ADMIN_ADVANCED, 'addons.change_addon': ADDONS_EDIT, + 'addons.delete_addonuser': ADMIN_ADVANCED, # Users with Admin:Curation can do anything to ReplacementAddon. # In addition, the modeladmin will also check for Addons:Edit and give them # read-only access to the changelist (obj=None passed to the @@ -100,8 +107,11 @@ DJANGO_PERMISSIONS_MAPPING.update({ 'discovery.change_discoveryitem': DISCOVERY_EDIT, 'discovery.delete_discoveryitem': DISCOVERY_EDIT, + 'reviewers.delete_reviewerscore': ADMIN_ADVANCED, + 'users.change_userprofile': USERS_EDIT, 'users.delete_userprofile': ADMIN_ADVANCED, 'ratings.change_rating': RATINGS_MODERATE, + 'ratings.delete_rating': ADMIN_ADVANCED, }) diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index 2051b625c9..228b07c036 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -419,6 +419,8 @@ class UserProfile(OnChangeMixin, ModelBase, AbstractBaseUser): addon.delete() else: addon.force_disable() + else: + addon.addonuser_set.filter(user=self).delete() user_responsible = core.get_user() self._ratings_all.all().delete(user_responsible=user_responsible) self.delete_picture() diff --git a/src/olympia/users/tests/test_admin.py b/src/olympia/users/tests/test_admin.py index 612032607c..c45d6151e0 100644 --- a/src/olympia/users/tests/test_admin.py +++ b/src/olympia/users/tests/test_admin.py @@ -10,15 +10,16 @@ import mock from pyquery import PyQuery as pq - from olympia import amo, core from olympia.abuse.models import AbuseReport from olympia.activity.models import ActivityLog +from olympia.addons.models import AddonUser from olympia.amo.tests import ( - addon_factory, TestCase, user_factory, version_factory) + addon_factory, collection_factory, TestCase, user_factory, version_factory) from olympia.amo.urlresolvers import reverse from olympia.bandwagon.models import Collection from olympia.ratings.models import Rating +from olympia.reviewers.models import ReviewerScore from olympia.users.admin import UserAdmin from olympia.users.models import UserProfile @@ -95,6 +96,7 @@ class TestUserAdmin(TestCase): core.set_user(user) response = self.client.get(self.delete_url, follow=True) assert response.status_code == 200 + assert 'Cannot delete user' not in response.content response = self.client.post(self.delete_url, {'post': 'yes'}, follow=True) assert response.status_code == 200 @@ -109,6 +111,63 @@ class TestUserAdmin(TestCase): assert alog.action == amo.LOG.ADMIN_USER_ANONYMIZED.id assert alog.arguments == [self.user] + def test_can_delete_with_related_objects_with_admin_advanced_permission( + self): + # Add related instances... + addon = addon_factory() + addon_with_other_authors = addon_factory() + AddonUser.objects.create( + addon=addon_with_other_authors, user=user_factory()) + relations_that_should_be_deleted = [ + AddonUser.objects.create( + addon=addon_with_other_authors, user=self.user), + Rating.objects.create( + addon=addon_factory(), rating=5, user=self.user), + addon, # Has no other author, should be deleted. + collection_factory(author=self.user) + ] + relations_that_should_survive = [ + AbuseReport.objects.create(reporter=self.user), + AbuseReport.objects.create(user=self.user), + ActivityLog.create(user=self.user, action=amo.LOG.USER_EDITED), + ReviewerScore.objects.create(user=self.user, score=42), + addon_with_other_authors, # Has other authors, should be kept. + + # Bit of a weird case, but because the user was the only author of + # this add-on, the addonuser relation is kept, and both the add-on + # and the user are soft-deleted. This is in contrast with the case + # where the user is *not* the only author, in which case the + # addonuser relation is deleted, but the add-on is left intact. + AddonUser.objects.create(addon=addon, user=self.user), + ] + + # Now test as normal. + user = user_factory() + assert not self.user.deleted + self.grant_permission(user, 'Admin:Tools') + self.grant_permission(user, 'Admin:Advanced') + self.client.login(email=user.email) + core.set_user(user) + response = self.client.get(self.delete_url, follow=True) + assert response.status_code == 200 + assert 'Cannot delete user' not in response.content + response = self.client.post(self.delete_url, {'post': 'yes'}, + follow=True) + assert response.status_code == 200 + self.user.reload() + assert self.user.deleted + assert self.user.email is None + alog = ActivityLog.objects.filter( + action=amo.LOG.ADMIN_USER_ANONYMIZED.id).get() + assert alog.arguments == [self.user] + + # Test the related instances we created earlier. + for obj in relations_that_should_be_deleted: + assert not obj.__class__.objects.filter(pk=obj.pk).exists() + + for obj in relations_that_should_survive: + assert obj.__class__.objects.filter(pk=obj.pk).exists() + def test_get_actions(self): user_admin = UserAdmin(UserProfile, admin.site) request = RequestFactory().get('/')