diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py index 71880d3c7..cca93dfae 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 api calls 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..ee82eb6ff 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 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) else: diff --git a/securedrop_client/api_jobs/uploads.py b/securedrop_client/api_jobs/uploads.py index 72f38ff42..89116aa74 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 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) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index a3c69f019..690640939 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 60d1b61ac..bd4f986ec 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -304,11 +304,16 @@ 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. 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(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) @@ -363,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. """ @@ -374,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 " @@ -401,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.gui.clear_error_status() # remove any permanent error status message + with open(self.sync_flag, 'w') as f: f.write(arrow.now().format()) @@ -412,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, @@ -439,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: @@ -519,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) @@ -542,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) @@ -695,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: @@ -714,6 +749,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: @@ -770,6 +806,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() @@ -787,6 +824,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') 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() 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( 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( diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 9c0f71f93..3fd4d804d 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)