honeymoose
/
hapi-fhir
mirror of https://github.com/hapifhir/hapi-fhir.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'remove-parallel-non-gets' of github.com:hapifhir/hapi-fhir into remove-parallel-non-gets

This commit is contained in:
Tadgh 2021-09-28 16:43:36 -04:00
parent 040d09cfa6 6f36a0aab3
commit 63ef58c0b9
3 changed files with 198 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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."

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

@ -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
* 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<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 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<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);
}
}
}

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

@ -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<String> toStrings(BaseAndListParam<? extends IQueryParameterOr<?>> theParams) {