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

SearchBuilder should not allow $everything operation to follow links to Group or List resources (#5283) 

* SearchBuilder should not allow $everything operation to follow links to Group or List resources - tests

* SearchBuilder should not allow $everything operation to follow links to Group or List resources - implementation

* SearchBuilder should not allow $everything operation to follow links to Group or List resources - spotless check fix and changelog added

* Add a new permission: FHIR_OP_PATIENT_EVERYTHING_ACCESS_ALL for $everything operation - fixes

Co-authored-by: James Agnew <jamesagnew@gmail.com>

* MongoIncludeSvc should not allow $everything operation to follow links to Group or List resources - fixes

* MongoIncludeSvc should not allow $everything operation to follow links to Group or List resources - fixes

---------

Co-authored-by: James Agnew <jamesagnew@gmail.com>
This commit is contained in:
volodymyr-korzh 2023-09-12 11:39:53 -06:00 committed by GitHub
parent 03ebabad5b
commit fe8c6c066e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 143 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5275-allow-to-configure-permission-rules-for-operation-with-access-to-all-resources.yaml Normal file
View File

@ -0,0 +1,6 @@
---
type: add
issue: 5275
title: "Added an API that allows to configure permission rules for operations with access to all resources.
This permission is needed to allow a search across the entire patient's record in the scope of the $everything operation to access all
resources that references input Patient, including resources outside of the patient's compartment."

6
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5275-searchbuilder-should-not-allow-everything-operation-to-follow-links-to-group-or-list-resources.yaml Normal file
View File

@ -0,0 +1,6 @@
---
type: fix
issue: 5275
title: "Previously, when calling `$everything` operation on a Patient instance, it was possible to retrieve data related
to another patient via a List or Group resources. This has been fixed."

4
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java
View File

@ -129,6 +129,7 @@ import javax.persistence.Tuple;
import javax.persistence.TypedQuery;
import javax.persistence.criteria.CriteriaBuilder;
import static ca.uhn.fhir.jpa.model.util.JpaConstants.UNDESIRED_RESOURCE_LINKAGES_FOR_EVERYTHING_ON_PATIENT_INSTANCE;
import static ca.uhn.fhir.jpa.search.builder.QueryStack.LOCATION_POSITION;
import static org.apache.commons.lang3.StringUtils.defaultString;
import static org.apache.commons.lang3.StringUtils.isBlank;
@ -1380,7 +1381,8 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
if (myParams != null
&& myParams.getEverythingMode() == SearchParameterMap.EverythingModeEnum.PATIENT_INSTANCE) {
sqlBuilder.append(" AND r.myTargetResourceType != 'Patient'");
sqlBuilder.append(" AND r.mySourceResourceType != 'Provenance'");
sqlBuilder.append(UNDESIRED_RESOURCE_LINKAGES_FOR_EVERYTHING_ON_PATIENT_INSTANCE.stream()
.collect(Collectors.joining("', '", " AND r.mySourceResourceType NOT IN ('", "')")));
}
if (hasDesiredResourceTypes) {
sqlBuilder.append(" AND r.myTargetResourceType IN (:desired_target_resource_types)");

5
hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/util/JpaConstants.java
View File

@ -23,6 +23,8 @@ import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.server.provider.ProviderConstants;
import ca.uhn.fhir.util.HapiExtensions;
import java.util.Set;
public class JpaConstants {
/**
@ -306,7 +308,8 @@ public class JpaConstants {
public static final String BULK_META_EXTENSION_JOB_ID = "https://hapifhir.org/NamingSystem/bulk-export-job-id";
public static final String BULK_META_EXTENSION_RESOURCE_TYPE =
"https://hapifhir.org/NamingSystem/bulk-export-binary-resource-type";
public static final Set<String> UNDESIRED_RESOURCE_LINKAGES_FOR_EVERYTHING_ON_PATIENT_INSTANCE =
Set.of("Provenance", "List", "Group");
/**
* Non-instantiable
*/

82
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4EverythingTest.java
View File

@ -20,9 +20,11 @@ import org.hl7.fhir.r4.model.Composition;
import org.hl7.fhir.r4.model.Condition;
import org.hl7.fhir.r4.model.Device;
import org.hl7.fhir.r4.model.Encounter;
import org.hl7.fhir.r4.model.Group;
import org.hl7.fhir.r4.model.IdType;
import org.hl7.fhir.r4.model.InstantType;
import org.hl7.fhir.r4.model.IntegerType;
import org.hl7.fhir.r4.model.ListResource;
import org.hl7.fhir.r4.model.Location;
import org.hl7.fhir.r4.model.Medication;
import org.hl7.fhir.r4.model.MedicationRequest;
@ -977,13 +979,7 @@ public class ResourceProviderR4EverythingTest extends BaseResourceProviderR4Test
myFhirContext.getRestfulClientFactory().setSocketTimeout(300 * 1000);
Bundle response = myClient
.operation()
.onInstance(id)
.named("everything")
.withNoParameters(Parameters.class)
.returnResourceType(Bundle.class)
.execute();
Bundle response = executeEverythingOperationOnInstance(id);
assertEquals(1, response.getEntry().size());
}
@ -1126,13 +1122,7 @@ public class ResourceProviderR4EverythingTest extends BaseResourceProviderR4Test
prov.addTarget().setReference(badPid);
String provid = myProvenanceDao.create(prov, mySrd).getId().toUnqualifiedVersionless().getValue();
Bundle response = myClient
.operation()
.onInstance(new IdType(goodPid))
.named("everything")
.withNoParameters(Parameters.class)
.returnResourceType(Bundle.class)
.execute();
Bundle response = executeEverythingOperationOnInstance(new IdType(goodPid));
List<String> ids = toUnqualifiedVersionlessIdValues(response);
// We should not pick up other resources via the provenance
@ -1208,19 +1198,69 @@ public class ResourceProviderR4EverythingTest extends BaseResourceProviderR4Test
notDesiredProvenance.addTarget().setReference(compositionId);
final String notDesiredProvenanceId = myProvenanceDao.create(notDesiredProvenance, mySrd).getId().toUnqualifiedVersionless().getValue();
final Bundle response = myClient
.operation()
.onInstance(new IdType(desiredPid))
.named("everything")
.withNoParameters(Parameters.class)
.returnResourceType(Bundle.class)
.execute();
final Bundle response = executeEverythingOperationOnInstance(new IdType(desiredPid));
final List<String> actualResourceIds = toUnqualifiedVersionlessIdValues(response);
// We should not pick up other resources via the notDesiredProvenance
assertThat(actualResourceIds, containsInAnyOrder(desiredPid, desiredObservationId, desiredProvenanceId));
}
@Test
public void testEverything_withPatientLinkedByList_returnOnlyDesiredResources() {
// setup
IIdType desiredPid = createPatient(withActiveTrue());
IIdType desiredObservationId = createObservationForPatient(desiredPid, "1");
IIdType notDesiredPid = createPatient(withActiveTrue());
IIdType notDesiredObservationId = createObservationForPatient(notDesiredPid, "1");
ListResource list = new ListResource();
Arrays.asList(desiredPid, desiredObservationId, notDesiredPid, notDesiredObservationId)
.forEach(resourceIdType -> list.addEntry().getItem().setReferenceElement(resourceIdType));
IIdType listId = myListDao.create(list).getId().toUnqualifiedVersionless();
// execute
Bundle response = executeEverythingOperationOnInstance(desiredPid);
List<IIdType> actualResourceIds = toUnqualifiedVersionlessIds(response);
// verify - we should not pick up other resources linked by List
assertThat(actualResourceIds, containsInAnyOrder(desiredPid, desiredObservationId, listId));
}
@Test
public void testEverything_withPatientLinkedByGroup_returnOnlyDesiredResources() {
// setup
IIdType desiredPractitionerId = createPractitioner(withActiveTrue());
IIdType desiredPid = createPatient(withActiveTrue(), withReference("generalPractitioner", desiredPractitionerId));
IIdType notDesiredPractitionerId = createPractitioner(withActiveTrue());
IIdType notDesiredPid = createPatient(withActiveTrue(), withReference("generalPractitioner", notDesiredPractitionerId));
Group group = new Group();
Arrays.asList(desiredPid, desiredPractitionerId, notDesiredPid, notDesiredPractitionerId)
.forEach(resourceIdType -> group.addMember().getEntity().setReferenceElement(resourceIdType));
IIdType groupId = myGroupDao.create(group).getId().toUnqualifiedVersionless();
// execute
Bundle response = executeEverythingOperationOnInstance(desiredPid);
List<IIdType> actualResourceIds = toUnqualifiedVersionlessIds(response);
// verify - we should not pick up other resources linked by Group
assertThat(actualResourceIds, containsInAnyOrder(desiredPid, desiredPractitionerId, groupId));
}
private Bundle executeEverythingOperationOnInstance(IIdType theInstanceIdType) {
return myClient
.operation()
.onInstance(theInstanceIdType)
.named("everything")
.withNoParameters(Parameters.class)
.returnResourceType(Bundle.class)
.execute();
}
private IIdType createOrganization(String methodName, String s) {
Organization o1 = new Organization();
o1.setName(methodName + s);

4
hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/BaseJpaR4Test.java
View File

@ -144,6 +144,7 @@ import org.hl7.fhir.r4.model.ExplanationOfBenefit;
import org.hl7.fhir.r4.model.Group;
import org.hl7.fhir.r4.model.Immunization;
import org.hl7.fhir.r4.model.ImmunizationRecommendation;
import org.hl7.fhir.r4.model.ListResource;
import org.hl7.fhir.r4.model.Location;
import org.hl7.fhir.r4.model.Media;
import org.hl7.fhir.r4.model.Medication;
@ -339,6 +340,9 @@ public abstract class BaseJpaR4Test extends BaseJpaTest implements ITestDataBuil
@Qualifier("myGroupDaoR4")
protected IFhirResourceDao<Group> myGroupDao;
@Autowired
@Qualifier("myListDaoR4")
protected IFhirResourceDao<ListResource> myListDao;
@Autowired
@Qualifier("myMolecularSequenceDaoR4")
protected IFhirResourceDao<MolecularSequence> myMolecularSequenceDao;
@Autowired

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

@ -26,6 +26,14 @@ public interface IAuthRuleBuilderOperationNamedAndScoped {
*/
IAuthRuleBuilderRuleOpClassifierFinished andAllowAllResponses();
/**
* Responses for this operation will not be checked and access to all resources is allowed. This
* is intended for operations which are known to fetch a graph of resources that is known to be
* safe, such as `$everything` which may access and fetch resources outside the patient's compartment
* but enforces safety in what it fetches via strict SQL queries.
*/
IAuthRuleBuilderRuleOpClassifierFinished andAllowAllResponsesWithAllResourcesAccess();
/**
* Responses for this operation must be authorized by other rules. For example, if this
* rule is authorizing the Patient $everything operation, there must be a separate

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

@ -24,12 +24,14 @@ import ca.uhn.fhir.interceptor.api.Pointcut;
import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor.Verdict;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import javax.annotation.Nonnull;
class OperationRule extends BaseRule implements IAuthRule {
private String myOperationName;
@ -41,6 +43,7 @@ class OperationRule extends BaseRule implements IAuthRule {
private boolean myAppliesToAnyInstance;
private boolean myAppliesAtAnyLevel;
private boolean myAllowAllResponses;
private boolean myAllowAllResourcesAccess;
OperationRule(String theRuleName) {
super(theRuleName);
@ -54,6 +57,10 @@ class OperationRule extends BaseRule implements IAuthRule {
myAllowAllResponses = true;
}
public void allowAllResourcesAccess() {
myAllowAllResourcesAccess = true;
}
void appliesToAnyInstance() {
myAppliesToAnyInstance = true;
}
@ -93,7 +100,7 @@ class OperationRule extends BaseRule implements IAuthRule {
// Operation rules apply to the execution of the operation itself, not to side effects like
// loading resources (that will presumably be reflected in the response). Those loads need
// to be explicitly authorized
if (isResourceAccess(thePointcut)) {
if (!myAllowAllResourcesAccess && isResourceAccess(thePointcut)) {
return null;
}
@ -258,4 +265,25 @@ class OperationRule extends BaseRule implements IAuthRule {
boolean isAllowAllResponses() {
return myAllowAllResponses;
}
boolean isAllowAllResourcesAccess() {
return myAllowAllResourcesAccess;
}
@Override
@Nonnull
protected ToStringBuilder toStringBuilder() {
ToStringBuilder builder = super.toStringBuilder();
builder.append("op", myOperationName);
builder.append("appliesToServer", myAppliesToServer);
builder.append("appliesToTypes", myAppliesToTypes);
builder.append("appliesToIds", myAppliesToIds);
builder.append("appliesToInstancesOfType", myAppliesToInstancesOfType);
builder.append("appliesToAnyType", myAppliesToAnyType);
builder.append("appliesToAnyInstance", myAppliesToAnyInstance);
builder.append("appliesAtAnyLevel", myAppliesAtAnyLevel);
builder.append("allowAllResponses", myAllowAllResponses);
builder.append("allowAllResourcesAccess", myAllowAllResourcesAccess);
return builder;
}
}

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

@ -777,6 +777,14 @@ public class RuleBuilder implements IAuthRuleBuilder {
return new RuleBuilderFinished(myRule);
}
@Override
public IAuthRuleBuilderRuleOpClassifierFinished andAllowAllResponsesWithAllResourcesAccess() {
myRule.allowAllResponses();
myRule.allowAllResourcesAccess();
myRules.add(myRule);
return new RuleBuilderFinished(myRule);
}
@Override
public IAuthRuleBuilderRuleOpClassifierFinished andRequireExplicitResponseAuthorization() {
myRules.add(myRule);

10
hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/OperationRuleTestUtil.java
View File

@ -21,8 +21,10 @@ package ca.uhn.fhir.rest.server.interceptor.auth;
import ca.uhn.fhir.rest.api.server.bulk.BulkExportJobParameters;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
import java.util.HashSet;
import java.util.List;
public final class OperationRuleTestUtil {
private OperationRuleTestUtil() {}
@ -51,10 +53,18 @@ public final class OperationRuleTestUtil {
return ((OperationRule) theRule).getAppliesToInstancesOfType();
}
public static boolean isAllowResourceAccess(IAuthRule theRule) {
return ((OperationRule) theRule).isAllowAllResourcesAccess();
}
public static boolean isAllowAllResponses(IAuthRule theRule) {
return ((OperationRule) theRule).isAllowAllResponses();
}
public static List<IIdType> getAppliesToIds(IAuthRule theRule) {
return ((OperationRule) theRule).getAppliesToIds();
}
public static String getGroupId(IAuthRule theRule) {
return ((RuleBulkExportImpl) theRule).getGroupId();
}

4
hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/test/utilities/ITestDataBuilder.java
View File

@ -231,6 +231,10 @@ public interface ITestDataBuilder {
return createResource("Organization", theModifiers);
}
default IIdType createPractitioner(ICreationArgument... theModifiers) {
return createResource("Practitioner", theModifiers);
}
default IIdType createResource(String theResourceType, ICreationArgument... theModifiers) {
IBaseResource resource = buildResource(theResourceType, theModifiers);