Add ManagedUserTokenFetcher to fetch scoped-down tokens.

Add ManagedUserTokenFetcher to fetch scoped-down tokens.

TBR=rogerta@chromium.org
BUG=228833
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203015

[Bernhard Bauer, 2013-05-27 11:48:02]

Please review.

[Andrew T Wilson (Slow), 2013-05-27 13:52:15]

I'll be able to do a better job of reviewing this if I understand exactly what
ManagedUserTokenFetcher is supposed to do.

What token is it supposed to fetch? If the goal is just to return an oauth2
token scoped to chromesync_playpen, then I'm not sure why a caller would not
just use OAuth2TokenService directly, since that's what that class is for.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:37:
"https://www.googleapis.com/auth/chromesync_playpen";
You may want to move this into GaiaUrls - at the very least, it's useful to have
some way for QA to test this vs gaiastaging, and if you move this into GaiaUrls
then QA can override www.googleapis.com via a command-line switch.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:78: //
GaiaOAuthClient::Delegate implementation:
Incorrect indent on this comment.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:88: void
StartInternal();
Maybe a better name for this is worthwhile? What precisely does it do that
Start() does not?

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:130:
OAuth2TokenService::ScopeSet(), this);
What is this doing? You aren't passing any scopes in, so what are you expecting
the OAuth2TokenService to give you in this case?

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:162:
url_fetcher_->SetUploadData("application/x-www-form-urlencoded", body);
This is the piece I don't understand. You are calling OAuth2TokenFetcher to mint
tokens, then you are trying to use those tokens to mint tokens again using
GaiaOAuthClient? I think I'm missing something.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:259:
callback_.Run(error, std::string());
Does it make sense to refactor all these callback_.Run() + callback_.Reset()
calls into a single routine?

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.h (right):

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:22: class
ManagedUserTokenFetcher {
What does this class do? It's not clear to me what token is being returned? I
see that your implementation class is using GaiaOAuthClient, which is weird
since that's an OAuth1 class and OAuth2TokenService should already be giving you
an OAuth2 token to talk to whomever you wish.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (left):

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:153: const GoogleServiceAuthError&
error,
I'm OK with making this change, but it seems out of place in this CL - I guess
the motivation is that this static method is not necessary?

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

https://codereview.chromium.org/15977002/diff/26001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:206: (num_retries_ >=
source->GetMaxRetriesOn5xx())) {
Do we have a test that exercises this change?

[Bernhard Bauer, 2013-05-27 14:42:13]

On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> I'll be able to do a better job of reviewing this if I understand exactly what
> ManagedUserTokenFetcher is supposed to do.
> 
> What token is it supposed to fetch? If the goal is just to return an oauth2
> token scoped to chromesync_playpen, then I'm not sure why a caller would not
> just use OAuth2TokenService directly, since that's what that class is for.

Sorry, I should have been more detailed. 

The class fetches a downscoped refresh token that is tied to a particular
managed user profile. I added a comment explaining that and how the class
fetches it to the header.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:37:
"https://www.googleapis.com/auth/chromesync_playpen";
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> You may want to move this into GaiaUrls - at the very least, it's useful to
have
> some way for QA to test this vs gaiastaging, and if you move this into
GaiaUrls
> then QA can override http://www.googleapis.com via a command-line switch.

This is a scope, not an actual URL that we resolve, so it's constant.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:78: //
GaiaOAuthClient::Delegate implementation:
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> Incorrect indent on this comment.

Done.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:88: void
StartInternal();
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> Maybe a better name for this is worthwhile? What precisely does it do that
> Start() does not?

Renamed to StartFetching() and added a comment.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:130:
OAuth2TokenService::ScopeSet(), this);
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> What is this doing? You aren't passing any scopes in, so what are you
expecting
> the OAuth2TokenService to give you in this case?

See OAuth2AccessTokenFetcher::Start(): If the set of scopes is empty, the access
token will have the same scope as the refresh token. I added a comment to that
effect.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:162:
url_fetcher_->SetUploadData("application/x-www-form-urlencoded", body);
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> This is the piece I don't understand. You are calling OAuth2TokenFetcher to
mint
> tokens, then you are trying to use those tokens to mint tokens again using
> GaiaOAuthClient? I think I'm missing something.

Yes, the first time is for a regular access token for the custodian, the second
time fetches the downscoped token for the managed user.

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:259:
callback_.Run(error, std::string());
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> Does it make sense to refactor all these callback_.Run() + callback_.Reset()
> calls into a single routine?

Honestly, I don't know. I have three cases where I call the callback: 

* no error, received a token
* error with GoogleServiceAuthError::State, no token
* error with network error, no token

I could factor out a method that takes a GoogleServiceAuthError and a string,
but that would only save me a call to .Reset, and in exchange I'd need to
construct the GoogleServiceAuthError myself. You think it's worth doing?

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.h (right):

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:22: class
ManagedUserTokenFetcher {
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> What does this class do? It's not clear to me what token is being returned?

Added a comment.

> I
> see that your implementation class is using GaiaOAuthClient, which is weird
> since that's an OAuth1 class and OAuth2TokenService should already be giving
you
> an OAuth2 token to talk to whomever you wish.

Is it? I'm using the GetTokensFromAuthCode() method, and in particular with the
oauth2_token_url, so it should send an OAuth2-compatible HTTP request to the
right endpoint, no?

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/signin/oau...
File chrome/browser/signin/oauth2_token_service.h (left):

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.h:153: const GoogleServiceAuthError&
error,
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> I'm OK with making this change, but it seems out of place in this CL - I guess
> the motivation is that this static method is not necessary?

Yes, and I was touching the .cc file anyway. I can leave it out if you want, but
it seemed like a nice small cleanup.

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

https://codereview.chromium.org/15977002/diff/26001/google_apis/gaia/gaia_oau...
google_apis/gaia/gaia_oauth_client.cc:206: (num_retries_ >=
source->GetMaxRetriesOn5xx())) {
On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> Do we have a test that exercises this change?

device_cloud_policy_manager_chromeos_unittest.cc (which used to work around the
off-by-one error here), and also
ManagedUserTokenFetcherTest.FetchRefreshTokenTransientNetworkError, which tests
that the number of retries is correct.

[Andrew T Wilson (Slow), 2013-05-27 16:41:24]

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:130:
OAuth2TokenService::ScopeSet(), this);
On 2013/05/27 14:42:13, Bernhard Bauer wrote:
> On 2013/05/27 13:52:15, Andrew T Wilson wrote:
> > What is this doing? You aren't passing any scopes in, so what are you
> expecting
> > the OAuth2TokenService to give you in this case?
> 
> See OAuth2AccessTokenFetcher::Start(): If the set of scopes is empty, the
access
> token will have the same scope as the refresh token. I added a comment to that
> effect.

Yes, but OAuth2TokenService doesn't advertise that it does this - you're relying
on specific implementations of this class. I actually think that
OAuth2TokenService should probably add a DCHECK() to prevent calling it with an
empty scope, since the entire point of the class is to manage *scoped* tokens,
not pass back unscoped tokens.

I guess I still don't understand why you aren't just passing in
kChromeSyncManagedScope in to OAuth2TokenService and just returning the token it
provides (don't understand why you need to use GaiaAuthFetcher at all, since
OAuth2TokenService will return you a downscoped token). Come grab me tomorrow
morning and you can explain it to me because I think I'm just being dense.

[Andrew T Wilson (Slow), 2013-05-28 13:10:08]

Thanks for clarifying the design in our offline discussion.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:42:
"&response_type=code"
I wonder if it makes sense to extend OAuth2MintTokenFlow to support the extra
parameters you need instead of building your own parallel implementation here?
What do you think?

You mentioned that it makes testing a little trickier, but it sucks to basically
duplicate the same code just because we want to pass in profile_id/profile_name.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:52: class
ManagedUserTokenFetcherImpl : public ManagedUserTokenFetcher,
BTW, I like this pattern of keeping the Impl class hidden. Makes it so much
easier to read the public interface. People don't use this pattern enough -
thanks for doing this.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:127: StartFetching();
So, let's imagine that we have a really slow internet connection - this could
take a long time to mint this token. Is there UI displayed to the user? Can the
user cancel it? It seems like there's no way for a caller to cancel an
in-progress request (and, in fact, if there's an existing request already
outstanding, anyone invoking Start() will hit a DCHECK.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:131: // An empty set
of scopes means the same scope as the refresh token.
We should document this in OAuth2TokenService (that "no scopes" == "same scope
as the refresh token") if we're going to rely on this behavior.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:146: int id = 1;
nit: const int id

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:190: StartFetching();
If there's some bug that causes the server to permanently return 401s, this will
loop forever and hammer the server. Do we want to put a limit on the number of
retries?

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:254:
callback_.Run(GoogleServiceAuthError::FromConnectionError(error_code),
Maybe change this to:

{
 
DispatchGoogleServiceAuthError(GoogleServiceAuthError::FromConnectionError(error_code))
}

?

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.h (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:25: //   1. Get an
access token from OAuth2TokenService (either cached or fetched)
"Get an access token for the custodian from OAuth2TokenService..."

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:26: //   2. Call the
IssueToken API to request the downscoped token; the response
"Call the IssueToken API to mint a scoped authorization code for a managed user
from the custodian's access token."

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:28: //   3. Exchange
the authentication code for a refresh token and an access
"Exchange the authentication code for a refresh token for the managed user and
return it to the caller. The refresh token can only be used to mint
sync_playpen-scoped tokens"

(I think this is correct - we don't care about the access token, right? Looks
like we just drop it on the floor)

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:30: class
ManagedUserTokenFetcher {
Might want to change the name to make it clear that this code mints a new
*refresh* token. I took the name "ManagedUserTokenFetcher" to mean "fetches an
access token", which befuddled me since we already have lots of classes that do
that :).

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:33: const std::string&
/* token */)> TokenCallback;
token->refresh_token

[Bernhard Bauer, 2013-05-29 11:41:45]

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:42:
"&response_type=code"
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> I wonder if it makes sense to extend OAuth2MintTokenFlow to support the extra
> parameters you need instead of building your own parallel implementation here?
> What do you think?
> 
> You mentioned that it makes testing a little trickier, but it sucks to
basically
> duplicate the same code just because we want to pass in
profile_id/profile_name.

Well, there are also other problems:
* OAuth2MintTokenFlow uses OAuth2AccessTokenFetcher instead of
OAuth2TokenService, so it doesn't cache the access token
* It fetches an access token, not an authentication code for a refresh token.
* It takes the (custodian's) refresh token, and you said we don't want to expose
that.
* It also takes an extension ID, which we don't have.

If I went ahead and fixed that, I don't think there'd be that much duplication
left.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:127: StartFetching();
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> So, let's imagine that we have a really slow internet connection - this could
> take a long time to mint this token. Is there UI displayed to the user? Can
the
> user cancel it?

Yes, at least on desktop. I'll ask about Chrome OS.

> It seems like there's no way for a caller to cancel an
> in-progress request (and, in fact, if there's an existing request already
> outstanding, anyone invoking Start() will hit a DCHECK.

Well, in this class the request can be canceled by destroying it. There are some
tests for that.

Now, the way this class is currently used, it is owned by the
ManagedUserRegistrationService, which is a ProfileKeyedService, so it will only
get canceled when the user logs out (on Chrome OS) or Chrome shuts down (on
Desktop). I want to separate the SyncableService from the registration part
anyway though, which would mean that the registration part could be owned by its
client again.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:131: // An empty set
of scopes means the same scope as the refresh token.
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> We should document this in OAuth2TokenService (that "no scopes" == "same scope
> as the refresh token") if we're going to rely on this behavior.

Ok. Alternatively, I could pass in the login scope explicitly?

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:146: int id = 1;
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> nit: const int id

Done.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:190: StartFetching();
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> If there's some bug that causes the server to permanently return 401s, this
will
> loop forever and hammer the server. Do we want to put a limit on the number of
> retries?

Good idea! Done.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:254:
callback_.Run(GoogleServiceAuthError::FromConnectionError(error_code),
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> Maybe change this to:
> 
> {
>  
>
DispatchGoogleServiceAuthError(GoogleServiceAuthError::FromConnectionError(error_code))
> }
> 
> ?

Do you mean adding an overload that takes a GoogleServiceAuthError (as opposed
to a GoogleServiceAuthError::State)? Or changing the signature?

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.h (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:25: //   1. Get an
access token from OAuth2TokenService (either cached or fetched)
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> "Get an access token for the custodian from OAuth2TokenService..."

Done.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:26: //   2. Call the
IssueToken API to request the downscoped token; the response
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> "Call the IssueToken API to mint a scoped authorization code for a managed
user
> from the custodian's access token."

Done, with the additional explanation that the authorization code is for a
refresh token.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:28: //   3. Exchange
the authentication code for a refresh token and an access
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> "Exchange the authentication code for a refresh token for the managed user and
> return it to the caller. The refresh token can only be used to mint
> sync_playpen-scoped tokens"
> 
> (I think this is correct - we don't care about the access token, right? Looks
> like we just drop it on the floor)

Done. I didn't use the term "playpen" though, because that's not used in the
client code (except for the value of the scope itself, which is a constant
defined server-side).

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:30: class
ManagedUserTokenFetcher {
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> Might want to change the name to make it clear that this code mints a new
> *refresh* token. I took the name "ManagedUserTokenFetcher" to mean "fetches an
> access token", which befuddled me since we already have lots of classes that
do
> that :).

OK, done.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.h:33: const std::string&
/* token */)> TokenCallback;
On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> token->refresh_token

Done.

[Andrew T Wilson (Slow), 2013-05-29 12:05:33]

Getting very close now - some nits below and a few questions.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:127: StartFetching();
On 2013/05/29 11:41:45, Bernhard Bauer wrote:
> On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> > So, let's imagine that we have a really slow internet connection - this
could
> > take a long time to mint this token. Is there UI displayed to the user? Can
> the
> > user cancel it?
> 
> Yes, at least on desktop. I'll ask about Chrome OS.
> 
> > It seems like there's no way for a caller to cancel an
> > in-progress request (and, in fact, if there's an existing request already
> > outstanding, anyone invoking Start() will hit a DCHECK.
> 
> Well, in this class the request can be canceled by destroying it. There are
some
> tests for that.
> 
> Now, the way this class is currently used, it is owned by the
> ManagedUserRegistrationService, which is a ProfileKeyedService, so it will
only
> get canceled when the user logs out (on Chrome OS) or Chrome shuts down (on
> Desktop). I want to separate the SyncableService from the registration part
> anyway though, which would mean that the registration part could be owned by
its
> client again.

I guess I don't understand the flow, then.

1) User performs some kind of "create managed user" operation in the UI. They
get some kind of progress dialog.
2) Code invokes ManagedUserTokenFetcher::Start()
3) User cancels out of the progress dialog.

Now what? There's an operation in progress in ManagedUserTokenFetcher - how do
we cancel it? In theory, the caller could free the ManagedUserTokenFetcher, but
since it's passed in to ManagedUserService there's no way for ManagedUserService
to free it and obtain a new one.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:131: // An empty set
of scopes means the same scope as the refresh token.
On 2013/05/29 11:41:45, Bernhard Bauer wrote:
> On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> > We should document this in OAuth2TokenService (that "no scopes" == "same
scope
> > as the refresh token") if we're going to rely on this behavior.
> 
> Ok. Alternatively, I could pass in the login scope explicitly?
Passing in an explicit scope would be ideal.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:232:
GoogleServiceAuthError error(GoogleServiceAuthError::NONE);
Call DispatchGoogleServiceAuthError(GoogleServiceAuthError::NONE) here instead?
The idea is that the callback would be invoked/cleared from exactly one place.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:249: LOG(WARNING) <<
"HTTP error " << response_code;
I'm always leery of adding LOG() statements since they bloat the release client.
Maybe this should be a DLOG()?

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:254:
callback_.Run(GoogleServiceAuthError::FromConnectionError(error_code),
On 2013/05/29 11:41:45, Bernhard Bauer wrote:
> Do you mean adding an overload that takes a GoogleServiceAuthError (as opposed
> to a GoogleServiceAuthError::State)? Or changing the signature?

I mean that instead of calling callback_.Run() and callback_.Reset() here, you
might as well call DispatchGoogleServiceAuthError(). That way all the error
handling code that invokes a callback goes through a single routine which makes
it easier to maintain.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc:177:
callback_.Run(error, std::string());
DispatchGoogleServiceAuthError(error.state())?

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc:237:
callback_.Run(error, refresh_token);
Could also use GoogleServiceAuthError::AuthErrorNone() and omit the error
variable entirely. This is fine, though.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc:264:
GoogleServiceAuthError::State state) {
Consider taking a const GoogleServiceAuthError& here instead of State, to
simplify calling from DispatchNetworkError, OnGetTokenFailure(). It's fine as-is
though.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h (right):

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:25: //   1. Get
an access token for the custodian from OAuth2TokenService
very minor nit: this is a sentence, so should use sentence punctuation (have a
period at the end)

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:28: //     
refresh token managed user from the custodian's access token.
"refresh token for the managed user"

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:35: const
std::string& /* refersh_token */)>
nit: refersh->refresh

[Bernhard Bauer, 2013-05-29 12:56:55]

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:127: StartFetching();
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> On 2013/05/29 11:41:45, Bernhard Bauer wrote:
> > On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> > > So, let's imagine that we have a really slow internet connection - this
> could
> > > take a long time to mint this token. Is there UI displayed to the user?
Can
> > the
> > > user cancel it?
> > 
> > Yes, at least on desktop. I'll ask about Chrome OS.
> > 
> > > It seems like there's no way for a caller to cancel an
> > > in-progress request (and, in fact, if there's an existing request already
> > > outstanding, anyone invoking Start() will hit a DCHECK.
> > 
> > Well, in this class the request can be canceled by destroying it. There are
> some
> > tests for that.
> > 
> > Now, the way this class is currently used, it is owned by the
> > ManagedUserRegistrationService, which is a ProfileKeyedService, so it will
> only
> > get canceled when the user logs out (on Chrome OS) or Chrome shuts down (on
> > Desktop). I want to separate the SyncableService from the registration part
> > anyway though, which would mean that the registration part could be owned by
> its
> > client again.
> 
> I guess I don't understand the flow, then.
> 
> 1) User performs some kind of "create managed user" operation in the UI. They
> get some kind of progress dialog.
> 2) Code invokes ManagedUserTokenFetcher::Start()
> 3) User cancels out of the progress dialog.
> 
> Now what? There's an operation in progress in ManagedUserTokenFetcher - how do
> we cancel it? In theory, the caller could free the ManagedUserTokenFetcher,
but
> since it's passed in to ManagedUserService there's no way for
ManagedUserService
> to free it and obtain a new one.

Yes. I'm planning on adding that as soon as Pam has implemented the new managed
user creation flow on desktop, but I don't want to change the interface she's
using while her CL is in flight.

If you want to, I can add a CancelRegistration() method to ManagedUserService,
but it's going to change soon anyway.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:131: // An empty set
of scopes means the same scope as the refresh token.
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> On 2013/05/29 11:41:45, Bernhard Bauer wrote:
> > On 2013/05/28 13:10:08, Andrew T Wilson wrote:
> > > We should document this in OAuth2TokenService (that "no scopes" == "same
> scope
> > > as the refresh token") if we're going to rely on this behavior.
> > 
> > Ok. Alternatively, I could pass in the login scope explicitly?
> Passing in an explicit scope would be ideal.

Done.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:232:
GoogleServiceAuthError error(GoogleServiceAuthError::NONE);
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> Call DispatchGoogleServiceAuthError(GoogleServiceAuthError::NONE) here
instead?
> The idea is that the callback would be invoked/cleared from exactly one place.

Done.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:249: LOG(WARNING) <<
"HTTP error " << response_code;
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> I'm always leery of adding LOG() statements since they bloat the release
client.
> Maybe this should be a DLOG()?

Done. It does mean that you can only see the HTTP response code in a Debug build
however if something goes wrong.

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:254:
callback_.Run(GoogleServiceAuthError::FromConnectionError(error_code),
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> On 2013/05/29 11:41:45, Bernhard Bauer wrote:
> > Do you mean adding an overload that takes a GoogleServiceAuthError (as
opposed
> > to a GoogleServiceAuthError::State)? Or changing the signature?
> 
> I mean that instead of calling callback_.Run() and callback_.Reset() here, you
> might as well call DispatchGoogleServiceAuthError(). That way all the error
> handling code that invokes a callback goes through a single routine which
makes
> it easier to maintain.

Done.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc:177:
callback_.Run(error, std::string());
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> DispatchGoogleServiceAuthError(error.state())?

Done.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc:237:
callback_.Run(error, refresh_token);
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> Could also use GoogleServiceAuthError::AuthErrorNone() and omit the error
> variable entirely. This is fine, though.

Done.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.cc:264:
GoogleServiceAuthError::State state) {
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> Consider taking a const GoogleServiceAuthError& here instead of State, to
> simplify calling from DispatchNetworkError, OnGetTokenFailure(). It's fine
as-is
> though.

Yes, that's what I ended up doing.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h (right):

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:25: //   1. Get
an access token for the custodian from OAuth2TokenService
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> very minor nit: this is a sentence, so should use sentence punctuation (have a
> period at the end)

Done.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:28: //     
refresh token managed user from the custodian's access token.
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> "refresh token for the managed user"

Urr, yes. Done.

https://codereview.chromium.org/15977002/diff/41001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_refresh_token_fetcher.h:35: const
std::string& /* refersh_token */)>
On 2013/05/29 12:05:33, Andrew T Wilson wrote:
> nit: refersh->refresh

Argh, done.

[Andrew T Wilson (Slow), 2013-05-29 13:12:17]

LGTM

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:127: StartFetching();
On 2013/05/29 12:56:55, Bernhard Bauer wrote:
> If you want to, I can add a CancelRegistration() method to ManagedUserService,
> but it's going to change soon anyway.
Adding a TODO in the header file is sufficient for now.

[Bernhard Bauer, 2013-05-29 14:11:15]

TBR'ing Roger for GaiaOAuthClient (fixing an off-by-one error).

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_token_fetcher.cc (right):

https://codereview.chromium.org/15977002/diff/26002/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_token_fetcher.cc:127: StartFetching();
On 2013/05/29 13:12:17, Andrew T Wilson wrote:
> On 2013/05/29 12:56:55, Bernhard Bauer wrote:
> > If you want to, I can add a CancelRegistration() method to
ManagedUserService,
> > but it's going to change soon anyway.
> Adding a TODO in the header file is sufficient for now.

Done.

[commit-bot: I haz the power, 2013-05-29 14:11:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/15977002/50001

[commit-bot: I haz the power, 2013-05-29 15:42:10]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Bernhard Bauer, 2013-05-29 16:22:11]

(Actually adding rogerta)

Roger: TBR for GaiaOAuthClient (fixing an off-by-one error).

[commit-bot: I haz the power, 2013-05-29 16:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/15977002/56001

[commit-bot: I haz the power, 2013-05-29 16:52:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Roger Tawa OOO till Jul 10th, 2013-05-29 18:14:39]

lgtm fir browser/signin

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

https://codereview.chromium.org/15977002/diff/56001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:431: if (token_iterator !=
token_cache_.end() &&
Seems like there are is unit test for InvalidateToken().  Can you add a test
before committing?

[Bernhard Bauer, 2013-05-29 19:16:30]

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

https://codereview.chromium.org/15977002/diff/56001/chrome/browser/signin/oau...
chrome/browser/signin/oauth2_token_service.cc:431: if (token_iterator !=
token_cache_.end() &&
On 2013/05/29 18:14:40, Roger Tawa wrote:
> Seems like there are is unit test for InvalidateToken().  Can you add a test
> before committing?

Done.

[commit-bot: I haz the power, 2013-05-29 19:16:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/15977002/73001

[commit-bot: I haz the power, 2013-05-30 00:10:57]

Change committed as 203015