collapse multiple owners for same compartment into shared compartment (#2760)

* collapse multiple owners for same compartment into shared compartment

* changelog

* fix regression

* fix regression

* pre-review cleanup

* pre-review cleanup

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2760-collapse-shared-compartments.yaml

@@ -0,0 +1,6 @@
+---
+type: change
+issue: 2760
+title: "If two authorization compartments apply to the same targets and share the same compartment name,
+then instead of creating a new compartment, the rule builder now adds the new owner to the list of owners in the
+existing compartment."

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

@@ -1457,6 +1457,52 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 		}
 	}
 
+	@Test
+	public void testReadCompartmentTwoPatientIds() {
+		Patient patient1 = new Patient();
+		patient1.setActive(true);
+		IIdType p1id = myPatientDao.create(patient1).getId().toUnqualifiedVersionless();
+
+		Patient patient2 = new Patient();
+		patient2.setActive(true);
+		IIdType p2id = myPatientDao.create(patient2).getId().toUnqualifiedVersionless();
+
+		ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
+			@Override
+			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+				return new RuleBuilder()
+					.allow()
+					.read()
+					.resourcesOfType(Patient.class)
+					.inCompartment("Patient", p1id)
+					.andThen()
+					.allow()
+					.read()
+					.resourcesOfType(Patient.class)
+					.inCompartment("Patient", p2id)
+					.andThen()
+					.denyAll()
+					.build();
+			}
+		});
+
+		{
+			String url = "/Patient?_id=" + p1id.getIdPart();
+			Bundle result = myClient.search().byUrl(url).returnBundle(Bundle.class).execute();
+			assertEquals(1, result.getTotal());
+		}
+		{
+			String url = "/Patient?_id=" + p2id.getIdPart();
+			Bundle result = myClient.search().byUrl(url).returnBundle(Bundle.class).execute();
+			assertEquals(1, result.getTotal());
+		}
+		{
+			String url = "/Patient?_id=" + p1id.getIdPart() + "," + p2id.getIdPart();
+			Bundle result = myClient.search().byUrl(url).returnBundle(Bundle.class).execute();
+			assertEquals(2, result.getTotal());
+		}
+	}
+
 	@Test
 	public void testDeleteExpungeBlocked() {
 		// Create Patient, and Observation that refers to it

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

@@ -38,6 +38,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
@@ -46,7 +47,7 @@ import static org.apache.commons.lang3.ObjectUtils.defaultIfNull;
 public class RuleBuilder implements IAuthRuleBuilder {
 
 	private static final ConcurrentHashMap<Class<? extends IBaseResource>, String> ourTypeToName = new ConcurrentHashMap<>();
-	private ArrayList<IAuthRule> myRules;
+	private final ArrayList<IAuthRule> myRules;
 	private IAuthRuleBuilderRule myAllow;
 	private IAuthRuleBuilderRule myDeny;
 
@@ -229,8 +230,8 @@ public class RuleBuilder implements IAuthRuleBuilder {
 
 	private class RuleBuilderRule implements IAuthRuleBuilderRule {
 
-		private PolicyEnum myRuleMode;
-		private String myRuleName;
+		private final PolicyEnum myRuleMode;
+		private final String myRuleName;
 		private RuleBuilderRuleOp myReadRuleBuilder;
 		private RuleBuilderRuleOp myWriteRuleBuilder;
 
@@ -321,7 +322,7 @@ public class RuleBuilder implements IAuthRuleBuilder {
 
 			private AppliesTypeEnum myAppliesTo;
 			private Set<String> myAppliesToTypes;
-			private RestOperationTypeEnum myOperationType;
+			private final RestOperationTypeEnum myOperationType;
 
 			RuleBuilderRuleConditional(RestOperationTypeEnum theOperationType) {
 				myOperationType = theOperationType;
@@ -506,12 +507,26 @@ public class RuleBuilder implements IAuthRuleBuilder {
 					Validate.notBlank(theCompartmentName, "theCompartmentName must not be null");
 					Validate.notNull(theOwner, "theOwner must not be null");
 					validateOwner(theOwner);
+					Optional<RuleImplOp> oRule = findRuleByAppliesToAndCompartmentName(theCompartmentName);
+					if (oRule.isPresent()) {
+						RuleImplOp rule = oRule.get();
+						rule.addClassifierCompartmentOwner(theOwner);
+						return new RuleBuilderFinished(rule);
+					}
 					myInCompartmentName = theCompartmentName;
 					myInCompartmentOwners = Collections.singletonList(theOwner);
 					myClassifierType = ClassifierTypeEnum.IN_COMPARTMENT;
 					return finished();
 				}
 
+				private Optional<RuleImplOp> findRuleByAppliesToAndCompartmentName(String theCompartmentName) {
+					return myRules.stream()
+						.filter(RuleImplOp.class::isInstance)
+						.map(RuleImplOp.class::cast)
+						.filter(rule -> rule.matches(myAppliesTo, myAppliesToInstances, myAppliesToTypes, theCompartmentName))
+						.findFirst();
+				}
+
 				private void validateOwner(IIdType theOwner) {
 					Validate.notBlank(theOwner.getIdPart(), "owner.getIdPart() must not be null or empty");
 					Validate.notBlank(theOwner.getIdPart(), "owner.getResourceType() must not be null or empty");
@@ -546,7 +561,7 @@ public class RuleBuilder implements IAuthRuleBuilder {
 
 			private class RuleBuilderRuleOperationNamed implements IAuthRuleBuilderOperationNamed {
 
-				private String myOperationName;
+				private final String myOperationName;
 
 				RuleBuilderRuleOperationNamed(String theOperationName) {
 					if (theOperationName != null && !theOperationName.startsWith("$")) {

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

@@ -630,4 +630,24 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 	void setAppliesToDeleteExpunge(boolean theAppliesToDeleteExpunge) {
 		myAppliesToDeleteExpunge = theAppliesToDeleteExpunge;
 	}
+
+	public void addClassifierCompartmentOwner(IIdType theOwner) {
+		List<IIdType> newList = new ArrayList<>(myClassifierCompartmentOwners);
+		newList.add(theOwner);
+		myClassifierCompartmentOwners = newList;
+	}
+
+	public boolean matches(AppliesTypeEnum theAppliesTo, Collection<IIdType> theAppliesToInstances, Set<String> theAppliesToTypes, String theCompartmentName) {
+		switch (theAppliesTo) {
+			case TYPES:
+				return theAppliesToTypes.equals(myAppliesToTypes) && theCompartmentName.equals(myClassifierCompartmentName);
+			case INSTANCES:
+				return theAppliesToInstances.equals(myAppliesToInstances) && theCompartmentName.equals(myClassifierCompartmentName);
+			case ALL_RESOURCES:
+				return theCompartmentName.equals(myClassifierCompartmentName);
+			default:
+				// no more cases
+				return false;
+		}
+	}
 }