diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3047-inline-match-security.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3047-inline-match-security.yaml new file mode 100644 index 00000000000..b7137e0c9db --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3047-inline-match-security.yaml @@ -0,0 +1,5 @@ +--- +type: add +issue: 3047 +jira: SMILE-2903 +title: "Inline match URL searches (e.g. search URLs for conditional creates, conditional updates, etc.) are now subject to the same security and access control checks as other searches." diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java index 67abbb8f8f4..8f59563affe 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java @@ -8,10 +8,13 @@ import ca.uhn.fhir.jpa.partition.SystemRequestDetails; import ca.uhn.fhir.jpa.provider.r4.BaseResourceProviderR4Test; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.util.SqlQuery; +import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.api.SortSpec; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.param.ReferenceParam; import ca.uhn.fhir.rest.param.TokenParam; +import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor; +import ca.uhn.fhir.rest.server.interceptor.auth.PolicyEnum; import ca.uhn.fhir.util.BundleBuilder; import org.apache.commons.lang3.StringUtils; import org.hl7.fhir.instance.model.api.IIdType; @@ -1063,6 +1066,53 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test } + @Test + public void testTransactionWithMultipleInlineMatchUrlsWithAuthentication() { + myDaoConfig.setDeleteEnabled(false); + myDaoConfig.setMassIngestionMode(true); + myDaoConfig.setAllowInlineMatchUrlReferences(true); + myDaoConfig.setMatchUrlCacheEnabled(true); + + Location loc = new Location(); + loc.setId("LOC"); + loc.addIdentifier().setSystem("http://foo").setValue("123"); + myLocationDao.update(loc, mySrd); + + BundleBuilder bb = new BundleBuilder(myFhirCtx); + for (int i = 0; i < 5; i++) { + Encounter enc = new Encounter(); + enc.addLocation().setLocation(new Reference("Location?identifier=http://foo|123")); + bb.addTransactionCreateEntry(enc); + } + Bundle input = (Bundle) bb.getBundle(); + + when(mySrd.getRestOperationType()).thenReturn(RestOperationTypeEnum.TRANSACTION); + myInterceptorRegistry.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.ALLOW)); + + myCaptureQueriesListener.clear(); + mySystemDao.transaction(mySrd, input); + myCaptureQueriesListener.logSelectQueriesForCurrentThread(); + assertEquals(4, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(6, runInTransaction(() -> myResourceTableDao.count())); + + // Second identical pass + + bb = new BundleBuilder(myFhirCtx); + for (int i = 0; i < 5; i++) { + Encounter enc = new Encounter(); + enc.addLocation().setLocation(new Reference("Location?identifier=http://foo|123")); + bb.addTransactionCreateEntry(enc); + } + input = (Bundle) bb.getBundle(); + + myCaptureQueriesListener.clear(); + mySystemDao.transaction(mySrd, input); + myCaptureQueriesListener.logSelectQueriesForCurrentThread(); + assertEquals(2, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(11, runInTransaction(() -> myResourceTableDao.count())); + + } + @Test public void testTransactionWithMultipleForcedIdReferences() { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java index edbb5e46c7d..1064b3788f2 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java @@ -15,7 +15,9 @@ import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.model.api.ResourceMetadataKeyEnum; import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.Constants; +import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.param.ReferenceParam; import ca.uhn.fhir.rest.param.StringParam; import ca.uhn.fhir.rest.param.TokenParam; @@ -26,6 +28,10 @@ import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor; +import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRule; +import ca.uhn.fhir.rest.server.interceptor.auth.PolicyEnum; +import ca.uhn.fhir.rest.server.interceptor.auth.RuleBuilder; import ca.uhn.fhir.util.BundleBuilder; import ca.uhn.fhir.util.ClasspathUtil; import org.apache.commons.io.IOUtils; @@ -43,6 +49,7 @@ import org.hl7.fhir.r4.model.Bundle.BundleEntryResponseComponent; import org.hl7.fhir.r4.model.Bundle.BundleType; import org.hl7.fhir.r4.model.Bundle.HTTPVerb; import org.hl7.fhir.r4.model.CanonicalType; +import org.hl7.fhir.r4.model.CarePlan; import org.hl7.fhir.r4.model.Coding; import org.hl7.fhir.r4.model.Communication; import org.hl7.fhir.r4.model.Condition; @@ -90,6 +97,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.UUID; import java.util.stream.Collectors; import static org.hamcrest.MatcherAssert.assertThat; @@ -112,6 +120,8 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest { @@ -1388,6 +1398,149 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest { } + @Test + public void testTransactionCreateInlineMatchUrlWithAuthorizationAllowed() { + // setup + String methodName = "testTransactionCreateInlineMatchUrlWithAuthorizationAllowed"; + Bundle request = new Bundle(); + + myDaoConfig.setAllowInlineMatchUrlReferences(true); + + Patient p = new Patient(); + p.addIdentifier().setSystem("urn:system").setValue(methodName); + p.setId("Patient/" + methodName); + IIdType id = myPatientDao.update(p, mySrd).getId(); + ourLog.info("Created patient, got it: {}", id); + + Observation o = new Observation(); + o.getCode().setText("Some Observation"); + o.getSubject().setReference("Patient?identifier=urn%3Asystem%7C" + methodName); + request.addEntry().setResource(o).getRequest().setMethod(HTTPVerb.POST); + + when(mySrd.getRestOperationType()).thenReturn(RestOperationTypeEnum.TRANSACTION); + + myInterceptorRegistry.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.ALLOW)); + + // execute + Bundle resp = mySystemDao.transaction(mySrd, request); + + // validate + assertEquals(1, resp.getEntry().size()); + + BundleEntryComponent respEntry = resp.getEntry().get(0); + assertEquals(Constants.STATUS_HTTP_201_CREATED + " Created", respEntry.getResponse().getStatus()); + assertThat(respEntry.getResponse().getLocation(), containsString("Observation/")); + assertThat(respEntry.getResponse().getLocation(), endsWith("/_history/1")); + assertEquals("1", respEntry.getResponse().getEtag()); + + o = myObservationDao.read(new IdType(respEntry.getResponse().getLocationElement()), mySrd); + assertEquals(id.toVersionless().getValue(), o.getSubject().getReference()); + assertEquals("1", o.getIdElement().getVersionIdPart()); + + } + + @Test + public void testTransactionCreateInlineMatchUrlWithAuthorizationDenied() { + // setup + String methodName = "testTransactionCreateInlineMatchUrlWithAuthorizationDenied"; + Bundle request = new Bundle(); + + myDaoConfig.setAllowInlineMatchUrlReferences(true); + + Patient p = new Patient(); + p.addIdentifier().setSystem("urn:system").setValue(methodName); + p.setId("Patient/" + methodName); + IIdType id = myPatientDao.update(p, mySrd).getId(); + ourLog.info("Created patient, got it: {}", id); + + Observation o = new Observation(); + o.getCode().setText("Some Observation"); + o.getSubject().setReference("Patient?identifier=urn%3Asystem%7C" + methodName); + request.addEntry().setResource(o).getRequest().setMethod(HTTPVerb.POST); + + when(mySrd.getRestOperationType()).thenReturn(RestOperationTypeEnum.TRANSACTION); + when(mySrd.getFhirContext().getResourceType(any(Observation.class))).thenReturn("Observation"); + + myInterceptorRegistry.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.ALLOW) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("Rule 1").create().resourcesOfType(Observation.class).withAnyId().andThen() + .allow("Rule 2").read().resourcesOfType(Patient.class).inCompartment("Patient", new IdType("Patient/this-is-not-the-id-you-are-looking-for")).andThen() + .denyAll() + .build(); + } + }); + + try { + // execute + mySystemDao.transaction(mySrd, request); + + // verify + fail(); + } catch (ResourceNotFoundException e) { + assertEquals("Invalid match URL \"Patient?identifier=urn%3Asystem%7CtestTransactionCreateInlineMatchUrlWithAuthorizationDenied\" - No resources match this search", e.getMessage()); + } + + } + + @Test + public void testTransactionCreateInlineMatchUrlWithAuthorizationDeniedAndCacheEnabled() { + // setup + String patientId = UUID.randomUUID().toString(); + Bundle request1 = new Bundle(); + Bundle request2 = new Bundle(); + + myDaoConfig.setAllowInlineMatchUrlReferences(true); + myDaoConfig.setMatchUrlCacheEnabled(true); + + Patient p = new Patient(); + p.addIdentifier().setSystem("urn:system").setValue(patientId); + p.setId("Patient/" + patientId); + IIdType id = myPatientDao.update(p, mySrd).getId(); + ourLog.info("Created patient, got it: {}", id); + + Observation o1 = new Observation(); + o1.getCode().setText("Some Observation"); + o1.getSubject().setReference("Patient?identifier=urn%3Asystem%7C" + patientId); + request1.addEntry().setResource(o1).getRequest().setMethod(HTTPVerb.POST); + + Observation o2 = new Observation(); + o2.getCode().setText("Another Observation"); + o2.getSubject().setReference("Patient?identifier=urn%3Asystem%7C" + patientId); + request2.addEntry().setResource(o2).getRequest().setMethod(HTTPVerb.POST); + + // execute the first request before setting up the security rules, to populate the cache + mySystemDao.transaction(mySrd, request1); + + when(mySrd.getRestOperationType()).thenReturn(RestOperationTypeEnum.TRANSACTION); + when(mySrd.getFhirContext().getResourceType(any(Observation.class))).thenReturn("Observation"); + + myInterceptorRegistry.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.ALLOW) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("Rule 1").create().resourcesOfType(Observation.class).withAnyId().andThen() + .allow("Rule 2").read().resourcesOfType(Patient.class).inCompartment("Patient", new IdType("Patient/this-is-not-the-id-you-are-looking-for")).andThen() + .denyAll() + .build(); + } + }); + + try { + // execute + + // the second attempt to access the resource should fail even though the first one succeeded + mySystemDao.transaction(mySrd, request2); + + // verify + fail(); + } catch (ResourceNotFoundException e) { + assertEquals("Invalid match URL \"Patient?identifier=urn%3Asystem%7C" + patientId + "\" - No resources match this search", e.getMessage()); + } + + } + @Test public void testTransactionWithDuplicateConditionalCreates() { Bundle request = new Bundle(); diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/MatchResourceUrlService.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/MatchResourceUrlService.java index 9235872bf8c..c24bc7d1ea3 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/MatchResourceUrlService.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/MatchResourceUrlService.java @@ -32,9 +32,12 @@ import ca.uhn.fhir.jpa.model.search.StorageProcessingMessage; import ca.uhn.fhir.jpa.searchparam.MatchUrlService; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.util.MemoryCacheService; +import ca.uhn.fhir.rest.api.server.IPreResourceShowDetails; import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.api.server.SimplePreResourceShowDetails; import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId; import ca.uhn.fhir.rest.api.server.storage.TransactionDetails; +import ca.uhn.fhir.rest.server.exceptions.ForbiddenOperationException; import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; @@ -42,15 +45,25 @@ import ca.uhn.fhir.rest.server.util.CompositeInterceptorBroadcaster; import ca.uhn.fhir.util.StopWatch; import org.apache.commons.lang3.Validate; import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.instance.model.api.IIdType; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Service; import javax.annotation.Nullable; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; @Service public class MatchResourceUrlService { + + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(MatchResourceUrlService.class); + @Autowired private DaoRegistry myDaoRegistry; @Autowired @@ -75,11 +88,14 @@ public class MatchResourceUrlService { * Note that this will only return a maximum of 2 results!! */ public Set processMatchUrl(String theMatchUrl, Class theResourceType, TransactionDetails theTransactionDetails, RequestDetails theRequest, IBaseResource theConditionalOperationTargetOrNull) { + Set retVal = null; + String resourceType = myContext.getResourceType(theResourceType); String matchUrl = massageForStorage(resourceType, theMatchUrl); ResourcePersistentId resolvedInTransaction = theTransactionDetails.getResolvedMatchUrls().get(matchUrl); if (resolvedInTransaction != null) { + // If the resource has previously been looked up within the transaction, there's no need to re-authorize it. if (resolvedInTransaction == TransactionDetails.NOT_FOUND) { return Collections.emptySet(); } else { @@ -89,17 +105,51 @@ public class MatchResourceUrlService { ResourcePersistentId resolvedInCache = processMatchUrlUsingCacheOnly(resourceType, matchUrl); if (resolvedInCache != null) { - return Collections.singleton(resolvedInCache); + retVal = Collections.singleton(resolvedInCache); } - RuntimeResourceDefinition resourceDef = myContext.getResourceDefinition(theResourceType); - SearchParameterMap paramMap = myMatchUrlService.translateMatchUrl(matchUrl, resourceDef); - if (paramMap.isEmpty() && paramMap.getLastUpdated() == null) { - throw new InvalidRequestException("Invalid match URL[" + matchUrl + "] - URL has no search parameters"); - } - paramMap.setLoadSynchronousUpTo(2); + if (retVal == null) { + RuntimeResourceDefinition resourceDef = myContext.getResourceDefinition(theResourceType); + SearchParameterMap paramMap = myMatchUrlService.translateMatchUrl(matchUrl, resourceDef); + if (paramMap.isEmpty() && paramMap.getLastUpdated() == null) { + throw new InvalidRequestException("Invalid match URL[" + matchUrl + "] - URL has no search parameters"); + } + paramMap.setLoadSynchronousUpTo(2); - Set retVal = search(paramMap, theResourceType, theRequest, theConditionalOperationTargetOrNull); + retVal = search(paramMap, theResourceType, theRequest, theConditionalOperationTargetOrNull); + } + + // Interceptor broadcast: STORAGE_PRESHOW_RESOURCES + if (CompositeInterceptorBroadcaster.hasHooks(Pointcut.STORAGE_PRESHOW_RESOURCES, myInterceptorBroadcaster, theRequest)) { + Map resourceToPidMap = new HashMap<>(); + + IFhirResourceDao dao = getResourceDao(theResourceType); + + for (ResourcePersistentId pid : retVal) { + resourceToPidMap.put(dao.readByPid(pid), pid); + } + + SimplePreResourceShowDetails accessDetails = new SimplePreResourceShowDetails(resourceToPidMap.keySet()); + HookParams params = new HookParams() + .add(IPreResourceShowDetails.class, accessDetails) + .add(RequestDetails.class, theRequest) + .addIfMatchesType(ServletRequestDetails.class, theRequest); + + try { + CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequest, Pointcut.STORAGE_PRESHOW_RESOURCES, params); + + retVal = accessDetails.toList() + .stream() + .map(resourceToPidMap::get) + .filter(Objects::nonNull) + .collect(Collectors.toSet()); + } catch (ForbiddenOperationException e) { + // If the search matches a resource that the user does not have authorization for, + // we want to treat it the same as if the search matched no resources, in order not to leak information. + ourLog.warn("Inline match URL [" + matchUrl + "] specified a resource the user is not authorized to access.", e); + retVal = new HashSet<>(); + } + } if (retVal.size() == 1) { ResourcePersistentId pid = retVal.iterator().next(); @@ -112,6 +162,14 @@ public class MatchResourceUrlService { return retVal; } + private IFhirResourceDao getResourceDao(Class theResourceType) { + IFhirResourceDao dao = myDaoRegistry.getResourceDao(theResourceType); + if (dao == null) { + throw new InternalErrorException("No DAO for resource type: " + theResourceType.getName()); + } + return dao; + } + private String massageForStorage(String theResourceType, String theMatchUrl) { Validate.notBlank(theMatchUrl, "theMatchUrl must not be null or blank"); int questionMarkIdx = theMatchUrl.indexOf("?"); @@ -136,10 +194,7 @@ public class MatchResourceUrlService { public Set search(SearchParameterMap theParamMap, Class theResourceType, RequestDetails theRequest, @Nullable IBaseResource theConditionalOperationTargetOrNull) { StopWatch sw = new StopWatch(); - IFhirResourceDao dao = myDaoRegistry.getResourceDao(theResourceType); - if (dao == null) { - throw new InternalErrorException("No DAO for resource type: " + theResourceType.getName()); - } + IFhirResourceDao dao = getResourceDao(theResourceType); Set retVal = dao.searchForIds(theParamMap, theRequest, theConditionalOperationTargetOrNull);