issue #2369 - use the resource from the event after interceptor calls

This allows the interceptors to modify the incoming resource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

docs/src/pages/guides/FHIRServerUsersGuide.md

@@ -659,21 +659,21 @@ By default, notification messages are published for all _create_ and _update_ pe
 With the `includeResourceTypes`property set as in the preceding example, the FHIR server publishes notification events only for `Patient` and `Observation` resources. If you omit this property or set its value to `[]` (an empty array), then the FHIR server publishes notifications for all resource types.
 
 ## 4.3 Persistence interceptors
-The FHIR server supports a persistence interceptor feature that enables users to add their own logic to the REST API processing flow around persistence events. This could be used to enforce application-specific business rules associated with resources. Interceptor methods can be called immediately before or after _create_ and _update_ persistence operations.
+The FHIR server supports a persistence interceptor feature that enables users to add their own logic to the REST API processing flow around persistence events. This can be used to enforce application-specific business rules associated with resources. Interceptor methods are called immediately before or after each persistence operation.
 
 ### 4.3.1 FHIRPersistenceInterceptor interface
 A persistence interceptor implementation must implement the `com.ibm.fhir.persistence.interceptor.FHIRPersistenceInterceptor`
 interface.
 
 Each interceptor method receives a parameter of type `FHIRPersistenceEvent`, which contains context information related to the request being processed at the time that the interceptor method is invoked. It includes the FHIR resource, security information, request URI information, and the collection of HTTP headers associated with the request.
 
-There are two primary use cases for persistence interceptors:
+There are many use cases for persistence interceptors:
 
-1.  Enforce certain application-specific governance rules, such as making sure that a patient has signed a consent form prior to allowing his/her data to be stored in the FHIR server's datastore. In this case, the `beforeCreate` or `beforeUpdate` interceptor methods could verify that the patient has a consent agreement on file, and if not then throw a `FHIRPersistenceInterceptorException` to prevent the _create_ or _update_ persistence events from completing normally. The exception thrown by the interceptor method will be propagated back to the FHIR server request processing flow and would result in an `OperationOutcome` being returned in the REST API response, along with a `Bad Request` HTTP status code.
+1.  Enforce certain application-specific governance rules, such as making sure that a patient has signed a consent form prior to allowing his/her data to be stored in the FHIR server's datastore. For example, the `beforeCreate` and `beforeUpdate` methods could verify that the patient has a consent agreement on file and, if not, then throw a `FHIRPersistenceInterceptorException` to prevent the _create_ or _update_ events from completing. The exception thrown by the interceptor method should include one or more OperationOutcome issues and these issues will be added to an `OperationOutcome` in the REST API response. The HTTP status code of the response will be determined by the IssueType of the first issue in the list.
 
-2.  Perform some additional processing steps associated with a _create_ or _update_ persistence event, such as additional audit logging. In this case, the `afterCreate` and `afterUpdate` interceptor methods could add records to an audit log to indicate the request URI that was invoked, the user associated with the invocation request, and so forth.
+2.  Perform additional access control. For example, `beforeSearch` can be used to alter the incoming SearchContext (e.g. by adding additional search parameters). Similarly `afterRead`, `afterVRead`, `afterHistory`, and `afterSearch` can be used to verify that the end user is authorized to access the resources before they are returned.
 
-In general, the `beforeCreate` and `beforeUpdate` interceptor methods would be useful to perform an enforcement-type action where you would potentially want to prevent the request processing flow from finishing. Conversely, the `afterCreate` and `afterUpdate` interceptor methods would be useful in situations where you need to perform additional steps after the _create_ or _update_ persistence events have been performed.
+It is also possible to modify the incoming resources from the `beforeCreate` and `beforeUpdate` methods. For example, an interceptor could be used to add tags to resources on their way into the server. However, it is important to realize that interceptors are called *after* resource validation. Therefore, interceptor authors must be careful not to alter the resources in a way that breaks conformance with the profiles claimed in Resource.meta.profile or the secondary constraints in the specification. When in doubt, interceptors that modify the incoming resource can use the FHIRValidator to re-validate the resource(s) after they are altered.
 
 ### 4.3.2 Implementing a persistence interceptor
 To implement a persistence interceptor, complete the following steps:
@@ -2442,7 +2442,7 @@ A copy of this snippet is provided here for illustrative purposes:
             </security-role>
         </application-bnd>
     </webApplication>
-    
+
     <mpJwt id="jwtConsumer"
            jwksUri="http://keycloak:8080/auth/realms/test/protocol/openid-connect/certs"
            issuer="https://localhost:8443/auth/realms/test"

fhir-server/liberty-config/server.xml

@@ -46,7 +46,7 @@
         
         Example trace specifications: 
         <logging traceSpecification="*=info:com.ibm.fhir.*=finer" traceFormat="BASIC"/>
-        <logging traceSpecification="*=info:com.ibm.fhir.persistence.jdbc.dao.impl.*=fine" traceFormat="BASIC"/>
+        <logging traceSpecification="com.ibm.fhir.persistence.jdbc.dao.impl.*=fine:com.ibm.fhir.database.utils.query.QueryUtil.level=FINE" traceFormat="BASIC"/>
         <logging traceSpecification="${TRACE_SPEC}" traceFileName="${TRACE_FILE}" traceFormat="${TRACE_FORMAT}" consoleLogLevel="${WLP_LOGGING_CONSOLE_LOGLEVEL}"/>
         
         To send the trace messages to standard out, set TRACE_FILE to "stdout".

fhir-server/pom.xml

@@ -113,6 +113,10 @@
             <artifactId>fhir-persistence-jdbc</artifactId>
             <version>${project.version}</version>
         </dependency>
+        <dependency>
+            <groupId>org.owasp.encoder</groupId>
+            <artifactId>encoder</artifactId>
+        </dependency>
         <dependency>
             <groupId>org.glassfish.tyrus.bundles</groupId>
             <artifactId>tyrus-standalone-client-jdk</artifactId>
@@ -139,8 +143,9 @@
             <scope>test</scope>
         </dependency>
         <dependency>
-            <groupId>org.owasp.encoder</groupId>
-            <artifactId>encoder</artifactId>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <scope>test</scope>
         </dependency>
     </dependencies>
 

fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java

@@ -265,6 +265,9 @@ public FHIRRestOperationResponse doCreate(String type, Resource resource, String
                     new FHIRPersistenceEvent(resource, buildPersistenceEventProperties(type, null, null, null));
             getInterceptorMgr().fireBeforeCreateEvent(event);
 
+            // write the resource back in case the interceptors modified it in some way
+            resource = event.getFhirResource();
+
             FHIRPersistenceContext persistenceContext =
                     FHIRPersistenceContextFactory.createPersistenceContext(event);
 
@@ -514,6 +517,9 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
                 }
             }
 
+            // write the resource back in case the interceptors modified it in some way
+            newResource = event.getFhirResource();
+
             FHIRPersistenceContext persistenceContext =
                     FHIRPersistenceContextFactory.createPersistenceContext(event);
             SingleResourceResult<Resource> result = persistence.update(persistenceContext, id, newResource);
@@ -2014,8 +2020,6 @@ private Entry processEntryForPost(Entry requestEntry, Entry validationResponseEn
         MultivaluedMap<String, String> queryParams = requestURL.getQueryParameters();
         Resource resource = null;
 
-        FHIRRequestContext requestContext = FHIRRequestContext.get();
-
         // Process a POST (create or search, or custom operation).
         if (pathTokens.length > 0 && pathTokens[pathTokens.length - 1].startsWith("$")) {
             // This is a custom operation request.

fhir-server/src/test/java/com/ibm/fhir/server/util/FHIRRestHelperTest.java

@@ -7,6 +7,8 @@
 package com.ibm.fhir.server.util;
 
 import static com.ibm.fhir.model.type.String.string;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.fail;
@@ -15,6 +17,8 @@
 
 import javax.ws.rs.core.Response;
 
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
 import org.testng.annotations.Test;
 
 import com.ibm.fhir.config.FHIRRequestContext;
@@ -29,10 +33,14 @@
 import com.ibm.fhir.model.resource.Practitioner;
 import com.ibm.fhir.model.resource.Practitioner.Qualification;
 import com.ibm.fhir.model.resource.Procedure;
+import com.ibm.fhir.model.resource.Resource;
 import com.ibm.fhir.model.type.Code;
 import com.ibm.fhir.model.type.CodeableConcept;
 import com.ibm.fhir.model.type.Coding;
 import com.ibm.fhir.model.type.HumanName;
+import com.ibm.fhir.model.type.Id;
+import com.ibm.fhir.model.type.Instant;
+import com.ibm.fhir.model.type.Meta;
 import com.ibm.fhir.model.type.Narrative;
 import com.ibm.fhir.model.type.Reference;
 import com.ibm.fhir.model.type.Uri;
@@ -45,9 +53,15 @@
 import com.ibm.fhir.model.type.code.NarrativeStatus;
 import com.ibm.fhir.model.type.code.ProcedureStatus;
 import com.ibm.fhir.persistence.FHIRPersistence;
+import com.ibm.fhir.persistence.SingleResourceResult;
+import com.ibm.fhir.persistence.interceptor.FHIRPersistenceEvent;
+import com.ibm.fhir.persistence.interceptor.FHIRPersistenceInterceptor;
+import com.ibm.fhir.persistence.interceptor.FHIRPersistenceInterceptorException;
+import com.ibm.fhir.persistence.interceptor.impl.FHIRPersistenceInterceptorMgr;
 import com.ibm.fhir.search.context.FHIRSearchContext;
 import com.ibm.fhir.search.context.FHIRSearchContextFactory;
 import com.ibm.fhir.server.test.MockPersistenceImpl;
+import com.ibm.fhir.server.test.MockTransactionAdapter;
 
 public class FHIRRestHelperTest {
 
@@ -1978,4 +1992,74 @@ public void testBundleSearchBundleWithNullRsrcAndNoId() throws Exception {
         assertEquals("A resource with no data was found.", operationOutcome.getIssue().get(0).getDetails().getText().getValue());
         assertEquals("A resource with no id was found.", operationOutcome.getIssue().get(1).getDetails().getText().getValue());
     }
+
+    /**
+     * Test an interceptor that modifies the resource
+     */
+    @Test
+    public void testResourceModifyingInterceptor() throws Exception {
+        Coding TAG = Coding.builder().code(Code.of("test")).build();
+
+        FHIRPersistenceInterceptor interceptor = new FHIRPersistenceInterceptor() {
+            @Override
+            public void beforeCreate(FHIRPersistenceEvent event) throws FHIRPersistenceInterceptorException {
+                event.setFhirResource(addTag(event.getFhirResource()));
+            }
+
+            @Override
+            public void beforeUpdate(FHIRPersistenceEvent event) throws FHIRPersistenceInterceptorException {
+                event.setFhirResource(addTag(event.getFhirResource()));
+            }
+
+            @Override
+            public void beforePatch(FHIRPersistenceEvent event) throws FHIRPersistenceInterceptorException {
+                event.setFhirResource(addTag(event.getFhirResource()));
+            }
+
+            private Resource addTag(Resource r) throws FHIRPersistenceInterceptorException {
+                try {
+                    Meta.Builder metaBuilder = r.getMeta() != null ? r.getMeta().toBuilder()
+                            : Meta.builder().source(Uri.of("interceptor"));
+                    return r.toBuilder().meta(metaBuilder.tag(TAG).build()).build();
+                } catch (Exception e) {
+                    throw new FHIRPersistenceInterceptorException("Unexpected error while adding a tag", e);
+                }
+            }
+        };
+        FHIRPersistenceInterceptorMgr.getInstance().addInterceptor(interceptor);
+
+        Patient patientNoId = Patient.builder()
+                .name(HumanName.builder()
+                    .given(string("John"))
+                    .family(string("Doe"))
+                    .build())
+                .build();
+        Patient patientWithId = patientNoId.toBuilder()
+                .id("123")
+                .meta(Meta.builder()
+                    .lastUpdated(Instant.now())
+                    .versionId(Id.of("1"))
+                    .build())
+                .build();
+
+        FHIRPersistence persistence = Mockito.mock(FHIRPersistence.class);
+        @SuppressWarnings("unchecked")
+        SingleResourceResult<Resource> mockResult = Mockito.mock(SingleResourceResult.class);
+        when(mockResult.getResource()).thenReturn(patientWithId);
+
+        when(persistence.getTransaction()).thenReturn(new MockTransactionAdapter());
+        when(persistence.read(any(), any(), any())).thenReturn(mockResult);
+        when(persistence.create(any(), any())).thenReturn(mockResult);
+        when(persistence.update(any(), any(), any())).thenReturn(mockResult);
+        FHIRRestHelper helper = new FHIRRestHelper(persistence);
+
+        ArgumentCaptor<Patient> patientCaptor = ArgumentCaptor.forClass(Patient.class);
+        helper.doCreate("Patient", patientNoId, null);
+        Mockito.verify(persistence).create(any(), patientCaptor.capture());
+        assertEquals(patientCaptor.getValue().getMeta().getTag().get(0), TAG);
+
+        helper.doUpdate("Patient", "123", patientWithId, null, null, false);
+        Mockito.verify(persistence).update(any(), any(), patientCaptor.capture());
+        assertEquals(patientCaptor.getValue().getMeta().getTag().get(0), TAG);
+    }
 }