From 35e66a48059d67c02321c78a2046db36988ae729 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Mon, 16 Oct 2023 16:58:20 +1300 Subject: [PATCH] remove PREFERRED capabilities Way back when I added support for core boot classic installs, we did not have a way to indicate whether the installed model preferred to be installed encrypted or unencrypted. Roll time on 9 months or so and our API now supports a list of capabilites and we can define that we can indicate which is preferred by putting it first in the list. One part of the implementation is a bit delicate really -- relying on list.sort() being stable -- so maybe we should clean that up. But I think the other changes make everything nicer. --- subiquity/common/types.py | 7 +-- subiquity/server/controllers/filesystem.py | 40 ++++---------- .../controllers/tests/test_filesystem.py | 40 ++++++++++++-- subiquity/tests/api/test_api.py | 2 +- subiquity/ui/views/filesystem/guided.py | 52 +++++++++---------- 5 files changed, 71 insertions(+), 70 deletions(-) diff --git a/subiquity/common/types.py b/subiquity/common/types.py index 712f2699..2587e9d8 100644 --- a/subiquity/common/types.py +++ b/subiquity/common/types.py @@ -350,13 +350,12 @@ class GuidedCapability(enum.Enum): CORE_BOOT_ENCRYPTED = enum.auto() CORE_BOOT_UNENCRYPTED = enum.auto() - # These two are not valid as GuidedChoiceV2.capability: - CORE_BOOT_PREFER_ENCRYPTED = enum.auto() - CORE_BOOT_PREFER_UNENCRYPTED = enum.auto() DD = enum.auto() def __lt__(self, other) -> bool: + if self.is_core_boot() and other.is_core_boot(): + return False return self.value < other.value def is_lvm(self) -> bool: @@ -366,8 +365,6 @@ class GuidedCapability(enum.Enum): return self in [ GuidedCapability.CORE_BOOT_ENCRYPTED, GuidedCapability.CORE_BOOT_UNENCRYPTED, - GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED, - GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED, ] def supports_manual_customization(self) -> bool: diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 9995e4e9..fab0008d 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -394,11 +394,13 @@ class FilesystemController(SubiquityController, FilesystemManipulator): info.capability_info.allowed = [GuidedCapability.CORE_BOOT_ENCRYPTED] elif se.storage_safety == StorageSafety.PREFER_ENCRYPTED: info.capability_info.allowed = [ - GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED + GuidedCapability.CORE_BOOT_ENCRYPTED, + GuidedCapability.CORE_BOOT_UNENCRYPTED, ] elif se.storage_safety == StorageSafety.PREFER_UNENCRYPTED: info.capability_info.allowed = [ - GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED + GuidedCapability.CORE_BOOT_UNENCRYPTED, + GuidedCapability.CORE_BOOT_ENCRYPTED, ] return info @@ -651,26 +653,8 @@ class FilesystemController(SubiquityController, FilesystemManipulator): def set_info_for_capability(self, capability: GuidedCapability): """Given a request for a capability, select the variation to use.""" - if capability == GuidedCapability.CORE_BOOT_ENCRYPTED: - # If the request is for encryption, a variation with any - # of these capabilities is OK: - caps = { - GuidedCapability.CORE_BOOT_ENCRYPTED, - GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED, - GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED, - } - elif capability == GuidedCapability.CORE_BOOT_UNENCRYPTED: - # Similar if the request is for uncrypted - caps = { - GuidedCapability.CORE_BOOT_UNENCRYPTED, - GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED, - GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED, - } - else: - # Otherwise, just look for what we were asked for. - caps = {capability} for info in self._variation_info.values(): - if caps & set(info.capability_info.allowed): + if capability in info.capability_info.allowed: self._info = info return raise Exception("could not find variation for {}".format(capability)) @@ -1313,12 +1297,12 @@ class FilesystemController(SubiquityController, FilesystemManipulator): if name == "hybrid": # this check is conceptually unnecessary but results in a # much cleaner error message... - core_boot_caps = set() + core_boot_caps = [] for variation in self._variation_info.values(): if not variation.is_valid(): continue if variation.is_core_boot_classic(): - core_boot_caps.update(variation.capability_info.allowed) + core_boot_caps.extend(variation.capability_info.allowed) if not core_boot_caps: raise Exception( "can only use name: hybrid when installing core boot classic" @@ -1328,18 +1312,12 @@ class FilesystemController(SubiquityController, FilesystemManipulator): encrypted = layout.get("encrypted", None) GC = GuidedCapability if encrypted is None: - if ( - GC.CORE_BOOT_ENCRYPTED in core_boot_caps - or GC.CORE_BOOT_PREFER_ENCRYPTED in core_boot_caps - ): - capability = GC.CORE_BOOT_ENCRYPTED - else: - capability = GC.CORE_BOOT_UNENCRYPTED + capability = core_boot_caps[0] elif encrypted: capability = GC.CORE_BOOT_ENCRYPTED else: if ( - core_boot_caps == {GuidedCapability.CORE_BOOT_ENCRYPTED} + core_boot_caps == [GuidedCapability.CORE_BOOT_ENCRYPTED] and not encrypted ): raise Exception("cannot install this model unencrypted") diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index 4321601d..a0c55151 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -696,7 +696,9 @@ class TestLayout(IsolatedAsyncioTestCase): class TestGuidedV2(IsolatedAsyncioTestCase): - async def _setup(self, bootloader, ptable, fix_bios=True, **kw): + async def _setup( + self, bootloader, ptable, fix_bios=True, snapd_system_labels=(None,), **kw + ): self.app = make_app() self.app.opts.bootloader = bootloader.value self.fsc = FilesystemController(app=self.app) @@ -704,11 +706,16 @@ class TestGuidedV2(IsolatedAsyncioTestCase): self.fsc.calculate_suggested_install_min.return_value = 10 << 30 self.fsc.model = self.model = make_model(bootloader) self.fsc._examine_systems_task.start_sync() + self.app.snapdapi = snapdapi.make_api_client(AsyncSnapd(get_fake_connection())) self.app.dr_cfg = DRConfig() + self.app.dr_cfg.systems_dir_exists = True + self.app.base_model.source.search_drivers = False self.app.base_model.source.current.type = "fsimage" - self.app.base_model.source.current.variations = { - "default": CatalogEntryVariation(path="", size=1), - } + variations = self.app.base_model.source.current.variations = {} + for label in snapd_system_labels: + variations["var" + str(label)] = CatalogEntryVariation( + path="", size=1, snapd_system_label=label + ) self.app.controllers.Source.get_handler.return_value = TrivialSourceHandler("") await self.fsc._examine_systems_task.wait() self.disk = make_disk(self.model, ptable=ptable, **kw) @@ -719,7 +726,7 @@ class TestGuidedV2(IsolatedAsyncioTestCase): "filesystem": self.fs_probe, } self.fsc._probe_task.task = mock.Mock() - self.fsc._examine_systems_task.task = mock.Mock() + # self.fsc._examine_systems_task.task = mock.Mock() if bootloader == Bootloader.BIOS and ptable != "msdos" and fix_bios: make_partition( self.model, @@ -1153,6 +1160,29 @@ class TestGuidedV2(IsolatedAsyncioTestCase): self.assertEqual(expected, resp.targets) self.assertEqual(ProbeStatus.DONE, resp.status) + @parameterized.expand( + [ + ("mandatory", ["ENCRYPTED"]), + ("prefer-encrypted", ["ENCRYPTED", "UNENCRYPTED"]), + ("prefer-unencrypted", ["UNENCRYPTED", "ENCRYPTED"]), + ("unavailable", ["UNENCRYPTED"]), + ] + ) + async def test_core_boot_and_classic(self, label, core_boot_cap_names): + expected_core_boot_caps = [ + getattr(GuidedCapability, f"CORE_BOOT_{cap_name}") + for cap_name in core_boot_cap_names + ] + await self._setup( + Bootloader.UEFI, + "gpt", + snapd_system_labels=[None, label], + ) + resp = await self.fsc.v2_guided_GET() + [target, man] = resp.targets + got_core_boot_caps = [cap for cap in target.allowed if cap.is_core_boot()] + self.assertEqual(got_core_boot_caps, expected_core_boot_caps) + class TestManualBoot(IsolatedAsyncioTestCase): def _setup(self, bootloader, ptable, **kw): diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 8bce77fb..0557e62a 100644 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -629,7 +629,7 @@ class TestCore(TestAPI): await inst.post("/source", source_id="ubuntu-desktop") resp = await inst.get("/storage/v2/guided", wait=True) [reformat, manual] = resp["targets"] - self.assertIn("CORE_BOOT_PREFER_ENCRYPTED", reformat["allowed"]) + self.assertIn("CORE_BOOT_ENCRYPTED", reformat["allowed"]) data = dict(target=reformat, capability="CORE_BOOT_ENCRYPTED") await inst.post("/storage/v2/guided", data) v2resp = await inst.get("/storage/v2") diff --git a/subiquity/ui/views/filesystem/guided.py b/subiquity/ui/views/filesystem/guided.py index e720c8b4..23799a32 100644 --- a/subiquity/ui/views/filesystem/guided.py +++ b/subiquity/ui/views/filesystem/guided.py @@ -134,16 +134,11 @@ class TPMChoice: help: str -tpm_help_texts = { - "AVAILABLE_CAN_BE_DESELECTED": _( - "The entire disk will be encrypted and protected by the " - "TPM. If this option is deselected, the disk will be " - "unencrypted and without any protection." - ), - "AVAILABLE_CANNOT_BE_DESELECTED": _( +tpm_help_texts_1 = { + GuidedCapability.CORE_BOOT_ENCRYPTED: _( "The entire disk will be encrypted and protected by the TPM." ), - "UNAVAILABLE": + GuidedCapability.CORE_BOOT_UNENCRYPTED: # for translators: 'reason' is the reason FDE is unavailable. _( "TPM backed full-disk encryption is not available " @@ -151,22 +146,11 @@ tpm_help_texts = { ), } -choices = { - GuidedCapability.CORE_BOOT_ENCRYPTED: TPMChoice( - enabled=False, - default=True, - help=tpm_help_texts["AVAILABLE_CANNOT_BE_DESELECTED"], - ), - GuidedCapability.CORE_BOOT_UNENCRYPTED: TPMChoice( - enabled=False, default=False, help=tpm_help_texts["UNAVAILABLE"] - ), - GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED: TPMChoice( - enabled=True, default=True, help=tpm_help_texts["AVAILABLE_CAN_BE_DESELECTED"] - ), - GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED: TPMChoice( - enabled=True, default=False, help=tpm_help_texts["AVAILABLE_CAN_BE_DESELECTED"] - ), -} +tpm_help_texts_can_be_deselected = _( + "The entire disk will be encrypted and protected by the " + "TPM. If this option is deselected, the disk will be " + "unencrypted and without any protection." +) class GuidedChoiceForm(SubForm): @@ -223,16 +207,28 @@ class GuidedChoiceForm(SubForm): self.use_lvm.enabled = GuidedCapability.LVM in val.allowed core_boot_caps = [c for c in val.allowed if c.is_core_boot()] if core_boot_caps: - assert len(val.allowed) == 1 - cap = core_boot_caps[0] + assert len(val.allowed) == len(core_boot_caps) + reason = "" for disallowed in val.disallowed: if disallowed.capability == GuidedCapability.CORE_BOOT_ENCRYPTED: reason = disallowed.message - self.tpm_choice = choices[cap] + + cap0 = core_boot_caps[0] + if len(core_boot_caps) == 1: + self.tpm_choice = TPMChoice( + enabled=False, + default=cap0 == GuidedCapability.CORE_BOOT_ENCRYPTED, + help=tpm_help_texts_1[cap0], + ) + else: + self.tpm_choice = TPMChoice( + enabled=True, + default=cap0 == GuidedCapability.CORE_BOOT_ENCRYPTED, + help=tpm_help_texts_can_be_deselected, + ) self.use_tpm.enabled = self.tpm_choice.enabled self.use_tpm.value = self.tpm_choice.default - self.use_tpm.help = self.tpm_choice.help self.use_tpm.help = self.tpm_choice.help.format(reason=reason) else: self.use_tpm.enabled = False