ADUser, ADGroup: Specify samAccountName separately

[gaelicWizard]

Details of the scenario you tried and the problem that is occurring

We have a need to set the samAccountName of a group created by an application installer which uses the group SID during installation, but which is named incompatibly with a different application. We can already set DisplayName and CommonName by specifying 'GroupName' key property as the full SID or by Distinguished Name, but there's no way to set the samAccountName which is what we actually need to set.

My use case is for ADGroup, but the same applies for ADUser and ADManagedServiceAccount.

Verbose logs showing the problem

Suggested solution to the issue

Add an optional property samAccountName (or possibly "NewGroupName") to allow setting the group name. This could alsö be useful for ADUser and ADManagedServiceAccount.

The DSC configuration that is used to reproduce the issue (as detailed as possible)

# example using Hyper-V Administrators as it's SID is predictable
Invoke-DscResource -ModuleName ActiveDirectoryDsc -Name ADGroup -Method Get -Property @{GroupName
= 'S-1-5-32-578'}

The operating system the target node is running

OsName : Microsoft Windows Server 2019 Standard
OsOperatingSystemSKU : StandardServerEdition
OsArchitecture : 64-bit
WindowsVersion : 1809
WindowsBuildLabEx : 17763.1.amd64fre.rs5_release.180914-1434
OsLanguage : en-US
OsMuiLanguages : {en-US}

Version and build of PowerShell the target node is running

PSVersion 5.1.17763.1852
PSEdition Desktop
PSCompatibleVersions {1.0, 2.0, 3.0, 4.0...}
BuildVersion 10.0.17763.1852
CLRVersion 4.0.30319.42000
WSManStackVersion 3.0
PSRemotingProtocolVersion 2.3
SerializationVersion 1.1.0.1

Version of the DSC module that was used

ActiveDirectoryDsc 6.0.1

[johlju]

Happy to review a PR that fixes this.

[gaelicWizard]

(Would it be inappropriate for me to post this same request under xPSDesiredStateConfiguration module for local accounts?)

[gaelicWizard]

The local user/group version of this seems much simpler, so I've prepared a pull request for xPSDesiredStateConfiguration first.

For ActiveDirectoryDsc, ADUser seems like the existing code mixes and matches using '-Identity' and '-SamAccountName' parameters for Get/Set. It seems that the existing code would succeed in some circumstances and fail in others, for example using anything other than SamAccountName to reference an already-existing user versus a non-existing user.

I guess first a unit test to see if specifically that case is broken, then a slight refactor not to assume UserName is SamAccountName, then add SamAccountName specifically. ...I don't know how to make unit tests...

[gaelicWizard]

Narrator: The local version was not simpler.

[X-Guardian]

Hi @gaelicWizard , a few notes on what you have submitted so far.

Any change to these resources must be non-breaking, i.e. not affecting their current use for this edge case. You must also consider the full life cycle of both the resource and the property, i.e. manage adds, changes and deletions of both the resource and any properties you add/change.

The ADGroup resource uses a mix of AD PowerShell Cmdlets and System.Security.Principal .Net Framework classes. This is to cope with group members from child domains and trusted forest domains, both one-way and two-way. Your code will need to support these and all these scenarios need exhaustively testing.

There also needs to be both unit and integration tests proving these changes are working.

[gaelicWizard]

@X-Guardian, thanks for the tips!

Could you help me understand what should be returned from Get-TargetResource? Should it include additional managed properties? I've got my wires crossed and now my brain is refusing to grok.

I'm no good at phrasing for documentation. The current resource for ADUser claims that UserName is SamAccountName, but that's only actually true (in the current resource) during new user creation. In all other cases, UserName is actually the Identity parameter to *-ADUser (or similar/equivalent for the .NET class). I've copied the description from the help for Get-ADUser's Identity parameter and changed the singular existing use of -SamAccountName to -Name, but adding the SamAccountName parameter to the resource requires phrasing what the new parameter is and...I feel like this is important as it's user-facing documentation and I'm no good at that.

As to testing, I'm only learning unit testing literally right now so forgive me if it takes me a while to get the tests right! 😃

[gaelicWizard]

I think between ADUser, xUser, and xGroup there are three entirely different implementations of how to access and manage principals. I think I've figured them out, and I think I learned some unit testing too! I did my best with the parameter help descriptions.

I see that xGroup was refactored recently. Would it be helpful if I (separately) updated xUser to have a similar implementation? I expect some code could be moved to a helper module.

[johlju]

Could you help me understand what should be returned from Get-TargetResource?

Get-TargetResource should return all properties in the schema MOF.

...but adding the SamAccountName parameter to the resource requires phrasing what the new parameter is and...I feel like this is important as it's user-facing documentation and I'm no good at that.

Just write how you would want it to say and it is probably be good 🙂 During review if it is not clear the reviewer and you can bounce ideas how to make it better. But there should be something to go from at least. 🙂

I see that xGroup was refactored recently. Would it be helpful if I (separately) updated xUser to have a similar implementation? I expect some code could be moved to a helper module.

Those resources are in a different module, please create an issue in that repo for those discussions.

[johlju]

As to testing, I'm only learning unit testing literally right now so forgive me if it takes me a while to get the tests right!

There is no hurry at all. Take your time. Personally I was happy to learn to be able to make tests, that is something I had used for a lot since then. Keep up the good work! 😃

[gaelicWizard]

I split this up in to multiple pull requests so it's easier to review, and I'll open a separate issue under xPSDesiredStateConfiguration for that stuff. I kinda didn't expect there to be so many little bits to deal with here! Fun! 😃