From 0d5b9e9c17b81c283c401b6b0d03ca6c7bbb971d Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 11:39:45 +0200
Subject: [PATCH 01/17] remove "align" from FileWidget

---
 securedrop_client/gui/widgets.py | 20 ++++----------------
 tests/gui/test_widgets.py        | 18 +-----------------
 2 files changed, 5 insertions(+), 33 deletions(-)

diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py
index 7eccca292..da3e24965 100644
--- a/securedrop_client/gui/widgets.py
+++ b/securedrop_client/gui/widgets.py
@@ -1277,18 +1277,13 @@ class FileWidget(QWidget):
     def __init__(self, source_db_object, submission_db_object,
                  controller, file_ready_signal, align="left"):
         """
-        Given some text, an indication of alignment ('left' or 'right') and
-        a reference to the controller, make something to display a file.
-
-        Align is set to left by default because currently SecureDrop can only
-        accept files from sources to journalists.
+        Given some text and a reference to the controller, make something to display a file.
         """
         super().__init__()
         self.controller = controller
         self.source = source_db_object
         self.submission = submission_db_object
         self.file_uuid = self.submission.uuid
-        self.align = align
 
         self.layout = QHBoxLayout()
         self.update()
@@ -1296,7 +1291,7 @@ def __init__(self, source_db_object, submission_db_object,
 
         file_ready_signal.connect(self._on_file_download)
 
-    def update(self):
+    def update(self) -> None:
         icon = QLabel()
         icon.setPixmap(load_image('file.png'))
 
@@ -1306,18 +1301,11 @@ def update(self):
             human_filesize = humanize_filesize(self.submission.size)
             description = QLabel("Download ({})".format(human_filesize))
 
-        if self.align != "left":
-            # Float right...
-            self.layout.addStretch(5)
-
         self.layout.addWidget(icon)
         self.layout.addWidget(description, 5)
+        self.layout.addStretch(5)
 
-        if self.align == "left":
-            # Add space on right hand side...
-            self.layout.addStretch(5)
-
-    def clear(self):
+    def clear(self) -> None:
         while self.layout.count():
             child = self.layout.takeAt(0)
             if child.widget():
diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py
index 0cb523873..65db593ab 100644
--- a/tests/gui/test_widgets.py
+++ b/tests/gui/test_widgets.py
@@ -1139,7 +1139,7 @@ def test_FileWidget_init_left(mocker):
     source = factory.Source()
     file_ = factory.File(is_downloaded=True)
 
-    fw = FileWidget(source, file_, mock_controller, mock_signal, align='left')
+    fw = FileWidget(source, file_, mock_controller, mock_signal)
 
     assert isinstance(fw.layout.takeAt(0), QWidgetItem)
     assert isinstance(fw.layout.takeAt(0), QWidgetItem)
@@ -1147,22 +1147,6 @@ def test_FileWidget_init_left(mocker):
     assert fw.controller == mock_controller
 
 
-def test_FileWidget_init_right(mocker):
-    """
-    Check the FileWidget is configured correctly for align-right.
-    """
-    mock_controller = mocker.MagicMock()
-    mock_signal = mocker.MagicMock()  # not important for this test
-    source = factory.Source()
-    file_ = factory.File(is_downloaded=True)
-
-    fw = FileWidget(source, file_, mock_controller, mock_signal, align='right')
-    assert isinstance(fw.layout.takeAt(0), QSpacerItem)
-    assert isinstance(fw.layout.takeAt(0), QWidgetItem)
-    assert isinstance(fw.layout.takeAt(0), QWidgetItem)
-    assert fw.controller == mock_controller
-
-
 def test_FileWidget_mousePressEvent_download(mocker):
     """
     Should fire the expected download event handler in the logic layer.

From 71ea23371b2d0b150b19cede93b50092806c0b79 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 11:43:38 +0200
Subject: [PATCH 02/17] inject session_maker's into objects instead of passing
 Session's and SQLAlchemy objects

Currently we have managed to avoid threading issues with SQLA. Passing a
Session or Model (object corresponding to a DB row) across thread
boundaries is dangerous and will raise exception. We have managed to
avoid problems with this so far because most of our objects live in the
main GUI thread. Additionally, our signal/slot connections all live in
the main thread, and when they don't, we might actually still be
triggering actions in the main thread because we're not using a
Qt.QueuedConnection when we connect them.

This commit attempt to give objects their own session_maker so they can
create sessions on demand and fetch thread-local objects from the DB.
---
 create_dev_data.py                |  5 +-
 securedrop_client/app.py          | 14 +++---
 securedrop_client/crypto.py       | 20 ++++----
 securedrop_client/db.py           |  9 ++--
 securedrop_client/gui/main.py     |  7 +--
 securedrop_client/gui/widgets.py  | 80 ++++++++++++++++++++++---------
 securedrop_client/logic.py        | 17 ++++---
 securedrop_client/message_sync.py | 73 ++++++++++++++++------------
 8 files changed, 138 insertions(+), 87 deletions(-)

diff --git a/create_dev_data.py b/create_dev_data.py
index 394575cc4..087196cfe 100755
--- a/create_dev_data.py
+++ b/create_dev_data.py
@@ -3,10 +3,11 @@
 import os
 import sys
 from securedrop_client.config import Config
-from securedrop_client.db import Base, make_engine
+from securedrop_client.db import Base, make_session_maker
 
 sdc_home = sys.argv[1]
-Base.metadata.create_all(make_engine(sdc_home))
+session = make_session_maker(sdc_home)()
+Base.metadata.create_all(bind=session.get_bind())
 
 with open(os.path.join(sdc_home, Config.CONFIG_NAME), 'w') as f:
     f.write(json.dumps({
diff --git a/securedrop_client/app.py b/securedrop_client/app.py
index 95f5b5723..fbf9a7d47 100644
--- a/securedrop_client/app.py
+++ b/securedrop_client/app.py
@@ -25,7 +25,6 @@
 import sys
 import socket
 from argparse import ArgumentParser
-from sqlalchemy.orm import sessionmaker
 from PyQt5.QtWidgets import QApplication, QMessageBox
 from PyQt5.QtCore import Qt, QTimer
 from logging.handlers import TimedRotatingFileHandler
@@ -33,7 +32,7 @@
 from securedrop_client.logic import Controller
 from securedrop_client.gui.main import Window
 from securedrop_client.resources import load_icon, load_css
-from securedrop_client.db import make_engine
+from securedrop_client.db import make_session_maker
 from securedrop_client.utils import safe_mkdir
 
 DEFAULT_SDC_HOME = '~/.securedrop_client'
@@ -185,15 +184,14 @@ def start_app(args, qt_args) -> None:
 
     prevent_second_instance(app, args.sdc_home)
 
-    gui = Window()
+    session_maker = make_session_maker(args.sdc_home)
+
+    gui = Window(session_maker)
+
     app.setWindowIcon(load_icon(gui.icon))
     app.setStyleSheet(load_css('sdclient.css'))
 
-    engine = make_engine(args.sdc_home)
-    Session = sessionmaker(bind=engine)
-    session = Session()
-
-    controller = Controller("http://localhost:8081/", gui, session,
+    controller = Controller("http://localhost:8081/", gui, session_maker,
                             args.sdc_home, not args.no_proxy)
     controller.setup()
 
diff --git a/securedrop_client/crypto.py b/securedrop_client/crypto.py
index f44af1ab0..cd235c8eb 100644
--- a/securedrop_client/crypto.py
+++ b/securedrop_client/crypto.py
@@ -22,11 +22,11 @@
 import subprocess
 import tempfile
 
-from sqlalchemy.orm import sessionmaker
+from sqlalchemy.orm import scoped_session
 from uuid import UUID
 
 from securedrop_client.config import Config
-from securedrop_client.db import make_engine, Source
+from securedrop_client.db import Source
 from securedrop_client.utils import safe_mkdir
 
 logger = logging.getLogger(__name__)
@@ -39,7 +39,7 @@ class CryptoError(Exception):
 
 class GpgHelper:
 
-    def __init__(self, sdc_home: str, is_qubes: bool) -> None:
+    def __init__(self, sdc_home: str, session_maker: scoped_session, is_qubes: bool) -> None:
         '''
         :param sdc_home: Home directory for the SecureDrop client
         :param is_qubes: Whether the client is running in Qubes or not
@@ -47,9 +47,7 @@ def __init__(self, sdc_home: str, is_qubes: bool) -> None:
         safe_mkdir(os.path.join(sdc_home), "gpg")
         self.sdc_home = sdc_home
         self.is_qubes = is_qubes
-        engine = make_engine(sdc_home)
-        Session = sessionmaker(bind=engine)
-        self.session = Session()
+        self.session_maker = session_maker
 
         config = Config.from_home_dir(self.sdc_home)
         self.journalist_key_fingerprint = config.journalist_key_fingerprint
@@ -110,13 +108,14 @@ def _gpg_cmd_base(self) -> list:
         return cmd
 
     def import_key(self, source_uuid: UUID, key_data: str, fingerprint: str) -> None:
-        local_source = self.session.query(Source).filter_by(uuid=source_uuid).one()
+        session = self.session_maker()
+        local_source = session.query(Source).filter_by(uuid=source_uuid).one()
 
         self._import(key_data)
 
         local_source.fingerprint = fingerprint
-        self.session.add(local_source)
-        self.session.commit()
+        session.add(local_source)
+        session.commit()
 
     def _import(self, key_data: str) -> None:
         '''Wrapper for `gpg --import-keys`'''
@@ -145,7 +144,8 @@ def encrypt_to_source(self, source_uuid: str, data: str) -> str:
         '''
         :param data: A string of data to encrypt to a source.
         '''
-        source = self.session.query(Source).filter_by(uuid=source_uuid).one()
+        session = self.session_maker()
+        source = session.query(Source).filter_by(uuid=source_uuid).one()
         cmd = self._gpg_cmd_base()
 
         with tempfile.NamedTemporaryFile('w+') as content, \
diff --git a/securedrop_client/db.py b/securedrop_client/db.py
index 5f159c42c..d540cbdd9 100644
--- a/securedrop_client/db.py
+++ b/securedrop_client/db.py
@@ -4,9 +4,8 @@
 
 from sqlalchemy import Boolean, Column, create_engine, DateTime, ForeignKey, Integer, String, \
     Text, MetaData, CheckConstraint, text, UniqueConstraint
-from sqlalchemy.engine import Engine
 from sqlalchemy.ext.declarative import declarative_base
-from sqlalchemy.orm import relationship, backref
+from sqlalchemy.orm import relationship, backref, scoped_session, sessionmaker
 
 
 convention = {
@@ -22,9 +21,11 @@
 Base = declarative_base(metadata=metadata)  # type: Any
 
 
-def make_engine(home: str) -> Engine:
+def make_session_maker(home: str) -> scoped_session:
     db_path = os.path.join(home, 'svs.sqlite')
-    return create_engine('sqlite:///{}'.format(db_path))
+    engine = create_engine('sqlite:///{}'.format(db_path))
+    maker = sessionmaker(bind=engine)
+    return scoped_session(maker)
 
 
 class Source(Base):
diff --git a/securedrop_client/gui/main.py b/securedrop_client/gui/main.py
index 06aef76dd..f02b63284 100644
--- a/securedrop_client/gui/main.py
+++ b/securedrop_client/gui/main.py
@@ -20,11 +20,12 @@
 along with this program.  If not, see <http://www.gnu.org/licenses/>.
 """
 import logging
+
 from gettext import gettext as _
 from typing import Dict, List, Optional  # noqa: F401
-
 from PyQt5.QtWidgets import QMainWindow, QWidget, QHBoxLayout, QVBoxLayout, QDesktopWidget, \
     QApplication
+from sqlalchemy.orm import scoped_session
 
 from securedrop_client import __version__
 from securedrop_client.db import Source
@@ -43,7 +44,7 @@ class Window(QMainWindow):
 
     icon = 'icon.png'
 
-    def __init__(self):
+    def __init__(self, session_maker: scoped_session) -> None:
         """
         Create the default start state. The window contains a root widget into
         which is placed:
@@ -68,7 +69,7 @@ def __init__(self):
         layout.setSpacing(0)
         self.main_pane.setLayout(layout)
         self.left_pane = LeftPane()
-        self.main_view = MainView(self.main_pane)
+        self.main_view = MainView(session_maker, self.main_pane)
         layout.addWidget(self.left_pane, 1)
         layout.addWidget(self.main_view, 8)
 
diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py
index da3e24965..1eb23c169 100644
--- a/securedrop_client/gui/widgets.py
+++ b/securedrop_client/gui/widgets.py
@@ -18,17 +18,18 @@
 """
 import logging
 import arrow
-from gettext import gettext as _
 import html
 import sys
+
+from gettext import gettext as _
 from typing import List
 from uuid import uuid4
-
-from PyQt5.QtCore import Qt, pyqtSlot, pyqtSignal, QTimer, QSize
+from PyQt5.QtCore import Qt, pyqtSlot, pyqtSignal, QTimer, QSize, pyqtBoundSignal, QObject
 from PyQt5.QtGui import QIcon, QPalette, QBrush, QColor, QFont, QLinearGradient
 from PyQt5.QtWidgets import QListWidget, QLabel, QWidget, QListWidgetItem, QHBoxLayout, \
     QPushButton, QVBoxLayout, QLineEdit, QScrollArea, QDialog, QAction, QMenu, QMessageBox, \
     QToolButton, QSizePolicy, QTextEdit, QStatusBar, QGraphicsDropShadowEffect
+from sqlalchemy.orm import scoped_session
 
 from securedrop_client.db import Source, Message, File, Reply
 from securedrop_client.storage import source_exists
@@ -587,8 +588,9 @@ class MainView(QWidget):
     }
     '''
 
-    def __init__(self, parent):
+    def __init__(self, session_maker: scoped_session, parent: QObject):
         super().__init__(parent)
+        self.session_maker = session_maker
 
         self.setStyleSheet(self.CSS)
 
@@ -631,7 +633,11 @@ def on_source_changed(self):
         source = self.source_list.get_current_source()
 
         if source:
-            conversation_wrapper = SourceConversationWrapper(source, self.controller)
+            conversation_wrapper = SourceConversationWrapper(
+                self.session_maker,
+                source,
+                self.controller,
+            )
             self.set_conversation(conversation_wrapper)
         else:
             self.clear_conversation()
@@ -1274,31 +1280,46 @@ class FileWidget(QWidget):
     Represents a file.
     """
 
-    def __init__(self, source_db_object, submission_db_object,
-                 controller, file_ready_signal, align="left"):
+    def __init__(
+        self,
+        session_maker: scoped_session,
+        file_uuid: str,
+        controller: Controller,
+        file_ready_signal: pyqtBoundSignal,
+    ) -> None:
         """
         Given some text and a reference to the controller, make something to display a file.
         """
         super().__init__()
+        self.session_maker = session_maker
         self.controller = controller
-        self.source = source_db_object
-        self.submission = submission_db_object
-        self.file_uuid = self.submission.uuid
+        self.file_uuid = file_uuid
+        self.file_is_downloaded = False  # default to `False`, value updated in `update()`
 
         self.layout = QHBoxLayout()
         self.update()
         self.setLayout(self.layout)
 
-        file_ready_signal.connect(self._on_file_download)
+        file_ready_signal.connect(self._on_file_download, type=Qt.QueuedConnection)
 
     def update(self) -> None:
         icon = QLabel()
         icon.setPixmap(load_image('file.png'))
 
-        if self.submission.is_downloaded:
+        session = self.session_maker()
+
+        # we have to query to get the object we want
+        file_ = session.query(File).filter_by(uuid=self.file_uuid).one()
+        # and then force a refresh because SQLAlchemy might have a copy of this object
+        # in this thread already that isn't up to date
+        session.refresh(file_)
+
+        self.file_is_downloaded = file_.is_downloaded
+
+        if self.file_is_downloaded:
             description = QLabel("Open")
         else:
-            human_filesize = humanize_filesize(self.submission.size)
+            human_filesize = humanize_filesize(file_.size)
             description = QLabel("Download ({})".format(human_filesize))
 
         self.layout.addWidget(icon)
@@ -1322,12 +1343,12 @@ def mouseReleaseEvent(self, e):
         Handle a completed click via the program logic. The download state
         of the file distinguishes which function in the logic layer to call.
         """
-        if self.submission.is_downloaded:
+        if self.file_is_downloaded:
             # Open the already downloaded file.
-            self.controller.on_file_open(self.submission)
+            self.controller.on_file_open(self.file_uuid)
         else:
             # Download the file.
-            self.controller.on_file_download(self.source, self.submission)
+            self.controller.on_submission_download(File, self.file_uuid)
 
 
 class ConversationView(QWidget):
@@ -1339,8 +1360,14 @@ class ConversationView(QWidget):
     https://github.com/freedomofpress/securedrop-client/issues/273
     """
 
-    def __init__(self, source_db_object: Source, controller: Controller):
+    def __init__(
+        self,
+        session_maker: scoped_session,
+        source_db_object: Source,
+        controller: Controller,
+    ):
         super().__init__()
+        self.session_maker = session_maker
         self.source = source_db_object
         self.controller = controller
 
@@ -1391,8 +1418,13 @@ def add_file(self, source_db_object, submission_db_object):
         Add a file from the source.
         """
         self.conversation_layout.addWidget(
-            FileWidget(source_db_object, submission_db_object,
-                       self.controller, self.controller.file_ready))
+            FileWidget(
+                self.session_maker,
+                submission_db_object.uuid,
+                self.controller,
+                self.controller.file_ready,
+            ),
+        )
 
     def update_conversation_position(self, min_val, max_val):
         """
@@ -1460,15 +1492,19 @@ class SourceConversationWrapper(QWidget):
     per-source resources.
     """
 
-    def __init__(self, source: Source, controller: Controller) -> None:
+    def __init__(
+        self,
+        session_maker: scoped_session,
+        source: Source,
+        controller: Controller,
+    ) -> None:
         super().__init__()
-
         layout = QVBoxLayout()
         layout.setContentsMargins(0, 0, 0, 0)
         self.setLayout(layout)
 
         self.conversation_title_bar = SourceProfileShortWidget(source, controller)
-        self.conversation_view = ConversationView(source, controller)
+        self.conversation_view = ConversationView(session_maker, source, controller)
         self.reply_box = ReplyBoxWidget(source, controller)
 
         layout.addWidget(self.conversation_title_bar, 1)
diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index d3f98b9b1..f69634161 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -29,6 +29,7 @@
 from PyQt5.QtCore import QObject, QThread, pyqtSignal, QTimer, QProcess
 from sdclientapi import RequestTimeoutError
 from typing import Dict, Tuple  # noqa: F401
+from sqlalchemy.orm.session import sessionmaker
 
 from securedrop_client import storage
 from securedrop_client import db
@@ -117,7 +118,7 @@ class Controller(QObject):
     """
     file_ready = pyqtSignal(str)
 
-    def __init__(self, hostname, gui, session,
+    def __init__(self, hostname: str, gui, session_maker: sessionmaker,
                  home: str, proxy: bool = True) -> None:
         """
         The hostname, gui and session objects are used to coordinate with the
@@ -144,27 +145,29 @@ def __init__(self, hostname, gui, session,
 
         # Reference to the API for secure drop proxy.
         self.api = None  # type: sdclientapi.API
+
+        # Reference to the SqlAlchemy `sessionmaker` and `session`
+        self.session_maker = session_maker
+        self.session = session_maker()
+
         # Contains active threads calling the API.
         self.api_threads = {}  # type: Dict[str, Dict]
 
-        # Reference to the SqlAlchemy session.
-        self.session = session
+        self.gpg = GpgHelper(home, self.session_maker, proxy)
 
         # thread responsible for fetching messages
         self.message_thread = None
-        self.message_sync = MessageSync(self.api, self.home, self.proxy)
+        self.message_sync = MessageSync(self.api, self.gpg, self.session_maker)
 
         # thread responsible for fetching replies
         self.reply_thread = None
-        self.reply_sync = ReplySync(self.api, self.home, self.proxy)
+        self.reply_sync = ReplySync(self.api, self.gpg, self.session_maker)
 
         self.sync_flag = os.path.join(home, 'sync_flag')
 
         # File data.
         self.data_dir = os.path.join(self.home, 'data')
 
-        self.gpg = GpgHelper(home, proxy)
-
     @property
     def is_authenticated(self) -> bool:
         return self.__is_authenticated
diff --git a/securedrop_client/message_sync.py b/securedrop_client/message_sync.py
index 42bdf803a..4ddd7b792 100644
--- a/securedrop_client/message_sync.py
+++ b/securedrop_client/message_sync.py
@@ -28,9 +28,9 @@
 from PyQt5.QtCore import QObject, pyqtSignal
 from securedrop_client import storage
 from securedrop_client.crypto import GpgHelper, CryptoError
-from securedrop_client.db import make_engine, File, Message, Reply
-from sqlalchemy.orm import sessionmaker
-from sqlalchemy.orm.session import Session  # noqa: F401
+from securedrop_client.db import File, Message, Reply
+from sqlalchemy.orm import scoped_session
+from sqlalchemy.orm.session import Session
 from tempfile import NamedTemporaryFile
 
 
@@ -39,35 +39,46 @@
 
 class APISyncObject(QObject):
 
-    def __init__(self, api: API, home: str, is_qubes: bool) -> None:
+    def __init__(
+        self,
+        api: API,
+        gpg: GpgHelper,
+        session_maker: scoped_session,
+    ) -> None:
         super().__init__()
-
-        engine = make_engine(home)
-        current_session = sessionmaker(bind=engine)
-        self.session = current_session()  # type: Session
         self.api = api
-        self.home = home
-        self.is_qubes = is_qubes
-        self.gpg = GpgHelper(home, is_qubes)
-
-    def decrypt_the_thing(self, filepath: str, msg: Union[File, Message, Reply]) -> None:
+        self.gpg = gpg
+        self.session_maker = session_maker
+
+    def decrypt_the_thing(
+        self,
+        session: Session,
+        filepath: str,
+        msg: Union[File, Message, Reply],
+    ) -> None:
         with NamedTemporaryFile('w+') as plaintext_file:
             try:
                 self.gpg.decrypt_submission_or_reply(filepath, plaintext_file.name, False)
                 plaintext_file.seek(0)
                 content = plaintext_file.read()
-                storage.set_object_decryption_status_with_content(msg, self.session, True, content)
+                storage.set_object_decryption_status_with_content(msg, session, True, content)
                 logger.info("Message or reply decrypted: {}".format(msg.filename))
             except CryptoError:
-                storage.set_object_decryption_status_with_content(msg, self.session, False)
+                storage.set_object_decryption_status_with_content(msg, session, False)
                 logger.info("Message or reply failed to decrypt: {}".format(msg.filename))
 
-    def fetch_the_thing(self, item: Union[File, Message, Reply], msg: Union[File, Message, Reply],
-                        download_fn: Callable, update_fn: Callable) -> None:
+    def fetch_the_thing(
+        self,
+        session: Session,
+        item: Union[File, Message, Reply],
+        msg: Union[File, Message, Reply],
+        download_fn: Callable,
+        update_fn: Callable,
+    ) -> None:
         _, filepath = download_fn(item)
-        update_fn(msg.uuid, self.session)
+        update_fn(msg.uuid, session)
         logger.info("Stored message or reply at {}".format(msg.filename))
-        self.decrypt_the_thing(filepath, msg)
+        self.decrypt_the_thing(session, filepath, msg)
 
 
 class MessageSync(APISyncObject):
@@ -81,12 +92,10 @@ class MessageSync(APISyncObject):
     """
     message_ready = pyqtSignal([str, str])
 
-    def __init__(self, api: API, home: str, is_qubes: bool):
-        super().__init__(api, home, is_qubes)
-
     def run(self, loop: bool = True) -> None:
+        session = self.session_maker()
         while True:
-            submissions = storage.find_new_messages(self.session)
+            submissions = storage.find_new_messages(session)
 
             for db_submission in submissions:
                 try:
@@ -99,13 +108,15 @@ def run(self, loop: bool = True) -> None:
 
                     if not db_submission.is_downloaded and self.api:
                         # Download and decrypt
-                        self.fetch_the_thing(sdk_submission,
+                        self.fetch_the_thing(session,
+                                             sdk_submission,
                                              db_submission,
                                              self.api.download_submission,
                                              storage.mark_message_as_downloaded)
                     elif db_submission.is_downloaded:
                         # Just decrypt file that is already on disk
-                        self.decrypt_the_thing(db_submission.filename,
+                        self.decrypt_the_thing(session,
+                                               db_submission.filename,
                                                db_submission)
 
                     if db_submission.content is not None:
@@ -137,12 +148,10 @@ class ReplySync(APISyncObject):
     """
     reply_ready = pyqtSignal([str, str])
 
-    def __init__(self, api: API, home: str, is_qubes: bool):
-        super().__init__(api, home, is_qubes)
-
     def run(self, loop: bool = True) -> None:
+        session = self.session_maker()
         while True:
-            replies = storage.find_new_replies(self.session)
+            replies = storage.find_new_replies(session)
 
             for db_reply in replies:
                 try:
@@ -158,13 +167,15 @@ def run(self, loop: bool = True) -> None:
 
                     if not db_reply.is_downloaded and self.api:
                         # Download and decrypt
-                        self.fetch_the_thing(sdk_reply,
+                        self.fetch_the_thing(session,
+                                             sdk_reply,
                                              db_reply,
                                              self.api.download_reply,
                                              storage.mark_reply_as_downloaded)
                     elif db_reply.is_downloaded:
                         # Just decrypt file that is already on disk
-                        self.decrypt_the_thing(db_reply.filename,
+                        self.decrypt_the_thing(session,
+                                               db_reply.filename,
                                                db_reply)
 
                     if db_reply.content is not None:

From 6618a0e3eb194c20ace176860e1eb0fca6e8403a Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 11:50:35 +0200
Subject: [PATCH 03/17] created ApiJobQueue and related classes

---
 Makefile                   |   3 +-
 securedrop_client/queue.py | 235 +++++++++++++++++++++++++++++++++++++
 2 files changed, 237 insertions(+), 1 deletion(-)
 create mode 100644 securedrop_client/queue.py

diff --git a/Makefile b/Makefile
index 43d16b150..927a0ec92 100644
--- a/Makefile
+++ b/Makefile
@@ -14,7 +14,8 @@ mypy: ## Run static type checker
 		securedrop_client/gui/__init__.py \
 		securedrop_client/resources/__init__.py \
 		securedrop_client/storage.py \
-		securedrop_client/message_sync.py
+		securedrop_client/message_sync.py \
+		securedrop_client/queue.py
 
 .PHONY: clean
 clean:  ## Clean the workspace of generated resources
diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py
new file mode 100644
index 000000000..8874ec15d
--- /dev/null
+++ b/securedrop_client/queue.py
@@ -0,0 +1,235 @@
+import binascii
+import hashlib
+import logging
+import os
+import sdclientapi
+import shutil
+
+from PyQt5.QtCore import QObject, QThread, pyqtSlot, pyqtSignal
+from PyQt5.QtWidgets import QApplication
+from queue import Queue
+from sdclientapi import API, RequestTimeoutError, AuthError
+from sqlalchemy.orm import scoped_session
+from sqlalchemy.orm.session import Session
+from typing import Any, Union, Optional, Type, Tuple
+
+from securedrop_client import storage
+from securedrop_client.crypto import GpgHelper, CryptoError
+from securedrop_client.db import File, Message
+
+
+logger = logging.getLogger(__name__)
+
+
+class ApiInaccessibleError(Exception):
+
+    def __init__(self, message: Optional[str] = None) -> None:
+        if not message:
+            message = ('API is inaccessible either because there is no client or because the '
+                       'client is not properly authenticated.')
+        super().__init__(message)
+
+
+class ApiJob(QObject):
+
+    '''
+    Signal that is emitted after an job finishes successfully.
+    '''
+    success_signal = pyqtSignal('PyQt_PyObject')
+
+    '''
+    Signal that is emitted if there is a failure during the job.
+    '''
+    failure_signal = pyqtSignal(Exception)
+
+    def __init__(self) -> None:
+        super().__init__(None)  # `None` because the QOjbect has no parent
+
+    def _do_call_api(self, api_client: API, session: Session) -> None:
+        if not api_client:
+            raise ApiInaccessibleError()
+
+        try:
+            result = self.call_api(api_client, session)
+        except AuthError as e:
+            raise ApiInaccessibleError() from e
+        except RequestTimeoutError:
+            logger.debug('Job {} timed out'.format(self))
+            raise
+        except Exception as e:
+            logger.error('Job {} raised an exception: {}: {}'
+                         .format(self, type(e).__name__, e))
+            self.failure_signal.emit(e)
+        else:
+            self.success_signal.emit(result)
+
+    def call_api(self, api_client: API, session: Session) -> Any:
+        '''
+        Method for making the actual API call and handling the result.
+
+        This MUST resturn a value if the API call and other tasks were successful and MUST raise
+        an exception if and only iff the tasks failed. Presence of a raise exception indicates a
+        failure.
+        '''
+        raise NotImplementedError
+
+
+class DownloadSubmissionJob(ApiJob):
+
+    CHUNK_SIZE = 4096
+
+    def __init__(
+        self,
+        submission_type: Union[Type[File], Type[Message]],
+        submission_uuid: str,
+        data_dir: str,
+        gpg: GpgHelper,
+    ) -> None:
+        super().__init__()
+        self.data_dir = data_dir
+        self.submission_type = submission_type
+        self.submission_uuid = submission_uuid
+        self.gpg = gpg
+
+    def call_api(self, api_client: API, session: Session) -> Any:
+        db_object = session.query(self.submission_type) \
+            .filter_by(uuid=self.submission_uuid).one()
+
+        etag, download_path = self._make_call(db_object, api_client)
+
+        if not self._check_file_integrity(etag, download_path):
+            raise RuntimeError('Downloaded file had an invalid checksum.')
+
+        self._decrypt_file(session, db_object, download_path)
+
+        return db_object.uuid
+
+    def _make_call(self, db_object: Union[File, Message], api_client: API) -> Tuple[str, str]:
+        sdk_obj = sdclientapi.Submission(uuid=db_object.uuid)
+        sdk_obj.filename = db_object.filename
+        sdk_obj.source_uuid = db_object.source.uuid
+
+        return api_client.download_submission(sdk_obj)
+
+    @classmethod
+    def _check_file_integrity(cls, etag: str, file_path: str) -> bool:
+        '''
+        Checks if the file is valid.
+        :return: `True` if valid or unknown, `False` otherwise.
+        '''
+        if not etag:
+            logger.debug('No ETag. Skipping integrity check for file at {}'.format(file_path))
+            return True
+
+        alg, checksum = etag.split(':')
+
+        if alg == 'sha256':
+            hasher = hashlib.sha256()
+        else:
+            logger.debug('Unknown hash algorithm ({}). Skipping integrity check for file at {}'
+                         .format(alg, file_path))
+            return True
+
+        with open(file_path, 'rb') as f:
+            while True:
+                read_bytes = f.read(cls.CHUNK_SIZE)
+                if not read_bytes:
+                    break
+                hasher.update(read_bytes)
+
+        calculated_checksum = binascii.hexlify(hasher.digest()).decode('utf-8')
+        return calculated_checksum == checksum
+
+    def _decrypt_file(
+        self,
+        session: Session,
+        db_object: Union[File, Message],
+        file_path: str,
+    ) -> None:
+        file_uuid = db_object.uuid
+        server_filename = db_object.filename
+
+        # The filename contains the location where the file has been stored. On non-Qubes OSes, this
+        # will be the data directory. On Qubes OS, this will a ~/QubesIncoming directory. In case we
+        # are on Qubes, we should move the file to the data directory and name it the same as the
+        # server (e.g. spotless-tater-msg.gpg).
+        filepath_in_datadir = os.path.join(self.data_dir, server_filename)
+        shutil.move(file_path, filepath_in_datadir)
+        storage.mark_file_as_downloaded(file_uuid, session)
+
+        try:
+            self.gpg.decrypt_submission_or_reply(filepath_in_datadir, server_filename, is_doc=True)
+        except CryptoError as e:
+            logger.debug('Failed to decrypt file {}: {}'.format(server_filename, e))
+            storage.set_object_decryption_status_with_content(db_object, session, False)
+            raise e
+
+        storage.set_object_decryption_status_with_content(db_object, session, True)
+
+
+class RunnableQueue(QObject):
+
+    def __init__(self, api_client: API, session_maker: scoped_session) -> None:
+        super().__init__()
+        self.api_client = api_client
+        self.session_maker = session_maker
+        self.queue = Queue()  # type: Queue[ApiJob]
+        self.last_job = None  # type: Optional[ApiJob]
+
+    @pyqtSlot()
+    def process(self) -> None:  # pragma: nocover
+        self.__process(False)
+
+    def __process(self, exit_loop: bool) -> None:
+        session = self.session_maker()
+        while True:
+            # retry the "cached" job if it exists, otherwise get the next job
+            if self.last_job is not None:
+                job = self.last_job
+                self.last_job = None
+            else:
+                job = self.queue.get(block=True)
+
+            try:
+                job._do_call_api(self.api_client, session)
+            except RequestTimeoutError:
+                self.last_job = job  # "cache" the last job since we can't re-queue it
+                return
+
+            # process events to allow this thread to handle incoming signals
+            QApplication.processEvents()
+
+            if exit_loop:
+                return
+
+
+class ApiJobQueue(QObject):
+
+    def __init__(self, api_client: API, session_maker: scoped_session) -> None:
+        super().__init__(None)
+        self.api_client = api_client
+
+        self.main_thread = QThread()
+        self.download_thread = QThread()
+
+        self.main_queue = RunnableQueue(self.api_client, session_maker)
+        self.download_queue = RunnableQueue(self.api_client, session_maker)
+
+        self.main_queue.moveToThread(self.main_thread)
+        self.download_queue.moveToThread(self.download_thread)
+
+        self.main_thread.started.connect(self.main_queue.process)
+        self.download_thread.started.connect(self.download_queue.process)
+
+    def start_queues(self, api_client: API) -> None:
+        self.main_queue.api_client = api_client
+        self.download_queue.api_client = api_client
+
+        self.main_thread.start()
+        self.download_thread.start()
+
+    def enqueue(self, job: ApiJob) -> None:
+        if isinstance(job, DownloadSubmissionJob):
+            self.download_queue.queue.put_nowait(job)
+        else:
+            self.main_queue.queue.put_nowait(job)

From 90523f2139530c8dba661b6f8116d3de65224757 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 11:50:51 +0200
Subject: [PATCH 04/17] moved submission downloads to use new api queue

---
 securedrop_client/logic.py | 100 ++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 58 deletions(-)

diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index f69634161..704fcf38e 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -21,21 +21,21 @@
 import logging
 import os
 import sdclientapi
-import shutil
 import traceback
 import uuid
 
 from gettext import gettext as _
-from PyQt5.QtCore import QObject, QThread, pyqtSignal, QTimer, QProcess
+from PyQt5.QtCore import QObject, QThread, pyqtSignal, QTimer, QProcess, Qt
 from sdclientapi import RequestTimeoutError
-from typing import Dict, Tuple  # noqa: F401
 from sqlalchemy.orm.session import sessionmaker
+from typing import Dict, Tuple, Union, Any, Type  # noqa: F401
 
 from securedrop_client import storage
 from securedrop_client import db
-from securedrop_client.utils import check_dir_permissions
 from securedrop_client.crypto import GpgHelper, CryptoError
 from securedrop_client.message_sync import MessageSync, ReplySync
+from securedrop_client.queue import ApiJobQueue, DownloadSubmissionJob
+from securedrop_client.utils import check_dir_permissions
 
 logger = logging.getLogger(__name__)
 
@@ -150,6 +150,9 @@ def __init__(self, hostname: str, gui, session_maker: sessionmaker,
         self.session_maker = session_maker
         self.session = session_maker()
 
+        # Queue that handles running API job
+        self.api_job_queue = ApiJobQueue(self.api, self.session_maker)
+
         # Contains active threads calling the API.
         self.api_threads = {}  # type: Dict[str, Dict]
 
@@ -311,9 +314,12 @@ def on_authenticate_success(self, result):
         self.gui.hide_login()
         self.sync_api()
         self.gui.show_main_window(self.api.username)
+
         self.start_message_thread()
         self.start_reply_thread()
 
+        self.api_job_queue.start_queues(self.api)
+
         # Clear the sidebar error status bar if a message was shown
         # to the user indicating they should log in.
         self.gui.clear_error_status()
@@ -512,79 +518,57 @@ def on_file_open(self, file_db_object):
             # Non Qubes OS. Just log the event for now.
             logger.info('Opening file "{}".'.format(submission_filepath))
 
-    def on_file_download(self, source_db_object, message):
+    def on_reply_download(self, source_db_object: db.Source, reply: db.Reply) -> None:
         """
-        Download the file associated with the associated message (which may
-        be a Submission or Reply).
+        Download the file associated with the Reply.
         """
         if not self.api:  # Then we should tell the user they need to login.
             self.on_action_requiring_login()
             return
 
-        if isinstance(message, db.File) or isinstance(message, db.Message):
-            # Handle submissions.
-            func = self.api.download_submission
-            sdk_object = sdclientapi.Submission(uuid=message.uuid)
-            sdk_object.filename = message.filename
-            sdk_object.source_uuid = source_db_object.uuid
-        elif isinstance(message, db.Reply):
-            # Handle journalist's replies.
-            func = self.api.download_reply
-            sdk_object = sdclientapi.Reply(uuid=message.uuid)
-            sdk_object.filename = message.filename
-            sdk_object.source_uuid = source_db_object.uuid
+        sdk_object = sdclientapi.Reply(uuid=reply.uuid)
+        sdk_object.filename = reply.filename
+        sdk_object.source_uuid = source_db_object.uuid
 
         self.set_status(_('Downloading {}'.format(sdk_object.filename)))
-        self.call_api(func,
+
+        self.call_api(self.api.download_reply,
                       self.on_file_download_success,
                       self.on_file_download_failure,
                       sdk_object,
                       self.data_dir,
-                      current_object=message)
+                      current_object=reply)
 
-    def on_file_download_success(self, result, current_object):
+    def on_submission_download(
+        self,
+        submission_type: Union[Type[db.File], Type[db.Message]],
+        submission_uuid: str,
+    ) -> None:
         """
-        Called when a file has downloaded. Cause a refresh to the conversation view to display the
-        contents of the new file.
+        Download the file associated with the Submission (which may be a File or Message).
         """
-        file_uuid = current_object.uuid
-        server_filename = current_object.filename
-        _, filename = result
-        # The filename contains the location where the file has been
-        # stored. On non-Qubes OSes, this will be the data directory.
-        # On Qubes OS, this will a ~/QubesIncoming directory. In case
-        # we are on Qubes, we should move the file to the data directory
-        # and name it the same as the server (e.g. spotless-tater-msg.gpg).
-        filepath_in_datadir = os.path.join(self.data_dir, server_filename)
-        shutil.move(filename, filepath_in_datadir)
-        storage.mark_file_as_downloaded(file_uuid, self.session)
+        job = DownloadSubmissionJob(
+            submission_type,
+            submission_uuid,
+            self.data_dir,
+            self.gpg,
+        )
+        job.success_signal.connect(self.on_file_download_success, type=Qt.QueuedConnection)
+        job.failure_signal.connect(self.on_file_download_failure, type=Qt.QueuedConnection)
 
-        try:
-            # Attempt to decrypt the file.
-            self.gpg.decrypt_submission_or_reply(
-                filepath_in_datadir, server_filename, is_doc=True)
-            storage.set_object_decryption_status_with_content(
-                current_object, self.session, True)
-        except CryptoError as e:
-            logger.debug('Failed to decrypt file {}: {}'.format(server_filename, e))
-            storage.set_object_decryption_status_with_content(
-                current_object, self.session, False)
-            self.set_status("Failed to decrypt file, "
-                            "please try again or talk to your administrator.")
-            # TODO: We should save the downloaded content, and just
-            # try to decrypt again if there was a failure.
-            return  # If we failed we should stop here.
-
-        self.set_status('Finished downloading {}'.format(current_object.filename))
-        self.file_ready.emit(file_uuid)
-
-    def on_file_download_failure(self, result, current_object):
+        self.api_job_queue.enqueue(job)
+        self.set_status(_('Downloading file'))
+
+    def on_file_download_success(self, result: Any) -> None:
+        """
+        Called when a file has downloaded.
+        """
+        self.file_ready.emit(result)
+
+    def on_file_download_failure(self, exception: Exception) -> None:
         """
         Called when a file fails to download.
         """
-        server_filename = current_object.filename
-        logger.debug('Failed to download file {}'.format(server_filename))
-        # Update the UI in some way to indicate a failure state.
         self.set_status("The file download failed. Please try again.")
 
     def on_delete_source_success(self, result) -> None:

From 14e49f9d0cfc301c29110665ace6229eeb3d5e15 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 15:59:29 +0200
Subject: [PATCH 05/17] remove unnecessray re-definition of User.__init__

---
 securedrop_client/db.py      | 3 ---
 securedrop_client/storage.py | 2 +-
 tests/test_models.py         | 6 +++---
 tests/test_storage.py        | 2 +-
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/securedrop_client/db.py b/securedrop_client/db.py
index d540cbdd9..ef023a834 100644
--- a/securedrop_client/db.py
+++ b/securedrop_client/db.py
@@ -216,8 +216,5 @@ class User(Base):
     uuid = Column(String(36), unique=True, nullable=False)
     username = Column(String(255), nullable=False)
 
-    def __init__(self, username: str) -> None:
-        self.username = username
-
     def __repr__(self) -> str:
         return "<Journalist: {}>".format(self.username)
diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py
index 859ecdd3a..c64c31996 100644
--- a/securedrop_client/storage.py
+++ b/securedrop_client/storage.py
@@ -303,7 +303,7 @@ def find_or_create_user(uuid: str, username: str, session: Session) -> User:
         return user
     else:
         # User does not exist in the local database.
-        new_user = User(username)
+        new_user = User(username=username)
         new_user.uuid = uuid
         session.add(new_user)
         session.commit()
diff --git a/tests/test_models.py b/tests/test_models.py
index 388f64081..af9f2da14 100644
--- a/tests/test_models.py
+++ b/tests/test_models.py
@@ -5,7 +5,7 @@
 
 
 def test_string_representation_of_user():
-    user = User('hehe')
+    user = User(username='hehe')
     user.__repr__()
 
 
@@ -29,7 +29,7 @@ def test_string_representation_of_file():
 
 
 def test_string_representation_of_reply():
-    user = User('hehe')
+    user = User(username='hehe')
     source = factory.Source()
     reply = Reply(source=source, journalist=user, filename="1-reply.gpg",
                   size=1234, uuid='test')
@@ -43,7 +43,7 @@ def test_source_collection():
                  download_url='http://test/test')
     message = Message(source=source, uuid="test", size=123, filename="3-test.doc.gpg",
                       download_url='http://test/test')
-    user = User('hehe')
+    user = User(username='hehe')
     reply = Reply(source=source, journalist=user, filename="1-reply.gpg",
                   size=1234, uuid='test')
     source.files = [file_]
diff --git a/tests/test_storage.py b/tests/test_storage.py
index 707cc8cc4..c550658dc 100644
--- a/tests/test_storage.py
+++ b/tests/test_storage.py
@@ -425,7 +425,7 @@ def test_update_sources_deletes_files_associated_with_the_source(
     msg_submission = db.File(
         source=local_source, uuid="test", size=123, filename=msg_server_filename,
         download_url='http://test/test')
-    user = db.User('hehe')
+    user = db.User(username='hehe')
     reply = db.Reply(
         source=local_source, journalist=user, filename=reply_server_filename,
         size=1234, uuid='test')

From 1d6fb3fd7cf71bd8446723be1c925c5d0c7e6333 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 16:00:09 +0200
Subject: [PATCH 06/17] added user factotry for tests

---
 tests/factory.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/factory.py b/tests/factory.py
index e9715a476..eb2a5255b 100644
--- a/tests/factory.py
+++ b/tests/factory.py
@@ -8,6 +8,20 @@
 MESSAGE_COUNT = 0
 FILE_COUNT = 0
 REPLY_COUNT = 0
+USER_COUNT = 0
+
+
+def User(**attrs):
+    global USER_COUNT
+    USER_COUNT += 1
+    defaults = dict(
+        uuid='user-uuid-{}'.format(USER_COUNT),
+        username='test-user-id-{}'.format(USER_COUNT),
+    )
+
+    defaults.update(attrs)
+
+    return db.User(**defaults)
 
 
 def Source(**attrs):

From f796f6d810adceaf43f6dbd63a71129bd0c40f55 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 16:00:25 +0200
Subject: [PATCH 07/17] added session_maker fixture

---
 tests/conftest.py | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tests/conftest.py b/tests/conftest.py
index be27be49c..9b397ede5 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -7,8 +7,7 @@
 from datetime import datetime
 from securedrop_client.config import Config
 from securedrop_client.app import configure_locale_and_language
-from securedrop_client.db import Base, make_engine, Source
-from sqlalchemy.orm import sessionmaker
+from securedrop_client.db import Base, make_session_maker, Source
 from uuid import uuid4
 
 
@@ -82,13 +81,16 @@ def _alembic_config(homedir):
 
 
 @pytest.fixture(scope='function')
-def session(homedir):
-    engine = make_engine(homedir)
-    Base.metadata.create_all(bind=engine, checkfirst=False)
-    Session = sessionmaker(bind=engine)
-    session = Session()
-    yield session
-    session.close()
+def session_maker(homedir):
+    return make_session_maker(homedir)
+
+
+@pytest.fixture(scope='function')
+def session(session_maker):
+    sess = session_maker()
+    Base.metadata.create_all(bind=sess.get_bind(), checkfirst=False)
+    yield sess
+    sess.close()
 
 
 @pytest.fixture(scope='function')

From 7bbe7bb854ffeced434424d995ccba0548f34c04 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 16:00:53 +0200
Subject: [PATCH 08/17] update alembic tests to use new session creation
 functions

---
 tests/test_alembic.py | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/tests/test_alembic.py b/tests/test_alembic.py
index 21e66cf14..327b9cee6 100644
--- a/tests/test_alembic.py
+++ b/tests/test_alembic.py
@@ -9,10 +9,9 @@
 from alembic.script import ScriptDirectory
 from os import path
 from sqlalchemy import text
-from sqlalchemy.orm import sessionmaker
 
 from . import conftest
-from securedrop_client.db import make_engine, Base, convention
+from securedrop_client.db import make_session_maker, Base, convention
 
 MIGRATION_PATH = path.join(path.dirname(__file__), '..', 'alembic', 'versions')
 
@@ -98,25 +97,24 @@ def test_alembic_head_matches_db_models(tmpdir):
     '''
     models_homedir = str(tmpdir.mkdir('models'))
     subprocess.check_call(['sqlite3', os.path.join(models_homedir, 'svs.sqlite'), '.databases'])
-    engine = make_engine(models_homedir)
-    Base.metadata.create_all(bind=engine, checkfirst=False)
+
+    session_maker = make_session_maker(models_homedir)
+    session = session_maker()
+    Base.metadata.create_all(bind=session.get_bind(), checkfirst=False)
     assert Base.metadata.naming_convention == convention
-    session = sessionmaker(engine)()
     models_schema = get_schema(session)
-    Base.metadata.drop_all(bind=engine)
+    Base.metadata.drop_all(bind=session.get_bind())
     session.close()
-    engine.dispose()
 
     alembic_homedir = str(tmpdir.mkdir('alembic'))
     subprocess.check_call(['sqlite3', os.path.join(alembic_homedir, 'svs.sqlite'), '.databases'])
-    engine = make_engine(alembic_homedir)
-    session = sessionmaker(engine)()
+    session_maker = make_session_maker(alembic_homedir)
+    session = session_maker()
     alembic_config = conftest._alembic_config(alembic_homedir)
     upgrade(alembic_config, 'head')
     alembic_schema = get_schema(session)
-    Base.metadata.drop_all(bind=engine)
+    Base.metadata.drop_all(bind=session.get_bind())
     session.close()
-    engine.dispose()
 
     # The initial migration creates the table 'alembic_version', but this is
     # not present in the schema created by `Base.metadata.create_all()`.
@@ -160,13 +158,13 @@ def test_schema_unchanged_after_up_then_downgrade(alembic_config,
         # get the database to some base state.
         pass
 
-    session = sessionmaker(make_engine(str(tmpdir.mkdir('original'))))()
+    session = make_session_maker(str(tmpdir.mkdir('original')))()
     original_schema = get_schema(session)
 
     upgrade(alembic_config, '+1')
     downgrade(alembic_config, '-1')
 
-    session = sessionmaker(make_engine(str(tmpdir.mkdir('reverted'))))()
+    session = make_session_maker(str(tmpdir.mkdir('reverted')))()
     reverted_schema = get_schema(session)
 
     # The initial migration is a degenerate case because it creates the table

From bd8f4025a6a04ba0d9ebe351341df0cd75cf159d Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 16:18:10 +0200
Subject: [PATCH 09/17] start porting tests to use new code

---
 tests/gui/test_main.py    |  57 +++--
 tests/gui/test_widgets.py |   1 +
 tests/test_app.py         |  14 +-
 tests/test_crypto.py      |  28 +-
 tests/test_logic.py       | 525 +++++++++++++++++---------------------
 5 files changed, 296 insertions(+), 329 deletions(-)

diff --git a/tests/gui/test_main.py b/tests/gui/test_main.py
index 28aebce4d..a46240aba 100644
--- a/tests/gui/test_main.py
+++ b/tests/gui/test_main.py
@@ -22,12 +22,13 @@ def test_init(mocker):
     mock_mv = mocker.patch('securedrop_client.gui.main.MainView')
     mocker.patch('securedrop_client.gui.main.QHBoxLayout', mock_lo)
     mocker.patch('securedrop_client.gui.main.QMainWindow')
+    mock_session_maker = mocker.MagicMock()
 
-    w = Window()
+    w = Window(mock_session_maker)
 
     mock_li.assert_called_once_with(w.icon)
     mock_lp.assert_called_once_with()
-    mock_mv.assert_called_once_with(w.main_pane)
+    mock_mv.assert_called_once_with(mock_session_maker, w.main_pane)
     assert mock_lo().addWidget.call_count == 2
 
 
@@ -36,8 +37,10 @@ def test_setup(mocker):
     Ensure the passed in controller is referenced and the various views are
     instantiated as expected.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
     mock_controller = mocker.MagicMock()
+
+    w = Window(mock_session_maker)
     w.show_login = mocker.MagicMock()
     w.top_pane = mocker.MagicMock()
     w.left_pane = mocker.MagicMock()
@@ -53,7 +56,8 @@ def test_setup(mocker):
 
 
 def test_show_main_window(mocker):
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.autosize_window = mocker.MagicMock()
     w.show = mocker.MagicMock()
     w.set_logged_in_as = mocker.MagicMock()
@@ -66,7 +70,8 @@ def test_show_main_window(mocker):
 
 
 def test_show_main_window_without_username(mocker):
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.autosize_window = mocker.MagicMock()
     w.show = mocker.MagicMock()
     w.set_logged_in_as = mocker.MagicMock()
@@ -82,7 +87,8 @@ def test_autosize_window(mocker):
     """
     Check the autosizing fits to the full screen size.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.resize = mocker.MagicMock()
     mock_screen = mocker.MagicMock()
     mock_screen.width.return_value = 1024
@@ -99,7 +105,8 @@ def test_show_login(mocker):
     """
     The login dialog is displayed with a clean state.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.controller = mocker.MagicMock()
     mock_ld = mocker.patch('securedrop_client.gui.main.LoginDialog')
 
@@ -114,7 +121,8 @@ def test_show_login_error(mocker):
     """
     Ensures that an error message is displayed in the login dialog.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.show_login = mocker.MagicMock()
     w.setup(mocker.MagicMock())
     w.login_dialog = mocker.MagicMock()
@@ -128,7 +136,8 @@ def test_hide_login(mocker):
     """
     Ensure the login dialog is closed and hidden.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.show_login = mocker.MagicMock()
     ld = mocker.MagicMock()
     w.login_dialog = ld
@@ -143,7 +152,8 @@ def test_show_sources(mocker):
     """
     Ensure the sources list is passed to the main view to be updated.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.main_view = mocker.MagicMock()
     w.show_sources([1, 2, 3])
     w.main_view.show_sources.assert_called_once_with([1, 2, 3])
@@ -154,7 +164,8 @@ def test_update_error_status_default(mocker):
     Ensure that the error to be shown in the error status bar will be passed to the top pane with a
     default duration of 10 seconds.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.top_pane = mocker.MagicMock()
     w.update_error_status(message='test error message')
     w.top_pane.update_error_status.assert_called_once_with('test error message', 10000)
@@ -165,7 +176,8 @@ def test_update_error_status(mocker):
     Ensure that the error to be shown in the error status bar will be passed to the top pane with
     the duration of seconds provided.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.top_pane = mocker.MagicMock()
     w.update_error_status(message='test error message', duration=123)
     w.top_pane.update_error_status.assert_called_once_with('test error message', 123)
@@ -176,7 +188,8 @@ def test_update_activity_status_default(mocker):
     Ensure that the activity to be shown in the activity status bar will be passed to the top pane
     with a default duration of 0 seconds, i.e. forever.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.top_pane = mocker.MagicMock()
     w.update_activity_status(message='test message')
     w.top_pane.update_activity_status.assert_called_once_with('test message', 0)
@@ -187,7 +200,8 @@ def test_update_activity_status(mocker):
     Ensure that the activity to be shown in the activity status bar will be passed to the top pane
     with the duration of seconds provided.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.top_pane = mocker.MagicMock()
     w.update_activity_status(message='test message', duration=123)
     w.top_pane.update_activity_status.assert_called_once_with('test message', 123)
@@ -197,7 +211,8 @@ def test_clear_error_status(mocker):
     """
     Ensure clear_error_status is called.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.top_pane = mocker.MagicMock()
 
     w.clear_error_status()
@@ -209,7 +224,8 @@ def test_show_sync(mocker):
     """
     If there's a value display the result of its "humanize" method.humanize
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.update_activity_status = mocker.MagicMock()
     updated_on = mocker.MagicMock()
     w.show_sync(updated_on)
@@ -221,7 +237,8 @@ def test_show_sync_no_sync(mocker):
     """
     If there's no value to display, default to a "waiting" message.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.update_activity_status = mocker.MagicMock()
     w.show_sync(None)
     w.update_activity_status.assert_called_once_with('Waiting to refresh...', 5000)
@@ -231,7 +248,8 @@ def test_set_logged_in_as(mocker):
     """
     Given a username, the left pane is appropriately called to update.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.left_pane = mocker.MagicMock()
 
     w.set_logged_in_as('test')
@@ -243,7 +261,8 @@ def test_logout(mocker):
     """
     Ensure the left pane updates to the logged out state.
     """
-    w = Window()
+    mock_session_maker = mocker.MagicMock()
+    w = Window(mock_session_maker)
     w.left_pane = mocker.MagicMock()
     w.top_pane = mocker.MagicMock()
 
diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py
index 65db593ab..42f6ce0cc 100644
--- a/tests/gui/test_widgets.py
+++ b/tests/gui/test_widgets.py
@@ -376,6 +376,7 @@ def test_MainView_init():
     """
     Ensure the MainView instance is correctly set up.
     """
+    mock_session_maker = mocker.MagicMock()
     mv = MainView(None)
     assert isinstance(mv.source_list, SourceList)
     assert isinstance(mv.view_holder, QWidget)
diff --git a/tests/test_app.py b/tests/test_app.py
index 56bef9782..61691a522 100644
--- a/tests/test_app.py
+++ b/tests/test_app.py
@@ -111,7 +111,7 @@ def test_start_app(homedir, mocker):
     """
     Ensure the expected things are configured and the application is started.
     """
-    mock_session_class = mocker.MagicMock()
+    mock_session_maker = mocker.MagicMock()
     mock_args = mocker.MagicMock()
     mock_qt_args = mocker.MagicMock()
     mock_args.sdc_home = str(homedir)
@@ -123,13 +123,13 @@ def test_start_app(homedir, mocker):
     mock_controller = mocker.patch('securedrop_client.app.Controller')
     mocker.patch('securedrop_client.app.prevent_second_instance')
     mocker.patch('securedrop_client.app.sys')
-    mocker.patch('securedrop_client.app.sessionmaker', return_value=mock_session_class)
+    mocker.patch('securedrop_client.app.make_session_maker', return_value=mock_session_maker)
 
     start_app(mock_args, mock_qt_args)
     mock_app.assert_called_once_with(mock_qt_args)
-    mock_win.assert_called_once_with()
+    mock_win.assert_called_once_with(mock_session_maker)
     mock_controller.assert_called_once_with('http://localhost:8081/',
-                                            mock_win(), mock_session_class(),
+                                            mock_win(), mock_session_maker,
                                             homedir, False)
 
 
@@ -170,7 +170,7 @@ def test_start_app(homedir, mocker):
 def test_create_app_dir_permissions(tmpdir, mocker):
 
     for idx, case in enumerate(PERMISSIONS_CASES):
-        mock_session_class = mocker.MagicMock()
+        mock_session_maker = mocker.MagicMock()
         mock_args = mocker.MagicMock()
         mock_qt_args = mocker.MagicMock()
 
@@ -192,7 +192,7 @@ def test_create_app_dir_permissions(tmpdir, mocker):
         mocker.patch('securedrop_client.app.Controller')
         mocker.patch('securedrop_client.app.sys')
         mocker.patch('securedrop_client.app.prevent_second_instance')
-        mocker.patch('securedrop_client.app.sessionmaker', return_value=mock_session_class)
+        mocker.patch('securedrop_client.app.make_session_maker', return_value=mock_session_maker)
 
         def func():
             start_app(mock_args, mock_qt_args)
@@ -245,7 +245,7 @@ def test_signal_interception(mocker, homedir):
     mocker.patch('securedrop_client.app.QApplication')
     mocker.patch('securedrop_client.app.prevent_second_instance')
     mocker.patch('sys.exit')
-    mocker.patch('securedrop_client.db.make_engine')
+    mocker.patch('securedrop_client.db.make_session_maker')
     mocker.patch('securedrop_client.app.init')
     mocker.patch('securedrop_client.logic.Controller.setup')
     mocker.patch('securedrop_client.logic.GpgHelper')
diff --git a/tests/test_crypto.py b/tests/test_crypto.py
index 6696c6447..82605baef 100644
--- a/tests/test_crypto.py
+++ b/tests/test_crypto.py
@@ -13,12 +13,12 @@
     JOURNO_KEY = f.read()
 
 
-def test_message_logic(homedir, config, mocker):
+def test_message_logic(homedir, config, mocker, session_maker):
     """
     Ensure that messages are handled.
     Using the `config` fixture to ensure the config is written to disk.
     """
-    gpg = GpgHelper(homedir, is_qubes=False)
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
 
     test_msg = 'tests/files/test-msg.gpg'
     expected_output_filename = 'test-msg'
@@ -32,12 +32,12 @@ def test_message_logic(homedir, config, mocker):
     assert dest == os.path.join(homedir, 'data', expected_output_filename)
 
 
-def test_gunzip_logic(homedir, config, mocker):
+def test_gunzip_logic(homedir, config, mocker, session_maker):
     """
     Ensure that gzipped documents/files are handled
     Using the `config` fixture to ensure the config is written to disk.
     """
-    gpg = GpgHelper(homedir, is_qubes=False)
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
 
     test_gzip = 'tests/files/test-doc.gz.gpg'
     expected_output_filename = 'test-doc'
@@ -52,12 +52,12 @@ def test_gunzip_logic(homedir, config, mocker):
     assert mock_unlink.call_count == 2
 
 
-def test_subprocess_raises_exception(homedir, config, mocker):
+def test_subprocess_raises_exception(homedir, config, mocker, session_maker):
     """
     Ensure that failed GPG commands raise an exception.
     Using the `config` fixture to ensure the config is written to disk.
     """
-    gpg = GpgHelper(homedir, is_qubes=False)
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
 
     test_gzip = 'tests/files/test-doc.gz.gpg'
     output_filename = 'test-doc'
@@ -73,21 +73,21 @@ def test_subprocess_raises_exception(homedir, config, mocker):
     assert mock_unlink.call_count == 1
 
 
-def test_import_key(homedir, config, source):
+def test_import_key(homedir, config, source, session_maker):
     '''
     Check the happy path that we can import a single PGP key.
     Using the `config` fixture to ensure the config is written to disk.
     '''
-    helper = GpgHelper(homedir, is_qubes=False)
+    helper = GpgHelper(homedir, session_maker, is_qubes=False)
     helper.import_key(source['uuid'], source['public_key'], source['fingerprint'])
 
 
-def test_import_key_gpg_call_fail(homedir, config, mocker):
+def test_import_key_gpg_call_fail(homedir, config, mocker, session_maker):
     '''
     Check that a `CryptoError` is raised if calling `gpg` fails.
     Using the `config` fixture to ensure the config is written to disk.
     '''
-    helper = GpgHelper(homedir, is_qubes=False)
+    helper = GpgHelper(homedir, session_maker, is_qubes=False)
     err = CalledProcessError(cmd=['foo'], returncode=1)
     mock_call = mocker.patch('securedrop_client.crypto.subprocess.check_call',
                              side_effect=err)
@@ -99,12 +99,12 @@ def test_import_key_gpg_call_fail(homedir, config, mocker):
     assert mock_call.called
 
 
-def test_encrypt(homedir, source, config, mocker):
+def test_encrypt(homedir, source, config, mocker, session_maker):
     '''
     Check that calling `encrypt` encrypts the message.
     Using the `config` fixture to ensure the config is written to disk.
     '''
-    helper = GpgHelper(homedir, is_qubes=False)
+    helper = GpgHelper(homedir, session_maker, is_qubes=False)
 
     # first we have to ensure the pubkeys are available
     helper._import(PUB_KEY)
@@ -134,12 +134,12 @@ def test_encrypt(homedir, source, config, mocker):
     assert decrypted == plaintext
 
 
-def test_encrypt_fail(homedir, source, config, mocker):
+def test_encrypt_fail(homedir, source, config, mocker, session_maker):
     '''
     Check that a `CryptoError` is raised if the call to `gpg` fails.
     Using the `config` fixture to ensure the config is written to disk.
     '''
-    helper = GpgHelper(homedir, is_qubes=False)
+    helper = GpgHelper(homedir, session_maker, is_qubes=False)
 
     # first we have to ensure the pubkeys are available
     helper._import(PUB_KEY)
diff --git a/tests/test_logic.py b/tests/test_logic.py
index fdf2dc25a..24c05e898 100644
--- a/tests/test_logic.py
+++ b/tests/test_logic.py
@@ -5,9 +5,12 @@
 import arrow
 import os
 import pytest
+import sdclientapi
 
+from PyQt5.QtCore import Qt
 from sdclientapi import sdlocalobjects, RequestTimeoutError
 from tests import factory
+
 from securedrop_client import storage, db
 from securedrop_client.crypto import CryptoError
 from securedrop_client.logic import APICallRunner, Controller
@@ -59,27 +62,27 @@ def test_APICallRunner_with_exception(mocker):
     cr.call_failed.emit.assert_called_once_with()
 
 
-def test_Controller_init(homedir, config, mocker):
+def test_Controller_init(homedir, config, mocker, session_maker):
     """
     The passed in gui, app and session instances are correctly referenced and,
     where appropriate, have a reference back to the client.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost/', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost/', mock_gui, session_maker, homedir)
     assert co.hostname == 'http://localhost/'
     assert co.gui == mock_gui
-    assert co.session == mock_session
+    assert co.session_maker == session_maker
     assert co.api_threads == {}
 
 
-def test_Controller_setup(homedir, config, mocker):
+def test_Controller_setup(homedir, config, mocker, session_maker):
     """
     Ensure the application is set up with the following default state:
     Using the `config` fixture to ensure the config is written to disk.
     """
-    co = Controller('http://localhost', mocker.MagicMock(), mocker.MagicMock(), homedir)
+    co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir)
     co.update_sources = mocker.MagicMock()
 
     co.setup()
@@ -87,85 +90,94 @@ def test_Controller_setup(homedir, config, mocker):
     co.gui.setup.assert_called_once_with(co)
 
 
-def test_Controller_start_message_thread(homedir, config, mocker):
+def test_Controller_start_message_thread(homedir, config, mocker, session_maker):
     """
     When starting message-fetching thread, make sure we do a few things.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     mock_qthread = mocker.patch('securedrop_client.logic.QThread')
     mocker.patch('securedrop_client.logic.MessageSync')
     co.message_sync = mocker.MagicMock()
+
     co.start_message_thread()
+
     co.message_sync.moveToThread.assert_called_once_with(mock_qthread())
     co.message_thread.started.connect.assert_called_once_with(co.message_sync.run)
     co.message_thread.start.assert_called_once_with()
 
 
-def test_Controller_start_message_thread_if_already_running(homedir, config, mocker):
+def test_Controller_start_message_thread_if_already_running(homedir, config, mocker, session_maker):
     """
     Ensure that when starting the message thread, we don't start another thread
     if it's already running. Instead, we just authenticate the existing thread.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.api = 'api object'
     co.message_sync = mocker.MagicMock()
     co.message_thread = mocker.MagicMock()
     co.message_thread.api = None
+
     co.start_message_thread()
-    co.message_sync.api = co.api
+
     co.message_thread.start.assert_not_called()
 
 
-def test_Controller_start_reply_thread(homedir, config, mocker):
+def test_Controller_start_reply_thread(homedir, config, mocker, session_maker):
     """
     When starting reply-fetching thread, make sure we do a few things.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     mock_qthread = mocker.patch('securedrop_client.logic.QThread')
     mocker.patch('securedrop_client.logic.ReplySync')
     co.reply_sync = mocker.MagicMock()
+
     co.start_reply_thread()
+
     co.reply_sync.moveToThread.assert_called_once_with(mock_qthread())
-    co.reply_thread.started.connect.assert_called_once_with(
-        co.reply_sync.run)
+    co.reply_thread.started.connect.assert_called_once_with(co.reply_sync.run)
     co.reply_thread.start.assert_called_once_with()
 
 
-def test_Controller_start_reply_thread_if_already_running(homedir, config, mocker):
+def test_Controller_start_reply_thread_if_already_running(homedir, config, mocker, session_maker):
     """
     Ensure that when starting the reply thread, we don't start another thread
     if it's already running. Instead, we just authenticate the existing thread.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     co.api = 'api object'
     co.reply_sync = mocker.MagicMock()
     co.reply_thread = mocker.MagicMock()
     co.reply_thread.api = None
+
     co.start_reply_thread()
-    co.reply_sync.api = co.api
+
     co.reply_thread.start.assert_not_called()
 
 
-def test_Controller_call_api(homedir, config, mocker):
+def test_Controller_call_api(homedir, config, mocker, session_maker):
     """
     A new thread and APICallRunner is created / setup.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     co.finish_api_call = mocker.MagicMock()
     mocker.patch('securedrop_client.logic.QThread')
     mocker.patch('securedrop_client.logic.APICallRunner')
@@ -188,18 +200,20 @@ def test_Controller_call_api(homedir, config, mocker):
     runner.call_timed_out.connect.call_count == 1
 
 
-def test_Controller_login(homedir, config, mocker):
+def test_Controller_login(homedir, config, mocker, session_maker):
     """
     Ensures the API is called in the expected manner for logging in the user
     given the username, password and 2fa token.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
-    co.call_api = mocker.MagicMock()
     mock_api = mocker.patch('securedrop_client.logic.sdclientapi.API')
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+    co.call_api = mocker.MagicMock()
+
     co.login('username', 'password', '123456')
+
     co.call_api.assert_called_once_with(mock_api().authenticate,
                                         co.on_authenticate_success,
                                         co.on_authenticate_failure)
@@ -229,30 +243,33 @@ def test_Controller_login_offline_mode(homedir, config, mocker):
     co.update_sources.assert_called_once_with()
 
 
-def test_Controller_on_authenticate_failure(homedir, config, mocker):
+def test_Controller_on_authenticate_failure(homedir, config, mocker, session_maker):
     """
     If the server responds with a negative to the request to authenticate, make
     sure the user knows.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     result_data = Exception('oh no')
     co.on_authenticate_failure(result_data)
+
     mock_gui.show_login_error.\
         assert_called_once_with(error='There was a problem signing in. Please '
                                 'verify your credentials and try again.')
 
 
-def test_Controller_on_authenticate_success(homedir, config, mocker):
+def test_Controller_on_authenticate_success(homedir, config, mocker, session_maker):
     """
     Ensure the client syncs when the user successfully logs in.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    mock_api_job_queue = mocker.patch("securedrop_client.logic.ApiJobQueue")
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.sync_api = mocker.MagicMock()
     co.api = mocker.MagicMock()
     co.start_message_thread = mocker.MagicMock()
@@ -266,19 +283,30 @@ def test_Controller_on_authenticate_success(homedir, config, mocker):
     co.gui.show_main_window.assert_called_once_with('test')
     co.gui.clear_error_status.assert_called_once_with()
 
+    # ensure mocks are used
+    assert mock_api_job_queue.called
+
 
-def test_Controller_completed_api_call_without_current_object(homedir, config, mocker):
+def test_Controller_completed_api_call_without_current_object(
+    homedir,
+    config,
+    mocker,
+    session_maker,
+):
     """
     Ensure that cleanup is performed if an API call returns in the expected
     time.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
+    result = 'result'
+
     mock_thread = mocker.MagicMock()
     mock_runner = mocker.MagicMock()
-    mock_runner.result = 'result'
+    mock_runner.result = result
     mock_runner.current_object = None
     co.api_threads = {
         'thread_uuid': {
@@ -287,23 +315,29 @@ def test_Controller_completed_api_call_without_current_object(homedir, config, m
         }
     }
     mock_user_callback = mocker.MagicMock()
+
     co.completed_api_call('thread_uuid', mock_user_callback)
-    mock_user_callback.assert_called_once_with('result')
+
+    mock_user_callback.assert_called_once_with(result)
 
 
-def test_Controller_completed_api_call_with_current_object(homedir, config, mocker):
+def test_Controller_completed_api_call_with_current_object(homedir, config, mocker, session_maker):
     """
     Ensure that cleanup is performed if an API call returns in the expected
     time.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
+    result = 'result'
+    current_object = 'current_object'
+
     mock_thread = mocker.MagicMock()
     mock_runner = mocker.MagicMock()
-    mock_runner.result = 'result'
-    mock_runner.current_object = 'current_object'
+    mock_runner.result = result
+    mock_runner.current_object = current_object
     co.api_threads = {
         'thread_uuid': {
             'thread': mock_thread,
@@ -316,94 +350,105 @@ def test_Controller_completed_api_call_with_current_object(homedir, config, mock
     mocker.patch('securedrop_client.logic.inspect.getfullargspec', return_value=mock_arg_spec)
 
     co.completed_api_call('thread_uuid', mock_user_callback)
-    mock_user_callback.assert_called_once_with('result',
-                                               current_object='current_object')
+    mock_user_callback.assert_called_once_with(result,
+                                               current_object=current_object)
 
 
-def test_Controller_on_action_requiring_login(homedir, config, mocker):
+def test_Controller_on_action_requiring_login(homedir, config, mocker, session_maker):
     """
     Ensure that when on_action_requiring_login is called, an error
     is shown in the GUI status area.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     co.on_action_requiring_login()
+
     mock_gui.update_error_status.assert_called_once_with(
         'You must sign in to perform this action.')
 
 
-def test_Controller_authenticated_yes(homedir, config, mocker):
+def test_Controller_authenticated_yes(homedir, config, mocker, session_maker):
     """
     If the API is authenticated return True.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.api = mocker.MagicMock()
     co.api.token = 'foo'
+
     assert co.authenticated() is True
 
 
-def test_Controller_authenticated_no(homedir, config, mocker):
+def test_Controller_authenticated_no(homedir, config, mocker, session_maker):
     """
     If the API is authenticated return True.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     co.api = mocker.MagicMock()
     co.api.token = None
+
     assert co.authenticated() is False
 
 
-def test_Controller_authenticated_no_api(homedir, config, mocker):
+def test_Controller_authenticated_no_api(homedir, config, mocker, session_maker):
     """
     If the API is authenticated return True.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.api = None
+
     assert co.authenticated() is False
 
 
-def test_Controller_sync_api_not_authenticated(homedir, config, mocker):
+def test_Controller_sync_api_not_authenticated(homedir, config, mocker, session_maker):
     """
     If the API isn't authenticated, don't sync.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.authenticated = mocker.MagicMock(return_value=False)
     co.call_api = mocker.MagicMock()
+
     co.sync_api()
+
     assert co.call_api.call_count == 0
 
 
-def test_Controller_sync_api(homedir, config, mocker):
+def test_Controller_sync_api(homedir, config, mocker, session_maker):
     """
     Sync the API is authenticated.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     co.authenticated = mocker.MagicMock(return_value=True)
     co.call_api = mocker.MagicMock()
+
     co.sync_api()
+
     co.call_api.assert_called_once_with(storage.get_remote_data,
                                         co.on_sync_success,
                                         co.on_sync_failure,
                                         co.api)
 
 
-def test_Controller_last_sync_with_file(homedir, config, mocker):
+def test_Controller_last_sync_with_file(homedir, config, mocker, session_maker):
     """
     The flag indicating the time of the last sync with the API is stored in a
     dotfile in the user's home directory. If such a file exists, ensure an
@@ -411,40 +456,47 @@ def test_Controller_last_sync_with_file(homedir, config, mocker):
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     timestamp = '2018-10-10 18:17:13+01:00'
     mocker.patch("builtins.open", mocker.mock_open(read_data=timestamp))
+
     result = co.last_sync()
+
     assert isinstance(result, arrow.Arrow)
     assert result.format() == timestamp
 
 
-def test_Controller_last_sync_no_file(homedir, config, mocker):
+def test_Controller_last_sync_no_file(homedir, config, mocker, session_maker):
     """
     If there's no sync file, then just return None.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     mocker.patch("builtins.open", mocker.MagicMock(side_effect=Exception()))
     assert co.last_sync() is None
 
 
-def test_Controller_on_sync_failure(homedir, config, mocker):
+def test_Controller_on_sync_failure(homedir, config, mocker, session_maker):
     """
     If there's no result to syncing, then don't attempt to update local storage
     and perhaps implement some as-yet-undefined UI update.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
     co.update_sources = mocker.MagicMock()
     result_data = Exception('Boom')  # Not the expected tuple.
     mock_storage = mocker.patch('securedrop_client.logic.storage')
+
     co.on_sync_failure(result_data)
+
     assert mock_storage.update_local_storage.call_count == 0
     co.update_sources.assert_called_once_with()
 
@@ -456,7 +508,9 @@ def test_Controller_on_sync_success(homedir, config, mocker):
     """
     mock_gui = mocker.MagicMock()
     mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    mock_session_maker = mocker.MagicMock(return_value=mock_session)
+
+    co = Controller('http://localhost', mock_gui, mock_session_maker, homedir)
     co.update_sources = mocker.MagicMock()
     co.api_runner = mocker.MagicMock()
     co.gpg = mocker.MagicMock()
@@ -495,7 +549,9 @@ def test_Controller_on_sync_success_with_non_pgp_key(homedir, config, mocker):
     """
     mock_gui = mocker.MagicMock()
     mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    mock_session_maker = mocker.MagicMock(return_value=mock_session)
+
+    co = Controller('http://localhost', mock_gui, mock_session_maker, homedir)
     co.update_sources = mocker.MagicMock()
     co.api_runner = mocker.MagicMock()
 
@@ -525,7 +581,9 @@ def test_Controller_on_sync_success_with_key_import_fail(homedir, config, mocker
     """
     mock_gui = mocker.MagicMock()
     mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    mock_session_maker = mocker.MagicMock(return_value=mock_session)
+
+    co = Controller('http://localhost', mock_gui, mock_session_maker, homedir)
     co.update_sources = mocker.MagicMock()
     co.api_runner = mocker.MagicMock()
 
@@ -550,14 +608,13 @@ def test_Controller_on_sync_success_with_key_import_fail(homedir, config, mocker
     co.update_sources.assert_called_once_with()
 
 
-def test_Controller_update_sync(homedir, config, mocker):
+def test_Controller_update_sync(homedir, config, mocker, session_maker):
     """
     Cause the UI to update with the result of self.last_sync().
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.last_sync = mocker.MagicMock()
     co.update_sync()
     assert co.last_sync.call_count == 1
@@ -572,23 +629,27 @@ def test_Controller_update_sources(homedir, config, mocker):
     """
     mock_gui = mocker.MagicMock()
     mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    mock_session_maker = mocker.MagicMock(return_value=mock_session)
+
+    co = Controller('http://localhost', mock_gui, mock_session_maker, homedir)
+
     mock_storage = mocker.patch('securedrop_client.logic.storage')
     source_list = [factory.Source(last_updated=2), factory.Source(last_updated=1)]
     mock_storage.get_local_sources.return_value = source_list
+
     co.update_sources()
+
     mock_storage.get_local_sources.assert_called_once_with(mock_session)
     mock_gui.show_sources.assert_called_once_with(source_list)
 
 
-def test_Controller_unstars_a_source_if_starred(homedir, config, mocker):
+def test_Controller_unstars_a_source_if_starred(homedir, config, mocker, session_maker):
     """
     Ensure that the client unstars a source if it is starred.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
 
     source_db_object = mocker.MagicMock()
     source_db_object.uuid = mocker.MagicMock()
@@ -614,14 +675,13 @@ def test_Controller_unstars_a_source_if_starred(homedir, config, mocker):
     mock_gui.clear_error_status.assert_called_once_with()
 
 
-def test_Controller_unstars_a_source_if_unstarred(homedir, config, mocker):
+def test_Controller_unstars_a_source_if_unstarred(homedir, config, mocker, session_maker):
     """
     Ensure that the client stars a source if it is unstarred.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
 
     source_db_object = mocker.MagicMock()
     source_db_object.uuid = mocker.MagicMock()
@@ -647,15 +707,14 @@ def test_Controller_unstars_a_source_if_unstarred(homedir, config, mocker):
     mock_gui.clear_error_status.assert_called_once_with()
 
 
-def test_Controller_update_star_not_logged_in(homedir, config, mocker):
+def test_Controller_update_star_not_logged_in(homedir, config, mocker, session_maker):
     """
     Ensure that starring/unstarring a source when not logged in calls
     the method that displays an error status in the left sidebar.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     source_db_object = mocker.MagicMock()
     co.on_action_requiring_login = mocker.MagicMock()
     co.api = None
@@ -663,15 +722,14 @@ def test_Controller_update_star_not_logged_in(homedir, config, mocker):
     co.on_action_requiring_login.assert_called_with()
 
 
-def test_Controller_on_update_star_success(homedir, config, mocker):
+def test_Controller_on_update_star_success(homedir, config, mocker, session_maker):
     """
     If the starring occurs successfully, then a sync should occur to update
     locally.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     result = True
     co.call_reset = mocker.MagicMock()
     co.sync_api = mocker.MagicMock()
@@ -680,15 +738,14 @@ def test_Controller_on_update_star_success(homedir, config, mocker):
     mock_gui.clear_error_status.assert_called_once_with()
 
 
-def test_Controller_on_update_star_failed(homedir, config, mocker):
+def test_Controller_on_update_star_failed(homedir, config, mocker, session_maker):
     """
     If the starring does not occur properly, then an error should appear
     on the error status sidebar, and a sync will not occur.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     result = Exception('boom')
     co.call_reset = mocker.MagicMock()
     co.sync_api = mocker.MagicMock()
@@ -697,15 +754,14 @@ def test_Controller_on_update_star_failed(homedir, config, mocker):
     mock_gui.update_error_status.assert_called_once_with('Failed to update star.')
 
 
-def test_Controller_logout(homedir, config, mocker):
+def test_Controller_logout(homedir, config, mocker, session_maker):
     """
     The API is reset to None and the UI is set to logged out state.
     The message and reply threads should also have the
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.api = mocker.MagicMock()
     co.message_sync = mocker.MagicMock()
     co.reply_sync = mocker.MagicMock()
@@ -718,14 +774,13 @@ def test_Controller_logout(homedir, config, mocker):
     co.gui.logout.assert_called_once_with()
 
 
-def test_Controller_set_activity_status(homedir, config, mocker):
+def test_Controller_set_activity_status(homedir, config, mocker, session_maker):
     """
     Ensure the GUI set_status API is called.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.set_status("Hello, World!", 1000)
     mock_gui.update_activity_status.assert_called_once_with("Hello, World!", 1000)
 
@@ -750,13 +805,12 @@ def test_Controller_set_activity_status(homedir, config, mocker):
 ]
 
 
-def test_create_client_dir_permissions(tmpdir, mocker):
+def test_create_client_dir_permissions(tmpdir, mocker, session_maker):
     '''
     Check that creating an app behaves appropriately with different
     permissions on the various directories needed for it to function.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
 
     # we can't rely on the config fixture, and because of the order of exectution,
     # we can't create the config at the right time, we we have to mock both
@@ -772,7 +826,7 @@ def test_create_client_dir_permissions(tmpdir, mocker):
             os.mkdir(sdc_home, case['home_perms'])
 
         def func() -> None:
-            Controller('http://localhost', mock_gui, mock_session, sdc_home)
+            Controller('http://localhost', mock_gui, session_maker, sdc_home)
 
         if case['should_pass']:
             func()
@@ -785,191 +839,87 @@ def func() -> None:
     assert mock_json.called
 
 
-def test_Controller_on_file_download_Submission(homedir, config, mocker):
+def test_Controller_on_file_download_Submission(homedir, config, session, mocker, session_maker):
     """
-    If the handler is passed a submission, check the download_submission
+    If the handler is passed a Submission, check the download_submission
     function is the one called against the API.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
-    co.call_api = mocker.MagicMock()
-    co.api = mocker.MagicMock()
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
+    mock_success_signal = mocker.MagicMock()
+    mock_failure_signal = mocker.MagicMock()
+    mock_job = mocker.MagicMock(success_signal=mock_success_signal,
+                                failure_signal=mock_failure_signal)
+    mock_job_cls = mocker.patch(
+        "securedrop_client.logic.DownloadSubmissionJob", return_value=mock_job)
+    mock_queue = mocker.patch.object(co, 'api_job_queue')
 
     source = factory.Source()
-    file_ = db.File(source=source, uuid='uuid', size=1234, filename='1-myfile.doc.gpg',
-                    download_url='http://myserver/myfile', is_downloaded=False)
+    file_ = factory.File(is_downloaded=None, is_decrypted=None, source=source)
+    session.add(source)
+    session.add(file_)
+    session.commit()
 
-    submission_sdk_object = mocker.MagicMock()
-    mock_submission = mocker.patch('sdclientapi.Submission')
-    mock_submission.return_value = submission_sdk_object
+    co.on_submission_download(db.File, file_.uuid)
 
-    co.on_file_download(source, file_)
-    co.call_api.assert_called_once_with(
-        co.api.download_submission,
-        co.on_file_download_success,
-        co.on_file_download_failure,
-        submission_sdk_object,
+    mock_job_cls.assert_called_once_with(
+        db.File,
+        file_.uuid,
         co.data_dir,
-        current_object=file_,
+        co.gpg,
     )
+    mock_queue.enqueue.assert_called_once_with(mock_job)
+    mock_success_signal.connect.assert_called_once_with(
+        co.on_file_download_success, type=Qt.QueuedConnection)
+    mock_failure_signal.connect.assert_called_once_with(
+        co.on_file_download_failure, type=Qt.QueuedConnection)
 
 
-def test_Controller_on_file_downloaded_success(homedir, config, mocker):
+def test_Controller_on_file_downloaded_success(homedir, config, mocker, session_maker):
     '''
     Using the `config` fixture to ensure the config is written to disk.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
-    co.update_sources = mocker.MagicMock()
-    co.api_runner = mocker.MagicMock()
-    co.file_ready = mocker.MagicMock()  # signal when file is downloaded
 
-    test_filename = "1-my-file-location-msg.gpg"
-    test_object_uuid = 'uuid-of-downloaded-object'
-    co.call_reset = mocker.MagicMock()
-    result_data = ('this-is-a-sha256-sum', test_filename)
-    submission_db_object = mocker.MagicMock()
-    submission_db_object.uuid = test_object_uuid
-    submission_db_object.filename = test_filename
-    mock_storage = mocker.patch('securedrop_client.logic.storage')
-    mock_gpg = mocker.patch.object(co.gpg, 'decrypt_submission_or_reply', return_value='filepath')
-    mocker.patch('shutil.move')
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
 
-    co.on_file_download_success(result_data, current_object=submission_db_object)
+    # signal when file is downloaded
+    mock_file_ready = mocker.patch.object(co, 'file_ready')
+    mock_uuid = 'mock'
 
-    mock_gpg.call_count == 1
-    mock_storage.mark_file_as_downloaded.assert_called_once_with(
-        test_object_uuid, mock_session)
-    mock_storage.set_object_decryption_status_with_content.assert_called_once_with(
-        submission_db_object, mock_session, True)
+    co.on_file_download_success(mock_uuid)
 
-    # Signal should be emitted with UUID of the successfully downloaded object
-    co.file_ready.emit.assert_called_once_with(test_object_uuid)
+    mock_file_ready.emit.assert_called_once_with(mock_uuid)
 
 
-def test_Controller_on_file_downloaded_api_failure(homedir, config, mocker):
+def test_Controller_on_file_downloaded_api_failure(homedir, config, mocker, session_maker):
     '''
     Using the `config` fixture to ensure the config is written to disk.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
-    co.file_ready = mocker.MagicMock()  # signal when file is downloaded
-    co.update_sources = mocker.MagicMock()
-    co.api_runner = mocker.MagicMock()
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
 
-    test_filename = "1-my-file-location-msg.gpg"
-    co.api_runner.result = ("", test_filename)
-    co.call_reset = mocker.MagicMock()
-    co.set_status = mocker.MagicMock()
+    # signal when file is downloaded
+    mock_file_ready = mocker.patch.object(co, 'file_ready')
+    mock_set_status = mocker.patch.object(co, 'set_status')
     result_data = Exception('error message')
-    submission_db_object = mocker.MagicMock()
-    submission_db_object.uuid = 'myuuid'
-    submission_db_object.filename = 'filename'
-
-    co.on_file_download_failure(result_data, current_object=submission_db_object)
-
-    co.set_status.assert_called_once_with("The file download failed. Please try again.")
-    co.file_ready.emit.assert_not_called()
-
-
-def test_Controller_on_file_downloaded_decrypt_failure(homedir, config, mocker):
-    '''
-    Using the `config` fixture to ensure the config is written to disk.
-    '''
-    mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
-
-    co.update_sources = mocker.MagicMock()
-    co.api_runner = mocker.MagicMock()
-    co.file_ready = mocker.MagicMock()  # signal when file is downloaded
-
-    test_filename = "1-my-file-location-msg.gpg"
-    co.api_runner.result = ("", test_filename)
-    co.set_status = mocker.MagicMock()
-
-    result_data = ('this-is-a-sha256-sum', test_filename)
-    submission_db_object = mocker.MagicMock()
-    submission_db_object.uuid = 'myuuid'
-    submission_db_object.filename = 'filename'
-    mock_gpg = mocker.patch.object(co.gpg, 'decrypt_submission_or_reply',
-                                   side_effect=CryptoError())
-    mock_storage = mocker.patch('securedrop_client.logic.storage')
-    mocker.patch('shutil.move')
-
-    co.on_file_download_success(result_data, current_object=submission_db_object)
-    mock_gpg.call_count == 1
-    co.set_status.assert_called_once_with(
-        "Failed to decrypt file, please try again or talk to your administrator.")
-    mock_storage.set_object_decryption_status_with_content.assert_called_once_with(
-        submission_db_object, mock_session, False)
-    co.file_ready.emit.assert_not_called()
 
+    co.on_file_download_failure(result_data)
 
-def test_Controller_on_file_download_user_not_signed_in(homedir, config, mocker):
-    """
-    If a user clicks the download button but is not logged in,
-    an error should appear.
-    Using the `config` fixture to ensure the config is written to disk.
-    """
-    mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
-    source = factory.Source()
-    file_ = db.File(source=source, uuid='uuid', size=1234, filename='1-myfile.doc.gpg',
-                    download_url='http://myserver/myfile', is_downloaded=False)
-    co.on_action_requiring_login = mocker.MagicMock()
-    co.api = None
-    submission_sdk_object = mocker.MagicMock()
-    mock_submission = mocker.patch('sdclientapi.Submission')
-    mock_submission.return_value = submission_sdk_object
-    co.on_file_download(source, file_)
-    co.on_action_requiring_login.assert_called_once_with()
-
-
-def test_Controller_on_file_download_Reply(homedir, config, mocker):
-    """
-    If the handler is passed a reply, check the download_reply
-    function is the one called against the API.
-    Using the `config` fixture to ensure the config is written to disk.
-    """
-    mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
-    source = factory.Source()
-    journalist = db.User('Testy mcTestface')
-    reply = db.Reply(uuid='reply-uuid',
-                     journalist=journalist,
-                     source=source,
-                     filename='1-my-reply.gpg',
-                     size=123)  # Not a sdclientapi.Submission
-    co.call_api = mocker.MagicMock()
-    co.api = mocker.MagicMock()
-    reply_sdk_object = mocker.MagicMock()
-    mock_reply = mocker.patch('sdclientapi.Reply')
-    mock_reply.return_value = reply_sdk_object
-    co.on_file_download(source, reply)
-    co.call_api.assert_called_once_with(co.api.download_reply,
-                                        co.on_file_download_success,
-                                        co.on_file_download_failure,
-                                        reply_sdk_object,
-                                        co.data_dir,
-                                        current_object=reply)
+    mock_set_status.assert_called_once_with("The file download failed. Please try again.")
+    mock_file_ready.emit.assert_not_called()
 
 
-def test_Controller_on_file_open(homedir, config, mocker):
+def test_Controller_on_file_open(homedir, config, mocker, session_maker):
     """
     If running on Qubes, a new QProcess with the expected command and args
     should be started.
     Using the `config` fixture to ensure the config is written to disk.
     """
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.proxy = True
     mock_submission = mocker.MagicMock()
     mock_submission.filename = '1-test.pdf'
@@ -981,39 +931,36 @@ def test_Controller_on_file_open(homedir, config, mocker):
     mock_subprocess.start.call_count == 1
 
 
-def test_Controller_on_delete_source_success(homedir, config, mocker):
+def test_Controller_on_delete_source_success(homedir, config, mocker, session_maker):
     '''
     Using the `config` fixture to ensure the config is written to disk.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.sync_api = mocker.MagicMock()
     co.on_delete_source_success(True)
     co.sync_api.assert_called_with()
     co.gui.clear_error_status.assert_called_with()
 
 
-def test_Controller_on_delete_source_failure(homedir, config, mocker):
+def test_Controller_on_delete_source_failure(homedir, config, mocker, session_maker):
     '''
     Using the `config` fixture to ensure the config is written to disk.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.sync_api = mocker.MagicMock()
     co.on_delete_source_failure(Exception())
     co.gui.update_error_status.assert_called_with('Failed to delete source at server')
 
 
-def test_Controller_delete_source(homedir, config, mocker):
+def test_Controller_delete_source(homedir, config, mocker, session_maker):
     '''
     Using the `config` fixture to ensure the config is written to disk.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
     mock_source = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.call_api = mocker.MagicMock()
     co.api = mocker.MagicMock()
     co.delete_source(mock_source)
@@ -1025,15 +972,14 @@ def test_Controller_delete_source(homedir, config, mocker):
     )
 
 
-def test_Controller_send_reply_success(homedir, mocker):
+def test_Controller_send_reply_success(homedir, mocker, session_maker):
     '''
     Check that the "happy path" of encrypting a message and sending it to the sever behaves as
     expected.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
 
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
 
     co.call_api = mocker.Mock()
     co.api = mocker.Mock()
@@ -1065,14 +1011,13 @@ def test_Controller_send_reply_success(homedir, mocker):
     assert mock_source_init.called  # to prevent stale mocks
 
 
-def test_Controller_send_reply_gpg_error(homedir, mocker):
+def test_Controller_send_reply_gpg_error(homedir, mocker, session_maker):
     '''
     Check that if gpg fails when sending a message, we alert the UI and do *not* call the API.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
 
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
 
     co.call_api = mocker.Mock()
     co.api = mocker.Mock()
@@ -1099,42 +1044,46 @@ def test_Controller_send_reply_gpg_error(homedir, mocker):
     assert mock_source_init.called  # to prevent stale mocks
 
 
-def test_Controller_on_reply_success(homedir, mocker):
+def test_Controller_on_reply_success(homedir, mocker, session_maker, session):
     '''
     Check that when the result is a success, the client emits the correct signal.
     '''
+    user = factory.User()
+    source = factory.Source()
+    msg = factory.Message(source=source)
+    session.add(user)
+    session.add(source)
+    session.add(msg)
+    session.commit()
+
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    mock_reply_init = mocker.patch('securedrop_client.logic.db.Reply')
 
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.api = mocker.Mock()
-    journalist_uuid = 'abc123'
-    co.api.token_journalist_uuid = journalist_uuid
+
+    reply = sdclientapi.Reply(uuid='wat', filename='1-lol')
+
+    co.api.token_journalist_uuid = user.uuid
     mock_reply_succeeded = mocker.patch.object(co, 'reply_succeeded')
     mock_reply_failed = mocker.patch.object(co, 'reply_failed')
 
-    reply = sdlocalobjects.Reply(uuid='xyz456', filename='1-wat.gpg')
-
-    source_uuid = 'foo111'
-    msg_uuid = 'bar222'
-    current_object = (source_uuid, msg_uuid)
+    current_object = (source.uuid, msg.uuid)
     co.on_reply_success(reply, current_object)
-    co.session.commit.assert_called_once_with()
-    mock_reply_succeeded.emit.assert_called_once_with(msg_uuid)
+    mock_reply_succeeded.emit.assert_called_once_with(msg.uuid)
     assert not mock_reply_failed.emit.called
 
-    assert mock_reply_init.called  # to prevent stale mocks
+    # check that this was writtent to the DB
+    replies = session.query(db.Reply).all()
+    assert len(replies) == 1
 
 
-def test_Controller_on_reply_failure(homedir, mocker):
+def test_Controller_on_reply_failure(homedir, mocker, session_maker):
     '''
     Check that when the result is a failure, the client emits the correct signal.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
 
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.api = mocker.Mock()
     journalist_uuid = 'abc123'
     co.api.token_journalist_uuid = journalist_uuid
@@ -1149,7 +1098,7 @@ def test_Controller_on_reply_failure(homedir, mocker):
     assert not mock_reply_succeeded.emit.called
 
 
-def test_Controller_is_authenticated_property(homedir, mocker):
+def test_Controller_is_authenticated_property(homedir, mocker, session_maker):
     '''
     Check that the @property `is_authenticated`:
       - Cannot be deleted
@@ -1157,9 +1106,8 @@ def test_Controller_is_authenticated_property(homedir, mocker):
       - Sets internal state to ensure signals are only set when the state changes
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
 
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     mock_signal = mocker.patch.object(co, 'authentication_state')
 
     # default state is unauthenticated
@@ -1206,13 +1154,12 @@ def test_APICallRunner_api_call_timeout(mocker):
     mock_timeout_signal.emit.assert_called_once_with()
 
 
-def test_Controller_api_call_timeout(homedir, config, mocker):
+def test_Controller_api_call_timeout(homedir, config, mocker, session_maker):
     '''
     Using the `config` fixture to ensure the config is written to disk.
     '''
     mock_gui = mocker.MagicMock()
-    mock_session = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, mock_session, homedir)
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.on_api_timeout()
     mock_gui.update_error_status.assert_called_once_with(
         'The connection to the SecureDrop server timed out. Please try again.')

From d5f66bce4c3dafdb4ae4598b235655a7e4c37486 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 16:52:07 +0200
Subject: [PATCH 10/17] basic tests for ApiJob

---
 tests/test_queue.py | 119 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 tests/test_queue.py

diff --git a/tests/test_queue.py b/tests/test_queue.py
new file mode 100644
index 000000000..1c15f6d3d
--- /dev/null
+++ b/tests/test_queue.py
@@ -0,0 +1,119 @@
+'''
+Testing for the ApiJobQueue and related classes.
+'''
+import pytest
+
+from sdclientapi import AuthError, RequestTimeoutError
+
+from securedrop_client.queue import ApiInaccessibleError, ApiJob
+
+
+def test_ApiInaccessibleError_init():
+    # check default value
+    err = ApiInaccessibleError()
+    assert str(err).startswith('API is inaccessible')
+    assert isinstance(err, Exception)
+
+    # check custom
+    msg = 'foo'
+    err = ApiInaccessibleError(msg)
+    assert str(err) == msg
+
+
+def test_ApiJob_raises_NotImplemetedError():
+    job = ApiJob()
+
+    with pytest.raises(NotImplementedError):
+        job.call_api(None, None)
+
+
+def dummy_job_factory(mocker, return_value):
+    '''
+    Factory that creates dummy `ApiJob`s to DRY up test code.
+    '''
+    class DummyApiJob(ApiJob):
+        success_signal = mocker.MagicMock()
+        failure_signal = mocker.MagicMock()
+
+        def __init__(self, *nargs, **kwargs):
+            super().__init__(*nargs, **kwargs)
+
+        def call_api(self, api_client, session):
+            if isinstance(return_value, Exception):
+                raise return_value
+            else:
+                return return_value
+
+    return DummyApiJob
+
+
+def test_ApiJob_no_api(mocker):
+    return_value = 'wat'
+    api_job_cls = dummy_job_factory(mocker, return_value)
+    api_job = api_job_cls()
+
+    mock_session = mocker.MagicMock()
+
+    with pytest.raises(ApiInaccessibleError):
+        api_job._do_call_api(None, mock_session)
+
+    assert not api_job.success_signal.emit.called
+    assert not api_job.failure_signal.emit.called
+
+
+def test_ApiJob_success(mocker):
+    return_value = 'wat'
+    api_job_cls = dummy_job_factory(mocker, return_value)
+    api_job = api_job_cls()
+
+    mock_api_client = mocker.MagicMock()
+    mock_session = mocker.MagicMock()
+
+    api_job._do_call_api(mock_api_client, mock_session)
+
+    api_job.success_signal.emit.assert_called_once_with(return_value)
+    assert not api_job.failure_signal.emit.called
+
+
+def test_ApiJob_auth_error(mocker):
+    return_value = AuthError('oh no')
+    api_job_cls = dummy_job_factory(mocker, return_value)
+    api_job = api_job_cls()
+
+    mock_api_client = mocker.MagicMock()
+    mock_session = mocker.MagicMock()
+
+    with pytest.raises(ApiInaccessibleError):
+        api_job._do_call_api(mock_api_client, mock_session)
+
+    assert not api_job.success_signal.emit.called
+    assert not api_job.failure_signal.emit.called
+
+
+def test_ApiJob_timeout_error(mocker):
+    return_value = RequestTimeoutError()
+    api_job_cls = dummy_job_factory(mocker, return_value)
+    api_job = api_job_cls()
+
+    mock_api_client = mocker.MagicMock()
+    mock_session = mocker.MagicMock()
+
+    with pytest.raises(RequestTimeoutError):
+        api_job._do_call_api(mock_api_client, mock_session)
+
+    assert not api_job.success_signal.emit.called
+    assert not api_job.failure_signal.emit.called
+
+
+def test_ApiJob_other_error(mocker):
+    return_value = Exception()
+    api_job_cls = dummy_job_factory(mocker, return_value)
+    api_job = api_job_cls()
+
+    mock_api_client = mocker.MagicMock()
+    mock_session = mocker.MagicMock()
+
+    api_job._do_call_api(mock_api_client, mock_session)
+
+    assert not api_job.success_signal.emit.called
+    api_job.failure_signal.emit.assert_called_once_with(return_value)

From e6a379cc4d31d051edb34456bd1cf37e9e6a5174 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 17:34:58 +0200
Subject: [PATCH 11/17] test for queue caching behavior on timeouts

---
 securedrop_client/queue.py |  4 +-
 tests/test_queue.py        | 93 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 91 insertions(+), 6 deletions(-)

diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py
index 8874ec15d..c0a78c19d 100644
--- a/securedrop_client/queue.py
+++ b/securedrop_client/queue.py
@@ -178,9 +178,9 @@ def __init__(self, api_client: API, session_maker: scoped_session) -> None:
 
     @pyqtSlot()
     def process(self) -> None:  # pragma: nocover
-        self.__process(False)
+        self._process(False)
 
-    def __process(self, exit_loop: bool) -> None:
+    def _process(self, exit_loop: bool) -> None:
         session = self.session_maker()
         while True:
             # retry the "cached" job if it exists, otherwise get the next job
diff --git a/tests/test_queue.py b/tests/test_queue.py
index 1c15f6d3d..ce39476d7 100644
--- a/tests/test_queue.py
+++ b/tests/test_queue.py
@@ -3,9 +3,10 @@
 '''
 import pytest
 
+from queue import Queue
 from sdclientapi import AuthError, RequestTimeoutError
 
-from securedrop_client.queue import ApiInaccessibleError, ApiJob
+from securedrop_client.queue import ApiInaccessibleError, ApiJob, RunnableQueue
 
 
 def test_ApiInaccessibleError_init():
@@ -37,12 +38,13 @@ class DummyApiJob(ApiJob):
 
         def __init__(self, *nargs, **kwargs):
             super().__init__(*nargs, **kwargs)
+            self.return_value = return_value
 
         def call_api(self, api_client, session):
-            if isinstance(return_value, Exception):
-                raise return_value
+            if isinstance(self.return_value, Exception):
+                raise self.return_value
             else:
-                return return_value
+                return self.return_value
 
     return DummyApiJob
 
@@ -117,3 +119,86 @@ def test_ApiJob_other_error(mocker):
 
     assert not api_job.success_signal.emit.called
     api_job.failure_signal.emit.assert_called_once_with(return_value)
+
+
+def test_RunnableQueue_init(mocker):
+    mock_api_client = mocker.MagicMock()
+    mock_session_maker = mocker.MagicMock()
+
+    queue = RunnableQueue(mock_api_client, mock_session_maker)
+    assert queue.api_client == mock_api_client
+    assert queue.session_maker == mock_session_maker
+    assert isinstance(queue.queue, Queue)
+    assert queue.queue.empty()
+    assert queue.last_job is None
+
+
+def test_RunnableQueue_happy_path(mocker):
+    '''
+    Add one job to the queue, run it.
+    '''
+    mock_process_events = mocker.patch('securedrop_client.queue.QApplication.processEvents')
+    mock_api_client = mocker.MagicMock()
+    mock_session = mocker.MagicMock()
+    mock_session_maker = mocker.MagicMock(return_value=mock_session)
+    return_value = 'foo'
+
+    dummy_job_cls = dummy_job_factory(mocker, return_value)
+
+    queue = RunnableQueue(mock_api_client, mock_session_maker)
+    queue.queue.put_nowait(dummy_job_cls())
+
+    queue._process(exit_loop=True)
+
+    # this needs to be called at the end of the loop
+    assert mock_process_events.called
+
+    assert queue.last_job is None
+    assert queue.queue.empty()
+
+
+def test_RunnableQueue_job_timeout(mocker):
+    '''
+    Add two jobs to the queue. The first times out, and then gets "cached" for the next pass
+    through the loop.
+    '''
+    mock_process_events = mocker.patch('securedrop_client.queue.QApplication.processEvents')
+    mock_api_client = mocker.MagicMock()
+    mock_session = mocker.MagicMock()
+    mock_session_maker = mocker.MagicMock(return_value=mock_session)
+
+    return_value = RequestTimeoutError()
+    dummy_job_cls = dummy_job_factory(mocker, return_value)
+    job1 = dummy_job_cls()
+    job2 = dummy_job_cls()
+
+    queue = RunnableQueue(mock_api_client, mock_session_maker)
+    queue.queue.put_nowait(job1)
+    queue.queue.put_nowait(job2)
+
+    # attempt to process job1 knowing that it times out
+    queue._process(exit_loop=True)
+
+    # check that job1 is "cached" and a job is in the queue
+    assert queue.last_job is job1
+    assert queue.queue.qsize() == 1
+
+    # update job1 to not raise an error so it can be processed
+    job1.return_value = 'foo'
+
+    # attempt to process the job1 again
+    queue._process(exit_loop=True)
+
+    # check that job has not been cached again
+    assert queue.last_job is None
+    assert queue.queue.qsize() == 1
+
+    # attempt to process job2 knowing that it times out
+    queue._process(exit_loop=True)
+
+    # check that job2 was cached and that the queue is empty
+    assert queue.last_job is job2
+    assert queue.queue.empty()
+
+    # ensure we don't have stale mocks
+    assert mock_process_events.called

From 41faea746a40e9cc092af2b396970e83d071c9eb Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Wed, 22 May 2019 18:29:22 +0200
Subject: [PATCH 12/17] testing etags on downloaded files

---
 tests/test_queue.py | 164 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 163 insertions(+), 1 deletion(-)

diff --git a/tests/test_queue.py b/tests/test_queue.py
index ce39476d7..2847b410f 100644
--- a/tests/test_queue.py
+++ b/tests/test_queue.py
@@ -1,12 +1,19 @@
 '''
 Testing for the ApiJobQueue and related classes.
 '''
+import os
 import pytest
+import sdclientapi
 
+from . import factory
 from queue import Queue
 from sdclientapi import AuthError, RequestTimeoutError
+from typing import Tuple
 
-from securedrop_client.queue import ApiInaccessibleError, ApiJob, RunnableQueue
+from securedrop_client import db
+from securedrop_client.crypto import GpgHelper
+from securedrop_client.queue import ApiInaccessibleError, ApiJob, RunnableQueue, \
+    DownloadSubmissionJob
 
 
 def test_ApiInaccessibleError_init():
@@ -202,3 +209,158 @@ def test_RunnableQueue_job_timeout(mocker):
 
     # ensure we don't have stale mocks
     assert mock_process_events.called
+
+
+def test_DownloadSubmissionJob_happy_path_no_etag(mocker, homedir, session, session_maker):
+    source = factory.Source()
+    file_ = factory.File(source=source)
+    session.add(source)
+    session.add(file_)
+    session.commit()
+
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+
+    def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
+        '''
+        :return: (etag, path_to_dl)
+        '''
+        full_path = os.path.join(homedir, 'somepath')
+        with open(full_path, 'wb') as f:
+            f.write(b'')
+        return ('', full_path)
+
+    api_client = mocker.MagicMock()
+    api_client.download_submission = fake_download
+
+    job = DownloadSubmissionJob(
+        db.File,
+        file_.uuid,
+        homedir,
+        gpg,
+    )
+
+    mock_decrypt = mocker.patch.object(job, '_decrypt_file')
+    mock_logger = mocker.patch('securedrop_client.queue.logger')
+
+    job.call_api(api_client, session)
+
+    log_msg = mock_logger.debug.call_args_list[0][0][0]
+    assert log_msg.startswith('No ETag. Skipping integrity check')
+
+    # ensure mocks aren't stale
+    assert mock_decrypt.called
+
+
+def test_DownloadSubmissionJob_happy_path_sha256_etag(mocker, homedir, session, session_maker):
+    source = factory.Source()
+    file_ = factory.File(source=source)
+    session.add(source)
+    session.add(file_)
+    session.commit()
+
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+
+    def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
+        '''
+        :return: (etag, path_to_dl)
+        '''
+        full_path = os.path.join(homedir, 'somepath')
+        with open(full_path, 'wb') as f:
+            f.write(b'wat')
+
+        # sha256 of b'wat'
+        return ('sha256:f00a787f7492a95e165b470702f4fe9373583fbdc025b2c8bdf0262cc48fcff4',
+                full_path)
+
+    api_client = mocker.MagicMock()
+    api_client.download_submission = fake_download
+
+    job = DownloadSubmissionJob(
+        db.File,
+        file_.uuid,
+        homedir,
+        gpg,
+    )
+
+    mock_decrypt = mocker.patch.object(job, '_decrypt_file')
+
+    job.call_api(api_client, session)
+
+    # ensure mocks aren't stale
+    assert mock_decrypt.called
+
+
+def test_DownloadSubmissionJob_bad_sha256_etag(mocker, homedir, session, session_maker):
+    source = factory.Source()
+    file_ = factory.File(source=source)
+    session.add(source)
+    session.add(file_)
+    session.commit()
+
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+
+    def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
+        '''
+        :return: (etag, path_to_dl)
+        '''
+        full_path = os.path.join(homedir, 'somepath')
+        with open(full_path, 'wb') as f:
+            f.write(b'')
+
+        return ('sha256:not-a-sha-sum',
+                full_path)
+
+    api_client = mocker.MagicMock()
+    api_client.download_submission = fake_download
+
+    job = DownloadSubmissionJob(
+        db.File,
+        file_.uuid,
+        homedir,
+        gpg,
+    )
+
+    # we currently don't handle errors in the checksum
+    with pytest.raises(RuntimeError):
+        job.call_api(api_client, session)
+
+
+def test_DownloadSubmissionJob_happy_path_unknown_etag(mocker, homedir, session, session_maker):
+    source = factory.Source()
+    file_ = factory.File(source=source)
+    session.add(source)
+    session.add(file_)
+    session.commit()
+
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+
+    def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
+        '''
+        :return: (etag, path_to_dl)
+        '''
+        full_path = os.path.join(homedir, 'somepath')
+        with open(full_path, 'wb') as f:
+            f.write(b'')
+        return ('UNKNOWN:abc123',
+                full_path)
+
+    api_client = mocker.MagicMock()
+    api_client.download_submission = fake_download
+
+    job = DownloadSubmissionJob(
+        db.File,
+        file_.uuid,
+        homedir,
+        gpg,
+    )
+
+    mock_decrypt = mocker.patch.object(job, '_decrypt_file')
+    mock_logger = mocker.patch('securedrop_client.queue.logger')
+
+    job.call_api(api_client, session)
+
+    log_msg = mock_logger.debug.call_args_list[0][0][0]
+    assert log_msg.startswith('Unknown hash algorithm')
+
+    # ensure mocks aren't stale
+    assert mock_decrypt.called

From e21084ea3fbbe14b86ecbc3a653f19171f1c8cca Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Fri, 24 May 2019 13:24:07 -0700
Subject: [PATCH 13/17] move sqlalchemy session management out of gui

---
 securedrop_client/app.py         |  2 +-
 securedrop_client/gui/main.py    |  5 ++-
 securedrop_client/gui/widgets.py | 54 +++++++++++---------------------
 securedrop_client/logic.py       |  9 ++++--
 4 files changed, 29 insertions(+), 41 deletions(-)

diff --git a/securedrop_client/app.py b/securedrop_client/app.py
index fbf9a7d47..098bc18cf 100644
--- a/securedrop_client/app.py
+++ b/securedrop_client/app.py
@@ -186,7 +186,7 @@ def start_app(args, qt_args) -> None:
 
     session_maker = make_session_maker(args.sdc_home)
 
-    gui = Window(session_maker)
+    gui = Window()
 
     app.setWindowIcon(load_icon(gui.icon))
     app.setStyleSheet(load_css('sdclient.css'))
diff --git a/securedrop_client/gui/main.py b/securedrop_client/gui/main.py
index f02b63284..37426d29a 100644
--- a/securedrop_client/gui/main.py
+++ b/securedrop_client/gui/main.py
@@ -25,7 +25,6 @@
 from typing import Dict, List, Optional  # noqa: F401
 from PyQt5.QtWidgets import QMainWindow, QWidget, QHBoxLayout, QVBoxLayout, QDesktopWidget, \
     QApplication
-from sqlalchemy.orm import scoped_session
 
 from securedrop_client import __version__
 from securedrop_client.db import Source
@@ -44,7 +43,7 @@ class Window(QMainWindow):
 
     icon = 'icon.png'
 
-    def __init__(self, session_maker: scoped_session) -> None:
+    def __init__(self) -> None:
         """
         Create the default start state. The window contains a root widget into
         which is placed:
@@ -69,7 +68,7 @@ def __init__(self, session_maker: scoped_session) -> None:
         layout.setSpacing(0)
         self.main_pane.setLayout(layout)
         self.left_pane = LeftPane()
-        self.main_view = MainView(session_maker, self.main_pane)
+        self.main_view = MainView(self.main_pane)
         layout.addWidget(self.left_pane, 1)
         layout.addWidget(self.main_view, 8)
 
diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py
index 1eb23c169..459056e9a 100644
--- a/securedrop_client/gui/widgets.py
+++ b/securedrop_client/gui/widgets.py
@@ -29,7 +29,6 @@
 from PyQt5.QtWidgets import QListWidget, QLabel, QWidget, QListWidgetItem, QHBoxLayout, \
     QPushButton, QVBoxLayout, QLineEdit, QScrollArea, QDialog, QAction, QMenu, QMessageBox, \
     QToolButton, QSizePolicy, QTextEdit, QStatusBar, QGraphicsDropShadowEffect
-from sqlalchemy.orm import scoped_session
 
 from securedrop_client.db import Source, Message, File, Reply
 from securedrop_client.storage import source_exists
@@ -588,9 +587,8 @@ class MainView(QWidget):
     }
     '''
 
-    def __init__(self, session_maker: scoped_session, parent: QObject):
+    def __init__(self, parent: QObject):
         super().__init__(parent)
-        self.session_maker = session_maker
 
         self.setStyleSheet(self.CSS)
 
@@ -633,11 +631,7 @@ def on_source_changed(self):
         source = self.source_list.get_current_source()
 
         if source:
-            conversation_wrapper = SourceConversationWrapper(
-                self.session_maker,
-                source,
-                self.controller,
-            )
+            conversation_wrapper = SourceConversationWrapper(source, self.controller)
             self.set_conversation(conversation_wrapper)
         else:
             self.clear_conversation()
@@ -1282,7 +1276,6 @@ class FileWidget(QWidget):
 
     def __init__(
         self,
-        session_maker: scoped_session,
         file_uuid: str,
         controller: Controller,
         file_ready_signal: pyqtBoundSignal,
@@ -1291,35 +1284,23 @@ def __init__(
         Given some text and a reference to the controller, make something to display a file.
         """
         super().__init__()
-        self.session_maker = session_maker
         self.controller = controller
-        self.file_uuid = file_uuid
-        self.file_is_downloaded = False  # default to `False`, value updated in `update()`
+        self.file = self.controller.get_file(file_uuid)
 
         self.layout = QHBoxLayout()
         self.update()
         self.setLayout(self.layout)
 
-        file_ready_signal.connect(self._on_file_download, type=Qt.QueuedConnection)
+        file_ready_signal.connect(self._on_file_downloaded, type=Qt.QueuedConnection)
 
     def update(self) -> None:
         icon = QLabel()
         icon.setPixmap(load_image('file.png'))
 
-        session = self.session_maker()
-
-        # we have to query to get the object we want
-        file_ = session.query(File).filter_by(uuid=self.file_uuid).one()
-        # and then force a refresh because SQLAlchemy might have a copy of this object
-        # in this thread already that isn't up to date
-        session.refresh(file_)
-
-        self.file_is_downloaded = file_.is_downloaded
-
-        if self.file_is_downloaded:
+        if self.file.is_downloaded:
             description = QLabel("Open")
         else:
-            human_filesize = humanize_filesize(file_.size)
+            human_filesize = humanize_filesize(self.file.size)
             description = QLabel("Download ({})".format(human_filesize))
 
         self.layout.addWidget(icon)
@@ -1333,8 +1314,12 @@ def clear(self) -> None:
                 child.widget().deleteLater()
 
     @pyqtSlot(str)
-    def _on_file_download(self, file_uuid: str) -> None:
-        if file_uuid == self.file_uuid:
+    def _on_file_downloaded(self, file_uuid: str) -> None:
+        if file_uuid == self.file.uuid:
+            # update state
+            self.file = self.controller.get_file(self.file.uuid)
+
+            # update gui
             self.clear()  # delete existing icon and label
             self.update()  # draw modified widget
 
@@ -1343,12 +1328,15 @@ def mouseReleaseEvent(self, e):
         Handle a completed click via the program logic. The download state
         of the file distinguishes which function in the logic layer to call.
         """
-        if self.file_is_downloaded:
+        # update state
+        self.file = self.controller.get_file(self.file.uuid)
+
+        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.uuid)
         else:
             # Download the file.
-            self.controller.on_submission_download(File, self.file_uuid)
+            self.controller.on_submission_download(File, self.file.uuid)
 
 
 class ConversationView(QWidget):
@@ -1362,12 +1350,10 @@ class ConversationView(QWidget):
 
     def __init__(
         self,
-        session_maker: scoped_session,
         source_db_object: Source,
         controller: Controller,
     ):
         super().__init__()
-        self.session_maker = session_maker
         self.source = source_db_object
         self.controller = controller
 
@@ -1419,7 +1405,6 @@ def add_file(self, source_db_object, submission_db_object):
         """
         self.conversation_layout.addWidget(
             FileWidget(
-                self.session_maker,
                 submission_db_object.uuid,
                 self.controller,
                 self.controller.file_ready,
@@ -1494,7 +1479,6 @@ class SourceConversationWrapper(QWidget):
 
     def __init__(
         self,
-        session_maker: scoped_session,
         source: Source,
         controller: Controller,
     ) -> None:
@@ -1504,7 +1488,7 @@ def __init__(
         self.setLayout(layout)
 
         self.conversation_title_bar = SourceProfileShortWidget(source, controller)
-        self.conversation_view = ConversationView(session_maker, source, controller)
+        self.conversation_view = ConversationView(source, controller)
         self.reply_box = ReplyBoxWidget(source, controller)
 
         layout.addWidget(self.conversation_title_bar, 1)
diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index 704fcf38e..b797edf4f 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -501,8 +501,8 @@ def on_file_open(self, file_db_object):
         # Once downloaded, submissions are stored in the data directory
         # with the same filename as the server, except with the .gz.gpg
         # stripped off.
-        server_filename = file_db_object.filename
-        fn_no_ext, _ = os.path.splitext(os.path.splitext(server_filename)[0])
+        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)
 
         if self.proxy:
@@ -641,3 +641,8 @@ def on_reply_success(self, result, current_object: Tuple[str, str]) -> None:
     def on_reply_failure(self, result, current_object: Tuple[str, str]) -> None:
         source_uuid, reply_uuid = current_object
         self.reply_failed.emit(reply_uuid)
+
+    def get_file(self, file_uuid: str) -> db.File:
+        file = storage.get_file(self.session, file_uuid)
+        self.session.refresh(file)
+        return file

From 161b93f36d0227788804bd187a9617a4fd78df51 Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Fri, 24 May 2019 14:42:02 -0700
Subject: [PATCH 14/17] fix file open

---
 securedrop_client/logic.py   | 2 +-
 securedrop_client/storage.py | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index b797edf4f..6b5c5baf2 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -494,7 +494,7 @@ def set_status(self, message, duration=5000):
         """
         self.gui.update_activity_status(message, duration)
 
-    def on_file_open(self, file_db_object):
+    def on_file_open(self, file_uuid: str) -> None:
         """
         Open the already downloaded file associated with the message (which is a `File`).
         """
diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py
index c64c31996..7d04ec345 100644
--- a/securedrop_client/storage.py
+++ b/securedrop_client/storage.py
@@ -422,3 +422,6 @@ def source_exists(session: Session, source_uuid: str) -> bool:
         return True
     except NoResultFound:
         return False
+
+def get_file(session: Session, file_uuid: str) -> File:
+    return session.query(File).filter_by(uuid=file_uuid).one()

From 60e2244b9b7135bb63be6c80a615c5459bf2ac39 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Tue, 28 May 2019 10:42:25 +0200
Subject: [PATCH 15/17] fixed tests, lints

---
 securedrop_client/storage.py |   1 +
 tests/factory.py             |   2 +-
 tests/gui/test_main.py       |  56 +++----
 tests/gui/test_widgets.py    | 114 +++++++++------
 tests/test_app.py            |   2 +-
 tests/test_logic.py          |  13 +-
 tests/test_message_sync.py   | 273 +++++++++++++++++------------------
 7 files changed, 233 insertions(+), 228 deletions(-)

diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py
index 7d04ec345..845e7e9cd 100644
--- a/securedrop_client/storage.py
+++ b/securedrop_client/storage.py
@@ -423,5 +423,6 @@ def source_exists(session: Session, source_uuid: str) -> bool:
     except NoResultFound:
         return False
 
+
 def get_file(session: Session, file_uuid: str) -> File:
     return session.query(File).filter_by(uuid=file_uuid).one()
diff --git a/tests/factory.py b/tests/factory.py
index eb2a5255b..7c3a19b8f 100644
--- a/tests/factory.py
+++ b/tests/factory.py
@@ -83,7 +83,7 @@ def File(**attrs):
     FILE_COUNT += 1
     defaults = dict(
         uuid='source-uuid-{}'.format(FILE_COUNT),
-        filename='{}-reply.gpg'.format(FILE_COUNT),
+        filename='{}-doc.gz.gpg'.format(FILE_COUNT),
         size=123,
         download_url='http://wat.onion/abc',
         is_decrypted=True,
diff --git a/tests/gui/test_main.py b/tests/gui/test_main.py
index a46240aba..03e867524 100644
--- a/tests/gui/test_main.py
+++ b/tests/gui/test_main.py
@@ -22,13 +22,12 @@ def test_init(mocker):
     mock_mv = mocker.patch('securedrop_client.gui.main.MainView')
     mocker.patch('securedrop_client.gui.main.QHBoxLayout', mock_lo)
     mocker.patch('securedrop_client.gui.main.QMainWindow')
-    mock_session_maker = mocker.MagicMock()
 
-    w = Window(mock_session_maker)
+    w = Window()
 
     mock_li.assert_called_once_with(w.icon)
     mock_lp.assert_called_once_with()
-    mock_mv.assert_called_once_with(mock_session_maker, w.main_pane)
+    mock_mv.assert_called_once_with(w.main_pane)
     assert mock_lo().addWidget.call_count == 2
 
 
@@ -37,10 +36,9 @@ def test_setup(mocker):
     Ensure the passed in controller is referenced and the various views are
     instantiated as expected.
     """
-    mock_session_maker = mocker.MagicMock()
     mock_controller = mocker.MagicMock()
 
-    w = Window(mock_session_maker)
+    w = Window()
     w.show_login = mocker.MagicMock()
     w.top_pane = mocker.MagicMock()
     w.left_pane = mocker.MagicMock()
@@ -56,8 +54,7 @@ def test_setup(mocker):
 
 
 def test_show_main_window(mocker):
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.autosize_window = mocker.MagicMock()
     w.show = mocker.MagicMock()
     w.set_logged_in_as = mocker.MagicMock()
@@ -70,8 +67,7 @@ def test_show_main_window(mocker):
 
 
 def test_show_main_window_without_username(mocker):
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.autosize_window = mocker.MagicMock()
     w.show = mocker.MagicMock()
     w.set_logged_in_as = mocker.MagicMock()
@@ -87,8 +83,7 @@ def test_autosize_window(mocker):
     """
     Check the autosizing fits to the full screen size.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.resize = mocker.MagicMock()
     mock_screen = mocker.MagicMock()
     mock_screen.width.return_value = 1024
@@ -105,8 +100,7 @@ def test_show_login(mocker):
     """
     The login dialog is displayed with a clean state.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.controller = mocker.MagicMock()
     mock_ld = mocker.patch('securedrop_client.gui.main.LoginDialog')
 
@@ -121,8 +115,7 @@ def test_show_login_error(mocker):
     """
     Ensures that an error message is displayed in the login dialog.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.show_login = mocker.MagicMock()
     w.setup(mocker.MagicMock())
     w.login_dialog = mocker.MagicMock()
@@ -136,8 +129,7 @@ def test_hide_login(mocker):
     """
     Ensure the login dialog is closed and hidden.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.show_login = mocker.MagicMock()
     ld = mocker.MagicMock()
     w.login_dialog = ld
@@ -152,8 +144,7 @@ def test_show_sources(mocker):
     """
     Ensure the sources list is passed to the main view to be updated.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.main_view = mocker.MagicMock()
     w.show_sources([1, 2, 3])
     w.main_view.show_sources.assert_called_once_with([1, 2, 3])
@@ -164,8 +155,7 @@ def test_update_error_status_default(mocker):
     Ensure that the error to be shown in the error status bar will be passed to the top pane with a
     default duration of 10 seconds.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.top_pane = mocker.MagicMock()
     w.update_error_status(message='test error message')
     w.top_pane.update_error_status.assert_called_once_with('test error message', 10000)
@@ -176,8 +166,7 @@ def test_update_error_status(mocker):
     Ensure that the error to be shown in the error status bar will be passed to the top pane with
     the duration of seconds provided.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.top_pane = mocker.MagicMock()
     w.update_error_status(message='test error message', duration=123)
     w.top_pane.update_error_status.assert_called_once_with('test error message', 123)
@@ -188,8 +177,7 @@ def test_update_activity_status_default(mocker):
     Ensure that the activity to be shown in the activity status bar will be passed to the top pane
     with a default duration of 0 seconds, i.e. forever.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.top_pane = mocker.MagicMock()
     w.update_activity_status(message='test message')
     w.top_pane.update_activity_status.assert_called_once_with('test message', 0)
@@ -200,8 +188,7 @@ def test_update_activity_status(mocker):
     Ensure that the activity to be shown in the activity status bar will be passed to the top pane
     with the duration of seconds provided.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.top_pane = mocker.MagicMock()
     w.update_activity_status(message='test message', duration=123)
     w.top_pane.update_activity_status.assert_called_once_with('test message', 123)
@@ -211,8 +198,7 @@ def test_clear_error_status(mocker):
     """
     Ensure clear_error_status is called.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.top_pane = mocker.MagicMock()
 
     w.clear_error_status()
@@ -224,8 +210,7 @@ def test_show_sync(mocker):
     """
     If there's a value display the result of its "humanize" method.humanize
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.update_activity_status = mocker.MagicMock()
     updated_on = mocker.MagicMock()
     w.show_sync(updated_on)
@@ -237,8 +222,7 @@ def test_show_sync_no_sync(mocker):
     """
     If there's no value to display, default to a "waiting" message.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.update_activity_status = mocker.MagicMock()
     w.show_sync(None)
     w.update_activity_status.assert_called_once_with('Waiting to refresh...', 5000)
@@ -248,8 +232,7 @@ def test_set_logged_in_as(mocker):
     """
     Given a username, the left pane is appropriately called to update.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.left_pane = mocker.MagicMock()
 
     w.set_logged_in_as('test')
@@ -261,8 +244,7 @@ def test_logout(mocker):
     """
     Ensure the left pane updates to the logged out state.
     """
-    mock_session_maker = mocker.MagicMock()
-    w = Window(mock_session_maker)
+    w = Window()
     w.left_pane = mocker.MagicMock()
     w.top_pane = mocker.MagicMock()
 
diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py
index 42f6ce0cc..285ae1220 100644
--- a/tests/gui/test_widgets.py
+++ b/tests/gui/test_widgets.py
@@ -376,7 +376,6 @@ def test_MainView_init():
     """
     Ensure the MainView instance is correctly set up.
     """
-    mock_session_maker = mocker.MagicMock()
     mv = MainView(None)
     assert isinstance(mv.source_list, SourceList)
     assert isinstance(mv.view_holder, QWidget)
@@ -1137,10 +1136,9 @@ def test_FileWidget_init_left(mocker):
     """
     mock_controller = mocker.MagicMock()
     mock_signal = mocker.MagicMock()  # not important for this test
-    source = factory.Source()
-    file_ = factory.File(is_downloaded=True)
+    mock_uuid = 'mock'
 
-    fw = FileWidget(source, file_, mock_controller, mock_signal)
+    fw = FileWidget(mock_uuid, mock_controller, mock_signal)
 
     assert isinstance(fw.layout.takeAt(0), QWidgetItem)
     assert isinstance(fw.layout.takeAt(0), QWidgetItem)
@@ -1148,44 +1146,63 @@ def test_FileWidget_init_left(mocker):
     assert fw.controller == mock_controller
 
 
-def test_FileWidget_mousePressEvent_download(mocker):
+def test_FileWidget_mousePressEvent_download(mocker, session, source):
     """
     Should fire the expected download event handler in the logic layer.
     """
-    mock_controller = mocker.MagicMock()
     mock_signal = mocker.MagicMock()  # not important for this test
-    source = factory.Source()
-    file_ = factory.File(is_downloaded=False)
 
-    fw = FileWidget(source, file_, mock_controller, mock_signal)
+    file_ = factory.File(source=source['source'],
+                         is_downloaded=False,
+                         is_decrypted=None)
+    session.add(file_)
+    session.commit()
+
+    mock_get_file = mocker.MagicMock(return_value=file_)
+    mock_controller = mocker.MagicMock(get_file=mock_get_file)
+
+    fw = FileWidget(file_.uuid, mock_controller, mock_signal)
+    mock_get_file.assert_called_once_with(file_.uuid)
+    mock_get_file.reset_mock()
+
     fw.mouseReleaseEvent(None)
-    fw.controller.on_file_download.assert_called_once_with(source, file_)
+    mock_get_file.assert_called_once_with(file_.uuid)
+    mock_controller.on_submission_download.assert_called_once_with(
+        db.File, file_.uuid)
 
 
-def test_FileWidget_mousePressEvent_open(mocker):
+def test_FileWidget_mousePressEvent_open(mocker, session, source):
     """
     Should fire the expected open event handler in the logic layer.
     """
-    mock_controller = mocker.MagicMock()
     mock_signal = mocker.MagicMock()  # not important for this test
-    source = factory.Source()
-    file_ = factory.File(is_downloaded=True)
 
-    fw = FileWidget(source, file_, mock_controller, mock_signal)
+    file_ = factory.File(source=source['source'], is_downloaded=True)
+    session.add(file_)
+    session.commit()
+
+    mock_get_file = mocker.MagicMock(return_value=file_)
+    mock_controller = mocker.MagicMock(get_file=mock_get_file)
+
+    fw = FileWidget(file_.uuid, mock_controller, mock_signal)
     fw.mouseReleaseEvent(None)
-    fw.controller.on_file_open.assert_called_once_with(file_)
+    fw.controller.on_file_open.assert_called_once_with(file_.uuid)
 
 
-def test_FileWidget_clear_deletes_items(mocker, homedir):
+def test_FileWidget_clear_deletes_items(mocker, session, source):
     """
     Calling the clear() method on FileWidget should delete the existing items in the layout.
     """
-    mock_controller = mocker.MagicMock()
     mock_signal = mocker.MagicMock()  # not important for this test
-    source = factory.Source()
-    file_ = factory.File(is_downloaded=True)
 
-    fw = FileWidget(source, file_, mock_controller, mock_signal)
+    file_ = factory.File(source=source['source'], is_downloaded=True)
+    session.add(file_)
+    session.commit()
+
+    mock_get_file = mocker.MagicMock(return_value=file_)
+    mock_controller = mocker.MagicMock(get_file=mock_get_file)
+
+    fw = FileWidget(file_.uuid, mock_controller, mock_signal)
     assert fw.layout.count() != 0
 
     fw.clear()
@@ -1193,39 +1210,49 @@ def test_FileWidget_clear_deletes_items(mocker, homedir):
     assert fw.layout.count() == 0
 
 
-def test_FileWidget_on_file_download_updates_items_when_uuid_matches(mocker, homedir):
+def test_FileWidget_on_file_download_updates_items_when_uuid_matches(mocker, source, session):
     """
     The _on_file_download method should clear and update the FileWidget
     """
-    mock_controller = mocker.MagicMock()
     mock_signal = mocker.MagicMock()  # not important for this test
-    source = factory.Source()
-    file_ = factory.File(is_downloaded=True)
 
-    fw = FileWidget(source, file_, mock_controller, mock_signal)
+    file_ = factory.File(source=source['source'], is_downloaded=True)
+    session.add(file_)
+    session.commit()
+
+    mock_get_file = mocker.MagicMock(return_value=file_)
+    mock_controller = mocker.MagicMock(get_file=mock_get_file)
+
+    fw = FileWidget(file_.uuid, mock_controller, mock_signal)
     fw.clear = mocker.MagicMock()
     fw.update = mocker.MagicMock()
 
-    fw._on_file_download(file_.uuid)
+    fw._on_file_downloaded(file_.uuid)
 
     fw.clear.assert_called_once_with()
     fw.update.assert_called_once_with()
 
 
-def test_FileWidget_on_file_download_updates_items_when_uuid_does_not_match(mocker, homedir):
+def test_FileWidget_on_file_download_updates_items_when_uuid_does_not_match(
+    mocker, homedir, session, source,
+):
     """
     The _on_file_download method should clear and update the FileWidget
     """
-    mock_controller = mocker.MagicMock()
     mock_signal = mocker.MagicMock()  # not important for this test
-    source = factory.Source()
-    file_ = factory.File(is_downloaded=True)
 
-    fw = FileWidget(source, file_, mock_controller, mock_signal)
+    file_ = factory.File(source=source['source'], is_downloaded=True)
+    session.add(file_)
+    session.commit()
+
+    mock_get_file = mocker.MagicMock(return_value=file_)
+    mock_controller = mocker.MagicMock(get_file=mock_get_file)
+
+    fw = FileWidget(file_.uuid, mock_controller, mock_signal)
     fw.clear = mocker.MagicMock()
     fw.update = mocker.MagicMock()
 
-    fw._on_file_download('not a matching uuid')
+    fw._on_file_downloaded('not a matching uuid')
 
     fw.clear.assert_not_called()
     fw.update.assert_not_called()
@@ -1513,26 +1540,29 @@ def test_ConversationView_add_downloaded_file(mocker, homedir):
     assert isinstance(cal[0][0][0], FileWidget)
 
 
-def test_ConversationView_add_not_downloaded_file(mocker, homedir):
+def test_ConversationView_add_not_downloaded_file(mocker, homedir, source, session):
     """
     Adding a file results in a new FileWidget added to the layout with the
     proper QLabel.
     """
-    mocked_source = mocker.MagicMock()
-    mocked_controller = mocker.MagicMock()
+    file_ = factory.File(source=source['source'],
+                         is_downloaded=False,
+                         is_decrypted=None,
+                         size=123)
+    session.add(file_)
+    session.commit()
 
-    cv = ConversationView(mocked_source, mocked_controller)
+    mock_get_file = mocker.MagicMock(return_value=file_)
+    mocked_controller = mocker.MagicMock(get_file=mock_get_file)
+
+    cv = ConversationView(source['source'], mocked_controller)
     cv.conversation_layout = mocker.MagicMock()
 
-    mock_source = mocker.MagicMock()
-    mock_file = mocker.MagicMock()
-    mock_file.is_downloaded = False
-    mock_file.size = 123
     mock_label = mocker.patch('securedrop_client.gui.widgets.QLabel')
     mocker.patch('securedrop_client.gui.widgets.QHBoxLayout.addWidget')
     mocker.patch('securedrop_client.gui.widgets.FileWidget.setLayout')
 
-    cv.add_file(mock_source, mock_file)
+    cv.add_file(source['source'], file_)
     mock_label.assert_called_with("Download (123 bytes)")
     assert cv.conversation_layout.addWidget.call_count == 1
 
diff --git a/tests/test_app.py b/tests/test_app.py
index 61691a522..cc35c6a5d 100644
--- a/tests/test_app.py
+++ b/tests/test_app.py
@@ -127,7 +127,7 @@ def test_start_app(homedir, mocker):
 
     start_app(mock_args, mock_qt_args)
     mock_app.assert_called_once_with(mock_qt_args)
-    mock_win.assert_called_once_with(mock_session_maker)
+    mock_win.assert_called_once_with()
     mock_controller.assert_called_once_with('http://localhost:8081/',
                                             mock_win(), mock_session_maker,
                                             homedir, False)
diff --git a/tests/test_logic.py b/tests/test_logic.py
index 24c05e898..c800c1a17 100644
--- a/tests/test_logic.py
+++ b/tests/test_logic.py
@@ -8,7 +8,7 @@
 import sdclientapi
 
 from PyQt5.QtCore import Qt
-from sdclientapi import sdlocalobjects, RequestTimeoutError
+from sdclientapi import RequestTimeoutError
 from tests import factory
 
 from securedrop_client import storage, db
@@ -912,7 +912,7 @@ def test_Controller_on_file_downloaded_api_failure(homedir, config, mocker, sess
     mock_file_ready.emit.assert_not_called()
 
 
-def test_Controller_on_file_open(homedir, config, mocker, session_maker):
+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.
@@ -921,12 +921,15 @@ def test_Controller_on_file_open(homedir, config, mocker, session_maker):
     mock_gui = mocker.MagicMock()
     co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.proxy = True
-    mock_submission = mocker.MagicMock()
-    mock_submission.filename = '1-test.pdf'
+
+    submission = factory.File(source=source['source'])
+    session.add(submission)
+    session.commit()
+
     mock_subprocess = mocker.MagicMock()
     mock_process = mocker.MagicMock(return_value=mock_subprocess)
     mocker.patch('securedrop_client.logic.QProcess', mock_process)
-    co.on_file_open(mock_submission)
+    co.on_file_open(submission.uuid)
     mock_process.assert_called_once_with(co)
     mock_subprocess.start.call_count == 1
 
diff --git a/tests/test_message_sync.py b/tests/test_message_sync.py
index e6dafc39a..c47855594 100644
--- a/tests/test_message_sync.py
+++ b/tests/test_message_sync.py
@@ -1,32 +1,31 @@
 """
 Make sure the message sync object behaves as expected.
 """
-from securedrop_client.crypto import CryptoError
+from securedrop_client.crypto import GpgHelper, CryptoError
 from securedrop_client.message_sync import MessageSync, ReplySync
 from tests import factory
 
 
-def test_MessageSync_init(mocker, homedir):
+def test_MessageSync_init(mocker, session_maker):
     """
     Ensure things are set up as expected
     """
-    # patch the session and use our own
-    mock_session_class = mocker.MagicMock()
-    mocker.patch('securedrop_client.db.make_engine')
-    mocker.patch('securedrop_client.message_sync.sessionmaker', return_value=mock_session_class)
 
-    api = mocker.MagicMock()
-    is_qubes = False
+    mock_api = mocker.MagicMock()
+    mock_gpg = mocker.MagicMock()
 
-    ms = MessageSync(api, homedir, is_qubes)
+    ms = MessageSync(mock_api, mock_gpg, session_maker)
 
-    assert ms.home == homedir
-    assert ms.api == api
-    assert ms.session == mock_session_class()
+    assert ms.api == mock_api
+    assert ms.gpg == mock_gpg
+    assert ms.session_maker == session_maker
 
 
-def test_MessageSync_run_success(mocker, session, source):
-    """Test when a message successfully downloads and decrypts."""
+def test_MessageSync_run_success(mocker, session, source, session_maker, homedir):
+    """
+    Test when a message successfully downloads and decrypts.
+    Using the `homedir` fixture to get a GPG keyring.
+    """
     message = factory.Message(source=source['source'],
                               is_downloaded=False,
                               is_decrypted=None,
@@ -36,17 +35,6 @@ def test_MessageSync_run_success(mocker, session, source):
 
     expected_content = 'foo'
 
-    def set_object_decryption_status_with_content_side_effect(*nargs, **kwargs):
-        message.content = expected_content
-
-    # mock the fetching of submissions
-    mocker.patch('securedrop_client.storage.find_new_messages', return_value=[message])
-    mock_download_status = mocker.patch(
-        'securedrop_client.message_sync.storage.mark_message_as_downloaded')
-    mock_decryption_status = mocker.patch(
-        'securedrop_client.message_sync.storage.set_object_decryption_status_with_content',
-        side_effect=set_object_decryption_status_with_content_side_effect)
-
     # don't create the signal
     mocker.patch('securedrop_client.message_sync.pyqtSignal')
 
@@ -54,13 +42,15 @@ def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
         with open(plaintext_filename, 'w') as f:
             f.write(expected_content)
 
-    # mock the GpgHelper creation since we don't have directories/keys setup
-    mock_gpg_helper = mocker.MagicMock(decrypt_submission_or_reply=mock_decrypt_submission_or_reply)
-    mocker.patch('securedrop_client.message_sync.GpgHelper', return_value=mock_gpg_helper)
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    mocker.patch.object(
+        gpg,
+        'decrypt_submission_or_reply',
+        side_effect=mock_decrypt_submission_or_reply,
+    )
 
     api = mocker.MagicMock(session=session)
-    ms = MessageSync(api, 'mock', True)
-    ms.session = session  # "patch" it with a real session
+    ms = MessageSync(api, gpg, session_maker)
     ms.api.download_submission = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
 
     mock_message_ready = mocker.patch.object(ms, 'message_ready')
@@ -68,13 +58,19 @@ def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
     # check that it runs without raising exceptions
     ms.run(False)
 
-    mock_decryption_status.assert_called_once_with(message, api.session, True, expected_content)
-    mock_download_status.called_once_with(message, api.mock_session)
     mock_message_ready.emit.assert_called_once_with(message.uuid, expected_content)
 
+    session.refresh(message)
+    assert message.content == expected_content
+    assert message.is_downloaded is True
+    assert message.is_decrypted is True
 
-def test_MessageSync_run_decryption_error(mocker, session, source):
-    """Test when a message successfully downloads, but does not successfully decrypt."""
+
+def test_MessageSync_run_decryption_error(mocker, session, source, session_maker, homedir):
+    """
+    Test when a message successfully downloads, but does not successfully decrypt.
+    Using the `homedir` fixture to get a GPG keyring.
+    """
     message = factory.Message(source=source['source'],
                               is_downloaded=False,
                               is_decrypted=None,
@@ -82,22 +78,14 @@ def test_MessageSync_run_decryption_error(mocker, session, source):
     session.add(message)
     session.commit()
 
-    # mock the fetching of submissions
-    mocker.patch('securedrop_client.storage.find_new_messages', return_value=[message])
-    mock_download_status = mocker.patch(
-        'securedrop_client.message_sync.storage.mark_message_as_downloaded')
-    mock_decryption_status = mocker.patch(
-        'securedrop_client.message_sync.storage.set_object_decryption_status_with_content')
-
     # don't create the signal
     mocker.patch('securedrop_client.message_sync.pyqtSignal')
-    # mock the GpgHelper creation since we don't have directories/keys setup
-    mocker.patch('securedrop_client.message_sync.GpgHelper')
 
     api = mocker.MagicMock(session=session)
 
-    ms = MessageSync(api, 'mock', True)
-    ms.session = session  # "patch" it with a real session
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+
+    ms = MessageSync(api, gpg, session_maker)
     mocker.patch.object(ms.gpg, 'decrypt_submission_or_reply', side_effect=CryptoError)
 
     ms.api.download_submission = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
@@ -107,60 +95,73 @@ def test_MessageSync_run_decryption_error(mocker, session, source):
     # check that it runs without raising exceptions
     ms.run(False)
 
-    mock_download_status.assert_called_once_with(message.uuid, session)
-    mock_decryption_status.assert_called_once_with(message, api.session, False)
     mock_message_ready.emit.assert_called_once_with(message.uuid, '<Message not yet available>')
+    assert message.content is None
+    assert message.is_downloaded is True
+    assert message.is_decrypted is False
 
 
-def test_MessageSync_exception(homedir, config, mocker, source):
+def test_MessageSync_exception(homedir, config, mocker, source, session, session_maker):
     """
-    Mostly here for code coverage- makes sure that if an exception is
-    raised in the download thread, the code which catches it is actually
-    run.
+    Makes sure that if an exception is raised in the download thread, the code which catches it is
+    actually run.
     Using the `config` fixture to ensure the config is written to disk.
     """
-    message = factory.Message(source=source['source'])
-    api = mocker.MagicMock()
-    is_qubes = False
+    message = factory.Message(source=source['source'],
+                              is_downloaded=False,
+                              is_decrypted=None,
+                              content=None)
+    session.add(message)
+    session.commit()
 
-    # mock to return the submission we want
-    mocker.patch('securedrop_client.storage.find_new_messages', return_value=[message])
-    mocker.patch('securedrop_client.storage.find_new_files', return_value=[])
-    # mock to prevent GpgHelper from raising errors on init
-    mocker.patch('securedrop_client.crypto.safe_mkdir')
+    mock_message = mocker.patch("sdclientapi.sdlocalobjects.Submission",
+                                mocker.MagicMock(side_effect=Exception()))
 
-    ms = MessageSync(api, str(homedir), is_qubes)
-    mocker.patch.object(ms.gpg, 'decrypt_submission_or_reply', side_effect=CryptoError)
+    api = mocker.MagicMock()
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    ms = MessageSync(api, gpg, session_maker)
 
     # check that it runs without raising exceptions
     ms.run(False)
 
+    # ensure this was called and an exception was raised somewhere
+    assert mock_message.called
 
-def test_MessageSync_run_failure(mocker, source):
-    message = factory.Message(source=source['source'])
 
-    # mock the fetching of submissions
-    mocker.patch('securedrop_client.storage.find_new_messages', return_value=[message])
-    mocker.patch('securedrop_client.storage.find_new_files', return_value=[])
-    # mock the handling of the downloaded files
-    mocker.patch('securedrop_client.message_sync.storage.mark_file_as_downloaded')
-    mocker.patch('securedrop_client.message_sync.storage.mark_message_as_downloaded')
-    # mock the GpgHelper creation since we don't have directories/keys setup
-    mocker.patch('securedrop_client.message_sync.GpgHelper')
+def test_MessageSync_run_failure(mocker, source, session, session_maker, homedir):
+    message = factory.Message(source=source['source'])
+    session.add(message)
+    session.commit()
 
     api = mocker.MagicMock()
-    home = "/home/user/.sd"
-    is_qubes = False
-
-    ms = MessageSync(api, home, is_qubes)
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    ms = MessageSync(api, gpg, session_maker)
     ms.api.download_submission = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
 
     # check that it runs without raising exceptions
     ms.run(False)
 
 
-def test_ReplySync_run_success(mocker, session, source):
-    """Test when a reply successfully downloads and decrypts."""
+def test_ReplySync_init(mocker, session_maker):
+    """
+    Ensure things are set up as expected
+    """
+
+    mock_api = mocker.MagicMock()
+    mock_gpg = mocker.MagicMock()
+
+    rs = ReplySync(mock_api, mock_gpg, session_maker)
+
+    assert rs.api == mock_api
+    assert rs.gpg == mock_gpg
+    assert rs.session_maker == session_maker
+
+
+def test_ReplySync_run_success(mocker, session, source, session_maker, homedir):
+    """
+    Test when a reply successfully downloads and decrypts.
+    Using the `homedir` fixture to get a GPG keyring.
+    """
     reply = factory.Reply(source=source['source'],
                           is_downloaded=False,
                           is_decrypted=None,
@@ -170,17 +171,6 @@ def test_ReplySync_run_success(mocker, session, source):
 
     expected_content = 'foo'
 
-    def set_object_decryption_status_with_content_side_effect(*nargs, **kwargs):
-        reply.content = expected_content
-
-    # mock the fetching of replies
-    mocker.patch('securedrop_client.storage.find_new_replies', return_value=[reply])
-    mock_download_status = mocker.patch(
-        'securedrop_client.message_sync.storage.mark_reply_as_downloaded')
-    mock_decryption_status = mocker.patch(
-        'securedrop_client.message_sync.storage.set_object_decryption_status_with_content',
-        side_effect=set_object_decryption_status_with_content_side_effect)
-
     # don't create the signal
     mocker.patch('securedrop_client.message_sync.pyqtSignal')
 
@@ -188,13 +178,15 @@ def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
         with open(plaintext_filename, 'w') as f:
             f.write(expected_content)
 
-    # mock the GpgHelper creation since we don't have directories/keys setup
-    mock_gpg_helper = mocker.MagicMock(decrypt_submission_or_reply=mock_decrypt_submission_or_reply)
-    mocker.patch('securedrop_client.message_sync.GpgHelper', return_value=mock_gpg_helper)
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    mocker.patch.object(
+        gpg,
+        'decrypt_submission_or_reply',
+        side_effect=mock_decrypt_submission_or_reply,
+    )
 
     api = mocker.MagicMock(session=session)
-    rs = ReplySync(api, 'mock', True)
-    rs.session = session  # "patch" it with a real session
+    rs = ReplySync(api, gpg, session_maker)
     rs.api.download_reply = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
 
     mock_reply_ready = mocker.patch.object(rs, 'reply_ready')
@@ -202,34 +194,19 @@ def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
     # check that it runs without raising exceptions
     rs.run(False)
 
-    mock_decryption_status.assert_called_once_with(reply, api.session, True, expected_content)
-    mock_download_status.called_once_with(reply, api.mock_session)
     mock_reply_ready.emit.assert_called_once_with(reply.uuid, expected_content)
 
+    session.refresh(reply)
+    assert reply.content == expected_content
+    assert reply.is_downloaded is True
+    assert reply.is_decrypted is True
+
 
-def test_ReplySync_exception(mocker):
+def test_ReplySync_run_decryption_error(mocker, session, source, session_maker, homedir):
     """
-    Mostly here for code coverage- makes sure that if an exception is
-    raised in the download thread, the code which catches it is actually
-    run
+    Test when a reply successfully downloads, but does not successfully decrypt.
+    Using the `homedir` fixture to get a GPG keyring.
     """
-    reply = factory.Reply()
-    api = mocker.MagicMock()
-    home = "/home/user/.sd"
-    is_qubes = False
-
-    mocker.patch('securedrop_client.storage.find_new_replies', return_value=[reply])
-    mocker.patch('securedrop_client.message_sync.GpgHelper')
-    mocker.patch("sdclientapi.sdlocalobjects.Reply", mocker.MagicMock(side_effect=Exception()))
-
-    rs = ReplySync(api, home, is_qubes)
-
-    # check that it runs without raise exceptions
-    rs.run(False)
-
-
-def test_ReplySync_run_decryption_error(mocker, session, source):
-    """Test when a reply successfully downloads, but does not successfully decrypt."""
     reply = factory.Reply(source=source['source'],
                           is_downloaded=False,
                           is_decrypted=None,
@@ -237,22 +214,14 @@ def test_ReplySync_run_decryption_error(mocker, session, source):
     session.add(reply)
     session.commit()
 
-    # mock the fetching of replies
-    mocker.patch('securedrop_client.storage.find_new_replies', return_value=[reply])
-    mock_download_status = mocker.patch(
-        'securedrop_client.message_sync.storage.mark_reply_as_downloaded')
-    mock_decryption_status = mocker.patch(
-        'securedrop_client.message_sync.storage.set_object_decryption_status_with_content')
-
     # don't create the signal
     mocker.patch('securedrop_client.message_sync.pyqtSignal')
-    # mock the GpgHelper creation since we don't have directories/keys setup
-    mocker.patch('securedrop_client.message_sync.GpgHelper')
 
     api = mocker.MagicMock(session=session)
 
-    rs = ReplySync(api, 'mock', True)
-    rs.session = session  # "patch" it with a real session
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+
+    rs = ReplySync(api, gpg, session_maker)
     mocker.patch.object(rs.gpg, 'decrypt_submission_or_reply', side_effect=CryptoError)
 
     rs.api.download_reply = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
@@ -262,28 +231,48 @@ def test_ReplySync_run_decryption_error(mocker, session, source):
     # check that it runs without raising exceptions
     rs.run(False)
 
-    mock_download_status.assert_called_once_with(reply.uuid, session)
-    mock_decryption_status.assert_called_once_with(reply, api.session, False)
     mock_reply_ready.emit.assert_called_once_with(reply.uuid, '<Reply not yet available>')
+    assert reply.content is None
+    assert reply.is_downloaded is True
+    assert reply.is_decrypted is False
 
 
-def test_ReplySync_run_failure(mocker, session, source):
-    reply = factory.Reply(source=source['source'])
+def test_ReplySync_exception(homedir, config, mocker, source, session, session_maker):
+    """
+    Makes sure that if an exception is raised in the download thread, the code which catches it is
+    actually run.
+    Using the `config` fixture to ensure the config is written to disk.
+    """
+    reply = factory.Reply(source=source['source'],
+                          is_downloaded=False,
+                          is_decrypted=None,
+                          content=None)
     session.add(reply)
     session.commit()
 
-    # mock finding new replies
-    mocker.patch('securedrop_client.storage.find_new_replies', return_value=[reply])
-    # mock handling the new reply
-    mocker.patch('securedrop_client.message_sync.storage.mark_reply_as_downloaded')
-    mocker.patch('securedrop_client.message_sync.GpgHelper')
+    mock_reply = mocker.patch("sdclientapi.sdlocalobjects.Reply",
+                              mocker.MagicMock(side_effect=Exception()))
 
     api = mocker.MagicMock()
-    home = "/home/user/.sd"
-    is_qubes = False
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    rs = ReplySync(api, gpg, session_maker)
 
-    ms = ReplySync(api, home, is_qubes)
-    ms.api.download_submission = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
+    # check that it runs without raising exceptions
+    rs.run(False)
 
-    # check that it runs without raise exceptions
-    ms.run(False)
+    # ensure this was called and an exception was raised somewhere
+    assert mock_reply.called
+
+
+def test_ReplySync_run_failure(mocker, source, session, session_maker, homedir):
+    reply = factory.Reply(source=source['source'])
+    session.add(reply)
+    session.commit()
+
+    api = mocker.MagicMock()
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    rs = ReplySync(api, gpg, session_maker)
+    rs.api.download_reply = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
+
+    # check that it runs without raising exceptions
+    rs.run(False)

From 434954cc3283426492822e0e3e3731789326717b Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Tue, 28 May 2019 10:48:14 +0200
Subject: [PATCH 16/17] remove dead code

---
 securedrop_client/logic.py | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index 6b5c5baf2..60689c7b2 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -518,27 +518,6 @@ def on_file_open(self, file_uuid: str) -> None:
             # Non Qubes OS. Just log the event for now.
             logger.info('Opening file "{}".'.format(submission_filepath))
 
-    def on_reply_download(self, source_db_object: db.Source, reply: db.Reply) -> None:
-        """
-        Download the file associated with the Reply.
-        """
-        if not self.api:  # Then we should tell the user they need to login.
-            self.on_action_requiring_login()
-            return
-
-        sdk_object = sdclientapi.Reply(uuid=reply.uuid)
-        sdk_object.filename = reply.filename
-        sdk_object.source_uuid = source_db_object.uuid
-
-        self.set_status(_('Downloading {}'.format(sdk_object.filename)))
-
-        self.call_api(self.api.download_reply,
-                      self.on_file_download_success,
-                      self.on_file_download_failure,
-                      sdk_object,
-                      self.data_dir,
-                      current_object=reply)
-
     def on_submission_download(
         self,
         submission_type: Union[Type[db.File], Type[db.Message]],

From d842bc7457ae105c25d9fbcf2905000a5735fc92 Mon Sep 17 00:00:00 2001
From: heartsucker <heartsucker@autistici.org>
Date: Tue, 28 May 2019 11:49:29 +0200
Subject: [PATCH 17/17] final tests for queue, fixes for message sync

---
 tests/test_message_sync.py |  94 ++++++++++++++++++++++++++++++++++
 tests/test_queue.py        | 102 ++++++++++++++++++++++++++++++++++---
 2 files changed, 190 insertions(+), 6 deletions(-)

diff --git a/tests/test_message_sync.py b/tests/test_message_sync.py
index c47855594..73533ad8b 100644
--- a/tests/test_message_sync.py
+++ b/tests/test_message_sync.py
@@ -66,6 +66,53 @@ def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
     assert message.is_decrypted is True
 
 
+def test_MessageSync_run_decrypt_only(mocker, session, source, session_maker, homedir):
+    """
+    Test when a message successfully downloads and decrypts.
+    Using the `homedir` fixture to get a GPG keyring.
+    """
+    message = factory.Message(source=source['source'],
+                              is_downloaded=True,
+                              is_decrypted=False,
+                              content=None)
+    session.add(message)
+    session.commit()
+
+    expected_content = 'foo'
+
+    # don't create the signal
+    mocker.patch('securedrop_client.message_sync.pyqtSignal')
+
+    def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
+        with open(plaintext_filename, 'w') as f:
+            f.write(expected_content)
+
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    mocker.patch.object(
+        gpg,
+        'decrypt_submission_or_reply',
+        side_effect=mock_decrypt_submission_or_reply,
+    )
+
+    api = mocker.MagicMock(session=session)
+    ms = MessageSync(api, gpg, session_maker)
+    ms.api.download_submission = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
+    mock_fetch = mocker.patch.object(ms, 'fetch_the_thing')
+
+    mock_message_ready = mocker.patch.object(ms, 'message_ready')
+
+    # check that it runs without raising exceptions
+    ms.run(False)
+
+    mock_message_ready.emit.assert_called_once_with(message.uuid, expected_content)
+
+    session.refresh(message)
+    assert message.content == expected_content
+    assert message.is_downloaded is True
+    assert message.is_decrypted is True
+    assert not mock_fetch.called
+
+
 def test_MessageSync_run_decryption_error(mocker, session, source, session_maker, homedir):
     """
     Test when a message successfully downloads, but does not successfully decrypt.
@@ -202,6 +249,53 @@ def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
     assert reply.is_decrypted is True
 
 
+def test_ReplySync_run_decrypt_only(mocker, session, source, session_maker, homedir):
+    """
+    Test when a message successfully downloads and decrypts.
+    Using the `homedir` fixture to get a GPG keyring.
+    """
+    reply = factory.Reply(source=source['source'],
+                          is_downloaded=True,
+                          is_decrypted=False,
+                          content=None)
+    session.add(reply)
+    session.commit()
+
+    expected_content = 'foo'
+
+    # don't create the signal
+    mocker.patch('securedrop_client.message_sync.pyqtSignal')
+
+    def mock_decrypt_submission_or_reply(filepath, plaintext_filename, is_doc):
+        with open(plaintext_filename, 'w') as f:
+            f.write(expected_content)
+
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    mocker.patch.object(
+        gpg,
+        'decrypt_submission_or_reply',
+        side_effect=mock_decrypt_submission_or_reply,
+    )
+
+    api = mocker.MagicMock(session=session)
+    rs = ReplySync(api, gpg, session_maker)
+    rs.api.download_submission = mocker.MagicMock(return_value=(1234, "/home/user/downloads/foo"))
+    mock_fetch = mocker.patch.object(rs, 'fetch_the_thing')
+
+    mock_message_ready = mocker.patch.object(rs, 'reply_ready')
+
+    # check that it runs without raising exceptions
+    rs.run(False)
+
+    mock_message_ready.emit.assert_called_once_with(reply.uuid, expected_content)
+
+    session.refresh(reply)
+    assert reply.content == expected_content
+    assert reply.is_downloaded is True
+    assert reply.is_decrypted is True
+    assert not mock_fetch.called
+
+
 def test_ReplySync_run_decryption_error(mocker, session, source, session_maker, homedir):
     """
     Test when a reply successfully downloads, but does not successfully decrypt.
diff --git a/tests/test_queue.py b/tests/test_queue.py
index 2847b410f..41362a9a1 100644
--- a/tests/test_queue.py
+++ b/tests/test_queue.py
@@ -11,9 +11,9 @@
 from typing import Tuple
 
 from securedrop_client import db
-from securedrop_client.crypto import GpgHelper
+from securedrop_client.crypto import GpgHelper, CryptoError
 from securedrop_client.queue import ApiInaccessibleError, ApiJob, RunnableQueue, \
-    DownloadSubmissionJob
+    DownloadSubmissionJob, ApiJobQueue
 
 
 def test_ApiInaccessibleError_init():
@@ -219,6 +219,7 @@ def test_DownloadSubmissionJob_happy_path_no_etag(mocker, homedir, session, sess
     session.commit()
 
     gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    mock_decrypt = mocker.patch.object(gpg, 'decrypt_submission_or_reply')
 
     def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
         '''
@@ -239,7 +240,6 @@ def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
         gpg,
     )
 
-    mock_decrypt = mocker.patch.object(job, '_decrypt_file')
     mock_logger = mocker.patch('securedrop_client.queue.logger')
 
     job.call_api(api_client, session)
@@ -259,6 +259,7 @@ def test_DownloadSubmissionJob_happy_path_sha256_etag(mocker, homedir, session,
     session.commit()
 
     gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    mock_decrypt = mocker.patch.object(gpg, 'decrypt_submission_or_reply')
 
     def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
         '''
@@ -282,8 +283,6 @@ def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
         gpg,
     )
 
-    mock_decrypt = mocker.patch.object(job, '_decrypt_file')
-
     job.call_api(api_client, session)
 
     # ensure mocks aren't stale
@@ -354,7 +353,7 @@ def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
         gpg,
     )
 
-    mock_decrypt = mocker.patch.object(job, '_decrypt_file')
+    mock_decrypt = mocker.patch('securedrop_client.crypto.GpgHelper.decrypt_submission_or_reply')
     mock_logger = mocker.patch('securedrop_client.queue.logger')
 
     job.call_api(api_client, session)
@@ -364,3 +363,94 @@ def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
 
     # ensure mocks aren't stale
     assert mock_decrypt.called
+
+
+def test_DownloadSubmissionJob_decryption_error(mocker, homedir, session, session_maker):
+    source = factory.Source()
+    file_ = factory.File(source=source)
+    session.add(source)
+    session.add(file_)
+    session.commit()
+
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
+    mock_decrypt = mocker.patch.object(gpg, 'decrypt_submission_or_reply',
+                                       side_effect=CryptoError)
+
+    def fake_download(sdk_obj: sdclientapi.Submission) -> Tuple[str, str]:
+        '''
+        :return: (etag, path_to_dl)
+        '''
+        full_path = os.path.join(homedir, 'somepath')
+        with open(full_path, 'wb') as f:
+            f.write(b'wat')
+
+        # sha256 of b'wat'
+        return ('sha256:f00a787f7492a95e165b470702f4fe9373583fbdc025b2c8bdf0262cc48fcff4',
+                full_path)
+
+    api_client = mocker.MagicMock()
+    api_client.download_submission = fake_download
+
+    job = DownloadSubmissionJob(
+        db.File,
+        file_.uuid,
+        homedir,
+        gpg,
+    )
+
+    mock_logger = mocker.patch('securedrop_client.queue.logger')
+
+    with pytest.raises(CryptoError):
+        job.call_api(api_client, session)
+
+    log_msg = mock_logger.debug.call_args_list[0][0][0]
+    assert log_msg.startswith('Failed to decrypt file')
+
+    # ensure mocks aren't stale
+    assert mock_decrypt.called
+
+
+def test_ApiJobQueue_enqueue(mocker):
+    mock_client = mocker.MagicMock()
+    mock_session_maker = mocker.MagicMock()
+
+    job_queue = ApiJobQueue(mock_client, mock_session_maker)
+    mock_download_queue = mocker.patch.object(job_queue, 'download_queue')
+    mock_main_queue = mocker.patch.object(job_queue, 'main_queue')
+
+    dl_job = DownloadSubmissionJob(db.File, 'mock', 'mock', 'mock')
+    job_queue.enqueue(dl_job)
+
+    mock_download_queue.queue.put_nowait.assert_called_once_with(dl_job)
+    assert not mock_main_queue.queue.put_nowait.called
+
+    # reset for next test
+    mock_download_queue.reset_mock()
+    mock_main_queue.reset_mock()
+
+    dummy_job = dummy_job_factory(mocker, 'mock')()
+    job_queue.enqueue(dummy_job)
+
+    mock_main_queue.queue.put_nowait.assert_called_once_with(dummy_job)
+    assert not mock_download_queue.queue.put_nowait.called
+
+
+def test_ApiJobQueue_start_queues(mocker):
+    mock_api = mocker.MagicMock()
+    mock_client = mocker.MagicMock()
+    mock_session_maker = mocker.MagicMock()
+
+    job_queue = ApiJobQueue(mock_client, mock_session_maker)
+
+    mock_main_queue = mocker.patch.object(job_queue, 'main_queue')
+    mock_download_queue = mocker.patch.object(job_queue, 'download_queue')
+    mock_main_thread = mocker.patch.object(job_queue, 'main_thread')
+    mock_download_thread = mocker.patch.object(job_queue, 'download_thread')
+
+    job_queue.start_queues(mock_api)
+
+    assert mock_main_queue.api_client == mock_api
+    assert mock_download_queue.api_client == mock_api
+
+    mock_main_thread.start.assert_called_once_with()
+    mock_download_thread.start.assert_called_once_with()