Issue #218 Address PR comments with minor updates

mminns · mminns · commit ff53cd2b89c0 · 2021-12-10T14:47:18.000Z
diff --git a/src/shared/Atlassian.Bitbucket.Tests/BitbucketHostProviderTest.cs b/src/shared/Atlassian.Bitbucket.Tests/BitbucketHostProviderTest.cs
@@ -156,7 +156,7 @@ public void BitbucketHostProvider_GetCredentialAsync_Succeeds_ForFreshValidBasic
 
             MockUserEntersValidBasicCredentials(bitbucketAuthentication, input, password);
 
-            if(BITBUCKET_DOT_ORG_HOST.Equals(host)) 
+            if (BITBUCKET_DOT_ORG_HOST.Equals(host)) 
             {
                 MockRemoteOAuthAccountIsValid(bitbucketApi, input, password, true);
             }
@@ -190,12 +190,6 @@ public void BitbucketHostProvider_GetCredentialAsync_Succeeds_ForFreshValid2FAAc
 
             var provider = new BitbucketHostProvider(context, bitbucketAuthentication.Object, bitbucketApi.Object);
 
-            //if (userEntersCredentials.HasValue && !userEntersCredentials.Value)
-            //{
-            //    Assert.ThrowsAsync<Exception>(async () => await provider.GetCredentialAsync(input));
-            //    return;
-            //}
-
             var credential = provider.GetCredentialAsync(input);
 
             VerifyOAuthFlowRan(password, false, true, input, credential, null);
@@ -222,7 +216,6 @@ public void BitbucketHostProvider_GetCredentialAsync_ForcedAuthMode_IsRespected(
             MockUserEntersValidBasicCredentials(bitbucketAuthentication, input, password);
             MockRemoteBasicAuthAccountIsValidRequires2FA(bitbucketApi, input, password);
             bitbucketAuthentication.Setup(m => m.ShowOAuthRequiredPromptAsync()).ReturnsAsync(true);
-            //MockRemoteValidRefreshToken();
 
             var provider = new BitbucketHostProvider(context, bitbucketAuthentication.Object, bitbucketApi.Object);
 
@@ -536,25 +529,25 @@ private void VerifyInteractiveOAuthFlowNeverRan(InputArguments input, System.Thr
         private void VerifyValidateBasicAuthCredentialsNeverRan() 
         {
             // never check username/password works
-            bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), BitbucketHostProvider.USE_BASIC_TOKEN), Times.Never);
+            bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), false), Times.Never);
         }
 
         private void VerifyValidateBasicAuthCredentialsRan() 
         {
             // check username/password works
-            bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), BitbucketHostProvider.USE_BASIC_TOKEN), Times.Once);
+            bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), false), Times.Once);
         }
 
         private void VerifyValidateOAuthCredentialsNeverRan() 
         {
             // never check username/password works
-            bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), BitbucketHostProvider.USE_BASIC_TOKEN), Times.Never);
+            bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), false), Times.Never);
         }
 
         private void VerifyValidateOAuthCredentialsRan() 
         {
             // check username/password works
-            bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), BitbucketHostProvider.USE_BEARER_TOKEN), Times.Once);
+            bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), true), Times.Once);
         }
 
         private void MockStoredOAuthAccount(TestCommandContext context, InputArguments input)
@@ -592,7 +585,7 @@ private static void MockRemoteBasicAuthAccountIsValid(Mock<IBitbucketRestApi> bi
         {
             var userInfo = new UserInfo() { IsTwoFactorAuthenticationEnabled = twoFAEnabled };
             // Basic
-            bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, BitbucketHostProvider.USE_BASIC_TOKEN)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));
+            bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, false)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));
 
         }
             
@@ -610,15 +603,15 @@ private static void MockRemoteBasicAuthAccountIsInvalid(Mock<IBitbucketRestApi>
         {
             var userInfo = new UserInfo();
             // Basic
-            bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, BitbucketHostProvider.USE_BASIC_TOKEN)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.Forbidden, userInfo));
+            bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, false)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.Forbidden, userInfo));
 
         }
 
         private static void MockRemoteOAuthAccountIsValid(Mock<IBitbucketRestApi> bitbucketApi, InputArguments input, string password, bool twoFAEnabled)
         {
             var userInfo = new UserInfo() { IsTwoFactorAuthenticationEnabled = twoFAEnabled };
             // OAuth
-            bitbucketApi.Setup(x => x.GetUserInformationAsync(null, password, BitbucketHostProvider.USE_BEARER_TOKEN)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));
+            bitbucketApi.Setup(x => x.GetUserInformationAsync(null, password, true)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));
         }
 
         private static void MockStoredAccount(TestCommandContext context, InputArguments input, string password)
diff --git a/src/shared/Atlassian.Bitbucket.Tests/BitbucketOauth2ClientTest.cs b/src/shared/Atlassian.Bitbucket.Tests/BitbucketOauth2ClientTest.cs
@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.IO;
 using System.Net;
 using System.Net.Http;
 using System.Threading;
@@ -81,7 +80,6 @@ private void VerifyOAuth2TokenResult(OAuth2TokenResult result)
             IEnumerable<char> tokenType = null;
             Assert.Equal(tokenType, result.TokenType);
             Assert.Equal(null, result.Scopes);
-            //Assert.Equal(pkceCodeVerifier, result.ExpiresIn);
         }
 
         private void VerifyAuthorizationCodeResult(OAuth2AuthorizationCodeResult result)
diff --git a/src/shared/Atlassian.Bitbucket.Tests/BitbucketRestApiTest.cs b/src/shared/Atlassian.Bitbucket.Tests/BitbucketRestApiTest.cs
@@ -33,15 +33,15 @@ public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSu
             var httpHandler = new TestHttpMessageHandler();
             httpHandler.Setup(HttpMethod.Get, expectedRequestUri, request =>
             {
-                if(isBearerToken)
+                if (isBearerToken)
                 {
                     RestTestUtilities.AssertBearerAuth(request, password);
                 }
                 else
                 {
                     RestTestUtilities.AssertBasicAuth(request, username, password);
                 }
-                //AssertAuthCode(request, testAuthCode);
+
                 return httpResponse;
             });
             context.HttpClientFactory.MessageHandler = httpHandler;
diff --git a/src/shared/Atlassian.Bitbucket/BitbucketHostProvider.cs b/src/shared/Atlassian.Bitbucket/BitbucketHostProvider.cs
@@ -10,8 +10,6 @@ namespace Atlassian.Bitbucket
 {
     public class BitbucketHostProvider : IHostProvider
     {
-        public const bool USE_BEARER_TOKEN = true;
-        public const bool USE_BASIC_TOKEN = !USE_BEARER_TOKEN;
         private readonly ICommandContext _context;
         private readonly IBitbucketAuthentication _bitbucketAuth;
         private readonly IBitbucketRestApi _bitbucketApi;
@@ -96,7 +94,7 @@ private static void ValidateTargetUri(InputArguments input)
 
         private async Task<ICredential> GetStoredCredentials(InputArguments input)
         {
-            if(_context.Settings.TryGetSetting(BitbucketConstants.EnvironmentVariables.AlwaysRefreshCredentials, 
+            if (_context.Settings.TryGetSetting(BitbucketConstants.EnvironmentVariables.AlwaysRefreshCredentials, 
                 Constants.GitConfiguration.Credential.SectionName, BitbucketConstants.GitConfiguration.Credential.AlwaysRefreshCredentials, 
                 out string alwaysRefreshCredentials) && alwaysRefreshCredentials.ToBooleanyOrDefault(false))
             {
@@ -110,16 +108,16 @@ private async Task<ICredential> GetStoredCredentials(InputArguments input)
 
             var credentials = _context.CredentialStore.Get(credentialService, input.UserName);
 
-            if(credentials == null)
+            if (credentials == null)
             {
                 _context.Trace.WriteLine($"    Found none");
                 return null;
             }
 
-            _context.Trace.WriteLine($"    Found credentials: {credentials.Account}/**********");
+            _context.Trace.WriteLineSecrets($"    Found credentials: {credentials.Account}/{{0}}", new object[] {credentials.Password});
             
             //check credentials are still valid
-            if(!await ValidateCredentialsWork(input, credentials, GetSupportedAuthenticationModes(targetUri))) {
+            if (!await ValidateCredentialsWork(input, credentials, GetSupportedAuthenticationModes(targetUri))) {
                 return null;
             }
 
@@ -154,7 +152,7 @@ private async Task<ICredential> GetRefreshedCredentials(InputArguments input)
 
                     var basicCredentials = await GetBasicCredentialsInteractive(input, authModes);
 
-                    if(await ValidateCredentialsWork(input, basicCredentials, authModes))
+                    if (await ValidateCredentialsWork(input, basicCredentials, authModes))
                     {
                         return basicCredentials;
                     }
@@ -181,8 +179,6 @@ private async Task<ICredential> GetRefreshedCredentials(InputArguments input)
             else
             {
                 _context.Trace.WriteLine($"    Found refresh token: {refreshToken} ");
-                // TODO: should we try and compute if the AT has expired and use it?
-                // This needs support from the credential store to record the expiry time!
 
                 // It's very likely that any access token expired between the last time we used/stored it.
                 // To ensure the AT is as 'fresh' as it can be, always first try to use the refresh token
@@ -292,7 +288,7 @@ private static bool SupportsBasicAuth(AuthenticationModes authModes)
 
         public AuthenticationModes GetSupportedAuthenticationModes(Uri targetUri)
         {
-            if(!IsBitbucketOrg(targetUri))
+            if (!IsBitbucketOrg(targetUri))
             {
                 // Bitbucket Server/DC should use Basic only
                 return BitbucketConstants.ServerAuthenticationModes;
@@ -361,7 +357,7 @@ public Task EraseCredentialAsync(InputArguments input)
 
         private async Task<string> ResolveOAuthUserNameAsync(string accessToken)
         {
-            RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(null, accessToken, USE_BEARER_TOKEN);
+            RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(null, accessToken, isBearerToken: true);
             if (result.Succeeded)
             {
                 return result.Response.UserName;
@@ -372,7 +368,7 @@ private async Task<string> ResolveOAuthUserNameAsync(string accessToken)
 
         private async Task<string> ResolveBasicAuthUserNameAsync(string username, string password)
         {
-            RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(username, password, USE_BASIC_TOKEN);
+            RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(username, password, isBearerToken: false);
             if (result.Succeeded)
             {
                 return result.Response.UserName;
@@ -383,15 +379,15 @@ private async Task<string> ResolveBasicAuthUserNameAsync(string username, string
 
         private async Task<bool> RequiresTwoFactorAuthenticationAsync(ICredential credentials, AuthenticationModes authModes)
         {
-            _context.Trace.WriteLine($"Check if 2FA si required for credentials ({credentials.Account}/**********) {authModes} ...");
+            _context.Trace.WriteLineSecrets($"Check if 2FA si required for credentials ({credentials.Account}/{{0}}) {authModes} ...", new object[] {credentials.Password});
 
-            if(!SupportsOAuth(authModes))
+            if (!SupportsOAuth(authModes))
             {
                 return false;
             }
 
             RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(
-                credentials.Account, credentials.Password, USE_BASIC_TOKEN);
+                credentials.Account, credentials.Password, isBearerToken: false);
             switch (result.StatusCode)
             {
                 // 2FA may not be required
@@ -413,26 +409,30 @@ private async Task<bool> RequiresTwoFactorAuthenticationAsync(ICredential creden
 
         private async Task<bool> ValidateCredentialsWork(InputArguments input, ICredential credentials, AuthenticationModes authModes) 
         {
-            if(credentials == null)
+            if (credentials == null)
             {
                 return false;
             }
 
+            // TODO ideally we'd also check if the credentials have expired based on some local metadata (once/if we get such metadata storage), 
+            // and return false if they have.
+            // This would be more efficient than having to make REST API calls to check.
+
             var targetUri = input.GetRemoteUri();
-            _context.Trace.WriteLine($"Validate credentials ({credentials.Account}/**********) are fresh for {targetUri} ...");
+            _context.Trace.WriteLineSecrets($"Validate credentials ({credentials.Account}/{{0}}) are fresh for {targetUri} ...", new object[] {credentials.Password});
 
-            if(!IsBitbucketOrg(targetUri))
+            if (!IsBitbucketOrg(targetUri))
             {
                 // TODO Validate DC/Server credentials before returning them to Git
                 // Currently credentials for DC/Server are not checked by GCM.
-                // Instead the validation relies on Git to try and fail with the creneitals and then request GCM to erase them
+                // Instead the validation relies on Git to try and fail with the credentials and then request GCM to erase them
                 _context.Trace.WriteLine("For DC/Server skip validating existing credentials");
                 return await Task.FromResult(true);
             }
 
             // Bitbucket supports both OAuth + Basic Auth unless there is explicit GCM configuration
             // So the credentials could be for either scheme therefore need to potentiall test both posibilities.
-            if(SupportsOAuth(authModes))
+            if (SupportsOAuth(authModes))
             {
                 try
                 {
@@ -446,7 +446,7 @@ private async Task<bool> ValidateCredentialsWork(InputArguments input, ICredenti
                 }
             }
 
-            if(SupportsBasicAuth(authModes))
+            if (SupportsBasicAuth(authModes))
             {
                 try
                 {

Original file line number	Diff line number	Diff line change
`@@ -156,7 +156,7 @@ public void BitbucketHostProvider_GetCredentialAsync_Succeeds_ForFreshValidBasic`
`156`	`156`
`157`	`157`	`MockUserEntersValidBasicCredentials(bitbucketAuthentication, input, password);`
`158`	`158`
`159`		`- if(BITBUCKET_DOT_ORG_HOST.Equals(host))`
	`159`	`+ if (BITBUCKET_DOT_ORG_HOST.Equals(host))`
`160`	`160`	`{`
`161`	`161`	`MockRemoteOAuthAccountIsValid(bitbucketApi, input, password, true);`
`162`	`162`	`}`
`@@ -190,12 +190,6 @@ public void BitbucketHostProvider_GetCredentialAsync_Succeeds_ForFreshValid2FAAc`
`190`	`190`
`191`	`191`	`var provider = new BitbucketHostProvider(context, bitbucketAuthentication.Object, bitbucketApi.Object);`
`192`	`192`
`193`		`- //if (userEntersCredentials.HasValue && !userEntersCredentials.Value)`
`194`		`- //{`
`195`		`- // Assert.ThrowsAsync<Exception>(async () => await provider.GetCredentialAsync(input));`
`196`		`- // return;`
`197`		`- //}`
`198`		`-`
`199`	`193`	`var credential = provider.GetCredentialAsync(input);`
`200`	`194`
`201`	`195`	`VerifyOAuthFlowRan(password, false, true, input, credential, null);`
`@@ -222,7 +216,6 @@ public void BitbucketHostProvider_GetCredentialAsync_ForcedAuthMode_IsRespected(`
`222`	`216`	`MockUserEntersValidBasicCredentials(bitbucketAuthentication, input, password);`
`223`	`217`	`MockRemoteBasicAuthAccountIsValidRequires2FA(bitbucketApi, input, password);`
`224`	`218`	`bitbucketAuthentication.Setup(m => m.ShowOAuthRequiredPromptAsync()).ReturnsAsync(true);`
`225`		`- //MockRemoteValidRefreshToken();`
`226`	`219`
`227`	`220`	`var provider = new BitbucketHostProvider(context, bitbucketAuthentication.Object, bitbucketApi.Object);`
`228`	`221`
`@@ -536,25 +529,25 @@ private void VerifyInteractiveOAuthFlowNeverRan(InputArguments input, System.Thr`
`536`	`529`	`private void VerifyValidateBasicAuthCredentialsNeverRan()`
`537`	`530`	`{`
`538`	`531`	`// never check username/password works`
`539`		`- bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), BitbucketHostProvider.USE_BASIC_TOKEN), Times.Never);`
	`532`	`+ bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), false), Times.Never);`
`540`	`533`	`}`
`541`	`534`
`542`	`535`	`private void VerifyValidateBasicAuthCredentialsRan()`
`543`	`536`	`{`
`544`	`537`	`// check username/password works`
`545`		`- bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), BitbucketHostProvider.USE_BASIC_TOKEN), Times.Once);`
	`538`	`+ bitbucketApi.Verify(m => m.GetUserInformationAsync(It.IsAny<string>(), It.IsAny<string>(), false), Times.Once);`
`546`	`539`	`}`
`547`	`540`
`548`	`541`	`private void VerifyValidateOAuthCredentialsNeverRan()`
`549`	`542`	`{`
`550`	`543`	`// never check username/password works`
`551`		`- bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), BitbucketHostProvider.USE_BASIC_TOKEN), Times.Never);`
	`544`	`+ bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), false), Times.Never);`
`552`	`545`	`}`
`553`	`546`
`554`	`547`	`private void VerifyValidateOAuthCredentialsRan()`
`555`	`548`	`{`
`556`	`549`	`// check username/password works`
`557`		`- bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), BitbucketHostProvider.USE_BEARER_TOKEN), Times.Once);`
	`550`	`+ bitbucketApi.Verify(m => m.GetUserInformationAsync(null, It.IsAny<string>(), true), Times.Once);`
`558`	`551`	`}`
`559`	`552`
`560`	`553`	`private void MockStoredOAuthAccount(TestCommandContext context, InputArguments input)`
`@@ -592,7 +585,7 @@ private static void MockRemoteBasicAuthAccountIsValid(Mock<IBitbucketRestApi> bi`
`592`	`585`	`{`
`593`	`586`	`var userInfo = new UserInfo() { IsTwoFactorAuthenticationEnabled = twoFAEnabled };`
`594`	`587`	`// Basic`
`595`		`- bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, BitbucketHostProvider.USE_BASIC_TOKEN)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));`
	`588`	`+ bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, false)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));`
`596`	`589`
`597`	`590`	`}`
`598`	`591`
`@@ -610,15 +603,15 @@ private static void MockRemoteBasicAuthAccountIsInvalid(Mock<IBitbucketRestApi>`
`610`	`603`	`{`
`611`	`604`	`var userInfo = new UserInfo();`
`612`	`605`	`// Basic`
`613`		`- bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, BitbucketHostProvider.USE_BASIC_TOKEN)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.Forbidden, userInfo));`
	`606`	`+ bitbucketApi.Setup(x => x.GetUserInformationAsync(input.UserName, password, false)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.Forbidden, userInfo));`
`614`	`607`
`615`	`608`	`}`
`616`	`609`
`617`	`610`	`private static void MockRemoteOAuthAccountIsValid(Mock<IBitbucketRestApi> bitbucketApi, InputArguments input, string password, bool twoFAEnabled)`
`618`	`611`	`{`
`619`	`612`	`var userInfo = new UserInfo() { IsTwoFactorAuthenticationEnabled = twoFAEnabled };`
`620`	`613`	`// OAuth`
`621`		`- bitbucketApi.Setup(x => x.GetUserInformationAsync(null, password, BitbucketHostProvider.USE_BEARER_TOKEN)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));`
	`614`	`+ bitbucketApi.Setup(x => x.GetUserInformationAsync(null, password, true)).ReturnsAsync(new RestApiResult<UserInfo>(System.Net.HttpStatusCode.OK, userInfo));`
`622`	`615`	`}`
`623`	`616`
`624`	`617`	`private static void MockStoredAccount(TestCommandContext context, InputArguments input, string password)`
Original file line number	Diff line number	Diff line change
`@@ -33,15 +33,15 @@ public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSu`
`33`	`33`	`var httpHandler = new TestHttpMessageHandler();`
`34`	`34`	`httpHandler.Setup(HttpMethod.Get, expectedRequestUri, request =>`
`35`	`35`	`{`
`36`		`- if(isBearerToken)`
	`36`	`+ if (isBearerToken)`
`37`	`37`	`{`
`38`	`38`	`RestTestUtilities.AssertBearerAuth(request, password);`
`39`	`39`	`}`
`40`	`40`	`else`
`41`	`41`	`{`
`42`	`42`	`RestTestUtilities.AssertBasicAuth(request, username, password);`
`43`	`43`	`}`
`44`		`- //AssertAuthCode(request, testAuthCode);`
	`44`	`+`
`45`	`45`	`return httpResponse;`
`46`	`46`	`});`
`47`	`47`	`context.HttpClientFactory.MessageHandler = httpHandler;`
Original file line number	Diff line number	Diff line change
`@@ -10,8 +10,6 @@ namespace Atlassian.Bitbucket`
`10`	`10`	`{`
`11`	`11`	`public class BitbucketHostProvider : IHostProvider`
`12`	`12`	`{`
`13`		`- public const bool USE_BEARER_TOKEN = true;`
`14`		`- public const bool USE_BASIC_TOKEN = !USE_BEARER_TOKEN;`
`15`	`13`	`private readonly ICommandContext _context;`
`16`	`14`	`private readonly IBitbucketAuthentication _bitbucketAuth;`
`17`	`15`	`private readonly IBitbucketRestApi _bitbucketApi;`
`@@ -96,7 +94,7 @@ private static void ValidateTargetUri(InputArguments input)`
`96`	`94`
`97`	`95`	`private async Task<ICredential> GetStoredCredentials(InputArguments input)`
`98`	`96`	`{`
`99`		`- if(_context.Settings.TryGetSetting(BitbucketConstants.EnvironmentVariables.AlwaysRefreshCredentials,`
	`97`	`+ if (_context.Settings.TryGetSetting(BitbucketConstants.EnvironmentVariables.AlwaysRefreshCredentials,`
`100`	`98`	`Constants.GitConfiguration.Credential.SectionName, BitbucketConstants.GitConfiguration.Credential.AlwaysRefreshCredentials,`
`101`	`99`	`out string alwaysRefreshCredentials) && alwaysRefreshCredentials.ToBooleanyOrDefault(false))`
`102`	`100`	`{`
`@@ -110,16 +108,16 @@ private async Task<ICredential> GetStoredCredentials(InputArguments input)`
`110`	`108`
`111`	`109`	`var credentials = _context.CredentialStore.Get(credentialService, input.UserName);`
`112`	`110`
`113`		`- if(credentials == null)`
	`111`	`+ if (credentials == null)`
`114`	`112`	`{`
`115`	`113`	`_context.Trace.WriteLine($" Found none");`
`116`	`114`	`return null;`
`117`	`115`	`}`
`118`	`116`
`119`		`- _context.Trace.WriteLine($" Found credentials: {credentials.Account}/**********");`
	`117`	`+ _context.Trace.WriteLineSecrets($" Found credentials: {credentials.Account}/{{0}}", new object[] {credentials.Password});`
`120`	`118`
`121`	`119`	`//check credentials are still valid`
`122`		`- if(!await ValidateCredentialsWork(input, credentials, GetSupportedAuthenticationModes(targetUri))) {`
	`120`	`+ if (!await ValidateCredentialsWork(input, credentials, GetSupportedAuthenticationModes(targetUri))) {`
`123`	`121`	`return null;`
`124`	`122`	`}`
`125`	`123`
`@@ -154,7 +152,7 @@ private async Task<ICredential> GetRefreshedCredentials(InputArguments input)`
`154`	`152`
`155`	`153`	`var basicCredentials = await GetBasicCredentialsInteractive(input, authModes);`
`156`	`154`
`157`		`- if(await ValidateCredentialsWork(input, basicCredentials, authModes))`
	`155`	`+ if (await ValidateCredentialsWork(input, basicCredentials, authModes))`
`158`	`156`	`{`
`159`	`157`	`return basicCredentials;`
`160`	`158`	`}`
`@@ -181,8 +179,6 @@ private async Task<ICredential> GetRefreshedCredentials(InputArguments input)`
`181`	`179`	`else`
`182`	`180`	`{`
`183`	`181`	`_context.Trace.WriteLine($" Found refresh token: {refreshToken} ");`
`184`		`- // TODO: should we try and compute if the AT has expired and use it?`
`185`		`- // This needs support from the credential store to record the expiry time!`
`186`	`182`
`187`	`183`	`// It's very likely that any access token expired between the last time we used/stored it.`
`188`	`184`	`// To ensure the AT is as 'fresh' as it can be, always first try to use the refresh token`
`@@ -292,7 +288,7 @@ private static bool SupportsBasicAuth(AuthenticationModes authModes)`
`292`	`288`
`293`	`289`	`public AuthenticationModes GetSupportedAuthenticationModes(Uri targetUri)`
`294`	`290`	`{`
`295`		`- if(!IsBitbucketOrg(targetUri))`
	`291`	`+ if (!IsBitbucketOrg(targetUri))`
`296`	`292`	`{`
`297`	`293`	`// Bitbucket Server/DC should use Basic only`
`298`	`294`	`return BitbucketConstants.ServerAuthenticationModes;`
`@@ -361,7 +357,7 @@ public Task EraseCredentialAsync(InputArguments input)`
`361`	`357`
`362`	`358`	`private async Task<string> ResolveOAuthUserNameAsync(string accessToken)`
`363`	`359`	`{`
`364`		`- RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(null, accessToken, USE_BEARER_TOKEN);`
	`360`	`+ RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(null, accessToken, isBearerToken: true);`
`365`	`361`	`if (result.Succeeded)`
`366`	`362`	`{`
`367`	`363`	`return result.Response.UserName;`
`@@ -372,7 +368,7 @@ private async Task<string> ResolveOAuthUserNameAsync(string accessToken)`
`372`	`368`
`373`	`369`	`private async Task<string> ResolveBasicAuthUserNameAsync(string username, string password)`
`374`	`370`	`{`
`375`		`- RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(username, password, USE_BASIC_TOKEN);`
	`371`	`+ RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(username, password, isBearerToken: false);`
`376`	`372`	`if (result.Succeeded)`
`377`	`373`	`{`
`378`	`374`	`return result.Response.UserName;`
`@@ -383,15 +379,15 @@ private async Task<string> ResolveBasicAuthUserNameAsync(string username, string`
`383`	`379`
`384`	`380`	`private async Task<bool> RequiresTwoFactorAuthenticationAsync(ICredential credentials, AuthenticationModes authModes)`
`385`	`381`	`{`
`386`		`- _context.Trace.WriteLine($"Check if 2FA si required for credentials ({credentials.Account}/**********) {authModes} ...");`
	`382`	`+ _context.Trace.WriteLineSecrets($"Check if 2FA si required for credentials ({credentials.Account}/{{0}}) {authModes} ...", new object[] {credentials.Password});`
`387`	`383`
`388`		`- if(!SupportsOAuth(authModes))`
	`384`	`+ if (!SupportsOAuth(authModes))`
`389`	`385`	`{`
`390`	`386`	`return false;`
`391`	`387`	`}`
`392`	`388`
`393`	`389`	`RestApiResult<UserInfo> result = await _bitbucketApi.GetUserInformationAsync(`
`394`		`- credentials.Account, credentials.Password, USE_BASIC_TOKEN);`
	`390`	`+ credentials.Account, credentials.Password, isBearerToken: false);`
`395`	`391`	`switch (result.StatusCode)`
`396`	`392`	`{`
`397`	`393`	`// 2FA may not be required`
`@@ -413,26 +409,30 @@ private async Task<bool> RequiresTwoFactorAuthenticationAsync(ICredential creden`
`413`	`409`
`414`	`410`	`private async Task<bool> ValidateCredentialsWork(InputArguments input, ICredential credentials, AuthenticationModes authModes)`
`415`	`411`	`{`
`416`		`- if(credentials == null)`
	`412`	`+ if (credentials == null)`
`417`	`413`	`{`
`418`	`414`	`return false;`
`419`	`415`	`}`
`420`	`416`
	`417`	`+ // TODO ideally we'd also check if the credentials have expired based on some local metadata (once/if we get such metadata storage),`
	`418`	`+ // and return false if they have.`
	`419`	`+ // This would be more efficient than having to make REST API calls to check.`
	`420`	`+`
`421`	`421`	`var targetUri = input.GetRemoteUri();`
`422`		`- _context.Trace.WriteLine($"Validate credentials ({credentials.Account}/**********) are fresh for {targetUri} ...");`
	`422`	`+ _context.Trace.WriteLineSecrets($"Validate credentials ({credentials.Account}/{{0}}) are fresh for {targetUri} ...", new object[] {credentials.Password});`
`423`	`423`
`424`		`- if(!IsBitbucketOrg(targetUri))`
	`424`	`+ if (!IsBitbucketOrg(targetUri))`
`425`	`425`	`{`
`426`	`426`	`// TODO Validate DC/Server credentials before returning them to Git`
`427`	`427`	`// Currently credentials for DC/Server are not checked by GCM.`
`428`		`- // Instead the validation relies on Git to try and fail with the creneitals and then request GCM to erase them`
	`428`	`+ // Instead the validation relies on Git to try and fail with the credentials and then request GCM to erase them`
`429`	`429`	`_context.Trace.WriteLine("For DC/Server skip validating existing credentials");`
`430`	`430`	`return await Task.FromResult(true);`
`431`	`431`	`}`
`432`	`432`
`433`	`433`	`// Bitbucket supports both OAuth + Basic Auth unless there is explicit GCM configuration`
`434`	`434`	`// So the credentials could be for either scheme therefore need to potentiall test both posibilities.`
`435`		`- if(SupportsOAuth(authModes))`
	`435`	`+ if (SupportsOAuth(authModes))`
`436`	`436`	`{`
`437`	`437`	`try`
`438`	`438`	`{`
`@@ -446,7 +446,7 @@ private async Task<bool> ValidateCredentialsWork(InputArguments input, ICredenti`
`446`	`446`	`}`
`447`	`447`	`}`
`448`	`448`
`449`		`- if(SupportsBasicAuth(authModes))`
	`449`	`+ if (SupportsBasicAuth(authModes))`
`450`	`450`	`{`
`451`	`451`	`try`
`452`	`452`	`{`