Ansible.ModuleUtils.SID - allow SID as an input to allow people to specify well know SIDs instead of the name (#39400)

changelogs/fragments/win_module_utils_sid-conversion.yaml

@@ -0,0 +1,6 @@
+minor_changes:
+- PowerShell modules that use Convert-ToSID in Ansible.ModuleUtils.SID.psm1
+  like win_user_right now accept an actual SID as an input string. This means
+  any local or domain accounts that are named like a SID need to be prefixed
+  with the domain, hostname, or . to ensure it converts to that accounts SID
+  https://github.com/ansible/ansible/issues/38502

lib/ansible/module_utils/powershell/Ansible.ModuleUtils.SID.psm1

@@ -12,12 +12,15 @@ Function Convert-FromSID($sid) {
     } catch {
         Fail-Json -obj @{} -message "failed to convert sid '$sid' to a logon name: $($_.Exception.Message)"
     }
-    
+
     return $nt_account.Value
 }
 
-Function Convert-ToSID($account_name) {
+Function Convert-ToSID {
+    [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingEmptyCatchBlock", "", Justification="We don't care if converting to a SID fails, just that it failed or not")]
+    param($account_name)
     # Converts an account name to a SID, it can take in the following forms
+    # SID: Will just return the SID value that was passed in
     # UPN:
     #   principal@domain (Domain users only)
     # Down-Level Login Name
@@ -28,6 +31,11 @@ Function Convert-ToSID($account_name) {
     # Login Name
     #   principal (Local/Local Service Accounts)
 
+    try {
+        $sid = New-Object -TypeName System.Security.Principal.SecurityIdentifier -ArgumentList $account_name
+        return $sid.Value
+    } catch {}
+
     if ($account_name -like "*\*") {
         $account_name_split = $account_name -split "\\"
         if ($account_name_split[0] -eq ".") {

lib/ansible/modules/windows/win_user_right.ps1

@@ -5,6 +5,8 @@
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
 #Requires -Module Ansible.ModuleUtils.Legacy
+#Requires -Module Ansible.ModuleUtils.SID
+
 $ErrorActionPreference = 'Stop'
 
 $params = Parse-Args $args -supports_check_mode $true
@@ -264,78 +266,6 @@ namespace Ansible
 }
 "@
 
-Function Get-Username($sid) {
-    # converts the SID (if it is one) to a username
-
-    $object = New-Object System.Security.Principal.SecurityIdentifier($sid)
-    $user = $object.Translate([System.Security.Principal.NTAccount])
-    return $user.Value
-}
-
-Function Get-SID($account_name) {
-    # Can take in the following account name forms and convert to a SID
-    # UPN:
-    #   username@domain (Domain)
-    # Down-Level Login Name
-    #   domain\username (Domain)
-    #   computername\username (Local)
-    #   .\username (Local)
-    # Login Name
-    #   username (Local)
-
-    if ($account_name -like "*\*") {
-        $account_name_split = $account_name -split "\\"
-        if ($account_name_split[0] -eq ".") {
-            $domain = $env:COMPUTERNAME
-        } else {
-            $domain = $account_name_split[0]
-        }
-        $username = $account_name_split[1]
-    } elseif ($account_name -like "*@*") {
-        $account_name_split = $account_name -split "@"
-        $domain = $account_name_split[1]
-        $username = $account_name_split[0]
-    } else {
-        $domain = $null
-        $username = $account_name
-    }
-
-    if ($domain) {
-        # searching for a local group with the servername prefixed will fail,
-        # need to check for this situation and only use NTAccount(String)
-        if ($domain -eq $env:COMPUTERNAME) {
-            $adsi = [ADSI]("WinNT://$env:COMPUTERNAME,computer")
-            $group = $adsi.psbase.children | Where-Object { $_.schemaClassName -eq "group" } | Where-Object { $_.Name -eq $username }
-        } else {
-            $group = $null
-        }
-        if ($group) {
-            $account = New-Object System.Security.Principal.NTAccount($username)
-        } else {
-            $account = New-Object System.Security.Principal.NTAccount($domain, $username)
-        }
-    } else {
-        # when in a domain NTAccount(String) will favour domain lookups check
-        # if username is a local user and explictly search on the localhost for
-        # that account
-        $adsi = [ADSI]("WinNT://$env:COMPUTERNAME,computer")
-        $user = $adsi.psbase.children | Where-Object { $_.schemaClassName -eq "user" } | Where-Object { $_.Name -eq $username }
-        if ($user) {
-            $account = New-Object System.Security.Principal.NTAccount($env:COMPUTERNAME, $username)
-        } else {
-            $account = New-Object System.Security.Principal.NTAccount($username)
-        }
-    }
-    
-    try {
-        $account_sid = $account.Translate([System.Security.Principal.SecurityIdentifier])
-    } catch {
-        Fail-Json $result "Account Name: $account_name is not a valid account, cannot get SID: $($_.Exception.Message)"
-    }
-    
-    return $account_sid.Value
-}
-
 Function Compare-UserList($existing_users, $new_users) {  
     $added_users = [String[]]@()
     $removed_users = [String[]]@()
@@ -361,7 +291,7 @@ $lsa_helper = New-Object -TypeName Ansible.LsaRightHelper
 
 $new_users = [System.Collections.ArrayList]@()
 foreach ($user in $users) {
-    $new_users.Add((Get-SID -account_name $user))
+    $new_users.Add((Convert-ToSID -account_name $user))
 }
 $new_users = [String[]]$new_users.ToArray()
 try {
@@ -383,7 +313,7 @@ if (($change_result.added.Length -gt 0) -or ($change_result.removed.Length -gt 0
         if (-not $check_mode) {
             $lsa_helper.RemovePrivilege($user, $name)
         }
-        $user_name = Get-Username -sid $user
+        $user_name = Convert-FromSID -sid $user
         $result.removed += $user_name
         $diff_text += "-$user_name`n"
         $new_user_list.Remove($user)
@@ -392,7 +322,7 @@ if (($change_result.added.Length -gt 0) -or ($change_result.removed.Length -gt 0
         if (-not $check_mode) {
             $lsa_helper.AddPrivilege($user, $name)
         }
-        $user_name = Get-Username -sid $user
+        $user_name = Convert-FromSID -sid $user
         $result.added += $user_name
         $diff_text += "+$user_name`n"
         $new_user_list.Add($user)

test/integration/targets/win_module_utils/library/sid_utils_test.ps1

@@ -3,6 +3,9 @@
 #Requires -Module Ansible.ModuleUtils.Legacy
 #Requires -Module Ansible.ModuleUtils.SID
 
+$params = Parse-Args $args
+$sid_account = Get-AnsibleParam -obj $params -name "sid_account" -type "str" -failifempty $true
+
 Function Assert-Equals($actual, $expected) {
     if ($actual -ne $expected) {
         Fail-Json @{} "actual != expected`nActual: $actual`nExpected: $expected"
@@ -76,4 +79,15 @@ foreach ($test in $tests) {
     }
 }
 
+# the account to SID test is run outside of the normal run as we can't test it
+# in the normal test suite
+# Calling Convert-ToSID with a string like a SID should return that SID back
+$actual = Convert-ToSID -account_name $sid_account
+Assert-Equals -actual $actual -expected $sid_account
+
+# Calling COnvert-ToSID with a string prefixed with .\ should return the SID
+# for a user that is called that SID and not the SID passed in
+$actual = Convert-ToSID -account_name ".\$sid_account"
+Assert-Equals -actual ($actual -ne $sid_account) -expected $true
+
 Exit-Json @{ data = "success" }

test/integration/targets/win_module_utils/tasks/main.yml

@@ -56,9 +56,23 @@
     that:
     - camel_conversion.data == 'success'
 
-- name: call module with SID tests
-  sid_utils_test:
-  register: sid_test
+- block:
+  - name: create test user with well know SID as the name
+    win_user:
+      name: S-1-0-0
+      password: AbcDef123!@#
+      state: present
+
+  - name: call module with SID tests
+    sid_utils_test:
+      sid_account: S-1-0-0
+    register: sid_test
+
+  always:
+  - name: remove test SID user
+    win_user:
+      name: S-1-0-0
+      state: absent
 
 - assert:
     that:

test/integration/targets/win_user_right/tasks/tests.yml

@@ -19,7 +19,7 @@
     name: '{{test_win_user_right_name}}'
     users: FakeUser
   register: fail_invalid_user
-  failed_when: "'Account Name: FakeUser is not a valid account, cannot get SID' not in fail_invalid_user.msg"
+  failed_when: "'account_name FakeUser is not a valid account, cannot get SID' not in fail_invalid_user.msg"
 
 - name: remove from empty right check
   win_user_right: