From a9f0590f746a6b04084a3fa1b743c93850a5690c Mon Sep 17 00:00:00 2001 From: Ro Date: Thu, 12 Sep 2024 15:08:28 -0400 Subject: [PATCH] Handle NoResultFound when querying for a file, message, or reply. Refactor FileWidget to accept a File instead of a file uuid, avoiding potentially-null database query during widget construction. Add tests for deleted db record condition. Fixes #2217. --- client/securedrop_client/gui/widgets.py | 42 +- client/securedrop_client/logic.py | 69 +- client/securedrop_client/storage.py | 21 +- .../data/test_deleted_file_filewidget.yml | 1242 +++++++++++++++++ .../test_deleted_file_filewidget.py | 67 + client/tests/gui/test_widgets.py | 95 +- client/tests/test_logic.py | 6 +- client/tests/test_storage.py | 33 +- 8 files changed, 1492 insertions(+), 83 deletions(-) create mode 100644 client/tests/functional/data/test_deleted_file_filewidget.yml create mode 100644 client/tests/functional/test_deleted_file_filewidget.py diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index 74a486524..715b8e3ce 100644 --- a/client/securedrop_client/gui/widgets.py +++ b/client/securedrop_client/gui/widgets.py @@ -2240,7 +2240,7 @@ class FileWidget(QWidget): def __init__( self, - file_uuid: str, + file: File, controller: Controller, file_download_started: pyqtBoundSignal, file_ready_signal: pyqtBoundSignal, @@ -2255,8 +2255,8 @@ def __init__( self.controller = controller - self.file = self.controller.get_file(file_uuid) - self.uuid = file_uuid + self.file = file + self.uuid = file.uuid self.index = index self.downloading = False @@ -2494,16 +2494,20 @@ def _on_left_click(self) -> None: of the file distinguishes which function in the logic layer to call. """ # update state - self.file = self.controller.get_file(self.uuid) - - if self.file.is_decrypted: - # Open the already downloaded and decrypted file. - self.controller.on_file_open(self.file) - elif not self.downloading: - if self.controller.api: - self.start_button_animation() - # Download the file. - self.controller.on_submission_download(File, self.uuid) + file = self.controller.get_file(self.uuid) + if file is None: + # the record was deleted from the database, so delete the widget. + self.deleteLater() + else: + self.file = file + if self.file.is_decrypted: + # Open the already downloaded and decrypted file. + self.controller.on_file_open(self.file) + elif not self.downloading: + if self.controller.api: + self.start_button_animation() + # Download the file. + self.controller.on_submission_download(File, self.uuid) def start_button_animation(self) -> None: """ @@ -2531,8 +2535,12 @@ def stop_button_animation(self) -> None: Stops the download animation and restores the button to its default state. """ self.download_animation.stop() - self.file = self.controller.get_file(self.file.uuid) - self._set_file_state() + file = self.controller.get_file(self.file.uuid) + if file is None: + self.deleteLater() + else: + self.file = file + self._set_file_state() class ConversationScrollArea(QScrollArea): @@ -2860,9 +2868,11 @@ def add_file(self, file: File, index: int) -> None: """ Add a file from the source. """ + # If we encounter any issues with FileWidget rendering updated information, the + # reference can be refreshed here before the widget is created. logger.debug(f"Adding file for {file.uuid}") conversation_item = FileWidget( - file.uuid, + file, self.controller, self.controller.file_download_started, self.controller.file_ready, diff --git a/client/securedrop_client/logic.py b/client/securedrop_client/logic.py index 34cec4246..565774ce5 100644 --- a/client/securedrop_client/logic.py +++ b/client/securedrop_client/logic.py @@ -855,7 +855,11 @@ def on_message_download_success(self, uuid: str) -> None: """ self.session.commit() # Needed to flush stale data. message = storage.get_message(self.session, uuid) - self.message_ready.emit(message.source.uuid, message.uuid, message.content) + if message: + self.message_ready.emit(message.source.uuid, message.uuid, message.content) + else: + logger.error("Failed to find message uuid in database") + logger.debug(f"Message {uuid} missing from database but download successful") def on_message_download_failure(self, exception: DownloadException) -> None: """ @@ -867,12 +871,12 @@ def on_message_download_failure(self, exception: DownloadException) -> None: self._submit_download_job(exception.object_type, exception.uuid) self.session.commit() - try: - message = storage.get_message(self.session, exception.uuid) + message = storage.get_message(self.session, exception.uuid) + if message: self.message_download_failed.emit(message.source.uuid, message.uuid, str(message)) - except Exception as e: - logger.error("Could not emit message_download_failed") - logger.debug(f"Could not emit message_download_failed: {e}") + else: + logger.warning("Message download failure for uuid not in database.") + logger.debug(f"Message {exception.uuid} missing from database, was it deleted?") def download_new_replies(self) -> None: replies = storage.find_new_replies(self.session) @@ -890,7 +894,11 @@ def on_reply_download_success(self, uuid: str) -> None: """ self.session.commit() # Needed to flush stale data. reply = storage.get_reply(self.session, uuid) - self.reply_ready.emit(reply.source.uuid, reply.uuid, reply.content) + if reply: + self.reply_ready.emit(reply.source.uuid, reply.uuid, reply.content) + else: + logger.error("Reply downloaded but reply uuid missing from database") + logger.debug(f"Reply {uuid} downloaded but uuid missing from database") def on_reply_download_failure(self, exception: DownloadException) -> None: """ @@ -902,12 +910,13 @@ def on_reply_download_failure(self, exception: DownloadException) -> None: self._submit_download_job(exception.object_type, exception.uuid) self.session.commit() - try: - reply = storage.get_reply(self.session, exception.uuid) + reply = storage.get_reply(self.session, exception.uuid) + if reply: self.reply_download_failed.emit(reply.source.uuid, reply.uuid, str(reply)) - except Exception as e: - logger.error("Could not emit reply_download_failed") - logger.debug(f"Could not emit reply_download_failed: {e}") + else: + # Not necessarily an error, it may have been deleted. Warn. + logger.warning("Reply download failure for uuid not in database") + logger.debug(f"Reply {exception.uuid} not found in database") def downloaded_file_exists(self, file: db.File, silence_errors: bool = False) -> bool: """ @@ -964,6 +973,12 @@ def on_file_download_success(self, uuid: str) -> None: """ self.session.commit() file_obj = storage.get_file(self.session, uuid) + if not file_obj: + # This shouldn't happen + logger.error("File downloaded but uuid missing from database") + logger.debug(f"File {uuid} downloaded but uuid missing from database") + return + file_obj.download_error = None storage.update_file_size(uuid, self.data_dir, self.session) self._state.record_file_download(state.FileId(uuid)) @@ -981,7 +996,17 @@ def on_file_download_failure(self, exception: Exception) -> None: else: if isinstance(exception, DownloadDecryptionException): logger.error("Failed to decrypt %s", exception.uuid) - f = self.get_file(exception.uuid) + f = storage.get_file(self.session, exception.uuid) + if not f: + # This isn't necessarily an error; the file may have been deleted + # at the server and has been removed from the database. + logger.warning("File download failure for uuid not in database.") + logger.debug( + f"File download failure but uuid {exception.uuid} uuid not in database, " + "was it deleted?" + ) + return + self.file_missing.emit(f.source.uuid, f.uuid, str(f)) self.gui.update_error_status(_("The file download failed. Please try again.")) @@ -1114,7 +1139,11 @@ def on_reply_success(self, reply_uuid: str) -> None: logger.info(f"{reply_uuid} sent successfully") self.session.commit() reply = storage.get_reply(self.session, reply_uuid) - self.reply_succeeded.emit(reply.source.uuid, reply_uuid, reply.content) + if reply: + self.reply_succeeded.emit(reply.source.uuid, reply_uuid, reply.content) + else: + logger.error("Reply uuid not found in database") + logger.debug(f"Reply {reply_uuid} uuid not found in database") def on_reply_failure(self, exception: SendReplyJobError | SendReplyJobTimeoutError) -> None: logger.debug(f"{exception.reply_uuid} failed to send") @@ -1123,8 +1152,18 @@ def on_reply_failure(self, exception: SendReplyJobError | SendReplyJobTimeoutErr if isinstance(exception, SendReplyJobError): self.reply_failed.emit(exception.reply_uuid) - def get_file(self, file_uuid: str) -> db.File: + def get_file(self, file_uuid: str) -> db.File | None: + """ + Wraps storage.py. GUI caller can use this method; internally, prioritize + storage.get_file. + """ file = storage.get_file(self.session, file_uuid) + if not file: + # Not necessarily an error + logger.warning("get_file for uuid not in database") + logger.debug(f"File {file_uuid} not found in database") + return None + self.session.refresh(file) return file diff --git a/client/securedrop_client/storage.py b/client/securedrop_client/storage.py index ffb2d9526..b364674cd 100644 --- a/client/securedrop_client/storage.py +++ b/client/securedrop_client/storage.py @@ -1055,16 +1055,25 @@ def source_exists(session: Session, source_uuid: str) -> bool: return False -def get_file(session: Session, uuid: str) -> File: - return session.query(File).filter_by(uuid=uuid).one() +def get_file(session: Session, uuid: str) -> File | None: + """ + Get File object by uuid. + """ + return session.query(File).filter_by(uuid=uuid).one_or_none() -def get_message(session: Session, uuid: str) -> Message: - return session.query(Message).filter_by(uuid=uuid).one() +def get_message(session: Session, uuid: str) -> Message | None: + """ + Get Message object by uuid. + """ + return session.query(Message).filter_by(uuid=uuid).one_or_none() -def get_reply(session: Session, uuid: str) -> Reply: - return session.query(Reply).filter_by(uuid=uuid).one() +def get_reply(session: Session, uuid: str) -> Reply | None: + """ + Get Reply object by uuid. + """ + return session.query(Reply).filter_by(uuid=uuid).one_or_none() def mark_all_pending_drafts_as_failed(session: Session) -> list[DraftReply]: diff --git a/client/tests/functional/data/test_deleted_file_filewidget.yml b/client/tests/functional/data/test_deleted_file_filewidget.yml new file mode 100644 index 000000000..d21284c47 --- /dev/null +++ b/client/tests/functional/data/test_deleted_file_filewidget.yml @@ -0,0 +1,1242 @@ +interactions: +- request: + body: '{"username": "journalist", "passphrase": "correct horse battery staple + profanity oil chewy", "one_time_code": "918636"}' + headers: {} + method: POST + uri: api/v1/token + response: !!python/object:securedrop_client.sdk.JSONResponse + data: + expiration: '2024-02-29T23:59:28.948390Z' + journalist_first_name: null + journalist_last_name: null + journalist_uuid: c3375c32-955a-435e-b5d4-5737e2ac6d5e + token: ImVQSHpYcUtKa3lya0RxVEpERFF6cGdZdWlUMTRDVUY1RDlib0lyYnh6X2si.ZeD-QA.w0_YrvPSX-gwlLikWg_8DoJbDuU + headers: + connection: close + content-length: '290' + content-type: application/json + date: Thu, 29 Feb 2024 21:59:28 GMT + server: Werkzeug/2.2.3 Python/3.8.10 + status: 200 +- request: + body: null + headers: + Accept: + - application/json + Authorization: + - Token ImVQSHpYcUtKa3lya0RxVEpERFF6cGdZdWlUMTRDVUY1RDlib0lyYnh6X2si.ZeD-QA.w0_YrvPSX-gwlLikWg_8DoJbDuU + Content-Type: + - application/json + method: GET + uri: api/v1/users + response: !!python/object:securedrop_client.sdk.JSONResponse + data: + users: + - first_name: null + last_name: null + username: journalist + uuid: c3375c32-955a-435e-b5d4-5737e2ac6d5e + - first_name: null + last_name: null + username: dellsberg + uuid: 653f977b-f651-4229-a4d8-08806cd99926 + - first_name: null + last_name: null + username: deleted + uuid: 9a2d3d07-1dcd-4fe3-b715-a9ffd583000c + headers: + connection: close + content-length: '474' + content-type: application/json + date: Thu, 29 Feb 2024 21:59:29 GMT + server: Werkzeug/2.2.3 Python/3.8.10 + status: 200 +- request: + body: null + headers: + Accept: + - application/json + Authorization: + - Token ImVQSHpYcUtKa3lya0RxVEpERFF6cGdZdWlUMTRDVUY1RDlib0lyYnh6X2si.ZeD-QA.w0_YrvPSX-gwlLikWg_8DoJbDuU + Content-Type: + - application/json + method: GET + uri: api/v1/sources + response: !!python/object:securedrop_client.sdk.JSONResponse + data: + sources: + - add_star_url: /api/v1/sources/c7482aed-6830-42b4-9fa0-eb393f46d8dc/add_star + interaction_count: 6 + is_flagged: false + is_starred: false + journalist_designation: binomial survivalist + key: + fingerprint: 9DDCE0E92E70C18D8C30224CEF9EF0A4EA9AE066 + public: '-----BEGIN PGP PUBLIC KEY BLOCK----- + + Comment: 9DDC E0E9 2E70 C18D 8C30 224C EF9E F0A4 EA9A E066 + + Comment: Source Key bool: @@ -1794,6 +1791,30 @@ def test_get_reply(mocker, session): assert result == reply +def test_get_object_does_not_exist_returns_None(mocker, session): + source = factory.Source() + # Add the source, but not the items + session.add(source) + + items = [ + factory.File(source=source), + factory.Message(source=source), + factory.Reply(source=source), + ] + + def get_object(item) -> callable: + if isinstance(item, db.File): + return get_file + elif isinstance(item, db.Reply): + return get_reply + else: # db.Message + return get_message + + for item in items: + get_item = get_object(item) + assert get_item(session, item.uuid) is None + + def test_mark_pending_replies_as_failed(mocker, session, reply_status_codes): source = factory.Source() pending_status = (