From e484af6acbd6cc1425ff54246625601b5a48ecd0 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Fri, 27 May 2022 19:31:09 +0100 Subject: [PATCH 1/4] Implement addon PUT as full-update or create --- docs/topics/api/addons.rst | 21 + docs/topics/api/overview.rst | 1 + src/olympia/addons/serializers.py | 28 +- src/olympia/addons/tests/test_views.py | 528 +++++++++++++------------ src/olympia/addons/validators.py | 34 +- src/olympia/addons/views.py | 49 ++- src/olympia/files/tests/test_models.py | 1 + 7 files changed, 393 insertions(+), 269 deletions(-) diff --git a/docs/topics/api/addons.rst b/docs/topics/api/addons.rst index ae3a32988ed6..293166bd7509 100644 --- a/docs/topics/api/addons.rst +++ b/docs/topics/api/addons.rst @@ -383,6 +383,27 @@ Note: as form-data can not include objects, and creating an add-on requires the :reqheader Content-Type: multipart/form-data +-------------------- +Put - Create or Edit +-------------------- + +.. _addon-put: + +This endpoint allows a submission of an upload, which will either update an existing add-on and create a new version if the guid already exists, or will create a new add-on if the guid does not exist. +See the :ref:`Add-on Create ` documentation for details of the request and restrictions. + + .. note:: + This API requires :doc:`authentication `, and for the user to be an author of the add-on if the add-on exists already. + + .. note:: + If the add-on guid is specified in the manifest it must match the the guid in the url. + + .. note:: + A submission that results in a new add-on will have metadata defaults taken from the manifest (e.g. name), but a submission that updates an existing listing will not use data from the manifest. + +.. http:put:: /api/v5/addons/addon/(string:guid)/ + + ------ Delete ------ diff --git a/docs/topics/api/overview.rst b/docs/topics/api/overview.rst index 68c58e50dadf..d3c2ea8e4e13 100644 --- a/docs/topics/api/overview.rst +++ b/docs/topics/api/overview.rst @@ -443,6 +443,7 @@ These are `v5` specific changes - `v4` changes apply also. * 2022-05-05: added the ability to delete add-on authors. https://github.com/mozilla/addons-server/issues/19163 * 2022-05-12: added the ability to add new add-on authors, as pending authors. https://github.com/mozilla/addons-server/issues/19164 * 2022-06-02: enabled setting ``default_locale`` via addon submission and edit endpoints. https://github.com/mozilla/addons-server/issues/18235 +* 2022-06-16: added the ability to "PUT" an add-on upload to either create or update an add-on. https://github.com/mozilla/addons-server/issues/15353 .. _`#11380`: https://github.com/mozilla/addons-server/issues/11380/ .. _`#11379`: https://github.com/mozilla/addons-server/issues/11379/ diff --git a/src/olympia/addons/serializers.py b/src/olympia/addons/serializers.py index e52cfc36d884..b379aa90fde4 100644 --- a/src/olympia/addons/serializers.py +++ b/src/olympia/addons/serializers.py @@ -79,6 +79,7 @@ from .validators import ( AddonMetadataValidator, AddonDefaultLocaleValidator, + MatchingGuidValidator, VersionAddonMetadataValidator, VersionLicenseValidator, VerifyMozillaTrademark, @@ -998,15 +999,17 @@ def get_ratings(self, obj): def get_is_source_public(self, obj): return False - def run_validation(self, *args, **kwargs): + def run_validation(self, data=serializers.empty): # We want name and summary to be required fields so they're not cleared, but # *only* if this is an existing add-on with listed versions. # - see AddonMetadataValidator for new add-ons/versions. if self.instance and self.instance.has_listed_versions(): - self.fields['name'].required = True - self.fields['summary'].required = True - self.fields['categories'].required = True - return super().run_validation(*args, **kwargs) + for field in ('name', 'summary', 'categories'): + if field in data: + self.fields[field].required = True + if self.instance: + self.fields['version'].addon = self.instance + return super().run_validation(data) def validate_slug(self, value): slug_validator(value) @@ -1083,6 +1086,9 @@ def log(self, instance, validated_data): if 'tag_list' in validated_data: # Tag.add_tag and Tag.remove_tag have their own logging so don't repeat it. validated_data.pop('tag_list') + if 'version' in validated_data: + # version is always a new object, and not a property either + validated_data.pop('version') if validated_data: ActivityLog.create( @@ -1140,6 +1146,11 @@ def update(self, instance, validated_data): if 'tag_list' in validated_data: del instance.tag_list # super.update will have set it. instance.set_tag_list(validated_data['tag_list']) + if 'version' in validated_data: + self.fields['version'].create( + {**validated_data.get('version', {}), 'addon': instance} + ) + self.log(instance, validated_data) return instance @@ -1154,6 +1165,13 @@ class Meta(AddonSerializer.Meta): ) +class AddonSerializerFromPut(AddonSerializerWithUnlistedData): + version = DeveloperVersionSerializer( + write_only=True, + validators=(VersionLicenseValidator(), MatchingGuidValidator()), + ) + + class SimpleAddonSerializer(AddonSerializer): class Meta: model = Addon diff --git a/src/olympia/addons/tests/test_views.py b/src/olympia/addons/tests/test_views.py index daf9fc9cae91..0ab69c96352c 100644 --- a/src/olympia/addons/tests/test_views.py +++ b/src/olympia/addons/tests/test_views.py @@ -84,8 +84,8 @@ from ..serializers import ( AddonAuthorSerializer, AddonPendingAuthorSerializer, - AddonSerializer, AddonSerializerWithUnlistedData, + CompactLicenseSerializer, DeveloperVersionSerializer, LicenseSerializer, ) @@ -737,11 +737,22 @@ def test_with_grouped_ratings(self): assert data == {'detail': 'show_grouped_ratings parameter should be a boolean'} -class AddonViewSetCreateUpdateMixin: - SUCCESS_STATUS_CODE = 200 +class RequestMixin: + client_request_verb = None + + def request(self, *, data=None, format=None, **kwargs): + verb = getattr(self.client, self.client_request_verb, None) + if not verb: + raise NotImplementedError + return verb( + self.url, + data={**getattr(self, 'minimal_data', {}), **(data or kwargs)}, + format=format, + ) - def request(self, **kwargs): - raise NotImplementedError + +class AddonViewSetCreateUpdateMixin(RequestMixin): + SUCCESS_STATUS_CODE = 200 def test_set_contributions_url(self): response = self.request(contributions_url='https://foo.baa/xxx') @@ -831,6 +842,7 @@ def test_name_and_summary_not_symbols_only(self): class TestAddonViewSetCreate(UploadMixin, AddonViewSetCreateUpdateMixin, TestCase): client_class = APITestClientSessionID + client_request_verb = 'post' SUCCESS_STATUS_CODE = 201 def setUp(self): @@ -848,14 +860,8 @@ def setUp(self): self.minimal_data = {'version': {'upload': self.upload.uuid}} self.statsd_incr_mock = self.patch('olympia.addons.serializers.statsd.incr') - def request(self, **kwargs): - return self.client.post(self.url, data={**self.minimal_data, **kwargs}) - def test_basic_unlisted(self): - response = self.client.post( - self.url, - data=self.minimal_data, - ) + response = self.request() assert response.status_code == 201, response.content data = response.data assert data['name'] == {'en-US': 'My WebExtension Addon'} @@ -864,9 +870,9 @@ def test_basic_unlisted(self): request = APIRequestFactory().get('/') request.version = 'v5' request.user = self.user - assert data == AddonSerializer(context={'request': request}).to_representation( - addon - ) + assert data == AddonSerializerWithUnlistedData( + context={'request': request} + ).to_representation(addon) assert ( addon.find_latest_version(channel=None).channel == amo.RELEASE_CHANNEL_UNLISTED @@ -881,8 +887,7 @@ def test_basic_unlisted(self): def test_basic_listed(self): self.upload.update(automated_signing=False) - response = self.client.post( - self.url, + response = self.request( data={ 'categories': {'firefox': ['bookmarks']}, 'version': { @@ -899,9 +904,9 @@ def test_basic_listed(self): request = APIRequestFactory().get('/') request.version = 'v5' request.user = self.user - assert data == AddonSerializer(context={'request': request}).to_representation( - addon - ) + assert data == AddonSerializerWithUnlistedData( + context={'request': request} + ).to_representation(addon) assert addon.current_version.channel == amo.RELEASE_CHANNEL_LISTED assert ( ActivityLog.objects.for_addons(addon) @@ -913,8 +918,7 @@ def test_basic_listed(self): def test_listed_metadata_missing(self): self.upload.update(automated_signing=False) - response = self.client.post( - self.url, + response = self.request( data={ 'version': {'upload': self.upload.uuid}, }, @@ -937,8 +941,7 @@ def test_listed_metadata_missing(self): for key, value in parse_addon(*arg, **kw).items() if key not in ('name', 'summary') } - response = self.client.post( - self.url, + response = self.request( data={ 'summary': {'en-US': 'replacement summary'}, 'name': {}, # will override the name in the manifest @@ -958,8 +961,7 @@ def test_listed_metadata_missing(self): def test_listed_metadata_null(self): self.upload.update(automated_signing=False) # name and summary are defined in the manifest but we're trying override them - response = self.client.post( - self.url, + response = self.request( data={ 'summary': {'en-US': None}, 'name': {'en-US': None}, @@ -978,10 +980,7 @@ def test_listed_metadata_null(self): def test_not_authenticated(self): self.client.logout_api() - response = self.client.post( - self.url, - data=self.minimal_data, - ) + response = self.request() assert response.status_code == 401 assert response.data == { 'detail': 'Authentication credentials were not provided.' @@ -990,10 +989,7 @@ def test_not_authenticated(self): def test_not_read_agreement(self): self.user.update(read_dev_agreement=None) - response = self.client.post( - self.url, - data=self.minimal_data, - ) + response = self.request() assert response.status_code in [401, 403] # JWT auth is a 401; web auth is 403 assert 'agreement' in response.data['detail'].lower() assert not Addon.objects.all() @@ -1007,10 +1003,7 @@ def test_waffle_flag_disabled(self): ) } with override_settings(DRF_API_GATES=gates): - response = self.client.post( - self.url, - data=self.minimal_data, - ) + response = self.request() assert response.status_code == 403 assert response.data == { 'detail': 'You do not have permission to perform this action.' @@ -1018,39 +1011,31 @@ def test_waffle_flag_disabled(self): assert not Addon.objects.all() def test_missing_version(self): - response = self.client.post( - self.url, - data={'categories': {'firefox': ['bookmarks']}}, - ) + self.minimal_data = {} + response = self.request(data={'categories': {'firefox': ['bookmarks']}}) assert response.status_code == 400, response.content assert response.data == {'version': ['This field is required.']} assert not Addon.objects.all() def test_invalid_categories(self): - response = self.client.post( - self.url, + response = self.request( # performance is an android category - data={**self.minimal_data, 'categories': {'firefox': ['performance']}}, + data={'categories': {'firefox': ['performance']}}, ) assert response.status_code == 400, response.content assert response.data == {'categories': ['Invalid category name.']} - response = self.client.post( - self.url, + response = self.request( # general is an firefox category but for dicts and lang packs - data={**self.minimal_data, 'categories': {'firefox': ['general']}}, + data={'categories': {'firefox': ['general']}} ) assert response.status_code == 400, response.content assert response.data == {'categories': ['Invalid category name.']} assert not Addon.objects.all() def test_other_category_cannot_be_combined(self): - response = self.client.post( - self.url, - data={ - **self.minimal_data, - 'categories': {'firefox': ['bookmarks', 'other']}, - }, + response = self.request( + data={'categories': {'firefox': ['bookmarks', 'other']}} ) assert response.status_code == 400, response.content assert response.data == { @@ -1061,21 +1046,15 @@ def test_other_category_cannot_be_combined(self): assert not Addon.objects.all() # but it's only enforced per app though. - response = self.client.post( - self.url, - data={ - **self.minimal_data, - 'categories': {'firefox': ['bookmarks'], 'android': ['other']}, - }, + response = self.request( + data={'categories': {'firefox': ['bookmarks'], 'android': ['other']}} ) assert response.status_code == 201 def test_too_many_categories(self): - response = self.client.post( - self.url, + response = self.request( data={ - **self.minimal_data, - 'categories': {'android': ['performance', 'shopping', 'experimental']}, + 'categories': {'android': ['performance', 'shopping', 'experimental']} }, ) assert response.status_code == 400, response.content @@ -1084,10 +1063,8 @@ def test_too_many_categories(self): } # check the limit is only applied per app - more than 2 in total is okay. - response = self.client.post( - self.url, + response = self.request( data={ - **self.minimal_data, 'categories': { 'android': ['performance', 'experimental'], 'firefox': ['bookmarks'], @@ -1098,13 +1075,7 @@ def test_too_many_categories(self): def test_set_slug(self): # Check for slugs with invalid characters in it - response = self.client.post( - self.url, - data={ - **self.minimal_data, - 'slug': '!@!#!@##@$$%$#%#%$^^%&%', - }, - ) + response = self.request(data={'slug': '!@!#!@##@$$%$#%#%$^^%&%'}) assert response.status_code == 400, response.content assert response.data == { 'slug': [ @@ -1115,26 +1086,14 @@ def test_set_slug(self): # Check for a slug in the DeniedSlug list DeniedSlug.objects.create(name='denied-slug') - response = self.client.post( - self.url, - data={ - **self.minimal_data, - 'slug': 'denied-slug', - }, - ) + response = self.request(data={'slug': 'denied-slug'}) assert response.status_code == 400, response.content assert response.data == { 'slug': ['This slug cannot be used. Please choose another.'] } # Check for all numeric slugs - DeniedSlug.blocked checks for these too. - response = self.client.post( - self.url, - data={ - **self.minimal_data, - 'slug': '1234', - }, - ) + response = self.request(data={'slug': '1234'}) assert response.status_code == 400, response.content assert response.data == { 'slug': ['This slug cannot be used. Please choose another.'] @@ -1143,10 +1102,8 @@ def test_set_slug(self): def test_slug_uniqueness(self): # Check for duplicate - we get this for free because Addon.slug is unique=True addon_factory(slug='foo', status=amo.STATUS_DISABLED) - response = self.client.post( - self.url, + response = self.request( data={ - **self.minimal_data, 'slug': 'foo', }, ) @@ -1169,10 +1126,7 @@ def test_set_extra_data(self): 'support_url': {'en-US': 'https://my.home.page/support/'}, 'version': {'upload': self.upload.uuid, 'license': self.license.slug}, } - response = self.client.post( - self.url, - data=data, - ) + response = self.request(**data) assert response.status_code == 201, response.content addon = Addon.objects.get() @@ -1217,10 +1171,7 @@ def test_override_manifest_localization(self): 'summary': {'en-US': 'new summary', 'fr': 'lé summary'}, 'version': {'upload': upload.uuid, 'license': self.license.slug}, } - response = self.client.post( - self.url, - data=data, - ) + response = self.request(**data) assert response.status_code == 201, response.content addon = Addon.objects.get() @@ -1245,14 +1196,10 @@ def test_override_manifest_localization(self): def test_fields_max_length(self): data = { - **self.minimal_data, 'name': {'fr': 'é' * 51}, 'summary': {'en-US': 'a' * 251}, } - response = self.client.post( - self.url, - data=data, - ) + response = self.request(**data) assert response.status_code == 400, response.content assert response.data == { 'name': ['Ensure this field has no more than 50 characters.'], @@ -1263,14 +1210,10 @@ def test_empty_strings_disallowed(self): # if a string is required-ish (at least in some circumstances) we'll prevent # empty strings data = { - **self.minimal_data, 'summary': {'en-US': ''}, 'name': {'en-US': ''}, } - response = self.client.post( - self.url, - data=data, - ) + response = self.request(**data) assert response.status_code == 400, response.content assert response.data == { 'summary': ['This field may not be blank.'], @@ -1278,13 +1221,10 @@ def test_empty_strings_disallowed(self): } def test_set_disabled(self): - data = { - **self.minimal_data, - 'is_disabled': True, - } - response = self.client.post( - self.url, - data=data, + response = self.request( + data={ + 'is_disabled': True, + } ) addon = Addon.objects.get() @@ -1296,15 +1236,11 @@ def test_set_disabled(self): @override_settings(EXTERNAL_SITE_URL='https://amazing.site') def test_set_homepage_support_url_email(self): data = { - **self.minimal_data, 'homepage': {'ro': '#%^%&&%^&^&^*'}, 'support_email': {'en-US': '#%^%&&%^&^&^*'}, 'support_url': {'fr': '#%^%&&%^&^&^*'}, } - response = self.client.post( - self.url, - data=data, - ) + response = self.request(**data) assert response.status_code == 400, response.content assert response.data == { @@ -1314,14 +1250,10 @@ def test_set_homepage_support_url_email(self): } data = { - **self.minimal_data, 'homepage': {'ro': settings.EXTERNAL_SITE_URL}, 'support_url': {'fr': f'{settings.EXTERNAL_SITE_URL}/foo/'}, } - response = self.client.post( - self.url, - data=data, - ) + response = self.request(**data) msg = ( 'This field can only be used to link to external websites. ' f'URLs on {settings.EXTERNAL_SITE_URL} are not allowed.' @@ -1333,10 +1265,7 @@ def test_set_homepage_support_url_email(self): } def test_set_tags(self): - response = self.client.post( - self.url, - data={**self.minimal_data, 'tags': ['foo', 'bar']}, - ) + response = self.request(data={'tags': ['foo', 'bar']}) assert response.status_code == 400, response.content assert response.data == { 'tags': { @@ -1345,10 +1274,8 @@ def test_set_tags(self): } } - response = self.client.post( - self.url, + response = self.request( data={ - **self.minimal_data, 'tags': list(Tag.objects.values_list('tag_text', flat=True)), }, ) @@ -1357,9 +1284,8 @@ def test_set_tags(self): 'tags': ['Ensure this field has no more than 10 elements.'], } - response = self.client.post( - self.url, - data={**self.minimal_data, 'tags': ['zoom', 'music']}, + response = self.request( + data={'tags': ['zoom', 'music']}, ) assert response.status_code == 201, response.content assert response.data['tags'] == ['zoom', 'music'] @@ -1367,20 +1293,15 @@ def test_set_tags(self): assert [tag.tag_text for tag in addon.tags.all()] == ['music', 'zoom'] def test_default_locale_with_invalid_locale(self): - response = self.client.post( - self.url, - data={**self.minimal_data, 'default_locale': 'zz'}, - ) + response = self.request(data={'default_locale': 'zz'}) assert response.status_code == 400 assert response.data == {'default_locale': ['"zz" is not a valid choice.']} def test_default_locale(self): # An xpi without localization - the values are in the manifest directly so will # be intepretted as whatever locale is specified as the default locale. - response = self.client.post( - self.url, + response = self.request( data={ - **self.minimal_data, 'default_locale': 'fr', # the field will have a translation in de, but won't have a value in fr 'description': {'de': 'Das description'}, @@ -1396,10 +1317,8 @@ def test_default_locale(self): # A field is provided with a value in new default # B field already has a value in new default # C field has no other translations - response = self.client.post( - self.url, + response = self.request( data={ - **self.minimal_data, 'default_locale': 'fr', 'name': {'fr': 'nom française'}, # A - a value in fr # B no summary provided, but has a value in the manifest already @@ -1432,8 +1351,7 @@ def test_default_locale_localized_xpi(self): # failure cases: # A field doesn't have a value in the xpi in new default, or # B field has other translations provided - response = self.client.post( - self.url, + response = self.request( data={ 'version': {'upload': upload.uuid}, 'default_locale': 'de', @@ -1452,8 +1370,7 @@ def test_default_locale_localized_xpi(self): # A field is provided with a value in new default # B field already has a value in new default # C field isn't required and has no other translations - response = self.client.post( - self.url, + response = self.request( data={ 'version': {'upload': upload.uuid}, 'default_locale': 'de', @@ -1480,6 +1397,44 @@ def test_default_locale_localized_xpi(self): assert addon.homepage.locale == 'de' +class TestAddonViewSetCreatePut(TestAddonViewSetCreate): + client_request_verb = 'put' + + def setUp(self): + super().setUp() + self.set_guid('@webextension-guid') + + def set_guid(self, guid): + self.guid = guid + self.url = reverse_ns('addon-detail', kwargs={'pk': guid}, api_version='v5') + + def test_default_locale_localized_xpi(self): + self.set_guid('notify-link-clicks-i18n@notzilla.org') + super().test_default_locale_localized_xpi() + + def test_override_manifest_localization(self): + self.set_guid('notify-link-clicks-i18n@notzilla.org') + super().test_override_manifest_localization() + + def test_guid_mismatch(self): + def parse_xpi_mock(pkg, addon, minimal, user): + return {**parse_xpi(pkg, addon, minimal, user), 'guid': '@something'} + + with patch('olympia.files.utils.parse_xpi', side_effect=parse_xpi_mock): + response = self.request() + assert response.status_code == 400, response.content + assert response.data == { + 'version': { + 'non_field_errors': ['GUID mismatch between the URL and manifest.'] + } + } + + def test_only_guid_works_in_url(self): + self.url = reverse_ns('addon-detail', kwargs={'pk': 'slug'}, api_version='v5') + response = self.request() + assert response.status_code == 404 + + class TestAddonViewSetCreateJWTAuth(TestAddonViewSetCreate): client_class = APITestClientJWT @@ -1487,6 +1442,7 @@ class TestAddonViewSetCreateJWTAuth(TestAddonViewSetCreate): class TestAddonViewSetUpdate(AddonViewSetCreateUpdateMixin, TestCase): client_class = APITestClientSessionID SUCCESS_STATUS_CODE = 200 + client_request_verb = 'patch' def setUp(self): super().setUp() @@ -1498,14 +1454,8 @@ def setUp(self): self.client.login_api(self.user) self.statsd_incr_mock = self.patch('olympia.addons.serializers.statsd.incr') - def request(self, **kwargs): - return self.client.patch(self.url, data={**kwargs}) - def test_basic(self): - response = self.client.patch( - self.url, - data={'summary': {'en-US': 'summary update!'}}, - ) + response = self.request(data={'summary': {'en-US': 'summary update!'}}) self.addon.reload() assert response.status_code == 200, response.content data = response.data @@ -1519,15 +1469,16 @@ def test_basic(self): context={'request': request} ).to_representation(self.addon) assert self.addon.summary == 'summary update!' - alog = ActivityLog.objects.get() + alog = ActivityLog.objects.exclude(action=amo.LOG.ADD_VERSION.id).get() assert alog.user == self.user assert alog.action == amo.LOG.EDIT_PROPERTIES.id assert alog.details == ['summary'] + return data @override_settings(API_THROTTLING=False) def test_translated_fields(self): def patch(description_dict): - return self.client.patch(self.url, data={'description': description_dict}) + return self.request(data={'description': description_dict}) self.addon.reload() self.addon.description = 'description' @@ -1539,6 +1490,7 @@ def patch(description_dict): assert response.status_code == 200, response.content assert response.data['description'] == {'en-US': desc_en_us} assert self.addon.reload().description == desc_en_us + self.addon.versions.first().update(version='0.123.1') # add description in other locale desc_es = 'descripción en español' @@ -1550,6 +1502,7 @@ def patch(description_dict): assert self.addon.reload().description == desc_es with self.activate('fr'): assert self.addon.reload().description == desc_en_us # default fallback + self.addon.versions.first().update(version='0.123.2') # delete description in other locale (and add one) desc_fr = 'descriptif en français' @@ -1561,6 +1514,7 @@ def patch(description_dict): assert self.addon.reload().description == desc_en_us # default fallback with self.activate('fr'): assert self.addon.reload().description == desc_fr + self.addon.versions.first().update(version='0.123.3') # delete description in default_locale but not "fr" - not allowed response = patch({'en-US': None}) @@ -1578,10 +1532,12 @@ def patch(description_dict): assert response.data['description'] is None self.addon = Addon.objects.get(id=self.addon.id) assert self.addon.description is None + self.addon.versions.first().update(version='0.123.4') # And repeat the same call response = patch({'en-US': None, 'fr': None}) assert response.status_code == 200, response.content + self.addon.versions.first().update(version='0.123.5') # and then set it back again response = patch({'en-US': 'something'}) @@ -1592,10 +1548,7 @@ def patch(description_dict): def test_not_authenticated(self): self.client.logout_api() - response = self.client.patch( - self.url, - data={'summary': {'en-US': 'summary update!'}}, - ) + response = self.request(data={'summary': {'en-US': 'summary update!'}}) assert response.status_code == 401 assert response.data == { 'detail': 'Authentication credentials were not provided.' @@ -1604,10 +1557,7 @@ def test_not_authenticated(self): def test_not_read_agreement(self): self.user.update(read_dev_agreement=None) - response = self.client.patch( - self.url, - data={'summary': {'en-US': 'summary update!'}}, - ) + response = self.request(data={'summary': {'en-US': 'summary update!'}}) assert response.status_code in [401, 403] # JWT auth is a 401; web auth is 403 assert 'agreement' in response.data['detail'].lower() assert self.addon.reload().summary != 'summary update!' @@ -1616,10 +1566,7 @@ def test_not_your_addon(self): self.addon.addonuser_set.get(user=self.user).update( role=amo.AUTHOR_ROLE_DELETED ) - response = self.client.patch( - self.url, - data={'summary': {'en-US': 'summary update!'}}, - ) + response = self.request(data={'summary': {'en-US': 'summary update!'}}) assert response.status_code == 403 assert response.data['detail'] == ( 'You do not have permission to perform this action.' @@ -1635,10 +1582,7 @@ def test_waffle_flag_disabled(self): ) } with override_settings(DRF_API_GATES=gates): - response = self.client.patch( - self.url, - data={'summary': {'en-US': 'summary update!'}}, - ) + response = self.request(summary={'en-US': 'summary update!'}) assert response.status_code == 403 assert response.data == { 'detail': 'You do not have permission to perform this action.' @@ -1646,10 +1590,7 @@ def test_waffle_flag_disabled(self): assert self.addon.reload().summary != 'summary update!' def test_cant_update_version(self): - response = self.client.patch( - self.url, - data={'version': {'release_notes': {'en-US': 'new notes'}}}, - ) + response = self.request(version={'release_notes': {'en-US': 'new notes'}}) assert response.status_code == 200, response.content assert self.addon.current_version.reload().release_notes != 'new notes' @@ -1660,20 +1601,15 @@ def test_update_categories(self): AddonCategory.objects.filter(addon=self.addon).update(category_id=tabs_cat.id) assert self.addon.app_categories == {'firefox': [tabs_cat]} - response = self.client.patch( - self.url, - data={'categories': {'firefox': ['bookmarks']}}, - ) + response = self.request(data={'categories': {'firefox': ['bookmarks']}}) assert response.status_code == 200, response.content assert response.data['categories'] == {'firefox': ['bookmarks']} self.addon = Addon.objects.get() assert self.addon.reload().app_categories == {'firefox': [bookmarks_cat]} + self.addon.versions.first().update(version='0.123.1') # repeat, but with the `other` category - response = self.client.patch( - self.url, - data={'categories': {'firefox': ['other']}}, - ) + response = self.request(data={'categories': {'firefox': ['other']}}) assert response.status_code == 200, response.content assert response.data['categories'] == {'firefox': ['other']} self.addon = Addon.objects.get() @@ -1685,17 +1621,15 @@ def test_invalid_categories(self): assert self.addon.app_categories == {'firefox': [tabs_cat]} del self.addon.all_categories - response = self.client.patch( - self.url, + response = self.request( # performance is an android category - data={'categories': {'firefox': ['performance']}}, + data={'categories': {'firefox': ['performance']}} ) assert response.status_code == 400, response.content assert response.data == {'categories': ['Invalid category name.']} assert self.addon.reload().app_categories == {'firefox': [tabs_cat]} - response = self.client.patch( - self.url, + response = self.request( # general is a firefox category, but for langpacks and dicts only data={'categories': {'firefox': ['general']}}, ) @@ -1704,8 +1638,7 @@ def test_invalid_categories(self): assert self.addon.reload().app_categories == {'firefox': [tabs_cat]} def test_set_slug_invalid(self): - response = self.client.patch( - self.url, + response = self.request( data={'slug': '!@!#!@##@$$%$#%#%$^^%&%'}, ) assert response.status_code == 400, response.content @@ -1718,8 +1651,7 @@ def test_set_slug_invalid(self): def test_set_slug_denied(self): DeniedSlug.objects.create(name='denied-slug') - response = self.client.patch( - self.url, + response = self.request( data={'slug': 'denied-slug'}, ) assert response.status_code == 400, response.content @@ -1727,8 +1659,7 @@ def test_set_slug_denied(self): 'slug': ['This slug cannot be used. Please choose another.'] } - response = self.client.patch( - self.url, + response = self.request( data={'slug': '1234'}, ) assert response.status_code == 400, response.content @@ -1738,8 +1669,7 @@ def test_set_slug_denied(self): # except if the slug was already in use (e.g. admin allowed) self.addon.update(slug='denied-slug') - response = self.client.patch( - self.url, + response = self.request( data={'slug': 'denied-slug'}, ) assert response.status_code == 200, response.content @@ -1759,8 +1689,7 @@ def test_set_extra_data(self): 'support_email': {'en-US': 'email@me.me'}, 'support_url': {'en-US': 'https://my.home.page/support/'}, } - response = self.client.patch( - self.url, + response = self.request( data=patch_data, ) addon = Addon.objects.get() @@ -1787,14 +1716,13 @@ def test_set_extra_data(self): assert addon.support_email == 'email@me.me' assert data['support_url']['url'] == {'en-US': 'https://my.home.page/support/'} assert addon.support_url == 'https://my.home.page/support/' - alog = ActivityLog.objects.get() + alog = ActivityLog.objects.exclude(action=amo.LOG.ADD_VERSION.id).get() assert alog.user == self.user assert alog.action == amo.LOG.EDIT_PROPERTIES.id assert alog.details == list(patch_data.keys()) def test_set_disabled(self): - response = self.client.patch( - self.url, + response = self.request( data={'is_disabled': True}, ) addon = Addon.objects.get() @@ -1804,7 +1732,7 @@ def test_set_disabled(self): assert data['is_disabled'] is True assert addon.is_disabled is True assert addon.disabled_by_user is True # sets the user property - alog = ActivityLog.objects.get() + alog = ActivityLog.objects.exclude(action=amo.LOG.ADD_VERSION.id).get() assert alog.user == self.user assert alog.action == amo.LOG.USER_DISABLE.id @@ -1812,8 +1740,7 @@ def test_set_enabled(self): addon = Addon.objects.get() # Confirm that a STATUS_DISABLED can't be overriden addon.update(status=amo.STATUS_DISABLED) - response = self.client.patch( - self.url, + response = self.request( data={'is_disabled': False}, ) addon.reload() @@ -1825,8 +1752,7 @@ def test_set_enabled(self): # But a user disabled addon can be re-enabled addon.update(status=amo.STATUS_APPROVED, disabled_by_user=True) assert addon.is_disabled is True - response = self.client.patch( - self.url, + response = self.request( data={'is_disabled': False}, ) addon.reload() @@ -1836,15 +1762,14 @@ def test_set_enabled(self): assert data['is_disabled'] is False assert addon.is_disabled is False assert addon.disabled_by_user is False # sets the user property - alog = ActivityLog.objects.get() + alog = ActivityLog.objects.exclude(action=amo.LOG.ADD_VERSION.id).get() assert alog.user == self.user assert alog.action == amo.LOG.USER_ENABLE.id def test_write_site_permission(self): addon = Addon.objects.get() self.addon.update(type=amo.ADDON_SITE_PERMISSION) - response = self.client.patch( - self.url, + response = self.request( data={'slug': 'a-new-slug'}, ) addon.reload() @@ -1861,8 +1786,7 @@ def test_set_homepage_support_url_email(self): 'support_email': {'en-US': '#%^%&&%^&^&^*'}, 'support_url': {'fr': '#%^%&&%^&^&^*'}, } - response = self.client.patch( - self.url, + response = self.request( data=data, ) @@ -1877,8 +1801,7 @@ def test_set_homepage_support_url_email(self): 'homepage': {'ro': settings.EXTERNAL_SITE_URL}, 'support_url': {'fr': f'{settings.EXTERNAL_SITE_URL}/foo/'}, } - response = self.client.patch( - self.url, + response = self.request( data=data, ) msg = ( @@ -1892,8 +1815,7 @@ def test_set_homepage_support_url_email(self): } def test_set_tags(self): - response = self.client.patch( - self.url, + response = self.request( data={'tags': ['foo', 'bar']}, ) assert response.status_code == 400, response.content @@ -1904,8 +1826,7 @@ def test_set_tags(self): } } - response = self.client.patch( - self.url, + response = self.request( data={'tags': list(Tag.objects.values_list('tag_text', flat=True))}, ) assert response.status_code == 400, response.content @@ -1917,15 +1838,14 @@ def test_set_tags(self): Tag.objects.get(tag_text='zoom').add_tag(self.addon) Tag.objects.get(tag_text='security').add_tag(self.addon) - response = self.client.patch( - self.url, + response = self.request( data={'tags': ['zoom', 'music']}, ) assert response.status_code == 200, response.content assert response.data['tags'] == ['zoom', 'music'] self.addon.reload() assert [tag.tag_text for tag in self.addon.tags.all()] == ['music', 'zoom'] - alogs = ActivityLog.objects.all() + alogs = ActivityLog.objects.exclude(action=amo.LOG.ADD_VERSION.id) assert len(alogs) == 2, [(a.action, a.details) for a in alogs] assert alogs[0].action == amo.LOG.REMOVE_TAG.id assert alogs[1].action == amo.LOG.ADD_TAG.id @@ -1942,8 +1862,8 @@ def _get_upload(self, filename): # We're mocking resize_icon because the async update of icon_hash messes up urls def test_upload_icon(self, resize_icon_mock): def patch_with_error(filename): - response = self.client.patch( - self.url, data={'icon': _get_upload(filename)}, format='multipart' + response = self.request( + data={'icon': _get_upload(filename)}, format='multipart' ) assert response.status_code == 400, response.content return response.data['icon'] @@ -1959,8 +1879,7 @@ def patch_with_error(filename): ] assert self.addon.icon_type == '' - response = self.client.patch( - self.url, + response = self.request( data={'icon': _get_upload('mozilla-sq.png')}, format='multipart', ) @@ -1987,9 +1906,9 @@ def patch_with_error(filename): assert alog.action == amo.LOG.CHANGE_MEDIA.id @mock.patch('olympia.addons.serializers.remove_icons') - def _test_delete_icon(self, request_kwargs, remove_icons_mock): + def _test_delete_icon(self, request_data, request_format, remove_icons_mock): self.addon.update(icon_type='image/png') - response = self.client.patch(self.url, **request_kwargs) + response = self.request(data=request_data, format=request_format) assert response.status_code == 200, response.content self.addon.reload() @@ -2000,19 +1919,18 @@ def _test_delete_icon(self, request_kwargs, remove_icons_mock): } assert self.addon.icon_type == '' remove_icons_mock.assert_called() - alog = ActivityLog.objects.get() + alog = ActivityLog.objects.exclude(action=amo.LOG.ADD_VERSION.id).get() assert alog.user == self.user assert alog.action == amo.LOG.CHANGE_MEDIA.id def test_delete_icon_json(self): - self._test_delete_icon({'data': {'icon': None}}) + self._test_delete_icon({'icon': None}, None) def test_delete_icon_formdata(self): - self._test_delete_icon({'data': {'icon': ''}, 'format': 'multipart'}) + self._test_delete_icon({'icon': ''}, 'multipart') def _test_metadata_content_review(self): - response = self.client.patch( - self.url, + response = self.request( data={'name': {'en-US': 'new name'}, 'summary': {'en-US': 'new summary'}}, ) assert response.status_code == 200 @@ -2077,7 +1995,7 @@ def test_metadata_change_same_content(self): def test_metadata_required(self): # name and summary are treated as required for updates data = {'name': {'en-US': None}, 'summary': {'en-US': None}, 'categories': {}} - response = self.client.patch(self.url, data=data) + response = self.request(data=data) assert response.status_code == 400 assert response.data == { 'name': ['A value in the default locale of "en-US" is required.'], @@ -2087,11 +2005,11 @@ def test_metadata_required(self): # this requirement isn't enforced for addons without listed versions though self.addon.current_version.update(channel=amo.RELEASE_CHANNEL_UNLISTED) - response = self.client.patch(self.url, data=data) + response = self.request(data=data) assert response.status_code == 200 def test_default_locale(self): - response = self.client.patch(self.url, data={'default_locale': 'zz'}) + response = self.request(data={'default_locale': 'zz'}) assert response.status_code == 400 assert response.data == {'default_locale': ['"zz" is not a valid choice.']} @@ -2106,8 +2024,7 @@ def test_default_locale(self): # B field is set to None in update # C field isn't required, but has other translations already # D field isn't required, but has other translations provided - response = self.client.patch( - self.url, + response = self.request( data={ 'default_locale': 'fr', # A no name, doesn't have a value in fr @@ -2130,8 +2047,7 @@ def test_default_locale(self): # A field is provided with a value in new default in the postdata # B field already has a value in new default - we created the Translation above # C field isn't required and has no other translations - we set description=None - response = self.client.patch( - self.url, + response = self.request( data={ 'default_locale': 'fr', 'name': {'fr': 'nom française'}, # A @@ -2144,6 +2060,121 @@ def test_default_locale(self): assert self.addon.default_locale == 'fr' +class TestAddonViewSetUpdatePut(UploadMixin, TestAddonViewSetUpdate): + client_request_verb = 'put' + + def setUp(self): + super().setUp() + self.addon.update(guid='@webextension-guid') + self.url = reverse_ns( + 'addon-detail', kwargs={'pk': self.addon.guid}, api_version='v5' + ) + + @property + def minimal_data(self): + # we generate this dynamically as some of the tests do repeated requests + upload = self.get_upload( + 'webextension.xpi', + user=self.user, + source=amo.UPLOAD_SOURCE_ADDON_API, + channel=getattr(self, 'channel', amo.RELEASE_CHANNEL_UNLISTED), + ) + return {'version': {'upload': upload.uuid}} + + def test_basic(self): + self.addon.current_version.update(license=None) + version_data = super().test_basic()['latest_unlisted_version'] + assert version_data['license'] is None + assert version_data['compatibility'] == { + 'firefox': {'max': '*', 'min': '42.0'}, + } + assert self.addon.versions.count() == 2 + version = self.addon.find_latest_version(channel=None) + assert version.channel == amo.RELEASE_CHANNEL_UNLISTED + self.statsd_incr_mock.assert_any_call('addons.submission.version.unlisted') + + def test_basic_listed(self): + license = self.addon.current_version.license + self.channel = amo.RELEASE_CHANNEL_LISTED + self.addon.current_version.file.update(status=amo.STATUS_DISABLED) + self.addon.update_status() + assert self.addon.status == amo.STATUS_NULL + + version_data = super().test_basic()['current_version'] + assert version_data['license'] == CompactLicenseSerializer().to_representation( + license + ) + assert version_data['compatibility'] == { + 'firefox': {'max': '*', 'min': '42.0'}, + } + assert self.addon.versions.count() == 2 + version = self.addon.find_latest_version(channel=None) + assert version.channel == amo.RELEASE_CHANNEL_LISTED + assert self.addon.status == amo.STATUS_NOMINATED + self.statsd_incr_mock.assert_any_call('addons.submission.version.listed') + + def test_listed_metadata_missing(self): + self.channel = amo.RELEASE_CHANNEL_LISTED + self.addon.current_version.update(license=None) + self.addon.set_categories([]) + response = self.request() + assert response.status_code == 400, response.content + assert response.data == { + 'version': { + 'license': [ + 'This field, or custom_license, is required for listed versions.' + ], + } + } + + # If the license is set we'll get further validation errors from about the addon + # fields that aren't set. + license = License.objects.create(builtin=2) + response = self.client.put( + self.url, + data={ + 'version': { + 'upload': self.minimal_data['version']['upload'], + 'license': license.slug, + } + }, + ) + assert response.status_code == 400, response.content + assert response.data == { + 'categories': ['This field is required for add-ons with listed versions.'] + } + + assert self.addon.reload().versions.count() == 1 + + def test_license_inherited_from_previous_version(self): + self.channel = amo.RELEASE_CHANNEL_LISTED + previous_license = self.addon.current_version.license + super().test_basic() + assert self.addon.versions.count() == 2 + version = self.addon.find_latest_version(channel=None) + assert version.license == previous_license + self.statsd_incr_mock.assert_any_call('addons.submission.version.listed') + + def test_only_guid_works_in_url(self): + self.url = reverse_ns( + 'addon-detail', kwargs={'pk': self.addon.slug}, api_version='v5' + ) + response = self.request() + assert response.status_code == 404 + + def test_upload_icon(self): + # It's not possible to send formdata format with nested json + pass + + def test_delete_icon_formdata(self): + # It's not possible to send formdata format with nested json + pass + + def test_cant_update_version(self): + # With put you *can* update version + pass + + class TestAddonViewSetUpdateJWTAuth(TestAddonViewSetUpdate): client_class = APITestClientJWT @@ -2398,12 +2429,9 @@ def test_developer_version_serializer_used_for_authors(self): assert 'source' in self.client.get(self.url).data -class VersionViewSetCreateUpdateMixin: +class VersionViewSetCreateUpdateMixin(RequestMixin): SUCCESS_STATUS_CODE = 200 - def request(self, **kwargs): - raise NotImplementedError - def _submit_source(self, filepath, error=False): raise NotImplementedError @@ -2786,6 +2814,7 @@ def test_cannot_specify_invalid_license_slug(self): class TestVersionViewSetCreate(UploadMixin, VersionViewSetCreateUpdateMixin, TestCase): client_class = APITestClientSessionID + client_request_verb = 'post' SUCCESS_STATUS_CODE = 201 @classmethod @@ -2824,9 +2853,6 @@ def setUp(self): self.minimal_data = {'upload': self.upload.uuid} self.statsd_incr_mock = self.patch('olympia.addons.serializers.statsd.incr') - def request(self, **kwargs): - return self.client.post(self.url, data={**self.minimal_data, **kwargs}) - def test_basic_unlisted(self): response = self.client.post( self.url, @@ -3170,6 +3196,7 @@ class TestVersionViewSetCreateJWTAuth(TestVersionViewSetCreate): class TestVersionViewSetUpdate(UploadMixin, VersionViewSetCreateUpdateMixin, TestCase): client_class = APITestClientSessionID + client_request_verb = 'patch' @classmethod def setUpTestData(cls): @@ -3206,9 +3233,6 @@ def setUp(self): ) self.client.login_api(self.user) - def request(self, **kwargs): - return self.client.patch(self.url, data={**kwargs}) - def test_basic(self): response = self.client.patch( self.url, diff --git a/src/olympia/addons/validators.py b/src/olympia/addons/validators.py index fb75fa3fabea..284e36f0d22e 100644 --- a/src/olympia/addons/validators.py +++ b/src/olympia/addons/validators.py @@ -112,22 +112,21 @@ def has_metadata(self, data, addon, field): return any(val for val in values if val) if values else bool(addon.get(field)) def get_addon_data(self, serializer): - if hasattr(serializer.fields.get('version'), 'parsed_data'): - # if we have a version field with parsed_data it's a new addon, so get it. + if not serializer.instance: + # if it's a new add-on, default values will come from parsed_data. parsed = serializer.fields['version'].parsed_data return {field: parsed.get(field) for field in self.fields} else: - # else it's a new version, so extract the existing addon data - addon = getattr(serializer, 'addon', None) - return {field: getattr(addon, field, None) for field in self.fields} + # otherwise extract the existing addon data + return {field: getattr(serializer.instance, field) for field in self.fields} def get_channel(self, data): - upload = data.get('upload', data.get('version', {}).get('upload')) + upload = data.get('version', {}).get('upload') return upload.channel if upload else None def __call__(self, data, serializer): if ( - not serializer.instance + not serializer.partial and self.get_channel(data) == amo.RELEASE_CHANNEL_LISTED ): # Check that the required metadata is set for an addon with listed versions @@ -146,6 +145,13 @@ def __call__(self, data, serializer): class VersionAddonMetadataValidator(AddonMetadataValidator): + def get_addon_data(self, serializer): + addon = getattr(serializer, 'addon', None) + return {field: getattr(addon, field, None) for field in self.fields} + + def get_channel(self, data): + return super().get_channel({'version': data}) + def __call__(self, data, serializer): try: return super().__call__(data=data, serializer=serializer) @@ -217,3 +223,17 @@ def __call__(self, data, serializer): if errors: raise exceptions.ValidationError(errors) + + +class MatchingGuidValidator: + requires_context = True + + def __call__(self, data, serializer): + if ( + (manifest_guid := serializer.parsed_data.get('guid')) + and (view_guid := serializer.context['view'].kwargs.get('guid')) + and manifest_guid != view_guid + ): + raise exceptions.ValidationError( + gettext('GUID mismatch between the URL and manifest.') + ) diff --git a/src/olympia/addons/views.py b/src/olympia/addons/views.py index 6b3671705b88..fbad540f7467 100644 --- a/src/olympia/addons/views.py +++ b/src/olympia/addons/views.py @@ -80,6 +80,7 @@ from .models import Addon, AddonUser, AddonUserPendingConfirmation, ReplacementAddon from .serializers import ( AddonEulaPolicySerializer, + AddonSerializerFromPut, AddonAuthorSerializer, AddonSerializer, AddonSerializerWithUnlistedData, @@ -207,7 +208,7 @@ def find_replacement_addon(request): class AddonViewSet( CreateModelMixin, RetrieveModelMixin, - UpdateModelMixin, + # UpdateModelMixin, - we're implementing update ourselves DestroyModelMixin, GenericViewSet, ): @@ -271,10 +272,10 @@ def get_serializer_class(self): # we are allowed to access unlisted data. obj = getattr(self, 'instance', None) request = self.request - if acl.is_unlisted_addons_viewer_or_reviewer(request.user) or ( - obj - and request.user.is_authenticated - and obj.authors.filter(pk=request.user.pk).exists() + if request.user.is_authenticated and ( + self.action in ('create', 'update', 'partial_update') + or acl.is_unlisted_addons_viewer_or_reviewer(request.user) + or (obj and obj.authors.filter(pk=request.user.pk).exists()) ): return self.serializer_class_with_unlisted_data return self.serializer_class @@ -362,6 +363,44 @@ def destroy(self, request, *args, **kwargs): return super().destroy(request, *args, **kwargs) + def partial_update(self, request, *args, **kwargs): + instance = self.get_object() + serializer = self.get_serializer(instance, data=request.data, partial=True) + serializer.is_valid(raise_exception=True) + serializer.save() + + if getattr(instance, '_prefetched_objects_cache', None): + # If 'prefetch_related' has been applied to a queryset, we need to + # forcibly invalidate the prefetch cache on the instance. + instance._prefetched_objects_cache = {} + + return Response(serializer.data) + + def update(self, request, *args, **kwargs): + try: + instance = self.get_object() + except http.Http404: + instance = None + save_kwargs = {'guid': self.kwargs.get('guid')} + else: + save_kwargs = {} + if self.lookup_field != 'guid': + raise http.Http404 + + serializer = AddonSerializerFromPut( + instance=instance, data=request.data, context=self.get_serializer_context() + ) + serializer.is_valid(raise_exception=True) + serializer.save(**save_kwargs) + + if getattr(instance, '_prefetched_objects_cache', None): + # If 'prefetch_related' has been applied to a queryset, we need to + # forcibly invalidate the prefetch cache on the instance. + instance._prefetched_objects_cache = {} + + status_code = status.HTTP_201_CREATED if not instance else status.HTTP_200_OK + return Response(serializer.data, status=status_code) + class AddonChildMixin: """Mixin containing method to retrieve the parent add-on object.""" diff --git a/src/olympia/files/tests/test_models.py b/src/olympia/files/tests/test_models.py index 6757e4125a6a..70c6285e5e44 100644 --- a/src/olympia/files/tests/test_models.py +++ b/src/olympia/files/tests/test_models.py @@ -52,6 +52,7 @@ class UploadMixin(amo.tests.AMOPaths): """ def setUp(self): + super().setUp() create_default_webext_appversion() def file_path(self, *args, **kw): From 5e58930bbed6b1df1914d9d98f1d2c779dc13351 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Thu, 9 Jun 2022 19:35:52 +0100 Subject: [PATCH 2/4] refactor serializers and views to use common code --- src/olympia/addons/serializers.py | 14 ++++---- src/olympia/addons/views.py | 54 ++++++++++++------------------- src/olympia/amo/validators.py | 13 ++++++++ 3 files changed, 40 insertions(+), 41 deletions(-) diff --git a/src/olympia/addons/serializers.py b/src/olympia/addons/serializers.py index b379aa90fde4..c59f4d54c2b8 100644 --- a/src/olympia/addons/serializers.py +++ b/src/olympia/addons/serializers.py @@ -21,6 +21,7 @@ from olympia.amo.validators import ( CreateOnlyValidator, OneOrMoreLetterOrNumberCharacterValidator, + PreventPartialUpdateValidator, ) from olympia.api.fields import ( EmailTranslationField, @@ -863,7 +864,11 @@ class AddonSerializer(serializers.ModelSerializer): version = DeveloperVersionSerializer( write_only=True, # Note: we're purposefully omitting VersionAddonMetadataValidator - validators=(CreateOnlyValidator(), VersionLicenseValidator()), + validators=( + PreventPartialUpdateValidator(), + VersionLicenseValidator(), + MatchingGuidValidator(), + ), ) versions_url = serializers.SerializerMethodField() @@ -1165,13 +1170,6 @@ class Meta(AddonSerializer.Meta): ) -class AddonSerializerFromPut(AddonSerializerWithUnlistedData): - version = DeveloperVersionSerializer( - write_only=True, - validators=(VersionLicenseValidator(), MatchingGuidValidator()), - ) - - class SimpleAddonSerializer(AddonSerializer): class Meta: model = Addon diff --git a/src/olympia/addons/views.py b/src/olympia/addons/views.py index fbad540f7467..f9c70aa2fd43 100644 --- a/src/olympia/addons/views.py +++ b/src/olympia/addons/views.py @@ -80,7 +80,6 @@ from .models import Addon, AddonUser, AddonUserPendingConfirmation, ReplacementAddon from .serializers import ( AddonEulaPolicySerializer, - AddonSerializerFromPut, AddonAuthorSerializer, AddonSerializer, AddonSerializerWithUnlistedData, @@ -208,7 +207,7 @@ def find_replacement_addon(request): class AddonViewSet( CreateModelMixin, RetrieveModelMixin, - # UpdateModelMixin, - we're implementing update ourselves + UpdateModelMixin, DestroyModelMixin, GenericViewSet, ): @@ -284,10 +283,11 @@ def get_lookup_field(self, identifier): return Addon.get_lookup_field(identifier) def get_object(self): - identifier = self.kwargs.get('pk') - self.lookup_field = self.get_lookup_field(identifier) - self.kwargs[self.lookup_field] = identifier - self.instance = super().get_object() + if not hasattr(self, 'instance'): + identifier = self.kwargs.get('pk') + self.lookup_field = self.get_lookup_field(identifier) + self.kwargs[self.lookup_field] = identifier + self.instance = super().get_object() return self.instance def check_permissions(self, request): @@ -363,43 +363,31 @@ def destroy(self, request, *args, **kwargs): return super().destroy(request, *args, **kwargs) - def partial_update(self, request, *args, **kwargs): - instance = self.get_object() - serializer = self.get_serializer(instance, data=request.data, partial=True) - serializer.is_valid(raise_exception=True) - serializer.save() - - if getattr(instance, '_prefetched_objects_cache', None): - # If 'prefetch_related' has been applied to a queryset, we need to - # forcibly invalidate the prefetch cache on the instance. - instance._prefetched_objects_cache = {} - - return Response(serializer.data) - def update(self, request, *args, **kwargs): + if kwargs.get('partial'): + # PATCH uses the standard update always + return super().update(request, *args, **kwargs) + + # for PUT we check if the object exists first try: instance = self.get_object() except http.Http404: instance = None - save_kwargs = {'guid': self.kwargs.get('guid')} - else: - save_kwargs = {} + + # We only support guid style ids for PUT requests if self.lookup_field != 'guid': raise http.Http404 - serializer = AddonSerializerFromPut( - instance=instance, data=request.data, context=self.get_serializer_context() - ) - serializer.is_valid(raise_exception=True) - serializer.save(**save_kwargs) + if instance: + # if the add-on exists we can use the standard update + return super().update(request, *args, **kwargs) - if getattr(instance, '_prefetched_objects_cache', None): - # If 'prefetch_related' has been applied to a queryset, we need to - # forcibly invalidate the prefetch cache on the instance. - instance._prefetched_objects_cache = {} + # otherwise we create a new add-on + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + serializer.save(guid=self.kwargs['guid']) - status_code = status.HTTP_201_CREATED if not instance else status.HTTP_200_OK - return Response(serializer.data, status=status_code) + return Response(serializer.data, status=status.HTTP_201_CREATED) class AddonChildMixin: diff --git a/src/olympia/amo/validators.py b/src/olympia/amo/validators.py index 67e5940054a2..b15203b994be 100644 --- a/src/olympia/amo/validators.py +++ b/src/olympia/amo/validators.py @@ -47,3 +47,16 @@ class CreateOnlyValidator: def __call__(self, value, serializer_field): if serializer_field.parent.instance is not None: raise fields.SkipField() + + +class PreventPartialUpdateValidator: + """ + This validator raises SkipField if the field is used in a partial=True + (partial_update / PATCH) serializer instance. + """ + + requires_context = True + + def __call__(self, value, serializer_field): + if serializer_field.parent.partial: + raise fields.SkipField() From 4445dd36723a9b9c65df34918f93c8464e8647bc Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 13 Jun 2022 09:44:54 +0100 Subject: [PATCH 3/4] update self.action when we create from a PUT --- src/olympia/addons/views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/olympia/addons/views.py b/src/olympia/addons/views.py index f9c70aa2fd43..696eda8eea49 100644 --- a/src/olympia/addons/views.py +++ b/src/olympia/addons/views.py @@ -383,6 +383,7 @@ def update(self, request, *args, **kwargs): return super().update(request, *args, **kwargs) # otherwise we create a new add-on + self.action = 'create' serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) serializer.save(guid=self.kwargs['guid']) From 3cd55f96d7a103d853b66c8ccee13b08631ed8b4 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 13 Jun 2022 14:34:10 +0100 Subject: [PATCH 4/4] Enforce a GUID in the manifest for PUT --- docs/topics/api/addons.rst | 2 +- src/olympia/addons/tests/test_views.py | 13 +++++++++++++ src/olympia/addons/validators.py | 17 +++++++++-------- src/olympia/addons/views.py | 12 ++++-------- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/docs/topics/api/addons.rst b/docs/topics/api/addons.rst index 293166bd7509..ad1d7c2a7fce 100644 --- a/docs/topics/api/addons.rst +++ b/docs/topics/api/addons.rst @@ -396,7 +396,7 @@ See the :ref:`Add-on Create ` documentation for details of the req This API requires :doc:`authentication `, and for the user to be an author of the add-on if the add-on exists already. .. note:: - If the add-on guid is specified in the manifest it must match the the guid in the url. + The guid in the url must match a guid specified in the manifest. .. note:: A submission that results in a new add-on will have metadata defaults taken from the manifest (e.g. name), but a submission that updates an existing listing will not use data from the manifest. diff --git a/src/olympia/addons/tests/test_views.py b/src/olympia/addons/tests/test_views.py index 0ab69c96352c..af8c9b1fee1d 100644 --- a/src/olympia/addons/tests/test_views.py +++ b/src/olympia/addons/tests/test_views.py @@ -1429,6 +1429,19 @@ def parse_xpi_mock(pkg, addon, minimal, user): } } + def test_no_guid_in_manifest(self): + def parse_xpi_mock(pkg, addon, minimal, user): + return {**parse_xpi(pkg, addon, minimal, user), 'guid': None} + + with patch('olympia.files.utils.parse_xpi', side_effect=parse_xpi_mock): + response = self.request() + assert response.status_code == 400, response.content + assert response.data == { + 'version': { + 'non_field_errors': ['A GUID must be specified in the manifest.'] + } + } + def test_only_guid_works_in_url(self): self.url = reverse_ns('addon-detail', kwargs={'pk': 'slug'}, api_version='v5') response = self.request() diff --git a/src/olympia/addons/validators.py b/src/olympia/addons/validators.py index 284e36f0d22e..d30be5f39b24 100644 --- a/src/olympia/addons/validators.py +++ b/src/olympia/addons/validators.py @@ -229,11 +229,12 @@ class MatchingGuidValidator: requires_context = True def __call__(self, data, serializer): - if ( - (manifest_guid := serializer.parsed_data.get('guid')) - and (view_guid := serializer.context['view'].kwargs.get('guid')) - and manifest_guid != view_guid - ): - raise exceptions.ValidationError( - gettext('GUID mismatch between the URL and manifest.') - ) + if view_guid := serializer.context['view'].kwargs.get('guid'): + if not (manifest_guid := serializer.parsed_data.get('guid')): + raise exceptions.ValidationError( + gettext('A GUID must be specified in the manifest.') + ) + elif manifest_guid != view_guid: + raise exceptions.ValidationError( + gettext('GUID mismatch between the URL and manifest.') + ) diff --git a/src/olympia/addons/views.py b/src/olympia/addons/views.py index 696eda8eea49..326913a556d3 100644 --- a/src/olympia/addons/views.py +++ b/src/olympia/addons/views.py @@ -381,14 +381,10 @@ def update(self, request, *args, **kwargs): if instance: # if the add-on exists we can use the standard update return super().update(request, *args, **kwargs) - - # otherwise we create a new add-on - self.action = 'create' - serializer = self.get_serializer(data=request.data) - serializer.is_valid(raise_exception=True) - serializer.save(guid=self.kwargs['guid']) - - return Response(serializer.data, status=status.HTTP_201_CREATED) + else: + # otherwise we create a new add-on + self.action = 'create' + return self.create(request, *args, **kwargs) class AddonChildMixin: