Release/1.6 (#49)

* first attempt to solve defaults/plist resource whitespacing

* create the plist if it is missing

* better handling of new plists and novel entries

* Improve testing for macos_user (#48)

* better "admin" test coverage for macos_user

* all default suite tests passing

* fix kitchen yaml to enforce idempotency

* address comments in plist whitespace pull request

* fix syntax error with shellescape

* fix heredoc/cookstyle issue

* adjust whitespace for heredoc

* bump to release version

test/cookbooks/macos_test/.kitchen.yml → .kitchen.yml

@@ -17,11 +17,11 @@ platforms:
   - name: apex/macos-10.12.6
   - name: apex/macos-10.11.6
 
-
 suites:
   - name: default
-    multiple_converge: 2
-    enforce_idempotency: true
+    provisioner:
+      multiple_converge: 3
+      enforce_idempotency: true
     run_list:
       - recipe[macos::keep_awake]
       - recipe[macos_test::new_users]
@@ -43,8 +43,9 @@ suites:
         - test/smoke/xcode
 
   - name: spotlight
-    multiple_converge: 2
-    enforce_idempotency: true
+    provisioner:
+      multiple_converge: 2
+      enforce_idempotency: true
     run_list:
       - recipe[macos_test::spotlight]
     verifier:

Berksfile

@@ -1,3 +1,7 @@
 source 'https://supermarket.chef.io'
 
 metadata
+
+group :integration do
+  cookbook 'macos_test', path: './test/cookbooks/macos_test'
+end

libraries/plist.rb

@@ -10,6 +10,8 @@ def convert_to_data_type_from_string(type, value)
         value.to_f
       when 'string'
         value
+      when nil
+        ''
       else
         raise "Unknown or unsupported data type: #{type.class}"
       end
@@ -47,6 +49,11 @@ def type_to_commandline_string(value)
       end
     end
 
+    def print_entry_value(entry, path)
+      cmd = shell_out(plistbuddy_command(:print, entry, path))
+      cmd.exitstatus == 0
+    end
+
     def hardware_uuid
       system_profiler_hardware_output = shell_out('system_profiler', 'SPHardwareDataType').stdout
       hardware_overview = Psych.load(system_profiler_hardware_output)['Hardware']['Hardware Overview']
@@ -62,13 +69,12 @@ def plistbuddy_command(subcommand, entry, path, value = nil)
             else
               value
             end
-      entry = "\"#{entry}\"" if entry.include?(' ')
-      entry_with_arg = [entry, arg].join(' ').strip
+      entry_with_arg = ["\"#{entry}\"", arg].join(' ').strip
       subcommand = "#{subcommand.capitalize} :#{entry_with_arg}"
-      [plistbuddy_executable, '-c', "\'#{subcommand}\'", path].join(' ')
+      [plistbuddy_executable, '-c', "\'#{subcommand}\'", "\"#{path}\""].join(' ')
     end
 
-    def setting_from_plist(path, entry)
+    def setting_from_plist(entry, path)
       defaults_read_type_output = shell_out(defaults_executable, 'read-type', path, entry).stdout
       defaults_read_output = shell_out(defaults_executable, 'read', path, entry).stdout
       { key_type: defaults_read_type_output.split.last, key_value: defaults_read_output.strip }

metadata.rb

@@ -5,7 +5,7 @@
 description 'Resources for configuring and provisioning macOS'
 long_description IO.read(File.join(File.dirname(__FILE__), 'README.md'))
 chef_version '~> 13.0' if respond_to?(:chef_version)
-version '1.5.0'
+version '1.6.0'
 
 source_url 'https://github.com/Microsoft/macos-cookbook'
 issues_url 'https://github.com/Microsoft/macos-cookbook/issues'

resources/defaults.rb

@@ -1,7 +1,5 @@
 resource_name :defaults
 
-BASE_COMMAND = '/usr/bin/defaults'.freeze
-
 property :domain, String, name_property: true
 property :option, String, default: 'write'
 property :read_only, [true, false], default: false
@@ -12,16 +10,9 @@
 action :run do
   new_resource.option = 'read' if new_resource.read_only
   new_resource.settings.each do |setting, value|
-    cases = { Array.to_s => "-array #{value}",
-              Integer.to_s => "-int #{value}",
-              TrueClass.to_s => '-bool TRUE',
-              FalseClass.to_s => '-bool FALSE',
-              Hash.to_s => "-dict #{value}",
-              String.to_s => "-string #{value}",
-              Float.to_s => "-float #{value}" }
-    value = cases[value.class.to_s]
-    execute BASE_COMMAND do
-      command "#{BASE_COMMAND} #{new_resource.option} #{new_resource.domain} #{setting} #{value}"
+    value = "-#{convert_to_string_from_data_type(value)}"
+    execute "#{setting} to #{value}" do
+      command "#{defaults_executable} #{new_resource.option} #{Shellwords.shellescape(new_resource.domain)} #{Shellwords.shellescape(setting)} #{value}"
       user new_resource.user
     end
   end

resources/macos_user.rb

@@ -56,11 +56,10 @@ def user_already_exists?
   end
 
   if property_is_set?(:autologin)
-    setup_assistant_keypair_values.each do |key, setting|
-      plist "set #{setting} to #{key}" do
-        path setup_assistant_plist
-        entry key
-        value setting
+    setup_assistant_keypair_values.each do |e, v|
+      plist setup_assistant_plist do
+        entry e
+        value v
       end
     end
 
@@ -79,17 +78,17 @@ def user_already_exists?
   end
 
   if property_is_set?(:groups)
-    if groups.is_a? String
-      group groups do
+    if new_resource.groups.is_a? String
+      group new_resource.groups do
         action :create
-        members username
+        members new_resource.username
         append true
       end
     else
-      groups.each do |g|
+      new_resource.groups.each do |g|
         group g do
           action :create
-          members username
+          members new_resource.username
           append true
         end
       end

resources/plist.rb

@@ -1,32 +1,51 @@
 resource_name :plist
 
-property :path, String, name_property: true
+property :path, String, name_property: true, desired_state: true
 property :entry, String, desired_state: true
 property :value, [TrueClass, FalseClass, String, Integer, Float], desired_state: true
+property :binary, [TrueClass, FalseClass], desired_state: true, default: true
 
 load_current_value do |desired|
-  setting = setting_from_plist(desired.path, desired.entry)
-  current_value_does_not_exist! if setting[:key_type].nil?
-  entry desired.entry unless setting[:key_type].nil?
+  current_value_does_not_exist! unless ::File.exist?(desired.path)
+  entry desired.entry if print_entry_value(desired.entry, desired.path)
+  setting = setting_from_plist(desired.entry, desired.path)
   value convert_to_data_type_from_string(setting[:key_type], setting[:key_value])
+  binary shell_out('/usr/bin/file', '--brief', '--mime-encoding', desired.path).stdout.chomp == 'binary'
 end
 
 action :set do
+  converge_if_changed :path do
+    converge_by "creating \"#{new_resource.path}\"" do
+      file new_resource.path do
+        content <<-EOF
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+</dict>
+</plist>
+EOF
+      end
+    end
+  end
+
   converge_if_changed :entry do
-    converge_by "add \"#{new_resource.entry}\" to #{new_resource.path.split('/').last}" do
+    converge_by "adding entry \"#{new_resource.entry}\" to #{new_resource.path.split('/').last}" do
       execute plistbuddy_command(:add, new_resource.entry, new_resource.path, new_resource.value)
     end
   end
 
   converge_if_changed :value do
-    converge_by "set \"#{new_resource.entry}\" to #{new_resource.value} at #{new_resource.path.split('/').last}" do
+    converge_by "#{new_resource.path.split('/').last}: set new_resource.entry to new_resource.value" do
       execute plistbuddy_command(:set, new_resource.entry, new_resource.path, new_resource.value)
     end
   end
 
-  unless binary?
+  converge_if_changed :binary do
     converge_by "convert \"#{new_resource.path.split('/').last}\" to binary" do
-      execute ['/usr/bin/plutil', '-convert', 'binary1', new_resource.path]
+      execute "convert #{new_resource.path} to binary format" do
+        command ['/usr/bin/plutil', '-convert', 'binary1', new_resource.path]
+      end
     end
   end
 end
@@ -36,4 +55,8 @@ def binary?
     file_type_output = shell_out('/usr/bin/file', '--brief', '--mime-encoding', new_resource.path).stdout
     file_type_output.chomp == 'binary'
   end
+
+  def convert_to_binary(_path)
+    execute ['/usr/bin/plutil', '-convert', 'binary1', new_resource.path]
+  end
 end

spec/unit/libraries/plist_spec.rb

@@ -30,6 +30,18 @@
       expect(plistbuddy_command(:print, 'Foo Bar Baz', 'path/to/file.plist')).to eq "/usr/libexec/PlistBuddy -c 'Print :\"Foo Bar Baz\"' path/to/file.plist"
     end
   end
+
+  context 'The value to be added contains spaces' do
+    it 'returns the value properly formatted with double quotes' do
+      expect(plistbuddy_command(:add, 'Foo Bar Baz', 'path/to/file.plist', true)).to eq "/usr/libexec/PlistBuddy -c 'Add :\"Foo Bar Baz\" bool' path/to/file.plist"
+    end
+  end
+
+  context 'The plist itself contains spaces' do
+    it 'returns the value properly formatted with double quotes' do
+      expect(plistbuddy_command(:print, 'Foo Bar Baz', 'Library/Preferences/com.parallels.Parallels Desktop.plist')).to eq "/usr/libexec/PlistBuddy -c 'Print :\"Foo Bar Baz\"' \"Library/Preferences/com.parallels.Parallels Desktop.plist\""
+    end
+  end
 end
 
 describe MacOS::PlistHelpers, '#convert_to_data_type_from_string' do

test/cookbooks/macos_test/recipes/new_users.rb

@@ -1,19 +1,19 @@
-macos_user 'create admin user randall and enable automatic login' do
+macos_user 'create admin user with autologin' do
   username 'randall'
   password 'correct-horse-battery-staple'
   autologin true
   admin true
   groups 'alpha'
 end
 
-macos_user 'create non-admin user johnny' do
+macos_user 'create non-admin user with groups' do
   username 'johnny'
   fullname 'Johnny Appleseed'
   password 'yang-yolked-cordon-karate'
   groups %w(alpha beta)
 end
 
-macos_user 'create non-admin user paul' do
+macos_user 'create non-admin without groups' do
   username 'paul'
-  password 'yang-yolked-cordon-karate'
+  password 'bacon-saffron-doormat-educe'
 end

test/cookbooks/macos_test/test/smoke/default/new_users_test.rb → test/smoke/default/new_users_test.rb

@@ -7,15 +7,24 @@
     its('gid') { should eq 20 }
     its('home') { should eq '/Users/randall' }
     its('groups') { should include 'alpha' }
+    its('groups') { should include  'staff' }
+    its('groups') { should include  'admin' }
   end
 
   describe user('johnny') do
     it { should exist }
     its('uid') { should eq 504 }
     its('gid') { should eq 20 }
     its('home') { should eq '/Users/johnny' }
+    its('groups') { should include 'staff' }
     its('groups') { should include 'alpha' }
     its('groups') { should include 'beta' }
+    its('groups') { should_not include  'admin' }
+  end
+
+  describe group('admin') do
+    it { should exist }
+    its('gid') { should eq 80 }
   end
 
   realname_cmd = 'dscl . read /Users/johnny RealName | grep -v RealName | cut -c 2-'
@@ -27,7 +36,20 @@
   describe user('paul') do
     it { should exist }
     its('uid') { should eq 505 }
-    its('gid') { should eq 20 }
+    its('groups') { should include 'staff' }
+    its('groups') { should_not include  'admin' }
     its('home') { should eq '/Users/paul' }
   end
+
+  describe command('su -l randall -c "id -G"') do
+    its('stdout.split') { should include '80' }
+  end
+
+  describe command('su -l johnny -c "id -G"') do
+    its('stdout.split') { should_not include '80' }
+  end
+
+  describe command('su -l paul -c "id -G"') do
+    its('stdout.split') { should_not include '80' }
+  end
 end