Skip to content
Permalink
Browse files

(PUP-1174) Use Puppet::Util.absolute_path? to validate purge_ssh_keys

Previously, the `purge_ssh_keys` parameter was validated as either starting
with '/', '~' or '%h', but the first one is not a valid absolute path on
Windows.

This commit uses the Puppet::Util.absolute_path? to check for absoluteness.
Also updates the spec tests to be specific about the type of exception and
message that they expect. And in cases where we don't expect an exception, we
don't need to expect { .. }.to_not raise_error, since rspec will fail any
test that does raise.

Paired-with: Charlie Sharpsteen <chuck@puppetlabs.com>
  • Loading branch information
joshcooper committed Mar 12, 2014
1 parent fd0b12e commit a71ab022f7fee60ac1589beb27da972d33660728
Showing with 31 additions and 11 deletions.
  1. +4 −1 lib/puppet/type/user.rb
  2. +27 −10 spec/unit/type/user_spec.rb
@@ -593,8 +593,11 @@ def eval_generate
value = [ value ] if value.is_a?(String)
if value.is_a?(Array)
value.each do |entry|

raise ArgumentError, "Each entry for purge_ssh_keys must be a string, not a #{entry.class}" unless entry.is_a?(String)
raise ArgumentError, "Paths to keyfiles must be absolute, not #{entry}" unless entry =~ %r{^/|^~/|^%h/}

valid_home = Puppet::Util.absolute_path?(entry) || entry =~ %r{^~/|^%h/}
raise ArgumentError, "Paths to keyfiles must be absolute, not #{entry}" unless valid_home
end
return
end
@@ -418,36 +418,52 @@ def check_valid_shell; end

describe "when purging ssh keys" do
it "should not accept a keyfile with a relative path" do
expect { described_class.new(:name => "a", :purge_ssh_keys => "keys") }.to raise_error
expect {
described_class.new(:name => "a", :purge_ssh_keys => "keys")
}.to raise_error(Puppet::Error, /Paths to keyfiles must be absolute, not keys/)
end

context "with a home directory specified" do
it "should accept true" do
expect { described_class.new(:name => "a", :home => "/tmp", :purge_ssh_keys => true) }.to_not raise_error
described_class.new(:name => "a", :home => "/tmp", :purge_ssh_keys => true)
end
it "should accept the ~ wildcard" do
expect { described_class.new(:name => "a", :home => "/tmp", :purge_ssh_keys => "~/keys") }.to_not raise_error
described_class.new(:name => "a", :home => "/tmp", :purge_ssh_keys => "~/keys")
end
it "should accept the %h wildcard" do
expect { described_class.new(:name => "a", :home => "/tmp", :purge_ssh_keys => "%h/keys") }.to_not raise_error
described_class.new(:name => "a", :home => "/tmp", :purge_ssh_keys => "%h/keys")
end
it "raises when given a relative path" do
expect {
described_class.new(:name => "a", :home => "/tmp", :purge_ssh_keys => "keys")
}.to raise_error(Puppet::Error, /Paths to keyfiles must be absolute/)
end
end

context "with no home directory specified" do
it "should not accept true" do
expect { described_class.new(:name => "a", :purge_ssh_keys => true) }.to raise_error
expect {
described_class.new(:name => "a", :purge_ssh_keys => true)
}.to raise_error(Puppet::Error, /purge_ssh_keys can only be true for users with a defined home directory/)
end
it "should not accept the ~ wildcard" do
expect { described_class.new(:name => "a", :purge_ssh_keys => "~/keys") }.to raise_error
expect {
described_class.new(:name => "a", :purge_ssh_keys => "~/keys")
}.to raise_error(Puppet::Error, /meta character ~ or %h only allowed for users with a defined home directory/)
end
it "should not accept the %h wildcard" do
expect { described_class.new(:name => "a", :purge_ssh_keys => "%h/keys") }.to raise_error
expect {
described_class.new(:name => "a", :purge_ssh_keys => "%h/keys")
}.to raise_error(Puppet::Error, /meta character ~ or %h only allowed for users with a defined home directory/)
end
end

context "with a valid parameter" do
let(:paths) do
[ "/dev/null", "/tmp/keyfile" ].map { |path| File.expand_path(path) }
end
subject do
res = described_class.new(:name => "test", :purge_ssh_keys => [ "/dev/null", "/tmp/keyfile" ])
res = described_class.new(:name => "test", :purge_ssh_keys => paths)
res.catalog = Puppet::Resource::Catalog.new
res
end
@@ -456,8 +472,9 @@ def check_valid_shell; end
subject.eval_generate
end
it "should check each keyfile for readability" do
File.expects(:readable?).with("/dev/null")
File.expects(:readable?).with("/tmp/keyfile")
paths.each do |path|
File.expects(:readable?).with(path)
end
subject.eval_generate
end
end

0 comments on commit a71ab02

Please sign in to comment.
You can’t perform that action at this time.