Skip to content

Commit

Permalink
Export unit tests WIP. Improved error-handling in export.py.
Browse files Browse the repository at this point in the history
  • Loading branch information
@rocodes
rocodes committed Feb 6, 2024
1 parent 97ce094 commit 78440ee
Show file tree
Hide file tree
Showing 4 changed files with 251 additions and 149 deletions.
147 changes: 92 additions & 55 deletions client/securedrop_client/export.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,32 +62,41 @@ def run_printer_preflight_checks(self) -> None:
"""
logger.info("Beginning printer preflight check")
self.tmpdir = mkdtemp()
archive_path = self._create_archive(
archive_dir=self.tmpdir,
archive_fn=self._PRINTER_PREFLIGHT_FN,
metadata=self._PRINTER_PREFLIGHT_METADATA,
)
self._run_qrexec_export(
archive_path, self._on_print_preflight_success, self._on_print_prefight_error
)

try:
archive_path = self._create_archive(
archive_dir=self.tmpdir,
archive_fn=self._PRINTER_PREFLIGHT_FN,
metadata=self._PRINTER_PREFLIGHT_METADATA,
)
self._run_qrexec_export(
archive_path, self._on_print_preflight_complete, self._on_print_prefight_error
)
except ExportError as e:
logger.error("Error creating archive: {e}")
self._on_print_prefight_error()

def run_export_preflight_checks(self) -> None:
"""
Run preflight check to verify that a valid USB device is connected.
"""
logger.debug("Beginning export preflight check")

self.tmpdir = mkdtemp()
try:
self.tmpdir = mkdtemp()

archive_path = self._create_archive(
archive_dir=self.tmpdir,
archive_fn=self._USB_TEST_FN,
metadata=self._USB_TEST_METADATA,
)
# Emits status via on_process_completed()
self._run_qrexec_export(
archive_path, self._on_export_process_finished, self._on_export_process_error
)
archive_path = self._create_archive(
archive_dir=self.tmpdir,
archive_fn=self._USB_TEST_FN,
metadata=self._USB_TEST_METADATA,
)
# Emits status via on_process_completed()
self._run_qrexec_export(
archive_path, self._on_export_process_complete, self._on_export_process_error
)
except ExportError:
logger.error("Export preflight check failed during archive creation")
self._on_export_process_error()

def export(self, filepaths: List[str], passphrase: Optional[str]) -> None:
"""
Expand All @@ -112,21 +121,31 @@ def export(self, filepaths: List[str], passphrase: Optional[str]) -> None:

# Emits status through callbacks
self._run_qrexec_export(
archive_path, self._on_export_process_finished, self._on_export_process_error
archive_path, self._on_export_process_complete, self._on_export_process_error
)

except IOError as e:
logger.error("Export failed")
logger.debug(f"Export failed: {e}")
self.export_state_changed.emit(ExportStatus.ERROR_EXPORT)

# ExportStatus.ERROR_MISSING_FILES
except ExportError as err:
if err.status:
logger.error("Export failed while creating archive")
self.export_state_changed.emit(ExportError(err.status))
else:
logger.error("Export failed while creating archive (no status supplied)")
self.export_state_changed.emit(ExportError(ExportStatus.ERROR_EXPORT))

def _run_qrexec_export(
self, archive_path: str, success_callback: Callable, error_callback: Callable
) -> None:
"""
Send the archive to the Export VM, where the archive will be processed.
Uses qrexec-client-vm (via QProcess). Results are emitted via the
`on_process_finished` callback; errors are reported via `on_process_error`.
`finished` signal; errors are reported via `onError`. User-defined callback
functions must be connected to those signals.
Args:
archive_path (str): The path to the archive to be processed.
Expand Down Expand Up @@ -168,68 +187,77 @@ def _cleanup_tmpdir(self):
if self.tmpdir and os.path.exists(self.tmpdir):
shutil.rmtree(self.tmpdir)

def _on_export_process_finished(self):
def _on_export_process_complete(self):
"""
Callback, handle and emit QProcess result. As with all such callbacks,
the method signature cannot change.
"""
self._cleanup_tmpdir()
# securedrop-export writes status to stderr
err = self.process.readAllStandardError()
if self.process:
err = self.process.readAllStandardError()

logger.debug(f"stderr: {err}")
logger.debug(f"stderr: {err}")

try:
result = err.data().decode("utf-8").strip()
if result:
logger.debug(f"Result is {result}")
# This is a bit messy, but make sure we are just taking the last line
# (no-op if no newline, since we already stripped whitespace above)
status_string = result.split("\n")[-1]
self.export_state_changed.emit(ExportStatus(status_string))
try:
result = err.data().decode("utf-8").strip()
if result:
logger.debug(f"Result is {result}")
# This is a bit messy, but make sure we are just taking the last line
# (no-op if no newline, since we already stripped whitespace above)
status_string = result.split("\n")[-1]
self.export_state_changed.emit(ExportStatus(status_string))

else:
logger.error("Export subprocess did not return a value we could parse")
self.export_state_changed.emit(ExportStatus.UNEXPECTED_RETURN_STATUS)
else:
logger.error("Export preflight returned empty result")
self.export_state_changed.emit(ExportStatus.UNEXPECTED_RETURN_STATUS)

except ValueError as e:
logger.debug(f"Export subprocess returned unexpected value: {e}")
self.export_state_changed.emit(ExportStatus.UNEXPECTED_RETURN_STATUS)
except ValueError as e:
logger.debug(f"Export preflight returned unexpected value: {e}")
logger.error("Export preflight returned unexpected value")
self.export_state_changed.emit(ExportStatus.UNEXPECTED_RETURN_STATUS)

def _on_export_process_error(self):
"""
Callback, called if QProcess cannot complete export. As with all such, the method
signature cannot change.
"""
self._cleanup_tmpdir()
err = self.process.readAllStandardError().data().decode("utf-8").strip()
if self.process:
err = self.process.readAllStandardError().data().decode("utf-8").strip()

logger.error(f"Export process error: {err}")
logger.error(f"Export process error: {err}")
self.export_state_changed.emit(ExportStatus.CALLED_PROCESS_ERROR)

def _on_print_preflight_success(self):
def _on_print_preflight_complete(self):
"""
Print preflight success callback.
Print preflight completion callback.
"""
self._cleanup_tmpdir()
output = self.process.readAllStandardError().data().decode("utf-8").strip()
try:
status = ExportStatus(output)
self.print_preflight_check_succeeded.emit(status)
logger.debug("Print preflight success")

except ValueError as error:
logger.debug(f"Print preflight check failed: {error}")
logger.error("Print preflight check failed")
self.print_preflight_check_failed.emit(ExportError(ExportStatus.ERROR_PRINT))

if self.process:
output = self.process.readAllStandardError().data().decode("utf-8").strip()
try:
status = ExportStatus(output)
if status == ExportStatus.PRINT_PREFLIGHT_SUCCESS:
self.print_preflight_check_succeeded.emit(status)
logger.debug("Print preflight success")
else:
logger.debug(f"Print preflight failure ({status.value})")
self.print_preflight_check_failed.emit(status)

except ValueError as error:
logger.debug(f"Print preflight check failed: {error}")
logger.error("Print preflight check failed")
self.print_preflight_check_failed.emit(ExportError(ExportStatus.ERROR_PRINT))

def _on_print_prefight_error(self):
"""
Print Preflight error callback.
Print Preflight error callback. Occurs when the QProcess itself fails.
"""
self._cleanup_tmpdir()
err = self.process.readAllStandardError().data().decode("utf-8").strip()
logger.debug(f"Print preflight error: {err}")
if self.process:
err = self.process.readAllStandardError().data().decode("utf-8").strip()
logger.debug(f"Print preflight error: {err}")
self.print_preflight_check_failed.emit(ExportError(ExportStatus.ERROR_PRINT))

# Todo: not sure if we need to connect here, since the print dialog is managed by sd-devices.
Expand Down Expand Up @@ -283,6 +311,15 @@ def print(self, filepaths: List[str]) -> None:
logger.debug(f"Export failed: {e}")
self.print_failed.emit(ExportError(ExportStatus.ERROR_PRINT))

# ExportStatus.ERROR_MISSING_FILES
except ExportError as err:
if err.status:
logger.error("Print failed while creating archive")
self.print_failed.emit(ExportError(err.status))
else:
logger.error("Print failed while creating archive (no status supplied)")
self.print_failed.emit(ExportError(ExportStatus.ERROR_PRINT))

self.export_completed.emit(filepaths)

def _create_archive(
Expand Down
4 changes: 3 additions & 1 deletion client/securedrop_client/gui/conversation/export/export_wizard_constants.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,21 @@
Export wizard page ordering, human-readable status messages
"""


# Sequential list of pages (the enum value matters as a ranked ordering.)
# The reason the 'error' page is second is because the other pages have
# validation logic that means they can't be bypassed by QWizard::next.
# When we need to show an error, it's easier to go 'back' to the error
# page and set it to be a FinalPage than it is to try to skip the conditional
# pages. PyQt6 introduces behaviour that may deprecate this requirement.
# pages. PyQt6 introduces behaviour that may deprecate this requirement.
class Pages(IntEnum):
PREFLIGHT = 0
ERROR = 1
INSERT_USB = 2
UNLOCK_USB = 3
EXPORT_DONE = 4


# Human-readable status info
STATUS_MESSAGES = {
ExportStatus.NO_DEVICE_DETECTED: _("No device detected"),
Expand Down
19 changes: 12 additions & 7 deletions client/securedrop_client/gui/conversation/export/export_wizard_page.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ def _build_layout(self) -> QVBoxLayout:
self.header.setObjectName("ModalDialog_header")
header_container_layout.addWidget(self.header_icon)
header_container_layout.addWidget(self.header_spinner_label)
header_container_layout.addWidget(self.header, alignment=Qt.AlignLeft) # Prev: AlignCenter
header_container_layout.addWidget(self.header, alignment=Qt.AlignLeft) # Prev: AlignCenter
header_container_layout.addStretch()
self.header_line = QWidget()
self.header_line.setObjectName("ModalDialog_header_line")
Expand All @@ -122,7 +122,7 @@ def _build_layout(self) -> QVBoxLayout:
self.body_layout.addWidget(self.body)
self.body_layout.setSizeConstraint(QLayout.SetMinimumSize)

# TODO: it's either like this, or in the parent layout elements
# TODO: it's either like this, or in the parent layout elements
self.body_layout.setSizeConstraint(QLayout.SetMinimumSize)

# Widget for displaying error messages (hidden by default)
Expand Down Expand Up @@ -258,6 +258,7 @@ def on_status_received(self, status: ExportStatus):
self.header.setText(header)
self.status = status


class ErrorPage(ExportWizardPage):
def __init__(self, export, summary):
header = _("Export Failed")
Expand All @@ -267,11 +268,12 @@ def __init__(self, export, summary):

def isComplete(self) -> bool:
return False

@pyqtSlot(object)
def on_status_received(self, status: ExportStatus):
pass


class InsertUSBPage(ExportWizardPage):
def __init__(self, export, summary):
self.summary = summary
Expand All @@ -290,12 +292,12 @@ def on_status_received(self, status: ExportStatus) -> None:
should_show_hint = status in (
ExportStatus.MULTI_DEVICE_DETECTED,
ExportStatus.INVALID_DEVICE_DETECTED,
) or (self.status == status == ExportStatus.NO_DEVICE_DETECTED)
) or (self.status == status == ExportStatus.NO_DEVICE_DETECTED)
self.update_content(status, should_show_hint)
self.status = status
self.completeChanged.emit()
if status in (ExportStatus.DEVICE_LOCKED, ExportStatus.DEVICE_WRITABLE):
self.wizard().next()
self.wizard().next()

def validatePage(self) -> bool:
"""
Expand All @@ -321,7 +323,6 @@ def validatePage(self) -> bool:
logger.warning("InsertUSBPage encountered unexpected status")
return super().validatePage()


def nextId(self):
"""
Override builtin to allow bypassing the password page if device unlocked
Expand Down Expand Up @@ -435,7 +436,11 @@ def on_status_received(self, status: ExportStatus) -> None:
def validatePage(self):
# Also to add: DEVICE_BUSY for unmounting.
# This shouldn't stop us from going "back" to an error page
return self.status in (ExportStatus.DEVICE_WRITABLE, ExportStatus.SUCCESS_EXPORT, ExportStatus.ERROR_EXPORT_CLEANUP)
return self.status in (
ExportStatus.DEVICE_WRITABLE,
ExportStatus.SUCCESS_EXPORT,
ExportStatus.ERROR_EXPORT_CLEANUP,
)

def nextId(self):
if self.status == ExportStatus.SUCCESS_EXPORT:
Expand Down
Loading

0 comments on commit 78440ee

Please sign in to comment.