nk3: Improve error handling during firmware update

This patch improves the error handling for firmware updates.  First of
all, it actually shows the error message to the user instead of just
logging it.  Also, it adds a distinction between failed updates and
aborted updates (due to a user decision).

Fixes: #296
Fixes: #339

Author: robin-nitrokey

nitrokeyapp/device_data.py

@@ -6,7 +6,7 @@
 from nitrokey.trussed import TrussedBase, TrussedDevice, Uuid, Version
 from nitrokey.trussed.admin_app import Status
 
-from nitrokeyapp.update import Nk3Context, UpdateGUI
+from nitrokeyapp.update import Nk3Context, UpdateGUI, UpdateResult, UpdateStatus
 
 logger = logging.getLogger(__name__)
 
@@ -106,14 +106,12 @@ def update(
         self,
         ui: UpdateGUI,
         image: Optional[str] = None,
-    ) -> bool:
+    ) -> UpdateResult:
         self.updating = True
-        try:
-            Nk3Context(self.path).update(ui, image)
+        result = Nk3Context(self.path).update(ui, image)
+        if result.status == UpdateStatus.SUCCESS:
             logger.info("Nitrokey 3 successfully updated")
-            self.updating = False
-            return True
-        except Exception as e:
-            logger.info(f"Nitrokey 3 failed to update - {e}")
-            self.updating = False
-            return False
+        else:
+            logger.error(f"Nitrokey 3 update failed: {result}")
+        self.updating = False
+        return result

nitrokeyapp/overview_tab/__init__.py

@@ -9,6 +9,7 @@
 from nitrokeyapp.common_ui import CommonUi
 from nitrokeyapp.device_data import DeviceData
 from nitrokeyapp.qt_utils_mix_in import QtUtilsMixIn
+from nitrokeyapp.update import UpdateResult, UpdateStatus
 from nitrokeyapp.worker import Worker
 
 from .worker import OverviewWorker
@@ -208,13 +209,24 @@ def run_update(self) -> None:
 
         self.trigger_update.emit(self.data, self.is_qubesos)
 
-    @Slot(bool)
-    def device_updated(self, success: bool) -> None:
+    @Slot(UpdateResult)
+    def device_updated(self, result: UpdateResult) -> None:
         self.update_btns_during_update(True)
-        if success:
-            self.common_ui.info.info.emit("Nitrokey 3 successfully updated")
+
+        msg = ""
+        if result.message is not None:
+            msg = ": " + result.message
+
+        if result.status == UpdateStatus.SUCCESS:
+            self.common_ui.info.info.emit(f"Nitrokey 3 successfully updated{msg}")
+        elif result.status == UpdateStatus.ERROR:
+            self.common_ui.info.error.emit(f"Nitrokey 3 update failed{msg}")
+        elif result.status == UpdateStatus.ABORTED:
+            self.common_ui.info.error.emit(f"Nitrokey 3 update aborted{msg}")
         else:
-            self.common_ui.info.error.emit("Nitrokey 3 update failed")
+            self.common_ui.info.error.emit(
+                f"Unexpected update result: {result.status}{msg}"
+            )
 
         self.common_ui.gui.refresh_devices.emit()
 

nitrokeyapp/overview_tab/worker.py

@@ -5,14 +5,14 @@
 
 from nitrokeyapp.common_ui import CommonUi
 from nitrokeyapp.device_data import DeviceData
-from nitrokeyapp.update import UpdateGUI
+from nitrokeyapp.update import UpdateGUI, UpdateResult
 from nitrokeyapp.worker import Job, Worker
 
 logger = logging.getLogger(__name__)
 
 
 class UpdateDevice(Job):
-    device_updated = Signal(bool)
+    device_updated = Signal(UpdateResult)
 
     def __init__(
         self,
@@ -34,11 +34,11 @@ def __init__(
 
     def run(self) -> None:
         if not self.image:
-            success = self.data.update(self.update_gui)
+            result = self.data.update(self.update_gui)
         else:
-            success = self.data.update(self.update_gui, self.image)
+            result = self.data.update(self.update_gui, self.image)
 
-        self.device_updated.emit(success)
+        self.device_updated.emit(result)
 
     @Slot()
     def cleanup(self) -> None:
@@ -51,7 +51,7 @@ def cancel_busy_wait(self, confirmed: bool) -> None:
 
 class OverviewWorker(Worker):
     # TODO: remove DeviceData from signatures
-    device_updated = Signal(bool)
+    device_updated = Signal(UpdateResult)
 
     def __init__(self, common_ui: CommonUi) -> None:
         super().__init__(common_ui)

nitrokeyapp/update.py

@@ -1,5 +1,7 @@
 import logging
 from contextlib import contextmanager
+from dataclasses import dataclass
+from enum import Enum
 from time import sleep
 from typing import (
     TYPE_CHECKING,
@@ -29,25 +31,45 @@
 T = TypeVar("T", bound=TrussedBase)
 
 
+class UpdateStatus(Enum):
+    SUCCESS = "success"
+    ERROR = "error"
+    ABORTED = "aborted"
+
+
+@dataclass
+class UpdateResult:
+    status: UpdateStatus
+    message: str | None = None
+
+
+@dataclass
+class UpdateException(Exception):
+    def __init__(self, status: UpdateStatus, *msgs: Any) -> None:
+        super().__init__(*msgs)
+        self.status = status
+
+
 class UpdateGUI(UpdateUi):
     def __init__(self, common_ui: "CommonUi", is_qubesos: bool) -> None:
         super().__init__()
 
-        self._version_printed = False
         self.common_ui = common_ui
         self.is_qubesos = is_qubesos
 
         # blocking wait, set by parent during confirm-prompt
         self.await_confirmation: Optional[bool] = None
 
     def error(self, *msgs: Any) -> Exception:
-        return Exception(*msgs)
+        logger.error(f"Error during firmware update: {msgs}")
+        return UpdateException(UpdateStatus.ERROR, *msgs)
 
     def abort(self, *msgs: Any) -> Exception:
-        return Exception(*msgs)
+        logger.warning(f"Firmware update aborted: {msgs}")
+        return UpdateException(UpdateStatus.ABORTED, *msgs)
 
     def raise_warning(self, warning: Warning) -> Exception:
-        return Exception(warning.message)
+        return UpdateException(UpdateStatus.ERROR, warning.message)
 
     def show_warning(self, warning: Warning) -> None:
         res = self.run_confirm_dialog(
@@ -60,8 +82,9 @@ def show_warning(self, warning: Warning) -> None:
         logger.info("OK clicked (warning dialog)")
 
     def abort_downgrade(self, current: Version, image: Version) -> Exception:
-        self._print_firmware_versions(current, image)
-        return self.abort("Abort: firmware older as on device")
+        return self.abort(
+            f"firmware {image} is older than the firmware on the device ({current})"
+        )
 
     def run_confirm_dialog(self, title: str, desc: str) -> bool:
         self.common_ui.prompt.confirm.emit(title, desc)
@@ -106,7 +129,7 @@ def confirm_update(self, current: Optional[Version], new: Version) -> None:
             )
         if not res:
             logger.info("Cancel clicked (confirm update)")
-            raise self.abort("Abort: canceled by user (confirm update)")
+            raise self.abort("canceled by user (confirm update)")
 
         logger.info("OK clicked (confirm update)")
         self.common_ui.touch.start.emit()
@@ -122,30 +145,30 @@ def confirm_update_same_version(self, version: Version) -> None:
         )
         if not res:
             logger.info("Cancel clicked (confirm same version)")
-            raise self.abort("Abort: canceled by user (confirm same version)")
+            raise self.abort("canceled by user (confirm same version)")
 
         logger.info("OK clicked (confirm same version)")
 
     def confirm_extra_information(self, txt: List[str]) -> None:
         res = self.run_confirm_dialog("Confirm extra information", " ".join(txt))
         if not res:
             logger.info("Cancel clicked (confirm extra info)")
-            raise self.abort("Abort: canceled by user (confirm extra info)")
+            raise self.abort("canceled by user (confirm extra info)")
 
         logger.info("OK clicked (confirm same version)")
 
     def abort_pynitrokey_version(
         self, current: Version, required: Version
     ) -> Exception:
-        raise self.abort(f"Abort: pynitrokey {required} too old, need: {current}")
+        raise self.abort(f"pynitrokey {required} too old, need: {current}")
 
     def confirm_pynitrokey_version(self, current: Version, required: Version) -> None:
         # TODO: implement
-        raise self.abort(f"Abort: pynitrokey {required} too old, need: {current}")
+        raise self.abort(f"pynitrokey {required} too old, need: {current}")
 
     def request_repeated_update(self) -> Exception:
         logger.info("Bootloader mode enabled. Repeat to update")
-        return self.abort("Abort: bootloader enabled")
+        return self.abort("bootloader enabled")
 
     def request_bootloader_confirmation(self) -> None:
         logger.info("requesting bootloader confirmation")
@@ -173,15 +196,6 @@ def finalization_progress_bar(
         self.common_ui.progress.start.emit("Finalization")
         yield self.common_ui.progress.progress.emit
 
-    def _print_firmware_versions(
-        self, current: Optional[Version], new: Optional[Version]
-    ) -> None:
-        if not self._version_printed:
-            current_str = str(current) if current else "[unknown]"
-            logger.info(f"Current firmware version:  {current_str}")
-            logger.info(f"Updated firmware version:  {new}")
-            self._version_printed = True
-
 
 class Nk3Context(DeviceHandler):
     def __init__(self, path: str) -> None:
@@ -245,21 +259,31 @@ def update(
         self,
         ui: UpdateGUI,
         image: Optional[str] = None,
-        version: Optional[str] = None,
-        ignore_pynitrokey_version: bool = False,
-    ) -> None:
-        with self.connect() as device:
-            updater = Updater(ui, self)
-            _, status = updater.update(
-                device, image, version, ignore_pynitrokey_version
-            )
+    ) -> UpdateResult:
+        try:
+            with self.connect() as device:
+                updater = Updater(ui, self)
+                _, status = updater.update(
+                    device=device, image=image, update_version=None
+                )
+        except UpdateException as e:
+            return UpdateResult(status=e.status, message=str(e))
+        except Exception as e:
+            return UpdateResult(status=UpdateStatus.ERROR, message=str(e))
+
         if status.init_status is not None:
             if status.init_status & InitStatus.EXT_FLASH_NEED_REFORMAT:
-                raise Exception(
-                    "External filesystem needs to be reformatted."
-                    " Please contact support@nitrokey.com for more information on how to solve this issue."
+                logger.error(
+                    f"Problematic init status after update: {status.init_status}"
+                )
+                return UpdateResult(
+                    status=UpdateStatus.ERROR,
+                    message="External filesystem needs to be reformatted."
+                    " Please contact support@nitrokey.com for more information on how to solve this issue.",
                 )
 
+        return UpdateResult(status=UpdateStatus.SUCCESS)
+
 
 class Try:
     """Utility class for an execution of a repeated action with Retries."""