Merge pull request #11986 from jasonwbarnett/bugfix/knife-client-create

Fix knife client create

3 files changed, 98 insertions, 53 deletions

diff --git a/knife/lib/chef/knife/client_create.rb b/knife/lib/chef/knife/client_create.rb
index 9c17174322..12cf0eed02 100644
--- a/knife/lib/chef/knife/client_create.rb
+++ b/knife/lib/chef/knife/client_create.rb
@@ -54,6 +54,10 @@ class Chef
         @client_field ||= Chef::ApiClientV1.new
       end
 
+      def file
+        config[:file]
+      end
+
       def create_client(client)
         # should not be using save :( bad behavior
         Chef::ApiClientV1.from_hash(client).save
@@ -81,13 +85,7 @@ class Chef
           client.public_key File.read(File.expand_path(config[:public_key]))
         end
 
-        # Check the file before creating the client so the api is more transactional.
-        if config[:file]
-          file = config[:file]
-          dir_name = File.dirname(file)
-          check_writable_or_exists(dir_name, "Directory")
-          check_writable_or_exists(file, "File")
-        end
+        file_is_writable!
 
         output = edit_hash(client)
         final_client = create_client(output)
@@ -105,15 +103,35 @@ class Chef
         end
       end
 
-      # To check if file or directory exists or writable and raise exception accordingly
-      def check_writable_or_exists(file, type)
-        if File.exist?(file)
-          unless File.writable?(file)
-            ui.fatal "#{type} #{file} is not writable. Check permissions."
-            exit 1
-          end
-        else
-          ui.fatal "#{type} #{file} does not exist."
+      #
+      # This method is used to verify that the file and it's containing
+      # directory are writable.  This ensures that you don't create the client
+      # and then lose the private key because you weren't able to write it to
+      # disk.
+      #
+      # @return [void]
+      #
+      def file_is_writable!
+        return unless file
+
+        dir = File.dirname(File.expand_path(file))
+        unless File.exist?(dir)
+          ui.fatal "Directory #{dir} does not exist. Please create and retry."
+          exit 1
+        end
+
+        unless File.directory?(dir)
+          ui.fatal "#{dir} exists, but is not a directory. Please update your file path (--file #{file}) or re-create #{dir} as a directory."
+          exit 1
+        end
+
+        unless File.writable?(dir)
+          ui.fatal "Directory #{dir} is not writable. Please check the permissions."
+          exit 1
+        end
+
+        if File.exist?(file) && !File.writable?(file)
+          ui.fatal "File #{file} is not writable. Please check the permissions."
           exit 1
         end
       end
diff --git a/knife/spec/integration/client_create_spec.rb b/knife/spec/integration/client_create_spec.rb
index 337582d858..3898ff9d24 100644
--- a/knife/spec/integration/client_create_spec.rb
+++ b/knife/spec/integration/client_create_spec.rb
@@ -50,7 +50,6 @@ describe "knife client create", :workstation do
 
     it "saves the private key to a file" do
       Dir.mktmpdir do |tgt|
-        File.open("#{tgt}/bah.pem", "w") { |pub| pub.write("test key") }
         knife("client create -f #{tgt}/bah.pem bah").should_succeed stderr: out
         expect(File).to exist("#{tgt}/bah.pem")
       end
diff --git a/knife/spec/unit/knife/client_create_spec.rb b/knife/spec/unit/knife/client_create_spec.rb
index ada1d9d7bc..55122f8c01 100644
--- a/knife/spec/unit/knife/client_create_spec.rb
+++ b/knife/spec/unit/knife/client_create_spec.rb
@@ -54,6 +54,19 @@ describe Chef::Knife::ClientCreate do
     Chef::Config[:node_name] = "webmonkey.example.com"
   end
 
+  let(:tmpdir) { Dir.mktmpdir }
+  let(:file_path) { File.join(tmpdir, "client.pem") }
+  let(:dir_path) { File.dirname(file_path) }
+
+  before do
+    allow(File).to receive(:exist?).and_call_original
+    allow(File).to receive(:exist?).with(file_path).and_return(false)
+    allow(File).to receive(:exist?).with(dir_path).and_return(true)
+    allow(File).to receive(:directory?).with(dir_path).and_return(true)
+    allow(File).to receive(:writable?).with(file_path).and_return(true)
+    allow(File).to receive(:writable?).with(dir_path).and_return(true)
+  end
+
   describe "run" do
     context "when nothing is passed" do
       # from spec/support/shared/unit/knife_shared.rb
@@ -118,18 +131,66 @@ describe Chef::Knife::ClientCreate do
 
       describe "with -f or --file" do
         before do
+          knife.config[:file] = file_path
           client.private_key "woot"
         end
 
         it "should write the private key to a file" do
-          file = Tempfile.new
-          file_path = file.path
-          knife.config[:file] = file_path
           filehandle = double("Filehandle")
           expect(filehandle).to receive(:print).with("woot")
           expect(File).to receive(:open).with(file_path, "w").and_yield(filehandle)
           knife.run
         end
+
+        context "when the directory does not exist" do
+          before { allow(File).to receive(:exist?).with(dir_path).and_return(false) }
+
+          it "writes a fatal message and exits 1" do
+            expect(knife.ui).to receive(:fatal).with("Directory #{dir_path} does not exist. Please create and retry.")
+            expect { knife.run }.to raise_error(SystemExit)
+          end
+        end
+
+        context "when the directory is not writable" do
+          before { allow(File).to receive(:writable?).with(dir_path).and_return(false) }
+
+          it "writes a fatal message and exits 1" do
+            expect(knife.ui).to receive(:fatal).with("Directory #{dir_path} is not writable. Please check the permissions.")
+            expect { knife.run }.to raise_error(SystemExit)
+          end
+        end
+
+        context "when the directory is a file" do
+          before { allow(File).to receive(:directory?).with(dir_path).and_return(false) }
+
+          it "writes a fatal message and exits 1" do
+            expect(knife.ui).to receive(:fatal).with("#{dir_path} exists, but is not a directory. Please update your file path (--file #{file_path}) or re-create #{dir_path} as a directory.")
+            expect { knife.run }.to raise_error(SystemExit)
+          end
+        end
+
+        context "when the file does not exist" do
+          before do
+            allow(File).to receive(:exist?).with(file_path).and_return(false)
+          end
+
+          it "does not log a fatal message and does not raise exception" do
+            expect(knife.ui).not_to receive(:fatal)
+            expect { knife.run }.not_to raise_error
+          end
+        end
+
+        context "when the file exists and is not writable" do
+          before do
+            allow(File).to receive(:exist?).with(file_path).and_return(true)
+            allow(File).to receive(:writable?).with(file_path).and_return(false)
+          end
+
+          it "writes a fatal message and exits 1" do
+            expect(knife.ui).to receive(:fatal).with("File #{file_path} is not writable. Please check the permissions.")
+            expect { knife.run }.to raise_error(SystemExit)
+          end
+        end
       end
 
       describe "with -p or --public-key" do
@@ -166,39 +227,6 @@ describe Chef::Knife::ClientCreate do
           expect(client.validator).to be_truthy
         end
       end
-
-      describe "with -f or --file when dir or file is not writable or does not exists" do
-        let(:dir_path) { File.expand_path(File.join(CHEF_SPEC_DATA, "knife", "temp_dir")) }
-        let(:file_path) { File.expand_path(File.join(dir_path, "tmp.pem")) }
-
-        it "when the directory does not exists" do
-          knife.config[:file] = "example/client1.pem"
-          expect(knife.ui).to receive(:fatal).with("Directory example does not exist.")
-          expect { knife.run }.to raise_error(SystemExit)
-        end
-
-        it "when the directory not writable" do
-          knife.config[:file] = file_path
-          File.chmod(777, dir_path)
-          expect(knife.ui).to receive(:fatal).with("Directory #{dir_path} is not writable. Check permissions.")
-          expect { knife.run }.to raise_error(SystemExit)
-        end
-
-        it "when the file does not exists" do
-          path = "#{dir_path}/client1.pem"
-          knife.config[:file] = path
-          File.chmod(0755, dir_path)
-          expect(knife.ui).to receive(:fatal).with("File #{path} does not exist.")
-          expect { knife.run }.to raise_error(SystemExit)
-        end
-
-        it "when the file is not writable" do
-          knife.config[:file] = file_path
-          File.chmod(777, file_path)
-          expect(knife.ui).to receive(:fatal).with("File #{file_path} is not writable. Check permissions.")
-          expect { knife.run }.to raise_error(SystemExit)
-        end
-      end
     end
   end
 end