Don't fail when removing non-existing group member

3 files changed, 28 insertions, 7 deletions

diff --git a/lib/chef/provider/group.rb b/lib/chef/provider/group.rb
index fcae24f8f9..8936bd2031 100644
--- a/lib/chef/provider/group.rb
+++ b/lib/chef/provider/group.rb
@@ -94,6 +94,7 @@ class Chef
           missing_members = []
           @new_resource.members.each do |member|
             next if has_current_group_member?(member)
+            validate_member!(member)
             missing_members << member
           end
           if missing_members.length > 0
@@ -122,6 +123,12 @@ class Chef
         @current_resource.members.include?(member)
       end
 
+      def validate_member!(member)
+        # Sub-classes can do any validation if needed
+        # and raise an error if validation fails
+        true
+      end
+
       def action_create
         case @group_exists
         when false
diff --git a/lib/chef/provider/group/windows.rb b/lib/chef/provider/group/windows.rb
index 64b4587487..c115c40ccf 100644
--- a/lib/chef/provider/group/windows.rb
+++ b/lib/chef/provider/group/windows.rb
@@ -61,7 +61,7 @@ class Chef
           if @new_resource.append
             members_to_be_added = [ ]
             @new_resource.members.each do |member|
-              members_to_be_added << member if ! has_current_group_member?(member)
+              members_to_be_added << member if ! has_current_group_member?(member) && validate_member!(member)
             end
 
             # local_add_members will raise ERROR_MEMBER_IN_ALIAS if a
@@ -70,7 +70,7 @@ class Chef
 
             members_to_be_removed = [ ]
             @new_resource.excluded_members.each do |member|
-              member_sid = local_group_name_to_sid(member)
+              member_sid = lookup_account_name(member)
               members_to_be_removed << member if has_current_group_member?(member)
             end
             @net_group.local_delete_members(members_to_be_removed) unless members_to_be_removed.empty?
@@ -80,7 +80,7 @@ class Chef
         end
 
         def has_current_group_member?(member)
-          member_sid = local_group_name_to_sid(member)
+          member_sid = lookup_account_name(member)
           @current_resource.members.include?(member_sid)
         end
 
@@ -88,10 +88,23 @@ class Chef
           @net_group.local_delete
         end
 
-        def local_group_name_to_sid(group_name)
-          locally_qualified_name = group_name.include?("\\") ? group_name : "#{ENV['COMPUTERNAME']}\\#{group_name}"
-          Chef::ReservedNames::Win32::Security.lookup_account_name(locally_qualified_name)[1].to_s
+        def locally_qualified_name(account_name)
+          account_name.include?("\\") ? account_name : "#{ENV['COMPUTERNAME']}\\#{account_name}"
         end
+
+        def validate_member!(member)
+          Chef::ReservedNames::Win32::Security.lookup_account_name(locally_qualified_name(member))[1].to_s
+        end
+
+        def lookup_account_name(account_name)
+          begin
+            Chef::ReservedNames::Win32::Security.lookup_account_name(locally_qualified_name(account_name))[1].to_s
+          rescue Chef::Exceptions::Win32APIError
+            Chef::Log.warn("SID for '#{locally_qualified_name}' could not be found")
+            ""
+          end
+        end
+
       end
     end
   end
diff --git a/spec/unit/provider/group/windows_spec.rb b/spec/unit/provider/group/windows_spec.rb
index c424e35e0e..f551a898d9 100644
--- a/spec/unit/provider/group/windows_spec.rb
+++ b/spec/unit/provider/group/windows_spec.rb
@@ -55,7 +55,8 @@ describe Chef::Provider::Group::Windows do
       allow(Chef::Util::Windows::NetGroup).to receive(:new).and_return(@net_group)
       allow(@net_group).to receive(:local_add_members)
       allow(@net_group).to receive(:local_set_members)
-      allow(@provider).to receive(:local_group_name_to_sid)
+      allow(@provider).to receive(:lookup_account_name)
+      allow(@provider).to receive(:validate_member!).and_return(true)
       @provider.current_resource = @current_resource
     end