diff options
author | Jason Barnett <jason.w.barnett@gmail.com> | 2021-08-31 17:00:53 -0600 |
---|---|---|
committer | Jason Barnett <jason.w.barnett@gmail.com> | 2021-08-31 19:12:02 -0600 |
commit | f4dfb9c3b3b9e4fa334b94d495d60d33c245920b (patch) | |
tree | 90ba1c09ac58873a13935acf33dbe77e18c4d87f | |
parent | 2c6232c52c8b2745eefb417720f7c4188ad8b8ed (diff) | |
download | chef-f4dfb9c3b3b9e4fa334b94d495d60d33c245920b.tar.gz |
Fix knife client create
Signed-off-by: Jason Barnett <jason.w.barnett@gmail.com>
-rw-r--r-- | knife/lib/chef/knife/client_create.rb | 50 | ||||
-rw-r--r-- | knife/spec/integration/client_create_spec.rb | 1 | ||||
-rw-r--r-- | knife/spec/unit/knife/client_create_spec.rb | 100 |
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 |