Merge pull request #1989 from opscode/sersut/port-chef-1982-10

Port Chef 1982 to 10-stable

3 files changed, 40 insertions, 30 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b9550fc4d1..e066b538ba 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,10 +1,15 @@
 # Chef Client 10.x Changelog
 
-## 10.34.0
+## Unreleased:
+
+* Fix a bug in user dscl provider to enable managing password and other properties at the same time.
+
+## 10.34.2
 
 * [**Phil Dibowitz**](https://github.com/jaymzh):
   'group' provider on OSX properly uses 'dscl' to determine existing groups
 
+----------------------
 
 
 * `options` attribute of mount resource now supports lazy evaluation. (CHEF-5163)
diff --git a/chef/lib/chef/provider/user/dscl.rb b/chef/lib/chef/provider/user/dscl.rb
index ac5b7e019d..296d12559e 100644
--- a/chef/lib/chef/provider/user/dscl.rb
+++ b/chef/lib/chef/provider/user/dscl.rb
@@ -112,18 +112,18 @@ user password using shadow hash.")
           @current_resource = Chef::Resource::User.new(@new_resource.username)
           @current_resource.username(@new_resource.username)
 
-          user_info = read_user_info
-          if user_info
-            @current_resource.uid(dscl_get(user_info, :uid))
-            @current_resource.gid(dscl_get(user_info, :gid))
-            @current_resource.home(dscl_get(user_info, :home))
-            @current_resource.shell(dscl_get(user_info, :shell))
-            @current_resource.comment(dscl_get(user_info, :comment))
-            @authentication_authority = dscl_get(user_info, :auth_authority)
-
-            if @new_resource.password && dscl_get(user_info, :password) == "********"
+          @user_info = read_user_info
+          if @user_info
+            @current_resource.uid(dscl_get(@user_info, :uid))
+            @current_resource.gid(dscl_get(@user_info, :gid))
+            @current_resource.home(dscl_get(@user_info, :home))
+            @current_resource.shell(dscl_get(@user_info, :shell))
+            @current_resource.comment(dscl_get(@user_info, :comment))
+            @authentication_authority = dscl_get(@user_info, :auth_authority)
+
+            if @new_resource.password && dscl_get(@user_info, :password) == "********"
               # A password is set. Let's get the password information from shadow file
-              shadow_hash_binary = dscl_get(user_info, :shadow_hash)
+              shadow_hash_binary = dscl_get(@user_info, :shadow_hash)
 
               # Calling shell_out directly since we want to give an input stream
               shadow_hash_xml = convert_binary_plist_to_xml(shadow_hash_binary.string)
@@ -160,22 +160,26 @@ user password using shadow hash.")
 
         def create_user
           dscl_create_user
+          # set_password modifies the plist file of the user directly. So update
+          # the password first before making any modifications to the user.
+          set_password
           dscl_create_comment
           dscl_set_uid
           dscl_set_gid
           dscl_set_home
           dscl_set_shell
-          set_password
         end
 
         def manage_user
+          # set_password modifies the plist file of the user directly. So update
+          # the password first before making any modifications to the user.
+          set_password        if diverged_password?
           dscl_create_user    if diverged?(:username)
           dscl_create_comment if diverged?(:comment)
           dscl_set_uid        if diverged?(:uid)
           dscl_set_gid        if diverged?(:gid)
           dscl_set_home       if diverged?(:home)
           dscl_set_shell      if diverged?(:shell)
-          set_password        if diverged_password?
         end
 
         #
@@ -341,22 +345,18 @@ user password using shadow hash.")
             :input => shadow_info.to_plist, :live_stream => shadow_info_binary)
           command.run_command
 
+          if @user_info.nil?
+            # User is  just created. read_user_info() will read the fresh information
+            # for the user with a cache flush. However with experimentation we've seen
+            # that dscl cache is not immediately updated after the creation of the user
+            # This is odd and needs to be investigated further.
+            sleep 3
+            @user_info = read_user_info
+          end
+
           # Replace the shadow info in user's plist
-          user_info = read_user_info
-          dscl_set(user_info, :shadow_hash, shadow_info_binary)
-
-          #
-          # Before saving the user's plist file we need to wait for dscl to
-          # update its caches and flush them to disk. In order to achieve this
-          # we need to wait first for our changes to get into the dscl cache
-          # and then flush the cache to disk before saving password into the
-          # plist file. 3 seconds is the minimum experimental value for dscl
-          # cache to be updated. We can get rid of this sleep when we find a
-          # trigger to update dscl cache.
-          #
-          sleep 3
-          shell_out("dscacheutil '-flushcache'")
-          save_user_info(user_info)
+          dscl_set(@user_info, :shadow_hash, shadow_info_binary)
+          save_user_info(@user_info)
         end
 
         #
@@ -557,6 +557,10 @@ user password using shadow hash.")
         def read_user_info
           user_info = nil
 
+          # We flush the cache here in order to make sure that we read fresh information
+          # for the user. 
+          shell_out("dscacheutil '-flushcache'")
+
           begin
             user_plist_file = "#{USER_PLIST_DIRECTORY}/#{@new_resource.username}.plist"
             user_plist_info = run_plutil("convert xml1 -o - #{user_plist_file}")
diff --git a/chef/spec/unit/provider/user/dscl_spec.rb b/chef/spec/unit/provider/user/dscl_spec.rb
index 3cb18f1979..b272eceada 100644
--- a/chef/spec/unit/provider/user/dscl_spec.rb
+++ b/chef/spec/unit/provider/user/dscl_spec.rb
@@ -376,6 +376,7 @@ ea18e18b720e358e7fbe3cfbeaa561456f6ba008937a30"
     let(:user_plist_file) { nil }
 
     before do
+      provider.should_receive(:shell_out).with("dscacheutil '-flushcache'")
       provider.should_receive(:shell_out).with("plutil -convert xml1 -o - /var/db/dslocal/nodes/Default/users/toor.plist") do
         if user_plist_file.nil?
           ShellCmdResult.new('Can not find the file', 'Sorry!!', 1)
@@ -715,7 +716,6 @@ ea18e18b720e358e7fbe3cfbeaa561456f6ba008937a30")
       provider.should_receive(:read_user_info)
       provider.should_receive(:dscl_set)
       provider.should_receive(:sleep).with(3)
-      provider.should_receive(:shell_out).with("dscacheutil '-flushcache'")
       provider.should_receive(:save_user_info)
       provider.set_password
     end
@@ -822,6 +822,7 @@ ea18e18b720e358e7fbe3cfbeaa561456f6ba008937a30")
 
   describe "when the user exists" do
     before do
+      provider.should_receive(:shell_out).with("dscacheutil '-flushcache'")
       provider.should_receive(:shell_out).with("plutil -convert xml1 -o - /var/db/dslocal/nodes/Default/users/toor.plist") do
         ShellCmdResult.new(File.read(File.join(CHEF_SPEC_DATA, "mac_users/10.9.plist.xml")), "", 0)
       end