Add a mode to OAuth2MintTokenFlow that fetches the messages to show to the user.

Add a mode to OAuth2MintTokenFlow that fetches the messages to show to the user.
Change OAuth2MintTokenFlow to use OAuth2ApiCallFlow.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132242

[Munjal (Google), 2012-04-10 21:04:03]

Use the abstract API flow class I added in
https://chromiumcodereview.appspot.com/9937004/ to implement token mint API
call. Also add different modes to the token mint flow class to perform all flows
allowed by IssueToken endpoint.

[asargent_no_longer_on_chrome, 2012-04-10 22:47:25]

lgtm

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:47: new
OAuth2MintTokenFlow(
nit: this could go on the preceding line just after "reset(" 

(otherwise, the lines underneath this one would need to increase their indent
level)

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
File chrome/common/net/gaia/oauth2_mint_token_flow.cc (right):

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
chrome/common/net/gaia/oauth2_mint_token_flow.cc:212: return
static_cast<base::DictionaryValue*>(value.release());
nit: I think you can use Value::GetAsDictionary here and avoid the cast (maybe
this would simplify the code a couple lines above too).

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
File chrome/common/net/gaia/oauth2_mint_token_flow.h (right):

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
chrome/common/net/gaia/oauth2_mint_token_flow.h:73: : mode(MODE_ISSUE_ADVICE) {
nit: I think this can go alongside the line above, along with the closing brace
too

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
chrome/common/net/gaia/oauth2_mint_token_flow.h:76: const std::string& eid,
nit: these should be indented at the same level as |rt|

eg.

Parameters(const std::string& rt,
           const std::string& eid,
           ...

[Munjal (Google), 2012-04-10 23:15:35]

Thanks for the quick review.

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:47: new
OAuth2MintTokenFlow(
On 2012/04/10 22:47:25, Antony Sargent wrote:
> nit: this could go on the preceding line just after "reset(" 
> 
> (otherwise, the lines underneath this one would need to increase their indent
> level)

Done.

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
File chrome/common/net/gaia/oauth2_mint_token_flow.cc (right):

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
chrome/common/net/gaia/oauth2_mint_token_flow.cc:212: return
static_cast<base::DictionaryValue*>(value.release());
On 2012/04/10 22:47:25, Antony Sargent wrote:
> nit: I think you can use Value::GetAsDictionary here and avoid the cast (maybe
> this would simplify the code a couple lines above too).

Done.

Note that it did not simplify a lot of the code since I have to still do
value.release() conditionally, but the new thing is more readable and avoids a
cast. That is nice.

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
File chrome/common/net/gaia/oauth2_mint_token_flow.h (right):

https://chromiumcodereview.appspot.com/10012051/diff/10004/chrome/common/net/...
chrome/common/net/gaia/oauth2_mint_token_flow.h:76: const std::string& eid,
On 2012/04/10 22:47:25, Antony Sargent wrote:
> nit: these should be indented at the same level as |rt|
> 
> eg.
> 
> Parameters(const std::string& rt,
>            const std::string& eid,
>            ...
> 
> 

Done.

[jstritar, 2012-04-11 19:14:24]

This will be really useful!

What do you all think of the class's name now? How about using the noun form of
Mint?

OAuth2TokenMint
OAuth2ScopedTokenMint
OAuth2AccessTokenMint
AccessTokenMint
ScopedTokenMint
AccessTokenGenerator
ScopedTokenGenerator
AccessTokenFetchor

... or some other combination? Not sure..

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
File chrome/common/net/gaia/oauth2_mint_token_flow.cc (right):

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.cc:207: base::JSONReader reader;
Do we need to be parsing this in a sandboxed utility process? I think this is OK
since we're getting this from a google domain over HTTPS.

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.cc:242: if
(!scopes_list->GetDictionary(index, &scopes_entry)) {
Not required, but you could compact this a bit by defining all the variables at
once and combining the ifs:

base::DictionaryValue* scopes_entry = NULL;
IssueAdviceInfoEntry entry;
std::string detail;
if (!scopes_list->GetDictionary(index, &scopes_entry) ||
    !scopes_entry->GetString(kDescriptionKey, &entry.description) ||
    !scopes_entry->GetString(kDetailKey, &detail)) {
  success = false;
  break;
}

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
File chrome/common/net/gaia/oauth2_mint_token_flow.h (right):

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.h:32: //   - Details 1.2
nit: details-> detail?

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.h:55: : public OAuth2ApiCallFlow {
nit: this can go on the same line now

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.h:76: const
std::vector<std::string>& scopes_arg,
nit: don't think you need to actually have different names. e.g. scopes(scopes)
would work still.

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.h:130: const
GoogleServiceAuthError& error) OVERRIDE;
Just verifying behavior: this error method would be called if we're in mode
MODE_MINT_TOKEN_NO_FORCE and the scopes had been revoked server side by the
user?

[Munjal (Google), 2012-04-11 19:37:55]

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
File chrome/common/net/gaia/oauth2_mint_token_flow.cc (right):

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.cc:207: base::JSONReader reader;
On 2012/04/11 19:14:29, jstritar wrote:
> Do we need to be parsing this in a sandboxed utility process? I think this is
OK
> since we're getting this from a google domain over HTTPS.

I don't think we need sandboxing here. There is already a ton of code in
net/gaia that parses responses from various Google endpoints into JSON. None of
them use sandboxing.

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.cc:242: if
(!scopes_list->GetDictionary(index, &scopes_entry)) {
On 2012/04/11 19:14:29, jstritar wrote:
> Not required, but you could compact this a bit by defining all the variables
at
> once and combining the ifs:
> 
> base::DictionaryValue* scopes_entry = NULL;
> IssueAdviceInfoEntry entry;
> std::string detail;
> if (!scopes_list->GetDictionary(index, &scopes_entry) ||
>     !scopes_entry->GetString(kDescriptionKey, &entry.description) ||
>     !scopes_entry->GetString(kDetailKey, &detail)) {
>   success = false;
>   break;
> }

Done.

I like it. I think I have a tendency to do it separately since sometimes we need
to log separate messages or report different error code. But in this case that
is not true.

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
File chrome/common/net/gaia/oauth2_mint_token_flow.h (right):

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.h:55: : public OAuth2ApiCallFlow {
On 2012/04/11 19:14:29, jstritar wrote:
> nit: this can go on the same line now

Done.

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.h:76: const
std::vector<std::string>& scopes_arg,
On 2012/04/11 19:14:29, jstritar wrote:
> nit: don't think you need to actually have different names. e.g.
scopes(scopes)
> would work still.

I see. I think this is still better for readability. Otherwise, a code reader
could be not sure and might think it is a bug.

http://codereview.chromium.org/10012051/diff/11006/chrome/common/net/gaia/oau...
chrome/common/net/gaia/oauth2_mint_token_flow.h:130: const
GoogleServiceAuthError& error) OVERRIDE;
On 2012/04/11 19:14:29, jstritar wrote:
> Just verifying behavior: this error method would be called if we're in mode
> MODE_MINT_TOKEN_NO_FORCE and the scopes had been revoked server side by the
> user?

Actually it will be called for all errors that can occur in minting a token, not
just in NO_FORCE mode. In force mode too, an auth error can occur or some other
error can happen.

[Munjal (Google), 2012-04-11 19:38:53]

I forgot to address your top level comment about naming.

I think a better name would be OAuth2IssueTokenFlow since that is the exact
endpoint we are hitting: an IssueToken endpoint.

Let us do name change in a different CL to be less disruptive and we can take
our time to figure out the right name.

[jstritar, 2012-04-11 19:43:47]

LGTM, and the name you suggested sounds good to me.

[Munjal (Google), 2012-04-11 21:38:52]

I was just thinking... OAuth2IssueTokenFlow and OAuth2MintTokenFlow are so
similar that the effort of renaming it may not be worth it. "mint" is as
commonly used a verb as "issue".

[msw, 2012-04-11 21:47:55]

On 2012/04/11 21:38:52, munjal wrote:
> I was just thinking... OAuth2IssueTokenFlow and OAuth2MintTokenFlow are so
> similar that the effort of renaming it may not be worth it. "mint" is as
> commonly used a verb as "issue".

Drive-by opinion: I'm ignorant here, but I had no idea that "mint" was a verb or
what it meant when I first saw such classes... The meaning and use of the
classes was almost lost on me until sail@ explicitly pointed me to them. I would
prefer naming consistency with the IssueToken operation.