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

add authorization check to inline matches (#3048) 

* add authorization check to inline matches

* code review feedback: cached results case

* Add query count test

* Update hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3047-inline-match-security.yaml

Accept code review suggestion

Co-authored-by: James Agnew <jamesagnew@gmail.com>

Co-authored-by: James Agnew <jamesagnew@gmail.com>
This commit is contained in:
JasonRoberts-smile 2021-10-18 08:21:26 -04:00 committed by GitHub
parent 5ba07e2e7c
commit b5e31abf9b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 275 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3047-inline-match-security.yaml Normal file
View File

@ -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."

50
hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java
View File

@ -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() {

153
hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java
View File

@ -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<IAuthRule> 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<IAuthRule> 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();

79
hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/MatchResourceUrlService.java
View File

@ -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 <R extends IBaseResource> Set<ResourcePersistentId> processMatchUrl(String theMatchUrl, Class<R> theResourceType, TransactionDetails theTransactionDetails, RequestDetails theRequest, IBaseResource theConditionalOperationTargetOrNull) {
Set<ResourcePersistentId> 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<ResourcePersistentId> 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<IBaseResource, ResourcePersistentId> resourceToPidMap = new HashMap<>();
IFhirResourceDao<R> 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 <R extends IBaseResource> IFhirResourceDao<R> getResourceDao(Class<R> theResourceType) {
IFhirResourceDao<R> 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 <R extends IBaseResource> Set<ResourcePersistentId> search(SearchParameterMap theParamMap, Class<R> theResourceType, RequestDetails theRequest, @Nullable IBaseResource theConditionalOperationTargetOrNull) {
StopWatch sw = new StopWatch();
IFhirResourceDao<R> dao = myDaoRegistry.getResourceDao(theResourceType);
if (dao == null) {
throw new InternalErrorException("No DAO for resource type: " + theResourceType.getName());
}
IFhirResourceDao<R> dao = getResourceDao(theResourceType);
Set<ResourcePersistentId> retVal = dao.searchForIds(theParamMap, theRequest, theConditionalOperationTargetOrNull);