Record and classify Entra MFA broker failures instead of showing one line and logging nothing (#3196)

[erikdarlingdata]

Adding an Azure SQL Database server with Microsoft Entra MFA can fail with a Windows account-broker error that the process records nowhere. AddServerDialog.RunConnectionTestAsync catches the exception, keeps ex.Message, and never calls AppLogger — there is no AppLogger reference in that file at all — so the dialog text is the only artefact the failure leaves behind. The reporter of #3196 hit exactly that, went looking for a log, and found neither the data directory nor a logs directory under it.

This does not fix the authentication failure. It cannot: see the mechanism below, and see the note about verification.

What the driver actually does, and why there is no fallback to reach for

Read off Microsoft.Data.SqlClient 7.0.2, Microsoft.Identity.Client 4.84.2, Microsoft.Identity.Client.Broker 4.84.2 and msalruntime 0.20.6 — all four are what the lockfile resolves for Lite.

ActiveDirectoryAuthenticationProviderOptions.UseWamBroker looks like an opt-out and is not one. The provider computes:

_useWamBroker = _applicationClientId == "2fd908ad-0664-4344-b9be-cd3e8b574c38" || options.UseWamBroker;

That GUID is the driver's own first-party Entra application id and it is the field's initializer, so an application that does not supply an ApplicationClientId — which is every caller of the parameterless new ActiveDirectoryAuthenticationProvider(), including EntraInteractiveAuth — pins the broker on and makes the flag inert. The driver's own doc comment agrees: "When null, the SqlClient first-party application id is used and WAM broker mode is forced on (regardless of UseWamBroker)."

MSAL falls back to a browser only when the broker is unavailable — IsBrokerInstalledAndInvokable returns false for an unsupported OS, a non-AAD authority, or a failed msalruntime init. An invoked broker that returns an error is propagated, not retried another way. So a broker refusal has no in-process remedy.

The broker's own explanation of why it refused is also unreachable. msalruntime redacts it to the literal string Context: (pii) unless PII is enabled, MSAL only enables it via s_lazyCore.Value.EnablePii(_logger.PiiLoggingEnabled), and SqlClient's CreateClientAppInstance never calls WithLogging. There is no seam to turn it on from here.

What is left is the stage the handshake died at, which the error text does settle — and the stage is the difference between a bug in this application and a condition on the user's machine:

Stage	Marker	Whose problem
WindowHandleMissing	window_handle_required	ours — IntPtr.Zero reached WithParentActivityOrWindow
BrokerUnavailable	wam_runtime_init_failed	the machine — msalruntime did not load
BrokerRejected	unknown_broker_error, WAM_provider_error_*	the Windows account side, past anything we control

That table is also why #2184 is not the defect here. RuntimeBroker.AcquireTokenInteractiveAsync throws window_handle_required when _parentHandle == IntPtr.Zero, and the reported error is unknown_broker_error — so the handle was non-zero and #2184's plumbing is working. Two further checks agree: v3.6.0 (the reported version) contains EntraInteractiveAuth.cs, and MSAL's WithParentActivityOrWindowFunc invokes the callback per AcquireTokenInteractive rather than capturing it at Build(), so a cached IPublicClientApplication cannot hand the broker a stale HWND.

The change

EntraBrokerFailure (shared) classifies a failure by stage, walking the whole inner-exception chain — the driver wraps MSAL's exception, which wraps the broker's, and Message is the outermost layer only. Matching is on the rendered text rather than on a typed MsalException, so Lite is not pinned to whichever MSAL version the driver happens to carry; the codes travel in the text because SqlClient interpolates them as Error code 0x<code>.

ConnectionFailureMessage composes the dialog body: the driver's error first (it carries the codes a report needs), then what stage it came from, then the resolved log directory. The log location is in the dialog because a path like %LOCALAPPDATA%\PerformanceMonitorLite-Data\logs is not guessable from a dialog that does not name it.

AddServerDialog logs the exception through AppLogger.Error, which already walks the inner chain recursively — the call was simply missing. Cancellation is excluded: it is a decision the user made and has its own dialog, so logging it as an error would file user intent as a fault. The log line names the selected authentication mode, taken from the radio buttons rather than read back off the connection string, because the connection string holds a password.

ServerManager.CheckServerConnectivity's SqlException arm now passes the exception rather than ex.Message, matching the generic arm beside it. That is the path a saved Entra MFA server hits on every collection cycle, and a SqlException is the shape this failure takes.

Verification

Not verified against a live Entra-MFA tenant. It cannot be from here — reproducing an interactive Entra failure needs a real tenant and a Windows host, which is the same limit #2184 had (its fix was confirmed by that issue's reporter, not by CI). The classifier's doc comment says so in the file. What is verified is the mechanism, read off the four packages above, and the classification boundaries, pinned by 21 tests.

Every pin was red-proved by mutation, restored, and re-confirmed against a Total: 21, Failed: 0, Not Run: 0 baseline:

Mutation	Result
chain walk reduced to the outermost Message	Failed: 1
refusal arm tested ahead of the handle arm	Failed: 1
refusal marker loosened to the bare word broker	Failed: 2 (both Service Broker cases)
Explain returns null for BrokerUnavailable	Failed: 1
Compose never appends the driver error	Failed: 4
blank log directory renders a pointer to nowhere	Failed: 1
the AppLogger.Error call deleted, its comment kept	Failed: 1

The last one needed its instrument mutated too. ConnectionTest_WritesTheExceptionToTheLog asserts a call site in source, because the catch lives in a WPF Window that behavioural coverage cannot reach — and the block carries a comment naming AppLogger.Error. With the call deleted and CSharpSourceWalker.StripCommentsAndStrings removed from the pin, the pin passes: Failed: 0. The strip is what makes it a check on code rather than on my own comment.

Service Broker is in the negative cases deliberately: SQL Server's own feature shares the word and nothing else, and a substring match on broker tells a user their Windows account is broken when a queue is disabled.

SKU parity

Darling needs no equivalent. Its Viewer rejects Entra MFA outright at the credential step (ServerStoreCredential.UnsupportedAuthMessage, AddServerDialog.xaml.cs:340 — "no Darling service connect path"), so there is no interactive-auth seam there to keep in parity. EntraBrokerFailure still lands in PerformanceMonitor.Common beside MfaAuthenticationHelper, which is where the existing Entra helper lives.

Not done here

Two routes would actually unblock this user class, and both are product decisions rather than bug fixes:

• Ship an own Entra application id. Supplying any ApplicationClientId other than the driver's flips UseWamBroker to defaulting false, which moves the redirect URI to http://localhost and the flow to the system browser. It needs a registered multi-tenant Entra application, and a third-party client id may need the user's tenant admin to consent.
• Offer a WAM-free Entra mode. Lite's vocabulary is Windows / SQL Server / Entra MFA / service principal / managed identity, and the only interactive one is the broker path. ActiveDirectoryDefault returns from AcquireTokenAsync before any MSAL public-client application is built, so it never touches WAM at all — it runs DefaultAzureCredential, which picks up an az login. ActiveDirectoryDeviceCodeFlow also avoids the broker (MSAL's DeviceCodeRequest references it nowhere) and works with the driver's own client id, needing only a callback to show the code, since the driver's default writes it to a console this app does not have.

CHANGELOG

Not edited, per lane convention. Entry text:

• Adding an Entra MFA server could fail with a Windows broker error that was recorded nowhere ([Authentication error connecting to Azure SQL Database. #3196]) - AddServerDialog's connection test kept ex.Message and never called AppLogger, so a failed Entra MFA sign-in left a dialog and no log entry; the reporter of Authentication error connecting to Azure SQL Database. #3196 could not send anything but a screenshot. The failure is now classified by which stage of the WAM broker handshake it died at - the application not supplying a window handle, the broker runtime not loading, or the broker running and refusing - and the whole inner exception chain is logged, since the driver wraps MSAL's exception which wraps the broker's and Message is only the outermost layer. The dialog names the resolved log directory, because that path is not guessable from a dialog that does not print it. ServerManager's SqlException arm passes the exception rather than its message, matching the generic arm beside it - that is the path a saved Entra MFA server takes on every collection cycle. The authentication failure itself is not fixed and cannot be from here: Microsoft.Data.SqlClient forces the WAM broker on for any caller using the driver's own Entra application id, UseWamBroker is inert in that configuration, MSAL falls back to a browser only when the broker is unavailable rather than when an invoked broker refuses, and the broker's stated reason is redacted to Context: (pii) with no seam to enable it. Unverified against a live Entra-MFA tenant, which is the only place this class can be confirmed

[claude]

Reviewed the diff (Lite.Tests/EntraBrokerFailureTests.cs, Lite/Helpers/ConnectionFailureMessage.cs, Lite/Services/ServerManager.cs, Lite/Windows/AddServerDialog.xaml.cs, PerformanceMonitor.Common/Helpers/EntraBrokerFailure.cs).

Correctness — logic checks out: EntraBrokerFailure.Classify walks the whole inner-exception chain, orders the specific arms (window handle, runtime init) ahead of the catch-all refusal arm so a message carrying both markers reports the more actionable one, and the Service Broker negative test cases correctly guard the bare-broker substring trap. ConnectionFailureMessage.Compose orders driver-error → explanation → log-path as documented and correctly omits the log pointer for blank/null paths rather than pointing at nothing. The logs subdirectory path built in AddServerDialog.ComposeFailureDetail (Path.Combine(App.DataDirectory, "logs")) matches what App.xaml.cs actually passes to AppLogger.Initialize, so the dialog's stated log location is accurate. Left one inline comment on a doc/comment bug: the new comment in AddServerDialog.xaml.cs says logging is "not gated on mfaCancelled" directly above an if (!mfaCancelled) check that gates it — the code is correct (matches the PR description), only the comment contradicts it.

Lite/Darling parity — no drift. The PR body's claim that Darling needs no equivalent checks out: Darling/PerformanceMonitor.Darling.Viewer/AddServerDialog.xaml.cs rejects EntraMFA/ServicePrincipal/ManagedIdentity outright via ServerStoreCredential.UnsupportedAuthMessage before any broker path is reachable, so there's no interactive-auth seam to keep in sync. EntraBrokerFailure living in PerformanceMonitor.Common rather than Lite is consistent with where MfaAuthenticationHelper already lives.

Security — no concerns. The log line names the selected auth mode via radio-button state rather than the connection string, so no password/secret reaches the log. This is a local desktop-app change with no network/SQL surface.

Performance — negligible; this only touches the failure path.

Nothing else stood out — the classifier's marker ordering, the ServerManager.CheckServerConnectivity change (passing the exception instead of ex.Message, matching the sibling generic-catch arm), and the two AddServerDialog call sites (TestButton_Click, save-path connection test) are all consistent with the PR's stated intent and each other.

[claude]

Review

Went through the diff (EntraBrokerFailure, ConnectionFailureMessage, the AddServerDialog.xaml.cs/ServerManager.cs call sites, and the new test file) against correctness, Lite/Darling parity, security, and performance.

Correctness

• EntraBrokerFailure.Classify walks the full inner-exception chain and orders the three broker markers correctly (handle-missing and runtime-init are checked ahead of the generic-refusal marker, matching the "specific arm wins" comment and its pinning test).
• ConnectionFailureMessage.Compose ordering (driver error → explanation → log path) and the null/whitespace guards for driverError/logDirectory match the composition tests.
• ComposeFailureDetail's log directory (Path.Combine(App.DataDirectory, "logs")) matches exactly where AppLogger.Initialize actually points (Lite/App.xaml.cs), so the dialog won't point at a directory that doesn't exist.
• AddServerDialog.RunConnectionTestAsync's new tuple element and both call sites (TestButton_Click, SaveButton_Click) destructure consistently, and the mfa-cancelled branch is untouched/still shown via its own dialog.
• ServerManager.CheckConnectionAsync's SqlException arm now passes ex to LogWarning using the standard ILogger (Exception, string, args) overload — correct usage, and now matches the sibling generic-Exception arm.
• AppLogger.Error(string, string, Exception?) signature matches the new call site; PerformanceMonitor.Common was already imported in AddServerDialog.xaml.cs, so no missing-using issue.

Lite/Darling parity

• Confirmed Darling's AddServerDialog/AddMultipleServersDialog reject Entra MFA outright via ServerStoreCredential.UnsupportedAuthMessage (Darling/PerformanceMonitor.Darling.Viewer/ServerStoreCredential.cs:54) — there's genuinely no equivalent interactive-broker seam on that side, so skipping a Darling change here is correct. EntraBrokerFailure living in PerformanceMonitor.Common alongside MfaAuthenticationHelper is the right home for a type only Lite currently consumes.
• No Darling analog of ServerManager's SqlException/LogWarning pattern exists, so no parity gap there either.

Security

• No secrets in the new log line (DescribeSelectedAuthentication() reads the radio buttons, not the connection string, specifically to avoid leaking the password). MSAL's own Context: (pii) redaction means nothing more sensitive leaks through the classifier.

Tests

• New Lite.Tests/EntraBrokerFailureTests.cs uses the existing Darling.Tests.CSharpSourceWalker linked-compile idiom already present in Lite.Tests.csproj (no new cross-app wiring needed), and the ReadRepoFile walk-up helper matches the pattern used elsewhere in this test project.

Nothing blocking found. One very minor, non-blocking observation: ConnectionFailureMessage.Compose treats an empty (but non-null) driverError as "nothing to add" (IsNullOrWhiteSpace), whereas the old inline code showed "\n\nError: " even for an empty string — harmless in practice since SqlException.Message is never empty, just noting the behavior shifted slightly.