[AppLaunchAutomation] Fix templates not populating.

This Cl adds in a fix that prevented app launch automation templates from populating correctly. This is done by having the admin template service observe the preference store rather than depend on being called by the policy handler. Design: go/admin-templates-storage-design Bug: b/281857868 Change-Id: I67ce46a75979f3424af6c882dfe9fa78918ba59a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4598416 Reviewed-by: Dominic Battre <battre@chromium.org> Reviewed-by: Alexander Hendrich <hendrich@chromium.org> Reviewed-by: Daniel Andersson <dandersson@chromium.org> Commit-Queue: Avynn Donaghe <avynn@google.com> Reviewed-by: Yanzhu Du <yzd@google.com> Cr-Commit-Position: refs/heads/main@{#1158925}
chromium · Jun 16, 2023 · 780fed4 · 780fed4
1 parent eeb856e
commit 780fed4
Show file tree

Hide file tree

Showing 10 changed files with 215 additions and 131 deletions.
diff --git a/ash/wm/desks/templates/saved_desk_test_helper.cc b/ash/wm/desks/templates/saved_desk_test_helper.cc
@@ -10,13 +10,16 @@
 #include "components/desks_storage/core/admin_template_service.h"
 #include "components/desks_storage/core/desk_test_util.h"
 #include "components/desks_storage/core/local_desk_data_manager.h"
+#include "components/prefs/testing_pref_service.h"
 
 namespace ash {
 
 SavedDeskTestHelper::SavedDeskTestHelper()
     : account_id_(AccountId::FromUserEmail("test@gmail.com")) {
   CHECK(desk_model_data_dir_.CreateUniqueTempDir());
 
+  test_pref_service_ = std::make_unique<TestingPrefServiceSimple>();
+
   saved_desk_model_ = std::make_unique<desks_storage::LocalDeskDataManager>(
       desk_model_data_dir_.GetPath(), account_id_);
 
@@ -25,7 +28,8 @@ SavedDeskTestHelper::SavedDeskTestHelper()
   // client.
   admin_template_service_ =
       std::make_unique<desks_storage::AdminTemplateService>(
-          desk_model_data_dir_.GetPath(), account_id_);
+          desk_model_data_dir_.GetPath(), account_id_,
+          test_pref_service_.get());
 
   // Install desk model.
   static_cast<TestSavedDeskDelegate*>(Shell::Get()->saved_desk_delegate())

diff --git a/ash/wm/desks/templates/saved_desk_test_helper.h b/ash/wm/desks/templates/saved_desk_test_helper.h
@@ -20,6 +20,8 @@ class AdminTemplateService;
 class DeskModel;
 }
 
+class TestingPrefServiceSimple;
+
 namespace ash {
 
 // This class creates a desk model and has functionality used by unit tests that
@@ -44,6 +46,10 @@ class SavedDeskTestHelper {
 
   desks_storage::DeskModel* desk_model() { return saved_desk_model_.get(); }
 
+  TestingPrefServiceSimple* test_pref_service() {
+    return test_pref_service_.get();
+  }
+
  private:
   AccountId account_id_;
 
@@ -54,6 +60,8 @@ class SavedDeskTestHelper {
   std::unique_ptr<desks_storage::DeskModel> saved_desk_model_;
 
   std::unique_ptr<apps::AppRegistryCache> cache_;
+
+  std::unique_ptr<TestingPrefServiceSimple> test_pref_service_;
 };
 
 }  // namespace ash

diff --git a/chrome/browser/ash/policy/handlers/app_launch_automation_policy_browsertest.cc b/chrome/browser/ash/policy/handlers/app_launch_automation_policy_browsertest.cc
@@ -5,12 +5,14 @@
 #include "chrome/browser/ash/policy/handlers/app_launch_automation_policy_handler.h"
 
 #include "ash/constants/ash_features.h"
+#include "ash/constants/ash_pref_names.h"
 #include "ash/public/cpp/saved_desk_delegate.h"
 #include "ash/shell.h"
 #include "base/json/json_reader.h"
 #include "base/json/values_util.h"
 #include "base/run_loop.h"
 #include "base/test/scoped_feature_list.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/desks_storage/core/admin_template_service.h"
 #include "components/desks_storage/core/desk_test_util.h"
@@ -30,19 +32,6 @@ base::Value ParsePolicyFromString(base::StringPiece policy) {
 
   return std::move(parsed_json.value());
 }
-
-policy::PolicyMap GetPolicyMap(const base::Value& policy_value) {
-  policy::PolicyMap::Entry entry(
-      policy::PolicyLevel::POLICY_LEVEL_RECOMMENDED,
-      policy::PolicyScope::POLICY_SCOPE_USER,
-      policy::PolicySource::POLICY_SOURCE_ENTERPRISE_DEFAULT,
-      absl::optional<base::Value>(policy_value.Clone()),
-      /*extenal_data_fetcher =*/nullptr);
-
-  policy::PolicyMap map;
-  map.Set(policy::key::kAppLaunchAutomation, std::move(entry));
-  return map;
-}
 }  // namespace
 
 class AppLaunchAutomationPolicyTest : public InProcessBrowserTest {
@@ -69,63 +58,66 @@ class AppLaunchAutomationPolicyTest : public InProcessBrowserTest {
     }
   }
 
-  PrefValueMap* GetPrefValueMap() { return &pref_map_; }
-
-  // Returns a policy map containing the correct value.
-  policy::PolicyMap GetStandardPolicyMap() {
-    return GetPolicyMap(ParsePolicyFromString(
-        desks_storage::desk_test_util::kAdminTemplatePolicy));
+  // Sets the correct policy.
+  void SetStandardPolicy() {
+    ProfileManager::GetPrimaryUserProfile()->GetPrefs()->SetList(
+        ash::prefs::kAppLaunchAutomation,
+        ParsePolicyFromString(
+            desks_storage::desk_test_util::kAdminTemplatePolicy)
+            .TakeList());
   }
 
-  policy::PolicyMap GetModifiedPolicyMap() {
-    return GetPolicyMap(ParsePolicyFromString(
-        desks_storage::desk_test_util::kAdminTemplatePolicyWithOneTemplate));
+  void SetModifiedPolicy() {
+    ProfileManager::GetPrimaryUserProfile()->GetPrefs()->SetList(
+        ash::prefs::kAppLaunchAutomation,
+        ParsePolicyFromString(
+            desks_storage::desk_test_util::kAdminTemplatePolicyWithOneTemplate)
+            .TakeList());
   }
 
-  policy::PolicyMap GetEmptyPolicyMap() {
-    return GetPolicyMap(base::Value(base::Value::List()));
+  void SetEmptyPolicy() {
+    ProfileManager::GetPrimaryUserProfile()->GetPrefs()->SetList(
+        ash::prefs::kAppLaunchAutomation, base::Value::List());
   }
 
  private:
-  PrefValueMap pref_map_;
-
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_F(AppLaunchAutomationPolicyTest,
                        AppliesPolicySettingCorrectly) {
   WaitForAdminTemplateService();
-  policy::AppLaunchAutomationPolicyHandler handler(policy::GetChromeSchema());
-
-  handler.ApplyPolicySettings(GetStandardPolicyMap(), GetPrefValueMap());
+  SetStandardPolicy();
+  base::RunLoop().RunUntilIdle();
 
   auto* admin_service = GetAdminService();
   ASSERT_TRUE(admin_service != nullptr);
 
   EXPECT_EQ(admin_service->GetFullDeskModel()->GetEntryCount(), 2UL);
+
+  SetEmptyPolicy();
 }
 
 IN_PROC_BROWSER_TEST_F(AppLaunchAutomationPolicyTest,
                        AppliesModifiedPolicySettingCorrectly) {
   WaitForAdminTemplateService();
-  policy::AppLaunchAutomationPolicyHandler handler(policy::GetChromeSchema());
-
-  handler.ApplyPolicySettings(GetStandardPolicyMap(), GetPrefValueMap());
-  handler.ApplyPolicySettings(GetModifiedPolicyMap(), GetPrefValueMap());
+  SetStandardPolicy();
+  SetModifiedPolicy();
+  base::RunLoop().RunUntilIdle();
 
   auto* admin_service = GetAdminService();
   ASSERT_TRUE(admin_service != nullptr);
 
   EXPECT_EQ(admin_service->GetFullDeskModel()->GetEntryCount(), 1UL);
+  SetEmptyPolicy();
 }
 
 IN_PROC_BROWSER_TEST_F(AppLaunchAutomationPolicyTest,
                        AppliesEmptyPolicySettingCorrectly) {
   WaitForAdminTemplateService();
-  policy::AppLaunchAutomationPolicyHandler handler(policy::GetChromeSchema());
-
-  handler.ApplyPolicySettings(GetStandardPolicyMap(), GetPrefValueMap());
-  handler.ApplyPolicySettings(GetEmptyPolicyMap(), GetPrefValueMap());
+  SetStandardPolicy();
+  SetEmptyPolicy();
+  base::RunLoop().RunUntilIdle();
 
   auto* admin_service = GetAdminService();
   ASSERT_TRUE(admin_service != nullptr);
@@ -136,13 +128,13 @@ IN_PROC_BROWSER_TEST_F(AppLaunchAutomationPolicyTest,
 IN_PROC_BROWSER_TEST_F(AppLaunchAutomationPolicyTest,
                        AppliesAdditionalPolicySettingCorrectly) {
   WaitForAdminTemplateService();
-  policy::AppLaunchAutomationPolicyHandler handler(policy::GetChromeSchema());
-
-  handler.ApplyPolicySettings(GetModifiedPolicyMap(), GetPrefValueMap());
-  handler.ApplyPolicySettings(GetStandardPolicyMap(), GetPrefValueMap());
+  SetModifiedPolicy();
+  SetStandardPolicy();
+  base::RunLoop().RunUntilIdle();
 
   auto* admin_service = GetAdminService();
   ASSERT_TRUE(admin_service != nullptr);
 
   EXPECT_EQ(admin_service->GetFullDeskModel()->GetEntryCount(), 2UL);
+  SetEmptyPolicy();
 }
diff --git a/chrome/browser/ash/policy/handlers/app_launch_automation_policy_handler.cc b/chrome/browser/ash/policy/handlers/app_launch_automation_policy_handler.cc
@@ -18,81 +18,6 @@
 #include "components/policy/policy_constants.h"
 #include "components/prefs/pref_value_map.h"
 
-namespace {
-
-void UpdateModelWithPolicy(
-    std::vector<std::unique_ptr<ash::DeskTemplate>> desk_templates,
-    desks_storage::DeskModel* desk_model) {
-  if (desk_model == nullptr) {
-    return;
-  }
-
-  // If templates exist that aren't in the current policy we should delete them.
-  std::vector<base::Uuid> desk_uuids_to_delete = desk_model->GetAllEntryUuids();
-  std::set<base::Uuid> desk_uuids_to_delete_set(desk_uuids_to_delete.begin(),
-                                                desk_uuids_to_delete.end());
-
-  for (auto& desk_template : desk_templates) {
-    // Something went wrong when parsing the template.
-    if (desk_template == nullptr) {
-      continue;
-    }
-
-    // Something has gone wrong if the field isn't a dict.
-    CHECK(desk_template->policy_definition().type() == base::Value::Type::DICT);
-
-    // Query model to determine if this entry exists already.
-    auto get_entry_result = desk_model->GetEntryByUUID(desk_template->uuid());
-    auto entry_status = get_entry_result.status;
-
-    // If this template exists in the current policy then don't delete it after
-    // updating the locally stored policy. Note: this call is a noop if the
-    // template in question is a new template.
-    if (entry_status == desks_storage::DeskModel::GetEntryByUuidStatus::kOk ||
-        entry_status ==
-            desks_storage::DeskModel::GetEntryByUuidStatus::kNotFound) {
-      desk_uuids_to_delete_set.erase(desk_template->uuid());
-
-      // There was an error when retrieving the template, do nothing and delete
-      // the template.
-    } else {
-      continue;
-    }
-
-    // If the policy template already exists in the model and has been unchanged
-    // since the last policy update don't overwrite the data.  This will
-    // preserve the user's window information for that template.
-    if (entry_status == desks_storage::DeskModel::GetEntryByUuidStatus::kOk &&
-        get_entry_result.entry->policy_definition() ==
-            desk_template->policy_definition()) {
-      continue;
-    }
-
-    // If the policy template exists in an updated form or is new then either
-    // add it to the model or overwrite the existing definition.
-    desk_model->AddOrUpdateEntry(std::move(desk_template), base::DoNothing());
-  }
-
-  // Remove all templates that aren't in the policy.  If the policy is empty
-  // then this will remove all admin templates from the device.
-  for (auto uuid : desk_uuids_to_delete_set) {
-    desk_model->DeleteEntry(uuid, base::DoNothing());
-  }
-}
-
-desks_storage::DeskModel* GetDeskModel() {
-  auto* admin_template_service =
-      ash::Shell::Get()->saved_desk_delegate()->GetAdminTemplateService();
-
-  if (admin_template_service == nullptr) {
-    return nullptr;
-  }
-
-  return admin_template_service->GetFullDeskModel();
-}
-
-}  // namespace
-
 namespace policy {
 
 AppLaunchAutomationPolicyHandler::AppLaunchAutomationPolicyHandler(
@@ -120,11 +45,6 @@ void AppLaunchAutomationPolicyHandler::ApplyPolicySettings(
     return;
   }
 
-  UpdateModelWithPolicy(
-      desks_storage::desk_template_conversion::
-          ParseAdminTemplatesFromPolicyValue(policy_value->Clone()),
-      GetDeskModel());
-
   prefs->SetValue(ash::prefs::kAppLaunchAutomation,
                   base::Value::FromUniquePtrValue(std::move(policy_value)));
 }

diff --git a/chrome/browser/ui/ash/desks/admin_template_service_factory.cc b/chrome/browser/ui/ash/desks/admin_template_service_factory.cc
@@ -41,11 +41,10 @@ AdminTemplateServiceFactory::AdminTemplateServiceFactory()
 KeyedService* AdminTemplateServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   Profile* profile = Profile::FromBrowserContext(context);
-  const AccountId account_id =
-      multi_user_util::GetAccountIdFromProfile(profile);
 
-  return new desks_storage::AdminTemplateService(profile->GetPath(),
-                                                 account_id);
+  return new desks_storage::AdminTemplateService(
+      profile->GetPath(), multi_user_util::GetAccountIdFromProfile(profile),
+      profile->GetPrefs());
 }
 
 }  // namespace ash
diff --git a/components/desks_storage/BUILD.gn b/components/desks_storage/BUILD.gn
@@ -34,6 +34,8 @@ static_library("desks_storage") {
     "//components/account_id",
     "//components/app_constants",
     "//components/app_restore",
+    "//components/policy/core/common",
+    "//components/prefs",
     "//components/sync",
     "//components/sync/model",
     "//components/sync/protocol",

diff --git a/components/desks_storage/DEPS b/components/desks_storage/DEPS
@@ -6,6 +6,8 @@ include_rules = [
   "+components/account_id/account_id.h",
   "+components/app_constants/constants.h",
   "+components/app_restore",
+  "+components/prefs",
+  "+components/policy",
   "+components/keyed_service/core",
   "+components/services/app_service/public/cpp",
   "+components/sync",