feat: improve behavior of HTTP redirects

This commit introduces a new okhttp interceptor that will
implement customized behavior for HTTP redirects.
Specifically, "safe" headers will be included in a
redirected request if the original and redirected hosts
are the same, or are both within IBM's "cloud.ibm.com" domain.

Signed-off-by: Phil Adams <[email protected]>

Author: padamstx

.travis.yml

@@ -16,6 +16,13 @@ cache:
   directories:
   - "$HOME/.m2"
 
+addons:
+  hosts:
+    - region1.cloud.ibm.com
+    - region1.notcloud.ibm.com
+    - region2.cloud.ibm.com
+    - region2.notcloud.ibm.com
+
 env:
   global:
     - MVN_ARGS="--settings build/.travis.settings.xml"

src/main/java/com/ibm/cloud/sdk/core/http/HttpClientSingleton.java

@@ -16,6 +16,8 @@
 import com.ibm.cloud.sdk.core.http.HttpConfigOptions.LoggingLevel;
 import com.ibm.cloud.sdk.core.http.gzip.GzipRequestInterceptor;
 import com.ibm.cloud.sdk.core.service.security.DelegatingSSLSocketFactory;
+import com.ibm.cloud.sdk.core.util.EnvironmentUtils;
+
 import okhttp3.Authenticator;
 import okhttp3.ConnectionSpec;
 import okhttp3.Interceptor;
@@ -179,6 +181,7 @@ private OkHttpClient configureHttpClient() {
     final OkHttpClient.Builder builder = new OkHttpClient.Builder();
 
     addCookieJar(builder);
+    addRedirectInterceptor(builder);
 
     builder.connectTimeout(60, TimeUnit.SECONDS);
     builder.writeTimeout(60, TimeUnit.SECONDS);
@@ -197,11 +200,34 @@ private OkHttpClient configureHttpClient() {
    *
    * @param builder the builder
    */
-  private void addCookieJar(final OkHttpClient.Builder builder) {
+  private OkHttpClient.Builder addCookieJar(final OkHttpClient.Builder builder) {
     final CookieManager cookieManager = new CookieManager();
     cookieManager.setCookiePolicy(CookiePolicy.ACCEPT_ALL);
 
     builder.cookieJar(new ServiceCookieJar(cookieManager));
+    return builder;
+  }
+
+  /**
+   * Adds the RedirectInterceptor to the client builder.
+   * @param builder
+   */
+  private OkHttpClient.Builder addRedirectInterceptor(final OkHttpClient.Builder builder) {
+      String envvar = EnvironmentUtils.getenv("IBMCLOUD_BYPASS_CUSTOM_REDIRECTS");
+      if (!Boolean.valueOf(envvar)) {
+          // Disable the built-in "followRedirects" so that okhttp will ignore redirects.
+          builder.followRedirects(false).followSslRedirects(false);
+
+          // Add our RedirectInterceptor to the client.
+          builder.addInterceptor(new RedirectInterceptor());
+          LOG.log(Level.FINE, "Registered the redirect interceptor.");
+      } else {
+          LOG.log(Level.FINE, "Bypassed redirect interceptor via options.");
+
+          // Set the defaults for these two options.
+          builder.followRedirects(true).followSslRedirects(true);
+      }
+      return builder;
   }
 
   /**
@@ -337,30 +363,6 @@ public static void setupTLSProtocol(final OkHttpClient.Builder builder) {
     }
   }
 
-  /**
-   * Sets a new list of interceptors for the specified {@link OkHttpClient} instance by removing the specified
-   * interceptor and returns a new instance with the interceptors configured as requested.
-   *
-   * @param client the {@link OkHttpClient} instance to remove the interceptors from
-   * @param interceptorToRemove the class name of the interceptor to remove
-   * @return the new {@link OkHttpClient} instance with the new list of interceptors
-   */
-  private OkHttpClient reconfigureClientInterceptors(OkHttpClient client, String interceptorToRemove) {
-    OkHttpClient.Builder builder = client.newBuilder();
-
-    if (!builder.interceptors().isEmpty()) {
-      for (Iterator<Interceptor> iter = builder.interceptors().iterator(); iter.hasNext(); ) {
-        Interceptor element = iter.next();
-        if (interceptorToRemove.equals(element.getClass().getSimpleName())) {
-          LOG.log(Level.FINE, "Removing interceptor " + element.getClass().getName() + " from http client instance.");
-          iter.remove();
-        }
-      }
-    }
-
-    return builder.build();
-  }
-
   /**
    * Sets a new list of interceptors for the specified {@link OkHttpClient} instance by removing any interceptors
    * that implement "interfaceToRemove".
@@ -370,7 +372,7 @@ private OkHttpClient reconfigureClientInterceptors(OkHttpClient client, String i
    * @return the new {@link OkHttpClient} instance with the updated list of interceptors
    */
   private OkHttpClient reconfigureClientInterceptors(OkHttpClient client,
-      Class<? extends Interceptor> interfaceToRemove) {
+    Class<? extends Interceptor> interfaceToRemove) {
     OkHttpClient.Builder builder = client.newBuilder();
 
     if (!builder.interceptors().isEmpty()) {
@@ -395,6 +397,7 @@ private OkHttpClient reconfigureClientInterceptors(OkHttpClient client,
   public OkHttpClient createHttpClient() {
     Builder builder = okHttpClient.newBuilder();
     addCookieJar(builder);
+    addRedirectInterceptor(builder);
     return builder.build();
   }
 
@@ -419,7 +422,10 @@ public OkHttpClient configureClient(HttpConfigOptions options) {
    * @return a new {@link OkHttpClient} instance with the specified options applied
    */
   public OkHttpClient configureClient(OkHttpClient client, HttpConfigOptions options) {
+    Boolean customRedirects = null;
     if (options != null) {
+      customRedirects = options.getCustomRedirects();
+
       if (options.shouldDisableSslVerification()) {
         client = disableSslVerification(client);
       }
@@ -443,6 +449,7 @@ public OkHttpClient configureClient(OkHttpClient client, HttpConfigOptions optio
                   options.getAuthenticator());
           if (retryInterceptor != null) {
             client = client.newBuilder().addInterceptor(retryInterceptor).build();
+            LOG.log(Level.FINE, "Registered the retry interceptor.");
           } else {
             LOG.log(Level.WARNING,
                 "The retry interceptor factory returned a null IRetryInterceptor instance. Retries are disabled.");
@@ -453,14 +460,35 @@ public OkHttpClient configureClient(OkHttpClient client, HttpConfigOptions optio
       // Configure the GZIP interceptor.
       Boolean enableGzip = options.getGzipCompression();
       if (enableGzip != null) {
-        client = reconfigureClientInterceptors(client, "GzipRequestInterceptor");
+        client = reconfigureClientInterceptors(client, GzipRequestInterceptor.class);
         if (enableGzip.booleanValue()) {
           client = client.newBuilder()
                   .addInterceptor(new GzipRequestInterceptor())
                   .build();
+          LOG.log(Level.FINE, "Registered the gzip interceptor.");
         }
       }
     }
+
+    // Configure the redirect interceptor.
+    // We want "custom redirects" to be enabled by default (including a scenario
+    // where "options" is null), hence the need to handle this feature outside the "if" above.
+    // We want to register the interceptor if:
+    // 1. options == null
+    // 2. options != null and "custom redirects" is explicitly set to Boolean.TRUE
+    //
+    // We also want to allow "custom redirects" to be bypassed by setting an environment variable.
+    // This may be only temporary until we are comfortable with this new function, but I'm
+    // adding support for this environment variable as a quick fix in case there are "unforeseen
+    // consequences" after this feature hits the street.
+    // Example:
+    //     export IBMCLOUD_BYPASS_CUSTOM_REDIRECTS=true
+    // Setting this environment variable will revert back to using the okhttp builtin support for redirects.
+    client = reconfigureClientInterceptors(client, RedirectInterceptor.class);
+    if (customRedirects == null || customRedirects.booleanValue()) {
+        client = addRedirectInterceptor(client.newBuilder()).build();
+    }
+
     return client;
   }
 }

src/main/java/com/ibm/cloud/sdk/core/http/HttpConfigOptions.java

@@ -39,6 +39,7 @@ public enum LoggingLevel {
   private Boolean enableRetries;
   private int maxRetries;
   private int maxRetryInterval;
+  private Boolean enableCustomRedirects = Boolean.TRUE;
   private Proxy proxy;
   private Authenticator proxyAuthenticator;
   private LoggingLevel loggingLevel;
@@ -66,6 +67,10 @@ public int getMaxRetryInterval() {
     return this.maxRetryInterval;
   }
 
+  public Boolean getCustomRedirects() {
+      return this.enableCustomRedirects;
+  }
+
   public Proxy getProxy() {
     return this.proxy;
   }
@@ -98,6 +103,7 @@ public static class Builder {
     private Boolean enableRetries;
     private int maxRetries;
     private int maxRetryInterval;
+    private Boolean enableCustomRedirects = Boolean.TRUE;
     private Proxy proxy;
     private Authenticator proxyAuthenticator;
     private LoggingLevel loggingLevel;
@@ -122,10 +128,10 @@ public Builder disableSslVerification(boolean disableSslVerification) {
     }
 
     /**
-     * Sets flag to enable gzip compression of request bodies during HTTP requests. This should ONLY be used if truly
+     * Sets flag to enable compression of request bodies during HTTP requests. This should ONLY be used if truly
      * intended, as many webservers can't handle this.
      *
-     * @param enableGzipCompression whether to disable SSL verification or not
+     * @param enableGzipCompression whether to perform gzip-compression of request bodies or not
      * @return the builder
      */
     public Builder enableGzipCompression(Boolean enableGzipCompression) {
@@ -178,6 +184,17 @@ public Builder disableRetries() {
       return this;
     }
 
+    /**
+     * Sets flag to enable our custom redirect behavior.
+     *
+     * @param enableCustomRedirects whether to enable our custom redirect behavior or not
+     * @return the builder
+     */
+    public Builder enableCustomRedirects(Boolean enableCustomRedirects) {
+      this.enableCustomRedirects = enableCustomRedirects;
+      return this;
+    }
+
     /**
      * Sets HTTP proxy to be used by connections with the current client.
      *
@@ -218,6 +235,7 @@ private HttpConfigOptions(Builder builder) {
     this.enableRetries = builder.enableRetries;
     this.maxRetries = builder.maxRetries;
     this.maxRetryInterval = builder.maxRetryInterval;
+    this.enableCustomRedirects = builder.enableCustomRedirects;
     this.proxy = builder.proxy;
     this.proxyAuthenticator = builder.proxyAuthenticator;
     this.loggingLevel = builder.loggingLevel;