From f034bf70bc4347db83bbee073ebd6b373db97827 Mon Sep 17 00:00:00 2001 From: Krzysztof Wilczynski Date: Wed, 26 Nov 2014 01:29:35 +0000 Subject: Make cron provider work when Chef is run as non-root user. Also, move it onto Chef::Mixin::ShellOut. Signed-off-by: Krzysztof Wilczynski --- lib/chef/provider/cron.rb | 72 ++++++++++----- spec/unit/provider/cron_spec.rb | 199 +++++++++++++++++++++++++++++++--------- 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 -- cgit v1.2.1