Track whether a given user session has completed initialization, and use

components/user_manager/user_manager_base.cc

Index: components/user_manager/user_manager_base.cc
diff --git a/components/user_manager/user_manager_base.cc b/components/user_manager/user_manager_base.cc
index 30fe4559b1880c57b96b9c7797f508bcfc3b24c6..30d0c29147c9375fab39e72c4efa8c10d058b0ab 100644
--- a/components/user_manager/user_manager_base.cc
+++ b/components/user_manager/user_manager_base.cc
@@ -54,6 +54,13 @@ const char kUserOAuthTokenStatus[] = "OAuthTokenStatus";
 // authentication against GAIA should be enforced during the next sign-in.
 const char kUserForceOnlineSignin[] = "UserForceOnlineSignin";
 
+// A dictionary that maps user IDs to a flag indicating whether session
+// initialization has been completed at least once on this session. If the
+// flag is set to false (or the user ID is missing from the dictionary) then
+// the session did not complete initialization and the next signin should
+// require a network connection (for example, to register for policy).
+const char kUserSessionInitialized[] = "UserSessionInitialized";

[emaxx, 2017/02/23 21:45:25]

To me, the name sounds a bit misleading: the flag is not actually talking about
the current active _sessions_, but it's meaning whether the _profile_ has been
fully initialized.
Maybe something like "UserProfileFullyInitialized" would be more precise?

The same concern applies to the User and the UserManager methods: it's far from
obvious to understand that the flag is not transient and is not scoped to the
current session, but is persistent and stays here forever.

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

Yeah, the problem is that profile initialization is something different - it
refers to the initialization of an actual object (e.g. completion of
ProfileImpl::DoFinalInit()) in the currently running instance of the session. So
it feels like talking about profile initalization (esp since
User::is_profile_created() already exists and is transient/not persisted) would
be the wrong choice.

Maybe "session" is the wrong term to use, but I can't think of a better one.
WDYT?

[emaxx, 2017/02/24 17:54:09]

But the notification to set this new flag is set exactly from
ProfileImpl::DoFinalInit(), so I believe the word "profile" is very much
justified here.

However I agree with your concern that the persistence of the flag needs to be
shown more explicitly.

Maybe something like "UserProfileEverInitialized" or
"UserProfileWasInitializedOnce"? Lengthy, but gives more clue about the actual
meaning.

I'm just personally feeling strongly against the current name
"UserSessionInitialized", because it sounds not like it actually means, and may
be easily misused.

+
 // A dictionary that maps user ID to the user type.
 const char kUserType[] = "UserType";
 
@@ -82,6 +89,7 @@ void UserManagerBase::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(kUserDisplayEmail);
   registry->RegisterDictionaryPref(kUserOAuthTokenStatus);
   registry->RegisterDictionaryPref(kUserForceOnlineSignin);
+  registry->RegisterDictionaryPref(kUserSessionInitialized);
   registry->RegisterDictionaryPref(kUserType);
   registry->RegisterStringPref(kLastActiveUser, std::string());
 
@@ -263,6 +271,26 @@ void UserManagerBase::OnSessionStarted() {
   GetLocalState()->CommitPendingWrite();
 }
 
+void UserManagerBase::OnSessionInitialized(User* user) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  // Mark the user as having an initialized session.
+  user->set_session_initialized(true);
+
+  // Do not update local state if data stored or cached outside the user's

[emaxx, 2017/02/23 21:45:25]

I'm curious how then is the flag preserved for the ephemeral users across
restarts. Is it transmitted from the crashed session differently somehow?

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

Great catch. I suspect the way to deal with this is to store the pref in two
places - local state and in the user cryptohome. I'll tackle this in a followup
CL since the behavior for ephemeral users will be identical to the current
behavior (treat them as new, so force a policy load).

[emaxx, 2017/02/24 17:54:09]

Could you please drop a note regarding ephemeral users in the CL description?

+  // cryptohome is to be treated as ephemeral.
+  if (IsUserNonCryptohomeDataEphemeral(user->GetAccountId()))
+    return;
+
+  {
+    DictionaryPrefUpdate user_session_initialized(GetLocalState(),
+                                                  kUserSessionInitialized);
+    user_session_initialized->SetBooleanWithoutPathExpansion(

[Alexander Alekseev, 2017/02/24 09:58:11]

We have known_user database for this.
Could you use it? It will also save the problem with user IDs.

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

Done.

+        user->GetAccountId().GetUserEmail(), true);

[emaxx, 2017/02/23 21:45:25]

The docs for AccountId prescribe to use GetAccountIdKey() instead of
GetUserEmail() for the purpose of keying in a storage.
I don't know all the reasoning behind that (supporting changing e-mail?
ambiguous serialization? distinguishing the Active Directory case from GAIA?).
Perhaps better ask OWNERS for this.

Also I know that GetAccountIdKey() currently crashes in some cases (when only
e-mail, not id, was specified to the constructor). IIRC, at least for the guest
users. So maybe it's still not ready to be used widely.

But at least worth looking into this.

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

I'm now just using known_user, so I think we're OK.

+  }
+  GetLocalState()->CommitPendingWrite();
+}
+
 void UserManagerBase::RemoveUser(const AccountId& account_id,
                                  RemoveUserDelegate* delegate) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
@@ -782,8 +810,24 @@ void UserManagerBase::EnsureUsersLoaded() {
   std::set<AccountId> regular_users_set;
   ParseUserList(*prefs_regular_users, device_local_accounts_set, &regular_users,
                 &regular_users_set);
+
+  // If there is no stored SessionInitialized state, this either means this is
+  // the first boot on a new device or this is a migration from an older device
+  // that was not tracking SessionInitialized state. If there are already users

[emaxx, 2017/02/23 21:45:25]

It's not clear how this will work in the following scenario: new device with no
existing users, a new user is added and starts logging in, then the crash
happens before the profile gets fully initialized.
If the pref doesn't get stored (with an empty value) by that point, then the
"migration" process would start erroneously.

Even if that's not the case and everything works correctly, it's still worth
adding a comment on that. And, ideally, a browser test.

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

I don't think this can really happen - very very early in session initialization
(RegularUserLoggedIn()) we explicitly set the value to false and commit this to
disk.

Not really clear how to add a browser test for a crash
mid-session-initialization.

[emaxx, 2017/02/24 17:54:09]

Acknowledged. And looks like the existing tests verify this to some extent.

+  // on the device then we know this is a migration, so we will mark all of
+  // the sessions as initialized.
+  const bool mark_user_sessions_initialized =

[emaxx, 2017/02/23 21:45:25]

Maybe reorganize the migration code so that it forms a single separate block
that is easier to follow? Like:

for (... regular_users ...) {
   ... old stuff ...
}

if (!local_state->HasPrefPath(kUserSessionInitialized)) {
  DictionaryPrefUpdate user_session_initialized(
      GetLocalState(), kUserSessionInitialized);
  for (... users ...) {
    LOG...
    user_session_initialized->SetBooleanWithoutPathExpansion...;
    user->set_session_initialized(true);
  }
  local_state->CommitPendingWrite();
}

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

Moved migration code to known_user::IsSessionInitialized().

+      !local_state->HasPrefPath(kUserSessionInitialized);
+
   for (std::vector<AccountId>::const_iterator it = regular_users.begin();
        it != regular_users.end(); ++it) {
+    if (mark_user_sessions_initialized) {
+      LOG(WARNING) << "Migrating existing user to session_initialized=true";

[emaxx, 2017/02/23 21:45:25]

nit: Maybe also log something that specifies the user being migrated, like their
e-mail?

[emaxx, 2017/02/23 21:45:25]

nit: Isn't the INFO level better here? Because this code branch isn't about any
errors or unusual behavior - it's a normal procedure for all devices used before
your change.

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

Yes, but should be a one-off per user. So it's a warning because shortly after
we roll this out, this should never be seen again (once every session has signed
in once).

[Andrew T Wilson (Slow), 2017/02/24 16:15:15]

Intentionally not doing this since I believe logging usernames in system logs
that are globally accessible is a privacy issue.

+      DictionaryPrefUpdate user_session_initialized(GetLocalState(),
+                                                    kUserSessionInitialized);
+      user_session_initialized->SetBooleanWithoutPathExpansion(
+          it->GetUserEmail(), true);
+    }
     User* user = nullptr;
     if (IsSupervisedAccountId(*it)) {
       user = User::CreateSupervisedUser(*it);
@@ -799,6 +843,7 @@ 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_session_initialized(LoadSessionInitialized(*it));
     user->set_using_saml(known_user::IsUsingSAML(*it));
     users_.push_back(user);
 
@@ -821,6 +866,9 @@ void UserManagerBase::EnsureUsersLoaded() {
     }
   }
 
+  if (mark_user_sessions_initialized)
+    local_state->CommitPendingWrite();
+
   user_loading_stage_ = STAGE_LOADED;
 
   PerformPostUserListLoadingActions();
@@ -883,6 +931,11 @@ 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)));
+    DictionaryPrefUpdate user_session_initialized(GetLocalState(),
+                                                  kUserSessionInitialized);
+    user_session_initialized->SetBooleanWithoutPathExpansion(
+        active_user_->GetAccountId().GetUserEmail(),
+        active_user_->session_initialized());
   }
 
   AddUserRecord(active_user_);
@@ -939,6 +992,20 @@ bool UserManagerBase::LoadForceOnlineSignin(const AccountId& account_id) const {
   return force_online_signin;
 }
 
+bool UserManagerBase::LoadSessionInitialized(
+    const AccountId& account_id) const {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  const base::DictionaryValue* prefs_session_initialized =
+      GetLocalState()->GetDictionary(kUserSessionInitialized);
+  bool session_initialized = false;
+  if (prefs_session_initialized) {
+    prefs_session_initialized->GetBooleanWithoutPathExpansion(
+        account_id.GetUserEmail(), &session_initialized);
+  }
+  return session_initialized;
+}
+
 void UserManagerBase::RemoveNonCryptohomeData(const AccountId& account_id) {
   PrefService* prefs = GetLocalState();
   DictionaryPrefUpdate prefs_display_name_update(prefs, kUserDisplayName);
@@ -961,6 +1028,11 @@ void UserManagerBase::RemoveNonCryptohomeData(const AccountId& account_id) {
   prefs_force_online_update->RemoveWithoutPathExpansion(
       account_id.GetUserEmail(), nullptr);
 
+  DictionaryPrefUpdate prefs_session_initialized(prefs,
+                                                 kUserSessionInitialized);
+  prefs_session_initialized->RemoveWithoutPathExpansion(
+      account_id.GetUserEmail(), nullptr);
+
   known_user::RemovePrefs(account_id);
 
   const AccountId last_active_user =