Merge pull request #959 from ancorgs/refactor_encrypt

Improve behavior when editing encrypted devices

package/yast2-storage-ng.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Thu Sep  5 15:03:50 UTC 2019 - Ancor Gonzalez Sosa <ancor@suse.com>
+
+- Partitioner: better handling of existing encryptions, including
+  the possibility of reusing them (related to jsc#SLE-7376).
+
 -------------------------------------------------------------------
 Wed Sep 04 14:46:12 CEST 2019 - aschnell@suse.com
 

src/lib/y2partitioner/actions/controllers/encryption.rb

@@ -27,51 +27,102 @@ module Controllers
       # device that has been edited by the given Filesystem controller.
       class Encryption < Base
         include Yast::Logger
+        include Yast::I18n
+
+        # Action to perform when {#finish} is called
+        #
+        # * :keep preserves the encryption layer from the system devicegraph
+        # * :encrypt adds an encryption device or modifies the previously added one
+        # * :remove ensures the block device will not be encrypted
+        #
+        # @return [Symbol] :keep, :encrypt, :remove
+        attr_accessor :action
 
         # @return [String] Password for the encryption device
-        attr_accessor :encrypt_password
+        attr_accessor :password
 
         # Contructor
         #
         # @param fs_controller [Filesystem] see {#fs_controller}
         def initialize(fs_controller)
           super()
+          textdomain "storage"
+
           @fs_controller = fs_controller
+          @password = blk_device.encrypted? ? encryption.password : ""
+          @action = actions.first
         end
 
-        # Whether a new encryption device will be created for the block device
+        # Whether the dialog to select and configure the action makes sense
         #
         # @return [Boolean]
-        def to_be_encrypted?
-          return false unless can_change_encrypt?
+        def show_dialog?
+          can_change_encrypt? && fs_controller.encrypt
+        end
 
-          fs_controller.encrypt && !blk_device.encrypted?
+        # Actions that make sense for the block device
+        #
+        # @see #action
+        #
+        # If there is more than one possible action, the user should be able to
+        # use the UI to select which one to perform
+        #
+        # @return [Array<Symbol>]
+        def actions
+          return @actions if @actions
+
+          @actions =
+            if fs_controller.encrypt
+              if can_keep?
+                [:keep, :encrypt]
+              else
+                [:encrypt]
+              end
+            else
+              [:remove]
+            end
         end
 
         # Applies last changes to the block device at the end of the wizard, which
         # mainly means
         #
         #   * removing unused LvmPv descendant (bsc#1129663)
-        #   * encrypting the device or removing the encryption layer.
+        #   * encrypting the device, modifying the encryption layer or removing it
         def finish
           return unless can_change_encrypt?
 
           remove_unused_lvm_pv
 
-          if to_be_encrypted?
-            blk_device.encrypt(password: encrypt_password)
-          elsif blk_device.encrypted? && !fs_controller.encrypt
-            blk_device.remove_encryption
+          return if action == :keep
+
+          if action == :encrypt
+            finish_encrypt
+          else
+            finish_remove
           end
         ensure
           fs_controller.update_checkpoint
         end
 
-        # Name of the plain device
+        # Encryption device currently associated to the block device, if any
         #
+        # @return [Y2Storage::Encryption, nil]
+        def encryption
+          blk_device.encryption
+        end
+
+        # Title to display in the dialog during the process
         # @return [String]
-        def blk_device_name
-          blk_device.name
+        def wizard_title
+          title =
+            if actions.include?(:keep)
+              _("Encryption for %s")
+            elsif new_encryption?
+              _("Modify encryption of %s")
+            else
+              _("Encrypt %s")
+            end
+          format(title, blk_device.name)
         end
 
         private
@@ -90,18 +141,60 @@ def blk_device
           fs_controller.blk_device
         end
 
+        # Whether it's possible to remove or replace the encryption device
+        # currently associated to the block device
+        #
+        # @return [Boolean] false if the device is formatted in the system and
+        #   the user wants to preserve that filesystem
         def can_change_encrypt?
           blk_device.filesystem.nil? || new?(blk_device.filesystem)
         end
 
         # Removes from the block device or its encryption layer a LvmPv not associated to an LvmVg
         # (bsc#1129663)
         def remove_unused_lvm_pv
-          device = blk_device.encryption || blk_device
+          device = encryption || blk_device
           lvm_pv = device.lvm_pv
 
           device.remove_descendants if lvm_pv&.descendants&.none?
         end
+
+        # @see #finish
+        def finish_remove
+          return unless blk_device.encrypted?
+
+          blk_device.remove_encryption
+        end
+
+        # @see #finish
+        def finish_encrypt
+          if new_encryption?
+            encryption.password = password
+          else
+            blk_device.remove_encryption if blk_device.encrypted?
+            blk_device.encrypt(password: password)
+          end
+        end
+
+        # Whether the block device is associated to an encryption device that
+        # does not exists in the system yet
+        #
+        # In other words, if the device is going to be (re)encrypted
+        #
+        # @return [Boolean]
+        def new_encryption?
+          blk_device.encrypted? && new?(encryption)
+        end
+
+        # Whether it makes sense to offer the :keep action
+        #
+        # @return [Boolean]
+        def can_keep?
+          return false unless blk_device.encrypted?
+          return false if new?(encryption)
+
+          encryption.active?
+        end
       end
     end
   end

src/lib/y2partitioner/actions/controllers/filesystem.rb

@@ -441,17 +441,27 @@ def update_checkpoint
         private
 
         def delete_filesystem
-          blk_device.remove_descendants
+          filesystem_parent.remove_descendants
           # Shadowing control of btrfs subvolumes might be needed if the deleted
           # filesystem had mount point
           Y2Storage::Filesystems::Btrfs.refresh_subvolumes_shadowing(working_graph)
         end
 
         def create_filesystem(type, label: nil)
-          blk_device.create_blk_filesystem(type)
+          filesystem_parent.create_blk_filesystem(type)
           filesystem.label = label unless label.nil?
         end
 
+        # Device containing the filesystem
+        #
+        # Unlike {#blk_device}, that always returns the plain device, this
+        # method will return the encryption device for encrypted filesystems
+        #
+        # @return [Y2Storage::BlkDevice]
+        def filesystem_parent
+          blk_device.encrypted? ? blk_device.encryption : blk_device
+        end
+
         def restore_filesystem
           mount_path = filesystem.mount_path
           mount_by = filesystem.mount_by

src/lib/y2partitioner/actions/filesystem_steps.rb

@@ -71,7 +71,7 @@ def format_options
       def encrypt_password
         @encrypt_controller = Controllers::Encryption.new(fs_controller)
 
-        return :next unless encrypt_controller.to_be_encrypted?
+        return :next unless encrypt_controller.show_dialog?
 
         Dialogs::Encryption.run(encrypt_controller)
       end

src/lib/y2partitioner/dialogs/encryption.rb

@@ -20,10 +20,12 @@
 require "yast"
 require "y2partitioner/dialogs/base"
 require "y2partitioner/widgets/encrypt_password"
+require "y2partitioner/widgets/controller_radio_buttons"
 
 module Y2Partitioner
   module Dialogs
-    # Ask for a password to assign to an encrypted device.
+    # Ask for the concrete action to perform when the goal is to obtain an
+    # encrypted device, including details like the password.
     # Part of {Actions::AddPartition} and {Actions::EditBlkDevice}.
     # Formerly MiniWorkflowStepPassword
     class Encryption < Base
@@ -34,14 +36,87 @@ def initialize(controller)
         @controller = controller
       end
 
+      # @macro seeDialog
       def title
-        _("Encryption password for %s") % @controller.blk_device_name
+        @controller.wizard_title
       end
 
+      # @macro seeDialog
       def contents
-        HVSquash(
-          Widgets::EncryptPassword.new(@controller)
-        )
+        main_widget =
+          if @controller.actions.include?(:keep)
+            ActionWidget.new(@controller)
+          else
+            Widgets::EncryptPassword.new(@controller)
+          end
+
+        HVSquash(main_widget)
+      end
+
+      # Internal widget used when both :keep and :encrypt options are possible
+      class ActionWidget < Widgets::ControllerRadioButtons
+        # @param controller [Actions::Controllers::Encryption]
+        #   a controller collecting data for managing the encryption of a device
+        def initialize(controller)
+          textdomain "storage"
+          @controller = controller
+        end
+
+        # @macro seeAbstractWidget
+        def label
+          _("Choose an Action")
+        end
+
+        # @macro seeItemsSelection
+        def items
+          keep_label =
+            format(_("Preserve existing encryption (%s)"), encryption_type.to_human_string)
+
+          [
+            [:keep, keep_label],
+            [:encrypt, _("Encrypt the device (replaces current encryption)")]
+          ]
+        end
+
+        # @see Widgets::ControllerRadioButtons
+        def widgets
+          @widgets ||= [
+            CWM::Empty.new("empty"),
+            Widgets::EncryptPassword.new(@controller)
+          ]
+        end
+
+        # @macro seeAbstractWidget
+        def init
+          self.value = @controller.action
+          # trigger disabling the other subwidgets
+          handle("ID" => value)
+        end
+
+        # @macro seeAbstractWidget
+        def store
+          current_widget.store if current_widget.respond_to?(:store)
+          @controller.action = value
+        end
+
+        # @macro seeAbstractWidget
+        def help
+          # helptext
+          _(
+            "<p>Choose the encryption layer.</p>\n" \
+            "<p>The device is already encrypted in the system, it's possible to re-encrypt it" \
+            "with new settings or to use the existing encryption layer.</p>"
+          )
+        end
+
+        private
+
+        # Type of the encryption currently used by the device
+        #
+        # @return [Y2Storage::EncryptionType]
+        def encryption_type
+          @controller.encryption.type
+        end
       end
     end
   end

src/lib/y2partitioner/widgets/encrypt_password.rb

@@ -31,7 +31,7 @@ def help
 
       # @macro seeAbstractWidget
       def validate
-        msg = checker.error_msg(pw1, pw2)
+        msg = checker.error_msg(pw1, pw2) if enabled?
         return true unless msg
 
         Yast::Report.Error(msg)
@@ -41,7 +41,7 @@ def validate
 
       # @macro seeAbstractWidget
       def store
-        @controller.encrypt_password = pw1
+        @controller.password = pw1
       end
 
       # @macro seeAbstractWidget
@@ -51,30 +51,19 @@ def cleanup
 
       # @macro seeCustomWidget
       def contents
-        Frame(
-          _("Encryption Password"),
-          MarginBox(
-            1.45,
-            0.5,
-            VBox(
-              Password(
-                Id(:pw1),
-                Opt(:hstretch),
-                # Label: get password for user root
-                # Please use newline if label is longer than 40 characters
-                _("&Enter a Password for your File System:"),
-                ""
-              ),
-              Password(
-                Id(:pw2),
-                Opt(:hstretch),
-                # Label: get same password again for verification
-                # Please use newline if label is longer than 40 characters
-                _("Reenter the Password for &Verification:"),
-                ""
-              ),
-              VSpacing(0.5)
-            )
+        VBox(
+          Id(widget_id),
+          Password(
+            Id(:pw1),
+            Opt(:hstretch),
+            _("&Enter an Encryption Password:"),
+            @controller.password
+          ),
+          Password(
+            Id(:pw2),
+            Opt(:hstretch),
+            _("Reenter the Password for &Verification:"),
+            @controller.password
           )
         )
       end