Enhance module dependencies and connection handling #734
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the ZeroTrustAssessment module to support multiple Microsoft services and addresses DLL version conflicts between modules. The main objective is to enable connections to Exchange Online, Security & Compliance, and SharePoint Online in addition to the existing Microsoft Graph and Azure support, while preventing Microsoft.Identity.Client.dll conflicts through proper module loading order.
Key Changes:
- Added comprehensive multi-service connection support (Exchange Online, Security & Compliance, SharePoint Online)
- Implemented DLL conflict prevention through pre-loading strategy and module import order optimization
- Separated synchronous and parallel test execution for improved reliability
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
ZeroTrustAssessment.psd1 |
Reordered RequiredModules to prioritize ExchangeOnlineManagement, downgraded Graph module versions to 2.0.0, and added Initialize-Dependencies.ps1 to ScriptsToProcess |
Initialize-Dependencies.ps1 |
New script that pre-loads ExchangeOnlineManagement assemblies before module import to prevent DLL conflicts |
Get-ModuleImportOrder.ps1 |
New utility function that determines optimal module import order based on Microsoft.Identity.Client.dll versions |
Connect-ZtAssessment.ps1 |
Significantly expanded to support multiple services (Azure, Exchange Online, Security & Compliance, SharePoint Online) with dynamic module loading based on import order |
Invoke-ZtTests.ps1 |
Refactored to separate synchronous tests (Compliance/Exchange) from parallel tests for better execution control |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (-not $ExoUPN) { | ||
| Write-Host "`nUnable to determine a UserPrincipalName for Security & Compliance. Please supply -UserPrincipalName or connect to Exchange Online first." -ForegroundColor Yellow | ||
| continue |
There was a problem hiding this comment.
The condition uses 'continue' which is only valid inside loops. In the context of a switch statement at this level, 'continue' will skip to the next item in the switch, but since the switch is iterating over $OrderedImport.Name, this may not behave as intended. Consider using 'break' to exit the current switch case instead.
| # Get UPN from Exchange connection or Graph context | ||
| $ExoUPN = $UserPrincipalName | ||
|
|
||
| # Attempt to resolve UPN before any connection to avoid token acquisition failures without identity | ||
| $connectionInformation = $null | ||
| try { | ||
| $connectionInformation = Get-ConnectionInformation | ||
| } | ||
| catch { | ||
| # Intentionally swallow errors here; fall back to provided UPN if any | ||
| } | ||
|
|
||
| if (-not $ExoUPN) { | ||
| $ExoUPN = $connectionInformation | Where-Object { $_.IsEopSession -ne $true -and $_.State -eq 'Connected' } | Select-Object -ExpandProperty UserPrincipalName -First 1 -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| if (-not $ExoUPN) { | ||
| Write-Host "`nUnable to determine a UserPrincipalName for Security & Compliance. Please supply -UserPrincipalName or connect to Exchange Online first." -ForegroundColor Yellow | ||
| continue | ||
| } |
There was a problem hiding this comment.
The logic for Security & Compliance requires a UserPrincipalName but attempts to retrieve it from Exchange connection information. If the user specifies 'SecurityCompliance' without 'ExchangeOnline' in the Service parameter, and without providing UserPrincipalName, the code will automatically add 'ExchangeOnline' (line 106). However, if the Exchange connection fails, the Get-ConnectionInformation call may not return valid data. Consider validating that the UserPrincipalName was successfully obtained before attempting the Security & Compliance connection.
| $connectionInformation = Get-ConnectionInformation | ||
| } | ||
| catch { | ||
| # Intentionally swallow errors here; fall back to provided UPN if any |
There was a problem hiding this comment.
The catch block intentionally swallows all errors with only a comment explaining the intent. While this may be intentional for resilience, it could hide legitimate errors that would help diagnose connection issues. Consider logging these errors at a verbose or debug level so they can be reviewed if needed.
| # Intentionally swallow errors here; fall back to provided UPN if any | |
| # Intentionally swallow errors here; fall back to provided UPN if any | |
| $connectionInfoError = $_ | |
| Write-Verbose "Get-ConnectionInformation failed; falling back to provided UserPrincipalName if available. Error: $($connectionInfoError.Exception.Message)" |
| # Initialize-Dependencies.ps1 | ||
| # This script is run by the module manifest (ScriptsToProcess) before the module is imported. | ||
| # It ensures that dependencies are loaded in the correct order to avoid DLL conflicts. | ||
| # 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. | ||
|
|
||
| Write-Host "=== Initialize-Dependencies.ps1 Starting ===" -ForegroundColor Cyan | ||
|
|
||
| try { | ||
| # Check if MSAL is already loaded | ||
| $loadedAssemblies = [System.AppDomain]::CurrentDomain.GetAssemblies() | Where-Object { $_.GetName().Name -eq 'Microsoft.Identity.Client' } | ||
|
|
||
| if ($loadedAssemblies) { | ||
| Write-Host "MSAL assembly is already loaded in the current session:" -ForegroundColor Yellow | ||
| foreach ($asm in $loadedAssemblies) { | ||
| $loadedVersion = $asm.GetName().Version | ||
| Write-Host " Version $loadedVersion from $($asm.Location)" -ForegroundColor Yellow | ||
| } | ||
| Write-Host "" -ForegroundColor Yellow | ||
| Write-Host "This will cause DLL conflicts. To fix:" -ForegroundColor Red | ||
| Write-Host " 1. Close this PowerShell session" -ForegroundColor Cyan | ||
| Write-Host " 2. Open a NEW PowerShell session" -ForegroundColor Cyan | ||
| Write-Host " 3. Import ZeroTrustAssessment FIRST before any other modules" -ForegroundColor Cyan | ||
| Write-Host "" -ForegroundColor Yellow | ||
| } else { | ||
| Write-Host "MSAL not yet loaded - proceeding with pre-load" -ForegroundColor Green | ||
|
|
||
| # Dot-source the helper function to determine proper module import order | ||
| $helperPath = "$PSScriptRoot\private\utility\Get-ModuleImportOrder.ps1" | ||
| Write-Host "Loading helper from: $helperPath" -ForegroundColor Gray | ||
|
|
||
| if (-not (Test-Path $helperPath)) { | ||
| Write-Error "Cannot find Get-ModuleImportOrder.ps1 at $helperPath" | ||
| return | ||
| } | ||
|
|
||
| . $helperPath | ||
|
|
||
| # Define dependencies and their required versions | ||
| $dependencies = @('Az.Accounts', 'ExchangeOnlineManagement', 'Microsoft.Graph.Authentication', 'Microsoft.Graph.Beta.Teams') | ||
| $requiredVersions = @{ | ||
| 'ExchangeOnlineManagement' = '3.8.0' | ||
| } | ||
|
|
||
| # CRITICAL: ExchangeOnlineManagement must be loaded FIRST due to its specific Broker DLL requirements | ||
| # Even if other modules have newer MSAL versions, Exchange needs its specific Broker extensions | ||
| Write-Host "Looking for ExchangeOnlineManagement module..." -ForegroundColor Gray | ||
| $exoModule = Get-ModuleImportOrder -Name 'ExchangeOnlineManagement' -RequiredVersions $requiredVersions | ||
|
|
||
| if ($exoModule) { | ||
| Write-Host "Loading MSAL and Broker from ExchangeOnlineManagement v$($exoModule.ModuleVersion)" -ForegroundColor Green | ||
|
|
||
| if ($exoModule.DLLPath -and (Test-Path $exoModule.DLLPath)) { | ||
| Write-Host "Explicitly loading MSAL assembly from: $($exoModule.DLLPath)" -ForegroundColor Cyan | ||
| $loadedAsm = [System.Reflection.Assembly]::LoadFrom($exoModule.DLLPath) | ||
| Write-Host "Successfully loaded MSAL DLL version $($loadedAsm.GetName().Version)" -ForegroundColor Green | ||
|
|
||
| # Load ALL Microsoft.Identity.Client related DLLs from Exchange's directory | ||
| $msalDirectory = Split-Path $exoModule.DLLPath | ||
| $relatedDlls = Get-ChildItem -Path $msalDirectory -Filter "Microsoft.Identity.Client*.dll" -File | ||
|
|
||
| foreach ($dll in $relatedDlls) { | ||
| if ($dll.Name -ne "Microsoft.Identity.Client.dll") { | ||
| Write-Host "Loading related assembly: $($dll.Name)" -ForegroundColor Gray | ||
| try { | ||
| [System.Reflection.Assembly]::LoadFrom($dll.FullName) | Out-Null | ||
| Write-Host " Successfully loaded $($dll.Name)" -ForegroundColor Green | ||
| } catch { | ||
| Write-Host " Failed to load $($dll.Name): $_" -ForegroundColor Yellow | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| Write-Warning "Could not find Microsoft.Identity.Client.dll for ExchangeOnlineManagement at $($exoModule.DLLPath)" | ||
| } | ||
| } else { | ||
| Write-Warning "ExchangeOnlineManagement 3.8.0 or higher not found. MSAL DLL conflict will occur." | ||
| } | ||
| } | ||
| Write-Host "=== Initialize-Dependencies.ps1 Complete ===" -ForegroundColor Cyan | ||
| } | ||
| catch { | ||
| Write-Host "=== Initialize-Dependencies.ps1 FAILED ===" -ForegroundColor Red | ||
| Write-Host "Error: $_" -ForegroundColor Red | ||
| Write-Host $_.ScriptStackTrace -ForegroundColor Red | ||
| # Don't throw - let RequiredModules try to proceed | ||
| } |
There was a problem hiding this comment.
The Initialize-Dependencies.ps1 script will execute every time the module is imported, which could add noticeable overhead during module loading. The DLL conflict checks and assembly loading operations are relatively expensive. Consider caching the results or adding a mechanism to skip this initialization if it has already been performed in the current session.
| Write-Host "`nFailed to import SharePoint Online module: $_" -ForegroundColor Red | ||
| Write-PSFMessage "Failed to import SharePoint Online module: $_" -Level Error |
There was a problem hiding this comment.
The Import-Module with -UseWindowsPowerShell flag is used for PowerShell Core compatibility, but this requires that the module is installed in Windows PowerShell (5.1). The error message at line 342 doesn't provide guidance about this prerequisite. Consider enhancing the error message to inform users that the SharePoint module must be installed in Windows PowerShell 5.1 when running from PowerShell Core.
| Write-Host "`nFailed to import SharePoint Online module: $_" -ForegroundColor Red | |
| Write-PSFMessage "Failed to import SharePoint Online module: $_" -Level Error | |
| # Provide clearer guidance when import fails, especially under PowerShell Core | |
| if ($PSVersionTable.PSEdition -ne 'Desktop') { | |
| $message = "Failed to import SharePoint Online module. When running in PowerShell Core, 'Microsoft.Online.SharePoint.PowerShell' must be installed in Windows PowerShell 5.1 (Desktop) for -UseWindowsPowerShell to work. Underlying error: $_" | |
| } | |
| else { | |
| $message = "Failed to import SharePoint Online module: $_" | |
| } | |
| Write-Host "`n$message" -ForegroundColor Red | |
| Write-PSFMessage $message -Level Error |
| $azParams = @{ | ||
| UseDeviceAuthentication = $UseDeviceCode | ||
| Environment = $azEnvironment | ||
| Tenant = $TenantId ? $TenantId : $contextTenantId |
There was a problem hiding this comment.
The variable $contextTenantId is used on line 187 but may not be defined if the Graph service was not connected (e.g., if Service only contains 'Azure'). This will cause an error or unexpected behavior when attempting to connect to Azure without first connecting to Graph.
| $azParams = @{ | |
| UseDeviceAuthentication = $UseDeviceCode | |
| Environment = $azEnvironment | |
| Tenant = $TenantId ? $TenantId : $contextTenantId | |
| $tenantParam = $TenantId | |
| if (-not $tenantParam) { | |
| $tenantVar = Get-Variable -Name 'contextTenantId' -ErrorAction SilentlyContinue | |
| if ($tenantVar) { | |
| $tenantParam = $tenantVar.Value | |
| } | |
| } | |
| $azParams = @{ | |
| UseDeviceAuthentication = $UseDeviceCode | |
| Environment = $azEnvironment | |
| Tenant = $tenantParam |
| $adminUrl = $SharePointAdminUrl | ||
| if (-not $adminUrl) { | ||
| # Try to infer from Graph context | ||
| if ($contextTenantId) { |
There was a problem hiding this comment.
The variable $contextTenantId is referenced but may not be defined if the Graph connection was not established or skipped. This could lead to runtime errors when trying to infer the SharePoint Admin URL.
| if ($contextTenantId) { | |
| $contextTenantIdValue = Get-Variable -Name contextTenantId -ErrorAction SilentlyContinue -ValueOnly | |
| if ($contextTenantIdValue) { |
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests | ||
| $syncTestIds = @('35003', '35004', '35005', '35010') | ||
| $syncTests = $testsToRun | Where-Object { $_.TestId -in $syncTestIds } | ||
| $parallelTests = $testsToRun | Where-Object { $_.TestId -notin $syncTestIds } | ||
|
|
There was a problem hiding this comment.
Hard-coded test IDs for sync tests may be fragile if test IDs change or new tests require synchronous execution. Consider using a more maintainable approach such as adding a property to the test definition to indicate whether it requires synchronous execution, or documenting why these specific test IDs must run synchronously.
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests | |
| $syncTestIds = @('35003', '35004', '35005', '35010') | |
| $syncTests = $testsToRun | Where-Object { $_.TestId -in $syncTestIds } | |
| $parallelTests = $testsToRun | Where-Object { $_.TestId -notin $syncTestIds } | |
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests. | |
| # Prefer using the test definition's RequiresSynchronousExecution property when available. | |
| # The following IDs are kept as a legacy fallback for existing tests that are known to require | |
| # synchronous execution due to dependencies on shared resources or ordering constraints. | |
| $legacySyncTestIds = @('35003', '35004', '35005', '35010') | |
| $syncTests = $testsToRun | Where-Object { | |
| ( | |
| $_.PSObject.Properties.Match('RequiresSynchronousExecution').Count -gt 0 -and | |
| $_.RequiresSynchronousExecution | |
| ) -or | |
| $_.TestId -in $legacySyncTestIds | |
| } | |
| $parallelTests = $testsToRun | Where-Object { | |
| -not ( | |
| ( | |
| $_.PSObject.Properties.Match('RequiresSynchronousExecution').Count -gt 0 -and | |
| $_.RequiresSynchronousExecution | |
| ) -or | |
| $_.TestId -in $legacySyncTestIds | |
| ) | |
| } |
| RequiredModules = @(@{ModuleName = 'ExchangeOnlineManagement'; GUID = 'b5eced50-afa4-455b-847a-d8fb64140a22'; ModuleVersion = '3.8.0'; }, | ||
| @{ModuleName = 'Microsoft.Graph.Authentication'; GUID = '883916f2-9184-46ee-b1f8-b6a2fb784cee'; ModuleVersion = '2.0.0'; }, | ||
| @{ModuleName = 'Microsoft.Graph.Beta.Teams'; GUID = 'e264919d-7ae2-4a89-ba8b-524bd93ddc08'; ModuleVersion = '2.0.0'; }, | ||
| @{ModuleName = 'Az.Accounts'; GUID = '17a2feff-488b-47f9-8729-e2cec094624c'; ModuleVersion = '4.0.2'; }, |
There was a problem hiding this comment.
The module reordering changes ExchangeOnlineManagement to be first in the RequiredModules list, but this may cause issues since the module manifest specifies that modules are loaded in the order they appear. The Initialize-Dependencies.ps1 script is meant to handle this, but if ScriptsToProcess fails or is skipped, having ExchangeOnlineManagement first could still cause the intended DLL loading order to be bypassed. Consider adding documentation or comments explaining this dependency.
Revert Microsoft.Graph.Authentication and Microsoft.Graph.Beta.Teams versions from 2.0.0 to 2.32.0 Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests | ||
| $syncTestIds = @('35003', '35004', '35005', '35010') |
There was a problem hiding this comment.
Hard-coded test IDs make this code fragile and difficult to maintain. Consider using a more maintainable approach such as adding a 'RequiresSync' property to test definitions, or storing this configuration in a central location that can be easily updated.
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests | |
| $syncTestIds = @('35003', '35004', '35005', '35010') | |
| function Get-SyncTestIds { | |
| <# | |
| .SYNOPSIS | |
| Returns the list of test IDs that must run synchronously. | |
| .DESCRIPTION | |
| Attempts to load sync test IDs from a JSON configuration file | |
| located alongside this script (SyncTests.json), falling back to | |
| the default hard-coded IDs if the file or property is not present. | |
| Expected JSON structure: | |
| { | |
| "SyncTestIds": [ "35003", "35004", "35005", "35010" ] | |
| } | |
| #> | |
| # Default sync test IDs (current behavior) | |
| $defaultSyncTestIds = @('35003', '35004', '35005', '35010') | |
| # Attempt to load overrides from configuration file, if available | |
| try { | |
| $configPath = Join-Path -Path $PSScriptRoot -ChildPath 'SyncTests.json' | |
| if (Test-Path -LiteralPath $configPath) { | |
| $configContent = Get-Content -LiteralPath $configPath -Raw | |
| if ($configContent) { | |
| $config = $configContent | ConvertFrom-Json | |
| if ($config.SyncTestIds -and $config.SyncTestIds.Count -gt 0) { | |
| return [string[]]$config.SyncTestIds | |
| } | |
| } | |
| } | |
| } | |
| catch { | |
| # Swallow any errors and fall back to the default list | |
| } | |
| return $defaultSyncTestIds | |
| } | |
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests | |
| $syncTestIds = Get-SyncTestIds |
| Write-Host "`nThe Exchange Online module is not installed. Please install the module using the following command.`nFor more information see https://learn.microsoft.com/powershell/exchange/exchange-online-powershell-v2" -ForegroundColor Red | ||
| Write-Host "`nInstall-Module ExchangeOnlineManagement -Scope CurrentUser`n" -ForegroundColor Yellow |
There was a problem hiding this comment.
The error message should clarify that ExchangeOnlineManagement version 3.8.0 or higher is specifically required, not just any version of the module. This matches the required version specified in the module manifest.
| Write-Host "`nThe Exchange Online module is not installed. Please install the module using the following command.`nFor more information see https://learn.microsoft.com/powershell/exchange/exchange-online-powershell-v2" -ForegroundColor Red | |
| Write-Host "`nInstall-Module ExchangeOnlineManagement -Scope CurrentUser`n" -ForegroundColor Yellow | |
| Write-Host "`nThe Exchange Online module (ExchangeOnlineManagement 3.8.0 or higher) is not installed. Please install the required version using the following command.`nFor more information see https://learn.microsoft.com/powershell/exchange/exchange-online-powershell-v2" -ForegroundColor Red | |
| Write-Host "`nInstall-Module ExchangeOnlineManagement -Scope CurrentUser -MinimumVersion 3.8.0`n" -ForegroundColor Yellow |
| $testsToRun = $testsToRun | Where-Object { $_.Pillar -in $stablePillars } | ||
| } | ||
|
|
||
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests |
There was a problem hiding this comment.
The comment uses inconsistent capitalization for 'Compliance/Exchange'. In PowerShell comments, service names should be consistently capitalized. Consider using 'Exchange/Compliance' to match the order they appear in the test IDs or be consistent with how these services are named elsewhere in the codebase.
| # Separate Sync Tests (Compliance/Exchange) from Parallel Tests | |
| # Separate Sync Tests (Exchange/Compliance) from Parallel Tests |
| switch ($OrderedImport.Name) { | ||
| 'Microsoft.Graph.Authentication' { | ||
| if ($Service -contains 'Graph' -or $Service -contains 'All') { | ||
| Write-Host "`nConnecting to Microsoft Graph" -ForegroundColor Yellow | ||
| Write-PSFMessage 'Connecting to Microsoft Graph' | ||
|
|
||
| try { | ||
| Write-PSFMessage "Connecting to Microsoft Graph with params: $($params | Out-String)" -Level Verbose | ||
| Connect-MgGraph @params -ErrorAction Stop | ||
| $contextTenantId = (Get-MgContext).TenantId | ||
| } | ||
| catch [Management.Automation.CommandNotFoundException] { | ||
| Write-Host "`nThe Graph PowerShell module is not installed. Please install the module using the following command. For more information see https://learn.microsoft.com/powershell/microsoftgraph/installation" -ForegroundColor Red | ||
| Write-Host "`Install-Module Microsoft.Graph.Authentication -Scope CurrentUser`n" -ForegroundColor Yellow | ||
| } | ||
| catch { | ||
| Stop-PSFFunction -Message "Failed to authenticate to Graph" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
|
|
||
| try { | ||
| Write-Verbose "Verifying Zero Trust context and permissions..." | ||
| Test-ZtContext | ||
| } | ||
| catch { | ||
| Stop-PSFFunction -Message "Authenticated to Graph, but the requirements for the ZeroTrustAssessment are not met by the established session:`n$_" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 'Az.Accounts' { | ||
| if ($SkipAzureConnection) { | ||
| continue | ||
| } | ||
|
|
||
| if ($Service -contains 'Azure' -or $Service -contains 'All') { | ||
| Write-Host "`nConnecting to Azure" -ForegroundColor Yellow | ||
| Write-PSFMessage 'Connecting to Azure' | ||
|
|
||
| $azEnvironment = 'AzureCloud' | ||
| if ($Environment -eq 'China') { | ||
| $azEnvironment = Get-AzEnvironment -Name AzureChinaCloud | ||
| } | ||
| elseif ($Environment -in 'USGov', 'USGovDoD') { | ||
| $azEnvironment = 'AzureUSGovernment' | ||
| } | ||
|
|
||
| $azParams = @{ | ||
| UseDeviceAuthentication = $UseDeviceCode | ||
| Environment = $azEnvironment | ||
| Tenant = $TenantId ? $TenantId : $contextTenantId | ||
| } | ||
| if ($ClientId -and $Certificate) { | ||
| $azParams.ApplicationId = $ClientId | ||
| $azParams.CertificateThumbprint = $Certificate.Certificate.Thumbprint | ||
| } | ||
|
|
||
| try { | ||
| Connect-AzAccount @azParams -ErrorAction Stop | ||
| } | ||
| catch { | ||
| Stop-PSFFunction -Message "Failed to authenticate to Azure: $_" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 'ExchangeOnlineManagement' { | ||
| if ($Service -contains 'ExchangeOnline' -or $Service -contains 'All') { | ||
| Write-Verbose 'Connecting to Microsoft Exchange Online' | ||
| try { | ||
| if ($UseDeviceCode -and $PSVersionTable.PSEdition -eq 'Desktop') { | ||
| Write-Host 'The Exchange Online module in Windows PowerShell does not support device code flow authentication.' -ForegroundColor Red | ||
| Write-Host '💡Please use the Exchange Online module in PowerShell Core.' -ForegroundColor Yellow | ||
| } | ||
| elseif ($UseDeviceCode) { | ||
| Connect-ExchangeOnline -ShowBanner:$false -Device:$UseDeviceCode -ExchangeEnvironmentName $ExchangeEnvironmentName | ||
| } | ||
| else { | ||
| Connect-ExchangeOnline -ShowBanner:$false -ExchangeEnvironmentName $ExchangeEnvironmentName | ||
| } | ||
|
|
||
| # Fix for Get-Label visibility in other scopes | ||
| if (Get-Command Get-Label -ErrorAction SilentlyContinue) { | ||
| $module = Get-Command Get-Label | Select-Object -ExpandProperty Module | ||
| if ($module -and $module.Name -like 'tmp_*') { | ||
| Import-Module $module -Global -Force | ||
| } | ||
| } | ||
| } | ||
| catch { | ||
| Write-Host "`nFailed to connect to Exchange Online: $_" -ForegroundColor Red | ||
| } | ||
| } | ||
|
|
||
| if ($Service -contains 'SecurityCompliance' -or $Service -contains 'All') { | ||
| $Environments = @{ | ||
| 'O365China' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.partner.outlook.cn/powershell-liveid' | ||
| AuthZEndpointUri = 'https://login.chinacloudapi.cn/common' | ||
| } | ||
| 'O365GermanyCloud' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | ||
| } | ||
| 'O365Default' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | ||
| } | ||
| 'O365USGovGCCHigh' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.office365.us/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.us/common' | ||
| } | ||
| 'O365USGovDoD' = @{ | ||
| ConnectionUri = 'https://l5.ps.compliance.protection.office365.us/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.us/common' | ||
| } | ||
| Default = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | ||
| } | ||
| } | ||
| Write-Verbose 'Connecting to Microsoft Security & Compliance PowerShell' | ||
|
|
||
| if ($UseDeviceCode) { | ||
| Write-Host "`nThe Security & Compliance module does not support device code flow authentication." -ForegroundColor Red | ||
| } | ||
| else { | ||
| # Get UPN from Exchange connection or Graph context | ||
| $ExoUPN = $UserPrincipalName | ||
|
|
||
| # Attempt to resolve UPN before any connection to avoid token acquisition failures without identity | ||
| $connectionInformation = $null | ||
| try { | ||
| $connectionInformation = Get-ConnectionInformation | ||
| } | ||
| catch { | ||
| # Intentionally swallow errors here; fall back to provided UPN if any | ||
| } | ||
|
|
||
| if (-not $ExoUPN) { | ||
| $ExoUPN = $connectionInformation | Where-Object { $_.IsEopSession -ne $true -and $_.State -eq 'Connected' } | Select-Object -ExpandProperty UserPrincipalName -First 1 -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| if (-not $ExoUPN) { | ||
| Write-Host "`nUnable to determine a UserPrincipalName for Security & Compliance. Please supply -UserPrincipalName or connect to Exchange Online first." -ForegroundColor Yellow | ||
| continue | ||
| } | ||
|
|
||
| try { | ||
| $ippSessionParams = @{ | ||
| BypassMailboxAnchoring = $true | ||
| UserPrincipalName = $ExoUPN | ||
| ShowBanner = $false | ||
| ErrorAction = 'Stop' | ||
| } | ||
|
|
||
| # Only override endpoints for non-default clouds to reduce token acquisition failures in Default | ||
| if ($ExchangeEnvironmentName -ne 'O365Default') { | ||
| $ippSessionParams.ConnectionUri = $Environments[$ExchangeEnvironmentName].ConnectionUri | ||
| $ippSessionParams.AzureADAuthorizationEndpointUri = $Environments[$ExchangeEnvironmentName].AuthZEndpointUri | ||
| } | ||
|
|
||
| Write-Verbose "Connecting to Security & Compliance with UPN: $ExoUPN" | ||
| Connect-IPPSSession @ippSessionParams | ||
| } | ||
| catch [Management.Automation.CommandNotFoundException] { | ||
| if (-not $ExchangeModuleNotInstalledWarningShown) { | ||
| Write-Host "`nThe Exchange Online module is not installed. Please install the module using the following command.`nFor more information see https://learn.microsoft.com/powershell/exchange/exchange-online-powershell-v2" -ForegroundColor Red | ||
| Write-Host "`nInstall-Module ExchangeOnlineManagement -Scope CurrentUser`n" -ForegroundColor Yellow | ||
| } | ||
| } | ||
| catch { | ||
| $exception = $_ | ||
| if ($exception.Exception.InnerException.Message -like "*Method not found*Microsoft.Identity.Client*" -or $exception.Exception.Message -like "*Method not found*Microsoft.Identity.Client*") { | ||
| Write-Warning "DLL Conflict detected (Method not found in Microsoft.Identity.Client). This usually happens if Microsoft.Graph is loaded before ExchangeOnlineManagement." | ||
| Write-Warning "Please RESTART your PowerShell session and run Connect-ZtAssessment again." | ||
| } | ||
|
|
||
| Write-Host "`nFailed to connect to the Security & Compliance PowerShell: $exception" -ForegroundColor Red | ||
| } | ||
| } | ||
|
|
||
| # Fix for Get-Label visibility in other scopes | ||
| if (Get-Command Get-Label -ErrorAction SilentlyContinue) { | ||
| $module = Get-Command Get-Label | Select-Object -ExpandProperty Module | ||
| if ($module -and $module.Name -like 'tmp_*') { | ||
| Import-Module $module -Global -Force | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 'Microsoft.Online.SharePoint.PowerShell' { | ||
| if ($Service -contains 'SharePointOnline' -or $Service -contains 'All') { | ||
| try { | ||
| # Import module with compatibility if needed | ||
| if ($PSVersionTable.PSEdition -ne 'Desktop') { | ||
| # Assume module is installed in Windows PowerShell as per instructions | ||
| Import-Module Microsoft.Online.SharePoint.PowerShell -UseWindowsPowerShell -WarningAction SilentlyContinue -ErrorAction Stop -Global | ||
| } | ||
| else { | ||
| Import-Module Microsoft.Online.SharePoint.PowerShell -ErrorAction Stop -Global | ||
| } | ||
| } | ||
| catch { | ||
| Write-Host "`nFailed to import SharePoint Online module: $_" -ForegroundColor Red | ||
| Write-PSFMessage "Failed to import SharePoint Online module: $_" -Level Error | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The switch statement processes modules in the order returned by Get-ModuleImportOrder, but there's no guarantee about the order of execution for services that depend on each other. For example, if SharePoint connection needs Graph context but Graph comes after SharePoint in the import order, the code at line 357 will fail because $contextTenantId won't be set. Consider either ensuring Graph is always processed first when needed, or restructuring the logic to not depend on switch execution order.
| $SkipAzureConnection | ||
| $SkipAzureConnection, | ||
|
|
||
| # The services to connect to such as Azure and EXO. Default is Graph. |
There was a problem hiding this comment.
The parameter description for $Service uses inconsistent terminology. It says "such as Azure and EXO" but the ValidateSet includes 'ExchangeOnline', not 'EXO'. The comment should use the full service name 'ExchangeOnline' to match the actual parameter values, or update all references to be consistent.
| # The services to connect to such as Azure and EXO. Default is Graph. | |
| # The services to connect to such as Azure and ExchangeOnline. Default is Graph. |
| } | ||
| catch { | ||
| $exception = $_ | ||
| if ($exception.Exception.InnerException.Message -like "*Method not found*Microsoft.Identity.Client*" -or $exception.Exception.Message -like "*Method not found*Microsoft.Identity.Client*") { |
There was a problem hiding this comment.
Catching a generic exception type and checking its message for specific strings is fragile and may not catch all DLL conflict scenarios. Consider using more specific exception types or creating a custom exception for DLL conflicts. Additionally, the check for InnerException and direct Exception could be consolidated.
| if ($exception.Exception.InnerException.Message -like "*Method not found*Microsoft.Identity.Client*" -or $exception.Exception.Message -like "*Method not found*Microsoft.Identity.Client*") { | |
| $methodNotFoundException = $null | |
| # Detect DLL conflict via a specific MissingMethodException, preferring the inner exception when present | |
| if ($exception.Exception.InnerException -is [System.MissingMethodException]) { | |
| $methodNotFoundException = $exception.Exception.InnerException | |
| } | |
| elseif ($exception.Exception -is [System.MissingMethodException]) { | |
| $methodNotFoundException = $exception.Exception | |
| } | |
| if ($methodNotFoundException -and $methodNotFoundException.Message -like "*Microsoft.Identity.Client*") { |
… on the 'Data' pillar
…and error messaging
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RequiredModules = @(@{ModuleName = 'ExchangeOnlineManagement'; GUID = 'b5eced50-afa4-455b-847a-d8fb64140a22'; ModuleVersion = '3.8.0'; }, | ||
| @{ModuleName = 'Microsoft.Graph.Authentication'; GUID = '883916f2-9184-46ee-b1f8-b6a2fb784cee'; ModuleVersion = '2.32.0'; }, | ||
| @{ModuleName = 'Microsoft.Graph.Beta.Teams'; GUID = 'e264919d-7ae2-4a89-ba8b-524bd93ddc08'; ModuleVersion = '2.32.0'; }, | ||
| @{ModuleName = 'Az.Accounts'; GUID = '17a2feff-488b-47f9-8729-e2cec094624c'; ModuleVersion = '4.0.2'; }, | ||
| @{ModuleName = 'PSFramework'; GUID = '8028b914-132b-431f-baa9-94a6952f21ff'; ModuleVersion = '1.13.419'; }) |
There was a problem hiding this comment.
The ordering of RequiredModules has changed to load ExchangeOnlineManagement first. However, when PowerShell processes RequiredModules, it loads them sequentially which can still cause DLL conflicts if Microsoft.Graph.Authentication (line 55) loads its MSAL version before ExchangeOnlineManagement's modules are fully initialized. The Initialize-Dependencies.ps1 script attempts to pre-load the DLLs, but there's a potential race condition. Consider documenting why this order is critical, or ensure the Initialize-Dependencies script is robust enough to handle cases where other modules might already be partially loaded.
| try { | ||
| $workflow = Start-ZtTestExecution -Tests $testsToRun -DbPath $Database.Database -ThrottleLimit $ThrottleLimit | ||
| Wait-ZtTest -Workflow $workflow | ||
| # Run Sync Tests in the main thread | ||
| foreach ($test in $syncTests) { | ||
| Invoke-ZtTest -Test $test -Database $Database | ||
| } | ||
|
|
||
| # Run Parallel Tests | ||
| if ($parallelTests) { | ||
| $workflow = Start-ZtTestExecution -Tests $parallelTests -DbPath $Database.Database -ThrottleLimit $ThrottleLimit | ||
| Wait-ZtTest -Workflow $workflow | ||
| } | ||
| } | ||
| finally { | ||
| if ($workflow) { |
There was a problem hiding this comment.
The variable 'workflow' is referenced in the finally block on line 82, but it may not be defined if execution never enters the try block or if only syncTests are run without parallelTests. This will cause an error when checking 'if ($workflow)'. Initialize the variable before the try block (e.g., '$workflow = $null') to avoid potential undefined variable errors.
| $contextTenantId = (Get-MgContext).TenantId | ||
| } | ||
|
|
||
| catch { | ||
| Stop-PSFFunction -Message "Failed to authenticate to Graph" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
|
|
||
| try { | ||
| Write-Verbose "Verifying Zero Trust context and permissions..." | ||
| Test-ZtContext | ||
| } | ||
| catch { | ||
| Stop-PSFFunction -Message "Authenticated to Graph, but the requirements for the ZeroTrustAssessment are not met by the established session:`n$_" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 'Az.Accounts' { | ||
| if ($SkipAzureConnection) { | ||
| continue | ||
| } | ||
|
|
||
| if ($Service -contains 'Azure' -or $Service -contains 'All') { | ||
| Write-Host "`nConnecting to Azure" -ForegroundColor Yellow | ||
| Write-PSFMessage 'Connecting to Azure' | ||
|
|
||
| $azEnvironment = 'AzureCloud' | ||
| if ($Environment -eq 'China') { | ||
| $azEnvironment = Get-AzEnvironment -Name AzureChinaCloud | ||
| } | ||
| elseif ($Environment -in 'USGov', 'USGovDoD') { | ||
| $azEnvironment = 'AzureUSGovernment' | ||
| } | ||
|
|
||
| $tenantParam = $TenantId | ||
| if (-not $tenantParam) { | ||
| $tenantVar = Get-Variable -Name 'contextTenantId' -ErrorAction SilentlyContinue | ||
| if ($tenantVar) { | ||
| $tenantParam = $tenantVar.Value | ||
| } |
There was a problem hiding this comment.
The variable 'contextTenantId' is used on line 133, but it's only set when connecting to Graph. If the 'Graph' service is not in the Service array, this variable will be undefined when referenced in the 'Az.Accounts' switch case on line 169-172. This can cause errors when trying to connect to Azure without Graph. Consider checking if the variable exists before using it or ensuring Graph is connected first if Azure connection requires it.
| # CRITICAL: ExchangeOnlineManagement must be loaded FIRST due to its specific Broker DLL requirements | ||
| # Even if other modules have newer MSAL versions, Exchange needs its specific Broker extensions |
There was a problem hiding this comment.
The comment on line 45-46 states that ExchangeOnlineManagement must be loaded FIRST due to Broker DLL requirements, but the code then uses Get-ModuleImportOrder which may return modules in a different order based on DLL versions. This creates a contradiction: the function might determine that another module should be loaded first (based on MSAL version), but the comment says Exchange must always be first. Clarify the logic: should Exchange always be first, or should it be ordered by DLL version? The current implementation is ambiguous.
| # CRITICAL: ExchangeOnlineManagement must be loaded FIRST due to its specific Broker DLL requirements | |
| # Even if other modules have newer MSAL versions, Exchange needs its specific Broker extensions | |
| # CRITICAL: ExchangeOnlineManagement is the FIRST module used to load MSAL/Broker DLLs in this script | |
| # Get-ModuleImportOrder is only used here to locate the appropriate ExchangeOnlineManagement version/path, | |
| # not to choose a different module to load MSAL from, even if other modules have newer MSAL versions. |
| Default = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | ||
| } |
There was a problem hiding this comment.
The ExchangeEnvironmentName parameter defaults to 'O365Default', but the environment mapping for SecurityCompliance on line 244 uses 'Default' as a key which doesn't match any ValidateSet value. If someone passes an unexpected value or if there's a logic error, the hashtable lookup will fail silently and fall through to the Default key. This inconsistency should be resolved—either the Default key should be removed (since O365Default covers it), or the fallback logic should be made explicit.
| Default = @{ | |
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | |
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | |
| } |
|
|
||
| # Ensure ExchangeOnline is included if SecurityCompliance is requested | ||
| if ($Service -contains 'SecurityCompliance' -and $Service -notcontains 'ExchangeOnline' -and $Service -notcontains 'All') { | ||
| Write-Verbose "Adding ExchangeOnline to the list of services to connect to as it is required for SecurityCompliance." |
There was a problem hiding this comment.
The verbose message on line 103 will be hidden from users unless they use -Verbose. Since automatically adding ExchangeOnline when SecurityCompliance is requested is important behavior that affects which services are connected, consider using Write-Host or Write-PSFMessage at a higher level so users are aware of this automatic dependency resolution.
| Write-Verbose "Adding ExchangeOnline to the list of services to connect to as it is required for SecurityCompliance." | |
| Write-PSFMessage -Level Host -Message "Adding ExchangeOnline to the list of services to connect to as it is required for SecurityCompliance." |
| switch ($OrderedImport.Name) { | ||
| 'Microsoft.Graph.Authentication' { | ||
| if ($Service -contains 'Graph' -or $Service -contains 'All') { | ||
| Write-Host "`nConnecting to Microsoft Graph" -ForegroundColor Yellow | ||
| Write-PSFMessage 'Connecting to Microsoft Graph' | ||
|
|
||
| try { | ||
| Write-PSFMessage "Connecting to Microsoft Graph with params: $($params | Out-String)" -Level Verbose | ||
| Connect-MgGraph @params -ErrorAction Stop | ||
| $contextTenantId = (Get-MgContext).TenantId | ||
| } | ||
|
|
||
| catch { | ||
| Stop-PSFFunction -Message "Failed to authenticate to Graph" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
|
|
||
| try { | ||
| Write-Verbose "Verifying Zero Trust context and permissions..." | ||
| Test-ZtContext | ||
| } | ||
| catch { | ||
| Stop-PSFFunction -Message "Authenticated to Graph, but the requirements for the ZeroTrustAssessment are not met by the established session:`n$_" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 'Az.Accounts' { | ||
| if ($SkipAzureConnection) { | ||
| continue | ||
| } | ||
|
|
||
| if ($Service -contains 'Azure' -or $Service -contains 'All') { | ||
| Write-Host "`nConnecting to Azure" -ForegroundColor Yellow | ||
| Write-PSFMessage 'Connecting to Azure' | ||
|
|
||
| $azEnvironment = 'AzureCloud' | ||
| if ($Environment -eq 'China') { | ||
| $azEnvironment = Get-AzEnvironment -Name AzureChinaCloud | ||
| } | ||
| elseif ($Environment -in 'USGov', 'USGovDoD') { | ||
| $azEnvironment = 'AzureUSGovernment' | ||
| } | ||
|
|
||
| $tenantParam = $TenantId | ||
| if (-not $tenantParam) { | ||
| $tenantVar = Get-Variable -Name 'contextTenantId' -ErrorAction SilentlyContinue | ||
| if ($tenantVar) { | ||
| $tenantParam = $tenantVar.Value | ||
| } | ||
| } | ||
|
|
||
| $azParams = @{ | ||
| UseDeviceAuthentication = $UseDeviceCode | ||
| Environment = $azEnvironment | ||
| Tenant = $tenantParam | ||
| } | ||
| if ($ClientId -and $Certificate) { | ||
| $azParams.ApplicationId = $ClientId | ||
| $azParams.CertificateThumbprint = $Certificate.Certificate.Thumbprint | ||
| } | ||
|
|
||
| try { | ||
| Connect-AzAccount @azParams -ErrorAction Stop | ||
| } | ||
| catch { | ||
| Stop-PSFFunction -Message "Failed to authenticate to Azure: $_" -ErrorRecord $_ -EnableException $true -Cmdlet $PSCmdlet | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 'ExchangeOnlineManagement' { | ||
| if ($Service -contains 'ExchangeOnline' -or $Service -contains 'All') { | ||
| Write-Verbose 'Connecting to Microsoft Exchange Online' | ||
| try { | ||
| if ($UseDeviceCode -and $PSVersionTable.PSEdition -eq 'Desktop') { | ||
| Write-Host 'The Exchange Online module in Windows PowerShell does not support device code flow authentication.' -ForegroundColor Red | ||
| Write-Host '💡Please use the Exchange Online module in PowerShell Core.' -ForegroundColor Yellow | ||
| } | ||
| elseif ($UseDeviceCode) { | ||
| Connect-ExchangeOnline -ShowBanner:$false -Device:$UseDeviceCode -ExchangeEnvironmentName $ExchangeEnvironmentName | ||
| } | ||
| else { | ||
| Connect-ExchangeOnline -ShowBanner:$false -ExchangeEnvironmentName $ExchangeEnvironmentName | ||
| } | ||
|
|
||
| # Fix for Get-Label visibility in other scopes | ||
| if (Get-Command Get-Label -ErrorAction SilentlyContinue) { | ||
| $module = Get-Command Get-Label | Select-Object -ExpandProperty Module | ||
| if ($module -and $module.Name -like 'tmp_*') { | ||
| Import-Module $module -Global -Force | ||
| } | ||
| } | ||
| } | ||
| catch { | ||
| Write-Host "`nFailed to connect to Exchange Online: $_" -ForegroundColor Red | ||
| } | ||
| } | ||
|
|
||
| if ($Service -contains 'SecurityCompliance' -or $Service -contains 'All') { | ||
| $Environments = @{ | ||
| 'O365China' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.partner.outlook.cn/powershell-liveid' | ||
| AuthZEndpointUri = 'https://login.chinacloudapi.cn/common' | ||
| } | ||
| 'O365GermanyCloud' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | ||
| } | ||
| 'O365Default' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | ||
| } | ||
| 'O365USGovGCCHigh' = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.office365.us/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.us/common' | ||
| } | ||
| 'O365USGovDoD' = @{ | ||
| ConnectionUri = 'https://l5.ps.compliance.protection.office365.us/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.us/common' | ||
| } | ||
| Default = @{ | ||
| ConnectionUri = 'https://ps.compliance.protection.outlook.com/powershell-liveid/' | ||
| AuthZEndpointUri = 'https://login.microsoftonline.com/common' | ||
| } | ||
| } | ||
| Write-Verbose 'Connecting to Microsoft Security & Compliance PowerShell' | ||
|
|
||
| if ($UseDeviceCode) { | ||
| Write-Host "`nThe Security & Compliance module does not support device code flow authentication." -ForegroundColor Red | ||
| } | ||
| else { | ||
| # Get UPN from Exchange connection or Graph context | ||
| $ExoUPN = $UserPrincipalName | ||
|
|
||
| # Attempt to resolve UPN before any connection to avoid token acquisition failures without identity | ||
| $connectionInformation = $null | ||
| try { | ||
| $connectionInformation = Get-ConnectionInformation | ||
| } | ||
| catch { | ||
| # Intentionally swallow errors here; fall back to provided UPN if any | ||
| $connectionInfoError = $_ | ||
| Write-Verbose "Get-ConnectionInformation failed; falling back to provided UserPrincipalName if available. Error: $($connectionInfoError.Exception.Message)" | ||
| } | ||
|
|
||
| if (-not $ExoUPN) { | ||
| $ExoUPN = $connectionInformation | Where-Object { $_.IsEopSession -ne $true -and $_.State -eq 'Connected' } | Select-Object -ExpandProperty UserPrincipalName -First 1 -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| if (-not $ExoUPN) { | ||
| Write-Host "`nUnable to determine a UserPrincipalName for Security & Compliance. Please supply -UserPrincipalName or connect to Exchange Online first." -ForegroundColor Yellow | ||
| continue | ||
| } | ||
|
|
||
| try { | ||
| $ippSessionParams = @{ | ||
| BypassMailboxAnchoring = $true | ||
| UserPrincipalName = $ExoUPN | ||
| ShowBanner = $false | ||
| ErrorAction = 'Stop' | ||
| } | ||
|
|
||
| # Only override endpoints for non-default clouds to reduce token acquisition failures in Default | ||
| if ($ExchangeEnvironmentName -ne 'O365Default') { | ||
| $ippSessionParams.ConnectionUri = $Environments[$ExchangeEnvironmentName].ConnectionUri | ||
| $ippSessionParams.AzureADAuthorizationEndpointUri = $Environments[$ExchangeEnvironmentName].AuthZEndpointUri | ||
| } | ||
|
|
||
| Write-Verbose "Connecting to Security & Compliance with UPN: $ExoUPN" | ||
| Connect-IPPSSession @ippSessionParams | ||
| } | ||
|
|
||
| catch { | ||
| $exception = $_ | ||
| $methodNotFoundException = $null | ||
|
|
||
| # Detect DLL conflict via a specific MissingMethodException, preferring the inner exception when present | ||
| if ($exception.Exception.InnerException -is [System.MissingMethodException]) { | ||
| $methodNotFoundException = $exception.Exception.InnerException | ||
| } | ||
| elseif ($exception.Exception -is [System.MissingMethodException]) { | ||
| $methodNotFoundException = $exception.Exception | ||
| } | ||
|
|
||
| if ($methodNotFoundException -and $methodNotFoundException.Message -like "*Microsoft.Identity.Client*") { | ||
| Write-Warning "DLL Conflict detected (Method not found in Microsoft.Identity.Client). This usually happens if Microsoft.Graph is loaded before ExchangeOnlineManagement." | ||
| Write-Warning "Please RESTART your PowerShell session and run Connect-ZtAssessment again." | ||
| } | ||
|
|
||
| Write-Host "`nFailed to connect to the Security & Compliance PowerShell: $exception" -ForegroundColor Red | ||
| } | ||
| } | ||
|
|
||
| # Fix for Get-Label visibility in other scopes | ||
| if (Get-Command Get-Label -ErrorAction SilentlyContinue) { | ||
| $module = Get-Command Get-Label | Select-Object -ExpandProperty Module | ||
| if ($module -and $module.Name -like 'tmp_*') { | ||
| Import-Module $module -Global -Force | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 'Microsoft.Online.SharePoint.PowerShell' { | ||
| if ($Service -contains 'SharePointOnline' -or $Service -contains 'All') { | ||
| try { | ||
| # Import module with compatibility if needed | ||
| if ($PSVersionTable.PSEdition -ne 'Desktop') { | ||
| # Assume module is installed in Windows PowerShell as per instructions | ||
| Import-Module Microsoft.Online.SharePoint.PowerShell -UseWindowsPowerShell -WarningAction SilentlyContinue -ErrorAction Stop -Global | ||
| } | ||
| else { | ||
| Import-Module Microsoft.Online.SharePoint.PowerShell -ErrorAction Stop -Global | ||
| } | ||
| } | ||
| catch { | ||
| # Provide clearer guidance when import fails, especially under PowerShell Core | ||
| if ($PSVersionTable.PSEdition -ne 'Desktop') { | ||
| $message = "Failed to import SharePoint Online module. When running in PowerShell Core, 'Microsoft.Online.SharePoint.PowerShell' must be installed in Windows PowerShell 5.1 (Desktop) for -UseWindowsPowerShell to work. Underlying error: $_" | ||
| } | ||
| else { | ||
| $message = "Failed to import SharePoint Online module: $_" | ||
| } | ||
| Write-Host "`n$message" -ForegroundColor Red | ||
| Write-PSFMessage $message -Level Error | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The switch statement on line 124 switches on module names from 'OrderedImport.Name', but the cases inside don't execute sequentially—they execute based on which module name matches. This means the "import order" established by Get-ModuleImportOrder isn't actually enforced for the connection sequence. The modules are connected in the order the cases are written in the switch, not the order returned by Get-ModuleImportOrder. If the intention is to connect services in a specific DLL-version order, the switch should be replaced with a foreach loop that iterates through OrderedImport in sequence.
| $contextTenantIdValue = Get-Variable -Name contextTenantId -ErrorAction SilentlyContinue -ValueOnly | ||
| if ($contextTenantIdValue) { | ||
| try { | ||
| $org = Invoke-ZtGraphRequest -RelativeUri 'organization' | ||
| $initialDomain = $org.verifiedDomains | Where-Object { $_.isInitial } | Select-Object -ExpandProperty name -First 1 | ||
| if ($initialDomain) { | ||
| $tenantName = $initialDomain.Split('.')[0] | ||
| $adminUrl = "https://$tenantName-admin.sharepoint.com" | ||
| Write-Verbose "Inferred SharePoint Admin URL: $adminUrl" | ||
| } | ||
| } | ||
| catch { | ||
| Write-Verbose "Failed to infer SharePoint Admin URL from Graph: $_" | ||
| } |
There was a problem hiding this comment.
The variable 'contextTenantIdValue' is retrieved on line 362 but is only used to check for its existence. Later on lines 365-370, the code attempts to use Graph API (Invoke-ZtGraphRequest) without verifying that the Graph connection was actually established. If Graph is not in the Service array, this will fail. Add a check to ensure Graph is connected before attempting to use Invoke-ZtGraphRequest.
| catch { | ||
| Write-Host "=== Initialize-Dependencies.ps1 FAILED ===" -ForegroundColor Red | ||
| Write-Host "Error: $_" -ForegroundColor Red | ||
| Write-Host $_.ScriptStackTrace -ForegroundColor Red | ||
| # Don't throw - let RequiredModules try to proceed | ||
| } |
There was a problem hiding this comment.
The catch block on line 82 silently continues after a failure in Initialize-Dependencies.ps1 with only console output (Write-Host). This means errors won't be logged through the PSFramework logging system that the rest of the module uses. Additionally, the comment 'Don't throw - let RequiredModules try to proceed' suggests intentional failure tolerance, but this may hide critical initialization failures. Consider using Write-PSFMessage for consistency with the module's logging approach and clearly documenting the decision to allow failures.
| # Define dependencies and their required versions | ||
| $dependencies = @('Az.Accounts', 'ExchangeOnlineManagement', 'Microsoft.Graph.Authentication', 'Microsoft.Graph.Beta.Teams') | ||
| $requiredVersions = @{ | ||
| 'ExchangeOnlineManagement' = '3.8.0' | ||
| } | ||
|
|
There was a problem hiding this comment.
The variables $dependencies and $requiredVersions are defined but never used in the Initialize-Dependencies.ps1 script. The $dependencies variable is defined with multiple module names, but only 'ExchangeOnlineManagement' is actually processed. Either remove these unused definitions or implement the intended logic to process all dependencies.
| # Define dependencies and their required versions | |
| $dependencies = @('Az.Accounts', 'ExchangeOnlineManagement', 'Microsoft.Graph.Authentication', 'Microsoft.Graph.Beta.Teams') | |
| $requiredVersions = @{ | |
| 'ExchangeOnlineManagement' = '3.8.0' | |
| } | |
| # Define required versions for dependencies | |
| $requiredVersions = @{ | |
| 'ExchangeOnlineManagement' = '3.8.0' | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| Write-PSFMessage "Connecting to Microsoft Graph with params: $($params | Out-String)" -Level Verbose | ||
| Connect-MgGraph @params -ErrorAction Stop | ||
| $contextTenantId = (Get-MgContext).TenantId |
There was a problem hiding this comment.
The assignment of $contextTenantId happens inside a conditional block (lines 126-147) that only executes if 'Graph' or 'All' is in the Service array. However, this variable is used later at line 362 and line 169-172 without checking if it was actually set. If a user connects only to Azure or ExchangeOnline without Graph, $contextTenantId will be undefined, causing potential errors when referenced. Consider setting $contextTenantId to $null before the switch statement or adding proper null checks before using it.
| Write-Warning "Please RESTART your PowerShell session and run Connect-ZtAssessment again." | ||
| } | ||
|
|
||
| Write-Host "`nFailed to connect to the Security & Compliance PowerShell: $exception" -ForegroundColor Red |
There was a problem hiding this comment.
The error message includes the raw exception object using string interpolation. This could result in verbose and unhelpful output. Consider using $exception.Exception.Message or providing a more user-friendly error message that explains what went wrong and how to resolve it.
| Write-Host "`nFailed to connect to the Security & Compliance PowerShell: $exception" -ForegroundColor Red | |
| Write-Host "`nFailed to connect to the Security & Compliance PowerShell: $($exception.Exception.Message)" -ForegroundColor Red |
| $connectionInfoError = $_ | ||
| Write-Verbose "Get-ConnectionInformation failed; falling back to provided UserPrincipalName if available. Error: $($connectionInfoError.Exception.Message)" |
There was a problem hiding this comment.
The variable $connectionInfoError is assigned at line 265 but never used afterwards. This creates dead code that adds no value. Either remove the variable assignment and just use $_ in the Write-Verbose message, or remove the variable entirely if it's not needed.
| $connectionInfoError = $_ | |
| Write-Verbose "Get-ConnectionInformation failed; falling back to provided UserPrincipalName if available. Error: $($connectionInfoError.Exception.Message)" | |
| Write-Verbose "Get-ConnectionInformation failed; falling back to provided UserPrincipalName if available. Error: $($_.Exception.Message)" |
| [Parameter( | ||
| Position = 0, | ||
| ValueFromPipelineByPropertyName, | ||
| HelpMessage = 'Enter a list of names to evaluate. Wildcards are allowed.' |
There was a problem hiding this comment.
The parameter help message says 'Enter a list of names to evaluate. Wildcards are allowed.' but the code doesn't actually process wildcards - it passes the names directly to Get-Module which does support wildcards, but there's no explicit wildcard expansion or validation in this function. The help message should clarify that wildcard support is provided by the underlying Get-Module cmdlet, or the function should explicitly handle and document wildcard behavior.
| $tenantParam = $TenantId | ||
| if (-not $tenantParam) { | ||
| $tenantVar = Get-Variable -Name 'contextTenantId' -ErrorAction SilentlyContinue | ||
| if ($tenantVar) { | ||
| $tenantParam = $tenantVar.Value | ||
| } | ||
| } | ||
|
|
||
| $azParams = @{ | ||
| UseDeviceAuthentication = $UseDeviceCode | ||
| Environment = $azEnvironment | ||
| Tenant = $tenantParam |
There was a problem hiding this comment.
The variable $tenantParam is used to construct $azParams but the logic for obtaining it from $contextTenantId may fail silently. If $TenantId is not provided and $contextTenantId variable doesn't exist (because Graph connection was skipped), $tenantParam will be $null. Then Connect-AzAccount will be called with Tenant = $null, which might cause unexpected behavior. Consider validating that $tenantParam has a value before calling Connect-AzAccount, or provide a more helpful error message if neither TenantId parameter nor contextTenantId variable is available.
| foreach ($test in $syncTests) { | ||
| Invoke-ZtTest -Test $test -Database $Database | ||
| } |
There was a problem hiding this comment.
If an exception occurs during the sync tests execution (lines 72-74), the parallel tests will never run and the error won't be explicitly handled. Consider wrapping the sync tests in a try-catch block to handle errors gracefully and ensure parallel tests can still execute even if some sync tests fail.
| $ippSessionParams = @{ | ||
| BypassMailboxAnchoring = $true | ||
| UserPrincipalName = $ExoUPN | ||
| ShowBanner = $false | ||
| ErrorAction = 'Stop' | ||
| } |
There was a problem hiding this comment.
The variable name 'ippSessionParams' uses inconsistent casing (lowercase 'ipp'). PowerShell convention typically uses PascalCase for variable names in this context. Consider renaming to 'IppSessionParams' or 'IPPSSessionParams' for consistency with other variables in this function like '$azParams' and '$params'.
| ) | ||
|
|
||
| begin { | ||
| $ModulesWithVersionSortedIdentityClient = [System.Collections.Generic.List[PSCustomobject]]::new() |
There was a problem hiding this comment.
The code uses [System.Collections.Generic.List[PSCustomobject]] but 'PSCustomobject' uses incorrect casing. The correct type name in PowerShell is 'PSCustomObject' with a capital 'O'. While PowerShell may be case-insensitive in many contexts, using the proper casing improves code clarity and follows best practices.
| $ModulesWithVersionSortedIdentityClient = [System.Collections.Generic.List[PSCustomobject]]::new() | |
| $ModulesWithVersionSortedIdentityClient = [System.Collections.Generic.List[PSCustomObject]]::new() |
| try { | ||
| [System.Reflection.Assembly]::LoadFrom($_.FullName) | Out-Null | ||
| } | ||
| catch { |
There was a problem hiding this comment.
The empty catch block at lines 48-50 silently swallows all exceptions without any logging or handling. While the comment indicates this is intentional, completely ignoring errors can hide bugs and make debugging difficult. Consider at minimum adding a Write-Verbose or Write-Debug statement to log that an error occurred during the DLL loading attempt.
| try { | |
| [System.Reflection.Assembly]::LoadFrom($_.FullName) | Out-Null | |
| } | |
| catch { | |
| $dependentAssemblyPath = $_.FullName | |
| try { | |
| [System.Reflection.Assembly]::LoadFrom($dependentAssemblyPath) | Out-Null | |
| } | |
| catch { | |
| Write-Verbose "Failed to load dependent MSAL assembly '$dependentAssemblyPath': $($_.Exception.Message)" |
| # Load Main DLL | ||
| [System.Reflection.Assembly]::LoadFrom($exoModule.DLLPath) | Out-Null | ||
|
|
||
| # Load related DLLs (Brokers, etc.) |
There was a problem hiding this comment.
The comment says 'Brokers, etc.' but the code specifically filters for files matching 'Microsoft.Identity.Client*.dll' excluding the main 'Microsoft.Identity.Client.dll'. This is more specific than the comment implies. Consider updating the comment to be more precise about what additional DLLs are being loaded, for example: 'Load related MSAL DLLs (e.g., Microsoft.Identity.Client.Broker.dll, Microsoft.Identity.Client.Extensions.dll)'.
| # Load related DLLs (Brokers, etc.) | |
| # Load related MSAL DLLs (e.g., Microsoft.Identity.Client.Broker.dll, Microsoft.Identity.Client.Extensions.dll) |
RequiredModulesin module manifest to includeExchangeOnlineManagementand adjust versions.Invoke-ZtTeststo separate sync and parallel tests for improved execution.Connect-ZtAssessmentto support multiple services and environments, including SharePoint Online.Initialize-Dependencies.ps1to manage DLL loading order and prevent conflicts.Get-ModuleImportOrderfunction to evaluate module import order based on DLL versions.