PR Review: Microsoft Entra MFA Support

Overall well-structured feature with good UX thinking around the MFA popup problem. The UserCancelledMfa flag and serialization via s_mfaAuthLock are solid approaches. A few issues to address, one of which is a breaking change for existing users.

Critical: Backward compatibility break

ServerConnection.AuthenticationType defaults to "Windows". Existing users with SQL Server auth have UseWindowsAuth = false in their servers.json but no AuthenticationType field. On deserialization, AuthenticationType will be "Windows" while UseWindowsAuth is false.

Since BuildConnectionString and GetConnectionString now branch on AuthenticationType instead of UseWindowsAuth, existing SQL auth connections will silently switch to Windows auth and break.

Needs migration logic in LoadServers() or a computed getter:

public string AuthenticationType 
{ 
    get => _authenticationType ?? (UseWindowsAuth ? "Windows" : "SqlServer");
    set => _authenticationType = value;
}

Bug: AADSTS50126 is not cancellation

IsMfaCancelledException includes aadsts50126 ("Error validating credentials due to invalid username or password"). This is wrong credentials, not user cancellation. A user who types the wrong password would have their server silently disabled until manual reconnect. This error code should be removed from the check.

AADSTS50058 ("Need to select account") is also debatable — it fires when multi-account selection is needed, not necessarily when the user cancels.

Code duplication: IsMfaCancelledException appears 3 times

Identical method in RemoteCollectorService.cs, ServerManager.cs, and AddServerDialog.xaml.cs. Should be extracted to a shared static helper (e.g., on ServerConnection or a small utility class).

Magic strings

"Windows", "SqlServer", "EntraMFA" are scattered across 7 files as raw string comparisons. One typo and things silently break. Consider constants or an enum:

public static class AuthTypes
{
    public const string Windows = "Windows";
    public const string SqlServer = "SqlServer";
    public const string EntraMFA = "EntraMFA";
}

Dead code in CheckConnectionAsync

Two sequential checks (around lines 355-382 in the diff):

// First check — returns for ALL MFA servers when !allowInteractiveAuth
if (!allowInteractiveAuth && server.AuthenticationType == "EntraMFA") { return...; }

// Second check — unreachable, first block already returned
if (server.AuthenticationType == "EntraMFA" && previousStatus.UserCancelledMfa && !allowInteractiveAuth) { return...; }

The second block is unreachable because the first already returns for all MFA servers during background checks.

UseWindowsAuth is now redundant

Both UseWindowsAuth and AuthenticationType are set and persisted. They could get out of sync. Consider making UseWindowsAuth a computed property (=> AuthenticationType == "Windows") or removing it (ties back to the migration issue above).

Minor: _isDialogOpen static flag

Uses Closing event — if the dialog crashes before that fires, the flag stays true permanently and MFA connections are blocked forever. Closed event is safer since it always fires.

Summary

All six issues from the first review have been addressed:

• Backward compatibility: UseWindowsAuth is now a computed property with a migration setter — old configs work correctly
• AADSTS50126 removed: Only explicit user cancellation messages are detected now
• Shared helper: MfaAuthenticationHelper eliminates the 3x duplication
• Constants: AuthenticationTypes class replaces magic strings throughout
• Unreachable check: Combined into single block with ternary
• Closed event: Safer than Closing for the dialog flag

One minor nit: MigrateServerAuthentication() is effectively a no-op (just debug logging) since migration happens in the UseWindowsAuth setter. Not blocking.

LGTM — nice work on the update, Claudio!