diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/5952-get-page-bundle-fails-forbidden-with-read-bundle-permission.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/5952-get-page-bundle-fails-forbidden-with-read-bundle-permission.yaml new file mode 100644 index 00000000000..cb714f6f550 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/5952-get-page-bundle-fails-forbidden-with-read-bundle-permission.yaml @@ -0,0 +1,6 @@ +--- +type: add +issue: 5952 +title: "Previously, since hapi-fhir 7.0, when retrieving the consecutive pages of a Bundle resource search operation +using a client with read permissions for Bundle resources, the request would fail with a 403 Forbidden error. +This has been fixed." diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java index 422db3d6fe7..d5e2f12f691 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java @@ -12,11 +12,8 @@ import ca.uhn.fhir.jpa.term.TermTestUtil; import ca.uhn.fhir.model.primitive.IdDt; 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; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; @@ -64,7 +61,8 @@ 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.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.NullSource; import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; @@ -73,15 +71,17 @@ import org.springframework.beans.factory.annotation.Autowired; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.List; +import java.util.stream.Stream; import static org.hamcrest.MatcherAssert.assertThat; 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.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -95,9 +95,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes private SearchParamMatcher mySearchParamMatcher; @Autowired private ThreadSafeResourceDeleterSvc myThreadSafeResourceDeleterSvc; - private AuthorizationInterceptor myReadAllBundleInterceptor; - private AuthorizationInterceptor myReadAllPatientInterceptor; - private AuthorizationInterceptor myWriteResourcesInTransactionAuthorizationInterceptor; @BeforeEach @Override @@ -106,9 +103,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes myStorageSettings.setAllowMultipleDelete(true); myStorageSettings.setExpungeEnabled(true); myStorageSettings.setDeleteExpungeEnabled(true); - myReadAllBundleInterceptor = new ReadAllAuthorizationInterceptor("Bundle"); - myReadAllPatientInterceptor = new ReadAllAuthorizationInterceptor("Patient"); - myWriteResourcesInTransactionAuthorizationInterceptor = new WriteResourcesInTransactionAuthorizationInterceptor(); } @Override @@ -116,6 +110,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes public void after() throws Exception { super.after(); myInterceptorRegistry.unregisterInterceptorsIf(t -> t instanceof AuthorizationInterceptor); + myClient.getInterceptorService().unregisterInterceptorsIf(t -> t instanceof SimpleRequestHeaderInterceptor); } /** @@ -232,9 +227,8 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes return new RuleBuilder() .allow().create().resourcesOfType("Patient").withAnyId().withTester(new IAuthRuleTester() { @Override - public boolean matches(RestOperationTypeEnum theOperation, RequestDetails theRequestDetails, IIdType theInputResourceId, IBaseResource theInputResource) { - if (theInputResource instanceof Patient) { - Patient patient = (Patient) theInputResource; + public boolean matches(RuleTestRequest theRequest) { + if (theRequest.resource instanceof Patient patient) { return patient .getIdentifier() .stream() @@ -317,53 +311,121 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes } + public static Stream getReadPatientArguments() { + return Stream.of( + Arguments.of(new ReadAllOfTypeAuthorizationInterceptor("Bundle"), false), + Arguments.of(new ReadAllOfTypeAuthorizationInterceptor("Patient"), true) + ); + } - @Test - public void testReadInTransaction() { + public static Stream getReadPatientInTransactionArguments() { + return Stream.of( + Arguments.of(new ReadAllOfTypeAndTransactionAuthorizationInterceptor("Bundle"), false), + Arguments.of(new ReadAllOfTypeAndTransactionAuthorizationInterceptor("Patient"), true) + ); + } - Patient patient = new Patient(); + @ParameterizedTest + @MethodSource(value = "getReadPatientArguments") + public void testReadPatientById(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + IIdType patient = createPatient(); + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); + + assertReadByIdAllowed(patient, theShouldAllow); + } + + @ParameterizedTest + @MethodSource(value = "getReadPatientInTransactionArguments") + public void testReadPatientInTransaction(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + + final Patient patient = new Patient(); patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100"); patient.addName().setFamily("Tester").addGiven("Raghad"); IIdType id = myClient.update().resource(patient).conditionalByUrl("Patient?identifier=http://uhn.ca/mrns|100").execute().getId().toUnqualifiedVersionless(); - myServer.getRestfulServer().registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { - @Override - public List buildRuleList(RequestDetails theRequestDetails) { - String authHeader = theRequestDetails.getHeader("Authorization"); - if (!"Bearer AAA".equals(authHeader)) { - throw new AuthenticationException("Invalid auth header: " + authHeader); - } - return new RuleBuilder() - .allow().transaction().withAnyOperation().andApplyNormalRules().andThen() - .allow().read().resourcesOfType(Patient.class).withAnyId() - .build(); - } - }); + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); + + SimpleRequestHeaderInterceptor interceptor = new SimpleRequestHeaderInterceptor("Authorization", "Bearer AAA"); + try { + myClient.registerInterceptor(interceptor); + + // Read Patient by id + Bundle bundle = new Bundle().setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl(id.getValue()); + assertTransactionAllowed(bundle, theShouldAllow); + + // Search all Patients + bundle = new Bundle().setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl("Patient?"); + assertTransactionAllowed(bundle, theShouldAllow); + } finally { + myClient.unregisterInterceptor(interceptor); + } + } + + public static Stream getReadStandaloneBundleArguments() { + return Stream.of( + Arguments.of(new ReadAllOfTypeAuthorizationInterceptor("Bundle"), true), + Arguments.of(new ReadAllOfTypeAuthorizationInterceptor("Patient"), false) + ); + } + + public static Stream getReadStandaloneBundleInTransactionArguments() { + return Stream.of( + Arguments.of(new ReadAllOfTypeAndTransactionAuthorizationInterceptor("Bundle"), true), + Arguments.of(new ReadAllOfTypeAndTransactionAuthorizationInterceptor("Patient"), false) + ); + } + + @ParameterizedTest + @MethodSource(value = "getReadStandaloneBundleArguments") + public void testReadBundleById(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + Bundle bundle = createDocumentBundle(createPatient("John", "Smith")); + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); + + assertReadByIdAllowed(bundle.getIdElement(), theShouldAllow); + } + + @ParameterizedTest + @MethodSource(value = "getReadStandaloneBundleInTransactionArguments") + public void testReadBundleInTransaction(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + Bundle documentBundle1 = createDocumentBundle(createPatient("John", "Smith")); + createDocumentBundle(createPatient("Jane", "Doe")); + IIdType collectionBundle1Id = documentBundle1.getIdElement(); + + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); SimpleRequestHeaderInterceptor interceptor = new SimpleRequestHeaderInterceptor("Authorization", "Bearer AAA"); try { myClient.registerInterceptor(interceptor); Bundle bundle; - Bundle responseBundle; - // Read + // Read Bundle bundle = new Bundle(); bundle.setType(Bundle.BundleType.TRANSACTION); - bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl(id.getValue()); - responseBundle = myClient.transaction().withBundle(bundle).execute(); - patient = (Patient) responseBundle.getEntry().get(0).getResource(); - assertEquals("Tester", patient.getNameFirstRep().getFamily()); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl(collectionBundle1Id.toUnqualifiedVersionless().getValue()); + assertTransactionAllowed(bundle, theShouldAllow); // Search bundle = new Bundle(); bundle.setType(Bundle.BundleType.TRANSACTION); - bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl("Patient?"); - responseBundle = myClient.transaction().withBundle(bundle).execute(); - responseBundle = (Bundle) responseBundle.getEntry().get(0).getResource(); - patient = (Patient) responseBundle.getEntry().get(0).getResource(); - assertEquals("Tester", patient.getNameFirstRep().getFamily()); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl("Bundle?type=collection"); + assertTransactionAllowed(bundle, theShouldAllow); + // Simple Search count 1 + Bundle responseBundle = assertSearchAllowed("/Bundle", theShouldAllow); + + // Get next page + if (responseBundle != null) { + Bundle.BundleLinkComponent next = responseBundle.getLink("next"); + assertNotNull(next); + + bundle = new Bundle(); + bundle.setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.GET).setUrl("/Bundle?" + Constants.PARAM_PAGINGACTION + next.getUrl()); + assertTransactionAllowed(bundle, theShouldAllow); + } } finally { myClient.unregisterInterceptor(interceptor); } @@ -396,7 +458,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes } }); - Bundle bundle; Observation response; // Read (no masking) @@ -412,7 +473,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes .elementsSubset("status") .execute(); assertEquals(ObservationStatus.FINAL, response.getStatus()); - assertEquals(null, response.getSubject().getReference()); + assertNull(response.getSubject().getReference()); // Read a non-allowed observation try { @@ -481,7 +542,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes patient.setId("Patient/A"); patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100"); patient.addName().setFamily("Tester").addGiven("Raghad"); - IIdType id = myClient.update().resource(patient).execute().getId(); + myClient.update().resource(patient).execute(); Observation obs = new Observation(); obs.setId("Observation/B"); @@ -796,12 +857,10 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes post = new HttpPost(myServerBase + "/Patient"); post.setEntity(new StringEntity(resource, ContentType.create(Constants.CT_FHIR_XML, "UTF-8"))); response = ourHttpClient.execute(post); - final IdType id2; try { assertEquals(201, response.getStatusLine().getStatusCode()); String newIdString = response.getFirstHeader(Constants.HEADER_LOCATION_LC).getValue(); assertThat(newIdString, startsWith(myServerBase + "/Patient/")); - id2 = new IdType(newIdString); } finally { response.close(); } @@ -945,7 +1004,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes httpGet = new HttpGet(myServerBase + "/Patient/B/$graphql?query=" + UrlUtil.escapeUrlParam(query)); try (CloseableHttpResponse response = ourHttpClient.execute(httpGet)) { - String resp = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); assertEquals(403, response.getStatusLine().getStatusCode()); } @@ -1185,7 +1243,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes myClient.create().resource(enc).execute(); try { - outcome = myClient + myClient .operation() .onInstance(pid1) .named(JpaConstants.OPERATION_EVERYTHING) @@ -1230,9 +1288,10 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes } }); - String patchBody = "[\n" + - " { \"op\": \"replace\", \"path\": \"/status\", \"value\": \"amended\" }\n" + - " ]"; + String patchBody = """ + [ + { "op": "replace", "path": "/status", "value": "amended" } + ]"""; // Allowed myClient.patch().withBody(patchBody).withId(oid1).execute(); @@ -1418,7 +1477,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes { String url = "/ExplanationOfBenefit?patient=" + p1id.getIdPart() + "," + p3id.getIdPart(); try { - Bundle result = myClient.search().byUrl(url).returnBundle(Bundle.class).execute(); + myClient.search().byUrl(url).returnBundle(Bundle.class).execute(); fail(); } catch (ForbiddenOperationException e) { assertThat(e.getMessage(), startsWith("HTTP 403 Forbidden: " + Msg.code(333) + "Access denied by rule")); @@ -1641,40 +1700,39 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes } - @Test - public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForDocumentBundles(){ - myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor); + @ParameterizedTest + @MethodSource(value = "getReadStandaloneBundleArguments") + public void testGetNextPage_forDocumentBundles(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); + Bundle bundle = createDocumentBundle(createPatient("John", "Smith")); + Bundle firstBundle = new Bundle(); + firstBundle.addLink().setRelation("next").setUrl(myClient.getServerBase() + "/"+ bundle.getIdElement().toUnqualifiedVersionless()); + assertGetNextPageAllowed(firstBundle, theShouldAllow); + } - Bundle bundle1 = createDocumentBundle(createPatient("John", "Smith")); - Bundle bundle2 = createDocumentBundle(createPatient("Jane", "Doe")); - assertSearchContainsResources("/Bundle", bundle1, bundle2); + @ParameterizedTest + @MethodSource(value = "getReadStandaloneBundleArguments") + public void testSearchBundles_forDocumentBundles(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); + createDocumentBundle(createPatient("John", "Smith")); + assertSearchAllowed("/Bundle", theShouldAllow); + } + + @ParameterizedTest + @MethodSource(value = "getReadStandaloneBundleArguments") + public void testSearchBundles_forMessageBundles(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); + createMessageHeaderBundle(createPatient("John", "Smith")); + assertSearchAllowed("/Bundle", theShouldAllow); } @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(){ + 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()) + new ReadInCompartmentAuthorizationInterceptor("Bundle", bundle1.getIdElement()) ); assertSearchContainsResources("/Bundle?_id=" + bundle1.getIdPart(), bundle1); @@ -1682,26 +1740,16 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes assertSearchFailsWith403Forbidden("/Bundle"); } - @Test - public void testSearchPatients_withPermissionToSearchAllBundles_returns403Forbidden(){ - myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor); - + @ParameterizedTest + @MethodSource(value = "getReadPatientArguments") + public void testSearchPatients(AuthorizationInterceptor theAuthorizationInterceptor, boolean theShouldAllow) { + myServer.getRestfulServer().registerInterceptor(theAuthorizationInterceptor); createPatient("John", "Smith"); - createPatient("Jane", "Doe"); - assertSearchFailsWith403Forbidden("/Patient"); + assertSearchAllowed("/Patient", theShouldAllow); } @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(){ + public void testSearchPatients_withPermissionToViewOnePatient_onlyAllowsViewingOnePatient() { Patient patient1 = createPatient("John", "Smith"); Patient patient2 = createPatient("Jane", "Doe"); @@ -1714,72 +1762,9 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes 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 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 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)); - } - - @ParameterizedTest - @ValueSource(strings = {"collection", "document", "message"}) - public void testPermissionsToPostTransaction_withValidNestedBundleRequest_successfullyPostsTransactions(String theBundleType){ + @ValueSource(strings = {"document", "message", "collection"}) + public void testTransactionBundle_withNestedNonTransactionBundle_allowed(String theBundleType) { BundleBuilder builder = new BundleBuilder(myFhirContext); builder.setType(theBundleType); IBaseBundle nestedBundle = builder.getBundle(); @@ -1788,7 +1773,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes builder.addTransactionCreateEntry(nestedBundle); IBaseBundle transaction = builder.getBundle(); - myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor); + myServer.getRestfulServer().registerInterceptor(new WriteResourcesInTransactionAuthorizationInterceptor()); myClient .transaction() @@ -1806,7 +1791,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes @ParameterizedTest @NullSource @ValueSource(strings = {"", "/"}) - public void testPermissionsToPostTransaction_withInvalidNestedBundleRequest_blocksTransaction(String theInvalidUrl){ + public void testTransactionBundle_withNestedTransactionBundle_notAllowed(String theInvalidUrl) { // inner transaction Patient patient = new Patient(); BundleBuilder builder = new BundleBuilder(myFhirContext); @@ -1820,7 +1805,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes entry.setResource(innerTransaction); entry.getRequest().setUrl(theInvalidUrl).setMethod(Bundle.HTTPVerb.POST); - myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor); + myServer.getRestfulServer().registerInterceptor(new WriteResourcesInTransactionAuthorizationInterceptor()); try { myClient @@ -1838,9 +1823,9 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes } @Test - public void testPermissionToPostTransaction_withUpdateParameters_blocksTransaction(){ + public void testTransactionBundle_withUpdateParameters_blocksTransaction() { DateType originalBirthDate = new DateType("2000-01-01"); - Patient patient = createPatient(originalBirthDate); + createPatient(originalBirthDate); DateType newBirthDate = new DateType("2005-01-01"); Parameters birthDatePatch = createPatientBirthdatePatch(newBirthDate); @@ -1849,7 +1834,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes bundleBuilder.addTransactionUpdateEntry(birthDatePatch); IBaseBundle transaction = bundleBuilder.getBundle(); - myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor); + myServer.getRestfulServer().registerInterceptor(new WriteResourcesInTransactionAuthorizationInterceptor()); try { myClient @@ -1870,7 +1855,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes } @Test - public void testPermissionToPostTransaction_withPatchParameters_successfullyPostsTransaction(){ + public void testTransactionBundle_withPatchParameters_allowed() { DateType originalBirthDate = new DateType("2000-01-01"); Patient patient = createPatient(originalBirthDate); @@ -1881,7 +1866,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes bundleBuilder.addTransactionFhirPatchEntry(patient.getIdElement(), birthDatePatch); IBaseBundle transaction = bundleBuilder.getBundle(); - myServer.getRestfulServer().registerInterceptor(myWriteResourcesInTransactionAuthorizationInterceptor); + myServer.getRestfulServer().registerInterceptor(new WriteResourcesInTransactionAuthorizationInterceptor()); myClient .transaction() @@ -1895,7 +1880,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes assertEquals(newBirthDate.getValueAsString(), savedPatient.getBirthDateElement().getValueAsString()); } - private Patient createPatient(String theFirstName, String theLastName){ + private Patient createPatient(String theFirstName, String theLastName) { Patient patient = new Patient(); patient.addName().addGiven(theFirstName).setFamily(theLastName); return (Patient) myPatientDao.create(patient, mySrd).getResource(); @@ -1907,7 +1892,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes return (Patient) myPatientDao.create(patient, mySrd).getResource(); } - private Bundle createDocumentBundle(Patient thePatient){ + 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()); @@ -1919,13 +1904,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes 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); @@ -1940,10 +1918,86 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes return (Bundle) myBundleDao.create(bundle, mySrd).getResource(); } - private void assertSearchContainsResources(String theUrl, Resource... theExpectedResources){ - List expectedIds = Arrays.stream(theExpectedResources) - .map(resource -> resource.getIdPart()) - .toList(); + private void assertReadByIdAllowed(IIdType theId, boolean theShouldAllow) { + if (theShouldAllow) { + IBaseResource resource = myClient.read() + .resource(theId.getResourceType()) + .withId(theId.toUnqualifiedVersionless().getValue()) + .execute(); + assertNotNull(resource); + return; + } + + try { + myClient.read() + .resource(theId.getResourceType()) + .withId(theId.toUnqualifiedVersionless().getValue()) + .execute(); + fail(); + } catch (Exception e) { + assertTrue(e.getMessage().contains("HTTP 403 Forbidden")); + } + } + + private void assertTransactionAllowed(Bundle theBundle, boolean theShouldAllow) { + Bundle outcome = myClient.transaction().withBundle(theBundle).execute(); + assertNotNull(outcome); + + if (theShouldAllow) { + assertNotNull(outcome.getEntry().get(0).getResource()); + } else { + assertNull(outcome.getEntry().get(0).getResource()); + assertTrue(outcome.getEntry().get(0).getResponse().getStatus().contains("403 Forbidden")); + } + } + + private Bundle assertSearchAllowed(String theUrl, boolean theShouldAllow) { + if (theShouldAllow) { + Bundle outcome = myClient.search() + .byUrl(theUrl) + .count(1) + .returnBundle(Bundle.class) + .execute(); + assertNotNull(outcome); + assertNotNull(outcome.getEntry().get(0).getResource()); + return outcome; + } + + try { + myClient.search() + .byUrl(theUrl) + .count(1) + .returnBundle(Bundle.class) + .execute(); + fail(); + } catch (Exception e) { + assertTrue(e.getMessage().contains("HTTP 403 Forbidden")); + } + + return null; + } + + private void assertGetNextPageAllowed(Bundle theBundle, boolean theShouldAllow) { + if (theShouldAllow) { + Bundle nextResult = myClient.loadPage() + .next(theBundle) + .execute(); + assertNotNull(nextResult); + return; + } + + try { + myClient.loadPage() + .next(theBundle) + .execute(); + fail(); + } catch (Exception e) { + assertTrue(e.getMessage().contains("HTTP 403 Forbidden")); + } + } + + private void assertSearchContainsResources(String theUrl, Resource... theExpectedResources) { + List expectedIds = Arrays.stream(theExpectedResources).map(Resource::getIdPart).toList(); Bundle searchResult = myClient .search() @@ -1959,23 +2013,16 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes assertTrue(expectedIds.containsAll(actualIds)); } - private void assertSearchFailsWith403Forbidden(String theUrl){ + private void assertSearchFailsWith403Forbidden(String theUrl) { try { myClient.search().byUrl(theUrl).execute(); fail(); - } catch (Exception e){ + } 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; - } - - private Parameters createPatientBirthdatePatch(DateType theNewBirthDate){ + private Parameters createPatientBirthdatePatch(DateType theNewBirthDate) { final Parameters patch = new Parameters(); final Parameters.ParametersParameterComponent op = patch.addParameter().setName("operation"); @@ -1986,20 +2033,33 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes return patch; } - static class ReadAllAuthorizationInterceptor extends AuthorizationInterceptor { + static class ReadAllOfTypeAndTransactionAuthorizationInterceptor extends ReadAllOfTypeAuthorizationInterceptor { - private final String myResourceType; + ReadAllOfTypeAndTransactionAuthorizationInterceptor(String... theResourceTypes) { + super(theResourceTypes); + } - public ReadAllAuthorizationInterceptor(String theResourceType){ + public List buildRuleList(RequestDetails theRequestDetails) { + List rules = new ArrayList<>(super.buildRuleList(theRequestDetails)); + List rulesToAdd = new RuleBuilder().allow().transaction().withAnyOperation().andApplyNormalRules().build(); + rules.addAll(rulesToAdd); + return rules; + } + } + + static class ReadAllOfTypeAuthorizationInterceptor extends AuthorizationInterceptor { + private final String[] myResourceTypes; + + public ReadAllOfTypeAuthorizationInterceptor(String... theResourceTypes) { super(PolicyEnum.DENY); - myResourceType = theResourceType; + myResourceTypes = theResourceTypes; } @Override public List buildRuleList(RequestDetails theRequestDetails) { - return new RuleBuilder() - .allow().read().resourcesOfType(myResourceType).withAnyId().andThen() - .build(); + return Arrays.stream(myResourceTypes).map(resourceType -> new RuleBuilder() + .allow().read().resourcesOfType(resourceType).withAnyId().andThen() + .build()).flatMap(Collection::stream).toList(); } } @@ -2008,7 +2068,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes private final String myResourceType; private final IIdType myId; - public ReadInCompartmentAuthorizationInterceptor(String theResourceType, IIdType theId){ + public ReadInCompartmentAuthorizationInterceptor(String theResourceType, IIdType theId) { super(PolicyEnum.DENY); myResourceType = theResourceType; myId = theId; @@ -2024,7 +2084,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes static class WriteResourcesInTransactionAuthorizationInterceptor extends AuthorizationInterceptor { - public WriteResourcesInTransactionAuthorizationInterceptor(){ + public WriteResourcesInTransactionAuthorizationInterceptor() { super(PolicyEnum.DENY); } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java index 1efa0e7e751..9e41218338e 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java @@ -80,11 +80,10 @@ 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 STANDALONE_BUNDLE_RESOURCE_TYPES = - Set.of(BundleTypeEnum.DOCUMENT, BundleTypeEnum.COLLECTION, BundleTypeEnum.MESSAGE); + Set.of(BundleTypeEnum.DOCUMENT, BundleTypeEnum.MESSAGE); private final int myInstanceIndex = ourInstanceCount.incrementAndGet(); private final String myRequestSeenResourcesKey = @@ -531,7 +530,7 @@ public class AuthorizationInterceptor implements IRuleApplier { case EXTENDED_OPERATION_TYPE: case EXTENDED_OPERATION_INSTANCE: { if (theResponseObject != null) { - resources = toListOfResourcesAndExcludeContainer(theRequestDetails, theResponseObject, fhirContext); + resources = toListOfResourcesAndExcludeContainer(theResponseObject, fhirContext); } break; } @@ -578,22 +577,15 @@ public class AuthorizationInterceptor implements IRuleApplier { OUT, } - public static List toListOfResourcesAndExcludeContainer( - RequestDetails theRequestDetails, IBaseResource theResponseObject, FhirContext fhirContext) { + protected static List toListOfResourcesAndExcludeContainer( + IBaseResource theResponseObject, FhirContext fhirContext) { if (theResponseObject == null) { return Collections.emptyList(); } List retVal; - boolean shouldExamineChildResources = false; - if (theResponseObject instanceof IBaseBundle) { - IBaseBundle bundle = (IBaseBundle) theResponseObject; - shouldExamineChildResources = shouldExamineBundleChildResources(theRequestDetails, fhirContext, bundle); - } else if (theResponseObject instanceof IBaseParameters) { - shouldExamineChildResources = true; - } - + boolean shouldExamineChildResources = shouldExamineChildResources(theResponseObject, fhirContext); if (!shouldExamineChildResources) { return Collections.singletonList(theResponseObject); } @@ -601,7 +593,7 @@ public class AuthorizationInterceptor implements IRuleApplier { retVal = fhirContext.newTerser().getAllPopulatedChildElementsOfType(theResponseObject, IBaseResource.class); // Exclude the container - if (retVal.size() > 0 && retVal.get(0) == theResponseObject) { + if (!retVal.isEmpty() && retVal.get(0) == theResponseObject) { retVal = retVal.subList(1, retVal.size()); } @@ -612,23 +604,22 @@ public class AuthorizationInterceptor implements IRuleApplier { } /** - * 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. + * This method determines if the given Resource should have permissions applied to the resources inside or + * to the Resource itself. + * For Parameters resources, we include child resources when checking the permissions. + * For Bundle resources, we only look at 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) { + protected static boolean shouldExamineChildResources(IBaseResource theResource, FhirContext theFhirContext) { + if (theResource instanceof IBaseParameters) { return true; } - BundleTypeEnum bundleType = BundleUtil.getBundleTypeEnum(theFhirContext, theBundle); - boolean isStandaloneBundleResource = - bundleType != null && STANDALONE_BUNDLE_RESOURCE_TYPES.contains(bundleType); - return !isStandaloneBundleResource; + if (theResource instanceof IBaseBundle) { + BundleTypeEnum bundleType = BundleUtil.getBundleTypeEnum(theFhirContext, ((IBaseBundle) theResource)); + boolean isStandaloneBundleResource = + bundleType != null && STANDALONE_BUNDLE_RESOURCE_TYPES.contains(bundleType); + return !isStandaloneBundleResource; + } + return false; } public static class Verdict { diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java index b924cf92a70..9cf6469e5a1 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java @@ -832,7 +832,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { } else if (theOutputResource != null) { List outputResources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer( - theRequestDetails, theOutputResource, theRequestDetails.getFhirContext()); + theOutputResource, theRequestDetails.getFhirContext()); Verdict verdict = null; for (IBaseResource nextResource : outputResources) { diff --git a/hapi-fhir-validation/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java b/hapi-fhir-validation/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java index 372b8ead4a8..5c19f58cf19 100644 --- a/hapi-fhir-validation/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java +++ b/hapi-fhir-validation/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java @@ -36,6 +36,7 @@ import ca.uhn.fhir.rest.api.ValidationModeEnum; import ca.uhn.fhir.rest.api.server.IPreResourceShowDetails; import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.api.server.SimplePreResourceShowDetails; +import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.api.server.storage.TransactionDetails; import ca.uhn.fhir.rest.param.ReferenceParam; import ca.uhn.fhir.rest.param.TokenAndListParam; @@ -68,6 +69,7 @@ import org.hl7.fhir.r4.model.Bundle; import org.hl7.fhir.r4.model.CarePlan; 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.Consent; import org.hl7.fhir.r4.model.Device; @@ -89,6 +91,8 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -204,7 +208,7 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline return retVal; } - private Resource createPatient(Integer theId) { + private Patient createPatient(Integer theId) { Patient retVal = new Patient(); if (theId != null) { retVal.setId(new IdType("Patient", (long) theId)); @@ -213,8 +217,8 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline return retVal; } - private Resource createPatient(Integer theId, int theVersion) { - Resource retVal = createPatient(theId); + private Patient createPatient(Integer theId, int theVersion) { + Patient retVal = createPatient(theId); retVal.setId(retVal.getIdElement().withVersion(Integer.toString(theVersion))); return retVal; } @@ -4222,6 +4226,59 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline assertTrue(ourHitMethod); } + @Test + public void testToListOfResourcesAndExcludeContainer_withSearchSetContainingDocumentBundles_onlyRecursesOneLevelDeep() { + Patient patient = createPatient(1); + Bundle bundle = new Bundle(); + bundle.setType(Bundle.BundleType.DOCUMENT); + bundle.addEntry().setResource(new Composition()); + bundle.addEntry().setResource(patient); + Bundle searchSet = new Bundle(); + searchSet.setType(Bundle.BundleType.SEARCHSET); + searchSet.addEntry().setResource(bundle); + + RequestDetails requestDetails = new SystemRequestDetails(); + requestDetails.setResourceName("Bundle"); + + List resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(searchSet, ourCtx); + assertEquals(1, resources.size()); + assertTrue(resources.contains(bundle)); + } + + @Test + public void testToListOfResourcesAndExcludeContainer_withSearchSetContainingPatients_returnsPatients() { + Patient patient1 = createPatient(1); + Patient patient2 = createPatient(2); + Bundle searchSet = new Bundle(); + searchSet.setType(Bundle.BundleType.SEARCHSET); + searchSet.addEntry().setResource(patient1); + searchSet.addEntry().setResource(patient2); + + RequestDetails requestDetails = new SystemRequestDetails(); + requestDetails.setResourceName("Patient"); + + List resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(searchSet, ourCtx); + assertEquals(2, resources.size()); + assertTrue(resources.contains(patient1)); + assertTrue(resources.contains(patient2)); + } + + @ParameterizedTest + @EnumSource(value = Bundle.BundleType.class, names = {"DOCUMENT", "MESSAGE"}) + public void testShouldExamineBundleResources_withBundleRequestAndStandAloneBundleType_returnsFalse(Bundle.BundleType theBundleType) { + Bundle bundle = new Bundle(); + bundle.setType(theBundleType); + assertFalse(AuthorizationInterceptor.shouldExamineChildResources(bundle, ourCtx)); + } + + @ParameterizedTest + @EnumSource(value = Bundle.BundleType.class, names = {"DOCUMENT", "MESSAGE"}, mode= EnumSource.Mode.EXCLUDE) + public void testShouldExamineBundleResources_withBundleRequestAndNonStandAloneBundleType_returnsTrue(Bundle.BundleType theBundleType) { + Bundle bundle = new Bundle(); + bundle.setType(theBundleType); + assertTrue(AuthorizationInterceptor.shouldExamineChildResources(bundle, ourCtx)); + } + @AfterAll public static void afterClassClearContext() throws Exception { TestUtil.randomizeLocaleAndTimezone();