DisableExtensions now uninstalls them

This is to remain consistent in how the Chrome Cleanup Tool removes other force installed extensions. They interact badly with administrator permissions and so removing them entirely is cleaner than simply disabling like we do with normally installed extensions when the cleanup tool detects UwS.

Change-Id: I1b6b814d521e78edccd7c603f1eae3ef7a9a6c5f
Reviewed-on: https://chromium-review.googlesource.com/c/1278852Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Preston Carpenter <timidger@google.com>
Cr-Commit-Position: refs/heads/master@{#600059}

chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc

@@ -5,16 +5,18 @@
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h"
 
 #include <algorithm>
+#include <string>
 #include <utility>
 
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "components/crx_file/id_util.h"
 #include "content/public/browser/browser_thread.h"
-#include "extensions/browser/disable_reason.h"
+#include "extensions/browser/uninstall_reason.h"
 
 namespace safe_browsing {
 
@@ -59,6 +61,9 @@ void ChromePromptImpl::PromptUser(
         extension_ids
             ? ExtensionCollection(extension_ids->begin(), extension_ids->end())
             : ExtensionCollection());
+    if (extension_ids.has_value()) {
+      extension_ids_ = extension_ids;
+    }
     std::move(on_prompt_user_)
         .Run(std::move(scanner_results), std::move(callback));
   }
@@ -67,25 +72,40 @@ void ChromePromptImpl::PromptUser(
 void ChromePromptImpl::DisableExtensions(
     const std::vector<base::string16>& extension_ids,
     ChromePrompt::DisableExtensionsCallback callback) {
-  if (extension_service_ == nullptr) {
+  if (extension_service_ == nullptr || !extension_ids_.has_value()) {
     std::move(callback).Run(false);
     return;
   }
+  // Clear the stored extension_ids by moving it onto this stack frame,
+  // so subsequent calls will fail.
+  base::Optional<std::vector<base::string16>> optional_verified_extension_ids{};
+  extension_ids_.swap(optional_verified_extension_ids);
+  std::vector<base::string16> verified_extension_ids =
+      optional_verified_extension_ids.value();
   bool ids_are_valid = std::all_of(
-      extension_ids.begin(), extension_ids.end(), [](const base::string16& id) {
-        return crx_file::id_util::IdIsValid(base::UTF16ToUTF8(id));
+      extension_ids.begin(), extension_ids.end(),
+      [this, &verified_extension_ids](const base::string16& id) {
+        std::string id_utf8 = base::UTF16ToUTF8(id);
+        return crx_file::id_util::IdIsValid(id_utf8) &&
+               base::ContainsValue(verified_extension_ids, id) &&
+               extension_service_->GetInstalledExtension(id_utf8) != nullptr;
       });
   if (!ids_are_valid) {
     std::move(callback).Run(false);
     return;
   }
 
-  int reason = extensions::disable_reason::DISABLE_EXTERNAL_EXTENSION;
+  // This only uninstalls extensions that have been displayed to the user on
+  // the cleanup page.
+  extensions::UninstallReason reason =
+      extensions::UNINSTALL_REASON_USER_INITIATED;
+  bool result = true;
   for (const base::string16& extension_id : extension_ids) {
-    extension_service_->DisableExtension(base::UTF16ToUTF8(extension_id),
-                                         reason);
+    result = extension_service_->UninstallExtension(
+                 base::UTF16ToUTF8(extension_id), reason, nullptr) &&
+             result;
   }
-  std::move(callback).Run(true);
+  std::move(callback).Run(result);
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h

@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results.h"
 #include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
@@ -49,6 +50,7 @@ class ChromePromptImpl : public chrome_cleaner::mojom::ChromePrompt {
   mojo::Binding<chrome_cleaner::mojom::ChromePrompt> binding_;
   extensions::ExtensionService* extension_service_;
   OnPromptUser on_prompt_user_;
+  base::Optional<std::vector<base::string16>> extension_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromePromptImpl);
 };

chrome/browser/safe_browsing/chrome_cleaner/srt_delete_extension_win_unittest.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "chrome/browser/extensions/test_extension_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/mock_extension_system.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
@@ -47,38 +48,96 @@ TEST_F(ExtensionDeletionTest, DisableExtensionTest) {
   std::unique_ptr<ChromePromptImpl> chrome_prompt =
       std::make_unique<ChromePromptImpl>(extension_service, nullptr,
                                          base::DoNothing(), base::DoNothing());
+  chrome_prompt->PromptUser({}, {}, extension_ids, base::DoNothing());
   std::vector<base::string16> extensions_to_disable{extension_ids[0]};
   chrome_prompt->DisableExtensions(
       extensions_to_disable,
       base::BindOnce([](bool result) { EXPECT_TRUE(result); }));
-  EXPECT_FALSE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[0])));
-  EXPECT_TRUE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[1])));
-  EXPECT_TRUE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[2])));
+  EXPECT_EQ(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[0])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[1])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[2])),
+            nullptr);
 
-  extensions_to_disable.push_back(extension_ids[2]);
+  chrome_prompt = std::make_unique<ChromePromptImpl>(
+      extension_service, nullptr, base::DoNothing(), base::DoNothing());
+  chrome_prompt->PromptUser({}, {}, extension_ids, base::DoNothing());
+  extensions_to_disable = {extension_ids[2], extension_ids[1]};
   chrome_prompt->DisableExtensions(
       extensions_to_disable,
       base::BindOnce([](bool result) { EXPECT_TRUE(result); }));
-  EXPECT_FALSE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[0])));
-  EXPECT_TRUE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[1])));
-  EXPECT_FALSE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[2])));
+  EXPECT_EQ(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[0])),
+            nullptr);
+  EXPECT_EQ(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[1])),
+            nullptr);
+  EXPECT_EQ(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[2])),
+            nullptr);
+}
 
-  extensions_to_disable.push_back(extension_ids[1]);
+TEST_F(ExtensionDeletionTest, CantDeleteNonPromptedExtensions) {
+  std::vector<base::string16> extension_ids{};
+  extensions::ExtensionService* extension_service = this->service();
+  for (int i = 40; i < 43; i++) {
+    scoped_refptr<const extensions::Extension> extension =
+        extensions::ExtensionBuilder(base::NumberToString(i))
+            .SetManifestKey("version", "1")
+            .Build();
+    auto id = extension->id();
+    extension_ids.push_back(base::UTF8ToUTF16(id));
+    extension_service->AddExtension(extension.get());
+    extension_service->EnableExtension(id);
+  }
+  std::unique_ptr<ChromePromptImpl> chrome_prompt =
+      std::make_unique<ChromePromptImpl>(extension_service, nullptr,
+                                         base::DoNothing(), base::DoNothing());
+  std::vector<base::string16> extensions_to_disable{extension_ids[0]};
   chrome_prompt->DisableExtensions(
       extensions_to_disable,
-      base::BindOnce([](bool result) { EXPECT_TRUE(result); }));
-  EXPECT_FALSE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[0])));
-  EXPECT_FALSE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[1])));
-  EXPECT_FALSE(extension_service->IsExtensionEnabled(
-      base::UTF16ToUTF8(extension_ids[2])));
+      base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[0])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[1])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[2])),
+            nullptr);
+
+  chrome_prompt = std::make_unique<ChromePromptImpl>(
+      extension_service, nullptr, base::DoNothing(), base::DoNothing());
+  chrome_prompt->DisableExtensions(
+      extension_ids, base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[0])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[1])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[2])),
+            nullptr);
+
+  chrome_prompt->PromptUser({}, {}, {{extension_ids[2]}}, base::DoNothing());
+  chrome_prompt->DisableExtensions(
+      {extension_ids[0], extension_ids[1]},
+      base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[0])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[1])),
+            nullptr);
+  EXPECT_NE(extension_service->GetInstalledExtension(
+                base::UTF16ToUTF8(extension_ids[2])),
+            nullptr);
 }
 
 TEST_F(ExtensionDeletionTest, EmptyDeletionTest) {
@@ -87,6 +146,7 @@ TEST_F(ExtensionDeletionTest, EmptyDeletionTest) {
   std::unique_ptr<ChromePromptImpl> chrome_prompt =
       std::make_unique<ChromePromptImpl>(extension_service, nullptr,
                                          base::DoNothing(), base::DoNothing());
+  chrome_prompt->PromptUser({}, {}, extension_ids, base::DoNothing());
   for (int i = 40; i < 43; i++) {
     scoped_refptr<const extensions::Extension> extension =
         extensions::ExtensionBuilder(base::NumberToString(i))
@@ -149,7 +209,7 @@ TEST_F(ExtensionDeletionTest, NotInstalledExtensionTest) {
     extension_ids.push_back(base::UTF8ToUTF16(id));
   }
   chrome_prompt->DisableExtensions(
-      extension_ids, base::BindOnce([](bool result) { EXPECT_TRUE(result); }));
+      extension_ids, base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
 }
 
 }  // namespace safe_browsing

components/chrome_cleaner/public/interfaces/chrome_prompt.mojom

@@ -59,6 +59,7 @@ interface ChromePrompt {
              [MinVersion=2] array<ExtensionId>? extension_ids)
       => (PromptAcceptance prompt_acceptance);
 
+  // Actually uninstalls the extensions.
   // Params:
   //  - extension_ids: list of IDs of extensions that will be removed from
   //        Chrome. If there are any invalid IDs, none are removed and false is