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

Do not run elevated by default #884

Merged
merged 6 commits into from
Dec 16, 2019
Merged

Do not run elevated by default #884

merged 6 commits into from
Dec 16, 2019

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Dec 6, 2019

Summary of the Pull Request

Make the runner not run as elevated by default. Add a setting for "run PowerToys as elevated" and buttons to restart the process with different elevation level.

PR Checklist

Validation Steps Performed

Manual

@enricogior
Copy link
Contributor

It looks great.
We may need to change a little bit the text in the settings to clarify the difference between the option to start PT elevated and the button to restart PT elevated.

@enricogior
Copy link
Contributor

Regarding the ShotcutGuide running non-elevated: when the current active window is elevated, the guide will not show up. Is that caused by not being able to become the top most window or by failing to generate the preview? In the second case can we just not show the preview?

@bzoz
Copy link
Contributor Author

bzoz commented Dec 9, 2019

I think it is because keyboard hook not working when the active window is elevated, but I should double check this.

yuyoyuppe
yuyoyuppe suggested changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@yuyoyuppe yuyoyuppe left a comment

Choose a reason for hiding this comment

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

LGTM with some nits/clarifications

src/common/common.cpp Outdated Show resolved Hide resolved
src/runner/general_settings.cpp Outdated Show resolved Hide resolved
src/runner/main.cpp Outdated Show resolved Hide resolved
src/runner/restart_elevated.cpp Outdated Show resolved Hide resolved
src/runner/general_settings.cpp Outdated Show resolved Hide resolved
src/runner/settings_window.cpp Outdated Show resolved Hide resolved
@enricogior
Copy link
Contributor

When both Start at login and Run PowerToys with elevated privileges are on, at login you get the UAC dialog. We need to make sure there is no way to avoid this.

@yuyoyuppe yuyoyuppe self-requested a review December 9, 2019 12:34
@enricogior
Copy link
Contributor

Can we reduce the space between the two controls?

image

enricogior
enricogior reviewed Dec 9, 2019
View reviewed changes
src/runner/main.cpp Outdated
is_process_elevated() ?
L"Could not restart PowerToys as a non-elevated process!" :
L"Could not restart PowerToys as an elevated process!",
L"Error", MB_OK | MB_ICONERROR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put these strings in the resource file.

@enricogior
Copy link
Contributor

enricogior commented Dec 9, 2019 •
edited
Loading

Run PowerToys with elevated privileges -> By default, run PowerToys with elevated privileges

Not sure if it sounds better PowerToys are... or PowerToys is... in the first case it refers to the modules, in the second case it refers to the application. I'm more for is.
Instead of changing the button text that looks fine, I suggest to add the explanation in the description:

PowerToys is currently running without elevated privileges. You can restart it to run elevated only for this session, without changing the default setting.

Feel free to suggest different wording.

@crutkas
Copy link
Member

crutkas commented Dec 10, 2019

lets hold here.

It is unclear why someone needs to run with the elevated permissions. That is what must clearly be articulated to the end user.

Is it to be on top of everything for Shortcut Guide, that requires admin?

@enricogior
Copy link
Contributor

@crutkas
any module that needs to interact with elevated windows needs to run elevated.
For example FZ can't resize any elevated window if it's not running elevated.

@crutkas
Copy link
Member

crutkas commented Dec 10, 2019

I think we need to be crisp and clear and document which modules need what. Can we detect if a window is at elevated privilege then elevate us? Or warn the users we detected an app and require to be launched elevated?

@enricogior
Copy link
Contributor

enricogior commented Dec 10, 2019 •
edited
Loading

@crutkas
any PowerToy that interacts with other applications may need elevated privileges, but it's not something we can enable on a per-module base, it has to be the runner to be elevated and make all modules elevated.
Currently the ShortcutGuide and FancyZones, but also AlwaysOnTop, MaximizeToNewDesktop would require it. Only PowerRename doesn't need it since it's not running the PowerToys process.

I'm not sure we can reliably detect if a window is running elevated without running elevated ourselves.
I suggest that for now we improve the documentation, to let users know that PowerToys needs to run elevated to interact with elevated windows and see the feedback.

EDIT: the runner doesn't receive the key pressed events and other windows related events when the active window is elevated and the runner is not elevated.

@enricogior enricogior mentioned this pull request Dec 11, 2019
Merged
1 task
@crutkas
Copy link
Member

crutkas commented Dec 13, 2019

why does shortcut guide need it?

@crutkas crutkas mentioned this pull request Dec 13, 2019
Closed
@crutkas
Copy link
Member

crutkas commented Dec 13, 2019

I'm ok checking this in now. I added a .15 item to review language and a .16 item to document the items @enricogior described here

@enricogior
Copy link
Contributor

why does shortcut guide need it?

Because if the active window is elevated, the keyboards events can't be received unless PowerToys also runs elevated.

bzoz added 6 commits December 16, 2019 18:09
@bzoz
Do not run elevated by default
44dbf8e 
Make the runner not run as elevated by default. Add a setting for
"run PowerToys as elevated" and buttons to restart the process
with different elevation level.
@bzoz
fixup: nits and comments
c3c27f0
@bzoz
fixup: VS warnings
2554360
@bzoz
fixup: make the gap smaller
33bd5b9
@bzoz
fixup: move runner texts to resource
328d3ec
@bzoz
fixup: change settings texts
88b46ca
@bzoz bzoz merged commit 619ed23 into microsoft:master Dec 16, 2019
@bzoz bzoz mentioned this pull request Dec 16, 2019
Closed
yuyoyuppe pushed a commit that referenced this pull request Jan 23, 2020
@bzoz @yuyoyuppe
Do not run elevated by default (#884)
7af115a 
Make the runner not run as elevated by default. Add a setting for
"run PowerToys as elevated" and buttons to restart the process
with the different elevation levels.
yuyoyuppe added a commit that referenced this pull request Jan 23, 2020
@yuyoyuppe
Revert "Do not run elevated by default (#884)"
a9e22f3 
This reverts commit 7af115a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuyoyuppe yuyoyuppe yuyoyuppe approved these changes

@enricogior enricogior enricogior approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bzoz @enricogior @crutkas @yuyoyuppe