From 028cbe976725f88cf3bf07672ecbbc0a23fefb42 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 2 Mar 2022 16:33:59 -0800 Subject: [PATCH 1/3] Rename source auto update policy --- doc/admx/DesktopAppInstaller.admx | 4 +- doc/admx/en-US/DesktopAppInstaller.adml | 8 ++-- src/AppInstallerCLITests/GroupPolicy.cpp | 44 +++++++++++++++++++ src/AppInstallerCLITests/TestSettings.h | 3 +- .../WorkflowGroupPolicy.cpp | 14 +++--- src/AppInstallerCommonCore/GroupPolicy.cpp | 12 ++++- .../Public/winget/GroupPolicy.h | 2 +- 7 files changed, 71 insertions(+), 16 deletions(-) diff --git a/doc/admx/DesktopAppInstaller.admx b/doc/admx/DesktopAppInstaller.admx index e0220de956..e70bfa3c63 100644 --- a/doc/admx/DesktopAppInstaller.admx +++ b/doc/admx/DesktopAppInstaller.admx @@ -83,11 +83,11 @@ - + - + diff --git a/doc/admx/en-US/DesktopAppInstaller.adml b/doc/admx/en-US/DesktopAppInstaller.adml index 969295a9c6..da3cc4ee7b 100644 --- a/doc/admx/en-US/DesktopAppInstaller.adml +++ b/doc/admx/en-US/DesktopAppInstaller.adml @@ -53,8 +53,8 @@ If you do not configure this setting, the Microsoft Store source for the Windows If you enable this setting, the Microsoft Store source for the Windows Package Manager will be available and cannot be removed. If you disable this setting the Microsoft Store source for the Windows Package Manager will not be available. - Set App Installer Source Auto Update Interval In Minutes - This policy controls the auto update interval for package-based sources. + Set App Installer Source Auto Update Interval In Minutes + This policy controls the auto update interval for package-based sources. If you disable or do not configure this setting, the default interval or the value specified in settings will be used by the Windows Package Manager. @@ -77,8 +77,8 @@ If you enable this policy, only the sources specified can be added or removed fr If you disable this policy, no additional sources can be configured for the Windows Package Manager. - - Source Auto Update Interval In Minutes + + Source Auto Update Interval In Minutes Additional Sources: diff --git a/src/AppInstallerCLITests/GroupPolicy.cpp b/src/AppInstallerCLITests/GroupPolicy.cpp index 3b13835c22..f8eb0a98e8 100644 --- a/src/AppInstallerCLITests/GroupPolicy.cpp +++ b/src/AppInstallerCLITests/GroupPolicy.cpp @@ -60,6 +60,50 @@ TEST_CASE("GroupPolicy_UpdateInterval", "[groupPolicy]") } } + +TEST_CASE("GroupPolicy_UpdateInterval_OldName", "[groupPolicy]") +{ + auto policiesKey = RegCreateVolatileTestRoot(); + + SECTION("New name shadows old") + { + SECTION("When old is valid") + { + SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, 3); + } + SECTION("When old is invalid") + { + SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, L"Invalid type"); + } + + SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyValueName, 1); + GroupPolicy groupPolicy{ policiesKey.get() }; + + auto policy = groupPolicy.GetValue(); + REQUIRE(policy.has_value()); + REQUIRE(*policy == 1); + } + + SECTION("Fallback to old name") + { + SECTION("When new name has invalid data") + { + SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyValueName, L"Wrong type"); + } + SECTION("When new name is missing") + { + // Don't add the registry key + } + + SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, 20); + GroupPolicy groupPolicy{ policiesKey.get() }; + + auto policy = groupPolicy.GetValue(); + REQUIRE(policy.has_value()); + REQUIRE(*policy == 20); + } +} + TEST_CASE("GroupPolicy_Sources", "[groupPolicy]") { auto policiesKey = RegCreateVolatileTestRoot(); diff --git a/src/AppInstallerCLITests/TestSettings.h b/src/AppInstallerCLITests/TestSettings.h index 45a517b063..24cbd15a5f 100644 --- a/src/AppInstallerCLITests/TestSettings.h +++ b/src/AppInstallerCLITests/TestSettings.h @@ -19,7 +19,8 @@ namespace TestCommon const std::wstring AdditionalSourcesPolicyValueName = L"EnableAdditionalSources"; const std::wstring AllowedSourcesPolicyValueName = L"EnableAllowedSources"; - const std::wstring SourceUpdateIntervalPolicyValueName = L"SourceAutoUpdateIntervalInMinutes"; + const std::wstring SourceUpdateIntervalPolicyValueName = L"SourceAutoUpdateInterval"; + const std::wstring SourceUpdateIntervalPolicyOldValueName = L"SourceAutoUpdateIntervalInMinutes"; const std::wstring AdditionalSourcesPolicyKeyName = L"AdditionalSources"; const std::wstring AllowedSourcesPolicyKeyName = L"AllowedSources"; diff --git a/src/AppInstallerCLITests/WorkflowGroupPolicy.cpp b/src/AppInstallerCLITests/WorkflowGroupPolicy.cpp index 98f9a73175..75c192ba77 100644 --- a/src/AppInstallerCLITests/WorkflowGroupPolicy.cpp +++ b/src/AppInstallerCLITests/WorkflowGroupPolicy.cpp @@ -98,7 +98,7 @@ TEST_CASE("GroupPolicy_Info", "[groupPolicy]") Execution::Context context{ output, std::cin }; context.Args.AddArg(Execution::Args::Type::Info); RootCommand rootCommand({}); - + SECTION("Does not list not configured") { rootCommand.Execute(context); @@ -109,9 +109,9 @@ TEST_CASE("GroupPolicy_Info", "[groupPolicy]") } SECTION("Shows enabled policies") { - policies.SetState(TogglePolicy::Policy::HashOverride, PolicyState::Enabled); + policies.SetState(TogglePolicy::Policy::HashOverride, PolicyState::Enabled); - rootCommand.Execute(context); + rootCommand.Execute(context); INFO(output.str()); REQUIRE_FALSE(context.IsTerminated()); @@ -120,7 +120,7 @@ TEST_CASE("GroupPolicy_Info", "[groupPolicy]") } SECTION("Shows disabled policies") { - policies.SetState(TogglePolicy::Policy::LocalManifestFiles, PolicyState::Disabled); + policies.SetState(TogglePolicy::Policy::LocalManifestFiles, PolicyState::Disabled); rootCommand.Execute(context); INFO(output.str()); @@ -131,7 +131,7 @@ TEST_CASE("GroupPolicy_Info", "[groupPolicy]") } SECTION("Shows auto update interval") { - policies.SetValue(60); + policies.SetValue(60); rootCommand.Execute(context); INFO(output.str()); @@ -147,7 +147,7 @@ TEST_CASE("GroupPolicy_Info", "[groupPolicy]") source.Type = "Test.Type"; source.Arg = "test-arg"; policies.SetState(TogglePolicy::Policy::AdditionalSources, PolicyState::Enabled); - policies.SetValue({ source }); + policies.SetValue({ source }); rootCommand.Execute(context); INFO(output.str()); @@ -167,7 +167,7 @@ TEST_CASE("GroupPolicy_Info", "[groupPolicy]") source.Type = "Test.Type"; source.Arg = "test-arg"; policies.SetState(TogglePolicy::Policy::AllowedSources, PolicyState::Enabled); - policies.SetValue({ source }); + policies.SetValue({ source }); rootCommand.Execute(context); INFO(output.str()); diff --git a/src/AppInstallerCommonCore/GroupPolicy.cpp b/src/AppInstallerCommonCore/GroupPolicy.cpp index 88db9873f4..82d64974c2 100644 --- a/src/AppInstallerCommonCore/GroupPolicy.cpp +++ b/src/AppInstallerCommonCore/GroupPolicy.cpp @@ -207,8 +207,18 @@ namespace AppInstaller::Settings std::optional ValuePolicyMapping::ReadAndValidate(const Registry::Key& policiesKey) { + // This policy used to have another name in the registry. + // Try to read first with the current name, and if it's not present + // check if the old name is present. using Mapping = ValuePolicyMapping; - return GetRegistryValue(policiesKey, Mapping::ValueName); + auto registryValue = GetRegistryValue(policiesKey, Mapping::ValueName); + + if (!registryValue.has_value()) + { + registryValue = GetRegistryValue(policiesKey, "SourceAutoUpdateIntervalInMinutes"sv); + } + + return registryValue; } std::optional ValuePolicyMapping::ReadAndValidateItem(const Registry::Value& item) diff --git a/src/AppInstallerCommonCore/Public/winget/GroupPolicy.h b/src/AppInstallerCommonCore/Public/winget/GroupPolicy.h index 1a5ad81d93..a1839e9a6b 100644 --- a/src/AppInstallerCommonCore/Public/winget/GroupPolicy.h +++ b/src/AppInstallerCommonCore/Public/winget/GroupPolicy.h @@ -136,7 +136,7 @@ namespace AppInstaller::Settings static std::optional ReadAndValidateItem(const Registry::Value& item); \ ) - POLICY_MAPPING_VALUE_SPECIALIZATION(ValuePolicy::SourceAutoUpdateIntervalInMinutes, uint32_t, "SourceAutoUpdateIntervalInMinutes"sv, Registry::Value::Type::DWord); + POLICY_MAPPING_VALUE_SPECIALIZATION(ValuePolicy::SourceAutoUpdateIntervalInMinutes, uint32_t, "SourceAutoUpdateInterval"sv, Registry::Value::Type::DWord); POLICY_MAPPING_LIST_SPECIALIZATION(ValuePolicy::AdditionalSources, SourceFromPolicy, "AdditionalSources"sv); POLICY_MAPPING_LIST_SPECIALIZATION(ValuePolicy::AllowedSources, SourceFromPolicy, "AllowedSources"sv); From 21e999c949ad8ec6ca124112bf8c5f3f6485b36a Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Thu, 3 Mar 2022 12:48:29 -0800 Subject: [PATCH 2/3] PR comments --- src/AppInstallerCLITests/GroupPolicy.cpp | 21 ++++++---- src/AppInstallerCommonCore/GroupPolicy.cpp | 48 ++++++++++++++-------- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/AppInstallerCLITests/GroupPolicy.cpp b/src/AppInstallerCLITests/GroupPolicy.cpp index f8eb0a98e8..908814596c 100644 --- a/src/AppInstallerCLITests/GroupPolicy.cpp +++ b/src/AppInstallerCLITests/GroupPolicy.cpp @@ -89,18 +89,23 @@ TEST_CASE("GroupPolicy_UpdateInterval_OldName", "[groupPolicy]") SECTION("When new name has invalid data") { SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyValueName, L"Wrong type"); + SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, 20); + GroupPolicy groupPolicy{ policiesKey.get() }; + + // We should not fall back on this case + auto policy = groupPolicy.GetValue(); + REQUIRE(!policy.has_value()); } SECTION("When new name is missing") { - // Don't add the registry key - } - - SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, 20); - GroupPolicy groupPolicy{ policiesKey.get() }; + // Don't add the registry value with the new name + SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, 20); + GroupPolicy groupPolicy{ policiesKey.get() }; - auto policy = groupPolicy.GetValue(); - REQUIRE(policy.has_value()); - REQUIRE(*policy == 20); + auto policy = groupPolicy.GetValue(); + REQUIRE(policy.has_value()); + REQUIRE(*policy == 20); + } } } diff --git a/src/AppInstallerCommonCore/GroupPolicy.cpp b/src/AppInstallerCommonCore/GroupPolicy.cpp index 82d64974c2..a8e9bd02d5 100644 --- a/src/AppInstallerCommonCore/GroupPolicy.cpp +++ b/src/AppInstallerCommonCore/GroupPolicy.cpp @@ -23,8 +23,7 @@ namespace AppInstaller::Settings return (s_override ? *s_override : s_groupPolicy); } - template - std::optional().GetValue())> GetRegistryValue(const Registry::Key& key, const std::string_view valueName) + std::optional GetRegistryValueObject(const Registry::Key& key, const std::string_view valueName) { if (!key) { @@ -32,14 +31,13 @@ namespace AppInstaller::Settings return std::nullopt; } - auto regValue = key[valueName]; - if (!regValue.has_value()) - { - // Value does not exist - return std::nullopt; - } + return key[valueName]; + } - auto value = regValue->TryGetValue(); + template + std::optional().GetValue())> GetRegistryValueData(const Registry::Value& regValue, const std::string_view valueName) + { + auto value = regValue.TryGetValue(); if (!value.has_value()) { AICLI_LOG(Core, Warning, << "Value for policy '" << valueName << "' does not have expected type"); @@ -49,9 +47,22 @@ namespace AppInstaller::Settings return std::move(value.value()); } + template + std::optional().GetValue())> GetRegistryValueData(const Registry::Key& key, const std::string_view valueName) + { + auto regValue = GetRegistryValueObject(key, valueName); + if (!regValue.has_value()) + { + // Value does not exist; there's nothing to return + return std::nullopt; + } + + return GetRegistryValueData(regValue.value(), valueName); + } + std::optional RegistryValueIsTrue(const Registry::Key& key, std::string_view valueName) { - auto intValue = GetRegistryValue(key, valueName); + auto intValue = GetRegistryValueData(key, valueName); if (!intValue.has_value()) { return std::nullopt; @@ -211,14 +222,17 @@ namespace AppInstaller::Settings // Try to read first with the current name, and if it's not present // check if the old name is present. using Mapping = ValuePolicyMapping; - auto registryValue = GetRegistryValue(policiesKey, Mapping::ValueName); - if (!registryValue.has_value()) + auto regValueWithCurrentName = GetRegistryValueObject(policiesKey, Mapping::ValueName); + if (regValueWithCurrentName.has_value()) { - registryValue = GetRegistryValue(policiesKey, "SourceAutoUpdateIntervalInMinutes"sv); + // We use the current name even if it doesn't have valid data. + return GetRegistryValueData(regValueWithCurrentName.value(), Mapping::ValueName); + } + else + { + return GetRegistryValueData(policiesKey, "SourceAutoUpdateIntervalInMinutes"sv); } - - return registryValue; } std::optional ValuePolicyMapping::ReadAndValidateItem(const Registry::Value& item) @@ -238,8 +252,8 @@ namespace AppInstaller::Settings { case TogglePolicy::Policy::WinGet: return TogglePolicy(policy, "EnableAppInstaller"sv, String::PolicyEnableWinGet); - case TogglePolicy::Policy::Settings: return - TogglePolicy(policy, "EnableSettings"sv, String::PolicyEnableWingetSettings); + case TogglePolicy::Policy::Settings: + return TogglePolicy(policy, "EnableSettings"sv, String::PolicyEnableWingetSettings); case TogglePolicy::Policy::ExperimentalFeatures: return TogglePolicy(policy, "EnableExperimentalFeatures"sv, String::PolicyEnableExperimentalFeatures); case TogglePolicy::Policy::LocalManifestFiles: From 93c30e74614a650f963be910f4ef349b67308349 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Mon, 7 Mar 2022 13:54:08 -0800 Subject: [PATCH 3/3] Fix E2E test --- src/AppInstallerCLIE2ETests/GroupPolicyHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AppInstallerCLIE2ETests/GroupPolicyHelper.cs b/src/AppInstallerCLIE2ETests/GroupPolicyHelper.cs index b50a25e457..2b9c70d71c 100644 --- a/src/AppInstallerCLIE2ETests/GroupPolicyHelper.cs +++ b/src/AppInstallerCLIE2ETests/GroupPolicyHelper.cs @@ -79,7 +79,7 @@ public static class Attributes public static GroupPolicyHelper EnableMicrosoftStoreSource = new GroupPolicyHelper("EnableMicrosoftStoreSource"); public static GroupPolicyHelper EnableAdditionalSources = new GroupPolicyHelper("EnableAdditionalSources", "AdditionalSources"); public static GroupPolicyHelper EnableAllowedSources = new GroupPolicyHelper("EnableAllowedSources", "AllowedSources"); - public static GroupPolicyHelper SourceAutoUpdateInterval = new GroupPolicyHelper("SourceAutoUpdateIntervalInMinutes", "SourceAutoUpdateIntervalInMinutes"); + public static GroupPolicyHelper SourceAutoUpdateInterval = new GroupPolicyHelper("SourceAutoUpdateInterval", "SourceAutoUpdateInterval"); private static GroupPolicyHelper[] AllPolicies = new GroupPolicyHelper[] {