Make cron provider work when Chef is run as non-root user.kwilczynski/cron-provider

Also, move it onto Chef::Mixin::ShellOut.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>

2 files changed, 206 insertions, 65 deletions

diff --git a/lib/chef/provider/cron.rb b/lib/chef/provider/cron.rb
index 1590c624f6..c017c9d691 100644
--- a/lib/chef/provider/cron.rb
+++ b/lib/chef/provider/cron.rb
@@ -23,7 +23,7 @@ require 'chef/provider'
 class Chef
   class Provider
     class Cron < Chef::Provider
-      include Chef::Mixin::Command
+      include Chef::Mixin::ShellOut
 
       SPECIAL_TIME_VALUES = [:reboot, :yearly, :annually, :monthly, :weekly, :daily, :midnight, :hourly]
       CRON_ATTRIBUTES = [:minute, :hour, :day, :month, :weekday, :time, :command, :mailto, :path, :shell, :home, :environment]
@@ -47,7 +47,7 @@ class Chef
       def load_current_resource
         crontab_lines = []
         @current_resource = Chef::Resource::Cron.new(@new_resource.name)
-        @current_resource.user(@new_resource.user)
+        @current_resource.user(@new_resource.user(determine_user))
         @cron_exists = false
         if crontab = read_crontab
           cron_found = false
@@ -196,6 +196,15 @@ class Chef
 
       private
 
+      def root?
+        return false if Chef::Platform.windows?
+        Process.euid == 0
+      end
+
+      def determine_user
+        root? ? @new_resource.user : Etc.getpwuid(Process.uid).name
+      end
+
       def set_environment_var(attr_name, attr_value)
         if %w(MAILTO PATH SHELL HOME).include?(attr_name)
           @current_resource.send(attr_name.downcase.to_sym, attr_value)
@@ -204,30 +213,49 @@ class Chef
         end
       end
 
-      def read_crontab
-        crontab = nil
-        status = popen4("crontab -l -u #{@new_resource.user}") do |pid, stdin, stdout, stderr|
-          crontab = stdout.read
-        end
-        if status.exitstatus > 1
-          raise Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: #{status.exitstatus}"
+      def shell_out_crontab(*arguments)
+        options = arguments.pop if arguments[-1].is_a?(Hash)
+        # A great majority of the "crontab" (the user space binary) implementations
+        # will only ever accept the "-u" flag when the user is either root or said
+        # user has elevated privileges (effective UID is 0 via "sudo", etc.).
+        crontab_cmd = "crontab "
+        if root?
+          crontab_cmd += "-u #{@new_resource.user} "
+        else
+          Chef::Log.warn("Not running as root! Will only be able to access cron jobs for user: #{@new_resource.user}")
         end
-        crontab
+        crontab_cmd += arguments.join(' ')
+        shell_out(crontab_cmd, options)
       end
 
-      def write_crontab(crontab)
-        write_exception = false
-        status = popen4("crontab -u #{@new_resource.user} -", :waitlast => true) do |pid, stdin, stdout, stderr|
-          begin
-            stdin.write crontab
-          rescue Errno::EPIPE => e
-            # popen4 could yield while child has already died.
-            write_exception = true
-            Chef::Log.debug("#{e.message}")
-          end
+      def read_crontab
+        result = shell_out_crontab('-l')
+        status = result.status.exitstatus
+
+        # A non-zero exit code is an indicator of error for majority
+        # of "crontab" (the user space binary) implementations,
+        # like i.e., vixie-cron, cronie, dcron, fcron, bcron, etc.,
+        # and even mcron which returns a whole range of exit codes.
+        Chef::Log.debug(result.format_for_exception) if status > 0
+
+        # Besides mcron, probably no other implementation
+        # will ever emit exit code greater or equal to two.
+        if status > 1
+          raise Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: #{status}"
         end
-        if status.exitstatus > 0 || write_exception
-          raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{status.exitstatus}"
+
+        return nil if status > 0
+        result.stdout
+      end
+
+      def write_crontab(content)
+        result = shell_out_crontab('-', input: content)
+        status = result.status.exitstatus
+
+        # See note about viable exit codes in the "read_crontab" method.
+        if status > 0
+          Chef::Log.debug(result.format_for_exception)
+          raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{status}"
         end
       end
 
diff --git a/spec/unit/provider/cron_spec.rb b/spec/unit/provider/cron_spec.rb
index 7a917a4f27..836d59c798 100644
--- a/spec/unit/provider/cron_spec.rb
+++ b/spec/unit/provider/cron_spec.rb
@@ -154,8 +154,9 @@ CRONTAB
   end
 
   describe "when examining the current system state" do
-    context "with no crontab for the user" do
+    context "with no crontab for the root user" do
       before :each do
+        allow(@provider).to receive(:root?).and_return(true)
         allow(@provider).to receive(:read_crontab).and_return(nil)
       end
 
@@ -171,6 +172,25 @@ CRONTAB
       end
     end
 
+    context "with no crontab for a non-root user" do
+      before :each do
+        allow(@provider).to receive(:root?).and_return(true)
+        allow(@provider).to receive(:read_crontab).and_return(nil)
+        allow(@new_resource).to receive(:user).and_return('bartholomeow')
+      end
+
+      it "should set cron_empty" do
+        @provider.load_current_resource
+        expect(@provider.cron_empty).to eq(true)
+        expect(@provider.cron_exists).to eq(false)
+      end
+
+      it "should report an empty crontab" do
+        expect(Chef::Log).to receive(:debug).with("Cron empty for '#{@new_resource.user}'")
+        @provider.load_current_resource
+      end
+    end
+
     context "with no matching entry in the user's crontab" do
       before :each do
         allow(@provider).to receive(:read_crontab).and_return(<<-CRONTAB)
@@ -880,8 +900,7 @@ MAILTO=foo@example.com
 
   describe "read_crontab" do
     before :each do
-      @status = double("Status", :exitstatus => 0)
-      @stdout = StringIO.new(<<-CRONTAB)
+      @stdout = <<-CRONTAB
 0 2 * * * /some/other/command
 
 # Chef Name: something else
@@ -889,12 +908,55 @@ MAILTO=foo@example.com
 
 # Another comment
       CRONTAB
-      allow(@provider).to receive(:popen4).and_yield(1234, StringIO.new, @stdout, StringIO.new).and_return(@status)
+      allow(Chef::Log).to receive(:debug)
+      @status = double("Process::Status", :exitstatus => 0)
+      @shell_out = double("Mixlib::ShellOut", :status => @status, :stdout => @stdout)
+      allow(@shell_out).to receive(:format_for_exception).and_return("formatted command output")
+      allow(@provider).to receive(:shell_out).and_return(@shell_out)
     end
 
-    it "should call crontab -l with the user" do
-      expect(@provider).to receive(:popen4).with("crontab -l -u #{@new_resource.user}").and_return(@status)
-      @provider.send(:read_crontab)
+    describe "when the user is root" do
+      before :each do
+        allow(@new_resource).to receive(:user).and_return("root")
+        allow(@provider).to receive(:root?).and_return(true)
+        allow(@provider).to receive(:determine_user).and_return("root")
+      end
+
+      it "should set user name to current user" do
+        @provider.load_current_resource
+        expect(@provider).to have_received(:determine_user)
+        expect(@new_resource).to have_received(:user).with("root")
+      end
+
+      it "should call crontab -l as root" do
+        @provider.send(:read_crontab)
+        expect(@provider).to have_received(:shell_out).with("crontab -u root -l", nil)
+      end
+
+      it "should call crontab -l as root with the user" do
+        allow(@new_resource).to receive(:user).and_return("porkchop")
+        @provider.send(:read_crontab)
+        expect(@provider).to have_received(:shell_out).with("crontab -u #{@new_resource.user} -l", nil)
+      end
+    end
+
+    describe "when the user is a non-root user" do
+      before :each do
+        allow(@new_resource).to receive(:user)
+        allow(@provider).to receive(:root?).and_return(false)
+        allow(@provider).to receive(:determine_user).and_return("porkchop")
+      end
+
+      it "should set user name to current user" do
+        @provider.load_current_resource
+        expect(@provider).to have_received(:determine_user)
+        expect(@new_resource).to have_received(:user).with("porkchop")
+      end
+
+      it "should call crontab -l as current user" do
+        @provider.send(:read_crontab)
+        expect(@provider).to have_received(:shell_out).with("crontab -l", nil)
+      end
     end
 
     it "should return the contents of the crontab" do
@@ -909,61 +971,112 @@ MAILTO=foo@example.com
       CRONTAB
     end
 
-    it "should return nil if the user has no crontab" do
-      status = double("Status", :exitstatus => 1)
-      allow(@provider).to receive(:popen4).and_return(status)
-      expect(@provider.send(:read_crontab)).to eq(nil)
-    end
+    describe "when the user has no crontab" do
+      before :each do
+        allow(@status).to receive(:exitstatus).and_return(1)
+      end
+
+      it "should return nil if the user has no crontab" do
+        expect(@provider.send(:read_crontab)).to eq(nil)
+      end
 
-    it "should raise an exception if another error occurs" do
-      status = double("Status", :exitstatus => 2)
-      allow(@provider).to receive(:popen4).and_return(status)
-      expect do
+      it "logs the crontab output to debug" do
         @provider.send(:read_crontab)
-      end.to raise_error(Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: 2")
+        expect(Chef::Log).to have_received(:debug).with("formatted command output")
+      end
+    end
+
+    describe "when any other error occurs" do
+      before :each do
+        allow(@status).to receive(:exitstatus).and_return(2)
+      end
+
+      it "should raise an exception if another error occurs" do
+        expect do
+          @provider.send(:read_crontab)
+        end.to raise_error(Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: 2")
+      end
+
+      it "logs the crontab output to debug" do
+        @provider.send(:read_crontab) rescue nil
+        expect(Chef::Log).to have_received(:debug).with("formatted command output")
+      end
     end
   end
 
   describe "write_crontab" do
     before :each do
-      @status = double("Status", :exitstatus => 0)
-      @stdin = StringIO.new
-      allow(@provider).to receive(:popen4).and_yield(1234, @stdin, StringIO.new, StringIO.new).and_return(@status)
+      allow(Chef::Log).to receive(:warn)
+      allow(Chef::Log).to receive(:debug)
+      @status = double("Process::Status", :exitstatus => 0)
+      @shell_out = double("Mixlib::ShellOut", :status => @status, :stdout => "")
+      allow(@shell_out).to receive(:format_for_exception).and_return("formatted command output")
+      allow(@provider).to receive(:shell_out).and_return(@shell_out)
     end
 
-    it "should call crontab for the user" do
-      expect(@provider).to receive(:popen4).with("crontab -u #{@new_resource.user} -", :waitlast => true).and_return(@status)
-      @provider.send(:write_crontab, "Foo")
-    end
+    describe "when the user is root" do
+      before :each do
+        allow(@new_resource).to receive(:user).and_return("root")
+        allow(@provider).to receive(:root?).and_return(true)
+        allow(@provider).to receive(:determine_user).and_return("root")
+      end
 
-    it "should write the given string to the crontab command" do
-      @provider.send(:write_crontab, "Foo\n# wibble\n wah!!")
-      expect(@stdin.string).to eq("Foo\n# wibble\n wah!!")
-    end
+      it "should set user name to current user" do
+        @provider.load_current_resource
+        expect(@provider).to have_received(:determine_user)
+        expect(@new_resource).to have_received(:user).with("root")
+        expect(Chef::Log).not_to have_received(:warn)
+      end
 
-    it "should raise an exception if the command returns non-zero" do
-      allow(@status).to receive(:exitstatus).and_return(1)
-      expect do
+      it "should call crontab as root" do
         @provider.send(:write_crontab, "Foo")
-      end.to raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
+        expect(@provider).to have_received(:shell_out).with("crontab -u root -", { :input => "Foo" })
+      end
+
+      it "should call crontab for as root with the user" do
+        allow(@new_resource).to receive(:user).and_return("puffypaws")
+        @provider.send(:write_crontab, "Foo")
+        expect(@provider).to have_received(:shell_out).with("crontab -u #{@new_resource.user} -", { :input => "Foo" })
+      end
     end
 
-    it "should raise an exception if the command die's and parent tries to write" do
-      class WriteErrPipe
-        def write(str)
-          raise Errno::EPIPE, "Test"
-        end
+    describe "when the user is a non-root user" do
+      before :each do
+        allow(@new_resource).to receive(:user)
+        allow(@provider).to receive(:root?).and_return(false)
+        allow(@provider).to receive(:determine_user).and_return("whiskers")
       end
-      allow(@status).to receive(:exitstatus).and_return(1)
-      allow(@provider).to receive(:popen4).and_yield(1234, WriteErrPipe.new, StringIO.new, StringIO.new).and_return(@status)
 
-      expect(Chef::Log).to receive(:debug).with("Broken pipe - Test")
+      it "should set user name to current user" do
+        @provider.load_current_resource
+        expect(@provider).to have_received(:determine_user)
+        expect(@new_resource).to have_received(:user).with("whiskers")
+        expect(Chef::Log).to have_received(:warn).with("Not running as root! Will only be able to access cron jobs for user: #{@new_resource.user}")
+      end
 
-      expect do
-        @provider.send(:write_crontab, "Foo")
-      end.to raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
+      it "should call crontab as current user" do
+        @provider.send(:write_crontab, "Foo\n# wibble\n wah!!")
+        expect(@provider).to have_received(:shell_out).with("crontab -", { :input => "Foo\n# wibble\n wah!!" })
+      end
     end
 
+    describe "when writing the crontab fails" do
+      before :each do
+        allow(@status).to receive(:exitstatus).and_return(1)
+      end
+
+      it "should raise an exception if the command returns non-zero" do
+        allow(@status).to receive(:exitstatus).and_return(1)
+        expect do
+          @provider.send(:write_crontab, "Foo")
+        end.to raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
+      end
+
+      it "logs the crontab output to debug" do
+        @provider.send(:read_crontab) rescue nil
+        expect(Chef::Log).to have_received(:debug).with("formatted command output")
+      end
+    end
   end
 
   describe "weekday_in_crontab" do