Skip to content

Commit

Permalink
Have BaseErrors trigger DownloadDecryptionException in download flow
Browse files Browse the repository at this point in the history
DownloadDecryptionException is specially handled in
Controller.on_file_download_failure(), so if we raise a different
exception class, the download animation will never be stopped because
file_missing is never emitted.

It is worth a proper re-examination of our exception heirarchy and
reducing the amount of indirection in the download flow, but that can be
done outside of a hotfix.

More debug logging is added to trace when slots are triggered.

Refs #1633.
  • Loading branch information
legoktm committed Sep 3, 2024
1 parent 7d0abf1 commit 3b183fb
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 4 deletions.
4 changes: 1 addition & 3 deletions client/securedrop_client/api_jobs/downloads.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,7 @@ def _download(self, api: API, db_object: File | Message | Reply, session: Sessio
mark_as_downloaded(type(db_object), db_object.uuid, session)
logger.info(f"File downloaded to {destination}")
return destination
except BaseError as e:
raise e
except (ValueError, FileNotFoundError, RuntimeError) as e:
except (ValueError, FileNotFoundError, RuntimeError, BaseError) as e:
logger.error("Download failed")
logger.debug(f"Download failed: {e}")
raise DownloadDecryptionException(
Expand Down
6 changes: 6 additions & 0 deletions client/securedrop_client/gui/widgets.py
Original file line number Diff line number Diff line change
Expand Up @@ -2434,6 +2434,9 @@ def _on_file_download_started(self, id: state.FileId) -> None:

@pyqtSlot(str, str, str)
def _on_file_downloaded(self, source_uuid: str, file_uuid: str, filename: str) -> None:
logger.debug(
f"_on_file_downloaded: {source_uuid} / {file_uuid} ({filename}), expected {self.uuid}"
)
if file_uuid == self.uuid:
self.downloading = False
QTimer.singleShot(
Expand All @@ -2442,6 +2445,9 @@ def _on_file_downloaded(self, source_uuid: str, file_uuid: str, filename: str) -

@pyqtSlot(str, str, str)
def _on_file_missing(self, source_uuid: str, file_uuid: str, filename: str) -> None:
logger.debug(
f"_on_file_missing: {source_uuid} / {file_uuid} ({filename}), expected {self.uuid}"
)
if file_uuid == self.uuid:
self.downloading = False
QTimer.singleShot(
Expand Down
2 changes: 1 addition & 1 deletion client/tests/api_jobs/test_downloads.py
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ def test_MessageDownloadJob_with_base_error(mocker, homedir, session, session_ma
mocker.patch.object(api_client, "download_submission", side_effect=BaseError)
decrypt_fn = mocker.patch.object(job.gpg, "decrypt_submission_or_reply")

with pytest.raises(BaseError):
with pytest.raises(DownloadDecryptionException):
job.call_api(api_client, session)

assert message.content is None
Expand Down

0 comments on commit 3b183fb

Please sign in to comment.