force logout when token invalid and stop queues on logout

securedrop_client/gui/main.py

@@ -110,7 +110,7 @@ def autosize_window(self):
         screen = QDesktopWidget().screenGeometry()
         self.resize(screen.width(), screen.height())
 
-    def show_login(self):
+    def show_login(self, error: str = ''):
         """
         Show the login form.
         """
@@ -122,9 +122,10 @@ def show_login(self):
         x_center = (screen_size.width() - login_dialog_size.width()) / 2
         y_center = (screen_size.height() - login_dialog_size.height()) / 2
         self.login_dialog.move(x_center, y_center)
-
         self.login_dialog.setup(self.controller)
         self.login_dialog.reset()
+        if error:
+            self.login_dialog.error(error)
         self.login_dialog.exec()
 
     def show_login_error(self, error):

securedrop_client/logic.py

@@ -32,6 +32,7 @@
 
 from securedrop_client import storage
 from securedrop_client import db
+from securedrop_client.api_jobs.base import ApiInaccessibleError
 from securedrop_client.api_jobs.downloads import FileDownloadJob, MessageDownloadJob, \
     ReplyDownloadJob, DownloadChecksumMismatchException, MetadataSyncJob
 from securedrop_client.api_jobs.sources import DeleteSourceJob
@@ -275,13 +276,23 @@ def call_api(self,
         new_api_thread.start()
 
     def on_queue_paused(self) -> None:
-        if self.api is None:
-            self.gui.update_error_status(_('The SecureDrop server cannot be reached.'))
-        else:
-            self.gui.update_error_status(
-                _('The SecureDrop server cannot be reached.'),
-                duration=0,
-                retry=True)
+        # TODO: remove if block once https://github.com/freedomofpress/securedrop-client/pull/739
+        # is merged and rely on continuous metadata sync to encounter same auth error from the
+        # server which will log the user out in the on_sync_failure handler
+        if (
+            not self.api or
+            not self.api_job_queue.main_queue.api_client or
+            not self.api_job_queue.download_file_queue.api_client or
+            not self.api_job_queue.metadata_queue.api_client
+        ):
+            self.logout()
+            self.gui.show_login(error=_('Your session expired. Please log in again.'))
+            return
+
+        self.gui.update_error_status(
+            _('The SecureDrop server cannot be reached.'),
+            duration=0,
+            retry=True)
 
     def resume_queues(self) -> None:
         self.api_job_queue.resume_queues()
@@ -333,6 +344,7 @@ def on_authenticate_success(self, result):
             self.session)
         self.gui.show_main_window(user)
         self.api_job_queue.login(self.api)
+        self.update_sources()
         self.sync_api()
         self.is_authenticated = True
         self.resume_queues()
@@ -419,14 +431,15 @@ def on_sync_success(self) -> None:
 
     def on_sync_failure(self, result: Exception) -> None:
         """
-        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.
+        Called when syncronisation of data via the API fails after a background sync. If the reason
+        a sync fails is ApiInaccessibleError then we need to log the user out for security reasons
+        and show them the login window in order to get a new token.
         """
         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)
+
+        if isinstance(result, ApiInaccessibleError):
+            self.logout()
+            self.gui.show_login(error=_('Your session expired. Please log in again.'))
 
     def update_sync(self):
         """
@@ -735,6 +748,7 @@ def on_delete_source_success(self, result) -> None:
 
     def on_delete_source_failure(self, result: Exception) -> None:
         logging.info("failed to delete source at server")
+
         error = _('Failed to delete source at server')
         self.gui.update_error_status(error)
 

securedrop_client/queue.py

@@ -136,7 +136,11 @@ def process(self) -> None:
                 session = self.session_maker()
                 job._do_call_api(self.api_client, session)
                 self.pinged.emit()
-            except (RequestTimeoutError, ApiInaccessibleError) as e:
+            except ApiInaccessibleError as e:
+                logger.debug('Job {} raised an exception: {}: {}'.format(self, type(e).__name__, e))
+                self.api_client = None
+                self.add_job(PauseQueueJob())
+            except RequestTimeoutError as e:
                 logger.debug('Job {} raised an exception: {}: {}'.format(self, type(e).__name__, e))
                 self.add_job(PauseQueueJob())
                 self.re_add_job(job)
@@ -183,6 +187,18 @@ def logout(self) -> None:
         self.download_file_queue.api_client = None
         self.metadata_queue.api_client = None
 
+        if self.main_thread.isRunning():
+            logger.debug('Stopping main queue thread')
+            self.main_thread.quit()
+
+        if self.download_file_thread.isRunning():
+            logger.debug('Stopping download queue thread')
+            self.download_file_thread.quit()
+
+        if self.metadata_thread.isRunning():
+            logger.debug('Stopping metadata queue thread')
+            self.metadata_thread.quit()
+
     def login(self, api_client: API) -> None:
         logger.debug('Passing API token to queues')
         self.main_queue.api_client = api_client

tests/gui/test_main.py

@@ -112,6 +112,22 @@ def test_show_login(mocker):
     w.login_dialog.exec.assert_called_once_with()
 
 
+def test_show_login_with_error_message(mocker):
+    """
+    The login dialog is displayed with a clean state.
+    """
+    w = Window()
+    w.controller = mocker.MagicMock()
+    mock_ld = mocker.patch('securedrop_client.gui.main.LoginDialog')
+
+    w.show_login('this-is-an-error-message-to-show-on-login-window')
+
+    mock_ld.assert_called_once_with(w)
+    w.login_dialog.reset.assert_called_once_with()
+    w.login_dialog.exec.assert_called_once_with()
+    w.login_dialog.error.assert_called_once_with('this-is-an-error-message-to-show-on-login-window')
+
+
 def test_show_login_error(mocker):
     """
     Ensures that an error message is displayed in the login dialog.

tests/test_logic.py

@@ -12,6 +12,7 @@
 
 from securedrop_client import db
 from securedrop_client.logic import APICallRunner, Controller
+from securedrop_client.api_jobs.base import ApiInaccessibleError
 from securedrop_client.api_jobs.downloads import DownloadChecksumMismatchException
 from securedrop_client.api_jobs.uploads import SendReplyJobError
 
@@ -424,6 +425,25 @@ def test_Controller_on_sync_failure(homedir, config, mocker, session_maker):
     assert mock_storage.update_local_storage.call_count == 0
 
 
+def test_Controller_on_sync_failure_due_to_invalid_token(homedir, config, mocker, session_maker):
+    """
+    If the sync fails because the api is inaccessible then ensure user is logged out and shown the
+    login window.
+    """
+    gui = mocker.MagicMock()
+    co = Controller('http://localhost', gui, session_maker, homedir)
+    co.logout = mocker.MagicMock()
+    co.gui = mocker.MagicMock()
+    co.gui.show_login = mocker.MagicMock()
+    mock_storage = mocker.patch('securedrop_client.logic.storage')
+
+    co.on_sync_failure(ApiInaccessibleError())
+
+    assert mock_storage.update_local_storage.call_count == 0
+    co.logout.assert_called_once_with()
+    co.gui.show_login.assert_called_once_with(error='Your session expired. Please log in again.')
+
+
 def test_Controller_on_sync_success(homedir, config, mocker):
     """
     If there's a result to syncing, then update local storage.
@@ -1444,22 +1464,29 @@ def test_Controller_on_queue_paused(homedir, config, mocker, session_maker):
     '''
     mock_gui = mocker.MagicMock()
     co = Controller('http://localhost', mock_gui, session_maker, homedir)
-    co.api = 'mock'
+    mocker.patch.object(co, 'api_job_queue')
+    co.api = 'not none'
     co.on_queue_paused()
     mock_gui.update_error_status.assert_called_once_with(
         'The SecureDrop server cannot be reached.', duration=0, retry=True)
 
 
-def test_Controller_on_queue_paused_when_logged_out(homedir, config, mocker, session_maker):
-    '''
-    Check that a paused queue is communicated to the user via the error status bar. There should not
-    be a retry option displayed to the user
-    '''
-    mock_gui = mocker.MagicMock()
-    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+def test_Controller_on_queue_paused_due_to_invalid_token(homedir, config, mocker, session_maker):
+    """
+    If the api is inaccessible then ensure user is logged out and shown the login window. Also check
+    that "SecureDrop server cannot be reached" is not shown when the user is not authenticated.
+    """
+    gui = mocker.MagicMock()
+    co = Controller('http://localhost', gui, session_maker, homedir)
     co.api = None
+    co.logout = mocker.MagicMock()
+    co.gui = mocker.MagicMock()
+    co.gui.show_login = mocker.MagicMock()
+
     co.on_queue_paused()
-    mock_gui.update_error_status.assert_called_once_with('The SecureDrop server cannot be reached.')
+
+    co.logout.assert_called_once_with()
+    co.gui.show_login.assert_called_once_with(error='Your session expired. Please log in again.')
 
 
 def test_Controller_call_update_star_success(homedir, config, mocker, session_maker, session):

tests/test_queue.py

@@ -176,38 +176,6 @@ def test_RunnableQueue_resubmitted_jobs(mocker):
     queue.re_add_job(job1)
 
 
-def test_RunnableQueue_job_ApiInaccessibleError(mocker):
-    '''
-    Add two jobs to the queue. The first runs into an auth error, and then gets resubmitted for the
-    next pass through the loop.
-    '''
-    queue = RunnableQueue(mocker.MagicMock(), mocker.MagicMock())
-    queue.pause = mocker.MagicMock()
-    job_cls = factory.dummy_job_factory(mocker, ApiInaccessibleError())
-    queue.JOB_PRIORITIES = {PauseQueueJob: 0, job_cls: 1}
-
-    # ApiInaccessibleError will cause the queue to pause, use our fake pause method instead
-    def fake_pause() -> None:
-        queue.add_job(PauseQueueJob())
-    queue.pause.emit = fake_pause
-
-    # Add two jobs that timeout during processing to the queues
-    job1 = job_cls()
-    job2 = job_cls()
-    queue.add_job(job1)
-    queue.add_job(job2)
-
-    # attempt to process job1 knowing that it times out
-    queue.process()
-    assert queue.queue.qsize() == 2  # queue contains: job1, job2
-
-    # now process after making it so job1 no longer times out
-    job1.return_value = 'mock'
-    queue.process()
-    assert queue.queue.qsize() == 1  # queue contains: job2
-    assert queue.queue.get(block=True) == (1, job2)
-
-
 def test_RunnableQueue_job_generic_exception(mocker):
     '''
     Add two jobs to the queue, the first of which will cause a generic exception, which is handled
@@ -233,25 +201,24 @@ def test_RunnableQueue_job_generic_exception(mocker):
 
 def test_RunnableQueue_does_not_run_jobs_when_not_authed(mocker):
     '''
-    Add a job to the queue, ensure we don't run it when not authenticated.
+    Check that the queue is paused when a job returns with aApiInaccessibleError. Check that the
+    job does not get resubmitted since it is not authorized and that its api_client is None.
     '''
     queue = RunnableQueue(mocker.MagicMock(), mocker.MagicMock())
-    queue.pause = mocker.MagicMock()
+    queue.paused = mocker.MagicMock()
+    queue.paused.emit = mocker.MagicMock()
     job_cls = factory.dummy_job_factory(mocker, ApiInaccessibleError())
     queue.JOB_PRIORITIES = {PauseQueueJob: 0, job_cls: 1}
 
-    # ApiInaccessibleError will cause the queue to pause, use our fake pause method instead
-    def fake_pause() -> None:
-        queue.add_job(PauseQueueJob())
-    queue.pause.emit = fake_pause
-
     # Add a job that results in an ApiInaccessibleError to the queue
     job = job_cls()
     queue.add_job(job)
 
-    # attempt to process job1 knowing that it times out
+    # attempt to process job knowing that it errors
     queue.process()
-    assert queue.queue.qsize() == 1  # queue contains: job1
+    assert queue.queue.qsize() == 0  # queue should not contain job since it was not resubmitted
+    assert queue.api_client is None
+    queue.paused.emit.assert_called_once_with()
 
 
 def test_ApiJobQueue_enqueue(mocker):
@@ -433,3 +400,26 @@ def test_ApiJobQueue_logout_removes_api_client(mocker):
     assert job_queue.main_queue.api_client is None
     assert job_queue.download_file_queue.api_client is None
     assert job_queue.metadata_queue.api_client is None
+
+
+def test_ApiJobQueue_logout_stops_queue_threads(mocker):
+    job_queue = ApiJobQueue(mocker.MagicMock(), mocker.MagicMock())
+
+    job_queue.logout()
+
+    assert not job_queue.main_thread.isRunning()
+    assert not job_queue.download_file_thread.isRunning()
+    assert not job_queue.metadata_thread.isRunning()
+
+
+def test_ApiJobQueue_logout_results_in_queue_threads_not_running(mocker):
+    job_queue = ApiJobQueue(mocker.MagicMock(), mocker.MagicMock())
+    job_queue.main_thread = mocker.MagicMock()
+    job_queue.download_file_thread = mocker.MagicMock()
+    job_queue.metadata_thread = mocker.MagicMock()
+
+    job_queue.logout()
+
+    job_queue.main_thread.quit.assert_called_once_with()
+    job_queue.download_file_thread.quit.assert_called_once_with()
+    job_queue.metadata_thread.quit.assert_called_once_with()