From 54d1c1528003cc104f059543fa37e0bd1301d9b1 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 7 Nov 2023 11:19:26 +0000 Subject: [PATCH] flag abuse reports reviewers will handle; send to seperate queue (#21385) * flag abuse reports reviewers will handle; send to seperate queue * add needs human review reasons to choices * flag the correct version for needshumanreview if specified * choices fix * fix webhook view * rename CinderAddonByReviewers --- src/olympia/abuse/cinder.py | 40 +++++++++-- src/olympia/abuse/models.py | 33 +++++++++- src/olympia/abuse/tests/test_cinder.py | 66 +++++++++++++++++-- src/olympia/abuse/tests/test_models.py | 64 +++++++++++++++++- src/olympia/abuse/utils.py | 8 ++- src/olympia/abuse/views.py | 2 +- src/olympia/addons/models.py | 30 +++++++-- src/olympia/addons/tests/test_models.py | 34 ++++++++++ .../0032_alter_needshumanreview_reason.py | 18 +++++ src/olympia/reviewers/models.py | 10 +++ 10 files changed, 282 insertions(+), 23 deletions(-) create mode 100644 src/olympia/reviewers/migrations/0032_alter_needshumanreview_reason.py diff --git a/src/olympia/abuse/cinder.py b/src/olympia/abuse/cinder.py index f9a693453a..40a87fefcf 100644 --- a/src/olympia/abuse/cinder.py +++ b/src/olympia/abuse/cinder.py @@ -4,7 +4,8 @@ import requests class CinderEntity: - QUEUE = 'amo-content-infringement' + # This queue is for reports that T&S / TaskUs look at + queue = 'amo-content-infringement' type = None # Needs to be defined by subclasses @property @@ -38,7 +39,7 @@ class CinderEntity: reporter.get_relationship_data(self, 'amo_reporter_of') ] return { - 'queue_slug': self.QUEUE, + 'queue_slug': self.queue, 'entity_type': self.type, 'entity': self.get_attributes(), 'reasoning': report_text, @@ -77,7 +78,7 @@ class CinderEntity: 'authorization': f'Bearer {settings.CINDER_API_TOKEN}', } data = { - 'queue_slug': self.QUEUE, + 'queue_slug': self.queue, 'appealer_entity_type': appealer.type, 'appealer_entity': appealer.get_attributes(), 'reasoning': appeal_text, @@ -150,8 +151,9 @@ class CinderUnauthenticatedReporter(CinderEntity): class CinderAddon(CinderEntity): type = 'amo_addon' - def __init__(self, addon): + def __init__(self, addon, version=None): self.addon = addon + self.version = version @property def id(self): @@ -202,3 +204,33 @@ class CinderRating(CinderEntity): cinder_user.get_relationship_data(self, 'amo_rating_author_of') ], } + + +class CinderAddonHandledByReviewers(CinderAddon): + # This queue is not monitored on cinder - reports are resolved via AMO instead + queue = 'amo-addon-infringement' + + def flag_for_human_review(self, appeal=False): + from olympia.reviewers.models import NeedsHumanReview + + reason = ( + NeedsHumanReview.REASON_ABUSE_ADDON_VIOLATION_APPEAL + if appeal + else NeedsHumanReview.REASON_ABUSE_ADDON_VIOLATION + ) + if self.version: + NeedsHumanReview.objects.get_or_create( + version=self.version, reason=reason, is_active=True + ) + else: + self.addon.set_needs_human_review_on_latest_versions( + reason=reason, ignore_reviewed=False, unique_reason=True + ) + + def report(self, *args, **kwargs): + self.flag_for_human_review(appeal=False) + return super().report(*args, **kwargs) + + def appeal(self, *args, **kwargs): + self.flag_for_human_review(appeal=True) + return super().appeal(*args, **kwargs) diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index 7a0fc9cd07..b8fe6da564 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -11,7 +11,13 @@ from olympia.api.utils import APIChoices, APIChoicesWithNone from olympia.ratings.models import Rating from olympia.users.models import UserProfile -from .cinder import CinderAddon, CinderRating, CinderUnauthenticatedReporter, CinderUser +from .cinder import ( + CinderAddon, + CinderAddonHandledByReviewers, + CinderRating, + CinderUnauthenticatedReporter, + CinderUser, +) from .utils import ( CinderActionApprove, CinderActionBanUser, @@ -123,6 +129,8 @@ class AbuseReport(ModelBase): 'REPORTABLE_REASONS', ('HATEFUL_VIOLENT_DECEPTIVE', 'ILLEGAL', 'POLICY_VIOLATION', 'OTHER'), ) + # Abuse in these locations are handled by reviewers + REASONS.add_subset('REVIEWER_HANDLED', ('POLICY_VIOLATION',)) # https://searchfox.org # /mozilla-central/source/toolkit/components/telemetry/Events.yaml#122-131 @@ -201,6 +209,8 @@ class AbuseReport(ModelBase): ('ADDON', 2, 'Inside Add-on'), ('BOTH', 3, 'Both on AMO and inside Add-on'), ) + # Abuse in these locations are handled by reviewers + LOCATION.add_subset('REVIEWER_HANDLED', ('ADDON', 'BOTH')) # NULL if the reporter was not authenticated. reporter = models.ForeignKey( @@ -367,6 +377,15 @@ class AbuseReport(ModelBase): return self.rating return None + @property + def is_handled_by_reviewers(self): + return ( + (target := self.target) + and isinstance(target, Addon) + and self.reason in AbuseReport.REASONS.REVIEWER_HANDLED + and self.location in AbuseReport.LOCATION.REVIEWER_HANDLED + ) + class CantBeAppealed(Exception): pass @@ -409,7 +428,17 @@ class CinderReport(ModelBase): target = self.abuse_report.target if target: if isinstance(target, Addon): - return CinderAddon(target) + version = ( + self.abuse_report.addon_version + and target.versions(manager='unfiltered_for_relations') + .filter(version=self.abuse_report.addon_version) + .no_transforms() + .first() + ) + if self.abuse_report.is_handled_by_reviewers: + return CinderAddonHandledByReviewers(target, version) + else: + return CinderAddon(target, version) elif isinstance(target, UserProfile): return CinderUser(target) elif isinstance(target, Rating): diff --git a/src/olympia/abuse/tests/test_cinder.py b/src/olympia/abuse/tests/test_cinder.py index a9b6af307e..a4ac929685 100644 --- a/src/olympia/abuse/tests/test_cinder.py +++ b/src/olympia/abuse/tests/test_cinder.py @@ -2,11 +2,14 @@ from django.conf import settings import responses -from olympia.amo.tests import TestCase, addon_factory, user_factory +from olympia import amo +from olympia.amo.tests import TestCase, addon_factory, user_factory, version_factory from olympia.ratings.models import Rating +from olympia.reviewers.models import NeedsHumanReview from ..cinder import ( CinderAddon, + CinderAddonHandledByReviewers, CinderRating, CinderUnauthenticatedReporter, CinderUser, @@ -71,9 +74,11 @@ class BaseTestCinderCase: def test_report(self): self._test_report(self.cinder_class(self._create_dummy_target())) - def _test_appeal(self, appealer): + def _test_appeal(self, appealer, cinder_instance=None): fake_decision_id = 'decision-id-to-appeal-666' - cinder_instance = self.cinder_class(self._create_dummy_target()) + cinder_instance = cinder_instance or self.cinder_class( + self._create_dummy_target() + ) responses.add( responses.POST, @@ -120,7 +125,7 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): report_text=reason, category=None, reporter=None ) assert data == { - 'queue_slug': 'amo-content-infringement', + 'queue_slug': self.cinder_class.queue, 'entity_type': 'amo_addon', 'entity': { 'id': str(addon.id), @@ -267,6 +272,57 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): } +class TestCinderAddonHandledByReviewers(TestCinderAddon): + cinder_class = CinderAddonHandledByReviewers + + def setUp(self): + user_factory(id=settings.TASK_USER_ID) + + def test_report(self): + addon = self._create_dummy_target() + addon.current_version.file.update(is_signed=True) + self._test_report(self.cinder_class(addon)) + assert ( + addon.current_version.needshumanreview_set.get().reason + == NeedsHumanReview.REASON_ABUSE_ADDON_VIOLATION + ) + + def test_report_with_version(self): + addon = self._create_dummy_target() + addon.current_version.file.update(is_signed=True) + other_version = version_factory( + addon=addon, + file_kw={'is_signed': True, 'status': amo.STATUS_AWAITING_REVIEW}, + ) + self._test_report(self.cinder_class(addon, other_version)) + assert not addon.current_version.needshumanreview_set.exists() + # that there's only one is required - _test_report calls report() multiple times + assert ( + other_version.needshumanreview_set.get().reason + == NeedsHumanReview.REASON_ABUSE_ADDON_VIOLATION + ) + + def test_appeal_anonymous(self): + addon = self._create_dummy_target() + addon.current_version.file.update(is_signed=True) + self._test_appeal( + CinderUnauthenticatedReporter('itsme', 'm@r.io'), self.cinder_class(addon) + ) + assert ( + addon.current_version.needshumanreview_set.get().reason + == NeedsHumanReview.REASON_ABUSE_ADDON_VIOLATION_APPEAL + ) + + def test_appeal_logged_in(self): + addon = self._create_dummy_target() + addon.current_version.file.update(is_signed=True) + self._test_appeal(CinderUser(user_factory()), self.cinder_class(addon)) + assert ( + addon.current_version.needshumanreview_set.get().reason + == NeedsHumanReview.REASON_ABUSE_ADDON_VIOLATION_APPEAL + ) + + class TestCinderUser(BaseTestCinderCase, TestCase): cinder_class = CinderUser @@ -282,7 +338,7 @@ class TestCinderUser(BaseTestCinderCase, TestCase): report_text=reason, category=None, reporter=None ) assert data == { - 'queue_slug': 'amo-content-infringement', + 'queue_slug': self.cinder_class.queue, 'entity_type': 'amo_user', 'entity': { 'id': str(user.id), diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index 318e4c7932..e553c28bd7 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -9,7 +9,12 @@ import responses from olympia.amo.tests import TestCase, addon_factory, user_factory from olympia.ratings.models import Rating -from ..cinder import CinderAddon, CinderRating, CinderUser +from ..cinder import ( + CinderAddon, + CinderAddonHandledByReviewers, + CinderRating, + CinderUser, +) from ..models import AbuseReport, CinderReport from ..utils import ( CinderActionApprove, @@ -254,6 +259,30 @@ class TestAbuse(TestCase): report.update(user=None, rating=rating) assert report.target == rating + def test_is_handled_by_reviewers(self): + addon = addon_factory() + abuse_report = AbuseReport.objects.create( + guid=addon.guid, + reason=AbuseReport.REASONS.ILLEGAL, + location=AbuseReport.LOCATION.BOTH, + ) + # location is in REVIEWER_HANDLED (BOTH) but reason is not (ILLEGAL) + assert not abuse_report.is_handled_by_reviewers + + abuse_report.update(reason=AbuseReport.REASONS.POLICY_VIOLATION) + # now reason is in REVIEWER_HANDLED it will be reported differently + assert abuse_report.is_handled_by_reviewers + + abuse_report.update(location=AbuseReport.LOCATION.AMO) + # but not if the location is not in REVIEWER_HANDLED (i.e. AMO) + assert not abuse_report.is_handled_by_reviewers + + # test non-addons are False regardless + abuse_report.update(location=AbuseReport.LOCATION.ADDON) + assert abuse_report.is_handled_by_reviewers + abuse_report.update(user=user_factory(), guid=None) + assert not abuse_report.is_handled_by_reviewers + def test_constraint(self): report = AbuseReport() constraints = report.get_constraints() @@ -323,14 +352,43 @@ class TestCinderReport(TestCase): user = user_factory() cinder_report = CinderReport.objects.create( abuse_report=AbuseReport.objects.create( - guid=addon.guid, reason=AbuseReport.REASONS.ILLEGAL + guid=addon.guid, + reason=AbuseReport.REASONS.ILLEGAL, + location=AbuseReport.LOCATION.BOTH, ) ) helper = cinder_report.get_entity_helper() + # location is in REVIEWER_HANDLED (BOTH) but reason is not (ILLEGAL) assert isinstance(helper, CinderAddon) + assert not isinstance(helper, CinderAddonHandledByReviewers) assert helper.addon == addon + assert helper.version is None - cinder_report.abuse_report.update(guid=None, user=user) + cinder_report.abuse_report.update(reason=AbuseReport.REASONS.POLICY_VIOLATION) + helper = cinder_report.get_entity_helper() + # now reason is in REVIEWER_HANDLED it will be reported differently + assert isinstance(helper, CinderAddon) + assert isinstance(helper, CinderAddonHandledByReviewers) + assert helper.addon == addon + assert helper.version is None + + cinder_report.abuse_report.update(addon_version=addon.current_version.version) + helper = cinder_report.get_entity_helper() + # if we got a version too we pass it on to the helper + assert isinstance(helper, CinderAddon) + assert isinstance(helper, CinderAddonHandledByReviewers) + assert helper.addon == addon + assert helper.version == addon.current_version + + cinder_report.abuse_report.update(location=AbuseReport.LOCATION.AMO) + helper = cinder_report.get_entity_helper() + # but not if the location is not in REVIEWER_HANDLED (i.e. AMO) + assert isinstance(helper, CinderAddon) + assert not isinstance(helper, CinderAddonHandledByReviewers) + assert helper.addon == addon + assert helper.version == addon.current_version + + cinder_report.abuse_report.update(guid=None, user=user, addon_version=None) helper = cinder_report.get_entity_helper() assert isinstance(helper, CinderUser) assert helper.user == user diff --git a/src/olympia/abuse/utils.py b/src/olympia/abuse/utils.py index de266b64a2..67358acd42 100644 --- a/src/olympia/abuse/utils.py +++ b/src/olympia/abuse/utils.py @@ -61,9 +61,13 @@ class CinderActionEscalateAddon(CinderAction): .first() ) if version_obj: - NeedsHumanReview.objects.create(version=version_obj, reason=reason) + NeedsHumanReview.objects.create( + version=version_obj, reason=reason, is_active=True + ) else: - addon.set_needs_human_review_on_latest_versions(reason=reason) + addon.set_needs_human_review_on_latest_versions( + reason=reason, ignore_reviewed=False, unique_reason=True + ) class CinderActionApprove(CinderAction): diff --git a/src/olympia/abuse/views.py b/src/olympia/abuse/views.py index c91e3bc3b2..87ee19700d 100644 --- a/src/olympia/abuse/views.py +++ b/src/olympia/abuse/views.py @@ -172,7 +172,7 @@ def cinder_webhook(request): ): source = payload.get('source', {}) job = source.get('job', {}) - if (queue_name := job.get('queue', {}).get('slug')) == CinderEntity.QUEUE: + if (queue_name := job.get('queue', {}).get('slug')) == CinderEntity.queue: log.info('Valid Payload from AMO queue: %s', payload) job_id = job.get('id', '') decision_id = source.get('decision', {}).get('id') diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py index 7bc2b62551..e4602cafba 100644 --- a/src/olympia/addons/models.py +++ b/src/olympia/addons/models.py @@ -684,17 +684,33 @@ class Addon(OnChangeMixin, ModelBase): def disable_all_files(self): File.objects.filter(version__addon=self).update(status=amo.STATUS_DISABLED) - def set_needs_human_review_on_latest_versions(self, *, reason, due_date=None): + def set_needs_human_review_on_latest_versions( + self, *, reason, due_date=None, ignore_reviewed=True, unique_reason=False + ): set_listed = self._set_needs_human_review_on_latest_signed_version( - channel=amo.CHANNEL_LISTED, due_date=due_date, reason=reason + channel=amo.CHANNEL_LISTED, + due_date=due_date, + reason=reason, + ignore_reviewed=ignore_reviewed, + unique_reason=unique_reason, ) set_unlisted = self._set_needs_human_review_on_latest_signed_version( - channel=amo.CHANNEL_UNLISTED, due_date=due_date, reason=reason + channel=amo.CHANNEL_UNLISTED, + due_date=due_date, + reason=reason, + ignore_reviewed=ignore_reviewed, + unique_reason=unique_reason, ) return set_listed or set_unlisted def _set_needs_human_review_on_latest_signed_version( - self, *, channel, reason, due_date=None + self, + *, + channel, + reason, + due_date=None, + ignore_reviewed=True, + unique_reason=False, ): from olympia.reviewers.models import NeedsHumanReview @@ -706,8 +722,10 @@ class Addon(OnChangeMixin, ModelBase): ) if ( not version - or version.human_review_date - or version.needshumanreview_set.filter(is_active=True).exists() + or (ignore_reviewed and version.human_review_date) + or version.needshumanreview_set.filter( + is_active=True, **({'reason': reason} if unique_reason else {}) + ).exists() ): return False had_due_date_already = bool(version.due_date) diff --git a/src/olympia/addons/tests/test_models.py b/src/olympia/addons/tests/test_models.py index ba8f01b68e..d5f8f5e508 100644 --- a/src/olympia/addons/tests/test_models.py +++ b/src/olympia/addons/tests/test_models.py @@ -2063,6 +2063,40 @@ class TestAddonDueDate(TestCase): ) assert version.needshumanreview_set.filter(is_active=True).count() == 0 + assert addon.set_needs_human_review_on_latest_versions( + reason=NeedsHumanReview.REASON_PROMOTED_GROUP, ignore_reviewed=False + ) + assert version.needshumanreview_set.filter(is_active=True).count() == 1 + assert ( + version.needshumanreview_set.get().reason + == NeedsHumanReview.REASON_PROMOTED_GROUP + ) + + def test_set_needs_human_review_on_latest_versions_unique_reason(self): + addon = Addon.objects.get(id=3615) + version = addon.current_version + NeedsHumanReview.objects.create( + version=version, reason=NeedsHumanReview.REASON_SCANNER_ACTION + ) + + assert not addon.set_needs_human_review_on_latest_versions( + reason=NeedsHumanReview.REASON_PROMOTED_GROUP, unique_reason=False + ) + assert version.needshumanreview_set.filter(is_active=True).count() == 1 + assert ( + version.needshumanreview_set.get().reason + == NeedsHumanReview.REASON_SCANNER_ACTION + ) + + assert addon.set_needs_human_review_on_latest_versions( + reason=NeedsHumanReview.REASON_PROMOTED_GROUP, unique_reason=True + ) + assert version.needshumanreview_set.filter(is_active=True).count() == 2 + assert list(version.needshumanreview_set.values_list('reason', flat=True)) == [ + NeedsHumanReview.REASON_SCANNER_ACTION, + NeedsHumanReview.REASON_PROMOTED_GROUP, + ] + def test_set_needs_human_review_on_latest_versions_even_deleted(self): addon = Addon.objects.get(id=3615) version = addon.current_version diff --git a/src/olympia/reviewers/migrations/0032_alter_needshumanreview_reason.py b/src/olympia/reviewers/migrations/0032_alter_needshumanreview_reason.py new file mode 100644 index 0000000000..7657751496 --- /dev/null +++ b/src/olympia/reviewers/migrations/0032_alter_needshumanreview_reason.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.7 on 2023-11-06 16:04 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('reviewers', '0031_reviewactionreason_canned_block_reason'), + ] + + operations = [ + migrations.AlterField( + model_name='needshumanreview', + name='reason', + field=models.SmallIntegerField(choices=[(0, 'Unknown'), (1, 'Hit scanner rule'), (2, 'Belongs to a promoted group'), (3, 'Over growth threshold for usage tier'), (4, 'Previous version in channel had needs human review set'), (5, 'Sources provided while pending rejection'), (6, 'Developer replied'), (7, 'Manually set as needing human review by a reviewer'), (8, 'Auto-approved but still had an approval delay set in the past'), (9, 'Over abuse reports threshold for usage tier'), (10, 'Escalated for an abuse report, via cinder'), (11, 'Reported for abuse within the add-on'), (12, 'Appeal about a decision on abuse reported within the add-on')], default=0, editable=False), + ), + ] diff --git a/src/olympia/reviewers/models.py b/src/olympia/reviewers/models.py index f1ca3e1820..d2bd5914ec 100644 --- a/src/olympia/reviewers/models.py +++ b/src/olympia/reviewers/models.py @@ -756,6 +756,8 @@ class NeedsHumanReview(ModelBase): REASON_AUTO_APPROVED_PAST_APPROVAL_DELAY = 8 REASON_ABUSE_REPORTS_THRESHOLD = 9 REASON_CINDER_ESCALATION = 10 + REASON_ABUSE_ADDON_VIOLATION = 11 + REASON_ABUSE_ADDON_VIOLATION_APPEAL = 12 reason = models.SmallIntegerField( default=0, @@ -792,6 +794,14 @@ class NeedsHumanReview(ModelBase): REASON_CINDER_ESCALATION, 'Escalated for an abuse report, via cinder', ), + ( + REASON_ABUSE_ADDON_VIOLATION, + 'Reported for abuse within the add-on', + ), + ( + REASON_ABUSE_ADDON_VIOLATION_APPEAL, + 'Appeal about a decision on abuse reported within the add-on', + ), ), editable=False, )