Fix #2012 - Always filter total from search results when consent interceptor in use (#2020)

* consent bug test

* Fix #2012 - Always filter total from search results when consent
interceptor in use

* Add changelog

* Address coverage issues

Co-authored-by: Jens Kristian Villadsen <46567685+jvitrifork@users.noreply.github.com>
Co-authored-by: jvi <jvi@trifork.com>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_1_0/2012-always-filter-bundle-total-for-consent.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 2012
+title: "In some cases, the Bundle total was not getting filtered from search results when using the consent interceptor. Thanks
+  to Jens Kristian Villadsen for reporting!"

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java

@@ -813,7 +813,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
 							if (nonSkippedCount == 0 || (myMaxResultsToFetch != null && totalFetched < myMaxResultsToFetch)) {
 								ourLog.trace("Setting search status to FINISHED");
 								mySearch.setStatus(SearchStatusEnum.FINISHED);
-								mySearch.setTotalCount(myCountSavedTotal);
+								mySearch.setTotalCount(myCountSavedTotal - countBlocked);
 							} else if (myAdditionalPrefetchThresholdsRemaining) {
 								ourLog.trace("Setting search status to PASSCMPLET");
 								mySearch.setStatus(SearchStatusEnum.PASSCMPLET);
@@ -821,7 +821,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
 							} else {
 								ourLog.trace("Setting search status to FINISHED");
 								mySearch.setStatus(SearchStatusEnum.FINISHED);
-								mySearch.setTotalCount(myCountSavedTotal);
+								mySearch.setTotalCount(myCountSavedTotal - countBlocked);
 							}
 						}
 					}

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

@@ -3,11 +3,15 @@ package ca.uhn.fhir.jpa.provider.r4;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.jpa.config.BaseConfig;
 import ca.uhn.fhir.jpa.config.TestR4Config;
+import ca.uhn.fhir.jpa.entity.Search;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.PreferReturnEnum;
+import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.client.interceptor.CapturingInterceptor;
+import ca.uhn.fhir.rest.gclient.StringClientParam;
 import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException;
+import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
 import ca.uhn.fhir.rest.server.interceptor.consent.ConsentInterceptor;
 import ca.uhn.fhir.rest.server.interceptor.consent.ConsentOperationStatusEnum;
@@ -32,6 +36,8 @@ import org.apache.http.entity.StringEntity;
 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.Enumerations;
+import org.hl7.fhir.r4.model.HumanName;
 import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.OperationOutcome;
@@ -52,6 +58,7 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import static org.apache.commons.lang3.StringUtils.leftPad;
@@ -533,6 +540,92 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid
 
 	}
 
+	@Test
+	public void testBundleTotalIsStripped() {
+		myConsentInterceptor = new ConsentInterceptor(new ConsentSvcCantSeeFemales());
+		ourRestServer.getInterceptorService().registerInterceptor(myConsentInterceptor);
+
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.MALE).addName(new HumanName().setFamily("1"))).execute();
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.MALE).addName(new HumanName().setFamily("2"))).execute();
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.FEMALE).addName(new HumanName().setFamily("3"))).execute();
+
+		Bundle response = myClient.search().forResource(Patient.class).count(1).returnBundle(Bundle.class).execute();
+		String searchId = response.getId();
+
+		// 2 results returned, but no total since it's stripped
+		assertEquals(1, response.getEntry().size());
+		assertNull(response.getTotalElement().getValue());
+
+		// Load next page
+		response = myClient.loadPage().next(response).execute();
+		assertEquals(1, response.getEntry().size());
+		assertNull(response.getTotalElement().getValue());
+
+		// The paging should have ended now - but the last redacted female result is an empty existing page which should never have been there.
+		assertNull(BundleUtil.getLinkUrlOfType(myFhirCtx, response, "next"));
+
+		runInTransaction(()->{
+			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(searchId).orElseThrow(()->new IllegalStateException());
+			assertEquals(3, search.getNumFound());
+			assertEquals(1, search.getNumBlocked());
+			assertEquals(2, search.getTotalCount());
+		});
+	}
+
+	/**
+	 * Make sure the default methods all work and allow the response to proceed
+	 */
+	@Test
+	public void testDefaultInterceptorAllowsAll() {
+		myConsentInterceptor = new ConsentInterceptor(new IConsentService() {});
+		ourRestServer.getInterceptorService().registerInterceptor(myConsentInterceptor);
+
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.MALE).addName(new HumanName().setFamily("1"))).execute();
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.MALE).addName(new HumanName().setFamily("2"))).execute();
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.FEMALE).addName(new HumanName().setFamily("3"))).execute();
+
+		Bundle response = myClient.search().forResource(Patient.class).count(1).returnBundle(Bundle.class).execute();
+		String searchId = response.getId();
+
+		assertEquals(1, response.getEntry().size());
+		assertNull(response.getTotalElement().getValue());
+
+		// Load next page
+		response = myClient.loadPage().next(response).execute();
+		assertEquals(1, response.getEntry().size());
+		assertNull(response.getTotalElement().getValue());
+
+		// The paging should have ended now - but the last redacted female result is an empty existing page which should never have been there.
+		assertNotNull(BundleUtil.getLinkUrlOfType(myFhirCtx, response, "next"));
+
+		runInTransaction(()->{
+			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(searchId).orElseThrow(()->new IllegalStateException());
+			assertEquals(3, search.getNumFound());
+			assertEquals(0, search.getNumBlocked());
+			assertEquals(3, search.getTotalCount());
+		});
+	}
+
+	/**
+	 * Make sure the default methods all work and allow the response to proceed
+	 */
+	@Test
+	public void testDefaultInterceptorAllowsFailure() {
+		myConsentInterceptor = new ConsentInterceptor(new IConsentService() {});
+		ourRestServer.getInterceptorService().registerInterceptor(myConsentInterceptor);
+
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.MALE).addName(new HumanName().setFamily("1"))).execute();
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.MALE).addName(new HumanName().setFamily("2"))).execute();
+		myClient.create().resource(new Patient().setGender(Enumerations.AdministrativeGender.FEMALE).addName(new HumanName().setFamily("3"))).execute();
+
+		try {
+			myClient.search().forResource(Patient.class).where(new StringClientParam("INVALID_PARAM").matchesExactly().value("value")).returnBundle(Bundle.class).execute();
+			fail();
+		} catch (InvalidRequestException e) {
+			assertThat(e.getMessage(), containsString("INVALID_PARAM"));
+		}
+	}
+
 	@Test
 	public void testGraphQL_MaskLinkedResource() throws IOException {
 		createPatientAndOrg();
@@ -699,6 +792,23 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid
 
 	}
 
+	private static class ConsentSvcCantSeeFemales implements IConsentService {
+
+		@Override
+		public ConsentOutcome canSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices) {
+			if (theRequestDetails.getRestOperationType() != RestOperationTypeEnum.CREATE) {
+				Patient patient = (Patient) theResource;
+				if (patient.getGender() == Enumerations.AdministrativeGender.FEMALE) {
+					return ConsentOutcome.REJECT;
+				}
+				return ConsentOutcome.PROCEED;
+
+			}
+			return ConsentOutcome.AUTHORIZED;
+		}
+
+	}
+
 	private static class ConsentSvcCantSeeEvenNumbered implements IConsentService {
 
 		@Override

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/consent/ConsentInterceptor.java

@@ -208,6 +208,11 @@ public class ConsentInterceptor {
 				theResource.setResponseResource(outcome.getResource());
 			}
 
+			// Clear the total
+			if (theResource.getResponseResource() instanceof IBaseBundle) {
+				BundleUtil.setTotal(theRequestDetails.getFhirContext(), (IBaseBundle) theResource.getResponseResource(), null);
+			}
+
 			switch (outcome.getStatus()) {
 				case REJECT:
 					if (outcome.getOperationOutcome() != null) {

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/consent/IConsentService.java

@@ -24,6 +24,9 @@ import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 
+/**
+ * Note: Since HAPI FHIR 5.1.0, methods in this interface have default methods that return {@link ConsentOutcome#PROCEED}
+ */
 public interface IConsentService {
 
 	/**
@@ -42,7 +45,9 @@ public interface IConsentService {
 	 *                           consent directives.
 	 * @return An outcome object. See {@link ConsentOutcome}
 	 */
-	ConsentOutcome startOperation(RequestDetails theRequestDetails, IConsentContextServices theContextServices);
+	default ConsentOutcome startOperation(RequestDetails theRequestDetails, IConsentContextServices theContextServices) {
+		return ConsentOutcome.PROCEED;
+	}
 
 	/**
 	 * This method is called if a user may potentially see a resource via READ
@@ -73,7 +78,9 @@ public interface IConsentService {
 	 *                           consent directives.
 	 * @return An outcome object. See {@link ConsentOutcome}
 	 */
-	ConsentOutcome canSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices);
+	default ConsentOutcome canSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices) {
+		return ConsentOutcome.PROCEED;
+	}
 
 	/**
 	 * This method is called if a user is about to see a resource, either completely
@@ -108,7 +115,9 @@ public interface IConsentService {
 	 *                           consent directives.
 	 * @return An outcome object. See method documentation for a description.
 	 */
-	ConsentOutcome willSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices);
+	default ConsentOutcome willSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices) {
+		return ConsentOutcome.PROCEED;
+	}
 
 	/**
 	 * This method is called when an operation is complete. It can be used to perform
@@ -129,7 +138,8 @@ public interface IConsentService {
 	 *                           consent directives.
 	 * @see #completeOperationFailure(RequestDetails, BaseServerResponseException, IConsentContextServices)
 	 */
-	void completeOperationSuccess(RequestDetails theRequestDetails, IConsentContextServices theContextServices);
+	default void completeOperationSuccess(RequestDetails theRequestDetails, IConsentContextServices theContextServices) {
+	}
 
 	/**
 	 * This method is called when an operation is complete. It can be used to perform
@@ -151,5 +161,6 @@ public interface IConsentService {
 	 *                           consent directives.
 	 * @see #completeOperationSuccess(RequestDetails, IConsentContextServices)
 	 */
-	void completeOperationFailure(RequestDetails theRequestDetails, BaseServerResponseException theException, IConsentContextServices theContextServices);
+	default void completeOperationFailure(RequestDetails theRequestDetails, BaseServerResponseException theException, IConsentContextServices theContextServices) {
+	}
 }