+ New assertion: assert_uid_gid will check that the UID and GID have been changed to the proper values. This uses a fork and a FIFO to achieve it's checking.

! nonrootuser and nonrootgroup now only return users/groups that are less than 255, due to the "Darwin debacle"
! many, many, many fixes for suidmanager. This is rather embarassing.


git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1674 980ebf18-57e1-0310-9a29-db15c13687c0

4 files changed, 92 insertions, 32 deletions

diff --git a/lib/puppet/suidmanager.rb b/lib/puppet/suidmanager.rb
index 2f4d428e3..abe99c8b2 100644
--- a/lib/puppet/suidmanager.rb
+++ b/lib/puppet/suidmanager.rb
@@ -1,17 +1,22 @@
 require 'facter'
-require 'puppet'
 
 module Puppet
     module SUIDManager
         platform = Facter["kernel"].value
-        [:uid=, :uid, :gid=, :gid].each do |method|
+        [:uid=, :gid=, :uid, :gid].each do |method|
             define_method(method) do |*args|
-                if platform == "Darwin" and (Facter['rubyversion'] <=> "1.8.5") < 0
-                    Puppet.warning "Cannot change real UID on Darwin on Ruby versions earlier than 1.8.5"
-                    method = ("e" + method.to_s).intern unless method.to_s[0] == 'e'
+                # NOTE: 'method' is closed here.
+                newmethod = method
+
+                if platform == "Darwin"
+                    if !@darwinwarned
+                        Puppet.warning "Cannot change real UID on Darwin"
+                        @darwinwarned = true
+                    end
+                    newmethod = ("e" + method.to_s).intern
                 end
 
-                return Process.send(method, *args)
+                return Process.send(newmethod, *args)
             end
             module_function method
         end
@@ -47,28 +52,39 @@ module Puppet
         module_function :run_and_capture
 
         def system(command, new_uid=self.euid, new_gid=self.egid)
+            status = nil
             asuser(new_uid, new_gid) do
                 Kernel.system(command)
+                status = $?.dup
             end
+            status
         end
         
         module_function :system
 
-        def asuser(new_euid, new_egid)
-            new_euid = Puppet::Util.uid(new_euid)
-            new_egid = Puppet::Util.uid(new_egid)
+        def asuser(new_euid=nil, new_egid=nil)
+            begin
+                old_egid = old_euid = nil
+                if new_egid
+                    new_egid = Puppet::Util.uid(new_egid)
+                    old_egid = self.egid
+                    self.egid = new_egid
+                end
+                if new_euid
+                    new_euid = Puppet::Util.uid(new_euid)
+                    old_euid = self.euid
+                    self.euid = new_euid
+                end
 
-            old_euid, old_egid = [ self.euid, self.egid ]
-            self.egid = new_egid ? new_egid : old_egid
-            self.euid = new_euid ? new_euid : old_euid
-            output = yield
-            self.egid = old_egid
-            self.euid = old_euid
+                output = yield
 
-            output
+                output
+            ensure
+                self.egid = old_egid
+                self.euid = old_euid
+            end
         end
 
         module_function :asuser
     end
 end
-
diff --git a/test/lib/puppettest/support/assertions.rb b/test/lib/puppettest/support/assertions.rb
index 0e272002e..d5c94fb90 100644
--- a/test/lib/puppettest/support/assertions.rb
+++ b/test/lib/puppettest/support/assertions.rb
@@ -1,6 +1,32 @@
 require 'puppettest'
+require 'fileutils'
 
 module PuppetTest
+
+    def assert_uid_gid(uid, gid, filename)
+        flunk "Must be uid 0 to run these tests" unless Process.uid == 0
+
+        fork do 
+            Puppet::SUIDManager.gid = gid
+            Puppet::SUIDManager.uid = uid
+            # FIXME: use the tempfile method from puppettest.rb
+            system("mkfifo "+filename)
+            f = File.open(filename, "w")
+            f << "#{Puppet::SUIDManager.uid}\n#{Puppet::SUIDManager.gid}\n"
+            yield if block_given?
+        end
+
+        # avoid a race.
+        true while !File.exists? filename
+
+        f = File.open(filename, "r")
+
+        a = f.readlines 
+        assert_equal(uid, a[0].chomp.to_i)
+        assert_equal(gid, a[1].chomp.to_i)
+        FileUtils.rm(filename)
+    end
+
     def assert_rollback_events(events, trans, msg = nil)
         run_events(:rollback, events, trans, msg)
     end
diff --git a/test/lib/puppettest/support/helpers.rb b/test/lib/puppettest/support/helpers.rb
index cbcbcb1f6..bd5356d53 100644
--- a/test/lib/puppettest/support/helpers.rb
+++ b/test/lib/puppettest/support/helpers.rb
@@ -1,9 +1,11 @@
 require 'puppettest'
 
 module PuppetTest
+    # NOTE: currently both of these will produce bogus results on Darwin due to the wonderful
+    # UID of nobody.
     def nonrootuser
         Etc.passwd { |user|
-            if user.uid != Puppet::SUIDManager.uid and user.uid > 0
+            if user.uid != Puppet::SUIDManager.uid and user.uid > 0 and user.uid < 255
                 return user
             end
         }
@@ -11,7 +13,7 @@ module PuppetTest
 
     def nonrootgroup
         Etc.group { |group|
-            if group.gid != Puppet::SUIDManager.gid and group.gid > 0
+            if group.gid != Puppet::SUIDManager.gid and group.gid > 0 and group.gid < 255
                 return group
             end
         }
diff --git a/test/puppet/suidmanager.rb b/test/puppet/suidmanager.rb
index f5cb8496e..fc56c6072 100644
--- a/test/puppet/suidmanager.rb
+++ b/test/puppet/suidmanager.rb
@@ -1,22 +1,37 @@
-require 'test/unit'
+require 'puppet'
 require 'puppettest'
+require 'test/unit'
+
+class TestSUIDManager < Test::Unit::TestCase
+    include PuppetTest
 
-class TestProcess < Test::Unit::TestCase
     def setup
         if Process.uid != 0
-            $stderr.puts "Process tests must be run as root"
+            warn "Process tests must be run as root"
             @run = false
         else 
             @run = true
         end
+        super
+    end
+
+    def test_metaprogramming_function_additions
+        # NOTE: the way that we are dynamically generating the methods in SUIDManager for
+        # the UID/GID calls was causing problems due to the modification
+        # of a closure. Should the bug rear itself again, this test
+        # will fail.
+        assert_nothing_raised do
+            Puppet::SUIDManager.uid
+            Puppet::SUIDManager.uid
+        end
     end
 
     def test_id_set
         if @run
-            # FIXME: use the test framework uid finder
+            user = nonrootuser
             assert_nothing_raised do
-                Puppet::SUIDManager.egid = 501
-                Puppet::SUIDManager.euid = 501
+                Puppet::SUIDManager.egid = user.gid
+                Puppet::SUIDManager.euid = user.uid
             end
             
             assert_equal(Puppet::SUIDManager.euid, Process.euid)
@@ -27,32 +42,33 @@ class TestProcess < Test::Unit::TestCase
                 Puppet::SUIDManager.egid = 0
             end
 
-            assert_uid_gid(501, 501)
+            assert_uid_gid(user.uid, user.gid, tempfile)
         end
     end
 
     def test_asuser
         if @run
+            user = nonrootuser
             uid, gid = [nil, nil]
 
             assert_nothing_raised do
-                Puppet::SUIDManager.asuser(501, 501) do 
+                Puppet::SUIDManager.asuser(user.uid, user.gid) do 
                     uid = Puppet::SUIDManager.euid
                     gid = Puppet::SUIDManager.egid
                 end
             end
 
-            assert_equal(501, uid)
-            assert_equal(501, gid)
+            assert_equal(user.uid, uid)
+            assert_equal(user.gid, gid)
         end
     end
 
     def test_system
         # NOTE: not sure what shells this will work on..
-        # FIXME: use the test framework uid finder, however the uid needs to be < 255
         if @run 
-            Puppet::SUIDManager.system("exit $EUID", 10, 10)
-            assert_equal($?.exitstatus, 10)
+            user = nonrootuser
+            status = Puppet::SUIDManager.system("exit $EUID", user.uid, user.gid)
+            assert_equal(status.exitstatus, user.uid)
         end
     end