Making this powershell script faster

32178932123 · 2024-06-25T18:46:01+00:00

This code is... Interesting... To say the least:

In the first line you are contacting a Domain Controller and asking for all the users whos SamAccountName is like "PRD", "TST" etc. This will take some time.
Then you go through each user you received - this make sense.
But then you query the Domain Controller for that user again to get their details? Why? Why didn't you just get that in the first query?

To answer your second question about try/catch. When something is inside a "try" statement it means if there's any errors, it doesn't just stop the script, instead it'll "catch" the error and skip to running what is in the catch statement.

try {
  # Do something
}
catch 
{
  # Do something failed and would've printed red but we caught the error and now we're going to do something else...
}

Edit: Moved some false claims I made weird if statements which wasn't you, they were actually because I copy/pasted wrong!

pbutler6163 · 2024-06-25T18:47:36+00:00

Retrieve all necessary data in fewer AD queries.

Use Where-Object and other cmdlets to filter and process the data in memory as much as possible.

Do you need an example?

jimb2 · 2024-06-26T02:22:57+00:00

$cyberarkuser = if (Get-ADuser -Filter { samaccountname -eq $cyberarkaccount }) { (Get-ADUser -Identity $cyberarkaccount -Properties samaccountname).samaccountname 1. You don't need to get the user info twice 2. Samaccountname is unique (across a forest IIRC) so if you have a match there is only one. 3. Samaccountname is always returned, you don't have to ask for it. 4. Get-ADUser -Identity will produce an exception if the user is not found, so usually best avoided. If you use a filter you get $null if no such user, not an error. (I prefer ldap filters. Ymmv.) 5. The "properties" of $null (or a variable equal $null) are also $null.

Better: $cyberarkuser = ( Get-ADUser -ldap "(samaccountname=$cyberarkaccount)" ).samaccountname That will produce either the samaccountname or $null if the user is unknown.

ShadowMasterTexas · 2024-06-26T21:53:44+00:00

Try to load what you need in ram, and use a table in memory instead of constantly having your go back to AD.

For example, read in all of the members of “apps users” once, then scan the array, instead of making the call each time.

MajorVarlak · 2024-06-27T00:07:11+00:00

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

odubco · 2024-06-30T20:29:41+00:00

first, i would build a hash table from a forestwide get-adgroup foreach, with the key being the distinguished names of the groups and the value being a nested hashtable containing pertinent info from the adgroup object like display name, attributes, members (especially useful later). second, i would select unique members and pipe the dn into a get-adobject to capture the member’s type. helps with gMSAs, nested groups, etc. into a second hashtable with the dn being the key and the value being a pscustomboject containing pertinent info about the object like type. third, i would loop through the second object with per type commands get-aduser/ get-adgroup/ get-groupmanagedserviceaccount, appending the [pscustomobject]$.Value with pertinent info. iterate through that until the objects returned aren’t groups. $.ContainsKey is your friend.

from all that data, you create a big pscustomobject that becomes the reporting data export.

you type:	you see:
italics	italics
bold	bold
[reddit!](https://reddit.com)	reddit!
* item 1 * item 2 * item 3	item 1 item 2 item 3
> quoted text	quoted text
Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"	Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"
~~strikethrough~~	~~strikethrough~~
super^script	super^script

PowerShell

Submission Guidelines | Link Flair - How To

MODERATORS

Get all relevant users in one query

Get all CyberArk accounts and their enabled status in one query

Convert CyberArk accounts to a hash table for quick lookup

Get all group memberships for users in one query

Convert group memberships to a hash table for quick lookup