More optimization of code and updated get_account_with_user_rights to return account_name with domain

Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

4 files changed, 30 insertions, 21 deletions

diff --git a/lib/chef/resource/windows_user_privilege.rb b/lib/chef/resource/windows_user_privilege.rb
index 85f1557d09..685354cfb4 100644
--- a/lib/chef/resource/windows_user_privilege.rb
+++ b/lib/chef/resource/windows_user_privilege.rb
@@ -97,7 +97,7 @@ class Chef
 
       action :add do
         ([*new_resource.privilege] - [*current_resource.privilege]).each do |user_right|
-          converge_by("adding user privilege #{user_right}") do
+          converge_by("adding user '#{new_resource.principal}' privilege #{user_right}") do
             Chef::ReservedNames::Win32::Security.add_account_right(new_resource.principal, user_right)
           end
         end
@@ -113,7 +113,7 @@ class Chef
         # Getting users with its domain for comparison
         new_resource.users.each do |user|
           user = Chef::ReservedNames::Win32::Security.lookup_account_name(user)
-          users << user[1].account if user
+          users << user[1].account_name if user
         end
 
         new_resource.privilege.each do |privilege|
@@ -121,14 +121,16 @@ class Chef
 
           # comparing the existing accounts for privilege with users
           unless users == accounts
-            accounts.each do |account|
-              converge_by("removing user #{account[1]} from privilege #{privilege}") do
-                Chef::ReservedNames::Win32::Security.remove_account_right(account[1], privilege)
+            # Removing only accounts which is not matching with users in new_resource
+            (accounts - users).each do |account|
+              converge_by("removing user '#{account}' from privilege #{privilege}") do
+                Chef::ReservedNames::Win32::Security.remove_account_right(account, privilege)
               end
             end
 
-            new_resource.users.each do |user|
-              converge_by("adding user #{user} to privilege #{privilege}") do
+            # Adding only users which is not already exist
+            (users - accounts).each do |user|
+              converge_by("adding user '#{user}' to privilege #{privilege}") do
                 Chef::ReservedNames::Win32::Security.add_account_right(user, privilege)
               end
             end
@@ -145,7 +147,7 @@ class Chef
         end
 
         (new_resource.privilege - missing_res_privileges).each do |user_right|
-          converge_by("removing user privilege #{user_right}") do
+          converge_by("removing user #{new_resource.principal} from privilege #{user_right}") do
             Chef::ReservedNames::Win32::Security.remove_account_right(new_resource.principal, user_right)
           end
         end
diff --git a/lib/chef/win32/security.rb b/lib/chef/win32/security.rb
index 2879131210..2c0f63684a 100644
--- a/lib/chef/win32/security.rb
+++ b/lib/chef/win32/security.rb
@@ -227,15 +227,19 @@ class Chef
         accounts = []
         with_lsa_policy(nil) do |policy_handle, sid|
           result = LsaEnumerateAccountsWithUserRight(policy_handle.read_pointer, privilege_pointer, buffer, count)
-          win32_error = LsaNtStatusToWinError(result)
-          return [] if win32_error == 1313 # NO_SUCH_PRIVILEGE - https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes--1300-1699-
-
-          test_and_raise_lsa_nt_status(result)
-
-          count.read_ulong.times do |i|
-            sid = LSA_ENUMERATION_INFORMATION.new(buffer.read_pointer + i * LSA_ENUMERATION_INFORMATION.size)
-            sid_name = lookup_account_sid(sid[:Sid])
-            accounts << sid_name
+          if result == 0
+            win32_error = LsaNtStatusToWinError(result)
+            return [] if win32_error == 1313 # NO_SUCH_PRIVILEGE - https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes--1300-1699-
+
+            test_and_raise_lsa_nt_status(result)
+
+            count.read_ulong.times do |i|
+              sid = LSA_ENUMERATION_INFORMATION.new(buffer.read_pointer + i * LSA_ENUMERATION_INFORMATION.size)
+              sid_name = lookup_account_sid(sid[:Sid])
+              domain, name, use = sid_name
+              account_name = (!domain.nil? && domain.length > 0) ? "#{domain}\\#{name}" : name
+              accounts << account_name
+            end
           end
 
           result = LsaFreeMemory(buffer.read_pointer)
diff --git a/spec/functional/resource/windows_user_privilege_spec.rb b/spec/functional/resource/windows_user_privilege_spec.rb
index 9bfb1d731e..6dca54016a 100644
--- a/spec/functional/resource/windows_user_privilege_spec.rb
+++ b/spec/functional/resource/windows_user_privilege_spec.rb
@@ -51,6 +51,8 @@ describe Chef::Resource::WindowsUserPrivilege, :windows_only do
       let(:privilege) { "SeCreateSymbolicLinkPrivilege" }
 
       it "adds user to privilege" do
+        # Removing so that add update happens
+        subject.run_action(:remove)
         subject.run_action(:add)
         expect(subject).to be_updated_by_last_action
       end
@@ -64,7 +66,7 @@ describe Chef::Resource::WindowsUserPrivilege, :windows_only do
 
     context "when privilege is passed as array" do
       let(:principal) { "Administrator" }
-      let(:privilege) { ["SeCreateSymbolicLinkPrivilege", "SeCreatePagefilePrivilege"] }
+      let(:privilege) { %w{SeCreateSymbolicLinkPrivilege SeCreatePagefilePrivilege} }
 
       it "adds user to privilege" do
         subject.run_action(:add)
@@ -125,7 +127,7 @@ describe Chef::Resource::WindowsUserPrivilege, :windows_only do
     end
 
     context "when privilege is passed as array" do
-      let(:privilege) { ["SeCreateSymbolicLinkPrivilege", "SeCreatePagefilePrivilege"] }
+      let(:privilege) { %w{SeCreateSymbolicLinkPrivilege SeCreatePagefilePrivilege} }
       it "remove user from privilege" do
         subject.run_action(:add)
         subject.run_action(:remove)
diff --git a/spec/functional/win32/security_spec.rb b/spec/functional/win32/security_spec.rb
index 8caacffd2c..c01e9be9a3 100644
--- a/spec/functional/win32/security_spec.rb
+++ b/spec/functional/win32/security_spec.rb
@@ -200,14 +200,15 @@ describe "Chef::Win32::Security", :windows_only do
   end
 
   describe ".get_account_with_user_rights" do
+    let(:domain) { ENV["COMPUTERNAME"] }
     let(:username) { ENV["USERNAME"] }
 
     context "when given a valid user right" do
       it "gets all accounts associated with given user right" do
         Chef::ReservedNames::Win32::Security.add_account_right(username, "SeBatchLogonRight")
-        expect(Chef::ReservedNames::Win32::Security.get_account_with_user_rights("SeBatchLogonRight").flatten).to include(username)
+        expect(Chef::ReservedNames::Win32::Security.get_account_with_user_rights("SeBatchLogonRight").flatten).to include("#{domain}\\#{username}")
         Chef::ReservedNames::Win32::Security.remove_account_right(username, "SeBatchLogonRight")
-        expect(Chef::ReservedNames::Win32::Security.get_account_with_user_rights("SeBatchLogonRight").flatten).not_to include(username)
+        expect(Chef::ReservedNames::Win32::Security.get_account_with_user_rights("SeBatchLogonRight").flatten).not_to include("#{domain}\\#{username}")
       end
     end