honeymoose
/
hapi-fhir
mirror of https://github.com/hapifhir/hapi-fhir.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix content negotiation on binary streaming (#4053) 

* Fix content negotiation on binary streaming

* Update hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4051-content-type-negotiation-binaries.yaml

Co-authored-by: michaelabuckley <michael.buckley@smilecdr.com>

* Tidy logic

* Tidy logic

* Naming

* Wip

Co-authored-by: michaelabuckley <michael.buckley@smilecdr.com>
This commit is contained in:
Tadgh 2022-09-21 12:05:35 -07:00 committed by GitHub
parent dbe52b429c
commit 392bfb790f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 135 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4051-content-type-negotiation-binaries.yaml Normal file
View File

@ -0,0 +1,7 @@
---
type: fix
issue: 4051
jira: SMILE-5170
title: "There was a bug in content-type negotiation when reading Binary resources. Previously, when a client requested a Binary resource and with an `Accept`
header that matched the `contentType` of the stored resource, the server would return an XML representation of the Binary resource. This has been fixed, and a request with a matching `Accept` header will receive
the stored binary data directly as the requested content type."

1
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/BulkDataExportTest.java
View File

@ -268,6 +268,7 @@ public class BulkDataExportTest extends BaseResourceProviderR4Test {
verifyBulkExportResults(options, List.of("\"P1\"", "\"" + obsId + "\"", "\"" + encId + "\"", "\"P2\"", "\"" + obsId2 + "\"", "\"" + encId2 + "\""), List.of("\"P3\"", "\"" + obsId3 + "\"", "\"" + encId3 + "\""));
}
private void verifyBulkExportResults(BulkDataExportOptions theOptions, List<String> theContainedList, List<String> theExcludedList) {
Batch2JobStartResponse startResponse = myJobRunner.startNewJob(BulkExportUtils.createBulkExportJobParametersFromExportOptions(theOptions));

91
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/BulkExportUseCaseTest.java
View File

@ -12,9 +12,15 @@ import ca.uhn.fhir.rest.api.server.bulk.BulkDataExportOptions;
import ca.uhn.fhir.util.JsonUtil;
import ca.uhn.fhir.util.SearchParameterUtil;
import com.google.common.collect.Sets;
import org.apache.commons.io.Charsets;
import org.apache.commons.io.IOUtils;
import org.apache.http.Header;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.hamcrest.Matchers;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.r4.model.Binary;
import org.hl7.fhir.r4.model.Bundle;
import org.hl7.fhir.r4.model.Coverage;
import org.hl7.fhir.r4.model.Enumerations;
import org.hl7.fhir.r4.model.Group;
@ -30,6 +36,7 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@ -37,8 +44,10 @@ import java.util.List;
import java.util.Map;
import static org.awaitility.Awaitility.await;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.not;
import static org.junit.jupiter.api.Assertions.assertEquals;
@ -52,7 +61,84 @@ public class BulkExportUseCaseTest extends BaseResourceProviderR4Test {
@Nested
public class SystemBulkExportTests {
//TODO add use case tests here
@Test
public void testBinariesAreStreamedWithRespectToAcceptHeader() throws IOException {
int patientCount = 5;
for (int i=0; i<patientCount; i++) {
Patient patient = new Patient();
patient.setId("pat-" + i);
myPatientDao.update(patient);
}
HashSet<String> types = Sets.newHashSet("Patient");
BulkExportJobResults bulkExportJobResults = startSystemBulkExportJobAndAwaitCompletion(types, new HashSet<String>());
Map<String, List<String>> resourceTypeToBinaryIds = bulkExportJobResults.getResourceTypeToBinaryIds();
assertThat(resourceTypeToBinaryIds.get("Patient"), hasSize(1));
String patientBinaryId = resourceTypeToBinaryIds.get("Patient").get(0);
String replace = patientBinaryId.replace("_history/1", "");
{ // Test with the Accept Header omitted should stream out the results.
HttpGet expandGet = new HttpGet(ourServerBase + "/" + replace);
try (CloseableHttpResponse status = ourHttpClient.execute(expandGet)) {
Header[] headers = status.getHeaders("Content-Type");
String response = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
logContentTypeAndResponse(headers, response);
validateNdJsonResponse(headers, response, patientCount);
}
}
{ //Test with the Accept Header set to application/fhir+ndjson should stream out the results.
HttpGet expandGet = new HttpGet(ourServerBase + "/" + replace);
expandGet.addHeader(Constants.HEADER_ACCEPT, Constants.CT_FHIR_NDJSON);
try (CloseableHttpResponse status = ourHttpClient.execute(expandGet)) {
Header[] headers = status.getHeaders("Content-Type");
String response = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
logContentTypeAndResponse(headers, response);
validateNdJsonResponse(headers, response, patientCount);
}
}
{ //Test that demanding octet-stream will force it to whatever the Binary's content-type is set to.
HttpGet expandGet = new HttpGet(ourServerBase + "/" + replace);
expandGet.addHeader(Constants.HEADER_ACCEPT, Constants.CT_OCTET_STREAM);
try (CloseableHttpResponse status = ourHttpClient.execute(expandGet)) {
Header[] headers = status.getHeaders("Content-Type");
String response = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
logContentTypeAndResponse(headers, response);
validateNdJsonResponse(headers, response, patientCount);
}
}
{ //Test with the Accept Header set to application/fhir+json should simply return the Binary resource.
HttpGet expandGet = new HttpGet(ourServerBase + "/" + replace);
expandGet.addHeader(Constants.HEADER_ACCEPT, Constants.CT_FHIR_JSON);
try (CloseableHttpResponse status = ourHttpClient.execute(expandGet)) {
Header[] headers = status.getHeaders("Content-Type");
String response = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
logContentTypeAndResponse(headers, response);
assertThat(headers[0].getValue(), containsString(Constants.CT_FHIR_JSON));
assertThat(response, is(not(containsString("\n"))));
Binary binary= myFhirContext.newJsonParser().parseResource(Binary.class, response);
assertThat(binary.getIdElement().getValue(), is(equalTo(patientBinaryId)));
}
}
}
private void logContentTypeAndResponse(Header[] headers, String response) {
ourLog.info("**************************");
ourLog.info("Content-Type is: {}", headers[0]);
ourLog.info("Response is: {}", response);
ourLog.info("**************************");
}
private void validateNdJsonResponse(Header[] headers, String response, int theExpectedCount) {
assertThat(headers[0].getValue(), containsString(Constants.CT_FHIR_NDJSON));
assertThat(response, is(containsString("\n")));
Bundle bundle = myFhirContext.newNDJsonParser().parseResource(Bundle.class, response);
assertThat(bundle.getEntry(), hasSize(theExpectedCount));
}
}
@Nested
@ -578,7 +664,10 @@ public class BulkExportUseCaseTest extends BaseResourceProviderR4Test {
}
BulkExportJobResults startGroupBulkExportJobAndAwaitCompletion(HashSet<String> theResourceTypes, HashSet<String> theFilters, String theGroupId) {
return startBulkExportJobAndAwaitCompletion(BulkDataExportOptions.ExportStyle.GROUP, theResourceTypes, theFilters, theGroupId);
}
BulkExportJobResults startSystemBulkExportJobAndAwaitCompletion(HashSet<String> theResourceTypes, HashSet<String> theFilters) {
return startBulkExportJobAndAwaitCompletion(BulkDataExportOptions.ExportStyle.SYSTEM, theResourceTypes, theFilters, null);
}
BulkExportJobResults startBulkExportJobAndAwaitCompletion(BulkDataExportOptions.ExportStyle theExportStyle, HashSet<String> theResourceTypes, HashSet<String> theFilters, String theGroupOrPatientId) {

44
hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java
View File

@ -76,6 +76,7 @@ import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.StringTokenizer;
import java.util.TreeSet;
@ -943,15 +944,10 @@ public class RestfulServerUtils {
// If the user didn't explicitly request FHIR as a response, return binary
// content directly
if (responseEncoding == null) {
if (isNotBlank(bin.getContentType())) {
contentType = bin.getContentType();
} else {
contentType = Constants.CT_OCTET_STREAM;
}
if (shouldStreamContents(responseEncoding, bin)) {
// Force binary resources to download - This is a security measure to prevent
// malicious images or HTML blocks being served up as content.
contentType = getBinaryContentTypeOrDefault(bin);
response.addHeader(Constants.HEADER_CONTENT_DISPOSITION, "Attachment;");
return response.sendAttachmentResponse(bin, theStatusCode, contentType);
@ -1037,6 +1033,40 @@ public class RestfulServerUtils {
return response.sendWriterResponse(theStatusCode, contentType, charset, writer);
}
private static String getBinaryContentTypeOrDefault(IBaseBinary theBinary) {
String contentType;
if (isNotBlank(theBinary.getContentType())) {
contentType = theBinary.getContentType();
} else {
contentType = Constants.CT_OCTET_STREAM;
}
return contentType;
}
/**
* Determines whether we should stream out Binary resource content based on the content-type. Logic is:
* - If they request octet-stream, return true;
* - If the content-type happens to be a match, return true.
* - Construct an EncodingEnum out of the contentType. If this matches the responseEncoding, return true.
* - Otherwise, return false.
*
* @param theResponseEncoding the requested {@link EncodingEnum} determined by the incoming Content-Type header.
* @param theBinary the {@link IBaseBinary} resource to be streamed out.
* @return True if response can be streamed as the requested encoding type, false otherwise.
*/
private static boolean shouldStreamContents(ResponseEncoding theResponseEncoding, IBaseBinary theBinary) {
String contentType = theBinary.getContentType();
if (theResponseEncoding == null) {
return true;
} if (isBlank(contentType)) {
return Constants.CT_OCTET_STREAM.equals(theResponseEncoding.getContentType());
} else if (contentType.equalsIgnoreCase(theResponseEncoding.getContentType())) {
return true;
} else {
return Objects.equals(EncodingEnum.forContentType(contentType), theResponseEncoding.getEncoding());
}
}
public static String createEtag(String theVersionId) {
return "W/\"" + theVersionId + '"';
}