diff --git a/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/AuthorizationInterceptors.java b/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/AuthorizationInterceptors.java index 4ce9fd1420a..4073d33be8d 100644 --- a/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/AuthorizationInterceptors.java +++ b/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/AuthorizationInterceptors.java @@ -211,7 +211,7 @@ public class AuthorizationInterceptors { @Override public List buildRuleList(RequestDetails theRequestDetails) { AdditionalCompartmentSearchParameters additionalSearchParams = new AdditionalCompartmentSearchParameters(); - additionalSearchParams.addSearchParameters("device", "patient"); + additionalSearchParams.addSearchParameters("device:patient", "device:subject"); return new RuleBuilder() .allow().read().allResources().inCompartmentWithAdditionalSearchParams("Patient", new IdType("Patient/123"), additionalSearchParams) .build(); diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AdditionalCompartmentSearchParameters.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AdditionalCompartmentSearchParameters.java index 3fd2eaa4369..39e1927e3a5 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AdditionalCompartmentSearchParameters.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AdditionalCompartmentSearchParameters.java @@ -11,7 +11,7 @@ import java.util.Set; /** * This class is used in RuleBuilder, as a way to provide a compartment permission additional resource search params that * are to be included as "in" the given compartment. For example, if you were to populate this map with - * [device -> ["patient"] + * [device -> ["patient", "subject"] * and apply it to compartment Patient/123, then any device with Patient/123 as its patient would be considered "in" * the compartment, despite the fact that device is technically not part of the compartment definition for patient. */ @@ -22,9 +22,17 @@ public class AdditionalCompartmentSearchParameters { myResourceTypeToParameterCodeMap = new HashMap<>(); } - public void addSearchParameters(@Nonnull String theResourceType, @Nonnull String... theParameterCodes) { - Arrays.stream(theParameterCodes).forEach(code -> { - myResourceTypeToParameterCodeMap.computeIfAbsent(theResourceType.toLowerCase(), (key) -> new HashSet<>()).add(code.toLowerCase()); + public void addSearchParameters(@Nonnull String... theQualifiedSearchParameters) { + Arrays.stream(theQualifiedSearchParameters).forEach(code -> { + if (code == null || !code.contains(":")) { + throw new IllegalArgumentException(code + " is not a valid search parameter. Search parameters must be in the form resourcetype:parametercode, e.g. 'Device:patient'"); + } + String[] split = code.split(":"); + if (split.length != 2) { + throw new IllegalArgumentException(code + " is not a valid search parameter. Search parameters must be in the form resourcetype:parametercode, e.g. 'Device:patient'"); + } else { + myResourceTypeToParameterCodeMap.computeIfAbsent(split[0].toLowerCase(), (key) -> new HashSet<>()).add(split[1].toLowerCase()); + } }); } 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 cda03b1d43f..2b79752822f 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 @@ -84,6 +84,8 @@ import java.util.concurrent.TimeUnit; import static org.apache.commons.lang3.StringUtils.isNotBlank; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -379,11 +381,12 @@ public class AuthorizationInterceptorR4Test { @Test public void testCustomCompartmentSpsOnMultipleInstances() throws Exception { + //Given ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { @Override public List buildRuleList(RequestDetails theRequestDetails) { AdditionalCompartmentSearchParameters additionalCompartmentSearchParameters = new AdditionalCompartmentSearchParameters(); - additionalCompartmentSearchParameters.addSearchParameters("device", "patient"); + additionalCompartmentSearchParameters.addSearchParameters("Device:patient"); List relatedIds = new ArrayList<>(); relatedIds.add(new IdType("Patient/123")); relatedIds.add(new IdType("Patient/456")); @@ -399,8 +402,6 @@ public class AuthorizationInterceptorR4Test { HttpResponse status; Patient patient; - - patient = new Patient(); patient.setId("Patient/123"); Device d = new Device(); @@ -408,12 +409,89 @@ public class AuthorizationInterceptorR4Test { ourHitMethod = false; ourReturn = Collections.singletonList(d); + + //When httpGet = new HttpGet("http://localhost:" + ourPort + "/Device/124456"); status = ourClient.execute(httpGet); extractResponseAndClose(status); - assertEquals(200, status.getStatusLine().getStatusCode()); + + //Then assertTrue(ourHitMethod); + assertEquals(200, status.getStatusLine().getStatusCode()); } + + @Test + public void testCustomSearchParamsDontOverPermit() throws Exception { + //Given + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + AdditionalCompartmentSearchParameters additionalCompartmentSearchParameters = new AdditionalCompartmentSearchParameters(); + additionalCompartmentSearchParameters.addSearchParameters("Encounter:patient"); + List relatedIds = new ArrayList<>(); + relatedIds.add(new IdType("Patient/123")); + return new RuleBuilder() + .allow().read().allResources() + .inCompartmentWithAdditionalSearchParams("Patient", relatedIds, additionalCompartmentSearchParameters) + .andThen().denyAll() + .build(); + } + }); + + HttpGet httpGet; + HttpResponse status; + + Patient patient; + patient = new Patient(); + patient.setId("Patient/123"); + Device d = new Device(); + d.getPatient().setResource(patient); + + ourHitMethod = false; + ourReturn = Collections.singletonList(d); + + //When + httpGet = new HttpGet("http://localhost:" + ourPort + "/Device/124456"); + status = ourClient.execute(httpGet); + extractResponseAndClose(status); + + //then + assertFalse(ourHitMethod); + assertEquals(403, status.getStatusLine().getStatusCode()); + } + + @Test + public void testRuleBuilderAdditionalSearchParamsInvalidValues() { + //Too many colons + try { + AdditionalCompartmentSearchParameters additionalCompartmentSearchParameters = new AdditionalCompartmentSearchParameters(); + additionalCompartmentSearchParameters.addSearchParameters("too:many:colons"); + new RuleBuilder() + .allow().read().allResources() + .inCompartmentWithAdditionalSearchParams("Patient", new IdType("Patient/123"), additionalCompartmentSearchParameters) + .andThen().denyAll() + .build(); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), is(equalTo("too:many:colons is not a valid search parameter. Search parameters must be in the form resourcetype:parametercode, e.g. 'Device:patient'"))); + } + + + //No colons + try { + AdditionalCompartmentSearchParameters additionalCompartmentSearchParameters = new AdditionalCompartmentSearchParameters(); + additionalCompartmentSearchParameters.addSearchParameters("no-colons"); + new RuleBuilder() + .allow().read().allResources() + .inCompartmentWithAdditionalSearchParams("Patient", new IdType("Patient/123"), additionalCompartmentSearchParameters) + .andThen().denyAll() + .build(); + fail(); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), is(equalTo("no-colons is not a valid search parameter. Search parameters must be in the form resourcetype:parametercode, e.g. 'Device:patient'"))); + } + } + @Test public void testAllowByCompartmentUsingUnqualifiedIds() throws Exception { ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {