diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3018-search-narrowing-interceptor-forbidden-scope.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3018-search-narrowing-interceptor-forbidden-scope.yaml new file mode 100644 index 00000000000..376fdeac523 --- /dev/null +++ b/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." diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptor.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptor.java index 99dcf610533..30f0b79f0fb 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptor.java +++ b/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 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 nextRequestedValues = QualifiedParamList.splitQueryStringByCommasIgnoreEscape(null, nextExistingValue); - List nextPermittedValues = ListUtils.intersection(nextRequestedValues, nextAllowedValues); + List 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 - * list as a new list. Ultimately this scenario actually means that the client - * shouldn't get *any* results back, and adding a new AND parameter (that doesn't - * overlap at all with the others) is one way of ensuring that. + * with the values that the user is allowed to see, the client shouldn't + * get *any* results back. We return an error code indicating that the + * caller is forbidden from accessing the resources they requested. */ if (!restrictedExistingList) { - String[] newValues = Arrays.copyOf(existingValues, existingValues.length + 1); - newValues[existingValues.length] = ParameterUtil.escapeAndJoinOrList(nextAllowedValues); - newParameters.put(nextParamName, newValues); + theResponse.setStatus(Constants.STATUS_HTTP_403_FORBIDDEN); + return false; } } @@ -246,21 +253,7 @@ public class SearchNarrowingInterceptor { } else if (theAreCompartments) { - List 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 called "patient". - RuntimeSearchParam searchParam = - searchParams - .stream() - .filter(t -> t.getName().equalsIgnoreCase(compartmentName)) - .findFirst() - .orElse(searchParams.get(0)); - searchParamName = searchParam.getName(); - - } + searchParamName = selectBestSearchParameterForCompartment(theRequestDetails, theResDef, compartmentName); } lastCompartmentName = compartmentName; @@ -275,4 +268,69 @@ public class SearchNarrowingInterceptor { } } + private String selectBestSearchParameterForCompartment(RequestDetails theRequestDetails, RuntimeResourceDefinition theResDef, String compartmentName) { + String searchParamName = null; + + Set queryParameters = theRequestDetails.getParameters().keySet(); + + List 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 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 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 findSynonyms(List 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); + } + } + } diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptorTest.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptorTest.java index f58374a1a82..e3b76672911 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptorTest.java +++ b/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")) - .and(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/888")) + .where(Observation.PATIENT.hasAnyOfIds("456", "777")) + .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 toStrings(BaseAndListParam> theParams) {