Fixed #791 - You should now be able to create and find a user/group in one transaction.

The real problem was that the 'gid' and 'uid' methods didn't handle
the case where 'get_posix_field' didn't return a value, and
the subsequent 'get_posix_field' calls couldn't handle that.

This commit moves the tests for Posix to spec, and fixes the
specific bug.

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

4 files changed, 268 insertions, 194 deletions

diff --git a/CHANGELOG b/CHANGELOG
index f4b50ec06..87b5c0969 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -19,6 +19,8 @@
 
     Fixed #1622 - Users and their groups should again add in one transaction
 
+    Fixed #791 - You should now be able to create and find a user/group in one transaction.
+
     Fixed #1610 - Raise "Filebucketed" messages to Notice priority
 
     Added a number of confines to package providers
diff --git a/lib/puppet/util/posix.rb b/lib/puppet/util/posix.rb
index 9281169ea..b969a041c 100755
--- a/lib/puppet/util/posix.rb
+++ b/lib/puppet/util/posix.rb
@@ -7,10 +7,8 @@ module Puppet::Util::POSIX
     # method search_posix_field in the gid and uid methods if a sanity check
     # fails
     def get_posix_field(space, field, id)
-        unless id
-            raise ArgumentError, "Did not get id"
-        end
-        prefix = "get" + space.to_s
+        raise Puppet::DevError, "Did not get id from caller" unless id
+
         if id.is_a?(Integer)
             if id > Puppet[:maximum_uid].to_i
                 Puppet.err "Tried to get %s field for silly id %s" % [field, id]
@@ -132,16 +130,16 @@ module Puppet::Util::POSIX
     # Get the GID of a given group, provided either a GID or a name
     def gid(group)
         begin
-          group = Integer(group)
+            group = Integer(group)
         rescue ArgumentError
-          # pass
+            # pass
         end
         if group.is_a?(Integer)
-            name = get_posix_field(:group, :name, group)
+            return nil unless name = get_posix_field(:group, :name, group)
             gid = get_posix_field(:group, :gid, name)
             check_value = gid
         else
-            gid = get_posix_field(:group, :gid, group)
+            return nil unless gid = get_posix_field(:group, :gid, group)
             name = get_posix_field(:group, :name, gid)
             check_value = name
         end
@@ -155,16 +153,16 @@ module Puppet::Util::POSIX
     # Get the UID of a given user, whether a UID or name is provided
     def uid(user)
         begin
-          user = Integer(user)
+            user = Integer(user)
         rescue ArgumentError
-          # pass
+            # pass
         end
         if user.is_a?(Integer)
-            name = get_posix_field(:passwd, :name, user)
+            return nil unless name = get_posix_field(:passwd, :name, user)
             uid = get_posix_field(:passwd, :uid, name)
             check_value = uid
         else
-            uid = get_posix_field(:passwd, :uid, user)
+            return nil unless uid = get_posix_field(:passwd, :uid, user)
             name = get_posix_field(:passwd, :name, uid)
             check_value = name
         end
diff --git a/spec/unit/util/posix.rb b/spec/unit/util/posix.rb
new file mode 100755
index 000000000..95a5608bd
--- /dev/null
+++ b/spec/unit/util/posix.rb
@@ -0,0 +1,256 @@
+#!/usr/bin/env ruby
+
+Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
+
+require 'puppet/util/posix'
+
+class PosixTest
+    include Puppet::Util::POSIX
+end
+
+describe Puppet::Util::POSIX do
+    before do
+        @posix = PosixTest.new
+    end
+
+    [:group, :gr].each do |name|
+        it "should return :gid as the field for %s" % name do
+            @posix.idfield(name).should == :gid
+        end
+
+        it "should return :getgrgid as the id method for %s" % name do
+            @posix.methodbyid(name).should == :getgrgid
+        end
+
+        it "should return :getgrnam as the name method for %s" % name do
+            @posix.methodbyname(name).should == :getgrnam
+        end
+    end
+
+    [:user, :pw, :passwd].each do |name|
+        it "should return :uid as the field for %s" % name do
+            @posix.idfield(name).should == :uid
+        end
+
+        it "should return :getpwuid as the id method for %s" % name do
+            @posix.methodbyid(name).should == :getpwuid
+        end
+
+        it "should return :getpwnam as the name method for %s" % name do
+            @posix.methodbyname(name).should == :getpwnam
+        end
+    end
+
+    describe "when retrieving a posix field" do
+        before do
+            @thing = stub 'thing', :field => "asdf"
+        end
+
+        it "should fail if no id was passed" do
+            lambda { @posix.get_posix_field("asdf", "bar", nil) }.should raise_error(Puppet::DevError)
+        end
+
+        describe "and the id is an integer" do
+            it "should log an error and return nil if the specified id is greater than the maximum allowed ID" do
+                Puppet.settings.expects(:value).with(:maximum_uid).returns 100
+                Puppet.expects(:err)
+
+                @posix.get_posix_field("asdf", "bar", 200).should be_nil
+            end
+
+            it "should use the method return by :methodbyid and return the specified field" do
+                Etc.expects(:getgrgid).returns @thing
+
+                @thing.expects(:field).returns "myval"
+
+                @posix.get_posix_field(:gr, :field, 200).should == "myval"
+            end
+
+            it "should return nil if the method throws an exception" do
+                Etc.expects(:getgrgid).raises ArgumentError
+
+                @thing.expects(:field).never
+
+                @posix.get_posix_field(:gr, :field, 200).should be_nil
+            end
+        end
+
+        describe "and the id is not an integer" do
+            it "should use the method return by :methodbyid and return the specified field" do
+                Etc.expects(:getgrnam).returns @thing
+
+                @thing.expects(:field).returns "myval"
+
+                @posix.get_posix_field(:gr, :field, "asdf").should == "myval"
+            end
+
+            it "should return nil if the method throws an exception" do
+                Etc.expects(:getgrnam).raises ArgumentError
+
+                @thing.expects(:field).never
+
+                @posix.get_posix_field(:gr, :field, "asdf").should be_nil
+            end
+        end
+    end
+
+    describe "when returning the gid" do
+        before do
+            @posix.stubs(:get_posix_field)
+        end
+
+        describe "and the group is an integer" do
+            it "should convert integers specified as a string into an integer" do
+                @posix.expects(:get_posix_field).with(:group, :name, 100)
+
+                @posix.gid("100")
+            end
+
+            it "should look up the name for the group" do
+                @posix.expects(:get_posix_field).with(:group, :name, 100)
+
+                @posix.gid(100)
+            end
+
+            it "should return nil if the group cannot be found" do
+                @posix.expects(:get_posix_field).once.returns nil
+                @posix.expects(:search_posix_field).never
+
+                @posix.gid(100).should be_nil
+            end
+
+            it "should use the found name to look up the id" do
+                @posix.expects(:get_posix_field).with(:group, :name, 100).returns "asdf"
+                @posix.expects(:get_posix_field).with(:group, :gid, "asdf").returns 100
+
+                @posix.gid(100).should == 100
+            end
+
+            # LAK: This is because some platforms have a broken Etc module that always return
+            # the same group.
+            it "should use :search_posix_field if the discovered id does not match the passed-in id" do
+                @posix.expects(:get_posix_field).with(:group, :name, 100).returns "asdf"
+                @posix.expects(:get_posix_field).with(:group, :gid, "asdf").returns 50
+
+                @posix.expects(:search_posix_field).with(:group, :gid, 100).returns "asdf"
+
+                @posix.gid(100).should == "asdf"
+            end
+        end
+
+        describe "and the group is a string" do
+            it "should look up the gid for the group" do
+                @posix.expects(:get_posix_field).with(:group, :gid, "asdf")
+
+                @posix.gid("asdf")
+            end
+
+            it "should return nil if the group cannot be found" do
+                @posix.expects(:get_posix_field).once.returns nil
+                @posix.expects(:search_posix_field).never
+
+                @posix.gid("asdf").should be_nil
+            end
+
+            it "should use the found gid to look up the nam" do
+                @posix.expects(:get_posix_field).with(:group, :gid, "asdf").returns 100
+                @posix.expects(:get_posix_field).with(:group, :name, 100).returns "asdf"
+
+                @posix.gid("asdf").should == 100
+            end
+
+            it "should use :search_posix_field if the discovered name does not match the passed-in name" do
+                @posix.expects(:get_posix_field).with(:group, :gid, "asdf").returns 100
+                @posix.expects(:get_posix_field).with(:group, :name, 100).returns "boo"
+
+                @posix.expects(:search_posix_field).with(:group, :gid, "asdf").returns "asdf"
+
+                @posix.gid("asdf").should == "asdf"
+            end
+        end
+    end
+
+    describe "when returning the uid" do
+        before do
+            @posix.stubs(:get_posix_field)
+        end
+
+        describe "and the group is an integer" do
+            it "should convert integers specified as a string into an integer" do
+                @posix.expects(:get_posix_field).with(:passwd, :name, 100)
+
+                @posix.uid("100")
+            end
+
+            it "should look up the name for the group" do
+                @posix.expects(:get_posix_field).with(:passwd, :name, 100)
+
+                @posix.uid(100)
+            end
+
+            it "should return nil if the group cannot be found" do
+                @posix.expects(:get_posix_field).once.returns nil
+                @posix.expects(:search_posix_field).never
+
+                @posix.uid(100).should be_nil
+            end
+
+            it "should use the found name to look up the id" do
+                @posix.expects(:get_posix_field).with(:passwd, :name, 100).returns "asdf"
+                @posix.expects(:get_posix_field).with(:passwd, :uid, "asdf").returns 100
+
+                @posix.uid(100).should == 100
+            end
+
+            # LAK: This is because some platforms have a broken Etc module that always return
+            # the same group.
+            it "should use :search_posix_field if the discovered id does not match the passed-in id" do
+                @posix.expects(:get_posix_field).with(:passwd, :name, 100).returns "asdf"
+                @posix.expects(:get_posix_field).with(:passwd, :uid, "asdf").returns 50
+
+                @posix.expects(:search_posix_field).with(:passwd, :uid, 100).returns "asdf"
+
+                @posix.uid(100).should == "asdf"
+            end
+        end
+
+        describe "and the group is a string" do
+            it "should look up the uid for the group" do
+                @posix.expects(:get_posix_field).with(:passwd, :uid, "asdf")
+
+                @posix.uid("asdf")
+            end
+
+            it "should return nil if the group cannot be found" do
+                @posix.expects(:get_posix_field).once.returns nil
+                @posix.expects(:search_posix_field).never
+
+                @posix.uid("asdf").should be_nil
+            end
+
+            it "should use the found uid to look up the nam" do
+                @posix.expects(:get_posix_field).with(:passwd, :uid, "asdf").returns 100
+                @posix.expects(:get_posix_field).with(:passwd, :name, 100).returns "asdf"
+
+                @posix.uid("asdf").should == 100
+            end
+
+            it "should use :search_posix_field if the discovered name does not match the passed-in name" do
+                @posix.expects(:get_posix_field).with(:passwd, :uid, "asdf").returns 100
+                @posix.expects(:get_posix_field).with(:passwd, :name, 100).returns "boo"
+
+                @posix.expects(:search_posix_field).with(:passwd, :uid, "asdf").returns "asdf"
+
+                @posix.uid("asdf").should == "asdf"
+            end
+        end
+    end
+
+    it "should be able to iteratively search for posix values" do
+        @posix.should respond_to(:search_posix_field)
+    end
+
+    describe "when searching for posix values iteratively" do
+        it "should iterate across all of the structs returned by Etc and return the appropriate field from the first matching value"
+    end
+end
diff --git a/test/util/posixtest.rb b/test/util/posixtest.rb
deleted file mode 100755
index f64a95d18..000000000
--- a/test/util/posixtest.rb
+++ /dev/null
@@ -1,182 +0,0 @@
-#!/usr/bin/env ruby
-#
-#  Created by Luke Kanies on 2006-11-07.
-#  Copyright (c) 2006. All rights reserved.
-
-require File.dirname(__FILE__) + '/../lib/puppettest'
-
-require 'puppettest'
-require 'puppet/util/posix'
-
-class TestPosixUtil < Test::Unit::TestCase
-	include PuppetTest
-	include Puppet::Util::POSIX
-	
-	def mk_posix_resource(type, obj)
-	    field = idfield(type)
-        res = Puppet::Type.type(type).create(
-            :name => obj.name,
-            field => obj.send(field)
-        )
-        res.setdefaults
-        res
-    end
-	
-	def test_get_posix_field
-	    {:group => nonrootgroup, :passwd => nonrootuser}.each do |space, obj|
-            id = Puppet::Util.idfield(space)
-	        [obj.name, obj.send(id)].each do |test|
-        	    value = nil
-        	    assert_nothing_raised do
-        	        value = get_posix_field(space, :name, test)
-        	    end
-        	    assert_equal(obj.name, value, "did not get correct value from get_posix_field (known to be broken on some platforms)")
-    	    end
-	    end
-    end
-    
-    def test_search_posix_field
-        {:group => nonrootgroup, :passwd => nonrootuser}.each do |space, obj|
-              id = Puppet::Util.idfield(space)
-            [obj.name, obj.send(id)].each do |test|
-                value = nil
-                assert_nothing_raised do
-                    value = search_posix_field(space, :name, test)
-                end
-                assert_equal(obj.name, value, "did not get correct value from search_posix_field")
-            end
-        end
-      end
-    
-    def test_get_provider_value
-        user = nonrootuser
-        obj = mk_posix_resource(:user, user)
-        
-        assert_nothing_raised do
-            assert_equal(user.uid, get_provider_value(:user, :uid, user.uid))
-            assert_equal(user.name, get_provider_value(:user, :name, user.name))
-        end
-    end
-    
-    def test_gid_and_uid
-        {:user => nonrootuser, :group => nonrootgroup}.each do |type, obj|
-            method = idfield(type)
-            # First make sure we get it back with both name and id with no object
-            [obj.name, obj.send(method)].each do |value|
-                assert_equal(obj.send(method), send(method, value))
-            end
-            
-            # Now make a Puppet resource and make sure we get it from that.
-            resource = mk_posix_resource(type, obj)
-            
-            [obj.name, obj.send(method)].each do |value|
-                assert_equal(obj.send(method), send(method, value))
-            end
-        end
-    end
-    
-    def test_util_methods
-        assert(Puppet::Util.respond_to?(:uid), "util does not have methods")
-    end
-    
-    # First verify we can convert a known user
-    def test_gidbyname
-        %x{groups}.split(" ").each { |group|
-            gid = nil
-            assert_nothing_raised {
-                gid = Puppet::Util.gid(group)
-            }
-
-            assert(gid, "Could not retrieve gid for %s" % group)
-        }
-    end
-
-    # Then verify we can retrieve a known group by gid
-    def test_gidbyid
-        %x{groups}.split(" ").each { |group|
-            obj = Puppet.type(:group).create(
-                :name => group,
-                :check => [:gid]
-            )
-            obj.setdefaults
-            current = obj.retrieve
-            id = nil
-            current.find { |prop, value| id = value if prop.name == :gid }
-            gid = nil
-            assert_nothing_raised {
-                gid = Puppet::Util.gid(id)
-            }
-
-            assert(gid, "Could not retrieve gid for %s" % group)
-            assert_equal(id, gid, "Got mismatched ids")
-        }
-    end
-
-    # Finally, verify that we can find groups by id even if we don't
-    # know them
-    def test_gidbyunknownid
-        gid = nil
-        group = Puppet::Util::SUIDManager.gid
-        assert_nothing_raised {
-            gid = Puppet::Util.gid(group)
-        }
-
-        assert(gid, "Could not retrieve gid for %s" % group)
-        assert_equal(group, gid, "Got mismatched ids")
-    end
-
-    def user
-        require 'etc'
-        unless defined? @user
-            obj = Etc.getpwuid(Puppet::Util::SUIDManager.uid)
-            @user = obj.name
-        end
-        return @user
-    end
-
-    # And do it all over again for users
-    # First verify we can convert a known user
-    def test_uidbyname
-        user = user()
-        uid = nil
-        assert_nothing_raised {
-            uid = Puppet::Util.uid(user)
-        }
-
-        assert(uid, "Could not retrieve uid for %s" % user)
-        assert_equal(Puppet::Util::SUIDManager.uid, uid, "UIDs did not match")
-    end
-
-    # Then verify we can retrieve a known user by uid
-    def test_uidbyid
-        user = user()
-        obj = Puppet.type(:user).create(
-            :name => user,
-            :check => [:uid]
-        )
-        obj.setdefaults
-        obj.retrieve
-        id = obj.provider.uid
-        uid = nil
-        assert_nothing_raised {
-            uid = Puppet::Util.uid(id)
-        }
-
-        assert(uid, "Could not retrieve uid for %s" % user)
-        assert_equal(id, uid, "Got mismatched ids")
-    end
-
-    # Finally, verify that we can find users by id even if we don't
-    # know them
-    def test_uidbyunknownid
-        uid = nil
-        user = Puppet::Util::SUIDManager.uid
-        assert_nothing_raised {
-            uid = Puppet::Util.uid(user)
-        }
-
-        assert(uid, "Could not retrieve uid for %s" % user)
-        assert_equal(user, uid, "Got mismatched ids")
-    end
-end
-