Skip to content

Commit

Permalink
(#16137) Tilde expansion should only occur for normal users
Browse files Browse the repository at this point in the history 
The previous fix for #15185 caused File.expand_path to be called on
the user default config and var directories even when running as root.
This caused puppet starting as a service to fail because the HOME
environment variable is not set when most services are started.

This change ensures that the File.expand_path will only happen on a path
with a '~' when we are running as a non-root user, in which case the
HOME env var should always be there. As a part of untangling this code,
this also teases apart the Windows path handling from the Unix path
handling and allows us to use the more "Windowsy" path for the user's
local directories.
  • Loading branch information
@zaphod42
zaphod42 committed Aug 29, 2012
1 parent 43590bd commit b53e600
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 97 deletions.
27 changes: 2 additions & 25 deletions lib/puppet/settings.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,29 +55,6 @@ def self.hostname_fact()

def self.domain_fact()
Facter["domain"].value
end


def self.default_global_config_dir
Puppet.features.microsoft_windows? ? File.join(Dir::COMMON_APPDATA, "PuppetLabs", "puppet", "etc") : "/etc/puppet"
end

def self.default_user_config_dir
# ensure HOME is set, if not set, get user's home directory
require 'etc'
ENV["HOME"] ||= Etc.getpwuid(Process.uid).dir
File.expand_path("~/.puppet")
end

def self.default_global_var_dir
Puppet.features.microsoft_windows? ? File.join(Dir::COMMON_APPDATA, "PuppetLabs", "puppet", "var") : "/var/lib/puppet"
end

def self.default_user_var_dir
# ensure HOME is set, if not set, get user's home directory
require 'etc'
ENV["HOME"] ||= Etc.getpwuid(Process.uid).dir
File.expand_path("~/.puppet/var")
end

def self.default_config_file_name
Expand Down Expand Up @@ -525,13 +502,13 @@ def main_config_file
if explicit_config_file?
return self[:config]
else
return File.join(self.class.default_global_config_dir, config_file_name)
return File.join(Puppet::Util::RunMode[:master].conf_dir, config_file_name)
end
end
private :main_config_file

def user_config_file
return File.join(self.class.default_user_config_dir, config_file_name)
return File.join(Puppet::Util::RunMode[:user].conf_dir, config_file_name)
end
private :user_config_file

Expand Down
66 changes: 39 additions & 27 deletions lib/puppet/util/run_mode.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@ def initialize(name)
@name = name.to_sym
end

@@run_modes = Hash.new {|h, k| h[k] = RunMode.new(k)}

attr :name

def self.[](name)
@@run_modes[name]
@run_modes ||= {}
if Puppet.features.microsoft_windows?
@run_modes[name] ||= WindowsRunMode.new(name)
else
@run_modes[name] ||= UnixRunMode.new(name)
end
end

def master?
Expand All @@ -27,21 +30,6 @@ def user?
name == :user
end


def conf_dir
which_dir(
Puppet::Settings.default_global_config_dir,
Puppet::Settings.default_user_config_dir
)
end

def var_dir
which_dir(
Puppet::Settings.default_global_var_dir,
Puppet::Settings.default_user_var_dir
)
end

def run_dir
"$vardir/run"
end
Expand All @@ -50,24 +38,48 @@ def log_dir
"$vardir/log"
end

private
private

def which_dir( global, user )
#FIXME: we should test if we're user "puppet"
# there's a comment that suggests that we do that
# and we currently don't.
expand_path case
when name == :master; global
when Puppet.features.root?; global
else user
end
File.expand_path(if in_global_context? then global else user end)
end

def in_global_context?
name == :master || Puppet.features.root?
end
end

class UnixRunMode < RunMode
def conf_dir
which_dir("/etc/puppet", "~/.puppet")
end

def var_dir
which_dir("/var/lib/puppet", "~/.puppet/var")
end
end

class WindowsRunMode < RunMode
def conf_dir
which_dir(File.join(windows_common_base("etc")), File.join(*windows_local_base))
end

def expand_path( dir )
ENV["HOME"] ||= Etc.getpwuid(Process.uid).dir
File.expand_path(dir)
def var_dir
which_dir(File.join(windows_common_base("var")), File.join(*windows_local_base("var")))
end

private

def windows_common_base(*extra)
[Dir::COMMON_APPDATA, "PuppetLabs", "puppet"] + extra
end

def windows_local_base(*extra)
[Dir::LOCAL_APPDATA, "PuppetLabs", "puppet"] + extra
end
end
end
end
17 changes: 2 additions & 15 deletions spec/unit/settings_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,8 @@
describe Puppet::Settings do
include PuppetSpec::Files

MAIN_CONFIG_FILE_DEFAULT_LOCATION = File.join(Puppet::Settings.default_global_config_dir, "puppet.conf")
USER_CONFIG_FILE_DEFAULT_LOCATION = File.join(Puppet::Settings.default_user_config_dir, "puppet.conf")

describe "when dealing with user default directories" do
context "user config dir" do
it "should expand the value to an absolute path" do
Pathname.new(Puppet::Settings.default_user_config_dir).absolute?.should be_true
end
end
context "user var dir" do
it "should expand the value to an absolute path" do
Pathname.new(Puppet::Settings.default_user_var_dir).absolute?.should be_true
end
end
end
MAIN_CONFIG_FILE_DEFAULT_LOCATION = File.join(Puppet::Util::RunMode[:master].conf_dir, "puppet.conf")
USER_CONFIG_FILE_DEFAULT_LOCATION = File.join(Puppet::Util::RunMode[:user].conf_dir, "puppet.conf")

describe "when specifying defaults" do
before do
Expand Down
121 changes: 91 additions & 30 deletions spec/unit/util/run_mode_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,44 +6,105 @@
@run_mode = Puppet::Util::RunMode.new('fake')
end

it "should have confdir /etc/puppet when run as root" do
Puppet.features.stubs(:root?).returns(true)
etcdir = Puppet.features.microsoft_windows? ? File.join(Dir::COMMON_APPDATA, "PuppetLabs", "puppet", "etc") : '/etc/puppet'
# REMIND: issue with windows backslashes
@run_mode.conf_dir.should == File.expand_path(etcdir)
it "has rundir depend on vardir" do
@run_mode.run_dir.should == '$vardir/run'
end

it "should have confdir ~/.puppet when run as non-root" do
Puppet.features.stubs(:root?).returns(false)
@run_mode.conf_dir.should == File.expand_path("~/.puppet")
end
describe Puppet::Util::UnixRunMode do
before do
@run_mode = Puppet::Util::UnixRunMode.new('fake')
end

it "should have vardir /var/lib/puppet when run as root" do
Puppet.features.stubs(:root?).returns(true)
vardir = Puppet.features.microsoft_windows? ? File.join(Dir::COMMON_APPDATA, "PuppetLabs", "puppet", "var") : '/var/lib/puppet'
# REMIND: issue with windows backslashes
@run_mode.var_dir.should == File.expand_path(vardir)
end
describe "#conf_dir" do
it "has confdir /etc/puppet when run as root" do
as_root { @run_mode.conf_dir.should == '/etc/puppet' }
end

it "should have vardir ~/.puppet/var when run as non-root" do
Puppet.features.stubs(:root?).returns(false)
@run_mode.var_dir.should == File.expand_path("~/.puppet/var")
it "has confdir ~/.puppet when run as non-root" do
as_non_root { @run_mode.conf_dir.should == File.expand_path('~/.puppet') }
end

it "fails when asking for the conf_dir as non-root and there is no $HOME" do
as_non_root do
without_home do
expect { @run_mode.conf_dir }.to raise_error ArgumentError, /couldn't find HOME/
end
end
end
end

describe "#var_dir" do
it "has vardir /var/lib/puppet when run as root" do
as_root { @run_mode.var_dir.should == '/var/lib/puppet' }
end

it "has vardir ~/.puppet/var when run as non-root" do
as_non_root { @run_mode.var_dir.should == File.expand_path('~/.puppet/var') }
end

it "fails when asking for the var_dir as non-root and there is no $HOME" do
as_non_root do
without_home do
expect { @run_mode.var_dir }.to raise_error ArgumentError, /couldn't find HOME/
end
end
end
end

def without_home
saved_home = ENV["HOME"]
ENV.delete "HOME"
yield
ensure
ENV["HOME"] = saved_home
end
end

it "should have rundir depend on vardir" do
@run_mode.run_dir.should == '$vardir/run'
describe Puppet::Util::WindowsRunMode do
before do
if not Dir.const_defined? :COMMON_APPDATA
Dir.const_set :COMMON_APPDATA, "/CommonFakeBase"
Dir.const_set :LOCAL_APPDATA, "/LocalFakeBase"
@remove_const = true
end
@run_mode = Puppet::Util::WindowsRunMode.new('fake')
end

after do
if @remove_const
Dir.send :remove_const, :COMMON_APPDATA
Dir.send :remove_const, :LOCAL_APPDATA
end
end

describe "#conf_dir" do
it "has confdir /etc/puppet when run as root" do
as_root { @run_mode.conf_dir.should == File.join(Dir::COMMON_APPDATA, "PuppetLabs", "puppet", "etc") }
end

it "has confdir in the local appdata when run as non-root" do
as_non_root { @run_mode.conf_dir.should == File.join(Dir::LOCAL_APPDATA, "PuppetLabs", "puppet") }
end
end

describe "#var_dir" do
it "has vardir /var/lib/puppet when run as root" do
as_root { @run_mode.var_dir.should == File.join(Dir::COMMON_APPDATA, "PuppetLabs", "puppet", "var") }
end

it "has vardir local appdata when run as non-root" do
as_non_root { @run_mode.var_dir.should == File.join(Dir::LOCAL_APPDATA, "PuppetLabs", "puppet", "var") }
end
end
end

def as_root
Puppet.features.stubs(:root?).returns(true)
yield
end

it "should have logopts return a hash with $vardir/log and other metadata if runmode is master" do
pending("runmode.logopts functionality is being moved")
@run_mode.expects(:master?).returns true
@run_mode.logopts.should == {
:default => "$vardir/log",
:mode => 0750,
:owner => "service",
:group => "service",
:desc => "The Puppet log directory.",
}
def as_non_root
Puppet.features.stubs(:root?).returns(false)
yield
end
end

0 comments on commit b53e600

Please sign in to comment.