Fix permissions among record data in 412 errors (fixes Kinto/kinto#224)

cliquet/resource/__init__.py

@@ -345,12 +345,12 @@ def put(self):
             if existing:
                 self._raise_412_if_modified(existing)
 
-        new_record = self.request.validated['data']
+        post_record = self.request.validated['data']
 
-        record_id = new_record.setdefault(id_field, self.record_id)
+        record_id = post_record.setdefault(id_field, self.record_id)
         self._raise_400_if_id_mismatch(record_id, self.record_id)
 
-        new_record = self.process_record(new_record, old=existing)
+        new_record = self.process_record(post_record, old=existing)
 
         try:
             unique = self.mapping.get_option('unique_fields')
@@ -414,7 +414,7 @@ def patch(self):
             try:
                 unique_fields = self.mapping.get_option('unique_fields')
                 new_record = self.collection.update_record(
-                    updated,
+                    new_record,
                     unique_fields=unique_fields)
             except storage_exceptions.UnicityError as e:
                 self._raise_conflict(e)
@@ -943,21 +943,32 @@ def _extract_filters(self, queryparams=None):
 
         return filters
 
+    def _raise_412_if_modified(self, record=None):
+        """Do not provide the permissions among the record fields.
+        Ref: https://github.com/Kinto/kinto/issues/224
+        """
+        if record:
+            record = record.copy()
+            record.pop(self.collection.permissions_field, None)
+        return super(ProtectedResource, self)._raise_412_if_modified(record)
+
     def process_record(self, new, old=None):
         """Read permissions from request body, and in the case of ``PUT`` every
         existing ACE is removed (using empty list).
         """
         permissions = self.request.validated.get('permissions', {})
 
+        annotated = new.copy()
+
         if permissions:
             is_put = (self.request.method.lower() == 'put')
             if is_put:
                 # Remove every existing ACEs using empty lists.
                 for perm in self.permissions:
                     permissions.setdefault(perm, [])
-            new[self.collection.permissions_field] = permissions
+            annotated[self.collection.permissions_field] = permissions
 
-        return new
+        return annotated
 
     def postprocess(self, result):
         """Add ``permissions`` attribute in response body.
@@ -966,8 +977,11 @@ def postprocess(self, result):
         outside the ``data`` attribute.
         """
         result = super(ProtectedResource, self).postprocess(result)
-        if not isinstance(result['data'], list):
-            perms = result['data'].pop(self.collection.permissions_field, None)
-            if perms is not None:
-                result['permissions'] = {k: list(p) for k, p in perms.items()}
+        if isinstance(result['data'], list):
+            # collection endpoint.
+            return result
+
+        perms = result['data'].pop(self.collection.permissions_field, None)
+        if perms is not None:
+            result['permissions'] = {k: list(p) for k, p in perms.items()}
         return result

cliquet/resource/model.py

@@ -269,30 +269,32 @@ def delete_records(self, filters=None, parent_id=None):
     def get_record(self, record_id, parent_id=None):
         """Fetch current permissions and add them to returned record.
         """
-        record = super(ProtectedModel, self).get_record(
-            record_id, parent_id)
+        record = super(ProtectedModel, self).get_record(record_id, parent_id)
         perm_object_id = self.get_permission_object_id(record_id)
         permissions = self.permission.object_permissions(perm_object_id)
-        record[self.permissions_field] = permissions
-        return record
+
+        annotated = record.copy()
+        annotated[self.permissions_field] = permissions
+        return annotated
 
     def create_record(self, record, parent_id=None, unique_fields=None):
         """Create record and set specified permissions.
 
         The current principal is added to the owner (``write`` permission).
         """
         permissions = record.pop(self.permissions_field, {})
-        record = super(ProtectedModel, self).create_record(
-            record, parent_id, unique_fields)
-
+        record = super(ProtectedModel, self).create_record(record,
+                                                           parent_id,
+                                                           unique_fields)
         record_id = record[self.id_field]
         perm_object_id = self.get_permission_object_id(record_id)
         self.permission.replace_object_permissions(perm_object_id, permissions)
         self._allow_write(perm_object_id)
         permissions = self.permission.object_permissions(perm_object_id)
-        record[self.permissions_field] = permissions
 
-        return record
+        annotated = record.copy()
+        annotated[self.permissions_field] = permissions
+        return annotated
 
     def update_record(self, record, parent_id=None, unique_fields=None):
         """Update record and the specified permissions.
@@ -303,24 +305,24 @@ def update_record(self, record, parent_id=None, unique_fields=None):
         The current principal is added to the owner (``write`` permission).
         """
         permissions = record.pop(self.permissions_field, {})
-        record = super(ProtectedModel, self).update_record(
-            record, parent_id, unique_fields)
-
+        record = super(ProtectedModel, self).update_record(record,
+                                                           parent_id,
+                                                           unique_fields)
         record_id = record[self.id_field]
         perm_object_id = self.get_permission_object_id(record_id)
         self.permission.replace_object_permissions(perm_object_id, permissions)
         self._allow_write(perm_object_id)
         permissions = self.permission.object_permissions(perm_object_id)
-        record[self.permissions_field] = permissions
 
-        return record
+        annotated = record.copy()
+        annotated[self.permissions_field] = permissions
+        return annotated
 
     def delete_record(self, record_id, parent_id=None):
         """Delete record and its associated permissions.
         """
-        record = super(ProtectedModel, self).delete_record(
-            record_id, parent_id)
-
+        record = super(ProtectedModel, self).delete_record(record_id,
+                                                           parent_id)
         perm_object_id = self.get_permission_object_id(record_id)
         self.permission.delete_object_permissions(perm_object_id)
 

cliquet/tests/resource/test_object_permissions.py

@@ -1,5 +1,7 @@
 import mock
 
+from pyramid import httpexceptions
+
 from cliquet.resource import ProtectedResource
 from cliquet.tests.resource import BaseTest
 from cliquet.permission.memory import Permission
@@ -69,8 +71,8 @@ def test_permissions_gives_lists_of_principals_per_ace(self):
 class SpecifyRecordPermissionTest(PermissionTest):
     def setUp(self):
         super(SpecifyRecordPermissionTest, self).setUp()
-        record = self.resource.collection.create_record({})
-        record_id = record['id']
+        self.record = self.resource.collection.create_record({})
+        record_id = self.record['id']
         self.record_uri = '/articles/%s' % record_id
         self.permission.add_principal_to_ace(self.record_uri,
                                              'read',
@@ -147,6 +149,16 @@ def test_permissions_can_be_removed_with_patch(self):
         self.assertEqual(sorted(result['permissions']['write']),
                          ['basicauth:userid', 'jean-louis'])
 
+    def test_412_errors_do_not_put_permission_in_record(self):
+        self.resource.request.headers['If-Match'] = '"1234567"'  # invalid
+        try:
+            self.resource.put()
+        except httpexceptions.HTTPPreconditionFailed as e:
+            error = e
+        self.assertEqual(error.json['details']['existing'],
+                         {'id': self.record['id'],
+                          'last_modified': self.record['last_modified']})
+
 
 class DeletedRecordPermissionTest(PermissionTest):
     def setUp(self):