From 99dea8e638af4b9e30a9cf4f1d83dea0cd686862 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 10 Jul 2023 10:52:35 -0700 Subject: [PATCH 1/9] msauth: let caller select if MSA-PT behaviour is used Let the caller in to the `IMicrosoftAuthentication` component decide if Microsoft Account Passthrough (MSA-PT) behaviour should be used. Azure DevOps requires MSA-PT, so set that to `true` in usages. --- .../Authentication/MicrosoftAuthenticationTests.cs | 2 +- .../Core/Authentication/MicrosoftAuthentication.cs | 10 +++++----- .../AzureReposHostProviderTests.cs | 14 +++++++------- .../Microsoft.AzureRepos/AzureReposHostProvider.cs | 6 ++++-- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/shared/Core.Tests/Authentication/MicrosoftAuthenticationTests.cs b/src/shared/Core.Tests/Authentication/MicrosoftAuthenticationTests.cs index 9ab5770c8..ef0f50a86 100644 --- a/src/shared/Core.Tests/Authentication/MicrosoftAuthenticationTests.cs +++ b/src/shared/Core.Tests/Authentication/MicrosoftAuthenticationTests.cs @@ -24,7 +24,7 @@ public async System.Threading.Tasks.Task MicrosoftAuthentication_GetAccessTokenA var msAuth = new MicrosoftAuthentication(context); await Assert.ThrowsAsync( - () => msAuth.GetTokenAsync(authority, clientId, redirectUri, scopes, userName)); + () => msAuth.GetTokenAsync(authority, clientId, redirectUri, scopes, userName, false)); } } } diff --git a/src/shared/Core/Authentication/MicrosoftAuthentication.cs b/src/shared/Core/Authentication/MicrosoftAuthentication.cs index 900cb8d65..19efd90e7 100644 --- a/src/shared/Core/Authentication/MicrosoftAuthentication.cs +++ b/src/shared/Core/Authentication/MicrosoftAuthentication.cs @@ -23,7 +23,7 @@ namespace GitCredentialManager.Authentication public interface IMicrosoftAuthentication { Task GetTokenAsync(string authority, string clientId, Uri redirectUri, - string[] scopes, string userName); + string[] scopes, string userName, bool msaPt = false); } public interface IMicrosoftAuthenticationResult @@ -59,7 +59,7 @@ public MicrosoftAuthentication(ICommandContext context) #region IMicrosoftAuthentication public async Task GetTokenAsync( - string authority, string clientId, Uri redirectUri, string[] scopes, string userName) + string authority, string clientId, Uri redirectUri, string[] scopes, string userName, bool msaPt) { // Check if we can and should use OS broker authentication bool useBroker = CanUseBroker(); @@ -70,7 +70,7 @@ public MicrosoftAuthentication(ICommandContext context) try { // Create the public client application for authentication - IPublicClientApplication app = await CreatePublicClientApplicationAsync(authority, clientId, redirectUri, useBroker); + IPublicClientApplication app = await CreatePublicClientApplicationAsync(authority, clientId, redirectUri, useBroker, msaPt); AuthenticationResult result = null; @@ -308,7 +308,7 @@ private async Task GetAccessTokenSilentlyAsync(IPublicClie } private async Task CreatePublicClientApplicationAsync( - string authority, string clientId, Uri redirectUri, bool enableBroker) + string authority, string clientId, Uri redirectUri, bool enableBroker, bool msaPt) { var httpFactoryAdaptor = new MsalHttpClientFactoryAdaptor(Context.HttpClientFactory); @@ -370,7 +370,7 @@ private async Task GetAccessTokenSilentlyAsync(IPublicClie new BrokerOptions(BrokerOptions.OperatingSystems.Windows) { Title = "Git Credential Manager", - MsaPassthrough = true, + MsaPassthrough = msaPt, } ); #endif diff --git a/src/shared/Microsoft.AzureRepos.Tests/AzureReposHostProviderTests.cs b/src/shared/Microsoft.AzureRepos.Tests/AzureReposHostProviderTests.cs index 80aef5dee..d7fc916e1 100644 --- a/src/shared/Microsoft.AzureRepos.Tests/AzureReposHostProviderTests.cs +++ b/src/shared/Microsoft.AzureRepos.Tests/AzureReposHostProviderTests.cs @@ -170,7 +170,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_ azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl); var msAuthMock = new Mock(MockBehavior.Strict); - msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount)) + msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true)) .ReturnsAsync(authResult); var authorityCacheMock = new Mock(MockBehavior.Strict); @@ -219,7 +219,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_ azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl); var msAuthMock = new Mock(MockBehavior.Strict); - msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount)) + msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true)) .ReturnsAsync(authResult); var authorityCacheMock = new Mock(MockBehavior.Strict); @@ -268,7 +268,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_ azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl); var msAuthMock = new Mock(MockBehavior.Strict); - msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null)) + msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true)) .ReturnsAsync(authResult); var authorityCacheMock = new Mock(MockBehavior.Strict); @@ -315,7 +315,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_ var azDevOpsMock = new Mock(MockBehavior.Strict); var msAuthMock = new Mock(MockBehavior.Strict); - msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null)) + msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true)) .ReturnsAsync(authResult); var authorityCacheMock = new Mock(MockBehavior.Strict); @@ -363,7 +363,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_ var azDevOpsMock = new Mock(MockBehavior.Strict); var msAuthMock = new Mock(MockBehavior.Strict); - msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, account)) + msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, account, true)) .ReturnsAsync(authResult); var authorityCacheMock = new Mock(MockBehavior.Strict); @@ -413,7 +413,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_NoCachedAuthorit azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl); var msAuthMock = new Mock(MockBehavior.Strict); - msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null)) + msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true)) .ReturnsAsync(authResult); var authorityCacheMock = new Mock(MockBehavior.Strict); @@ -462,7 +462,7 @@ public async Task AzureReposProvider_GetCredentialAsync_PatMode_NoExistingPat_Ge .ReturnsAsync(personalAccessToken); var msAuthMock = new Mock(MockBehavior.Strict); - msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null)) + msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true)) .ReturnsAsync(authResult); var authorityCacheMock = new Mock(MockBehavior.Strict); diff --git a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs index 353bf4a09..e696e504d 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs @@ -202,7 +202,8 @@ private async Task GeneratePersonalAccessTokenAsync(InputArguments GetClientId(), GetRedirectUri(), AzureDevOpsConstants.AzureDevOpsDefaultScopes, - null); + null, + msaPt: true); _context.Trace.WriteLineSecrets( $"Acquired Azure access token. Account='{result.AccountUpn}' Token='{{0}}'", new object[] {result.AccessToken}); @@ -293,7 +294,8 @@ private async Task GetAzureAccessTokenAsync(Inpu GetClientId(), GetRedirectUri(), AzureDevOpsConstants.AzureDevOpsDefaultScopes, - userName); + userName, + msaPt: true); _context.Trace.WriteLineSecrets( $"Acquired Azure access token. Account='{result.AccountUpn}' Token='{{0}}'", new object[] {result.AccessToken}); From 68bcc34cabca749383f3ab3bc4bae4105aac213d Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 10 Jul 2023 10:54:28 -0700 Subject: [PATCH 2/9] msauth: workaround MSAL.NET issue with MSA-PT accounts When we have a Microsoft Account (MSA) in the cache and attempt to do a silent authentication, if we're an MSA-PT app we need to specify the special MSA transfer tenant ID to make sure we get the a token silently, correctly. See the issue [1] in the MSAL repo for more information. [1] https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3077 --- .../Authentication/MicrosoftAuthentication.cs | 32 +++++++++++++++---- src/shared/Core/Constants.cs | 11 +++++++ 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/shared/Core/Authentication/MicrosoftAuthentication.cs b/src/shared/Core/Authentication/MicrosoftAuthentication.cs index 19efd90e7..386316d52 100644 --- a/src/shared/Core/Authentication/MicrosoftAuthentication.cs +++ b/src/shared/Core/Authentication/MicrosoftAuthentication.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; using System.Net.Http; using System.Threading.Tasks; using GitCredentialManager.Interop.Windows.Native; @@ -78,7 +79,7 @@ public MicrosoftAuthentication(ICommandContext context) bool hasExistingUser = !string.IsNullOrWhiteSpace(userName); if (hasExistingUser) { - result = await GetAccessTokenSilentlyAsync(app, scopes, userName); + result = await GetAccessTokenSilentlyAsync(app, scopes, userName, msaPt); } // @@ -116,7 +117,7 @@ public MicrosoftAuthentication(ICommandContext context) // account then the user may become stuck in a loop of authentication failures. if (!hasExistingUser && Context.Settings.UseMsAuthDefaultAccount) { - result = await GetAccessTokenSilentlyAsync(app, scopes, null); + result = await GetAccessTokenSilentlyAsync(app, scopes, null, msaPt); if (result is null || !await UseDefaultAccountAsync(result.Account.Username)) { @@ -281,7 +282,8 @@ internal MicrosoftAuthenticationFlowType GetFlowType() /// /// Obtain an access token without showing UI or prompts. /// - private async Task GetAccessTokenSilentlyAsync(IPublicClientApplication app, string[] scopes, string userName) + private async Task GetAccessTokenSilentlyAsync( + IPublicClientApplication app, string[] scopes, string userName, bool msaPt) { try { @@ -295,9 +297,27 @@ private async Task GetAccessTokenSilentlyAsync(IPublicClie { Context.Trace.WriteLine($"Attempting to acquire token silently for user '{userName}'..."); - // We can either call `app.GetAccountsAsync` and filter through the IAccount objects for the instance with the correct user name, - // or we can just pass the user name string we have as the `loginHint` and let MSAL do exactly that for us instead! - return await app.AcquireTokenSilent(scopes, loginHint: userName).ExecuteAsync(); + // Enumerate all accounts and find the one matching the user name + IEnumerable accounts = await app.GetAccountsAsync(); + IAccount account = accounts.FirstOrDefault(x => StringComparer.OrdinalIgnoreCase.Equals(x.Username, userName)); + if (account is null) + { + Context.Trace.WriteLine($"No cached account found for user '{userName}'..."); + return null; + } + + var atsBuilder = app.AcquireTokenSilent(scopes, account); + + // Is we are operating with an MSA passthrough app we need to ensure that we target the + // special MSA 'transfer' tenant explicitly. This is a workaround for MSAL issue: + // https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3077 + if (msaPt && Guid.TryParse(account.HomeAccountId.TenantId, out Guid homeTenantId) && + homeTenantId == Constants.MsaHomeTenantId) + { + atsBuilder = atsBuilder.WithTenantId(Constants.MsaTransferTenantId.ToString("D")); + } + + return await atsBuilder.ExecuteAsync(); } } catch (MsalUiRequiredException) diff --git a/src/shared/Core/Constants.cs b/src/shared/Core/Constants.cs index 9b82d0673..4c9c4f730 100644 --- a/src/shared/Core/Constants.cs +++ b/src/shared/Core/Constants.cs @@ -18,6 +18,17 @@ public static class Constants public static readonly Guid DevBoxPartnerId = new("e3171dd9-9a5f-e5be-b36c-cc7c4f3f3bcf"); + /// + /// Home tenant ID for Microsoft Accounts (MSA). + /// + public static readonly Guid MsaHomeTenantId = new("9188040d-6c67-4c5b-b112-36a304b66dad"); + + /// + /// Special tenant ID for transferring between Microsoft Account (MSA) native tokens + /// and AAD tokens. Only required for MSA-Passthrough applications. + /// + public static readonly Guid MsaTransferTenantId = new("f8cdef31-a31e-4b4a-93e4-5f571e91255a"); + public static class CredentialStoreNames { public const string WindowsCredentialManager = "wincredman"; From 88cae6bc4d65ad478ccfebac65971c465486df53 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Tue, 11 Jul 2023 15:16:41 -0600 Subject: [PATCH 3/9] rename: remove git-credential-manager-core symlinks Since 2 versions of Git have released since the rename of the executable from git-credential-manager-core to git-credential-manager, remove the associated symlinks and warnings, (as outlined in docs/rename.md). --- src/linux/Packaging.Linux/build.sh | 6 ---- src/linux/Packaging.Linux/pack.sh | 6 ---- src/osx/Installer.Mac/scripts/postinstall | 3 -- src/shared/Core/Constants.cs | 1 - src/shared/Git-Credential-Manager/Program.cs | 30 -------------------- src/windows/Installer.Windows/layout.ps1 | 13 ++------- 6 files changed, 3 insertions(+), 56 deletions(-) diff --git a/src/linux/Packaging.Linux/build.sh b/src/linux/Packaging.Linux/build.sh index 308530afe..3b179e22d 100755 --- a/src/linux/Packaging.Linux/build.sh +++ b/src/linux/Packaging.Linux/build.sh @@ -70,12 +70,6 @@ if [ $INSTALL_FROM_SOURCE = true ]; then "$LINK_TO/git-credential-manager" || exit 1 fi - # Create legacy symlink with older name - if [ ! -f "$LINK_TO/git-credential-manager-core" ]; then - ln -s -r "$INSTALL_TO/git-credential-manager" \ - "$LINK_TO/git-credential-manager-core" || exit 1 - fi - echo "Install complete." else # Pack diff --git a/src/linux/Packaging.Linux/pack.sh b/src/linux/Packaging.Linux/pack.sh index 3c1072d55..249ffb8e8 100755 --- a/src/linux/Packaging.Linux/pack.sh +++ b/src/linux/Packaging.Linux/pack.sh @@ -126,12 +126,6 @@ if [ ! -f "$LINK_TO/git-credential-manager" ]; then "$LINK_TO/git-credential-manager" || exit 1 fi -# Create legacy symlink with older name -if [ ! -f "$LINK_TO/git-credential-manager-core" ]; then - ln -s -r "$INSTALL_TO/git-credential-manager" \ - "$LINK_TO/git-credential-manager-core" || exit 1 -fi - dpkg-deb -Zxz --build "$DEBROOT" "$DEBPKG" || exit 1 echo $MESSAGE diff --git a/src/osx/Installer.Mac/scripts/postinstall b/src/osx/Installer.Mac/scripts/postinstall index 9e4b0bed5..9eed9aa7a 100755 --- a/src/osx/Installer.Mac/scripts/postinstall +++ b/src/osx/Installer.Mac/scripts/postinstall @@ -30,9 +30,6 @@ fi mkdir -p /usr/local/bin /bin/ln -Fs "$INSTALL_DESTINATION/git-credential-manager" /usr/local/bin/git-credential-manager -# Create legacy symlink to GCMCore in /usr/local/bin -/bin/ln -Fs "$INSTALL_DESTINATION/git-credential-manager" /usr/local/bin/git-credential-manager-core - # Configure GCM for the current user (running as the current user to avoid root # from taking ownership of ~/.gitconfig) sudo -u ${USER} "$INSTALL_DESTINATION/git-credential-manager" configure diff --git a/src/shared/Core/Constants.cs b/src/shared/Core/Constants.cs index 9b82d0673..80df2e127 100644 --- a/src/shared/Core/Constants.cs +++ b/src/shared/Core/Constants.cs @@ -210,7 +210,6 @@ public static class HelpUrls public const string GcmCredentialStores = "https://aka.ms/gcm/credstores"; public const string GcmWamComSecurity = "https://aka.ms/gcm/wamadmin"; public const string GcmAutoDetect = "https://aka.ms/gcm/autodetect"; - public const string GcmExecRename = "https://aka.ms/gcm/rename"; public const string GcmDefaultAccount = "https://aka.ms/gcm/defaultaccount"; public const string GcmMultipleUsers = "https://aka.ms/gcm/multipleusers"; } diff --git a/src/shared/Git-Credential-Manager/Program.cs b/src/shared/Git-Credential-Manager/Program.cs index bb9b02dc2..59f579b9f 100644 --- a/src/shared/Git-Credential-Manager/Program.cs +++ b/src/shared/Git-Credential-Manager/Program.cs @@ -55,36 +55,6 @@ private static void AppMain(object o) // Write the start and version events context.Trace2.Start(context.ApplicationPath, args); - // - // Git Credential Manager's executable used to be named "git-credential-manager-core" before - // dropping the "-core" suffix. In order to prevent "helper not found" errors for users who - // haven't updated their configuration, we include either a 'shim' or symlink with the old name - // that print warning messages about using the old name, and then continue execution of GCM. - // - // On Windows the shim is an exact copy of the main "git-credential-manager.exe" executable - // with the old name. We inspect argv[0] to see which executable we are launched as. - // - // On UNIX systems we do the same check, except instead of a copy we use a symlink. - // - string appPath = context.ApplicationPath; - if (!string.IsNullOrWhiteSpace(appPath)) - { - // Trim any (.exe) file extension if we're on Windows - // Note that in some circumstances (like being called by Git when config is set - // to just `helper = manager-core`) we don't always have ".exe" at the end. - if (PlatformUtils.IsWindows() && appPath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) - { - appPath = appPath.Substring(0, appPath.Length - 4); - } - if (appPath.EndsWith("git-credential-manager-core", StringComparison.OrdinalIgnoreCase)) - { - context.Streams.Error.WriteLine( - "warning: git-credential-manager-core was renamed to git-credential-manager"); - context.Streams.Error.WriteLine( - $"warning: see {Constants.HelpUrls.GcmExecRename} for more information"); - } - } - // Register all supported host providers at the normal priority. // The generic provider should never win against a more specific one, so register it with low priority. app.RegisterProvider(new AzureReposHostProvider(context), HostProviderPriority.Normal); diff --git a/src/windows/Installer.Windows/layout.ps1 b/src/windows/Installer.Windows/layout.ps1 index c42e926c8..8ae338986 100644 --- a/src/windows/Installer.Windows/layout.ps1 +++ b/src/windows/Installer.Windows/layout.ps1 @@ -54,24 +54,17 @@ dotnet publish "$GCM_UI_SRC" ` Write-Output "Publishing Bitbucket UI helper..." dotnet publish "$BITBUCKET_UI_SRC" ` --configuration "$CONFIGURATION" ` - --output "$PAYLOAD" + --output "$PAYLOAD" Write-Output "Publishing GitHub UI helper..." dotnet publish "$GITHUB_UI_SRC" ` --configuration "$CONFIGURATION" ` - --output "$PAYLOAD" + --output "$PAYLOAD" Write-Output "Publishing GitLab UI helper..." dotnet publish "$GITLAB_UI_SRC" ` --configuration "$CONFIGURATION" ` - --output "$PAYLOAD" - -# Create copy of main GCM executable with older "GCM Core" name -Copy-Item -Path "$PAYLOAD/git-credential-manager.exe" ` - -Destination "$PAYLOAD/git-credential-manager-core.exe" - -Copy-Item -Path "$PAYLOAD/git-credential-manager.exe.config" ` - -Destination "$PAYLOAD/git-credential-manager-core.exe.config" + --output "$PAYLOAD" # Delete libraries that are not needed for Windows but find their way # into the publish output. From 97d26d3d351e280316cd919cbd70739a3c146711 Mon Sep 17 00:00:00 2001 From: Adrian Wright <95350065+cyclr-adrian@users.noreply.github.com> Date: Thu, 13 Jul 2023 16:21:26 +0100 Subject: [PATCH 4/9] Update multiple-users.md --- docs/multiple-users.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/multiple-users.md b/docs/multiple-users.md index 16ec56965..a0478a765 100644 --- a/docs/multiple-users.md +++ b/docs/multiple-users.md @@ -93,7 +93,7 @@ which you should also be aware of if you're using it. You can use the `github [list | login | logout]` commands to manage your GitHub accounts. These commands are documented in the [command-line usage][cli-usage] -or by running `git-credential-manager github --help`. +or by running `git credential-manager github --help`. ## TL;DR: Tell GCM to remember which account to use From 5afa56b0964b50c29fd7e28176a0b42809479262 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 17 Jul 2023 10:09:34 -0600 Subject: [PATCH 5/9] diagnose: add http fallback uri Currently, failure to access http://example.com causes failure of the Networking Diagnostic portion of the diagnose command. To improve the experience for users who are unable to access http://example.com, this change: 1. Adds a fallback URI - if accessing http://example.com throws an exception, we now try http://httpforever.com. 2. Prints a warning when either the primary or both the primary and fallback uris throw an exception (instead of failing the Networking Diagnostic). --- .../Commands/DiagnoseCommandTests.cs | 82 +++++++++++++++++++ .../Core/Diagnostics/NetworkingDiagnostic.cs | 23 +++++- .../Objects/TestHttpMessageHandler.cs | 8 ++ 3 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 src/shared/Core.Tests/Commands/DiagnoseCommandTests.cs diff --git a/src/shared/Core.Tests/Commands/DiagnoseCommandTests.cs b/src/shared/Core.Tests/Commands/DiagnoseCommandTests.cs new file mode 100644 index 000000000..0118e9d85 --- /dev/null +++ b/src/shared/Core.Tests/Commands/DiagnoseCommandTests.cs @@ -0,0 +1,82 @@ +using System; +using System.Net.Http; +using System.Security.AccessControl; +using System.Text; +using GitCredentialManager.Diagnostics; +using GitCredentialManager.Tests.Objects; +using Xunit; + +namespace Core.Tests.Commands; + +public class DiagnoseCommandTests +{ + [Fact] + public void NetworkingDiagnostic_SendHttpRequest_Primary_OK() + { + var primaryUriString = "http://example.com"; + var sb = new StringBuilder(); + var context = new TestCommandContext(); + var networkingDiagnostic = new NetworkingDiagnostic(context); + var primaryUri = new Uri(primaryUriString); + var httpHandler = new TestHttpMessageHandler(); + var httpResponse = new HttpResponseMessage(); + var expected = $"Sending HEAD request to {primaryUriString}... OK{Environment.NewLine}"; + + httpHandler.Setup(HttpMethod.Head, primaryUri, httpResponse); + + networkingDiagnostic.SendHttpRequest(sb, new HttpClient(httpHandler)); + + httpHandler.AssertRequest(HttpMethod.Head, primaryUri, expectedNumberOfCalls: 1); + Assert.Contains(expected, sb.ToString()); + } + + [Fact] + public void NetworkingDiagnostic_SendHttpRequest_Backup_OK() + { + var primaryUriString = "http://example.com"; + var backupUriString = "http://httpforever.com"; + var sb = new StringBuilder(); + var context = new TestCommandContext(); + var networkingDiagnostic = new NetworkingDiagnostic(context); + var primaryUri = new Uri(primaryUriString); + var backupUri = new Uri(backupUriString); + var httpHandler = new TestHttpMessageHandler { SimulatePrimaryUriFailure = true }; + var httpResponse = new HttpResponseMessage(); + var expected = $"Sending HEAD request to {primaryUriString}... warning: HEAD request failed{Environment.NewLine}" + + $"Sending HEAD request to {backupUriString}... OK{Environment.NewLine}"; + + httpHandler.Setup(HttpMethod.Head, primaryUri, httpResponse); + httpHandler.Setup(HttpMethod.Head, backupUri, httpResponse); + + networkingDiagnostic.SendHttpRequest(sb, new HttpClient(httpHandler)); + + httpHandler.AssertRequest(HttpMethod.Head, primaryUri, expectedNumberOfCalls: 1); + httpHandler.AssertRequest(HttpMethod.Head, backupUri, expectedNumberOfCalls: 1); + Assert.Contains(expected, sb.ToString()); + } + + [Fact] + public void NetworkingDiagnostic_SendHttpRequest_No_Network() + { + var primaryUriString = "http://example.com"; + var backupUriString = "http://httpforever.com"; + var sb = new StringBuilder(); + var context = new TestCommandContext(); + var networkingDiagnostic = new NetworkingDiagnostic(context); + var primaryUri = new Uri(primaryUriString); + var backupUri = new Uri(backupUriString); + var httpHandler = new TestHttpMessageHandler { SimulateNoNetwork = true }; + var httpResponse = new HttpResponseMessage(); + var expected = $"Sending HEAD request to {primaryUriString}... warning: HEAD request failed{Environment.NewLine}" + + $"Sending HEAD request to {backupUriString}... warning: HEAD request failed{Environment.NewLine}"; + + httpHandler.Setup(HttpMethod.Head, primaryUri, httpResponse); + httpHandler.Setup(HttpMethod.Head, backupUri, httpResponse); + + networkingDiagnostic.SendHttpRequest(sb, new HttpClient(httpHandler)); + + httpHandler.AssertRequest(HttpMethod.Head, primaryUri, expectedNumberOfCalls: 1); + httpHandler.AssertRequest(HttpMethod.Head, backupUri, expectedNumberOfCalls: 1); + Assert.Contains(expected, sb.ToString()); + } +} diff --git a/src/shared/Core/Diagnostics/NetworkingDiagnostic.cs b/src/shared/Core/Diagnostics/NetworkingDiagnostic.cs index 310b26361..50ab5b4da 100644 --- a/src/shared/Core/Diagnostics/NetworkingDiagnostic.cs +++ b/src/shared/Core/Diagnostics/NetworkingDiagnostic.cs @@ -12,6 +12,7 @@ namespace GitCredentialManager.Diagnostics public class NetworkingDiagnostic : Diagnostic { private const string TestHttpUri = "http://example.com"; + private const string TestHttpUriFallback = "http://httpforever.com"; private const string TestHttpsUri = "https://example.com"; public NetworkingDiagnostic(ICommandContext commandContext) @@ -28,9 +29,7 @@ protected override async Task RunInternalAsync(StringBuilder log, IList RunInternalAsync(StringBuilder log, IList { TestHttpUri, TestHttpUriFallback }) + { + try + { + log.Append($"Sending HEAD request to {uri}..."); + using var httpResponse = await httpClient.HeadAsync(uri); + log.AppendLine(" OK"); + break; + } + catch (HttpRequestException) + { + log.AppendLine(" warning: HEAD request failed"); + } + } + } } } diff --git a/src/shared/TestInfrastructure/Objects/TestHttpMessageHandler.cs b/src/shared/TestInfrastructure/Objects/TestHttpMessageHandler.cs index 9dce7898f..0a2c46e18 100644 --- a/src/shared/TestInfrastructure/Objects/TestHttpMessageHandler.cs +++ b/src/shared/TestInfrastructure/Objects/TestHttpMessageHandler.cs @@ -23,6 +23,8 @@ public class TestHttpMessageHandler : HttpMessageHandler public bool ThrowOnUnexpectedRequest { get; set; } public bool SimulateNoNetwork { get; set; } + public bool SimulatePrimaryUriFailure { get; set; } + public IDictionary<(HttpMethod method, Uri uri), int> RequestCounts => _requestCounts; public void Setup(HttpMethod method, Uri uri, AsyncRequestHandler handler) @@ -80,6 +82,12 @@ protected override async Task SendAsync(HttpRequestMessage throw new HttpRequestException("Simulated no network"); } + if (SimulatePrimaryUriFailure && request.RequestUri != null && + request.RequestUri.ToString().Equals("http://example.com/")) + { + throw new HttpRequestException("Simulated http failure."); + } + foreach (var kvp in _handlers) { if (kvp.Key == requestKey) From 92cc90542e6367269bb9ad94d7b9f6609daced75 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 17 Jul 2023 13:06:27 -0600 Subject: [PATCH 6/9] Trace2FileWriter: ignore some exceptions on write These exceptions were discovered while exploring trace2 settings with a full Git client. Git can take a directory location as a trace2 target and will create new files for every process. GCM currently throws a DirectoryNotFoundException given such a parameter. Git processes somehow can append to the same file across multiple subprocesses. When GCM attempts to append to the file, it gets an UnauthorizedAccessException on Windows due to multiple writers being problematic. This exception could also happen on other platforms if the setting is pointing to a file with restricted permissions. In both of these cases, we chose to do nothing. The traces are lost, but that's better than crashing the process. Future directions could include: 1. Sending a warning over stderr if these exceptions occur, to make it clear why trace2 are not showing up. 2. Directories could be noticed as a different kind of trace target and we create a new file for the process before passing it to the Trace2FileWriter. 3. Perhaps there is a way for Git to pass the handle to the trace file so we can append to the file that Git was using. (Alternatively, Git could close the file and then reopen it after running the GCM subprocess.) Each of these issues are a bit complicated, so this quick fix is chosen as a stop gap to avoid this problem for current users. --- src/shared/Core/Trace2FileWriter.cs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/shared/Core/Trace2FileWriter.cs b/src/shared/Core/Trace2FileWriter.cs index 47f1bf19c..dbf1b5d6d 100644 --- a/src/shared/Core/Trace2FileWriter.cs +++ b/src/shared/Core/Trace2FileWriter.cs @@ -1,4 +1,5 @@ using System.IO; +using System; namespace GitCredentialManager; @@ -13,6 +14,22 @@ public Trace2FileWriter(Trace2FormatTarget formatTarget, string path) : base(for public override void Write(Trace2Message message) { - File.AppendAllText(_path, Format(message)); + try + { + File.AppendAllText(_path, Format(message)); + } + catch (DirectoryNotFoundException) + { + // Do nothing, as this either means we don't have the + // parent directories above the file, or this trace2 + // target points to a directory. + } + catch (UnauthorizedAccessException) + { + // Do nothing, as this either means the file is not + // accessible with current permissions, or we are on + // Windows and the file is currently open for writing + // by another process (likely Git itself.) + } } } From 1e09a4445612649680cdea60a5441492207bad3c Mon Sep 17 00:00:00 2001 From: shimarisu_121 <73901165+kawana77b@users.noreply.github.com> Date: Tue, 25 Jul 2023 01:49:17 +0900 Subject: [PATCH 7/9] docs: update required dotnet-sdk version in install.md Update the version notation because since the release of git-credential-manager version 2.2.1, the sdk version required for installation is .NET 7. --- docs/install.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/install.md b/docs/install.md index 08b3c358c..ce11ae08f 100644 --- a/docs/install.md +++ b/docs/install.md @@ -205,7 +205,7 @@ the preferred install method for Linux because you can use it to install on any distribution][dotnet-supported-distributions]. You can also use this method on macOS if you so choose. -**Note:** Make sure you have installed [version 6.0 of the .NET +**Note:** Make sure you have installed [version 7.0 of the .NET SDK][dotnet-install] before attempting to run the following `dotnet tool` commands. After installing, you will also need to follow the output instructions to add the tools directory to your `PATH`. From 725ab49075f1ae156d11cb33f190fe6b13ce1c9d Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 31 Jul 2023 13:10:44 -0700 Subject: [PATCH 8/9] msauth: add extra tracing of MSA-PT and ATS --- .../Authentication/MicrosoftAuthentication.cs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/shared/Core/Authentication/MicrosoftAuthentication.cs b/src/shared/Core/Authentication/MicrosoftAuthentication.cs index 386316d52..06bd7330d 100644 --- a/src/shared/Core/Authentication/MicrosoftAuthentication.cs +++ b/src/shared/Core/Authentication/MicrosoftAuthentication.cs @@ -68,6 +68,11 @@ public MicrosoftAuthentication(ICommandContext context) ? "OS broker is available and enabled." : "OS broker is not available or enabled."); + if (msaPt) + { + Context.Trace.WriteLine("MSA passthrough is enabled."); + } + try { // Create the public client application for authentication @@ -289,9 +294,11 @@ internal MicrosoftAuthenticationFlowType GetFlowType() { if (userName is null) { - Context.Trace.WriteLine("Attempting to acquire token silently for current operating system account..."); + Context.Trace.WriteLine( + "Attempting to acquire token silently for current operating system account..."); - return await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount).ExecuteAsync(); + return await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount) + .ExecuteAsync(); } else { @@ -299,7 +306,8 @@ internal MicrosoftAuthenticationFlowType GetFlowType() // Enumerate all accounts and find the one matching the user name IEnumerable accounts = await app.GetAccountsAsync(); - IAccount account = accounts.FirstOrDefault(x => StringComparer.OrdinalIgnoreCase.Equals(x.Username, userName)); + IAccount account = accounts.FirstOrDefault(x => + StringComparer.OrdinalIgnoreCase.Equals(x.Username, userName)); if (account is null) { Context.Trace.WriteLine($"No cached account found for user '{userName}'..."); @@ -325,6 +333,12 @@ internal MicrosoftAuthenticationFlowType GetFlowType() Context.Trace.WriteLine("Failed to acquire token silently; user interaction is required."); return null; } + catch (Exception ex) + { + Context.Trace.WriteLine("Failed to acquire token silently."); + Context.Trace.WriteException(ex); + return null; + } } private async Task CreatePublicClientApplicationAsync( From 8c430c9484c90433ab30c25df7fc1005fe2f4ba4 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Tue, 1 Aug 2023 11:10:05 -0600 Subject: [PATCH 9/9] version: bump to 2.3.0 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index f0cda3295..11e7722c5 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.2.2.0 +2.3.0.0