search narrowing interceptor responds with 403 Forbidden instead of creating an impossible query

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 included a search parameter that was for the same resource type but a
+different resource instance from the one specified in the token, 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;
@@ -164,15 +165,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);
 					}
 				}
 

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);
@@ -226,19 +229,20 @@ public class SearchNarrowingInterceptorTest {
 
 		ourNextCompartmentList = new AuthorizedList().addCompartments("Patient/123", "Patient/456");
 
-		ourClient
+		try {
-			.search()
+			ourClient
-			.forResource("Observation")
+				.search()
-			.where(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/777"))
+				.forResource("Observation")
-			.and(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/888"))
+				.where(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/777"))
-			.execute();
+				.and(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/888"))
+				.execute();
 
-		assertEquals("Observation.search", ourLastHitMethod);
+			fail("Expected a 403 error");
-		assertNull(ourLastIdParam);
+		} catch (ForbiddenOperationException e) {
-		assertNull(ourLastCodeParam);
+			assertEquals(Constants.STATUS_HTTP_403_FORBIDDEN, e.getStatusCode());
-		assertNull(ourLastSubjectParam);
+		}
-		assertNull(ourLastPerformerParam);
+
-		assertThat(toStrings(ourLastPatientParam), Matchers.contains("Patient/111,Patient/777", "Patient/111,Patient/888", "Patient/123,Patient/456"));
+		assertNull(ourLastHitMethod);
 	}
 
 	private List<String> toStrings(BaseAndListParam<? extends IQueryParameterOr<?>> theParams) {