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

Act on elevation requirements in majority cases #2126

Merged
merged 6 commits into from
May 5, 2022
Merged

Act on elevation requirements in majority cases #2126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7afc57a
Act on elevation requirements in majority cases
JohnMcPMS Apr 26, 2022
58afc1d
Spelling
JohnMcPMS Apr 26, 2022
e1513f3
PR feedback and add elevation warning to direct MSI installs that ele…
JohnMcPMS Apr 27, 2022
1603f99
Align brace
JohnMcPMS Apr 27, 2022
2c1f858
Merge from master
JohnMcPMS Apr 27, 2022
fad5712
Merge from master
JohnMcPMS May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/allow.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ Rpc
rpc
rubengustorage
ruleset
runas
runsettings
runtimes
safecast
Expand Down
2 changes: 2 additions & 0 deletions src/AppInstallerCLICore/Resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(InstallCommandShortDescription);
WINGET_DEFINE_RESOURCE_STRINGID(InstalledPackageNotAvailable);
WINGET_DEFINE_RESOURCE_STRINGID(InstalledPackageVersionNotAvailable);
WINGET_DEFINE_RESOURCE_STRINGID(InstallerElevationExpected);
WINGET_DEFINE_RESOURCE_STRINGID(InstallerBlockedByPolicy);
WINGET_DEFINE_RESOURCE_STRINGID(InstallerFailedSecurityCheck);
WINGET_DEFINE_RESOURCE_STRINGID(InstallerFailedVirusScan);
Expand All @@ -126,6 +127,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashMismatchOverrideRequired);
WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashVerified);
WINGET_DEFINE_RESOURCE_STRINGID(InstallerLogAvailable);
WINGET_DEFINE_RESOURCE_STRINGID(InstallerProhibitsElevation);
WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowInstallSuccess);
WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowRegistrationDeferred);
WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowReturnCodeAlreadyInstalled);
Expand Down
8 changes: 8 additions & 0 deletions src/AppInstallerCLICore/Workflows/InstallFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,14 @@ namespace AppInstaller::CLI::Workflow
}

context << EnsureSupportForInstall;

// 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 •
edited
Loading

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.

{
context.Reporter.Error() << Resource::String::InstallerProhibitsElevation << std::endl;
AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_PROHIBITS_ELEVATION);
}
}

void ShowInstallationDisclaimer(Execution::Context& context)
Expand Down
7 changes: 7 additions & 0 deletions src/AppInstallerCLICore/Workflows/MsiInstallFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,17 @@ namespace AppInstaller::CLI::Workflow
{
context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl;

const auto& installer = context.Get<Execution::Data::Installer>();
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)
{
context.Reporter.Warn() << Resource::String::InstallerElevationExpected << std::endl;
}

auto installResult = context.Reporter.ExecuteWithProgress(
std::bind(InvokeMsiInstallProduct,
installerPath,
Expand Down
29 changes: 27 additions & 2 deletions src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace AppInstaller::CLI::Workflow
namespace
{
// ShellExecutes the given path.
std::optional<DWORD> InvokeShellExecute(const std::filesystem::path& filePath, const std::string& args, IProgressCallback& progress)
std::optional<DWORD> InvokeShellExecuteEx(const std::filesystem::path& filePath, const std::string& args, bool useRunAs, IProgressCallback& progress)
{
AICLI_LOG(CLI, Info, << "Starting: '" << filePath.u8string() << "' with arguments '" << args << '\'');

Expand All @@ -28,6 +28,13 @@ namespace AppInstaller::CLI::Workflow
// Verified setting to SW_SHOW does not hurt silent mode since no UI will be shown.
execInfo.nShow = SW_SHOW;

// This installer must be run elevated, but we are not currently.
// Have ShellExecute elevate the installer since it won't do so itself.
if (useRunAs)
{
execInfo.lpVerb = L"runas";
}

THROW_LAST_ERROR_IF(!ShellExecuteExW(&execInfo) || !execInfo.hProcess);

wil::unique_process_handle process{ execInfo.hProcess };
Expand Down Expand Up @@ -58,6 +65,11 @@ namespace AppInstaller::CLI::Workflow
}
}

std::optional<DWORD> InvokeShellExecute(const std::filesystem::path& filePath, const std::string& args, IProgressCallback& progress)
{
return InvokeShellExecuteEx(filePath, args, false, progress);
}

// Gets the escaped installer args.
std::string GetInstallerArgsTemplate(Execution::Context& context)
{
Expand Down Expand Up @@ -187,12 +199,25 @@ namespace AppInstaller::CLI::Workflow
{
context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl;

const auto& installer = context.Get<Execution::Data::Installer>();
const std::string& installerArgs = context.Get<Execution::Data::InstallerArgs>();

// Inform of elevation requirements
bool isElevated = Runtime::IsRunningAsAdmin();

// The installer will run elevated, either by direct request or through the installer itself doing so.
if ((installer->ElevationRequirement == ElevationRequirementEnum::ElevationRequired ||
installer->ElevationRequirement == ElevationRequirementEnum::ElevatesSelf)
&& !isElevated)
{
context.Reporter.Warn() << Resource::String::InstallerElevationExpected << std::endl;
}

auto installResult = context.Reporter.ExecuteWithProgress(
std::bind(InvokeShellExecute,
std::bind(InvokeShellExecuteEx,
context.Get<Execution::Data::InstallerPath>(),
installerArgs,
installer->ElevationRequirement == ElevationRequirementEnum::ElevationRequired && !isElevated,
std::placeholders::_1));

if (!installResult)
Expand Down
6 changes: 6 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1287,6 +1287,12 @@ 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 as administrator, expect a prompt.</value>
</data>
<data name="InstallerProhibitsElevation" xml:space="preserve">
<value>The installer cannot be run from an administrator context.</value>
</data>
<data name="ModifiedPathRequiresShellRestart" xml:space="preserve">
<value>Path environment variable modified; restart your shell to use the new value.</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/AppInstallerCommonCore/Errors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ namespace AppInstaller
return "A higher version of this application is already installed.";
case APPINSTALLER_CLI_ERROR_INSTALL_BLOCKED_BY_POLICY:
return "Organization policies are preventing installation. Contact your admin.";
case APPINSTALLER_CLI_ERROR_INSTALLER_PROHIBITS_ELEVATION:
return "The installer cannot be run from an administrator context.";
default:
return "Unknown Error Code";
}
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCommonCore/Public/AppInstallerErrors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
#define APPINSTALLER_CLI_ERROR_PORTABLE_REPARSE_POINT_NOT_SUPPORTED ((HRESULT)0x8A150053)
#define APPINSTALLER_CLI_ERROR_PORTABLE_PACKAGE_ALREADY_EXISTS ((HRESULT)0x8A150054)
#define APPINSTALLER_CLI_ERROR_PORTABLE_SYMLINK_PATH_IS_DIRECTORY ((HRESULT)0x8A150055)
#define APPINSTALLER_CLI_ERROR_INSTALLER_PROHIBITS_ELEVATION ((HRESULT)0x8A150056)

// Install errors.
#define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE ((HRESULT)0x8A150101)
Expand Down