Commit 46172ffd by falhassen Committed by Oliver Woodman

Preserve original on redirect with the set-cookie flow.

We need to make sure the original header is retained when we redirect.

I filed a request on Cronet to allow headers to be provided to the UrlRequest#followRedirect method:
https://bugs.chromium.org/p/chromium/issues/detail?id=779611

Until that API is changed, i.e., pulled into GMSCore, and most clients are using the version of GMSCore with the API change, we can stick with this approach.

FYI

Cronet generally uses the original headers on redirect:

http://[]

but modifies the headers for these special cases:

hhttp://[]

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=174074572
parent 72b67916
...@@ -714,7 +714,7 @@ public final class CronetDataSourceTest { ...@@ -714,7 +714,7 @@ public final class CronetDataSourceTest {
} }
@Test @Test
public void testRedirectParseAndAttachCookie_dataSourceHandlesSetCookie() public void testRedirectParseAndAttachCookie_dataSourceHandlesSetCookie_andPreservesOriginalRequestHeaders()
throws HttpDataSourceException { throws HttpDataSourceException {
dataSourceUnderTest = spy( dataSourceUnderTest = spy(
new CronetDataSource( new CronetDataSource(
...@@ -728,12 +728,46 @@ public final class CronetDataSourceTest { ...@@ -728,12 +728,46 @@ public final class CronetDataSourceTest {
mockClock, mockClock,
null, null,
true)); true));
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
mockSingleRedirectSuccess();
testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video");
dataSourceUnderTest.open(testDataSpec);
verify(mockUrlRequestBuilder).addHeader(eq("Cookie"), any(String.class));
verify(mockUrlRequestBuilder, never()).addHeader(eq("Range"), any(String.class));
verify(mockUrlRequestBuilder, times(2)).addHeader("Content-Type", TEST_CONTENT_TYPE);
verify(mockUrlRequest, never()).followRedirect();
verify(mockUrlRequest, times(2)).start();
}
@Test
public void testRedirectParseAndAttachCookie_dataSourceHandlesSetCookie_andPreservesOriginalRequestHeadersIncludingByteRangeHeader()
throws HttpDataSourceException {
testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null);
dataSourceUnderTest = spy(
new CronetDataSource(
mockCronetEngine,
mockExecutor,
mockContentTypePredicate,
mockTransferListener,
TEST_CONNECT_TIMEOUT_MS,
TEST_READ_TIMEOUT_MS,
true, // resetTimeoutOnRedirects
mockClock,
null,
true));
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
mockSingleRedirectSuccess(); mockSingleRedirectSuccess();
testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video"); testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video");
dataSourceUnderTest.open(testDataSpec); dataSourceUnderTest.open(testDataSpec);
verify(mockUrlRequestBuilder).addHeader(eq("Cookie"), any(String.class)); verify(mockUrlRequestBuilder).addHeader(eq("Cookie"), any(String.class));
verify(mockUrlRequestBuilder, times(2)).addHeader("Range", "bytes=1000-5999");
verify(mockUrlRequestBuilder, times(2)).addHeader("Content-Type", TEST_CONTENT_TYPE);
verify(mockUrlRequest, never()).followRedirect(); verify(mockUrlRequest, never()).followRedirect();
verify(mockUrlRequest, times(2)).start(); verify(mockUrlRequest, times(2)).start();
} }
......
...@@ -263,7 +263,11 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou ...@@ -263,7 +263,11 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou
operation.close(); operation.close();
resetConnectTimeout(); resetConnectTimeout();
currentDataSpec = dataSpec; currentDataSpec = dataSpec;
currentUrlRequest = buildRequest(dataSpec); try {
currentUrlRequest = buildRequestBuilder(dataSpec).build();
} catch (IOException e) {
throw new OpenException(e, currentDataSpec, Status.IDLE);
}
currentUrlRequest.start(); currentUrlRequest.start();
try { try {
...@@ -439,9 +443,18 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou ...@@ -439,9 +443,18 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou
if (!handleSetCookieRequests || isEmpty(headers.get(SET_COOKIE))) { if (!handleSetCookieRequests || isEmpty(headers.get(SET_COOKIE))) {
request.followRedirect(); request.followRedirect();
} else { } else {
UrlRequest.Builder requestBuilder =
cronetEngine.newUrlRequestBuilder(newLocationUrl, /* callback= */ this, executor);
currentUrlRequest.cancel(); currentUrlRequest.cancel();
DataSpec redirectUrlDataSpec = new DataSpec(Uri.parse(newLocationUrl),
currentDataSpec.postBody, currentDataSpec.absoluteStreamPosition,
currentDataSpec.position, currentDataSpec.length, currentDataSpec.key,
currentDataSpec.flags);
UrlRequest.Builder requestBuilder;
try {
requestBuilder = buildRequestBuilder(redirectUrlDataSpec);
} catch (IOException e) {
exception = e;
return;
}
String cookieHeadersValue = parseCookies(headers.get(SET_COOKIE)); String cookieHeadersValue = parseCookies(headers.get(SET_COOKIE));
attachCookies(requestBuilder, cookieHeadersValue); attachCookies(requestBuilder, cookieHeadersValue);
currentUrlRequest = requestBuilder.build(); currentUrlRequest = requestBuilder.build();
...@@ -494,7 +507,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou ...@@ -494,7 +507,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou
// Internal methods. // Internal methods.
private UrlRequest buildRequest(DataSpec dataSpec) throws OpenException { private UrlRequest.Builder buildRequestBuilder(DataSpec dataSpec) throws IOException {
UrlRequest.Builder requestBuilder = cronetEngine.newUrlRequestBuilder( UrlRequest.Builder requestBuilder = cronetEngine.newUrlRequestBuilder(
dataSpec.uri.toString(), this, executor).allowDirectExecutor(); dataSpec.uri.toString(), this, executor).allowDirectExecutor();
// Set the headers. // Set the headers.
...@@ -513,17 +526,16 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou ...@@ -513,17 +526,16 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou
requestBuilder.addHeader(key, headerEntry.getValue()); requestBuilder.addHeader(key, headerEntry.getValue());
} }
if (dataSpec.postBody != null && dataSpec.postBody.length != 0 && !isContentTypeHeaderSet) { if (dataSpec.postBody != null && dataSpec.postBody.length != 0 && !isContentTypeHeaderSet) {
throw new OpenException("POST request with non-empty body must set Content-Type", dataSpec, throw new IOException("POST request with non-empty body must set Content-Type");
Status.IDLE);
} }
// Set the Range header. // Set the Range header.
if (currentDataSpec.position != 0 || currentDataSpec.length != C.LENGTH_UNSET) { if (dataSpec.position != 0 || dataSpec.length != C.LENGTH_UNSET) {
StringBuilder rangeValue = new StringBuilder(); StringBuilder rangeValue = new StringBuilder();
rangeValue.append("bytes="); rangeValue.append("bytes=");
rangeValue.append(currentDataSpec.position); rangeValue.append(dataSpec.position);
rangeValue.append("-"); rangeValue.append("-");
if (currentDataSpec.length != C.LENGTH_UNSET) { if (dataSpec.length != C.LENGTH_UNSET) {
rangeValue.append(currentDataSpec.position + currentDataSpec.length - 1); rangeValue.append(dataSpec.position + dataSpec.length - 1);
} }
requestBuilder.addHeader("Range", rangeValue.toString()); requestBuilder.addHeader("Range", rangeValue.toString());
} }
...@@ -541,7 +553,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou ...@@ -541,7 +553,7 @@ public class CronetDataSource extends UrlRequest.Callback implements HttpDataSou
executor); executor);
} }
} }
return requestBuilder.build(); return requestBuilder;
} }
private boolean blockUntilConnectTimeout() throws InterruptedException { private boolean blockUntilConnectTimeout() throws InterruptedException {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment