From 3d94761bcb631aea501b15da6008d2f19dedb358 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Sun, 6 Jan 2019 18:08:32 -0500 Subject: [PATCH] Improve response for transactions --- .../ca/uhn/fhir/i18n/hapi-messages.properties | 3 + .../server/interceptor/auth/RuleImplOp.java | 23 +++--- .../AuthorizationInterceptorDstu3Test.java | 80 +++++++++++++++++++ src/changes/changes.xml | 4 + 4 files changed, 101 insertions(+), 9 deletions(-) diff --git a/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties b/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties index c3c435c85b1..356532eb5a6 100644 --- a/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties +++ b/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties @@ -11,6 +11,7 @@ ca.uhn.fhir.context.RuntimeResourceDefinition.typeWrongVersion=This context is f ca.uhn.fhir.rest.client.impl.BaseClient.ioExceptionDuringOperation=Encountered IOException when performing {0} to URL {1} - {2} ca.uhn.fhir.rest.client.impl.BaseClient.failedToParseResponse=Failed to parse response from server when performing {0} to URL {1} - {2} + ca.uhn.fhir.rest.client.impl.GenericClient.cantDetermineRequestType=Unable to determing encoding of request (body does not appear to be valid XML or JSON) ca.uhn.fhir.rest.client.impl.GenericClient.noPagingLinkFoundInBundle=Can not perform paging operation because no link was found in Bundle with relation "{0}" ca.uhn.fhir.rest.client.impl.GenericClient.noVersionIdForVread=No version specified in URL for 'vread' operation: {0} @@ -19,6 +20,8 @@ ca.uhn.fhir.rest.client.impl.GenericClient.cannotDetermineResourceTypeFromUri=Un ca.uhn.fhir.rest.client.impl.RestfulClientFactory.failedToRetrieveConformance=Failed to retrieve the server metadata statement during client initialization. URL used was {0} ca.uhn.fhir.rest.client.impl.RestfulClientFactory.wrongVersionInConformance=The server at base URL "{0}" returned a conformance statement indicating that it supports FHIR version "{1}" which corresponds to {2}, but this client is configured to use {3} (via the FhirContext). +ca.uhn.fhir.rest.server.interceptor.auth.RuleImplOp.invalidRequestBundleTypeForTransaction=Invalid request Bundle.type value for transaction: {0} + ca.uhn.fhir.rest.server.method.BaseOutcomeReturningMethodBindingWithResourceParam.incorrectIdForUpdate=Can not update resource, resource body must contain an ID element which matches the request URL for update (PUT) operation - Resource body ID of "{0}" does not match URL ID of "{1}" ca.uhn.fhir.rest.server.method.BaseOutcomeReturningMethodBindingWithResourceParam.noIdInBodyForUpdate=Can not update resource, resource body must contain an ID element for update (PUT) operation ca.uhn.fhir.rest.server.method.BaseOutcomeReturningMethodBindingWithResourceParam.noIdInUrlForUpdate=Can not update resource, request URL must contain an ID element for update (PUT) operation (it must be of the form [base]/[resource type]/[id]) 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 aea34e35773..6d805304ab5 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,6 +7,8 @@ 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; import ca.uhn.fhir.util.BundleUtil.BundleEntryParts; @@ -24,6 +26,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import static org.apache.commons.lang3.StringUtils.defaultString; import static org.apache.commons.lang3.StringUtils.isNotBlank; /* @@ -35,9 +38,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. @@ -466,16 +469,18 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { } IBaseBundle request = (IBaseBundle) theInputResource; - String bundleType = BundleUtil.getBundleType(theContext, request); + String bundleType = defaultString(BundleUtil.getBundleType(theContext, request)); //noinspection EnumSwitchStatementWhichMissesCases - switch (theOp) { - case TRANSACTION: - return "transaction".equals(bundleType) - || "batch".equals(bundleType); - default: - return false; + if (theOp == RuleOpEnum.TRANSACTION) { + if ("transaction".equals(bundleType) || "batch".equals(bundleType)) { + return true; + } else { + String msg = theContext.getLocalizer().getMessage(RuleImplOp.class, "invalidRequestBundleTypeForTransaction", '"' + bundleType + '"'); + throw new UnprocessableEntityException(msg); + } } + return false; } public void setAppliesTo(AppliesTypeEnum theAppliesTo) { 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 76f3421b592..dec9892b22e 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 @@ -46,6 +46,7 @@ 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; @@ -2525,6 +2526,85 @@ public class AuthorizationInterceptorDstu3Test { } + @Test + public void testTransactionWithSearch() throws IOException { + + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("transactions").transaction().withAnyOperation().andApplyNormalRules().andThen() + .allow("read patient").read().resourcesOfType(Patient.class).withAnyId().andThen() + .denyAll("deny all") + .build(); + } + }); + + // Request is a transaction with 1 search + Bundle requestBundle = new Bundle(); + requestBundle.setType(Bundle.BundleType.TRANSACTION); + String patientId = "10000003857"; + Bundle.BundleEntryComponent bundleEntryComponent = requestBundle.addEntry(); + Bundle.BundleEntryRequestComponent bundleEntryRequestComponent = new Bundle.BundleEntryRequestComponent(); + bundleEntryRequestComponent.setMethod(Bundle.HTTPVerb.GET); + bundleEntryRequestComponent.setUrl(ResourceType.Patient + "?identifier=" + patientId); + bundleEntryComponent.setRequest(bundleEntryRequestComponent); + + /* + * Response is a transaction response containing the search results + */ + Bundle searchResponseBundle = new Bundle(); + Patient patent = new Patient(); + patent.setActive(true); + patent.setId("Patient/123"); + searchResponseBundle.addEntry().setResource(patent); + + Bundle responseBundle = new Bundle(); + responseBundle + .addEntry() + .setResource(searchResponseBundle); + ourReturn = Collections.singletonList(responseBundle); + + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/"); + httpPost.setEntity(createFhirResourceEntity(requestBundle)); + CloseableHttpResponse status = ourClient.execute(httpPost); + String resp = extractResponseAndClose(status); + assertEquals(200, status.getStatusLine().getStatusCode()); + + } + + @Test + public void testTransactionWithNoBundleType() throws IOException { + + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("transactions").transaction().withAnyOperation().andApplyNormalRules().andThen() + .allow("read patient").read().resourcesOfType(Patient.class).withAnyId().andThen() + .denyAll("deny all") + .build(); + } + }); + + // Request is a transaction with 1 search + Bundle requestBundle = new Bundle(); + String patientId = "10000003857"; + Bundle.BundleEntryComponent bundleEntryComponent = requestBundle.addEntry(); + Bundle.BundleEntryRequestComponent bundleEntryRequestComponent = new Bundle.BundleEntryRequestComponent(); + bundleEntryRequestComponent.setMethod(Bundle.HTTPVerb.GET); + bundleEntryRequestComponent.setUrl(ResourceType.Patient + "?identifier=" + patientId); + bundleEntryComponent.setRequest(bundleEntryRequestComponent); + + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/"); + httpPost.setEntity(createFhirResourceEntity(requestBundle)); + CloseableHttpResponse status = ourClient.execute(httpPost); + String resp = extractResponseAndClose(status); + assertEquals(422, status.getStatusLine().getStatusCode()); + assertThat(resp, containsString("Invalid request Bundle.type value for transaction: \\\"\\\"")); + + } + /** * See #762 */ diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 5a2c5d77d6f..a90b3ebf41f 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -243,6 +243,10 @@ new setting on the DaoConfig. This can be used to force a total to always be calculated for searches, including large ones. + + AuthorizationInterceptor now rejects transactions with an invalid or unset request + using an HTTP 422 response Bundle type instead of silently refusing to authorize them. +