Skip to content

Commit

Permalink
Fix bugs with display paths (#3157)
Browse files Browse the repository at this point in the history
Change
This change fixes two bugs with display paths:

The recent change to use case insensitive ICU for directory name equality inverted the comparison, making the replacement not happen correctly.
The recent change to enable display paths generically left them marked for creation, resulting in errors if directories that required administrator privileges to create did not exist and were requested from an unelevated context (ex. run winget --info from medium IL process when the portable machine links location(s) did not exist).
In addition, I tried to generally make forDisplay apply broadly since it is now allowed to be requested on any path.

Validation
winget --info now contains the expected output instead of an error (2) or the full paths containing the user's account name (1).
A few unit tests that were being hit by the error are also working now.
  • Loading branch information
JohnMcPMS authored Apr 18, 2023
1 parent 16fd465 commit 578f451
Show file tree
Hide file tree
Showing 10 changed files with 128 additions and 61 deletions.
12 changes: 6 additions & 6 deletions src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,6 @@
<ClCompile Include="Archive.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="Filesystem.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="FolderFileWatcher.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
Expand All @@ -242,9 +239,6 @@
<ClCompile Include="PackageDependenciesValidationUtil.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="RestInterface_1_4.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="AdminSettings.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
Expand Down Expand Up @@ -293,6 +287,12 @@
<ClCompile Include="WindowsFeature.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="Filesystem.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="RestInterface_1_4.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<None Include="PropertySheet.props" />
Expand Down
16 changes: 15 additions & 1 deletion src/AppInstallerCLITests/Filesystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,18 @@ TEST_CASE("VerifyIsSameVolume", "[filesystem]")
REQUIRE_FALSE(IsSameVolume(path5, path6));
REQUIRE_FALSE(IsSameVolume(path4, path6));
REQUIRE_FALSE(IsSameVolume(path6, path8));
}
}

TEST_CASE("ReplaceCommonPathPrefix", "[filesystem]")
{
std::filesystem::path prefix = "C:\\test1\\test2";
std::string replacement = "%TEST%";

std::filesystem::path shouldReplace = "C:\\test1\\test2\\subdir1\\subdir2";
REQUIRE(ReplaceCommonPathPrefix(shouldReplace, prefix, replacement));
REQUIRE(shouldReplace.u8string() == (replacement + "\\subdir1\\subdir2"));

std::filesystem::path shouldNotReplace = "C:\\test1\\test3\\subdir1\\subdir2";
REQUIRE(!ReplaceCommonPathPrefix(shouldNotReplace, prefix, replacement));
REQUIRE(shouldNotReplace.u8string() == "C:\\test1\\test3\\subdir1\\subdir2");
}
22 changes: 21 additions & 1 deletion src/AppInstallerCLITests/Runtime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// Licensed under the MIT License.
#include "pch.h"
#include "TestCommon.h"
#include "TestHooks.h"
#include <AppInstallerRuntime.h>
#include <winget/Filesystem.h>

using namespace AppInstaller;
using namespace AppInstaller::Runtime;
Expand Down Expand Up @@ -113,4 +115,22 @@ TEST_CASE("VerifyDevModeEnabledCheck", "[runtime]")

REQUIRE(modifiedState != initialState);
REQUIRE(revertedState == initialState);
}
}

TEST_CASE("EnsureUserProfileNotPresentInDisplayPaths", "[runtime]")
{
// Clear the overrides that we use when testing as they don't consider display purposes
Runtime::TestHook_ClearPathOverrides();
auto restorePaths = wil::scope_exit([]() { TestCommon::SetTestPathOverrides(); });

std::filesystem::path userProfilePath = Filesystem::GetKnownFolderPath(FOLDERID_Profile);
std::string userProfileString = userProfilePath.u8string();

for (auto i = ToIntegral(ToEnum<PathName>(0)); i < ToIntegral(PathName::Max); ++i)
{
std::filesystem::path displayPath = GetPathTo(ToEnum<PathName>(i), true);
std::string displayPathString = displayPath.u8string();
INFO(i << " = " << displayPathString);
REQUIRE(displayPathString.find(userProfileString) == std::string::npos);
}
}
13 changes: 13 additions & 0 deletions src/AppInstallerCLITests/TestCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
#include <AppInstallerMsixInfo.h>
#include <AppInstallerDownloader.h>

using namespace AppInstaller;

namespace TestCommon
{
namespace
Expand Down Expand Up @@ -363,4 +365,15 @@ namespace TestCommon

return root;
}

void SetTestPathOverrides()
{
// Force all tests to run against settings inside this container.
// This prevents test runs from trashing the users actual settings.
Runtime::TestHook_SetPathOverride(Runtime::PathName::LocalState, Runtime::GetPathTo(Runtime::PathName::LocalState) / "Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::UserFileSettings, Runtime::GetPathTo(Runtime::PathName::UserFileSettings) / "Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::StandardSettings, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForRead, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "WinGet_SecureSettings_Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForWrite, Runtime::GetPathDetailsFor(Runtime::PathName::SecureSettingsForRead));
}
}
3 changes: 3 additions & 0 deletions src/AppInstallerCLITests/TestCommon.h
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,7 @@ namespace TestCommon

// Convert to Json::Value
Json::Value ConvertToJson(const std::string& content);

// Sets up the test path overrides.
void SetTestPathOverrides();
}
9 changes: 1 addition & 8 deletions src/AppInstallerCLITests/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include <Telemetry/TraceLogging.h>

#include "TestCommon.h"
#include "TestHooks.h"
#include "TestSettings.h"

using namespace winrt;
Expand Down Expand Up @@ -151,13 +150,7 @@ int main(int argc, char** argv)
// Forcibly enable event writing to catch bugs in that code
g_IsTelemetryProviderEnabled = true;

// Force all tests to run against settings inside this container.
// This prevents test runs from trashing the users actual settings.
Runtime::TestHook_SetPathOverride(Runtime::PathName::LocalState, Runtime::GetPathTo(Runtime::PathName::LocalState) / "Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::UserFileSettings, Runtime::GetPathTo(Runtime::PathName::UserFileSettings) / "Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::StandardSettings, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForRead, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "WinGet_SecureSettings_Tests");
Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForWrite, Runtime::GetPathDetailsFor(Runtime::PathName::SecureSettingsForRead));
TestCommon::SetTestPathOverrides();

// Remove any existing settings files in the new tests path
TestCommon::UserSettingsFileGuard settingsGuard;
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLITests/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <objbase.h>
#include <urlmon.h>
#include <Msi.h>
#include <KnownFolders.h>

#include <catch.hpp>

Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCommonCore/Filesystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ namespace AppInstaller::Filesystem

while (prefixItr != prefix.end() && sourceItr != source.end())
{
if (Utility::ICUCaseInsensitiveEquals(prefixItr->u8string(), sourceItr->u8string()))
if (!Utility::ICUCaseInsensitiveEquals(prefixItr->u8string(), sourceItr->u8string()))
{
break;
}
Expand Down
2 changes: 2 additions & 0 deletions src/AppInstallerCommonCore/Public/AppInstallerRuntime.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ namespace AppInstaller::Runtime
PortableLinksMachineLocation,
// The root location for the package containing the winget application.
SelfPackageRoot,
// Always one more than the last path; for being able to iterate paths in tests.
Max
};

// The principal that an ACE applies to.
Expand Down
Loading

0 comments on commit 578f451

Please sign in to comment.