Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1983433002:
Add timeout for SAML webcam logins (Closed)
|
Created:
4 years, 7 months ago by Kevin Cernekee Modified:
4 years, 7 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@saml-ui Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd timeout for SAML webcam logins
In order to avoid leaving the camera on for extended periods of time,
ensure that SAML webcam logins time out after one minute. This has
the same effect as clicking the "X" button.
BUG=606979
TEST=manual
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/75619fb6b1c62cfce2ac70a0dab6c0e5ddbd1049
Cr-Commit-Position: refs/heads/master@{#394571}
Patch Set 1 #
Total comments: 1
Patch Set 2 : cancel timer if login is aborted #
Total comments: 2
Patch Set 3 : incorporate code review feedback #
Total comments: 2
Patch Set 4 : cancel timeout on auth completed #
Total comments: 8
Patch Set 5 : incorporate code review feedback #
Total comments: 2
Patch Set 6 : fix comment #Messages
Total messages: 23 (6 generated)
Description was changed from ========== Add timeout for SAML webcam logins In order to avoid leaving the camera on for extended periods of time, ensure that SAML webcam logins time out after one minute. This has the same effect as clicking the "X" button. BUG=606979 TEST=manual ========== to ========== Add timeout for SAML webcam logins In order to avoid leaving the camera on for extended periods of time, ensure that SAML webcam logins time out after one minute. This has the same effect as clicking the "X" button. BUG=606979 TEST=manual CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
cernekee@chromium.org changed reviewers: + bartfab@chromium.org, emaxx@chromium.org
https://codereview.chromium.org/1983433002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:651: this.videoTimer_ = setTimeout(this.cancel.bind(this), This timer needs to be canceled if the user clicks the "X" button or if login succeeds. I can't test the successful login flow right now. Do you have any thoughts on where in the code the "cancel timer" logic belongs? Is onAuthFlowChange a good candidate? I've also noticed that manually canceling + restarting the flow does not reset the videoEnabled flag, but maybe that should be reset elsewhere. FWIW I am testing all of this by clicking "Add person" on a Chromebook that is already owned by a test account. Other flows that probably need to be handled include: - Enterprise login with no default user - Enterprise login with "click here for video login" enabled - OOBE enrollment - do we care about this?
cernekee@chromium.org changed reviewers: + achuith@chromium.org
https://codereview.chromium.org/1983433002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:33: /** @const */ var MAX_VIDEO_LOGIN_TIME_SEC = 60; Just make this in millisec: var VIDEO_LOGIN_TIMEOUT = 60 * 1000;
https://codereview.chromium.org/1983433002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:33: /** @const */ var MAX_VIDEO_LOGIN_TIME_SEC = 60; On 2016/05/17 21:49:32, achuithb wrote: > Just make this in millisec: > var VIDEO_LOGIN_TIMEOUT = 60 * 1000; Done.
lgtm
https://codereview.chromium.org/1983433002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:650: if (this.gaiaAuthHost_.videoEnabled && this.videoTimer_ === undefined) { If the user signs in, ie, cancel is not called, where does the videoTimer_ get reset?
https://codereview.chromium.org/1983433002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:650: if (this.gaiaAuthHost_.videoEnabled && this.videoTimer_ === undefined) { On 2016/05/17 22:14:09, achuithb wrote: > If the user signs in, ie, cancel is not called, where does the videoTimer_ get > reset? Added cancel to onAuthCompleted_()
https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:874: if (this.videoTimer_ !== undefined) { Maybe add a method like clearVideoTimer()?
lgtm https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:33: /** @const */ var VIDEO_LOGIN_TIMEOUT = 60 * 1000; According to http://crbug.com/606979#c8, the timeout should be 180 seconds. https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:653: } I guess that, as an effect of the Authenticator.resetStates_ method, the videoEnabled flag may change back from true to false here. The cancel method below may cover all such possible cases, but it's quite difficult to track down all possible execution flows. So I would suggest, being on the safe side, to add here a branch with clearing the timeout when the video is not enabled (and +1 to separating the timer stopping operation into another method).
https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:975: if (this.videoTimer_ !== undefined) { Shouldn't this be moved to the top of the function? Because currently it may happen that the video timer fires, but its id stored in the videoTimer_ member is not cleared here because of the above code; this would result in ignoring all new attempts to set up a video timer.
https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:33: /** @const */ var VIDEO_LOGIN_TIMEOUT = 60 * 1000; On 2016/05/18 01:14:53, emaxx wrote: > According to http://crbug.com/606979#c8, the timeout should be 180 seconds. Done. https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:653: } On 2016/05/18 01:14:53, emaxx wrote: > I guess that, as an effect of the Authenticator.resetStates_ method, the > videoEnabled flag may change back from true to false here. The cancel method > below may cover all such possible cases, but it's quite difficult to track down > all possible execution flows. > So I would suggest, being on the safe side, to add here a branch with clearing > the timeout when the video is not enabled (and +1 to separating the timer > stopping operation into another method). Done. https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:874: if (this.videoTimer_ !== undefined) { On 2016/05/17 23:38:38, achuithb wrote: > Maybe add a method like clearVideoTimer()? Done. https://codereview.chromium.org/1983433002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:975: if (this.videoTimer_ !== undefined) { On 2016/05/18 01:30:17, emaxx wrote: > Shouldn't this be moved to the top of the function? > Because currently it may happen that the video timer fires, but its id stored in > the videoTimer_ member is not cleared here because of the above code; this would > result in ignoring all new attempts to set up a video timer. Done.
https://codereview.chromium.org/1983433002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:639: * Invoked when the authDomain property is changed on the GAIA host. Fix comment?
https://codereview.chromium.org/1983433002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right): https://codereview.chromium.org/1983433002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/screen_gaia_signin.js:639: * Invoked when the authDomain property is changed on the GAIA host. On 2016/05/18 21:08:25, achuithb wrote: > Fix comment? Done.
lgtm
The CQ bit was checked by cernekee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/1983433002/#ps100001 (title: "fix comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983433002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983433002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add timeout for SAML webcam logins In order to avoid leaving the camera on for extended periods of time, ensure that SAML webcam logins time out after one minute. This has the same effect as clicking the "X" button. BUG=606979 TEST=manual CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add timeout for SAML webcam logins In order to avoid leaving the camera on for extended periods of time, ensure that SAML webcam logins time out after one minute. This has the same effect as clicking the "X" button. BUG=606979 TEST=manual CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/75619fb6b1c62cfce2ac70a0dab6c0e5ddbd1049 Cr-Commit-Position: refs/heads/master@{#394571} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/75619fb6b1c62cfce2ac70a0dab6c0e5ddbd1049 Cr-Commit-Position: refs/heads/master@{#394571} |
