cros: Terminate if merge session fails for online sign-in

cros: Terminate if merge session fails for online sign-in

Merge session should always succeed for a just-processed online
sign-in. Terminate current user session when it happens. Otherwise,
login code ends up into an invalid state and things depends on auth
token such as chrome sync etc would be broken.

BUG=677312
TEST=OAuth2Test.TerminateOnBadMergeSessionAfterOnlineAuth

Review-Url: https://codereview.chromium.org/2903123002
Cr-Commit-Position: refs/heads/master@{#475550}
Committed: https://chromium.googlesource.com/chromium/src/+/c3ac4d286f2c890ef88178e7ec552178a25cce50

[xiyuan, 2017-05-24 22:43:53]

Description was changed from

==========
cros: Terminate if merge session failed for online sign-in

Merge session should always succeed for a just-processed online
sign-in. Terminate current user session when it happens. Otherwise,
login code ends up into an invalid state and things depends on auth
token such as chrome sync etc would be broken.

BUG=677312
==========

to

==========
cros: Terminate if merge session fails for online sign-in

Merge session should always succeed for a just-processed online
sign-in. Terminate current user session when it happens. Otherwise,
login code ends up into an invalid state and things depends on auth
token such as chrome sync etc would be broken.

BUG=677312
==========

[xiyuan, 2017-05-24 22:43:58]

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

[commit-bot: I haz the power, 2017-05-24 22:45:03]

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-05-24 23:19:17]

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

[commit-bot: I haz the power, 2017-05-24 23:19:18]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-25 19:24:43]

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

[commit-bot: I haz the power, 2017-05-25 19:25:12]

Dry run: CQ is trying da patch.

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

[xiyuan, 2017-05-25 19:31:14]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org, msarda@chromium.org

[xiyuan, 2017-05-25 19:31:14]

achuith@, please review everthing.

msarda@, for owner review of fake_gaia.cc
  Add content for bad /OAuthLogin call. This is needed
  because GaiaAuthFetcher does not look at http status code.
  Instead, it checks the response body. If it does not
  recognize the body, it generates SERVICE_UNAVAILABLE.
  This is treated as a transient error and would cause
  the unnecessary retries and make OAuth2LoginManager think
  it is a connection error.

[xiyuan, 2017-05-25 19:42:55]

Description was changed from

==========
cros: Terminate if merge session fails for online sign-in

Merge session should always succeed for a just-processed online
sign-in. Terminate current user session when it happens. Otherwise,
login code ends up into an invalid state and things depends on auth
token such as chrome sync etc would be broken.

BUG=677312
==========

to

==========
cros: Terminate if merge session fails for online sign-in

Merge session should always succeed for a just-processed online
sign-in. Terminate current user session when it happens. Otherwise,
login code ends up into an invalid state and things depends on auth
token such as chrome sync etc would be broken.

BUG=677312
TEST=OAuth2Test.TerminateOnBadMergeSessionAfterOnlineAuth
==========

[achuithb, 2017-05-25 19:44:17]

lgtm

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:870: const bool
is_online_signin =
I think a comment explaining that this is necessary to prevent a policy exploit
and a link to the bug might me useful?

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_browsertest.cc (right):

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:590:
IN_PROC_BROWSER_TEST_F(OAuth2Test, TerminateOnBadMergeSessionAfterOnlineAuth) {
Let's add a comment and a link to the bug

[xiyuan, 2017-05-25 20:24:10]

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:870: const bool
is_online_signin =
On 2017/05/25 19:44:17, achuithb wrote:
> I think a comment explaining that this is necessary to prevent a policy
exploit
> and a link to the bug might me useful?

Done.

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/oauth2_browsertest.cc (right):

https://codereview.chromium.org/2903123002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/oauth2_browsertest.cc:590:
IN_PROC_BROWSER_TEST_F(OAuth2Test, TerminateOnBadMergeSessionAfterOnlineAuth) {
On 2017/05/25 19:44:17, achuithb wrote:
> Let's add a comment and a link to the bug

Done.

[xiyuan, 2017-05-25 20:24:19]

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

[commit-bot: I haz the power, 2017-05-25 20:25:22]

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-05-25 21:51:36]

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

[commit-bot: I haz the power, 2017-05-25 21:51:37]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-05-29 11:44:00]

LGTM for google_apis/gaia/fake_gaia.cc

[xiyuan, 2017-05-30 15:06:56]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-05-30 15:06:56]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2903123002/#ps40001
(title: "add comments")

[commit-bot: I haz the power, 2017-05-30 15:07:12]

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-05-30 16:12:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496156816155280,
"parent_rev": "27875ca388fe43aa376f1f15ab496f594e80f8d5", "commit_rev":
"c3ac4d286f2c890ef88178e7ec552178a25cce50"}

[commit-bot: I haz the power, 2017-05-30 16:12:42]

Description was changed from

==========
cros: Terminate if merge session fails for online sign-in

Merge session should always succeed for a just-processed online
sign-in. Terminate current user session when it happens. Otherwise,
login code ends up into an invalid state and things depends on auth
token such as chrome sync etc would be broken.

BUG=677312
TEST=OAuth2Test.TerminateOnBadMergeSessionAfterOnlineAuth
==========

to

==========
cros: Terminate if merge session fails for online sign-in

Merge session should always succeed for a just-processed online
sign-in. Terminate current user session when it happens. Otherwise,
login code ends up into an invalid state and things depends on auth
token such as chrome sync etc would be broken.

BUG=677312
TEST=OAuth2Test.TerminateOnBadMergeSessionAfterOnlineAuth

Review-Url: https://codereview.chromium.org/2903123002
Cr-Commit-Position: refs/heads/master@{#475550}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3ac4d286f2c890ef88178e7ec55...
==========

[commit-bot: I haz the power, 2017-05-30 16:12:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c3ac4d286f2c890ef88178e7ec55...