Unable to POST a transaction Bundle with nested collection Bundle entries (#5693)

* failing test

* fix

* fix

* improve / fix old tests

* changelog

* cleanup

* comment

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5731-fix-transactions-with-nested-bundles.yaml

@@ -0,0 +1,8 @@
+---
+type: fix
+issue: 5731
+title: "Previously, transactions that had entries with `Bundle` requests failed with 
+`HAPI-0339: Can not handle transaction with nested resource of type Bundle`. This has 
+been fixed and now transactions are permitted that contain entries with Bundle requests 
+that have a specified url (i.e. `POST` to `/Bundle`), but are rejected if no url is specified 
+(i.e. nested transactions or batches)."

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java

@@ -5,6 +5,7 @@ import ca.uhn.fhir.jpa.delete.ThreadSafeResourceDeleterSvc;
 import ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor;
 import ca.uhn.fhir.jpa.model.util.JpaConstants;
 import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
+import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.searchparam.matcher.AuthorizationSearchParamMatcher;
 import ca.uhn.fhir.jpa.searchparam.matcher.SearchParamMatcher;
 import ca.uhn.fhir.jpa.term.TermTestUtil;
@@ -17,6 +18,7 @@ import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 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.InvalidRequestException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException;
 import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor;
 import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRule;
@@ -24,6 +26,7 @@ import ca.uhn.fhir.rest.server.interceptor.auth.IAuthRuleTester;
 import ca.uhn.fhir.rest.server.interceptor.auth.PolicyEnum;
 import ca.uhn.fhir.rest.server.interceptor.auth.RuleBuilder;
 import ca.uhn.fhir.rest.server.provider.ProviderConstants;
+import ca.uhn.fhir.util.BundleBuilder;
 import ca.uhn.fhir.util.UrlUtil;
 import org.apache.commons.io.IOUtils;
 import org.apache.http.client.methods.CloseableHttpResponse;
@@ -32,13 +35,16 @@ import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.StringEntity;
+import org.hl7.fhir.instance.model.api.IBaseBundle;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.Bundle;
+import org.hl7.fhir.r4.model.CodeType;
 import org.hl7.fhir.r4.model.CodeableConcept;
 import org.hl7.fhir.r4.model.Coding;
 import org.hl7.fhir.r4.model.Composition;
 import org.hl7.fhir.r4.model.Condition;
+import org.hl7.fhir.r4.model.DateType;
 import org.hl7.fhir.r4.model.Encounter;
 import org.hl7.fhir.r4.model.ExplanationOfBenefit;
 import org.hl7.fhir.r4.model.IdType;
@@ -59,6 +65,8 @@ import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -88,6 +96,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 	private ThreadSafeResourceDeleterSvc myThreadSafeResourceDeleterSvc;
 	private AuthorizationInterceptor myReadAllBundleInterceptor;
 	private AuthorizationInterceptor myReadAllPatientInterceptor;
+	private AuthorizationInterceptor myWriteResourcesInTransactionAuthorizationInterceptor;
 
 	@BeforeEach
 	@Override
@@ -98,6 +107,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 		myStorageSettings.setDeleteExpungeEnabled(true);
 		myReadAllBundleInterceptor = new ReadAllAuthorizationInterceptor("Bundle");
 		myReadAllPatientInterceptor = new ReadAllAuthorizationInterceptor("Patient");
+		myWriteResourcesInTransactionAuthorizationInterceptor = new WriteResourcesInTransactionAuthorizationInterceptor();
 	}
 
 	@Override
@@ -1653,12 +1663,136 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 		assertTrue(AuthorizationInterceptor.shouldExamineBundleChildResources(requestDetails, myFhirContext, bundle));
 	}
 
+	@ParameterizedTest
+	@ValueSource(strings = {"collection", "document", "message"})
+	public void testPermissionsToPostTransaction_withValidNestedBundleRequest_successfullyPostsTransactions(String theBundleType){
+		BundleBuilder builder = new BundleBuilder(myFhirContext);
+		builder.setType(theBundleType);
+		IBaseBundle nestedBundle = builder.getBundle();
+
+		builder = new BundleBuilder(myFhirContext);
+		builder.addTransactionCreateEntry(nestedBundle);
+		IBaseBundle transaction = builder.getBundle();
+
+		myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor);
+
+		myClient
+			.transaction()
+			.withBundle(transaction)
+			.execute();
+
+		List<IBaseResource> savedBundles = myBundleDao.search(SearchParameterMap.newSynchronous(), mySrd).getAllResources();
+		assertEquals(1, savedBundles.size());
+
+		Bundle savedBundle = (Bundle) savedBundles.get(0);
+		assertEquals(theBundleType, savedBundle.getType().toCode());
+		assertTrue(savedBundle.getEntry().isEmpty());
+	}
+
+	@ParameterizedTest
+	@NullSource
+	@ValueSource(strings = {"", "/"})
+	public void testPermissionsToPostTransaction_withInvalidNestedBundleRequest_blocksTransaction(String theInvalidUrl){
+		// inner transaction
+		Patient patient = new Patient();
+		BundleBuilder builder = new BundleBuilder(myFhirContext);
+		builder.addTransactionCreateEntry(patient);
+		Bundle innerTransaction = (Bundle) builder.getBundle();
+
+		// outer transaction
+		Bundle outerTransaction = new Bundle();
+		outerTransaction.setType(Bundle.BundleType.TRANSACTION);
+		Bundle.BundleEntryComponent entry = outerTransaction.addEntry();
+		entry.setResource(innerTransaction);
+		entry.getRequest().setUrl(theInvalidUrl).setMethod(Bundle.HTTPVerb.POST);
+
+		myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor);
+
+		try {
+			myClient
+				.transaction()
+				.withBundle(outerTransaction)
+				.execute();
+			fail();
+		} catch (InvalidRequestException e) {
+			String expectedMessage = "HTTP 400 Bad Request: HAPI-2504: Can not handle nested Bundle request with url:";
+			assertTrue(e.getMessage().contains(expectedMessage));
+		}
+
+		// verify nested Patient transaction did NOT execute
+		assertTrue(myPatientDao.search(SearchParameterMap.newSynchronous(), mySrd).isEmpty());
+	}
+
+	@Test
+	public void testPermissionToPostTransaction_withUpdateParameters_blocksTransaction(){
+		DateType originalBirthDate = new DateType("2000-01-01");
+		Patient patient = createPatient(originalBirthDate);
+
+		DateType newBirthDate = new DateType("2005-01-01");
+		Parameters birthDatePatch = createPatientBirthdatePatch(newBirthDate);
+
+		BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext);
+		bundleBuilder.addTransactionUpdateEntry(birthDatePatch);
+		IBaseBundle transaction = bundleBuilder.getBundle();
+
+		myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor);
+
+		try {
+			myClient
+				.transaction()
+				.withBundle(transaction)
+				.execute();
+			fail();
+		} catch (InvalidRequestException e) {
+			String expectedMessage = "HTTP 400 Bad Request: HAPI-0339: Can not handle nested Parameters with UPDATE operation";
+			assertEquals(expectedMessage, e.getMessage());
+		}
+
+		List<IBaseResource> allPatients = myPatientDao.search(SearchParameterMap.newSynchronous(), mySrd).getAllResources();
+		assertEquals(1, allPatients.size());
+
+		Patient savedPatient = (Patient) allPatients.get(0);
+		assertEquals(originalBirthDate.getValueAsString(), savedPatient.getBirthDateElement().getValueAsString());
+	}
+
+	@Test
+	public void testPermissionToPostTransaction_withPatchParameters_successfullyPostsTransaction(){
+		DateType originalBirthDate = new DateType("2000-01-01");
+		Patient patient = createPatient(originalBirthDate);
+
+		DateType newBirthDate = new DateType("2005-01-01");
+		Parameters birthDatePatch = createPatientBirthdatePatch(newBirthDate);
+
+		BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext);
+		bundleBuilder.addTransactionFhirPatchEntry(patient.getIdElement(), birthDatePatch);
+		IBaseBundle transaction = bundleBuilder.getBundle();
+
+		myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor);
+
+		myClient
+			.transaction()
+			.withBundle(transaction)
+			.execute();
+
+		List<IBaseResource> allPatients = myPatientDao.search(SearchParameterMap.newSynchronous(), mySrd).getAllResources();
+		assertEquals(1, allPatients.size());
+
+		Patient savedPatient = (Patient) allPatients.get(0);
+		assertEquals(newBirthDate.getValueAsString(), savedPatient.getBirthDateElement().getValueAsString());
+	}
+
 	private Patient createPatient(String theFirstName, String theLastName){
 		Patient patient = new Patient();
 		patient.addName().addGiven(theFirstName).setFamily(theLastName);
 		return (Patient) myPatientDao.create(patient, mySrd).getResource();
 	}
 
+	private Patient createPatient(DateType theBirthDate) {
+		Patient patient = new Patient();
+		patient.setBirthDateElement(theBirthDate);
+		return (Patient) myPatientDao.create(patient, mySrd).getResource();
+	}
+
 	private Bundle createDocumentBundle(Patient thePatient){
 		Composition composition = new Composition();
 		composition.setType(new CodeableConcept().addCoding(new Coding().setSystem("http://example.org").setCode("some-type")));
@@ -1727,6 +1861,17 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 		return bundle;
 	}
 
+	private Parameters createPatientBirthdatePatch(DateType theNewBirthDate){
+		final Parameters patch = new Parameters();
+
+		final Parameters.ParametersParameterComponent op = patch.addParameter().setName("operation");
+		op.addPart().setName("type").setValue(new CodeType("replace"));
+		op.addPart().setName("path").setValue(new CodeType("Patient.birthDate"));
+		op.addPart().setName("value").setValue(theNewBirthDate);
+
+		return patch;
+	}
+
 	static class ReadAllAuthorizationInterceptor extends AuthorizationInterceptor {
 
 		private final String myResourceType;
@@ -1762,4 +1907,19 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 				.build();
 		}
 	}
+
+	static class WriteResourcesInTransactionAuthorizationInterceptor extends AuthorizationInterceptor {
+
+		public WriteResourcesInTransactionAuthorizationInterceptor(){
+			super(PolicyEnum.DENY);
+		}
+
+		@Override
+		public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+			return new RuleBuilder()
+				.allow().transaction().withAnyOperation().andApplyNormalRules().andThen()
+				.allow().write().allResources().withAnyId().andThen()
+				.build();
+		}
+	}
 }

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/auth/RuleImplOpTest.java

@@ -1,135 +0,0 @@
-package ca.uhn.fhir.jpa.auth;
-
-import ca.uhn.fhir.context.FhirContext;
-import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
-import ca.uhn.fhir.rest.api.server.RequestDetails;
-import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
-import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
-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.IRuleApplier;
-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 org.hl7.fhir.instance.model.api.IBaseBundle;
-import org.hl7.fhir.r4.model.CodeType;
-import org.hl7.fhir.r4.model.IdType;
-import org.hl7.fhir.r4.model.Parameters;
-import org.hl7.fhir.r4.model.Patient;
-import org.hl7.fhir.r4.model.StringType;
-import org.junit.jupiter.api.Test;
-
-import java.util.HashSet;
-import java.util.List;
-
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.equalTo;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.fail;
-
-public class RuleImplOpTest {
-	private static final String OPERATION = "operation";
-	private static final String TYPE = "type";
-	private static final String PATH = "path";
-	private static final String VALUE = "value";
-	private static final String REPLACE = "replace";
-	private static final String PATIENT_BIRTH_DATE = "Patient.birthDate";
-	private static final Parameters PARAMETERS = buildParameters();
-	private static final String DOCUMENT = "document";
-	private static final String ERROR_TEMPLATE = "HAPI-0339: Can not handle transaction with nested resource of type %s";
-	private static final String ERROR_PARAMETERS = String.format(ERROR_TEMPLATE, "Parameters");
-	private static final String ERROR_BUNDLE = String.format(ERROR_TEMPLATE, "Bundle");
-
-	private static final String REQUEST_RULELIST = AuthorizationInterceptor.class.getName() + "_1_RULELIST";
-	private final Patient myPatient = buildPatient();
-
-	private final List<IAuthRule> myRules = new RuleBuilder()
-		.allow()
-		.transaction()
-		.withAnyOperation()
-		.andApplyNormalRules()
-		.andThen()
-		.allow()
-		.write()
-		.allResources()
-		.withAnyId()
-		.build();
-
-	private final IAuthRule myRule = myRules.get(0);
-	private final FhirContext myFhirContext = FhirContext.forR4Cached();
-	private final IBaseBundle myInnerBundle = buildInnerBundler(myFhirContext);
-
-	private final RequestDetails mySystemRequestDetails = buildSystemRequestDetails(myFhirContext, myRules);
-	private final IRuleApplier myRuleApplier = new AuthorizationInterceptor();
-
-	@Test
-	void testTransactionBundleUpdateWithParameters() {
-		final BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext);
-		bundleBuilder.addTransactionUpdateEntry(PARAMETERS);
-
-		try {
-			applyRule(bundleBuilder.getBundle());
-			fail("Expected an InvalidRequestException");
-		} catch (InvalidRequestException exception) {
-			assertEquals(ERROR_PARAMETERS, exception.getMessage());
-		}
-	}
-
-	@Test
-	void testTransactionBundleWithNestedBundle() {
-		final BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext);
-		bundleBuilder.addTransactionCreateEntry(myInnerBundle);
-
-		try {
-			applyRule(bundleBuilder.getBundle());
-			fail("Expected an InvalidRequestException");
-		} catch (InvalidRequestException exception) {
-			assertEquals(ERROR_BUNDLE, exception.getMessage());
-		}
-	}
-
-	@Test
-	void testTransactionBundlePatchWithParameters() {
-		final BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext);
-		bundleBuilder.addTransactionFhirPatchEntry(myPatient.getIdElement(), PARAMETERS);
-
-		final AuthorizationInterceptor.Verdict verdict = applyRule(bundleBuilder.getBundle());
-
-		assertThat(verdict.getDecision(), equalTo(PolicyEnum.ALLOW));
-	}
-
-	private AuthorizationInterceptor.Verdict applyRule(IBaseBundle theBundle) {
-		return myRule.applyRule(RestOperationTypeEnum.TRANSACTION, mySystemRequestDetails, theBundle, myPatient.getIdElement(), myPatient, myRuleApplier, new HashSet<>(), null);
-	}
-
-	private static Parameters buildParameters() {
-		final Parameters patch = new Parameters();
-
-		final Parameters.ParametersParameterComponent op = patch.addParameter().setName(OPERATION);
-		op.addPart().setName(TYPE).setValue(new CodeType(REPLACE));
-		op.addPart().setName(PATH).setValue(new CodeType(PATIENT_BIRTH_DATE));
-		op.addPart().setName(VALUE).setValue(new StringType("1912-04-14"));
-
-		return patch;
-	}
-
-	private static RequestDetails buildSystemRequestDetails(FhirContext theFhirContext, List<IAuthRule> theRules) {
-        final SystemRequestDetails systemRequestDetails = new SystemRequestDetails();
-		systemRequestDetails.setFhirContext(theFhirContext);
-		systemRequestDetails.getUserData().put(REQUEST_RULELIST, theRules);
-
-		return systemRequestDetails;
-	}
-
-	private static Patient buildPatient() {
-		final Patient patient = new Patient();
-		patient.setId(new IdType("Patient", "1"));
-		return patient;
-	}
-
-	private static IBaseBundle buildInnerBundler(FhirContext theFhirContext) {
-		final BundleBuilder innerBundleBuilder = new BundleBuilder(theFhirContext);
-		innerBundleBuilder.setType(DOCUMENT);
-		return innerBundleBuilder.getBundle();
-	}
-}

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java

@@ -57,6 +57,7 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 import static org.apache.commons.lang3.StringUtils.defaultString;
+import static org.apache.commons.lang3.StringUtils.isBlank;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 import static org.hl7.fhir.instance.model.api.IAnyResource.SP_RES_ID;
 
@@ -722,6 +723,11 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 			boolean allComponentsAreGets = true;
 			for (BundleEntryParts nextPart : inputResources) {
 
+				if (isInvalidNestedBundleRequest(nextPart)) {
+					throw new InvalidRequestException(
+							Msg.code(2504) + "Can not handle nested Bundle request with url: " + nextPart.getUrl());
+				}
+
 				IBaseResource inputResource = nextPart.getResource();
 				IIdType inputResourceId = null;
 				if (isNotBlank(nextPart.getUrl())) {
@@ -766,20 +772,11 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 							+ "Can not handle transaction with operation of type " + nextPart.getRequestType());
 				}
 
-				/*
+				// This is done because a Bundle can contain a nested PATCH with Parameters,
-				 * This is basically just being conservative - Be careful of transactions containing
+				// which is supported but a non-PATCH nested Parameters resource may be problematic.
-				 * nested operations and nested transactions. We block them by default. At some point
+				if (isInvalidNestedParametersRequest(ctx, nextPart, operation)) {
-				 * it would be nice to be more nuanced here.
+					throw new InvalidRequestException(Msg.code(339)
-				 */
+							+ String.format("Can not handle nested Parameters with %s operation", operation));
-				if (nextPart.getResource() != null) {
-					RuntimeResourceDefinition resourceDef = ctx.getResourceDefinition(nextPart.getResource());
-
-					// TODO:  LD:  We should pursue a more ideal fix after the release to inspect the bundle more deeply
-					// to ensure that it's a valid request
-					if (shouldRejectBundleEntry(resourceDef, operation)) {
-						throw new InvalidRequestException(Msg.code(339)
-								+ "Can not handle transaction with nested resource of type " + resourceDef.getName());
-					}
 				}
 
 				String previousFixedConditionalUrl = theRequestDetails.getFixedConditionalUrl();
@@ -840,22 +837,31 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 		}
 	}
 
-	/**
+	private boolean isInvalidNestedBundleRequest(BundleEntryParts theEntry) {
-	 * Ascertain whether this transaction request contains a nested operations or nested transactions.
+		IBaseResource resource = theEntry.getResource();
-	 * This is done carefully because a bundle can contain a nested PATCH with Parameters, which is supported but
+		if (!(resource instanceof IBaseBundle)) {
-	 * a non-PATCH nested Parameters resource may be problematic.
+			return false;
-	 *
+		}
-	 * @param theResourceDef The {@link RuntimeResourceDefinition} associated with this bundle entry
+
-	 * @param theOperation The {@link RestOperationTypeEnum} associated with this bundle entry
+		// transactions are permitted that contain entries with Bundle requests that
-	 * @return true if we should reject this reject
+		// have a specified url (i.e. POST to /Bundle), but are rejected if no url is specified
-	 */
+		// (i.e. nested transactions or batches).
-	private boolean shouldRejectBundleEntry(
+		String url = theEntry.getUrl();
-			RuntimeResourceDefinition theResourceDef, RestOperationTypeEnum theOperation) {
+		return isBlank(url) || "/".equals(url);
-		final boolean isResourceParameters = PARAMETERS.equals(theResourceDef.getName());
+	}
-		final boolean isResourceBundle = BUNDLE.equals(theResourceDef.getName());
+
+	private boolean isInvalidNestedParametersRequest(
+			FhirContext theContext, BundleEntryParts theEntry, RestOperationTypeEnum theOperation) {
+		IBaseResource resource = theEntry.getResource();
+		if (resource == null) {
+			return false;
+		}
+
+		RuntimeResourceDefinition resourceDefinition = theContext.getResourceDefinition(resource);
+		final boolean isResourceParameters = PARAMETERS.equals(resourceDefinition.getName());
 		final boolean isOperationPatch = theOperation == RestOperationTypeEnum.PATCH;
 
-		return (isResourceParameters && !isOperationPatch) || isResourceBundle;
+		return isResourceParameters && !isOperationPatch;
 	}
 
 	private void setTargetFromResourceId(RequestDetails theRequestDetails, FhirContext ctx, RuleTarget target) {