search

dotnet / SqlClient

Microsoft.Data.SqlClient provides database connectivity to SQL Server for .NET applications.
MIT License
853 stars 286 forks source link

Custom AAD token provider in Winforms can still deadlock in GetFedAuthInfo #1630

Open shueybubbles opened 2 years ago

shueybubbles commented 2 years ago

Using M.D.S 3.1

Describe the bug

I am using a custom MSAL-based authentication provider (code is way too big to paste here and I've not had time to make a smaller repro app).

The crux of the issue is that using MSAL with WAM/broker and Winforms doesn't work with M.D.S.

This is how we construct the MSAL app and fetch a token:

PublicClientApplicationBuilder.Create(appKey.ClientId)
                                            .WithAuthority(string.Format("{0}/{1}/", appKey.Authority.TrimEnd('/'), AzureAuthenticationConfiguration.TenantWildcard), validateAuthority: false)
                                            .WithRedirectUri("https://login.microsoftonline.com/common/oauth2/nativeclient")
                                            .WithWindowsBroker()
                                            .WithWindowsBrokerOptions(new WindowsBrokerOptions()
                                            {
                                                // GetAccounts will return Work and School accounts from Windows
                                                ListWindowsWorkAndSchoolAccounts = true,

                                                // Legacy support for 1st party apps only
                                                MsaPassthrough = true
                                            })
                                            .WithLogging(LoggingCallback, LogLevel.Verbose, enablePiiLogging: true)
                                            .Build();

...

return await clientApp.AcquireTokenInteractive(scopes)
                        .WithPrompt(promptType)
                        // To make MSAL auth pop up as modal of SSMS pop ups.
                        .WithParentActivityOrWindow(windowHandle)
                        .WithTenantId(tenant)
                        .ExecuteAsync();

The UI thread gets here:

                                authParamsBuilder.WithUserId(ConnectionOptions.UserID);
                                fedAuthToken = Task.Run(async () => await authProvider.AcquireTokenAsync(authParamsBuilder)).GetAwaiter().GetResult().ToSqlFedAuthToken();
                                _activeDirectoryAuthTimeoutRetryHelper.CachedToken = fedAuthToken;

>   Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.GetFedAuthToken(Microsoft.Data.SqlClient.SqlFedAuthInfo fedAuthInfo) Line 2815   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.OnFedAuthInfo(Microsoft.Data.SqlClient.SqlFedAuthInfo fedAuthInfo) Line 2661 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.TdsParser.TryRun(Microsoft.Data.SqlClient.RunBehavior runBehavior, Microsoft.Data.SqlClient.SqlCommand cmdHandler, Microsoft.Data.SqlClient.SqlDataReader dataStream, Microsoft.Data.SqlClient.BulkCopySimpleResultSet bulkCopyHandler, Microsoft.Data.SqlClient.TdsParserStateObject stateObj, out bool dataReady) Line 2683 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.TdsParser.Run(Microsoft.Data.SqlClient.RunBehavior runBehavior, Microsoft.Data.SqlClient.SqlCommand cmdHandler, Microsoft.Data.SqlClient.SqlDataReader dataStream, Microsoft.Data.SqlClient.BulkCopySimpleResultSet bulkCopyHandler, Microsoft.Data.SqlClient.TdsParserStateObject stateObj) Line 2222    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.CompleteLogin(bool enlistOK) Line 1447   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.AttemptOneLogin(Microsoft.Data.SqlClient.ServerInfo serverInfo, string newPassword, System.Security.SecureString newSecurePassword, bool ignoreSniOpenTimeout, Microsoft.Data.ProviderBase.TimeoutTimer timeout, bool withFailover, bool isFirstTransparentAttempt, bool disableTnir) Line 2320  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.LoginNoFailover(Microsoft.Data.SqlClient.ServerInfo serverInfo, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, Microsoft.Data.ProviderBase.TimeoutTimer timeout) Line 1850  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.OpenLoginEnlist(Microsoft.Data.ProviderBase.TimeoutTimer timeout, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance) Line 1692  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.SqlInternalConnectionTds(Microsoft.Data.ProviderBase.DbConnectionPoolIdentity identity, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, object providerInfo, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance, Microsoft.Data.SqlClient.SqlConnectionString userConnectionOptions, Microsoft.Data.SqlClient.SessionData reconnectSessionData, Microsoft.Data.SqlClient.ServerCertificateValidationCallback serverCallback, Microsoft.Data.SqlClient.ClientCertificateRetrievalCallback clientCallback, Microsoft.Data.ProviderBase.DbConnectionPool pool, string accessToken, Microsoft.Data.SqlClient.SqlClientOriginalNetworkAddressInfo originalNetworkAddressInfo, bool applyTransientFaultHandling) Line 537  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnectionFactory.CreateConnection(Microsoft.Data.Common.DbConnectionOptions options, Microsoft.Data.Common.DbConnectionPoolKey poolKey, object poolGroupProviderInfo, Microsoft.Data.ProviderBase.DbConnectionPool pool, System.Data.Common.DbConnection owningConnection, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 145    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionFactory.CreatePooledConnection(Microsoft.Data.ProviderBase.DbConnectionPool pool, System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions options, Microsoft.Data.Common.DbConnectionPoolKey poolKey, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 163  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.CreateObject(System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection) Line 853    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.UserCreateRequest(System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection) Line 2014  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(System.Data.Common.DbConnection owningObject, uint waitForMultipleObjectsTimeout, bool allowCreate, bool onlyOneCheckConnection, Microsoft.Data.Common.DbConnectionOptions userOptions, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 1555   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(System.Data.Common.DbConnection owningObject, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 1302 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionFactory.TryGetConnection(System.Data.Common.DbConnection owningConnection, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 354   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionInternal.TryOpenConnectionInternal(System.Data.Common.DbConnection outerConnection, Microsoft.Data.ProviderBase.DbConnectionFactory connectionFactory, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 759    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.TryOpenInner(System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry) Line 2134 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.TryOpen(System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.SqlClient.SqlConnectionOverrides overrides) Line 2105   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.Open(Microsoft.Data.SqlClient.SqlConnectionOverrides overrides) Line 1658   C#
    MsalTestApp.exe!MsalTestApp.Form1.ConnectButton_Click(object sender, System.EventArgs e) Line 39    C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.OnClick(System.EventArgs e)   Unknown

Meanwhile the MSAL code sees it's running on an MTA thread instead of a UI thread so it tries to spin up its own splash window:


        private Task<bool> ShowPickerWithSplashScreenAsync()
        {

            if (Thread.CurrentThread.GetApartmentState() == ApartmentState.MTA)
            {
                TaskCompletionSource<bool> tcs = new TaskCompletionSource<bool>();

                Thread thread = new Thread(() =>
                {
                    try
                    {
                        ShowPickerWithSplashScreenImpl();
                        tcs.SetResult(true);
                    }
                    catch (Exception e)
                    {
                        tcs.SetException(e);
                    }
                });
                thread.SetApartmentState(ApartmentState.STA);
                thread.Start();
                return tcs.Task;
            }
            else
            {
                ShowPickerWithSplashScreenImpl();
                return Task.FromResult(true);
            }
        }

That thread ends up trying to disable windows on the original UI thread because we gave it the application's window handle. That call deadlocks.

    mscorlib.dll!System.Threading.WaitHandle.InternalWaitOne(System.Runtime.InteropServices.SafeHandle waitableSafeHandle, long millisecondsTimeout, bool hasThreadAffinity, bool exitContext) Line 243 C#
    mscorlib.dll!System.Threading.WaitHandle.WaitOne(int millisecondsTimeout, bool exitContext) Line 194    C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.WaitForWaitHandle(System.Threading.WaitHandle waitHandle) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Control.MarshaledInvoke(System.Windows.Forms.Control caller, System.Delegate method, object[] args, bool synchronous) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Control.Invoke(System.Delegate method, object[] args) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ModalApplicationContext.DisableThreadWindows(bool disable, bool onlyWinForms) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.DisableWindowsForModalLoop(bool onlyWinForms, System.Windows.Forms.ApplicationContext context)  Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.BeginModalMessageLoop(System.Windows.Forms.ApplicationContext context)  Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner(int reason, System.Windows.Forms.ApplicationContext context)    Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.RunMessageLoop(int reason, System.Windows.Forms.ApplicationContext context) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Form.ShowDialog(System.Windows.Forms.IWin32Window owner)  Unknown
    Microsoft.Identity.Client.Desktop.dll!Microsoft.Identity.Client.Platforms.Features.WamBroker.AccountPicker.ShowPickerWithSplashScreenImpl() Line 167    C#
>   Microsoft.Identity.Client.Desktop.dll!Microsoft.Identity.Client.Platforms.Features.WamBroker.AccountPicker.ShowPickerWithSplashScreenAsync.AnonymousMethod__0() Line 130    C#
    mscorlib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 980  C#
    mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 928  C#
    mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state) Line 917    C#
    mscorlib.dll!System.Threading.ThreadHelper.ThreadStart() Line 111   C#

I feel like SqlConnection.Open needs to capture the synchronization context at the time of the call and make sure it doesn't block it while waiting for the AAD callbacks to finish. Maybe we need some way for the application to hand it some special waiter implementation so winforms apps can just process Windows messages.

This bug would likely block adoption of WAM-based auth for SQL connections in SSMS.

shueybubbles commented 2 years ago

There are two scenarios to figure out for complex apps like SSMS

  1. When it calls SqlConnection.Open directly on a UI thread. That may be the app's main thread or it could be a separate STA thread launched to host a dialog.
  2. When it calls SqlConnection.Open on a BackgroundWorker thread or other non-UI thread. Both scenarios are common, and it's extra tricky because often the SqlConnection.Open call is buried inside some library like SMO or DacFx, which knows nothing about UI threads.

I think we would need to add some new interface that a SqlAuthenticationProvider could implement to provide the task await behavior appropriate for the app. That would enable an app to set some state on a thread before creating a SqlConnection and its auth provider could pick up that state and implement an alertable wait to keep messages pumping on the UI thread while token acquisition runs. There may be other solutions and I think it'd be worth some further investigation.

shueybubbles commented 2 years ago

It's not just the WAM broker, the legacy web ui does the same thing.

UI thread:

    mscorlib.dll!System.Threading.ManualResetEventSlim.Wait(int millisecondsTimeout, System.Threading.CancellationToken cancellationToken) Line 688 C#
    mscorlib.dll!System.Threading.Tasks.Task.SpinThenBlockingWait(int millisecondsTimeout, System.Threading.CancellationToken cancellationToken) Line 3320  C#
    mscorlib.dll!System.Threading.Tasks.Task.InternalWait(int millisecondsTimeout, System.Threading.CancellationToken cancellationToken) Line 3259  C#
>   Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.GetFedAuthToken(Microsoft.Data.SqlClient.SqlFedAuthInfo fedAuthInfo) Line 2815   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.OnFedAuthInfo(Microsoft.Data.SqlClient.SqlFedAuthInfo fedAuthInfo) Line 2661 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.TdsParser.TryRun(Microsoft.Data.SqlClient.RunBehavior runBehavior, Microsoft.Data.SqlClient.SqlCommand cmdHandler, Microsoft.Data.SqlClient.SqlDataReader dataStream, Microsoft.Data.SqlClient.BulkCopySimpleResultSet bulkCopyHandler, Microsoft.Data.SqlClient.TdsParserStateObject stateObj, out bool dataReady) Line 2683 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.TdsParser.Run(Microsoft.Data.SqlClient.RunBehavior runBehavior, Microsoft.Data.SqlClient.SqlCommand cmdHandler, Microsoft.Data.SqlClient.SqlDataReader dataStream, Microsoft.Data.SqlClient.BulkCopySimpleResultSet bulkCopyHandler, Microsoft.Data.SqlClient.TdsParserStateObject stateObj) Line 2222    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.CompleteLogin(bool enlistOK) Line 1447   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.AttemptOneLogin(Microsoft.Data.SqlClient.ServerInfo serverInfo, string newPassword, System.Security.SecureString newSecurePassword, bool ignoreSniOpenTimeout, Microsoft.Data.ProviderBase.TimeoutTimer timeout, bool withFailover, bool isFirstTransparentAttempt, bool disableTnir) Line 2320  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.LoginNoFailover(Microsoft.Data.SqlClient.ServerInfo serverInfo, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, Microsoft.Data.ProviderBase.TimeoutTimer timeout) Line 1850  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.OpenLoginEnlist(Microsoft.Data.ProviderBase.TimeoutTimer timeout, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance) Line 1692  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.SqlInternalConnectionTds(Microsoft.Data.ProviderBase.DbConnectionPoolIdentity identity, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, object providerInfo, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance, Microsoft.Data.SqlClient.SqlConnectionString userConnectionOptions, Microsoft.Data.SqlClient.SessionData reconnectSessionData, Microsoft.Data.SqlClient.ServerCertificateValidationCallback serverCallback, Microsoft.Data.SqlClient.ClientCertificateRetrievalCallback clientCallback, Microsoft.Data.ProviderBase.DbConnectionPool pool, string accessToken, Microsoft.Data.SqlClient.SqlClientOriginalNetworkAddressInfo originalNetworkAddressInfo, bool applyTransientFaultHandling) Line 537  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnectionFactory.CreateConnection(Microsoft.Data.Common.DbConnectionOptions options, Microsoft.Data.Common.DbConnectionPoolKey poolKey, object poolGroupProviderInfo, Microsoft.Data.ProviderBase.DbConnectionPool pool, System.Data.Common.DbConnection owningConnection, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 145    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionFactory.CreatePooledConnection(Microsoft.Data.ProviderBase.DbConnectionPool pool, System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions options, Microsoft.Data.Common.DbConnectionPoolKey poolKey, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 163  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.CreateObject(System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection) Line 853    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.UserCreateRequest(System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection) Line 2014  C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(System.Data.Common.DbConnection owningObject, uint waitForMultipleObjectsTimeout, bool allowCreate, bool onlyOneCheckConnection, Microsoft.Data.Common.DbConnectionOptions userOptions, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 1555   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(System.Data.Common.DbConnection owningObject, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 1302 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionFactory.TryGetConnection(System.Data.Common.DbConnection owningConnection, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 354   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionInternal.TryOpenConnectionInternal(System.Data.Common.DbConnection outerConnection, Microsoft.Data.ProviderBase.DbConnectionFactory connectionFactory, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 759    C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.TryOpenInner(System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry) Line 2134 C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.TryOpen(System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.SqlClient.SqlConnectionOverrides overrides) Line 2105   C#
    Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.Open(Microsoft.Data.SqlClient.SqlConnectionOverrides overrides) Line 1658   C#
    MsalTestApp.exe!MsalTestApp.Form1.ConnectButton_Click(object sender, System.EventArgs e) Line 39    C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.OnClick(System.EventArgs e)   Unknown

Background task fetching the token:

    mscorlib.dll!System.Threading.WaitHandle.InternalWaitOne(System.Runtime.InteropServices.SafeHandle waitableSafeHandle, long millisecondsTimeout, bool hasThreadAffinity, bool exitContext) Line 243 C#
    mscorlib.dll!System.Threading.WaitHandle.WaitOne(int millisecondsTimeout, bool exitContext) Line 194    C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.WaitForWaitHandle(System.Threading.WaitHandle waitHandle) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Control.MarshaledInvoke(System.Windows.Forms.Control caller, System.Delegate method, object[] args, bool synchronous) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Control.Invoke(System.Delegate method, object[] args) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ModalApplicationContext.DisableThreadWindows(bool disable, bool onlyWinForms) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.DisableWindowsForModalLoop(bool onlyWinForms, System.Windows.Forms.ApplicationContext context)  Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.BeginModalMessageLoop(System.Windows.Forms.ApplicationContext context)  Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner(int reason, System.Windows.Forms.ApplicationContext context)    Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.RunMessageLoop(int reason, System.Windows.Forms.ApplicationContext context) Unknown
    System.Windows.Forms.dll!System.Windows.Forms.Form.ShowDialog(System.Windows.Forms.IWin32Window owner)  Unknown
>   Microsoft.Identity.Client.dll!Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi.WindowsFormsWebAuthenticationDialog.ShowBrowser.AnonymousMethod__0() Line 63 C#
    Microsoft.Identity.Client.dll!Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi.WindowsFormsWebAuthenticationDialog.ShowBrowser() Line 63    C#
    Microsoft.Identity.Client.dll!Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi.WindowsFormsWebAuthenticationDialog.OnAuthenticate() Line 56 C#
    Microsoft.Identity.Client.dll!Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi.WindowsFormsWebAuthenticationDialogBase.AuthenticateAAD(System.Uri requestUri, System.Uri callbackUri) Line 279  C#
    Microsoft.Identity.Client.dll!Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi.InteractiveWebUI.OnAuthenticate() Line 29    C#
    Microsoft.Identity.Client.dll!Microsoft.Identity.Client.Platforms.Features.WinFormsLegacyWebUi.WebUI.AcquireAuthorizationAsync.AnonymousMethod__0() Line 38 C#
    mscorlib.dll!System.Threading.Tasks.Task.Execute() Line 2498    C#
    mscorlib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 980  C#
    mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 928  C#
    mscorlib.dll!System.Threading.Tasks.Task.ExecuteWithThreadLocal(ref System.Threading.Tasks.Task currentTaskSlot) Line 2827  C#
    mscorlib.dll!System.Threading.Tasks.Task.ExecuteEntry(bool bPreventDoubleExecution) Line 2767   C#
    Microsoft.Identity.Client.dll!Microsoft.Identity.Client.Platforms.Features.DesktopOs.StaTaskScheduler..ctor.AnonymousMethod__2_2() Line 47  C#
    mscorlib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 980  C#
    mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 928  C#
    mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state) Line 917    C#
    mscorlib.dll!System.Threading.ThreadHelper.ThreadStart() Line 111   C#
roji commented 2 years ago

See https://github.com/dotnet/SqlClient/pull/1213#pullrequestreview-731870331 where this was discussed. The code is synchronously blocking on an async call (aka sync-over-async) - a well-known deadlock-producing anti-pattern (see this, this).

I'm not sure if there's an async version of GetFedAuthToken; if so, I'd recommend using that (via OpenAsync) and avoiding this sync-over-async path.

shueybubbles commented 2 years ago

Somehow the custom provider implementation needs an opportunity to call Application.DoEvents on the owner window handle's thread while the token acquisition task is running.

To do this without coupling the driver itself to winforms requires an update to the provider interfaces, afaict.

JRahnama commented 2 years ago

Hi @shueybubbles we will look into this after the release. Thank you.

shueybubbles commented 2 years ago

@JRahnama any update on this ? The MSAL folks are planning to make the window handle parameter mandatory per https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/wam#parent-window-handles

JRahnama commented 1 year ago

@shueybubbles one observation and a question, have you tried setting SetIWin32WindowFunc. Seems like that functionality is provided in the driver for WithParentActivityOrWindow, but since it is passing as null never gets there. I tried setting that in the repro app inside Form1_HandleCreated by

 var app = new ActiveDirectoryAuthenticationProvider(deviceCodeCallback, "<clientID");
app.SetIWin32WindowFunc(<func that returns IWin32Windows>)
SqlAuthenticationProvider.SetProvider(SqlAuthenticationMethod.ActiveDirectoryDeviceCodeFlow, app);

However, the value was always passed as null when created in the Form1_HandleCreated , when I moved the code to ConnectButton_Click right before opening the connection and set the provider there looked like it was working and I was able to get response on the screen. Is it possible for you to give that a try. I will keep investigating the issue next week.

shueybubbles commented 1 year ago

I hadn't noticed that function existed. Our authentication provider for SSMS is registered in the config file. Why would a custom AAD interactive provider call such a function when it can pass a window handle directly to MSAL?

David-Engel commented 1 year ago

I hadn't noticed that function existed. Our authentication provider for SSMS is registered in the config file. Why would a custom AAD interactive provider call such a function when it can pass a window handle directly to MSAL?

SqlClient's provider is public and has some extra APIs on it to allow more scenarios. So, ActiveDirectoryAuthenticationProvider from SqlClient can be used instead of completely implementing your own. But it is registered as a custom provider after setting additional properties.

mdaigle commented 1 month ago

As far as I can tell, the SetIWin32WindowFunc method on ActiveDirectoryAuthenticationProvider is not working the way it's intended when used with forms. Passing a window handle in this manner actually triggers InvalidOperationException: Cross-thread operation not valid. The issue is that down in SqlInternalConnectionTds, when we do the sync-over-async fed auth token acquisition (as called out above), the task may be scheduled to run on a different thread (not the UI thread). So our window handle is passed off to a background thread that is not allowed to interact with it. When I force a sample application to use a single thread, the issue goes away and the interactive auth window is shown.

mdaigle commented 1 month ago

I wonder if we can capture a dispatcher for the UI thread at the time the auth provider is registered. Then we can invoke just the MSAL token acquisition on the UI thread without also forcing the rest of the SQL login handshake onto the UI thread.

My read is that something like this is required, otherwise forms apps will only be able to make sql connections on the UI thread when we do the work to enable WAM.

mdaigle commented 1 month ago

This approach seems to work as long as long as the open is async (sync still seems to give a deadlock): https://github.com/dotnet/SqlClient/pull/2884/files#diff-7b20b6d8d7c505871ab9d3762e51086d6848bae49bf6864f8e5b5adfc3070846R320

Would love any feedback on this approach as I investigate further @shueybubbles @roji @cheenamalhotra If this general idea looks good, I can look into the best way to get the Control injected into the auth provider. Thanks!

I'm using a simple form app to repro

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
        Button connectButton = new Button();
        this.Controls.Add(connectButton);
        connectButton.Text = "connect";
        connectButton.Click += connectButton_Click;
    }

    private async void connectButton_Click(object sender, EventArgs eventArgs)
    {

        if (this.InvokeRequired)
        {
            var t = new string[0];
        }
        string AdoClientId = "2fd908ad-0664-4344-b9be-cd3e8b574c38";

        var provider = new ActiveDirectoryAuthenticationProvider(AdoClientId);
        provider.SetIWin32WindowFunc(this);

        SqlAuthenticationProvider.SetProvider(
            SqlAuthenticationMethod.ActiveDirectoryInteractive,
            provider
        );

        using (SqlConnection sqlConnection = new SqlConnection(new SqlConnectionStringBuilder()
        {
            DataSource = "malcolm-test.database.windows.net,1433",
            InitialCatalog = "Northwind",
            Authentication = SqlAuthenticationMethod.ActiveDirectoryInteractive,
            Pooling = false
        }.ConnectionString))
        {
            await sqlConnection.OpenAsync();
        }

        Console.Out.WriteLine("Connected successfully!");

    }
}

Also including a zip here: WindowsFormsAppWAMExample.zip