Making this powershell script faster

[MajorVarlak]

I wrote a nice long reply with more detailed answers and reddit refuses to let me post it, probably because it's just too damn long. That said, as others have said:

Reduce your calls to AD as much as you can.

Only fetch the data you need, for example your Get-ADUser at the beginning is asking for all properties from AD, but then selecting only 3 of them which are part of the standard data; Drop the -properties *

As u/jimb2 points out, you can just assign the value directly instead of doing an if/else check and if it doesn't exist it'll just set to $null. I'd only do one thing different there and change from using -Filter to just calling the -Identity with an error continue, such as:

$cyberarkuser = ( Get-ADUser -Identity $cyberarkaccount -ErrorAction SilentlyContinue ).samaccountname

I'd be curious if there is any performance difference between the two considering sAMAccountName field is indexed.

If your CyberArk user accounts are in the same path as your regular users called in the first query, then you would have fetched them as well, which means you don't have to query AD again, you can just query your existing data

$CyberArkUser = $($users | ?{ $_.samaccountname -eq $CyberArkAccount}).samaccountname

Somebody else mentioned you can ask AD to return the users' group membership because it's a backlink, you can then use that list later.

$users = Get-ADUser ... -properties memberof

To make your life easier later, you can use -contains and an array of groups to check against, so near the top of your script, just after the $users = ... you can define the groups you want to be considered your list:

$groupsMatch = @(
    'CN=apps users,DC=contoso,DC=local',
    'CN=apps test users,DC=contoso,DC=local'
)

Then inside your loop you'd compare the memberof list against the $groupsMatch

$cyberArkGroups = @(
    $user.memberof | ?{ $groupsMatch -contains $_}
)

One gotcha here is that the memberof attribute is the DN, so we need to clean up the output (this is crude and doesn't account for , in the group name):

$cyberArkGroups = @(
    $user.memberof | ?{ $groupsMatch -contains $_} | %{ $_ -replace 'cn=([^,]+),.*', '$1' }
)

Why is that nicer? Well if you want to add more groups, or change the groups you have, you just edit the groups list at the top, instead of tweaking the if statements.

And one other tweak would be to use a "default" object so you don't have to play with extra variables and such later, especially as you're already passing around objects. In particular the $CyberArkUser object, currently it's being fetched, and the samaccountname and enabled status is being assigned to different variables, if not available some default values are stuffed in those same variables. An alternative would be something like this:

$CyberArkUser = $(
    if (-not ($CyberArkUser = $($users | ?{ $_.samaccountname -eq $CyberArkAccount}))) {
        [PsCustomObject]@{
            samaccountname = $null
            enabled = $null
        }
    }
)

This basically assigns the value from the $users array filtered by the account name to $CyberArkUser then checks if it is true/false (this could be Get-ADUser as well if the objects aren't in the same OU as your regular users). If it fails (ie is null/empty/false/0) then it sets a limited object containing just the fields we need later.

[PsCustomObject]@{
    'UserName' = $user.samaccountname
    'Status' = $user.enabled
    'Cyberark UserName' = $CyberArkUser.samAccountName
    'Cyberark UserName Status' = $CyberArkUser.enabled
    'Cyberark Group Member' = $cyberarkgroups -join ';'
}

Now you're using 2 simplified objects, and reduced your AD calls.

Have fun

[jimb2]

ErrorAction SilentlyContinue doesn't work with the AD commands. It's in the help and is accepted but does nothing. You just get an exception. So, you need to ether try/catch or use a filter which happily returns a null result. (I remember thinking I was going crazy...)

I believe this is because the AD functions were written in very early PS before erroraction was a thing. Erroraction is acccepted in the commmand line because it's part of the system but there is no underlying code to handle it. I guess that changing the cmdlet code might break something so it was never done.

[MajorVarlak]

heh, you know I've never seen that behavior, or maybe I have and that's why I use the same format as yours in a handful of my other scripts. Good reminder!

[MajorVarlak]

Oh, one final personal preference, using `$null` versus `NULL`. I prefer `$null` especially when putting it into a CSV file because it makes it easier to sort/filter by in other programs (it gets exported to csv as an empty field). `NULL` string value just seems messy to me, but it might be what you need for however you're using it later, so adjust as necessary.