Better browser/interactive session detection inside of WSL #1148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ldennington
approved these changes
Mar 14, 2023
| return _wslVersion > 0; | ||
| } | ||
|
|
||
| private static int GetWslVersion(IEnvironment env, IFileSystem fs) |
There was a problem hiding this comment.
Comments were really helpful in understanding what this method was doing - thanks!
| return false; | ||
| } | ||
|
|
||
| // |
There was a problem hiding this comment.
Out of curiosity, when should one add // <empty line> before a comment versus // <comment>?
There was a problem hiding this comment.
It's more personal taste. When a comment block ends up feeling like a novel with multiple paragraphs, I think it's easier to read (and stands out more as something important to understand!) with surrounding whitespace.
So let's say "a comment block with multiple paragraphs should have a // blank line at start and end"?
Comment on lines
+12
to
+13
| !string.IsNullOrWhiteSpace(System.Environment.GetEnvironmentVariable("DISPLAY")) || | ||
| !string.IsNullOrWhiteSpace(System.Environment.GetEnvironmentVariable("WAYLAND_DISPLAY")); |
There was a problem hiding this comment.
Why the addition of System here and removal of the using?
There was a problem hiding this comment.
Ahh.. good point. It's using System.Environment because we were using the BCL Environment class to read these variables initally.
Now we've added an IEnvironment instance to the SessionManager, there's a property named Environment (same name), so the namespace prefix is required to still reference the BCL static class.
But.. we should probably be using the IEnvironment here now that we have it.
There was a problem hiding this comment.
Pushed a new version, with a new commit at the front of the series that refactors the IEnvironment hierarchy a bit to remove a code smell.
We had a virtual member call in the constructor of the various IEnvironment implementations, which is a code smell. Instead, lazily populate the `_variables` cache dictionary, and add an explict `Refresh` method to refresh the cache. We call `Refresh` in the `SetEnvironmentVariable` method for convenience. For testing we simply pipe through the pre-computed variables in the `internal` constructor.
Also check for the WAYLAND_DISPLAY environment variable when determining if a graphical session/display exists for POSIX systems.
Add abstractions to the IFileSystem interface for enumerating directories and reading the entire contents of a file to a string.
Split the web browser check from the desktop session checks. In future commits we will enhance the browser-check to take in to account the WSL special case.
Add methods to the WslUtils class to enable detection of a WSL distribution, and also determine which WSL version is being used. Version 1 uses the Windows NT kernel and runs the distribution in the same user-mode space as Windows programs. Version 2 uses a light-weight VM to host a real Linux kernel, and runs the distribution also inside the VM; interop with Windows is achieved using other mechanisms.
Add a basic INI file deserialiser
Add ability to launch cmd.exe or PowerShell.exe scripts from inside a WSL distribution. In order to discover the location of cmd.exe/powershell.exe we need search the Windows file system that's mounted by default /mnt/c. We inspect the /etc/wsl.conf file to respect users who have changed the default mount location for Windows drives.
In order to detect if we have an interactive Windows desktop session from inside WSL we need to 'punch out' from WSL to determine the session ID and window station. Strictly speaking, except for session 0 (from Vista onwards), any Windows session can have exactly one interactive window station (always called WinSta0). However, because we cannot easily determine the window station name from a simple cmd/powershell script, we take a simplified approach which isn't 100% accurate. Instead, we only permit browser auth methods if we are NOT in Windows session 0; any other Windows session we assume we are in WinSta0. The default OpenSSH Server configuration (Windows 10+) has `sshd` running as the built-in NT user NETWORK_SERVICE, which means it runs in session 0 (the services session). This is most common scenario, other than using WSL from a 'real', interactive Windows session that we're likely to face.
9 tasks
mjcheetham
added a commit
that referenced
this pull request
May 2, 2023
**Changes:** - Support ports in URL-scoped config (#825) - Support URL-scoped enterprise default settings (#1149) - Add support for client TLS certificates (#1152) - Add TRACE2 support(#1131, #1151, #1156, #1162) - Better browser detection inside of WSL (#1148) - Handle expired OAuth refresh token for generic auth (#1196) - Target *-latest runner images in CI workflow (#1178) - Various bug fixes: - Ensure we create a WindowsProcessManager on Windows (#1146) - Ensure we start child processes created with ProcessManager (#1177) - Fix app path name of Windows dropping file extension (#1181) - Ensure we init IEnvironment before SessionManager (#1167) - git: consistently read from stdout before exit wait (#1136) - trace2: guard against null pipe client in dispose (#1135) - Make Avalonia UI the default Windows and move to in-process (#1207) - Add Git configuration options for trace & debug (#1228) - Transition from Nerdbank.GitVersioning to a version file (#1231) - Add support for using the current Windows user for WAM on DevBox (#1197) - Various documentation updates: - org-rename: update references to GitCredentialManager (#1141) - issue templates: remove core suffix (#1180) - readme: add link to project roadmap (#1204) - docs: add bitbucket app password requirements (#1213) - .net tool: clarify install instructions (#1126) - docs: call out different GCM install paths in WSL docs (#1168) - docs: add trace2 to config/env documentation (#1230)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today we're gating browser-based authentication mechanisms behind a GUI-possible session, which isn't 100% correct. When inside of a WSL session, without WSLg enabled, it is still possible to launch a web browser in the hosting Windows session.
Split the determination between "GUI session" and "browser possible", and add the special case for WSL inside an interactive Windows session.
Things are complicated here because it is possible to SSH in to the WSL instance from outside of the Windows host, meaning the Windows session also doesn't have UI. This case we still need to prevent browser-based options.
In order to detect these cases we need to 'punch out' to the Windows host and run a script/cmdlet to determine the session type.
Fixes #878