Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses module dependency loading issues for PowerShell 7 by implementing a comprehensive MSAL DLL preloading mechanism and adding a build script to resolve dependencies. The changes aim to prevent DLL conflicts when multiple modules (ExchangeOnlineManagement, Az.Accounts, Microsoft.Graph) that share the Microsoft.Identity.Client.dll are loaded.
Changes:
- Enhanced Initialize-Dependencies.ps1 to preload MSAL DLLs from all required modules in sorted order
- Added WindowsPowerShellRequiredModules custom property to module manifest for Windows PowerShell-only dependencies
- Added Resolve-PSDependencies.ps1 build script to install dependencies with PSResourceGet/PowerShellGet support
- Updated Get-ZtAssessmentResults.ps1 to support both PSResourceGet and PowerShellGet for module version checking
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/powershell/Initialize-Dependencies.ps1 | Refactored to preload MSAL DLLs from all required modules instead of just ExchangeOnlineManagement, with improved error handling and verbose logging |
| src/powershell/ZeroTrustAssessment.psd1 | Reformatted RequiredModules for readability and added WindowsPowerShellRequiredModules custom property |
| src/powershell/private/core/Get-ZtAssessmentResults.ps1 | Added PSResourceGet support with fallback to PowerShellGet for checking module versions |
| src/powershell/private/tests/Invoke-ZtTests.ps1 | Enhanced comment explaining why Data pillar tests run synchronously |
| build/powershell/Resolve-PSDependencies.ps1 | New script to resolve and save module dependencies with support for both Save-PSResource and Save-Module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Specifically, ExchangeOnlineManagement and Az.Accounts/Graph both use Microsoft.Identity.Client.dll. | ||
| # We must ensure the oldest compatible version is loaded first, BEFORE any modules import. | ||
|
|
||
| # $VerbosePreference = 'continue' # Ensure verbose messages are shown during module import |
There was a problem hiding this comment.
The commented-out line at line 5 sets VerbosePreference to 'continue' with a note about ensuring verbose messages are shown during module import. This is commented out, which is correct for production code, but the comment suggests it might be useful for debugging. Consider either removing this commented line entirely (since developers can set this themselves when debugging), or moving it to a developer documentation file if this is a common debugging step.
| # $VerbosePreference = 'continue' # Ensure verbose messages are shown during module import |
| Write-Verbose -Message ('Module with DLLs to load: {0}' -f (([Microsoft.PowerShell.Commands.ModuleSpecification[]]$moduleManifest.RequiredModules).Name -join ', ')) | ||
| # This method does not necessarily load the right dll (it ignores the load logic from the modules) | ||
| $msalToLoadInOrder = Get-ModuleImportOrder -Name $moduleManifest.RequiredModules.ModuleName |
There was a problem hiding this comment.
The Get-ModuleImportOrder function is being called with only module names but the function expects to filter modules that have 'Microsoft.Identity.Client.dll'. When passing $moduleManifest.RequiredModules.ModuleName, this will include modules like 'PSFramework' and 'Microsoft.Graph.Beta.Teams' which don't contain MSAL DLLs. The function will skip these modules at line 226-229 of Get-ModuleImportOrder.ps1 with a Write-Verbose message, but this means you're attempting to load DLLs from modules that don't have them. Consider filtering the modules to only those that are known to contain MSAL DLLs (e.g., ExchangeOnlineManagement, Az.Accounts, Microsoft.Graph.Authentication) before calling Get-ModuleImportOrder.
| Write-Verbose -Message ('Module with DLLs to load: {0}' -f (([Microsoft.PowerShell.Commands.ModuleSpecification[]]$moduleManifest.RequiredModules).Name -join ', ')) | |
| # This method does not necessarily load the right dll (it ignores the load logic from the modules) | |
| $msalToLoadInOrder = Get-ModuleImportOrder -Name $moduleManifest.RequiredModules.ModuleName | |
| $requiredModules = [Microsoft.PowerShell.Commands.ModuleSpecification[]]$moduleManifest.RequiredModules | |
| Write-Verbose -Message ('All required modules from manifest: {0}' -f ($requiredModules.Name -join ', ')) | |
| # Only consider modules known to ship Microsoft.Identity.Client.dll (MSAL) | |
| $msalModuleNames = @( | |
| 'ExchangeOnlineManagement' | |
| 'Az.Accounts' | |
| 'Microsoft.Graph.Authentication' | |
| ) | |
| $msalRequiredModules = $requiredModules | Where-Object { $msalModuleNames -contains $_.Name } | |
| if (-not $msalRequiredModules) { | |
| Write-Verbose -Message 'No MSAL-relevant required modules found in manifest; skipping MSAL pre-load.' | |
| return | |
| } | |
| Write-Verbose -Message ('Modules with MSAL DLLs to load: {0}' -f ($msalRequiredModules.Name -join ', ')) | |
| # This method does not necessarily load the right dll (it ignores the load logic from the modules) | |
| $msalToLoadInOrder = Get-ModuleImportOrder -Name $msalRequiredModules.Name |
| $msalToLoadInOrder.ForEach{ | ||
| Write-Verbose -Message ('Loading MSAL v{0} for dependency {1} version {2}' -f $_.DLLVersion, $_.Name, $_.ModuleVersion) | ||
| if ([System.AppDomain]::CurrentDomain.GetAssemblies().Where{$_.GetName().Name -eq 'Microsoft.Identity.Client'}) { | ||
| Write-Verbose "MSAL v$($_.DLLVersion) is already loaded, skipping." | ||
| } | ||
| else | ||
| { | ||
| $null = [System.Reflection.Assembly]::LoadFrom($_.DLLPath) | ||
| } | ||
|
|
||
| # Load related DLLs (Brokers, etc.) | ||
| $msalDir = Split-Path $exoModule.DLLPath | ||
| Get-ChildItem -Path $msalDir -Filter "Microsoft.Identity.Client*.dll" -File | ForEach-Object { | ||
| if ($_.Name -ne "Microsoft.Identity.Client.dll") { | ||
| try { | ||
| [System.Reflection.Assembly]::LoadFrom($_.FullName) | Out-Null | ||
| } | ||
| catch { | ||
| } | ||
| } | ||
| $brokerInteropToLoad = Get-ChildItem -Path (Split-Path -Path $_.DLLPath) -Filter "Microsoft.Identity.Client*.dll" -File | Where-Object { $_.Name -ne "Microsoft.Identity.Client.dll" } | ||
| foreach ($broker in $brokerInteropToLoad) { | ||
| Write-Verbose -Message ('Loading related MSAL broker/interop assembly {0}' -f $broker.Name) | ||
| try { | ||
| $null = [System.Reflection.Assembly]::LoadFrom($broker.FullName) | ||
| } | ||
| catch { | ||
| Write-Warning -Message ("Failed to load related MSAL assembly {0}: {1}" -f $broker.FullName, $_) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ForEach method on line 33 is used on a collection that could potentially be empty or null if Get-ModuleImportOrder returns no modules with MSAL DLLs. While this won't cause an error in PowerShell, it's worth adding a null/empty check before the ForEach to avoid unnecessary processing and to make the code more explicit about its expectations. Additionally, if no modules are found with MSAL DLLs, the script continues silently which might mask a misconfiguration issue.
| if ([System.AppDomain]::CurrentDomain.GetAssemblies().Where{$_.GetName().Name -eq 'Microsoft.Identity.Client'}) { | ||
| Write-Verbose "MSAL v$($_.DLLVersion) is already loaded, skipping." | ||
| } | ||
| else | ||
| { | ||
| $null = [System.Reflection.Assembly]::LoadFrom($_.DLLPath) | ||
| } | ||
|
|
||
| # Load related DLLs (Brokers, etc.) | ||
| $msalDir = Split-Path $exoModule.DLLPath | ||
| Get-ChildItem -Path $msalDir -Filter "Microsoft.Identity.Client*.dll" -File | ForEach-Object { | ||
| if ($_.Name -ne "Microsoft.Identity.Client.dll") { | ||
| try { | ||
| [System.Reflection.Assembly]::LoadFrom($_.FullName) | Out-Null | ||
| } | ||
| catch { | ||
| } | ||
| } | ||
| $brokerInteropToLoad = Get-ChildItem -Path (Split-Path -Path $_.DLLPath) -Filter "Microsoft.Identity.Client*.dll" -File | Where-Object { $_.Name -ne "Microsoft.Identity.Client.dll" } | ||
| foreach ($broker in $brokerInteropToLoad) { | ||
| Write-Verbose -Message ('Loading related MSAL broker/interop assembly {0}' -f $broker.Name) | ||
| try { | ||
| $null = [System.Reflection.Assembly]::LoadFrom($broker.FullName) | ||
| } | ||
| catch { | ||
| Write-Warning -Message ("Failed to load related MSAL assembly {0}: {1}" -f $broker.FullName, $_) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic at lines 35-41 has a flaw. The check for whether MSAL is already loaded happens inside the ForEach loop, which means it will check and potentially skip loading for every module in the list. However, once the first module loads MSAL (line 40), all subsequent modules in the loop will hit the condition at line 35 and skip loading their DLL. This means only the first module's MSAL DLL gets loaded, but the broker DLLs (lines 43-52) are still attempted for all modules. This seems intentional based on the sorting done by Get-ModuleImportOrder, but it would be clearer to break out of the loop after the first DLL is loaded or add a comment explaining why we continue processing broker DLLs for all modules even after MSAL is loaded.
| $requiredModuleToSave.ForEach{ | ||
| $_ | &$saveModuleCmd @saveModuleCmdParams |
There was a problem hiding this comment.
The Save-PSResource and Save-Module commands have different parameter requirements. Save-PSResource uses 'Name' parameter to specify which module to save, while Save-Module also uses 'Name'. However, on line 85, you're piping ModuleSpecification objects (which have properties like ModuleName, GUID, ModuleVersion) to these commands. The commands expect either a string name or specific parameters. Save-Module can accept pipeline input for the Name parameter, but it won't automatically map the ModuleName property from a ModuleSpecification object. You should either use '-Name $.ModuleName -RequiredVersion $.ModuleVersion' explicitly, or convert the ModuleSpecification objects appropriately before piping them.
| $requiredModuleToSave.ForEach{ | |
| $_ | &$saveModuleCmd @saveModuleCmdParams | |
| $usingSavePSResource = $saveModuleCmd.Name -eq 'Save-PSResource' | |
| $requiredModuleToSave.ForEach{ | |
| $moduleParams = @{ | |
| Name = $_.Name | |
| } | |
| if ($usingSavePSResource) | |
| { | |
| if ($_.RequiredVersion) | |
| { | |
| $moduleParams.Version = $_.RequiredVersion | |
| } | |
| elseif ($_.ModuleVersion) | |
| { | |
| $moduleParams.Version = $_.ModuleVersion | |
| } | |
| } | |
| else | |
| { | |
| if ($_.RequiredVersion) | |
| { | |
| $moduleParams.RequiredVersion = $_.RequiredVersion | |
| } | |
| elseif ($_.ModuleVersion) | |
| { | |
| $moduleParams.RequiredVersion = $_.ModuleVersion | |
| } | |
| } | |
| & $saveModuleCmd @saveModuleCmdParams @moduleParams |
| } | ||
| else { | ||
| Write-Verbose -Message "Neither PowerShellGet nor PSResourceGet is available. Cannot determine latest module version." | ||
| 'Unknown' |
There was a problem hiding this comment.
Line 35 has inconsistent indentation. The rest of the codebase uses tabs for indentation, but this line appears to use spaces. This should be changed to use tabs to match the surrounding code and the rest of the codebase.
| 'Unknown' | |
| 'Unknown' |
|
|
||
| $msalToLoadInOrder.ForEach{ | ||
| Write-Verbose -Message ('Loading MSAL v{0} for dependency {1} version {2}' -f $_.DLLVersion, $_.Name, $_.ModuleVersion) | ||
| if ([System.AppDomain]::CurrentDomain.GetAssemblies().Where{$_.GetName().Name -eq 'Microsoft.Identity.Client'}) { | ||
| Write-Verbose "MSAL v$($_.DLLVersion) is already loaded, skipping." | ||
| } | ||
| else | ||
| { | ||
| $null = [System.Reflection.Assembly]::LoadFrom($_.DLLPath) |
There was a problem hiding this comment.
The Where-Object filter on line 35 checks if 'Microsoft.Identity.Client' is already loaded in the AppDomain. However, this check happens inside the ForEach loop after Get-ModuleImportOrder has already done the work to find and sort all modules. The check at line 35 will only be true once (when the first MSAL DLL is loaded), meaning subsequent iterations will still execute the Where-Object but find nothing. This is inefficient. Consider checking if MSAL is already loaded before calling Get-ModuleImportOrder entirely, and skip the entire DLL loading process if it's already loaded.
| $msalToLoadInOrder.ForEach{ | |
| Write-Verbose -Message ('Loading MSAL v{0} for dependency {1} version {2}' -f $_.DLLVersion, $_.Name, $_.ModuleVersion) | |
| if ([System.AppDomain]::CurrentDomain.GetAssemblies().Where{$_.GetName().Name -eq 'Microsoft.Identity.Client'}) { | |
| Write-Verbose "MSAL v$($_.DLLVersion) is already loaded, skipping." | |
| } | |
| else | |
| { | |
| $null = [System.Reflection.Assembly]::LoadFrom($_.DLLPath) | |
| $msalLoaded = $false | |
| $msalToLoadInOrder.ForEach{ | |
| Write-Verbose -Message ('Loading MSAL v{0} for dependency {1} version {2}' -f $_.DLLVersion, $_.Name, $_.ModuleVersion) | |
| if ($msalLoaded) { | |
| Write-Verbose "MSAL v$($_.DLLVersion) is already loaded, skipping." | |
| } | |
| else { | |
| $null = [System.Reflection.Assembly]::LoadFrom($_.DLLPath) | |
| $msalLoaded = $true |
| Write-Verbose -Message ('Loading related MSAL broker/interop assembly {0}' -f $broker.Name) | ||
| try { | ||
| $null = [System.Reflection.Assembly]::LoadFrom($broker.FullName) | ||
| } | ||
| catch { | ||
| Write-Warning -Message ("Failed to load related MSAL assembly {0}: {1}" -f $broker.FullName, $_) |
There was a problem hiding this comment.
The broker/interop DLL loading at lines 43-52 happens for every module in the list, even after the main MSAL DLL is loaded. This could result in attempting to load broker DLLs multiple times from different module versions. The try-catch block will suppress errors, but this could lead to unexpected behavior if different versions of broker DLLs exist across modules. Consider checking if each broker DLL is already loaded before attempting to load it, similar to the check done for the main MSAL DLL.
| Write-Verbose -Message ('Loading related MSAL broker/interop assembly {0}' -f $broker.Name) | |
| try { | |
| $null = [System.Reflection.Assembly]::LoadFrom($broker.FullName) | |
| } | |
| catch { | |
| Write-Warning -Message ("Failed to load related MSAL assembly {0}: {1}" -f $broker.FullName, $_) | |
| $brokerAssemblyName = [System.IO.Path]::GetFileNameWithoutExtension($broker.Name) | |
| if ([System.AppDomain]::CurrentDomain.GetAssemblies().Where{ $_.GetName().Name -eq $brokerAssemblyName }) { | |
| Write-Verbose -Message ("Broker/interop assembly {0} is already loaded, skipping." -f $brokerAssemblyName) | |
| } | |
| else { | |
| Write-Verbose -Message ('Loading related MSAL broker/interop assembly {0}' -f $broker.Name) | |
| try { | |
| $null = [System.Reflection.Assembly]::LoadFrom($broker.FullName) | |
| } | |
| catch { | |
| Write-Warning -Message ("Failed to load related MSAL assembly {0}: {1}" -f $broker.FullName, $_) | |
| } |
| (Find-PSResource -Name ZeroTrustAssessment).Version -as [string] | ||
| } | ||
| elseif (Get-Command 'Find-Module' -ErrorAction SilentlyContinue) { | ||
| (Find-Module -Name ZeroTrustAssessment).Version -as [string] | ||
| } | ||
| else { | ||
| Write-Verbose -Message "Neither PowerShellGet nor PSResourceGet is available. Cannot determine latest module version." | ||
| 'Unknown' |
There was a problem hiding this comment.
The return statements at lines 28, 31, and 35 are not using explicit 'return' keyword. While this works in PowerShell, it means these values are emitted to the pipeline. In the context of this nested function (inside Get-LatestVersionFromPSGallery), this is correct behavior. However, the function could return multiple values if both conditions on lines 27 and 30 are somehow true (though unlikely). Consider adding explicit 'return' keywords for clarity and to ensure only one value is returned.
| (Find-PSResource -Name ZeroTrustAssessment).Version -as [string] | |
| } | |
| elseif (Get-Command 'Find-Module' -ErrorAction SilentlyContinue) { | |
| (Find-Module -Name ZeroTrustAssessment).Version -as [string] | |
| } | |
| else { | |
| Write-Verbose -Message "Neither PowerShellGet nor PSResourceGet is available. Cannot determine latest module version." | |
| 'Unknown' | |
| return (Find-PSResource -Name ZeroTrustAssessment).Version -as [string] | |
| } | |
| elseif (Get-Command 'Find-Module' -ErrorAction SilentlyContinue) { | |
| return (Find-Module -Name ZeroTrustAssessment).Version -as [string] | |
| } | |
| else { | |
| Write-Verbose -Message "Neither PowerShellGet nor PSResourceGet is available. Cannot determine latest module version." | |
| return 'Unknown' |
| if ((Get-Module -Name 'Microsoft.PowerShell.PSResourceGet') -or (Get-Command 'Find-PSResource' -ErrorAction Ignore)) { | ||
| (Find-PSResource -Name ZeroTrustAssessment).Version -as [string] | ||
| } | ||
| elseif (Get-Command 'Find-Module' -ErrorAction SilentlyContinue) { | ||
| (Find-Module -Name ZeroTrustAssessment).Version -as [string] | ||
| } | ||
| else { | ||
| Write-Verbose -Message "Neither PowerShellGet nor PSResourceGet is available. Cannot determine latest module version." | ||
| 'Unknown' | ||
| } |
There was a problem hiding this comment.
The condition at line 27 checks if PSResourceGet module is loaded OR if Find-PSResource command exists. However, both Find-PSResource and Find-Module can return multiple results (e.g., from different repositories), and the code doesn't handle this. Additionally, these commands can fail if no internet connection is available or if the module doesn't exist in the repository. Add error handling with -ErrorAction Stop/SilentlyContinue and handle the case where the command returns null or multiple results. Consider using 'Select-Object -First 1' to ensure only one version is returned.
| if ((Get-Module -Name 'Microsoft.PowerShell.PSResourceGet') -or (Get-Command 'Find-PSResource' -ErrorAction Ignore)) { | |
| (Find-PSResource -Name ZeroTrustAssessment).Version -as [string] | |
| } | |
| elseif (Get-Command 'Find-Module' -ErrorAction SilentlyContinue) { | |
| (Find-Module -Name ZeroTrustAssessment).Version -as [string] | |
| } | |
| else { | |
| Write-Verbose -Message "Neither PowerShellGet nor PSResourceGet is available. Cannot determine latest module version." | |
| 'Unknown' | |
| } | |
| try { | |
| if ((Get-Module -Name 'Microsoft.PowerShell.PSResourceGet' -ErrorAction SilentlyContinue) -or | |
| (Get-Command 'Find-PSResource' -ErrorAction SilentlyContinue)) { | |
| $resource = Find-PSResource -Name ZeroTrustAssessment -ErrorAction Stop | Select-Object -First 1 | |
| if ($null -ne $resource) { | |
| $resource.Version -as [string] | |
| } | |
| else { | |
| Write-Verbose -Message "Find-PSResource did not return any results for ZeroTrustAssessment." | |
| 'Unknown' | |
| } | |
| } | |
| elseif (Get-Command 'Find-Module' -ErrorAction SilentlyContinue) { | |
| $module = Find-Module -Name ZeroTrustAssessment -ErrorAction Stop | Select-Object -First 1 | |
| if ($null -ne $module) { | |
| $module.Version -as [string] | |
| } | |
| else { | |
| Write-Verbose -Message "Find-Module did not return any results for ZeroTrustAssessment." | |
| 'Unknown' | |
| } | |
| } | |
| else { | |
| Write-Verbose -Message "Neither PowerShellGet nor PSResourceGet is available. Cannot determine latest module version." | |
| 'Unknown' | |
| } | |
| } | |
| catch { | |
| Write-Verbose -Message ("Failed to determine latest module version: {0}" -f $_.Exception.Message) | |
| 'Unknown' | |
| } |
This PR resolves the preloading of the MSAL dll for every PS7 dependencies.
It also provides a step towards solving the Windows PS dependency, but leaving in draft until this is integrated.
To test this, if you don't yet have the dependencies installed you can run:
There are some improvements and integration to be done before marking this PR as ready:
Let me know if I'm missing anything.