diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index 696743447fe2..7e9bfcfd3614 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -21,10 +21,10 @@ from olympia.amo.templatetags.jinja_helpers import absolutify from olympia.amo.utils import send_mail from olympia.bandwagon.models import Collection -from olympia.blocklist.models import Block, BlocklistSubmission, BlockType +from olympia.blocklist.models import Block, BlocklistSubmission from olympia.blocklist.utils import delete_versions_from_blocks, save_versions_to_blocks from olympia.constants.abuse import DECISION_ACTIONS -from olympia.constants.blocklist import BlockReason +from olympia.constants.blocklist import BlockReason, BlockType from olympia.constants.permissions import ADDONS_HIGH_IMPACT_APPROVE from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT from olympia.files.models import File @@ -1095,6 +1095,28 @@ def get_existing_blocks_from_decision(cls, decision): ) return decision._existing_blocks + def log_action( + self, + activity_log_action, + *extra_args, + extra_details=None, + skip_private_notes=False, + ): + # Note: we're calling log_create directly, skipping the policies in + # ContentActionAddon.log_action + return log_create( + activity_log_action, + self.target, + self.decision, + *extra_args, + **( + {'user': self.decision.reviewer_user} + if self.decision.reviewer_user + else {} + ), + details={'human_review': self.is_human_reviewer(), **(extra_details or {})}, + ) + def process_action(self, release_hold=False): versions_qs = self.versions_block_will_affect # if this is a followup action, and the primary action is rejecting specific @@ -1124,6 +1146,22 @@ def process_action(self, release_hold=False): ) submission.save() + return self.log_action( + amo.LOG.BLOCKLIST_VERSION_DELAY_BLOCKED + if self.block_type == BlockType.BLOCKED + else amo.LOG.BLOCKLIST_VERSION_DELAY_SOFT_BLOCKED, + *versions, + self.delay_days, + extra_details={ + 'comments': ( + f'Add-on versions will be {self.block_type.label}, ' + f'after {self.delay_days} days, on {delayed_until.isoformat()}' + ), + 'delayed_until': delayed_until.isoformat(), + 'versions': [ver.version for ver in versions], + }, + ) + @classmethod def reverse_action(cls, *, reversed_decision, new_decision): # Note: when the original primary action was ContentDisableAddon, the versions @@ -1167,16 +1205,26 @@ def reverse_action(cls, *, reversed_decision, new_decision): ).exclude(signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED) for submission in upcoming_submissions: submission_version_ids = set(submission.changed_version_ids) - if not_blocked_version_ids == submission_version_ids: + still_block_version_ids = submission_version_ids - not_blocked_version_ids + to_not_block_version_ids = submission_version_ids & not_blocked_version_ids + + if not to_not_block_version_ids: + # if there's no crossover, ignore and continue + continue + + if not still_block_version_ids: # all versions are in the submission, so we can just delete it. submission.delete() - elif not_blocked_version_ids & submission_version_ids: - # otherwise, there's some crossover so remove offending versions. - submission.update( - changed_version_ids=list( - submission_version_ids - not_blocked_version_ids - ) - ) + else: + # otherwise, remove offending versions but keep the submission. + submission.update(changed_version_ids=list(still_block_version_ids)) + + cls(new_decision).log_action( + amo.LOG.BLOCKLIST_VERSION_DELAY_BLOCK_CANCELLED + if cls.block_type == BlockType.BLOCKED + else amo.LOG.BLOCKLIST_VERSION_DELAY_SOFT_BLOCK_CANCELLED, + *((Version, v_id) for v_id in to_not_block_version_ids), + ) @classmethod def should_be_skipped_by_automation(cls, **kwargs): diff --git a/src/olympia/abuse/tests/test_actions.py b/src/olympia/abuse/tests/test_actions.py index 65c4437705bc..8ae75fde571c 100644 --- a/src/olympia/abuse/tests/test_actions.py +++ b/src/olympia/abuse/tests/test_actions.py @@ -43,7 +43,7 @@ from olympia.files.models import File from olympia.ratings.models import Rating from olympia.reviewers.models import AutoApprovalSummary, NeedsHumanReview -from olympia.versions.models import VersionReviewerFlags +from olympia.versions.models import Version, VersionReviewerFlags from ..actions import ( ContentAction, @@ -2451,6 +2451,8 @@ class TestContentActionDelayedShortSoftBlockAddon( ActionClass = ContentActionDelayedShortSoftBlockAddon default_decision_action = DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON block_type = BlockType.SOFT_BLOCKED + activity_log_action = amo.LOG.BLOCKLIST_VERSION_DELAY_SOFT_BLOCKED + reverse_activity_log_action = amo.LOG.BLOCKLIST_VERSION_DELAY_SOFT_BLOCK_CANCELLED def setUp(self): super().setUp() @@ -2480,11 +2482,28 @@ def setUp(self): (self.version, self.old_version) ) + def test_log_action_saves_policy_texts(self): + # We don't actually save policy text with delayed actions, so testing that. + self.policy.update(text='This is {JUDGEMENT} thing') + self.decision.update( + metadata={ + ContentDecision.POLICY_DYNAMIC_VALUES: { + self.policy.uuid: {'JUDGEMENT': 'a Térrible'} + } + } + ) + assert ( + 'policy_texts' + not in self.ActionClass(self.decision) + .log_action(self.activity_log_action) + .details + ) + def _test_process_action(self, version_ids, followup_action): assert not BlocklistSubmission.objects.exists() action_helper = self.ActionClass(self.decision, followup_action) assert action_helper.action == self.default_decision_action - action_helper.process_action() + alog = action_helper.process_action() assert BlocklistSubmission.objects.count() == 1 submission = BlocklistSubmission.objects.get() assert submission.input_guids == self.addon.guid @@ -2512,6 +2531,18 @@ def _test_process_action(self, version_ids, followup_action): ) assert submission.from_followup == followup_action + assert alog.log == self.activity_log_action + assert alog.arguments == [ + self.addon, + self.decision, + *Version.objects.filter(id__in=version_ids), + self.ActionClass.delay_days, + ] + + assert str(alog).endswith( + f'Blocklist after {self.ActionClass.delay_days} days.' + ) + action_helper.notify_owners() if followup_action: assert len(mail.outbox) == 1 @@ -2655,6 +2686,18 @@ def _test_approve_appeal_or_override(self, ActionClass): assert BlocklistSubmission.objects.get().changed_version_ids == [ yet_another_version.id ] + assert ( + ActivityLog.objects.filter( + action=self.reverse_activity_log_action.id + ).count() + == 1 + ) + alog = ActivityLog.objects.get(action=self.reverse_activity_log_action.id) + assert alog.arguments == [ + self.addon, + self.decision, + self.another_version, + ] def test_approve_appeal_success(self): self.past_negative_decision.update( @@ -2751,6 +2794,8 @@ class TestContentActionDelayedMidHardBlockAddon( ActionClass = ContentActionDelayedMidHardBlockAddon default_decision_action = DECISION_ACTIONS.AMO_FU_DELAY_MID_HARD_BLOCK_ADDON block_type = BlockType.BLOCKED + activity_log_action = amo.LOG.BLOCKLIST_VERSION_DELAY_BLOCKED + reverse_activity_log_action = amo.LOG.BLOCKLIST_VERSION_DELAY_BLOCK_CANCELLED def setUp(self): super().setUp() diff --git a/src/olympia/constants/activity.py b/src/olympia/constants/activity.py index e8d1f0b1121c..5cf7dab2977c 100644 --- a/src/olympia/constants/activity.py +++ b/src/olympia/constants/activity.py @@ -1338,6 +1338,50 @@ class DECISION_CREATED(_LOG): reviewer_review_action = True +class BLOCKLIST_VERSION_DELAY_BLOCKED(_LOG): + # takes add-on, decision, version(s), int (number of days delay). {1} is the days + + id = 217 + keep = True + action_class = 'add' + hide_developer = True + format = '{addon} {version} will be added to Blocklist after {1} days.' + short = 'Version Delayed Blocked' + + +class BLOCKLIST_VERSION_DELAY_SOFT_BLOCKED(_LOG): + # takes add-on, decision, version(s), int (number of days delay). {1} is the days + + id = 218 + keep = True + action_class = 'add' + hide_developer = True + format = '{addon} {version} will be added to Soft Blocklist after {1} days.' + short = 'Version Delayed Soft Blocked' + + +class BLOCKLIST_VERSION_DELAY_BLOCK_CANCELLED(_LOG): + # takes add-on, decision, version(s) + + id = 219 + keep = True + action_class = 'delete' + hide_developer = True + format = '{addon} {version} delayed Blocklist addition cancelled.' + short = 'Version Delayed Block Cancelled' + + +class BLOCKLIST_VERSION_DELAY_SOFT_BLOCK_CANCELLED(_LOG): + # takes add-on, decision, version(s) + + id = 220 + keep = True + action_class = 'delete' + hide_developer = True + format = '{addon} {version} delayed Soft Blocklist addition cancelled.' + short = 'Version Delayed Soft Block Cancelled' + + LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG] # Make sure there's no duplicate IDs. assert len(LOGS) == len({log.id for log in LOGS}) diff --git a/src/olympia/scanners/tests/test_actions.py b/src/olympia/scanners/tests/test_actions.py index e1c17861b1db..aef6b17e177a 100644 --- a/src/olympia/scanners/tests/test_actions.py +++ b/src/olympia/scanners/tests/test_actions.py @@ -1719,10 +1719,13 @@ def test_execute_policy_enforcement_action_reject_with_followup_actions(self): assert submission.changed_version_ids == [self.version.pk] assert submission.delayed_until - assert ActivityLog.objects.count() == 1 - activity = ActivityLog.objects.latest('pk') - assert activity.action == amo.LOG.REJECT_VERSION.id - assert activity.arguments == [self.addon, decision, policy, self.version] + assert ActivityLog.objects.count() == 2 + reject_log = ActivityLog.objects.earliest('pk') + assert reject_log.action == amo.LOG.REJECT_VERSION.id + assert reject_log.arguments == [self.addon, decision, policy, self.version] + submission_log = ActivityLog.objects.latest('pk') + assert submission_log.action == amo.LOG.BLOCKLIST_VERSION_DELAY_BLOCKED.id + assert submission_log.arguments == [self.addon, decision, self.version, 14] assert len(mail.outbox) == 1 body = mail.outbox[0].body