Repository: ansible/ansible
Pull request: Make win_domain_user idempotent for password changes
##### SUMMARY
win_domain_user always returns changed if a password is specified.
This pr implements an additional option to not only update the password "on_create" or "always", but also "when_changed"

Fixes #58246 

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
win_domain_user


_OT side note I just learned, that GitHub does not allow to reopen PRs after you force pushed to the branche._

Problem statement:
win_domain_user not idempotent for setting passwords
##### SUMMARY
win_domain_user always returns changed if a password is specified.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
win_domain_user

##### ANSIBLE VERSION
```paste below
ansible 2.8.1
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.7.3 (default, Mar 26 2019, 21:43:19) [GCC 8.2.1 20181127]
```

##### CONFIGURATION

##### OS / ENVIRONMENT

ArchLinux

##### STEPS TO REPRODUCE
```yaml
- name: Set AD User Password
      win_domain_user:
        name: '{{ user_name }}'
        password: '{{ password }}'
        state: present
- name: Set AD User Password again
      win_domain_user:
        name: '{{ user_name }}'
        password: '{{ password }}'
        state: present
```

##### EXPECTED RESULTS
The 2nd task to show as not changed.

##### ACTUAL RESULTS
The 2nd task shows as changed, even though the password was already set.

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/windows/win_domain_user.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_domain_user.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @ShachafGoldstein @if-meaton @jborean93 @jhawkesworth @nitzmahone @nwchandler
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
This is not possible, there is no way to check the password without doing login attempts which I think is an overkill myself.
Every time a password is given we should be ok to assume the user is changed, at least in my opinion.
I wrestled with this for some time when I did the initial implementation of this. I don’t like showing that the password changed if it actually didn’t, but I could not come up with a clean way to verify whether it did or did not. If anyone can come up with a way to do this easily, that would be great, but I don’t suspect there is a great way to accomplish this. 
The only way to test it is to try login with the given password and if it works, return ok.
but as someone who managed an enterprise AD, I personally wouldn't want a 3rd party tool to start playing with tokens and maybe lock users or have permissions issue in the server side just for the OK
As an FYI we currently do this with win_user https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_user.ps1#L43-L107. It uses LogonUser right now but historically it used DirectoryyServices ValidateCredentials. The reason why it was changed was because it relied on RPC to test the credentials and was quite sensitive to a few bugs like name resolution failures as well as permissions when running through WinRM.

As for whether we should implement the same in win_domain_user I would bring that to the next IRC meeting so we can discuss it with a group.
I think within my pr #58247 I addressed the issue ShachafGoldstein mentioned about failed logins by just adding an additional option for `update_password` namely `when_changed`. That way if one wants the module to not validating the credentials one could just use `update_password: always` and the module will set the password regardless of the current value (aka. the current behaviour).

If either my approach using `[System.DirectoryServices.AccountManagement.PrincipalContext]::new('domain').ValidateCredentials($username, $password)` (which requires ADDS RSAT Tools, the same required for setting the password), or the more advanced function of jborean93, which requests a new token should be used, may need to be also discussed in the next IRC meeting.
@agowa338: I would really recommend not using `ValidateCredentials` due to the issues we came across with win_user. Also as an FYI the `::new()` syntaxt is a PowerShell v5 feature and we need to support v3+. The alternative syntax is `New-Object -TypeName System.DirectoryServices.AccountManagement.PrincipalContext -ArgumentList 'domain`
oh, thanks for the hint. Than I'm going to close my pr and wait for the outcome of the next meeting. As there is already a working implementation (that does not have these issues) in win_user, it could just be copied afterwards if everyone in the meeting agrees.
So we talked about this in the IRC meeting, the end result is that it is ok to have something like this but we should add it under a 3rd option for `update_password`. The naming of this option isn't too important (as long as it makes sense) but people can explicitly opt into the logon test as a way of idempotently change the password. The default of `always` and optional choice `on_create` would stay as they are today.

Commit message: Update win_domain_user.ps1

Fix ci

Patch to review:
diff --git a/changelogs/fragments/58246-win_domain_user-idempotency.yaml b/changelogs/fragments/58246-win_domain_user-idempotency.yaml
new file mode 100644
index 00000000000000..9e7c16f2f53477
--- /dev/null
+++ b/changelogs/fragments/58246-win_domain_user-idempotency.yaml
@@ -0,0 +1,3 @@
+---
+bugfixes:
+  - "win_domain_user - Allow to only set password when it actually changed (https://github.com/ansible/ansible/issues/58246)"
diff --git a/lib/ansible/modules/windows/win_domain_user.ps1 b/lib/ansible/modules/windows/win_domain_user.ps1
index def9c1e08b0ccd..1011ab296ade48 100644
--- a/lib/ansible/modules/windows/win_domain_user.ps1
+++ b/lib/ansible/modules/windows/win_domain_user.ps1
@@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
+        $LOGON32_LOGON_TYPE = 3 # NetworkLogon
+    )
+    if (($Username.ToCharArray()) -contains [char]'@') {
+        # UserPrincipalName
+        $Username = ($Username -split '@')[0]
+        $Domain = ($Username -split '@')[1]
+    } elseif (($Username.ToCharArray()) -contains [char]'\') {
+        # Pre Win2k Account Name
+        $Username = ($Username -split '\')[0]
+        $Domain = ($Username -split '\')[1]
+    } else {
+        # No domain provided, so maybe local user?
+    }
+    $null = $Domain # Make CI-Check happy...
+
+    # More information about LogonUser at https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-logonusera
+    $platform_util = @'
+using System;
+using System.Runtime.InteropServices;
+using System.ComponentModel;
+using System.Security;
+namespace Ansible
+{
+    public class WinUserPInvoke
+    {
+        const int LOGON32_LOGON_NETWORK           = 3;
+        const int LOGON32_PROVIDER_DEFAULT        = 0; // Negotiate if domain == null and username not upn else NTLM
+        const int LOGON32_PROVIDER_WINNT40        = 2; // Negotiate
+        const int LOGON32_PROVIDER_WINNT50        = 3; // NTLM
+        // Refere to: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/18d8fbe8-a967-4f1c-ae50-99ca8e491d2d
+        static int[] success_codes = new int[] {
+            0x0000052F,  // ERROR_ACCOUNT_RESTRICTION
+            0x00000530,  // ERROR_INVALID_LOGON_HOURS
+            0x00000531,  // ERROR_INVALID_WORKSTATION
+            0x00000569   // ERROR_LOGON_TYPE_GRANTED
+        };
+        static int[] failed_codes = new int[] {
+            0x0000052E,  // ERROR_LOGON_FAILURE - the user or pass was incorrect
+            0x00000006,  // ERROR_INVALID_HANDLE - the handle is invalid
+            0x00000532   // ERROR_PASSWORD_EXPIRED - The password has exired
+        };
+        [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
+        internal static extern bool LogonUser(String username, String domain, IntPtr password,
+            int logonType, int logonProvider, out IntPtr token);
+        [DllImport("kernel32.dll", SetLastError = true)]
+        internal static extern bool CloseHandle(
+            IntPtr hObject);
+        public static bool TestCredential(string userName, SecureString password, string domainName = null)
+        {
+            IntPtr tokenHandle = IntPtr.Zero;
+            IntPtr passwordPtr = IntPtr.Zero;
+            bool returnValue   = false;
+            int err_code = 0;
+
+            // Marshal the SecureString to unmanaged memory.
+            passwordPtr = Marshal.SecureStringToGlobalAllocUnicode(password);
+
+            // Pass LogonUser the decrypted copy of the password.
+            returnValue = LogonUser(userName, domainName, passwordPtr,
+                                    LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT,
+                                    out tokenHandle);
+            // Perform cleanup whether or not the call succeeded.
+            // Secure delete the plaintext password from memory.
+            Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr);
+            // We don't need the token, so close it again.
+            CloseHandle(tokenHandle);
+            if (returnValue || !(tokenHandle == IntPtr.Zero)) {
+                return true;
+            } else {
+                err_code = Marshal.GetLastWin32Error();
+                if (Array.Exists(success_codes, element => element == err_code)) {
+                    return true;
+                } else if (Array.Exists(failed_codes, element => element == err_code)) {
+                    return false;
+                } else {
+                    throw new System.ComponentModel.Win32Exception(err_code);
+                }
+            }
+        }
+    }
+}
+'@
+    if ($null -ne $Username) {
+        $original_tmp = $env:TMP
+        $env:TMP = $_remote_tmp
+        Add-Type -TypeDefinition $platform_util
+        $env:TMP = $original_tmp
+
+        # Todo: Make sure $UserName is always in UPN format.
+        # Than $Domain can be $null
+        $logon_res = [Ansible.WinUserPInvoke]::TestCredential($Username, (ConvertTo-SecureString $Password -AsPlainText -Force), $Domain)
+        return $logon_res
+    } else {
+        return $false
+    }
+}
+
 try {
     Import-Module ActiveDirectory
  }
@@ -23,7 +126,7 @@ $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -default
 
 # Module control parameters
 $state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "present" -validateset "present","absent","query"
-$update_password = Get-AnsibleParam -obj $params -name "update_password" -type "str" -default "always" -validateset "always","on_create"
+$update_password = Get-AnsibleParam -obj $params -name "update_password" -type "str" -default "always" -validateset "always","on_create","when_changed"
 $groups_action = Get-AnsibleParam -obj $params -name "groups_action" -type "str" -default "replace" -validateset "add","remove","replace"
 $domain_username = Get-AnsibleParam -obj $params -name "domain_username" -type "str"
 $domain_password = Get-AnsibleParam -obj $params -name "domain_password" -type "str" -failifempty ($null -ne $domain_username)
@@ -104,12 +207,32 @@ If ($state -eq 'present') {
     }
 
     # Set the password if required
-    If ($password -and (($new_user -and $update_password -eq "on_create") -or $update_password -eq "always")) {
-        $secure_password = ConvertTo-SecureString $password -AsPlainText -Force
-        Set-ADAccountPassword -Identity $username -Reset:$true -Confirm:$false -NewPassword $secure_password -WhatIf:$check_mode @extra_args
-        $user_obj = Get-ADUser -Identity $username -Properties * @extra_args
-        $result.password_updated = $true
-        $result.changed = $true
+    If (
+        (
+            $password
+        ) -and (
+            (
+                $new_user -and $update_password -eq "on_create"
+            ) -or (
+                $update_password -in @("always"; "when_changed")
+            )
+        )
+    ) {
+        If (-not $new_user) { # Don't uncecessary check if the credentials works, if we know it doesn't.
+            $test_new_credentials = Test-Credential -Username $username -Password $password
+        } else {
+            $test_new_credentials = $false
+        }
+        If ($test_new_credentials -and ($update_password -ne "always")) {
+            $result.password_updated = $false
+            $result.changed = $false
+        } Else {
+            $secure_password = ConvertTo-SecureString $password -AsPlainText -Force
+            Set-ADAccountPassword -Identity $username -Reset:$true -Confirm:$false -NewPassword $secure_password -WhatIf:$check_mode @extra_args
+            $user_obj = Get-ADUser -Identity $username -Properties * @extra_args
+            $result.password_updated = $true
+            $result.changed = $true
+        }
     }
 
     # Configure password policies
diff --git a/lib/ansible/modules/windows/win_domain_user.py b/lib/ansible/modules/windows/win_domain_user.py
index de88643b7c490a..01ef07c9632665 100644
--- a/lib/ansible/modules/windows/win_domain_user.py
+++ b/lib/ansible/modules/windows/win_domain_user.py
@@ -75,11 +75,9 @@
     description:
       - C(always) will update passwords if they differ.
       - C(on_create) will only set the password for newly created users.
-      - Note that C(always) will always report an Ansible status of 'changed'
-        because we cannot determine whether the new password differs from
-        the old password.
+      - C(when_changed) will only set the password when it differs.
     type: str
-    choices: [ always, on_create ]
+    choices: [ always, on_create, when_changed ]
     default: always
   password_expired:
     description:


Code review comments:
diff_hunk: @@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
I don't see a reason why we need to expose both the provider and type right now. Maybe if we make this a generic module util but for now it just complicates matters.

diff_hunk: @@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
+        $LOGON32_LOGON_TYPE = 3 # NetworkLogon
+    )
+    if (($Username.ToCharArray()) -contains [char]'@') {
+        # UserPrincipalName
+        $Username = ($Username -split '@')[0]
+        $Domain = ($Username -split '@')[1]
+    } elseif (($Username.ToCharArray()) -contains [char]'\') {
+        # Pre Win2k Account Name
+        $Username = ($Username -split '\')[0]
+        $Domain = ($Username -split '\')[1]
+    } else {
+        # No domain provided, so maybe local user?
+    }
+    $null = $Domain # Make CI-Check happy...
+
+    # More information about LogonUser at https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-logonusera
+    $platform_util = @'
+using System;
+using System.Runtime.InteropServices;
+using System.ComponentModel;
+using System.Security;
+namespace Ansible
+{
+    public class WinUserPInvoke
+    {
+        const int LOGON32_LOGON_NETWORK           = 3;
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
Really isn't a need to provide these contants, if we are always using a network logon and the default provider just pass those values as is an just add a comment.

diff_hunk: @@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
+        $LOGON32_LOGON_TYPE = 3 # NetworkLogon
+    )
+    if (($Username.ToCharArray()) -contains [char]'@') {
+        # UserPrincipalName
+        $Username = ($Username -split '@')[0]
+        $Domain = ($Username -split '@')[1]
+    } elseif (($Username.ToCharArray()) -contains [char]'\') {
+        # Pre Win2k Account Name
+        $Username = ($Username -split '\')[0]
+        $Domain = ($Username -split '\')[1]
+    } else {
+        # No domain provided, so maybe local user?
+    }
+    $null = $Domain # Make CI-Check happy...
+
+    # More information about LogonUser at https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-logonusera
+    $platform_util = @'
+using System;
+using System.Runtime.InteropServices;
+using System.ComponentModel;
+using System.Security;
+namespace Ansible
+{
+    public class WinUserPInvoke
+    {
+        const int LOGON32_LOGON_NETWORK           = 3;
+        const int LOGON32_PROVIDER_DEFAULT        = 0; // Negotiate if domain == null and username not upn else NTLM
+        const int LOGON32_PROVIDER_WINNT40        = 2; // Negotiate
+        const int LOGON32_PROVIDER_WINNT50        = 3; // NTLM
+        // Refere to: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/18d8fbe8-a967-4f1c-ae50-99ca8e491d2d
+        static int[] success_codes = new int[] {
+            0x0000052F,  // ERROR_ACCOUNT_RESTRICTION
+            0x00000530,  // ERROR_INVALID_LOGON_HOURS
+            0x00000531,  // ERROR_INVALID_WORKSTATION
+            0x00000569   // ERROR_LOGON_TYPE_GRANTED
+        };
+        static int[] failed_codes = new int[] {
+            0x0000052E,  // ERROR_LOGON_FAILURE - the user or pass was incorrect
+            0x00000006,  // ERROR_INVALID_HANDLE - the handle is invalid
+            0x00000532   // ERROR_PASSWORD_EXPIRED - The password has exired
+        };
+        [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
+        internal static extern bool LogonUser(String username, String domain, IntPtr password,
+            int logonType, int logonProvider, out IntPtr token);
+        [DllImport("kernel32.dll", SetLastError = true)]
+        internal static extern bool CloseHandle(
+            IntPtr hObject);
+        public static bool TestCredential(string userName, SecureString password, string domainName = null)
+        {
+            IntPtr tokenHandle = IntPtr.Zero;
+            IntPtr passwordPtr = IntPtr.Zero;
+            bool returnValue   = false;
+            int err_code = 0;
+
+            // Marshal the SecureString to unmanaged memory.
+            passwordPtr = Marshal.SecureStringToGlobalAllocUnicode(password);
+
+            // Pass LogonUser the decrypted copy of the password.
+            returnValue = LogonUser(userName, domainName, passwordPtr,
+                                    LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT,
+                                    out tokenHandle);
+            // Perform cleanup whether or not the call succeeded.
+            // Secure delete the plaintext password from memory.
+            Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr);
+            // We don't need the token, so close it again.
+            CloseHandle(tokenHandle);
+            if (returnValue || !(tokenHandle == IntPtr.Zero)) {
+                return true;
+            } else {
+                err_code = Marshal.GetLastWin32Error();
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
You should call this straight after `LogonUser`. If you don't you run the risk of any of the preceding calls overwriting the value and then you getting the wrong error code. Right now this should always be returning `0` as the `CloseHandle` will be succeeding.

diff_hunk: @@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
+        $LOGON32_LOGON_TYPE = 3 # NetworkLogon
+    )
+    if (($Username.ToCharArray()) -contains [char]'@') {
+        # UserPrincipalName
+        $Username = ($Username -split '@')[0]
+        $Domain = ($Username -split '@')[1]
+    } elseif (($Username.ToCharArray()) -contains [char]'\') {
+        # Pre Win2k Account Name
+        $Username = ($Username -split '\')[0]
+        $Domain = ($Username -split '\')[1]
+    } else {
+        # No domain provided, so maybe local user?
+    }
+    $null = $Domain # Make CI-Check happy...
+
+    # More information about LogonUser at https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-logonusera
+    $platform_util = @'
+using System;
+using System.Runtime.InteropServices;
+using System.ComponentModel;
+using System.Security;
+namespace Ansible
+{
+    public class WinUserPInvoke
+    {
+        const int LOGON32_LOGON_NETWORK           = 3;
+        const int LOGON32_PROVIDER_DEFAULT        = 0; // Negotiate if domain == null and username not upn else NTLM
+        const int LOGON32_PROVIDER_WINNT40        = 2; // Negotiate
+        const int LOGON32_PROVIDER_WINNT50        = 3; // NTLM
+        // Refere to: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/18d8fbe8-a967-4f1c-ae50-99ca8e491d2d
+        static int[] success_codes = new int[] {
+            0x0000052F,  // ERROR_ACCOUNT_RESTRICTION
+            0x00000530,  // ERROR_INVALID_LOGON_HOURS
+            0x00000531,  // ERROR_INVALID_WORKSTATION
+            0x00000569   // ERROR_LOGON_TYPE_GRANTED
+        };
+        static int[] failed_codes = new int[] {
+            0x0000052E,  // ERROR_LOGON_FAILURE - the user or pass was incorrect
+            0x00000006,  // ERROR_INVALID_HANDLE - the handle is invalid
+            0x00000532   // ERROR_PASSWORD_EXPIRED - The password has exired
+        };
+        [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
+        internal static extern bool LogonUser(String username, String domain, IntPtr password,
+            int logonType, int logonProvider, out IntPtr token);
+        [DllImport("kernel32.dll", SetLastError = true)]
+        internal static extern bool CloseHandle(
+            IntPtr hObject);
+        public static bool TestCredential(string userName, SecureString password, string domainName = null)
+        {
+            IntPtr tokenHandle = IntPtr.Zero;
+            IntPtr passwordPtr = IntPtr.Zero;
+            bool returnValue   = false;
+            int err_code = 0;
+
+            // Marshal the SecureString to unmanaged memory.
+            passwordPtr = Marshal.SecureStringToGlobalAllocUnicode(password);
+
+            // Pass LogonUser the decrypted copy of the password.
+            returnValue = LogonUser(userName, domainName, passwordPtr,
+                                    LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT,
+                                    out tokenHandle);
+            // Perform cleanup whether or not the call succeeded.
+            // Secure delete the plaintext password from memory.
+            Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr);
+            // We don't need the token, so close it again.
+            CloseHandle(tokenHandle);
+            if (returnValue || !(tokenHandle == IntPtr.Zero)) {
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
No need to check the handle value, just base it off the LogonUser return value.

diff_hunk: @@ -104,12 +207,32 @@ If ($state -eq 'present') {
     }
 
     # Set the password if required
-    If ($password -and (($new_user -and $update_password -eq "on_create") -or $update_password -eq "always")) {
-        $secure_password = ConvertTo-SecureString $password -AsPlainText -Force
-        Set-ADAccountPassword -Identity $username -Reset:$true -Confirm:$false -NewPassword $secure_password -WhatIf:$check_mode @extra_args
-        $user_obj = Get-ADUser -Identity $username -Properties * @extra_args
-        $result.password_updated = $true
-        $result.changed = $true
+    If (
+        (
+            $password
+        ) -and (
+            (
+                $new_user -and $update_password -eq "on_create"
+            ) -or (
+                $update_password -in @("always"; "when_changed")
+            )
+        )
+    ) {
+        If (-not $new_user) { # Don't uncecessary check if the credentials works, if we know it doesn't.
+            $test_new_credentials = Test-Credential -Username $username -Password $password
+        } else {
+            $test_new_credentials = $false
+        }
+        If ($test_new_credentials -and ($update_password -ne "always")) {
+            $result.password_updated = $false
+            $result.changed = $false
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
You shouldn't set this to `$false` as it could overwrite code above that set it to `$true`. Just rely on an implicit value of no changes and only set to `$true` if a change occurred.

diff_hunk: @@ -104,12 +207,32 @@ If ($state -eq 'present') {
     }
 
     # Set the password if required
-    If ($password -and (($new_user -and $update_password -eq "on_create") -or $update_password -eq "always")) {
-        $secure_password = ConvertTo-SecureString $password -AsPlainText -Force
-        Set-ADAccountPassword -Identity $username -Reset:$true -Confirm:$false -NewPassword $secure_password -WhatIf:$check_mode @extra_args
-        $user_obj = Get-ADUser -Identity $username -Properties * @extra_args
-        $result.password_updated = $true
-        $result.changed = $true
+    If (
+        (
+            $password
+        ) -and (
+            (
+                $new_user -and $update_password -eq "on_create"
+            ) -or (
+                $update_password -in @("always"; "when_changed")
+            )
+        )
+    ) {
+        If (-not $new_user) { # Don't uncecessary check if the credentials works, if we know it doesn't.
+            $test_new_credentials = Test-Credential -Username $username -Password $password
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
We also don't want to test the credentials if `update_password` is `on_create` and we didn't create a new user. Right now this will always check and update the password for both `on_create` and `when_changed` effectively making them the same thing. We could always make this more efficient by not testing the credentials when `update_password` is `always` as we will always update it regardless of this result.

@author:
No it won't $new_user is only true if the user was newly created https://github.com/ansible/ansible/pull/58383/files#diff-a814da53b65fce105cb5fb909b79b267R201 therefore it won't be touched.

The outer if-statement checks:
```
($password) ∧ (($new_user ∧ $update_password == "on_create") ∨ ($update_password ∈ {'always', 'when_chantged'}))
```

The inner if-statement:
```
¬($new_user)
```

So Test-Credential is executed when:
```
($password) ∧ (($new_user ∧ $update_password == "on_create") ∨ ($update_password ∈ {'always', 'when_chantged'})) ∧ ¬($new_user)
```

And for the case where `$password = 'something' ∧ update_password == 'on_create' ∧ $new_user == $true` (we just created the user) it will results in:

1. `$password` => $true
2. `(($new_user ∧ $update_password == "on_create")` => $true
3. ¬($new_user) => $false

=> So it will not be executed.

And for the case where `$password = 'something' ∧ update_password == 'on_create' ∧ $new_user == $false` (we the user already existed) it will results in:

1. `$password` => $true
2. `($new_user ∧ $update_password == "on_create")` => $false
3. `($update_password ∈ {'always', 'when_chantged'})` => $false

=> So it will not be executed either.

Maybe this if statement is too complicated, I'll consider rewriting it.

@user1:
Ahh yes I see it now sorry, we should still avoid testing the credentials when the credentials is `always` as we will be changing it anyway.

It did seem a bit confusing when I was reading through it originally so if you can simplify it then by all means go ahead.

@author:
Sometimes you can't see the forest for the trees...
It's now way easier.

diff_hunk: @@ -75,11 +75,9 @@
     description:
       - C(always) will update passwords if they differ.
path: lib/ansible/modules/windows/win_domain_user.py
text: @user1:
Looks like there is a mistake in the docs, are you able to change it to something like `C(always) will always update the password`.

diff_hunk: @@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
+        $LOGON32_LOGON_TYPE = 3 # NetworkLogon
+    )
+    if (($Username.ToCharArray()) -contains [char]'@') {
+        # UserPrincipalName
+        $Username = ($Username -split '@')[0]
+        $Domain = ($Username -split '@')[1]
+    } elseif (($Username.ToCharArray()) -contains [char]'\') {
+        # Pre Win2k Account Name
+        $Username = ($Username -split '\')[0]
+        $Domain = ($Username -split '\')[1]
+    } else {
+        # No domain provided, so maybe local user?
+    }
+    $null = $Domain # Make CI-Check happy...
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
What's the CI check, we use `$Domain` down below so I assume it's not for an unused variable.

@author:
This is the check: https://app.shippable.com/github/ansible/ansible/runs/130742/1/tests

@user1:
That doesn't make sense, you are using `$Domain` later on, will have to look into PSScriptAnalyzer to try and figure out what's happening there.

@author:
Ok, I'm going to remove this workaround.

diff_hunk: @@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
+        $LOGON32_LOGON_TYPE = 3 # NetworkLogon
+    )
+    if (($Username.ToCharArray()) -contains [char]'@') {
+        # UserPrincipalName
+        $Username = ($Username -split '@')[0]
+        $Domain = ($Username -split '@')[1]
+    } elseif (($Username.ToCharArray()) -contains [char]'\') {
+        # Pre Win2k Account Name
+        $Username = ($Username -split '\')[0]
+        $Domain = ($Username -split '\')[1]
+    } else {
+        # No domain provided, so maybe local user?
+    }
+    $null = $Domain # Make CI-Check happy...
+
+    # More information about LogonUser at https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-logonusera
+    $platform_util = @'
+using System;
+using System.Runtime.InteropServices;
+using System.ComponentModel;
+using System.Security;
+namespace Ansible
+{
+    public class WinUserPInvoke
+    {
+        const int LOGON32_LOGON_NETWORK           = 3;
+        const int LOGON32_PROVIDER_DEFAULT        = 0; // Negotiate if domain == null and username not upn else NTLM
+        const int LOGON32_PROVIDER_WINNT40        = 2; // Negotiate
+        const int LOGON32_PROVIDER_WINNT50        = 3; // NTLM
+        // Refere to: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/18d8fbe8-a967-4f1c-ae50-99ca8e491d2d
+        static int[] success_codes = new int[] {
+            0x0000052F,  // ERROR_ACCOUNT_RESTRICTION
+            0x00000530,  // ERROR_INVALID_LOGON_HOURS
+            0x00000531,  // ERROR_INVALID_WORKSTATION
+            0x00000569   // ERROR_LOGON_TYPE_GRANTED
+        };
+        static int[] failed_codes = new int[] {
+            0x0000052E,  // ERROR_LOGON_FAILURE - the user or pass was incorrect
+            0x00000006,  // ERROR_INVALID_HANDLE - the handle is invalid
+            0x00000532   // ERROR_PASSWORD_EXPIRED - The password has exired
+        };
+        [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
+        internal static extern bool LogonUser(String username, String domain, IntPtr password,
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
Handling the SecureString -> Ptr is really unecessary considering the original source is a string. It will be a lot less complicated if you just pass the password as a string and not a pointer.

@author:
This is the recommended implementation microsoft in there examples like here: https://docs.microsoft.com/de-de/dotnet/api/system.runtime.interopservices.marshal.securestringtoglobalallocunicode?view=netframework-4.8

The password will spread around memory without being cleaned up. And I don't know if even the garbage collector will pick up copies to/in unmanaged code... Right now I've my doubts that this would be secure, I don't want to accidentally leak secrets just because security was inconvenient. Can you assure me that using a string here is as secure as my current approach?

@user1:
It is the recommended implementation if the input value is a secure string, in this case the input value is a string (passing in as a string from the controller). You are manually converting to a secure string then doing the whole copying to unmanaged memory then clearing it out there later. If the original value was a SecureString and we never dealt with just a String I would agree with you here but ultimately that is not the case.

> The password will spread around memory without being cleaned up

If you let the PInvoke marshaler deal with this you don't have to worry about it at all. Having the function definition be a string will make the .NET CLR automatically convert a string to unmanaged memory and pass in a pointer itself. This is also guaranteed by the CLR to be released once the function is returned. You are just skipping that step and doing it all yourself manually.

> Right now I've my doubts that this would be secure, I don't want to accidentally leak secrets just because security was inconvenient

Right now the password is already in memory as plaintext due to the input value from the controller just being a json string that is converted. There's nothing you can do about this as Ansible has no way of copying a secure string without it being a string originally.

> Can you assure me that using a string here is as secure as my current approach?

The marshaler does this all for you and guarantees the memory is cleared once the function has returned. Even if something goes horribly wrong the process will exit and also clear out the data in memory.

In the end you are trying to make something secure that is not already secure. This can't be done with how it works right now. If you can't trust the machine you are connecting to with Ansible then how can you trust it when you've promoted it as a domain controller.

I admire what you are trying to achieve and it's great to see but really it isn't doing anything in the bigger picture and it's just making this module even harder to maintain.

@author:
> The marshaler does this all for you and guarantees the memory is cleared once the function has returned. Even if something goes horribly wrong the process will exit and also clear out the data in memory.

I did not know that, that makes it way less important to care about.

> There's nothing you can do about this as Ansible has no way of copying a secure string without it being a string originally.

Until now, but there is another Ticket to address this issue (Ok the other issue is about non pipelining connectors writing the file with the secrets to disk, but it's basically the same problem).

> In the end you are trying to make something secure that is not already secure.

I just did not want to contribute to the problem, and because I did not know about the "automated cleanup" it looked a bit more important to think about ;-)

> If you can't trust the machine you are connecting to with Ansible then how can you trust it when you've promoted it as a domain controller.

It's not about trusting the machine now, but more of trusting the machine in the future, bootstrapping the environment before handing over to the customer, but you're right, it does not quite fit in here.

> and it's just making this module even harder to maintain.

I still disagree on that one, I consider both solutions equally difficult to maintain and both have there unique pitfalls and weaknesses. But anyway, I'm switching this for a string, as you're right, it is less important than I thought it was.

diff_hunk: @@ -4,6 +4,109 @@
 
 #Requires -Module Ansible.ModuleUtils.Legacy
 
+Function Test-Credential {
+    param(
+        [String]$Username,
+        [String]$Password,
+        [String]$Domain = $null,
+        $LOGON32_PROVIDER = 0, # Default
+        $LOGON32_LOGON_TYPE = 3 # NetworkLogon
+    )
+    if (($Username.ToCharArray()) -contains [char]'@') {
+        # UserPrincipalName
+        $Username = ($Username -split '@')[0]
+        $Domain = ($Username -split '@')[1]
+    } elseif (($Username.ToCharArray()) -contains [char]'\') {
+        # Pre Win2k Account Name
+        $Username = ($Username -split '\')[0]
+        $Domain = ($Username -split '\')[1]
+    } else {
+        # No domain provided, so maybe local user?
+    }
+    $null = $Domain # Make CI-Check happy...
+
+    # More information about LogonUser at https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-logonusera
+    $platform_util = @'
+using System;
+using System.Runtime.InteropServices;
+using System.ComponentModel;
+using System.Security;
+namespace Ansible
+{
+    public class WinUserPInvoke
+    {
+        const int LOGON32_LOGON_NETWORK           = 3;
+        const int LOGON32_PROVIDER_DEFAULT        = 0; // Negotiate if domain == null and username not upn else NTLM
+        const int LOGON32_PROVIDER_WINNT40        = 2; // Negotiate
+        const int LOGON32_PROVIDER_WINNT50        = 3; // NTLM
+        // Refere to: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/18d8fbe8-a967-4f1c-ae50-99ca8e491d2d
+        static int[] success_codes = new int[] {
+            0x0000052F,  // ERROR_ACCOUNT_RESTRICTION
+            0x00000530,  // ERROR_INVALID_LOGON_HOURS
+            0x00000531,  // ERROR_INVALID_WORKSTATION
+            0x00000569   // ERROR_LOGON_TYPE_GRANTED
+        };
+        static int[] failed_codes = new int[] {
+            0x0000052E,  // ERROR_LOGON_FAILURE - the user or pass was incorrect
+            0x00000006,  // ERROR_INVALID_HANDLE - the handle is invalid
path: lib/ansible/modules/windows/win_domain_user.ps1
text: @user1:
Why this one, sounds like a problem if you are not passing in a null ptr to LogonUser which shouldn't happen as we set that?

@author:
Right, that should no longer be there. It was left in from debugging.