Display confirmation dialog for untrusted signins

Display confirmation dialog for untrusted signins

BUG=252062
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208520

[Roger Tawa OOO till Jul 10th, 2013-06-20 07:51:29]

https://codereview.chromium.org/17482002/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/17482002/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:233:
args.confirmation_required;
Can probably get rid of this local var too.  Its now only used once at line 239.

https://codereview.chromium.org/17482002/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:991:
untrusted_confirmation_required_ = true;
I don't think this change and the one at line 1063 below are strictly required,
since the untrusted check is performed at the end of GAIA sign in process when
we get the G-A-S header.  However, these changes are useful in case the process
swap logic fails for some reason.

https://codereview.chromium.org/17482002/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:1154: false /*
confirmation_required */, source_),
I think we should change this false to a true as well.  Its not explicitly
mentioned in the bug, but we are disabling confirmation here because we assume
the advanced sync config dialog will handle aborting the sign in, but it no
longer does.

[noms (inactive), 2013-06-21 16:59:24]

https://codereview.chromium.org/17482002/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/17482002/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:233:
args.confirmation_required;
On 2013/06/20 07:51:29, Roger Tawa wrote:
> Can probably get rid of this local var too.  Its now only used once at line
239.

Done.

https://codereview.chromium.org/17482002/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:991:
untrusted_confirmation_required_ = true;
As discussed, removed this change.
On 2013/06/20 07:51:29, Roger Tawa wrote:
> I don't think this change and the one at line 1063 below are strictly
required,
> since the untrusted check is performed at the end of GAIA sign in process when
> we get the G-A-S header.  However, these changes are useful in case the
process
> swap logic fails for some reason.

[Roger Tawa OOO till Jul 10th, 2013-06-21 17:06:22]

lgtm

Thanks Monica!

https://codereview.chromium.org/17482002/diff/7001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/17482002/diff/7001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:320:
chrome::NavigateParams params(browser_, GURL(chrome::kChromeUINewTabURL),
I wonder if you should call EnsureBrowser() here, like is done at line 387
below.  This could happen if the user closes the browser window before the
starter is done.

[noms (inactive), 2013-06-21 17:21:20]

https://codereview.chromium.org/17482002/diff/7001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/17482002/diff/7001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:320:
chrome::NavigateParams params(browser_, GURL(chrome::kChromeUINewTabURL),
On 2013/06/21 17:06:22, Roger Tawa wrote:
> I wonder if you should call EnsureBrowser() here, like is done at line 387
> below.  This could happen if the user closes the browser window before the
> starter is done.

Done.

[noms (inactive), 2013-06-21 17:39:34]

+ pam for owners stamp on chrome/browser/ui/webui/inline_login_ui.cc

[Pam (message me for reviews), 2013-06-21 18:48:47]

LGTM.

Just a note, this kind of change -- only adjusting a method call to match a
change already approved in another part of the code -- can be also done with TBR
owner approval.

- Pam

[commit-bot: I haz the power, 2013-06-21 22:23:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-21 23:40:01]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-21 23:50:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[nasko, 2013-06-22 00:55:26]

Drive by: Why is this change missing a test? How will we detect regression to
this if we don't have one?

[commit-bot: I haz the power, 2013-06-22 01:30:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-22 02:44:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-22 03:07:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-22 03:35:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-22 03:42:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-22 06:11:04]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-23 16:42:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-23 17:53:43]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Andrew T Wilson (Slow), 2013-06-24 08:48:12]

https://codereview.chromium.org/17482002/diff/18001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/17482002/diff/18001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:1150: true /*
confirmation_required */, source_),
Why are we *always* displaying this confirmation, and not just only in the case
where untrusted_confirmation_required == true? Doesn't this display an extra
confirmation even in a trusted GAIA flow?

[commit-bot: I haz the power, 2013-06-24 17:41:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-24 21:07:07]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-25 14:19:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-25 15:31:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-25 15:58:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/17482002/18001

[commit-bot: I haz the power, 2013-06-25 17:43:15]

Change committed as 208520