diff options
author | Salim Alam <salam@chef.io> | 2016-04-11 11:07:58 -0700 |
---|---|---|
committer | Salim Alam <salam@chef.io> | 2016-04-11 11:07:58 -0700 |
commit | 53b769dc1fc29c1620a367411bebcf175fa29284 (patch) | |
tree | d9f632cc5ed2030467366e3d61be8aa889b79002 | |
parent | e8bf4a11d3cd85d3b4dba82623cd2e7aad1053a8 (diff) | |
parent | 42a7bd45202848b19f25e90a4131b967cbf4f675 (diff) | |
download | chef-53b769dc1fc29c1620a367411bebcf175fa29284.tar.gz |
Merge pull request #4812 from chef/salam/group-fix
Removing non-existent members from group should not fail
-rw-r--r-- | lib/chef/provider/group.rb | 7 | ||||
-rw-r--r-- | lib/chef/provider/group/windows.rb | 25 | ||||
-rw-r--r-- | spec/functional/resource/group_spec.rb | 8 | ||||
-rw-r--r-- | spec/unit/provider/group/windows_spec.rb | 10 |
4 files changed, 39 insertions, 11 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..5873e42a6b 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(account_name)}' could not be found") + "" + end + end + end end end diff --git a/spec/functional/resource/group_spec.rb b/spec/functional/resource/group_spec.rb index 521c8e887d..f7c232b588 100644 --- a/spec/functional/resource/group_spec.rb +++ b/spec/functional/resource/group_spec.rb @@ -267,14 +267,14 @@ describe Chef::Resource::Group, :requires_root_or_running_windows, :not_supporte end describe "when removing members" do - it "raises an error for a non well-formed domain name" do + it "does not raise an error for a non well-formed domain name" do group_resource.excluded_members [invalid_domain_user_name] - expect { group_resource.run_action(tested_action) }.to raise_error Chef::Exceptions::Win32APIError + expect { group_resource.run_action(tested_action) }.to_not raise_error Chef::Exceptions::Win32APIError end - it "raises an error for a nonexistent domain" do + it "does not raise an error for a nonexistent domain" do group_resource.excluded_members [nonexistent_domain_user_name] - expect { group_resource.run_action(tested_action) }.to raise_error Chef::Exceptions::Win32APIError + expect { group_resource.run_action(tested_action) }.to_not raise_error Chef::Exceptions::Win32APIError end end end diff --git a/spec/unit/provider/group/windows_spec.rb b/spec/unit/provider/group/windows_spec.rb index c424e35e0e..f059f2e717 100644 --- a/spec/unit/provider/group/windows_spec.rb +++ b/spec/unit/provider/group/windows_spec.rb @@ -51,11 +51,13 @@ describe Chef::Provider::Group::Windows do @new_resource.members([ "us" ]) @current_resource = Chef::Resource::Group.new("staff") @current_resource.members %w{all your base} + @new_resource.excluded_members %w{all} 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 @@ -71,6 +73,12 @@ describe Chef::Provider::Group::Windows do @provider.manage_group end + it "should call @net_group.local_delete_members" do + allow(@new_resource).to receive(:append).and_return(true) + allow(@provider).to receive(:lookup_account_name).with("all").and_return("all") + expect(@net_group).to receive(:local_delete_members).with(@new_resource.excluded_members) + @provider.manage_group + end end describe "remove_group" do |