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

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}
Committed: https://chromium.googlesource.com/chromium/src/+/d5a7eabf41ff530632d5cd18b7136b52bbdc5f20

[Andrew T Wilson (Slow), 2017-02-23 16:25:37]

The CQ bit was checked by atwilson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 16:25:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 17:07:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 17:07:29]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2017-02-23 17:27:03]

atwilson@chromium.org changed reviewers:
+ alemate@chromium.org, emaxx@chromium.org

[Andrew T Wilson (Slow), 2017-02-23 17:27:03]

alemate: ptal at user/user_manager stuff
emaxx: ptal overall

This CL is primarily to setup session_initialized() - I'll create a followup CL
to integrate session_initialized() more deeply in our policy initialization
code, to fix cases where people are currently escaping policy.

[emaxx, 2017-02-23 21:45:26]

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:199: void
FakeChromeUserManager::OnSessionInitialized(user_manager::User* user) {
nit: Add an empty line before?

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:202: //
We want to block for policy in a few situations: if the user is new, or if
nit: The comment should be updated to correspond to the code.

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:209:
(user_manager->IsCurrentUserNew() ||
IIUC, IsCurrentUserNew() returns true for ephemeral users even during
restart-after-crash. So, even if there IS actually cached policy, we would start
waiting for policy fetch (which, if the policy fetch fails, would terminate the
user session).

Or you planned to implement the fine-grained checks in the policy stack in a
separate CL?

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:62: const char
kUserSessionInitialized[] = "UserSessionInitialized";
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.

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:280: // Do not update local state
if data stored or cached outside the user's
I'm curious how then is the flag preserved for the ephemeral users across
restarts. Is it transmitted from the crashed session differently somehow?

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:289:
user->GetAccountId().GetUserEmail(), true);
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.

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:816: // that was not tracking
SessionInitialized state. If there are already users
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.

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:819: const bool
mark_user_sessions_initialized =
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();
}

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:825: LOG(WARNING) << "Migrating
existing user to session_initialized=true";
nit: Maybe also log something that specifies the user being migrated, like their
e-mail?

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:825: LOG(WARNING) << "Migrating
existing user to session_initialized=true";
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.

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

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:288:
user_session_initialized->SetBooleanWithoutPathExpansion(
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:16]

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:199: void
FakeChromeUserManager::OnSessionInitialized(user_manager::User* user) {
On 2017/02/23 21:45:25, emaxx wrote:
> nit: Add an empty line before?

Done.

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:202: //
We want to block for policy in a few situations: if the user is new, or if
On 2017/02/23 21:45:25, emaxx wrote:
> nit: The comment should be updated to correspond to the code.

Done.

https://codereview.chromium.org/2711113003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:209:
(user_manager->IsCurrentUserNew() ||
Good point, I'll remove the check for new user as it doesn't buy us anything
(per our discussion). I still plan to put more checks in policy code in a
followup CL, but want to fix this logic now since it fixes a bug.

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:62: const char
kUserSessionInitialized[] = "UserSessionInitialized";
On 2017/02/23 21:45:25, emaxx wrote:
> 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.

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?

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:280: // Do not update local state
if data stored or cached outside the user's
On 2017/02/23 21:45:25, emaxx wrote:
> I'm curious how then is the flag preserved for the ephemeral users across
> restarts. Is it transmitted from the crashed session differently somehow?

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).

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:288:
user_session_initialized->SetBooleanWithoutPathExpansion(
On 2017/02/24 09:58:11, Alexander Alekseev wrote:
> We have known_user database for this.
> Could you use it? It will also save the problem with user IDs.

Done.

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:289:
user->GetAccountId().GetUserEmail(), true);
On 2017/02/23 21:45:25, emaxx wrote:
> 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.

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

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:816: // that was not tracking
SessionInitialized state. If there are already users
On 2017/02/23 21:45:25, emaxx wrote:
> 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.

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.

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:819: const bool
mark_user_sessions_initialized =
On 2017/02/23 21:45:25, emaxx wrote:
> 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();
> }

Moved migration code to known_user::IsSessionInitialized().

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:825: LOG(WARNING) << "Migrating
existing user to session_initialized=true";
On 2017/02/23 21:45:25, emaxx wrote:
> nit: Maybe also log something that specifies the user being migrated, like
their
> e-mail?

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

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:825: LOG(WARNING) << "Migrating
existing user to session_initialized=true";
On 2017/02/23 21:45:25, emaxx wrote:
> 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.

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:17:07]

Description was changed from

==========
Track whether a given user session has completed initialization, 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.

BUG=684031
==========

to

==========
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.

BUG=684031
==========

[Andrew T Wilson (Slow), 2017-02-24 16:17:17]

The CQ bit was checked by atwilson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 16:17:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Andrew T Wilson (Slow), 2017-02-24 16:18:36]

atwilson@chromium.org changed reviewers:
+ mlerman@chromium.org

[Andrew T Wilson (Slow), 2017-02-24 16:18:37]

mlerman, ptal at chrome/browser/profiles

alemate,emaxx: ptal - I believe I've addressed your feedback.

Maksim, I'll improve ephemeral user support in a followup CL.

[Mike Lerman, 2017-02-24 16:22:32]

mlerman@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[Mike Lerman, 2017-02-24 16:22:33]

Re-assigning to Anthony.

[Andrew T Wilson (Slow), 2017-02-24 16:30:27]

The CQ bit was checked by atwilson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 16:30:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[anthonyvd, 2017-02-24 16:44:29]

c/b/profiles lgtm

[commit-bot: I haz the power, 2017-02-24 17:14:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 17:14:28]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:62: const char
kUserSessionInitialized[] = "UserSessionInitialized";
On 2017/02/24 16:15:15, Andrew T Wilson (Slow) wrote:
> On 2017/02/23 21:45:25, emaxx wrote:
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:280: // Do not update local state
if data stored or cached outside the user's
On 2017/02/24 16:15:15, Andrew T Wilson (Slow) wrote:
> On 2017/02/23 21:45:25, emaxx wrote:
> > I'm curious how then is the flag preserved for the ephemeral users across
> > restarts. Is it transmitted from the crashed session differently somehow?
> 
> 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).

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

https://codereview.chromium.org/2711113003/diff/20001/components/user_manager...
components/user_manager/user_manager_base.cc:816: // that was not tracking
SessionInitialized state. If there are already users
On 2017/02/24 16:15:15, Andrew T Wilson (Slow) wrote:
> On 2017/02/23 21:45:25, emaxx wrote:
> > 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.
> 
> 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.
> 

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

[Andrew T Wilson (Slow), 2017-02-24 21:12:27]

On 2017/02/24 17:54:09, emaxx wrote:

> 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.

alemate@: you are the owner of this code - do you have an opinion here? I
understand Maksim's concerns, but I'm not convinced that
user->profile_ever_initialized() is the most succinct term. If you can't think
of anything better, I'll go with
ProfileEverInitialized/profile_ever_initialized()


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

Done.

[Andrew T Wilson (Slow), 2017-02-24 21:14:19]

Description was changed from

==========
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.

BUG=684031
==========

to

==========
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
==========

[emaxx, 2017-02-27 13:48:15]

LGTM - leaving the final decision on the pref/method renaming to Drew and
Alexander.

[Alexander Alekseev, 2017-02-28 04:42:13]

On 2017/02/24 21:12:27, Andrew T Wilson (Slow) wrote:
> On 2017/02/24 17:54:09, emaxx wrote:
> 
> > 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.
> 
> alemate@: you are the owner of this code - do you have an opinion here? I
> understand Maksim's concerns, but I'm not convinced that
> user->profile_ever_initialized() is the most succinct term. If you can't think
> of anything better, I'll go with
> ProfileEverInitialized/profile_ever_initialized()

I am actually used to thinking of "Profile initialized" as "profile directory
and storages inside are ready to use". So I won't be confused with
"ProfileInitializationCompleted".
I am also OK with "ProfileEverInitialized" because it means exactly the same to
me.

I also find you comment covering all the issues, and I think it is enough to
keep this comment with any name you choose.
Thus I am fine with any name provided we have clear comments on that.

So it's up to you to choose what sounds better.

[Andrew T Wilson (Slow), 2017-03-07 17:16:07]

The CQ bit was checked by atwilson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 17:17:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 18:10:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:10:06]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2017-03-08 10:32:17]

alemate: PTAL, I renamed the functions per our discussion.

[Alexander Alekseev, 2017-03-09 01:57:29]

On 2017/03/08 10:32:17, Andrew T Wilson (Slow) wrote:
> alemate: PTAL, I renamed the functions per our discussion.

lgtm

[Alexander Alekseev, 2017-03-09 02:04:04]

lgtm with nits actually ;)

https://codereview.chromium.org/2711113003/diff/80001/components/user_manager...
File components/user_manager/known_user.h (right):

https://codereview.chromium.org/2711113003/diff/80001/components/user_manager...
components/user_manager/known_user.h:153: void USER_MANAGER_EXPORT
RemovePrefs(const AccountId& account_id);
nit: Could you leave this private, and create a new (exported) proxy method
RemovePrefsForTesting(...), so that our presubmit hooks will work, preventing
RemovePrefsForTesting() this to be called from non-test code outside of
components/user_manager ?

[Andrew T Wilson (Slow), 2017-03-09 12:33:15]

https://codereview.chromium.org/2711113003/diff/80001/components/user_manager...
File components/user_manager/known_user.h (right):

https://codereview.chromium.org/2711113003/diff/80001/components/user_manager...
components/user_manager/known_user.h:153: void USER_MANAGER_EXPORT
RemovePrefs(const AccountId& account_id);
On 2017/03/09 02:04:04, Alexander Alekseev wrote:
> nit: Could you leave this private, and create a new (exported) proxy method
> RemovePrefsForTesting(...), so that our presubmit hooks will work, preventing
> RemovePrefsForTesting() this to be called from non-test code outside of
> components/user_manager ?

Done.

[Andrew T Wilson (Slow), 2017-03-09 12:33:21]

The CQ bit was checked by atwilson@chromium.org

[Andrew T Wilson (Slow), 2017-03-09 12:33:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org, emaxx@chromium.org, alemate@chromium.org
Link to the patchset: https://codereview.chromium.org/2711113003/#ps100001
(title: "Only expose known_user::RemovePrefsForTesting() for tests")

[commit-bot: I haz the power, 2017-03-09 12:33:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 13:18:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489062801564200,
"parent_rev": "ea7669c009692703286a55c3ac90ae1e11178501", "commit_rev":
"d5a7eabf41ff530632d5cd18b7136b52bbdc5f20"}

[commit-bot: I haz the power, 2017-03-09 13:19:25]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/d5a7eabf41ff530632d5cd18b713...
==========

[commit-bot: I haz the power, 2017-03-09 13:19:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d5a7eabf41ff530632d5cd18b713...