From 184058a3cdeca03862fbe754d4ca8cb0c79eb1d8 Mon Sep 17 00:00:00 2001 From: Johan Ljunggren Date: Sun, 1 Sep 2019 14:40:39 +0200 Subject: [PATCH] Fix issue #151, issue #189, issue #493 --- CHANGELOG.md | 13 + DSCResources/MSFT_ADGroup/MSFT_ADGroup.psm1 | 242 ++++++++++-------- .../MSFT_ADGroup/MSFT_ADGroup.schema.mof | 1 + .../en-US/MSFT_ADGroup.strings.psd1 | 2 +- .../ActiveDirectoryDsc.Common.psm1 | 77 +++--- .../ActiveDirectoryDsc.Common.strings.psd1 | 2 +- .../MSFT_ADGroup.Integration.Tests.ps1 | 85 +++++- Tests/Integration/MSFT_ADGroup.config.ps1 | 37 ++- Tests/Unit/ActiveDirectory.Common.Tests.ps1 | 6 - Tests/Unit/MSFT_ADGroup.Tests.ps1 | 1 + 10 files changed, 305 insertions(+), 161 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2866910a..c4e7e3707 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,12 @@ authentication exceptions when the credentials cannot be authenticated. - Updated the function `Test-ADReplicationSite` to make the parameter `Credential` mandatory. + - Update helper function `Add-ADCommonGroupMember` to reduce duplicated + code, and add an evaluation if `Members` is empty. + - Updated helper function `Restore-ADCommonObject` to write out a verbose + message when no object was found in the recycle bin. + - Updated helper function `Assert-MemberParameters` to not throw an error + if the parameter `Members` is en empty array. - Changes to WaitForADDomain - Correct grammar issues in example descriptions. - An optional parameter `WaitForValidCredentials` can be set to $true @@ -70,6 +76,13 @@ - Now Get-TargetResource returns correct value when the group does not exist. - Added integration tests ([issue #350](https://github.com/PowerShell/ActiveDirectoryDsc/issues/350)). + - Added a read-only property `DistinguishedName`. + - Refactor the function `Set-TargetResource` to use the function + `Get-TargetResource` so that `Set-TargetResource` can correctly throw + an error when something goes wrong ([issue #151](https://github.com/PowerShell/ActiveDirectoryDsc/issues/151), + [issue #493](https://github.com/PowerShell/ActiveDirectoryDsc/issues/493)). + - It is now possible to enforce a group with no members by using + `Members = @()` in a configuration ([issue #189](https://github.com/PowerShell/xActiveDirectory/issues/189)). ## 4.0.0.0 diff --git a/DSCResources/MSFT_ADGroup/MSFT_ADGroup.psm1 b/DSCResources/MSFT_ADGroup/MSFT_ADGroup.psm1 index 8cb50c106..2ca6f3ab8 100644 --- a/DSCResources/MSFT_ADGroup/MSFT_ADGroup.psm1 +++ b/DSCResources/MSFT_ADGroup/MSFT_ADGroup.psm1 @@ -162,13 +162,14 @@ function Get-TargetResource MembershipAttribute = $MembershipAttribute ManagedBy = $null Notes = $null + DistinguishedName = $null } - $adGroupParameters = Get-ADCommonParameters @PSBoundParameters + $commonParameters = Get-ADCommonParameters @PSBoundParameters try { - $adGroup = Get-ADGroup @adGroupParameters -Properties @( + $adGroup = Get-ADGroup @commonParameters -Properties @( 'Name', 'GroupScope', 'GroupCategory', @@ -184,12 +185,13 @@ function Get-TargetResource if ($adGroup) { # Retrieve the current list of members, returning the specified membership attribute - [System.Array] $adGroupMembers = (Get-ADGroupMember @adGroupParameters).$MembershipAttribute + [System.Array] $adGroupMembers = (Get-ADGroupMember @commonParameters).$MembershipAttribute $getTargetResourceReturnValue['Ensure'] = 'Present' $getTargetResourceReturnValue['GroupName'] = $adGroup.Name $getTargetResourceReturnValue['GroupScope'] = $adGroup.GroupScope $getTargetResourceReturnValue['Category'] = $adGroup.GroupCategory + $getTargetResourceReturnValue['DistinguishedName'] = $adGroup.DistinguishedName $getTargetResourceReturnValue['Path'] = Get-ADObjectParentDN -DN $adGroup.DistinguishedName $getTargetResourceReturnValue['Description'] = $adGroup.Description $getTargetResourceReturnValue['DisplayName'] = $adGroup.DisplayName @@ -347,24 +349,25 @@ function Test-TargetResource ) # Validate parameters before we even attempt to retrieve anything - $assertMemberParameters = @{ } + $assertMemberParameters = @{} - if ($PSBoundParameters.ContainsKey('Members') -and -not [system.string]::IsNullOrEmpty($Members)) + # Members parameter should always be tested to enforce an empty group (issue #189) + if ($PSBoundParameters.ContainsKey('Members')) { $assertMemberParameters['Members'] = $Members } - if ($PSBoundParameters.ContainsKey('MembersToInclude') -and -not [system.string]::IsNullOrEmpty($MembersToInclude)) + if ($PSBoundParameters.ContainsKey('MembersToInclude') -and -not [System.String]::IsNullOrEmpty($MembersToInclude)) { $assertMemberParameters['MembersToInclude'] = $MembersToInclude } - if ($PSBoundParameters.ContainsKey('MembersToExclude') -and -not [system.string]::IsNullOrEmpty($MembersToExclude)) + if ($PSBoundParameters.ContainsKey('MembersToExclude') -and -not [System.String]::IsNullOrEmpty($MembersToExclude)) { $assertMemberParameters['MembersToExclude'] = $MembersToExclude } - Assert-MemberParameters @assertMemberParameters -ErrorAction Stop + Assert-MemberParameters @assertMemberParameters $targetResource = Get-TargetResource @PSBoundParameters @@ -570,89 +573,100 @@ function Set-TargetResource Assert-Module -ModuleName 'ActiveDirectory' - $adGroupParams = Get-ADCommonParameters @PSBoundParameters + $assertMemberParameters = @{} - try + # Members parameter should always be added to enforce an empty group (issue #189) + if ($PSBoundParameters.ContainsKey('Members')) { - if ($MembershipAttribute -eq 'DistinguishedName') - { - $allMembers = $Members + $MembersToInclude + $MembersToExclude + $assertMemberParameters['Members'] = $Members + } - $groupMemberDomains = @() + if ($PSBoundParameters.ContainsKey('MembersToInclude') -and -not [System.String]::IsNullOrEmpty($MembersToInclude)) + { + $assertMemberParameters['MembersToInclude'] = $MembersToInclude + } - foreach ($member in $allMembers) - { - $groupMemberDomains += Get-ADDomainNameFromDistinguishedName -DistinguishedName $member - } + if ($PSBoundParameters.ContainsKey('MembersToExclude') -and -not [System.String]::IsNullOrEmpty($MembersToExclude)) + { + $assertMemberParameters['MembersToExclude'] = $MembersToExclude + } - $uniqueGroupMemberDomainCount = $groupMemberDomains | - Select-Object -Unique + Assert-MemberParameters @assertMemberParameters - $GroupMemberDomainCount = $uniqueGroupMemberDomainCount.count + if ($MembershipAttribute -eq 'DistinguishedName') + { + $allMembers = $Members + $MembersToInclude + $MembersToExclude - if ($GroupMemberDomainCount -gt 1 -or ($groupMemberDomains -ine (Get-DomainName)).Count -gt 0) - { - Write-Verbose -Message ($script:localizedData.GroupMembershipMultipleDomains -f $GroupMemberDomainCount) - $MembersInMultipleDomains = $true - } + $groupMemberDomains = @() + + foreach ($member in $allMembers) + { + $groupMemberDomains += Get-ADDomainNameFromDistinguishedName -DistinguishedName $member } - $adGroup = Get-ADGroup @adGroupParams -Properties @( - 'Name', - 'GroupScope', - 'GroupCategory', - 'DistinguishedName', - 'Description', - 'DisplayName', - 'ManagedBy', - 'Info' - ) + $uniqueGroupMemberDomainCount = $groupMemberDomains | + Select-Object -Unique + + $GroupMemberDomainCount = $uniqueGroupMemberDomainCount.count + + if ($GroupMemberDomainCount -gt 1 -or ($groupMemberDomains -ine (Get-DomainName)).Count -gt 0) + { + Write-Verbose -Message ($script:localizedData.GroupMembershipMultipleDomains -f $GroupMemberDomainCount) + $membersInMultipleDomains = $true + } + } + + $commonParameters = Get-ADCommonParameters @PSBoundParameters + $getTargetResourceResult = Get-TargetResource @PSBoundParameters + + if ($getTargetResourceResult.Ensure -eq 'Present') + { if ($Ensure -eq 'Present') { - $setADGroupParams = $adGroupParams.Clone() - $setADGroupParams['Identity'] = $adGroup.DistinguishedName + $setADGroupParams = $commonParameters.Clone() + $setADGroupParams['Identity'] = $getTargetResourceResult.DistinguishedName # Update existing group properties - if ($PSBoundParameters.ContainsKey('Category') -and $Category -ne $adGroup.GroupCategory) + if ($PSBoundParameters.ContainsKey('Category') -and $Category -ne $getTargetResourceResult.Category) { Write-Verbose -Message ($script:localizedData.UpdatingGroupProperty -f 'Category', $Category) $setADGroupParams['GroupCategory'] = $Category } - if ($PSBoundParameters.ContainsKey('GroupScope') -and $GroupScope -ne $adGroup.GroupScope) + if ($PSBoundParameters.ContainsKey('GroupScope') -and $GroupScope -ne $getTargetResourceResult.GroupScope) { # Cannot change DomainLocal to Global or vice versa directly. Need to change them to a Universal group first! - Set-ADGroup -Identity $adGroup.DistinguishedName -GroupScope Universal + Set-ADGroup -Identity $getTargetResourceResult.DistinguishedName -GroupScope 'Universal' -ErrorAction 'Stop' Write-Verbose -Message ($script:localizedData.UpdatingGroupProperty -f 'GroupScope', $GroupScope) $setADGroupParams['GroupScope'] = $GroupScope } - if ($Description -and ($Description -ne $adGroup.Description)) + if ($Description -and ($Description -ne $getTargetResourceResult.Description)) { Write-Verbose -Message ($script:localizedData.UpdatingGroupProperty -f 'Description', $Description) $setADGroupParams['Description'] = $Description } - if ($DisplayName -and ($DisplayName -ne $adGroup.DisplayName)) + if ($DisplayName -and ($DisplayName -ne $getTargetResourceResult.DisplayName)) { Write-Verbose -Message ($script:localizedData.UpdatingGroupProperty -f 'DisplayName', $DisplayName) $setADGroupParams['DisplayName'] = $DisplayName } - if ($ManagedBy -and ($ManagedBy -ne $adGroup.ManagedBy)) + if ($ManagedBy -and ($ManagedBy -ne $getTargetResourceResult.ManagedBy)) { Write-Verbose -Message ($script:localizedData.UpdatingGroupProperty -f 'ManagedBy', $ManagedBy) $setADGroupParams['ManagedBy'] = $ManagedBy } - if ($Notes -and ($Notes -ne $adGroup.Info)) + if ($Notes -and ($Notes -ne $getTargetResourceResult.Notes)) { Write-Verbose -Message ($script:localizedData.UpdatingGroupProperty -f 'Notes', $Notes) @@ -663,64 +677,70 @@ function Set-TargetResource Write-Verbose -Message ($script:localizedData.UpdatingGroup -f $GroupName) - Set-ADGroup @setADGroupParams + Set-ADGroup @setADGroupParams -ErrorAction 'Stop' + + $groupParentDistinguishedName = Get-ADObjectParentDN -DN $getTargetResourceResult.DistinguishedName # Move group if the path is not correct - if ($Path -and ($Path -ne (Get-ADObjectParentDN -DN $adGroup.DistinguishedName))) + if ($Path -and $Path -ne $groupParentDistinguishedName) { Write-Verbose -Message ($script:localizedData.MovingGroup -f $GroupName, $Path) - $moveADObjectParams = $adGroupParams.Clone() - $moveADObjectParams['Identity'] = $adGroup.DistinguishedName + $moveADObjectParams = $commonParameters.Clone() + $moveADObjectParams['Identity'] = $getTargetResourceResult.DistinguishedName + $moveADObjectParams['TargetPath'] = $Path + $moveADObjectParams['ErrorAction'] = 'Stop' - Move-ADObject @moveADObjectParams -TargetPath $Path + Move-ADObject @moveADObjectParams } - Write-Verbose -Message ($script:localizedData.RetrievingGroupMembers -f $MembershipAttribute) + if ($assertMemberParameters.Count -gt 0) + { + Write-Verbose -Message ($script:localizedData.RetrievingGroupMembers -f $MembershipAttribute) - $adGroupMembers = (Get-ADGroupMember @adGroupParams).$MembershipAttribute + $adGroupMembers = (Get-ADGroupMember @commonParameters).$MembershipAttribute - if (-not (Test-Members -ExistingMembers $adGroupMembers -Members $Members -MembersToInclude $MembersToInclude -MembersToExclude $MembersToExclude)) - { - <# - The fact that we're in the Set method, there is no need to - validate the parameter combination as this was performed in - the Test method. - #> - if ($PSBoundParameters.ContainsKey('Members') -and -not [system.string]::IsNullOrEmpty($Members)) - { - # Remove all existing first and add explicit members - $Members = Remove-DuplicateMembers -Members $Members + $assertMemberParameters['ExistingMembers'] = $adGroupMembers - # We can only remove members if there are members already in the group! - if ($adGroupMembers.Count -gt 0) + # Return $false if the members mismatch. + if (-not (Test-Members @assertMemberParameters)) + { + # Members parameter should always be enforce if it is bound (issue #189) + if ($PSBoundParameters.ContainsKey('Members')) { - Write-Verbose -Message ($script:localizedData.RemovingGroupMembers -f $adGroupMembers.Count, $GroupName) + # Remove all existing first and add explicit members + $Members = Remove-DuplicateMembers -Members $Members - Remove-ADGroupMember @adGroupParams -Members $adGroupMembers -Confirm:$false - } + # We can only remove members if there are members already in the group! + if ($adGroupMembers.Count -gt 0) + { + Write-Verbose -Message ($script:localizedData.RemovingGroupMembers -f $adGroupMembers.Count, $GroupName) - Write-Verbose -Message ($script:localizedData.AddingGroupMembers -f $Members.Count, $GroupName) + Remove-ADGroupMember @commonParameters -Members $adGroupMembers -Confirm:$false -ErrorAction 'Stop' + } - Add-ADCommonGroupMember -Parameter $adGroupParams -Members $Members -MembersInMultipleDomains:$MembersInMultipleDomains - } + Write-Verbose -Message ($script:localizedData.AddingGroupMembers -f $Members.Count, $GroupName) - if ($PSBoundParameters.ContainsKey('MembersToInclude') -and -not [system.string]::IsNullOrEmpty($MembersToInclude)) - { - $MembersToInclude = Remove-DuplicateMembers -Members $MembersToInclude + Add-ADCommonGroupMember -Parameter $commonParameters -Members $Members -MembersInMultipleDomains:$membersInMultipleDomains + } - Write-Verbose -Message ($script:localizedData.AddingGroupMembers -f $MembersToInclude.Count, $GroupName) + if ($PSBoundParameters.ContainsKey('MembersToInclude') -and -not [System.String]::IsNullOrEmpty($MembersToInclude)) + { + $MembersToInclude = Remove-DuplicateMembers -Members $MembersToInclude - Add-ADCommonGroupMember -Parameter $adGroupParams -Members $MembersToInclude -MembersInMultipleDomains:$MembersInMultipleDomains - } + Write-Verbose -Message ($script:localizedData.AddingGroupMembers -f $MembersToInclude.Count, $GroupName) - if ($PSBoundParameters.ContainsKey('MembersToExclude') -and -not [system.string]::IsNullOrEmpty($MembersToExclude)) - { - $MembersToExclude = Remove-DuplicateMembers -Members $MembersToExclude + Add-ADCommonGroupMember -Parameter $commonParameters -Members $MembersToInclude -MembersInMultipleDomains:$membersInMultipleDomains + } + + if ($PSBoundParameters.ContainsKey('MembersToExclude') -and -not [System.String]::IsNullOrEmpty($MembersToExclude)) + { + $MembersToExclude = Remove-DuplicateMembers -Members $MembersToExclude - Write-Verbose -Message ($script:localizedData.RemovingGroupMembers -f $MembersToExclude.Count, $GroupName) + Write-Verbose -Message ($script:localizedData.RemovingGroupMembers -f $MembersToExclude.Count, $GroupName) - Remove-ADGroupMember @adGroupParams -Members $MembersToExclude -Confirm:$false + Remove-ADGroupMember @commonParameters -Members $MembersToExclude -Confirm:$false -ErrorAction 'Stop' + } } } } @@ -729,38 +749,42 @@ function Set-TargetResource # Remove existing group Write-Verbose -Message ($script:localizedData.RemovingGroup -f $GroupName) - Remove-ADGroup @adGroupParams -Confirm:$false + Remove-ADGroup @commonParameters -Confirm:$false -ErrorAction 'Stop' } } - catch [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException] + else { - # The AD group doesn't exist + # The Active Directory group does not exist, check if it should. if ($Ensure -eq 'Present') { - Write-Verbose -Message ($script:localizedData.GroupNotFound -f $GroupName) + $commonParametersUsingName = Get-ADCommonParameters @PSBoundParameters -UseNameParameter - $adGroupParams = Get-ADCommonParameters @PSBoundParameters -UseNameParameter + $newAdGroupParameters = $commonParametersUsingName.Clone() + $newAdGroupParameters['GroupCategory'] = $Category + $newAdGroupParameters['GroupScope'] = $GroupScope - if ($Description) + if ($PSBoundParameters.ContainsKey('Description')) { - $adGroupParams['Description'] = $Description + $newAdGroupParameters['Description'] = $Description } - if ($DisplayName) + if ($PSBoundParameters.ContainsKey('DisplayName')) { - $adGroupParams['DisplayName'] = $DisplayName + $newAdGroupParameters['DisplayName'] = $DisplayName } - if ($ManagedBy) + if ($PSBoundParameters.ContainsKey('ManagedBy')) { - $adGroupParams['ManagedBy'] = $ManagedBy + $newAdGroupParameters['ManagedBy'] = $ManagedBy } - if ($Path) + if ($PSBoundParameters.ContainsKey('Path')) { - $adGroupParams['Path'] = $Path + $newAdGroupParameters['Path'] = $Path } + $adGroup = $null + # Create group. Try to restore account first if it exists. if ($RestoreFromRecycleBin) { @@ -768,49 +792,57 @@ function Set-TargetResource $restoreParams = Get-ADCommonParameters @PSBoundParameters - $adGroup = Restore-ADCommonObject @restoreParams -ObjectClass Group -ErrorAction Stop + $adGroup = Restore-ADCommonObject @restoreParams -ObjectClass 'Group' } + <# + Check if the Active Directory group was restored, if not create + the group. + #> if (-not $adGroup) { Write-Verbose -Message ($script:localizedData.AddingGroup -f $GroupName) - $adGroup = New-ADGroup @adGroupParams -GroupCategory $Category -GroupScope $GroupScope -PassThru + $adGroup = New-ADGroup @newAdGroupParameters -PassThru -ErrorAction 'Stop' } <# Only the New-ADGroup cmdlet takes a -Name parameter. Refresh the parameters with the -Identity parameter rather than -Name. #> - $adGroupParams = Get-ADCommonParameters @PSBoundParameters + $commonParameters = Get-ADCommonParameters @PSBoundParameters - if ($Notes) + if ($PSBoundParameters.ContainsKey('Notes')) { # Can't set the Notes field when creating the group Write-Verbose -Message ($script:localizedData.UpdatingGroupProperty -f 'Notes', $Notes) - $setADGroupParams = $adGroupParams.Clone() + $setADGroupParams = $commonParameters.Clone() $setADGroupParams['Identity'] = $adGroup.DistinguishedName + $setADGroupParams['ErrorAction'] = 'Stop' + $setADGroupParams['Add'] = @{ + Info = $Notes + } - Set-ADGroup @setADGroupParams -Add @{ Info = $Notes } + Set-ADGroup @setADGroupParams } # Add the required members - if ($PSBoundParameters.ContainsKey('Members') -and -not [system.string]::IsNullOrEmpty($Members)) + if ($PSBoundParameters.ContainsKey('Members') -and -not [System.String]::IsNullOrEmpty($Members)) { $Members = Remove-DuplicateMembers -Members $Members Write-Verbose -Message ($script:localizedData.AddingGroupMembers -f $Members.Count, $GroupName) - Add-ADCommonGroupMember -Parameter $adGroupParams -Members $Members -MembersInMultipleDomains:$MembersInMultipleDomains + Add-ADCommonGroupMember -Parameter $commonParameters -Members $Members -MembersInMultipleDomains:$membersInMultipleDomains } - elseif ($PSBoundParameters.ContainsKey('MembersToInclude') -and -not [system.string]::IsNullOrEmpty($MembersToInclude)) + elseif ($PSBoundParameters.ContainsKey('MembersToInclude') -and -not [System.String]::IsNullOrEmpty($MembersToInclude)) { $MembersToInclude = Remove-DuplicateMembers -Members $MembersToInclude Write-Verbose -Message ($script:localizedData.AddingGroupMembers -f $MembersToInclude.Count, $GroupName) - Add-ADCommonGroupMember -Parameter $adGroupParams -Members $MembersToInclude -MembersInMultipleDomains:$MembersInMultipleDomains + Add-ADCommonGroupMember -Parameter $commonParameters -Members $MembersToInclude -MembersInMultipleDomains:$membersInMultipleDomains } } } #end catch diff --git a/DSCResources/MSFT_ADGroup/MSFT_ADGroup.schema.mof b/DSCResources/MSFT_ADGroup/MSFT_ADGroup.schema.mof index 3084043dd..7021d37c2 100644 --- a/DSCResources/MSFT_ADGroup/MSFT_ADGroup.schema.mof +++ b/DSCResources/MSFT_ADGroup/MSFT_ADGroup.schema.mof @@ -17,4 +17,5 @@ class MSFT_ADGroup : OMI_BaseResource [Write, Description("Active Directory managed by attribute specified as a DistinguishedName.")] String ManagedBy; [Write, Description("Active Directory group notes field.")] String Notes; [Write, Description("Try to restore the group from the recycle bin before creating a new one.")] Boolean RestoreFromRecycleBin; + [Read, Description("Returns the distinguished name of the Active Directory group.")] String DistinguishedName; }; diff --git a/DSCResources/MSFT_ADGroup/en-US/MSFT_ADGroup.strings.psd1 b/DSCResources/MSFT_ADGroup/en-US/MSFT_ADGroup.strings.psd1 index 0080cc028..7194ef50a 100644 --- a/DSCResources/MSFT_ADGroup/en-US/MSFT_ADGroup.strings.psd1 +++ b/DSCResources/MSFT_ADGroup/en-US/MSFT_ADGroup.strings.psd1 @@ -4,7 +4,7 @@ ConvertFrom-StringData @' GroupMembershipNotDesiredState = Group membership is NOT in the desired state. (ADG0002) AddingGroupMembers = Adding '{0}' member(s) to AD group '{1}'. (ADG0003) RemovingGroupMembers = Removing '{0}' member(s) from AD group '{1}'. (ADG0004) - AddingGroup = Adding AD Group '{0}'. (ADG0005) + AddingGroup = Creating AD Group '{0}'. (ADG0005) UpdatingGroup = Updating AD Group '{0}'. (ADG0006) RemovingGroup = Removing AD Group '{0}'. (ADG0007) MovingGroup = Moving AD Group '{0}' to '{1}'. (ADG0008) diff --git a/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 b/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 index 5b83fa27c..3edcc63d4 100644 --- a/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 +++ b/Modules/ActiveDirectoryDsc.Common/ActiveDirectoryDsc.Common.psm1 @@ -664,7 +664,6 @@ function Assert-MemberParameters param ( [Parameter()] - [ValidateNotNull()] [System.String[]] $Members, @@ -687,12 +686,6 @@ function Assert-MemberParameters $errorMessage = $script:localizedData.MembersAndIncludeExcludeError -f 'Members', 'MembersToInclude', 'MembersToExclude' New-InvalidArgumentException -ArgumentName 'Members' -Message $errorMessage } - - if ($Members.Length -eq 0) - { - $errorMessage = $script:localizedData.MembersIsNullError -f 'Members', 'MembersToInclude', 'MembersToExclude' - New-InvalidArgumentException -ArgumentName 'Members' -Message $errorMessage - } } if ($PSBoundParameters.ContainsKey('MembersToInclude')) @@ -1350,6 +1343,7 @@ function Restore-ADCommonObject $restoreParams['ErrorAction'] = 'Stop' $restoreParams['Identity'] = $restorableObject.DistinguishedName $restoredObject = Restore-ADObject @restoreParams + Write-Verbose -Message ($script:localizedData.RecycleBinRestoreSuccessful -f $Identity, $ObjectClass) -Verbose } catch [Microsoft.ActiveDirectory.Management.ADException] @@ -1359,6 +1353,10 @@ function Restore-ADCommonObject New-InvalidOperationException -Message $errorMessage -ErrorRecord $_ } } + else + { + Write-Verbose -Message ($script:localizedData.NoObjectFoundInRecycleBin) -Verbose + } return $restoredObject } @@ -1454,41 +1452,52 @@ function Add-ADCommonGroupMember Assert-Module -ModuleName ActiveDirectory - if ($MembersInMultipleDomains.IsPresent) + if ($Members) { - foreach ($member in $Members) + if ($MembersInMultipleDomains.IsPresent) { - $memberDomain = Get-ADDomainNameFromDistinguishedName -DistinguishedName $member - - if (-not $memberDomain) + foreach ($member in $Members) { - $errorMessage = $script:localizedData.EmptyDomainError -f $member, $Parameters.Identity - New-InvalidOperationException -Message $errorMessage - } + $memberDomain = Get-ADDomainNameFromDistinguishedName -DistinguishedName $member - Write-Verbose -Message ($script:localizedData.AddingGroupMember -f $member, $memberDomain, $Parameters.Identity) + if (-not $memberDomain) + { + $errorMessage = $script:localizedData.EmptyDomainError -f $member, $Parameters.Identity + New-InvalidOperationException -Message $errorMessage + } - $memberObjectClass = (Get-ADObject -Identity $member -Server $memberDomain -Properties ObjectClass).ObjectClass + Write-Verbose -Message ($script:localizedData.AddingGroupMember -f $member, $memberDomain, $Parameters.Identity) - if ($memberObjectClass -eq 'computer') - { - $memberObject = Get-ADComputer -Identity $member -Server $memberDomain - } - elseif ($memberObjectClass -eq 'group') - { - $memberObject = Get-ADGroup -Identity $member -Server $memberDomain - } - elseif ($memberObjectClass -eq 'user') - { - $memberObject = Get-ADUser -Identity $member -Server $memberDomain - } + $commonParameters = @{ + Identity = $member + Server = $memberDomain + ErrorAction = 'Stop' + } + + $activeDirectoryObject = Get-ADObject @commonParameters -Properties @('ObjectClass') + + $memberObjectClass = $activeDirectoryObject.ObjectClass + + if ($memberObjectClass -eq 'computer') + { + $memberObject = Get-ADComputer @commonParameters + } + elseif ($memberObjectClass -eq 'group') + { + $memberObject = Get-ADGroup @commonParameters + } + elseif ($memberObjectClass -eq 'user') + { + $memberObject = Get-ADUser @commonParameters + } - Add-ADGroupMember @Parameters -Members $memberObject + Add-ADGroupMember @Parameters -Members $memberObject -ErrorAction 'Stop' + } + } + else + { + Add-ADGroupMember @Parameters -Members $Members -ErrorAction 'Stop' } - } - else - { - Add-ADGroupMember @Parameters -Members $Members } } diff --git a/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1 b/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1 index d9fca561b..14d6d50e8 100644 --- a/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1 +++ b/Modules/ActiveDirectoryDsc.Common/en-US/ActiveDirectoryDsc.Common.strings.psd1 @@ -12,7 +12,6 @@ ConvertFrom-StringData @' UnableToCompareType = Unable to compare the type {0} as it is not handled by the Test-DscPropertyState cmdlet. (ADCOMMON0009) ModuleNotFoundError = Please ensure that the PowerShell module for role '{0}' is installed. (ADCOMMON0010) MembersAndIncludeExcludeError = The '{0}' and '{1}' and/or '{2}' parameters conflict. The '{0}' parameter should not be used in any combination with the '{1}' and '{2}' parameters. (ADCOMMON0011) - 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 = Failed restoring {0} ({1}) from the recycle bin. (ADCOMMON0017) @@ -51,4 +50,5 @@ ConvertFrom-StringData @' SearchingForDomainController = Searching for a domain controller in the domain '{0}'. (ADCOMMON0052) SearchingForDomainControllerInSite = Searching for a domain controller in the site '{0}' in the domain '{1}'. (ADCOMMON0053) IgnoreCredentialError = Suppressing the credential error '{0}' with the message '{1}'. (ADCOMMON0054) + NoObjectFoundInRecycleBin = Did not find a restorable object in the recycle bin. (ADCOMMON0055) '@ diff --git a/Tests/Integration/MSFT_ADGroup.Integration.Tests.ps1 b/Tests/Integration/MSFT_ADGroup.Integration.Tests.ps1 index 810f7f107..3f5d79937 100644 --- a/Tests/Integration/MSFT_ADGroup.Integration.Tests.ps1 +++ b/Tests/Integration/MSFT_ADGroup.Integration.Tests.ps1 @@ -76,7 +76,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group1_Name $resourceCurrentState.GroupScope | Should -Be 'Global' $resourceCurrentState.Category | Should -Be 'Security' - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -137,7 +137,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group2_Name $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group2_Scope $resourceCurrentState.Category | Should -Be 'Security' - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -198,7 +198,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group3_Name $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group3_Scope $resourceCurrentState.Category | Should -Be 'Security' - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -259,7 +259,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group3_Name $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group3_Scope $resourceCurrentState.Category | Should -Be 'Distribution' - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -320,7 +320,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group4_Name $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group4_Scope $resourceCurrentState.Category | Should -Be 'Security' - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -381,7 +381,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group4_Name $resourceCurrentState.GroupScope | Should -Be 'Global' $resourceCurrentState.Category | Should -Be 'Security' - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -503,7 +503,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group1_Name $resourceCurrentState.GroupScope | Should -Be 'Global' $resourceCurrentState.Category | Should -Be 'Security' - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -564,7 +564,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group1_Name $resourceCurrentState.GroupScope | Should -Be 'Global' $resourceCurrentState.Category | Should -Be 'Security' - $resourceCurrentState.Path | Should -Be 'CN=Computers,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Computers,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -Be 'A DSC description' $resourceCurrentState.DisplayName | Should -Be 'DSC Group 1' $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -575,7 +575,7 @@ try $resourceCurrentState.MembersToInclude | Should -BeNullOrEmpty $resourceCurrentState.MembersToExclude | Should -BeNullOrEmpty $resourceCurrentState.MembershipAttribute | Should -Be 'SamAccountName' - $resourceCurrentState.ManagedBy | Should -Be 'CN=Administrator,CN=Users,DC=contoso,DC=com' + $resourceCurrentState.ManagedBy | Should -Be ('CN=Administrator,CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Notes | Should -Be 'Notes for this group' $resourceCurrentState.RestoreFromRecycleBin | Should -BeNullOrEmpty } @@ -627,7 +627,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group5_Name $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group5_Scope $resourceCurrentState.Category | Should -Be $ConfigurationData.AllNodes.Group5_Category - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -689,7 +689,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group5_Name $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group5_Scope $resourceCurrentState.Category | Should -Be $ConfigurationData.AllNodes.Group5_Category - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -753,7 +753,7 @@ try $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group5_Name $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group5_Scope $resourceCurrentState.Category | Should -Be $ConfigurationData.AllNodes.Group5_Category - $resourceCurrentState.Path | Should -Be 'CN=Users,DC=contoso,DC=com' + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) $resourceCurrentState.Description | Should -BeNullOrEmpty $resourceCurrentState.DisplayName | Should -BeNullOrEmpty $resourceCurrentState.Credential | Should -BeNullOrEmpty @@ -774,6 +774,67 @@ try } } + $configurationName = "$($script:dscResourceName)_ClearMembersGroup5_Config" + + Context ('When using configuration {0}' -f $configurationName) { + It 'Should compile and apply the MOF without throwing' { + { + $configurationParameters = @{ + OutputPath = $TestDrive + # The variable $ConfigurationData was dot-sourced above. + ConfigurationData = $ConfigurationData + } + + & $configurationName @configurationParameters + + $startDscConfigurationParameters = @{ + Path = $TestDrive + ComputerName = 'localhost' + Wait = $true + Verbose = $true + Force = $true + ErrorAction = 'Stop' + } + + Start-DscConfiguration @startDscConfigurationParameters + } | Should -Not -Throw + } + + It 'Should be able to call Get-DscConfiguration without throwing' { + { + $script:currentConfiguration = Get-DscConfiguration -Verbose -ErrorAction Stop + } | Should -Not -Throw + } + + It 'Should have set the resource and all the parameters should match' { + $resourceCurrentState = $script:currentConfiguration | Where-Object -FilterScript { + $_.ConfigurationName -eq $configurationName ` + -and $_.ResourceId -eq $resourceId + } + + $resourceCurrentState.Ensure | Should -Be 'Present' + $resourceCurrentState.GroupName | Should -Be $ConfigurationData.AllNodes.Group5_Name + $resourceCurrentState.GroupScope | Should -Be $ConfigurationData.AllNodes.Group5_Scope + $resourceCurrentState.Category | Should -Be $ConfigurationData.AllNodes.Group5_Category + $resourceCurrentState.Path | Should -Be ('CN=Users,{0}' -f $ConfigurationData.AllNodes.DomainDistinguishedName) + $resourceCurrentState.Description | Should -BeNullOrEmpty + $resourceCurrentState.DisplayName | Should -BeNullOrEmpty + $resourceCurrentState.Credential | Should -BeNullOrEmpty + $resourceCurrentState.DomainController | Should -BeNullOrEmpty + $resourceCurrentState.Members | Should -BeNullOrEmpty + $resourceCurrentState.MembersToInclude | Should -BeNullOrEmpty + $resourceCurrentState.MembersToExclude | Should -BeNullOrEmpty + $resourceCurrentState.MembershipAttribute | Should -Be 'SamAccountName' + $resourceCurrentState.ManagedBy | Should -BeNullOrEmpty + $resourceCurrentState.Notes | Should -BeNullOrEmpty + $resourceCurrentState.RestoreFromRecycleBin | Should -BeNullOrEmpty + } + + It 'Should return $true when Test-DscConfiguration is run' { + Test-DscConfiguration -Verbose | Should -Be 'True' + } + } + $configurationName = "$($script:dscResourceName)_Cleanup_Config" Context ('When using configuration {0}' -f $configurationName) { diff --git a/Tests/Integration/MSFT_ADGroup.config.ps1 b/Tests/Integration/MSFT_ADGroup.config.ps1 index b544647a7..dcd4bffd3 100644 --- a/Tests/Integration/MSFT_ADGroup.config.ps1 +++ b/Tests/Integration/MSFT_ADGroup.config.ps1 @@ -15,6 +15,7 @@ else { $currentDomain = Get-ADDomain $netBiosDomainName = $currentDomain.NetBIOSName + $domainDistinguishedName = $currentDomain.DistinguishedName $ConfigurationData = @{ AllNodes = @( @@ -22,6 +23,10 @@ else NodeName = 'localhost' CertificateFile = $env:DscPublicCertificatePath + PsDscAllowPlainTextPassword = $true + + DomainDistinguishedName = $domainDistinguishedName + Group1_Name = 'DscGroup1' Group2_Name = 'DscGroup2' @@ -259,11 +264,11 @@ Configuration MSFT_ADGroup_UpdateGroup1_Config { Ensure = 'Present' GroupName = $Node.Group1_Name - Path = 'CN=Computers,DC=contoso,DC=com' + Path = 'CN=Computers,{0}' -f $Node.DomainDistinguishedName DisplayName = 'DSC Group 1' Description = 'A DSC description' Notes = 'Notes for this group' - ManagedBy = 'CN=Administrator,CN=Users,DC=contoso,DC=com' + ManagedBy = 'CN=Administrator,CN=Users,{0}' -f $Node.DomainDistinguishedName Members = @( 'Administrator', 'Guest' @@ -369,6 +374,34 @@ Configuration MSFT_ADGroup_EnforceMembersGroup5_Config } } +<# + .SYNOPSIS + Enforce no members in a group. + + .NOTES + Regression test for issue #189. +#> +Configuration MSFT_ADGroup_ClearMembersGroup5_Config +{ + Import-DscResource -ModuleName 'ActiveDirectoryDsc' + + node $AllNodes.NodeName + { + ADGroup 'Integration_Test' + { + GroupName = $Node.Group5_Name + Members = @() + + Credential = New-Object ` + -TypeName System.Management.Automation.PSCredential ` + -ArgumentList @( + $Node.AdministratorUserName, + (ConvertTo-SecureString -String $Node.AdministratorPassword -AsPlainText -Force) + ) + } + } +} + <# .SYNOPSIS Cleanup everything diff --git a/Tests/Unit/ActiveDirectory.Common.Tests.ps1 b/Tests/Unit/ActiveDirectory.Common.Tests.ps1 index f1bec72a7..79f359de5 100644 --- a/Tests/Unit/ActiveDirectory.Common.Tests.ps1 +++ b/Tests/Unit/ActiveDirectory.Common.Tests.ps1 @@ -897,12 +897,6 @@ InModuleScope 'ActiveDirectoryDsc.Common' { } Describe 'ActiveDirectoryDsc.Common\Assert-MemberParameters' { - It 'Should throw if parameter Members is specified but is empty' { - { - Assert-MemberParameters -Members @() - } | Should -Throw ($script:localizedData.MembersIsNullError -f 'Members', 'MembersToInclude', 'MembersToExclude') - } - It 'Should throws if both parameters Members and MembersToInclude are specified' { { Assert-MemberParameters -Members @('User1') -MembersToInclude @('User2') diff --git a/Tests/Unit/MSFT_ADGroup.Tests.ps1 b/Tests/Unit/MSFT_ADGroup.Tests.ps1 index 6b5b5b3a8..c4b5e76ed 100644 --- a/Tests/Unit/MSFT_ADGroup.Tests.ps1 +++ b/Tests/Unit/MSFT_ADGroup.Tests.ps1 @@ -344,6 +344,7 @@ try #region Function Set-TargetResource Describe 'ADGroup\Set-TargetResource' { Mock -CommandName Assert-Module -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } + Mock -CommandName Assert-MemberParameters It "Calls 'New-ADGroup' when 'Ensure' is 'Present' and the group does not exist" { Mock -CommandName Get-ADGroup -MockWith { throw New-Object Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException }