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

Act on elevation requirements in majority cases #2126

Merged
merged 6 commits into from
May 5, 2022

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Apr 26, 2022

Fixes #1537, #1538

Change

When ElevationRequirement is set to elevationProhibited is set and winget is run elevated, block the installation with an error.

When elevatesSelf or elevationRequired is set and winget is not run elevated, inform the user that an elevation prompt is expected. Also in the case of elevationRequired, use the runas verb on ShellExecute to run the installer elevated. This can be used on installers that will fail if they are not run elevated.

The message is a warning (yellow text) and is the last line below:

Successfully verified installer hash
Starting package install...
The installer will request to run as administrator, expect a prompt.

Validation

Manually validated each of the 3 elevation requirement options, as automated testing of elevation is not possible on the build server.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner April 26, 2022 23:44
@ghost ghost added the Issue-Feature This is a feature request for the Windows Package Manager client. label Apr 26, 2022
@JohnMcPMS JohnMcPMS linked an issue Apr 26, 2022 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Apr 26, 2022

@check-spelling-bot Report

Unrecognized words, please review:

  • runas
Previously acknowledged words that are now absent activatable amd Archs dsc enr FWW Globals hackathon lww mytool Packagedx parametermap Uninitialize WDAG whatif wsb
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:JohnMcPMS/winget-cli.git repository
on the elevationReqs branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1110352997" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

@@ -1287,4 +1287,10 @@ Please specify one of them using the `--source` option to proceed.</value>
<data name="WaitArgumentDescription" xml:space="preserve">
<value>Prompts the user to press any key before exiting</value>
</data>
<data name="InstallerElevationExpected" xml:space="preserve">
<value>The installer will request to run elevated, expect a prompt.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

elevated

nit: "as administrator" since this is user facing string?


// This installer cannot be run elevated, but we are running elevated.
// Implementation of de-elevation is complex; simply block for now.
if (installer->ElevationRequirement == ElevationRequirementEnum::ElevationProhibited && Runtime::IsRunningAsAdmin())
Copy link
Contributor

@yao-msft yao-msft Apr 27, 2022

Choose a reason for hiding this comment

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

if (installer->ElevationRequirement == ElevationRequirementEnum::ElevationProhibited && Runtime::IsRunningAsAdmin())

I'm sort of mixed. Did you think of moving to installer selection in case there're other applicable installers? Or it's preferred to tell the user to run non-elevated again since this is what we think is the best installer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be best to have it in the installer, especially given that there is a feature request to filter by elevation requirement. However, if there are no applicable installers based solely on the fact that the user is running in an elevated terminal, then it still should say elevation prohibited rather than no applicable installer found

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that installer selection is appropriate, we don't even have user intent without #1898. I don't consider "is winget running as admin or not" user intent here, because I want to be able to run my Terminal as user and elevate installers as needed. #1898 should be implemented in installer selection (as well as search) though, because we would have user intent.

@JohnMcPMS
Copy link
Member Author

Side note: I broke VS Code on my machine due to forcing it to install elevated. Be safe out there...

const std::filesystem::path& installerPath = context.Get<Execution::Data::InstallerPath>();

Msi::MsiParsedArguments parsedArgs = Msi::ParseMSIArguments(context.Get<Execution::Data::InstallerArgs>());

// Inform of elevation requirements
if (!Runtime::IsRunningAsAdmin() && installer->ElevationRequirement == Manifest::ElevationRequirementEnum::ElevatesSelf)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alignment

Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@Trenly
Copy link
Contributor

Trenly commented Apr 28, 2022

Does this also resolve #152 and #271 ?

@JohnMcPMS
Copy link
Member Author

Does this also resolve #152 and #271 ?

@denelon to say if it resolves #152. #271 is more of a general Windows issue; we can't magically bypass the UAC prompt. I don't know how one could even secure a sudo command in Windows if it doesn't trigger the UAC prompt...

@Trenly
Copy link
Contributor

Trenly commented Apr 28, 2022

Does this also resolve #152 and #271 ?

@denelon to say if it resolves #152. #271 is more of a general Windows issue; we can't magically bypass the UAC prompt. I don't know how one could even secure a sudo command in Windows if it doesn't trigger the UAC prompt...

I thought that using runas would suppress the prompt, as suggested in the issue; But, perhaps I read it incorrectly or was mistaken

@Masamune3210
Copy link

Nope, not having a way to bypass UAC is very much an intentional part of the design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature This is a feature request for the Windows Package Manager client.
Projects
None yet
4 participants