diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2015-05-26 10:13:47 -0700 |
---|---|---|
committer | Lamont Granquist <lamont@scriptkiddie.org> | 2015-05-26 10:13:47 -0700 |
commit | a6d886da4a4a9771c35b020267fb10f465ccce0d (patch) | |
tree | 6cc4e88053802b0458f3a0a757413151945e293a | |
parent | ef28eff998eba6433d00c81376a38e6180985c10 (diff) | |
parent | 4044b25b8636a193d3079dc75aad25f76b735c5c (diff) | |
download | chef-a6d886da4a4a9771c35b020267fb10f465ccce0d.tar.gz |
Merge pull request #3397 from chef/lcg/directory-missing-owner-validation-check
Lcg/directory missing owner validation check
-rw-r--r-- | CHANGELOG.md | 20 | ||||
-rw-r--r-- | lib/chef/provider/directory.rb | 3 | ||||
-rw-r--r-- | spec/unit/provider/directory_spec.rb | 334 |
3 files changed, 214 insertions, 143 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index a739e4b5e1..d4a9181a44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## Unreleased + +* [pr#3397](https://github.com/chef/chef/pull/3397): Validate owner exists in directory resources + ## 12.4.0 * [**Phil Dibowitz**](https://github.com/jaymzh): @@ -29,24 +33,24 @@ * Add an integration test of chef-client with empty ENV. #3321 * Switch over Windows builds to universal builds. #3278 * Convert bootstrap template to use sh #2877 -* [Issue #3316](https://github.com/chef/chef/issues/3316) Fix idempotency issues with the `windows_package` resource +* [Issue #3316](https://github.com/chef/chef/issues/3316): Fix idempotency issues with the `windows_package` resource * [pr#3295](https://github.com/chef/chef/pull/3295): Stop mutating `new_resource.checksum` in file providers. Fixes some ChecksumMismatch exceptions like [issue#3168](https://github.com/chef/chef/issues/3168) * [pr#3320] Sanitize non-UTF8 characters in the node data before doing node.save(). Works around many UTF8 exception issues reported on node.save(). * Implemented X-Ops-Server-API-Version with a API version of 0, as well as error handling when the Chef server does not support the API version that the client supports. * [pr#3327](https://github.com/chef/chef/pull/3327): Fix unreliable AIX service group parsing mechanism. * [pr#3333](https://github.com/chef/chef/pull/3333): Fix SSL errors when connecting to private Supermarkets * [pr#3340](https://github.com/chef/chef/pull/3340): Allow Event dispatch subscribers to be inspected. -* [Issue #3055](https://github.com/chef/chef/issues/3055) Fix regex parsing for recipe failures on Windows -* [pr#3345](https://github.com/chef/chef/pull/3345) Windows Event log logger -* [pr#3336](https://github.com/chef/chef/pull/3336) Remote file understands UNC paths +* [Issue #3055](https://github.com/chef/chef/issues/3055): Fix regex parsing for recipe failures on Windows +* [pr#3345](https://github.com/chef/chef/pull/3345): Windows Event log logger +* [pr#3336](https://github.com/chef/chef/pull/3336): Remote file understands UNC paths * [pr#3269](https://github.com/chef/chef/pull/3269): Deprecate automatic recipe DSL for classes in `Chef::Resource` * [pr#3360](https://github.com/chef/chef/pull/3360): Add check_resource_semantics! lifecycle method to provider * [pr#3344](https://github.com/chef/chef/pull/3344): Rewrite Windows user resouce code to use ffi instead of win32-api -* [pr#3318](https://github.com/chef/chef/pull/3318) Modify Windows package provider to allow for url source -* [pr#3381](https://github.com/chef/chef/pull/3381) warn on cookbook self-deps +* [pr#3318](https://github.com/chef/chef/pull/3318): Modify Windows package provider to allow for url source +* [pr#3381](https://github.com/chef/chef/pull/3381): warn on cookbook self-deps * [pr#2312](https://github.com/chef/chef/pull/2312): fix `node[:recipes]` duplication, add `node[:cookbooks]` and `node[:expanded_run_list]` -* [pr#3325](https://github.com/chef/chef/pull/3325) enforce passing a node name with validatorless bootstrapping -* [pr#3398](https://github.com/chef/chef/pull/3398) Allow spaces in files for the `remote_file` resource +* [pr#3325](https://github.com/chef/chef/pull/3325): enforce passing a node name with validatorless bootstrapping +* [pr#3398](https://github.com/chef/chef/pull/3398): Allow spaces in files for the `remote_file` resource ## 12.3.0 diff --git a/lib/chef/provider/directory.rb b/lib/chef/provider/directory.rb index 416393ac60..4d5423d0e8 100644 --- a/lib/chef/provider/directory.rb +++ b/lib/chef/provider/directory.rb @@ -43,6 +43,9 @@ class Chef end def define_resource_requirements + # deep inside FAC we have to assert requirements, so call FACs hook to set that up + access_controls.define_resource_requirements + requirements.assert(:create) do |a| # Make sure the parent dir exists, or else fail. # for why run, print a message explaining the potential error. diff --git a/spec/unit/provider/directory_spec.rb b/spec/unit/provider/directory_spec.rb index 6489f2ca50..38d6db8320 100644 --- a/spec/unit/provider/directory_spec.rb +++ b/spec/unit/provider/directory_spec.rb @@ -16,173 +16,237 @@ # limitations under the License. # -require 'ostruct' - require 'spec_helper' require 'tmpdir' describe Chef::Provider::Directory do - before(:each) do - @new_resource = Chef::Resource::Directory.new(Dir.tmpdir) - if !windows? - @new_resource.owner(500) - @new_resource.group(500) - @new_resource.mode(0644) + let(:tmp_dir) { Dir.mktmpdir } + let(:new_resource) { Chef::Resource::Directory.new(tmp_dir) } + let(:node) { Chef::Node.new } + let(:events) { Chef::EventDispatch::Dispatcher.new } + let(:run_context) { Chef::RunContext.new(node, {}, events) } + let(:directory) { Chef::Provider::Directory.new(new_resource, run_context) } + + describe "#load_current_resource" do + describe "scanning file security metadata" + describe "on unix", unix_only: true do + describe "when the directory exists" do + let(:dir_stat) { File::Stat.new(tmp_dir) } + let(:expected_uid) { dir_stat.uid } + let(:expected_gid) { dir_stat.gid } + let(:expected_mode) { "0%o" % ( dir_stat.mode & 007777 ) } + let(:expected_pwnam) { Etc.getpwuid(expected_uid).name } + let(:expected_grnam) { Etc.getgrgid(expected_gid).name } + + it "describes the access mode as a String of octal integers" do + directory.load_current_resource + expect(directory.current_resource.mode).to eq(expected_mode) + end + + it "when the new_resource.owner is numeric, describes the owner as a numeric uid" do + new_resource.owner(500) + directory.load_current_resource + expect(directory.current_resource.owner).to eql(expected_uid) + end + + it "when the new_resource.group is numeric, describes the group as a numeric gid" do + new_resource.group(500) + directory.load_current_resource + expect(directory.current_resource.group).to eql(expected_gid) + end + + it "when the new_resource.owner is a string, describes the owner as a string" do + new_resource.owner("foo") + directory.load_current_resource + expect(directory.current_resource.owner).to eql(expected_pwnam) + end + + it "when the new_resource.group is a string, describes the group as a string" do + new_resource.group("bar") + directory.load_current_resource + expect(directory.current_resource.group).to eql(expected_grnam) + end + end end - @node = Chef::Node.new - @events = Chef::EventDispatch::Dispatcher.new - @run_context = Chef::RunContext.new(@node, {}, @events) - @directory = Chef::Provider::Directory.new(@new_resource, @run_context) - end + describe "on windows", windows_only: true do + describe "when the directory exists" do + it "the mode is always nil" do + directory.load_current_resource + expect(directory.current_resource.mode).to be nil + end + + it "the owner is always nil" do + directory.load_current_resource + expect(directory.current_resource.owner).to be nil + end + + it "the group is always nil" do + directory.load_current_resource + expect(directory.current_resource.group).to be nil + end + + it "rights are always nil (incorrectly)" do + directory.load_current_resource + expect(directory.current_resource.rights).to be nil + end + + it "inherits is always nil (incorrectly)" do + directory.load_current_resource + expect(directory.current_resource.inherits).to be nil + end + end + end + describe "when the directory does not exist" do + before do + FileUtils.rmdir tmp_dir + end - describe "scanning file security metadata on windows" do - before do + it "sets the mode, group and owner to nil" do + directory.load_current_resource + expect(directory.current_resource.mode).to eq(nil) + expect(directory.current_resource.group).to eq(nil) + expect(directory.current_resource.owner).to eq(nil) + end end - it "describes the directory's access rights" do - skip - end end - describe "scanning file security metadata on unix" do - before do - allow(ChefConfig).to receive(:windows?).and_return(false) - end - let(:mock_stat) do - cstats = double("stats") - allow(cstats).to receive(:uid).and_return(500) - allow(cstats).to receive(:gid).and_return(500) - allow(cstats).to receive(:mode).and_return(0755) - cstats - end + describe "#define_resource_requirements" do + describe "on unix", unix_only: true do + it "raises an exception if the user does not exist" do + new_resource.owner("arglebargle_iv") + expect(Etc).to receive(:getpwnam).with("arglebargle_iv").and_raise(ArgumentError) + directory.action = :create + directory.load_current_resource + expect(directory.access_controls).to receive(:define_resource_requirements).and_call_original + directory.define_resource_requirements + expect { directory.process_resource_requirements }.to raise_error(ArgumentError) + end - it "describes the access mode as a String of octal integers" do - allow(File).to receive(:exists?).and_return(true) - expect(File).to receive(:stat).and_return(mock_stat) - @directory.load_current_resource - expect(@directory.current_resource.mode).to eq("0755") + it "raises an exception if the group does not exist" do + new_resource.group("arglebargle_iv") + expect(Etc).to receive(:getgrnam).with("arglebargle_iv").and_raise(ArgumentError) + directory.action = :create + directory.load_current_resource + expect(directory.access_controls).to receive(:define_resource_requirements).and_call_original + directory.define_resource_requirements + expect { directory.process_resource_requirements }.to raise_error(ArgumentError) + end end + end - context "when user and group are specified with UID/GID" do - it "describes the current owner and group as UID and GID" do - allow(File).to receive(:exists?).and_return(true) - expect(File).to receive(:stat).and_return(mock_stat) - @directory.load_current_resource - expect(@directory.current_resource.path).to eql(@new_resource.path) - expect(@directory.current_resource.owner).to eql(500) - expect(@directory.current_resource.group).to eql(500) + describe "#run_action(:create)" do + describe "when the directory exists" do + it "does not create the directory" do + expect(Dir).not_to receive(:mkdir).with(new_resource.path) + directory.run_action(:create) + end + + it "should not set the resource as updated" do + directory.run_action(:create) + expect(new_resource).not_to be_updated end end - context "when user/group are specified with user/group names" do + describe "when the directory does not exist" do + before do + FileUtils.rmdir tmp_dir + end + + it "creates the directory" do + directory.run_action(:create) + expect(File.exist?(tmp_dir)).to be true + end + + it "sets the new resource as updated" do + directory.run_action(:create) + expect(new_resource).to be_updated + end end - end - # Unix only for now. While file security attribute reporting for windows is - # disabled, unix and windows differ in the number of exists? calls that are - # made by the provider. - it "should create a new directory on create, setting updated to true", :unix_only do - @new_resource.path "/tmp/foo" + describe "when the parent directory does not exist" do + before do + new_resource.path "#{tmp_dir}/foobar" + FileUtils.rmdir tmp_dir + end - expect(File).to receive(:exists?).at_least(:once).and_return(false) - expect(File).to receive(:directory?).with("/tmp").and_return(true) - expect(Dir).to receive(:mkdir).with(@new_resource.path).once.and_return(true) + it "raises an exception when recursive is false" do + new_resource.recursive false + expect { directory.run_action(:create) }.to raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist) + end - expect(@directory).to receive(:do_acl_changes) - allow(@directory).to receive(:do_selinux) - @directory.run_action(:create) - expect(@directory.new_resource).to be_updated - end + it "creates the directories when recursive is true" do + new_resource.recursive true + directory.run_action(:create) + expect(new_resource).to be_updated + expect(File.exist?("#{tmp_dir}/foobar")).to be true + end - it "should raise an exception if the parent directory does not exist and recursive is false" do - @new_resource.path "/tmp/some/dir" - @new_resource.recursive false - expect { @directory.run_action(:create) }.to raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist) - end + it "raises an exception when the parent directory is a file and recursive is true" do + FileUtils.touch tmp_dir + new_resource.recursive true + expect { directory.run_action(:create) }.to raise_error + end - # Unix only for now. While file security attribute reporting for windows is - # disabled, unix and windows differ in the number of exists? calls that are - # made by the provider. - it "should create a new directory when parent directory does not exist if recursive is true and permissions are correct", :unix_only do - @new_resource.path "/path/to/dir" - @new_resource.recursive true - expect(File).to receive(:exists?).with(@new_resource.path).ordered.and_return(false) - - expect(File).to receive(:exists?).with('/path/to').ordered.and_return(false) - expect(File).to receive(:exists?).with('/path').ordered.and_return(true) - expect(Chef::FileAccessControl).to receive(:writable?).with('/path').ordered.and_return(true) - expect(File).to receive(:exists?).with(@new_resource.path).ordered.and_return(false) - - expect(FileUtils).to receive(:mkdir_p).with(@new_resource.path).and_return(true) - expect(@directory).to receive(:do_acl_changes) - allow(@directory).to receive(:do_selinux) - @directory.run_action(:create) - expect(@new_resource).to be_updated + it "raises the right exception when the parent directory is a file and recursive is true" do + pending "this seems to return the wrong error" # FIXME + FileUtils.touch tmp_dir + new_resource.recursive true + expect { directory.run_action(:create) }.to raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist) + end + end end + describe "#run_action(:create)" do + describe "when the directory exists" do + it "deletes the directory" do + directory.run_action(:delete) + expect(File.exist?(tmp_dir)).to be false + end - it "should raise an error when creating a directory when parent directory is a file" do - expect(File).to receive(:directory?).and_return(false) - expect(Dir).not_to receive(:mkdir).with(@new_resource.path) - expect { @directory.run_action(:create) }.to raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist) - expect(@directory.new_resource).not_to be_updated - end + it "sets the new resource as updated" do + directory.run_action(:delete) + expect(new_resource).to be_updated + end + end - # Unix only for now. While file security attribute reporting for windows is - # disabled, unix and windows differ in the number of exists? calls that are - # made by the provider. - it "should not create the directory if it already exists", :unix_only do - stub_file_cstats - @new_resource.path "/tmp/foo" - expect(File).to receive(:directory?).at_least(:once).and_return(true) - expect(Chef::FileAccessControl).to receive(:writable?).with("/tmp").and_return(true) - expect(File).to receive(:exists?).at_least(:once).and_return(true) - expect(Dir).not_to receive(:mkdir).with(@new_resource.path) - expect(@directory).to receive(:do_acl_changes) - @directory.run_action(:create) - end + describe "when the directory does not exist" do + before do + FileUtils.rmdir tmp_dir + end - it "should delete the directory if it exists, and is writable with action_delete" do - expect(File).to receive(:directory?).and_return(true) - expect(Chef::FileAccessControl).to receive(:writable?).once.and_return(true) - expect(Dir).to receive(:delete).with(@new_resource.path).once.and_return(true) - @directory.run_action(:delete) - end + it "does not delete the directory" do + expect(Dir).not_to receive(:delete).with(new_resource.path) + directory.run_action(:delete) + end - it "should raise an exception if it cannot delete the directory due to bad permissions" do - allow(File).to receive(:exists?).and_return(true) - allow(Chef::FileAccessControl).to receive(:writable?).and_return(false) - expect { @directory.run_action(:delete) }.to raise_error(RuntimeError) - end + it "sets the new resource as updated" do + directory.run_action(:delete) + expect(new_resource).not_to be_updated + end + end - it "should take no action when deleting a target directory that does not exist" do - @new_resource.path "/an/invalid/path" - allow(File).to receive(:exists?).and_return(false) - expect(Dir).not_to receive(:delete).with(@new_resource.path) - @directory.run_action(:delete) - expect(@directory.new_resource).not_to be_updated - end + describe "when the directory is not writable" do + before do + allow(Chef::FileAccessControl).to receive(:writable?).and_return(false) + end - it "should raise an exception when deleting a directory when target directory is a file" do - stub_file_cstats - @new_resource.path "/an/invalid/path" - allow(File).to receive(:exists?).and_return(true) - expect(File).to receive(:directory?).and_return(false) - expect(Dir).not_to receive(:delete).with(@new_resource.path) - expect { @directory.run_action(:delete) }.to raise_error(RuntimeError) - expect(@directory.new_resource).not_to be_updated - end + it "cannot delete it and raises an exception" do + expect { directory.run_action(:delete) }.to raise_error(RuntimeError) + end + end + + describe "when the target directory is a file" do + before do + FileUtils.rmdir tmp_dir + FileUtils.touch tmp_dir + end - def stub_file_cstats - cstats = double("stats") - allow(cstats).to receive(:uid).and_return(500) - allow(cstats).to receive(:gid).and_return(500) - allow(cstats).to receive(:mode).and_return(0755) - # File.stat is called in: - # - Chef::Provider::File.load_current_resource_attrs - # - Chef::ScanAccessControl via Chef::Provider::File.setup_acl - allow(File).to receive(:stat).and_return(cstats) + it "cannot delete it and raises an exception" do + expect { directory.run_action(:delete) }.to raise_error(RuntimeError) + end + end end end |