Skip to content

Commit

Permalink
PUP-1681 Stat doesn't expose correct mode on Win
Browse files Browse the repository at this point in the history
 - An issue was discovered where the values returned from
   Puppet::FileSystem.stat and Puppet::Util::Windows::Security.get_mode
   were not aligning as they should.  The call to get_mode returns
   a mode value that should more correctly express a simulated POSIX
   mode on Windows
 - The stat instance returned from Ruby has been further monkey-patched
   on Windows to add an appropriate mode value using the existing
   lower-level code that reads a files security descriptor.
 - Some tests that were performing special handling of mode values were
   updated now that mode should be more consistent across platforms.
 - In some cases, an existing call to File.chmod was changed to call
   set_mode under Windows.  In the future, our FileSystem abstraction
   should be modified to create an OS-agnostic single point of entry
   for setting mode on files.
  • Loading branch information
Iristyle committed Feb 13, 2014
1 parent 0930041 commit 857291e
Show file tree
Hide file tree
Showing 9 changed files with 51 additions and 9 deletions.
16 changes: 15 additions & 1 deletion lib/puppet/file_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ def self.symlink(path, dest, options = {})
end

# @return [Boolean] true if the file is a symbolic link.
#
#
# @api public
#
def self.symlink?(path)
Expand Down Expand Up @@ -340,4 +340,18 @@ def self.path_string(path)
def self.exclusive_create(path, mode, &block)
@impl.exclusive_create(assert_path(path), mode, &block)
end

# Changes permission bits on the named path to the bit pattern represented
# by mode.
#
# @param mode [Integer] The mode to apply to the file if it is created
# @param path [String] The path to the file, can also accept [PathName]
#
# @raise [Errno::ENOENT]: path doesn't exist
#
# @api public
#
def self.chmod(mode, path)
@impl.chmod(mode, path)
end
end
4 changes: 4 additions & 0 deletions lib/puppet/file_system/file19windows.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ def lstat(path)
Puppet::Util::Windows::File.lstat(path)
end

def chmod(mode, path)
Puppet::Util::Windows::Security.set_mode(mode, path.to_s)
end

private

def raise_if_symlinks_unsupported
Expand Down
3 changes: 3 additions & 0 deletions lib/puppet/file_system/file_impl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,7 @@ def compare_stream(path, stream)
open(path, 0, 'rb') { |this| FileUtils.compare_stream(this, stream) }
end

def chmod(mode, path)
FileUtils.chmod(mode, path)
end
end
19 changes: 17 additions & 2 deletions lib/puppet/util/windows/file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -219,14 +219,23 @@ def readlink(link_name)
def stat(file_name)
file_name = file_name.to_s # accomodate PathName or String
stat = File.stat(file_name)
singleton_class = class << stat; self; end
target_path = file_name

if symlink?(file_name)
link_ftype = File.stat(readlink(file_name)).ftype
target_path = readlink(file_name)
link_ftype = File.stat(target_path).ftype

# sigh, monkey patch instance method for instance, and close over link_ftype
singleton_class = class << stat; self; end
singleton_class.send(:define_method, :ftype) do
link_ftype
end
end

singleton_class.send(:define_method, :mode) do
Puppet::Util::Windows::Security.get_mode(target_path)
end

stat
end
module_function :stat
Expand All @@ -235,6 +244,12 @@ def lstat(file_name)
file_name = file_name.to_s # accomodate PathName or String
# monkey'ing around!
stat = File.lstat(file_name)

singleton_class = class << stat; self; end
singleton_class.send(:define_method, :mode) do
Puppet::Util::Windows::Security.get_mode(file_name)
end

if symlink?(file_name)
def stat.ftype
"link"
Expand Down
3 changes: 2 additions & 1 deletion spec/integration/configurer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@
@configurer.run :catalog => @catalog, :report => report
t2 = Time.now.tv_sec

file_mode = Puppet.features.microsoft_windows? ? '100644' : '100666'
# sticky bit only applies to directories in windows
file_mode = Puppet.features.microsoft_windows? ? '666' : '100666'

Puppet::FileSystem.stat(Puppet[:lastrunfile]).mode.to_s(8).should == file_mode

Expand Down
6 changes: 4 additions & 2 deletions spec/integration/type/file_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -886,13 +886,14 @@ def build_path(dir)
it "should be able to copy files with spaces in their names" do
dest = tmpfile("destwith spaces")
source = tmpfile_with_contents("filewith spaces", "foo")
File.chmod(0755, source)

expected_mode = 0755
Puppet::FileSystem.chmod(expected_mode, source)

catalog.add_resource described_class.new(:path => dest, :source => source)

catalog.apply

expected_mode = Puppet.features.microsoft_windows? ? 0644 : 0755
File.read(dest).should == "foo"
(Puppet::FileSystem.stat(dest).mode & 007777).should == expected_mode
end
Expand Down Expand Up @@ -1021,6 +1022,7 @@ def expects_at_least_one_inherited_system_ace_grants_full_access(path)
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(
Expand Down
4 changes: 3 additions & 1 deletion spec/integration/type/nagios_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ def get_mode(file)
:mode => '0640'
)
run_in_catalog(resource)
( "%o" % get_mode(target_file) ).should == "100640"
# sticky bit only applies to directories in Windows
mode = Puppet.features.microsoft_windows? ? "640" : "100640"
( "%o" % get_mode(target_file) ).should == mode
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/integration/util/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def define_settings(section, settings_hash)

settings.use(:main)

expect(Puppet::FileSystem.stat(settings[:maindir]).mode & 007777).to eq(Puppet.features.microsoft_windows? ? 0755 : 0750)
expect(Puppet::FileSystem.stat(settings[:maindir]).mode & 007777).to eq(0750)
end

it "reparses configuration if configuration file is touched", :if => !Puppet.features.microsoft_windows? do
Expand Down
3 changes: 2 additions & 1 deletion spec/unit/type/file/mode_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@

it "changes only the requested bits" do
# lower nibble must be set to 4 for the sake of passing on Windows
FileUtils.chmod 0464, path
Puppet::FileSystem.chmod(0464, path)

mode_sym.sync
stat = Puppet::FileSystem.stat(path)
(stat.mode & 0777).to_s(8).should == "644"
Expand Down

0 comments on commit 857291e

Please sign in to comment.