queue: rework login/logout

* security bugfix: deauth queue when user logs out (#397)

* make sure queues are started when we enqueue a new job (#380)

* also ensure that we can, in one run of the client:

1. Log in, be authed to make network calls
2. Log out, not be authed to make network calls
3. Log _back_ in, once again be authed to make network calls

* show "user must login" message when download clicked if offline

securedrop_client/api_jobs/base.py

@@ -41,6 +41,9 @@ def _do_call_api(self, api_client: API, session: Session) -> None:
             result = self.call_api(api_client, session)
         except AuthError as e:
             raise ApiInaccessibleError() from e
+        except ApiInaccessibleError as e:
+            # Do not let ApiInaccessibleError emit the regular failure signal, raise now
+            raise ApiInaccessibleError() from e
         except RequestTimeoutError:
             logger.debug('Job {} timed out'.format(self))
             raise

securedrop_client/logic.py

@@ -301,13 +301,14 @@ def on_authenticate_success(self, result):
         """
         Handles a successful authentication call against the API.
         """
+        logger.info('{} successfully logged in'.format(self.api.username))
         self.gui.hide_login()
         self.sync_api()
         self.gui.show_main_window(self.api.username)
 
         self.start_reply_thread()
 
-        self.api_job_queue.start_queues(self.api)
+        self.api_job_queue.login(self.api)
 
         # Clear the sidebar error status bar if a message was shown
         # to the user indicating they should log in.
@@ -476,6 +477,7 @@ def logout(self):
         """
         self.api = None
         self.reply_sync.api = None
+        self.api_job_queue.logout()
         self.gui.logout()
         self.is_authenticated = False
 
@@ -543,17 +545,20 @@ def on_submission_download(
         """
         Download the file associated with the Submission (which may be a File or Message).
         """
-        job = FileDownloadJob(
-            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)
+        if self.api:
+            job = FileDownloadJob(
+                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)
 
-        self.api_job_queue.enqueue(job)
-        self.set_status(_('Downloading file'))
+            self.api_job_queue.enqueue(job)
+            self.set_status(_('Downloading file'))
+        else:
+            self.on_action_requiring_login()
 
     def on_file_download_success(self, result: Any) -> None:
         """

securedrop_client/queue.py

@@ -7,7 +7,7 @@
 from sqlalchemy.orm import scoped_session
 from typing import Optional  # noqa: F401
 
-from securedrop_client.api_jobs.base import ApiJob
+from securedrop_client.api_jobs.base import ApiJob, ApiInaccessibleError
 from securedrop_client.api_jobs.downloads import FileDownloadJob
 
 
@@ -28,8 +28,8 @@ def process(self) -> None:  # pragma: nocover
         self._process(False)
 
     def _process(self, exit_loop: bool) -> None:
-        session = self.session_maker()
         while True:
+            session = self.session_maker()
             # retry the "cached" job if it exists, otherwise get the next job
             if self.last_job is not None:
                 job = self.last_job
@@ -41,10 +41,15 @@ def _process(self, exit_loop: bool) -> None:
                 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
+            except ApiInaccessibleError:
+                # This is a guard against #397, we should pause the queue execution when this
+                # happens in the future and flag the situation to the user (see ticket #379).
+                logger.error('Client is not authenticated, skipping job...')
+            finally:
+                # process events to allow this thread to handle incoming signals
+                QApplication.processEvents()
 
-            # process events to allow this thread to handle incoming signals
-            QApplication.processEvents()
+                session.close()
 
             if exit_loop:
                 return
@@ -68,15 +73,32 @@ def __init__(self, api_client: API, session_maker: scoped_session) -> None:
         self.main_thread.started.connect(self.main_queue.process)
         self.download_file_thread.started.connect(self.download_file_queue.process)
 
-    def start_queues(self, api_client: API) -> None:
+    def logout(self) -> None:
+        self.main_queue.api_client = None
+        self.download_file_queue.api_client = None
+
+    def login(self, api_client: API) -> None:
+        logger.debug('Passing API token to queues')
         self.main_queue.api_client = api_client
         self.download_file_queue.api_client = api_client
+        self.start_queues()
 
-        self.main_thread.start()
-        self.download_file_thread.start()
+    def start_queues(self) -> None:
+        if not self.main_thread.isRunning():
+            logger.debug('Starting main thread')
+            self.main_thread.start()
+
+        if not self.download_file_thread.isRunning():
+            logger.debug('Starting download thread')
+            self.download_file_thread.start()
 
     def enqueue(self, job: ApiJob) -> None:
+        # First check the queues are started in case they died for some reason.
+        self.start_queues()
+
         if isinstance(job, FileDownloadJob):
+            logger.debug('Adding job to download queue')
             self.download_file_queue.queue.put_nowait(job)
         else:
+            logger.debug('Adding job to main queue')
             self.main_queue.queue.put_nowait(job)

tests/test_logic.py

@@ -721,11 +721,14 @@ def test_Controller_logout(homedir, config, mocker, session_maker):
     mock_gui = mocker.MagicMock()
     co = Controller('http://localhost', mock_gui, session_maker, homedir)
     co.api = mocker.MagicMock()
+    co.api_job_queue = mocker.MagicMock()
+    co.api_job_queue.logout = mocker.MagicMock()
     co.reply_sync = mocker.MagicMock()
     co.reply_sync.api = mocker.MagicMock()
     co.logout()
     assert co.api is None
     assert co.reply_sync.api is None
+    co.api_job_queue.logout.assert_called_once_with()
     co.gui.logout.assert_called_once_with()
 
 
@@ -802,6 +805,7 @@ def test_Controller_on_file_download_Submission(homedir, config, session, mocker
     """
     mock_gui = mocker.MagicMock()
     co = Controller('http://localhost', mock_gui, session_maker, homedir)
+    co.api = 'this has a value'
 
     mock_success_signal = mocker.MagicMock()
     mock_failure_signal = mocker.MagicMock()
@@ -832,6 +836,35 @@ def test_Controller_on_file_download_Submission(homedir, config, session, mocker
         co.on_file_download_failure, type=Qt.QueuedConnection)
 
 
+def test_Controller_on_file_download_Submission_no_auth(homedir, config, session,
+                                                        mocker, session_maker):
+    """If the controller is not authenticated, do not enqueue a download job"""
+    mock_gui = mocker.MagicMock()
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+    co.api = None
+
+    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.FileDownloadJob", return_value=mock_job)
+    mock_queue = mocker.patch.object(co, 'api_job_queue')
+
+    source = factory.Source()
+    file_ = factory.File(is_downloaded=None, is_decrypted=None, source=source)
+    session.add(source)
+    session.add(file_)
+    session.commit()
+
+    co.on_submission_download(db.File, file_.uuid)
+
+    assert not mock_job_cls.called
+    assert not mock_queue.enqueue.called
+    assert not mock_success_signal.connect.called
+    assert not mock_failure_signal.connect.called
+
+
 def test_Controller_on_file_downloaded_success(homedir, config, mocker, session_maker):
     '''
     Using the `config` fixture to ensure the config is written to disk.

tests/test_queue.py

@@ -6,6 +6,7 @@
 
 from securedrop_client import db
 from securedrop_client.api_jobs.downloads import FileDownloadJob
+from securedrop_client.api_jobs.base import ApiInaccessibleError
 from securedrop_client.queue import RunnableQueue, ApiJobQueue
 from tests import factory
 
@@ -93,13 +94,42 @@ def test_RunnableQueue_job_timeout(mocker):
     assert mock_process_events.called
 
 
+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.
+    '''
+    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 = ApiInaccessibleError()
+    dummy_job_cls = factory.dummy_job_factory(mocker, return_value)
+    job = dummy_job_cls()
+
+    queue = RunnableQueue(mock_api_client, mock_session_maker)
+    queue.queue.put_nowait(job)
+
+    mock_logger = mocker.patch('securedrop_client.queue.logger')
+
+    # attempt to process job
+    queue._process(exit_loop=True)
+
+    # assert we logged an error message
+    assert "Client is not authenticated" in mock_logger.error.call_args[0][0]
+
+    assert mock_process_events.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_file_queue = mocker.patch.object(job_queue, 'download_file_queue')
     mock_main_queue = mocker.patch.object(job_queue, 'main_queue')
+    mock_start_queues = mocker.patch.object(job_queue, 'start_queues')
 
     dl_job = FileDownloadJob(db.File, 'mock', 'mock', 'mock')
     job_queue.enqueue(dl_job)
@@ -116,9 +146,10 @@ def test_ApiJobQueue_enqueue(mocker):
 
     mock_main_queue.queue.put_nowait.assert_called_once_with(dummy_job)
     assert not mock_download_file_queue.queue.put_nowait.called
+    assert mock_start_queues.called
 
 
-def test_ApiJobQueue_start_queues(mocker):
+def test_ApiJobQueue_login_if_queues_not_running(mocker):
     mock_api = mocker.MagicMock()
     mock_client = mocker.MagicMock()
     mock_session_maker = mocker.MagicMock()
@@ -129,11 +160,50 @@ def test_ApiJobQueue_start_queues(mocker):
     mock_download_file_queue = mocker.patch.object(job_queue, 'download_file_queue')
     mock_main_thread = mocker.patch.object(job_queue, 'main_thread')
     mock_download_file_thread = mocker.patch.object(job_queue, 'download_file_thread')
+    job_queue.main_thread.isRunning = mocker.MagicMock(return_value=False)
+    job_queue.download_file_thread.isRunning = mocker.MagicMock(return_value=False)
 
-    job_queue.start_queues(mock_api)
+    job_queue.login(mock_api)
 
     assert mock_main_queue.api_client == mock_api
     assert mock_download_file_queue.api_client == mock_api
 
     mock_main_thread.start.assert_called_once_with()
     mock_download_file_thread.start.assert_called_once_with()
+
+
+def test_ApiJobQueue_login_if_queues_running(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_file_queue = mocker.patch.object(job_queue, 'download_file_queue')
+    mock_main_thread = mocker.patch.object(job_queue, 'main_thread')
+    mock_download_file_thread = mocker.patch.object(job_queue, 'download_file_thread')
+    job_queue.main_thread.isRunning = mocker.MagicMock(return_value=True)
+    job_queue.download_file_thread.isRunning = mocker.MagicMock(return_value=True)
+
+    job_queue.login(mock_api)
+
+    assert mock_main_queue.api_client == mock_api
+    assert mock_download_file_queue.api_client == mock_api
+
+    assert not mock_main_thread.start.called
+    assert not mock_download_file_thread.start.called
+
+
+def test_ApiJobQueue_logout_removes_api_client(mocker):
+    mock_client = mocker.MagicMock()
+    mock_session_maker = mocker.MagicMock()
+
+    job_queue = ApiJobQueue(mock_client, mock_session_maker)
+    job_queue.main_queue.api_client = 'my token!!!'
+    job_queue.download_file_queue.api_client = 'my token!!!'
+
+    job_queue.logout()
+
+    assert job_queue.main_queue.api_client is None
+    assert job_queue.download_file_queue.api_client is None