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

Remove scope filter from being applied to portables #2383

Merged
merged 11 commits into from
Jul 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions doc/Settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,24 +59,24 @@ The `disableInstallNotes` behavior affects whether installation notes are shown
```

### Portable Package User Root
The `PortablePackageUserRoot` setting affects the default root directory where packages are installed to under `User` scope. This setting only applies to packages with the `portable` installer type. Defaults to `%LOCALAPPDATA%/Microsoft/WinGet/Packages/` if value is not set or is invalid.
The `portablePackageUserRoot` setting affects the default root directory where packages are installed to under `User` scope. This setting only applies to packages with the `portable` installer type. Defaults to `%LOCALAPPDATA%/Microsoft/WinGet/Packages/` if value is not set or is invalid.

> Note: This setting value must be an absolute path.

```json
"installBehavior": {
"PortablePackageUserRoot": "C:/Users/FooBar/Packages"
"portablePackageUserRoot": "C:/Users/FooBar/Packages"
},
```

### Portable Package Machine Root
The `PortablePackageMachineRoot` setting affects the default root directory where packages are installed to under `Machine` scope. This setting only applies to packages with the `portable` installer type. Defaults to `%PROGRAMFILES%/WinGet/Packages/` if value is not set or is invalid.
The `portablePackageMachineRoot` setting affects the default root directory where packages are installed to under `Machine` scope. This setting only applies to packages with the `portable` installer type. Defaults to `%PROGRAMFILES%/WinGet/Packages/` if value is not set or is invalid.

> Note: This setting value must be an absolute path.

```json
"installBehavior": {
"PortablePackageMachineRoot": "C:/Program Files/Packages/Portable"
"portablePackageMachineRoot": "C:/Program Files/Packages/Portable"
},
```

Expand Down
4 changes: 2 additions & 2 deletions schemas/JSON/settings/settings.schema.0.2.json
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,12 @@
"type": "boolean",
"default": false
},
"PortablePackageUserRoot": {
"portablePackageUserRoot": {
"description": "The default root directory where packages are installed to under User scope. Applies to the portable installer type.",
"type": "string",
"default": "%LOCALAPPDATA%/Microsoft/WinGet/Packages/"
},
"PortablePackageMachineRoot": {
"portablePackageMachineRoot": {
"description": "The default root directory where packages are installed to under Machine scope. Applies to the portable installer type.",
"type": "string",
"default": "%PROGRAMFILES%/WinGet/Packages/"
Expand Down
4 changes: 2 additions & 2 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ namespace AppInstaller::CLI::Workflow
InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override
{
// We have to assume the unknown scope will match our required scope, or the entire catalog would stop working for upgrade.
if (installer.Scope == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement)
if (installer.Scope == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement || DoesInstallerIgnoreScopeFromManifest(installer))
{
return InapplicabilityFlags::None;
}
Expand Down Expand Up @@ -342,7 +342,7 @@ namespace AppInstaller::CLI::Workflow

InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override
{
if (m_requirement == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement)
if (m_requirement == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement || DoesInstallerIgnoreScopeFromManifest(installer))
{
return InapplicabilityFlags::None;
}
Expand Down
30 changes: 28 additions & 2 deletions src/AppInstallerCLICore/Workflows/PortableFlow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -556,12 +556,38 @@ namespace AppInstaller::CLI::Workflow
}
}
}

void EnsureRunningAsAdminForMachineScopeInstall(Execution::Context& context)
{
// Admin is required for machine scope install or else creating a symlink in the %PROGRAMFILES% link location will fail.
Manifest::ScopeEnum scope = ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope));
if (scope == Manifest::ScopeEnum::Machine)
{
context << Workflow::EnsureRunningAsAdmin;
}
}
}

void PortableInstallImpl(Execution::Context& context)
{
Manifest::ScopeEnum scope = Manifest::ScopeEnum::Unknown;
bool isUpdate = WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerExecutionUseUpdate);
if (isUpdate)
{
IPackageVersion::Metadata installationMetadata = context.Get<Execution::Data::InstalledPackageVersion>()->GetMetadata();
auto installerScopeItr = installationMetadata.find(Repository::PackageVersionMetadata::InstalledScope);
if (installerScopeItr != installationMetadata.end())
{
scope = Manifest::ConvertToScopeEnum(installerScopeItr->second);
}
}
else
{
scope = ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope));
}

PortableARPEntry uninstallEntry = PortableARPEntry(
ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)),
scope,
context.Get<Execution::Data::Installer>()->Arch,
GetPortableProductCode(context));

Expand Down Expand Up @@ -589,7 +615,6 @@ namespace AppInstaller::CLI::Workflow

// Perform cleanup only if the install fails and is not an update.
const auto& installReturnCode = context.Get<Execution::Data::OperationReturnCode>();
bool isUpdate = WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerExecutionUseUpdate);

if (installReturnCode != 0 && installReturnCode != APPINSTALLER_CLI_ERROR_PORTABLE_PACKAGE_ALREADY_EXISTS && !isUpdate)
{
Expand Down Expand Up @@ -634,6 +659,7 @@ namespace AppInstaller::CLI::Workflow
{
context <<
EnsureSymlinkCreationPrivilege <<
EnsureRunningAsAdminForMachineScopeInstall <<
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like theres a duplicated call here, but only in certain scenarios. When dev mode is disabled, it checks for admin to ensure synlink creation, then checks for admin again if running a machine scope install.

I’m sure that this will work, I’m just wondering if maybe theres a better way to handle checking the elevation level?

Copy link
Member

Choose a reason for hiding this comment

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

It is less efficient, but EnsureSymlinkCreationPrivilege is planned to be temporary, so it is easier to keep it separated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EnsureSymlinkCreationPrivilege is meant to be a temporary check to handle the special case where a user may have developer mode off. I expect that this check will be removed once we fully address this issue in 1.4, so I decided to keep these as separate checks for better clarity and cleanup in the near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess I needed to refresh the page :)

EnsureValidArgsForPortableInstall <<
EnsureVolumeSupportsReparsePoints;
}
Expand Down
10 changes: 10 additions & 0 deletions src/AppInstallerCLIE2ETests/BaseCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ public void ConfigureFeature(string featureName, bool status)
File.WriteAllText(Path.Combine(localAppDataPath, TestCommon.SettingsJsonFilePath), settingsJson.ToString());
}

public void ConfigureInstallBehavior(string settingName, string value)
{
string localAppDataPath = Environment.GetEnvironmentVariable(Constants.LocalAppData);
JObject settingsJson = JObject.Parse(File.ReadAllText(Path.Combine(localAppDataPath, TestCommon.SettingsJsonFilePath)));
JObject installBehavior = (JObject)settingsJson["installBehavior"];
installBehavior[settingName] = value;

File.WriteAllText(Path.Combine(localAppDataPath, TestCommon.SettingsJsonFilePath), settingsJson.ToString());
}

public void InitializeAllFeatures(bool status)
{
ConfigureFeature("experimentalArg", status);
Expand Down
7 changes: 7 additions & 0 deletions src/AppInstallerCLIE2ETests/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ public class Constants
// Registry keys
public const string WinGetPackageIdentifier = "WinGetPackageIdentifier";
public const string WinGetSourceIdentifier = "WinGetSourceIdentifier";
public const string UninstallSubKey = @"Software\Microsoft\Windows\CurrentVersion\Uninstall";
public const string PathSubKey_User = @"Environment";
public const string PathSubKey_Machine = @"SYSTEM\CurrentControlSet\Control\Session Manager\Environment";

// User settings
public const string PortablePackageUserRoot = "portablePackageUserRoot";
public const string PortablePackageMachineRoot = "portablePackageMachineRoot";

public class ErrorCode
{
Expand Down
40 changes: 38 additions & 2 deletions src/AppInstallerCLIE2ETests/InstallCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public void InstallPortableFailsWithCleanup()
commandAlias = fileName = "AppInstallerTestExeInstaller.exe";

// Create a directory with the same name as the symlink in order to cause install to fail.
string symlinkDirectory = TestCommon.GetPortableSymlinkDirectory();
string symlinkDirectory = TestCommon.GetPortableSymlinkDirectory(TestCommon.Scope.User);
string conflictDirectory = Path.Combine(symlinkDirectory, commandAlias);
Directory.CreateDirectory(conflictDirectory);

Expand All @@ -285,7 +285,7 @@ public void ReinstallPortable()
var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestPortableExe");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);

string symlinkDirectory = TestCommon.GetPortableSymlinkDirectory();
string symlinkDirectory = TestCommon.GetPortableSymlinkDirectory(TestCommon.Scope.User);
string symlinkPath = Path.Combine(symlinkDirectory, commandAlias);

// Clean first install should not display file overwrite message.
Expand All @@ -301,6 +301,42 @@ public void ReinstallPortable()
TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true);
}

[Test]
public void InstallPortable_UserScope()
{
string installDir = TestCommon.GetRandomTestDir();
ConfigureInstallBehavior(Constants.PortablePackageUserRoot, installDir);

string packageId, commandAlias, fileName, packageDirName, productCode;
packageId = "AppInstallerTest.TestPortableExe";
packageDirName = productCode = packageId + "_" + Constants.TestSourceIdentifier;
commandAlias = fileName = "AppInstallerTestExeInstaller.exe";

var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestPortableExe --scope user");
ConfigureInstallBehavior(Constants.PortablePackageUserRoot, string.Empty);
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Successfully installed"));
TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true);
}

[Test]
public void InstallPortable_MachineScope()
{
string installDir = TestCommon.GetRandomTestDir();
ConfigureInstallBehavior(Constants.PortablePackageMachineRoot, installDir);

string packageId, commandAlias, fileName, packageDirName, productCode;
packageId = "AppInstallerTest.TestPortableExe";
packageDirName = productCode = packageId + "_" + Constants.TestSourceIdentifier;
commandAlias = fileName = "AppInstallerTestExeInstaller.exe";

var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestPortableExe --scope machine");
ConfigureInstallBehavior(Constants.PortablePackageMachineRoot, string.Empty);
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Successfully installed"));
TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true, TestCommon.Scope.Machine);
}

[Test]
public void InstallZipWithExe()
{
Expand Down
5 changes: 5 additions & 0 deletions src/AppInstallerCLIE2ETests/SetUpFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ public void InitializeWingetSettings()
{
enableSelfInitiatedMinidump = true
},
installBehavior = new
{
portablePackageUserRoot = "",
portablePackageMachineRoot = "",
}
};

// Run winget one time to initialize settings directory
Expand Down
33 changes: 24 additions & 9 deletions src/AppInstallerCLIE2ETests/TestCommon.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ namespace AppInstallerCLIE2ETests
using System;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Threading;

public class TestCommon
Expand Down Expand Up @@ -46,6 +45,12 @@ public static string SettingsJsonFilePath {
}
}

public enum Scope
{
User,
Machine
}

public struct RunCommandResult
{
public int ExitCode;
Expand Down Expand Up @@ -280,9 +285,16 @@ public static bool RemoveMsix(string name)
return RunCommand("powershell", $"Get-AppxPackage \"{name}\" | Remove-AppxPackage");
}

public static string GetPortableSymlinkDirectory()
public static string GetPortableSymlinkDirectory(Scope scope)
{
return Path.Combine(Environment.GetEnvironmentVariable("LocalAppData"), "Microsoft", "WinGet", "Links");
if (scope == Scope.User)
{
return Path.Combine(Environment.GetEnvironmentVariable("LocalAppData"), "Microsoft", "WinGet", "Links");
}
else
{
return Path.Combine(Environment.GetEnvironmentVariable("ProgramFiles"), "WinGet", "Links");
}
}

public static string GetPortablePackagesDirectory()
Expand All @@ -295,25 +307,28 @@ public static void VerifyPortablePackage(
string commandAlias,
string filename,
string productCode,
bool shouldExist)
bool shouldExist,
Scope scope = Scope.User)
{
string exePath = Path.Combine(installDir, filename);
bool exeExists = File.Exists(exePath);

string symlinkDirectory = GetPortableSymlinkDirectory();
string symlinkDirectory = GetPortableSymlinkDirectory(scope);
string symlinkPath = Path.Combine(symlinkDirectory, commandAlias);
bool symlinkExists = File.Exists(symlinkPath);

bool portableEntryExists;
string subKey = @$"Software\Microsoft\Windows\CurrentVersion\Uninstall";
using (RegistryKey uninstallRegistryKey = Registry.CurrentUser.OpenSubKey(subKey, true))
RegistryKey baseKey = (scope == Scope.User) ? Registry.CurrentUser : Registry.LocalMachine;
string uninstallSubKey = Constants.UninstallSubKey;
using (RegistryKey uninstallRegistryKey = baseKey.OpenSubKey(uninstallSubKey, true))
{
RegistryKey portableEntry = uninstallRegistryKey.OpenSubKey(productCode, true);
portableEntryExists = portableEntry != null;
}

bool isAddedToPath;
using (RegistryKey environmentRegistryKey = Registry.CurrentUser.OpenSubKey(@"Environment", true))
string pathSubKey = (scope == Scope.User) ? Constants.PathSubKey_User : Constants.PathSubKey_Machine;
using (RegistryKey environmentRegistryKey = baseKey.OpenSubKey(pathSubKey, true))
{
string pathName = "Path";
var currentPathValue = (string)environmentRegistryKey.GetValue(pathName);
Expand All @@ -328,7 +343,7 @@ public static void VerifyPortablePackage(

Assert.AreEqual(shouldExist, exeExists, $"Expected portable exe path: {exePath}");
Assert.AreEqual(shouldExist, symlinkExists, $"Expected portable symlink path: {symlinkPath}");
Assert.AreEqual(shouldExist, portableEntryExists, $"Expected {productCode} subkey in path: {subKey}");
Assert.AreEqual(shouldExist, portableEntryExists, $"Expected {productCode} subkey in path: {uninstallSubKey}");
Assert.AreEqual(shouldExist, isAddedToPath, $"Expected path variable: {symlinkDirectory}");
}

Expand Down
28 changes: 25 additions & 3 deletions src/AppInstallerCLIE2ETests/UpgradeCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public class UpgradeCommand : BaseCommand
[Test]
public void UpgradePortable()
{
string installDir = Path.Combine(System.Environment.GetEnvironmentVariable("LocalAppData"), "Microsoft", "WinGet", "Packages");
string installDir = TestCommon.GetPortablePackagesDirectory();
string packageId, commandAlias, fileName, packageDirName, productCode;
packageId = "AppInstallerTest.TestPortableExe";
packageDirName = productCode = packageId + "_" + Constants.TestSourceIdentifier;
Expand Down Expand Up @@ -53,7 +53,7 @@ public void UpgradePortableARPMismatch()
[Test]
public void UpgradePortableForcedOverride()
{
string installDir = Path.Combine(System.Environment.GetEnvironmentVariable("LocalAppData"), "Microsoft", "WinGet", "Packages");
string installDir = TestCommon.GetPortablePackagesDirectory();
string packageId, commandAlias, fileName, packageDirName, productCode;
packageId = "AppInstallerTest.TestPortableExe";
packageDirName = productCode = packageId + "_" + Constants.TestSourceIdentifier;
Expand All @@ -76,7 +76,7 @@ public void UpgradePortableForcedOverride()
[Test]
public void UpgradePortableUninstallPrevious()
{
string installDir = Path.Combine(System.Environment.GetEnvironmentVariable("LocalAppData"), "Microsoft", "WinGet", "Packages");
string installDir = TestCommon.GetPortablePackagesDirectory();
string packageId, commandAlias, fileName, packageDirName, productCode;
packageId = "AppInstallerTest.TestPortableExe";
packageDirName = productCode = packageId + "_" + Constants.TestSourceIdentifier;
Expand All @@ -91,5 +91,27 @@ public void UpgradePortableUninstallPrevious()
Assert.True(result2.StdOut.Contains("Successfully installed"));
TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true);
}

[Test]
public void UpgradePortableMachineScope()
{
string installDir = TestCommon.GetRandomTestDir();
ConfigureInstallBehavior(Constants.PortablePackageMachineRoot, installDir);

string packageId, commandAlias, fileName, packageDirName, productCode;
packageId = "AppInstallerTest.TestPortableExe";
packageDirName = productCode = packageId + "_" + Constants.TestSourceIdentifier;
commandAlias = fileName = "AppInstallerTestExeInstaller.exe";

var result = TestCommon.RunAICLICommand("install", $"{packageId} -v 1.0.0.0 --scope machine");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Successfully installed"));

var result2 = TestCommon.RunAICLICommand("upgrade", $"{packageId} -v 2.0.0.0");
ConfigureInstallBehavior(Constants.PortablePackageMachineRoot, string.Empty);
Assert.AreEqual(Constants.ErrorCode.S_OK, result2.ExitCode);
Assert.True(result2.StdOut.Contains("Successfully installed"));
TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true, TestCommon.Scope.Machine);
}
}
}
Loading