(PUP-11067) Prioritize user type ensure before purge_ssh_keys

This commit better guards against missing or invalid homedir of a user
when the `purge_ssh_keys` parameter is set in a manifest. It also
prioritizes the `ensure` field (when it is set to `present`) instead of
`purge_ssh_keys`.

Author: luchihoratiu

acceptance/tests/resource/user/should_manage_purge_ssh_keys_for_new_user.rb

@@ -0,0 +1,86 @@
+test_name 'should manage purge_ssh_keys for new user' do
+
+  tag 'audit:high',
+      'audit:acceptance'
+
+  name = "usr#{rand(9999).to_i}"
+
+  agents.each do |agent|
+    teardown do
+      on(agent, puppet_resource('user', "#{name}", 'ensure=absent'))
+    end
+
+    home = agent.tmpdir(name)
+    authorized_keys_file = agent.tmpfile("authorized_keys")
+
+    step "create user #{name} with ssh keys purged and expect no failure" do
+      apply_manifest_on(agent, <<-MANIFEST, { :catch_failures => true, :debug => true }) do |result|
+          user {'#{name}':
+            ensure => present,
+            home => '#{home}',
+            purge_ssh_keys => true
+          }
+        MANIFEST
+
+        assert_no_match(/User '#{name}' has no home directory set to purge ssh keys from./, result.stdout)
+      end
+    end
+
+    step "remove user #{name} and purge ssh keys purged and expect no failure" do
+      apply_manifest_on(agent, <<-MANIFEST, { :catch_failures => true, :debug => true }) do |result|
+          user {'#{name}':
+            ensure => absent,
+            home => '#{home}',
+            purge_ssh_keys => true
+          }
+        MANIFEST
+
+        assert_no_match(/User '#{name}' has no home directory set to purge ssh keys from./, result.stdout)
+      end
+    end
+
+    # Platforms such as macOS does not support the `managehome` parameter
+    # which we're expecting to remove homedir when ensure of user is set
+    # to absent
+    step "remove homedir" do
+      agent.rm_rf(home)
+    end
+
+    step "expect debug log with home directory missing on second run of same manifest" do
+      apply_manifest_on(agent, <<-MANIFEST, { :catch_failures => true, :debug => true }) do |result|
+          user {'#{name}':
+            ensure => absent,
+            home => '#{home}',
+            purge_ssh_keys => true
+          }
+        MANIFEST
+
+        assert_match(/User '#{name}' has no home directory set to purge ssh keys from./, result.stdout)
+      end
+    end
+
+    step "expect debug log with home directory missing when purge_ssh_keys has relative path to home" do
+      apply_manifest_on(agent, <<-MANIFEST, { :catch_failures => true, :debug => true }) do |result|
+          user {'#{name}':
+            ensure => absent,
+            purge_ssh_keys => '~/authorized_keys'
+          }
+        MANIFEST
+
+        assert_match(/User '#{name}' has no home directory set to purge ssh keys from./, result.stdout)
+      end
+    end
+
+    step "expect no debug log with home directory missing when purge_ssh_keys has absolute path" do
+      apply_manifest_on(agent, <<-MANIFEST, { :catch_failures => true, :debug => true }) do |result|
+          user {'#{name}':
+            ensure => absent,
+            purge_ssh_keys => '#{authorized_keys_file}'
+          }
+        MANIFEST
+
+        assert_no_match(/User '#{name}' has no home directory set to purge ssh keys from./, result.stdout)
+      end
+    end
+  end
+end

lib/puppet/type/user.rb

@@ -67,6 +67,7 @@ module Puppet
     newproperty(:ensure, :parent => Puppet::Property::Ensure) do
       newvalue(:present, :event => :user_created) do
         provider.create
+        @resource.generate
       end
 
       newvalue(:absent, :event => :user_removed) do
@@ -695,6 +696,7 @@ def delimiter
 
     def generate
       if !self[:purge_ssh_keys].empty?
+        return [] if self[:ensure] == :present && !provider.exists? 
         if Puppet::Type.type(:ssh_authorized_key).nil?
           warning _("Ssh_authorized_key type is not available. Cannot purge SSH keys.")
         else
@@ -743,25 +745,6 @@ def generate
         end
         raise ArgumentError, _("purge_ssh_keys must be true, false, or an array of file names, not %{value}") % { value: value.inspect }
       end
-
-      munge do |value|
-        # Resolve string, boolean and symbol forms of true and false to a
-        # single representation.
-        test_sym = value.to_s.intern
-        value = test_sym if [:true, :false].include? test_sym
-
-        return [] if value == :false
-        home = resource[:home] || Dir.home(resource[:name])
-
-        return [ "#{home}/.ssh/authorized_keys" ] if value == :true
-        # value is an array - munge each value
-        [ value ].flatten.map do |entry|
-          # make sure frozen value is duplicated by using a gsub, second mutating gsub! is then ok
-          entry = entry.gsub(/^~\//, "#{home}/")
-          entry.gsub!(/^%h\//, "#{home}/")
-          entry
-        end
-      end
     end
 
     newproperty(:loginclass, :required_features => :manages_loginclass) do
@@ -783,7 +766,7 @@ def generate
     # @see generate
     # @api private
     def find_unmanaged_keys
-      self[:purge_ssh_keys].
+      munged_unmanaged_keys.
         select { |f| File.readable?(f) }.
         map { |f| unknown_keys_in_file(f) }.
         flatten.each do |res|
@@ -795,6 +778,41 @@ def find_unmanaged_keys
         end
     end
 
+    def munged_unmanaged_keys
+      value = self[:purge_ssh_keys]
+
+      # Resolve string, boolean and symbol forms of true and false to a
+      # single representation.
+      test_sym = value.to_s.intern
+      value = test_sym if [:true, :false].include? test_sym
+
+      return [] if value == :false
+
+      home = self[:home]
+      begin
+        home ||= provider.home
+      rescue
+        Puppet.debug("User '#{self[:name]}' does not exist")
+      end
+
+      if home.to_s.empty? || !Dir.exist?(home.to_s)
+        if value == :true || [ value ].flatten.any? { |v| v.start_with?('~/', '%h/') }
+          Puppet.debug("User '#{self[:name]}' has no home directory set to purge ssh keys from.")
+          return []
+        end
+      end
+
+      return [ "#{home}/.ssh/authorized_keys" ] if value == :true
+
+      # value is an array - munge each value
+      [ value ].flatten.map do |entry|
+        # make sure frozen value is duplicated by using a gsub, second mutating gsub! is then ok
+        entry = entry.gsub(/^~\//, "#{home}/")
+        entry.gsub!(/^%h\//, "#{home}/")
+        entry
+      end
+    end
+
     # Parse an ssh authorized keys file superficially, extract the comments
     # on the keys. These are considered names of possible ssh_authorized_keys
     # resources. Keys that are managed by the present catalog are ignored.

spec/unit/type/user_spec.rb

@@ -174,6 +174,51 @@ def self.instances; []; end
     end
   end
 
+  describe "when managing the purge_ssh_keys property" do
+    context "with valid input" do
+      it "should support a :true value" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => :true) }.to_not raise_error
+      end
+
+      it "should support a :false value" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => :false) }.to_not raise_error
+      end
+
+      it "should support a String value" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => File.expand_path('home/foo/.ssh/authorized_keys')) }.to_not raise_error
+      end
+
+      it "should support an Array value" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => [File.expand_path('home/foo/.ssh/authorized_keys'),
+          File.expand_path('custom/authorized_keys')]) }.to_not raise_error
+      end
+    end
+
+    context "with faulty input" do
+      it "should raise error for relative path" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => 'home/foo/.ssh/authorized_keys') }.to raise_error(Puppet::ResourceError,
+          /Paths to keyfiles must be absolute/ )
+      end
+
+      it "should raise error for invalid type" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => :invalid) }.to raise_error(Puppet::ResourceError,
+          /purge_ssh_keys must be true, false, or an array of file names/ )
+      end
+
+      it "should raise error for array with relative path" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => ['home/foo/.ssh/authorized_keys',
+          File.expand_path('custom/authorized_keys')]) }.to raise_error(Puppet::ResourceError,
+          /Paths to keyfiles must be absolute/ )
+      end
+
+      it "should raise error for array with invalid type" do
+        expect { described_class.new(:name => 'foo', :purge_ssh_keys => [:invalid,
+          File.expand_path('custom/authorized_keys')]) }.to raise_error(Puppet::ResourceError,
+          /Each entry for purge_ssh_keys must be a string/ )
+      end
+    end
+  end
+
   describe "when managing the uid property" do
     it "should convert number-looking strings into actual numbers" do
       expect(described_class.new(:name => 'foo', :uid => '50')[:uid]).to eq(50)