Allow explicitly whitelisted apps/extensions in public sessions

chrome/browser/chromeos/policy/device_local_account_browsertest.cc

Index: chrome/browser/chromeos/policy/device_local_account_browsertest.cc
diff --git a/chrome/browser/chromeos/policy/device_local_account_browsertest.cc b/chrome/browser/chromeos/policy/device_local_account_browsertest.cc
index c125281f4547b33a63715d117fe8efc9f0af7643..483d86bd7019d96661e7f53128bf8a924522e942 100644
--- a/chrome/browser/chromeos/policy/device_local_account_browsertest.cc
+++ b/chrome/browser/chromeos/policy/device_local_account_browsertest.cc
@@ -4,9 +4,12 @@
 
 #include <map>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
@@ -33,6 +36,8 @@
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/policy/device_policy_builder.h"
 #include "chrome/browser/chromeos/policy/device_policy_cros_browser_test.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/policy/cloud/cloud_policy_constants.h"
 #include "chrome/browser/policy/cloud/policy_builder.h"
@@ -52,12 +57,14 @@
 #include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/fake_cryptohome_client.h"
 #include "chromeos/dbus/fake_session_manager_client.h"
 #include "chromeos/dbus/session_manager_client.h"
+#include "content/public/browser/notification_details.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/test/browser_test_utils.h"
@@ -65,7 +72,10 @@
 #include "crypto/rsa_private_key.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
+#include "net/http/http_status_code.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -91,10 +101,120 @@ const char* kStartupURLs[] = {
 };
 const char kExistentTermsOfServicePath[] = "chromeos/enterprise/tos.txt";
 const char kNonexistentTermsOfServicePath[] = "chromeos/enterprise/tos404.txt";
+const char kUpdatePath[] = "/service/update2/crx";
+const char kUpdateManifestHeader[] =
+    "<?xml version='1.0' encoding='UTF-8'?>\n"
+    "<gupdate xmlns='http://www.google.com/update2/response' protocol='2.0'>\n";
+const char kUpdateManifestTemplate[] =
+    "  <app appid='$1'>\n"
+    "    <updatecheck codebase='$2' version='$3' />\n"
+    "  </app>\n";
+const char kUpdateManifestFooter[] =
+    "</gupdate>\n";
+const char kHostedAppID[] = "kbmnembihfiondgfjekmnmcbddelicoi";
+const char kHostedAppCRXPath[] = "extensions/hosted_app.crx";
+const char kHostedAppVersion[] = "0.1";
+const char kGoodExtensionID[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
+const char kGoodExtensionPath[] = "extensions/good.crx";
+const char kGoodExtensionVersion[] = "1.0";
+
+class ExtensionInstallSuccessObserver

[Mattias Nissler (ping if slow), 2013/09/23 12:53:29]

Is there any good reasons to make these classes if you can just base::Bind
CheckNotification?

[bartfab (slow), 2013/09/23 17:03:05]

CheckNotification() needs to access the NotificationDetails. This is not
normally possible from a WindowedNotificationObserver callback because the
callback has no pointer to the WindowedNotificationObserver it belongs to. I
could instantiate a callback with a pointer to a pointer argument, instantiate a
WindowedNotificationObserver that uses the callback and then inject a pointer to
the WindowedNotificationObserver via the argument from the first step. But I
think that this is clumsy and error-prone.

[Mattias Nissler (ping if slow), 2013/09/25 12:04:08]

Sounds like a subclass for WindowedNotificationObserver that forwards the
Details pointer to the check callback could at least reduce the number of
subclasses to only one then? 

Arguably, this should be supported by WindowedNotificationObserver directly. I
guess you could generalize it to support callbacks that receive Details and
Source, and then you can have a convenience ctor that adapts the function type.

[bartfab (slow), 2013/09/26 12:39:13]

Done.

+    : public content::WindowedNotificationObserver {
+ public:
+  explicit ExtensionInstallSuccessObserver(const std::string& id);
+  ~ExtensionInstallSuccessObserver();
+
+ private:
+  bool CheckNotification();
+
+  const std::string id_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionInstallSuccessObserver);
+};
+
+class ExtensionInstallFailureObserver
+    : public content::WindowedNotificationObserver {
+ public:
+  explicit ExtensionInstallFailureObserver(const std::string& id);
+  ~ExtensionInstallFailureObserver();
+
+ private:
+  bool CheckNotification();
+
+  const string16 id_;

[Mattias Nissler (ping if slow), 2013/09/23 12:53:29]

Why is this a string16, while it is a std::string in line 130?

[bartfab (slow), 2013/09/23 17:03:05]

They listen for different notification types. An installation success provides
the extension ID as an std::string, an installation failure provides the
extension ID as a string16.

[Mattias Nissler (ping if slow), 2013/09/25 12:04:08]

Doh. Does the extension team have a bug for this? If not, please file one.

[bartfab (slow), 2013/09/26 12:39:13]

Filed crbug.com/298810

+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionInstallFailureObserver);
+};
+
+ExtensionInstallSuccessObserver::ExtensionInstallSuccessObserver(
+    const std::string& id)
+    : content::WindowedNotificationObserver(
+          chrome::NOTIFICATION_EXTENSION_INSTALLED,
+          base::Bind(&ExtensionInstallSuccessObserver::CheckNotification,
+                     base::Unretained(this))),
+      id_(id) {
+}
+
+ExtensionInstallSuccessObserver::~ExtensionInstallSuccessObserver() {
+}
+
+bool ExtensionInstallSuccessObserver::CheckNotification() {
+  return content::Details<const extensions::InstalledExtensionInfo>(
+      details())->extension->id() == id_;
+}
+
+ExtensionInstallFailureObserver::ExtensionInstallFailureObserver(
+    const std::string& id)
+    : content::WindowedNotificationObserver(
+          chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
+          base::Bind(&ExtensionInstallFailureObserver::CheckNotification,
+                     base::Unretained(this))),
+      id_(UTF8ToUTF16(id)) {
+}
+
+ExtensionInstallFailureObserver::~ExtensionInstallFailureObserver() {
+}
+
+bool ExtensionInstallFailureObserver::CheckNotification() {
+  return content::Details<const string16>(details())->find(id_) !=
+      string16::npos;
+}
 
 }  // namespace
 
 class DeviceLocalAccountTest : public DevicePolicyCrosBrowserTest {
+ public:
+  scoped_ptr<net::test_server::HttpResponse> HandleRequest(

[Mattias Nissler (ping if slow), 2013/09/23 12:53:29]

I guess it would be good to factor this out so other tests that need to serve
update manifests can use the same code. Speaking of which: Are you sure this
doesn't exist somewhere else already?

[bartfab (slow), 2013/09/23 17:03:05]

Refactored into a reusable TestingUpdateManifestProvider.

AFAICT, there is no code that does something similar. Other tests use a fake
URLFetcherFactory and work with hard-coded manifest files in the test data
directory. I wanted to dynamically construct the manifests so that the embedded
test server can be used for the CRX files instead.

+      const net::test_server::HttpRequest& request) {
+    if (request.relative_url.find(kUpdatePath) != 0)
+      return scoped_ptr<net::test_server::HttpResponse>();

[Mattias Nissler (ping if slow), 2013/09/23 12:53:29]

nit: blank line here for readability

[bartfab (slow), 2013/09/23 17:03:05]

Done.

+    std::string content = kUpdateManifestHeader;
+    size_t id_pos = 0;
+    while ((id_pos = request.relative_url.find("id%3D", id_pos)) !=

[Mattias Nissler (ping if slow), 2013/09/23 12:53:29]

Can you use proper URL parsing, for GetValueForKeyInQuery in
net/base/url_util.h?

Also not that there is switches::kAppsGalleryURL, which can potentially make all
this significantly easier.

[bartfab (slow), 2013/09/23 17:03:05]

Re GetValueForKeyInQuery(): Unfortunately, I cannot use that. It will extract
only one value per key and the key I am looking for repeats several times in the
URL.

Re switches::kAppsGalleryURL: As discussed offline, this is not applicable here.

[Mattias Nissler (ping if slow), 2013/09/25 12:04:08]

Ad hoc URL parsing still isn't a good idea. Note that the underlying url_parse
functionality can handle this case. Maybe we should just make a helper that
decodes a query string to a std::multimap and put that in net/base/url_util.h
for everybody to use?

[bartfab (slow), 2013/09/26 12:39:13]

Done.

+               std::string::npos) {
+      id_pos += 5;
+      const std::string id = request.relative_url.substr(id_pos, 32);
+      id_pos += 32;
+      ExtensionMap::const_iterator entry = extension_map_.find(id);
+      if (entry != extension_map_.end()) {
+        std::vector<std::string> parts;
+        parts.push_back(id);
+        parts.push_back(embedded_test_server()->GetURL(
+            std::string("/") + entry->second.first).spec());
+        parts.push_back(entry->second.second);
+        content += ReplaceStringPlaceholders(

[Mattias Nissler (ping if slow), 2013/09/23 12:53:29]

base::StringPrintf is probably simpler to use here.

[bartfab (slow), 2013/09/23 17:03:05]

Funny. I specifically chose ReplaceStringPlaceholders because I find
base::StringPrintf clumsy with all the c_str() it requires. But I have no strong
preference for either one. Done.

+            kUpdateManifestTemplate, parts, NULL);
+      }
+    }
+    content += kUpdateManifestFooter;
+    scoped_ptr<net::test_server::BasicHttpResponse>
+        http_response(new net::test_server::BasicHttpResponse);
+    http_response->set_code(net::HTTP_OK);
+    http_response->set_content(content);
+    http_response->set_content_type("text/xml");
+    return http_response.PassAs<net::test_server::HttpResponse>();
+  }
+
  protected:
   DeviceLocalAccountTest()
       : user_id_1_(GenerateDeviceLocalAccountUserId(
@@ -208,6 +328,10 @@ class DeviceLocalAccountTest : public DevicePolicyCrosBrowserTest {
 
   UserPolicyBuilder device_local_account_policy_;
   LocalPolicyTestServer test_server_;
+
+  typedef std::map<std::string, std::pair<std::string, std::string> >
+      ExtensionMap;
+  ExtensionMap extension_map_;
 };
 
 static bool IsKnownUser(const std::string& account_id) {
@@ -421,6 +545,84 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, FullscreenDisallowed) {
   EXPECT_FALSE(browser_window->IsFullscreen());
 }
 
+IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, ExtensionWhitelist) {
+  // Make it possible to force-install a hosted app and an extension.
+  extension_map_[kHostedAppID] =
+      std::pair<std::string, std::string>(kHostedAppCRXPath,
+                                          kHostedAppVersion);
+  extension_map_[kGoodExtensionID] =
+      std::pair<std::string, std::string>(kGoodExtensionPath,
+                                          kGoodExtensionVersion);
+
+  // Specify policy to force-install the hosted app and the extension.
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  embedded_test_server()->RegisterRequestHandler(
+      base::Bind(&DeviceLocalAccountTest::HandleRequest,
+                 base::Unretained(this)));
+  em::StringList* forcelist = device_local_account_policy_.payload()
+      .mutable_extensioninstallforcelist()->mutable_value();
+  std::vector<std::string> parts;
+  parts.push_back(kHostedAppID);
+  parts.push_back(embedded_test_server()->GetURL(kUpdatePath).spec());
+  forcelist->add_entries(ReplaceStringPlaceholders("$1;$2", parts, NULL));

[Mattias Nissler (ping if slow), 2013/09/23 12:53:29]

StringPrintf is simpler

[bartfab (slow), 2013/09/23 17:03:05]

Done.

+  parts.clear();
+  parts.push_back(kGoodExtensionID);
+  parts.push_back(embedded_test_server()->GetURL(kUpdatePath).spec());
+  forcelist->add_entries(ReplaceStringPlaceholders("$1;$2", parts, NULL));
+
+  UploadAndInstallDeviceLocalAccountPolicy();
+  AddPublicSessionToDevicePolicy(kAccountId1);
+
+  // This observes the display name becoming available as this indicates
+  // device-local account policy is fully loaded, which is a prerequisite for
+  // successful login.
+  content::WindowedNotificationObserver(
+      chrome::NOTIFICATION_USER_LIST_CHANGED,
+      base::Bind(&DisplayNameMatches, user_id_1_, kDisplayName)).Wait();
+
+  // Wait for the login UI to be ready.
+  chromeos::LoginDisplayHostImpl* host =
+      reinterpret_cast<chromeos::LoginDisplayHostImpl*>(
+          chromeos::LoginDisplayHostImpl::default_host());
+  ASSERT_TRUE(host);
+  chromeos::OobeUI* oobe_ui = host->GetOobeUI();
+  ASSERT_TRUE(oobe_ui);
+  base::RunLoop run_loop;
+  const bool oobe_ui_ready = oobe_ui->IsJSReady(run_loop.QuitClosure());
+  if (!oobe_ui_ready)
+    run_loop.Run();
+
+  // Ensure that the browser stays alive, even though no windows are opened
+  // during session start.
+  chrome::StartKeepAlive();
+
+  // Start listening for app/extension installation results.
+  ExtensionInstallSuccessObserver hosted_app_observer(kHostedAppID);
+  ExtensionInstallFailureObserver extension_observer(kGoodExtensionID);
+
+  // Start login into the device-local account.
+  host->StartSignInScreen();
+  chromeos::ExistingUserController* controller =
+      chromeos::ExistingUserController::current_controller();
+  ASSERT_TRUE(controller);
+  controller->LoginAsPublicAccount(user_id_1_);
+
+  // Wait for the hosted app installation to succeed and the extension
+  // installation to fail.
+  hosted_app_observer.Wait();
+  extension_observer.Wait();
+
+  // Verify that the hosted app was installed.
+  Profile* profile = ProfileManager::GetDefaultProfile();
+  ASSERT_TRUE(profile);
+  ExtensionService* extension_service =
+      extensions::ExtensionSystem::Get(profile)->extension_service();
+  EXPECT_TRUE(extension_service->GetExtensionById(kHostedAppID, true));
+
+  // Verify that the extension was not installed.
+  EXPECT_FALSE(extension_service->GetExtensionById(kGoodExtensionID, true));
+}
+
 class TermsOfServiceTest : public DeviceLocalAccountTest,
                            public testing::WithParamInterface<bool> {
 };