diff --git a/securedrop_client/api_jobs/base.py b/securedrop_client/api_jobs/base.py index 0271c0402..7935cc005 100644 --- a/securedrop_client/api_jobs/base.py +++ b/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 diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index a80c17b9d..097d4c813 100644 --- a/securedrop_client/logic.py +++ b/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: """ diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py index bc5375b31..72d44f2d7 100644 --- a/securedrop_client/queue.py +++ b/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) diff --git a/tests/test_logic.py b/tests/test_logic.py index dc7033046..4806c48c4 100644 --- a/tests/test_logic.py +++ b/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. diff --git a/tests/test_queue.py b/tests/test_queue.py index f73fe02a5..af17efb08 100644 --- a/tests/test_queue.py +++ b/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,6 +94,34 @@ 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() @@ -100,6 +129,7 @@ def test_ApiJobQueue_enqueue(mocker): 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