-
Notifications
You must be signed in to change notification settings - Fork 331
WAM POC #2884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WAM POC #2884
Changes from 5 commits
c4e5c4a
6b0a7b9
f2022ae
14767bc
37d77ee
86d23d4
b5ceede
4fbdd68
c32327d
dd3df24
0ded77a
3fbce21
3b08acf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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.Extensions.Caching.Memory" Version="$(MicrosoftExtensionsCachingMemoryVersion)" /> | ||
| <PackageReference Include="Microsoft.Identity.Client.Broker" Version="4.64.0" /> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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,18 @@ | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| 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 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,12 +111,32 @@ public override void BeforeUnload(SqlAuthenticationMethod authentication) | |||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| #if NETFRAMEWORK | ||||||||||||||||||||||||||||||||||||||||
| private Func<System.Windows.Forms.IWin32Window> _iWin32WindowFunc = null; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /// <include file='../../../../../../doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml' path='docs/members[@name="ActiveDirectoryAuthenticationProvider"]/SetIWin32WindowFunc/*'/> | ||||||||||||||||||||||||||||||||||||||||
| public void SetIWin32WindowFunc(Func<System.Windows.Forms.IWin32Window> iWin32WindowFunc) => this._iWin32WindowFunc = iWin32WindowFunc; | ||||||||||||||||||||||||||||||||||||||||
| public void SetIWin32WindowFunc(Func<System.Windows.Forms.IWin32Window> iWin32WindowFunc) => SetParentActivityOrWindow(iWin32WindowFunc); | ||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private Func<object> _parentActivityOrWindowFunc = null; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||
| /// <param name="parentActivityOrWindowFunc"></param> | ||||||||||||||||||||||||||||||||||||||||
| public void SetParentActivityOrWindow(Func<object> parentActivityOrWindowFunc) => this._parentActivityOrWindowFunc = parentActivityOrWindowFunc; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private delegate Task<AuthenticationResult> InvokeDelegate(IPublicClientApplication app, string[] scopes, Guid connectionId, string userId, | ||||||||||||||||||||||||||||||||||||||||
| SqlAuthenticationMethod authenticationMethod, CancellationTokenSource cts, ICustomWebUi customWebUI, Func<DeviceCodeResult, Task> deviceCodeFlowCallback); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private SynchronizationContext _synchronizationContext = null; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||
| /// <param name="synchronizationContext"></param> | ||||||||||||||||||||||||||||||||||||||||
| public void SetSynchronizationContext(SynchronizationContext synchronizationContext) => this._synchronizationContext = synchronizationContext; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /// <include file='../../../../../../doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml' path='docs/members[@name="ActiveDirectoryAuthenticationProvider"]/AcquireTokenAsync/*'/> | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| public override async Task<SqlAuthenticationToken> AcquireTokenAsync(SqlAuthenticationParameters parameters) | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -205,11 +229,7 @@ public override async Task<SqlAuthenticationToken> AcquireTokenAsync(SqlAuthenti | |||||||||||||||||||||||||||||||||||||||
| redirectUri = "http://localhost"; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||
| PublicClientAppKey pcaKey = new(parameters.Authority, redirectUri, _applicationClientId | ||||||||||||||||||||||||||||||||||||||||
| #if NETFRAMEWORK | ||||||||||||||||||||||||||||||||||||||||
| , _iWin32WindowFunc | ||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||
| PublicClientAppKey pcaKey = new(parameters.Authority, redirectUri, _applicationClientId, _parentActivityOrWindowFunc); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| AuthenticationResult result = null; | ||||||||||||||||||||||||||||||||||||||||
| IPublicClientApplication app = await GetPublicClientAppInstanceAsync(pcaKey, cts.Token).ConfigureAwait(false); | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -284,14 +304,45 @@ 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. | ||||||||||||||||||||||||||||||||||||||||
| result = await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback).ConfigureAwait(false); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| InteractiveAuthStateObject state = new InteractiveAuthStateObject() | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| app = app, | ||||||||||||||||||||||||||||||||||||||||
| scopes = scopes, | ||||||||||||||||||||||||||||||||||||||||
| connectionId = parameters.ConnectionId, | ||||||||||||||||||||||||||||||||||||||||
| userId = parameters.UserId, | ||||||||||||||||||||||||||||||||||||||||
| authenticationMethod = parameters.AuthenticationMethod, | ||||||||||||||||||||||||||||||||||||||||
| cts = cts, | ||||||||||||||||||||||||||||||||||||||||
| customWebUI = _customWebUI, | ||||||||||||||||||||||||||||||||||||||||
| deviceCodeFlowCallback = _deviceCodeFlowCallback, | ||||||||||||||||||||||||||||||||||||||||
| _taskCompletionSource = new TaskCompletionSource<AuthenticationResult>() | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| _synchronizationContext.Post(AcquireTokenInteractiveDeviceFlowAsync, state); | ||||||||||||||||||||||||||||||||||||||||
| result = await state._taskCompletionSource.Task; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| 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. | ||||||||||||||||||||||||||||||||||||||||
| result = await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod, cts, _customWebUI, _deviceCodeFlowCallback).ConfigureAwait(false); | ||||||||||||||||||||||||||||||||||||||||
| InteractiveAuthStateObject state = new InteractiveAuthStateObject() | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| app = app, | ||||||||||||||||||||||||||||||||||||||||
| scopes = scopes, | ||||||||||||||||||||||||||||||||||||||||
| connectionId = parameters.ConnectionId, | ||||||||||||||||||||||||||||||||||||||||
| userId = parameters.UserId, | ||||||||||||||||||||||||||||||||||||||||
| authenticationMethod = parameters.AuthenticationMethod, | ||||||||||||||||||||||||||||||||||||||||
| cts = cts, | ||||||||||||||||||||||||||||||||||||||||
| customWebUI = _customWebUI, | ||||||||||||||||||||||||||||||||||||||||
| deviceCodeFlowCallback = _deviceCodeFlowCallback, | ||||||||||||||||||||||||||||||||||||||||
| _taskCompletionSource = new TaskCompletionSource<AuthenticationResult>() | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| _synchronizationContext.Post(AcquireTokenInteractiveDeviceFlowAsync, state); | ||||||||||||||||||||||||||||||||||||||||
| result = await state._taskCompletionSource.Task; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -345,12 +396,27 @@ private static async Task<AuthenticationResult> TryAcquireTokenSilent(IPublicCli | |||||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private static async Task<AuthenticationResult> AcquireTokenInteractiveDeviceFlowAsync(IPublicClientApplication app, string[] scopes, Guid connectionId, string userId, | ||||||||||||||||||||||||||||||||||||||||
| SqlAuthenticationMethod authenticationMethod, CancellationTokenSource cts, ICustomWebUi customWebUI, Func<DeviceCodeResult, Task> deviceCodeFlowCallback) | ||||||||||||||||||||||||||||||||||||||||
| private class InteractiveAuthStateObject | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+487
to
+495
|
||||||||||||||||||||||||||||||||||||||||
| 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
AI
Jan 7, 2026
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 method signature changed from returning Task<AuthenticationResult> to async void. Using async void is problematic because exceptions thrown cannot be caught by the caller and will crash the application. The current implementation uses TaskCompletionSource to communicate results, but if an exception occurs before reaching the try-catch block, it will be unhandled. Consider refactoring to use a different pattern that allows proper exception handling.
| private static async void AcquireTokenInteractiveDeviceFlowAsync(object state) | |
| { | |
| InteractiveAuthStateObject interactiveAuthStateObject = (InteractiveAuthStateObject)state; | |
| private static void AcquireTokenInteractiveDeviceFlowAsync(object state) | |
| { | |
| // Use a safe cast to avoid throwing before we can handle errors. | |
| InteractiveAuthStateObject interactiveAuthStateObject = state as InteractiveAuthStateObject; | |
| if (interactiveAuthStateObject is null) | |
| { | |
| return; | |
| } | |
| // Fire-and-forget the internal async implementation. All exceptions are | |
| // handled inside and propagated through the TaskCompletionSource. | |
| _ = AcquireTokenInteractiveDeviceFlowAsync(interactiveAuthStateObject); | |
| } | |
| private static async Task AcquireTokenInteractiveDeviceFlowAsync(InteractiveAuthStateObject interactiveAuthStateObject) | |
| { |
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 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.)
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.
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.
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.
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.
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 😆