From cf6b45f355cdb6c936b75339b4086e34742211f7 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 26 Jan 2024 13:34:47 -0800 Subject: [PATCH 1/3] 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 commandMatchFunction = + Func 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(); From 90cf37d120c3ec311e4cd7ebe5471d38560e21a3 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 15 Feb 2024 08:51:42 -0800 Subject: [PATCH 2/3] Style fixes from pull request feeback --- GVFS/GVFS.Common/Git/GitAuthentication.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs index 970a718795..d82053ef56 100644 --- a/GVFS/GVFS.Common/Git/GitAuthentication.cs +++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs @@ -91,13 +91,13 @@ public void RejectCredentials(ITracer tracer, string credentialString) { lock (this.gitAuthLock) { - var cachedCredentialAtStartOfReject = this.cachedCredentialString; + string cachedCredentialAtStartOfReject = this.cachedCredentialString; // Don't stomp a different credential 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; + int attemptsBeforeCheckingExistingCredential = this.numberOfAttempts; if (this.TryCallGitCredential(tracer, out string getCredentialError)) { if (this.cachedCredentialString != cachedCredentialAtStartOfReject) From cbd0cefb9cc87744286fde23632860354f5c62ee Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 15 Feb 2024 10:01:27 -0500 Subject: [PATCH 3/3] GVFSVerb: Use OAuth credentials by default To more rapidly adopt OAuth tokens, set that as the default for GVFS repos. This will update on mount, so all users will update to this mode when they upgrade to a version including this commit. This may cause some initial frustration as the first time I ran a fetch in OAuth mode my local clone had a failure with GCM and defaulted to username/password checks over command line. The second fetch worked just fine, though. --- GVFS/GVFS/CommandLine/GVFSVerb.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/GVFS/GVFS/CommandLine/GVFSVerb.cs b/GVFS/GVFS/CommandLine/GVFSVerb.cs index 8449b5bab1..cd74600e43 100644 --- a/GVFS/GVFS/CommandLine/GVFSVerb.cs +++ b/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))