Finishing spec work for data bag UX (https://gist.github.com/sersut/94c8daad5c11369bd2e8). Tests up next, breaking into multiple commits to keep the review smaller.

5 files changed, 174 insertions, 178 deletions

diff --git a/lib/chef/knife/data_bag_common.rb b/lib/chef/knife/data_bag_common.rb
new file mode 100644
index 0000000000..916989cbb4
--- /dev/null
+++ b/lib/chef/knife/data_bag_common.rb
@@ -0,0 +1,137 @@
+#
+# Author:: Tyler Ball (<tball@opscode.com>)
+# Copyright:: Copyright (c) 2014 Opscode, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'mixlib/cli'
+# TODO these were in a `deps` call before - okay that they aren't anymore?
+require 'chef/config'
+#require 'chef/data_bag'
+require 'chef/encrypted_data_bag_item'
+
+class Chef
+  class Knife
+    module DataBagSecretOptions
+      include Mixlib::CLI
+
+      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 :encrypt,
+             :long => "--encrypt",
+             :description => "Only use the secret configured in knife.rb when this is true",
+             :boolean => true,
+             :default => false
+
+      ##
+      # Determine if the user has specified an appropriate secret for encrypting data bag items.
+      # @returns boolean
+      def encryption_secret_provided?
+        validate_secrets
+
+        return true if config[:secret] || config[: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
+            return true
+          end
+        end
+        return false
+      end
+
+      def read_secret
+        if config[:secret]
+          config[:secret]
+        elsif config[:secret_file]
+          Chef::EncryptedDataBagItem.load_secret(config[:secret_file])
+        elsif secret = knife_config[:secret] || Chef::Config[:secret]
+          secret
+        else
+          secret_file = knife_config[:secret_file] || Chef::Config[:secret_file]
+          Chef::EncryptedDataBagItem.load_secret(secret_file)
+        end
+      end
+
+      def validate_secrets
+        if config[:secret] && config[:secret_file]
+          ui.fatal("Please specify only one of --secret, --secret-file")
+          exit(1)
+        end
+
+        # TODO is there validation on the knife.rb schema?  If so, this validation should go there
+        if has_secret? && has_secret_file?
+          ui.fatal("Please specify only one of 'secret' or 'secret_file' in your config")
+          exit(1)
+        end
+      end
+
+      def has_secret?
+        knife_config[:secret] || Chef::Config[:secret]
+      end
+
+      def has_secret_file?
+        knife_config[:secret_file] || Chef::Config[:secret_file]
+      end
+
+      # TODO duplicated from data_query.rb
+      # Tries to autodetect if the item's raw hash appears to be encrypted.
+      def encrypted?(raw_data)
+        data = raw_data.reject { |k, _| k == "id" } # Remove the "id" key.
+        # Assume hashes containing only the "id" key are not encrypted.
+        # Otherwise, remove the keys that don't appear to be encrypted and compare
+        # the result with the hash. If some entry has been removed, then some entry
+        # doesn't appear to be encrypted and we assume the entire hash is not encrypted.
+        data.empty? ? false : data.reject { |_, v| !looks_like_encrypted?(v) } == data
+      end
+
+      private
+
+      def knife_config
+        Chef::Config.key?(:knife) ? Chef::Config[:knife] : {}
+      end
+
+      # Checks if data looks like it has been encrypted by
+      # Chef::EncryptedDataBagItem::Encryptor::VersionXEncryptor. Returns
+      # true only when there is an exact match between the VersionXEncryptor
+      # keys and the hash's keys.
+      def looks_like_encrypted?(data)
+        return false unless data.is_a?(Hash) && data.has_key?("version")
+        case data["version"]
+          when 1
+            Chef::EncryptedDataBagItem::Encryptor::Version1Encryptor.encryptor_keys.sort == data.keys.sort
+          when 2
+            Chef::EncryptedDataBagItem::Encryptor::Version2Encryptor.encryptor_keys.sort == data.keys.sort
+          when 3
+            Chef::EncryptedDataBagItem::Encryptor::Version3Encryptor.encryptor_keys.sort == data.keys.sort
+          else
+            false # version means something else... assume not encrypted.
+        end
+      end
+
+    end
+  end
+end
diff --git a/lib/chef/knife/data_bag_create.rb b/lib/chef/knife/data_bag_create.rb
index d54d047db4..048d34f543 100644
--- a/lib/chef/knife/data_bag_create.rb
+++ b/lib/chef/knife/data_bag_create.rb
@@ -22,7 +22,9 @@ require 'chef/knife'
 class Chef
   class Knife
     class DataBagCreate < Knife
+      include DataBagSecretOptions
 
+      # TODO duplicating deps here and in the DataBagSecretOptions module
       deps do
         require 'chef/data_bag'
         require 'chef/encrypted_data_bag_item'
@@ -31,73 +33,6 @@ class Chef
       banner "knife data bag create BAG [ITEM] (options)"
       category "data bag"
 
-      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 :encrypt,
-        :long => "--encrypt",
-        :description => "Only encrypt data bag when specified.",
-        :boolean => true,
-        :default => false
-
-      def read_secret
-        if config[:secret]
-          config[:secret]
-        elsif config[:secret_file]
-          Chef::EncryptedDataBagItem.load_secret(config[:secret_file])
-        elsif secret = knife_config[:secret] || Chef::Config[:secret]
-          secret
-        else
-          secret_file = knife_config[:secret_file] || Chef::Config[:secret_file]
-          Chef::EncryptedDataBagItem.load_secret(secret_file)
-        end
-      end
-
-      def knife_config
-        Chef::Config.key?(:knife) ? Chef::Config[:knife] : {}
-      end
-
-      def has_secret?
-        knife_config[:secret] || Chef::Config[:secret]
-      end
-
-      def has_secret_file?
-        knife_config[:secret_file] || Chef::Config[:secret_file]
-      end
-
-      def use_encryption
-        # Ensure only one of --secret and --secret-file has been given.
-        if config[:secret] && config[:secret_file]
-          ui.fatal("Please specify only one of --secret, --secret-file")
-          exit(1)
-        end
-
-        # TODO is there validation on the config schema?  If so, this validation should go there
-        if has_secret? && has_secret_file?
-          ui.fatal("Please specify only one of 'secret' or 'secret_file' in your config")
-          exit(1)
-        end
-
-        return true if config[:secret] || config[: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
-            return true
-          end
-        end
-        return false
-      end
-
       def run
         @data_bag_name, @data_bag_item_name = @name_args
 
@@ -127,7 +62,7 @@ class Chef
         if @data_bag_item_name
           create_object({ "id" => @data_bag_item_name }, "data_bag_item[#{@data_bag_item_name}]") do |output|
             item = Chef::DataBagItem.from_hash(
-              if use_encryption
+              if encryption_secret_provided?
                 Chef::EncryptedDataBagItem.encrypt_data_bag_item(output, read_secret)
               else
                 output
diff --git a/lib/chef/knife/data_bag_edit.rb b/lib/chef/knife/data_bag_edit.rb
index 2486edd5dd..13d51daee0 100644
--- a/lib/chef/knife/data_bag_edit.rb
+++ b/lib/chef/knife/data_bag_edit.rb
@@ -22,6 +22,7 @@ require 'chef/knife'
 class Chef
   class Knife
     class DataBagEdit < Knife
+      include DataBagSecretOptions
 
       deps do
         require 'chef/data_bag_item'
@@ -31,46 +32,15 @@ class Chef
       banner "knife data bag edit BAG ITEM (options)"
       category "data bag"
 
-      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 :encrypted,
-        :long => "--encrypted",
-        :description => "Only encrypt data bag when specified.",
-        :proc => Proc.new { |e| Chef::Config[:knife][:encrypted] = e }
-
-      def read_secret
-        if config[:secret]
-          config[:secret]
-        else
-          Chef::EncryptedDataBagItem.load_secret(config[:secret_file])
-        end
-      end
-
-      def use_encryption
-        if config[:encrypted]
-          if config[:secret] && config[:secret_file]
-            ui.fatal("please specify only one of --secret, --secret-file")
-            exit(1)
-          end
-          config[:secret] || config[:secret_file]
-        else
-          false
-        end
-      end
-
       def load_item(bag, item_name)
         item = Chef::DataBagItem.load(bag, item_name)
-        if use_encryption
-          Chef::EncryptedDataBagItem.new(item, read_secret).to_hash
+        if encrypted?(item.raw_data)
+          if encryption_secret_provided?
+            Chef::EncryptedDataBagItem.new(item, read_secret).to_hash
+          else
+            ui.fatal("You cannot edit an encrypted data bag without providing the secret.")
+            exit(1)
+          end
         else
           item
         end
@@ -78,9 +48,11 @@ class Chef
 
       def edit_item(item)
         output = edit_data(item)
-        if use_encryption
+        if encryption_secret_provided?
+          ui.info("Encrypting data bag using provided secret.")
           Chef::EncryptedDataBagItem.encrypt_data_bag_item(output, read_secret)
         else
+          ui.info("Saving data bag unencrypted.  To encrypt it, provide an appropriate secret.")
           output
         end
       end
@@ -95,6 +67,7 @@ class Chef
         output = edit_item(item)
         rest.put_rest("data/#{@name_args[0]}/#{@name_args[1]}", output)
         stdout.puts("Saved data_bag_item[#{@name_args[1]}]")
+        # TODO this is trying to read :print_after from the CLI, not the knife.rb
         ui.output(output) if config[:print_after]
       end
     end
diff --git a/lib/chef/knife/data_bag_from_file.rb b/lib/chef/knife/data_bag_from_file.rb
index 2ff79b6256..4e61f1b53f 100644
--- a/lib/chef/knife/data_bag_from_file.rb
+++ b/lib/chef/knife/data_bag_from_file.rb
@@ -23,6 +23,7 @@ require 'chef/util/path_helper'
 class Chef
   class Knife
     class DataBagFromFile < Knife
+      include DataBagSecretOptions
 
       deps do
         require 'chef/data_bag'
@@ -35,38 +36,11 @@ class Chef
       banner "knife data bag from file BAG FILE|FOLDER [FILE|FOLDER..] (options)"
       category "data bag"
 
-      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 :all,
         :short => "-a",
         :long  => "--all",
         :description => "Upload all data bags or all items for specified data bags"
 
-      def read_secret
-        if config[:secret]
-          config[:secret]
-        else
-          Chef::EncryptedDataBagItem.load_secret(config[:secret_file])
-        end
-      end
-
-      def use_encryption
-        if config[:secret] && config[:secret_file]
-          ui.fatal("please specify only one of --secret, --secret-file")
-          exit(1)
-        end
-        config[:secret] || config[:secret_file]
-      end
-
       def loader
         @loader ||= Knife::Core::ObjectLoader.new(DataBagItem, ui)
       end
@@ -109,9 +83,8 @@ class Chef
         item_paths = normalize_item_paths(items)
         item_paths.each do |item_path|
           item = loader.load_from("#{data_bags_path}", data_bag, item_path)
-          item = if use_encryption
-                   secret = read_secret
-                   Chef::EncryptedDataBagItem.encrypt_data_bag_item(item, secret)
+          item = if encryption_secret_provided?
+                   Chef::EncryptedDataBagItem.encrypt_data_bag_item(item, read_secret)
                  else
                    item
                  end
diff --git a/lib/chef/knife/data_bag_show.rb b/lib/chef/knife/data_bag_show.rb
index c236bea53b..0392d417cc 100644
--- a/lib/chef/knife/data_bag_show.rb
+++ b/lib/chef/knife/data_bag_show.rb
@@ -22,6 +22,7 @@ require 'chef/knife'
 class Chef
   class Knife
     class DataBagShow < Knife
+      include DataBagSecretOptions
 
       deps do
         require 'chef/data_bag'
@@ -31,54 +32,30 @@ class Chef
       banner "knife data bag show BAG [ITEM] (options)"
       category "data bag"
 
-      option :secret,
-        :short => "-s SECRET",
-        :long  => "--secret ",
-        :description => "The secret key to use to decrypt 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 decrypt data bag item values",
-        :proc => Proc.new { |sf| Chef::Config[:knife][:secret_file] = sf }
-
-      option :encrypted,
-        :long => "--encrypted",
-        :description => "Only encrypt data bag when specified.",
-        :proc => Proc.new { |e| Chef::Config[:knife][:encrypted] = e }
-
-      def read_secret
-        if config[:secret]
-          config[:secret]
-        else
-          Chef::EncryptedDataBagItem.load_secret(config[:secret_file])
-        end
-      end
-
-      def use_encryption
-        if config[:encrypted]
-          if config[:secret] && config[:secret_file]
-            ui.fatal("please specify only one of --secret, --secret-file")
-            exit(1)
-          end
-          config[:secret] || config[:secret_file]
-        else
-          false
-        end
-      end
-
       def run
         display = case @name_args.length
-        when 2
-          if use_encryption
+        when 2 # Bag and Item names provided
+          secret = read_secret
+          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],
                                                   @name_args[1],
-                                                  read_secret)
+                                                  secret)
             format_for_display(raw.to_hash)
+          elsif encrypted && !secret
+            ui.warn("Encrypted data bag detected, but no secret provided for decoding.  Displaying encrypted data.")
+            format_for_display(raw_data)
           else
-            format_for_display(Chef::DataBagItem.load(@name_args[0], @name_args[1]).raw_data)
+            ui.info("Unencrypted data bag detected, ignoring any provided secret options.")
+            format_for_display(raw_data)
           end
-        when 1
+
+        when 1 # Only Bag name provided
           format_list_for_display(Chef::DataBag.load(@name_args[0]))
         else
           stdout.puts opt_parser
@@ -86,6 +63,7 @@ class Chef
         end
         output(display)
       end
+
     end
   end
 end