From ee314f5cf0389446cebe7ed46d88f6caa1069a60 Mon Sep 17 00:00:00 2001 From: Vivek Mittal Date: Wed, 5 Apr 2023 15:57:39 +0530 Subject: [PATCH 1/3] Define access decision preprocess stage to mutate the request params --- .../plugin/AccessGrantedAndUpdateList.java | 7 +++++++ .../BearerAuthorizationInterceptor.java | 20 +++++++++++++++++++ .../fhir/gateway/CapabilityPostProcessor.java | 7 +++++++ .../gateway/interfaces/AccessDecision.java | 12 ++++++++++- .../interfaces/NoOpAccessDecision.java | 5 +++++ .../gateway/interfaces/RequestMutation.java | 16 +++++++++++++++ 6 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 server/src/main/java/com/google/fhir/gateway/interfaces/RequestMutation.java 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 46d87ba4..7ea988ed 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 @@ -26,6 +26,8 @@ import com.google.fhir.gateway.HttpFhirClient; import com.google.fhir.gateway.HttpUtil; import com.google.fhir.gateway.interfaces.AccessDecision; +import com.google.fhir.gateway.interfaces.RequestDetailsReader; +import com.google.fhir.gateway.interfaces.RequestMutation; import java.io.IOException; import java.util.Set; import org.apache.http.HttpResponse; @@ -62,6 +64,11 @@ private AccessGrantedAndUpdateList( this.resourceTypeExpected = resourceTypeExpected; } + @Override + public RequestMutation preprocess(RequestDetailsReader requestDetailsReader) { + return null; + } + @Override public boolean canAccess() { return true; 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 fb847aa6..aa6dfa9a 100644 --- a/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java +++ b/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java @@ -39,6 +39,7 @@ import com.google.fhir.gateway.interfaces.AccessCheckerFactory; import com.google.fhir.gateway.interfaces.AccessDecision; import com.google.fhir.gateway.interfaces.RequestDetailsReader; +import com.google.fhir.gateway.interfaces.RequestMutation; import com.google.gson.JsonObject; import com.google.gson.JsonParser; import java.io.IOException; @@ -62,6 +63,7 @@ import org.apache.http.util.EntityUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.util.CollectionUtils; @Interceptor public class BearerAuthorizationInterceptor { @@ -270,6 +272,7 @@ public boolean authorizeRequest(RequestDetails requestDetails) { return false; } AccessDecision outcome = checkAuthorization(requestDetails); + preprocessRequest(servletDetails, outcome); logger.debug("Authorized request path " + requestPath); try { HttpResponse response = fhirClient.handleRequest(servletDetails); @@ -390,4 +393,21 @@ private void serveWellKnown(ServletRequestDetails request) { ExceptionUtil.throwRuntimeExceptionAndLog(logger, e.getMessage(), e); } } + + private void preprocessRequest( + ServletRequestDetails servletRequestDetails, AccessDecision accessDecision) { + RequestMutation mutation = + accessDecision.preprocess(new RequestDetailsToReader(servletRequestDetails)); + if (mutation == null || CollectionUtils.isEmpty(mutation.getQueryParams())) { + return ; + } + + mutation + .getQueryParams() + .forEach((key, value) -> servletRequestDetails.addParameter( + key, value.toArray(new String[0]))); + + // TODO update the query params in search by Post + + } } 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 1ebd7c23..73d2da0e 100644 --- a/server/src/main/java/com/google/fhir/gateway/CapabilityPostProcessor.java +++ b/server/src/main/java/com/google/fhir/gateway/CapabilityPostProcessor.java @@ -20,6 +20,8 @@ import com.google.common.base.Preconditions; import com.google.common.io.CharStreams; import com.google.fhir.gateway.interfaces.AccessDecision; +import com.google.fhir.gateway.interfaces.RequestDetailsReader; +import com.google.fhir.gateway.interfaces.RequestMutation; import java.io.IOException; import org.apache.http.HttpResponse; import org.hl7.fhir.instance.model.api.IBaseResource; @@ -52,6 +54,11 @@ static synchronized CapabilityPostProcessor getInstance(FhirContext fhirContext) return instance; } + @Override + public RequestMutation preprocess(RequestDetailsReader requestDetailsReader) { + return null; + } + @Override public boolean canAccess() { return true; 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 c32f00bf..83f586c7 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 @@ -20,7 +20,17 @@ public interface AccessDecision { - /** @return true iff access was granted. */ + /** + * Allows the incoming request mutation based on the access decision. + * + * @param requestDetailsReader details about the resource and operation requested + * @return the mutation to be applied on the incoming request + */ + RequestMutation preprocess(RequestDetailsReader requestDetailsReader); + + /** + * @return true iff access was granted. + */ boolean canAccess(); /** 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 84e65a4a..b9d609b3 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 @@ -25,6 +25,11 @@ public NoOpAccessDecision(boolean accessGranted) { this.accessGranted = accessGranted; } + @Override + public RequestMutation preprocess(RequestDetailsReader requestDetailsReader) { + return null; + } + @Override public boolean canAccess() { return accessGranted; 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 new file mode 100644 index 00000000..00481f8e --- /dev/null +++ b/server/src/main/java/com/google/fhir/gateway/interfaces/RequestMutation.java @@ -0,0 +1,16 @@ +package com.google.fhir.gateway.interfaces; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import lombok.Builder; +import lombok.Getter; + +/** Defines mutations that can be applied to the incoming request by an {@link AccessChecker}. */ +@Builder +@Getter +public class RequestMutation { + + // Additional query parameters that should be added to the outgoing FHIR request + @Builder.Default Map> queryParams = new HashMap<>(); +} From 31de3f4599f9edcfeab995ce8074583264a15457 Mon Sep 17 00:00:00 2001 From: Vivek Mittal Date: Thu, 13 Apr 2023 17:51:12 +0530 Subject: [PATCH 2/3] 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")); + } } From 8d4d5166914f716b27ab0fc8920ca1d3221389a9 Mon Sep 17 00:00:00 2001 From: Vivek Mittal Date: Tue, 18 Apr 2023 11:50:25 +0530 Subject: [PATCH 3/3] Removed static identifier from request mutation method --- .../com/google/fhir/gateway/BearerAuthorizationInterceptor.java | 2 +- .../google/fhir/gateway/BearerAuthorizationInterceptorTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 42bb010c..4aebd203 100644 --- a/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java +++ b/server/src/main/java/com/google/fhir/gateway/BearerAuthorizationInterceptor.java @@ -395,7 +395,7 @@ private void serveWellKnown(ServletRequestDetails request) { } @VisibleForTesting - static void mutateRequest(RequestDetails requestDetails, AccessDecision accessDecision) { + void mutateRequest(RequestDetails requestDetails, AccessDecision accessDecision) { RequestMutation mutation = accessDecision.getRequestMutation(new RequestDetailsToReader(requestDetails)); if (mutation == null || CollectionUtils.isEmpty(mutation.getQueryParams())) { 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 db17209d..b08ecc11 100644 --- a/server/src/test/java/com/google/fhir/gateway/BearerAuthorizationInterceptorTest.java +++ b/server/src/test/java/com/google/fhir/gateway/BearerAuthorizationInterceptorTest.java @@ -381,7 +381,7 @@ public String postProcess(HttpResponse response) throws IOException { } }; - BearerAuthorizationInterceptor.mutateRequest(requestDetails, mutableAccessDecision); + testInstance.mutateRequest(requestDetails, mutableAccessDecision); assertThat( requestDetails.getParameters().get("param1"), arrayContainingInAnyOrder("param1-value2"));