🧹 chore: code quality audit — fix critical, high, medium, and low bugs (#15)

* 🐛 fix(scripts): fix all critical code bugs found during audit

- F04: add missing comma in Purge-InstalledModules module name array
- F06: add missing -Value to Set-Variable in Get-ADUserTransitiveGroupMembership
- F08: add missing -Value to Set-Variable in Export-AllADUserTransitiveGroupMemberships
- F10: fix broken AD -Filter (single-quoted string) in Export-AllGroupMemberships
- F12: fix ValidateSet mismatch ('Office' -> 'physicalDeliveryOfficeName') in Get-ADAttributeUniqueValues
- F13/F15: convert empty hard-coded \ to mandatory parameter; fix date format (hh-m-ss -> HH-mm-ss); add TODO comments on unreplaceable placeholders in Archive-ObsoleteGroups
- F21: fix Select-Object -ExpandProperty with multiple properties in Get-FSMORoleDetails (only accepts one); cache Forest/Domain objects and access properties directly
- F23: fix \$._Index typo -> \ in Update-DnsServerList
- F26: replace Enter-PSSession/Exit-PSSession (interactive-only) with Invoke-Command -Session in Push-DNSClientServerAddresses; add Remove-PSSession in finally block

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* 🐛 fix(function): resolve high-severity bugs in AD, DNS, Exchange, and general scripts

F07: Move Sort-Object emit from end to process block in Get-ADUserTransitiveGroupMembership
     to prevent pipeline clobbering (only last input was returned to end block)
F09: Fix undefined \\\ -> \\.Count\ in Export-AllADUserTransitiveGroupMemberships
F16: Fix operator precedence bug in Get-InactiveADUser (add parens to -not comparison)
F17: Use \\\ parameter value as export path when provided in Get-InactiveADUser
F19: Replace \continue\ with \
eturn\ in catch outside loop in Get-LockedOutLocation
F22: Move pipeline identity resolution from begin to process block in Get-ADObjectFromPipeline
F24: Make \\\ mandatory in Update-ModuleVersion to prevent null member access
F27: Replace empty placeholder strings with mandatory params in Push-DNSClientServerAddresses
F29: Remove hardcoded DOMAINNAME.org placeholder from Exchange ConnectionUri

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* 🐛 fix(function): resolve medium-severity bugs across scripts

F02: Fix RegistryAccessRule from FullControl to ReadKey in Activate and Get License
     (comment/output said 'read permissions' but code was granting FullControl)
F05: Fix Write-Error \\Cannot validate argument on parameter 'MaximumHistoryCount'. The 0 argument is less than the minimum allowed range of 1. Supply an argument that is greater than or equal to 1 and then try the command again.[0]\ -> \\\ in Get Hostnames from CSV IP Addresses
     (\\Cannot validate argument on parameter 'MaximumHistoryCount'. The 0 argument is less than the minimum allowed range of 1. Supply an argument that is greater than or equal to 1 and then try the command again.[0]\ may not reflect the current exception in a catch block)
F15: Confirmed already fixed in critical pass (Get-Date format HH-mm-ss correct)
F18: Replace \�reak 1\ with \
eturn\ in two catch blocks in Get-InactiveUsers
     (break outside a loop throws LoopFlowException in PowerShell)
F20: Add BadPasswordTime to Get-ADUser -Properties list in Get-LockedOutLocation
     (property was used in output object but never requested from AD)
F25: Fix undefined \\\ -> \\\\ in Update-ModuleVersion
F28: Remove Set-ExecutionPolicy RemoteSigned from Parse-TransportLogs
     (modifies machine security policy as a script side effect)
F30: Add bounds check before split('<')[1] in Parse-TransportLogs
     (throws index-out-of-range if no '<' delimiter present in log data)
F31: Replace \\Continue\ global mutation with try/catch for DNS lookup
     in Parse-TransportLogs (scoped error handling instead of global state change)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* 🧹 chore(function): remove dead code re-fetch of ProductKey at EOF

F03: The final line re-fetched \ via Get-CimInstance but the value
was never used. The variable is already assigned earlier in the script where
it is consumed by Invoke-Expression slmgr.vbs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: SamErde

Active Directory/AD Groups/Archive-ObsoleteGroups.ps1

@@ -21,11 +21,23 @@
 #Import the Active Directory module so we can work with AD groups.
 Import-Module ActiveDirectory
 
-#Set the Active Directory server name that will be used. Using a serverless domain name here may also work.
-$Domain = ''
+# TODO: This script requires customization before running. The $Domain, archive paths,
+# TODO: group identity/OU values in the loop, and Move-ADObject target path must all be set
+# TODO: for your environment. See inline TODO comments below.
+param (
+    # The Active Directory domain controller or domain name to run against.
+    [Parameter(Mandatory)]
+    [string]
+    $Domain,
+
+    # Path to the file listing obsolete group names (one per line).
+    [Parameter()]
+    [string]
+    $GroupListPath = 'C:\Scripts\ObsoleteGroups\ObsoleteGroups.csv'
+)
 
 #Read in the CSV or text file of group names.
-$File = Get-Content -Path C:\Scripts\ObsoleteGroups\ObsoleteGroups.csv
+$File = Get-Content -Path $GroupListPath
 
 #Loop through each line of the text file and run the following commands for each line:
 Foreach ($Group in $File) {
@@ -37,11 +49,13 @@ Foreach ($Group in $File) {
         This section will require special customization until we further develop the script to pull the full group DN.
         In the interest of time today, I have hard coded some of the information.
         * * * * * * * * * *
-    /#>
+    #>
+    # TODO: Replace the -group, -ou, and -domain placeholder values with real values for your environment.
     .\Remove-AllGroupMembers.ps1 -group "CN=$Group" -ou 'OU=' -domain 'DC='
+    # TODO: Replace -Identity and -TargetPath with the correct DN values for your environment.
     Move-ADObject -Server $Domain -Identity 'CN=ps,DC=' -TargetPath ''
 }
 
 #Copy and rename the CSV file with a timestamp to keep as a record of run history.
-$timeStamp = Get-Date -Format 'yyyy-MM-dd hh-m-ss'
-Copy-Item -Path C:\Scripts\ObsoleteGroups\ObsoleteGroups.csv -Destination "C:\Scripts\ObsoleteGroups\Run History\ObsoleteGroups $timeStamp.csv"
+$timeStamp = Get-Date -Format 'yyyy-MM-dd HH-mm-ss'
+Copy-Item -Path $GroupListPath -Destination "C:\Scripts\ObsoleteGroups\Run History\ObsoleteGroups $timeStamp.csv"

Active Directory/AD Users/Get-InactiveADUser.ps1

@@ -96,7 +96,7 @@
 
         if ($CheckAllDCs) {
             # Skip the check across all DCs if there is already a LastLogonDate within the past 14 days and if the most recent logon is more recent than the inactive date threshold.
-            if ( $MostRecentLogon -lt (Get-Date).AddDays(-14) -and (-not $MostRecentLogon -lt $InactiveDate) ) {
+            if ( $MostRecentLogon -lt (Get-Date).AddDays(-14) -and (-not ($MostRecentLogon -lt $InactiveDate)) ) {
                     # Check LastLogon (non-replicated) on every domain controller.
                     foreach ($DC in $DomainControllers) {
                         try {
@@ -143,7 +143,7 @@
 
         # Optional: Export to CSV
         if ($PSBoundParameters.ContainsKey('ExportCSV')) {
-            $ExportPath = ".\InactiveUsers_$(Get-Date -Format 'yyyyMMdd_HHmmss').csv"
+            $ExportPath = if ($ExportCSV) { $ExportCSV } else { ".\InactiveUsers_$(Get-Date -Format 'yyyyMMdd_HHmmss').csv" }
             $Results | Export-Csv -Path $ExportPath -NoTypeInformation
             Write-Host "Results exported to: $ExportPath" -ForegroundColor Green
         }

Active Directory/AD Users/Get-InactiveUsers.ps1

@@ -73,7 +73,7 @@
         Write-Verbose 'Active Directory module imported successfully'
     } catch {
         Write-Error "Failed to import Active Directory module: $($_.Exception.Message)"
-        break 1
+        return
     }
 
     # Calculate the cutoff date and its file time representation for the AD filter.
@@ -99,7 +99,7 @@
         Write-Host "Found $($InactiveUsersResult.Count) inactive user account(s)." -ForegroundColor Green
     } catch {
         Write-Error "Failed to get user accounts: $($_.Exception.Message)"
-        break 1
+        return
     }
 
     $InactiveUsers = @()

Active Directory/AD Users/Get-LockedOutLocation.ps1

@@ -48,7 +48,7 @@
             $DCCounter++
             Write-Progress -Activity 'Contacting DCs for lockout info' -Status "Querying $($DC.Hostname)" -PercentComplete (($DCCounter / $DomainControllers.Count) * 100)
             try {
-                $UserInfo = Get-ADUser -Identity $Identity -Server $DC.Hostname -Properties AccountLockoutTime, LastBadPasswordAttempt, BadPwdCount, LockedOut -ErrorAction Stop
+                $UserInfo = Get-ADUser -Identity $Identity -Server $DC.Hostname -Properties AccountLockoutTime, BadPasswordTime, LastBadPasswordAttempt, BadPwdCount, LockedOut -ErrorAction Stop
             } catch {
                 Write-Warning $_
                 continue
@@ -74,7 +74,7 @@
             $LockedOutEvents = Get-WinEvent -ComputerName $PDCEmulator.HostName -FilterHashtable @{LogName = 'Security'; Id = 4740 } -ErrorAction Stop | Sort-Object -Property TimeCreated -Descending
         } catch {
             Write-Warning $_
-            continue
+            return
         }#end catch
 
         foreach ($item in $LockedOutEvents) {

Active Directory/Domain Services/Get-FSMORoleDetails.ps1

@@ -1,8 +1,16 @@
 #WIP
 
 # Get the hostname and AD site location of domain controllers that hold the AD FSMO roles.
-$FSMORoles = Get-ADForest | Select-Object -ExpandProperty SchemaMaster, DomainNamingMaster
-$FSMORoles += Get-ADDomain | Select-Object -ExpandProperty PDCEmulator, RIDMaster, InfrastructureMaster
+# Note: -ExpandProperty only accepts one property at a time; collect each role separately.
+$Forest = Get-ADForest
+$Domain = Get-ADDomain
+$FSMORoles = @(
+    $Forest.SchemaMaster
+    $Forest.DomainNamingMaster
+    $Domain.PDCEmulator
+    $Domain.RIDMaster
+    $Domain.InfrastructureMaster
+)
 
 # Get the details of each FSMO role holder
 foreach ($role in $FSMORoles) {

Active Directory/Export-AllADUserTransitiveGroupMemberships.ps1

@@ -52,7 +52,7 @@ begin {
                         Get-ADUserTransitiveGroupMembership -UserDN $_.DistinguishedName
                     }
                 }
-            Write-Verbose -Message "  - Found $($UserCount) users in the domain."
+            Write-Verbose -Message "  - Found $($Users.Count) users in the domain."
 
             # Export the data to a JSON file.
             $JsonData = $Users | ConvertTo-Json
@@ -148,7 +148,7 @@ begin {
             }
 
             $CurrentProgressPreference = Get-Variable -Name ProgressPreference -ValueOnly
-            Set-Variable -Name ProgressPreference 'SilentlyContinue' -Scope Global -Force -ErrorAction SilentlyContinue
+            Set-Variable -Name ProgressPreference -Value 'SilentlyContinue' -Scope Global -Force -ErrorAction SilentlyContinue
             # Check if the global catalog server is available on the specified port.
             if (-not (Test-NetConnection -ComputerName $Server -Port $Port -InformationLevel Quiet -ErrorAction SilentlyContinue)) {
                 if (-not (Test-NetConnection -ComputerName $Server -Port $AltPort -InformationLevel Quiet -ErrorAction SilentlyContinue)) {

Active Directory/Export-AllGroupMemberships.ps1

@@ -30,7 +30,7 @@ function Export-AllUserGroupMemberships {
     process {
         # Get all users in the domain and their group memberships.
         Write-Verbose -Message 'Getting all enabled users in the domain.'
-        $Users = Get-ADUser -Filter 'Enabled -eq $true' -Properties EmployeeId, memberOf |
+        $Users = Get-ADUser -Filter { Enabled -eq $true } -Properties EmployeeId, memberOf |
             Select-Object Name, DisplayName, samAccountName, userPrincipalName, EmployeeId, memberOf
         Write-Verbose -Message "  - Found $($Users.Count) users in the domain."
 

Active Directory/Get-ADAttributeUniqueValues.ps1

@@ -47,7 +47,7 @@ function Get-ADAttributeUniqueValues {
         [ValidateNotNullOrEmpty()]
         [ValidateSet('company', 'country', 'department', 'homeDrive', 'l', 'physicalDeliveryOfficeName', 'postalCode', 'state', 'streetAddress', 'title')]
         [string[]]
-        $AttributesToCheck = @('Company', 'Department', 'Office', 'Title'),
+        $AttributesToCheck = @('company', 'department', 'physicalDeliveryOfficeName', 'title'),
 
         # The directory to save the exported JSON file in. (Optional, defaults to C:\Temp.)
         [Parameter()]

Active Directory/Get-ADObjectFromPipeline.ps1

@@ -16,7 +16,10 @@ function Get-ADObjectFromPipeline {
     begin {
         Import-Module ActiveDirectory
         $GlobalCatalog = Get-ADDomainController -Discover -Service GlobalCatalog
+    }
 
+    process {
+        # Resolve identity type in process block where pipeline input ($Identity) is available.
         if ($Identity -is [Microsoft.ActiveDirectory.Management.ADUser]) {
             # We have an ADUser object
             # Might want to normalize the type to an ADObject IF we can get sidHistory from an ADObject
@@ -30,9 +33,6 @@ function Get-ADObjectFromPipeline {
             $Identity = Get-ADObject -Filter "Name -eq `"$Identity`""
         }
         $IdentityType = $Identity.ObjectClass
-    }
-
-    process {
         switch ($IdentityType) {
             'user' {
                 # Not Complete

Active Directory/Get-ADUserTransitiveGroupMembership.ps1

@@ -57,7 +57,7 @@ function Get-ADUserTransitiveGroupMembership {
         }
 
         $CurrentProgressPreference = Get-Variable -Name ProgressPreference -ValueOnly
-        Set-Variable -Name ProgressPreference 'SilentlyContinue' -Force -Scope Global -ErrorAction SilentlyContinue
+        Set-Variable -Name ProgressPreference -Value 'SilentlyContinue' -Force -Scope Global -ErrorAction SilentlyContinue
         # Check if the global catalog server is available on the specified port.
         if (-not (Test-NetConnection -ComputerName $Server -Port $Port -InformationLevel Quiet -ErrorAction SilentlyContinue)) {
             if (-not (Test-NetConnection -ComputerName $Server -Port $AltPort -InformationLevel Quiet -ErrorAction SilentlyContinue)) {
@@ -80,10 +80,11 @@ function Get-ADUserTransitiveGroupMembership {
         $TransitiveMemberOfGroupDNs = foreach ($result in ($results.properties)) {
             $result['distinguishedname']
         }
+        # Emit deduplicated results per user in process block so pipeline results are not overwritten.
+        $TransitiveMemberOfGroupDNs | Sort-Object -Unique
     }
 
     end {
-        $TransitiveMemberOfGroupDNs | Sort-Object -Unique
         Remove-Variable Filter, TransitiveMemberOfGroupDNs, Results, Searcher, Server, Port, UserDN -ErrorAction SilentlyContinue
     }
 } # end function Get-ADUserTransitiveGroupMembership