ADObjectPermissionEntry: Fix Exception When the Object Path Does Not Exist

[oliveirt]

Pull Request (PR) description

Fixes issue with ADObjectPermissionEntry resource, where unable to run Get-DscConfiguration or Test-DscConfiguration where the object path of the associated AD resource (on which the PermissionEntry is defined) doesn't exist yet.

This Pull Request (PR) fixes the following issues

• Fixes ADObjectPermissionEntry: throws an exception when target path does not yet exist #552

---

This change is 

[X-Guardian]

Hi @oliveirt, this PR still contains the changes for both resources. If you need help setting this up, message me on the PowerShell Slack channel: http://slack.poshcode.org.

[oliveirt]

Apologies Simon - how about now? Should only include the first of the two commits. Regards Tony.

…

________________________________ From: Simon Heather <[email protected]> Sent: 04 February 2020 15:12 To: dsccommunity/ActiveDirectoryDsc <[email protected]> Cc: Tony Oliveira <[email protected]>; Mention <[email protected]> Subject: Re: [dsccommunity/ActiveDirectoryDsc] ADObjectPermissionEntry: Fixes #552 (#555) Hi @oliveirt<https://github.com/oliveirt>, this PR still contains the changes for both resources. If you need help setting this up, message me on the PowerShell Slack channel: http://slack.poshcode.org. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#555?email_source=notifications&email_token=AC76VJWMLKZ5EMQWNWPJCZLRBGAXRA5CNFSM4KPD6XCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKX7FCQ#issuecomment-581956234>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC76VJU3CRWT6KQIY5ZPZYLRBGAXRANCNFSM4KPD6XCA>.

[X-Guardian]

Reviewed 2 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @oliveirt)

---

source/DSCResources/MSFT_ADObjectPermissionEntry/MSFT_ADObjectPermissionEntry.psm1, line 95 at r1 (raw file):

    }

    foreach ($access in $acl.Access)

Can we improve this function further by adding an if/else block around this foreachblock to check if the $acl variable is $true, move the ObjectPermissionEntryNotFound verbose message into the else block and then only have one return $returnValue?

---

Tests/Unit/MSFT_ADObjectPermissionEntry.Tests.ps1, line 160 at r1 (raw file):

                Mock -CommandName 'Get-Acl' -MockWith { throw New-Object System.Management.Automation.ItemNotFoundException }

                It 'Should return a valid result if the AD object path is absent and not throw an exception' {

Can we change this to 'Should return a valid result if the AD object path is absent'? No need to say 'not throw an exception'.

[oliveirt]

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @oliveirt and @X-Guardian)

---

source/DSCResources/MSFT_ADObjectPermissionEntry/MSFT_ADObjectPermissionEntry.psm1, line 95 at r1 (raw file):

ObjectPermissionEntryNotFound
I think it is OK as it is. If the acl is null then will not enter the foreach block. The first return statement on 114 returns the ace if found, whereas the one on 121 returns the absent ace as defined on line 70.

[X-Guardian]

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @oliveirt)

---

source/DSCResources/MSFT_ADObjectPermissionEntry/MSFT_ADObjectPermissionEntry.psm1, line 95 at r1 (raw file):

Previously, oliveirt (Tony Oliveira) wrote…

ObjectPermissionEntryNotFound
I think it is OK as it is. If the acl is null then will not enter the foreach block. The first return statement on 114 returns the ace if found, whereas the one on 121 returns the absent ace as defined on line 70.

Yes, technically the code works, but performing a Foreach loop on the property of an object that doesn't even exist and having two exit points on the function are not good programming practice, so lets take the opportunity to clean this up.

[X-Guardian]

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

---

Tests/Unit/MSFT_ADObjectPermissionEntry.Tests.ps1, line 160 at r1 (raw file):

Previously, X-Guardian (Simon Heather) wrote…

Can we change this to 'Should return a valid result if the AD object path is absent'? No need to say 'not throw an exception'.

Done

[X-Guardian]

Many thanks for this @oliveirt.

If you are in the mood for doing another PR, this resource could really do with some integration tests... #353.