From 31de3f4599f9edcfeab995ce8074583264a15457 Mon Sep 17 00:00:00 2001 From: Vivek Mittal Date: Thu, 13 Apr 2023 17:51:12 +0530 Subject: [PATCH] Documentation for request mutation change. Added test cases. --- .../plugin/AccessGrantedAndUpdateList.java | 2 +- .../BearerAuthorizationInterceptor.java | 16 +++----- .../fhir/gateway/CapabilityPostProcessor.java | 2 +- .../gateway/interfaces/AccessDecision.java | 19 +++++---- .../interfaces/NoOpAccessDecision.java | 2 +- .../gateway/interfaces/RequestMutation.java | 6 ++- .../BearerAuthorizationInterceptorTest.java | 39 +++++++++++++++++++ 7 files changed, 65 insertions(+), 21 deletions(-) diff --git a/plugins/src/main/java/com/google/fhir/gateway/plugin/AccessGrantedAndUpdateList.java b/plugins/src/main/java/com/google/fhir/gateway/plugin/AccessGrantedAndUpdateList.java index 7ea988ed..fdd36907 100644 --- a/plugins/src/main/java/com/google/fhir/gateway/plugin/AccessGrantedAndUpdateList.java +++ b/plugins/src/main/java/com/google/fhir/gateway/plugin/AccessGrantedAndUpdateList.java @@ -65,7 +65,7 @@ private AccessGrantedAndUpdateList( } @Override - public RequestMutation preprocess(RequestDetailsReader requestDetailsReader) { + public RequestMutation getRequestMutation(RequestDetailsReader requestDetailsReader) { return null; } diff --git a/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java b/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java index aa6dfa9a..42bb010c 100644 --- a/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java +++ b/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java @@ -272,7 +272,7 @@ public boolean authorizeRequest(RequestDetails requestDetails) { return false; } AccessDecision outcome = checkAuthorization(requestDetails); - preprocessRequest(servletDetails, outcome); + mutateRequest(requestDetails, outcome); logger.debug("Authorized request path " + requestPath); try { HttpResponse response = fhirClient.handleRequest(servletDetails); @@ -394,20 +394,16 @@ private void serveWellKnown(ServletRequestDetails request) { } } - private void preprocessRequest( - ServletRequestDetails servletRequestDetails, AccessDecision accessDecision) { + @VisibleForTesting + static void mutateRequest(RequestDetails requestDetails, AccessDecision accessDecision) { RequestMutation mutation = - accessDecision.preprocess(new RequestDetailsToReader(servletRequestDetails)); + accessDecision.getRequestMutation(new RequestDetailsToReader(requestDetails)); if (mutation == null || CollectionUtils.isEmpty(mutation.getQueryParams())) { - return ; + return; } mutation .getQueryParams() - .forEach((key, value) -> servletRequestDetails.addParameter( - key, value.toArray(new String[0]))); - - // TODO update the query params in search by Post - + .forEach((key, value) -> requestDetails.addParameter(key, value.toArray(new String[0]))); } } diff --git a/server/src/main/java/com/google/fhir/gateway/CapabilityPostProcessor.java b/server/src/main/java/com/google/fhir/gateway/CapabilityPostProcessor.java index 73d2da0e..399fffe0 100644 --- a/server/src/main/java/com/google/fhir/gateway/CapabilityPostProcessor.java +++ b/server/src/main/java/com/google/fhir/gateway/CapabilityPostProcessor.java @@ -55,7 +55,7 @@ static synchronized CapabilityPostProcessor getInstance(FhirContext fhirContext) } @Override - public RequestMutation preprocess(RequestDetailsReader requestDetailsReader) { + public RequestMutation getRequestMutation(RequestDetailsReader requestDetailsReader) { return null; } diff --git a/server/src/main/java/com/google/fhir/gateway/interfaces/AccessDecision.java b/server/src/main/java/com/google/fhir/gateway/interfaces/AccessDecision.java index 83f586c7..cc49026f 100644 --- a/server/src/main/java/com/google/fhir/gateway/interfaces/AccessDecision.java +++ b/server/src/main/java/com/google/fhir/gateway/interfaces/AccessDecision.java @@ -16,22 +16,27 @@ package com.google.fhir.gateway.interfaces; import java.io.IOException; +import javax.annotation.Nullable; import org.apache.http.HttpResponse; public interface AccessDecision { + /** @return true iff access was granted. */ + boolean canAccess(); + /** * Allows the incoming request mutation based on the access decision. * + *

Response is used to mutate the incoming request before executing the FHIR operation. We + * currently only support query parameters update for GET Http method. This is expected to be + * called after checking the access using @canAccess method. Mutating the request before checking + * access can have side effect of wrong access check. + * * @param requestDetailsReader details about the resource and operation requested - * @return the mutation to be applied on the incoming request + * @return mutation to be applied on the incoming request or null if no mutation required */ - RequestMutation preprocess(RequestDetailsReader requestDetailsReader); - - /** - * @return true iff access was granted. - */ - boolean canAccess(); + @Nullable + RequestMutation getRequestMutation(RequestDetailsReader requestDetailsReader); /** * Depending on the outcome of the FHIR operations, this does any post-processing operations that diff --git a/server/src/main/java/com/google/fhir/gateway/interfaces/NoOpAccessDecision.java b/server/src/main/java/com/google/fhir/gateway/interfaces/NoOpAccessDecision.java index b9d609b3..d0394811 100644 --- a/server/src/main/java/com/google/fhir/gateway/interfaces/NoOpAccessDecision.java +++ b/server/src/main/java/com/google/fhir/gateway/interfaces/NoOpAccessDecision.java @@ -26,7 +26,7 @@ public NoOpAccessDecision(boolean accessGranted) { } @Override - public RequestMutation preprocess(RequestDetailsReader requestDetailsReader) { + public RequestMutation getRequestMutation(RequestDetailsReader requestDetailsReader) { return null; } diff --git a/server/src/main/java/com/google/fhir/gateway/interfaces/RequestMutation.java b/server/src/main/java/com/google/fhir/gateway/interfaces/RequestMutation.java index 00481f8e..d1daaee6 100644 --- a/server/src/main/java/com/google/fhir/gateway/interfaces/RequestMutation.java +++ b/server/src/main/java/com/google/fhir/gateway/interfaces/RequestMutation.java @@ -11,6 +11,10 @@ @Getter public class RequestMutation { - // Additional query parameters that should be added to the outgoing FHIR request + // Additional query parameters and list of values for a parameter that should be added to the + // outgoing FHIR request. + // New values overwrites the old one if there is a conflict for a request param (i.e. a returned + // parameter in RequestMutation is already present in the original request). + // Old parameter values should be explicitly retained while mutating values for that parameter. @Builder.Default Map> queryParams = new HashMap<>(); } diff --git a/server/src/test/java/com/google/fhir/gateway/BearerAuthorizationInterceptorTest.java b/server/src/test/java/com/google/fhir/gateway/BearerAuthorizationInterceptorTest.java index c481fce3..db17209d 100644 --- a/server/src/test/java/com/google/fhir/gateway/BearerAuthorizationInterceptorTest.java +++ b/server/src/test/java/com/google/fhir/gateway/BearerAuthorizationInterceptorTest.java @@ -16,6 +16,7 @@ package com.google.fhir.gateway; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.arrayContainingInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.mockito.ArgumentMatchers.any; @@ -42,6 +43,7 @@ import com.google.fhir.gateway.interfaces.AccessDecision; import com.google.fhir.gateway.interfaces.NoOpAccessDecision; import com.google.fhir.gateway.interfaces.RequestDetailsReader; +import com.google.fhir.gateway.interfaces.RequestMutation; import com.google.gson.Gson; import java.io.IOException; import java.io.StringWriter; @@ -56,6 +58,8 @@ import java.security.interfaces.RSAPrivateKey; import java.security.interfaces.RSAPublicKey; import java.util.Base64; +import java.util.HashMap; +import java.util.List; import java.util.Map; import javax.servlet.http.HttpServletRequest; import org.apache.http.HttpResponse; @@ -352,4 +356,39 @@ public void deniedRequest() throws IOException { testInstance.authorizeRequest(requestMock); } + + @Test + public void mutateRequest() { + ServletRequestDetails requestDetails = new ServletRequestDetails(); + requestDetails.addParameter("param1", new String[] {"param1-value1"}); + requestDetails.addParameter("param2", new String[] {"param2-value1"}); + + HashMap> paramMutations = new HashMap<>(); + paramMutations.put("param1", List.of("param1-value2")); + paramMutations.put("param3", List.of("param3-value1", "param3-value2")); + AccessDecision mutableAccessDecision = + new AccessDecision() { + public boolean canAccess() { + return true; + } + + public RequestMutation getRequestMutation(RequestDetailsReader requestDetailsReader) { + return RequestMutation.builder().queryParams(paramMutations).build(); + } + + public String postProcess(HttpResponse response) throws IOException { + return null; + } + }; + + BearerAuthorizationInterceptor.mutateRequest(requestDetails, mutableAccessDecision); + + assertThat( + requestDetails.getParameters().get("param1"), arrayContainingInAnyOrder("param1-value2")); + assertThat( + requestDetails.getParameters().get("param2"), arrayContainingInAnyOrder("param2-value1")); + assertThat( + requestDetails.getParameters().get("param3"), + arrayContainingInAnyOrder("param3-value2", "param3-value1")); + } }