Skip to content

Commit

Permalink
(PUP-2350) Disallow non-string file modes
Browse files Browse the repository at this point in the history 
In Puppet 4.0 integers will be parsed more strictly, which means that
file modes like `644` will be octal 1204. The alternative to this is to
force file modes to be strings where they cannot be interpreted
incorrectly. PUP-2349 deprecated non-string modes; this commit removes
support for them entirely.
  • Loading branch information
@adrienthebo
adrienthebo committed Oct 1, 2014
1 parent f659bb6 commit 7d5aa0d
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 31 deletions.
3 changes: 1 addition & 2 deletions lib/puppet/type/file/mode.rb
View file
Expand Up @@ -66,8 +66,7 @@ module Puppet

validate do |value|
if !value.is_a?(String)
Puppet.deprecation_warning("Non-string values for the file mode property are deprecated. It must be a string, " \
"either a symbolic mode like 'o+w,a+r' or an octal representation like '0644' or '755'.")
raise Puppet::Error, "The file mode specification must be a string, not '#{value.class.name}'"
end
unless value.nil? or valid_symbolic_mode?(value)
raise Puppet::Error, "The file mode specification is invalid: #{value.inspect}"
Expand Down
43 changes: 21 additions & 22 deletions spec/integration/type/file_spec.rb
View file
Expand Up @@ -145,7 +145,7 @@ def get_aces_for_path_by_sid(path, sid)
let(:target) { tmpdir('dir_mode') }

it "should set executable bits for newly created directories" do
catalog.add_resource described_class.new(:path => target, :ensure => :directory, :mode => 0600)
catalog.add_resource described_class.new(:path => target, :ensure => :directory, :mode => '0600')

catalog.apply

Expand All @@ -163,7 +163,7 @@ def get_aces_for_path_by_sid(path, sid)

it "should not set executable bits for unreadable directories" do
begin
catalog.add_resource described_class.new(:path => target, :ensure => :directory, :mode => 0300)
catalog.add_resource described_class.new(:path => target, :ensure => :directory, :mode => '0300')

catalog.apply

Expand All @@ -175,7 +175,7 @@ def get_aces_for_path_by_sid(path, sid)
end

it "should set user, group, and other executable bits" do
catalog.add_resource described_class.new(:path => target, :ensure => :directory, :mode => 0664)
catalog.add_resource described_class.new(:path => target, :ensure => :directory, :mode => '0664')

catalog.apply

Expand All @@ -186,7 +186,7 @@ def get_aces_for_path_by_sid(path, sid)
target_path = tmpfile_with_contents('executable', '')
set_mode(0444, target_path)

catalog.add_resource described_class.new(:path => target_path, :ensure => :directory, :mode => 0666, :backup => false)
catalog.add_resource described_class.new(:path => target_path, :ensure => :directory, :mode => '0666', :backup => false)
catalog.apply

(get_mode(target_path) & 07777).should == 0777
Expand All @@ -196,7 +196,7 @@ def get_aces_for_path_by_sid(path, sid)

describe "for files" do
it "should not set executable bits" do
catalog.add_resource described_class.new(:path => path, :ensure => :file, :mode => 0666)
catalog.add_resource described_class.new(:path => path, :ensure => :file, :mode => '0666')
catalog.apply

(get_mode(path) & 07777).should == 0666
Expand Down Expand Up @@ -229,7 +229,7 @@ def get_aces_for_path_by_sid(path, sid)
end

it "should not set the executable bit on the link nor the target" do
catalog.add_resource described_class.new(:path => link, :ensure => :link, :mode => 0666, :target => link_target, :links => :manage)
catalog.add_resource described_class.new(:path => link, :ensure => :link, :mode => '0666', :target => link_target, :links => :manage)

catalog.apply

Expand All @@ -240,7 +240,7 @@ def get_aces_for_path_by_sid(path, sid)
it "should ignore dangling symlinks (#6856)" do
File.delete(link_target)

catalog.add_resource described_class.new(:path => link, :ensure => :link, :mode => 0666, :target => link_target, :links => :manage)
catalog.add_resource described_class.new(:path => link, :ensure => :link, :mode => '0666', :target => link_target, :links => :manage)
catalog.apply

Puppet::FileSystem.exist?(link).should be_false
Expand All @@ -265,7 +265,7 @@ def get_aces_for_path_by_sid(path, sid)
Puppet::FileSystem.symlink(target, link)
File.delete(target)

catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0600, :links => :follow)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0600', :links => :follow)
catalog.apply
end

Expand All @@ -284,7 +284,7 @@ def get_aces_for_path_by_sid(path, sid)

describe "that is readable" do
it "should set the executable bits when creating the destination (#10315)" do
catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0666, :links => :follow)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0666', :links => :follow)
catalog.apply

File.should be_directory(path)
Expand All @@ -294,7 +294,7 @@ def get_aces_for_path_by_sid(path, sid)
it "should set the executable bits when overwriting the destination (#10315)" do
FileUtils.touch(path)

catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0666, :links => :follow, :backup => false)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0666', :links => :follow, :backup => false)
catalog.apply

File.should be_directory(path)
Expand All @@ -313,7 +313,7 @@ def get_aces_for_path_by_sid(path, sid)
end

it "should set executable bits when creating the destination (#10315)" do
catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0666, :links => :follow)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0666', :links => :follow)
catalog.apply

File.should be_directory(path)
Expand All @@ -323,7 +323,7 @@ def get_aces_for_path_by_sid(path, sid)
it "should set executable bits when overwriting the destination" do
FileUtils.touch(path)

catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0666, :links => :follow, :backup => false)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0666', :links => :follow, :backup => false)
catalog.apply

File.should be_directory(path)
Expand All @@ -342,7 +342,7 @@ def get_aces_for_path_by_sid(path, sid)
end

it "should create the file, not a symlink (#2817, #10315)" do
catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0600, :links => :follow)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0600', :links => :follow)
catalog.apply

File.should be_file(path)
Expand All @@ -352,7 +352,7 @@ def get_aces_for_path_by_sid(path, sid)
it "should overwrite the file" do
FileUtils.touch(path)

catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0600, :links => :follow)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0600', :links => :follow)
catalog.apply

File.should be_file(path)
Expand All @@ -378,7 +378,7 @@ def get_aces_for_path_by_sid(path, sid)

describe "when following all links" do
it "should create the destination and apply executable bits (#10315)" do
catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0600, :links => :follow)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0600', :links => :follow)
catalog.apply

File.should be_directory(path)
Expand All @@ -388,7 +388,7 @@ def get_aces_for_path_by_sid(path, sid)
it "should overwrite the destination and apply executable bits" do
FileUtils.mkdir(path)

catalog.add_resource described_class.new(:path => path, :source => link, :mode => 0600, :links => :follow)
catalog.add_resource described_class.new(:path => path, :source => link, :mode => '0600', :links => :follow)
catalog.apply

File.should be_directory(path)
Expand Down Expand Up @@ -536,7 +536,7 @@ def build_path(dir)
it "should be able to recurse over a nonexistent file" do
@file = described_class.new(
:name => path,
:mode => 0644,
:mode => '0644',
:recurse => true,
:backup => false
)
Expand All @@ -553,7 +553,7 @@ def build_path(dir)

file = described_class.new(
:name => path,
:mode => 0644,
:mode => '0644',
:recurse => true,
:backup => false
)
Expand Down Expand Up @@ -648,18 +648,17 @@ def build_path(dir)
it "should recursively manage files even if there is an explicit file whose name is a prefix of the managed file" do
managed = File.join(path, "file")
generated = File.join(path, "file_with_a_name_starting_with_the_word_file")
managed_mode = 0700

FileUtils.mkdir_p(path)
FileUtils.touch(managed)
FileUtils.touch(generated)

catalog.add_resource described_class.new(:name => path, :recurse => true, :backup => false, :mode => managed_mode)
catalog.add_resource described_class.new(:name => path, :recurse => true, :backup => false, :mode => '0700')
catalog.add_resource described_class.new(:name => managed, :recurse => true, :backup => false, :mode => "644")

catalog.apply

(get_mode(generated) & 007777).should == managed_mode
(get_mode(generated) & 007777).should == 0700
end

describe "when recursing remote directories" do
Expand Down Expand Up @@ -981,7 +980,7 @@ def build_path(dir)
:ensure => :file,
:source => source,
:backup => false,
:mode => 0440
:mode => '0440'
)

catalog.add_resource file
Expand Down
2 changes: 1 addition & 1 deletion spec/unit/provider/file/posix_spec.rb
View file
Expand Up @@ -6,7 +6,7 @@
include PuppetSpec::Files

let(:path) { tmpfile('posix_file_spec') }
let(:resource) { Puppet::Type.type(:file).new :path => path, :mode => 0777, :provider => described_class.name }
let(:resource) { Puppet::Type.type(:file).new :path => path, :mode => '0777', :provider => described_class.name }
let(:provider) { resource.provider }

describe "#mode" do
Expand Down
2 changes: 1 addition & 1 deletion spec/unit/type/file/ensure_spec.rb
View file
Expand Up @@ -88,7 +88,7 @@
end

it "should accept octal mode as fixnum" do
resource[:mode] = 0700
resource[:mode] = '0700'
resource.expects(:property_fix)
Dir.expects(:mkdir).with(path, 0700)

Expand Down
6 changes: 4 additions & 2 deletions spec/unit/type/file/mode_spec.rb
View file
Expand Up @@ -10,8 +10,10 @@
let(:mode) { resource.property(:mode) }

describe "#validate" do
it "should accept values specified as integers" do
expect { mode.value = 0755 }.not_to raise_error
it "should reject non-string values" do
expect {
mode.value = 0755
}.to raise_error(Puppet::Error, /The file mode specification must be a string, not 'Fixnum'/)
end

it "should accept values specified as octal numbers in strings" do
Expand Down
4 changes: 2 additions & 2 deletions spec/unit/type/file/source_spec.rb
View file
Expand Up @@ -249,14 +249,14 @@
it "should not copy the metadata's owner, group, checksum and mode to the resource if they are already set" do
@resource[:owner] = 1
@resource[:group] = 2
@resource[:mode] = 3
@resource[:mode] = '173'
@resource[:content] = "foobar"

@source.copy_source_values

@resource[:owner].must == 1
@resource[:group].must == 2
@resource[:mode].must == "3"
@resource[:mode].must == '173'
@resource[:content].should_not == @metadata.checksum
end

Expand Down
2 changes: 1 addition & 1 deletion spec/unit/type/file_spec.rb
View file
Expand Up @@ -1219,7 +1219,7 @@

describe "#property_fix" do
{
:mode => 0777,
:mode => '0777',
:owner => 'joeuser',
:group => 'joeusers',
:seluser => 'seluser',
Expand Down

0 comments on commit 7d5aa0d

Please sign in to comment.