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

Do not attempt post install ARP correlation if PackageFamilyName is provided and present for the user #3391

Merged
merged 8 commits into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
9 changes: 9 additions & 0 deletions src/AppInstallerCLICore/Workflows/InstallFlow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,15 @@ namespace AppInstaller::CLI::Workflow
return;
}

// If the installer claims to have a PackageFamilyName, and that family name is currently registered for the user,
// let that be the correlated item and skip any attempt at further ARP correlation.
const auto& installer = context.Get<Execution::Data::Installer>();

if (installer && !installer->PackageFamilyName.empty() && Deployment::IsRegistered(installer->PackageFamilyName))
{
return;
}

const auto& manifest = context.Get<Execution::Data::Manifest>();
auto& arpCorrelationData = context.Get<Execution::Data::ARPCorrelationData>();

Expand Down
9 changes: 0 additions & 9 deletions src/AppInstallerCLIE2ETests/AppInstallerCLIE2ETests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,6 @@
</Content>
</ItemGroup>

<ItemGroup>
<None Remove="TestData\Configuration\ConfigServerUnexpectedExit.yml" />
<None Remove="TestData\Configuration\Configure_TestRepo.yml" />
<None Remove="TestData\Configuration\DependentResources_Failure.yml" />
<None Remove="TestData\Configuration\IndependentResources_OneFailure.yml" />
<None Remove="TestData\configuration\Init-TestRepository.ps1" />
<None Remove="TestData\Configuration\ShowDetails_TestRepo.yml" />
</ItemGroup>

<ItemGroup>
<Content Include="..\..\doc\admx\DesktopAppInstaller.admx" Link="TestData\DesktopAppInstaller.admx">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
Expand Down
46 changes: 46 additions & 0 deletions src/AppInstallerCLIE2ETests/InstallCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -671,5 +671,51 @@ public void InstallWithPackageDependency_RefreshPathVariable()
TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true);
Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(testDir));
}

/// <summary>
/// This test flow is intended to test an EXE that actually installs an MSIX internally, and whose name+publisher
/// information resembles an existing installation. Given this, the goal is to get correlation to stick to the
/// MSIX rather than the ARP entry that we would match with in the absence of the package family name being present.
/// </summary>
[Test]
public void InstallExeThatInstallsMSIX()
{
string targetPackageIdentifier = "AppInstallerTest.TestExeInstallerInstallsMSIX";
string fakeProductCode = "e35f5799-cce3-41fd-886c-c36fcb7104fe";

// Insert fake ARP entry as if a non-MSIX version of the package is already installed.
// The name here must not match the normalized name of the package, but be close enough to meet
// the confidence requirements for correlation after an install operation (so we drop one word here).
TestCommon.CreateARPEntry(fakeProductCode, new
{
DisplayName = "EXE Installer that Installs MSIX",
Publisher = "AppInstallerTest",
DisplayVersion = "1.0.0",
});

// We should not find it before installing because the normalized name doesn't match
var result = TestCommon.RunAICLICommand("list", targetPackageIdentifier);
Assert.AreEqual(Constants.ErrorCode.ERROR_NO_APPLICATIONS_FOUND, result.ExitCode);

// Add the MSIX to simulate an installer doing it
TestCommon.InstallMsix(TestCommon.MsixInstallerPath);

// Install our exe that "installs" the MSIX
result = TestCommon.RunAICLICommand("install", $"{targetPackageIdentifier} --force");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);

// We should find the package now, and it should be correlated to the MSIX (although we don't actually know that from this probe)
result = TestCommon.RunAICLICommand("list", targetPackageIdentifier);
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);

// Remove the MSIX outside of winget's knowledge to keep the tracking data.
TestCommon.RemoveMsix(Constants.MsixInstallerName);

// We should not find the package now that the MSIX is gone, confirming that it was correlated
result = TestCommon.RunAICLICommand("list", targetPackageIdentifier);
Assert.AreEqual(Constants.ErrorCode.ERROR_NO_APPLICATIONS_FOUND, result.ExitCode);

TestCommon.RemoveARPEntry(fakeProductCode);
}
}
}
43 changes: 41 additions & 2 deletions src/AppInstallerCLIE2ETests/TestCommon.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace AppInstallerCLIE2ETests
using System;
using System.Diagnostics;
using System.IO;
using System.Reflection;
using System.Threading;
using Microsoft.Win32;
using NUnit.Framework;
Expand Down Expand Up @@ -701,8 +702,7 @@ public static bool VerifyTestMsixUninstalled(bool isProvisioned = false)
/// <param name="value">Value.</param>
public static void ModifyPortableARPEntryValue(string productCode, string name, string value)
{
const string uninstallSubKey = @"Software\Microsoft\Windows\CurrentVersion\Uninstall";
using (RegistryKey uninstallRegistryKey = Registry.CurrentUser.OpenSubKey(uninstallSubKey, true))
using (RegistryKey uninstallRegistryKey = Registry.CurrentUser.OpenSubKey(Constants.UninstallSubKey, true))
{
RegistryKey entry = uninstallRegistryKey.OpenSubKey(productCode, true);
entry.SetValue(name, value);
Expand Down Expand Up @@ -798,6 +798,45 @@ public static void EnsureModuleState(string moduleName, bool present, string rep
}
}

/// <summary>
/// Creates an ARP entry from the given values.
/// </summary>
/// <param name="productCode">Product code of the entry.</param>
/// <param name="properties">The properties to set in the entry.</param>
/// <param name="scope">Scope of the entry.</param>
public static void CreateARPEntry(
string productCode,
object properties,
Scope scope = Scope.User)
{
RegistryKey baseKey = (scope == Scope.User) ? Registry.CurrentUser : Registry.LocalMachine;
using (RegistryKey uninstallRegistryKey = baseKey.OpenSubKey(Constants.UninstallSubKey, true))
{
RegistryKey entry = uninstallRegistryKey.CreateSubKey(productCode, true);

foreach (PropertyInfo property in properties.GetType().GetProperties())
{
entry.SetValue(property.Name, property.GetValue(properties));
}
}
}

/// <summary>
/// Removes an ARP entry.
/// </summary>
/// <param name="productCode">Product code of the entry.</param>
/// <param name="scope">Scope of the entry.</param>
public static void RemoveARPEntry(
string productCode,
Scope scope = Scope.User)
{
RegistryKey baseKey = (scope == Scope.User) ? Registry.CurrentUser : Registry.LocalMachine;
using (RegistryKey uninstallRegistryKey = baseKey.OpenSubKey(Constants.UninstallSubKey, true))
{
uninstallRegistryKey.DeleteSubKey(productCode);
}
}

/// <summary>
/// Run command result.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
Id: AppInstallerTest.TestExeInstallerInstallsMSIX
Name: EXE Installer that Installs MSIX - extra
Version: 1.0.0.0
Publisher: AppInstallerTest
License: Test
Installers:
- Arch: x86
Url: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe
Sha256: <EXEHASH>
InstallerType: exe
PackageFamilyName: 6c6338fe-41b7-46ca-8ba6-b5ad5312bb0e_8wekyb3d8bbwe
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the intended way of doing the correlation? I thought that's what the AppsAndFeaturesEntries were for, so that the different InstallerType could be specified. In fact, this isn't the first time I've seen the issue where an exe installs MSIX packages -

If this is the intended way, that's great, I would only ask that it be documented somewhere that the PackageFamilyName may be used for correlation even though it is outside of the AppsAndFeaturesEntries, since based on the current manifest documentation, I would have thought that it wouldn't be used. And then, since the package behaves differently based on the OS Version, that there would be an installer node for each MinimumOSVersion that has a unique AppsAndFeaturesEntries to make the correlation unambiguous.

Copy link
Member Author

Choose a reason for hiding this comment

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

AppsAndFeaturesEntries is for the Add/Remove Programs data in the Uninstall registry key. It doesn't contain the package family name:

"AppsAndFeaturesEntry": {
"type": "object",
"properties": {
"DisplayName": {
"type": [ "string", "null" ],
"minLength": 1,
"maxLength": 256,
"description": "The DisplayName registry value"
},
"Publisher": {
"type": [ "string", "null" ],
"minLength": 1,
"maxLength": 256,
"description": "The Publisher registry value"
},
"DisplayVersion": {
"type": [ "string", "null" ],
"minLength": 1,
"maxLength": 128,
"description": "The DisplayVersion registry value"
},
"ProductCode": {
"$ref": "#/definitions/ProductCode"
},
"UpgradeCode": {
"$ref": "#/definitions/ProductCode"
},
"InstallerType": {
"$ref": "#/definitions/InstallerType"
}
},
"description": "Various key values under installer's ARP entry"
},

Yes, the WinApp SDK / Runtime is what caused us to open that value up to installer types that were not definitely MSIX based. Where would you suggest that it be documented that Installer > PackageFamilyName be used when an MSIX is installed?

The OS version targeting portion is relevant to the real manifest in question, not the test one. I don't think it should be necessary if we can help it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, that clarifies things. I think it can be documented at pkgs in the manifest docs; I can do that after this is merged.

Thank you for the explanation!

Switches:
Custom: /execustom
SilentWithProgress: /exeswp
Silent: /exesilent
Interactive: /exeinteractive
Language: /exeenus
Log: /LogFile <LOGPATH>
InstallLocation: /InstallDir <INSTALLPATH>
ManifestVersion: 0.1.0
10 changes: 10 additions & 0 deletions src/AppInstallerCommonCore/Deployment.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -312,4 +312,14 @@ namespace AppInstaller::Deployment
RemovePackage(packageFullName, RemovalOptions::RemoveForAllUsers, progress);
}
}

bool IsRegistered(std::string_view packageFamilyName)
{
std::wstring wideFamilyName = Utility::ConvertToUTF16(packageFamilyName);

PackageManager packageManager;
auto packages = packageManager.FindPackagesForUser({}, wideFamilyName);

return packages.begin() != packages.end();
}
}
3 changes: 3 additions & 0 deletions src/AppInstallerCommonCore/Public/AppInstallerDeployment.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,7 @@ namespace AppInstaller::Deployment
std::string_view packageFamilyName,
std::string_view packageFullName,
IProgressCallback& callback);

// Calls winrt::Windows::Management::Deployment::PackageManager::FindPackagesForUser
bool IsRegistered(std::string_view packageFamilyName);
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>
<TargetFramework>net6.0</TargetFramework>
</PropertyGroup>

<ItemGroup>
Expand Down