Skip to content

Add some basic spam removal features to admin #3131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 5, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions readthedocs/core/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ def is_banned(self, obj):
return hasattr(obj, 'profile') and obj.profile.banned

is_banned.short_description = 'Banned'
is_banned.boolean = True

def ban_user(self, request, queryset):
users = []
Expand Down
75 changes: 73 additions & 2 deletions readthedocs/projects/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,21 @@

from __future__ import absolute_import
from django.contrib import admin
from django.contrib import messages
from django.contrib.admin.actions import delete_selected
from django.utils.translation import ugettext_lazy as _
from guardian.admin import GuardedModelAdmin

from readthedocs.core.models import UserProfile
from readthedocs.core.utils import broadcast
from readthedocs.builds.models import Version
from readthedocs.redirects.models import Redirect
from readthedocs.notifications.views import SendNotificationView

from .notifications import ResourceUsageNotification
from .models import (Project, ImportedFile,
ProjectRelationship, EmailHook, WebHook, Domain)
from .tasks import remove_dir


class ProjectSendNotificationView(SendNotificationView):
Expand Down Expand Up @@ -65,20 +71,45 @@ class DomainInline(admin.TabularInline):
# return instance.click_ratio * 100


class ProjectOwnerBannedFilter(admin.SimpleListFilter):

"""Filter for projects with banned owners

There are problems adding `users__profile__banned` to the `list_filter`
attribute, so we'll create a basic filter to capture banned owners.
"""

title = 'project owner banned'
parameter_name = 'project_owner_banned'

OWNER_BANNED = 'true'

def lookups(self, request, model_admin):
return (
(self.OWNER_BANNED, _('Yes')),
)

def queryset(self, request, queryset):
if self.value() == self.OWNER_BANNED:
return queryset.filter(users__profile__banned=True)
return queryset


class ProjectAdmin(GuardedModelAdmin):

"""Project model admin view"""

prepopulated_fields = {'slug': ('name',)}
list_display = ('name', 'repo', 'repo_type', 'allow_comments', 'featured', 'theme')
list_filter = ('repo_type', 'allow_comments', 'featured', 'privacy_level',
'documentation_type', 'programming_language')
'documentation_type', 'programming_language',
ProjectOwnerBannedFilter)
list_editable = ('featured',)
search_fields = ('slug', 'repo')
inlines = [ProjectRelationshipInline, RedirectInline,
VersionInline, DomainInline]
raw_id_fields = ('users', 'main_language_project')
actions = ['send_owner_email']
actions = ['send_owner_email', 'ban_owner']

def send_owner_email(self, request, queryset):
view = ProjectSendNotificationView.as_view(
Expand All @@ -88,6 +119,46 @@ def send_owner_email(self, request, queryset):

send_owner_email.short_description = 'Notify project owners'

def ban_owner(self, request, queryset):
"""Ban project owner

This will only ban single owners, because a malicious user could add a
user as a co-owner of the project. We don't want to induce and
collatoral damage when flagging users.
"""
total = 0
for project in queryset:
if project.users.count() == 1:
count = (UserProfile.objects
.filter(user__projects=project)
.update(banned=True))
total += count
else:
messages.add_message(request, messages.ERROR,
'Project has multiple owners: {0}'.format(project))
if total == 0:
messages.add_message(request, messages.ERROR, 'No users banned')
else:
messages.add_message(request, messages.INFO,
'Banned {0} user(s)'.format(total))

ban_owner.short_description = 'Ban project owner'

def delete_selected_and_artifacts(self, request, queryset):
if request.POST.get('post'):
for project in queryset:
broadcast(type='app', task=remove_dir, args=[project.doc_path])
return delete_selected(self, request, queryset)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add a docstring about why we're doing this. To clean up left over stuff from build servers? We could also just call .delete() directly, which should trigger the logic that does this, instead of special casing this one cleanup task.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The admin calls .delete() on the Project queryset, bulking the operations, this was to avoid messing with any of the logic there.

Project also doesn't have a delete method that does this currently, because we need to make a decision on whether a user deleting a project should remove the files from webs.


def get_actions(self, request):
actions = super(ProjectAdmin, self).get_actions(request)
actions['delete_selected'] = (
self.__class__.delete_selected_and_artifacts,
'delete_selected',
delete_selected.short_description
)
return actions


class ImportedFileAdmin(admin.ModelAdmin):

Expand Down
5 changes: 5 additions & 0 deletions readthedocs/projects/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,8 @@ def make_api_project(project_data):
project = Project(**project_data)
project.save = _new_save
return project


def delete_project(project):
broadcast(type='app', task=tasks.remove_dir, args=[project.doc_path])
project.delete()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant. Isn't this already called on project.delete()? If not, it should be.

Empty file.
80 changes: 80 additions & 0 deletions readthedocs/rtd_tests/tests/projects/test_admin_actions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import mock
import django_dynamic_fixture as fixture
from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME
from django.contrib.auth.models import User
from django.core import urlresolvers
from django.test import TestCase

from readthedocs.core.models import UserProfile
from readthedocs.projects.models import Project


class ProjectAdminActionsTest(TestCase):

@classmethod
def setUpTestData(cls):
cls.owner = fixture.get(User)
cls.profile = fixture.get(UserProfile, user=cls.owner, banned=False)
cls.admin = fixture.get(User, is_staff=True, is_superuser=True)
cls.project = fixture.get(
Project,
main_language_project=None,
users=[cls.owner],
)

def setUp(self):
self.client.force_login(self.admin)

def test_project_ban_owner(self):
self.assertFalse(self.owner.profile.banned)
action_data = {
ACTION_CHECKBOX_NAME: [self.project.pk],
'action': 'ban_owner',
'index': 0,
}
resp = self.client.post(
urlresolvers.reverse('admin:projects_project_changelist'),
action_data
)
self.assertTrue(self.project.users.filter(profile__banned=True).exists())
self.assertFalse(self.project.users.filter(profile__banned=False).exists())

def test_project_ban_multiple_owners(self):
owner_b = fixture.get(User)
profile_b = fixture.get(UserProfile, user=owner_b, banned=False)
self.project.users.add(owner_b)
self.assertFalse(self.owner.profile.banned)
self.assertFalse(owner_b.profile.banned)
action_data = {
ACTION_CHECKBOX_NAME: [self.project.pk],
'action': 'ban_owner',
'index': 0,
}
resp = self.client.post(
urlresolvers.reverse('admin:projects_project_changelist'),
action_data
)
self.assertFalse(self.project.users.filter(profile__banned=True).exists())
self.assertEqual(self.project.users.filter(profile__banned=False).count(), 2)

@mock.patch('readthedocs.projects.admin.remove_dir')
def test_project_delete(self, remove_dir):
"""Test project and artifacts are removed"""
action_data = {
ACTION_CHECKBOX_NAME: [self.project.pk],
'action': 'delete_selected',
'index': 0,
'post': 'yes',
}
resp = self.client.post(
urlresolvers.reverse('admin:projects_project_changelist'),
action_data
)
self.assertFalse(Project.objects.filter(pk=self.project.pk).exists())
remove_dir.apply_async.assert_has_calls([
mock.call(
kwargs={},
queue='celery',
args=[self.project.doc_path]
),
])