Get enrollment token from DMServer when an Active Directory user uses ARC

Get enrollment token from DMServer when an Active Directory user uses ARC

When ARC asks Chrome for authentication info, if the current user is an
Active Directory user, Chrome gets an enrollment token from DMServer
instead of the auth codes which are used for regular GAIA accounts.

This CL also enables the Play Store icon for Active Directory users.

BUG=680954, 680955, 680956
Review-Url: https://codereview.chromium.org/2655873002
Cr-Commit-Position: refs/heads/master@{#448682}
Committed: https://chromium.googlesource.com/chromium/src/+/56c49db6fa925b9190d362d2a3d1207f7194242d

[Marton Hunyady, 2017-01-30 16:22:25]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 16:22:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 17:41:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-30 17:41:51]

Dry run: This issue passed the CQ dry run.

[Marton Hunyady, 2017-01-31 11:47:33]

Description was changed from

==========
Add constants required for Active Directory requests to DMServer.

BUG=
==========

to

==========
Get enrollment token from DMServer when an Active Directory user uses ARC

When ARC asks Chrome for authentication info, if the current user is an
Active Directory user, Chrome gets an enrollment token from DMServer
instead of the auth codes which are used for regular GAIA accounts.

This CL also enables the Play Store icon for Active Directory users.

BUG=680954, 680956
==========

[Marton Hunyady, 2017-01-31 11:47:33]

hunyadym@chromium.org changed reviewers:
+ tnagel@chromium.org

[Luis Héctor Chávez, 2017-01-31 18:01:08]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2017-01-31 18:01:10]

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:111: if (!is_enforced) {
nit: elide braces (or use a ternary).

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:243:
weak_ptr_factory_.GetWeakPtr()));
nit: return; to avoid the else block altogether.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc
(right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc:37:
callback_ = callback;
DCHECK(callback_.is_null()); (due to
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/auth/arc_aut...
)

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc:80:
callback_.Run(
base::ResetAndReturn(&callback_).Run(...)

Same in L76.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h
(right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:14:
#include "chrome/browser/chromeos/policy/dm_token_storage.h"
nit: this can also be fwd-declared.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:35:
void Fetch(const FetchCallback& callback) override;
Can this be in private:?

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:38:
void OnDMTokenAvailable(const std::string& dm_token);
iwyu: #include <string>

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/auth/arc_auth_info_fetcher.h (right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_auth_info_fetcher.h:22: // Fetch() should
be called once par instance, and it is expected that
nit: not your fault, but please s/par/per/

https://codereview.chromium.org/2655873002/diff/60001/components/policy_strin...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/2655873002/diff/60001/components/policy_strin...
components/policy_strings.grdp:59: ARC++ is not enabled for this domain
We cannot use this wording. At the very least s/ARC++/Play Store/, but I am not
the expert.

Please send an email to mitsuji@ for the exact way we should phrase this.

[Marton Hunyady, 2017-01-31 18:16:21]

hunyadym@chromium.org changed reviewers:
+ rsesek@chromium.org

[Marton Hunyady, 2017-02-01 12:20:51]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 12:21:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marton Hunyady, 2017-02-01 12:21:51]

Thank you for the review!

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:111: if (!is_enforced) {
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> nit: elide braces (or use a ternary).

Done.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:243:
weak_ptr_factory_.GetWeakPtr()));
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> nit: return; to avoid the else block altogether.

Done.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc
(right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc:37:
callback_ = callback;
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> DCHECK(callback_.is_null()); (due to
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/auth/arc_aut...
> )

Done.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.cc:80:
callback_.Run(
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> base::ResetAndReturn(&callback_).Run(...)
> 
> Same in L76.

Done.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h
(right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> 2017

Done.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:14:
#include "chrome/browser/chromeos/policy/dm_token_storage.h"
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> nit: this can also be fwd-declared.

Done.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:35:
void Fetch(const FetchCallback& callback) override;
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> Can this be in private:?

It can be, but since this is public in the ArcAuthInfoFetcher interface, I don't
think it would be useful.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:38:
void OnDMTokenAvailable(const std::string& dm_token);
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> iwyu: #include <string>

Done.

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/auth/arc_auth_info_fetcher.h (right):

https://codereview.chromium.org/2655873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/auth/arc_auth_info_fetcher.h:22: // Fetch() should
be called once par instance, and it is expected that
On 2017/01/31 18:01:09, Luis Héctor Chávez wrote:
> nit: not your fault, but please s/par/per/

Done.

[commit-bot: I haz the power, 2017-02-01 12:22:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 12:22:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marton Hunyady, 2017-02-01 13:04:16]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 13:04:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marton Hunyady, 2017-02-01 13:04:49]

hunyadym@chromium.org changed reviewers:
+ achuith@chromium.org

[Marton Hunyady, 2017-02-01 13:04:49]

achuith: PTAL at
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc

[commit-bot: I haz the power, 2017-02-01 14:05:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 14:05:13]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-02-02 00:23:05]

On 2017/02/01 13:04:49, Marton Hunyady wrote:
> achuith: PTAL at
> chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc

rubberstamp lgtm for this file, didn't look at anything else

[Thiemo Nagel, 2017-02-02 13:24:59]

I've only looked at */policy/* and at the enrollment helper.

https://codereview.chromium.org/2655873002/diff/120001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2655873002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:361:
case policy::DM_STATUS_SERVICE_ARC_DISABLED:
I think that should be NOTREACHED instead.  (And please add a break; at the
end.)

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
File components/policy/core/browser/cloud/message_util.cc (right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
components/policy/core/browser/cloud/message_util.cc:52: return
IDS_POLICY_DM_STATUS_SERVICE_ARC_DISABLED;
I think it's fair to return UNKNOWN_ERROR here since neither enrollment nor
chrome://policy page should ever display that error message.

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:261: case
DM_STATUS_SERVICE_ARC_DISABLED:
Since this is error is not related to policy fetch, I don't think it should
trigger a policy refresh.

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/enterprise_metrics.h (right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
components/policy/core/common/cloud/enterprise_metrics.h:232: // ARC is disabled
for this domain.
Not needed: These are enrollment metrics.

https://codereview.chromium.org/2655873002/diff/120001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy_stri...
components/policy_strings.grdp:58: <message
name="IDS_POLICY_DM_STATUS_SERVICE_ARC_DISABLED" desc="Message indicating that
Play Store is not enabled for this domain.">
Not needed: Never surfaced anywhere.

[Marton Hunyady, 2017-02-02 18:19:06]

Thanks for the review!

I also added a browsertest for ArcActiveDirectoryEnrollmentTokenFetcher.

https://codereview.chromium.org/2655873002/diff/120001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2655873002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:361:
case policy::DM_STATUS_SERVICE_ARC_DISABLED:
On 2017/02/02 13:24:59, Thiemo Nagel (slow) wrote:
> I think that should be NOTREACHED instead.  (And please add a break; at the
> end.)

Done.

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
File components/policy/core/browser/cloud/message_util.cc (right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
components/policy/core/browser/cloud/message_util.cc:52: return
IDS_POLICY_DM_STATUS_SERVICE_ARC_DISABLED;
On 2017/02/02 13:24:59, Thiemo Nagel (slow) wrote:
> I think it's fair to return UNKNOWN_ERROR here since neither enrollment nor
> chrome://policy page should ever display that error message.

Done.

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:261: case
DM_STATUS_SERVICE_ARC_DISABLED:
On 2017/02/02 13:24:59, Thiemo Nagel (slow) wrote:
> Since this is error is not related to policy fetch, I don't think it should
> trigger a policy refresh.

Done.

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/enterprise_metrics.h (right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy/core...
components/policy/core/common/cloud/enterprise_metrics.h:232: // ARC is disabled
for this domain.
On 2017/02/02 13:24:59, Thiemo Nagel (slow) wrote:
> Not needed: These are enrollment metrics.

Done.

https://codereview.chromium.org/2655873002/diff/120001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/2655873002/diff/120001/components/policy_stri...
components/policy_strings.grdp:58: <message
name="IDS_POLICY_DM_STATUS_SERVICE_ARC_DISABLED" desc="Message indicating that
Play Store is not enabled for this domain.">
On 2017/02/02 13:24:59, Thiemo Nagel (slow) wrote:
> Not needed: Never surfaced anywhere.

Done.

[Marton Hunyady, 2017-02-02 18:19:15]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 18:19:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 19:19:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 19:19:03]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-02-03 12:45:49]

*/policy/* lgtm with nit

https://codereview.chromium.org/2655873002/diff/140001/components/policy/core...
File components/policy/core/browser/cloud/message_util.cc (right):

https://codereview.chromium.org/2655873002/diff/140001/components/policy/core...
components/policy/core/browser/cloud/message_util.cc:52: return
IDS_POLICY_DM_STATUS_UNKNOWN_ERROR;
Nit: I'd suggest to add a comment that the ARC_DISABLED error is never surfaced
in UI.

[Marton Hunyady, 2017-02-03 13:23:26]

https://codereview.chromium.org/2655873002/diff/140001/components/policy/core...
File components/policy/core/browser/cloud/message_util.cc (right):

https://codereview.chromium.org/2655873002/diff/140001/components/policy/core...
components/policy/core/browser/cloud/message_util.cc:52: return
IDS_POLICY_DM_STATUS_UNKNOWN_ERROR;
On 2017/02/03 12:45:49, Thiemo Nagel (slow) wrote:
> Nit: I'd suggest to add a comment that the ARC_DISABLED error is never
surfaced
> in UI.

Done.

[Marton Hunyady, 2017-02-03 13:23:34]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 13:23:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 14:15:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 14:15:43]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2017-02-03 16:16:51]

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:109:
account_info->enrollment_token = auth_info;
be aware that not setting account_info->auth_code always triggers a "GMS Sign-In
Internal Error" in Android.

How do you plan to work around this? (Keep in mind that .mojom changes must land
in Chrome first to avoid version races)

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:286: void
ArcAuthService::OnEnrollmentTokenFetched(
nit: can this be before OnAuthCodeFetched so it better matches the order in
which they are mentioned in ArcAuthService::RequestAccountInfoInternal? Same in
the .h

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:297: notifier_->Notify(true,
enrollment_token,
nit: use named constants (e.g. constexpr bool kIsEnforced = true) or add a
comment with the name of the param after the param (e.g. true /*is_enforced*/)

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc
(right):

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:35:
namespace {
nit: newline before, for consistency

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:42:
net::URLRequestJob* ResponseJob(net::URLRequest* request,
iwyu nit: #include "net/url_request/url_request.h"

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:43:
net::NetworkDelegate* network_delegate) {
iwyu nit: #include "net/base/network_delegate.h"

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:66:
net::HttpRequestHeaders request_headers = request->extra_request_headers();
iwyu nit: #include "net/http/http_request_headers.h"

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:100:
fake_cryptohome_client_ = new chromeos::FakeCryptohomeClient;
it's better to do

auto cryptohome_client = base::MakeUnique<chromeos::CryptohomeClient>();
fake_cryptohome_client_ = cryptohome_client.get();
chromeos::DBusThreadManager::GetSetterForTesting()->SetCryptohomeClient(std::move(cryptohome_client));

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:133:
run_loop.Run();
can this work with RunUntilIdle() and removing the run_loop->Quit() above? If it
can't (e.g. due to thread hops), mention that fact in a comment (similar to what
you did in L167).

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:173:
chromeos::FakeCryptohomeClient* fake_cryptohome_client_;
mention that the DBusThreadManager will own this.

[Marton Hunyady, 2017-02-03 18:11:12]

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:109:
account_info->enrollment_token = auth_info;
On 2017/02/03 16:16:50, Luis Héctor Chávez wrote:
> be aware that not setting account_info->auth_code always triggers a "GMS
Sign-In
> Internal Error" in Android.
> 
> How do you plan to work around this? (Keep in mind that .mojom changes must
land
> in Chrome first to avoid version races)

Until we make it possible in DMServer to get enrollment tokens (by returning an
enrollment token instead of an HTTP error) the enrollment flow won't work, so I
think it's fine to show an error on the Android side in this case. (Also,
according to my tests, the user is still able to log in and use the Play Store
in this case with a regular GAIA account.) The admin has to explicitly enable
ARC in ActiveDirectory to make the Play Store appear and be able to start this
flow, so it's not likely users will see this error).

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:286: void
ArcAuthService::OnEnrollmentTokenFetched(
On 2017/02/03 16:16:50, Luis Héctor Chávez wrote:
> nit: can this be before OnAuthCodeFetched so it better matches the order in
> which they are mentioned in ArcAuthService::RequestAccountInfoInternal? Same
in
> the .h

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:297: notifier_->Notify(true,
enrollment_token,
On 2017/02/03 16:16:50, Luis Héctor Chávez wrote:
> nit: use named constants (e.g. constexpr bool kIsEnforced = true) or add a
> comment with the name of the param after the param (e.g. true /*is_enforced*/)

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc
(right):

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:35:
namespace {
On 2017/02/03 16:16:51, Luis Héctor Chávez wrote:
> nit: newline before, for consistency

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:42:
net::URLRequestJob* ResponseJob(net::URLRequest* request,
On 2017/02/03 16:16:50, Luis Héctor Chávez wrote:
> iwyu nit: #include "net/url_request/url_request.h"

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:43:
net::NetworkDelegate* network_delegate) {
On 2017/02/03 16:16:51, Luis Héctor Chávez wrote:
> iwyu nit: #include "net/base/network_delegate.h"

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:66:
net::HttpRequestHeaders request_headers = request->extra_request_headers();
On 2017/02/03 16:16:51, Luis Héctor Chávez wrote:
> iwyu nit: #include "net/http/http_request_headers.h"

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:100:
fake_cryptohome_client_ = new chromeos::FakeCryptohomeClient;
On 2017/02/03 16:16:50, Luis Héctor Chávez wrote:
> it's better to do
> 
> auto cryptohome_client = base::MakeUnique<chromeos::CryptohomeClient>();
> fake_cryptohome_client_ = cryptohome_client.get();
>
chromeos::DBusThreadManager::GetSetterForTesting()->SetCryptohomeClient(std::move(cryptohome_client));

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:133:
run_loop.Run();
On 2017/02/03 16:16:50, Luis Héctor Chávez wrote:
> can this work with RunUntilIdle() and removing the run_loop->Quit() above? If
it
> can't (e.g. due to thread hops), mention that fact in a comment (similar to
what
> you did in L167).

Done.

https://codereview.chromium.org/2655873002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:173:
chromeos::FakeCryptohomeClient* fake_cryptohome_client_;
On 2017/02/03 16:16:50, Luis Héctor Chávez wrote:
> mention that the DBusThreadManager will own this.

Done.

[Marton Hunyady, 2017-02-06 12:48:31]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 12:48:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 13:20:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 13:20:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marton Hunyady, 2017-02-06 13:41:02]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 13:41:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 14:44:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 14:44:31]

Dry run: This issue passed the CQ dry run.

[Marton Hunyady, 2017-02-06 16:39:57]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 16:40:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 17:39:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 17:39:45]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2017-02-06 22:00:15]

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:61: // GAIA account. An exception is
Kiosk mode and Active Directory users, which
nit: reword to "Play Store requires either an appropriate application install
mechanism. Normal users do this through GAIA, but Kiosk and Active Directory
...".

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:65: // Arc is allowed if we have
either...
nit: s/Arc/ARC/

also, can we split off this huge conditional into various checks? Let's also
apply de Morgan's law to get rid of the outermost paren:

const bool hasGaiaAccount = user && user->HasGaiaAccount();
const bool ...

if (!hasGaiaAccount && !...) {
  VLOG(1) ...
}

alternatively instead of having 'const bool' variables, you can extract all
three checks to standalone functions (esp. after addressing the comment below),
which will be more readable.

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:70:
base::CommandLine::ForCurrentProcess()->HasSwitch(
Please move all flag-related logic to components/arc/arc_util.cc

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util_unittest.cc:41: case
user_manager::USER_TYPE_REGULAR:
nit: // fallthrough

https://codereview.chromium.org/2655873002/diff/260001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2655873002/diff/260001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:305: "enable-android-with-active-directory";
We're trying to consolidate the multiple ARC-related flags into one (see
https://codereview.chromium.org/2671463002/ ). Can you rebase on top of that
change and add a new entry to arc-availability to indicate AD support?

[Marton Hunyady, 2017-02-07 14:52:27]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 14:52:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marton Hunyady, 2017-02-07 14:53:42]

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_util.cc (right):

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:61: // GAIA account. An exception is
Kiosk mode and Active Directory users, which
On 2017/02/06 22:00:15, Luis Héctor Chávez wrote:
> nit: reword to "Play Store requires either an appropriate application install
> mechanism. Normal users do this through GAIA, but Kiosk and Active Directory
> ...".

Done.

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:65: // Arc is allowed if we have
either...
On 2017/02/06 22:00:15, Luis Héctor Chávez wrote:
> nit: s/Arc/ARC/
> 
> also, can we split off this huge conditional into various checks? Let's also
> apply de Morgan's law to get rid of the outermost paren:
> 
> const bool hasGaiaAccount = user && user->HasGaiaAccount();
> const bool ...
> 
> if (!hasGaiaAccount && !...) {
>   VLOG(1) ...
> }
> 
> alternatively instead of having 'const bool' variables, you can extract all
> three checks to standalone functions (esp. after addressing the comment
below),
> which will be more readable.

Done (without standalone functions).

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util.cc:70:
base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/02/06 22:00:15, Luis Héctor Chávez wrote:
> Please move all flag-related logic to components/arc/arc_util.cc

Done.

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_util_unittest.cc (right):

https://codereview.chromium.org/2655873002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_util_unittest.cc:41: case
user_manager::USER_TYPE_REGULAR:
On 2017/02/06 22:00:15, Luis Héctor Chávez wrote:
> nit: // fallthrough

Done.

https://codereview.chromium.org/2655873002/diff/260001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2655873002/diff/260001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:305: "enable-android-with-active-directory";
On 2017/02/06 22:00:15, Luis Héctor Chávez wrote:
> We're trying to consolidate the multiple ARC-related flags into one (see
> https://codereview.chromium.org/2671463002/ ). Can you rebase on top of that
> change and add a new entry to arc-availability to indicate AD support?

Done.

[Marton Hunyady, 2017-02-07 15:57:19]

Description was changed from

==========
Get enrollment token from DMServer when an Active Directory user uses ARC

When ARC asks Chrome for authentication info, if the current user is an
Active Directory user, Chrome gets an enrollment token from DMServer
instead of the auth codes which are used for regular GAIA accounts.

This CL also enables the Play Store icon for Active Directory users.

BUG=680954, 680956
==========

to

==========
Get enrollment token from DMServer when an Active Directory user uses ARC

When ARC asks Chrome for authentication info, if the current user is an
Active Directory user, Chrome gets an enrollment token from DMServer
instead of the auth codes which are used for regular GAIA accounts.

This CL also enables the Play Store icon for Active Directory users.

BUG=680954, 680955, 680956
==========

[commit-bot: I haz the power, 2017-02-07 16:01:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 16:01:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marton Hunyady, 2017-02-07 16:09:06]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 16:09:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Luis Héctor Chávez, 2017-02-07 16:33:04]

lgtm with nits.

+hidehiko@ FYI (for the flag change)

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h
(right):

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:19:
}
nit: // namespace enterprise_management

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:24:
}
nit: // namespace policy

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc
(right):

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:37:
namespace arc {
does this work without the namespace? We're trying to move all tests to use the
anonymous namespace only.

https://codereview.chromium.org/2655873002/diff/300001/components/arc/arc_uti...
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2655873002/diff/300001/components/arc/arc_uti...
components/arc/arc_util.cc:68: if
(command_line->HasSwitch(chromeos::switches::kArcAvailability)) {
nit:

if (!command_line->HasSwitch(...))
  return false;

return ...

https://codereview.chromium.org/2655873002/diff/300001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/2655873002/diff/300001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_constants.h:114: // Service
error: Arc is not enabled on this domain.
nit: ARC

[commit-bot: I haz the power, 2017-02-07 17:09:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 17:09:52]

Dry run: This issue passed the CQ dry run.

[Marton Hunyady, 2017-02-07 17:27:47]

The CQ bit was checked by hunyadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 17:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marton Hunyady, 2017-02-07 17:28:48]

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h
(right):

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:19:
}
On 2017/02/07 16:33:03, Luis Héctor Chávez wrote:
> nit: // namespace enterprise_management

Done.

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher.h:24:
}
On 2017/02/07 16:33:03, Luis Héctor Chávez wrote:
> nit: // namespace policy

Done.

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc
(right):

https://codereview.chromium.org/2655873002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/auth/arc_active_directory_enrollment_token_fetcher_browsertest.cc:37:
namespace arc {
On 2017/02/07 16:33:03, Luis Héctor Chávez wrote:
> does this work without the namespace? We're trying to move all tests to use
the
> anonymous namespace only.

Done.

https://codereview.chromium.org/2655873002/diff/300001/components/arc/arc_uti...
File components/arc/arc_util.cc (right):

https://codereview.chromium.org/2655873002/diff/300001/components/arc/arc_uti...
components/arc/arc_util.cc:68: if
(command_line->HasSwitch(chromeos::switches::kArcAvailability)) {
On 2017/02/07 16:33:03, Luis Héctor Chávez wrote:
> nit:
> 
> if (!command_line->HasSwitch(...))
>   return false;
> 
> return ...

Done.

https://codereview.chromium.org/2655873002/diff/300001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/2655873002/diff/300001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_constants.h:114: // Service
error: Arc is not enabled on this domain.
On 2017/02/07 16:33:03, Luis Héctor Chávez wrote:
> nit: ARC

Done.

[Marton Hunyady, 2017-02-07 17:29:39]

rsesek: PTAL at components/arc/common/auth.mojom

[commit-bot: I haz the power, 2017-02-07 18:24:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 18:24:46]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-02-07 18:35:13]

lgtm

[Marton Hunyady, 2017-02-07 18:38:07]

The CQ bit was checked by hunyadym@chromium.org

[Marton Hunyady, 2017-02-07 18:38:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, tnagel@chromium.org, lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2655873002/#ps320001
(title: "Fix Luis's comments")

[commit-bot: I haz the power, 2017-02-07 18:38:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 18:57:46]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1486492687002960,
"parent_rev": "61bfd27852074dac34ab7b3225fcbf92d9b0689f", "commit_rev":
"56c49db6fa925b9190d362d2a3d1207f7194242d"}

[commit-bot: I haz the power, 2017-02-07 19:01:47]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1486492687002960,
"parent_rev": "61bfd27852074dac34ab7b3225fcbf92d9b0689f", "commit_rev":
"56c49db6fa925b9190d362d2a3d1207f7194242d"}

[commit-bot: I haz the power, 2017-02-07 19:04:43]

Description was changed from

==========
Get enrollment token from DMServer when an Active Directory user uses ARC

When ARC asks Chrome for authentication info, if the current user is an
Active Directory user, Chrome gets an enrollment token from DMServer
instead of the auth codes which are used for regular GAIA accounts.

This CL also enables the Play Store icon for Active Directory users.

BUG=680954, 680955, 680956
==========

to

==========
Get enrollment token from DMServer when an Active Directory user uses ARC

When ARC asks Chrome for authentication info, if the current user is an
Active Directory user, Chrome gets an enrollment token from DMServer
instead of the auth codes which are used for regular GAIA accounts.

This CL also enables the Play Store icon for Active Directory users.

BUG=680954, 680955, 680956

Review-Url: https://codereview.chromium.org/2655873002
Cr-Commit-Position: refs/heads/master@{#448682}
Committed:
https://chromium.googlesource.com/chromium/src/+/56c49db6fa925b9190d362d2a3d1...
==========

[commit-bot: I haz the power, 2017-02-07 19:04:44]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/56c49db6fa925b9190d362d2a3d1...