diff --git a/changelogs/fragments/win_module_utils_sid-conversion.yaml b/changelogs/fragments/win_module_utils_sid-conversion.yaml new file mode 100644 index 0000000000..ce434987ee --- /dev/null +++ b/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 diff --git a/lib/ansible/module_utils/powershell/Ansible.ModuleUtils.SID.psm1 b/lib/ansible/module_utils/powershell/Ansible.ModuleUtils.SID.psm1 index a29842e39b..cb1a8b0059 100644 --- a/lib/ansible/module_utils/powershell/Ansible.ModuleUtils.SID.psm1 +++ b/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 ".") { diff --git a/lib/ansible/modules/windows/win_user_right.ps1 b/lib/ansible/modules/windows/win_user_right.ps1 index 97f74ca268..0f308a919f 100644 --- a/lib/ansible/modules/windows/win_user_right.ps1 +++ b/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) diff --git a/test/integration/targets/win_module_utils/library/sid_utils_test.ps1 b/test/integration/targets/win_module_utils/library/sid_utils_test.ps1 index e64f5cf454..baf891e111 100644 --- a/test/integration/targets/win_module_utils/library/sid_utils_test.ps1 +++ b/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" } diff --git a/test/integration/targets/win_module_utils/tasks/main.yml b/test/integration/targets/win_module_utils/tasks/main.yml index 8f4ba6f851..2c8561fc7e 100644 --- a/test/integration/targets/win_module_utils/tasks/main.yml +++ b/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: diff --git a/test/integration/targets/win_user_right/tasks/tests.yml b/test/integration/targets/win_user_right/tasks/tests.yml index 998046d1db..649e8fbe35 100644 --- a/test/integration/targets/win_user_right/tasks/tests.yml +++ b/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: