WAM POC [updated] #3874
Closed
WAM POC [updated] #3874
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
This PR is the successor of #2884 |
Closed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a proof-of-concept (POC) for Windows Authentication Manager (WAM) broker support in the Active Directory authentication provider. The changes aim to enable broker-based authentication through the Microsoft.Identity.Client.Broker package.
Key changes:
- Adds Microsoft.Identity.Client.Broker package dependency across all project configurations
- Introduces INTERACTIVE_AUTH compilation constant to conditionally enable broker authentication features
- Refactors authentication method handling from if-else chains to a switch statement
- Adds platform-specific partial classes for Windows and Unix implementations with parent window/activity handling
- Implements SynchronizationContext-based threading for interactive authentication flows
- Adds Windows interop methods for console window retrieval (GetConsoleWindow, GetAncestor)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| ActiveDirectoryAuthenticationProvider.cs | Refactors authentication flow to switch statement; adds SynchronizationContext support; introduces new async void overload for interactive auth; contains critical syntax errors |
| ActiveDirectoryAuthenticationProvider.Windows.cs | Platform-specific implementation for Windows with parent window handling and console window retrieval fallback |
| ActiveDirectoryAuthenticationProvider.Unix.cs | Platform-specific implementation for Unix with minimal parent activity support |
| Interop.GetAncestor.cs | Windows interop for retrieving ancestor window handles |
| Interop.GetConsoleWindow.cs | Windows interop for retrieving console window handle |
| netfx/src/Microsoft.Data.SqlClient.csproj | Adds INTERACTIVE_AUTH constant for Debug builds and broker package reference |
| netfx/ref/Microsoft.Data.SqlClient.csproj | Adds broker package reference to reference assembly |
| netcore/src/Microsoft.Data.SqlClient.csproj | Adds broker package reference but missing INTERACTIVE_AUTH constant configuration |
| netcore/ref/Microsoft.Data.SqlClient.csproj | Adds broker package reference to reference assembly |
| Directory.Packages.props | Adds Microsoft.Identity.Client.Broker version 4.78.0 |
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:125
- The IsSupported method has been removed but its method body (lines 114-124) remains orphaned after the SetDeviceCodeFlowCallback method. These lines need to be either removed or properly placed within a method declaration. The removed method signature at line 113 indicates SetAcquireAuthorizationCodeAsyncCallback and IsSupported were deleted but their code wasn't cleanly removed.
return authentication == SqlAuthenticationMethod.ActiveDirectoryIntegrated
#pragma warning disable 0618 // Type or member is obsolete
|| authentication == SqlAuthenticationMethod.ActiveDirectoryPassword
#pragma warning restore 0618 // Type or member is obsolete
|| authentication == SqlAuthenticationMethod.ActiveDirectoryInteractive
|| authentication == SqlAuthenticationMethod.ActiveDirectoryServicePrincipal
|| authentication == SqlAuthenticationMethod.ActiveDirectoryDeviceCodeFlow
|| authentication == SqlAuthenticationMethod.ActiveDirectoryManagedIdentity
|| authentication == SqlAuthenticationMethod.ActiveDirectoryMSI
|| authentication == SqlAuthenticationMethod.ActiveDirectoryDefault
|| authentication == SqlAuthenticationMethod.ActiveDirectoryWorkloadIdentity;
}
| internal class PublicClientAppKey | ||
| { | ||
| public readonly string _authority; | ||
| public readonly string _redirectUri; |
There was a problem hiding this comment.
The PublicClientAppKey class is missing the _applicationClientId field declaration. The field is used in the constructor (line 872) and Equals method (line 882) but is not declared as a class member. Add 'public readonly string _applicationClientId;' field declaration after line 863.
Suggested change
| public readonly string _redirectUri; | |
| public readonly string _redirectUri; | |
| public readonly string _applicationClientId; |
Comment on lines
+32
to
+34
| /// | ||
| /// </summary> | ||
| /// <param name="parentActivityOrWindowFunc"></param> |
There was a problem hiding this comment.
The SetParentActivityOrWindow method is missing documentation. Add XML documentation comments to explain the purpose of this method, the parentActivityOrWindowFunc parameter, and when this should be used in the authentication flow.
Suggested change
| /// | |
| /// </summary> | |
| /// <param name="parentActivityOrWindowFunc"></param> | |
| /// Sets a callback that returns the parent activity or window to use for interactive authentication dialogs. | |
| /// </summary> | |
| /// <param name="parentActivityOrWindowFunc"> | |
| /// A delegate that returns an object representing the parent activity or window (for example, | |
| /// a <see cref="System.Windows.Forms.IWin32Window"/> or a platform-specific window handle). | |
| /// If this delegate is not provided, the provider falls back to using the console or terminal window handle. | |
| /// </param> | |
| /// <remarks> | |
| /// Call this method before initiating an Azure Active Directory authentication flow that may display UI, | |
| /// so that any interactive dialogs are correctly parented to the specified application window or activity. | |
| /// This is typically used in desktop or GUI applications that host interactive authentication experiences. | |
| /// </remarks> |
Comment on lines
+284
to
+419
| case SqlAuthenticationMethod.ActiveDirectoryInteractive: | ||
| case SqlAuthenticationMethod.ActiveDirectoryDeviceCodeFlow: | ||
| case SqlAuthenticationMethod.ActiveDirectoryIntegrated: | ||
| { | ||
| entry.Value = GetHash(parameters.Password); | ||
| entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromHours(s_accountPwCacheTtlInHours); | ||
| PublicClientAppKey pcaKey = new(parameters.Authority, redirectUri, _applicationClientId, ParentActivityOrWindow); | ||
| IPublicClientApplication app = await GetPublicClientAppInstanceAsync(pcaKey, cts.Token).ConfigureAwait(false); | ||
|
|
||
| AuthenticationResult result = null; | ||
|
|
||
| try | ||
| { | ||
| result = await TryAcquireTokenSilent(app, parameters, scopes, cts).ConfigureAwait(false); | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (silent) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
| catch (MsalUiRequiredException) | ||
| { | ||
| // 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. | ||
|
|
||
| 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. | ||
| 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); | ||
| } | ||
|
|
||
| return new SqlAuthenticationToken(result.AccessToken, result.ExpiresOn); | ||
| } | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token for Active Directory Password auth mode. Expiry Time: {0}", result?.ExpiresOn); | ||
| } | ||
| } | ||
| else if (parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryInteractive || | ||
| parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryDeviceCodeFlow) | ||
| { | ||
| try | ||
| { | ||
| result = await TryAcquireTokenSilent(app, parameters, scopes, cts).ConfigureAwait(false); | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (silent) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
| catch (MsalUiRequiredException) | ||
| { | ||
| // 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); | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
| #endif | ||
| default: | ||
| { | ||
| PublicClientAppKey pcaKey = new(parameters.Authority, redirectUri, _applicationClientId, ParentActivityOrWindow); | ||
| IPublicClientApplication app = await GetPublicClientAppInstanceAsync(pcaKey, cts.Token).ConfigureAwait(false); | ||
| AuthenticationResult result = null; | ||
|
|
||
| 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); | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | {0} authentication mode not supported by ActiveDirectoryAuthenticationProvider class.", parameters.AuthenticationMethod); | ||
| throw SQL.UnsupportedAuthenticationSpecified(parameters.AuthenticationMethod); | ||
| } | ||
| if (parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryIntegrated) | ||
| { | ||
| result = await TryAcquireTokenSilent(app, parameters, scopes, cts).ConfigureAwait(false); | ||
|
|
||
| if (result == null) | ||
| { | ||
| if (!string.IsNullOrEmpty(parameters.UserId)) | ||
| { | ||
| // The AcquireTokenByIntegratedWindowsAuth method is marked as obsolete in MSAL.NET | ||
| // but it is still a supported way to acquire tokens for Active Directory Integrated authentication. | ||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| result = await app.AcquireTokenByIntegratedWindowsAuth(scopes) | ||
| #pragma warning restore CS0618 // Type or member is obsolete | ||
| .WithCorrelationId(parameters.ConnectionId) | ||
| .WithUsername(parameters.UserId) | ||
| .ExecuteAsync(cancellationToken: cts.Token) | ||
| .ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| result = await app.AcquireTokenByIntegratedWindowsAuth(scopes) | ||
| #pragma warning restore CS0618 // Type or member is obsolete | ||
| .WithCorrelationId(parameters.ConnectionId) | ||
| .ExecuteAsync(cancellationToken: cts.Token) | ||
| .ConfigureAwait(false); | ||
| } | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token for Active Directory Integrated auth mode. Expiry Time: {0}", result?.ExpiresOn); | ||
| } | ||
| } | ||
| else if (parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryInteractive || | ||
| parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryDeviceCodeFlow) | ||
| { | ||
| try | ||
| { | ||
| result = await TryAcquireTokenSilent(app, parameters, scopes, cts).ConfigureAwait(false); | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (silent) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
| catch (MsalUiRequiredException) | ||
| { | ||
| // 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); | ||
| 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); | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result?.ExpiresOn); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | {0} authentication mode not supported by ActiveDirectoryAuthenticationProvider class.", parameters.AuthenticationMethod); | ||
| throw SQL.UnsupportedAuthenticationSpecified(parameters.AuthenticationMethod); | ||
| } | ||
|
|
||
| return new SqlAuthenticationToken(result.AccessToken, result.ExpiresOn); | ||
| return new SqlAuthenticationToken(result.AccessToken, result.ExpiresOn); | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between the INTERACTIVE_AUTH conditional section (lines 284-350) and the default case (lines 352-419). The ActiveDirectoryIntegrated, ActiveDirectoryInteractive, and ActiveDirectoryDeviceCodeFlow authentication methods are handled in both places. When INTERACTIVE_AUTH is defined, these methods will be handled in the specific case statement, but when it's not defined, they'll fall through to the default case. This creates maintenance burden and potential for bugs if changes are made to only one location.
Comment on lines
+478
to
+565
| private static async void AcquireTokenInteractiveDeviceFlowAsync(object state) | ||
| { | ||
| InteractiveAuthStateObject interactiveAuthStateObject = (InteractiveAuthStateObject)state; | ||
|
|
||
| try | ||
| { | ||
| if (interactiveAuthStateObject.authenticationMethod == SqlAuthenticationMethod.ActiveDirectoryInteractive) | ||
| { | ||
| CancellationTokenSource ctsInteractive = new(); | ||
| #if NET | ||
| /* | ||
| * On .NET Core, MSAL will start the system browser as a separate process. MSAL does not have control over this browser, | ||
| * but once the user finishes authentication, the web page is redirected in such a way that MSAL can intercept the Uri. | ||
| * MSAL cannot detect if the user navigates away or simply closes the browser. Apps using this technique are encouraged | ||
| * to define a timeout (via CancellationToken). We recommend a timeout of at least a few minutes, to take into account | ||
| * cases where the user is prompted to change password or perform 2FA. | ||
| * | ||
| * https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/System-Browser-on-.Net-Core#system-browser-experience | ||
| */ | ||
| ctsInteractive.CancelAfter(180000); | ||
| #endif | ||
|
|
||
| if (interactiveAuthStateObject.customWebUI != null) | ||
| { | ||
| var result = await interactiveAuthStateObject.app.AcquireTokenInteractive(interactiveAuthStateObject.scopes) | ||
| .WithCorrelationId(interactiveAuthStateObject.connectionId) | ||
| .WithCustomWebUi(interactiveAuthStateObject.customWebUI) | ||
| .WithLoginHint(interactiveAuthStateObject.userId) | ||
| .ExecuteAsync(ctsInteractive.Token) | ||
| .ConfigureAwait(false); | ||
| interactiveAuthStateObject._taskCompletionSource.SetResult(result); | ||
| return; | ||
| } | ||
| else | ||
| { | ||
| /* | ||
| * We will use the MSAL Embedded or System web browser which changes by Default in MSAL according to this table: | ||
| * | ||
| * Framework Embedded System Default | ||
| * ------------------------------------------- | ||
| * .NET Classic Yes Yes^ Embedded | ||
| * .NET Core No Yes^ System | ||
| * .NET Standard No No NONE | ||
| * UWP Yes No Embedded | ||
| * Xamarin.Android Yes Yes System | ||
| * Xamarin.iOS Yes Yes System | ||
| * Xamarin.Mac Yes No Embedded | ||
| * | ||
| * ^ Requires "http://localhost" redirect URI | ||
| * | ||
| * https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/MSAL.NET-uses-web-browser#at-a-glance | ||
| */ | ||
| var result = await interactiveAuthStateObject.app.AcquireTokenInteractive(interactiveAuthStateObject.scopes) | ||
| .WithCorrelationId(interactiveAuthStateObject.connectionId) | ||
| .WithLoginHint(interactiveAuthStateObject.userId) | ||
| .ExecuteAsync(ctsInteractive.Token) | ||
| .ConfigureAwait(false); | ||
| interactiveAuthStateObject._taskCompletionSource.SetResult(result); | ||
| return; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| AuthenticationResult result = await interactiveAuthStateObject.app.AcquireTokenWithDeviceCode(interactiveAuthStateObject.scopes, | ||
| deviceCodeResult => interactiveAuthStateObject.deviceCodeFlowCallback(deviceCodeResult)) | ||
| .WithCorrelationId(interactiveAuthStateObject.connectionId) | ||
| .ExecuteAsync(cancellationToken: interactiveAuthStateObject.cts.Token) | ||
| .ConfigureAwait(false); | ||
| interactiveAuthStateObject._taskCompletionSource.SetResult(result); | ||
| return; | ||
| } | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenInteractiveDeviceFlowAsync | Operation timed out while acquiring access token."); | ||
|
|
||
| var error = (interactiveAuthStateObject.authenticationMethod == SqlAuthenticationMethod.ActiveDirectoryInteractive) ? | ||
| SQL.ActiveDirectoryInteractiveTimeout() : | ||
| SQL.ActiveDirectoryDeviceFlowTimeout(); | ||
|
|
||
| interactiveAuthStateObject._taskCompletionSource.SetException(error); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenInteractiveDeviceFlowAsync | Operation failed while acquiring access token."); | ||
| interactiveAuthStateObject._taskCompletionSource.SetException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The method AcquireTokenInteractiveDeviceFlowAsync uses the 'async void' pattern, which is generally discouraged outside of event handlers. While the TaskCompletionSource pattern is used to propagate results and exceptions, 'async void' methods make exception handling more complex and can lead to application crashes if unhandled exceptions occur. Consider using 'async Task' with proper awaiting or ensure comprehensive exception handling is in place.
Comment on lines
1084
to
1088
| <PackageReference Include="Microsoft.Bcl.Cryptography" /> | ||
| <PackageReference Include="Microsoft.Data.SqlClient.SNI.runtime" /> | ||
| <PackageReference Include="Microsoft.Extensions.Caching.Memory" /> | ||
| <PackageReference Include="Microsoft.Identity.Client.Broker"/> | ||
| <PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" /> |
There was a problem hiding this comment.
The INTERACTIVE_AUTH compilation constant is defined for Debug builds in the netfx project but is not defined in the netcore project at all. This means that interactive authentication with broker support will only work in .NET Framework Debug builds or when InteractiveAuthEnabled is explicitly set to true. The netcore project needs a similar conditional compilation setup to enable broker authentication support, otherwise the new broker authentication code will never be compiled for .NET Core/NET builds.
|
|
||
| if (result == null) | ||
| { | ||
| //TODO: need to use broker here too? |
There was a problem hiding this comment.
There's a comment "TODO: need to use broker here too?" at line 261 in the ActiveDirectoryPassword authentication case. This indicates incomplete implementation. Clarify whether broker authentication should be used for password-based authentication flows and either implement it or remove the TODO comment with an explanation of why it's not needed.
Comment on lines
+318
to
+320
| SynchronizationContext.Post(AcquireTokenInteractiveDeviceFlowAsync, state); | ||
|
|
||
| result = await state._taskCompletionSource.Task; |
There was a problem hiding this comment.
Potential null reference exception when awaiting state._taskCompletionSource.Task. If AcquireTokenInteractiveDeviceFlowAsync encounters an exception that is not caught or doesn't call SetResult/SetException on the TaskCompletionSource, the await will hang indefinitely. Consider adding a timeout or ensuring all code paths in AcquireTokenInteractiveDeviceFlowAsync properly complete the task.
Comment on lines
+478
to
+565
| private static async void AcquireTokenInteractiveDeviceFlowAsync(object state) | ||
| { | ||
| InteractiveAuthStateObject interactiveAuthStateObject = (InteractiveAuthStateObject)state; | ||
|
|
||
| try | ||
| { | ||
| if (interactiveAuthStateObject.authenticationMethod == SqlAuthenticationMethod.ActiveDirectoryInteractive) | ||
| { | ||
| CancellationTokenSource ctsInteractive = new(); | ||
| #if NET | ||
| /* | ||
| * On .NET Core, MSAL will start the system browser as a separate process. MSAL does not have control over this browser, | ||
| * but once the user finishes authentication, the web page is redirected in such a way that MSAL can intercept the Uri. | ||
| * MSAL cannot detect if the user navigates away or simply closes the browser. Apps using this technique are encouraged | ||
| * to define a timeout (via CancellationToken). We recommend a timeout of at least a few minutes, to take into account | ||
| * cases where the user is prompted to change password or perform 2FA. | ||
| * | ||
| * https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/System-Browser-on-.Net-Core#system-browser-experience | ||
| */ | ||
| ctsInteractive.CancelAfter(180000); | ||
| #endif | ||
|
|
||
| if (interactiveAuthStateObject.customWebUI != null) | ||
| { | ||
| var result = await interactiveAuthStateObject.app.AcquireTokenInteractive(interactiveAuthStateObject.scopes) | ||
| .WithCorrelationId(interactiveAuthStateObject.connectionId) | ||
| .WithCustomWebUi(interactiveAuthStateObject.customWebUI) | ||
| .WithLoginHint(interactiveAuthStateObject.userId) | ||
| .ExecuteAsync(ctsInteractive.Token) | ||
| .ConfigureAwait(false); | ||
| interactiveAuthStateObject._taskCompletionSource.SetResult(result); | ||
| return; | ||
| } | ||
| else | ||
| { | ||
| /* | ||
| * We will use the MSAL Embedded or System web browser which changes by Default in MSAL according to this table: | ||
| * | ||
| * Framework Embedded System Default | ||
| * ------------------------------------------- | ||
| * .NET Classic Yes Yes^ Embedded | ||
| * .NET Core No Yes^ System | ||
| * .NET Standard No No NONE | ||
| * UWP Yes No Embedded | ||
| * Xamarin.Android Yes Yes System | ||
| * Xamarin.iOS Yes Yes System | ||
| * Xamarin.Mac Yes No Embedded | ||
| * | ||
| * ^ Requires "http://localhost" redirect URI | ||
| * | ||
| * https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/MSAL.NET-uses-web-browser#at-a-glance | ||
| */ | ||
| var result = await interactiveAuthStateObject.app.AcquireTokenInteractive(interactiveAuthStateObject.scopes) | ||
| .WithCorrelationId(interactiveAuthStateObject.connectionId) | ||
| .WithLoginHint(interactiveAuthStateObject.userId) | ||
| .ExecuteAsync(ctsInteractive.Token) | ||
| .ConfigureAwait(false); | ||
| interactiveAuthStateObject._taskCompletionSource.SetResult(result); | ||
| return; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| AuthenticationResult result = await interactiveAuthStateObject.app.AcquireTokenWithDeviceCode(interactiveAuthStateObject.scopes, | ||
| deviceCodeResult => interactiveAuthStateObject.deviceCodeFlowCallback(deviceCodeResult)) | ||
| .WithCorrelationId(interactiveAuthStateObject.connectionId) | ||
| .ExecuteAsync(cancellationToken: interactiveAuthStateObject.cts.Token) | ||
| .ConfigureAwait(false); | ||
| interactiveAuthStateObject._taskCompletionSource.SetResult(result); | ||
| return; | ||
| } | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenInteractiveDeviceFlowAsync | Operation timed out while acquiring access token."); | ||
|
|
||
| var error = (interactiveAuthStateObject.authenticationMethod == SqlAuthenticationMethod.ActiveDirectoryInteractive) ? | ||
| SQL.ActiveDirectoryInteractiveTimeout() : | ||
| SQL.ActiveDirectoryDeviceFlowTimeout(); | ||
|
|
||
| interactiveAuthStateObject._taskCompletionSource.SetException(error); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| SqlClientEventSource.Log.TryTraceEvent("AcquireTokenInteractiveDeviceFlowAsync | Operation failed while acquiring access token."); | ||
| interactiveAuthStateObject._taskCompletionSource.SetException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between the new AcquireTokenInteractiveDeviceFlowAsync method (lines 478-565) and the existing async Task version (lines 568+). Both methods contain nearly identical logic for handling interactive and device code flow authentication. Consider refactoring to extract the common logic into a shared helper method to improve maintainability and reduce the risk of inconsistencies.
| /// <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)] | ||
| internal static extern IntPtr GetAncestor(IntPtr hwnd, GetAncestorFlags flags); |
There was a problem hiding this comment.
Minimise the use of unmanaged code.
| internal partial class Kernel32 | ||
| { | ||
| [DllImport("kernel32.dll")] | ||
| internal static extern IntPtr GetConsoleWindow(); |
There was a problem hiding this comment.
Minimise the use of unmanaged code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Provide a summary of the changes being introduced. Important topics to cover
include:
High quality descriptions will lead to a smoother review experience.
Issues
Link to any relevant issues, bugs, or discussions (e.g.,
Closes #123,Fixes issue #456).Testing
Describe the automated tests (unit, integration) you created or modified.
Provide justification for any gap in automated testing. List any manual testing
steps that were performed to ensure the changes work.
Guidelines
Please review the contribution guidelines before submitting a pull request: