From ab7de98719b94de8ac345b62c2366e8c00d3dc32 Mon Sep 17 00:00:00 2001 From: Johan Ljunggren Date: Sun, 7 Jul 2019 11:39:55 +0200 Subject: [PATCH 1/5] Changes to xActiveDirectory - Removed the helper function `ThrowInvalidOperationError` in favor of new helper functions for localization (issue #316). - Cleaned up some minor style violations in the code. --- CHANGELOG.md | 42 ++++++++++---- .../MSFT_xADDomain/MSFT_xADDomain.psm1 | 4 +- DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1 | 2 +- .../xActiveDirectory.Common.strings.psd1 | 2 +- .../xActiveDirectory.Common.psm1 | 29 +--------- Tests/Unit/MSFT_xADDomain.Tests.ps1 | 56 +++++++++++++------ Tests/Unit/MSFT_xADKDSKey.Tests.ps1 | 3 +- Tests/Unit/MSFT_xADUser.Tests.ps1 | 6 +- Tests/Unit/xActiveDirectory.Common.Tests.ps1 | 26 +++++---- 9 files changed, 97 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fda1d4f83..3292bcfd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,18 +3,32 @@ ## Unreleased - Changes to xActiveDirectory - - Added a Requirements section to every DSC resource README with the bullet point stating "Target machine must be running Windows Server 2008 R2 or later" ([issue #399](https://github.com/PowerShell/xActiveDirectory/issues/399)). - - Added 'about_\.help.txt' file to all resources ([issue #404](https://github.com/PowerShell/xActiveDirectory/issues/404)). + - Added a Requirements section to every DSC resource README with the + bullet point stating "Target machine must be running Windows Server + 2008 R2 or later" ([issue #399](https://github.com/PowerShell/xActiveDirectory/issues/399)). + - Added 'about_\.help.txt' file to all resources + ([issue #404](https://github.com/PowerShell/xActiveDirectory/issues/404)). + - Removed the helper function `ThrowInvalidOperationError` in favor of + [new helper functions for localization](https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#helper-functions-for-localization) + ([issue #316](https://github.com/PowerShell/xActiveDirectory/issues/316)). + - Cleaned up some minor style violations in the code. - Changes to xADManagedServiceAccount - - Added a requirement to README stating "Group Managed Service Accounts need at least one Windows Server 2012 Domain Controller" ([issue #399](https://github.com/PowerShell/xActiveDirectory/issues/399)). + - Added a requirement to README stating "Group Managed Service Accounts + need at least one Windows Server 2012 Domain Controller" + ([issue #399](https://github.com/PowerShell/xActiveDirectory/issues/399)). - Changes to xADComputer - - Fixed the GUID in Example 3-AddComputerAccountSpecificPath_Config. ([issue #410](https://github.com/PowerShell/xActiveDirectory/issues/410)). + - Fixed the GUID in Example 3-AddComputerAccountSpecificPath_Config + ([issue #410](https://github.com/PowerShell/xActiveDirectory/issues/410)). - Changes to xADOrganizationalUnit - - Catch exception when the path property specifies a non-existing path ([issue #408](https://github.com/PowerShell/xActiveDirectory/issues/408)). + - Catch exception when the path property specifies a non-existing path + ([issue #408](https://github.com/PowerShell/xActiveDirectory/issues/408)) - Changes to xADUser - - Fixes exception when creating a user with an empty string property ([issue #407](https://github.com/PowerShell/xActiveDirectory/issues/407)). - - Fixes exception when updating `CommonName` and `Path` concurrently ([issue #402](https://github.com/PowerShell/xActiveDirectory/issues/402)). - - Fixes ChangePasswordAtLogon Property to be only set to `true` at User Creation ([issue #414](https://github.com/PowerShell/xActiveDirectory/issues/414)). + - Fixes exception when creating a user with an empty string property + ([issue #407](https://github.com/PowerShell/xActiveDirectory/issues/407)). + - Fixes exception when updating `CommonName` and `Path` concurrently + ([issue #402](https://github.com/PowerShell/xActiveDirectory/issues/402)). + - Fixes ChangePasswordAtLogon Property to be only set to `true` at User + Creation ([issue #414](https://github.com/PowerShell/xActiveDirectory/issues/414)). ## 3.0.0.0 @@ -58,7 +72,8 @@ ([issue #374](https://github.com/PowerShell/xActiveDirectory/issues/374)). - Removed unused legacy test files from the root of the repository. - Updated Example List README with missing resources. - - Added missing examples for xADReplicationSubnet, xADServicePrincipalName and xWaitForADDomain. ([issue #395](https://github.com/PowerShell/xActiveDirectory/issues/395)). + - Added missing examples for xADReplicationSubnet, xADServicePrincipalName + and xWaitForADDomain. ([issue #395](https://github.com/PowerShell/xActiveDirectory/issues/395)). - Changes to xADComputer - Refactored the resource and the unit tests. - BREAKING CHANGE: The `Enabled` property is **DEPRECATED** and is no @@ -88,7 +103,8 @@ - Changes to xADOrganizationalUnit - Change the description of the property RestoreFromRecycleBin. - Code cleanup. - - Fix incorrect verbose message when this resource has Ensure set to Absent ([issue #276](https://github.com/PowerShell/xActiveDirectory/issues/276)). + - Fix incorrect verbose message when this resource has Ensure set to + Absent ([issue #276](https://github.com/PowerShell/xActiveDirectory/issues/276)). - Changes to xADUser - Change the description of the property RestoreFromRecycleBin. - Added ServicePrincipalNames property ([issue #153](https://github.com/PowerShell/xActiveDirectory/issues/153)). @@ -174,12 +190,14 @@ and [@kungfu71186](https://github.com/kungfu71186) - Removing the Misc Folder, as it is no longer required. - Added xADKDSKey resource to create KDS Root Keys for gMSAs. [@kungfu71186](https://github.com/kungfu71186) - - Combined DscResource.LocalizationHelper and DscResource.Common Modules into xActiveDirectory.Common + - Combined DscResource.LocalizationHelper and DscResource.Common Modules + into xActiveDirectory.Common - Changes to xADReplicationSiteLink - Make use of the new localization helper functions. - Changes to xAdDomainController - Added new parameter to disable or enable the Global Catalog (GC) - ([issue #75](https://github.com/PowerShell/xActiveDirectory/issues/75)). [Eric Foskett @Merto410](https://github.com/Merto410) + ([issue #75](https://github.com/PowerShell/xActiveDirectory/issues/75)). + [Eric Foskett @Merto410](https://github.com/Merto410) - Fixed a bug with the parameter `InstallationMediaPath` that it would not be added if it was specified in a configuration. Now the parameter `InstallationMediaPath` is correctly passed to `Install-ADDSDomainController`. diff --git a/DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1 b/DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1 index ef0292999..7edd0f1dc 100644 --- a/DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1 +++ b/DSCResources/MSFT_xADDomain/MSFT_xADDomain.psm1 @@ -150,7 +150,7 @@ function Get-TargetResource catch [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException] { $errorMessage = $script:localizedData.ExistingDomainMemberError -f $DomainName - ThrowInvalidOperationError -ErrorId 'xADDomain_DomainMember' -ErrorMessage $errorMessage + New-ObjectNotFoundException -Message $errorMessage -ErrorRecord $_ } catch [Microsoft.ActiveDirectory.Management.ADServerDownException] { @@ -161,7 +161,7 @@ function Get-TargetResource catch [System.Security.Authentication.AuthenticationException] { $errorMessage = $script:localizedData.InvalidCredentialError -f $DomainName - ThrowInvalidOperationError -ErrorId 'xADDomain_InvalidCredential' -ErrorMessage $errorMessage + New-InvalidOperationException -Message $errorMessage -ErrorRecord $_ } catch { diff --git a/DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1 b/DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1 index 199c71769..e7822145f 100644 --- a/DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1 +++ b/DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1 @@ -103,7 +103,7 @@ function Get-TargetResource Write-Verbose -Message ($script:localizedData.RetrievingGroupMembers -f $MembershipAttribute) # Retrieve the current list of members, returning the specified membership attribute - [System.Array]$adGroupMembers = (Get-ADGroupMember @adGroupParams).$MembershipAttribute + [System.Array] $adGroupMembers = (Get-ADGroupMember @adGroupParams).$MembershipAttribute $targetResource = @{ GroupName = $adGroup.Name diff --git a/Modules/xActiveDirectory.Common/en-US/xActiveDirectory.Common.strings.psd1 b/Modules/xActiveDirectory.Common/en-US/xActiveDirectory.Common.strings.psd1 index 079f6ffa4..6bf9054ba 100644 --- a/Modules/xActiveDirectory.Common/en-US/xActiveDirectory.Common.strings.psd1 +++ b/Modules/xActiveDirectory.Common/en-US/xActiveDirectory.Common.strings.psd1 @@ -15,7 +15,7 @@ ConvertFrom-StringData @' MembersIsNullError = The Members parameter value is null. The '{0}' parameter must be provided if neither '{1}' nor '{2}' is provided. (ADCOMMON0012) IncludeAndExcludeConflictError = The member '{0}' is included in both '{1}' and '{2}' parameter values. The same member must not be included in both '{1}' and '{2}' parameter values. (ADCOMMON0014) IncludeAndExcludeAreEmptyError = The '{0}' and '{1}' parameters are either both null or empty. At least one member must be specified in one of these parameters. (ADCOMMON0015) - RecycleBinRestoreFailed = Restoring {0} ({1}) from the recycle bin failed. Error message: {2}. (ADCOMMON0017) + RecycleBinRestoreFailed = Failed restoring {0} ({1}) from the recycle bin. (ADCOMMON0017) EmptyDomainError = No domain name retrieved for group member {0} in group {1}. (ADCOMMON0018) CheckingMembers = Checking for '{0}' members. (ADCOMMON0019) MembershipCountMismatch = Membership count is not correct. Expected '{0}' members, actual '{1}' members. (ADCOMMON0020) diff --git a/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 b/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 index 605f738a5..23b484388 100644 --- a/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 +++ b/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 @@ -488,9 +488,8 @@ function Assert-Module if (-not (Get-Module -Name $ModuleName -ListAvailable)) { - $errorId = '{0}_ModuleNotFound' -f $ModuleName $errorMessage = $script:localizedData.RoleNotFoundError -f $moduleName - ThrowInvalidOperationError -ErrorId $errorId -ErrorMessage $errorMessage + New-ObjectNotFoundException -Message $errorMessage } if ($ImportModule) @@ -1039,28 +1038,6 @@ function Get-ADCommonParameters return $adConnectionParameters } #end function Get-ADCommonParameters -function ThrowInvalidOperationError -{ - [CmdletBinding()] - param - ( - [Parameter(Mandatory = $true)] - [ValidateNotNullOrEmpty()] - [System.String] - $ErrorId, - - [Parameter(Mandatory = $true)] - [ValidateNotNullOrEmpty()] - [System.String] - $ErrorMessage - ) - - $exception = New-Object -TypeName 'System.InvalidOperationException' -ArgumentList $ErrorMessage - $errorCategory = [System.Management.Automation.ErrorCategory]::InvalidOperation - $errorRecord = New-Object -TypeName 'System.Management.Automation.ErrorRecord' -ArgumentList @($exception, $ErrorId, $errorCategory, $null) - throw $errorRecord -} - function ThrowInvalidArgumentError { [CmdletBinding()] @@ -1277,7 +1254,8 @@ function Restore-ADCommonObject catch [Microsoft.ActiveDirectory.Management.ADException] { # After Get-TargetResource is through, only one error can occur here: Object parent does not exist - ThrowInvalidOperationError -ErrorId "$($Identity)_RecycleBinRestoreFailed" -ErrorMessage ($script:localizedData.RecycleBinRestoreFailed -f $Identity, $ObjectClass, $_.Exception.Message) + $errorMessage = $script:localizedData.RecycleBinRestoreFailed -f $Identity, $ObjectClass + New-InvalidOperationException -Message $errorMessage -ErrorRecord $_ } } @@ -1835,7 +1813,6 @@ Export-ModuleMember -Function @( 'ConvertTo-TimeSpan' 'ConvertFrom-TimeSpan' 'Get-ADCommonParameters' - 'ThrowInvalidOperationError' 'ThrowInvalidArgumentError' 'Test-ADReplicationSite' 'ConvertTo-DeploymentForestMode' diff --git a/Tests/Unit/MSFT_xADDomain.Tests.ps1 b/Tests/Unit/MSFT_xADDomain.Tests.ps1 index 09e7fe214..762c0bca1 100644 --- a/Tests/Unit/MSFT_xADDomain.Tests.ps1 +++ b/Tests/Unit/MSFT_xADDomain.Tests.ps1 @@ -79,12 +79,17 @@ try It 'Calls "Assert-Module" to check "ADDSDeployment" module is installed' { Mock -CommandName Get-ADDomain -MockWith { - [psobject]@{ + [PSObject] @{ Forest = $correctDomainName DomainMode = $mgmtDomainMode } } - Mock -CommandName Get-ADForest -MockWith { [psobject]@{ForestMode = $mgmtForestMode} } + + Mock -CommandName Get-ADForest -MockWith { + [PSObject] @{ + ForestMode = $mgmtForestMode + } + } $result = Get-TargetResource @testDefaultParams -DomainName $correctDomainName @@ -93,13 +98,17 @@ try It 'Returns "System.Collections.Hashtable" object type' { Mock -CommandName Get-ADDomain { - [psobject]@{ + [PSObject] @{ Forest = $correctDomainName DomainMode = $mgmtDomainMode } } - Mock -CommandName Get-ADForest -MockWith { [psobject]@{ForestMode = $mgmtForestMode} } + Mock -CommandName Get-ADForest -MockWith { + [PSObject] @{ + ForestMode = $mgmtForestMode + } + } $result = Get-TargetResource @testDefaultParams -DomainName $correctDomainName @@ -109,7 +118,7 @@ try It 'Calls "Get-ADDomain" without credentials if domain member' { Mock -CommandName Test-DomainMember -MockWith { $true; } Mock -CommandName Get-ADDomain -ParameterFilter { $Credential -eq $null } -MockWith { - [psobject]@{ + [PSObject] @{ Forest = $correctDomainName DomainMode = $mgmtDomainMode } @@ -123,12 +132,17 @@ try It 'Calls "Get-ADForest" without credentials if domain member' { Mock -CommandName Test-DomainMember -MockWith { $true; } Mock -CommandName Get-ADDomain -ParameterFilter { $Credential -eq $null } -MockWith { - [psobject]@{ + [PSObject] @{ Forest = $correctDomainName DomainMode = $mgmtDomainMode } } - Mock -CommandName Get-ADForest -ParameterFilter { $Credential -eq $null } -MockWith { [psobject]@{ForestMode = $mgmtForestMode} } + + Mock -CommandName Get-ADForest -ParameterFilter { $Credential -eq $null } -MockWith { + [PSObject] @{ + ForestMode = $mgmtForestMode + } + } $result = Get-TargetResource @testDefaultParams -DomainName $correctDomainName @@ -137,24 +151,24 @@ try It 'Throws "Invalid credentials" when domain is available but authentication fails' { Mock -CommandName Get-ADDomain -ParameterFilter { $Identity.ToString() -eq $incorrectDomainName } -MockWith { - Write-Error -Exception (New-Object System.Security.Authentication.AuthenticationException) + throw New-Object System.Security.Authentication.AuthenticationException } # Match operator is case-sensitive! - { Get-TargetResource @testDefaultParams -DomainName $incorrectDomainName } | Should -Throw 'invalid credentials' + { Get-TargetResource @testDefaultParams -DomainName $incorrectDomainName } | Should -Throw ($script:localizedData.InvalidCredentialError -f $incorrectDomainName) } It 'Throws "Computer is already a domain member" when is already a domain member' { Mock -CommandName Get-ADDomain -ParameterFilter { $Identity.ToString() -eq $incorrectDomainName } -MockWith { - Write-Error -Exception (New-Object Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException) + throw New-Object Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException } - { Get-TargetResource @testDefaultParams -DomainName $incorrectDomainName } | Should -Throw 'Computer is already a domain member' + { Get-TargetResource @testDefaultParams -DomainName $incorrectDomainName } | Should -Throw ($script:localizedData.ExistingDomainMemberError -f $incorrectDomainName) } It 'Does not throw when domain cannot be located' { Mock -CommandName Get-ADDomain -ParameterFilter { $Identity.ToString() -eq $missingDomainName } -MockWith { - Write-Error -Exception (New-Object Microsoft.ActiveDirectory.Management.ADServerDownException) + throw New-Object Microsoft.ActiveDirectory.Management.ADServerDownException } { Get-TargetResource @testDefaultParams -DomainName $missingDomainName } | Should -Not -Throw @@ -162,24 +176,34 @@ try It 'Returns the correct domain mode' { Mock -CommandName Get-ADDomain -MockWith { - [psobject]@{ + [PSObject] @{ Forest = $correctDomainName DomainMode = $mgmtDomainMode } } - Mock -CommandName Get-ADForest -MockWith { [psobject]@{ForestMode = $mgmtForestMode} } + + Mock -CommandName Get-ADForest -MockWith { + [PSObject] @{ + ForestMode = $mgmtForestMode + } + } (Get-TargetResource @testDefaultParams -DomainName $correctDomainName).DomainMode | Should -Be $domainMode } It 'Returns the correct forest mode' { Mock -CommandName Get-ADDomain -MockWith { - [psobject]@{ + [PSObject] @{ Forest = $correctDomainName DomainMode = $mgmtDomainMode } } - Mock -CommandName Get-ADForest -MockWith { [psobject]@{ForestMode = $mgmtForestMode} } + + Mock -CommandName Get-ADForest -MockWith { + [PSObject] @{ + ForestMode = $mgmtForestMode + } + } (Get-TargetResource @testDefaultParams -DomainName $correctDomainName).ForestMode | Should -Be $forestMode } diff --git a/Tests/Unit/MSFT_xADKDSKey.Tests.ps1 b/Tests/Unit/MSFT_xADKDSKey.Tests.ps1 index bf872765f..fb5cf4ed5 100644 --- a/Tests/Unit/MSFT_xADKDSKey.Tests.ps1 +++ b/Tests/Unit/MSFT_xADKDSKey.Tests.ps1 @@ -248,8 +248,9 @@ try Describe -Name 'MSFT_xADKDSKey\Get-ADRootDomainDN' { BeforeAll { Mock -CommandName New-Object -MockWith { - $object = [PSCustomObject]@{} + $object = [PSCustomObject] @{} $object | Add-Member -MemberType ScriptMethod -Name 'Get' -Value { return $mockADDomain } + return $object } } diff --git a/Tests/Unit/MSFT_xADUser.Tests.ps1 b/Tests/Unit/MSFT_xADUser.Tests.ps1 index 5f0427bfa..8547a0bb2 100644 --- a/Tests/Unit/MSFT_xADUser.Tests.ps1 +++ b/Tests/Unit/MSFT_xADUser.Tests.ps1 @@ -844,9 +844,11 @@ try $script:mockCounter = 0 - Mock -CommandName Restore-ADCommonObject -MockWith { return [PSCustomObject]@{ + Mock -CommandName Restore-ADCommonObject -MockWith { + return [PSCustomObject] @{ ObjectClass = 'user' - } } + } + } Set-TargetResource @restoreParam diff --git a/Tests/Unit/xActiveDirectory.Common.Tests.ps1 b/Tests/Unit/xActiveDirectory.Common.Tests.ps1 index b9822f278..9bbae6805 100644 --- a/Tests/Unit/xActiveDirectory.Common.Tests.ps1 +++ b/Tests/Unit/xActiveDirectory.Common.Tests.ps1 @@ -1205,7 +1205,7 @@ InModuleScope 'xActiveDirectory.Common' { } ) - $restoreAdObjectReturnValue = [PSCustomObject]@{ + $restoreAdObjectReturnValue = [PSCustomObject] @{ DistinguishedName = 'CN=a375347,CN=Accounts,DC=contoso,DC=com' Name = 'a375347' ObjectClass = 'user' @@ -1311,39 +1311,39 @@ InModuleScope 'xActiveDirectory.Common' { Mock -CommandName Assert-Module -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } $memberData = @( - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Account1,DC=contoso,DC=com' Domain = 'contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Group1,DC=contoso,DC=com' Domain = 'contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Computer1,DC=contoso,DC=com' Domain = 'contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Account1,DC=a,DC=contoso,DC=com' Domain = 'a.contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Group1,DC=a,DC=contoso,DC=com' Domain = 'a.contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Computer1,DC=a,DC=contoso,DC=com' Domain = 'a.contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Account1,DC=b,DC=contoso,DC=com' Domain = 'b.contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Group1,DC=b,DC=contoso,DC=com' Domain = 'b.contoso.com' } - [pscustomobject]@{ + [PSCustomObject] @{ Name = 'CN=Computer1,DC=b,DC=contoso,DC=com' Domain = 'b.contoso.com' } @@ -1400,9 +1400,11 @@ InModuleScope 'xActiveDirectory.Common' { {$Identity -match 'Computer'} { 'computer' } } - return ([PSCustomObject]@{ + return ( + [PSCustomObject] @{ objectClass = $objectClass - }) + } + ) } # Mocks should return something that is used with Add-ADGroupMember Mock -CommandName Get-ADComputer -MockWith { return 'placeholder' } From ca90d8a402e252a0c9a5cfa3c6cbd588c4e70a7e Mon Sep 17 00:00:00 2001 From: Johan Ljunggren Date: Sun, 7 Jul 2019 12:22:08 +0200 Subject: [PATCH 2/5] Removed ThrowInvalidArgumentError --- CHANGELOG.md | 11 +++-- DSCResources/MSFT_xADUser/MSFT_xADUser.psm1 | 15 ++---- .../xActiveDirectory.Common.psm1 | 49 +++++-------------- Tests/Unit/MSFT_xADUser.Tests.ps1 | 17 ++++--- Tests/Unit/xActiveDirectory.Common.Tests.ps1 | 36 +++++++++----- 5 files changed, 58 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3292bcfd5..7bac987c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,15 @@ 2008 R2 or later" ([issue #399](https://github.com/PowerShell/xActiveDirectory/issues/399)). - Added 'about_\.help.txt' file to all resources ([issue #404](https://github.com/PowerShell/xActiveDirectory/issues/404)). - - Removed the helper function `ThrowInvalidOperationError` in favor of + - Removed the helper function `ThrowInvalidOperationError` and + `ThrowInvalidArgumentError` in favor of the [new helper functions for localization](https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#helper-functions-for-localization) - ([issue #316](https://github.com/PowerShell/xActiveDirectory/issues/316)). + ([issue #316](https://github.com/PowerShell/xActiveDirectory/issues/316), + [issue #317](https://github.com/PowerShell/xActiveDirectory/issues/317)). - Cleaned up some minor style violations in the code. + - Fixed an issue that the helper function `Add-ADCommonGroupMember` was + not outputting the correct group name in a verbose message and in an + error message. - Changes to xADManagedServiceAccount - Added a requirement to README stating "Group Managed Service Accounts need at least one Windows Server 2012 Domain Controller" @@ -341,7 +346,7 @@ - xADDomain: Added check for Active Directory cmdlets. - xADDomain: Added additional error trapping, verbose and diagnostic information. - xADDomain: Added unit test coverage. -- Fixes CredentialAttribute and other PSScriptAnalyzer tests in xADCommon, xADDomin, xADGroup, xADOrganizationalUnit and xADUser resources. +- Fixes CredentialAttribute and other PSScriptAnalyzer tests in xADCommon, xADDomain, xADGroup, xADOrganizationalUnit and xADUser resources. ## 2.9.0.0 diff --git a/DSCResources/MSFT_xADUser/MSFT_xADUser.psm1 b/DSCResources/MSFT_xADUser/MSFT_xADUser.psm1 index dcd078d34..08d710f19 100644 --- a/DSCResources/MSFT_xADUser/MSFT_xADUser.psm1 +++ b/DSCResources/MSFT_xADUser/MSFT_xADUser.psm1 @@ -1708,23 +1708,16 @@ function Assert-Parameters # We cannot test/set passwords on disabled AD accounts if (($PSBoundParameters.ContainsKey('Password')) -and ($Enabled -eq $false)) { - $throwInvalidArgumentErrorParams = @{ - ErrorId = 'xADUser_DisabledAccountPasswordConflict' - ErrorMessage = $script:localizedData.PasswordParameterConflictError -f 'Enabled', $false, 'Password' - } - - ThrowInvalidArgumentError @throwInvalidArgumentErrorParams + $errorMessage = $script:localizedData.PasswordParameterConflictError -f 'Enabled', $false, 'Password' + New-InvalidArgumentException -ArgumentName 'Password' -Message $errorMessage } # ChangePasswordAtLogon cannot be set for an account that also has PasswordNeverExpires set if ($PSBoundParameters.ContainsKey('ChangePasswordAtLogon') -and $PSBoundParameters['ChangePasswordAtLogon'] -eq $true -and $PSBoundParameters.ContainsKey('PasswordNeverExpires') -and $PSBoundParameters['PasswordNeverExpires'] -eq $true) { - $throwInvalidArgumentErrorParams = @{ - ErrorId = 'xADUser_ChangePasswordParameterConflict' - ErrorMessage = $script:localizedData.ChangePasswordParameterConflictError - } - ThrowInvalidArgumentError @throwInvalidArgumentErrorParams + $errorMessage = $script:localizedData.ChangePasswordParameterConflictError + New-InvalidArgumentException -ArgumentName 'ChangePasswordAtLogon, PasswordNeverExpires' -Message $errorMessage } } #end function Assert-Parameters diff --git a/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 b/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 index 23b484388..f91c8ca83 100644 --- a/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 +++ b/Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1 @@ -622,16 +622,14 @@ function Assert-MemberParameters if ($PSBoundParameters.ContainsKey('MembersToInclude') -or $PSBoundParameters.ContainsKey('MembersToExclude')) { # If Members are provided, Include and Exclude are not allowed. - $errorId = '{0}_MembersPlusIncludeOrExcludeConflict' -f $ModuleName $errorMessage = $script:localizedData.MembersAndIncludeExcludeError -f 'Members', 'MembersToInclude', 'MembersToExclude' - ThrowInvalidArgumentError -ErrorId $errorId -ErrorMessage $errorMessage + New-InvalidArgumentException -ArgumentName 'Members' -Message $errorMessage } if ($Members.Length -eq 0) { - $errorId = '{0}_MembersIsNull' -f $ModuleName $errorMessage = $script:localizedData.MembersIsNullError -f 'Members', 'MembersToInclude', 'MembersToExclude' - ThrowInvalidArgumentError -ErrorId $errorId -ErrorMessage $errorMessage + New-InvalidArgumentException -ArgumentName 'Members' -Message $errorMessage } } @@ -649,19 +647,17 @@ function Assert-MemberParameters { if (($MembersToInclude.Length -eq 0) -and ($MembersToExclude.Length -eq 0)) { - $errorId = '{0}_EmptyIncludeAndExclude' -f $ModuleName $errorMessage = $script:localizedData.IncludeAndExcludeAreEmptyError -f 'MembersToInclude', 'MembersToExclude' - ThrowInvalidArgumentError -ErrorId $errorId -ErrorMessage $errorMessage + New-InvalidArgumentException -ArgumentName 'MembersToInclude, MembersToExclude' -Message $errorMessage } - # Both MembersToInclude and MembersToExlude were provided. Check if they have common principals. + # Both MembersToInclude and MembersToExclude were provided. Check if they have common principals. foreach ($member in $MembersToInclude) { if ($member -in $MembersToExclude) { - $errorId = '{0}_IncludeAndExcludeConflict' -f $ModuleName $errorMessage = $script:localizedData.IncludeAndExcludeConflictError -f $member, 'MembersToInclude', 'MembersToExclude' - ThrowInvalidArgumentError -ErrorId $errorId -ErrorMessage $errorMessage + New-InvalidArgumentException -ArgumentName 'MembersToInclude, MembersToExclude' -Message $errorMessage } } } @@ -714,7 +710,7 @@ function Remove-DuplicateMembers } #end function RemoveDuplicateMembers # Internal function to test whether the existing array members match the defined explicit array -# members, the included members are present and the exlcuded members are not present. +# members, the included members are present and the excluded members are not present. function Test-Members { [CmdletBinding()] @@ -886,7 +882,7 @@ function ConvertTo-TimeSpan <# .SYNOPSIS - Converts a System.TimeSpan into the number of seconds, mintutes, hours or days. + Converts a System.TimeSpan into the number of seconds, minutes, hours or days. .PARAMETER TimeSpan TimeSpan to convert into an integer @@ -1038,29 +1034,6 @@ function Get-ADCommonParameters return $adConnectionParameters } #end function Get-ADCommonParameters -function ThrowInvalidArgumentError -{ - [CmdletBinding()] - param - ( - [Parameter(Mandatory = $true)] - [ValidateNotNullOrEmpty()] - [System.String] - $ErrorId, - - [Parameter(Mandatory = $true)] - [ValidateNotNullOrEmpty()] - [System.String] - $ErrorMessage - ) - - $exception = New-Object -TypeName 'System.ArgumentException' -ArgumentList $ErrorMessage - $errorCategory = [System.Management.Automation.ErrorCategory]::InvalidArgument - $errorRecord = New-Object -TypeName 'System.Management.Automation.ErrorRecord' -ArgumentList @($exception, $ErrorId, $errorCategory, $null) - throw $errorRecord - -} #end function ThrowInvalidArgumentError - # Internal function to test site availability function Test-ADReplicationSite { @@ -1337,11 +1310,14 @@ function Add-ADCommonGroupMember if (-not $memberDomain) { - ThrowInvalidArgumentError -ErrorId "$($member)_EmptyDomainError" -ErrorMessage ($script:localizedData.EmptyDomainError -f $member, $Parameters.GroupName) + $errorMessage = $script:localizedData.EmptyDomainError -f $member, $Parameters.Identity + New-InvalidOperationException -Message $errorMessage } - Write-Verbose -Message ($script:localizedData.AddingGroupMember -f $member, $memberDomain, $Parameters.GroupName) + Write-Verbose -Message ($script:localizedData.AddingGroupMember -f $member, $memberDomain, $Parameters.Identity) + $memberObjectClass = (Get-ADObject -Identity $member -Server $memberDomain -Properties ObjectClass).ObjectClass + if ($memberObjectClass -eq 'computer') { $memberObject = Get-ADComputer -Identity $member -Server $memberDomain @@ -1813,7 +1789,6 @@ Export-ModuleMember -Function @( 'ConvertTo-TimeSpan' 'ConvertFrom-TimeSpan' 'Get-ADCommonParameters' - 'ThrowInvalidArgumentError' 'Test-ADReplicationSite' 'ConvertTo-DeploymentForestMode' 'ConvertTo-DeploymentDomainMode' diff --git a/Tests/Unit/MSFT_xADUser.Tests.ps1 b/Tests/Unit/MSFT_xADUser.Tests.ps1 index 8547a0bb2..16e0352a9 100644 --- a/Tests/Unit/MSFT_xADUser.Tests.ps1 +++ b/Tests/Unit/MSFT_xADUser.Tests.ps1 @@ -891,21 +891,24 @@ try #region Function Assert-TargetResource Describe 'xADUser\Assert-Parameters' { - It "Does not throw when 'PasswordNeverExpires' and 'CannotChangePassword' are specified" { + It 'Should not throw when both parameters PasswordNeverExpires and CannotChangePassword are specified' { { Assert-Parameters -PasswordNeverExpires $true -CannotChangePassword $true } | Should -Not -Throw } - It "Throws when account is disabled and 'Password' is specified" { - { Assert-Parameters -Password $testCredential -Enabled $false } | Should -Throw + It 'Should throws the correct error when the parameter Enabled is set to $false and the parameter Password is also specified' { + { + Assert-Parameters -Password $testCredential -Enabled $false + } | Should -Throw ($script:localizedData.PasswordParameterConflictError -f 'Enabled', $false, 'Password') } - It "Does not throw when 'TrustedForDelegation' is specified" { + It 'Should not throw when the parameter TrustedForDelegation is specified' { { Assert-Parameters -TrustedForDelegation $true } | Should -Not -Throw } - It "Should throw the correct error when 'PasswordNeverExpires' and 'ChangePasswordAtLogon' are specified" { - { Assert-Parameters -PasswordNeverExpires $true -ChangePasswordAtLogon $true } | ` - Should -Throw $script:localizedData.ChangePasswordParameterConflictError + It 'Should throw the correct error when both parameters PasswordNeverExpires and ChangePasswordAtLogon are specified' { + { + Assert-Parameters -PasswordNeverExpires $true -ChangePasswordAtLogon $true + } | Should -Throw $script:localizedData.ChangePasswordParameterConflictError } } #endregion diff --git a/Tests/Unit/xActiveDirectory.Common.Tests.ps1 b/Tests/Unit/xActiveDirectory.Common.Tests.ps1 index 9bbae6805..587dbec72 100644 --- a/Tests/Unit/xActiveDirectory.Common.Tests.ps1 +++ b/Tests/Unit/xActiveDirectory.Common.Tests.ps1 @@ -863,24 +863,34 @@ InModuleScope 'xActiveDirectory.Common' { } Describe 'xActiveDirectory.Common\Assert-MemberParameters' { - It "Throws if 'Members' is specified but is empty" { - { Assert-MemberParameters -Members @() } | Should -Throw ($script:localizedData.MembersIsNullError -f 'Members', 'MembersToInclude', 'MembersToExclude') + It 'Should throw if parameter Members is specified but is empty' { + { + Assert-MemberParameters -Members @() + } | Should -Throw ($script:localizedData.MembersIsNullError -f 'Members', 'MembersToInclude', 'MembersToExclude') } - It "Throws if 'Members' and 'MembersToInclude' are specified" { - { Assert-MemberParameters -Members @('User1') -MembersToInclude @('User1') } | Should -Throw 'parameters conflict' + It 'Should throws if both parameters Members and MembersToInclude are specified' { + { + Assert-MemberParameters -Members @('User1') -MembersToInclude @('User2') + } | Should -Throw ($script:localizedData.MembersAndIncludeExcludeError -f 'Members', 'MembersToInclude', 'MembersToExclude') } - It "Throws if 'Members' and 'MembersToExclude' are specified" { - { Assert-MemberParameters -Members @('User1') -MembersToExclude @('User2') } | Should -Throw 'parameters conflict' + It 'Should throw if both parameters Members and MembersToExclude are specified' { + { + Assert-MemberParameters -Members @('User1') -MembersToExclude @('User2') + } | Should -Throw ($script:localizedData.MembersAndIncludeExcludeError -f 'Members', 'MembersToInclude', 'MembersToExclude') } - It "Throws if 'MembersToInclude' and 'MembersToExclude' contain the same member" { - { Assert-MemberParameters -MembersToExclude @('user1') -MembersToInclude @('USER1') } | Should -Throw 'member must not be included in both' + It 'Should throws if the both parameters MembersToInclude and MembersToExclude contain the same member' { + { + Assert-MemberParameters -MembersToExclude @('user1') -MembersToInclude @('USER1') + } | Should -Throw ($errorMessage = $script:localizedData.IncludeAndExcludeConflictError -f 'user1', 'MembersToInclude', 'MembersToExclude') } - It "Throws if 'MembersToInclude' and 'MembersToExclude' are empty" { - { Assert-MemberParameters -MembersToExclude @() -MembersToInclude @() } | Should -Throw 'At least one member must be specified' + It 'Should throw if both parameters MembersToInclude and MembersToExclude contains no members (are empty)' { + { + Assert-MemberParameters -MembersToExclude @() -MembersToInclude @() + } | Should -Throw ($script:localizedData.IncludeAndExcludeAreEmptyError -f 'MembersToInclude', 'MembersToExclude') } } @@ -1424,8 +1434,10 @@ InModuleScope 'xActiveDirectory.Common' { } Context 'When the domain name cannot be determined' { - It 'Should throw an InvalidArgumentException' { - {Add-ADCommonGroupMember -Members $invalidMemberData -Parameters $fakeParameters -MembersInMultipleDomains} | Should -Throw -ExceptionType ([System.ArgumentException]) + It 'Should throw the correct error' { + { + Add-ADCommonGroupMember -Members $invalidMemberData -Parameters $fakeParameters -MembersInMultipleDomains + } | Should -Throw ($script:localizedData.EmptyDomainError -f $invalidMemberData[0], $fakeParameters.Identity) } } } From 6f6a95d29e4a5cc8fe5673eadd446c57e3262e8c Mon Sep 17 00:00:00 2001 From: Johan Ljunggren Date: Sun, 7 Jul 2019 12:36:18 +0200 Subject: [PATCH 3/5] Fix failing test --- Tests/Unit/xActiveDirectory.Common.Tests.ps1 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Tests/Unit/xActiveDirectory.Common.Tests.ps1 b/Tests/Unit/xActiveDirectory.Common.Tests.ps1 index 587dbec72..1d0b7b3b3 100644 --- a/Tests/Unit/xActiveDirectory.Common.Tests.ps1 +++ b/Tests/Unit/xActiveDirectory.Common.Tests.ps1 @@ -1254,9 +1254,13 @@ InModuleScope 'xActiveDirectory.Common' { } It 'Should throw an InvalidOperationException when object parent does not exist' { - Mock -CommandName Restore-ADObject -MockWith { throw (New-Object -TypeName Microsoft.ActiveDirectory.Management.ADException)} + Mock -CommandName Restore-ADObject -MockWith { + throw New-Object -TypeName Microsoft.ActiveDirectory.Management.ADException + } - {Restore-ADCommonObject -Identity $restoreIdentity -ObjectClass $restoreObjectClass} | Should -Throw -ExceptionType ([System.InvalidOperationException]) + { + Restore-ADCommonObject -Identity $restoreIdentity -ObjectClass $restoreObjectClass + } | Should -Throw ($script:localizedData.RecycleBinRestoreFailed -f $restoreIdentity, $restoreObjectClass) } } From e881edb2b6a3ca5e8194e69aa89db24972bdee5a Mon Sep 17 00:00:00 2001 From: Johan Ljunggren Date: Sun, 7 Jul 2019 12:43:38 +0200 Subject: [PATCH 4/5] Cleanup for issue #332 --- CHANGELOG.md | 5 +++++ Tests/Unit/MSFT_xADRecycleBin.Tests.ps1 | 6 ------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bac987c8..0b305b736 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,11 @@ ([issue #402](https://github.com/PowerShell/xActiveDirectory/issues/402)). - Fixes ChangePasswordAtLogon Property to be only set to `true` at User Creation ([issue #414](https://github.com/PowerShell/xActiveDirectory/issues/414)). +- xADDomain + - Updated tests and replaced `Write-Error` with `throw` + ([issue #332](https://github.com/PowerShell/xActiveDirectory/pull/332)). +- Changes to xADRecycleBin + - Updated tests and remove unnecessary mocks of `Write-Error`. ## 3.0.0.0 diff --git a/Tests/Unit/MSFT_xADRecycleBin.Tests.ps1 b/Tests/Unit/MSFT_xADRecycleBin.Tests.ps1 index 0676ff77f..adcaf59f0 100644 --- a/Tests/Unit/MSFT_xADRecycleBin.Tests.ps1 +++ b/Tests/Unit/MSFT_xADRecycleBin.Tests.ps1 @@ -111,8 +111,6 @@ try } Context 'When Get-AdObject throws an exception' { - Mock -CommandName Write-Error - It 'Should throw ADIdentityNotFoundException' { Mock -CommandName Get-ADObject -MockWith { throw (New-Object -TypeName Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException) @@ -167,8 +165,6 @@ try } Context 'When Get-AdObject throws an exception' { - Mock -CommandName Write-Error - It 'Should throw ADIdentityNotFoundException' { Mock -CommandName Get-ADObject -MockWith { throw (New-Object -TypeName Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException) @@ -231,8 +227,6 @@ try } Context 'When Get-AdForest throws an exception' { - Mock -CommandName Write-Error - It 'Should throw ADIdentityNotFoundException' { Mock -CommandName Get-ADForest -MockWith { throw (New-Object -TypeName Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException) From 7f63fcbfc5cad3e128f65734abcec75ce0d6632d Mon Sep 17 00:00:00 2001 From: Johan Ljunggren Date: Thu, 11 Jul 2019 09:52:56 +0200 Subject: [PATCH 5/5] Fix review comments at r1 --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b305b736..088b8a21a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ ([issue #410](https://github.com/PowerShell/xActiveDirectory/issues/410)). - Changes to xADOrganizationalUnit - Catch exception when the path property specifies a non-existing path - ([issue #408](https://github.com/PowerShell/xActiveDirectory/issues/408)) + ([issue #408](https://github.com/PowerShell/xActiveDirectory/issues/408)). - Changes to xADUser - Fixes exception when creating a user with an empty string property ([issue #407](https://github.com/PowerShell/xActiveDirectory/issues/407)). @@ -83,7 +83,7 @@ - Removed unused legacy test files from the root of the repository. - Updated Example List README with missing resources. - Added missing examples for xADReplicationSubnet, xADServicePrincipalName - and xWaitForADDomain. ([issue #395](https://github.com/PowerShell/xActiveDirectory/issues/395)). + and xWaitForADDomain ([issue #395](https://github.com/PowerShell/xActiveDirectory/issues/395)). - Changes to xADComputer - Refactored the resource and the unit tests. - BREAKING CHANGE: The `Enabled` property is **DEPRECATED** and is no