Merge pull request #366 from fowillar/timeoutFix

Improve reliability of non-interactive login through AAD

Cli-Shared/OperationArguments.cs

@@ -190,7 +190,6 @@ namespace Microsoft.Alm.Cli
             }
         }
         public Interactivity Interactivity { get; set; }
-        public string LoginHint { get; set; }
         public bool PreserveCredentials { get; set; }
         public Uri ProxyUri
         {

Cli-Shared/Program.cs

@@ -385,8 +385,7 @@ namespace Microsoft.Alm.Cli
                     // detect the authority
                     authority = BaseVstsAuthentication.GetAuthentication(operationArguments.TargetUri,
                                                                          VstsCredentialScope,
-                                                                         secrets,
-                                                                         operationArguments.LoginHint)
+                                                                         secrets)
                              ?? GitHubAuthentication.GetAuthentication(operationArguments.TargetUri,
                                                                        GitHubCredentialScope,
                                                                        secrets,
@@ -421,7 +420,7 @@ namespace Microsoft.Alm.Cli
                     Git.Trace.WriteLine($"authority for '{operationArguments.TargetUri}' is Azure Directory.");
 
                     // return the allocated authority or a generic AAD backed VSTS authentication object
-                    return authority ?? new VstsAadAuthentication(Guid.Empty, VstsCredentialScope, secrets, operationArguments.LoginHint);
+                    return authority ?? new VstsAadAuthentication(Guid.Empty, VstsCredentialScope, secrets);
 
                 case AuthorityType.Basic:
                     // enforce basic authentication only
@@ -631,24 +630,6 @@ namespace Microsoft.Alm.Cli
 
                 operationArguments.CustomNamespace = value;
             }
-
-            // look for an AAD login hint
-            if (TryReadString(operationArguments, ConfigPrefix, EnvironLoginHintKey, out value))
-            {
-                Git.Trace.WriteLine($"{EnvironLoginHintKey} = '{value}'.");
-                operationArguments.LoginHint = value;
-            }
-            else
-            {
-                Configuration.Entry loginHint;
-                if (operationArguments.GitConfiguration.TryGetEntry(ConfigPrefix, operationArguments.QueryUri, ConfigLoginHintKey, out loginHint)
-                    && !String.IsNullOrWhiteSpace(loginHint.Value))
-                {
-                    Git.Trace.WriteLine($"{ConfigLoginHintKey} = '{loginHint}'.");
-
-                    operationArguments.LoginHint = loginHint.Value;
-                }
-            }
         }
 
         private static void PrintArgs(string[] args)

Docs/Automation.md

@@ -27,7 +27,3 @@ If your agents rely on an on premise instance of Team Foundation Server and [Win
 To avoid unnecessary service account credential validation, when relying on Microsoft Account or Azure Active Directory use:
 
     git config --global credential.validate false
-
-If your Azure Directory uses an ADFS authority other than visualstudio.com, use:
-
-    git config --global credential.microsoft.visualstudio.com.loginhint microsoft.com

Docs/Configuration.md

@@ -50,15 +50,6 @@ The value should the URL of the proxy server.
 
   `git config --global credential.microsoft.visualstudio.com.interactive never`
 
-### loginhint
-
-Specifies an Azure Active Directory login hint.
-This is helpful in non-interactive scenarios requiring an authoritative domain apart from visualstudio.com.
-
-Hints are typically of the form: `username@domain` or `domain` without quotes.
-
-    git config --global credential.microsoft.visualstudio.com.loginhint microsoft.com
-
 ### modalPrompt
 
 Forces authentication to use a modal dialog instead of asking for credentials at the command prompt.

Microsoft.Alm.Authentication.Test/AuthorityFake.cs

@@ -25,10 +25,8 @@ namespace Microsoft.Alm.Authentication.Test
             return await Task.Run(() => { return new Token("token-access", TokenType.Access); });
         }
 
-        public async Task<Token> NoninteractiveAcquireToken(TargetUri targetUri, string clientId, string resource, Uri redirectUri, string queryParameters = null)
+        public async Task<Token> NoninteractiveAcquireToken(TargetUri targetUri, string clientId, string resource, Uri redirectUri)
         {
-            Assert.AreEqual(this.ExpectedQueryParameters, queryParameters);
-
             return await Task.Run(() => { return new Token("token-access", TokenType.Access); });
         }
 

Microsoft.Alm.Authentication.Test/VstsAadTests.cs

@@ -14,7 +14,7 @@ namespace Microsoft.Alm.Authentication.Test
         public void VstsAadDeleteCredentialsTest()
         {
             TargetUri targetUri = DefaultTargetUri;
-            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-delete", null);
+            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-delete");
 
             aadAuthentication.PersonalAccessTokenStore.WriteCredentials(targetUri, DefaultPersonalAccessToken);
 
@@ -29,7 +29,7 @@ namespace Microsoft.Alm.Authentication.Test
         public void VstsAadGetCredentialsTest()
         {
             TargetUri targetUri = DefaultTargetUri;
-            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-get", null);
+            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-get");
 
             Assert.IsNull(aadAuthentication.GetCredentials(targetUri), "Credentials were retrieved unexpectedly.");
 
@@ -42,7 +42,7 @@ namespace Microsoft.Alm.Authentication.Test
         public void VstsAadInteractiveLogonTest()
         {
             TargetUri targetUri = DefaultTargetUri;
-            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-logon", null);
+            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-logon");
 
             Assert.IsNull(aadAuthentication.PersonalAccessTokenStore.ReadCredentials(targetUri), "Personal Access Token found in store unexpectedly.");
 
@@ -55,7 +55,7 @@ namespace Microsoft.Alm.Authentication.Test
         public void VstsAadNoninteractiveLogonTest()
         {
             TargetUri targetUri = DefaultTargetUri;
-            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-noninteractive", null);
+            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-noninteractive");
 
             Assert.IsNotNull(Task.Run(async () => { return await aadAuthentication.NoninteractiveLogon(targetUri, false); }).Result, "Non-interactive logon unexpectedly failed.");
 
@@ -66,7 +66,7 @@ namespace Microsoft.Alm.Authentication.Test
         public void VstsAadSetCredentialsTest()
         {
             TargetUri targetUri = DefaultTargetUri;
-            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-set", null);
+            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-set");
             Credential credentials = DefaultCredentials;
 
             aadAuthentication.SetCredentials(targetUri, credentials);
@@ -78,7 +78,7 @@ namespace Microsoft.Alm.Authentication.Test
         [TestMethod]
         public void VstsAadValidateCredentialsTest()
         {
-            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-validate", null);
+            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-validate");
             Credential credentials = null;
 
             Assert.IsFalse(Task.Run(async () => { return await aadAuthentication.ValidateCredentials(DefaultTargetUri, credentials); }).Result, "Credential validation unexpectedly failed.");
@@ -92,25 +92,21 @@ namespace Microsoft.Alm.Authentication.Test
         public void VstsAadValidateLoginHintTest()
         {
             TargetUri targetUri = DefaultTargetUri;
-            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-loginhint", "username@domain");
+            VstsAadAuthentication aadAuthentication = GetVstsAadAuthentication("aad-loginhint");
 
             Assert.IsNotNull(Task.Run(async () => { return await aadAuthentication.NoninteractiveLogon(targetUri, false); }).Result, "Non-interactive logon unexpectedly failed.");
 
             Assert.IsNotNull(aadAuthentication.PersonalAccessTokenStore.ReadCredentials(targetUri), "Personal Access Token not found in store as expected.");
         }
 
-        private static VstsAadAuthentication GetVstsAadAuthentication(string @namespace, string loginHint)
+        private static VstsAadAuthentication GetVstsAadAuthentication(string @namespace)
         {
-            string expectedQueryParamters = null;
-            if (loginHint != null)
-            {
-                expectedQueryParamters = "login_hint=" + loginHint;
-            }
+            string expectedQueryParameters = null;
 
             ICredentialStore tokenStore1 = new SecretCache(@namespace + 1);
             ITokenStore tokenStore2 = new SecretCache(@namespace + 2);
-            IVstsAuthority vstsAuthority = new AuthorityFake(expectedQueryParamters);
-            return new VstsAadAuthentication(tokenStore1, tokenStore2, vstsAuthority, loginHint);
+            IVstsAuthority vstsAuthority = new AuthorityFake(expectedQueryParameters);
+            return new VstsAadAuthentication(tokenStore1, tokenStore2, vstsAuthority);
         }
     }
 }

Microsoft.Alm.Authentication/AzureAuthority.cs

@@ -135,12 +135,8 @@ namespace Microsoft.Alm.Authentication
         /// <param name="redirectUri">
         /// Address to return to upon receiving a response from the authority.
         /// </param>
-        /// <param name="queryParameters">
-        /// Optional: appended as-is to the query string in the HTTP authentication request to the
-        /// authority.
-        /// </param>
         /// <returns>If successful a <see cref="TokenPair"/>; otherwise <see langword="null"/>.</returns>
-        public async Task<Token> NoninteractiveAcquireToken(TargetUri targetUri, string clientId, string resource, Uri redirectUri, string queryParameters = null)
+        public async Task<Token> NoninteractiveAcquireToken(TargetUri targetUri, string clientId, string resource, Uri redirectUri)
         {
             if (ReferenceEquals(targetUri, null))
                 throw new ArgumentNullException(nameof(targetUri));
@@ -154,17 +150,13 @@ namespace Microsoft.Alm.Authentication
                 throw new ArgumentException(nameof(redirectUri));
 
             Token token = null;
-            queryParameters = queryParameters ?? String.Empty;
 
             try
             {
                 AuthenticationContext authCtx = new AuthenticationContext(AuthorityHostUrl, _adalTokenCache);
                 AuthenticationResult authResult = await authCtx.AcquireTokenAsync(resource,
                                                                                   clientId,
-                                                                                  redirectUri,
-                                                                                  new PlatformParameters(PromptBehavior.Never),
-                                                                                  UserIdentifier.AnyUser,
-                                                                                  queryParameters);
+                                                                                  new UserCredential());
 
                 token = new Token(authResult, TokenType.Access);
 

Microsoft.Alm.Authentication/BaseVstsAuthentication.cs

@@ -242,15 +242,13 @@ namespace Microsoft.Alm.Authentication
         /// An implementation of <see cref="BaseAuthentication"/> if one was detected;
         /// <see langword="null"/> otherwise.
         /// </param>
-        /// <param name="authenticationHint">An optional authentication hint for AAD</param>
         /// <returns>
         /// <see langword="true"/> if an authority could be determined; <see langword="false"/> otherwise.
         /// </returns>
         public static BaseAuthentication GetAuthentication(
             TargetUri targetUri,
             VstsTokenScope scope,
-            ICredentialStore personalAccessTokenStore,
-            string authenticationHint)
+            ICredentialStore personalAccessTokenStore)
         {
             BaseSecureStore.ValidateTargetUri(targetUri);
             if (ReferenceEquals(scope, null))
@@ -272,7 +270,7 @@ namespace Microsoft.Alm.Authentication
                 else
                 {
                     Git.Trace.WriteLine($"AAD authority for tenant '{tenantId}' detected.");
-                    authentication = new VstsAadAuthentication(tenantId, scope, personalAccessTokenStore, authenticationHint);
+                    authentication = new VstsAadAuthentication(tenantId, scope, personalAccessTokenStore);
                     (authentication as VstsAadAuthentication).TenantId = tenantId;
                 }
             }

Microsoft.Alm.Authentication/IAzureAuthority.cs

@@ -64,11 +64,7 @@ namespace Microsoft.Alm.Authentication
         /// <param name="redirectUri">
         /// Address to return to upon receiving a response from the authority.
         /// </param>
-        /// <param name="queryParameters">
-        /// Optional: appended as-is to the query string in the HTTP authentication request to the
-        /// authority.
-        /// </param>
         /// <returns>If successful a <see cref="TokenPair"/>; otherwise <see langword="null"/>.</returns>
-        Task<Token> NoninteractiveAcquireToken(TargetUri targetUri, string clientId, string resource, Uri redirectUri, string queryParameters = null);
+        Task<Token> NoninteractiveAcquireToken(TargetUri targetUri, string clientId, string resource, Uri redirectUri);
     }
 }

Microsoft.Alm.Authentication/VstsAadAuthentication.cs

@@ -47,15 +47,12 @@ namespace Microsoft.Alm.Authentication
         /// access tokens acquired.</param>
         /// <param name="adaRefreshTokenStore">The secure secret store for storing any Azure tokens
         /// acquired.</param>
-        /// <param name="loginHint">An optional login username and/or domain for guiding non-interactive logon.</param>
         public VstsAadAuthentication(
             Guid tenantId,
             VstsTokenScope tokenScope,
-            ICredentialStore personalAccessTokenStore,
-            string loginHint)
+            ICredentialStore personalAccessTokenStore)
             : base(tokenScope, personalAccessTokenStore)
         {
-            this.loginHint = loginHint;
             if (tenantId == Guid.Empty)
             {
                 this.VstsAuthority = new VstsAzureAuthority(AzureAuthority.DefaultAuthorityHostUrl);
@@ -74,17 +71,13 @@ namespace Microsoft.Alm.Authentication
         internal VstsAadAuthentication(
             ICredentialStore personalAccessTokenStore,
             ITokenStore vstsIdeTokenCache,
-            IVstsAuthority vstsAuthority,
-            string loginHint)
+            IVstsAuthority vstsAuthority)
             : base(personalAccessTokenStore,
                    vstsIdeTokenCache,
                    vstsAuthority)
         {
-            this.loginHint = loginHint;
         }
 
-        internal readonly String loginHint;
-
         /// <summary>
         /// <para>Creates an interactive logon session, using ADAL secure browser GUI, which
         /// enables users to authenticate with the Azure tenant and acquire the necessary access
@@ -148,19 +141,8 @@ namespace Microsoft.Alm.Authentication
 
             try
             {
-                string hint = null;
-                if (!String.IsNullOrWhiteSpace(this.loginHint))
-                {
-                    // Do not escape these parameters. Hints such as 'username@domain' will stop working.
-                    hint = String.Format(System.Globalization.CultureInfo.InvariantCulture,
-                                         "login_hint={0}",
-                                         this.loginHint);
-
-                    Git.Trace.WriteLine($"AAD extra query parameters ='{hint}'");
-                }
-
                 Token token;
-                if ((token = await this.VstsAuthority.NoninteractiveAcquireToken(targetUri, this.ClientId, this.Resource, new Uri(RedirectUrl), hint)) != null)
+                if ((token = await this.VstsAuthority.NoninteractiveAcquireToken(targetUri, this.ClientId, this.Resource, new Uri(RedirectUrl))) != null)
                 {
                     Git.Trace.WriteLine($"token acquisition for '{targetUri}' succeeded");