Merge 6ac84a2 into dd5fac2

lib/SampleService/core/acls.py

@@ -217,26 +217,43 @@ def is_update(self, update: SampleACLDelta) -> bool:
         :param update: the update.
         :returns: True if the update would change the ACLs, False if not. The timestamp is not
             considered.
-        :raises UnauthorizedError: if the update would affect the owner.
+        :raises UnauthorizedError: if the update would affect the owner and update.at_least is
+            not true, or if the owner is in the remove list regardless of at_least.
         '''
         _not_falsy(update, 'update')
         o = self.owner
-        if o in update.admin or o in update.write or o in update.read or o in update.remove:
+        ownerchange = o in update.admin or o in update.write or o in update.read
+        if (ownerchange and not update.at_least) or o in update.remove:
             raise _UnauthorizedError(
                 f'ACLs for the sample owner {o.id} may not be modified by a delta update.')
+
         rem = set(update.remove)
         admin = set(self.admin)
         write = set(self.write)
         read = set(self.read)
-        pub = update.public_read
-
-        return (not set(update.admin).issubset(admin) or
-                not set(update.write).issubset(write) or
-                not set(update.read).issubset(read) or
-                not rem.isdisjoint(admin) or
-                not rem.isdisjoint(write) or
-                not rem.isdisjoint(read) or
-                (pub is not None and pub is not self.public_read))
+
+        # check if users are removed
+        if not rem.isdisjoint(admin) or not rem.isdisjoint(write) or not rem.isdisjoint(read):
+            return True
+
+        # check if public read is changed
+        if update.public_read is not None and update.public_read is not self.public_read:
+            return True
+
+        uadmin = set(update.admin)
+        uwrite = set(update.write)
+        uread = set(update.read)
+        owner = set([o])
+
+        # check if users' permission is changed
+        if update.at_least:
+            return (not uadmin.issubset(admin | owner) or
+                    not uwrite.issubset(write | admin | owner) or
+                    not uread.issubset(read | write | admin | owner))
+        else:
+            return (not uadmin.issubset(admin) or
+                    not uwrite.issubset(write) or
+                    not uread.issubset(read))
 
     def __eq__(self, other):
         if type(other) is type(self):

lib/SampleService/core/storage/arango_sample_storage.py

@@ -894,9 +894,13 @@ 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]
+
+        # we remove the owner from the update list for the case where update.at_least is
+        # true so that we don't add the owner to another ACL. If at_least is false, the
+        # update class would've thrown an error.
+        a = [u.id for u in update.admin if u != owner]
+        w = [u.id for u in update.write if u != owner]
+        r = [u.id for u in update.read if u != owner]
         rem = [u.id for u in update.remove]
 
         bind_vars = {'@col': self._col_sample.name,
@@ -914,8 +918,8 @@ def _update_sample_acls_pt2(self, id_, update, owner, update_time):
         else:
             bind_vars['admin_remove'] = w + r + rem
             bind_vars['write_remove'] = a + r + rem
-        # Could return a subset of s to save bandwith
-        # ensures the owner hasn't changed since we pulled the acls above.
+        # Could return a subset of s to save bandwith (see query text)
+        # ensures the owner hasn't changed since we pulled the acls above (see query text).
         aql = self._UPDATE_ACLS_AT_LEAST_AQL if update.at_least else self._UPDATE_ACLS_AQL
         if update.public_read is not None:
             aql += f''',

test/arango_controller.py

@@ -72,9 +72,9 @@ def __init__(self, arangoexe: Path, arangojs: Path, root_temp_dir: Path) -> None
 
     def destroy(self, delete_temp_files: bool) -> None:
         """
-        Shut down the MongoDB server.
+        Shut down the ArangoDB server.
 
-        :param delete_temp_files: delete all the MongoDB data files and logs generated during the
+        :param delete_temp_files: delete all the ArangoDB data files and logs generated during the
             test.
         """
         if self._proc:

test/core/acls_test.py

@@ -191,21 +191,36 @@ def test_is_update():
     assert s.is_update(SampleACLDelta()) is False
 
     assert s.is_update(SampleACLDelta([u('a1')])) is False
+    assert s.is_update(SampleACLDelta([u('a1')], at_least=True)) is False
+    assert s.is_update(SampleACLDelta([u('o')], at_least=True)) is False
     assert s.is_update(SampleACLDelta([u('a3')])) is True
 
     assert s.is_update(SampleACLDelta(write=[u('w2')])) is False
+    assert s.is_update(SampleACLDelta(write=[u('w2')], at_least=True)) is False
+    assert s.is_update(SampleACLDelta(write=[u('o')], at_least=True)) is False
+    assert s.is_update(SampleACLDelta(write=[u('a1')])) is True
+    assert s.is_update(SampleACLDelta(write=[u('a1')], at_least=True)) is False
     assert s.is_update(SampleACLDelta(write=[u('w4')])) is True
 
     assert s.is_update(SampleACLDelta(read=[u('r1')])) is False
+    assert s.is_update(SampleACLDelta(read=[u('r1')], at_least=True)) is False
+    assert s.is_update(SampleACLDelta(read=[u('o')], at_least=True)) is False
+    assert s.is_update(SampleACLDelta(read=[u('a1')])) is True
+    assert s.is_update(SampleACLDelta(read=[u('a1')], at_least=True)) is False
+    assert s.is_update(SampleACLDelta(read=[u('w1')])) is True
+    assert s.is_update(SampleACLDelta(read=[u('w1')], at_least=True)) is False
     assert s.is_update(SampleACLDelta(read=[u('r3')])) is True
 
     assert s.is_update(SampleACLDelta(remove=[u('a1')])) is True
+    assert s.is_update(SampleACLDelta(remove=[u('a1')], at_least=True)) is True
     assert s.is_update(SampleACLDelta(remove=[u('a3')])) is False
 
     assert s.is_update(SampleACLDelta(remove=[u('w2')])) is True
+    assert s.is_update(SampleACLDelta(remove=[u('w2')], at_least=True)) is True
     assert s.is_update(SampleACLDelta(remove=[u('w4')])) is False
 
     assert s.is_update(SampleACLDelta(remove=[u('r1')])) is True
+    assert s.is_update(SampleACLDelta(remove=[u('r1')], at_least=True)) is True
     assert s.is_update(SampleACLDelta(remove=[u('r3')])) is False
 
     assert s.is_update(SampleACLDelta(public_read=False)) is True
@@ -229,6 +244,9 @@ def test_is_update_fail():
     _is_update_fail(
         s, SampleACLDelta([u('a')], remove=[u('v'), u('u')]),
         UnauthorizedError('ACLs for the sample owner u may not be modified by a delta update.'))
+    _is_update_fail(
+        s, SampleACLDelta([u('a')], remove=[u('v'), u('u')], at_least=True),
+        UnauthorizedError('ACLs for the sample owner u may not be modified by a delta update.'))
 
 
 def _is_update_fail(sample, delta, expected):

test/core/storage/arango_sample_storage_test.py

@@ -1240,6 +1240,54 @@ def _update_sample_acls(samplestorage, at_least):
         True)
 
 
+def test_update_sample_acls_with_at_least_True_and_owner_in_admin_acl(samplestorage):
+    # owner should be included in any changes with at_least = True.
+    _update_sample_acls_with_owner_in_acl(
+        samplestorage,
+        [UserID('foo'), UserID('bar1'), UserID('user')],
+        [UserID('baz1'), UserID('bat')],
+        [UserID('whoo1')],
+    )
+
+
+def test_update_sample_acls_with_at_least_True_and_owner_in_write_acl(samplestorage):
+    # owner should be included in any changes with at_least = True.
+    _update_sample_acls_with_owner_in_acl(
+        samplestorage,
+        [UserID('foo'), UserID('bar1')],
+        [UserID('baz1'), UserID('bat'), UserID('user')],
+        [UserID('whoo1')],
+    )
+
+
+def test_update_sample_acls_with_at_least_True_and_owner_in_read_acl(samplestorage):
+    # owner should be included in any changes with at_least = True.
+    _update_sample_acls_with_owner_in_acl(
+        samplestorage,
+        [UserID('foo'), UserID('bar1')],
+        [UserID('baz1'), UserID('bat')],
+        [UserID('whoo1'), UserID('user')],
+    )
+
+
+def _update_sample_acls_with_owner_in_acl(samplestorage, admin, write, read):
+    id_ = uuid.UUID('1234567890abcdef1234567890abcdef')
+    assert samplestorage.save_sample(
+        SavedSample(id_, UserID('user'), [TEST_NODE], dt(1), 'foo')) is True
+
+    samplestorage.update_sample_acls(
+        id_, SampleACLDelta(admin, write, read, public_read=True, at_least=True), dt(101))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(101),
+        [UserID('foo'), UserID('bar1')],
+        [UserID('baz1'), UserID('bat')],
+        [UserID('whoo1')],
+        True)
+
+
 def test_update_sample_acls_noop_with_at_least_False(samplestorage):
     _update_sample_acls_noop(samplestorage, False)
 
@@ -1546,13 +1594,6 @@ def test_update_sample_acls_fail_alters_owner(samplestorage):
     _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(write=[UserID('us')]), t, err)
     _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(read=[UserID('us')]), t, err)
     _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(remove=[UserID('us')]), t, err)
-
-    _update_sample_acls_fail(
-        samplestorage, id_, SampleACLDelta([UserID('us')], at_least=True), t, err)
-    _update_sample_acls_fail(
-        samplestorage, id_, SampleACLDelta(write=[UserID('us')], at_least=True), t, err)
-    _update_sample_acls_fail(
-        samplestorage, id_, SampleACLDelta(read=[UserID('us')], at_least=True), t, err)
     _update_sample_acls_fail(
         samplestorage, id_, SampleACLDelta(remove=[UserID('us')], at_least=True), t, err)