Device robot refresh token integrity validation.

Device robot refresh token integrity validation.

Before using the robot account refresh token stored in an enterprise
device's Local State, verify that the token is owned by the service
account id found in the device policy.

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

[Mattias Nissler (ping if slow), 2013-06-17 05:34:17]

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:162: if
(policy) {
nit: no need for curlies, I guess you could switch to a ternary entirely.

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:123: LOG(INFO)
<< "Device service account owner in policy is empty.";
I guess this should be at least WARN, if not ERROR.

https://codereview.chromium.org/17109006/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.h:137: virtual bool
StartRefreshTokenValidation(
I wonder whether making the concept of refresh token validation visible on the
OAuth2TokenService level is actually a good idea. Have you thought about just
making GetRefreshToken asynchronous, i.e.:

void GetRefreshToken(base::Callback<void(const std::string&)>);

Then, you could hide away the validation part in DeviceOAuth2TokenService.

https://codereview.chromium.org/17109006/diff/1/google_apis/gaia/gaia_oauth_c...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/17109006/diff/1/google_apis/gaia/gaia_oauth_c...
google_apis/gaia/gaia_oauth_client.cc:131: void
GaiaOAuthClient::Core::GetTokenInfo(const std::string& oauth_access_token,
Hm, shouldn't this logically be closer to GetUserInfo() than MakeGaiaRequest()?
You seem to follow the latter pattern.

https://codereview.chromium.org/17109006/diff/1/google_apis/gaia/gaia_oauth_c...
google_apis/gaia/gaia_oauth_client.cc:289: // TODO: remove passed-in gaia_url?
TODOs without owners/bugs are not worth much...

[David Roche, 2013-06-18 04:12:08]

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:162: if
(policy) {
On 2013/06/17 05:34:17, Mattias Nissler wrote:
> nit: no need for curlies, I guess you could switch to a ternary entirely.

Done.

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:123: LOG(INFO)
<< "Device service account owner in policy is empty.";
On 2013/06/17 05:34:17, Mattias Nissler wrote:
> I guess this should be at least WARN, if not ERROR.

Done.

https://codereview.chromium.org/17109006/diff/1/chrome/browser/signin/oauth2_...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/1/chrome/browser/signin/oauth2_...
chrome/browser/signin/oauth2_token_service.h:137: virtual bool
StartRefreshTokenValidation(
On 2013/06/17 05:34:17, Mattias Nissler wrote:
> I wonder whether making the concept of refresh token validation visible on the
> OAuth2TokenService level is actually a good idea. Have you thought about just
> making GetRefreshToken asynchronous, i.e.:
> 
> void GetRefreshToken(base::Callback<void(const std::string&)>);
> 
> Then, you could hide away the validation part in DeviceOAuth2TokenService.

I started doing that initially, but unfortunately it got to be messy.  The
problem is that the refresh token getter is on the token-service singleton,
while the async/concurrent fetch requests are on the inner Fetcher class.  So,
to stay with the current design of OAuth2TokenService, the fetching should stay
in the Fetcher, since it automatically takes care of handling concurrent
fetches, etc.  Then the obvious idea is to continue with the attempt to make
GetRefreshToken async, but call it from the Fetcher.  This was my attempt #2,
but it had issues b/c the OAuth2TokenService keeps the map of in-flight Fetchers
indexed by refresh_token/scopes pair.  So, I need the refresh token value before
the Fetcher's CreateAndStart method is called.  I considered a big refactor of
the OAuth2TokenService logic around Fetcher lifecycle tracking, so that it was a
2-phase affair -- the first tracking a list of Fetchers that were getting the
refresh token (indexed only by scope) and the 2nd phase of Fetchers that had a
refresh token and were fetching the access token (indexed by refresh token /
scope pair).  But, in the end that seemed like a lot of changes.  So, what you
see is attempt #3 ... keeping the change as simple as possible by having the
fetcher created with the refresh token initially (so it can be added to the
existing map), and then optionally validating the token in the Fetcher. 
Conceptually, validating the refresh token is something that other subclasses
could want to do in the future, so it seemed reasonable enough.  Although, I
agree with your initial design instinct ... it would have been nice to hide the
validation in DeviceOAuth2TokenService, if it could have been done relatively
easily.

https://codereview.chromium.org/17109006/diff/1/google_apis/gaia/gaia_oauth_c...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/17109006/diff/1/google_apis/gaia/gaia_oauth_c...
google_apis/gaia/gaia_oauth_client.cc:131: void
GaiaOAuthClient::Core::GetTokenInfo(const std::string& oauth_access_token,
On 2013/06/17 05:34:17, Mattias Nissler wrote:
> Hm, shouldn't this logically be closer to GetUserInfo() than
MakeGaiaRequest()?
> You seem to follow the latter pattern.

I'm not sure I follow.  Are you saying to keep the order of the methods
GetUserInfo
GetTokenInfo
... here as it is above and below?

I swapped the 2 *Info methods here due to the line 180 comment in
MakeGaiaRequest referencing the comment "above", but I suppose the comment is
still valid even if I separate the 2 comments a bit more with an extra method.

https://codereview.chromium.org/17109006/diff/1/google_apis/gaia/gaia_oauth_c...
google_apis/gaia/gaia_oauth_client.cc:289: // TODO: remove passed-in gaia_url?
On 2013/06/17 05:34:17, Mattias Nissler wrote:
> TODOs without owners/bugs are not worth much...

Oops, left that as a note to be cleaned up before I sent the review.  I went
ahead and made the change.  It doesn't make sense to pass in the URL when 1/2
the methods ignore it and use another flag value.  All the clients of this class
passed in the same flag value anyway...

[David Roche, 2013-06-19 03:35:57]

Mattias, I've added unit tests for everything I can think of.  PTAL.

[Mattias Nissler (ping if slow), 2013-06-19 17:53:17]

Couple more comments, the policy stuff LGTM at this point. I'm still a bit
uncomfortable about having token validation in the base class, but I'm not the
gatekeeper on this.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:109: if
(connector) {
nit: no curlies

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:127: } else {
should we clear the local state pref here for consistency?

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:14: #include
"base/values.h"
forward-declare base::DictionaryValue?

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:67: friend class
TestDeviceOAuth2TokenService;
Is this friend decl really needed?

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:39: }
nit: newline

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:44: }
nit: newline

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:69:
delete oauth2_service_;
wrap in a scoped_ptr_, or make a plain class member.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:188: const std::string&
refresh_token,
nit: indentation

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:473: const std::string
refresh_token,
nit: indentation

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:105: virtual void
OnRefreshTokenValidationComplete(
this should just be a base::Callback

https://codereview.chromium.org/17109006/diff/14001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client_unittest.cc (right):

https://codereview.chromium.org/17109006/diff/14001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client_unittest.cc:66: if (complete_immediately_) {
nit: no curlies needed

[David Roche, 2013-06-20 02:07:25]

Mattias, I moved the validation logic out of the OAuth2TokenService API, as
discussed.  I think this is cleaner, thanks for the idea.  PTAL.  (I didn't have
a chance to go thru your other minor review comments in detail, I'll look at
addressing those tomorrow.)

Andrew, can you take a look at the signin/* files?  There's a few tweaks to
OAuthTokenService for minor cleanup and testability.

[Andrew T Wilson (Slow), 2013-06-20 17:47:27]

signin/* lgtm with a few suggestions

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:200:
InformWaitingRequestsAndQuit();
"AndQuit" -> "AndDelete" since "Quit" doesn't mean anything.

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:460:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
See my previous comment - would rather see this injected in the constructors.

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:99: void
set_max_authorization_token_fetch_retries_for_testing(int max_retries);
can we inject this in the constructor instead and pass that along to the
Fetcher?

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:104: class RequestImpl : public
base::SupportsWeakPtr<RequestImpl>,
This is fine, but I wonder now if it's time to move this into a separate header
file rather than in this public file.

[David Roche, 2013-06-20 17:49:29]

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:109: if
(connector) {
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:127: } else {
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> should we clear the local state pref here for consistency?

I figured we'd just leave it since this class refuses to use it.  Then it's
there for analysis, and also if there's a hiccup in fetching the owner from
tokeninfo, we don't nuke the refresh token on all devices.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:14: #include
"base/values.h"
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> forward-declare base::DictionaryValue?

Moved to .cc file with new ValidatingConsumer.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:67: friend class
TestDeviceOAuth2TokenService;
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> Is this friend decl really needed?

Yes, since the constructor/destructor is private here.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc
(right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:39: }
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> nit: newline

Done.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:44: }
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> nit: newline

Done.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:69:
delete oauth2_service_;
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> wrap in a scoped_ptr_, or make a plain class member.

Can't use scoped_ptr, since the constructor/destructor is private.  This test is
a friend, so it can construct/delete, but the scoped_ptr can't.

Made plain class member.  I always forget that there's a new instance of the
test class per test method execution.  It always seems like if there needs to be
a new instance of the class-under-test that it should be in the SetUp method,
but I guess not.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:188: const std::string&
refresh_token,
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> nit: indentation

Removed.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:473: const std::string
refresh_token,
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> nit: indentation

Removed.

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/14001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:105: virtual void
OnRefreshTokenValidationComplete(
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> this should just be a base::Callback

Removed completely.

https://codereview.chromium.org/17109006/diff/14001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client_unittest.cc (right):

https://codereview.chromium.org/17109006/diff/14001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client_unittest.cc:66: if (complete_immediately_) {
On 2013/06/19 17:53:17, Mattias Nissler wrote:
> nit: no curlies needed

Done.

[David Roche, 2013-06-20 18:17:17]

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.cc (right):

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:200:
InformWaitingRequestsAndQuit();
On 2013/06/20 17:47:28, Andrew T Wilson wrote:
> "AndQuit" -> "AndDelete" since "Quit" doesn't mean anything.

Done.

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (right):

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:99: void
set_max_authorization_token_fetch_retries_for_testing(int max_retries);
On 2013/06/20 17:47:28, Andrew T Wilson wrote:
> can we inject this in the constructor instead and pass that along to the
> Fetcher?

Passing this to the constructor would require plumbing it through to all the
subclass implementations and the factories that create them:

DeviceOAuth2TokenService
DeviceOAuth2TokenServiceFactory
ProfileOAuth2TokenService
ProfileOAuth2TokenServiceFactory
AndroidProfileOAuth2TokenService

Since this was only for test purposes, I was inclined to keep this as a simple
*_for_testing method rather that impacting all those classes.  WDYT?

https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:104: class RequestImpl : public
base::SupportsWeakPtr<RequestImpl>,
On 2013/06/20 17:47:28, Andrew T Wilson wrote:
> This is fine, but I wonder now if it's time to move this into a separate
header
> file rather than in this public file.

Ideally this would just be hidden, since it's an impl detail of this class.  I
could do that now, but I need to work cancellable requests into
DeviceOAuth2TokenService, which may call for this to be exposed to subclasses. 
I'll take a note to resolve this one way or the other when I'm making that
change.

[Andrew T Wilson (Slow), 2013-06-20 18:36:57]

On 2013/06/20 18:17:17, David Roche wrote:
>
https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
> File chrome/browser/signin/oauth2_token_service.cc (right):
> 
>
https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
> chrome/browser/signin/oauth2_token_service.cc:200:
> InformWaitingRequestsAndQuit();
> On 2013/06/20 17:47:28, Andrew T Wilson wrote:
> > "AndQuit" -> "AndDelete" since "Quit" doesn't mean anything.
> 
> Done.
> 
>
https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
> File chrome/browser/signin/oauth2_token_service.h (right):
> 
>
https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
> chrome/browser/signin/oauth2_token_service.h:99: void
> set_max_authorization_token_fetch_retries_for_testing(int max_retries);
> On 2013/06/20 17:47:28, Andrew T Wilson wrote:
> > can we inject this in the constructor instead and pass that along to the
> > Fetcher?
> 
> Passing this to the constructor would require plumbing it through to all the
> subclass implementations and the factories that create them:
> 
> DeviceOAuth2TokenService
> DeviceOAuth2TokenServiceFactory
> ProfileOAuth2TokenService
> ProfileOAuth2TokenServiceFactory
> AndroidProfileOAuth2TokenService
> 
> Since this was only for test purposes, I was inclined to keep this as a simple
> *_for_testing method rather that impacting all those classes.  WDYT?

SGTM

> 
>
https://codereview.chromium.org/17109006/diff/25001/chrome/browser/signin/oau...
> chrome/browser/signin/oauth2_token_service.h:104: class RequestImpl : public
> base::SupportsWeakPtr<RequestImpl>,
> On 2013/06/20 17:47:28, Andrew T Wilson wrote:
> > This is fine, but I wonder now if it's time to move this into a separate
> header
> > file rather than in this public file.
> 
> Ideally this would just be hidden, since it's an impl detail of this class.  I
> could do that now, but I need to work cancellable requests into
> DeviceOAuth2TokenService, which may call for this to be exposed to subclasses.

> I'll take a note to resolve this one way or the other when I'm making that
> change.

Sure, a TODO is fine here.

[David Roche, 2013-06-20 18:47:35]

pam@: can you review the small tweaks to chrome/browser/managed_mode/ due to
GaiaOAuthClient API changes?

vitalybuka@: can you review the cloudprint tweaks due to GaiaOAuthClient API
changes?

zelidrag@: can you review the addition of GaiaOAuthClient::GetTokenInfo and the
other cleanup tweaks to that class?

[Mattias Nissler (ping if slow), 2013-06-20 18:58:29]

https://chromiumcodereview.appspot.com/17109006/diff/29002/chrome/browser/chr...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://chromiumcodereview.appspot.com/17109006/diff/29002/chrome/browser/chr...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:209: // object
owns.
The other problem now is that you start validations for each request (it'd be
sufficient to only start one.

[David Roche, 2013-06-20 20:13:59]

https://chromiumcodereview.appspot.com/17109006/diff/29002/chrome/browser/chr...
File chrome/browser/chromeos/settings/device_oauth2_token_service.cc (right):

https://chromiumcodereview.appspot.com/17109006/diff/29002/chrome/browser/chr...
chrome/browser/chromeos/settings/device_oauth2_token_service.cc:209: // object
owns.
On 2013/06/20 18:58:30, Mattias Nissler wrote:
> The other problem now is that you start validations for each request (it'd be
> sufficient to only start one.

Only until the validation is successful, after which no more validations are
done.  Since getting parallel requests seemed unlikely, I liked the simplicity
of just having a single ValidatingConsumer instance per in-flight request.  But,
I can look at optimizing this away if you feel strongly about it.

[Pam (message me for reviews), 2013-06-20 20:38:32]

Bernhard is the right person to review these managed_mode/ changes.

[Vitaly Buka (NO REVIEWS), 2013-06-20 20:46:45]

chrome/service/cloud_print/cloud_print_auth.cc LGTM

[Bernhard Bauer, 2013-06-20 20:47:06]

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:10: #include "base/string_util.cc"
Wut?

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:30: const int
GaiaOAuthClient::kUrlFetcherId = 1;
Yeah... except for ManagedUserRefreshTokenFetcher for example, which uses 1 in a
different place in the code (to avoid collisions!). If you're already using a
constant here (which I think is a Good Thing), why not choose a random number
between INT_MIN and INT_MAX? That makes accidental collisions far less likely :)

[David Roche, 2013-06-20 20:57:32]

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:10: #include "base/string_util.cc"
On 2013/06/20 20:47:06, Bernhard Bauer wrote:
> Wut?

Using JoinString() when building up the list of scopes for the OAuth request.

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:30: const int
GaiaOAuthClient::kUrlFetcherId = 1;
On 2013/06/20 20:47:06, Bernhard Bauer wrote:
> Yeah... except for ManagedUserRefreshTokenFetcher for example, which uses 1 in
a
> different place in the code (to avoid collisions!). If you're already using a
> constant here (which I think is a Good Thing), why not choose a random number
> between INT_MIN and INT_MAX? That makes accidental collisions far less likely
:)

Good call.  I used the code review #.

[Bernhard Bauer, 2013-06-20 21:22:17]

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/17109006/diff/29002/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:10: #include "base/string_util.cc"
On 2013/06/20 20:57:32, David Roche wrote:
> On 2013/06/20 20:47:06, Bernhard Bauer wrote:
> > Wut?
> 
> Using JoinString() when building up the list of scopes for the OAuth request.

Yes, but you're including a .cc file :)

[Bernhard Bauer, 2013-06-20 21:22:45]

LGTM once you've included the .h file instead.

[David Roche, 2013-06-21 02:19:12]

rogerta@: can you review the google_apis/gaia changes to GaiaOAuthClient?

[Roger Tawa OOO till Jul 10th, 2013-06-21 18:22:41]

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:30: const int
GaiaOAuthClient::kUrlFetcherId = 17109006;
For my own info, can you explain this further?  I don't see the unit tests doing
anything different based on fetcher ids.

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:185: request_->Start();
Seems like there is an opportunity to refactor this code and have
GaiaOAuthClient::Core::GetUserInfo() call this.  Seems the only difference
between GetUserInfo() and this method is that the former does a GET and the
latter does a POST?

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client.h (right):

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.h:41: int expires_in_seconds) {};
don't need trailing ; here and lines 44 and 46.

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.h:82: Delegate* delegate);
Can you please document the args for these three methods?  The comment at lines
64-67 explains |max_retries| but none of the method specific args are described.
 I know you didn't write all these functions, but since you're here and adding
methods/args... :-)

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client_unittest.cc (right):

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client_unittest.cc:164: } OVERRIDE;
The OVERRIDE macro should go at line 160 above.

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client_unittest.cc:192: virtual void
OnGetTokenInfoResponse(scoped_ptr<DictionaryValue> token_info) {
Need OVERRIDE macro.

[David Roche, 2013-06-21 18:40:45]

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client.cc (right):

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:30: const int
GaiaOAuthClient::kUrlFetcherId = 17109006;
On 2013/06/21 18:22:41, Roger Tawa wrote:
> For my own info, can you explain this further?  I don't see the unit tests
doing
> anything different based on fetcher ids.

This ID is used only for testing.  The DeviceOAuth2TokenService unit test needs
to address 2 parallel URLFetchers, so they can't both hard-code and ID of 0. 
Any int is fine, but another class already uses 1, so I chose the code review #
to make it unique.

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:185: request_->Start();
On 2013/06/21 18:22:41, Roger Tawa wrote:
> Seems like there is an opportunity to refactor this code and have
> GaiaOAuthClient::Core::GetUserInfo() call this.  Seems the only difference
> between GetUserInfo() and this method is that the former does a GET and the
> latter does a POST?

Yeah, I wasn't sure how to test the GetUserInfo, so I was hesitant to change it.
 I do know that GetTokenInfo didn't work with a GET, so I was hesitant to change
the GetUserInfo impl...

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client.h (right):

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.h:41: int expires_in_seconds) {};
On 2013/06/21 18:22:41, Roger Tawa wrote:
> don't need trailing ; here and lines 44 and 46.

Done.

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.h:82: Delegate* delegate);
On 2013/06/21 18:22:41, Roger Tawa wrote:
> Can you please document the args for these three methods?  The comment at
lines
> 64-67 explains |max_retries| but none of the method specific args are
described.
>  I know you didn't write all these functions, but since you're here and adding
> methods/args... :-)

Sure, I can document this.  Can I do this in a CL on Monday and send to you?  I
need to look up a few details to get the docs right, and would like to get this
CL slogging through the CQ ASAP.

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
File google_apis/gaia/gaia_oauth_client_unittest.cc (right):

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client_unittest.cc:164: } OVERRIDE;
On 2013/06/21 18:22:41, Roger Tawa wrote:
> The OVERRIDE macro should go at line 160 above.

Done.

https://codereview.chromium.org/17109006/diff/59001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client_unittest.cc:192: virtual void
OnGetTokenInfoResponse(scoped_ptr<DictionaryValue> token_info) {
On 2013/06/21 18:22:41, Roger Tawa wrote:
> Need OVERRIDE macro.

Done.

[zel, 2013-06-21 18:51:06]

lgtm

[commit-bot: I haz the power, 2013-06-21 19:51:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/78001

[Roger Tawa OOO till Jul 10th, 2013-06-21 20:00:42]

np David, so can send me a follow up CL on monday.  Thanks!

[commit-bot: I haz the power, 2013-06-21 20:16:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[David Roche, 2013-06-21 22:34:10]

wez@: can you review the remoting change?

[Wez, 2013-06-21 22:34:17]

lgtm

[commit-bot: I haz the power, 2013-06-21 22:35:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/68004

[commit-bot: I haz the power, 2013-06-21 23:13:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-22 01:17:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/70003

[commit-bot: I haz the power, 2013-06-22 01:18:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/70003

[commit-bot: I haz the power, 2013-06-22 01:20:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/70003

[commit-bot: I haz the power, 2013-06-22 01:27:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/70003

[commit-bot: I haz the power, 2013-06-22 02:35:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/70003

[commit-bot: I haz the power, 2013-06-22 02:52:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-22 05:10:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/108003

[commit-bot: I haz the power, 2013-06-22 06:36:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-22 06:46:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidroche@chromium.org/17109006/114001

[commit-bot: I haz the power, 2013-06-22 13:38:24]

Change committed as 208019