Track whether a given user session has completed initialization

Start tracking user session initialization in local state and use
that flag to determine whether to force a policy load instead of relying
on the oauth token status. This is important because forcing a policy load
when it's not strictly required can lock users out of their accounts.

This flag is not persisted across restarts of ephemeral sessions (including crash-induced restarts) so policy code will force a policy reload after a crash (identical to previous ephemeral behavior, so no behavior change from this CL)

BUG=684031

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

chrome/browser/chromeos/login/screens/user_selection_screen.cc

@@ -277,7 +277,11 @@ bool UserSelectionScreen::ShouldForceOnlineSignIn(
   if (token_status == user_manager::User::OAUTH2_TOKEN_STATUS_INVALID)
     RecordReauthReason(user->GetAccountId(), ReauthReason::OTHER);
 
-  return user->force_online_signin() ||
+  // We need to force an online signin if the user is marked as requiring it,
+  // or if the user's session never completed initialization (still need to
+  // check for policy/management state) or if there's an invalid OAUTH token
+  // that needs to be refreshed.
+  return user->force_online_signin() || !user->profile_ever_initialized() ||
          (token_status == user_manager::User::OAUTH2_TOKEN_STATUS_INVALID) ||
          (token_status == user_manager::User::OAUTH_TOKEN_STATUS_UNKNOWN);
 }

chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc

@@ -197,6 +197,10 @@ const AccountId& FakeChromeUserManager::GetOwnerAccountId() const {
 
 void FakeChromeUserManager::OnSessionStarted() {}
 
+void FakeChromeUserManager::OnProfileInitialized(user_manager::User* user) {
+  user->set_profile_ever_initialized(true);
+}
+
 void FakeChromeUserManager::RemoveUser(
     const AccountId& account_id,
     user_manager::RemoveUserDelegate* delegate) {}

chrome/browser/chromeos/login/users/fake_chrome_user_manager.h

@@ -55,6 +55,7 @@ class FakeChromeUserManager : public ChromeUserManager {
   void SwitchActiveUser(const AccountId& account_id) override;
   void SwitchToLastActiveUser() override;
   void OnSessionStarted() override;
+  void OnProfileInitialized(user_manager::User* user) override;
   void RemoveUser(const AccountId& account_id,
                   user_manager::RemoveUserDelegate* delegate) override;
   void RemoveUserFromList(const AccountId& account_id) override;

chrome/browser/chromeos/login/users/user_manager_unittest.cc

@@ -26,6 +26,7 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "components/prefs/pref_service.h"
+#include "components/user_manager/known_user.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/common/content_switches.h"
@@ -240,4 +241,36 @@ TEST_F(UserManagerTest, ScreenLockAvailability) {
   ResetUserManager();
 }
 
+TEST_F(UserManagerTest, ProfileInitialized) {
+  user_manager::UserManager::Get()->UserLoggedIn(
+      owner_account_id_at_invalid_domain_,
+      owner_account_id_at_invalid_domain_.GetUserEmail(), false);
+  const user_manager::UserList* users =
+      &user_manager::UserManager::Get()->GetUsers();
+  ASSERT_EQ(1U, users->size());
+  EXPECT_FALSE((*users)[0]->profile_ever_initialized());
+  ResetUserManager();
+  users = &user_manager::UserManager::Get()->GetUsers();
+  ASSERT_EQ(1U, users->size());
+  EXPECT_FALSE((*users)[0]->profile_ever_initialized());
+}
+
+TEST_F(UserManagerTest, ProfileInitializedMigration) {
+  user_manager::UserManager::Get()->UserLoggedIn(
+      owner_account_id_at_invalid_domain_,
+      owner_account_id_at_invalid_domain_.GetUserEmail(), false);
+  const user_manager::UserList* users =
+      &user_manager::UserManager::Get()->GetUsers();
+  ASSERT_EQ(1U, users->size());
+  EXPECT_FALSE((*users)[0]->profile_ever_initialized());
+
+  // Clear the stored user data - when UserManager loads again, it should
+  // migrate existing users by setting session_initialized to true for them.
+  user_manager::known_user::RemovePrefsForTesting((*users)[0]->GetAccountId());
+  ResetUserManager();
+  users = &user_manager::UserManager::Get()->GetUsers();
+  ASSERT_EQ(1U, users->size());
+  EXPECT_TRUE((*users)[0]->profile_ever_initialized());
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc

@@ -199,17 +199,14 @@ UserPolicyManagerFactoryChromeOS::CreateManagerForProfile(
   const user_manager::UserManager* const user_manager =
       user_manager::UserManager::Get();
 
-  // We want to block for policy in a few situations: if the user is new, or if
-  // we are forcing an online signin. An online signin will be forced if there
-  // has been a credential error, or if the initial session creation was not
-  // completed (the oauth_token_status is not set to valid by OAuth2LoginManager
-  // until profile creation/session restore is complete).
+  // We want to block for policy if the session has never been initialized
+  // (generally true if the user is new, or if there was a crash before the
+  // profile finished initializing). There is code in UserSelectionScreen to
+  // force an online signin for uninitialized sessions to help ensure we are
+  // able to load policy.
   const bool block_forever_for_policy =
       !user_manager->IsLoggedInAsStub() &&
-      (user_manager->IsCurrentUserNew() ||
-       user_manager->GetActiveUser()->force_online_signin() ||
-       user_manager->GetActiveUser()->oauth_token_status() !=
-           user_manager::User::OAUTH2_TOKEN_STATUS_VALID);
+      !user_manager->GetActiveUser()->profile_ever_initialized();
 
   const bool wait_for_policy_fetch =
       block_forever_for_policy || !is_browser_restart;

chrome/browser/profiles/profile_browsertest.cc

@@ -350,6 +350,13 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, CreateNewProfileSynchronous) {
     std::unique_ptr<Profile> profile(CreateProfile(
         temp_dir.GetPath(), &delegate, Profile::CREATE_MODE_SYNCHRONOUS));
     CheckChromeVersion(profile.get(), true);
+
+#if defined(OS_CHROMEOS)
+    // Make sure session is marked as initialized.
+    user_manager::User* user =
+        chromeos::ProfileHelper::Get()->GetUserByProfile(profile.get());
+    EXPECT_TRUE(user->profile_ever_initialized());
+#endif
   }
 
   FlushIoTaskRunnerAndSpinThreads();
@@ -396,6 +403,12 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
     // Wait for the profile to be created.
     observer.Wait();
     CheckChromeVersion(profile.get(), true);
+#if defined(OS_CHROMEOS)
+    // Make sure session is marked as initialized.
+    user_manager::User* user =
+        chromeos::ProfileHelper::Get()->GetUserByProfile(profile.get());
+    EXPECT_TRUE(user->profile_ever_initialized());
+#endif
   }
 
   FlushIoTaskRunnerAndSpinThreads();

chrome/browser/profiles/profile_impl.cc

@@ -644,6 +644,16 @@ void ProfileImpl::DoFinalInit() {
   dom_distiller::RegisterViewerSource(this);
 
 #if defined(OS_CHROMEOS)
+  // Finished profile initialization - let the UserManager know so it can
+  // mark the session as initialized. Need to do this before we restart below
+  // so we don't get in a weird state where we restart before the session is
+  // marked as initialized and so try to initialize it again.
+  if (!chromeos::ProfileHelper::IsSigninProfile(this)) {
+    chromeos::ProfileHelper* profile_helper = chromeos::ProfileHelper::Get();
+    user_manager::UserManager::Get()->OnProfileInitialized(
+        profile_helper->GetUserByProfile(this));
+  }
+
   if (chromeos::UserSessionManager::GetInstance()
           ->RestartToApplyPerSessionFlagsIfNeed(this, true)) {
     return;

components/user_manager/fake_user_manager.cc

@@ -54,6 +54,10 @@ const user_manager::User* FakeUserManager::AddUserWithAffiliation(
   return user;
 }
 
+void FakeUserManager::OnProfileInitialized(User* user) {
+  user->set_profile_ever_initialized(true);
+}
+
 void FakeUserManager::RemoveUserFromList(const AccountId& account_id) {
   const user_manager::UserList::iterator it =
       std::find_if(users_.begin(), users_.end(),

components/user_manager/fake_user_manager.h

@@ -64,6 +64,7 @@ class USER_MANAGER_EXPORT FakeUserManager : public UserManagerBase {
   user_manager::UserList GetUnlockUsers() const override;
   const AccountId& GetOwnerAccountId() const override;
   void OnSessionStarted() override {}
+  void OnProfileInitialized(User* user) override;
   void RemoveUser(const AccountId& account_id,
                   user_manager::RemoveUserDelegate* delegate) override {}
   void RemoveUserFromList(const AccountId& account_id) override;

components/user_manager/known_user.cc

@@ -53,6 +53,9 @@ const char kReauthReasonKey[] = "reauth_reason";
 // Key for the GaiaId migration status.
 const char kGaiaIdMigration[] = "gaia_id_migration";
 
+// Key of the boolean flag telling if user session has finished init yet.
+const char kProfileEverInitialized[] = "profile_ever_initialized";
+
 PrefService* GetLocalState() {
   if (!UserManager::IsInitialized())
     return nullptr;
@@ -451,6 +454,22 @@ bool IsUsingSAML(const AccountId& account_id) {
   return false;
 }
 
+bool WasProfileEverInitialized(const AccountId& account_id) {
+  bool profile_ever_initialized;
+  if (GetBooleanPref(account_id, kProfileEverInitialized,
+                     &profile_ever_initialized))
+    return profile_ever_initialized;
+
+  // Sessions created before we started setting the session_initialized flag
+  // should default to "initialized = true".
+  LOG(WARNING) << "Treating unmigrated user as profile_ever_initialized=true";
+  return true;
+}
+
+void SetProfileEverInitialized(const AccountId& account_id, bool initialized) {
+  SetBooleanPref(account_id, kProfileEverInitialized, initialized);
+}
+
 void UpdateReauthReason(const AccountId& account_id, const int reauth_reason) {
   SetIntegerPref(account_id, kReauthReasonKey, reauth_reason);
 }
@@ -478,6 +497,11 @@ void RemovePrefs(const AccountId& account_id) {
   }
 }
 
+// Exported so tests can call this from other components.
+void RemovePrefsForTesting(const AccountId& account_id) {
+  RemovePrefs(account_id);
+}
+
 void RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(kKnownUsers);
 }

components/user_manager/known_user.h

@@ -127,6 +127,17 @@ void USER_MANAGER_EXPORT UpdateUsingSAML(const AccountId& account_id,
 // returns false.
 bool USER_MANAGER_EXPORT IsUsingSAML(const AccountId& account_id);
 
+// Returns true if the user's session has already completed initialization
+// (set to false when session is created, and then is set to true once
+// the profile is intiaiized - this allows us to detect crashes/restarts during
+// initial session creation so we can recover gracefully).
+bool USER_MANAGER_EXPORT WasProfileEverInitialized(const AccountId& account_id);
+
+// Sets the flag that denotes whether the session associated with a user has
+// completed initialization at least once.
+void USER_MANAGER_EXPORT SetProfileEverInitialized(const AccountId& account_id,
+                                                   bool initialized);
+
 // Saves why the user has to go through re-auth flow.
 void USER_MANAGER_EXPORT UpdateReauthReason(const AccountId& account_id,
                                             const int reauth_reason);
@@ -138,8 +149,10 @@ bool USER_MANAGER_EXPORT FindReauthReason(const AccountId& account_id,
                                           int* out_value);
 
 // Removes all user preferences associated with |account_id|.
-// (This one used by user_manager only and thus not exported.)
+// Not exported as code should not be calling this outside this component
+// (with the exception of tests, so a test-only API is exposed).
 void RemovePrefs(const AccountId& account_id);
+void USER_MANAGER_EXPORT RemovePrefsForTesting(const AccountId& account_id);
 
 // Register known user prefs.
 void USER_MANAGER_EXPORT RegisterPrefs(PrefRegistrySimple* registry);

components/user_manager/user.h

@@ -171,6 +171,9 @@ class USER_MANAGER_EXPORT User : public UserInfo {
   // user's next sign-in.
   bool force_online_signin() const { return force_online_signin_; }
 
+  // Whether the user's session has completed initialization yet.
+  bool profile_ever_initialized() const { return profile_ever_initialized_; }
+
   // True if the user's session can be locked (i.e. the user has a password with
   // which to unlock the session).
   bool can_lock() const;
@@ -200,6 +203,7 @@ class USER_MANAGER_EXPORT User : public UserInfo {
   friend class chromeos::MockUserManager;
   friend class chromeos::UserAddingScreenTest;
   FRIEND_TEST_ALL_PREFIXES(UserTest, DeviceLocalAccountAffiliation);
+  FRIEND_TEST_ALL_PREFIXES(UserTest, UserSessionInitialized);
 
   // Do not allow anyone else to create new User instances.
   static User* CreateRegularUser(const AccountId& account_id);
@@ -249,6 +253,10 @@ class USER_MANAGER_EXPORT User : public UserInfo {
     force_online_signin_ = force_online_signin;
   }
 
+  void set_profile_ever_initialized(bool profile_ever_initialized) {
+    profile_ever_initialized_ = profile_ever_initialized;
+  }
+
   void set_username_hash(const std::string& username_hash) {
     username_hash_ = username_hash;
   }
@@ -276,6 +284,7 @@ class USER_MANAGER_EXPORT User : public UserInfo {
   std::unique_ptr<UserImage> user_image_;
   OAuthTokenStatus oauth_token_status_ = OAUTH_TOKEN_STATUS_UNKNOWN;
   bool force_online_signin_ = false;
+  bool profile_ever_initialized_ = false;
 
   // This is set to chromeos locale if account data has been downloaded.
   // (Or failed to download, but at least one download attempt finished).

components/user_manager/user_manager.h

@@ -175,6 +175,14 @@ class USER_MANAGER_EXPORT UserManager {
   // Invoked by session manager to inform session start.
   virtual void OnSessionStarted() = 0;
 
+  // Invoked once profile initialization has been completed. This allows various
+  // subsystems (for example, policy framework) to skip an expensive online
+  // initialization process, and also allows the signin screen to force an
+  // online signin if it knows that profile initialization has not yet
+  // completed. |user| is the User associated with the profile that has
+  // completed initialization.
+  virtual void OnProfileInitialized(User* user) = 0;
+
   // Removes the user from the device. Note, it will verify that the given user
   // isn't the owner, so calling this method for the owner will take no effect.
   // Note, |delegate| can be NULL.

components/user_manager/user_manager_base.cc

@@ -264,6 +264,16 @@ void UserManagerBase::OnSessionStarted() {
   GetLocalState()->CommitPendingWrite();
 }
 
+void UserManagerBase::OnProfileInitialized(User* user) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  // Mark the user as having an initialized session and persist this in
+  // the known_user DB.
+  user->set_profile_ever_initialized(true);
+  known_user::SetProfileEverInitialized(user->GetAccountId(), true);
+  GetLocalState()->CommitPendingWrite();
+}
+
 void UserManagerBase::RemoveUser(const AccountId& account_id,
                                  RemoveUserDelegate* delegate) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
@@ -799,6 +809,8 @@ void UserManagerBase::EnsureUsersLoaded() {
     const AccountId account_id = user->GetAccountId();
     user->set_oauth_token_status(LoadUserOAuthStatus(*it));
     user->set_force_online_signin(LoadForceOnlineSignin(*it));
+    user->set_profile_ever_initialized(
+        known_user::WasProfileEverInitialized(*it));
     user->set_using_saml(known_user::IsUsingSAML(*it));
     users_.push_back(user);
 
@@ -820,7 +832,6 @@ void UserManagerBase::EnsureUsersLoaded() {
       user->set_display_email(display_email);
     }
   }
-
   user_loading_stage_ = STAGE_LOADED;
 
   PerformPostUserListLoadingActions();
@@ -883,6 +894,8 @@ void UserManagerBase::RegularUserLoggedIn(const AccountId& account_id) {
     active_user_->set_oauth_token_status(LoadUserOAuthStatus(account_id));
     SaveUserDisplayName(active_user_->GetAccountId(),
                         base::UTF8ToUTF16(active_user_->GetAccountName(true)));
+    known_user::SetProfileEverInitialized(
+        active_user_->GetAccountId(), active_user_->profile_ever_initialized());
   }
 
   AddUserRecord(active_user_);

components/user_manager/user_manager_base.h

@@ -56,6 +56,7 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
   void SwitchActiveUser(const AccountId& account_id) override;
   void SwitchToLastActiveUser() override;
   void OnSessionStarted() override;
+  void OnProfileInitialized(User* user) override;
   void RemoveUser(const AccountId& account_id,
                   RemoveUserDelegate* delegate) override;
   void RemoveUserFromList(const AccountId& account_id) override;
@@ -311,6 +312,10 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
   // be enforced during the user's next sign-in from local state preferences.
   bool LoadForceOnlineSignin(const AccountId& account_id) const;
 
+  // Read a flag indicating whether session initialization has completed at
+  // least once.
+  bool LoadSessionInitialized(const AccountId& account_id) const;
+
   // Notifies observers that merge session state had changed.
   void NotifyMergeSessionStateChanged();
 

components/user_manager/user_unittest.cc

@@ -44,4 +44,12 @@ TEST(UserTest, DeviceLocalAccountAffiliation) {
   EXPECT_TRUE(arc_kiosk_user.IsAffiliated());
 }
 
+TEST(UserTest, UserSessionInitialized) {
+  const AccountId account_id = AccountId::FromUserEmailGaiaId(kEmail, kGaiaId);
+  std::unique_ptr<User> user(User::CreateRegularUser(account_id));
+  EXPECT_FALSE(user->profile_ever_initialized());
+  user->set_profile_ever_initialized(true);
+  EXPECT_TRUE(user->profile_ever_initialized());
+}
+
 }  // namespace user_manager