From bff2bcd477face3395f7034dbdb16b95189f4f1d Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 15 Oct 2014 17:18:52 -0700 Subject: [PATCH 1/2] (PUP-2609) Make copying permissions on Win an error Copying permissions on Windows are problematic, so it isn't done. Change the deprecation warning about source_permissions => use/use_when_creating to an error. --- lib/puppet/type/file/source.rb | 7 +- spec/integration/type/file_spec.rb | 132 ++++++----------------------- spec/unit/type/file/source_spec.rb | 73 +++++----------- 3 files changed, 48 insertions(+), 164 deletions(-) diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb index 7679b4c1a12..054e3417c65 100644 --- a/lib/puppet/type/file/source.rb +++ b/lib/puppet/type/file/source.rb @@ -127,11 +127,10 @@ def copy_source_values if [:use, :use_when_creating].include?(resource[:source_permissions]) && (resource[:owner] == nil || resource[:group] == nil || resource[:mode] == nil) - warning = "Copying %s from the source" << - " file on Windows is deprecated;" << + err_msg = "Copying %s from the source" << + " file on Windows is not supported;" << " use source_permissions => ignore." - Puppet.deprecation_warning(warning % 'owner/mode/group') - resource.debug(warning % metadata_method.to_s) + self.fail Puppet::Error, err_msg % 'owner/mode/group' end # But never try to copy remote owner/group on Windows next if [:owner, :group].include?(metadata_method) && !local? diff --git a/spec/integration/type/file_spec.rb b/spec/integration/type/file_spec.rb index 4e7563489fb..2998afdcc6a 100755 --- a/spec/integration/type/file_spec.rb +++ b/spec/integration/type/file_spec.rb @@ -949,23 +949,25 @@ def build_path(dir) describe "when sourcing" do let(:source) { tmpfile_with_contents("source_default_values", "yay") } - it "should apply the source metadata values" do - set_mode(0770, source) + describe "on POSIX systems", :if => Puppet.features.posix? do + it "should apply the source metadata values" do + set_mode(0770, source) - file = described_class.new( - :path => path, - :ensure => :file, - :source => source, - :source_permissions => :use, - :backup => false - ) + file = described_class.new( + :path => path, + :ensure => :file, + :source => source, + :source_permissions => :use, + :backup => false + ) - catalog.add_resource file - catalog.apply + catalog.add_resource file + catalog.apply - get_owner(path).should == get_owner(source) - get_group(path).should == get_group(source) - (get_mode(path) & 07777).should == 0770 + get_owner(path).should == get_owner(source) + get_group(path).should == get_group(source) + (get_mode(path) & 07777).should == 0770 + end end it "should override the default metadata values" do @@ -975,7 +977,6 @@ def build_path(dir) :path => path, :ensure => :file, :source => source, - :source_permissions => :use, :backup => false, :mode => '0440' ) @@ -1019,26 +1020,6 @@ def expects_at_least_one_inherited_system_ace_grants_full_access(path) expects_at_least_one_inherited_ace_grants_full_access(path, @sids[:system]) end - it "should provide valid default values when ACLs are not supported" do - Puppet::Util::Windows::Security.stubs(:supports_acl?).returns(false) - Puppet::Util::Windows::Security.stubs(:supports_acl?).with(source).returns false - - file = described_class.new( - :path => path, - :ensure => :file, - :source => source, - :backup => false, - :source_permissions => :use - ) - - catalog.add_resource file - catalog.apply - - get_owner(path).should =~ /^S\-1\-5\-.*$/ - get_group(path).should =~ /^S\-1\-0\-0.*$/ - get_mode(path).should == 0644 - end - describe "when processing SYSTEM ACEs" do before do @sids = { @@ -1063,56 +1044,26 @@ def expects_at_least_one_inherited_system_ace_grants_full_access(path) catalog.add_resource @file end - describe "when source permissions are ignored" do - it "preserves the inherited SYSTEM ACE" do - catalog.apply - - expects_at_least_one_inherited_system_ace_grants_full_access(path) - end - end - - describe "when permissions are insync?" do - before :each do - @file[:source_permissions] = :use - end - - it "preserves the explicit SYSTEM ACE" do - FileUtils.touch(path) - - sd = Puppet::Util::Windows::Security.get_security_descriptor(path) - sd.protect = true - sd.owner = @sids[:none] - sd.group = @sids[:none] - Puppet::Util::Windows::Security.set_security_descriptor(source, sd) - Puppet::Util::Windows::Security.set_security_descriptor(path, sd) - - catalog.apply - - expects_system_granted_full_access_explicitly(path) - end - end - describe "when permissions are not insync?" do before :each do @file[:owner] = 'None' @file[:group] = 'None' - @file[:source_permissions] = :use end - it "replaces inherited SYSTEM ACEs with an uninherited one for an existing file" do + it "preserves the inherited SYSTEM ACE for an existing file" do FileUtils.touch(path) expects_at_least_one_inherited_system_ace_grants_full_access(path) catalog.apply - expects_system_granted_full_access_explicitly(path) + expects_at_least_one_inherited_system_ace_grants_full_access(path) end - it "replaces inherited SYSTEM ACEs for a new file with an uninherited one" do + it "applies the inherited SYSTEM ACEs for a new file" do catalog.apply - expects_system_granted_full_access_explicitly(path) + expects_at_least_one_inherited_system_ace_grants_full_access(path) end end @@ -1121,7 +1072,6 @@ def expects_at_least_one_inherited_system_ace_grants_full_access(path) @file[:owner] = @sids[:users] @file[:group] = @sids[:system] @file[:mode] = '0644' - @file[:source_permissions] = :use catalog.apply end @@ -1182,60 +1132,26 @@ def grant_everyone_full_access(path) grant_everyone_full_access(dir) end - describe "when source permissions are ignored" do - it "preserves the inherited SYSTEM ACE" do - catalog.apply - - expects_at_least_one_inherited_system_ace_grants_full_access(dir) - end - end - - describe "when permissions are insync?" do - before :each do - @directory[:source_permissions] = :use - end - - it "preserves the explicit SYSTEM ACE" do - Dir.mkdir(dir) - - source_dir = tmpdir('source_dir') - @directory[:source] = source_dir - - sd = Puppet::Util::Windows::Security.get_security_descriptor(source_dir) - sd.protect = true - sd.owner = @sids[:none] - sd.group = @sids[:none] - Puppet::Util::Windows::Security.set_security_descriptor(source_dir, sd) - Puppet::Util::Windows::Security.set_security_descriptor(dir, sd) - - catalog.apply - - expects_system_granted_full_access_explicitly(dir) - end - end - describe "when permissions are not insync?" do before :each do @directory[:owner] = 'None' @directory[:group] = 'None' - @directory[:mode] = '0444' - @directory[:source_permissions] = :use end - it "replaces inherited SYSTEM ACEs with an uninherited one for an existing directory" do + it "preserves the inherited SYSTEM ACEs for an existing directory" do FileUtils.mkdir(dir) expects_at_least_one_inherited_system_ace_grants_full_access(dir) catalog.apply - expects_system_granted_full_access_explicitly(dir) + expects_at_least_one_inherited_system_ace_grants_full_access(dir) end - it "replaces inherited SYSTEM ACEs with an uninherited one for an existing directory" do + it "applies the inherited SYSTEM ACEs for a new directory" do catalog.apply - expects_system_granted_full_access_explicitly(dir) + expects_at_least_one_inherited_system_ace_grants_full_access(dir) end describe "created with SYSTEM as the group" do diff --git a/spec/unit/type/file/source_spec.rb b/spec/unit/type/file/source_spec.rb index 0c1c0eca1ad..81ff042c901 100755 --- a/spec/unit/type/file/source_spec.rb +++ b/spec/unit/type/file/source_spec.rb @@ -186,28 +186,24 @@ Puppet.features.stubs(:root?).returns true end - it "should not issue a deprecation warning if the source mode value is a Numeric" do + it "should not issue an error - except on Windows - if the source mode value is a Numeric" do @metadata.stubs(:mode).returns 0173 @resource[:source_permissions] = :use if Puppet::Util::Platform.windows? - Puppet.expects(:deprecation_warning).with(regexp_matches(/Copying owner\/mode\/group from the source file on Windows is deprecated/)).at_least_once + expect { @source.copy_source_values }.to raise_error("Copying owner/mode/group from the source file on Windows is not supported; use source_permissions => ignore.") else - Puppet.expects(:deprecation_warning).never + expect { @source.copy_source_values }.not_to raise_error end - - @source.copy_source_values end - it "should not issue a deprecation warning if the source mode value is a String" do + it "should not issue an error - except on Windows - if the source mode value is a String" do @metadata.stubs(:mode).returns "173" @resource[:source_permissions] = :use if Puppet::Util::Platform.windows? - Puppet.expects(:deprecation_warning).with(regexp_matches(/Copying owner\/mode\/group from the source file on Windows is deprecated/)).at_least_once + expect { @source.copy_source_values }.to raise_error("Copying owner/mode/group from the source file on Windows is not supported; use source_permissions => ignore.") else - Puppet.expects(:deprecation_warning).never + expect { @source.copy_source_values }.not_to raise_error end - - @source.copy_source_values end it "should fail if there is no metadata" do @@ -395,81 +391,54 @@ before :each do Puppet.features.stubs(:microsoft_windows?).returns true @resource[:source_permissions] = "use" - end - let(:deprecation_message) { "Copying owner/mode/group from the" << - " source file on Windows is deprecated;" << + end + let(:err_message) { "Copying owner/mode/group from the" << + " source file on Windows is not supported;" << " use source_permissions => ignore." } - it "should copy only mode from remote sources" do + it "should issue error when copying from remote sources" do @source.stubs(:local?).returns false - @source.copy_source_values - - @resource[:owner].must be_nil - @resource[:group].must be_nil - @resource[:mode].must == "173" + expect { @source.copy_source_values }.to raise_error(err_message) end - it "should copy mode from remote sources" do - @source.stubs(:local?).returns false - - @source.copy_source_values - - @resource[:mode].must == "173" - end - - it "should copy owner and group from local sources" do + it "should issue error when copying from local sources" do @source.stubs(:local?).returns true - @source.copy_source_values - - @resource[:owner].must == 100 - @resource[:group].must == 200 - @resource[:mode].must == "173" + expect { @source.copy_source_values }.to raise_error(err_message) end - it "should issue deprecation warning when copying metadata from remote sources when group, owner, and mode are unspecified" do + it "should issue error when copying metadata from remote sources if only user is unspecified" do @source.stubs(:local?).returns false - Puppet.expects(:deprecation_warning).with(deprecation_message).at_least_once - - @source.copy_source_values - end - - it "should issue deprecation warning when copying metadata from remote sources if only user is unspecified" do - @source.stubs(:local?).returns false - Puppet.expects(:deprecation_warning).with(deprecation_message).at_least_once @resource[:group] = 2 @resource[:mode] = "0003" - @source.copy_source_values + expect { @source.copy_source_values }.to raise_error(err_message) end - it "should issue deprecation warning when copying metadata from remote sources if only group is unspecified" do + it "should issue error when copying metadata from remote sources if only group is unspecified" do @source.stubs(:local?).returns false - Puppet.expects(:deprecation_warning).with(deprecation_message).at_least_once @resource[:owner] = 1 @resource[:mode] = "0003" - @source.copy_source_values + expect { @source.copy_source_values }.to raise_error(err_message) end - it "should issue deprecation warning when copying metadata from remote sources if only mode is unspecified" do + it "should issue error when copying metadata from remote sources if only mode is unspecified" do @source.stubs(:local?).returns false - Puppet.expects(:deprecation_warning).with(deprecation_message).at_least_once @resource[:owner] = 1 @resource[:group] = 2 - @source.copy_source_values + expect { @source.copy_source_values }.to raise_error(err_message) end - it "should not issue deprecation warning when copying metadata from remote sources if group, owner, and mode are all specified" do + it "should not issue error when copying metadata from remote sources if group, owner, and mode are all specified" do @source.stubs(:local?).returns false - Puppet.expects(:deprecation_warning).with(deprecation_message).never @resource[:owner] = 1 @resource[:group] = 2 @resource[:mode] = "0003" - @source.copy_source_values + expect { @source.copy_source_values }.not_to raise_error end end end From 02bf9735b18bf1ae4df87d2cbd4390fb381dceb2 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 22 Oct 2014 15:05:54 -0700 Subject: [PATCH 2/2] (PUP-2609) Update Metadata default permission Update Metadata's default permission model to ignore source permissions, and remove unreachable code. Update spec tests for new behavior. --- lib/puppet/file_serving/metadata.rb | 43 +------------ spec/unit/file_serving/metadata_spec.rb | 82 ++++++++++++++++++------- 2 files changed, 63 insertions(+), 62 deletions(-) diff --git a/lib/puppet/file_serving/metadata.rb b/lib/puppet/file_serving/metadata.rb index 54644d78b50..386a9428bea 100644 --- a/lib/puppet/file_serving/metadata.rb +++ b/lib/puppet/file_serving/metadata.rb @@ -27,7 +27,7 @@ class MetaStat def initialize(stat, source_permissions = nil) @stat = stat - @source_permissions_ignore = source_permissions == :ignore + @source_permissions_ignore = (!source_permissions || source_permissions == :ignore) end def owner @@ -53,6 +53,7 @@ class WindowsStat < MetaStat def initialize(stat, path, source_permissions = nil) super(stat, source_permissions) @path = path + raise(ArgumentError, "Unsupported Windows source permissions option #{source_permissions}") unless @source_permissions_ignore end { :owner => 'S-1-5-32-544', @@ -60,45 +61,7 @@ def initialize(stat, path, source_permissions = nil) :mode => 0644 }.each do |method, default_value| define_method method do - return default_value if @source_permissions_ignore - - # this code remains for when source_permissions is not set to :ignore - begin - Puppet::Util::Windows::Security.send("get_#{method}", @path) || default_value - rescue Puppet::Util::Windows::Error => detail - # Very carefully catch only this specific error that result from - # trying to read permissions on a symlinked file that is on a volume - # that does not support ACLs. - # - # Unfortunately readlink method will not return the target path when - # the given path is not the symlink. - # - # For instance, consider: - # symlink c:\link points to c:\target - # FileSystem.readlink('c:/link') returns 'c:/target' - # FileSystem.readlink('c:/link/file') will NOT return 'c:/target/file' - # - # Since detecting this up front is costly, since the path in question - # needs to be recursively split and tested at each depth in the path, - # we catch the standard error that will result from trying to read a - # file that doesn't have a DACL - 1336 is ERROR_INVALID_DACL - # - # Note that this affects any manually created symlinks as well as - # paths like puppet:///modules - return default_value if detail.code == 1336 - - # Also handle a VirtualBox bug where ERROR_INVALID_FUNCTION is - # returned when following a symlink to a volume that is not NTFS. - # It appears that the VirtualBox file system is not propagating - # the standard Win32 error code above like it should. - # - # Apologies to all who enter this code path at a later date - if detail.code == 1 && Facter.value(:virtual) == 'virtualbox' - return default_value - end - - raise - end + return default_value end end end diff --git a/spec/unit/file_serving/metadata_spec.rb b/spec/unit/file_serving/metadata_spec.rb index 6c9b5633f4c..0478d281b8a 100755 --- a/spec/unit/file_serving/metadata_spec.rb +++ b/spec/unit/file_serving/metadata_spec.rb @@ -98,20 +98,6 @@ FileUtils.touch(path) end - it "should set the owner to the file's current owner" do - metadata.owner.should == owner - end - - it "should set the group to the file's current group" do - metadata.group.should == group - end - - it "should set the mode to the file's masked mode" do - set_mode(33261, path) - - metadata.mode.should == 0755 - end - describe "checksumming" do with_digest_algorithms do before :each do @@ -191,17 +177,13 @@ path = tmpfile('bar') FileUtils.touch(path) - Puppet::Util::Windows::Security.stubs(:get_owner).with(path).raises(invalid_error) - Puppet::Util::Windows::Security.stubs(:get_group).with(path).raises(invalid_error) - Puppet::Util::Windows::Security.stubs(:get_mode).with(path).raises(invalid_error) + Puppet::Util::Windows::Security.stubs(:get_owner).with(path, :use).raises(invalid_error) + Puppet::Util::Windows::Security.stubs(:get_group).with(path, :use).raises(invalid_error) + Puppet::Util::Windows::Security.stubs(:get_mode).with(path, :use).raises(invalid_error) stat = Puppet::FileSystem.stat(path) - win_stat = Puppet::FileServing::Metadata::WindowsStat.new(stat, path) - - expect { win_stat.owner }.to raise_error(ArgumentError) - expect { win_stat.group }.to raise_error(ArgumentError) - expect { win_stat.mode }.to raise_error(ArgumentError) + expect { Puppet::FileServing::Metadata::WindowsStat.new(stat, path, :use) }.to raise_error("Unsupported Windows source permissions option use") end end @@ -280,6 +262,34 @@ File::Stat.any_instance.stubs(:gid).returns group end + describe "when collecting attributes when managing files" do + let(:metadata) do + data = described_class.new(path) + data.collect + data + end + + let(:path) { tmpfile('file_serving_metadata') } + + before :each do + FileUtils.touch(path) + end + + it "should set the owner to the Process's current owner" do + metadata.owner.should == Process.euid + end + + it "should set the group to the Process's current group" do + metadata.group.should == Process.egid + end + + it "should set the mode to the default mode" do + set_mode(33261, path) + + metadata.mode.should == 0644 + end + end + it_should_behave_like "metadata collector" it_should_behave_like "metadata collector symlinks" @@ -298,6 +308,34 @@ def set_mode(mode, path) Puppet::Util::Windows::Security.stubs(:get_group).returns group end + describe "when collecting attributes when managing files" do + let(:metadata) do + data = described_class.new(path) + data.collect + data + end + + let(:path) { tmpfile('file_serving_metadata') } + + before :each do + FileUtils.touch(path) + end + + it "should set the owner to the Process's current owner" do + metadata.owner.should == "S-1-5-32-544" + end + + it "should set the group to the Process's current group" do + metadata.group.should == "S-1-0-0" + end + + it "should set the mode to the default mode" do + set_mode(33261, path) + + metadata.mode.should == 0644 + end + end + it_should_behave_like "metadata collector" it_should_behave_like "metadata collector symlinks" if Puppet.features.manages_symlinks?