Merge pull request #91 from jhoblitt/exec_as_root_when_root

always exec the concatfragments script as root when running as root

manifests/fragment.pp

@@ -30,7 +30,7 @@ define concat::fragment(
     $source  = undef,
     $order   = 10,
     $ensure  = 'present',
-    $mode    = '0644',
+    $mode    = '0640',
     $owner   = undef,
     $group   = undef,
     $backup  = undef

manifests/init.pp

@@ -125,20 +125,18 @@ define concat(
   }
 
   File {
-    owner   => $owner,
-    group   => $group,
-    mode    => $mode,
-    replace => $replace,
     backup  => false,
   }
 
   if $ensure == 'present' {
     file { $fragdir:
       ensure => directory,
+      mode   => '0750',
     }
 
     file { "${fragdir}/fragments":
       ensure  => directory,
+      mode    => '0750',
       force   => true,
       ignore  => ['.svn', '.git', '.gitignore'],
       notify  => Exec["concat_${name}"],
@@ -148,28 +146,35 @@ define concat(
 
     file { "${fragdir}/fragments.concat":
       ensure => present,
+      mode   => '0640',
     }
 
     file { "${fragdir}/${concat_name}":
       ensure => present,
+      mode   => '0640',
     }
 
     file { $name:
-      ensure => present,
-      path   => $path,
-      alias  => "concat_${name}",
-      source => "${fragdir}/${concat_name}",
-      backup => $backup,
+      ensure  => present,
+      owner   => $owner,
+      group   => $group,
+      mode    => $mode,
+      replace => $replace,
+      path    => $path,
+      alias   => "concat_${name}",
+      source  => "${fragdir}/${concat_name}",
+      backup  => $backup,
     }
 
-    # remove extra whitespace from string interopolation to make testing easier
+    # remove extra whitespace from string interpolation to make testing easier
     $command = strip(regsubst("${script_command} -o ${fragdir}/${concat_name} -d ${fragdir} ${warnflag} ${forceflag} ${orderflag} ${newlineflag}", '\s+', ' ', 'G'))
 
+    # if puppet is running as root, this exec should also run as root to allow
+    # the concatfragments.sh script to potentially be installed in path that
+    # may not be accessible by a target non-root owner.
     exec { "concat_${name}":
       alias     => "concat_${fragdir}",
       command   => $command,
-      user      => $owner,
-      group     => $group,
       notify    => File[$name],
       subscribe => File[$fragdir],
       unless    => "${command} -t",

manifests/setup.pp

@@ -37,6 +37,8 @@ class concat::setup {
   }
 
   file { $script_path:
+    ensure => file,
+    owner  => $::id,
     mode   => '0755',
     source => "puppet:///modules/concat/${script_name}",
   }

spec/system/concat_spec.rb

@@ -1,11 +1,65 @@
 require 'spec_helper_system'
 
 describe 'basic concat test' do
-  context 'should run successfully' do
+
+  shared_examples 'successfully_applied' do |pp|
+    context puppet_apply(pp) do
+      its(:stderr) { should be_empty }
+      its(:exit_code) { should_not == 1 }
+      its(:refresh) { should be_nil }
+      its(:stderr) { should be_empty }
+      its(:exit_code) { should be_zero }
+    end
+
+    describe file('/var/lib/puppet/concat') do
+      it { should be_directory }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 755 }
+    end
+    describe file('/var/lib/puppet/concat/bin') do
+      it { should be_directory }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 755 }
+    end
+    describe file('/var/lib/puppet/concat/bin/concatfragments.sh') do
+      it { should be_file }
+      it { should be_owned_by 'root' }
+      #it { should be_grouped_into 'root' }
+      it { should be_mode 755 }
+    end
+    describe file('/var/lib/puppet/concat/_tmp_file') do
+      it { should be_directory }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 750 }
+    end
+    describe file('/var/lib/puppet/concat/_tmp_file/fragments') do
+      it { should be_directory }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 750 }
+    end
+    describe file('/var/lib/puppet/concat/_tmp_file/fragments.concat') do
+      it { should be_file }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 640 }
+    end
+    describe file('/var/lib/puppet/concat/_tmp_file/fragments.concat.out') do
+      it { should be_file }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 640 }
+    end
+  end
+
+  context 'owner/group root' do
     pp="
       concat { '/tmp/file':
-        owner => root,
-        group => root,
+        owner => 'root',
+        group => 'root',
         mode  => '0644',
       }
 
@@ -22,34 +76,79 @@ describe 'basic concat test' do
       }
     "
 
-    context puppet_apply(pp) do
-      its(:stderr) { should be_empty }
-      its(:exit_code) { should_not == 1 }
-      its(:refresh) { should be_nil }
-      its(:stderr) { should be_empty }
-      its(:exit_code) { should be_zero }
-    end
+    it_behaves_like 'successfully_applied', pp
 
     describe file('/tmp/file') do
       it { should be_file }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 644 }
       it { should contain '1' }
       it { should contain '2' }
     end
-
-    # Test that all the relevant bits exist on disk after it
-    # concats.
-    describe file('/var/lib/puppet/concat') do
-      it { should be_directory }
-    end
-    describe file('/var/lib/puppet/concat/_tmp_file') do
-      it { should be_directory }
-    end
-    describe file('/var/lib/puppet/concat/_tmp_file/fragments') do
-      it { should be_directory }
-    end
-    describe file('/var/lib/puppet/concat/_tmp_file/fragments.concat') do
+    describe file('/var/lib/puppet/concat/_tmp_file/fragments/01_1') do
       it { should be_file }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 640 }
+    end
+    describe file('/var/lib/puppet/concat/_tmp_file/fragments/02_2') do
+      it { should be_file }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 640 }
+    end
+  end
+
+  context 'owner/group non-root' do
+    before(:all) do
+      shell "groupadd -g 42 bob"
+      shell "useradd -u 42 -g 42 bob"
     end
 
+    pp="
+      concat { '/tmp/file':
+        owner => 'bob',
+        group => 'bob',
+        mode  => '0644',
+      }
+
+      concat::fragment { '1':
+        target  => '/tmp/file',
+        content => '1',
+        order   => '01',
+      }
+
+      concat::fragment { '2':
+        target  => '/tmp/file',
+        content => '2',
+        order   => '02',
+      }
+    "
+
+    it_behaves_like 'successfully_applied', pp
+
+    describe file('/tmp/file') do
+      it { should be_file }
+      it { should be_owned_by 'bob' }
+      it { should be_grouped_into 'bob' }
+      it { should be_mode 644 }
+      it { should contain '1' }
+      it { should contain '2' }
+    end
+    describe file('/var/lib/puppet/concat/_tmp_file/fragments/01_1') do
+      it { should be_file }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 640 }
+      it { should contain '1' }
+    end
+    describe file('/var/lib/puppet/concat/_tmp_file/fragments/02_2') do
+      it { should be_file }
+      it { should be_owned_by 'root' }
+      it { should be_grouped_into 'root' }
+      it { should be_mode 640 }
+      it { should contain '2' }
+    end
   end
 end

spec/unit/defines/concat_fragment_spec.rb

@@ -10,7 +10,7 @@ describe 'concat::fragment', :type => :define do
       :source  => nil,
       :order   => 10,
       :ensure  => 'present',
-      :mode    => '0644',
+      :mode    => '0640',
       :owner   => nil,
       :group   => nil,
       :backup  => 'puppet',

spec/unit/defines/concat_spec.rb

@@ -2,8 +2,9 @@ require 'spec_helper'
 
 describe 'concat', :type => :define do
 
-  shared_examples 'concat' do |title, params| 
+  shared_examples 'concat' do |title, params, id| 
     params = {} if params.nil?
+    id = 'root' if id.nil?
 
     # default param values
     p = {
@@ -28,27 +29,25 @@ describe 'concat', :type => :define do
     default_warn_message = '# This file is managed by Puppet. DO NOT EDIT.'
 
     file_defaults = {
-      :owner   => p[:owner],
-      :group   => p[:group],
-      :mode    => p[:mode],
       :backup  => false,
-      :replace => p[:replace],
     }
 
     let(:title) { title }
     let(:params) { params }
-    let(:facts) {{ :concat_basedir => concatdir }}
+    let(:facts) {{ :concat_basedir => concatdir, :id => id }}
 
     if p[:ensure] == 'present'
       it do
         should contain_file(fragdir).with(file_defaults.merge({
           :ensure => 'directory',
+          :mode   => '0750',
         }))
       end
 
       it do
         should contain_file("#{fragdir}/fragments").with(file_defaults.merge({
           :ensure  => 'directory',
+          :mode    => '0750',
           :force   => true,
           :ignore  => ['.svn', '.git', '.gitignore'],
           :purge   => true,
@@ -63,17 +62,22 @@ describe 'concat', :type => :define do
         it do
           should contain_file(file).with(file_defaults.merge({
             :ensure => 'present',
+            :mode   => '0640',
           }))
         end
       end
 
       it do
         should contain_file(title).with(file_defaults.merge({
-          :ensure => 'present',
-          :path   => p[:path],
-          :alias  => "concat_#{title}",
-          :source => "#{fragdir}/#{concat_name}",
-          :backup => p[:backup],
+          :ensure  => 'present',
+          :owner   => p[:owner],
+          :group   => p[:group],
+          :mode    => p[:mode],
+          :replace => p[:replace],
+          :path    => p[:path],
+          :alias   => "concat_#{title}",
+          :source  => "#{fragdir}/#{concat_name}",
+          :backup  => p[:backup],
         }))
       end
 
@@ -95,8 +99,6 @@ describe 'concat', :type => :define do
         should contain_exec("concat_#{title}").with({
           :alias   => "concat_#{fragdir}",
           :command => cmd,
-          :user    => p[:owner],
-          :group   => p[:group], 
           :unless  => "#{cmd} -t",
         })
       end
@@ -162,6 +164,10 @@ describe 'concat', :type => :define do
     end
   end # title =>
 
+  context 'as non-root user' do
+    it_behaves_like 'concat', '/etc/foo.bar', {}, 'bob'
+  end
+
   context 'ensure =>' do
     ['present', 'absent'].each do |ens|
       context ens do