Fix subscription validation not to validate partition ID when invoked from an update pointcut (#4484)

* First commit:  Make SubscriptionValidatingInterceptor aware of which Pointcut is being called.  In validatePermissions(), skip determinePartition() if the Pointcut is STORAGE_PRESTORAGE_RESOURCE_UPDATED.   Fix resulting compile errors in various unit tests.

* Fix/enhance unit tests.  Mark methods as deprecated instead of deleting them.  Add proper error code.  Complete changelog.

* Remove erroneous TODOs and tweak the validation logic.

* Enhance unit tests and fix changelog.

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4485-subscription-cross-partition-custom-interceptor-put-update-error-hapi-2010.yaml

@@ -0,0 +1,5 @@
+type: fix
+issue: 4485
+jira:  SMILE-5561
+title: "Cross-partition subscription PUT with a custom interceptor will fail on validation because the read partition ID is used.
+        This has been fixed by skipping validation if the validator invoked during an update operation"

hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptor.java

@@ -70,12 +70,12 @@ public class SubscriptionValidatingInterceptor {
 
 	@Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED)
 	public void resourcePreCreate(IBaseResource theResource, RequestDetails theRequestDetails, RequestPartitionId theRequestPartitionId) {
-		validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId);
+		validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId, Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED);
 	}
 
 	@Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED)
-	public void resourcePreCreate(IBaseResource theOldResource, IBaseResource theResource, RequestDetails theRequestDetails, RequestPartitionId theRequestPartitionId) {
+	public void resourceUpdated(IBaseResource theOldResource, IBaseResource theResource, RequestDetails theRequestDetails, RequestPartitionId theRequestPartitionId) {
-		validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId);
+		validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId, Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED);
 	}
 
 	@Autowired
@@ -83,14 +83,30 @@ public class SubscriptionValidatingInterceptor {
 		myFhirContext = theFhirContext;
 	}
 
+	// This will be deleted once the next snapshot (6.3.15) is published
 	@Deprecated
 	public void validateSubmittedSubscription(IBaseResource theSubscription) {
-		validateSubmittedSubscription(theSubscription, null, null);
+		validateSubmittedSubscription(theSubscription, null, null, Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED);
 	}
 
+	// This will be deleted once the next snapshot (6.3.15) is published
+	@Deprecated(since="6.3.14")
 	public void validateSubmittedSubscription(IBaseResource theSubscription,
 															RequestDetails theRequestDetails,
 															RequestPartitionId theRequestPartitionId) {
+
+		validateSubmittedSubscription(theSubscription, theRequestDetails, theRequestPartitionId, Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED);
+	}
+
+	@VisibleForTesting
+	void validateSubmittedSubscription(IBaseResource theSubscription,
+												  RequestDetails theRequestDetails,
+												  RequestPartitionId theRequestPartitionId,
+												  Pointcut thePointcut) {
+		if (Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED != thePointcut && Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED != thePointcut) {
+			throw new UnprocessableEntityException(Msg.code(2267) + "Expected Pointcut to be either STORAGE_PRESTORAGE_RESOURCE_CREATED or STORAGE_PRESTORAGE_RESOURCE_UPDATED but was: " + thePointcut);
+		}
+
 		if (!"Subscription".equals(myFhirContext.getResourceType(theSubscription))) {
 			return;
 		}
@@ -117,7 +133,7 @@ public class SubscriptionValidatingInterceptor {
 				break;
 		}
 
-		validatePermissions(theSubscription, subscription, theRequestDetails, theRequestPartitionId);
+		validatePermissions(theSubscription, subscription, theRequestDetails, theRequestPartitionId, thePointcut);
 
 		mySubscriptionCanonicalizer.setMatchingStrategyTag(theSubscription, null);
 
@@ -151,13 +167,18 @@ public class SubscriptionValidatingInterceptor {
 	protected void validatePermissions(IBaseResource theSubscription,
 												  CanonicalSubscription theCanonicalSubscription,
 												  RequestDetails theRequestDetails,
-												  RequestPartitionId theRequestPartitionId) {
+												  RequestPartitionId theRequestPartitionId,
+												  Pointcut thePointcut) {
 		// If the subscription has the cross partition tag
 		if (SubscriptionUtil.isCrossPartition(theSubscription) && !(theRequestDetails instanceof SystemRequestDetails)) {
 			if (!myDaoConfig.isCrossPartitionSubscriptionEnabled()){
 				throw new UnprocessableEntityException(Msg.code(2009) + "Cross partition subscription is not enabled on this server");
 			}
 
+			if (theRequestPartitionId == null && Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED == thePointcut) {
+				return;
+			}
+
 			// if we have a partition id already, we'll use that
 			// otherwise we might end up with READ and CREATE pointcuts
 			// returning conflicting partitions (say, all vs default)

hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptorTest.java

@@ -1,12 +1,14 @@
 package ca.uhn.fhir.jpa.subscription.submit.interceptor;
 
 import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.interceptor.api.Pointcut;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc;
 import ca.uhn.fhir.jpa.subscription.match.matcher.matching.SubscriptionStrategyEvaluator;
 import ca.uhn.fhir.jpa.subscription.match.registry.SubscriptionCanonicalizer;
+import ca.uhn.fhir.jpa.subscription.model.CanonicalSubscription;
 import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
 import org.hl7.fhir.r4.model.Subscription;
 import org.junit.jupiter.api.BeforeEach;
@@ -20,6 +22,8 @@ import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.test.context.junit.jupiter.SpringExtension;
 
+import javax.annotation.Nonnull;
+
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.fail;
@@ -50,7 +54,7 @@ public class SubscriptionValidatingInterceptorTest {
 	public void testEmptySub() {
 		try {
 			Subscription badSub = new Subscription();
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(8) + "Can not process submitted Subscription - Subscription.status must be populated on this server"));
@@ -63,7 +67,7 @@ public class SubscriptionValidatingInterceptorTest {
 		try {
 			Subscription badSub = new Subscription();
 			badSub.setStatus(Subscription.SubscriptionStatus.ACTIVE);
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(11) + "Subscription.criteria must be populated"));
@@ -76,7 +80,7 @@ public class SubscriptionValidatingInterceptorTest {
 			Subscription badSub = new Subscription();
 			badSub.setStatus(Subscription.SubscriptionStatus.ACTIVE);
 			badSub.setCriteria("Patient");
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(14) + "Subscription.criteria must be in the form \"{Resource Type}?[params]\""));
@@ -89,7 +93,7 @@ public class SubscriptionValidatingInterceptorTest {
 			Subscription badSub = new Subscription();
 			badSub.setStatus(Subscription.SubscriptionStatus.ACTIVE);
 			badSub.setCriteria("Patient?");
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(20) + "Subscription.channel.type must be populated"));
@@ -104,7 +108,7 @@ public class SubscriptionValidatingInterceptorTest {
 			badSub.setCriteria("Patient?");
 			Subscription.SubscriptionChannelComponent channel = badSub.getChannel();
 			channel.setType(Subscription.SubscriptionChannelType.MESSAGE);
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(16) + "No endpoint defined for message subscription"));
@@ -121,7 +125,7 @@ public class SubscriptionValidatingInterceptorTest {
 
 		channel.setEndpoint("foo");
 		try {
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(17) + "Only 'channel' protocol is supported for Subscriptions with channel type 'message'"));
@@ -129,7 +133,7 @@ public class SubscriptionValidatingInterceptorTest {
 
 		channel.setEndpoint("channel");
 		try {
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(17) + "Only 'channel' protocol is supported for Subscriptions with channel type 'message'"));
@@ -137,7 +141,7 @@ public class SubscriptionValidatingInterceptorTest {
 
 		channel.setEndpoint("channel:");
 		try {
-			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+			mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), is(Msg.code(19) + "Invalid subscription endpoint uri channel:"));
@@ -145,7 +149,36 @@ public class SubscriptionValidatingInterceptorTest {
 
 		// Happy path
 		channel.setEndpoint("channel:my-queue-name");
-		mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub);
+		mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null);
+	}
+
+	@Test
+	public void testSubscriptionUpdate() {
+		final Subscription subscription = createSubscription();
+
+		// Assert there is no Exception thrown here.
+		mySubscriptionValidatingInterceptor.resourceUpdated(subscription, subscription, null, null);
+	}
+
+	@Test
+	public void testInvalidPointcut() {
+		try {
+			mySubscriptionValidatingInterceptor.validateSubmittedSubscription(createSubscription(), null, null, Pointcut.TEST_RB);
+			fail();
+		} catch (UnprocessableEntityException e) {
+			assertThat(e.getMessage(), is(Msg.code(2267) + "Expected Pointcut to be either STORAGE_PRESTORAGE_RESOURCE_CREATED or STORAGE_PRESTORAGE_RESOURCE_UPDATED but was: " + Pointcut.TEST_RB));
+		}
+	}
+
+	@Nonnull
+	private static Subscription createSubscription() {
+		final Subscription subscription = new Subscription();
+		subscription.setStatus(Subscription.SubscriptionStatus.REQUESTED);
+		subscription.setCriteria("Patient?");
+		final Subscription.SubscriptionChannelComponent channel = subscription.getChannel();
+		channel.setType(Subscription.SubscriptionChannelType.RESTHOOK);
+		channel.setEndpoint("channel");
+		return subscription;
 	}
 
 	@Configuration

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/subscription/SubscriptionValidatingInterceptorTest.java

@@ -6,6 +6,7 @@ import ca.uhn.fhir.interceptor.model.RequestPartitionId;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc;
+import ca.uhn.fhir.jpa.subscription.match.matcher.matching.SubscriptionMatchingStrategy;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.jpa.subscription.match.matcher.matching.SubscriptionStrategyEvaluator;
 import ca.uhn.fhir.jpa.subscription.match.registry.SubscriptionCanonicalizer;
@@ -15,6 +16,7 @@ import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
 import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
 import ca.uhn.fhir.util.HapiExtensions;
+import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.BooleanType;
 import org.hl7.fhir.r4.model.Patient;
 import org.hl7.fhir.r4.model.Subscription;
@@ -30,10 +32,16 @@ import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.fail;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.isA;
+import static org.mockito.ArgumentMatchers.nullable;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 @ExtendWith(MockitoExtension.class)
@@ -49,11 +57,13 @@ public class SubscriptionValidatingInterceptorTest {
 	private IRequestPartitionHelperSvc myRequestPartitionHelperSvc;
 	@Mock
 	private DaoConfig myDaoConfig;
+	private SubscriptionCanonicalizer mySubscriptionCanonicalizer;
 
 	@BeforeEach
 	public void before() {
 		mySvc = new SubscriptionValidatingInterceptor();
-		mySvc.setSubscriptionCanonicalizerForUnitTest(new SubscriptionCanonicalizer(myCtx));
+		mySubscriptionCanonicalizer = spy(new SubscriptionCanonicalizer(myCtx));
+		mySvc.setSubscriptionCanonicalizerForUnitTest(mySubscriptionCanonicalizer);
 		mySvc.setDaoRegistryForUnitTest(myDaoRegistry);
 		mySvc.setSubscriptionStrategyEvaluatorForUnitTest(mySubscriptionStrategyEvaluator);
 		mySvc.setFhirContext(myCtx);
@@ -66,7 +76,7 @@ public class SubscriptionValidatingInterceptorTest {
 		Subscription subscription = new Subscription();
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription);
+			mySvc.resourcePreCreate(subscription, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), containsString("Subscription.status must be populated on this server"));
@@ -84,7 +94,7 @@ public class SubscriptionValidatingInterceptorTest {
 		subscription.getChannel().setPayload("application/fhir+json");
 		subscription.getChannel().setEndpoint("http://foo");
 
-		mySvc.validateSubmittedSubscription(subscription);
+		mySvc.resourcePreCreate(subscription, null, null);
 	}
 
 	@Test
@@ -99,7 +109,7 @@ public class SubscriptionValidatingInterceptorTest {
 		subscription.getChannel().setEndpoint("http://foo");
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription);
+			mySvc.resourcePreCreate(subscription, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), containsString("Subscription.criteria contains invalid/unsupported resource type: Patient"));
@@ -118,7 +128,7 @@ public class SubscriptionValidatingInterceptorTest {
 		subscription.getChannel().setEndpoint("http://foo");
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription);
+			mySvc.resourcePreCreate(subscription, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), containsString("Subscription.criteria contains invalid/unsupported resource type: Patient"));
@@ -136,7 +146,7 @@ public class SubscriptionValidatingInterceptorTest {
 		subscription.getChannel().setPayload("application/fhir+json");
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription);
+			mySvc.resourcePreCreate(subscription, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), containsString("Rest-hook subscriptions must have Subscription.channel.endpoint defined"));
@@ -155,7 +165,7 @@ public class SubscriptionValidatingInterceptorTest {
 		subscription.getChannel().setEndpoint("http://foo");
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription);
+			mySvc.resourcePreCreate(subscription, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), containsString("Subscription.channel.type must be populated"));
@@ -172,7 +182,7 @@ public class SubscriptionValidatingInterceptorTest {
 		subscription.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK);
 		subscription.getChannel().setEndpoint("http://foo");
 
-		mySvc.validateSubmittedSubscription(subscription);
+		mySvc.resourcePreCreate(subscription, null, null);
 	}
 
 	@Test
@@ -184,7 +194,7 @@ public class SubscriptionValidatingInterceptorTest {
 		subscription.getChannel().setEndpoint("http://foo");
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription);
+			mySvc.resourcePreCreate(subscription, null, null);
 			fail();
 		} catch (UnprocessableEntityException e) {
 			assertThat(e.getMessage(), containsString("Subscription.criteria must be populated"));
@@ -210,7 +220,7 @@ public class SubscriptionValidatingInterceptorTest {
 
 		// No asserts here because the function should throw an UnprocessableEntityException exception if the subscription
 		// is invalid
-		assertDoesNotThrow(() -> mySvc.validateSubmittedSubscription(subscription, requestDetails, null));
+		assertDoesNotThrow(() -> mySvc.resourcePreCreate(subscription, requestDetails, null));
 		Mockito.verify(myDaoConfig, times(1)).isCrossPartitionSubscriptionEnabled();
 		Mockito.verify(myDaoRegistry, times(1)).isResourceTypeSupported(eq("Patient"));
 		Mockito.verify(myRequestPartitionHelperSvc, times(1)).determineCreatePartitionForRequest(isA(RequestDetails.class), isA(Subscription.class), eq("Subscription"));
@@ -233,7 +243,7 @@ public class SubscriptionValidatingInterceptorTest {
 		requestDetails.setRestOperationType(RestOperationTypeEnum.CREATE);
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription, requestDetails, null);
+			mySvc.resourcePreCreate(subscription, requestDetails, null);
 			fail();
 		} catch (UnprocessableEntityException theUnprocessableEntityException) {
 			assertEquals(Msg.code(2010) + "Cross partition subscription must be created on the default partition", theUnprocessableEntityException.getMessage());
@@ -256,7 +266,7 @@ public class SubscriptionValidatingInterceptorTest {
 		requestDetails.setRestOperationType(RestOperationTypeEnum.CREATE);
 
 		try {
-			mySvc.validateSubmittedSubscription(subscription, requestDetails, null);
+			mySvc.resourcePreCreate(subscription, requestDetails, null);
 			fail();
 		} catch (UnprocessableEntityException theUnprocessableEntityException) {
 			assertEquals(Msg.code(2009) + "Cross partition subscription is not enabled on this server", theUnprocessableEntityException.getMessage());
@@ -280,10 +290,36 @@ public class SubscriptionValidatingInterceptorTest {
 
 		// No asserts here because the function should throw an UnprocessableEntityException exception if the subscription
 		// is invalid
-		mySvc.validateSubmittedSubscription(subscription, requestDetails, null);
+		mySvc.resourcePreCreate(subscription, requestDetails, null);
 		Mockito.verify(myDaoConfig, never()).isCrossPartitionSubscriptionEnabled();
 		Mockito.verify(myDaoRegistry, times(1)).isResourceTypeSupported(eq("Patient"));
 		Mockito.verify(myRequestPartitionHelperSvc, never()).determineCreatePartitionForRequest(isA(RequestDetails.class), isA(Patient.class), eq("Patient"));
 	}
 
+	@Test
+	public void testSubscriptionUpdate() {
+		when(myDaoRegistry.isResourceTypeSupported(eq("Patient"))).thenReturn(true);
+		when(myDaoConfig.isCrossPartitionSubscriptionEnabled()).thenReturn(true);
+		lenient()
+			.when(myRequestPartitionHelperSvc.determineReadPartitionForRequestForRead(isA(RequestDetails.class), isA(String.class), isA(IIdType.class)))
+			.thenReturn(RequestPartitionId.allPartitions());
+
+		final Subscription subscription = new Subscription();
+		subscription.setId("customId1");
+		subscription.setStatus(Subscription.SubscriptionStatus.REQUESTED);
+		subscription.setCriteria("Patient?identifier=foo");
+		subscription.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK);
+		subscription.getChannel().setEndpoint("http://foo");
+
+		subscription.addExtension().setUrl(HapiExtensions.EXTENSION_SUBSCRIPTION_CROSS_PARTITION).setValue(new BooleanType(true));
+
+		final RequestDetails requestDetails = mock(RequestDetails.class);
+		lenient()
+			.when(requestDetails.getRestOperationType()).thenReturn(RestOperationTypeEnum.UPDATE);
+
+		mySvc.resourceUpdated(subscription, subscription, requestDetails, null);
+
+		verify(mySubscriptionStrategyEvaluator).determineStrategy(anyString());
+		verify(mySubscriptionCanonicalizer, times(2)).setMatchingStrategyTag(eq(subscription), nullable(SubscriptionMatchingStrategy.class));
+	}
 }