enforce a higher than current version number for new listed versions (#19889)

* enforce a higher than current version number for new listed versions

* Update serializers.py

src/olympia/addons/serializers.py

@@ -73,7 +73,10 @@ from .models import (
     ReplacementAddon,
 )
 from .tasks import resize_icon, resize_preview
-from .utils import fetch_translations_from_addon
+from .utils import (
+    fetch_translations_from_addon,
+    is_version_number_not_greater_than_current,
+)
 from .validators import (
     AddonMetadataValidator,
     AddonDefaultLocaleValidator,
@@ -516,21 +519,24 @@ class DeveloperVersionSerializer(VersionSerializer):
     def validate(self, data):
         if not self.instance:
             guid = self.addon.guid if self.addon else self.parsed_data.get('guid')
-            self._check_blocklist(guid, self.parsed_data.get('version'))
+            version_string = self.parsed_data.get('version')
+            self._check_blocklist(guid, version_string)
             if self.addon:
-                self._check_for_existing_versions(self.parsed_data.get('version'))
+                self._check_for_existing_versions(version_string)
 
-                # Also check for submitting listed versions when disabled.
-                if (
-                    data['upload'].channel == amo.CHANNEL_LISTED
-                    and self.addon.disabled_by_user
-                ):
-                    raise exceptions.ValidationError(
-                        gettext(
-                            'Listed versions cannot be submitted while add-on is '
-                            'disabled.'
+                if data['upload'].channel == amo.CHANNEL_LISTED:
+                    if error_message := is_version_number_not_greater_than_current(
+                        self.addon, version_string
+                    ):
+                        raise exceptions.ValidationError({'version': error_message})
+                    # Also check for submitting listed versions when disabled.
+                    if self.addon.disabled_by_user:
+                        raise exceptions.ValidationError(
+                            gettext(
+                                'Listed versions cannot be submitted while add-on is '
+                                'disabled.'
+                            )
                         )
-                    )
         elif 'source' in data:
             # We need to manually trigger this as null/empty values aren't validated.
             try:

src/olympia/addons/tests/test_utils_.py

@@ -30,6 +30,7 @@ from ..utils import (
     get_addon_recommendations,
     get_addon_recommendations_invalid,
     is_outcome_recommended,
+    is_version_number_not_greater_than_current,
     RestrictionChecker,
     TAAR_LITE_FALLBACK_REASON_EMPTY,
     TAAR_LITE_FALLBACK_REASON_TIMEOUT,
@@ -491,3 +492,20 @@ def test_webext_version_stats():
             'prefix.for.logging',
         )
         incr_mock.assert_called_with('prefix.for.logging.webext_version.12_34_56')
+
+
+def test_is_version_number_not_greater_than_current():
+    addon = addon_factory(version_kw={'version': '123.0'})
+    # version number isn't greater (its the same)
+    assert is_version_number_not_greater_than_current(addon, '123')
+    # version number is less than the current version
+    assert is_version_number_not_greater_than_current(addon, '122.9')
+    # version number is greater, so it's okay
+    assert not is_version_number_not_greater_than_current(addon, '123.1')
+
+    addon.current_version.file.update(status=amo.STATUS_DISABLED)
+    assert not addon.current_version
+    # with no current version, it's okay
+    assert not is_version_number_not_greater_than_current(addon, '123.1')
+    # also check the edge case when addon is None
+    assert not is_version_number_not_greater_than_current(None, '123.0')

src/olympia/addons/tests/test_views.py

@@ -2199,6 +2199,7 @@ class TestAddonViewSetUpdatePut(UploadMixin, TestAddonViewSetUpdate):
     def setUp(self):
         super().setUp()
         self.addon.update(guid='@webextension-guid')
+        self.addon.current_version.update(version='0.0.0.99')
         self.url = reverse_ns(
             'addon-detail', kwargs={'pk': self.addon.guid}, api_version='v5'
         )
@@ -2971,7 +2972,11 @@ class TestVersionViewSetCreate(UploadMixin, VersionViewSetCreateUpdateMixin, Tes
             source=amo.UPLOAD_SOURCE_ADDON_API,
             channel=amo.CHANNEL_UNLISTED,
         )
-        self.addon = addon_factory(users=(self.user,), guid='@webextension-guid')
+        self.addon = addon_factory(
+            users=(self.user,),
+            guid='@webextension-guid',
+            version_kw={'version': '0.0.0.999'},
+        )
         self.url = reverse_ns(
             'addon-version-list',
             kwargs={'addon_pk': self.addon.slug},
@@ -3298,6 +3303,35 @@ class TestVersionViewSetCreate(UploadMixin, VersionViewSetCreateUpdateMixin, Tes
             'version': ['Version 0.0.1 was uploaded before and deleted.'],
         }
 
+    def test_greater_version_number_error(self):
+        self.addon.current_version.update(version='0.0.2')
+        self.upload.update(channel=amo.CHANNEL_LISTED)
+        response = self.client.post(
+            self.url,
+            data=self.minimal_data,
+        )
+        assert response.status_code == 400, response.content
+        assert response.data == {
+            'version': [
+                'Version 0.0.1 must be greater than the previous approved version '
+                '0.0.2.'
+            ],
+        }
+
+        # And check for the "same" version number
+        self.addon.current_version.update(version='0.0.1.0')
+        response = self.client.post(
+            self.url,
+            data=self.minimal_data,
+        )
+        assert response.status_code == 400, response.content
+        assert response.data == {
+            'version': [
+                'Version 0.0.1 must be greater than the previous approved version '
+                '0.0.1.0.'
+            ],
+        }
+
     def _submit_source(self, filepath, error=False):
         _, filename = os.path.split(filepath)
         src = SimpleUploadedFile(

src/olympia/addons/utils.py

@@ -300,3 +300,19 @@ def webext_version_stats(request, source):
         log.info(f'webext_version_stats webext_version: {webext_version}')
         statsd.incr(f'{source}.webext_version.{webext_version}')
     log.info('webext_version_stats no match')
+
+
+def is_version_number_not_greater_than_current(addon, version_string):
+    if (
+        addon
+        and (previous_version := addon.current_version)
+        and previous_version.version >= version_string
+    ):
+        msg = gettext(
+            'Version {version_string} must be greater than the previous approved '
+            'version {previous_version_string}.'
+        )
+        return msg.format(
+            version_string=version_string,
+            previous_version_string=previous_version.version,
+        )

src/olympia/devhub/forms.py

@@ -36,6 +36,7 @@ from olympia.addons.models import (
 )
 from olympia.addons.utils import (
     fetch_translations_from_addon,
+    is_version_number_not_greater_than_current,
     RestrictionChecker,
     verify_mozilla_trademark,
 )
@@ -1062,31 +1063,30 @@ class NewUploadForm(CheckThrottlesMixin, forms.Form):
             raise forms.ValidationError(formatted_msg)
 
     def check_for_existing_versions(self, version_string):
-        if self.addon:
-            # Make sure we don't already have this version.
-            existing_versions = Version.unfiltered.filter(
-                addon=self.addon, version=version_string
-            )
-            if existing_versions.exists():
-                version = existing_versions[0]
-                if version.deleted:
-                    msg = gettext('Version {version} was uploaded before and deleted.')
-                elif version.file.status == amo.STATUS_AWAITING_REVIEW:
-                    next_url = reverse(
-                        'devhub.submit.version.details',
-                        args=[self.addon.slug, version.pk],
+        # Make sure we don't already have this version.
+        existing_versions = Version.unfiltered.filter(
+            addon=self.addon, version=version_string
+        )
+        if existing_versions.exists():
+            version = existing_versions[0]
+            if version.deleted:
+                msg = gettext('Version {version} was uploaded before and deleted.')
+            elif version.file.status == amo.STATUS_AWAITING_REVIEW:
+                next_url = reverse(
+                    'devhub.submit.version.details',
+                    args=[self.addon.slug, version.pk],
+                )
+                msg = DoubleSafe(
+                    '%s <a href="%s">%s</a>'
+                    % (
+                        gettext('Version {version} already exists.'),
+                        next_url,
+                        gettext('Continue with existing upload instead?'),
                     )
-                    msg = DoubleSafe(
-                        '%s <a href="%s">%s</a>'
-                        % (
-                            gettext('Version {version} already exists.'),
-                            next_url,
-                            gettext('Continue with existing upload instead?'),
-                        )
-                    )
-                else:
-                    msg = gettext('Version {version} already exists.')
-                raise forms.ValidationError(msg.format(version=version_string))
+                )
+            else:
+                msg = gettext('Version {version} already exists.')
+            raise forms.ValidationError(msg.format(version=version))
 
     def clean(self):
         self.check_throttles(self.request)
@@ -1101,7 +1101,13 @@ class NewUploadForm(CheckThrottlesMixin, forms.Form):
                 self.addon.guid if self.addon else parsed_data.get('guid'),
                 parsed_data.get('version'),
             )
-            self.check_for_existing_versions(parsed_data.get('version'))
+            if self.addon:
+                self.check_for_existing_versions(parsed_data.get('version'))
+                if self.cleaned_data['upload'].channel == amo.CHANNEL_LISTED:
+                    if error_message := is_version_number_not_greater_than_current(
+                        self.addon, parsed_data.get('version')
+                    ):
+                        raise forms.ValidationError(error_message)
 
             self.cleaned_data['parsed_data'] = parsed_data
         return self.cleaned_data

src/olympia/devhub/tests/test_views_submit.py

@@ -641,7 +641,7 @@ class TestAddonSubmitUpload(UploadMixin, TestCase):
         addon = Addon.objects.get()
         self.assert3xx(response, reverse('devhub.submit.details', args=[addon.slug]))
         assert addon.current_version.file.file.name.endswith(
-            f'{addon.pk}/weta_fade-1.0.zip'
+            f'{addon.pk}/weta_fade-2.9.zip'
         )
         assert addon.type == amo.ADDON_STATICTHEME
         previews = list(addon.current_version.previews.all())
@@ -660,7 +660,7 @@ class TestAddonSubmitUpload(UploadMixin, TestCase):
         latest_version = addon.find_latest_version(channel=amo.CHANNEL_UNLISTED)
         self.assert3xx(response, reverse('devhub.submit.finish', args=[addon.slug]))
         assert latest_version.file.file.name.endswith(
-            f'{addon.pk}/{addon.slug}-1.0.zip'
+            f'{addon.pk}/{addon.slug}-2.9.zip'
         )
         assert addon.type == amo.ADDON_STATICTHEME
         # Only listed submissions need a preview generated.
@@ -688,7 +688,7 @@ class TestAddonSubmitUpload(UploadMixin, TestCase):
         # Next step is same as non-wizard flow too.
         self.assert3xx(response, reverse('devhub.submit.details', args=[addon.slug]))
         assert addon.current_version.file.file.name.endswith(
-            f'{addon.pk}/weta_fade-1.0.zip'
+            f'{addon.pk}/weta_fade-2.9.zip'
         )
         assert addon.type == amo.ADDON_STATICTHEME
         previews = list(addon.current_version.previews.all())
@@ -719,7 +719,7 @@ class TestAddonSubmitUpload(UploadMixin, TestCase):
         # Next step is same as non-wizard flow too.
         self.assert3xx(response, reverse('devhub.submit.finish', args=[addon.slug]))
         assert latest_version.file.file.name.endswith(
-            f'{addon.pk}/{addon.slug}-1.0.zip'
+            f'{addon.pk}/{addon.slug}-2.9.zip'
         )
         assert addon.type == amo.ADDON_STATICTHEME
         # Only listed submissions need a preview generated.
@@ -1981,7 +1981,7 @@ class VersionSubmitUploadMixin:
         self.user = UserProfile.objects.get(email='del@icio.us')
         self.client.force_login(self.user)
         self.user.update(last_login_ip='192.168.1.1')
-        self.addon.versions.update(channel=self.channel)
+        self.addon.versions.update(channel=self.channel, version='0.0.0.99')
         channel = 'listed' if self.channel == amo.CHANNEL_LISTED else 'unlisted'
         self.url = reverse(
             'devhub.submit.version.upload', args=[self.addon.slug, channel]
@@ -2125,6 +2125,7 @@ class VersionSubmitUploadMixin:
     def test_static_theme_wizard(self):
         channel = 'listed' if self.channel == amo.CHANNEL_LISTED else 'unlisted'
         self.addon.update(type=amo.ADDON_STATICTHEME)
+        self.version.update(version='2.1')
         # Get the correct template.
         self.url = reverse(
             'devhub.submit.version.wizard', args=[self.addon.slug, channel]
@@ -2179,6 +2180,7 @@ class VersionSubmitUploadMixin:
     def test_static_theme_wizard_unsupported_properties(self):
         channel = 'listed' if self.channel == amo.CHANNEL_LISTED else 'unlisted'
         self.addon.update(type=amo.ADDON_STATICTHEME)
+        self.version.update(version='2.1')
         # Get the correct template.
         self.url = reverse(
             'devhub.submit.version.wizard', args=[self.addon.slug, channel]
@@ -2368,6 +2370,20 @@ class TestVersionSubmitUploadListed(VersionSubmitUploadMixin, UploadMixin, TestC
             302,
         )
 
+    def test_version_num_must_be_greater(self):
+        self.version.update(version='0.0.2')
+        response = self.post(expected_status=200)
+        assert pq(response.content)('ul.errorlist').text() == (
+            'Version 0.0.1 must be greater than the previous approved version 0.0.2.'
+        )
+
+    def test_version_num_must_be_numerically_greater(self):
+        self.version.update(version='0.0.1.0')
+        response = self.post(expected_status=200)
+        assert pq(response.content)('ul.errorlist').text() == (
+            'Version 0.0.1 must be greater than the previous approved version 0.0.1.0.'
+        )
+
 
 class TestVersionSubmitUploadUnlisted(VersionSubmitUploadMixin, UploadMixin, TestCase):
     channel = amo.CHANNEL_UNLISTED

src/olympia/signing/tests/test_views.py

@@ -274,6 +274,28 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
         assert response.status_code == 200
         assert 'processed' in response.data
 
+    def test_version_num_must_be_greater(self):
+        Version.objects.filter(addon__guid=self.guid, version='2.1.072').update(
+            version='3.1'
+        )
+
+        response = self.request('PUT', self.url(self.guid, '3.0'), channel='listed')
+        assert response.status_code == 409
+        assert response.data['error'] == (
+            'Version 3.0 must be greater than the previous approved version 3.1.'
+        )
+
+    def test_version_num_must_be_numerically_greater(self):
+        Version.objects.filter(addon__guid=self.guid, version='2.1.072').update(
+            version='3.0.0'
+        )
+
+        response = self.request('PUT', self.url(self.guid, '3.0'), channel='listed')
+        assert response.status_code == 409
+        assert response.data['error'] == (
+            'Version 3.0 must be greater than the previous approved version 3.0.0.'
+        )
+
     def test_version_added_is_experiment(self):
         self.grant_permission(self.user, 'Experiments:submit')
         guid = '@experiment-inside-webextension-guid'
@@ -563,7 +585,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': '63.245.208.194',
                     'HTTP_X_FORWARDED_FOR': f'63.245.208.194, {get_random_ip()}',
@@ -578,7 +599,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': '63.245.208.194',
                     'HTTP_X_FORWARDED_FOR': f'63.245.208.194, {get_random_ip()}',
@@ -612,7 +632,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': '63.245.208.194',
                     'HTTP_X_FORWARDED_FOR': f'63.245.208.194, {get_random_ip()}',
@@ -627,7 +646,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': '63.245.208.194',
                     'HTTP_X_FORWARDED_FOR': f'63.245.208.194, {get_random_ip()}',
@@ -642,7 +660,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': '63.245.208.194',
                     'HTTP_X_FORWARDED_FOR': f'63.245.208.194, {get_random_ip()}',
@@ -668,7 +685,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -683,7 +699,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -710,7 +725,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -725,7 +739,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -739,13 +752,12 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
                 },
             )
-            assert response.status_code == expected_status
+            assert response.status_code == expected_status, response.json()
 
     def _test_throttling_verb_user_daily(self, verb, url, expected_status=201):
         with freeze_time('2019-04-08 15:16:23.42') as frozen_time:
@@ -765,7 +777,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -780,7 +791,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -794,7 +804,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -808,7 +817,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 verb,
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': get_random_ip(),
                     'HTTP_X_FORWARDED_FOR': f'{get_random_ip()}, {get_random_ip()}',
@@ -837,30 +845,30 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
         self._test_throttling_verb_user_daily('POST', url)
 
     def test_throttling_put_ip_burst(self):
-        url = self.url(self.guid, '1.0')
+        url = self.url(self.guid, '3.0')
         self._test_throttling_verb_ip_burst('PUT', url, expected_status=202)
 
     def test_throttling_put_ip_hourly(self):
-        url = self.url(self.guid, '1.0')
+        url = self.url(self.guid, '3.0')
         self._test_throttling_verb_ip_hourly('PUT', url, expected_status=202)
 
     def test_throttling_put_user_burst(self):
-        url = self.url(self.guid, '1.0')
+        url = self.url(self.guid, '3.0')
         self._test_throttling_verb_user_burst('PUT', url, expected_status=202)
 
     def test_throttling_put_user_hourly(self):
-        url = self.url(self.guid, '1.0')
+        url = self.url(self.guid, '3.0')
         self._test_throttling_verb_user_hourly('PUT', url, expected_status=202)
 
     def test_throttling_put_user_daily(self):
-        url = self.url(self.guid, '1.0')
+        url = self.url(self.guid, '3.0')
         self._test_throttling_verb_user_daily('PUT', url, expected_status=202)
 
     def test_throttling_ignored_for_special_users(self):
         self.grant_permission(
             self.user, ':'.join(amo.permissions.API_BYPASS_THROTTLING)
         )
-        url = self.url(self.guid, '1.0')
+        url = self.url(self.guid, '3.0')
         with freeze_time('2019-04-08 15:16:23.42'):
             for x in range(0, 60):
                 # With that many actions all throttling classes should prevent
@@ -878,7 +886,6 @@ class TestUploadVersion(BaseUploadVersionTestMixin, TestCase):
                 'PUT',
                 url=url,
                 guid='@create-webextension',
-                version='1.0',
                 extra_kwargs={
                     'REMOTE_ADDR': '1.2.3.4',
                     'HTTP_X_FORWARDED_FOR': f'1.2.3.4, {get_random_ip()}',

src/olympia/signing/views.py

@@ -14,7 +14,10 @@ import olympia.core.logger
 from olympia import amo
 from olympia.access import acl
 from olympia.addons.models import Addon
-from olympia.addons.utils import webext_version_stats
+from olympia.addons.utils import (
+    is_version_number_not_greater_than_current,
+    webext_version_stats,
+)
 from olympia.amo.decorators import use_primary_db
 from olympia.api.authentication import JWTKeyAuthentication
 from olympia.amo.templatetags.jinja_helpers import absolutify
@@ -236,6 +239,15 @@ class VersionView(APIView):
                     ),
                     status.HTTP_400_BAD_REQUEST,
                 )
+        if channel == amo.CHANNEL_LISTED and (
+            error_message := is_version_number_not_greater_than_current(
+                addon, version_string
+            )
+        ):
+            raise forms.ValidationError(
+                error_message,
+                status.HTTP_409_CONFLICT,
+            )
 
         # Note: The following function call contains a log statement that is
         # used by foxsec-pipeline - if refactoring, keep in mind we need to