2
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – briantist Jan 31 '17 at 16:43
  • \$\begingroup\$ 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. \$\endgroup\$ – BigotesDeGato Jan 31 '17 at 16:58
  • 3
    \$\begingroup\$ I would recommend using -ErrorAction SilentlyContinue and/or -WarningAction SilentlyContinue as needed, instead. \$\endgroup\$ – briantist Jan 31 '17 at 16:59
  • \$\begingroup\$ 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. \$\endgroup\$ – Nick W. Apr 30 '18 at 17:10
0
\$\begingroup\$

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 {}
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.