From 4439f48910e52fc2e3b248ebc45c6644d8f02ea4 Mon Sep 17 00:00:00 2001 From: David Manthey Date: Thu, 12 Mar 2020 15:10:09 -0400 Subject: [PATCH] Fix an issue with annotation permissions. If a user or group has been removed from the system such that it still exists in an access control list, when the access list is read, Girder prunes the missing users and groups and writes back the access list. However, for annotations, this could cause a save with an empty array of elements, effectively removing the elements from the annotation. This, instead, updates the annotation's access list. --- .../models/annotation.py | 22 ++++++++++++++++--- .../test_annotation/test_annotations.py | 22 +++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/girder_annotation/girder_large_image_annotation/models/annotation.py b/girder_annotation/girder_large_image_annotation/models/annotation.py index 24cbc8d09..2bb6c61d0 100644 --- a/girder_annotation/girder_large_image_annotation/models/annotation.py +++ b/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 diff --git a/girder_annotation/test_annotation/test_annotations.py b/girder_annotation/test_annotation/test_annotations.py index 8d27c8f83..76e4a718b 100644 --- a/girder_annotation/test_annotation/test_annotations.py +++ b/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')