WAM POC #2884
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -766,7 +766,9 @@ | |
| </COMReference> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="$(MicrosoftExtensionsCachingMemoryVersion)" /> | ||
| <PackageReference Include="Microsoft.Identity.Client.Broker" Version="4.64.0" /> | ||
|
Contributor
There was a problem hiding this comment. M.I.C.Broker is not available for net6-windows, .Netframwork and legacy application. Also adding one more dependency.......
Member
There was a problem hiding this comment. @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. |
||
| <PackageReference Include="System.Memory" Version="4.5.5" /> | ||
| <PackageReference Include="System.Text.Encodings.Web"> | ||
| <Version>$(SystemTextEncodingsWebVersion)</Version> | ||
| </PackageReference> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,19 @@ | |
|
|
||
| using System; | ||
| using System.Collections.Concurrent; | ||
| using System.Diagnostics; | ||
| using System.Runtime.InteropServices; | ||
| using System.Security.Cryptography; | ||
| using System.Text; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using System.Windows.Forms; | ||
| using Azure.Core; | ||
| using Azure.Identity; | ||
| using Microsoft.Data.Common; | ||
| using Microsoft.Extensions.Caching.Memory; | ||
| using Microsoft.Identity.Client; | ||
| using Microsoft.Identity.Client.Broker; | ||
| using Microsoft.Identity.Client.Extensibility; | ||
|
|
||
| namespace Microsoft.Data.SqlClient | ||
|
|
@@ -107,10 +112,19 @@ public override void BeforeUnload(SqlAuthenticationMethod authentication) | |
| } | ||
|
|
||
| #if NETFRAMEWORK | ||
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| public delegate Task<AuthenticationResult> InvokeDelegate(IPublicClientApplication app, string[] scopes, Guid connectionId, string userId, | ||
| SqlAuthenticationMethod authenticationMethod, CancellationTokenSource cts, ICustomWebUi customWebUI, Func<DeviceCodeResult, Task> deviceCodeFlowCallback); | ||
|
|
||
| private System.Windows.Forms.Control _control = null; | ||
|
|
||
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| /// <param name="control"></param> | ||
| public void SetIWin32WindowFunc(System.Windows.Forms.Control control) => this._control = control; | ||
|
Contributor
There was a problem hiding this comment. 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 API compatibility could be maintained by marking the .NET Framework-only
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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 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... |
||
| #endif | ||
|
|
||
| /// <include file='../../../../../../doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml' path='docs/members[@name="ActiveDirectoryAuthenticationProvider"]/AcquireTokenAsync/*'/> | ||
|
|
@@ -207,7 +221,7 @@ public override async Task<SqlAuthenticationToken> AcquireTokenAsync(SqlAuthenti | |
| #endif | ||
| PublicClientAppKey pcaKey = new(parameters.Authority, redirectUri, _applicationClientId | ||
| #if NETFRAMEWORK | ||
| , () => (IWin32Window) _control | ||
| #endif | ||
| ); | ||
|
|
||
|
|
@@ -284,14 +298,37 @@ previousPw is byte[] previousPwBytes && | |
| // An 'MsalUiRequiredException' is thrown in the case where an interaction is required with the end user of the application, | ||
| // for instance, if no refresh token was in the cache, or the user needs to consent, or re-sign-in (for instance if the password expired), | ||
| // or the user needs to perform two factor authentication. | ||
|
|
||
| #if NETFRAMEWORK | ||
| Func<Task<AuthenticationResult>> func = async () => | ||
| { | ||
| return await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback | ||
| ).ConfigureAwait(false); | ||
| }; | ||
|
|
||
| result = await (Task<AuthenticationResult>)_control.Invoke(func); | ||
|
Contributor
There was a problem hiding this comment. Similar to the above - but we only really need a 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.
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. 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?
Contributor
There was a problem hiding this comment. 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
Contributor
Author
There was a problem hiding this comment. 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);
}
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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 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.)
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. This solution worked as-is for WPF on net core.
Contributor
There was a problem hiding this comment. 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. |
||
| #else | ||
| result = await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback | ||
| ).ConfigureAwait(false); | ||
| #endif | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
|
|
||
| if (result == null) | ||
| { | ||
| // If no existing 'account' is found, we request user to sign in interactively. | ||
| #if NETFRAMEWORK | ||
| Func<Task<AuthenticationResult>> func = async () => | ||
| { | ||
| return await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback | ||
| ).ConfigureAwait(false); | ||
| }; | ||
|
|
||
| result = await (Task<AuthenticationResult>)_control.Invoke(func); | ||
| #else | ||
| result = await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback | ||
| ).ConfigureAwait(false); | ||
| #endif | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
| } | ||
|
|
@@ -365,6 +402,7 @@ private static async Task<AuthenticationResult> AcquireTokenInteractiveDeviceFlo | |
| */ | ||
| ctsInteractive.CancelAfter(180000); | ||
| #endif | ||
|
|
||
| if (customWebUI != null) | ||
| { | ||
| return await app.AcquireTokenInteractive(scopes) | ||
|
|
@@ -393,12 +431,12 @@ private static async Task<AuthenticationResult> AcquireTokenInteractiveDeviceFlo | |
| * | ||
| * https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/MSAL.NET-uses-web-browser#at-a-glance | ||
| */ | ||
| return await app.AcquireTokenInteractive(scopes) | ||
| .WithCorrelationId(connectionId) | ||
| .WithLoginHint(userId) | ||
| .ExecuteAsync(ctsInteractive.Token) | ||
| .ConfigureAwait(false); | ||
| } | ||
| } | ||
| else | ||
| { | ||
|
|
@@ -545,14 +583,15 @@ private IPublicClientApplication CreateClientAppInstance(PublicClientAppKey publ | |
| IPublicClientApplication publicClientApplication; | ||
|
|
||
| #if NETFRAMEWORK | ||
|
Contributor
There was a problem hiding this comment. With the comments above, a lot of these conditional compilations can probably be removed. |
||
| if (_control != null) | ||
| { | ||
| publicClientApplication = PublicClientApplicationBuilder.Create(publicClientAppKey._applicationClientId) | ||
| .WithAuthority(publicClientAppKey._authority) | ||
| .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) | ||
| .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) | ||
| .WithRedirectUri(publicClientAppKey._redirectUri) | ||
| .WithParentActivityOrWindow(() => _control) | ||
| .WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows)) | ||
| .Build(); | ||
| } | ||
| else | ||
|
|
@@ -569,6 +608,46 @@ private IPublicClientApplication CreateClientAppInstance(PublicClientAppKey publ | |
| return publicClientApplication; | ||
| } | ||
|
|
||
|
|
||
| // This is your window handle! | ||
| static IntPtr GetParent() | ||
| { | ||
| Process currentProcess = Process.GetCurrentProcess(); | ||
| return currentProcess.MainWindowHandle; | ||
| } | ||
|
|
||
| enum GetAncestorFlags | ||
| { | ||
| GetParent = 1, | ||
| GetRoot = 2, | ||
| /// <summary> | ||
| /// Retrieves the owned root window by walking the chain of parent and owner windows returned by GetParent. | ||
| /// </summary> | ||
| GetRootOwner = 3 | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Retrieves the handle to the ancestor of the specified window. | ||
| /// </summary> | ||
| /// <param name="hwnd">A handle to the window whose ancestor is to be retrieved. | ||
| /// If this parameter is the desktop window, the function returns NULL. </param> | ||
| /// <param name="flags">The ancestor to be retrieved.</param> | ||
| /// <returns>The return value is the handle to the ancestor window.</returns> | ||
| [DllImport("user32.dll", ExactSpelling = true)] | ||
| static extern IntPtr GetAncestor(IntPtr hwnd, GetAncestorFlags flags); | ||
|
|
||
| [DllImport("kernel32.dll")] | ||
| static extern IntPtr GetConsoleWindow(); | ||
|
|
||
| // This is your window handle! | ||
| IntPtr GetConsoleOrTerminalWindow() | ||
| { | ||
| IntPtr consoleHandle = GetConsoleWindow(); | ||
| IntPtr handle = GetAncestor(consoleHandle, GetAncestorFlags.GetRootOwner); | ||
|
|
||
| return handle; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. 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:
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
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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. |
||
|
|
||
| private static TokenCredentialData CreateTokenCredentialInstance(TokenCredentialKey tokenCredentialKey, string secret) | ||
| { | ||
| if (tokenCredentialKey._tokenCredentialType == typeof(DefaultAzureCredential)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The surrounding documentation says:
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
Conditionattribute 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😆