From d69b424d1ac71593881070430aa74adc396906a8 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 17 Jul 2015 13:45:36 +0200 Subject: [PATCH 1/3] Fix schema validation with PATCH (fixes #374, #375) --- CHANGELOG.rst | 2 ++ cliquet/resource.py | 34 +++++++++++-------- .../tests/resource/test_object_permissions.py | 2 +- cliquet/tests/resource/test_views.py | 15 ++++++-- 4 files changed, 36 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7dc0aee4..11362343 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -15,6 +15,8 @@ This document describes changes between each past release. - ``data`` is not mandatory in request body if the resource does not define any schema or if no field is mandatory (fixes mozilla-services/kinto#63) +- Fix no validation error on PATCH with unknown attribute (fixes #374) +- Fix permissions not validated on PATCH (fixes #375) 2.3.1 (2015-07-15) ------------------ diff --git a/cliquet/resource.py b/cliquet/resource.py index 95aa87f9..26c4db9f 100644 --- a/cliquet/resource.py +++ b/cliquet/resource.py @@ -35,7 +35,7 @@ class ViewSet(object): collection_methods = ('GET', 'POST', 'DELETE') record_methods = ('GET', 'PUT', 'PATCH', 'DELETE') - validate_schema_for = ('POST', 'PUT') + validate_schema_for = ('POST', 'PUT', 'PATCH') readonly_methods = ('GET',) @@ -93,27 +93,31 @@ def get_view_arguments(self, endpoint_type, resource, method): def get_record_schema(self, resource, method): """Return the Cornice schema for the given method. """ + simple_mapping = colander.MappingSchema(unknown='preserve') + if method.lower() not in map(str.lower, self.validate_schema_for): # Simply validate that posted body is a mapping. - return colander.MappingSchema(unknown='preserve') - - record_mapping = resource.mapping + return simple_mapping - try: - record_mapping.deserialize({}) - is_empty_accepted = True - except colander.Invalid: - is_empty_accepted = False + if method.lower() == 'patch': + record_mapping = simple_mapping + else: + record_mapping = resource.mapping - record_mapping.missing = {} if is_empty_accepted else colander.required + try: + record_mapping.deserialize({}) + # Empty data accepted. + record_mapping.missing = {} + except colander.Invalid: + pass - class RecordPayload(colander.MappingSchema): + class PayloadSchema(colander.MappingSchema): data = record_mapping def schema_type(self, **kw): return colander.Mapping(unknown='raise') - return RecordPayload() + return PayloadSchema() def get_view(self, endpoint_type, method): """Return the view method name located on the resource object, for the @@ -168,6 +172,10 @@ def get_record_schema(self, resource, method): if method.lower() not in map(str.lower, self.validate_schema_for): return schema + if method.lower() == 'patch': + # Data is optional when patching permissions. + schema.children[-1].missing = colander.drop + permissions_node = PermissionsSchema(missing=colander.drop, permissions=resource.permissions, name='permissions') @@ -1176,8 +1184,6 @@ def patch(self): result = super(ProtectedResource, self).patch() object_id = authorization.get_object_id(self.request.path) - # Since there is no schema on PATCH, set JSON as validated. - self.request.validated = self.request.json self._store_permissions(object_id=object_id) result['permissions'] = self._build_permissions(object_id=object_id) return result diff --git a/cliquet/tests/resource/test_object_permissions.py b/cliquet/tests/resource/test_object_permissions.py index f9f97f8b..8fa5d844 100644 --- a/cliquet/tests/resource/test_object_permissions.py +++ b/cliquet/tests/resource/test_object_permissions.py @@ -109,7 +109,7 @@ def test_permissions_are_replaced_with_put(self): def test_permissions_are_modified_with_patch(self): perms = {'write': ['jean-louis']} - self.resource.request.json = {'permissions': perms} + self.resource.request.validated = {'permissions': perms} self.resource.request.method = 'PATCH' result = self.resource.patch() self.assertEqual(result['permissions'], diff --git a/cliquet/tests/resource/test_views.py b/cliquet/tests/resource/test_views.py index 34b37496..8957756b 100644 --- a/cliquet/tests/resource/test_views.py +++ b/cliquet/tests/resource/test_views.py @@ -345,6 +345,12 @@ def test_modify_with_invalid_record_returns_400(self): headers=self.headers, status=400) + def test_modify_with_unknown_attribute_returns_400(self): + self.app.patch_json(self.get_item_url(), + {"datta": {}}, + headers=self.headers, + status=400) + def test_replace_with_invalid_record_returns_400(self): self.app.put_json(self.get_item_url(), self.invalid_record, @@ -483,7 +489,6 @@ def test_modify_with_empty_returns_400(self): '', headers=self.headers, status=400) - self.assertIn('Empty body', resp.json['message']) class InvalidPermissionsTest(BaseWebTest): @@ -497,7 +502,7 @@ def setUp(self): headers=self.headers) self.record = resp.json['data'] self.invalid_body = {'data': MINIMALIST_RECORD, - 'permissions': {'read': 'book'}} + 'permissions': {'read': 'book'}} # book not list def test_permissions_are_not_accepted_on_normal_resources(self): body = {'data': MINIMALIST_RECORD, @@ -512,6 +517,12 @@ def test_create_invalid_body_returns_400(self): headers=self.headers, status=400) + def test_modify_with_invalid_permissions_returns_400(self): + self.app.patch_json(self.get_item_url(), + self.invalid_body, + headers=self.headers, + status=400) + def test_invalid_body_returns_json_formatted_error(self): resp = self.app.post_json(self.collection_url, self.invalid_body, From e450c70d8ee0bc35b916a7e2a68ac0d4e785fd71 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 17 Jul 2015 14:05:48 +0200 Subject: [PATCH 2/3] Remove dead code --- cliquet/resource.py | 7 ------- cliquet/tests/resource/test_views.py | 8 ++++---- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/cliquet/resource.py b/cliquet/resource.py index 26c4db9f..86f31fc0 100644 --- a/cliquet/resource.py +++ b/cliquet/resource.py @@ -571,13 +571,6 @@ def patch(self): old_record = self._get_record_or_404(self.record_id) self._raise_412_if_modified(old_record) - # Empty body for patch is invalid. - if not self.request.body: - error_details = { - 'description': 'Empty body' - } - raise_invalid(self.request, **error_details) - changes = self.request.json.get('data', {}) # May patch only perms. updated = self.apply_changes(old_record, changes=changes) diff --git a/cliquet/tests/resource/test_views.py b/cliquet/tests/resource/test_views.py index 8957756b..17bfefbd 100644 --- a/cliquet/tests/resource/test_views.py +++ b/cliquet/tests/resource/test_views.py @@ -485,10 +485,10 @@ def test_modify_with_invalid_uft8_returns_400(self): self.assertIn('escape sequence', resp.json['message']) def test_modify_with_empty_returns_400(self): - resp = self.app.patch(self.get_item_url(), - '', - headers=self.headers, - status=400) + self.app.patch(self.get_item_url(), + '', + headers=self.headers, + status=400) class InvalidPermissionsTest(BaseWebTest): From c617abd2317a5032feef525f6d9261b5241341c8 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 17 Jul 2015 16:25:10 +0200 Subject: [PATCH 3/3] Do not use mutable value for schema.missing --- cliquet/resource.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cliquet/resource.py b/cliquet/resource.py index 86f31fc0..3a7b7492 100644 --- a/cliquet/resource.py +++ b/cliquet/resource.py @@ -107,7 +107,8 @@ def get_record_schema(self, resource, method): try: record_mapping.deserialize({}) # Empty data accepted. - record_mapping.missing = {} + record_mapping.missing = colander.drop + record_mapping.default = {} except colander.Invalid: pass