xGroup not modifying group anymore

[wickedave]

I've tried to modify the administrators groups on a server with xGroup. Members are a mix of local and domain group and I'm receiving the error:
Exception calling "Save" with "0" argument(s): "Cannot perform this operation on built-in accounts.
If I put only domain groups, I don't have the error but the Administrators group didn't get modified.

[PlagueHO]

Hi @wickedave - which version of the xPSDesiredStateConfiguration resource are you using?

[wickedave]

Hi @PlagueHO,

I've tried the release 3.11 and 3.12. xGroup was working fine on version 3.10

[PlagueHO]

I'll see if I can replicate this tonight. Are you able to post a snippet of the config that is causing the failure? Thank you!

[wickedave]

You can see here. I've simplified it with only 1 domain group and 1 local user but in my real case, I have multiple domain group.

configuration TestGroup
{
param
(
[string]$NodeName,

        [Parameter(Mandatory)]
        [ValidateNotNullOrEmpty()]
        [pscredential] $credential
    ) 

 Import-DSCResource -ModuleName xPSDesiredStateConfiguration

 Node $NodeName 
    {
    $node.PSDscAllowDomainUser  = $true
    $node.PSDscAllowPlainTextPassword = $true

    xGroup Admins
        {
        GroupName = "Administrators"
        Members = "Domain\MyGroup","Administrator"
        Credential = $credential
        }
    }
}

[pscredential]$cred = Get-Credential

TestGroup -NodeName MyServer -Credential $cred -OutputPath c:\Configs\TestGroup

Start-DscConfiguration C:\Configs\TestGroup -Wait -Verbose -ComputerName MyServer -force

[PlagueHO]

Sorry about the delay. Looking into this now.

[PlagueHO]

Quick update: I've managed to replicate the problem. There are actually some other bugs in the current Dev branch that have to be resolved before the bug you're seeing actually shows up.

I think what is happening is that the Administrator principal is being mistakenly removed from the Administrators local group (which is not allowed to happen). I've got to do some further investigation on this.

The error is occurring on this line:
https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/dev/DSCResources/MSFT_xGroupResource/MSFT_xGroupResource.psm1#L523

[PlagueHO]

Update: I've found the cause of the problem. It is caused by this line:
https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/dev/DSCResources/MSFT_xGroupResource/MSFT_xGroupResource.psm1#L438

This clears the content of a group before adding the members listed in the Members parameter. This causes an error for built-in groups because it tries to delete the Administrator account from it. I can see in the older version of this resource it didn't remove all members and rebuild the group anytime it needed to be updated.

@kwirkykat - it looks to me like the newer code isn't going to work in this situation. The only solution can see is to revert to the older design pattern (although I much prefer your newer code as it's much cleaner). What are your thoughts?

That said, I think we've reached a point where any further changes without full unit test and integration test coverage on this resource is just too risky.

Also, in the process of investigating this issue I ran into at least 6 other minor (but still significant) bugs. The resource also violates a few of the style guidelines. So there really is quite a bit of work involved in bringing this resource up to HQRM levels (I'm only talking about xGroup here). I imagine the other issues that have been raised with this xGroup will also be fixed by this process.

IMHO we need to completely fix up xGroup in one go, including full Unit Tests and Integration tests as well as fixing all outstanding bugs and making it meet the styleguidelines. I'm OK to put the work in to get this up to scratch over the next couple of weeks but if you peeps are already working on it then I won't.

[Zuldan]

@PlagueHO, thank you for spending so much of your personal on adding to and fixing DSC resources. I was planning on upgrading our production to 3.12.0.0 but I think I'll wait until you've done your magic and fixed up xGroup.

[PlagueHO]

Hi @Zuldan! As always, it's a pleasure contributing to the community 😄 - but I think you're probably best using 3.10 or earlier for the moment. Although you can still use this resource to manage built-in groups if you just use the IncludeMembers and ExcludeMembers parameters to control the group members. It only becomes a problem when using the Members parameter to set the content of Built-in groups that have "required" members.

@kwirkykat - If it is OK with you I'll put together the unit/integration tests for this resource over the weekend (as well as fixing the bugs) - unless you're working on this in parallel?

[kwirkykat]

Sorry. Was out for a funeral.

@PlagueHO I can fix this line to only remove members that are not specified in Members rather than clearing all the members in the group. Are you already working on this?

Style issues will be fixed later as part of #160. Haven't gotten to style yet.

Can you file issues for the other bugs you found? Or did you already?

[PlagueHO]

Hi @kwirkykat. I'm so sorry to hear that! My condolences.

I can submit a PR for the issues I've fixed. I haven't worked on the actual bug above yet so you can fix that one if you like?

My main thought is that we really need unit/integration tests in place so we can raise our levels of confidence. I'm happy to put these together if you're not already working on them? I thought I recalled you may have been working on them. Didn't want to duplicate effort.

So to sum up:

• I'll submit pr with minor fixes
• you can submit pr to fix issue with clearing builtin groups before setting members
• I'll work on unit/integration tests

That okay with you?

[kwirkykat]

@PlagueHO Sounds good. I would love more tests! I'm not on that part of #160 yet. Currently working on porting the Process resource.

I added a bunch of tests for xGroup here. The tests with domain credentials have to be skipped currently though because we don't have domains set up on the AppVeyor machines and we wouldn't know what your credentials are if you ran them locally. I didn't want to remove them though in case we could somehow get testing with domains working later.

[kwirkykat]

Fixed by #182