Give the WAM broker a parent window handle so Entra MFA works (#425) - #426
Conversation
Interactive Microsoft Entra MFA could not work in Studio on Windows 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.BuildConnectionString set the authentication mode but nothing in the repo ever registered an auth provider, so instead of a prompt users got "0xwindow_handle_required / A window handle must be configured" and the connection failed. Not tenant-specific: broken for every Windows user on a current build. Studio 1.4.3 worked because it predates WAM being the default and fell back to a browser. Reported by joshdbe, who supplied the diagnostic that made it findable: 1.4.3 works, 1.19.1 does not. Same defect in PerformanceMonitor Lite (erikdarlingdata/PerformanceMonitor#2184); doing Studio first because that is the tool the reporter actually uses. EntraInteractiveAuth.Register takes a handle provider and installs an ActiveDirectoryAuthenticationProvider. Three deliberate choices: - Registered ONCE at app startup rather than per connection. SqlAuthenticationProvider installs against the authentication METHOD, not a connection, so one call covers every SqlConnection the process opens. Studio opens them from several unrelated places (connection dialog, query session control, schema service) and threading a handle through all of them is a change every future call site could forget. - WINDOWS ONLY. WAM does not exist on macOS or Linux, where interactive auth already works through the system browser and needs no handle, so registering a handle-supplying provider there would add a failure mode to the platforms Avalonia exists to serve. - The handle is resolved PER PROMPT, not captured at startup, and prefers the active window over the main window. A platform handle is not valid until its window is sourced, and a connection is usually triggered from the connection dialog - parenting the picker to the main window behind it would let the picker appear behind the dialog the user is looking at. The CLI gets a clear refusal instead of MSAL's error. planview accepts --auth entra and has no window to give, so it would have failed deep inside MSAL with a message that tells the operator nothing. It now says so up front and names the modes that work headless. Deliberately NOT silently substituting another auth mode: connecting as a different identity than the one asked for is worse than refusing. Whether the CLI should grow device-code flow is the open question on #425 rather than something to guess at here. Tests cover what is verifiable without a tenant: null handle provider rejected at wiring time rather than at prompt time, the non-Windows contract (Register declines, the provider is never consulted, and IsSupported stays TRUE so the CLI guard does not refuse entra on platforms where it works), and idempotent registration, since SetProvider is process-wide and a second call would otherwise silently replace the first once the SSMS extension wires in too. Whether the picker actually authenticates can only be established against a real Entra tenant on an Azure VM; the reporter has offered to test that and this is not claimed as verified until he does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c306841 to
956b778
Compare
|
Split the test-hang watchdog out of this branch into #427. It was unrelated infrastructure sitting on a feature PR, and more importantly it coupled a low-risk fix that protects everyone now to a fix that's blocked on tenant verification — this one shouldn't hold that one up. This branch is back to a single commit; verified the watchdog files and the workflow flags are absent from it. |
| private static IntPtr ActiveWindowHandle() | ||
| { | ||
| if (Current?.ApplicationLifetime is not IClassicDesktopStyleApplicationLifetime desktop) | ||
| return IntPtr.Zero; | ||
|
|
||
| var window = desktop.Windows.FirstOrDefault(w => w.IsActive) ?? desktop.MainWindow; | ||
|
|
||
| return window?.TryGetPlatformHandle()?.Handle ?? IntPtr.Zero; | ||
| } |
There was a problem hiding this comment.
ActiveWindowHandle isn't marshaled to the UI thread, but MSAL will very likely call it from one that isn't UI. The call chain is conn.OpenAsync() awaited from an async void handler (e.g. QuerySessionControl.Connection.cs, ConnectionDialog.axaml.cs) → SqlAuthenticationProvider → MSAL's interactive/WAM flow, none of which guarantee a UI-thread continuation; SqlClient auth providers are commonly invoked off thread-pool continuations to avoid sync-over-async deadlocks. If that happens here, desktop.Windows.FirstOrDefault(...) is enumerating an Avalonia-managed collection that the UI thread can mutate concurrently (e.g. the connection dialog itself opening/closing right around the moment auth kicks in), which risks InvalidOperationException: Collection was modified or just flaky results.
This codebase already has the fix pattern in hand elsewhere (Dispatcher.UIThread is used in several PlanViewer.App files for exactly this reason). Worth wrapping the body in Dispatcher.UIThread.Invoke(...) (or checking CheckAccess()) here too, given the PR itself notes end-to-end auth against a real tenant hasn't been verified yet.
There was a problem hiding this comment.
Fixed in 28be5ed. The lookup now runs through Dispatcher.UIThread (CheckAccess fast path, blocking Invoke otherwise), catching to IntPtr.Zero so a dispatcher failure degrades to MSAL's normal no-handle error instead of throwing from inside the auth callback. Checked the deadlock precondition before choosing the blocking Invoke: no connection path blocks the UI thread on auth — the only sync .Open() calls in the repo are Avalonia geometry contexts, and there's no sync-over-async on any connection path — so the UI thread is always free to service it.
| one the operator asked for is worse than refusing. Whether the CLI should grow device-code flow is | ||
| the open question on #425, not something to guess at from here. */ | ||
| if (authType == AuthenticationTypes.EntraMFA && !EntraInteractiveAuth.IsSupported) | ||
| { | ||
| Console.Error.WriteLine( | ||
| "Interactive Microsoft Entra MFA (--auth entra) needs a desktop window for the Windows " + | ||
| "account picker, so it cannot run from the CLI."); | ||
| Console.Error.WriteLine( | ||
| "Use the Studio app for interactive sign-in, or a non-interactive identity here: " + | ||
| "--auth sql with a stored credential, or a service principal / managed identity."); | ||
| Environment.ExitCode = 1; | ||
| throw new InvalidOperationException("Interactive Entra authentication is not available headless"); |
There was a problem hiding this comment.
This is the actual behavior change users will hit (planview ... --auth entra refusing headless), but the tests added in this PR only exercise EntraInteractiveAuth.Register/IsSupported in isolation — nothing calls BuildServerConnection with auth: "entra" to confirm it throws, sets ExitCode = 1, and writes the guidance text. BuildServerConnection is a plain static method with an ICredentialService seam (already exercised by the "No credential found" branch above), so a same-shaped test here would be cheap and would pin the actual regression this PR fixes for the CLI.
Separately worth noting for whoever adds that test: EntraInteractiveAuth._registered is a process-wide static that OnWindows_RegistersOnceAndIsIdempotent flips to true and never resets. If a future CLI test asserting the entra-refusal path runs in the same test process after that test (xunit doesn't guarantee cross-class ordering), IsSupported would read true from the leaked state and the refusal branch would silently stop being exercised on Windows CI.
There was a problem hiding this comment.
Fixed in 28be5ed — CliConnectionResolverTests now pins the refusal with a real BuildServerConnection call. Your static-state warning turned out to be the observed behavior, not a hypothetical: xunit runs the registration tests first in this process, so a skip-if-contaminated guard skipped on exactly the platform where the branch is reachable. Made it deterministic instead: an internal test-only reset of the one-way flag (InternalsVisibleTo already in place), plus a shared collection across both test classes so the process-wide state can't flip mid-test. 292 tests, 291 pass, 1 skip (the off-Windows pin, correctly skipped on Windows).
…efusal Two findings from the PR review: - ActiveWindowHandle is called by MSAL from whatever thread SqlClient's token acquisition runs on, while desktop.Windows is a UI-thread-owned collection that can be mutated mid-enumeration by a dialog opening or closing. Marshal the lookup through Dispatcher.UIThread (safe: no connection path blocks the UI thread on auth - every open in the app is async), and degrade to IntPtr.Zero if the dispatcher can't deliver, which is MSAL's normal no-handle failure rather than a new one. - The CLI's --auth entra refusal had no test. Pin it with a real call through BuildServerConnection, made deterministic by an internal test-only reset of the process-wide one-way registration flag (the registration tests do run first in this process, so a skip-if- contaminated guard would have skipped on exactly the platform where the branch is reachable). The two test classes share a collection so the global state can't flip mid-test. Also drop using directives already provided as global usings in both projects, which the IDE flags on every touch of these files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed. This is a clean, well-scoped fix — no correctness, security, or convention issues found. Specifics I checked:
No inline findings to flag. |
What does this PR do?
Closes #425.
Microsoft Entra MFA could not work in Studio on Windows at all. Current
Microsoft.Data.SqlClient(7.0.2 here) routesAuthentication=ActiveDirectoryInteractivethrough the Windows WAM broker, and WAM requires the application to supply the HWND that will own its account picker.ServerConnection.BuildConnectionStringset the authentication mode but nothing in the repo ever registered an auth provider, so users gotinstead of a prompt. Not tenant-specific — broken for every Windows user on a current build. 1.4.3 worked because it predates WAM being the default and fell back to a browser.
Reported by @joshdbe (via erikdarlingdata/PerformanceMonitor#2184), whose observation that 1.4.3 works and 1.19.1 doesn't is what turned a vague auth error into a specific dependency behaviour change. Studio goes first because it's the tool he actually uses; the Lite half follows.
The three decisions worth reviewing
Registered once at startup, not per connection.
SqlAuthenticationProvider.SetProviderinstalls against the authentication method, not a connection, so one call covers everySqlConnectionthe process opens. Studio opens them from at least three unrelated places — the connection dialog, the query session control, andSchemaQueryService— and threading a handle through all of them would be a change every future call site could forget to make.Windows-only. WAM doesn't exist on macOS or Linux, where interactive auth already works through the system browser and needs no handle. Registering a handle-supplying provider unconditionally would add a failure mode to the platforms Avalonia exists to serve.
Registerreturnsfalsewhen it declines, so callers can distinguish "not applicable" from "done".Handle resolved per prompt, preferring the active window. Two reasons it isn't captured at startup: a window's platform handle isn't valid until the window has been sourced, and the right parent is whichever window is actually in front. Connections are usually triggered from the connection dialog, so parenting the picker to the main window behind it would let the picker appear behind the dialog the user is looking at. Falls back to main window, then
IntPtr.Zero— which MSAL treats as no handle, so the prompt fails rather than the app crashing.The CLI
planviewaccepts--auth entraand has no window to give, so it would have failed deep inside MSAL with a message that tells the operator nothing. It now refuses up front and names the modes that work headless.Deliberately not silently substituting another auth mode — connecting as a different identity than the one asked for is worse than refusing. Whether the CLI should grow device-code flow is left as the open question on #425 rather than guessed at here.
How was this tested?
Honestly: partially, and the important half isn't done yet.
ActiveDirectoryAuthenticationProvider,SetParentActivityOrWindowFuncandSqlAuthenticationProvider.SetProviderall exist and bind on 7.0.2.Registerdeclines, the provider is never even consulted, andIsSupportedstays true so the CLI guard doesn't refuseentraon platforms where it works); and idempotent registration, which matters becauseSetProvideris process-wide and a second call would otherwise silently replace the first once the SSMS extension wires in.What is NOT verified: that the picker actually authenticates against a real tenant. I can only demonstrate a prompt appears; @joshdbe has offered to test against his Entra-MFA-on-Azure-VM environment and I'm not claiming this fixed until he does.
Note on the local suite: the full
PlanViewer.Core.Testsrun can't be used as a gate on macOS ARM64 — it wedges in a CoreCLR GC-suspension livelock (.NET 10.0.8), unrelated to this change. CI's--blame-hangwatchdog is the regression gate here. Three orphaned test hosts from that hang were burning ~108% CPU each and have been cleaned up.Follow-ups deliberately not in scope
WindowInteropHelperinstead of Avalonia's.Update after review (28be5ed)
The revived review gate (#428) produced two findings on this diff, both taken:
ActiveWindowHandleis now marshaled throughDispatcher.UIThread(fast path when already on it), catching toIntPtr.Zeroso a dispatcher failure degrades to MSAL's normal no-handle error. Verified the deadlock precondition first: no connection path blocks the UI thread on auth — every open in the app is async — so the blocking Invoke is safe.BuildServerConnectiontest. The reviewer's static-state warning was the observed behavior: the registration tests run first in-process, so a skip-if-contaminated guard would have skipped on exactly the platform where the branch is reachable. The test instead resets the one-way flag via an internal test-only hook, and both test classes share a collection so the process-wide state can't flip mid-test.Suite is now 292 tests: 291 pass, 1 skip (the off-Windows contract pin, correctly skipped on Windows).