diff --git a/lib/SampleService/core/storage/arango_sample_storage.py b/lib/SampleService/core/storage/arango_sample_storage.py index 721314d5..e3e1f4f7 100644 --- a/lib/SampleService/core/storage/arango_sample_storage.py +++ b/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]: ''' diff --git a/test/core/acls_test.py b/test/core/acls_test.py index 11a58d7b..9c55f0c3 100644 --- a/test/core/acls_test.py +++ b/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) diff --git a/test/core/storage/arango_sample_storage_test.py b/test/core/storage/arango_sample_storage_test.py index 4f0ab422..7c90e78e 100644 --- a/test/core/storage/arango_sample_storage_test.py +++ b/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')