Merge pull request #11898 from chef/api-based-macos-userdefaults

rewrite macos_userdefaults using corefoundation gem

7 files changed, 225 insertions, 223 deletions

diff --git a/.github/workflows/func_spec.yml b/.github/workflows/func_spec.yml
index 70089b4bb1..27cee8a7f4 100644
--- a/.github/workflows/func_spec.yml
+++ b/.github/workflows/func_spec.yml
@@ -23,3 +23,18 @@ jobs:
         ruby-version: ${{ matrix.ruby }}
         bundler-cache: true
     - run: bundle exec rspec spec/functional/resource/chocolatey_package_spec.rb
+  userdefaults:
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [macos-10.15, macos-11]
+        # Due to https://github.com/actions/runner/issues/849, we have to use quotes for '3.0'
+        ruby: [2.7, '3.0']
+    runs-on: ${{ matrix.os }}
+    steps:
+    - uses: actions/checkout@v2
+    - uses: ruby/setup-ruby@v1
+      with:
+        ruby-version: ${{ matrix.ruby }}
+        bundler-cache: true
+    - run: bundle exec rspec spec/functional/resource/macos_userdefaults_spec.rb
diff --git a/Gemfile.lock b/Gemfile.lock
index e276527027..c0b5bd8b73 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -43,6 +43,7 @@ PATH
       chef-utils (= 17.7.8)
       chef-vault
       chef-zero (>= 14.0.11)
+      corefoundation (~> 0.3.4)
       diff-lcs (>= 1.2.4, < 1.4.0)
       erubis (~> 2.7)
       ffi (>= 1.5.0)
@@ -73,6 +74,7 @@ PATH
       chef-utils (= 17.7.8)
       chef-vault
       chef-zero (>= 14.0.11)
+      corefoundation (~> 0.3.4)
       diff-lcs (>= 1.2.4, < 1.4.0)
       erubis (~> 2.7)
       ffi (>= 1.5.0)
@@ -179,6 +181,8 @@ GEM
       net-ssh
     coderay (1.1.3)
     concurrent-ruby (1.1.9)
+    corefoundation (0.3.4)
+      ffi (>= 1.15.0)
     crack (0.4.5)
       rexml
     debug_inspector (1.1.0)
diff --git a/chef.gemspec b/chef.gemspec
index 199c3bd8fd..cd815b2535 100644
--- a/chef.gemspec
+++ b/chef.gemspec
@@ -52,6 +52,7 @@ Gem::Specification.new do |s|
   s.add_dependency "addressable"
   s.add_dependency "syslog-logger", "~> 1.6"
   s.add_dependency "uuidtools", ">= 2.1.5", "< 3.0" # osx_profile resource
+  s.add_dependency "corefoundation", "~> 0.3.4" # macos_userdefaults resource
 
   s.add_dependency "proxifier", "~> 1.0"
 
diff --git a/kitchen-tests/cookbooks/end_to_end/recipes/_macos_userdefaults.rb b/kitchen-tests/cookbooks/end_to_end/recipes/_macos_userdefaults.rb
index 80cbfddfcc..d38e7e16a2 100644
--- a/kitchen-tests/cookbooks/end_to_end/recipes/_macos_userdefaults.rb
+++ b/kitchen-tests/cookbooks/end_to_end/recipes/_macos_userdefaults.rb
@@ -52,6 +52,14 @@ macos_userdefaults "Bogus key with dict value" do
   value "User": "/Library/Managed Installs/way_fake.log"
 end
 
+# test that we can set an array with dict value
+macos_userdefaults "Bogus key with array value with dict items" do
+  domain "/Library/Preferences/ManagedInstalls"
+  type "array"
+  key "ArrayWithDict"
+  value [ { "User": "/Library/Managed Installs/way_fake.log" } ]
+end
+
 # test that we can set a bool
 macos_userdefaults "Bogus key with boolean value" do
   domain "/Library/Preferences/ManagedInstalls"
diff --git a/lib/chef/resource/macos_userdefaults.rb b/lib/chef/resource/macos_userdefaults.rb
index 036bd96d8b..ee84278392 100644
--- a/lib/chef/resource/macos_userdefaults.rb
+++ b/lib/chef/resource/macos_userdefaults.rb
@@ -78,172 +78,87 @@ class Chef
         required: true
 
       property :host, [String, Symbol],
-        description: "Set either :current or a hostname to set the user default at the host level.",
+        description: "Set either :current, :all or a hostname to set the user default at the host level.",
         desired_state: false,
-        introduced: "16.3"
+        introduced: "16.3",
+        coerce: proc { |value| to_cf_host(value) }
 
       property :value, [Integer, Float, String, TrueClass, FalseClass, Hash, Array],
         description: "The value of the key. Note: With the `type` property set to `bool`, `String` forms of Boolean true/false values that Apple accepts in the defaults command will be coerced: 0/1, 'TRUE'/'FALSE,' 'true'/false', 'YES'/'NO', or 'yes'/'no'.",
-        required: [:write],
-        coerce: proc { |v| v.is_a?(Hash) ? v.transform_keys(&:to_s) : v } # make sure keys are all strings for comparison
+        required: [:write]
 
       property :type, String,
         description: "The value type of the preference key.",
         equal_to: %w{bool string int float array dict},
-        desired_state: false
+        desired_state: false,
+        deprecated: true
 
-      property :user, String,
-        description: "The system user that the default will be applied to.",
-        desired_state: false
+      property :user, [String, Symbol],
+        description: "The system user that the default will be applied to. Set :current for current user, :all for all users or pass a valid username",
+        desired_state: false,
+        coerce: proc { |value| to_cf_user(value) }
 
       property :sudo, [TrueClass, FalseClass],
         description: "Set to true if the setting you wish to modify requires privileged access. This requires passwordless sudo for the `/usr/bin/defaults` command to be setup for the user running #{ChefUtils::Dist::Infra::PRODUCT}.",
         default: false,
-        desired_state: false
+        desired_state: false,
+        deprecated: true
 
       load_current_value do |new_resource|
-        Chef::Log.debug "#load_current_value: shelling out \"#{defaults_export_cmd(new_resource).join(" ")}\" to determine state"
-        state = shell_out(defaults_export_cmd(new_resource), user: new_resource.user)
-
-        if state.error? || state.stdout.empty?
-          Chef::Log.debug "#load_current_value: #{defaults_export_cmd(new_resource).join(" ")} returned stdout: #{state.stdout} and stderr: #{state.stderr}"
-          current_value_does_not_exist!
-        end
-
-        plist_data = ::Plist.parse_xml(state.stdout)
-
-        # handle the situation where the key doesn't exist in the domain
-        if plist_data.key?(new_resource.key)
-          key new_resource.key
-        else
-          current_value_does_not_exist!
-        end
+        Chef::Log.debug "#load_current_value: attempting to read \"#{new_resource.domain}\" value from preferences to determine state"
 
-        value plist_data[new_resource.key]
-      end
-
-      #
-      # The defaults command to export a domain
-      #
-      # @return [Array] defaults command
-      #
-      def defaults_export_cmd(resource)
-        state_cmd = ["/usr/bin/defaults"]
-
-        if resource.host == "current"
-          state_cmd.concat(["-currentHost"])
-        elsif resource.host # they specified a non-nil value, which is a hostname
-          state_cmd.concat(["-host", resource.host])
-        end
+        pref = get_preference(new_resource)
+        current_value_does_not_exist! if pref.nil?
 
-        state_cmd.concat(["export", resource.domain, "-"])
-        state_cmd
+        key new_resource.key
+        value pref
       end
 
       action :write, description: "Write the value to the specified domain/key." do
         converge_if_changed do
-          cmd = defaults_modify_cmd
-          Chef::Log.debug("Updating defaults value by shelling out: #{cmd.join(" ")}")
-
-          shell_out!(cmd, user: new_resource.user)
+          Chef::Log.debug("Updating defaults value for #{new_resource.key} in #{new_resource.domain}")
+          CF::Preferences.set!(new_resource.key, new_resource.value, new_resource.domain, new_resource.user, new_resource.host)
         end
       end
 
       action :delete, description: "Delete a key from a domain." do
         # if it's not there there's nothing to remove
-        return unless current_resource
+        return if current_resource.nil?
 
         converge_by("delete domain:#{new_resource.domain} key:#{new_resource.key}") do
-
-          cmd = defaults_modify_cmd
-          Chef::Log.debug("Removing defaults key by shelling out: #{cmd.join(" ")}")
-
-          shell_out!(cmd, user: new_resource.user)
+          Chef::Log.debug("Removing defaults key: #{new_resource.key}")
+          CF::Preferences.set!(new_resource.key, nil, new_resource.domain, new_resource.user, new_resource.host)
         end
       end
 
-      action_class do
-        #
-        # The command used to write or delete delete values from domains
-        #
-        # @return [Array] Array representation of defaults command to run
-        #
-        def defaults_modify_cmd
-          cmd = ["/usr/bin/defaults"]
-
-          if new_resource.host == :current
-            cmd.concat(["-currentHost"])
-          elsif new_resource.host # they specified a non-nil value, which is a hostname
-            cmd.concat(["-host", new_resource.host])
-          end
+      def get_preference(new_resource)
+        CF::Preferences.get(new_resource.key, new_resource.domain, new_resource.user, new_resource.host)
+      end
 
-          cmd.concat([action.to_s, new_resource.domain, new_resource.key])
-          cmd.concat(processed_value) if action == :write
-          cmd.prepend("sudo") if new_resource.sudo
-          cmd
-        end
+      action_class do
+        require "corefoundation" if RUBY_PLATFORM.match?(/darwin/)
 
-        #
-        # convert the provided value into the format defaults expects
-        #
-        # @return [array] array of values starting with the type if applicable
-        #
-        def processed_value
-          type = new_resource.type || value_type(new_resource.value)
-
-          # when dict this creates an array of values ["Key1", "Value1", "Key2", "Value2" ...]
-          cmd_values = ["-#{type}"]
-
-          case type
-          when "dict"
-            cmd_values.concat(new_resource.value.flatten)
-          when "array"
-            cmd_values.concat(new_resource.value)
-          when "bool"
-            cmd_values.concat(bool_to_defaults_bool(new_resource.value))
+        # Return valid hostname based on the input from host property
+        def to_cf_host(value)
+          case value
+          when :all
+            CF::Preferences::ALL_HOSTS
+          when :current
+            CF::Preferences::CURRENT_HOST
           else
-            cmd_values.concat([new_resource.value])
+            value
           end
-
-          cmd_values
         end
 
-        #
-        # defaults booleans on the CLI must be 'TRUE' or 'FALSE' so convert various inputs to that
-        #
-        # @param [String, Integer, Boolean] input <description>
-        #
-        # @return [String] TRUE or FALSE
-        #
-        def bool_to_defaults_bool(input)
-          return ["TRUE"] if [true, "TRUE", "1", "true", "YES", "yes"].include?(input)
-          return ["FALSE"] if [false, "FALSE", "0", "false", "NO", "no"].include?(input)
-
-          # make sure it's very clear bad input was given
-          raise ArgumentError, "#{input} cannot be converted to a boolean value for use with Apple's defaults command. Acceptable values are: 'TRUE', 'YES', 'true, 'yes', '0', true, 'FALSE', 'false', 'NO', 'no', '1', or false."
-        end
-
-        #
-        # convert ruby type to defaults type
-        #
-        # @param [Integer, Float, String, TrueClass, FalseClass, Hash, Array] value The value being set
-        #
-        # @return [string, nil] the type value used by defaults or nil if not applicable
-        #
-        def value_type(value)
+        # Return valid username based on the input from user property
+        def to_cf_user(value)
           case value
-          when true, false
-            "bool"
-          when Integer
-            "int"
-          when Float
-            "float"
-          when Hash
-            "dict"
-          when Array
-            "array"
-          when String
-            "string"
+          when :all
+            CF::Preferences::ALL_USERS
+          when :current
+            CF::Preferences::CURRENT_USER
+          else
+            value
           end
         end
       end
diff --git a/spec/functional/resource/macos_userdefaults_spec.rb b/spec/functional/resource/macos_userdefaults_spec.rb
new file mode 100644
index 0000000000..a9ed206019
--- /dev/null
+++ b/spec/functional/resource/macos_userdefaults_spec.rb
@@ -0,0 +1,119 @@
+#
+# Copyright:: Copyright (c) Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+require "spec_helper"
+
+describe Chef::Resource::MacosUserDefaults do
+  def create_resource
+    node = Chef::Node.new
+    events = Chef::EventDispatch::Dispatcher.new
+    run_context = Chef::RunContext.new(node, {}, events)
+    resource = Chef::Resource::MacosUserDefaults.new("test", run_context)
+    resource
+  end
+
+  let(:resource) do
+    create_resource
+  end
+
+  context "has a default value" do
+    it ":macos_userdefaults for resource name" do
+      expect(resource.name).to eq("test")
+    end
+
+    it "NSGlobalDomain for the domain property" do
+      expect(resource.domain).to eq("NSGlobalDomain")
+    end
+
+    it "nil for the host property" do
+      expect(resource.host).to be_nil
+    end
+
+    it "nil for the user property" do
+      expect(resource.user).to be_nil
+    end
+
+    it ":write for resource action" do
+      expect(resource.action).to eq([:write])
+    end
+  end
+
+  it "supports :write, :delete actions" do
+    expect { resource.action :write }.not_to raise_error
+    expect { resource.action :delete }.not_to raise_error
+  end
+
+  context "can process expected data" do
+    it "set array values" do
+      resource.domain "/Library/Preferences/ManagedInstalls"
+      resource.key "TestArrayValues"
+      resource.value [ "/Library/Managed Installs/fake.log", "/Library/Managed Installs/also_fake.log"]
+      resource.run_action(:write)
+      expect(resource.get_preference resource).to eq([ "/Library/Managed Installs/fake.log", "/Library/Managed Installs/also_fake.log"])
+    end
+
+    it "set dictionary value" do
+      resource.domain "/Library/Preferences/ManagedInstalls"
+      resource.key "TestDictionaryValues"
+      resource.value "User": "/Library/Managed Installs/way_fake.log"
+      resource.run_action(:write)
+      expect(resource.get_preference resource).to eq("User" => "/Library/Managed Installs/way_fake.log")
+    end
+
+    it "set array of dictionaries" do
+      resource.domain "/Library/Preferences/ManagedInstalls"
+      resource.key "TestArrayWithDictionary"
+      resource.value [ { "User": "/Library/Managed Installs/way_fake.log" } ]
+      resource.run_action(:write)
+      expect(resource.get_preference resource).to eq([ { "User" => "/Library/Managed Installs/way_fake.log" } ])
+    end
+
+    it "set boolean for preference value" do
+      resource.domain "/Library/Preferences/ManagedInstalls"
+      resource.key "TestBooleanValue"
+      resource.value true
+      resource.run_action(:write)
+      expect(resource.get_preference resource).to eq(true)
+    end
+
+    it "sets value to global domain when domain is not passed" do
+      resource.key "TestKey"
+      resource.value 1
+      resource.run_action(:write)
+      expect(resource.get_preference resource).to eq(1)
+    end
+
+    it "short domain names" do
+      resource.domain "com.apple.dock"
+      resource.key "titlesize"
+      resource.value "20"
+      resource.run_action(:write)
+      expect(resource.get_preference resource).to eq("20")
+    end
+  end
+
+  it "we can delete a preference with full path" do
+    resource.domain "/Library/Preferences/ManagedInstalls"
+    resource.key "TestKey"
+    expect { resource.run_action(:delete) }. to_not raise_error
+  end
+
+  it "we can delete a preference with short name" do
+    resource.domain "com.apple.dock"
+    resource.key "titlesize"
+    expect { resource.run_action(:delete) }. to_not raise_error
+  end
+end
diff --git a/spec/unit/resource/macos_user_defaults_spec.rb b/spec/unit/resource/macos_user_defaults_spec.rb
index 2c643ab266..19ff31c430 100644
--- a/spec/unit/resource/macos_user_defaults_spec.rb
+++ b/spec/unit/resource/macos_user_defaults_spec.rb
@@ -18,119 +18,59 @@
 require "spec_helper"
 
 describe Chef::Resource::MacosUserDefaults do
-
-  let(:resource) { Chef::Resource::MacosUserDefaults.new("foo") }
-  let(:provider) { resource.provider_for_action(:write) }
-
-  it "has a resource name of :macos_userdefaults" do
-    expect(resource.resource_name).to eq(:macos_userdefaults)
-  end
-
-  it "the domain property defaults to NSGlobalDomain" do
-    expect(resource.domain).to eq("NSGlobalDomain")
-  end
-
-  it "the value property coerces keys in hashes to strings so we can compare them with plist data" do
-    resource.value "User": "/Library/Managed Installs/way_fake.log"
-    expect(resource.value).to eq({ "User" => "/Library/Managed Installs/way_fake.log" })
-  end
-
-  it "the host property defaults to nil" do
-    expect(resource.host).to be_nil
-  end
-
-  it "the sudo property defaults to false" do
-    expect(resource.sudo).to be false
-  end
-
-  it "sets the default action as :write" do
-    expect(resource.action).to eq([:write])
-  end
-
-  it "supports :write action" do
-    expect { resource.action :write }.not_to raise_error
-  end
-
-  describe "#defaults_export_cmd" do
-    it "exports NSGlobalDomain if no domain is set" do
-      expect(provider.defaults_export_cmd(resource)).to eq(["/usr/bin/defaults", "export", "NSGlobalDomain", "-"])
+  let(:test_value) { "fakest_key_value" }
+  let(:test_key) { "fakest_key" }
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:resource) {
+    Chef::Resource::MacosUserDefaults.new("foo", run_context).tap do |r|
+      r.value test_value
+      r.key test_key
     end
+  }
 
-    it "exports a provided domain" do
-      resource.domain "com.tim"
-      expect(provider.defaults_export_cmd(resource)).to eq(["/usr/bin/defaults", "export", "com.tim", "-"])
+  context "has a default value" do
+    it ":macos_userdefaults for resource name" do
+      expect(resource.resource_name).to eq(:macos_userdefaults)
     end
 
-    it "sets -currentHost if host is 'current'" do
-      resource.host "current"
-      expect(provider.defaults_export_cmd(resource)).to eq(["/usr/bin/defaults", "-currentHost", "export", "NSGlobalDomain", "-"])
+    it "NSGlobalDomain for the domain property" do
+      expect(resource.domain).to eq("NSGlobalDomain")
     end
 
-    it "sets -host 'tim-laptop if host is 'tim-laptop'" do
-      resource.host "tim-laptop"
-      expect(provider.defaults_export_cmd(resource)).to eq(["/usr/bin/defaults", "-host", "tim-laptop", "export", "NSGlobalDomain", "-"])
-    end
-  end
-
-  describe "#defaults_modify_cmd" do
-    # avoid needing to set these required values over and over. We'll overwrite them where necessary
-    before do
-      resource.key = "foo"
-      resource.value = "bar"
-    end
-
-    it "writes to NSGlobalDomain if domain isn't specified" do
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "NSGlobalDomain", "foo", "-string", "bar"])
-    end
-
-    it "uses the domain property if set" do
-      resource.domain = "MyCustomDomain"
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "MyCustomDomain", "foo", "-string", "bar"])
+    it "nil for the host property" do
+      expect(resource.host).to be_nil
     end
 
-    it "sets host specific values using host property" do
-      resource.host = "tims_laptop"
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "-host", "tims_laptop", "write", "NSGlobalDomain", "foo", "-string", "bar"])
+    it "nil for the user property" do
+      expect(resource.user).to be_nil
     end
 
-    it "if host is set to :current it passes CurrentHost" do
-      resource.host = :current
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "-currentHost", "write", "NSGlobalDomain", "foo", "-string", "bar"])
-    end
-
-    it "raises ArgumentError if bool is specified, but the value can't be made into a bool" do
-      resource.type "bool"
-      expect { provider.defaults_modify_cmd }.to raise_error(ArgumentError)
-    end
-
-    it "autodetects array type and passes individual values" do
-      resource.value = %w{one two three}
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "NSGlobalDomain", "foo", "-array", "one", "two", "three"])
-    end
-
-    it "autodetects string type and passes a single value" do
-      resource.value = "one"
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "NSGlobalDomain", "foo", "-string", "one"])
+    it ":write for resource action" do
+      expect(resource.action).to eq([:write])
     end
+  end
 
-    it "autodetects integer type and passes a single value" do
-      resource.value = 1
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "NSGlobalDomain", "foo", "-int", 1])
+  context ":write" do
+    it "is a supported action" do
+      expect { resource.action :write }.not_to raise_error
     end
 
-    it "autodetects boolean type from TrueClass value and passes a 'TRUE' string" do
-      resource.value = true
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "NSGlobalDomain", "foo", "-bool", "TRUE"])
+    it "successfully updates the preference" do
+      resource.run_action(:write)
+      expect(resource.get_preference resource).eql? test_value
     end
+  end
 
-    it "autodetects boolean type from FalseClass value and passes a 'FALSE' string" do
-      resource.value = false
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "NSGlobalDomain", "foo", "-bool", "FALSE"])
+  context ":delete" do
+    it "is a supported action" do
+      expect { resource.action :delete }.not_to raise_error
     end
 
-    it "autodetects dict type from Hash value and flattens keys & values" do
-      resource.value = { "foo" => "bar" }
-      expect(provider.defaults_modify_cmd).to eq(["/usr/bin/defaults", "write", "NSGlobalDomain", "foo", "-dict", "foo", "bar"])
+    it "successfully deletes the preference" do
+      resource.run_action(:delete)
+      expect(resource.get_preference resource).to be_nil
     end
   end
 end