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 65e0cc6da72..00125a6cbc3 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 @@ -239,6 +239,52 @@ public class AuthorizationInterceptorResourceProviderR4Test extends BaseResource } } + @Test + public void testDeleteIsAllowedForCompartmentUsingTransaction() { + + 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() + .allow().transaction().withAnyOperation().andApplyNormalRules().andThen() + .denyAll() + .build(); + } + }); + + Bundle bundle; + + bundle = new Bundle(); + bundle.setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl(obsInCompartmentId.toUnqualifiedVersionless().getValue()); + ourClient.transaction().withBundle(bundle).execute(); + + try { + bundle = new Bundle(); + bundle.setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl(obsNotInCompartmentId.toUnqualifiedVersionless().getValue()); + ourClient.transaction().withBundle(bundle).execute(); + fail(); + } catch (ForbiddenOperationException e) { + // good + } + } + /** * See #503 */ 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 2ae10cfe149..7ed4209830d 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 @@ -7,7 +7,6 @@ import ca.uhn.fhir.rest.api.RequestTypeEnum; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; -import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor.Verdict; import ca.uhn.fhir.util.BundleUtil; @@ -38,9 +37,9 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank; * Licensed under the Apache License, Version 2.0 (the "License"); * 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 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -198,7 +197,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { for (BundleEntryParts nextPart : inputResources) { IBaseResource inputResource = nextPart.getResource(); - RestOperationTypeEnum operation = null; + RestOperationTypeEnum operation; if (nextPart.getRequestType() == RequestTypeEnum.GET) { continue; } else { @@ -208,18 +207,22 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { operation = RestOperationTypeEnum.CREATE; } else if (nextPart.getRequestType() == RequestTypeEnum.PUT) { operation = RestOperationTypeEnum.UPDATE; + } else if (nextPart.getRequestType() == RequestTypeEnum.DELETE) { + operation = RestOperationTypeEnum.DELETE; } else { throw new InvalidRequestException("Can not handle transaction with operation of type " + nextPart.getRequestType()); } /* * This is basically just being conservative - Be careful of transactions containing - * nested operations and nested transactions. We block the by default. At some point + * nested operations and nested transactions. We block them by default. At some point * it would be nice to be more nuanced here. */ - RuntimeResourceDefinition resourceDef = ctx.getResourceDefinition(nextPart.getResource()); - if ("Parameters".equals(resourceDef.getName()) || "Bundle".equals(resourceDef.getName())) { - throw new InvalidRequestException("Can not handle transaction with nested resource of type " + resourceDef.getName()); + if (nextPart.getResource() != null) { + RuntimeResourceDefinition resourceDef = ctx.getResourceDefinition(nextPart.getResource()); + if ("Parameters".equals(resourceDef.getName()) || "Bundle".equals(resourceDef.getName())) { + throw new InvalidRequestException("Can not handle transaction with nested resource of type " + resourceDef.getName()); + } } Verdict newVerdict = theRuleApplier.applyRulesAndReturnDecision(operation, theRequestDetails, inputResource, null, null); diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorDstu3Test.java index bebc8140e92..603194a0e35 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorDstu3Test.java @@ -4,8 +4,8 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.context.api.AddProfileTagEnum; import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.annotation.*; -import ca.uhn.fhir.rest.api.*; import ca.uhn.fhir.rest.api.Constants; +import ca.uhn.fhir.rest.api.*; import ca.uhn.fhir.rest.api.server.IRequestOperationCallback; import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.param.ReferenceParam; @@ -46,7 +46,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; import static org.apache.commons.lang3.StringUtils.isNotBlank; import static org.hamcrest.Matchers.containsString; @@ -62,6 +61,7 @@ public class AuthorizationInterceptorDstu3Test { private static boolean ourHitMethod; private static int ourPort; private static List ourReturn; + private static List ourDeleted; private static Server ourServer; private static RestfulServer ourServlet; @@ -73,6 +73,7 @@ public class AuthorizationInterceptorDstu3Test { } ourServlet.setTenantIdentificationStrategy(null); ourReturn = null; + ourDeleted = null; ourHitMethod = false; ourConditionalCreateId = "1123"; } @@ -598,6 +599,75 @@ public class AuthorizationInterceptorDstu3Test { assertTrue(ourHitMethod); } + @Test + public void testDeleteByCompartmentUsingTransaction() throws Exception { + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("Rule 1").delete().resourcesOfType(Patient.class).inCompartment("Patient", new IdType("Patient/1")).andThen() + .allow("Rule 2").delete().resourcesOfType(Observation.class).inCompartment("Patient", new IdType("Patient/1")).andThen() + .allow().transaction().withAnyOperation().andApplyNormalRules().andThen() + .build(); + } + }); + + HttpPost httpPost; + HttpResponse status; + String responseString; + + Bundle responseBundle = new Bundle(); + responseBundle.setType(Bundle.BundleType.TRANSACTIONRESPONSE); + + ourHitMethod = false; + Bundle bundle = new Bundle(); + 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))); + status = ourClient.execute(httpPost); + responseString = extractResponseAndClose(status); + assertEquals(responseString,403, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + bundle.getEntry().clear(); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl("Patient/1"); + ourReturn = Collections.singletonList(responseBundle); + ourDeleted = Collections.singletonList(createPatient(1)); + httpPost = new HttpPost("http://localhost:" + ourPort + "/"); + httpPost.setEntity(new StringEntity(ourCtx.newJsonParser().encodeResourceToString(bundle), ContentType.create(Constants.CT_FHIR_JSON_NEW, Charsets.UTF_8))); + status = ourClient.execute(httpPost); + responseString = extractResponseAndClose(status); + assertEquals(responseString,200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + ourHitMethod = false; + bundle.getEntry().clear(); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl("Observation?subject=Patient/2"); + ourReturn = Collections.singletonList(responseBundle); + ourDeleted = Collections.singletonList(createObservation(99, "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))); + status = ourClient.execute(httpPost); + responseString = extractResponseAndClose(status); + assertEquals(responseString,403, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + ourHitMethod = false; + bundle.getEntry().clear(); + bundle.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl("Observation?subject=Patient/1"); + ourReturn = Collections.singletonList(responseBundle); + ourDeleted = Collections.singletonList(createObservation(99, "Patient/1")); + httpPost = new HttpPost("http://localhost:" + ourPort + "/"); + httpPost.setEntity(new StringEntity(ourCtx.newJsonParser().encodeResourceToString(bundle), ContentType.create(Constants.CT_FHIR_JSON_NEW, Charsets.UTF_8))); + status = ourClient.execute(httpPost); + responseString = extractResponseAndClose(status); + assertEquals(responseString,200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + } + /** * #528 */ @@ -3519,11 +3589,18 @@ public class AuthorizationInterceptorDstu3Test { } @Transaction() - public Bundle search(@TransactionParam Bundle theInput) { + public Bundle search(IRequestOperationCallback theRequestOperationCallback, @TransactionParam Bundle theInput) { ourHitMethod = true; + if (ourDeleted != null){ + for (IBaseResource next : ourDeleted) { + theRequestOperationCallback.resourceDeleted(next); + } + } return (Bundle) ourReturn.get(0); } } + + } diff --git a/src/changes/changes.xml b/src/changes/changes.xml index a90b3ebf41f..1f3ddcc66f4 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -247,6 +247,10 @@ AuthorizationInterceptor now rejects transactions with an invalid or unset request using an HTTP 422 response Bundle type instead of silently refusing to authorize them. + + AuthorizationInterceptor is now able to authorize DELETE operations performed via a + transaction operation. Previously these were always denied. +