Unpin some policy-pinned apps in Demo Mode when offline

Devices in Demo Mode are enrolled into the cros-demo-mode.com domain,
which uses enterprise policy to set the list of force-installed apps
and which of those apps are force-pinned to the shelf. This lets us
choose which first and third party apps to feature in the demo.

A few demo apps only work online. When a device in Demo Mode is
offline, we don't want to feature those apps, but they're
unconditionally force-pinned by policy.

This CL updates the shelf's pinned policy pref handling to ignore
certain apps pinned by policy when the device is in Demo Mode and
offline. The user can still pin and unpin those apps themselves.

Bug: 870846
Change-Id: Ie15ea89f77fd5ed6a789614a83c39bb2aee286d2
Reviewed-on: https://chromium-review.googlesource.com/1179318
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586015}

chrome/browser/chromeos/login/demo_mode/demo_session.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/login/demo_mode/demo_session.h"
 
+#include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ -19,6 +20,7 @@
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/settings/install_attributes.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -26,6 +28,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/session_manager/core/session_manager.h"
 #include "content/public/browser/browser_thread.h"
+#include "net/base/network_change_notifier.h"
 
 namespace chromeos {
 
@@ -61,6 +64,18 @@ bool IsDemoModeOfflineEnrolled() {
   return DemoSession::GetDemoConfig() == DemoSession::DemoModeConfig::kOffline;
 }
 
+// Returns the list of apps normally pinned by Demo Mode policy that shouldn't
+// be pinned if the device is offline.
+std::vector<std::string> GetIgnorePinPolicyApps() {
+  return {
+      // Popular third-party game preinstalled in Demo Mode that is
+      // online-only, so shouldn't be featured in the shelf when offline.
+      "com.pixonic.wwr.chbkdemo",
+      // TODO(michaelpg): YouTube is also pinned as a *default* app.
+      extension_misc::kYoutubeAppId,
+  };
+}
+
 }  // namespace
 
 // static
@@ -219,6 +234,11 @@ void DemoSession::EnsureOfflineResourcesLoaded(
   }
 }
 
+void DemoSession::SetOfflineResourcesLoadedForTesting(
+    const base::FilePath& path) {
+  OnOfflineResourcesLoaded(path);
+}
+
 base::FilePath DemoSession::GetDemoAppsPath() const {
   if (offline_resources_path_.empty())
     return base::FilePath();
@@ -240,8 +260,28 @@ base::FilePath DemoSession::GetOfflineResourceAbsolutePath(
   return offline_resources_path_.Append(relative_path);
 }
 
+bool DemoSession::ShouldIgnorePinPolicy(const std::string& app_id_or_package) {
+  if (!g_demo_session || !g_demo_session->started())
+    return false;
+
+  // TODO(michaelpg): Update shelf when network status changes.
+  // TODO(michaelpg): Also check for captive portal.
+  if (!net::NetworkChangeNotifier::IsOffline())
+    return false;
+
+  return std::find(ignore_pin_policy_offline_apps_.begin(),
+                   ignore_pin_policy_offline_apps_.end(),
+                   app_id_or_package) != ignore_pin_policy_offline_apps_.end();
+}
+
+void DemoSession::OverrideIgnorePinPolicyAppsForTesting(
+    std::vector<std::string> apps) {
+  ignore_pin_policy_offline_apps_ = std::move(apps);
+}
+
 DemoSession::DemoSession()
     : offline_enrolled_(IsDemoModeOfflineEnrolled()),
+      ignore_pin_policy_offline_apps_(GetIgnorePinPolicyApps()),
       session_manager_observer_(this),
       weak_ptr_factory_(this) {
   session_manager_observer_.Add(session_manager::SessionManager::Get());

chrome/browser/chromeos/login/demo_mode/demo_session.h

@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_CHROMEOS_LOGIN_DEMO_MODE_DEMO_SESSION_H_
 
 #include <list>
+#include <string>
+#include <vector>
 
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
@@ -82,6 +84,10 @@ class DemoSession : public session_manager::SessionManagerObserver {
   // |load_callback| will be run once the offline resource load finishes.
   void EnsureOfflineResourcesLoaded(base::OnceClosure load_callback);
 
+  // Fakes offline demo session resources having been requested and mounted at
+  // the given path (or not mounted if |path| is empty).
+  void SetOfflineResourcesLoadedForTesting(const base::FilePath& path);
+
   // Gets the path of the image containing demo session Android apps. The path
   // will be set when the offline resources get loaded.
   base::FilePath GetDemoAppsPath() const;
@@ -98,6 +104,15 @@ class DemoSession : public session_manager::SessionManagerObserver {
   base::FilePath GetOfflineResourceAbsolutePath(
       const base::FilePath& relative_path) const;
 
+  // Returns true if the Chrome app or ARC++ package, which is normally pinned
+  // by policy, should actually not be force-pinned because the device is
+  // in Demo Mode and offline.
+  bool ShouldIgnorePinPolicy(const std::string& app_id_or_package);
+
+  // Sets app IDs and package names that shouldn't be pinned by policy when the
+  // device is offline in Demo Mode.
+  void OverrideIgnorePinPolicyAppsForTesting(std::vector<std::string> apps);
+
   bool offline_enrolled() const { return offline_enrolled_; }
 
   bool started() const { return started_; }
@@ -140,6 +155,9 @@ class DemoSession : public session_manager::SessionManagerObserver {
   // Path at which offline demo mode resources were loaded.
   base::FilePath offline_resources_path_;
 
+  // Apps that ShouldIgnorePinPolicy() will check for if the device is offline.
+  std::vector<std::string> ignore_pin_policy_offline_apps_;
+
   // List of pending callbacks passed to EnsureOfflineResourcesLoaded().
   std::list<base::OnceClosure> offline_resources_load_callbacks_;
 

chrome/browser/ui/ash/chrome_launcher_prefs.cc

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/values.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
+#include "chrome/browser/chromeos/login/demo_mode/demo_session.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
@@ -199,6 +200,12 @@ void GetAppsPinnedByPolicy(const PrefService* prefs,
       LOG(ERROR) << "Cannot extract policy app info from prefs.";
       continue;
     }
+
+    if (chromeos::DemoSession::Get() &&
+        chromeos::DemoSession::Get()->ShouldIgnorePinPolicy(app_id)) {
+      continue;
+    }
+
     if (IsAppIdArcPackage(app_id)) {
       if (!arc_app_list_pref)
         continue;

chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

@@ -39,6 +39,7 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
+#include "chrome/browser/chromeos/login/demo_mode/demo_session.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
@@ -82,6 +83,8 @@
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_image_loader_client.h"
 #include "components/account_id/account_id.h"
 #include "components/arc/arc_prefs.h"
 #include "components/arc/arc_util.h"
@@ -91,6 +94,7 @@
 #include "components/exo/shell_surface.h"
 #include "components/keep_alive_registry/scoped_keep_alive.h"
 #include "components/prefs/pref_notifier_impl.h"
+#include "components/session_manager/core/session_manager.h"
 #include "components/sync/model/fake_sync_change_processor.h"
 #include "components/sync/model/sync_error_factory_mock.h"
 #include "components/sync/protocol/sync.pb.h"
@@ -108,6 +112,8 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/grit/extensions_browser_resources.h"
+#include "net/base/mock_network_change_notifier.h"
+#include "net/base/network_change_notifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/client/window_parenting_client.h"
 #include "ui/aura/window.h"
@@ -4395,3 +4401,157 @@ TEST_F(ChromeLauncherControllerTest, InternalAppWindowPropertyChanged) {
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(launcher_controller_->GetItem(shelf_id));
 }
+
+class ChromeLauncherControllerDemoModeTest
+    : public ChromeLauncherControllerTest {
+ protected:
+  ChromeLauncherControllerDemoModeTest() { auto_start_arc_test_ = true; }
+  ~ChromeLauncherControllerDemoModeTest() override {}
+
+  void SetUp() override {
+    arc::SetArcAlwaysStartForTesting(true);
+
+    // To prevent crash on test exit and pending decode request.
+    ArcAppIcon::DisableSafeDecodingForTesting();
+
+    // Fake online Demo Mode.
+    session_manager_ = std::make_unique<session_manager::SessionManager>();
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetImageLoaderClient(
+        std::make_unique<chromeos::FakeImageLoaderClient>());
+    chromeos::DemoSession::SetDemoConfigForTesting(
+        chromeos::DemoSession::DemoModeConfig::kOnline);
+    ASSERT_TRUE(chromeos::DemoSession::StartIfInDemoMode());
+    chromeos::DemoSession::Get()->SetOfflineResourcesLoadedForTesting(
+        base::FilePath());
+
+    ChromeLauncherControllerTest::SetUp();
+  }
+
+  void TearDown() override {
+    ChromeLauncherControllerTest::TearDown();
+
+    chromeos::DemoSession::ShutDownIfInitialized();
+    chromeos::DemoSession::ResetDemoConfigForTesting();
+    chromeos::DBusThreadManager::Shutdown();
+  }
+
+ private:
+  std::unique_ptr<session_manager::SessionManager> session_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeLauncherControllerDemoModeTest);
+};
+
+TEST_F(ChromeLauncherControllerDemoModeTest, PinnedAppsOnline) {
+  net::test::MockNetworkChangeNotifier notifier;
+  notifier.SetConnectionType(net::NetworkChangeNotifier::CONNECTION_ETHERNET);
+
+  InitLauncherControllerWithBrowser();
+
+  base::ListValue policy_value;
+
+  extension_service_->AddExtension(extension1_.get());
+  extension_service_->AddExtension(extension2_.get());
+  InsertPrefValue(&policy_value, 0, extension1_->id());
+  InsertPrefValue(&policy_value, 1, extension2_->id());
+
+  arc::mojom::AppInfo appinfo =
+      CreateAppInfo("Some App", "SomeActivity", "com.example.app");
+  const std::string app_id = AddArcAppAndShortcut(appinfo);
+
+  arc::mojom::AppInfo online_only_appinfo =
+      CreateAppInfo("Some App", "SomeActivity", "com.example.onlineonly");
+  const std::string online_only_app_id =
+      AddArcAppAndShortcut(online_only_appinfo);
+
+  InsertPrefValue(&policy_value, 2, appinfo.package_name);
+  InsertPrefValue(&policy_value, 3, online_only_appinfo.package_name);
+
+  // If the device is offline, extension2 and onlineonly should be unpinned.
+  chromeos::DemoSession::Get()->OverrideIgnorePinPolicyAppsForTesting(
+      {extension2_->id(), online_only_appinfo.package_name});
+
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kPolicyPinnedLauncherApps, policy_value.CreateDeepCopy());
+
+  // Since the device is online, all policy pinned apps are pinned.
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(extension1_->id()));
+  EXPECT_EQ(AppListControllerDelegate::PIN_FIXED,
+            GetPinnableForAppID(extension1_->id(), profile()));
+
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(extension2_->id()));
+  EXPECT_EQ(AppListControllerDelegate::PIN_FIXED,
+            GetPinnableForAppID(extension2_->id(), profile()));
+
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(app_id));
+  EXPECT_EQ(AppListControllerDelegate::PIN_FIXED,
+            GetPinnableForAppID(app_id, profile()));
+
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(online_only_app_id));
+  EXPECT_EQ(AppListControllerDelegate::PIN_FIXED,
+            GetPinnableForAppID(online_only_app_id, profile()));
+}
+
+TEST_F(ChromeLauncherControllerDemoModeTest, PinnedAppsOffline) {
+  net::test::MockNetworkChangeNotifier notifier;
+  notifier.SetConnectionType(net::NetworkChangeNotifier::CONNECTION_NONE);
+
+  InitLauncherControllerWithBrowser();
+
+  base::ListValue policy_value;
+
+  extension_service_->AddExtension(extension1_.get());
+  extension_service_->AddExtension(extension2_.get());
+  InsertPrefValue(&policy_value, 0, extension1_->id());
+  InsertPrefValue(&policy_value, 1, extension2_->id());
+
+  arc::mojom::AppInfo appinfo =
+      CreateAppInfo("Some App", "SomeActivity", "com.example.app");
+  const std::string app_id = AddArcAppAndShortcut(appinfo);
+
+  arc::mojom::AppInfo online_only_appinfo =
+      CreateAppInfo("Some App", "SomeActivity", "com.example.onlineonly");
+  const std::string online_only_app_id =
+      AddArcAppAndShortcut(online_only_appinfo);
+
+  InsertPrefValue(&policy_value, 2, appinfo.package_name);
+  InsertPrefValue(&policy_value, 3, online_only_appinfo.package_name);
+
+  // If the device is offline, extension2 and onlineonly should be unpinned.
+  chromeos::DemoSession::Get()->OverrideIgnorePinPolicyAppsForTesting(
+      {extension2_->id(), online_only_appinfo.package_name});
+
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kPolicyPinnedLauncherApps, policy_value.CreateDeepCopy());
+
+  // Since the device is online, the policy pinned apps that shouldn't be pinned
+  // in Demo Mode are unpinned.
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(extension1_->id()));
+  EXPECT_EQ(AppListControllerDelegate::PIN_FIXED,
+            GetPinnableForAppID(extension1_->id(), profile()));
+
+  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
+  EXPECT_EQ(AppListControllerDelegate::PIN_EDITABLE,
+            GetPinnableForAppID(extension2_->id(), profile()));
+
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(app_id));
+  EXPECT_EQ(AppListControllerDelegate::PIN_FIXED,
+            GetPinnableForAppID(app_id, profile()));
+
+  EXPECT_FALSE(launcher_controller_->IsAppPinned(online_only_app_id));
+  EXPECT_EQ(AppListControllerDelegate::PIN_EDITABLE,
+            GetPinnableForAppID(online_only_app_id, profile()));
+
+  // Pin a Chrome app that would have been pinned by policy but was suppressed
+  // for Demo Mode.
+  launcher_controller_->PinAppWithID(extension2_->id());
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(extension2_->id()));
+  EXPECT_EQ(AppListControllerDelegate::PIN_EDITABLE,
+            GetPinnableForAppID(extension2_->id(), profile()));
+
+  // Pin an ARC app that would have been pinned by policy but was suppressed
+  // for Demo Mode.
+  launcher_controller_->PinAppWithID(online_only_app_id);
+  EXPECT_TRUE(launcher_controller_->IsAppPinned(app_id));
+  EXPECT_EQ(AppListControllerDelegate::PIN_EDITABLE,
+            GetPinnableForAppID(online_only_app_id, profile()));
+}

chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h"
 
+#include "chrome/browser/chromeos/login/demo_mode/demo_session.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "chrome/browser/ui/ash/chrome_launcher_prefs.h"
@@ -35,12 +36,12 @@ AppListControllerDelegate::Pinnable GetPinnableForAppID(
   // (package name + activity). This means that we must identify the package
   // from the hash, and check if this package is pinned by policy.
   const ArcAppListPrefs* const arc_prefs = ArcAppListPrefs::Get(profile);
-  std::string arc_app_packege_name;
+  std::string arc_app_package_name;
   if (arc_prefs) {
     std::unique_ptr<ArcAppListPrefs::AppInfo> app_info =
         arc_prefs->GetApp(app_id);
     if (app_info)
-      arc_app_packege_name = app_info->package_name;
+      arc_app_package_name = app_info->package_name;
   }
   for (size_t index = 0; index < pref->GetSize(); ++index) {
     const base::DictionaryValue* app = nullptr;
@@ -48,7 +49,12 @@ AppListControllerDelegate::Pinnable GetPinnableForAppID(
     if (pref->GetDictionary(index, &app) &&
         app->GetString(kPinnedAppsPrefAppIDPath, &app_id_or_package) &&
         (app_id == app_id_or_package ||
-         arc_app_packege_name == app_id_or_package)) {
+         arc_app_package_name == app_id_or_package)) {
+      if (chromeos::DemoSession::Get() &&
+          chromeos::DemoSession::Get()->ShouldIgnorePinPolicy(
+              app_id_or_package)) {
+        return AppListControllerDelegate::PIN_EDITABLE;
+      }
       return AppListControllerDelegate::PIN_FIXED;
     }
   }