Fetch policy with refresh token.

Fetch policy with refresh token.

WildcardLoginChecker and UserCloudPolicyManagerChromeOS now use this mechanism to fetch policy.

BUG=478432, 470984, 462036, 480447
TEST=manual

Committed: https://crrev.com/174b27ddc1d2ef0600219bd0d2b9595bde975495
Cr-Commit-Position: refs/heads/master@{#327375}

[achuithb, 2015-04-27 23:28:48]

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

[commit-bot: I haz the power, 2015-04-27 23:29:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108983002/1

[achuithb, 2015-04-27 23:30:31]

achuith@chromium.org changed reviewers:
+ mnissler@chromium.org, xiyuan@chromium.org

[achuithb, 2015-04-27 23:30:31]

Xiyuan, Mattias, please take a look.

[commit-bot: I haz the power, 2015-04-27 23:58:06]

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

[commit-bot: I haz the power, 2015-04-27 23:58:06]

Dry run: This issue passed the CQ dry run.

[achuithb, 2015-04-28 00:19:40]

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

[commit-bot: I haz the power, 2015-04-28 00:19:57]

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

[commit-bot: I haz the power, 2015-04-28 00:40:40]

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

[commit-bot: I haz the power, 2015-04-28 00:40:41]

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

[xiyuan, 2015-04-28 01:52:58]

LGTM with greet build + Mattias's review. :)

https://codereview.chromium.org/1108983002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/1108983002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:159:
(user_session_manager->has_auth_cookies() || !refresh_token.empty()) &&
With the refresh token code added to UserCloudPolicyManagerChromeOS,
has_auth_cookies() check is no longer necessary since
UserCloudPolicyManagerChromeOS::FetchPolicyOAuthToken is smart to handle both
auth cookie and refresh token cases.

That is, this line (and the TODO at line 152 :) can be deleted now.

[achuithb, 2015-04-28 06:28:58]

achuith@chromium.org changed reviewers:
+ dpolukhin@google.com

[achuithb, 2015-04-28 06:28:59]

Dmitry for review + owner lgtm for chromeos/login

https://codereview.chromium.org/1108983002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/1108983002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:159:
(user_session_manager->has_auth_cookies() || !refresh_token.empty()) &&
On 2015/04/28 01:52:58, xiyuan wrote:
> With the refresh token code added to UserCloudPolicyManagerChromeOS,
> has_auth_cookies() check is no longer necessary since
> UserCloudPolicyManagerChromeOS::FetchPolicyOAuthToken is smart to handle both
> auth cookie and refresh token cases.
> 
> That is, this line (and the TODO at line 152 :) can be deleted now.

Done.

[achuithb, 2015-04-28 06:29:07]

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

[achuithb, 2015-04-28 06:29:08]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1108983002/#ps40001
(title: "comment and unit test")

[commit-bot: I haz the power, 2015-04-28 06:29:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108983002/40001

[Mattias Nissler (ping if slow), 2015-04-28 06:50:43]

LGTM w/ nits

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h (right):

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:58: void
StartWithRefreshToken(const std::string& oauth2_refresh_token);
Please turn this into a 3rd constructor for consistency.

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:292: if
(!refresh_token_.empty()) {
It's kinda inconsistent that the signin_context below gets accessed via
chromeos::login::GetSigninContext(), while we have to pass the refresh token at
construction time.

[commit-bot: I haz the power, 2015-04-28 07:01:31]

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

[commit-bot: I haz the power, 2015-04-28 07:01:32]

Dry run: This issue passed the CQ dry run.

[Dmitry Polukhin, 2015-04-28 08:59:11]

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

[Dmitry Polukhin, 2015-04-28 08:59:11]

LGTM

You also need to add something like patch set #1 from my CL
https://codereview.chromium.org/1107873002/#ps1 to enabled corresponding tests.
Tried patch set #1 on top of your CL and it works locally. You can integrate my
changes into your CL or I can commit them later, it is up to you.

[Nikita (slow), 2015-04-28 13:17:29]

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

[Nikita (slow), 2015-04-28 13:17:30]

lgtm

I suggest include my CL  as a part of yours to make sure that those policy tests
now pass with webview https://codereview.chromium.org/1052683006/

[achuithb, 2015-04-28 18:16:28]

On 2015/04/28 08:59:11, Dmitry Polukhin wrote:
> LGTM
> 
> You also need to add something like patch set #1 from my CL
> https://codereview.chromium.org/1107873002/#ps1 to enabled corresponding
tests.
> Tried patch set #1 on top of your CL and it works locally. You can integrate
my
> changes into your CL or I can commit them later, it is up to you.

Thank you for the review. I'll add your changes in a follow-up CL if you don't
mind.

[achuithb, 2015-04-28 18:17:16]

On 2015/04/28 13:17:30, Nikita Kostylev wrote:
> lgtm
> 
> I suggest include my CL  as a part of yours to make sure that those policy
tests
> now pass with webview https://codereview.chromium.org/1052683006/

Thank you for the review - I'll do this in a follow-up CL as well if you don't
mind.

[achuithb, 2015-04-28 18:20:08]

Thank you for the review, Mattias. I'm no longer passing in the refresh_token as
an argument.

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h (right):

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:58: void
StartWithRefreshToken(const std::string& oauth2_refresh_token);
On 2015/04/28 06:50:43, Mattias Nissler wrote:
> Please turn this into a 3rd constructor for consistency.

I considered this. The first problem is that the type of auth_code and
refresh_token is the same (const std::string&), so the ctor signatures would be
the same. The bigger issue is that multiple ctors are just a variation of
function overloading which is not recommended stylistically. 

I think the way to go is to have one ctor, and have methods
StartWithBrowserContext, StartWithAuthCode, and StartWithRefreshToken. I'll do
this in a follow-up cleanup CL to avoid clutter in this CL. 

Hope that's ok.

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:292: if
(!refresh_token_.empty()) {
On 2015/04/28 06:50:43, Mattias Nissler wrote:
> It's kinda inconsistent that the signin_context below gets accessed via
> chromeos::login::GetSigninContext(), while we have to pass the refresh token
at
> construction time.

Done.

[achuithb, 2015-04-28 18:20:23]

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

[achuithb, 2015-04-28 18:20:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, xiyuan@chromium.org, nkostylev@chromium.org,
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1108983002/#ps60001
(title: "Mattias feedback")

[achuithb, 2015-04-28 18:21:24]

Xiyuan - do you mind taking one last look before I land this?

[commit-bot: I haz the power, 2015-04-28 18:22:29]

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

[xiyuan, 2015-04-28 18:33:41]

LGTM++

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

https://codereview.chromium.org/1108983002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:18:
#include "chrome/browser/chromeos/login/session/user_session_manager.h"
nit: This probably can go now.

[achuithb, 2015-04-28 18:56:22]

Thank you, Xiyuan!

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

https://codereview.chromium.org/1108983002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:18:
#include "chrome/browser/chromeos/login/session/user_session_manager.h"
On 2015/04/28 18:33:41, xiyuan wrote:
> nit: This probably can go now.

Done. Good catch!

[achuithb, 2015-04-28 18:56:29]

The CQ bit was checked by achuith@chromium.org

[achuithb, 2015-04-28 18:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, xiyuan@chromium.org, nkostylev@chromium.org,
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1108983002/#ps80001
(title: "remove unnecessary include")

[commit-bot: I haz the power, 2015-04-28 18:57:29]

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

[commit-bot: I haz the power, 2015-04-28 19:53:04]

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

[commit-bot: I haz the power, 2015-04-28 19:53:05]

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_...)

[achuithb, 2015-04-28 20:04:39]

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

[achuithb, 2015-04-28 20:04:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, xiyuan@chromium.org, nkostylev@chromium.org,
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1108983002/#ps100001
(title: "fix browser tests")

[commit-bot: I haz the power, 2015-04-28 20:05:06]

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

[xiyuan, 2015-04-28 20:06:42]

On 2015/04/28 19:53:05, I haz the power (commit-bot) wrote:
> 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_...)

The failures are in a unit_test, probalby because there is no
UserSessionManager. We might need to do NULL check in
UserCloudPolicyManagerChromeOS::FetchPolicyOAuthToken.

[achuithb, 2015-04-28 20:07:49]

On 2015/04/28 20:06:42, xiyuan wrote:
> On 2015/04/28 19:53:05, I haz the power (commit-bot) wrote:
> > 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_...)
> 
> The failures are in a unit_test, probalby because there is no
> UserSessionManager. We might need to do NULL check in
> UserCloudPolicyManagerChromeOS::FetchPolicyOAuthToken.

I'll take a look!

[achuithb, 2015-04-28 20:08:26]

The CQ bit was unchecked by achuith@chromium.org

[achuithb, 2015-04-28 20:08:34]

Patchset #6 (id:100001) has been deleted

[achuithb, 2015-04-28 20:58:23]

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

[achuithb, 2015-04-28 20:58:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpolukhin@chromium.org, xiyuan@chromium.org, nkostylev@chromium.org,
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1108983002/#ps120001
(title: "Fix user_cloud_policy_manager_chromeos_unittest")

[commit-bot: I haz the power, 2015-04-28 20:58:38]

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

[achuithb, 2015-04-28 20:58:43]

On 2015/04/28 20:07:49, achuithb wrote:
> On 2015/04/28 20:06:42, xiyuan wrote:
> > On 2015/04/28 19:53:05, I haz the power (commit-bot) wrote:
> > > 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_...)
> > 
> > The failures are in a unit_test, probalby because there is no
> > UserSessionManager. We might need to do NULL check in
> > UserCloudPolicyManagerChromeOS::FetchPolicyOAuthToken.
> 
> I'll take a look!

We need a fake UserManager. PTAL, Xiyuan.

[xiyuan, 2015-04-28 21:02:39]

SLGTM

[achuithb, 2015-04-28 21:03:23]

On 2015/04/28 21:02:39, xiyuan wrote:
> SLGTM

danke schön!

[achuithb, 2015-04-28 22:04:57]

The CQ bit was unchecked by achuith@chromium.org

[achuithb, 2015-04-28 22:05:01]

The CQ bit was checked by achuith@chromium.org

[commit-bot: I haz the power, 2015-04-28 22:06:24]

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

[commit-bot: I haz the power, 2015-04-28 22:13:05]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-04-28 22:13:55]

Patchset 6 (id:??) landed as
https://crrev.com/174b27ddc1d2ef0600219bd0d2b9595bde975495
Cr-Commit-Position: refs/heads/master@{#327375}

[Mattias Nissler (ping if slow), 2015-04-29 07:10:52]

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h (right):

https://codereview.chromium.org/1108983002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h:58: void
StartWithRefreshToken(const std::string& oauth2_refresh_token);
On 2015/04/28 18:20:08, achuithb wrote:
> On 2015/04/28 06:50:43, Mattias Nissler wrote:
> > Please turn this into a 3rd constructor for consistency.
> 
> I considered this. The first problem is that the type of auth_code and
> refresh_token is the same (const std::string&), so the ctor signatures would
be
> the same.

That's a good point.

> The bigger issue is that multiple ctors are just a variation of
> function overloading which is not recommended stylistically. 

I don't agree with this. The style guide allows overloads where they make sense.
Having different constructors that initialize the class differently seems
totally fine here. Having a single Start() function also makes it clearer what
the calling code is supposed to do, whereas multiple StartWithXYZ functions may
create the misunderstanding that they could / should be called in order.

> 
> I think the way to go is to have one ctor, and have methods
> StartWithBrowserContext, StartWithAuthCode, and StartWithRefreshToken. I'll do
> this in a follow-up cleanup CL to avoid clutter in this CL. 

Please do. Happy to review the CL.