xGroup: Refactored Principal Resolution Functions for Enhanced Virtual Account Support #764

kenny-scelfo · 2023-11-06T18:21:08Z

Pull Request (PR) description

Enhanced Test-IsLocalMachine to recognize 'IIS APPPOOL' and 'NT VIRTUAL MACHINE' as local machine scopes, improving virtual account type coverage.
Updated Split-MemberName to preserve original scope, allowing local accounts that do not use the computer name to be resolved.
Extended Find-Principal with a new Scope parameter, enabling searches additional local machine scopes.
Augmented Find-Principal to handle a predefined list of NT account scopes ('NT Authority', 'NT Service', 'IIS APPPOOL', 'NT Virtual Machine'), utilizing System.Security.Principal.NTAccount for SID resolution. This enhancement focuses on local virtual account identification.
Implemented new integration tests for ConvertTo-Principal to verify local, virtual account resolution.
Introduced Get-ScopeFromDistinguishedName to accurately parse scope from distinguished names, addressing issues with multiple domain components and escaped commas.
Added unit tests for Get-ScopeFromDistinguishedName, ensuring functional correctness.
Adjusted outside domain test to expect 'domain.com' instead of 'domain'.

This Pull Request (PR) fixes the following issues

Task list

Added an entry to the change log under the Unreleased section of the
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

Support - Enhanced Test-IsLocalMachine to recognize 'IIS APPPOOL' and 'NT VIRTUAL MACHINE' as local machine scopes, improving virtual account type coverage. - Updated Split-MemberName to preserve original scope, allowing local accounts that do not use the computer name to be resolved. - Extended Find-Principal with a new Scope parameter, enabling searches additional local machine scopes. - Augmented Find-Principal to handle a predefined list of NT account scopes ('NT Authority', 'NT Service', 'IIS APPPOOL', 'NT Virtual Machine'), utilizing System.Security.Principal.NTAccount for SID resolution. This enhancement focuses on local virtual account identification. - Implemented new integration tests for ConvertTo-Principal to verify local, virtual account resolution. - Introduced Get-ScopeFromDistinguishedName to accurately parse scope from distinguished names, addressing issues with multiple domain components and escaped commas. - Added unit tests for Get-ScopeFromDistinguishedName, ensuring functional correctness. - Adjusted outside domain test to expect 'domain.com' instead of 'domain'.

mock call count.

Test-Path mock to account for the short-circuiting of the if conditions on line 449 of DSC_xDSCWebService.psm1 preventing the third condition from being evaluated.

codecov · 2023-11-07T16:11:05Z

Codecov Report

Merging #764 (c7261bb) into main (475885c) will decrease coverage by 1%.
Report is 9 commits behind head on main.
The diff coverage is 78%.

@@         Coverage Diff         @@
##           main   #764   +/-   ##
===================================
- Coverage    72%    72%   -1%     
===================================
  Files        30     30           
  Lines      4427   4449   +22     
===================================
+ Hits       3213   3228   +15     
- Misses     1214   1221    +7

Files	Coverage Δ
...sources/DSC_xDSCWebService/DSC_xDSCWebService.psm1	`88% <ø> (ø)`
...xEnvironmentResource/DSC_xEnvironmentResource.psm1	`91% <100%> (ø)`
...sionConfiguration/DSC_xPSSessionConfiguration.psm1	`0% <ø> (ø)`
...ces/DSC_xPackageResource/DSC_xPackageResource.psm1	`80% <ø> (ø)`
...urces/DSC_xScriptResource/DSC_xScriptResource.psm1	`100% <ø> (ø)`
...ces/DSC_xServiceResource/DSC_xServiceResource.psm1	`93% <ø> (ø)`
...Resources/DSC_xUserResource/DSC_xUserResource.psm1	`53% <ø> (ø)`
...urces/DSC_xWindowsFeature/DSC_xWindowsFeature.psm1	`86% <ø> (ø)`
.../xWindowsFeatureSet/xWindowsFeatureSet.schema.psm1	`0% <ø> (ø)`
....PSWSIIS/xPSDesiredStateConfiguration.PSWSIIS.psm1	`38% <100%> (+<1%)`	⬆️
... and 1 more

PlagueHO

Great stuff @kenny-scelfo - thanks for submitting this. Just some minor bits and will merge.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @kenny-scelfo)

CHANGELOG.md line 9 at r1 (raw file):

- xGroup
  - Fixed a bug where the resource would fail if any of the group members were

Do these fix xGroupResource Throws Exceptions when Using Multiple Local Virtual Account Types · Issue #763 · dsccommunity/xPSDesiredStateConfiguration (github.com)? If so, can we referrence the issue with - Fixes [Issue #763](https://github.com/dsccommunity/xPSDesiredStateConfiguration/issues/763)?

source/DSCResources/DSC_xGroupResource/DSC_xGroupResource.psm1 line 2258 at r1 (raw file):

    Specifies the distinguished name string from which the scope is to be extracted.
.EXAMPLE
    Get-ScopeFromDistinguishedName -DistinguishedName "CN=John Doe,OU=Users,DC=example,DC=com"

Can we include an example with an unescaped string shows (e.g., like in the unit tests - this helps explain why the function is implemented as a loop over the char array rather than a -split).

source/DSCResources/DSC_xGroupResource/DSC_xGroupResource.psm1 line 2270 at r1 (raw file):

    $domainComponents = @()
    $escaped = $false
    $currentComponent = ""

Can we change to single quotes?

source/DSCResources/DSC_xGroupResource/DSC_xGroupResource.psm1 line 2430 at r1 (raw file):

        [Parameter(Mandatory = $true)]
        [string]

Can we use [System.String]?

source/DSCResources/DSC_xGroupResource/DSC_xGroupResource.psm1 line 2447 at r1 (raw file):

    {
        $fullIdentityValue = "$Scope\$IdentityValue"
        $ntAccount = New-Object System.Security.Principal.NTAccount($fullIdentityValue)

Can you add the parameter name here? e.g. New-Object -TypeName ...?

source/DSCResources/DSC_xGroupResource/DSC_xGroupResource.psm1 line 2450 at r1 (raw file):

        $sid = $ntAccount.Translate([System.Security.Principal.SecurityIdentifier]).Value
        return [System.DirectoryServices.AccountManagement.Principal]::FindByIdentity($PrincipalContext, [System.DirectoryServices.AccountManagement.IdentityType]::Sid, $sid)
    }

Can you add a blank line after this one?

tests/Unit/DSC_xGroupResource.Tests.ps1 line 321 at r1 (raw file):

            Context 'xGroupResource\Get-ScopeFromDistinguishedName' {

Nit: can you remove this blank line?

tests/Unit/DSC_xGroupResource.Tests.ps1 line 322 at r1 (raw file):

            Context 'xGroupResource\Get-ScopeFromDistinguishedName' {

                It 'when given a valid distinguished name it returns the correct domain name' {

Nit: to help make the test output more readable, can you begin each It description with a "Should". E.g., Should return the correct domain name when given a valid distinguished name - and throughout the remaining tests.

review process

kenny-scelfo

Thanks for the quick review.

Reviewable status: 2 of 5 files reviewed, 6 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md line 9 at r1 (raw file):