Switch to Python 3.8 (#15074)

* Remove obsolete code to build a webext dictionary from a legacy one

* Simplify SafeZip initialization code

We don't need to store is_valid, since an exception will be raised
if something is invalid.

* Cleanup temporary directory for invalid zips in extract_extension_to_dest()

* Switch to Python 3.8

- Rename BadZipfile to BadZipFile as the former is deprecated
- Support BadZipFile being raised earlier (when instantiating
  the ZipFile) in validation process

* Also remove test for build_webext_dictionary_from_legacy

* Remove a stray `is_valid` on a `SafeZip` instance

* Fix test now that we consider badzipfile errors fatal

.travis.yml

@@ -2,7 +2,7 @@ language: python
 dist: xenial
 
 python:
-  - 3.6
+  - 3.8
 
 addons:
   apt:

Dockerfile

@@ -1,4 +1,4 @@
-FROM python:3.6-slim-buster
+FROM python:3.8-slim-buster
 
 ENV PYTHONDONTWRITEBYTECODE=1
 

Dockerfile.deploy

@@ -1,4 +1,4 @@
-FROM python:3.6-slim-buster
+FROM python:3.8-slim-buster
 
 # need to compile swig
 ENV SWIG_FEATURES="-D__x86_64__"

src/olympia/addons/tests/test_utils_.py

@@ -1,17 +1,11 @@
 # -*- coding: utf-8 -*-
-import json
 from unittest import mock
 import pytest
-import tempfile
-import zipfile
 
 from django.conf import settings
 from django.forms import ValidationError
-from django.utils.encoding import force_text
 
-from olympia import amo
 from olympia.addons.utils import (
-    build_webext_dictionary_from_legacy,
     get_addon_recommendations, get_addon_recommendations_invalid,
     is_outcome_recommended,
     TAAR_LITE_FALLBACK_REASON_EMPTY, TAAR_LITE_FALLBACK_REASON_TIMEOUT,
@@ -19,7 +13,7 @@ from olympia.addons.utils import (
     TAAR_LITE_OUTCOME_REAL_FAIL, TAAR_LITE_OUTCOME_REAL_SUCCESS,
     TAAR_LITE_FALLBACK_REASON_INVALID,
     verify_mozilla_trademark)
-from olympia.amo.tests import AMOPaths, TestCase, addon_factory, user_factory
+from olympia.amo.tests import TestCase, addon_factory, user_factory
 
 
 @pytest.mark.django_db
@@ -126,70 +120,3 @@ class TestGetAddonRecommendations(TestCase):
         assert not is_outcome_recommended(TAAR_LITE_OUTCOME_REAL_FAIL)
         assert not is_outcome_recommended(TAAR_LITE_OUTCOME_CURATED)
         assert not self.recommendation_server_mock.called
-
-
-class TestBuildWebextDictionaryFromLegacy(AMOPaths, TestCase):
-    def setUp(self):
-        self.addon = addon_factory(
-            target_locale='ar', type=amo.ADDON_DICT,
-            version_kw={'version': '1.0'},
-            file_kw={'is_webextension': False})
-        self.xpi_copy_over(
-            self.addon.current_version.all_files[0], 'dictionary-test.xpi')
-
-    def check_xpi_file_contents(self, xpi_file_path, expected_version):
-        with zipfile.ZipFile(xpi_file_path, 'r', zipfile.ZIP_DEFLATED) as xpi:
-            # Check that manifest is present, contains proper version and
-            # dictionaries properties.
-            manifest = force_text(xpi.read('manifest.json'))
-            manifest_json = json.loads(manifest)
-            assert (
-                manifest_json['browser_specific_settings']['gecko']['id'] ==
-                self.addon.guid)
-            assert manifest_json['version'] == expected_version
-            expected_dict_obj = {'ar': 'dictionaries/ar.dic'}
-            assert manifest_json['dictionaries'] == expected_dict_obj
-
-            # Check that we haven't included any useless files.
-            expected_files = sorted([
-                'dictionaries/',
-                'dictionaries/ar.aff',
-                'dictionaries/ar.dic',
-                'dictionaries/license.txt',
-                'manifest.json'
-            ])
-            assert sorted([x.filename for x in xpi.filelist]) == expected_files
-
-    def test_basic(self):
-        with tempfile.NamedTemporaryFile(suffix='.xpi') as destination:
-            build_webext_dictionary_from_legacy(self.addon, destination)
-            self.check_xpi_file_contents(destination, '1.0.1webext')
-
-    def test_current_not_valid_raises(self):
-        mod = 'olympia.files.utils.SafeZip.initialize_and_validate'
-        with mock.patch(mod) as is_valid:
-            is_valid.return_value = False
-            with tempfile.NamedTemporaryFile(suffix='.xpi') as destination:
-                with self.assertRaises(ValidationError):
-                    build_webext_dictionary_from_legacy(
-                        self.addon, destination)
-
-    def test_addon_has_no_target_locale(self):
-        self.addon.update(target_locale=None)
-        with tempfile.NamedTemporaryFile(suffix='.xpi') as destination:
-            build_webext_dictionary_from_legacy(self.addon, destination)
-            self.check_xpi_file_contents(destination, '1.0.1webext')
-        self.addon.reload()
-
-    def test_invalid_dictionary_path_raises(self):
-        self.xpi_copy_over(
-            self.addon.current_version.all_files[0], 'extension.xpi')
-        with tempfile.NamedTemporaryFile(suffix='.xpi') as destination:
-            with self.assertRaises(ValidationError):
-                build_webext_dictionary_from_legacy(self.addon, destination)
-
-    def test_version_number_typefix(self):
-        self.addon.current_version.update(version='1.1-typefix')
-        with tempfile.NamedTemporaryFile(suffix='.xpi') as destination:
-            build_webext_dictionary_from_legacy(self.addon, destination)
-            self.check_xpi_file_contents(destination, '1.2webext')

src/olympia/addons/utils.py

@@ -1,16 +1,11 @@
-import json
-import os.path
-import re
 import uuid
-import zipfile
 
 from django import forms
 from django.conf import settings
-from django.forms import ValidationError
 from django.utils.translation import ugettext
 
 from olympia import amo
-from olympia.amo.utils import normalize_string, to_language
+from olympia.amo.utils import normalize_string
 from olympia.discovery.utils import call_recommendation_server
 from olympia.translations.fields import LocaleErrorMessage
 
@@ -96,86 +91,3 @@ def get_addon_recommendations_invalid():
     return (
         TAAR_LITE_FALLBACKS, TAAR_LITE_OUTCOME_REAL_FAIL,
         TAAR_LITE_FALLBACK_REASON_INVALID)
-
-
-MULTIPLE_STOPS_REGEX = re.compile(r'\.{2,}')
-
-
-def build_webext_dictionary_from_legacy(addon, destination):
-    """Create a webext package of a legacy dictionary `addon`, and put it in
-    `destination` path."""
-    from olympia.files.utils import SafeZip  # Avoid circular import.
-    old_path = addon.current_version.all_files[0].file_path
-    old_zip = SafeZip(old_path)
-    if not old_zip.is_valid:
-        raise ValidationError('Current dictionary xpi is not valid')
-
-    dictionary_path = ''
-
-    with zipfile.ZipFile(destination, 'w', zipfile.ZIP_DEFLATED) as new_zip:
-        for obj in old_zip.filelist:
-            splitted = obj.filename.split('/')
-            # Ignore useless directories and files.
-            if splitted[0] in ('META-INF', '__MACOSX', 'chrome',
-                               'chrome.manifest', 'install.rdf'):
-                continue
-
-            # Also ignore javascript (regardless of where they are, not just at
-            # the root), since dictionaries should not contain any code.
-            if splitted[-1].endswith('.js'):
-                continue
-
-            # Store the path of the last .dic file we find. It can be inside a
-            # directory.
-            if (splitted[-1].endswith('.dic')):
-                dictionary_path = obj.filename
-
-            new_zip.writestr(obj.filename, old_zip.read(obj.filename))
-
-        # Now that all files we want from the old zip are copied, build and
-        # add manifest.json.
-        if not dictionary_path:
-            # This should not happen... It likely means it's an invalid
-            # dictionary to begin with, or one that has its .dic file in a
-            # chrome/ directory for some reason. Abort!
-            raise ValidationError('Current dictionary xpi has no .dic file')
-
-        if addon.target_locale:
-            target_language = addon.target_locale
-        else:
-            # Guess target_locale since we don't have one already. Note that
-            # for extra confusion, target_locale is a language, not a locale.
-            target_language = to_language(os.path.splitext(
-                os.path.basename(dictionary_path))[0])
-            if target_language not in settings.AMO_LANGUAGES:
-                # We couldn't find that language in the list we support. Let's
-                # try with just the prefix.
-                target_language = target_language.split('-')[0]
-                if target_language not in settings.AMO_LANGUAGES:
-                    # We tried our best.
-                    raise ValidationError(u'Addon has no target_locale and we'
-                                          u' could not guess one from the xpi')
-
-        # Dumb version number increment. This will be invalid in some cases,
-        # but some of the dictionaries we have currently already have wild
-        # version numbers anyway.
-        version_number = addon.current_version.version
-        if version_number.endswith('.1-typefix'):
-            version_number = version_number.replace('.1-typefix', '.2webext')
-        else:
-            version_number = '%s.1webext' % version_number
-
-        manifest = {
-            'manifest_version': 2,
-            'name': str(addon.name),
-            'browser_specific_settings': {
-                'gecko': {
-                    'id': addon.guid,
-                },
-            },
-            'version': version_number,
-            'dictionaries': {target_language: dictionary_path},
-        }
-
-        # Write manifest.json we just build.
-        new_zip.writestr('manifest.json', json.dumps(manifest))

src/olympia/devhub/forms.py

@@ -748,7 +748,7 @@ class WithSourceMixin(object):
                     zip_file = SafeZip(source)
                     # testzip() returns None if there are no broken CRCs.
                     if zip_file.zip_file.testzip() is not None:
-                        raise zipfile.BadZipfile()
+                        raise zipfile.BadZipFile()
                 elif source.name.endswith(('.tar.gz', '.tar.bz2', '.tgz')):
                     # For tar files we need to do a little more work.
                     mode = 'r:bz2' if source.name.endswith('bz2') else 'r:gz'
@@ -759,7 +759,7 @@ class WithSourceMixin(object):
                 else:
                     raise forms.ValidationError(
                         self.get_invalid_source_file_type_message())
-            except (zipfile.BadZipfile, tarfile.ReadError, IOError, EOFError):
+            except (zipfile.BadZipFile, tarfile.ReadError, IOError, EOFError):
                 raise forms.ValidationError(
                     ugettext('Invalid or broken archive.'))
         return source

src/olympia/devhub/tasks.py

@@ -9,7 +9,7 @@ import tempfile
 from copy import deepcopy
 from decimal import Decimal
 from functools import wraps
-from zipfile import BadZipfile
+from zipfile import BadZipFile
 
 from django.conf import settings
 from django.core.cache import cache
@@ -180,11 +180,12 @@ def validation_task(fn):
                 results, type_='error',
                 message=exc.message, msg_id='unsupported_filetype')
             return results
-        except BadZipfile:
+        except BadZipFile:
+            # If we raised a BadZipFile we can return a single exception with
+            # a generic message indicating the zip is invalid or corrupt.
             results = deepcopy(amo.VALIDATOR_SKELETON_EXCEPTION_WEBEXT)
-            annotations.insert_validation_message(
-                results, type_='error',
-                message=ugettext('Invalid or corrupt add-on file.'))
+            results['messages'][0]['message'] = ugettext(
+                'Invalid or corrupt add-on file.')
             return results
         except Exception as exc:
             log.exception('Unhandled error during validation: %r' % exc)
@@ -395,7 +396,7 @@ def check_for_api_keys_in_file(results, upload_pk):
                             revoke_api_key.apply_async(
                                 kwargs={'key_id': key.id}, countdown=120)
             zipfile.close()
-    except (ValidationError, BadZipfile, IOError):
+    except (ValidationError, BadZipFile, IOError):
         pass
 
     return results

src/olympia/devhub/tests/test_views.py

@@ -1352,8 +1352,7 @@ class TestUploadDetail(BaseUploadTest):
         # We do raise a specific error message explaining that the archive is
         # not valid instead of a generic exception.
         assert message == [
-            ('Invalid or corrupt add-on file.', False),
-            ('Sorry, we couldn\'t load your WebExtension.', True),
+            ('Invalid or corrupt add-on file.', True),
         ]
 
     @mock.patch('olympia.devhub.tasks.run_addons_linter')

src/olympia/files/tests/test_file_viewer.py

@@ -464,12 +464,11 @@ class TestSafeZipFile(TestCase, amo.tests.AMOPaths):
             SafeZip(self.xpi_path('langpack-localepicker'))
 
     def test_unzip_fatal(self):
-        with pytest.raises(zipfile.BadZipfile):
+        with pytest.raises(zipfile.BadZipFile):
             SafeZip(self.xpi_path('search.xml'))
 
     def test_read(self):
         zip_file = SafeZip(self.xpi_path('langpack-localepicker'))
-        assert zip_file.is_valid
         assert b'locale browser de' in zip_file.read('chrome.manifest')
 
     def test_not_secure(self):

src/olympia/files/tests/test_utils.py

@@ -1211,7 +1211,7 @@ def test_id_to_path(value, expected):
 
 class TestSafeZip(TestCase):
     def test_raises_error_for_invalid_webextension_xpi(self):
-        with pytest.raises(forms.ValidationError):
+        with pytest.raises(zipfile.BadZipFile):
             utils.SafeZip(get_addon_file('invalid_webextension.xpi'))
 
     def test_raises_validation_error_when_uncompressed_size_is_too_large(self):

src/olympia/files/utils.py

@@ -744,16 +744,12 @@ class SafeZip(object):
         self.info_list = None
         self.mode = mode
         self.force_fsync = force_fsync
-        self.is_valid = self.initialize_and_validate()
+        self.initialize_and_validate()
 
     def initialize_and_validate(self):
         """
         Runs some overall archive checks.
         """
-        # Shortcut to avoid expensive check over and over again
-        if getattr(self, 'is_valid', False):
-            return True
-
         if self.force_fsync:
             zip_file = FSyncedZipFile(self.source, self.mode)
         else:
@@ -772,7 +768,6 @@ class SafeZip(object):
 
         self.info_list = info_list
         self.zip_file = zip_file
-        return True
 
     def is_signed(self):
         """Tells us if an addon is signed."""
@@ -870,6 +865,7 @@ def extract_extension_to_dest(source, dest=None, force_fsync=False):
     :returns: Extraction target directory, if `dest` is `None` it'll be a
               temporary directory.
     :raises FileNotFoundError: if the source file is not found on the filestem
+    :raises forms.ValidationError: if the zip is invalid
     """
     target, tempdir = None, None
 
@@ -894,14 +890,17 @@ def extract_extension_to_dest(source, dest=None, force_fsync=False):
             shutil.copy(source, target)
             if force_fsync:
                 FSyncMixin()._fsync_file(target)
-    except (zipfile.BadZipfile, tarfile.ReadError, IOError) as e:
+    except (zipfile.BadZipFile, tarfile.ReadError, IOError,
+            forms.ValidationError) as e:
         if tempdir is not None:
             rm_local_tmp_dir(tempdir)
-        if isinstance(e, FileNotFoundError):
+        if isinstance(e, (FileNotFoundError, forms.ValidationError)):
             # We let FileNotFoundError (which are a subclass of IOError, or
-            # rather OSError but that's an alias) be raised, the caller will
-            # have to deal with it.
+            # rather OSError but that's an alias) and ValidationError be
+            # raised, the caller will have to deal with it.
             raise
+        # Any other exceptions we caught, we raise a generic ValidationError
+        # instead.
         raise forms.ValidationError(
             ugettext('Invalid or broken archive.'))
     return target

src/olympia/lib/crypto/signing.py

@@ -210,7 +210,7 @@ def is_signed(file_path):
     try:
         with zipfile.ZipFile(file_path, mode='r') as zf:
             filenames = set(zf.namelist())
-    except (zipfile.BadZipfile, IOError):
+    except (zipfile.BadZipFile, IOError):
         filenames = set()
     return set([u'META-INF/mozilla.rsa', u'META-INF/mozilla.sf',
                 u'META-INF/manifest.mf']).issubset(filenames)

src/olympia/lib/jingo_minify_helpers.py

@@ -1,3 +1,4 @@
+import errno
 import os
 import subprocess
 import time
@@ -51,7 +52,7 @@ def ensure_path_exists(path):
         os.makedirs(os.path.dirname(path))
     except OSError as e:
         # If the directory already exists, that is fine. Otherwise re-raise.
-        if e.errno != os.errno.EEXIST:
+        if e.errno != errno.EEXIST:
             raise
 
     return path