Merge pull request #5243 from chef/lcg/rewrite-linux-user-provider

Rewrite linux useradd provider

8 files changed, 190 insertions, 56 deletions

diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index 1f7deb21d2..a258a91075 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -78,6 +78,36 @@ class Chef
         return my_command_args
       end
 
+      # Helper for sublcasses to convert an array of string args into a string.  It
+      # will compact nil or empty strings in the array and will join the array elements
+      # with spaces, without introducing any double spaces for nil/empty elements.
+      #
+      # @param args [String] variable number of string arguments
+      # @return [String] nicely concatenated string or empty string
+      def a_to_s(*args)
+        clean_array(*args).join(" ")
+      end
+
+      # Helper for sublcasses to reject nil and empty strings out of an array.  It allows
+      # using the array form of shell_out (which avoids the need to surround arguments with
+      # quote marks to deal with shells).
+      #
+      # Usage:
+      #   shell_out!(*clean_array("useradd", universal_options, useradd_options, new_resource.username))
+      #
+      # universal_options and useradd_options can be nil, empty array, empty string, strings or arrays
+      # and the result makes sense.
+      #
+      # keeping this separate from shell_out!() makes it a bit easier to write expectations against the
+      # shell_out args and be able to omit nils and such in the tests (and to test that the nils are
+      # being rejected correctly).
+      #
+      # @param args [String] variable number of string arguments
+      # @return [Array] array of strings with nil and null string rejection
+      def clean_array(*args)
+        args.flatten.reject { |i| i.nil? || i == "" }.map(&:to_s)
+      end
+
       private
 
       def shell_out_command(*command_args)
diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb
index 82b85fb1e4..3f641145e6 100644
--- a/lib/chef/provider/package.rb
+++ b/lib/chef/provider/package.rb
@@ -588,16 +588,6 @@ class Chef
         end
         args
       end
-
-      # Helper for sublcasses to convert an array of string args into a string.  It
-      # will compact nil or empty strings in the array and will join the array elements
-      # with spaces, without introducing any double spaces for nil/empty elements.
-      #
-      # @param args [String] variable number of string arguments
-      # @return [String] nicely concatenated string or empty string
-      def a_to_s(*args)
-        args.flatten.reject { |i| i.nil? || i == "" }.join(" ")
-      end
     end
   end
 end
diff --git a/lib/chef/provider/user.rb b/lib/chef/provider/user.rb
index 85bd674d8d..2bc4cc10bc 100644
--- a/lib/chef/provider/user.rb
+++ b/lib/chef/provider/user.rb
@@ -147,10 +147,6 @@ class Chef
         end
       end
 
-      def remove_user
-        raise NotImplementedError
-      end
-
       def action_manage
         if @user_exists && compare_user
           converge_by("manage user #{@new_resource.username}") do
@@ -160,10 +156,6 @@ class Chef
         end
       end
 
-      def manage_user
-        raise NotImplementedError
-      end
-
       def action_modify
         if compare_user
           converge_by("modify user #{@new_resource.username}") do
@@ -184,14 +176,6 @@ class Chef
         end
       end
 
-      def check_lock
-        raise NotImplementedError
-      end
-
-      def lock_user
-        raise NotImplementedError
-      end
-
       def action_unlock
         if check_lock() == true
           converge_by("unlock user #{@new_resource.username}") do
@@ -203,9 +187,29 @@ class Chef
         end
       end
 
+      def create_user
+        raise NotImplementedError
+      end
+
+      def remove_user
+        raise NotImplementedError
+      end
+
+      def manage_user
+        raise NotImplementedError
+      end
+
+      def lock_user
+        raise NotImplementedError
+      end
+
       def unlock_user
         raise NotImplementedError
       end
+
+      def check_lock
+        raise NotImplementedError
+      end
     end
   end
 end
diff --git a/lib/chef/provider/user/linux.rb b/lib/chef/provider/user/linux.rb
index ca331311f1..ff1bc9c4c8 100644
--- a/lib/chef/provider/user/linux.rb
+++ b/lib/chef/provider/user/linux.rb
@@ -14,18 +14,125 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-require "chef/provider/user/useradd"
+require "chef/provider/user"
 
 class Chef
   class Provider
     class User
-      class Linux < Chef::Provider::User::Useradd
-        # MAJOR XXX: the implementation of "linux" is the base class and all needs to be moved here
+      class Linux < Chef::Provider::User
         provides :linux_user
         provides :user, os: "linux"
 
-        def managing_home_dir?
-          new_resource.manage_home # linux always 'supports' manage_home
+        def create_user
+          shell_out!(*clean_array("useradd", universal_options, useradd_options, new_resource.username))
+        end
+
+        def manage_user
+          shell_out!(*clean_array("usermod", universal_options, usermod_options, new_resource.username))
+        end
+
+        def remove_user
+          shell_out!(*clean_array("userdel", userdel_options, new_resource.username))
+        end
+
+        def lock_user
+          shell_out!(*clean_array("usermod", "-L", new_resource.username))
+        end
+
+        def unlock_user
+          shell_out!(*clean_array("usermod", "-U", new_resource.username))
+        end
+
+        # common to usermod and useradd
+        def universal_options
+          opts = []
+          opts << "-c" << new_resource.comment if should_set?(:comment)
+          opts << "-g" << new_resource.gid if should_set?(:gid)
+          opts << "-p" << new_resource.password if should_set?(:password)
+          opts << "-s" << new_resource.shell if should_set?(:shell)
+          opts << "-u" << new_resource.uid if should_set?(:uid)
+          opts << "-d" << new_resource.home if updating_home?
+          opts << "-o" if new_resource.non_unique
+          opts
+        end
+
+        def usermod_options
+          opts = []
+          if updating_home?
+            if new_resource.manage_home
+              opts << "-m"
+            end
+          end
+          opts
+        end
+
+        def useradd_options
+          opts = []
+          opts << "-r" if new_resource.system
+          if new_resource.manage_home
+            opts << "-m"
+          else
+            opts << "-M"
+          end
+          opts
+        end
+
+        def userdel_options
+          opts = []
+          opts << "-r" if new_resource.manage_home
+          opts << "-f" if new_resource.force
+          opts
+        end
+
+        def should_set?(sym)
+          current_resource.send(sym).to_s != new_resource.send(sym).to_s && new_resource.send(sym)
+        end
+
+        def updating_home?
+          return false unless new_resource.home
+          return true unless current_resource.home
+          new_resource.home && Pathname.new(current_resource.home).cleanpath != Pathname.new(new_resource.home).cleanpath
+        end
+
+        # FIXME: see if we can clean this up
+        def check_lock
+          # we can get an exit code of 1 even when it's successful on
+          # rhel/centos (redhat bug 578534). See additional error checks below.
+          passwd_s = shell_out!("passwd", "-S", new_resource.username, :returns => [0, 1])
+          if whyrun_mode? && passwd_s.stdout.empty? && passwd_s.stderr.match(/does not exist/)
+            # if we're in whyrun mode and the user is not yet created we assume it would be
+            return false
+          end
+
+          raise Chef::Exceptions::User, "Cannot determine if #{@new_resource} is locked!" if passwd_s.stdout.empty?
+
+          status_line = passwd_s.stdout.split(" ")
+          case status_line[1]
+          when /^P/
+            @locked = false
+          when /^N/
+            @locked = false
+          when /^L/
+            @locked = true
+          end
+
+          unless passwd_s.exitstatus == 0
+            raise_lock_error = false
+            if %w{redhat centos}.include?(node[:platform])
+              passwd_version_check = shell_out!("rpm -q passwd")
+              passwd_version = passwd_version_check.stdout.chomp
+
+              unless passwd_version == "passwd-0.73-1"
+                raise_lock_error = true
+              end
+            else
+              raise_lock_error = true
+            end
+
+            raise Chef::Exceptions::User, "Cannot determine if #{new_resource} is locked!" if raise_lock_error
+          end
+
+          @locked
         end
       end
     end
diff --git a/lib/chef/resource/user/linux_user.rb b/lib/chef/resource/user/linux_user.rb
index 3452459e39..23d6129373 100644
--- a/lib/chef/resource/user/linux_user.rb
+++ b/lib/chef/resource/user/linux_user.rb
@@ -32,7 +32,7 @@ class Chef
             manage_home: true,
             non_unique: true,
           }
-          @manage_home = true
+          @manage_home = false
         end
 
         def supports(args = {})
@@ -42,7 +42,6 @@ class Chef
         end
 
         def supports=(args)
-          Chef.log_deprecation "setting supports on the linux_user resource is deprecated"
           # setting is deliberately disabled
           supports({})
         end
diff --git a/spec/functional/resource/user/useradd_spec.rb b/spec/functional/resource/user/useradd_spec.rb
index fb16a2b858..dadf696185 100644
--- a/spec/functional/resource/user/useradd_spec.rb
+++ b/spec/functional/resource/user/useradd_spec.rb
@@ -94,6 +94,7 @@ describe Chef::Provider::User::Useradd, metadata do
 
     ["cf-test"].each do |u|
       r = resource_for_platform("DELETE USER", run_context)
+      r.manage_home true
       r.username("cf-test")
       r.run_action(:remove)
     end
@@ -257,6 +258,14 @@ describe Chef::Provider::User::Useradd, metadata do
             expect(File).to exist(home)
           end
         end
+
+        context "and manage_home is the default" do
+          let(:manage_home) { nil }
+
+          it "does not create the home dir without `manage_home'" do
+            expect(File).not_to exist(home)
+          end
+        end
       end
 
       context "when a password is specified" do
diff --git a/spec/support/shared/unit/provider/useradd_based_user_provider.rb b/spec/support/shared/unit/provider/useradd_based_user_provider.rb
index de851c4ad4..fc12b2d5b6 100644
--- a/spec/support/shared/unit/provider/useradd_based_user_provider.rb
+++ b/spec/support/shared/unit/provider/useradd_based_user_provider.rb
@@ -100,7 +100,7 @@ shared_examples_for "a useradd-based user provider" do |supported_useradd_option
 
       it "should set useradd -r" do
         @new_resource.system(true)
-        expect(provider.useradd_options).to eq([ "-r" ])
+        expect(provider.useradd_options).to eq([ "-r", "-m" ])
       end
     end
 
@@ -111,21 +111,21 @@ shared_examples_for "a useradd-based user provider" do |supported_useradd_option
       end
 
       it "should set -m -d /homedir" do
-        expect(provider.universal_options).to eq(%w{-d /wowaweea -m})
-        expect(provider.useradd_options).to eq([])
+        expect(provider.universal_options).to eq(%w{-d /wowaweea})
+        expect(provider.usermod_options).to eq(%w{-m})
       end
     end
 
     describe "when the resource has a different home directory and supports home directory management (using real attributes)" do
       before do
-        allow(@new_resource).to receive(:home).and_return("/wowaweea")
-        allow(@new_resource).to receive(:manage_home).and_return(true)
-        allow(@new_resource).to receive(:non_unique).and_return(false)
+        @new_resource.home("/wowaweea")
+        @new_resource.manage_home true
+        @new_resource.non_unique false
       end
 
       it "should set -m -d /homedir" do
-        expect(provider.universal_options).to eql(%w{-d /wowaweea -m})
-        expect(provider.useradd_options).to eq([])
+        expect(provider.universal_options).to eq(%w{-d /wowaweea})
+        expect(provider.usermod_options).to eq(%w{-m})
       end
     end
 
@@ -179,7 +179,7 @@ shared_examples_for "a useradd-based user provider" do |supported_useradd_option
         command.concat(["-p", "abracadabra"]) if supported_useradd_options.key?("password")
         command.concat([ "-s", "/usr/bin/zsh",
                          "-u", "1000",
-                         "-r",
+                         "-r", "-m",
                          "adam" ])
         expect(provider).to receive(:shell_out!).with(*command).and_return(true)
         provider.create_user
@@ -220,7 +220,7 @@ shared_examples_for "a useradd-based user provider" do |supported_useradd_option
     end
 
     it "CHEF-3429: does not set -m if we aren't changing the home directory" do
-      expect(provider).to receive(:updating_home?).and_return(false)
+      expect(provider).to receive(:updating_home?).at_least(:once).and_return(false)
       command = ["usermod",
                   "-g", "23",
                   "adam" ]
diff --git a/spec/unit/provider/user/linux_spec.rb b/spec/unit/provider/user/linux_spec.rb
index ef12cec234..ac94592859 100644
--- a/spec/unit/provider/user/linux_spec.rb
+++ b/spec/unit/provider/user/linux_spec.rb
@@ -39,16 +39,6 @@ describe Chef::Provider::User::Linux do
 
   include_examples "a useradd-based user provider", supported_useradd_options
 
-  describe "manage_user" do
-    # CHEF-5247: Chef::Provider::User::Solaris subclasses Chef::Provider::User::Useradd, but does not use usermod to change passwords.
-    # Thus, a call to Solaris#manage_user calls Solaris#manage_password and Useradd#manage_user, but the latter should be a no-op.
-    it "should not run the command if universal_options is an empty array" do
-      allow(provider).to receive(:universal_options).and_return([])
-      expect(provider).not_to receive(:shell_out!)
-      provider.manage_user
-    end
-  end
-
   describe "manage_home behavior" do
     before(:each) do
       @new_resource = Chef::Resource::User::LinuxUser.new("adam", @run_context)
@@ -66,16 +56,21 @@ describe Chef::Provider::User::Linux do
     end
 
     it "defaults manage_home to true" do
-      expect( @new_resource.manage_home ).to be true
+      expect( @new_resource.manage_home ).to be false
     end
 
-    it "by default manage_home is true and we do not -M" do
-      expect( provider.useradd_options ).to eql([])
+    it "by default manage_home is false and we use -M" do
+      expect( provider.useradd_options ).to eql(["-M"])
     end
 
     it "setting manage_home to false includes -M" do
       @new_resource.manage_home false
       expect( provider.useradd_options ).to eql(["-M"])
     end
+
+    it "setting manage_home to true includes -m" do
+      @new_resource.manage_home true
+      expect( provider.useradd_options ).to eql(["-m"])
+    end
   end
 end