Searching for Bundles with read all Bundles permission returns 403 (#5644)

* failing test

* another failing test case

* fix

* changelog

* fix bug

* spotless

* cr
This commit is contained in:
Nathan Doef 2024-02-02 11:12:00 -05:00 committed by GitHub
parent ec1b8fe7ee
commit 0537ab58db
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 346 additions and 12 deletions

View File

@ -91,7 +91,7 @@ public enum BundleTypeEnum {
/**
* Returns the enumerated value associated with this code
*/
public BundleTypeEnum forCode(String theCode) {
public static BundleTypeEnum forCode(String theCode) {
BundleTypeEnum retVal = CODE_TO_ENUM.get(theCode);
return retVal;
}

View File

@ -26,6 +26,7 @@ import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.context.RuntimeResourceDefinition;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.model.primitive.IdDt;
import ca.uhn.fhir.model.valueset.BundleTypeEnum;
import ca.uhn.fhir.rest.api.PatchTypeEnum;
import ca.uhn.fhir.rest.api.RequestTypeEnum;
import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
@ -235,6 +236,14 @@ public class BundleUtil {
return null;
}
public static BundleTypeEnum getBundleTypeEnum(FhirContext theContext, IBaseBundle theBundle) {
String bundleTypeCode = BundleUtil.getBundleType(theContext, theBundle);
if (isBlank(bundleTypeCode)) {
return null;
}
return BundleTypeEnum.forCode(bundleTypeCode);
}
public static void setBundleType(FhirContext theContext, IBaseBundle theBundle, String theType) {
RuntimeResourceDefinition def = theContext.getResourceDefinition(theBundle);
BaseRuntimeChildDefinition entryChild = def.getChildByName("type");

View File

@ -0,0 +1,8 @@
---
type: fix
issue: 5644
title: "Previously, searching for `Bundle` resources with read all `Bundle` resources permissions, returned an
HTTP 403 Forbidden error. This was because the `AuthorizationInterceptor` applied permissions to the resources inside
the `Bundle`, instead of the `Bundle` itself. This has been fixed and permissions are no longer applied to the resources
inside a `Bundle` of type `document`, `message`, or `collection` for `Bundle` requests."

View File

@ -1,7 +1,6 @@
package ca.uhn.fhir.jpa.provider.r4;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.batch2.jobs.export.BulkDataExportProvider;
import ca.uhn.fhir.jpa.delete.ThreadSafeResourceDeleterSvc;
import ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor;
import ca.uhn.fhir.jpa.model.util.JpaConstants;
@ -14,6 +13,7 @@ import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.api.MethodOutcome;
import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
import ca.uhn.fhir.rest.client.interceptor.SimpleRequestHeaderInterceptor;
import ca.uhn.fhir.rest.server.exceptions.AuthenticationException;
import ca.uhn.fhir.rest.server.exceptions.ForbiddenOperationException;
@ -37,11 +37,13 @@ import org.hl7.fhir.instance.model.api.IIdType;
import org.hl7.fhir.r4.model.Bundle;
import org.hl7.fhir.r4.model.CodeableConcept;
import org.hl7.fhir.r4.model.Coding;
import org.hl7.fhir.r4.model.Composition;
import org.hl7.fhir.r4.model.Condition;
import org.hl7.fhir.r4.model.Encounter;
import org.hl7.fhir.r4.model.ExplanationOfBenefit;
import org.hl7.fhir.r4.model.IdType;
import org.hl7.fhir.r4.model.Identifier;
import org.hl7.fhir.r4.model.MessageHeader;
import org.hl7.fhir.r4.model.Observation;
import org.hl7.fhir.r4.model.Observation.ObservationStatus;
import org.hl7.fhir.r4.model.Organization;
@ -49,11 +51,14 @@ import org.hl7.fhir.r4.model.Parameters;
import org.hl7.fhir.r4.model.Patient;
import org.hl7.fhir.r4.model.Practitioner;
import org.hl7.fhir.r4.model.Reference;
import org.hl7.fhir.r4.model.Resource;
import org.hl7.fhir.r4.model.StringType;
import org.hl7.fhir.r4.model.ValueSet;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
@ -68,7 +73,9 @@ import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Test {
@ -79,6 +86,8 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
private SearchParamMatcher mySearchParamMatcher;
@Autowired
private ThreadSafeResourceDeleterSvc myThreadSafeResourceDeleterSvc;
private AuthorizationInterceptor myReadAllBundleInterceptor;
private AuthorizationInterceptor myReadAllPatientInterceptor;
@BeforeEach
@Override
@ -87,7 +96,8 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
myStorageSettings.setAllowMultipleDelete(true);
myStorageSettings.setExpungeEnabled(true);
myStorageSettings.setDeleteExpungeEnabled(true);
myServer.getRestfulServer().registerInterceptor(new BulkDataExportProvider());
myReadAllBundleInterceptor = new ReadAllAuthorizationInterceptor("Bundle");
myReadAllPatientInterceptor = new ReadAllAuthorizationInterceptor("Patient");
}
@Override
@ -1506,4 +1516,250 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
}
}
@Test
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForDocumentBundles(){
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);
Bundle bundle1 = createDocumentBundle(createPatient("John", "Smith"));
Bundle bundle2 = createDocumentBundle(createPatient("Jane", "Doe"));
assertSearchContainsResources("/Bundle", bundle1, bundle2);
}
@Test
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForCollectionBundles(){
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);
Bundle bundle1 = createCollectionBundle(createPatient("John", "Smith"));
Bundle bundle2 = createCollectionBundle(createPatient("Jane", "Doe"));
assertSearchContainsResources("/Bundle", bundle1, bundle2);
}
@Test
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForMessageBundles(){
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);
Bundle bundle1 = createMessageHeaderBundle(createPatient("John", "Smith"));
Bundle bundle2 = createMessageHeaderBundle(createPatient("Jane", "Doe"));
assertSearchContainsResources("/Bundle", bundle1, bundle2);
}
@Test
public void testSearchBundles_withPermissionToViewOneBundle_onlyAllowsViewingOneBundle(){
Bundle bundle1 = createMessageHeaderBundle(createPatient("John", "Smith"));
Bundle bundle2 = createMessageHeaderBundle(createPatient("Jane", "Doe"));
myServer.getRestfulServer().getInterceptorService().registerInterceptor(
new ReadInCompartmentAuthorizationInterceptor("Bundle", bundle1.getIdElement())
);
assertSearchContainsResources("/Bundle?_id=" + bundle1.getIdPart(), bundle1);
assertSearchFailsWith403Forbidden("/Bundle?_id=" + bundle2.getIdPart());
assertSearchFailsWith403Forbidden("/Bundle");
}
@Test
public void testSearchPatients_withPermissionToSearchAllBundles_returns403Forbidden(){
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);
createPatient("John", "Smith");
createPatient("Jane", "Doe");
assertSearchFailsWith403Forbidden("/Patient");
}
@Test
public void testSearchPatients_withPermissionToSearchAllPatients_returnsAllPatients(){
myServer.getRestfulServer().registerInterceptor(myReadAllPatientInterceptor);
Patient patient1 = createPatient("John", "Smith");
Patient patient2 = createPatient("Jane", "Doe");
assertSearchContainsResources("/Patient", patient1, patient2);
}
@Test
public void testSearchPatients_withPermissionToViewOnePatient_onlyAllowsViewingOnePatient(){
Patient patient1 = createPatient("John", "Smith");
Patient patient2 = createPatient("Jane", "Doe");
myServer.getRestfulServer().getInterceptorService().registerInterceptor(
new ReadInCompartmentAuthorizationInterceptor("Patient", patient1.getIdElement())
);
assertSearchContainsResources("/Patient?_id=" + patient1.getIdPart(), patient1);
assertSearchFailsWith403Forbidden("/Patient?_id=" + patient2.getIdPart());
assertSearchFailsWith403Forbidden("/Patient");
}
@Test
public void testToListOfResourcesAndExcludeContainer_withSearchSetContainingDocumentBundles_onlyRecursesOneLevelDeep() {
Bundle bundle1 = createDocumentBundle(createPatient("John", "Smith"));
Bundle bundle2 = createDocumentBundle(createPatient("John", "Smith"));
Bundle searchSet = createSearchSet(bundle1, bundle2);
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Bundle");
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(requestDetails, searchSet, myFhirContext);
assertEquals(2, resources.size());
assertTrue(resources.contains(bundle1));
assertTrue(resources.contains(bundle2));
}
@Test
public void testToListOfResourcesAndExcludeContainer_withSearchSetContainingPatients_returnsPatients() {
Patient patient1 = createPatient("John", "Smith");
Patient patient2 = createPatient("Jane", "Doe");
Bundle searchSet = createSearchSet(patient1, patient2);
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Patient");
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(requestDetails, searchSet, myFhirContext);
assertEquals(2, resources.size());
assertTrue(resources.contains(patient1));
assertTrue(resources.contains(patient2));
}
@ParameterizedTest
@EnumSource(value = Bundle.BundleType.class, names = {"DOCUMENT", "COLLECTION", "MESSAGE"})
public void testShouldExamineBundleResources_withBundleRequestAndStandAloneBundleType_returnsFalse(Bundle.BundleType theBundleType){
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Bundle");
Bundle bundle = new Bundle();
bundle.setType(theBundleType);
assertFalse(AuthorizationInterceptor.shouldExamineBundleChildResources(requestDetails, myFhirContext, bundle));
}
@ParameterizedTest
@EnumSource(value = Bundle.BundleType.class, names = {"DOCUMENT", "COLLECTION", "MESSAGE"}, mode= EnumSource.Mode.EXCLUDE)
public void testShouldExamineBundleResources_withBundleRequestAndNonStandAloneBundleType_returnsTrue(Bundle.BundleType theBundleType){
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Bundle");
Bundle bundle = new Bundle();
bundle.setType(theBundleType);
assertTrue(AuthorizationInterceptor.shouldExamineBundleChildResources(requestDetails, myFhirContext, bundle));
}
@ParameterizedTest
@EnumSource(value = Bundle.BundleType.class)
public void testShouldExamineBundleResources_withNonBundleRequests_returnsTrue(Bundle.BundleType theBundleType){
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Patient");
Bundle bundle = new Bundle();
bundle.setType(theBundleType);
assertTrue(AuthorizationInterceptor.shouldExamineBundleChildResources(requestDetails, myFhirContext, bundle));
}
private Patient createPatient(String theFirstName, String theLastName){
Patient patient = new Patient();
patient.addName().addGiven(theFirstName).setFamily(theLastName);
return (Patient) myPatientDao.create(patient, mySrd).getResource();
}
private Bundle createDocumentBundle(Patient thePatient){
Composition composition = new Composition();
composition.setType(new CodeableConcept().addCoding(new Coding().setSystem("http://example.org").setCode("some-type")));
composition.getSubject().setReference(thePatient.getIdElement().getValue());
Bundle bundle = new Bundle();
bundle.setType(Bundle.BundleType.DOCUMENT);
bundle.addEntry().setResource(composition);
bundle.addEntry().setResource(thePatient);
return (Bundle) myBundleDao.create(bundle, mySrd).getResource();
}
private Bundle createCollectionBundle(Patient thePatient) {
Bundle bundle = new Bundle();
bundle.setType(Bundle.BundleType.COLLECTION);
bundle.addEntry().setResource(thePatient);
return (Bundle) myBundleDao.create(bundle, mySrd).getResource();
}
private Bundle createMessageHeaderBundle(Patient thePatient) {
Bundle bundle = new Bundle();
bundle.setType(Bundle.BundleType.MESSAGE);
MessageHeader messageHeader = new MessageHeader();
Coding event = new Coding().setSystem("http://acme.com").setCode("some-event");
messageHeader.setEvent(event);
messageHeader.getFocusFirstRep().setReference(thePatient.getIdElement().getValue());
bundle.addEntry().setResource(messageHeader);
bundle.addEntry().setResource(thePatient);
return (Bundle) myBundleDao.create(bundle, mySrd).getResource();
}
private void assertSearchContainsResources(String theUrl, Resource... theExpectedResources){
List<String> expectedIds = Arrays.stream(theExpectedResources)
.map(resource -> resource.getIdPart())
.toList();
Bundle searchResult = myClient
.search()
.byUrl(theUrl)
.returnBundle(Bundle.class)
.execute();
List<String> actualIds = searchResult.getEntry().stream()
.map(entry -> entry.getResource().getIdPart())
.toList();
assertEquals(expectedIds.size(), actualIds.size());
assertTrue(expectedIds.containsAll(actualIds));
}
private void assertSearchFailsWith403Forbidden(String theUrl){
try {
myClient.search().byUrl(theUrl).execute();
fail();
} catch (Exception e){
assertTrue(e.getMessage().contains("HTTP 403 Forbidden"));
}
}
private Bundle createSearchSet(Resource... theResources){
Bundle bundle = new Bundle();
bundle.setType(Bundle.BundleType.SEARCHSET);
Arrays.stream(theResources).forEach(resource -> bundle.addEntry().setResource(resource));
return bundle;
}
static class ReadAllAuthorizationInterceptor extends AuthorizationInterceptor {
private final String myResourceType;
public ReadAllAuthorizationInterceptor(String theResourceType){
super(PolicyEnum.DENY);
myResourceType = theResourceType;
}
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().read().resourcesOfType(myResourceType).withAnyId().andThen()
.build();
}
}
static class ReadInCompartmentAuthorizationInterceptor extends AuthorizationInterceptor {
private final String myResourceType;
private final IIdType myId;
public ReadInCompartmentAuthorizationInterceptor(String theResourceType, IIdType theId){
super(PolicyEnum.DENY);
myResourceType = theResourceType;
myId = theId;
}
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().read().allResources().inCompartment(myResourceType, myId).andThen()
.build();
}
}
}

View File

@ -25,12 +25,14 @@ import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.interceptor.api.Hook;
import ca.uhn.fhir.interceptor.api.Interceptor;
import ca.uhn.fhir.interceptor.api.Pointcut;
import ca.uhn.fhir.model.valueset.BundleTypeEnum;
import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
import ca.uhn.fhir.rest.api.server.IPreResourceShowDetails;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.api.server.bulk.BulkExportJobParameters;
import ca.uhn.fhir.rest.server.exceptions.ForbiddenOperationException;
import ca.uhn.fhir.rest.server.interceptor.consent.ConsentInterceptor;
import ca.uhn.fhir.util.BundleUtil;
import com.google.common.collect.Lists;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
@ -78,8 +80,12 @@ public class AuthorizationInterceptor implements IRuleApplier {
public static final String REQUEST_ATTRIBUTE_BULK_DATA_EXPORT_OPTIONS =
AuthorizationInterceptor.class.getName() + "_BulkDataExportOptions";
public static final String BUNDLE = "Bundle";
private static final AtomicInteger ourInstanceCount = new AtomicInteger(0);
private static final Logger ourLog = LoggerFactory.getLogger(AuthorizationInterceptor.class);
private static final Set<BundleTypeEnum> STANDALONE_BUNDLE_RESOURCE_TYPES =
Set.of(BundleTypeEnum.DOCUMENT, BundleTypeEnum.COLLECTION, BundleTypeEnum.MESSAGE);
private final int myInstanceIndex = ourInstanceCount.incrementAndGet();
private final String myRequestSeenResourcesKey =
AuthorizationInterceptor.class.getName() + "_" + myInstanceIndex + "_SEENRESOURCES";
@ -525,7 +531,7 @@ public class AuthorizationInterceptor implements IRuleApplier {
case EXTENDED_OPERATION_TYPE:
case EXTENDED_OPERATION_INSTANCE: {
if (theResponseObject != null) {
resources = toListOfResourcesAndExcludeContainer(theResponseObject, fhirContext);
resources = toListOfResourcesAndExcludeContainer(theRequestDetails, theResponseObject, fhirContext);
}
break;
}
@ -572,22 +578,23 @@ public class AuthorizationInterceptor implements IRuleApplier {
OUT,
}
static List<IBaseResource> toListOfResourcesAndExcludeContainer(
IBaseResource theResponseObject, FhirContext fhirContext) {
public static List<IBaseResource> toListOfResourcesAndExcludeContainer(
RequestDetails theRequestDetails, IBaseResource theResponseObject, FhirContext fhirContext) {
if (theResponseObject == null) {
return Collections.emptyList();
}
List<IBaseResource> retVal;
boolean isContainer = false;
boolean shouldExamineChildResources = false;
if (theResponseObject instanceof IBaseBundle) {
isContainer = true;
IBaseBundle bundle = (IBaseBundle) theResponseObject;
shouldExamineChildResources = shouldExamineBundleChildResources(theRequestDetails, fhirContext, bundle);
} else if (theResponseObject instanceof IBaseParameters) {
isContainer = true;
shouldExamineChildResources = true;
}
if (!isContainer) {
if (!shouldExamineChildResources) {
return Collections.singletonList(theResponseObject);
}
@ -604,6 +611,26 @@ public class AuthorizationInterceptor implements IRuleApplier {
return retVal;
}
/**
* This method determines if the given Bundle should have permissions applied to the resources inside or
* to the Bundle itself.
*
* This distinction is important in Bundle requests where a user has permissions to view all Bundles. In
* this scenario we want to apply permissions to the Bundle itself and not the resources inside if
* the Bundle is of type document, collection, or message.
*/
public static boolean shouldExamineBundleChildResources(
RequestDetails theRequestDetails, FhirContext theFhirContext, IBaseBundle theBundle) {
boolean isBundleRequest = theRequestDetails != null && BUNDLE.equals(theRequestDetails.getResourceName());
if (!isBundleRequest) {
return true;
}
BundleTypeEnum bundleType = BundleUtil.getBundleTypeEnum(theFhirContext, theBundle);
boolean isStandaloneBundleResource =
bundleType != null && STANDALONE_BUNDLE_RESOURCE_TYPES.contains(bundleType);
return !isStandaloneBundleResource;
}
public static class Verdict {
private final IAuthRule myDecidingRule;

View File

@ -817,7 +817,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
} else if (theOutputResource != null) {
List<IBaseResource> outputResources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(
theOutputResource, theRequestDetails.getFhirContext());
theRequestDetails, theOutputResource, theRequestDetails.getFhirContext());
Verdict verdict = null;
for (IBaseResource nextResource : outputResources) {

View File

@ -3,10 +3,12 @@ package ca.uhn.fhir.util.bundle;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.model.valueset.BundleEntrySearchModeEnum;
import ca.uhn.fhir.model.valueset.BundleTypeEnum;
import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
import ca.uhn.fhir.util.BundleBuilder;
import ca.uhn.fhir.util.BundleUtil;
import ca.uhn.fhir.util.TestUtil;
import jakarta.annotation.Nonnull;
import org.hl7.fhir.instance.model.api.IBase;
import org.hl7.fhir.instance.model.api.IBaseBundle;
import org.hl7.fhir.instance.model.api.IBaseResource;
@ -25,8 +27,9 @@ import org.hl7.fhir.r4.model.UriType;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import jakarta.annotation.Nonnull;
import java.util.Collections;
import java.util.List;
import java.util.function.Consumer;
@ -555,6 +558,37 @@ public class BundleUtilTest {
assertNull(actual);
}
@ParameterizedTest
@CsvSource({
// Actual BundleType Expected BundleTypeEnum
"TRANSACTION, TRANSACTION",
"DOCUMENT, DOCUMENT",
"MESSAGE, MESSAGE",
"BATCHRESPONSE, BATCH_RESPONSE",
"TRANSACTIONRESPONSE, TRANSACTION_RESPONSE",
"HISTORY, HISTORY",
"SEARCHSET, SEARCHSET",
"COLLECTION, COLLECTION"
})
public void testGetBundleTypeEnum_withKnownBundleTypes_returnsCorrectBundleTypeEnum(Bundle.BundleType theBundleType, BundleTypeEnum theExpectedBundleTypeEnum){
Bundle bundle = new Bundle();
bundle.setType(theBundleType);
assertEquals(theExpectedBundleTypeEnum, BundleUtil.getBundleTypeEnum(ourCtx, bundle));
}
@Test
public void testGetBundleTypeEnum_withNullBundleType_returnsNull(){
Bundle bundle = new Bundle();
bundle.setType(Bundle.BundleType.NULL);
assertNull(BundleUtil.getBundleTypeEnum(ourCtx, bundle));
}
@Test
public void testGetBundleTypeEnum_withNoBundleType_returnsNull(){
Bundle bundle = new Bundle();
assertNull(BundleUtil.getBundleTypeEnum(ourCtx, bundle));
}
@Nonnull
private static Bundle withBundle(Resource theResource) {
final Bundle bundle = new Bundle();