From cacd8e302cf2896fad7bc556d33e8ab01d370403 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 19 Nov 2024 12:56:04 +0000 Subject: [PATCH] allow unsigned versions to be flagged for NHR (#22857) --- src/olympia/abuse/cinder.py | 74 ++++++++++----------- src/olympia/abuse/models.py | 8 ++- src/olympia/abuse/tests/test_cinder.py | 78 ++++++++++++----------- src/olympia/abuse/tests/test_models.py | 50 +++++++++++++++ src/olympia/abuse/tests/test_tasks.py | 1 + src/olympia/versions/models.py | 10 +-- src/olympia/versions/tests/test_models.py | 17 ++++- 7 files changed, 153 insertions(+), 85 deletions(-) diff --git a/src/olympia/abuse/cinder.py b/src/olympia/abuse/cinder.py index 24ab1062a3..5660bd90c6 100644 --- a/src/olympia/abuse/cinder.py +++ b/src/olympia/abuse/cinder.py @@ -494,9 +494,7 @@ class CinderAddonHandledByReviewers(CinderAddon): # No special appeal queue for reviewer handled jobs return self.queue - def flag_for_human_review( - self, *, reported_versions, appeal=False, forwarded=False - ): + def flag_for_human_review(self, *, related_versions, appeal=False, forwarded=False): from olympia.reviewers.models import NeedsHumanReview waffle_switch_name = ( @@ -525,62 +523,64 @@ class CinderAddonHandledByReviewers(CinderAddon): version_objs = ( set( self.addon.versions(manager='unfiltered_for_relations') - .filter(version__in=reported_versions) - .exclude( - needshumanreview__reason=reason, - needshumanreview__is_active=True, - ) + .filter(version__in=related_versions) .no_transforms() ) - if reported_versions + if related_versions else set() ) - nhr_object = None - # We need custom save() and post_save to be triggered, so we can't - # optimize this via bulk_create(). - for version in version_objs: - nhr_object = NeedsHumanReview( - version=version, reason=reason, is_active=True - ) - nhr_object.save(_no_automatic_activity_log=True) # If we have more versions specified than versions we flagged, flag latest # to be safe. (Either because there was an unknown version, or a None) - if len(version_objs) != len(reported_versions) or len(reported_versions) == 0: - version_objs = version_objs.union( - self.addon.set_needs_human_review_on_latest_versions( - reason=reason, - ignore_reviewed=False, - unique_reason=True, - skip_activity_log=True, - ) + if len(version_objs) != len(related_versions) or len(related_versions) == 0: + version_objs.add( + self.addon.versions(manager='unfiltered_for_relations') + .filter(channel=amo.CHANNEL_LISTED) + .no_transforms() + .first() ) - if version_objs: - version_objs = sorted(version_objs, key=lambda v: v.id) - # we just need this for get_reason_display - nhr_object = nhr_object or NeedsHumanReview( - version=version_objs[-1], - reason=reason, - is_active=True, + version_objs = sorted(version_objs, key=lambda v: v.id) + log.debug( + 'Found %s versions potentially needing NHR [%s]', + len(version_objs), + ','.join(v.version for v in version_objs), + ) + existing_nhrs = { + nhr.version + for nhr in NeedsHumanReview.objects.filter( + version__in=version_objs, is_active=True, reason=reason ) + } + # We need custom save() and post_save to be triggered, so we can't + # optimize this via bulk_create(). + nhr = None + for version in version_objs: + if version in existing_nhrs: + # if there's already an active NHR for this reason, don't duplicate it + continue + nhr = NeedsHumanReview(version=version, reason=reason, is_active=True) + nhr.save(_no_automatic_activity_log=True) + + if nhr: activity.log_create( amo.LOG.NEEDS_HUMAN_REVIEW_CINDER, *version_objs, - details={'comments': nhr_object.get_reason_display()}, + details={'comments': nhr.get_reason_display()}, user=core.get_user() or get_task_user(), ) def post_report(self, job): if not job.is_appeal: - reported_version = self.version_string self.flag_for_human_review( - reported_versions={reported_version}, appeal=False + related_versions={self.version_string}, appeal=False ) # If our report was added to an appeal job (i.e. an appeal was ongoing, # and a report was made against the add-on), don't flag the add-on for # human review again: we should already have one because of the appeal. def appeal(self, *args, **kwargs): - self.flag_for_human_review(reported_versions=set(), appeal=True) + related_versions = {self.version_string} if self.version_string else set() + # TODO: use the version(s) we took action on, rather than the versions reported + self.flag_for_human_review(related_versions=related_versions, appeal=True) return super().appeal(*args, **kwargs) def workflow_recreate(self, *, job): @@ -592,7 +592,7 @@ class CinderAddonHandledByReviewers(CinderAddon): reported_versions = set( job.abusereport_set.values_list('addon_version', flat=True) ) - self.flag_for_human_review(reported_versions=reported_versions, forwarded=True) + self.flag_for_human_review(related_versions=reported_versions, forwarded=True) class CinderReport(CinderEntity): diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index 4134362953..13a09d7ee8 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -1172,7 +1172,9 @@ class ContentDecision(ModelBase): raise CantBeAppealed entity_helper = CinderJob.get_entity_helper( - self.target, resolved_in_reviewer_tools=resolvable_in_reviewer_tools + self.target, + resolved_in_reviewer_tools=resolvable_in_reviewer_tools, + addon_version_string=getattr(abuse_report, 'addon_version', None), ) appeal_id = entity_helper.appeal( decision_cinder_id=self.cinder_id, @@ -1201,8 +1203,8 @@ class ContentDecision(ModelBase): entity_helper, appealed_action=None, ): - """Calling this method calls cinder to create a decision, or notfies the content - owner/reporter by email, or both. + """Calling this method calls Cinder to create a decision, or notifies the + content owner/reporter by email, or both. If a decision is created in cinder the instance will be saved, along with relevant policies; if a cinder decision isn't need the instance won't be saved. diff --git a/src/olympia/abuse/tests/test_cinder.py b/src/olympia/abuse/tests/test_cinder.py index 96c4afcaf3..ceeee7f4d7 100644 --- a/src/olympia/abuse/tests/test_cinder.py +++ b/src/olympia/abuse/tests/test_cinder.py @@ -1125,7 +1125,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): # Make sure this is testing the case where no user is set (we fall back # to the task user). assert core.get_user() is None - addon.current_version.file.update(is_signed=True) # Trigger switch_is_active to ensure it's cached to make db query # count more predictable. waffle.switch_is_active('dsa-abuse-reports-review') @@ -1134,25 +1133,24 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): assert addon.current_version.needshumanreview_set.count() == 0 job = CinderJob.objects.create(job_id='1234-xyz') cinder_instance = self.CinderClass(addon) - with self.assertNumQueries(13): + with self.assertNumQueries(11): # - 1 Fetch Cinder Decision # - 2 Fetch Version - # - 3 Fetch Translations for that Version - # - 4 Fetch NeedsHumanReview - # - 5 Create NeedsHumanReview - # - 6 Fetch NeedsHumanReview - # - 7 Update due date on Versions - # - 8 Fetch Latest signed Version - # - 9 Fetch task user - # - 10 Create ActivityLog - # - 11 Create ActivityLogComment - # - 12 Update ActivityLogComment - # - 13 Create VersionLog + # - 3 Fetch NeedsHumanReview + # - 4 Create NeedsHumanReview + # - 5 Query if due_date is needed for version + # - 6 Update due date on Version + # - 7 Fetch task user + # - 8 Create ActivityLog + # - 9 Update ActivityLog + # - 10 Create ActivityLogComment + # - 11 Create VersionLog cinder_instance.post_report(job) assert ( addon.current_version.needshumanreview_set.get().reason == NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION ) + assert addon.current_version.reload().due_date assert ActivityLog.objects.for_versions(addon.current_version).filter( action=amo.LOG.NEEDS_HUMAN_REVIEW_CINDER.id ) @@ -1160,7 +1158,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): @override_switch('dsa-abuse-reports-review', active=False) def test_report_waffle_switch_off(self): addon = self._create_dummy_target() - addon.current_version.file.update(is_signed=True) # Trigger switch_is_active to ensure it's cached to make db query # count more predictable. waffle.switch_is_active('dsa-abuse-reports-review') @@ -1173,10 +1170,8 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): 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}, + addon=addon, file_kw={'status': amo.STATUS_AWAITING_REVIEW} ) responses.add( responses.POST, @@ -1188,7 +1183,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): guid=addon.guid, addon_version=other_version.version ) report = CinderReport(abuse_report) - cinder_instance = self.CinderClass(addon, version_string=other_version) + cinder_instance = self.CinderClass(addon, version_string=other_version.version) assert cinder_instance.report(report=report, reporter=None) job = CinderJob.objects.create(job_id='1234-xyz') assert not addon.current_version.needshumanreview_set.exists() @@ -1201,10 +1196,10 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): other_version.needshumanreview_set.get().reason == NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION ) + assert other_version.reload().due_date 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.CinderClass(addon) ) @@ -1212,20 +1207,37 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): addon.current_version.needshumanreview_set.get().reason == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL ) + assert addon.current_version.reload().due_date 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.CinderClass(addon)) assert ( addon.current_version.needshumanreview_set.get().reason == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL ) + assert addon.current_version.reload().due_date + + def test_appeal_specific_version(self): + addon = self._create_dummy_target() + other_version = version_factory( + addon=addon, file_kw={'status': amo.STATUS_AWAITING_REVIEW} + ) + self._test_appeal( + CinderUser(user_factory()), + self.CinderClass(addon, version_string=other_version.version), + ) + assert not addon.current_version.needshumanreview_set.exists() + assert ( + other_version.needshumanreview_set.get().reason + == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL + ) + assert not addon.current_version.reload().due_date + assert other_version.reload().due_date @override_switch('dsa-appeals-review', active=False) def test_appeal_waffle_switch_off(self): addon = self._create_dummy_target() - addon.current_version.file.update(is_signed=True) # We are no longer doing the queries for the activitylog, needshumanreview # etc since the waffle switch is off. So we're back to the same number of # queries made by the reports that go to Cinder. @@ -1235,7 +1247,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): def test_report_with_ongoing_appeal(self): addon = self._create_dummy_target() - addon.current_version.file.update(is_signed=True) job = CinderJob.objects.create(job_id='1234-xyz') job.appealed_decisions.add( ContentDecision.objects.create( @@ -1255,7 +1266,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): def test_report_with_ongoing_forwarded_appeal(self): addon = self._create_dummy_target() - addon.current_version.file.update(is_signed=True) job = CinderJob.objects.create(job_id='1234-xyz') CinderJob.objects.create(forwarded_to_job=job) job.appealed_decisions.add( @@ -1380,10 +1390,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): def _setup_workflow_move_test(self): addon = self._create_dummy_target() listed_version = addon.current_version - listed_version.file.update(is_signed=True) - unlisted_version = version_factory( - addon=addon, channel=amo.CHANNEL_UNLISTED, file_kw={'is_signed': True} - ) + unlisted_version = version_factory(addon=addon, channel=amo.CHANNEL_UNLISTED) ActivityLog.objects.all().delete() cinder_instance = self.CinderClass(addon) cinder_job = CinderJob.objects.create(target_addon=addon, job_id='1') @@ -1407,15 +1414,14 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): listed_version.reload().needshumanreview_set.get().reason == NeedsHumanReview.REASONS.CINDER_ESCALATION ) - assert ( - unlisted_version.reload().needshumanreview_set.get().reason - == NeedsHumanReview.REASONS.CINDER_ESCALATION - ) + assert not unlisted_version.reload().needshumanreview_set.exists() + assert listed_version.reload().due_date + assert not unlisted_version.reload().due_date assert ActivityLog.objects.count() == 1 activity = ActivityLog.objects.filter( action=amo.LOG.NEEDS_HUMAN_REVIEW_CINDER.id ).get() - assert activity.arguments == [listed_version, unlisted_version] + assert activity.arguments == [listed_version] assert activity.user == self.task_user def test_workflow_move(self): @@ -1433,8 +1439,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): self._setup_workflow_move_test() ) other_version = version_factory( - addon=listed_version.addon, - file_kw={'status': amo.STATUS_DISABLED, 'is_signed': True}, + addon=listed_version.addon, file_kw={'status': amo.STATUS_DISABLED} ) assert not other_version.due_date cinder_job.abusereport_set.update(addon_version=other_version.version) @@ -1444,8 +1449,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): assert not listed_version.reload().needshumanreview_set.exists() assert not unlisted_version.reload().needshumanreview_set.exists() - other_version.reload() - assert other_version.due_date + assert other_version.reload().due_date assert ( other_version.needshumanreview_set.get().reason == NeedsHumanReview.REASONS.CINDER_ESCALATION @@ -1528,7 +1532,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon): other_version = version_factory( addon=listed_version.addon, - file_kw={'status': amo.STATUS_DISABLED, 'is_signed': True}, + file_kw={'status': amo.STATUS_DISABLED}, ) assert not other_version.due_date ActivityLog.objects.all().delete() diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index e09abcc72e..38565af772 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -2698,6 +2698,56 @@ class TestContentDecision(TestCase): abuse_report.reload() assert abuse_report.cinderappeal + def test_appeal_as_reporter_specific_version(self): + addon = addon_factory(version_kw={'human_review_date': datetime.now()}) + original_version = addon.current_version + version_factory(addon=addon, human_review_date=datetime.now()) + abuse_report = AbuseReport.objects.create( + guid=addon.guid, + reason=AbuseReport.REASONS.ILLEGAL, + reporter=user_factory(), + addon_version=original_version.version, + ) + abuse_report.update( + cinder_job=CinderJob.objects.create( + target_addon=addon, + resolvable_in_reviewer_tools=True, + decision=ContentDecision.objects.create( + cinder_id='4815162342-lost', + action_date=self.days_ago(179), + action=DECISION_ACTIONS.AMO_APPROVE, + addon=addon, + ), + ) + ) + responses.add( + responses.POST, + f'{settings.CINDER_SERVER_URL}appeal', + json={'external_id': '2432615184-tsol'}, + status=201, + ) + assert not original_version.due_date + + abuse_report.cinder_job.decision.appeal( + abuse_report=abuse_report, + appeal_text='appeal text', + user=abuse_report.reporter, + is_reporter=True, + ) + + abuse_report.cinder_job.reload() + assert abuse_report.cinder_job.decision.appeal_job + assert abuse_report.cinder_job.decision.appeal_job.job_id == '2432615184-tsol' + assert abuse_report.cinder_job.decision.appeal_job.target_addon == addon + abuse_report.reload() + assert abuse_report.cinderappeal + assert CinderAppeal.objects.count() == 1 + appeal_text_obj = CinderAppeal.objects.get() + assert appeal_text_obj.text == 'appeal text' + assert appeal_text_obj.decision == abuse_report.cinder_job.decision + assert appeal_text_obj.reporter_report == abuse_report + assert original_version.reload().due_date + def test_appeal_improperly_configured_reporter(self): cinder_job = CinderJob.objects.create( decision=ContentDecision.objects.create( diff --git a/src/olympia/abuse/tests/test_tasks.py b/src/olympia/abuse/tests/test_tasks.py index 61d3595c46..b62a00005b 100644 --- a/src/olympia/abuse/tests/test_tasks.py +++ b/src/olympia/abuse/tests/test_tasks.py @@ -591,6 +591,7 @@ def test_addon_appeal_to_cinder_authenticated_reporter(): @pytest.mark.django_db def test_addon_appeal_to_cinder_authenticated_author(): user = user_factory(fxa_id='fake-fxa-id') + user_factory(pk=settings.TASK_USER_ID) addon = addon_factory(users=[user]) decision = ContentDecision.objects.create( cinder_id='4815162342-abc', diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 20867853df..40d2ab9af7 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -239,7 +239,7 @@ class VersionManager(ManagerBase): # Versions that haven't been disabled or have ever been signed and have # the explicit needs human review flag should have a due date (it gets # dropped on various reviewer actions). - is_needs_human_review = Q( + is_other_needs_human_review = Q( ~Q(file__status=amo.STATUS_DISABLED) | Q(file__is_signed=True), needshumanreview__is_active=True, ) @@ -251,19 +251,19 @@ class VersionManager(ManagerBase): ) return { 'needs_human_review_from_cinder': Q( - is_needs_human_review, + needshumanreview__is_active=True, needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, ), 'needs_human_review_from_abuse': Q( - is_needs_human_review, + needshumanreview__is_active=True, needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, ), 'needs_human_review_from_appeal': Q( - is_needs_human_review, + needshumanreview__is_active=True, needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, ), 'needs_human_review_other': Q( - is_needs_human_review, + is_other_needs_human_review, ~Q( needshumanreview__reason__in=( NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION.value, diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index edefc3167a..99284ce7e2 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -1122,7 +1122,7 @@ class TestVersion(AMOPaths, TestCase): # Any non-disabled status with needs_human_review is enough to get a # due date, even if not signed. - NeedsHumanReview.objects.create(version=version) + nhr = NeedsHumanReview.objects.create(version=version) version.file.update(is_signed=False, status=amo.STATUS_AWAITING_REVIEW) assert version.should_have_due_date @@ -1132,12 +1132,20 @@ class TestVersion(AMOPaths, TestCase): version.file.update(is_signed=False, status=amo.STATUS_DISABLED) assert not version.should_have_due_date - # If it was signed however, it should get a due date. + # If was reported for abuse or appealed should also get a due_date, + # even if unsigned. + nhr.update(reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL) + assert version.should_have_due_date + + # Otherwise it needs to be signed to get a due date. + nhr.update(reason=NeedsHumanReview.REASONS.UNKNOWN) + assert not version.should_have_due_date version.file.update(is_signed=True) assert version.should_have_due_date # Even if deleted (which internally disables the file), as long as it # was signed and needs human review, it should keep the due date. + version.file.update(is_signed=True) version.delete() assert version.should_have_due_date @@ -1255,7 +1263,10 @@ class TestVersion(AMOPaths, TestCase): version.file.update(is_signed=False) for reason in NeedsHumanReview.REASONS.values.keys() - [ - NeedsHumanReview.REASONS.DEVELOPER_REPLY + NeedsHumanReview.REASONS.DEVELOPER_REPLY, + NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, + NeedsHumanReview.REASONS.CINDER_ESCALATION, + NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, ]: # Every other reason shouldn't result in a due date since the # version is disabled and not signed at this point.