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

components/user_manager/user_manager_base.cc

 // Copyright 2014 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 "components/user_manager/user_manager_base.h"
 
 #include <stddef.h>
 #include <set>
 #include <utility>
 
@@ skipping 36 matching lines @@
 // A dictionary that maps user IDs to the displayed (non-canonical) emails.
 const char kUserDisplayEmail[] = "UserDisplayEmail";
 
 // A dictionary that maps user IDs to OAuth token presence flag.
 const char kUserOAuthTokenStatus[] = "OAuthTokenStatus";
 
 // A dictionary that maps user IDs to a flag indicating whether online
 // 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";
 
 // A string pref containing the ID of the last user who logged in if it was
 // a user with gaia account (regular) or an empty string if it was another type
 // of user (guest, kiosk, public account, etc.).
 const char kLastLoggedInGaiaUser[] = "LastLoggedInRegularUser";
 
 // A string pref containing the ID of the last active user.
 // In case of browser crash, this pref will be used to set active user after
 // session restore.
 const char kLastActiveUser[] = "LastActiveUser";
 
 // Upper bound for a histogram metric reporting the amount of time between
 // one regular user logging out and a different regular user logging in.
 const int kLogoutToLoginDelayMaxSec = 1800;
 
 }  // namespace
 
 // static
 void UserManagerBase::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(kRegularUsers);
   registry->RegisterStringPref(kLastLoggedInGaiaUser, std::string());
   registry->RegisterDictionaryPref(kUserDisplayName);
   registry->RegisterDictionaryPref(kUserGivenName);
   registry->RegisterDictionaryPref(kUserDisplayEmail);
   registry->RegisterDictionaryPref(kUserOAuthTokenStatus);
   registry->RegisterDictionaryPref(kUserForceOnlineSignin);
+  registry->RegisterDictionaryPref(kUserSessionInitialized);
   registry->RegisterDictionaryPref(kUserType);
   registry->RegisterStringPref(kLastActiveUser, std::string());
 
   known_user::RegisterPrefs(registry);
 }
 
 UserManagerBase::UserManagerBase(scoped_refptr<base::TaskRunner> task_runner)
     : task_runner_(task_runner), weak_factory_(this) {}
 
 UserManagerBase::~UserManagerBase() {
@@ skipping 161 matching lines @@
   last_session_active_account_id_.clear();
 }
 
 void UserManagerBase::OnSessionStarted() {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   CallUpdateLoginState();
   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());
 
   if (!CanUserBeRemoved(FindUser(account_id)))
     return;
 
   RemoveUserInternal(account_id, delegate);
 }
 
@@ skipping 499 matching lines @@
 
   // Load public sessions first.
   std::set<AccountId> device_local_accounts_set;
   LoadDeviceLocalAccounts(&device_local_accounts_set);
 
   // Load regular users and supervised users.
   std::vector<AccountId> regular_users;
   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);
     } else {
       user = User::CreateRegularUser(*it);
       int user_type;
       if (prefs_user_types->GetIntegerWithoutPathExpansion(it->GetUserEmail(),
                                                            &user_type) &&
           user_type == USER_TYPE_CHILD) {
         ChangeUserChildStatus(user, true /* is child */);
       }
     }
     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);
 
     base::string16 display_name;
     if (prefs_display_names->GetStringWithoutPathExpansion(it->GetUserEmail(),
                                                            &display_name)) {
       user->set_display_name(display_name);
     }
 
     base::string16 given_name;
     if (prefs_given_names->GetStringWithoutPathExpansion(it->GetUserEmail(),
                                                          &given_name)) {
       user->set_given_name(given_name);
     }
 
     std::string display_email;
     if (prefs_display_emails->GetStringWithoutPathExpansion(it->GetUserEmail(),
                                                             &display_email)) {
       user->set_display_email(display_email);
     }
   }
 
+  if (mark_user_sessions_initialized)
+    local_state->CommitPendingWrite();
+
   user_loading_stage_ = STAGE_LOADED;
 
   PerformPostUserListLoadingActions();
 }
 
 UserList& UserManagerBase::GetUsersAndModify() {
   EnsureUsersLoaded();
   return users_;
 }
 
@@ skipping 42 matching lines @@
   // Remove the user from the user list.
   active_user_ = RemoveRegularOrSupervisedUserFromList(account_id);
 
   // If the user was not found on the user list, create a new user.
   SetIsCurrentUserNew(!active_user_);
   if (IsCurrentUserNew()) {
     active_user_ = User::CreateRegularUser(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_);
 
   // Make sure that new data is persisted to Local State.
   GetLocalState()->CommitPendingWrite();
 }
 
 void UserManagerBase::RegularUserLoggedInAsEphemeral(
     const AccountId& account_id) {
@@ skipping 36 matching lines @@
   const base::DictionaryValue* prefs_force_online =
       GetLocalState()->GetDictionary(kUserForceOnlineSignin);
   bool force_online_signin = false;
   if (prefs_force_online) {
     prefs_force_online->GetBooleanWithoutPathExpansion(
         account_id.GetUserEmail(), &force_online_signin);
   }
   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);
   prefs_display_name_update->RemoveWithoutPathExpansion(
       account_id.GetUserEmail(), nullptr);
 
   DictionaryPrefUpdate prefs_given_name_update(prefs, kUserGivenName);
   prefs_given_name_update->RemoveWithoutPathExpansion(account_id.GetUserEmail(),
                                                       nullptr);
 
   DictionaryPrefUpdate prefs_display_email_update(prefs, kUserDisplayEmail);
   prefs_display_email_update->RemoveWithoutPathExpansion(
       account_id.GetUserEmail(), nullptr);
 
   DictionaryPrefUpdate prefs_oauth_update(prefs, kUserOAuthTokenStatus);
   prefs_oauth_update->RemoveWithoutPathExpansion(account_id.GetUserEmail(),
                                                  nullptr);
 
   DictionaryPrefUpdate prefs_force_online_update(prefs, kUserForceOnlineSignin);
   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 =
       AccountId::FromUserEmail(GetLocalState()->GetString(kLastActiveUser));
   if (account_id == last_active_user)
     GetLocalState()->SetString(kLastActiveUser, std::string());
 }
 
 User* UserManagerBase::RemoveRegularOrSupervisedUserFromList(
     const AccountId& account_id) {
@@ skipping 115 matching lines @@
 }
 
 void UserManagerBase::DeleteUser(User* user) {
   const bool is_active_user = (user == active_user_);
   delete user;
   if (is_active_user)
     active_user_ = nullptr;
 }
 
 }  // namespace user_manager