xADUser: Fix CN/Path Concurrent Change and Empty String Property on Creation Exceptions #412

X-Guardian · 2019-07-02T22:50:19Z

Pull Request (PR) description

This PR makes the following changes to the xADUser resource:

Adds an additional Null/Empty parameter & property check to prevent the exception when creating a User with an empty string property.
Relocates the user move/rename code to after the Set-ADUser code and performs the Move-ADObject before the Rename-ADObject to prevent the exception when updating CommonName and Path at the same time.

This Pull Request (PR) fixes the following issues

Fixes xADUser: updating CommonName and Path at same time on existing object fails #402
Fixes xADUser: Exception When Creating a User with an Empty String Property #407

Task list

Added an entry under the Unreleased section in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in resource directory README.md.
Resource parameter descriptions added/updated in 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 Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

codecov-io · 2019-07-02T22:59:32Z

Codecov Report

Merging #412 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #412    +/-   ##
====================================
+ Coverage    92%    92%   +<1%     
====================================
  Files        20     20            
  Lines      2541   2549     +8     
  Branches     10     10            
====================================
+ Hits       2348   2356     +8     
  Misses      183    183            
  Partials     10     10

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1

johlju · 2019-07-03T07:19:40Z

Was testing GitHub review tool again, just to see how far it gotten with the latest changes... sadly still to far away from Reviewable in my opinion. But I like the suggestion thing, see above. Maybe that works in reviewable to since it is just a code block with suggestion as the block type. 🤔 Update: It did not work from Reviewable. 😄

I will review in Reviewable now 🙂

johlju

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @X-Guardian)

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1546 at r2 (raw file):

Quoted 9 lines of code…

                elseif (([System.String]::IsNullOrEmpty($PSBoundParameters.$parameter)) -and ([System.String]::IsNullOrEmpty($targetResource.$parameter)))
                {
                    # Both values are null/empty and therefore we are compliant
                }
                # Use Compare-Object to allow comparison of string and array parameters
                elseif (($null -ne $PSBoundParameters.$parameter -and $null -eq $targetResource.$parameter) -or
                        ($null -eq $PSBoundParameters.$parameter -and $null -ne $targetResource.$parameter) -or
                        (Compare-Object -ReferenceObject $PSBoundParameters.$parameter -DifferenceObject $targetResource.$parameter))
                {

Instead of an empty elseif-block could we put these two evaluation together? Or maybe make a helper function to make such a comaprision? Would the helper function Test-DscPropertyState work - or extended to work without breaking anything else? 🤔

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1621 at r2 (raw file):

Quoted 5 lines of code…

            # Cannot move users by updating the DistinguishedName property
            $adCommonParameters = Get-ADCommonParameters @PSBoundParameters

            # Using the SamAccountName for identity with Move-ADObject does not work, use the DN instead
            $adCommonParameters['Identity'] = $targetResource.DistinguishedName

These are duplicate below, could we add an if-block around these two if-blocks so we only need to specify these rows once?

Code block below is an example what I thought of, but also a test to use suggestion to see what happens in GitHub.

        if ($moveUserRequired -or $renameUserRequired)
        {
            # Cannot move users by updating the DistinguishedName property
            $adCommonParameters = Get-ADCommonParameters @PSBoundParameters

            # Using the SamAccountName for identity with Move-ADObject does not work, use the DN instead
            $adCommonParameters['Identity'] = $targetResource.DistinguishedName

            if ($moveUserRequired)
            {
                Write-Verbose -Message ($script:localizedData.MovingADUser -f $targetResource.Path, $PSBoundParameters.Path)

                Move-ADObject @adCommonParameters -TargetPath $PSBoundParameters.Path

                # Set new target resource DN in case a rename is also required
                $targetResource.DistinguishedName = "cn=$($targetResource.CommonName),$($PSBoundParameters.Path)"
            }

            if ($renameUserRequired)
            {
                Write-Verbose -Message ($script:localizedData.RenamingADUser -f $targetResource.CommonName, $PSBoundParameters.CommonName)

                Rename-ADObject @adCommonParameters -NewName $PSBoundParameters.CommonName
            }
        }

X-Guardian

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1504 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                    $moveUserRequired = $true

Done.

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1546 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                elseif (([System.String]::IsNullOrEmpty($PSBoundParameters.$parameter)) -and ([System.String]::IsNullOrEmpty($targetResource.$parameter)))
                {
                    # Both values are null/empty and therefore we are compliant
                }
                # Use Compare-Object to allow comparison of string and array parameters
                elseif (($null -ne $PSBoundParameters.$parameter -and $null -eq $targetResource.$parameter) -or
                        ($null -eq $PSBoundParameters.$parameter -and $null -ne $targetResource.$parameter) -or
                        (Compare-Object -ReferenceObject $PSBoundParameters.$parameter -DifferenceObject $targetResource.$parameter))
                {

Instead of an empty elseif-block could we put these two evaluation together? Or maybe make a helper function to make such a comaprision? Would the helper function Test-DscPropertyState work - or extended to work without breaking anything else? 🤔

No, that's the problem. The Compare-Object function can't take a Null value for its Reference or Difference objects. I looked at that helper function and it has the same problem, it would break given a Null array parameter. There is so much other code in it too, I don't want to touch it.
This is now the same code pattern as is in Test-TargetResource.

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1621 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

            # Cannot move users by updating the DistinguishedName property
            $adCommonParameters = Get-ADCommonParameters @PSBoundParameters

            # Using the SamAccountName for identity with Move-ADObject does not work, use the DN instead
            $adCommonParameters['Identity'] = $targetResource.DistinguishedName

These are duplicate below, could we add an if-block around these two if-blocks so we only need to specify these rows once?

Code block below is an example what I thought of, but also a test to use suggestion to see what happens in GitHub.

        if ($moveUserRequired -or $renameUserRequired)
        {
            # Cannot move users by updating the DistinguishedName property
            $adCommonParameters = Get-ADCommonParameters @PSBoundParameters

            # Using the SamAccountName for identity with Move-ADObject does not work, use the DN instead
            $adCommonParameters['Identity'] = $targetResource.DistinguishedName

            if ($moveUserRequired)
            {
                Write-Verbose -Message ($script:localizedData.MovingADUser -f $targetResource.Path, $PSBoundParameters.Path)

                Move-ADObject @adCommonParameters -TargetPath $PSBoundParameters.Path

                # Set new target resource DN in case a rename is also required
                $targetResource.DistinguishedName = "cn=$($targetResource.CommonName),$($PSBoundParameters.Path)"
            }

            if ($renameUserRequired)
            {
                Write-Verbose -Message ($script:localizedData.RenamingADUser -f $targetResource.CommonName, $PSBoundParameters.CommonName)

                Rename-ADObject @adCommonParameters -NewName $PSBoundParameters.CommonName
            }
        }

These are parameter sets being built to call two different functions, so shouldn't be shared. I've adjusted the variable names to make that clearer.

johlju

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @X-Guardian)

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1546 at r2 (raw file):

Previously, X-Guardian (Simon Heather) wrote…

No, that's the problem. The Compare-Object function can't take a Null value for its Reference or Difference objects. I looked at that helper function and it has the same problem, it would break given a Null array parameter. There is so much other code in it too, I don't want to touch it.
This is now the same code pattern as is in Test-TargetResource.

I understand. Could we add this to the comment to better explain the design choice?

X-Guardian

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1546 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I understand. Could we add this to the comment to better explain the design choice?

Done. Added to the comments in both the Test-TargetResource and Set-TargetResource functions.

johlju

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @X-Guardian)

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1067 at r5 (raw file):

                {
                    # Both values are null/empty and therefore we are compliant
                    # Must catch this scenario separately, as Compare-Object can't compare Null objects

Can we both rows to a comment block? Same at the line 1545.

<#
    Text...
#>

X-Guardian

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @johlju)

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 1067 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we both rows to a comment block? Same at the line 1545.
<#
    Text...
#>

Done.

johlju

Reviewed 2 of 2 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

X-Guardian added 3 commits July 2, 2019 23:19

Empty String fix

b58b470

Relocate Move/Rename user code to after Set-ADUser

6631f07

Identity fix

3b04ae1

Update CHANGELOG

521d192

X-Guardian marked this pull request as ready for review July 2, 2019 23:04

johlju added the needs review The pull request needs a code review. label Jul 3, 2019

johlju reviewed Jul 3, 2019

View reviewed changes

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1 Outdated Show resolved Hide resolved

johlju requested changes Jul 3, 2019

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 3, 2019

X-Guardian commented Jul 3, 2019

View reviewed changes

X-Guardian and others added 2 commits July 3, 2019 19:32

review updates

8d3e607

Merge branch 'dev' into xADUser-Empty-String-Fix

aea4a0f

johlju requested changes Jul 3, 2019

View reviewed changes

Add Compare-Object comments

2c39286

X-Guardian commented Jul 3, 2019

View reviewed changes

johlju requested changes Jul 5, 2019

View reviewed changes

Update MSFT_xADUser.psm1

45f5458

X-Guardian commented Jul 5, 2019

View reviewed changes

Merge branch 'dev' into xADUser-Empty-String-Fix

5ea4735

johlju approved these changes Jul 5, 2019

View reviewed changes

johlju merged commit f81e3b7 into dsccommunity:dev Jul 5, 2019

johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 5, 2019

X-Guardian deleted the xADUser-Empty-String-Fix branch July 5, 2019 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xADUser: Fix CN/Path Concurrent Change and Empty String Property on Creation Exceptions #412

xADUser: Fix CN/Path Concurrent Change and Empty String Property on Creation Exceptions #412

X-Guardian commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited

Loading

johlju commented Jul 3, 2019 •

edited

Loading

johlju left a comment

X-Guardian left a comment

johlju left a comment

X-Guardian left a comment

johlju left a comment

X-Guardian left a comment

johlju left a comment

xADUser: Fix CN/Path Concurrent Change and Empty String Property on Creation Exceptions #412

xADUser: Fix CN/Path Concurrent Change and Empty String Property on Creation Exceptions #412

Conversation

X-Guardian commented Jul 2, 2019 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov-io commented Jul 2, 2019 • edited Loading

Codecov Report

johlju commented Jul 3, 2019 • edited Loading

johlju left a comment

Choose a reason for hiding this comment

X-Guardian left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

X-Guardian left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

X-Guardian left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

X-Guardian commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited

Loading

johlju commented Jul 3, 2019 •

edited

Loading