Connect enrollment screen to cloud policy subsystem.

Connect enrollment screen to cloud policy subsystem.

Poke the policy subsystem after authenticating GAIA in order to finalize
enrollment.

BUG=chromium-os:13277
TEST=manual

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

[Mattias Nissler (ping if slow), 2011-04-11 16:32:11]

This CL finally makes the whole thing functional! :)

[Jakob Kummerow, 2011-04-11 20:49:01]

LGTM :-)

http://codereview.chromium.org/6821045/diff/1001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821045/diff/1001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:10: #include
"chrome/browser/browser_process.h"
nit: duplicate include

[Mattias Nissler (ping if slow), 2011-04-12 08:40:04]

Fixed nit. Nikita, any feedback?

http://codereview.chromium.org/6821045/diff/1001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821045/diff/1001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:10: #include
"chrome/browser/browser_process.h"
On 2011/04/11 20:49:01, jkummerow wrote:
> nit: duplicate include

Done.

[Nikita (slow), 2011-04-12 12:46:22]

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:98:
connector->SetCredentials(user_, auth_token, GetMachineId());
Is this process somehow shown to user, like spinner or so?
Also is it possible for the user to close screen somehow while enrollment is in
process. What happens then?

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:191: if
(!sys_lib->GetMachineStatistic(kMachineInfoSystemHwqual, &system_hwqual) ||
Just FYI, this information is only accessible during OOBE.
So after first user signs in, it will not be accessible.
Is this fine with enrollment flow?

[Nikita (slow), 2011-04-12 12:49:30]

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:54:
observer->OnExit(ScreenObserver::ENTERPRISE_ENROLLMENT_CANCELLED);
We don't need to call
g_browser_process->browser_policy_connector()->StopAutoRetry();
here because it will continue retrying in the background?

More general question is what happens if enrollment is unsuccessful/cancelled
during OOBE. Machine would have to go through OOBE again to retry it, right?

[Mattias Nissler (ping if slow), 2011-04-12 14:07:15]

New version with proper cancellation is up. PTAL. Thanks.

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:54:
observer->OnExit(ScreenObserver::ENTERPRISE_ENROLLMENT_CANCELLED);
On 2011/04/12 12:49:30, Nikita Kostylev wrote:
> We don't need to call
> g_browser_process->browser_policy_connector()->StopAutoRetry();
> here because it will continue retrying in the background?
> 
> More general question is what happens if enrollment is unsuccessful/cancelled
> during OOBE. Machine would have to go through OOBE again to retry it, right?

You have a point. We should definitely make the StopAutoRetry() call here, since
we don't want it to auto-retry in the background. Fixed.

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:98:
connector->SetCredentials(user_, auth_token, GetMachineId());
On 2011/04/12 12:46:22, Nikita Kostylev wrote:
> Is this process somehow shown to user, like spinner or so?
> Also is it possible for the user to close screen somehow while enrollment is
in
> process. What happens then?

It's just the login spinner on the GAIA login box, so there is some indication
of "something is going on". I know this is not perfect, but I think it's
sufficient for now. Note that the whole process completes in less then a second
in my tests, so we should be good.

Concerning the cancel button, see my comment above.

http://codereview.chromium.org/6821045/diff/13/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:191: if
(!sys_lib->GetMachineStatistic(kMachineInfoSystemHwqual, &system_hwqual) ||
On 2011/04/12 12:46:22, Nikita Kostylev wrote:
> Just FYI, this information is only accessible during OOBE.
> So after first user signs in, it will not be accessible.
> Is this fine with enrollment flow?

That's fine, since we can only enroll if the device hasn't been claimed (which
happens once a user logs in).

[Nikita (slow), 2011-04-12 14:22:58]

LGTM

Having GAIA spinner is fine. My point was that we need to make sure that UI is
locked during enrollment and there's some indication that it is not stuck.

[Mattias Nissler (ping if slow), 2011-04-12 16:29:45]

Sorry guys, I've realized that there actually is a field for the machine model
in the proto which we should fill in properly. While doing that, I've also moved
the whole machine id and model checking into DevicePolicyIdentityStrategy
(that's where it belongs IMHO). So could you please take another look (in
particular Jakob concerning the policy stuff)? Thanks.

[Jakob Kummerow, 2011-04-12 16:42:59]

LGTM with a minor comment for your consideration.

http://codereview.chromium.org/6821045/diff/20/chrome/browser/policy/device_p...
File chrome/browser/policy/device_policy_identity_strategy.cc (right):

http://codereview.chromium.org/6821045/diff/20/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_identity_strategy.cc:33:
!sys_lib->GetMachineStatistic(kMachineInfoSerialNumber,
Due to lazy evaluation of conditions, if fetching the model failed, then
fetching the serial number will not even be attempted. We could put in more
effort here, but I'm not sure if it really makes a difference.

[Jakob Kummerow, 2011-04-12 16:43:49]

Oh, and making it compile would of course be nice ;-)

[Mattias Nissler (ping if slow), 2011-04-12 16:49:46]

http://codereview.chromium.org/6821045/diff/20/chrome/browser/policy/device_p...
File chrome/browser/policy/device_policy_identity_strategy.cc (right):

http://codereview.chromium.org/6821045/diff/20/chrome/browser/policy/device_p...
chrome/browser/policy/device_policy_identity_strategy.cc:33:
!sys_lib->GetMachineStatistic(kMachineInfoSerialNumber,
On 2011/04/12 16:42:59, jkummerow wrote:
> Due to lazy evaluation of conditions, if fetching the model failed, then
> fetching the serial number will not even be attempted. We could put in more
> effort here, but I'm not sure if it really makes a difference.

Good point, no reason to not be more careful.
Done.

[xiyuan, 2011-04-12 17:26:22]

FYI, the change description has the wrong issue number. I think you meant
chromium-os:13277 rather than 13777.

[Mattias Nissler (ping if slow), 2011-04-12 17:27:41]

On 2011/04/12 17:26:22, xiyuan wrote:
> FYI, the change description has the wrong issue number. I think you meant
> chromium-os:13277 rather than 13777.

That's correct. Fixed.

[Jakob Kummerow, 2011-04-13 07:42:33]

Fixed unit tests LGTM.