summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTim Smith <tsmith@chef.io>2021-09-01 14:47:41 -0700
committerGitHub <noreply@github.com>2021-09-01 14:47:41 -0700
commite83cc918bc2f2ab2b4e33ab61e8b2d21d5fd8e7a (patch)
tree22cc44b46ef0465de040035f23241e1591ff4048
parent2428843412bc06445130ae7724e5fd35c8bff9d4 (diff)
parentf4dfb9c3b3b9e4fa334b94d495d60d33c245920b (diff)
downloadchef-e83cc918bc2f2ab2b4e33ab61e8b2d21d5fd8e7a.tar.gz
Merge pull request #11986 from jasonwbarnett/bugfix/knife-client-create
Fix knife client create
-rw-r--r--knife/lib/chef/knife/client_create.rb50
-rw-r--r--knife/spec/integration/client_create_spec.rb1
-rw-r--r--knife/spec/unit/knife/client_create_spec.rb100
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