From 1d3cd0ee83ae2b1ecb770d55381b68b5ed9f40d4 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 4 Jun 2024 14:19:18 +0200 Subject: [PATCH 1/6] update manifest PermissionsValidator, keep Version.permissions field as is --- extensions/models.py | 4 +- .../tests/files/invalid-missing-wheels.zip | Bin 741 -> 723 bytes extensions/tests/test_manifest.py | 14 +++---- extensions/views/api.py | 4 +- files/validators.py | 38 +++++++++++++++--- 5 files changed, 46 insertions(+), 14 deletions(-) diff --git a/extensions/models.py b/extensions/models.py index 8c604325..b4d6b123 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -492,7 +492,9 @@ class VersionManager(models.Manager): def update_or_create(self, *args, **kwargs): # Stash the ManyToMany to be created after the Version has a valid ID already licenses = kwargs.pop('licenses', []) - permissions = kwargs.pop('permissions', []) + permissions = kwargs.pop('permissions', {}) + if permissions: + permissions = list(permissions.keys()) platforms = kwargs.pop('platforms', []) tags = kwargs.pop('tags', []) diff --git a/extensions/tests/files/invalid-missing-wheels.zip b/extensions/tests/files/invalid-missing-wheels.zip index 76c124847cc83a995de279142ec88ea29645356c..93aae3b3e35d71e333882e83c540e9f87a2633bf 100644 GIT binary patch delta 326 zcmaFLdYN^C7w@s$BM}C$rgm1s2{}Ba& z*7tw9+N>{x?ag^~Hbq~~>|;`r&*5n&r?F@xy1o8ao&HM7)}i9P;eF;g;wS2UwX_xL z-c4T4e*1RZ>PN;MDh{t|IAp|b>l^HA&vDj! zHGGRUeY#`$_p&Lw-Xtr=T{v>*LCHTPRy<3;Dh{$8#{d4BU z@gKjuIdNU&Y|nWMUY57YZ(Y}$lA*Y&qJVE-$8m|?Z{MtTZ|jymX1#VBtHMLSE$`ed zG>w1HEI;-s{LMC%wtD6OZ)O%Qpzj$N{F0YNOzvZxiW(@B4VX-sfgv(^H>1qt7A85y Pyvd81Y}j@(g0uhtDRGgn delta 331 zcmcc2`jmBo7q3R$hKT%!f6ZPqFfe3I3|Fte!{?R;!lf1542&!C$rgs;D?|6v1x zyMMLU%_2es%QLwv7nA4I z8~zXLJsA4+9M4j{2=Pe1XHIz)OHNj?m@50;RNa?0)k|};y8bo$L%TVZMIVXIl937Bcx<0_ zKa07P(x+|D_&x84xzq>xRx{?_T)r}Y=hDc!>}@PZyMMD!&SPAR8ZwjRm`o>sVw9Y` YpOI~H5tAHa!{mufHf*OEfx*TA0Aoj$bpQYW diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 431a1e3d..71e644d0 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -523,19 +523,19 @@ class ValidateManifestFields(TestCase): **self.optional_fields, } - data['permissions'] = ['files'] + data['permissions'] = {'files': 'test files'} ManifestValidator(data) data.pop('permissions') ManifestValidator(data) - data['permissions'] = ['non-supported-permission'] + data['permissions'] = {'non-supported-permission': 'lalala'} with self.assertRaises(ValidationError) as e: ManifestValidator(data) - message_begin = "Manifest value error: permissions expects a list of" + message_begin = "Manifest value error: permissions expects key/value pairs of" self.assertIn(message_begin, e.exception.messages[0]) - self.assertIn('[\'files\', \'network\']', e.exception.messages[0]) + self.assertIn('files, network', e.exception.messages[0]) # Check the permissions that will always be around. message_end = e.exception.messages[0][len(message_begin) :] @@ -546,13 +546,13 @@ class ValidateManifestFields(TestCase): for permission in VersionPermission.objects.all(): self.assertIn(permission.slug, message_end) - data['permissions'] = [] + data['permissions'] = {} with self.assertRaises(ValidationError) as e: ManifestValidator(data) self.assertEqual(1, len(e.exception.messages)) # Make sure permissions are only defined for add-ons, but fail for themes. - data['permissions'] = ['files'] + data['permissions'] = {'files': 'test files'} data['type'] = EXTENSION_TYPE_SLUGS_SINGULAR[EXTENSION_TYPE_CHOICES.THEME] data['tags'] = [] with self.assertRaises(ValidationError) as e: @@ -693,7 +693,7 @@ class VersionPermissionsTest(CreateFileTest): def test_version_permission_register(self): """Make sure permissions are saved""" - permissions = ['network'] + permissions = {'network': 'talk to server'} latest_version = self._test_version_permission(permissions) self.assertEqual(latest_version.permissions.count(), 1) self.assertEqual(latest_version.permissions.first().slug, 'network') diff --git a/extensions/views/api.py b/extensions/views/api.py index 1f410ca4..3fa5aa64 100644 --- a/extensions/views/api.py +++ b/extensions/views/api.py @@ -102,7 +102,9 @@ class ListedExtensionsSerializer(serializers.ModelSerializer): # avoid triggering additional db queries, reuse the prefetched queryset 'maintainer': instance.team and instance.team.name or str(instance.authors.all()[0]), 'license': [license_iter.slug for license_iter in matching_version.licenses.all()], - 'permissions': [permission.slug for permission in matching_version.permissions.all()], + 'permissions': { + permission.slug: '-' for permission in matching_version.permissions.all() + }, 'platforms': [platform.slug for platform in matching_version.platforms.all()], # TODO: handle copyright 'tags': [str(tag) for tag in matching_version.tags.all()], diff --git a/files/validators.py b/files/validators.py index 63fb1956..db03dd6a 100644 --- a/files/validators.py +++ b/files/validators.py @@ -321,7 +321,10 @@ class TypeValidator: class PermissionsValidator: - example = ['files', 'network'] + example = { + 'files': "Import/export FBX from/to disk", + 'network': "Need to sync motion-capture data to server", + } @classmethod def validate(cls, *, name: str, value: str, manifest: dict) -> str: @@ -342,22 +345,47 @@ class PermissionsValidator: if not is_bpy: return - if (type(value) != list) or (not value): + if not isinstance(value, dict) or not value: is_error = True else: - for permission in value: + for permission, reason in value.items(): try: VersionPermission.get_by_slug(permission) except VersionPermission.DoesNotExist: is_error = True logger.info(f'Permission unavailable: {permission}') + if not isinstance(reason, str): + return mark_safe( + 'Manifest value error: permissions reasons must be strings.' + ) + + if not reason: + return mark_safe( + f'Manifest value error: permissions {permission} reason ' + 'is empty.' + ) + + if reason[-1] in {'.', '!', '?'}: + return mark_safe( + 'Manifest value error: permissions {permission} reason ' + 'cannot end with any punctuation (.!?).' + ) + + max_length = 64 + if len(reason) > max_length: + return mark_safe( + f'Manifest value error: permissions {permission} reason is ' + 'too long ({len(value)}), ' + f'max-length: {max_length} characters' + ) if not is_error: return error_message = ( - f'Manifest value error: permissions expects a list of ' - f'supported permissions. e.g.: {cls.example}. The supported permissions are: ' + 'Manifest value error: permissions expects key/value pairs of ' + 'supported permissions with the reasons why they are requred. ' + 'The supported permissions are: ' ) for permission_ob in VersionPermission.objects.all(): -- 2.30.2 From 767added7ee8d4b2341120bef9a086b59c37f769 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 4 Jun 2024 14:27:09 +0200 Subject: [PATCH 2/6] tests for reason validation --- extensions/tests/test_manifest.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 71e644d0..94480a3c 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -529,10 +529,26 @@ class ValidateManifestFields(TestCase): data.pop('permissions') ManifestValidator(data) - data['permissions'] = {'non-supported-permission': 'lalala'} + # punctuation + data['permissions'] = {'files': 'lalala.'} with self.assertRaises(ValidationError) as e: ManifestValidator(data) + # empty reason + data['permissions'] = {'files': ''} + with self.assertRaises(ValidationError) as e: + ManifestValidator(data) + + # too long reason + data['permissions'] = { + 'files': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', + } + with self.assertRaises(ValidationError) as e: + ManifestValidator(data) + + data['permissions'] = {'non-supported-permission': 'lalala'} + with self.assertRaises(ValidationError) as e: + ManifestValidator(data) message_begin = "Manifest value error: permissions expects key/value pairs of" self.assertIn(message_begin, e.exception.messages[0]) self.assertIn('files, network', e.exception.messages[0]) -- 2.30.2 From dc298d5c8a2fe29d721e53013ec9ba77e9347182 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 4 Jun 2024 16:00:33 +0200 Subject: [PATCH 3/6] test: permissions saved in file metadata --- .../tests/files/addon-with-permissions.zip | Bin 0 -> 748 bytes extensions/tests/test_submit.py | 17 +++++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 extensions/tests/files/addon-with-permissions.zip diff --git a/extensions/tests/files/addon-with-permissions.zip b/extensions/tests/files/addon-with-permissions.zip new file mode 100644 index 0000000000000000000000000000000000000000..45939564465d7f3b4881dc82d73ad89e157de0ab GIT binary patch literal 748 zcmWIWW@h1H0D%WnJ0rjhD8bDj!w?^znU`4-AFo$X85+XLz|2x8ng+t972FJrEH9ZE z7+78bi2%4E69YOUN|*n=BnmVHgn5C66zAur#wVtvMe(_bd6{Xc z#U*+r`MEh@r>>TbPXl3y9WOvm1rfe|hS`S=1X|1gaxJ`j%y1@`Y?RmL5{?SZmV|ZJ zu39S1jq%JoxqrQ%X9m0O`^)9;Yb{c`3;!IOq4D8G=hww%x3}e6FE%>UC!s3Yl3$$qSLsfxw&(uEatTAEHXI!@9E#f?e*EGHmV&t=6Ecl?-X;P9{=JS=dW{C zomjmzL8SSAcFn3@x1}#zR@EXRF1Us>+FsgJsz=Q8_HQK!lL zqSD{%?!WrocYVXA)|h|Oy!cvqs0xfftZmDF%TFh3`-hafh1a#1C7KI q-4G*j#Vx|fEsYC-M&gKRpwU=jIl!Bh4XB5K83_La>FXe;F#rIf*ymLM literal 0 HcmV?d00001 diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index 3cc6cbe1..300b63da 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -63,6 +63,22 @@ EXPECTED_EXTENSION_DATA = { 'version_str': '1.0.8', 'slug': 'amaranth', }, + 'addon-with-permissions.zip': { + 'metadata': { + 'tagline': 'Some add-on tag line', + 'name': 'Some Add-on', + 'id': 'some_addon', + 'version': '0.1.0', + 'blender_version_min': '4.2.0', + 'type': 'add-on', + 'permissions': {'files': 'reading files', 'network': 'talking to server'}, + }, + 'file_hash': 'sha256:cce5f6268cb096cf8b070b17cab78ad2bc10f2ff85c625ef4a2b6db7c01a29ad', + 'size_bytes': 748, + 'tags': [], + 'version_str': '0.1.0', + 'slug': 'some-addon', + }, } EXPECTED_VALIDATION_ERRORS = { 'empty.txt': {'source': ['Only .zip files are accepted.']}, @@ -147,6 +163,7 @@ class SubmitFileTest(TestCase): self.assertEqual(file.hash, file_hash) self.assertEqual(file.get_type_display(), 'Add-on') self.assertEqual(file.metadata['version'], version_str) + self.assertEqual(file.metadata.get('permissions'), other_metadata.get('permissions')) def test_not_allowed_anonymous(self): with open(TEST_FILES_DIR / 'edit_breakdown-0.1.0.zip', 'rb') as fp: -- 2.30.2 From 656b4899ec4eece5a07ca6d03c9a01e478e2d829 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 4 Jun 2024 16:06:07 +0200 Subject: [PATCH 4/6] fix typo --- files/validators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/files/validators.py b/files/validators.py index db03dd6a..f3923f1a 100644 --- a/files/validators.py +++ b/files/validators.py @@ -384,7 +384,7 @@ class PermissionsValidator: error_message = ( 'Manifest value error: permissions expects key/value pairs of ' - 'supported permissions with the reasons why they are requred. ' + 'supported permissions with the reasons why they are required. ' 'The supported permissions are: ' ) -- 2.30.2 From ad9761713c90cb4f9b802dc2eda9a034498e9fe5 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 4 Jun 2024 16:23:02 +0200 Subject: [PATCH 5/6] add comment about the permissions mismatch in model and manifest --- extensions/models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/extensions/models.py b/extensions/models.py index b4d6b123..1880982c 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -493,6 +493,8 @@ class VersionManager(models.Manager): # Stash the ManyToMany to be created after the Version has a valid ID already licenses = kwargs.pop('licenses', []) permissions = kwargs.pop('permissions', {}) + # FIXME: ideally this dict->list transformation should be a caller's responsibility + # but we plan to change the model soon, so getting a dict as an input here makes sense if permissions: permissions = list(permissions.keys()) platforms = kwargs.pop('platforms', []) -- 2.30.2 From 944284520b66acd8c501d9d838ddcccf8d008f6a Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 4 Jun 2024 17:17:07 +0200 Subject: [PATCH 6/6] better stub for api: use help text --- extensions/views/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/extensions/views/api.py b/extensions/views/api.py index 3fa5aa64..0fe10298 100644 --- a/extensions/views/api.py +++ b/extensions/views/api.py @@ -103,7 +103,8 @@ class ListedExtensionsSerializer(serializers.ModelSerializer): 'maintainer': instance.team and instance.team.name or str(instance.authors.all()[0]), 'license': [license_iter.slug for license_iter in matching_version.licenses.all()], 'permissions': { - permission.slug: '-' for permission in matching_version.permissions.all() + permission.slug: permission.help + for permission in matching_version.permissions.all() }, 'platforms': [platform.slug for platform in matching_version.platforms.all()], # TODO: handle copyright -- 2.30.2