From 742389642dde5c6dad63b5089ba5dfc860b198d4 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Mon, 1 Jul 2019 10:49:38 -0400 Subject: [PATCH] Add tests for delete conflicts and authorizationinterceptor --- .../java/ca/uhn/fhir/rest/api/Constants.java | 3 +- .../main/java/ca/uhn/fhir/util/UrlUtil.java | 4 +- .../fhir/jpa/delete/DeleteConflictList.java | 3 +- .../jpa/delete/DeleteConflictService.java | 12 ++ .../CascadingDeleteInterceptor.java | 6 +- .../delete/DeleteConflictServiceR4Test.java | 32 ++++ ...tionInterceptorResourceProviderR4Test.java | 6 +- .../r4/CascadingDeleteInterceptorR4Test.java | 39 ++++- .../auth/AuthorizationInterceptor.java | 10 +- .../auth/IAuthRuleBuilderAppliesTo.java | 7 + .../auth/IAuthRuleBuilderRuleOp.java | 1 + .../server/interceptor/auth/RuleBuilder.java | 41 ++++- .../interceptor/auth/RuleImplConditional.java | 12 +- .../server/interceptor/auth/RuleImplOp.java | 48 ++++-- .../auth/AuthorizationInterceptorR4Test.java | 154 +++++++++++------- src/site/xdoc/doc_jpa.xml | 4 +- 16 files changed, 276 insertions(+), 106 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java index 96b6c749367..9f6f8c5d244 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java @@ -208,7 +208,8 @@ public class Constants { public static final String PARAM_SEARCH_TOTAL_MODE = "_total"; public static final String CAPABILITYSTATEMENT_WEBSOCKET_URL = "http://hl7.org/fhir/StructureDefinition/capabilitystatement-websocket"; public static final String PARAMETER_CASCADE_DELETE = "_cascade"; - public static final String HEADER_CASCADE_DELETE = "X-Cascade-Delete"; + public static final String HEADER_CASCADE = "X-Cascade"; + public static final String CASCADE_DELETE = "delete"; static { CHARSET_UTF8 = Charset.forName(CHARSET_NAME_UTF8); diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/UrlUtil.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/UrlUtil.java index 779a64e8103..2429f3a4088 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/UrlUtil.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/UrlUtil.java @@ -16,9 +16,7 @@ import java.net.URLDecoder; import java.util.*; import java.util.Map.Entry; -import static org.apache.commons.lang3.StringUtils.defaultIfBlank; -import static org.apache.commons.lang3.StringUtils.defaultString; -import static org.apache.commons.lang3.StringUtils.isBlank; +import static org.apache.commons.lang3.StringUtils.*; /* * #%L diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictList.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictList.java index 7167b1ac4d6..56da09824d0 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictList.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictList.java @@ -27,7 +27,7 @@ import java.util.Iterator; import java.util.List; import java.util.function.Predicate; -public class DeleteConflictList { +public class DeleteConflictList implements Iterable { private final List myList = new ArrayList<>(); public void add(DeleteConflict theDeleteConflict) { @@ -38,6 +38,7 @@ public class DeleteConflictList { return myList.isEmpty(); } + @Override public Iterator iterator() { return myList.iterator(); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictService.java index bf0e2f6015c..d9bb46fe9c4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictService.java @@ -42,8 +42,11 @@ import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Service; +import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; @Service public class DeleteConflictService { @@ -88,6 +91,15 @@ public class DeleteConflictService { if (resultList.isEmpty()) { return null; } + + // Don't send two conflict events for the same source resource + Set sourceIds = new HashSet<>(); + resultList = resultList + .stream() + .filter(t -> sourceIds.add(t.getSourceResourcePid())) + .collect(Collectors.toList()); + + return handleConflicts(theRequest, theDeleteConflicts, theEntity, theForValidate, resultList); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/CascadingDeleteInterceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/CascadingDeleteInterceptor.java index 2ce4e7f9cd8..313922f4023 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/CascadingDeleteInterceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/CascadingDeleteInterceptor.java @@ -177,12 +177,12 @@ public class CascadingDeleteInterceptor { protected boolean shouldCascade(RequestDetails theRequest) { if (theRequest != null) { String[] cascadeParameters = theRequest.getParameters().get(Constants.PARAMETER_CASCADE_DELETE); - if (cascadeParameters != null && Arrays.asList(cascadeParameters).contains("true")) { + if (cascadeParameters != null && Arrays.asList(cascadeParameters).contains(Constants.CASCADE_DELETE)) { return true; } - String cascadeHeader = theRequest.getHeader(Constants.HEADER_CASCADE_DELETE); - if ("true".equals(cascadeHeader)) { + String cascadeHeader = theRequest.getHeader(Constants.HEADER_CASCADE); + if (Constants.CASCADE_DELETE.equals(cascadeHeader)) { return true; } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/delete/DeleteConflictServiceR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/delete/DeleteConflictServiceR4Test.java index 3ae6961748b..169e58564e9 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/delete/DeleteConflictServiceR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/delete/DeleteConflictServiceR4Test.java @@ -7,6 +7,7 @@ import ca.uhn.fhir.jpa.util.DeleteConflict; import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException; import org.hl7.fhir.instance.model.api.IIdType; +import org.hl7.fhir.r4.model.Condition; import org.hl7.fhir.r4.model.Organization; import org.hl7.fhir.r4.model.Patient; import org.hl7.fhir.r4.model.Reference; @@ -16,7 +17,9 @@ import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.ArrayList; import java.util.Iterator; +import java.util.List; import java.util.function.Function; import static org.junit.Assert.*; @@ -151,6 +154,35 @@ public class DeleteConflictServiceR4Test extends BaseJpaR4Test { assertEquals(1 + DeleteConflictService.MAX_RETRY_ATTEMPTS, myDeleteInterceptor.myCallCount); } + @Test + public void testNoDuplicateConstraintReferences() { + Patient patient = new Patient(); + patient.setActive(true); + IIdType patientId = myPatientDao.create(patient).getId().toUnqualifiedVersionless(); + + Condition condition = new Condition(); + condition.setSubject(new Reference(patientId)); + condition.setAsserter(new Reference(patientId)); + myConditionDao.create(condition); + + List conflicts = new ArrayList<>(); + myDeleteInterceptor.deleteConflictFunction = t -> { + for (DeleteConflict next : t) { + conflicts.add(next); + } + return new DeleteConflictOutcome().setShouldRetryCount(0); + }; + + try { + myPatientDao.delete(patientId); + fail(); + } catch (ResourceVersionConflictException e) { + // good + } + + assertEquals(1, conflicts.size()); + } + private DeleteConflictOutcome deleteConflicts(DeleteConflictList theList) { Iterator iterator = theList.iterator(); while (iterator.hasNext()) { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java index 33f6db3fa60..05b82a63f63 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorResourceProviderR4Test.java @@ -456,7 +456,7 @@ public class AuthorizationInterceptorResourceProviderR4Test extends BaseResource ourClient .delete() .resourceById(patientId) - .withAdditionalHeader(Constants.HEADER_CASCADE_DELETE, "true") + .withAdditionalHeader(Constants.HEADER_CASCADE, "true") .execute(); fail(); } catch (ForbiddenOperationException e) { @@ -500,7 +500,7 @@ public class AuthorizationInterceptorResourceProviderR4Test extends BaseResource ourClient .delete() .resourceById(patientId) - .withAdditionalHeader(Constants.HEADER_CASCADE_DELETE, "true") + .withAdditionalHeader(Constants.HEADER_CASCADE, "true") .execute(); } finally { @@ -541,7 +541,7 @@ public class AuthorizationInterceptorResourceProviderR4Test extends BaseResource ourClient .delete() .resourceById(patientId) - .withAdditionalHeader(Constants.HEADER_CASCADE_DELETE, "true") + .withAdditionalHeader(Constants.HEADER_CASCADE, "true") .execute(); fail(); } catch (ForbiddenOperationException e) { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/CascadingDeleteInterceptorR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/CascadingDeleteInterceptorR4Test.java index 7b39364dd43..fa1ad11186b 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/CascadingDeleteInterceptorR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/CascadingDeleteInterceptorR4Test.java @@ -12,9 +12,7 @@ import org.apache.commons.io.IOUtils; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpDelete; import org.hl7.fhir.instance.model.api.IIdType; -import org.hl7.fhir.r4.model.DiagnosticReport; -import org.hl7.fhir.r4.model.Observation; -import org.hl7.fhir.r4.model.Patient; +import org.hl7.fhir.r4.model.*; import org.junit.After; import org.junit.AfterClass; import org.junit.Before; @@ -39,6 +37,7 @@ public class CascadingDeleteInterceptorR4Test extends BaseResourceProviderR4Test private IIdType myPatientId; private CascadingDeleteInterceptor myDeleteInterceptor; private IIdType myObservationId; + private IIdType myConditionId; @Override @Before @@ -69,6 +68,11 @@ public class CascadingDeleteInterceptorR4Test extends BaseResourceProviderR4Test dr.setStatus(DiagnosticReport.DiagnosticReportStatus.FINAL); dr.addResult().setReference(myObservationId.getValue()); myDiagnosticReportId = ourClient.create().resource(dr).execute().getId().toUnqualifiedVersionless(); + + Condition condition = new Condition(); + condition.setSubject(new Reference(myPatientId)); + condition.setAsserter(new Reference(myPatientId)); + myConditionId = ourClient.create().resource(condition).execute().getId().toUnqualifiedVersionless(); } @Test @@ -106,13 +110,38 @@ public class CascadingDeleteInterceptorR4Test extends BaseResourceProviderR4Test ourRestServer.getInterceptorService().registerInterceptor(myDeleteInterceptor); - HttpDelete delete = new HttpDelete(ourServerBase + "/" + myPatientId.getValue() + "?_cascade=true&_pretty=true"); + HttpDelete delete = new HttpDelete(ourServerBase + "/" + myPatientId.getValue() + "?" + Constants.PARAMETER_CASCADE_DELETE + "=" + Constants.CASCADE_DELETE + "&_pretty=true"); delete.addHeader(Constants.HEADER_ACCEPT, Constants.CT_FHIR_JSON_NEW); try (CloseableHttpResponse response = ourHttpClient.execute(delete)) { assertEquals(200, response.getStatusLine().getStatusCode()); String deleteResponse = IOUtils.toString(response.getEntity().getContent(), Charsets.UTF_8); ourLog.info("Response: {}", deleteResponse); - assertThat(deleteResponse, containsString("Cascaded delete to 2 resources: [" + myDiagnosticReportId + "/_history/1, " + myObservationId + "/_history/1]")); + assertThat(deleteResponse, containsString("Cascaded delete to 3 resources: [" + myDiagnosticReportId + "/_history/1, " + myObservationId + "/_history/1, " + myConditionId + "/_history/1]")); + } + + try { + ourLog.info("Reading {}", myPatientId); + ourClient.read().resource(Patient.class).withId(myPatientId).execute(); + fail(); + } catch (ResourceGoneException e) { + // good + } + } + + @Test + public void testDeleteCascadingByHeader() throws IOException { + createResources(); + + ourRestServer.getInterceptorService().registerInterceptor(myDeleteInterceptor); + + HttpDelete delete = new HttpDelete(ourServerBase + "/" + myPatientId.getValue() + "?_pretty=true"); + delete.addHeader(Constants.HEADER_CASCADE, Constants.CASCADE_DELETE); + delete.addHeader(Constants.HEADER_ACCEPT, Constants.CT_FHIR_JSON_NEW); + try (CloseableHttpResponse response = ourHttpClient.execute(delete)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + String deleteResponse = IOUtils.toString(response.getEntity().getContent(), Charsets.UTF_8); + ourLog.info("Response: {}", deleteResponse); + assertThat(deleteResponse, containsString("Cascaded delete to 3 resources: [" + myDiagnosticReportId + "/_history/1, " + myObservationId + "/_history/1, " + myConditionId + "/_history/1]")); } try { diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java index fa2fcca3d5f..0defc78c398 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java @@ -40,10 +40,12 @@ import org.hl7.fhir.instance.model.api.IIdType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nonnull; import java.util.*; import java.util.concurrent.atomic.AtomicInteger; import static org.apache.commons.lang3.StringUtils.defaultString; +import static org.apache.commons.lang3.StringUtils.isNotBlank; /** * This class is a base class for interceptors which can be used to @@ -299,6 +301,10 @@ public class AuthorizationInterceptor implements IRuleApplier { case BOTH: inputResource = theRequest.getResource(); inputResourceId = theRequest.getId(); + if (inputResourceId == null && isNotBlank(theRequest.getResourceName())) { + inputResourceId = theRequest.getFhirContext().getVersion().newIdType(); + inputResourceId.setParts(null, theRequest.getResourceName(), null, null); + } break; case OUT: // inputResource = null; @@ -330,8 +336,8 @@ public class AuthorizationInterceptor implements IRuleApplier { checkPointcutAndFailIfDeny(theRequestDetails, thePointcut, theResourceToDelete); } - private void checkPointcutAndFailIfDeny(RequestDetails theRequestDetails, Pointcut thePointcut, IBaseResource theInputResource) { - applyRulesAndFailIfDeny(theRequestDetails.getRestOperationType(), theRequestDetails, theInputResource, null, null, thePointcut); + private void checkPointcutAndFailIfDeny(RequestDetails theRequestDetails, Pointcut thePointcut, @Nonnull IBaseResource theInputResource) { + applyRulesAndFailIfDeny(theRequestDetails.getRestOperationType(), theRequestDetails, theInputResource, theInputResource.getIdElement(), null, thePointcut); } private void checkOutgoingResourceAndFailIfDeny(RequestDetails theRequestDetails, IBaseResource theResponseObject, Pointcut thePointcut) { diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderAppliesTo.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderAppliesTo.java index aba5e493d63..167dc2aad5c 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderAppliesTo.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderAppliesTo.java @@ -34,4 +34,11 @@ public interface IAuthRuleBuilderAppliesTo { */ T resourcesOfType(Class theType); + /** + * Rule applies to resources of the given type + * + * @param theType E.g. "Patient" + */ + T resourcesOfType(String theType); + } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderRuleOp.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderRuleOp.java index bd5dae2f5fc..865f9755bc5 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderRuleOp.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderRuleOp.java @@ -76,4 +76,5 @@ public interface IAuthRuleBuilderRuleOp extends IAuthRuleBuilderAppliesTo theIds); + } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBuilder.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBuilder.java index e6c961a6537..da02c6e8d63 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBuilder.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBuilder.java @@ -20,6 +20,7 @@ package ca.uhn.fhir.rest.server.interceptor.auth; * #L% */ +import ca.uhn.fhir.model.api.annotation.ResourceDef; import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.api.server.RequestDetails; @@ -29,12 +30,14 @@ import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import static org.apache.commons.lang3.ObjectUtils.defaultIfNull; public class RuleBuilder implements IAuthRuleBuilder { private static final String[] EMPTY_STRING_ARRAY = new String[0]; + private static final ConcurrentHashMap, String> ourTypeToName = new ConcurrentHashMap<>(); private ArrayList myRules; private IAuthRuleBuilderRule myAllow; private IAuthRuleBuilderRule myDeny; @@ -257,7 +260,7 @@ public class RuleBuilder implements IAuthRuleBuilder { private class RuleBuilderRuleConditional implements IAuthRuleBuilderRuleConditional { private AppliesTypeEnum myAppliesTo; - private Set myAppliesToTypes; + private Set myAppliesToTypes; private RestOperationTypeEnum myOperationType; RuleBuilderRuleConditional(RestOperationTypeEnum theOperationType) { @@ -273,6 +276,13 @@ public class RuleBuilder implements IAuthRuleBuilder { @Override public IAuthRuleBuilderRuleConditionalClassifier resourcesOfType(Class theType) { Validate.notNull(theType, "theType must not be null"); + + String typeName = toTypeName(theType); + return resourcesOfType(typeName); + } + + @Override + public IAuthRuleBuilderRuleConditionalClassifier resourcesOfType(String theType) { myAppliesTo = AppliesTypeEnum.TYPES; myAppliesToTypes = Collections.singleton(theType); return new RuleBuilderRuleConditionalClassifier(); @@ -306,7 +316,7 @@ public class RuleBuilder implements IAuthRuleBuilder { private RuleBuilderRuleOpClassifier myInstancesBuilder; private boolean myOnCascade; - public RuleBuilderRuleOp(RuleOpEnum theRuleOp) { + RuleBuilderRuleOp(RuleOpEnum theRuleOp) { myRuleOp = theRuleOp; } @@ -345,8 +355,16 @@ public class RuleBuilder implements IAuthRuleBuilder { } } + @Override public IAuthRuleBuilderRuleOpClassifier resourcesOfType(Class theType) { + Validate.notNull(theType, "theType must not be null"); + String resourceName = toTypeName(theType); + return resourcesOfType(resourceName); + } + + @Override + public IAuthRuleBuilderRuleOpClassifier resourcesOfType(String theType) { Validate.notNull(theType, "theType must not be null"); return new RuleBuilderRuleOpClassifier(AppliesTypeEnum.TYPES, Collections.singleton(theType)); } @@ -360,7 +378,7 @@ public class RuleBuilder implements IAuthRuleBuilder { private class RuleBuilderRuleOpClassifier implements IAuthRuleBuilderRuleOpClassifier { private final AppliesTypeEnum myAppliesTo; - private final Set myAppliesToTypes; + private final Set myAppliesToTypes; private ClassifierTypeEnum myClassifierType; private String myInCompartmentName; private Collection myInCompartmentOwners; @@ -370,7 +388,7 @@ public class RuleBuilder implements IAuthRuleBuilder { /** * Constructor */ - RuleBuilderRuleOpClassifier(AppliesTypeEnum theAppliesTo, Set> theAppliesToTypes) { + RuleBuilderRuleOpClassifier(AppliesTypeEnum theAppliesTo, Set theAppliesToTypes) { super(); myAppliesTo = theAppliesTo; myAppliesToTypes = theAppliesToTypes; @@ -551,7 +569,7 @@ public class RuleBuilder implements IAuthRuleBuilder { private final OperationRule myRule; - public RuleBuilderOperationNamedAndScoped(OperationRule theRule) { + RuleBuilderOperationNamedAndScoped(OperationRule theRule) { myRule = theRule; } @@ -599,7 +617,7 @@ public class RuleBuilder implements IAuthRuleBuilder { private class PatchBuilder implements IAuthRuleBuilderPatch { - public PatchBuilder() { + PatchBuilder() { super(); } @@ -625,4 +643,15 @@ public class RuleBuilder implements IAuthRuleBuilder { } } + private static String toTypeName(Class theType) { + String retVal = ourTypeToName.get(theType); + if (retVal == null) { + ResourceDef resourceDef = theType.getAnnotation(ResourceDef.class); + retVal = resourceDef.name(); + Validate.notBlank(retVal, "Could not determine resource type of class %s", theType); + ourTypeToName.put(theType, retVal); + } + return retVal; + } + } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplConditional.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplConditional.java index 72c86ceae71..1dd0bca5e4d 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplConditional.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplConditional.java @@ -32,7 +32,7 @@ import java.util.Set; public class RuleImplConditional extends BaseRule implements IAuthRule { private AppliesTypeEnum myAppliesTo; - private Set myAppliesToTypes; + private Set myAppliesToTypes; private RestOperationTypeEnum myOperationType; RuleImplConditional(String theRuleName) { @@ -47,7 +47,7 @@ public class RuleImplConditional extends BaseRule implements IAuthRule { return null; } - if (theInputResourceId != null) { + if (theInputResourceId != null && theInputResourceId.hasIdPart()) { return null; } @@ -63,12 +63,12 @@ public class RuleImplConditional extends BaseRule implements IAuthRule { case TYPES: if (myOperationType == RestOperationTypeEnum.DELETE) { String resourceName = theRequestDetails.getResourceName(); - Class resourceType = theRequestDetails.getFhirContext().getResourceDefinition(resourceName).getImplementingClass(); - if (!myAppliesToTypes.contains(resourceType)) { + if (!myAppliesToTypes.contains(resourceName)) { return null; } } else { - if (theInputResource == null || !myAppliesToTypes.contains(theInputResource.getClass())) { + String inputResourceName = theRequestDetails.getFhirContext().getResourceDefinition(theInputResource).getName(); + if (theInputResource == null || !myAppliesToTypes.contains(inputResourceName)) { return null; } } @@ -95,7 +95,7 @@ public class RuleImplConditional extends BaseRule implements IAuthRule { myAppliesTo = theAppliesTo; } - void setAppliesToTypes(Set theAppliesToTypes) { + void setAppliesToTypes(Set theAppliesToTypes) { myAppliesToTypes = theAppliesToTypes; } 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 94316132aa6..11766300b35 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 @@ -13,8 +13,8 @@ import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor.Verdict; import ca.uhn.fhir.util.BundleUtil; import ca.uhn.fhir.util.BundleUtil.BundleEntryParts; -import ca.uhn.fhir.util.CollectionUtil; import ca.uhn.fhir.util.FhirTerser; +import ca.uhn.fhir.util.UrlUtil; import com.google.common.annotations.VisibleForTesting; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.builder.ToStringBuilder; @@ -53,7 +53,7 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank; class RuleImplOp extends BaseRule /* implements IAuthRule */ { private AppliesTypeEnum myAppliesTo; - private Set myAppliesToTypes; + private Set myAppliesToTypes; private String myClassifierCompartmentName; private Collection myClassifierCompartmentOwners; private ClassifierTypeEnum myClassifierType; @@ -212,9 +212,18 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { if (myAppliesToDeleteCascade != (thePointcut == Pointcut.STORAGE_CASCADE_DELETE)) { return null; } - if (theInputResource == null) { + if (theInputResourceId == null) { + return null; + } + if (theInputResourceId.hasIdPart() == false) { + // This is a conditional DELETE, so we'll authorize it using STORAGE events instead + // so just let it through for now.. return newVerdict(); } + if (theInputResource== null && myClassifierCompartmentOwners != null && myClassifierCompartmentOwners.size() > 0) { + return newVerdict(); + } + appliesToResource = theInputResource; appliesToResourceId = Collections.singleton(theInputResourceId); } else { @@ -242,6 +251,15 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { for (BundleEntryParts nextPart : inputResources) { IBaseResource inputResource = nextPart.getResource(); + IIdType inputResourceId = null; + if (isNotBlank(nextPart.getUrl())) { + + UrlUtil.UrlParts parts = UrlUtil.parseUrl(nextPart.getUrl()); + + inputResourceId = theRequestDetails.getFhirContext().getVersion().newIdType(); + inputResourceId.setParts(null, parts.getResourceType(), parts.getResourceId(), null); + } + RestOperationTypeEnum operation; if (nextPart.getRequestType() == RequestTypeEnum.GET) { continue; @@ -270,7 +288,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { } } - Verdict newVerdict = theRuleApplier.applyRulesAndReturnDecision(operation, theRequestDetails, inputResource, null, null, thePointcut); + Verdict newVerdict = theRuleApplier.applyRulesAndReturnDecision(operation, theRequestDetails, inputResource, inputResourceId, null, thePointcut); if (newVerdict == null) { continue; } else if (verdict == null) { @@ -377,7 +395,8 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { case TYPES: if (appliesToResource != null) { if (myClassifierType == ClassifierTypeEnum.ANY_ID) { - if (myAppliesToTypes.contains(appliesToResource.getClass()) == false) { + String type = theRequestDetails.getFhirContext().getResourceDefinition(appliesToResource).getName(); + if (myAppliesToTypes.contains(type) == false) { return null; } } @@ -385,21 +404,20 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { if (appliesToResourceId != null) { for (IIdType nextRequestAppliesToResourceId : appliesToResourceId) { if (nextRequestAppliesToResourceId.hasResourceType()) { - Class type = theRequestDetails.getServer().getFhirContext().getResourceDefinition(nextRequestAppliesToResourceId.getResourceType()).getImplementingClass(); - if (myAppliesToTypes.contains(type) == false) { + String nextRequestAppliesToResourceIdType = nextRequestAppliesToResourceId.getResourceType(); + if (myAppliesToTypes.contains(nextRequestAppliesToResourceIdType) == false) { return null; } } } } if (appliesToResourceType != null) { - Class type = theRequestDetails.getServer().getFhirContext().getResourceDefinition(appliesToResourceType).getImplementingClass(); - if (myAppliesToTypes.contains(type)) { + if (myAppliesToTypes.contains(appliesToResourceType)) { if (!applyTesters(theOperation, theRequestDetails, theInputResourceId, theInputResource, theOutputResource)) { return null; } if (myClassifierType == ClassifierTypeEnum.ANY_ID) { - return new Verdict(PolicyEnum.ALLOW, this); + return newVerdict(); } else if (myClassifierType == ClassifierTypeEnum.IN_COMPARTMENT) { // ok we'll check below } @@ -554,19 +572,19 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { myAppliesTo = theAppliesTo; } - public void setAppliesToTypes(Set theAppliesToTypes) { + void setAppliesToTypes(Set theAppliesToTypes) { myAppliesToTypes = theAppliesToTypes; } - public void setClassifierCompartmentName(String theClassifierCompartmentName) { + void setClassifierCompartmentName(String theClassifierCompartmentName) { myClassifierCompartmentName = theClassifierCompartmentName; } - public void setClassifierCompartmentOwners(Collection theInCompartmentOwners) { + void setClassifierCompartmentOwners(Collection theInCompartmentOwners) { myClassifierCompartmentOwners = theInCompartmentOwners; } - public void setClassifierType(ClassifierTypeEnum theClassifierType) { + void setClassifierType(ClassifierTypeEnum theClassifierType) { myClassifierType = theClassifierType; } @@ -590,7 +608,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { return builder.toString(); } - public void setAppliesToDeleteCascade(boolean theAppliesToDeleteCascade) { + void setAppliesToDeleteCascade(boolean theAppliesToDeleteCascade) { myAppliesToDeleteCascade = theAppliesToDeleteCascade; } diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java index 6812a623858..bd3e9a6087b 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java @@ -255,7 +255,7 @@ public class AuthorizationInterceptorR4Test { ourLog.info(response); assertThat(response, containsString("Access denied by rule: Rule 1")); assertEquals(403, status.getStatusLine().getStatusCode()); - assertTrue(ourHitMethod); + assertFalse(ourHitMethod); ourHitMethod = false; httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/$validate"); @@ -299,7 +299,7 @@ public class AuthorizationInterceptorR4Test { ourLog.info(response); assertThat(response, containsString("Access denied by rule: Rule 1")); assertEquals(403, status.getStatusLine().getStatusCode()); - assertTrue(ourHitMethod); + assertFalse(ourHitMethod); ourHitMethod = false; httpGet = new HttpGet("http://localhost:" + ourPort + "/TENANTA/Patient/1/$validate"); @@ -626,12 +626,13 @@ public class AuthorizationInterceptorR4Test { Bundle responseBundle = new Bundle(); responseBundle.setType(Bundle.BundleType.TRANSACTIONRESPONSE); + Bundle bundle = new Bundle(); + bundle.setType(Bundle.BundleType.TRANSACTION); ourHitMethod = false; - Bundle bundle = new Bundle(); + bundle.getEntry().clear(); ourReturn = Collections.singletonList(responseBundle); ourDeleted = Collections.singletonList(createPatient(2)); - bundle.setType(Bundle.BundleType.TRANSACTION); bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl("Patient/2"); httpPost = new HttpPost("http://localhost:" + ourPort + "/"); httpPost.setEntity(new StringEntity(ourCtx.newJsonParser().encodeResourceToString(bundle), ContentType.create(Constants.CT_FHIR_JSON_NEW, Charsets.UTF_8))); @@ -640,6 +641,7 @@ public class AuthorizationInterceptorR4Test { assertEquals(responseString, 403, status.getStatusLine().getStatusCode()); assertTrue(ourHitMethod); + ourHitMethod = false; bundle.getEntry().clear(); bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl("Patient/1"); ourReturn = Collections.singletonList(responseBundle); @@ -676,6 +678,40 @@ public class AuthorizationInterceptorR4Test { assertTrue(ourHitMethod); } + @Test + public void testDeleteByType() throws Exception { + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("Rule 1").delete().resourcesOfType(Patient.class).withAnyId().andThen() + .build(); + } + }); + + HttpDelete httpDelete; + HttpResponse status; + String responseString; + + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpDelete = new HttpDelete("http://localhost:" + ourPort + "/Patient/1"); + status = ourClient.execute(httpDelete); + responseString = extractResponseAndClose(status); + assertEquals(responseString, 204, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpDelete = new HttpDelete("http://localhost:" + ourPort + "/Observation/1"); + status = ourClient.execute(httpDelete); + responseString = extractResponseAndClose(status); + assertEquals(responseString, 403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + + + } + /** * #528 */ @@ -2575,52 +2611,52 @@ public class AuthorizationInterceptorR4Test { HttpGet httpGet; ourReturn = Collections.singletonList(createPatient(900)); -// ourHitMethod = false; -// httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=900"); -// status = ourClient.execute(httpGet); -// extractResponseAndClose(status); -// assertEquals(200, status.getStatusLine().getStatusCode()); -// assertTrue(ourHitMethod); -// -// ourHitMethod = false; -// httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=Patient/900"); -// status = ourClient.execute(httpGet); -// extractResponseAndClose(status); -// assertEquals(200, status.getStatusLine().getStatusCode()); -// assertTrue(ourHitMethod); -// -// ourHitMethod = false; -// httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=901"); -// status = ourClient.execute(httpGet); -// response = extractResponseAndClose(status); -// assertEquals(403, status.getStatusLine().getStatusCode()); -// assertEquals(ERR403, response); -// assertFalse(ourHitMethod); -// -// ourHitMethod = false; -// httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=Patient/901"); -// status = ourClient.execute(httpGet); -// response = extractResponseAndClose(status); -// assertEquals(403, status.getStatusLine().getStatusCode()); -// assertEquals(ERR403, response); -// assertFalse(ourHitMethod); -// -// ourHitMethod = false; -// // technically this is invalid, but just in case.. -// httpGet = new HttpGet("http://localhost:" + ourPort + "/Observation?_id=Patient/901"); -// status = ourClient.execute(httpGet); -// response = extractResponseAndClose(status); -// assertEquals(403, status.getStatusLine().getStatusCode()); -// assertEquals(ERR403, response); -// assertFalse(ourHitMethod); -// -// ourHitMethod = false; -// httpGet = new HttpGet("http://localhost:" + ourPort + "/Observation?_id=901"); -// status = ourClient.execute(httpGet); -// response = extractResponseAndClose(status); -// assertEquals(403, status.getStatusLine().getStatusCode()); -// assertEquals(ERR403, response); -// assertFalse(ourHitMethod); + ourHitMethod = false; + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=900"); + status = ourClient.execute(httpGet); + extractResponseAndClose(status); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + ourHitMethod = false; + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=Patient/900"); + status = ourClient.execute(httpGet); + extractResponseAndClose(status); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + ourHitMethod = false; + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=901"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertEquals(ERR403, response); + assertFalse(ourHitMethod); + + ourHitMethod = false; + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=Patient/901"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertEquals(ERR403, response); + assertFalse(ourHitMethod); + + ourHitMethod = false; + // technically this is invalid, but just in case.. + httpGet = new HttpGet("http://localhost:" + ourPort + "/Observation?_id=Patient/901"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertEquals(ERR403, response); + assertFalse(ourHitMethod); + + ourHitMethod = false; + httpGet = new HttpGet("http://localhost:" + ourPort + "/Observation?_id=901"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertEquals(ERR403, response); + assertFalse(ourHitMethod); ourHitMethod = false; httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=Patient/900,Patient/700"); @@ -3043,15 +3079,15 @@ public class AuthorizationInterceptorR4Test { HttpResponse status; String response; -// // Wrong resource -// ourReturn = Collections.singletonList(createPatient(1)); -// ourHitMethod = false; -// httpDelete = new HttpDelete("http://localhost:" + ourPort + "/Patient?foo=bar"); -// status = ourClient.execute(httpDelete); -// response = extractResponseAndClose(status); -// ourLog.info(response); -// assertEquals(403, status.getStatusLine().getStatusCode()); -// assertTrue(ourHitMethod); + // Wrong resource + ourReturn = Collections.singletonList(createPatient(1)); + ourHitMethod = false; + httpDelete = new HttpDelete("http://localhost:" + ourPort + "/Patient?foo=bar"); + status = ourClient.execute(httpDelete); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); // Right resource ourReturn = Collections.singletonList(createPatient(2)); diff --git a/src/site/xdoc/doc_jpa.xml b/src/site/xdoc/doc_jpa.xml index 5cb1c4289cc..dfee8e9ab03 100644 --- a/src/site/xdoc/doc_jpa.xml +++ b/src/site/xdoc/doc_jpa.xml @@ -577,10 +577,10 @@ delete from hfj_res_ver where res_id in (select res_id from hfj_resource where s

  • The request may include the following parameter: - _cascade=true + _cascade=delete
  • The request may include the following header: - X-Cascade-Delete: true + X-Cascade: delete