microsoft / microsoft/CSS-Exchange

Standardize Exchange Online / Graph connection logic in a shared helper (adopt across all EXO-connecting scripts)

Open
#2,590 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Enhancement Public Folders Shared Function Triage Work - Large Change
Dominant language
PowerShell
Stars
1.3k
Forks
395
Avg merge
14h 7m
Merged PRs (30d)
5

Description

The problem

Every CSS-Exchange script that talks to Exchange Online currently rolls its own Connect-ExchangeOnline block. They all have subtle variations of the same bugs, and every new script we add is another chance to reintroduce them.

PR #2589 recently fixed a real customer-impacting version of this in Sync-ModernMailPublicFolders.ps1. But when we surveyed the rest of the repo we found the identical broken pattern still lives in 8 of the 9 root scripts under PublicFolders/, plus more elsewhere. Fixing them one PR at a time is not scalable. We already have a shared helper in flight (PR #2090, open since May 2024) that solves this for the whole repo. This issue is about finishing #2090 with the lessons from #2589 folded in, and then adopting it everywhere.

🚨 Sovereign / national cloud support is currently broken across the board

None of the current EXO-connect code paths β€” and neither of the shipped versions of the shared helper in PR #2090 β€” accept -AzureADAuthorizationEndpointUri.

That means GCC High, DoD, 21Vianet (China), and Germany customers cannot make these scripts connect correctly. Connect-ExchangeOnline needs a matching AAD authorization endpoint alongside the sovereign ConnectionUri, and there is nowhere to plumb it. The commercial endpoint is used regardless, so the connection either fails outright or authenticates the wrong tenant.

PR #2589 added this parameter to Sync-ModernMailPublicFolders.ps1 only. Every other EXO-connecting script in the repo β€” plus the proposed shared helper β€” is still broken for sovereign clouds. This must be a first-class parameter on Connect-EXOAdvanced before #2090 is merged.

Blast radius β€” PublicFolders alone

Every one of these is a root/entry-point script (not dot-sourced by anything else β€” it ships as its own dist/*.ps1):

# Root script Status
1 MailPublicFolderSync/Sync-ModernMailPublicFolders.ps1 βœ… Fixed by PR #2589
2 MailPublicFolderSync/Sync-MailPublicFolders.ps1 ❌ Buggy
3 MailPublicFolderSync/Sync-MailPublicFoldersCloudToOnprem.ps1 ❌ Buggy
4 MailPublicFolderSync/Import-MailPublicFolders.ps1 ❌ Buggy + unguarded Disconnect (see below)
5 MailPublicFolderSync/Import-PublicFolderMailboxes.ps1 ❌ Buggy
6 Migration/ToMicrosoft365Groups/AddMembersToGroups.ps1 ❌ Buggy
7 Migration/ToMicrosoft365Groups/LockAndSavePublicFolderProperties.ps1 ❌ Buggy
8 Migration/ToMicrosoft365Groups/UnlockAndRestorePublicFolderProperties.ps1 ❌ Buggy
9 ValidateEXOPFDumpster.ps1 🟑 Partial (uses -ErrorAction Stop on Connect, but no module import check, no sovereign-cloud params)

1 out of 9 fixed. Plus Admin/CrossTenantMailboxMigrationValidation.ps1 has a different but equally broken pattern (Connect-ExchangeOnline -Prefix Source -ShowBanner:$false with no error action, no import check).

What the broken pattern looks like

Straight from Sync-MailPublicFolders.ps1#L169-L193:

try {
    Import-Module ExchangeOnlineManagement -ErrorAction SilentlyContinue
    if (Get-Module ExchangeOnlineManagement) {
        $connectParams = @{
            ConnectionUri = $ConnectionUri
            Prefix        = "Remote"
            ErrorAction   = "SilentlyContinue"       # πŸ› (2)
        }
        if ($null -ne $Credential) { $connectParams.Credential = $Credential }
        Connect-ExchangeOnline @connectParams
        $script:isConnectedToExchangeOnline = $true  # πŸ› (3) set even if Connect failed
    } else {
        Write-Warning $LocalizedStrings.EXOV2ModuleNotInstalled  # πŸ› (1)
        exit
    }
} finally { ... }

Three defects, all of them silent:

  1. Import-Module -ErrorAction SilentlyContinue β€” if the module is present but fails to load (assembly conflict, older V2 vs V3, Constrained Language Mode, AppLocker), the error is swallowed. Get-Module returns nothing and the script exits with only a warning. Users see "why did my sync do nothing?"
  2. Connect-ExchangeOnline with ErrorAction = "SilentlyContinue" β€” auth failures (expired creds, MFA canceled, wrong ConnectionUri for a sovereign cloud, conditional access) are non-terminating. Execution falls through.
  3. $isConnectedToExchangeOnline = $true unconditionally β€” set on the next line even if Connect failed silently. The script now proceeds with a phantom session. For MailPublicFolder sync this can lead to destructive delete operations against the cloud side.
Additional finding β€” unguarded Disconnect in Import-MailPublicFolders.ps1

Import-MailPublicFolders.ps1:244 and line 257 call Disconnect-ExchangeOnline -Confirm:$false without checking $isConnectedToExchangeOnline. If the script exits early before connecting, this tears down whatever unrelated EXO session the user already had open in their console. The shared helper should own connect and disconnect symmetry so this class of bug goes away.

What "good" looks like

PR #2589 fixed one script β€” see Sync-ModernMailPublicFolders.ps1#L256-L285:

$script:isConnectedToExchangeOnline = $false          # βœ… pre-clear
try {
    Import-Module -Name ExchangeOnlineManagement -ErrorAction Stop   # βœ… (1)
    $connectParams = @{
        ConnectionUri = $ConnectionUri
        Prefix        = "Remote"
        ErrorAction   = "Stop"                        # βœ… (2)
    }
    if ($null -ne $Credential) { $connectParams.Credential = $Credential }
    if (-not [string]::IsNullOrEmpty($AzureADAuthorizationEndpointUri)) {
        $connectParams.AzureADAuthorizationEndpointUri = $AzureADAuthorizationEndpointUri   # βœ… sovereign cloud
    }
    Connect-ExchangeOnline @connectParams
    $script:isConnectedToExchangeOnline = $true       # βœ… (3) only on success
} finally { ... }

That is the exact shape the shared helper needs. We shouldn't have to write it eight more times.

The shared helper β€” PR #2090

PR #2090 already introduces:

  • Shared/M365/EXOConnection.ps1 β€” Connect-EXOAdvanced
  • Shared/M365/GraphConnection.ps1 β€” Connect-GraphAdvanced
  • Shared/ModuleHandle.ps1 β€” Request-Module (installs on demand, CurrentUser or AllUsers)

It already handles the important stuff:

  • Get-ConnectionInformation (V3) to detect and reuse an existing session
  • Connect-ExchangeOnline -ErrorAction Stop -ShowBanner:$false
  • Auto-install missing module, minimum-version enforcement, SupportsShouldProcess
  • Returns $null on failure and prints tenant / UPN / prefix on success

It has been open for ~2 years with zero adoption. We need to get it landed.

Gaps to close before merging #2090 (lessons from #2589 + survey of callers)

Connect-EXOAdvanced
  • 🚨 Add -AzureADAuthorizationEndpointUri β€” sovereign clouds are completely unsupported without it. Non-negotiable for GCC High / DoD / 21Vianet / Germany.
  • Add -ConnectionUri β€” required by every MailPublicFolderSync script and by 21Vianet
  • Add -Credential β€” required by non-interactive MailPublicFolderSync scenarios
  • Fix the -Prefix parameter set binding. Today it's bound only to the AllowMultipleSessions set:
    powershell [Parameter(Mandatory = $false, ParameterSetName = 'AllowMultipleSessions')] [string]$Prefix = $null,
    That means a caller who wants a single session with a prefix β€” the exact pattern every MailPublicFolderSync script uses (-Prefix "Remote") β€” can't specify one without also asserting -AllowMultipleSessions. -Prefix should be available in the SingleSession set too.
  • Prefix default should be '' not $null, and Connect should only splat -Prefix when non-empty (avoids passing -Prefix '' to Connect-ExchangeOnline on every default call).
  • Session reuse must consider prefix + tenant + UPN, not just ModulePrefix. Today a stray unrelated session with an empty prefix will be silently reused. Mirror the tenant check that already exists in Connect-GraphAdvanced.
  • Add a Disconnect-EXOAdvanced companion (net-new scope). PR #2090 was designed to never disconnect ("Important: we do not disconnect at anytime"). That leaves every caller writing its own Disconnect-ExchangeOnline block, which is exactly how Import-MailPublicFolders.ps1 ended up tearing down unrelated user sessions. The helper needs to own the connect/disconnect pair, guarded by its own state, so callers only ever disconnect sessions the helper opened.
  • Fallback to Get-Module -ListAvailable when Get-InstalledModule returns nothing (side-loaded / MSI-installed / pre-provisioned modules aren't visible to Get-InstalledModule).
  • Document unattended usage β€” ConfirmImpact = "High" will block automation unless the caller passes -Confirm:$false. Say so in the function help.
  • Document the return contract β€” $null on failure vs throw. Callers need one shape to code against.
Request-Module (Shared/ModuleHandle.ps1)
  • Add -MaximumVersion β€” some scripts need to pin around a known-bad module release
  • Reconsider Install-Module -AllowClobber β€” silently overrides other modules' cmdlets
  • Same Get-Module -ListAvailable fallback
Connect-GraphAdvanced
  • Add -Environment (Global / USGov / USGovDoD / China) for parity with the EXO sovereign-cloud story
  • Add Pester tests covering scope mismatch and tenant mismatch reconnect paths

How a junior dev will use it (target end state)

Before (current β€” every script owns this):

function InitializeExchangeOnlineRemoteSession {
    # 30 lines of Import-Module / Get-Module / Connect-ExchangeOnline / state flags
    # ... any of which can be wrong in a subtle way
}

After (target):

. $PSScriptRoot\..\..\Shared\M365\EXOConnection.ps1

$exo = Connect-EXOAdvanced -ConnectionUri $ConnectionUri `
                           -AzureADAuthorizationEndpointUri $AzureADAuthorizationEndpointUri `
                           -Credential $Credential `
                           -Prefix 'Remote'
if (-not $exo) { return }   # helper already printed the reason
try {
    # ...do EXO work...
} finally {
    Disconnect-EXOAdvanced -Connection $exo   # only disconnects what we opened
}

Or the CrossTenant Source/Target case:

. $PSScriptRoot\..\Shared\M365\EXOConnection.ps1

$source = Connect-EXOAdvanced -AllowMultipleSessions -Prefix Source
$target = Connect-EXOAdvanced -AllowMultipleSessions -Prefix Target
if (-not $source -or -not $target) { return }

That's the whole contract. No $script:isConnectedToExchangeOnline, no -ErrorAction SilentlyContinue, no manual Get-Module check, no sovereign-cloud one-off, no unguarded Disconnect-ExchangeOnline.

Sub-issues

  • #2591 β€” Finish shared helper PR #2090 (with sovereign-cloud, Prefix, Disconnect, tests)
  • #2592 β€” Adopt shared helper across PublicFolders/MailPublicFolderSync/*
  • #2593 β€” Adopt shared helper across PublicFolders/Migration/ToMicrosoft365Groups/*
  • #2594 β€” Adopt shared helper in remaining EXO callers (ValidateEXOPFDumpster.ps1, Admin/CrossTenantMailboxMigrationValidation.ps1)

Adoption plan (follow-up PRs β€” not part of #2090)

Once #2090 is merged with the gaps above closed:

  • Refactor Sync-ModernMailPublicFolders.ps1 β€” remove the connect block PR #2589 just added; call Connect-EXOAdvanced
  • MailPublicFolderSync/Sync-MailPublicFolders.ps1
  • MailPublicFolderSync/Sync-MailPublicFoldersCloudToOnprem.ps1
  • MailPublicFolderSync/Import-MailPublicFolders.ps1 (also fixes the unguarded Disconnect)
  • MailPublicFolderSync/Import-PublicFolderMailboxes.ps1
  • Migration/ToMicrosoft365Groups/AddMembersToGroups.ps1
  • Migration/ToMicrosoft365Groups/LockAndSavePublicFolderProperties.ps1
  • Migration/ToMicrosoft365Groups/UnlockAndRestorePublicFolderProperties.ps1
  • PublicFolders/ValidateEXOPFDumpster.ps1
  • Admin/CrossTenantMailboxMigrationValidation.ps1 (great -AllowMultipleSessions test case)
  • Add a rule to .github/copilot-instructions.md and the code-review agent: new EXO-touching scripts must use Connect-EXOAdvanced. Do not open-code Connect-ExchangeOnline.

References

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up β€” it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Begin with Shared/M365/EXOConnection.ps1 and Connect-EXOAdvanced, comparing its contract with Shared/M365/GraphConnection.ps1 and the connection block in PublicFolders/MailPublicFolderSync/Sync-ModernMailPublicFolders.ps1. Trace the listed PublicFolders callers and Admin/CrossTenantMailboxMigrationValidation.ps1, using PRs #2090 and #2589 as references. Done means the helper covers the listed connection and disconnect requirements, callers adopt it, and unattended and sovereign-cloud usage is documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
powershell
Domain
cloud, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.