Merge pull request #750 from freedomofpress/when-token-invalid-user-m…

…ust-log-back-in

When token invalid user must log back in

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
@@ -279,13 +280,24 @@ 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.invalidate_token()
+            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()
@@ -344,9 +356,8 @@ def on_authenticate_success(self, result):
 
     def on_authenticate_failure(self, result: Exception) -> None:
         # Failed to authenticate. Reset state with failure message.
-        self.api = None
-        error = _('There was a problem signing in. '
-                  'Please verify your credentials and try again.')
+        self.invalidate_token()
+        error = _('There was a problem signing in. Please verify your credentials and try again.')
         self.gui.show_login_error(error=error)
 
     def login_offline_mode(self):
@@ -424,14 +435,16 @@ 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.invalidate_token()
+            self.logout()
+            self.gui.show_login(error=_('Your session expired. Please log in again.'))
 
     def update_sync(self):
         """
@@ -481,18 +494,26 @@ def update_star(self, source_db_object, callback):
 
     def logout(self):
         """
-        Call logout function in the API, reset the API object, and force the UI
-        to update into a logged out state.
+        If the token is not already invalid, make an api call to logout and invalidate the token.
+        Then mark all pending draft replies as failed, stop the queues, and show the user as logged
+        out in the GUI.
         """
-        self.call_api(self.api.logout,
-                      self.on_logout_success,
-                      self.on_logout_failure)
-        self.api = None
+        if self.api is not None:
+            self.call_api(self.api.logout, self.on_logout_success, self.on_logout_failure)
+            self.invalidate_token()
+
+        failed_replies = storage.mark_all_pending_drafts_as_failed(self.session)
+        for failed_reply in failed_replies:
+            self.reply_failed.emit(failed_reply.uuid)
+
         self.api_job_queue.logout()
-        storage.mark_all_pending_drafts_as_failed(self.session)
         self.gui.logout()
+
         self.is_authenticated = False
 
+    def invalidate_token(self):
+        self.api = None
+
     def set_status(self, message, duration=5000):
         """
         Set a textual status message to be displayed to the user for a certain
@@ -507,7 +528,7 @@ def _submit_download_job(self,
         if object_type == db.Reply:
             job = ReplyDownloadJob(
                 uuid, self.data_dir, self.gpg
-                )  # type: Union[ReplyDownloadJob, MessageDownloadJob, FileDownloadJob]
+            )  # type: Union[ReplyDownloadJob, MessageDownloadJob, FileDownloadJob]
             job.success_signal.connect(self.on_reply_download_success, type=Qt.QueuedConnection)
             job.failure_signal.connect(self.on_reply_download_failure, type=Qt.QueuedConnection)
         elif object_type == db.Message:
@@ -697,6 +718,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)
@@ -179,9 +183,17 @@ def __init__(self, api_client: API, session_maker: scoped_session) -> None:
         self.metadata_queue.pinged.connect(self.resume_queues)
 
     def logout(self) -> None:
-        self.main_queue.api_client = 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')

securedrop_client/storage.py

@@ -529,20 +529,19 @@ def get_reply(session: Session, uuid: str) -> Reply:
     return session.query(Reply).filter_by(uuid=uuid).one()
 
 
-def mark_all_pending_drafts_as_failed(session: Session) -> None:
+def mark_all_pending_drafts_as_failed(session: Session) -> List[DraftReply]:
     """
-    When we login (offline or online) or logout, we need to set all
-    the pending replies as failed.
+    When we login (offline or online) or logout, we need to set all the pending replies as failed.
     """
     pending_status = session.query(ReplySendStatus).filter_by(
         name=ReplySendStatusCodes.PENDING.value).one()
     failed_status = session.query(ReplySendStatus).filter_by(
         name=ReplySendStatusCodes.FAILED.value).one()
 
-    pending_drafts = session.query(DraftReply).filter_by(
-        send_status=pending_status
-    ).all()
+    pending_drafts = session.query(DraftReply).filter_by(send_status=pending_status).all()
     for pending_draft in pending_drafts:
         pending_draft.send_status = failed_status
 
     session.commit()
+
+    return pending_drafts

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
 
@@ -426,6 +427,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.
@@ -533,6 +553,68 @@ def test_Controller_on_update_star_failed(homedir, config, mocker, session_maker
     mock_gui.update_error_status.assert_called_once_with('Failed to update star.')
 
 
+def test_Controller_invalidate_token(mocker, homedir, session_maker):
+    '''
+    Ensure the controller's api token is set to None.
+    '''
+    co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir)
+    co.api = 'not None'
+
+    co.invalidate_token()
+
+    assert co.api is None
+
+
+def test_Controller_logout_with_pending_replies(mocker, session_maker, homedir, reply_status_codes):
+    '''
+    Ensure draft reply fails on logout and that the reply_failed signal is emitted.
+    '''
+    co = Controller('http://localhost', mocker.MagicMock(), session_maker, homedir)
+    co.api_job_queue = mocker.MagicMock()
+    co.api_job_queue.logout = mocker.MagicMock()
+    co.call_api = mocker.MagicMock()
+    co.reply_failed = mocker.MagicMock()
+
+    source = factory.Source()
+    session = session_maker()
+    pending_status = session.query(db.ReplySendStatus).filter_by(
+        name=db.ReplySendStatusCodes.PENDING.value).one()
+    failed_status = session.query(db.ReplySendStatus).filter_by(
+        name=db.ReplySendStatusCodes.FAILED.value).one()
+    pending_draft_reply = factory.DraftReply(source=source, send_status=pending_status)
+    session.add(source)
+    session.add(pending_draft_reply)
+
+    co.logout()
+
+    for draft in session.query(db.DraftReply).all():
+        assert draft.send_status == failed_status
+
+    co.reply_failed.emit.assert_called_once_with(pending_draft_reply.uuid)
+
+
+def test_Controller_logout_with_no_api(homedir, config, mocker, session_maker):
+    '''
+    Ensure we don't attempt to make an api call to logout when the api has been set to None
+    because token is invalid.
+    '''
+    mock_gui = mocker.MagicMock()
+    co = Controller('http://localhost', mock_gui, session_maker, homedir)
+    co.api = None
+    co.api_job_queue = mocker.MagicMock()
+    co.api_job_queue.logout = mocker.MagicMock()
+    co.call_api = mocker.MagicMock()
+    fail_draft_replies = mocker.patch(
+        'securedrop_client.storage.mark_all_pending_drafts_as_failed')
+
+    co.logout()
+
+    co.call_api.assert_not_called()
+    co.api_job_queue.logout.assert_called_once_with()
+    co.gui.logout.assert_called_once_with()
+    fail_draft_replies.called_once_with(co.session)
+
+
 def test_Controller_logout_success(homedir, config, mocker, session_maker):
     """
     Ensure the API is called on logout and if the API call succeeds,
@@ -1359,22 +1441,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):