Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(9)

Issue 134263005: Implement inline signin with iframe (Closed)

Created:
6 years, 10 months ago by Roger Tawa OOO till Jul 10th
Modified:
6 years, 9 months ago
Reviewers:
xiyuan, jam, guohui, nasko
CC:
chromium-reviews, extensions-reviews_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, arv+watch_chromium.org, chromium-apps-reviews_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Address review comments #

Patch Set 3 : rebased #

Patch Set 4 : rebased #

Patch Set 5 : Make embedded view messaging work #

Total comments: 4

Patch Set 6 : Added browser tests for inline sign in #

Patch Set 7 : Rebased and merged #

Patch Set 8 : Better fix for messaging #

Patch Set 9 : Better fix for message, take 2. Fix CRLF #

Patch Set 10 : rebased #

Patch Set 11 : Fix endless loop when switching to full tab #

Patch Set 12 : rebased #

Patch Set 13 : Test for no webui in iframe #

Total comments: 2

Patch Set 14 : rebased #

Patch Set 15 : Fix typo #

Total comments: 2

Patch Set 16 : Don't check title1.html ids #

Total comments: 1

Patch Set 17 : Reapply CL #

Patch Set 18 : fix for various iframe bugs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+511 lines, -354 lines) Patch
M chrome/browser/browser_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +10 lines, -1 line 0 comments Download
M chrome/browser/extensions/signin/gaia_auth_extension_loader.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/resources/component_extension_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/resources/gaia_auth/background.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +138 lines, -19 lines 0 comments Download
A + chrome/browser/resources/gaia_auth/desktop_injected.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +6 lines, -15 lines 0 comments Download
D chrome/browser/resources/gaia_auth/inline_injected.js View 1 chunk +0 lines, -46 lines 0 comments Download
D chrome/browser/resources/gaia_auth/inline_main.html View 1 chunk +0 lines, -13 lines 0 comments Download
M chrome/browser/resources/gaia_auth/main.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 7 chunks +90 lines, -121 lines 0 comments Download
A + chrome/browser/resources/gaia_auth/manifest_desktop.json View 1 2 3 4 5 6 2 chunks +3 lines, -9 lines 0 comments Download
D chrome/browser/resources/gaia_auth/manifest_inline.json View 1 chunk +0 lines, -32 lines 0 comments Download
M chrome/browser/resources/gaia_auth_host/gaia_auth_host.js View 1 2 3 4 5 6 7 8 9 9 chunks +10 lines, -57 lines 0 comments Download
M chrome/browser/resources/inline_login/inline_login.js View 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_sync_starter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +5 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/signin/inline_login_handler.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/signin/inline_login_handler_impl.h View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/webui/signin/inline_login_handler_impl.cc View 11 chunks +12 lines, -19 lines 0 comments Download
M chrome/browser/ui/webui/signin/inline_login_ui.cc View 1 2 3 4 5 6 7 8 3 chunks +4 lines, -1 line 0 comments Download
A chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +163 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/webui/url_data_manager_backend.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +53 lines, -8 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
Roger Tawa OOO till Jul 10th
Hi guys, I cannot upload to Hui's original CL, so I had to create a ...
6 years, 10 months ago (2014-02-07 16:19:37 UTC) #1
xiyuan
LGTM after fixing the issue mentioned. https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gaia_auth/background.js File chrome/browser/resources/gaia_auth/background.js (right): https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gaia_auth/background.js#newcode126 chrome/browser/resources/gaia_auth/background.js:126: var tabId = ...
6 years, 10 months ago (2014-02-07 17:52:14 UTC) #2
Roger Tawa OOO till Jul 10th
Thanks Xiyuan. Changes uploaded. https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gaia_auth/background.js File chrome/browser/resources/gaia_auth/background.js (right): https://codereview.chromium.org/134263005/diff/1/chrome/browser/resources/gaia_auth/background.js#newcode126 chrome/browser/resources/gaia_auth/background.js:126: var tabId = getTabIdFromPort_(port); On ...
6 years, 10 months ago (2014-02-07 19:36:50 UTC) #3
guohui
https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc File chrome/browser/ui/webui/signin/inline_login_ui.cc (left): https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc#oldcode51 chrome/browser/ui/webui/signin/inline_login_ui.cc:51: extensions::TabHelper::CreateForWebContents(web_ui->GetWebContents()); To fix the embedded signin flow, instead of ...
6 years, 10 months ago (2014-02-11 22:29:56 UTC) #4
xiyuan
Welcome back, Hui! https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc File chrome/browser/ui/webui/signin/inline_login_ui.cc (left): https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc#oldcode51 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: ...
6 years, 10 months ago (2014-02-11 22:39:55 UTC) #5
guohui
https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc File chrome/browser/ui/webui/signin/inline_login_ui.cc (left): https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc#oldcode51 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 ...
6 years, 10 months ago (2014-02-11 23:22:43 UTC) #6
Roger Tawa OOO till Jul 10th
All looks good now once r250360 is reverted. https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc File chrome/browser/ui/webui/signin/inline_login_ui.cc (left): https://codereview.chromium.org/134263005/diff/380001/chrome/browser/ui/webui/signin/inline_login_ui.cc#oldcode51 chrome/browser/ui/webui/signin/inline_login_ui.cc:51: extensions::TabHelper::CreateForWebContents(web_ui->GetWebContents()); ...
6 years, 10 months ago (2014-02-12 03:07:12 UTC) #7
jam
lgtm
6 years, 10 months ago (2014-02-12 03:56:40 UTC) #8
Roger Tawa OOO till Jul 10th
Thanks John for your help in writing the tests to make sure webui's cannot be ...
6 years, 10 months ago (2014-02-14 03:10:21 UTC) #9
nasko
LGTM with a nit. https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc#newcode50 chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:50: params.SetInteger("authMode", InlineLoginHandler::kDesktopAuthMode); nit: Why change ...
6 years, 10 months ago (2014-02-14 16:48:45 UTC) #10
Roger Tawa OOO till Jul 10th
https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/134263005/diff/2410001/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc#newcode50 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: ...
6 years, 10 months ago (2014-02-14 19:52:16 UTC) #11
Roger Tawa OOO till Jul 10th
The CQ bit was checked by rogerta@chromium.org
6 years, 10 months ago (2014-02-14 20:56:27 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerta@chromium.org/134263005/2520001
6 years, 10 months ago (2014-02-14 21:08:53 UTC) #13
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-14 23:00:02 UTC) #14
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=264710
6 years, 10 months ago (2014-02-14 23:00:04 UTC) #15
jam
lgtm https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc#newcode151 chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:151: // created when navigated to nromally. nit: normally
6 years, 10 months ago (2014-02-14 23:45:14 UTC) #16
Roger Tawa OOO till Jul 10th
Thanks John. https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/134263005/diff/2170001/chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc#newcode151 chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:151: // created when navigated to nromally. On ...
6 years, 10 months ago (2014-02-15 00:48:46 UTC) #17
Roger Tawa OOO till Jul 10th
The CQ bit was checked by rogerta@chromium.org
6 years, 10 months ago (2014-02-15 00:49:16 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerta@chromium.org/134263005/2520001
6 years, 10 months ago (2014-02-15 00:57:06 UTC) #19
commit-bot: I haz the power
Change committed as 251503
6 years, 10 months ago (2014-02-15 03:55:08 UTC) #20
guohui
https://codereview.chromium.org/134263005/diff/2520001/content/browser/webui/url_data_manager_backend.cc File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/134263005/diff/2520001/content/browser/webui/url_data_manager_backend.cc#newcode362 content/browser/webui/url_data_manager_backend.cc:362: allowed = true; Caused crbug/344355. Suspect we need to ...
6 years, 10 months ago (2014-02-19 15:38:35 UTC) #21
guohui
A revert of this CL has been created in https://codereview.chromium.org/171533014/ by guohui@chromium.org. The reason for ...
6 years, 10 months ago (2014-02-20 01:47:55 UTC) #22
mtomasz
6 years, 10 months ago (2014-02-20 05:25:15 UTC) #23
Message was sent while issue was closed.
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).

Powered by Google App Engine
This is Rietveld 408576698