OAuth 2.0: Credential, BrowserClientRequestUrl, BearerToken, & TokenResponseException

[danielcar, 2011-12-06 03:08:41]

LGTM

http://codereview.appspot.com/5440105/diff/3003/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AccessProtectedResource.java
(right):

http://codereview.appspot.com/5440105/diff/3003/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AccessProtectedResource.java:81:
static final Logger LOGGER =
Logger.getLogger(AccessProtectedResource.class.getName());
Consider using:
import com.google.common.labs.misc.Loggers;

    private static final Logger log = Loggers.getContextLogger();

Makes for easier cut and paste.

[yanivi, 2011-12-06 15:07:55]

http://codereview.appspot.com/5440105/diff/3003/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AccessProtectedResource.java
(right):

http://codereview.appspot.com/5440105/diff/3003/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AccessProtectedResource.java:81:
static final Logger LOGGER =
Logger.getLogger(AccessProtectedResource.class.getName());
On 2011/12/06 03:08:41, danielcar wrote:
> Consider using:
> import com.google.common.labs.misc.Loggers;
> 
>     private static final Logger log = Loggers.getContextLogger();
> 
> Makes for easier cut and paste.

There is such Loggers class in Google Guava
(http://code.google.com/p/guava-libraries/).

[yanivi, 2012-01-19 21:10:12]

Ready for review!

[yanivi, 2012-01-26 20:46:11]

Ping.

[Daniel.Cardenas, 2012-01-26 21:35:14]

LGTM

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeTokenRequest.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeTokenRequest.java:137:
public final String getCode() {
I wonder if using final for security purposes is a good idea.  If a user is
programming on a platform and has access to the open source, could they just
replace the libraries with their own?  The use of final's makes mock testing
difficult.

[yanivi, 2012-01-27 12:57:47]

Thanks Daniel.  Now waiting for Ravi to review.

[rmistry, 2012-01-27 17:52:32]

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:27:
*
href="http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-4.1">Authorization
Code</a>).
Seems to me that class could be extended to work with different authorization
grant types and not just authorization code. Update javadoc to instead point to
http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-4 (Obtaining
Authorization) ?

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:129:
*        containing multiple space-separated scopes)
same comment as below for varargs here.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:199:
*        multiple space-separated scopes)
javadoc for varargs, maybe something like:

@param scopes the scopes to be joined by a space separator (or a single value
containing multiple space-separated scopes). The number of arguments is variable
and may be zero. The maximum number of arguments is limited by the maximum
dimension of a Java array as defined by the <a
href='http://java.sun.com/docs/books/vmspec/'>Java Virtual Machine
Specification</a>.

May be overkill, leaving it upto you.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/ClientParametersAuthentication.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/ClientParametersAuthentication.java:39:
* Sample usage can be found for {@link AuthorizationCodeTokenRequest}, except
substitute
can be found "in".

Why not just paste the sample usage here instead of pointing users somewhere
else? is it because of having to maintain in multiple places?

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:184:
* we are 10 seconds away from expiration. If token server is unavailable, it
will try to use the
implementation seems to refresh if token will expire in a minute

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponse.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponse.java:54:
* or {@code null} for none.
as described in http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-6

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponse.java:59:
/** Scope of the access token or {@code null} for none. */
as described in http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-3.3

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponseException.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponseException.java:29:
* access token that uses
incomplete sentence?

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponseException.java:55:
private TokenResponseException(JsonFactory jsonFactory, HttpResponse response,
jsonFactory is unused

[yanivi, 2012-01-27 22:09:56]

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeTokenRequest.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationCodeTokenRequest.java:137:
public final String getCode() {
On 2012/01/26 21:35:14, Daniel.Cardenas wrote:
> I wonder if using final for security purposes is a good idea.  If a user is
> programming on a platform and has access to the open source, could they just
> replace the libraries with their own?  The use of final's makes mock testing
> difficult.

I don't understand what security consideration you are referring to here.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:27:
*
href="http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-4.1">Authorization
Code</a>).
On 2012/01/27 17:52:32, rmistry wrote:
> Seems to me that class could be extended to work with different authorization
> grant types and not just authorization code. Update javadoc to instead point
to
> http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-4 (Obtaining
> Authorization) ?

Done.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:129:
*        containing multiple space-separated scopes)
On 2012/01/27 17:52:32, rmistry wrote:
> same comment as below for varargs here.

See below.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/AuthorizationRequestUrl.java:199:
*        multiple space-separated scopes)
On 2012/01/27 17:52:32, rmistry wrote:
> javadoc for varargs, maybe something like:
> 
> @param scopes the scopes to be joined by a space separator (or a single value
> containing multiple space-separated scopes). The number of arguments is
variable
> and may be zero. The maximum number of arguments is limited by the maximum
> dimension of a Java array as defined by the <a
> href='http://java.sun.com/docs/books/vmspec/'>Java Virtual Machine
> Specification</a>.
> 
> May be overkill, leaving it upto you.

I feel that it is overkill to explain to developers what varargs is, just based
on the JavaDoc I've seen elsewhere for varargs in the JDK and Guava.  Let me
know if you can find an example that goes into this much detail.

With respect to specifying that zero args is allowed, I think any time you have
a list or a variable number of args you should assume zero parameters is
allowed.  Otherwise, I would have declared it setScopes(String firstScope,
String... otherScores).  Again, this JavaDoc style is consistent with JavaDoc
I've seen in other places like Guava.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/ClientParametersAuthentication.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/ClientParametersAuthentication.java:39:
* Sample usage can be found for {@link AuthorizationCodeTokenRequest}, except
substitute
On 2012/01/27 17:52:32, rmistry wrote:
> can be found "in".
> 
> Why not just paste the sample usage here instead of pointing users somewhere
> else? is it because of having to maintain in multiple places?

Done.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:184:
* we are 10 seconds away from expiration. If token server is unavailable, it
will try to use the
On 2012/01/27 17:52:32, rmistry wrote:
> implementation seems to refresh if token will expire in a minute

Done.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponse.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponse.java:54:
* or {@code null} for none.
On 2012/01/27 17:52:32, rmistry wrote:
> as described in http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-6

Done-ish: I point here to RefreshTokenRequest instead

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponse.java:59:
/** Scope of the access token or {@code null} for none. */
On 2012/01/27 17:52:32, rmistry wrote:
> as described in http://tools.ietf.org/html/draft-ietf-oauth-v2-22#section-3.3

Done.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponseException.java
(right):

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponseException.java:29:
* access token that uses
On 2012/01/27 17:52:32, rmistry wrote:
> incomplete sentence?

Done.

http://codereview.appspot.com/5440105/diff/8085/google-oauth-client/src/main/...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/TokenResponseException.java:55:
private TokenResponseException(JsonFactory jsonFactory, HttpResponse response,
On 2012/01/27 17:52:32, rmistry wrote:
> jsonFactory is unused

Done.

[rmistry, 2012-01-28 00:11:17]

LGTM

[rmistry, 2012-01-28 00:17:29]

http://codereview.appspot.com/5440105/diff/26007/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/package-info.java
(right):

http://codereview.appspot.com/5440105/diff/26007/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/package-info.java:41:
*
Maybe add a section here about browser based client flow like you did in:
http://codereview.appspot.com/5453048/diff2/13002:27003/google-api-client/src...

[yanivi, 2012-01-28 12:46:42]

http://codereview.appspot.com/5440105/diff/26007/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/package-info.java
(right):

http://codereview.appspot.com/5440105/diff/26007/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/package-info.java:41:
*
On 2012/01/28 00:17:29, rmistry wrote:
> Maybe add a section here about browser based client flow like you did in:
>
http://codereview.appspot.com/5453048/diff2/13002:27003/google-api-client/src...

Done.