Make sure the device recovers from policy loss in the consumer case.

Make sure the device recovers from policy loss in the consumer case.

Lock down login to the owner only and restore policy on successful owner login.

The machine is forced to restart if the owner didn't login because we have
no better way to clear the key store loaded for another user.

BUG=chromium-os:26793
TEST=TBA. Manually: Delete the policy blob and verify that policy is recovered correctly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128877

[pastarmovj, 2012-02-24 16:39:56]

Hey Chris,
This is the first draft of my proposed solution to the policy file recovery
issue. Do you mind having a look at it. I still have to polish some rough edges
and add some tests to that but I'd like to know if you agree on the general
idea.
Thanks,
Julian

[Chris Masone, 2012-02-24 18:49:45]

This approach certainly looks fine to me.

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/device_...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/device_...
chrome/browser/chromeos/device_settings_provider.cc:541: // we repopulate the
whitelist with the owner and any other possible every
Any other possible what?

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/e...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/existing_user_controller.cc:468:
base::Bind(&PowerManagerClient::RequestRestart,
Is this going to reboot the machine?  That's probably overkill, but it works. 
Meh.

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/p...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/p...
chrome/browser/chromeos/login/parallel_authenticator.cc:469:
current_state_->ResetCryptohomeStatus();
Hm.  This doesn't actually unmount the user's cryptohome, does it?  It'd be cool
if we could do that as well, just to be sure that we really can't proceed with a
login.

[pastarmovj, 2012-03-13 15:21:55]

Hey Chris,
Please review this CL now with proper testing included. I have one more test
that I'll write in a separate CL that will do an end-to-end test with pyauto but
until then the unit-test coverage should be sufficiently good.

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/device_...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/device_...
chrome/browser/chromeos/device_settings_provider.cc:541: // we repopulate the
whitelist with the owner and any other possible every
On 2012/02/24 18:49:45, Chris Masone wrote:
> Any other possible what?

Sorry it seems I have drifted away while writing this comment. Fixed it now.

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/e...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/existing_user_controller.cc:468:
base::Bind(&PowerManagerClient::RequestRestart,
On 2012/02/24 18:49:45, Chris Masone wrote:
> Is this going to reboot the machine?  That's probably overkill, but it works. 
> Meh.

True. I changed it to session restart instead which is faster.

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/p...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/9466005/diff/1/chrome/browser/chromeos/login/p...
chrome/browser/chromeos/login/parallel_authenticator.cc:469:
current_state_->ResetCryptohomeStatus();
On 2012/02/24 18:49:45, Chris Masone wrote:
> Hm.  This doesn't actually unmount the user's cryptohome, does it?  It'd be
cool
> if we could do that as well, just to be sure that we really can't proceed with
a
> login.

Done.

[Chris Masone, 2012-03-13 16:45:52]

It seems like you broke the OnLoginSuccess unit test, per your trybot runs

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
File chrome/browser/chromeos/device_settings_provider_unittest.cc (right):

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/device_settings_provider_unittest.cc:211: // No calls to
the SignedSettingsHelper shoud occur in this case!
This comment copy-pasted?

Something more like "Clearing and setting new expectations for this test." would
make sense.

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/device_settings_provider_unittest.cc:231: // No calls to
the SignedSettingsHelper shoud occur in this case!
Same comment as above

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator.cc:211: LOG(ERROR) << "@@@
Resetting for " << username;
why LOG(ERROR)?

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator.cc:247: LOG(ERROR) << "@@@
Resetting for " << username;
Same question as above

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator.cc:413: // Now we can
continue reading the private key.
but you never read the private key here...

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator.cc:431: // Now we can
continue reading the private key.
you read it here

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator.cc:438: LOG(ERROR) << "@@@
Check finished: " << user_can_login_;
ERROR?

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator.cc:747: LOG(ERROR) << "@@@
cryptohome state " << owner_is_verified_;
ERROR?

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator.h:247: // A lock for
|owner_is_verified_| and |current_user_is_owner_|.
I don't see current_user_is_owner_

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
File chrome/browser/chromeos/login/user_manager.h (right):

http://chromiumcodereview.appspot.com/9466005/diff/12001/chrome/browser/chrom...
chrome/browser/chromeos/login/user_manager.h:149: 
extra newline

[pastarmovj, 2012-03-22 11:47:59]

Fixed the issues and split this CL which was starting to drag too much stuff in.
Now I have the changes needed in the UserManager (except for the direct ones
related to this change) committed and the device settings unit tests and changes
also deserved their own CL. 
I hope this makes reviewing this one a much more enjoyable experience now. :)

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider_unittest.cc (right):

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider_unittest.cc:211: // No calls to
the SignedSettingsHelper shoud occur in this case!
On 2012/03/13 16:45:53, Chris Masone wrote:
> This comment copy-pasted?
> 
> Something more like "Clearing and setting new expectations for this test."
would
> make sense.

This file is not part of this CL anymore. It was sort of test creep anyhow. I am
going to commit those tests in a separate CL.

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:211: LOG(ERROR) << "@@@
Resetting for " << username;
On 2012/03/13 16:45:53, Chris Masone wrote:
> why LOG(ERROR)?

Debug output and it is gone now :)

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:247: LOG(ERROR) << "@@@
Resetting for " << username;
On 2012/03/13 16:45:53, Chris Masone wrote:
> Same question as above

Debug output and it is gone now :)

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:413: // Now we can
continue reading the private key.
On 2012/03/13 16:45:53, Chris Masone wrote:
> but you never read the private key here...

Comment was wrong. Fixed.

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:431: // Now we can
continue reading the private key.
On 2012/03/13 16:45:53, Chris Masone wrote:
> you read it here

Done.

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:438: LOG(ERROR) << "@@@
Check finished: " << user_can_login_;
On 2012/03/13 16:45:53, Chris Masone wrote:
> ERROR?

Debug output. Gone.

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:747: LOG(ERROR) << "@@@
cryptohome state " << owner_is_verified_;
On 2012/03/13 16:45:53, Chris Masone wrote:
> ERROR?

Ditto.

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.h:247: // A lock for
|owner_is_verified_| and |current_user_is_owner_|.
On 2012/03/13 16:45:53, Chris Masone wrote:
> I don't see current_user_is_owner_

Done.

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/9466005/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager.h:149: 
On 2012/03/13 16:45:53, Chris Masone wrote:
> extra newline 

Done.

[Chris Masone, 2012-03-22 16:18:25]

Mostly good!  Just one question in a unittest file

http://chromiumcodereview.appspot.com/9466005/diff/28001/chrome/browser/chrom...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://chromiumcodereview.appspot.com/9466005/diff/28001/chrome/browser/chrom...
chrome/browser/chromeos/login/existing_user_controller.cc:95: // Delay for
rebooting the machine if safe-mode login has failed.
s/reboot/logout/

Or something that's not 'reboot', since we're not rebooting anymore :-)  In the
kOnstant name too.

http://chromiumcodereview.appspot.com/9466005/diff/28001/chrome/browser/chrom...
File chrome/browser/chromeos/login/parallel_authenticator_unittest.cc (right):

http://chromiumcodereview.appspot.com/9466005/diff/28001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:101:
old_user_manager_ = UserManager::Set(mock_user_manager_.get());
This confuses me...don't you have two pointers to the same thing, now?  And when
mock_user_manager_ goes out of scope, it'll destroy the thing old_user_manager_
is pointing to...

[pastarmovj, 2012-03-22 16:25:46]

Only responded to the question for mocking UserManager.

http://chromiumcodereview.appspot.com/9466005/diff/28001/chrome/browser/chrom...
File chrome/browser/chromeos/login/parallel_authenticator_unittest.cc (right):

http://chromiumcodereview.appspot.com/9466005/diff/28001/chrome/browser/chrom...
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:101:
old_user_manager_ = UserManager::Set(mock_user_manager_.get());
On 2012/03/22 16:18:25, Chris Masone wrote:
> This confuses me...don't you have two pointers to the same thing, now?  And
when
> mock_user_manager_ goes out of scope, it'll destroy the thing
old_user_manager_
> is pointing to...

Actually UserManager::Set *replaces* the current object in the singleton. What
it returns is the previous object so that we can restore it in the end of the
test. It is especially important for unit tests where the singleton instance is
preserved across all tests.

Btw I have a CL in review which automates this work. You can see it here :
http://codereview.chromium.org/9826010/

[Chris Masone, 2012-03-22 16:31:44]

Ah.  LGTM, once trybots pass, then.  ANd with that comment nit :-)

[pastarmovj, 2012-03-23 13:00:25]

Adding satorux for owner approval for chrome/browser/chromeos/dbus/.

http://codereview.chromium.org/9466005/diff/28001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/9466005/diff/28001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:95: // Delay for
rebooting the machine if safe-mode login has failed.
On 2012/03/22 16:18:25, Chris Masone wrote:
> s/reboot/logout/
> 
> Or something that's not 'reboot', since we're not rebooting anymore :-)  In
the
> kOnstant name too.

Done.

[satorux1, 2012-03-23 16:05:24]

chromeos/dbus lgtm