diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index dd00fbd75d2f..b0d46123cfc1 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -1,4 +1,5 @@ import random +from collections import defaultdict from datetime import datetime from django.conf import settings @@ -18,6 +19,7 @@ from olympia.amo.templatetags.jinja_helpers import absolutify from olympia.amo.utils import send_mail from olympia.bandwagon.models import Collection +from olympia.constants.abuse import DECISION_ACTIONS from olympia.files.models import File from olympia.ratings.models import Rating from olympia.users.models import UserProfile @@ -624,3 +626,27 @@ class ContentActionAlreadyRemoved(AnyTargetMixin, NoActionMixin, ContentAction): class ContentActionNotImplemented(NoActionMixin, ContentAction): pass + + +CONTENT_ACTION_FROM_DECISION_ACTION = defaultdict( + lambda: ContentActionNotImplemented, + { + DECISION_ACTIONS.AMO_BAN_USER: ContentActionBanUser, + DECISION_ACTIONS.AMO_DISABLE_ADDON: ContentActionDisableAddon, + DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON: ContentActionRejectVersion, + DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON: ( + ContentActionRejectVersionDelayed + ), + DECISION_ACTIONS.AMO_ESCALATE_ADDON: ContentActionForwardToReviewers, + DECISION_ACTIONS.AMO_DELETE_COLLECTION: ContentActionDeleteCollection, + DECISION_ACTIONS.AMO_DELETE_RATING: ContentActionDeleteRating, + DECISION_ACTIONS.AMO_APPROVE: ContentActionApproveNoAction, + DECISION_ACTIONS.AMO_APPROVE_VERSION: ContentActionApproveInitialDecision, + DECISION_ACTIONS.AMO_IGNORE: ContentActionIgnore, + DECISION_ACTIONS.AMO_CLOSED_NO_ACTION: ContentActionAlreadyRemoved, + DECISION_ACTIONS.AMO_LEGAL_FORWARD: ContentActionForwardToLegal, + DECISION_ACTIONS.AMO_CHANGE_PENDING_REJECTION_DATE: ( + ContentActionChangePendingRejectionDate + ), + }, +) diff --git a/src/olympia/abuse/admin.py b/src/olympia/abuse/admin.py index e2ac26c3e2df..cfd588f580c6 100644 --- a/src/olympia/abuse/admin.py +++ b/src/olympia/abuse/admin.py @@ -374,7 +374,6 @@ class CinderPolicyAdmin(AMOModelAdmin): 'text', 'expose_in_reviewer_tools', 'present_in_cinder', - 'default_cinder_action', ) list_display = ( 'uuid', @@ -383,12 +382,10 @@ class CinderPolicyAdmin(AMOModelAdmin): 'linked_review_reasons', 'expose_in_reviewer_tools', 'present_in_cinder', - 'default_cinder_action', + 'enforcement_actions', 'text', ) - readonly_fields = tuple( - set(fields) - {'expose_in_reviewer_tools', 'default_cinder_action'} - ) + readonly_fields = tuple(set(fields) - {'expose_in_reviewer_tools'}) ordering = ('parent__name', 'name') list_select_related = ('parent',) view_on_site = False diff --git a/src/olympia/abuse/migrations/0051_add_cinderpolicy_enforcement_actions.py b/src/olympia/abuse/migrations/0051_add_cinderpolicy_enforcement_actions.py new file mode 100644 index 000000000000..e0637d624a6e --- /dev/null +++ b/src/olympia/abuse/migrations/0051_add_cinderpolicy_enforcement_actions.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.19 on 2025-03-10 15:51 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('abuse', '0050_contentdecision_metadata'), + ] + + operations = [ + migrations.AddField( + model_name='cinderpolicy', + name='enforcement_actions', + field=models.JSONField(default=list, null=True), + ), + migrations.AlterField( + model_name='contentdecision', + name='action', + field=models.PositiveSmallIntegerField(choices=[(1, 'User ban'), (2, 'Add-on disable'), (3, 'Forward add-on to reviewers'), (5, 'Rating delete'), (6, 'Collection delete'), (7, 'Approved (no action)'), (8, 'Add-on version reject'), (9, 'Add-on version delayed reject warning'), (10, 'Approved (new version approval)'), (11, 'Invalid report, so ignored'), (12, 'Content already removed (no action)'), (13, 'Forward add-on to legal'), (14, 'Pending rejection date changed')]), + ), + ] diff --git a/src/olympia/abuse/migrations/0052_auto_fill_cinderpolicy_enforcement_actions.py b/src/olympia/abuse/migrations/0052_auto_fill_cinderpolicy_enforcement_actions.py new file mode 100644 index 000000000000..64a6c5e2de27 --- /dev/null +++ b/src/olympia/abuse/migrations/0052_auto_fill_cinderpolicy_enforcement_actions.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.19 on 2025-03-12 11:36 + +from django.db import migrations + + + +def fill_enforcement_actions(apps, schema_editor): + ACTIONS = {7: 'amo-approve', 11: 'amo-ignore'} + CinderPolicy = apps.get_model('abuse', 'CinderPolicy') + for policy in CinderPolicy.objects.filter(default_cinder_action__isnull=False): + if api_value := ACTIONS.get(policy.default_cinder_action): + policy.update(enforcement_actions=[api_value]) + + +class Migration(migrations.Migration): + + dependencies = [ + ('abuse', '0051_add_cinderpolicy_enforcement_actions'), + ] + + operations = [ + migrations.RunPython(fill_enforcement_actions, migrations.RunPython.noop) + ] diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index 39b455e73aab..89c8862be86c 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -29,21 +29,8 @@ from olympia.versions.models import Version, VersionReviewerFlags from .actions import ( - ContentActionAlreadyRemoved, - ContentActionApproveInitialDecision, - ContentActionApproveNoAction, - ContentActionBanUser, - ContentActionChangePendingRejectionDate, - ContentActionDeleteCollection, - ContentActionDeleteRating, - ContentActionDisableAddon, - ContentActionForwardToLegal, - ContentActionForwardToReviewers, - ContentActionIgnore, - ContentActionNotImplemented, + CONTENT_ACTION_FROM_DECISION_ACTION, ContentActionOverrideApprove, - ContentActionRejectVersion, - ContentActionRejectVersionDelayed, ContentActionTargetAppealApprove, ContentActionTargetAppealRemovalAffirmation, ) @@ -892,9 +879,7 @@ class CinderPolicy(ModelBase): related_name='children', ) expose_in_reviewer_tools = models.BooleanField(default=False) - default_cinder_action = models.PositiveSmallIntegerField( - choices=DECISION_ACTIONS.choices, null=True, blank=True - ) + enforcement_actions = models.JSONField(default=list, null=True) present_in_cinder = models.BooleanField(null=True) objects = CinderPolicyQuerySet.as_manager() @@ -916,6 +901,23 @@ def full_text(self, canned_response_text=None): class Meta: verbose_name_plural = 'Cinder Policies' + @classmethod + def get_decision_actions_from_policies(cls, policies, *, for_entity=None): + actions = { + action.value + for policy in policies + for api_value in policy.enforcement_actions + if policy.enforcement_actions + and DECISION_ACTIONS.has_api_value(api_value) + and (action := DECISION_ACTIONS.for_api_value(api_value)) + and ( + not for_entity + or for_entity + in CONTENT_ACTION_FROM_DECISION_ACTION[action.value].valid_targets + ) + } + return list(actions) + class ContentDecisionManager(ManagerBase): def awaiting_action(self): @@ -1070,31 +1072,9 @@ def get_target_display(self): def is_third_party_initiated(self): return bool((job := self.originating_job) and job.all_abuse_reports) - @classmethod - def get_action_helper_class(cls, decision_action): - return { - DECISION_ACTIONS.AMO_BAN_USER: ContentActionBanUser, - DECISION_ACTIONS.AMO_DISABLE_ADDON: ContentActionDisableAddon, - DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON: ContentActionRejectVersion, - DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON: ( - ContentActionRejectVersionDelayed - ), - DECISION_ACTIONS.AMO_ESCALATE_ADDON: ContentActionForwardToReviewers, - DECISION_ACTIONS.AMO_DELETE_COLLECTION: ContentActionDeleteCollection, - DECISION_ACTIONS.AMO_DELETE_RATING: ContentActionDeleteRating, - DECISION_ACTIONS.AMO_APPROVE: ContentActionApproveNoAction, - DECISION_ACTIONS.AMO_APPROVE_VERSION: ContentActionApproveInitialDecision, - DECISION_ACTIONS.AMO_IGNORE: ContentActionIgnore, - DECISION_ACTIONS.AMO_CLOSED_NO_ACTION: ContentActionAlreadyRemoved, - DECISION_ACTIONS.AMO_LEGAL_FORWARD: ContentActionForwardToLegal, - DECISION_ACTIONS.AMO_CHANGE_PENDING_REJECTION_DATE: ( - ContentActionChangePendingRejectionDate - ), - }.get(decision_action, ContentActionNotImplemented) - def get_action_helper(self): # Base case when it's a new decision, that wasn't an appeal - ContentActionClass = self.get_action_helper_class(self.action) + ContentActionClass = CONTENT_ACTION_FROM_DECISION_ACTION[self.action] skip_reporter_notify = False any_cinder_job = self.originating_job diff --git a/src/olympia/abuse/tasks.py b/src/olympia/abuse/tasks.py index 865c5d6b322c..403f5e66e4bc 100644 --- a/src/olympia/abuse/tasks.py +++ b/src/olympia/abuse/tasks.py @@ -14,6 +14,7 @@ from olympia.amo.celery import task from olympia.amo.decorators import use_primary_db from olympia.amo.utils import to_language +from olympia.constants.abuse import DECISION_ACTIONS from olympia.reviewers.models import NeedsHumanReview, UsageTier from olympia.users.models import UserProfile @@ -171,6 +172,11 @@ def sync_policies(data, parent_id=None): # If the policy is labelled, but not for AMO, skip it continue policies_in_cinder.add(policy['uuid']) + actions = [ + action['slug'] + for action in policy.get('enforcement_actions', []) + if DECISION_ACTIONS.has_api_value(action['slug']) + ] cinder_policy, _ = CinderPolicy.objects.update_or_create( uuid=policy['uuid'], defaults={ @@ -179,6 +185,7 @@ def sync_policies(data, parent_id=None): 'parent_id': parent_id, 'modified': datetime.now(), 'present_in_cinder': True, + 'enforcement_actions': actions, }, ) diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index de768ba44b37..12450ee8f058 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -35,9 +35,11 @@ from olympia.core import set_user from olympia.ratings.models import Rating from olympia.reviewers.models import NeedsHumanReview +from olympia.users.models import UserProfile from olympia.versions.models import Version, VersionReviewerFlags from ..actions import ( + CONTENT_ACTION_FROM_DECISION_ACTION, ContentActionBanUser, ContentActionDeleteCollection, ContentActionDeleteRating, @@ -1953,6 +1955,46 @@ def test_without_parents_if_their_children_are_present(self): lone_policy, } + def test_get_decision_actions_from_policies(self): + policies = ( + # no actions, ignored + CinderPolicy.objects.create(uuid='1', enforcement_actions=[]), + # multiple actions + CinderPolicy.objects.create( + uuid='2', + enforcement_actions=[ + 'amo-disable-addon', + 'amo-approve', + 'amo-ban-user', + ], + ), + # some duplicates, and unsupported actions + CinderPolicy.objects.create( + uuid='3', enforcement_actions=['amo-disable-addon', 'not-amo-action'] + ), + ) + assert sorted(CinderPolicy.get_decision_actions_from_policies(policies)) == [ + DECISION_ACTIONS.AMO_BAN_USER, + DECISION_ACTIONS.AMO_DISABLE_ADDON, + DECISION_ACTIONS.AMO_APPROVE, + ] + + assert sorted( + CinderPolicy.get_decision_actions_from_policies(policies, for_entity=Addon) + ) == [ + DECISION_ACTIONS.AMO_DISABLE_ADDON, + DECISION_ACTIONS.AMO_APPROVE, + ] + + assert sorted( + CinderPolicy.get_decision_actions_from_policies( + policies, for_entity=UserProfile + ) + ) == [ + DECISION_ACTIONS.AMO_BAN_USER, + DECISION_ACTIONS.AMO_APPROVE, + ] + class TestContentDecisionManager(TestCase): def test_held_for_2nd_level_approval(self): @@ -2121,7 +2163,7 @@ def test_get_action_helper(self): }, } action_to_class = [ - (decision_action, ContentDecision.get_action_helper_class(decision_action)) + (decision_action, CONTENT_ACTION_FROM_DECISION_ACTION[decision_action]) for decision_action in DECISION_ACTIONS.values ] # base cases, where it's a decision without an override or appeal involved @@ -2188,7 +2230,7 @@ def test_get_action_helper(self): ) action_existing_to_class_no_reporter_emails = { - (action, action): ContentDecision.get_action_helper_class(action) + (action, action): CONTENT_ACTION_FROM_DECISION_ACTION[action] for action in DECISION_ACTIONS.REMOVING.values } for ( @@ -2245,7 +2287,7 @@ def test_get_action_helper_override(self): # But if there is no action_date the override is ignored action_existing_to_class[(approve_action, action, None, None)] = ( - ContentDecision.get_action_helper_class(approve_action) + CONTENT_ACTION_FROM_DECISION_ACTION[approve_action] ) # Previous decisions are also considered though diff --git a/src/olympia/abuse/tests/test_tasks.py b/src/olympia/abuse/tests/test_tasks.py index d2ac6499595f..0d87e487a1c8 100644 --- a/src/olympia/abuse/tests/test_tasks.py +++ b/src/olympia/abuse/tests/test_tasks.py @@ -837,6 +837,10 @@ def setUp(self): 'name': 'test-name', 'description': 'test-description', 'nested_policies': [], + 'enforcement_actions': [ + {'slug': 'amo-disable-addon'}, + {'slug': 'amo-ban-user'}, + ], } def test_sync_cinder_policies_headers(self): @@ -1103,6 +1107,35 @@ def test_only_amo_labelled_policies_added(self): assert CinderPolicy.objects.count() == 6 assert CinderPolicy.objects.filter(text='ADDED').count() == 6 + def test_enforcement_actions_synced(self): + data = [ + { + 'uuid': 'no-actions', + 'name': 'no actions', + 'description': '', + 'enforcement_actions': [], + }, + { + 'uuid': 'multiple', + 'name': 'multiple', + 'description': '', + 'enforcement_actions': [ + {'slug': 'amo-disable-addon'}, + {'slug': 'amo-approve'}, + {'slug': 'amo-ban-user'}, + {'slug': 'some-unsupported-action'}, + ], + }, + ] + + responses.add(responses.GET, self.url, json=data, status=200) + sync_cinder_policies.delay() + assert CinderPolicy.objects.get(uuid='multiple').enforcement_actions == [ + 'amo-disable-addon', + 'amo-approve', + 'amo-ban-user', + ] + def do_handle_escalate_action(*, from_2nd_level, expected_nhr_reason): user_factory(id=settings.TASK_USER_ID) diff --git a/src/olympia/abuse/views.py b/src/olympia/abuse/views.py index 1e3f796345ff..db873fb83899 100644 --- a/src/olympia/abuse/views.py +++ b/src/olympia/abuse/views.py @@ -31,6 +31,7 @@ from olympia.ratings.views import RatingViewSet from olympia.users.models import UserProfile +from .actions import CONTENT_ACTION_FROM_DECISION_ACTION from .cinder import CinderAddon from .forms import AbuseAppealEmailForm, AbuseAppealForm from .models import AbuseReport, CinderJob, ContentDecision @@ -178,7 +179,7 @@ def filter_enforcement_actions(enforcement_actions, cinder_job): if DECISION_ACTIONS.has_api_value(action_slug) and (action := DECISION_ACTIONS.for_api_value(action_slug)) and target.__class__ - in ContentDecision.get_action_helper_class(action.value).valid_targets + in CONTENT_ACTION_FROM_DECISION_ACTION[action.value].valid_targets ] diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index 0c7e9811edb2..39a5014a7159 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -504,7 +504,7 @@ def clean(self): if self.cleaned_data.get('cinder_jobs_to_resolve') and self.cleaned_data.get( 'cinder_policies' ): - actions = self.helper.handler.get_cinder_actions_from_policies( + actions = self.helper.handler.get_decision_actions_from_policies( self.cleaned_data.get('cinder_policies') ) if len(actions) == 0: diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index c29b1cb6a31b..8df85015a6eb 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -417,7 +417,7 @@ def test_policies_required_with_cinder_jobs(self): uuid='x', name='ok', expose_in_reviewer_tools=True, - default_cinder_action=DECISION_ACTIONS.AMO_IGNORE, + enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value], ) form = self.get_form() assert not form.is_bound @@ -478,19 +478,19 @@ def test_only_one_cinder_action_selected(self): uuid='a', name='ignore', expose_in_reviewer_tools=True, - default_cinder_action=DECISION_ACTIONS.AMO_IGNORE, + enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value], ) action_policy_b = CinderPolicy.objects.create( uuid='b', name='ignore again', expose_in_reviewer_tools=True, - default_cinder_action=DECISION_ACTIONS.AMO_IGNORE, + enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value], ) action_policy_c = CinderPolicy.objects.create( uuid='c', name='approve', expose_in_reviewer_tools=True, - default_cinder_action=DECISION_ACTIONS.AMO_APPROVE, + enforcement_actions=[DECISION_ACTIONS.AMO_APPROVE.api_value], ) form = self.get_form() assert not form.is_bound @@ -537,7 +537,7 @@ def test_cinder_jobs_filtered_for_resolve_reports_job_and_resolve_appeal_job(sel uuid='a', name='ignore', expose_in_reviewer_tools=True, - default_cinder_action=DECISION_ACTIONS.AMO_IGNORE, + enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value], ) data = { diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index e15ca82c1d67..bf9cb078c42f 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -1090,7 +1090,8 @@ def test_record_decision_sets_policies_with_allow_policies(self, report_mock): 'cinder_policies': [ CinderPolicy.objects.create(uuid='x'), CinderPolicy.objects.create( - uuid='z', default_cinder_action=DECISION_ACTIONS.AMO_IGNORE + uuid='z', + enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value], ), ], 'cinder_jobs_to_resolve': [cinder_job], @@ -3636,7 +3637,7 @@ def test_enable_addon_version_is_awaiting_review_fall_back_to_nominated(self): def _record_decision_called_everywhere_checkbox_shown(self, actions): job, _ = CinderJob.objects.get_or_create(job_id='1234') policy, _ = CinderPolicy.objects.get_or_create( - default_cinder_action=DECISION_ACTIONS.AMO_APPROVE + enforcement_actions=[DECISION_ACTIONS.AMO_APPROVE.api_value] ) self.helper.handler.data = { 'versions': [self.review_version], @@ -3688,7 +3689,7 @@ def _record_decision_called_everywhere_checkbox_shown(self, actions): ) assert ( decision.action == actions[action_name]['cinder_action'] - or policy.default_cinder_action + or policy.enforcement_actions ) assert job.decision == decision diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index eeafd4316024..0f748f6ea526 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -2613,7 +2613,7 @@ def test_resolve_reports_job(self, resolve_mock): policy = CinderPolicy.objects.create( uuid='x', expose_in_reviewer_tools=True, - default_cinder_action=DECISION_ACTIONS.AMO_IGNORE, + enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value], ) AbuseReport.objects.create( guid=self.addon.guid, @@ -7539,7 +7539,9 @@ def setUp(self): CinderPolicy.objects.create(uuid='1', name='Bad Things') ) CinderPolicy.objects.create( - uuid='2', name='Approve', default_cinder_action=DECISION_ACTIONS.AMO_APPROVE + uuid='2', + name='Approve', + enforcement_actions=[DECISION_ACTIONS.AMO_APPROVE.api_value], ) # CinderJob.objects.create(cinder) self.url = reverse('reviewers.decision_review', args=(self.decision.id,)) diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index ba4bc8997fe4..87ba32ea7995 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -989,7 +989,7 @@ def record_decision( # If there isn't a cinder_action from the activity action already, get # it from the policy. There should only be one in the list as form # validation raises for multiple cinder actions. - (actions := self.get_cinder_actions_from_policies(policies)) + (actions := self.get_decision_actions_from_policies(policies)) and actions[0].value ) assert cinder_action @@ -1095,14 +1095,18 @@ def clear_specific_needs_human_review_flags( # explicitly. version.reset_due_date() - def get_cinder_actions_from_policies(self, policies): - return list( - { - DECISION_ACTIONS.for_value(policy.default_cinder_action) - for policy in policies - if getattr(policy, 'default_cinder_action', None) - } + def get_decision_actions_from_policies(self, policies): + actions = CinderPolicy.get_decision_actions_from_policies( + policies, for_entity=Addon ) + # Until https://mozilla-hub.atlassian.net/browse/AMOENG-672 completes the + # integration we don't want to accidentally disable addons based on the actions + # linked to reviewreasons, so filter out all but the approve/ignore actions + return [ + action + for action in actions + if action in (DECISION_ACTIONS.AMO_APPROVE, DECISION_ACTIONS.AMO_IGNORE) + ] def log_action( self, diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py index 480123908a47..d8fe0fd32484 100644 --- a/src/olympia/reviewers/views.py +++ b/src/olympia/reviewers/views.py @@ -1268,7 +1268,7 @@ def decision_review(request, decision_id): ) new_decision.policies.set( CinderPolicy.objects.filter( - default_cinder_action=DECISION_ACTIONS.AMO_APPROVE + enforcement_actions__in=DECISION_ACTIONS.AMO_APPROVE.api_value ) ) new_decision.execute_action(release_hold=True)