(PUP-10967) Debug logs clean up when resolving account SID on Windows

This commit restricts the `Could not retrieve raw SID bytes from` debug
log to when an actual SID is passed to `ConvertStringSidToSidW`. The
order in which the `Puppet::Util::Windows::SID.name_to_principal` method
checks and tries to convert the input (be it an account name, domain
qualified account name or SID) to a
`Puppet::Util::Windows::SID::Principal` object made it print error
messages to debug when it was actually succeeding in the end. Since
every Puppet Agent run needs information about at least the currently
logged on user, this was creating unnecessary noise in the debug logs.

Author: luchihoratiu

lib/puppet/util/windows/sid.rb

@@ -75,7 +75,7 @@ def name_to_principal(name, allow_unresolved = false)
           raw_sid_bytes = sid_ptr.read_array_of_uchar(get_length_sid(sid_ptr))
         end
       rescue => e
-        Puppet.debug("Could not retrieve raw SID bytes from '#{name}': #{e.message}")
+        Puppet.debug("Could not retrieve raw SID bytes from '#{name}': #{e.message}") if /^S-\d+-\d+/.match(name)
       end
 
       raw_sid_bytes ? Principal.lookup_account_sid(raw_sid_bytes) : Principal.lookup_account_name(name)

spec/unit/util/windows/sid_spec.rb

@@ -131,38 +131,73 @@
       expect(subject.name_to_principal(unknown_name)).to be_nil
     end
 
+    it "should print a debug message if the account does not exist" do
+      expect(Puppet).to receive(:debug).with(/No mapping between account names and security IDs was done/)
+      subject.name_to_principal(unknown_name)
+    end
+
     it "should return a Puppet::Util::Windows::SID::Principal instance for any valid sid" do
       expect(subject.name_to_principal(sid)).to be_an_instance_of(Puppet::Util::Windows::SID::Principal)
     end
 
+    it "should not print debug messages for valid sid" do
+      expect(Puppet).not_to receive(:debug).with(/Could not retrieve raw SID bytes from/)
+      expect(Puppet).not_to receive(:debug).with(/No mapping between account names and security IDs was done/)
+      subject.name_to_principal(sid)
+    end
+
+    it "should print debug messages for invalid sid" do
+      expect(Puppet).to receive(:debug).with(/Could not retrieve raw SID bytes from 'S-1-5-21-INVALID-SID'/)
+      expect(Puppet).to receive(:debug).with(/No mapping between account names and security IDs was done/)
+      subject.name_to_principal('S-1-5-21-INVALID-SID')
+    end
+
     it "should accept unqualified account name" do
       # NOTE: lookup by name works in localized environments only for a few instances
       # this works in French Windows, even though the account is really Syst\u00E8me
       expect(subject.name_to_principal('SYSTEM').sid).to eq(sid)
     end
 
+    it "should not print debug messages for unqualified account name" do
+      expect(Puppet).not_to receive(:debug).with(/Could not retrieve raw SID bytes from/)
+      expect(Puppet).not_to receive(:debug).with(/No mapping between account names and security IDs was done/)
+      subject.name_to_principal('SYSTEM')
+    end
+
     it "should be case-insensitive" do
       # NOTE: lookup by name works in localized environments only for a few instances
       # this works in French Windows, even though the account is really Syst\u00E8me
       expect(subject.name_to_principal('SYSTEM')).to eq(subject.name_to_principal('system'))
     end
 
+    it "should not print debug messages for wrongly cased account name" do
+      expect(Puppet).not_to receive(:debug).with(/Could not retrieve raw SID bytes from/)
+      expect(Puppet).not_to receive(:debug).with(/No mapping between account names and security IDs was done/)
+      subject.name_to_principal('system')
+    end
+
     it "should be leading and trailing whitespace-insensitive" do
       # NOTE: lookup by name works in localized environments only for a few instances
       # this works in French Windows, even though the account is really Syst\u00E8me
       expect(subject.name_to_principal('SYSTEM')).to eq(subject.name_to_principal(' SYSTEM '))
     end
 
+    it "should not print debug messages for account name with leading and trailing whitespace" do
+      expect(Puppet).not_to receive(:debug).with(/Could not retrieve raw SID bytes from/)
+      expect(Puppet).not_to receive(:debug).with(/No mapping between account names and security IDs was done/)
+      subject.name_to_principal(' SYSTEM ')
+    end
+
     it "should accept domain qualified account names" do
       # NOTE: lookup by name works in localized environments only for a few instances
       # this works in French Windows, even though the account is really AUTORITE NT\\Syst\u00E8me
       expect(subject.name_to_principal('NT AUTHORITY\SYSTEM').sid).to eq(sid)
     end
 
-    it "should print a debug message on failures" do
-      expect(Puppet).to receive(:debug).with(/Could not retrieve raw SID bytes from 'NonExistingUser'/)
-      expect(Puppet).to receive(:debug).with(/No mapping between account names and security IDs was done/)
-      subject.name_to_principal('NonExistingUser')
+    it "should not print debug messages for domain qualified account names" do
+      expect(Puppet).not_to receive(:debug).with(/Could not retrieve raw SID bytes from/)
+      expect(Puppet).not_to receive(:debug).with(/No mapping between account names and security IDs was done/)
+      subject.name_to_principal(' SYSTEM ')
     end
   end