Force enterprise enrollment flow upon detection of inconsistent state.

Force enterprise enrollment flow upon detection of inconsistent state.

At the time LoginDisplayHostImpl decides whether enrollment flow is to be
started, policy hasn't been read yet, so LoginDisplayHostImpl is not in a
position to decide whether recovery is required.  To work around this, upon
policy load on machines requiring recovery, a flag is stored in prefs which
is accessed by LoginDisplayHostImpl early during (next) boot.

Enrollment recovery depends on the availability of the machine id and is
silently skipped when the latter is missing.  There'll be a separate CL to
have session manager provide the machine id on devices that require recovery.

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

[Thiemo Nagel, 2014-07-07 18:27:52]

Hi Julian and Pavel,

could you please be so kind to take a look at this CL which is intended to
re-enroll devices that have "spontaneously unenrolled"?

Thank you and best regards,
Thiemo

[pastarmovj, 2014-07-08 12:36:31]

Some initial comments mostly nits though.

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1197: (
first_screen_name.empty() && oobe_complete &&
nit: no spaces in the braces.

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1199:
!connector->IsEnterpriseManaged() );
I think you should still respect the first_Screen_name if set and respect it.

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:134: &&
chromeos::StartupUtils::IsEnrollmentRecoveryRequired()) {
nit: Move the && to the line above.

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:139:
->GetMachineStatistic(chromeos::system::kHardwareClassKey, &machine_id);
nit this line should be further 4 spaces in. I think if you add a const pointer
to the StatisticsProvider instance before you use it, it will look cleaner.

https://codereview.chromium.org/374573006/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/374573006/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1876: const char kEnrollmentRecoveryRequired[] =
"EnrollmentRecoveryRequired";
Document this one.

[dzhioev (left Google), 2014-07-08 13:03:16]

On 2014/07/08 12:36:31, pastarmovj wrote:
> Some initial comments mostly nits though.
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1197: (
> first_screen_name.empty() && oobe_complete &&
> nit: no spaces in the braces.
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1199:
> !connector->IsEnterpriseManaged() );
> I think you should still respect the first_Screen_name if set and respect it.
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc
(right):
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:134: &&
> chromeos::StartupUtils::IsEnrollmentRecoveryRequired()) {
> nit: Move the && to the line above.
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:139:
> ->GetMachineStatistic(chromeos::system::kHardwareClassKey, &machine_id);
> nit this line should be further 4 spaces in. I think if you add a const
pointer
> to the StatisticsProvider instance before you use it, it will look cleaner.
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/common/pref_names.cc
> File chrome/common/pref_names.cc (right):
> 
>
https://codereview.chromium.org/374573006/diff/40001/chrome/common/pref_names...
> chrome/common/pref_names.cc:1876: const char kEnrollmentRecoveryRequired[] =
> "EnrollmentRecoveryRequired";
> Document this one.

All formatting issues can be fixed with a single run of "git cl format".

[dzhioev (left Google), 2014-07-08 13:05:44]

Hi, Thiemo. Can you give me more context about what "spontaneously unenrolled"
means? I thought if device was enrolled once it can't be unenrolled anymore.

[Thiemo Nagel, 2014-07-08 13:43:50]

On 2014/07/08 13:05:44, dzhioev wrote:
> Hi, Thiemo. Can you give me more context about what "spontaneously unenrolled"
> means? I thought if device was enrolled once it can't be unenrolled anymore.

Hi Pavel,

there's some context in https://crbug.com/389481 but the main discussion is at
https://crbug.com/305457 .  In a nutshell, sometimes devices "lose" their
enrollment which is very problematic for a variety of reasons.  This CL recovers
enrollment on devices where local state has been clobbered but
install_attributes.pb is still ok.

Does that answer your question?  I'd be happy to explain the details over chat
or VC.

Cheers,
Thiemo

[Thiemo Nagel, 2014-07-08 14:02:05]

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1197: (
first_screen_name.empty() && oobe_complete &&
On 2014/07/08 12:36:31, pastarmovj wrote:
> nit: no spaces in the braces.

Done.

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1199:
!connector->IsEnterpriseManaged() );
On 2014/07/08 12:36:31, pastarmovj wrote:
> I think you should still respect the first_Screen_name if set and respect it.

Ok, thanks.

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:134: &&
chromeos::StartupUtils::IsEnrollmentRecoveryRequired()) {
On 2014/07/08 12:36:31, pastarmovj wrote:
> nit: Move the && to the line above.

Done.

https://codereview.chromium.org/374573006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:139:
->GetMachineStatistic(chromeos::system::kHardwareClassKey, &machine_id);
On 2014/07/08 12:36:31, pastarmovj wrote:
> nit this line should be further 4 spaces in. I think if you add a const
pointer
> to the StatisticsProvider instance before you use it, it will look cleaner.

I've tried it, to my eyes that looked worse.  But I've fixed the indent.

https://codereview.chromium.org/374573006/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/374573006/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1876: const char kEnrollmentRecoveryRequired[] =
"EnrollmentRecoveryRequired";
On 2014/07/08 12:36:31, pastarmovj wrote:
> Document this one.

Done.  I had totally forgotten that.

[dzhioev (left Google), 2014-07-08 16:27:38]

LGTM

[pastarmovj, 2014-07-09 08:48:58]

LGTM but please test that enrollment into kiosk and retail modes work as
expected before you land.

[Thiemo Nagel, 2014-07-09 11:36:58]

On 2014/07/09 08:48:58, pastarmovj wrote:
> LGTM but please test that enrollment into kiosk and retail modes work as
> expected before you land.

For the record:
* I'm ignoring linux_chromeos_asan because that bot fails in very similar ways
on trunk.
* I've tested the old retail mode and it is not affected, i.e. neither is it
broken by the change nor is enrollment recovery triggered for it.  (There is a
problem with the "Done" button after enrollment to retail mode, but this is
present on trunk, too.)
* I've tested kiosk mode and it is not affected, either.

[Thiemo Nagel, 2014-07-09 11:37:04]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-07-09 11:37:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/374573006/100001

[commit-bot: I haz the power, 2014-07-09 13:34:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-07-09 14:04:28]

Change committed as 282028