Added a feature flag for ChromePromptImpl::DisableExtensions

Details:
Flag name: kChromeCleanupExtensionsFeature
When ON, extensions are included when prompting the user for UwS cleanup.
When OFF, extensions are NOT included when prompting the user for UwS cleanup.

Bug: 904958
Change-Id: Ie483ddb64de11de015429557343f0ee5c4676ff5
Reviewed-on: https://chromium-review.googlesource.com/c/1337790
Commit-Queue: Sebastien Lalancette <seblalancette@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609859}

chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc

@@ -14,6 +14,7 @@
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "components/crx_file/id_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/uninstall_reason.h"
@@ -53,35 +54,41 @@ void ChromePromptImpl::PromptUser(
   using ExtensionCollection = ChromeCleanerScannerResults::ExtensionCollection;
 
   if (on_prompt_user_) {
+    if (base::FeatureList::IsEnabled(kChromeCleanupExtensionsFeature) &&
+        extension_ids) {
+      extension_ids_ = extension_ids.value();
+    } else {
+      extension_ids_.clear();
+    }
+
     ChromeCleanerScannerResults scanner_results(
         FileCollection(files_to_delete.begin(), files_to_delete.end()),
         registry_keys ? RegistryKeyCollection(registry_keys->begin(),
                                               registry_keys->end())
                       : RegistryKeyCollection(),
-        extension_ids
-            ? ExtensionCollection(extension_ids->begin(), extension_ids->end())
-            : ExtensionCollection());
-    if (extension_ids.has_value()) {
-      extension_ids_ = extension_ids;
-    }
+        extension_ids_.empty() ? ExtensionCollection()
+                               : ExtensionCollection(extension_ids_.begin(),
+                                                     extension_ids_.end()));
+
     std::move(on_prompt_user_)
         .Run(std::move(scanner_results), std::move(callback));
   }
 }
 
+// The |extensions_ids| passed to this function are a subset of the
+// |extension_ids| passed to PromptUser because the extensions are not all
+// disabled at the same time.
 void ChromePromptImpl::DisableExtensions(
     const std::vector<base::string16>& extension_ids,
     ChromePrompt::DisableExtensionsCallback callback) {
-  if (extension_service_ == nullptr || !extension_ids_.has_value()) {
+  if (extension_service_ == nullptr || extension_ids_.empty()) {
     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();
+  std::vector<base::string16> verified_extension_ids{};
+  extension_ids_.swap(verified_extension_ids);
   bool ids_are_valid = std::all_of(
       extension_ids.begin(), extension_ids.end(),
       [this, &verified_extension_ids](const base::string16& id) {

chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h

@@ -50,7 +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_;
+  std::vector<base::string16> extension_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromePromptImpl);
 };

chrome/browser/safe_browsing/chrome_cleaner/srt_delete_extension_win_unittest.cc

@@ -4,13 +4,20 @@
 
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h"
 
+#include <vector>
+
+#include "base/feature_list.h"
 #include "base/location.h"
+#include "base/macros.h"
 #include "base/sequenced_task_runner.h"
+#include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "chrome/browser/extensions/test_extension_service.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/mock_extension_system.h"
@@ -23,16 +30,34 @@ namespace safe_browsing {
 
 class ExtensionDeletionTest : public extensions::ExtensionServiceTestBase {
  public:
-  ExtensionDeletionTest() { InitializeEmptyExtensionService(); }
   ~ExtensionDeletionTest() override = default;
 
-  void SetUp() override {}
+  void SetUp() override {
+    const std::vector<base::Feature> enabled_features = GetEnabledFeatures();
+    const std::vector<base::Feature> disabled_features = GetDisabledFeatures();
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(ExtensionDeletionTest);
-};
+    scoped_feature_list_.InitWithFeatures(enabled_features, disabled_features);
+
+    ExtensionServiceTestBase::SetUp();
+  }
 
-TEST_F(ExtensionDeletionTest, DisableExtensionTest) {
+ protected:
+  // Protected constructor to make this class abstract. Following
+  // implementations will be explicit about the feature flag state.
+  ExtensionDeletionTest() { InitializeEmptyExtensionService(); }
+
+  // Hooks to set up feature flags.
+  virtual const std::vector<base::Feature> GetEnabledFeatures() const {
+    return {};
+  }
+
+  virtual const std::vector<base::Feature> GetDisabledFeatures() const {
+    return {};
+  }
+
+  // Creates some extension IDs and registers them in the service.
+  std::vector<base::string16> PopulateExtensionIds(
+      bool installExtensions = true) {
     std::vector<base::string16> extension_ids{};
     extensions::ExtensionService* extension_service = this->service();
     for (int i = 40; i < 43; i++) {
@@ -42,12 +67,55 @@ TEST_F(ExtensionDeletionTest, DisableExtensionTest) {
               .Build();
       auto id = extension->id();
       extension_ids.push_back(base::UTF8ToUTF16(id));
+      if (installExtensions) {
         extension_service->AddExtension(extension.get());
         extension_service->EnableExtension(id);
       }
+    }
+
+    return extension_ids;
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+  DISALLOW_COPY_AND_ASSIGN(ExtensionDeletionTest);
+};
+
+class ExtensionDeletionEnabledTest : public ExtensionDeletionTest {
+ public:
+  ExtensionDeletionEnabledTest() = default;
+  ~ExtensionDeletionEnabledTest() override = default;
+
+ protected:
+  const std::vector<base::Feature> GetEnabledFeatures() const override {
+    return {kChromeCleanupExtensionsFeature};
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExtensionDeletionEnabledTest);
+};
+
+class ExtensionDeletionDisabledTest : public ExtensionDeletionTest {
+ public:
+  ExtensionDeletionDisabledTest() = default;
+  ~ExtensionDeletionDisabledTest() override = default;
+
+ protected:
+  const std::vector<base::Feature> GetDisabledFeatures() const override {
+    return {kChromeCleanupExtensionsFeature};
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExtensionDeletionDisabledTest);
+};
+
+TEST_F(ExtensionDeletionEnabledTest, DisableExtensionTest) {
+  std::vector<base::string16> extension_ids = PopulateExtensionIds();
+  extensions::ExtensionService* extension_service = this->service();
   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(
@@ -81,22 +149,13 @@ TEST_F(ExtensionDeletionTest, DisableExtensionTest) {
             nullptr);
 }
 
-TEST_F(ExtensionDeletionTest, CantDeleteNonPromptedExtensions) {
-  std::vector<base::string16> extension_ids{};
+TEST_F(ExtensionDeletionEnabledTest, CantDeleteNonPromptedExtensions) {
+  std::vector<base::string16> extension_ids = PopulateExtensionIds();
   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,
@@ -140,23 +199,14 @@ TEST_F(ExtensionDeletionTest, CantDeleteNonPromptedExtensions) {
             nullptr);
 }
 
-TEST_F(ExtensionDeletionTest, EmptyDeletionTest) {
-  std::vector<base::string16> extension_ids{};
+TEST_F(ExtensionDeletionEnabledTest, EmptyDeletionTest) {
+  std::vector<base::string16> extension_ids = PopulateExtensionIds();
   extensions::ExtensionService* extension_service = this->service();
   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))
-            .SetManifestKey("version", "1")
-            .Build();
-    auto id = extension->id();
-    extension_ids.push_back(base::UTF8ToUTF16(id));
-    extension_service->AddExtension(extension.get());
-    extension_service->EnableExtension(id);
-  }
   chrome_prompt->DisableExtensions(
       {}, base::BindOnce([](bool result) { EXPECT_TRUE(result); }));
   EXPECT_TRUE(extension_service->IsExtensionEnabled(
@@ -167,22 +217,13 @@ TEST_F(ExtensionDeletionTest, EmptyDeletionTest) {
       base::UTF16ToUTF8(extension_ids[2])));
 }
 
-TEST_F(ExtensionDeletionTest, BadlyFormattedDeletionTest) {
-  std::vector<base::string16> extension_ids{};
+TEST_F(ExtensionDeletionEnabledTest, BadlyFormattedDeletionTest) {
+  std::vector<base::string16> extension_ids = PopulateExtensionIds();
   extensions::ExtensionService* extension_service = this->service();
   std::unique_ptr<ChromePromptImpl> chrome_prompt =
       std::make_unique<ChromePromptImpl>(extension_service, nullptr,
                                          base::DoNothing(), base::DoNothing());
-  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);
-  }
+
   chrome_prompt->DisableExtensions(
       {L"bad-extension-id"},
       base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
@@ -193,23 +234,41 @@ TEST_F(ExtensionDeletionTest, BadlyFormattedDeletionTest) {
       base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
 }
 
-TEST_F(ExtensionDeletionTest, NotInstalledExtensionTest) {
-  std::vector<base::string16> extension_ids{};
+TEST_F(ExtensionDeletionEnabledTest, NotInstalledExtensionTest) {
+  // Don't actually install the extension
+  std::vector<base::string16> extension_ids = PopulateExtensionIds(false);
   extensions::ExtensionService* extension_service = this->service();
   std::unique_ptr<ChromePromptImpl> chrome_prompt =
       std::make_unique<ChromePromptImpl>(extension_service, nullptr,
                                          base::DoNothing(), base::DoNothing());
-  for (int i = 40; i < 43; i++) {
-    // Don't actually install the extension
-    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));
-  }
+
   chrome_prompt->DisableExtensions(
       extension_ids, base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
 }
 
+TEST_F(ExtensionDeletionDisabledTest, CannotDisableExtensionTest) {
+  std::vector<base::string16> extension_ids = PopulateExtensionIds();
+  extensions::ExtensionService* extension_service = this->service();
+
+  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_FALSE(result); }));
+
+  // Even if we called disable, the extension doesn't get disabled.
+  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);
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.cc

@@ -47,6 +47,9 @@ const base::Feature kChromeCleanupDistributionFeature{
 const base::Feature kChromeCleanupQuarantineFeature{
     "ChromeCleanupQuarantine", base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kChromeCleanupExtensionsFeature{
+    "ChromeCleanupExtensions", base::FEATURE_DISABLED_BY_DEFAULT};
+
 bool IsInSRTPromptFieldTrialGroups() {
   return !base::StartsWith(base::FieldTrialList::FindFullName(kSRTPromptTrial),
                            kSRTPromptOffGroup, base::CompareCase::SENSITIVE);

chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h

@@ -74,6 +74,10 @@ extern const base::Feature kChromeCleanupDistributionFeature;
 // files.
 extern const base::Feature kChromeCleanupQuarantineFeature;
 
+// Extensions cleanup feature. When enabled, Chrome Cleaner will prompt users
+// for, and cleanup, bad extensions.
+extern const base::Feature kChromeCleanupExtensionsFeature;
+
 extern const char kSRTPromptTrial[];
 
 // Returns true if this Chrome is in a field trial group which shows the SRT