Skip to content

Fix authorization handling for Bundle resources in the output #5953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 28, 2024
Merged

Fix authorization handling for Bundle resources in the output #5953

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3dbaef2
Fix authorization handling for Bundle resources in the output. When t…
codeforgreen May 23, 2024
73d4812
Merge remote-tracking branch 'origin/master' into mm-20240522-bundle-…
codeforgreen May 24, 2024
f5ce68c
Add some more tests to cover more use cases.
codeforgreen May 25, 2024
b259409
Add more tests. Exclude collection type as it can be returned by a cu…
codeforgreen May 27, 2024
34a74b3
Address code review comment. Remove unused method.
codeforgreen May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...hir/changelog/7_4_0/5952-get-page-bundle-fails-forbidden-with-read-bundle-permission.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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."
128 changes: 63 additions & 65 deletions ...-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;

import static org.hamcrest.MatcherAssert.assertThat;
Expand All @@ -96,7 +97,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
@Autowired
private ThreadSafeResourceDeleterSvc myThreadSafeResourceDeleterSvc;
private AuthorizationInterceptor myReadAllBundleInterceptor;
private AuthorizationInterceptor myReadAllPatientInterceptor;
private AuthorizationInterceptor myWriteResourcesInTransactionAuthorizationInterceptor;

@BeforeEach
Expand All @@ -107,7 +107,6 @@ public void before() throws Exception {
myStorageSettings.setExpungeEnabled(true);
myStorageSettings.setDeleteExpungeEnabled(true);
myReadAllBundleInterceptor = new ReadAllAuthorizationInterceptor("Bundle");
myReadAllPatientInterceptor = new ReadAllAuthorizationInterceptor("Patient");
myWriteResourcesInTransactionAuthorizationInterceptor = new WriteResourcesInTransactionAuthorizationInterceptor();
}

Expand Down Expand Up @@ -396,7 +395,6 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
}
});

Bundle bundle;
Observation response;

// Read (no masking)
Expand All @@ -412,7 +410,7 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
.elementsSubset("status")
.execute();
assertEquals(ObservationStatus.FINAL, response.getStatus());
assertEquals(null, response.getSubject().getReference());
assertNull(response.getSubject().getReference());

// Read a non-allowed observation
try {
Expand Down Expand Up @@ -481,7 +479,7 @@ public void testDeleteInCompartmentIsBlocked() {
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");
Expand Down Expand Up @@ -796,12 +794,10 @@ public void testDeleteResourceConditional() throws IOException {
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();
}
Expand Down Expand Up @@ -945,7 +941,7 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {

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);
IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8);
assertEquals(403, response.getStatusLine().getStatusCode());
}

Expand Down Expand Up @@ -1185,7 +1181,7 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
myClient.create().resource(enc).execute();

try {
outcome = myClient
myClient
.operation()
.onInstance(pid1)
.named(JpaConstants.OPERATION_EVERYTHING)
Expand Down Expand Up @@ -1230,9 +1226,10 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
}
});

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();
Expand Down Expand Up @@ -1418,7 +1415,7 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
{
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"));
Expand Down Expand Up @@ -1642,7 +1639,24 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
}

@Test
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForDocumentBundles(){
public void testSearchBundlesWithPaging_withPermissionToSearchAllBundles_succeedsForDocumentBundles() {
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);

createDocumentBundle(createPatient("John", "Smith"));
createDocumentBundle(createPatient("Jane", "Doe"));
Bundle searchResult = myClient
.search()
.byUrl("/Bundle")
.count(1)
.returnBundle(Bundle.class)
.execute();
myClient.loadPage()
.next(searchResult)
.execute();
}

@Test
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForDocumentBundles() {
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);

Bundle bundle1 = createDocumentBundle(createPatient("John", "Smith"));
Expand All @@ -1651,7 +1665,7 @@ public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403F
}

@Test
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForCollectionBundles(){
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForCollectionBundles() {
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);

Bundle bundle1 = createCollectionBundle(createPatient("John", "Smith"));
Expand All @@ -1660,7 +1674,7 @@ public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403F
}

@Test
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForMessageBundles(){
public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403ForbiddenForMessageBundles() {
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);

Bundle bundle1 = createMessageHeaderBundle(createPatient("John", "Smith"));
Expand All @@ -1669,7 +1683,7 @@ public void testSearchBundles_withPermissionToSearchAllBundles_doesNotReturn403F
}

@Test
public void testSearchBundles_withPermissionToViewOneBundle_onlyAllowsViewingOneBundle(){
public void testSearchBundles_withPermissionToViewOneBundle_onlyAllowsViewingOneBundle() {
Bundle bundle1 = createMessageHeaderBundle(createPatient("John", "Smith"));
Bundle bundle2 = createMessageHeaderBundle(createPatient("Jane", "Doe"));

Expand All @@ -1683,7 +1697,7 @@ public void testSearchBundles_withPermissionToViewOneBundle_onlyAllowsViewingOne
}

@Test
public void testSearchPatients_withPermissionToSearchAllBundles_returns403Forbidden(){
public void testSearchPatients_withPermissionToSearchAllBundles_returns403Forbidden() {
myServer.getRestfulServer().registerInterceptor(myReadAllBundleInterceptor);

createPatient("John", "Smith");
Expand All @@ -1692,16 +1706,16 @@ public void testSearchPatients_withPermissionToSearchAllBundles_returns403Forbid
}

@Test
public void testSearchPatients_withPermissionToSearchAllPatients_returnsAllPatients(){
myServer.getRestfulServer().registerInterceptor(myReadAllPatientInterceptor);
public void testSearchPatients_withPermissionToSearchAllPatients_returnsAllPatients() {
myServer.getRestfulServer().registerInterceptor(new ReadAllAuthorizationInterceptor("Patient"));

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");

Expand All @@ -1723,7 +1737,7 @@ public void testToListOfResourcesAndExcludeContainer_withSearchSetContainingDocu
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Bundle");

List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(requestDetails, searchSet, myFhirContext);
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(searchSet, myFhirContext);
assertEquals(2, resources.size());
assertTrue(resources.contains(bundle1));
assertTrue(resources.contains(bundle2));
Expand All @@ -1738,48 +1752,33 @@ public void testToListOfResourcesAndExcludeContainer_withSearchSetContainingPati
RequestDetails requestDetails = new SystemRequestDetails();
requestDetails.setResourceName("Patient");

List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(requestDetails, searchSet, myFhirContext);
List<IBaseResource> resources = AuthorizationInterceptor.toListOfResourcesAndExcludeContainer(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");
public void testShouldExamineBundleResources_withBundleRequestAndStandAloneBundleType_returnsFalse(Bundle.BundleType theBundleType) {
Bundle bundle = new Bundle();
bundle.setType(theBundleType);

assertFalse(AuthorizationInterceptor.shouldExamineBundleChildResources(requestDetails, myFhirContext, bundle));
assertFalse(AuthorizationInterceptor.shouldExamineChildResources(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");
public void testShouldExamineBundleResources_withBundleRequestAndNonStandAloneBundleType_returnsTrue(Bundle.BundleType theBundleType) {
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));
assertTrue(AuthorizationInterceptor.shouldExamineChildResources(myFhirContext, bundle));
}

@ParameterizedTest
@ValueSource(strings = {"collection", "document", "message"})
public void testPermissionsToPostTransaction_withValidNestedBundleRequest_successfullyPostsTransactions(String theBundleType){
public void testPermissionsToPostTransaction_withValidNestedBundleRequest_successfullyPostsTransactions(String theBundleType) {
BundleBuilder builder = new BundleBuilder(myFhirContext);
builder.setType(theBundleType);
IBaseBundle nestedBundle = builder.getBundle();
Expand All @@ -1806,7 +1805,7 @@ public void testPermissionsToPostTransaction_withValidNestedBundleRequest_succes
@ParameterizedTest
@NullSource
@ValueSource(strings = {"", "/"})
public void testPermissionsToPostTransaction_withInvalidNestedBundleRequest_blocksTransaction(String theInvalidUrl){
public void testPermissionsToPostTransaction_withInvalidNestedBundleRequest_blocksTransaction(String theInvalidUrl) {
// inner transaction
Patient patient = new Patient();
BundleBuilder builder = new BundleBuilder(myFhirContext);
Expand Down Expand Up @@ -1838,9 +1837,9 @@ public void testPermissionsToPostTransaction_withInvalidNestedBundleRequest_bloc
}

@Test
public void testPermissionToPostTransaction_withUpdateParameters_blocksTransaction(){
public void testPermissionToPostTransaction_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);
Expand Down Expand Up @@ -1870,7 +1869,7 @@ public void testPermissionToPostTransaction_withUpdateParameters_blocksTransacti
}

@Test
public void testPermissionToPostTransaction_withPatchParameters_successfullyPostsTransaction(){
public void testPermissionToPostTransaction_withPatchParameters_successfullyPostsTransaction() {
DateType originalBirthDate = new DateType("2000-01-01");
Patient patient = createPatient(originalBirthDate);

Expand All @@ -1895,7 +1894,7 @@ public void testPermissionToPostTransaction_withPatchParameters_successfullyPost
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();
Expand All @@ -1907,7 +1906,7 @@ private Patient createPatient(DateType theBirthDate) {
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());
Expand Down Expand Up @@ -1940,14 +1939,13 @@ private Bundle createMessageHeaderBundle(Patient 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();
private void assertSearchContainsResources(String theUrl, Resource... theExpectedResources) {
List<String> expectedIds = Arrays.stream(theExpectedResources).map(Resource::getIdPart).toList();

Bundle searchResult = myClient
.search()
.byUrl(theUrl)
.count(theExpectedResources.length)
.returnBundle(Bundle.class)
.execute();

Expand All @@ -1959,23 +1957,23 @@ private void assertSearchContainsResources(String theUrl, Resource... theExpecte
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){
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");
Expand All @@ -1988,18 +1986,18 @@ private Parameters createPatientBirthdatePatch(DateType theNewBirthDate){

static class ReadAllAuthorizationInterceptor extends AuthorizationInterceptor {

private final String myResourceType;
private final String[] myResourceTypes;

public ReadAllAuthorizationInterceptor(String theResourceType){
public ReadAllAuthorizationInterceptor(String... theResourceTypes) {
super(PolicyEnum.DENY);
myResourceType = theResourceType;
myResourceTypes = theResourceTypes;
}

@Override
public List<IAuthRule> 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();
}
}

Expand All @@ -2008,7 +2006,7 @@ static class ReadInCompartmentAuthorizationInterceptor extends AuthorizationInte
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;
Expand All @@ -2024,7 +2022,7 @@ public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {

static class WriteResourcesInTransactionAuthorizationInterceptor extends AuthorizationInterceptor {

public WriteResourcesInTransactionAuthorizationInterceptor(){
public WriteResourcesInTransactionAuthorizationInterceptor() {
super(PolicyEnum.DENY);
}

Expand Down
Loading