From 977409511ab8e61bd0bfb3a54847b1ba625ccd13 Mon Sep 17 00:00:00 2001
From: Ricki Hirner <hirner@bitfire.at>
Date: Tue, 21 Jun 2016 20:51:52 +0200
Subject: [PATCH] Handle cookies correctly using a name/domain/path MultiKeyMap

---
 .../bitfire/davdroid/MemoryCookieStore.java   | 43 +++++++++++++++----
 .../at/bitfire/davdroid/HttpClientTest.java   | 21 ++++++---
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/app/src/main/java/at/bitfire/davdroid/MemoryCookieStore.java b/app/src/main/java/at/bitfire/davdroid/MemoryCookieStore.java
index 94603657..0f97faeb 100644
--- a/app/src/main/java/at/bitfire/davdroid/MemoryCookieStore.java
+++ b/app/src/main/java/at/bitfire/davdroid/MemoryCookieStore.java
@@ -8,10 +8,13 @@
 
 package at.bitfire.davdroid;
 
-import java.util.Collections;
+import org.apache.commons.collections4.MapIterator;
+import org.apache.commons.collections4.keyvalue.MultiKey;
+import org.apache.commons.collections4.map.HashedMap;
+import org.apache.commons.collections4.map.MultiKeyMap;
+
+import java.util.LinkedList;
 import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 
 import okhttp3.Cookie;
 import okhttp3.CookieJar;
@@ -25,20 +28,42 @@ public class MemoryCookieStore implements CookieJar {
 
     public static final MemoryCookieStore INSTANCE = new MemoryCookieStore();
 
-    protected final Map<HttpUrl, List<Cookie>> store = new ConcurrentHashMap<>();
-
+    /**
+     * Stored cookies. The multi-key consists of three parts: name, domain, and path.
+     * This ensures that cookies can be overwritten. [RFC 6265 5.3 Storage Model]
+     * Not thread-safe!
+     */
+    protected final MultiKeyMap<String, Cookie> storage = MultiKeyMap.multiKeyMap(new HashedMap<MultiKey<? extends String>, Cookie>());
 
     @Override
     public void saveFromResponse(HttpUrl url, List<Cookie> cookies) {
-        store.put(url, cookies);
+        synchronized(storage) {
+            for (Cookie cookie : cookies)
+                storage.put(cookie.name(), cookie.domain(), cookie.path(), cookie);
+        }
     }
 
     @Override
     public List<Cookie> loadForRequest(HttpUrl url) {
-        List<Cookie> cookies = store.get(url);
+        List<Cookie> cookies = new LinkedList<>();
 
-        if (cookies == null)
-            cookies = Collections.emptyList();
+        synchronized(storage) {
+            MapIterator<MultiKey<? extends String>, Cookie> iter = storage.mapIterator();
+            while (iter.hasNext()) {
+                iter.next();
+                Cookie cookie = iter.getValue();
+
+                // remove expired cookies
+                if (cookie.expiresAt() <= System.currentTimeMillis()) {
+                    iter.remove();
+                    continue;
+                }
+
+                // add applicable cookies
+                if (cookie.matches(url))
+                    cookies.add(cookie);
+            }
+        }
 
         return cookies;
     }
diff --git a/app/src/test/java/at/bitfire/davdroid/HttpClientTest.java b/app/src/test/java/at/bitfire/davdroid/HttpClientTest.java
index c80b7614..7a7ea638 100644
--- a/app/src/test/java/at/bitfire/davdroid/HttpClientTest.java
+++ b/app/src/test/java/at/bitfire/davdroid/HttpClientTest.java
@@ -38,12 +38,13 @@ public class HttpClientTest extends TestCase {
     }
 
     public void testCookies() throws IOException, InterruptedException, URISyntaxException {
-        HttpUrl url = server.url("/");
+        HttpUrl url = server.url("/test");
 
-        // set cookie in first response
+        // set cookie for root path (/) and /test path in first response
         server.enqueue(new MockResponse()
                 .setResponseCode(200)
-                .setHeader("Set-Cookie", "theme=light; path=/")
+                .addHeader("Set-Cookie", "cookie1=1; path=/")
+                .addHeader("Set-Cookie", "cookie2=2")
                 .setBody("Cookie set"));
         httpClient.newCall(new Request.Builder()
                 .get().url(url)
@@ -51,14 +52,22 @@ public class HttpClientTest extends TestCase {
         assertNull(server.takeRequest().getHeader("Cookie"));
 
         // cookie should be sent with second request
+        // second response lets first cookie expire and overwrites second cookie
+        server.enqueue(new MockResponse()
+                .addHeader("Set-Cookie", "cookie1=1a; path=/; Max-Age=0")
+                .addHeader("Set-Cookie", "cookie2=2a")
+                .setResponseCode(200));
+        httpClient.newCall(new Request.Builder()
+                .get().url(url)
+                .build()).execute();
+        assertEquals("cookie2=2; cookie1=1", server.takeRequest().getHeader("Cookie"));
+
         server.enqueue(new MockResponse()
                 .setResponseCode(200));
         httpClient.newCall(new Request.Builder()
                 .get().url(url)
                 .build()).execute();
-        //assertEquals("theme=light", server.takeRequest().getHeader("Cookie"));
-
-        // doesn't work for URLs with ports, see https://code.google.com/p/android/issues/detail?id=193475
+        assertEquals("cookie2=2a", server.takeRequest().getHeader("Cookie"));
     }
 
 }