[Extensions] Remove the prompt to re-enable app when visiting its site

We currently prompt the user to re-enable a disabled hosted app if they
visit the corresponding site. However, this can be really annoying, and
confusing, since we don't offer much context on the choice, and don't
persist the preference.

This is offering relatively little value, and the site (if it wanted)
could detect and inform the user that the app wasn't enabled, and give
them sufficiently more context.

However, we should keep this when the user visits a chrome-extension
url - this is a much stronger signal, and we can't display anything else
anyway (whereas visiting a hosted app's site still shows the site).

This was never tested, so add some tests.

BUG=678631

Review-Url: https://codereview.chromium.org/2621953003
Cr-Commit-Position: refs/heads/master@{#443439}

chrome/browser/extensions/navigation_observer.cc

@@ -20,6 +20,11 @@ using content::NavigationEntry;
 
 namespace extensions {
 
+namespace {
+// Whether to repeatedly prompt for the same extension id.
+bool g_repeat_prompting = false;
+}
+
 NavigationObserver::NavigationObserver(Profile* profile)
     : profile_(profile),
       extension_registry_observer_(this),
@@ -44,6 +49,11 @@ void NavigationObserver::Observe(int type,
   PromptToEnableExtensionIfNecessary(controller);
 }
 
+// static
+void NavigationObserver::SetAllowedRepeatedPromptingForTesting(bool allowed) {
+  g_repeat_prompting = allowed;
+}
+
 void NavigationObserver::RegisterForNotifications() {
   registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                  content::NotificationService::AllSources());
@@ -59,19 +69,33 @@ void NavigationObserver::PromptToEnableExtensionIfNecessary(
   if (!nav_entry)
     return;
 
-  ExtensionRegistry* registry = extensions::ExtensionRegistry::Get(profile_);
-  const Extension* extension =
-      registry->disabled_extensions().GetExtensionOrAppByURL(
-          nav_entry->GetURL());
+  const GURL& url = nav_entry->GetURL();
+  // NOTE: We only consider chrome-extension:// urls, and deliberately don't
+  // consider hosted app urls. This is because it's really annoying to visit the
+  // site associated with a hosted app (like calendar.google.com or
+  // drive.google.com) and have it repeatedly prompt you to re-enable an item.
+  // Visiting a chrome-extension:// url is a much stronger signal, and, without
+  // the item enabled, we won't show anything.
+  // TODO(devlin): While true, I still wonder how useful this is. We should get
+  // metrics.
+  if (!url.SchemeIs(kExtensionScheme))
+    return;
+
+  const Extension* extension = ExtensionRegistry::Get(profile_)
+                                   ->disabled_extensions()
+                                   .GetExtensionOrAppByURL(url);
   if (!extension)
     return;
 
   // Try not to repeatedly prompt the user about the same extension.
-  if (prompted_extensions_.find(extension->id()) != prompted_extensions_.end())
+  if (!prompted_extensions_.insert(extension->id()).second &&
+      !g_repeat_prompting) {
     return;
-  prompted_extensions_.insert(extension->id());
+  }
 
   ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
+  // TODO(devlin): Why do we only consider extensions that escalate permissions?
+  // Maybe because it's the only one we have a good prompt for?
   if (extension_prefs->DidExtensionEscalatePermissions(extension->id())) {
     // Keep track of the extension id and nav controller we're prompting for.
     // These must be reset in OnInstallPromptDone.
@@ -113,6 +137,10 @@ void NavigationObserver::OnInstallPromptDone(
     extension_service->GrantPermissionsAndEnableExtension(extension);
     nav_controller->Reload(content::ReloadType::NORMAL, true);
   } else {
+    // TODO(devlin): These metrics aren't very useful, since they're lumped in
+    // with the same for re-enabling/canceling when the extension first gets
+    // disabled, which is likely significantly more common (though impossible to
+    // tell). We need to separate these.
     std::string histogram_name =
        result == ExtensionInstallPrompt::Result::USER_CANCELED
             ? "ReEnableCancel"

chrome/browser/extensions/navigation_observer.h

@@ -40,6 +40,10 @@ class NavigationObserver : public content::NotificationObserver,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
+  // Allows showing the prompt for the same extensions multiple times in a
+  // session.
+  static void SetAllowedRepeatedPromptingForTesting(bool allowed);
+
  private:
   // Registers for the NOTIFICATION_NAV_ENTRY_COMMITTED notification.
   void RegisterForNotifications();

chrome/browser/extensions/navigation_observer_browsertest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/callback_helpers.h"
+#include "base/run_loop.h"
+#include "chrome/browser/extensions/chrome_test_extension_loader.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/navigation_observer.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "extensions/browser/extension_dialog_auto_confirm.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension.h"
+
+namespace extensions {
+
+// Test that visiting an url associated with a disabled extension offers to
+// re-enable it.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest,
+                       PromptToReEnableExtensionsOnNavigation) {
+  NavigationObserver::SetAllowedRepeatedPromptingForTesting(true);
+  base::ScopedClosureRunner reset_repeated_prompting(base::Bind([]() {
+    NavigationObserver::SetAllowedRepeatedPromptingForTesting(false);
+  }));
+  scoped_refptr<const Extension> extension =
+      ChromeTestExtensionLoader(profile()).LoadExtension(
+          test_data_dir_.AppendASCII("simple_with_file"));
+  ASSERT_TRUE(extension);
+  const std::string kExtensionId = extension->id();
+  const GURL kUrl = extension->GetResourceURL("file.html");
+
+  // We always navigate in a new tab because when we disable the extension, it
+  // closes all tabs for that extension. If we only opened in the current tab,
+  // this would result in the only open tab being closed, and the test quitting.
+  auto navigate_to_url_in_new_tab = [this](const GURL& url) {
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  };
+
+  ExtensionRegistry* registry = ExtensionRegistry::Get(profile());
+  EXPECT_TRUE(registry->enabled_extensions().Contains(kExtensionId));
+
+  // Disable the extension due to a permissions increase.
+  extension_service()->DisableExtension(
+      kExtensionId, Extension::DISABLE_PERMISSIONS_INCREASE);
+  EXPECT_TRUE(registry->disabled_extensions().Contains(kExtensionId));
+
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+  EXPECT_EQ(Extension::DISABLE_PERMISSIONS_INCREASE,
+            prefs->GetDisableReasons(kExtensionId));
+
+  {
+    // Visit an associated url and deny the prompt. The extension should remain
+    // disabled.
+    ScopedTestDialogAutoConfirm auto_deny(ScopedTestDialogAutoConfirm::CANCEL);
+    navigate_to_url_in_new_tab(kUrl);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(registry->disabled_extensions().Contains(kExtensionId));
+    EXPECT_EQ(Extension::DISABLE_PERMISSIONS_INCREASE,
+              prefs->GetDisableReasons(kExtensionId));
+  }
+
+  {
+    // Visit an associated url and accept the prompt. The extension should get
+    // re-enabled.
+    ScopedTestDialogAutoConfirm auto_accept(
+        ScopedTestDialogAutoConfirm::ACCEPT);
+    navigate_to_url_in_new_tab(kUrl);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(registry->enabled_extensions().Contains(kExtensionId));
+    EXPECT_EQ(Extension::DISABLE_NONE, prefs->GetDisableReasons(kExtensionId));
+  }
+
+  // Disable the extension for something other than a permissions increase.
+  extension_service()->DisableExtension(kExtensionId,
+                                        Extension::DISABLE_USER_ACTION);
+  EXPECT_TRUE(registry->disabled_extensions().Contains(kExtensionId));
+  EXPECT_EQ(Extension::DISABLE_USER_ACTION,
+            prefs->GetDisableReasons(kExtensionId));
+
+  {
+    // We only prompt for permissions increases, not any other disable reason.
+    // As such, the extension should stay disabled.
+    ScopedTestDialogAutoConfirm auto_accept(
+        ScopedTestDialogAutoConfirm::ACCEPT);
+    navigate_to_url_in_new_tab(kUrl);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(registry->disabled_extensions().Contains(kExtensionId));
+    EXPECT_EQ(Extension::DISABLE_USER_ACTION,
+              prefs->GetDisableReasons(kExtensionId));
+  }
+
+  // Load a hosted app and disable it for a permissions increase.
+  scoped_refptr<const Extension> hosted_app =
+      ChromeTestExtensionLoader(profile()).LoadExtension(
+          test_data_dir_.AppendASCII("hosted_app"));
+  ASSERT_TRUE(hosted_app);
+  const std::string kHostedAppId = hosted_app->id();
+  const GURL kHostedAppUrl("http://localhost/extensions/hosted_app/main.html");
+  EXPECT_EQ(hosted_app, registry->enabled_extensions().GetExtensionOrAppByURL(
+                            kHostedAppUrl));
+
+  extension_service()->DisableExtension(
+      kHostedAppId, Extension::DISABLE_PERMISSIONS_INCREASE);
+  EXPECT_TRUE(registry->disabled_extensions().Contains(kHostedAppId));
+  EXPECT_EQ(Extension::DISABLE_PERMISSIONS_INCREASE,
+            prefs->GetDisableReasons(kHostedAppId));
+
+  {
+    // When visiting a site that's associated with a hosted app, but not a
+    // chrome-extension url, we don't prompt to re-enable. This is to avoid
+    // prompting when visiting a regular website like calendar.google.com.
+    // See crbug.com/678631.
+    ScopedTestDialogAutoConfirm auto_accept(
+        ScopedTestDialogAutoConfirm::ACCEPT);
+    navigate_to_url_in_new_tab(kHostedAppUrl);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(registry->disabled_extensions().Contains(kHostedAppId));
+    EXPECT_EQ(Extension::DISABLE_PERMISSIONS_INCREASE,
+              prefs->GetDisableReasons(kHostedAppId));
+  }
+}
+
+}  // namespace extensions

chrome/test/BUILD.gn

@@ -1520,6 +1520,7 @@ test("browser_tests") {
       "../browser/extensions/lazy_background_page_test_util.h",
       "../browser/extensions/mutation_observers_apitest.cc",
       "../browser/extensions/native_bindings_apitest.cc",
+      "../browser/extensions/navigation_observer_browsertest.cc",
       "../browser/extensions/options_page_apitest.cc",
       "../browser/extensions/page_action_browsertest.cc",
       "../browser/extensions/process_management_browsertest.cc",