Implement ad object restore #217

nyanhp · 2018-07-27T15:18:21Z

Implemented restoring groups, users, computers and OUs from recycle bin. The resource will throw an error when the restoration cannot be completed due to the container not existing.
Using the new feature is opt-in and requires using the new parameter RestoreFromRecycleBin.

Fixes #211

This change is

codecov-io · 2018-07-27T15:22:19Z

Codecov Report

Merging #217 into dev will increase coverage by <1%.
The diff coverage is 92%.

@@         Coverage Diff          @@
##            dev   #217    +/-   ##
====================================
+ Coverage    81%    82%   +<1%     
====================================
  Files        14     14            
  Lines      1240   1284    +44     
  Branches     10     10            
====================================
+ Hits       1016   1059    +43     
- Misses      214    215     +1     
  Partials     10     10

johlju

Great work on this one! 😃 Out of time today, got some of the tests left to review.

Reviewed 14 of 20 files at r1.
Reviewable status: 14 of 20 files reviewed, 50 unresolved discussions (waiting on @nyanhp)

README.md, line 82 at r1 (raw file):

Indicates whether or not the computer should be restored instead of created if possible.

We should say that it is first restored if it exist, if it does not exist, then it is created. So maybe this:

Indicates whether or not the computer object should first tried to be restored from the recycle bin before creating a new computer object.

Throughout the parameter descriptions.

README.md, line 324 at r1 (raw file):

  * Removed duplicated code from examples in README.md ([issue #198](https://github.com/PowerShell/xActiveDirectory/issues/198)). [thequietman44 (@thequietman44)](https://github.com/thequietman44)
  * xADDomain is now capable of setting the forest and domain functional level ([issue #187](https://github.com/PowerShell/xActiveDirectory/issues/187)). [Jan-Hendrik Peters (@nyanhp)](https://github.com/nyanhp)
  * xADComputer, xADGroup, xADOrganizationalUnit and xADUser now support restoring from AD recycle bin ([Issue #221](https://github.com/PowerShell/xActiveDirectory/issues/211)). [Jan-Hendrik Peters (@nyanhp)](https://github.com/nyanhp)

Please rebase this PR to get the latest changes from the last release. Make sure to move this entry back under the unreleased section after rebase.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 12 at r1 (raw file):

RestoreFailed

Can we rename this to RecycleBinRestoreFailedto distinguish it better?

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 12 at r1 (raw file):

Restoring {0} ({1}) failed. {2}.

Maybe we can make this a bit more explanatory?

Restoring {0} ({1}) from the recycle bin failed. Error message: {2}.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 22 at r1 (raw file):

in recycle bin

Maybe add 'the'?

...in the recycle bin...

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 22 at r1 (raw file):

matching filter

Maybe add 'the'?

...matching the filter...

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 23 at r1 (raw file):

FoundRestoreTarget

Can we rename this to FoundRestoreTargetInRecycleBinto distinguish it better?

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 23 at r1 (raw file):

in recycle bin

Maybe add 'the'?

...in the recycle bin...

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 24 at r1 (raw file):

RestoreOk

Can we rename this to RecycleBinRestoreSuccessfulto distinguish it better?

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 24 at r1 (raw file):

Successfully restored object {0} ({1}).

Maybe we can make this a bit more explanatory?

Successfully restored object {0} ({1}) from the recycle bin.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 727 at r1 (raw file):

        [Parameter()]
        [switch]
        $PassThru

Instead of this, can we make this helper function always return a return value. Either the object that was restored, or $null if no object was restored?
See next comment for Restore-ADObject below too.

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 733 at r1 (raw file):

    Write-Verbose -Message ($localizedString.FindInRecycleBin -f $restoreFilter)

    # Using IsDeleted and IncludeDeletedObjects will mean that the cmdlet does not throw

Please make this a comment-block

<#
    Row1...
    Row2...
#>

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 735 at r1 (raw file):

$restoreParams

Could we rename this to $getAdObjectParameters?

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 737 at r1 (raw file):

-Filter $restoreFilter -IncludeDeletedObjects

Can we add these two, to the $getAdObjectParameters so we splat all the parameters? (See previous comment to about the rename).

DSCResources/MSFT_xADCommon/MSFT_xADCommon.psm1, line 745 at r1 (raw file):

        try
        {
            $restorableObject | Restore-ADObject @restoreParams -ErrorAction Stop -PassThru:$PassThru

Could we make this clearer that we are returning the object here with a return statement, preferably after the try-block, so that we return $null if no object was restored? Would that work with the intended behavior?

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 94 at r1 (raw file):

        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We don't have to add the non-mandatory parameters in the Get-TargetResource function. We can remove it here since it is not used.

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 232 at r1 (raw file):

        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We should have the parameter variable name on a separate row.

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 355 at r1 (raw file):

        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We should have the parameter variable name on a separate row.

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 367 at r1 (raw file):

    {
        if ($targetResource.Ensure -eq 'Absent') {
            ## Try to restore account if it exists

Comments should have one #. I see other comments are made with two ##. But lets start to resolve this here. :)

Throughout.

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 372 at r1 (raw file):

                Write-Verbose -Message ($LocalizedData.RestoringADComputer -f $ComputerName)
                $restoreParams = Get-ADCommonParameters @PSBoundParameters
                $restorationSuccessful = Restore-ADCommonObject @restoreParams -ObjectClass Computer -ErrorAction Stop -PassThru

If restoration was successful, should we call $targetResource = Get-TargetResource @PSBoundParameters again, to update the $targetResource variable, so that the set-part of below (line ~429) works correctly? Saw that this is called if the account is created, so maybe we should do it here too?

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 375 at r1 (raw file):

            }

            ## Computer does not exist and needs creating

Please make this a comment-block.

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 377 at r1 (raw file):

            ## Computer does not exist and needs creating
            ## or account not present in recycle bin
            if ((-not $RestoreFromRecycleBin -and $RequestFile) -or ($RestoreFromRecycleBin -and -not $restorationSuccessful -and $RequestFile))

Do we need this complex check (and the one that was changed below)? Cant we just make new if-block that checks -not $RestoreFromRecycleBin -or ($RestoreFromRecycleBin -and -not $restorationSuccessful), if either of those are true then we should try to create the object? Then we can leave the original if-blocks intact?

DSCResources/MSFT_xADComputer/MSFT_xADComputer.psm1, line 413 at r1 (raw file):

                        $DomainName,$ComputerName,$RequestFile)
            }
            elseif (-not ($RequestFile -or $RestoreFromRecycleBin) -or (-not $RequestFile -and $RestoreFromRecycleBin -and -not $restorationSuccessful))

See previous comment.

DSCResources/MSFT_xADComputer/MSFT_xADComputer.schema.mof, line 18 at r1 (raw file):

    [Write, Description("Specifies the full path to the Offline Domain Join Request file to create.")] String RequestFile;
    [Write, ValueMap{"Present", "Absent"},Values{"Present", "Absent"}] String Ensure;
    [Write, Description("Indicates whether or not objects should be restored from recycle bin instead of recreating them")] Boolean RestoreFromRecycleBin;

Please update this from the README.md when the description there changed.

DSCResources/MSFT_xADComputer/en-US/MSFT_xADComputer.psd1, line 21 at r1 (raw file):

restore computer

Maybe: ...restore the computer object {0} from the recycle bin.

Please add full stop (.) at the end of the sentence.

DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1, line 22 at r1 (raw file):

restore group

Maybe: ...restore the group object {0} from the recycle bin.

Please add full stop (.) at the end of the sentence.

DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1, line 112 at r1 (raw file):

        [ValidateNotNull()]
        [System.Boolean]
        $RestoreFromRecycleBin

We don't have to add the non-mandatory parameters in the Get-TargetResource function. We can remove it here since it is not used.

DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1, line 521 at r1 (raw file):

                $adGroupParams['Path'] = $Path
            }
            # Create group

Can you concatenate these comment, or create a comment-block?

DSCResources/MSFT_xADGroup/MSFT_xADGroup.schema.mof, line 19 at r1 (raw file):

    [Write, Description("Active Directory managed by attribute specified as a DistinguishedName")] String ManagedBy;
    [Write, Description("Active Directory group notes field")] String Notes;
    [Write, Description("Indicates whether or not objects should be restored from recycle bin instead of recreating them")] Boolean RestoreFromRecycleBin;

Please update this from the README.md when the description there changed.

DSCResources/MSFT_xADOrganizationalUnit/MSFT_xADOrganizationalUnit.psm1, line 17 at r1 (raw file):

restore OU

Maybe: ...restore the organizational unit (OU) object {0} from the recycle bin.

Please add full stop (.) at the end of the sentence.

DSCResources/MSFT_xADOrganizationalUnit/MSFT_xADOrganizationalUnit.psm1, line 81 at r1 (raw file):

        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We should have the parameter variable name on a separate row.

DSCResources/MSFT_xADOrganizationalUnit/MSFT_xADOrganizationalUnit.psm1, line 164 at r1 (raw file):

        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We should have the parameter variable name on a separate row.

DSCResources/MSFT_xADOrganizationalUnit/MSFT_xADOrganizationalUnit.psm1, line 213 at r1 (raw file):

        }

        return # return from Set method to make it easier to test for a succesful restore

Can we try to change the code so we don't have to return here? See next comment.

DSCResources/MSFT_xADOrganizationalUnit/MSFT_xADOrganizationalUnit.psm1, line 215 at r1 (raw file):

        return # return from Set method to make it easier to test for a succesful restore
    }
    elseif ($RestoreFromRecycleBin)

If we leave this as an else block, than add a if ($RestoreFromRecycleBin) inside the else-block, and after that, still in the else-block, have if (-not $RestoreFromRecycleBin -or ($RestoreFromRecycleBin -and -not $restoreSuccessful)), then we don't have to add the return statement above?

DSCResources/MSFT_xADOrganizationalUnit/MSFT_xADOrganizationalUnit.schema.mof, line 11 at r1 (raw file):

    [Write, Description("Defaults to True")] boolean ProtectedFromAccidentalDeletion;
    [Write, Description("The description of the OU")] string Description;
    [Write, Description("Indicates whether or not objects should be restored from recycle bin instead of recreating them")] Boolean RestoreFromRecycleBin;

Please update this from the README.md when the description there changed.

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 34 at r1 (raw file):

restore user

Maybe: ...restore the user object {0} from the recycle bin.

Please add full stop (.) at the end of the sentence.

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 273 at r1 (raw file):

        ## Indicates whether or not objects should be restored from recycle bin instead of recreating them
        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We don't have to add the non-mandatory parameters in the Get-TargetResource function. We can remove it here since it is not used.

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 538 at r1 (raw file):

        ## Indicates whether or not objects should be restored from recycle bin instead of recreating them
        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We should have the parameter variable name on a separate row.

DSCResources/MSFT_xADUser/MSFT_xADUser.psm1, line 791 at r1 (raw file):

        ## Indicates whether or not objects should be restored from recycle bin instead of recreating them
        [ValidateNotNull()]
        [System.Boolean] $RestoreFromRecycleBin

We should have the parameter variable name on a separate row.

DSCResources/MSFT_xADUser/MSFT_xADUser.schema.mof, line 49 at r1 (raw file):

    [Write, Description("Specifies the user account credentials to use to perform this task"), EmbeddedInstance("MSFT_Credential")] String DomainAdministratorCredential;
    [Write, Description("Specifies the authentication context type used when testing passwords"), ValueMap{"Default","Negotiate"},Values{"Default","Negotiate"}] String PasswordAuthentication;
    [Write, Description("Indicates whether or not objects should be restored from recycle bin instead of recreating them")] Boolean RestoreFromRecycleBin;

Please update this from the README.md when the description there changed.

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 633 at r1 (raw file):

            $restoreObjectWrongClass = 'wrong'

            Context 'Objects in recycle bin' {

We should start the context-blocks with 'When...'. Throughout.

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 638 at r1 (raw file):

'Does not throw

We should start the It-blocks with 'Should... '. Throughout.

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 646 at r1 (raw file):

'Returns objects if PassThru is specified'

We want to test so that the correct object is returned, so we should change this description to 'Should return the correct object...'

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 648 at r1 (raw file):

                It 'Returns objects if PassThru is specified' {
                    Mock -CommandName Restore-ADObject -MockWith { return $getAdObjectReturnValue}
                    Restore-ADCommonObject -Identity $restoreIdentity -ObjectClass $restoreObjectClass -PassThru | Should -Not -Be $null

We should verify that the correct object is returned, not just any non-null value.

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 651 at r1 (raw file):

'Throws when invalid parameters are used'

'Throws the correct error when invalid parameters are used'.

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 652 at r1 (raw file):

                It 'Throws when invalid parameters are used' {
                    {Restore-ADCommonObject -Identity $restoreIdentity -ObjectClass $restoreObjectWrongClass} | Should -Throw

We should test so that the correct error message are thrown here.

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 656 at r1 (raw file):

                It 'Calls Get-ADObject as well as Restore-ADObject' {
                    Assert-VerifiableMock

For this assert to work, the mocks must use the -Verifiable parameter.

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 662 at r1 (raw file):

                    Mock -CommandName Restore-ADObject -MockWith { throw (New-Object -TypeName Microsoft.ActiveDirectory.Management.ADException)}

                    {Restore-ADCommonObject -Identity $restoreIdentity -ObjectClass $restoreObjectClass} | Should -Throw

Can we try this? It's new for Pester 4.x.
This way we test that the type is actually thrown.

Should -Throw -ExceptionType ([Microsoft.ActiveDirectory.Management.ADException])

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 671 at r1 (raw file):

                Mock -CommandName Restore-ADObject

                It 'Should not call Restore-ADObject' {

This block does not call any function to test. It should call Restore-ADCommonObject, and it should also test so that we get the expect value back ($null value?).

Tests/Unit/MSFT_xADCommon.Tests.ps1, line 672 at r1 (raw file):

                It 'Should not call Restore-ADObject' {
                    Assert-MockCalled -CommandName Restore-ADObject -Exactly -Times 0

We should add -Scope It so we test that the it is called for this test.

johlju

I review one more tests, but since there are similar comments on the other tests that I haven't reviewed yet, as I commented on on this one, I wait until you resolved these until I continue the review.

Reviewed 3 of 20 files at r1.
Reviewable status: 17 of 20 files reviewed, 59 unresolved discussions (waiting on @nyanhp)

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 539 at r1 (raw file):

{ $true}

We should either remove the first blank space, or add another one after $true? Throughout the new tests.

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 539 at r1 (raw file):

                }}
                Mock -CommandName New-ADComputer
                Mock -CommandName Set-ADComputer -ParameterFilter { $true} # Had to overwrite parameter filter from an earlier test

Please move the comment above the row, on a separate line, instead of after the code. Easier to read, and makes the lines shorter when reviewing. Throughout the new tests.

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 539 at r1 (raw file):

                }}
                Mock -CommandName New-ADComputer
                Mock -CommandName Set-ADComputer -ParameterFilter { $true} # Had to overwrite parameter filter from an earlier test

Curious, if you make a new new nested Context-block, e.g

Context 'When the parameter RestoreFromRecycleBin is used` {
}

and put the It-blocks inside that Context-block, do you need to override the parameter filter then too, or does Pester handle that itself then? Throughout the new test if it works.

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 543 at r1 (raw file):

                Set-TargetResource @restoreParam

                Assert-MockCalled -CommandName Restore-ADCommonObject -Scope It

We should add -Exactly -Times 1 here?

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 545 at r1 (raw file):

                Assert-MockCalled -CommandName Restore-ADCommonObject -Scope It
                Assert-MockCalled -CommandName New-ADComputer -Times 0 -Exactly -Scope It
                Assert-MockCalled -CommandName Set-ADComputer -Scope It

We should add -Exactly -Times 1 here?

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 567 at r1 (raw file):

                Set-TargetResource @restoreParam

                Assert-MockCalled -CommandName Restore-ADCommonObject -Scope It

We should add -Exactly -Times 1 here, and for the other below too.

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 572 at r1 (raw file):

'Throws when

'Throws the correct error when...'. See also next comment,.

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 589 at r1 (raw file):

                Mock -CommandName Set-ADComputer -ParameterFilter { $true} # Had to overwrite parameter filter from an earlier test

                {Set-TargetResource @restoreParam} | Should -Throw

We should make sure to test that it throws the correct error here, not just any error.

Tests/Unit/MSFT_xADComputer.Tests.ps1, line 591 at r1 (raw file):

                {Set-TargetResource @restoreParam} | Should -Throw

                Assert-MockCalled -CommandName Restore-ADCommonObject -Scope It

We should add -Exactly -Times 1 here

nyanhp · 2018-08-09T13:10:28Z

Hi @johlju , will get to the review shortly. I was on vacation

johlju · 2018-08-10T08:25:16Z

@nyanhp I hope you had a great vacation! 😃

nyanhp

Sure did... I worked through the review now and have adressed all open comments. I tested the functionality to the best of my abilities with 2012R2 and 2016

Reviewable status: 4 of 20 files reviewed, 59 unresolved discussions (waiting on @johlju and @nyanhp)

README.md, line 82 at r1 (raw file):