Add update_sample_acls to database

lib/SampleService/core/storage/arango_sample_storage.py

@@ -77,7 +77,7 @@
 from apscheduler.schedulers.background import BackgroundScheduler as _BackgroundScheduler
 from arango.database import StandardDatabase
 
-from SampleService.core.acls import SampleACL
+from SampleService.core.acls import SampleACL, SampleACLDelta
 from SampleService.core.core_types import PrimitiveType as _PrimitiveType
 from SampleService.core.data_link import DataLink
 from SampleService.core.sample import SavedSample
@@ -88,13 +88,15 @@
 from SampleService.core.arg_checkers import not_falsy_in_iterable as _not_falsy_in_iterable
 from SampleService.core.arg_checkers import check_string as _check_string
 from SampleService.core.arg_checkers import check_timestamp as _check_timestamp
-from SampleService.core.errors import ConcurrencyError as _ConcurrencyError
-from SampleService.core.errors import DataLinkExistsError as _DataLinkExistsError
-from SampleService.core.errors import NoSuchLinkError as _NoSuchLinkError
-from SampleService.core.errors import NoSuchSampleError as _NoSuchSampleError
-from SampleService.core.errors import NoSuchSampleVersionError as _NoSuchSampleVersionError
-from SampleService.core.errors import NoSuchSampleNodeError as _NoSuchSampleNodeError
-from SampleService.core.errors import TooManyDataLinksError as _TooManyDataLinksError
+from SampleService.core.errors import (
+    ConcurrencyError as _ConcurrencyError,
+    DataLinkExistsError as _DataLinkExistsError,
+    NoSuchLinkError as _NoSuchLinkError,
+    NoSuchSampleError as _NoSuchSampleError,
+    NoSuchSampleVersionError as _NoSuchSampleVersionError,
+    NoSuchSampleNodeError as _NoSuchSampleNodeError,
+    TooManyDataLinksError as _TooManyDataLinksError,
+)
 from SampleService.core.storage.errors import SampleStorageError as _SampleStorageError
 from SampleService.core.storage.errors import StorageInitError as _StorageInitError
 from SampleService.core.storage.errors import OwnerChangedError as _OwnerChangedError
@@ -771,12 +773,14 @@ def replace_sample_acls(self, id_: UUID, acls: SampleACL):
                 FILTER s.{_FLD_ARANGO_KEY} == @id
                 FILTER s.{_FLD_ACLS}.{_FLD_OWNER} == @owner
                 UPDATE s WITH {{{_FLD_ACLS}: MERGE(s.{_FLD_ACLS}, @acls),
-                                {_FLD_ACL_UPDATE_TIME}: {acls.lastupdate.timestamp()}}} IN @@col
+                                {_FLD_ACL_UPDATE_TIME}: @ts
+                                }} IN @@col
                 RETURN s
             '''
         bind_vars = {'@col': self._col_sample.name,
                      'id': str(id_),
                      'owner': acls.owner.id,
+                     'ts': acls.lastupdate.timestamp(),
                      'acls': {_FLD_ADMIN: [u.id for u in acls.admin],
                               _FLD_WRITE: [u.id for u in acls.write],
                               _FLD_READ: [u.id for u in acls.read],
@@ -792,7 +796,84 @@ def replace_sample_acls(self, id_: UUID, acls: SampleACL):
         except _arango.exceptions.AQLQueryExecuteError as e:  # this is a real pain to test
             raise _SampleStorageError('Connection to database failed: ' + str(e)) from e
 
-    # TODO change acls with more granularity
+    def update_sample_acls(self, id_: UUID, update: SampleACLDelta) -> None:
+        '''
+        Update a sample's ACLs via a delta specification.
+
+        :param id_: the sample ID.
+        :param update: the update to apply to the ACLs.
+        :raises NoSuchSampleError: if the sample does not exist.
+        :raises SampleStorageError: if the sample could not be retrieved.
+        :raises UnauthorizedError: if the update attempts to alter the sample owner.
+        '''
+        # Needs to ensure owner is not added to another ACL
+        # could make an option to just ignore the update to the owner? YAGNI for now.
+        _not_falsy(update, 'update')
+        s = self.get_sample_acls(id_)
+        if not s.is_update(update):
+            # noop. Theoretically the values in the DB may have changed since we pulled the ACLs,
+            # but now we're talking about millisecond ordering differences, so don't worry
+            # about it.
+            return
+        self._update_sample_acls_pt2(id_, update, s.owner)
+
+    def _update_sample_acls_pt2(self, id_, update, owner):
+        # this method is split solely to allow testing the owner change case.
+
+        # At this point we're committed to a DB update and therefore an ACL update time bump
+        # (unless we make the query very complicated, which probably isn't worth the
+        # complexity). Even with the noop checking code above, it's still possible for the DB
+        # update to be a noop and yet bump the update time. What that means, though, is that
+        # 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.
+
+        bind_vars = {'@col': self._col_sample.name,
+                     'id': str(id_),
+                     'owner': owner.id,
+                     'ts': update.lastupdate.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],
+                     }
+        # Could return a subset of s to save bandwith
+        # ensures the owner hasn't changed since we pulled the acls above.
+        aql = f'''
+            FOR s in @@col
+                FILTER s.{_FLD_ARANGO_KEY} == @id
+                FILTER s.{_FLD_ACLS}.{_FLD_OWNER} == @owner
+                UPDATE s WITH {{
+                    {_FLD_ACL_UPDATE_TIME}: @ts,
+                    {_FLD_ACLS}: {{
+                        {_FLD_ADMIN}: REMOVE_VALUES(UNION_DISTINCT(
+                            s.{_FLD_ACLS}.{_FLD_ADMIN}, @admin), @remove),
+                        {_FLD_WRITE}: REMOVE_VALUES(UNION_DISTINCT(
+                            s.{_FLD_ACLS}.{_FLD_WRITE}, @write), @remove),
+                        {_FLD_READ}: REMOVE_VALUES(UNION_DISTINCT(
+                            s.{_FLD_ACLS}.{_FLD_READ}, @read), @remove)
+        '''
+        if update.public_read is not None:
+            aql += f''',
+                        {_FLD_PUBLIC_READ}: @pubread'''
+            bind_vars['pubread'] = update.public_read
+        aql += '''
+                        }
+                    } IN @@col
+                RETURN s
+            '''
+
+        try:
+            cur = self._db.aql.execute(aql, bind_vars=bind_vars, count=True)
+            if not cur.count():
+                # Assume cur.count() is never > 1 as we're filtering on _key.
+                # We already know the sample exists, and samples at this point can't be
+                # deleted, so just raise.
+                raise _OwnerChangedError(  # if this happens a lot make a retry loop.
+                    'The sample owner unexpectedly changed during the operation. Please retry. ' +
+                    'If this error occurs frequently, code changes may be necessary.')
+        except _arango.exceptions.AQLQueryExecuteError as e:  # this is a real pain to test
+            raise _SampleStorageError('Connection to database failed: ' + str(e)) from e
 
     def create_data_link(self, link: DataLink, update: bool = False) -> Optional[UUID]:
         '''

test/core/acls_test.py

@@ -216,22 +216,22 @@ def test_is_update():
 def test_is_update_fail():
     s = SampleACL(u('u'), dt(1))
 
-    is_update_fail(s, None, ValueError('update cannot be a value that evaluates to false'))
-    is_update_fail(
+    _is_update_fail(s, None, ValueError('update cannot be a value that evaluates to false'))
+    _is_update_fail(
         s, SampleACLDelta(dt(2), [u('a'), u('u')], [u('v')]),
         UnauthorizedError('ACLs for the sample owner u may not be modified by a delta update.'))
-    is_update_fail(
+    _is_update_fail(
         s, SampleACLDelta(dt(2), [u('a')], write=[u('v'), u('u')]),
         UnauthorizedError('ACLs for the sample owner u may not be modified by a delta update.'))
-    is_update_fail(
+    _is_update_fail(
         s, SampleACLDelta(dt(2), [u('a')], read=[u('v'), u('u')]),
         UnauthorizedError('ACLs for the sample owner u may not be modified by a delta update.'))
-    is_update_fail(
+    _is_update_fail(
         s, SampleACLDelta(dt(2), [u('a')], remove=[u('v'), u('u')]),
         UnauthorizedError('ACLs for the sample owner u may not be modified by a delta update.'))
 
 
-def is_update_fail(sample, delta, expected):
+def _is_update_fail(sample, delta, expected):
     with raises(Exception) as got:
         sample.is_update(delta)
     assert_exception_correct(got.value, expected)

test/core/storage/arango_sample_storage_test.py

@@ -6,13 +6,15 @@
 from core import test_utils
 from core.test_utils import assert_exception_correct
 from arango_controller import ArangoController
-from SampleService.core.acls import SampleACL
+from SampleService.core.acls import SampleACL, SampleACLDelta
 from SampleService.core.data_link import DataLink
 from SampleService.core.sample import SavedSample, SampleNode, SubSampleType, SampleNodeAddress
 from SampleService.core.sample import SampleAddress
-from SampleService.core.errors import MissingParameterError, NoSuchSampleError, ConcurrencyError
-from SampleService.core.errors import NoSuchSampleVersionError, DataLinkExistsError
-from SampleService.core.errors import TooManyDataLinksError, NoSuchLinkError, NoSuchSampleNodeError
+from SampleService.core.errors import (
+    MissingParameterError, NoSuchSampleError, ConcurrencyError, UnauthorizedError,
+    NoSuchSampleVersionError, DataLinkExistsError, TooManyDataLinksError, NoSuchLinkError,
+    NoSuchSampleNodeError
+)
 from SampleService.core.storage.arango_sample_storage import ArangoSampleStorage
 from SampleService.core.storage.errors import SampleStorageError, StorageInitError
 from SampleService.core.storage.errors import OwnerChangedError
@@ -1137,6 +1139,174 @@ def test_replace_sample_acls_fail_owner_changed(samplestorage):
     assert_exception_correct(got.value, OwnerChangedError())
 
 
+def test_update_sample_acls(samplestorage):
+    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(
+        dt(58),
+        [UserID('foo'), UserID('bar1')],
+        [UserID('baz1'), UserID('bat')],
+        [UserID('whoo1')],
+        public_read=True))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(58),
+        [UserID('foo'), UserID('bar1')],
+        [UserID('baz1'), UserID('bat')],
+        [UserID('whoo1')],
+        True)
+
+
+def test_update_sample_acls_noop(samplestorage):
+    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),
+        [UserID('foo'), UserID('bar')],
+        [UserID('baz'), UserID('bat')],
+        [UserID('whoo')],
+        True))
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(
+        dt(98),
+        [UserID('foo')],
+        [UserID('bat')],
+        [UserID('whoo')],
+        [UserID('nouser'), UserID('nouser2')],
+        public_read=True))
+
+    res = samplestorage.get_sample_acls(id_)
+    print(res)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(56),
+        [UserID('foo'), UserID('bar')],
+        [UserID('baz'), UserID('bat')],
+        [UserID('whoo')],
+        True)
+
+
+def test_update_sample_acls_with_remove_and_null_public(samplestorage):
+    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),
+        [UserID('foo'), UserID('bar')],
+        [UserID('baz'), UserID('bat')],
+        [UserID('whoo')],
+        True))
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(
+        dt(98),
+        [UserID('admin')],
+        [UserID('write'), UserID('write2')],
+        [UserID('read')],
+        [UserID('foo'), UserID('bat'), UserID('whoo'), UserID('notauser')]
+    ))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(98),
+        [UserID('bar'), UserID('admin')],
+        [UserID('baz'), UserID('write'), UserID('write2')],
+        [UserID('read')],
+        True)
+
+
+def test_update_sample_acls_with_false_public(samplestorage):
+    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),
+        [UserID('foo'), UserID('bar')],
+        [UserID('baz'), UserID('bat')],
+        [UserID('whoo')],
+        True))
+
+    samplestorage.update_sample_acls(id_, SampleACLDelta(dt(98), public_read=False))
+
+    res = samplestorage.get_sample_acls(id_)
+    assert res == SampleACL(
+        UserID('user'),
+        dt(98),
+        [UserID('bar'), UserID('foo')],
+        [UserID('baz'), UserID('bat')],
+        [UserID('whoo')],
+        False)
+
+
+def test_update_sample_acls_fail_bad_args(samplestorage):
+    id_ = uuid.uuid4()
+    s = SampleACLDelta(dt(2))
+
+    _update_sample_acls_fail(
+        samplestorage, None, s, ValueError('id_ cannot be a value that evaluates to false'))
+    _update_sample_acls_fail(
+        samplestorage, id_, None, ValueError('update cannot be a value that evaluates to false'))
+
+
+def test_update_sample_acls_fail_no_sample(samplestorage):
+    id_ = uuid.UUID('1234567890abcdef1234567890abcdef')
+    assert samplestorage.save_sample(
+        SavedSample(id_, UserID('user'), [TEST_NODE], dt(1), 'foo')) is True
+
+    _update_sample_acls_fail(
+        samplestorage, uuid.UUID('1234567890abcdef1234567890abcde1'), SampleACLDelta(dt(1)),
+        NoSuchSampleError('12345678-90ab-cdef-1234-567890abcde1'))
+
+
+def test_update_sample_acls_fail_alters_owner(samplestorage):
+    id_ = uuid.UUID('1234567890abcdef1234567890abcdef')
+    assert samplestorage.save_sample(
+        SavedSample(id_, UserID('us'), [TEST_NODE], dt(1), 'foo')) is True
+
+    err = UnauthorizedError('ACLs for the sample owner us may not be modified by a delta update.')
+
+    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), [UserID('us')]), err)
+    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), write=[UserID('us')]), err)
+    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), read=[UserID('us')]), err)
+    _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), remove=[UserID('us')]), err)
+
+
+def test_update_sample_acls_fail_owner_changed(samplestorage):
+    '''
+    This tests a race condition that could occur when the owner of a sample changes after
+    the sample ACLs are pulled from Arango to check against the sample delta to ensure the owner
+    is not altered by the delta.
+    '''
+    id_ = uuid.UUID('1234567890abcdef1234567890abcdef')
+    assert samplestorage.save_sample(
+        SavedSample(id_, UserID('user'), [TEST_NODE], dt(1), 'foo')) is True
+
+    with raises(Exception) as got:
+        samplestorage._update_sample_acls_pt2(
+            id_, SampleACLDelta(dt(2), [UserID('a')]), UserID('user2'))
+    assert_exception_correct(got.value, OwnerChangedError(
+        # we don't really ever expect this to happen, but just in case...
+        'The sample owner unexpectedly changed during the operation. Please retry. ' +
+        'If this error occurs frequently, code changes may be necessary.'))
+
+
+def _update_sample_acls_fail(samplestorage, id_, update, expected):
+    with raises(Exception) as got:
+        samplestorage.update_sample_acls(id_, update)
+    assert_exception_correct(got.value, expected)
+
+
 def test_create_and_get_data_link(samplestorage):
     id1 = uuid.UUID('1234567890abcdef1234567890abcdef')
     id2 = uuid.UUID('1234567890abcdef1234567890abcdee')