Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988113004:
Clear the login webview when SAML flow is canceled (Closed)
|
Created:
4 years, 7 months ago by emaxx Modified:
4 years, 6 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear the login webview when SAML flow is canceled
Previously, the SAML webview running third-party website was kept
running in the background even after the SAML login flow is
canceled.
This CL adds a redirection of the SAML webview to a blank page
each time the login screen mode is changed.
BUG=613245
TEST=existing login tests, manual testing with going to camera-capturing
SAML web page and checking that the camera lid is off once the SAML flow
is canceled manually or terminated due to a timeout or canceled due to
network disconnection
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/9ac3388d7cb865125b9af3c4e562a9677412e206
Cr-Commit-Position: refs/heads/master@{#399769}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Reset SAML webview with 'about:blank'. Reset when network is disconnected. #
Total comments: 3
Patch Set 3 : Add comment #Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : Fix test failures: fire 'ready' event for 'about:blank' page too #Patch Set 7 : Reset the webview only when the containing screen was shown and is about to be hidden #Patch Set 8 : Make LoginTest.GaiaAuthOffline test work #
Messages
Total messages: 38 (13 generated)
Description was changed from ========== Reload the GAIA auth host when resetting the GAIA signin screen This resolves the issue when the SAML webview was kept running in the background even after the SAML login flow is canceled. BUG=613245 ========== to ========== Reload the GAIA auth host when resetting the GAIA signin screen This resolves the issue when the SAML webview was kept running in the background even after the SAML login flow is canceled. BUG=613245 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
emaxx@chromium.org changed reviewers: + achuith@chromium.org, cernekee@chromium.org
Hi Achuith and Kevin, PTAL. https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:926: this.gaiaAuthHost_.reload(); Authenticator.prototype.reload does the same as Authenticator.prototype.resetStates_, but additionally resets the webview URL to the initial one. (Not sure why the reload method was not used here initially.) Achuith, am I correct that the initial URL is always pointing to a Google-owned page, so that it's safe to leave the webview with it opened in the background?
xiyuan@chromium.org changed reviewers: + afakhry@chromium.org, xiyuan@chromium.org
+afakhry https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:926: this.gaiaAuthHost_.reload(); On 2016/05/19 19:02:16, emaxx wrote: > Authenticator.prototype.reload does the same as > Authenticator.prototype.resetStates_, but additionally resets the webview URL to > the initial one. > (Not sure why the reload method was not used here initially.) > > > Achuith, am I correct that the initial URL is always pointing to a Google-owned > page, so that it's safe to leave the webview with it opened in the background? We have a policy to make Gaia auto redirect to saml idp. And if that policy is enabled and our saml idp is using webcam, this change would not really fix the problem.
On 2016/05/19 22:47:09, xiyuan wrote: > +afakhry > > https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... > File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): > > https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/login/screen_gaia_signin.js:926: > this.gaiaAuthHost_.reload(); > On 2016/05/19 19:02:16, emaxx wrote: > > Authenticator.prototype.reload does the same as > > Authenticator.prototype.resetStates_, but additionally resets the webview URL > to > > the initial one. > > (Not sure why the reload method was not used here initially.) > > > > > > Achuith, am I correct that the initial URL is always pointing to a > Google-owned > > page, so that it's safe to leave the webview with it opened in the background? > > We have a policy to make Gaia auto redirect to saml idp. And if that policy is > enabled and our saml idp is using webcam, this change would not really fix the > problem. What Xiyuan said. I believe Ahmed implemented this.
https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:926: this.gaiaAuthHost_.reload(); On 2016/05/19 22:47:08, xiyuan wrote: > On 2016/05/19 19:02:16, emaxx wrote: > > Authenticator.prototype.reload does the same as > > Authenticator.prototype.resetStates_, but additionally resets the webview URL > to > > the initial one. > > (Not sure why the reload method was not used here initially.) > > > > > > Achuith, am I correct that the initial URL is always pointing to a > Google-owned > > page, so that it's safe to leave the webview with it opened in the background? > > We have a policy to make Gaia auto redirect to saml idp. And if that policy is > enabled and our saml idp is using webcam, this change would not really fix the > problem. Authenticator.prototype.reload() doesn't reset the GAIA sign in screen. It will assign webview_.src to the same reloadUrl_ that was most recently calculated in the most recent load(). Calling reload() here will reload the same SAML IdP Url again in the Webview (assuming the previous load() loaded that SAML IdP page). I suggest (as xiyuan mention on the bug thread) to keep using resetStates_(); but modify it to reset webview_.src to some default URL, say about:blank or something.
https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1988113004/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:926: this.gaiaAuthHost_.reload(); On 2016/05/20 00:14:21, afakhry wrote: > On 2016/05/19 22:47:08, xiyuan wrote: > > On 2016/05/19 19:02:16, emaxx wrote: > > > Authenticator.prototype.reload does the same as > > > Authenticator.prototype.resetStates_, but additionally resets the webview > URL > > to > > > the initial one. > > > (Not sure why the reload method was not used here initially.) > > > > > > > > > Achuith, am I correct that the initial URL is always pointing to a > > Google-owned > > > page, so that it's safe to leave the webview with it opened in the > background? > > > > We have a policy to make Gaia auto redirect to saml idp. And if that policy is > > enabled and our saml idp is using webcam, this change would not really fix the > > problem. > > Authenticator.prototype.reload() doesn't reset the GAIA sign in screen. It will > assign webview_.src to the same reloadUrl_ that was most recently calculated in > the most recent load(). > > Calling reload() here will reload the same SAML IdP Url again in the Webview > (assuming the previous load() loaded that SAML IdP page). I suggest (as xiyuan > mention on the bug thread) to keep using resetStates_(); but modify it to reset > webview_.src to some default URL, say about:blank or something. OK, restored this back to calling resetStates_. And added into resetStates_ the redirection of webview to 'about:blank'. https://codereview.chromium.org/1988113004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1988113004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:596: this.gaiaAuthHost_.resetStates(); This (plus the new line 601) resolves the issue that the SAML webview was still working in the background, when the network becomes disconnected. The network disconnection results in loading the first screen, which is performed by the GaiaSigninScreen.loadAuthExtension method call. With this patch, this results in calling resetStates() on all code paths (note: when this.screenMode_==ScreenMode.DEFAULT, GaiaSigninScreen.loadGaiaAuthHost_ will call resetStates itself). https://codereview.chromium.org/1988113004/diff/20001/chrome/browser/resource... File chrome/browser/resources/gaia_auth_host/authenticator.js (left): https://codereview.chromium.org/1988113004/diff/20001/chrome/browser/resource... chrome/browser/resources/gaia_auth_host/authenticator.js:199: Authenticator.prototype.resetStates_ = function() { Removed trailing underscore, as this method is called from other classes and is therefore public.
lgtm
On 2016/05/24 19:27:12, xiyuan wrote: > lgtm Please update the CL description as well.
lgtm https://codereview.chromium.org/1988113004/diff/20001/chrome/browser/resource... File chrome/browser/resources/gaia_auth_host/authenticator.js (right): https://codereview.chromium.org/1988113004/diff/20001/chrome/browser/resource... chrome/browser/resources/gaia_auth_host/authenticator.js:216: this.webview_.src = 'about:blank'; Is it worthwhile adding a short comment here about why we're doing this?
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988113004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988113004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Reload the GAIA auth host when resetting the GAIA signin screen This resolves the issue when the SAML webview was kept running in the background even after the SAML login flow is canceled. BUG=613245 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Clear the login webview when resetting the GAIA signin screen Previously, the SAML webview running third-party website was kept running in the background even after the SAML login flow is canceled. This CL adds a redirection of the SAML webview to a blank page each time the login screen mode is changed. BUG=613245 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
emaxx@chromium.org changed reviewers: - cernekee@chromium.org
Hi all, looks like I'm stuck with trying to make all tests work. The problem with the previously reviewed iteration was that many login-related tests failed because they blocked on waiting for "ready" event from the JS side, which never occurred. The reason for this was that opening "about:blank" page in a webview does not fire the "contentload" event (whose listener is normally responsible for firing the "ready" event). I tried to fix in the latest patchset, by sending the "ready" event also when a "loadstop" event is received from a webview if the "about:blank" page is currently opened. Also I tweaked the code so that it doesn't redirect to "about:blank" if the webview is already blank (either because it's just created or it's just cleared). However, this is still not a good solution, because this potentially introduces premature "ready" events: if, for example, webview is initially redirected to some URL, then reset to "about:blank" and then again redirected to another URL. The "ready" event would be sent before the final redirection happens. I'm not sure whether such situations can occur in any real scenarios, but this is what happening in the WebviewLoginTest.EmailPrefill test. Has anybody any ideas how to deal with this stuff?
On 2016/06/08 19:21:20, emaxx wrote: > Hi all, looks like I'm stuck with trying to make all tests work. > > The problem with the previously reviewed iteration was that many > login-related tests failed because they blocked on waiting for > "ready" event from the JS side, which never occurred. The reason > for this was that opening "about:blank" page in a webview does not > fire the "contentload" event (whose listener is normally > responsible for firing the "ready" event). > > > I tried to fix in the latest patchset, by sending the "ready" > event also when a "loadstop" event is received from a webview > if the "about:blank" page is currently opened. > Also I tweaked the code so that it doesn't redirect to > "about:blank" if the webview is already blank (either because it's > just created or it's just cleared). > > However, this is still not a good solution, because this > potentially introduces premature "ready" events: if, for example, > webview is initially redirected to some URL, then reset to > "about:blank" and then again redirected to another URL. The > "ready" event would be sent before the final redirection happens. > > I'm not sure whether such situations can occur in any real > scenarios, but this is what happening in the > WebviewLoginTest.EmailPrefill test. > > > Has anybody any ideas how to deal with this stuff? How about loading this.idpOrigin_ + EMBEDDED_SETUP_CHROMEOS_ENDPOINT instead of 'about:blank' when we want to reset the webview's content?
On 2016/06/08 19:42:12, xiyuan wrote: > How about loading > > this.idpOrigin_ + EMBEDDED_SETUP_CHROMEOS_ENDPOINT > > instead of 'about:blank' when we want to reset the webview's content? Not sure about the benefits of this solution: switching to this URL inside resetStates() will result in potentially new 'contentload' event, which may lead to firing the 'ready' event earlier than it used to be. I'm starting to think that the whole approach with modifying resetStates() has to be changed to something else. With this approach, the new code in resetStates() is triggered both _before_ loading the IdP page and _after_ the login flow is canceled. Which makes these two cases hardly distinguishable, while they have to be distinguished for firing the 'ready' event in correct time. But I may be overseeing some easy trick that would do the stuff.
I thought we are trying to the get 'ready' event to be fired and use that url would have the 'ready' event fired. But looking at the thread again, it occurs to me that not firing 'ready' event after resetState should be the right thing. If we reset and not load Gaia again, there should be no 'ready' event. So we should not be doing that. What happens when the test fails? Can you clarify which part is expecting the 'ready' event? Is it that error screen shows up? If so, we need to clear the loading timer as well when we do this.gaiaAuthHost_.resetStates().
Description was changed from ========== Clear the login webview when resetting the GAIA signin screen Previously, the SAML webview running third-party website was kept running in the background even after the SAML login flow is canceled. This CL adds a redirection of the SAML webview to a blank page each time the login screen mode is changed. BUG=613245 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Clear the login webview when SAML flow is canceled Previously, the SAML webview running third-party website was kept running in the background even after the SAML login flow is canceled. This CL adds a redirection of the SAML webview to a blank page each time the login screen mode is changed. BUG=613245 TEST=existing login tests, manual testing with going to camera-capturing SAML web page and checking that the camera lid is off once the SAML flow is canceled manually or terminated due to a timeout or canceled due to network disconnection CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
On 2016/06/09 17:18:41, xiyuan wrote: > I thought we are trying to the get 'ready' event to be fired and use that url > would have the 'ready' event fired. But looking at the thread again, it occurs > to me that not firing 'ready' event after resetState should be the right thing. > If we reset and not load Gaia again, there should be no 'ready' event. So we > should not be doing that. > > What happens when the test fails? Can you clarify which part is expecting the > 'ready' event? Is it that error screen shows up? If so, we need to clear the > loading timer as well when we do this.gaiaAuthHost_.resetStates(). Yes, looks like a lot in the existing code is based on the knowledge of the exact situations that can lead in firing the 'ready' event. So we should avoid firing 'ready' event where it was not fired previously. When the WebviewLoginTest.EmailPrefill test failed due to my changes, it was because the 'ready' event happened too early: when the OobeBaseTest::WaitForGaiaPageReload method returned, the form fields were not set up as expected yet. When the LoginTest.GaiaAuthOffline test failed due to my changes, it hanged while waiting for the second 'ready' event that was never fired. Look at LoginTest::StartGaiaAuthOffline for details. P.S. I was wrong in one of my previous messages stating that 'contentload' is not fired for 'about:blank', sorry for that. I was confused by the tricky execution flows in the login code.
I changed the implementation now so that it resets the webview only when containing screen was shown and is about to be hidden. Now all the tests pass (and the manual tests - see the issue description - work too). However, the implementation doesn't look clean to me. I added a new method resetWebview into the Authenticator class, which is called from the GaiaSigninScreen.loadAuthExtension method each time the login screen mode is switching from DEFAULT. PTAL.
On 2016/06/13 15:16:02, emaxx wrote: > I changed the implementation now so that it resets the webview only when > containing screen was shown and is about to be hidden. > > Now all the tests pass (and the manual tests - see the issue description > - work too). > > However, the implementation doesn't look clean to me. I added a new > method resetWebview into the Authenticator class, which is called from > the GaiaSigninScreen.loadAuthExtension method each time the login screen > mode is switching from DEFAULT. > > PTAL. LGTM!
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988113004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by emaxx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988113004/140001
Message was sent while issue was closed.
Description was changed from ========== Clear the login webview when SAML flow is canceled Previously, the SAML webview running third-party website was kept running in the background even after the SAML login flow is canceled. This CL adds a redirection of the SAML webview to a blank page each time the login screen mode is changed. BUG=613245 TEST=existing login tests, manual testing with going to camera-capturing SAML web page and checking that the camera lid is off once the SAML flow is canceled manually or terminated due to a timeout or canceled due to network disconnection CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Clear the login webview when SAML flow is canceled Previously, the SAML webview running third-party website was kept running in the background even after the SAML login flow is canceled. This CL adds a redirection of the SAML webview to a blank page each time the login screen mode is changed. BUG=613245 TEST=existing login tests, manual testing with going to camera-capturing SAML web page and checking that the camera lid is off once the SAML flow is canceled manually or terminated due to a timeout or canceled due to network disconnection CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Clear the login webview when SAML flow is canceled Previously, the SAML webview running third-party website was kept running in the background even after the SAML login flow is canceled. This CL adds a redirection of the SAML webview to a blank page each time the login screen mode is changed. BUG=613245 TEST=existing login tests, manual testing with going to camera-capturing SAML web page and checking that the camera lid is off once the SAML flow is canceled manually or terminated due to a timeout or canceled due to network disconnection CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Clear the login webview when SAML flow is canceled Previously, the SAML webview running third-party website was kept running in the background even after the SAML login flow is canceled. This CL adds a redirection of the SAML webview to a blank page each time the login screen mode is changed. BUG=613245 TEST=existing login tests, manual testing with going to camera-capturing SAML web page and checking that the camera lid is off once the SAML flow is canceled manually or terminated due to a timeout or canceled due to network disconnection CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/9ac3388d7cb865125b9af3c4e562a9677412e206 Cr-Commit-Position: refs/heads/master@{#399769} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9ac3388d7cb865125b9af3c4e562a9677412e206 Cr-Commit-Position: refs/heads/master@{#399769}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2068953003/ by jdufault@chromium.org. The reason for reverting is: Makes SAMLPolicyTest.SAMLInterstitialNext extremely flaky. See crbug.com/620353. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
