Fixes to AccessProtectedResources and subclasses.

http://codereview.appspot.com/4742048/

google-oauth-client-extensions/src/main/java/com/google/api/client/extensions/auth/helpers/appengine/AccessAppResource.java

@@ -16,7 +16,6 @@
 
 import com.google.api.client.auth.oauth2.draft10.AccessProtectedResource;
 import com.google.api.client.auth.oauth2.draft10.AccessTokenRequest.AssertionGrant;
-import com.google.api.client.auth.oauth2.draft10.AccessTokenResponse;
 import com.google.api.client.extensions.appengine.auth.SignedTokenGenerator;
 import com.google.api.client.http.HttpTransport;
 import com.google.api.client.json.JsonFactory;
@@ -105,9 +104,6 @@ protected boolean executeRefreshToken() throws IOException {
     }
     AssertionGrant tokenRequest = new AssertionGrant(
         transport, jsonFactory, authorizationServerUrl, ASSERTION_TYPE, assertion);
-    AccessTokenResponse tokenResponse = tokenRequest.execute();
-    setAccessToken(tokenResponse.accessToken);
-
-    return true;
+    return executeAccessTokenRequest(tokenRequest);
   }
 }

google-oauth-client-extensions/src/main/java/com/google/api/client/extensions/auth/helpers/appengine/AppAssertionCredential.java

@@ -53,7 +53,7 @@ public final class AppAssertionCredential implements Credential {
   private String applicationName;
 
   /**
-   * Access token to use for OAuth2 authenticated requests.
+   * Access token to use for OAuth2 authenticated requests or {@code null} for none.
    */
   @Persistent
   private String accessToken;
@@ -102,7 +102,7 @@ public AppAssertionCredential(
   /**
    * Set the access token to a new value.
    *
-   * @param accessToken New access token.
+   * @param accessToken New access token or {@code null} for none
    */
   public void setAccessToken(String accessToken) {
     this.accessToken = accessToken;

google-oauth-client-extensions/src/main/java/com/google/api/client/extensions/auth/helpers/oauth/OAuthHmacCredential.java

@@ -67,7 +67,8 @@ public final class OAuthHmacCredential implements Credential, InstanceCallbacks
   private String tokenSharedSecret;
 
   /**
-   * Token that has been authorized by the end user to allow the server to access the resources.
+   * Token that has been authorized by the end user to allow the server to access the resources or
+   * {@code null} for none.
    */
   @Persistent
   private String token;
@@ -86,7 +87,7 @@ public final class OAuthHmacCredential implements Credential, InstanceCallbacks
    * @param sharedSecret Secret that the server shares with the service provider.
    * @param tokenSharedSecret Token secret that server uses to authenticate the requests.
    * @param token Token that has been authorized by the end user to allow the server to access the
-   *        resources.
+   *        resources or {@code null} for none
    */
   public OAuthHmacCredential(String userId, String consumerKey, String sharedSecret,
       String tokenSharedSecret, String token) {

google-oauth-client-extensions/src/main/java/com/google/api/client/extensions/auth/helpers/oauth2/draft10/OAuth2Credential.java

@@ -53,9 +53,7 @@ public final class OAuth2Credential implements Credential, InstanceCallbacks {
   @PrimaryKey
   private String userId;
 
-  /**
-   * Access token used to authorize requests.
-   */
+  /** Access token used to authorize requests or {@code null} for none. */
   @Persistent
   private volatile String accessToken;
 
@@ -180,6 +178,7 @@ public String getAccessToken() {
     return authInterceptor.getAccessToken();
   }
 
+  /** Sets the access token or {@code null} for none. */
   public void setAccessToken(String accessToken) {
     this.accessToken = accessToken;
   }

google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/draft10/AccessProtectedResource.java

@@ -370,14 +370,17 @@ public boolean handleResponse(
    * Request a new access token from the authorization endpoint.
    *
    * <p>
-   * Default implementation executes the refresh token grant parameter passed to the constructor or
-   * {@code false} if it was {@code null}. Subclasses may override. If a new access token was
-   * retrieved, implementations must call {@link #setAccessToken(String)}. Implementations can
-   * assume proper thread synchronization is already taken care of inside {@link #refreshToken()},
-   * where this is called from.
+   * If the refresh token grant parameter passed to the constructor was {@code null}, default
+   * implementation uses just always returns {@code false}. Otherwise, it uses
+   * {@link RefreshTokenGrant} based on the refresh token and then passes it to
+   * {@link #executeAccessTokenRequest(AccessTokenRequest)} to execute.
+   * </p>
+   * <p>
+   * Subclasses may override. Implementations can assume proper thread synchronization is already
+   * taken care of inside {@link #refreshToken()}, where this is called from.
    * </p>
    *
-   * @return whether a new access token was retrieved
+   * @return whether a new access token was successfully retrieved
    * @throws IOException I/O exception
    */
   protected boolean executeRefreshToken() throws IOException {
@@ -388,18 +391,37 @@ protected boolean executeRefreshToken() throws IOException {
           clientId,
           clientSecret,
           refreshToken);
-      try {
-        setAccessToken(request.execute().accessToken);
-      } catch (HttpResponseException e) {
-        // We were unable to get a new access token (e.g. it may have been revoked), we must now
-        // indicate that our current token is invalid.
-        setAccessToken(null);
-      }
-      return true;
+      return executeAccessTokenRequest(request);
     }
     return false;
   }
 
+  /**
+   * Executes the given access token request and calls {@link #setAccessToken(String)} to the access
+   * token from the response or {@code null} for an error response (whose error message is silently
+   * ignored).
+   *
+   * @param request access token request
+   * @return whether a new access token was successfully retrieved
+   * @throws IOException any I/O problem except {@link HttpResponseException} which is silently
+   *         handled
+   * @since 1.5
+   */
+  protected final boolean executeAccessTokenRequest(AccessTokenRequest request) throws IOException {
+    String newAccessToken;
+    try {
+      newAccessToken = request.execute().accessToken;
+    } catch (HttpResponseException e) {
+      // We were unable to get a new access token (e.g. it may have been revoked), we must now
+      // indicate that our current token is invalid.
+      newAccessToken = null;
+      // ignore the error response
+      e.getResponse().ignore();
+    }
+    setAccessToken(newAccessToken);
+    return newAccessToken != null;
+  }
+
   /**
    * Notifies of a new access token.
    *

google-oauth-client/src/test/java/com/google/api/client/auth/oauth2/draft10/AccessProtectedResourceTest.java

@@ -23,6 +23,7 @@
 import com.google.api.client.http.LowLevelHttpResponse;
 import com.google.api.client.http.UrlEncodedContent;
 import com.google.api.client.json.Json;
+import com.google.api.client.json.JsonFactory;
 import com.google.api.client.json.jackson.JacksonFactory;
 import com.google.api.client.testing.http.MockHttpTransport;
 import com.google.api.client.testing.http.MockLowLevelHttpRequest;
@@ -40,24 +41,33 @@
  */
 public class AccessProtectedResourceTest extends TestCase {
 
+  static final JsonFactory JSON_FACTORY = new JacksonFactory();
+  static final String ACCESS_TOKEN = "abc";
+  static final String NEW_ACCESS_TOKEN = "def";
+  static final String AUTHORIZATION_SERVER_URL = "http://foo.com";
+  static final String CLIENT_ID = "id";
+  static final String CLIENT_SECRET = "secret";
+  static final String REFRESH_TOKEN = "refreshToken";
+
   public void testAccessProtectedResource_header() throws IOException {
     AccessProtectedResource credential =
-        new AccessProtectedResource("abc", Method.AUTHORIZATION_HEADER);
+        new AccessProtectedResource(ACCESS_TOKEN, Method.AUTHORIZATION_HEADER);
     HttpRequest request = subtestAccessProtectedResource(credential);
     assertEquals("OAuth abc", request.getHeaders().getAuthorization());
   }
 
   public void testAccessProtectedResource_queryParam() throws IOException {
-    AccessProtectedResource credential = new AccessProtectedResource("abc", Method.QUERY_PARAMETER);
+    AccessProtectedResource credential =
+        new AccessProtectedResource(ACCESS_TOKEN, Method.QUERY_PARAMETER);
     HttpRequest request = subtestAccessProtectedResource(credential);
-    assertEquals("abc", request.getUrl().get("oauth_token"));
+    assertEquals(ACCESS_TOKEN, request.getUrl().get("oauth_token"));
   }
 
   public void testAccessProtectedResource_body() throws IOException {
     AccessProtectedResource credential =
-        new AccessProtectedResource("abc", Method.FORM_ENCODED_BODY);
+        new AccessProtectedResource(ACCESS_TOKEN, Method.FORM_ENCODED_BODY);
     HttpRequest request = subtestAccessProtectedResource(credential);
-    assertEquals("abc",
+    assertEquals(ACCESS_TOKEN,
         ((GenericData) ((UrlEncodedContent) request.getContent()).getData()).get("oauth_token"));
   }
 
@@ -89,44 +99,66 @@ public boolean checkAuth(MockLowLevelHttpRequest req) {
             return req.getUrl().contains("oauth_token=def");
           }
         });
-    assertEquals("def", request.getUrl().get("oauth_token"));
+    assertEquals(NEW_ACCESS_TOKEN, request.getUrl().get("oauth_token"));
   }
 
   public void testAccessProtectedResource_expiredBody() throws IOException {
     HttpRequest request =
         subtestAccessProtectedResource_expired(Method.FORM_ENCODED_BODY, new CheckAuth() {
 
           public boolean checkAuth(MockLowLevelHttpRequest req) {
-            return "def".equals(
+            return NEW_ACCESS_TOKEN.equals(
                 ((GenericData) ((UrlEncodedContent) req.getContent()).getData()).get(
                     "oauth_token"));
           }
         });
-    assertEquals("def",
+    assertEquals(NEW_ACCESS_TOKEN,
         ((GenericData) ((UrlEncodedContent) request.getContent()).getData()).get("oauth_token"));
   }
 
   interface CheckAuth {
     boolean checkAuth(MockLowLevelHttpRequest req);
   }
 
+  static class AccessTokenTransport extends MockHttpTransport {
+
+    boolean error = false;
+
+    @Override
+    public LowLevelHttpRequest buildPostRequest(String url) {
+      return new MockLowLevelHttpRequest(url) {
+        @Override
+        public LowLevelHttpResponse execute() {
+          final MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
+          response.setContentType(Json.CONTENT_TYPE);
+          GenericData responseData;
+          if (error) {
+            AccessTokenErrorResponse json = new AccessTokenErrorResponse();
+            json.error =
+                AccessTokenErrorResponse.KnownError.INVALID_CLIENT.toString().toLowerCase();
+            responseData = json;
+          } else {
+            AccessTokenResponse json = new AccessTokenResponse();
+            json.accessToken = NEW_ACCESS_TOKEN;
+            responseData = json;
+          }
+          response.setContent(JSON_FACTORY.toString(responseData));
+          return response;
+        }
+      };
+    }
+  }
+
   private HttpRequest subtestAccessProtectedResource_expired(
       Method method, final CheckAuth checkAuth) throws IOException {
-    final AccessProtectedResource credential =
-        new AccessProtectedResource("abc", method, new MockHttpTransport() {
-          @Override
-          public LowLevelHttpRequest buildPostRequest(String url) {
-            return new MockLowLevelHttpRequest(url) {
-              @Override
-              public LowLevelHttpResponse execute() {
-                final MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
-                response.setContentType(Json.CONTENT_TYPE);
-                response.setContent("{\"access_token\":\"def\"}");
-                return response;
-              }
-            };
-          }
-        }, new JacksonFactory(), "http://foo.com", "id", "secret", "refreshToken");
+    final AccessProtectedResource credential = new AccessProtectedResource(ACCESS_TOKEN,
+        method,
+        new AccessTokenTransport(),
+        JSON_FACTORY,
+        AUTHORIZATION_SERVER_URL,
+        CLIENT_ID,
+        CLIENT_SECRET,
+        REFRESH_TOKEN);
     class MyTransport extends MockHttpTransport {
       boolean resetAccessToken;
 
@@ -139,7 +171,7 @@ public LowLevelHttpResponse execute() {
             if (!checkAuth.checkAuth(this)) {
               response.setStatusCode(401);
               if (resetAccessToken) {
-                credential.setAccessToken("def");
+                credential.setAccessToken(NEW_ACCESS_TOKEN);
               }
             }
             return response;
@@ -151,9 +183,44 @@ public LowLevelHttpResponse execute() {
     HttpRequestFactory requestFactory = transport.createRequestFactory(credential);
     HttpRequest request = requestFactory.buildDeleteRequest(new GenericUrl());
     request.execute();
-    credential.setAccessToken("abc");
+    credential.setAccessToken(ACCESS_TOKEN);
     transport.resetAccessToken = true;
     request.execute();
     return request;
   }
+
+  public void testRefreshToken_noRefreshToken() throws IOException {
+    AccessProtectedResource accesss =
+        new AccessProtectedResource(ACCESS_TOKEN, Method.QUERY_PARAMETER);
+    assertFalse(accesss.refreshToken());
+  }
+
+  public void testRefreshToken_refreshToken() throws IOException {
+    AccessTokenTransport transport = new AccessTokenTransport();
+    AccessProtectedResource access = new AccessProtectedResource(ACCESS_TOKEN,
+        Method.QUERY_PARAMETER,
+        transport,
+        JSON_FACTORY,
+        AUTHORIZATION_SERVER_URL,
+        CLIENT_ID,
+        CLIENT_SECRET,
+        REFRESH_TOKEN);
+    assertTrue(access.refreshToken());
+    assertEquals(NEW_ACCESS_TOKEN, access.getAccessToken());
+  }
+
+  public void testRefreshToken_refreshTokenError() throws IOException {
+    AccessTokenTransport transport = new AccessTokenTransport();
+    transport.error = true;
+    AccessProtectedResource access = new AccessProtectedResource(ACCESS_TOKEN,
+        Method.QUERY_PARAMETER,
+        transport,
+        JSON_FACTORY,
+        AUTHORIZATION_SERVER_URL,
+        CLIENT_ID,
+        CLIENT_SECRET,
+        REFRESH_TOKEN);
+    assertFalse(access.refreshToken());
+    assertNull(access.getAccessToken());
+  }
 }