Merge pull request #3023 from hapifhir/jr-20210922-narrowing-interceptor

Jr 20210922 narrowing interceptor

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3018-search-narrowing-interceptor-forbidden-scope.yaml

@@ -0,0 +1,8 @@
+---
+type: add
+issue: 3018
+jira: SMILE-782
+title: "Previously, when a search query explicitly includes a search parameter that is for the same resource type but a
+different resource instance from the one(s) specified on the authorized list, the search narrowing interceptor would include
+both search parameters in the final query, resulting in an empty bundle being returned to the caller. Now, such a call
+will result in a 403 Forbidden error, making it more clear why no resources were returned."

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

@@ -25,6 +25,7 @@ import ca.uhn.fhir.context.RuntimeResourceDefinition;
 import ca.uhn.fhir.context.RuntimeSearchParam;
 import ca.uhn.fhir.interceptor.api.Hook;
 import ca.uhn.fhir.interceptor.api.Pointcut;
+import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.QualifiedParamList;
 import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
@@ -48,6 +49,7 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.util.*;
 import java.util.function.Consumer;
+import java.util.stream.Collectors;
 
 /**
  * This interceptor can be used to automatically narrow the scope of searches in order to
@@ -149,12 +151,19 @@ public class SearchNarrowingInterceptor {
 					 * requested, and the values that the user is allowed to see
 					 */
 					String[] existingValues = newParameters.get(nextParamName);
+					List<String> nextAllowedValueIds = nextAllowedValues
+						.stream()
+						.map(t -> t.lastIndexOf("/") > -1 ? t.substring(t.lastIndexOf("/") + 1) : t)
+						.collect(Collectors.toList());
 					boolean restrictedExistingList = false;
 					for (int i = 0; i < existingValues.length; i++) {
 
 						String nextExistingValue = existingValues[i];
 						List<String> nextRequestedValues = QualifiedParamList.splitQueryStringByCommasIgnoreEscape(null, nextExistingValue);
-						List<String> nextPermittedValues = ListUtils.intersection(nextRequestedValues, nextAllowedValues);
+						List<String> nextPermittedValues = ListUtils.union(
+							ListUtils.intersection(nextRequestedValues, nextAllowedValues),
+							ListUtils.intersection(nextRequestedValues, nextAllowedValueIds)
+						);
 						if (nextPermittedValues.size() > 0) {
 							restrictedExistingList = true;
 							existingValues[i] = ParameterUtil.escapeAndJoinOrList(nextPermittedValues);
@@ -164,15 +173,13 @@ public class SearchNarrowingInterceptor {
 
 					/*
 					 * If none of the values that were requested by the client overlap at all
-					 * with the values that the user is allowed to see, we'll just add the permitted
+					 * with the values that the user is allowed to see, the client shouldn't
-					 * list as a new list. Ultimately this scenario actually means that the client
+					 * get *any* results back. We return an error code indicating that the
-					 * shouldn't get *any* results back, and adding a new AND parameter (that doesn't
+					 * caller is forbidden from accessing the resources they requested.
-					 * overlap at all with the others) is one way of ensuring that.
 					 */
 					if (!restrictedExistingList) {
-						String[] newValues = Arrays.copyOf(existingValues, existingValues.length + 1);
+						theResponse.setStatus(Constants.STATUS_HTTP_403_FORBIDDEN);
-						newValues[existingValues.length] = ParameterUtil.escapeAndJoinOrList(nextAllowedValues);
+						return false;
-						newParameters.put(nextParamName, newValues);
 					}
 				}
 
@@ -246,21 +253,7 @@ public class SearchNarrowingInterceptor {
 
 				} else if (theAreCompartments) {
 
-					List<RuntimeSearchParam> searchParams = theResDef.getSearchParamsForCompartmentName(compartmentName);
+					searchParamName = selectBestSearchParameterForCompartment(theRequestDetails, theResDef, compartmentName);
-					if (searchParams.size() > 0) {
-
-						// Resources like Observation have several fields that add the resource to
-						// the compartment. In the case of Observation, it's subject, patient and performer.
-						// For this kind of thing, we'll prefer the one called "patient".
-						RuntimeSearchParam searchParam =
-							searchParams
-								.stream()
-								.filter(t -> t.getName().equalsIgnoreCase(compartmentName))
-								.findFirst()
-								.orElse(searchParams.get(0));
-						searchParamName = searchParam.getName();
-
-					}
 				}
 
 				lastCompartmentName = compartmentName;
@@ -275,4 +268,69 @@ public class SearchNarrowingInterceptor {
 		}
 	}
 
+	private String selectBestSearchParameterForCompartment(RequestDetails theRequestDetails, RuntimeResourceDefinition theResDef, String compartmentName) {
+		String searchParamName = null;
+
+		Set<String> queryParameters = theRequestDetails.getParameters().keySet();
+
+		List<RuntimeSearchParam> searchParams = theResDef.getSearchParamsForCompartmentName(compartmentName);
+		if (searchParams.size() > 0) {
+
+			// Resources like Observation have several fields that add the resource to
+			// the compartment. In the case of Observation, it's subject, patient and performer.
+			// For this kind of thing, we'll prefer the one that matches the compartment name.
+			Optional<RuntimeSearchParam> primarySearchParam =
+				searchParams
+					.stream()
+					.filter(t -> t.getName().equalsIgnoreCase(compartmentName))
+					.findFirst();
+
+			if (primarySearchParam.isPresent()) {
+				String primarySearchParamName = primarySearchParam.get().getName();
+				// If the primary search parameter is actually in use in the query, use it.
+				if (queryParameters.contains(primarySearchParamName)) {
+					searchParamName = primarySearchParamName;
+				} else {
+					// If the primary search parameter itself isn't in use, check to see whether any of its synonyms are.
+					Optional<RuntimeSearchParam> synonymInUse = findSynonyms(searchParams, primarySearchParam.get())
+						.stream()
+						.filter(t -> queryParameters.contains(t.getName()))
+						.findFirst();
+					if (synonymInUse.isPresent()) {
+						// if a synonym is in use, use it
+						searchParamName = synonymInUse.get().getName();
+					} else {
+						// if not, i.e., the original query is not filtering on this field at all, use the primary search param
+						searchParamName = primarySearchParamName;
+					}
+				}
+			} else {
+				// Otherwise, fall back to whatever search parameter is available
+				searchParamName = searchParams.get(0).getName();
+			}
+
+		}
+		return searchParamName;
+	}
+
+	private List<RuntimeSearchParam> findSynonyms(List<RuntimeSearchParam> searchParams, RuntimeSearchParam primarySearchParam) {
+		// We define two search parameters in a compartment as synonyms if they refer to the same field in the model, ignoring any qualifiers
+
+		String primaryBasePath = getBasePath(primarySearchParam);
+
+		return searchParams
+			.stream()
+			.filter(t -> primaryBasePath.equals(getBasePath(t)))
+			.collect(Collectors.toList());
+	}
+
+	private String getBasePath(RuntimeSearchParam searchParam) {
+		int qualifierIndex = searchParam.getPath().indexOf(".where");
+		if (qualifierIndex == -1) {
+			return searchParam.getPath();
+		} else {
+			return searchParam.getPath().substring(0, qualifierIndex);
+		}
+	}
+
 }

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptorTest.java

@@ -7,6 +7,7 @@ import ca.uhn.fhir.rest.annotation.OptionalParam;
 import ca.uhn.fhir.rest.annotation.Search;
 import ca.uhn.fhir.rest.annotation.Transaction;
 import ca.uhn.fhir.rest.annotation.TransactionParam;
+import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.client.api.IGenericClient;
 import ca.uhn.fhir.rest.client.api.ServerValidationModeEnum;
@@ -17,6 +18,7 @@ import ca.uhn.fhir.rest.param.TokenAndListParam;
 import ca.uhn.fhir.rest.server.FifoMemoryPagingProvider;
 import ca.uhn.fhir.rest.server.IResourceProvider;
 import ca.uhn.fhir.rest.server.RestfulServer;
+import ca.uhn.fhir.rest.server.exceptions.ForbiddenOperationException;
 import ca.uhn.fhir.test.utilities.JettyUtil;
 import ca.uhn.fhir.util.TestUtil;
 import org.eclipse.jetty.server.Server;
@@ -44,6 +46,7 @@ import java.util.stream.Collectors;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.fail;
 
 public class SearchNarrowingInterceptorTest {
 	private static final Logger ourLog = LoggerFactory.getLogger(SearchNarrowingInterceptorTest.class);
@@ -222,15 +225,15 @@ public class SearchNarrowingInterceptorTest {
 	}
 
 	@Test
-	public void testNarrowObservationsByPatientContext_ClientRequestedNoOverlap() {
+	public void testNarrowObservationsByPatientContext_ClientRequestedSomeOverlap_ShortIds() {
 
 		ourNextCompartmentList = new AuthorizedList().addCompartments("Patient/123", "Patient/456");
 
 		ourClient
 			.search()
 			.forResource("Observation")
-			.where(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/777"))
+			.where(Observation.PATIENT.hasAnyOfIds("456", "777"))
-			.and(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/888"))
+			.and(Observation.PATIENT.hasAnyOfIds("456", "888"))
 			.execute();
 
 		assertEquals("Observation.search", ourLastHitMethod);
@@ -238,7 +241,109 @@ public class SearchNarrowingInterceptorTest {
 		assertNull(ourLastCodeParam);
 		assertNull(ourLastSubjectParam);
 		assertNull(ourLastPerformerParam);
-		assertThat(toStrings(ourLastPatientParam), Matchers.contains("Patient/111,Patient/777", "Patient/111,Patient/888", "Patient/123,Patient/456"));
+		assertThat(toStrings(ourLastPatientParam), Matchers.contains("456", "456"));
+	}
+
+	@Test
+	public void testNarrowObservationsByPatientContext_ClientRequestedSomeOverlap_UseSynonym() {
+
+		ourNextCompartmentList = new AuthorizedList().addCompartments("Patient/123", "Patient/456");
+
+		ourClient
+			.search()
+			.forResource("Observation")
+			.where(Observation.SUBJECT.hasAnyOfIds("Patient/456", "Patient/777"))
+			.and(Observation.SUBJECT.hasAnyOfIds("Patient/456", "Patient/888"))
+			.execute();
+
+		assertEquals("Observation.search", ourLastHitMethod);
+		assertNull(ourLastIdParam);
+		assertNull(ourLastCodeParam);
+		assertThat(toStrings(ourLastSubjectParam), Matchers.contains("Patient/456", "Patient/456"));
+		assertNull(ourLastPerformerParam);
+		assertNull(ourLastPatientParam);
+	}
+
+	@Test
+	public void testNarrowObservationsByPatientContext_ClientRequestedNoOverlap() {
+
+		ourNextCompartmentList = new AuthorizedList().addCompartments("Patient/123", "Patient/456");
+
+		try {
+			ourClient
+				.search()
+				.forResource("Observation")
+				.where(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/777"))
+				.and(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/888"))
+				.execute();
+
+			fail("Expected a 403 error");
+		} catch (ForbiddenOperationException e) {
+			assertEquals(Constants.STATUS_HTTP_403_FORBIDDEN, e.getStatusCode());
+		}
+
+		assertNull(ourLastHitMethod);
+	}
+
+	@Test
+	public void testNarrowObservationsByPatientContext_ClientRequestedNoOverlap_UseSynonym() {
+
+		ourNextCompartmentList = new AuthorizedList().addCompartments("Patient/123", "Patient/456");
+
+		try {
+			ourClient
+				.search()
+				.forResource("Observation")
+				.where(Observation.SUBJECT.hasAnyOfIds("Patient/111", "Patient/777"))
+				.and(Observation.SUBJECT.hasAnyOfIds("Patient/111", "Patient/888"))
+				.execute();
+
+			fail("Expected a 403 error");
+		} catch (ForbiddenOperationException e) {
+			assertEquals(Constants.STATUS_HTTP_403_FORBIDDEN, e.getStatusCode());
+		}
+
+		assertNull(ourLastHitMethod);
+	}
+
+	@Test
+	public void testNarrowObservationsByPatientContext_ClientRequestedBadParameter() {
+
+		ourNextCompartmentList = new AuthorizedList().addCompartments("Patient/123", "Patient/456");
+
+		try {
+			ourClient
+				.search()
+				.forResource("Observation")
+				.where(Observation.PATIENT.hasAnyOfIds("Patient/"))
+				.execute();
+
+			fail("Expected a 403 error");
+		} catch (ForbiddenOperationException e) {
+			assertEquals(Constants.STATUS_HTTP_403_FORBIDDEN, e.getStatusCode());
+		}
+
+		assertNull(ourLastHitMethod);
+	}
+
+	@Test
+	public void testNarrowObservationsByPatientContext_ClientRequestedBadPermission() {
+
+		ourNextCompartmentList = new AuthorizedList().addCompartments("Patient/");
+
+		try {
+			ourClient
+				.search()
+				.forResource("Observation")
+				.where(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/777"))
+				.execute();
+
+			fail("Expected a 403 error");
+		} catch (ForbiddenOperationException e) {
+			assertEquals(Constants.STATUS_HTTP_403_FORBIDDEN, e.getStatusCode());
+		}
+
+		assertNull(ourLastHitMethod);
 	}
 
 	private List<String> toStrings(BaseAndListParam<? extends IQueryParameterOr<?>> theParams) {