Merge pull request #428 from girder/annotations-permissions

Fix an issue with annotation permissions.

girder_annotation/girder_large_image_annotation/models/annotation.py

@@ -490,12 +490,12 @@ def _copyAnnotationsFromOtherItem(self, srcItemId, destItem):
                 continue
             annotation['itemId'] = destItemId
             del annotation['_id']
-            # Remove existing permissionsi, then give it the same permissions
+            # Remove existing permissions, then give it the same permissions
             # as the item's folder.
             annotation.pop('access', None)
             self.copyAccessPolicies(destItem, annotation, save=False)
             self.setPublic(annotation, folder.get('public'), save=False)
-            Annotation().save(annotation)
+            self.save(annotation)
             count += 1
         logger.info('Copied %d annotations from %s to %s ',
                     count, srcItemId, destItemId)
@@ -646,7 +646,8 @@ def remove(self, annotation, *args, **kwargs):
             # just mark the annotations as inactive
             result = self.update({'_id': annotation['_id']}, {'$set': {'_active': False}})
         else:
-            delete_one = self.collection.delete_one
+            with self._writeLock:
+                delete_one = self.collection.delete_one
 
             def deleteElements(query, *args, **kwargs):
                 ret = delete_one(query, *args, **kwargs)
@@ -1001,3 +1002,18 @@ def injectAnnotationGroupSet(self, annotation):
             }
             self.collection.update_one(query, update)
         return annotation
+
+    def setAccessList(self, doc, access, save=False, **kwargs):
+        """
+        The super class's setAccessList function can save a document.  However,
+        annotations which have not loaded elements lose their elements when
+        this occurs, because the validation step of the save function adds an
+        empty element list.  By using an update instead of a save, this
+        prevents the problem.
+        """
+        update = save and '_id' in doc
+        save = save and '_id' not in doc
+        doc = super(Annotation, self).setAccessList(doc, access, save=save, **kwargs)
+        if update:
+            self.update({'_id': doc['_id']}, {'$set': {'access': doc['access']}})
+        return doc

girder_annotation/test_annotation/test_annotations.py

@@ -11,6 +11,7 @@
 from girder.constants import AccessType
 from girder.exceptions import AccessException, ValidationException
 from girder.models.item import Item
+from girder.models.group import Group
 from girder.models.setting import Setting
 
 from girder_large_image_annotation.models import annotation
@@ -338,6 +339,27 @@ def testRevertVersion(self, admin):
         assert len(Annotation().revertVersion(
             annot['_id'], user=admin)['annotation']['elements']) == 1
 
+    def testPermissions(self, admin):
+        publicFolder = utilities.namedFolder(admin, 'Public')
+        item = Item().createItem('sample', admin, publicFolder)
+        annot = Annotation().createAnnotation(item, admin, sampleAnnotation)
+        group = Group().createGroup('Delete Me', admin)
+        annot['access']['groups'].append({'id': str(group['_id']), 'level': 0, 'flags': []})
+        Annotation().save(annot)
+        annot = Annotation().load(annot['_id'], getElements=False, force=True)
+        acl = Annotation().getFullAccessList(annot)
+        assert len(annot['access']['groups']) == 1
+        assert len(acl['groups']) == 1
+        # If you remove the group using the remove method, the acls will be
+        # pruned.  If you use removeWithQuery, they won't be, and getting the
+        # access list will cause the access list to be resaved.
+        Group().removeWithQuery({'_id': group['_id']})
+        acl = Annotation().getFullAccessList(annot)
+        assert len(acl['groups']) == 0
+        assert len(annot['access']['groups']) == 0
+        check = Annotation().load(annot['_id'], force=True)
+        assert len(check['annotation']['elements']) > 0
+
 
 @pytest.mark.usefixtures('unbindLargeImage', 'unbindAnnotation')
 @pytest.mark.plugin('large_image_annotation')