Address review feedback round 1

* Fix 'iterations' description
* Update 'gid' property, remove coercion, resolve group name and id in
  provider
* Move validation of password value when given salt to resource property
  coercion
* Restructure exception handling around calls
* Don't re-map properties onto current_resource after
  load_current_resource
* Execute inline group resources with run_action
* Always set the home directory when creating a user
* Poll for user creating instead of a hard sleep
* Remove old ascii encoding copy pasta

Signed-off-by: Ryan Cragun <ryan@chef.io>

2 files changed, 150 insertions, 115 deletions

diff --git a/lib/chef/provider/user/mac.rb b/lib/chef/provider/user/mac.rb
index 826b862251..196f52cc31 100644
--- a/lib/chef/provider/user/mac.rb
+++ b/lib/chef/provider/user/mac.rb
@@ -74,14 +74,27 @@ class Chef
         end
 
         def load_current_resource
-          convert_group_name if new_resource.property_is_set?(:gid)
-
           @current_resource = Chef::Resource::User::MacUser.new(new_resource.username)
           current_resource.username(new_resource.username)
 
-          @user_plist = load_user
+          reload_user_plist
           if user_plist
-            reload_with_user_plist
+
+            current_resource.uid(dscl_get(user_plist, :uid)[0])
+            current_resource.gid(dscl_get(user_plist, :gid)[0])
+            current_resource.home(dscl_get(user_plist, :home)[0])
+            current_resource.shell(dscl_get(user_plist, :shell)[0])
+            current_resource.comment(dscl_get(user_plist, :comment)[0])
+
+            shadow_hash = dscl_get(user_plist, :shadow_hash)
+            if shadow_hash
+              current_resource.password(shadow_hash[0]["SALTED-SHA512-PBKDF2"]["entropy"].string.unpack("H*")[0])
+              current_resource.salt(shadow_hash[0]["SALTED-SHA512-PBKDF2"]["salt"].string.unpack("H*")[0])
+              current_resource.iterations(shadow_hash[0]["SALTED-SHA512-PBKDF2"]["iterations"].to_i)
+            end
+
+            current_resource.secure_token(secure_token_enabled?)
+            current_resource.admin(admin_user?)
           else
             @user_exists = false
             logger.trace("#{new_resource} user does not exist")
@@ -90,73 +103,63 @@ class Chef
           current_resource
         end
 
-        def load_user
+        def reload_user_plist
+          @admin_group_info = nil
+
           # Load the user information.
-          user_xml = run_dscl("read", "/Users/#{new_resource.username}")
+          begin
+            user_xml = run_dscl("read", "/Users/#{new_resource.username}")
+          rescue Chef::Exceptions::DsclCommandFailed
+            return nil
+          end
 
           # User doesn't exist yet so there's not info
           return nil if user_xml.nil? || user_xml == ""
 
-          user_plist = Plist.parse_xml(user_xml)
-
-          # The password infomation is stored in the ShadowHashData key in the
-          # plist. However, parsing it is a bit tricky as the value is itself
-          # another encoded binary plist. We have to extract the encoded plist,
-          # decode it from hex to a binary plist and then convert the binary
-          # into XML plist. From there we can extract the hash data.
-          #
-          # NOTE: `dscl -read` and `plutil -convert` return different values for ShadowHashData.
-          #
-          # `dscl` returns the value encoded as an octal hex string and stored as a <string>
-          # `plutil` returns the value encoded as a base64 string stored as <data>
-          #
-          #  eg:
-          #
-          # <array>
-          #   <string>77687920 63616e27 74206170 706c6520 6275696c 6420636f 6e736973 74656e74 20746f6f 6c696e67</string>
-          # </array>
-          #
-          # vs
-          #
-          # <array>
-          #   <data>AADKAAAKAA4LAA0MAAAAAAAAAAA=</data>
-          # </array>
-          #
-          shadow_hash_hex = dscl_get(user_plist, :shadow_hash)[0]
-
-          return user_plist unless shadow_hash_hex && shadow_hash_hex != ""
-
-          shadow_binary_plist = [shadow_hash_hex.delete(" ")].pack("H*")
-          shadow_xml_plist = shell_out("plutil", "-convert", "xml1", "-o", "-", "-", input: shadow_binary_plist).stdout
-          dscl_set(user_plist, :shadow_hash, Plist.parse_xml(shadow_xml_plist))
+          @user_plist = Plist.parse_xml(user_xml)
 
-          user_plist
-        rescue Chef::Exceptions::PlistUtilCommandFailed, Chef::Exceptions::DsclCommandFailed
-          nil
-        end
-
-        # Map updated user information from the user_plist onto the current resource
-        # and instance variables.
-        def reload_with_user_plist
-          current_resource.uid(dscl_get(user_plist, :uid)[0])
-          current_resource.gid(dscl_get(user_plist, :gid)[0])
-          current_resource.home(dscl_get(user_plist, :home)[0])
-          current_resource.shell(dscl_get(user_plist, :shell)[0])
-          current_resource.comment(dscl_get(user_plist, :comment)[0])
-
-          shadow_hash = dscl_get(user_plist, :shadow_hash)
-          if shadow_hash
-            current_resource.password(shadow_hash[0]["SALTED-SHA512-PBKDF2"]["entropy"].string.unpack("H*")[0])
-            current_resource.salt(shadow_hash[0]["SALTED-SHA512-PBKDF2"]["salt"].string.unpack("H*")[0])
-            current_resource.iterations(shadow_hash[0]["SALTED-SHA512-PBKDF2"]["iterations"].to_i)
+          begin
+            @auth_authority = dscl_get(user_plist, :auth_authority)
+            @guid = dscl_get(user_plist, :guid)
+            shadow_hash_hex = dscl_get(user_plist, :shadow_hash)[0]
+          rescue Chef::Exceptions::DsclCommandFailed
+            # It's possible that the users don't have the fields we're trying
+            # to access so we'll gracefully handle those errors.
+            return nil
           end
 
-          @auth_authority = dscl_get(user_plist, :auth_authority)
-          @guid = dscl_get(user_plist, :guid)
-          @admin_group_info = nil
-
-          current_resource.secure_token(secure_token_enabled?)
-          current_resource.admin(admin_user?)
+          return unless shadow_hash_hex && shadow_hash_hex != ""
+
+          begin
+            # The password infomation is stored in the ShadowHashData key in the
+            # plist. However, parsing it is a bit tricky as the value is itself
+            # another encoded binary plist. We have to extract the encoded plist,
+            # decode it from hex to a binary plist and then convert the binary
+            # into XML plist. From there we can extract the hash data.
+            #
+            # NOTE: `dscl -read` and `plutil -convert` return different values for ShadowHashData.
+            #
+            # `dscl` returns the value encoded as a hex string and stored as a <string>
+            # `plutil` returns the value encoded as a base64 string stored as <data>
+            #
+            #  eg:
+            #
+            # <array>
+            #   <string>77687920 63616e27 74206170 706c6520 6275696c 6420636f 6e736973 74656e74 20746f6f 6c696e67</string>
+            # </array>
+            #
+            # vs
+            #
+            # <array>
+            #   <data>AADKAAAKAA4LAA0MAAAAAAAAAAA=</data>
+            # </array>
+            #
+            shadow_binary_plist = [shadow_hash_hex.delete(" ")].pack("H*")
+            shadow_xml_plist = shell_out("plutil", "-convert", "xml1", "-o", "-", "-", input: shadow_binary_plist).stdout
+            dscl_set(user_plist, :shadow_hash, Plist.parse_xml(shadow_xml_plist))
+          rescue Chef::Exceptions::PlistUtilCommandFailed, Chef::Exceptions::DsclCommandFailed
+            nil
+          end
         end
 
         #
@@ -167,7 +170,7 @@ class Chef
           cmd = [-"-addUser", new_resource.username]
           cmd += ["-fullName", new_resource.comment]  if new_resource.property_is_set?(:comment)
           cmd += ["-UID", new_resource.uid]           if new_resource.property_is_set?(:uid)
-          cmd += ["-shell", new_resource.shell]       if new_resource.property_is_set?(:shell)
+          cmd += ["-shell", new_resource.shell]
           cmd += ["-home", new_resource.home]         if new_resource.property_is_set?(:home)
           cmd += ["-admin"]                           if new_resource.admin
 
@@ -187,12 +190,11 @@ class Chef
             raise Chef::Exceptions::User, "error when creating user: #{res}"
           end
 
-          # Wait for the user to be created and for dscl to flush everything
-          sleep 3
+          # Wait for the user to show up in the ds cache
+          wait_for_user
 
           # Reload with up-to-date user information
-          @user_plist = load_user
-          reload_with_user_plist
+          reload_user_plist
 
           converge_by("set password") { set_password } if new_resource.property_is_set?(:password)
 
@@ -218,9 +220,9 @@ class Chef
             declare_resource(:group, group_name) do
               members new_resource.username
               gid group_id if group_id
-              action group_action
+              action :nothing
               append true
-            end
+            end.run_action(group_action)
 
             converge_by("create primary group ID") do
               run_dscl("create", "/Users/#{new_resource.username}", "PrimaryGroupID", new_resource.gid)
@@ -228,16 +230,10 @@ class Chef
           end
 
           converge_by("alter SecureToken") { toggle_secure_token } if diverged?(:secure_token)
-
-          @user_exists = true
         end
 
         def compare_user
-          %i{comment shell uid gid salt password admin secure_token}.each do |m|
-            return true if diverged?(m)
-          end
-
-          false
+          %i{comment shell uid gid salt password admin secure_token}.any? { |m| diverged?(m) }
         end
 
         def manage_user
@@ -270,9 +266,9 @@ class Chef
                   excluded_members new_resource.username
                 end
 
-                action :create
+                action :nothing
                 append true
-              end
+              end.run_action(:create)
 
               admins = dscl_get(admin_group_info, :group_members)
               if new_resource.admin
@@ -291,9 +287,9 @@ class Chef
           declare_resource(:group, group_name) do
             gid group_id if group_id
             members new_resource.username
-            action group_action
+            action :nothing
             append true
-          end
+          end.run_action(group_action)
 
           if diverged?(:gid)
             converge_by("alter group membership") do
@@ -348,17 +344,23 @@ class Chef
         def diverged?(prop)
           prop = prop.to_sym
 
-          if prop == :password
-            return password_diverged?
-          elsif prop == :gid
-            return user_group_diverged?
+          case prop
+          when :password
+            password_diverged?
+          when :gid
+            user_group_diverged?
+          when :secure_token
+            secure_token_diverged?
+          else
+            # Other fields are have been set on current resource so just compare
+            # them.
+            new_resource.property_is_set?(prop) && (new_resource.send(prop) != current_resource.send(prop))
           end
-
-          # Other fields are have been set on current resource so just compare
-          # them.
-          new_resource.property_is_set?(prop) && (new_resource.send(prop) != current_resource.send(prop))
         end
 
+        # Attempt to resolve the group name, gid, and the action required for
+        # associated group resource. If a group exists we'll modify it, otherwise
+        # create it.
         def user_group_info
           @user_group_info ||= begin
             if new_resource.gid.is_a?(String)
@@ -387,6 +389,10 @@ class Chef
           end
         end
 
+        def secure_token_diverged?
+          new_resource.secure_token ? !secure_token_enabled? : secure_token_enabled?
+        end
+
         def toggle_secure_token
           # Check for this lazily as we only need to validate for these credentials
           # if we're toggling secure token.
@@ -426,9 +432,13 @@ class Chef
         def user_group_diverged?
           return false unless new_resource.property_is_set?(:gid)
 
-          _, group_id = user_group_info
+          group_name, group_id = user_group_info
 
-          current_resource.gid != group_id
+          if current_resource.gid.is_a?(String)
+            current_resource.gid != group_name
+          else
+            current_resource.gid != group_id.to_i
+          end
         end
 
         def password_diverged?
@@ -553,6 +563,23 @@ class Chef
           ::File.delete(import_file) if defined?(import_file) && ::File.exist?(import_file)
         end
 
+        def wait_for_user
+          timeout = Time.now + 5
+
+          loop do
+            begin
+              run_dscl("read", "/Users/#{new_resource.username}", "ShadowHashData")
+              break
+            rescue Chef::Exceptions::DsclCommandFailed => e
+              if Time.now < timeout
+                sleep 0.1
+              else
+                raise Chef::Exceptions::User, e.message
+              end
+            end
+          end
+        end
+
         def run_dsimport(*args)
           shell_out!("dsimport", *(args.compact))
         end
@@ -595,11 +622,7 @@ class Chef
           result = shell_out("plutil", "-#{args[0]}", args[1..-1])
           raise(Chef::Exceptions::PlistUtilCommandFailed, "plutil error: #{result.inspect}") unless result.exitstatus == 0
 
-          if result.stdout.encoding == Encoding::ASCII_8BIT
-            result.stdout.encode("utf-8", "binary", undef: :replace, invalid: :replace, replace: "?")
-          else
-            result.stdout
-          end
+          result.stdout
         end
       end
     end
diff --git a/lib/chef/resource/user/mac_user.rb b/lib/chef/resource/user/mac_user.rb
index 29f52b9ed2..37b985fabc 100644
--- a/lib/chef/resource/user/mac_user.rb
+++ b/lib/chef/resource/user/mac_user.rb
@@ -29,11 +29,12 @@ class Chef
       # * This resource and the corresponding provider have been modified to
       #   work with default macOS TCC policies. Direct access to user binary
       #   plists are no longer permitted by default, thus we've chosen to use
-      #   use newer methods of creating, modifying and removing users.
+      #   a combination of newer utilities for managing user lifecycles and older
+      #   utilities for managing passwords.
       #
-      # * Due to the tooling required by the provider this provider is only
-      #   suitable for use on macOS >= 10.14. Support for older platforms has
-      #   been removed.
+      # * Due to tooling changes that were necessitated by the new policy
+      #   restrictions the mac_user resource is only suitable for use on macOS
+      #   >= 10.14. Support for older platforms has been removed.
       #
       # New Features:
       #
@@ -42,15 +43,20 @@ class Chef
       # * 'admin' is now a boolean property that configures a user to an admin.
       #
       # * 'admin_username' and 'admin_password' are new properties that define the
-      #   admin user credentials required for toggling SecureToken for an
-      #   exiting user.
+      #   admin user credentials required for toggling SecureToken for a user.
       #
-      #   The 'admin_username' must correspond to a system admin with SecureToken
-      #   enabled in order to toggle SecureToken.
+      #   The value of 'admin_username' must correspond to a system user that
+      #   is part of the 'admin' with SecureToken enabled in order to toggle
+      #   SecureToken.
       #
       # * 'secure_token' is a boolean property that sets the desired state
       #   for SecureToken. SecureToken token is required for FileVault full
       #   disk encryption.
+      #
+      # * 'secure_token_password' is the plaintext password required to enable
+      #   or disable secure_token for a user. If no salt is specified we assume
+      #   the 'password' property corresponds to a plaintext password and will
+      #   attempt to use it in place of secure_token_password if it not set.
       class MacUser < Chef::Resource::User
         resource_name :mac_user
 
@@ -58,26 +64,32 @@ class Chef
         provides :user, os: "darwin", platform_version: ">= 10.14"
 
         property :iterations, Integer,
-          description: "macOS platform only. The number of iterations for a password with a SALTED-SHA512-PBKDF2 shadow hash.",
+          description: "The number of iterations for a password with a SALTED-SHA512-PBKDF2 shadow hash.",
           default: 57803, desired_state: false
 
-        # Overload gid so we can set our default. NilClass is for backwards compat
-        # and 20 is the macOS "staff" group.
-        property :gid, [String, Integer, NilClass], description: "The numeric group identifier.", default: 20, coerce: ->(gid) do
+        # Overload gid to set our default gid to 20, the macOS "staff" group.
+        # We also allow a string group name here which we'll attempt to resolve
+        # or create in the provider.
+        property :gid, [Integer, String], description: "The numeric group identifier.", default: 20, coerce: ->(gid) do
           begin
-            return 20 if gid.nil?
-
-            return Etc.getgrnam(gid).gid if gid.is_a?(String)
-
-            Integer(gid)
+            Integer(gid) # Try and coerce a group id string into an integer
           rescue
-            gid
+            gid # assume we have a group name
           end
         end
 
         # Overload the password so we can set a length requirements and update the
         # description.
-        property :password, String, description: "The plain text user password", sensitive: true, callbacks: {
+        property :password, String, description: "The plain text user password", sensitive: true, coerce: ->(password) {
+          # It would be nice if this could be in callbacks but we need the context
+          # of the resource to get the salt property so we have to do it in coerce.
+          if salt && password !~ /^[[:xdigit:]]{256}$/
+            raise Chef::Exceptions::User, "Password must be a SALTED-SHA512-PBKDF2 shadow hash entropy when a shadow hash salt is given"
+          end
+
+          password
+        },
+        callbacks: {
           "Password length must be >= 4" => ->(password) { password.size >= 4 },
         }