Conversation
Replace seven identical Get-ADObject | Select-Object objectClass calls in Private\Set-RiskRating.ps1 with a new Get-SidObjectClass helper that wraps the query in a script-scoped hashtable cache (\). During a Locksmith scan, the same SID (Domain Users, Authenticated Users, Domain Computers, etc.) appears across many issues. Previously each issue evaluation issued a separate LDAP query for each SID. With the cache, every SID is resolved at most once per module session. Changes: - Add Get-SidObjectClass private helper at top of Set-RiskRating.ps1 - Replace all 7 Get-ADObject lookup sites with Get-SidObjectClass calls - Preserve exact return types and semantics for all call sites: - Pattern A (6 sites): returns PSCustomObject with objectClass property - Pattern B (1 site, ESC5): extracts .objectClass from the PSCustomObject - No changes to risk scoring logic, remediation, or test coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces repeated Active Directory lookups during a Locksmith scan by introducing a script-scoped SID→objectClass cache and routing existing Get-ADObject calls through a new helper in Set-RiskRating.
Changes:
- Added
Get-SidObjectClasshelper that cachesGet-ADObjectSID lookups in a script-scoped hashtable. - Replaced 7 repeated
Get-ADObject ... | Select-Object objectClasscall sites withGet-SidObjectClassto reuse cached results.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use -ErrorAction Stop with try/catch so that a transient ADWS error does not populate the cache with . Legitimate 'not found' (SID absent from AD) still returns and caches normally; transient errors emit Write-Warning and leave the key absent, allowing retry on the next call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nvoke-Locksmith.ps1 Invoke-Locksmith.ps1 is the manually-maintained monolithic script that must stay in parity with the Private/*.ps1 sources. Apply the same Get-SidObjectClass helper (with try/catch error-handling) and replace the same 7 Get-ADObject | Select-Object objectClass call sites that were already applied to Private\Set-RiskRating.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The current state of the PR looks good. Both issues from the previous review have been resolved:
No new issues found. |
Replace Write-Warning with Write-Error -ErrorRecord $_ in the Get-SidObjectClass catch block so that AD lookup failures surface on the error stream and callers' $ErrorActionPreference (including Stop) is respected. Failed lookups are still not cached so each SID can be retried on the next call. Applied to both Private\Set-RiskRating.ps1 and Invoke-Locksmith.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Eliminates repeated Active Directory LDAP queries for the same SID during a Locksmith scan run.
Problem
Set-RiskRatinghad seven call sites all performingGet-ADObject -Filter { objectSid -eq $SID } | Select-Object objectClass. Common principals appear in many template ACLs and were re-queried on every issue.Solution
New
Get-SidObjectClasshelper with$script:SidObjectClassCacheinPrivate/Set-RiskRating.ps1. Transient ADWS errors are not cached (key stays absent for retry); errors re-emitted viaWrite-Error -ErrorRecord $_so callers can detect failures and$ErrorActionPreferenceis honoured. All 7 call sites replaced.Scope
Set-RiskRating.ps1Invoke-Locksmith.ps1manually updated for parity (Build/Build-Module.ps1is NOT run in CI)Static analysis
PSScriptAnalyzer: no new warnings introduced.