Merge pull request.

#421 Modify PR to ensure that the new implementation is thread safe. Can't share a rest template that is swapping request factory between threads. [#127482707] https://www.pivotaltracker.com/story/show/127482707 Reviewed By: Filip Hanik <fhanik@pivotal.io>
cloudfoundry · Oct 12, 2016 · 871e925 · 871e925
1 parent d8571a2
commit 871e925
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 19 deletions.
diff --git a/...c/main/java/org/cloudfoundry/identity/uaa/provider/oauth/XOAuthAuthenticationManager.java b/...c/main/java/org/cloudfoundry/identity/uaa/provider/oauth/XOAuthAuthenticationManager.java
@@ -66,11 +66,10 @@
 import static org.cloudfoundry.identity.uaa.provider.ExternalIdentityProviderDefinition.GROUP_ATTRIBUTE_NAME;
 import static org.cloudfoundry.identity.uaa.provider.ExternalIdentityProviderDefinition.USER_NAME_ATTRIBUTE_NAME;
 import static org.cloudfoundry.identity.uaa.util.TokenValidation.validate;
-import static org.cloudfoundry.identity.uaa.util.UaaHttpRequestUtils.getNoValidatingClientHttpRequestFactory;
+import static org.cloudfoundry.identity.uaa.util.UaaHttpRequestUtils.createRequestFactory;
 
 public class XOAuthAuthenticationManager extends ExternalLoginAuthenticationManager<XOAuthAuthenticationManager.AuthenticationData> {
 
-    private RestTemplate restTemplate = new RestTemplate();
     private IdentityProviderProvisioning providerProvisioning;
 
     public XOAuthAuthenticationManager(IdentityProviderProvisioning providerProvisioning) {
@@ -259,10 +258,42 @@ protected boolean isAcceptedInvitationAuthentication() {
         return false;
     }
 
-    public RestTemplate getRestTemplate() {
-        return restTemplate;
+    /*
+     * BEGIN
+     * The following thread local only exists to satisfy that the unit test
+     * that require the template to be bound to the mock server
+     */
+    public static class RestTemplateHolder {
+        private final RestTemplate skipSslValidationTemplate;
+        private final RestTemplate restTemplate;
+
+        public RestTemplateHolder() {
+            skipSslValidationTemplate = new RestTemplate(createRequestFactory(true));
+            restTemplate = new RestTemplate(createRequestFactory(false));
+        }
+
+        public RestTemplate getRestTemplate(boolean skipSslValidation) {
+            return skipSslValidation ? skipSslValidationTemplate : restTemplate;
+        }
     }
 
+    private ThreadLocal<RestTemplateHolder> restTemplateHolder = new ThreadLocal<RestTemplateHolder>() {
+        @Override
+        protected RestTemplateHolder initialValue() {
+            return new RestTemplateHolder();
+        }
+    };
+
+    public RestTemplate getRestTemplate(AbstractXOAuthIdentityProviderDefinition config) {
+        return restTemplateHolder.get().getRestTemplate(config.isSkipSslValidation());
+    }
+
+    /*
+     * END
+     * The following thread local only exists to satisfy that the unit test
+     * that require the template to be bound to the mock server
+     */
+
     private String getResponseType(AbstractXOAuthIdentityProviderDefinition config) {
         if (RawXOAuthIdentityProviderDefinition.class.isAssignableFrom(config.getClass())) {
             return "token";
@@ -301,7 +332,7 @@ private String getTokenKeyFromOAuth(AbstractXOAuthIdentityProviderDefinition con
         headers.add("Authorization", getClientAuthHeader(config));
         headers.add("Accept", "application/json");
         HttpEntity tokenKeyRequest = new HttpEntity<>(null, headers);
-        ResponseEntity<Map<String, Object>> responseEntity = restTemplate.exchange(tokenKeyUrl, HttpMethod.GET, tokenKeyRequest, new ParameterizedTypeReference<Map<String, Object>>() {});
+        ResponseEntity<Map<String, Object>> responseEntity = getRestTemplate(config).exchange(tokenKeyUrl, HttpMethod.GET, tokenKeyRequest, new ParameterizedTypeReference<Map<String, Object>>() {});
         return (String) responseEntity.getBody().get("value");
     }
 
@@ -326,12 +357,17 @@ private String getTokenFromCode(XOAuthCodeToken codeToken, AbstractXOAuthIdentit
         }
 
         try {
-            if (config.isSkipSslValidation()) {
-                restTemplate.setRequestFactory(getNoValidatingClientHttpRequestFactory());
-            }
-            ResponseEntity<Map<String, String>> responseEntity = restTemplate.exchange(requestUri, HttpMethod.POST, requestEntity, new ParameterizedTypeReference<Map<String, String>>() {});
+            // A configuration that skips SSL/TLS validation requires clobbering the rest template request factory
+            // setup by the bean initializer.
+            ResponseEntity<Map<String, String>> responseEntity =
+                getRestTemplate(config)
+                    .exchange(requestUri,
+                              HttpMethod.POST,
+                              requestEntity,
+                              new ParameterizedTypeReference<Map<String, String>>() {}
+                    );
             return responseEntity.getBody().get(ID_TOKEN);
-        } catch (HttpServerErrorException|HttpClientErrorException ex) {
+        } catch (HttpServerErrorException | HttpClientErrorException ex) {
             throw ex;
         }
     }

diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaHttpRequestUtils.java b/server/src/main/java/org/cloudfoundry/identity/uaa/util/UaaHttpRequestUtils.java
@@ -21,8 +21,21 @@
 
 public abstract class UaaHttpRequestUtils {
 
+    public static ClientHttpRequestFactory createRequestFactory(boolean skipSslValidation) {
+        ClientHttpRequestFactory clientHttpRequestFactory;
+        if (skipSslValidation) {
+            clientHttpRequestFactory = getNoValidatingClientHttpRequestFactory();
+        } else {
+            clientHttpRequestFactory = getClientHttpRequestFactory();
+        }
+        return clientHttpRequestFactory;
+    }
+
+    public static ClientHttpRequestFactory createRequestFactory() {
+        return createRequestFactory(false);
+    }
+
     public static ClientHttpRequestFactory getNoValidatingClientHttpRequestFactory() {
-        ClientHttpRequestFactory requestFactory;
         SSLContext sslContext;
         try {
             sslContext = new SSLContextBuilder().loadTrustMaterial(null, new TrustSelfSignedStrategy()).build();
@@ -33,13 +46,15 @@ public static ClientHttpRequestFactory getNoValidatingClientHttpRequestFactory()
         } catch (KeyStoreException e) {
             throw new RuntimeException(e);
         }
-        //
+        // Build the HTTP client from the system properties so that it uses the system proxy settings.
         CloseableHttpClient httpClient =
-                HttpClients.custom()
-                        .setSslcontext(sslContext)
-                        .setRedirectStrategy(new DefaultRedirectStrategy()).build();
+            HttpClients.custom()
+                .useSystemProperties()
+                .setSslcontext(sslContext)
+                .setRedirectStrategy(new DefaultRedirectStrategy())
+                .build();
 
-        requestFactory = new HttpComponentsClientHttpRequestFactory(httpClient);
+        ClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(httpClient);
         return requestFactory;
     }
 
@@ -56,4 +71,13 @@ private static String encodeParameter(String value) {
             throw new RuntimeException(e);
         }
     }
+
+    public static ClientHttpRequestFactory getClientHttpRequestFactory() {
+        CloseableHttpClient httpClient =
+            HttpClients.custom()
+                .useSystemProperties()
+                .setRedirectStrategy(new DefaultRedirectStrategy())
+                .build();
+        return new HttpComponentsClientHttpRequestFactory(httpClient);
+    }
 }
diff --git a/...st/java/org/cloudfoundry/identity/uaa/provider/oauth/XOAuthAuthenticationManagerTest.java b/...st/java/org/cloudfoundry/identity/uaa/provider/oauth/XOAuthAuthenticationManagerTest.java
@@ -147,7 +147,6 @@ public void setUp() throws Exception {
         xoAuthAuthenticationManager = new XOAuthAuthenticationManager(provisioning);
         xoAuthAuthenticationManager.setUserDatabase(userDatabase);
         xoAuthAuthenticationManager.setApplicationEventPublisher(publisher);
-        mockUaaServer = MockRestServiceServer.createServer(xoAuthAuthenticationManager.getRestTemplate());
         xCodeToken = new XOAuthCodeToken(CODE, ORIGIN, "http://localhost/callback/the_origin");
         claims = map(
             entry("sub", "12345"),
@@ -187,6 +186,9 @@ public void setUp() throws Exception {
                     "MFswDQYJKoZIhvcNAQEBBQADSgAwRwJAcjAgsHEfrUxeTFwQPb17AkZ2Im4SfZdp\n" +
                     "Y8Ada9pZfxXz1PZSqv9TPTMAzNx+EkzMk2IMYN+uNm1bfDzaxVdz+QIDAQAB\n" +
                     "-----END PUBLIC KEY-----");
+
+        mockUaaServer = MockRestServiceServer.createServer(xoAuthAuthenticationManager.getRestTemplate(config));
+
     }
 
     @Test

diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaHttpRequestUtilsTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/util/UaaHttpRequestUtilsTest.java
@@ -1,14 +1,17 @@
 package org.cloudfoundry.identity.uaa.util;
 
 import org.cloudfoundry.identity.uaa.ServerRunning;
-import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
+import org.springframework.web.client.RestClientException;
 import org.springframework.web.client.RestTemplate;
 
+import javax.net.ssl.SSLHandshakeException;
 import java.util.Map;
 
 import static org.cloudfoundry.identity.uaa.util.UaaHttpRequestUtils.getNoValidatingClientHttpRequestFactory;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 import static org.springframework.http.HttpStatus.OK;
 
 public class UaaHttpRequestUtilsTest {
@@ -20,6 +23,21 @@ public class UaaHttpRequestUtilsTest {
     public void skipSslValidation() {
         RestTemplate restTemplate = new RestTemplate();
         restTemplate.setRequestFactory(getNoValidatingClientHttpRequestFactory());
-        Assert.assertEquals(OK, restTemplate.getForEntity("https://login.identity.cf-app.com/info", Map.class).getStatusCode());
+        assertEquals(OK, restTemplate.getForEntity("https://login.identity.cf-app.com/info", Map.class).getStatusCode());
+
+        restTemplate.setRequestFactory(UaaHttpRequestUtils.createRequestFactory(true));
+        assertEquals(OK, restTemplate.getForEntity("https://login.identity.cf-app.com/info", Map.class).getStatusCode());
+    }
+
+
+    @Test
+    public void trustedOnly() {
+        RestTemplate restTemplate = new RestTemplate();
+        try {
+            restTemplate.getForEntity("https://login.identity.cf-app.com/info", Map.class);
+            fail("We should not reach this step if the above URL is using a self signed certificate");
+        } catch (RestClientException e) {
+            assertEquals(SSLHandshakeException.class, e.getCause().getClass());
+        }
     }
 }