From 8aa8db02cfaec066ac904cb1aa041de0dfc34b2f Mon Sep 17 00:00:00 2001 From: Carl in 't Veld Date: Tue, 13 Oct 2026 13:32:02 +0200 Subject: [PATCH] entra: serialize interactive sign-in across GCM processes When a cached token expires, every GCM process that needs it (for example one per Git worktree, or one per parallel fetch) falls back to interactive authentication on its own. They share one MSAL token cache, so only one sign-in is needed, but after it completes the other processes never look at the cache again and keep prompting or waiting. Take a per-client, per-authority lock file in the GCM data directory before starting an interactive or broker flow. Once the lock is held, resolve the account again and retry silent authentication, so a token stored by another process is used without a second prompt. Waiting honours cancellation and gives up after five minutes. If the lock cannot be taken, authentication continues as before. Verified with four parallel `get` requests against an expired account: before, four device-code flows started; now one starts and three wait. Assisted-by: Claude Sonnet 5.5 Co-Authored-By: Claude Sonnet 5.5 --- docs/hostprovider.md | 11 ++ ...EntraAuthenticationInteractiveLockTests.cs | 128 ++++++++++++++++++ .../Entra/EntraAuthentication.PublicClient.cs | 74 ++++++++++ 3 files changed, 213 insertions(+) create mode 100644 src/Core.Tests/Authentication/Entra/EntraAuthenticationInteractiveLockTests.cs diff --git a/docs/hostprovider.md b/docs/hostprovider.md index 4fb56df00..d49b092f9 100644 --- a/docs/hostprovider.md +++ b/docs/hostprovider.md @@ -215,6 +215,17 @@ available and a terminal/TTY is attached then a provider MUST first check if terminal prompts are enabled (`ISettings.IsTerminalPromptsEnabled`), and an exception MUST be thrown if interaction has been disallowed. +Several GCM processes can need the same credential at the same time, for +example one per Git worktree when a shared cached token has expired. Prompting +the user once per process is not useful when a single sign-in satisfies all of +them. Where the providers share a token cache, a host provider SHOULD serialize +interactive authentication across processes and, once it holds the lock, retry +the non-interactive path before prompting, because another process may have +completed the sign-in in the meantime. The Microsoft authentication (Entra) +helper does this with a lock file in the GCM data directory, `entra-signin-*.lock` +(one per client, authority and cache). If the lock cannot be taken, for example +because the data directory is not writable, authentication continues without it. + User-facing output and prompts SHOULD use `ICommandContext.Console`. Output-only messages go to stderr; interactive prompts use the controlling terminal. Providers MUST NOT consume Git's stdin or write user messages to its diff --git a/src/Core.Tests/Authentication/Entra/EntraAuthenticationInteractiveLockTests.cs b/src/Core.Tests/Authentication/Entra/EntraAuthenticationInteractiveLockTests.cs new file mode 100644 index 000000000..be1824dff --- /dev/null +++ b/src/Core.Tests/Authentication/Entra/EntraAuthenticationInteractiveLockTests.cs @@ -0,0 +1,128 @@ +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; +using GitCredentialManager.Authentication.Entra; +using GitCredentialManager.Tests.Objects; +using Xunit; + +namespace GitCredentialManager.Tests.Authentication.Entra; + +public class EntraAuthenticationInteractiveLockTests : IDisposable +{ + private const string Authority = "https://login.microsoftonline.com/tenant-1"; + + private readonly TestCommandContext _context = new(); + private readonly string _root; + + public EntraAuthenticationInteractiveLockTests() + { + // The lock is a real file, so back the test file system with a real directory. + _root = Path.Combine(Path.GetTempPath(), $"gcmtest-lock-{Guid.NewGuid():N}"); + _context.FileSystem.UserDataDirectoryPath = Path.Combine(_root, ".gcm"); + Directory.CreateDirectory(_context.FileSystem.UserDataDirectoryPath); + } + + public void Dispose() + { + try { Directory.Delete(_root, recursive: true); } catch { /* best effort */ } + } + + private EntraAuthentication CreateAuth(string clientId = "client-1") => + new(_context, new PublicClientConfig { ClientId = clientId }); + + [Fact] + public async Task AcquireInteractiveLockAsync_NotHeld_ReturnsLock() + { + var auth = CreateAuth(); + + using IDisposable handle = await auth.AcquireInteractiveLockAsync(Authority, CancellationToken.None); + + Assert.NotNull(handle); + } + + [Fact] + public async Task AcquireInteractiveLockAsync_HeldByOther_WaitsUntilReleased() + { + var auth = CreateAuth(); + IDisposable first = await auth.AcquireInteractiveLockAsync(Authority, CancellationToken.None); + Assert.NotNull(first); + + Task second = auth.AcquireInteractiveLockAsync( + Authority, CancellationToken.None, TimeSpan.FromSeconds(30)); + + await Task.Delay(TimeSpan.FromSeconds(1)); + Assert.False(second.IsCompleted, "The second caller must wait while the first holds the lock."); + + first.Dispose(); + + using IDisposable handle = await second.WaitAsync(TimeSpan.FromSeconds(10)); + Assert.NotNull(handle); + } + + [Fact] + public async Task AcquireInteractiveLockAsync_HeldByOtherPastTimeout_ReturnsNull() + { + var auth = CreateAuth(); + using IDisposable first = await auth.AcquireInteractiveLockAsync(Authority, CancellationToken.None); + Assert.NotNull(first); + + IDisposable second = await auth.AcquireInteractiveLockAsync( + Authority, CancellationToken.None, TimeSpan.FromMilliseconds(100)); + + // Locking must never block authentication for good: the caller continues without it. + Assert.Null(second); + } + + [Fact] + public async Task AcquireInteractiveLockAsync_DifferentAuthority_DoesNotWait() + { + var auth = CreateAuth(); + using IDisposable first = await auth.AcquireInteractiveLockAsync(Authority, CancellationToken.None); + Assert.NotNull(first); + + using IDisposable second = await auth.AcquireInteractiveLockAsync( + "https://login.microsoftonline.com/tenant-2", CancellationToken.None, TimeSpan.FromMilliseconds(100)); + + Assert.NotNull(second); + } + + [Fact] + public async Task AcquireInteractiveLockAsync_DifferentClientId_DoesNotWait() + { + using IDisposable first = await CreateAuth("client-1") + .AcquireInteractiveLockAsync(Authority, CancellationToken.None); + Assert.NotNull(first); + + using IDisposable second = await CreateAuth("client-2") + .AcquireInteractiveLockAsync(Authority, CancellationToken.None, TimeSpan.FromMilliseconds(100)); + + Assert.NotNull(second); + } + + [Fact] + public async Task AcquireInteractiveLockAsync_Canceled_Throws() + { + var auth = CreateAuth(); + using IDisposable first = await auth.AcquireInteractiveLockAsync(Authority, CancellationToken.None); + Assert.NotNull(first); + + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(200)); + + await Assert.ThrowsAnyAsync(() => + auth.AcquireInteractiveLockAsync(Authority, cts.Token, TimeSpan.FromSeconds(30))); + } + + [Fact] + public async Task AcquireInteractiveLockAsync_UserDataDirectoryUnusable_ReturnsNull() + { + // A file where the directory should be: the lock file can never be created. + string blocker = Path.Combine(_root, "blocker"); + File.WriteAllText(blocker, string.Empty); + _context.FileSystem.UserDataDirectoryPath = Path.Combine(blocker, ".gcm"); + + IDisposable handle = await CreateAuth().AcquireInteractiveLockAsync(Authority, CancellationToken.None); + + Assert.Null(handle); + } +} diff --git a/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs b/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs index e0d506499..37019ba1f 100644 --- a/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs +++ b/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs @@ -218,6 +218,26 @@ public async Task GetTokenForUserAsync(string[] scop ThrowIfUserInteractionDisabled(); + // Many GCM processes can need to sign in at the same time (e.g. one per worktree after the cached + // token expired). They all share one token cache, so only one needs to prompt the user. Take a lock + // so the others wait, then retry silent authentication to pick up the token the first one stored. + using var interactiveLock = await AcquireInteractiveLockAsync(authority, ct); + if (interactiveLock is not null) + { + // The account may only have reached the cache while we were waiting, so resolve it again. + if (account is not null) + { + msalAccount = await ResolveAccountAsync(app, account) ?? msalAccount; + } + + result = await GetTokenForUserSilentAsync(app, scopes, msalAccount, ct); + if (result is not null) + { + Trace2.WriteData(Trace2Category, "flow", "silent_after_wait"); + return AuthResult.FromMsalResult(result); + } + } + // Try interactive auth if we couldn't do so with a cached account if (useBroker) { @@ -233,6 +253,60 @@ public async Task GetTokenForUserAsync(string[] scop return AuthResult.FromMsalResult(result); } + /// + /// Serialize interactive sign-in across GCM processes that share the same token cache. + /// + /// + /// The held lock, or if it could not be taken (in which case the caller proceeds + /// without it, as before). + /// + internal /* for testing purposes */ async Task AcquireInteractiveLockAsync( + string authority, CancellationToken ct, TimeSpan? timeout = null) + { + string key = $"{PublicClientConfig.ClientId}|{authority}|{PublicClientConfig.UseSharedCache}" + .ToLowerInvariant(); + string hash = Convert.ToHexString( + System.Security.Cryptography.SHA256.HashData(Encoding.UTF8.GetBytes(key)))[..16]; + + string dir = Context.FileSystem.UserDataDirectoryPath; + string path = System.IO.Path.Combine(dir, $"entra-signin-{hash}.lock"); + + timeout ??= TimeSpan.FromMinutes(5); + var sw = Stopwatch.StartNew(); + try + { + Context.FileSystem.CreateDirectory(dir); + + while (true) + { + ct.ThrowIfCancellationRequested(); + try + { + // The OS releases the handle if this process exits or crashes, so no stale locks. + var stream = new System.IO.FileStream(path, System.IO.FileMode.OpenOrCreate, + System.IO.FileAccess.ReadWrite, System.IO.FileShare.None); + Trace2.WriteData(Trace2Category, "signin_lock", "acquired"); + return stream; + } + catch (System.IO.IOException ex) when (ex is not (System.IO.FileNotFoundException or + System.IO.DirectoryNotFoundException) && + sw.Elapsed < timeout) + { + Context.Trace.WriteLine("Another GCM process is signing in; waiting for it to finish..."); + await Task.Delay(TimeSpan.FromMilliseconds(500), ct); + } + } + } + catch (Exception ex) when (ex is System.IO.IOException or UnauthorizedAccessException) + { + // Locking is an optimisation; never fail authentication because of it. + Trace2.WriteData(Trace2Category, "signin_lock", "unavailable"); + Context.Trace.WriteLine("Unable to take the sign-in lock; continuing without it."); + Context.Trace.WriteException(ex); + return null; + } + } + private async Task GetTokenForUserSilentAsync( IPublicClientApplication app, string[] scopes, IAccount msalAccount, CancellationToken ct) {