WAM POC

[mdaigle]

This PR enforces Web Account Manager (WAM) usage whenever possible when performing interactive authentication.

Different UI frameworks have different dependency requirements to enable WAM, as listed below.

• Each requires at least Microsoft.Identity.Client. Maui apps and apps targeting -windows .net versions come bundled with WAM support.
• Other UI frameworks also require the Microsoft.Identity.Client.Broker package. This package adds broker support to interactive token acquisition flows.
• The Microsoft.Identity.Client.Desktop package is only required for apps that want both WAM and embedded browser support for interactive auth. Because we want to enforce WAM usage, we do not use this package. Apps that cannot support WAM can choose to fall back to interactive auth via the system browser.

 

NetCore behavior:

Framework	SynchronizationContext Required	WindowHandle required
Console-based	❌	❌
Maui	❌	✅
UI3	❌	✅
WPF	✅	✅
Headless app	undefined	undefined
UWP (not supported)	undefined	undefined

NetFX behavior:

Framework	SynchronizationContext Required	WindowHandle required
Console	❌	❌
Windows Forms	✅	✅
WPF
Headless app	undefined	undefined

[JRahnama]

M.I.C.Broker is not available for net6-windows, .Netframwork and legacy application.

Also adding one more dependency.......

[cheenamalhotra]

@mdaigle we wanna conditionally include this package dependency, based on environment/config it's available in.

Secondly, please consider including this in nuspec as well.

[edwardneal]

Thanks @mdaigle. A few comments below, mostly about cross-targeting more of this and breaking the hard dependency on Windows Forms so it'll work with WPF, Android and iOS.

There's a hardcoded reference to the package version in the csproj files, and a set of unused PInvoke-related methods at the end of the file - probably nothing which needs correcting in the POC.

[edwardneal]

I've been thinking about how to eliminate this discrepancy between the .NET Core and .NET Framework API surfaces for a little while. What do you think about replacing the .NET Framework-only SetIWin32WindowFunc(Control) method with a cross-targeted SetParentActivityOrWindow(object) method? This would introduce the same support on Android, and contribute to removing the dependency on System.Windows.Forms.

API compatibility could be maintained by marking the .NET Framework-only SetIWin32WindowFunc method as obsolete and having it wrap SetParentActivityOrWindow.

[mdaigle]

I do think it's a nicer experience to provide a strongly typed public API. That way it's harder for a consumer to pass the wrong object and get unexpected errors. At the same time, we'd need to effectively duplicate the MSAL SetParentActivityOrWindow options (there are 5 total) and maintain them over time. I think I'd lean towards that duplication for the improved experience given that these methods are just simple passthroughs that cast their inputs to object.

[mdaigle]

We need a dependency on M.I.C.Broker, which already depends on System.Windows.Forms itself so I don't think we can avoid that dependency.

[edwardneal]

I do think it's a nicer experience to provide a strongly typed public API.

I agree, but I'm also pretty averse to making SqlClient depend on GUI implementation details by default. One older version of Azure.Identity introduced a similar reference to System.Windows.Forms by accident, and apparently (#2309) this required the .NET desktop runtime to be installed on a headless web server.

Would some kind of OnConfigurePublicClientApplication hook which accepts the PublicClientApplicationBuilder work for SSMS here? Or could the class be unsealed, with CreateClientAppInstance renamed and made virtual? Either of these would push the GUI dependency out of the data access layer.

[mdaigle]

Yeah, I agree there too. The trouble is that we're also tasked with enforcing broker usage as much as possible for any consumer of our hard coded entra id application: https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs#L1198

To me this really gets into the boundaries of responsibility for a library. I'm of the opinion that none of this code should actually be living in the driver itself. I'd prefer consumers to orchestrate token credential acquisition at the application level and pass it in to the driver. It would sidestep a lot of these concerns. We're also a really unique library in this respect. I've yet to find another library that provides its own default application id. It's more common in end-user applications like powershell or azure-cli.

I'd started to consider whether we could just remove our default implementation for interactive auth given that it's becoming so tied to individual UI frameworks. What I bumped into is that the intention was to provide a default implementation to make it super easy to integrate your app with entra id without having to set up your own entra id application, grant it permissions, etc. That's why we provide the hard coded app id. It's a reasonable goal because it would be annoying to set up an entra id application if all you want is to connect to an azure sql instance for POC work, for example.

Maybe it makes the most sense to split default interactive auth support out into its own package. That way we can enforce broker usage and not worry about dependency bloat.

[edwardneal]

I agree, and I think SqlClient does the right thing by providing a default Azure authentication implementation (and associated app id). The situation becomes murkier when this means we're explicitly tying ourselves to a specific UI framework though; if SqlClient is part of a non-interactive service running on an Azure VM with a managed identity, it's reasonable to ask why that particular dependency's necessary.

If we supply several strongly-typed wrappers to set the value which is ultimately supplied to WithParentActivityOrWindow, I think we'd get the worst of all worlds because we'd introduce an API surface dependency on all UI frameworks which PublicClientApplicationBuilder supports. I think I see why the parentActivityOrWindowFunc parameter has the comment: "The parent as an object, so that it can be used from shared NetStandard assemblies". It's trying to avoid that situation too!

I'm pretty sure there are plans to split MDS and MDS.Azure.Auth apart, but those are for v7 and the next release is v6-preview2.

It's an awkward set of constraints, and I've not really got much to add unfortunately. It makes a certain amount of sense to me that the default Azure authentication provider should provide a default PublicClientApplication which works for a POC, then let downstream clients customise it if they want to add WAM support. This doesn't let us enforce broker usage though - doing so requires a GUI, and SqlClient simply doesn't know whether it's being used in a GUI context...

[edwardneal]

Similar to the above - but we only really need a SynchronizationContext here. We can pass in an instance of WindowsFormsSynchronizationContext or DispatcherSynchronizationContext, which enables cross-target WPF support.

SynchronizationContext.Send is a little awkward to use - we can pass in a state object and set a property on it, but we don't get a return value.

I'm pretty sure that this (and the almost-identical code below) eliminates the System.Windows.Forms dependency.

[mdaigle]

This makes sense to me, but I'll want to think about how best to expose this in the public API. The nice thing about passing in a Control is that it gives parent window and dispatcher together in one object. Having a separate SynchronizationContext specifiable will require the API to be clear that it's required when using the parent window param.

[mdaigle]

I took a shot at this change. It's very awkward to pass an async callback to SynchronizationContext.Post. So I had to add a hacky semaphore to signal back to the auth provider that the interactive window on the UI thread completed.

Do you know of any tricks to improve that interaction?

[edwardneal]

I see what you're talking about! When using SynchronizationContext.Send, I think we'd be hedged in to either the semaphore approach you've taken, or a TaskCompletionSource - but it'll have the same net effect.

Would it be workable to record SynchronizationContext.Current, call SynchronizationContext.SetSynchronizationContext to force the thread into the right sync context, await AcquireTokenInteractive, then revert back to the original sync context in a finally block? I'm less sure, but I suspect that this await on AcquireTokenInteractive might actually need us to pass true to ConfigureAwait (so it'll continue on the original sync context, ready for the finally block to revert it.)

[mdaigle]

I tried this out and it didn't give me the behavior I expected. It started returning cross-thread access exceptions again. I think the catch is that setting the synchronization context doesn't actually switch you to another thread, it only affects where the continuations run. So the first thing that actually runs async still runs on "this" thread, which is not guaranteed to be the UI thread.

So this doesn't work:

var currentSynchronizationContext = SynchronizationContext.Current;

try
{
    SynchronizationContext.SetSynchronizationContext(_synchronizationContext);
    result = await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback);
}
finally
{
    SynchronizationContext.SetSynchronizationContext(currentSynchronizationContext);
}

But something like this does:

var currentSynchronizationContext = SynchronizationContext.Current;

try
{
    SynchronizationContext.SetSynchronizationContext(_synchronizationContext);

    var t = Task.CompletedTask;

    result = await await t.ContinueWith(antecedent =>

         AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback)

        , TaskScheduler.FromCurrentSynchronizationContext());

}
finally
{
    SynchronizationContext.SetSynchronizationContext(currentSynchronizationContext);
}

[mdaigle]

The above didn't work exactly, but led to a similar approach using TaskCompletionSource that I think it more readable than manually switching between synchronization contexts.

[edwardneal]

Definitely, thanks - that looks a lot neater than my suggestion!

The only edge case I can think of is that ASP.NET Core doesn't have a synchronisation context at all. We'd hit a NullReferenceException if someone was trying to use an authentication method of ActiveDirectoryInteractive or ActiveDirectoryDeviceCodeFlow, which sounds like a misconfiguration to me. If we can't get a token and _synchronizationContext is null, it might be worth logging a trace event and throwing a more descriptive exception before trying to post to it.

Does interactive authentication now work with WPF on .NET Core? This scenario should test both of the situations I'm thinking of (any async changes between .NET Framework and Core, and any differences in synchronization context and cross-thread access between WPF and WinForms.)

[mdaigle]

We can detect a default parent window for console apps using the pinvoke methods listed at the bottom of the changeset. When we use that, we don't need to execute the token acquisition in any particular synchronization context. I need to do another pass to incorporate that in a way that makes sense. It will also need to be included conditionally with the broker library, so I'm just holding off until I figure out how I want to do that.

I'll check out how things look with WPF, I've been testing mainly with winforms.

[mdaigle]

This solution worked as-is for WPF on net core.

[edwardneal]

Great! I think this gives us the best of both worlds then, until we're ready for MDS.Azure.Auth: we don't introduce any direct dependency on System.Windows.Forms in the .NET Core build (so although we've still got a transitive dependency when compiled for Windows, that'll disappear on the library split), and we expand functionality slightly, (to WPF and iOS & Android) since .NET Core can now call WithParentActivityOrWindow. Thanks for bearing with me on this.

Looking at the PInvokes, I've not got any comments on the overall idea. I have a few comments on some behaviour, but I'll drop them in the right place.

[edwardneal]

With the comments above, a lot of these conditional compilations can probably be removed.

[edwardneal]

Picking up from JRahnama's comments - I imagine you'll probably have to conditionally reference Microsoft.Identity.Client.Broker on net8.0 and Microsoft.Identity.Client.Desktop on net462/net6.0.

Just for awareness, M.I.C.D will drag in Microsoft.Web.WebView2 - an 8MB package.

[JRahnama]

I don't see a better or more straightforward option than using an environment variable to determine if M.I.C.B is needed by the client.

I was hoping to find a way to read this from the client's config or JSON file, but that seems more challenging.

[edwardneal]

The surrounding documentation says:

For migration purposes, and if you have a .NET 6, .NET Core, or a .NET Standard application that needs to use both WAM and the embedded browser, you will also need to use the Microsoft.Identity.Client.Desktop package.

It's worth checking, but I think this means we can use MIC.Broker on .NET 8.0, and MIC.Desktop on .NET Framework or .NET 6.0. At that point, I'd expect it to be nothing more than an MSBuild Condition attribute in the netcore project file.

It's worth keeping in mind that .NET 6.0 is leaving support on 12th November, so if SqlClient v6.0 drops support at the same time this question might just be academic.

[JRahnama]

I was considering adding something like IsWamRequested, which could be sourced from a JSON file or application config within the client application. If this flag is set, the relevant setup and configuration could be enabled or applied dynamically. This was just an idea that came to mind... Just thinking out loud here 😆

[edwardneal]

Picking up from my last comment...

Very minor nit: I'm not sure where PInvokes should live; one version of the library uses the .NET Runtime-style of having one folder per DLL, one file per method, another has a static class containing them all. I've got no preference either way, just as long as it's consistent post-POC.

To stress test the idea here, I'd wonder what behaviours users should expect when an app referencing SqlClient is exercising the code path in various situations:

• App is running as a Windows service. Personal guess here: GetConsoleWindow will return IntPtr.Zero, which is indistinguishable from the desktop's hWnd. Windows will either notify the user that the service is trying to display a desktop, or it'll shuffle the login prompt "somewhere".
• Same as above, but on Linux - perhaps as a systemd unit.
• App is running on a Linux desktop environment. This is pretty predictable; we'd just need to say that the login prompt will be a child of the desktop windows on Linux. It might be possible to find an equivalent PInvoke, but I'm not sure, given the number of desktop environments there might be.
• App is running in a Docker container.
• Somebody connects to a Windows environment from another PC via PowerShell Remoting or SSH, then runs the app in that Remoting or SSH session. I'm pretty sure GetConsoleWindow will see this as a pseudoconsole session and return a real window handle, but won't display the login prompt.

Some of these situations are ridiculous. I expect the results will be pretty broken, to the point that the library can't reasonably be expected to work.

I've not thought hard about all of them, but my first instinct is to make the method available and documented, but to require developers to manually pass it as a delegate to ActiveDirectoryAuthenticationProvider.SetParentActivityOrWindow. In that situation, this could become a static method which returns the console window handle (or IntPtr.Zero) on Windows and IntPtr.Zero on other platforms, and we could just re-iterate the obvious in documentation: interactive Azure authentication is only supported when a GUI or a console window is available at runtime, and users will see undefined behaviour if that GUI or console window isn't available (with SSH as an explicit example of something which looks like a console window, but isn't.)

[mdaigle]

For now, the system broker is only available on windows through MSAL. On Linux (and other platforms) it should fall back to the embedded or system browser depending on the framework and platform combo: https://review.learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/desktop-mobile/wam?branch=main#wam-limitations

My idea is to require an opt in via a compile time constant to use interactive auth, but curious to get feedback on that. See this commit: 4fbdd68

I'm going to work on a matrix to show the expected behavior, otherwise it's hard to think through all the cases. That way we can also compare the experiences between the different options for conditional compile options.

[edwardneal]

Yes, that's fair enough.

The problem I'm thinking about is that whether we're running on Windows or Linux, interactive programs can be run non-interactively in ways that we can't easily detect. If I run something via SSH and it tries to use interactive authentication, we've got unusual behaviour: on Windows, GetConsoleHandle will return a non-zero hWnd which we can't use as a parent window; on Linux, a browser might open on the remote PC or it might just throw an exception. If I run the same app direct from the server console, it'd work as expected. I think it's mainly a question of how far we want to take any auto-detection logic, and when we just document some situations as unsupported.

[mdaigle]

Closing for now until this work is prioritized.

[Copilot]

Pull request overview

This PR introduces Web Account Manager (WAM) support for interactive authentication in Microsoft.Data.SqlClient. WAM provides a more secure and user-friendly authentication experience on Windows by leveraging the operating system's native authentication broker.

Key changes:

• Refactors authentication flow from if-else chains to a switch statement for better maintainability
• Introduces SynchronizationContext support to handle UI thread requirements for interactive authentication
• Adds platform-specific partial classes to handle Windows vs Unix implementations
• Integrates Microsoft.Identity.Client.Broker package for WAM support

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 20 comments.

Show a summary per file

File	Description
ActiveDirectoryAuthenticationProvider.cs	Core authentication provider refactored to use switch statement; adds SynchronizationContext support and WAM broker configuration; introduces async callback pattern for interactive auth
ActiveDirectoryAuthenticationProvider.Windows.cs	Windows-specific partial class implementing ParentActivityOrWindow property and console window retrieval for WAM
ActiveDirectoryAuthenticationProvider.Unix.cs	Unix-specific partial class with minimal ParentActivityOrWindow stub
Interop.GetAncestor.cs	Windows P/Invoke declaration for retrieving ancestor window handles
Interop.GetConsoleWindow.cs	Windows P/Invoke declaration for retrieving console window handle
netfx/src/Microsoft.Data.SqlClient.csproj	Adds broker package dependency and INTERACTIVE_AUTH compilation constant configuration
netfx/ref/Microsoft.Data.SqlClient.csproj	Adds conditional broker package reference for API surface
netcore/ref/Microsoft.Data.SqlClient.csproj	Adds conditional broker package reference for API surface

[Copilot]

Missing comma after the ParentActivityOrWindow property assignment. This will cause a compilation error.

Suggested change

	ParentActivityOrWindow = ParentActivityOrWindow
	ParentActivityOrWindow = ParentActivityOrWindow,

[Copilot]

The TODO comment on line 261 should be addressed or removed. If using the broker for ActiveDirectoryPassword authentication is still under consideration, this should be clarified. Otherwise, the comment should be removed before merging.

Suggested change

	//TODO: need to use broker here too?
	// For ActiveDirectoryPassword, acquire tokens directly using the username/password flow without broker.

[Copilot]

The SetParentActivityOrWindow method (line 31) lacks XML documentation. Since this is a new public API method, it should include proper XML documentation similar to other public methods in this class, describing its purpose, parameters, and usage.

Suggested change

	///
	/// </summary>
	/// <param name="parentActivityOrWindowFunc"></param>
	/// Sets the delegate used to obtain the parent activity or window for interactive authentication prompts.
	/// </summary>
	/// <param name="parentActivityOrWindowFunc">
	/// A function that returns an object representing the parent activity or window handle to associate with UI dialogs.
	/// </param>

[Copilot]

In the default case (lines 350-353), there's a code block that uses AcquireTokenByIntegratedWindowsAuth with the comment that this method is obsolete but still supported. However, this appears to be incomplete - the referenced code (lines 352-353) is not visible in the diff. The switch statement's default case should handle unsupported authentication methods appropriately, either by throwing an exception or by explicitly handling the ActiveDirectoryIntegrated case that was previously in an if statement.

[Copilot]

The InteractiveAuthStateObject class has all internal fields but lacks any access modifiers. While 'internal' is the default for nested classes, it's better practice to be explicit about the intended accessibility, especially for fields. Consider adding explicit access modifiers (likely 'internal' or 'public') to these fields for clarity.

Suggested change

	internal IPublicClientApplication app;
	internal string[] scopes;
	internal Guid connectionId;
	internal string userId;
	internal SqlAuthenticationMethod authenticationMethod;
	internal CancellationTokenSource cts;
	internal ICustomWebUi customWebUI;
	internal Func<DeviceCodeResult, Task> deviceCodeFlowCallback;
	internal TaskCompletionSource<AuthenticationResult> _taskCompletionSource;
	private IPublicClientApplication app;
	private string[] scopes;
	private Guid connectionId;
	private string userId;
	private SqlAuthenticationMethod authenticationMethod;
	private CancellationTokenSource cts;
	private ICustomWebUi customWebUI;
	private Func<DeviceCodeResult, Task> deviceCodeFlowCallback;
	private TaskCompletionSource<AuthenticationResult> _taskCompletionSource;

[Copilot]

Local scope variable 'previousPw' shadows ActiveDirectoryAuthenticationProvider.previousPw.

Suggested change

	object previousPw = s_accountPwCache.Get(pwCacheKey);
	byte[] currPwHash = GetHash(parameters.Password);
	if (previousPw != null &&
	previousPw is byte[] previousPwBytes &&
	object cachedPwEntry = s_accountPwCache.Get(pwCacheKey);
	byte[] currPwHash = GetHash(parameters.Password);
	if (cachedPwEntry != null &&
	cachedPwEntry is byte[] previousPwBytes &&

[Copilot]

Local scope variable 'currPwHash' shadows ActiveDirectoryAuthenticationProvider.currPwHash.

Suggested change

	byte[] currPwHash = GetHash(parameters.Password);
	if (previousPw != null &&
	previousPw is byte[] previousPwBytes &&
	// Only get the cached token if the current password hash matches the previously used password hash
	AreEqual(currPwHash, previousPwBytes))
	byte[] currentPwHash = GetHash(parameters.Password);
	if (previousPw != null &&
	previousPw is byte[] previousPwBytes &&
	// Only get the cached token if the current password hash matches the previously used password hash
	AreEqual(currentPwHash, previousPwBytes))

[Copilot]

Local scope variable 'previousPwBytes' shadows ActiveDirectoryAuthenticationProvider.previousPwBytes.

Suggested change

	previousPw is byte[] previousPwBytes &&
	// Only get the cached token if the current password hash matches the previously used password hash
	AreEqual(currPwHash, previousPwBytes))
	previousPw is byte[] cachedPwBytes &&
	// Only get the cached token if the current password hash matches the previously used password hash
	AreEqual(currPwHash, cachedPwBytes))

[Copilot]

Minimise the use of unmanaged code.

[Copilot]

Minimise the use of unmanaged code.

[mdaigle]

Succeeded by #3874