Make diff operation work securely (#1859)

* Make diff operation work securely

* Build tweak

* FIx tests
This commit is contained in:
James Agnew 2020-05-21 19:20:48 -04:00 committed by GitHub
parent ce5ade53d9
commit 63ff0ede9c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
18 changed files with 267 additions and 35 deletions

View File

@ -31,7 +31,7 @@ jobs:
inputs:
goals: 'clean install'
# These are Maven CLI options (and show up in the build logs) - "-nsu"=Don't update snapshots. We can remove this when Maven OSS is more healthy
options: '-P ALLMODULES,JACOCO,CI,ERRORPRONE -nsu -e -B -Dmaven.repo.local=$(MAVEN_CACHE_FOLDER)'
options: '-P ALLMODULES,JACOCO,CI,ERRORPRONE -e -B -Dmaven.repo.local=$(MAVEN_CACHE_FOLDER)'
# These are JVM options (and don't show up in the build logs)
mavenOptions: '-Xmx1024m $(MAVEN_OPTS) -Dorg.slf4j.simpleLogger.showDateTime=true -Dorg.slf4j.simpleLogger.dateTimeFormat=HH:mm:ss,SSS -Duser.timezone=America/Toronto'
jdkVersionOption: 1.11

View File

@ -1,5 +1,25 @@
package ca.uhn.fhir.context;
/*-
* #%L
* HAPI FHIR - Core Library
* %%
* Copyright (C) 2014 - 2020 University Health Network
* %%
* 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.
* See the License for the specific language governing permissions and
* limitations under the License.
* #L%
*/
import com.google.common.collect.Sets;
import org.apache.commons.lang3.Validate;
import org.hl7.fhir.instance.model.api.IBase;

View File

@ -1,5 +1,25 @@
package ca.uhn.fhir.parser.path;
/*-
* #%L
* HAPI FHIR - Core Library
* %%
* Copyright (C) 2014 - 2020 University Health Network
* %%
* 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.
* See the License for the specific language governing permissions and
* limitations under the License.
* #L%
*/
import org.apache.commons.lang3.Validate;
import java.util.ArrayList;

View File

@ -1,5 +1,25 @@
package ca.uhn.fhir.parser.path;
/*-
* #%L
* HAPI FHIR - Core Library
* %%
* Copyright (C) 2014 - 2020 University Health Network
* %%
* 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.
* See the License for the specific language governing permissions and
* limitations under the License.
* #L%
*/
import org.apache.commons.lang3.Validate;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;

View File

@ -51,4 +51,13 @@ public interface IBaseOn<T> {
*/
T onInstance(IIdType theId);
/**
* Perform the operation across all versions of a specific resource (by ID and type) on the server.
* Note that <code>theId</code> must be populated with both a resource type and a resource ID at
* a minimum.
*
* @throws IllegalArgumentException If <code>theId</code> does not contain at least a resource type and ID
*/
T onInstance(String theId);
}

View File

@ -61,4 +61,5 @@ public interface IOperation extends IBaseOn<IOperationUnnamed> {
* using FHIR Resources</a>
*/
IOperationProcessMsg processMessage();
}

View File

@ -931,6 +931,14 @@ public class GenericClient extends BaseClient implements IGenericClient {
return this;
}
@Override
public IHistoryUntyped onInstance(String theId) {
Validate.notBlank(theId, "theId must not be null or blank");
IIdType id = myContext.getVersion().newIdType();
id.setValue(theId);
return onInstance(id);
}
@Override
public IHistoryUntyped onServer() {
return this;
@ -1297,7 +1305,7 @@ public class GenericClient extends BaseClient implements IGenericClient {
return retVal;
}
IClientResponseHandler handler = new ResourceOrBinaryResponseHandler()
.setPreferResponseTypes(getPreferResponseTypes(myType));
.setPreferResponseTypes(getPreferResponseTypes(myType));
if (myReturnMethodOutcome) {
handler = new MethodOutcomeResponseHandler(handler);
@ -1339,6 +1347,14 @@ public class GenericClient extends BaseClient implements IGenericClient {
return this;
}
@Override
public IOperationUnnamed onInstance(String theId) {
Validate.notBlank(theId, "theId must not be null or blank");
IIdType id = myContext.getVersion().newIdType();
id.setValue(theId);
return onInstance(id);
}
@Override
public IOperationUnnamed onInstanceVersion(IIdType theId) {
myId = theId;
@ -1478,7 +1494,7 @@ public class GenericClient extends BaseClient implements IGenericClient {
}
private final class MethodOutcomeResponseHandler implements IClientResponseHandler<MethodOutcome> {
private final class MethodOutcomeResponseHandler implements IClientResponseHandler<MethodOutcome> {
private final IClientResponseHandler<? extends IBaseResource> myWrap;
private MethodOutcomeResponseHandler(IClientResponseHandler<? extends IBaseResource> theWrap) {

View File

@ -1,5 +1,25 @@
package ca.uhn.fhir.jpa.patch;
/*-
* #%L
* HAPI FHIR JPA Server
* %%
* Copyright (C) 2014 - 2020 University Health Network
* %%
* 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.
* See the License for the specific language governing permissions and
* limitations under the License.
* #L%
*/
import ca.uhn.fhir.context.BaseRuntimeChildDefinition;
import ca.uhn.fhir.context.BaseRuntimeElementCompositeDefinition;
import ca.uhn.fhir.context.BaseRuntimeElementDefinition;

View File

@ -1,5 +1,25 @@
package ca.uhn.fhir.jpa.provider;
/*-
* #%L
* HAPI FHIR JPA Server
* %%
* Copyright (C) 2014 - 2020 University Health Network
* %%
* 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.
* See the License for the specific language governing permissions and
* limitations under the License.
* #L%
*/
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;

View File

@ -1,6 +1,7 @@
package ca.uhn.fhir.jpa.provider.r4;
import ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor;
import ca.uhn.fhir.jpa.model.util.ProviderConstants;
import ca.uhn.fhir.model.primitive.IdDt;
import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.api.MethodOutcome;
@ -33,9 +34,11 @@ import org.hl7.fhir.r4.model.Identifier;
import org.hl7.fhir.r4.model.Observation;
import org.hl7.fhir.r4.model.Observation.ObservationStatus;
import org.hl7.fhir.r4.model.Organization;
import org.hl7.fhir.r4.model.Parameters;
import org.hl7.fhir.r4.model.Patient;
import org.hl7.fhir.r4.model.Practitioner;
import org.hl7.fhir.r4.model.Reference;
import org.hl7.fhir.r4.model.StringType;
import org.junit.AfterClass;
import org.junit.Test;
import org.slf4j.Logger;
@ -181,8 +184,8 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
return patient
.getIdentifier()
.stream()
.filter(t-> "http://uhn.ca/mrns".equals(t.getSystem()))
.anyMatch(t-> "100".equals(t.getValue()));
.filter(t -> "http://uhn.ca/mrns".equals(t.getSystem()))
.anyMatch(t -> "100".equals(t.getValue()));
}
return false;
}
@ -730,6 +733,87 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
}
@Test
public void testDiffOperation_AllowedByType_Instance() {
createPatient(withId("A"), withActiveTrue());
createPatient(withId("A"), withActiveFalse());
createObservation(withId("B"), withStatus("final"));
ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onAnyInstance().andAllowAllResponses().andThen()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onServer().andAllowAllResponses().andThen()
.allow().read().resourcesOfType(Patient.class).withAnyId().andThen()
.denyAll()
.build();
}
});
Parameters diff;
diff = ourClient.operation().onInstance("Patient/A").named(ProviderConstants.DIFF_OPERATION_NAME).withNoParameters(Parameters.class).execute();
assertEquals(1, diff.getParameter().size());
diff = ourClient.operation().onInstanceVersion(new IdType("Patient/A/_history/2")).named(ProviderConstants.DIFF_OPERATION_NAME).withNoParameters(Parameters.class).execute();
assertEquals(1, diff.getParameter().size());
try {
ourClient.operation().onInstance("Observation/B").named(ProviderConstants.DIFF_OPERATION_NAME).withNoParameters(Parameters.class).execute();
fail();
} catch (ForbiddenOperationException e) {
// good
}
}
@Test
public void testDiffOperation_AllowedByType_Server() {
createPatient(withId("A"), withActiveTrue());
createPatient(withId("B"), withActiveFalse());
createObservation(withId("C"), withStatus("final"));
createObservation(withId("D"), withStatus("amended"));
ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
return new RuleBuilder()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onAnyInstance().andAllowAllResponses().andThen()
.allow().operation().named(ProviderConstants.DIFF_OPERATION_NAME).onServer().andAllowAllResponses().andThen()
.allow().read().resourcesOfType(Patient.class).withAnyId().andThen()
.denyAll()
.build();
}
});
Parameters diff;
diff = ourClient
.operation()
.onServer()
.named(ProviderConstants.DIFF_OPERATION_NAME)
.withParameter(Parameters.class, ProviderConstants.DIFF_FROM_PARAMETER, new StringType("Patient/A"))
.andParameter( ProviderConstants.DIFF_TO_PARAMETER, new StringType("Patient/B"))
.execute();
assertEquals(2, diff.getParameter().size());
try {
ourClient
.operation()
.onServer()
.named(ProviderConstants.DIFF_OPERATION_NAME)
.withParameter(Parameters.class, ProviderConstants.DIFF_FROM_PARAMETER, new StringType("Observation/C"))
.andParameter( ProviderConstants.DIFF_TO_PARAMETER, new StringType("Observation/D"))
.execute();
fail();
} catch (ForbiddenOperationException e) {
// good
}
}
/**
* See #762
*/

View File

@ -2487,7 +2487,7 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test {
ourLog.info("Res ID: {}", id);
Bundle history = ourClient.history().onInstance(id).andReturnBundle(Bundle.class).prettyPrint().summaryMode(SummaryEnum.DATA).execute();
Bundle history = ourClient.history().onInstance(id.getValue()).andReturnBundle(Bundle.class).prettyPrint().summaryMode(SummaryEnum.DATA).execute();
assertEquals(3, history.getEntry().size());
assertEquals(id.withVersion("3").getValue(), history.getEntry().get(0).getResource().getId());
assertEquals(1, ((Patient) history.getEntry().get(0).getResource()).getName().size());

View File

@ -120,8 +120,7 @@ public class AuthorizationInterceptor implements IRuleApplier {
Verdict verdict = null;
for (IAuthRule nextRule : rules) {
ourLog.trace("Rule being applied - {}",
nextRule);
ourLog.trace("Rule being applied - {}", nextRule);
verdict = nextRule.applyRule(theOperation, theRequestDetails, theInputResource, theInputResourceId, theOutputResource, this, flags, thePointcut);
if (verdict != null) {
ourLog.trace("Rule {} returned decision {}", nextRule, verdict.getDecision());

View File

@ -84,6 +84,14 @@ class OperationRule extends BaseRule implements IAuthRule {
public Verdict applyRule(RestOperationTypeEnum theOperation, RequestDetails theRequestDetails, IBaseResource theInputResource, IIdType theInputResourceId, IBaseResource theOutputResource, IRuleApplier theRuleApplier, Set<AuthorizationFlagsEnum> theFlags, Pointcut thePointcut) {
FhirContext ctx = theRequestDetails.getServer().getFhirContext();
// Operation rules apply to the execution of the operation itself, not to side effects like
// loading resources (that will presumably be reflected in the response). Those loads need
// to be explicitly authorized
boolean isResourceAccess = thePointcut.equals(Pointcut.STORAGE_PREACCESS_RESOURCES) || thePointcut.equals(Pointcut.STORAGE_PRESHOW_RESOURCES);
if (isResourceAccess) {
return null;
}
boolean applies = false;
switch (theOperation) {
case ADD_TAGS:
@ -126,7 +134,7 @@ class OperationRule extends BaseRule implements IAuthRule {
}
if (requestResourceId != null) {
if (myAppliesToIds != null) {
String instanceId = requestResourceId .toUnqualifiedVersionless().getValue();
String instanceId = requestResourceId.toUnqualifiedVersionless().getValue();
for (IIdType next : myAppliesToIds) {
if (next.toUnqualifiedVersionless().getValue().equals(instanceId)) {
applies = true;
@ -138,7 +146,7 @@ class OperationRule extends BaseRule implements IAuthRule {
// TODO: Convert to a map of strings and keep the result
for (Class<? extends IBaseResource> next : myAppliesToInstancesOfType) {
String resName = ctx.getResourceDefinition(next).getName();
if (resName.equals(requestResourceId .getResourceType())) {
if (resName.equals(requestResourceId.getResourceType())) {
applies = true;
break;
}

View File

@ -102,11 +102,6 @@ public final class HapiWorkerContext extends I18nBase implements IWorkerContext
throw new UnsupportedOperationException();
}
@Override
public org.hl7.fhir.r5.utils.INarrativeGenerator getNarrativeGenerator(String thePrefix, String theBasePath) {
throw new UnsupportedOperationException();
}
@Override
public IParser getParser(ParserType theType) {
throw new UnsupportedOperationException();

View File

@ -5,7 +5,6 @@ import ca.uhn.fhir.context.FhirVersionEnum;
import ca.uhn.fhir.context.support.ConceptValidationOptions;
import ca.uhn.fhir.context.support.IValidationSupport;
import ca.uhn.fhir.util.ClasspathUtil;
import ca.uhn.fhir.util.FileUtil;
import org.apache.commons.lang3.Validate;
import org.fhir.ucum.UcumEssenceService;
import org.fhir.ucum.UcumException;
@ -15,12 +14,15 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import javax.annotation.Nonnull;
import java.io.IOException;
import javax.annotation.Nullable;
import java.io.InputStream;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import static org.apache.commons.lang3.StringUtils.defaultString;
import static org.apache.commons.lang3.StringUtils.isBlank;
/**
* This {@link IValidationSupport validation support module} can be used to validate codes against common
* CodeSystems that are commonly used, but are not distriuted with the FHIR specification for various reasons
@ -36,10 +38,10 @@ public class CommonCodeSystemsTerminologyService implements IValidationSupport {
public static final String CURRENCIES_CODESYSTEM_URL = "urn:iso:std:iso:4217";
public static final String CURRENCIES_VALUESET_URL = "http://hl7.org/fhir/ValueSet/currencies";
public static final String UCUM_CODESYSTEM_URL = "http://unitsofmeasure.org";
public static final String UCUM_VALUESET_URL = "http://hl7.org/fhir/ValueSet/ucum-units";
private static final String USPS_CODESYSTEM_URL = "https://www.usps.com/";
private static final String USPS_VALUESET_URL = "http://hl7.org/fhir/us/core/ValueSet/us-core-usps-state";
private static final Logger ourLog = LoggerFactory.getLogger(CommonCodeSystemsTerminologyService.class);
public static final String UCUM_VALUESET_URL = "http://hl7.org/fhir/ValueSet/ucum-units";
private static Map<String, String> USPS_CODES = Collections.unmodifiableMap(buildUspsCodes());
private static Map<String, String> ISO_4217_CODES = Collections.unmodifiableMap(buildIso4217Codes());
private final FhirContext myFhirContext;
@ -56,7 +58,11 @@ public class CommonCodeSystemsTerminologyService implements IValidationSupport {
@Override
public CodeValidationResult validateCodeInValueSet(IValidationSupport theRootValidationSupport, ConceptValidationOptions theOptions, String theCodeSystem, String theCode, String theDisplay, @Nonnull IBaseResource theValueSet) {
String url = getValueSetUrl(theValueSet);
return validateCode(theRootValidationSupport, theOptions, theCodeSystem, theCode, theDisplay, url);
}
@Override
public CodeValidationResult validateCode(IValidationSupport theRootValidationSupport, ConceptValidationOptions theOptions, String theCodeSystem, String theCode, String theDisplay, String theValueSetUrl) {
/* **************************************************************************************
* NOTE: Update validation_support_modules.html if any of the support in this module
* changes in any way!
@ -64,7 +70,7 @@ public class CommonCodeSystemsTerminologyService implements IValidationSupport {
Map<String, String> handlerMap = null;
String expectSystem = null;
switch (url) {
switch (defaultString(theValueSetUrl)) {
case USPS_VALUESET_URL:
handlerMap = USPS_CODES;
expectSystem = USPS_CODESYSTEM_URL;
@ -87,13 +93,9 @@ public class CommonCodeSystemsTerminologyService implements IValidationSupport {
if (system == null && theOptions.isInferSystem()) {
system = UCUM_CODESYSTEM_URL;
}
LookupCodeResult lookupResult = lookupCode(theRootValidationSupport, system, theCode);
if (lookupResult != null) {
if (lookupResult.isFound()) {
return new CodeValidationResult()
.setCode(lookupResult.getSearchedForCode())
.setDisplay(lookupResult.getCodeDisplay());
}
CodeValidationResult validationResult = validateLookupCode(theRootValidationSupport, theCode, system);
if (validationResult != null) {
return validationResult;
}
}
}
@ -113,9 +115,31 @@ public class CommonCodeSystemsTerminologyService implements IValidationSupport {
.setMessage("Code \"" + theCode + "\" is not in system: " + USPS_CODESYSTEM_URL);
}
if (isBlank(theValueSetUrl)) {
CodeValidationResult validationResult = validateLookupCode(theRootValidationSupport, theCode, theCodeSystem);
if (validationResult != null) {
return validationResult;
}
}
return null;
}
@Nullable
public CodeValidationResult validateLookupCode(IValidationSupport theRootValidationSupport, String theCode, String theSystem) {
LookupCodeResult lookupResult = lookupCode(theRootValidationSupport, theSystem, theCode);
CodeValidationResult validationResult = null;
if (lookupResult != null) {
if (lookupResult.isFound()) {
validationResult = new CodeValidationResult()
.setCode(lookupResult.getSearchedForCode())
.setDisplay(lookupResult.getCodeDisplay());
}
}
return validationResult;
}
@Override
public LookupCodeResult lookupCode(IValidationSupport theRootValidationSupport, String theSystem, String theCode) {

View File

@ -26,7 +26,6 @@ import org.hl7.fhir.r5.model.Resource;
import org.hl7.fhir.r5.model.StructureDefinition;
import org.hl7.fhir.r5.model.ValueSet;
import org.hl7.fhir.r5.terminologies.ValueSetExpander;
import org.hl7.fhir.r5.utils.INarrativeGenerator;
import org.hl7.fhir.r5.utils.IResourceValidator;
import org.hl7.fhir.utilities.TranslationServices;
import org.hl7.fhir.utilities.cache.NpmPackage;
@ -323,11 +322,6 @@ class VersionSpecificWorkerContextWrapper extends I18nBase implements IWorkerCon
throw new UnsupportedOperationException();
}
@Override
public INarrativeGenerator getNarrativeGenerator(String prefix, String basePath) {
throw new UnsupportedOperationException();
}
@Override
public IParser getParser(ParserType type) {
throw new UnsupportedOperationException();

View File

@ -1360,8 +1360,9 @@ public class FhirInstanceValidatorR4Test extends BaseTest {
input.getValueQuantity().setCode("Heck");
output = myVal.validateWithResult(input);
all = logResultsAndReturnNonInformationalOnes(output);
assertEquals(1, all.size());
assertThat(all.get(0).getMessage(), containsString("The value provided (\"Heck\") is not in the value set http://hl7.org/fhir/ValueSet/ucum-bodytemp"));
assertEquals(2, all.size());
assertThat(all.get(0).getMessage(), containsString("Validation failed for \"http://unitsofmeasure.org#Heck\""));
assertThat(all.get(1).getMessage(), containsString("The value provided (\"Heck\") is not in the value set http://hl7.org/fhir/ValueSet/ucum-bodytemp"));
}

View File

@ -22,7 +22,8 @@
}
],
"sourceAttachment": {
"title": "The terms of the consent in lawyer speak."
"title": "The terms of the consent in lawyer speak.",
"url": "http://foo"
},
"identifier": [
{