diff --git a/src/olympia/blocklist/cron.py b/src/olympia/blocklist/cron.py index 550d44e6d517..238249e40d2e 100644 --- a/src/olympia/blocklist/cron.py +++ b/src/olympia/blocklist/cron.py @@ -1,5 +1,4 @@ from datetime import datetime -from typing import List import waffle from django_statsd.clients import statsd @@ -8,6 +7,7 @@ from olympia.constants.blocklist import ( MLBF_BASE_ID_CONFIG_KEY, MLBF_TIME_CONFIG_KEY, + BlockListAction, ) from olympia.zadmin.models import get_config @@ -74,39 +74,36 @@ def _upload_mlbf_to_remote_settings(*, force_base=False): ) base_filters: dict[BlockType, MLBF | None] = {key: None for key in BlockType} - base_filters_to_update: List[BlockType] = [] - create_stash = False + + upload_filters = False + upload_stash = False # Determine which base filters need to be re uploaded # and whether a new stash needs to be created. for block_type in BlockType: - # This prevents us from updating a stash or filter based on new soft blocks - # until we are ready to enable soft blocking. - if block_type == BlockType.SOFT_BLOCKED and not waffle.switch_is_active( - 'enable-soft-blocking' - ): - log.info( - 'Skipping soft-blocks because enable-soft-blocking switch is inactive' - ) - continue - base_filter = MLBF.load_from_storage(get_base_generation_time(block_type)) base_filters[block_type] = base_filter - # Add this block type to the list of filters to be re-uploaded. + # For now we upload both filters together when either exceeds + # the change threshold. Additionally we brute force clear all stashes + # when uploading filters. This is the easiest way to ensure that stashes + # are always newer than any existing filters, a requirement of the way + # FX is reading the blocklist stash and filter sets. + # We may attempt handling block types separately in the future as a + # performance optimization https://github.com/mozilla/addons/issues/15217. if ( force_base or base_filter is None or mlbf.should_upload_filter(block_type, base_filter) ): - base_filters_to_update.append(block_type) + upload_filters = True + upload_stash = False # Only update the stash if we should AND if we aren't already - # re-uploading the filter for this block type. + # re-uploading the filters. elif mlbf.should_upload_stash(block_type, previous_filter or base_filter): - create_stash = True + upload_stash = True - skip_update = len(base_filters_to_update) == 0 and not create_stash - if skip_update: + if not upload_filters and not upload_stash: log.info('No new/modified/deleted Blocks in database; skipping MLBF generation') # Delete the locally generated MLBF directory and files as they are not needed. mlbf.delete() @@ -125,7 +122,19 @@ def _upload_mlbf_to_remote_settings(*, force_base=False): len(mlbf.data.not_blocked_items), ) - if create_stash: + if upload_filters: + for block_type in BlockType: + mlbf.generate_and_write_filter(block_type) + + # Upload both filters and clear the stash to keep + # all of the records in sync with the expectations of FX. + actions = [ + BlockListAction.UPLOAD_BLOCKED_FILTER, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER, + BlockListAction.CLEAR_STASH, + ] + + elif upload_stash: # We generate unified stashes, which means they can contain data # for both soft and hard blocks. We need the base filters of each # block type to determine what goes in a stash. @@ -134,15 +143,12 @@ def _upload_mlbf_to_remote_settings(*, force_base=False): blocked_base_filter=base_filters[BlockType.BLOCKED], soft_blocked_base_filter=base_filters[BlockType.SOFT_BLOCKED], ) + actions = [ + BlockListAction.UPLOAD_STASH, + ] - for block_type in base_filters_to_update: - mlbf.generate_and_write_filter(block_type) - - upload_filter.delay( - mlbf.created_at, - filter_list=[key.name for key in base_filters_to_update], - create_stash=create_stash, - ) + # Serialize the actions to strings because celery doesn't support enums. + upload_filter.delay(mlbf.created_at, actions=[action.name for action in actions]) def process_blocklistsubmissions(): diff --git a/src/olympia/blocklist/mlbf.py b/src/olympia/blocklist/mlbf.py index e8968592b048..87b2c625734f 100644 --- a/src/olympia/blocklist/mlbf.py +++ b/src/olympia/blocklist/mlbf.py @@ -7,7 +7,6 @@ from django.utils.functional import cached_property -import waffle from filtercascade import FilterCascade from filtercascade.fileformats import HashAlgorithm @@ -352,14 +351,13 @@ def generate_and_write_stash( stash_json[STASH_KEYS[BlockType.BLOCKED]] = blocked_added stash_json[UNBLOCKED_STASH_KEY] = blocked_removed - if waffle.switch_is_active('enable-soft-blocking'): - soft_blocked_added, soft_blocked_removed, _ = diffs[BlockType.SOFT_BLOCKED] - added_items.update(soft_blocked_added) - if not self.should_upload_filter( - BlockType.SOFT_BLOCKED, soft_blocked_base_filter - ): - stash_json[STASH_KEYS[BlockType.SOFT_BLOCKED]] = soft_blocked_added - stash_json[UNBLOCKED_STASH_KEY].extend(soft_blocked_removed) + soft_blocked_added, soft_blocked_removed, _ = diffs[BlockType.SOFT_BLOCKED] + added_items.update(soft_blocked_added) + if not self.should_upload_filter( + BlockType.SOFT_BLOCKED, soft_blocked_base_filter + ): + stash_json[STASH_KEYS[BlockType.SOFT_BLOCKED]] = soft_blocked_added + stash_json[UNBLOCKED_STASH_KEY].extend(soft_blocked_removed) # Remove any items that were added to a block type. stash_json[UNBLOCKED_STASH_KEY] = [ diff --git a/src/olympia/blocklist/tasks.py b/src/olympia/blocklist/tasks.py index f7fd14b743a9..47a77e115eae 100644 --- a/src/olympia/blocklist/tasks.py +++ b/src/olympia/blocklist/tasks.py @@ -10,7 +10,6 @@ from django.db import transaction from django.utils.encoding import force_str -import waffle from django_statsd.clients import statsd import olympia.core.logger @@ -21,6 +20,7 @@ MLBF_BASE_ID_CONFIG_KEY, MLBF_TIME_CONFIG_KEY, REMOTE_SETTINGS_COLLECTION_MLBF, + BlockListAction, ) from olympia.lib.remote_settings import RemoteSettings from olympia.zadmin.models import get_config, set_config @@ -105,8 +105,12 @@ def monitor_remote_settings(): @task -def upload_filter(generation_time, filter_list=None, create_stash=False): - filters_to_upload: List[BlockType] = [] +def upload_filter(generation_time: str, actions: List[str] = None): + # Deserialize the actions from the string list + # We have to do this because celery does not support enum arguments + actions = [BlockListAction[action] for action in actions] + + filters_to_upload = [] base_filter_ids = dict() bucket = settings.REMOTE_SETTINGS_WRITER_BUCKET server = RemoteSettings( @@ -118,26 +122,21 @@ def upload_filter(generation_time, filter_list=None, create_stash=False): old_records = server.records() attachment_types_to_delete = [] - for block_type in BlockType: - # Skip soft blocked filters if the switch is not active. - if block_type == BlockType.SOFT_BLOCKED and not waffle.switch_is_active( - 'enable-soft-blocking' - ): - continue + if BlockListAction.UPLOAD_BLOCKED_FILTER in actions: + filters_to_upload.append(BlockType.BLOCKED) - # Only upload filters that are in the filter_list arg. - # We cannot send enum values to tasks so we serialize - # them in the filter_list arg as the name of the enum. - if filter_list and block_type.name in filter_list: - filters_to_upload.append(block_type) + if BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER in actions: + filters_to_upload.append(BlockType.SOFT_BLOCKED) + # Get the last updated timestamp for each filter type + # regardless of whether we are uploading it or not. + # This will help us identify stale records that should be cleaned up. + for block_type in BlockType: base_filter_id = get_config( MLBF_BASE_ID_CONFIG_KEY(block_type, compat=True), json_value=True, ) - # If there is an existing base filter id, we need to keep track of it - # so we can potentially delete stashes older than this timestamp. if base_filter_id is not None: base_filter_ids[block_type] = base_filter_id @@ -157,15 +156,14 @@ def upload_filter(generation_time, filter_list=None, create_stash=False): attachment_types_to_delete.append(attachment_type) statsd.incr('blocklist.tasks.upload_filter.upload_mlbf.base') - # Update the base filter id for this block type to the generation time - # so we can delete stashes older than this new filter. + # If we are re-uploading a filter, we should overwrite the timestamp + # to ensure we delete records older than the new filter and not the old one. base_filter_ids[block_type] = generation_time # It is possible to upload a stash and a filter in the same task. - if create_stash: + if BlockListAction.UPLOAD_STASH in actions: with mlbf.storage.open(mlbf.stash_path, 'r') as stash_file: stash_data = json.load(stash_file) - # If we have a stash, write that stash_upload_data = { 'key_format': MLBF.KEY_FORMAT, 'stash_time': generation_time, @@ -174,26 +172,29 @@ def upload_filter(generation_time, filter_list=None, create_stash=False): server.publish_record(stash_upload_data) statsd.incr('blocklist.tasks.upload_filter.upload_stash') - # Get the oldest base filter id so we can delete only stashes - # that are definitely not needed anymore. + # Get the oldest base filter id so we can identify stale records + # that should be removed from remote settings or file storage. oldest_base_filter_id = min(base_filter_ids.values()) if base_filter_ids else None for record in old_records: - # Delete attachment records that match the - # attachment types of filters we just uploaded. - # This ensures we only have one filter attachment - # per block_type. if 'attachment' in record: + # Delete attachment records that match the + # attachment types of filters we just uploaded. + # This ensures we only have one filter attachment + # per block_type. attachment_type = record['attachment_type'] if attachment_type in attachment_types_to_delete: server.delete_record(record['id']) - # Delete stash records that are older than the oldest - # pre-existing filter attachment records. These records - # cannot apply to any existing filter since we uploaded. - elif 'stash' in record and oldest_base_filter_id is not None: - record_time = record['stash_time'] - if record_time < oldest_base_filter_id: + elif 'stash' in record: + # Delete stash records if that is one of the actions to perform. + # Currently we have a brute force approach to clearing stashes + # because we always upload both filters together in order to prevent + # stale stashes from being used by FX instances. Eventually, we may + # want support independently uploading filters and stashes which would + # require a more fine grained approach to clearing or even re-writing + # stashes based on which filters are being re-uploaded. + if BlockListAction.CLEAR_STASH in actions: server.delete_record(record['id']) # Commit the changes to remote settings for review + signing. diff --git a/src/olympia/blocklist/tests/test_cron.py b/src/olympia/blocklist/tests/test_cron.py index bee596f7dd14..a3558e0cfee3 100644 --- a/src/olympia/blocklist/tests/test_cron.py +++ b/src/olympia/blocklist/tests/test_cron.py @@ -1,7 +1,7 @@ import json import uuid from datetime import datetime, timedelta -from typing import List, Union +from typing import List from unittest import mock from django.conf import settings @@ -33,6 +33,7 @@ BASE_REPLACE_THRESHOLD_KEY, MLBF_BASE_ID_CONFIG_KEY, MLBF_TIME_CONFIG_KEY, + BlockListAction, ) from olympia.zadmin.models import set_config @@ -42,7 +43,6 @@ @freeze_time('2020-01-01 12:34:56') @override_switch('blocklist_mlbf_submit', active=True) -@override_switch('enable-soft-blocking', active=False) class TestUploadToRemoteSettings(TestCase): def setUp(self): self.user = user_factory() @@ -90,106 +90,49 @@ def _block_version( block=block, version=version, block_type=block_type ) - def _test_skip_update_unless_force_base(self, enable_soft_blocking=False): - """ - skip update unless force_base is true - """ - # We skip update at this point because there is no reason to update. - upload_mlbf_to_remote_settings(force_base=False) - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called - - filter_list = [BlockType.BLOCKED.name] - - if enable_soft_blocking: - filter_list.append(BlockType.SOFT_BLOCKED.name) - - with override_switch('enable-soft-blocking', active=enable_soft_blocking): - upload_mlbf_to_remote_settings(force_base=True) - - assert ( - mock.call( - self.current_time, - filter_list=filter_list, - create_stash=False, - ) - ) in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list - - # Check that both filters were created on the second attempt - mlbf = MLBF.load_from_storage(self.current_time) - self.assertTrue( - mlbf.storage.exists(mlbf.filter_path(BlockType.BLOCKED)), - ) - self.assertEqual( - mlbf.storage.exists(mlbf.filter_path(BlockType.SOFT_BLOCKED)), - enable_soft_blocking, - ) - assert not mlbf.storage.exists(mlbf.stash_path) - - def test_skip_update_unless_forced_soft_blocking_disabled(self): - self._test_skip_update_unless_force_base(enable_soft_blocking=False) - - def test_skip_update_unless_forced_soft_blocking_enabled(self): - self._test_skip_update_unless_force_base(enable_soft_blocking=True) - - def _test_skip_update_unless_no_base_mlbf( - self, block_type: BlockType, filter_list: Union[List[BlockType], None] = None - ): + def test_skip_update_unless_no_base_mlbf(self): """ skip update unless there is no base mlbf for the given block type """ # We skip update at this point because there is a base filter. upload_mlbf_to_remote_settings(force_base=False) - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_count == 0 + # Delete the base filter so we can test again with different conditions self.mocks['olympia.blocklist.cron.get_base_generation_time'].side_effect = ( - lambda _block_type: None if _block_type == block_type else self.base_time - ) - upload_mlbf_to_remote_settings(force_base=False) - - if filter_list is None: - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called - else: - assert ( - mock.call( - self.current_time, - filter_list=filter_list, - create_stash=False, - ) - ) in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list - - def test_skip_update_unless_no_base_mlbf_for_blocked(self): - self._test_skip_update_unless_no_base_mlbf( - BlockType.BLOCKED, filter_list=[BlockType.BLOCKED.name] + lambda _block_type: None ) - @override_switch('enable-soft-blocking', active=True) - def test_skip_update_unless_no_base_mlbf_for_soft_blocked_with_switch_enabled(self): - self._test_skip_update_unless_no_base_mlbf( - BlockType.SOFT_BLOCKED, filter_list=[BlockType.SOFT_BLOCKED.name] - ) + upload_mlbf_to_remote_settings(force_base=False) - def test_skip_update_unless_no_base_mlbf_for_soft_blocked_with_switch_disabled( - self, - ): - self._test_skip_update_unless_no_base_mlbf( - BlockType.SOFT_BLOCKED, filter_list=None + # Now that the base filter is missing, we expect to upload a new filter + assert ( + mock.call( + self.current_time, + actions=[ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ], + ) + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list ) def test_missing_last_filter_uses_base_filter(self): """ When there is a base filter and no last filter, - fallback to using the base filter + fallback to using the base filter for comparing stashes. """ block_version = self._block_version(is_signed=True) # Re-create the last filter so we ensure # the block is already processed comparing to previous MLBF.generate_from_db(self.last_time) + # Ensure the block was created before the last filter assert datetime_to_ts(block_version.modified) < self.last_time - # We skip the update at this point because the new last filter already - # accounted for the new block. + # We skip the update at this point because the block is already accounted for upload_mlbf_to_remote_settings(force_base=False) - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_count == 0 # We don't skip because the base filter is now used to compare against # and it has not accounted for the new block. @@ -197,63 +140,37 @@ def test_missing_last_filter_uses_base_filter(self): 'olympia.blocklist.cron.get_last_generation_time' ].return_value = None upload_mlbf_to_remote_settings(force_base=False) + # We expect to upload a stash because the block is not accounted for in the base + # filter and the last filter is missing. assert ( mock.call( self.current_time, - filter_list=[], - create_stash=True, + actions=[ + BlockListAction.UPLOAD_STASH.name, + ], ) - ) in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list + ) - @override_switch('enable-soft-blocking', active=True) def test_skip_update_if_unsigned_blocks_added(self): """ skip update if there are only unsigned new blocks """ - self._block_version(block_type=BlockType.BLOCKED, is_signed=False) - self._block_version(block_type=BlockType.SOFT_BLOCKED, is_signed=False) + for block_type in BlockType: + self._block_version(block_type=block_type, is_signed=False) upload_mlbf_to_remote_settings(force_base=False) - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_count == 0 - def _test_skip_update_unless_new_blocks( - self, block_type: BlockType, enable_soft_blocking=False, expect_update=False - ): + def test_skip_update_when_there_are_no_new_blocks(self): """ - skip update unless there are new blocks + If there are no new blocks, and we are not forcing a base update, + then it is a no-op. """ - with override_switch('enable-soft-blocking', active=enable_soft_blocking): - upload_mlbf_to_remote_settings(force_base=False) - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called - - # Now there is a new blocked version - self._block_version(block_type=block_type, is_signed=True) - upload_mlbf_to_remote_settings(force_base=False) - - self.assertEqual( - expect_update, - self.mocks['olympia.blocklist.cron.upload_filter.delay'].called, - ) - - def test_skip_update_unless_new_blocks_for_blocked(self): - self._test_skip_update_unless_new_blocks( - block_type=BlockType.BLOCKED, - expect_update=True, - ) - - def test_skip_update_unless_new_blocks_for_soft_blocked_with_switch_disabled(self): - self._test_skip_update_unless_new_blocks( - block_type=BlockType.SOFT_BLOCKED, - enable_soft_blocking=False, - expect_update=False, - ) + upload_mlbf_to_remote_settings(force_base=False) - def test_skip_update_unless_new_blocks_for_soft_blocked_with_switch_enabled(self): - self._test_skip_update_unless_new_blocks( - block_type=BlockType.SOFT_BLOCKED, - enable_soft_blocking=True, - expect_update=True, - ) + assert MLBF.load_from_storage(self.current_time) is None + assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_count == 0 def test_send_statsd_counts(self): """ @@ -290,329 +207,175 @@ def test_send_statsd_counts(self): @override_switch('blocklist_mlbf_submit', active=False) def test_skip_upload_if_switch_is_disabled(self): upload_mlbf_to_remote_settings() - assert not self.mocks['olympia.blocklist.cron.statsd.incr'].called + assert self.mocks['olympia.blocklist.cron.statsd.incr'].call_count == 0 upload_mlbf_to_remote_settings(bypass_switch=True) - assert self.mocks['olympia.blocklist.cron.statsd.incr'].called + assert self.mocks['olympia.blocklist.cron.statsd.incr'].call_count == 1 - def _test_upload_stash_unless_force_base( - self, - block_types: List[BlockType], - expect_stash: bool, - filter_list: Union[List[BlockType], None], - enable_soft_blocking: bool, - ): + def test_upload_stash_unless_force_base(self): """ Upload a stash unless force_base is true. When there is a new block, We expect to upload a stash, unless the force_base is true, in which case we upload a new filter. """ - for block_type in block_types: - self._block_version(block_type=block_type) - - with override_switch('enable-soft-blocking', active=enable_soft_blocking): - upload_mlbf_to_remote_settings(force_base=False) - - self.assertEqual( - expect_stash, - mock.call( - self.current_time, - filter_list=[], - create_stash=True, - ) - in self.mocks[ - 'olympia.blocklist.cron.upload_filter.delay' - ].call_args_list, - ) - - mlbf = MLBF.load_from_storage(self.current_time) + # First we run without any new blocks + upload_mlbf_to_remote_settings(force_base=False) - if expect_stash: - assert mlbf.storage.exists(mlbf.stash_path) + assert MLBF.load_from_storage(self.current_time) is None + # Are not uploading anything because there are no changes + assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_count == 0 - for block_type in BlockType: - assert not mlbf.storage.exists(mlbf.filter_path(block_type)) - else: - assert mlbf is None + # Create a new blocked version for each block type + for block_type in BlockType: + self._block_version(block_type=block_type) - upload_mlbf_to_remote_settings(force_base=True) - next_mlbf = MLBF.load_from_storage(self.current_time) - expected_block_types = [] + # Now we run again with the new blocks + upload_mlbf_to_remote_settings(force_base=False) - for block_type in filter_list: - assert next_mlbf.storage.exists(next_mlbf.filter_path(block_type)) - expected_block_types.append(block_type.name) + mlbf = MLBF.load_from_storage(self.current_time) - assert ( - mock.call( - self.current_time, - filter_list=expected_block_types, - create_stash=False, - ) - in self.mocks[ - 'olympia.blocklist.cron.upload_filter.delay' - ].call_args_list + # Expect a stash because there is a new hard block and force_base is False + assert ( + mock.call( + self.current_time, + actions=[ + BlockListAction.UPLOAD_STASH.name, + ], ) - - def test_upload_stash_unless_force_base_for_blocked_with_switch_disabled(self): - """ - When force base is false, it uploads a stash because there is a new hard blocked - version. When force base is true, it uploads the blocked filter for the same - reason. - """ - self._test_upload_stash_unless_force_base( - block_types=[BlockType.BLOCKED], - expect_stash=True, - filter_list=[BlockType.BLOCKED], - enable_soft_blocking=False, - ) - - def test_upload_stash_unless_force_base_for_blocked_with_switch_enabled(self): - """ - When force base is false, it uploads a stash because soft block is enabled - and there is a new hard blocked version. When force base is true, it uploads - both blocked and soft blocked filters for the previous reason and because - soft blocking is enabled. - """ - self._test_upload_stash_unless_force_base( - block_types=[BlockType.BLOCKED], - expect_stash=True, - filter_list=[BlockType.BLOCKED, BlockType.SOFT_BLOCKED], - enable_soft_blocking=True, - ) - - def test_upload_stash_unless_force_base_for_soft_blocked_with_switch_disabled(self): - """ - When force base is false, it does not upload a stash even when there is a new - soft blocked version, because soft blocking is disabled. - When force base is true, it uploads only the blocked filter - for the same reason. - """ - self._test_upload_stash_unless_force_base( - block_types=[BlockType.SOFT_BLOCKED], - expect_stash=False, - filter_list=[BlockType.BLOCKED], - enable_soft_blocking=False, - ) - - def test_upload_stash_unless_force_base_for_soft_blocked_with_switch_enabled(self): - """ - When force base is false, it uploads a stash because soft block is enabled - and there is a new soft blocked version. When force base is true, it uploads - both blocked and soft blocked filters. - """ - self._test_upload_stash_unless_force_base( - block_types=[BlockType.SOFT_BLOCKED], - expect_stash=True, - filter_list=[BlockType.BLOCKED, BlockType.SOFT_BLOCKED], - enable_soft_blocking=True, - ) - - def test_upload_stash_unless_force_base_for_both_blocked_with_switch_disabled(self): - """ - When force base is false, it uploads a stash even though soft blocking disabled - because there is a hard blocked version. When force base is true, - it uploads only the blocked filter for the same reason. - """ - self._test_upload_stash_unless_force_base( - block_types=[BlockType.BLOCKED, BlockType.SOFT_BLOCKED], - expect_stash=True, - filter_list=[BlockType.BLOCKED], - enable_soft_blocking=False, + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list ) + assert mlbf.storage.exists(mlbf.stash_path) - def test_upload_stash_unless_force_base_for_both_blocked_with_switch_enabled(self): - """ - When force base is false, it uploads a stash because there are new hard and soft - blocked versions. When force base is true, - it uploads both blocked + soft blocked filters for the same reason. - """ - self._test_upload_stash_unless_force_base( - block_types=[BlockType.BLOCKED, BlockType.SOFT_BLOCKED], - expect_stash=True, - filter_list=[BlockType.BLOCKED, BlockType.SOFT_BLOCKED], - enable_soft_blocking=True, - ) + for block_type in BlockType: + assert not mlbf.storage.exists(mlbf.filter_path(block_type)) - def test_dont_upload_stash_unless_force_base_for_both_blocked_with_switch_enabled( - self, - ): - """ - When force base is false, it does not upload a stash because - there are no new versions.When force base is true, - it uploads both blocked and soft blocked filters because - soft blocking is enabled. - """ - self._test_upload_stash_unless_force_base( - block_types=[], - expect_stash=False, - filter_list=[BlockType.BLOCKED, BlockType.SOFT_BLOCKED], - enable_soft_blocking=True, - ) + # Delete the MLBF so we can test again with different conditions + mlbf.delete() - def test_upload_stash_unless_missing_base_filter(self): - """ - Upload a stash unless there is no base filter. - """ - self._block_version(is_signed=True) - upload_mlbf_to_remote_settings() - assert self.mocks[ - 'olympia.blocklist.cron.upload_filter.delay' - ].call_args_list == [ - mock.call( - self.current_time, - filter_list=[], - create_stash=True, - ) - ] - mlbf = MLBF.load_from_storage(self.current_time) - assert not mlbf.storage.exists(mlbf.filter_path(BlockType.BLOCKED)) - assert not mlbf.storage.exists(mlbf.filter_path(BlockType.SOFT_BLOCKED)) - assert mlbf.storage.exists(mlbf.stash_path) + upload_mlbf_to_remote_settings(force_base=True) - self.mocks['olympia.blocklist.cron.get_base_generation_time'].side_effect = ( - lambda _block_type: None - ) - upload_mlbf_to_remote_settings() + # Now expect a new filter because force_base is True assert ( mock.call( self.current_time, - filter_list=[BlockType.BLOCKED.name], - create_stash=False, + actions=[ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ], ) in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list ) - assert mlbf.storage.exists(mlbf.filter_path(BlockType.BLOCKED)) - - with override_switch('enable-soft-blocking', active=True): - upload_mlbf_to_remote_settings() - assert mlbf.storage.exists(mlbf.filter_path(BlockType.SOFT_BLOCKED)) - assert ( - mock.call( - self.current_time, - filter_list=[BlockType.BLOCKED.name, BlockType.SOFT_BLOCKED.name], - create_stash=False, - ) - ) in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list @mock.patch('olympia.blocklist.mlbf.get_base_replace_threshold') - @override_switch('enable-soft-blocking', active=True) def test_upload_stash_unless_enough_changes(self, mock_get_base_replace_threshold): """ - When there are new blocks, upload either a stash or a filter depending on - whether we have surpased the threshold amount. + When there are new blocks, upload a stash unless + we have surpased the threshold amount. """ mock_get_base_replace_threshold.return_value = 1 - block_type = BlockType.BLOCKED - for _block_type in BlockType: - self._block_version(is_signed=True, block_type=_block_type) + + for block_type in BlockType: + self._block_version(is_signed=True, block_type=block_type) upload_mlbf_to_remote_settings() - assert self.mocks[ - 'olympia.blocklist.cron.upload_filter.delay' - ].call_args_list == [ + assert ( mock.call( self.current_time, - filter_list=[], - create_stash=True, + actions=[ + BlockListAction.UPLOAD_STASH.name, + ], ) - ] + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list + ) mlbf = MLBF.load_from_storage(self.current_time) - assert not mlbf.storage.exists(mlbf.filter_path(block_type)) + for block_type in BlockType: + assert not mlbf.storage.exists(mlbf.filter_path(block_type)) assert mlbf.storage.exists(mlbf.stash_path) # delete the mlbf so we can test again with different conditions mlbf.delete() - self._block_version(is_signed=True, block_type=block_type) + # Create another blocked version to exceed the threshold + self._block_version(is_signed=True, block_type=BlockType.BLOCKED) upload_mlbf_to_remote_settings() assert ( mock.call( self.current_time, - filter_list=[block_type.name], - create_stash=True, + actions=[ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ], ) in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list ) new_mlbf = MLBF.load_from_storage(self.current_time) - assert new_mlbf.storage.exists(new_mlbf.filter_path(block_type)) - assert new_mlbf.storage.exists(new_mlbf.stash_path) - with new_mlbf.storage.open(new_mlbf.stash_path, 'r') as f: - data = json.load(f) - # We expect an empty list for hard blocks because we are - # uploading a new hard filter. - assert len(data['blocked']) == 0 - # We can still see there are 2 hard blocked versions in the new filter. - assert len(new_mlbf.data.blocked_items) == 2 - assert len(data['softblocked']) == 1 + + for block_type in BlockType: + assert new_mlbf.storage.exists(new_mlbf.filter_path(block_type)) + + assert not new_mlbf.storage.exists(new_mlbf.stash_path) def _test_upload_stash_and_filter( self, - enable_soft_blocking: bool, - expected_stash: dict | None, - expected_filters: List[BlockType], + expected_actions: List[BlockListAction], ): set_config(BASE_REPLACE_THRESHOLD_KEY, 1) - with override_switch('enable-soft-blocking', active=enable_soft_blocking): - upload_mlbf_to_remote_settings() + upload_mlbf_to_remote_settings() # Generation time is set to current time so we can load the MLBF. mlbf = MLBF.load_from_storage(self.current_time) - if expected_stash is None: - assert not mlbf.storage.exists( - mlbf.stash_path - ), 'Expected no stash but one exists' - else: + if BlockListAction.UPLOAD_BLOCKED_FILTER.name in expected_actions: assert mlbf.storage.exists( - mlbf.stash_path - ), f'Expected stash {expected_stash} but none exists' - with mlbf.storage.open(mlbf.stash_path, 'r') as f: - data = json.load(f) - for key, expected_count in expected_stash.items(): - assert ( - len(data[key]) == expected_count - ), f'Expected {expected_count} {key} but got {len(data[key])}' - - for expected_filter in expected_filters: + mlbf.filter_path(BlockType.BLOCKED) + ), 'Expected filter {BlockType.BLOCKED} but none exists' + + if BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name in expected_actions: + assert mlbf.storage.exists( + mlbf.filter_path(BlockType.SOFT_BLOCKED) + ), 'Expected filter {BlockType.SOFT_BLOCKED} but none exists' + + if BlockListAction.UPLOAD_STASH.name in expected_actions: assert mlbf.storage.exists( - mlbf.filter_path(expected_filter) - ), f'Expected filter {expected_filter} but none exists' + mlbf.stash_path + ), 'Expected stash but none exists' + + assert ( + mock.call( + self.current_time, + actions=expected_actions, + ) + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list + ) def test_upload_blocked_stash_and_softblock_filter(self): + """ + When there are enough blocked versions for a stash, and enough soft blocked + version for a filter, then if soft blocking is disabled, we expect a stash + and if it is enabled we expect both filters to be uploaded. + """ # Enough blocks for a stash self._block_version(is_signed=True, block_type=BlockType.BLOCKED) # Enough soft blocks for a filter self._block_version(is_signed=True, block_type=BlockType.SOFT_BLOCKED) self._block_version(is_signed=True, block_type=BlockType.SOFT_BLOCKED) - # Expected stash does not change - expected_stash = { - 'blocked': 1, - # Expect no soft blocks because there are enough for a filter. - 'softblocked': 0, - # There are no unblocked versions - 'unblocked': 0, - } - self._test_upload_stash_and_filter( - # Even though there are enough soft blocks, soft blocking is disabled - enable_soft_blocking=False, - expected_stash=expected_stash, - # Expect no filter as soft blocking is disabled - expected_filters=[], - ) - - # Now try again with soft blocking enabled - self._test_upload_stash_and_filter( - # Soft blocking is enabled, so we expect the same stash and a new filter - enable_soft_blocking=True, - expected_stash=expected_stash, - # Expect a soft blocked filter - expected_filters=[BlockType.SOFT_BLOCKED], + [ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ] ) def test_upload_soft_blocked_stash_and_blocked_filter(self): + """ + When there are enough soft blocks for a stash, and enough blocks for a filter, + Then we always upload a new filter, + regardless if softblocking is enabled or not. + """ # Enough soft blocks for a stash self._block_version(is_signed=True, block_type=BlockType.SOFT_BLOCKED) # Enough blocked versions for a filter @@ -620,32 +383,18 @@ def test_upload_soft_blocked_stash_and_blocked_filter(self): self._block_version(is_signed=True, block_type=BlockType.BLOCKED) self._test_upload_stash_and_filter( - enable_soft_blocking=False, - # Expect no stash because there are enough blocked versions for a filter - # and soft blocking is disabled - expected_stash=None, - # Expect a blocked filter - expected_filters=[BlockType.BLOCKED], - ) - - # Now try again with soft blocking enabled - self._test_upload_stash_and_filter( - enable_soft_blocking=True, - # Expect a stash and a blocked filter - expected_stash={ - # Expect no blocked versions because there are enough for a filter - 'blocked': 0, - # Expect a soft blocked version when there is one - # and soft blocking is enabled - 'softblocked': 1, - # There are no unblocked versions - 'unblocked': 0, - }, - # Expect a blocked filter - expected_filters=[BlockType.BLOCKED], + [ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ] ) def test_upload_blocked_and_softblocked_filter(self): + """ + When there are enough blocked and soft blocked versions for a filter, + then we expect to upload both filters. + """ # Enough blocked versions for a filter self._block_version(is_signed=True, block_type=BlockType.BLOCKED) self._block_version(is_signed=True, block_type=BlockType.BLOCKED) @@ -654,50 +403,27 @@ def test_upload_blocked_and_softblocked_filter(self): self._block_version(is_signed=True, block_type=BlockType.SOFT_BLOCKED) self._test_upload_stash_and_filter( - enable_soft_blocking=False, - expected_stash=None, - expected_filters=[BlockType.BLOCKED], - ) - - # Now try again with soft blocking enabled - self._test_upload_stash_and_filter( - enable_soft_blocking=True, - expected_stash=None, - expected_filters=[BlockType.BLOCKED, BlockType.SOFT_BLOCKED], + [ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ] ) def test_upload_blocked_and_softblocked_stash(self): + """ + When there are enough blocked and soft blocked versions for a stash, + then we expect to upload a new stash. + """ # Enough blocked versions for a stash self._block_version(is_signed=True, block_type=BlockType.BLOCKED) # Enough soft blocks for a stash self._block_version(is_signed=True, block_type=BlockType.SOFT_BLOCKED) self._test_upload_stash_and_filter( - enable_soft_blocking=False, - expected_stash={ - # Expect a blocked version - 'blocked': 1, - # Expect no soft blocks because soft blocking is disabled - 'softblocked': 0, - # There are no unblocked versions - 'unblocked': 0, - }, - expected_filters=[], - ) - - # Now try again with soft blocking enabled - self._test_upload_stash_and_filter( - enable_soft_blocking=True, - expected_stash={ - # We still have the blocked version - 'blocked': 1, - # Expect a soft blocked version because there is one - # and soft blocking is enabled - 'softblocked': 1, - # There are no unblocked versions - 'unblocked': 0, - }, - expected_filters=[], + [ + BlockListAction.UPLOAD_STASH.name, + ] ) def test_remove_storage_if_no_update(self): @@ -705,58 +431,76 @@ def test_remove_storage_if_no_update(self): If there is no update, remove the storage used by the current mlbf. """ upload_mlbf_to_remote_settings(force_base=False) - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_count == 0 assert MLBF.load_from_storage(self.current_time) is None def test_creates_base_filter_if_base_generation_time_invalid(self): """ When a base_generation_time is provided, but no filter exists for it, - raise no filter found. + then we create a new filter. """ self.mocks['olympia.blocklist.cron.get_base_generation_time'].return_value = 1 upload_mlbf_to_remote_settings(force_base=True) - assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + assert ( + mock.call( + self.current_time, + actions=[ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ], + ) + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list + ) def test_compares_against_base_filter_if_missing_previous_filter(self): """ When no previous filter is found, compare blocks against the base filter of that block type. """ - # Hard block version is accounted for in the base filter - self._block_version(block_type=BlockType.BLOCKED) - MLBF.generate_from_db(self.base_time) - # Soft block version is not accounted for in the base filter - # but accounted for in the last filter - self._block_version(block_type=BlockType.SOFT_BLOCKED) + blocked_version = self._block_version(block_type=BlockType.BLOCKED) MLBF.generate_from_db(self.last_time) + # The block is created at the same time as the base filter + # but before the last filter + assert datetime_to_ts(blocked_version.modified) == self.base_time + assert datetime_to_ts(blocked_version.modified) < self.last_time + upload_mlbf_to_remote_settings(force_base=False) - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_count == 0 # Delete the last filter, now the base filter will be used to compare MLBF.load_from_storage(self.last_time).delete() upload_mlbf_to_remote_settings(force_base=False) - # We expect to not upload anything as soft blocking is disabled - # and only the soft blocked version is missing from the base filter - assert not self.mocks['olympia.blocklist.cron.upload_filter.delay'].called - - # now with softblocking enabled we can account for the soft blocked version - with override_switch('enable-soft-blocking', active=True): - upload_mlbf_to_remote_settings(force_base=False) - assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + # We expect to not upload anything as the hard block has already been + # accounted for in the base filter. + assert ( + mock.call( + self.current_time, + actions=[BlockListAction.UPLOAD_STASH.name], + ) + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list + ) - @override_switch('enable-soft-blocking', active=True) def _test_dont_skip_update_if_all_blocked_or_not_blocked( self, block_type: BlockType ): """ - If all versions are either blocked or not blocked, skip the update. + If all versions are either blocked or not blocked, don't skip the update. """ for _ in range(0, 10): self._block_version(block_type=block_type) upload_mlbf_to_remote_settings() - assert self.mocks['olympia.blocklist.cron.upload_filter.delay'].called + assert ( + mock.call( + self.current_time, + actions=[ + BlockListAction.UPLOAD_STASH.name, + ], + ) + in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list + ) def test_dont_skip_update_if_all_blocked_or_not_blocked_for_soft_blocked(self): self._test_dont_skip_update_if_all_blocked_or_not_blocked( @@ -798,8 +542,9 @@ def test_invalid_cache_results_in_diff(self): assert ( mock.call( next_time, - filter_list=[], - create_stash=True, + actions=[ + BlockListAction.UPLOAD_STASH.name, + ], ) in self.mocks['olympia.blocklist.cron.upload_filter.delay'].call_args_list ) @@ -810,18 +555,17 @@ def test_pass_correct_arguments_to_upload_filter(self): 'olympia.blocklist.cron.upload_filter.delay', wraps=upload_filter.delay ) as spy_delay: upload_mlbf_to_remote_settings(force_base=True) - spy_delay.assert_called_with( - self.current_time, - filter_list=[BlockType.BLOCKED.name], - create_stash=False, - ) - with override_switch('enable-soft-blocking', active=True): - upload_mlbf_to_remote_settings(force_base=True) - spy_delay.assert_called_with( + assert ( + mock.call( self.current_time, - filter_list=[BlockType.BLOCKED.name, BlockType.SOFT_BLOCKED.name], - create_stash=False, + actions=[ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + BlockListAction.CLEAR_STASH.name, + ], ) + in spy_delay.call_args_list + ) class TestTimeMethods(TestCase): diff --git a/src/olympia/blocklist/tests/test_mlbf.py b/src/olympia/blocklist/tests/test_mlbf.py index 4c2cab0f6f56..188bc69f3449 100644 --- a/src/olympia/blocklist/tests/test_mlbf.py +++ b/src/olympia/blocklist/tests/test_mlbf.py @@ -2,8 +2,6 @@ from functools import cached_property from unittest import mock -from waffle.testutils import override_switch - from olympia import amo from olympia.addons.models import GUID_REUSE_FORMAT from olympia.amo.tests import ( @@ -474,15 +472,9 @@ def test_block_added(self): } assert next_mlbf.generate_and_write_stash(previous_mlbf=base_mlbf) == { 'blocked': [new_block_hash], - 'softblocked': [], + 'softblocked': [new_soft_block_hash], 'unblocked': [], } - with override_switch('enable-soft-blocking', active=True): - assert next_mlbf.generate_and_write_stash(previous_mlbf=base_mlbf) == { - 'blocked': [new_block_hash], - 'softblocked': [new_soft_block_hash], - 'unblocked': [], - } def test_block_removed(self): addon, block = self._blocked_addon() @@ -570,15 +562,9 @@ def test_block_hard_to_soft(self): } assert next_mlbf.generate_and_write_stash(previous_mlbf=base_mlbf) == { 'blocked': [], - 'softblocked': [], - 'unblocked': [block_hash], + 'softblocked': [block_hash], + 'unblocked': [], } - with override_switch('enable-soft-blocking', active=True): - assert next_mlbf.generate_and_write_stash(previous_mlbf=base_mlbf) == { - 'blocked': [], - 'softblocked': [block_hash], - 'unblocked': [], - } def test_block_soft_to_hard(self): addon, block = self._blocked_addon() @@ -634,17 +620,10 @@ def test_hard_to_soft_multiple(self, mock_get_base_replace_threshold): assert next_mlbf.generate_and_write_stash(previous_mlbf=base_mlbf) == { 'blocked': [], - 'softblocked': [], - 'unblocked': block_hashes, + 'softblocked': block_hashes, + 'unblocked': [], } - with override_switch('enable-soft-blocking', active=True): - assert next_mlbf.generate_and_write_stash(previous_mlbf=base_mlbf) == { - 'blocked': [], - 'softblocked': block_hashes, - 'unblocked': [], - } - @mock.patch('olympia.blocklist.mlbf.get_base_replace_threshold') def test_stash_is_empty_if_uploading_new_filter( self, mock_get_base_replace_threshold @@ -679,21 +658,12 @@ def test_stash_is_empty_if_uploading_new_filter( # No new filter yet assert not next_mlbf.should_upload_filter(BlockType.BLOCKED, mlbf) - # Only the hard blocked version is included in the stash assert next_mlbf.generate_and_write_stash(mlbf) == { 'blocked': [hard_stash], - 'softblocked': [], + 'softblocked': [soft_stash], 'unblocked': [], } - # With soft blocking enabled, both versions are included in the stash - with override_switch('enable-soft-blocking', active=True): - assert next_mlbf.generate_and_write_stash(mlbf) == { - 'blocked': [hard_stash], - 'softblocked': [soft_stash], - 'unblocked': [], - } - # Harden the soft blocked version soft_block.update(block_type=BlockType.BLOCKED) final_mlbf = MLBF.generate_from_db('test_three') @@ -809,21 +779,12 @@ def test_diff_all_possible_changes(self): BlockType.SOFT_BLOCKED: ([three_hash, four_hash], [], 2), } - # The first time we generate the stash, we expect 3 to 6 to be in the stash - # as they have some kind of block applied. assert first_mlbf.generate_and_write_stash() == { 'blocked': [five_hash, six_hash], - 'softblocked': [], + 'softblocked': [three_hash, four_hash], 'unblocked': [], } - with override_switch('enable-soft-blocking', active=True): - assert first_mlbf.generate_and_write_stash() == { - 'blocked': [five_hash, six_hash], - 'softblocked': [three_hash, four_hash], - 'unblocked': [], - } - # Update the existing blocks, and create new ones for # the versions "one" and "two". @@ -865,21 +826,12 @@ def test_diff_all_possible_changes(self): assert second_mlbf.generate_and_write_stash(previous_mlbf=first_mlbf) == { 'blocked': [three_hash, two_hash], - # Soft blocked is empty because the waffle switch is off - 'softblocked': [], - # 4 is omitted because it's soft blocked and the waffle switch is off - 'unblocked': [five_hash, six_hash], + 'softblocked': [five_hash, one_hash], + # 3 and 5 are omitted because they transitioned + # from one block type to another + 'unblocked': [six_hash, four_hash], } - with override_switch('enable-soft-blocking', active=True): - assert second_mlbf.generate_and_write_stash(previous_mlbf=first_mlbf) == { - 'blocked': [three_hash, two_hash], - 'softblocked': [five_hash, one_hash], - # 3 and 5 are omitted because they transitioned - # from one block type to another - 'unblocked': [six_hash, four_hash], - } - def test_generate_stash_returns_expected_stash(self): addon, block = self._blocked_addon() block_versions = [ @@ -901,34 +853,23 @@ def test_generate_stash_returns_expected_stash(self): 'unblocked': [], } - with override_switch('enable-soft-blocking', active=True): - assert mlbf.generate_and_write_stash() == { - 'blocked': MLBF.hash_filter_inputs(expected_blocked), - 'softblocked': [], - 'unblocked': [], - } + assert mlbf.generate_and_write_stash() == { + 'blocked': MLBF.hash_filter_inputs(expected_blocked), + 'softblocked': [], + 'unblocked': [], + } # Remove the last block version block_versions[-1].delete() expected_unblocked = expected_blocked[-1:] next_mlbf = MLBF.generate_from_db('test_two') - next_mlbf.generate_and_write_stash(previous_mlbf=mlbf) - - with next_mlbf.storage.open(next_mlbf.stash_path, 'r') as f: - assert json.load(f) == { - 'blocked': [], - # Soft blocked is empty because the waffle switch is off - 'softblocked': [], - 'unblocked': MLBF.hash_filter_inputs(expected_unblocked), - } - with override_switch('enable-soft-blocking', active=True): - assert next_mlbf.generate_and_write_stash(previous_mlbf=mlbf) == { - 'blocked': [], - 'softblocked': [], - 'unblocked': MLBF.hash_filter_inputs(expected_unblocked), - } + assert next_mlbf.generate_and_write_stash(previous_mlbf=mlbf) == { + 'blocked': [], + 'softblocked': [], + 'unblocked': MLBF.hash_filter_inputs(expected_unblocked), + } @mock.patch('olympia.blocklist.mlbf.get_base_replace_threshold') def test_generate_empty_stash_when_all_items_in_filter( @@ -960,49 +901,27 @@ def test_generate_empty_stash_when_all_items_in_filter( mlbf = MLBF.generate_from_db('test') - # We should create a soft blocked filter and a stash assert mlbf.should_upload_filter(BlockType.SOFT_BLOCKED, base_mlbf) + # We have a softened block so we should upload stash, even though + # it will be empty since the block will be handled by the filter assert mlbf.should_upload_stash(BlockType.BLOCKED, base_mlbf) - # If soft blocking is disabled, then softening a block is treated - # as an unblock. + # If soft blocking is enabled, then we will expect a new soft block filter + # and no softblock stash since the blocks will be handled by the filter + # similarly we do not include the blocked version in the unblocked stash + # because it is now soft blocked. + # We actually would like this to result in no stash being created + # Bug: https://github.com/mozilla/addons/issues/15202 assert mlbf.generate_and_write_stash( previous_mlbf=base_mlbf, blocked_base_filter=base_mlbf, - soft_blocked_base_filter=None, + soft_blocked_base_filter=base_mlbf, ) == { 'blocked': [], 'softblocked': [], - 'unblocked': [hard_block_hash], + 'unblocked': [], } - # Recreate the mlbf and re-run with softblocking enabled - mlbf.delete() - - with override_switch('enable-soft-blocking', active=True): - mlbf.generate_from_db('test') - - assert mlbf.should_upload_filter(BlockType.SOFT_BLOCKED, base_mlbf) - # We have a softened block so we should upload stash, even though - # it will be empty since the block will be handled by the filter - assert mlbf.should_upload_stash(BlockType.BLOCKED, base_mlbf) - - # If soft blocking is enabled, then we will expect a new soft block filter - # and no softblock stash since the blocks will be handled by the filter - # similarly we do not include the blocked version in the unblocked stash - # because it is now soft blocked. - # We actually would like this to result in no stash being created - # Bug: https://github.com/mozilla/addons/issues/15202 - assert mlbf.generate_and_write_stash( - previous_mlbf=base_mlbf, - blocked_base_filter=base_mlbf, - soft_blocked_base_filter=base_mlbf, - ) == { - 'blocked': [], - 'softblocked': [], - 'unblocked': [], - } - def test_generate_filter_returns_expected_data(self): addon, block = self._blocked_addon() not_blocked = self._version(addon) diff --git a/src/olympia/blocklist/tests/test_tasks.py b/src/olympia/blocklist/tests/test_tasks.py index 2c9100858a76..e0fe6c4c6b54 100644 --- a/src/olympia/blocklist/tests/test_tasks.py +++ b/src/olympia/blocklist/tests/test_tasks.py @@ -9,7 +9,6 @@ from django.test.testcases import TransactionTestCase import pytest -from waffle.testutils import override_switch from olympia.amo.tests import ( addon_factory, @@ -18,7 +17,11 @@ version_factory, ) from olympia.blocklist.mlbf import MLBF -from olympia.constants.blocklist import MLBF_BASE_ID_CONFIG_KEY, MLBF_TIME_CONFIG_KEY +from olympia.constants.blocklist import ( + MLBF_BASE_ID_CONFIG_KEY, + MLBF_TIME_CONFIG_KEY, + BlockListAction, +) from olympia.zadmin.models import get_config, set_config from ..models import BlocklistSubmission, BlockType, BlockVersion @@ -172,23 +175,17 @@ def _block_version( block=block, version=version, block_type=block_type ) - def test_server_setup(self): - pass - - def test_statsd_increments(self): - pass - - def test_invalid_block_type_raises(self): + def test_invalid_action_raises(self): with self.assertRaises(ValueError): BLOCKLIST_RECORD_MLBF_BASE('foo') MLBF.generate_from_db(self.generation_time) - upload_filter( - self.generation_time, - filter_list=['foo'], - ) - assert not self.mocks['delete_record'].called - assert not self.mocks['publish_record'].called + + with self.assertRaises(KeyError): + upload_filter( + self.generation_time, + actions=['foo'], + ) def _test_upload_base_filter(self, *block_types: BlockType): self._block_version(is_signed=True) @@ -196,9 +193,14 @@ def _test_upload_base_filter(self, *block_types: BlockType): for block_type in block_types: mlbf.generate_and_write_filter(block_type) + actions_map = { + BlockType.BLOCKED: BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockType.SOFT_BLOCKED: BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name, + } + upload_filter( self.generation_time, - filter_list=[block_type.name for block_type in block_types], + actions=[actions_map[block_type] for block_type in block_types], ) assert not self.mocks['delete_record'].called @@ -249,39 +251,54 @@ def _test_upload_base_filter(self, *block_types: BlockType): def test_upload_blocked_filter(self): self._test_upload_base_filter(BlockType.BLOCKED) - @override_switch('enable-soft-blocking', active=True) def test_upload_soft_blocked_filter(self): self._test_upload_base_filter(BlockType.SOFT_BLOCKED) - @override_switch('enable-soft-blocking', active=True) def test_upload_soft_and_blocked_filter(self): self._test_upload_base_filter(BlockType.BLOCKED, BlockType.SOFT_BLOCKED) - def test_skip_cleanup_when_no_filters_or_config_keys(self): + def test_cleanup_old_files_called_with_oldest_config_key(self): + """ + We clean up stale records, older than the oldest config key, + even if no actions are provided. This ensures that we do not + let stale data persist in the remote settings collection. + """ MLBF.generate_from_db(self.generation_time) self.mocks['records'].return_value = [ self._attachment(0, 'bloomfilter-base', self.generation_time), self._stash(1, self.generation_time - 1), ] - upload_filter(self.generation_time, filter_list=[]) + upload_filter(self.generation_time, actions=[]) assert ( get_config(MLBF_BASE_ID_CONFIG_KEY(BlockType.BLOCKED, compat=True)) is None ) - assert not self.mocks['delete_record'].called + # If there is no base filter id, then we pass none and let the task + # figure out what to do with that information. + assert ( + mock.call(base_filter_id=None) + in self.mocks['cleanup_old_files.delay'].call_args_list + ) set_config( MLBF_BASE_ID_CONFIG_KEY(BlockType.BLOCKED, compat=True), self.generation_time, json_value=True, ) - upload_filter(self.generation_time, filter_list=[]) + upload_filter(self.generation_time, actions=[]) - # We should delete the record older than the config key - assert self.mocks['delete_record'].call_args_list == [mock.call(1)] + # We should now pass the oldest config key to the cleanup task + assert ( + mock.call(base_filter_id=self.generation_time) + in self.mocks['cleanup_old_files.delay'].call_args_list + ) - def test_cleanup_records_with_matching_attachment(self): + def test_delete_matching_attachment_when_uploading_filters(self): + """ + When we upload a new filter, given the filter type is enabled, we should + delete any existing attachment with the same name. + """ mlbf = MLBF.generate_from_db(self.generation_time) mlbf.generate_and_write_filter(BlockType.BLOCKED) mlbf.generate_and_write_filter(BlockType.SOFT_BLOCKED) @@ -291,158 +308,54 @@ def test_cleanup_records_with_matching_attachment(self): self._attachment(1, 'softblocks-bloomfilter-base', self.generation_time), ] - upload_filter(self.generation_time, filter_list=[BlockType.BLOCKED.name]) + upload_filter( + self.generation_time, actions=[BlockListAction.UPLOAD_BLOCKED_FILTER.name] + ) - # Delete the matching block filter record + # The previously uploaded blocked filter record "0" should be deleted assert self.mocks['delete_record'].call_args_list == [mock.call(0)] - # Don't delete the soft blocked filter record because the switch is disabled upload_filter( self.generation_time, - filter_list=[BlockType.SOFT_BLOCKED.name], + actions=[BlockListAction.UPLOAD_SOFT_BLOCKED_FILTER.name], ) - assert mock.call(1) not in self.mocks['delete_record'].call_args_list + assert mock.call(1) in self.mocks['delete_record'].call_args_list - with override_switch('enable-soft-blocking', active=True): - upload_filter( - self.generation_time, - filter_list=[BlockType.SOFT_BLOCKED.name], - ) - assert mock.call(1) in self.mocks['delete_record'].call_args_list - - def test_cleanup_records_older_than_oldest_base_filter_id(self): + def test_clear_stash_deletes_all_stash_records(self): """ - Delete records older than the oldest active base filter id - """ - t0 = self.generation_time - 3 - t1 = self.generation_time - 2 - t2 = self.generation_time - 1 - t3 = self.generation_time - - mlbf = MLBF.generate_from_db(t3) - mlbf.generate_and_write_filter(BlockType.BLOCKED) - - # Return existing records for all times before t3 - self.mocks['records'].return_value = [ - self._stash(0, t0), - self._stash(1, t1), - self._stash(2, t2), - ] - - # Delete records older than the oldest config key - # which is t2 - set_config( - MLBF_BASE_ID_CONFIG_KEY(BlockType.BLOCKED, compat=True), - t2, - json_value=True, - ) - upload_filter(self.generation_time, filter_list=[]) - assert self.mocks['delete_record'].call_args_list == [ - mock.call(0), - mock.call(1), - ] - self.mocks['delete_record'].reset_mock() - - # Delete all records older than the new active filter - # which is t3 - upload_filter(t3, filter_list=[BlockType.BLOCKED.name]) - assert self.mocks['delete_record'].call_args_list == [ - mock.call(0), - mock.call(1), - mock.call(2), - ] - self.mocks['delete_record'].reset_mock() - - # Create an older soft block base filter ID, - # but ignore it because the switch is disabled - set_config( - MLBF_BASE_ID_CONFIG_KEY(BlockType.SOFT_BLOCKED), - t1, - json_value=True, - ) - upload_filter(self.generation_time, filter_list=[]) - assert self.mocks['delete_record'].call_args_list == [ - mock.call(0), - mock.call(1), - ] - self.mocks['delete_record'].reset_mock() - - # If the switch is enabled, consider the soft blocked filter - # and only delete records older than the oldest active base id - # which is now t1 - with override_switch('enable-soft-blocking', active=True): - upload_filter(self.generation_time, filter_list=[]) - assert self.mocks['delete_record'].call_args_list == [ - mock.call(0), - ] - - def test_cleanup_old_stash_records_when_uploading_new_filter(self): - """ - When we upload a new filter, we should delete existing stash records - older than the new filter. + When CLEAR_STASH action is provided, we should delete all stash records. """ t0 = self.generation_time - 2 t1 = self.generation_time - 1 t2 = self.generation_time - mlbf = MLBF.generate_from_db(t2) mlbf.generate_and_write_filter(BlockType.BLOCKED) + mlbf.generate_and_write_stash() - # Remote settings returns the old filter - # and a stash created before the new filter - # this stash should also be deleted self.mocks['records'].return_value = [ self._attachment(0, 'bloomfilter-base', t0), self._stash(1, t1), ] - set_config( - MLBF_BASE_ID_CONFIG_KEY(BlockType.BLOCKED, compat=True), - t0, - json_value=True, - ) - - upload_filter(self.generation_time, filter_list=[BlockType.BLOCKED.name]) + upload_filter(t2, actions=[BlockListAction.CLEAR_STASH.name]) + # We should only delete the stash record, not the attachment assert self.mocks['delete_record'].call_args_list == [ - mock.call(0), # old attachment is deleted - mock.call(1), # old stash is deleted + mock.call(1), ] - def test_ignore_soft_blocked_if_switch_is_disabled(self): + def test_upload_stash_sends_correct_data(self): """ - If softblocking is disabled, we should ignore the soft blocked - timestamp when determining the oldest base filter id. + When there is an UPLOAD_STASH action, we should upload the correct data, + commit the data, and update the config key indicating a change was made. """ - t0 = self.generation_time - 1 - t1 = self.generation_time - - mlbf = MLBF.generate_from_db(t1) - mlbf.generate_and_write_filter(BlockType.SOFT_BLOCKED) - - self.mocks['records'].return_value = [ - self._stash(1, t0), - ] - - upload_filter(self.generation_time, filter_list=[BlockType.SOFT_BLOCKED.name]) - - assert not self.mocks['delete_record'].called - - with override_switch('enable-soft-blocking', active=True): - upload_filter( - self.generation_time, filter_list=[BlockType.SOFT_BLOCKED.name] - ) - assert self.mocks['delete_record'].called - - def test_create_stashed_filter(self): old_mlbf = MLBF.generate_from_db(self.generation_time - 1) blocked_version = self._block_version(is_signed=True) mlbf = MLBF.generate_from_db(self.generation_time) mlbf.generate_and_write_stash(old_mlbf) - upload_filter(self.generation_time, create_stash=True) + upload_filter(self.generation_time, actions=[BlockListAction.UPLOAD_STASH.name]) - assert not self.mocks['delete_record'].called with mlbf.storage.open(mlbf.stash_path, 'rb') as stash_file: actual_stash = self.mocks['publish_record'].call_args_list[0][0][0] stash_data = json.load(stash_file) @@ -453,10 +366,8 @@ def test_create_stashed_filter(self): 'stash': stash_data, } - assert actual_stash['stash']['blocked'] == list( - MLBF.hash_filter_inputs( - [(blocked_version.block.guid, blocked_version.version.version)] - ) + assert actual_stash['stash']['blocked'] == MLBF.hash_filter_inputs( + [(blocked_version.block.guid, blocked_version.version.version)] ) assert ( @@ -470,36 +381,64 @@ def test_create_stashed_filter(self): in self.mocks['set_config'].call_args_list ) - def test_raises_when_no_filter_exists(self): + def test_upload_filter_raises_when_no_filter_exists(self): with self.assertRaises(FileNotFoundError): - upload_filter(self.generation_time, filter_list=[BlockType.BLOCKED.name]) + upload_filter( + self.generation_time, + actions=[BlockListAction.UPLOAD_BLOCKED_FILTER.name], + ) - def test_raises_when_no_stash_exists(self): + def test_upload_stash_raises_when_no_stash_exists(self): with self.assertRaises(FileNotFoundError): - upload_filter(self.generation_time, create_stash=True) + upload_filter( + self.generation_time, actions=[BlockListAction.UPLOAD_STASH.name] + ) + + def test_no_passed_actions_is_no_op(self): + """ + If no actions are provided we should do nothing. + """ + MLBF.generate_from_db(self.generation_time) + upload_filter(self.generation_time, actions=[]) - def test_default_is_no_op(self): - MLBF.generate_from_db(self.generation_time).generate_and_write_filter( - BlockType.BLOCKED + # We should query existing records, commit the session, + # set the config to indicate the task was run, clean up old files, + # and increment the statsd counter. + assert self.mocks['records'].call_count == 1 + assert self.mocks['complete_session'].call_count == 1 + assert ( + mock.call(MLBF_TIME_CONFIG_KEY, self.generation_time, json_value=True) + in self.mocks['set_config'].call_args_list ) - upload_filter(self.generation_time) - assert not self.mocks['delete_record'].called - assert not self.mocks['publish_record'].called + assert ( + mock.call('blocklist.tasks.upload_filter.reset_collection') + in self.mocks['statsd.incr'].call_args_list + ) + # If there is no existing config key, we pass None to cleanup_old_files + # also triggering a no-op on that task. + assert ( + mock.call(base_filter_id=None) + in self.mocks['cleanup_old_files.delay'].call_args_list + ) + + # We should not modify any records in RemoteSettings + assert self.mocks['publish_attachment'].call_count == 0 + assert self.mocks['publish_record'].call_count == 0 + assert self.mocks['delete_record'].call_count == 0 - def test_complete_session_is_called_after_upload_stash(self): + def test_complete_session_is_called_after_all_actions(self): """ server.complete_session is called to "commit" all of the changes made to remote settings. For this reason, it is important that we call it - after uploading the new stash. + after executing any actions. """ mlbf = MLBF.generate_from_db(self.generation_time) - mlbf.generate_and_write_stash() mlbf.generate_and_write_filter(BlockType.BLOCKED) + mlbf.generate_and_write_stash() mocks = [ 'delete_record', 'complete_session', - 'records', 'publish_record', 'publish_attachment', ] @@ -519,20 +458,25 @@ def test_complete_session_is_called_after_upload_stash(self): with mock.patch('olympia.blocklist.tasks.RemoteSettings', return_value=manager): upload_filter( self.generation_time, - filter_list=[BlockType.BLOCKED.name], - create_stash=True, + actions=[ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + BlockListAction.UPLOAD_STASH.name, + BlockListAction.CLEAR_STASH.name, + ], ) manager.assert_has_calls( [ - # First we get the existing records + # First we download the old records mock.call.records(), - # Then we publish the new attachment (and stash) + # Then we publish the new attachment mock.call.publish_attachment(mock.ANY, mock.ANY), + # Then we publish the new stash mock.call.publish_record(mock.ANY), - # Then we delete the old attachment and stash - mock.call.delete_record(attachment_record['id']), - mock.call.delete_record(stash_record['id']), + # Then we delete the old attachment + mock.call.delete_record(0), + # Then we delete the old stash + mock.call.delete_record(1), # Then we commit the changes mock.call.complete_session(), ] @@ -545,7 +489,6 @@ def test_config_is_updated_only_after_complete_session(self): should not be updated. """ mlbf = MLBF.generate_from_db(self.generation_time) - mlbf.generate_and_write_stash() mlbf.generate_and_write_filter(BlockType.BLOCKED) self.mocks['complete_session'].side_effect = Exception('Something went wrong') @@ -553,8 +496,9 @@ def test_config_is_updated_only_after_complete_session(self): with self.assertRaises(Exception): # noqa: B017 upload_filter( self.generation_time, - filter_list=[BlockType.BLOCKED.name], - create_stash=True, + actions=[ + BlockListAction.UPLOAD_BLOCKED_FILTER.name, + ], ) assert self.mocks['complete_session'].called diff --git a/src/olympia/constants/blocklist.py b/src/olympia/constants/blocklist.py index c164a15bd6f4..99d2b3daa178 100644 --- a/src/olympia/constants/blocklist.py +++ b/src/olympia/constants/blocklist.py @@ -1,4 +1,6 @@ # How many guids should there be in the stashes before we make a new base. +from enum import Enum + from olympia.blocklist.models import BlockType @@ -20,3 +22,14 @@ def MLBF_BASE_ID_CONFIG_KEY(block_type: BlockType, compat: bool = False): REMOTE_SETTINGS_COLLECTION_MLBF = 'addons-bloomfilters' + + +class BlockListAction(Enum): + # Re-upload the Blocked filter + UPLOAD_BLOCKED_FILTER = 'upload_blocked_filter' + # Re-upload the Soft Blocked filter + UPLOAD_SOFT_BLOCKED_FILTER = 'upload_soft_blocked_filter' + # Upload a new stash entry + UPLOAD_STASH = 'upload_stash' + # Clear the Stash (clear all stashes) + CLEAR_STASH = 'clear_stash'