Don't register gmail users at the device management server

Don't register gmail users at the device management server

Make DeviceTokenFetcher wait until the name of the logged-in user is broadcasted, and only allow registering at DMServer if the user has a non-gmail address.

BUG=62477
TEST=DeviceTokenFetcherTest.FetchBetweenTokenNotifyAndLoginNotify,DeviceTokenFetcherTest.FetchWithNonDasherUsername

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

[gfeher, 2010-11-15 18:11:40]

Here is the long-awaited, beautiful hack!
Please review it.

[Mattias Nissler (ping if slow), 2010-11-16 09:54:31]

Looks pretty good, the only thing that bothers me is the injected_username hack.
Heck, it's a meta-hack even :)

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:35: int kNumNonDasherDomains = 2;
You can move this down and use arraysize(kNonDasherDomains).

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:103: if (injected_username.get()
== NULL) {
Is the inject_username quirk really necessary? I see that your test support code
just sends the notifications. Can't you just configure a UserManager or
TestingProfile for the tests?

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:113: // |kNonDasherDomains|.
I feel the reference to |kNonDasherDomains| here is not useful, since that's an
implementation detail.

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:154: scoped_ptr<std::string>
injected_username;
Why scoped_ptr-wrapped?

Also, it seems a bit odd to me to effectively have a separate implementation for
tests?

[gfeher, 2010-11-16 17:37:00]

I've removed the hack which has resulted in adding a TODO at another place of
the code. Please take another look.

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:35: int kNumNonDasherDomains = 2;
On 2010/11/16 09:54:31, Mattias Nissler wrote:
> You can move this down and use arraysize(kNonDasherDomains).

Done.

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:103: if (injected_username.get()
== NULL) {
On 2010/11/16 09:54:31, Mattias Nissler wrote:
> Is the inject_username quirk really necessary? I see that your test support
code
> just sends the notifications. Can't you just configure a UserManager or
> TestingProfile for the tests?

Done.

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:113: // |kNonDasherDomains|.
On 2010/11/16 09:54:31, Mattias Nissler wrote:
> I feel the reference to |kNonDasherDomains| here is not useful, since that's
an
> implementation detail.

Done.

http://codereview.chromium.org/4960003/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:154: scoped_ptr<std::string>
injected_username;
On 2010/11/16 09:54:31, Mattias Nissler wrote:
> Why scoped_ptr-wrapped?
To allow it to be NULL. Let's forget it. :)

> Also, it seems a bit odd to me to effectively have a separate implementation
for
> tests?

Done.

[gfeher, 2010-11-17 15:15:50]

I have removed support for sync SigninManager for now, and created
TestingDeviceTokenFetcher for tests.
Please take another look.

[gfeher, 2010-11-17 16:18:34]

Bots hopefully fixed.

[gfeher, 2010-11-22 16:05:51]

I've added back support for Sync.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:138: //
immediately provided and no refresh should be triggered.
There is a logic change here, because the TestingProfile and the TokenService
gets reused now.

[Mattias Nissler (ping if slow), 2010-11-22 20:36:08]

A first round of comments.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:64:
static_cast<TestingDeviceTokenFetcher*> (
No space before (

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:138: //
immediately provided and no refresh should be triggered.
On 2010/11/22 16:05:51, gfeher wrote:
> There is a logic change here, because the TestingProfile and the TokenService
> gets reused now.

Can we just reinitialize everything, i.e. the profile, the provider and the
fetcher at this point?

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:34: static const char
kPlaceholderDeviceID[] = "placeholder_device_id";
What's this?

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:36: int kNumNonDasherDomains = 2;
arraysize()?

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:62: token_service_ =
profile_->GetTokenService();
If you store the profile, you can always do this when you need it, so no need
for a token_service_ member anymore.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:71: #if defined(OS_CHROMEOS)
indentation

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:97: #if defined(OS_CHROMEOS)
indentation

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:215:
ASSERT_TRUE(fetcher_->IsTokenPending());
I think IsTokenPending() should return false at this point, otherwise we cannot
distinguish this case from an actual error.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:1336: this));
Note this part of the code needs rebasing, the policy provider is now
constructed in a different place.

http://codereview.chromium.org/4960003/diff/56002/chrome/test/testing_device_...
File chrome/test/testing_device_token_fetcher.h (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.h:31: virtual void SimulateLogin(const
std::string& username);
Why is this virtual?

[gfeher, 2010-11-23 13:47:50]

Sorry about the junk. I've addressed your issues.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:64:
static_cast<TestingDeviceTokenFetcher*> (
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> No space before (

Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider_unittest.cc:138: //
immediately provided and no refresh should be triggered.
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> On 2010/11/22 16:05:51, gfeher wrote:
> > There is a logic change here, because the TestingProfile and the
TokenService
> > gets reused now.
> 
> Can we just reinitialize everything, i.e. the profile, the provider and the
> fetcher at this point?

No changes made, as discussed.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:34: static const char
kPlaceholderDeviceID[] = "placeholder_device_id";
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> What's this?

Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:36: int kNumNonDasherDomains = 2;
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> arraysize()?

Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:62: token_service_ =
profile_->GetTokenService();
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> If you store the profile, you can always do this when you need it, so no need
> for a token_service_ member anymore.

Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:71: #if defined(OS_CHROMEOS)
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:97: #if defined(OS_CHROMEOS)
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:215:
ASSERT_TRUE(fetcher_->IsTokenPending());
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> I think IsTokenPending() should return false at this point, otherwise we
cannot
> distinguish this case from an actual error.

I've mixed up the names of these two tests. :/
Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:1336: this));
On 2010/11/22 20:36:08, Mattias Nissler wrote:
> Note this part of the code needs rebasing, the policy provider is now
> constructed in a different place.

Done.

http://codereview.chromium.org/4960003/diff/56002/chrome/test/testing_device_...
File chrome/test/testing_device_token_fetcher.h (right):

http://codereview.chromium.org/4960003/diff/56002/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.h:31: virtual void SimulateLogin(const
std::string& username);
Memories of Java...

[Mattias Nissler (ping if slow), 2010-11-23 16:00:43]

Looks pretty good to me, just a few nits to fix.

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
File chrome/test/testing_device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.cc:8: #include
"chrome/browser/policy/device_token_fetcher.h"
alphabetize.

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.cc:12: #if defined(OS_CHROMEOS)
Put a newline before in order to establish this as a new block.

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
File chrome/test/testing_device_token_fetcher.h (right):

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.h:29: const FilePath& token_path) :
Move the colon to the next line. Don't the arguments fit after the paren on line
26?

[danno, 2010-11-23 17:04:49]

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:32: const char*
kNonDasherDomains[] = {
See comment below about Dasher.

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:40: bool CanBeInDasherDomain(const
std::string& username) {
Is it possible to login with a user name that has trailing whitespace? Does this
still work?

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:220: if
(CanBeInDasherDomain(username)) {
Don't use Dasher, use something like "IsGoogleAccountsForYourDomainUser" or
similar.

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:228:
OnError(DeviceManagementBackend::kErrorServiceManagementNotSupported);
There is a new method on the delegate for this, OnNonManaged, you should send
that instead.

[gfeher, 2010-11-24 19:38:40]

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:32: const char*
kNonDasherDomains[] = {
On 2010/11/23 17:04:49, danno wrote:
> See comment below about Dasher.

Done.

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:40: bool CanBeInDasherDomain(const
std::string& username) {
I've made an experiment with ChromeOS. I could add trailing whitespaces and this
still worked. I guess whitespaces are trimmed at the GUI level. Or at least,
they should be.

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:220: if
(CanBeInDasherDomain(username)) {
On 2010/11/23 17:04:49, danno wrote:
> Don't use Dasher, use something like "IsGoogleAccountsForYourDomainUser" or
> similar.

How about simply saying "non-managed"?

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:228:
OnError(DeviceManagementBackend::kErrorServiceManagementNotSupported);
On 2010/11/23 17:04:49, danno wrote:
> There is a new method on the delegate for this, OnNonManaged, you should send
> that instead.

Done.

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
File chrome/test/testing_device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.cc:8: #include
"chrome/browser/policy/device_token_fetcher.h"
On 2010/11/23 16:00:44, Mattias Nissler wrote:
> alphabetize.

Done.

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.cc:12: #if defined(OS_CHROMEOS)
On 2010/11/23 16:00:44, Mattias Nissler wrote:
> Put a newline before in order to establish this as a new block.

Done.

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
File chrome/test/testing_device_token_fetcher.h (right):

http://codereview.chromium.org/4960003/diff/78001/chrome/test/testing_device_...
chrome/test/testing_device_token_fetcher.h:29: const FilePath& token_path) :
On 2010/11/23 16:00:44, Mattias Nissler wrote:
> Move the colon to the next line. Don't the arguments fit after the paren on
line
> 26?

No, not even on line 27.

[Mattias Nissler (ping if slow), 2010-11-25 09:25:18]

LGTM.

[Nikita (slow), 2010-11-25 12:18:00]

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/78001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:33: "@googlemail.com",
Just a note, that I've been able to login with Google Account that is not
gmail/googlemail account.
It's also not an Apps account.

[danno, 2010-11-25 13:22:00]

LGTM.

[danno, 2010-11-25 13:23:54]

The changes to ensure token/policy path consistency LGTM.

[Jakob Kummerow, 2010-11-27 10:56:29]

I know this CL has already been landed, but the following is important (I
think):

http://codereview.chromium.org/4960003/diff/84002/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4960003/diff/84002/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:233: && !username.empty()) {
I strongly suggest to remove this line. 
The case "username.empty() == true" is handled by CanBeInManagedDomain(), and it
is imperative that kStateNotManaged is set in that case, which won't happen with
this additional check being in place. (Otherwise, the DeviceTokenFetcher will
never signal anything to the outside world, possibly leading to other components
waiting forever.)

[Jakob Kummerow, 2010-11-29 09:48:58]

>
http://codereview.chromium.org/4960003/diff/84002/chrome/browser/policy/devic...
> File chrome/browser/policy/device_token_fetcher.cc (right):
> 
>
http://codereview.chromium.org/4960003/diff/84002/chrome/browser/policy/devic...
> chrome/browser/policy/device_token_fetcher.cc:233: && !username.empty()) {
> I strongly suggest to remove this line. 

OK, just for the record: In our offline discussion you convinced me that
considering the way the authentication system works, this line doesn't hurt us.