diff --git a/securedrop_client/message_sync.py b/securedrop_client/message_sync.py index 274bdd03b..15f89e7fc 100644 --- a/securedrop_client/message_sync.py +++ b/securedrop_client/message_sync.py @@ -70,7 +70,6 @@ def __init__(self, api, home, is_qubes): def run(self, loop=True): while True: - logger.debug('Syncing messages.') submissions = storage.find_new_submissions(self.session) for db_submission in submissions: @@ -93,7 +92,7 @@ def run(self, loop=True): tb = traceback.format_exc() logger.critical("Exception while downloading submission!\n{}".format(tb)) - logger.debug('Completed message sync.') + logger.debug('Submissions synced') if not loop: break @@ -117,7 +116,6 @@ def __init__(self, api, home, is_qubes): def run(self, loop=True): while True: - logger.debug('Syncing replies.') replies = storage.find_new_replies(self.session) for db_reply in replies: @@ -143,7 +141,7 @@ def run(self, loop=True): tb = traceback.format_exc() logger.critical("Exception while downloading reply!\n{}".format(tb)) - logger.debug('Completed reply sync.') + logger.debug('Replies synced') if not loop: break diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index 8949a4803..508659943 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -121,7 +121,7 @@ def update_sources(remote_sources, local_sources, session, data_dir): # Removing the UUID from local_uuids ensures this record won't be # deleted at the end of this function. local_uuids.remove(source.uuid) - logger.info('Updated source {}'.format(source.uuid)) + logger.debug('Updated source {}'.format(source.uuid)) else: # A new source to be added to the database. ns = Source(uuid=source.uuid, @@ -133,7 +133,7 @@ def update_sources(remote_sources, local_sources, session, data_dir): last_updated=parse(source.last_updated), document_count=source.number_of_documents) session.add(ns) - logger.info('Added new source {}'.format(source.uuid)) + logger.debug('Added new source {}'.format(source.uuid)) # The uuids remaining in local_uuids do not exist on the remote server, so # delete the related records. @@ -142,7 +142,7 @@ def update_sources(remote_sources, local_sources, session, data_dir): delete_single_submission_or_reply_on_disk(document, data_dir) session.delete(deleted_source) - logger.info('Deleted source {}'.format(deleted_source.uuid)) + logger.debug('Deleted source {}'.format(deleted_source.uuid)) session.commit() @@ -157,9 +157,13 @@ def update_submissions(remote_submissions, local_submissions, session, data_dir) local_uuids = {submission.uuid for submission in local_submissions} for submission in remote_submissions: if submission.uuid in local_uuids: - # Update an existing record. local_submission = [s for s in local_submissions if s.uuid == submission.uuid][0] + # Update files on disk to match new filename. + if (local_submission.filename != submission.filename): + rename_file(data_dir, local_submission.filename, + submission.filename) + # Update an existing record. local_submission.filename = submission.filename local_submission.size = submission.size local_submission.is_read = submission.is_read @@ -168,7 +172,7 @@ def update_submissions(remote_submissions, local_submissions, session, data_dir) # Removing the UUID from local_uuids ensures this record won't be # deleted at the end of this function. local_uuids.remove(submission.uuid) - logger.info('Updated submission {}'.format(submission.uuid)) + logger.debug('Updated submission {}'.format(submission.uuid)) else: # A new submission to be added to the database. _, source_uuid = submission.source_url.rsplit('/', 1) @@ -178,7 +182,7 @@ def update_submissions(remote_submissions, local_submissions, session, data_dir) filename=submission.filename, download_url=submission.download_url) session.add(ns) - logger.info('Added new submission {}'.format(submission.uuid)) + logger.debug('Added new submission {}'.format(submission.uuid)) # The uuids remaining in local_uuids do not exist on the remote server, so # delete the related records. @@ -186,7 +190,7 @@ def update_submissions(remote_submissions, local_submissions, session, data_dir) if s.uuid in local_uuids]: delete_single_submission_or_reply_on_disk(deleted_submission, data_dir) session.delete(deleted_submission) - logger.info('Deleted submission {}'.format(deleted_submission.uuid)) + logger.debug('Deleted submission {}'.format(deleted_submission.uuid)) session.commit() @@ -204,8 +208,11 @@ def update_replies(remote_replies, local_replies, session, data_dir): local_uuids = {reply.uuid for reply in local_replies} for reply in remote_replies: if reply.uuid in local_uuids: - # Update an existing record. local_reply = [r for r in local_replies if r.uuid == reply.uuid][0] + # Update files on disk to match new filename. + if (local_reply.filename != reply.filename): + rename_file(data_dir, local_reply.filename, reply.filename) + # Update an existing record. user = find_or_create_user(reply.journalist_uuid, reply.journalist_username, session) local_reply.journalist_id = user.id @@ -213,7 +220,7 @@ def update_replies(remote_replies, local_replies, session, data_dir): local_reply.size = reply.size local_uuids.remove(reply.uuid) - logger.info('Updated reply {}'.format(reply.uuid)) + logger.debug('Updated reply {}'.format(reply.uuid)) else: # A new reply to be added to the database. source_uuid = reply.source_uuid @@ -226,14 +233,14 @@ def update_replies(remote_replies, local_replies, session, data_dir): filename=reply.filename, size=reply.size) session.add(nr) - logger.info('Added new reply {}'.format(reply.uuid)) + logger.debug('Added new reply {}'.format(reply.uuid)) # The uuids remaining in local_uuids do not exist on the remote server, so # delete the related records. for deleted_reply in [r for r in local_replies if r.uuid in local_uuids]: delete_single_submission_or_reply_on_disk(deleted_reply, data_dir) session.delete(deleted_reply) - logger.info('Deleted reply {}'.format(deleted_reply.uuid)) + logger.debug('Deleted reply {}'.format(deleted_reply.uuid)) session.commit() @@ -320,6 +327,16 @@ def delete_single_submission_or_reply_on_disk(obj_db, data_dir): 'File {} already deleted, skipping'.format(file_to_delete)) +def rename_file(data_dir: str, filename: str, new_filename: str): + filename, _ = os.path.splitext(filename) + new_filename, _ = os.path.splitext(new_filename) + try: + os.rename(os.path.join(data_dir, filename), + os.path.join(data_dir, new_filename)) + except OSError as e: + logger.debug('File could not be renamed: {}'.format(e)) + + def get_data(sdc_home: str, filename: str) -> str: filename, _ = os.path.splitext(filename) full_path = os.path.join(sdc_home, 'data', filename) @@ -328,5 +345,5 @@ def get_data(sdc_home: str, filename: str) -> str: msg = f.read() except FileNotFoundError: logger.debug('File not found: {}'.format(full_path)) - msg = '' + msg = '' return msg diff --git a/tests/test_storage.py b/tests/test_storage.py index 6015e4547..64b26c39b 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -9,7 +9,7 @@ from securedrop_client.storage import get_local_sources, get_local_submissions, get_local_replies, \ get_remote_data, update_local_storage, update_sources, update_submissions, update_replies, \ find_or_create_user, find_new_submissions, find_new_replies, mark_file_as_downloaded, \ - mark_reply_as_downloaded, delete_single_submission_or_reply_on_disk, get_data + mark_reply_as_downloaded, delete_single_submission_or_reply_on_disk, get_data, rename_file from securedrop_client import db from sdclientapi import Source, Submission, Reply @@ -38,9 +38,9 @@ def make_remote_submission(source_uuid): generate a valid URL. """ source_url = '/api/v1/sources/{}'.format(source_uuid) - return Submission(download_url='test', filename='test', is_read=False, - size=123, source_url=source_url, submission_url='test', - uuid=str(uuid.uuid4())) + return Submission(download_url='test', filename='submission.filename', + is_read=False, size=123, source_url=source_url, + submission_url='test', uuid=str(uuid.uuid4())) def make_remote_reply(source_uuid, journalist_uuid='testymctestface'): @@ -50,7 +50,7 @@ def make_remote_reply(source_uuid, journalist_uuid='testymctestface'): generate a valid URL. """ source_url = '/api/v1/sources/{}'.format(source_uuid) - return Reply(filename='test.filename', journalist_uuid=journalist_uuid, + return Reply(filename='reply.filename', journalist_uuid=journalist_uuid, journalist_username='test', is_deleted_by_source=False, reply_url='test', size=1234, source_url=source_url, uuid=str(uuid.uuid4())) @@ -198,6 +198,83 @@ def test_update_sources(homedir, mocker): assert mock_session.commit.call_count == 1 +def test_update_submissions_renames_file_on_disk(homedir, mocker): + """ + Check that: + + * Submissions are renamed on disk after sync. + """ + data_dir = os.path.join(homedir, 'data') + mock_session = mocker.MagicMock() + # Remote submission with new filename + server_filename = '1-spotted-potato-msg.gpg' + remote_submission = mocker.MagicMock() + remote_submission.uuid = 'test-uuid' + remote_submission.filename = server_filename + remote_submissions = [remote_submission] + # Local submission that needs to be updated + local_filename = '1-pericardial-surfacing-msg.gpg' + local_submission = mocker.MagicMock() + local_submission.uuid = 'test-uuid' + local_submission.filename = local_filename + local_submissions = [local_submission] + # Add submission file to test directory + local_filename_decrypted = '1-pericardial-surfacing-msg' + add_test_file_to_temp_dir(data_dir, local_filename_decrypted) + # There needs to be a corresponding local_source. + local_source = mocker.MagicMock() + local_source.uuid = 'test-source-uuid' + local_source.id = 123 + mock_session.query().filter_by.return_value = [local_source, ] + + update_submissions(remote_submissions, local_submissions, mock_session, + data_dir) + + updated_local_filename = '1-spotted-potato-msg' + assert local_submission.filename == remote_submission.filename + assert os.path.exists(os.path.join(data_dir, updated_local_filename)) + + +def test_update_replies_renames_file_on_disk(homedir, mocker): + """ + Check that: + + * Replies are renamed on disk after sync. + """ + data_dir = os.path.join(homedir, 'data') + mock_session = mocker.MagicMock() + # Remote replies with new filename + server_filename = '1-spotted-potato-reply.gpg' + remote_reply = mocker.MagicMock() + remote_reply.uuid = 'test-uuid' + remote_reply.filename = server_filename + remote_replies = [remote_reply] + # Local reply that needs to be updated + local_filename = '1-pericardial-surfacing-reply.gpg' + local_reply = mocker.MagicMock() + local_reply.uuid = 'test-uuid' + local_reply.filename = local_filename + local_replies = [local_reply] + # Add reply file to test directory + local_filename_decrypted = '1-pericardial-surfacing-reply' + add_test_file_to_temp_dir(data_dir, local_filename_decrypted) + # There needs to be a corresponding local_source and local_user + local_source = mocker.MagicMock() + local_source.uuid = 'test-source-uuid' + local_source.id = 123 + local_user = mocker.MagicMock() + local_user.username = 'jounalist designation' + local_user.id = 42 + mock_focu = mocker.MagicMock(return_value=local_user) + mocker.patch('securedrop_client.storage.find_or_create_user', mock_focu) + + update_replies(remote_replies, local_replies, mock_session, data_dir) + + updated_local_filename = '1-spotted-potato-reply' + assert local_reply.filename == remote_reply.filename + assert os.path.exists(os.path.join(data_dir, updated_local_filename)) + + def add_test_file_to_temp_dir(home_dir, filename): """ Add test file with the given filename to data dir. @@ -376,7 +453,12 @@ def test_update_submissions(homedir, mocker): * New submissions have an entry in the local database. * Local submission not returned by the remote server are deleted from the local database. + * `rename_file` is called if local data files need to be updated with new + filenames to match remote. Note: The only reason this should happen is if + there is a new journalist_designation that causes remote filenames to be + updated. """ + data_dir = os.path.join(homedir, 'data') mock_session = mocker.MagicMock() # Source object related to the submissions. source = mocker.MagicMock() @@ -384,39 +466,47 @@ def test_update_submissions(homedir, mocker): # Some submission objects from the API, one of which will exist in the # local database, the other will NOT exist in the local source database # (this will be added to the database) - submission_update = make_remote_submission(source.uuid) - submission_create = make_remote_submission(source.uuid) - remote_submissions = [submission_update, submission_create] + remote_sub_update = make_remote_submission(source.uuid) + remote_sub_create = make_remote_submission(source.uuid) + remote_submissions = [remote_sub_update, remote_sub_create] # Some local submission objects. One already exists in the API results # (this will be updated), one does NOT exist in the API results (this will # be deleted from the local database). - local_sub1 = mocker.MagicMock() - local_sub1.uuid = submission_update.uuid - local_sub2 = mocker.MagicMock() - local_sub2.uuid = str(uuid.uuid4()) - local_submissions = [local_sub1, local_sub2] + local_sub_update = mocker.MagicMock() + local_sub_update.uuid = remote_sub_update.uuid + local_sub_update.filename = "overwrite_this.filename" + local_sub_delete = mocker.MagicMock() + local_sub_delete.uuid = str(uuid.uuid4()) + local_sub_delete.filename = "local_sub_delete.filename" + local_submissions = [local_sub_update, local_sub_delete] # There needs to be a corresponding local_source. local_source = mocker.MagicMock() local_source.uuid = source.uuid - local_source.id = 666 # ;-) + local_source.id = 666 # };-) mock_session.query().filter_by.return_value = [local_source, ] + patch_rename_file = mocker.patch('securedrop_client.storage.rename_file') + update_submissions(remote_submissions, local_submissions, mock_session, - homedir) + data_dir) + # Check the expected local submission object has been updated with values # from the API. - assert local_sub1.filename == submission_update.filename - assert local_sub1.size == submission_update.size - assert local_sub1.is_read == submission_update.is_read + assert local_sub_update.filename == remote_sub_update.filename + assert local_sub_update.size == remote_sub_update.size + assert local_sub_update.is_read == remote_sub_update.is_read + # Check that rename_file is called if the local storage filenames need to + # be updated + assert patch_rename_file.called # Check the expected local source object has been created with values from # the API. assert mock_session.add.call_count == 1 new_sub = mock_session.add.call_args_list[0][0][0] - assert new_sub.uuid == submission_create.uuid + assert new_sub.uuid == remote_sub_create.uuid assert new_sub.source_id == local_source.id - assert new_sub.filename == submission_create.filename + assert new_sub.filename == remote_sub_create.filename # Ensure the record for the local source that is missing from the results # of the API is deleted. - mock_session.delete.assert_called_once_with(local_sub2) + mock_session.delete.assert_called_once_with(local_sub_delete) # Session is committed to database. assert mock_session.commit.call_count == 1 @@ -430,7 +520,12 @@ def test_update_replies(homedir, mocker): * Local replies not returned by the remote server are deleted from the local database. * References to journalist's usernames are correctly handled. + * `rename_file` is called if local data files need to be updated with new + filenames to match remote. Note: The only reason this should happen is if + there is a new journalist_designation that causes remote filenames to be + updated. """ + data_dir = os.path.join(homedir, 'data') mock_session = mocker.MagicMock() # Source object related to the submissions. source = mocker.MagicMock() @@ -438,50 +533,57 @@ def test_update_replies(homedir, mocker): # Some remote reply objects from the API, one of which will exist in the # local database, the other will NOT exist in the local database # (this will be added to the database) - reply_update = make_remote_reply(source.uuid) - reply_create = make_remote_reply(source.uuid, 'unknownuser') - remote_replies = [reply_update, reply_create] + remote_reply_update = make_remote_reply(source.uuid) + remote_reply_create = make_remote_reply(source.uuid, 'unknownuser') + remote_replies = [remote_reply_update, remote_reply_create] # Some local reply objects. One already exists in the API results # (this will be updated), one does NOT exist in the API results (this will # be deleted from the local database). - local_reply1 = mocker.MagicMock() - local_reply1.uuid = reply_update.uuid - local_reply1.journalist_uuid = str(uuid.uuid4()) - local_reply2 = mocker.MagicMock() - local_reply2.uuid = str(uuid.uuid4()) - local_reply2.journalist_uuid = str(uuid.uuid4()) - local_replies = [local_reply1, local_reply2] + local_reply_update = mocker.MagicMock() + local_reply_update.uuid = remote_reply_update.uuid + local_reply_update.filename = "overwrite_this.filename" + local_reply_update.journalist_uuid = str(uuid.uuid4()) + local_reply_delete = mocker.MagicMock() + local_reply_delete.uuid = str(uuid.uuid4()) + local_reply_delete.filename = "local_reply_delete.filename" + local_reply_delete.journalist_uuid = str(uuid.uuid4()) + local_replies = [local_reply_update, local_reply_delete] # There needs to be a corresponding local_source and local_user local_source = mocker.MagicMock() local_source.uuid = source.uuid - local_source.id = 666 # ;-) + local_source.id = 666 # };-) local_user = mocker.MagicMock() - local_user.username = reply_create.journalist_username + local_user.username = remote_reply_create.journalist_username local_user.id = 42 mock_session.query().filter_by.side_effect = [[local_source, ], [local_user, ], [local_user, ], ] mock_focu = mocker.MagicMock(return_value=local_user) mocker.patch('securedrop_client.storage.find_or_create_user', mock_focu) - update_replies(remote_replies, local_replies, mock_session, - homedir) + patch_rename_file = mocker.patch('securedrop_client.storage.rename_file') + + update_replies(remote_replies, local_replies, mock_session, data_dir) + # Check the expected local reply object has been updated with values # from the API. - assert local_reply1.journalist_id == local_user.id - assert local_reply1.filename == reply_update.filename - assert local_reply1.size == reply_update.size + assert local_reply_update.journalist_id == local_user.id + assert local_reply_update.filename == remote_reply_update.filename + assert local_reply_update.size == remote_reply_update.size + # Check that rename_file is called if the local storage filenames need to + # be updated + assert patch_rename_file.called # Check the expected local source object has been created with values from # the API. assert mock_session.add.call_count == 1 new_reply = mock_session.add.call_args_list[0][0][0] - assert new_reply.uuid == reply_create.uuid + assert new_reply.uuid == remote_reply_create.uuid assert new_reply.source_id == local_source.id assert new_reply.journalist_id == local_user.id - assert new_reply.size == reply_create.size - assert new_reply.filename == reply_create.filename + assert new_reply.size == remote_reply_create.size + assert new_reply.filename == remote_reply_create.filename # Ensure the record for the local source that is missing from the results # of the API is deleted. - mock_session.delete.assert_called_once_with(local_reply2) + mock_session.delete.assert_called_once_with(local_reply_delete) # Session is committed to database. assert mock_session.commit.call_count == 1 @@ -586,6 +688,35 @@ def test_delete_single_submission_or_reply_race_guard(homedir, mocker): mock_remove.call_count == 1 +def test_rename_file_does_not_throw(homedir): + """ + If file cannot be found then OSError is caught and logged. + """ + rename_file(os.path.join(homedir, 'data'), 'nonexistent.txt', 'bar.txt') + # quick coherence check + out = get_data(homedir, 'nonexistent.txt') + assert out == '' + + +def test_rename_file_success(homedir): + """ + If file was renamed successfully then we should be able to retrieve the + file's content using the new filename. + """ + data_dir = os.path.join(homedir, 'data') + orig_filename = 'foo.txt' + new_filename = 'bar.txt' + filename, _ = os.path.splitext(orig_filename) + contents = 'bar' + with open(os.path.join(data_dir, filename), 'w') as f: + f.write(contents) + + rename_file(data_dir, orig_filename, new_filename) + + out = get_data(homedir, new_filename) + assert out == contents + + def test_get_data_success(homedir): orig_filename = 'foo.txt' filename, _ = os.path.splitext(orig_filename) @@ -599,4 +730,4 @@ def test_get_data_success(homedir): def test_get_data_missing_data(homedir): orig_filename = 'foo.txt' out = get_data(homedir, orig_filename) - assert out == '' + assert out == ''