Skip to content

Commit

Permalink
Merge pull request #5129 from salbeck-sit/AADGroupUnitTests
Browse files Browse the repository at this point in the history 
AADGroup - fixes issues with code that is never executed and incorrect removal of assigned licenses
  • Loading branch information
@NikCharlebois
NikCharlebois authored Oct 2, 2024
2 parents ddf9b22 + b328025 commit f62b393
Show file tree
Hide file tree
Showing 4 changed files with 500 additions and 44 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@
* AADFeatureRolloutPolicy
* Initial release
* AADGroup
* Fixes issue with incorrect removal of assigned license(s)
FIXES [#5128](https://github.com/microsoft/Microsoft365DSC/issues/5128)
* Fixes logic to evaluate license assignments and disabled plans.
FIXES [#5101](https://github.com/microsoft/Microsoft365DSC/issues/5101)
* Fixes issue with code that is never executed
FIXES [#5001](https://github.com/microsoft/Microsoft365DSC/issues/5001)
* Adds support to assign Service Principal as members or owners.
FIXES [#4972](https://github.com/microsoft/Microsoft365DSC/issues/4972)
* AADPasswordRuleSettings
Expand Down
83 changes: 55 additions & 28 deletions Modules/Microsoft365DSC/DSCResources/MSFT_AADGroup/MSFT_AADGroup.psm1
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,10 +591,6 @@ function Set-TargetResource
-Source $MyInvocation.MyCommand.ModuleName
}
}
if ($assignedLicensesGUIDs.Length -gt 0)
{
Set-MgGroupLicense -GroupId $currentGroup.Id -AddLicenses $licensesToAdd -RemoveLicenses @()
}
}
if ($Ensure -eq 'Present')
{
Expand All @@ -620,7 +616,7 @@ function Set-TargetResource
Update-MgGroup @currentParameters | Out-Null
}

if (($licensesToAdd.Length -gt 0 -or $licensesToRemove.Length -gt 0) -and $AssignedLicenses -ne $null)
if (($licensesToAdd.Length -gt 0 -or $licensesToRemove.Length -gt 0) -and $PSBoundParameters.ContainsKey('AssignedLicenses'))
{
try
{
Expand Down Expand Up @@ -1088,11 +1084,11 @@ function Test-TargetResource
try
{
if ($null -ne $CurrentValues.AssignedLicenses -and $CurrentValues.AssignedLicenses.Length -gt 0 -and `
$null -eq $AssignedLicenses)
($PSBoundParameters.ContainsKey('AssignedLicenses') -and $null -eq $AssignedLicenses))
{
Write-Verbose -Message "The group currently has licenses assigned but it shouldn't"
Write-Verbose -Message "The group {$DisplayName} currently has licenses assigned but it shouldn't"
Write-Verbose -Message "Test-TargetResource returned $false"
$EventMessage = "Assigned Licenses for Azure AD Group {$DisplayName} were not in the desired state.`r`nThe group should not have any licenses assigned but instead contained {$($CurrentValues.AssignedLicenses.SkuId)}"
$EventMessage = "Assigned Licenses for Azure AD Group {$DisplayName} were not in the desired state.`r`nThe group should not have any licenses assigned but instead contained {$($CurrentValues.AssignedLicenses.SkuId -join ',')}"
Add-M365DSCEvent -Message $EventMessage -EntryType 'Warning' `
-EventID 1 -Source $($MyInvocation.MyCommand.Source)

Expand All @@ -1101,59 +1097,90 @@ function Test-TargetResource
elseif ($null -eq $CurrentValues.AssignedLicenses -and $null -ne $AssignedLicenses -and `
$AssignedLicenses.Length -gt 0)
{
Write-Verbose -Message "The group currently doesn't have licenses assigned but it should"
Write-Verbose -Message "The group {$DisplayName} currently doesn't have licenses assigned but it should"
Write-Verbose -Message "Test-TargetResource returned $false"
$EventMessage = "Assigned Licenses for Azure AD Group {$DisplayName} were not in the desired state.`r`nThe group doesn't not have any licenses assigned but should have {$($CurrentValues.AssignedLicenses.SkuId)}"
$EventMessage = "Assigned Licenses for Azure AD Group {$DisplayName} were not in the desired state.`r`nThe group doesn't not have any licenses assigned but should have {$($CurrentValues.AssignedLicenses.SkuId -join ',')}"
Add-M365DSCEvent -Message $EventMessage -EntryType 'Warning' `
-EventID 1 -Source $($MyInvocation.MyCommand.Source)

return $false
}
elseif ($CurrentValues.AssignedLicenses.Length -gt 0 -and $AssignedLicenses.Length -gt 0)
{
Write-Verbose -Message "Current assigned licenses and desired assigned licenses are not null"
Write-Verbose -Message "Current assigned licenses and desired assigned licenses for group {$DisplayName} are not null and will be compared"
$licensesDiff = Compare-Object -ReferenceObject ($CurrentValues.AssignedLicenses.SkuId) -DifferenceObject ($AssignedLicenses.SkuId)
if ($null -ne $licensesDiff)
{
Write-Verbose -Message "AssignedLicenses differ: $($licensesDiff | Out-String)"
Write-Verbose -Message "AssignedLicenses differ for group {$DisplayName}: $($licensesDiff | Out-String)"
Write-Verbose -Message "Test-TargetResource returned $false"
$EventMessage = "Assigned Licenses for Azure AD Group {$DisplayName} were not in the desired state.`r`nThey should contain {$($AssignedLicenses.SkuId)} but instead contained {$($CurrentValues.AssignedLicenses.SkuId)}"
$EventMessage = "Assigned Licenses for Azure AD Group {$DisplayName} were not in the desired state.`r`nThey should contain {$($AssignedLicenses.SkuId -join ',')} but instead contained {$($CurrentValues.AssignedLicenses.SkuId -join ',')}"
Add-M365DSCEvent -Message $EventMessage -EntryType 'Warning' `
-EventID 1 -Source $($MyInvocation.MyCommand.Source)

return $false
}
else
{
Write-Verbose -Message 'AssignedLicenses for Azure AD Group are the same'
Write-Verbose -Message "AssignedLicenses for Azure AD Group {$DisplayName} are the same, checking DisabledPlans"
}

# Disabled Plans
$licensesDiff = Compare-Object -ReferenceObject ($CurrentValues.AssignedLicenses.DisabledPlans) -DifferenceObject ($AssignedLicenses.DisabledPlans)
if ($null -ne $licensesDiff)
#Compare DisabledPlans for each SkuId - all SkuId's are processed regardless of result
$result = $true
foreach ($assignedLicense in $AssignedLicenses)
{
Write-Verbose -Message "DisabledPlans differ: $($licensesDiff | Out-String)"
Write-Verbose -Message "Test-TargetResource returned $false"
$EventMessage = "Disabled Plans for Azure AD Group Licenses {$DisplayName} were not in the desired state.`r`n" + `
"They should contain {$($AssignedLicenses.DisabledPlans)} but instead contained {$($CurrentValues.AssignedLicenses.DisabledPlans)}"
Add-M365DSCEvent -Message $EventMessage -EntryType 'Warning' `
-EventID 1 -Source $($MyInvocation.MyCommand.Source)

return $false
Write-Verbose "Compare DisabledPlans for SkuId $($assignedLicense.SkuId) in group {$DisplayName}"
$currentLicense = $CurrentValues.AssignedLicenses | Where-Object -FilterScript {$_.SkuId -eq $assignedLicense.SkuId}
if ($assignedLicense.DisabledPlans.Count -ne 0 -or $currentLicense.DisabledPlans.Count -ne 0)
{
try {
$licensesDiff = Compare-Object -ReferenceObject $assignedLicense.DisabledPlans -DifferenceObject $currentLicense.DisabledPlans
if ($null -ne $licensesDiff)
{
Write-Verbose -Message "DisabledPlans for SkuId $($assignedLicense.SkuId) differ: $($licensesDiff | Out-String)"
Write-Verbose -Message "Test-TargetResource returned $false"
$EventMessage = "Disabled Plans for Azure AD Group Licenses {$DisplayName} SkuId $($assignedLicense.SkuId) were not in the desired state.`r`n" + `
"They should contain {$($assignedLicense.DisabledPlans -join ',')} but instead contained {$($currentLicense.DisabledPlans -join ',')}"
Add-M365DSCEvent -Message $EventMessage -EntryType 'Warning' `
-EventID 1 -Source $($MyInvocation.MyCommand.Source)

$result = $false
}
else
{
Write-Verbose -Message "DisabledPlans for SkuId $($assignedLicense.SkuId) are the same"
}
}
catch
{
Write-Verbose -Message "Test-TargetResource returned `$false (DisabledPlans: $($_.Exception.Message))"
$result = $false
}
}
}
else
if ($true -ne $result)
{
Write-Verbose -Message 'DisabledPlans for Azure AD Group Licensing are the same'
return $result
}
}
elseif ($PSBoundParameters.ContainsKey('AssignedLicenses'))
{
Write-Verbose -Message "The group {$DisplayName} currently has licenses assigned but it shouldn't have"
Write-Verbose -Message "Test-TargetResource returned $false"
$EventMessage = "Assigned Licenses for Azure AD Group {$DisplayName} were not in the desired state.`r`nThe group has licenses assigned but shouldn't have {$($CurrentValues.AssignedLicenses.SkuId)}"
Add-M365DSCEvent -Message $EventMessage -EntryType 'Warning' `
-EventID 1 -Source $($MyInvocation.MyCommand.Source)

return $false
}
else
{
Write-Verbose -Message "Both the current and desired assigned licenses lists are empty."
Write-Verbose -Message "Both the current and desired assigned licenses lists for group {$DisplayName} are empty or not specified."
}
}
catch
{
Write-Verbose -Message "Error evaluating the AssignedLicenses: $_"
Write-Verbose -Message "Error evaluating the AssignedLicenses for group {$DisplayName}: $_"
Write-Verbose -Message "Test-TargetResource returned $false"
return $false
}
Expand Down
27 changes: 16 additions & 11 deletions Modules/Microsoft365DSC/Examples/Resources/AADGroup/2-Update.ps1
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,22 @@ Configuration Example
{
AADGroup 'MyGroups'
{
DisplayName = "DSCGroup"
Description = "Microsoft DSC Group Updated" # Updated Property
SecurityEnabled = $True
MailEnabled = $True
GroupTypes = @("Unified")
MailNickname = "M365DSC"
Members = @("AdeleV@$TenantId")
GroupAsMembers = @("Group1")
Visibility = "Private"
Owners = @("admin@$TenantId", "AdeleV@$TenantId")
Ensure = "Present"
DisplayName = "DSCGroup"
Description = "Microsoft DSC Group Updated" # Updated Property
SecurityEnabled = $True
MailEnabled = $True
GroupTypes = @("Unified")
MailNickname = "M365DSC"
Members = @("AdeleV@$TenantId")
GroupAsMembers = @("Group1")
Visibility = "Private"
Owners = @("admin@$TenantId", "AdeleV@$TenantId")
AssignedLicenses = @(
MSFT_AADGroupLicense {
SkuId = 'AAD_PREMIUM_P2'
}
)
Ensure = "Present"
ApplicationId = $ApplicationId
TenantId = $TenantId
CertificateThumbprint = $CertificateThumbprint
Expand Down
Loading

0 comments on commit f62b393

Please sign in to comment.