From e52d11b5ba025dcf52d8c1c1644b7d61d0d12aad Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Wed, 22 Sep 2021 14:00:12 -0400 Subject: [PATCH] search narrowing interceptor responds with 403 Forbidden instead of creating an impossible query --- ...narrowing-interceptor-forbidden-scope.yaml | 8 ++++++ .../auth/SearchNarrowingInterceptor.java | 13 ++++----- .../auth/SearchNarrowingInterceptorTest.java | 28 +++++++++++-------- 3 files changed, 30 insertions(+), 19 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3018-search-narrowing-interceptor-forbidden-scope.yaml 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..7dd2f146001 --- /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 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." 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..b5f5df07615 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; @@ -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 - * 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; } } 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 ce601a2ea39..5d7682a17e4 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); @@ -226,19 +229,20 @@ public class SearchNarrowingInterceptorTest { 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")) - .execute(); + try { + ourClient + .search() + .forResource("Observation") + .where(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/777")) + .and(Observation.PATIENT.hasAnyOfIds("Patient/111", "Patient/888")) + .execute(); - assertEquals("Observation.search", ourLastHitMethod); - assertNull(ourLastIdParam); - assertNull(ourLastCodeParam); - assertNull(ourLastSubjectParam); - assertNull(ourLastPerformerParam); - assertThat(toStrings(ourLastPatientParam), Matchers.contains("Patient/111,Patient/777", "Patient/111,Patient/888", "Patient/123,Patient/456")); + fail("Expected a 403 error"); + } catch (ForbiddenOperationException e) { + assertEquals(Constants.STATUS_HTTP_403_FORBIDDEN, e.getStatusCode()); + } + + assertNull(ourLastHitMethod); } private List toStrings(BaseAndListParam> theParams) {