Skip to content

Commit 04b949a

Browse files
mjcheethamkyle-rader-msft
mjcheetham
and
kyle-rader-msft
committed
linuxsessionmgr: make browser detection more robust
In non-WSL environments we make our checks for a browser more robust by checking for a 'shell execute' handler (like xdg-open). Previously we just relied on a desktop session ($DISPLAY or $WAYLAND_DISPLAY), which isn't accurate since in `OpenBrowser` we require a shell execute handler. The check and implementation are now aligned! We refactor the method to avoid nesting `if`s and use early returns to make things a little clearer. Helped-by: Marc Becker <becm@gmx.de> Co-authored-by: Kyle Rader <kyrader@microsoft.com> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
1 parent 1d07064 commit 04b949a

6 files changed

Lines changed: 44 additions & 35 deletions

File tree

‎src/shared/Core/CommandContext.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ public CommandContext()
103103
{
104104
FileSystem = new WindowsFileSystem();
105105
Environment = new WindowsEnvironment(FileSystem);
106-
SessionManager = new WindowsSessionManager(Environment, FileSystem);
106+
SessionManager = new WindowsSessionManager(Trace, Environment, FileSystem);
107107
ProcessManager = new WindowsProcessManager(Trace2);
108108
Terminal = new WindowsTerminal(Trace, Trace2);
109109
string gitPath = GetGitPath(Environment, FileSystem, Trace);
@@ -120,7 +120,7 @@ public CommandContext()
120120
{
121121
FileSystem = new MacOSFileSystem();
122122
Environment = new MacOSEnvironment(FileSystem);
123-
SessionManager = new MacOSSessionManager(Environment, FileSystem);
123+
SessionManager = new MacOSSessionManager(Trace, Environment, FileSystem);
124124
ProcessManager = new ProcessManager(Trace2);
125125
Terminal = new MacOSTerminal(Trace, Trace2);
126126
string gitPath = GetGitPath(Environment, FileSystem, Trace);
@@ -137,7 +137,7 @@ public CommandContext()
137137
{
138138
FileSystem = new LinuxFileSystem();
139139
Environment = new PosixEnvironment(FileSystem);
140-
SessionManager = new LinuxSessionManager(Environment, FileSystem);
140+
SessionManager = new LinuxSessionManager(Trace, Environment, FileSystem);
141141
ProcessManager = new ProcessManager(Trace2);
142142
Terminal = new LinuxTerminal(Trace, Trace2);
143143
string gitPath = GetGitPath(Environment, FileSystem, Trace);

‎src/shared/Core/ISessionManager.cs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,17 @@ public static void OpenBrowser(this ISessionManager sm, string url)
4040

4141
public abstract class SessionManager : ISessionManager
4242
{
43+
protected ITrace Trace { get; }
4344
protected IEnvironment Environment { get; }
4445
protected IFileSystem FileSystem { get; }
4546

46-
protected SessionManager(IEnvironment env, IFileSystem fs)
47+
protected SessionManager(ITrace trace, IEnvironment env, IFileSystem fs)
4748
{
49+
EnsureArgument.NotNull(trace, nameof(trace));
4850
EnsureArgument.NotNull(env, nameof(env));
4951
EnsureArgument.NotNull(fs, nameof(fs));
5052

53+
Trace = trace;
5154
Environment = env;
5255
FileSystem = fs;
5356
}
@@ -69,6 +72,7 @@ public void OpenBrowser(Uri uri)
6972

7073
protected virtual void OpenBrowserInternal(string url)
7174
{
75+
Trace.WriteLine("Opening browser using framework shell-execute: " + url);
7276
var psi = new ProcessStartInfo(url) { UseShellExecute = true };
7377
Process.Start(psi);
7478
}

‎src/shared/Core/Interop/Linux/LinuxSessionManager.cs‎

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ public class LinuxSessionManager : PosixSessionManager
88
{
99
private bool? _isWebBrowserAvailable;
1010

11-
public LinuxSessionManager(IEnvironment env, IFileSystem fs) : base(env, fs)
11+
public LinuxSessionManager(ITrace trace, IEnvironment env, IFileSystem fs) : base(trace, env, fs)
1212
{
1313
PlatformUtils.EnsureLinux();
1414
}
@@ -41,6 +41,8 @@ protected override void OpenBrowserInternal(string url)
4141
throw new Exception("Failed to locate a utility to launch the default web browser.");
4242
}
4343

44+
Trace.WriteLine($"Opening browser using '{shellExecPath}: {url}");
45+
4446
var psi = new ProcessStartInfo(shellExecPath, url)
4547
{
4648
RedirectStandardOutput = true,
@@ -53,44 +55,47 @@ protected override void OpenBrowserInternal(string url)
5355

5456
private bool GetWebBrowserAvailable()
5557
{
58+
// We need a shell execute handler to be able to launch to browser
59+
if (!TryGetShellExecuteHandler(Environment, out _))
60+
{
61+
Trace.WriteLine("Could not locate a shell execute handler for Linux - browser is not available.");
62+
return false;
63+
}
64+
5665
// If this is a Windows Subsystem for Linux distribution we may
57-
// be able to launch the web browser of the host Windows OS.
66+
// be able to launch the web browser of the host Windows OS, but
67+
// there are further checks to do on the Windows host's session.
68+
//
69+
// If we are in Windows logon session 0 then the user can never interact,
70+
// even in the WinSta0 window station. This is typical when SSH-ing into a
71+
// Windows 10+ machine using the default OpenSSH Server configuration,
72+
// which runs in the 'services' session 0.
73+
//
74+
// If we're in any other session, and in the WinSta0 window station then
75+
// the user can possibly interact. However, since it's hard to determine
76+
// the window station from PowerShell cmdlets (we'd need to write P/Invoke
77+
// code and that's just messy and too many levels of indirection quite
78+
// frankly!) we just assume any non session 0 is interactive.
79+
//
80+
// This assumption doesn't hold true if the user has changed the user that
81+
// the OpenSSH Server service runs as (not a built-in NT service) *AND*
82+
// they've SSH-ed into the Windows host (and then started a WSL shell).
83+
// This feels like a very small subset of users...
84+
//
5885
if (WslUtils.IsWslDistribution(Environment, FileSystem, out _))
5986
{
60-
// We need a shell execute handler to be able to launch to browser
61-
if (!TryGetShellExecuteHandler(Environment, out _))
62-
{
63-
return false;
64-
}
65-
66-
//
67-
// If we are in Windows logon session 0 then the user can never interact,
68-
// even in the WinSta0 window station. This is typical when SSH-ing into a
69-
// Windows 10+ machine using the default OpenSSH Server configuration,
70-
// which runs in the 'services' session 0.
71-
//
72-
// If we're in any other session, and in the WinSta0 window station then
73-
// the user can possibly interact. However, since it's hard to determine
74-
// the window station from PowerShell cmdlets (we'd need to write P/Invoke
75-
// code and that's just messy and too many levels of indirection quite
76-
// frankly!) we just assume any non session 0 is interactive.
77-
//
78-
// This assumption doesn't hold true if the user has changed the user that
79-
// the OpenSSH Server service runs as (not a built-in NT service) *AND*
80-
// they've SSH-ed into the Windows host (and then started a WSL shell).
81-
// This feels like a very small subset of users...
82-
//
8387
if (WslUtils.GetWindowsSessionId(FileSystem) == 0)
8488
{
89+
Trace.WriteLine("This is a WSL distribution, but Windows session 0 was detected - browser is not available.");
8590
return false;
8691
}
8792

88-
// If we are not in session 0, or we cannot get the Windows session ID,
89-
// assume that we *CAN* launch the browser so that users are never blocked.
93+
// Not on session 0 - we assume the user can interact with browser on Windows.
94+
Trace.WriteLine("This is a WSL distribution - browser is available.");
9095
return true;
9196
}
9297

93-
// We require an interactive desktop session to be able to launch a browser
98+
// We need a desktop session to be able to launch the browser in the general case
9499
return IsDesktopSession;
95100
}
96101

‎src/shared/Core/Interop/MacOS/MacOSSessionManager.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ namespace GitCredentialManager.Interop.MacOS
55
{
66
public class MacOSSessionManager : PosixSessionManager
77
{
8-
public MacOSSessionManager(IEnvironment env, IFileSystem fs) : base(env, fs)
8+
public MacOSSessionManager(ITrace trace, IEnvironment env, IFileSystem fs) : base(trace, env, fs)
99
{
1010
PlatformUtils.EnsureMacOS();
1111
}

‎src/shared/Core/Interop/Posix/PosixSessionManager.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ namespace GitCredentialManager.Interop.Posix
22
{
33
public abstract class PosixSessionManager : SessionManager
44
{
5-
protected PosixSessionManager(IEnvironment env, IFileSystem fs) : base(env, fs)
5+
protected PosixSessionManager(ITrace trace, IEnvironment env, IFileSystem fs) : base(trace, env, fs)
66
{
77
PlatformUtils.EnsurePosix();
88
}

‎src/shared/Core/Interop/Windows/WindowsSessionManager.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ namespace GitCredentialManager.Interop.Windows
55
{
66
public class WindowsSessionManager : SessionManager
77
{
8-
public WindowsSessionManager(IEnvironment env, IFileSystem fs) : base(env, fs)
8+
public WindowsSessionManager(ITrace trace, IEnvironment env, IFileSystem fs) : base(trace, env, fs)
99
{
1010
PlatformUtils.EnsureWindows();
1111
}

0 commit comments

Comments
 (0)