Closed
Conversation
…with the correct profile
Member
Author
|
I'm marking this one as a draft until the spec is approved. There might be some outstanding issues that we need resolving, esp. regarding throwing a dialog up to confirm the commandline before elevating it. |
This was referenced Jul 26, 2021
Member
Author
|
I'm gonna temporarily close this PR. There's gonna be a lot more delta in this area before I can get back to doing this particular element. I may as well start with a fresh branch when I do. |
4 tasks
4 tasks
ghost
pushed a commit
that referenced
this pull request
Jan 12, 2022
## Summary of the Pull Request This is the resurrection of #8514 and #11310. WE determined that we didn't want to do #11308 after all, so this should be profile auto-elevation, without the warning. This PR adds two features: * the `elevate: bool` property to profiles - If the user is running unelevated, **and** `elevate` is set to `true`, then instead of opening a new tab, we'll open an elevated Terminal window with the profile. - Otherwise, we'll just open a new tab in the existing window. This includes cases where the window is elevated, and the profile is set to `elevate:false`. `elevate:false` basically just means "do nothing special with me". * the `elevate: bool?` property to `NewTerminalArgs` (`newTab`, `splitPane`) - This allows a user to create an action that will elevate the profile, even if the profile is not otherwise set to auto-elevate. - `elevate:null` (_the default_) does not change the profile's elevation status. The action will use whatever is set by the profile. - `elevate:true` will attempt to auto-elevate the profile - `elevate:false` will do nothing special. ## References * #5000 for obvious reasons * Spec'd in #8455 ## PR Checklist * [x] Closes #632 * [x] I work here * [x] Tests added/passed * [ ] Requires documentation to be updated - sure does, but that'll come all at the end. ## Detailed Description of the Pull Request / Additional comments After playing with de-elevation a bit, it turns out it behaves weirdly with packaged applications. I can try and ask `explorer.exe` to launch the process on our behalf. However, if the thing we're launching is an execution alias (`wt.exe`), and we're elevated, then the child process will _still launch elevated_. There's also something super BODGEY at work here. `ShellExecute` is the function we use to ask the OS to elevate something for us. But `ShellExecute` needs to be able to send a window message to the process that called it (if the caller was a WINDOWS subsystem application). So if we die immediately after calling `ShellExecute`, then the elevated process never actually spawns - sad. So we're adding a helper process, `elevate-shim.exe`, that lives in our process. That'll be the one that actually calls `ShellExecute`, so that it can live for the duration of the UAC prompt. ## Validation Steps Performed * Ran tests * Opened a bunch of terminal tabs at various different elevation levels * opened new splits too * In the defaults (base layer) as well, for madness Some settings to use for testing <details> ```jsonc "keybindings" : [ ////////// ELEVATION /////////////// { "keys": "f1", "name": "ELEVATED TAB", "icon": "\uEA18", "command": { "action": "newTab", "elevate": true } }, { "keys": "f2", "name": "ELEVATED, Color", "icon": "\uEA18", "command": { "action": "newTab", "elevate": true, "commandline": "PowerShell.exe", "startingDirectory": "C:\\Windows", "tabColor": "#bbaa00" } }, { "keys": "f3", "name": "unelevated ELEVATED", "icon": "🙃", "command": { "action": "newTab", "elevate": false, "profile": "elevated cmd" } }, ////////////////////////////// ], "profiles": { "defaults": { "elevate": true, }, "list": [ { "hidden":false, "name" : "cmd", "commandline" : "cmd.exe", "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}", "startingDirectory" : "%USERPROFILE%", "opacity" : 20 }, { "name" : "the COOLER cmd", "commandline" : "c:\\windows\\system32\\cmd.exe", "startingDirectory" : "%USERPROFILE%", }, { "name" : "the sneaky cmd", "commandline" : "c:\\windows\\system32\\cmd.exe /k echo sneaky sneaks", "startingDirectory" : "%USERPROFILE%", }, { "name": "elevated cmd", "commandline": "cmd.exe /k echo This profile is always elevated", "startingDirectory" : "well this is garbage", "elevate": true, "background": "#9C1C0C", "tabColor": "#9C1C0C", "colorScheme": "Desert" }, { "name": "unelevated cmd", "commandline": "cmd.exe /k echo This profile is just as elevated as you started with", "elevate": false, "background": "#1C0C9C", "tabColor": "#1C0C9C", "colorScheme": "DotGov", "useAcrylic": true }, ] ``` </details> Also try: * `wtd nt -p "elevated cmd" ; sp -p "elevated cmd"` * `wtd nt -p "elevated cmd" ; nt -p "elevated cmd"` This was merged manually via ``` git diff dev/migrie/f/non-terminal-content-elevation-warning dev/migrie/f/632-on-warning-dialog > ..\632.patch git apply ..\632.patch --ignore-whitespace --reject ```
4 tasks
ghost
pushed a commit
that referenced
this pull request
Jan 27, 2022
There are a couple places where we now bail immediately on startup, if we think the window is going to get created without any tabs. We do that to prevent a blank window from flashing on the screen when launching auto-elevate profiles. Unfortunately, those broke defterm in a particularly hard to debug way. In the defterm invocation, there actually aren't any tabs when the app completes initialization. We use the initialization to actually accept the defterm handoff. So what would happen is that the window would immediately close itself gracefully, never accepting the handoff. In my defense, #8514, the original auto-elevated PR, predates defterm merging (906edf7) by a few months, so I totally forgot to test this when rolling it into the subsequent iterations of that PR. * Related to: * #7489 * #12137 * #12205 * [x] Closes #12267 * [x] I work here * [ ] No tests on this code unfortunately * [x] Tested manually Includes a semi-related code fix to #10922 to make that quieter. That is perpetually noisy, and when trying to debug defterm, you've only got about 30s to do that before it bails, so the `sxe eh` breaks in there are quite annoying.
This file contains hidden or 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
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.
Summary of the Pull Request
This PR is a huge chunk of the implementation of my spec over in #8455. I know it's bad juju to send the implementation before the spec is approved, but here we are.
This PR adds two features:
elevate: boolproperty to profileselevateis set totrue, then instead of opening a new tab, we'll open an elevated Terminal window with the profile.elevate:false.elevate:falsebasically just means "do nothing special with me".elevate: bool?property toNewTerminalArgs(newTab,splitPane)elevate:null(the default) does not change the profile's elevation status. The action will use whatever is set by the profile.elevate:truewill attempt to auto-elevate the profileelevate:falsewill do nothing special.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
After playing with de-elevation a bit, it turns out it behaves weirdly with packaged applications. I can try and ask
explorer.exeto launch the process on our behalf. However, if the thing we're launching is an execution alias (wt.exe), and we're elevated, then the child process will still launch elevated.Validation Steps Performed
TODO
TerminalSettingsand use that)defaultProfileis set toelevate:true? Probably the same thing as the above. Might need a provision to have the first launch check if the profile we're about to create is elevated, and just bail after launching it.ShellExecuteExso I can indicate the caller might go away before the child is launched.