Cleanup ImpersonatedCredentials (#212)

* Cleanup

* Fix tests

* No need for singular ERROR_PREFIX that doesn't provide extra information

* Fix spelling error

Author: chingor13

oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java

@@ -59,15 +59,10 @@
 import com.google.common.collect.ImmutableMap;
 
 /**
- * ImpersonatedCredentials allowing credentials issued to a user or
- * service account to impersonate another.
- * <br/>
- * The source project using ImpersonatedCredentials must enable the
- * "IAMCredentials" API.<br/>
- * Also, the target service account must grant the orginating principal the
- * "Service Account Token Creator" IAM role.
- * <br/>
- * Usage:<br/>
+ * ImpersonatedCredentials allowing credentials issued to a user or service account to impersonate
+ * another. <br> The source project using ImpersonatedCredentials must enable the "IAMCredentials"
+ * API.<br> Also, the target service account must grant the orginating principal the "Service
+ * Account Token Creator" IAM role. <br> Usage:<br>
  * <pre>
  * String credPath = "/path/to/svc_account.json";
  * ServiceAccountCredentials sourceCredentials = ServiceAccountCredentials
@@ -92,7 +87,6 @@ public class ImpersonatedCredentials extends GoogleCredentials {
   private static final String RFC3339 = "yyyy-MM-dd'T'HH:mm:ss'Z'";
   private static final int ONE_HOUR_IN_SECONDS = 3600;
   private static final String CLOUD_PLATFORM_SCOPE = "https://www.googleapis.com/auth/cloud-platform";
-  private static final String ERROR_PREFIX = "Error processng IamCredentials generateAccessToken: ";
   private static final String IAM_ENDPOINT = "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/%s:generateAccessToken";
 
   private static final String SCOPE_EMPTY_ERROR = "Scopes cannot be null";
@@ -108,74 +102,64 @@ public class ImpersonatedCredentials extends GoogleCredentials {
   private transient HttpTransportFactory transportFactory;
 
   /**
-   * @param sourceCredentials The source credential used as to acquire the
-   * impersonated credentials
-   * @param targetPrincipal   The service account to impersonate.
-   * @param delegates         The chained list of delegates required to grant
-   * the final access_token.  <br/>If set, the sequence of identities must
-   * have "Service Account Token Creator" capability granted to the
-   * prceeding identity.  <br/>For example, if set to
-   * [serviceAccountB, serviceAccountC], the sourceCredential
-   * must have the Token Creator role on serviceAccountB. serviceAccountB must
-   * have the Token Creator on serviceAccountC.  <br/>Finally, C must have
-   * Token Creator on target_principal. If left unset, sourceCredential
-   * must have that role on targetPrincipal.
-   * @param scopes            Scopes to request during the authorization grant.
-   * @param lifetime          Number of seconds the delegated credential should
-   * be valid for (upto 3600).
-   * @param transportFactory  HTTP transport factory, creates the transport used
-   *                          to get access tokens.
+   * @param sourceCredentials The source credential used as to acquire the impersonated credentials
+   * @param targetPrincipal The service account to impersonate.
+   * @param delegates The chained list of delegates required to grant the final access_token. If
+   * set, the sequence of identities must have "Service Account Token Creator" capability granted to
+   * the preceding identity. For example, if set to [serviceAccountB, serviceAccountC], the
+   * sourceCredential must have the Token Creator role on serviceAccountB. serviceAccountB must have
+   * the Token Creator on serviceAccountC. Finally, C must have Token Creator on target_principal.
+   * If left unset, sourceCredential must have that role on targetPrincipal.
+   * @param scopes Scopes to request during the authorization grant.
+   * @param lifetime Number of seconds the delegated credential should be valid for (up to 3600).
+   * @param transportFactory HTTP transport factory, creates the transport used to get access
+   * tokens.
    */
-  public static ImpersonatedCredentials create(GoogleCredentials sourceCredentials, String targetPrincipal,
-      List<String> delegates, List<String> scopes, int lifetime, HttpTransportFactory transportFactory) {
-    return ImpersonatedCredentials.newBuilder().setSourceCredentials(sourceCredentials)
-        .setTargetPrincipal(targetPrincipal).setDelegates(delegates).setScopes(scopes).setLifetime(lifetime)
-        .setHttpTransportFactory(transportFactory).build();
+  public static ImpersonatedCredentials create(GoogleCredentials sourceCredentials,
+      String targetPrincipal,
+      List<String> delegates, List<String> scopes, int lifetime,
+      HttpTransportFactory transportFactory) {
+    return ImpersonatedCredentials.newBuilder()
+        .setSourceCredentials(sourceCredentials)
+        .setTargetPrincipal(targetPrincipal)
+        .setDelegates(delegates)
+        .setScopes(scopes)
+        .setLifetime(lifetime)
+        .setHttpTransportFactory(transportFactory)
+        .build();
   }
 
   /**
-   * @param sourceCredentials The source credential used as to acquire the
-   * impersonated credentials
-   * @param targetPrincipal   The service account to impersonate.
-   * @param delegates         The chained list of delegates required to grant
-   * the final access_token.  <br/>If set, the sequence of identities must
-   * have "Service Account Token Creator" capability granted to the
-   * prceeding identity.  <br/>For example, if set to
-   * [serviceAccountB, serviceAccountC], the sourceCredential
-   * must have the Token Creator role on serviceAccountB. serviceAccountB must
-   * have the Token Creator on serviceAccountC.  <br/>Finally, C must have
-   * Token Creator on target_principal. If left unset, sourceCredential
-   * must have that role on targetPrincipal.
-   * @param scopes            Scopes to request during the authorization grant.
-   * @param lifetime          Number of seconds the delegated credential should
-   * be valid for (upto 3600).
-   */ 
-  public static ImpersonatedCredentials create(GoogleCredentials sourceCredentials, String targetPrincipal,
+   * @param sourceCredentials The source credential used as to acquire the impersonated credentials
+   * @param targetPrincipal The service account to impersonate.
+   * @param delegates The chained list of delegates required to grant the final access_token. If
+   * set, the sequence of identities must have "Service Account Token Creator" capability granted to
+   * the preceding identity. For example, if set to [serviceAccountB, serviceAccountC], the
+   * sourceCredential must have the Token Creator role on serviceAccountB. serviceAccountB must have
+   * the Token Creator on serviceAccountC. Finally, C must have Token Creator on target_principal.
+   * If left unset, sourceCredential must have that role on targetPrincipal.
+   * @param scopes Scopes to request during the authorization grant.
+   * @param lifetime Number of seconds the delegated credential should be valid for (up to 3600).
+   */
+  public static ImpersonatedCredentials create(GoogleCredentials sourceCredentials,
+      String targetPrincipal,
       List<String> delegates, List<String> scopes, int lifetime) {
-    return ImpersonatedCredentials.newBuilder().setSourceCredentials(sourceCredentials)
-        .setTargetPrincipal(targetPrincipal).setDelegates(delegates).setScopes(scopes).setLifetime(lifetime).build();
+    return ImpersonatedCredentials.newBuilder()
+        .setSourceCredentials(sourceCredentials)
+        .setTargetPrincipal(targetPrincipal)
+        .setDelegates(delegates)
+        .setScopes(scopes)
+        .setLifetime(lifetime)
+        .build();
   }
 
-  /**
-   * @param sourceCredentials = Source Credentials.
-   * @param targetPrincipal   = targetPrincipal;
-   * @param delegates         = delegates;
-   * @param scopes            = scopes;
-   * @param lifetime          = lifetime;
-   * @param transportFactory  = HTTP transport factory, creates the transport used
-   *                          to get access tokens.
-   * @deprecated Use {@link #create(ImpersonatedCredentials)} instead. This constructor
-   *             will either be deleted or made private in a later version.
-   */
-  @Deprecated
-  private ImpersonatedCredentials(GoogleCredentials sourceCredentials, String targetPrincipal, List<String> delegates,
-      List<String> scopes, int lifetime, HttpTransportFactory transportFactory) {
-    this.sourceCredentials = sourceCredentials;
-    this.targetPrincipal = targetPrincipal;
-    this.delegates = delegates;
-    this.scopes = scopes;
-    this.lifetime = lifetime;
-    this.transportFactory = firstNonNull(transportFactory,
+  private ImpersonatedCredentials(Builder builder) {
+    this.sourceCredentials = builder.getSourceCredentials();
+    this.targetPrincipal = builder.getTargetPrincipal();
+    this.delegates = builder.getDelegates();
+    this.scopes = builder.getScopes();
+    this.lifetime = builder.getLifetime();
+    this.transportFactory = firstNonNull(builder.getHttpTransportFactory(),
         getFromServiceLoader(HttpTransportFactory.class, OAuth2Utils.HTTP_TRANSPORT_FACTORY));
     this.transportFactoryClassName = this.transportFactory.getClass().getName();
     if (this.delegates == null) {
@@ -189,47 +173,17 @@ private ImpersonatedCredentials(GoogleCredentials sourceCredentials, String targ
     }
   }
 
-  /**
-   * @param sourceCredentials = Source Credentials.
-   * @param targetPrincipal   = targetPrincipal;
-   * @param scopes            = scopes;
-   * @param lifetime          = lifetime;
-   * @param transportFactory  = HTTP transport factory, creates the transport used
-   *                          to get access tokens.
-   * @deprecated Use {@link #create(ImpersonatedCredentials)} instead. This constructor
-   *             will either be deleted or made private in a later version.
-   */
-  @Deprecated
-  private ImpersonatedCredentials(GoogleCredentials sourceCredentials, String targetPrincipal, List<String> scopes,
-      int lifetime, HttpTransportFactory transportFactory) {
-    this(sourceCredentials, targetPrincipal, new ArrayList<String>(), scopes, lifetime, transportFactory);
-  }
-
-  /**
-   * @param sourceCredentials = Source Credentials.
-   * @param targetPrincipal   = targetPrincipal;
-   * @param delegates         = delegates;
-   * @param scopes            = scopes;
-   * @param lifetime          = lifetime;
-   * @deprecated Use {@link #create(ImpersonatedCredentials)} instead. This constructor
-   *             will either be deleted or made private in a later version.
-   */
-  @Deprecated
-  private ImpersonatedCredentials(GoogleCredentials sourceCredentials, String targetPrincipal, List<String> scopes,
-      int lifetime) {
-    this(sourceCredentials, targetPrincipal, new ArrayList<String>(), scopes, lifetime, null);
-  }
-
   @Override
   public AccessToken refreshAccessToken() throws IOException {
-    
+    if (this.sourceCredentials.getAccessToken() == null) {
+      this.sourceCredentials = this.sourceCredentials
+          .createScoped(Arrays.asList(CLOUD_PLATFORM_SCOPE));
+    }
+
     try {
-      if (this.sourceCredentials.getAccessToken() == null) {
-        this.sourceCredentials = this.sourceCredentials.createScoped(Arrays.asList(CLOUD_PLATFORM_SCOPE));
-      }
       this.sourceCredentials.refreshIfExpired();
     } catch (IOException e) {
-      throw new IOException(ERROR_PREFIX + "Unable to refresh sourceCredentials " + e.toString());
+      throw new IOException("Unable to refresh sourceCredentials", e);
     }
 
     HttpTransport httpTransport = this.transportFactory.create();
@@ -241,7 +195,8 @@ public AccessToken refreshAccessToken() throws IOException {
     String endpointUrl = String.format(IAM_ENDPOINT, this.targetPrincipal);
     GenericUrl url = new GenericUrl(endpointUrl);
 
-    Map<String, Object> body = ImmutableMap.<String, Object>of("delegates", this.delegates, "scope", this.scopes,
+    Map<String, Object> body = ImmutableMap.<String, Object>of("delegates", this.delegates, "scope",
+        this.scopes,
         "lifetime", this.lifetime + "s");
 
     HttpContent requestContent = new JsonHttpContent(parser.getJsonFactory(), body);
@@ -253,21 +208,23 @@ public AccessToken refreshAccessToken() throws IOException {
     try {
       response = request.execute();
     } catch (IOException e) {
-      throw new IOException(ERROR_PREFIX + e.toString());
+      throw new IOException("Error requesting access token", e);
     }
 
     GenericData responseData = response.parseAs(GenericData.class);
     response.disconnect();
 
-    String accessToken = OAuth2Utils.validateString(responseData, "accessToken", ERROR_PREFIX);
-    String expireTime = OAuth2Utils.validateString(responseData, "expireTime", ERROR_PREFIX);
+    String accessToken = OAuth2Utils
+        .validateString(responseData, "accessToken", "Expected to find an accessToken");
+    String expireTime = OAuth2Utils
+        .validateString(responseData, "expireTime", "Expected to find an expireTime");
 
     DateFormat format = new SimpleDateFormat(RFC3339);
     Date date;
     try {
       date = format.parse(expireTime);
     } catch (ParseException pe) {
-      throw new IOException(ERROR_PREFIX + pe.getMessage());
+      throw new IOException("Error parsing expireTime: " + pe.getMessage());
     }
     return new AccessToken(accessToken, date);
   }
@@ -383,8 +340,7 @@ public HttpTransportFactory getHttpTransportFactory() {
     }
 
     public ImpersonatedCredentials build() {
-      return new ImpersonatedCredentials(this.sourceCredentials, this.targetPrincipal, this.delegates, this.scopes,
-          this.lifetime, this.transportFactory);
+      return new ImpersonatedCredentials(this);
     }
 
   }

oauth2_http/javatests/com/google/auth/oauth2/ImpersonatedCredentialsTest.java

@@ -43,7 +43,6 @@
 import java.util.Arrays;
 import java.util.List;
 
-import com.google.api.client.http.HttpResponseException;
 import com.google.api.client.http.HttpStatusCodes;
 import com.google.api.client.http.HttpTransport;
 import com.google.api.client.json.JsonFactory;
@@ -134,7 +133,8 @@ public void refreshAccessToken_unauthorized() throws IOException {
       targetCredentials.refreshAccessToken().getTokenValue();
       fail(String.format("Should throw exception with message containing '%s'", expectedMessage));
     } catch (IOException expected) {
-      assertTrue(expected.getMessage().contains(expectedMessage));
+      assertEquals("Error requesting access token", expected.getMessage());
+      assertTrue(expected.getCause().getMessage().contains(expectedMessage));
     }
   }
 
@@ -158,7 +158,8 @@ public void refreshAccessToken_malformedTarget() throws IOException {
       targetCredentials.refreshAccessToken().getTokenValue();
       fail(String.format("Should throw exception with message containing '%s'", expectedMessage));
     } catch (IOException expected) {
-      assertTrue(expected.getMessage().contains(expectedMessage));
+      assertEquals("Error requesting access token", expected.getMessage());
+      assertTrue(expected.getCause().getMessage().contains(expectedMessage));
     }
   }