Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable auto-elevation for a profile, redux #11310

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Sep 22, 2021

targets #11308

Summary of the Pull Request

This is the resurrection of #8514.

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.

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

    "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
            },
        ]

Also try:

  • wtd nt -p "elevated cmd" ; sp -p "elevated cmd"
  • wtd nt -p "elevated cmd" ; nt -p "elevated cmd"

@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 Sep 22, 2021
@github-actions

This comment has been minimized.

@miniksa
Copy link
Member

miniksa commented Sep 23, 2021

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.

That is odd, but seems like only a subset of the world, not the majority issue. So perhaps we log a follow up for that?

@zadjii-msft
Copy link
Member Author

That is odd, but seems like only a subset of the world, not the majority issue. So perhaps we log a follow up for that?

I think there's already a deliverable on the msix team for this one 😉

src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/ActionArgs.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 23, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have anything to add other than "update Hash() too". I would like some resolution/follow-up on (1) the sleep issue and (2) the window not being created sometimes issue.

src/cascadia/TerminalSettingsModel/ActionArgs.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 27, 2021
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all little things and I'm sure @miniksa (or somebody else) will come back to review this. Thanks!

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/ElevateShim/elevate-shim.cpp Show resolved Hide resolved
Comment on lines +2810 to +2846
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|Any CPU.ActiveCfg = AuditMode|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|ARM.ActiveCfg = AuditMode|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|DotNet_x64Test.ActiveCfg = AuditMode|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|DotNet_x86Test.ActiveCfg = AuditMode|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|x64.ActiveCfg = AuditMode|x64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|x64.Build.0 = AuditMode|x64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|x86.ActiveCfg = AuditMode|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.AuditMode|x86.Build.0 = AuditMode|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|Any CPU.ActiveCfg = Debug|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|ARM.ActiveCfg = Debug|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|ARM64.ActiveCfg = Debug|ARM64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|ARM64.Build.0 = Debug|ARM64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|DotNet_x64Test.ActiveCfg = Debug|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|DotNet_x86Test.ActiveCfg = Debug|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|x64.ActiveCfg = Debug|x64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|x64.Build.0 = Debug|x64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|x86.ActiveCfg = Debug|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Debug|x86.Build.0 = Debug|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Fuzzing|Any CPU.ActiveCfg = Fuzzing|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Fuzzing|ARM.ActiveCfg = Fuzzing|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Fuzzing|ARM64.ActiveCfg = Fuzzing|ARM64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Fuzzing|DotNet_x64Test.ActiveCfg = Fuzzing|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Fuzzing|DotNet_x86Test.ActiveCfg = Fuzzing|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Fuzzing|x64.ActiveCfg = Fuzzing|x64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Fuzzing|x86.ActiveCfg = Fuzzing|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|Any CPU.ActiveCfg = Release|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|ARM.ActiveCfg = Release|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|ARM64.ActiveCfg = Release|ARM64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|ARM64.Build.0 = Release|ARM64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|DotNet_x64Test.ActiveCfg = Release|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|DotNet_x86Test.ActiveCfg = Release|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|x64.ActiveCfg = Release|x64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|x64.Build.0 = Release|x64
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|x86.ActiveCfg = Release|Win32
{416FD703-BAA7-4F6E-9361-64F550EC8FCA}.Release|x86.Build.0 = Release|Win32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double check if we want to remove any of these. I'm not sure what configs we want for elevate-shim

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something in my head thought that elevated-shim.exe could maybe be an Out-of-proc call server for the Terminal... but then I went meh.

If we already had terminalsvc.exe or whatever (like we want in order to capture the launch hotkey when the Terminal isn't already running), the elevate shim call could be OOP'd to there as it would stay alive for a long time.

But whatever. This works and makes sense.

return S_OK;
}
// We can't go in the other direction (elevated->unelevated)
// unfortunately. This seems to be due to Centennial quirks. It works
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we filed this in papercuts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or written BODGY here... cuz that's a representation of cut workarounds...)

@zadjii-msft
Copy link
Member Author

thought that elevated-shim.exe could maybe be an Out-of-proc call server for the Terminal

that's a fun idea but something about the idea of elevated, packaged COM made me think that certainly won't work. I wonder why I'd have that preconception in my head 😝

@miniksa
Copy link
Member

miniksa commented Nov 3, 2021

thought that elevated-shim.exe could maybe be an Out-of-proc call server for the Terminal

that's a fun idea but something about the idea of elevated, packaged COM made me think that certainly won't work. I wonder why I'd have that preconception in my head 😝

We'll just make a custom transport on a loopback socket... or we could just do this.

lhecker pushed a commit that referenced this pull request Nov 13, 2021
## Summary of the Pull Request

This creates an `elevated-state.json` that lives in `%LOCALAPPDATA%` next to `state.json`, that's only writable when elevated. It doesn't _use_ this file for anything, it just puts the framework down for use later.

It's _just like `ApplicationState`_. We'll use it the same way. 

It's readable when unelevated, which is nice, but not writable. If you're dumb and try to write to the file when unelevated, it'll just silently do nothing.

If we try opening the file and find out the permissions are different, we'll _blow the file away entirely_. This is to prevent someone from renaming the original file (which they can do unelevated), then slapping a new file that's writable by them down in it's place. 

## References
* We're going to use this in #11096, but these PRs need to be broken up.

## PR Checklist
* [x] Closes nothing
* [x] I work here
* [x] Tests added/passed
* [ ] Requires documentation to be updated - maybe? not sure we have docs on `state.json` at all yet

## Validation Steps Performed
I've played with this much more in `dev/migrie/f/non-terminal-content-elevation-warning`

###### followed by #11308, #11310
@zadjii-msft zadjii-msft self-assigned this Jan 4, 2022
@zadjii-msft
Copy link
Member Author

As discussed, we're gonna rebase this onto main, without #11308.

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
```
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
3 participants