1

The following script works perfectly, but I think it's way too complex and slow for what it needs to do.

Basically, for a list of users in a variable (manually or obtained from Get-ADUser, doesn't matter), I want to query all Domain Controllers and get the LastLogonDate for each user. I'll later use it for bad password etc.

Any suggestions on cleaning it up please that would improve my coding skills?

$UserList = "User1", "User2"

$DCs = (Get-ADDomainController -Filter *).Name

$Combined = foreach ($User in $UserList)
{
    $DCarray = [ordered] @{}
    foreach ($DC in $DCs)
    {
        $DCresponse = Get-ADUser $User -Properties DisplayName, LastLogonDate -Server $DC | Select-Object Name, DisplayName, LastLogonDate
        if( -not $DCarray.Contains("Name")) { $DCarray.Add("Name",$DCresponse.name) }
        if( -not $DCarray.Contains("DisplayName")) { $DCarray.Add("DisplayName",$DCresponse.DisplayName) }
        if( -not $DCarray.Contains($DC)) { $DCarray.Add($DC,$DCresponse.LastLogonDate) }
    }
    $Return = New-Object -TypeName psobject
        foreach ($Key in $DCarray.keys)
        {
            $Each = $DCarray[$Key]
            
            $Return | Add-Member -MemberType NoteProperty -Name $Key -Value $Each
        }
    $Return
}

$Combined | Format-Table -AutoSize
Aubs
  • 180
  • 2
  • 10
  • I would cast `[pscustomobject]` instead of `ordered hashtable` which then gets converted to `psobject` as it is quite inefficient you can also query all users per Domain Controller at the same time using a hacky LDAPFilter. – Santiago Squarzon Nov 26 '21 at 22:34
  • You can use a *workflow* which would allow you to run them threaded, using `-Parallel` switch with a *foreach* statement. – Abraham Zinala Nov 26 '21 at 23:08
  • 1
    @AbrahamZinala `worflow` is a lot slower than a linear loop tho, and i'm not even sure if the AD Module would work in the `parallel` scope. See this answer for more details https://stackoverflow.com/questions/41796959/why-powershell-workflow-is-significantly-slower-than-non-workflow-script-for-xml. Runspace would significantly increase the runtime but OP asked for lower complexity not higher hehe – Santiago Squarzon Nov 26 '21 at 23:29
  • 1
    @Santi, man... I just suck at run spaces lol didn't realize workflows were slower in this case. Got any good articles I can read up on run spaces? – Abraham Zinala Nov 27 '21 at 00:31
  • 1
    @AbrahamZinala I just learnt by looking at Mathias's code from that answer. You could look into this [article](https://adamtheautomator.com/powershell-multithreading/). I also wrote a test run for [Runspace vs ThreadJob vs Linear Loops](https://github.com/santysq/Linear-Loops-vs-ThreadJob-vs-Runspace) on my github if you want to use the code as your blueprint. – Santiago Squarzon Nov 27 '21 at 00:37
  • I read that article not too long ago, may have to re-read it. – Abraham Zinala Nov 27 '21 at 00:47
  • 1
    Thanks AbrahamZinala and @SantiagoSquarzon (for your answer too); I'm back at work on Monday, so will see how it performs then. Although I said lower complexity, I've never heard of Runspace, so I'll also take a look at that when I get some time. – Aubs Nov 27 '21 at 19:47

2 Answers2

1

I think the logic is mostly the same but this should be easier to understand and maintain. In addition, the use of the LDAPFilter should improve the runtime a bit.

$UserList = "User1", "User2", "User3"
$filter = "(|(name={0}))" -f ($UserList -join ')(name=')
# LDAP query string would look like: (|(name=User1)(name=User2)(name=User3))

$DCs = (Get-ADDomainController -Filter *).Name

$props = @{
    Properties = 'DisplayName', 'LastLogonDate'
    LDAPFitler = $filter
}

$result = foreach($dc in $DCs)
{
    $props.Server = $dc
    $users = Get-ADUser @props

    foreach($user in $users)
    {
        # If this User's LastLogonDate attribute is NOT null
        if($user.LastLogonDate)
        {
            [pscustomobject]@{
                DomainController = $dc
                UserName = $user.Name
                DisplayName = $user.DisplayName
                LastLogonDate = $user.LastLogonDate
            }
        }
    }
}

$result | Sort-Object UserName, LastLogonDate | Out-GridView
Santiago Squarzon
  • 41,465
  • 5
  • 14
  • 37
0

I can't leave a comment, so posting as an Answer instead.

For a non-Powershell solution have a look at this tool which will retrieve the the last logon time from all DC in the forest\domain for a list of users, based on a list of samaccountnames.

https://nettools.net/last-logon-time/

Gary

gazza
  • 11
  • 1
  • Thank you, NetTools looks interesting. Unfortunately, I'm not able to test 'unknown' (to me) third party apps without knowing exactly what it does. Working in a healthcare setting, we can't afford to have malicious code wreaking havoc - It's touch enough as it is. (the website has been up since 2019). Still, good to post as someone else may find it useful. – Aubs Dec 13 '21 at 23:31
  • Completely understand, I've worked in the same industry as well. Basically it does the same as the powershell script, except uses C++, multi-threaded and native ldap API, so is significantly faster. – gazza Dec 14 '21 at 01:54