[Bug] Tasks using login module keeps returning changed true even when login already present

[diegotercero]

Describe the bug
I'm using lowlydba.sqlserver.login module version 2.3.3
The return value of changed is always set to true even when the user in the server already exists.

To Reproduce

    - name: Create a login for user
      lowlydba.sqlserver.login:
        sql_instance: '{{ mssql_instance }}'
        sql_username: '{{ mssql_sa_login }}'
        sql_password: '{{ mssql_sa_password }}'
        login: '{{ mssql_user_login }}'
        password: '{{ mssql_user_password }}'
        state: present

Expected behavior
If I run this taks when the user in the database doesn't exist, the return value of changed should be true, which it is.
If I run when the user in the database already exists, the return value of changed should be false.
Which is not the case for me, I keep getting a changed return value.

Versions:

• OS: Windows Server 2019 WSL1 on controller node / Windows Server 2019 on target node.
• SQL Server: 2019
• PowerShell: 5.1
• lowlydba.sqlserver: 2.3.3

Additional context
Ansible output given : (I replaced my actuals hosts, instance names and user logins values with replacement names).
For information I later used a PowerShell task using win_powershell and invoking dbatools cmdlets to add this users to the sysadmin server role (that's why in the output the user appears as a member of the sysadmin role).

TASK [Create a login for Open Media Edito user] **************************************************************************************************************************************
task path: /home/tercero/dev/infrastructure/playbooks/omserverdb.yml:22
Using module file /home/tercero/.ansible/collections/ansible_collections/lowlydba/sqlserver/plugins/modules/login.ps1
Pipelining is enabled.
<myserverhost.com> ESTABLISH WINRM CONNECTION FOR USER:  on PORT 5985 TO myserverhost.com
EXEC (via pipeline wrapper)
changed: [myserverhost.com] => {
    "changed": true,
    "data": {
        "ComputerName": "COMPUTERNAME",
        "DefaultDatabase": "master",
        "DenyLogin": false,
        "InstanceName": "INSTANCENAME",
        "IsDisabled": false,
        "IsLocked": false,
        "Language": "Français",
        "MustChangePassword": false,
        "Name": "SQL_USER",
        "Notes": null,
        "PasswordChanged": true,
        "PasswordExpirationEnabled": false,
        "PasswordPolicyEnforced": false,
        "ServerRole": "sysadmin",
        "SqlInstance": "SERVER\\INSTANCE"
    },
    "invocation": {
        "module_args": {
            "default_database": null,
            "enabled": true,
            "language": null,
            "login": "SQL_USER",
            "password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "password_expiration_enabled": null,
            "password_must_change": null,
            "password_policy_enforced": null,
            "sid": null,
            "skip_password_reset": false,
            "sql_instance": "SERVER\\INSTANCE",
            "sql_password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "sql_username": "SQL_SA_USERNAME",
            "state": "present"
        }
    }
}
Read vars_file '../group_vars/all/vault'

[lowlydba]

This is an intentional thing right now, since it isn't easy to detect if we've actually changed a login's password or not. If you have a suggestion for a better implementation, I would accept a PR for it.

[lowlydba]

I'm going to close this in the mean time since the functionality is intended.

[diegotercero]

Sorry for the delay in my own reply. I do realize now that dealing with passwords and idempotency is tricky. I moved on to some other topics and haven't got the time to play around with DBAtools, but I'm curious about the return value PasswordChanged that we can see in the output log that I posted. I suspect that this flag return value should be "false" if i run the task again with the same login/password that I first used upon creation. I'll try to test this tomorrow.

And this is off topic about current issue. But THANKS a lot for this sqlserver Ansible collection. Your "nonquery" module has helped me SO much!

[lowlydba]

Hm, sorry I may have missed the "PasswordChanged" initially. I want to say that that field wasn't present in DBATools when I first wrote this, but if they're already handling that upstream then it should be easy to use that data versus implementing the logic in this collection.

I'll take another look.

[lowlydba]

It seems upstream they also can only infer if the pass has changed, so I don't think using the PassswordChanged value will actually be a different functionality:

https://github.com/dataplat/dbatools/blob/918182a32f96c20ee9fa28a7a6ede2a48eba233f/public/Set-DbaLogin.ps1#L432-L433

A workaround may be to "create" the login with a task in check mode first, then depending on if it exists or not, followup with a task to create the login (including password) or modify the login (not including password, or using skip_reset).

Or, another route would be to use a nonquery with T-SQL to ensure the login simply exists & has the desired password, then perform all the other config using a login task, removing the need to specify the password to the task.

That may be as close as we can get unfortunately.