From f2ba8a3bcffe3b3ed9a91bc951b1af7921601393 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Thu, 22 Oct 2020 12:51:02 -0700 Subject: [PATCH 1/6] Implement public client application global cache --- .../ActiveDirectoryAuthenticationProvider.cs | 216 +++++++++++++++--- 1 file changed, 178 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs index 507d34b103..ac86212e0e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Concurrent; using System.Linq; using System.Security; using System.Threading; @@ -15,6 +16,9 @@ namespace Microsoft.Data.SqlClient /// public sealed class ActiveDirectoryAuthenticationProvider : SqlAuthenticationProvider { + private static ConcurrentDictionary s_pcaMap + = new ConcurrentDictionary(); + private static readonly string s_nativeClientRedirectUri = "https://login.microsoftonline.com/common/oauth2/nativeclient"; private static readonly string s_defaultScopeSuffix = "/.default"; private readonly string _type = typeof(ActiveDirectoryAuthenticationProvider).Name; private readonly SqlClientLogger _logger = new SqlClientLogger(); @@ -67,10 +71,10 @@ public override void BeforeUnload(SqlAuthenticationMethod authentication) } #if NETSTANDARD - private Func parentActivityOrWindowFunc = null; + private Func _parentActivityOrWindowFunc = null; /// - public void SetParentActivityOrWindowFunc(Func parentActivityOrWindowFunc) => this.parentActivityOrWindowFunc = parentActivityOrWindowFunc; + public void SetParentActivityOrWindowFunc(Func parentActivityOrWindowFunc) => this._parentActivityOrWindowFunc = parentActivityOrWindowFunc; #endif #if NETFRAMEWORK @@ -108,51 +112,24 @@ public override Task AcquireTokenAsync(SqlAuthentication * * https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-app-registration#redirect-uris */ - string redirectURI = "https://login.microsoftonline.com/common/oauth2/nativeclient"; + string redirectUri = s_nativeClientRedirectUri; #if NETCOREAPP if (parameters.AuthenticationMethod != SqlAuthenticationMethod.ActiveDirectoryDeviceCodeFlow) { - redirectURI = "http://localhost"; - } -#endif - IPublicClientApplication app; - -#if NETSTANDARD - if (parentActivityOrWindowFunc != null) - { - app = PublicClientApplicationBuilder.Create(_applicationClientId) - .WithAuthority(parameters.Authority) - .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) - .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) - .WithRedirectUri(redirectURI) - .WithParentActivityOrWindow(parentActivityOrWindowFunc) - .Build(); + redirectUri = "http://localhost"; } #endif + PublicClientAppKey pcaKey = new PublicClientAppKey(parameters.Authority, redirectUri, _applicationClientId #if NETFRAMEWORK - if (_iWin32WindowFunc != null) - { - app = PublicClientApplicationBuilder.Create(_applicationClientId) - .WithAuthority(parameters.Authority) - .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) - .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) - .WithRedirectUri(redirectURI) - .WithParentActivityOrWindow(_iWin32WindowFunc) - .Build(); - } + , _iWin32WindowFunc #endif -#if !NETCOREAPP - else +#if NETSTANDARD + , _parentActivityOrWindowFunc #endif - { - app = PublicClientApplicationBuilder.Create(_applicationClientId) - .WithAuthority(parameters.Authority) - .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) - .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) - .WithRedirectUri(redirectURI) - .Build(); - } + ); + + IPublicClientApplication app = GetPublicClientAppInstance(pcaKey); if (parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryIntegrated) { @@ -320,5 +297,168 @@ private class CustomWebUi : ICustomWebUi public Task AcquireAuthorizationCodeAsync(Uri authorizationUri, Uri redirectUri, CancellationToken cancellationToken) => _acquireAuthorizationCodeAsyncCallback.Invoke(authorizationUri, redirectUri, cancellationToken); } + + private IPublicClientApplication GetPublicClientAppInstance(PublicClientAppKey publicClientAppKey) + { + IPublicClientApplication clientApplicationInstance; + + if (s_pcaMap.ContainsKey(publicClientAppKey)) + { + s_pcaMap.TryGetValue(publicClientAppKey, out clientApplicationInstance); + } + else + { + clientApplicationInstance = CreateClientAppInstance(publicClientAppKey); + s_pcaMap.TryAdd(publicClientAppKey, clientApplicationInstance); + } + return clientApplicationInstance; + } + + private IPublicClientApplication CreateClientAppInstance(PublicClientAppKey publicClientAppKey) + { + IPublicClientApplication publicClientApplication; + +#if NETSTANDARD + if (_parentActivityOrWindowFunc != null) + { + publicClientApplication = PublicClientApplicationBuilder.Create(publicClientAppKey._applicationClientId) + .WithAuthority(publicClientAppKey._authority) + .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) + .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) + .WithRedirectUri(publicClientAppKey._redirectUri) + .WithParentActivityOrWindow(_parentActivityOrWindowFunc) + .Build(); + } +#endif +#if NETFRAMEWORK + if (_iWin32WindowFunc != null) + { + publicClientApplication = PublicClientApplicationBuilder.Create(publicClientAppKey._applicationClientId) + .WithAuthority(publicClientAppKey._authority) + .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) + .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) + .WithRedirectUri(publicClientAppKey._redirectUri) + .WithParentActivityOrWindow(_iWin32WindowFunc) + .Build(); + } +#endif +#if !NETCOREAPP + else +#endif + { + publicClientApplication = PublicClientApplicationBuilder.Create(publicClientAppKey._applicationClientId) + .WithAuthority(publicClientAppKey._authority) + .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) + .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) + .WithRedirectUri(publicClientAppKey._redirectUri) + .Build(); + } + + return publicClientApplication; + } + + internal class PublicClientAppKey + { + public readonly string _authority; + public readonly string _redirectUri; + public readonly string _applicationClientId; +#if NETFRAMEWORK + public readonly Func _iWin32WindowFunc; +#endif +#if NETSTANDARD + public readonly Func _parentActivityOrWindowFunc; +#endif + private int _hashValue; + + public PublicClientAppKey(string authority, string redirectUri, string applicationClientId +#if NETFRAMEWORK + , Func iWin32WindowFunc +#endif +#if NETSTANDARD + , Func parentActivityOrWindowFunc +#endif + ) + { + _authority = authority; + _redirectUri = redirectUri; + _applicationClientId = applicationClientId; +#if NETFRAMEWORK + _iWin32WindowFunc = iWin32WindowFunc; +#endif +#if NETSTANDARD + _parentActivityOrWindowFunc = parentActivityOrWindowFunc; +#endif + } + + public override bool Equals(object obj) + { + if (obj == null) + { + return false; + } + + PublicClientAppKey pcaKey = obj as PublicClientAppKey; + return (string.CompareOrdinal(_authority, pcaKey._authority) == 0 + && string.CompareOrdinal(_redirectUri, pcaKey._redirectUri) == 0 + && string.CompareOrdinal(_applicationClientId, pcaKey._applicationClientId) == 0 +#if NETFRAMEWORK + && pcaKey._iWin32WindowFunc == _iWin32WindowFunc +#endif +#if NETSTANDARD + && pcaKey._parentActivityOrWindowFunc == _parentActivityOrWindowFunc +#endif + ); + } + + public override int GetHashCode() + { + return _hashValue; + } + + private void CalculateHashCode() + { + _hashValue = base.GetHashCode(); + + if (_authority != null) + { + unchecked + { + _hashValue = _hashValue * 17 + _authority.GetHashCode(); + } + } + if (_redirectUri != null) + { + unchecked + { + _hashValue = _hashValue * 17 + _redirectUri.GetHashCode(); + } + } + if (_applicationClientId != null) + { + unchecked + { + _hashValue = _hashValue * 17 + _applicationClientId.GetHashCode(); + } + } +#if NETFRAMEWORK + if (_iWin32WindowFunc != null) + { + unchecked + { + _hashValue = _hashValue * 17 + _iWin32WindowFunc.GetHashCode(); + } + } +#endif +#if NETSTANDARD + if (_parentActivityOrWindowFunc != null) + { + unchecked + { + _hashValue = _hashValue * 17 + _parentActivityOrWindowFunc.GetHashCode(); + } + } +#endif + } + } } } From 9638ad8ac0dfe0663f924c719b8b2cd80d4db9c1 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 23 Oct 2020 13:11:11 -0700 Subject: [PATCH 2/6] Add comments + modify Equals --- .../ActiveDirectoryAuthenticationProvider.cs | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs index ac86212e0e..9c9b20bc00 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs @@ -16,6 +16,11 @@ namespace Microsoft.Data.SqlClient /// public sealed class ActiveDirectoryAuthenticationProvider : SqlAuthenticationProvider { + /// + /// This is a static cache instance meant to hold instances of "PublicClientApplication" mapping to information available in PublicClientAppKey. + /// The purpose of this cache is to allow re-use of Access Tokens fetched for a user interactively or with any other mode + /// to avoid interactive authentication request every-time, within application scope making use of MSAL's userTokenCache. + /// private static ConcurrentDictionary s_pcaMap = new ConcurrentDictionary(); private static readonly string s_nativeClientRedirectUri = "https://login.microsoftonline.com/common/oauth2/nativeclient"; @@ -162,6 +167,7 @@ public override Task AcquireTokenAsync(SqlAuthentication else if (parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryInteractive || parameters.AuthenticationMethod == SqlAuthenticationMethod.ActiveDirectoryDeviceCodeFlow) { + // Fetch available accounts from 'app' instance System.Collections.Generic.IEnumerable accounts = await app.GetAccountsAsync(); IAccount account; if (!string.IsNullOrEmpty(parameters.UserId)) @@ -177,17 +183,23 @@ public override Task AcquireTokenAsync(SqlAuthentication { try { + // If 'account' is available in 'app', we use the same to acquire token silently. + // Read More on API docs: https://docs.microsoft.com/dotnet/api/microsoft.identity.client.clientapplicationbase.acquiretokensilent result = await app.AcquireTokenSilent(scopes, account).ExecuteAsync(); 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); SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result.ExpiresOn); } } else { + // If no existing 'account' is found, we request user to sign in interactively. result = await AcquireTokenInteractiveDeviceFlowAsync(app, scopes, parameters.ConnectionId, parameters.UserId, parameters.AuthenticationMethod); SqlClientEventSource.Log.TryTraceEvent("AcquireTokenAsync | Acquired access token (interactive) for {0} auth mode. Expiry Time: {1}", parameters.AuthenticationMethod, result.ExpiresOn); } @@ -392,22 +404,20 @@ public PublicClientAppKey(string authority, string redirectUri, string applicati public override bool Equals(object obj) { - if (obj == null) + if (obj != null && obj is PublicClientAppKey pcaKey) { - return false; - } - - PublicClientAppKey pcaKey = obj as PublicClientAppKey; - return (string.CompareOrdinal(_authority, pcaKey._authority) == 0 - && string.CompareOrdinal(_redirectUri, pcaKey._redirectUri) == 0 - && string.CompareOrdinal(_applicationClientId, pcaKey._applicationClientId) == 0 + return (string.CompareOrdinal(_authority, pcaKey._authority) == 0 + && string.CompareOrdinal(_redirectUri, pcaKey._redirectUri) == 0 + && string.CompareOrdinal(_applicationClientId, pcaKey._applicationClientId) == 0 #if NETFRAMEWORK - && pcaKey._iWin32WindowFunc == _iWin32WindowFunc + && pcaKey._iWin32WindowFunc == _iWin32WindowFunc #endif #if NETSTANDARD - && pcaKey._parentActivityOrWindowFunc == _parentActivityOrWindowFunc + && pcaKey._parentActivityOrWindowFunc == _parentActivityOrWindowFunc #endif - ); + ); + } + return false; } public override int GetHashCode() From 5c772bf6a651f9b292a928b0a0e4e84f9bf6a4ac Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 23 Oct 2020 13:46:23 -0700 Subject: [PATCH 3/6] Address review feedback --- .../SqlClient/ActiveDirectoryAuthenticationProvider.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs index 9c9b20bc00..9acf7e0ab1 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs @@ -312,13 +312,7 @@ public Task AcquireAuthorizationCodeAsync(Uri authorizationUri, Uri redirec private IPublicClientApplication GetPublicClientAppInstance(PublicClientAppKey publicClientAppKey) { - IPublicClientApplication clientApplicationInstance; - - if (s_pcaMap.ContainsKey(publicClientAppKey)) - { - s_pcaMap.TryGetValue(publicClientAppKey, out clientApplicationInstance); - } - else + if (!s_pcaMap.TryGetValue(publicClientAppKey, out IPublicClientApplication clientApplicationInstance)) { clientApplicationInstance = CreateClientAppInstance(publicClientAppKey); s_pcaMap.TryAdd(publicClientAppKey, clientApplicationInstance); From db8ff3f66dbdcb2d6f2b72aefceaa4cacf078ebb Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 23 Oct 2020 14:04:45 -0700 Subject: [PATCH 4/6] calculate hashcode --- .../Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs index 9acf7e0ab1..5548246ca6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs @@ -394,6 +394,7 @@ public PublicClientAppKey(string authority, string redirectUri, string applicati #if NETSTANDARD _parentActivityOrWindowFunc = parentActivityOrWindowFunc; #endif + CalculateHashCode(); } public override bool Equals(object obj) From f0dd9d8043bc936ccea8ee3a07b5bac0b20198d7 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 23 Oct 2020 14:19:09 -0700 Subject: [PATCH 5/6] Use constant starting hashcode to allow comparison by value --- .../Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs index 5548246ca6..314baf862a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs @@ -422,7 +422,8 @@ public override int GetHashCode() private void CalculateHashCode() { - _hashValue = base.GetHashCode(); + // use const value here to be able to compare by value and not by reference. + _hashValue = 1430287; if (_authority != null) { From 7ee9883b11c516cfbf496e95a282028f115dadb6 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 23 Oct 2020 14:34:59 -0700 Subject: [PATCH 6/6] Improvements --- .../ActiveDirectoryAuthenticationProvider.cs | 52 ++----------------- 1 file changed, 4 insertions(+), 48 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs index 314baf862a..2421511e62 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs @@ -374,7 +374,6 @@ internal class PublicClientAppKey #if NETSTANDARD public readonly Func _parentActivityOrWindowFunc; #endif - private int _hashValue; public PublicClientAppKey(string authority, string redirectUri, string applicationClientId #if NETFRAMEWORK @@ -394,7 +393,6 @@ public PublicClientAppKey(string authority, string redirectUri, string applicati #if NETSTANDARD _parentActivityOrWindowFunc = parentActivityOrWindowFunc; #endif - CalculateHashCode(); } public override bool Equals(object obj) @@ -415,56 +413,14 @@ public override bool Equals(object obj) return false; } - public override int GetHashCode() - { - return _hashValue; - } - - private void CalculateHashCode() - { - // use const value here to be able to compare by value and not by reference. - _hashValue = 1430287; - - if (_authority != null) - { - unchecked - { - _hashValue = _hashValue * 17 + _authority.GetHashCode(); - } - } - if (_redirectUri != null) - { - unchecked - { - _hashValue = _hashValue * 17 + _redirectUri.GetHashCode(); - } - } - if (_applicationClientId != null) - { - unchecked - { - _hashValue = _hashValue * 17 + _applicationClientId.GetHashCode(); - } - } + public override int GetHashCode() => Tuple.Create(_authority, _redirectUri, _applicationClientId #if NETFRAMEWORK - if (_iWin32WindowFunc != null) - { - unchecked - { - _hashValue = _hashValue * 17 + _iWin32WindowFunc.GetHashCode(); - } - } + , _iWin32WindowFunc #endif #if NETSTANDARD - if (_parentActivityOrWindowFunc != null) - { - unchecked - { - _hashValue = _hashValue * 17 + _parentActivityOrWindowFunc.GetHashCode(); - } - } + , _parentActivityOrWindowFunc #endif - } + ).GetHashCode(); } } }