From aee7b2b882fd9ed363cfcfc95bcf46a77b96c852 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 10 Jan 2019 07:26:04 -0700 Subject: [PATCH] Make sure that sub-request transaction searches and reads preserve HTTP headers --- .../uhn/fhir/jpa/dao/FhirSystemDaoDstu2.java | 2 +- .../fhir/jpa/dao/TransactionProcessor.java | 2 +- .../provider/ServletSubRequestDetails.java | 22 ++++++-- .../dbcache/DaoSubscriptionProvider.java | 5 +- .../dbmatcher/DaoSubscriptionMatcher.java | 5 +- ...tionInterceptorResourceProviderR4Test.java | 55 +++++++++++++++++++ .../server/servlet/ServletRequestDetails.java | 18 +++++- src/changes/changes.xml | 6 ++ 8 files changed, 98 insertions(+), 17 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirSystemDaoDstu2.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirSystemDaoDstu2.java index 32bdbe3a77f..1d4e7456b9e 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirSystemDaoDstu2.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirSystemDaoDstu2.java @@ -233,7 +233,7 @@ public class FhirSystemDaoDstu2 extends BaseHapiFhirSystemDao { Integer originalOrder = originalRequestOrder.get(nextReqEntry); Entry nextRespEntry = response.getEntry().get(originalOrder); - ServletSubRequestDetails requestDetails = new ServletSubRequestDetails(); + ServletSubRequestDetails requestDetails = new ServletSubRequestDetails(theRequestDetails); requestDetails.setServletRequest(theRequestDetails.getServletRequest()); requestDetails.setRequestType(RequestTypeEnum.GET); requestDetails.setServer(theRequestDetails.getServer()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java index 898895ede5a..a8fd3445431 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java @@ -384,7 +384,7 @@ public class TransactionProcessor { Integer originalOrder = originalRequestOrder.get(nextReqEntry); BUNDLEENTRY nextRespEntry = myVersionAdapter.getEntries(response).get(originalOrder); - ServletSubRequestDetails requestDetails = new ServletSubRequestDetails(); + ServletSubRequestDetails requestDetails = new ServletSubRequestDetails(theRequestDetails); requestDetails.setServletRequest(theRequestDetails.getServletRequest()); requestDetails.setRequestType(RequestTypeEnum.GET); requestDetails.setServer(theRequestDetails.getServer()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/ServletSubRequestDetails.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/ServletSubRequestDetails.java index 3ba5e226b4d..1fba4b84008 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/ServletSubRequestDetails.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/ServletSubRequestDetails.java @@ -29,11 +29,25 @@ import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; public class ServletSubRequestDetails extends ServletRequestDetails { - private Map> myHeaders = new HashMap<>(); + private Map> myHeaders = new HashMap<>(); + + /** + * Constructor + * + * @param theRequestDetails The parent request details + */ + public ServletSubRequestDetails(ServletRequestDetails theRequestDetails) { + if (theRequestDetails != null) { + Map> headers = theRequestDetails.getHeaders(); + for (Map.Entry> next : headers.entrySet()) { + myHeaders.put(next.getKey().toLowerCase(), next.getValue()); + } + } + } public void addHeader(String theName, String theValue) { String lowerCase = theName.toLowerCase(); - ArrayList list = myHeaders.get(lowerCase); + List list = myHeaders.get(lowerCase); if (list == null) { list = new ArrayList<>(); myHeaders.put(lowerCase, list); @@ -43,7 +57,7 @@ public class ServletSubRequestDetails extends ServletRequestDetails { @Override public String getHeader(String theName) { - ArrayList list = myHeaders.get(theName.toLowerCase()); + List list = myHeaders.get(theName.toLowerCase()); if (list == null || list.isEmpty()) { return null; } @@ -52,7 +66,7 @@ public class ServletSubRequestDetails extends ServletRequestDetails { @Override public List getHeaders(String theName) { - ArrayList list = myHeaders.get(theName.toLowerCase()); + List list = myHeaders.get(theName.toLowerCase()); if (list == null || list.isEmpty()) { return null; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbcache/DaoSubscriptionProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbcache/DaoSubscriptionProvider.java index 97a5e46dc5a..18725dbe386 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbcache/DaoSubscriptionProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbcache/DaoSubscriptionProvider.java @@ -42,10 +42,7 @@ public class DaoSubscriptionProvider implements ISubscriptionProvider { @Override public IBundleProvider search(SearchParameterMap theMap) { IFhirResourceDao subscriptionDao = myDaoRegistry.getSubscriptionDao(); - RequestDetails req = new ServletSubRequestDetails(); - req.setSubRequest(true); - - return subscriptionDao.search(theMap, req); + return subscriptionDao.search(theMap); } @Override diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbmatcher/DaoSubscriptionMatcher.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbmatcher/DaoSubscriptionMatcher.java index eca40b9b2b7..24c41e9c560 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbmatcher/DaoSubscriptionMatcher.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/dbmatcher/DaoSubscriptionMatcher.java @@ -72,12 +72,9 @@ public class DaoSubscriptionMatcher implements ISubscriptionMatcher { RuntimeResourceDefinition responseResourceDef = subscriptionDao.validateCriteriaAndReturnResourceDefinition(theCriteria); SearchParameterMap responseCriteriaUrl = myMatchUrlService.translateMatchUrl(theCriteria, responseResourceDef); - RequestDetails req = new ServletSubRequestDetails(); - req.setSubRequest(true); - IFhirResourceDao responseDao = myDaoRegistry.getResourceDao(responseResourceDef.getImplementingClass()); responseCriteriaUrl.setLoadSynchronousUpTo(1); - return responseDao.search(responseCriteriaUrl, req); + return responseDao.search(responseCriteriaUrl); } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java index 00125a6cbc3..347aab83d0e 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java @@ -4,6 +4,8 @@ import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.MethodOutcome; import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.client.interceptor.SimpleRequestHeaderInterceptor; +import ca.uhn.fhir.rest.server.exceptions.AuthenticationException; import ca.uhn.fhir.rest.server.exceptions.ForbiddenOperationException; import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor; @@ -28,6 +30,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.*; @@ -148,6 +151,58 @@ public class AuthorizationInterceptorResourceProviderR4Test extends BaseResource } + @Test + public void testReadInTransaction() { + + Patient patient = new Patient(); + patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100"); + patient.addName().setFamily("Tester").addGiven("Raghad"); + IIdType id = ourClient.update().resource(patient).conditionalByUrl("Patient?identifier=http://uhn.ca/mrns|100").execute().getId().toUnqualifiedVersionless(); + + ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + String authHeader = theRequestDetails.getHeader("Authorization"); + if (!"Bearer AAA".equals(authHeader)) { + throw new AuthenticationException("Invalid auth header: " + authHeader); + } + return new RuleBuilder() + .allow().transaction().withAnyOperation().andApplyNormalRules().andThen() + .allow().read().resourcesOfType(Patient.class).withAnyId() + .build(); + } + }); + + SimpleRequestHeaderInterceptor interceptor = new SimpleRequestHeaderInterceptor("Authorization", "Bearer AAA"); + try { + ourClient.registerInterceptor(interceptor); + + Bundle bundle; + Bundle responseBundle; + + // Read + bundle = new Bundle(); + bundle.setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl(id.getValue()); + responseBundle = ourClient.transaction().withBundle(bundle).execute(); + patient = (Patient) responseBundle.getEntry().get(0).getResource(); + assertEquals("Tester", patient.getNameFirstRep().getFamily()); + + // Search + bundle = new Bundle(); + bundle.setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl("Patient?"); + responseBundle = ourClient.transaction().withBundle(bundle).execute(); + responseBundle = (Bundle) responseBundle.getEntry().get(0).getResource(); + patient = (Patient) responseBundle.getEntry().get(0).getResource(); + assertEquals("Tester", patient.getNameFirstRep().getFamily()); + + } finally { + ourClient.unregisterInterceptor(interceptor); + } + + } + /** * See #751 */ diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java index 33b9b096ed0..f6d4448a05c 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java @@ -34,9 +34,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.Reader; import java.nio.charset.Charset; -import java.util.Collections; -import java.util.Enumeration; -import java.util.List; +import java.util.*; import java.util.zip.GZIPInputStream; import static org.apache.commons.lang3.StringUtils.isNotBlank; @@ -145,4 +143,18 @@ public class ServletRequestDetails extends RequestDetails { this.myServletResponse = myServletResponse; } + public Map> getHeaders() { + Map> retVal = new HashMap<>(); + Enumeration names = myServletRequest.getHeaderNames(); + while (names.hasMoreElements()) { + String nextName = names.nextElement(); + ArrayList headerValues = new ArrayList<>(); + retVal.put(nextName, headerValues); + Enumeration valuesEnum = myServletRequest.getHeaders(nextName); + while (valuesEnum.hasMoreElements()) { + headerValues.add(valuesEnum.nextElement()); + } + } + return Collections.unmodifiableMap(retVal); + } } diff --git a/src/changes/changes.xml b/src/changes/changes.xml index a62b977bde1..d425b41e8fc 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -277,6 +277,12 @@ part of the chain was lost when the chain was described in the server CapabilityStatement. This has been corrected. + + In the JPA server, search/read operations being performed within a transaction bundle + did not pass the client request HTTP headers to the sub-request. This meant that + AuthorizationInterceptor could not authorize these requests if it was depending on + headers being present. +