Skip to content

Spam: deny dashboard on spammy projects #8792

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 1 commit into from
Jan 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 0 additions & 2 deletions pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ markers =
python_files = tests.py test_*.py *_tests.py
filterwarnings =
# Ignore external dependencies warning deprecations
# textclassifier
ignore:The 'warn' method is deprecated, use 'warning' instead:DeprecationWarning
# django-rest-framework
ignore:Pagination may yield inconsistent results with an unordered object_list.*:django.core.paginator.UnorderedObjectListWarning
# docutils
Expand Down
10 changes: 0 additions & 10 deletions readthedocs/projects/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,3 @@ def get_default_message(self):
if settings.ALLOW_PRIVATE_REPOS:
return self.PRIVATE_ALLOWED
return self.PRIVATE_NOT_ALLOWED


class ProjectSpamError(Exception):

"""
Error raised when a project field has detected spam.

This error is not raised to users, we use this for banning users in the
background.
"""
3 changes: 0 additions & 3 deletions readthedocs/projects/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,13 @@
from django.contrib.auth.models import User
from django.template.loader import render_to_string
from django.utils.translation import ugettext_lazy as _
from textclassifier.validators import ClassifierValidator

from readthedocs.builds.constants import INTERNAL
from readthedocs.core.history import SimpleHistoryModelForm
from readthedocs.core.utils import slugify, trigger_build
from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.integrations.models import Integration
from readthedocs.oauth.models import RemoteRepository
from readthedocs.projects.exceptions import ProjectSpamError
from readthedocs.projects.models import (
Domain,
EmailHook,
Expand Down Expand Up @@ -173,7 +171,6 @@ class Meta:
)

description = forms.CharField(
validators=[ClassifierValidator(raises=ProjectSpamError)],
required=False,
max_length=150,
widget=forms.Textarea,
Expand Down
42 changes: 15 additions & 27 deletions readthedocs/projects/views/base.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
"""Mix-in classes for project views."""
import structlog
from datetime import timedelta
from functools import lru_cache

from django.conf import settings
from django.http import HttpResponseRedirect
from django.shortcuts import get_object_or_404
from django.shortcuts import render, get_object_or_404
from django.urls import reverse
from django.utils import timezone

from readthedocs.projects.exceptions import ProjectSpamError
from readthedocs.projects.models import Project

log = structlog.get_logger(__name__)
Expand Down Expand Up @@ -86,26 +83,17 @@ def get_form(self, data=None, files=None, **kwargs):

class ProjectSpamMixin:

"""Protects POST views from spammers."""

def post(self, request, *args, **kwargs):
log.bind(user_username=request.user.username)
if request.user.profile.banned:
log.info('Rejecting project POST from shadowbanned user.')
return HttpResponseRedirect(self.get_failure_url())
try:
return super().post(request, *args, **kwargs)
except ProjectSpamError:
date_maturity = timezone.now() - timedelta(
days=settings.USER_MATURITY_DAYS
)
if request.user.date_joined > date_maturity:
request.user.profile.banned = True
Copy link
Member

Choose a reason for hiding this comment

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

So we will only have banned users manually now, correct? That seems fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I didn't know about this code and I was surprised when I saw it yesterday. I think it wasn't working, tho. We can get back to "auto ban" in the future if we find a reliable way of doing it.

request.user.profile.save()
log.info('Spam detected from new user, shadowbanned user.')
else:
log.info('Spam detected from user.')
return HttpResponseRedirect(self.get_failure_url())

def get_failure_url(self):
return reverse('homepage')
"""
Protects views for spammy projects.

It shows a ``Project marked as spam`` page and return 410 GONE if the
project's dashboard is denied.
"""

def get(self, request, *args, **kwargs):
if 'readthedocsext.spamfighting' in settings.INSTALLED_APPS:
from readthedocsext.spamfighting.utils import is_show_dashboard_denied # noqa
if is_show_dashboard_denied(self.get_project()):
return render(request, template_name='spam.html', status=410)

return super().get(request, *args, **kwargs)
11 changes: 11 additions & 0 deletions readthedocs/projects/views/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,17 @@
from celery import chain
from django.shortcuts import get_object_or_404

import structlog

from readthedocs.core.resolver import resolve, resolve_path
from readthedocs.core.utils import prepare_build
from readthedocs.projects.models import Project
from readthedocs.projects.signals import project_import


log = structlog.get_logger(__name__)


class ProjectRelationMixin:

"""
Expand Down Expand Up @@ -117,6 +122,12 @@ def finish_import_project(self, request, project, tags=None):
for tag in tags:
project.tags.add(tag)

log.info(
'Project imported.',
project_slug=project.slug,
user_username=request.user.username,
)

# TODO: this signal could be removed, or used for sync task
project_import.send(sender=project, request=request)

Expand Down
22 changes: 13 additions & 9 deletions readthedocs/projects/views/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
)
from readthedocs.projects.notifications import EmailConfirmNotification
from readthedocs.projects.utils import get_csv_file
from readthedocs.projects.views.base import ProjectAdminMixin, ProjectSpamMixin
from readthedocs.projects.views.base import ProjectAdminMixin
from readthedocs.projects.views.mixins import (
ProjectImportMixin,
ProjectRelationListMixin,
Expand Down Expand Up @@ -143,7 +143,7 @@ def get_queryset(self):
return self.model.objects.for_admin_user(self.request.user)


class ProjectUpdate(ProjectSpamMixin, ProjectMixin, UpdateView):
class ProjectUpdate(ProjectMixin, UpdateView):

form_class = UpdateProjectForm
success_message = _('Project settings updated')
Expand All @@ -153,7 +153,7 @@ def get_success_url(self):
return reverse('projects_detail', args=[self.object.slug])


class ProjectAdvancedUpdate(ProjectSpamMixin, ProjectMixin, UpdateView):
class ProjectAdvancedUpdate(ProjectMixin, UpdateView):

form_class = ProjectAdvancedForm
success_message = _('Project settings updated')
Expand Down Expand Up @@ -253,10 +253,7 @@ def post(self, request, *args, **kwargs):
return HttpResponseRedirect(self.get_success_url())


class ImportWizardView(
ProjectImportMixin, ProjectSpamMixin, PrivateViewMixin,
SessionWizardView,
):
class ImportWizardView(ProjectImportMixin, PrivateViewMixin, SessionWizardView):

"""
Project import wizard.
Expand Down Expand Up @@ -287,10 +284,17 @@ def _set_initial_dict(self):
else:
self.initial_dict = self.storage.data.get(self.initial_dict_key, {})

def post(self, *args, **kwargs):
def post(self, request, *args, **kwargs):
self._set_initial_dict()

log.bind(user_username=request.user.username)

if request.user.profile.banned:
log.info('Rejecting project POST from shadowbanned user.')
return HttpResponseRedirect(reverse('homepage'))

# The storage is reset after everything is done.
return super().post(*args, **kwargs)
return super().post(request, *args, **kwargs)

def get_form_kwargs(self, step=None):
"""Get args to pass into form instantiation."""
Expand Down
11 changes: 8 additions & 3 deletions readthedocs/projects/views/public.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
from readthedocs.storage import build_media_storage

from ..constants import PRIVATE
from .base import ProjectOnboardMixin
from .base import ProjectOnboardMixin, ProjectSpamMixin

log = structlog.get_logger(__name__)
search_log = structlog.get_logger(__name__ + '.search')
Expand Down Expand Up @@ -87,8 +87,13 @@ def project_redirect(request, invalid_project_slug):
))


class ProjectDetailViewBase(ProjectRelationListMixin, BuildTriggerMixin,
ProjectOnboardMixin, DetailView):
class ProjectDetailViewBase(
ProjectSpamMixin,
ProjectRelationListMixin,
BuildTriggerMixin,
ProjectOnboardMixin,
DetailView,
):

"""Display project onboard steps."""

Expand Down
16 changes: 0 additions & 16 deletions readthedocs/rtd_tests/tests/test_project_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from django.test.utils import override_settings
from django.utils.translation import ugettext_lazy as _
from django_dynamic_fixture import get
from textclassifier.validators import ClassifierValidator

from readthedocs.builds.constants import EXTERNAL, LATEST, STABLE
from readthedocs.builds.models import Version
Expand All @@ -16,7 +15,6 @@
REPO_TYPE_HG,
SPHINX,
)
from readthedocs.projects.exceptions import ProjectSpamError
from readthedocs.projects.forms import (
EmailHookForm,
EnvironmentVariableForm,
Expand All @@ -36,20 +34,6 @@

class TestProjectForms(TestCase):

@mock.patch.object(ClassifierValidator, '__call__')
def test_form_spam(self, mocked_validator):
"""Form description field fails spam validation."""
mocked_validator.side_effect = ProjectSpamError

data = {
'description': 'foo',
'documentation_type': 'sphinx',
'language': 'en',
}
form = ProjectExtraForm(data)
with self.assertRaises(ProjectSpamError):
form.is_valid()

def test_import_repo_url(self):
"""Validate different type of repository URLs on importing a Project."""

Expand Down
66 changes: 3 additions & 63 deletions readthedocs/rtd_tests/tests/test_project_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
from readthedocs.oauth.models import RemoteRepository, RemoteRepositoryRelation
from readthedocs.organizations.models import Organization
from readthedocs.projects.constants import PUBLIC
from readthedocs.projects.exceptions import ProjectSpamError
from readthedocs.projects.models import Domain, Project, WebHook, WebHookEvent
from readthedocs.projects.views.mixins import ProjectRelationMixin
from readthedocs.projects.views.private import ImportWizardView
Expand All @@ -25,7 +24,7 @@


@mock.patch('readthedocs.projects.tasks.update_docs_task', mock.MagicMock())
class TestProfileMiddleware(RequestFactoryTestMixin, TestCase):
class TestImportProjectBannedUser(RequestFactoryTestMixin, TestCase):

wizard_class_slug = 'import_wizard_view'
url = '/dashboard/import/manual/'
Expand All @@ -50,26 +49,15 @@ def setUp(self):
for (k, v) in list(data[key].items())})
self.data['{}-current_step'.format(self.wizard_class_slug)] = 'extra'

def test_profile_middleware_no_profile(self):
def test_not_banned_user(self):
"""User without profile and isn't banned."""
req = self.request(method='post', path='/projects/import', data=self.data)
req.user = get(User, profile=None)
resp = ImportWizardView.as_view()(req)
self.assertEqual(resp.status_code, 302)
self.assertEqual(resp['location'], '/projects/foobar/')

@mock.patch('readthedocs.projects.views.private.ProjectBasicsForm.clean')
def test_profile_middleware_spam(self, form):
"""User will be banned."""
form.side_effect = ProjectSpamError
req = self.request(method='post', path='/projects/import', data=self.data)
req.user = get(User)
resp = ImportWizardView.as_view()(req)
self.assertEqual(resp.status_code, 302)
self.assertEqual(resp['location'], '/')
self.assertTrue(req.user.profile.banned)

def test_profile_middleware_banned(self):
def test_banned_user(self):
"""User is banned."""
req = self.request(method='post', path='/projects/import', data=self.data)
req.user = get(User)
Expand Down Expand Up @@ -296,54 +284,6 @@ def test_remote_repository_is_added(self):
self.assertIsNotNone(proj)
self.assertEqual(proj.remote_repository, remote_repo)

@mock.patch(
'readthedocs.projects.views.private.ProjectExtraForm.clean_description',
create=True,
)
def test_form_spam(self, mocked_validator):
"""Don't add project on a spammy description."""
self.user.date_joined = timezone.now() - timedelta(days=365)
self.user.save()
mocked_validator.side_effect = ProjectSpamError

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')

resp = self.post_step('basics')
self.assertWizardResponse(resp, 'extra')
resp = self.post_step('extra', session=list(resp._request.session.items()))
self.assertIsInstance(resp, HttpResponseRedirect)
self.assertEqual(resp.status_code, 302)
self.assertEqual(resp['location'], '/')

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')
self.assertFalse(self.user.profile.banned)

@mock.patch(
'readthedocs.projects.views.private.ProjectExtraForm.clean_description',
create=True,
)
def test_form_spam_ban_user(self, mocked_validator):
"""Don't add spam and ban new user."""
self.user.date_joined = timezone.now()
self.user.save()
mocked_validator.side_effect = ProjectSpamError

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')

resp = self.post_step('basics')
self.assertWizardResponse(resp, 'extra')
resp = self.post_step('extra', session=list(resp._request.session.items()))
self.assertIsInstance(resp, HttpResponseRedirect)
self.assertEqual(resp.status_code, 302)
self.assertEqual(resp['location'], '/')

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')
self.assertTrue(self.user.profile.banned)


@mock.patch('readthedocs.core.utils.trigger_build', mock.MagicMock())
class TestPublicViews(TestCase):
Expand Down
1 change: 0 additions & 1 deletion readthedocs/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ def INSTALLED_APPS(self): # noqa
'rest_framework',
'rest_framework.authtoken',
'corsheaders',
'textclassifier',
'annoying',
'django_extensions',
'crispy_forms',
Expand Down
6 changes: 0 additions & 6 deletions requirements/pip.txt
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,6 @@ django-crispy-forms==1.13.0

docker==5.0.3

django-textclassifier==1.0
# django-textclassifier doesn't have pinned versions
# if there is an update they could break our code
nltk==3.6.6
textblob==0.17.1
Copy link
Member

Choose a reason for hiding this comment

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

These are big dependencies, so 👍 on getting rid of them if we aren't using them.


django-annoying==0.10.6
django-messages-extends==0.6.2
djangorestframework-jsonp==1.0.2
Expand Down