diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs index 96f47673d..58c0529ad 100644 --- a/GVFS/GVFS.Common/Git/GitAuthentication.cs +++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs @@ -91,7 +91,7 @@ public void ApproveCredentials(ITracer tracer, string credentialString) } } - public void RejectCredentials(ITracer tracer, string credentialString) + public void RejectCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs) { lock (this.gitAuthLock) { @@ -102,7 +102,7 @@ public void RejectCredentials(ITracer tracer, string credentialString) // 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.TryCallGitCredential(tracer, out string getCredentialError, credentialTimeoutMs)) { if (this.cachedCredentialString != cachedCredentialAtStartOfReject) { @@ -153,7 +153,7 @@ public void RejectCredentials(ITracer tracer, string credentialString) } } - public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage) + public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage, int credentialTimeoutMs = DefaultCredentialTimeoutMs) { if (!this.isInitialized) { @@ -173,7 +173,7 @@ public bool TryGetCredentials(ITracer tracer, out string credentialString, out s return false; } - if (!this.TryCallGitCredential(tracer, out errorMessage)) + if (!this.TryCallGitCredential(tracer, out errorMessage, credentialTimeoutMs)) { return false; } diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 0f9767dde..938149ee5 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -84,6 +84,16 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli protected ITracer Tracer { get; } + // Runtime credential fetches (object/pack downloads, incl. background + // maintenance prefetch) are bounded so a missed/ignored credential prompt + // can't hang forever. We use the generous BackgroundCredentialTimeoutMs + // (120s) rather than the 30s default: this same requestor is shared by + // interactive on-demand hydration and by the user-initiated prefetch/clone + // verbs, where a human may legitimately take longer than 30s to answer a + // GCM cold-start / MFA / smartcard prompt. 120s still bounds the hang while + // being long enough that a noticed prompt is not cut off spuriously. + protected virtual int CredentialTimeoutMs => GitAuthentication.BackgroundCredentialTimeoutMs; + public static long GetNewRequestId() { return Interlocked.Increment(ref requestCount); @@ -109,7 +119,7 @@ protected GitEndPointResponseData SendRequest( string authString = null; string errorMessage; if (!this.authentication.IsAnonymous && - !this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage)) + !this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage, this.CredentialTimeoutMs)) { return new GitEndPointResponseData( HttpStatusCode.Unauthorized, @@ -234,7 +244,7 @@ protected GitEndPointResponseData SendRequest( } else if (response.StatusCode == HttpStatusCode.Unauthorized || response.StatusCode == HttpStatusCode.BadRequest || response.StatusCode == HttpStatusCode.Redirect) { - this.authentication.RejectCredentials(this.Tracer, authString); + this.authentication.RejectCredentials(this.Tracer, authString, this.CredentialTimeoutMs); if (!this.authentication.IsBackingOff) { errorMessage = string.Format("Server returned error code {0} ({1}). Your PAT may be expired and we are asking for a new one. Original error message from server: {2}", statusInt, response.StatusCode, errorMessage); diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index c083ac586..935551c2a 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -273,6 +273,48 @@ public void RejectionShouldNotBeSentIfUnderlyingTokenHasChanged() gitProcess.CredentialRejections.ShouldBeEmpty(); } + [TestCase] + public void TryGetCredentialsTimesOutWhenCredentialManagerDoesNotRespond() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string authString; + string err; + dut.TryGetCredentials(tracer, out authString, out err).ShouldEqual(true, "Initial credential fetch should succeed: " + err); + + // Override the fill command to simulate a credential manager timeout + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out: git credential fill", GitProcess.Result.GenericFailureCode), + matchPrefix: true); + + // Reject clears the cache so the next TryGetCredentials must refetch + dut.RejectCredentials(tracer, authString); + + // The re-fetch should time out + dut.TryGetCredentials(tracer, out authString, out err, credentialTimeoutMs: 1000).ShouldEqual(false, "Expected timeout to cause failure"); + err.ShouldContain("did not respond"); + } + + [TestCase] + public void TryGetCredentialsSucceedsWithExplicitTimeout() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string cred; + string err; + dut.TryGetCredentials(tracer, out cred, out err, credentialTimeoutMs: 30000).ShouldEqual(true, "Expected success with explicit timeout: " + err); + cred.ShouldNotBeNull(); + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess();