[Reland] Chrome OS: Only restart chrome if site isolation command line flags change

On user session start, only request the chrome process to be restarted
if site isolation policies effectively change between the sign-in screen
and the user session.
The DeviceLoginScreenSitePerProcess and DeviceLoginScreenIsolateOrigins
policies map to --site-per-process and --isolate-origins command-line
flags passed to the sign-in screen chrome process by session manager
(see CL:924421).
The user session should respect SitePerProcess/IsolateOrigins user
policies instead.

Replace special handling of the site isolation flags with placing them
between --policy-flags-begin and --policy-flags-end sentinels.
This makes sure that chrome is restarted exactly when the user session
flags don't match the login screen flags.
Background: chrome decides if it should be restarted by comparing the
parts of the current command line with parts of the target command line.
Specifically, the parts between flags and policy sentinels are compared.

Also, evaluate the user policies mapped through user Profile prefs
instead of local_state.
Reason: When login_manager starts chrome with site isolation flags, the
prefs are set by ChromeCommandLinePrefStore.

TBR=xiyuan@chromium.org

Bug: 800117
Test: browser_tests --gtest_filter=*SiteIsolationFlagHandlingTest*
Change-Id: I85d47bd2a4da9155e9c6ad338284e1bfed5b478e
Reviewed-on: https://chromium-review.googlesource.com/924147
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#537733}
Reviewed-on: https://chromium-review.googlesource.com/926370
Cr-Commit-Position: refs/heads/master@{#537793}

chrome/browser/chrome_content_browser_client.cc

@@ -881,6 +881,11 @@ void ChromeContentBrowserClient::RegisterProfilePrefs(
   registry->RegisterBooleanPref(prefs::kDisable3DAPIs, false);
   registry->RegisterBooleanPref(prefs::kEnableHyperlinkAuditing, true);
   registry->RegisterListPref(prefs::kEnableDeprecatedWebPlatformFeatures);
+  // Register user prefs for mapping SitePerProcess and IsolateOrigins in
+  // user policy in addition to the same named ones in Local State (which are
+  // used for mapping the command-line flags).
+  registry->RegisterStringPref(prefs::kIsolateOrigins, std::string());
+  registry->RegisterBooleanPref(prefs::kSitePerProcess, false);
 }
 
 // static

chrome/browser/chromeos/login/session/user_session_manager.cc

@@ -124,6 +124,7 @@
 #include "extensions/common/features/feature_session_type.h"
 #include "net/cert/sth_distributor.h"
 #include "rlz/features/features.h"
+#include "third_party/cros_system_api/switches/chrome_switches.h"
 #include "ui/base/ime/chromeos/input_method_descriptor.h"
 #include "ui/base/ime/chromeos/input_method_manager.h"
 #include "ui/base/ime/chromeos/input_method_util.h"
@@ -274,7 +275,8 @@ base::CommandLine CreatePerSessionCommandLine(Profile* profile) {
   about_flags::ConvertFlagsToSwitches(&flags_storage_, &user_flags,
                                       flags_ui::kAddSentinels);
 
-  UserSessionManager::MaybeAppendPolicySwitches(&user_flags);
+  UserSessionManager::MaybeAppendPolicySwitches(profile->GetPrefs(),
+                                                &user_flags);
 
   return user_flags;
 }
@@ -291,19 +293,8 @@ bool NeedRestartToApplyPerSessionFlags(
   if (user_manager::UserManager::Get()->IsLoggedInAsSupervisedUser())
     return false;
 
-  // TODO: Remove this special handling for site isolation and isolate origins.
   auto* current_command_line = base::CommandLine::ForCurrentProcess();
-  if (current_command_line->HasSwitch(::switches::kSitePerProcess) !=
-      user_flags.HasSwitch(::switches::kSitePerProcess)) {
-    out_command_line_difference->insert(::switches::kSitePerProcess);
-  }
-  if (current_command_line->GetSwitchValueASCII(::switches::kIsolateOrigins) !=
-      user_flags.GetSwitchValueASCII(::switches::kIsolateOrigins)) {
-    out_command_line_difference->insert(::switches::kIsolateOrigins);
-  }
-
-  if (out_command_line_difference->empty() &&
-      about_flags::AreSwitchesIdenticalToCurrentCommandLine(
+  if (about_flags::AreSwitchesIdenticalToCurrentCommandLine(
           user_flags, *current_command_line, out_command_line_difference)) {
     return false;
   }
@@ -352,10 +343,16 @@ void LogCustomSwitches(const std::set<std::string>& switches) {
   }
 }
 
-void RestartOnTimeout() {
+// Calls the real AttemptRestart method. This is used to avoid taking a function
+// pointer to chrome::AttemptRestart directly.
+void CallChromeAttemptRestart() {
+  chrome::AttemptRestart();
+}
+
+void RestartOnTimeout(const base::RepeatingClosure& attempt_restart_closure) {
   LOG(WARNING) << "Restarting Chrome because the time out was reached."
                   "The session restore has not finished.";
-  chrome::AttemptRestart();
+  attempt_restart_closure.Run();
 }
 
 bool IsRunningTest() {
@@ -406,17 +403,49 @@ void UserSessionManager::RegisterPrefs(PrefRegistrySimple* registry) {
 
 // static
 void UserSessionManager::MaybeAppendPolicySwitches(
+    PrefService* user_profile_prefs,
     base::CommandLine* user_flags) {
+  // Get target values for --site-per-process and --isolate-origins for the user
+  // session according to policy. Values from command-line flags should not be
+  // honored at this point, so check |IsManaged()|.
+  const PrefService::Preference* site_per_process_pref =
+      user_profile_prefs->FindPreference(prefs::kSitePerProcess);
+  const PrefService::Preference* isolate_origins_pref =
+      user_profile_prefs->FindPreference(prefs::kIsolateOrigins);
+  bool site_per_process = site_per_process_pref->IsManaged() &&
+                          site_per_process_pref->GetValue()->GetBool();
+  std::string isolate_origins =
+      isolate_origins_pref->IsManaged()
+          ? isolate_origins_pref->GetValue()->GetString()
+          : std::string();
+
+  // Append sentinels indicating that these values originate from policy.
+  // This is important, because only command-line switches between the
+  // |"--policy-switches-begin"| / |"--policy-switches-end"| and the
+  // |"--flag-switches-begin"| / |"--flag-switches-end"| sentinels will be
+  // compared when comparing the current command line and the user session
+  // command line in order to decide if chrome should be restarted.
+  // We use the policy-style sentinels because these values originate from
+  // policy, and because login_manager uses the same sentinels when adding the
+  // login-screen site isolation flags.
+  bool use_policy_sentinels = site_per_process || !isolate_origins.empty();
+  if (use_policy_sentinels)
+    user_flags->AppendSwitch(chromeos::switches::kPolicySwitchesBegin);
+
   // Inject site isolation and isolate origins command line switch from
   // user policy.
-  auto* local_state = g_browser_process->local_state();
-  if (local_state->GetBoolean(prefs::kSitePerProcess)) {
+  if (site_per_process) {
     user_flags->AppendSwitch(::switches::kSitePerProcess);
   }
-  if (local_state->HasPrefPath(prefs::kIsolateOrigins)) {
+
+  if (!isolate_origins.empty()) {
     user_flags->AppendSwitchASCII(
         ::switches::kIsolateOrigins,
-        local_state->GetString(prefs::kIsolateOrigins));
+        user_profile_prefs->GetString(prefs::kIsolateOrigins));
+  }
+
+  if (use_policy_sentinels) {
+    user_flags->AppendSwitch(chromeos::switches::kPolicySwitchesEnd);
   }
 }
 
@@ -433,6 +462,7 @@ UserSessionManager::UserSessionManager()
       should_obtain_handles_(true),
       should_launch_browser_(true),
       waiting_for_child_account_status_(false),
+      attempt_restart_closure_(base::BindRepeating(&CallChromeAttemptRestart)),
       weak_factory_(this) {
   net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
   user_manager::UserManager::Get()->AddSessionStateObserver(this);
@@ -459,6 +489,11 @@ void UserSessionManager::SetShouldObtainHandleInTests(
   }
 }
 
+void UserSessionManager::SetAttemptRestartClosureInTests(
+    const base::RepeatingClosure& attempt_restart_closure) {
+  attempt_restart_closure_ = attempt_restart_closure;
+}
+
 void UserSessionManager::CompleteGuestSessionLogin(const GURL& start_url) {
   VLOG(1) << "Completing guest session login";
 
@@ -826,7 +861,7 @@ bool UserSessionManager::RestartToApplyPerSessionFlagsIfNeed(
       cryptohome::Identification(
           user_manager::UserManager::Get()->GetActiveUser()->GetAccountId()),
       flags);
-  AttemptRestart(profile);
+  attempt_restart_closure_.Run();
   return true;
 }
 
@@ -915,7 +950,7 @@ void UserSessionManager::OnSessionRestoreStateChanged(
 
     // We need to restart cleanly in this case to make sure OAuth2 RT is
     // actually saved.
-    chrome::AttemptRestart();
+    attempt_restart_closure_.Run();
   } else {
     // Schedule another flush after session restore for non-ephemeral profile
     // if not restarting.
@@ -1762,7 +1797,7 @@ void UserSessionManager::AttemptRestart(Profile* profile) {
   // Restart unconditionally in case if we are stuck somewhere in a session
   // restore process. http://crbug.com/520346.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::BindOnce(RestartOnTimeout),
+      FROM_HERE, base::BindOnce(RestartOnTimeout, attempt_restart_closure_),
       base::TimeDelta::FromSeconds(kMaxRestartDelaySeconds));
 
   if (running_easy_unlock_key_ops_) {
@@ -1773,7 +1808,7 @@ void UserSessionManager::AttemptRestart(Profile* profile) {
 
   if (session_restore_strategy_ !=
       OAuth2LoginManager::RESTORE_FROM_COOKIE_JAR) {
-    chrome::AttemptRestart();
+    attempt_restart_closure_.Run();
     return;
   }
 
@@ -1784,7 +1819,7 @@ void UserSessionManager::AttemptRestart(Profile* profile) {
   if (login_manager->state() != OAuth2LoginManager::SESSION_RESTORE_PREPARING &&
       login_manager->state() !=
           OAuth2LoginManager::SESSION_RESTORE_IN_PROGRESS) {
-    chrome::AttemptRestart();
+    attempt_restart_closure_.Run();
     return;
   }
 

chrome/browser/chromeos/login/session/user_session_manager.h

@@ -34,6 +34,7 @@
 class AccountId;
 class GURL;
 class PrefRegistrySimple;
+class PrefService;
 class Profile;
 class TokenHandleFetcher;
 
@@ -120,7 +121,8 @@ class UserSessionManager
 
   // Appends additional command switches to the given command line if
   // SitePerProcess/IsolateOrigins policy is present.
-  static void MaybeAppendPolicySwitches(base::CommandLine* user_flags);
+  static void MaybeAppendPolicySwitches(PrefService* user_profile_prefs,
+                                        base::CommandLine* user_flags);
 
   // Invoked after the tmpfs is successfully mounted.
   // Asks session_manager to restart Chrome in Guest session mode.
@@ -428,6 +430,10 @@ class UserSessionManager
   // Controls whether token handle fetching is enabled (used in tests).
   void SetShouldObtainHandleInTests(bool should_obtain_handles);
 
+  // Sets the function which is used to request a chrome restart.
+  void SetAttemptRestartClosureInTests(
+      const base::RepeatingClosure& attempt_restart_closure);
+
   // The user pods display type for histogram.
   enum UserPodsDisplay {
     // User pods enabling or disabling is possible either via local settings or
@@ -543,6 +549,9 @@ class UserSessionManager
 
   std::vector<base::OnceClosure> easy_unlock_key_ops_finished_callbacks_;
 
+  // Mapped to |chrome::AttemptRestart|, except in tests.
+  base::RepeatingClosure attempt_restart_closure_;
+
   base::WeakPtrFactory<UserSessionManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(UserSessionManager);

chrome/browser/chromeos/login/session/user_session_manager_test_api.cc

@@ -26,5 +26,10 @@ void UserSessionManagerTestApi::SetShouldObtainTokenHandleInTests(
   session_manager_->SetShouldObtainHandleInTests(should_obtain_handle);
 }
 
+void UserSessionManagerTestApi::SetAttemptRestartClosureInTests(
+    const base::RepeatingClosure& attempt_restart_closure) {
+  session_manager_->SetAttemptRestartClosureInTests(attempt_restart_closure);
+}
+
 }  // namespace test
 }  // namespace chromeos

chrome/browser/chromeos/login/session/user_session_manager_test_api.h

@@ -27,6 +27,10 @@ class UserSessionManagerTestApi {
   // Controls whether token handle fetching is enabled (used in tests).
   void SetShouldObtainTokenHandleInTests(bool should_obtain_handle);
 
+  // Sets the function which is used to request a chrome restart.
+  void SetAttemptRestartClosureInTests(
+      const base::RepeatingClosure& attempt_restart_closure);
+
  private:
   UserSessionManager* session_manager_;  // not owned
 

chrome/browser/ui/webui/flags_ui.cc

@@ -223,7 +223,8 @@ void FlagsDOMHandler::HandleRestartBrowser(const base::ListValue* args) {
 
   // Apply additional switches from policy that should not be dropped when
   // applying flags..
-  chromeos::UserSessionManager::MaybeAppendPolicySwitches(&user_flags);
+  chromeos::UserSessionManager::MaybeAppendPolicySwitches(
+      Profile::FromWebUI(web_ui())->GetPrefs(), &user_flags);
 
   base::CommandLine::StringVector flags;
   // argv[0] is the program name |base::CommandLine::NO_PROGRAM|.

chrome/test/BUILD.gn

@@ -1603,6 +1603,7 @@ test("browser_tests") {
         "../browser/chromeos/policy/power_policy_browsertest.cc",
         "../browser/chromeos/policy/restore_on_startup_browsertest_chromeos.cc",
         "../browser/chromeos/policy/signin_profile_apps_policy_browsertest.cc",
+        "../browser/chromeos/policy/site_isolation_flag_handling_browsertest.cc",
         "../browser/chromeos/policy/unaffiliated_arc_allowed_browsertest.cc",
         "../browser/chromeos/policy/user_affiliation_browsertest.cc",
         "../browser/chromeos/policy/user_cloud_external_data_manager_browsertest.cc",

chromeos/dbus/fake_session_manager_client.cc

@@ -457,12 +457,14 @@ void FakeSessionManagerClient::StoreDeviceLocalAccountPolicy(
 }
 
 bool FakeSessionManagerClient::SupportsRestartToApplyUserFlags() const {
-  return false;
+  return supports_restart_to_apply_user_flags_;
 }
 
 void FakeSessionManagerClient::SetFlagsForUser(
     const cryptohome::Identification& cryptohome_id,
-    const std::vector<std::string>& flags) {}
+    const std::vector<std::string>& flags) {
+  flags_for_user_[cryptohome_id] = flags;
+}
 
 void FakeSessionManagerClient::GetServerBackedStateKeys(
     StateKeysCallback callback) {
@@ -566,6 +568,17 @@ void FakeSessionManagerClient::NotifyArcInstanceStopped(
     observer.ArcInstanceStopped(clean, container_instance_id);
 }
 
+bool FakeSessionManagerClient::GetFlagsForUser(
+    const cryptohome::Identification& cryptohome_id,
+    std::vector<std::string>* out_flags_for_user) const {
+  auto iter = flags_for_user_.find(cryptohome_id);
+  if (iter == flags_for_user_.end())
+    return false;
+
+  *out_flags_for_user = iter->second;
+  return true;
+}
+
 const std::string& FakeSessionManagerClient::device_policy() const {
   return device_policy_;
 }

chromeos/dbus/fake_session_manager_client.h

@@ -101,6 +101,21 @@ class FakeSessionManagerClient : public SessionManagerClient {
   void NotifyArcInstanceStopped(bool clean,
                                 const std::string& conainer_instance_id);
 
+  // Returns true if flags for |cryptohome_id| have been set. If the return
+  // value is |true|, |*out_flags_for_user| is filled with the flags passed to
+  // |SetFlagsForUser|.
+  bool GetFlagsForUser(const cryptohome::Identification& cryptohome_id,
+                       std::vector<std::string>* out_flags_for_user) const;
+
+  // Sets whether FakeSessionManagerClient should advertise (through
+  // |SupportsRestartToApplyUserFlags|) that it supports restarting chrome to
+  // apply user-session flags. The default is |false|.
+  void set_supports_restart_to_apply_user_flags(
+      bool supports_restart_to_apply_user_flags) {
+    supports_restart_to_apply_user_flags_ =
+        supports_restart_to_apply_user_flags;
+  }
+
   void set_store_device_policy_success(bool success) {
     store_device_policy_success_ = success;
   }
@@ -167,6 +182,8 @@ class FakeSessionManagerClient : public SessionManagerClient {
   }
 
  private:
+  bool supports_restart_to_apply_user_flags_ = false;
+
   bool store_device_policy_success_ = true;
   std::string device_policy_;
   std::map<cryptohome::Identification, std::string> user_policies_;
@@ -203,6 +220,10 @@ class FakeSessionManagerClient : public SessionManagerClient {
   // multiple FakeSessionManagerOptions.
   uint32_t options_;
 
+  // The last-set flags for user set through |SetFlagsForUser|.
+  std::map<cryptohome::Identification, std::vector<std::string>>
+      flags_for_user_;
+
   base::WeakPtrFactory<FakeSessionManagerClient> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(FakeSessionManagerClient);
 };