Merge pull request #1541 from freedomofpress/refactor-remove-controll…

…er-dependency-from-export-device-1

Refactor remove controller dependency from export device (Part 1)

securedrop_client/app.py

@@ -34,7 +34,7 @@
 from PyQt5.QtCore import Qt, QThread, QTimer
 from PyQt5.QtWidgets import QApplication, QMessageBox
 
-from securedrop_client import __version__, state
+from securedrop_client import __version__, export, state
 from securedrop_client.database import Database
 from securedrop_client.db import make_session_maker
 from securedrop_client.gui.main import Window
@@ -237,9 +237,19 @@ def start_app(args, qt_args) -> NoReturn:  # type: ignore [no-untyped-def]
     session = session_maker()
     database = Database(session)
     app_state = state.State(database)
-    gui = Window(app_state)
 
-    with threads(4) as [export_thread, sync_thread, main_queue_thread, file_download_queue_thread]:
+    with threads(4) as [
+        export_service_thread,
+        sync_thread,
+        main_queue_thread,
+        file_download_queue_thread,
+    ]:
+        export_service = export.Service()
+        export_service.moveToThread(export_service_thread)
+        export_service_thread.start()
+
+        gui = Window(app_state, export_service)
+
         controller = Controller(
             "http://localhost:8081/",
             gui,
@@ -248,7 +258,6 @@ def start_app(args, qt_args) -> NoReturn:  # type: ignore [no-untyped-def]
             app_state,
             not args.no_proxy,
             not args.no_qubes,
-            export_thread,
             sync_thread,
             main_queue_thread,
             file_download_queue_thread,

securedrop_client/config.py

@@ -1,13 +1,9 @@
 import json
 import logging
 import os
-from typing import Type, TypeVar
 
 logger = logging.getLogger(__name__)
 
-# See: https://mypy.readthedocs.io/en/stable/generics.html#generic-methods-and-generic-self
-T = TypeVar("T", bound="Config")
-
 
 class Config:
 
@@ -17,8 +13,8 @@ def __init__(self, journalist_key_fingerprint: str) -> None:
         self.journalist_key_fingerprint = journalist_key_fingerprint
 
     @classmethod
-    def from_home_dir(cls: Type[T], sdc_home: str) -> T:
-        full_path = os.path.join(sdc_home, cls.CONFIG_NAME)
+    def from_home_dir(cls, sdc_home: str) -> "Config":
+        full_path = os.path.join(sdc_home, Config.CONFIG_NAME)
 
         try:
             with open(full_path) as f:
@@ -27,7 +23,9 @@ def from_home_dir(cls: Type[T], sdc_home: str) -> T:
             logger.error("Error opening config file at {}: {}".format(full_path, e))
             json_config = {}
 
-        return cls(journalist_key_fingerprint=json_config.get("journalist_key_fingerprint", None))
+        return Config(
+            journalist_key_fingerprint=json_config.get("journalist_key_fingerprint", None)
+        )
 
     @property
     def is_valid(self) -> bool:

securedrop_client/export.py

@@ -87,6 +87,21 @@ def __init__(
     ) -> None:
         super().__init__()
 
+        self.connect_signals(
+            export_preflight_check_requested,
+            export_requested,
+            print_preflight_check_requested,
+            print_requested,
+        )
+
+    def connect_signals(
+        self,
+        export_preflight_check_requested: Optional[pyqtSignal] = None,
+        export_requested: Optional[pyqtSignal] = None,
+        print_preflight_check_requested: Optional[pyqtSignal] = None,
+        print_requested: Optional[pyqtSignal] = None,
+    ) -> None:
+
         # This instance can optionally react to events to prevent
         # coupling it to dependent code.
         if export_preflight_check_requested is not None:
@@ -350,3 +365,6 @@ def print(self, filepaths: List[str]) -> None:
                 self.print_call_failure.emit(e)
 
         self.export_completed.emit(filepaths)
+
+
+Service = Export

securedrop_client/gui/conversation/export/device.py

@@ -1,10 +1,9 @@
 import logging
 import os
-from typing import Optional
 
-from PyQt5.QtCore import QObject, QThread, pyqtSignal
+from PyQt5.QtCore import QObject, pyqtSignal
 
-from securedrop_client.export import Export
+from securedrop_client import export
 from securedrop_client.logic import Controller
 
 logger = logging.getLogger(__name__)
@@ -35,14 +34,13 @@ class Device(QObject):
     print_succeeded = pyqtSignal()
     print_failed = pyqtSignal(object)
 
-    def __init__(
-        self, controller: Controller, export_service_thread: Optional[QThread] = None
-    ) -> None:
+    def __init__(self, controller: Controller, export_service: export.Service) -> None:
         super().__init__()
 
         self._controller = controller
+        self._export_service = export_service
 
-        self._export_service = Export(
+        self._export_service.connect_signals(
             self.export_preflight_check_requested,
             self.export_requested,
             self.print_preflight_check_requested,
@@ -67,35 +65,18 @@ def __init__(
         self._export_service.print_call_failure.connect(self.print_failed)
         self._export_service.print_call_success.connect(self.print_succeeded)
 
-        if export_service_thread is not None:
-            # Run export object in a separate thread context (a reference to the
-            # thread is kept on self such that it does not get garbage collected
-            # after this method returns) - we want to keep our export thread around for
-            # later processing.
-            self._move_export_service_to_thread(export_service_thread)
-
     def run_printer_preflight_checks(self) -> None:
         """
         Run preflight checks to make sure the Export VM is configured correctly.
         """
         logger.info("Running printer preflight check")
-
-        if not self._controller.qubes:
-            self.print_preflight_check_succeeded.emit()
-            return
-
         self.print_preflight_check_requested.emit()
 
     def run_export_preflight_checks(self) -> None:
         """
         Run preflight checks to make sure the Export VM is configured correctly.
         """
         logger.info("Running export preflight check")
-
-        if not self._controller.qubes:
-            self.export_preflight_check_succeeded.emit()
-            return
-
         self.export_preflight_check_requested.emit()
 
     def export_file_to_usb_drive(self, file_uuid: str, passphrase: str) -> None:
@@ -111,10 +92,6 @@ def export_file_to_usb_drive(self, file_uuid: str, passphrase: str) -> None:
         if not self._controller.downloaded_file_exists(file):
             return
 
-        if not self._controller.qubes:
-            self.export_succeeded.emit()
-            return
-
         self.export_requested.emit([file_location], passphrase)
 
     def print_file(self, file_uuid: str) -> None:
@@ -129,13 +106,4 @@ def print_file(self, file_uuid: str) -> None:
         if not self._controller.downloaded_file_exists(file):
             return
 
-        if not self._controller.qubes:
-            return
-
         self.print_requested.emit([file_location])
-
-    def _move_export_service_to_thread(self, thread: QThread) -> None:
-        self._export_service_thread = thread
-
-        self._export_service.moveToThread(self._export_service_thread)
-        self._export_service_thread.start()

securedrop_client/gui/main.py

@@ -27,7 +27,7 @@
 from PyQt5.QtGui import QClipboard, QGuiApplication, QIcon, QKeySequence
 from PyQt5.QtWidgets import QAction, QApplication, QHBoxLayout, QMainWindow, QVBoxLayout, QWidget
 
-from securedrop_client import __version__, state
+from securedrop_client import __version__, export, state
 from securedrop_client.db import Source, User
 from securedrop_client.gui.auth import LoginDialog
 from securedrop_client.gui.widgets import LeftPane, MainView, TopPane
@@ -45,7 +45,11 @@ class Window(QMainWindow):
 
     icon = "icon.png"
 
-    def __init__(self, app_state: Optional[state.State] = None) -> None:
+    def __init__(
+        self,
+        app_state: Optional[state.State] = None,
+        export_service: Optional[export.Service] = None,
+    ) -> None:
         """
         Create the default start state. The window contains a root widget into
         which is placed:
@@ -73,7 +77,7 @@ def __init__(self, app_state: Optional[state.State] = None) -> None:
         layout.setSpacing(0)
         self.main_pane.setLayout(layout)
         self.left_pane = LeftPane()
-        self.main_view = MainView(self.main_pane, app_state)
+        self.main_view = MainView(self.main_pane, app_state, export_service)
         layout.addWidget(self.left_pane)
         layout.addWidget(self.main_view)
 

securedrop_client/gui/widgets.py

@@ -58,7 +58,7 @@
     QWidget,
 )
 
-from securedrop_client import state
+from securedrop_client import export, state
 from securedrop_client.db import (
     DraftReply,
     File,
@@ -564,10 +564,16 @@ class MainView(QWidget):
     and main context view).
     """
 
-    def __init__(self, parent: QObject, app_state: Optional[state.State] = None) -> None:
+    def __init__(
+        self,
+        parent: QObject,
+        app_state: Optional[state.State] = None,
+        export_service: Optional[export.Service] = None,
+    ) -> None:
         super().__init__(parent)
 
         self._state = app_state
+        self._export_service = export_service
 
         # Set id and styles
         self.setObjectName("MainView")
@@ -665,7 +671,7 @@ def on_source_changed(self) -> None:
                 conversation_wrapper.conversation_view.update_conversation(source.collection)
             else:
                 conversation_wrapper = SourceConversationWrapper(
-                    source, self.controller, self._state
+                    source, self.controller, self._state, self._export_service
                 )
                 self.source_conversations[source.uuid] = conversation_wrapper
 
@@ -2190,14 +2196,23 @@ def __init__(
         file_missing: pyqtBoundSignal,
         index: int,
         container_width: int,
+        export_service: Optional[export.Service] = None,
     ) -> None:
         """
         Given some text and a reference to the controller, make something to display a file.
         """
         super().__init__()
 
         self.controller = controller
-        self._export_device = conversation.ExportDevice(controller, controller.export_thread)
+
+        if export_service is None:
+            # Note that injecting an export service that runs in a separate
+            # thread is greatly encouraged! But it is optional because strictly
+            # speaking it is not a dependency of this FileWidget.
+            export_service = export.Service()
+
+        self._export_device = conversation.ExportDevice(controller, export_service)
+
         self.file = self.controller.get_file(file_uuid)
         self.uuid = file_uuid
         self.index = index
@@ -2602,9 +2617,16 @@ class ConversationView(QWidget):
 
     SCROLL_BAR_WIDTH = 15
 
-    def __init__(self, source_db_object: Source, controller: Controller) -> None:
+    def __init__(
+        self,
+        source_db_object: Source,
+        controller: Controller,
+        export_service: Optional[export.Service] = None,
+    ) -> None:
         super().__init__()
 
+        self._export_service = export_service
+
         self.source = source_db_object
         self.source_uuid = source_db_object.uuid
         self.controller = controller
@@ -2790,6 +2812,7 @@ def add_file(self, file: File, index: int) -> None:
             self.controller.file_missing,
             index,
             self.scroll.widget().width(),
+            self._export_service,
         )
         self.scroll.add_widget_to_conversation(index, conversation_item, Qt.AlignLeft)
         self.current_messages[file.uuid] = conversation_item
@@ -2893,7 +2916,11 @@ class SourceConversationWrapper(QWidget):
     deleting_conversation = False
 
     def __init__(
-        self, source: Source, controller: Controller, app_state: Optional[state.State] = None
+        self,
+        source: Source,
+        controller: Controller,
+        app_state: Optional[state.State] = None,
+        export_service: Optional[export.Service] = None,
     ) -> None:
         super().__init__()
 
@@ -2919,7 +2946,7 @@ def __init__(
 
         # Create widgets
         self.conversation_title_bar = SourceProfileShortWidget(source, controller, app_state)
-        self.conversation_view = ConversationView(source, controller)
+        self.conversation_view = ConversationView(source, controller, export_service)
         self.reply_box = ReplyBoxWidget(source, controller)
         self.deletion_indicator = SourceDeletionIndicator()
         self.conversation_deletion_indicator = ConversationDeletionIndicator()

securedrop_client/logic.py

@@ -318,7 +318,6 @@ def __init__(  # type: ignore [no-untyped-def]
         state: state.State,
         proxy: bool = True,
         qubes: bool = True,
-        export_thread: Optional[QThread] = None,
         sync_thread: Optional[QThread] = None,
         main_queue_thread: Optional[QThread] = None,
         file_download_queue_thread: Optional[QThread] = None,
@@ -333,11 +332,6 @@ def __init__(  # type: ignore [no-untyped-def]
 
         self._state = state
 
-        if export_thread is not None:
-            self.export_thread = export_thread
-        else:
-            self.export_thread = QThread()
-
         if sync_thread is not None:
             self.sync_thread = sync_thread
         else:

tests/conftest.py

@@ -10,7 +10,7 @@
 from PyQt5.QtCore import Qt
 from PyQt5.QtWidgets import QMainWindow
 
-from securedrop_client import state
+from securedrop_client import export, state
 from securedrop_client.app import configure_locale_and_language
 from securedrop_client.config import Config
 from securedrop_client.db import (
@@ -127,14 +127,29 @@ def homedir(i18n):
 
 
 @pytest.fixture(scope="function")
-def functional_test_app_started_context(homedir, reply_status_codes, session, config, qtbot):
+def export_service():
+    """An export service that assumes the Qubes RPC calls are successful and skips them."""
+    export_service = export.Service()
+    # Ensure the export_service doesn't rely on Qubes OS:
+    export_service._run_disk_test = lambda dir: None
+    export_service._run_usb_test = lambda dir: None
+    export_service._run_disk_export = lambda dir, paths, pasphrase: None
+    export_service._run_printer_preflight = lambda dir: None
+    export_service._run_print = lambda dir, paths: None
+    return export_service
+
+
+@pytest.fixture(scope="function")
+def functional_test_app_started_context(
+    homedir, reply_status_codes, session, config, qtbot, export_service
+):
     """
     Returns a tuple containing the gui window and controller of a configured client. This should be
     used to for tests that need to start from the login dialog before the main application window
     is visible.
     """
     app_state = state.State()
-    gui = Window(app_state)
+    gui = Window(app_state, export_service)
     create_gpg_test_context(homedir)  # Configure test keys
     session_maker = make_session_maker(homedir)  # Configure and create the database
     controller = Controller(HOSTNAME, gui, session_maker, homedir, app_state, False, False)