Reimplement inline signin with iframe

Implement inline signin with iframe

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.

COLLABORATOR=rogerta@chromium.org
BUG=338127
R=jam@chromium.org, xiyuan@chromium.org

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

[guohui, 2014-01-25 22:11:22]

Hey,

could you please take a look at the CL?

xiyuan and rogerta for
chrome/browser/resources/... and chrome/browser/ui/webui/signin/...

jam for
chrome/browser/chrome_content_browser_client.cc	
content/browser/webui/url_data_manager_backend.cc
chrome/browser/renderer_host/...

Feel free to review all if you like, thanks!

[xiyuan, 2014-01-26 01:59:57]

+nkostylev, bartfab fyi

Can you also run linux_chromeos bot? I am interested to see how OobeTest and
SamlTest goes.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:293: <include
name="IDR_GAIA_AUTH_DESKTOP_MANIFEST"
file="resources\gaia_auth\manifest_desktop.json" type="BINDATA" />
This file seems missed from the CL.

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

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:42: ui_initialized_: false,
nit: ui_initialized_ -> uiInitialized_

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:123:
gaiaFrame.addEventListener('load', function() {
'load' event is not a reliable. Think it could fire on some edge case such as a
portal page etc. It is better to have our script running inside the iframe
(injected content script or better to host JS code there) to post back a message
as load confirmation.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:146: var msg = {
|msg| is used for both function argument and local var. We should use different
names.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth_host/gaia_auth_host.js (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:338: chooseWhatToSync:
msg.chooseWhatToSync,
Where is msg.chooseWhatTosync set?

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui.cc:25:
source->DisableContentSecurityPolicy(); // So we can use iframe.
nit: Could we use OverrideContentSecurityPolicyFrameSrc instead of disabling all
CSP?
e.g.
source->OverrideContentSecurityPolicyFrameSrc("frame-src chrome-extension:;");

[jam, 2014-01-26 02:51:46]

Thanks for switching over quickly!

On 2014/01/25 22:11:22, guohui wrote:
> Hey,
> 
> could you please take a look at the CL?
> 
> xiyuan and rogerta for
> chrome/browser/resources/... and chrome/browser/ui/webui/signin/...
> 
> jam for
> chrome/browser/chrome_content_browser_client.cc	
> content/browser/webui/url_data_manager_backend.cc
> chrome/browser/renderer_host/...

Since I wrote these parts in the cl, another person should review. Added Nasko
for them.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:803: site.host() ==
chrome::kChromeUIChromeSigninHost) {
nit: site.GetOrigin().spec() == kChromeUIChromeSigninURL would be shorter

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:588: if
(request->first_party_for_cookies().SchemeIs(chrome::kChromeUIScheme) &&
nit: also here, request->first_party_for_cookies().spec() ==
kChromeUIChromeSigninURL would be shorter

[jam, 2014-01-26 02:52:13]

On 2014/01/26 02:51:46, jam wrote:
> Thanks for switching over quickly!
> 
> On 2014/01/25 22:11:22, guohui wrote:
> > Hey,
> > 
> > could you please take a look at the CL?
> > 
> > xiyuan and rogerta for
> > chrome/browser/resources/... and chrome/browser/ui/webui/signin/...
> > 
> > jam for
> > chrome/browser/chrome_content_browser_client.cc	
> > content/browser/webui/url_data_manager_backend.cc
> > chrome/browser/renderer_host/...
> 
> Since I wrote these parts in the cl, another person should review. Added Nasko
> for them.

btw owners lgtm for these files

[guohui, 2014-01-26 17:49:20]

On 2014/01/26 02:52:13, jam wrote:
> On 2014/01/26 02:51:46, jam wrote:
> > Thanks for switching over quickly!
> > 
> > On 2014/01/25 22:11:22, guohui wrote:
> > > Hey,
> > > 
> > > could you please take a look at the CL?
> > > 
> > > xiyuan and rogerta for
> > > chrome/browser/resources/... and chrome/browser/ui/webui/signin/...
> > > 
> > > jam for
> > > chrome/browser/chrome_content_browser_client.cc	
> > > content/browser/webui/url_data_manager_backend.cc
> > > chrome/browser/renderer_host/...
> > 
> > Since I wrote these parts in the cl, another person should review. Added
Nasko
> > for them.
> 
> btw owners lgtm for these files

just realized the current implementation does not work with multiple instances.
There is exactly one copy of background script shared by all instances of inline
signin pages, thus setupForAuthMain_ and setupForInjected_ are called multiple
times, and since background.js currently only keeps the last channel for main
script and the last channel for injected script, thus only the last main script
and the last injected script receive messages (including messages for other
instances) from background script.

To fix this, we could either store all opened channels keyed by tabID (not sure
if it works without a tab, such as in an avatar bubble), or we move the logic to
native code. However, CrOS has implemented the password scraping flow for SAML
in JS using background script as well, the same issue would apply if we want to
reuse their logic on desktop. Thus i ll try the JS option first.

Also the security fix in url_data_manager_backend.cc breaks the resource loading
in webUI, need to investigate why.

I am flying in the next 24 hours, will try to upload a new patch by the end of
Tuesday.

[xiyuan, 2014-01-26 18:17:34]

On 2014/01/26 17:49:20, guohui wrote:
> On 2014/01/26 02:52:13, jam wrote:
> > On 2014/01/26 02:51:46, jam wrote:
> > > Thanks for switching over quickly!
> > > 
> > > On 2014/01/25 22:11:22, guohui wrote:
> > > > Hey,
> > > > 
> > > > could you please take a look at the CL?
> > > > 
> > > > xiyuan and rogerta for
> > > > chrome/browser/resources/... and chrome/browser/ui/webui/signin/...
> > > > 
> > > > jam for
> > > > chrome/browser/chrome_content_browser_client.cc	
> > > > content/browser/webui/url_data_manager_backend.cc
> > > > chrome/browser/renderer_host/...
> > > 
> > > Since I wrote these parts in the cl, another person should review. Added
> Nasko
> > > for them.
> > 
> > btw owners lgtm for these files
> 
> just realized the current implementation does not work with multiple
instances.
> There is exactly one copy of background script shared by all instances of
inline
> signin pages, thus setupForAuthMain_ and setupForInjected_ are called multiple
> times, and since background.js currently only keeps the last channel for main
> script and the last channel for injected script, thus only the last main
script
> and the last injected script receive messages (including messages for other
> instances) from background script.
> 
> To fix this, we could either store all opened channels keyed by tabID (not
sure
> if it works without a tab, such as in an avatar bubble), or we move the logic
to
> native code. However, CrOS has implemented the password scraping flow for SAML
> in JS using background script as well, the same issue would apply if we want
to
> reuse their logic on desktop. Thus i ll try the JS option first.
> 
> Also the security fix in url_data_manager_backend.cc breaks the resource
loading
> in webUI, need to investigate why.
> 
> I am flying in the next 24 hours, will try to upload a new patch by the end of
> Tuesday.

Another problem to think about is to the injected content script needs to only
mess with the targeted iframes. ChromeOS login only has two iframes but the
desktop case is much more complex. We need someway to figure out whether the
iframe is loaded under gaia auth extension, e.g. via a postMessage or something.

[guohui1, 2014-01-26 19:14:48]

The same issue should apply to cros in saml flows as well?

I think we may identify the main gaia frame by checking the parent frame id
since we know id of the extension frame based on url.
On Jan 26, 2014 1:17 PM, <xiyuan@chromium.org> wrote:

> On 2014/01/26 17:49:20, guohui wrote:
>
>> On 2014/01/26 02:52:13, jam wrote:
>> > On 2014/01/26 02:51:46, jam wrote:
>> > > Thanks for switching over quickly!
>> > >
>> > > On 2014/01/25 22:11:22, guohui wrote:
>> > > > Hey,
>> > > >
>> > > > could you please take a look at the CL?
>> > > >
>> > > > xiyuan and rogerta for
>> > > > chrome/browser/resources/... and chrome/browser/ui/webui/signin/...
>> > > >
>> > > > jam for
>> > > > chrome/browser/chrome_content_browser_client.cc
>> > > > content/browser/webui/url_data_manager_backend.cc
>> > > > chrome/browser/renderer_host/...
>> > >
>> > > Since I wrote these parts in the cl, another person should review.
>> Added
>> Nasko
>> > > for them.
>> >
>> > btw owners lgtm for these files
>>
>
>  just realized the current implementation does not work with multiple
>>
> instances.
>
>> There is exactly one copy of background script shared by all instances of
>>
> inline
>
>> signin pages, thus setupForAuthMain_ and setupForInjected_ are called
>> multiple
>> times, and since background.js currently only keeps the last channel for
>> main
>> script and the last channel for injected script, thus only the last main
>>
> script
>
>> and the last injected script receive messages (including messages for
>> other
>> instances) from background script.
>>
>
>  To fix this, we could either store all opened channels keyed by tabID (not
>>
> sure
>
>> if it works without a tab, such as in an avatar bubble), or we move the
>> logic
>>
> to
>
>> native code. However, CrOS has implemented the password scraping flow for
>> SAML
>> in JS using background script as well, the same issue would apply if we
>> want
>>
> to
>
>> reuse their logic on desktop. Thus i ll try the JS option first.
>>
>
>  Also the security fix in url_data_manager_backend.cc breaks the resource
>>
> loading
>
>> in webUI, need to investigate why.
>>
>
>  I am flying in the next 24 hours, will try to upload a new patch by the
>> end of
>> Tuesday.
>>
>
> Another problem to think about is to the injected content script needs to
> only
> mess with the targeted iframes. ChromeOS login only has two iframes but the
> desktop case is much more complex. We need someway to figure out whether
> the
> iframe is loaded under gaia auth extension, e.g. via a postMessage or
> something.
>
> https://codereview.chromium.org/130963006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-01-27 16:49:29]

On 2014/01/26 17:49:20, guohui wrote:
> On 2014/01/26 02:52:13, jam wrote:
> > On 2014/01/26 02:51:46, jam wrote:
> > > Thanks for switching over quickly!
> > > 
> > > On 2014/01/25 22:11:22, guohui wrote:
> > > > Hey,
> > > > 
> > > > could you please take a look at the CL?
> > > > 
> > > > xiyuan and rogerta for
> > > > chrome/browser/resources/... and chrome/browser/ui/webui/signin/...
> > > > 
> > > > jam for
> > > > chrome/browser/chrome_content_browser_client.cc	
> > > > content/browser/webui/url_data_manager_backend.cc
> > > > chrome/browser/renderer_host/...
> > > 
> > > Since I wrote these parts in the cl, another person should review. Added
> Nasko
> > > for them.
> > 
> > btw owners lgtm for these files
> 
> just realized the current implementation does not work with multiple
instances.
> There is exactly one copy of background script shared by all instances of
inline
> signin pages, thus setupForAuthMain_ and setupForInjected_ are called multiple
> times, and since background.js currently only keeps the last channel for main
> script and the last channel for injected script, thus only the last main
script
> and the last injected script receive messages (including messages for other
> instances) from background script.
> 
> To fix this, we could either store all opened channels keyed by tabID (not
sure
> if it works without a tab, such as in an avatar bubble), or we move the logic
to
> native code. However, CrOS has implemented the password scraping flow for SAML
> in JS using background script as well, the same issue would apply if we want
to
> reuse their logic on desktop. Thus i ll try the JS option first.
> 
> Also the security fix in url_data_manager_backend.cc breaks the resource
loading
> in webUI, need to investigate why.

ah, sorry about that. fix in patchset 3 of
https://codereview.chromium.org/141363006/

[nasko, 2014-01-27 17:21:50]

In general the code looks good, but I'd like to see tests for the security
guarantees we expect of this change.

* Check that a separate storage partition is indeed used.
* Check that navigating away from the inline signin page results in a process
swap and storage partition swap.
* Check that navigation to any other chrome:// or chrome-extension:// URL is not
allowed.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:275: 
nit: no need for blank line.

[guohui, 2014-01-29 12:50:51]

most review comments addressed, except for the unit test. Please let me know if
anyone has other concerns.

I will upload a new patch to address the multi-instance issue addressed and to
add unit tests as suggested by nasko later today.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:293: <include
name="IDR_GAIA_AUTH_DESKTOP_MANIFEST"
file="resources\gaia_auth\manifest_desktop.json" type="BINDATA" />
On 2014/01/26 01:59:57, xiyuan wrote:
> This file seems missed from the CL.

Done.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:803: site.host() ==
chrome::kChromeUIChromeSigninHost) {
On 2014/01/26 02:51:46, jam wrote:
> nit: site.GetOrigin().spec() == kChromeUIChromeSigninURL would be shorter

Done.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:588: if
(request->first_party_for_cookies().SchemeIs(chrome::kChromeUIScheme) &&
On 2014/01/26 02:51:46, jam wrote:
> nit: also here, request->first_party_for_cookies().spec() ==
> kChromeUIChromeSigninURL would be shorter

Done.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:275: 
On 2014/01/27 17:21:50, nasko wrote:
> nit: no need for blank line.

Done.

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

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:42: ui_initialized_: false,
On 2014/01/26 01:59:57, xiyuan wrote:
> nit: ui_initialized_ -> uiInitialized_

Removed tha variable since it is not really needed.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:123:
gaiaFrame.addEventListener('load', function() {
On 2014/01/26 01:59:57, xiyuan wrote:
> 'load' event is not a reliable. Think it could fire on some edge case such as
a
> portal page etc. It is better to have our script running inside the iframe
> (injected content script or better to host JS code there) to post back a
message
> as load confirmation.

the sole purpose of this listener now is to remove the spinner once the UI is
ready in the desktop signin flows, thus we are only interested in the first load
event, and the portal case seems irrelevant? and i think we are pretty tolerant
of false positive, feel it is better to show the UI faster in most cases and
show a brief blank page in edges cases than always showing it slower by adding a
delay of async postMessage.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:146: var msg = {
On 2014/01/26 01:59:57, xiyuan wrote:
> |msg| is used for both function argument and local var. We should use
different
> names.

Done.

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth_host/gaia_auth_host.js (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js:338: chooseWhatToSync:
msg.chooseWhatToSync,
On 2014/01/26 01:59:57, xiyuan wrote:
> Where is msg.chooseWhatTosync set? 

it is set in desktop_injected.js (missing in this patch).

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/130963006/diff/210001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui.cc:25:
source->DisableContentSecurityPolicy(); // So we can use iframe.
On 2014/01/26 01:59:57, xiyuan wrote:
> nit: Could we use OverrideContentSecurityPolicyFrameSrc instead of disabling
all
> CSP?
> e.g.
> source->OverrideContentSecurityPolicyFrameSrc("frame-src chrome-extension:;");

Done.

[guohui, 2014-01-29 16:42:53]

Fixed the multi-instance issue in patch 3.

@nasko, could you please elaborate on what you mean by "check that navigation to
any other chrome:// or chrome-extension:// URL is not allowed"? I thought
navigate to other chrome:// is still allowed, it just triggers an automatic
renderer swap, as covered in your second point.

[guohui, 2014-01-29 16:49:34]

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:134:
this.channelInjected_[this.getTabIdFromPort_(port)] = currentChannel;
@xiyuan, it doesn't seem that channelInjected_ is read anywhere yet, is that
intended?

[xiyuan, 2014-01-29 18:09:06]

LGTM

Please fix my comments and watch for no VM test failure on the cros_xxx bots.

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (right):

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:134:
this.channelInjected_[this.getTabIdFromPort_(port)] = currentChannel;
On 2014/01/29 16:49:34, guohui wrote:
> @xiyuan, it doesn't seem that channelInjected_ is read anywhere yet, is that
> intended?

This is because so far the communication from injected code is one-way. But we
still need to keep an reference of it so that it goes not get disposed.

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

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:159: 'email': opt_extraMsg.email ||
this.email_,
This would be a runtime error when |opt_extraMsg| is not passed.

'email': (opt_extraMsg && opt_extraMsg.email) || this.email_,

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:162: 'chooseWhatToSync':
this.chooseWhatToSync_,
Set a default value for this.chooseWhatToSync_.
Or use 'this.chooseWhatToSync_ || false'

[guohui, 2014-02-20 20:27:10]

Committed patchset #4 manually as r252364 (presubmit successful).

[guohui, 2014-02-20 20:47:48]

For background on why we revive this old CL, please see 173723002.

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

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:159: 'email': opt_extraMsg.email ||
this.email_,
On 2014/01/29 18:09:07, xiyuan wrote:
> This would be a runtime error when |opt_extraMsg| is not passed.
> 
> 'email': (opt_extraMsg && opt_extraMsg.email) || this.email_,

Done.

https://codereview.chromium.org/130963006/diff/440001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/main.js:162: 'chooseWhatToSync':
this.chooseWhatToSync_,
On 2014/01/29 18:09:07, xiyuan wrote:
> Set a default value for this.chooseWhatToSync_.
> Or use 'this.chooseWhatToSync_ || false'

Done.