Extra validation of the uploaded ZIP #73
@ -58,6 +58,7 @@ EXTENSION_TYPE_PLURAL = {
|
|||||||
EXTENSION_TYPE_CHOICES.THEME: _('Themes'),
|
EXTENSION_TYPE_CHOICES.THEME: _('Themes'),
|
||||||
}
|
}
|
||||||
EXTENSION_SLUGS_PATH = '|'.join(EXTENSION_TYPE_SLUGS.values())
|
EXTENSION_SLUGS_PATH = '|'.join(EXTENSION_TYPE_SLUGS.values())
|
||||||
|
EXTENSION_TYPES = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()}
|
||||||
|
|
||||||
ALLOWED_EXTENSION_MIMETYPES = ('application/zip', )
|
ALLOWED_EXTENSION_MIMETYPES = ('application/zip', )
|
||||||
# FIXME: this controls the initial widget rendered server-side, and server-side validation
|
# FIXME: this controls the initial widget rendered server-side, and server-side validation
|
||||||
|
BIN
extensions/tests/files/theme-more-than-one-xml.zip
Normal file
BIN
extensions/tests/files/theme-more-than-one-xml.zip
Normal file
Binary file not shown.
@ -157,6 +157,20 @@ class SubmitFileTest(TestCase):
|
|||||||
{'source': ['Only .zip files are accepted.']},
|
{'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 contain a single XML file']},
|
||||||
|
)
|
||||||
|
|
||||||
def test_theme_file(self):
|
def test_theme_file(self):
|
||||||
self.assertEqual(File.objects.count(), 0)
|
self.assertEqual(File.objects.count(), 0)
|
||||||
user = UserFactory()
|
user = UserFactory()
|
||||||
|
@ -4,6 +4,7 @@ import zipfile
|
|||||||
import tempfile
|
import tempfile
|
||||||
|
|
||||||
from django import forms
|
from django import forms
|
||||||
|
from django.core.exceptions import ValidationError
|
||||||
from django.utils.safestring import mark_safe
|
from django.utils.safestring import mark_safe
|
||||||
from django.utils.translation import gettext_lazy as _
|
from django.utils.translation import gettext_lazy as _
|
||||||
|
|
||||||
@ -12,10 +13,7 @@ from .validators import (
|
|||||||
FileMIMETypeValidator,
|
FileMIMETypeValidator,
|
||||||
ManifestValidator,
|
ManifestValidator,
|
||||||
)
|
)
|
||||||
from constants.base import (
|
from constants.base import EXTENSION_TYPES, ALLOWED_EXTENSION_MIMETYPES
|
||||||
EXTENSION_TYPE_SLUGS_SINGULAR,
|
|
||||||
ALLOWED_EXTENSION_MIMETYPES,
|
|
||||||
)
|
|
||||||
import files.models
|
import files.models
|
||||||
import files.utils as utils
|
import files.utils as utils
|
||||||
|
|
||||||
@ -28,6 +26,14 @@ logger = logging.getLogger(__name__)
|
|||||||
class FileForm(forms.ModelForm):
|
class FileForm(forms.ModelForm):
|
||||||
msg_only_zip_files = _('Only .zip files are accepted.')
|
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_toml': _('A valid manifest file could not be found'),
|
||||||
|
'invalid_zip_archive': msg_only_zip_files,
|
||||||
|
'invalid_theme_multiple_xmls': _('A theme ZIP archive should contain a single XML file'),
|
||||||
|
}
|
||||||
|
|
||||||
class Meta:
|
class Meta:
|
||||||
model = files.models.File
|
model = files.models.File
|
||||||
fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user')
|
fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user')
|
||||||
@ -38,7 +44,7 @@ class FileForm(forms.ModelForm):
|
|||||||
validators=[
|
validators=[
|
||||||
FileMIMETypeValidator(
|
FileMIMETypeValidator(
|
||||||
allowed_mimetypes=ALLOWED_EXTENSION_MIMETYPES,
|
allowed_mimetypes=ALLOWED_EXTENSION_MIMETYPES,
|
||||||
message=msg_only_zip_files,
|
message=error_messages['invalid_zip_archive'],
|
||||||
),
|
),
|
||||||
],
|
],
|
||||||
widget=forms.ClearableFileInput(
|
widget=forms.ClearableFileInput(
|
||||||
@ -128,22 +134,23 @@ class FileForm(forms.ModelForm):
|
|||||||
|
|
||||||
errors = []
|
errors = []
|
||||||
if not zipfile.is_zipfile(file_path):
|
if not zipfile.is_zipfile(file_path):
|
||||||
errors.append('File is not .zip')
|
errors.append(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(ValidationError(self.error_messages[code]))
|
||||||
|
|
||||||
if manifest is None:
|
if manifest is None:
|
||||||
errors.append('A valid manifest file could not be found')
|
errors.append(ValidationError(self.error_messages['invalid_manifest_toml']))
|
||||||
else:
|
else:
|
||||||
ManifestValidator(manifest)
|
ManifestValidator(manifest)
|
||||||
ExtensionIDManifestValidator(manifest, self.extension)
|
ExtensionIDManifestValidator(manifest, self.extension)
|
||||||
|
|
||||||
extension_types = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()}
|
|
||||||
if errors:
|
if errors:
|
||||||
raise forms.ValidationError({'source': errors}, code='invalid')
|
self.add_error('source', errors)
|
||||||
|
|
||||||
self.cleaned_data['metadata'] = manifest
|
self.cleaned_data['metadata'] = manifest
|
||||||
# TODO: Error handling
|
# TODO: Error handling
|
||||||
self.cleaned_data['type'] = extension_types[manifest['type']]
|
self.cleaned_data['type'] = EXTENSION_TYPES[manifest['type']]
|
||||||
|
|
||||||
return self.cleaned_data
|
return self.cleaned_data
|
||||||
|
@ -1,3 +1,5 @@
|
|||||||
|
import typing
|
||||||
|
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
import hashlib
|
import hashlib
|
||||||
import io
|
import io
|
||||||
@ -59,30 +61,51 @@ def find_file_inside_zip_list(file_to_read: str, name_list: list) -> str:
|
|||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
def find_files_inside_zip_list(file_extension: str, name_list: list) -> typing.Iterable[str]:
|
||||||
|
"""Return a generator of list of files with a given extension a ZIP name_list."""
|
||||||
|
for file_path in name_list:
|
||||||
|
# Remove leading/trailing whitespace from file path
|
||||||
|
file_path_stripped = file_path.strip()
|
||||||
|
# Get file'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() == file_extension.lower():
|
||||||
|
yield file_path_stripped
|
||||||
|
|
||||||
|
|
||||||
def read_manifest_from_zip(archive_path):
|
def read_manifest_from_zip(archive_path):
|
||||||
file_to_read = 'blender_manifest.toml'
|
file_to_read = 'blender_manifest.toml'
|
||||||
|
error_codes = []
|
||||||
try:
|
try:
|
||||||
with zipfile.ZipFile(archive_path) as myzip:
|
with zipfile.ZipFile(archive_path) as myzip:
|
||||||
manifest_filepath = find_file_inside_zip_list(file_to_read, myzip.namelist())
|
file_list = myzip.namelist()
|
||||||
|
manifest_filepath = find_file_inside_zip_list(file_to_read, file_list)
|
||||||
|
|
||||||
if manifest_filepath is None:
|
if manifest_filepath is None:
|
||||||
logger.info(f"File '{file_to_read}' not found in the archive.")
|
logger.info(f"File '{file_to_read}' not found in the archive.")
|
||||||
return None
|
return None, error_codes
|
||||||
|
|
||||||
# Extract the file content
|
# Extract the file content
|
||||||
with myzip.open(manifest_filepath) as file_content:
|
with myzip.open(manifest_filepath) as file_content:
|
||||||
# TODO: handle TOML loading error
|
|
||||||
toml_content = toml.loads(file_content.read().decode())
|
toml_content = toml.loads(file_content.read().decode())
|
||||||
return toml_content
|
|
||||||
|
# In case manifest TOML was successfully parsed, do additional type-specific validation
|
||||||
|
if toml_content['type'] == 'theme':
|
||||||
|
theme_xmls = find_files_inside_zip_list('.xml', file_list)
|
||||||
|
if len(list(theme_xmls)) != 1:
|
||||||
|
error_codes.append('invalid_theme_multiple_xmls')
|
||||||
|
|
||||||
|
return toml_content, error_codes
|
||||||
|
|
||||||
except toml.decoder.TomlDecodeError as e:
|
except toml.decoder.TomlDecodeError as e:
|
||||||
# TODO: This error should be propagate to the user
|
|
||||||
logger.error(f"Manifest Error: {e.msg}")
|
logger.error(f"Manifest Error: {e.msg}")
|
||||||
|
error_codes.append('invalid_manifest_toml')
|
||||||
|
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.error(f"Error extracting from archive: {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:
|
def guess_mimetype_from_ext(file_name: str) -> str:
|
||||||
|
Loading…
Reference in New Issue
Block a user