Inefficient PowerShell function for checking group membership

Question

I use the following function in a PowerShell script to check and - depending on the value of the $Action - either add or remove an Active Directory User Object from a Security Group.

Function Update-Group-Membership ($Action, $ADUser_Properties, $GroupName, $LogPathFileName) {
 $CurrentGroupMembers = Get-ADGroupMember -Identity:$GroupName | ? { $_.objectClass -ieq 'user' } | Select-Object -ExpandProperty "samAccountName"
 $ExistingMember = [bool]($ADUser_Properties | ? { $CurrentGroupMembers -contains $_.samAccountName })
 If ($Action -eq "ADD") {
 If ($ExistingMember) {
 "[" + (Get-Date -format "dd/MM/yyyy HH:mm:ss") + "] ... Update NOT Required: Existing Member of $GroupName" | Out-File $LogPathFileName -Append
 Write-Host " ... Update NOT Required: Existing Member of $GroupName" -ForegroundColor DarkGray 
 } Else {
 # Add to the group
 $PSCommand = $ADUser_Properties | Add-ADPrincipalGroupMembership -MemberOf:$GroupName
 "[" + (Get-Date -format "dd/MM/yyyy HH:mm:ss") + "] ... Updated: Added to $GroupName" | Out-File $LogPathFileName -Append
 Write-Host " ... Updated: Added to $GroupName" -ForegroundColor Yellow 
 }
 }
 If ($Action -eq "REMOVE") {
 If ($ExistingMember) {
 # Remove to the group
 $PSCommand = $ADUser_Properties | Remove-ADPrincipalGroupMembership -MemberOf:$GroupName -Confirm $False
 "[" + (Get-Date -format "dd/MM/yyyy HH:mm:ss") + "] ... Updated: Removed from $GroupName" | Out-File $LogPathFileName -Append
 Write-Host " ... Updated: Removed from $GroupName" -ForegroundColor Yellow
 }
} 
}

The function is called as follows:

Update-Group-Membership "ADD" $ADUser_Properties "SECURITY_GROUP_NAME" $LogPathFileName

$ADUser_Properties contains the output of the Get-ADUser command.

Whilst this function performs as expected, it is not particularly efficient and takes over a second to check a single user which consequently means processing several hundred users takes several hours.

Does anyone have any suggestions on how to improve or fine-tune it?

• briantist: There is a lot that could be improved here, but as a first step I need to question the workflow itself. Why do you need this function? Why not just directly call Add-ADPrincipalGroupMembership or Remove-ADPrincipalGroupMembership, which will work with many users at a time, much faster?
• BigotesDeGato: The function is used to check whether the user is already a member of the specified group (to prevent an ugly error in a scheduled script) and only update membership if necessary. As it's updating up to ten different security groups per user, it made sense to create a function over copy-pasting.
• briantist: I would recommend using -ErrorAction SilentlyContinue and/or -WarningAction SilentlyContinue as needed, instead.
• Nick W.: I will write up a basic script when I get home tonight to give you a better idea but the main points on this are as follows. Each and every time you call this function you are constantly making 2 seperate queries, you should keep a running list of the group memberships and users so you don't have to keep quering the same groups over and over. You can use scopes to make a variable run accessable after a funciton is done. Also make sure you run this directly on a domain controller to ensure that you take network latency out of the script execution time.

Answer 1

This is completely 100% NOT TESTED because I don't have the means to test it at this time however, it should work the way you want and a bit faster than before because it will do alot less queries. It should in theory only query each group once throughout the entire script which if your running even a single group against hundreds of users will save hundreds of queries. Next step I would suggest that you move to a more different loggin function such as streamwriter, is literally over 100x faster at writing logs.

Function Update-GroupMembership {
 Param (
 [Parameter(Mandatory=$True)]
 [ValidateSet('Add','Remove')]
 [String]$Action,
 [Parameter(Mandatory=$True)]
 $ADUser_Properties,
 [Parameter(Mandatory=$True)]
 [String]$ADGroup,
 [Parameter(Mandatory=$True)]
 [String]$LogPathFileName
 )
 IF (-NOT (Test-Path Variable:/Function)) {
 New-Variable -Force -ErrorAction Stop -Scope Script -Name UGM -Value @{}
 $Script:UGM.Users = @{}
 $Script:UGM.Groups = @{}
 if (-not (Get-Module -Name "ActiveDirectory")) {
 Try {
 Import-Module ActiveDirectory
 $Script:UGM.ActiveDirectory = $True
 } Catch {
 $Script:UGM.ActiveDirectory = $False
 }
 }
 }
 #Skip script is active directory not loaded.
 If (!$Script:UGM.ActiveDirectory) {
 Throw "Active Directory module not loaded."
 }
 IF (-NOT ($Script:UGM.ContainsKey($ADGroup))) {
 $Script:UGM.$ADGroupName = New-Object -Type 'System.Collections.ArrayList'
 Get-ADGroupMember -Identity $ADGroup |Select -ExpandProperty SamAccountName |ForEach {$Script:UGM.$ADGroupName.Add($_)}
 }
 $UGM.InGroup = $Script:UGM.$ADGroup.Contains($ADUser_Properties.SamAccountName)
 $Date = ([datetime]::Now).ToString('dd/MM/yyyy HH:mm:ss')
 Switch ($Action) {
 'Add' {
 If ($UGM.InGroup) {
 Write-Host "$($ADUser_Properties.Name) Already in $ADGroup"
 } Else {
 Try {
 Add-ADPrincipalGroupMembership -MemberOf:$ADGroup -Identity:$ADUser_Properties |Out-Null
 $Script:UGM.$ADGroup.Add($ADUser_Properties.SamAccountName)
 Write-Host " ... Updated: Added to $ADGroup" -ForegroundColor Yellow
 "[$Date] ... Updated: Added to $ADGroup" | Out-File $LogPathFileName -Append
 } Catch {
 Throw "[$Date] ... Failed: Unable to add $($ADUser_Properties.Name) to $ADGroup"
 }
 }
 }
 'Remove' {
 If ($UGM.InGroup) {
 Write-Host "$($ADUser_Properties.Name) not in $ADGroup"
 } Else {
 Try {
 Remove-ADPrincipalGroupMembership -MemberOf:$ADGroup -Identity:$ADUser_Properties |Out-Null
 $Script:UGM.$ADGroup.Remove($ADUser_Properties.SamAccountName)
 Write-Host " ... Updated: Removed from $ADGroup" -ForegroundColor Yellow
 "[$Date] ... Updated: Removed from $ADGroup" | Out-File $LogPathFileName -Append
 } Catch {
 Throw "[$Date] ... Failed: Unable to remove $($ADUser_Properties.Name) from $ADGroup"
 }
 }
 }
 Default {}
 }
}