Allow re-enrollment to the same domain in case of policy data loss.

Allow re-enrollment to the same domain in case of policy data loss.

As a side effect allow to reenroll with any other account from the same
domain as the initial enrollment user.

BUG=chromium-os:26157
TEST=As described in the bug + unit_tests:enterprise.

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

[pastarmovj, 2012-06-01 15:53:56]

Hi Mattias,
Can you please review the first CL in the series that will make re-enrollment in
case of emergency possible.

This one makes re-enrolling with any account from the same domain possible but
doesn't force it. Forcing will be implemented only after we have proper UI to
describe the need for that.

This CL will only work combined with a change to chromeos-init to enforce
creating the machine-info file not only when the owner.key is missing but also
when the policy file is missing.

[Mattias Nissler (ping if slow), 2012-06-04 10:17:52]

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

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:461: } else {
shouldn't this be else if (status == OwnershipService::OWNERSHIP_TAKEN)?

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:465:
CrosSettings::Get()->PrepareTrustedValues(base::Bind(
nit: can you break before base::Bind?

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (left):

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:178:
chromeos::Authenticator::Canonicalize(policy_data.username())) {
Remove the authenticator header as well. Also, don't you break
upper-case/lower-case domain names by removing this?

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache_unittest.cc:210: // Set new policy
information. This should fail due to invalid user.
comment is outdated.

[pastarmovj, 2012-06-08 09:54:32]

Please reexamine this CL.

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

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:461: } else {
On 2012/06/04 10:17:52, Mattias Nissler wrote:
> shouldn't this be else if (status == OwnershipService::OWNERSHIP_TAKEN)?

OwnershipServoce::GetStatusAsync is guaranteed to return with a valid status
which is either OWNERSHIP_NONE or ..._TAKEN so the else here is justified.

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:465:
CrosSettings::Get()->PrepareTrustedValues(base::Bind(
On 2012/06/04 10:17:52, Mattias Nissler wrote:
> nit: can you break before base::Bind?

Done. However the huge size of the function name eats another line by itself. :(

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (left):

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:178:
chromeos::Authenticator::Canonicalize(policy_data.username())) {
On 2012/06/04 10:17:52, Mattias Nissler wrote:
> Remove the authenticator header as well. Also, don't you break
> upper-case/lower-case domain names by removing this?

Done.

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache_unittest.cc:210: // Set new policy
information. This should fail due to invalid user.
On 2012/06/04 10:17:52, Mattias Nissler wrote:
> comment is outdated.

Done.

[pastarmovj, 2012-06-08 11:42:47]

@Joao: FYI

[Mattias Nissler (ping if slow), 2012-06-08 12:06:11]

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

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:461: } else {
On 2012/06/08 09:54:32, pastarmovj wrote:
> On 2012/06/04 10:17:52, Mattias Nissler wrote:
> > shouldn't this be else if (status == OwnershipService::OWNERSHIP_TAKEN)?
> 
> OwnershipServoce::GetStatusAsync is guaranteed to return with a valid status
> which is either OWNERSHIP_NONE or ..._TAKEN so the else here is justified.

I'm aware of that. To somebody new to this, it may be less obvious, which is why
I ask you to spell out your intention clearly.

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc
(right):

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc:241:
actor_->ShowFatalEnrollmentError();
This condition needs a proper error message.

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:194: // registration domain in
install attributes, so lower-case the data here.
Please rather introduce a helper function for this in Authenticator. I don't
think it's a good idea to sprinkle knowledge of how to pre-process usernames and
domains over the code base, as changes would have to be applied everywhere.

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.h:63: static std::string
ExtractDomainName(const std::string& email);
This should go into Authenticator, which already has the canonicalization helper
for the username already.

[pastarmovj, 2012-06-08 12:56:39]

@Mattias: PTAL
@Nikita: Please review as owner of chrome/browser/ui/webui/chromeos/login.

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

http://codereview.chromium.org/10443125/diff/3002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:461: } else {
On 2012/06/08 12:06:11, Mattias Nissler wrote:
> On 2012/06/08 09:54:32, pastarmovj wrote:
> > On 2012/06/04 10:17:52, Mattias Nissler wrote:
> > > shouldn't this be else if (status == OwnershipService::OWNERSHIP_TAKEN)?
> > 
> > OwnershipServoce::GetStatusAsync is guaranteed to return with a valid status
> > which is either OWNERSHIP_NONE or ..._TAKEN so the else here is justified.
> 
> I'm aware of that. To somebody new to this, it may be less obvious, which is
why
> I ask you to spell out your intention clearly.

Done.

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc
(right):

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc:241:
actor_->ShowFatalEnrollmentError();
On 2012/06/08 12:06:11, Mattias Nissler wrote:
> This condition needs a proper error message.

Done.

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:194: // registration domain in
install attributes, so lower-case the data here.
On 2012/06/08 12:06:11, Mattias Nissler wrote:
> Please rather introduce a helper function for this in Authenticator. I don't
> think it's a good idea to sprinkle knowledge of how to pre-process usernames
and
> domains over the code base, as changes would have to be applied everywhere.

Done.

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/10443125/diff/11001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.h:63: static std::string
ExtractDomainName(const std::string& email);
On 2012/06/08 12:06:11, Mattias Nissler wrote:
> This should go into Authenticator, which already has the canonicalization
helper
> for the username already.

Done.

[Mattias Nissler (ping if slow), 2012-06-08 13:13:44]

http://codereview.chromium.org/10443125/diff/16002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10443125/diff/16002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15967: +      <message
name="IDS_ENTERPRISE_WRONG_USER_ERROR" desc="Error message to show when the user
used to re-enroll the device does not belong to the domain that the device was
originally enrolled into.">
Name is not really descriptive. Maybe ENROLLMENT_DOMAIN_MISMATCH?

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen_actor.h
(right):

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen_actor.h:76:
virtual void ShowUserEnrollmentError() = 0;
We are getting to the point where we should probably switch to a single function
and pass the error message along.

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/existing_user_controller.cc:479: // re-enroll if
the policy information has been screwed.
s/has been screwed/is invalid/, we really don't know what happened :)

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:172:
StringToLowerASCII(install_attributes_->GetDomain()));
It would be nice if we could call the canonicalization code in Authenticator
here as well, since you're still assuming you know what Authenticator does here.

[pastarmovj, 2012-06-08 15:07:12]

Addressed your comments.

http://codereview.chromium.org/10443125/diff/16002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10443125/diff/16002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15967: +      <message
name="IDS_ENTERPRISE_WRONG_USER_ERROR" desc="Error message to show when the user
used to re-enroll the device does not belong to the domain that the device was
originally enrolled into.">
On 2012/06/08 13:13:44, Mattias Nissler wrote:
> Name is not really descriptive. Maybe ENROLLMENT_DOMAIN_MISMATCH?

Done.

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen_actor.h
(right):

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen_actor.h:76:
virtual void ShowUserEnrollmentError() = 0;
On 2012/06/08 13:13:44, Mattias Nissler wrote:
> We are getting to the point where we should probably switch to a single
function
> and pass the error message along.

Agreed but let me take this is AI for another CL this is growing too distracted
already.

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/existing_user_controller.cc:479: // re-enroll if
the policy information has been screwed.
On 2012/06/08 13:13:44, Mattias Nissler wrote:
> s/has been screwed/is invalid/, we really don't know what happened :)

Done.

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/16002/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:172:
StringToLowerASCII(install_attributes_->GetDomain()));
On 2012/06/08 13:13:44, Mattias Nissler wrote:
> It would be nice if we could call the canonicalization code in Authenticator
> here as well, since you're still assuming you know what Authenticator does
here.

Done.

[Mattias Nissler (ping if slow), 2012-06-08 15:24:36]

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:173:
install_attributes_->GetRegistrationUser()));
I though GetRegistrationUser() is meant to go away eventually?

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:171: registration_user_ =
enterprise_user;
Canonicalize here as well? In fact, how about canonicalizing everything here, so
the caller doesn't have to worry?

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:184:
chromeos::Authenticator::ExtractDomainName(registration_user_);
This is now inconsistent. If we read from the attribute, we'll get a
non-canonicalized version of the domain, but if it's coming from the user name,
then we do canonicalize.

[Nikita (slow), 2012-06-14 12:42:55]

chromeos/login lgtm

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:248:
void EnterpriseOAuthEnrollmentScreenHandler::ShowUserEnrollmentError() {
nit: rename to ShowDomainMismatchError()

[pastarmovj, 2012-06-18 14:57:27]

Addressed your comments PTAL.

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:173:
install_attributes_->GetRegistrationUser()));
On 2012/06/08 15:24:36, Mattias Nissler wrote:
> I though GetRegistrationUser() is meant to go away eventually?

Done.

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:171: registration_user_ =
enterprise_user;
On 2012/06/08 15:24:36, Mattias Nissler wrote:
> Canonicalize here as well? In fact, how about canonicalizing everything here,
so
> the caller doesn't have to worry?

Done.

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:184:
chromeos::Authenticator::ExtractDomainName(registration_user_);
On 2012/06/08 15:24:36, Mattias Nissler wrote:
> This is now inconsistent. If we read from the attribute, we'll get a
> non-canonicalized version of the domain, but if it's coming from the user
name,
> then we do canonicalize.

Done.

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

http://codereview.chromium.org/10443125/diff/14003/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:248:
void EnterpriseOAuthEnrollmentScreenHandler::ShowUserEnrollmentError() {
On 2012/06/14 12:42:55, Nikita Kostylev wrote:
> nit: rename to ShowDomainMismatchError()

Done.

[Mattias Nissler (ping if slow), 2012-06-18 16:45:17]

Getting close, I'd like to see the fixed test before LGTM

http://codereview.chromium.org/10443125/diff/25001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:16060: +        This user account does not
belong to the domain that the device is enrolled into.  If you want to enroll to
a different domain you need to go through device recovery first.
nit: s/enrolled into/enrolled to/

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/authenticator.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/authenticator.cc:45: // update this function in
sync with |Canonicalize| if this ever changes.
I think we only use bars for variable names, not for functions.

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:17: #include "base/string_util.h"
needed?

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache_unittest.cc:217:
base::FundamentalValue expected(120);
This check is weird. You don't propagate the new policy into the cache (but it
will eventually) and still test for the old value here.

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:182: if
(!cryptohome_->InstallAttributesGet(kAttrEnterpriseDomain,
nit: Invert the logic so this is easier to follow.

[pastarmovj, 2012-06-18 17:12:58]

Hope this it :)

http://codereview.chromium.org/10443125/diff/25001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:16060: +        This user account does not
belong to the domain that the device is enrolled into.  If you want to enroll to
a different domain you need to go through device recovery first.
On 2012/06/18 16:45:17, Mattias Nissler wrote:
> nit: s/enrolled into/enrolled to/

Done.

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/authenticator.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/authenticator.cc:45: // update this function in
sync with |Canonicalize| if this ever changes.
On 2012/06/18 16:45:17, Mattias Nissler wrote:
> I think we only use bars for variable names, not for functions.

I thought it is generic concept for making identifiers easier to spot in the
text but no problem i removed the bars.

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache.cc:17: #include "base/string_util.h"
On 2012/06/18 16:45:17, Mattias Nissler wrote:
> needed?

Nope a leftover.

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/devi...
chrome/browser/policy/device_policy_cache_unittest.cc:217:
base::FundamentalValue expected(120);
On 2012/06/18 16:45:17, Mattias Nissler wrote:
> This check is weird. You don't propagate the new policy into the cache (but it
> will eventually) and still test for the old value here.

Sorry this piece is a junk leftover when i copied stuff from the previous unit
test. I am only interested in the StartStorePolicyOp invocation. The previous
test already makes sure that whatever we store is coming out fine on the
retrieve end as well.

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/10443125/diff/25001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:182: if
(!cryptohome_->InstallAttributesGet(kAttrEnterpriseDomain,
On 2012/06/18 16:45:17, Mattias Nissler wrote:
> nit: Invert the logic so this is easier to follow.

Done.

[Mattias Nissler (ping if slow), 2012-06-19 09:15:09]

LGTM for realz now.

[commit-bot: I haz the power, 2012-06-19 09:30:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10443125/29002

[commit-bot: I haz the power, 2012-06-19 11:43:03]

Try job failure for 10443125-29002 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...