From c2e5fa3f1869c045817581007e376cade720010d Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 9 Jun 2017 11:48:17 -0400 Subject: [PATCH] Fix #667 - When using the AuthorizationInterceptor with the JPA server, when a client is updating a resource from A to B, the user now needs to have write permission for both A and B --- .../method/IRequestOperationCallback.java | 10 +++ .../uhn/fhir/rest/method/RequestDetails.java | 19 +++++- .../IServerOperationInterceptor.java | 33 +++++++--- .../ServerOperationInterceptorAdapter.java | 9 +++ .../auth/AuthorizationInterceptor.java | 25 +++----- .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 4 ++ .../RestHookSubscriptionDstu2Interceptor.java | 15 +++-- .../RestHookSubscriptionDstu3Interceptor.java | 15 +++-- ...WebSocketSubscriptionDstu3Interceptor.java | 27 ++++---- .../FhirResourceDaoDstu2InterceptorTest.java | 12 ++-- .../FhirResourceDaoDstu3InterceptorTest.java | 13 +++- ...nInterceptorResourceProviderDstu3Test.java | 61 ++++++++++++++++++- .../rest/server/InterceptorDstu3Test.java | 3 +- src/changes/changes.xml | 19 ++++++ 14 files changed, 194 insertions(+), 71 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/IRequestOperationCallback.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/IRequestOperationCallback.java index 3c9b9bbbd4b..e85026cae9a 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/IRequestOperationCallback.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/IRequestOperationCallback.java @@ -34,7 +34,17 @@ public interface IRequestOperationCallback { void resourcesDeleted(Collection theResource); + /** + * @deprecated Deprecated in HAPI FHIR 2.6 - Use {@link IRequestOperationCallback#resourceUpdated(IBaseResource, IBaseResource)} instead + */ + @Deprecated void resourcesUpdated(Collection theResource); + /** + * @deprecated Deprecated in HAPI FHIR 2.6 - Use {@link IRequestOperationCallback#resourceUpdated(IBaseResource, IBaseResource)} instead + */ + @Deprecated void resourceUpdated(IBaseResource theResource); + + void resourceUpdated(IBaseResource theOldResource, IBaseResource theNewResource); } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/RequestDetails.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/RequestDetails.java index 3ccd5fbf2d3..3a4172e8b59 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/RequestDetails.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/RequestDetails.java @@ -400,13 +400,21 @@ public abstract class RequestDetails { } } - @Override + /** + * @deprecated Deprecated in HAPI FHIR 2.6 - Use {@link IRequestOperationCallback#resourceUpdated(IBaseResource, IBaseResource)} instead + */ + @Deprecated public void resourcesUpdated(Collection theResource) { for (IBaseResource next : theResource) { resourceUpdated(next); } } + + /** + * @deprecated Deprecated in HAPI FHIR 2.6 - Use {@link IRequestOperationCallback#resourceUpdated(IBaseResource, IBaseResource)} instead + */ + @Deprecated @Override public void resourceUpdated(IBaseResource theResource) { for (IServerInterceptor next : getInterceptors()) { @@ -416,6 +424,15 @@ public abstract class RequestDetails { } } + @Override + public void resourceUpdated(IBaseResource theOldResource, IBaseResource theNewResource) { + for (IServerInterceptor next : getInterceptors()) { + if (next instanceof IServerOperationInterceptor) { + ((IServerOperationInterceptor) next).resourceUpdated(RequestDetails.this, theOldResource, theNewResource); + } + } + } + } } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/IServerOperationInterceptor.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/IServerOperationInterceptor.java index 8ad2ee51ca9..0593b02f447 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/IServerOperationInterceptor.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/IServerOperationInterceptor.java @@ -10,7 +10,7 @@ package ca.uhn.fhir.rest.server.interceptor; * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, @@ -25,7 +25,7 @@ import org.hl7.fhir.instance.model.api.IBaseResource; import ca.uhn.fhir.rest.method.RequestDetails; /** - * Server interceptor with added methods which can be called within the lifecycle of + * Server interceptor with added methods which can be called within the lifecycle of * write operations (create/update/delete) or within transaction and batch * operations that call these sub-operations. * @@ -33,12 +33,6 @@ import ca.uhn.fhir.rest.method.RequestDetails; */ public interface IServerOperationInterceptor extends IServerInterceptor { - /** - * User code may call this method to indicate to an interceptor that - * a resource is being deleted - */ - void resourceDeleted(RequestDetails theRequest, IBaseResource theResource); - /** * User code may call this method to indicate to an interceptor that * a resource is being created @@ -47,8 +41,29 @@ public interface IServerOperationInterceptor extends IServerInterceptor { /** * User code may call this method to indicate to an interceptor that - * a resource is being updated + * a resource is being deleted */ + void resourceDeleted(RequestDetails theRequest, IBaseResource theResource); + + /** + * User code may call this method to indicate to an interceptor that + * a resource is being updated + * + * @deprecated Deprecated in HAPI FHIR 2.6 in favour of {@link #resourceUpdated(RequestDetails, IBaseResource, IBaseResource)} + */ + @Deprecated void resourceUpdated(RequestDetails theRequest, IBaseResource theResource); + /** + * User code may call this method to indicate to an interceptor that + * a resource is being updated + * + * @param theOldResource + * The resource as it was before the update, or null if this is not available. Interceptors should be able to handle situations where this is null, since it is not always + * convenient or possible to provide a value for this field, but servers should try to populate it. + * @param theNewResource + * The resource as it will be after the update + */ + void resourceUpdated(RequestDetails theRequest, IBaseResource theOldResource, IBaseResource theNewResource); + } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ServerOperationInterceptorAdapter.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ServerOperationInterceptorAdapter.java index fed13c9876f..28d6bf9a484 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ServerOperationInterceptorAdapter.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ServerOperationInterceptorAdapter.java @@ -39,9 +39,18 @@ public class ServerOperationInterceptorAdapter extends InterceptorAdapter implem // nothing } + /** + * @deprecated Deprecated in HAPI FHIR 2.6 in favour of {@link #resourceUpdated(RequestDetails, IBaseResource, IBaseResource)} + */ + @Deprecated @Override public void resourceUpdated(RequestDetails theRequest, IBaseResource theResource) { // nothing } + @Override + public void resourceUpdated(RequestDetails theRequest, IBaseResource theOldResource, IBaseResource theNewResource) { + // nothing + } + } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java index c1b775947c4..9a0d5dcadaf 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java @@ -44,8 +44,7 @@ import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.method.RequestDetails; import ca.uhn.fhir.rest.server.exceptions.AuthenticationException; import ca.uhn.fhir.rest.server.exceptions.ForbiddenOperationException; -import ca.uhn.fhir.rest.server.interceptor.IServerOperationInterceptor; -import ca.uhn.fhir.rest.server.interceptor.InterceptorAdapter; +import ca.uhn.fhir.rest.server.interceptor.*; import ca.uhn.fhir.util.CoverageIgnore; /** @@ -58,7 +57,7 @@ import ca.uhn.fhir.util.CoverageIgnore; * for information on how to use this interceptor. *

*/ -public class AuthorizationInterceptor extends InterceptorAdapter implements IServerOperationInterceptor, IRuleApplier { +public class AuthorizationInterceptor extends ServerOperationInterceptorAdapter implements IRuleApplier { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(AuthorizationInterceptor.class); @@ -330,8 +329,11 @@ public class AuthorizationInterceptor extends InterceptorAdapter implements ISer } @Override - public void resourceUpdated(RequestDetails theRequest, IBaseResource theResource) { - handleUserOperation(theRequest, theResource, RestOperationTypeEnum.UPDATE); + public void resourceUpdated(RequestDetails theRequest, IBaseResource theOldResource, IBaseResource theNewResource) { + if (theOldResource != null) { + handleUserOperation(theRequest, theOldResource, RestOperationTypeEnum.UPDATE); + } + handleUserOperation(theRequest, theNewResource, RestOperationTypeEnum.UPDATE); } /** @@ -357,19 +359,6 @@ public class AuthorizationInterceptor extends InterceptorAdapter implements ISer return resources; } - // private List toListOfResources(FhirContext fhirContext, IBaseBundle responseBundle) { - // List retVal = BundleUtil.toListOfResources(fhirContext, responseBundle); - // for (int i = 0; i < retVal.size(); i++) { - // IBaseResource nextResource = retVal.get(i); - // if (nextResource instanceof IBaseBundle) { - // retVal.addAll(BundleUtil.toListOfResources(fhirContext, (IBaseBundle) nextResource)); - // retVal.remove(i); - // i--; - // } - // } - // return retVal; - // } - private static UnsupportedOperationException failForDstu1() { return new UnsupportedOperationException("Use of this interceptor on DSTU1 servers is not supportd"); } 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 edc3cc0565f..b4017a47416 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 @@ -1125,6 +1125,8 @@ public abstract class BaseHapiFhirResourceDao extends B notifyInterceptors(RestOperationTypeEnum.UPDATE, requestDetails); } + IBaseResource oldResource = toResource(entity, false); + // Perform update ResourceTable savedEntity = updateEntity(theResource, entity, null, thePerformIndexing, thePerformIndexing, new Date(), theForceUpdateVersion, thePerformIndexing); @@ -1143,6 +1145,7 @@ public abstract class BaseHapiFhirResourceDao extends B // Notify interceptors if (theRequestDetails != null) { theRequestDetails.getRequestOperationCallback().resourceUpdated(theResource); + theRequestDetails.getRequestOperationCallback().resourceUpdated(oldResource, theResource); for (IServerInterceptor next : getConfig().getInterceptors()) { if (next instanceof IJpaServerInterceptor) { ((IJpaServerInterceptor) next).resourceUpdated(requestDetails, entity); @@ -1152,6 +1155,7 @@ public abstract class BaseHapiFhirResourceDao extends B for (IServerInterceptor next : getConfig().getInterceptors()) { if (next instanceof IServerOperationInterceptor) { ((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, theResource); + ((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, oldResource, theResource); } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java index 3c1daa3ef8e..ea4f73bfc38 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java @@ -59,10 +59,9 @@ import ca.uhn.fhir.rest.param.TokenParam; import ca.uhn.fhir.rest.server.Constants; import ca.uhn.fhir.rest.server.EncodingEnum; import ca.uhn.fhir.rest.server.IBundleProvider; -import ca.uhn.fhir.rest.server.interceptor.IServerOperationInterceptor; -import ca.uhn.fhir.rest.server.interceptor.InterceptorAdapter; +import ca.uhn.fhir.rest.server.interceptor.*; -public class RestHookSubscriptionDstu2Interceptor extends InterceptorAdapter implements IServerOperationInterceptor { +public class RestHookSubscriptionDstu2Interceptor extends ServerOperationInterceptorAdapter { private static volatile ExecutorService executor; private final static int MAX_THREADS = 1; @@ -367,14 +366,14 @@ public class RestHookSubscriptionDstu2Interceptor extends InterceptorAdapter imp * a rest-hook subscription */ @Override - public void resourceUpdated(RequestDetails theRequest, IBaseResource theResource) { - String resourceType = getResourceName(theResource); - IIdType idType = theResource.getIdElement(); + public void resourceUpdated(RequestDetails theRequest, IBaseResource theOldResource, IBaseResource theNewResource) { + String resourceType = getResourceName(theNewResource); + IIdType idType = theNewResource.getIdElement(); ourLog.info("resource updated type: " + resourceType); - if (theResource instanceof Subscription) { - Subscription subscription = (Subscription) theResource; + if (theNewResource instanceof Subscription) { + Subscription subscription = (Subscription) theNewResource; if (subscription.getChannel() != null && subscription.getChannel().getTypeElement().getValueAsEnum() == SubscriptionChannelTypeEnum.REST_HOOK) { removeLocalSubscription(subscription.getIdElement().getIdPart()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java index 986e65e49c4..449a7535802 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java @@ -57,10 +57,9 @@ import ca.uhn.fhir.rest.param.TokenParam; import ca.uhn.fhir.rest.server.Constants; import ca.uhn.fhir.rest.server.EncodingEnum; import ca.uhn.fhir.rest.server.IBundleProvider; -import ca.uhn.fhir.rest.server.interceptor.IServerOperationInterceptor; -import ca.uhn.fhir.rest.server.interceptor.InterceptorAdapter; +import ca.uhn.fhir.rest.server.interceptor.*; -public class RestHookSubscriptionDstu3Interceptor extends InterceptorAdapter implements IServerOperationInterceptor { +public class RestHookSubscriptionDstu3Interceptor extends ServerOperationInterceptorAdapter { private static volatile ExecutorService executor; private final static int MAX_THREADS = 1; @@ -368,14 +367,14 @@ public class RestHookSubscriptionDstu3Interceptor extends InterceptorAdapter imp * a rest-hook subscription */ @Override - public void resourceUpdated(RequestDetails theRequest, IBaseResource theResource) { - String resourceType = getResourceName(theResource); - IIdType idType = theResource.getIdElement(); + public void resourceUpdated(RequestDetails theRequest, IBaseResource theOldResource, IBaseResource theNewResource) { + String resourceType = getResourceName(theNewResource); + IIdType idType = theNewResource.getIdElement(); ourLog.info("resource updated type: " + resourceType); - if (theResource instanceof Subscription) { - Subscription subscription = (Subscription) theResource; + if (theNewResource instanceof Subscription) { + Subscription subscription = (Subscription) theNewResource; if (subscription.getChannel() != null && subscription.getChannel().getType() == Subscription.SubscriptionChannelType.RESTHOOK) { removeLocalSubscription(subscription.getIdElement().getIdPart()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/WebSocketSubscriptionDstu3Interceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/WebSocketSubscriptionDstu3Interceptor.java index 088efe96878..1963e54ba08 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/WebSocketSubscriptionDstu3Interceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/WebSocketSubscriptionDstu3Interceptor.java @@ -1,6 +1,15 @@ package ca.uhn.fhir.jpa.interceptor; +import javax.annotation.PostConstruct; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.hl7.fhir.dstu3.model.Subscription; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; + /*- * #%L * HAPI FHIR JPA Server @@ -23,25 +32,13 @@ package ca.uhn.fhir.jpa.interceptor; import ca.uhn.fhir.jpa.dao.IFhirResourceDao; import ca.uhn.fhir.jpa.dao.IFhirResourceDaoSubscription; -import ca.uhn.fhir.jpa.entity.ResourceTable; import ca.uhn.fhir.rest.api.RequestTypeEnum; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.method.RequestDetails; import ca.uhn.fhir.rest.server.exceptions.AuthenticationException; -import ca.uhn.fhir.rest.server.interceptor.IServerOperationInterceptor; -import ca.uhn.fhir.rest.server.interceptor.InterceptorAdapter; -import org.hl7.fhir.dstu3.model.Subscription; -import org.hl7.fhir.instance.model.api.IBaseResource; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Qualifier; +import ca.uhn.fhir.rest.server.interceptor.ServerOperationInterceptorAdapter; -import javax.annotation.PostConstruct; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - -public class WebSocketSubscriptionDstu3Interceptor extends InterceptorAdapter implements IServerOperationInterceptor { +public class WebSocketSubscriptionDstu3Interceptor extends ServerOperationInterceptorAdapter { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(WebSocketSubscriptionDstu3Interceptor.class); @@ -104,7 +101,7 @@ public class WebSocketSubscriptionDstu3Interceptor extends InterceptorAdapter im } @Override - public void resourceUpdated(RequestDetails theRequest, IBaseResource theResource) { + public void resourceUpdated(RequestDetails theRequest, IBaseResource theOldResource, IBaseResource theNewResource) { // nothing } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2InterceptorTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2InterceptorTest.java index c53bf566f4d..8badff2c2e3 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2InterceptorTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2InterceptorTest.java @@ -91,7 +91,7 @@ public class FhirResourceDaoDstu2InterceptorTest extends BaseJpaDstu2Test { tableCapt = ArgumentCaptor.forClass(ResourceTable.class); verify(myJpaInterceptor, times(1)).resourceCreated(detailsCapt.capture(), tableCapt.capture()); verify(myJpaInterceptor, times(0)).resourceUpdated(detailsCapt.capture(), tableCapt.capture()); - + } @Test @@ -366,10 +366,10 @@ public class FhirResourceDaoDstu2InterceptorTest extends BaseJpaDstu2Test { doAnswer(new Answer() { @Override public Void answer(InvocationOnMock theInvocation) throws Throwable { - IBaseResource res = (IBaseResource) theInvocation.getArguments()[0]; + IBaseResource res = (IBaseResource) theInvocation.getArguments()[1]; assertEquals("Patient/" + id + "/_history/2", res.getIdElement().getValue()); return null; - }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class)); + }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); Bundle xactBundle = new Bundle(); xactBundle.setType(BundleTypeEnum.TRANSACTION); @@ -385,6 +385,7 @@ public class FhirResourceDaoDstu2InterceptorTest extends BaseJpaDstu2Test { assertEquals(2L, newId.getVersionIdPartAsLong().longValue()); verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class)); + verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); verify(myRequestOperationCallback, times(1)).resourceCreated(any(IBaseResource.class)); verifyNoMoreInteractions(myRequestOperationCallback); } @@ -398,10 +399,10 @@ public class FhirResourceDaoDstu2InterceptorTest extends BaseJpaDstu2Test { doAnswer(new Answer() { @Override public Void answer(InvocationOnMock theInvocation) throws Throwable { - IBaseResource res = (IBaseResource) theInvocation.getArguments()[0]; + IBaseResource res = (IBaseResource) theInvocation.getArguments()[1]; assertEquals("Patient/" + id + "/_history/2", res.getIdElement().getValue()); return null; - }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class)); + }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); p = new Patient(); p.setId(new IdDt("Patient/" + id)); @@ -410,6 +411,7 @@ public class FhirResourceDaoDstu2InterceptorTest extends BaseJpaDstu2Test { assertEquals(2L, newId.getVersionIdPartAsLong().longValue()); verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class)); + verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); verify(myRequestOperationCallback, times(1)).resourceCreated(any(IBaseResource.class)); verifyNoMoreInteractions(myRequestOperationCallback); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InterceptorTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InterceptorTest.java index 11b244456e5..5ad60064d7a 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InterceptorTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InterceptorTest.java @@ -226,10 +226,12 @@ public class FhirResourceDaoDstu3InterceptorTest extends BaseJpaDstu3Test { verify(myServerOperationInterceptor, times(1)).resourceCreated(Mockito.isNull(RequestDetails.class), any(IBaseResource.class)); } + @SuppressWarnings("deprecation") @Test public void testServerOperationUpdate() { verify(myServerOperationInterceptor, times(0)).resourceCreated(Mockito.isNull(RequestDetails.class), any(IBaseResource.class)); verify(myServerOperationInterceptor, times(0)).resourceUpdated(Mockito.isNull(RequestDetails.class), any(IBaseResource.class)); + verify(myServerOperationInterceptor, times(0)).resourceUpdated(Mockito.isNull(RequestDetails.class), any(IBaseResource.class), any(IBaseResource.class)); Patient p = new Patient(); p.addName().setFamily("PATIENT"); @@ -241,6 +243,7 @@ public class FhirResourceDaoDstu3InterceptorTest extends BaseJpaDstu3Test { verify(myServerOperationInterceptor, times(1)).resourceCreated(Mockito.isNull(RequestDetails.class), any(IBaseResource.class)); verify(myServerOperationInterceptor, times(1)).resourceUpdated(Mockito.isNull(RequestDetails.class), any(IBaseResource.class)); + verify(myServerOperationInterceptor, times(1)).resourceUpdated(Mockito.isNull(RequestDetails.class), any(IBaseResource.class), any(IBaseResource.class)); } @Test @@ -429,10 +432,10 @@ public class FhirResourceDaoDstu3InterceptorTest extends BaseJpaDstu3Test { doAnswer(new Answer() { @Override public Void answer(InvocationOnMock theInvocation) throws Throwable { - IBaseResource res = (IBaseResource) theInvocation.getArguments()[0]; + IBaseResource res = (IBaseResource) theInvocation.getArguments()[1]; assertEquals("Patient/" + id + "/_history/2", res.getIdElement().getValue()); return null; - }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class)); + }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); Bundle xactBundle = new Bundle(); xactBundle.setType(BundleType.TRANSACTION); @@ -448,6 +451,7 @@ public class FhirResourceDaoDstu3InterceptorTest extends BaseJpaDstu3Test { assertEquals(2L, newId.getVersionIdPartAsLong().longValue()); verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class)); + verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); verify(myRequestOperationCallback, times(1)).resourceCreated(any(IBaseResource.class)); verifyNoMoreInteractions(myRequestOperationCallback); } @@ -462,9 +466,11 @@ public class FhirResourceDaoDstu3InterceptorTest extends BaseJpaDstu3Test { @Override public Void answer(InvocationOnMock theInvocation) throws Throwable { IBaseResource res = (IBaseResource) theInvocation.getArguments()[0]; + assertEquals("Patient/" + id + "/_history/1", res.getIdElement().getValue()); + res = (IBaseResource) theInvocation.getArguments()[1]; assertEquals("Patient/" + id + "/_history/2", res.getIdElement().getValue()); return null; - }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class)); + }}).when(myRequestOperationCallback).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); p = new Patient(); p.setId(new IdType("Patient/" + id)); @@ -473,6 +479,7 @@ public class FhirResourceDaoDstu3InterceptorTest extends BaseJpaDstu3Test { assertEquals(2L, newId.getVersionIdPartAsLong().longValue()); verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class)); + verify(myRequestOperationCallback, times(1)).resourceUpdated(any(IBaseResource.class), any(IBaseResource.class)); verify(myRequestOperationCallback, times(1)).resourceCreated(any(IBaseResource.class)); verifyNoMoreInteractions(myRequestOperationCallback); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/AuthorizationInterceptorResourceProviderDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/AuthorizationInterceptorResourceProviderDstu3Test.java index a73317d1bcc..60033b05f24 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/AuthorizationInterceptorResourceProviderDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/AuthorizationInterceptorResourceProviderDstu3Test.java @@ -14,10 +14,8 @@ import org.apache.http.client.methods.HttpDelete; import org.apache.http.client.methods.HttpPost; import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; -import org.hl7.fhir.dstu3.model.IdType; -import org.hl7.fhir.dstu3.model.Observation; +import org.hl7.fhir.dstu3.model.*; import org.hl7.fhir.dstu3.model.Observation.ObservationStatus; -import org.hl7.fhir.dstu3.model.Patient; import org.hl7.fhir.instance.model.api.IIdType; import org.junit.AfterClass; import org.junit.Test; @@ -183,6 +181,63 @@ public class AuthorizationInterceptorResourceProviderDstu3Test extends BaseResou } + /** + * See #667 + */ + @Test + public void testBlockUpdatingPatientUserDoesnNotHaveAccessTo() throws IOException { + Patient pt1 = new Patient(); + pt1.setActive(true); + final IIdType pid1 = ourClient.create().resource(pt1).execute().getId().toUnqualifiedVersionless(); + + Patient pt2 = new Patient(); + pt2.setActive(false); + final IIdType pid2 = ourClient.create().resource(pt2).execute().getId().toUnqualifiedVersionless(); + + ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow().write().allResources().inCompartment("Patient", pid1).andThen() + .build(); + } + }); + + Observation obs = new Observation(); + obs.setStatus(ObservationStatus.FINAL); + obs.setSubject(new Reference(pid1)); + IIdType oid = ourClient.create().resource(obs).execute().getId().toUnqualified(); + + + unregisterInterceptors(); + ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow().write().allResources().inCompartment("Patient", pid2).andThen() + .build(); + } + }); + + /* + * Try to update to a new patient. The user has access to write to things in + * pid2's compartment, so this would normally be ok, but in this case they are overwriting + * a resource that is already in pid1's compartment, which shouldn't be allowed. + */ + obs = new Observation(); + obs.setId(oid); + obs.setStatus(ObservationStatus.FINAL); + obs.setSubject(new Reference(pid2)); + + try { + ourClient.update().resource(obs).execute(); + fail(); + } catch (ForbiddenOperationException e) { + // good + } + + } + @Test public void testDeleteResourceConditional() throws IOException { String methodName = "testDeleteResourceConditional"; diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/InterceptorDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/InterceptorDstu3Test.java index 46b3c322b81..b22c6484eaf 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/InterceptorDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/InterceptorDstu3Test.java @@ -69,12 +69,14 @@ public class InterceptorDstu3Test { ourServlet.setInterceptors(myInterceptor1, myInterceptor2); } + @SuppressWarnings("deprecation") @Test public void testServerOperationInterceptorAdapterMethods() { ServerOperationInterceptorAdapter i = new ServerOperationInterceptorAdapter(); i.resourceCreated(null, null); i.resourceDeleted(null, null); i.resourceUpdated(null, null); + i.resourceUpdated(null, null, null); } @Test @@ -172,7 +174,6 @@ public class InterceptorDstu3Test { return Patient.class; } - @SuppressWarnings("unused") @Validate() public MethodOutcome validate(@ResourceParam Patient theResource) { return new MethodOutcome(); diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 281d7754638..7dc713b157d 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -6,6 +6,25 @@ HAPI FHIR Changelog + + + When using the AuthorizationInterceptor with the JPA server, when a client is updating a resource + from A to B, the user now needs to have write permission for both A and B. This is particularly + important for cases where (for example) an Observation is being updated from having a subject of + Patient/A to Patient/B. If the user has write permission for Patient/B's compartment, this would + previously have been allowed even if the user did not have access to write to Patient/A's compartment. + Thanks to Eeva Turkka for reporting! + + + IServerOperationInterceptor now has a new method + resourceUpdated(RequestDetails, IBaseResource, IBaseResource)]]> + which replaces the previous + resourceUpdated(RequestDetails, IBaseResource)]]>. This allows + interceptors to be notified of resource updates, but also see what the resource + looked like before the update. This change was made to support the change above, but + seems like a useful feature all around. + +