Switch parameter handling and best filtering practice

[laage]

I’ve set up this script to grab inactive accounts in our AD(s). And it works as intended but I’d like to refine it a bit.

#Requires -version 5.1
#Requires -Modules ActiveDirectory

[CmdLetBinding()]
Param (
  [Parameter(Mandatory = $false, ValueFromPipeline = $true, ValueFromPipelineByPropertyName = $true)]
  [ValidateSet('AD01','AD02','AD03','AD04')]
  [string]$AD = 'AD01',
  [Parameter(Mandatory = $false, ValueFromPipeline = $false)]
  [int]$InactiveDays = 365,
  [Parameter(Mandatory = $false, ValueFromPipeline = $false)]
  [string]$ExportPath = 'C:\Temp\'
)
$CsvName = "$(Get-Date -Format 'yyyyMMdd_HHmm')-$($AD.ToUpper())-$($InactiveDays)DaysInactive.csv"
$ExportPath = Join-Path -Path $ExportPath -ChildPath $CsvName

switch ($AD) {
  'AD01'  {
    $Server = (Get-ADDomainController -DomainName 'AD01.local' -Discover).Hostname
    Continue
    }
  'AD02'     {
    $Server = (Get-ADDomainController -DomainName 'AD02.local' -Discover).Hostname
    Continue
    }
  'AD03'     {
    $Server = (Get-ADDomainController -DomainName 'AD03.local' -Discover).Hostname
    Continue
    }
  'AD04'     {
    $Server = (Get-ADDomainController -DomainName 'AD04.local' -Discover).Hostname
    Continue
    }
  Default   {
    Write-Warning -Message 'No valid domain chosen.'
    exit
  }
}

Write-Output "Contacting $Server in the $AD domain"
Write-Output "The timespan in days is $InactiveDays"
Try {
  Search-ADAccount -Server $($Server) -AccountInactive -UsersOnly -TimeSpan "$($InactiveDays):00:00:00" -ResultPageSize 2000 -ResultSetSize $null |
    Where-Object -FilterScript {$_.Enabled -eq $true} |
    Get-ADUser -Properties WhenCreated, LastLogonDate, Description |
    Select-Object Name, SamAccountName, DistinguishedName, WhenCreated, LastLogonDate, Description |
	Where-Object -FilterScript {$_.WhenCreated -lt (Get-Date).AddDays(-$($InactiveDays))} |
    Export-CSV -Path $ExportPath -Encoding utf8 -Delimiter ';' -NoTypeInformation
}

Catch {
  Write-Host -BackgroundColor Red "Error: $($_.Exception)"
  Break
}

So I’ve got two questions.

The primary is that I’d like to add a switch parameter to filter out accounts that have never logged in.
The Where-Object line is not a problem, but I’m not sure of the best way to implement it.

Would I have to duplicate the entire Search-AdAccount - Export-Csv block in an If-block like this:

If ($LoggedOn) {
	Search-AdAccount...
	Where-Object [LOGGED ON FILTER]
}
else {
	Search-AdAccount...
}

Or is there a more elegant solution where I can just modify the first Where-Object line depending on whether or not the Switch-parameter was set?

The other question is about the two existing Where-Object lines in the block. The recommendation is to filter as far to the left as possible in PowerShell. However, the WhenCreated property is not available on Search-AdAccount, so I’ve had to filter twice in this block. Is there a better way of doing this or should I just accept that this is how it should be in this instance or is there a better way of doing this?

[tonyd]

@laage

to filter out accounts that have never logged in

This is how I chose to do that. Maybe not the best way, but it meets my needs. Basically, if the Last LogonDate property exists, that would indicate they have logged in at some point.

$InactiveUsers = Search-AdAccount -AccountInactive -UsersOnly -TimeSpan "$($daysInactive):00:00:00" | Where-Object {$_.LastLogonDate -And $_.Enabled -eq $true}

[grey0ut]

If it was my script I think I would be breaking that middle part up in to some variables. I know it’s all wrapped in a try/catch block but I think as the author it might be easier to read if the tasks were distinct. I don’t know if there’s any performance gain to just stringing all those pipes together.
But in any case, the first thing that stood out to me is that you’re starting with a Search-AdAccount and then immediately having to pipe that to Get-AdUser just to get the extra information you want, so I wanted to do some testing. The environment i’m testing in has a little over 6000 user objects.

measure-command {
  $Search = Search-ADAccount  -AccountInactive -UsersOnly -TimeSpan "$($InactiveDays):00:00:00"  -ResultSetSize $null | Where-Object {$_.Enabled -eq $true}
}
# 1 second

measure-command {
  $GetAD = Get-ADUser -filter {Enabled -eq $true} -Properties LastLogonDate,WhenCreated,Description | Where-Object {$_.LastLogonDate -le (Get-Date).AddDays(-365)}
}
# 4 seconds

Search-AdAccount is definitely fast, and having switch statements up front for AccountInactive is nice, but Get-AdUser has the ability to filter for enabled accounts futher to the left without needing a Where-Object statement and we can ask for the additional properties we want right up front. Let’s see what happens when we add the Get-AdUser back to the Search-AdAccount.

measure-command {
  $Search = Search-ADAccount  -AccountInactive -UsersOnly -TimeSpan "$($InactiveDays):00:00:00"  -ResultSetSize $null | 
    Where-Object {$_.Enabled -eq $true} | 
    Get-AdUser -Properties WhenCreated, LastLogonDate, Description
}
# 29 seconds

measure-command {
  $GetAD = Get-ADUser -filter {Enabled -eq $true} -Properties LastLogonDate,WhenCreated,Description | Where-Object {$_.LastLogonDate -le (Get-Date).AddDays(-365)}
}
# 4 seconds

Also of interest, the second method purely using Get-AdUser consistently returned 3 more results than Search-AdAccount so I manually inspected those accounts. They are for sure inactive, at least by this logic: LastLogonDate is older than 365 days. But all of them have a modified date that falls within that time frame. So something changed.
Microsoft’s documentation claims that AccountInactive is just returning accounts based on their last logon date. I checked one of the accounts against all DCs and the LastLogonDate and LastLogonTimestamp is the same throughout so shrug.

Get-AdUser

to me this paints Get-AdUser as the winner for this purpose.

#Requires -version 5.1
#Requires -Modules ActiveDirectory

[CmdLetBinding(DefaultParameterSetName = "Filter")]
Param (
  [Parameter(Mandatory = $false, ValueFromPipeline = $true, ValueFromPipelineByPropertyName = $true)]
  [ValidateSet('AD01','AD02','AD03','AD04')]
  [string]$AD = 'AD01',
  [Parameter(ParameterSetName = "Filter", Mandatory = $false, ValueFromPipeline = $false)]
  [int]$InactiveDays = 365,
  [Parameter(ParameterSetName = "Filter", Mandatory=$false)]
  [Switch]$NeverLoggedIn,
  [Parameter(Mandatory = $false, ValueFromPipeline = $false)]
  [string]$ExportPath = 'C:\Temp\'
)
$CsvName = "$(Get-Date -Format 'yyyyMMdd_HHmm')-$($AD.ToUpper())-$($InactiveDays)DaysInactive.csv"
$ExportPath = Join-Path -Path $ExportPath -ChildPath $CsvName

switch ($AD) {
  'AD01'  {
    $Server = (Get-ADDomainController -DomainName 'AD01.local' -Discover).Hostname
    Continue
    }
  'AD02'     {
    $Server = (Get-ADDomainController -DomainName 'AD02.local' -Discover).Hostname
    Continue
    }
  'AD03'     {
    $Server = (Get-ADDomainController -DomainName 'AD03.local' -Discover).Hostname
    Continue
    }
  'AD04'     {
    $Server = (Get-ADDomainController -DomainName 'AD04.local' -Discover).Hostname
    Continue
    }
  Default   {
    Write-Warning -Message 'No valid domain chosen.'
    exit
  }
}

$WhereArray = [System.Collections.ArrayList]@()
        
Switch ($PSBoundParameters.Keys) {
    'NeverLoggedIn' {[void]$WhereArray.Add('$null -eq $_.LastLogonDate')}
    default {[void]$WhereArray.Add('$_.LastLogonDate -le (Get-Date).AddDays(-$InactiveDays)')}
}

$WhereString = $WhereArray -join " -and "
$WhereBlock = [ScriptBlock]::Create($WhereString)

Write-Output "Contacting $Server in the $AD domain"
Write-Output "The timespan in days is $InactiveDays"
Try {
  $AdResults = Get-ADUser -Server $Server -filter {Enabled -eq $true} -Properties LastLogonDate,WhenCreated,Description | Select-Object Name, SamAccountName, DistinguishedName, WhenCreated, LastLogonDate, Description
  $FilteredResults = $AdResults | Where-Object $WhereBlock
} Catch {
  Write-Host -BackgroundColor Red "Error: $($_.Exception)"
  Break
}

try {
  $FilteredResults | Export-CSV -Path $ExportPath -Encoding utf8 -Delimiter ';' -NoTypeInformation -ErrorAction Stop
} catch {
  $Error[0]
}

This is kind of a lot to look at, but I borrowed a method i’ve used in a couple functions where I want to dynamically filter results based on passed parameters. It looks a little silly here with only 2 parameters but hopefully this makes sense. Assuming we want “InactiveDays” as a default filter I made it the ‘default’ in the switch block so it’s always added to the Where-Object statement.
As different filtering parameters are called, their respective Where-Object statements are added to an ArrayList so later they can be joined together with " -and " and turned in to a scriptblock that can then be directly passed to Where-Object.

The AD work has been broken up in to two primary sections; getting the data out of AD, and then filtering that data. This arrangement seems to keeps us close to that approx 4 second runtime we looked at earlier.
Then I moved the export task to its own try/catch block. Doesn’t change much here, but just a demonstration that you’re performing different tasks here, and you may want to handle their exceptions differently, and thus, separate try/catch blocks. YMMV.

[laage]

Thank you and I’m sorry for the late reply.

I’ve tried implementing your suggestion and I’m not sure I’m getting it right.

When I run it without the $NeverLoggedIn parameter I’m getting 0 results though my original script returns 100+ users with more than 365 inactive days.
If I run it with the $NeverLoggedIn parameter I’m getting 75 users that have never logged in.

At first I’d missed a ‘$’ in front of InactiveDays on this line in the switch statement:

[void]$WhereArray.Add('$_.LastLogonDate -le (Get-Date).AddDays(-$($InactiveDays))')

I thought that was the problem, but even once I’d corrected that I’m still getting 0 results in my CSV.

I added a Write-Output to the first try/catch to see if there were results that somehow were not added to the CSV, but I’m getting 0 from there as well.

$AdResults =  Get-ADUser -Server $($Server) -Filter {Enabled -eq $true} -Properties LastLogonDate, WhenCreated, Description, Description |
 Select-Object -Property Name, SamAccountName, DistinguishedName, WhenCreated, LastLogonDate, Description
$FilteredResults = $AdResults | Where-Object $WhereBlock
Write-Output "Found $($FilteredResults.Count) inactive users"

[grey0ut]

I would step through it line by line in an IDE rather than executing the script in its entirety. Run a line, then check the value of a variable. Rinse and repeat until you find the breakdown.

[laage]

I tried stepping through it with VSCode and the debugger, but I never really got the switch-statement to work!?
For some reason it never seemed to populate the arraylist.

Correct me if I’m wrong, but since the only available options are $NeverLoggedIn=$true or Default the switch can be replaced with a simple if/else?

That’s the way I rewrote it and also made a change to the output CSV-name depending on whether it runs with the NeverLoggedIn switch active or not.
Also added this line after the if/else:

[void]$WhereArray.Add('$_.WhenCreated -lt (Get-Date).AddDays(-$($InactiveDays))')

So it checks both LastLogonDate and WhenCreated properties against InactiveDays so it does not pick up users created before the cutoff. Should not be an issue with users who have logged on at some point, but can be an issue with users without any logons.

It seems to do what I want/need, but I have some ideas for extra tweaks like checking SamAccountNames for prefixes so we can grab only service accounts for instance.

Here’s the script as it looks right now (comments or suggestions are welcome):

#Requires -version 5.1
#Requires -Modules ActiveDirectory

[CmdLetBinding()]
Param (
  [Parameter(Mandatory = $false, ValueFromPipeline = $true, ValueFromPipelineByPropertyName = $true)]
  [ValidateSet('AD01','AD02','AD03','AD04')]
  [string]$AD = 'AD01',
  [Parameter(Mandatory = $false, ValueFromPipeline = $false)]
  [int]$InactiveDays = 365,
  [Parameter(Mandatory = $false, ValueFromPipeline = $false)]
  [string]$ExportPath = 'C:\Temp\'
)
$CsvName = "$(Get-Date -Format 'yyyyMMdd_HHmm')-$($AD.ToUpper())-$($InactiveDays)DaysInactive.csv"
$ExportPath = Join-Path -Path $ExportPath -ChildPath $CsvName

switch ($AD) {
  'AD01'  {
    $Server = (Get-ADDomainController -DomainName 'AD01.local' -Discover).Hostname
    Continue
    }
  'AD02'     {
    $Server = (Get-ADDomainController -DomainName 'AD02.local' -Discover).Hostname
    Continue
    }
  'AD03'     {
    $Server = (Get-ADDomainController -DomainName 'AD03.local' -Discover).Hostname
    Continue
    }
  'AD04'     {
    $Server = (Get-ADDomainController -DomainName 'AD04.local' -Discover).Hostname
    Continue
    }
  Default   {
    Write-Warning -Message 'No valid domain chosen.'
    exit
  }
}

$WhereArray = [System.Collections.ArrayList]@()

if ($NeverLoggedIn) {
  $CsvName = "$(Get-Date -Format 'yyyyMMdd_HHmm')-$($AD.ToUpper())-$($InactiveDays)NeverLoggedIn.csv"
  [void]$WhereArray.Add('$null -eq $_.LastLogonDate')
}
else {
  $CsvName = "$(Get-Date -Format 'yyyyMMdd_HHmm')-$($AD.ToUpper())-$($InactiveDays)DaysInactive.csv"
  [void]$WhereArray.Add('$_.LastLogonDate -le (Get-Date).AddDays(-$($InactiveDays))')
}

$ExportPath = Join-Path -Path $ExportPath -ChildPath $CsvName

[void]$WhereArray.Add('$_.WhenCreated -lt (Get-Date).AddDays(-$($InactiveDays))')

$WhereString = $WhereArray -join ' -and '
Write-Host $WhereString -ForegroundColor Green
$WhereBlock = [scriptblock]::Create($WhereString)

Write-Output "Contacting $Server in the $AD domain"
Write-Output "The timespan in days is $InactiveDays"
Try {
  $AdResults =  Get-ADUser -Server $($Server) -Filter {Enabled -eq $true} -Properties LastLogonDate, WhenCreated, Description, Description |
    Select-Object -Property Name, SamAccountName, DistinguishedName, WhenCreated, LastLogonDate, Description
  $FilteredResults = $AdResults | Where-Object $WhereBlock
  Write-Output "Found $($FilteredResults.Count) inactive users"
}
Catch {
  Write-Host -BackgroundColor Red "Error: $($_.Exception)"
  Break
}

try {
  $FilteredResults | Export-Csv -Path $ExportPath -Encoding utf8 -Delimiter ';' -NoTypeInformation -ErrorAction Stop
}
catch {
  $Error[0]
}