PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through activeTab permission) down to the origin.

This change also causes the tab object passed to the [page|browser]Action.onClicked to be scrubbed for the given extension.

TEST=
  unit_tests --gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945

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

chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc

@@ -890,8 +890,8 @@ DeviceLocalAccountManagementPolicyProvider::
 
 // static
 bool DeviceLocalAccountManagementPolicyProvider::IsWhitelisted(
-    const extensions::Extension* extension) {
-  return ArrayContains(kPublicSessionWhitelist, extension->id());
+    const std::string& extension_id) {
+  return ArrayContains(kPublicSessionWhitelist, extension_id);
 }
 
 std::string DeviceLocalAccountManagementPolicyProvider::
@@ -921,7 +921,7 @@ bool DeviceLocalAccountManagementPolicyProvider::UserMayLoad(
 
     // Allow extension if its specific ID is whitelisted for use in public
     // sessions.
-    if (IsWhitelisted(extension)) {
+    if (IsWhitelisted(extension->id())) {
       return true;
     }
 

chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.h

@@ -25,7 +25,7 @@ class DeviceLocalAccountManagementPolicyProvider
   ~DeviceLocalAccountManagementPolicyProvider() override;
 
   // Used to check whether an extension is explicitly whitelisted.
-  static bool IsWhitelisted(const extensions::Extension* extension);
+  static bool IsWhitelisted(const std::string& extension_id);
 
   // extensions::ManagementPolicy::Provider:
   std::string GetDebugPolicyProviderName() const override;

chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc

@@ -614,14 +614,12 @@ TEST(DeviceLocalAccountManagementPolicyProviderTest, KioskAppSession) {
 
 TEST(DeviceLocalAccountManagementPolicyProviderTest, IsWhitelisted) {
   // Whitelisted extension, Chrome Remote Desktop.
-  auto extension = CreateRegularExtension(kWhitelistedId);
   EXPECT_TRUE(DeviceLocalAccountManagementPolicyProvider::IsWhitelisted(
-      extension.get()));
+      kWhitelistedId));
 
   // Bogus extension ID (never true).
-  extension = CreateRegularExtension(kBogusId);
   EXPECT_FALSE(DeviceLocalAccountManagementPolicyProvider::IsWhitelisted(
-      extension.get()));
+      kBogusId));
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc

@@ -7,6 +7,7 @@
 #include <string>
 
 #include "base/memory/ptr_util.h"
+#include "chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/common/extensions/api/tabs.h"
 #include "url/gurl.h"
@@ -21,7 +22,9 @@ void ExtensionTabUtilDelegateChromeOS::ScrubTabForExtension(
     const Extension* extension,
     content::WebContents* contents,
     api::tabs::Tab* tab) {
-  if (!profiles::IsPublicSession() || !tab->url) {
+  if (!profiles::IsPublicSession() || !tab->url ||
+      chromeos::DeviceLocalAccountManagementPolicyProvider::IsWhitelisted(
+          extension->id())) {
     return;
   }
   // Scrub URL down to the origin (security reasons inside Public Sessions).

chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.h

@@ -24,7 +24,8 @@ class Extension;
 // In Public Sessions, apps and extensions are force-installed by admin policy
 // so the user does not get a chance to review the permissions for these apps.
 // This is not acceptable from a security standpoint, so we scrub the URL
-// returned by chrome.tabs API down to the origin.
+// returned by chrome.tabs API down to the origin unless the extension ID is
+// whitelisted.
 class ExtensionTabUtilDelegateChromeOS : public ExtensionTabUtil::Delegate {
  public:
   ExtensionTabUtilDelegateChromeOS();

chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos_unittest.cc

@@ -7,15 +7,31 @@
 #include <string>
 
 #include "chromeos/login/login_state.h"
+#include "chromeos/login/scoped_test_public_session_login_state.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/value_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
 namespace {
 
+const char kWhitelistedId[] = "cbkkbcmdlboombapidmoeolnmdacpkch";
+// Use an extension ID that will never be whitelisted.
+const char kNonWhitelistedId[] = "bogus";
+
 const char kTestUrl[] = "http://www.foo.bar/baz?key=val";
 const char kFilteredUrl[] = "http://www.foo.bar/";
 
+scoped_refptr<Extension> CreateExtension(const std::string& id) {
+  return ExtensionBuilder()
+      .SetManifest(
+          DictionaryBuilder().Set("name", "test").Set("version", "0.1").Build())
+      .SetID(id)
+      .Build();
+}
+
 }  // namespace
 
 class ExtensionTabUtilDelegateChromeOSTest : public testing::Test {
@@ -30,25 +46,40 @@ void ExtensionTabUtilDelegateChromeOSTest::SetUp() {
   tab_.url.reset(new std::string(kTestUrl));
 }
 
-TEST_F(ExtensionTabUtilDelegateChromeOSTest, NoFilteringOutsidePublicSession) {
+TEST_F(ExtensionTabUtilDelegateChromeOSTest,
+       NoFilteringOutsidePublicSessionForWhitelisted) {
   ASSERT_FALSE(chromeos::LoginState::IsInitialized());
 
-  delegate_.ScrubTabForExtension(nullptr, nullptr, &tab_);
+  auto extension = CreateExtension(kWhitelistedId);
+  delegate_.ScrubTabForExtension(extension.get(), nullptr, &tab_);
   EXPECT_EQ(kTestUrl, *tab_.url);
 }
 
-TEST_F(ExtensionTabUtilDelegateChromeOSTest, ScrubURL) {
-  // Set Public Session state.
-  chromeos::LoginState::Initialize();
-  chromeos::LoginState::Get()->SetLoggedInState(
-    chromeos::LoginState::LOGGED_IN_ACTIVE,
-    chromeos::LoginState::LOGGED_IN_USER_PUBLIC_ACCOUNT);
+TEST_F(ExtensionTabUtilDelegateChromeOSTest,
+       NoFilteringOutsidePublicSessionForNonWhitelisted) {
+  ASSERT_FALSE(chromeos::LoginState::IsInitialized());
 
-  delegate_.ScrubTabForExtension(nullptr, nullptr, &tab_);
-  EXPECT_EQ(kFilteredUrl, *tab_.url);
+  auto extension = CreateExtension(kNonWhitelistedId);
+  delegate_.ScrubTabForExtension(extension.get(), nullptr, &tab_);
+  EXPECT_EQ(kTestUrl, *tab_.url);
+}
 
-  // Reset state at the end of test.
-  chromeos::LoginState::Shutdown();
+TEST_F(ExtensionTabUtilDelegateChromeOSTest,
+       NoFilteringInsidePublicSessionForWhitelisted) {
+  chromeos::ScopedTestPublicSessionLoginState state;
+
+  auto extension = CreateExtension(kWhitelistedId);
+  delegate_.ScrubTabForExtension(extension.get(), nullptr, &tab_);
+  EXPECT_EQ(kTestUrl, *tab_.url);
+}
+
+TEST_F(ExtensionTabUtilDelegateChromeOSTest,
+       FilterInsidePublicSessionNonWhitelisted) {
+  chromeos::ScopedTestPublicSessionLoginState state;
+
+  auto extension = CreateExtension(kNonWhitelistedId);
+  delegate_.ScrubTabForExtension(extension.get(), nullptr, &tab_);
+  EXPECT_EQ(kFilteredUrl, *tab_.url);
 }
 
 }  // namespace extensions

chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc

@@ -23,7 +23,7 @@ void PermissionsUpdaterDelegateChromeOS::InitializePermissions(
     std::unique_ptr<const PermissionSet>* granted_permissions) {
   if (!profiles::IsPublicSession() ||
       chromeos::DeviceLocalAccountManagementPolicyProvider::IsWhitelisted(
-          extension) ||
+          extension->id()) ||
       !(*granted_permissions)
            ->HasAPIPermission(APIPermission::kClipboardRead)) {
     return;

chrome/browser/extensions/api/extension_action/extension_action_api.cc

@@ -205,7 +205,8 @@ void ExtensionActionAPI::NotifyChange(ExtensionAction* extension_action,
 
 void ExtensionActionAPI::DispatchExtensionActionClicked(
     const ExtensionAction& extension_action,
-    WebContents* web_contents) {
+    WebContents* web_contents,
+    const Extension* extension) {
   events::HistogramValue histogram_value = events::UNKNOWN;
   const char* event_name = NULL;
   switch (extension_action.action_type()) {
@@ -225,7 +226,8 @@ void ExtensionActionAPI::DispatchExtensionActionClicked(
 
   if (event_name) {
     std::unique_ptr<base::ListValue> args(new base::ListValue());
-    args->Append(ExtensionTabUtil::CreateTabObject(web_contents)->ToValue());
+    args->Append(
+        ExtensionTabUtil::CreateTabObject(web_contents, extension)->ToValue());
 
     DispatchEventToExtension(web_contents->GetBrowserContext(),
                              extension_action.extension_id(), histogram_value,

chrome/browser/extensions/api/extension_action/extension_action_api.h

@@ -93,7 +93,8 @@ class ExtensionActionAPI : public BrowserContextKeyedAPI {
 
   // Dispatches the onClicked event for extension that owns the given action.
   void DispatchExtensionActionClicked(const ExtensionAction& extension_action,
-                                      content::WebContents* web_contents);
+                                      content::WebContents* web_contents,
+                                      const Extension* extension);
 
   // Clears the values for all ExtensionActions for the tab associated with the
   // given |web_contents| (and signals that page actions changed).

chrome/browser/extensions/extension_action_runner.cc

@@ -120,7 +120,8 @@ ExtensionAction::ShowAction ExtensionActionRunner::RunAction(
     return ExtensionAction::ACTION_SHOW_POPUP;
 
   ExtensionActionAPI::Get(browser_context_)
-      ->DispatchExtensionActionClicked(*extension_action, web_contents());
+      ->DispatchExtensionActionClicked(*extension_action, web_contents(),
+                                       extension);
   return ExtensionAction::ACTION_NONE;
 }
 

chrome/browser/extensions/native_bindings_apitest.cc

@@ -111,7 +111,7 @@ IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, DeclarativeEvents) {
   // And the extension should be notified of the click.
   ExtensionTestMessageListener clicked_listener("clicked and removed", false);
   ExtensionActionAPI::Get(profile())->DispatchExtensionActionClicked(
-      *page_action, web_contents);
+      *page_action, web_contents, extension);
   ASSERT_TRUE(clicked_listener.WaitUntilSatisfied());
 }
 

chromeos/BUILD.gn

@@ -548,6 +548,8 @@ static_library("test_support") {
     "login/auth/mock_auth_status_consumer.h",
     "login/auth/mock_url_fetchers.cc",
     "login/auth/mock_url_fetchers.h",
+    "login/scoped_test_public_session_login_state.cc",
+    "login/scoped_test_public_session_login_state.h",
     "network/fake_network_device_handler.cc",
     "network/fake_network_device_handler.h",
     "network/mock_managed_network_configuration_handler.cc",

chromeos/login/scoped_test_public_session_login_state.cc

+// Copyright 2017 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 "chromeos/login/scoped_test_public_session_login_state.h"
+
+#include "chromeos/login/login_state.h"
+
+namespace chromeos {
+
+ScopedTestPublicSessionLoginState::ScopedTestPublicSessionLoginState() {
+  // Set Public Session state.
+  chromeos::LoginState::Initialize();
+  chromeos::LoginState::Get()->SetLoggedInState(
+      chromeos::LoginState::LOGGED_IN_ACTIVE,
+      chromeos::LoginState::LOGGED_IN_USER_PUBLIC_ACCOUNT);
+}
+
+ScopedTestPublicSessionLoginState::~ScopedTestPublicSessionLoginState() {
+  // Reset state at the end of test.
+  chromeos::LoginState::Shutdown();
+}
+
+}  // namespace chromeos

chromeos/login/scoped_test_public_session_login_state.h

+// Copyright 2017 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 CHROMEOS_LOGIN_SCOPED_TEST_PUBLIC_SESSION_LOGIN_STATE_H_
+#define CHROMEOS_LOGIN_SCOPED_TEST_PUBLIC_SESSION_LOGIN_STATE_H_
+
+#include "base/macros.h"
+
+namespace chromeos {
+
+// A class to start and shutdown public session state for a test.
+class ScopedTestPublicSessionLoginState {
+ public:
+  ScopedTestPublicSessionLoginState();
+  ~ScopedTestPublicSessionLoginState();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ScopedTestPublicSessionLoginState);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_LOGIN_SCOPED_TEST_PUBLIC_SESSION_LOGIN_STATE_H_