From 46bf2540337c20fc51686399dd9f64e356f41900 Mon Sep 17 00:00:00 2001 From: falhassen Date: Thu, 26 Oct 2017 06:54:39 -0700 Subject: [PATCH] Add option for handling "Set-Cookie" requests in CronetDataSource. This time, we avoid using the problematic CookieManager and HttpCookie framework APIs by just forwarding the cookie request only when the client has enabled the feature and the server responds with a "Set-Cookie" response header. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=173532023 --- .../ext/cronet/CronetDataSourceTest.java | 143 +++++++++++++++--- .../ext/cronet/CronetDataSource.java | 63 +++++++- 2 files changed, 183 insertions(+), 23 deletions(-) diff --git a/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java b/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java index ea12edaf53..62b5fb261a 100644 --- a/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java +++ b/extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java @@ -103,6 +103,7 @@ public final class CronetDataSourceTest { @Mock private CronetEngine mockCronetEngine; private CronetDataSource dataSourceUnderTest; + private boolean redirectCalled; @Before public void setUp() throws Exception { @@ -119,10 +120,11 @@ public final class CronetDataSourceTest { TEST_READ_TIMEOUT_MS, true, // resetTimeoutOnRedirects mockClock, - null)); + null, + false)); when(mockContentTypePredicate.evaluate(anyString())).thenReturn(true); when(mockCronetEngine.newUrlRequestBuilder( - anyString(), any(UrlRequest.Callback.class), any(Executor.class))) + anyString(), any(UrlRequest.Callback.class), any(Executor.class))) .thenReturn(mockUrlRequestBuilder); when(mockUrlRequestBuilder.allowDirectExecutor()).thenReturn(mockUrlRequestBuilder); when(mockUrlRequestBuilder.build()).thenReturn(mockUrlRequest); @@ -698,6 +700,77 @@ public final class CronetDataSourceTest { assertEquals(1, openExceptions.get()); } + @Test + public void testRedirectParseAndAttachCookie_dataSourceDoesNotHandleSetCookie_followsRedirect() + throws HttpDataSourceException { + mockSingleRedirectSuccess(); + mockFollowRedirectSuccess(); + + testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video"); + + dataSourceUnderTest.open(testDataSpec); + verify(mockUrlRequestBuilder, never()).addHeader(eq("Cookie"), any(String.class)); + verify(mockUrlRequest).followRedirect(); + } + + @Test + public void testRedirectParseAndAttachCookie_dataSourceHandlesSetCookie() + throws HttpDataSourceException { + dataSourceUnderTest = spy( + new CronetDataSource( + mockCronetEngine, + mockExecutor, + mockContentTypePredicate, + mockTransferListener, + TEST_CONNECT_TIMEOUT_MS, + TEST_READ_TIMEOUT_MS, + true, // resetTimeoutOnRedirects + mockClock, + null, + true)); + mockSingleRedirectSuccess(); + + testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video"); + + dataSourceUnderTest.open(testDataSpec); + verify(mockUrlRequestBuilder).addHeader(eq("Cookie"), any(String.class)); + verify(mockUrlRequest, never()).followRedirect(); + verify(mockUrlRequest, times(2)).start(); + } + + @Test + public void testRedirectNoSetCookieFollowsRedirect() throws HttpDataSourceException { + mockSingleRedirectSuccess(); + mockFollowRedirectSuccess(); + + dataSourceUnderTest.open(testDataSpec); + verify(mockUrlRequestBuilder, never()).addHeader(eq("Cookie"), any(String.class)); + verify(mockUrlRequest).followRedirect(); + } + + @Test + public void testRedirectNoSetCookieFollowsRedirect_dataSourceHandlesSetCookie() + throws HttpDataSourceException { + dataSourceUnderTest = spy( + new CronetDataSource( + mockCronetEngine, + mockExecutor, + mockContentTypePredicate, + mockTransferListener, + TEST_CONNECT_TIMEOUT_MS, + TEST_READ_TIMEOUT_MS, + true, // resetTimeoutOnRedirects + mockClock, + null, + true)); + mockSingleRedirectSuccess(); + mockFollowRedirectSuccess(); + + dataSourceUnderTest.open(testDataSpec); + verify(mockUrlRequestBuilder, never()).addHeader(eq("Cookie"), any(String.class)); + verify(mockUrlRequest).followRedirect(); + } + @Test public void testExceptionFromTransferListener() throws HttpDataSourceException { mockResponseStartSuccess(); @@ -812,6 +885,38 @@ public final class CronetDataSourceTest { }).when(mockUrlRequest).start(); } + private void mockSingleRedirectSuccess() { + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + if (!redirectCalled) { + redirectCalled = true; + dataSourceUnderTest.onRedirectReceived( + mockUrlRequest, + createUrlResponseInfoWithUrl("http://example.com/video", 300), + "http://example.com/video/redirect"); + } else { + dataSourceUnderTest.onResponseStarted( + mockUrlRequest, + testUrlResponseInfo); + } + return null; + } + }).when(mockUrlRequest).start(); + } + + private void mockFollowRedirectSuccess() { + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + dataSourceUnderTest.onResponseStarted( + mockUrlRequest, + testUrlResponseInfo); + return null; + } + }).when(mockUrlRequest).followRedirect(); + } + private void mockResponseStartFailure() { doAnswer(new Answer() { @Override @@ -850,16 +955,16 @@ public final class CronetDataSourceTest { private void mockReadFailure() { doAnswer( - new Answer() { - @Override - public Object answer(InvocationOnMock invocation) throws Throwable { - dataSourceUnderTest.onFailed( - mockUrlRequest, - createUrlResponseInfo(500), // statusCode - mockNetworkException); - return null; - } - }) + new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + dataSourceUnderTest.onFailed( + mockUrlRequest, + createUrlResponseInfo(500), // statusCode + mockNetworkException); + return null; + } + }) .when(mockUrlRequest) .read(any(ByteBuffer.class)); } @@ -867,13 +972,13 @@ public final class CronetDataSourceTest { private ConditionVariable buildReadStartedCondition() { final ConditionVariable startedCondition = new ConditionVariable(); doAnswer( - new Answer() { - @Override - public Object answer(InvocationOnMock invocation) throws Throwable { - startedCondition.open(); - return null; - } - }) + new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + startedCondition.open(); + return null; + } + }) .when(mockUrlRequest) .read(any(ByteBuffer.class)); return startedCondition; diff --git a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java index 5f93bc5e23..546aba159d 100644 --- a/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java +++ b/extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSource.java @@ -97,6 +97,9 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou private static final String TAG = "CronetDataSource"; private static final String CONTENT_TYPE = "Content-Type"; + private static final String SET_COOKIE = "Set-Cookie"; + private static final String COOKIE = "Cookie"; + private static final Pattern CONTENT_RANGE_HEADER_PATTERN = Pattern.compile("^bytes (\\d+)-(\\d+)/(\\d+)$"); // The size of read buffer passed to cronet UrlRequest.read(). @@ -109,6 +112,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou private final int connectTimeoutMs; private final int readTimeoutMs; private final boolean resetTimeoutOnRedirects; + private final boolean handleSetCookieRequests; private final RequestProperties defaultRequestProperties; private final RequestProperties requestProperties; private final ConditionVariable operation; @@ -152,7 +156,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou public CronetDataSource(CronetEngine cronetEngine, Executor executor, Predicate contentTypePredicate, TransferListener listener) { this(cronetEngine, executor, contentTypePredicate, listener, DEFAULT_CONNECT_TIMEOUT_MILLIS, - DEFAULT_READ_TIMEOUT_MILLIS, false, null); + DEFAULT_READ_TIMEOUT_MILLIS, false, null, false); } /** @@ -176,13 +180,40 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou int connectTimeoutMs, int readTimeoutMs, boolean resetTimeoutOnRedirects, RequestProperties defaultRequestProperties) { this(cronetEngine, executor, contentTypePredicate, listener, connectTimeoutMs, - readTimeoutMs, resetTimeoutOnRedirects, Clock.DEFAULT, defaultRequestProperties); + readTimeoutMs, resetTimeoutOnRedirects, Clock.DEFAULT, defaultRequestProperties, false); + } + + /** + * @param cronetEngine A CronetEngine. + * @param executor The {@link java.util.concurrent.Executor} that will handle responses. + * This may be a direct executor (i.e. executes tasks on the calling thread) in order + * to avoid a thread hop from Cronet's internal network thread to the response handling + * thread. However, to avoid slowing down overall network performance, care must be taken + * to make sure response handling is a fast operation when using a direct executor. + * @param contentTypePredicate An optional {@link Predicate}. If a content type is rejected by the + * predicate then an {@link InvalidContentTypeException} is thrown from + * {@link #open(DataSpec)}. + * @param listener An optional listener. + * @param connectTimeoutMs The connection timeout, in milliseconds. + * @param readTimeoutMs The read timeout, in milliseconds. + * @param resetTimeoutOnRedirects Whether the connect timeout is reset when a redirect occurs. + * @param defaultRequestProperties The default request properties to be used. + * @param handleSetCookieRequests Whether "Set-Cookie" requests on redirect should be forwarded to + * the redirect url in the "Cookie" header. + */ + public CronetDataSource(CronetEngine cronetEngine, Executor executor, + Predicate contentTypePredicate, TransferListener listener, + int connectTimeoutMs, int readTimeoutMs, boolean resetTimeoutOnRedirects, + RequestProperties defaultRequestProperties, boolean handleSetCookieRequests) { + this(cronetEngine, executor, contentTypePredicate, listener, connectTimeoutMs, + readTimeoutMs, resetTimeoutOnRedirects, Clock.DEFAULT, defaultRequestProperties, + handleSetCookieRequests); } /* package */ CronetDataSource(CronetEngine cronetEngine, Executor executor, Predicate contentTypePredicate, TransferListener listener, int connectTimeoutMs, int readTimeoutMs, boolean resetTimeoutOnRedirects, Clock clock, - RequestProperties defaultRequestProperties) { + RequestProperties defaultRequestProperties, boolean handleSetCookieRequests) { this.cronetEngine = Assertions.checkNotNull(cronetEngine); this.executor = Assertions.checkNotNull(executor); this.contentTypePredicate = contentTypePredicate; @@ -192,6 +223,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou this.resetTimeoutOnRedirects = resetTimeoutOnRedirects; this.clock = Assertions.checkNotNull(clock); this.defaultRequestProperties = defaultRequestProperties; + this.handleSetCookieRequests = handleSetCookieRequests; requestProperties = new RequestProperties(); operation = new ConditionVariable(); } @@ -402,7 +434,19 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou if (resetTimeoutOnRedirects) { resetConnectTimeout(); } - request.followRedirect(); + + Map> headers = info.getAllHeaders(); + if (!handleSetCookieRequests || isEmpty(headers.get(SET_COOKIE))) { + request.followRedirect(); + } else { + UrlRequest.Builder requestBuilder = + cronetEngine.newUrlRequestBuilder(newLocationUrl, /* callback= */ this, executor); + currentUrlRequest.cancel(); + String cookieHeadersValue = parseCookies(headers.get(SET_COOKIE)); + attachCookies(requestBuilder, cookieHeadersValue); + currentUrlRequest = requestBuilder.build(); + currentUrlRequest.start(); + } } @Override @@ -567,6 +611,17 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou return contentLength; } + private static String parseCookies(List setCookieHeaders) { + return TextUtils.join(";", setCookieHeaders); + } + + private static void attachCookies(UrlRequest.Builder requestBuilder, String cookies) { + if (TextUtils.isEmpty(cookies)) { + return; + } + requestBuilder.addHeader(COOKIE, cookies); + } + private static int getStatus(UrlRequest request) throws InterruptedException { final ConditionVariable conditionVariable = new ConditionVariable(); final int[] statusHolder = new int[1];