Merge pull request #6628 from chef/more_task

windows_task: Add additional input validation to properties

3 files changed, 136 insertions, 53 deletions

diff --git a/lib/chef/resource/windows_task.rb b/lib/chef/resource/windows_task.rb
index 37a818c7fe..c9e8448938 100644
--- a/lib/chef/resource/windows_task.rb
+++ b/lib/chef/resource/windows_task.rb
@@ -44,22 +44,22 @@ class Chef
       property :interactive_enabled, [TrueClass, FalseClass], default: false
       property :frequency_modifier, [Integer, String], default: 1
       property :frequency, equal_to: [:minute,
-                                       :hourly,
-                                       :daily,
-                                       :weekly,
-                                       :monthly,
-                                       :once,
-                                       :on_logon,
-                                       :onstart,
-                                       :on_idle,
-                                       :none], default: :hourly
+                                      :hourly,
+                                      :daily,
+                                      :weekly,
+                                      :monthly,
+                                      :once,
+                                      :on_logon,
+                                      :onstart,
+                                      :on_idle,
+                                      :none], default: :hourly
       property :start_day, String
       property :start_time, String
       property :day, [String, Integer]
       property :months, String
       property :idle_time, Integer
-      property :random_delay, String
-      property :execution_time_limit, String
+      property :random_delay, [String, Integer]
+      property :execution_time_limit, [String, Integer], default: "PT72H" # 72 hours in ISO08601 duration format
 
       attr_accessor :exists, :status, :enabled
 
@@ -71,15 +71,11 @@ class Chef
         end
 
         if execution_time_limit
-          raise ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds as a String e.g. '60'." if execution_time_limit.to_i == 0
-          duration = sec_to_dur(execution_time_limit)
-          execution_time_limit(duration)
-        else
-          # schtask sets execution_time_limit as PT72H by default
-          # We are setting the default value here so that we can do idempotency check later
-          # Note: We can't use `default` in the property
-          # because it will raise error for Invalid values passed as "PT72H" is not in seconds
-          execution_time_limit("PT72H")
+          unless execution_time_limit == "PT72H" # don't double convert an iso08601 format duration
+            raise ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds as an Integer (e.g. 60) or a String with numeric values only (e.g. '60')." unless numeric_value_in_string?(execution_time_limit)
+            duration = sec_to_dur(execution_time_limit)
+            execution_time_limit(duration)
+          end
         end
 
         validate_start_time(start_time, frequency)
@@ -94,25 +90,36 @@ class Chef
 
       private
 
+      # Validate the passed value is numeric values only if it is a string
+      def numeric_value_in_string?(val)
+        return true if Integer(val)
+      rescue ArgumentError
+        false
+      end
+
       def validate_random_delay(random_delay, frequency)
         if [:once, :on_logon, :onstart, :on_idle, :none].include? frequency
           raise ArgumentError, "`random_delay` property is supported only for frequency :minute, :hourly, :daily, :weekly and :monthly"
         end
 
-        raise ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds as a String e.g. '60'." if random_delay.to_i == 0
+        raise ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds as an Integer (e.g. 60) or a String with numeric values only (e.g. '60')." unless numeric_value_in_string?(random_delay)
       end
 
       def validate_start_day(start_day, frequency)
         if [:once, :on_logon, :onstart, :on_idle, :none].include? frequency
           raise ArgumentError, "`start_day` property is not supported with frequency: #{frequency}"
         end
+
+        # make sure the start_day is in MM/DD/YYYY format: http://rubular.com/r/cgjHemtWl5
+        raise ArgumentError, "`start_day` property must be in the MM/DD/YYYY format." unless /^(0[1-9]|1[012])[- \/.](0[1-9]|[12][0-9]|3[01])[- \/.](19|20)\d\d$/.match?(start_day)
       end
 
       def validate_start_time(start_time, frequency)
-        if frequency == :once
-          raise ArgumentError, "`start_time` needs to be provided with `frequency :once`" unless start_time
-        elsif frequency == :none
-          raise ArgumentError, "`start_time` property is not supported with `frequency :none`" if start_time
+        if start_time
+          raise ArgumentError, "`start_time` property is not supported with `frequency :none`" if frequency == :none
+          raise ArgumentError, "`start_time` property must be in the HH:mm format (e.g. 6:20pm -> 18:20)." unless /^[0-2][0-9]:[0-5][0-9]$/.match?(start_time)
+        else
+          raise ArgumentError, "`start_time` needs to be provided with `frequency :once`" if frequency == :once
         end
       end
 
@@ -172,20 +179,18 @@ class Chef
           days = day.split(",")
           days.each do |d|
             unless ["mon", "tue", "wed", "thu", "fri", "sat", "sun", "*"].include?(d.strip.downcase)
-              raise "day property invalid. Only valid values are: MON, TUE, WED, THU, FRI, SAT, SUN and *. Multiple values must be separated by a comma."
+              raise ArgumentError, "day property invalid. Only valid values are: MON, TUE, WED, THU, FRI, SAT, SUN and *. Multiple values must be separated by a comma."
             end
           end
         end
       end
 
       def validate_create_months(months, frequency)
-        unless [:monthly].include?(frequency)
-          raise "months property is only valid for tasks that run monthly"
-        end
+        raise ArgumentError, "months property is only valid for tasks that run monthly" unless frequency == :monthly
         if months.is_a? String
           months.split(",").each do |month|
             unless ["JAN", "FEB", "MAR", "APR", "MAY", "JUN", "JUL", "AUG", "SEP", "OCT", "NOV", "DEC", "*"].include?(month.strip.upcase)
-              raise "months property invalid. Only valid values are: JAN, FEB, MAR, APR, MAY, JUN, JUL, AUG, SEP, OCT, NOV, DEC and *. Multiple values must be separated by a comma."
+              raise ArgumentError, "months property invalid. Only valid values are: JAN, FEB, MAR, APR, MAY, JUN, JUL, AUG, SEP, OCT, NOV, DEC and *. Multiple values must be separated by a comma."
             end
           end
         end
@@ -193,11 +198,11 @@ class Chef
 
       def validate_idle_time(idle_time, frequency)
         unless [:on_idle].include?(frequency)
-          raise "idle_time property is only valid for tasks that run on_idle"
+          raise ArgumentError, "idle_time property is only valid for tasks that run on_idle"
         end
 
-        unless idle_time.to_i > 0 && idle_time.to_i <= 999
-          raise "idle_time value #{idle_time} is invalid. Valid values for :on_idle frequency are 1 - 999."
+        unless idle_time > 0 && idle_time <= 999
+          raise ArgumentError, "idle_time value #{idle_time} is invalid. Valid values for :on_idle frequency are 1 - 999."
         end
       end
 
@@ -206,7 +211,6 @@ class Chef
       # @param [Integer] seconds The amount of seconds for this duration
       def sec_to_dur(seconds)
         seconds = seconds.to_i
-        return if seconds == 0
         iso_str = "P"
         if seconds > 604_800 # more than a week
           weeks = seconds / 604_800
@@ -218,7 +222,7 @@ class Chef
           seconds -= (86_400 * days)
           iso_str << "#{days}D"
         end
-        if seconds > 0
+        if seconds >= 0
           iso_str << "T"
           if seconds > 3600 # more than an hour
             hours = seconds / 3600
diff --git a/spec/functional/resource/windows_task_spec.rb b/spec/functional/resource/windows_task_spec.rb
index b6e6540693..b9aa6c6568 100644
--- a/spec/functional/resource/windows_task_spec.rb
+++ b/spec/functional/resource/windows_task_spec.rb
@@ -308,7 +308,7 @@ describe Chef::Resource::WindowsTask, :windows_only do
       it "raises error if invalid random_delay is passed" do
         subject.frequency :minute
         subject.random_delay "abc"
-        expect { subject.after_created }.to raise_error("Invalid value passed for `random_delay`. Please pass seconds as a String e.g. '60'.")
+        expect { subject.after_created }.to raise_error("Invalid value passed for `random_delay`. Please pass seconds as an Integer (e.g. 60) or a String with numeric values only (e.g. '60').")
       end
 
       it "raises error if random_delay is passed with frequency on_idle" do
diff --git a/spec/unit/resource/windows_task_spec.rb b/spec/unit/resource/windows_task_spec.rb
index 0498292888..131df524fc 100644
--- a/spec/unit/resource/windows_task_spec.rb
+++ b/spec/unit/resource/windows_task_spec.rb
@@ -65,12 +65,17 @@ describe Chef::Resource::WindowsTask do
   context "when user is set but password is not" do
     it "raises an error if the user is a non-system user" do
       resource.user "bob"
-      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, %q{Cannot specify a user other than the system users without specifying a password!. Valid passwordless users: 'NT AUTHORITY\SYSTEM', 'SYSTEM', 'NT AUTHORITY\LOCALSERVICE', 'NT AUTHORITY\NETWORKSERVICE', 'BUILTIN\USERS', 'USERS'})
+      expect { resource.after_created }.to raise_error(ArgumentError, %q{Cannot specify a user other than the system users without specifying a password!. Valid passwordless users: 'NT AUTHORITY\SYSTEM', 'SYSTEM', 'NT AUTHORITY\LOCALSERVICE', 'NT AUTHORITY\NETWORKSERVICE', 'BUILTIN\USERS', 'USERS'})
     end
 
     it "does not raise an error if the user is a system user" do
       resource.user 'NT AUTHORITY\SYSTEM'
-      expect { resource.after_created }.to_not raise_error(Chef::Exceptions::ArgumentError)
+      expect { resource.after_created }.to_not raise_error
+    end
+
+    it "does not raise an error if the user is a system user even if lowercase" do
+      resource.user 'nt authority\system'
+      expect { resource.after_created }.to_not raise_error
     end
   end
 
@@ -78,21 +83,34 @@ describe Chef::Resource::WindowsTask do
     it "raises error if frequency is `:once`" do
       resource.frequency :once
       resource.random_delay "20"
-      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "`random_delay` property is supported only for frequency :minute, :hourly, :daily, :weekly and :monthly")
+      expect { resource.after_created }.to raise_error(ArgumentError, "`random_delay` property is supported only for frequency :minute, :hourly, :daily, :weekly and :monthly")
     end
 
     it "raises error for invalid random_delay" do
       resource.frequency :monthly
       resource.random_delay "xyz"
-      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds as a String e.g. '60'.")
+      expect { resource.after_created }.to raise_error(ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds as an Integer (e.g. 60) or a String with numeric values only (e.g. '60').")
+    end
+
+    it "raises error for invalid random_delay which looks like an Integer" do
+      resource.frequency :monthly
+      resource.random_delay "5,000"
+      expect { resource.after_created }.to raise_error(ArgumentError, "Invalid value passed for `random_delay`. Please pass seconds as an Integer (e.g. 60) or a String with numeric values only (e.g. '60').")
     end
 
-    it "converts seconds into iso8601 duration format" do
+    it "converts seconds String into iso8601 duration format" do
       resource.frequency :monthly
       resource.random_delay "60"
       resource.after_created
       expect(resource.random_delay).to eq("PT60S")
     end
+
+    it "converts seconds Integer into iso8601 duration format" do
+      resource.frequency :monthly
+      resource.random_delay 60
+      resource.after_created
+      expect(resource.random_delay).to eq("PT60S")
+    end
   end
 
   context "when execution_time_limit isn't specified" do
@@ -105,10 +123,21 @@ describe Chef::Resource::WindowsTask do
   context "when execution_time_limit is passed" do
     it "raises error for invalid execution_time_limit" do
       resource.execution_time_limit "abc"
-      expect { resource.after_created }.to raise_error(Chef::Exceptions::ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds as a String e.g. '60'.")
+      expect { resource.after_created }.to raise_error(ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds as an Integer (e.g. 60) or a String with numeric values only (e.g. '60').")
     end
 
-    it "converts seconds into iso8601 format" do
+    it "raises error for invalid execution_time_limit that looks like an Integer" do
+      resource.execution_time_limit "5,000"
+      expect { resource.after_created }.to raise_error(ArgumentError, "Invalid value passed for `execution_time_limit`. Please pass seconds as an Integer (e.g. 60) or a String with numeric values only (e.g. '60').")
+    end
+
+    it "converts seconds Integer into iso8601 format" do
+      resource.execution_time_limit 60
+      resource.after_created
+      expect(resource.execution_time_limit).to eq("PT60S")
+    end
+
+    it "converts seconds String into iso8601 format" do
       resource.execution_time_limit "60"
       resource.after_created
       expect(resource.execution_time_limit).to eq("PT60S")
@@ -118,19 +147,48 @@ describe Chef::Resource::WindowsTask do
   context "#validate_start_time" do
     it "raises error if start_time is nil when frequency `:once`" do
       resource.frequency :once
-      expect { resource.send(:validate_start_time, nil, :once) }.to raise_error(Chef::Exceptions::ArgumentError, "`start_time` needs to be provided with `frequency :once`")
+      expect { resource.send(:validate_start_time, nil, :once) }.to raise_error(ArgumentError, "`start_time` needs to be provided with `frequency :once`")
     end
 
     it "raises error if start_time is given when frequency `:none`" do
       resource.frequency :none
-      expect { resource.send(:validate_start_time, "12.00", :none) }.to raise_error(Chef::Exceptions::ArgumentError, "`start_time` property is not supported with `frequency :none`")
+      expect { resource.send(:validate_start_time, "12.00", :none) }.to raise_error(ArgumentError, "`start_time` property is not supported with `frequency :none`")
+    end
+
+    it "raises error if start_time is not HH:mm format" do
+      resource.frequency :once
+      expect { resource.send(:validate_start_time, "2:30", :once) }.to raise_error(ArgumentError, "`start_time` property must be in the HH:mm format (e.g. 6:20pm -> 18:20).")
+    end
+
+    it "does not raise error if start_time is in HH:mm format" do
+      resource.frequency :once
+      expect { resource.send(:validate_start_time, "12:30", :once) }.not_to raise_error
     end
   end
 
   context "#validate_start_day" do
-    it "raise error if start_day is passed with frequency :on_logon" do
-      resource.frequency :on_logon
-      expect { resource.send(:validate_start_day, "Wed", :on_logon) }.to raise_error(Chef::Exceptions::ArgumentError, "`start_day` property is not supported with frequency: on_logon")
+    it "raise error if start_day is passed with invalid frequency (:on_logon)" do
+      expect { resource.send(:validate_start_day, "02/07/1984", :on_logon) }.to raise_error(ArgumentError, "`start_day` property is not supported with frequency: on_logon")
+    end
+
+    it "does not raise error if start_day is passed with valid frequency (:weekly)" do
+      expect { resource.send(:validate_start_day, "02/07/1984", :weekly) }.not_to raise_error
+    end
+
+    it "raise error if start_day is passed with invalid date format (DD/MM/YYYY)" do
+      expect { resource.send(:validate_start_day, "28/12/2009", :weekly) }.to raise_error(ArgumentError, "`start_day` property must be in the MM/DD/YYYY format.")
+    end
+
+    it "raise error if start_day is passed with invalid date format (M/DD/YYYY)" do
+      expect { resource.send(:validate_start_day, "2/07/1984", :weekly) }.to raise_error(ArgumentError, "`start_day` property must be in the MM/DD/YYYY format.")
+    end
+
+    it "raise error if start_day is passed with invalid date format (MM/D/YYYY)" do
+      expect { resource.send(:validate_start_day, "02/7/1984", :weekly) }.to raise_error(ArgumentError, "`start_day` property must be in the MM/DD/YYYY format.")
+    end
+
+    it "raise error if start_day is passed with invalid date format (MM/DD/YY)" do
+      expect { resource.send(:validate_start_day, "02/07/84", :weekly) }.to raise_error(ArgumentError, "`start_day` property must be in the MM/DD/YYYY format.")
     end
   end
 
@@ -190,13 +248,13 @@ describe Chef::Resource::WindowsTask do
     end
 
     it "raises error for invalid day value" do
-      expect  { resource.send(:validate_create_day, "xyz", :weekly) }.to raise_error("day property invalid. Only valid values are: MON, TUE, WED, THU, FRI, SAT, SUN and *. Multiple values must be separated by a comma.")
+      expect  { resource.send(:validate_create_day, "xyz", :weekly) }.to raise_error(ArgumentError, "day property invalid. Only valid values are: MON, TUE, WED, THU, FRI, SAT, SUN and *. Multiple values must be separated by a comma.")
     end
   end
 
   context "#validate_create_months" do
     it "raises error if frequency is not :monthly" do
-      expect  { resource.send(:validate_create_months, "Jan", :once) }.to raise_error("months property is only valid for tasks that run monthly")
+      expect  { resource.send(:validate_create_months, "Jan", :once) }.to raise_error(ArgumentError, "months property is only valid for tasks that run monthly")
     end
 
     it "accepts a valid single month" do
@@ -208,17 +266,38 @@ describe Chef::Resource::WindowsTask do
     end
 
     it "raises error for invalid month value" do
-      expect  { resource.send(:validate_create_months, "xyz", :monthly) }.to raise_error("months property invalid. Only valid values are: JAN, FEB, MAR, APR, MAY, JUN, JUL, AUG, SEP, OCT, NOV, DEC and *. Multiple values must be separated by a comma.")
+      expect  { resource.send(:validate_create_months, "xyz", :monthly) }.to raise_error(ArgumentError, "months property invalid. Only valid values are: JAN, FEB, MAR, APR, MAY, JUN, JUL, AUG, SEP, OCT, NOV, DEC and *. Multiple values must be separated by a comma.")
     end
   end
 
   context "#validate_idle_time" do
     it "raises error if frequency is not :on_idle" do
-      expect  { resource.send(:validate_idle_time, 5, :hourly) }.to raise_error("idle_time property is only valid for tasks that run on_idle")
+      expect  { resource.send(:validate_idle_time, 5, :hourly) }.to raise_error(ArgumentError, "idle_time property is only valid for tasks that run on_idle")
     end
 
     it "raises error if idle_time > 999" do
-      expect  { resource.send(:validate_idle_time, 1000, :on_idle) }.to raise_error("idle_time value 1000 is invalid. Valid values for :on_idle frequency are 1 - 999.")
+      expect  { resource.send(:validate_idle_time, 1000, :on_idle) }.to raise_error(ArgumentError, "idle_time value 1000 is invalid. Valid values for :on_idle frequency are 1 - 999.")
+    end
+  end
+
+  context "#sec_to_dur" do
+    it "return PT0S when passed 0" do
+      expect(resource.send(:sec_to_dur, 0)).to eql("PT0S")
+    end
+    it "return PT1S when passed 1" do
+      expect(resource.send(:sec_to_dur, 1)).to eql("PT1S")
+    end
+    it "return PT24H0S when passed 86400" do
+      expect(resource.send(:sec_to_dur, 86400)).to eql("PT24H0S")
+    end
+    it "return P1DT1S when passed 86401" do
+      expect(resource.send(:sec_to_dur, 86401)).to eql("P1DT1S")
+    end
+    it "return P1DT1M40S when passed 86500" do
+      expect(resource.send(:sec_to_dur, 86500)).to eql("P1DT1M40S")
+    end
+    it "return P1WT1S when passed 604801" do
+      expect(resource.send(:sec_to_dur, 604801)).to eql("P1WT1S")
     end
   end
 end