From 607a78a768347aad5f86fb8ad52aa4aadf5a7b29 Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Fri, 8 Nov 2019 01:20:00 -0800
Subject: [PATCH 1/8] mitigation for frequent metadata sync timeouts

---
 securedrop_client/logic.py |  5 ++++-
 securedrop_client/queue.py | 10 ++--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index 60d1b61ac..b50ccbae7 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -306,9 +306,12 @@ def login(self, username, password, totp):
         """
         Given a username, password and time based one-time-passcode (TOTP),
         create a new instance representing the SecureDrop api and authenticate.
+
+        Default to 60 seconds until we implement a better request timeout strategy.
         """
         storage.mark_all_pending_drafts_as_failed(self.session)
-        self.api = sdclientapi.API(self.hostname, username, password, totp, self.proxy)
+        self.api = sdclientapi.API(
+            self.hostname, username, password, totp, self.proxy, default_request_timeout=60)
         self.call_api(self.api.authenticate,
                       self.on_authenticate_success,
                       self.on_authenticate_failure)
diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py
index fd9da5124..c4755cc0b 100644
--- a/securedrop_client/queue.py
+++ b/securedrop_client/queue.py
@@ -136,13 +136,12 @@ 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_file_thread = QThread()
 
-        self.main_queue = RunnableQueue(self.api_client, session_maker)
-        self.download_file_queue = RunnableQueue(self.api_client, session_maker)
+        self.main_queue = RunnableQueue(api_client, session_maker)
+        self.download_file_queue = RunnableQueue(api_client, session_maker)
 
         self.main_queue.moveToThread(self.main_thread)
         self.download_file_queue.moveToThread(self.download_file_thread)
@@ -159,11 +158,6 @@ def logout(self) -> None:
 
     def login(self, api_client: API) -> None:
         logger.debug('Passing API token to queues')
-
-        # Setting realistic (shorter) timeout for general requests so that user feedback
-        # is faster
-        api_client.default_request_timeout = 5
-
         self.main_queue.api_client = api_client
         self.download_file_queue.api_client = api_client
         self.start_queues()

From cf211f88b7e8a5d084850bef58ddab290b2c2584 Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Tue, 3 Dec 2019 00:35:13 -0800
Subject: [PATCH 2/8] log and auto-retry bg syncs

---
 securedrop_client/gui/widgets.py |  2 +-
 securedrop_client/logic.py       | 42 ++++++++++++++++++++++++++------
 securedrop_client/queue.py       | 10 ++++++++
 tests/gui/test_widgets.py        |  4 +--
 tests/test_logic.py              | 35 ++++++++++++++++++++++++++
 5 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py
index 06db0caed..f449be4c0 100644
--- a/securedrop_client/gui/widgets.py
+++ b/securedrop_client/gui/widgets.py
@@ -231,7 +231,7 @@ def setup(self, controller):
         self.controller.sync_events.connect(self._on_refresh_complete)
 
     def _on_clicked(self):
-        self.controller.sync_api()
+        self.controller.sync_api(manual_refresh=True)
         # This is a temporary solution for showing the icon as active for the entire duration of a
         # refresh, rather than for just the duration of a click. The icon image will be replaced
         # when the controller tells us the refresh has finished. A cleaner solution would be to
diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index b50ccbae7..a1214c45d 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -304,10 +304,12 @@ def completed_api_call(self, thread_id, user_callback):
 
     def login(self, username, password, totp):
         """
-        Given a username, password and time based one-time-passcode (TOTP),
-        create a new instance representing the SecureDrop api and authenticate.
+        Given a username, password and time based one-time-passcode (TOTP), create a new instance
+        representing the SecureDrop api and authenticate.
 
-        Default to 60 seconds until we implement a better request timeout strategy.
+        Default to 60 seconds until we implement a better request timeout strategy. We lower the
+        default_request_timeout for Queue API requests in ApiJobQueue in order to display errors
+        faster.
         """
         storage.mark_all_pending_drafts_as_failed(self.session)
         self.api = sdclientapi.API(
@@ -366,7 +368,7 @@ def authenticated(self):
         """
         return bool(self.api and self.api.token is not None)
 
-    def sync_api(self):
+    def sync_api(self, manual_refresh: bool = False):
         """
         Grab data from the remote SecureDrop API in a non-blocking manner.
         """
@@ -377,9 +379,20 @@ def sync_api(self):
             logger.debug("You are authenticated, going to make your call")
 
             job = MetadataSyncJob(self.data_dir, self.gpg)
-
             job.success_signal.connect(self.on_sync_success, type=Qt.QueuedConnection)
-            job.failure_signal.connect(self.on_sync_failure, type=Qt.QueuedConnection)
+
+            # If the sync did not originate from a manual refrsh, increase the number of
+            # retry attempts (remaining_attempts) to 15, otherwise use the default so that a user
+            # finds out quicker whether or not their refresh-attempt failed.
+            #
+            # Set up failure-handling depending on whether or not the sync originated from a manual
+            # refresh.
+            if manual_refresh:
+                job.failure_signal.connect(self.on_refresh_failure, type=Qt.QueuedConnection)
+            else:
+                job.remaining_attempts = 15
+                job.failure_signal.connect(self.on_sync_failure, type=Qt.QueuedConnection)
+
             self.api_job_queue.enqueue(job)
 
             logger.debug("In sync_api, after call to submit job to queue, on "
@@ -404,6 +417,8 @@ def on_sync_success(self) -> None:
             * Download new messages and replies
             * Update missing files so that they can be re-downloaded
         """
+        self.set_status('')  # remove any permanent error status message
+
         with open(self.sync_flag, 'w') as f:
             f.write(arrow.now().format())
 
@@ -415,8 +430,21 @@ def on_sync_success(self) -> None:
 
     def on_sync_failure(self, result: Exception) -> None:
         """
-        Called when syncronisation of data via the API queue fails.
+        Called when syncronisation of data via the API fails after a background sync. Resume the
+        queues so that we continue to retry syncing with the server in the background.
+        """
+        logger.debug('The SecureDrop server cannot be reached due to Error: {}'.format(result))
+        self.gui.update_error_status(
+            _('The SecureDrop server cannot be reached.'),
+            duration=0,
+            retry=True)
+        self.resume_queues()
+
+    def on_refresh_failure(self, result: Exception) -> None:
+        """
+        Called when syncronisation of data via the API fails after a user manual clicks refresh.
         """
+        logger.debug('The SecureDrop server cannot be reached due to Error: {}'.format(result))
         self.gui.update_error_status(
             _('The SecureDrop server cannot be reached.'),
             duration=0,
diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py
index c4755cc0b..333b035ce 100644
--- a/securedrop_client/queue.py
+++ b/securedrop_client/queue.py
@@ -158,6 +158,16 @@ def logout(self) -> None:
 
     def login(self, api_client: API) -> None:
         logger.debug('Passing API token to queues')
+
+        # Setting realistic (shorter) timeout for general requests so that user feedback is faster.
+        # General requests includes:
+        #       FileDownloadJob
+        #       MessageDownloadJob
+        #       ReplyDownloadJo
+        #       SendReplyJob
+        #       UpdateStarJob
+        api_client.default_request_timeout = 5
+
         self.main_queue.api_client = api_client
         self.download_file_queue.api_client = api_client
         self.start_queues()
diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py
index 9d38d4828..68cb3d4d9 100644
--- a/tests/gui/test_widgets.py
+++ b/tests/gui/test_widgets.py
@@ -168,14 +168,14 @@ def test_RefreshButton_setup(mocker):
 
 def test_RefreshButton_on_clicked(mocker):
     """
-    When refresh button is clicked, sync_api should be called.
+    When refresh button is clicked, sync_api should be called with manual_refresh set to True.
     """
     rb = RefreshButton()
     rb.controller = mocker.MagicMock()
 
     rb._on_clicked()
 
-    rb.controller.sync_api.assert_called_once_with()
+    rb.controller.sync_api.assert_called_once_with(manual_refresh=True)
 
 
 def test_RefreshButton_on_refresh_complete(mocker):
diff --git a/tests/test_logic.py b/tests/test_logic.py
index 36b756a30..e1dc0c859 100644
--- a/tests/test_logic.py
+++ b/tests/test_logic.py
@@ -376,6 +376,23 @@ def test_Controller_sync_api(homedir, config, mocker, session_maker):
     co.api_job_queue.enqueue.call_count == 1
 
 
+def test_Controller_sync_api_manual_refresh(homedir, config, mocker, session_maker):
+    """
+    Syncing from a manual refresh also enqueues a job.
+    """
+    mock_gui = mocker.MagicMock()
+
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+
+    co.authenticated = mocker.MagicMock(return_value=True)
+    co.api_job_queue = mocker.MagicMock()
+    co.api_job_queue.enqueue = mocker.MagicMock()
+
+    co.sync_api(manual_refresh=True)
+
+    co.api_job_queue.enqueue.call_count == 1
+
+
 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
@@ -417,11 +434,29 @@ def test_Controller_on_sync_failure(homedir, config, mocker, session_maker):
     """
     gui = mocker.MagicMock()
     co = Controller('http://localhost', gui, session_maker, homedir)
+    co.resume_queues = mocker.MagicMock()
     exception = Exception('mock')  # Not the expected tuple.
     mock_storage = mocker.patch('securedrop_client.logic.storage')
 
     co.on_sync_failure(exception)
 
+    assert mock_storage.update_local_storage.call_count == 0
+    co.resume_queues.assert_called_once_with()
+
+
+def test_Controller_on_refresh_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.
+    """
+    gui = mocker.MagicMock()
+    co = Controller('http://localhost', gui, session_maker, homedir)
+    exception = Exception('mock')  # Not the expected tuple.
+    mock_storage = mocker.patch('securedrop_client.logic.storage')
+
+    co.on_refresh_failure(exception)
+
     assert mock_storage.update_local_storage.call_count == 0
     gui.update_error_status.assert_called_once_with(
         'The SecureDrop server cannot be reached.', duration=0, retry=True)

From b173a23dfe032cd9b99dca6c347ca9b06bcf0871 Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Fri, 6 Dec 2019 13:27:03 -0800
Subject: [PATCH 3/8] describe timeouts more accurately

---
 securedrop_client/queue.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py
index 333b035ce..aa595f8c9 100644
--- a/securedrop_client/queue.py
+++ b/securedrop_client/queue.py
@@ -159,13 +159,12 @@ def logout(self) -> None:
     def login(self, api_client: API) -> None:
         logger.debug('Passing API token to queues')
 
-        # Setting realistic (shorter) timeout for general requests so that user feedback is faster.
-        # General requests includes:
-        #       FileDownloadJob
-        #       MessageDownloadJob
-        #       ReplyDownloadJo
-        #       SendReplyJob
-        #       UpdateStarJob
+        # Setting shorter default timeout so that user feedback is faster. For download jobs, this
+        # timeout is adjusted to be more realistic depending on file size.
+        #
+        # Currently ReplyDownloadJobs do not use adjusted timeouts because it is dependent on adding
+        # an optional timeout parameter to the download_reply securedrop-sdk method, see:
+        # https://github.com/freedomofpress/securedrop-sdk/issues/108
         api_client.default_request_timeout = 5
 
         self.main_queue.api_client = api_client

From 0793d4e3048140d1972c1bede5a31d1bc8faf501 Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Mon, 9 Dec 2019 14:31:59 -0800
Subject: [PATCH 4/8] make sure we hide error msg on success

---
 securedrop_client/logic.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index a1214c45d..2fe1dabc9 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -417,7 +417,7 @@ def on_sync_success(self) -> None:
             * Download new messages and replies
             * Update missing files so that they can be re-downloaded
         """
-        self.set_status('')  # remove any permanent error status message
+        self.gui.clear_error_status()  # remove any permanent error status message
 
         with open(self.sync_flag, 'w') as f:
             f.write(arrow.now().format())
@@ -470,6 +470,7 @@ def on_update_star_success(self, result) -> None:
         """
         After we star a source, we should sync the API such that the local database is updated.
         """
+        self.gui.clear_error_status()  # remove any permanent error status message
         self.sync_api()  # Syncing the API also updates the source list UI
 
     def on_update_star_failure(self, result: UpdateStarJobException) -> None:
@@ -550,6 +551,7 @@ def on_message_download_success(self, uuid: str) -> None:
         """
         Called when a message has downloaded.
         """
+        self.gui.clear_error_status()  # remove any permanent error status message
         message = storage.get_message(self.session, uuid)
         self.message_ready.emit(message.uuid, message.content)
 
@@ -573,6 +575,7 @@ def on_reply_download_success(self, uuid: str) -> None:
         """
         Called when a reply has downloaded.
         """
+        self.gui.clear_error_status()  # remove any permanent error status message
         reply = storage.get_reply(self.session, uuid)
         self.reply_ready.emit(reply.uuid, reply.content)
 
@@ -745,6 +748,7 @@ def on_delete_source_success(self, result) -> None:
         """
         Handler for when a source deletion succeeds.
         """
+        self.gui.clear_error_status()  # remove any permanent error status message
         self.sync_api()
 
     def on_delete_source_failure(self, result: Exception) -> None:
@@ -801,6 +805,7 @@ def send_reply(self, source_uuid: str, reply_uuid: str, message: str) -> None:
 
     def on_reply_success(self, reply_uuid: str) -> None:
         logger.debug('{} sent successfully'.format(reply_uuid))
+        self.gui.clear_error_status()  # remove any permanent error status message
         self.reply_succeeded.emit(reply_uuid)
         self.sync_api()
 
@@ -818,6 +823,7 @@ def get_file(self, file_uuid: str) -> db.File:
 
     def on_logout_success(self, result) -> None:
         logging.info('Client logout successful')
+        self.gui.clear_error_status()  # remove any permanent error status message
 
     def on_logout_failure(self, result: Exception) -> None:
         logging.info('Client logout failure')

From 033301be70f9a9e91bd3647e161f7e49fbc03fce Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Fri, 6 Dec 2019 16:43:21 -0800
Subject: [PATCH 5/8] adjust timeouts

---
 securedrop_client/api_jobs/downloads.py  | 11 +++++++++-
 securedrop_client/api_jobs/updatestar.py |  4 ++++
 securedrop_client/api_jobs/uploads.py    |  5 +++++
 securedrop_client/queue.py               |  9 --------
 tests/api_jobs/test_downloads.py         | 27 ++++++++++++++++++++----
 5 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py
index 71880d3c7..891ad6a47 100644
--- a/securedrop_client/api_jobs/downloads.py
+++ b/securedrop_client/api_jobs/downloads.py
@@ -50,6 +50,10 @@ def call_api(self, api_client: API, session: Session) -> Any:
         jobs.
         '''
 
+        # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will want to
+        # pass the default request timeout to download_reply instead of setting it on the api object
+        # directly.
+        api_client.default_request_timeout = 20
         remote_sources, remote_submissions, remote_replies = \
             get_remote_data(api_client)
 
@@ -87,7 +91,7 @@ def __init__(self, data_dir: str) -> None:
 
     def _get_realistic_timeout(self, size_in_bytes: int) -> int:
         '''
-        Return a realistic timeout in seconds for a file, message, or reply based on its size.
+        Return a realistic timeout in seconds based on the size of the download.
 
         This simply scales the timeouts per file so that it increases as the file size increases.
 
@@ -260,6 +264,11 @@ def call_download_api(self, api: API, db_object: Reply) -> Tuple[str, str]:
         '''
         sdk_object = SdkReply(uuid=db_object.uuid, filename=db_object.filename)
         sdk_object.source_uuid = db_object.source.uuid
+
+        # TODO: Once https://github.com/freedomofpress/securedrop-sdk/issues/108 is implemented, we
+        # will want to pass the default request timeout to download_reply instead of setting it on
+        # the api object directly.
+        api.default_request_timeout = 20
         return api.download_reply(sdk_object)
 
     def call_decrypt(self, filepath: str, session: Session = None) -> str:
diff --git a/securedrop_client/api_jobs/updatestar.py b/securedrop_client/api_jobs/updatestar.py
index 4d82fad88..15b1984d6 100644
--- a/securedrop_client/api_jobs/updatestar.py
+++ b/securedrop_client/api_jobs/updatestar.py
@@ -24,6 +24,10 @@ def call_api(self, api_client: API, session: Session) -> str:
         try:
             source_sdk_object = sdclientapi.Source(uuid=self.source_uuid)
 
+            # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will
+            # want to pass the default request timeout to download_reply instead of setting it on
+            # the api object directly.
+            api_client.default_request_timeout = 5
             if self.star_status:
                 api_client.remove_star(source_sdk_object)
             else:
diff --git a/securedrop_client/api_jobs/uploads.py b/securedrop_client/api_jobs/uploads.py
index 72f38ff42..d2f3db9d9 100644
--- a/securedrop_client/api_jobs/uploads.py
+++ b/securedrop_client/api_jobs/uploads.py
@@ -92,6 +92,11 @@ def call_api(self, api_client: API, session: Session) -> str:
 
     def _make_call(self, encrypted_reply: str, api_client: API) -> sdclientapi.Reply:
         sdk_source = sdclientapi.Source(uuid=self.source_uuid)
+
+        # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will want to
+        # pass the default request timeout to download_reply instead of setting it on the api object
+        # directly.
+        api_client.default_request_timeout = 5
         return api_client.reply_source(sdk_source, encrypted_reply, self.reply_uuid)
 
 
diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py
index aa595f8c9..c4755cc0b 100644
--- a/securedrop_client/queue.py
+++ b/securedrop_client/queue.py
@@ -158,15 +158,6 @@ def logout(self) -> None:
 
     def login(self, api_client: API) -> None:
         logger.debug('Passing API token to queues')
-
-        # Setting shorter default timeout so that user feedback is faster. For download jobs, this
-        # timeout is adjusted to be more realistic depending on file size.
-        #
-        # Currently ReplyDownloadJobs do not use adjusted timeouts because it is dependent on adding
-        # an optional timeout parameter to the download_reply securedrop-sdk method, see:
-        # https://github.com/freedomofpress/securedrop-sdk/issues/108
-        api_client.default_request_timeout = 5
-
         self.main_queue.api_client = api_client
         self.download_file_queue.api_client = api_client
         self.start_queues()
diff --git a/tests/api_jobs/test_downloads.py b/tests/api_jobs/test_downloads.py
index 4a462d6c0..07d0d965b 100644
--- a/tests/api_jobs/test_downloads.py
+++ b/tests/api_jobs/test_downloads.py
@@ -38,7 +38,9 @@ def test_MetadataSyncJob_success(mocker, homedir, session, session_maker):
         'securedrop_client.api_jobs.downloads.get_remote_data',
         return_value=([mock_source], 'submissions', 'replies'))
 
-    api_client = 'foo'
+    api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
 
     mocker.patch(
         'securedrop_client.api_jobs.downloads.update_local_storage',
@@ -73,7 +75,8 @@ def test_MetadataSyncJob_success_with_key_import_fail(mocker, homedir, session,
         'securedrop_client.api_jobs.downloads.get_remote_data',
         return_value=([mock_source], 'submissions', 'replies'))
 
-    api_client = 'foo'
+    api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
 
     mocker.patch(
         'securedrop_client.api_jobs.downloads.update_local_storage',
@@ -107,7 +110,8 @@ def test_MetadataSyncJob_success_with_missing_key(mocker, homedir, session, sess
         'securedrop_client.api_jobs.downloads.get_remote_data',
         return_value=([mock_source], 'submissions', 'replies'))
 
-    api_client = 'foo'
+    api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
 
     mocker.patch(
         'securedrop_client.api_jobs.downloads.update_local_storage',
@@ -149,6 +153,7 @@ def test_ReplyDownloadJob_no_download_or_decrypt(mocker, homedir, session, sessi
     mocker.patch.object(job_1.gpg, 'decrypt_submission_or_reply')
     mocker.patch.object(job_2.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     path = os.path.join(homedir, 'data')
     api_client.download_submission = mocker.MagicMock(return_value=('', path))
 
@@ -194,6 +199,7 @@ def test_ReplyDownloadJob_message_already_downloaded(mocker, homedir, session, s
     job = ReplyDownloadJob(reply.uuid, homedir, gpg)
     mocker.patch.object(job.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     download_fn = mocker.patch.object(api_client, 'download_reply')
 
     return_uuid = job.call_api(api_client, session)
@@ -216,6 +222,7 @@ def test_ReplyDownloadJob_happiest_path(mocker, homedir, session, session_maker)
     job = ReplyDownloadJob(reply.uuid, homedir, gpg)
     mocker.patch.object(job.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     data_dir = os.path.join(homedir, 'data')
     api_client.download_reply = mocker.MagicMock(return_value=('', data_dir))
 
@@ -244,6 +251,7 @@ def test_MessageDownloadJob_no_download_or_decrypt(mocker, homedir, session, ses
     mocker.patch.object(job_1.gpg, 'decrypt_submission_or_reply')
     mocker.patch.object(job_2.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     path = os.path.join(homedir, 'data')
     api_client.download_submission = mocker.MagicMock(return_value=('', path))
 
@@ -269,6 +277,7 @@ def test_MessageDownloadJob_message_already_decrypted(mocker, homedir, session,
     job = MessageDownloadJob(message.uuid, homedir, gpg)
     decrypt_fn = mocker.patch.object(job.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     download_fn = mocker.patch.object(api_client, 'download_submission')
 
     return_uuid = job.call_api(api_client, session)
@@ -289,6 +298,7 @@ def test_MessageDownloadJob_message_already_downloaded(mocker, homedir, session,
     job = MessageDownloadJob(message.uuid, homedir, gpg)
     mocker.patch.object(job.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     download_fn = mocker.patch.object(api_client, 'download_submission')
 
     return_uuid = job.call_api(api_client, session)
@@ -311,6 +321,7 @@ def test_MessageDownloadJob_happiest_path(mocker, homedir, session, session_make
     job = MessageDownloadJob(message.uuid, homedir, gpg)
     mocker.patch.object(job.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     data_dir = os.path.join(homedir, 'data')
     api_client.download_submission = mocker.MagicMock(return_value=('', data_dir))
 
@@ -332,6 +343,7 @@ def test_MessageDownloadJob_with_base_error(mocker, homedir, session, session_ma
     gpg = GpgHelper(homedir, session_maker, is_qubes=False)
     job = MessageDownloadJob(message.uuid, homedir, gpg)
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     mocker.patch.object(api_client, 'download_submission', side_effect=BaseError)
     decrypt_fn = mocker.patch.object(job.gpg, 'decrypt_submission_or_reply')
 
@@ -357,7 +369,7 @@ def test_MessageDownloadJob_with_crypto_error(mocker, homedir, session, session_
     job = MessageDownloadJob(message.uuid, homedir, gpg)
     mocker.patch.object(job.gpg, 'decrypt_submission_or_reply', side_effect=CryptoError)
     api_client = mocker.MagicMock()
-    api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     path = os.path.join(homedir, 'data')
     api_client.download_submission = mocker.MagicMock(return_value=('', path))
 
@@ -380,6 +392,7 @@ def test_FileDownloadJob_message_already_decrypted(mocker, homedir, session, ses
     job = FileDownloadJob(file_.uuid, homedir, gpg)
     decrypt_fn = mocker.patch.object(job.gpg, 'decrypt_submission_or_reply')
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     download_fn = mocker.patch.object(api_client, 'download_submission')
 
     return_uuid = job.call_api(api_client, session)
@@ -400,6 +413,7 @@ def test_FileDownloadJob_message_already_downloaded(mocker, homedir, session, se
     job = FileDownloadJob(file_.uuid, os.path.join(homedir, 'data'), gpg)
     patch_decrypt(mocker, homedir, gpg, file_.filename)
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     download_fn = mocker.patch.object(api_client, 'download_submission')
 
     return_uuid = job.call_api(api_client, session)
@@ -429,6 +443,7 @@ def fake_download(sdk_obj: SdkSubmission, timeout: int) -> Tuple[str, str]:
         return ('', full_path)
 
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     api_client.download_submission = fake_download
 
     job = FileDownloadJob(
@@ -471,6 +486,7 @@ def fake_download(sdk_obj: SdkSubmission, timeout: int) -> Tuple[str, str]:
                 full_path)
 
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     api_client.download_submission = fake_download
 
     job = FileDownloadJob(
@@ -506,6 +522,7 @@ def fake_download(sdk_obj: SdkSubmission, timeout: int) -> Tuple[str, str]:
                 full_path)
 
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     api_client.download_submission = fake_download
 
     job = FileDownloadJob(
@@ -538,6 +555,7 @@ def fake_download(sdk_obj: SdkSubmission, timeout: int) -> Tuple[str, str]:
                 full_path)
 
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     api_client.download_submission = fake_download
 
     job = FileDownloadJob(
@@ -581,6 +599,7 @@ def fake_download(sdk_obj: SdkSubmission, timeout: int) -> Tuple[str, str]:
                 full_path)
 
     api_client = mocker.MagicMock()
+    api_client.default_request_timeout = mocker.MagicMock()
     api_client.download_submission = fake_download
 
     job = FileDownloadJob(

From 2326f4a4d6daf3f2211fed35d7fa65145bc83e2a Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Mon, 9 Dec 2019 11:42:29 -0800
Subject: [PATCH 6/8] caller logs and handles exception

---
 securedrop_client/storage.py | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py
index 494871c76..b44db9592 100644
--- a/securedrop_client/storage.py
+++ b/securedrop_client/storage.py
@@ -77,15 +77,10 @@ def get_remote_data(api: API) -> Tuple[List[SDKSource], List[SDKSubmission], Lis
     (remote_sources, remote_submissions, remote_replies)
     """
     remote_submissions = []  # type: List[SDKSubmission]
-    try:
-        remote_sources = api.get_sources()
-        for source in remote_sources:
-            remote_submissions.extend(api.get_submissions(source))
-        remote_replies = api.get_all_replies()
-    except Exception as ex:
-        # Log any errors but allow the caller to handle the exception.
-        logger.error(ex)
-        raise(ex)
+    remote_sources = api.get_sources()
+    for source in remote_sources:
+        remote_submissions.extend(api.get_submissions(source))
+    remote_replies = api.get_all_replies()
 
     logger.info('Fetched {} remote sources.'.format(len(remote_sources)))
     logger.info('Fetched {} remote submissions.'.format(

From 18c88982c40ffc20e6bbd8b084ee241e51aa20de Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Mon, 9 Dec 2019 14:43:28 -0800
Subject: [PATCH 7/8] update comments

---
 securedrop_client/api_jobs/downloads.py  | 2 +-
 securedrop_client/api_jobs/updatestar.py | 4 ++--
 securedrop_client/api_jobs/uploads.py    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py
index 891ad6a47..cca93dfae 100644
--- a/securedrop_client/api_jobs/downloads.py
+++ b/securedrop_client/api_jobs/downloads.py
@@ -51,7 +51,7 @@ def call_api(self, api_client: API, session: Session) -> Any:
         '''
 
         # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will want to
-        # pass the default request timeout to download_reply instead of setting it on the api object
+        # pass the default request timeout to api calls instead of setting it on the api object
         # directly.
         api_client.default_request_timeout = 20
         remote_sources, remote_submissions, remote_replies = \
diff --git a/securedrop_client/api_jobs/updatestar.py b/securedrop_client/api_jobs/updatestar.py
index 15b1984d6..ee82eb6ff 100644
--- a/securedrop_client/api_jobs/updatestar.py
+++ b/securedrop_client/api_jobs/updatestar.py
@@ -25,8 +25,8 @@ def call_api(self, api_client: API, session: Session) -> str:
             source_sdk_object = sdclientapi.Source(uuid=self.source_uuid)
 
             # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will
-            # want to pass the default request timeout to download_reply instead of setting it on
-            # the api object directly.
+            # want to pass the default request timeout to remove_star and add_star instead of
+            # setting it on the api object directly.
             api_client.default_request_timeout = 5
             if self.star_status:
                 api_client.remove_star(source_sdk_object)
diff --git a/securedrop_client/api_jobs/uploads.py b/securedrop_client/api_jobs/uploads.py
index d2f3db9d9..89116aa74 100644
--- a/securedrop_client/api_jobs/uploads.py
+++ b/securedrop_client/api_jobs/uploads.py
@@ -94,7 +94,7 @@ def _make_call(self, encrypted_reply: str, api_client: API) -> sdclientapi.Reply
         sdk_source = sdclientapi.Source(uuid=self.source_uuid)
 
         # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will want to
-        # pass the default request timeout to download_reply instead of setting it on the api object
+        # pass the default request timeout to reply_source instead of setting it on the api object
         # directly.
         api_client.default_request_timeout = 5
         return api_client.reply_source(sdk_source, encrypted_reply, self.reply_uuid)

From c439af1a59d166e71d0d05b731af828b5c273502 Mon Sep 17 00:00:00 2001
From: Allie Crevier <allie@freedom.press>
Date: Mon, 9 Dec 2019 15:05:12 -0800
Subject: [PATCH 8/8] clear error status upon file download success

---
 securedrop_client/logic.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py
index 2fe1dabc9..bd4f986ec 100644
--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -729,6 +729,7 @@ def on_file_download_success(self, result: Any) -> None:
         """
         Called when a file has downloaded.
         """
+        self.gui.clear_error_status()  # remove any permanent error status message
         self.file_ready.emit(result)
 
     def on_file_download_failure(self, exception: Exception) -> None: