diff --git a/lib/SampleService/core/samples.py b/lib/SampleService/core/samples.py index 0449b1b0..1eb26391 100644 --- a/lib/SampleService/core/samples.py +++ b/lib/SampleService/core/samples.py @@ -278,7 +278,7 @@ def update_sample_acls( update.remove, update.public_read ) - self._storage.update_sample_acls(id_, update) + self._storage.update_sample_acls(id_, update, self._now()) if self._kafka: self._kafka.notify_sample_acl_change(id_) diff --git a/lib/SampleService/core/storage/arango_sample_storage.py b/lib/SampleService/core/storage/arango_sample_storage.py index e3e1f4f7..d9f06337 100644 --- a/lib/SampleService/core/storage/arango_sample_storage.py +++ b/lib/SampleService/core/storage/arango_sample_storage.py @@ -84,10 +84,12 @@ from SampleService.core.sample import SampleNode as _SampleNode, SubSampleType as _SubSampleType from SampleService.core.sample import SampleNodeAddress as _SampleNodeAddress from SampleService.core.sample import SampleAddress -from SampleService.core.arg_checkers import not_falsy as _not_falsy -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.arg_checkers import ( + not_falsy as _not_falsy, + not_falsy_in_iterable as _not_falsy_in_iterable, + check_string as _check_string, + check_timestamp as _check_timestamp, +) from SampleService.core.errors import ( ConcurrencyError as _ConcurrencyError, DataLinkExistsError as _DataLinkExistsError, @@ -796,12 +798,14 @@ 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 - def update_sample_acls(self, id_: UUID, update: SampleACLDelta) -> None: + def update_sample_acls( + self, id_: UUID, update: SampleACLDelta, update_time: datetime.datetime) -> None: ''' Update a sample's ACLs via a delta specification. :param id_: the sample ID. :param update: the update to apply to the ACLs. + :param update_time: the update time to save in the database. :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. @@ -809,15 +813,16 @@ def update_sample_acls(self, id_: UUID, update: SampleACLDelta) -> None: # 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') + _check_timestamp(update_time, 'update_time') 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) + self._update_sample_acls_pt2(id_, update, s.owner, update_time) - def _update_sample_acls_pt2(self, id_, update, owner): + def _update_sample_acls_pt2(self, id_, update, owner, update_time): # 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 @@ -831,7 +836,7 @@ def _update_sample_acls_pt2(self, id_, update, owner): bind_vars = {'@col': self._col_sample.name, 'id': str(id_), 'owner': owner.id, - 'ts': update.lastupdate.timestamp(), + '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], diff --git a/test/core/samples_test.py b/test/core/samples_test.py index 00785947..ce524c3a 100644 --- a/test/core/samples_test.py +++ b/test/core/samples_test.py @@ -884,7 +884,8 @@ def _update_sample_acls(user: UserID, public_read): [u('z'), u('a')], [u('b'), u('c')], [u('r'), u('q')], - public_read)) + public_read), + dt(6)) kafka.notify_sample_acl_change.assert_called_once_with( UUID('1234567890abcdef1234567890abcde0')) @@ -927,7 +928,8 @@ def test_update_sample_acls_as_admin_without_notifier(): [u('z'), u('a')], [u('b'), u('c')], [u('r'), u('q')], - None)) + None), + dt(6)) def test_update_sample_acls_fail_bad_input(): diff --git a/test/core/storage/arango_sample_storage_test.py b/test/core/storage/arango_sample_storage_test.py index 7c90e78e..376df9dd 100644 --- a/test/core/storage/arango_sample_storage_test.py +++ b/test/core/storage/arango_sample_storage_test.py @@ -1149,12 +1149,13 @@ def test_update_sample_acls(samplestorage): [UserID('foo'), UserID('bar1')], [UserID('baz1'), UserID('bat')], [UserID('whoo1')], - public_read=True)) + public_read=True), + dt(101)) res = samplestorage.get_sample_acls(id_) assert res == SampleACL( UserID('user'), - dt(58), + dt(101), [UserID('foo'), UserID('bar1')], [UserID('baz1'), UserID('bat')], [UserID('whoo1')], @@ -1180,7 +1181,8 @@ def test_update_sample_acls_noop(samplestorage): [UserID('bat')], [UserID('whoo')], [UserID('nouser'), UserID('nouser2')], - public_read=True)) + public_read=True), + dt(103)) res = samplestorage.get_sample_acls(id_) print(res) @@ -1211,13 +1213,13 @@ def test_update_sample_acls_with_remove_and_null_public(samplestorage): [UserID('admin')], [UserID('write'), UserID('write2')], [UserID('read')], - [UserID('foo'), UserID('bat'), UserID('whoo'), UserID('notauser')] - )) + [UserID('foo'), UserID('bat'), UserID('whoo'), UserID('notauser')]), + dt(102)) res = samplestorage.get_sample_acls(id_) assert res == SampleACL( UserID('user'), - dt(98), + dt(102), [UserID('bar'), UserID('admin')], [UserID('baz'), UserID('write'), UserID('write2')], [UserID('read')], @@ -1237,12 +1239,12 @@ def test_update_sample_acls_with_false_public(samplestorage): [UserID('whoo')], True)) - samplestorage.update_sample_acls(id_, SampleACLDelta(dt(98), public_read=False)) + samplestorage.update_sample_acls(id_, SampleACLDelta(dt(98), public_read=False), dt(89)) res = samplestorage.get_sample_acls(id_) assert res == SampleACL( UserID('user'), - dt(98), + dt(89), [UserID('bar'), UserID('foo')], [UserID('baz'), UserID('bat')], [UserID('whoo')], @@ -1252,11 +1254,16 @@ def test_update_sample_acls_with_false_public(samplestorage): def test_update_sample_acls_fail_bad_args(samplestorage): id_ = uuid.uuid4() s = SampleACLDelta(dt(2)) + t = dt(1) _update_sample_acls_fail( - samplestorage, None, s, ValueError('id_ cannot be a value that evaluates to false')) + samplestorage, None, s, t, 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')) + samplestorage, id_, None, t, ValueError('update cannot be a value that evaluates to false')) + _update_sample_acls_fail(samplestorage, id_, s, None, ValueError( + 'update_time cannot be a value that evaluates to false')) + _update_sample_acls_fail(samplestorage, id_, s, datetime.datetime.fromtimestamp(1), ValueError( + 'update_time cannot be a naive datetime')) def test_update_sample_acls_fail_no_sample(samplestorage): @@ -1265,7 +1272,7 @@ def test_update_sample_acls_fail_no_sample(samplestorage): SavedSample(id_, UserID('user'), [TEST_NODE], dt(1), 'foo')) is True _update_sample_acls_fail( - samplestorage, uuid.UUID('1234567890abcdef1234567890abcde1'), SampleACLDelta(dt(1)), + samplestorage, uuid.UUID('1234567890abcdef1234567890abcde1'), SampleACLDelta(dt(1)), dt(1), NoSuchSampleError('12345678-90ab-cdef-1234-567890abcde1')) @@ -1275,11 +1282,15 @@ def test_update_sample_acls_fail_alters_owner(samplestorage): 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.') + t = dt(1) - _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) + _update_sample_acls_fail(samplestorage, id_, SampleACLDelta(dt(1), [UserID('us')]), t, err) + _update_sample_acls_fail( + samplestorage, id_, SampleACLDelta(dt(1), write=[UserID('us')]), t, err) + _update_sample_acls_fail( + samplestorage, id_, SampleACLDelta(dt(1), read=[UserID('us')]), t, err) + _update_sample_acls_fail( + samplestorage, id_, SampleACLDelta(dt(1), remove=[UserID('us')]), t, err) def test_update_sample_acls_fail_owner_changed(samplestorage): @@ -1294,16 +1305,16 @@ def test_update_sample_acls_fail_owner_changed(samplestorage): with raises(Exception) as got: samplestorage._update_sample_acls_pt2( - id_, SampleACLDelta(dt(2), [UserID('a')]), UserID('user2')) + id_, SampleACLDelta(dt(2), [UserID('a')]), UserID('user2'), dt(1)) 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): +def _update_sample_acls_fail(samplestorage, id_, update, update_time, expected): with raises(Exception) as got: - samplestorage.update_sample_acls(id_, update) + samplestorage.update_sample_acls(id_, update, update_time) assert_exception_correct(got.value, expected)