More authorizationinterceptor tweaks for better security

hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeResourceDefinition.java

@@ -185,14 +185,29 @@ public class RuntimeResourceDefinition extends BaseRuntimeElementCompositeDefini
 		});
 		mySearchParams = Collections.unmodifiableList(searchParams);
 
-		Map<String, List<RuntimeSearchParam>> compartmentNameToSearchParams = new HashMap<String, List<RuntimeSearchParam>>();
+		Map<String, List<RuntimeSearchParam>> compartmentNameToSearchParams = new HashMap<>();
 		for (RuntimeSearchParam next : searchParams) {
 			if (next.getProvidesMembershipInCompartments() != null) {
 				for (String nextCompartment : next.getProvidesMembershipInCompartments()) {
 					if (!compartmentNameToSearchParams.containsKey(nextCompartment)) {
-						compartmentNameToSearchParams.put(nextCompartment, new ArrayList<RuntimeSearchParam>());
+						compartmentNameToSearchParams.put(nextCompartment, new ArrayList<>());
+					}
+					List<RuntimeSearchParam> searchParamsForCompartment = compartmentNameToSearchParams.get(nextCompartment);
+					searchParamsForCompartment.add(next);
+
+					/*
+					 * If one search parameter marks an SP as making a resource
+					 * a part of a compartment, let's also denote all other
+					 * SPs with the same path the same way. This behaviour is
+					 * used by AuthorizationInterceptor
+					 */
+					for (RuntimeSearchParam nextAlternate : searchParams) {
+						if (nextAlternate.getPath().equals(next.getPath())) {
+							if (!nextAlternate.getName().equals(next.getName())) {
+								searchParamsForCompartment.add(nextAlternate);
+							}
+						}
 					}
-					compartmentNameToSearchParams.get(nextCompartment).add(next);
 				}
 			}
 		}

hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeSearchParam.java

@@ -5,6 +5,10 @@ import static org.apache.commons.lang3.StringUtils.trim;
 
 import java.util.*;
 
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.commons.lang3.builder.ToStringStyle;
 import org.hl7.fhir.instance.model.api.IIdType;
 
 import ca.uhn.fhir.rest.api.RestSearchParameterTypeEnum;
@@ -38,6 +42,18 @@ public class RuntimeSearchParam {
 	private final RestSearchParameterTypeEnum myParamType;
 	private final String myPath;
 	private final Set<String> myTargets;
+
+	@Override
+	public String toString() {
+		return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
+			.append("base", myBase)
+			.append("name", myName)
+			.append("path", myPath)
+			.append("id", myId)
+			.append("uri", myUri)
+			.toString();
+	}
+
 	private final Set<String> myProvidesMembershipInCompartments;
 	private final RuntimeSearchParamStatusEnum myStatus;
 	private final String myUri;
@@ -55,9 +71,36 @@ public class RuntimeSearchParam {
 		this(theId, theUri, theName, theDescription, thePath, theParamType, theCompositeOf, theProvidesMembershipInCompartments, theTargets, theStatus, null);
 	}
 
+	@Override
+	public boolean equals(Object theO) {
+		if (this == theO) return true;
+
+		if (theO == null || getClass() != theO.getClass()) return false;
+
+		RuntimeSearchParam that = (RuntimeSearchParam) theO;
+
+		return new EqualsBuilder()
+			.append(getId(), that.getId())
+			.append(getName(), that.getName())
+			.append(getPath(), that.getPath())
+			.append(getUri(), that.getUri())
+			.isEquals();
+	}
+
+	@Override
+	public int hashCode() {
+		return new HashCodeBuilder(17, 37)
+			.append(getId())
+			.append(getName())
+			.append(getPath())
+			.append(getUri())
+			.toHashCode();
+	}
+
 	public RuntimeSearchParam(IIdType theId, String theUri, String theName, String theDescription, String thePath, RestSearchParameterTypeEnum theParamType, List<RuntimeSearchParam> theCompositeOf,
-			Set<String> theProvidesMembershipInCompartments, Set<String> theTargets, RuntimeSearchParamStatusEnum theStatus, Collection<String> theBase) {
+									  Set<String> theProvidesMembershipInCompartments, Set<String> theTargets, RuntimeSearchParamStatusEnum theStatus, Collection<String> theBase) {
 		super();
+
 		myId = theId;
 		myUri = theUri;
 		myName = theName;

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleImplOp.java

@@ -14,10 +14,12 @@ import ca.uhn.fhir.util.FhirTerser;
 import org.apache.commons.codec.binary.StringUtils;
 import org.apache.commons.lang3.builder.ToStringBuilder;
 import org.apache.commons.lang3.builder.ToStringStyle;
+import org.hl7.fhir.instance.model.api.IAnyResource;
 import org.hl7.fhir.instance.model.api.IBaseBundle;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 
+import javax.annotation.Nullable;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
@@ -305,14 +307,12 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 						}
 					}
 				}
-//				if (myClassifierType == ClassifierTypeEnum.ANY_ID) {
+				if (appliesToResourceId != null && appliesToResourceId.hasResourceType()) {
-					if (appliesToResourceId != null && appliesToResourceId.hasResourceType()) {
+					Class<? extends IBaseResource> type = theRequestDetails.getServer().getFhirContext().getResourceDefinition(appliesToResourceId.getResourceType()).getImplementingClass();
-						Class<? extends IBaseResource> type = theRequestDetails.getServer().getFhirContext().getResourceDefinition(appliesToResourceId.getResourceType()).getImplementingClass();
+					if (myAppliesToTypes.contains(type) == false) {
-						if (myAppliesToTypes.contains(type) == false) {
+						return null;
-							return null;
-						}
 					}
-//				}
+				}
 				if (appliesToResourceType != null) {
 					Class<? extends IBaseResource> type = theRequestDetails.getServer().getFhirContext().getResourceDefinition(appliesToResourceType).getImplementingClass();
 					if (myAppliesToTypes.contains(type)) {
@@ -351,6 +351,19 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 						}
 					}
 
+					/*
+					 * If the client has permission to read compartment
+					 * Patient/ABC, then a search for Patient?_id=Patient/ABC
+					 * should be permitted. This is kind of a one-off case, but
+					 * it makes sense.
+					 */
+					if (next.getResourceType().equals(appliesToResourceType)) {
+						Verdict verdict = checkForSearchParameterMatchingCompartmentAndReturnSuccessfulVerdictOrNull(appliesToSearchParams, next, IAnyResource.SP_RES_ID);
+						if (verdict != null) {
+							return verdict;
+						}
+					}
+
 					/*
 					 * If we're trying to read a resource that could potentially be
 					 * in the given compartment, we'll let the request through and
@@ -377,13 +390,10 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 								 */
 								if (appliesToSearchParams != null && !theFlags.contains(AuthorizationFlagsEnum.NO_NOT_PROACTIVELY_BLOCK_COMPARTMENT_READ_ACCESS)) {
 									for (RuntimeSearchParam nextRuntimeSearchParam : params) {
-										String[] values = appliesToSearchParams.get(nextRuntimeSearchParam.getName());
+										String name = nextRuntimeSearchParam.getName();
-										if (values != null) {
+										Verdict verdict = checkForSearchParameterMatchingCompartmentAndReturnSuccessfulVerdictOrNull(appliesToSearchParams, next, name);
-											for (String nextParameterValue : values) {
+										if (verdict != null) {
-												if (nextParameterValue.equals(next.getValue())) {
+											return verdict;
-													return new Verdict(PolicyEnum.ALLOW, this);
-												}
-											}
 										}
 									}
 								} else {
@@ -409,6 +419,26 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ {
 		return newVerdict();
 	}
 
+	private Verdict checkForSearchParameterMatchingCompartmentAndReturnSuccessfulVerdictOrNull(Map<String, String[]> theSearchParams, IIdType theCompartmentOwner, String theSearchParamName) {
+		Verdict verdict = null;
+		if (theSearchParams != null) {
+			String[] values = theSearchParams.get(theSearchParamName);
+			if (values != null) {
+				for (String nextParameterValue : values) {
+					if (nextParameterValue.equals(theCompartmentOwner.getValue())) {
+						verdict = new Verdict(PolicyEnum.ALLOW, this);
+						break;
+					}
+					if (nextParameterValue.equals(theCompartmentOwner.getIdPart())) {
+						verdict = new Verdict(PolicyEnum.ALLOW, this);
+						break;
+					}
+				}
+			}
+		}
+		return verdict;
+	}
+
 	public TransactionAppliesToEnum getTransactionAppliesToOp() {
 		return myTransactionAppliesToOp;
 	}

hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/context/FhirContextDstu3Test.java

@@ -32,6 +32,12 @@ public class FhirContextDstu3Test {
 		assertEquals(FhirVersionEnum.DSTU3, ctx.getVersion().getVersion());
 	}
 
+	@Test
+	public void testRuntimeSearchParamToString() {
+		String val = ourCtx.getResourceDefinition("Patient").getSearchParam("gender").toString();
+		assertEquals("RuntimeSearchParam[base=[Patient],name=gender,path=Patient.gender,id=<null>,uri=<null>]", val);
+	}
+
 	@Test
 	public void testCustomTypeDoesntBecomeDefault() {
 		FhirContext ctx = FhirContext.forDstu3();
@@ -69,7 +75,7 @@ public class FhirContextDstu3Test {
 		final FhirContext ctx = FhirContext.forDstu3();
 
 		final int numThreads = 40;
-		final List<Throwable> exceptions = Collections.synchronizedList(new ArrayList<Throwable>());
+		final List<Throwable> exceptions = Collections.synchronizedList(new ArrayList<>());
 		final ExecutorService threadPool = Executors.newFixedThreadPool(numThreads);
 		try {
 			final CountDownLatch threadsReady = new CountDownLatch(numThreads);
@@ -77,19 +83,17 @@ public class FhirContextDstu3Test {
 
 			for (int i = 0; i < numThreads; i++) {
 				threadPool.submit(
-					new Runnable() {
+					() -> {
-						public void run() {
+						threadsReady.countDown();
-							threadsReady.countDown();
+						try {
-							try {
+							threadsReady.await();
-								threadsReady.await();
+							RuntimeResourceDefinition def = ctx.getResourceDefinition("patient");
-								RuntimeResourceDefinition def = ctx.getResourceDefinition("patient");
+							ourLog.info(def.toString());
-								ourLog.info(def.toString());
+							assertNotNull(def);
-								assertNotNull(def);
+						} catch (final Exception e) {
-							} catch (final Exception e) {
+							exceptions.add(e);
-								exceptions.add(e);
-							}
-							threadsFinished.countDown();
 						}
+						threadsFinished.countDown();
 					}
 				);
 			}
@@ -108,18 +112,15 @@ public class FhirContextDstu3Test {
 	 * See #794
 	 */
 	@Test
-	public void testInitializeThreadSafety2() throws InterruptedException {
+	public void testInitializeThreadSafety2() {
 		final FhirContext dstu3FhirContext = FhirContext.forDstu3();
 
 		final AtomicInteger count = new AtomicInteger();
 
 		for (int i = 0; i < 10; i++) {
-			new Thread(new Runnable() {
+			new Thread(() -> {
-				@Override
+				OperationOutcomeUtil.newInstance(dstu3FhirContext);
-				public void run() {
+				ourLog.info("Have finished {}", count.incrementAndGet());
-					OperationOutcomeUtil.newInstance(dstu3FhirContext);
-					ourLog.info("Have finished {}", count.incrementAndGet());
-				}
 			}).start();
 		}
 

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorR4Test.java

@@ -99,6 +99,16 @@ public class AuthorizationInterceptorR4Test {
 		return retVal;
 	}
 
+	private Resource createDiagnosticReport(Integer theId, String theSubjectId) {
+		DiagnosticReport retVal = new DiagnosticReport();
+		if (theId != null) {
+			retVal.setId(new IdType("DiagnosticReport", (long) theId));
+		}
+		retVal.getCode().setText("OBS");
+		retVal.setSubject(new Reference(theSubjectId));
+		return retVal;
+	}
+
 	private Organization createOrganization(int theIndex) {
 		Organization retVal = new Organization();
 		retVal.setId("" + theIndex);
@@ -1806,6 +1816,85 @@ public class AuthorizationInterceptorR4Test {
 
 	}
 
+
+	@Test
+	public void testReadByCompartmentReadByPatientParam() throws Exception {
+		ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
+			@Override
+			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+				return new RuleBuilder()
+					.allow("Rule 1").read().allResources().inCompartment("Patient", new IdType("Patient/1")).andThen()
+					.build();
+			}
+		});
+
+		HttpGet httpGet;
+		HttpResponse status;
+
+		ourReturn = Collections.singletonList(createDiagnosticReport(1, "Patient/1"));
+		ourHitMethod = false;
+		httpGet = new HttpGet("http://localhost:" + ourPort + "/DiagnosticReport?patient=Patient/1");
+		status = ourClient.execute(httpGet);
+		extractResponseAndClose(status);
+		assertEquals(200, status.getStatusLine().getStatusCode());
+		assertTrue(ourHitMethod);
+
+		ourReturn = Collections.singletonList(createDiagnosticReport(1, "Patient/1"));
+		ourHitMethod = false;
+		httpGet = new HttpGet("http://localhost:" + ourPort + "/DiagnosticReport?patient=1");
+		status = ourClient.execute(httpGet);
+		extractResponseAndClose(status);
+		assertEquals(200, status.getStatusLine().getStatusCode());
+		assertTrue(ourHitMethod);
+
+		ourReturn = Collections.singletonList(createDiagnosticReport(1, "Patient/1"));
+		ourHitMethod = false;
+		httpGet = new HttpGet("http://localhost:" + ourPort + "/DiagnosticReport?patient=Patient/2");
+		status = ourClient.execute(httpGet);
+		extractResponseAndClose(status);
+		assertEquals(403, status.getStatusLine().getStatusCode());
+		assertFalse(ourHitMethod);
+	}
+
+		@Test
+	public void testReadByCompartmentReadByIdParam() throws Exception {
+		ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
+			@Override
+			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
+				return new RuleBuilder()
+					.allow("Rule 1").read().allResources().inCompartment("Patient", new IdType("Patient/1")).andThen()
+					.build();
+			}
+		});
+
+		HttpGet httpGet;
+		HttpResponse status;
+
+		ourReturn = Collections.singletonList(createPatient(1));
+		ourHitMethod = false;
+		httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=Patient/1");
+		status = ourClient.execute(httpGet);
+		extractResponseAndClose(status);
+		assertEquals(200, status.getStatusLine().getStatusCode());
+		assertTrue(ourHitMethod);
+
+		ourReturn = Collections.singletonList(createPatient(1));
+		ourHitMethod = false;
+		httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=1");
+		status = ourClient.execute(httpGet);
+		extractResponseAndClose(status);
+		assertEquals(200, status.getStatusLine().getStatusCode());
+		assertTrue(ourHitMethod);
+
+		ourHitMethod = false;
+		httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_id=Patient/2");
+		status = ourClient.execute(httpGet);
+		extractResponseAndClose(status);
+		assertEquals(403, status.getStatusLine().getStatusCode());
+		assertFalse(ourHitMethod);
+	}
+
+
 	@Test
 	public void testReadByCompartmentRight() throws Exception {
 		ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@@ -2811,12 +2900,13 @@ public class AuthorizationInterceptorR4Test {
 		DummyOrganizationResourceProvider orgProv = new DummyOrganizationResourceProvider();
 		DummyEncounterResourceProvider encProv = new DummyEncounterResourceProvider();
 		DummyCarePlanResourceProvider cpProv = new DummyCarePlanResourceProvider();
+		DummyDiagnosticReportResourceProvider drProv = new DummyDiagnosticReportResourceProvider();
 		PlainProvider plainProvider = new PlainProvider();
 
 		ServletHandler proxyHandler = new ServletHandler();
 		ourServlet = new RestfulServer(ourCtx);
 		ourServlet.setFhirContext(ourCtx);
-		ourServlet.setResourceProviders(patProvider, obsProv, encProv, cpProv, orgProv);
+		ourServlet.setResourceProviders(patProvider, obsProv, encProv, cpProv, orgProv, drProv);
 		ourServlet.setPlainProviders(plainProvider);
 		ourServlet.setPagingProvider(new FifoMemoryPagingProvider(100));
 		ServletHolder servletHolder = new ServletHolder(ourServlet);
@@ -2892,6 +2982,23 @@ public class AuthorizationInterceptorR4Test {
 
 	}
 
+	public static class DummyDiagnosticReportResourceProvider implements IResourceProvider {
+
+
+		@Override
+		public Class<? extends IBaseResource> getResourceType() {
+			return DiagnosticReport.class;
+		}
+		@Search()
+		public List<Resource> search(
+			@OptionalParam(name = "subject") ReferenceParam theSubject,
+			@OptionalParam(name = "patient") ReferenceParam thePatient
+			) {
+			ourHitMethod = true;
+			return ourReturn;
+		}
+	}
+
 	@SuppressWarnings("unused")
 	public static class DummyObservationResourceProvider implements IResourceProvider {
 
@@ -3076,7 +3183,7 @@ public class AuthorizationInterceptorR4Test {
 		}
 
 		@Search()
-		public List<Resource> search() {
+		public List<Resource> search(@OptionalParam(name="_id") IdType theIdParam) {
 			ourHitMethod = true;
 			return ourReturn;
 		}