-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Enable auto-elevation for a profile, redux #11310
Conversation
…to dev/migrie/f/632-on-warning-dialog
This comment has been minimized.
This comment has been minimized.
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 😉 |
There was a problem hiding this 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.
…t-elevation-warning' into dev/migrie/f/632-on-warning-dialog
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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!
{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 |
There was a problem hiding this comment.
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
…to dev/migrie/f/632-on-warning-dialog
This comment has been minimized.
This comment has been minimized.
…to dev/migrie/f/632-on-warning-dialog
…to dev/migrie/f/632-on-warning-dialog
This comment has been minimized.
This comment has been minimized.
…to dev/migrie/f/632-on-warning-dialog
…to dev/migrie/f/632-on-warning-dialog
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...)
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. |
…to dev/migrie/f/632-on-warning-dialog
…to dev/migrie/f/632-on-warning-dialog
…to dev/migrie/f/632-on-warning-dialog
## 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
As discussed, we're gonna rebase this onto main, without #11308. |
## 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 ```
targets #11308
Summary of the Pull Request
This is the resurrection of #8514.
This PR adds two features:
elevate: bool
property to profileselevate
is set totrue
, then instead of opening a new tab, we'll open an elevated Terminal window with the profile.elevate:false
.elevate:false
basically 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:true
will attempt to auto-elevate the profileelevate:false
will do nothing special.References
state.json
#11096 as the vegetables to this PR's candy, for security's sake.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. ButShellExecute
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 callingShellExecute
, 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 callsShellExecute
, so that it can live for the duration of the UAC prompt.Validation Steps Performed
Some settings to use for testing
Also try:
wtd nt -p "elevated cmd" ; sp -p "elevated cmd"
wtd nt -p "elevated cmd" ; nt -p "elevated cmd"