4
\$\begingroup\$

I was recently asked to help our Spanish BU to audit their AD for inactive accounts.

Grabbing a script from online I gave them this: https://gallery.technet.microsoft.com/scriptcenter/Get-Active-Directory-User-bbcdd771

However that doesn't scale well when there's multiple users; as this takes one account then manually searches for the maximum last logon on any domain controller, rather than pulling back the values from all DCs then performing an aggregate function (which can then work the same way for 1 or many accounts).

As such I wrote this:

[string]$DCNameFilter = "ES*" #only get Spanish DCs (i.e. our company's naming convention prefixes server names with 2 char country iso code)
[string]$SearchBase = 'OU=es,DC=eu,DC=myCompany,DC=com' #only look within the Spanish OU; our OUs are under their collective region's domain (i.e. EU) within the company

#filter down to just the ES domain controllers
$DCs = Get-ADDomainController -Filter {Name -like $DCNameFilter} | Select -expand Name

#for each of the above domain controllers 
$DCs | %{
    #get a list of enabled users & their last logon dates
    Get-AdUser -SearchBase $SearchBase -Filter {Enabled -eq $true} -Properties LastLogonDate -Server $_ `
    | Select-Object samAccountName, Name, LastLogonDate #filter to only the columns we're interested in 
} | Group-Object -Property samAccountName | %{ #group results by account name
    #return the account name, full name, & their latest logon date
    $_ | select Name, @{Name='Full Name';Expression={$_.Group | select -ExpandProperty Name -first 1}}, @{Name='LastLogonDate';Expression={$_.Group | Measure-Object -Property LastLogonDate -Maximum | select -ExpandProperty Maximum}}
} | sort Name #nb: name is sAmAccountName; column name changed as result of earlier group-object statement.

I've seen lots of examples similar to the technet script above used before, but have never seen someone using the aggregate/group-object approach, so wondered if there was a reason for this (e.g. some inefficiency I'd not considered); hence posting here.

NB: I'm also aware of the Search-ADAccount -AccountInactive option, which would show inactive accounts; but the request was to return the last logon dates rather than simply what's inactive; so I provided both options so the local team could determine which was more suited to their requirements.

\$\endgroup\$
5
\$\begingroup\$

Aliases

You're used to my reviews by now so you're probably tired of me saying this, but avoid aliases and abbreviations (% vs ForEach-Object, Select vs Select-Object, -expand vs -ExpandProperty, sort vs Sort-Object, etc.).


Backticks ` as line continuation

Try to avoid shortening lines with backtick where possible. The backticks are hard to see / easy to miss.

Note that you can end a line with a pipe | or an operator and continue on the next line without the backtick, which I think is a much more flexible solution:

Get-ADObject -Filter { whatever } |
Sort-Object BySomething |
Select-Object Thing1,Thing2,Thing3 |
Where-Object {
    $_.A -eq 5 -and
    $_.Z -like '*what*' -and
    $_.BoolProp
} |
Format-Table

As an added bonus, it doesn't confuse the syntax highlighting on these sites!


Domain Controller Discovery

When finding a domain controller in a certain region, you might consider filtering on Site. Nothing wrong with going by your naming convention, although it's more likely for that to change, or for a short pattern like ES* to match something else; I'm sure you've considered that and your pattern is fine, just offering another suggestion.

I also like to add -Discover, -ForceDiscover, and -Service ADWS to ensure that you are finding DCs that are available and usable with PowerShell. Combined:

$DCs = Get-ADDomainController -Discover -ForceDiscover -SiteName Spain -Service ADWS | Select-Object -Expand Name

Logon Dates

You're probably already familiar with the logon date attributes in AD, but I'll summarize:

  • LastLogon — Updated every time a user logs on, but only on the domain controller that authenticated the user. This value is not replicated.
  • LastLogonTimestamp — This value is replicated, but to avoid heavy replication traffic it's not updated every time, so you sacrifice accuracy (it could be as much as 2 weeks off by default), but you only have to query a single DC to get the value. This value is also annoying to turn into an actual date.
  • LastLogonDate — This is not actually in AD. It's provided by the PowerShell cmdlets, and it's a real [DateTime] object that has the same value as LastLogonTimestamp, which makes it really convenient to use and filter on.

So, depending on how accurate you really need this process to be, you might avoid querying all the DCs in the first place and just take LastLogonDate as being "Close Enough" for your purposes. It would simplify things greatly if that's acceptable.

The thing is, this also reveals a problem in your script. You are already using LastLogonDate, you're just querying it from every DC instead of using LastLogon. As long as replication is working correctly, that value will always be the same, so querying multiple DCs is unnecessary.

If you want the precision, you need to use LogonDate (and do the appropriate conversions like in the technet script you linked to). This is probably why Group-Object is not often used. Because it's not so trivial to treat this attribute directly as a [DateTime].


Search-ADAccount

I'm not sure I understand the aversion to using Search-ADAccount. The object returned is still an AD account object, and it includes the LastLogonDate property so you can return that.

After re-reading it I understand; you're not filtering on any specific date in this script. So you're returning all users and their dates.


Simplification

Based on what I wrote above, I might suggest a more simple solution (I know this isn't SO but, it's basically a one-liner...):

$DC = Get-ADDomainController -Discover -ForceDiscover -SiteName Spain -Service ADWS | Select-Object -Expand Name -First 1
Get-ADUser -SearchBase $SearchBase -Filter { Enabled -eq $true } -Properties LastLogonDate -Server $DC |
Sort-Object SamAccountName |
Select-Object SamAccountName,Name,LastLogonDate

I would even add that because the attribute is replicated, there's no need to query a Spanish DC; you can leave out the whole discovery part and just let Get-ADUser use the logon server, or use discovery but don't limit it to a site. It should in theory be more efficient to simply query the closest DC to where the script is running.

\$\endgroup\$
  • \$\begingroup\$ Have you considered indenting all lines but the first when chaining together with |? I adopted that style from Richard Berg's style guide and I find the indentation makes the first line stand out better. \$\endgroup\$ – alx9r Feb 27 '16 at 0:13
  • \$\begingroup\$ @alx9r sounds good, I'll try it out in the future, thanks \$\endgroup\$ – briantist Feb 27 '16 at 0:31

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.