From 28bf9df16c6eb395b87d5b2e304a5739e6084050 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Mon, 27 Jan 2020 10:26:49 -0500 Subject: [PATCH 1/2] Improve download file handling Eliminate File.original_file column and hard links to downloaded files. Now as files are downloaded and decrypted, File.filename will be updated. Instead of downloading directly into the root of the data directory, create a directory therein for each downloadable object, named with its UUID, and process downloaded files in that directory. This should eliminate collisions if multiple sources upload documents with the same names. --- ...b657f2ee8a7_drop_file_original_filename.py | 71 ++++++ securedrop_client/api_jobs/downloads.py | 61 ++++-- securedrop_client/crypto.py | 8 +- securedrop_client/db.py | 50 ++++- securedrop_client/gui/widgets.py | 13 +- securedrop_client/logic.py | 73 ++----- securedrop_client/storage.py | 47 ++-- tests/factory.py | 1 - tests/gui/test_widgets.py | 4 +- tests/test_crypto.py | 31 ++- tests/test_logic.py | 205 ++++-------------- tests/test_storage.py | 116 ++++++---- 12 files changed, 351 insertions(+), 329 deletions(-) create mode 100644 alembic/versions/fb657f2ee8a7_drop_file_original_filename.py diff --git a/alembic/versions/fb657f2ee8a7_drop_file_original_filename.py b/alembic/versions/fb657f2ee8a7_drop_file_original_filename.py new file mode 100644 index 000000000..340707ea8 --- /dev/null +++ b/alembic/versions/fb657f2ee8a7_drop_file_original_filename.py @@ -0,0 +1,71 @@ +"""drop File.original_filename + +Revision ID: fb657f2ee8a7 +Revises: 86b01b6290da +Create Date: 2020-01-23 18:55:09.857324 + +""" +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = "fb657f2ee8a7" +down_revision = "86b01b6290da" +branch_labels = None +depends_on = None + + +def upgrade(): + conn = op.get_bind() + + op.rename_table("files", "original_files") + + conn.execute( + """ + CREATE TABLE files ( + id INTEGER NOT NULL, + uuid VARCHAR(36) NOT NULL, + filename VARCHAR(255) NOT NULL, + file_counter INTEGER NOT NULL, + size INTEGER NOT NULL, + download_url VARCHAR(255) NOT NULL, + is_downloaded BOOLEAN DEFAULT 0 NOT NULL, + is_read BOOLEAN DEFAULT 0 NOT NULL, + is_decrypted BOOLEAN, + source_id INTEGER NOT NULL, + CONSTRAINT pk_files PRIMARY KEY (id), + CONSTRAINT fk_files_source_id_sources FOREIGN KEY(source_id) REFERENCES sources (id), + CONSTRAINT uq_messages_source_id_file_counter UNIQUE (source_id, file_counter), + CONSTRAINT uq_files_uuid UNIQUE (uuid), + CONSTRAINT files_compare_is_downloaded_vs_is_decrypted CHECK (CASE WHEN is_downloaded = 0 THEN is_decrypted IS NULL ELSE 1 END), + CONSTRAINT ck_files_is_downloaded CHECK (is_downloaded IN (0, 1)), + CONSTRAINT ck_files_is_read CHECK (is_read IN (0, 1)), + CONSTRAINT ck_files_is_decrypted CHECK (is_decrypted IN (0, 1)) + ) + """ + ) + + conn.execute( + """ + INSERT INTO files + (id, uuid, filename, file_counter, size, download_url, is_downloaded, + is_decrypted, is_read, source_id) + SELECT id, uuid, filename, file_counter, size, download_url, is_downloaded, + is_decrypted, is_read, source_id + FROM original_files + """ + ) + + op.drop_table("original_files") + + +def downgrade(): + op.add_column( + "files", + sa.Column( + "original_filename", + sa.VARCHAR(length=255), + server_default=sa.text("''"), + nullable=False, + ), + ) diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py index 6b8998031..de656dbff 100644 --- a/securedrop_client/api_jobs/downloads.py +++ b/securedrop_client/api_jobs/downloads.py @@ -172,14 +172,14 @@ def call_api(self, api_client: API, session: Session) -> Any: self._decrypt(os.path.join(self.data_dir, db_object.filename), db_object, session) return db_object.uuid - self._download(api_client, db_object, session) - self._decrypt(os.path.join(self.data_dir, db_object.filename), db_object, session) + destination = self._download(api_client, db_object, session) + self._decrypt(destination, db_object, session) return db_object.uuid def _download(self, api: API, db_object: Union[File, Message, Reply], - session: Session) -> None: + session: Session) -> str: ''' Download the encrypted file. Check file integrity and move it to the data directory before marking it as downloaded. @@ -197,9 +197,12 @@ def _download(self, ) raise exception - shutil.move(download_path, os.path.join(self.data_dir, db_object.filename)) + destination = db_object.location(self.data_dir) + os.makedirs(os.path.dirname(destination), mode=0o700, exist_ok=True) + shutil.move(download_path, destination) mark_as_downloaded(type(db_object), db_object.uuid, session) - logger.info("File downloaded: {}".format(db_object.filename)) + logger.info("File downloaded to {}".format(destination)) + return destination except BaseError as e: logger.debug("Failed to download file: {}".format(db_object.filename)) raise e @@ -216,7 +219,9 @@ def _decrypt(self, mark_as_decrypted( type(db_object), db_object.uuid, session, original_filename=original_filename ) - logger.info("File decrypted: {}".format(os.path.basename(filepath))) + logger.info("File decrypted: {} (decrypted file: {})".format( + os.path.basename(filepath), original_filename) + ) except CryptoError as e: mark_as_decrypted(type(db_object), db_object.uuid, session, is_decrypted=False) logger.debug("Failed to decrypt file: {}".format(os.path.basename(filepath))) @@ -293,11 +298,21 @@ def call_decrypt(self, filepath: str, session: Session = None) -> str: ''' with NamedTemporaryFile('w+') as plaintext_file: self.gpg.decrypt_submission_or_reply(filepath, plaintext_file.name, is_doc=False) - set_message_or_reply_content( - model_type=Reply, - uuid=self.uuid, - session=session, - content=plaintext_file.read()) + try: + set_message_or_reply_content( + model_type=Reply, + uuid=self.uuid, + session=session, + content=plaintext_file.read()) + finally: + # clean up directory where decryption happened + try: + os.rmdir(os.path.dirname(filepath)) + except Exception as e: + logger.warning( + "Error deleting decryption directory of message %s: %s", self.uuid, e + ) + return "" @@ -339,12 +354,21 @@ def call_decrypt(self, filepath: str, session: Session = None) -> str: The return value is an empty string; messages have no original filename. ''' with NamedTemporaryFile('w+') as plaintext_file: - self.gpg.decrypt_submission_or_reply(filepath, plaintext_file.name, is_doc=False) - set_message_or_reply_content( - model_type=Message, - uuid=self.uuid, - session=session, - content=plaintext_file.read()) + try: + self.gpg.decrypt_submission_or_reply(filepath, plaintext_file.name, is_doc=False) + set_message_or_reply_content( + model_type=Message, + uuid=self.uuid, + session=session, + content=plaintext_file.read()) + finally: + # clean up directory where decryption happened + try: + os.rmdir(os.path.dirname(filepath)) + except Exception as e: + logger.warning( + "Error deleting decryption directory of message %s: %s", self.uuid, e + ) return "" @@ -385,8 +409,9 @@ def call_decrypt(self, filepath: str, session: Session = None) -> str: the file extensions, e.g. 1-impractical_thing-doc.gz.gpg -> 1-impractical_thing-doc ''' fn_no_ext, _ = os.path.splitext(os.path.splitext(os.path.basename(filepath))[0]) - plaintext_filepath = os.path.join(self.data_dir, fn_no_ext) + plaintext_filepath = os.path.join(os.path.dirname(filepath), fn_no_ext) original_filename = self.gpg.decrypt_submission_or_reply( filepath, plaintext_filepath, is_doc=True ) + logger.info("""Decrypted file "%s" to "%s" """, filepath, original_filename) return original_filename diff --git a/securedrop_client/crypto.py b/securedrop_client/crypto.py index d531ca464..68c2d56fc 100644 --- a/securedrop_client/crypto.py +++ b/securedrop_client/crypto.py @@ -125,8 +125,12 @@ def decrypt_submission_or_reply(self, # Store the plaintext in the file located at the specified plaintext_filepath if is_doc: - original_filename = read_gzip_header_filename(out.name) - with gzip.open(out.name, 'rb') as infile, open(plaintext_filepath, 'wb') as outfile: + original_filename = read_gzip_header_filename(out.name) or plaintext_filepath + decrypt_path = os.path.join( + os.path.dirname(filepath), + os.path.basename(original_filename) + ) + with gzip.open(out.name, 'rb') as infile, open(decrypt_path, 'wb') as outfile: shutil.copyfileobj(infile, outfile) else: shutil.copy(out.name, plaintext_filepath) diff --git a/securedrop_client/db.py b/securedrop_client/db.py index 05675f0a2..32f43cab5 100644 --- a/securedrop_client/db.py +++ b/securedrop_client/db.py @@ -123,6 +123,19 @@ def __str__(self) -> str: def __repr__(self) -> str: return ''.format(self.filename) + def location(self, data_dir: str) -> str: + ''' + Return the full path to the Message's file. + ''' + return os.path.abspath( + os.path.join( + data_dir, + self.__class__.__name__, + self.uuid, + self.filename, + ) + ) + class File(Base): @@ -135,15 +148,6 @@ class File(Base): uuid = Column(String(36), unique=True, nullable=False) filename = Column(String(255), nullable=False) - # Files from the SecureDrop journalist API are gzipped, then - # encrypted with GPG. The gzip header contains the original - # filename, which makes it easier for the client to open the file - # with the right application. We'll record that filename here - # after we've downloaded, decrypted and extracted the file. - # If the header does not contain the filename for some reason, - # this should be the same as filename. - original_filename = Column(String(255), nullable=False, server_default="") - file_counter = Column(Integer, nullable=False) size = Column(Integer, nullable=False) download_url = Column(String(255), nullable=False) @@ -179,13 +183,26 @@ def __str__(self) -> str: Return something that's a useful string representation of the file. """ if self.is_downloaded: - return "File: {}".format(self.original_filename) + return "File: {}".format(self.filename) else: return '' def __repr__(self) -> str: return ''.format(self.filename) + def location(self, data_dir: str) -> str: + ''' + Return the full path to the File's file. + ''' + return os.path.abspath( + os.path.join( + data_dir, + self.__class__.__name__, + self.uuid, + self.filename, + ) + ) + class Reply(Base): @@ -246,6 +263,19 @@ def __str__(self) -> str: def __repr__(self) -> str: return ''.format(self.filename) + def location(self, data_dir: str) -> str: + ''' + Return the full path to the Reply's file. + ''' + return os.path.abspath( + os.path.join( + data_dir, + self.__class__.__name__, + self.uuid, + self.filename, + ) + ) + class DraftReply(Base): diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 1c68194f2..28c362bb9 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1897,7 +1897,7 @@ def __init__( self.print_button.clicked.connect(self._on_print_clicked) # File name or default string - self.file_name = SecureQLabel(self.file.original_filename) + self.file_name = SecureQLabel(self.file.filename) self.file_name.setObjectName('file_name') self.file_name.installEventFilter(self) self.no_file_name = SecureQLabel('ENCRYPTED FILE ON SERVER') @@ -1952,7 +1952,7 @@ def _on_file_downloaded(self, file_uuid: str) -> None: if file_uuid == self.file.uuid: self.file = self.controller.get_file(self.file.uuid) if self.file.is_downloaded: - self.file_name.setText(self.file.original_filename) + self.file_name.setText(self.file.filename) self.download_button.hide() self.no_file_name.hide() self.export_button.show() @@ -1983,10 +1983,11 @@ def _on_export_clicked(self): """ Called when the export button is clicked. """ - if not self.controller.downloaded_file_exists(self.file.uuid): + if not self.controller.downloaded_file_exists(self.file): return - dialog = ExportDialog(self.controller, self.file.uuid, self.file.original_filename) + dialog = ExportDialog(self.controller, self.file.uuid, + self.file.filename) dialog.show() dialog.export() dialog.exec() @@ -1996,7 +1997,7 @@ def _on_print_clicked(self): """ Called when the print button is clicked. """ - if not self.controller.downloaded_file_exists(self.file.uuid): + if not self.controller.downloaded_file_exists(self.file): return dialog = PrintDialog(self.controller, self.file.uuid) @@ -2014,7 +2015,7 @@ def _on_left_click(self): if self.file.is_downloaded: # Open the already downloaded file. - self.controller.on_file_open(self.file.uuid) + self.controller.on_file_open(self.file) else: if self.controller.api: # Indicate in downloading state... but only after 0.3 seconds (i.e. diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 5368b0e64..4bc1ba876 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -187,7 +187,6 @@ def __init__(self, hostname: str, gui, session_maker: sessionmaker, self.gpg = GpgHelper(home, self.session_maker, proxy) self.export = Export() - self.export.export_completed.connect(self.cleanup_hardlinked_file) self.sync_flag = os.path.join(home, 'sync_flag') @@ -574,71 +573,36 @@ def on_reply_download_failure(self, exception: Exception) -> None: logger.debug('Failure due to checksum mismatch, retrying {}'.format(exception.uuid)) self._submit_download_job(exception.object_type, exception.uuid) - def downloaded_file_exists(self, file_uuid: str) -> bool: + def downloaded_file_exists(self, file: db.File) -> bool: ''' Check if the file specified by file_uuid exists. If it doesn't update the local db and GUI to show the file as not downloaded. ''' - file = self.get_file(file_uuid) - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(self.data_dir, fn_no_ext) - if not os.path.exists(filepath): + if not os.path.exists(file.location(self.data_dir)): + self.gui.update_error_status(_( + 'File does not exist in the data directory. Please try re-downloading.')) logger.debug('Cannot find {} in the data directory. File does not exist.'.format( - file.original_filename)) + file.filename)) storage.update_missing_files(self.data_dir, self.session) self.file_missing.emit(file.uuid) return False return True - def cleanup_hardlinked_file(cls, filepaths): - ''' - Once inter-vm communication is complete and we no longer need to keep file copies with the - original filenames, delete them. - ''' - for filepath in filepaths: - if os.path.exists(filepath): - os.remove(filepath) - - def get_path_to_file_with_original_name( - cls, file_dir: str, filename: str, filename_orig: str - ) -> str: - ''' - Create a hardlink with the original filename and return its path. - - Once a file is downloaded, it exists in the data directory with the same filename as the - server, except with the .gz.gpg stripped off. In order for the Display VM to know which - application to open the file in, we create a hard link to this file with the original file - name, including its extension. - ''' - - path_to_file_with_original_name = os.path.join(file_dir, filename_orig) - - if not os.path.exists(path_to_file_with_original_name): - fn_no_ext, dummy = os.path.splitext(os.path.splitext(filename)[0]) - filepath = os.path.join(file_dir, fn_no_ext) - os.link(filepath, path_to_file_with_original_name) - - return path_to_file_with_original_name - - def on_file_open(self, file_uuid: str) -> None: + def on_file_open(self, file: db.File) -> None: ''' Open the file specified by file_uuid. If the file is missing, update the db so that is_downloaded is set to False. ''' - file = self.get_file(file_uuid) - logger.info('Opening file "{}".'.format(file.original_filename)) + logger.info('Opening file "{}".'.format(file.location(self.data_dir))) - if not self.downloaded_file_exists(file.uuid): + if not self.downloaded_file_exists(file): return if not self.qubes: return - path_to_file_with_original_name = self.get_path_to_file_with_original_name( - self.data_dir, file.filename, file.original_filename) - command = "qvm-open-in-vm" - args = ['$dispvm:sd-viewer', path_to_file_with_original_name] + args = ['$dispvm:sd-viewer', file.location(self.data_dir)] process = QProcess(self) process.start(command, args) @@ -660,18 +624,15 @@ def export_file_to_usb_drive(self, file_uuid: str, passphrase: str) -> None: is_downloaded is set to False. ''' file = self.get_file(file_uuid) - logger.info('Exporting file {}'.format(file.original_filename)) + file_location = file.location(self.data_dir) - if not self.downloaded_file_exists(file.uuid): + if not self.downloaded_file_exists(file): return if not self.qubes: return - path_to_file_with_original_name = self.get_path_to_file_with_original_name( - self.data_dir, file.filename, file.original_filename) - - self.export.begin_usb_export.emit([path_to_file_with_original_name], passphrase) + self.export.begin_usb_export.emit([file_location], passphrase) def print_file(self, file_uuid: str) -> None: ''' @@ -679,18 +640,16 @@ def print_file(self, file_uuid: str) -> None: so that is_downloaded is set to False. ''' file = self.get_file(file_uuid) - logger.info('Printing file {}'.format(file.original_filename)) + file_location = file.location(self.data_dir) + logger.info('Printing file {}'.format(file_location)) - if not self.downloaded_file_exists(file.uuid): + if not self.downloaded_file_exists(file): return if not self.qubes: return - path_to_file_with_original_name = self.get_path_to_file_with_original_name( - self.data_dir, file.filename, file.original_filename) - - self.export.begin_print.emit([path_to_file_with_original_name]) + self.export.begin_print.emit([file_location]) def on_submission_download( self, diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index 172991bb0..5934a7e1b 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -208,12 +208,13 @@ def __update_submissions(model: Union[Type[File], Type[Message]], 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) + if ( + (local_submission.is_downloaded and not local_submission.is_decrypted) and + (local_submission.filename != submission.filename) + ): + rename_file(data_dir, local_submission, submission.filename) + 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 local_submission.download_url = submission.download_url @@ -259,11 +260,11 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply], 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. + rename_file(data_dir, local_reply, reply.filename) + local_reply.filename = reply.filename + user = find_or_create_user(reply.journalist_uuid, reply.journalist_username, session) local_reply.journalist_id = user.id - local_reply.filename = reply.filename local_reply.size = reply.size local_uuids.remove(reply.uuid) @@ -363,9 +364,7 @@ def update_missing_files(data_dir: str, session: Session) -> None: ''' files_that_have_been_downloaded = session.query(File).filter_by(is_downloaded=True).all() for file in files_that_have_been_downloaded: - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(data_dir, fn_no_ext) - if not os.path.exists(filepath): + if not os.path.exists(file.location(data_dir)): mark_as_not_downloaded(file.uuid, session) @@ -479,11 +478,8 @@ def mark_as_decrypted( db_obj = session.query(model_type).filter_by(uuid=uuid).one() db_obj.is_decrypted = is_decrypted - if model_type == File: - if original_filename: - db_obj.original_filename = original_filename - else: - db_obj.original_filename = db_obj.filename + if model_type == File and original_filename: + db_obj.filename = original_filename session.add(db_obj) session.commit() @@ -511,13 +507,6 @@ def delete_single_submission_or_reply_on_disk(obj_db: Union[File, Message, Reply Delete on disk a single submission or reply. """ files_to_delete = [] - try: - if obj_db.original_filename: - files_to_delete.append(os.path.join(data_dir, obj_db.original_filename)) - except AttributeError: - # only files have it - pass - filename_without_extensions = obj_db.filename.split('.')[0] file_glob_pattern = os.path.join( data_dir, @@ -532,12 +521,14 @@ def delete_single_submission_or_reply_on_disk(obj_db: Union[File, Message, Reply logging.info('File %s already deleted, skipping', file_to_delete) -def rename_file(data_dir: str, filename: str, new_filename: str) -> None: - filename, _ = os.path.splitext(filename) - new_filename, _ = os.path.splitext(new_filename) +def rename_file(data_dir: str, file: Union[Message, File, Reply], new_filename: str) -> None: try: - os.rename(os.path.join(data_dir, filename), - os.path.join(data_dir, new_filename)) + current_location = file.location(data_dir) + new_location = os.path.join( + os.path.dirname(current_location), + os.path.basename(new_filename) + ) + os.rename(current_location, new_location) except OSError as e: logger.debug('File could not be renamed: {}'.format(e)) diff --git a/tests/factory.py b/tests/factory.py index 2ef97f5b5..651787440 100644 --- a/tests/factory.py +++ b/tests/factory.py @@ -122,7 +122,6 @@ def File(**attrs): defaults = dict( uuid='file-uuid-{}'.format(FILE_COUNT), filename='{}-doc.gz.gpg'.format(FILE_COUNT), - original_filename='{}-doc.txt'.format(FILE_COUNT), size=123, download_url='http://wat.onion/abc', is_decrypted=True, diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index f822da574..9bffcfb90 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -1499,7 +1499,7 @@ def test_FileWidget_on_left_click_open(mocker, session, source): fw = FileWidget(file_.uuid, mock_controller, mocker.MagicMock(), mocker.MagicMock(), 0) fw._on_left_click() - fw.controller.on_file_open.assert_called_once_with(file_.uuid) + fw.controller.on_file_open.assert_called_once_with(file_) def test_FileWidget_set_button_animation_frame(mocker, session, source): @@ -1662,7 +1662,7 @@ def test_FileWidget__on_export_clicked(mocker, session, source): # Also assert that the dialog is initialized dialog = mocker.patch('securedrop_client.gui.widgets.ExportDialog') fw._on_export_clicked() - dialog.assert_called_once_with(controller, file.uuid, file.original_filename) + dialog.assert_called_once_with(controller, file.uuid, file.filename) def test_FileWidget__on_export_clicked_missing_file(mocker, session, source): diff --git a/tests/test_crypto.py b/tests/test_crypto.py index 2ca402dbe..21366110c 100644 --- a/tests/test_crypto.py +++ b/tests/test_crypto.py @@ -47,7 +47,7 @@ def test_gunzip_logic(homedir, config, mocker, session_maker): gpg._import(JOURNO_KEY) test_gzip = 'tests/files/test-doc.gz.gpg' - expected_output_filepath = 'tests/files/test-doc' + expected_output_filepath = 'tests/files/test-doc.txt' # mock_gpg = mocker.patch('subprocess.call', return_value=0) mock_unlink = mocker.patch('os.unlink') @@ -63,6 +63,35 @@ def test_gunzip_logic(homedir, config, mocker, session_maker): os.remove(expected_output_filepath) +def test_gzip_header_without_filename(homedir, config, mocker, session_maker): + """ + Test processing of a gzipped file without a filename in the header. + """ + gpg = GpgHelper(homedir, session_maker, is_qubes=False) + + gpg._import(PUB_KEY) + gpg._import(JOURNO_KEY) + + mocker.patch('os.unlink') + mocker.patch('gzip.open') + mocker.patch('shutil.copy') + mocker.patch('shutil.copyfileobj') + + # pretend the gzipped file header lacked the original filename + mock_read_gzip_header_filename = mocker.patch( + 'securedrop_client.crypto.read_gzip_header_filename' + ) + mock_read_gzip_header_filename.return_value = "" + + test_gzip = 'tests/files/test-doc.gz.gpg' + output_filename = 'test-doc' + expected_output_filename = 'tests/files/test-doc' + + original_filename = gpg.decrypt_submission_or_reply(test_gzip, output_filename, is_doc=True) + assert original_filename == output_filename + os.remove(expected_output_filename) + + def test_read_gzip_header_filename_with_bad_file(homedir): with tempfile.NamedTemporaryFile() as tf: tf.write(b'test') diff --git a/tests/test_logic.py b/tests/test_logic.py index 580b5d510..401e2264c 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -794,86 +794,29 @@ def test_Controller_on_file_downloaded_checksum_failure(homedir, config, mocker, mock_set_status.assert_not_called() -def test_get_path_to_file_with_original_name(mocker, homedir, session): - ''' - Test that the hardlink is created and returned. - ''' - co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir) - - with open(os.path.join(homedir, 'mock_filename'), 'w'): - pass - - path_to_file_with_original_name = co.get_path_to_file_with_original_name( - homedir, 'mock_filename', 'mock_filename_orig') - - assert path_to_file_with_original_name == os.path.join(homedir, 'mock_filename_orig') - - -def test_get_path_to_file_with_original_name_already_exists(mocker, homedir, session): - ''' - Test that the hardlink is returned if already exists. - ''' - co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir) - - with open(os.path.join(homedir, 'mock_filename_orig'), 'w'): - pass - - path_to_file_with_original_name = co.get_path_to_file_with_original_name( - homedir, 'mock_filename', 'mock_filename_orig') - - assert path_to_file_with_original_name == os.path.join(homedir, 'mock_filename_orig') - - -def test_cleanup_hardlinked_file(mocker, homedir): - ''' - Test that we delete all the files in the list. - ''' - co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir) - - filepath = os.path.join(homedir, 'testfile') - filepath2 = os.path.join(homedir, 'testfile2') - filepaths = [filepath, filepath2] - - for file in filepaths: - with open(file, 'w'): - pass - assert os.path.exists(file) - - co.cleanup_hardlinked_file(filepaths) - - assert not os.path.exists(filepath) - assert not os.path.exists(filepath2) - - def test_Controller_on_file_open(homedir, config, mocker, session, session_maker, source): """ If running on Qubes, a new QProcess with the expected command and args should be started when - the path to original_file does not exist. + the file does not exist. Using the `config` fixture to ensure the config is written to disk. """ co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.qubes = True file = factory.File(source=source['source']) - file.original_filename = 'original_filename.mock' session.add(file) session.commit() - mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) mock_subprocess = mocker.MagicMock() mock_process = mocker.MagicMock(return_value=mock_subprocess) mocker.patch('securedrop_client.logic.QProcess', mock_process) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath)) with open(filepath, 'w'): pass - co.on_file_open(file.uuid) + co.on_file_open(file) - co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_called_once_with( - os.path.join(homedir, 'data'), file.filename, file.original_filename) mock_process.assert_called_once_with(co) assert mock_subprocess.start.call_count == 1 @@ -885,21 +828,15 @@ def test_Controller_on_file_open_not_qubes(homedir, config, mocker, session, ses co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.qubes = False file = factory.File(source=source['source']) - file.original_filename = 'original_filename.mock' session.add(file) session.commit() - mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath)) with open(filepath, 'w'): pass - co.on_file_open(file.uuid) - - co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_not_called() + co.on_file_open(file) def test_Controller_on_file_open_when_orig_file_already_exists( @@ -914,29 +851,20 @@ def test_Controller_on_file_open_when_orig_file_already_exists( co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.qubes = True file = factory.File(source=source['source']) - file.original_filename = 'original_filename.mock' session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) mock_subprocess = mocker.MagicMock() mock_process = mocker.MagicMock(return_value=mock_subprocess) mocker.patch('securedrop_client.logic.QProcess', mock_process) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass - original_filepath = os.path.join(homedir, 'data', file.original_filename) - with open(original_filepath, 'w'): - pass - - co.on_file_open(file.uuid) + co.on_file_open(file) - co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_called_once_with( - os.path.join(homedir, 'data'), file.filename, file.original_filename) mock_process.assert_called_once_with(co) assert mock_subprocess.start.call_count == 1 @@ -950,25 +878,16 @@ def test_Controller_on_file_open_when_orig_file_already_exists_not_qubes( co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.qubes = False file = factory.File(source=source['source']) - file.original_filename = 'original_filename.mock' session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass - original_filepath = os.path.join(homedir, 'data', file.original_filename) - with open(original_filepath, 'w'): - pass - - co.on_file_open(file.uuid) - - co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_not_called() + co.on_file_open(file) def test_Controller_on_file_open_file_missing(mocker, homedir, session_maker, session, source): @@ -978,17 +897,16 @@ def test_Controller_on_file_open_file_missing(mocker, homedir, session_maker, se co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.sync_api = mocker.MagicMock() file = factory.File(source=source['source']) - file.original_filename = 'original_filename.mock' session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) debug_logger = mocker.patch('securedrop_client.logic.logger.debug') co.sync_api = mocker.MagicMock() - co.on_file_open(file.uuid) + co.on_file_open(file) log_msg = 'Cannot find {} in the data directory. File does not exist.'.format( - file.original_filename) + file.filename) debug_logger.assert_called_once_with(log_msg) co.sync_api.assert_not_called() @@ -1003,17 +921,16 @@ def test_Controller_on_file_open_file_missing_not_qubes( co.qubes = False co.sync_api = mocker.MagicMock() file = factory.File(source=source['source']) - file.original_filename = 'original_filename.mock' session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) debug_logger = mocker.patch('securedrop_client.logic.logger.debug') co.sync_api = mocker.MagicMock() - co.on_file_open(file.uuid) + co.on_file_open(file) log_msg = 'Cannot find {} in the data directory. File does not exist.'.format( - file.original_filename) + file.filename) debug_logger.assert_called_once_with(log_msg) co.sync_api.assert_not_called() @@ -1505,22 +1422,19 @@ def test_Controller_run_print_file(mocker, session, homedir): co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir) co.export = mocker.MagicMock() co.export.begin_print.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass co.print_file(file.uuid) co.export.begin_print.emit.call_count == 1 - co.get_path_to_file_with_original_name.assert_called_once_with( - os.path.join(homedir, 'data'), file.filename, file.original_filename) def test_Controller_run_print_file_not_qubes(mocker, session, homedir): @@ -1529,21 +1443,19 @@ def test_Controller_run_print_file_not_qubes(mocker, session, homedir): co.export = mocker.MagicMock() co.export.begin_print = mocker.MagicMock() co.export.begin_print.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass co.print_file(file.uuid) co.export.begin_print.emit.call_count == 0 - co.get_path_to_file_with_original_name.assert_not_called() def test_Controller_print_file_file_missing(homedir, mocker, session, session_maker): @@ -1553,7 +1465,7 @@ def test_Controller_print_file_file_missing(homedir, mocker, session, session_ma """ co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.sync_api = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) @@ -1563,7 +1475,7 @@ def test_Controller_print_file_file_missing(homedir, mocker, session, session_ma co.print_file(file.uuid) log_msg = 'Cannot find {} in the data directory. File does not exist.'.format( - file.original_filename) + file.filename) debug_logger.assert_called_once_with(log_msg) co.sync_api.assert_not_called() @@ -1578,7 +1490,7 @@ def test_Controller_print_file_file_missing_not_qubes( co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.qubes = False co.sync_api = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) @@ -1588,7 +1500,7 @@ def test_Controller_print_file_file_missing_not_qubes( co.print_file(file.uuid) log_msg = 'Cannot find {} in the data directory. File does not exist.'.format( - file.original_filename) + file.filename) debug_logger.assert_called_once_with(log_msg) co.sync_api.assert_not_called() @@ -1603,19 +1515,16 @@ def test_Controller_print_file_when_orig_file_already_exists( co.export = mocker.MagicMock() co.export.begin_print = mocker.MagicMock() co.export.begin_print.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) mocker.patch('os.path.exists', return_value=True) - co.get_path_to_file_with_original_name = mocker.MagicMock() co.print_file(file.uuid) co.export.begin_print.emit.call_count == 1 co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_called_once_with( - os.path.join(homedir, 'data'), file.filename, file.original_filename) def test_Controller_print_file_when_orig_file_already_exists_not_qubes( @@ -1629,27 +1538,20 @@ def test_Controller_print_file_when_orig_file_already_exists_not_qubes( co.export = mocker.MagicMock() co.export.begin_print = mocker.MagicMock() co.export.begin_print.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - mocker.patch('os.path.exists', return_value=True) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass - original_filepath = os.path.join(homedir, 'data', file.original_filename) - with open(original_filepath, 'w'): - pass - co.export_file_to_usb_drive(file.uuid, 'mock passphrase') co.export.begin_print.emit.call_count == 1 co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_not_called() def test_Controller_run_export_preflight_checks(homedir, mocker, session, source): @@ -1691,22 +1593,19 @@ def test_Controller_export_file_to_usb_drive(homedir, mocker, session): co.export = mocker.MagicMock() co.export.begin_usb_export = mocker.MagicMock() co.export.begin_usb_export.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass co.export_file_to_usb_drive(file.uuid, 'mock passphrase') co.export.begin_usb_export.emit.call_count == 1 - co.get_path_to_file_with_original_name.assert_called_once_with( - os.path.join(homedir, 'data'), file.filename, file.original_filename) def test_Controller_export_file_to_usb_drive_not_qubes(homedir, mocker, session): @@ -1718,14 +1617,13 @@ def test_Controller_export_file_to_usb_drive_not_qubes(homedir, mocker, session) co.export = mocker.MagicMock() co.export.begin_usb_export = mocker.MagicMock() co.export.begin_usb_export.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass @@ -1733,7 +1631,6 @@ def test_Controller_export_file_to_usb_drive_not_qubes(homedir, mocker, session) co.export.send_file_to_usb_device.assert_not_called() co.export.begin_usb_export.emit.call_count == 0 - co.get_path_to_file_with_original_name.assert_not_called() def test_Controller_export_file_to_usb_drive_file_missing(homedir, mocker, session, session_maker): @@ -1743,7 +1640,7 @@ def test_Controller_export_file_to_usb_drive_file_missing(homedir, mocker, sessi """ co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.sync_api = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) @@ -1753,7 +1650,7 @@ def test_Controller_export_file_to_usb_drive_file_missing(homedir, mocker, sessi co.export_file_to_usb_drive(file.uuid, 'mock passphrase') log_msg = 'Cannot find {} in the data directory. File does not exist.'.format( - file.original_filename) + file.filename) debug_logger.assert_called_once_with(log_msg) co.sync_api.assert_not_called() @@ -1768,7 +1665,7 @@ def test_Controller_export_file_to_usb_drive_file_missing_not_qubes( co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir) co.qubes = False co.sync_api = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) @@ -1778,7 +1675,7 @@ def test_Controller_export_file_to_usb_drive_file_missing_not_qubes( co.export_file_to_usb_drive(file.uuid, 'mock passphrase') log_msg = 'Cannot find {} in the data directory. File does not exist.'.format( - file.original_filename) + file.filename) debug_logger.assert_called_once_with(log_msg) co.sync_api.assert_not_called() @@ -1793,19 +1690,16 @@ def test_Controller_export_file_to_usb_drive_when_orig_file_already_exists( co.export = mocker.MagicMock() co.export.begin_usb_export = mocker.MagicMock() co.export.begin_usb_export.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) mocker.patch('os.path.exists', return_value=True) - co.get_path_to_file_with_original_name = mocker.MagicMock() co.export_file_to_usb_drive(file.uuid, 'mock passphrase') co.export.begin_usb_export.emit.call_count == 1 co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_called_once_with( - os.path.join(homedir, 'data'), file.filename, file.original_filename) def test_Controller_export_file_to_usb_drive_when_orig_file_already_exists_not_qubes( @@ -1819,33 +1713,26 @@ def test_Controller_export_file_to_usb_drive_when_orig_file_already_exists_not_q co.export = mocker.MagicMock() co.export.begin_usb_export = mocker.MagicMock() co.export.begin_usb_export.emit = mocker.MagicMock() - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file) - mocker.patch('os.path.exists', return_value=True) - co.get_path_to_file_with_original_name = mocker.MagicMock() - fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0]) - filepath = os.path.join(homedir, 'data', fn_no_ext) + filepath = file.location(co.data_dir) + os.makedirs(os.path.dirname(filepath), mode=0o700, exist_ok=True) with open(filepath, 'w'): pass - original_filepath = os.path.join(homedir, 'data', file.original_filename) - with open(original_filepath, 'w'): - pass - co.export_file_to_usb_drive(file.uuid, 'mock passphrase') co.export.begin_usb_export.emit.call_count == 1 co.get_file.assert_called_with(file.uuid) - co.get_path_to_file_with_original_name.assert_not_called() def test_get_file(mocker, session, homedir): co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir) storage = mocker.patch('securedrop_client.logic.storage') - file = factory.File(source=factory.Source(), original_filename='mock_filename') + file = factory.File(source=factory.Source()) session.add(file) session.commit() storage.get_file = mocker.MagicMock(return_value=file) diff --git a/tests/test_storage.py b/tests/test_storage.py index 125d6ca60..921aea923 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -250,32 +250,31 @@ def test_update_files_renames_file_on_disk(homedir, mocker): """ 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_submission = factory.File( + source=factory.Source(), + filename="1-unquestionable_complaint-msg.gpg", + is_downloaded=True, + is_decrypted=False, + ) 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, ] + add_test_file_to_temp_dir(data_dir, local_submission.location(data_dir)) + + mock_session.query().filter_by.return_value = [local_submission.source] + + # Remote submission with new filename + remote_submission = mocker.MagicMock() + remote_submission.uuid = local_submission.uuid + remote_submission.filename = '1-spotted-potato-msg.gpg' + remote_submissions = [remote_submission] update_files(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)) + assert os.path.exists(local_submission.location(data_dir)) def test_update_replies_renames_file_on_disk(homedir, mocker): @@ -292,30 +291,41 @@ def test_update_replies_renames_file_on_disk(homedir, mocker): 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_reply.location = lambda data_dir: os.path.join( + data_dir, "Reply", local_reply.uuid, local_reply.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) + add_test_file_to_temp_dir( + data_dir, + os.path.join( + os.path.dirname(local_reply.location(data_dir)), + os.path.splitext(local_reply.filename)[0] + ) + ) + # 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.username = 'journalist 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): @@ -324,6 +334,7 @@ def add_test_file_to_temp_dir(home_dir, filename): """ dest = os.path.join(home_dir, filename) + os.makedirs(os.path.dirname(dest), mode=0o700, exist_ok=True) with open(dest, 'w') as f: f.write('I am test content for tests') @@ -351,7 +362,6 @@ def test_update_submissions_deletes_files_associated_with_the_submission( local_submission = mocker.MagicMock() local_submission.uuid = 'test-uuid' local_submission.filename = server_filename - local_submission.original_filename = "localsub.txt" abs_server_filename = add_test_file_to_temp_dir( homedir, server_filename) abs_local_filename = add_test_file_to_temp_dir( @@ -397,7 +407,6 @@ def test_update_replies_deletes_files_associated_with_the_reply( local_reply = mocker.MagicMock() local_reply.uuid = 'test-uuid' local_reply.filename = server_filename - local_reply.original_filename = "" abs_server_filename = add_test_file_to_temp_dir( homedir, server_filename) abs_local_filename = add_test_file_to_temp_dir( @@ -518,12 +527,12 @@ def test_update_files(homedir, mocker): # be deleted from the local database). local_sub_update = mocker.MagicMock() local_sub_update.uuid = remote_sub_update.uuid + local_sub_update.is_downloaded = True + local_sub_update.is_decrypted = False local_sub_update.filename = "overwrite_this.filename" - local_sub_update.original_filename = "overwrite_this.filename.txt" local_sub_delete = mocker.MagicMock() local_sub_delete.uuid = str(uuid.uuid4()) local_sub_delete.filename = "local_sub_delete.filename" - local_sub_delete.original_filename = "local_sub_delete.filename.txt" local_submissions = [local_sub_update, local_sub_delete] # There needs to be a corresponding local_source. local_source = mocker.MagicMock() @@ -586,11 +595,11 @@ def test_update_messages(homedir, mocker): local_message_update = mocker.MagicMock() local_message_update.uuid = remote_message_update.uuid local_message_update.filename = "overwrite_this.filename" - local_message_update.original_filename = "" + local_message_update.is_downloaded = True + local_message_update.is_decrypted = False local_message_delete = mocker.MagicMock() local_message_delete.uuid = str(uuid.uuid4()) local_message_delete.filename = "local_message_delete.filename" - local_message_delete.original_filename = "" local_messages = [local_message_update, local_message_delete] # There needs to be a corresponding local_source and local_user local_source = mocker.MagicMock() @@ -660,12 +669,10 @@ def test_update_replies(homedir, mocker): local_reply_update = mocker.MagicMock() local_reply_update.uuid = remote_reply_update.uuid local_reply_update.filename = "overwrite_this.filename" - local_reply_update.original_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.original_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 @@ -1006,7 +1013,6 @@ def test_delete_single_submission_or_reply_race_guard(homedir, mocker): test_obj = mocker.MagicMock() test_obj.filename = '1-dissolved-steak-msg.gpg' - test_obj.original_filename = 'gelatinous-death.txt' add_test_file_to_temp_dir(homedir, test_obj.filename) mock_remove = mocker.patch('os.remove', side_effect=FileNotFoundError) @@ -1019,12 +1025,22 @@ def test_rename_file_does_not_throw(homedir): """ If file cannot be found then OSError is caught and logged. """ - original_file = 'foo.txt' - new_file = 'bar.txt' - rename_file(os.path.join(homedir, 'data'), original_file, new_file) + data_dir = os.path.join(homedir, 'data') - assert not os.path.exists(os.path.join(homedir, 'data', original_file)) - assert not os.path.exists(os.path.join(homedir, 'data', new_file)) + original_file = factory.File( + source=factory.Source(), + filename="1-unquestionable_complaint-msg.gpg", + is_downloaded=True, + is_decrypted=False, + ) + new_filename = "1-demonstrable_reciprocity-msg.gpg" + rename_file(data_dir, original_file, new_filename) + + assert not os.path.exists(original_file.location(data_dir)) + + original_file.filename = new_filename + + assert not os.path.exists(original_file.location(data_dir)) def test_rename_file_success(homedir): @@ -1033,17 +1049,27 @@ def test_rename_file_success(homedir): 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) - trunc_new_filename, _ = os.path.splitext(new_filename) + original_file = factory.File( + source=factory.Source(), + filename="1-unquestionable_complaint-msg.gpg", + is_downloaded=True, + is_decrypted=False, + ) contents = 'bar' - with open(os.path.join(data_dir, filename), 'w') as f: + os.makedirs(os.path.dirname(original_file.location(data_dir))) + with open(original_file.location(data_dir), 'w') as f: f.write(contents) - rename_file(data_dir, orig_filename, new_filename) + new_filename = "1-demonstrable_reciprocity-msg.gpg" + rename_file(os.path.join(homedir, 'data'), original_file, new_filename) + + assert not os.path.exists(original_file.location(data_dir)) + + original_file.filename = new_filename + + assert os.path.exists(original_file.location(data_dir)) - with open(os.path.join(homedir, 'data', trunc_new_filename)) as f: + with open(original_file.location(data_dir)) as f: out = f.read() assert out == contents From d6c832002345c51acdc40620f743093fecf83fba Mon Sep 17 00:00:00 2001 From: John Hensley Date: Thu, 30 Jan 2020 18:55:13 -0500 Subject: [PATCH 2/2] Use journalist designations, not UUIDs, in data directory Base directory and file names in the data directory on the journalist designation of their sources, plus file counters and extensions, instead of UUIDs. Remove handling of changes to source journalist designations on the server; local files will no longer be renamed. The ability to change journalist designations will soon be removed from SecureDrop core, so this will not be needed. --- securedrop_client/db.py | 22 +++-- securedrop_client/storage.py | 24 ----- tests/test_storage.py | 183 +++-------------------------------- 3 files changed, 26 insertions(+), 203 deletions(-) diff --git a/securedrop_client/db.py b/securedrop_client/db.py index 32f43cab5..72fc8e9cf 100644 --- a/securedrop_client/db.py +++ b/securedrop_client/db.py @@ -63,6 +63,12 @@ def collection(self) -> List: datetime.datetime(datetime.MINYEAR, 1, 1)))) return collection + @property + def journalist_filename(self) -> str: + valid_chars = 'abcdefghijklmnopqrstuvwxyz1234567890-_' + return ''.join([c for c in self.journalist_designation.lower().replace( + ' ', '_') if c in valid_chars]) + class Message(Base): @@ -130,9 +136,8 @@ def location(self, data_dir: str) -> str: return os.path.abspath( os.path.join( data_dir, - self.__class__.__name__, - self.uuid, - self.filename, + self.source.journalist_filename, + os.path.splitext(self.filename)[0] + '.txt' ) ) @@ -197,9 +202,9 @@ def location(self, data_dir: str) -> str: return os.path.abspath( os.path.join( data_dir, - self.__class__.__name__, - self.uuid, - self.filename, + self.source.journalist_filename, + '{}-{}-doc'.format(self.file_counter, self.source.journalist_filename), + self.filename ) ) @@ -270,9 +275,8 @@ def location(self, data_dir: str) -> str: return os.path.abspath( os.path.join( data_dir, - self.__class__.__name__, - self.uuid, - self.filename, + self.source.journalist_filename, + os.path.splitext(self.filename)[0] + '.txt' ) ) diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index 5934a7e1b..b9b9d1264 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -207,14 +207,6 @@ def __update_submissions(model: Union[Type[File], Type[Message]], 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.is_downloaded and not local_submission.is_decrypted) and - (local_submission.filename != submission.filename) - ): - rename_file(data_dir, local_submission, submission.filename) - local_submission.filename = submission.filename - local_submission.size = submission.size local_submission.is_read = submission.is_read local_submission.download_url = submission.download_url @@ -258,10 +250,6 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply], for reply in remote_replies: if reply.uuid in local_uuids: 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, reply.filename) - local_reply.filename = reply.filename user = find_or_create_user(reply.journalist_uuid, reply.journalist_username, session) local_reply.journalist_id = user.id @@ -521,18 +509,6 @@ def delete_single_submission_or_reply_on_disk(obj_db: Union[File, Message, Reply logging.info('File %s already deleted, skipping', file_to_delete) -def rename_file(data_dir: str, file: Union[Message, File, Reply], new_filename: str) -> None: - try: - current_location = file.location(data_dir) - new_location = os.path.join( - os.path.dirname(current_location), - os.path.basename(new_filename) - ) - os.rename(current_location, new_location) - except OSError as e: - logger.debug('File could not be renamed: {}'.format(e)) - - def source_exists(session: Session, source_uuid: str) -> bool: try: session.query(Source).filter_by(uuid=source_uuid).one() diff --git a/tests/test_storage.py b/tests/test_storage.py index 921aea923..c2cc58785 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -14,7 +14,7 @@ from securedrop_client.storage import get_local_sources, get_local_messages, get_local_replies, \ get_remote_data, update_local_storage, update_sources, update_files, update_messages, \ update_replies, find_or_create_user, find_new_messages, find_new_replies, \ - delete_single_submission_or_reply_on_disk, rename_file, get_local_files, find_new_files, \ + delete_single_submission_or_reply_on_disk, get_local_files, find_new_files, \ source_exists, set_message_or_reply_content, mark_as_downloaded, mark_as_decrypted, get_file, \ get_message, get_reply, update_and_get_user, update_missing_files, mark_as_not_downloaded, \ mark_all_pending_drafts_as_failed, delete_local_source_by_uuid @@ -242,92 +242,6 @@ def test_update_sources(homedir, mocker): assert mock_session.commit.call_count == 1 -def test_update_files_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() - - # Local submission that needs to be updated - local_submission = factory.File( - source=factory.Source(), - filename="1-unquestionable_complaint-msg.gpg", - is_downloaded=True, - is_decrypted=False, - ) - local_submissions = [local_submission] - - # Add submission file to test directory - add_test_file_to_temp_dir(data_dir, local_submission.location(data_dir)) - - mock_session.query().filter_by.return_value = [local_submission.source] - - # Remote submission with new filename - remote_submission = mocker.MagicMock() - remote_submission.uuid = local_submission.uuid - remote_submission.filename = '1-spotted-potato-msg.gpg' - remote_submissions = [remote_submission] - - update_files(remote_submissions, local_submissions, mock_session, data_dir) - - assert local_submission.filename == remote_submission.filename - assert os.path.exists(local_submission.location(data_dir)) - - -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_reply.location = lambda data_dir: os.path.join( - data_dir, "Reply", local_reply.uuid, local_reply.filename - ) - local_replies = [local_reply] - - # Add reply file to test directory - add_test_file_to_temp_dir( - data_dir, - os.path.join( - os.path.dirname(local_reply.location(data_dir)), - os.path.splitext(local_reply.filename)[0] - ) - ) - - # 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 = 'journalist 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) - - assert local_reply.filename == remote_reply.filename - - def add_test_file_to_temp_dir(home_dir, filename): """ Add test file with the given filename to data dir. @@ -506,10 +420,6 @@ def test_update_files(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() @@ -529,7 +439,8 @@ def test_update_files(homedir, mocker): local_sub_update.uuid = remote_sub_update.uuid local_sub_update.is_downloaded = True local_sub_update.is_decrypted = False - local_sub_update.filename = "overwrite_this.filename" + local_filename = "originalsubmissionname.txt" + local_sub_update.filename = local_filename local_sub_delete = mocker.MagicMock() local_sub_delete.uuid = str(uuid.uuid4()) local_sub_delete.filename = "local_sub_delete.filename" @@ -539,18 +450,15 @@ def test_update_files(homedir, mocker): local_source.uuid = source.uuid local_source.id = 666 # };-) mock_session.query().filter_by.return_value = [local_source, ] - patch_rename_file = mocker.patch('securedrop_client.storage.rename_file') update_files(remote_submissions, local_submissions, mock_session, data_dir) # Check the expected local submission object has been updated with values # from the API. - assert local_sub_update.filename == remote_sub_update.filename + assert local_sub_update.filename == local_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 @@ -573,10 +481,6 @@ def test_update_messages(homedir, mocker): * New messages have an entry in the local database. * Local messages 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() @@ -594,7 +498,8 @@ def test_update_messages(homedir, mocker): # be deleted from the local database). local_message_update = mocker.MagicMock() local_message_update.uuid = remote_message_update.uuid - local_message_update.filename = "overwrite_this.filename" + local_filename = "originalsubmissionname.txt" + local_message_update.filename = local_filename local_message_update.is_downloaded = True local_message_update.is_decrypted = False local_message_delete = mocker.MagicMock() @@ -612,17 +517,14 @@ def test_update_messages(homedir, mocker): [local_user, ], ] mock_focu = mocker.MagicMock(return_value=local_user) mocker.patch('securedrop_client.storage.find_or_create_user', mock_focu) - patch_rename_file = mocker.patch('securedrop_client.storage.rename_file') update_messages(remote_messages, local_messages, mock_session, data_dir) # Check the expected local message object has been updated with values # from the API. - assert local_message_update.filename == remote_message_update.filename + assert local_message_update.filename == local_filename assert local_message_update.size == remote_message_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 @@ -647,10 +549,6 @@ 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() @@ -668,7 +566,8 @@ def test_update_replies(homedir, mocker): # be deleted from the local database). local_reply_update = mocker.MagicMock() local_reply_update.uuid = remote_reply_update.uuid - local_reply_update.filename = "overwrite_this.filename" + local_filename = "originalsubmissionname.txt" + local_reply_update.filename = local_filename local_reply_update.journalist_uuid = str(uuid.uuid4()) local_reply_delete = mocker.MagicMock() local_reply_delete.uuid = str(uuid.uuid4()) @@ -686,18 +585,15 @@ def test_update_replies(homedir, mocker): NoResultFound()] mock_focu = mocker.MagicMock(return_value=local_user) mocker.patch('securedrop_client.storage.find_or_create_user', mock_focu) - 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_reply_update.journalist_id == local_user.id - assert local_reply_update.filename == remote_reply_update.filename + assert local_reply_update.filename == local_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 @@ -1021,59 +917,6 @@ 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. - """ - data_dir = os.path.join(homedir, 'data') - - original_file = factory.File( - source=factory.Source(), - filename="1-unquestionable_complaint-msg.gpg", - is_downloaded=True, - is_decrypted=False, - ) - new_filename = "1-demonstrable_reciprocity-msg.gpg" - rename_file(data_dir, original_file, new_filename) - - assert not os.path.exists(original_file.location(data_dir)) - - original_file.filename = new_filename - - assert not os.path.exists(original_file.location(data_dir)) - - -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') - original_file = factory.File( - source=factory.Source(), - filename="1-unquestionable_complaint-msg.gpg", - is_downloaded=True, - is_decrypted=False, - ) - contents = 'bar' - os.makedirs(os.path.dirname(original_file.location(data_dir))) - with open(original_file.location(data_dir), 'w') as f: - f.write(contents) - - new_filename = "1-demonstrable_reciprocity-msg.gpg" - rename_file(os.path.join(homedir, 'data'), original_file, new_filename) - - assert not os.path.exists(original_file.location(data_dir)) - - original_file.filename = new_filename - - assert os.path.exists(original_file.location(data_dir)) - - with open(original_file.location(data_dir)) as f: - out = f.read() - assert out == contents - - def test_source_exists_true(homedir, mocker): ''' Check that method returns True if a source is return from the query.