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.

Sign up for GitHub

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

Jump to bottom

Prompt before launching commandlines when elevated ; cache answers in elevated state.json #11096

Closed
35 of 40 tasks
zadjii-msft opened this issue Aug 31, 2021 · 1 comment
Closed
35 of 40 tasks

Prompt before launching commandlines when elevated ; cache answers in elevated state.json #11096

zadjii-msft opened this issue Aug 31, 2021 · 1 comment
Assignees
@zadjii-msft
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason.

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 31, 2021 •
edited
Loading

This is part of https://github.com/microsoft/terminal/projects/5, and a pre-req for #632. I'm promoting this to a full issue since I've got a lot of edge-case-y work that needs doing and tracking somewhere.

  • Hot reload elevated-state.json
  • Find the right place to store the file
  • Figure out the right way to do the permissions on this file.
    • SYSTEM: GENERIC_ALL, Everyone: GENERIC_READ - Didn't work. Elevated Terminal couldn't write the file.
    • Admins: GENERIC_ALL, Everyone: GENERIC_READ - Didn't work. Sublime could just delete the file and write something else in it's place. Un-elevated Terminal could even write the file!
    • DOMAIN_USER_RID_ADMIN "Admin", not "Admins"?
    • Manual experimentation says "MIGRIE-HOME\Administrators": Full Control + "Everyone": Read seems to work as I want. Now how do I get that?
    • Something else?
    • IT WAS SOMETHING ELSE. WriteFileAtomic was the problem - the rename would discard the elevated only version of the file, then write a new one in it's place. Presumably, this doesn't really matter. We should only be calling into that function from an elevated window, but it's unfortunate that it doesn't prevent it from working when unelevated.
  • Maybe decide to still do WriteUTF8FileAtomic. We don't need the link handling, so I'm not worried about that...
  • Move the isElevated() logic to a common place to make sure we have one implementation
  • Make a call to the common isElevated() in WriteUTF8File*(), so that we can bail early if we're unelevated.
  • Check that the file is only writable by admins when opening. If it isn't, blow it away and recreate it.
  • Use unique_sid and in general, make sure to clean up the things allocated for us.
  • Only pop the dialog when running elevated
  • Don't pop the dialog if we're elevated (but UAC is disabled) (this is probably just Unable to drag/drop a file to terminal with UAC turned off #7754)
  • Don't pop the dialog for commandlines that are literally just a path to a file in C:\windows\system32.
    • The file must be a full path
    • The file must be in C:\windows\system32
  • UHG also resolve environment variables, so %SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe will work.
  • UHG WSL distros too
  • Pop the dialog for panes too.
  • Actually format the dialog message with the commandline
  • Resolve what happens when multiple controls should be added at once (See Ideas A and B below)
    • What happens when you reject the creation of a pane? Probably should just close
    • What happens during startup for this dialog? Do we just immediately go to the Complete state, then process the actions one at a time?
    • What happens when running a wt action in an elevated window?
    • Rejecting the creation of the first&only (or all) tabs should just close the window.
    • What happens if you wt nt ; sp, then reject the first tab? There are no tabs now to split!
    • Approving a commandline in one pane should automatically update the other ones.
  • Don't add the same commandline multiple times
  • There's some weird focus issues - Replacing the content of the pane doesn't update the tab title (and presumably other properties as well)
  • What happens when the user isn't an admin, but runs the terminal as another user that's an admin (that also has the Terminal installed)?
  • Make sure to add a "why am I seeing this" link
  • Add the TermControl's background to the admin warning placeholder's background.
  • After 64d02f2, fix the hot-reloading for elevated windows

Engineering deets

what happens when multiple controls should be added at once

Idea A: for scenarios where we're going to perform multiple actions, check all of them for commandlines. If we find one action that has an unapproved commandline, then ask the user if they want to run all of the actions. They either approve them all, or none of them.

Idea B: Use a fake content dialog as a placeholder control, until the connection is approved.

  • B.1: We use non-terminal content in the Pane. We use a fake ContentDialog as a placeholder for the TermControl, then swap it out with the TermControl if the control is allowed.
    • This is in the branch from c66a566 onwards
    • How do we handle closing? I think in my initial implementation, there was a IPaneContent interface that exposed all the things a TermControl might do.
    • Like, what happens when the dialog is rejected. It raises a Closed()? How does the pane listen for that...
    • I suppose the AdminWarningPlaceholder will be holding the Pane, the
    • NO it won't hold the pane. That's A: bad circular refs; and B: the Panes will toss the controls around to other panes. So that's bad.
  • B.2: We tell the TermControl to display the dialog itself. It doesn't Start() the connection immediately, instead it starts with it's fake contentdialog.
    • How does this work with oop controls? It wouldn't. The ContentProcess needs to be started before the TermControl. So even if we told the TermControl "you need to ask permission", we'd have already started the content process. It would be harder for the Control to then tell the app "yes I have permission for the thing you asked", then pass it the ContentProcess.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 31, 2021
@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 31, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 31, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.12 milestone Aug 31, 2021
@zadjii-msft zadjii-msft self-assigned this Aug 31, 2021
This was referenced Sep 14, 2021
Merged
Merged
ghost pushed a commit that referenced this issue Sep 16, 2021
@zadjii-msft
Allow reordering tabs when UAC is disabled (#11221)
4793541 
When we're elevated, we disable drag/dropping tabs when elevated, because of a platform limitation that causes the app to _crash_ (see #4874). However, if the user has UAC disabled, this actually works alright. So I'm adding it back in that case.

I'm not positive if this is the best way to check if UAC is disabled, but normally, you'll get a [`TokenElevationTypeFull`] when elevated, not `TokenElevationTypeDefault`. If the app is elevated, but there's not a split token, that kinda implies there's no user account separation. If I'm wrong, it's just code, let's replace this with something that does work.

## Validation Steps Performed

Booted up a Win10 VM, set `enableLUA` to `0`, rebooted, and checked if this exploded. It didn't.

References #4874 
References #3581
Work done in pursuit of #11096
Closes #7754

[`TokenElevationTypeFull`]: https://docs.microsoft.com/en-us/windows/win32/api/winnt/ne-winnt-token_elevation_type
@zadjii-msft zadjii-msft mentioned this issue Sep 22, 2021
Closed
4 tasks
@ghost ghost added the In-PR This issue has a related PR label Sep 22, 2021
@zadjii-msft zadjii-msft mentioned this issue Sep 22, 2021
Closed
4 tasks
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 15, 2021
lhecker pushed a commit that referenced this issue Nov 13, 2021
@zadjii-msft
Add a file for storing elevated-only state (#11222)
c79334f 
## 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 added the Needs-Discussion Something that requires a team discussion before we can proceed label Jan 3, 2022
@zadjii-msft
Copy link
Member Author

We're cutting this for the time being. We'll wait for a more official plan before we ship something like this.

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jan 10, 2022
@zadjii-msft zadjii-msft removed this from the Terminal v1.13 milestone Jan 10, 2022
@zadjii-msft zadjii-msft added Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason. and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Jan 10, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 10, 2022
@ghost ghost removed the In-PR This issue has a related PR label Jan 11, 2022
@zadjii-msft zadjii-msft mentioned this issue Jun 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zadjii-msft zadjii-msft

Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Warn before the user runs a new commandline elevated microsoft/terminal
2 participants
@DHowett @zadjii-msft