Don't reinstall a policy app already installed as a policy app

Updating the logic introduced in https://crrev.com/c/3996913 to only
kick off the install of a policy app if it is not already installed as
a policy app.

Bug: 1421075
Change-Id: I24a6fcf1e706c09f7a9015e1807b0f22c25453fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4412319
Auto-Submit: Hoch Hochkeppel <mhochk@microsoft.com>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1135334}

chrome/browser/web_applications/externally_managed_app_manager_impl.cc

@@ -245,8 +245,11 @@ void ExternallyManagedAppManagerImpl::MaybeStartNextOnLockAcquired(
 
       // TODO(crbug.com/1300321): Investigate re-install of the app for all
       // WebAppManagement sources.
-      if (ConvertExternalInstallSourceToSource(
-              install_options.install_source) == WebAppManagement::kPolicy) {
+      if ((ConvertExternalInstallSourceToSource(
+               install_options.install_source) == WebAppManagement::kPolicy) &&
+          (!lock.registrar()
+                .GetAppById(app_id.value())
+                ->IsPolicyInstalledApp())) {
         StartInstallationTask(std::move(front));
         return;
       } else {

chrome/browser/web_applications/externally_managed_app_manager_impl_browsertest.cc

@@ -426,6 +426,35 @@ IN_PROC_BROWSER_TEST_F(ExternallyManagedAppManagerImplBrowserTest,
   }
 }
 
+IN_PROC_BROWSER_TEST_F(ExternallyManagedAppManagerImplBrowserTest,
+                       PolicyAppOverridesUserInstalledApp) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  absl::optional<AppId> app_id;
+  {
+    // Install user app
+    auto install_info = std::make_unique<WebAppInstallInfo>();
+    GURL url(
+        embedded_test_server()->GetURL("/banners/"
+                                       "manifest_test_page.html"));
+    install_info->start_url = url;
+    install_info->title = u"Test user app";
+    app_id = test::InstallWebApp(profile(), std::move(install_info));
+    ASSERT_TRUE(app_id.has_value());
+    ASSERT_TRUE(registrar().WasInstalledByUser(app_id.value()));
+    ASSERT_FALSE(registrar().HasExternalApp(app_id.value()));
+    ASSERT_EQ("Test user app", registrar().GetAppShortName(app_id.value()));
+  }
+  {
+    // Install policy app
+    GURL url(
+        embedded_test_server()->GetURL("/banners/manifest_test_page.html"));
+    absl::optional<AppId> policy_app_id = ForceInstallWebApp(profile(), url);
+    ASSERT_EQ(policy_app_id, app_id);
+    ASSERT_EQ("Manifest test app",
+              registrar().GetAppShortName(policy_app_id.value()));
+  }
+}
+
 // Test that adding a manifest that points to a chrome:// URL does not actually
 // install a web app that points to a chrome:// URL.
 IN_PROC_BROWSER_TEST_P(ExternallyManagedBrowserTestWithPrefMigrationRead,

chrome/browser/web_applications/externally_managed_app_manager_impl_unittest.cc

@@ -872,27 +872,23 @@ TEST_P(ExternallyManagedAppManagerImplTest,
             foo_run_loop.Quit();
           }));
 
-  base::RunLoop().RunUntilIdle();
-  externally_managed_app_manager_impl().SetNextInstallationTaskResult(
-      kFooWebAppUrl, webapps::InstallResultCode::kSuccessNewInstall);
+  foo_run_loop.Run();
 
   externally_managed_app_manager_impl().Install(
       GetInstallOptionsWithWebAppInfo(kFooWebAppUrl),
       base::BindLambdaForTesting(
           [&](const GURL& url,
               ExternallyManagedAppManager::InstallResult result) {
-            EXPECT_EQ(webapps::InstallResultCode::kSuccessNewInstall,
+            EXPECT_EQ(webapps::InstallResultCode::kSuccessAlreadyInstalled,
                       result.code);
             EXPECT_EQ(kFooWebAppUrl, url);
 
-            EXPECT_EQ(2u, install_run_count());
+            EXPECT_EQ(1u, install_run_count());
             EXPECT_EQ(GetInstallOptionsWithWebAppInfo(kFooWebAppUrl),
                       last_install_options());
 
             bar_run_loop.Quit();
           }));
-  foo_run_loop.Run();
-  base::RunLoop().RunUntilIdle();
   bar_run_loop.Run();
 }
 
@@ -1011,15 +1007,13 @@ TEST_P(ExternallyManagedAppManagerImplTest, Install_SerialCallsSameApp) {
   }
 
   {
-    externally_managed_app_manager_impl().SetNextInstallationTaskResult(
-        kFooWebAppUrl, webapps::InstallResultCode::kSuccessNewInstall);
     auto [url, code] = InstallAndWait(&externally_managed_app_manager_impl(),
                                       GetInstallOptions(kFooWebAppUrl));
 
-    EXPECT_EQ(webapps::InstallResultCode::kSuccessNewInstall, code);
+    EXPECT_EQ(webapps::InstallResultCode::kSuccessAlreadyInstalled, code);
     EXPECT_EQ(kFooWebAppUrl, url);
 
-    EXPECT_EQ(2u, install_run_count());
+    EXPECT_EQ(1u, install_run_count());
   }
 }
 
@@ -1038,13 +1032,12 @@ TEST_P(ExternallyManagedAppManagerImplTest, Install_ConcurrentCallsSameApp) {
       base::BindLambdaForTesting(
           [&](const GURL& url,
               ExternallyManagedAppManager::InstallResult result) {
-            EXPECT_EQ(webapps::InstallResultCode::kSuccessNewInstall,
+            EXPECT_EQ(webapps::InstallResultCode::kSuccessAlreadyInstalled,
                       result.code);
             EXPECT_EQ(kFooWebAppUrl, url);
 
-            // Both installation tasks run if the install was triggered
-            // by policy.
-            EXPECT_EQ(2u, install_run_count());
+            // Only the first installation task runs
+            EXPECT_EQ(1u, install_run_count());
             EXPECT_TRUE(first_callback_ran);
             run_loop.Quit();
           }));
@@ -1060,12 +1053,10 @@ TEST_P(ExternallyManagedAppManagerImplTest, Install_ConcurrentCallsSameApp) {
             EXPECT_EQ(1u, install_run_count());
             EXPECT_EQ(GetInstallOptions(kFooWebAppUrl), last_install_options());
             first_callback_ran = true;
-            externally_managed_app_manager_impl().SetNextInstallationTaskResult(
-                kFooWebAppUrl, webapps::InstallResultCode::kSuccessNewInstall);
           }));
   run_loop.Run();
 
-  EXPECT_EQ(2u, install_run_count());
+  EXPECT_EQ(1u, install_run_count());
   EXPECT_EQ(GetInstallOptions(kFooWebAppUrl), last_install_options());
 }
 

chrome/browser/web_applications/externally_managed_app_manager_unittest.cc

@@ -647,6 +647,57 @@ TEST_F(ExternallyAppManagerTest, InstallUrlChanges) {
                                              /*additional_policy_ids=*/{}))));
 }
 
+TEST_F(ExternallyAppManagerTest, PolicyAppOverridesUserInstalledApp) {
+  const GURL kStartUrl = GURL("https://www.example.com/index.html");
+  const GURL kInstallUrl =
+      GURL("https://www.example.com/nested/install_url.html");
+  const GURL kManifestUrl = GURL("https://www.example.com/manifest.json");
+
+  AppId app_id = PopulateBasicInstallPageWithManifest(kInstallUrl, kManifestUrl,
+                                                      kStartUrl);
+
+  {
+    // Install user app
+    auto& install_page_state =
+        web_contents_manager().GetOrCreatePageState(kInstallUrl);
+    install_page_state.opt_manifest->short_name = u"Test user app";
+
+    auto install_info = std::make_unique<WebAppInstallInfo>();
+    install_info->start_url = kStartUrl;
+    install_info->title = u"Test user app";
+    absl::optional<AppId> user_app_id =
+        test::InstallWebApp(profile(), std::move(install_info));
+
+    ASSERT_TRUE(user_app_id.has_value());
+    ASSERT_EQ(user_app_id.value(), app_id);
+    ASSERT_TRUE(app_registrar().WasInstalledByUser(app_id));
+    ASSERT_FALSE(app_registrar().HasExternalApp(app_id));
+    ASSERT_EQ("Test user app", app_registrar().GetAppShortName(app_id));
+  }
+  {
+    // Install policy app
+    auto& install_page_state =
+        web_contents_manager().GetOrCreatePageState(kInstallUrl);
+    install_page_state.opt_manifest->short_name = u"Test policy app";
+
+    SynchronizeFuture result;
+    provider().externally_managed_app_manager().SynchronizeInstalledApps(
+        CreateExternalInstallOptionsFromTemplate(
+            {kInstallUrl}, ExternalInstallSource::kExternalPolicy),
+        ExternalInstallSource::kExternalPolicy, result.GetCallback());
+    ASSERT_TRUE(result.Wait());
+    std::map<GURL, ExternallyManagedAppManager::InstallResult> install_results =
+        result.Get<InstallResults>();
+    EXPECT_THAT(
+        install_results,
+        ElementsAre(std::make_pair(
+            kInstallUrl,
+            ExternallyManagedAppManager::InstallResult(
+                webapps::InstallResultCode::kSuccessNewInstall, app_id))));
+    ASSERT_EQ("Test policy app", app_registrar().GetAppShortName(app_id));
+  }
+}
+
 TEST_F(ExternallyAppManagerTest, NoNetworkWithPlaceholder) {
   const GURL kInstallUrl = GURL("https://www.example.com/install_url.html");
   ExternalInstallOptions template_options(