diff --git a/src/olympia/files/tests/test_utils.py b/src/olympia/files/tests/test_utils.py index 6b1e7bfed047..f4af2e923f99 100644 --- a/src/olympia/files/tests/test_utils.py +++ b/src/olympia/files/tests/test_utils.py @@ -66,12 +66,48 @@ def create_webext_default_versions(cls): class TestManifestJSONExtractor(AppVersionsMixin, TestCase): + def test_valid_json(self): + assert self.parse({}) + def test_not_dict(self): with self.assertRaises(utils.InvalidManifest) as exc: utils.ManifestJSONExtractor('"foo@bar.com"').parse() assert isinstance(exc.exception, forms.ValidationError) assert exc.exception.message == 'Could not parse the manifest file.' + def test_parse_with_fields_that_should_be_dicts(self): + # parse() will get the value for these fields and therefore should + # raise the InvalidManifest exception if they are not dicts, so that + # the linter can display the proper error. + with self.assertRaises(utils.InvalidManifest) as exc: + self.parse({'browser_specific_settings': []}) + assert isinstance(exc.exception, forms.ValidationError) + assert exc.exception.message == 'Could not parse the manifest file.' + + with self.assertRaises(utils.InvalidManifest) as exc: + self.parse({'browser_specific_settings': {'gecko': 'lmao'}}) + assert isinstance(exc.exception, forms.ValidationError) + assert exc.exception.message == 'Could not parse the manifest file.' + + with self.assertRaises(utils.InvalidManifest) as exc: + self.parse({'developer': 'rotfl'}) + assert isinstance(exc.exception, forms.ValidationError) + assert exc.exception.message == 'Could not parse the manifest file.' + + def test_parse_data_collection_permissions_should_be_dict(self): + # As previous, this is automatically parsed for extensions and should + # raise InvalidManifest if not a dict. + with self.assertRaises(utils.InvalidManifest) as exc: + self.parse( + { + 'browser_specific_settings': { + 'gecko': {'data_collection_permissions': []} + } + } + ) + assert isinstance(exc.exception, forms.ValidationError) + assert exc.exception.message == 'Could not parse the manifest file.' + def test_parse_xpi_no_manifest(self): fake_zip = utils.make_xpi({'dummy': 'dummy'}) @@ -886,6 +922,10 @@ def parse(self, base_data): def test_type(self): assert self.parse({})['type'] == amo.ADDON_STATICTHEME + def test_parse_data_collection_permissions_should_be_dict(self): + # Overridden because themes don't support data collection permissions. + pass + def test_apps_use_default_versions_if_applications_is_omitted(self): """ Override this because static themes have a higher default version. diff --git a/src/olympia/files/utils.py b/src/olympia/files/utils.py index dfcc70ef02dd..919f2353c992 100644 --- a/src/olympia/files/utils.py +++ b/src/olympia/files/utils.py @@ -1,6 +1,7 @@ import contextlib import errno import fcntl +import functools import hashlib import io import json @@ -13,6 +14,7 @@ import tarfile import tempfile import zipfile +from collections.abc import Mapping from types import MappingProxyType from django import forms @@ -152,6 +154,17 @@ def get_simple_version(version_string): return VersionString(re.sub('[<=>]', '', version_string)) +def raise_invalid_manifest_if_not_mapping(f): + @functools.wraps(f) + def wrapper(self): + rval = f(self) + if not isinstance(rval, Mapping): + raise InvalidManifest(gettext('Could not parse the manifest file.')) + return rval + + return wrapper + + class ManifestJSONExtractor: """Extract add-on info from a manifest file.""" @@ -189,11 +202,16 @@ def __init__(self, manifest_data, *, certinfo=None): def get(self, key, default=None): return self.data.get(key, default) + @property + @raise_invalid_manifest_if_not_mapping + def developer(self): + return self.get('developer', EMPTY_FALLBACK_DICT) + @property def homepage(self): homepage_url = self.get('homepage_url') # `developer.url` in the manifest overrides `homepage_url`. - return self.get('developer', EMPTY_FALLBACK_DICT).get('url', homepage_url) + return self.developer.get('url', homepage_url) @property def is_experiment(self): @@ -206,20 +224,29 @@ def is_experiment(self): return any(bool(self.get(key)) for key in experiment_keys) @property + @raise_invalid_manifest_if_not_mapping + def browser_specific_settings(self): + return self.get( + 'browser_specific_settings', self.get('applications', EMPTY_FALLBACK_DICT) + ) + + @property + @raise_invalid_manifest_if_not_mapping def gecko(self): """Return the "applications|browser_specific_settings["gecko"]" part of the manifest.""" - parent_block = self.get( - 'browser_specific_settings', self.get('applications', EMPTY_FALLBACK_DICT) - ) - return parent_block.get('gecko', EMPTY_FALLBACK_DICT) + return self.browser_specific_settings.get('gecko', EMPTY_FALLBACK_DICT) @property + @raise_invalid_manifest_if_not_mapping def gecko_android(self): """Return `browser_specific_settings.gecko_android` if present.""" - return self.get('browser_specific_settings', EMPTY_FALLBACK_DICT).get( - 'gecko_android', EMPTY_FALLBACK_DICT - ) + return self.browser_specific_settings.get('gecko_android', EMPTY_FALLBACK_DICT) + + @property + @raise_invalid_manifest_if_not_mapping + def data_collection_permissions(self): + return self.gecko.get('data_collection_permissions', EMPTY_FALLBACK_DICT) @property def guid(self): @@ -441,12 +468,12 @@ def parse(self, minimal=False): 'permissions': self.get('permissions', []), 'host_permissions': self.get('host_permissions', []), 'content_scripts': self.get('content_scripts', []), - 'data_collection_permissions': self.gecko.get( - 'data_collection_permissions', {} - ).get('required', []), - 'optional_data_collection_permissions': self.gecko.get( - 'data_collection_permissions', {} - ).get('optional', []), + 'data_collection_permissions': ( + self.data_collection_permissions.get('required', []) + ), + 'optional_data_collection_permissions': ( + self.data_collection_permissions.get('optional', []) + ), } )