win_user sid support (fix #153)

[jantari]

SUMMARY

As explained in #153 I wanted to enable win_user to take group SIDs in the groups list.

The two new functions in this PR are fairly self-explanatory I think so I'm going to focus my explaining on the meat of the change, this code block:

    $ADSI.Children | Where-Object {
        $_.SchemaClassName -eq 'Group'
    } | Where-Object {
        $_.Name -eq $Name -or (
            # SID matching logic
            $binarySid -and ($_.objectSid | Foreach-Object { [System.Linq.Enumerable]::SequenceEqual([byte[]]$binarysid, [byte[]]$_) }) -contains $true
        )
    }

Design decisions here were:

• Match on $_.Name first and return immediately if a match is found so that behavior does not change for existing code
• Only if $_.Name does not find a match is the second part of the -or operator evaluated
• The current behavior of the Name-based comparison is to return $true when any of the Names inside the $_.Name array of the ADSI-object (yes, .Name is an array) match. That behavior was kept for the SID comparison. Pseudo-code:

IF $SidInputByUser IS EQUAL TO any of the SIDs in the array of the ADSI-object THEN return TRUE

• The current behavior of the Name-based comparison is to return nothing when no Group was matched. This also stays the same.

Fixes #153

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

ansible.windows.win_user

ADDITIONAL INFORMATION

Before change, trying to create a user and add them to the 'Users' group on a german system:

fatal: [172.30.147.238]: FAILED! => changed=false
  msg: group 'Users' not found

Before change, trying to create a user and add them to the 'Users' group on a german system by using the SID:

fatal: [172.30.147.238]: FAILED! => changed=false
  msg: group 'S-1-5-32-545' not found

After the change, using SIDs like a sane person:

changed: [172.30.147.238]

The PR is open for discussion and suggested changes are welcome. I am personally not entirely happy with the ugly comparison login in line 112 but wanted to provide a solution to go along with my issue that we can improve on.

[jborean93]

What you can actually do is change the elements of the groups option to sid

ansible.windows/plugins/modules/win_user.ps1

Line 15 in 937687a

groups = @{ type = 'list'; elements = 'str' }

. Ansible will parse the raw string input to a SecurityIdentifier according to these rules https://github.com/ansible/ansible/blob/0a60e5e341f9a781c27462e047ebe0f73129f4a1/lib/ansible/module_utils/csharp/Ansible.Basic.cs#L591-L603. Now that you have a list of groups as a SID you can just convert those entries to a byte array and do the comparison on the $_.objectSid for all cases. No need to add any special code to win_user to convert the input yourself if it's already available.

[jantari]

Thank you, I looked into this today and started working on it but I have a question:
Since all groups would be converted to SIDs and the module would work with data of the type [System.Security.Principal.SecurityIdentifier] internally then, should all of the diff-calculation from

ansible.windows/plugins/modules/win_user.ps1

Line 310 in 937687a

if ($null -ne $groups) {

onward also be done based on either binary or string-form SIDs or should I convert back to a group name beforehand and continue to do all of that logic based on strings/Names like it is now?

[jborean93]

I think it will be easiest to do the diff calculation with the string-form SIDs as you can continue to use the LINQ filtering logic when they are strings compared to an actual SecurityIdentifier or byte array values of the SID.

One thing we do want to be careful of is for the diff and return output to still be the human readable names and not the SID. That should be simple enough to convert though

[jborean93]

I've just opened #191 which takes a slightly different route to support human readable names in the diff and error messages.

[jborean93]

Closing due to this and other features being implemented with #191.