Retrieve the authenticated user's e-mail from GAIA during SAML login

Retrieve the authenticated user's e-mail from GAIA during SAML login

When using SAML, the e-mail address typed into the GAIA login form and
the e-mail address of the user who actually ends up authenticating may
not match. This CL ensures that the correct e-mail address gets used by
querying GAIA for the authenticated user's e-mail instead of using the
e-mail obtained from the GAIA login form.

BUG=332132
TEST=New browser test

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

[bartfab (slow), 2014-01-13 12:29:22]

Hi Xiyuan,

Could you please review the entire CL?

Hi Roger,

Could you please review the fake_gaia.[cc|h] changes? The actual change is
minimal (add /GetUserInfo) but I also took this opportunity to reorder the
methods in the implementation to match the definition order.

[Nikita (slow), 2014-01-13 13:07:15]

Bartosz,

I'm currently experimenting with using <webview> version of GAIA extension on
Chrome OS (https://code.google.com/p/chromium/issues/detail?id=329224).

This means that this feature you're now adding will be broken. This means that
once I'll have prototype working I'll have to copy support you're adding in this
CL into the <webview> version. 

One issue is that there seems to be no reliable test that makes sure that this
flow works, right? So we'll have to retest whether this still works manually.

[bartfab (slow), 2014-01-13 13:24:06]

On 2014/01/13 13:07:15, Nikita Kostylev wrote:
> Bartosz,
> 
> I'm currently experimenting with using <webview> version of GAIA extension on
> Chrome OS (https://code.google.com/p/chromium/issues/detail?id=329224).
> 
> This means that this feature you're now adding will be broken. This means that
> once I'll have prototype working I'll have to copy support you're adding in
this
> CL into the <webview> version. 
> 
> One issue is that there seems to be no reliable test that makes sure that this
> flow works, right? So we'll have to retest whether this still works manually.

We are actively working to make SAML work on Chrome OS. This will involve a lot
of manual and automated testing. I would not expect critical failures to slip
past us unnoticed. Even the current code has a browser test that should verify
basic functionality. But yes, you are right that additional work will be needed
when the switch to <webview> happens. What is the ETA for this?

[Nikita (slow), 2014-01-13 13:31:34]

On 2014/01/13 13:24:06, bartfab wrote:
> On 2014/01/13 13:07:15, Nikita Kostylev wrote:
> > Bartosz,
> > 
> > I'm currently experimenting with using <webview> version of GAIA extension
on
> > Chrome OS (https://code.google.com/p/chromium/issues/detail?id=329224).
> > 
> > This means that this feature you're now adding will be broken. This means
that
> > once I'll have prototype working I'll have to copy support you're adding in
> this
> > CL into the <webview> version. 
> > 
> > One issue is that there seems to be no reliable test that makes sure that
this
> > flow works, right? So we'll have to retest whether this still works
manually.
> 
> We are actively working to make SAML work on Chrome OS. This will involve a
lot
> of manual and automated testing. I would not expect critical failures to slip
> past us unnoticed. Even the current code has a browser test that should verify
> basic functionality. But yes, you are right that additional work will be
needed
> when the switch to <webview> happens. What is the ETA for this?

Hopefully something will be part of M34. Initial plans: make sure that it works
the same i.e. no regressions (incl. SAML).
Next - use that GAIA implementation for create account flow, but that's a
different scope.

[xiyuan, 2014-01-13 22:05:58]

LGTM

Thanks for exploring and finding a solution.

https://codereview.chromium.org/136573002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/saml_browsertest.cc (right):

https://codereview.chromium.org/136573002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml_browsertest.cc:252:
GetLoginDisplay()->ShowSigninScreenForCreds(kUserEmail, "");
Think this should be |gaia_email|.

https://codereview.chromium.org/136573002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml_browsertest.cc:411: // |bob@example|.com via
SAML. Verifies that the logged-in user is correctly
nit: |bob@example|.com -> |bob@example.com|

https://codereview.chromium.org/136573002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/136573002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:262: class
SigninScreenHandler::AuthenticatedUserEmailRetriever
This class seem generic enough to live out of SigninScreenHandler. Let's put it
in its own file.

[bartfab (slow), 2014-01-14 14:41:32]

https://codereview.chromium.org/136573002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/saml_browsertest.cc (right):

https://codereview.chromium.org/136573002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml_browsertest.cc:252:
GetLoginDisplay()->ShowSigninScreenForCreds(kUserEmail, "");
On 2014/01/13 22:05:59, xiyuan wrote:
> Think this should be |gaia_email|.

Right. Otherwise I am not actually testing that the e-mail gets fetched
correctly. Nice catch.

https://codereview.chromium.org/136573002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml_browsertest.cc:411: // |bob@example|.com via
SAML. Verifies that the logged-in user is correctly
On 2014/01/13 22:05:59, xiyuan wrote:
> nit: |bob@example|.com -> |bob@example.com|

Done.

https://codereview.chromium.org/136573002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/136573002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:262: class
SigninScreenHandler::AuthenticatedUserEmailRetriever
On 2014/01/13 22:05:59, xiyuan wrote:
> This class seem generic enough to live out of SigninScreenHandler. Let's put
it
> in its own file.

Done.

[commit-bot: I haz the power, 2014-01-14 14:42:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/136573002/110001

[commit-bot: I haz the power, 2014-01-14 14:55:17]

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

[Roger Tawa OOO till Jul 10th, 2014-01-14 17:00:32]

lgtm for fake_gaia, but might want to have Zel take a look too.

[bartfab (slow), 2014-01-14 17:05:54]

Hi Zel,

Could you take a look at fake_gaia.[cc|h]?

[zel, 2014-01-14 17:55:46]

lgtm

[commit-bot: I haz the power, 2014-01-14 18:28:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/136573002/110001

[commit-bot: I haz the power, 2014-01-14 23:23:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/136573002/110001

[commit-bot: I haz the power, 2014-01-15 02:51:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/136573002/110001

[commit-bot: I haz the power, 2014-01-16 02:59:47]

Change committed as 245081