From 200e4ee2051d91bc623396defb3dbdfb7fa5c0f9 Mon Sep 17 00:00:00 2001
From: Joshua Hoblitt <jhoblitt@cpan.org>
Date: Tue, 29 Oct 2013 21:44:14 -0700
Subject: [PATCH] always exec the concatfragments script as root when running
 as root

This is to allow the concatfragments script to be installed into a path that may not be accessible by a non-root user.
---
 manifests/init.pp                |  7 +--
 spec/system/concat_spec.rb       | 73 +++++++++++++++++++++++---------
 spec/unit/defines/concat_spec.rb | 11 +++--
 3 files changed, 64 insertions(+), 27 deletions(-)

diff --git a/manifests/init.pp b/manifests/init.pp
index 774113c..87f2768 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -162,14 +162,15 @@ define concat(
       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",
diff --git a/spec/system/concat_spec.rb b/spec/system/concat_spec.rb
index af360d6..b3b7350 100644
--- a/spec/system/concat_spec.rb
+++ b/spec/system/concat_spec.rb
@@ -1,27 +1,8 @@
 require 'spec_helper_system'
 
 describe 'basic concat test' do
-  context 'should run successfully' do
-    pp="
-      concat { '/tmp/file':
-        owner => root,
-        group => root,
-        mode  => '0644',
-      }
-
-      concat::fragment { '1':
-        target  => '/tmp/file',
-        content => '1',
-        order   => '01',
-      }
-
-      concat::fragment { '2':
-        target  => '/tmp/file',
-        content => '2',
-        order   => '02',
-      }
-    "
 
+  shared_examples 'concat' do |pp|
     context puppet_apply(pp) do
       its(:stderr) { should be_empty }
       its(:exit_code) { should_not == 1 }
@@ -50,6 +31,58 @@ describe 'basic concat test' do
     describe file('/var/lib/puppet/concat/_tmp_file/fragments.concat') do
       it { should be_file }
     end
+  end
 
+  context 'owner/group root' do
+    pp="
+      concat { '/tmp/file':
+        owner => 'root',
+        group => 'root',
+        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 'concat', pp
+  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 'concat', pp
   end
 end
diff --git a/spec/unit/defines/concat_spec.rb b/spec/unit/defines/concat_spec.rb
index 33b9beb..ddbd21a 100644
--- a/spec/unit/defines/concat_spec.rb
+++ b/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 = {
@@ -37,7 +38,7 @@ describe 'concat', :type => :define do
 
     let(:title) { title }
     let(:params) { params }
-    let(:facts) {{ :concat_basedir => concatdir }}
+    let(:facts) {{ :concat_basedir => concatdir, :id => id }}
 
     if p[:ensure] == 'present'
       it do
@@ -95,8 +96,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 +161,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