Fix acl update bug

lib/SampleService/core/acls.py

@@ -18,6 +18,8 @@
 )
 from SampleService.core.user import UserID
 
+# may need to add sane limits on ACL sizes if people act like jerks
+
 
 class SampleAccessType(IntEnum):
     '''

lib/SampleService/core/storage/arango_sample_storage.py

@@ -832,15 +832,21 @@ def _update_sample_acls_pt2(self, id_, update, owner, update_time):
         # some other thread of operation changed the ACLs to the exact state that application of
         # our delta update would result in. The only issue here is that the update time stamp will
         # be a few milliseconds later than it should be, so don't worry about it.
+        a = [u.id for u in update.admin]
+        w = [u.id for u in update.write]
+        r = [u.id for u in update.read]
+        rem = [u.id for u in update.remove]
 
         bind_vars = {'@col': self._col_sample.name,
                      'id': str(id_),
                      'owner': owner.id,
                      'ts': update_time.timestamp(),
-                     'admin': [u.id for u in update.admin],
-                     'write': [u.id for u in update.write],
-                     'read': [u.id for u in update.read],
-                     'remove': [u.id for u in update.remove],
+                     'admin': a,
+                     'admin_remove': w + r + rem,
+                     'write': w,
+                     'write_remove': a + r + rem,
+                     'read': r,
+                     'read_remove': a + w + rem,
                      }
         # Could return a subset of s to save bandwith
         # ensures the owner hasn't changed since we pulled the acls above.
@@ -852,11 +858,11 @@ def _update_sample_acls_pt2(self, id_, update, owner, update_time):
                     {_FLD_ACL_UPDATE_TIME}: @ts,
                     {_FLD_ACLS}: {{
                         {_FLD_ADMIN}: REMOVE_VALUES(UNION_DISTINCT(
-                            s.{_FLD_ACLS}.{_FLD_ADMIN}, @admin), @remove),
+                            s.{_FLD_ACLS}.{_FLD_ADMIN}, @admin), @admin_remove),
                         {_FLD_WRITE}: REMOVE_VALUES(UNION_DISTINCT(
-                            s.{_FLD_ACLS}.{_FLD_WRITE}, @write), @remove),
+                            s.{_FLD_ACLS}.{_FLD_WRITE}, @write), @write_remove),
                         {_FLD_READ}: REMOVE_VALUES(UNION_DISTINCT(
-                            s.{_FLD_ACLS}.{_FLD_READ}, @read), @remove)
+                            s.{_FLD_ACLS}.{_FLD_READ}, @read), @read_remove)
         '''
         if update.public_read is not None:
             aql += f''',

test/core/storage/arango_sample_storage_test.py

@@ -1248,6 +1248,92 @@ def test_update_sample_acls_with_false_public(samplestorage):
         False)
 
 
+def test_update_sample_acls_with_existing_users(samplestorage):
+    '''
+    Tests that when a user is added to an acl it's removed from any other acls.
+    '''
+    id_ = uuid.UUID('1234567890abcdef1234567890abcdef')
+    assert samplestorage.save_sample(
+        SavedSample(id_, UserID('user'), [TEST_NODE], dt(1), 'foo')) is True
+
+    samplestorage.replace_sample_acls(id_, SampleACL(
+        UserID('user'),
+        dt(56),
+        admin=[UserID('a1'), UserID('a2'), UserID('arem')],
+        write=[UserID('w1'), UserID('w2'), UserID('wrem')],
+        read=[UserID('r1'), UserID('r2'), UserID('rrem')]))
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(
+        admin=[UserID('w1')], remove=[UserID('arem')]), dt(89))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(89),
+        [UserID('a1'), UserID('a2'), UserID('w1')],
+        [UserID('w2'), UserID('wrem')],
+        [UserID('r1'), UserID('r2'), UserID('rrem')],
+        False)
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(admin=[UserID('r1')]), dt(90))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(90),
+        [UserID('a1'), UserID('a2'), UserID('w1'), UserID('r1')],
+        [UserID('w2'), UserID('wrem')],
+        [UserID('r2'), UserID('rrem')],
+        False)
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(
+        read=[UserID('w1')], remove=[UserID('wrem')]), dt(91))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(91),
+        [UserID('a1'), UserID('a2'), UserID('r1')],
+        [UserID('w2')],
+        [UserID('r2'), UserID('w1'), UserID('rrem')],
+        False)
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(read=[UserID('a1')]), dt(92))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(92),
+        [UserID('a2'), UserID('r1')],
+        [UserID('w2')],
+        [UserID('r2'), UserID('w1'), UserID('a1'), UserID('rrem')],
+        False)
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(
+        write=[UserID('a2')], remove=[UserID('rrem')]), dt(93))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(93),
+        [UserID('r1')],
+        [UserID('w2'), UserID('a2')],
+        [UserID('r2'), UserID('w1'), UserID('a1')],
+        False)
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(
+        write=[UserID('r2')], read=[UserID('a2'), UserID('a1')]), dt(94))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(94),
+        [UserID('r1')],
+        [UserID('w2'), UserID('r2')],
+        [UserID('w1'), UserID('a2'), UserID('a1')],
+        False)
+
+
 def test_update_sample_acls_fail_bad_args(samplestorage):
     id_ = uuid.uuid4()
     s = SampleACLDelta()