Merge pull request #398 from mozilla-services/374-375-fix-patch-valid…

…ation

Fix schema validation with PATCH (fixes #374, fixes #375)

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)
 ------------------

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,32 @@ 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
-
-        try:
-            record_mapping.deserialize({})
-            is_empty_accepted = True
-        except colander.Invalid:
-            is_empty_accepted = False
+            return simple_mapping
 
-        record_mapping.missing = {} if is_empty_accepted else colander.required
+        if method.lower() == 'patch':
+            record_mapping = simple_mapping
+        else:
+            record_mapping = resource.mapping
 
-        class RecordPayload(colander.MappingSchema):
+            try:
+                record_mapping.deserialize({})
+                # Empty data accepted.
+                record_mapping.missing = colander.drop
+                record_mapping.default = {}
+            except colander.Invalid:
+                pass
+
+        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 +173,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')
@@ -563,13 +572,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)
@@ -1176,8 +1178,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

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'],

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,
@@ -479,11 +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.assertIn('Empty body', resp.json['message'])
+        self.app.patch(self.get_item_url(),
+                       '',
+                       headers=self.headers,
+                       status=400)
 
 
 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,