Add AndroidManagementClient

Add AndroidManagementClient

Add check_android_management device management request/response.

BUG=602612
Committed: https://crrev.com/bcd3a5f2c390ced304fdd1700c08ed30e09b16a3
Cr-Commit-Position: refs/heads/master@{#390620}

[Polina Bondarenko, 2016-04-15 13:04:28]

pbond@chromium.org changed reviewers:
+ bartfab@chromium.org

[Polina Bondarenko, 2016-04-15 13:04:28]

PTAL.

The code is getting HTTP error code 500, a text of HTTP request itself is
looking good, I mean server URL & etc.

[Polina Bondarenko, 2016-04-18 14:12:44]

Patchset #3 (id:40001) has been deleted

[Polina Bondarenko, 2016-04-18 14:13:19]

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

[commit-bot: I haz the power, 2016-04-18 14:13:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892093002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892093002/60001

[bartfab (slow), 2016-04-18 14:44:46]

Can we get tests for this?

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/android_management_client.cc (right):

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:8: #include
"chrome/browser/chromeos/login/session/user_session_manager.h"
Nit: Not used.

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:9: #include
"chromeos/login/auth/user_context.h"
Nit: Not used.

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:19:
scoped_refptr<net::URLRequestContextGetter> request_context)
Nit: #include "net/url_request/url_request_context_getter.h"

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:26: const
AndroidManagementClient::StatusCallback& callback) {
Nit: Is the |AndroidManagementClient::| prefix really necessary here?

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:27:
DCHECK(service_);
Nit: #include "base/logging.h"

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:33: 
Nit: Why so many blank lines?

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:40:
base::Unretained(this), callback));
Nit: #include "base/bind_helpers.h"

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:44: const
AndroidManagementClient::StatusCallback& callback,
Nit: Is the |AndroidManagementClient::| prefix really necessary here?

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.cc:54:
callback.Run(status == DM_STATUS_SUCCESS);
How can the callback distinguish between "managed," "unmanaged" and "error" if
all we pass is a bool?

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/android_management_client.h (right):

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.h:24: // Interacts with
the device management service and determines whether the
Nit: s/the android/Android/

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.h:34:
scoped_refptr<net::URLRequestContextGetter> request_context);
Nit 1: #include "base/memory/ref_counted.h"
Nit 2: IIRC, scoped_refptr requires the type to be declared, not just
forward-declared.

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.h:37: // Sends a check
android management request to the device management service.
Nit: s/android/Android/

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.h:38: void
CheckAndroidManagement(const std::string& auth_token,
Nit: #include <string>

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.h:42: // Callback for
check android management requests.
Nit: s/android/Android/

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.h:52:
std::unique_ptr<DeviceManagementRequestJob> request_job_;
Nit: #include <memory>

https://codereview.chromium.org/1892093002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/android_management_client.h:54:
DISALLOW_COPY_AND_ASSIGN(AndroidManagementClient);
Nit: #include "base/macros.h"

https://codereview.chromium.org/1892093002/diff/60001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/1892093002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:1036: message
CheckAndroidManagementRequest {}
Nit: Add a note that this request must only be sent for users who are not
managed in Chrome OS.

https://codereview.chromium.org/1892093002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:1038: // Response from
server to device for check Android management request.
Nit: You call it "Android-for-Work service with DPC enforcement" in the request
and "Android management" in the response. Please unify the wording to make it
clear that you are talking about the same thing.

https://codereview.chromium.org/1892093002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:1151: // Check if user
is a managed Android for Work user which enforces a DPC.
The user does not enforce a DPC. A DPC is required for the uer.

[commit-bot: I haz the power, 2016-04-18 15:13:21]

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

[commit-bot: I haz the power, 2016-04-18 15:13:22]

Dry run: This issue passed the CQ dry run.

[Polina Bondarenko, 2016-04-18 15:43:46]

Patchset #1 (id:1) has been deleted

[Polina Bondarenko, 2016-04-18 15:43:54]

Patchset #1 (id:20001) has been deleted

[Polina Bondarenko, 2016-04-18 15:44:05]

Patchset #1 (id:60001) has been deleted

[Polina Bondarenko, 2016-04-18 15:47:51]

Patchset #1 (id:80001) has been deleted

[Polina Bondarenko, 2016-04-18 15:47:59]

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

[commit-bot: I haz the power, 2016-04-18 15:48:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892093002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892093002/100001

[commit-bot: I haz the power, 2016-04-18 16:43:55]

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

[commit-bot: I haz the power, 2016-04-18 16:43:55]

Dry run: This issue passed the CQ dry run.

[Polina Bondarenko, 2016-04-18 19:02:50]

Fixed comments, added test.

[Polina Bondarenko, 2016-04-18 19:40:59]

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

[commit-bot: I haz the power, 2016-04-18 19:41:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892093002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892093002/120001

[commit-bot: I haz the power, 2016-04-18 20:41:01]

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

[commit-bot: I haz the power, 2016-04-18 20:41:02]

Dry run: This issue passed the CQ dry run.

[Polina Bondarenko, 2016-04-19 13:23:35]

Bartosz, PTAL, added unit_test and removed unnecessary dm status constants.

[bartfab (slow), 2016-04-26 14:15:32]

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

[bartfab (slow), 2016-04-26 14:15:33]

Thiemo, would you have time to take a look? I started reviewing last week but
did not have time to finish.

https://codereview.chromium.org/1892093002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client_unittest.cc
(right):

https://codereview.chromium.org/1892093002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client_unittest.cc:51: void
SetUp() { CreateClient(); }
Nit: This one-line method does not look style guide compliant to me.

[Polina Bondarenko, 2016-04-26 14:22:56]

https://codereview.chromium.org/1892093002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client_unittest.cc
(right):

https://codereview.chromium.org/1892093002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client_unittest.cc:51: void
SetUp() { CreateClient(); }
On 2016/04/26 14:15:33, bartfab (slow) wrote:
> Nit: This one-line method does not look style guide compliant to me.

It has happened after "git cl format", should I change it?

[bartfab (slow), 2016-04-26 14:23:58]

https://codereview.chromium.org/1892093002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client_unittest.cc
(right):

https://codereview.chromium.org/1892093002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client_unittest.cc:51: void
SetUp() { CreateClient(); }
On 2016/04/26 14:22:56, Polina Bondarenko wrote:
> On 2016/04/26 14:15:33, bartfab (slow) wrote:
> > Nit: This one-line method does not look style guide compliant to me.
> 
> It has happened after "git cl format", should I change it? 

No, no need to fight with clang-format. I am still surprised by the style.

[Polina Bondarenko, 2016-04-27 14:50:15]

Added the fetch access token to AndroidManagementClient.

Dependent review, which uses AndroidManagementClient:
https://codereview.chromium.org/1892873002/

[Polina Bondarenko, 2016-04-27 15:02:50]

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

[commit-bot: I haz the power, 2016-04-27 15:03:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892093002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892093002/180001

[commit-bot: I haz the power, 2016-04-27 15:22:55]

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

[commit-bot: I haz the power, 2016-04-27 15:22:56]

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

[Polina Bondarenko, 2016-04-27 15:31:10]

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

[commit-bot: I haz the power, 2016-04-27 15:31:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892093002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892093002/200001

[Thiemo Nagel, 2016-04-28 13:27:00]

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client.cc (right):

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.cc:52: void
AndroidManagementClient::SetAccessTokenForTesting(
What would you think of passing the access token through the constructor and
running RequestAccessToken() if the passed token is empty?  That would allow to
remove the SetAccessTokenForTesting() method and the fake_access_token global.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.cc:74:
callback_.Run(policy::AndroidManagementClient::Result::RESULT_ERROR);
It should be possible to drop the namespace and class qualification for
Result::RESULT_ERROR.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.cc:127:
request_job_.reset();
Nit: I'd reset the |request_job_| as early as possible, i.e. before running the
callback.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client.h (right):

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.h:68: void
CheckAndroidManagement(const std::string& access_token);
Please add comment.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.h:77:
DeviceManagementService* service_;
I think that could be made a const pointer.

I'd prefer to call it |dm_service_| (or something similar).  Just |service_|
seems a little generic to me.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client_unittest.cc
(right):

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client_unittest.cc:54: void
ExpectCheckAndroidManagement(const std::string& oauth_token,
Since this is only used once, I'd consider it more readable to delete the method
and move the code to the test.

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
components/policy/proto/device_management_backend.proto:1039: // Response from
server to device for check for Android-for-Work service with DPC
Please wrap at 80 characters.

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
components/policy/proto/device_management_backend.proto:1041: // SC_CONFLICT
http code is returned in case ARC cannot be managed.
What exactly does SC_CONFLICT specify?  Is the user Android-managed or is the
user not Android-managed in that case?

http --> HTTP

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
components/policy/proto/device_management_backend.proto:1102: //  
check_android_management : check_android_management_request
Please delete space before colon.

[Polina Bondarenko, 2016-04-28 16:18:40]

Thanks for reviewing this CL! I fixed comments.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client.cc (right):

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.cc:52: void
AndroidManagementClient::SetAccessTokenForTesting(
On 2016/04/28 13:26:59, Thiemo Nagel wrote:
> What would you think of passing the access token through the constructor and
> running RequestAccessToken() if the passed token is empty?  That would allow
to
> remove the SetAccessTokenForTesting() method and the fake_access_token global.

Removed fake access token at all. It's possible to use
FakeProfileOAuth2TokenService as a token_service_ to issue right tokens. See
unittest.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.cc:74:
callback_.Run(policy::AndroidManagementClient::Result::RESULT_ERROR);
On 2016/04/28 13:26:59, Thiemo Nagel wrote:
> It should be possible to drop the namespace and class qualification for
> Result::RESULT_ERROR.

Done.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.cc:127:
request_job_.reset();
On 2016/04/28 13:26:59, Thiemo Nagel wrote:
> Nit: I'd reset the |request_job_| as early as possible, i.e. before running
the
> callback.

Done.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client.h (right):

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.h:68: void
CheckAndroidManagement(const std::string& access_token);
On 2016/04/28 13:26:59, Thiemo Nagel wrote:
> Please add comment.

Done.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client.h:77:
DeviceManagementService* service_;
On 2016/04/28 13:26:59, Thiemo Nagel wrote:
> I think that could be made a const pointer.
> 
> I'd prefer to call it |dm_service_| (or something similar).  Just |service_|
> seems a little generic to me.

Done.

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/android_management_client_unittest.cc
(right):

https://codereview.chromium.org/1892093002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/android_management_client_unittest.cc:54: void
ExpectCheckAndroidManagement(const std::string& oauth_token,
On 2016/04/28 13:27:00, Thiemo Nagel wrote:
> Since this is only used once, I'd consider it more readable to delete the
method
> and move the code to the test.

Done.

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
components/policy/proto/device_management_backend.proto:1039: // Response from
server to device for check for Android-for-Work service with DPC
On 2016/04/28 13:27:00, Thiemo Nagel wrote:
> Please wrap at 80 characters.

Done.

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
components/policy/proto/device_management_backend.proto:1041: // SC_CONFLICT
http code is returned in case ARC cannot be managed.
On 2016/04/28 13:27:00, Thiemo Nagel wrote:
> What exactly does SC_CONFLICT specify?  Is the user Android-managed or is the
> user not Android-managed in that case?
> 
> http --> HTTP

Done.

https://codereview.chromium.org/1892093002/diff/220001/components/policy/prot...
components/policy/proto/device_management_backend.proto:1102: //  
check_android_management : check_android_management_request
On 2016/04/28 13:27:00, Thiemo Nagel wrote:
> Please delete space before colon.

Done.

[Thiemo Nagel, 2016-04-29 10:38:19]

LGTM!

Nit: Whenever you rebase, please do it as a separate upload (and call it
"rebase") because that makes it easier for reviewers to keep track of which
changes you did yourself and which are part of the rebase.

[Polina Bondarenko, 2016-04-29 10:46:44]

On 2016/04/29 10:38:19, Thiemo Nagel wrote:
> LGTM!
> 
> Nit: Whenever you rebase, please do it as a separate upload (and call it
> "rebase") because that makes it easier for reviewers to keep track of which
> changes you did yourself and which are part of the rebase.

Thanks for reviewing, Thiemo. And sorry for rebase:(

[Polina Bondarenko, 2016-04-29 10:46:50]

The CQ bit was checked by pbond@chromium.org

[commit-bot: I haz the power, 2016-04-29 10:47:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892093002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892093002/240001

[commit-bot: I haz the power, 2016-04-29 11:46:31]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-04-30 17:25:05]

Patchset 8 (id:??) landed as
https://crrev.com/bcd3a5f2c390ced304fdd1700c08ed30e09b16a3
Cr-Commit-Position: refs/heads/master@{#390620}

[commit-bot: I haz the power, 2016-04-30 18:00:09]

Description was changed from

==========
Add AndroidManagementClient

Add check_android_management device management request/response.

BUG=602612
==========

to

==========
Add AndroidManagementClient

Add check_android_management device management request/response.

BUG=602612

Committed: https://crrev.com/bcd3a5f2c390ced304fdd1700c08ed30e09b16a3
Cr-Commit-Position: refs/heads/master@{#390620}
==========