launchd: start_calendar_interval validity checking

StartCalendarInterval has some gotchas so we coerce it to sanity check.

According to `man 5 launchd.plist`:
  StartCalendarInterval [is a] <dictionary of integers or array of dictionaries of integers>
    ... Missing arguments are considered to be wildcard.

What the man page doesn't state, but what was observed (OSX 10.11.5, launchctrl v3.4.0) is that key that are specified, _but with invalid values, will also be silently treated as a wildcard values.

This means that an entry like:
  { "Hour"=>0, "Weekday"=>"6-7"}
will not just run on midnight of Sat and Sun, rather it will run _every_ midnight.

We check to make sure that the values are integers and not strings, adhering to the manpage documentation.

This also ensures that only keys that StartCalendarInterval supports will be passed.
This eliminates passing a key (like `:hour` or `'hour') and thinking it works when in reality launchd is just treating it as a wildcard.

NOTE: This is a "breaking" change in that: it will break existing cookbooks that are using the current docs: https://docs.chef.io/resource_launchd.html (See start_calendar_interval); however the generated plist doesn't appear to actually do what was intended anyway.

2 files changed, 123 insertions, 4 deletions

diff --git a/lib/chef/resource/launchd.rb b/lib/chef/resource/launchd.rb
index e584693dee..e4e8b4fb01 100644
--- a/lib/chef/resource/launchd.rb
+++ b/lib/chef/resource/launchd.rb
@@ -46,6 +46,48 @@ class Chef
       property :source, String
       property :session_type, String
 
+      # StartCalendarInterval has some gotchas so we coerce it to help sanity
+      # check.  According to `man 5 launchd.plist`:
+      #   StartCalendarInterval <dictionary of integers or array of dictionaries of integers>
+      #     ... Missing arguments are considered to be wildcard.
+      # What the man page doesn't state, but what was observed (OSX 10.11.5, launchctrl v3.4.0)
+      # Is that keys that are specified, but invalid, will also be treated as a wildcard
+      # this means that an entry like:
+      #   { "Hour"=>0, "Weekday"=>"6-7"}
+      # will not just run on midnight of Sat and Sun, rather it will run _every_ midnight.
+      property :start_calendar_interval, [Hash, Array], coerce: proc { |type|
+        # Coerce into an array of hashes to make validation easier
+        array = type   if type.is_a?(Array)
+        array = [type] unless array
+
+        # Check to make sure that our array only has hashes
+        if array.any? { |obj| !obj.is_a?(Hash) }
+          error_msg = "start_calendar_interval must be a single hash or an array of hashes!"
+          raise Chef::Exceptions::ValidationFailed, error_msg
+        end
+
+        # Make sure the hashes don't have any incorrect keys/values
+        array.each do |entry|
+          allowed_keys = %w(Minute Hour Day Weekday Month)
+          error_msg = "Invalid key for start_calendar_interval, must be one of: #{allowed_keys.join(", ")}"
+          if entry.keys.any? { |key| !allowed_keys.include?(key) }
+            raise Chef::Exceptions::ValidationFailed, error_msg
+          end
+
+          error_msg = "Invalid value for start_calendar_interval item.  Values must be integers!"
+          if entry.values.any? { |val| !val.is_a?(Fixnum) }
+            raise Chef::Exceptions::ValidationFailed, error_msg
+          end
+        end
+
+        # Don't return array if we only have one entry
+        if array.size == 1
+          array.first
+        else
+          array
+        end
+      }
+
       property :type, String, default: "daemon", coerce: proc { |type|
         type = type ? type.downcase : "daemon"
         types = %w{daemon agent}
@@ -89,7 +131,6 @@ class Chef
       property :standard_error_path, String
       property :standard_in_path, String
       property :standard_out_path, String
-      property :start_calendar_interval, Hash
       property :start_interval, Integer
       property :start_on_mount, [ TrueClass, FalseClass ]
       property :throttle_interval, Integer
diff --git a/spec/unit/provider/launchd_spec.rb b/spec/unit/provider/launchd_spec.rb
index 2893044833..3289e6a365 100644
--- a/spec/unit/provider/launchd_spec.rb
+++ b/spec/unit/provider/launchd_spec.rb
@@ -40,7 +40,7 @@ describe Chef::Provider::Launchd do
 \t<string>/Library/scripts/call_mom.sh</string>
 \t<key>StartCalendarInterval</key>
 \t<dict>
-\t\t<key>Hourly</key>
+\t\t<key>Hour</key>
 \t\t<integer>10</integer>
 \t\t<key>Weekday</key>
 \t\t<integer>7</integer>
@@ -50,13 +50,42 @@ describe Chef::Provider::Launchd do
 </dict>
 </plist>
 XML
+    let(:test_plist_multiple_intervals) { String.new <<-XML }
+<?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>
+\t<key>Label</key>
+\t<string>call.mom.weekly</string>
+\t<key>Program</key>
+\t<string>/Library/scripts/call_mom.sh</string>
+\t<key>StartCalendarInterval</key>
+\t<array>
+\t\t<dict>
+\t\t\t<key>Hour</key>
+\t\t\t<integer>11</integer>
+\t\t\t<key>Weekday</key>
+\t\t\t<integer>1</integer>
+\t\t</dict>
+\t\t<dict>
+\t\t\t<key>Hour</key>
+\t\t\t<integer>12</integer>
+\t\t\t<key>Weekday</key>
+\t\t\t<integer>2</integer>
+\t\t</dict>
+\t</array>
+\t<key>TimeOut</key>
+\t<integer>300</integer>
+</dict>
+</plist>
+XML
 
     let(:test_hash) do
       {
       "Label" => "call.mom.weekly",
       "Program" => "/Library/scripts/call_mom.sh",
       "StartCalendarInterval" => {
-        "Hourly" => 10,
+        "Hour" => 10,
         "Weekday" => 7,
       },
       "TimeOut" => 300,
@@ -100,12 +129,61 @@ XML
         it "should produce the test_plist from properties" do
           new_resource.program "/Library/scripts/call_mom.sh"
           new_resource.time_out 300
-          new_resource.start_calendar_interval "Hourly" => 10, "Weekday" => 7
+          new_resource.start_calendar_interval "Hour" => 10, "Weekday" => 7
           expect(provider.content?).to be_truthy
           expect(provider.content).to eql(test_plist)
         end
       end
 
+      describe "start_calendar_interval is passed" do
+        it "should allow array of Hashes" do
+          allowed = (1..2).collect do |num|
+            {
+              "Hour"    => 10+num,
+              "Weekday" => num,
+            }
+          end
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          new_resource.start_calendar_interval allowed
+          expect(provider.content?).to be_truthy
+          expect(provider.content).to eql(test_plist_multiple_intervals)
+        end
+
+        it "should allow all StartCalendarInterval keys" do
+          allowed = {
+            "Minute"  => 1,
+            "Hour"    => 1,
+            "Day"     => 1,
+            "Weekday" => 1,
+            "Month"   => 1,
+          }
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          new_resource.start_calendar_interval allowed
+          expect(provider.content?).to be_truthy
+          %w(Minute Hour Day Weekday Month).each do |key|
+            expect(provider.content).to include("<key>#{key}</key>")
+          end
+        end
+
+        it "should not allow invalid ShowCalendarInterval keys" do
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          expect {
+            new_resource.start_calendar_interval "Hourly" => 1
+          }.to raise_error(/Invalid key/)
+        end
+
+        it "should not allow non-integer values" do
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          expect {
+            new_resource.start_calendar_interval "Hour" => "1-2"
+          }.to raise_error(/Invalid value/)
+        end
+      end
+
       describe "hash is passed" do
         it "should produce the test_plist from the hash" do
           new_resource.hash test_hash