Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3583013:
[cros] Add blocking UI on offline: OK, online auth: fail case. (Closed)
|
Created:
10 years, 2 months ago by Nikita (slow) Modified:
9 years, 7 months ago CC:
chromium-reviews, nkostylev+cc_chromium.org, davemoore+watch_chromium.org, ben+cc_chromium.org Base URL:
http://src.chromium.org/git/chromium.git Visibility:
Public. |
Description[cros] Add blocking UI on offline: OK, online auth: fail case.
Using ParallelAuthenticator to test
http://codereview.chromium.org/3442009
BUG=chromium-os:6638
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68013
Patch Set 1 #Patch Set 2 : iteration #1 #Patch Set 3 : proper description + cover edge cases #Patch Set 4 : release warning #
Total comments: 2
Patch Set 5 : merge, add/remove observer, proper check for screen lock #
Total comments: 22
Patch Set 6 : fix build, clear delegate #Patch Set 7 : fix build #
Total comments: 12
Patch Set 8 : adress avayvod comments #Patch Set 9 : merge #Patch Set 10 : fix crashes, use proper API #Patch Set 11 : check whether screen is already locked/unlocked #
Total comments: 4
Patch Set 12 : merge & revert ParallelAuthenticator fix #Patch Set 13 : remove debug logging #
Total comments: 15
Patch Set 14 : fix for review comments #
Total comments: 4
Patch Set 15 : add flag #Patch Set 16 : removed unused flag #
Total comments: 14
Patch Set 17 : address cmasone, oshima comments #Messages
Total messages: 25 (0 generated)
We'll be executing offline auth first and delete login windows, bring up UI once it's done. LoginPerformer instance is alive till online auth results are back. If it's OK - deleted. If not - screen is locked and ScreenLocker is used for password input, error messages display. This code has several corner cases, I've tried to cover them all except when I'm waiting for API update on Authenticator side and couple of TODOs. If Authenticator follows existing protocol (online, then offline, single callback to LoginStatusConsumer) then login process stays the same and new code is not executed.
On 2010/10/05 14:13:57, Nikita Kostylev wrote: > We'll be executing offline auth first and delete login windows, bring up UI once > it's done. > > LoginPerformer instance is alive till online auth results are back. If it's OK - > deleted. If not - screen is locked and ScreenLocker is used for password input, > error messages display. > > This code has several corner cases, I've tried to cover them all except when I'm > waiting for API update on Authenticator side and couple of TODOs. > > If Authenticator follows existing protocol (online, then offline, single > callback to LoginStatusConsumer) then login process stays the same and new code > is not executed. so far, this LGTM
I'll wait for Chris pending CLs and integrate with them.
What happens if A user closed a lid (or ctrl-alt-l) while it's performing remote auth? http://codereview.chromium.org/3583013/diff/8001/9002 File chrome/browser/chromeos/login/existing_user_controller.cc (right): http://codereview.chromium.org/3583013/diff/8001/9002#newcode467 chrome/browser/chromeos/login/existing_user_controller.cc:467: performer = NULL; Do we need to assign it to local variable, then NULLify?
- user locks the screen during remote auth In this case request for screen lock will be ignored and LoginPerformer won't receive LockScreen. I'll add a check whether ScreenLock is already active. That might eliminate blocking_ui_ member. - user closes the lid ScreenLock will be forced. So should be the same as with user force screen lock. I'll patch Chris CL with ParallelAuthenticator to check the behavior. http://codereview.chromium.org/3583013/diff/8001/9002 File chrome/browser/chromeos/login/existing_user_controller.cc (right): http://codereview.chromium.org/3583013/diff/8001/9002#newcode467 chrome/browser/chromeos/login/existing_user_controller.cc:467: performer = NULL; On 2010/10/05 18:18:26, oshima wrote: > Do we need to assign it to local variable, then NULLify? Yes, there's a compiler warning if return value is unused. Resetting it to NULL just to make sure it's not used any more.
On Tue, Oct 5, 2010 at 12:15 PM, <nkostylev@chromium.org> wrote: > - user locks the screen during remote auth > In this case request for screen lock will be ignored and LoginPerformer > won't > receive LockScreen. > I'll add a check whether ScreenLock is already active. That might eliminate > blocking_ui_ member. > - user closes the lid > ScreenLock will be forced. So should be the same as with user force screen > lock. > > There is no difference between closing lid and ctrl-alt-l from screen locker point of view, powermanager sends lock request to chrome. It looks to me that in both cases, we ignore signal from power manager and does not send signal back. How about changing ScreenLocker so that it forces remote auth when remote auth (for unlock) fails? It currently does local auth only but given what we're doing, it make sense to do the same in screen locker, and login process can use the same remote auth part of screen locker when remote auth failed. This way, we can handle locking and login process in the same way. Isn't this simpler? - oshima > I'll patch Chris CL with ParallelAuthenticator to check the behavior. > > > > http://codereview.chromium.org/3583013/diff/8001/9002 > File chrome/browser/chromeos/login/existing_user_controller.cc (right): > > http://codereview.chromium.org/3583013/diff/8001/9002#newcode467 > chrome/browser/chromeos/login/existing_user_controller.cc:467: performer > = NULL; > On 2010/10/05 18:18:26, oshima wrote: > >> Do we need to assign it to local variable, then NULLify? >> > > Yes, there's a compiler warning if return value is unused. > Resetting it to NULL just to make sure it's not used any more. > > > http://codereview.chromium.org/3583013/show >
On Tue, Oct 5, 2010 at 1:51 PM, oshima <oshima@chromium.org> wrote: > > > On Tue, Oct 5, 2010 at 12:15 PM, <nkostylev@chromium.org> wrote: > >> - user locks the screen during remote auth >> In this case request for screen lock will be ignored and LoginPerformer >> won't >> receive LockScreen. >> I'll add a check whether ScreenLock is already active. That might >> eliminate >> blocking_ui_ member. > > >> - user closes the lid >> ScreenLock will be forced. So should be the same as with user force screen >> lock. >> >> > There is no difference between closing lid and ctrl-alt-l from screen > locker point of view, > powermanager sends lock request to chrome. > It looks to me that in both cases, we ignore signal from power manager and > does not send > signal back. > > How about changing ScreenLocker so that it forces remote auth when remote > auth (for unlock) fails? > It currently does local auth only but given what we're doing, it make sense > to do the same in screen locker, > and login process can use the same remote auth part of screen locker when > remote auth failed. > This way, we can handle locking and login process in the same way. Isn't > this simpler? > We still wouldn't be able to handle locking and the login process in the same way, because login kicks off an attempt to mount the user's cryptohome. Screen lock just checks the key. So, we'd still need to distinguish between unclock and login. Furthermore, I think we chose not to have lock rely on online login because it'd take too long. > > - oshima > > > >> I'll patch Chris CL with ParallelAuthenticator to check the behavior. >> >> >> >> http://codereview.chromium.org/3583013/diff/8001/9002 >> File chrome/browser/chromeos/login/existing_user_controller.cc (right): >> >> http://codereview.chromium.org/3583013/diff/8001/9002#newcode467 >> chrome/browser/chromeos/login/existing_user_controller.cc:467: performer >> = NULL; >> On 2010/10/05 18:18:26, oshima wrote: >> >>> Do we need to assign it to local variable, then NULLify? >>> >> >> Yes, there's a compiler warning if return value is unused. >> Resetting it to NULL just to make sure it's not used any more. >> >> >> http://codereview.chromium.org/3583013/show >> > >
On Tue, Oct 5, 2010 at 9:54 PM, Chris Masone <cmasone@chromium.org> wrote: > > > On Tue, Oct 5, 2010 at 1:51 PM, oshima <oshima@chromium.org> wrote: > >> >> >> On Tue, Oct 5, 2010 at 12:15 PM, <nkostylev@chromium.org> wrote: >> >>> - user locks the screen during remote auth >>> In this case request for screen lock will be ignored and LoginPerformer >>> won't >>> receive LockScreen. >>> I'll add a check whether ScreenLock is already active. That might >>> eliminate >>> blocking_ui_ member. >> >> >>> - user closes the lid >>> ScreenLock will be forced. So should be the same as with user force >>> screen lock. >>> >>> >> There is no difference between closing lid and ctrl-alt-l from screen >> locker point of view, >> powermanager sends lock request to chrome. >> It looks to me that in both cases, we ignore signal from power manager and >> does not send >> signal back. >> >> How about changing ScreenLocker so that it forces remote auth when remote >> auth (for unlock) fails? >> It currently does local auth only but given what we're doing, it make >> sense to do the same in screen locker, >> and login process can use the same remote auth part of screen locker when >> remote auth failed. >> This way, we can handle locking and login process in the same way. Isn't >> this simpler? >> > > We still wouldn't be able to handle locking and the login process in the > same way, because login kicks off an attempt to mount the user's cryptohome. > Screen lock just checks the key. So, we'd still need to distinguish between > unclock and login. > That's fine because mounting happens before screen locker kicks in, and that happens at first login using local auth right? > Furthermore, I think we chose not to have lock rely on online login because > it'd take too long. > Yes, but we can do the same background online login and lock again if it fails. I wasn't involved discussion and I may be missing some important points, so sorry if that's the case. - oshima > >> >> - oshima >> >> >> >>> I'll patch Chris CL with ParallelAuthenticator to check the behavior. >>> >>> >>> >>> http://codereview.chromium.org/3583013/diff/8001/9002 >>> File chrome/browser/chromeos/login/existing_user_controller.cc (right): >>> >>> http://codereview.chromium.org/3583013/diff/8001/9002#newcode467 >>> chrome/browser/chromeos/login/existing_user_controller.cc:467: performer >>> = NULL; >>> On 2010/10/05 18:18:26, oshima wrote: >>> >>>> Do we need to assign it to local variable, then NULLify? >>>> >>> >>> Yes, there's a compiler warning if return value is unused. >>> Resetting it to NULL just to make sure it's not used any more. >>> >>> >>> http://codereview.chromium.org/3583013/show >>> >> >> >
I propose continue using AuthenticateToUnlock in ScreenLocker unless last online login attempt failed (i.e. LoginPerformer exists). This way unlock would be still quick to respond. Otherwise we have to go back to all those edge cases when online login fails with timeout. Note that if LoginPerformer gets one of the timeout errors it continues because local auth was OK.
On Wed, Oct 6, 2010 at 5:22 AM, <nkostylev@chromium.org> wrote: > I propose continue using AuthenticateToUnlock in ScreenLocker unless last > online > login attempt failed (i.e. LoginPerformer exists). > This way unlock would be still quick to respond. Otherwise we have to go > back to > all those edge cases when online login fails with timeout. > > Note that if LoginPerformer gets one of the timeout errors it continues > because > local auth was OK. > > Locker still use AuthenticatToUnlock to lock first, but will kicks off remote auth like login and the process will be the same as login. Lock is still quick as it unlocks using local auth first. In anycase, this is merely my proposal so you can ignore it, but you still need to address the out of sync issue. One potential issue is that it unlocks when it shouldn't. That is, 1 login performer shows lock screen. 2 user typed in correct password. 3 PM tries to lock because lid is closing. 4 lock screen simply sends signal back to PM 5* remote auth succeeds, login performer unlocks screen 6 ChromeOS goes to sleep. 7 Anyone who opens the lid next has access to the screen. * this process may happen after lid is opened. We keep the lock state in PM to avoid these edge cases. - oshima > > http://codereview.chromium.org/3583013/show >
http://codereview.chromium.org/3583013/diff/16001/17001 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/3583013/diff/16001/17001#newcode9865 chrome/app/generated_resources.grd:9865: <message name="IDS_LOGIN_ERROR_PASSWORD_CHANGED" desc="Couldn't log in because new password is invalid (when password change has been detected)"> It's "old" password that's invalid since there's a new one, right? http://codereview.chromium.org/3583013/diff/16001/17001#newcode9866 chrome/app/generated_resources.grd:9866: You password has changed. Sorry, your new password could not be verified. Please try again. You -> Your. Maybe: "Your password has changed. Try again with your new password."? In your message new should mean old actually, as I read it. http://codereview.chromium.org/3583013/diff/16001/17001#newcode9869 chrome/app/generated_resources.grd:9869: Couldn't validate your account. It has been deleted/disabled or not signed up. Please sign out. What's "Your account is signed up"? http://codereview.chromium.org/3583013/diff/16001/17003 File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/16001/17003#newcode49 chrome/browser/chromeos/login/login_performer.cc:49: void LoginPerformer::OnLoginFailure(const LoginFailure& failure) { This method is too long. Could you break it up in some small chunks with reasonable names? http://codereview.chromium.org/3583013/diff/16001/17003#newcode204 chrome/browser/chromeos/login/login_performer.cc:204: //blocking_ui_ = true; Why these commented assignments? http://codereview.chromium.org/3583013/diff/16001/17007 File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/3583013/diff/16001/17007#newcode59 chrome/browser/chromeos/login/screen_locker.cc:59: public Notificationkr { Some bad typo? http://codereview.chromium.org/3583013/diff/16001/17007#newcode826 chrome/browser/chromeos/login/screen_locker.cc:826: std::wstring(), // TODO: add help link TODO(nkostylev)? http://codereview.chromium.org/3583013/diff/16001/17007#newcode828 chrome/browser/chromeos/login/screen_locker.cc:828: if (mouse_event_relay_.get()) { Using {} for one line if's is not consistent even within this method. http://codereview.chromium.org/3583013/diff/16001/17008 File chrome/browser/chromeos/login/screen_locker.h (right): http://codereview.chromium.org/3583013/diff/16001/17008#newcode51 chrome/browser/chromeos/login/screen_locker.h:51: return screen_locker_; Can't we somehow avoid using these global variables? http://codereview.chromium.org/3583013/diff/16001/17008#newcode83 chrome/browser/chromeos/login/screen_locker.h:83: // If |sign_out_only| is true than all other input Unfinished comment? http://codereview.chromium.org/3583013/diff/16001/17009 File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/3583013/diff/16001/17009#newcode236 chrome/browser/chromeos/login/wizard_controller.cc:236: is_official_build_(true), Debug change? http://codereview.chromium.org/3583013/diff/21001/22002 File chrome/browser/chromeos/login/existing_user_controller.cc (right): http://codereview.chromium.org/3583013/diff/21001/22002#newcode459 chrome/browser/chromeos/login/existing_user_controller.cc:459: void ExistingUserController::OnLoginSuccess(const std::string& username, It was better with this argument on the next line, imho. http://codereview.chromium.org/3583013/diff/21001/22002#newcode468 chrome/browser/chromeos/login/existing_user_controller.cc:468: LoginPerformer* performer = login_performer_.release(); Can't we let login performer delete itself always and just use common pointer and set it to NULL here? http://codereview.chromium.org/3583013/diff/21001/22003 File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/21001/22003#newcode40 chrome/browser/chromeos/login/login_performer.cc:40: LOG(INFO) << "Deleting LoginPerformer"; Could be DLOG http://codereview.chromium.org/3583013/diff/21001/22003#newcode139 chrome/browser/chromeos/login/login_performer.cc:139: LOG(INFO) << "LoginSuccess, pending_requests " << pending_requests; Debug logging? http://codereview.chromium.org/3583013/diff/21001/22003#newcode217 chrome/browser/chromeos/login/login_performer.cc:217: << "power manager decided no to unlock the screen."; no -> not http://codereview.chromium.org/3583013/diff/21001/22007 File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/3583013/diff/21001/22007#newcode615 chrome/browser/chromeos/login/screen_locker.cc:615: LOG(INFO) << "Delegating authentication to LoginPerformer."; DLOG?
http://codereview.chromium.org/3583013/diff/16001/17001 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/3583013/diff/16001/17001#newcode9865 chrome/app/generated_resources.grd:9865: <message name="IDS_LOGIN_ERROR_PASSWORD_CHANGED" desc="Couldn't log in because new password is invalid (when password change has been detected)"> On 2010/10/06 16:22:34, whywhat wrote: > It's "old" password that's invalid since there's a new one, right? No, it's the case when - user has changed password - user enters old password - offline auth - OK, UI process starts - online login fails - user is asked for a correct new password - after it's entered and OK, homedir is migrated. http://codereview.chromium.org/3583013/diff/16001/17001#newcode9866 chrome/app/generated_resources.grd:9866: You password has changed. Sorry, your new password could not be verified. Please try again. On 2010/10/06 16:22:34, whywhat wrote: > You -> Your. > Maybe: "Your password has changed. Try again with your new password."? In your > message new should mean old actually, as I read it. Done. http://codereview.chromium.org/3583013/diff/16001/17001#newcode9869 chrome/app/generated_resources.grd:9869: Couldn't validate your account. It has been deleted/disabled or not signed up. Please sign out. On 2010/10/06 16:22:34, whywhat wrote: > What's "Your account is signed up"? It's GoogleServiceAuthError::USER_NOT_SIGNED_UP It's like never existed. http://codereview.chromium.org/3583013/diff/16001/17003 File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/16001/17003#newcode49 chrome/browser/chromeos/login/login_performer.cc:49: void LoginPerformer::OnLoginFailure(const LoginFailure& failure) { On 2010/10/06 16:22:34, whywhat wrote: > This method is too long. Could you break it up in some small chunks with > reasonable names? Done. http://codereview.chromium.org/3583013/diff/16001/17003#newcode204 chrome/browser/chromeos/login/login_performer.cc:204: //blocking_ui_ = true; On 2010/10/06 16:22:34, whywhat wrote: > Why these commented assignments? Done. http://codereview.chromium.org/3583013/diff/16001/17007 File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/3583013/diff/16001/17007#newcode59 chrome/browser/chromeos/login/screen_locker.cc:59: public Notificationkr { On 2010/10/06 16:22:34, whywhat wrote: > Some bad typo? Done. http://codereview.chromium.org/3583013/diff/16001/17007#newcode826 chrome/browser/chromeos/login/screen_locker.cc:826: std::wstring(), // TODO: add help link On 2010/10/06 16:22:34, whywhat wrote: > TODO(nkostylev)? Done. http://codereview.chromium.org/3583013/diff/16001/17007#newcode828 chrome/browser/chromeos/login/screen_locker.cc:828: if (mouse_event_relay_.get()) { On 2010/10/06 16:22:34, whywhat wrote: > Using {} for one line if's is not consistent even within this method. Done. http://codereview.chromium.org/3583013/diff/16001/17008 File chrome/browser/chromeos/login/screen_locker.h (right): http://codereview.chromium.org/3583013/diff/16001/17008#newcode51 chrome/browser/chromeos/login/screen_locker.h:51: return screen_locker_; On 2010/10/06 16:22:34, whywhat wrote: > Can't we somehow avoid using these global variables? Once screen is locked ScreenLocker is guaranteed to exist. Using this variable is direct way to use it. I'll think about alternatives. http://codereview.chromium.org/3583013/diff/16001/17008#newcode83 chrome/browser/chromeos/login/screen_locker.h:83: // If |sign_out_only| is true than all other input On 2010/10/06 16:22:34, whywhat wrote: > Unfinished comment? Done. http://codereview.chromium.org/3583013/diff/16001/17009 File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/3583013/diff/16001/17009#newcode236 chrome/browser/chromeos/login/wizard_controller.cc:236: is_official_build_(true), On 2010/10/06 16:22:34, whywhat wrote: > Debug change? Done. http://codereview.chromium.org/3583013/diff/21001/22002 File chrome/browser/chromeos/login/existing_user_controller.cc (right): http://codereview.chromium.org/3583013/diff/21001/22002#newcode459 chrome/browser/chromeos/login/existing_user_controller.cc:459: void ExistingUserController::OnLoginSuccess(const std::string& username, On 2010/10/06 16:22:34, whywhat wrote: > It was better with this argument on the next line, imho. Done. http://codereview.chromium.org/3583013/diff/21001/22002#newcode468 chrome/browser/chromeos/login/existing_user_controller.cc:468: LoginPerformer* performer = login_performer_.release(); On 2010/10/06 16:22:34, whywhat wrote: > Can't we let login performer delete itself always and just use common pointer > and set it to NULL here? As discussed: LoginPerformer lifetime in general is managed by ExistingUserController instance. EUC creates new LP instance on each different user login attempt. For several login attempts for a single user one LP instance is used. EUC releases LP ownership only in one case - OnLoginSuccess. LC deletes itself if there're no more pending request or waits for pending request result. http://codereview.chromium.org/3583013/diff/21001/22003 File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/21001/22003#newcode40 chrome/browser/chromeos/login/login_performer.cc:40: LOG(INFO) << "Deleting LoginPerformer"; On 2010/10/06 16:22:34, whywhat wrote: > Could be DLOG Done. http://codereview.chromium.org/3583013/diff/21001/22003#newcode139 chrome/browser/chromeos/login/login_performer.cc:139: LOG(INFO) << "LoginSuccess, pending_requests " << pending_requests; On 2010/10/06 16:22:34, whywhat wrote: > Debug logging? Done. http://codereview.chromium.org/3583013/diff/21001/22003#newcode217 chrome/browser/chromeos/login/login_performer.cc:217: << "power manager decided no to unlock the screen."; On 2010/10/06 16:22:34, whywhat wrote: > no -> not Done. http://codereview.chromium.org/3583013/diff/21001/22007 File chrome/browser/chromeos/login/screen_locker.cc (right): http://codereview.chromium.org/3583013/diff/21001/22007#newcode615 chrome/browser/chromeos/login/screen_locker.cc:615: LOG(INFO) << "Delegating authentication to LoginPerformer."; On 2010/10/06 16:22:34, whywhat wrote: > DLOG? Done.
Please take a look. I'm intending to land this change on TOT only. It's been a while but I've resurrected this CL and fixed almost all issues with it: - Changed the way how screen lock is requested/observed - Added check for cases when screen lock is already locked/unlocked - Using existing authenticator instance for RetryAuth() subsequent call - Use current_attempt_ for MigrateAttempt when we have correct new password. TODOs/left - When user enters correct new password, cryptohome is migrated but ParrallelAuthenticator/CryptohomeLibrary go into infinite loop. Investigating separately with Chris. - After screen is locked error bubble is shown. After recent days changes in shows in the incorrect place (investigating) - Surface CAPTCHA dialog on ScreenLock (P2, will fix in subsequent CL)
As for infinite loop in ParallelAuthenticator - I'll land my change w/o fix in parallel_authenticator.cc. In that case migration won't happen but that's tracked in a separate issue.
Issue in ParallelAuthenticator is now fixed at http://codereview.chromium.org/5108001/ Issue with mispositioned error bubble on ScreenLocker doesn't repro any more.
Let's do this next week. We're pretty busy now and should be working on R9.x issues.
http://codereview.chromium.org/3583013/diff/41001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/41001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:37: DCHECK(default_performer_ == NULL); Add a message to DCHECK? http://codereview.chromium.org/3583013/diff/41001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:42: DVLOG(1) << "Deleting LoginPerformer"; DCHECK for default_performer_ not NULL? http://codereview.chromium.org/3583013/diff/50001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/3583013/diff/50001/chrome/app/generated_resour... chrome/app/generated_resources.grd:10916: Couldn't validate your account. It has been deleted/disabled. Please sign out. "/" -> " or " http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:103: MessageLoop::current()->DeleteSoon(FROM_HERE, this); I thought delegate_ owns login performer if it exists. http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:105: //DCHECK(!pending_requests); Remove or uncomment? http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:112: return; Shouldn't LP delete itself here? http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:180: // Whitelist is performed during offline login only. Whitelist -> Whitelist check http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/password_changed_view.h (right): http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/password_changed_view.h:67: return false; Maybe just remove override? http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screen_locker.h (right): http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.h:85: // If |sign_out_only| is true than all other input except "Sign Out" than -> then
http://codereview.chromium.org/3583013/diff/41001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/41001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:37: DCHECK(default_performer_ == NULL); On 2010/11/25 15:40:37, whywhat wrote: > Add a message to DCHECK? Done. Plus added message for bunch of other DCHECKs. http://codereview.chromium.org/3583013/diff/41001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:42: DVLOG(1) << "Deleting LoginPerformer"; On 2010/11/25 15:40:37, whywhat wrote: > DCHECK for default_performer_ not NULL? Done. http://codereview.chromium.org/3583013/diff/50001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/3583013/diff/50001/chrome/app/generated_resour... chrome/app/generated_resources.grd:10916: Couldn't validate your account. It has been deleted/disabled. Please sign out. On 2010/11/25 15:40:37, whywhat wrote: > "/" -> " or " Done. http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:103: MessageLoop::current()->DeleteSoon(FROM_HERE, this); On 2010/11/25 15:40:37, whywhat wrote: > I thought delegate_ owns login performer if it exists. No, when delegate_->OnLoginSuccess is executed. Delegate releases LoginPerformer ownership as the first step. Added comment. http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:105: //DCHECK(!pending_requests); On 2010/11/25 15:40:37, whywhat wrote: > Remove or uncomment? Done. http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:112: return; On 2010/11/25 15:40:37, whywhat wrote: > Shouldn't LP delete itself here? Not yet, added comment. Checked other DeleteSoon calls. http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:180: // Whitelist is performed during offline login only. On 2010/11/25 15:40:37, whywhat wrote: > Whitelist -> Whitelist check Done. http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/password_changed_view.h (right): http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/password_changed_view.h:67: return false; On 2010/11/25 15:40:37, whywhat wrote: > Maybe just remove override? It's a virtual method, needs to be declared. http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/screen_locker.h (right): http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/screen_locker.h:85: // If |sign_out_only| is true than all other input except "Sign Out" On 2010/11/25 15:40:37, whywhat wrote: > than -> then Done.
LGTM http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/password_changed_view.h (right): http://codereview.chromium.org/3583013/diff/50001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/password_changed_view.h:67: return false; On 2010/11/29 11:20:25, Nikita Kostylev wrote: > On 2010/11/25 15:40:37, whywhat wrote: > > Maybe just remove override? > > It's a virtual method, needs to be declared. Ah, sorry, I missed the fact that it's a pure virtual method indeed.
Thanks, looks better now. Just one edge case we need to deal with. http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:181: ResolveScreenLocked(); screen can be locked even if auth succeeds if a user closed the lid after typing in correct password. Can ResolveScreenLocked handle this case correctly? Best way is to add reason for lock, but it requires changes to PM, so you may just want to keep the reason somewhere in the LP for now. http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.h (right): http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.h:64: ~LoginPerformer(); virtual (just a style nit)
Mitsuru, Chris, Please take a look. http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:181: ResolveScreenLocked(); On 2010/11/29 20:10:10, oshima wrote: > screen can be locked even if auth succeeds if a user closed the lid after typing > in correct password. Can ResolveScreenLocked handle this case correctly? Best > way is to add reason for lock, but it requires changes to PM, so you may just > want to keep the reason somewhere in the LP for now. Done. I've added additional flag. It might happen in case: - Correct password entered - RequestScreenUnlock - screen lock requested externally (closed lid) - screen lock doesn't change (generates another (SCREEN_LOCK_STATE_CHANGED, true) ?) - No need to call ResolveScreenLocked(). When screen will be finally unlocked, leave ResolveScreenUnlocked() as is. It'll remove LoginPerformer instance. http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.h (right): http://codereview.chromium.org/3583013/diff/61001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.h:64: ~LoginPerformer(); On 2010/11/29 20:10:10, oshima wrote: > virtual (just a style nit) Done.
http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:117: // TODO(nkostylev): Execute CookieFetcher->AttemptFetch() here once http://crosbug.com/9814 http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:362: // TODO(nkostylev): Instruct ScreenLocker to show CAPTCHA input. http://crosbug.com/9812
Unless the whitelist check part is actually wrong (and not just a comment that confused me), the parallel authenticator and login performer parts of this LGTM, with a few comment/logging nits. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:82: LOG(INFO) << "Online login timed out. " VLOG(1)? http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:198: // Whitelist check is performed during offline login only. During offline login only? It shouldn't happen during screen unlock, that's true, but it should always happen when someone's initially logging in to the device...is that what you mean? http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:305: LOG(INFO) << "Online login timed out - unlocking screen. " Maybe LOG(WARNING)? http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/parallel_authenticator.cc (right): http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/parallel_authenticator.cc:277: VLOG(1) << "Resolving state: " << state; nit: I might say "Resolved state to: " or something. You already figured out what state we're in, now we're just deciding what to do based on that state.
LGTM on locker bits. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:117: // TODO(nkostylev): Execute CookieFetcher->AttemptFetch() here once On 2010/12/01 15:02:52, Nikita Kostylev wrote: > http://crosbug.com/9814 Add the above link in the comment. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:362: // TODO(nkostylev): Instruct ScreenLocker to show CAPTCHA input. On 2010/12/01 15:02:52, Nikita Kostylev wrote: > http://crosbug.com/9812 include bug in the comment.
http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:82: LOG(INFO) << "Online login timed out. " On 2010/12/01 18:12:21, Chris Masone wrote: > VLOG(1)? Done. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:117: // TODO(nkostylev): Execute CookieFetcher->AttemptFetch() here once On 2010/12/01 23:46:53, oshima wrote: > On 2010/12/01 15:02:52, Nikita Kostylev wrote: > > http://crosbug.com/9814 > > Add the above link in the comment. Done. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:198: // Whitelist check is performed during offline login only. On 2010/12/01 18:12:21, Chris Masone wrote: > During offline login only? It shouldn't happen during screen unlock, that's > true, but it should always happen when someone's initially logging in to the > device...is that what you mean? Yes, comment was wrong, fixed. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:305: LOG(INFO) << "Online login timed out - unlocking screen. " On 2010/12/01 18:12:21, Chris Masone wrote: > Maybe LOG(WARNING)? Done. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:362: // TODO(nkostylev): Instruct ScreenLocker to show CAPTCHA input. On 2010/12/01 23:46:53, oshima wrote: > On 2010/12/01 15:02:52, Nikita Kostylev wrote: > > http://crosbug.com/9812 > > include bug in the comment. Done. http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/parallel_authenticator.cc (right): http://codereview.chromium.org/3583013/diff/74001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/parallel_authenticator.cc:277: VLOG(1) << "Resolving state: " << state; On 2010/12/01 18:12:21, Chris Masone wrote: > nit: I might say "Resolved state to: " or something. You already figured out > what state we're in, now we're just deciding what to do based on that state. Done. |
