From cf6b45f355cdb6c936b75339b4086e34742211f7 Mon Sep 17 00:00:00 2001
From: Tyrie Vella <tyrielv@gmail.com>
Date: Fri, 26 Jan 2024 13:34:47 -0800
Subject: [PATCH] fix: #1802 confirm cached credential before rejecting

---
 GVFS/GVFS.Common/Git/GitAuthentication.cs     | 27 ++++++++++++++++--
 .../Git/GitAuthenticationTests.cs             | 28 +++++++++++++++++++
 .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs |  5 ++--
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs
index bed1564686..970a718795 100644
--- a/GVFS/GVFS.Common/Git/GitAuthentication.cs
+++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs
@@ -91,9 +91,27 @@ public void RejectCredentials(ITracer tracer, string credentialString)
         {
             lock (this.gitAuthLock)
             {
+                var 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.
+                    var 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();
+                    }
                 }
             }
         }
diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs
index 648d6c4636..c083ac5862 100644
--- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs
+++ b/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();
diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs
index 26923e1803..29f2651ba6 100644
--- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs
+++ b/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 @@ protected override Result InvokeGitImpl(
                 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 @@ protected override Result InvokeGitImpl(
                     }
                 };
 
-            CommandInfo matchedCommand = this.expectedCommandInfos.Find(commandMatchFunction);
+            CommandInfo matchedCommand = this.expectedCommandInfos.Last(commandMatchFunction);
             matchedCommand.ShouldNotBeNull("Unexpected command: " + command);
 
             return matchedCommand.Result();