Skip to content
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

Jump to bottom

4941 performing javascript reject operation during consent validation does not return 403 #4951

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+119 −10
Open

4941 performing javascript reject operation during consent validation does not return 403 #4951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7460650
Add forbid methods
SouradeepSahaSmile May 25, 2023
fdf748b
Merge branch 'master' into 4941-performing-javascript-reject-operatio…
SouradeepSahaSmile May 26, 2023
b5953af
Remove Forbid from hapi-fhir and add to cdr
SouradeepSahaSmile May 30, 2023
cdce5ec
Interim code for ConsentInterceptorTest
SouradeepSahaSmile Jun 2, 2023
3d11a01
enhancing tests.
Jun 2, 2023
8d9b607
Interim code for ConsentInterceptorTest
SouradeepSahaSmile Jun 5, 2023
bdd3fd0
Final changes
SouradeepSahaSmile Jun 5, 2023
cf3f7f4
Update changelog
SouradeepSahaSmile Jun 5, 2023
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
4 changes: 2 additions & 2 deletions hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/ConsentInterceptors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ public ConsentOutcome startOperation(RequestDetails theRequestDetails, IConsentC
@Override
public ConsentOutcome canSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices) {
// In this basic example, we will filter out lab results so that they
// are never disclosed to the user. A real interceptor might do something
// more nuanced.
// are never disclosed to the user. A real interceptor might do something more nuance or entirely
// forbid the operation by returning ConsentOutcome.FORBID;
if (theResource instanceof Observation) {
Observation obs = (Observation)theResource;
if (obs.getCategoryFirstRep().hasCoding("https://hl7.org/fhir/codesystem-observation-category.html", "laboratory")) {
Expand Down
5 changes: 5 additions & 0 deletions ...s/ca/uhn/hapi/fhir/changelog/6_8_0/4941-adding-forbidden-operation-to-consentservice.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
type: add
issue: 4941
jira: smile-6485
title: "Performing javascript forbid operation during consent validation now returns error 403"
7 changes: 7 additions & 0 deletions ...-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/consent/ConsentInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ public void interceptPreHandled(RequestDetails theRequestDetails) {
Validate.notNull(outcome, "Consent service returned null outcome");

switch (outcome.getStatus()) {
case FORBID:
case REJECT:
throw toForbiddenOperationException(outcome);
case PROCEED:
Expand Down Expand Up @@ -246,6 +247,8 @@ public void interceptPreAccess(RequestDetails theRequestDetails, IPreResourceAcc
thePreResourceAccessDetails.setDontReturnResourceAtIndex(resourceIdx);
skipSubsequentServices = true;
break;
case FORBID:
throw toForbiddenOperationException(outcome);
}

if (skipSubsequentServices) {
Expand Down Expand Up @@ -295,6 +298,8 @@ public void interceptPreShow(RequestDetails theRequestDetails, IPreResourceShowD
thePreResourceShowDetails.setResource(i, newResource);
}
continue;
case FORBID:
epeartree marked this conversation as resolved.
Show resolved Hide resolved
throw toForbiddenOperationException(nextOutcome);
case REJECT:
if (nextOutcome.getOperationOutcome() != null) {
IBaseOperationOutcome newOperationOutcome = nextOutcome.getOperationOutcome();
Expand Down Expand Up @@ -345,6 +350,7 @@ public void interceptOutgoingResponse(RequestDetails theRequestDetails, Response
}

switch (outcome.getStatus()) {
case FORBID:
case REJECT:
if (outcome.getOperationOutcome() != null) {
theResource.setResponseResource(outcome.getOperationOutcome());
Expand Down Expand Up @@ -393,6 +399,7 @@ public boolean acceptElement(IBase theElement, List<IBase> theContainingElementP
boolean shouldReplaceResource = false;

switch (childOutcome.getStatus()) {
case FORBID:
case REJECT:
replacementResource = childOutcome.getOperationOutcome();
shouldReplaceResource = true;
Expand Down
7 changes: 7 additions & 0 deletions ...src/main/java/ca/uhn/fhir/rest/server/interceptor/consent/ConsentOperationStatusEnum.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,11 @@ public enum ConsentOperationStatusEnum {
*/
AUTHORIZED,

/**
* The requested operation cannot proceed, and an operation outcome suitable for
* the user is forbidden. This was added to allow for 403 forbidden error from
* ConsentServices.
*/
FORBID
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont' understand what this change is for. How is FORBID different from REJECT? It has identical javadocs which suggests that it is the same. If it's not the same, the javadocs need to be updated to reflect exactly when someone would use one vs the other, and the actual documentation on hapifhir.io/hapi-fhir/docs should also be updated to make this clear. Also Unit Tests should be added that exercize the differences.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intend behind adding the FORBID operation outcome is to permit a client to specify that the server should return 403 from any of the consent service javascript callback as REJECT will only trigger a 403 from 'consentStartOperation'.

Comment and documentation will be updated.


}
5 changes: 5 additions & 0 deletions ...fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/consent/ConsentOutcome.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ public class ConsentOutcome {
*/
public static final ConsentOutcome AUTHORIZED = new ConsentOutcome(ConsentOperationStatusEnum.AUTHORIZED);

/**
* Convenience constant containing <code>new ConsentOutcome(ConsentOperationStatusEnum.FORBID)</code>
*/
public static final ConsentOutcome FORBID = new ConsentOutcome(ConsentOperationStatusEnum.FORBID);

private final ConsentOperationStatusEnum myStatus;
private final IBaseOperationOutcome myOperationOutcome;
private final IBaseResource myResource;
Expand Down
101 changes: 93 additions & 8 deletions ...ructures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,11 @@

import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.interceptor.api.Hook;
import ca.uhn.fhir.interceptor.api.Pointcut;
import ca.uhn.fhir.rest.annotation.Operation;
import ca.uhn.fhir.rest.annotation.OperationParam;
import ca.uhn.fhir.rest.annotation.RequiredParam;
import ca.uhn.fhir.rest.annotation.Search;
import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.api.RequestTypeEnum;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.client.api.IGenericClient;
import ca.uhn.fhir.rest.param.StringParam;
Expand All @@ -28,8 +25,11 @@
import com.helger.commons.collection.iterate.EmptyEnumeration;
import org.apache.commons.collections4.iterators.IteratorEnumeration;
import org.apache.commons.io.IOUtils;
import org.apache.http.HttpHeaders;
epeartree marked this conversation as resolved.
Show resolved Hide resolved
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpPut;
import org.apache.http.entity.StringEntity;
import org.hl7.fhir.instance.model.api.IBaseParameters;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.r4.model.Bundle;
Expand All @@ -53,7 +53,6 @@
import javax.servlet.ServletInputStream;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.EOFException;
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintWriter;
Expand All @@ -64,18 +63,14 @@
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.not;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.timeout;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.withSettings;

@ExtendWith(MockitoExtension.class)
public class ConsentInterceptorTest {
Expand Down Expand Up @@ -153,6 +148,94 @@
verify(myConsentSvc, timeout(2000).times(0)).completeOperationFailure(any(), any(), any());
}

@Test
public void testContentService_whenForbiddingOperationOnServerIncomingRequestPreHande_returnsForbidden() throws IOException {

Check notice on line 152 in hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java#L152 

The JUnit 5 test method name 'testContentService_whenForbiddingOperationOnServerIncomingRequestPreHande_returnsForbidden' doesn't match '[a-z][a-zA-Z0-9]*'
Patient patientA = new Patient();
patientA.setId("PT-1-0");
patientA.setActive(true);
patientA.addName().setFamily("FAMILY").addGiven("GIVEN");
patientA.addIdentifier().setSystem("SYSTEM").setValue("VALUEA");
ourPatientProvider.store(patientA);

when(myConsentSvc.startOperation(any(), any())).thenReturn(ConsentOutcome.FORBID);

HttpPut httpPut = new HttpPut("https://localhost:" + myPort + "/Patient/PT-1-0");
httpPut.setHeader(HttpHeaders.CONTENT_TYPE, "application/json");

httpPut.setEntity(new StringEntity("{\"resourceType\": \"Patient\", \"id\": \"PT-1-0\",\"text\": {\"status\": \"generated\",\"div\": \"<div><p>A valid patient resource for testing purposes</p></div>\" },\"gender\": \"male\"}"));

try (CloseableHttpResponse status = myClient.execute(httpPut)) {
ourLog.info("RESULT {}", status);
assertEquals(403, status.getStatusLine().getStatusCode());
String responseContent = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
ourLog.info("Response: {}", responseContent);
}
}

@Test
public void testContentService_whenForbiddingOperationOnStoragePreaccessResources_returnsForbidden() throws IOException {

Check notice on line 176 in hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java#L176 

The JUnit 5 test method name 'testContentService_whenForbiddingOperationOnStoragePreaccessResources_returnsForbidden' doesn't match '[a-z][a-zA-Z0-9]*'
Patient patientA = new Patient();
patientA.setId("PT-1-0");
patientA.setActive(true);
patientA.addName().setFamily("FAMILY").addGiven("GIVEN");
patientA.addIdentifier().setSystem("SYSTEM").setValue("VALUEA");
ourPatientProvider.store(patientA);

when(myConsentSvc.canSeeResource(any(), any(), any())).thenReturn(ConsentOutcome.FORBID);

HttpGet httpGet = new HttpGet("https://localhost:" + myPort + "/Patient/PT-1-0");

try (CloseableHttpResponse status = myClient.execute(httpGet)) {
ourLog.info("RESULT {}", status);
assertEquals(403, status.getStatusLine().getStatusCode());
String responseContent = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
ourLog.info("Response: {}", responseContent);
}
}

@Test
public void testContentService_whenForbiddingOperationOnStoragePreShowResources_returnsForbidden() throws IOException {

Check notice on line 197 in hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java#L197 

The JUnit 5 test method name 'testContentService_whenForbiddingOperationOnStoragePreShowResources_returnsForbidden' doesn't match '[a-z][a-zA-Z0-9]*'
Patient patientA = new Patient();
patientA.setId("PT-1-0");
patientA.setActive(true);
patientA.addName().setFamily("FAMILY").addGiven("GIVEN");
patientA.addIdentifier().setSystem("SYSTEM").setValue("VALUEA");
ourPatientProvider.store(patientA);

when(myConsentSvc.willSeeResource(any(), any(), any())).thenReturn(ConsentOutcome.FORBID);

HttpGet httpGet = new HttpGet("https://localhost:" + myPort + "/Patient/PT-1-0");

try (CloseableHttpResponse status = myClient.execute(httpGet)) {
ourLog.info("RESULT {}", status);
assertEquals(403, status.getStatusLine().getStatusCode());
String responseContent = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
ourLog.info("Response: {}", responseContent);
}
}

@Test
public void testContentService_whenForbiddingOperationOnServerOutgoingResponse_returnsForbidden() throws IOException {

Check notice on line 218 in hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java#L218 

The JUnit 5 test method name 'testContentService_whenForbiddingOperationOnServerOutgoingResponse_returnsForbidden' doesn't match '[a-z][a-zA-Z0-9]*'
Patient patientA = new Patient();
patientA.setId("PT-1-0");
patientA.setActive(true);
patientA.addName().setFamily("FAMILY").addGiven("GIVEN");
patientA.addIdentifier().setSystem("SYSTEM").setValue("VALUEA");
ourPatientProvider.store(patientA);

when(myConsentSvc.willSeeResource(any(), any(), any())).thenReturn(ConsentOutcome.PROCEED);
when(myConsentSvc.willSeeResource(any(), any(), any())).thenReturn(ConsentOutcome.FORBID);

HttpGet httpGet = new HttpGet("https://localhost:" + myPort + "/Patient/PT-1-0");

try (CloseableHttpResponse status = myClient.execute(httpGet)) {
ourLog.info("RESULT {}", status);
assertEquals(403, status.getStatusLine().getStatusCode());
String responseContent = IOUtils.toString(status.getEntity().getContent(), Charsets.UTF_8);
ourLog.info("Response: {}", responseContent);
}
}

@Test
public void testTotalModeIgnoredForConsentQueries() throws IOException {
Patient patientA = new Patient();
Expand Down Expand Up @@ -247,6 +330,8 @@

}



@Test
public void testMetadataCallHasChecksSkipped() throws IOException{
HttpGet httpGet = new HttpGet("https://localhost:" + myPort + "/metadata");
Expand Down