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

8 files changed, 110 insertions, 152 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],
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index a1c5d168dd..1b1bf3a699 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -30,6 +30,7 @@ describe Chef::Knife::Bootstrap do
     k.merge_configs
 
     k.ui.stub(:stderr).and_return(stderr)
+    allow(k).to receive(:encryption_secret_provided?).with(false).and_return(false)
     k
   end
 
@@ -221,10 +222,6 @@ describe Chef::Knife::Bootstrap do
       k
     end
 
-    # Include a data bag secret in the options to prevent Bootstrap from
-    # attempting to access /etc/chef/encrypted_data_bag_secret, which
-    # can fail when the file exists but can't be accessed by the user
-    # running the tests.
     let(:options){ ["--bootstrap-no-proxy", setting, "-s", "foo"] }
     let(:template_file) { File.expand_path(File.join(CHEF_SPEC_DATA, "bootstrap", "no_proxy.erb")) }
     let(:rendered_template) do
@@ -290,7 +287,6 @@ describe Chef::Knife::Bootstrap do
 
   describe "specifying the encrypted data bag secret key" do
     let(:secret) { "supersekret" }
-    let(:secret_file) { File.join(CHEF_SPEC_DATA, 'bootstrap', 'encrypted_data_bag_secret') }
     let(:options) { [] }
     let(:bootstrap_template) { File.expand_path(File.join(CHEF_SPEC_DATA, "bootstrap", "secret.erb")) }
     let(:rendered_template) do
@@ -299,60 +295,18 @@ describe Chef::Knife::Bootstrap do
       knife.render_template
     end
 
-    context "via --secret" do
-      let(:options){ ["--secret", secret] }
-
-      it "creates a secret file" do
-        rendered_template.should match(%r{#{secret}})
-      end
-
-      it "renders the client.rb with an encrypted_data_bag_secret entry" do
-        rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"})
-      end
-    end
-
-    context "via --secret-file" do
-      let(:options) { ["--secret-file", secret_file] }
-      let(:secret) { IO.read(secret_file) }
-
-      it "creates a secret file" do
-        rendered_template.should match(%r{#{secret}})
-      end
-
-      it "renders the client.rb with an encrypted_data_bag_secret entry" do
-        rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"})
-      end
+    it "creates a secret file" do
+      expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(true)
+      expect(knife).to receive(:read_secret).and_return(secret)
+      rendered_template.should match(%r{#{secret}})
     end
 
-    context "secret via config" do
-      before do
-        Chef::Config[:knife][:secret] = secret
-      end
-
-      it "creates a secret file" do
-        rendered_template.should match(%r{#{secret}})
-      end
-
-      it "renders the client.rb with an encrypted_data_bag_secret entry" do
-        rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"})
-      end
+    it "renders the client.rb with an encrypted_data_bag_secret entry" do
+      expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(true)
+      expect(knife).to receive(:read_secret).and_return(secret)
+      rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"})
     end
 
-    context "secret-file via config" do
-      let(:secret) { IO.read(secret_file) }
-
-      before do
-        Chef::Config[:knife][:secret_file] = secret_file
-      end
-
-      it "creates a secret file" do
-        rendered_template.should match(%r{#{secret}})
-      end
-
-      it "renders the client.rb with an encrypted_data_bag_secret entry" do
-        rendered_template.should match(%r{encrypted_data_bag_secret\s*"/etc/chef/encrypted_data_bag_secret"})
-      end
-    end
   end
 
   describe "when transferring trusted certificates" do
diff --git a/spec/unit/knife/core/bootstrap_context_spec.rb b/spec/unit/knife/core/bootstrap_context_spec.rb
index 6427071a6b..266991a7dd 100644
--- a/spec/unit/knife/core/bootstrap_context_spec.rb
+++ b/spec/unit/knife/core/bootstrap_context_spec.rb
@@ -29,9 +29,10 @@ describe Chef::Knife::Core::BootstrapContext do
       :validation_client_name => 'chef-validator-testing'
     }
   end
-  let(:secret_file) { File.join(CHEF_SPEC_DATA, 'bootstrap', 'encrypted_data_bag_secret') }
 
-  subject(:bootstrap_context) { described_class.new(config, run_list, chef_config) }
+  let(:secret) { nil }
+
+  subject(:bootstrap_context) { described_class.new(config, run_list, chef_config, secret) }
 
   it "runs chef with the first-boot.json in the _default environment" do
     bootstrap_context.start_chef.should eq "chef-client -j /etc/chef/first-boot.json -E _default"
@@ -105,39 +106,9 @@ EXPECTED
   end
 
   describe "when an encrypted_data_bag_secret is provided" do
-    context "via config[:secret]" do
-      let(:chef_config) do
-        {
-          :knife => {:secret => "supersekret" }
-        }
-      end
-      it "reads the encrypted_data_bag_secret" do
-        bootstrap_context.encrypted_data_bag_secret.should eq "supersekret"
-      end
-    end
-
-    context "via config[:secret_file]" do
-      let(:chef_config) do
-        {
-          :knife => {:secret_file => secret_file}
-        }
-      end
-      it "reads the encrypted_data_bag_secret" do
-        bootstrap_context.encrypted_data_bag_secret.should eq IO.read(secret_file)
-      end
-    end
-
-    context "via config[:secret_file] with short home path" do
-      let(:chef_config) do
-        home_path = File.expand_path("~")
-        shorted_secret_file_path = secret_file.gsub(home_path, "~")
-        {
-          :knife => {:secret_file => shorted_secret_file_path}
-        }
-      end
-      it "reads the encrypted_data_bag_secret" do
-        bootstrap_context.encrypted_data_bag_secret.should eq IO.read(secret_file)
-      end
+    let(:secret) { "supersekret" }
+    it "reads the encrypted_data_bag_secret" do
+      bootstrap_context.encrypted_data_bag_secret.should eq "supersekret"
     end
   end
 
diff --git a/spec/unit/knife/data_bag_secret_options_spec.rb b/spec/unit/knife/data_bag_secret_options_spec.rb
index 98e876a5db..b45a95b73a 100644
--- a/spec/unit/knife/data_bag_secret_options_spec.rb
+++ b/spec/unit/knife/data_bag_secret_options_spec.rb
@@ -50,7 +50,8 @@ describe Chef::Knife::DataBagSecretOptions do
   describe "#validate_secrets" do
 
     it "throws an error when provided with both --secret and --secret-file on the CL" do
-      expect(example_db).to receive(:config).exactly(2).times.and_return({ :secret_file => secret_file.path, :secret => secret })
+      Chef::Config[:knife][:cl_secret_file] = secret_file.path
+      Chef::Config[:knife][:cl_secret] = secret
       expect(example_db).to receive(:exit).with(1)
       expect(example_db.ui).to receive(:fatal).with("Please specify only one of --secret, --secret-file")
 
@@ -61,7 +62,7 @@ describe Chef::Knife::DataBagSecretOptions do
       Chef::Config[:knife][:secret_file] = secret_file.path
       Chef::Config[:knife][:secret] = secret
       expect(example_db).to receive(:exit).with(1)
-      expect(example_db.ui).to receive(:fatal).with("Please specify only one of 'secret' or 'secret_file' in your config")
+      expect(example_db.ui).to receive(:fatal).with("Please specify only one of 'secret' or 'secret_file' in your config file")
 
       example_db.validate_secrets
     end
@@ -71,25 +72,25 @@ describe Chef::Knife::DataBagSecretOptions do
   describe "#read_secret" do
 
     it "returns the secret first" do
-      expect(example_db).to receive(:config).exactly(2).times.and_return({ :secret_file => secret_file.path, :secret => secret })
+      Chef::Config[:knife][:cl_secret] = secret
+      expect(example_db).to receive(:config).and_return({ :secret => secret })
       expect(example_db.read_secret).to eq(secret)
     end
 
     it "returns the secret_file only if secret does not exist" do
-      expect(example_db).to receive(:config).exactly(3).times.and_return({ :secret_file => secret_file.path })
+      Chef::Config[:knife][:cl_secret_file] = secret_file.path
+      expect(example_db).to receive(:config).and_return({ :secret_file => secret_file.path })
       expect(Chef::EncryptedDataBagItem).to receive(:load_secret).with(secret_file.path).and_return("secret file contents")
       expect(example_db.read_secret).to eq("secret file contents")
     end
 
     it "returns the secret from the knife.rb config" do
-      expect(example_db).to receive(:config).exactly(2).times.and_return({})
       Chef::Config[:knife][:secret_file] = secret_file.path
       Chef::Config[:knife][:secret] = secret
       expect(example_db.read_secret).to eq(secret)
     end
 
     it "returns the secret_file from the knife.rb config only if the secret does not exist" do
-      expect(example_db).to receive(:config).exactly(2).times.and_return({})
       Chef::Config[:knife][:secret_file] = secret_file.path
       expect(Chef::EncryptedDataBagItem).to receive(:load_secret).with(secret_file.path).and_return("secret file contents")
       expect(example_db.read_secret).to eq("secret file contents")
@@ -100,39 +101,65 @@ describe Chef::Knife::DataBagSecretOptions do
   describe "#encryption_secret_provided?" do
 
     it "returns true if the secret is passed on the CL" do
-      expect(example_db).to receive(:config).exactly(3).times.and_return({ :secret => secret })
+      Chef::Config[:knife][:cl_secret] = secret
       expect(example_db.encryption_secret_provided?).to eq(true)
     end
 
     it "returns true if the secret_file is passed on the CL" do
-      expect(example_db).to receive(:config).exactly(3).times.and_return({ :secret_file => secret_file.path })
+      Chef::Config[:knife][:cl_secret_file] = secret_file.path
       expect(example_db.encryption_secret_provided?).to eq(true)
     end
 
-    it "returns true if --encrypt is passed on the CL and :secret is in knife.rb" do
-      expect(example_db).to receive(:config).exactly(4).times.and_return({ :encrypt => true })
+    it "returns true if --encrypt is passed on the CL and :secret is in config" do
+      expect(example_db).to receive(:config).and_return({ :encrypt => true })
       Chef::Config[:knife][:secret] = secret
       expect(example_db.encryption_secret_provided?).to eq(true)
     end
 
-    it "returns true if --encrypt is passed on the CL and :secret_file is in knife.rb" do
-      expect(example_db).to receive(:config).exactly(4).times.and_return({ :encrypt => true })
+    it "returns true if --encrypt is passed on the CL and :secret_file is in config" do
+      expect(example_db).to receive(:config).and_return({ :encrypt => true })
       Chef::Config[:knife][:secret_file] = secret_file.path
       expect(example_db.encryption_secret_provided?).to eq(true)
     end
 
-    it "throws an error if --encrypt is passed and there is not :secret or :secret_file in the knife.rb" do
-      expect(example_db).to receive(:config).exactly(4).times.and_return({ :encrypt => true })
+    it "throws an error if --encrypt is passed and there is not :secret or :secret_file in the config" do
+      expect(example_db).to receive(:config).and_return({ :encrypt => true })
       expect(example_db).to receive(:exit).with(1)
       expect(example_db.ui).to receive(:fatal).with("No secret or secret_file specified in config, unable to encrypt item.")
       example_db.encryption_secret_provided?
     end
 
     it "returns false if no secret is passed" do
-      expect(example_db).to receive(:config).exactly(4).times.and_return({})
+      expect(example_db).to receive(:config).and_return({})
       expect(example_db.encryption_secret_provided?).to eq(false)
     end
 
+    it "returns false if --encrypt is not provided and :secret is in the config" do
+      expect(example_db).to receive(:config).and_return({})
+      Chef::Config[:knife][:secret] = secret
+      expect(example_db.encryption_secret_provided?).to eq(false)
+    end
+
+    it "returns false if --encrypt is not provided and :secret_file is in the config" do
+      expect(example_db).to receive(:config).and_return({})
+      Chef::Config[:knife][:secret_file] = secret_file.path
+      expect(example_db.encryption_secret_provided?).to eq(false)
+    end
+
+    it "returns true if --encrypt is not provided, :secret is in the config and need_encrypt_flag is false" do
+      Chef::Config[:knife][:secret] = secret
+      expect(example_db.encryption_secret_provided?(false)).to eq(true)
+    end
+
+    it "returns true if --encrypt is not provided, :secret_file is in the config and need_encrypt_flag is false" do
+      Chef::Config[:knife][:secret_file] = secret_file.path
+      expect(example_db.encryption_secret_provided?(false)).to eq(true)
+    end
+
+    it "returns false if --encrypt is not provided and need_encrypt_flag is false" do
+      expect(example_db.encryption_secret_provided?(false)).to eq(false)
+    end
+
   end
 
 end
diff --git a/spec/unit/knife/data_bag_show_spec.rb b/spec/unit/knife/data_bag_show_spec.rb
index 49632a2911..47778bdf15 100644
--- a/spec/unit/knife/data_bag_show_spec.rb
+++ b/spec/unit/knife/data_bag_show_spec.rb
@@ -64,7 +64,7 @@ describe Chef::Knife::DataBagShow do
     end
 
     it "decrypts and displays the encrypted data bag when the secret is provided" do
-      expect(knife).to receive(:encryption_secret_provided?).and_return(true)
+      expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(true)
       expect(knife).to receive(:read_secret).and_return(secret)
       expect(Chef::DataBagItem).to receive(:load).with(bag_name, item_name).and_return(data_bag_with_encoded_hash)
       expect(knife.ui).to receive(:info).with("Encrypted data bag detected, decrypting with provided secret.")
@@ -78,7 +78,7 @@ qux: http://localhost:4000/data/bag_o_data/qux|
     end
 
     it "displays the encrypted data bag when the secret is not provided" do
-      expect(knife).to receive(:encryption_secret_provided?).and_return(false)
+      expect(knife).to receive(:encryption_secret_provided?).with(false).and_return(false)
       expect(Chef::DataBagItem).to receive(:load).with(bag_name, item_name).and_return(data_bag_with_encoded_hash)
       expect(knife.ui).to receive(:warn).with("Encrypted data bag detected, but no secret provided for decoding.  Displaying encrypted data.")