Finishing UX spec - storing CL options in different config values so we can correctly validate, updating bootstrap to use shared module

4 files changed, 51 insertions, 45 deletions

diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index 36a0fc1e47..6d628f0224 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -17,11 +17,13 @@
 #
 
 require 'chef/knife'
+require 'chef/knife/data_bag_secret_options'
 require 'erubis'
 
 class Chef
   class Knife
     class Bootstrap < Knife
+      include DataBagSecretOptions
 
       deps do
         require 'chef/knife/core/bootstrap_context'
@@ -157,17 +159,6 @@ class Chef
           Chef::Config[:knife][:hints][name] = path ? Chef::JSONCompat.parse(::File.read(path)) : Hash.new
         }
 
-      option :secret,
-        :short => "-s SECRET",
-        :long  => "--secret ",
-        :description => "The secret key to use to encrypt data bag item values",
-        :proc => Proc.new { |s| Chef::Config[:knife][:secret] = s }
-
-      option :secret_file,
-        :long => "--secret-file SECRET_FILE",
-        :description => "A file containing the secret key to use to encrypt data bag item values",
-        :proc => Proc.new { |sf| Chef::Config[:knife][:secret_file] = sf }
-
       option :bootstrap_url,
         :long        => "--bootstrap-url URL",
         :description => "URL to a custom installation script",
@@ -248,7 +239,8 @@ class Chef
       def render_template
         template_file = find_template
         template = IO.read(template_file).chomp
-        context = Knife::Core::BootstrapContext.new(config, config[:run_list], Chef::Config)
+        secret = encryption_secret_provided?(false) ? read_secret : nil
+        context = Knife::Core::BootstrapContext.new(config, config[:run_list], Chef::Config, secret)
         Erubis::Eruby.new(template).evaluate(context)
       end
 
diff --git a/lib/chef/knife/core/bootstrap_context.rb b/lib/chef/knife/core/bootstrap_context.rb
index 03e3a42e4a..e681d7a49b 100644
--- a/lib/chef/knife/core/bootstrap_context.rb
+++ b/lib/chef/knife/core/bootstrap_context.rb
@@ -30,10 +30,11 @@ class Chef
       #
       class BootstrapContext
 
-        def initialize(config, run_list, chef_config)
+        def initialize(config, run_list, chef_config, secret)
           @config       = config
           @run_list     = run_list
           @chef_config  = chef_config
+          @secret       = secret
         end
 
         def bootstrap_environment
@@ -45,15 +46,7 @@ class Chef
         end
 
         def encrypted_data_bag_secret
-          knife_config[:secret] || begin
-            secret_file_path = knife_config[:secret_file]
-            expanded_secret_file_path = File.expand_path(secret_file_path.to_s)
-            if secret_file_path && File.exist?(expanded_secret_file_path)
-              IO.read(expanded_secret_file_path)
-            else
-              nil
-            end
-          end
+          @secret
         end
 
         def trusted_certs
diff --git a/lib/chef/knife/data_bag_secret_options.rb b/lib/chef/knife/data_bag_secret_options.rb
index b692fc767c..238d09667c 100644
--- a/lib/chef/knife/data_bag_secret_options.rb
+++ b/lib/chef/knife/data_bag_secret_options.rb
@@ -26,17 +26,26 @@ class Chef
       include Mixlib::CLI
       include Chef::EncryptedDataBagItem::CheckEncrypted
 
+      # The config object is populated by knife#merge_configs with knife.rb `knife[:*]` config values, but they do
+      # not overwrite the command line properties.  It does mean, however, that `knife[:secret]` and `--secret-file`
+      # passed at the same time populate both `config[:secret]` and `config[:secret_file]`.  We cannot differentiate
+      # the valid case (`knife[:secret]` in config file and `--secret-file` on CL) and the invalid case (`--secret`
+      # and `--secret-file` on the CL) - thats why I'm storing the CL options in a different config key if they
+      # are provided.
+
       def self.included(base)
         base.option :secret,
                :short => "-s SECRET",
                :long  => "--secret ",
                :description => "The secret key to use to encrypt data bag item values.  Can also be defaulted in your config with the key 'secret'",
-               :proc => Proc.new { |s| Chef::Config[:knife][:secret] = s }
+               # Need to store value from command line in separate variable - knife#merge_configs populates same keys
+               # on config object from
+               :proc => Proc.new { |s| set_cl_secret(s) }
 
         base.option :secret_file,
                :long => "--secret-file SECRET_FILE",
                :description => "A file containing the secret key to use to encrypt data bag item values.  Can also be defaulted in your config with the key 'secret_file'",
-               :proc => Proc.new { |sf| Chef::Config[:knife][:secret_file] = sf }
+               :proc => Proc.new { |sf| set_cl_secret_file(sf) }
 
         base.option :encrypt,
                :long => "--encrypt",
@@ -48,18 +57,23 @@ class Chef
       ##
       # Determine if the user has specified an appropriate secret for encrypting data bag items.
       # @returns boolean
-      def encryption_secret_provided?
+      def encryption_secret_provided?(need_encrypt_flag = true)
         validate_secrets
 
-        return true if config[:secret] || config[:secret_file]
+        return true if has_cl_secret? || has_cl_secret_file?
 
-        if config[:encrypt]
-          unless has_secret? || has_secret_file?
-            ui.fatal("No secret or secret_file specified in config, unable to encrypt item.")
-            exit(1)
-          else
+        if need_encrypt_flag
+          if config[:encrypt]
+            unless knife_config[:secret] || knife_config[:secret_file]
+              ui.fatal("No secret or secret_file specified in config, unable to encrypt item.")
+              exit(1)
+            end
             return true
           end
+          return false
+        elsif knife_config[:secret] || knife_config[:secret_file]
+          # Certain situations (show and bootstrap) don't need a --encrypt flag to use the config file secret
+          return true
         end
         return false
       end
@@ -69,39 +83,47 @@ class Chef
         # IE, if we are not running 'knife data bag *' we don't need to load 'chef/encrypted_data_bag_item'
         require 'chef/encrypted_data_bag_item'
 
-        if config[:secret]
+        if has_cl_secret?
           config[:secret]
-        elsif config[:secret_file]
+        elsif has_cl_secret_file?
           Chef::EncryptedDataBagItem.load_secret(config[:secret_file])
-        elsif secret = knife_config[:secret] || Chef::Config[:secret]
+        elsif secret = knife_config[:secret]
           secret
         else
-          secret_file = knife_config[:secret_file] || Chef::Config[:secret_file]
+          secret_file = knife_config[:secret_file]
           Chef::EncryptedDataBagItem.load_secret(secret_file)
         end
       end
 
       def validate_secrets
-        if config[:secret] && config[:secret_file]
+        if has_cl_secret? && has_cl_secret_file?
           ui.fatal("Please specify only one of --secret, --secret-file")
           exit(1)
         end
 
-        if has_secret? && has_secret_file?
-          ui.fatal("Please specify only one of 'secret' or 'secret_file' in your config")
+        if knife_config[:secret] && knife_config[:secret_file]
+          ui.fatal("Please specify only one of 'secret' or 'secret_file' in your config file")
           exit(1)
         end
       end
 
-      def has_secret?
-        knife_config[:secret] || Chef::Config[:secret]
+      private
+
+      def has_cl_secret?
+        Chef::Config[:knife].has_key?(:cl_secret)
       end
 
-      def has_secret_file?
-        knife_config[:secret_file] || Chef::Config[:secret_file]
+      def self.set_cl_secret(s)
+        Chef::Config[:knife][:cl_secret] = s
       end
 
-      private
+      def has_cl_secret_file?
+        Chef::Config[:knife].has_key?(:cl_secret_file)
+      end
+
+      def self.set_cl_secret_file(sf)
+        Chef::Config[:knife][:cl_secret_file] = sf
+      end
 
       def knife_config
         Chef::Config.key?(:knife) ? Chef::Config[:knife] : {}
diff --git a/lib/chef/knife/data_bag_show.rb b/lib/chef/knife/data_bag_show.rb
index 05f831a62d..2f97d36ca3 100644
--- a/lib/chef/knife/data_bag_show.rb
+++ b/lib/chef/knife/data_bag_show.rb
@@ -36,12 +36,11 @@ class Chef
       def run
         display = case @name_args.length
         when 2 # Bag and Item names provided
-          secret = encryption_secret_provided? ? read_secret : nil
+          secret = encryption_secret_provided?(false) ? read_secret : nil
           raw_data = Chef::DataBagItem.load(@name_args[0], @name_args[1]).raw_data
           encrypted = encrypted?(raw_data)
 
           if encrypted && secret
-            # TODO If the decryption fails with provided secret, what does that look like?
             # Users do not need to pass --encrypt to read data, we simply try to use the provided secret
             ui.info("Encrypted data bag detected, decrypting with provided secret.")
             raw = Chef::EncryptedDataBagItem.load(@name_args[0],