Skip to content

Give the WAM broker a parent window handle so Entra MFA works (#2184) - #2217

Merged
erikdarlingdata merged 2 commits into
devfrom
feature/2184-entra-wam-window-handle
Aug 12, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
feature/2184-entra-wam-window-handle

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes #2184.

Microsoft Entra MFA could not work in Lite at all. Current Microsoft.Data.SqlClient (7.0.2 here) routes Authentication=ActiveDirectoryInteractive through the Windows WAM broker, and WAM requires the application to supply the HWND that will own its account picker. ServerConnection.ApplyAuthentication set the authentication mode but nothing registered an auth provider, so users got 0xwindow_handle_required instead of a prompt. Not tenant-specific - broken for every user on a current build; old builds predate WAM being the default and fell back to a browser.

This is the direct port of the Studio fix (PerformanceStudio#426), which @joshdbe - this issue's reporter - verified yesterday against his real Entra-MFA-on-Azure-VM environment. Same three decisions, one WPF-specific difference:

  • Registered once at startup, process-wide. SqlAuthenticationProvider.SetProvider installs against the authentication method, so one call in OnStartup covers the Add/Edit dialog's Test Connection and every collector loop without per-site wiring anyone could forget.
  • Handle resolved per prompt, preferring the active window - the picker should land in front of the Add/Edit dialog, not behind it on the main window. Falls back to main window, then IntPtr.Zero, which MSAL treats as no handle: auth fails with its normal message rather than the app crashing.
  • Marshaled to the UI thread - and in WPF this is load-bearing, not defensive. MSAL invokes the handle callback from whatever thread the connection open runs on (collector worker threads included), and WPF throws on off-thread Window property reads where Avalonia merely races. The blocking Dispatcher.Invoke cannot deadlock: no UI-thread path in Lite blocks on a SQL connection open (all opens are async; the .Result reads in ServerTab partials are post-WhenAll completed-task reads).

No Lite analog of Studio's CLI refusal exists - Lite has no headless entry point that accepts Entra interactive - and no OS gate is needed on a net10.0-windows WPF app.

How was this tested?

  • Full Lite.Tests suite: 2,197 passed, 0 failed locally on Windows, including two new tests pinning the wiring contracts (null provider rejected at wiring time; registration is idempotent so a later caller can't silently replace the provider, made order-deterministic via an internal test-only reset).
  • The behavioral half is inherited from Studio's verification: same SqlClient version, same seam, same provider registration, tested by the reporter against a real tenant - including the silent-SSO case (Windows session already satisfies MFA, broker succeeds with no picker; that's WAM working, not the prompt failing to appear). The WPF handle source (WindowInteropHelper) is the documented pattern for exactly this registration.
  • CHANGELOG entry added under Unreleased > Fixed. A first draft of this branch accidentally flipped CHANGELOG.md's line endings wholesale (the file is CRLF; a 2,690-line phantom diff) - caught and rebuilt against the original bytes before this PR, so the diff is the real 3 lines.

🤖 Generated with Claude Code

Current Microsoft.Data.SqlClient (7.0.2) routes ActiveDirectoryInteractive
through the Windows WAM broker, which requires the application to supply
the HWND that will own its account picker. Lite set the authentication
mode but never registered an auth provider, so every Entra MFA connection
failed with 0xwindow_handle_required instead of prompting.

Register a process-wide provider at startup (covers the Add/Edit dialog's
Test Connection and every collector loop), resolving the owning window per
prompt - the active window first, so the picker lands in front of the
dialog rather than behind it - marshaled through the dispatcher because
MSAL calls from whatever thread the connection open runs on, and WPF
throws on off-thread Window property reads. The blocking Invoke is safe:
no UI-thread path blocks on a SQL connection open.

Direct port of the Studio fix (PerformanceStudio#426), verified there
against a real Entra-MFA tenant by this issue's reporter, including the
silent-SSO case (broker satisfies MFA from the Windows session and no
picker appears - that is WAM working).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread Lite/App.xaml.cs Outdated
Comment on lines +496 to +508
private static IntPtr ActiveWindowHandle()
{
try
{
var dispatcher = Current?.Dispatcher;
if (dispatcher is null)
return IntPtr.Zero;

return dispatcher.CheckAccess()
? ActiveWindowHandleOnUIThread()
: dispatcher.Invoke(ActiveWindowHandleOnUIThread);
}
catch

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ActiveWindowHandle() swallows every exception with no logging before falling back to IntPtr.Zero. Every other catch block in this file (webhook/SMTP credential lookups, DataRootMigration, the unhandled-exception handlers, etc.) logs via AppLogger before degrading — this is the one exception that degrades silently.

That matters more than usual here: IntPtr.Zero is exactly the input that reproduces the original symptom this PR fixes (0xwindow_handle_required). If ActiveWindowHandleOnUIThread() ever throws for a real reason (a future WPF/dispatcher change, app.Windows mutating mid-enumeration, etc.), the user is back to a bare MSAL failure with zero trace of why the handle resolution failed — the same silent-failure shape as #2184 itself, just one layer down.

Consider at least an AppLogger.Warn("App", ...) in the catch before returning IntPtr.Zero, so a future regression here is diagnosable instead of silent.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ec03916 — the catch now logs through AppLogger.Warn before degrading to Zero, guarded like the file's other late-lifecycle log calls since handle resolution can fire during shutdown after the logger is gone. The irony was the convincing part: a silent fallback here would be #2184's own failure shape one layer down.

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Reviewed the diff (CHANGELOG.md, Lite/Services/EntraInteractiveAuth.cs, Lite/App.xaml.cs, Lite.Tests/EntraInteractiveAuthTests.cs).

Parity check (Lite vs Darling): No drift. Confirmed Darling has no analog to flag — Darling/PerformanceMonitor.Darling.Viewer explicitly refuses EntraMFA/ServicePrincipal/ManagedIdentity for its own connect path (TryResolveCredential returns ServerStoreCredential.UnsupportedAuthMessage) because Darling's actual collection runs through the Windows service under Windows/SQL auth, not an interactive desktop session — it never reaches the WAM broker this PR is fixing for Lite. So this being Lite-only is correct, not an oversight.

Deadlock claim, verified: The PR body asserts no UI-thread path in Lite blocks on a SQL connection open, which is what makes the blocking dispatcher.Invoke in ActiveWindowHandle() safe. I checked this rather than taking it on faith — every .Result/.Wait() in Lite/Controls/*.cs is a post-await/post-Task.WhenAll read on an already-completed task, RunConnectionTestAsync uses await connection.OpenAsync() throughout, and there are zero synchronous .Open() calls anywhere in Lite/. Claim holds against current code.

Auth wiring: ApplyAuthentication only ever sets SqlAuthenticationMethod.ActiveDirectoryInteractive for EntraMFA (ServicePrincipal/ManagedIdentity use non-interactive methods), so registering the provider for just that one method is complete — no missed auth mode.

One finding posted inline: the catch-all in ActiveWindowHandle() (Lite/App.xaml.cs) swallows exceptions with no logging, unlike every other catch block in that file, which is a bit ironic given the bug this PR fixes was itself a silent, unexplained failure.

No SQL changed in this PR, so the T-SQL style guide doesn't apply here.

… silently

Zero is exactly the input that reproduces 0xwindow_handle_required, so a
throw inside ActiveWindowHandle must leave a trace in the log - a silent
fallback would be the #2184 bug's own shape one layer down, as the review
put it. Guarded like the file's other late-lifecycle log calls, because
handle resolution can fire during shutdown after the logger is gone.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Reviewed. This is a clean, well-scoped fix — no blocking issues found.

What I checked closely:

  • Lite/Darling parity: Darling's AddServerDialog.TryResolveCredential already rejects EntraMFA outright (/* EntraMFA / ServicePrincipal / ManagedIdentity — no Darling service connect path. */, Darling/PerformanceMonitor.Darling.Viewer/AddServerDialog.xaml.cs:311) — that's pre-existing scope, not something this PR touches or should touch. MonitoredServerConnection.BuildConnectionString (Darling's collector-side connection builder) only supports SQL auth or integrated security, with no Authentication= setting at all. So there's no parity gap here; Darling structurally doesn't have this bug to fix.
  • The deadlock claim: the PR body asserts the blocking Dispatcher.Invoke in ActiveWindowHandle can't deadlock because no UI-thread path blocks synchronously on a SQL connection open. I traced this: AddServerDialog.TestButton_Click → RunConnectionTestAsync uses await connection.OpenAsync() (not .Result/.Wait()), and the .Result usages elsewhere (ServerTab.Refresh.cs etc.) are all post-Task.WhenAll reads on already-completed tasks, not live blocking waits. RemoteCollectorService.cs also references MfaAuthenticationHelper.IsMfaCancelledException, confirming collector background threads are a real caller of this auth path (which is exactly why the cross-thread marshaling matters) — and those run off the UI thread entirely, so Invoke there is genuinely safe. Claim checks out.
  • API usage: ActiveDirectoryAuthenticationProvider.SetParentActivityOrWindowFunc(Func<object>) is the documented SqlClient WAM hookup for desktop/mobile apps — no new package reference needed, matches the Microsoft.Data.SqlClient 7.0.2 already referenced.
  • Thread-safety of the one-way _registered flag, the InternalsVisibleTo("Lite.Tests") wiring for ResetRegistrationForTests, and the CHANGELOG entry/link placement all check out.

No security concerns (no new input surface — this only threads a window handle through, not user data), no SQL touched, no correctness bugs found.

@erikdarlingdata
erikdarlingdata merged commit caca641 into dev Aug 12, 2026
5 checks passed
@erikdarlingdata
erikdarlingdata deleted the feature/2184-entra-wam-window-handle branch August 12, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant