Fixing #1622 - The user type only looks up groups when necessary.

Also added a bunch of tests to the user type, and refactored
as necessary for this to work.

Signed-off-by: Luke Kanies <luke@madstop.com>

2 files changed, 154 insertions, 58 deletions

diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 039bcb7cb..79cd5ff26 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -49,25 +49,6 @@ module Puppet
                     return :absent
                 end
             end
-
-            # The default 'sync' method only selects among a list of registered
-            # values.
-            def sync
-#                if self.insync?
-#                    self.info "already in sync"
-#                    return nil
-                #else
-                    #self.info "%s vs %s" % [self.is.inspect, self.should.inspect]
-#               end
-                unless self.class.values
-                    self.devfail "No values defined for %s" %
-                        self.class.name
-                end
-
-                # Set ourselves to whatever our should value is.
-                self.set(self.should)
-            end
-
         end
 
         newproperty(:uid) do
@@ -95,50 +76,26 @@ module Puppet
         newproperty(:gid) do
             desc "The user's primary group.  Can be specified numerically or
                 by name."
-            
-            def found?
-                defined? @found and @found
-            end
-
-            munge do |gid|
-                method = :getgrgid
-                case gid
-                when String
-                    if gid =~ /^[-0-9]+$/
-                        gid = Integer(gid)
-                    else
-                        method = :getgrnam
-                    end
-                when Symbol
-                    unless gid == :auto or gid == :absent
-                        self.devfail "Invalid GID %s" % gid
-                    end
-                    # these are treated specially by sync()
-                    return gid
-                end
 
-                if group = Puppet::Util.gid(gid)
-                    @found = true
-                    return group
+            munge do |value|
+                if value.is_a?(String) and value =~ /^[-0-9]+$/
+                    Integer(value)
                 else
-                    @found = false
-                    return gid
+                    value
                 end
             end
 
-            # *shudder*  Make sure that we've looked up the group and gotten
-            # an ID for it.  Yuck-o.
-            def should
-                unless defined? @should
-                    return super
-                end
-                unless found?
-                    @should = @should.each { |val|
-                        next unless val
-                        Puppet::Util.gid(val)
-                    }
+            def sync
+                found = false
+                @should.each do |value|
+                    if number = Puppet::Util.gid(value)
+                        provider.gid = number
+                        found = true
+                        break
+                    end
                 end
-                super
+
+                fail "Could not find group(s) %s" % @should.join(",") unless found
             end
         end
 
diff --git a/spec/unit/type/user.rb b/spec/unit/type/user.rb
index 4576f560b..8fe16901e 100755
--- a/spec/unit/type/user.rb
+++ b/spec/unit/type/user.rb
@@ -5,6 +5,11 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 user = Puppet::Type.type(:user)
 
 describe user do
+    before do
+        @provider = stub 'provider'
+        @resource = stub 'resource', :resource => nil, :provider => @provider, :line => nil, :file => nil
+    end
+
     after { user.clear }
 
     it "should have a default provider inheriting from Puppet::Provider" do
@@ -15,10 +20,144 @@ describe user do
         user.create(:name => "foo").should_not be_nil
     end
 
-    describe "instances" do
+    it "should have an allows_duplicates feature" do
+        user.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
+    end
+
+    it "should have an manages_passwords feature" do
+        user.provider_feature(:manages_passwords).should_not be_nil
+    end
 
+    describe "instances" do
         it "should have a valid provider" do
             user.create(:name => "foo").provider.class.ancestors.should be_include(Puppet::Provider)
         end
     end
+
+    [:ensure, :uid, :gid, :home, :comment, :shell, :password, :groups].each do |property|
+        it "should have a %s property" % property do
+            user.attrclass(property).ancestors.should be_include(Puppet::Property)
+        end
+
+        it "should have documentation for its %s property" % property do
+            user.attrclass(property).doc.should be_instance_of(String)
+        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
+        end
+
+        it "should support an :absent value" do
+            lambda { @ensure.should = :absent }.should_not raise_error
+        end
+
+        it "should call :create on the provider when asked to sync to the :present state" do
+            @provider.expects(:create)
+            @ensure.should = :present
+            @ensure.sync
+        end
+
+        it "should call :delete on the provider when asked to sync to the :absent state" do
+            @provider.expects(:delete)
+            @ensure.should = :absent
+            @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
+            end
+
+            it "should return :absent when the provider indicates the user does not exist" do
+                @provider.expects(:exists?).returns false
+                @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
+        end
+
+        it "should support UIDs as numbers" do
+            uid = user.attrclass(:uid).new(:resource => @resource)
+            uid.should = 50
+            uid.should.must == 50
+        end
+
+        it "should :absent as a value" do
+            uid = user.attrclass(:uid).new(:resource => @resource)
+            uid.should = :absent
+            uid.should.must == :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
+        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
+        end
+
+        it "should support GIDs specified as integers" do
+            gid = user.attrclass(:gid).new(:resource => @resource)
+            gid.should = 50
+            gid.should.must == 50
+        end
+
+        it "should support groups specified by name" do
+            gid = user.attrclass(:gid).new(:resource => @resource)
+            gid.should = "foo"
+            gid.should.must == "foo"
+        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
+
+                @gid.sync
+            end
+        end
+    end
+
+    describe "when managing passwords" do
+        before do
+            @password = user.attrclass(:password).new(:resource => @resource, :should => "mypass")
+        end
+
+        it "should not include the password in the change log when adding the password" do
+            @password.change_to_s(:absent, "mypass").should_not be_include("mypass")
+        end
+
+        it "should not include the password in the change log when changing the password" do
+            @password.change_to_s("other", "mypass").should_not be_include("mypass")
+        end
+    end
 end