Improve the passwordless user error, logic, and testsimproved_task_logging

Specify exactly what we're checking for in the error. This needs to go into the docs site as well.

Signed-off-by: Tim Smith <tsmith@chef.io>

3 files changed, 25 insertions, 21 deletions

diff --git a/lib/chef/resource/windows_task.rb b/lib/chef/resource/windows_task.rb
index d8e6818c0b..37a818c7fe 100644
--- a/lib/chef/resource/windows_task.rb
+++ b/lib/chef/resource/windows_task.rb
@@ -119,15 +119,14 @@ class Chef
       SYSTEM_USERS = ['NT AUTHORITY\SYSTEM', "SYSTEM", 'NT AUTHORITY\LOCALSERVICE', 'NT AUTHORITY\NETWORKSERVICE', 'BUILTIN\USERS', "USERS"].freeze
 
       def validate_user_and_password(user, password)
-        if user && use_password?(user)
-          if password.nil?
-            raise ArgumentError, "Cannot specify a user other than the 'SYSTEM' user without specifying a password!"
-          end
+        if password_required?(user) && password.nil?
+          raise 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
       end
 
-      def use_password?(user)
-        @use_password ||= !SYSTEM_USERS.include?(user.upcase)
+      def password_required?(user)
+        return false if user.nil?
+        @password_required ||= !SYSTEM_USERS.include?(user.upcase)
       end
 
       def validate_interactive_setting(interactive_enabled, password)
@@ -198,7 +197,7 @@ class Chef
         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."
+          raise "idle_time value #{idle_time} is invalid. Valid values for :on_idle frequency are 1 - 999."
         end
       end
 
diff --git a/spec/functional/resource/windows_task_spec.rb b/spec/functional/resource/windows_task_spec.rb
index b59f0670b6..b6e6540693 100644
--- a/spec/functional/resource/windows_task_spec.rb
+++ b/spec/functional/resource/windows_task_spec.rb
@@ -339,7 +339,7 @@ describe Chef::Resource::WindowsTask, :windows_only do
       it "raises error" do
         subject.user "Administrator"
         subject.frequency :onstart
-        expect { subject.after_created }.to raise_error("Cannot specify a user other than the 'SYSTEM' user without specifying a password!")
+        expect { subject.after_created }.to raise_error(%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
     end
 
@@ -363,7 +363,7 @@ describe Chef::Resource::WindowsTask, :windows_only do
       it "raises error" do
         subject.months "xyz"
         subject.frequency :monthly
-        expect { subject.after_created }.to raise_error("months attribute 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 { subject.after_created }.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.")
       end
     end
 
@@ -379,7 +379,7 @@ describe Chef::Resource::WindowsTask, :windows_only do
       it "raises error" do
         subject.idle_time 300
         subject.frequency :monthly
-        expect { subject.after_created }.to raise_error("idle_time attribute is only valid for tasks that run on_idle")
+        expect { subject.after_created }.to raise_error("idle_time property is only valid for tasks that run on_idle")
       end
     end
   end
diff --git a/spec/unit/resource/windows_task_spec.rb b/spec/unit/resource/windows_task_spec.rb
index dc5c362388..0498292888 100644
--- a/spec/unit/resource/windows_task_spec.rb
+++ b/spec/unit/resource/windows_task_spec.rb
@@ -62,6 +62,18 @@ describe Chef::Resource::WindowsTask do
     expect(resource.frequency).to eql(:hourly)
   end
 
+  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'})
+    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)
+    end
+  end
+
   context "when random_delay is passed" do
     it "raises error if frequency is `:once`" do
       resource.frequency :once
@@ -83,12 +95,14 @@ describe Chef::Resource::WindowsTask do
     end
   end
 
-  context "when execution_time_limit is passed" do
+  context "when execution_time_limit isn't specified" do
     it "sets the default value to PT72H" do
       resource.after_created
       expect(resource.execution_time_limit).to eq("PT72H")
     end
+  end
 
+  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'.")
@@ -120,15 +134,6 @@ describe Chef::Resource::WindowsTask do
     end
   end
 
-  context "#validate_user_and_password" do
-    context "when password is not passed" do
-      it "raises error with non-system users" do
-        allow(resource).to receive(:use_password?).and_return(true)
-        expect { resource.send(:validate_user_and_password, "Testuser", nil) }.to raise_error("Cannot specify a user other than the 'SYSTEM' user without specifying a password!")
-      end
-    end
-  end
-
   context "#validate_interactive_setting" do
     it "raises error when interactive_enabled is passed without password" do
       expect { resource.send(:validate_interactive_setting, true, nil) }.to raise_error("Please provide the password when attempting to set interactive/non-interactive.")
@@ -213,7 +218,7 @@ describe Chef::Resource::WindowsTask do
     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("idle_time value 1000 is invalid. Valid values for :on_idle frequency are 1 - 999.")
     end
   end
 end