Merge pull request #566 from freedomofpress/check-file

Handle missing files during an export and open

securedrop_client/export.py

@@ -256,4 +256,5 @@ def send_file_to_usb_device(self, filepaths: List[str], passphrase: str) -> None
                 logger.debug('Export successful')
                 self.export_usb_call_success.emit(filepaths)
             except ExportError as e:
-                self.export_usb_call_failure.emit(e.status)
+                logger.error(e)
+                self.export_usb_call_failure.emit(filepaths)

securedrop_client/gui/widgets.py

@@ -1799,6 +1799,10 @@ def _on_export_clicked(self):
         """
         Called when the export button is clicked.
         """
+        if not self.controller.downloaded_file_exists(self.file.uuid):
+            self.controller.sync_api()
+            return
+
         dialog = ExportDialog(self.controller, self.file.uuid)
         # The underlying function of the `export` method makes a blocking call that can potentially
         # take a long time to run (if the Export VM is not already running and needs to start, this

securedrop_client/logic.py

@@ -21,9 +21,8 @@
 import logging
 import os
 import sdclientapi
-import threading
 import uuid
-from typing import Dict, Tuple, Union, Any, Type  # noqa: F401
+from typing import Dict, Tuple, Union, Any, List, Type  # noqa: F401
 
 from gettext import gettext as _
 from PyQt5.QtCore import QObject, QThread, pyqtSignal, QTimer, QProcess, Qt
@@ -181,6 +180,8 @@ def __init__(self, hostname: str, gui, session_maker: sessionmaker,
         self.gpg = GpgHelper(home, self.session_maker, proxy)
 
         self.export = Export()
+        self.export.export_usb_call_success.connect(self.on_export_usb_call_success)
+        self.export.export_usb_call_failure.connect(self.on_export_usb_call_failure)
 
         self.sync_flag = os.path.join(home, 'sync_flag')
 
@@ -397,21 +398,25 @@ def last_sync(self):
 
     def on_sync_success(self, result) -> None:
         """
-        Called when syncronisation of data via the API succeeds
+        Called when syncronisation of data via the API succeeds.
+
+            * Update db with new metadata
+            * Set last sync flag
+            * Import keys into keyring
+            * Display the last sync time and updated list of sources in GUI
+            * Download new messages and replies
+            * Update missing files so that they can be re-downloaded
         """
-        # Update db with new metadata
         remote_sources, remote_submissions, remote_replies = result
         storage.update_local_storage(self.session,
                                      remote_sources,
                                      remote_submissions,
                                      remote_replies,
                                      self.data_dir)
 
-        # Set last sync flag
         with open(self.sync_flag, 'w') as f:
             f.write(arrow.now().format())
 
-        # Import keys into keyring
         for source in remote_sources:
             if source.key and source.key.get('type', None) == 'PGP':
                 pub_key = source.key.get('public', None)
@@ -423,6 +428,7 @@ def on_sync_success(self, result) -> None:
                 except CryptoError:
                     logger.warning('Failed to import key for source {}'.format(source.uuid))
 
+        storage.update_missing_files(self.data_dir, self.session)
         self.update_sources()
         self.download_new_messages()
         self.download_new_replies()
@@ -573,58 +579,112 @@ 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:
+        '''
+        Check if the file specified by file_uuid exists. If it doesn't sync the api so that any
+        missing files, including this one, are updated to be re-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):
+            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))
+            return False
+        return True
+
     def on_file_open(self, file_uuid: str) -> None:
-        """
-        Open the already downloaded file associated with the message (which is a `File`).
-        """
-        # Once downloaded, submissions are stored in the data directory
-        # with the same filename as the server, except with the .gz.gpg
-        # stripped off.
+        '''
+        Open the file specified by file_uuid.
+
+        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.
+
+        If the file is missing, update the db so that is_downloaded is set to False.
+        '''
         file = self.get_file(file_uuid)
-        fn_no_ext, _ = os.path.splitext(os.path.splitext(file.filename)[0])
-        submission_filepath = os.path.join(self.data_dir, fn_no_ext)
-        original_filepath = os.path.join(self.data_dir, file.original_filename)
-
-        if os.path.exists(original_filepath):
-            os.remove(original_filepath)
-        os.link(submission_filepath, original_filepath)
-        if self.proxy or self.qubes:
-            # Running on Qubes.
-            command = "qvm-open-in-vm"
-            args = ['@dispvm:sd-svs-disp', original_filepath]
-
-            # QProcess (Qt) or Python's subprocess? Who cares? They do the
-            # same thing. :-)
-            process = QProcess(self)
-            process.start(command, args)
-        else:  # pragma: no cover
-            # Non Qubes OS. Just log the event for now.
-            logger.info('Opening file "{}".'.format(original_filepath))
+        logger.info('Opening file "{}".'.format(file.original_filename))
+
+        if not self.downloaded_file_exists(file.uuid):
+            self.sync_api()
+            return
+
+        path_to_file_with_original_name = os.path.join(self.data_dir, file.original_filename)
+
+        if not os.path.exists(path_to_file_with_original_name):
+            fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0])
+            filepath = os.path.join(self.data_dir, fn_no_ext)
+            os.link(filepath, path_to_file_with_original_name)
+
+        if not self.qubes:
+            return
+
+        command = "qvm-open-in-vm"
+        args = ['$dispvm:sd-svs-disp', path_to_file_with_original_name]
+        process = QProcess(self)
+        process.start(command, args)
 
     def run_export_preflight_checks(self):
         '''
-        Run preflight checks to make sure the Export VM is configured correctly
+        Run preflight checks to make sure the Export VM is configured correctly.
         '''
-        logger.debug('Calling export preflight checks from thread {}'.format(
-            threading.current_thread().ident))
+        logger.info('Running export preflight checks')
 
         if not self.qubes:
             return
 
         self.export.begin_preflight_check.emit()
 
     def export_file_to_usb_drive(self, file_uuid: str, passphrase: str) -> None:
+        '''
+        Send the file specified by file_uuid to the Export VM with the user-provided passphrase for
+        unlocking the attached transfer device.
+
+        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 user to know which
+        application to open the file in, we export the file with a different name: the original
+        filename which includes the file extesion.
+
+        If the file is missing, update the db so that is_downloaded is set to False.
+        '''
         file = self.get_file(file_uuid)
+        logger.info('Exporting file {}'.format(file.original_filename))
 
-        logger.debug('Exporting {} from thread {}'.format(
-            file.original_filename, threading.current_thread().ident))
+        if not self.downloaded_file_exists(file.uuid):
+            self.sync_api()
+            return
+
+        path_to_file_with_original_name = os.path.join(self.data_dir, file.original_filename)
+
+        if not os.path.exists(path_to_file_with_original_name):
+            fn_no_ext, dummy = os.path.splitext(os.path.splitext(file.filename)[0])
+            filepath = os.path.join(self.data_dir, fn_no_ext)
+            os.link(filepath, path_to_file_with_original_name)
 
         if not self.qubes:
             return
 
-        filepath = os.path.join(self.data_dir, file.original_filename)
+        self.export.begin_usb_export.emit([path_to_file_with_original_name], passphrase)
 
-        self.export.begin_usb_export.emit([filepath], passphrase)
+    def on_export_usb_call_success(self, filepaths: List[str]):
+        '''
+        Clean export files that are hard links to the file on disk.
+        '''
+        for filepath in filepaths:
+            if os.path.exists(filepath):
+                os.remove(filepath)
+
+    def on_export_usb_call_failure(self, filepaths: List[str]):
+        '''
+        Clean export files that are hard links to the file on disk.
+        '''
+        for filepath in filepaths:
+            if os.path.exists(filepath):
+                os.remove(filepath)
 
     def on_submission_download(
         self,
@@ -657,7 +717,7 @@ def on_file_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)
         else:
-            self.set_status(_('The file download failed. Please try again.'))
+            self.gui.update_error_status(_('The file download failed. Please try again.'))
 
     def on_delete_source_success(self, result) -> None:
         """

securedrop_client/storage.py

@@ -332,6 +332,22 @@ def update_and_get_user(uuid: str,
     return user
 
 
+def update_missing_files(data_dir: str, session: Session) -> None:
+    '''
+    Update files that are marked as downloaded yet missing from the filesystem.
+    '''
+    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):
+            mark_as_not_downloaded(file.uuid, session)
+
+
+def find_new_files(session: Session) -> List[File]:
+    return session.query(File).filter_by(is_downloaded=False).all()
+
+
 def find_new_messages(session: Session) -> List[Message]:
     """
     Find messages to process. Those messages are those where one of the following
@@ -347,10 +363,6 @@ def find_new_messages(session: Session) -> List[Message]:
             Message.is_decrypted == None)).all()  # noqa: E711
 
 
-def find_new_files(session: Session) -> List[File]:
-    return session.query(File).filter_by(is_downloaded=False).all()
-
-
 def find_new_replies(session: Session) -> List[Reply]:
     """
     Find replies to process. Those replies are those where one of the following
@@ -366,6 +378,17 @@ def find_new_replies(session: Session) -> List[Reply]:
             Reply.is_decrypted == None)).all()  # noqa: E711
 
 
+def mark_as_not_downloaded(uuid: str, session: Session) -> None:
+    """
+    Mark File as not downloaded in the database.
+    """
+    db_obj = session.query(File).filter_by(uuid=uuid).one()
+    db_obj.is_downloaded = False
+    db_obj.is_decrypted = None
+    session.add(db_obj)
+    session.commit()
+
+
 def mark_as_downloaded(
     model_type: Union[Type[File], Type[Message], Type[Reply]],
     uuid: str,

tests/gui/test_widgets.py

@@ -1434,11 +1434,41 @@ def test_FileWidget__on_export_clicked(mocker, session, source):
     fw.update = mocker.MagicMock()
     mocker.patch('securedrop_client.gui.widgets.QDialog.exec')
     controller.run_export_preflight_checks = mocker.MagicMock()
+    controller.downloaded_file_exists = mocker.MagicMock(return_value=True)
 
     fw._on_export_clicked()
 
     controller.run_export_preflight_checks.assert_called_once_with()
 
+    # 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)
+
+
+def test_FileWidget__on_export_clicked_missing_file(mocker, session, source):
+    """
+    Ensure dialog does not open when the EXPORT button is clicked yet the file to export is missing
+    """
+    file = factory.File(source=source['source'], is_downloaded=True)
+    session.add(file)
+    session.commit()
+
+    get_file = mocker.MagicMock(return_value=file)
+    controller = mocker.MagicMock(get_file=get_file)
+
+    fw = FileWidget(file.uuid, controller, mocker.MagicMock())
+    fw.update = mocker.MagicMock()
+    mocker.patch('securedrop_client.gui.widgets.QDialog.exec')
+    controller.run_export_preflight_checks = mocker.MagicMock()
+    controller.downloaded_file_exists = mocker.MagicMock(return_value=False)
+    dialog = mocker.patch('securedrop_client.gui.widgets.ExportDialog')
+
+    fw._on_export_clicked()
+
+    controller.run_export_preflight_checks.assert_not_called()
+    dialog.assert_not_called()
+
 
 def test_ExportDialog_export(mocker):
     """

tests/test_export.py

@@ -36,13 +36,13 @@ def test_send_file_to_usb_device_error(mocker):
     export = Export()
     export.export_usb_call_failure = mocker.MagicMock()
     export.export_usb_call_failure.emit = mocker.MagicMock()
-    error = ExportError('bang')
+    error = ExportError('[mock_filepath]')
     _run_disk_export = mocker.patch.object(export, '_run_disk_export', side_effect=error)
 
     export.send_file_to_usb_device(['mock_filepath'], 'mock passphrase')
 
     _run_disk_export.assert_called_once_with('mock_temp_dir', ['mock_filepath'], 'mock passphrase')
-    export.export_usb_call_failure.emit.assert_called_once_with(error.status)
+    export.export_usb_call_failure.emit.assert_called_once_with(['mock_filepath'])
 
 
 def test_run_preflight_checks(mocker):