Inline sign in extracts gaia id from HTTP header and seeds account tracker

Inline login handler extracts gaia id from HTTP header and seeds account tracker
service during sign in.

BUG=341408
Committed: https://crrev.com/a8e7c94b1b79a0948d05a1fcfff53391d22ce37a
Cr-Commit-Position: refs/heads/master@{#300750}

[Roger Tawa OOO till Jul 10th, 2014-08-21 02:40:46]

Hi Hui,

Please review.  Where would I add a call to SeedAccountInfo() to support cros? 
Is there a common place that would work for both desktop and cros?  Thanks.

[guohui, 2014-08-21 18:39:44]

lgtm with one question.

https://codereview.chromium.org/473153002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/473153002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:133:
account_tracker->SeedAccountInfo(account_id, gaia_id_, email_);
why do we use email_ here and email at line 130? Are they guaranteed to be the
same?

[Roger Tawa OOO till Jul 10th, 2014-08-21 20:33:18]

Thanks Hui.  Answer below.

Note that you didn't address my cros question.  Thinking about other platforms
too, I think I will move the seed call to somewhere inside SigninManagerBase
instead.  I will send you a new patchset.

https://codereview.chromium.org/473153002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/473153002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:133:
account_tracker->SeedAccountInfo(account_id, gaia_id_, email_);
On 2014/08/21 18:39:43, guohui wrote:
> why do we use email_ here and email at line 130? Are they guaranteed to be the
> same?

|email| and |email_| are not guaranteed to be the same, especially with mixed
case email addresses.  |email_| is guaranteed to be the same as |display_email|.
 All the gaia apis we call return |display_email|, so I figured I would use that
one.

But moving forward, it should not matter which one we use.  This is for display
purposes, and any will do.  We should never be comparing these strings to
determine if two accounts are the same or not.  We should only compare the
|account_id| or the |gaia_id_|.

[Roger Tawa OOO till Jul 10th, 2014-08-26 19:37:11]

Patchset #5 (id:180001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-26 19:37:22]

Patchset #5 (id:200001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-26 19:37:31]

Patchset #5 (id:220001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-26 19:37:40]

Patchset #5 (id:240001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-27 01:10:55]

Patchset #5 (id:260001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-27 13:10:27]

rogerta@chromium.org changed reviewers:
+ bartfab@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-08-27 13:10:27]

Hi Bartosz,

This CL collects the gaiaid of the user during sign in.  However, my code does
not seem to be called on cros.  I guess I have not hooked up the right places. 
Can you take a look and let me know what I'm missing?  Thanks.

[Roger Tawa OOO till Jul 10th, 2014-08-27 15:21:40]

Patchset #6 (id:300001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-27 15:23:05]

Patchset #4 (id:160001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-27 15:23:24]

Patchset #4 (id:280001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-08-28 15:30:14]

Patchset #6 (id:360001) has been deleted

[bartfab (slow), 2014-08-28 16:49:34]

On 2014/08/27 13:10:27, Roger Tawa wrote:
> Hi Bartosz,
> 
> This CL collects the gaiaid of the user during sign in.  However, my code does
> not seem to be called on cros.  I guess I have not hooked up the right places.

> Can you take a look and let me know what I'm missing?  Thanks.

You did the right thing by hooking into gaia_auth/background.js. However, you
tapped into the desktop flow, not the CrOS flow. The CrOS flow starts on line
295 of that file.

[Roger Tawa OOO till Jul 10th, 2014-08-28 17:16:35]

On 2014/08/28 16:49:34, bartfab wrote:
> On 2014/08/27 13:10:27, Roger Tawa wrote:
> > Hi Bartosz,
> > 
> > This CL collects the gaiaid of the user during sign in.  However, my code
does
> > not seem to be called on cros.  I guess I have not hooked up the right
places.
> 
> > Can you take a look and let me know what I'm missing?  Thanks.
> 
> You did the right thing by hooking into gaia_auth/background.js. However, you
> tapped into the desktop flow, not the CrOS flow. The CrOS flow starts on line
> 295 of that file.

Thanks Bartosz.  I did some more debugging on cros, and found a few more place I
believe I need to hook.  See ps7 details.  Note I left myself some TODOs and
hardcoded strings for gaia-id, since I'm not sure how to set them.  Do you think
this is moving in the right direction?

[bartfab (slow), 2014-08-28 17:23:22]

On 2014/08/28 17:16:35, Roger Tawa wrote:
> On 2014/08/28 16:49:34, bartfab wrote:
> > On 2014/08/27 13:10:27, Roger Tawa wrote:
> > > Hi Bartosz,
> > > 
> > > This CL collects the gaiaid of the user during sign in.  However, my code
> does
> > > not seem to be called on cros.  I guess I have not hooked up the right
> places.
> > 
> > > Can you take a look and let me know what I'm missing?  Thanks.
> > 
> > You did the right thing by hooking into gaia_auth/background.js. However,
you
> > tapped into the desktop flow, not the CrOS flow. The CrOS flow starts on
line
> > 295 of that file.
> 
> Thanks Bartosz.  I did some more debugging on cros, and found a few more place
I
> believe I need to hook.  See ps7 details.  Note I left myself some TODOs and
> hardcoded strings for gaia-id, since I'm not sure how to set them.  Do you
think
> this is moving in the right direction?

* What is the reason for setting this.gaiaId_ to all these dummy values in
gaia_auth/main.js? The background script should be able to send the GAIA ID when
it comes back from GAIA. Until then, it should just be null.

* The methods in login/login_common.js and chromeos_login_ext/main.js are used
for tests only. Depending on what the tests do, a fake GAIA ID may be
sufficient.

[Roger Tawa OOO till Jul 10th, 2014-08-28 17:28:56]

On 2014/08/28 17:23:22, bartfab wrote:
> On 2014/08/28 17:16:35, Roger Tawa wrote:
> > On 2014/08/28 16:49:34, bartfab wrote:
> > > On 2014/08/27 13:10:27, Roger Tawa wrote:
> > > > Hi Bartosz,
> > > > 
> > > > This CL collects the gaiaid of the user during sign in.  However, my
code
> > does
> > > > not seem to be called on cros.  I guess I have not hooked up the right
> > places.
> > > 
> > > > Can you take a look and let me know what I'm missing?  Thanks.
> > > 
> > > You did the right thing by hooking into gaia_auth/background.js. However,
> you
> > > tapped into the desktop flow, not the CrOS flow. The CrOS flow starts on
> line
> > > 295 of that file.
> > 
> > Thanks Bartosz.  I did some more debugging on cros, and found a few more
place
> I
> > believe I need to hook.  See ps7 details.  Note I left myself some TODOs and
> > hardcoded strings for gaia-id, since I'm not sure how to set them.  Do you
> think
> > this is moving in the right direction?
> 
> * What is the reason for setting this.gaiaId_ to all these dummy values in
> gaia_auth/main.js? The background script should be able to send the GAIA ID
when
> it comes back from GAIA. Until then, it should just be null.
> 
> * The methods in login/login_common.js and chromeos_login_ext/main.js are used
> for tests only. Depending on what the tests do, a fake GAIA ID may be
> sufficient.

Got it.  I was investigating before I got your message, so those hardcoded
strings represent places in the code where I thought it would need to collect
and set the gaia-id.  However, I'll look at background.js as you mentioned. 
That may make all this moot.  Thanks.

[bartfab (slow), 2014-08-29 08:33:18]

On 2014/08/28 17:28:56, Roger Tawa wrote:
> On 2014/08/28 17:23:22, bartfab wrote:
> > On 2014/08/28 17:16:35, Roger Tawa wrote:
> > > On 2014/08/28 16:49:34, bartfab wrote:
> > > > On 2014/08/27 13:10:27, Roger Tawa wrote:
> > > > > Hi Bartosz,
> > > > > 
> > > > > This CL collects the gaiaid of the user during sign in.  However, my
> code
> > > does
> > > > > not seem to be called on cros.  I guess I have not hooked up the right
> > > places.
> > > > 
> > > > > Can you take a look and let me know what I'm missing?  Thanks.
> > > > 
> > > > You did the right thing by hooking into gaia_auth/background.js.
However,
> > you
> > > > tapped into the desktop flow, not the CrOS flow. The CrOS flow starts on
> > line
> > > > 295 of that file.
> > > 
> > > Thanks Bartosz.  I did some more debugging on cros, and found a few more
> place
> > I
> > > believe I need to hook.  See ps7 details.  Note I left myself some TODOs
and
> > > hardcoded strings for gaia-id, since I'm not sure how to set them.  Do you
> > think
> > > this is moving in the right direction?
> > 
> > * What is the reason for setting this.gaiaId_ to all these dummy values in
> > gaia_auth/main.js? The background script should be able to send the GAIA ID
> when
> > it comes back from GAIA. Until then, it should just be null.
> > 
> > * The methods in login/login_common.js and chromeos_login_ext/main.js are
used
> > for tests only. Depending on what the tests do, a fake GAIA ID may be
> > sufficient.
> 
> Got it.  I was investigating before I got your message, so those hardcoded
> strings represent places in the code where I thought it would need to collect
> and set the gaia-id.  However, I'll look at background.js as you mentioned. 
> That may make all this moot.  Thanks.

onHeadersReceived() in background.js is the method that sees all headers flying
past. If you tap into the CrOS flow there (!this.isDesktopFlow_) or simply into
all flows (both desktop and CrOS), you should be able to capture the ID returned
by GAIA.

[xiyuan, 2014-09-22 21:48:56]

xiyuan@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiyuan, 2014-09-22 21:48:57]

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:411: this.gaiaId_ = msg.gaiaId;
This does not work for cros login. 'attemptLogin' message is sent from Gaia when
submitting the login form and gaiaId. The posting code on Gaia page does not
post gaiaId info and gaiaId is not available either.

We need to change the logic so that it waits for the background.js to post back
the gaia id to continue the cros sign-in. That is, we cannot do
"this.onConfirmLogin_();" when 'confirmLogin' message is received. It needs to
be deferred until background.js pinged back.

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:436: this.gaiaId_ = msg.gaiaId;
This probably does not work either. 'setAuthenticatedUserEmail' does not have
gaiaId sent with it.

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:446: this.onConfirmLogin_();
This needs to be patched to defer until gaiaId is received from background.js.

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:06:19]

Patchset #9 (id:440001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:06:32]

Patchset #9 (id:460001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:06:47]

Patchset #10 (id:500001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:06:56]

Patchset #10 (id:520001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:07:18]

Patchset #9 (id:480001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:13:15]

Patchset #10 (id:560001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:36:57]

Patchset #10 (id:580001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 19:43:44]

Thanks Xiyuan!  With your suggestions (and some help from Hui) cros seems to
work and get the gaiaid.  Please take a look at patchset 10.

I'll keep testing that all permutations work.  What are those exactly?  I can
think of:

- sign-in from oobe
- sign-in from "add user"

In both cases, uses saml and non-saml accounts.

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:411: this.gaiaId_ = msg.gaiaId;
On 2014/09/22 21:48:56, xiyuan wrote:
> This does not work for cros login. 'attemptLogin' message is sent from Gaia
when
> submitting the login form and gaiaId. The posting code on Gaia page does not
> post gaiaId info and gaiaId is not available either.
> 
> We need to change the logic so that it waits for the background.js to post
back
> the gaia id to continue the cros sign-in. That is, we cannot do
> "this.onConfirmLogin_();" when 'confirmLogin' message is received. It needs to
> be deferred until background.js pinged back.

Done.

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:436: this.gaiaId_ = msg.gaiaId;
On 2014/09/22 21:48:56, xiyuan wrote:
> This probably does not work either. 'setAuthenticatedUserEmail' does not have
> gaiaId sent with it.

Done.

https://codereview.chromium.org/473153002/diff/540001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:446: this.onConfirmLogin_();
On 2014/09/22 21:48:56, xiyuan wrote:
> This needs to be patched to defer until gaiaId is received from background.js.

Done.

[xiyuan, 2014-09-25 02:52:39]

Sorry for the delay. This gets dropped from my radar.

It is mostly good. Think we can get rid of 'confirmLogin' message handing
(remove success.js as well) and merge onConfirmLogin_ with completeLogin_. 

You cases are good.  
- OOBE and 'Add user' are similar and uses the same gaia_auth extension. Testing
one should be enough. 
- There is a 2nd gaia_auth extension in enterprise enrollment (Ctrl+Atl+E) and
might worth testing.

For both two cases, SAML and non-SAML.
For SAML, also try http://crbug.com/332132 case where typing userA in gaia but
use userB in IdP.

https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:171: 'completeLogin',
this.completeLogin_.bind(this));
Think we need to merge onConfirmLogin_ with completeLogin_. Otherwise, this
might break SAML flow.

SAML flow, we cannot send 'completeLogin' back at the end of the flow. It has
two additional steps:
1. 'retrieveAuthenticatedUserEmail " that uses /ListAccount to get the actual
user email (http://crbug.com/332132) and wait for 'setAuthenticatedUserEmail'
for actual user email;
2. Trigger Chrome to show password verify UI and wait for
'verifyConfirmedPassword' message; 

We can probably optimize out the /ListAccount call since we get actual user
email from Gaia headers now. The confirm-verify flow needs to be kept for SAML
flow.

[Roger Tawa OOO till Jul 10th, 2014-09-25 20:36:41]

Thanks Xiyuan.  Working much better.  Regular gaia accounts work from oobe and
add user.

For saml, I was able to test with crosdev1.biz but not crosdev4.biz.  The latter
gives me a server error.  For the former, in both oobe and add user, I get to
the step when it asks me to confirm the password, and then hangs there.

See patchsets 13 and 14.

https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:171: 'completeLogin',
this.completeLogin_.bind(this));
On 2014/09/25 02:52:39, xiyuan wrote:
> Think we need to merge onConfirmLogin_ with completeLogin_. Otherwise, this
> might break SAML flow.
> 
> SAML flow, we cannot send 'completeLogin' back at the end of the flow. It has
> two additional steps:
> 1. 'retrieveAuthenticatedUserEmail " that uses /ListAccount to get the actual
> user email (http://crbug.com/332132) and wait for 'setAuthenticatedUserEmail'
> for actual user email;
> 2. Trigger Chrome to show password verify UI and wait for
> 'verifyConfirmedPassword' message; 
> 
> We can probably optimize out the /ListAccount call since we get actual user
> email from Gaia headers now. The confirm-verify flow needs to be kept for SAML
> flow.

Now calls maybeCompleteLogin_(), which is the renamed maybeCompleteSAMLLogin_().
 I changed it to also support non saml flows.

[xiyuan, 2014-09-26 18:08:05]

On 2014/09/25 20:36:41, Roger Tawa wrote:
> Thanks Xiyuan.  Working much better.  Regular gaia accounts work from oobe and
> add user.
> 
> For saml, I was able to test with crosdev1.biz but not crosdev4.biz.  The
latter
> gives me a server error.  For the former, in both oobe and add user, I get to
> the step when it asks me to confirm the password, and then hangs there.
> 
> See patchsets 13 and 14.
> 
>
https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
> File chrome/browser/resources/gaia_auth/main.js (right):
> 
>
https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
> chrome/browser/resources/gaia_auth/main.js:171: 'completeLogin',
> this.completeLogin_.bind(this));
> On 2014/09/25 02:52:39, xiyuan wrote:
> > Think we need to merge onConfirmLogin_ with completeLogin_. Otherwise, this
> > might break SAML flow.
> > 
> > SAML flow, we cannot send 'completeLogin' back at the end of the flow. It
has
> > two additional steps:
> > 1. 'retrieveAuthenticatedUserEmail " that uses /ListAccount to get the
actual
> > user email (http://crbug.com/332132) and wait for
'setAuthenticatedUserEmail'
> > for actual user email;
> > 2. Trigger Chrome to show password verify UI and wait for
> > 'verifyConfirmedPassword' message; 
> > 
> > We can probably optimize out the /ListAccount call since we get actual user
> > email from Gaia headers now. The confirm-verify flow needs to be kept for
SAML
> > flow.
> 
> Now calls maybeCompleteLogin_(), which is the renamed
maybeCompleteSAMLLogin_().
>  I changed it to also support non saml flows.

crosdev4.biz is Bartosz's test server. It was his dev box. Not sure the current
status. But we should be okay if all Saml browser tests pass.

I patched your CL and made some minor changes to get SAML working. 

https://codereview.chromium.org/606083002/
PS1 is your CL and PS2 is my modifications.

Basically, I merged onConfirmLogin_ into a new onCompleteLogin_ as callback for
'completeLogin' message and cache the login params for SAML until the password
confirmation flow is finished.

[Roger Tawa OOO till Jul 10th, 2014-10-13 16:26:45]

Hi Bartosz,

After help from Xiyuan and Hui, this is ready for your review. Please take a
look. Thanks.

[Roger Tawa OOO till Jul 10th, 2014-10-14 13:57:44]

Patchset #33 (id:1440001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-10-14 13:57:54]

Patchset #32 (id:1420001) has been deleted

[bartfab (slow), 2014-10-14 17:12:39]

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:195: //
|account_id| is assumed to be already canonicalized if its an email.
Nit: s/its/it's/

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/oauth2_login_manager.h (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/oauth2_login_manager.h:185: // retrieve the
associated account id.
Nit: s/id/info/

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/background.js:220: // TOOD(guohui): Show
password confirmation UI.
Nit: Indicate that this comment is applicable to the desktop flow only.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/background.js:271: (this.gaiaUrl_ &&
details.url.lastIndexOf(this.gaiaUrl_) == 0)) {
On desktop, we look for GAIA headers only when the URL matches |gaiaUrl_|.

But on Chrome OS, we look for GAIA headers in all responses. Why?

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:111: isInternalMessage_:
function(msg) {
Nit: No longer used.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:364: if (opt_extraMsg) {
How can |opt_extraMsg| ever not exist? With the changes you made to
background.js, every |onCompleteLogin_| call should contain e-mail, GAIA ID,
etc.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:368: // gets password by asking user
for confirm.
Nit: s/for/to/

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:375: // Retrieve the e-mail address
of the user who just authenticated from GAIA.
This is no longer necessary now that we get the user's e-mail address from a
GAIA header.

You can also remove that JS code that listens for the
|retrieveAuthenticatedUserEmail| message and the C++ code backing it.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:399: maybeCompleteSAMLLogin_:
function() {
This can be folded into onVerifyConfirmedPassword_() now because we no longer
need to wait for e-mail retrieval.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:442: this.gaiaId_ = null;
Should |chooseWhatToSync_|, |skipForNow_| and |sessionIndex_| not be reset here
as well?

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:448: } else if (msg.method ==
'setAuthenticatedUserEmail' &&
This is no longer necessary now that we get the user's e-mail address from a
GAIA header.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:81:
std::string gaia_id;
Nit: #include <string>

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:89:
std::string account_id =
Nit: const.

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.cc:457:
DCHECK(!gaia.empty());
Nit: #include "base/logging.h"

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.cc:476: bool
already_exists = ContainsKey(accounts_, account_id);
Nit: const.

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
File components/signin/core/browser/account_tracker_service.h (right):

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:82: // Picks the corect
account_id for the specified account depending on the
Nit: s/corect/correct/

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:90: // Seed the gaia id
and email for the given account_id.
Nit: No |account_id| is being given here, so it is unclear what the comment
refers to.

https://codereview.chromium.org/473153002/diff/1460001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1460001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:436: "email=\"%s\", obfuscatedid=\"12345\",
sessionindex=0",
Nit: Move "12345" to a constant like kTestObfuscatedGaiaID.

[bartfab (slow), 2014-10-14 17:17:07]

On 2014/09/25 20:36:41, Roger Tawa wrote:
> Thanks Xiyuan.  Working much better.  Regular gaia accounts work from oobe and
> add user.
> 
> For saml, I was able to test with crosdev1.biz but not crosdev4.biz.  The
latter
> gives me a server error.

This is fixed now by the way. crosdev4.biz fell over while I was on vacation.
The server is working again. It is worth testing with this server as well
because it uses our credentials passing API, leading to a different flow (no
password confirmation needed).

>  For the former, in both oobe and add user, I get to
> the step when it asks me to confirm the password, and then hangs there.
> 
> See patchsets 13 and 14.
> 
>
https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
> File chrome/browser/resources/gaia_auth/main.js (right):
> 
>
https://codereview.chromium.org/473153002/diff/620001/chrome/browser/resource...
> chrome/browser/resources/gaia_auth/main.js:171: 'completeLogin',
> this.completeLogin_.bind(this));
> On 2014/09/25 02:52:39, xiyuan wrote:
> > Think we need to merge onConfirmLogin_ with completeLogin_. Otherwise, this
> > might break SAML flow.
> > 
> > SAML flow, we cannot send 'completeLogin' back at the end of the flow. It
has
> > two additional steps:
> > 1. 'retrieveAuthenticatedUserEmail " that uses /ListAccount to get the
actual
> > user email (http://crbug.com/332132) and wait for
'setAuthenticatedUserEmail'
> > for actual user email;
> > 2. Trigger Chrome to show password verify UI and wait for
> > 'verifyConfirmedPassword' message; 
> > 
> > We can probably optimize out the /ListAccount call since we get actual user
> > email from Gaia headers now. The confirm-verify flow needs to be kept for
SAML
> > flow.
> 
> Now calls maybeCompleteLogin_(), which is the renamed
maybeCompleteSAMLLogin_().
>  I changed it to also support non saml flows.

[guohui, 2014-10-14 18:50:02]

still lgtm

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/background.js:271: (this.gaiaUrl_ &&
details.url.lastIndexOf(this.gaiaUrl_) == 0)) {
On 2014/10/14 17:12:37, bartfab wrote:
> On desktop, we look for GAIA headers only when the URL matches |gaiaUrl_|.
> 
> But on Chrome OS, we look for GAIA headers in all responses. Why?

+1, seems we could just remove the check for isDesktopFlow_.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:329: this.gaiaId_ = null;  //
TODO(rogerta): no idea what to do here.
we should update the saml api to send the gaia id as well.

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
File components/signin/core/browser/account_tracker_service.h (right):

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:90: // Seed the gaia id
and email for the given account_id.
nits:s/Seed/Seeds/

maybe better to rename gaia to gaia_id.

[bartfab (slow), 2014-10-15 08:39:07]

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:329: this.gaiaId_ = null;  //
TODO(rogerta): no idea what to do here.
On 2014/10/14 18:50:02, guohui wrote:
> we should update the saml api to send the gaia id as well.

Ah, I missed that one.

The API is called by a third-party IdP who has no notion of GAIA IDs. Also, the
API call is made very early in the login process. The user is not authenticated
yet and the GAIA ID is not know.

Fortunately, the solution is trivial: A login flow that uses the API still ends
with a GAIA redirect to the success page. Thus, the |onCompleteLogin_| method
will still be called and the |email| and |gaiaId| values it receives will be
used. The |this.email_ = call.user;| line above can actually be removed. The
value it sets will always be overriden later. In line with that, there is no
need to have |this.gaiaId_ = null;| here either. The GAIA ID will be set later,
no matter what.

[Roger Tawa OOO till Jul 10th, 2014-10-16 02:39:32]

Thanks Bartosz, Hui.  Comments addressed, changes uploaded.

Bartosz: with the changes you recommended, it seems there are now two failing
tests:

SamlTest.UseAutenticatedUserEmailAddress
SamlTest.FailToRetrieveAutenticatedUserEmailAddress

I'll look into it tomorrow, but if you have any ideas let me know.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:195: //
|account_id| is assumed to be already canonicalized if its an email.
On 2014/10/14 17:12:37, bartfab wrote:
> Nit: s/its/it's/

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/oauth2_login_manager.h (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/oauth2_login_manager.h:185: // retrieve the
associated account id.
On 2014/10/14 17:12:37, bartfab wrote:
> Nit: s/id/info/

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/background.js:220: // TOOD(guohui): Show
password confirmation UI.
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: Indicate that this comment is applicable to the desktop flow only.

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/background.js:271: (this.gaiaUrl_ &&
details.url.lastIndexOf(this.gaiaUrl_) == 0)) {
On 2014/10/14 17:12:37, bartfab wrote:
> On desktop, we look for GAIA headers only when the URL matches |gaiaUrl_|.
> 
> But on Chrome OS, we look for GAIA headers in all responses. Why?

Good catch.  I got the condition wrong.  Should always match |gaiaUrl_|.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/background.js:271: (this.gaiaUrl_ &&
details.url.lastIndexOf(this.gaiaUrl_) == 0)) {
On 2014/10/14 18:50:02, guohui wrote:
> On 2014/10/14 17:12:37, bartfab wrote:
> > On desktop, we look for GAIA headers only when the URL matches |gaiaUrl_|.
> > 
> > But on Chrome OS, we look for GAIA headers in all responses. Why?
> 
> +1, seems we could just remove the check for isDesktopFlow_.

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:111: isInternalMessage_:
function(msg) {
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:329: this.gaiaId_ = null;  //
TODO(rogerta): no idea what to do here.
On 2014/10/14 18:50:02, guohui wrote:
> we should update the saml api to send the gaia id as well.

Removed, since it cannot be set here and will be set later.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:329: this.gaiaId_ = null;  //
TODO(rogerta): no idea what to do here.
On 2014/10/15 08:39:07, bartfab wrote:
> On 2014/10/14 18:50:02, guohui wrote:
> > we should update the saml api to send the gaia id as well.
> 
> Ah, I missed that one.
> 
> The API is called by a third-party IdP who has no notion of GAIA IDs. Also,
the
> API call is made very early in the login process. The user is not
authenticated
> yet and the GAIA ID is not know.
> 
> Fortunately, the solution is trivial: A login flow that uses the API still
ends
> with a GAIA redirect to the success page. Thus, the |onCompleteLogin_| method
> will still be called and the |email| and |gaiaId| values it receives will be
> used. The |this.email_ = call.user;| line above can actually be removed. The
> value it sets will always be overriden later. In line with that, there is no
> need to have |this.gaiaId_ = null;| here either. The GAIA ID will be set
later,
> no matter what.

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:364: if (opt_extraMsg) {
On 2014/10/14 17:12:38, bartfab wrote:
> How can |opt_extraMsg| ever not exist? With the changes you made to
> background.js, every |onCompleteLogin_| call should contain e-mail, GAIA ID,
> etc.

It can't anymore.  Fixed.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:368: // gets password by asking user
for confirm.
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: s/for/to/

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:375: // Retrieve the e-mail address
of the user who just authenticated from GAIA.
On 2014/10/14 17:12:38, bartfab wrote:
> This is no longer necessary now that we get the user's e-mail address from a
> GAIA header.
> 
> You can also remove that JS code that listens for the
> |retrieveAuthenticatedUserEmail| message and the C++ code backing it.

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:399: maybeCompleteSAMLLogin_:
function() {
On 2014/10/14 17:12:38, bartfab wrote:
> This can be folded into onVerifyConfirmedPassword_() now because we no longer
> need to wait for e-mail retrieval.

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:448: } else if (msg.method ==
'setAuthenticatedUserEmail' &&
On 2014/10/14 17:12:38, bartfab wrote:
> This is no longer necessary now that we get the user's e-mail address from a
> GAIA header.

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:81:
std::string gaia_id;
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:89:
std::string account_id =
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.cc:457:
DCHECK(!gaia.empty());
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.cc:476: bool
already_exists = ContainsKey(accounts_, account_id);
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
File components/signin/core/browser/account_tracker_service.h (right):

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:82: // Picks the corect
account_id for the specified account depending on the
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: s/corect/correct/

Done.

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:90: // Seed the gaia id
and email for the given account_id.
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: No |account_id| is being given here, so it is unclear what the comment
> refers to.

Fixed up the comment.  Let me know if this is cleared.

https://codereview.chromium.org/473153002/diff/1460001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1460001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:436: "email=\"%s\", obfuscatedid=\"12345\",
sessionindex=0",
On 2014/10/14 17:12:38, bartfab wrote:
> Nit: Move "12345" to a constant like kTestObfuscatedGaiaID.

Done.

[bartfab (slow), 2014-10-16 09:04:53]

On 2014/10/16 02:39:32, Roger Tawa wrote:
> Thanks Bartosz, Hui.  Comments addressed, changes uploaded.
> 
> Bartosz: with the changes you recommended, it seems there are now two failing
> tests:
> 
> SamlTest.UseAutenticatedUserEmailAddress
> SamlTest.FailToRetrieveAutenticatedUserEmailAddress

That is expected. These tests verify that e-mail retrieval based on
/ListAccounts works. Since you switched to using the header instead, these tests
will need updating.

[bartfab (slow), 2014-10-17 09:54:56]

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
File components/signin/core/browser/account_tracker_service.h (right):

https://codereview.chromium.org/473153002/diff/1460001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:90: // Seed the gaia id
and email for the given account_id.
On 2014/10/16 02:39:32, Roger Tawa wrote:
> On 2014/10/14 17:12:38, bartfab wrote:
> > Nit: No |account_id| is being given here, so it is unclear what the comment
> > refers to.
> 
> Fixed up the comment.  Let me know if this is cleared.

The comment is clearer now, thanks. The logic is still a bit convoluted as you
generate a key from |gaia| and |email| under which you then store... |gaia| and
|email|. It is not trivial to figure out what this is about.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:566:
EXPECT_EQ(kFirstSAMLUserEmail, user->email());
This test actually has even more typos in it :(.

1) The comments said: Type Bob into the GAIA form, then log in as Alice via
SAML.

2) What the test acutally did was: Type Alice into the GAIA form, then log in as
Bob via SAML.

3) What the test does now: Type Bob into the GAIA form, then log in as Bob via
SAML.

While 2) was wrong, it was correct in spirit. I had just swapped Alice and Bob.
The current behavior, 3), is wrong. The test now uses the same e-mail address
for GAIA and SAML. The very point of this test is to use different e-mail
addresses. I think the easiest fix is to restore line 548 to what it was and
adjust the comments.

At that point, the test will start failing but that is because FakeGaia is
actually wrong: It returns the e-mail address typed into the GAIA login form in
HandleServiceLoginAuth(). For SAML users, the e-mail address should be returned
by HandleSSO() instead, based on the user who actually authenticated. Once
FakaGaia has been fixed, the test should start passing again.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:573:
fake_gaia_.SetFakeMergeSessionParams(
Once FakeGaia has been fixed to return the e-mail address and GAIA ID for SAML
users from HandleSSO(), this can move back to where it was.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (left):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:431:
chrome.send('usingSAMLAPI');
We still need to inform the C++ backend that the API was used. It will be
recorded in a UMA histogram.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
You will need to modify oobe_screen_oauth_enrollment.js and its C++ backend
analogously to this file.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:353: onCompleteLogin_:
function(extraMsg) {
Nit 1: Anecdotal evidence shows that the naming seems to be |opt_extraMsg| for
optional, extra data and simply |msg| for non-optional data.
Nit 2: Document the type of |extraMsg|.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:358: message: 'foo'},
I think the correct solution here would be analogous to the "no password" case:
Instead of a generic |showFatalAuthError| message, better add a |noGaiaID|
message that will do the right thing without us having to pass in a message.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:403: // SAML login is complete when
the user's e-mail address has
Nit: Remove the part of the comment about e-mail retrieval, since we no longer
need to initiate and wait for that.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:406: if (this.email_ !== null &&
this.passwordBytes_ !== null)
Nit: We no longer need this condition. |email_ != null| is guaranteed by
onCompleteLogin_(), which runs before us. |this.passwordBytes_ != null| is
guaranteed because we set it a couple of lines above.

The moment we find that |password| matches one of the scraped values, we set
|this.passwordBytes_ = password| and call |this.completeLogin_()|. There is no
longer the need to check anything else.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:436: this.gaiaId_ = null;
My question from patchset 32 still stands:

Should |chooseWhatToSync_|, |skipForNow_| and |sessionIndex_| not be reset here
as well?

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth_host/gaia_auth_host.js (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:377:
this.showFatalAuthErrorCallback_(msg.message);
Instead of a generic |showFatalAuthErrorCallback_| that requires a message
(which we do not have), better add a |noGaiaIDCallback_| that will show the
right error message without us having to pass it in.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:5:
#include <string>
Nit: "inline_login_handler_chromeos.h" should be included first, even before
system headers.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:11: #include
"base/bind_helpers.h"
Nit: No longer used.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:49: #include
"chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.h"
1: Nit: This file is no longer used here.
2: The AuthenticatedUserEmailRetriever is actually no longer used by anyone.
Please remove the whole class.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:75: #include
"net/url_request/url_request_context_getter.h"
Nit: No longer used.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (left):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:397: void
HandleRetrieveAuthenticatedUserEmail(double attempt_token);
You should remove the analogous method from |EnrollmentScreenHandler| as well.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:49: class
AuthenticatedUserEmailRetriever;
Nit: No longer used.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:522:
scoped_ptr<AuthenticatedUserEmailRetriever> email_retriever_;
Nit: No longer used.

https://codereview.chromium.org/473153002/diff/1500001/components/signin/core...
File components/signin/core/browser/account_tracker_service.h (right):

https://codereview.chromium.org/473153002/diff/1500001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:91: // with its
corresponding gaia id and email address.
Nit: s/gaia id/GAIA ID/

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:207:
gaia_urls->ListAccountsURLWithSource(std::string()), HandleListAccounts);
I think the Chrome OS login code may have been the only user of /ListAccounts in
FakeGAIA. Could you do a try run with this method removed? If no tests fail, we
can kill the method and associated plumbing.

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:436:
http_response->AddCustomHeader("google-accounts-signin",
This line is actually wrong - and was wrong before your CL already:

* For regular GAIA users, all is well. Login succeeds the moment you submit the
login form and it is correct to return the user's e-mail address and GAIA ID
here.

* For SAML users, we redirect to the SAML IdP. The user is not authenticated
yet. We should not be returning an e-mail address or GAIA ID yet. We should do
it in HandleSSO() instead, based on the user who actually authenticated via SAML
(we use the e-mail address in |merge_session_params_| as the SAML user's e-mail
address for now).

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:439: email.c_str(),
merge_session_params_.gaia_id.c_str()));
We have tests that set up multiple accounts and verify that an attempt to log in
as Alice really logs you in as Alice, not as Bob. Right now, the tests do this
by verifying that the logged-in user's e-mail address is correct, because the
e-mail address is the primary account identifier.

You are making these changes to pave the way for the GAIA ID to become the
primary account identifier. This means that tests using multiple accounts will
need to be able to tell these accounts apart by looking at their GAIA IDs.
Returning the same GAIA ID for all login attempts will not work at that point.
We can fix it later, or we can fix it now: The GAIA ID should depend on the
user's e-mail address. It should not be a fixed value read from
|merge_session_params_|.

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.h (right):

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.h:78: // The gaia_id address returned in GAIA
headers.
Nit: s/gaia_id address/GAIA ID/

[Roger Tawa OOO till Jul 10th, 2014-10-20 16:04:01]

Hi Bartosz, comments addressed, please take another look. Thanks.

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/1460001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:442: this.gaiaId_ = null;
On 2014/10/14 17:12:38, bartfab wrote:
> Should |chooseWhatToSync_|, |skipForNow_| and |sessionIndex_| not be reset
here
> as well?

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:566:
EXPECT_EQ(kFirstSAMLUserEmail, user->email());
On 2014/10/17 09:54:55, bartfab wrote:
> This test actually has even more typos in it :(.
> 
> 1) The comments said: Type Bob into the GAIA form, then log in as Alice via
> SAML.
> 
> 2) What the test acutally did was: Type Alice into the GAIA form, then log in
as
> Bob via SAML.
> 
> 3) What the test does now: Type Bob into the GAIA form, then log in as Bob via
> SAML.
> 
> While 2) was wrong, it was correct in spirit. I had just swapped Alice and
Bob.
> The current behavior, 3), is wrong. The test now uses the same e-mail address
> for GAIA and SAML. The very point of this test is to use different e-mail
> addresses. I think the easiest fix is to restore line 548 to what it was and
> adjust the comments.
> 
> At that point, the test will start failing but that is because FakeGaia is
> actually wrong: It returns the e-mail address typed into the GAIA login form
in
> HandleServiceLoginAuth(). For SAML users, the e-mail address should be
returned
> by HandleSSO() instead, based on the user who actually authenticated. Once
> FakaGaia has been fixed, the test should start passing again.

This test is even more complex than that.  It uses 3 emails: bob@ to start,
"fake_user" in the form, and then alice@ at the last step.  I'd suggest *not*
using constants like kFirstSAMLUserEmail and kSecondSAMLUserEmail and explicitly
putting the strings in the test.  Then even the comments go away since the code
if self commenting.  The only thing that should go into constants are things
that affect the outcome of the test itself.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:573:
fake_gaia_.SetFakeMergeSessionParams(
On 2014/10/17 09:54:55, bartfab wrote:
> Once FakeGaia has been fixed to return the e-mail address and GAIA ID for SAML
> users from HandleSSO(), this can move back to where it was.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (left):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:431:
chrome.send('usingSAMLAPI');
On 2014/10/17 09:54:55, bartfab wrote:
> We still need to inform the C++ backend that the API was used. It will be
> recorded in a UMA histogram.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2014/10/17 09:54:55, bartfab wrote:
> You will need to modify oobe_screen_oauth_enrollment.js and its C++ backend
> analogously to this file.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:353: onCompleteLogin_:
function(extraMsg) {
On 2014/10/17 09:54:56, bartfab wrote:
> Nit 1: Anecdotal evidence shows that the naming seems to be |opt_extraMsg| for
> optional, extra data and simply |msg| for non-optional data.
> Nit 2: Document the type of |extraMsg|.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:358: message: 'foo'},
On 2014/10/17 09:54:55, bartfab wrote:
> I think the correct solution here would be analogous to the "no password"
case:
> Instead of a generic |showFatalAuthError| message, better add a |noGaiaID|
> message that will do the right thing without us having to pass in a message.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:403: // SAML login is complete when
the user's e-mail address has
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: Remove the part of the comment about e-mail retrieval, since we no longer
> need to initiate and wait for that.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:406: if (this.email_ !== null &&
this.passwordBytes_ !== null)
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: We no longer need this condition. |email_ != null| is guaranteed by
> onCompleteLogin_(), which runs before us. |this.passwordBytes_ != null| is
> guaranteed because we set it a couple of lines above.
> 
> The moment we find that |password| matches one of the scraped values, we set
> |this.passwordBytes_ = password| and call |this.completeLogin_()|. There is no
> longer the need to check anything else.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth/main.js:436: this.gaiaId_ = null;
On 2014/10/17 09:54:55, bartfab wrote:
> My question from patchset 32 still stands:
> 
> Should |chooseWhatToSync_|, |skipForNow_| and |sessionIndex_| not be reset
here
> as well?

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth_host/gaia_auth_host.js (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:377:
this.showFatalAuthErrorCallback_(msg.message);
On 2014/10/17 09:54:56, bartfab wrote:
> Instead of a generic |showFatalAuthErrorCallback_| that requires a message
> (which we do not have), better add a |noGaiaIDCallback_| that will show the
> right error message without us having to pass it in.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc
(right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/inline_login_handler_chromeos.cc:5:
#include <string>
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: "inline_login_handler_chromeos.h" should be included first, even before
> system headers.

Oops, done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:11: #include
"base/bind_helpers.h"
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:49: #include
"chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.h"
On 2014/10/17 09:54:56, bartfab wrote:
> 1: Nit: This file is no longer used here.
> 2: The AuthenticatedUserEmailRetriever is actually no longer used by anyone.
> Please remove the whole class.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:75: #include
"net/url_request/url_request_context_getter.h"
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (left):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:397: void
HandleRetrieveAuthenticatedUserEmail(double attempt_token);
On 2014/10/17 09:54:56, bartfab wrote:
> You should remove the analogous method from |EnrollmentScreenHandler| as well.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:49: class
AuthenticatedUserEmailRetriever;
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:522:
scoped_ptr<AuthenticatedUserEmailRetriever> email_retriever_;
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/473153002/diff/1500001/components/signin/core...
File components/signin/core/browser/account_tracker_service.h (right):

https://codereview.chromium.org/473153002/diff/1500001/components/signin/core...
components/signin/core/browser/account_tracker_service.h:91: // with its
corresponding gaia id and email address.
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: s/gaia id/GAIA ID/

This file and the associated .cc file could use a global replace of |gaia| with
|gaia_id| and related such things.  I'll do that in a separate CL.

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:207:
gaia_urls->ListAccountsURLWithSource(std::string()), HandleListAccounts);
On 2014/10/17 09:54:56, bartfab wrote:
> I think the Chrome OS login code may have been the only user of /ListAccounts
in
> FakeGAIA. Could you do a try run with this method removed? If no tests fail,
we
> can kill the method and associated plumbing.

Best done in a separate CL.

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:436:
http_response->AddCustomHeader("google-accounts-signin",
On 2014/10/17 09:54:56, bartfab wrote:
> This line is actually wrong - and was wrong before your CL already:
> 
> * For regular GAIA users, all is well. Login succeeds the moment you submit
the
> login form and it is correct to return the user's e-mail address and GAIA ID
> here.
> 
> * For SAML users, we redirect to the SAML IdP. The user is not authenticated
> yet. We should not be returning an e-mail address or GAIA ID yet. We should do
> it in HandleSSO() instead, based on the user who actually authenticated via
SAML
> (we use the e-mail address in |merge_session_params_| as the SAML user's
e-mail
> address for now).

Done.

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:439: email.c_str(),
merge_session_params_.gaia_id.c_str()));
On 2014/10/17 09:54:56, bartfab wrote:
> We have tests that set up multiple accounts and verify that an attempt to log
in
> as Alice really logs you in as Alice, not as Bob. Right now, the tests do this
> by verifying that the logged-in user's e-mail address is correct, because the
> e-mail address is the primary account identifier.
> 
> You are making these changes to pave the way for the GAIA ID to become the
> primary account identifier. This means that tests using multiple accounts will
> need to be able to tell these accounts apart by looking at their GAIA IDs.
> Returning the same GAIA ID for all login attempts will not work at that point.
> We can fix it later, or we can fix it now: The GAIA ID should depend on the
> user's e-mail address. It should not be a fixed value read from
> |merge_session_params_|.

Not sure you can do that, or that it's needed.  Today the user enters and email
into gaia, and then reenters that email into saml page.  At the end we check
that the emails match.  You can't do that with gaia-ids; the user won't enter
them, and we can't get it until after sign in.  We only get one id and there is
nothing to check against.

I think the email checking logic is good even after the gaia-id migration.  No
need to change anything.

In any case, fixed this to make all tests work.

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.h (right):

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.h:78: // The gaia_id address returned in GAIA
headers.
On 2014/10/17 09:54:56, bartfab wrote:
> Nit: s/gaia_id address/GAIA ID/

Code removed.

[bartfab (slow), 2014-10-21 14:47:47]

lgtm

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:566:
EXPECT_EQ(kFirstSAMLUserEmail, user->email());
On 2014/10/20 16:04:00, Roger Tawa wrote:
> On 2014/10/17 09:54:55, bartfab wrote:
> > This test actually has even more typos in it :(.
> > 
> > 1) The comments said: Type Bob into the GAIA form, then log in as Alice via
> > SAML.
> > 
> > 2) What the test acutally did was: Type Alice into the GAIA form, then log
in
> as
> > Bob via SAML.
> > 
> > 3) What the test does now: Type Bob into the GAIA form, then log in as Bob
via
> > SAML.
> > 
> > While 2) was wrong, it was correct in spirit. I had just swapped Alice and
> Bob.
> > The current behavior, 3), is wrong. The test now uses the same e-mail
address
> > for GAIA and SAML. The very point of this test is to use different e-mail
> > addresses. I think the easiest fix is to restore line 548 to what it was and
> > adjust the comments.
> > 
> > At that point, the test will start failing but that is because FakeGaia is
> > actually wrong: It returns the e-mail address typed into the GAIA login form
> in
> > HandleServiceLoginAuth(). For SAML users, the e-mail address should be
> returned
> > by HandleSSO() instead, based on the user who actually authenticated. Once
> > FakaGaia has been fixed, the test should start passing again.
> 
> This test is even more complex than that.  It uses 3 emails: bob@ to start,
> "fake_user" in the form, and then alice@ at the last step.  I'd suggest *not*
> using constants like kFirstSAMLUserEmail and kSecondSAMLUserEmail and
explicitly
> putting the strings in the test.  Then even the comments go away since the
code
> if self commenting.  The only thing that should go into constants are things
> that affect the outcome of the test itself.

Agreed.

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:49: #include
"chrome/browser/ui/webui/chromeos/login/authenticated_user_email_retriever.h"
On 2014/10/20 16:04:00, Roger Tawa wrote:
> On 2014/10/17 09:54:56, bartfab wrote:
> > 1: Nit: This file is no longer used here.
> > 2: The AuthenticatedUserEmailRetriever is actually no longer used by anyone.
> > Please remove the whole class.
> 
> Done.

Please file a bug to remove AuthenticatedUserEmailRetriever completely. You can
assign it to me.

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1500001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:207:
gaia_urls->ListAccountsURLWithSource(std::string()), HandleListAccounts);
On 2014/10/20 16:04:01, Roger Tawa wrote:
> On 2014/10/17 09:54:56, bartfab wrote:
> > I think the Chrome OS login code may have been the only user of
/ListAccounts
> in
> > FakeGAIA. Could you do a try run with this method removed? If no tests fail,
> we
> > can kill the method and associated plumbing.
> 
> Best done in a separate CL.

Sure. If you are not planning to do this clean-up straight away, could you file
a bug so we do not forget about it? You can assign it to me.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:859:
kTestAuthLSIDCookie1);
Nit: You no longer need to introduce line wrapping here.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:875:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie1,
Nit: You no longer need to introduce line wrapping here.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:890:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie1,
Nit: You no longer need to introduce line wrapping here.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:907:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie2,
Nit: You no longer need to introduce line wrapping here.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:926:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie2,
Nit: You no longer need to introduce line wrapping here.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(left):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:222:
this.showError(loadTimeData.getString('fatalEnrollmentError'), false);
This error message should be hooked up as
this.gaiaAuthHost_.missingGaiaInfoCallback.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:273: //
A user has successfully authenticated via regular GAIA.
Nit: Update this comment. |completeLogin| is used for regular GAIA and SAML now.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:479: * Record that
SAML API was used during sign in.
Nit: s/sign in/sign-in/

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth_host/gaia_auth_host.js (right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:177: * Invoked to
record that the SAML API was used.
Nit: Could you change this to "credentials passing API"? We had all sorts of
names for this during development but "credentials passing API" is the name that
stuck.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:394:
console.error('GaiaAuthHost: Invalid missingGaiaInfoCallback.');
Nit: s/missingGaiaInfoCallback/missingGaiaInfoCallback_/ (for consistency with
the existing error messages)

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:403:
console.error('GaiaAuthHost: Invalid samlApiUsedCallback.');
Nit: s/samlApiUsedCallback/samlApiUsedCallback_/ (for consistency with the
existing error messages)

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc (left):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:10: #include
"base/bind_helpers.h"
Nit: This is still used in EnrollmentScreenHandler::FetchOAuthToken().

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:36: #include
"net/url_request/url_request_context_getter.h"
Nit: This is still used in EnrollmentScreenHandler::FetchOAuthToken().

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:454: if (!is_saml) {
Nit: You could simplify this by rversing the conditional:

if (is_saml)
  return;

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:456:
http_response->AddCustomHeader("google-accounts-signin",
Nit: This should be extracted into a helper method that HandleServiceLoginAuth()
and HandleSSO() can call.

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.h (right):

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.h:132: std::string GetGaiaIdOfEmail(const
std::string& email);
Nit: Make the method const.

[Roger Tawa OOO till Jul 10th, 2014-10-21 19:15:32]

Thanks Bartosz.  Comments addressed, changes uploaded.  I also made sure to test
the oobe experience with saml accounts, both crosdev4.biz and crosdev1.biz, as
well as testing the error cases, and all looked good.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:859:
kTestAuthLSIDCookie1);
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: You no longer need to introduce line wrapping here.

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:875:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie1,
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: You no longer need to introduce line wrapping here.

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:890:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie1,
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: You no longer need to introduce line wrapping here.

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:907:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie2,
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: You no longer need to introduce line wrapping here.

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:926:
LogInWithSAML(kFirstSAMLUserEmail, kTestAuthSIDCookie2,
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: You no longer need to introduce line wrapping here.

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(left):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:222:
this.showError(loadTimeData.getString('fatalEnrollmentError'), false);
On 2014/10/21 14:47:47, bartfab wrote:
> This error message should be hooked up as
> this.gaiaAuthHost_.missingGaiaInfoCallback.

Done.  Testing enrollment flow and all looks good.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:273: //
A user has successfully authenticated via regular GAIA.
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: Update this comment. |completeLogin| is used for regular GAIA and SAML
now.

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:479: * Record that
SAML API was used during sign in.
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: s/sign in/sign-in/

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
File chrome/browser/resources/gaia_auth_host/gaia_auth_host.js (right):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:177: * Invoked to
record that the SAML API was used.
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: Could you change this to "credentials passing API"? We had all sorts of
> names for this during development but "credentials passing API" is the name
that
> stuck.

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:394:
console.error('GaiaAuthHost: Invalid missingGaiaInfoCallback.');
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: s/missingGaiaInfoCallback/missingGaiaInfoCallback_/ (for consistency with
> the existing error messages)

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/resourc...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:403:
console.error('GaiaAuthHost: Invalid samlApiUsedCallback.');
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: s/samlApiUsedCallback/samlApiUsedCallback_/ (for consistency with the
> existing error messages)

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc (left):

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:10: #include
"base/bind_helpers.h"
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: This is still used in EnrollmentScreenHandler::FetchOAuthToken().

Done.

https://codereview.chromium.org/473153002/diff/1660001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:36: #include
"net/url_request/url_request_context_getter.h"
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: This is still used in EnrollmentScreenHandler::FetchOAuthToken().

Done.

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:454: if (!is_saml) {
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: You could simplify this by rversing the conditional:
> 
> if (is_saml)
>   return;

Done.

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:456:
http_response->AddCustomHeader("google-accounts-signin",
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: This should be extracted into a helper method that
HandleServiceLoginAuth()
> and HandleSSO() can call.

Done.

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.h (right):

https://codereview.chromium.org/473153002/diff/1660001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.h:132: std::string GetGaiaIdOfEmail(const
std::string& email);
On 2014/10/21 14:47:47, bartfab wrote:
> Nit: Make the method const.

Done.

[Roger Tawa OOO till Jul 10th, 2014-10-21 19:20:03]

rogerta@chromium.org changed reviewers:
+ achuith@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-10-21 19:20:06]

Hi Achuith, David,

Achuith: can you please do an owner review for:
tools/telemetry/telemetry/core/backends/chrome/chromeos_login_ext/main.js

David: can you please do an owner review for:
chromeos/login/auth/user_context.h
chromeos/login/auth/user_context.cc

Thanks.

[Roger Tawa OOO till Jul 10th, 2014-10-21 19:21:09]

rogerta@chromium.org changed reviewers:
+ davemoore@chromium.org

[achuithb, 2014-10-21 21:11:00]

https://codereview.chromium.org/473153002/diff/1700001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/login_common.js (right):

https://codereview.chromium.org/473153002/diff/1700001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/login_common.js:265: // TODO(rogerta):
expose gaiaId to caller?
Is there a bug-id that you could reference here? I don't think this TODO is
necessary since this is a 'fake' login.

What are the ramifications of gaiaId?

https://codereview.chromium.org/473153002/diff/1700001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/login_common.js:266:
chrome.send('completeLogin', ['fake-gaia-id', username, password, false]);
Is this going to work? I hope this won't break telemetry on chromeos.

https://codereview.chromium.org/473153002/diff/1700001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/backends/chrome/chromeos_login_ext/main.js
(right):

https://codereview.chromium.org/473153002/diff/1700001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/backends/chrome/chromeos_login_ext/main.js:15:
'gaiaId': '12345',
why not 'fake-gaia-id' as in the other place?

[Roger Tawa OOO till Jul 10th, 2014-10-21 21:35:26]

Thanks Achuith.

https://codereview.chromium.org/473153002/diff/1700001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/login_common.js (right):

https://codereview.chromium.org/473153002/diff/1700001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/login_common.js:265: // TODO(rogerta):
expose gaiaId to caller?
On 2014/10/21 21:11:00, achuithb wrote:
> Is there a bug-id that you could reference here? I don't think this TODO is
> necessary since this is a 'fake' login.
> 
> What are the ramifications of gaiaId?

This was more of a question than a bug.  I will remove the comment.  The gaiaid
is a number that uniquely identifies a gaia account.  The email is not good
enough because it can change over time, and some characters like "." are not
significant.

https://codereview.chromium.org/473153002/diff/1700001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/login_common.js:266:
chrome.send('completeLogin', ['fake-gaia-id', username, password, false]);
On 2014/10/21 21:11:00, achuithb wrote:
> Is this going to work? I hope this won't break telemetry on chromeos.

Oops, should be a numeric field.  Done.

https://codereview.chromium.org/473153002/diff/1700001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/backends/chrome/chromeos_login_ext/main.js
(right):

https://codereview.chromium.org/473153002/diff/1700001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/backends/chrome/chromeos_login_ext/main.js:15:
'gaiaId': '12345',
On 2014/10/21 21:11:00, achuithb wrote:
> why not 'fake-gaia-id' as in the other place?

A gaiaid is a long decimal number, so I think this is better.  I changed the
other place to use a number too.

[bartfab (slow), 2014-10-22 08:31:32]

Still LGTM. Ship it.

https://codereview.chromium.org/473153002/diff/1740001/google_apis/gaia/fake_...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/473153002/diff/1740001/google_apis/gaia/fake_...
google_apis/gaia/fake_gaia.cc:466: return;
Nit: Now the early return no longer simplifies anything because the code that
used to follow has shrunk to just one line :).

[xiyuan, 2014-10-22 16:21:48]

LGTM

Please also run the following command to exercise VMTest. The bots should send
you an emails when they are done. 

cbuildbot --remote -G <review_id> x86-generic-tot-chrome-pfq-informational
amd64-generic-tot-chrome-pfq-informational daisy-tot-chrome-pfq-informational

where <review_id> is 473153002 for this CL.

[Roger Tawa OOO till Jul 10th, 2014-10-22 16:59:06]

Thanks Xiyuan.  I already ran the cbuildbot tests and all passed, you can see
results here:

https://uberchromegw.corp.google.com/i/chromiumos.tryserver/waterfall?committ...

[achuithb, 2014-10-22 17:19:42]

On 2014/10/22 16:59:06, Roger Tawa wrote:
> Thanks Xiyuan.  I already ran the cbuildbot tests and all passed, you can see
> results here:
> 
>
https://uberchromegw.corp.google.com/i/chromiumos.tryserver/waterfall?committ...

Lgtm, thanks!

[DaveMoore, 2014-10-22 19:16:22]

cros LGTM

[Roger Tawa OOO till Jul 10th, 2014-10-22 19:20:23]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-10-22 19:22:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/473153002/1740001

[commit-bot: I haz the power, 2014-10-22 20:46:53]

Committed patchset #46 (id:1740001)

[commit-bot: I haz the power, 2014-10-22 20:53:12]

Patchset 46 (id:??) landed as
https://crrev.com/a8e7c94b1b79a0948d05a1fcfff53391d22ce37a
Cr-Commit-Position: refs/heads/master@{#300750}

[Nikita (slow), 2014-10-23 11:51:11]

A revert of this CL (patchset #46 id:1740001) has been created in
https://codereview.chromium.org/671183002/ by nkostylev@chromium.org.

The reason for reverting is: This breaks Chrome OS GAIA login.

After successful GAIA authentication uses sees blank white page and nothing
happens.

Please build chromeos=1 build and check that sign in as new user works fine
before relanding this CL. Unfortunately autotests rely on fake gaia server and
doesn't check this end-to-end sign in flow. .

[Nikita (slow), 2014-10-23 11:53:41]

nkostylev@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Nikita (slow), 2014-10-23 11:53:43]

I've verified that reverting this change fixes Chrome OS sign in.

https://codereview.chromium.org/473153002/diff/1740001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/login_common.js (right):

https://codereview.chromium.org/473153002/diff/1740001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/login_common.js:265:
chrome.send('completeLogin', ['12345', username, password, false]);
nit:

var test_gaia_id = '12345';
chrome.send('completeLogin', [test_gaia_id, username, password, false]);