Issue 70: Use WWW-Authenticate header to guess if token has expired

Issue 70: Use WWW-Authenticate header to guess if token has expired

Issue 77: better JavaDoc for 
Better JavaDoc on Credential.refreshToken()

BASED ON - https://codereview.appspot.com/10450044/

[yanivi, 2013-06-17 18:42:44]

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:100:
private static String INVALID_TOKEN_ERROR = "error=invalid_token";
maybe we should be a little more lenient on the whitespace and do something
like:

  private static final Pattern INVALID_TOKEN_ERROR =
Pattern.compile("\\s*error\\s*=\\s*invalid_token\\s*");

and then below INVALID_TOKEN_ERROR.matcher(headerValue).matches()

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:249:
Object authenticate = response.getHeaders().get(AUTHENTICATE_HEADER);
    Object authenticate = response.getHeaders().getAuthenticate();

or probably more intelligently, we should add
HttpHeaders.getAuthenticateAsList() and then iterate over the possibly multiple
WWW-Authenticate headers, looking for the one with value that
startsWith("\\s*Bearer")

alternatively, we could add a method like getAuthenticate(String challenge) that
would return the value for the given challenge type.

But for now, if it is easier to implement without making changes to
HttpResponse, I'm okay with this implementation that does
getHeaders().get(AUTHENTICATE_HEADER), but then you still need to iterate over
the values to find the one of type "Bearer".

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:252:
|| response.getStatusCode() == HttpStatusCodes.STATUS_CODE_UNAUTHORIZED;
Based on my reading of the spec, there is no reason to check the error code.  I
think the only reason to keep it in there is for backwards compatibility.  But
then it raises the question of what happens if we know error is not
"invalid_token" but the error code is still 401.  Logic should dictate that we
should not refresh the token and try again. 

Perhaps the best compromise is to only check the status code if we're working
with a server that didn't properly implement the spec in that it did not provide
the WWW-Authenticate Bearer challenge?  Incidentally, that was true of Google
until some number of months ago.

[yanivi, 2013-06-17 18:43:00]

please change description to include link to
https://code.google.com/p/google-oauth-java-client/issues/detail?id=70

[peleyal, 2013-06-20 20:03:44]

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:100:
private static String INVALID_TOKEN_ERROR = "error=invalid_token";
On 2013/06/17 18:42:44, yanivi wrote:
> maybe we should be a little more lenient on the whitespace and do something
> like:
> 
>   private static final Pattern INVALID_TOKEN_ERROR =
> Pattern.compile("\\s*error\\s*=\\s*invalid_token\\s*");
> 
> and then below INVALID_TOKEN_ERROR.matcher(headerValue).matches()

Done.

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:249:
Object authenticate = response.getHeaders().get(AUTHENTICATE_HEADER);
On 2013/06/17 18:42:44, yanivi wrote:
>     Object authenticate = response.getHeaders().getAuthenticate();
> 
> or probably more intelligently, we should add
> HttpHeaders.getAuthenticateAsList() and then iterate over the possibly
multiple
> WWW-Authenticate headers, looking for the one with value that
> startsWith("\\s*Bearer")
> 
> alternatively, we could add a method like getAuthenticate(String challenge)
that
> would return the value for the given challenge type.
> 
> But for now, if it is easier to implement without making changes to
> HttpResponse, I'm okay with this implementation that does
> getHeaders().get(AUTHENTICATE_HEADER), but then you still need to iterate over
> the values to find the one of type "Bearer".

Done.

https://codereview.appspot.com/9640045/diff/1/google-oauth-client/src/main/ja...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:252:
|| response.getStatusCode() == HttpStatusCodes.STATUS_CODE_UNAUTHORIZED;
On 2013/06/17 18:42:44, yanivi wrote:
> Based on my reading of the spec, there is no reason to check the error code. 
I
> think the only reason to keep it in there is for backwards compatibility.  But
> then it raises the question of what happens if we know error is not
> "invalid_token" but the error code is still 401.  Logic should dictate that we
> should not refresh the token and try again. 
> 
> Perhaps the best compromise is to only check the status code if we're working
> with a server that didn't properly implement the spec in that it did not
provide
> the WWW-Authenticate Bearer challenge?  Incidentally, that was true of Google
> until some number of months ago.

Done.

[yanivi, 2013-06-21 17:21:46]

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:97:
private static final Pattern INVALID_TOKEN_ERROR =
[optional] move this constant to BearerToken since it is specific to the Bearer
protocol

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:243:
* {@code invalid_token}, otherwise it calls {@link #refreshToken} in case the
status code is
instead of:

, otherwise

something like

.  If there is no "Bearer" challenge in a WWW-Authenticate header, then

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:244:
* {@code 401}. If {@link #executeRefreshToken()} throws an I/O exception, this
implementation
[optional] HttpStatusCodes.STATUS_CODE_UNAUTHORIZED instead of 401

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:252:
List<String> authenticateList = response.getHeaders().getAuthenticateAsList();
[optional] add a TODO to add an interface to make this logic pluggable like we
do for AccessMethod

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:257:
if (authenticate.startsWith("Bearer")) {
startsWith(BearerToken.AuthorizationHeaderAccessMethod.HEADER_PREFIX)

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
File
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthenticationTestBase.java
(right):

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthenticationTestBase.java:54:
boolean error400 = false;
[optional] maybe we should just have "int errorCode" instead of 3 boolean error*
fields.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
File
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/CredentialTest.java
(right):

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/CredentialTest.java:184:
public void testRefreshToken_request_401() throws Exception {
please ctrl-shift-f this code to format it in the standard style

[yanivi, 2013-06-23 12:25:38]

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:485:
public final boolean refreshToken() throws IOException {
while you are already editing this file, can you please fix this issue:

https://code.google.com/p/google-oauth-java-client/issues/detail?id=77

Better JavaDoc on Credential.refreshToken()

If you don't specify a refresh token, it will just quietly return false.  The
JavaDoc is unclear about that, so we should explain it better.

[peleyal, 2013-06-24 15:03:07]

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:97:
private static final Pattern INVALID_TOKEN_ERROR =
On 2013/06/21 17:21:46, yanivi wrote:
> [optional] move this constant to BearerToken since it is specific to the
Bearer
> protocol

Done.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:243:
* {@code invalid_token}, otherwise it calls {@link #refreshToken} in case the
status code is
On 2013/06/21 17:21:46, yanivi wrote:
> instead of:
> 
> , otherwise
> 
> something like
> 
> .  If there is no "Bearer" challenge in a WWW-Authenticate header, then

Done.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:244:
* {@code 401}. If {@link #executeRefreshToken()} throws an I/O exception, this
implementation
On 2013/06/21 17:21:46, yanivi wrote:
> [optional] HttpStatusCodes.STATUS_CODE_UNAUTHORIZED instead of 401

Done.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:252:
List<String> authenticateList = response.getHeaders().getAuthenticateAsList();
On 2013/06/21 17:21:46, yanivi wrote:
> [optional] add a TODO to add an interface to make this logic pluggable like we
> do for AccessMethod

Done.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:257:
if (authenticate.startsWith("Bearer")) {
On 2013/06/21 17:21:46, yanivi wrote:
> startsWith(BearerToken.AuthorizationHeaderAccessMethod.HEADER_PREFIX)

Done.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/main...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:485:
public final boolean refreshToken() throws IOException {
On 2013/06/23 12:25:38, yanivi wrote:
> while you are already editing this file, can you please fix this issue:
> 
> https://code.google.com/p/google-oauth-java-client/issues/detail?id=77
> 
> Better JavaDoc on Credential.refreshToken()
> 
> If you don't specify a refresh token, it will just quietly return false.  The
> JavaDoc is unclear about that, so we should explain it better.

Done.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
File
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthenticationTestBase.java
(right):

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/AuthenticationTestBase.java:54:
boolean error400 = false;
On 2013/06/21 17:21:46, yanivi wrote:
> [optional] maybe we should just have "int errorCode" instead of 3 boolean
error*
> fields.

Done.

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
File
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/CredentialTest.java
(right):

https://codereview.appspot.com/9640045/diff/9003/google-oauth-client/src/test...
google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/CredentialTest.java:184:
public void testRefreshToken_request_401() throws Exception {
On 2013/06/21 17:21:46, yanivi wrote:
> please ctrl-shift-f this code to format it in the standard style

Done.

[yanivi, 2013-06-24 15:48:00]

LGTM

but must fix this issue before submitting...

https://codereview.appspot.com/9640045/diff/26001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/9640045/diff/26001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:245:
List<String> authenticateList = response.getHeaders().getAuthenticateAsList();
Description	Resource	Path	Location	Type
List cannot be resolved to a
type	Credential.java	/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2	line
244	Java Problem

[yanivi, 2013-06-24 20:01:21]

LGTM

Please also update the changeset description to note that you fixed
https://code.google.com/p/google-oauth-java-client/issues/detail?id=77

[peleyal, 2013-06-25 14:54:31]

Done. Was pushed a minute ago

https://codereview.appspot.com/9640045/diff/26001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java
(right):

https://codereview.appspot.com/9640045/diff/26001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/Credential.java:245:
List<String> authenticateList = response.getHeaders().getAuthenticateAsList();
On 2013/06/24 15:48:00, yanivi wrote:
> Description	Resource	Path	Location	Type
> List cannot be resolved to a
>
type	Credential.java	/google-oauth-client/src/main/java/com/google/api/client/auth/oauth2	line
> 244	Java Problem

Done.