Custom AAD token provider in Winforms can still deadlock in GetFedAuthInfo #1630
Comments
|
There are two scenarios to figure out for complex apps like SSMS
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 |
shueybubbles
changed the title
|
It's not just the WAM broker, the legacy web ui does the same thing. UI thread: Background task fetching the token: |
|
See #1213 (review) 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. |
|
Somehow the custom provider implementation needs an opportunity to call To do this without coupling the driver itself to winforms requires an update to the provider interfaces, afaict. |
|
Hi @shueybubbles we will look into this after the release. Thank you. |
|
@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 |
|
@shueybubbles one observation and a question, have you tried setting SetIWin32WindowFunc. Seems like that functionality is provided in the driver for 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 |
|
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. |
|
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 |
|
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. |
|
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 I'm using a simple form app to repro Also including a zip here: |
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:
The UI thread gets here:
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:
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.
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.
The text was updated successfully, but these errors were encountered: