Please review pull request #528: #1076: User Type: Show warning if an empty group is specified opened by (stschulte)
Description:
If one specifies an empty string for the group property of the user type, puppet builds an incorrect usermod command.
Example taken from #1076
user { "pinky":
gid => "pinky",
ensure => present,
groups => ""
}
The correct way to remove all group membership is to use
groups => []so we should guide the user in that direction.The commit series also tries to clean up the user_spec.
- Opened: Mon Feb 27 00:33:50 UTC 2012
- Based on: puppetlabs:2.7.x (5f0f269428587e97dabaca1fa812f7c5d616ec29)
- Requested merge: stschulte:ticket/2.7.x/1076 (c6d656ae204c0a4f4ee34bfa1a3f11b20deb0336)
Diff follows:
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 6556a24..f3496a8 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -250,6 +250,7 @@ def should_to_s( newvalue )
raise ArgumentError, "Group names must be provided, not GID numbers."
end
raise ArgumentError, "Group names must be provided as an array, not a comma-separated list." if value.include?(",")
+ raise ArgumentError, "Group names must not be empty. If you want to specify \"no groups\" pass an empty array" if value.empty?
end
end
diff --git a/spec/unit/type/user_spec.rb b/spec/unit/type/user_spec.rb
index 2ca8d88..9f83e4e 100755
--- a/spec/unit/type/user_spec.rb
+++ b/spec/unit/type/user_spec.rb
@@ -1,60 +1,68 @@
#!/usr/bin/env rspec
require 'spec_helper'
-user = Puppet::Type.type(:user)
-
-describe user do
- before do
- ENV["PATH"] += File::PATH_SEPARATOR + "/usr/sbin" unless ENV["PATH"].split(File::PATH_SEPARATOR).include?("/usr/sbin")
- @provider = stub 'provider'
- @resource = stub 'resource', :resource => nil, :provider => @provider, :line => nil, :file => nil
+describe Puppet::Type.type(:user) do
+ before :each do
+ @provider_class = described_class.provide(:simple) do
+ has_features :manages_expiry, :manages_password_age, :manages_passwords, :manages_solaris_rbac
+ mk_resource_methods
+ def create; end
+ def delete; end
+ def exists?; get(:ensure) != :absent; end
+ def flush; end
+ def self.instances; []; end
+ end
+ described_class.stubs(:defaultprovider).returns @provider_class
end
it "should have a default provider inheriting from Puppet::Provider" do
- user.defaultprovider.ancestors.should be_include(Puppet::Provider)
+ described_class.defaultprovider.ancestors.should be_include(Puppet::Provider)
end
it "should be able to create a instance" do
- user.new(:name => "foo").should_not be_nil
+ described_class.new(:name => "foo").should_not be_nil
end
it "should have an allows_duplicates feature" do
- user.provider_feature(:allows_duplicates).should_not be_nil
+ described_class.provider_feature(:allows_duplicates).should_not be_nil
end
it "should have an manages_homedir feature" do
- user.provider_feature(:manages_homedir).should_not be_nil
+ described_class.provider_feature(:manages_homedir).should_not be_nil
end
it "should have an manages_passwords feature" do
- user.provider_feature(:manages_passwords).should_not be_nil
+ described_class.provider_feature(:manages_passwords).should_not be_nil
end
it "should have a manages_solaris_rbac feature" do
- user.provider_feature(:manages_solaris_rbac).should_not be_nil
+ described_class.provider_feature(:manages_solaris_rbac).should_not be_nil
end
it "should have a manages_expiry feature" do
- user.provider_feature(:manages_expiry).should_not be_nil
+ described_class.provider_feature(:manages_expiry).should_not be_nil
end
it "should have a manages_password_age feature" do
- user.provider_feature(:manages_password_age).should_not be_nil
+ described_class.provider_feature(:manages_password_age).should_not be_nil
end
it "should have a system_users feature" do
- user.provider_feature(:system_users).should_not be_nil
+ described_class.provider_feature(:system_users).should_not be_nil
end
describe "instances" do
it "should have a valid provider" do
- user.new(:name => "foo").provider.class.ancestors.should be_include(Puppet::Provider)
+ described_class.new(:name => "foo").provider.class.ancestors.should be_include(Puppet::Provider)
end
it "should delegate existence questions to its provider" do
- instance = user.new(:name => "foo")
- instance.provider.expects(:exists?).returns "eh"
- instance.exists?.should == "eh"
+ @provider = @provider_class.new(:name => 'foo', :ensure => :absent)
+ instance = described_class.new(:name => "foo", :provider => @provider)
+ instance.exists?.should == false
+
+ @provider.set(:ensure => :present)
+ instance.exists?.should == true
end
end
@@ -62,11 +70,11 @@
properties.each do |property|
it "should have a #{property} property" do
- user.attrclass(property).ancestors.should be_include(Puppet::Property)
+ described_class.attrclass(property).ancestors.should be_include(Puppet::Property)
end
it "should have documentation for its #{property} property" do
- user.attrclass(property).doc.should be_instance_of(String)
+ described_class.attrclass(property).doc.should be_instance_of(String)
end
end
@@ -74,28 +82,26 @@
list_properties.each do |property|
it "should have a list '#{property}'" do
- user.attrclass(property).ancestors.should be_include(Puppet::Property::List)
+ described_class.attrclass(property).ancestors.should be_include(Puppet::Property::List)
end
end
it "should have an ordered list 'profiles'" do
- user.attrclass(:profiles).ancestors.should be_include(Puppet::Property::OrderedList)
+ described_class.attrclass(:profiles).ancestors.should be_include(Puppet::Property::OrderedList)
end
it "should have key values 'keys'" do
- user.attrclass(:keys).ancestors.should be_include(Puppet::Property::KeyValue)
+ described_class.attrclass(:keys).ancestors.should be_include(Puppet::Property::KeyValue)
end
describe "when retrieving all current values" do
before do
- @user = user.new(:name => "foo", :uid => 10)
+ @provider = @provider_class.new(:name => 'foo', :ensure => :present, :uid => 15, :gid => 15)
+ @user = described_class.new(:name => "foo", :uid => 10, :provider => @provider)
end
it "should return a hash containing values for all set properties" do
@user[:gid] = 10
- @user.property(:ensure).expects(:retrieve).returns :present
- @user.property(:uid).expects(:retrieve).returns 15
- @user.property(:gid).expects(:retrieve).returns 15
values = @user.retrieve
[@user.property(:uid), @user.property(:gid)].each { |property| values.should be_include(property) }
end
@@ -107,178 +113,190 @@
end
it "should include the result of retrieving each property's current value if the user is present" do
- @user.property(:ensure).expects(:retrieve).returns :present
- @user.property(:uid).expects(:retrieve).returns 15
@user.retrieve[@user.property(:uid)].should == 15
end
end
describe "when managing the ensure property" do
- before do
- @ensure = user.attrclass(:ensure).new(:resource => @resource)
- end
-
it "should support a :present value" do
- lambda { @ensure.should = :present }.should_not raise_error
+ lambda { described_class.new(:name => 'foo', :ensure => :present) }.should_not raise_error
end
it "should support an :absent value" do
- lambda { @ensure.should = :absent }.should_not raise_error
+ lambda { described_class.new(:name => 'foo', :ensure => :absent) }.should_not raise_error
end
it "should call :create on the provider when asked to sync to the :present state" do
+ @provider = @provider_class.new(:name => 'foo', :ensure => :absent)
@provider.expects(:create)
- @ensure.should = :present
- @ensure.sync
+ described_class.new(:name => 'foo', :ensure => :present, :provider => @provider).parameter(:ensure).sync
end
it "should call :delete on the provider when asked to sync to the :absent state" do
+ @provider = @provider_class.new(:name => 'foo', :ensure => :present)
@provider.expects(:delete)
- @ensure.should = :absent
- @ensure.sync
+ described_class.new(:name => 'foo', :ensure => :absent, :provider => @provider).parameter(:ensure).sync
end
describe "and determining the current state" do
it "should return :present when the provider indicates the user exists" do
- @provider.expects(:exists?).returns true
- @ensure.retrieve.should == :present
+ @provider = @provider_class.new(:name => 'foo', :ensure => :present)
+ described_class.new(:name => 'foo', :ensure => :absent, :provider => @provider).parameter(:ensure).retrieve.should == :present
end
it "should return :absent when the provider indicates the user does not exist" do
- @provider.expects(:exists?).returns false
- @ensure.retrieve.should == :absent
+ @provider = @provider_class.new(:name => 'foo', :ensure => :absent)
+ described_class.new(:name => 'foo', :ensure => :present, :provider => @provider).parameter(:ensure).retrieve.should == :absent
end
end
end
describe "when managing the uid property" do
it "should convert number-looking strings into actual numbers" do
- uid = user.attrclass(:uid).new(:resource => @resource)
- uid.should = "50"
- uid.should.must == 50
+ described_class.new(:name => 'foo', :uid => '50')[:uid].should == 50
end
it "should support UIDs as numbers" do
- uid = user.attrclass(:uid).new(:resource => @resource)
- uid.should = 50
- uid.should.must == 50
+ described_class.new(:name => 'foo', :uid => 50)[:uid].should == 50
end
- it "should :absent as a value" do
- uid = user.attrclass(:uid).new(:resource => @resource)
- uid.should = :absent
- uid.should.must == :absent
+ it "should support :absent as a value" do
+ described_class.new(:name => 'foo', :uid => :absent)[:uid].should == :absent
end
end
describe "when managing the gid" do
- it "should :absent as a value" do
- gid = user.attrclass(:gid).new(:resource => @resource)
- gid.should = :absent
- gid.should.must == :absent
+ it "should support :absent as a value" do
+ described_class.new(:name => 'foo', :gid => :absent)[:gid].should == :absent
end
it "should convert number-looking strings into actual numbers" do
- gid = user.attrclass(:gid).new(:resource => @resource)
- gid.should = "50"
- gid.should.must == 50
+ described_class.new(:name => 'foo', :gid => '50')[:gid].should == 50
end
it "should support GIDs specified as integers" do
- gid = user.attrclass(:gid).new(:resource => @resource)
- gid.should = 50
- gid.should.must == 50
+ described_class.new(:name => 'foo', :gid => 50)[:gid].should == 50
end
it "should support groups specified by name" do
- gid = user.attrclass(:gid).new(:resource => @resource)
- gid.should = "foo"
- gid.should.must == "foo"
+ described_class.new(:name => 'foo', :gid => 'foo')[:gid].should == 'foo'
end
describe "when testing whether in sync" do
- before do
- @gid = user.attrclass(:gid).new(:resource => @resource, :should => %w{foo bar})
- end
-
it "should return true if no 'should' values are set" do
- @gid = user.attrclass(:gid).new(:resource => @resource)
-
- @gid.must be_safe_insync(500)
+ # this is currently not the case because gid has no default value, so we would never even
+ # call insync? on that property
+ if param = described_class.new(:name => 'foo').parameter(:gid)
+ param.must be_safe_insync(500)
+ end
end
it "should return true if any of the specified groups are equal to the current integer" do
Puppet::Util.expects(:gid).with("foo").returns 300
Puppet::Util.expects(:gid).with("bar").returns 500
-
- @gid.must be_safe_insync(500)
+ described_class.new(:name => 'baz', :gid => [ 'foo', 'bar' ]).parameter(:gid).must be_safe_insync(500)
end
it "should return false if none of the specified groups are equal to the current integer" do
Puppet::Util.expects(:gid).with("foo").returns 300
Puppet::Util.expects(:gid).with("bar").returns 500
-
- @gid.should_not be_safe_insync(700)
+ described_class.new(:name => 'baz', :gid => [ 'foo', 'bar' ]).parameter(:gid).must_not be_safe_insync(700)
end
end
describe "when syncing" do
- before do
- @gid = user.attrclass(:gid).new(:resource => @resource, :should => %w{foo bar})
- end
-
it "should use the first found, specified group as the desired value and send it to the provider" do
Puppet::Util.expects(:gid).with("foo").returns nil
Puppet::Util.expects(:gid).with("bar").returns 500
- @provider.expects(:gid=).with 500
+ @provider = @provider_class.new(:name => 'foo')
+ resource = described_class.new(:name => 'foo', :provider => @provider, :gid => [ 'foo', 'bar' ])
- @gid.sync
+ @provider.expects(:gid=).with 500
+ resource.parameter(:gid).sync
end
end
end
- describe "when managing expiry" do
- before do
- @expiry = user.attrclass(:expiry).new(:resource => @resource)
+ describe "when managing groups" do
+ it "should support a singe group" do
+ lambda { described_class.new(:name => 'foo', :groups => 'bar') }.should_not raise_error
end
- it "should fail if given an invalid date" do
- lambda { @expiry.should = "200-20-20" }.should raise_error(Puppet::Error)
+ it "should support multiple groups as an array" do
+ lambda { described_class.new(:name => 'foo', :groups => [ 'bar' ]) }.should_not raise_error
+ lambda { described_class.new(:name => 'foo', :groups => [ 'bar', 'baz' ]) }.should_not raise_error
+ end
+
+ it "should not support a comma separated list" do
+ lambda { described_class.new(:name => 'foo', :groups => 'bar,baz') }.should raise_error(Puppet::Error, /Group names must be provided as an array/)
+ end
+
+ it "should not support an empty string" do
+ lambda { described_class.new(:name => 'foo', :groups => '') }.should raise_error(Puppet::Error, /Group names must not be empty/)
+ end
+
+ describe "when testing is in sync" do
+
+ before :each do
+ # the useradd provider uses a single string to represent groups and so does Puppet::Property::List when converting to should values
+ @provider = @provider_class.new(:name => 'foo', :groups => 'a,b,e,f')
+ end
+
+ it "should not care about order" do
+ @property = described_class.new(:name => 'foo', :groups => [ 'a', 'c', 'b' ]).property(:groups)
+ @property.must be_safe_insync([ 'a', 'b', 'c' ])
+ @property.must be_safe_insync([ 'a', 'c', 'b' ])
+ @property.must be_safe_insync([ 'b', 'a', 'c' ])
+ @property.must be_safe_insync([ 'b', 'c', 'a' ])
+ @property.must be_safe_insync([ 'c', 'a', 'b' ])
+ @property.must be_safe_insync([ 'c', 'b', 'a' ])
+ end
+
+ it "should merge current value and desired value if membership minimal" do
+ @instance = described_class.new(:name => 'foo', :groups => [ 'a', 'c', 'b' ], :provider => @provider)
+ @instance[:membership] = :minimum
+ @instance[:groups].should == 'a,b,c,e,f'
+ end
+
+ it "should not treat a subset of groups insync if membership inclusive" do
+ @instance = described_class.new(:name => 'foo', :groups => [ 'a', 'c', 'b' ], :provider => @provider)
+ @instance[:membership] = :inclusive
+ @instance[:groups].should == 'a,b,c'
+ end
end
end
- describe "when managing minimum password age" do
- before do
- @age = user.attrclass(:password_min_age).new(:resource => @resource)
+
+ describe "when managing expiry" do
+ it "should fail if given an invalid date" do
+ lambda { described_class.new(:name => 'foo', :expiry => "200-20-20") }.should raise_error(Puppet::Error, /Expiry dates must be YYYY-MM-DD/)
end
+ end
+ describe "when managing minimum password age" do
it "should accept a negative minimum age" do
- expect { @age.should = -1 }.should_not raise_error
+ expect { described_class.new(:name => 'foo', :password_min_age => '-1') }.should_not raise_error
end
it "should fail with an empty minimum age" do
- expect { @age.should = '' }.should raise_error(Puppet::Error)
+ expect { described_class.new(:name => 'foo', :password_min_age => '') }.should raise_error(Puppet::Error, /minimum age must be provided as a number/)
end
end
describe "when managing maximum password age" do
- before do
- @age = user.attrclass(:password_max_age).new(:resource => @resource)
- end
-
it "should accept a negative maximum age" do
- expect { @age.should = -1 }.should_not raise_error
+ expect { described_class.new(:name => 'foo', :password_max_age => '-1') }.should_not raise_error
end
it "should fail with an empty maximum age" do
- expect { @age.should = '' }.should raise_error(Puppet::Error)
+ expect { described_class.new(:name => 'foo', :password_max_age => '') }.should raise_error(Puppet::Error, /maximum age must be provided as a number/)
end
end
describe "when managing passwords" do
before do
- @password = user.attrclass(:password).new(:resource => @resource, :should => "mypass")
+ @password = described_class.new(:name => 'foo', :password => 'mypass').parameter(:password)
end
it "should not include the password in the change log when adding the password" do
@@ -298,38 +316,24 @@
end
it "should fail if a ':' is included in the password" do
- lambda { @password.should = "some:thing" }.should raise_error(Puppet::Error)
+ lambda { described_class.new(:name => 'foo', :password => "some:thing") }.should raise_error(Puppet::Error, /Passwords cannot include ':'/)
end
it "should allow the value to be set to :absent" do
- lambda { @password.should = :absent }.should_not raise_error
+ lambda { described_class.new(:name => 'foo', :password => :absent) }.should_not raise_error
end
end
describe "when manages_solaris_rbac is enabled" do
- before do
- @provider.stubs(:satisfies?).returns(false)
- @provider.expects(:satisfies?).with([:manages_solaris_rbac]).returns(true)
- end
-
it "should support a :role value for ensure" do
- @ensure = user.attrclass(:ensure).new(:resource => @resource)
- lambda { @ensure.should = :role }.should_not raise_error
+ lambda { described_class.new(:name => 'foo', :ensure => :role) }.should_not raise_error
end
end
describe "when user has roles" do
- before do
- # To test this feature, we have to support it.
- user.new(:name => "foo").provider.class.stubs(:feature?).returns(true)
- end
-
it "should autorequire roles" do
- testuser = Puppet::Type.type(:user).new(:name => "testuser")
- testuser.provider.stubs(:send).with(:roles).returns("")
- testuser[:roles] = "testrole"
-
- testrole = Puppet::Type.type(:user).new(:name => "testrole")
+ testuser = described_class.new(:name => "testuser", :roles => ['testrole'] )
+ testrole = described_class.new(:name => "testrole")
config = Puppet::Resource::Catalog.new :testing do |conf|
[testuser, testrole].each { |resource| conf.add_resource resource }
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
