Allow explicitly whitelisted apps/extensions in public sessions

This CL adds an extension management policy provider that allows
explicitly whitelisted apps/extensions to be installed in public sessions.
Right now, QuickOffice and all hosted apps are whitelisted.

BUG=296868
TEST=New browser and unit tests

Review URL: https://codereview.chromium.org/24261010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@226494 0039d316-1c4b-4281-b951-d872f2087c98

chrome/app/generated_resources.grd

@@ -4403,6 +4403,9 @@ Make sure you do not expose any sensitive information.
       <message name="IDS_EXTENSION_CANT_INSTALL_POLICY_BLOCKED" desc="Error message when a user tries to install an extension that is blocked by administrator policy.">
         <ph name="EXTENSION_NAME">$1<ex>Google Talk</ex></ph> (extension ID "<ph name="EXTENSION_ID">$2<ex>nckgahadagoaajjgafhacjanaoiihapd</ex></ph>") is blocked by the administrator.
       </message>
+      <message name="IDS_EXTENSION_CANT_INSTALL_IN_DEVICE_LOCAL_ACCOUNT" desc="Error message when a user tries to install or the administrator tries to force-install through policy an extension that is not allowed in a device-local account.">
+        <ph name="EXTENSION_NAME">$1<ex>Google Talk</ex></ph> (extension ID "<ph name="EXTENSION_ID">$2<ex>nckgahadagoaajjgafhacjanaoiihapd</ex></ph>") is not allowed in this type of session.
+      </message>
       <message name="IDS_EXTENSION_CANT_MODIFY_POLICY_REQUIRED" desc="Error message when a user tries to remove or change an extension that is required by administrator policy.">
         The administrator of this machine requires <ph name="EXTENSION_NAME">$1<ex>Google Talk</ex></ph> to be installed. It cannot be removed or modified.
       </message>

chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc

+// Copyright 2013 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 "chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.h"
+
+#include <string>
+
+#include "base/logging.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/common/extensions/extension.h"
+#include "extensions/common/manifest.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace chromeos {
+
+namespace {
+
+// Apps/extensions explicitly whitelisted for use in device-local accounts.
+const char* kDeviceLocalAccountWhitelist[] = {
+  "bpmcpldpdmajfigpchkicefoigmkfalc",  // QuickOffice
+};
+
+}  // namespace
+
+DeviceLocalAccountManagementPolicyProvider::
+    DeviceLocalAccountManagementPolicyProvider(
+        policy::DeviceLocalAccount::Type account_type)
+    : account_type_(account_type) {
+}
+
+DeviceLocalAccountManagementPolicyProvider::
+    ~DeviceLocalAccountManagementPolicyProvider() {
+}
+
+std::string DeviceLocalAccountManagementPolicyProvider::
+    GetDebugPolicyProviderName() const {
+#if defined(NDEBUG)
+  NOTREACHED();
+  return std::string();
+#else
+  return "whitelist for device-local accounts";
+#endif
+}
+
+bool DeviceLocalAccountManagementPolicyProvider::UserMayLoad(
+    const extensions::Extension* extension,
+    string16* error) const {
+  if (account_type_ == policy::DeviceLocalAccount::TYPE_KIOSK_APP) {
+    // For single-app kiosk sessions, allow only platform apps.
+    if (extension->GetType() == extensions::Manifest::TYPE_PLATFORM_APP)
+      return true;
+
+  } else {
+    // Allow extension if its type is whitelisted for use in device-local
+    // accounts.
+    if (extension->GetType() == extensions::Manifest::TYPE_HOSTED_APP)
+      return true;
+
+    // Allow extension if its specific ID is whitelisted for use in device-local
+    // accounts.
+    for (size_t i = 0; i < arraysize(kDeviceLocalAccountWhitelist); ++i) {
+      if (extension->id() == kDeviceLocalAccountWhitelist[i])
+        return true;
+    }
+  }
+
+  // Disallow all other extensions.
+  if (error) {
+    *error = l10n_util::GetStringFUTF16(
+          IDS_EXTENSION_CANT_INSTALL_IN_DEVICE_LOCAL_ACCOUNT,
+          UTF8ToUTF16(extension->name()),
+          UTF8ToUTF16(extension->id()));
+  }
+  return false;
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.h

+// Copyright 2013 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_EXTENSIONS_DEVICE_LOCAL_ACCOUNT_MANAGEMENT_POLICY_PROVIDER_H_
+#define CHROME_BROWSER_CHROMEOS_EXTENSIONS_DEVICE_LOCAL_ACCOUNT_MANAGEMENT_POLICY_PROVIDER_H_
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "chrome/browser/chromeos/policy/device_local_account.h"
+#include "chrome/browser/extensions/management_policy.h"
+
+namespace chromeos {
+
+// A managed policy for device-local accounts that ensures only extensions whose
+// type or ID has been whitelisted for use in device-local accounts can be
+// installed.
+class DeviceLocalAccountManagementPolicyProvider
+    : public extensions::ManagementPolicy::Provider {
+ public:
+  explicit DeviceLocalAccountManagementPolicyProvider(
+      policy::DeviceLocalAccount::Type account_type);
+  virtual ~DeviceLocalAccountManagementPolicyProvider();
+
+  // extensions::ManagementPolicy::Provider:
+  virtual std::string GetDebugPolicyProviderName() const OVERRIDE;
+  virtual bool UserMayLoad(const extensions::Extension* extension,
+                           string16* error) const OVERRIDE;
+
+ private:
+  const policy::DeviceLocalAccount::Type account_type_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeviceLocalAccountManagementPolicyProvider);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_EXTENSIONS_DEVICE_LOCAL_ACCOUNT_MANAGEMENT_POLICY_PROVIDER_H_

chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc

+// Copyright 2013 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 "chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.h"
+
+#include <string>
+
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/values.h"
+#include "chrome/common/extensions/extension.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/manifest_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace {
+
+const char kWhitelistedId[] = "bpmcpldpdmajfigpchkicefoigmkfalc";
+
+scoped_refptr<const extensions::Extension> CreateExtensionFromValues(
+    const std::string& id,
+    base::DictionaryValue* values) {
+  values->SetString(extensions::manifest_keys::kName, "test");
+  values->SetString(extensions::manifest_keys::kVersion, "0.1");
+  std::string error;
+  return extensions::Extension::Create(base::FilePath(),
+                                       extensions::Manifest::INTERNAL,
+                                       *values,
+                                       extensions::Extension::NO_FLAGS,
+                                       id,
+                                       &error);
+}
+
+scoped_refptr<const extensions::Extension> CreateExtension(
+    const std::string& id) {
+  base::DictionaryValue values;
+  return CreateExtensionFromValues(id, &values);
+}
+
+scoped_refptr<const extensions::Extension> CreateHostedApp() {
+  base::DictionaryValue values;
+  values.Set(extensions::manifest_keys::kApp, new base::DictionaryValue);
+  values.Set(extensions::manifest_keys::kWebURLs, new base::ListValue);
+  return CreateExtensionFromValues(std::string(), &values);
+}
+
+scoped_refptr<const extensions::Extension> CreatePlatformApp() {
+  base::DictionaryValue values;
+  values.Set(extensions::manifest_keys::kApp, new base::DictionaryValue);
+  values.Set(extensions::manifest_keys::kPlatformAppBackground,
+             new base::DictionaryValue);
+  values.Set(extensions::manifest_keys::kPlatformAppBackgroundPage,
+             new base::StringValue("background.html"));
+  return CreateExtensionFromValues(std::string(), &values);
+}
+
+}  // namespace
+
+TEST(DeviceLocalAccountManagementPolicyProviderTest, PublicSession) {
+  DeviceLocalAccountManagementPolicyProvider
+      provider(policy::DeviceLocalAccount::TYPE_PUBLIC_SESSION);
+
+  // Verify that if an extension's type has been whitelisted for use in
+  // device-local accounts, the extension can be installed.
+  scoped_refptr<const extensions::Extension> extension = CreateHostedApp();
+  ASSERT_TRUE(extension);
+  string16 error;
+  EXPECT_TRUE(provider.UserMayLoad(extension.get(), &error));
+  EXPECT_EQ(string16(), error);
+  error.clear();
+
+  // Verify that if an extension's ID has been explicitly whitelisted for use in
+  // device-local accounts, the extension can be installed.
+  extension = CreateExtension(kWhitelistedId);
+  ASSERT_TRUE(extension);
+  EXPECT_TRUE(provider.UserMayLoad(extension.get(), &error));
+  EXPECT_EQ(string16(), error);
+  error.clear();
+
+  // Verify that if neither the type nor the ID of an extension have been
+  // whitelisted for use in device-local accounts, the extension cannot be
+  // installed.
+  extension = CreateExtension(std::string());
+  ASSERT_TRUE(extension);
+  EXPECT_FALSE(provider.UserMayLoad(extension.get(), &error));
+  EXPECT_NE(string16(), error);
+  error.clear();
+}
+
+TEST(DeviceLocalAccountManagementPolicyProviderTest, KioskAppSession) {
+  DeviceLocalAccountManagementPolicyProvider
+      provider(policy::DeviceLocalAccount::TYPE_KIOSK_APP);
+
+  // Verify that a platform app can be installed.
+  scoped_refptr<const extensions::Extension> extension = CreatePlatformApp();
+  ASSERT_TRUE(extension);
+  string16 error;
+  EXPECT_TRUE(provider.UserMayLoad(extension.get(), &error));
+  EXPECT_EQ(string16(), error);
+  error.clear();
+
+  // Verify that an extension whose type has been whitelisted for use in other
+  // types of device-local accounts cannot be installed in a single-app kiosk
+  // session.
+  extension = CreateHostedApp();
+  ASSERT_TRUE(extension);
+  EXPECT_FALSE(provider.UserMayLoad(extension.get(), &error));
+  EXPECT_NE(string16(), error);
+  error.clear();
+
+  // Verify that an extension whose ID has been whitelisted for use in other
+  // types of device-local accounts cannot be installed in a single-app kiosk
+  // session.
+  extension = CreateExtension(kWhitelistedId);
+  ASSERT_TRUE(extension);
+  EXPECT_FALSE(provider.UserMayLoad(extension.get(), &error));
+  EXPECT_NE(string16(), error);
+  error.clear();
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/login/user_manager_impl.cc

@@ -362,11 +362,15 @@ void UserManagerImpl::UserLoggedIn(const std::string& email,
     return;
   }
 
+  policy::DeviceLocalAccount::Type device_local_account_type;
   if (email == UserManager::kGuestUserName) {
     GuestUserLoggedIn();
   } else if (email == UserManager::kRetailModeUserName) {
     RetailModeUserLoggedIn();
-  } else if (policy::IsKioskAppUser(email)) {
+  } else if (policy::IsDeviceLocalAccountUser(email,
+                                              &device_local_account_type) &&
+             device_local_account_type ==
+                 policy::DeviceLocalAccount::TYPE_KIOSK_APP) {
     KioskAppLoggedIn(email);
   } else {
     EnsureUsersLoaded();
@@ -1396,7 +1400,11 @@ void UserManagerImpl::PublicAccountUserLoggedIn(User* user) {
 
 void UserManagerImpl::KioskAppLoggedIn(const std::string& username) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(policy::IsKioskAppUser(username));
+  policy::DeviceLocalAccount::Type device_local_account_type;
+  DCHECK(policy::IsDeviceLocalAccountUser(username,
+                                          &device_local_account_type));
+  DCHECK_EQ(policy::DeviceLocalAccount::TYPE_KIOSK_APP,
+            device_local_account_type);
 
   active_user_ = User::CreateKioskAppUser(username);
   active_user_->SetStubImage(User::kInvalidImageIndex, false);

chrome/browser/chromeos/policy/device_local_account.cc

@@ -6,6 +6,7 @@
 
 #include <set>
 
+#include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_number_conversions.h"
@@ -56,16 +57,31 @@ std::string GenerateDeviceLocalAccountUserId(const std::string& account_id,
       domain_prefix + kDeviceLocalAccountDomainSuffix);
 }
 
-bool IsDeviceLocalAccountUser(const std::string& user_id) {
-  return EndsWith(gaia::ExtractDomainName(user_id),
-                  kDeviceLocalAccountDomainSuffix,
-                  true);
-}
+bool IsDeviceLocalAccountUser(const std::string& user_id,
+                              DeviceLocalAccount::Type* type) {
+  const std::string domain = gaia::ExtractDomainName(user_id);
+  if (!EndsWith(domain, kDeviceLocalAccountDomainSuffix, true))
+    return false;
+
+  const std::string domain_prefix = domain.substr(
+      0, domain.size() - arraysize(kDeviceLocalAccountDomainSuffix) + 1);
+
+  if (domain_prefix == kPublicAccountDomainPrefix) {
+    if (type)
+      *type = DeviceLocalAccount::TYPE_PUBLIC_SESSION;
+    return true;
+  }
+  if (domain_prefix == kKioskAppAccountDomainPrefix) {
+    if (type)
+      *type = DeviceLocalAccount::TYPE_KIOSK_APP;
+    return true;
+  }
 
-bool IsKioskAppUser(const std::string& user_id) {
-  return gaia::ExtractDomainName(user_id) ==
-      std::string(kKioskAppAccountDomainPrefix) +
-          kDeviceLocalAccountDomainSuffix;
+  // |user_id| is a device-local account but its type is not recognized.
+  NOTREACHED();
+  if (type)
+    *type = DeviceLocalAccount::TYPE_COUNT;
+  return true;
 }
 
 void SetDeviceLocalAccounts(

chrome/browser/chromeos/policy/device_local_account.h

@@ -40,9 +40,10 @@ struct DeviceLocalAccount {
 std::string GenerateDeviceLocalAccountUserId(const std::string& account_id,
                                              DeviceLocalAccount::Type type);
 
-bool IsDeviceLocalAccountUser(const std::string& user_id);
-
-bool IsKioskAppUser(const std::string& user_id);
+// Determines whether |user_id| belongs to a device-local account and if so,
+// returns the type of device-local account in |type| unless |type| is NULL.
+bool IsDeviceLocalAccountUser(const std::string& user_id,
+                              DeviceLocalAccount::Type* type);
 
 // Stores a list of device-local accounts in |cros_settings|. The accounts are
 // stored as a list of dictionaries with each dictionary containing the

chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc

@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
-#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/policy/device_local_account_policy_provider.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
@@ -80,13 +79,6 @@ class DeviceLocalAccountPolicyServiceTest
                              POLICY_SCOPE_USER,
                              Value::CreateBooleanValue(false),
                              NULL);
-    scoped_ptr<base::ListValue> allowed_extension_types(new base::ListValue());
-    allowed_extension_types->AppendString("hosted_app");
-    expected_policy_map_.Set(key::kExtensionAllowedTypes,
-                             POLICY_LEVEL_MANDATORY,
-                             POLICY_SCOPE_USER,
-                             allowed_extension_types.release(),
-                             NULL);
 
     // Explicitly set value.
     expected_policy_map_.Set(key::kDisableSpdy,
@@ -502,8 +494,6 @@ TEST_F(DeviceLocalAccountPolicyProviderTest, Policy) {
       set_value("Always");
   device_local_account_policy_.payload().mutable_showlogoutbuttonintray()->
       set_value(false);
-  device_local_account_policy_.payload().mutable_extensionallowedtypes()->
-      mutable_value()->mutable_entries()->Clear();
   device_local_account_policy_.Build();
   device_settings_test_helper_.set_device_local_account_policy_blob(
       PolicyBuilder::kFakeUsername, device_local_account_policy_.GetBlob());

chrome/browser/chromeos/policy/device_local_account_policy_store.cc

@@ -86,7 +86,6 @@ void DeviceLocalAccountPolicyStore::UpdatePolicy(
                   base::Value::CreateIntegerValue(
                       chromeos::PowerPolicyController::ACTION_STOP_SESSION),
                   NULL);
-
   // Force the |ShelfAutoHideBehavior| policy to |Never|, ensuring that the ash
   // shelf does not auto-hide.
   policy_map_.Set(key::kShelfAutoHideBehavior,
@@ -108,16 +107,6 @@ void DeviceLocalAccountPolicyStore::UpdatePolicy(
                   POLICY_SCOPE_USER,
                   Value::CreateBooleanValue(false),
                   NULL);
-  // Restrict device-local accounts to hosted apps for now (i.e. no extensions,
-  // packaged apps etc.) for security/privacy reasons (i.e. we'd like to
-  // prevent the admin from stealing private information from random people).
-  scoped_ptr<base::ListValue> allowed_extension_types(new base::ListValue());
-  allowed_extension_types->AppendString("hosted_app");
-  policy_map_.Set(key::kExtensionAllowedTypes,
-                  POLICY_LEVEL_MANDATORY,
-                  POLICY_SCOPE_USER,
-                  allowed_extension_types.release(),
-                  NULL);
 
   status_ = STATUS_OK;
   NotifyStoreLoaded();

chrome/browser/extensions/extension_system.cc

@@ -48,6 +48,10 @@
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/app_mode/app_mode_utils.h"
+#include "chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.h"
+#include "chrome/browser/chromeos/login/user.h"
+#include "chrome/browser/chromeos/login/user_manager.h"
+#include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/login/login_state.h"
 #endif
@@ -107,7 +111,19 @@ void ExtensionSystemImpl::Shared::InitPrefs() {
 
   standard_management_policy_provider_.reset(
       new StandardManagementPolicyProvider(ExtensionPrefs::Get(profile_)));
-#endif
+
+#if defined (OS_CHROMEOS)
+  const chromeos::User* user = chromeos::UserManager::Get()->GetActiveUser();
+  policy::DeviceLocalAccount::Type device_local_account_type;
+  if (user && policy::IsDeviceLocalAccountUser(user->email(),
+                                               &device_local_account_type)) {
+    device_local_account_management_policy_provider_.reset(
+        new chromeos::DeviceLocalAccountManagementPolicyProvider(
+            device_local_account_type));
+  }
+#endif  // defined (OS_CHROMEOS)
+
+#endif  // defined(ENABLE_EXTENSIONS)
 }
 
 void ExtensionSystemImpl::Shared::RegisterManagementPolicyProviders() {
@@ -116,7 +132,15 @@ void ExtensionSystemImpl::Shared::RegisterManagementPolicyProviders() {
   DCHECK(standard_management_policy_provider_.get());
   management_policy_->RegisterProvider(
       standard_management_policy_provider_.get());
-#endif
+
+#if defined (OS_CHROMEOS)
+  if (device_local_account_management_policy_provider_) {
+    management_policy_->RegisterProvider(
+        device_local_account_management_policy_provider_.get());
+  }
+#endif  // defined (OS_CHROMEOS)
+
+#endif  // defined(ENABLE_EXTENSIONS)
 }
 
 void ExtensionSystemImpl::Shared::Init(bool extensions_enabled) {

chrome/browser/extensions/extension_system.h

@@ -18,6 +18,12 @@ class ExtensionProcessManager;
 class ExtensionService;
 class Profile;
 
+#if defined(OS_CHROMEOS)
+namespace chromeos {
+class DeviceLocalAccountManagementPolicyProvider;
+}
+#endif  // defined(OS_CHROMEOS)
+
 namespace extensions {
 class Blacklist;
 class ErrorConsole;
@@ -213,6 +219,11 @@ class ExtensionSystemImpl : public ExtensionSystem {
     scoped_ptr<ExtensionWarningBadgeService> extension_warning_badge_service_;
     scoped_ptr<ErrorConsole> error_console_;
 
+#if defined(OS_CHROMEOS)
+    scoped_ptr<chromeos::DeviceLocalAccountManagementPolicyProvider>
+        device_local_account_management_policy_provider_;
+#endif
+
     OneShotEvent ready_;
   };
 

chrome/browser/policy/browser_policy_connector.cc

@@ -407,7 +407,7 @@ UserAffiliation BrowserPolicyConnector::GetUserAffiliation(
   if (install_attributes_ &&
       (gaia::ExtractDomainName(gaia::CanonicalizeEmail(user_name)) ==
            install_attributes_->GetDomain() ||
-       policy::IsDeviceLocalAccountUser(user_name))) {
+       policy::IsDeviceLocalAccountUser(user_name, NULL))) {
     return USER_AFFILIATION_MANAGED;
   }
 #endif

chrome/chrome_browser_chromeos.gypi

@@ -297,6 +297,8 @@
         'browser/chromeos/enrollment_dialog_view.h',
         'browser/chromeos/extensions/default_app_order.cc',
         'browser/chromeos/extensions/default_app_order.h',
+        'browser/chromeos/extensions/device_local_account_management_policy_provider.cc',
+        'browser/chromeos/extensions/device_local_account_management_policy_provider.h',
         'browser/chromeos/extensions/echo_private_api.cc',
         'browser/chromeos/extensions/echo_private_api.h',
         'browser/chromeos/extensions/external_cache.cc',

chrome/chrome_tests_unit.gypi

@@ -654,6 +654,7 @@
         'browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc',
         'browser/chromeos/file_manager/url_util_unittest.cc',
         'browser/chromeos/file_manager/volume_manager_unittest.cc',
+        'browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc',
         'browser/chromeos/extensions/wallpaper_private_api_unittest.cc',
         'browser/chromeos/external_metrics_unittest.cc',
         'browser/chromeos/fileapi/file_access_permissions_unittest.cc',

content/public/test/test_utils.cc

@@ -67,6 +67,17 @@ void ScriptCallback::ResultCallback(const base::Value* result) {
   base::MessageLoop::current()->Quit();
 }
 
+// Adapter that makes a WindowedNotificationObserver::ConditionTestCallback from
+// a WindowedNotificationObserver::ConditionTestCallbackWithoutSourceAndDetails
+// by ignoring the notification source and details.
+bool IgnoreSourceAndDetails(
+    const WindowedNotificationObserver::
+        ConditionTestCallbackWithoutSourceAndDetails& callback,
+    const NotificationSource& source,
+    const NotificationDetails& details) {
+  return callback.Run();
+}
+
 }  // namespace
 
 void RunMessageLoop() {
@@ -186,6 +197,16 @@ WindowedNotificationObserver::WindowedNotificationObserver(
   registrar_.Add(this, notification_type, source_);
 }
 
+WindowedNotificationObserver::WindowedNotificationObserver(
+    int notification_type,
+    const ConditionTestCallbackWithoutSourceAndDetails& callback)
+    : seen_(false),
+      running_(false),
+      callback_(base::Bind(&IgnoreSourceAndDetails, callback)),
+      source_(NotificationService::AllSources()) {
+  registrar_.Add(this, notification_type, source_);
+}
+
 WindowedNotificationObserver::~WindowedNotificationObserver() {}
 
 void WindowedNotificationObserver::Wait() {
@@ -204,7 +225,7 @@ void WindowedNotificationObserver::Observe(
     const NotificationDetails& details) {
   source_ = source;
   details_ = details;
-  if (!callback_.is_null() && !callback_.Run())
+  if (!callback_.is_null() && !callback_.Run(source, details))
     return;
 
   seen_ = true;

content/public/test/test_utils.h

@@ -100,7 +100,8 @@ class MessageLoopRunner : public base::RefCounted<MessageLoopRunner> {
 // If the callback returns |true|, the condition is met. Otherwise, the
 // condition is not yet met and the callback will be invoked again every time a
 // notification of the expected type is received until the callback returns
-// |true|.
+// |true|. For convenience, two callback types are defined, one that is provided
+// with the notification source and details, and one that is not.
 //
 // This helper class exists to avoid the following common pattern in tests:
 //   PerformAction()
@@ -116,8 +117,14 @@ class MessageLoopRunner : public base::RefCounted<MessageLoopRunner> {
 class WindowedNotificationObserver : public NotificationObserver {
  public:
   // Callback invoked on notifications. Should return |true| when the condition
-  // being waited for is met.
-  typedef base::Callback<bool(void)> ConditionTestCallback;
+  // being waited for is met. For convenience, there is a choice between two
+  // callback types, one that is provided with the notification source and
+  // details, and one that is not.
+  typedef base::Callback<bool(const NotificationSource&,
+                              const NotificationDetails&)>
+      ConditionTestCallback;
+  typedef base::Callback<bool(void)>
+      ConditionTestCallbackWithoutSourceAndDetails;
 
   // Set up to wait for a simple condition. The condition is met when a
   // notification of the given |notification_type| from the given |source| is
@@ -131,6 +138,9 @@ class WindowedNotificationObserver : public NotificationObserver {
   // of |notification_type| from any source is received.
   WindowedNotificationObserver(int notification_type,
                                const ConditionTestCallback& callback);
+  WindowedNotificationObserver(
+      int notification_type,
+      const ConditionTestCallbackWithoutSourceAndDetails& callback);
 
   virtual ~WindowedNotificationObserver();
 

net/base/url_util.cc

@@ -4,6 +4,9 @@
 
 #include "net/base/url_util.h"
 
+#include <utility>
+
+#include "base/logging.h"
 #include "base/strings/string_piece.h"
 #include "net/base/escape.h"
 #include "url/gurl.h"
@@ -68,30 +71,66 @@ GURL AppendOrReplaceQueryParameter(const GURL& url,
   return url.ReplaceComponents(replacements);
 }
 
+QueryIterator::QueryIterator(const GURL& url)
+    : url_(url),
+      at_end_(!url.is_valid()) {
+  if (!at_end_) {
+    query_ = url.parsed_for_possibly_invalid_spec().query;
+    Advance();
+  }
+}
+
+QueryIterator::~QueryIterator() {
+}
+
+std::string QueryIterator::GetKey() const {
+  DCHECK(!at_end_);
+  if (key_.is_nonempty())
+    return url_.spec().substr(key_.begin, key_.len);
+  return std::string();
+}
+
+std::string QueryIterator::GetValue() const {
+  DCHECK(!at_end_);
+  if (value_.is_nonempty())
+    return url_.spec().substr(value_.begin, value_.len);
+  return std::string();
+}
+
+const std::string& QueryIterator::GetUnescapedValue() {
+  DCHECK(!at_end_);
+  if (value_.is_nonempty() && unescaped_value_.empty()) {
+    unescaped_value_ = UnescapeURLComponent(
+        GetValue(),
+        UnescapeRule::SPACES |
+        UnescapeRule::URL_SPECIAL_CHARS |
+        UnescapeRule::REPLACE_PLUS_WITH_SPACE);
+  }
+  return unescaped_value_;
+}
+
+bool QueryIterator::IsAtEnd() const {
+  return at_end_;
+}
+
+void QueryIterator::Advance() {
+  DCHECK (!at_end_);
+  key_.reset();
+  value_.reset();
+  unescaped_value_.clear();
+  at_end_ = !url_parse::ExtractQueryKeyValue(url_.spec().c_str(),
+                                             &query_,
+                                             &key_,
+                                             &value_);
+}
+
 bool GetValueForKeyInQuery(const GURL& url,
                            const std::string& search_key,
                            std::string* out_value) {
-  if (!url.is_valid())
-    return false;
-
-  url_parse::Component query = url.parsed_for_possibly_invalid_spec().query;
-  url_parse::Component key, value;
-  while (url_parse::ExtractQueryKeyValue(
-      url.spec().c_str(), &query, &key, &value)) {
-    if (key.is_nonempty()) {
-      std::string key_string = url.spec().substr(key.begin, key.len);
-      if (key_string == search_key) {
-        if (value.is_nonempty()) {
-          *out_value = UnescapeURLComponent(
-              url.spec().substr(value.begin, value.len),
-              UnescapeRule::SPACES |
-              UnescapeRule::URL_SPECIAL_CHARS |
-              UnescapeRule::REPLACE_PLUS_WITH_SPACE);
-        } else {
-          *out_value = "";
-        }
-        return true;
-      }
+  for (QueryIterator it(url); !it.IsAtEnd(); it.Advance()) {
+    if (it.GetKey() == search_key) {
+      *out_value = it.GetUnescapedValue();
+      return true;
     }
   }
   return false;

net/base/url_util.h

@@ -7,7 +7,9 @@
 
 #include <string>
 
+#include "base/compiler_specific.h"
 #include "net/base/net_export.h"
+#include "url/url_parse.h"
 
 class GURL;
 
@@ -44,6 +46,30 @@ NET_EXPORT GURL AppendOrReplaceQueryParameter(const GURL& url,
                                               const std::string& name,
                                               const std::string& value);
 
+// Iterates over the key-value pairs in the query portion of |url|.
+class NET_EXPORT QueryIterator {
+ public:
+  explicit QueryIterator(const GURL& url);
+  ~QueryIterator();
+
+  std::string GetKey() const;
+  std::string GetValue() const;
+  const std::string& GetUnescapedValue();
+
+  bool IsAtEnd() const;
+  void Advance();
+
+ private:
+  const GURL& url_;
+  url_parse::Component query_;
+  bool at_end_;
+  url_parse::Component key_;
+  url_parse::Component value_;
+  std::string unescaped_value_;
+
+  DISALLOW_COPY_AND_ASSIGN(QueryIterator);
+};
+
 // Looks for |search_key| in the query portion of |url|. Returns true if the
 // key is found and sets |out_value| to the unescaped value for the key.
 // Returns false if the key is not found.

net/base/url_util_unittest.cc

@@ -108,5 +108,56 @@ TEST(UrlUtilTest, GetValueForKeyInQueryInvalidURL) {
   EXPECT_FALSE(GetValueForKeyInQuery(url, "test", &value));
 }
 
+TEST(UrlUtilTest, ParseQuery) {
+  const GURL url("http://example.com/path?name=value&boolParam&"
+                 "url=http://test.com/q?n1%3Dv1%26n2&"
+                 "multikey=value1&multikey=value2&multikey");
+  QueryIterator it(url);
+
+  ASSERT_FALSE(it.IsAtEnd());
+  EXPECT_EQ("name", it.GetKey());
+  EXPECT_EQ("value", it.GetValue());
+  EXPECT_EQ("value", it.GetUnescapedValue());
+  it.Advance();
+
+  ASSERT_FALSE(it.IsAtEnd());
+  EXPECT_EQ("boolParam", it.GetKey());
+  EXPECT_EQ("", it.GetValue());
+  EXPECT_EQ("", it.GetUnescapedValue());
+  it.Advance();
+
+  ASSERT_FALSE(it.IsAtEnd());
+  EXPECT_EQ("url", it.GetKey());
+  EXPECT_EQ("http://test.com/q?n1%3Dv1%26n2", it.GetValue());
+  EXPECT_EQ("http://test.com/q?n1=v1&n2", it.GetUnescapedValue());
+  it.Advance();
+
+  ASSERT_FALSE(it.IsAtEnd());
+  EXPECT_EQ("multikey", it.GetKey());
+  EXPECT_EQ("value1", it.GetValue());
+  EXPECT_EQ("value1", it.GetUnescapedValue());
+  it.Advance();
+
+  ASSERT_FALSE(it.IsAtEnd());
+  EXPECT_EQ("multikey", it.GetKey());
+  EXPECT_EQ("value2", it.GetValue());
+  EXPECT_EQ("value2", it.GetUnescapedValue());
+  it.Advance();
+
+  ASSERT_FALSE(it.IsAtEnd());
+  EXPECT_EQ("multikey", it.GetKey());
+  EXPECT_EQ("", it.GetValue());
+  EXPECT_EQ("", it.GetUnescapedValue());
+  it.Advance();
+
+  EXPECT_TRUE(it.IsAtEnd());
+}
+
+TEST(UrlUtilTest, ParseQueryInvalidURL) {
+  const GURL url("http://%01/?test");
+  QueryIterator it(url);
+  EXPECT_TRUE(it.IsAtEnd());
+}
+
 }  // namespace
 }  // namespace net