From 3d2547f18886e771aaa9baa996a21136c93460c4 Mon Sep 17 00:00:00 2001 From: jwilson Date: Sat, 22 Dec 2012 13:34:22 -0500 Subject: [PATCH] Restore tests for response caching + TLS. One of the test cases bitrotted to failure as a consequence of the HTTP route selector change. I stopped assigning the socket, which it needed to cache the TLS metadata. This is fixed. --- .../internal/net/http/HttpConnectionPool.java | 7 +- .../okhttp/internal/net/http/HttpEngine.java | 8 + .../net/http/HttpsURLConnectionImpl.java | 13 +- .../net/http/HttpResponseCacheTest.java | 241 ++++++++++-------- 4 files changed, 151 insertions(+), 118 deletions(-) diff --git a/src/main/java/com/squareup/okhttp/internal/net/http/HttpConnectionPool.java b/src/main/java/com/squareup/okhttp/internal/net/http/HttpConnectionPool.java index 9e784b917..08a0828a3 100644 --- a/src/main/java/com/squareup/okhttp/internal/net/http/HttpConnectionPool.java +++ b/src/main/java/com/squareup/okhttp/internal/net/http/HttpConnectionPool.java @@ -18,7 +18,6 @@ package com.squareup.okhttp.internal.net.http; import com.squareup.okhttp.internal.util.Libcore; -import java.net.Socket; import java.net.SocketException; import java.util.ArrayList; import java.util.HashMap; @@ -73,8 +72,7 @@ final class HttpConnectionPool { } if (connection.isEligibleForRecycling()) { // Since Socket is recycled, re-tag before using - Socket socket = connection.getSocket(); - Libcore.tagSocket(socket); + Libcore.tagSocket(connection.getSocket()); return connection; } } @@ -91,9 +89,8 @@ final class HttpConnectionPool { throw new IllegalArgumentException(); // TODO: just 'return' here? } - Socket socket = connection.getSocket(); try { - Libcore.untagSocket(socket); + Libcore.untagSocket(connection.getSocket()); } catch (SocketException e) { // When unable to remove tagging, skip recycling and close Libcore.logW("Unable to untagSocket(): " + e); diff --git a/src/main/java/com/squareup/okhttp/internal/net/http/HttpEngine.java b/src/main/java/com/squareup/okhttp/internal/net/http/HttpEngine.java index 727575d2d..a4809fffa 100644 --- a/src/main/java/com/squareup/okhttp/internal/net/http/HttpEngine.java +++ b/src/main/java/com/squareup/okhttp/internal/net/http/HttpEngine.java @@ -287,6 +287,7 @@ public class HttpEngine { connection.connect(policy.getConnectTimeout(), policy.getReadTimeout(), getTunnelConfig()); } + connected(connection); Proxy proxy = connection.getProxy(); if (proxy != null) { policy.setProxy(proxy); @@ -295,6 +296,13 @@ public class HttpEngine { } } + /** + * Called after a socket connection has been created or retrieved from the + * pool. Subclasses use this hook to get a reference to the TLS data. + */ + protected void connected(HttpConnection connection) { + } + /** * @param body the response body, or null if it doesn't exist or isn't * available. diff --git a/src/main/java/com/squareup/okhttp/internal/net/http/HttpsURLConnectionImpl.java b/src/main/java/com/squareup/okhttp/internal/net/http/HttpsURLConnectionImpl.java index 51105e4af..972a096f6 100644 --- a/src/main/java/com/squareup/okhttp/internal/net/http/HttpsURLConnectionImpl.java +++ b/src/main/java/com/squareup/okhttp/internal/net/http/HttpsURLConnectionImpl.java @@ -395,14 +395,9 @@ public final class HttpsURLConnectionImpl extends HttpsURLConnection { } private static final class HttpsEngine extends HttpEngine { - /** - * Local stash of HttpsEngine.connection.sslSocket for answering - * queries such as getCipherSuite even after - * httpsEngine.Connection has been recycled. It's presence is also - * used to tell if the HttpsURLConnection is considered connected, - * as opposed to the connected field of URLConnection or the a - * non-null connect in HttpURLConnectionImpl + * Stash of HttpsEngine.connection.socket to implement requests like + * {@link #getCipherSuite} even after the connection has been recycled. */ private SSLSocket sslSocket; @@ -420,6 +415,10 @@ public final class HttpsURLConnectionImpl extends HttpsURLConnection { this.enclosing = enclosing; } + @Override protected void connected(HttpConnection connection) { + this.sslSocket = (SSLSocket) connection.getSocket(); + } + @Override protected boolean acceptCacheResponseType(CacheResponse cacheResponse) { return cacheResponse instanceof SecureCacheResponse; } diff --git a/src/test/java/com/squareup/okhttp/internal/net/http/HttpResponseCacheTest.java b/src/test/java/com/squareup/okhttp/internal/net/http/HttpResponseCacheTest.java index 828ff9245..ad54daf8e 100644 --- a/src/test/java/com/squareup/okhttp/internal/net/http/HttpResponseCacheTest.java +++ b/src/test/java/com/squareup/okhttp/internal/net/http/HttpResponseCacheTest.java @@ -21,6 +21,7 @@ import com.google.mockwebserver.MockWebServer; import com.google.mockwebserver.RecordedRequest; import static com.google.mockwebserver.SocketPolicy.DISCONNECT_AT_END; import com.squareup.okhttp.OkHttpClient; +import com.squareup.okhttp.internal.net.ssl.SslContextBuilder; import java.io.BufferedReader; import java.io.ByteArrayOutputStream; import java.io.File; @@ -35,12 +36,17 @@ import java.net.CookieHandler; import java.net.CookieManager; import java.net.HttpCookie; import java.net.HttpURLConnection; +import java.net.InetAddress; import java.net.ResponseCache; import java.net.SecureCacheResponse; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; import java.net.URLConnection; +import java.net.UnknownHostException; +import java.security.GeneralSecurityException; +import java.security.Principal; +import java.security.cert.Certificate; import java.text.DateFormat; import java.text.SimpleDateFormat; import java.util.ArrayList; @@ -57,30 +63,47 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.zip.GZIPOutputStream; +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.HttpsURLConnection; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLSession; import junit.framework.TestCase; /** * Android's HttpResponseCacheTest. */ public final class HttpResponseCacheTest extends TestCase { + private static final HostnameVerifier NULL_HOSTNAME_VERIFIER = new HostnameVerifier() { + @Override public boolean verify(String s, SSLSession sslSession) { + return true; + } + }; + private final OkHttpClient client = new OkHttpClient(); private MockWebServer server = new MockWebServer(); private HttpResponseCache cache; -// private final MockOs mockOs = new MockOs(); private final CookieManager cookieManager = new CookieManager(); + private static final SSLContext sslContext; + static { + try { + sslContext = new SslContextBuilder(InetAddress.getLocalHost().getHostName()).build(); + } catch (GeneralSecurityException e) { + throw new RuntimeException(e); + } catch (UnknownHostException e) { + throw new RuntimeException(e); + } + } + @Override protected void setUp() throws Exception { super.setUp(); - String tmp = System.getProperty("java.io.tmpdir"); File cacheDir = new File(tmp, "HttpCache-" + UUID.randomUUID()); cache = new HttpResponseCache(cacheDir, Integer.MAX_VALUE); ResponseCache.setDefault(cache); -// mockOs.install(); CookieHandler.setDefault(cookieManager); } @Override protected void tearDown() throws Exception { -// mockOs.uninstall(); server.shutdown(); ResponseCache.setDefault(null); cache.getCache().delete(); @@ -88,8 +111,8 @@ public final class HttpResponseCacheTest extends TestCase { super.tearDown(); } - private static HttpURLConnection openConnection(URL url) { - return new OkHttpClient().open(url); + private HttpURLConnection openConnection(URL url) { + return client.open(url); } /** @@ -294,58 +317,61 @@ public final class HttpResponseCacheTest extends TestCase { assertEquals(1, cache.getHitCount()); } -// public void testSecureResponseCaching() throws IOException { -// TestSSLContext testSSLContext = TestSSLContext.create(); -// server.useHttps(testSSLContext.serverContext.getSocketFactory(), false); -// server.enqueue(new MockResponse() -// .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS)) -// .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS)) -// .setBody("ABC")); -// server.play(); -// -// HttpsURLConnection connection = (HttpsURLConnection) server.getUrl("/").openConnection(); -// connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// assertEquals("ABC", readAscii(connection)); -// -// // OpenJDK 6 fails on this line, complaining that the connection isn't open yet -// String suite = connection.getCipherSuite(); -// List localCerts = toListOrNull(connection.getLocalCertificates()); -// List serverCerts = toListOrNull(connection.getServerCertificates()); -// Principal peerPrincipal = connection.getPeerPrincipal(); -// Principal localPrincipal = connection.getLocalPrincipal(); -// -// connection = (HttpsURLConnection) server.getUrl("/").openConnection(); // cached! -// connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// assertEquals("ABC", readAscii(connection)); -// -// assertEquals(2, cache.getRequestCount()); -// assertEquals(1, cache.getNetworkCount()); -// assertEquals(1, cache.getHitCount()); -// -// assertEquals(suite, connection.getCipherSuite()); -// assertEquals(localCerts, toListOrNull(connection.getLocalCertificates())); -// assertEquals(serverCerts, toListOrNull(connection.getServerCertificates())); -// assertEquals(peerPrincipal, connection.getPeerPrincipal()); -// assertEquals(localPrincipal, connection.getLocalPrincipal()); -// } -// -// public void testCacheReturnsInsecureResponseForSecureRequest() throws IOException { -// TestSSLContext testSSLContext = TestSSLContext.create(); -// server.useHttps(testSSLContext.serverContext.getSocketFactory(), false); -// server.enqueue(new MockResponse().setBody("ABC")); -// server.enqueue(new MockResponse().setBody("DEF")); -// server.play(); -// -// ResponseCache.setDefault(new InsecureResponseCache()); -// -// HttpsURLConnection connection = (HttpsURLConnection) server.getUrl("/").openConnection(); -// connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// assertEquals("ABC", readAscii(connection)); -// -// connection = (HttpsURLConnection) server.getUrl("/").openConnection(); // not cached! -// connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// assertEquals("DEF", readAscii(connection)); -// } + public void testSecureResponseCaching() throws IOException { + server.useHttps(sslContext.getSocketFactory(), false); + server.enqueue(new MockResponse() + .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS)) + .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS)) + .setBody("ABC")); + server.play(); + + HttpsURLConnection connection = (HttpsURLConnection) client.open(server.getUrl("/")); + connection.setSSLSocketFactory(sslContext.getSocketFactory()); + connection.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + assertEquals("ABC", readAscii(connection)); + + // OpenJDK 6 fails on this line, complaining that the connection isn't open yet + String suite = connection.getCipherSuite(); + List localCerts = toListOrNull(connection.getLocalCertificates()); + List serverCerts = toListOrNull(connection.getServerCertificates()); + Principal peerPrincipal = connection.getPeerPrincipal(); + Principal localPrincipal = connection.getLocalPrincipal(); + + connection = (HttpsURLConnection) client.open(server.getUrl("/")); // cached! + connection.setSSLSocketFactory(sslContext.getSocketFactory()); + connection.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + assertEquals("ABC", readAscii(connection)); + + assertEquals(2, cache.getRequestCount()); + assertEquals(1, cache.getNetworkCount()); + assertEquals(1, cache.getHitCount()); + + assertEquals(suite, connection.getCipherSuite()); + assertEquals(localCerts, toListOrNull(connection.getLocalCertificates())); + assertEquals(serverCerts, toListOrNull(connection.getServerCertificates())); + assertEquals(peerPrincipal, connection.getPeerPrincipal()); + assertEquals(localPrincipal, connection.getLocalPrincipal()); + } + + public void testCacheReturnsInsecureResponseForSecureRequest() throws IOException { + server.useHttps(sslContext.getSocketFactory(), false); + server.enqueue(new MockResponse().setBody("ABC")); + server.enqueue(new MockResponse().setBody("DEF")); + server.play(); + + ResponseCache.setDefault(new InsecureResponseCache()); + + HttpsURLConnection connection1 = (HttpsURLConnection) client.open(server.getUrl("/")); + connection1.setSSLSocketFactory(sslContext.getSocketFactory()); + connection1.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + assertEquals("ABC", readAscii(connection1)); + + // Not cached! + HttpsURLConnection connection2 = (HttpsURLConnection) client.open(server.getUrl("/")); + connection2.setSSLSocketFactory(sslContext.getSocketFactory()); + connection2.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + assertEquals("DEF", readAscii(connection2)); + } public void testResponseCachingAndRedirects() throws Exception { server.enqueue(new MockResponse() @@ -398,32 +424,34 @@ public final class HttpResponseCacheTest extends TestCase { assertEquals(2, request3.getSequenceNumber()); } -// public void testSecureResponseCachingAndRedirects() throws IOException { -// TestSSLContext testSSLContext = TestSSLContext.create(); -// server.useHttps(testSSLContext.serverContext.getSocketFactory(), false); -// server.enqueue(new MockResponse() -// .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS)) -// .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS)) -// .setResponseCode(HttpURLConnection.HTTP_MOVED_PERM) -// .addHeader("Location: /foo")); -// server.enqueue(new MockResponse() -// .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS)) -// .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS)) -// .setBody("ABC")); -// server.enqueue(new MockResponse().setBody("DEF")); -// server.play(); -// -// HttpsURLConnection connection = (HttpsURLConnection) server.getUrl("/").openConnection(); -// connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// assertEquals("ABC", readAscii(connection)); -// -// connection = (HttpsURLConnection) server.getUrl("/").openConnection(); // cached! -// connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// assertEquals("ABC", readAscii(connection)); -// -// assertEquals(4, cache.getRequestCount()); // 2 direct + 2 redirect = 4 -// assertEquals(2, cache.getHitCount()); -// } + public void testSecureResponseCachingAndRedirects() throws IOException { + server.useHttps(sslContext.getSocketFactory(), false); + server.enqueue(new MockResponse() + .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS)) + .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS)) + .setResponseCode(HttpURLConnection.HTTP_MOVED_PERM) + .addHeader("Location: /foo")); + server.enqueue(new MockResponse() + .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS)) + .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS)) + .setBody("ABC")); + server.enqueue(new MockResponse().setBody("DEF")); + server.play(); + + HttpsURLConnection connection1 = (HttpsURLConnection) client.open(server.getUrl("/")); + connection1.setSSLSocketFactory(sslContext.getSocketFactory()); + connection1.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + assertEquals("ABC", readAscii(connection1)); + + // Cached! + HttpsURLConnection connection2 = (HttpsURLConnection) client.open(server.getUrl("/")); + connection1.setSSLSocketFactory(sslContext.getSocketFactory()); + connection1.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + assertEquals("ABC", readAscii(connection2)); + + assertEquals(4, cache.getRequestCount()); // 2 direct + 2 redirect = 4 + assertEquals(2, cache.getHitCount()); + } public void testResponseCacheRequestHeaders() throws IOException, URISyntaxException { server.enqueue(new MockResponse().setBody("ABC")); @@ -1459,28 +1487,29 @@ public final class HttpResponseCacheTest extends TestCase { assertEquals("B", readAscii(openConnection(server.getUrl("/")))); } -// public void testVaryAndHttps() throws Exception { -// TestSSLContext testSSLContext = TestSSLContext.create(); -// server.useHttps(testSSLContext.serverContext.getSocketFactory(), false); -// server.enqueue(new MockResponse() -// .addHeader("Cache-Control: max-age=60") -// .addHeader("Vary: Accept-Language") -// .setBody("A")); -// server.enqueue(new MockResponse().setBody("B")); -// server.play(); -// -// URL url = server.getUrl("/"); -// HttpsURLConnection connection1 = (HttpsURLConnection) url.openConnection(); -// connection1.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// connection1.addRequestProperty("Accept-Language", "en-US"); -// assertEquals("A", readAscii(connection1)); -// -// HttpsURLConnection connection2 = (HttpsURLConnection) url.openConnection(); -// connection2.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// connection2.addRequestProperty("Accept-Language", "en-US"); -// assertEquals("A", readAscii(connection2)); -// } -// + public void testVaryAndHttps() throws Exception { + server.useHttps(sslContext.getSocketFactory(), false); + server.enqueue(new MockResponse() + .addHeader("Cache-Control: max-age=60") + .addHeader("Vary: Accept-Language") + .setBody("A")); + server.enqueue(new MockResponse().setBody("B")); + server.play(); + + URL url = server.getUrl("/"); + HttpsURLConnection connection1 = (HttpsURLConnection) client.open(url); + connection1.setSSLSocketFactory(sslContext.getSocketFactory()); + connection1.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + connection1.addRequestProperty("Accept-Language", "en-US"); + assertEquals("A", readAscii(connection1)); + + HttpsURLConnection connection2 = (HttpsURLConnection) client.open(url); + connection2.setSSLSocketFactory(sslContext.getSocketFactory()); + connection2.setHostnameVerifier(NULL_HOSTNAME_VERIFIER); + connection2.addRequestProperty("Accept-Language", "en-US"); + assertEquals("A", readAscii(connection2)); + } + // public void testDiskWriteFailureCacheDegradation() throws Exception { // Deque writeHandlers = mockOs.getHandlers("write"); // int i = 0;