Fixing the asuser method in Puppet::Util::SUIDManager

so that it correctly just yields if you're not root.  It
also no longer tries to set :uid or :gid; just :euid and :egid,
and it once again sets :egid before it sets :euid, which is
important because you usually can't change your group after
you've changed your user id.

2 files changed, 40 insertions, 24 deletions

diff --git a/lib/puppet/util/suidmanager.rb b/lib/puppet/util/suidmanager.rb
index 3108fdf5f..b071dca6f 100644
--- a/lib/puppet/util/suidmanager.rb
+++ b/lib/puppet/util/suidmanager.rb
@@ -21,21 +21,19 @@ module Puppet::Util::SUIDManager
     end
 
     # Runs block setting uid and gid if provided then restoring original ids
-    def asuser new_uid=nil, new_gid=nil
-      # We set both because some programs like to drop privs, i.e. bash.
-      old_uid, old_gid = self.uid, self.gid
-      old_euid, old_egid = self.euid, self.egid
-      begin
-          self.uid = convert_xid :uid, new_uid if new_uid
-          self.gid = convert_xid :gid, new_gid if new_gid
-          self.euid = convert_xid :uid, new_uid if new_uid
-          self.egid = convert_xid :gid, new_gid if new_gid
-
-          yield
-      ensure
-          self.uid, self.gid = old_uid, old_gid
-          self.euid, self.egid = old_euid, old_egid
-      end
+    def asuser(new_uid=nil, new_gid=nil)
+        return yield unless Process.uid == 0
+        # We set both because some programs like to drop privs, i.e. bash.
+        old_uid, old_gid = self.uid, self.gid
+        old_euid, old_egid = self.euid, self.egid
+        begin
+            self.egid = convert_xid :gid, new_gid if new_gid
+            self.euid = convert_xid :uid, new_uid if new_uid
+  
+            yield
+        ensure
+            self.euid, self.egid = old_euid, old_egid
+        end
     end
     module_function :asuser
     
diff --git a/test/puppet/tc_suidmanager.rb b/test/puppet/tc_suidmanager.rb
index 2e44ead6f..08d957cc0 100755
--- a/test/puppet/tc_suidmanager.rb
+++ b/test/puppet/tc_suidmanager.rb
@@ -42,19 +42,35 @@ class TestSUIDManager < Test::Unit::TestCase
         assert_not_equal(nil, Puppet::Util.uid(@user.name))
     end
 
-    def test_asuser
+    def test_asuser_as_root
+        Process.stubs(:uid).returns(0)
         expects_id_set_and_revert @user.uid, @user.gid
         Puppet::Util::SUIDManager.asuser @user.uid, @user.gid do end
     end
 
+    def test_asuser_as_nonroot
+        Process.stubs(:uid).returns(1)
+        expects_no_id_set
+        Puppet::Util::SUIDManager.asuser @user.uid, @user.gid do end
+    end
+
 
-    def test_system
+    def test_system_as_root
+        Process.stubs(:uid).returns(0)
         set_exit_status!
         expects_id_set_and_revert @user.uid, @user.gid
         Kernel.expects(:system).with('blah')
         Puppet::Util::SUIDManager.system('blah', @user.uid, @user.gid)
     end
 
+    def test_system_as_nonroot
+        Process.stubs(:uid).returns(1)
+        set_exit_status!
+        expects_no_id_set
+        Kernel.expects(:system).with('blah')
+        Puppet::Util::SUIDManager.system('blah', @user.uid, @user.gid)
+    end
+
     def test_run_and_capture
         if (RUBY_VERSION <=> "1.8.4") < 0
             warn "Cannot run this test on ruby < 1.8.4"
@@ -78,23 +94,25 @@ class TestSUIDManager < Test::Unit::TestCase
     end
 
     private
-    def expects_id_set_and_revert uid, gid
-        Process.expects(:uid).returns(99999)
-        Process.expects(:gid).returns(99998)
+
+    def expects_id_set_and_revert(uid, gid)
         Process.expects(:euid).returns(99997)
         Process.expects(:egid).returns(99996)
 
-        Process.expects(:uid=).with(uid)
-        Process.expects(:gid=).with(gid)
         Process.expects(:euid=).with(uid)
         Process.expects(:egid=).with(gid)
 
-        Process.expects(:uid=).with(99999)
-        Process.expects(:gid=).with(99998)
         Process.expects(:euid=).with(99997)
         Process.expects(:egid=).with(99996)
     end
 
+    def expects_no_id_set
+        Process.expects(:egid).never
+        Process.expects(:euid).never
+        Process.expects(:egid=).never
+        Process.expects(:euid=).never
+    end
+
     def set_exit_status!
       # We want to make sure $? is set, this is the only way I know how.
       Kernel.system '' if $?.nil?