Extra validation of the uploaded ZIP #73

Merged
Anna Sirota merged 13 commits from validation-single-theme-xml into main 2024-04-11 12:32:50 +02:00
12 changed files with 157 additions and 112 deletions
Showing only changes of commit 0d023f217f - Show all commits

Binary file not shown.

Binary file not shown.

Binary file not shown.

View File

@ -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

View File

@ -59,6 +59,26 @@ 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.',
'An add-on should have an __init__.py file.',
],
},
'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.'],
},
}
class SubmitFileTest(TestCase):
@ -116,74 +136,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:
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.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.']},
)
def test_validation_errors_more_than_one_theme_xml(self):
self.assertEqual(Extension.objects.count(), 0)
user = UserFactory()
self.client.force_login(user)
with open(TEST_FILES_DIR / 'theme-more-than-one-xml.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': ['A theme ZIP archive should only have one XML file']},
)
def test_validation_errors_no_init(self):
self.assertEqual(Extension.objects.count(), 0)
user = UserFactory()
self.client.force_login(user)
with open(TEST_FILES_DIR / 'addon-without-init.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': ['An add-on directory should contain an __init__.py file']},
)
self.assertEqual(response.status_code, 302)
def test_theme_file(self):
self.assertEqual(File.objects.count(), 0)
@ -232,31 +206,6 @@ class SubmitFileTest(TestCase):
self.assertEqual(response.status_code, 200, _get_all_form_errors(response))
self.assertEqual(File.objects.count(), 0)
def test_addon_single_py(self):
self.assertEqual(File.objects.count(), 0)
user = UserFactory()
self.client.force_login(user)
with open(TEST_FILES_DIR / 'addon-single-py-module.zip', 'rb') as fp:
response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True})
self.assertEqual(response.status_code, 302, _get_all_form_errors(response))
self.assertEqual(File.objects.count(), 1)
file = File.objects.first()
self.assertEqual(response['Location'], file.get_submit_url())
self.assertEqual(file.user, user)
self.assertEqual(file.original_name, 'addon-single-py-module.zip')
self.assertEqual(file.size_bytes, 558)
self.assertEqual(
file.original_hash,
'sha256:21408edc82b70a16fd332311faf297b86a29c96cea554b206a162394033c2451',
)
self.assertEqual(
file.hash, 'sha256:21408edc82b70a16fd332311faf297b86a29c96cea554b206a162394033c2451',
)
self.assertEqual(file.get_type_display(), 'Add-on')
self.assertEqual(file.metadata['name'], 'Some Add-on')
for file_name, data in EXPECTED_EXTENSION_DATA.items():

View File

@ -29,10 +29,15 @@ class FileForm(forms.ModelForm):
# 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_toml': _('A valid manifest file could not be found'),
'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.'),
'invalid_theme_multiple_xmls': _('A theme should only have one XML file.'),
'invalid_zip_archive': msg_only_zip_files,
'invalid_theme_multiple_xmls': _('A theme ZIP archive should only have one XML file'),
'invalid_missing_init': _('An add-on directory should contain an __init__.py file'),
'missing_manifest_toml': _('The manifest file is missing.'),
}
class Meta:
@ -142,16 +147,15 @@ class FileForm(forms.ModelForm):
errors.append(ValidationError(self.error_messages[code]))
if manifest is None:
errors.append(ValidationError(self.error_messages['invalid_manifest_toml']))
errors.append(ValidationError(self.error_messages['missing_manifest_toml']))
else:
ManifestValidator(manifest)
ExtensionIDManifestValidator(manifest, self.extension)
self.cleaned_data['metadata'] = manifest
self.cleaned_data['type'] = EXTENSION_SLUG_TYPES[manifest['type']]
if errors:
self.add_error('source', errors)
self.cleaned_data['metadata'] = manifest
# TODO: Error handling
self.cleaned_data['type'] = EXTENSION_SLUG_TYPES[manifest['type']]
return self.cleaned_data

View File

@ -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_full_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_full_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_full_path(name_list, 'foobar-1.0.3', '__init__.py')
self.assertEqual(path, 'foobar-1.0.3/__init__.py')
def test_find_full_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_full_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), [])

View File

@ -50,55 +50,86 @@ 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 occurrence 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 filter_file_paths_by_ext(paths: typing.Iterable[str], extension: str) -> typing.Iterable[str]:
"""Generate a list of paths having a given extension from a given list of file paths."""
def find_full_path(
paths: typing.List[str], *full_path_components: typing.List[str]
) -> typing.Optional[str]:
"""Return a path equal given path components if it exists in a given list of paths."""
matching_paths = (path for path in paths if path == os.path.join(*full_path_components))
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:
# Remove leading/trailing whitespace from file path
file_path_stripped = file_path.strip()
# Get file path's extension
_, file_path_ext = os.path.splitext(file_path_stripped)
# Check if this file's extension matches the extension we are looking for
if file_path_ext.lower() == extension.lower():
if file_path_ext.lower() == ext.lower():
yield file_path_stripped
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:
file_list = myzip.namelist()
manifest_filepath = find_file_inside_zip_list(file_to_read, file_list)
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.")
logger.info(f"File '{manifest_name}' not found in the archive.")
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')
# Extract the file content
with myzip.open(manifest_filepath) as file_content:
toml_content = toml.loads(file_content.read().decode())
is_a_directory = '/' in manifest_filepath
# In case manifest TOML was successfully parsed, do additional type-specific validation
# If manifest was parsed successfully, do additional type-specific validation
type_slug = toml_content['type']
if type_slug == 'theme':
theme_xmls = filter_file_paths_by_ext(file_list, '.xml')
theme_xmls = filter_paths_by_ext(file_list, '.xml')
if len(list(theme_xmls)) != 1:
error_codes.append('invalid_theme_multiple_xmls')
elif type_slug == 'add-on':
if is_a_directory:
init_filepath = find_file_inside_zip_list('__init__.py', file_list)
# __init__.py is expected to be next to the manifest
init_directory = os.path.dirname(manifest_filepath)
init_filepath = find_full_path(file_list, init_directory, '__init__.py')
if not init_filepath:
error_codes.append('invalid_missing_init')