Fix ScreenLocker to not send notifications with empty passwords.

Fix ScreenLocker to not send notifications with empty passwords.

bartfab ran into this as a result of an unrelated patch he was testing. The logic in ScreenLocker should match that in LoginUtils, and not send this notification out if the password is blank.

BUG=chromium-os:31600
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141252

[Andrew T Wilson (Slow), 2012-06-07 00:48:35]

PTAL

[Nikita (slow), 2012-06-07 09:43:59]

http://codereview.chromium.org/10544043/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/10544043/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/screen_locker.cc:189: if (profile &&
!password.empty()) {
How could you end up in this state with empty password?
OnLoginSuccess() is called by authenticator when password is validated. Empty
password wouldn't qualify as valid, right?

http://codereview.chromium.org/10544043/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/screen_locker.cc:192: if (!password.empty()) {
Why do you need another check for empty password after one at line #189?

[Nikita (slow), 2012-06-07 09:46:36]

In case of LoginUtils there's a clear state when we end up in that state:

// We may not always have a passphrase (for example, on a restart after a
// browser crash). Only notify the sync service if we have a passphrase,
// so it can do any required re-encryption.

That shouldn't happen at screen unlock.

[Andrew T Wilson (Slow), 2012-06-07 17:22:42]

PTAL. I can add a DCHECK() here if you are convinced that we should never get an
empty passphrase.

https://chromiumcodereview.appspot.com/10544043/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://chromiumcodereview.appspot.com/10544043/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/login/screen_locker.cc:189: if (profile &&
!password.empty()) {
On 2012/06/07 09:43:59, Nikita Kostylev wrote:
> How could you end up in this state with empty password?
> OnLoginSuccess() is called by authenticator when password is validated. Empty
> password wouldn't qualify as valid, right?

Here is what Bartosz reported to me:
-=-=-=-
Bartosz: void ScreenLocker::OnLoginSuccess()
this constructs details with the password it was given as an argument
which comes from ParallelAuthenticator::OnLoginSuccess()
this one takes the password from current_state_->password
the current_state_ gets set up in ParallelAuthenticator::AuthenticateToUnlock()
and this is the interesting one
it does current_state_.reset(new AuthAttemptState(...))
the constructor it uses takes a user name and a password hash. so user name and
hash exist, the plain text password is blank
  
this is how (i think) we end up with a blank password
-=-=-=-

It's possible that this is only the case for automated tests? I'm not 100%
familiar with the fail case.

If you're quite certain that we should never get called with an empty password,
I can change this to a DCHECK() instead - let me know what you'd like me to do.

https://chromiumcodereview.appspot.com/10544043/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/login/screen_locker.cc:192: if (!password.empty()) {
On 2012/06/07 09:43:59, Nikita Kostylev wrote:
> Why do you need another check for empty password after one at line #189?
Bah, I moved the check up above but forgot to remove this one.

[Nikita (slow), 2012-06-08 17:25:46]

lgtm

Yes, I think this issue could only reproduce in automated tests.

[commit-bot: I haz the power, 2012-06-08 17:36:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/10544043/6002

[commit-bot: I haz the power, 2012-06-08 19:22:37]

Change committed as 141252