policy: Uninstall apps that are no longer forced installed

Implements the WebAppPolicyManager side of uninstalling policy-installed
apps.

WebAppPolicyManager saves a set of the apps it last installed. Then when
a new policy arrives, it compares the saved set with the most recent
set and uninstalls any apps that are not in the latest policy.

We keep this set of last installed apps because if we were to retrieve
the list of installed apps from the ExtensionIdsMap, we could miss some
uninstalls i.e. uninstalling apps that are in the process of being
installed. For example, if we get a new policy with apps A and B. While
we install A, a new policy arrives only with app A. If we were to
retrieve the list of installed apps, we would only see A and would miss
uninstalling B.

Bug: 876174
Change-Id: I8805565ed8e38f2264a27a802b025bd8d4480cc2
Reviewed-on: https://chromium-review.googlesource.com/1195293
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587563}

chrome/browser/web_applications/bookmark_apps/BUILD.gn

@@ -41,11 +41,16 @@ source_set("unit_tests") {
     "//base",
     "//chrome/browser",
     "//chrome/browser/web_applications:web_app_group",
+    "//chrome/browser/web_applications:web_applications",
     "//chrome/browser/web_applications/components",
     "//chrome/browser/web_applications/components:test_support",
+    "//chrome/browser/web_applications/extensions",
     "//chrome/common:constants",
+    "//chrome/test:test_support",
+    "//components/crx_file:crx_file",
     "//components/sync_preferences:test_support",
     "//content/test:test_support",
+    "//extensions/common",
     "//skia",
     "//testing/gmock",
     "//testing/gtest",

chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.cc

@@ -4,14 +4,17 @@
 
 #include "chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.h"
 
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
+#include "base/stl_util.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_constants.h"
 #include "chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h"
+#include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
@@ -23,9 +26,11 @@
 
 namespace web_app {
 
-WebAppPolicyManager::WebAppPolicyManager(PrefService* pref_service,
+WebAppPolicyManager::WebAppPolicyManager(Profile* profile,
                                          PendingAppManager* pending_app_manager)
-    : pref_service_(pref_service), pending_app_manager_(pending_app_manager) {
+    : profile_(profile),
+      pref_service_(profile_->GetPrefs()),
+      pending_app_manager_(pending_app_manager) {
   content::BrowserThread::PostAfterStartupTask(
       FROM_HERE,
       content::BrowserThread::GetTaskRunnerForThread(
@@ -65,6 +70,14 @@ void WebAppPolicyManager::InitChangeRegistrarAndRefreshPolicyInstalledApps() {
       prefs::kWebAppInstallForceList,
       base::BindRepeating(&WebAppPolicyManager::RefreshPolicyInstalledApps,
                           weak_ptr_factory_.GetWeakPtr()));
+
+  // Populate `last_app_urls_` with the current policy-installed apps so that
+  // we can uninstall any apps that are no longer in the policy.
+  last_app_urls_ = ExtensionIdsMap::GetPolicyInstalledAppUrls(profile_);
+
+  // Sort so that we can later use base::STLSetDifference.
+  std::sort(last_app_urls_.begin(), last_app_urls_.end());
+
   RefreshPolicyInstalledApps();
 }
 
@@ -72,6 +85,7 @@ void WebAppPolicyManager::RefreshPolicyInstalledApps() {
   const base::Value* web_apps =
       pref_service_->GetList(prefs::kWebAppInstallForceList);
 
+  std::vector<GURL> app_urls;
   std::vector<PendingAppManager::AppInfo> apps_to_install;
   for (const base::Value& info : web_apps->GetList()) {
     const base::Value& url = *info.FindKey(kUrlKey);
@@ -92,9 +106,18 @@ void WebAppPolicyManager::RefreshPolicyInstalledApps() {
     // There is a separate policy to create shortcuts/pin apps to shelf.
     apps_to_install.push_back(PendingAppManager::AppInfo::Create(
         GURL(url.GetString()), container, false /* create_shortcuts */));
+    app_urls.emplace_back(url.GetString());
   }
+
   pending_app_manager_->InstallApps(std::move(apps_to_install),
                                     base::DoNothing());
+  std::sort(app_urls.begin(), app_urls.end());
+
+  auto apps_to_uninstall =
+      base::STLSetDifference<std::vector<GURL>>(last_app_urls_, app_urls);
+  pending_app_manager_->UninstallApps(std::move(apps_to_uninstall),
+                                      base::DoNothing());
+  last_app_urls_.swap(app_urls);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.h

@@ -30,7 +30,7 @@ class WebAppPolicyManager {
   // Constructs a WebAppPolicyManager instance that uses
   // |pending_app_manager| to manage apps. |pending_app_manager| should outlive
   // this class.
-  explicit WebAppPolicyManager(PrefService* pref_service,
+  explicit WebAppPolicyManager(Profile* profile,
                                PendingAppManager* pending_app_manager);
   ~WebAppPolicyManager();
 
@@ -43,6 +43,7 @@ class WebAppPolicyManager {
 
   void RefreshPolicyInstalledApps();
 
+  Profile* profile_;
   PrefService* pref_service_;
 
   // Used to install, uninstall, and update apps. Should outlive this class.
@@ -50,6 +51,11 @@ class WebAppPolicyManager {
 
   PrefChangeRegistrar pref_change_registrar_;
 
+  // URLs of the apps that this class last installed. Populated with the current
+  // policy-installed apps at start up and replaced every time this class calls
+  // PendingAppManager::InstallApps().
+  std::vector<GURL> last_app_urls_;
+
   base::WeakPtrFactory<WebAppPolicyManager> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(WebAppPolicyManager);

chrome/browser/web_applications/components/pending_app_manager.h

@@ -31,6 +31,8 @@ class PendingAppManager {
   using RepeatingInstallCallback =
       base::RepeatingCallback<void(const GURL& app_url,
                                    const base::Optional<std::string>&)>;
+  using UninstallCallback =
+      base::RepeatingCallback<void(const GURL& app_url, bool succeeded)>;
 
   // How the app will be launched after installation.
   enum class LaunchContainer {
@@ -94,6 +96,14 @@ class PendingAppManager {
   virtual void InstallApps(std::vector<AppInfo> apps_to_install,
                            const RepeatingInstallCallback& callback) = 0;
 
+  // Adds |apps_to_uninstall| to the queue of operations. Runs |callback|
+  // with the URL of the corresponding app in |apps_to_install| and with a
+  // bool indicating whether or not the uninstall succeeded. Runs |callback|
+  // for every completed uninstallation - whether or not the uninstallation
+  // actually succeeded.
+  virtual void UninstallApps(std::vector<GURL> apps_to_uninstall,
+                             const UninstallCallback& callback) = 0;
+
   DISALLOW_COPY_AND_ASSIGN(PendingAppManager);
 };
 

chrome/browser/web_applications/components/test_pending_app_manager.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/web_applications/components/test_pending_app_manager.h"
 
+#include <utility>
+
 #include "base/callback.h"
 #include "url/gurl.h"
 
@@ -33,4 +35,13 @@ void TestPendingAppManager::InstallApps(
     Install(std::move(app), callback);
 }
 
+void TestPendingAppManager::UninstallApps(std::vector<GURL> apps_to_uninstall,
+                                          const UninstallCallback& callback) {
+  for (auto& app : apps_to_uninstall) {
+    const GURL url(app);
+    uninstalled_apps_.push_back(std::move(app));
+    callback.Run(url, true /* succeeded */);
+  }
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/test_pending_app_manager.h

@@ -10,6 +10,7 @@
 
 #include "base/macros.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
+#include "url/gurl.h"
 
 namespace web_app {
 
@@ -18,14 +19,20 @@ class TestPendingAppManager : public PendingAppManager {
   TestPendingAppManager();
   ~TestPendingAppManager() override;
   const std::vector<AppInfo>& installed_apps() const { return installed_apps_; }
+  const std::vector<GURL>& uninstalled_apps() const {
+    return uninstalled_apps_;
+  }
 
   // PendingAppManager:
   void Install(AppInfo app_to_install, OnceInstallCallback callback) override;
   void InstallApps(std::vector<AppInfo> apps_to_install,
                    const RepeatingInstallCallback& callback) override;
+  void UninstallApps(std::vector<GURL> apps_to_uninstall,
+                     const UninstallCallback& callback) override;
 
  private:
   std::vector<AppInfo> installed_apps_;
+  std::vector<GURL> uninstalled_apps_;
 
   DISALLOW_COPY_AND_ASSIGN(TestPendingAppManager);
 };

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -104,6 +104,13 @@ void PendingBookmarkAppManager::SetTimerForTesting(
   timer_ = std::move(timer);
 }
 
+void PendingBookmarkAppManager::UninstallApps(
+    std::vector<GURL> apps_to_uninstall,
+    const UninstallCallback& callback) {
+  // TODO(crbug.com/876174) implement this method.
+  NOTIMPLEMENTED();
+}
+
 // Returns (as the base::Optional part) whether or not there is already a known
 // extension for the given ID. The bool inside the base::Optional is, when
 // known, whether the extension is installed (true) or uninstalled (false).

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h

@@ -50,6 +50,8 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
   void Install(AppInfo app_to_install, OnceInstallCallback callback) override;
   void InstallApps(std::vector<AppInfo> apps_to_install,
                    const RepeatingInstallCallback& callback) override;
+  void UninstallApps(std::vector<GURL> apps_to_uninstall,
+                     const UninstallCallback& callback) override;
 
   void SetFactoriesForTesting(WebContentsFactory web_contents_factory,
                               TaskFactory task_factory);

chrome/browser/web_applications/extensions/web_app_extension_ids_map.cc

@@ -4,12 +4,17 @@
 
 #include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
 
+#include <string>
+#include <vector>
+
 #include "base/values.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/browser/browser_thread.h"
+#include "extensions/browser/extension_registry.h"
 #include "url/gurl.h"
 
 namespace web_app {
@@ -41,6 +46,32 @@ bool ExtensionIdsMap::HasExtensionId(const PrefService* pref_service,
   return false;
 }
 
+// static
+std::vector<GURL> ExtensionIdsMap::GetPolicyInstalledAppUrls(Profile* profile) {
+  const base::DictionaryValue* urls_to_ids =
+      profile->GetPrefs()->GetDictionary(prefs::kWebAppsExtensionIDs);
+
+  std::vector<GURL> policy_installed_app_urls;
+
+  if (!urls_to_ids)
+    return policy_installed_app_urls;
+
+  for (const auto& url_to_id : urls_to_ids->DictItems()) {
+    const std::string& extension_id = url_to_id.second.GetString();
+    auto* extension =
+        extensions::ExtensionRegistry::Get(profile)->GetExtensionById(
+            extension_id, extensions::ExtensionRegistry::EVERYTHING);
+    if (!extension)
+      continue;
+    if (extension->location() != extensions::Manifest::EXTERNAL_POLICY)
+      continue;
+
+    policy_installed_app_urls.emplace_back(url_to_id.first);
+  }
+
+  return policy_installed_app_urls;
+}
+
 ExtensionIdsMap::ExtensionIdsMap(PrefService* pref_service)
     : pref_service_(pref_service) {}
 

chrome/browser/web_applications/extensions/web_app_extension_ids_map.h

@@ -6,12 +6,14 @@
 #define CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_WEB_APP_EXTENSION_IDS_MAP_H_
 
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/optional.h"
 
 class GURL;
 class PrefService;
+class Profile;
 
 namespace user_prefs {
 class PrefRegistrySyncable;
@@ -30,6 +32,9 @@ class ExtensionIdsMap {
   static bool HasExtensionId(const PrefService* pref_service,
                              const std::string& extension_id);
 
+  // Returns the URLs of the apps that were installed as policy-installed apps.
+  static std::vector<GURL> GetPolicyInstalledAppUrls(Profile* profile);
+
   explicit ExtensionIdsMap(PrefService* pref_service);
 
   void Insert(const GURL& url, const std::string& extension_id);

chrome/browser/web_applications/web_app_provider.cc

@@ -28,7 +28,7 @@ WebAppProvider::WebAppProvider(Profile* profile)
           std::make_unique<extensions::PendingBookmarkAppManager>(profile)) {
   if (WebAppPolicyManager::ShouldEnableForProfile(profile)) {
     web_app_policy_manager_ = std::make_unique<WebAppPolicyManager>(
-        profile->GetPrefs(), pending_app_manager_.get());
+        profile, pending_app_manager_.get());
   }
 
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,