address pr comments

Author: Allie Crevier

securedrop_client/export.py

@@ -43,9 +43,6 @@ class Export:
     securedrop-export repository.
     '''
 
-    QVM_OP = 'qvm-open-in-vm'
-    QVM_VM = 'sd-export-usb'
-
     METADATA_FN = 'metadata.json'
 
     USB_TEST_FN = 'usb-test.sd-export'
@@ -66,8 +63,8 @@ class Export:
     DISK_ENCRYPTION_KEY_NAME = 'encryption_key'
     DISK_EXPORT_DIR = 'export_data'
 
-    def __init__(self, is_qubes: bool) -> None:
-        self.is_qubes = is_qubes
+    def __init__(self) -> None:
+        pass
 
     def _export_archive(cls, archive_path: str) -> str:
         '''
@@ -92,7 +89,12 @@ def _export_archive(cls, archive_path: str) -> str:
             # Python's implementation of subprocess, see
             # https://docs.python.org/3/library/subprocess.html#security-considerations
             output = subprocess.check_output(
-                [quote(cls.QVM_OP), quote(cls.QVM_VM), quote(archive_path)],
+                [
+                    quote('qvm-open-in-vm'),
+                    quote('sd-export-usb'),
+                    quote(archive_path),
+                    '--view-only'
+                ],
                 stderr=subprocess.STDOUT)
             return output.decode('utf-8').strip()
         except subprocess.CalledProcessError as e:
@@ -165,8 +167,6 @@ def _run_usb_test(self, archive_dir: str) -> None:
         Raises:
             ExportError: Raised if the usb-test does not return a USB_CONNECTED status.
         '''
-        logger.debug('Running usb-test...')
-
         archive_path = self._create_archive(archive_dir, self.USB_TEST_FN, self.USB_TEST_METADATA)
         status = self._export_archive(archive_path)
         if status != ExportStatus.USB_CONNECTED.value:
@@ -182,8 +182,6 @@ def _run_disk_test(self, archive_dir: str) -> None:
         Raises:
             ExportError: Raised if the usb-test does not return a DISK_ENCRYPTED status.
         '''
-        logger.debug('Running disk-test...')
-
         archive_path = self._create_archive(archive_dir, self.DISK_TEST_FN, self.DISK_TEST_METADATA)
 
         status = self._export_archive(archive_path)
@@ -200,8 +198,6 @@ def _run_disk_export(self, archive_dir: str, filepaths: List[str], passphrase: s
         Raises:
             ExportError: Raised if the usb-test does not return a DISK_ENCRYPTED status.
         '''
-        logger.debug('Exporting to disk...')
-
         metadata = self.DISK_METADATA.copy()
         metadata[self.DISK_ENCRYPTION_KEY_NAME] = passphrase
         archive_path = self._create_archive(archive_dir, self.DISK_FN, metadata, filepaths)
@@ -210,15 +206,10 @@ def _run_disk_export(self, archive_dir: str, filepaths: List[str], passphrase: s
         if status:
             raise ExportError(status)
 
-        logger.debug('Export successful')
-
     def run_preflight_checks(self) -> None:
         '''
         Run preflight checks to verify that the usb device is connected and luks-encrypted.
         '''
-        if not self.is_qubes:
-            return
-
         with TemporaryDirectory() as temp_dir:
             self._run_usb_test(temp_dir)
             self._run_disk_test(temp_dir)
@@ -231,8 +222,5 @@ def send_file_to_usb_device(self, filepaths: List[str], passphrase: str) -> None
             filepath: The path of file to export.
             passphrase: The passphrase to unlock the luks-encrypted usb disk drive.
         '''
-        if not self.is_qubes:
-            return
-
         with TemporaryDirectory() as temp_dir:
             self._run_disk_export(temp_dir, filepaths, passphrase)

securedrop_client/gui/widgets.py

@@ -1883,7 +1883,7 @@ def __init__(self, controller, file_uuid):
         usb_form_layout = QVBoxLayout()
         self.insert_usb_form.setLayout(usb_form_layout)
         self.usb_error_message = SecureQLabel(_(
-            'Either the drive is not encrypted with VeraCrypt, or there is something else wrong'
+            'Either the drive is not luks-encrypted, or there is something else wrong'
             'with it. Please try another drive, or see your administrator for help.'))
         self.usb_error_message.setWordWrap(True)
         usb_instructions = SecureQLabel(_(
@@ -1928,11 +1928,18 @@ def __init__(self, controller, file_uuid):
         passphrase_form_layout.addWidget(buttons, alignment=Qt.AlignRight)
         self.passphrase_error_message.hide()
 
+        # Starting export message
+        self.exporting_message = SecureQLabel(_('Exporting...'))
+        self.exporting_message.setWordWrap(True)
+
         layout.addWidget(self.starting_export_message)
+        layout.addWidget(self.exporting_message)
         layout.addWidget(self.generic_error)
         layout.addWidget(self.insert_usb_form)
         layout.addWidget(self.passphrase_form)
 
+        self.starting_export_message.show()
+        self.exporting_message.hide()
         self.generic_error.hide()
         self.insert_usb_form.hide()
         self.passphrase_form.hide()
@@ -1972,6 +1979,9 @@ def _on_retry_export_button_clicked(self):
     @pyqtSlot()
     def _on_unlock_disk_clicked(self):
         try:
+            self.passphrase_form.hide()
+            self.exporting_message.show()
+            QApplication.processEvents()
             passphrase = self.passphrase_field.text()
             self.controller.export_file_to_usb_drive(self.file_uuid, passphrase)
             self.close()
@@ -1990,6 +2000,7 @@ def _request_to_insert_usb_device(self, encryption_not_supported: bool = False):
 
     def _request_passphrase(self, bad_passphrase: bool = False):
         self.starting_export_message.hide()
+        self.exporting_message.hide()
         self.passphrase_form.show()
         self.insert_usb_form.hide()
 

securedrop_client/logic.py

@@ -179,7 +179,7 @@ def __init__(self, hostname: str, gui, session_maker: sessionmaker,
 
         self.gpg = GpgHelper(home, self.session_maker, proxy)
 
-        self.export = Export(self.qubes)
+        self.export = Export()
 
         self.sync_flag = os.path.join(home, 'sync_flag')
 
@@ -593,13 +593,29 @@ def on_file_open(self, file_uuid: str) -> None:
             logger.info('Opening file "{}".'.format(original_filepath))
 
     def run_export_preflight_checks(self):
+        '''
+        Run preflight checks to make sure the Export VM is configured correctly and
+        '''
+        logger.debug('Running export preflight checks')
+
+        if not self.qubes:
+            return
+
         self.export.run_preflight_checks()
 
     def export_file_to_usb_drive(self, file_uuid: str, passphrase: str) -> None:
         file = self.get_file(file_uuid)
+
+        logger.debug('Exporting {}'.format(file.original_filename))
+
+        if not self.qubes:
+            return
+
         filepath = os.path.join(self.data_dir, file.original_filename)
         self.export.send_file_to_usb_device([filepath], passphrase)
 
+        logger.debug('Export successful')
+
     def on_submission_download(
         self,
         submission_type: Union[Type[db.File], Type[db.Message]],

tests/test_export.py

@@ -14,26 +14,14 @@ def test_send_file_to_usb_device(mocker):
     mock_temp_dir = mocker.MagicMock()
     mock_temp_dir.__enter__ = mocker.MagicMock(return_value='mock_temp_dir')
     mocker.patch('securedrop_client.export.TemporaryDirectory', return_value=mock_temp_dir)
-    export = Export(is_qubes=True)
+    export = Export()
     _run_disk_export = mocker.patch.object(export, '_run_disk_export')
 
     export.send_file_to_usb_device(['mock_filepath'], 'mock passphrase')
 
     _run_disk_export.assert_called_once_with('mock_temp_dir', ['mock_filepath'], 'mock passphrase')
 
 
-def test_send_file_to_usb_device_not_qubes(mocker):
-    '''
-    Ensure method returns without error and does not call qubes-only methods.
-    '''
-    export = Export(is_qubes=False)
-    _run_disk_export = mocker.patch.object(export, '_run_disk_export')
-
-    export.send_file_to_usb_device(['mock_filepath'], 'mock passphrase')
-
-    _run_disk_export.assert_not_called()
-
-
 def test_run_preflight_checks(mocker):
     '''
     Ensure TemporaryDirectory is used when creating and sending the archives during the preflight
@@ -42,7 +30,7 @@ def test_run_preflight_checks(mocker):
     mock_temp_dir = mocker.MagicMock()
     mock_temp_dir.__enter__ = mocker.MagicMock(return_value='mock_temp_dir')
     mocker.patch('securedrop_client.export.TemporaryDirectory', return_value=mock_temp_dir)
-    export = Export(is_qubes=True)
+    export = Export()
     _run_usb_export = mocker.patch.object(export, '_run_usb_test')
     _run_disk_export = mocker.patch.object(export, '_run_disk_test')
 
@@ -52,27 +40,13 @@ def test_run_preflight_checks(mocker):
     _run_disk_export.assert_called_once_with('mock_temp_dir')
 
 
-def test_run_preflight_checks_not_qubes(mocker):
-    '''
-    Ensure method returns without error and does not call qubes-only methods.
-    '''
-    export = Export(is_qubes=False)
-    _run_usb_test = mocker.patch.object(export, '_run_usb_test')
-    _run_disk_test = mocker.patch.object(export, '_run_disk_test')
-
-    export.run_preflight_checks()
-
-    _run_usb_test.assert_not_called()
-    _run_disk_test.assert_not_called()
-
-
 def test__run_disk_export(mocker):
     '''
     Ensure _export_archive and _create_archive are called with the expected parameters,
     _export_archive is called with the return value of _create_archive, and
     _run_disk_test returns without error if '' is the ouput status of _export_archive.
     '''
-    export = Export(is_qubes=False)
+    export = Export()
     export._create_archive = mocker.MagicMock(return_value='mock_archive_path')
     export._export_archive = mocker.MagicMock(return_value='')
 
@@ -94,7 +68,7 @@ def test__run_disk_export_raises_ExportError_if_not_empty_string(mocker):
     '''
     Ensure ExportError is raised if _run_disk_test returns anything other than ''.
     '''
-    export = Export(is_qubes=True)
+    export = Export()
     export._create_archive = mocker.MagicMock(return_value='mock_archive_path')
     export._export_archive = mocker.MagicMock(return_value='SOMETHING_OTHER_THAN_EMPTY_STRING')
 
@@ -108,7 +82,7 @@ def test__run_disk_test(mocker):
     _export_archive is called with the return value of _create_archive, and
     _run_disk_test returns without error if 'USB_ENCRYPTED' is the ouput status of _export_archive.
     '''
-    export = Export(is_qubes=False)
+    export = Export()
     export._create_archive = mocker.MagicMock(return_value='mock_archive_path')
     export._export_archive = mocker.MagicMock(return_value='USB_ENCRYPTED')
 
@@ -123,7 +97,7 @@ def test__run_disk_test_raises_ExportError_if_not_USB_ENCRYPTED(mocker):
     '''
     Ensure ExportError is raised if _run_disk_test returns anything other than 'USB_ENCRYPTED'.
     '''
-    export = Export(is_qubes=True)
+    export = Export()
     export._create_archive = mocker.MagicMock(return_value='mock_archive_path')
     export._export_archive = mocker.MagicMock(return_value='SOMETHING_OTHER_THAN_USB_ENCRYPTED')
 
@@ -137,7 +111,7 @@ def test__run_usb_test(mocker):
     _export_archive is called with the return value of _create_archive, and
     _run_disk_test returns without error if 'USB_CONNECTED' is the return value of _export_archive.
     '''
-    export = Export(is_qubes=False)
+    export = Export()
     export._create_archive = mocker.MagicMock(return_value='mock_archive_path')
     export._export_archive = mocker.MagicMock(return_value='USB_CONNECTED')
 
@@ -152,7 +126,7 @@ def test__run_usb_test_raises_ExportError_if_not_USB_CONNECTED(mocker):
     '''
     Ensure ExportError is raised if _run_disk_test returns anything other than 'USB_CONNECTED'.
     '''
-    export = Export(is_qubes=True)
+    export = Export()
     export._create_archive = mocker.MagicMock(return_value='mock_archive_path')
     export._export_archive = mocker.MagicMock(return_value='SOMETHING_OTHER_THAN_USB_CONNECTED')
 
@@ -164,7 +138,7 @@ def test__create_archive(mocker):
     '''
     Ensure _create_archive creates an archive in the supplied directory.
     '''
-    export = Export(is_qubes=True)
+    export = Export()
     archive_path = None
     with TemporaryDirectory() as temp_dir:
         archive_path = export._create_archive(temp_dir, 'mock.sd-export', {})
@@ -175,7 +149,7 @@ def test__create_archive(mocker):
 
 
 def test__create_archive_with_an_export_file(mocker):
-    export = Export(is_qubes=True)
+    export = Export()
     archive_path = None
     with TemporaryDirectory() as temp_dir, NamedTemporaryFile() as export_file:
         archive_path = export._create_archive(temp_dir, 'mock.sd-export', {}, [export_file.name])
@@ -189,7 +163,7 @@ def test__create_archive_with_multiple_export_files(mocker):
     '''
     Ensure an archive
     '''
-    export = Export(is_qubes=True)
+    export = Export()
     archive_path = None
     with TemporaryDirectory() as temp_dir, \
         NamedTemporaryFile() as export_file_one, \
@@ -207,7 +181,7 @@ def test__export_archive(mocker):
     Ensure the subprocess call returns the expected output.
     '''
     mocker.patch('subprocess.check_output', return_value=b'mock')
-    export = Export(is_qubes=True)
+    export = Export()
     status = export._export_archive('mock.sd-export')
 
     assert status == 'mock'
@@ -220,7 +194,7 @@ def test__export_archive_does_not_raise_ExportError_when_CalledProcessError(mock
     mock_error = subprocess.CalledProcessError('mock_cmd', 123)
     mocker.patch('subprocess.check_output', side_effect=mock_error)
 
-    export = Export(is_qubes=True)
+    export = Export()
 
     with pytest.raises(ExportError, match='CALLED_PROCESS_ERROR'):
         export._export_archive('mock.sd-export')
@@ -230,10 +204,10 @@ def test__export_archive_with_evil_command(mocker):
     '''
     Ensure shell command is shell-escaped.
     '''
-    export = Export(is_qubes=False)
+    export = Export()
     check_output = mocker.patch('subprocess.check_output', return_value=b'')
 
     export._export_archive('somefile; rm -rf ~')
 
     check_output.assert_called_once_with(
-        ['qvm-open-in-vm', 'sd-export-usb', "'somefile; rm -rf ~'"], stderr=-2)
+        ['qvm-open-in-vm', 'sd-export-usb', "'somefile; rm -rf ~'", '--view-only'], stderr=-2)

tests/test_logic.py

@@ -1417,16 +1417,30 @@ def test_Controller_call_update_star_success(homedir, config, mocker, session_ma
 
 
 def test_Controller_run_export_preflight_checks(homedir, mocker):
+    debug_logger = mocker.patch('securedrop_client.logic.logger.debug')
     co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir)
     co.export = mocker.MagicMock()
 
     co.run_export_preflight_checks()
 
     co.export.run_preflight_checks.assert_called_once_with()
+    debug_logger.assert_called_once_with('Running export preflight checks')
 
 
-def test_Controller_export_file_to_usb_drive(homedir, mocker, session):
+def test_Controller_run_export_preflight_checks_not_qubes(homedir, mocker):
+    debug_logger = mocker.patch('securedrop_client.logic.logger.debug')
+    co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir)
+    co.qubes = False
+    co.export = mocker.MagicMock()
+
+    co.run_export_preflight_checks()
+
+    co.export.run_preflight_checks.assert_not_called()
+    debug_logger.assert_called_once_with('Running export preflight checks')
+
 
+def test_Controller_export_file_to_usb_drive(homedir, mocker, session):
+    debug_logger = mocker.patch('securedrop_client.logic.logger.debug')
     co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir)
     co.export = mocker.MagicMock()
     file = factory.File(source=factory.Source(), original_filename='mock_filename')
@@ -1438,3 +1452,21 @@ def test_Controller_export_file_to_usb_drive(homedir, mocker, session):
 
     co.export.send_file_to_usb_device.assert_called_once_with(
         [os.path.join(co.data_dir, 'mock_filename')], 'mock passphrase')
+    debug_logger.call_args_list[0][0][0] == 'Exporting {}'.format(file.original_filename)
+    debug_logger.call_args_list[1][0][0] == 'Export successful'
+
+
+def test_Controller_export_file_to_usb_drive_not_qubes(homedir, mocker, session):
+    debug_logger = mocker.patch('securedrop_client.logic.logger.debug')
+    co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir)
+    co.qubes = False
+    co.export = mocker.MagicMock()
+    file = factory.File(source=factory.Source(), original_filename='mock_filename')
+    session.add(file)
+    session.commit()
+    mocker.patch('securedrop_client.logic.Controller.get_file', return_value=file)
+
+    co.export_file_to_usb_drive(file.uuid, 'mock passphrase')
+
+    co.export.send_file_to_usb_device.assert_not_called()
+    debug_logger.call_args_list[0][0][0] == 'Exporting {}'.format(file.original_filename)