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

Issue 171533014: Revert of Implement inline signin with iframe (Closed)

Created:
6 years, 10 months ago by guohui
Modified:
6 years, 10 months ago
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

Revert of Implement inline signin with iframe (https://codereview.chromium.org/134263005/) Reason for revert: 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. Original issue's 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 TBR=xiyuan@chromium.org,jam@chromium.org,nasko@chromium.org,rogerta@chromium.org NOTREECHECKS=true NOTRY=true BUG=338127

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+347 lines, -557 lines) Patch
M chrome/browser/browser_resources.grd View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/extensions/signin/gaia_auth_extension_loader.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc View 2 chunks +0 lines, -9 lines 0 comments Download
M chrome/browser/resources/component_extension_resources.grd View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/resources/gaia_auth/background.js View 5 chunks +19 lines, -136 lines 0 comments Download
D chrome/browser/resources/gaia_auth/desktop_injected.js View 1 chunk +0 lines, -38 lines 0 comments Download
A chrome/browser/resources/gaia_auth/inline_injected.js View 1 chunk +46 lines, -0 lines 0 comments Download
A chrome/browser/resources/gaia_auth/inline_main.html View 1 chunk +13 lines, -0 lines 0 comments Download
M chrome/browser/resources/gaia_auth/main.js View 7 chunks +141 lines, -98 lines 0 comments Download
D chrome/browser/resources/gaia_auth/manifest_desktop.json View 1 chunk +0 lines, -32 lines 0 comments Download
A chrome/browser/resources/gaia_auth/manifest_inline.json View 1 chunk +32 lines, -0 lines 0 comments Download
M chrome/browser/resources/gaia_auth_host/gaia_auth_host.js View 9 chunks +57 lines, -10 lines 0 comments Download
M chrome/browser/resources/inline_login/inline_login.js View 1 chunk +4 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 +2 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/signin/inline_login_handler_impl.cc View 11 chunks +19 lines, -12 lines 0 comments Download
M chrome/browser/ui/webui/signin/inline_login_ui.cc View 3 chunks +1 line, -4 lines 0 comments Download
D chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc View 1 chunk +0 lines, -163 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/webui/url_data_manager_backend.cc View 4 chunks +8 lines, -44 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
guohui
Created Revert of Implement inline signin with iframe
6 years, 10 months ago (2014-02-20 01:47:56 UTC) #1
xiyuan
Isn't r251503 already reverted in r251908 by dxie@chromium.org?
6 years, 10 months ago (2014-02-20 01:52:29 UTC) #2
Roger Tawa OOO till Jul 10th
On 2014/02/20 01:52:29, xiyuan wrote: > Isn't r251503 already reverted in r251908 by dxie@chromium.org? Daniel ...
6 years, 10 months ago (2014-02-20 01:53:00 UTC) #3
guohui1
he reverted on M34, not trunk. On Wed, Feb 19, 2014 at 8:52 PM, <xiyuan@chromium.org> ...
6 years, 10 months ago (2014-02-20 01:54:23 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/171533014/1
6 years, 10 months ago (2014-02-20 02:07:34 UTC) #5
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-20 02:07:40 UTC) #6
commit-bot: I haz the power
Failed to apply patch for chrome/browser/browser_resources.grd: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 10 months ago (2014-02-20 02:07:41 UTC) #7
guohui
The CQ bit was checked by guohui@chromium.org
6 years, 10 months ago (2014-02-20 02:08:04 UTC) #8
guohui
The CQ bit was unchecked by guohui@chromium.org
6 years, 10 months ago (2014-02-20 02:15:59 UTC) #9
guohui
6 years, 10 months ago (2014-02-20 02:21:20 UTC) #10
reverted in 173193005 using drover without conflict, not sure why ...

Powered by Google App Engine
This is Rietveld 408576698