From 427bdf330e92fd56686752919bca5e11ee873092 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Fri, 17 Jun 2022 15:51:47 -0400 Subject: [PATCH] fix 403 on updates when version submitted without rewrite header (#3716) * fix 403 on updates when version submitted without rewrite header * change log Co-authored-by: Ken Stevens --- .../6_1_0/3716-fix-history-rewrite.yaml | 5 +++ .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 2 +- .../FhirResourceDaoR4HistoryRewriteTest.java | 14 +++--- .../fhir/rest/api/server/RequestDetails.java | 8 ++++ .../server/interceptor/auth/RuleImplOp.java | 2 +- .../server/servlet/ServletRequestDetails.java | 22 +++++++--- .../servlet/ServletRequestDetailsTest.java | 44 +++++++++++++++++++ 7 files changed, 83 insertions(+), 14 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3716-fix-history-rewrite.yaml create mode 100644 hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetailsTest.java diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3716-fix-history-rewrite.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3716-fix-history-rewrite.yaml new file mode 100644 index 00000000000..366f38a3a3b --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3716-fix-history-rewrite.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 3716 +title: "Server was blocking updates with Forbidden 403 that included a resource version in the url without +the X-Rewrite-History header. This has been corrected." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java index fdbf9d8c8c8..00a657198a2 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java @@ -1712,7 +1712,7 @@ public abstract class BaseHapiFhirResourceDao extends B Runnable onRollback = () -> theResource.getIdElement().setValue(id); // Execute the update in a retryable transaction - if (myDaoConfig.isUpdateWithHistoryRewriteEnabled() && "true".equals(theRequest.getHeader(JpaConstants.HEADER_REWRITE_HISTORY))) { + if (myDaoConfig.isUpdateWithHistoryRewriteEnabled() && theRequest.isRewriteHistory()) { return myTransactionService.execute(theRequest, theTransactionDetails, tx -> doUpdateWithHistoryRewrite(theResource, theRequest, theTransactionDetails), onRollback); } else { return myTransactionService.execute(theRequest, theTransactionDetails, tx -> doUpdate(theResource, theMatchUrl, thePerformIndexing, theForceUpdateVersion, theRequest, theTransactionDetails), onRollback); diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4HistoryRewriteTest.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4HistoryRewriteTest.java index 60b985043eb..dbd0812885f 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4HistoryRewriteTest.java +++ b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4HistoryRewriteTest.java @@ -1,7 +1,7 @@ package ca.uhn.fhir.jpa.dao.r4; -import ca.uhn.fhir.jpa.model.util.JpaConstants; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; +import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException; @@ -42,7 +42,7 @@ public class FhirResourceDaoR4HistoryRewriteTest extends BaseJpaR4Test { @AfterEach public void tearDown() { myDaoConfig.setUpdateWithHistoryRewriteEnabled(false); - when(mySrd.getHeader(eq(JpaConstants.HEADER_REWRITE_HISTORY))).thenReturn(""); + when(mySrd.getHeader(eq(Constants.HEADER_REWRITE_HISTORY))).thenReturn(""); } @Test @@ -54,7 +54,7 @@ public class FhirResourceDaoR4HistoryRewriteTest extends BaseJpaR4Test { IIdType id = createPatientWithHistory(); // execute updates - when(mySrd.getHeader(eq(JpaConstants.HEADER_REWRITE_HISTORY))).thenReturn("true"); + when(mySrd.isRewriteHistory()).thenReturn(true); Patient p = new Patient(); p.addIdentifier().setSystem("urn:system").setValue(TEST_SYSTEM_NAME); @@ -100,7 +100,7 @@ public class FhirResourceDaoR4HistoryRewriteTest extends BaseJpaR4Test { int resourceVersionsSizeInit = myResourceHistoryTableDao.findAll().size(); // execute update - when(mySrd.getHeader(eq(JpaConstants.HEADER_REWRITE_HISTORY))).thenReturn("true"); + when(mySrd.isRewriteHistory()).thenReturn(true); Patient p = new Patient(); p.addIdentifier().setSystem("urn:system").setValue(TEST_SYSTEM_NAME); @@ -151,7 +151,7 @@ public class FhirResourceDaoR4HistoryRewriteTest extends BaseJpaR4Test { IIdType id = createPatientWithHistory(); // execute update - when(mySrd.getHeader(eq(JpaConstants.HEADER_REWRITE_HISTORY))).thenReturn("true"); + when(mySrd.isRewriteHistory()).thenReturn(true); Patient p = new Patient(); p.addIdentifier().setSystem("urn:system").setValue(TEST_SYSTEM_NAME); @@ -174,7 +174,7 @@ public class FhirResourceDaoR4HistoryRewriteTest extends BaseJpaR4Test { IIdType id = createPatientWithHistory(); // execute update - when(mySrd.getHeader(eq(JpaConstants.HEADER_REWRITE_HISTORY))).thenReturn("true"); + when(mySrd.isRewriteHistory()).thenReturn(true); Patient p = new Patient(); p.addIdentifier().setSystem("urn:system").setValue(TEST_SYSTEM_NAME); @@ -197,7 +197,7 @@ public class FhirResourceDaoR4HistoryRewriteTest extends BaseJpaR4Test { IIdType id = createPatientWithHistory(); // execute update - when(mySrd.getHeader(eq(JpaConstants.HEADER_REWRITE_HISTORY))).thenReturn("true"); + when(mySrd.isRewriteHistory()).thenReturn(true); Patient p = new Patient(); p.addIdentifier().setSystem("urn:system").setValue(TEST_SYSTEM_NAME); diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/RequestDetails.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/RequestDetails.java index c665c466681..0878b8f4cef 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/RequestDetails.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/RequestDetails.java @@ -76,6 +76,7 @@ public abstract class RequestDetails { private String myRequestId; private String myTransactionGuid; private String myFixedConditionalUrl; + private boolean myRewriteHistory; /** * Constructor @@ -534,4 +535,11 @@ public abstract class RequestDetails { } + public boolean isRewriteHistory() { + return myRewriteHistory; + } + + public void setRewriteHistory(boolean theRewriteHistory) { + myRewriteHistory = theRewriteHistory; + } } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java index f1983016cf6..68e29470381 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java @@ -175,7 +175,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { if (theInputResource == null && theInputResourceId == null) { return null; } - if (theRequestDetails.getId() != null && theRequestDetails.getId().hasVersionIdPart() && theOperation == RestOperationTypeEnum.UPDATE) { + if (theRequestDetails.isRewriteHistory() && theRequestDetails.getId() != null && theRequestDetails.getId().hasVersionIdPart() && theOperation == RestOperationTypeEnum.UPDATE) { return null; } switch (theOperation) { diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java index 175ec21e511..8115a6791d2 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetails.java @@ -20,8 +20,8 @@ package ca.uhn.fhir.rest.server.servlet; * #L% */ -import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.server.RequestDetails; @@ -30,6 +30,7 @@ import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.Validate; +import javax.annotation.Nonnull; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.ByteArrayInputStream; @@ -37,7 +38,12 @@ import java.io.IOException; import java.io.InputStream; import java.io.Reader; import java.nio.charset.Charset; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.zip.GZIPInputStream; import static org.apache.commons.lang3.StringUtils.isNotBlank; @@ -54,7 +60,7 @@ public class ServletRequestDetails extends RequestDetails { * Constructor for testing only */ public ServletRequestDetails() { - this((IInterceptorBroadcaster)null); + this((IInterceptorBroadcaster) null); } /** @@ -171,8 +177,13 @@ public class ServletRequestDetails extends RequestDetails { this.myServer = theServer; } - public ServletRequestDetails setServletRequest(HttpServletRequest myServletRequest) { + public ServletRequestDetails setServletRequest(@Nonnull HttpServletRequest myServletRequest) { this.myServletRequest = myServletRequest; + + // TODO KHS move a bunch of other initialization from RestfulServer into this method + if ("true".equals(myServletRequest.getHeader(Constants.HEADER_REWRITE_HISTORY))) { + setRewriteHistory(true); + } return this; } @@ -180,7 +191,7 @@ public class ServletRequestDetails extends RequestDetails { this.myServletResponse = myServletResponse; } - public Map> getHeaders() { + public Map> getHeaders() { Map> retVal = new HashMap<>(); Enumeration names = myServletRequest.getHeaderNames(); while (names.hasMoreElements()) { @@ -194,4 +205,5 @@ public class ServletRequestDetails extends RequestDetails { } return Collections.unmodifiableMap(retVal); } + } diff --git a/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetailsTest.java b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetailsTest.java new file mode 100644 index 00000000000..799a7f77482 --- /dev/null +++ b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/servlet/ServletRequestDetailsTest.java @@ -0,0 +1,44 @@ +package ca.uhn.fhir.rest.server.servlet; + +import ca.uhn.fhir.rest.api.Constants; +import org.junit.jupiter.api.Test; + +import javax.servlet.http.HttpServletRequest; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class ServletRequestDetailsTest { + @Test + public void testRewriteHistoryHeader() { + ServletRequestDetails servletRequestDetails = new ServletRequestDetails(); + HttpServletRequest httpRequest = mock(HttpServletRequest.class); + servletRequestDetails.setServletRequest(httpRequest); + when(httpRequest.getHeader(Constants.HEADER_REWRITE_HISTORY)).thenReturn("true"); + servletRequestDetails.setServletRequest(httpRequest); + assertTrue(servletRequestDetails.isRewriteHistory()); + } + + @Test + public void testRewriteHistoryHeaderNull() { + ServletRequestDetails servletRequestDetails = new ServletRequestDetails(); + HttpServletRequest httpRequest = mock(HttpServletRequest.class); + servletRequestDetails.setServletRequest(httpRequest); + when(httpRequest.getHeader(Constants.HEADER_REWRITE_HISTORY)).thenReturn(null); + servletRequestDetails.setServletRequest(httpRequest); + assertFalse(servletRequestDetails.isRewriteHistory()); + } + + @Test + public void testRewriteHistoryHeaderFalse() { + ServletRequestDetails servletRequestDetails = new ServletRequestDetails(); + HttpServletRequest httpRequest = mock(HttpServletRequest.class); + servletRequestDetails.setServletRequest(httpRequest); + when(httpRequest.getHeader(Constants.HEADER_REWRITE_HISTORY)).thenReturn("false"); + servletRequestDetails.setServletRequest(httpRequest); + assertFalse(servletRequestDetails.isRewriteHistory()); + } + +}