From fbbc6942cf2f4f31d74e5cc35a0812f9174cd0f6 Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Wed, 5 Mar 2014 17:23:51 +0000 Subject: [PATCH] Removing requestLine from HttpURLConnection.getRequestProperties() This was breaking an Android test and appears not to be part of the spec for HttpURLConnection. To fix the test fully another change was required to add additional headers passed to the CookieHandler. Also added are some tests ported over from Android that test CookieHandler/CookieManager and improve coverage of this area. --- .../okhttp/internal/http/CookiesTest.java | 351 ++++++++++++++++++ .../okhttp/internal/http/HttpEngine.java | 10 +- .../internal/http/HttpURLConnectionImpl.java | 5 +- 3 files changed, 360 insertions(+), 6 deletions(-) create mode 100644 okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/CookiesTest.java diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/CookiesTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/CookiesTest.java new file mode 100644 index 000000000..a44e6839f --- /dev/null +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/CookiesTest.java @@ -0,0 +1,351 @@ +/* + * Copyright (C) 2010 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.squareup.okhttp.internal.http; + +import com.squareup.okhttp.OkHttpClient; +import com.squareup.okhttp.mockwebserver.MockResponse; +import com.squareup.okhttp.mockwebserver.MockWebServer; +import com.squareup.okhttp.mockwebserver.RecordedRequest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; +import java.net.CookieHandler; +import java.net.CookieManager; +import java.net.HttpCookie; +import java.net.HttpURLConnection; +import java.net.URI; +import java.net.URLConnection; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static java.net.CookiePolicy.ACCEPT_ORIGINAL_SERVER; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** Android's CookiesTest. */ +public class CookiesTest { + + private OkHttpClient client; + + @Before + public void setUp() throws Exception { + client = new OkHttpClient(); + } + + @After + public void tearDown() throws Exception { + CookieHandler.setDefault(null); + } + + @Test + public void testNetscapeResponse() throws Exception { + CookieManager cookieManager = new CookieManager(null, ACCEPT_ORIGINAL_SERVER); + CookieHandler.setDefault(cookieManager); + MockWebServer server = new MockWebServer(); + server.play(); + + server.enqueue(new MockResponse().addHeader("Set-Cookie: a=android; " + + "expires=Fri, 31-Dec-9999 23:59:59 GMT; " + + "path=/path; " + + "domain=" + server.getCookieDomain() + "; " + + "secure")); + get(server, "/path/foo"); + + List cookies = cookieManager.getCookieStore().getCookies(); + assertEquals(1, cookies.size()); + HttpCookie cookie = cookies.get(0); + assertEquals("a", cookie.getName()); + assertEquals("android", cookie.getValue()); + assertEquals(null, cookie.getComment()); + assertEquals(null, cookie.getCommentURL()); + assertEquals(false, cookie.getDiscard()); + assertEquals(server.getCookieDomain(), cookie.getDomain()); + assertTrue(cookie.getMaxAge() > 100000000000L); + assertEquals("/path", cookie.getPath()); + assertEquals(true, cookie.getSecure()); + assertEquals(0, cookie.getVersion()); + } + + @Test public void testRfc2109Response() throws Exception { + CookieManager cookieManager = new CookieManager(null, ACCEPT_ORIGINAL_SERVER); + CookieHandler.setDefault(cookieManager); + MockWebServer server = new MockWebServer(); + server.play(); + + server.enqueue(new MockResponse().addHeader("Set-Cookie: a=android; " + + "Comment=this cookie is delicious; " + + "Domain=" + server.getCookieDomain() + "; " + + "Max-Age=60; " + + "Path=/path; " + + "Secure; " + + "Version=1")); + get(server, "/path/foo"); + + List cookies = cookieManager.getCookieStore().getCookies(); + assertEquals(1, cookies.size()); + HttpCookie cookie = cookies.get(0); + assertEquals("a", cookie.getName()); + assertEquals("android", cookie.getValue()); + assertEquals("this cookie is delicious", cookie.getComment()); + assertEquals(null, cookie.getCommentURL()); + assertEquals(false, cookie.getDiscard()); + assertEquals(server.getCookieDomain(), cookie.getDomain()); + assertEquals(60, cookie.getMaxAge()); + assertEquals("/path", cookie.getPath()); + assertEquals(true, cookie.getSecure()); + assertEquals(1, cookie.getVersion()); + } + + @Test public void testRfc2965Response() throws Exception { + CookieManager cookieManager = new CookieManager(null, ACCEPT_ORIGINAL_SERVER); + CookieHandler.setDefault(cookieManager); + MockWebServer server = new MockWebServer(); + server.play(); + + server.enqueue(new MockResponse().addHeader("Set-Cookie2: a=android; " + + "Comment=this cookie is delicious; " + + "CommentURL=http://google.com/; " + + "Discard; " + + "Domain=" + server.getCookieDomain() + "; " + + "Max-Age=60; " + + "Path=/path; " + + "Port=\"80,443," + server.getPort() + "\"; " + + "Secure; " + + "Version=1")); + get(server, "/path/foo"); + + List cookies = cookieManager.getCookieStore().getCookies(); + assertEquals(1, cookies.size()); + HttpCookie cookie = cookies.get(0); + assertEquals("a", cookie.getName()); + assertEquals("android", cookie.getValue()); + assertEquals("this cookie is delicious", cookie.getComment()); + assertEquals("http://google.com/", cookie.getCommentURL()); + assertEquals(true, cookie.getDiscard()); + assertEquals(server.getCookieDomain(), cookie.getDomain()); + assertEquals(60, cookie.getMaxAge()); + assertEquals("/path", cookie.getPath()); + assertEquals("80,443," + server.getPort(), cookie.getPortlist()); + assertEquals(true, cookie.getSecure()); + assertEquals(1, cookie.getVersion()); + } + + @Test public void testQuotedAttributeValues() throws Exception { + CookieManager cookieManager = new CookieManager(null, ACCEPT_ORIGINAL_SERVER); + CookieHandler.setDefault(cookieManager); + MockWebServer server = new MockWebServer(); + server.play(); + + server.enqueue(new MockResponse().addHeader("Set-Cookie2: a=\"android\"; " + + "Comment=\"this cookie is delicious\"; " + + "CommentURL=\"http://google.com/\"; " + + "Discard; " + + "Domain=\"" + server.getCookieDomain() + "\"; " + + "Max-Age=\"60\"; " + + "Path=\"/path\"; " + + "Port=\"80,443," + server.getPort() + "\"; " + + "Secure; " + + "Version=\"1\"")); + get(server, "/path/foo"); + + List cookies = cookieManager.getCookieStore().getCookies(); + assertEquals(1, cookies.size()); + HttpCookie cookie = cookies.get(0); + assertEquals("a", cookie.getName()); + assertEquals("android", cookie.getValue()); + assertEquals("this cookie is delicious", cookie.getComment()); + assertEquals("http://google.com/", cookie.getCommentURL()); + assertEquals(true, cookie.getDiscard()); + assertEquals(server.getCookieDomain(), cookie.getDomain()); + assertEquals(60, cookie.getMaxAge()); + assertEquals("/path", cookie.getPath()); + assertEquals("80,443," + server.getPort(), cookie.getPortlist()); + assertEquals(true, cookie.getSecure()); + assertEquals(1, cookie.getVersion()); + } + + @Test public void testSendingCookiesFromStore() throws Exception { + MockWebServer server = new MockWebServer(); + server.enqueue(new MockResponse()); + server.play(); + + CookieManager cookieManager = new CookieManager(null, ACCEPT_ORIGINAL_SERVER); + HttpCookie cookieA = new HttpCookie("a", "android"); + cookieA.setDomain(server.getCookieDomain()); + cookieA.setPath("/"); + cookieManager.getCookieStore().add(server.getUrl("/").toURI(), cookieA); + HttpCookie cookieB = new HttpCookie("b", "banana"); + cookieB.setDomain(server.getCookieDomain()); + cookieB.setPath("/"); + cookieManager.getCookieStore().add(server.getUrl("/").toURI(), cookieB); + CookieHandler.setDefault(cookieManager); + + get(server, "/"); + RecordedRequest request = server.takeRequest(); + + List receivedHeaders = request.getHeaders(); + assertContains(receivedHeaders, "Cookie: $Version=\"1\"; " + + "a=\"android\";$Path=\"/\";$Domain=\"" + server.getCookieDomain() + "\"; " + + "b=\"banana\";$Path=\"/\";$Domain=\"" + server.getCookieDomain() + "\""); + } + + @Test public void testRedirectsDoNotIncludeTooManyCookies() throws Exception { + MockWebServer redirectTarget = new MockWebServer(); + redirectTarget.enqueue(new MockResponse().setBody("A")); + redirectTarget.play(); + + MockWebServer redirectSource = new MockWebServer(); + redirectSource.enqueue(new MockResponse() + .setResponseCode(HttpURLConnection.HTTP_MOVED_TEMP) + .addHeader("Location: " + redirectTarget.getUrl("/"))); + redirectSource.play(); + + CookieManager cookieManager = new CookieManager(null, ACCEPT_ORIGINAL_SERVER); + HttpCookie cookie = new HttpCookie("c", "cookie"); + cookie.setDomain(redirectSource.getCookieDomain()); + cookie.setPath("/"); + String portList = Integer.toString(redirectSource.getPort()); + cookie.setPortlist(portList); + cookieManager.getCookieStore().add(redirectSource.getUrl("/").toURI(), cookie); + CookieHandler.setDefault(cookieManager); + + get(redirectSource, "/"); + RecordedRequest request = redirectSource.takeRequest(); + + assertContains(request.getHeaders(), "Cookie: $Version=\"1\"; " + + "c=\"cookie\";$Path=\"/\";$Domain=\"" + redirectSource.getCookieDomain() + + "\";$Port=\"" + portList + "\""); + + for (String header : redirectTarget.takeRequest().getHeaders()) { + if (header.startsWith("Cookie")) { + fail(header); + } + } + } + + /** + * Test which headers show up where. The cookie manager should be notified + * of both user-specified and derived headers like {@code Host}. Headers + * named {@code Cookie} or {@code Cookie2} that are returned by the cookie + * manager should show up in the request and in {@code + * getRequestProperties}. + */ + @Test public void testHeadersSentToCookieHandler() throws IOException, InterruptedException { + final Map> cookieHandlerHeaders = new HashMap>(); + CookieHandler.setDefault(new CookieManager() { + @Override + public Map> get(URI uri, + Map> requestHeaders) throws IOException { + cookieHandlerHeaders.putAll(requestHeaders); + Map> result = new HashMap>(); + result.put("Cookie", Collections.singletonList("Bar=bar")); + result.put("Cookie2", Collections.singletonList("Baz=baz")); + result.put("Quux", Collections.singletonList("quux")); + return result; + } + }); + MockWebServer server = new MockWebServer(); + server.enqueue(new MockResponse()); + server.play(); + + HttpURLConnection connection = client.open(server.getUrl("/")); + assertEquals(Collections.>emptyMap(), + connection.getRequestProperties()); + + connection.setRequestProperty("Foo", "foo"); + connection.setDoOutput(true); + connection.getOutputStream().write(5); + connection.getOutputStream().close(); + connection.getInputStream().close(); + + RecordedRequest request = server.takeRequest(); + + assertContainsAll(cookieHandlerHeaders.keySet(), "Foo"); + assertContainsAll(cookieHandlerHeaders.keySet(), + "Content-type", "User-Agent", "Connection", "Host"); + assertFalse(cookieHandlerHeaders.containsKey("Cookie")); + + /* + * The API specifies that calling getRequestProperties() on a connected instance should fail + * with an IllegalStateException, but the RI violates the spec and returns a valid map. + * http://www.mail-archive.com/net-dev@openjdk.java.net/msg01768.html + */ + try { + assertContainsAll(connection.getRequestProperties().keySet(), "Foo"); + assertContainsAll(connection.getRequestProperties().keySet(), + "Content-type", "Content-Length", "User-Agent", "Connection", "Host"); + assertContainsAll(connection.getRequestProperties().keySet(), "Cookie", "Cookie2"); + assertFalse(connection.getRequestProperties().containsKey("Quux")); + } catch (IllegalStateException expected) { + } + + assertContainsAll(request.getHeaders(), "Foo: foo", "Cookie: Bar=bar", "Cookie2: Baz=baz"); + assertFalse(request.getHeaders().contains("Quux: quux")); + } + + @Test public void testCookiesSentIgnoresCase() throws Exception { + CookieHandler.setDefault(new CookieManager() { + @Override public Map> get(URI uri, + Map> requestHeaders) throws IOException { + Map> result = new HashMap>(); + result.put("COOKIE", Collections.singletonList("Bar=bar")); + result.put("cooKIE2", Collections.singletonList("Baz=baz")); + return result; + } + }); + MockWebServer server = new MockWebServer(); + server. enqueue(new MockResponse()); + server.play(); + + get(server, "/"); + + RecordedRequest request = server.takeRequest(); + assertContainsAll(request.getHeaders(), "COOKIE: Bar=bar", "cooKIE2: Baz=baz"); + assertFalse(request.getHeaders().contains("Quux: quux")); + } + + private void assertContains(Collection collection, String element) { + for (String c : collection) { + if (c != null && c.equalsIgnoreCase(element)) { + return; + } + } + fail("No " + element + " in " + collection); + } + + private void assertContainsAll(Collection collection, String... toFind) { + for (String s : toFind) { + assertContains(collection, s); + } + } + + private Map> get(MockWebServer server, String path) throws Exception { + URLConnection connection = client.open(server.getUrl(path)); + Map> headers = connection.getHeaderFields(); + connection.getInputStream().close(); + return headers; + } + +} diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java index 1df047cf5..89a3991a4 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java @@ -495,8 +495,14 @@ public class HttpEngine { CookieHandler cookieHandler = client.getCookieHandler(); if (cookieHandler != null) { - Map> cookies = cookieHandler.get( - request.uri(), OkHeaders.toMultimap(request.getHeaders(), null)); + // Capture the request headers added so far so that they can be offered to the CookieHandler. + // This is mostly to stay close to the RI; it is unlikely any of the headers above would + // affect cookie choice besides "Host". + Map> headers = OkHeaders.toMultimap(result.build().headers(), null); + + Map> cookies = cookieHandler.get(request.uri(), headers); + + // Add any new cookies to the request. OkHeaders.addCookies(result, cookies); } diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java index da876cac4..f1a37cbf4 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java @@ -179,10 +179,7 @@ public class HttpURLConnectionImpl extends HttpURLConnection { "Cannot access request header fields after connection is set"); } - // For the request line property assigned to the null key, just use no proxy and HTTP 1.1. - Request request = new Request.Builder().url(getURL()).method(method, null).build(); - String requestLine = RequestLine.get(request, null, 1); - return OkHeaders.toMultimap(requestHeaders.build(), requestLine); + return OkHeaders.toMultimap(requestHeaders.build(), null); } @Override public final InputStream getInputStream() throws IOException {