Handle invalid chains on custom search params (#1553)

* Handle invalid chains on custom search params

* Add some more tests

* One more test fix
This commit is contained in:
James Agnew 2019-10-21 21:21:33 -04:00 committed by GitHub
parent 0af79b84d9
commit 73961072a6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 370 additions and 44 deletions

View File

@ -1,5 +1,12 @@
package ca.uhn.fhir.rest.api;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.model.api.IQueryParameterOr;
import ca.uhn.fhir.model.api.IQueryParameterType;
import java.util.ArrayList;
import java.util.StringTokenizer;
import static org.apache.commons.lang3.StringUtils.isBlank;
/*
@ -22,13 +29,6 @@ import static org.apache.commons.lang3.StringUtils.isBlank;
* #L%
*/
import java.util.ArrayList;
import java.util.StringTokenizer;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.model.api.IQueryParameterOr;
import ca.uhn.fhir.model.api.IQueryParameterType;
public class QualifiedParamList extends ArrayList<String> {
private static final long serialVersionUID = 1L;
@ -83,16 +83,16 @@ public class QualifiedParamList extends ArrayList<String> {
prev = null;
continue;
}
if (str.equals(",")) {
if (countTrailingSlashes(prev) % 2 == 1) {
int idx = retVal.size() - 1;
String existing = retVal.get(idx);
prev = existing.substring(0, existing.length() - 1) + ',';
retVal.set(idx, prev);
} else {
prev = null;
}
if (countTrailingSlashes(prev) % 2 == 1) {
int idx = retVal.size() - 1;
String existing = retVal.get(idx);
prev = existing.substring(0, existing.length() - 1) + ',';
retVal.set(idx, prev);
} else {
prev = null;
}
continue;
}
@ -108,11 +108,18 @@ public class QualifiedParamList extends ArrayList<String> {
}
// If no value was found, at least add that empty string as a value. It should get ignored later, but at
// least this lets us give a sensible error message if the parameter name was bad. See
// ResourceProviderR4Test#testParameterWithNoValueThrowsError_InvalidChainOnCustomSearch for an example
if (retVal.size() == 0) {
retVal.add("");
}
return retVal;
}
private static int countTrailingSlashes(String theString) {
if(theString==null) {
if (theString == null) {
return 0;
}
int retVal = 0;

View File

@ -559,11 +559,11 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
myEntityManager.merge(entity);
}
private void validateExpungeEnabled() {
if (!myDaoConfig.isExpungeEnabled()) {
throw new MethodNotAllowedException("$expunge is not enabled on this server");
}
}
private void validateExpungeEnabled() {
if (!myDaoConfig.isExpungeEnabled()) {
throw new MethodNotAllowedException("$expunge is not enabled on this server");
}
}
@Override
@Transactional(propagation = Propagation.NEVER)
@ -892,8 +892,8 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
boolean hasExternalizedBinaryReference = ((IBaseHasExtensions) nextBase64)
.getExtension()
.stream()
.filter(t-> t.getUserData(JpaConstants.EXTENSION_EXT_SYSTEMDEFINED) == null)
.anyMatch(t-> t.getUrl().equals(EXT_EXTERNALIZED_BINARY_ID));
.filter(t -> t.getUserData(JpaConstants.EXTENSION_EXT_SYSTEMDEFINED) == null)
.anyMatch(t -> t.getUrl().equals(EXT_EXTERNALIZED_BINARY_ID));
if (hasExternalizedBinaryReference) {
String msg = getContext().getLocalizer().getMessage(BaseHapiFhirDao.class, "externalizedBinaryStorageExtensionFoundInRequestBody", EXT_EXTERNALIZED_BINARY_ID);
throw new InvalidRequestException(msg);
@ -1326,12 +1326,10 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
RuntimeSearchParam paramDef = mySearchParamRegistry.getSearchParamByName(resourceDef, qualifiedParamName.getParamName());
for (String nextValue : theSource.get(nextParamName)) {
if (isNotBlank(nextValue)) {
QualifiedParamList qualifiedParam = QualifiedParamList.splitQueryStringByCommasIgnoreEscape(qualifiedParamName.getWholeQualifier(), nextValue);
List<QualifiedParamList> paramList = Collections.singletonList(qualifiedParam);
IQueryParameterAnd<?> parsedParam = ParameterUtil.parseQueryParams(getContext(), paramDef, nextParamName, paramList);
theTarget.add(qualifiedParamName.getParamName(), parsedParam);
}
QualifiedParamList qualifiedParam = QualifiedParamList.splitQueryStringByCommasIgnoreEscape(qualifiedParamName.getWholeQualifier(), nextValue);
List<QualifiedParamList> paramList = Collections.singletonList(qualifiedParam);
IQueryParameterAnd<?> parsedParam = ParameterUtil.parseQueryParams(getContext(), paramDef, nextParamName, paramList);
theTarget.add(qualifiedParamName.getParamName(), parsedParam);
}
}

View File

@ -51,7 +51,8 @@ public class JpaValidationSupportChainR5 extends ValidationSupportChain {
public JpaValidationSupportChainR5() {
super();
}
@PreDestroy
public void flush() {
myDefaultProfileValidationSupport.flush();
}
@ -83,10 +84,4 @@ public class JpaValidationSupportChainR5 extends ValidationSupportChain {
addValidationSupport(new SnapshotGeneratingValidationSupport(myFhirContext, this));
}
@PreDestroy
public void preDestroy() {
flush();
}
}

View File

@ -1,8 +1,13 @@
package ca.uhn.fhir.jpa.dao.r4;
import ca.uhn.fhir.interceptor.api.Hook;
import ca.uhn.fhir.interceptor.api.HookParams;
import ca.uhn.fhir.interceptor.api.IAnonymousInterceptor;
import ca.uhn.fhir.interceptor.api.Pointcut;
import ca.uhn.fhir.jpa.dao.DaoConfig;
import ca.uhn.fhir.jpa.model.entity.ModelConfig;
import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
import ca.uhn.fhir.jpa.model.search.StorageProcessingMessage;
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import ca.uhn.fhir.model.api.Include;
import ca.uhn.fhir.rest.api.server.IBundleProvider;
@ -10,11 +15,13 @@ import ca.uhn.fhir.rest.param.*;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
import ca.uhn.fhir.util.TestUtil;
import org.hamcrest.Matchers;
import org.hl7.fhir.instance.model.api.IIdType;
import org.hl7.fhir.r4.model.*;
import org.hl7.fhir.r4.model.Appointment.AppointmentStatus;
import org.hl7.fhir.r4.model.Enumerations.AdministrativeGender;
import org.junit.*;
import org.mockito.ArgumentCaptor;
import org.mockito.internal.util.collections.ListUtil;
import org.springframework.transaction.TransactionStatus;
import org.springframework.transaction.support.TransactionCallback;
@ -23,8 +30,11 @@ import org.springframework.transaction.support.TransactionTemplate;
import java.util.List;
import static org.hamcrest.Matchers.startsWith;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.*;
import static org.junit.Assert.*;
import static org.mockito.Mockito.*;
public class FhirResourceDaoR4SearchCustomSearchParamTest extends BaseJpaR4Test {
private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoR4SearchCustomSearchParamTest.class);
@ -181,7 +191,7 @@ public class FhirResourceDaoR4SearchCustomSearchParamTest extends BaseJpaR4Test
IBundleProvider outcome = myPatientDao.search(params);
List<String> ids = toUnqualifiedVersionlessIdValues(outcome);
ourLog.info("IDS: " + ids);
assertThat(ids, contains(pid.getValue()));
assertThat(ids, Matchers.contains(pid.getValue()));
}
@ -1326,6 +1336,39 @@ public class FhirResourceDaoR4SearchCustomSearchParamTest extends BaseJpaR4Test
}
@Test
public void testCompositeWithInvalidTarget() {
SearchParameter sp = new SearchParameter();
sp.addBase("Patient");
sp.setCode("myDoctor");
sp.setType(Enumerations.SearchParamType.COMPOSITE);
sp.setTitle("My Doctor");
sp.setStatus(org.hl7.fhir.r4.model.Enumerations.PublicationStatus.ACTIVE);
sp.addComponent()
.setDefinition("http://foo");
mySearchParameterDao.create(sp);
IAnonymousInterceptor interceptor = mock(IAnonymousInterceptor.class);
myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.JPA_PERFTRACE_WARNING, interceptor);
try {
mySearchParamRegistry.forceRefresh();
ArgumentCaptor<HookParams> paramsCaptor = ArgumentCaptor.forClass(HookParams.class);
verify(interceptor, times(1)).invoke(any(), paramsCaptor.capture());
StorageProcessingMessage msg = paramsCaptor.getValue().get(StorageProcessingMessage.class);
assertThat(msg.getMessage(), containsString("refers to unknown component foo, ignoring this parameter"));
} finally {
myInterceptorRegistry.unregisterInterceptor(interceptor);
}
}
@AfterClass
public static void afterClassClearContext() {
TestUtil.clearAllStaticFieldsForUnitTest();

View File

@ -116,6 +116,48 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test {
myDaoConfig.setSearchPreFetchThresholds(new DaoConfig().getSearchPreFetchThresholds());
}
@Test
public void testParameterWithNoValueThrowsError_InvalidChainOnCustomSearch() throws IOException {
SearchParameter searchParameter = new SearchParameter();
searchParameter.addBase("BodySite").addBase("Procedure");
searchParameter.setCode("focalAccess");
searchParameter.setType(Enumerations.SearchParamType.REFERENCE);
searchParameter.setExpression("Procedure.extension('Procedure#focalAccess')");
searchParameter.setXpathUsage(SearchParameter.XPathUsageType.NORMAL);
searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE);
ourClient.create().resource(searchParameter).execute();
mySearchParamRegistry.forceRefresh();
HttpGet get = new HttpGet(ourServerBase + "/Procedure?focalAccess.a%20ne%20e");
try (CloseableHttpResponse resp = ourHttpClient.execute(get)) {
String output = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);
assertThat(output, containsString("Invalid parameter chain: focalAccess.a ne e"));
assertEquals(400, resp.getStatusLine().getStatusCode());
}
}
@Test
public void testParameterWithNoValueThrowsError_InvalidRootParam() throws IOException {
SearchParameter searchParameter = new SearchParameter();
searchParameter.addBase("BodySite").addBase("Procedure");
searchParameter.setCode("focalAccess");
searchParameter.setType(Enumerations.SearchParamType.REFERENCE);
searchParameter.setExpression("Procedure.extension('Procedure#focalAccess')");
searchParameter.setXpathUsage(SearchParameter.XPathUsageType.NORMAL);
searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE);
ourClient.create().resource(searchParameter).execute();
mySearchParamRegistry.forceRefresh();
HttpGet get = new HttpGet(ourServerBase + "/Procedure?a");
try (CloseableHttpResponse resp = ourHttpClient.execute(get)) {
String output = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);
assertThat(output, containsString("Unknown search parameter &quot;a&quot;"));
assertEquals(400, resp.getStatusLine().getStatusCode());
}
}
@Test
public void testSearchForTokenValueOnlyUsesValueHash() {

View File

@ -239,6 +239,24 @@ public class ResourceReindexingSvcImplTest extends BaseJpaTest {
verifyNoMoreInteractions(myReindexJobDao);
}
@Test
public void testReindexThrowsError() {
mockNothingToExpunge();
mockSingleReindexingJob("Patient");
List<Long> values = Arrays.asList(0L, 1L, 2L, 3L);
when(myResourceTableDao.findIdsOfResourcesWithinUpdatedRangeOrderedFromOldest(myPageRequestCaptor.capture(), myTypeCaptor.capture(), myLowCaptor.capture(), myHighCaptor.capture())).thenReturn(new SliceImpl<>(values));
when(myResourceTableDao.findById(anyLong())).thenThrow(new NullPointerException("A MESSAGE"));
int count = mySvc.forceReindexingPass();
assertEquals(0, count);
// Make sure we didn't do anything unexpected
verify(myReindexJobDao, times(1)).findAll(any(), eq(false));
verify(myReindexJobDao, times(1)).findAll(any(), eq(true));
verify(myReindexJobDao, times(1)).setSuspendedUntil(any());
verifyNoMoreInteractions(myReindexJobDao);
}
private void mockWhenResourceTableFindById(long[] theUpdatedTimes, String[] theResourceTypes) {
when(myResourceTableDao.findById(any())).thenAnswer(t -> {
ResourceTable retVal = new ResourceTable();

View File

@ -149,8 +149,7 @@ public final class ResourceIndexedSearchParams {
* @param theResourceType E.g. <code>Patient
* @param thePartsChoices E.g. <code>[[gender=male], [name=SMITH, name=JOHN]]</code>
*/
public static Set<String> extractCompositeStringUniquesValueChains(String
theResourceType, List<List<String>> thePartsChoices) {
public static Set<String> extractCompositeStringUniquesValueChains(String theResourceType, List<List<String>> thePartsChoices) {
for (List<String> next : thePartsChoices) {
next.removeIf(StringUtils::isBlank);

View File

@ -3,14 +3,17 @@ package ca.uhn.fhir.jpa.searchparam.extractor;
import ca.uhn.fhir.jpa.model.entity.ForcedId;
import ca.uhn.fhir.jpa.model.entity.ResourceLink;
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
import ca.uhn.fhir.model.primitive.IdDt;
import ca.uhn.fhir.rest.param.ReferenceParam;
import org.hl7.fhir.instance.model.api.IIdType;
import com.google.common.collect.Lists;
import org.junit.Before;
import org.junit.Test;
import java.util.Date;
import java.util.List;
import java.util.Set;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.empty;
import static org.junit.Assert.*;
public class ResourceIndexedSearchParamsTest {
@ -79,4 +82,30 @@ public class ResourceIndexedSearchParamsTest {
return retval;
}
@Test
public void testExtractCompositeStringUniquesValueChains() {
List<List<String>> partsChoices;
Set<String> values;
partsChoices = Lists.newArrayList(
Lists.newArrayList("gender=male"),
Lists.newArrayList("name=SMITH", "name=JOHN")
);
values = ResourceIndexedSearchParams.extractCompositeStringUniquesValueChains("Patient", partsChoices);
assertThat(values.toString(), values, containsInAnyOrder("Patient?gender=male&name=JOHN","Patient?gender=male&name=SMITH"));
partsChoices = Lists.newArrayList(
Lists.newArrayList("gender=male", ""),
Lists.newArrayList("name=SMITH", "name=JOHN", "")
);
values = ResourceIndexedSearchParams.extractCompositeStringUniquesValueChains("Patient", partsChoices);
assertThat(values.toString(), values, containsInAnyOrder("Patient?gender=male&name=JOHN","Patient?gender=male&name=SMITH"));
partsChoices = Lists.newArrayList(
);
values = ResourceIndexedSearchParams.extractCompositeStringUniquesValueChains("Patient", partsChoices);
assertThat(values.toString(), values, empty());
}
}

View File

@ -83,6 +83,12 @@ public class SubscriptionLoader {
}
}
@VisibleForTesting
void acquireSemaphoreForUnitTest() throws InterruptedException {
mySyncSubscriptionsSemaphore.acquire();
}
@PostConstruct
public void registerScheduledJob() {
ScheduledJobDefinition jobDetail = new ScheduledJobDefinition();

View File

@ -1,6 +1,8 @@
package ca.uhn.fhir.jpa.subscription.module.standalone;
package ca.uhn.fhir.jpa.subscription.module.cache;
import ca.uhn.fhir.jpa.subscription.module.cache.SubscriptionLoader;
import ca.uhn.fhir.jpa.subscription.module.config.MockFhirClientSubscriptionProvider;
import ca.uhn.fhir.jpa.subscription.module.standalone.BaseBlockingQueueSubscribableChannelDstu3Test;
import org.hl7.fhir.dstu3.model.Subscription;
import org.junit.After;
import org.junit.Before;
@ -43,4 +45,13 @@ public class SubscriptionLoaderTest extends BaseBlockingQueueSubscribableChannel
mySubscriptionActivatedPost.awaitExpected();
assertEquals(0, myMockFhirClientSubscriptionProvider.getFailCount());
}
@Test
public void testMultipleThreadsDontBlock() throws InterruptedException {
SubscriptionLoader svc = new SubscriptionLoader();
svc.acquireSemaphoreForUnitTest();
svc.syncSubscriptions();
}
}

View File

@ -2,6 +2,7 @@ package ca.uhn.fhir.rest.server.method;
import ca.uhn.fhir.context.ConfigurationException;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.context.RuntimeResourceDefinition;
import ca.uhn.fhir.interceptor.api.HookParams;
import ca.uhn.fhir.interceptor.api.Pointcut;
import ca.uhn.fhir.model.api.IResource;
@ -103,7 +104,8 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi
// type let's grab it
if (!Modifier.isAbstract(theReturnResourceType.getModifiers()) && !Modifier.isInterface(theReturnResourceType.getModifiers())) {
Class<? extends IBaseResource> resourceType = (Class<? extends IResource>) theReturnResourceType;
myResourceName = theContext.getResourceDefinition(resourceType).getName();
RuntimeResourceDefinition resourceDefinition = theContext.getResourceDefinition(resourceType);
myResourceName = resourceDefinition.getName();
}
}
}

View File

@ -0,0 +1,170 @@
package ca.uhn.fhir.rest.server.method;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.context.FhirVersionEnum;
import ca.uhn.fhir.context.RuntimeResourceDefinition;
import ca.uhn.fhir.model.api.annotation.ResourceDef;
import ca.uhn.fhir.model.primitive.IdDt;
import ca.uhn.fhir.rest.annotation.IdParam;
import ca.uhn.fhir.rest.annotation.Read;
import ca.uhn.fhir.rest.api.RequestTypeEnum;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import org.hl7.fhir.instance.model.api.IBaseMetaType;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import java.lang.reflect.Method;
import java.util.List;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;
@SuppressWarnings("unchecked")
@RunWith(MockitoJUnitRunner.class)
public class ReadMethodBindingTest {
@Mock
private FhirContext myCtx;
@Mock
private RequestDetails myRequestDetails;
@Mock
private RuntimeResourceDefinition definition;
@Test
public void testIncomingServerRequestMatchesMethod_Read() throws NoSuchMethodException {
class MyProvider {
@Read(version = false)
public IBaseResource read(@IdParam IIdType theIdType) {
return null;
}
}
when(myCtx.getResourceDefinition(any(Class.class))).thenReturn(definition);
when(definition.getName()).thenReturn("Patient");
when(myRequestDetails.getResourceName()).thenReturn("Patient");
when(myRequestDetails.getRequestType()).thenReturn(RequestTypeEnum.GET);
// Read
ReadMethodBinding binding = createBinding(new MyProvider());
when(myRequestDetails.getId()).thenReturn(new IdDt("Patient/123"));
assertTrue(binding.incomingServerRequestMatchesMethod(myRequestDetails));
// VRead
when(myRequestDetails.getResourceName()).thenReturn("Patient");
when(myRequestDetails.getOperation()).thenReturn("_history");
assertFalse(binding.incomingServerRequestMatchesMethod(myRequestDetails));
}
@Test
public void testIncomingServerRequestMatchesMethod_VRead() throws NoSuchMethodException {
class MyProvider {
@Read(version = true)
public IBaseResource read(@IdParam IIdType theIdType) {
return null;
}
}
when(myCtx.getResourceDefinition(any(Class.class))).thenReturn(definition);
when(definition.getName()).thenReturn("Patient");
when(myRequestDetails.getResourceName()).thenReturn("Patient");
when(myRequestDetails.getRequestType()).thenReturn(RequestTypeEnum.GET);
// Read - wrong resource type
ReadMethodBinding binding = createBinding(new MyProvider());
when(myRequestDetails.getResourceName()).thenReturn("Observation");
when(myRequestDetails.getId()).thenReturn(new IdDt("Observation/123"));
assertFalse(binding.incomingServerRequestMatchesMethod(myRequestDetails));
// Read
when(myRequestDetails.getResourceName()).thenReturn("Patient");
when(myRequestDetails.getId()).thenReturn(new IdDt("Patient/123"));
assertTrue(binding.incomingServerRequestMatchesMethod(myRequestDetails));
// VRead
when(myRequestDetails.getId()).thenReturn(new IdDt("Patient/123/_history/123"));
when(myRequestDetails.getOperation()).thenReturn("_history");
assertTrue(binding.incomingServerRequestMatchesMethod(myRequestDetails));
// Some other operation
when(myRequestDetails.getId()).thenReturn(new IdDt("Patient/123/_history/123"));
when(myRequestDetails.getOperation()).thenReturn("$foo");
assertFalse(binding.incomingServerRequestMatchesMethod(myRequestDetails));
}
public ReadMethodBinding createBinding(Object theProvider) throws NoSuchMethodException {
Method method = theProvider.getClass().getMethod("read", IIdType.class);
return new ReadMethodBinding(MyPatient.class, method, myCtx, theProvider);
}
@ResourceDef(name = "Patient")
private static class MyPatient implements IBaseResource {
@Override
public IBaseMetaType getMeta() {
return null;
}
@Override
public IIdType getIdElement() {
return null;
}
@Override
public IBaseResource setId(String theId) {
return null;
}
@Override
public IBaseResource setId(IIdType theId) {
return null;
}
@Override
public FhirVersionEnum getStructureFhirVersionEnum() {
return null;
}
@Override
public boolean isEmpty() {
return false;
}
@Override
public boolean hasFormatComment() {
return false;
}
@Override
public List<String> getFormatCommentsPre() {
return null;
}
@Override
public List<String> getFormatCommentsPost() {
return null;
}
@Override
public Object getUserData(String theName) {
return null;
}
@Override
public void setUserData(String theName, Object theValue) {
}
}
}

View File

@ -433,6 +433,12 @@
JavaScript dependency libraries. This reduces our Git repository size and
should make it easier to stay up-to-date.
</action>
<action type="fix">
In the (fairly unlikely) circumstance that a JPA server was called with a parameter where the parameter name referenced
a custom search parameter with an invalid chain attached, and the value was missing entirely (e.g.
<![CDATA[<code>ProcedureRequest?someCustomParameter.BAD_NAME=</code>]]>, the server would ignore this
parameter instead of incorrectly returning an error. This has been corrected.
</action>
</release>
<release version="4.0.3" date="2019-09-03" description="Igloo (Point Release)">
<action type="fix">