Fix Chrome OS enrollment with SAML accounts

Fix Chrome OS enrollment with SAML accounts

CL 677703002 caused a regression in the Chrome OS enrollment flow: If
the user authenticates via SAML and the credentials passing API is not
used, the enrollment flow will get stuck. This happens because the
GAIA auth extension wants to proceed with scraped password
verification but enrollment does not need the password and does not
implement the verification step.

This CL fixes enrollment by skipping password verification for
enrollment. The CL also adds a regression test - the first UI-driven
end-to-end enrollment test AFAICT.

BUG=438471
TEST=New browser test

Committed: https://crrev.com/9ebcd79ec2de9af9b2855bf3a901d138f3af2267
Cr-Commit-Position: refs/heads/master@{#308374}

[bartfab (slow), 2014-12-03 23:42:50]

bartfab@chromium.org changed reviewers:
+ antrim@chromium.org, mnissler@chromium.org, xiyuan@chromium.org

[bartfab (slow), 2014-12-03 23:42:52]

Hi Xiyuan,

Could you please review:
chrome/browser/resources/gaia_auth/*
chrome/browser/resources/gaia_auth_host/*

Hi Denis,

Could you please review:
chrome/browser/chromeos/login/saml/*
chrome/browser/resources/chromeos/login/*

Hi Mattias,

Could you please review:
chrome/browser/policy/test/local_policy_test_server.cc


I was a bit upset that we lacked browser test coverage for enrollment, causing
this P1 regression to hit. 10 hours later, I understood why we had no test
coverage... the finished test is straightforward enough but getting there took a
lot of effort. I think that we should expand the test to cover e.g. non-SAML
enrollment but I want to keep the CL as small as possible for now since it will
need merging.

[xiyuan, 2014-12-04 00:15:46]

LGTM!

Thank you for adding test coverage for that.

[Mattias Nissler (ping if slow), 2014-12-04 19:46:02]

Nice, thanks for writing that test.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:699:
ASSERT_EQ(static_cast<int>(strlen(kPolicy)),
If I'm not mistaken, you can just use sizeof instead of strlen.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:712:
command_line->AppendSwitch(switches::kEnterpriseEnrollmentSkipRobotAuth);
Any good reason for this?

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:776: void
SAMLEnrollmentTest::WaitForEnrollmentSuccess() {
Instead of this, why not just observe DeviceCloudPolicyStoreChromeOS? That'd
give you a simple C++ observer way of learning about successful enrollment.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/policy/test/l...
File chrome/browser/policy/test/local_policy_test_server.cc (right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/policy/test/l...
chrome/browser/policy/test/local_policy_test_server.cc:208:
.AppendASCII("proto"));
Ha, I'm surprised this only gets noticed now...

[bartfab (slow), 2014-12-05 12:34:36]

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:699:
ASSERT_EQ(static_cast<int>(strlen(kPolicy)),
On 2014/12/04 19:46:02, Mattias Nissler wrote:
> If I'm not mistaken, you can just use sizeof instead of strlen.

I can use |sizeof(kPolicy) - 1| but that feels more clunky and brittle as the |-
1| could easily get lost in some refactor.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:712:
command_line->AppendSwitch(switches::kEnterpriseEnrollmentSkipRobotAuth);
On 2014/12/04 19:46:02, Mattias Nissler wrote:
> Any good reason for this?

The robot account generation code causes an OAuth token fetch with a |code|
parameter that FakeGaia does not expect. I have two options: Set up FakeGaia to
expect this |code| or skip the robot account setup. I chose the latter because
it is easier and simpler. If you prefer, I am happy to set up FakeGaia to handle
the robot account instead.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:776: void
SAMLEnrollmentTest::WaitForEnrollmentSuccess() {
On 2014/12/04 19:46:02, Mattias Nissler wrote:
> Instead of this, why not just observe DeviceCloudPolicyStoreChromeOS? That'd
> give you a simple C++ observer way of learning about successful enrollment.

This verifies that the enrollment success reaches the UI. I am adding this test
to protect against a regression that manifested itself in the UI getting stuck.
I think it is nice to verify that after enrollment, the UI reaches a valid
state.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/policy/test/l...
File chrome/browser/policy/test/local_policy_test_server.cc (right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/policy/test/l...
chrome/browser/policy/test/local_policy_test_server.cc:208:
.AppendASCII("proto"));
On 2014/12/04 19:46:02, Mattias Nissler wrote:
> Ha, I'm surprised this only gets noticed now...

I guess this is a tribute to YAPS' popularity.

[Mattias Nissler (ping if slow), 2014-12-05 13:07:09]

LGTM

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:699:
ASSERT_EQ(static_cast<int>(strlen(kPolicy)),
On 2014/12/05 12:34:36, bartfab wrote:
> On 2014/12/04 19:46:02, Mattias Nissler wrote:
> > If I'm not mistaken, you can just use sizeof instead of strlen.
> 
> I can use |sizeof(kPolicy) - 1| but that feels more clunky and brittle as the
|-
> 1| could easily get lost in some refactor.

Well, then the test would fail. Anyhow, fair enough.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:712:
command_line->AppendSwitch(switches::kEnterpriseEnrollmentSkipRobotAuth);
On 2014/12/05 12:34:36, bartfab wrote:
> On 2014/12/04 19:46:02, Mattias Nissler wrote:
> > Any good reason for this?
> 
> The robot account generation code causes an OAuth token fetch with a |code|
> parameter that FakeGaia does not expect. I have two options: Set up FakeGaia
to
> expect this |code| or skip the robot account setup. I chose the latter because
> it is easier and simpler. If you prefer, I am happy to set up FakeGaia to
handle
> the robot account instead.

If I ever get to write an end-to-end FRE/enrollment browser test, robot auth
token fetching should be tested as well. I'm fine with leaving it out for now
though. Can you add a comment and/or TODO saying that this is only a shortcut
that can (and perhaps should be?) removed once we have the code infrastructure
to deal with the robot auth requests?

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:776: void
SAMLEnrollmentTest::WaitForEnrollmentSuccess() {
On 2014/12/05 12:34:36, bartfab wrote:
> On 2014/12/04 19:46:02, Mattias Nissler wrote:
> > Instead of this, why not just observe DeviceCloudPolicyStoreChromeOS? That'd
> > give you a simple C++ observer way of learning about successful enrollment.
> 
> This verifies that the enrollment success reaches the UI. I am adding this
test
> to protect against a regression that manifested itself in the UI getting
stuck.
> I think it is nice to verify that after enrollment, the UI reaches a valid
> state.

OK, fair enough. Might additionally want to click the OK button then and check
for the enrollment success exit code to WizardController? That's perhaps out of
scope of this test though.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/policy/test/l...
File chrome/browser/policy/test/local_policy_test_server.cc (right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/policy/test/l...
chrome/browser/policy/test/local_policy_test_server.cc:208:
.AppendASCII("proto"));
On 2014/12/05 12:34:36, bartfab wrote:
> On 2014/12/04 19:46:02, Mattias Nissler wrote:
> > Ha, I'm surprised this only gets noticed now...
> 
> I guess this is a tribute to YAPS' popularity.

Not really, I'm surprised that no tests failed because of missing protobuf code.
Anyhow, that's tangential.

[Denis Kuznetsov (DE-MUC), 2014-12-05 14:33:19]

lgtm on my side.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:154: url
+= '&needPassword=0';
I know it is all our playground, but do we happen to have some url-building
library (instead of simple concatenation?

[bartfab (slow), 2014-12-15 17:10:21]

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:712:
command_line->AppendSwitch(switches::kEnterpriseEnrollmentSkipRobotAuth);
On 2014/12/05 13:07:09, Mattias Nissler wrote:
> On 2014/12/05 12:34:36, bartfab wrote:
> > On 2014/12/04 19:46:02, Mattias Nissler wrote:
> > > Any good reason for this?
> > 
> > The robot account generation code causes an OAuth token fetch with a |code|
> > parameter that FakeGaia does not expect. I have two options: Set up FakeGaia
> to
> > expect this |code| or skip the robot account setup. I chose the latter
because
> > it is easier and simpler. If you prefer, I am happy to set up FakeGaia to
> handle
> > the robot account instead.
> 
> If I ever get to write an end-to-end FRE/enrollment browser test, robot auth
> token fetching should be tested as well. I'm fine with leaving it out for now
> though. Can you add a comment and/or TODO saying that this is only a shortcut
> that can (and perhaps should be?) removed once we have the code infrastructure
> to deal with the robot auth requests?

Done.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/saml/saml_browsertest.cc:776: void
SAMLEnrollmentTest::WaitForEnrollmentSuccess() {
On 2014/12/05 13:07:09, Mattias Nissler wrote:
> On 2014/12/05 12:34:36, bartfab wrote:
> > On 2014/12/04 19:46:02, Mattias Nissler wrote:
> > > Instead of this, why not just observe DeviceCloudPolicyStoreChromeOS?
That'd
> > > give you a simple C++ observer way of learning about successful
enrollment.
> > 
> > This verifies that the enrollment success reaches the UI. I am adding this
> test
> > to protect against a regression that manifested itself in the UI getting
> stuck.
> > I think it is nice to verify that after enrollment, the UI reaches a valid
> > state.
> 
> OK, fair enough. Might additionally want to click the OK button then and check
> for the enrollment success exit code to WizardController? That's perhaps out
of
> scope of this test though.

I had a look. We cannot get at the exit code because this test uses the real
WizardController that has no hooks for screen exit codes. We would need to
inject some mock/fake that extracts the code.

https://codereview.chromium.org/781623003/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/781623003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:154: url
+= '&needPassword=0';
On 2014/12/05 14:33:19, Denis Kuznetsov(sick to 13ish) wrote:
> I know it is all our playground, but do we happen to have some url-building
> library (instead of simple concatenation?

I am not aware of any. We have GURL in C++ but nothing I know of in JS.

[bartfab (slow), 2014-12-15 17:10:47]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-12-15 17:11:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/781623003/20001

[commit-bot: I haz the power, 2014-12-15 18:21:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-15 18:22:28]

Patchset 2 (id:??) landed as
https://crrev.com/9ebcd79ec2de9af9b2855bf3a901d138f3af2267
Cr-Commit-Position: refs/heads/master@{#308374}