From aa0180d69ab45fca3310cc5344d0bf153cfd9d83 Mon Sep 17 00:00:00 2001
From: Joshua Hoblitt <jhoblitt@cpan.org>
Date: Thu, 24 Oct 2013 08:48:37 -0700
Subject: [PATCH] remove default owner/user and group values

The use of $::id to set the default user/owner and group has caused
multiple bugs in the past, is incorrectly used to infer the egid,
introduces a dependency on the `id` fact, and provides no functionally
that can't be accomplished by passing `undef` or not setting the
respective params on the file & exec types.

A possible alternative would be to introduce a dep on the $::gid fact
but that would mean the entire module would depend on a version of
facter than hasn't shipped yet (unworkable) or to add a gid/egid fact
into this module (ugly).
---
 manifests/fragment.pp                     |  9 ++----
 manifests/init.pp                         | 21 ++++----------
 manifests/setup.pp                        | 12 +-------
 spec/unit/classes/concat_setup_spec.rb    | 26 ++++-------------
 spec/unit/defines/concat_fragment_spec.rb | 34 ++++++++---------------
 spec/unit/defines/concat_spec.rb          | 22 ++++-----------
 6 files changed, 29 insertions(+), 95 deletions(-)

diff --git a/manifests/fragment.pp b/manifests/fragment.pp
index 1f90b38..a3d0101 100644
--- a/manifests/fragment.pp
+++ b/manifests/fragment.pp
@@ -32,7 +32,7 @@ define concat::fragment(
     $order   = 10,
     $ensure  = 'present',
     $mode    = '0644',
-    $owner   = $::id,
+    $owner   = undef,
     $group   = undef
 ) {
   validate_absolute_path($target)
@@ -51,11 +51,6 @@ define concat::fragment(
   $concatdir        = $concat::setup::concatdir
   $fragdir          = "${concatdir}/${safe_target_name}"
 
-  $safe_group = $group ? {
-    undef   => $concat::setup::root_group,
-    default => $group,
-  }
-
   # if content is passed, use that, else if source is passed use that
   # if neither passed, but $ensure is in symlink form, make a symlink
   case $ensure {
@@ -73,7 +68,7 @@ define concat::fragment(
     ensure  => $ensure,
     mode    => $mode,
     owner   => $owner,
-    group   => $safe_group,
+    group   => $group,
     source  => $source,
     content => $content,
     backup  => false,
diff --git a/manifests/init.pp b/manifests/init.pp
index f0356f7..6396605 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -55,7 +55,7 @@
 define concat(
   $ensure         = 'present',
   $path           = $name,
-  $owner          = $::id,
+  $owner          = undef,
   $group          = undef,
   $mode           = '0644',
   $warn           = false,
@@ -87,11 +87,6 @@ define concat(
   $concat_name          = 'fragments.concat.out'
   $default_warn_message = '# This file is managed by Puppet. DO NOT EDIT.'
 
-  $safe_group = $group ? {
-    undef   => $concat::setup::root_group,
-    default => $group,
-  }
-
   if $warn == true {
     $use_warn_message = $warn_message ? {
       undef   => $default_warn_message,
@@ -124,7 +119,7 @@ define concat(
 
   File {
     owner   => $owner,
-    group   => $safe_group,
+    group   => $group,
     mode    => $mode,
     replace => $replace,
     backup  => false,
@@ -166,6 +161,8 @@ define concat(
     exec { "concat_${name}":
       alias     => "concat_${fragdir}",
       command   => $command,
+      user      => $owner,
+      group     => $group,
       notify    => File[$name],
       subscribe => File[$fragdir],
       unless    => "${command} -t",
@@ -175,15 +172,7 @@ define concat(
         File["${fragdir}/fragments.concat"],
       ],
     }
-
-    if $::id == 'root' {
-      Exec["concat_${name}"] {
-        user  => root,
-        group => $safe_group,
-      }
-    }
-  }
-  else {
+  } else {
     file { [
       $fragdir,
       "${fragdir}/fragments",
diff --git a/manifests/setup.pp b/manifests/setup.pp
index 0359f2b..2e744e1 100644
--- a/manifests/setup.pp
+++ b/manifests/setup.pp
@@ -14,12 +14,6 @@ class concat::setup {
     fail("Use of private class ${name} by ${caller_module_name}")
   }
 
-  $owner = $::id
-  $root_group = $owner ? {
-    root    => 0,
-    default => $owner
-  }
-
   if $::concat_basedir {
     $concatdir = $::concat_basedir
   } else {
@@ -31,17 +25,13 @@ class concat::setup {
   }
 
   file { "${concatdir}/bin/concatfragments.sh":
-    owner  => $owner,
-    group  => $root_group,
     mode   => '0755',
     source => 'puppet:///modules/concat/concatfragments.sh',
   }
 
   file { [ $concatdir, "${concatdir}/bin" ]:
     ensure => directory,
-    owner  => $owner,
-    group  => $root_group,
-    mode   => '0750',
+    mode   => '0755',
   }
 
   ## Old versions of this module used a different path.
diff --git a/spec/unit/classes/concat_setup_spec.rb b/spec/unit/classes/concat_setup_spec.rb
index a000c06..47c4069 100644
--- a/spec/unit/classes/concat_setup_spec.rb
+++ b/spec/unit/classes/concat_setup_spec.rb
@@ -2,16 +2,13 @@ require 'spec_helper'
 
 describe 'concat::setup', :type => :class do
 
-  shared_examples 'setup' do |id, concatdir|
-    id        = 'root' if id.nil?
+  shared_examples 'setup' do |concatdir|
     concatdir = '/foo' if concatdir.nil?
 
-    let(:facts) {{ :id => id, :concat_basedir => concatdir }}
+    let(:facts) {{ :concat_basedir => concatdir }}
 
     it do
       should contain_file("#{concatdir}/bin/concatfragments.sh").with({
-        :owner  => id,
-        :group  => id == 'root' ? 0 : id,
         :mode   => '0755',
         :source => 'puppet:///modules/concat/concatfragments.sh',
         :backup => false,
@@ -22,9 +19,7 @@ describe 'concat::setup', :type => :class do
       it do
         should contain_file(file).with({
           :ensure => 'directory',
-          :owner  => id,
-          :group  => id == 'root' ? 0 : id,
-          :mode   => '0750',
+          :mode   => '0755',
           :backup => false,
         })
       end
@@ -45,23 +40,12 @@ describe 'concat::setup', :type => :class do
       "$caller_module_name = 'concat'"
     end
 
-    context 'id =>' do
-      context 'root' do
-        it_behaves_like 'setup', 'root'
-      end
-
-      context 'apenny' do
-        it_behaves_like 'setup', 'apenny'
-      end
-    end
-
     context 'concat_basedir =>' do
       context '/foo' do
-        it_behaves_like 'setup', 'root', '/foo'
+        it_behaves_like 'setup', '/foo'
       end
 
       context 'undef' do
-        let(:facts) {{ :id => 'root' }}
         it 'should fail' do
           expect { should }.to raise_error(Puppet::Error, /#{Regexp.escape('$concat_basedir not defined. Try running again with pluginsync=true on the [master] and/or [main] section of your node\'s \'/etc/puppet/puppet.conf\'.')}/)
         end
@@ -71,7 +55,7 @@ describe 'concat::setup', :type => :class do
   end # facts
 
   context 'called from another module namespace' do
-    let(:facts) {{ :id => 'root', :concat_basedir => '/foo' }}
+    let(:facts) {{ :concat_basedir => '/foo' }}
     it 'should fail' do
       expect { should }.to raise_error(Puppet::Error, /Use of private class concat::setup/)
     end
diff --git a/spec/unit/defines/concat_fragment_spec.rb b/spec/unit/defines/concat_fragment_spec.rb
index b7147b2..57d9ecf 100644
--- a/spec/unit/defines/concat_fragment_spec.rb
+++ b/spec/unit/defines/concat_fragment_spec.rb
@@ -5,15 +5,13 @@ describe 'concat::fragment', :type => :define do
   shared_examples 'fragment' do |title, params|
     params = {} if params.nil?
 
-    id = 'root'
-
     p = {
       :content => nil,
       :source  => nil,
       :order   => 10,
       :ensure  => 'present',
       :mode    => '0644',
-      :owner   => id,
+      :owner   => nil,
       :group   => nil,
       :backup  => 'puppet',
     }.merge(params)
@@ -22,19 +20,9 @@ describe 'concat::fragment', :type => :define do
     safe_target_name = p[:target].gsub(/[\/\n]/, '_')
     concatdir        = '/var/lib/puppet/concat'
     fragdir          = "#{concatdir}/#{safe_target_name}"
-    if p[:group].nil?
-      safe_group = id == 'root' ? 0 : id
-    else
-      safe_group = p[:group]
-    end
 
     let(:title) { title }
-    let(:facts) do
-      {
-       :concat_basedir => concatdir,
-       :id             => id,
-      }
-    end
+    let(:facts) {{ :concat_basedir => concatdir }}
     let(:params) { params }
     let(:pre_condition) do
       "concat{ '#{p[:target]}': }"
@@ -47,7 +35,7 @@ describe 'concat::fragment', :type => :define do
         :ensure  => p[:ensure],
         :mode    => p[:mode],
         :owner   => p[:owner],
-        :group   => safe_group,
+        :group   => p[:group],
         :source  => p[:source],
         :content => p[:content],
         :alias   => "concat_fragment_#{title}",
@@ -66,7 +54,7 @@ describe 'concat::fragment', :type => :define do
     ['./etc/motd', 'etc/motd', false].each do |target|
       context target do
         let(:title) { 'motd_header' }
-        let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+        let(:facts) {{ :concat_basedir => '/tmp' }}
         let(:params) {{ :target => target }}
 
         it 'should fail' do
@@ -88,7 +76,7 @@ describe 'concat::fragment', :type => :define do
 
     context 'invalid' do
       let(:title) { 'motd_header' }
-      let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+      let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :ensure => 'invalid', :target => '/etc/motd' }}
 
       it 'should fail' do
@@ -109,7 +97,7 @@ describe 'concat::fragment', :type => :define do
 
     context 'false' do
       let(:title) { 'motd_header' }
-      let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+      let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :content => false, :target => '/etc/motd' }}
 
       it 'should fail' do
@@ -130,7 +118,7 @@ describe 'concat::fragment', :type => :define do
 
     context 'false' do
       let(:title) { 'motd_header' }
-      let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+      let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :source => false, :target => '/etc/motd' }}
 
       it 'should fail' do
@@ -151,7 +139,7 @@ describe 'concat::fragment', :type => :define do
 
     context 'false' do
       let(:title) { 'motd_header' }
-      let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+      let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :order => false, :target => '/etc/motd' }}
 
       it 'should fail' do
@@ -170,7 +158,7 @@ describe 'concat::fragment', :type => :define do
 
     context 'false' do
       let(:title) { 'motd_header' }
-      let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+      let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :mode => false, :target => '/etc/motd' }}
 
       it 'should fail' do
@@ -189,7 +177,7 @@ describe 'concat::fragment', :type => :define do
 
     context 'false' do
       let(:title) { 'motd_header' }
-      let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+      let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :owner => false, :target => '/etc/motd' }}
 
       it 'should fail' do
@@ -208,7 +196,7 @@ describe 'concat::fragment', :type => :define do
 
     context 'false' do
       let(:title) { 'motd_header' }
-      let(:facts) {{ :concat_basedir => '/tmp', :id => 'root' }}
+      let(:facts) {{ :concat_basedir => '/tmp' }}
       let(:params) {{ :group => false, :target => '/etc/motd' }}
 
       it 'should fail' do
diff --git a/spec/unit/defines/concat_spec.rb b/spec/unit/defines/concat_spec.rb
index 5ec0786..7e69b4f 100644
--- a/spec/unit/defines/concat_spec.rb
+++ b/spec/unit/defines/concat_spec.rb
@@ -5,14 +5,12 @@ describe 'concat', :type => :define do
   shared_examples 'concat' do |title, params| 
     params = {} if params.nil?
 
-    id = 'root'
-
     # default param values
     p = {
       :ensure         => 'present',
       :path           => title,
-      :owner          => id,
-      :group          => '0',
+      :owner          => nil,
+      :group          => nil,
       :mode           => '0644',
       :warn           => false,
       :warn_message   => nil,
@@ -39,12 +37,7 @@ describe 'concat', :type => :define do
 
     let(:title) { title }
     let(:params) { params }
-    let(:facts) do
-      {
-        :concat_basedir => concatdir,
-        :id             => id,
-      }
-    end
+    let(:facts) {{ :concat_basedir => concatdir }}
 
     if p[:ensure] == 'present'
       it do
@@ -102,15 +95,10 @@ 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",
         })
-
-        if id == 'root'
-          should contain_exec("concat_#{title}").with({
-            :user  => 'root',
-            :group => p[:group], 
-          })
-        end
       end
     else
       [