diff --git a/constants/base.py b/constants/base.py index f8aeb088..e9c80385 100644 --- a/constants/base.py +++ b/constants/base.py @@ -58,6 +58,7 @@ EXTENSION_TYPE_PLURAL = { EXTENSION_TYPE_CHOICES.THEME: _('Themes'), } EXTENSION_SLUGS_PATH = '|'.join(EXTENSION_TYPE_SLUGS.values()) +EXTENSION_SLUG_TYPES = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()} ALLOWED_EXTENSION_MIMETYPES = ('application/zip', ) # FIXME: this controls the initial widget rendered server-side, and server-side validation diff --git a/extensions/tests/files/addon-without-dir.zip b/extensions/tests/files/addon-without-dir.zip new file mode 100644 index 00000000..0c488cef Binary files /dev/null and b/extensions/tests/files/addon-without-dir.zip differ diff --git a/extensions/tests/files/invalid-addon-dir-no-init.zip b/extensions/tests/files/invalid-addon-dir-no-init.zip new file mode 100644 index 00000000..a4424c78 Binary files /dev/null and b/extensions/tests/files/invalid-addon-dir-no-init.zip differ diff --git a/extensions/tests/files/invalid-addon-no-init.zip b/extensions/tests/files/invalid-addon-no-init.zip new file mode 100644 index 00000000..740e82fc Binary files /dev/null and b/extensions/tests/files/invalid-addon-no-init.zip differ diff --git a/extensions/tests/files/not_a.zip b/extensions/tests/files/invalid-archive.zip similarity index 100% rename from extensions/tests/files/not_a.zip rename to extensions/tests/files/invalid-archive.zip diff --git a/extensions/tests/files/invalid-manifest-path.zip b/extensions/tests/files/invalid-manifest-path.zip new file mode 100644 index 00000000..afc4e498 Binary files /dev/null and b/extensions/tests/files/invalid-manifest-path.zip differ diff --git a/extensions/tests/files/invalid-manifest-toml.zip b/extensions/tests/files/invalid-manifest-toml.zip new file mode 100644 index 00000000..942089d4 Binary files /dev/null and b/extensions/tests/files/invalid-manifest-toml.zip differ diff --git a/extensions/tests/files/invalid-no-manifest.zip b/extensions/tests/files/invalid-no-manifest.zip new file mode 100644 index 00000000..d946f997 Binary files /dev/null and b/extensions/tests/files/invalid-no-manifest.zip differ diff --git a/extensions/tests/files/invalid-theme-multiple-xmls.zip b/extensions/tests/files/invalid-theme-multiple-xmls.zip new file mode 100644 index 00000000..392e518f Binary files /dev/null and b/extensions/tests/files/invalid-theme-multiple-xmls.zip differ diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 197ba99c..9b50158d 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -74,6 +74,14 @@ class CreateFileTest(TestCase): version = combined_meta_data.get("version", "0.1.0") extension_id = combined_meta_data.get("id", "foobar").strip() + type_slug = combined_meta_data['type'] + init_path = None + + if type_slug == 'add-on': + # Add the required __init__.py file + init_path = os.path.join(self.temp_directory, '__init__.py') + with open(init_path, 'w') as init_file: + init_file.write('') with open(manifest_path, "w") as manifest_file: toml.dump(combined_meta_data, manifest_file) @@ -81,6 +89,10 @@ class CreateFileTest(TestCase): with zipfile.ZipFile(output_path, "w") as my_zip: arcname = f"{extension_id}-{version}/{os.path.basename(manifest_path)}" my_zip.write(manifest_path, arcname=arcname) + if init_path: + # Write the __init__.py file too + arcname = f"{extension_id}-{version}/{os.path.basename(init_path)}" + my_zip.write(init_path, arcname=arcname) os.remove(manifest_path) return output_path @@ -259,7 +271,7 @@ class ValidateManifestTest(CreateFileTest): self.client.force_login(user) file_data = { - "id": "id-with-hyphens", + "id": "id-with-hyphens", } bad_file = self._create_file_from_data("theme.zip", file_data, self.user) @@ -271,7 +283,7 @@ class ValidateManifestTest(CreateFileTest): self.assertEqual(response.status_code, 200) error = response.context['form'].errors.get('source') self.assertEqual(len(error), 1) - self.assertIn('"<b>id-with-hyphens</b>"', error[0]) + self.assertIn('"id-with-hyphens"', error[0]) class ValidateManifestFields(TestCase): diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index 4813536f..607b8b2e 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -59,6 +59,27 @@ EXPECTED_EXTENSION_DATA = { 'version_str': '1.0.8', }, } +EXPECTED_VALIDATION_ERRORS = { + 'empty.txt': {'source': ['Only .zip files are accepted.']}, + 'empty.zip': {'source': ['The submitted file is empty.']}, + 'invalid-archive.zip': {'source': ['Only .zip files are accepted.']}, + 'invalid-manifest-path.zip': { + 'source': [ + 'The manifest file should be at the top level of the archive, or one level deep.', + ], + }, + 'invalid-addon-no-init.zip': { + 'source': ['An add-on should have an __init__.py file.'], + }, + 'invalid-addon-dir-no-init.zip': { + 'source': ['An add-on should have an __init__.py file.'], + }, + 'invalid-no-manifest.zip': { + 'source': ['The manifest file is missing.'], + }, + 'invalid-manifest-toml.zip': {'source': ['Could not parse the manifest file.']}, + 'invalid-theme-multiple-xmls.zip': {'source': ['A theme should have exactly one XML file.']}, +} class SubmitFileTest(TestCase): @@ -116,46 +137,28 @@ class SubmitFileTest(TestCase): {'agreed_with_terms': ['This field is required.']}, ) - def test_validation_errors_invalid_extension(self): + def test_validation_errors(self): self.assertEqual(Extension.objects.count(), 0) user = UserFactory() self.client.force_login(user) - with open(TEST_FILES_DIR / 'empty.txt', 'rb') as fp: - response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True}) + for test_archive, extected_errors in EXPECTED_VALIDATION_ERRORS.items(): + with self.subTest(test_archive=test_archive): + with open(TEST_FILES_DIR / test_archive, 'rb') as fp: + response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True}) - self.assertEqual(response.status_code, 200) - self.assertDictEqual( - response.context['form'].errors, - {'source': ['Only .zip files are accepted.']}, - ) + self.assertEqual(response.status_code, 200) + self.assertDictEqual(response.context['form'].errors, extected_errors) - def test_validation_errors_empty_file(self): + def test_addon_without_top_level_directory(self): self.assertEqual(Extension.objects.count(), 0) user = UserFactory() self.client.force_login(user) - with open(TEST_FILES_DIR / 'empty.zip', 'rb') as fp: + with open(TEST_FILES_DIR / 'addon-without-dir.zip', 'rb') as fp: response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True}) - self.assertEqual(response.status_code, 200) - self.assertDictEqual( - response.context['form'].errors, - {'source': ['The submitted file is empty.']}, - ) - - def test_validation_errors_not_actually_a_zip(self): - self.assertEqual(Extension.objects.count(), 0) - user = UserFactory() - self.client.force_login(user) - - with open(TEST_FILES_DIR / 'not_a.zip', 'rb') as fp: - response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True}) - - self.assertDictEqual( - response.context['form'].errors, - {'source': ['Only .zip files are accepted.']}, - ) + self.assertEqual(response.status_code, 302) def test_theme_file(self): self.assertEqual(File.objects.count(), 0) diff --git a/files/forms.py b/files/forms.py index 7de275b5..31320fdd 100644 --- a/files/forms.py +++ b/files/forms.py @@ -12,10 +12,7 @@ from .validators import ( FileMIMETypeValidator, ManifestValidator, ) -from constants.base import ( - EXTENSION_TYPE_SLUGS_SINGULAR, - ALLOWED_EXTENSION_MIMETYPES, -) +from constants.base import EXTENSION_SLUG_TYPES, ALLOWED_EXTENSION_MIMETYPES import files.models import files.utils as utils @@ -28,6 +25,20 @@ logger = logging.getLogger(__name__) class FileForm(forms.ModelForm): msg_only_zip_files = _('Only .zip files are accepted.') + # Mimicking how django.forms.fields.Field handles validation error messages. + # TODO: maybe this should be a custom SourceFileField with all these validators and messages + error_messages = { + 'invalid_manifest_path': _( + 'The manifest file should be at the top level of the archive, or one level deep.' + ), + # TODO: surface TOML parsing errors? + 'invalid_manifest_toml': _('Could not parse the manifest file.'), + 'invalid_missing_init': _('An add-on should have an __init__.py file.'), + 'missing_or_multiple_theme_xml': _('A theme should have exactly one XML file.'), + 'invalid_zip_archive': msg_only_zip_files, + 'missing_manifest_toml': _('The manifest file is missing.'), + } + class Meta: model = files.models.File fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user') @@ -38,7 +49,7 @@ class FileForm(forms.ModelForm): validators=[ FileMIMETypeValidator( allowed_mimetypes=ALLOWED_EXTENSION_MIMETYPES, - message=msg_only_zip_files, + message=error_messages['invalid_zip_archive'], ), ], widget=forms.ClearableFileInput( @@ -128,22 +139,19 @@ class FileForm(forms.ModelForm): errors = [] if not zipfile.is_zipfile(file_path): - errors.append('File is not .zip') + raise forms.ValidationError(self.error_messages['invalid_zip_archive']) - manifest = utils.read_manifest_from_zip(file_path) + manifest, error_codes = utils.read_manifest_from_zip(file_path) + for code in error_codes: + errors.append(forms.ValidationError(self.error_messages[code])) + if errors: + self.add_error('source', errors) - if manifest is None: - errors.append('A valid manifest file could not be found') - else: + if manifest: ManifestValidator(manifest) ExtensionIDManifestValidator(manifest, self.extension) - extension_types = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()} - if errors: - raise forms.ValidationError({'source': errors}, code='invalid') - - self.cleaned_data['metadata'] = manifest - # TODO: Error handling - self.cleaned_data['type'] = extension_types[manifest['type']] + self.cleaned_data['metadata'] = manifest + self.cleaned_data['type'] = EXTENSION_SLUG_TYPES[manifest['type']] return self.cleaned_data diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index 881b3a0f..1a581841 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -1,6 +1,6 @@ from django.test import TestCase -from files.utils import find_file_inside_zip_list +from files.utils import find_path_by_name, find_exact_path, filter_paths_by_ext class UtilsTest(TestCase): @@ -10,7 +10,7 @@ class UtilsTest(TestCase): name_list = [ "blender_manifest.toml", ] - manifest_file = find_file_inside_zip_list(self.manifest, name_list) + manifest_file = find_path_by_name(name_list, self.manifest) self.assertEqual(manifest_file, "blender_manifest.toml") def test_find_manifest_nested(self): @@ -23,21 +23,21 @@ class UtilsTest(TestCase): "foobar-1.0.3/manifest.toml", "foobar-1.0.3/manifest.json", ] - manifest_file = find_file_inside_zip_list(self.manifest, name_list) + manifest_file = find_path_by_name(name_list, self.manifest) self.assertEqual(manifest_file, "foobar-1.0.3/blender_manifest.toml") def test_find_manifest_no_zipped_folder(self): name_list = [ "foobar-1.0.3/blender_manifest.toml", ] - manifest_file = find_file_inside_zip_list(self.manifest, name_list) + manifest_file = find_path_by_name(name_list, self.manifest) self.assertEqual(manifest_file, "foobar-1.0.3/blender_manifest.toml") def test_find_manifest_no_manifest(self): name_list = [ "foobar-1.0.3/", ] - manifest_file = find_file_inside_zip_list(self.manifest, name_list) + manifest_file = find_path_by_name(name_list, self.manifest) self.assertEqual(manifest_file, None) def test_find_manifest_with_space(self): @@ -47,5 +47,54 @@ class UtilsTest(TestCase): "foobar-1.0.3/blender_manifest.toml.txt", "blender_manifest.toml/my_files.py", ] - manifest_file = find_file_inside_zip_list(self.manifest, name_list) + manifest_file = find_path_by_name(name_list, self.manifest) self.assertEqual(manifest_file, None) + + def test_find_exact_path_found(self): + name_list = [ + 'foobar-1.0.3/theme.xml', + 'foobar-1.0.3/theme1.xml', + 'foobar-1.0.3/theme2.txt', + 'foobar-1.0.3/__init__.py', + 'foobar-1.0.3/foobar/__init__.py', + 'foobar-1.0.3/foobar-1.0.3/__init__.py', + 'blender_manifest.toml', + ] + path = find_exact_path(name_list, 'foobar-1.0.3/__init__.py') + self.assertEqual(path, 'foobar-1.0.3/__init__.py') + + def test_find_exact_path_nothing_found(self): + name_list = [ + 'foobar-1.0.3/theme.xml', + 'foobar-1.0.3/theme1.xml', + 'foobar-1.0.3/theme2.txt', + 'foobar-1.0.3/foobar/__init__.py', + 'foobar-1.0.3/foobar-1.0.3/__init__.py', + 'blender_manifest.toml', + ] + path = find_exact_path(name_list, 'foobar-1.0.3/__init__.py') + self.assertIsNone(path) + + def test_filter_paths_by_ext_found(self): + name_list = [ + 'foobar-1.0.3/theme.xml', + 'foobar-1.0.3/theme1.xml', + 'foobar-1.0.3/theme2.txt', + 'foobar-1.0.3/__init__.py', + 'foobar-1.0.3/foobar-1.0.3/__init__.py', + 'blender_manifest.toml', + ] + paths = filter_paths_by_ext(name_list, '.xml') + self.assertEqual(list(paths), ['foobar-1.0.3/theme.xml', 'foobar-1.0.3/theme1.xml']) + + def test_filter_paths_by_ext_nothing_found(self): + name_list = [ + 'foobar-1.0.3/theme.xml', + 'foobar-1.0.3/theme1.md.xml', + 'foobar-1.0.3/theme2.txt', + 'foobar-1.0.3/__init__.py', + 'foobar-1.0.3/foobar-1.0.3/__init__.py', + 'blender_manifest.toml', + ] + paths = filter_paths_by_ext(name_list, '.md') + self.assertEqual(list(paths), []) diff --git a/files/utils.py b/files/utils.py index c02008e8..3c4f413f 100644 --- a/files/utils.py +++ b/files/utils.py @@ -5,6 +5,7 @@ import logging import mimetypes import os import toml +import typing import zipfile from lxml import etree @@ -48,41 +49,104 @@ def get_sha256_from_value(value: str): return hash_.hexdigest() -def find_file_inside_zip_list(file_to_read: str, name_list: list) -> str: - """Return the first occurance of file_to_read insize a zip name_list""" - for file_path in name_list: +def find_path_by_name(paths: typing.List[str], name: str) -> typing.Optional[str]: + """Return the first occurrence of file name in a given list of paths.""" + for file_path in paths: # Remove leading/trailing whitespace from file path file_path_stripped = file_path.strip() # Check if the basename of the stripped path is equal to the target file name - if os.path.basename(file_path_stripped) == file_to_read: + if os.path.basename(file_path_stripped) == name: return file_path_stripped return None +def find_exact_path(paths: typing.List[str], exact_path: str) -> typing.Optional[str]: + """Return a first path equal to a given one if it exists in a given list of paths.""" + matching_paths = (path for path in paths if path == exact_path) + return next(matching_paths, None) + + +def filter_paths_by_ext(paths: typing.List[str], ext: str) -> typing.Iterable[str]: + """Generate a list of paths having a given extension from a given list of paths.""" + for file_path in paths: + # Get file path's extension + _, file_path_ext = os.path.splitext(file_path) + # Check if this file's extension matches the extension we are looking for + if file_path_ext.lower() == ext.lower(): + yield file_path + + def read_manifest_from_zip(archive_path): - file_to_read = 'blender_manifest.toml' + """Read and validate extension's manifest file and contents of the archive. + + In any extension archive, a valid `blender_manifest.toml` file is expected + to be found at the top level of the archive, or inside a single nested directory. + Additionally, depending on the extension type defined in the manifest, + the archive is expected to have a particular file structure: + + * for themes, a single XML file is expected next to the manifest; + + * for add-ons, the following structure is expected: + + ``` + some-addon.zip + └─ an-optional-dir + ├─ blender_manifest.toml + ├─ __init__.py + └─ (...) + ``` + """ + manifest_name = 'blender_manifest.toml' + error_codes = [] try: with zipfile.ZipFile(archive_path) as myzip: - manifest_filepath = find_file_inside_zip_list(file_to_read, myzip.namelist()) + bad_file = myzip.testzip() + if bad_file is not None: + logger.error('Bad file in ZIP') + error_codes.append('invalid_zip_archive') + return None, error_codes + + file_list = myzip.namelist() + manifest_filepath = find_path_by_name(file_list, manifest_name) if manifest_filepath is None: - logger.info(f"File '{file_to_read}' not found in the archive.") - return None + logger.info(f"File '{manifest_name}' not found in the archive.") + error_codes.append('missing_manifest_toml') + return None, error_codes + + # Manifest file is expected to be no deeper than one directory down + if os.path.dirname(os.path.dirname(manifest_filepath)) != '': + error_codes.append('invalid_manifest_path') + return None, error_codes # Extract the file content with myzip.open(manifest_filepath) as file_content: - # TODO: handle TOML loading error toml_content = toml.loads(file_content.read().decode()) - return toml_content + + # If manifest was parsed successfully, do additional type-specific validation + type_slug = toml_content['type'] + if type_slug == 'theme': + theme_xmls = filter_paths_by_ext(file_list, '.xml') + if len(list(theme_xmls)) != 1: + error_codes.append('missing_or_multiple_theme_xml') + elif type_slug == 'add-on': + # __init__.py is expected to be next to the manifest + expected_init_path = os.path.join(os.path.dirname(manifest_filepath), '__init__.py') + init_filepath = find_exact_path(file_list, expected_init_path) + if not init_filepath: + error_codes.append('invalid_missing_init') + + return toml_content, error_codes except toml.decoder.TomlDecodeError as e: - # TODO: This error should be propagate to the user logger.error(f"Manifest Error: {e.msg}") + error_codes.append('invalid_manifest_toml') except Exception as e: logger.error(f"Error extracting from archive: {e}") + error_codes.append('invalid_zip_archive') - return None + return None, error_codes def guess_mimetype_from_ext(file_name: str) -> str: