diff --git a/changelogs/fragments/win_group_membership-sid-bugfix.yml b/changelogs/fragments/win_group_membership-sid-bugfix.yml new file mode 100644 index 0000000000..83d707384f --- /dev/null +++ b/changelogs/fragments/win_group_membership-sid-bugfix.yml @@ -0,0 +1,2 @@ +bugfixes: +- win_group_membership - uses the internal Ansible SID conversion logic and uses that when comparing group membership instead of the name https://github.com/ansible/ansible/issues/40649 diff --git a/lib/ansible/modules/windows/win_group_membership.ps1 b/lib/ansible/modules/windows/win_group_membership.ps1 index 52624a31d5..44d5407c63 100644 --- a/lib/ansible/modules/windows/win_group_membership.ps1 +++ b/lib/ansible/modules/windows/win_group_membership.ps1 @@ -1,107 +1,36 @@ #!powershell -# (c) 2017, Andrew Saraceni -# -# This file is part of Ansible -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . +# Copyright: (c) 2017, Andrew Saraceni +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) -# WANT_JSON -# POWERSHELL_COMMON +#Requires -Module Ansible.ModuleUtils.Legacy +#Requires -Module Ansible.ModuleUtils.SID $ErrorActionPreference = "Stop" function Test-GroupMember { <# .SYNOPSIS - Parse desired member into domain and username. + Return SID and consistent account name (DOMAIN\Username) format of desired member. Also, ensure member can be resolved/exists on the target system by checking its SID. .NOTES Returns a hashtable of the same type as returned from Get-GroupMember. - Accepts username (users, groups) and domains in the following formats: - - username - - .\username - - SERVERNAME\username - - NT AUTHORITY\username - - DOMAIN\username - - username@DOMAIN + Accepts username (users, groups) and domains in the formats accepted by Convert-ToSID. #> param( [String]$GroupMember ) $parsed_member = @{ - domain = $null - username = $null - combined = $null + sid = $null + account_name = $null } - # Split domain and account name into separate values - # '\' or '@' needs additional parsing, otherwise assume local computer + $sid = Convert-ToSID -account_name $GroupMember + $account_name = Convert-FromSID -sid $sid - if ($GroupMember -match "\\") { - # DOMAIN\username - $split_member = $GroupMember.Split("\") - - if ($split_member[0] -in @($env:COMPUTERNAME, ".")) { - # Local - $parsed_member.domain = $env:COMPUTERNAME - } - else { - # Domain or service (i.e. NT AUTHORITY) - $parsed_member.domain = $split_member[0] - } - $parsed_member.username = $split_member[1] - } - elseif ($GroupMember -match "@") { - # username@DOMAIN - $parsed_member.domain = $GroupMember.Split("@")[1] - $parsed_member.username = $GroupMember.Split("@")[0] - } - else { - # Local - $parsed_member.domain = $env:COMPUTERNAME - $parsed_member.username = $GroupMember - } - - if ($parsed_member.domain -match "\.") { - # Assume FQDN was passed - change to NetBIOS/short name for later ADSI membership comparisons - $netbios_name = (Get-CimInstance -ClassName Win32_NTDomain -Filter "DnsForestName = '$($parsed_member.domain)'").DomainName - - if (!$netbios_name) { - Fail-Json -obj $result -message "Could not resolve NetBIOS name for domain $($parsed_member.domain)" - } - $parsed_member.domain = $netbios_name - } - - # Set SID check arguments, and 'combined' for later comparison and output reporting - if ($parsed_member.domain -eq $env:COMPUTERNAME) { - $sid_check_args = @($parsed_member.username) - $parsed_member.combined = "{0}" -f $parsed_member.username - } - else { - $sid_check_args = @($parsed_member.domain, $parsed_member.username) - $parsed_member.combined = "{0}\{1}" -f $parsed_member.domain, $parsed_member.username - } - - try { - $user_object = New-Object -TypeName System.Security.Principal.NTAccount -ArgumentList $sid_check_args - $user_object.Translate([System.Security.Principal.SecurityIdentifier]) - } - catch { - Fail-Json -obj $result -message "Could not resolve group member $GroupMember" - } + $parsed_member.sid = $sid + $parsed_member.account_name = $account_name return $parsed_member } @@ -120,35 +49,28 @@ function Get-GroupMember { $members = @() $current_members = $Group.psbase.Invoke("Members") | ForEach-Object { - ([ADSI]$_).InvokeGet("ADsPath") + $bytes = ([ADSI]$_).InvokeGet("objectSID") + $sid = New-Object -TypeName Security.Principal.SecurityIdentifier -ArgumentList $bytes, 0 + $adspath = ([ADSI]$_).InvokeGet("ADsPath") + + @{sid = $sid; adspath = $adspath} | Write-Output } foreach ($current_member in $current_members) { $parsed_member = @{ - domain = $null - username = $null - combined = $null + sid = $current_member.sid + account_name = $null } - $rootless_adspath = $current_member.Replace("WinNT://", "") + $rootless_adspath = $current_member.adspath.Replace("WinNT://", "") $split_adspath = $rootless_adspath.Split("/") - if ($split_adspath -match $env:COMPUTERNAME) { - # Local - $parsed_member.domain = $env:COMPUTERNAME - $parsed_member.username = $split_adspath[-1] - $parsed_member.combined = $split_adspath[-1] - } - elseif ($split_adspath.Count -eq 1 -and $split_adspath[0] -like "S-1*") { - # Broken SID - $parsed_member.username = $split_adspath[0] - $parsed_member.combined = $split_adspath[0] - } - else { - # Domain or service (i.e. NT AUTHORITY) - $parsed_member.domain = $split_adspath[0] - $parsed_member.username = $split_adspath[1] - $parsed_member.combined = "{0}\{1}" -f $split_adspath[0], $split_adspath[1] + # Ignore lookup on a broken SID, and just return the SID as the account_name + if ($split_adspath.Count -eq 1 -and $split_adspath[0] -like "S-1*") { + $parsed_member.account_name = $split_adspath[0] + } else { + $account_name = Convert-FromSID -sid $current_member.sid + $parsed_member.account_name = $account_name } $members += $parsed_member @@ -170,8 +92,7 @@ $result = @{ } if ($state -eq "present") { $result.added = @() -} -elseif ($state -eq "absent") { +} elseif ($state -eq "absent") { $result.removed = @() } @@ -189,31 +110,29 @@ foreach ($member in $members) { $user_in_group = $false foreach ($current_member in $current_members) { - if ($current_member.combined -eq $group_member.combined) { + if ($current_member.sid -eq $group_member.sid) { $user_in_group = $true break } } - $member_adspath = "WinNT://{0}/{1}" -f $group_member.domain, $group_member.username + $member_sid = "WinNT://{0}" -f $group_member.sid try { if ($state -eq "present" -and !$user_in_group) { if (!$check_mode) { - $group.Add($member_adspath) - $result.added += $group_member.combined + $group.Add($member_sid) + $result.added += $group_member.account_name } $result.changed = $true - } - elseif ($state -eq "absent" -and $user_in_group) { + } elseif ($state -eq "absent" -and $user_in_group) { if (!$check_mode) { - $group.Remove($member_adspath) - $result.removed += $group_member.combined + $group.Remove($member_sid) + $result.removed += $group_member.account_name } $result.changed = $true } - } - catch { + } catch { Fail-Json -obj $result -message $_.Exception.Message } } @@ -221,10 +140,9 @@ foreach ($member in $members) { $final_members = Get-GroupMember -Group $group if ($final_members) { - $result.members = [Array]$final_members.combined -} -else { + $result.members = [Array]$final_members.account_name +} else { $result.members = @() } -Exit-Json -obj $result +Exit-Json -obj $result \ No newline at end of file diff --git a/lib/ansible/modules/windows/win_group_membership.py b/lib/ansible/modules/windows/win_group_membership.py index b6817dae6d..1e92bbc159 100644 --- a/lib/ansible/modules/windows/win_group_membership.py +++ b/lib/ansible/modules/windows/win_group_membership.py @@ -4,9 +4,6 @@ # Copyright: (c) 2017, Andrew Saraceni # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) -# this is a windows documentation stub. actual code lives in the .ps1 -# file of the same name - ANSIBLE_METADATA = {'metadata_version': '1.1', 'status': ['preview'], 'supported_by': 'community'} @@ -27,9 +24,11 @@ options: members: description: - A list of members to ensure are present/absent from the group. - - Accepts local users as username, .\username, and SERVERNAME\username. + - Accepts local users as .\username, and SERVERNAME\username. - Accepts domain users and groups as DOMAIN\username and username@DOMAIN. - Accepts service users as NT AUTHORITY\username. + - Accepts all local, domain and service user types as username, + favoring domain lookups when in a domain. required: yes state: description: @@ -69,7 +68,7 @@ added: empty if no members are added. returned: success and C(state) is C(present) type: list - sample: ["NewLocalAdmin", "DOMAIN\\TestUser"] + sample: ["SERVERNAME\\NewLocalAdmin", "DOMAIN\\TestUser"] removed: description: A list of members removed when C(state) is C(absent); this is empty if no members are removed. @@ -81,5 +80,5 @@ members: if the group contains no members. returned: success type: list - sample: ["DOMAIN\\TestUser", "NewLocalAdmin"] + sample: ["DOMAIN\\TestUser", "SERVERNAME\\NewLocalAdmin"] ''' diff --git a/test/integration/targets/win_group_membership/tasks/main.yml b/test/integration/targets/win_group_membership/tasks/main.yml index 64247200b4..d26e2e6c96 100644 --- a/test/integration/targets/win_group_membership/tasks/main.yml +++ b/test/integration/targets/win_group_membership/tasks/main.yml @@ -6,11 +6,22 @@ name: WinGroupMembershipTest state: absent +- name: Remove potentially leftover test user + win_user: &wu_absent + name: WinTestUser + state: absent + - name: Add new test group win_group: name: WinGroupMembershipTest state: present +- name: Add new test user + win_user: + name: WinTestUser + password: "W1nGr0upM3mb3rsh1pT3$tP@$$w0rd" + state: present + - name: Run tests for win_group_membership block: @@ -18,14 +29,19 @@ import_tasks: tests.yml vars: win_local_group: WinGroupMembershipTest + win_local_user: WinTestUser in_check_mode: no - name: Test in check-mode import_tasks: tests.yml vars: win_local_group: WinGroupMembershipTest + win_local_user: WinTestUser in_check_mode: yes check_mode: yes - name: Remove test group win_group: *wg_absent + +- name: Remove test user + win_group: *wu_absent \ No newline at end of file diff --git a/test/integration/targets/win_group_membership/tasks/tests.yml b/test/integration/targets/win_group_membership/tasks/tests.yml index f0f2729b5d..f6ff8d8718 100644 --- a/test/integration/targets/win_group_membership/tasks/tests.yml +++ b/test/integration/targets/win_group_membership/tasks/tests.yml @@ -1,21 +1,7 @@ # Test code for win_group_membership -# (c) 2017, Andrew Saraceni -# -# This file is part of Ansible -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . +# Copyright: (c) 2017, Andrew Saraceni +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) - name: Look up built-in Administrator account name (-500 user whose domain == computer name) raw: $machine_sid = (Get-CimInstance Win32_UserAccount -Filter "Domain='$env:COMPUTERNAME'")[0].SID -replace '(S-1-5-21-\d+-\d+-\d+)-\d+', '$1'; (Get-CimInstance Win32_UserAccount -Filter "SID='$machine_sid-500'").Name @@ -30,7 +16,7 @@ name: "{{ win_local_group }}" members: - "{{ admin_account_name }}" - - Guest + - "{{ win_local_user }}" - NT AUTHORITY\SYSTEM - NT AUTHORITY\NETWORK SERVICE state: absent @@ -53,17 +39,7 @@ - FakeUser state: present register: add_fake_local_user - failed_when: add_fake_local_user.changed != false or add_fake_local_user.msg != "Could not resolve group member FakeUser" - - -- name: Add fake FQDN domain user - win_group_membership: - name: "{{ win_local_group }}" - members: - - FakeUser@domain.fake - state: present - register: add_fake_fqdn_domain_user - failed_when: add_fake_fqdn_domain_user.changed != false or add_fake_fqdn_domain_user.msg != "Could not resolve NetBIOS name for domain domain.fake" + failed_when: add_fake_local_user.changed != false or add_fake_local_user.msg is not search("account_name FakeUser is not a valid account, cannot get SID.*") - name: Add users to group @@ -71,7 +47,7 @@ name: "{{ win_local_group }}" members: - "{{ admin_account_name }}" - - Guest + - "{{ win_local_user }}" - NT AUTHORITY\SYSTEM state: present register: add_users_to_group @@ -80,8 +56,8 @@ assert: that: - add_users_to_group.changed == true - - add_users_to_group.added == [admin_account_name, "Guest", "NT AUTHORITY\\SYSTEM"] - - add_users_to_group.members == [admin_account_name, "Guest", "NT AUTHORITY\\SYSTEM"] + - add_users_to_group.added == ["{{ ansible_hostname }}\\{{ admin_account_name }}", "{{ ansible_hostname }}\\{{ win_local_user }}", "NT AUTHORITY\\SYSTEM"] + - add_users_to_group.members == ["{{ ansible_hostname }}\\{{ admin_account_name }}", "{{ ansible_hostname }}\\{{ win_local_user }}", "NT AUTHORITY\\SYSTEM"] when: not in_check_mode - name: Test add_users_to_group (check-mode) @@ -102,7 +78,7 @@ that: - add_users_to_group_again.changed == false - add_users_to_group_again.added == [] - - add_users_to_group_again.members == [admin_account_name, "Guest", "NT AUTHORITY\\SYSTEM"] + - add_users_to_group_again.members == ["{{ ansible_hostname }}\\{{ admin_account_name }}", "{{ ansible_hostname }}\\{{ win_local_user }}", "NT AUTHORITY\\SYSTEM"] when: not in_check_mode @@ -111,7 +87,7 @@ <<: *wgm_present members: - '{{ ansible_hostname }}\{{ admin_account_name }}' - - .\Guest + - '.\{{ win_local_user }}' register: add_different_syntax_users_to_group_again - name: Test add_different_syntax_users_to_group_again (normal mode) @@ -119,7 +95,7 @@ that: - add_different_syntax_users_to_group_again.changed == false - add_different_syntax_users_to_group_again.added == [] - - add_different_syntax_users_to_group_again.members == [admin_account_name, "Guest", "NT AUTHORITY\\SYSTEM"] + - add_different_syntax_users_to_group_again.members == ["{{ ansible_hostname }}\\{{ admin_account_name }}", "{{ ansible_hostname }}\\{{ win_local_user }}", "NT AUTHORITY\\SYSTEM"] when: not in_check_mode - name: Test add_different_syntax_users_to_group_again (check-mode) @@ -143,7 +119,7 @@ that: - add_another_user_to_group.changed == true - add_another_user_to_group.added == ["NT AUTHORITY\\NETWORK SERVICE"] - - add_another_user_to_group.members == [admin_account_name, "Guest", "NT AUTHORITY\\SYSTEM", "NT AUTHORITY\\NETWORK SERVICE"] + - add_another_user_to_group.members == ["{{ ansible_hostname }}\\{{ admin_account_name }}", "{{ ansible_hostname }}\\{{ win_local_user }}", "NT AUTHORITY\\SYSTEM", "NT AUTHORITY\\NETWORK SERVICE"] when: not in_check_mode - name: Test add_another_user_to_group (check-mode) @@ -164,7 +140,7 @@ that: - add_another_user_to_group_again.changed == false - add_another_user_to_group_again.added == [] - - add_another_user_to_group_again.members == [admin_account_name, "Guest", "NT AUTHORITY\\SYSTEM", "NT AUTHORITY\\NETWORK SERVICE"] + - add_another_user_to_group_again.members == ["{{ ansible_hostname }}\\{{ admin_account_name }}", "{{ ansible_hostname }}\\{{ win_local_user }}", "NT AUTHORITY\\SYSTEM", "NT AUTHORITY\\NETWORK SERVICE"] when: not in_check_mode @@ -178,7 +154,7 @@ assert: that: - remove_users_from_group.changed == true - - remove_users_from_group.removed == [admin_account_name, "Guest", "NT AUTHORITY\\SYSTEM"] + - remove_users_from_group.removed == ["{{ ansible_hostname }}\\{{ admin_account_name }}", "{{ ansible_hostname }}\\{{ win_local_user }}", "NT AUTHORITY\\SYSTEM"] - remove_users_from_group.members == ["NT AUTHORITY\\NETWORK SERVICE"] when: not in_check_mode @@ -209,7 +185,7 @@ <<: *wgm_absent members: - '{{ ansible_hostname }}\{{ admin_account_name }}' - - .\Guest + - '.\{{ win_local_user }}' register: remove_different_syntax_users_from_group_again - name: Test remove_different_syntax_users_from_group_again (normal mode) @@ -263,4 +239,4 @@ - remove_another_user_from_group_again.changed == false - remove_another_user_from_group_again.removed == [] - remove_another_user_from_group_again.members == [] - when: not in_check_mode + when: not in_check_mode \ No newline at end of file