diff --git a/docs/environment.md b/docs/environment.md index 888e13adb..c6d258f52 100644 --- a/docs/environment.md +++ b/docs/environment.md @@ -177,6 +177,12 @@ Defaults to enabled. Specify the parent window handle (`HWND`) for modal dialogs on Windows. +When this variable is unset, GCM tries to parent authentication dialogs to the +console's visible root-owner window. Hidden or invalid windows are ignored. + +Explicitly setting a handle using this variable takes precedence and it is not +checked for visibility. + > [!NOTE] > This is intended for use by application developers who are calling Git and/or > GCM from their application and wish to correctly parent authentication dialogs diff --git a/src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs b/src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs index 831ae489f..b60a70558 100644 --- a/src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs +++ b/src/Core.Tests/Authentication/Entra/MsalParentWindowAdapterTests.cs @@ -10,60 +10,22 @@ public class MsalParentWindowAdapterTests [Theory] [InlineData(42, false)] [InlineData(42, true)] - [InlineData(-1, false)] - [InlineData(-1, true)] - public void GetWindow_ExplicitParent_TakesPrecedence(int parentValue, bool createIfMissing) + public void GetWindow_ExplicitParent_ReturnsParent(int parentValue, bool createIfMissing) { object parentWindow = new IntPtr(parentValue); using var adapter = new MsalParentWindowAdapter(parentWindow, createIfMissing, - () => throw new InvalidOperationException("Console lookup should not be called."), - _ => throw new InvalidOperationException("Visibility should not be checked."), _ => throw new InvalidOperationException("A progress window should not be created.")); object result = adapter.GetWindow(); Assert.Same(parentWindow, result); } - [Theory] - [InlineData(false)] - [InlineData(true)] - public void GetWindow_VisibleConsoleRoot_PrecedesProgressWindow(bool createIfMissing) - { - var consoleRoot = new IntPtr(42); - bool visibilityChecked = false; - using var adapter = new MsalParentWindowAdapter(IntPtr.Zero, createIfMissing, - () => consoleRoot, - hwnd => - { - Assert.Equal(consoleRoot, hwnd); - visibilityChecked = true; - return true; - }, - _ => throw new InvalidOperationException("A progress window should not be created.")); - - IntPtr result = Assert.IsType(adapter.GetWindow()); - - Assert.Equal(consoleRoot, result); - Assert.True(visibilityChecked); - } - - [Theory] - [InlineData(0)] - [InlineData(42)] - public void GetWindow_NoVisibleConsoleRoot_CreatesProgressWhenRequired(int consoleRootValue) + [Fact] + public void GetWindow_NoParent_Required_CreatesProgressWindow() { - var consoleRoot = new IntPtr(consoleRootValue); - var progressWindow = new IntPtr(84); - int visibilityChecks = 0; + var progressWindow = new IntPtr(42); var windowToken = CancellationToken.None; using (var adapter = new MsalParentWindowAdapter(IntPtr.Zero, true, - () => consoleRoot, - hwnd => - { - Assert.Equal(consoleRoot, hwnd); - visibilityChecks++; - return false; - }, ct => { windowToken = ct; @@ -73,7 +35,6 @@ public void GetWindow_NoVisibleConsoleRoot_CreatesProgressWhenRequired(int conso IntPtr result = Assert.IsType(adapter.GetWindow()); Assert.Equal(progressWindow, result); - Assert.Equal(consoleRootValue == 0 ? 0 : 1, visibilityChecks); Assert.True(windowToken.CanBeCanceled); Assert.False(windowToken.IsCancellationRequested); } @@ -81,58 +42,14 @@ public void GetWindow_NoVisibleConsoleRoot_CreatesProgressWhenRequired(int conso Assert.True(windowToken.IsCancellationRequested); } - [Theory] - [InlineData(0)] - [InlineData(42)] - public void GetWindow_NoVisibleConsoleRoot_ReturnsNullWhenParentNotRequired(int consoleRootValue) - { - var consoleRoot = new IntPtr(consoleRootValue); - int visibilityChecks = 0; - using var adapter = new MsalParentWindowAdapter(IntPtr.Zero, false, - () => consoleRoot, - hwnd => - { - Assert.Equal(consoleRoot, hwnd); - visibilityChecks++; - return false; - }, - _ => throw new InvalidOperationException("A progress window should not be created.")); - - object result = adapter.GetWindow(); - - Assert.Null(result); - Assert.Equal(consoleRootValue == 0 ? 0 : 1, visibilityChecks); - } - [Fact] - public void GetWindow_VisibilityCheckThrows_PropagatesException() + public void GetWindow_NoParent_NotRequired_ReturnsNull() { - var exception = new InvalidOperationException("Visibility check failed."); - using var adapter = new MsalParentWindowAdapter(IntPtr.Zero, true, - () => new IntPtr(42), - _ => throw exception, + using var adapter = new MsalParentWindowAdapter(IntPtr.Zero, false, _ => throw new InvalidOperationException("A progress window should not be created.")); - var thrown = Assert.Throws(adapter.GetWindow); - Assert.Same(exception, thrown); - } - - [Fact] - public void Create_ExplicitParent_IsReturned() - { - object parentWindow = new IntPtr(42); - using var adapter = MsalParentWindowAdapter.Create(parentWindow, true); - object result = adapter.GetWindow(); - Assert.Same(parentWindow, result); - } - - [PosixFact] - public void Create_NoExplicitParent_Posix_ReturnsNull() - { - using var adapter = MsalParentWindowAdapter.Create(IntPtr.Zero); - object result = adapter.GetWindow(); Assert.Null(result); } } diff --git a/src/Core/Authentication/AuthenticationBase.cs b/src/Core/Authentication/AuthenticationBase.cs index fb177d112..5bfc1a9d0 100644 --- a/src/Core/Authentication/AuthenticationBase.cs +++ b/src/Core/Authentication/AuthenticationBase.cs @@ -5,7 +5,6 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; -using GitCredentialManager.Tty; using GitCredentialManager.UI.ViewModels; namespace GitCredentialManager.Authentication @@ -125,11 +124,19 @@ protected void ThrowIfWindowCancelled(WindowViewModel viewModel) protected IntPtr GetParentWindowHandle() { + // Check if the user has specified a parent window ID if (int.TryParse(Context.Settings.ParentWindowId, out int id)) { return new IntPtr(id); } + // Check if we can use the console window as a parent + IntPtr consoleParent = PlatformUtils.GetConsoleParentWindow(); + if (consoleParent != IntPtr.Zero) + { + return consoleParent; + } + return IntPtr.Zero; } diff --git a/src/Core/Authentication/Entra/MsalAdaptors.cs b/src/Core/Authentication/Entra/MsalAdaptors.cs index e169ee838..2f335649c 100644 --- a/src/Core/Authentication/Entra/MsalAdaptors.cs +++ b/src/Core/Authentication/Entra/MsalAdaptors.cs @@ -1,7 +1,6 @@ using System; using System.Net.Http; using System.Threading; -using GitCredentialManager.Interop.Windows.Native; using GitCredentialManager.UI.Controls; using Microsoft.Identity.Client; @@ -31,48 +30,32 @@ internal class MsalParentWindowAdapter : IDisposable private readonly bool _createIfMissing; private readonly CancellationTokenSource _cts = new(); - private readonly Func _getConsoleParentWindow; - private readonly Func _isWindowVisible; private readonly Func _createWindow; public static MsalParentWindowAdapter Create(object parentWindow, bool createIfMissing = false) { - return new MsalParentWindowAdapter(parentWindow, createIfMissing, - GetConsoleParentWindow, User32.IsWindowVisible, ProgressWindow.ShowAndGetHandle); + return new MsalParentWindowAdapter(parentWindow, createIfMissing, ProgressWindow.ShowAndGetHandle); } internal MsalParentWindowAdapter(object parentWindow, bool createIfMissing, - Func getConsoleParentWindow, Func isWindowVisible, Func createWindow) { - EnsureArgument.NotNull(getConsoleParentWindow, nameof(getConsoleParentWindow)); - EnsureArgument.NotNull(isWindowVisible, nameof(isWindowVisible)); EnsureArgument.NotNull(createWindow, nameof(createWindow)); _parentWindow = parentWindow; _createIfMissing = createIfMissing; - - _getConsoleParentWindow = getConsoleParentWindow; - _isWindowVisible = isWindowVisible; _createWindow = createWindow; } public object GetWindow() { + // Try to use the parent window we were given (if any) if (_parentWindow is IntPtr p && p != IntPtr.Zero) { return _parentWindow; } - // See if we can use the console window as a parent. - // We only consider the window if it is valid and visible. - IntPtr consoleParent = _getConsoleParentWindow(); - if (consoleParent != IntPtr.Zero && _isWindowVisible(consoleParent)) - { - return consoleParent; - } - - // Create a stub window to use as a parent + // Create a stub window to use as a parent instead (if the caller requires it) if (_createIfMissing) { return _createWindow(_cts.Token); @@ -81,23 +64,6 @@ public object GetWindow() return null; } - private static IntPtr GetConsoleParentWindow() - { - // On Windows we can try and get the console window parent handle if that exists - if (!PlatformUtils.IsWindows()) - { - return IntPtr.Zero; - } - - IntPtr consoleHandle = Kernel32.GetConsoleWindow(); - - // When the parent is using ConPTY (pseudo-terminals) the console - // window may be a fake/stub (as is the case with Windows Terminal). - // This means we need to walk up the ancestor chain to get the actual - // root owner (typically the terminal emulator's window). - return User32.GetAncestor(consoleHandle, GetAncestorFlags.GetRootOwner); - } - public void Dispose() { // Close and clean up any stub window we may have created diff --git a/src/Core/PlatformUtils.cs b/src/Core/PlatformUtils.cs index cba2c4e3c..6f8dc38e9 100644 --- a/src/Core/PlatformUtils.cs +++ b/src/Core/PlatformUtils.cs @@ -5,6 +5,7 @@ using System.Runtime.InteropServices; using System.Runtime.Versioning; using GitCredentialManager.Interop.Posix.Native; +using GitCredentialManager.Interop.Windows.Native; namespace GitCredentialManager { @@ -211,6 +212,36 @@ public static bool IsElevatedUser() return false; } + public static IntPtr GetConsoleParentWindow() + { + // On Windows we can try and get the console window parent handle if that exists + if (!IsWindows()) + { + return IntPtr.Zero; + } + + IntPtr consoleHandle = Kernel32.GetConsoleWindow(); + + // When the parent is using ConPTY (pseudo-terminals) the console + // window may be a fake/stub (as is the case with Windows Terminal). + // This means we need to walk up the ancestor chain to get the actual + // root owner (typically the terminal emulator's window). + IntPtr parent = User32.GetAncestor(consoleHandle, GetAncestorFlags.GetRootOwner); + + // On Windows the parent window may be hidden (not visible) if the console is + // running in a background process (like a service). + // We do not consider a hidden window as a valid parent. We do however consider + // a minimised window as a valid parent (and the user is able to restore it and + // see our child window too). + if (User32.IsWindowVisible(parent)) + { + return parent; + } + + // No visible console window available! + return IntPtr.Zero; + } + #region Platform Entry Path Utils /// diff --git a/src/Core/UI/HelperCommand.cs b/src/Core/UI/HelperCommand.cs index f75096ddb..49492bf1f 100644 --- a/src/Core/UI/HelperCommand.cs +++ b/src/Core/UI/HelperCommand.cs @@ -16,11 +16,19 @@ public HelperCommand(ICommandContext context, string name, string description) protected IntPtr GetParentHandle() { + // Check if the user has specified a parent window ID if (int.TryParse(Context.Settings.ParentWindowId, out int id)) { return new IntPtr(id); } + // Check if we can use the console window as a parent + IntPtr consoleParent = PlatformUtils.GetConsoleParentWindow(); + if (consoleParent != IntPtr.Zero) + { + return consoleParent; + } + return IntPtr.Zero; }