From 37c40c4f9ef1167e1d4e17a114b9a8cda2e11168 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 13 Jan 2017 11:06:00 -0500 Subject: [PATCH] Fix #503 - Checking authorization again patient compartment fails with delete operation --- .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 3 +- ...nInterceptorResourceProviderDstu3Test.java | 76 +++++++++++++++++++ src/changes/changes.xml | 7 ++ 3 files changed, 85 insertions(+), 1 deletion(-) 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 58d125b103a..db57a3485c4 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 @@ -183,6 +183,8 @@ public abstract class BaseHapiFhirResourceDao extends B throw new ResourceVersionConflictException("Trying to delete " + theId + " but this is not the current version"); } + T resourceToDelete = toResource(myResourceType, entity, false); + validateOkToDelete(deleteConflicts, entity); // Notify interceptors @@ -197,7 +199,6 @@ public abstract class BaseHapiFhirResourceDao extends B // Notify JPA interceptors if (theRequestDetails != null) { ActionRequestDetails requestDetails = new ActionRequestDetails(theRequestDetails, getContext(), theId.getResourceType(), theId); - T resourceToDelete = toResource(myResourceType, entity, false); theRequestDetails.getRequestOperationCallback().resourceDeleted(resourceToDelete); for (IServerInterceptor next : getConfig().getInterceptors()) { if (next instanceof IJpaServerInterceptor) { 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 6b44b384562..a73317d1bcc 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 @@ -15,7 +15,10 @@ 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.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; @@ -55,6 +58,79 @@ public class AuthorizationInterceptorResourceProviderDstu3Test extends BaseResou } } + /** + * See #503 + */ + @Test + public void testDeleteIsBlocked() { + + ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .deny().delete().allResources().withAnyId().andThen() + .allowAll() + .build(); + } + }); + + Patient patient = new Patient(); + patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100"); + patient.addName().setFamily("Tester").addGiven("Raghad"); + IIdType id = ourClient.create().resource(patient).execute().getId(); + + try { + ourClient.delete().resourceById(id.toUnqualifiedVersionless()).execute(); + fail(); + } catch (ForbiddenOperationException e) { + // good + } + + patient = ourClient.read().resource(Patient.class).withId(id.toUnqualifiedVersionless()).execute(); + assertEquals(id.getValue(), patient.getId()); + } + + + /** + * See #503 + */ + @Test + public void testDeleteIsAllowedForCompartment() { + + Patient patient = new Patient(); + patient.addIdentifier().setSystem("http://uhn.ca/mrns").setValue("100"); + patient.addName().setFamily("Tester").addGiven("Raghad"); + final IIdType id = ourClient.create().resource(patient).execute().getId(); + + Observation obsInCompartment = new Observation(); + obsInCompartment.setStatus(ObservationStatus.FINAL); + obsInCompartment.getSubject().setReferenceElement(id.toUnqualifiedVersionless()); + IIdType obsInCompartmentId = ourClient.create().resource(obsInCompartment).execute().getId().toUnqualifiedVersionless(); + + Observation obsNotInCompartment = new Observation(); + obsNotInCompartment.setStatus(ObservationStatus.FINAL); + IIdType obsNotInCompartmentId = ourClient.create().resource(obsNotInCompartment).execute().getId().toUnqualifiedVersionless(); + + ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow().delete().resourcesOfType(Observation.class).inCompartment("Patient", id).andThen() + .deny().delete().allResources().withAnyId().andThen() + .allowAll() + .build(); + } + }); + + ourClient.delete().resourceById(obsInCompartmentId.toUnqualifiedVersionless()).execute(); + + try { + ourClient.delete().resourceById(obsNotInCompartmentId.toUnqualifiedVersionless()).execute(); + fail(); + } catch (ForbiddenOperationException e) { + // good + } + } @Test public void testCreateConditional() { diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 9cedbf31eb1..63ed1f00e39 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -185,6 +185,13 @@ framework was not actually useful. Thanks to GitHub user @mattiuusitalo for reporting! + + AuthorizationInterceptor on JPA server did not correctly + apply rules on deleting resources in a specific compartment + because the resource metadata was stripped by the JPA server + before the interceptor could see it. Thanks to + GitHub user @eevaturkka for reporting! +