Merge pull request #1803 from tyrielv/gcm-cache

fix: #1802 confirm cached credential before rejecting

GVFS/GVFS.Common/Git/GitAuthentication.cs

@@ -91,9 +91,27 @@ public void RejectCredentials(ITracer tracer, string credentialString)
         {
             lock (this.gitAuthLock)
             {
+                string cachedCredentialAtStartOfReject = this.cachedCredentialString;
                 // Don't stomp a different credential
-                if (credentialString == this.cachedCredentialString && this.cachedCredentialString != null)
+                if (credentialString == cachedCredentialAtStartOfReject && cachedCredentialAtStartOfReject != null)
                 {
+                    // We can't assume that the credential store's cached credential is the same as the one we have.
+                    // Reload the credential from the store to ensure we're rejecting the correct one.
+                    int attemptsBeforeCheckingExistingCredential = this.numberOfAttempts;
+                    if (this.TryCallGitCredential(tracer, out string getCredentialError))
+                    {
+                        if (this.cachedCredentialString != cachedCredentialAtStartOfReject)
+                        {
+                            // If the store already had a different credential, we don't want to reject it without trying it.
+                            this.isCachedCredentialStringApproved = false;
+                            return;
+                        }
+                    }
+                    else
+                    {
+                        tracer.RelatedWarning(getCredentialError);
+                    }
+
                     // If we can we should pass the actual username/password values we used (and found to be invalid)
                     // to `git-credential reject` so the credential helpers can attempt to check if they're erasing
                     // the expected credentials, if they so choose to.
@@ -121,7 +139,12 @@ public void RejectCredentials(ITracer tracer, string credentialString)
 
                     this.cachedCredentialString = null;
                     this.isCachedCredentialStringApproved = false;
-                    this.UpdateBackoff();
+
+                    // Backoff may have already been incremented by a failure in TryCallGitCredential
+                    if (attemptsBeforeCheckingExistingCredential == this.numberOfAttempts)
+                    {
+                        this.UpdateBackoff();
+                    }
                 }
             }
         }

GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Linq;
 using GVFS.Common.Git;
 using GVFS.Tests;
@@ -245,6 +246,33 @@ public void DontStoreDifferentCredentialFromCachedValue()
             gitProcess.StoredCredentials.Single().Key.ShouldEqual("mock://repoUrl");
         }
 
+        [TestCase]
+        public void RejectionShouldNotBeSentIfUnderlyingTokenHasChanged()
+        {
+            MockTracer tracer = new MockTracer();
+            MockGitProcess gitProcess = this.GetGitProcess();
+
+            GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl");
+            dut.TryInitializeAndRequireAuth(tracer, out _);
+
+            // Get and store an initial value that will be cached
+            string authString;
+            dut.TryGetCredentials(tracer, out authString, out _).ShouldBeTrue();
+            dut.ApproveCredentials(tracer, authString);
+
+            // Change the underlying token
+            gitProcess.SetExpectedCommandResult(
+                $"{AzureDevOpsUseHttpPathString} credential fill",
+                () => new GitProcess.Result("username=username\r\npassword=password" + Guid.NewGuid() + "\r\n", string.Empty, GitProcess.Result.SuccessCode));
+
+            // Try and reject it. We should get a new token, but without forwarding the rejection to the
+            // underlying credential store
+            dut.RejectCredentials(tracer, authString);
+            dut.TryGetCredentials(tracer, out var newAuthString, out _).ShouldBeTrue();
+            newAuthString.ShouldNotEqual(authString);
+            gitProcess.CredentialRejections.ShouldBeEmpty();
+        }
+
         private MockGitProcess GetGitProcess()
         {
             MockGitProcess gitProcess = new MockGitProcess();

GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs

@@ -5,6 +5,7 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Text;
 
 namespace GVFS.UnitTests.Mock.Git
@@ -91,7 +92,7 @@ public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string
                 return new Result(string.Empty, string.Empty, Result.GenericFailureCode);
             }
 
-            Predicate<CommandInfo> commandMatchFunction =
+            Func<CommandInfo, bool> commandMatchFunction =
                 (CommandInfo commandInfo) =>
                 {
                     if (commandInfo.MatchPrefix)
@@ -104,7 +105,7 @@ public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string
                     }
                 };
 
-            CommandInfo matchedCommand = this.expectedCommandInfos.Find(commandMatchFunction);
+            CommandInfo matchedCommand = this.expectedCommandInfos.Last(commandMatchFunction);
             matchedCommand.ShouldNotBeNull("Unexpected command: " + command);
 
             return matchedCommand.Result();

GVFS/GVFS/CommandLine/GVFSVerb.cs

@@ -312,6 +312,11 @@ public static bool TrySetRequiredGitConfigSettings(Enlistment enlistment)
 
                 // Disable the builtin FS Monitor in case it was enabled globally.
                 { "core.useBuiltinFSMonitor", "false" },
+
+                // Set the GCM credential method to use OAuth tokens.
+                // See https://github.com/git-ecosystem/git-credential-manager/blob/release/docs/configuration.md#credentialazreposcredentialtype
+                // for more information.
+                { "credential.azreposCredentialType", "oauth" },
             };
 
             if (!TrySetConfig(enlistment, requiredSettings, isRequired: true))