Update RuleBuilder to effectively handle Patient Type-Level Exports (#5556)

* Update RuleBuilder to effectively handle Patient Type-Level Exports

* Remove excess from changelog and add additional tests

* Add additional test cases

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/4634-rulebuilder-bulkexport-patient.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 4634
+title: "Previously, rule builder could not effectively handle Patient Type-Level Exports. It would over-permit requests
+    in certain scenarios. This fix allows for accumulation of ids on a Patient Type-Level Bulk export to enable us to 
+    properly match the requested Patient IDs against the users permitted Patient IDs."

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

@@ -252,6 +252,7 @@ public class RuleBuilder implements IAuthRuleBuilder {
 		private final String myRuleName;
 		private RuleBuilderRuleOp myReadRuleBuilder;
 		private RuleBuilderRuleOp myWriteRuleBuilder;
+		private RuleBuilderBulkExport ruleBuilderBulkExport;
 
 		RuleBuilderRule(PolicyEnum theRuleMode, String theRuleName) {
 			myRuleMode = theRuleMode;
@@ -333,7 +334,10 @@ public class RuleBuilder implements IAuthRuleBuilder {
 
 		@Override
 		public IAuthRuleBuilderRuleBulkExport bulkExport() {
-			return new RuleBuilderBulkExport();
+			if (ruleBuilderBulkExport == null) {
+				ruleBuilderBulkExport = new RuleBuilderBulkExport();
+			}
+			return ruleBuilderBulkExport;
 		}
 
 		@Override
@@ -859,6 +863,7 @@ public class RuleBuilder implements IAuthRuleBuilder {
 		}
 
 		private class RuleBuilderBulkExport implements IAuthRuleBuilderRuleBulkExport {
+			private RuleBulkExportImpl ruleBulkExport;
 
 			@Override
 			public IAuthRuleBuilderRuleBulkExportWithTarget groupExportOnGroup(@Nonnull String theFocusResourceId) {
@@ -872,12 +877,21 @@ public class RuleBuilder implements IAuthRuleBuilder {
 
 			@Override
 			public IAuthRuleBuilderRuleBulkExportWithTarget patientExportOnPatient(@Nonnull String theFocusResourceId) {
-				RuleBulkExportImpl rule = new RuleBulkExportImpl(myRuleName);
+				if (ruleBulkExport == null) {
-				rule.setAppliesToPatientExport(theFocusResourceId);
+					RuleBulkExportImpl rule = new RuleBulkExportImpl(myRuleName);
-				rule.setMode(myRuleMode);
+					rule.setAppliesToPatientExport(theFocusResourceId);
-				myRules.add(rule);
+					rule.setMode(myRuleMode);
+					ruleBulkExport = rule;
+				} else {
+					ruleBulkExport.setAppliesToPatientExport(theFocusResourceId);
+				}
 
-				return new RuleBuilderBulkExportWithTarget(rule);
+				// prevent duplicate rules being added
+				if (!myRules.contains(ruleBulkExport)) {
+					myRules.add(ruleBulkExport);
+				}
+
+				return new RuleBuilderBulkExportWithTarget(ruleBulkExport);
 			}
 
 			@Override

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

@@ -27,6 +27,7 @@ import ca.uhn.fhir.rest.api.server.bulk.BulkExportJobParameters;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Objects;
@@ -40,13 +41,14 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank;
 public class RuleBulkExportImpl extends BaseRule {
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(RuleBulkExportImpl.class);
 	private String myGroupId;
-	private String myPatientId;
+	private final Collection<String> myPatientIds;
 	private BulkExportJobParameters.ExportStyle myWantExportStyle;
 	private Collection<String> myResourceTypes;
 	private boolean myWantAnyStyle;
 
 	RuleBulkExportImpl(String theRuleName) {
 		super(theRuleName);
+		myPatientIds = new ArrayList<>();
 	}
 
 	@Override
@@ -111,19 +113,25 @@ public class RuleBulkExportImpl extends BaseRule {
 			}
 		}
 
-		// TODO This is a _bad bad bad implementation_ but we are out of time.
+		// 1. If each of the requested resource IDs in the parameters are present in the users permissions, Approve
-		// 1. If a claimed resource ID is present in the parameters, and the permission contains one, check for
+		// 2. If any requested ID is not present in the users permissions, Deny.
-		// membership
+		if (myWantExportStyle == BulkExportJobParameters.ExportStyle.PATIENT && isNotEmpty(myPatientIds)) {
-		// 2. If not a member, Deny.
+			List<String> permittedPatientIds = myPatientIds.stream()
-		if (myWantExportStyle == BulkExportJobParameters.ExportStyle.PATIENT && isNotBlank(myPatientId)) {
+					.map(id -> new IdDt(id).toUnqualifiedVersionless().getValue())
-			final String expectedPatientId =
+					.collect(Collectors.toList());
-					new IdDt(myPatientId).toUnqualifiedVersionless().getValue();
 			if (!options.getPatientIds().isEmpty()) {
 				ourLog.debug("options.getPatientIds() != null");
-				final String actualPatientIds = options.getPatientIds().stream()
+				List<String> requestedPatientIds = options.getPatientIds().stream()
 						.map(t -> new IdDt(t).toUnqualifiedVersionless().getValue())
-						.collect(Collectors.joining(","));
+						.collect(Collectors.toList());
-				if (actualPatientIds.contains(expectedPatientId)) {
+				boolean requestedPatientsPermitted = true;
+				for (String requestedPatientId : requestedPatientIds) {
+					if (!permittedPatientIds.contains(requestedPatientId)) {
+						requestedPatientsPermitted = false;
+						break;
+					}
+				}
+				if (requestedPatientsPermitted) {
 					return newVerdict(
 							theOperation,
 							theRequestDetails,
@@ -138,8 +146,6 @@ public class RuleBulkExportImpl extends BaseRule {
 
 			final List<String> filters = options.getFilters();
 
-			// TODO:  LD:  This admittedly adds more to the tech debt above, and should really be addressed by
-			// https://github.com/hapifhir/hapi-fhir/issues/4990
 			if (!filters.isEmpty()) {
 				ourLog.debug("filters not empty");
 				final Set<String> patientIdsInFilters = filters.stream()
@@ -147,7 +153,15 @@ public class RuleBulkExportImpl extends BaseRule {
 						.map(filter -> filter.replace("?_id=", "/"))
 						.collect(Collectors.toUnmodifiableSet());
 
-				if (patientIdsInFilters.contains(expectedPatientId)) {
+				boolean filteredPatientIdsPermitted = true;
+				for (String patientIdInFilters : patientIdsInFilters) {
+					if (!permittedPatientIds.contains(patientIdInFilters)) {
+						filteredPatientIdsPermitted = false;
+						break;
+					}
+				}
+
+				if (filteredPatientIdsPermitted) {
 					return newVerdict(
 							theOperation,
 							theRequestDetails,
@@ -176,7 +190,7 @@ public class RuleBulkExportImpl extends BaseRule {
 
 	public void setAppliesToPatientExport(String thePatientId) {
 		myWantExportStyle = BulkExportJobParameters.ExportStyle.PATIENT;
-		myPatientId = thePatientId;
+		myPatientIds.add(thePatientId);
 	}
 
 	public void setAppliesToSystem() {

hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBuilderTest.java

@@ -10,6 +10,7 @@ import java.util.List;
 import static org.hamcrest.Matchers.contains;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
 
 public class RuleBuilderTest {
@@ -87,6 +88,19 @@ public class RuleBuilderTest {
 
 	}
 
+	@Test
+	public void testBulkExport_PatientExportOnPatient_MultiplePatientsSingleRule() {
+		RuleBuilder builder = new RuleBuilder();
+		List<String> resourceTypes = new ArrayList<>();
+		resourceTypes.add("Patient");
+
+		builder.allow().bulkExport().patientExportOnPatient("Patient/pat1").withResourceTypes(resourceTypes);
+		builder.allow().bulkExport().patientExportOnPatient("Patient/pat2").withResourceTypes(resourceTypes);
+		List<IAuthRule> rules = builder.build();
+		assertEquals(rules.size(),1);
+		assertTrue(rules.get(0) instanceof RuleBulkExportImpl);
+	}
+
 	@Test
 	public void testNullConditional() {
 		IAuthRuleBuilder ruleBuilder = new RuleBuilder().allow().metadata().andThen();

hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBulkExportImplTest.java

@@ -249,4 +249,99 @@ public class RuleBulkExportImplTest {
 		//Then: The patient IDs do NOT match so this is not permitted.
 		assertEquals(PolicyEnum.DENY, verdict.getDecision());
 	}
+
+	@Test
+	public void testPatientExportRulesOnTypeLevelExportUnpermittedPatient() {
+		//Given
+		final RuleBulkExportImpl myRule = new RuleBulkExportImpl("b");
+		myRule.setAppliesToPatientExport("Patient/123");
+		myRule.setMode(PolicyEnum.ALLOW);
+		final BulkExportJobParameters options = new BulkExportJobParameters();
+		options.setExportStyle(BulkExportJobParameters.ExportStyle.PATIENT);
+		options.setPatientIds(Set.of("Patient/456"));
+		options.setResourceTypes(Set.of("Patient"));
+		when(myRequestDetails.getAttribute(any())).thenReturn(options);
+
+		//When
+		final AuthorizationInterceptor.Verdict verdict = myRule.applyRule(myOperation, myRequestDetails, null, null, null, myRuleApplier, myFlags, myPointcut);
+
+		//Then: We do not have permissions on the requested patient so this is not permitted.
+		assertEquals(PolicyEnum.DENY, verdict.getDecision());
+	}
+
+	@Test
+	public void testPatientExportRulesOnTypeLevelExportPermittedPatient() {
+		//Given
+		final RuleBulkExportImpl myRule = new RuleBulkExportImpl("b");
+		myRule.setAppliesToPatientExport("Patient/123");
+		myRule.setMode(PolicyEnum.ALLOW);
+		final BulkExportJobParameters options = new BulkExportJobParameters();
+		options.setExportStyle(BulkExportJobParameters.ExportStyle.PATIENT);
+		options.setPatientIds(Set.of("Patient/123"));
+		options.setResourceTypes(Set.of("Patient"));
+		when(myRequestDetails.getAttribute(any())).thenReturn(options);
+
+		//When
+		final AuthorizationInterceptor.Verdict verdict = myRule.applyRule(myOperation, myRequestDetails, null, null, null, myRuleApplier, myFlags, myPointcut);
+
+		//Then: We have permissions on the requested patient so this is permitted.
+		assertEquals(PolicyEnum.ALLOW, verdict.getDecision());
+	}
+
+	@Test
+	public void testPatientExportRulesOnTypeLevelExportPermittedPatients() {
+		//Given
+		final RuleBulkExportImpl myRule = new RuleBulkExportImpl("b");
+		myRule.setAppliesToPatientExport("Patient/123");
+		myRule.setAppliesToPatientExport("Patient/456");
+		myRule.setMode(PolicyEnum.ALLOW);
+		final BulkExportJobParameters options = new BulkExportJobParameters();
+		options.setExportStyle(BulkExportJobParameters.ExportStyle.PATIENT);
+		options.setPatientIds(Set.of("Patient/123", "Patient/456"));
+		options.setResourceTypes(Set.of("Patient"));
+		when(myRequestDetails.getAttribute(any())).thenReturn(options);
+
+		//When
+		final AuthorizationInterceptor.Verdict verdict = myRule.applyRule(myOperation, myRequestDetails, null, null, null, myRuleApplier, myFlags, myPointcut);
+
+		//Then: We have permissions on both requested patients so this is permitted.
+		assertEquals(PolicyEnum.ALLOW, verdict.getDecision());
+	}
+
+	@Test
+	public void testPatientExportRulesOnTypeLevelExportWithPermittedAndUnpermittedPatients() {
+		//Given
+		final RuleBulkExportImpl myRule = new RuleBulkExportImpl("b");
+		myRule.setAppliesToPatientExport("Patient/123");
+		myRule.setMode(PolicyEnum.ALLOW);
+		final BulkExportJobParameters options = new BulkExportJobParameters();
+		options.setExportStyle(BulkExportJobParameters.ExportStyle.PATIENT);
+		options.setPatientIds(Set.of("Patient/123","Patient/456"));
+		options.setResourceTypes(Set.of("Patient"));
+		when(myRequestDetails.getAttribute(any())).thenReturn(options);
+
+		//When
+		final AuthorizationInterceptor.Verdict verdict = myRule.applyRule(myOperation, myRequestDetails, null, null, null, myRuleApplier, myFlags, myPointcut);
+
+		//Then: There are unpermitted patients in the request so this is not permitted.
+		assertEquals(PolicyEnum.DENY, verdict.getDecision());
+	}
+	@Test
+	public void testPatientExportRulesOnTypeLevelExportWithPermittedAndUnpermittedPatientFilters() {
+		//Given
+		final RuleBulkExportImpl myRule = new RuleBulkExportImpl("b");
+		myRule.setAppliesToPatientExport("Patient/123");
+		myRule.setMode(PolicyEnum.ALLOW);
+		final BulkExportJobParameters options = new BulkExportJobParameters();
+		options.setExportStyle(BulkExportJobParameters.ExportStyle.PATIENT);
+		options.setFilters(Set.of("Patient?_id=123","Patient?_id=456"));
+		options.setResourceTypes(Set.of("Patient"));
+		when(myRequestDetails.getAttribute(any())).thenReturn(options);
+
+		//When
+		final AuthorizationInterceptor.Verdict verdict = myRule.applyRule(myOperation, myRequestDetails, null, null, null, myRuleApplier, myFlags, myPointcut);
+
+		//Then: There are unpermitted patients in the request so this is not permitted.
+		assertEquals(PolicyEnum.DENY, verdict.getDecision());
+	}
 }