Refactor/fix win_group_membership to use SIDs for internal comparisons (#40725)

* refactor win_group_membership to use SIDs for comparisons instead of name parsing * carry over previous doc cleanup changes * remove trailing whitespace from docs
2018-06-26 20:54:27 -04:00 · 2018-06-26 20:54:27 -04:00 · bcb49f2575
commit bcb49f2575
parent cc97e89620
4 changed files with 74 additions and 165 deletions
--- 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 <andrew.saraceni@gmail.com>
-#
-# 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 <http://www.gnu.org/licenses/>.
+# Copyright: (c) 2017, Andrew Saraceni <andrew.saraceni@gmail.com>
+# 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
--- 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 <andrew.saraceni@gmail.com>
 # 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"]
 '''
--- 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
--- 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 <andrew.saraceni@gmail.com>
-#
-# 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 <http://www.gnu.org/licenses/>.
+# Copyright: (c) 2017, Andrew Saraceni <andrew.saraceni@gmail.com>
+# 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