Skip to content

Enable auto-elevation for a profile#8514

Closed
zadjii-msft wants to merge 10 commits intomainfrom
dev/migrie/f/632-elevated-profiles
Closed

Enable auto-elevation for a profile#8514
zadjii-msft wants to merge 10 commits intomainfrom
dev/migrie/f/632-elevated-profiles

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Dec 7, 2020

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:

  • 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

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.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.

Validation Steps Performed

  • Ran tests
  • Opened a bunch of terminal tabs at various different elevation levels
  • opened new splits too

TODO

  • Add a dialog to ensure the user knows what thing they're about to elevate. Either that, or we need to manually put the commandline of the profile in the UAC dialog (not too hard - we'd just take the commandline from the final TerminalSettings and use that)
    • This is pending a bigger review in the spec.
  • Opening an elevated profile from the jumplist will create an empty (unelevated) terminal window that can't be closed. We'll need to update the jumplist logic to handle elevated profiles
  • What happens if the defaultProfile is set to elevate: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.
  • If the Terminal exits before the elevated child spawns, then the elevation won't complete. I tried just having the original terminal bail to fix the above TODOs, but that revealed this one. I might need to use ShellExecuteEx so I can indicate the caller might go away before the child is launched.

@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Dec 7, 2020
@zadjii-msft zadjii-msft self-assigned this Dec 7, 2020
@zadjii-msft
Copy link
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.

@zadjii-msft
Copy link
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.

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
```
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Question] Configuring Windows Terminal profile to always launch elevated

1 participant