Update container installation logic to allow in-place updates

The isolation provider `install()` method is now passed a
`should_upgrade` argument, which is read from the settings and
represents the user decision about updates.

The tests have been updated to reflect these changes.

dangerzone/cli.py

@@ -71,8 +71,8 @@ def cli_main(
 ) -> None:
     setup_logging()
     display_banner()
+    settings = Settings()
     if set_container_runtime:
-        settings = Settings()
         if set_container_runtime == "default":
             settings.unset_custom_runtime()
             click.echo(
@@ -117,7 +117,8 @@ def cli_main(
             sys.exit(1)
 
     # Ensure container is installed
-    dangerzone.isolation_provider.install()
+    should_upgrade = bool(settings.get("updater_check_all"))
+    dangerzone.isolation_provider.install(should_upgrade)
 
     # Convert the document
     print_header("Converting document to safe PDF")

dangerzone/gui/main_window.py

@@ -450,9 +450,9 @@ class InstallContainerThread(QtCore.QThread):
     def run(self) -> None:
         error = None
         try:
-            should_upgrade = self.dangerzone.settings.get("updater_check_all")
+            should_upgrade = bool(self.dangerzone.settings.get("updater_check_all"))
             installed = self.dangerzone.isolation_provider.install(
-                should_upgrade=bool(should_upgrade), callback=self.process_stdout.emit
+                should_upgrade=should_upgrade, callback=self.process_stdout.emit
             )
         except Exception as e:
             log.error("Container installation problem")

dangerzone/isolation_provider/container.py

@@ -95,31 +95,49 @@ class Container(IsolationProvider):
 
     @staticmethod
     def install(
-        should_upgrade: bool, callback: Callable, last_try: bool = False
+        should_upgrade: bool,
+        callback: Optional[Callable] = sys.stdout.write,
+        last_try: bool = False,
     ) -> bool:
-        """Check if an update is available and install it if necessary."""
+        """
+        Install a (local or remote) container image.
+
+        Use the local `container.tar` image if:
+
+        - No image is currently installed and `should_upgrade` is set to False
+        - No image is currently installed and no upgrades are available
+
+        Upgrade to the last remote container image if:
+
+        - An upgrade is available and `should_upgrade` is set to True
+        """
+
+        installed_tags = container_utils.list_image_tags()
         if not should_upgrade:
             log.debug("Skipping container upgrade check as requested by the settings")
+            if not installed_tags:
+                install_local_container_tar()
         else:
-            update_available, image_digest = updater.is_update_available(
-                container_utils.CONTAINER_NAME,
-                updater.DEFAULT_PUBKEY_LOCATION,
+            update_available, image_digest = is_update_available(
+                CONTAINER_NAME,
+                DEFAULT_PUBKEY_LOCATION,
             )
             if update_available and image_digest:
                 log.debug("Upgrading container image to %s", image_digest)
-                updater.upgrade_container_image(
-                    container_utils.CONTAINER_NAME,
+                upgrade_container_image(
+                    CONTAINER_NAME,
                     image_digest,
-                    updater.DEFAULT_PUBKEY_LOCATION,
+                    DEFAULT_PUBKEY_LOCATION,
                     callback=callback,
                 )
             else:
-                log.debug("No update available for the container")
+                log.debug("No update available for the container.")
+                if not installed_tags:
+                    install_local_container_tar()
         try:
-            updater.verify_local_image(
-                container_utils.CONTAINER_NAME, updater.DEFAULT_PUBKEY_LOCATION
-            )
-        except errors.ImageNotPresentException:
+            verify_local_image(CONTAINER_NAME)
+        except UpdaterError:
+            # delete_image()
             if last_try:
                 raise
             log.debug("Container image not found, trying to install it.")
@@ -210,13 +228,8 @@ class Container(IsolationProvider):
     ) -> subprocess.Popen:
         runtime = Runtime()
 
-        image_digest = container_utils.get_local_image_digest(
-            container_utils.CONTAINER_NAME
-        )
-        updater.verify_local_image(
-            container_utils.CONTAINER_NAME,
-            updater.DEFAULT_PUBKEY_LOCATION,
-        )
+        image_digest = container_utils.get_local_image_digest(CONTAINER_NAME)
+        verify_local_image(CONTAINER_NAME)
         security_args = self.get_runtime_security_args()
         debug_args = []
         if self.debug:
@@ -225,7 +238,7 @@ class Container(IsolationProvider):
         enable_stdin = ["-i"]
         set_name = ["--name", name]
         prevent_leakage_args = ["--rm"]
-        image_name = [container_utils.CONTAINER_NAME + "@sha256:" + image_digest]
+        image_name = [CONTAINER_NAME + "@sha256:" + image_digest]
         args = (
             ["run"]
             + security_args

dangerzone/isolation_provider/dummy.py

@@ -36,7 +36,7 @@ class Dummy(IsolationProvider):
             )
         super().__init__()
 
-    def install(self) -> bool:
+    def install(self, *args, **kwargs) -> bool:
         return True
 
     @staticmethod

dangerzone/isolation_provider/qubes.py

@@ -18,7 +18,7 @@ log = logging.getLogger(__name__)
 class Qubes(IsolationProvider):
     """Uses a disposable qube for performing the conversion"""
 
-    def install(self) -> bool:
+    def install(self, *args, **kwargs) -> bool:
         return True
 
     @staticmethod

tests/conftest.py

@@ -9,6 +9,7 @@ import pytest
 
 from dangerzone.document import SAFE_EXTENSION
 from dangerzone.gui import Application
+from dangerzone.isolation_provider import container
 
 sys.dangerzone_dev = True  # type: ignore[attr-defined]
 

tests/gui/test_updater.py

@@ -283,13 +283,13 @@ def test_update_errors(
 ) -> None:
     """Test update check errors."""
     settings = updater.dangerzone.settings
+    # Always assume that we can perform multiple update checks in a row.
+    monkeypatch.setattr(releases, "_should_postpone_update_check", lambda _: False)
+
     # Mock requests.get().
     mocker.patch("dangerzone.updater.releases.requests.get")
     requests_mock = releases.requests.get
 
-    # Always assume that we can perform multiple update checks in a row.
-    monkeypatch.setattr(releases, "_should_postpone_update_check", lambda: False)
-
     # Test 1 - Check that request exceptions are being detected as errors.
     requests_mock.side_effect = Exception("bad url")  # type: ignore [attr-defined]
     report = releases.check_for_updates(settings)

tests/isolation_provider/test_container.py

@@ -6,9 +6,10 @@ from pytest_mock import MockerFixture
 from pytest_subprocess import FakeProcess
 
 from dangerzone import errors
-from dangerzone.container_utils import Runtime
+from dangerzone.container_utils import CONTAINER_NAME, Runtime
 from dangerzone.isolation_provider.container import Container
 from dangerzone.isolation_provider.qubes import is_qubes_native_conversion
+from dangerzone.updater import SignatureError, UpdaterError
 from dangerzone.util import get_resource_path
 
 from .base import IsolationProviderTermination, IsolationProviderTest
@@ -57,8 +58,13 @@ class TestContainer(IsolationProviderTest):
         )
         provider.is_available()
 
-    def test_install_raise_if_image_cant_be_installed(
-        self, provider: Container, fp: FakeProcess, runtime_path: str
+    def test_install_raise_if_local_image_cant_be_installed(
+        self,
+        provider: Container,
+        fp: FakeProcess,
+        runtime_path: str,
+        skip_image_verification,
+        mocker: MockerFixture,
     ) -> None:
         """When an image installation fails, an exception should be raised"""
 
@@ -74,60 +80,85 @@ class TestContainer(IsolationProviderTest):
                 "list",
                 "--format",
                 "{{ .Tag }}",
-                "dangerzone.rocks/dangerzone",
+                CONTAINER_NAME,
             ],
             occurrences=2,
         )
-
-        fp.register_subprocess(
-            [
-                runtime_path,
-                "load",
-                "-i",
-                get_resource_path("container.tar").absolute(),
-            ],
-            returncode=-1,
+        mocker.patch(
+            "dangerzone.isolation_provider.container.install_local_container_tar",
+            side_effect=UpdaterError,
         )
 
-        with pytest.raises(errors.ImageInstallationException):
-            provider.install()
+        with pytest.raises(UpdaterError):
+            provider.install(should_upgrade=False)
 
-    def test_install_raises_if_still_not_installed(
-        self, provider: Container, fp: FakeProcess, runtime_path: str
+    def test_install_raise_if_local_image_cant_be_verified(
+        self,
+        provider: Container,
+        runtime_path: str,
+        skip_image_verification,
+        mocker: MockerFixture,
     ) -> None:
-        """When an image keep being not installed, it should return False"""
-        fp.register_subprocess(
-            [runtime_path, "version", "-f", "{{.Client.Version}}"],
-            stdout="4.0.0",
+        """In case an image has been installed but its signature cannot be verified, an exception should be raised"""
+
+        mocker.patch(
+            "dangerzone.isolation_provider.container.container_utils.list_image_tags",
+            return_value=["a-tag"],
+        )
+        mocker.patch(
+            "dangerzone.isolation_provider.container.verify_local_image",
+            side_effect=SignatureError,
         )
 
-        fp.register_subprocess(
-            [runtime_path, "image", "ls"],
+        with pytest.raises(SignatureError):
+            provider.install(should_upgrade=False)
+
+    def test_install_raise_if_local_image_install_works_on_second_try(
+        self,
+        provider: Container,
+        runtime_path: str,
+        skip_image_verification,
+        mocker: MockerFixture,
+    ) -> None:
+        """In case an image has been installed but its signature cannot be verified, an exception should be raised"""
+
+        mocker.patch(
+            "dangerzone.isolation_provider.container.container_utils.list_image_tags",
+            return_value=["a-tag"],
+        )
+        mocker.patch(
+            "dangerzone.isolation_provider.container.verify_local_image",
+            side_effect=[SignatureError, True],
         )
 
-        # First check should return nothing.
-        fp.register_subprocess(
-            [
-                runtime_path,
-                "image",
-                "list",
-                "--format",
-                "{{ .Tag }}",
-                "dangerzone.rocks/dangerzone",
-            ],
-            occurrences=2,
+        provider.install(should_upgrade=False)
+
+    def test_install_upgrades_if_available(
+        self,
+        provider: Container,
+        runtime_path: str,
+        skip_image_verification,
+        mocker: MockerFixture,
+    ) -> None:
+        """In case an image has been installed but its signature cannot be verified, an exception should be raised"""
+
+        mocker.patch(
+            "dangerzone.isolation_provider.container.container_utils.list_image_tags",
+            return_value=["a-tag"],
+        )
+        mocker.patch(
+            "dangerzone.isolation_provider.container.is_update_available",
+            return_value=(True, "digest"),
+        )
+        upgrade = mocker.patch(
+            "dangerzone.isolation_provider.container.upgrade_container_image",
+        )
+        mocker.patch(
+            "dangerzone.isolation_provider.container.verify_local_image",
         )
 
-        fp.register_subprocess(
-            [
-                runtime_path,
-                "load",
-                "-i",
-                get_resource_path("container.tar").absolute(),
-            ],
-        )
-        with pytest.raises(errors.ImageNotPresentException):
-            provider.install()
+        provider.install(should_upgrade=True)
+        upgrade.assert_called()
 
     @pytest.mark.skipif(
         platform.system() not in ("Windows", "Darwin"),