From 95012623f632a56bb8a91e0ab10af6258652995b Mon Sep 17 00:00:00 2001 From: Nick Goupinets Date: Tue, 22 Jun 2021 15:12:10 -0400 Subject: [PATCH 1/3] Added null checks --- .../interceptor/MdmStorageInterceptor.java | 46 +++++++++++++++++-- .../ca/uhn/fhir/mdm/util/MdmResourceUtil.java | 6 +++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptor.java b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptor.java index 14dc07b62db..4ea7ae83ba4 100644 --- a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptor.java +++ b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptor.java @@ -43,12 +43,16 @@ import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Service; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; @Service public class MdmStorageInterceptor implements IMdmStorageInterceptor { + private static final Logger ourLog = LoggerFactory.getLogger(MdmStorageInterceptor.class); + @Autowired private ExpungeEverythingService myExpungeEverythingService; @Autowired @@ -63,13 +67,21 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor { @Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED) public void blockManualResourceManipulationOnCreate(IBaseResource theBaseResource, RequestDetails theRequestDetails, ServletRequestDetails theServletRequestDetails) { + ourLog.debug("Starting pre-storage resource created hook for {}, {}, {}", theBaseResource, theRequestDetails, theServletRequestDetails); + if (theBaseResource == null) { + ourLog.warn("Attempting to block golden resource manipulation on a null resource"); + return; + } + //If running in single EID mode, forbid multiple eids. if (myMdmSettings.isPreventMultipleEids()) { + ourLog.debug("Forbidding multiple EIDs on ", theBaseResource); forbidIfHasMultipleEids(theBaseResource); } // TODO GGG MDM find a better way to identify internal calls? if (isInternalRequest(theRequestDetails)) { + ourLog.debug("Internal request - completed processing"); return; } @@ -78,8 +90,16 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor { @Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED) public void blockManualGoldenResourceManipulationOnUpdate(IBaseResource theOldResource, IBaseResource theUpdatedResource, RequestDetails theRequestDetails, ServletRequestDetails theServletRequestDetails) { + ourLog.debug("Starting pre-storage resource updated hook for {}, {}, {}, {}", theOldResource, theUpdatedResource, theRequestDetails, theServletRequestDetails); + + if (theUpdatedResource == null) { + ourLog.warn("Attempting to block golden resource manipulation on a null resource"); + return; + } + //If running in single EID mode, forbid multiple eids. if (myMdmSettings.isPreventMultipleEids()) { + ourLog.debug("Forbidding multiple EIDs on ", theUpdatedResource); forbidIfHasMultipleEids(theUpdatedResource); } @@ -92,10 +112,16 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor { } if (isInternalRequest(theRequestDetails)) { + ourLog.debug("Internal request - completed processing"); return; } - forbidIfMdmManagedTagIsPresent(theOldResource); - forbidModifyingMdmTag(theUpdatedResource, theOldResource); + + if (theOldResource != null) { + forbidIfMdmManagedTagIsPresent(theOldResource); + forbidModifyingMdmTag(theUpdatedResource, theOldResource); + } else { + ourLog.warn("Null theOldResource for {} {}", theUpdatedResource == null ? "null updated resource" : theUpdatedResource.getIdElement(), theRequestDetails); + } if (myMdmSettings.isPreventEidUpdates()) { forbidIfModifyingExternalEidOnTarget(theUpdatedResource, theOldResource); @@ -111,8 +137,15 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor { } private void forbidIfModifyingExternalEidOnTarget(IBaseResource theNewResource, IBaseResource theOldResource) { - List newExternalEids = myEIDHelper.getExternalEid(theNewResource); - List oldExternalEids = myEIDHelper.getExternalEid(theOldResource); + List newExternalEids = Collections.emptyList(); + List oldExternalEids = Collections.emptyList(); + if (theNewResource != null) { + newExternalEids = myEIDHelper.getExternalEid(theNewResource); + } + if (theOldResource != null) { + oldExternalEids = myEIDHelper.getExternalEid(theOldResource); + } + if (oldExternalEids.isEmpty()) { return; } @@ -152,6 +185,11 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor { } private void forbidIfMdmManagedTagIsPresent(IBaseResource theResource) { + if (theResource == null) { + ourLog.warn("Attempting to forbid MDM on a null resource"); + return; + } + if (MdmResourceUtil.isMdmManaged(theResource)) { throwModificationBlockedByMdm(); } diff --git a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java index fbaf0557e51..1333103662c 100644 --- a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java +++ b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java @@ -70,10 +70,16 @@ public final class MdmResourceUtil { } private static boolean resourceHasTag(IBaseResource theTheBaseResource, String theSystem, String theCode) { + if (theTheBaseResource == null) { + return false; + } return theTheBaseResource.getMeta().getTag(theSystem, theCode) != null; } private static boolean resourceHasTagWithSystem(IBaseResource theTheBaseResource, String theSystem) { + if (theTheBaseResource == null) { + return false; + } return theTheBaseResource.getMeta().getTag().stream().anyMatch(tag -> tag.getSystem().equalsIgnoreCase(theSystem)); } From 5c9f2617dad7bd8df038b45e22d0cad900a2d904 Mon Sep 17 00:00:00 2001 From: Nick Goupinets Date: Tue, 22 Jun 2021 15:23:03 -0400 Subject: [PATCH 2/3] Code review --- .../java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java index 1333103662c..8b8717e1bee 100644 --- a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java +++ b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/util/MdmResourceUtil.java @@ -69,18 +69,18 @@ public final class MdmResourceUtil { return resourceHasTag(theBaseResource, MdmConstants.SYSTEM_GOLDEN_RECORD_STATUS, MdmConstants.CODE_GOLDEN_RECORD_REDIRECTED); } - private static boolean resourceHasTag(IBaseResource theTheBaseResource, String theSystem, String theCode) { - if (theTheBaseResource == null) { + private static boolean resourceHasTag(IBaseResource theBaseResource, String theSystem, String theCode) { + if (theBaseResource == null) { return false; } - return theTheBaseResource.getMeta().getTag(theSystem, theCode) != null; + return theBaseResource.getMeta().getTag(theSystem, theCode) != null; } - private static boolean resourceHasTagWithSystem(IBaseResource theTheBaseResource, String theSystem) { - if (theTheBaseResource == null) { + private static boolean resourceHasTagWithSystem(IBaseResource theBaseResource, String theSystem) { + if (theBaseResource == null) { return false; } - return theTheBaseResource.getMeta().getTag().stream().anyMatch(tag -> tag.getSystem().equalsIgnoreCase(theSystem)); + return theBaseResource.getMeta().getTag().stream().anyMatch(tag -> tag.getSystem().equalsIgnoreCase(theSystem)); } private static Optional getTagWithSystem(IBaseResource theResource, String theSystem) { From 9caa5e68fc8406807d41779788270da0763cfc40 Mon Sep 17 00:00:00 2001 From: Nick Goupinets Date: Tue, 22 Jun 2021 15:52:21 -0400 Subject: [PATCH 3/3] Added changelog --- .../5_5_0/2747-null-checks-on-mdm-manual-updates.yaml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2747-null-checks-on-mdm-manual-updates.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2747-null-checks-on-mdm-manual-updates.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2747-null-checks-on-mdm-manual-updates.yaml new file mode 100644 index 00000000000..193995be3e6 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2747-null-checks-on-mdm-manual-updates.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 2747 +title: "Added null checks to MDM resource interceptor in order to avoid NPEs."