Rename source auto update group policy (#1995)

From internal review of the group policies, we are removing "InMinutes" from the name of the policy `SourceAutoUpdateIntervalInMinutes`. This updates the policy definition with the new name, and changes to read the policy from the new registry value (while respecting the old value for compatibility). Updated the tests to use the new value name, and added tests for respecting the old name.
microsoft · Mar 10, 2022 · e50f66b · e50f66b
1 parent 92e662e
commit e50f66b
Show file tree

Hide file tree

Showing 8 changed files with 103 additions and 29 deletions.
diff --git a/doc/admx/DesktopAppInstaller.admx b/doc/admx/DesktopAppInstaller.admx
@@ -83,11 +83,11 @@
         <decimal value="0" />
       </disabledValue>
     </policy>
-    <policy name="SourceAutoUpdateIntervalInMinutes" class="Machine" displayName="$(string.SourceAutoUpdateIntervalInMinutes)" explainText="$(string.SourceAutoUpdateIntervalInMinutesExplanation)" presentation="$(presentation.SourceAutoUpdateIntervalInMinutes)" key="Software\Policies\Microsoft\Windows\AppInstaller">
+    <policy name="SourceAutoUpdateInterval" class="Machine" displayName="$(string.SourceAutoUpdateInterval)" explainText="$(string.SourceAutoUpdateIntervalExplanation)" presentation="$(presentation.SourceAutoUpdateInterval)" key="Software\Policies\Microsoft\Windows\AppInstaller">
       <parentCategory ref="AppInstaller" />
       <supportedOn ref="windows:SUPPORTED_Windows_10_0_RS5" />
       <elements>
-        <decimal id="SourceAutoUpdateIntervalInMinutes" valueName="SourceAutoUpdateIntervalInMinutes" maxValue="43200" />
+        <decimal id="SourceAutoUpdateInterval" valueName="SourceAutoUpdateInterval" maxValue="43200" />
       </elements>
     </policy>
     <policy name="EnableAdditionalSources" class="Machine" displayName="$(string.EnableAdditionalSources)" explainText="$(string.EnableAdditionalSourcesExplanation)" presentation="$(presentation.AdditionalSources)" key="Software\Policies\Microsoft\Windows\AppInstaller" valueName="EnableAdditionalSources">

diff --git 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.</string>
-      <string id="SourceAutoUpdateIntervalInMinutes">Set App Installer Source Auto Update Interval In Minutes</string>
-      <string id="SourceAutoUpdateIntervalInMinutesExplanation">This policy controls the auto update interval for package-based sources.
+      <string id="SourceAutoUpdateInterval">Set App Installer Source Auto Update Interval In Minutes</string>
+      <string id="SourceAutoUpdateIntervalExplanation">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.</string>
     </stringTable>
     <presentationTable>
-      <presentation id="SourceAutoUpdateIntervalInMinutes">
-        <decimalTextBox refId="SourceAutoUpdateIntervalInMinutes" defaultValue="5">Source Auto Update Interval In Minutes</decimalTextBox>
+      <presentation id="SourceAutoUpdateInterval">
+        <decimalTextBox refId="SourceAutoUpdateInterval" defaultValue="5">Source Auto Update Interval In Minutes</decimalTextBox>
       </presentation>
       <presentation id="AdditionalSources">
         <listBox refId="AdditionalSources" required="false">Additional Sources: </listBox>

diff --git 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[]
         {

diff --git a/src/AppInstallerCLITests/GroupPolicy.cpp b/src/AppInstallerCLITests/GroupPolicy.cpp
@@ -60,6 +60,55 @@ 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<ValuePolicy::SourceAutoUpdateIntervalInMinutes>();
+        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");
+            SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, 20);
+            GroupPolicy groupPolicy{ policiesKey.get() };
+
+            // We should not fall back on this case
+            auto policy = groupPolicy.GetValue<ValuePolicy::SourceAutoUpdateIntervalInMinutes>();
+            REQUIRE(!policy.has_value());
+        }
+        SECTION("When new name is missing")
+        {
+            // Don't add the registry value with the new name
+            SetRegistryValue(policiesKey.get(), SourceUpdateIntervalPolicyOldValueName, 20);
+            GroupPolicy groupPolicy{ policiesKey.get() };
+
+            auto policy = groupPolicy.GetValue<ValuePolicy::SourceAutoUpdateIntervalInMinutes>();
+            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
@@ -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
@@ -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<ValuePolicy::SourceAutoUpdateIntervalInMinutes>(60);
+        policies.SetValue<ValuePolicy::SourceAutoUpdateIntervalInMinutes>(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<ValuePolicy::AdditionalSources>({ source });
+        policies.SetValue<ValuePolicy::AdditionalSources>({ 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<ValuePolicy::AllowedSources>({ source });
+        policies.SetValue<ValuePolicy::AllowedSources>({ source });
 
         rootCommand.Execute(context);
         INFO(output.str());

diff --git a/src/AppInstallerCommonCore/GroupPolicy.cpp b/src/AppInstallerCommonCore/GroupPolicy.cpp
@@ -23,23 +23,21 @@ namespace AppInstaller::Settings
             return (s_override ? *s_override : s_groupPolicy);
         }
 
-        template<Registry::Value::Type T>
-        std::optional<decltype(std::declval<Registry::Value>().GetValue<T>())> GetRegistryValue(const Registry::Key& key, const std::string_view valueName)
+        std::optional<Registry::Value> GetRegistryValueObject(const Registry::Key& key, const std::string_view valueName)
         {
             if (!key)
             {
                 // Key does not exist; there's nothing to return
                 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<T>();
+        template<Registry::Value::Type T>
+        std::optional<decltype(std::declval<Registry::Value>().GetValue<T>())> GetRegistryValueData(const Registry::Value& regValue, const std::string_view valueName)
+        {
+            auto value = regValue.TryGetValue<T>();
             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<Registry::Value::Type T>
+        std::optional<decltype(std::declval<Registry::Value>().GetValue<T>())> 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<T>(regValue.value(), valueName);
+        }
+
         std::optional<bool> RegistryValueIsTrue(const Registry::Key& key, std::string_view valueName)
         {
-            auto intValue = GetRegistryValue<Registry::Value::Type::DWord>(key, valueName);
+            auto intValue = GetRegistryValueData<Registry::Value::Type::DWord>(key, valueName);
             if (!intValue.has_value())
             {
                 return std::nullopt;
@@ -207,8 +218,21 @@ namespace AppInstaller::Settings
 
         std::optional<uint32_t> ValuePolicyMapping<ValuePolicy::SourceAutoUpdateIntervalInMinutes>::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<ValuePolicy::SourceAutoUpdateIntervalInMinutes>;
-            return GetRegistryValue<Mapping::ValueType>(policiesKey, Mapping::ValueName);
+
+            auto regValueWithCurrentName = GetRegistryValueObject(policiesKey, Mapping::ValueName);
+            if (regValueWithCurrentName.has_value())
+            {
+                // We use the current name even if it doesn't have valid data.
+                return GetRegistryValueData<Mapping::ValueType>(regValueWithCurrentName.value(), Mapping::ValueName);
+            }
+            else
+            {
+                return GetRegistryValueData<Mapping::ValueType>(policiesKey, "SourceAutoUpdateIntervalInMinutes"sv);
+            }
         }
 
         std::optional<SourceFromPolicy> ValuePolicyMapping<ValuePolicy::AdditionalSources>::ReadAndValidateItem(const Registry::Value& item)
@@ -228,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:

diff --git a/src/AppInstallerCommonCore/Public/winget/GroupPolicy.h b/src/AppInstallerCommonCore/Public/winget/GroupPolicy.h
@@ -136,7 +136,7 @@ namespace AppInstaller::Settings
             static std::optional<item_t> 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);