Register granting of previously withheld permissions as a permissions

chrome/browser/extensions/crx_installer_browsertest.cc

Index: chrome/browser/extensions/crx_installer_browsertest.cc
diff --git a/chrome/browser/extensions/crx_installer_browsertest.cc b/chrome/browser/extensions/crx_installer_browsertest.cc
index 3bef3d6ada0a9620c23d489d7a38f6b01a43becc..9f1b8fd6a092124a32bb28fc7e652f9072ad0c62 100644
--- a/chrome/browser/extensions/crx_installer_browsertest.cc
+++ b/chrome/browser/extensions/crx_installer_browsertest.cc
@@ -13,6 +13,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/fake_safe_browsing_database_manager.h"
+#include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -25,6 +26,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/download_test_observer.h"
 #include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/management_policy.h"
 #include "extensions/browser/notification_types.h"
@@ -236,15 +238,15 @@ class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
 #if !defined(OS_CHROMEOS)
 IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, Whitelisting) {
   std::string id = "hdgllgikmikobbofgnabhfimcfoopgnd";
-  ExtensionService* service = extensions::ExtensionSystem::Get(
-      browser()->profile())->extension_service();
+  ExtensionRegistry* registry = ExtensionRegistry::Get(
+      browser()->profile());
 
   // Even whitelisted extensions with NPAPI should not prompt.
   scoped_refptr<MockPromptProxy> mock_prompt =
       CreateMockPromptProxyForBrowser(browser());
   InstallWithPrompt("uitest/plugins", id, mock_prompt);
   EXPECT_FALSE(mock_prompt->confirmation_requested());
-  EXPECT_TRUE(service->GetExtensionById(id, false));
+  EXPECT_TRUE(registry->enabled_extensions().GetByID(id));
 }
 #endif
 
@@ -371,16 +373,15 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, HiDpiThemeTest) {
   ASSERT_TRUE(InstallExtension(crx_path, 1));
 
   const std::string extension_id("gllekhaobjnhgeagipipnkpmmmpchacm");
-  ExtensionService* service = extensions::ExtensionSystem::Get(
-      browser()->profile())->extension_service();
-  ASSERT_TRUE(service);
+  ExtensionRegistry* registry = ExtensionRegistry::Get(
+      browser()->profile());
   const extensions::Extension* extension =
-     service->GetExtensionById(extension_id, false);
+     registry->enabled_extensions().GetByID(extension_id);
   ASSERT_TRUE(extension);
   EXPECT_EQ(extension_id, extension->id());
 
   UninstallExtension(extension_id);
-  EXPECT_FALSE(service->GetExtensionById(extension_id, false));
+  EXPECT_FALSE(registry->enabled_extensions().GetByID(extension_id));
 }
 
 // See http://crbug.com/315299.
@@ -398,12 +399,15 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
       browser()->profile());
   ExtensionService* service = extension_system->extension_service();
   ASSERT_TRUE(service);
+  ExtensionRegistry* registry = ExtensionRegistry::Get(
+      browser()->profile());
+  ASSERT_TRUE(registry);
 
   // Install version 1 of the test extension. This extension does not have
   // a background page but does have a browser action.
   ASSERT_TRUE(InstallExtension(crx_path.AppendASCII("v1.crx"), 1));
   const extensions::Extension* extension =
-     service->GetExtensionById(extension_id, false);
+     registry->enabled_extensions().GetByID(extension_id);
   ASSERT_TRUE(extension);
   ASSERT_EQ(extension_id, extension->id());
   ASSERT_EQ("1.0", extension->version()->GetString());
@@ -426,7 +430,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
       extension_id, crx_path.AppendASCII("v2.crx"), 0));
 
   ASSERT_EQ(1u, service->delayed_installs()->size());
-  extension = service->GetExtensionById(extension_id, false);
+  extension = registry->enabled_extensions().GetByID(extension_id);
   ASSERT_EQ("1.0", extension->version()->GetString());
 
   // Make the extension idle again by closing the popup. This should not trigger
@@ -442,14 +446,14 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
   // this install should succeed.
   ASSERT_TRUE(UpdateExtensionWaitForIdle(
       extension_id, crx_path.AppendASCII("v3.crx"), 0));
-  extension = service->GetExtensionById(extension_id, false);
+  extension = registry->enabled_extensions().GetByID(extension_id);
   ASSERT_EQ("3.0", extension->version()->GetString());
 
   // The version 2 delayed install should be cleaned up, and finishing
   // delayed extension installation shouldn't break anything.
   ASSERT_EQ(0u, service->delayed_installs()->size());
   service->MaybeFinishDelayedInstallations();
-  extension = service->GetExtensionById(extension_id, false);
+  extension = registry->enabled_extensions().GetByID(extension_id);
   ASSERT_EQ("3.0", extension->version()->GetString());
 }
 
@@ -520,9 +524,9 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, InstallToSharedLocation) {
 
   std::string extension_id = extension->id();
   UninstallExtension(extension_id);
-  ExtensionService* service = extensions::ExtensionSystem::Get(
-      browser()->profile())->extension_service();
-  EXPECT_FALSE(service->GetExtensionById(extension_id, false));
+  ExtensionRegistry* registry = ExtensionRegistry::Get(
+      browser()->profile());
+  EXPECT_FALSE(registry->enabled_extensions().GetByID(extension_id));
 
   // In the worst case you need to repeat this up to 3 times to make sure that
   // all pending tasks we sent from UI thread to task runner and back to UI.
@@ -564,4 +568,39 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, ManagementPolicy) {
   EXPECT_FALSE(InstallExtension(crx_path, 0));
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, WithheldElevationCheck) {
+  // Enable consent flag and install extension. The <all_hosts> permission will
+  // be withheld.
+  scoped_ptr<FeatureSwitch::ScopedOverride> enable_scripts_switch(
+      new FeatureSwitch::ScopedOverride(
+          FeatureSwitch::scripts_require_action(), true));
+
+  const char kManifest[] =
+      "{"
+      "  \"name\": \"Withheld test\","
+      "  \"version\": \"1.0\","
+      "  \"permissions\": ["
+      "    \"http://*/*\""
+      "  ],"
+      "  \"manifest_version\": 2"
+      "}";
+  TestExtensionDir dir;
+  dir.WriteManifest(kManifest);
+  base::FilePath crx_path = dir.Pack();
+  EXPECT_FALSE(crx_path.empty());
+  const extensions::Extension* extension = InstallExtension(crx_path, 1);

[Devlin, 2014/09/10 21:52:02]

nit: no prefix.

[gpdavis, 2014/09/11 00:51:01]

Done (will arrive in next patch set).

+  EXPECT_TRUE(base::PathExists(extension->path()));
+
+  std::string extension_id = extension->id();
+  ExtensionRegistry* registry = ExtensionRegistry::Get(
+      browser()->profile());
+  EXPECT_TRUE(registry->enabled_extensions().GetByID(extension_id));
+
+  // Disable consent flag and reinstall extension. It should now be disabled
+  // because previously withheld permissions are now being requested.
+  enable_scripts_switch.reset();
+  extension = InstallExtension(crx_path, -1);

[Devlin, 2014/09/10 21:52:02]

This part still isn't quite right.  Ideally, what we'd test is restarting the
browser with the switch off - but those types of tests tend to be a bit flaky. 
Instead, let's go ahead and use ExtensionService::ReloadExtensionsForTest() -
that's a pretty close approximation.

Also, we shouldn't just check that it's not in enabled extensions - that could
mean that it doesn't exist in the system at all (which is wrong).  We should
check a few things here, to be extra thorough:
- The extension should not be in enabled_extensions() (the check you have)
- The extension *should* be in disabled_extensions()
- The extension should have a disable reason of
Extension::DISABLE_PERMISSIONS_INCREASE.

[gpdavis, 2014/09/11 00:51:01]

ReloadExtensionsForTest is problematic in the browser test environment.  It ends
up calling ExtensionUpdater::Start(), which has already been called when
ExtensionSystem initializes for the browser test, and ExtensionUpdater::Stop()
never gets called inbetween.  So we end up in a situation where Start is called
twice, and that fails a DCHECK.  There seems to be no simple way to get around
this, unfortunately.  I had tried this before, but didn't dig as deeply into it
since I found I could use InstallExtension instead.

Using InstallExtension as is in this patch set, it passes all three of the
checks you just outlined.  Is there any better way to simulate a browser restart
that doesn't use ReloadExtensionsForTest?  Or is there something I'm missing
about the lifetimes of these objects in the browser test environment?

+  EXPECT_FALSE(registry->enabled_extensions().GetByID(extension_id));
+}
+
 }  // namespace extensions