Implement inline signin with iframe

Implement inline signin with iframe

=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=
This is a dup of https://codereview.chromium.org/130963006/ since I cannot
upload to that issue.  The only change is to address Xiyuan's two comments
in patchset 3 of that CL.
=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=

Inline signin chrome://chrome-signin is currently implemented using webview embedded in webUI, which breaks a couple of features in webUI and has serious accessbility issues. Since webview will be reimplemented based on OOPIF in the near future, and all the issues we have today will no longer apply, thus it is not worth the effort to fix them as they are throw away work. Instead, as suggested by John and prototyped in https://codereview.chromium.org/141363006/, we decide to switch to iframe instead. A few issues worth to mention,

1. The iframe shares the same renderer as the embedder webUI, and thus could be potentially exposed to dangerous webUI privileges. John suggested a fix by assigning a unique storage partition ID to the inline signin page. As a result the inline signin and its embedded web content should never share the same renderer with other webUI pages.

2. webview provides a direct API to inject script and to monitor requests/responses, which is not (directly) available with iframe. The CL works around the issue using content script and background script, quite similar to what CrOS is doing for SAML flow today. Thus it is also the first step towards unifying SAML flows on CrOS and desktop.

3. with webview approach, we used to have a unique temporary partition for each instance of inline signin, in order to make sure multiple instances do not interfere with each other. This is more difficult with the iframe approach, since the partition ID is hardcoded in a quite low layer. In this CL, all inline signin pages share the same persistent partition, which means we have to handle the case when user loads the sign in page with a dirty cookie jar, and thus the newly connected user may not be stored in the primary session. The CL solves the issue by reading 'session_index' from 'google-accounts-signin' header.

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

[Roger Tawa OOO till Jul 10th, 2014-02-07 16:19:37]

Hi guys,

I cannot upload to Hui's original CL, so I had to create a new one.  This should
be the same as https://codereview.chromium.org/130963006/, except I addressed
Xiyuan's two comments.

I will next address Nasko's extra tests.

[xiyuan, 2014-02-07 17:52:14]

LGTM after fixing the issue mentioned.

https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gai...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/background.js:126: var tabId =
getTabIdFromPort_(port);
Sorry, this was missed from previous review. getTabIdFromPort_ needs to be
accessed from this.
i.e.
  var tabId = this.getTabIdFromPort_(port);

[Roger Tawa OOO till Jul 10th, 2014-02-07 19:36:50]

Thanks Xiyuan.  Changes uploaded.

https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gai...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gai...
chrome/browser/resources/gaia_auth/background.js:126: var tabId =
getTabIdFromPort_(port);
On 2014/02/07 17:52:15, xiyuan wrote:
> Sorry, this was missed from previous review. getTabIdFromPort_ needs to be
> accessed from this.
> i.e.
>   var tabId = this.getTabIdFromPort_(port);

Done.

[guohui, 2014-02-11 22:29:56]

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (left):

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui.cc:51:
extensions::TabHelper::CreateForWebContents(web_ui->GetWebContents());
To fix the embedded signin flow, instead of creating the extension tab helper in
profile_chooser_view, you might want to just revert the changes here. Both do
the same thing, except the code here makes sure regardless of where the inline
signin is triggered, it always creates an extension tab helper.

[xiyuan, 2014-02-11 22:39:55]

Welcome back, Hui!

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (left):

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui.cc:51:
extensions::TabHelper::CreateForWebContents(web_ui->GetWebContents());
On 2014/02/11 22:29:57, guohui wrote:
> To fix the embedded signin flow, instead of creating the extension tab helper
in
> profile_chooser_view, you might want to just revert the changes here. Both do
> the same thing, except the code here makes sure regardless of where the inline
> signin is triggered, it always creates an extension tab helper.

We need "ExtensionWebContentsObserver". But this looks like a better place than
profile_chooser_view

[guohui, 2014-02-11 23:22:43]

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (left):

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui.cc:51:
extensions::TabHelper::CreateForWebContents(web_ui->GetWebContents());
On 2014/02/11 22:39:55, xiyuan wrote:
> On 2014/02/11 22:29:57, guohui wrote:
> > To fix the embedded signin flow, instead of creating the extension tab
helper
> in
> > profile_chooser_view, you might want to just revert the changes here. Both
do
> > the same thing, except the code here makes sure regardless of where the
inline
> > signin is triggered, it always creates an extension tab helper.
> 
> We need "ExtensionWebContentsObserver". But this looks like a better place
than
> profile_chooser_view

I had similar issue before when implementing the embedded flow with webview, and
fixed it by adding the lines here. Thought it would apply to the iframe approach
as well.

@Roger, if these lines do not work as expected, then you might want to replace
the TabHelper with ExtensionWebContentsObserver as XIyuan suggested.

[Roger Tawa OOO till Jul 10th, 2014-02-12 03:07:12]

All looks good now once r250360 is reverted.

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (left):

https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui.cc:51:
extensions::TabHelper::CreateForWebContents(web_ui->GetWebContents());
On 2014/02/11 23:22:43, guohui wrote:
> On 2014/02/11 22:39:55, xiyuan wrote:
> > On 2014/02/11 22:29:57, guohui wrote:
> > > To fix the embedded signin flow, instead of creating the extension tab
> helper
> > in
> > > profile_chooser_view, you might want to just revert the changes here. Both
> do
> > > the same thing, except the code here makes sure regardless of where the
> inline
> > > signin is triggered, it always creates an extension tab helper.
> > 
> > We need "ExtensionWebContentsObserver". But this looks like a better place
> than
> > profile_chooser_view
> 
> I had similar issue before when implementing the embedded flow with webview,
and
> fixed it by adding the lines here. Thought it would apply to the iframe
approach
> as well.
> 
> @Roger, if these lines do not work as expected, then you might want to replace
> the TabHelper with ExtensionWebContentsObserver as XIyuan suggested.

Implemented Xiyuan's suggestion here.

[jam, 2014-02-12 03:56:40]

lgtm

[Roger Tawa OOO till Jul 10th, 2014-02-14 03:10:21]

Thanks John for your help in writing the tests to make sure webui's cannot be
loaded inside the iframe.  See patchset #13 for details.

Nasko: please review patchsets #6 and #13 for the tests you requested in Hui's
original CL.

Thanks.

[nasko, 2014-02-14 16:48:45]

LGTM with a nit.

https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:50:
params.SetInteger("authMode", InlineLoginHandler::kDesktopAuthMode);
nit: Why change the constant name from inline to desktop, while the class is
still called Inline?

[Roger Tawa OOO till Jul 10th, 2014-02-14 19:52:16]

https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:50:
params.SetInteger("authMode", InlineLoginHandler::kDesktopAuthMode);
On 2014/02/14 16:48:45, Nasko Oskov (OOO) wrote:
> nit: Why change the constant name from inline to desktop, while the class is
> still called Inline?

I'll talk to Hui about what the reason was, but I'll leave it like this for now.

[Roger Tawa OOO till Jul 10th, 2014-02-14 20:56:27]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:08:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/134263005/2520001

[commit-bot: I haz the power, 2014-02-14 23:00:02]

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

[commit-bot: I haz the power, 2014-02-14 23:00:04]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jam, 2014-02-14 23:45:14]

lgtm

https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right):

https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:151: // created
when navigated to nromally.
nit: normally

[Roger Tawa OOO till Jul 10th, 2014-02-15 00:48:46]

Thanks John.

https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right):

https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:151: // created
when navigated to nromally.
On 2014/02/14 23:45:14, jam wrote:
> nit: normally

Yup :-)  fixed in patchset 15

[Roger Tawa OOO till Jul 10th, 2014-02-15 00:49:16]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/134263005/2520001

[commit-bot: I haz the power, 2014-02-15 03:55:08]

Change committed as 251503

[guohui, 2014-02-19 15:38:35]

https://codereview.chromium.org/134263005/diff/2520001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/134263005/diff/2520001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:362: allowed = true;
Caused crbug/344355. Suspect we need to whitelist other chrome resources, such
as theme, thumb, favicon.

[guohui, 2014-02-20 01:47:55]

A revert of this CL has been created in
https://codereview.chromium.org/171533014/ by guohui@chromium.org.

The reason for reverting is: As discussed with Karen, this CL will hit canary
again by tongiht. Since we won't have a fix till tmrw noon (EST) at the
earliest, we need to revert it for now..

[mtomasz, 2014-02-20 05:25:15]

On 2014/02/20 01:47:55, guohui wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/171533014/ by mailto:guohui@chromium.org.
> 
> The reason for reverting is: As discussed with Karen, this CL will hit canary
> again by tongiht. Since we won't have a fix till tmrw noon (EST) at the
> earliest, we need to revert it for now..

Also caused crbug.com/344480, which has the same root cause (chrome:// not
accessible by Files app).