diff --git a/src/olympia/blocklist/models.py b/src/olympia/blocklist/models.py index 34011211525c..0ab066f40154 100644 --- a/src/olympia/blocklist/models.py +++ b/src/olympia/blocklist/models.py @@ -346,11 +346,34 @@ def all_adu_safe(self): def has_version_changes(self): return bool(self.changed_version_ids) - def update_signoff_for_auto_approval(self): + def has_potential_conflicts(self, *, ignoring=None): + """Check whether or not this blocklistsubmission contains versions that + are part of another non-published blocklistsubmission. + + An optional list of blocklistsubmissions to ignore when performing + the check can be passed.""" + if ignoring is None: + ignoring = [self] + if self.pk not in ignoring: + ignoring = ignoring + [self] + submissions = self.get_all_submission_versions(ignoring=ignoring) + return set(self.changed_version_ids) & set(submissions) + + def update_signoff_for_auto_approval(self, *, ignoring=None): + """Update signoff state to auto-approved if possible. + + An optional list of blocklistsubmissions to ignore when performing + the check for conflicts can be passed. + """ is_pending = self.signoff_state == self.SIGNOFF_STATES.PENDING add_action = self.action == self.ACTIONS.ADDCHANGE if is_pending and ( - self.all_adu_safe() or add_action and not self.has_version_changes() + ( + self.all_adu_safe() + and not self.has_potential_conflicts(ignoring=ignoring) + ) + or add_action + and not self.has_version_changes() ): self.update(signoff_state=self.SIGNOFF_STATES.AUTOAPPROVED) @@ -548,10 +571,16 @@ def get_submissions_from_version_id(cls, version_id): ).filter(changed_version_ids__contains=version_id) @classmethod - def get_all_submission_versions(cls): - submission_qs = cls.objects.exclude( - signoff_state__in=cls.SIGNOFF_STATES.STATES_FINISHED.values - ).values_list('id', 'changed_version_ids') + def get_all_submission_versions(cls, *, ignoring=None): + if ignoring is None: + ignoring = [] + submission_qs = ( + cls.objects.exclude( + signoff_state__in=cls.SIGNOFF_STATES.STATES_FINISHED.values + ) + .exclude(id__in=[ignored.id for ignored in ignoring]) + .values_list('id', 'changed_version_ids') + ) return { ver_id: sub_id for sub_id, id_list in submission_qs for ver_id in id_list } diff --git a/src/olympia/blocklist/tasks.py b/src/olympia/blocklist/tasks.py index a7913191237d..7758c3e9bb6d 100644 --- a/src/olympia/blocklist/tasks.py +++ b/src/olympia/blocklist/tasks.py @@ -1,3 +1,4 @@ +import itertools import json import os import re @@ -13,11 +14,15 @@ from django_statsd.clients import statsd import olympia.core.logger -from olympia import amo +from olympia import amo, core from olympia.amo.celery import task from olympia.amo.decorators import use_primary_db from olympia.amo.utils import SafeStorage -from olympia.constants.blocklist import REMOTE_SETTINGS_COLLECTION_MLBF, BlockListAction +from olympia.constants.blocklist import ( + REASON_USER_BANNED, + REMOTE_SETTINGS_COLLECTION_MLBF, + BlockListAction, +) from olympia.lib.remote_settings import RemoteSettings from olympia.zadmin.models import get_config, set_config @@ -241,3 +246,137 @@ def cleanup_old_files(*, base_filter_id): else: log.info('Deleting %s because > 6 months old (%s)', dir, dir_as_date) storage.rm_stored_dir(os.path.join(settings.MLBF_STORAGE_PATH, dir)) + + +@task +@use_primary_db +def block_addons_on_user_ban(addonusers_ids): + """Automatically create BlocklistSubmission for add-ons corresponding to + AddonUser ids passed, and record them as BlockedAddonsSubmissionsModel on + the corresponding user. + + Blocks created from this task should not disable the relevant add-ons, + that should be done separately. This ensures those blocks are silent, + as this is meant to be used on ban which will already notify the affected + user(s).""" + from olympia.addons.models import AddonUser + from olympia.users.models import BannedUserContent + + sole_addonusers_qs = ( + AddonUser.objects.filter(pk__in=addonusers_ids) + .values('user', 'addon__guid') + .order_by('user') + ) + BlockedAddonsSubmissionsModel = BannedUserContent.blocked_addons_submissions.through + banned_blocklist_submissions = [] + for user_id, values in itertools.groupby( + sole_addonusers_qs, + key=lambda values: values['user'], + ): + # For each user, create up to 2 submissions: one for versions + # that weren't already soft-blocked, one for versions that were + # and need to be upgraded to a hard-block. We can ignore + # add-ons that were entirely blocked before. + new_blocks_versions = [] + new_blocks_guids = set() + existing_blocks_versions = [] + existing_blocks_guids = set() + processed = BlocklistSubmission.process_input_guids( + '\r\n'.join([value['addon__guid'] for value in values]), + filter_existing=False, + load_full_objects=False, + ) + for block in processed['blocks']: + for version in block.addon_versions: + if version.is_blocked: + if version.is_soft_blocked: + existing_blocks_versions.append(version.id) + existing_blocks_guids.add(block.guid) + # Nothing to do if the version was hard-blocked. + else: + new_blocks_versions.append(version.id) + new_blocks_guids.add(block.guid) + if new_blocks_versions and new_blocks_guids: + banned_blocklist_submissions.append( + BlockedAddonsSubmissionsModel( + bannedusercontent_id=user_id, + blocklistsubmission=BlocklistSubmission( + action=BlocklistSubmission.ACTIONS.ADDCHANGE, + reason=REASON_USER_BANNED, + updated_by=core.get_user(), + input_guids='\r\n'.join(new_blocks_guids), + changed_version_ids=new_blocks_versions, + # Add-ons will already be disabled above. + disable_addon=False, + ), + ) + ) + if existing_blocks_versions and existing_blocks_guids: + banned_blocklist_submissions.append( + BlockedAddonsSubmissionsModel( + bannedusercontent_id=user_id, + blocklistsubmission=BlocklistSubmission( + action=BlocklistSubmission.ACTIONS.HARDEN, + reason=REASON_USER_BANNED, + updated_by=core.get_user(), + input_guids='\r\n'.join(existing_blocks_guids), + changed_version_ids=existing_blocks_versions, + # Add-ons will already be disabled above. + disable_addon=False, + ), + ) + ) + for banned_blocklist_submission in banned_blocklist_submissions: + submission = banned_blocklist_submission.blocklistsubmission + submission.save() + submission.update_signoff_for_auto_approval( + # We ignore conflicts caused by other submissions in this ban, we + # want them to be recorded separately for each user so that it can + # be undone individually if only one of them gets unbanned. + ignoring=[bb.blocklistsubmission for bb in banned_blocklist_submissions] + ) + if submission.is_submission_ready: + process_blocklistsubmission.delay(submission.id) + + # Keep track of the blocklist submissions we created to undo them + # on unban if they have been published. + BlockedAddonsSubmissionsModel.objects.bulk_create(banned_blocklist_submissions) + + +@task +@use_primary_db +def revert_published_blocklist_submissions(submission_ids): + """Automatically create "opposite" BlocklistSubmissions to the ones passed + in argument that have been published, effectively reverting them. + + For safety this doesn't allow reverting a DELETE action.""" + submissions = BlocklistSubmission.objects.filter( + pk__in=submission_ids, + signoff_state=BlocklistSubmission.SIGNOFF_STATES.PUBLISHED, + ) + new_submissions = [] + for submission in submissions: + if submission.action == BlocklistSubmission.ACTIONS.ADDCHANGE: + submission.action = BlocklistSubmission.ACTIONS.DELETE + elif submission.action == BlocklistSubmission.ACTIONS.HARDEN: + submission.action = BlocklistSubmission.ACTIONS.SOFTEN + elif submission.action == BlocklistSubmission.ACTIONS.SOFTEN: + submission.action = BlocklistSubmission.ACTIONS.HARDEN + else: + log.error( + 'Unexpected BlocklistSubmission passed to ' + 'revert_published_blocklist_submissions %s', + submission.pk, + ) + continue + submission.pk = None + submission.updated_by = core.get_user() + submission.reason = f'Revert "{submission.reason}"' + submission.signoff_state = BlocklistSubmission.SIGNOFF_STATES.PENDING + submission.save() + new_submissions.append(submission) + + for submission in new_submissions: + submission.update_signoff_for_auto_approval(ignoring=new_submissions) + if submission.is_submission_ready: + process_blocklistsubmission.delay(submission.id) diff --git a/src/olympia/lib/settings_base.py b/src/olympia/lib/settings_base.py index 309e23d1f951..e4cd169b8b1a 100644 --- a/src/olympia/lib/settings_base.py +++ b/src/olympia/lib/settings_base.py @@ -747,6 +747,10 @@ def get_language_url_map(): 'olympia.addons.tasks.index_addons': {'queue': 'priority'}, 'olympia.blocklist.tasks.process_blocklistsubmission': {'queue': 'priority'}, 'olympia.blocklist.tasks.upload_filter': {'queue': 'priority'}, + 'olympia.blocklist.tasks.block_addons_on_user_ban': {'queue': 'priority'}, + 'olympia.blocklist.tasks.revert_published_blocklist_submissions': { + 'queue': 'priority' + }, 'olympia.versions.tasks.generate_static_theme_preview': {'queue': 'priority'}, # Adhoc # A queue to be used for one-off tasks that could be resource intensive or diff --git a/src/olympia/users/migrations/0025_bannedusercontent_blocked_addons_submissions.py b/src/olympia/users/migrations/0025_bannedusercontent_blocked_addons_submissions.py new file mode 100644 index 000000000000..73dab10aa526 --- /dev/null +++ b/src/olympia/users/migrations/0025_bannedusercontent_blocked_addons_submissions.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.28 on 2026-03-02 20:53 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('blocklist', '0042_alter_blockversion_block_type'), + ('users', '0024_alter_userprofile_fxa_id'), + ] + + operations = [ + migrations.AddField( + model_name='bannedusercontent', + name='blocked_addons_submissions', + field=models.ManyToManyField(to='blocklist.blocklistsubmission'), + ), + ] diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index 2f7f8acdcfc3..f80dc616d9a0 100644 --- a/src/olympia/users/models.py +++ b/src/olympia/users/models.py @@ -50,7 +50,6 @@ id_to_path, ) from olympia.amo.validators import OneOrMoreLetterOrNumberCharacterValidator -from olympia.constants.blocklist import REASON_USER_BANNED from olympia.files.models import File from olympia.translations.query import order_by_translation from olympia.users.notifications import NOTIFICATIONS_BY_ID @@ -154,8 +153,7 @@ def ban_and_disable_related_content( from olympia.addons.models import Addon, AddonUser from olympia.addons.tasks import index_addons from olympia.bandwagon.models import Collection - from olympia.blocklist.models import BlocklistSubmission - from olympia.blocklist.tasks import process_blocklistsubmission + from olympia.blocklist.tasks import block_addons_on_user_ban from olympia.ratings.models import Rating from olympia.users.tasks import delete_photo @@ -241,25 +239,9 @@ def ban_and_disable_related_content( # Hard-block all versions of addons we force disabled, if the relevant # boolean is True. if hard_block_addons: - submission = BlocklistSubmission( - action=BlocklistSubmission.ACTIONS.ADDCHANGE, - input_guids='\r\n'.join([addon.guid for addon in addons_sole]), - reason=REASON_USER_BANNED, - updated_by=core.get_user(), - disable_addon=False, # Add-ons will already be disabled above. + block_addons_on_user_ban.delay( + list(sole_addonusers_qs.values_list('pk', flat=True)) ) - submission.changed_version_ids = [ - version.id - for block in submission.process_input_guids( - submission.input_guids, load_full_objects=False - )['blocks'] - for version in block.addon_versions - if not version.is_blocked - ] - submission.save() - submission.update_signoff_for_auto_approval() - if submission.is_submission_ready: - process_blocklistsubmission.delay(submission.id) # Soft-delete the other content associated with the user: Ratings and # Collections. @@ -313,7 +295,18 @@ def ban_and_disable_related_content( def unban_and_reenable_related_content(self, *, skip_activity_log=False): """Admin method to unban users and restore their content that was disabled when they were banned.""" - for user in self: + from olympia.blocklist.models import BlocklistSubmission + from olympia.blocklist.tasks import revert_published_blocklist_submissions + + users = self.all() + # Grab blocklist submissions we need to revert before deleting the + # BannedUserContent instances and the relationships... + blocklist_submissions_pks = list( + BlocklistSubmission.objects.filter( + bannedusercontent__user__in=users + ).values_list('pk', flat=True) + ) + for user in users: banned_user_content = BannedUserContent.objects.filter(user=user).first() if banned_user_content: banned_user_content.restore() @@ -325,6 +318,7 @@ def unban_and_reenable_related_content(self, *, skip_activity_log=False): EmailUserRestriction.objects.filter( email_pattern=EmailUserRestriction.normalize_email(user.email) ).delete() + revert_published_blocklist_submissions.delay(blocklist_submissions_pks) class UserManager(BaseUserManager, ManagerBase): @@ -1488,11 +1482,10 @@ def watch_changes(old_attr=None, new_attr=None, instance=None, sender=None, **kw class BannedUserContent(ModelBase): - """Link between a user and the content that was disabled when they were - banned. + """Link between a user and the content we altered when they were banned. That link should be removed if the user is unbanned, and the content - re-enabled. + restored as it was before. """ user = models.OneToOneField( @@ -1503,6 +1496,7 @@ class BannedUserContent(ModelBase): ) collections = models.ManyToManyField('bandwagon.Collection') addons = models.ManyToManyField('addons.Addon') + blocked_addons_submissions = models.ManyToManyField('blocklist.BlocklistSubmission') addons_users = models.ManyToManyField('addons.AddonUser') ratings = models.ManyToManyField('ratings.Rating') picture_backup_name = models.CharField( @@ -1535,6 +1529,7 @@ def restore(self): # If something wrong happens here, we won't restore the picture # but we want to be able to continue. log.exception(e) + activity.log_create(amo.LOG.ADMIN_USER_CONTENT_RESTORED, self.user) self.delete() # Should delete the ManyToMany relationships diff --git a/src/olympia/users/tests/test_models.py b/src/olympia/users/tests/test_models.py index 47fa3770f091..efa84d3109d1 100644 --- a/src/olympia/users/tests/test_models.py +++ b/src/olympia/users/tests/test_models.py @@ -25,6 +25,7 @@ from olympia.amo.tests import ( TestCase, addon_factory, + block_factory, collection_factory, user_factory, version_factory, @@ -358,6 +359,11 @@ def test_ban_and_disable_related_content_bulk( f'{user_sole.pk}_original.png' ) + if hard_block_addons: + assert banned_content.blocked_addons_submissions.get() == submission + else: + assert not banned_content.blocked_addons_submissions.exists() + banned_content = user_multi.content_disabled_on_ban self.assertQuerySetContentsEqual( banned_content.ratings(manager='unfiltered_for_relations').all(), @@ -382,6 +388,7 @@ def test_ban_and_disable_related_content_bulk( # Generated by our mock above - real one would be a hexdigest hash f'{user_multi.pk}_original.png' ) + assert not banned_content.blocked_addons_submissions.exists() assert not self.storage.exists(user_sole.picture_path) assert not self.storage.exists(user_sole.picture_path_original) @@ -499,9 +506,6 @@ def test_ban_and_disable_related_content_bulk_with_hard_block_not_auto_approved( self, ): user_factory(pk=settings.TASK_USER_ID) - fake_admin = user_factory(display_name='Fake Admin') - core.set_user(fake_admin) # Needed for activity log - user = user_factory(display_name='Ban Me Please') addon = addon_factory( users=[user], @@ -525,6 +529,152 @@ def test_ban_and_disable_related_content_bulk_with_hard_block_not_auto_approved( assert submission.changed_version_ids == [version.pk] assert submission.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING assert not Block.objects.exists() + # BlocklistSubmission is still linked to the ban + assert ( + user.content_disabled_on_ban.blocked_addons_submissions.get() == submission + ) + + def test_ban_and_disable_related_content_bulk_with_hard_block_existing_harden(self): + user_factory(pk=settings.TASK_USER_ID) + fake_admin = user_factory(display_name='Fake Admin') + core.set_user(fake_admin) + user = user_factory(display_name='Ban Me Please') + soft_blocked_addon = addon_factory( + name='Was soft blocked', users=[user], average_daily_users=1 + ) + block_factory( + guid=soft_blocked_addon.guid, + block_type=BlockType.SOFT_BLOCKED, + updated_by=fake_admin, + ) + regular_addon = addon_factory( + name='Regular', users=[user], average_daily_users=2 + ) + + UserProfile.objects.filter(pk__in=[user.pk]).ban_and_disable_related_content( + hard_block_addons=True + ) + + user.reload() + assert user.banned + for addon in [regular_addon, soft_blocked_addon]: + version = addon.current_version + addon.reload() + version.reload() + assert addon.status == amo.STATUS_DISABLED + # We should have 1 BlocklistSubmissions for each add-on: one for + # blocking, one for hardening. + submission = BlocklistSubmission.objects.get(input_guids=addon.guid) + assert submission.changed_version_ids == [version.pk] + assert ( + submission.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED + ) + + # In the end both add-ons should have had their versions hard-blocked + assert version.is_blocked + assert version.blockversion.block_type == BlockType.BLOCKED + assert version.blockversion.block.reason == REASON_USER_BANNED + + assert BlocklistSubmission.objects.count() == 2 + + assert ( + BlocklistSubmission.objects.get(input_guids=soft_blocked_addon.guid).action + == BlocklistSubmission.ACTIONS.HARDEN + ) + + assert set( + user.content_disabled_on_ban.blocked_addons_submissions.all() + ) == set(BlocklistSubmission.objects.all()) + + def test_ban_and_disable_related_content_bulk_with_hard_block_authors_mixup(self): + user_factory(pk=settings.TASK_USER_ID) + fake_admin = user_factory(display_name='Fake Admin') + core.set_user(fake_admin) + + user1 = user_factory(display_name='Ban Me Please') + user2 = user_factory(display_name='Also Ban Me Please') + addon1 = addon_factory( + name='First Addon', users=[user1, user2], average_daily_users=1 + ) + addon2 = addon_factory( + name='Second Addon', users=[user1, user2], average_daily_users=2 + ) + UserProfile.objects.filter( + pk__in=[user1.pk, user2.pk] + ).ban_and_disable_related_content(hard_block_addons=True) + for user in [user1, user2]: + user.reload() + assert user.banned + for addon in [addon1, addon2]: + version = addon.current_version + addon.reload() + version.reload() + assert addon.status == amo.STATUS_DISABLED + # In the end both add-ons should have had their versions hard-blocked + assert version.is_blocked + assert version.blockversion.block_type == BlockType.BLOCKED + assert version.blockversion.block.reason == REASON_USER_BANNED + + # We should have 2 BlocklistSubmissions total because we have 2 + # users, both should be identical and contain both add-ons, but we + # want to track them individually in case one user gets unbanned + # and not the other. + assert BlocklistSubmission.objects.count() == 2 + for submission in BlocklistSubmission.objects.all(): + assert sorted(submission.input_guids.split('\r\n')) == sorted( + [addon1.guid, addon2.guid] + ) + assert submission.changed_version_ids == [ + addon2.versions.get().pk, + addon1.versions.get().pk, + ] + assert ( + submission.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PUBLISHED + ) + assert submission.reason == REASON_USER_BANNED + assert submission.action == BlocklistSubmission.ACTIONS.ADDCHANGE + + assert user1.content_disabled_on_ban.blocked_addons_submissions.count() == 1 + assert user2.content_disabled_on_ban.blocked_addons_submissions.count() == 1 + assert ( + user1.content_disabled_on_ban.blocked_addons_submissions.all() + != user2.content_disabled_on_ban.blocked_addons_submissions.all() + ) + + def test_ban_and_disable_related_content_bulk_with_hard_block_conflict_hold(self): + user_factory(pk=settings.TASK_USER_ID) + fake_admin = user_factory(display_name='Fake Admin') + core.set_user(fake_admin) + + user = user_factory(display_name='Ban Me Please') + addon = addon_factory( + name='Already has a submission', users=[user], average_daily_users=1 + ) + existing_submission = BlocklistSubmission.objects.create( + input_guids=addon.guid, + changed_version_ids=[addon.current_version.pk], + updated_by=fake_admin, + ) + UserProfile.objects.filter(pk__in=[user.pk]).ban_and_disable_related_content( + hard_block_addons=True + ) + user.reload() + assert user.banned + version = addon.current_version + addon.reload() + assert addon.status == amo.STATUS_DISABLED + submission = ( + BlocklistSubmission.objects.exclude(id=existing_submission.id) + .filter(input_guids=addon.guid) + .get() + ) + assert submission.changed_version_ids == [version.pk] + assert submission.signoff_state == BlocklistSubmission.SIGNOFF_STATES.PENDING + assert not Block.objects.exists() + # BlocklistSubmission is still linked to the ban + assert ( + user.content_disabled_on_ban.blocked_addons_submissions.get() == submission + ) @mock.patch('olympia.users.models.download_file_contents_from_backup_storage') @mock.patch('olympia.users.models.backup_storage_enabled', lambda: True) @@ -534,9 +684,10 @@ def test_unban_and_restore_banned_content( download_file_contents_from_backup_storage_mock.side_effect = ( lambda remote_path: f'Fake content from {remote_path}'.encode('utf-8') ) + user_factory(pk=settings.TASK_USER_ID) fake_admin = user_factory(display_name='Fake Admin') core.set_user(fake_admin) # Needed for activity log - users = self.test_ban_and_disable_related_content_bulk() + users = self.test_ban_and_disable_related_content_bulk(hard_block_addons=True) user_sole = users['user_sole'] user_multi = users['user_multi'] assert BannedUserContent.objects.filter(user=user_sole).exists() @@ -553,6 +704,8 @@ def test_unban_and_restore_banned_content( assert user_sole.addons.count() == 2 for addon in user_sole.addons.all(): assert addon.status == amo.STATUS_APPROVED + for version in addon.versions.all(): + assert not version.is_blocked assert user_sole._ratings_all.all().count() == 2 # Includes replies assert user_sole.collections.count() == 1 assert not BannedUserContent.objects.filter(user=user_sole).exists()