diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_1_0/1864-allow-authinterceptor-graphql.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_1_0/1864-allow-authinterceptor-graphql.yaml new file mode 100644 index 00000000000..7172ea26b36 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_1_0/1864-allow-authinterceptor-graphql.yaml @@ -0,0 +1,6 @@ +--- +type: add +issue: 1864 +title: "AuthorizationInterceptor can now fully secure GraphQL operations in the JPA server, meaning that + it is possible to use compartment and type rules to restrict the specific data that is available through + the GraphQL interface." diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java index a9ca9726609..f572cfdf669 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java @@ -17,8 +17,11 @@ import ca.uhn.fhir.rest.server.interceptor.auth.PolicyEnum; import ca.uhn.fhir.rest.server.interceptor.auth.RuleBuilder; import ca.uhn.fhir.rest.server.provider.ProviderConstants; import ca.uhn.fhir.util.TestUtil; +import ca.uhn.fhir.util.UrlUtil; +import org.apache.commons.io.IOUtils; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpDelete; +import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpPost; import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; @@ -45,9 +48,11 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.List; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; @@ -757,7 +762,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes 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(); + 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 { @@ -795,7 +800,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes .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")) + .andParameter(ProviderConstants.DIFF_TO_PARAMETER, new StringType("Patient/B")) .execute(); assertEquals(2, diff.getParameter().size()); @@ -805,7 +810,7 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes .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")) + .andParameter(ProviderConstants.DIFF_TO_PARAMETER, new StringType("Observation/D")) .execute(); fail(); } catch (ForbiddenOperationException e) { @@ -814,6 +819,42 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes } + + @Test + public void testGraphQL_AllowedByType_Instance() throws IOException { + createPatient(withId("A"), withFamily("MY_FAMILY")); + createPatient(withId("B"), withFamily("MY_FAMILY")); + + ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow().graphQL().any().andThen() + .allow().read().instance("Patient/A").andThen() + .denyAll() + .build(); + } + }); + + HttpGet httpGet; + String query = "{name{family,given}}"; + + httpGet = new HttpGet(ourServerBase + "/Patient/A/$graphql?query=" + UrlUtil.escapeUrlParam(query)); + try (CloseableHttpResponse response = ourHttpClient.execute(httpGet)) { + String resp = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); + assertEquals(200, response.getStatusLine().getStatusCode()); + assertThat(resp, containsString("MY_FAMILY")); + } + + httpGet = new HttpGet(ourServerBase + "/Patient/B/$graphql?query=" + UrlUtil.escapeUrlParam(query)); + try (CloseableHttpResponse response = ourHttpClient.execute(httpGet)) { + String resp = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); + assertEquals(403, response.getStatusLine().getStatusCode()); + } + + } + + /** * See #762 */ diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java index 214c1e4d9ff..1321c88bec1 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.java @@ -208,7 +208,7 @@ public class AuthorizationInterceptor implements IRuleApplier { return OperationExamineDirection.NONE; case GRAPHQL_REQUEST: - return OperationExamineDirection.IN; + return OperationExamineDirection.BOTH; default: // Should not happen diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/BaseRule.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/BaseRule.java index 706fa5cf344..b12718d1234 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/BaseRule.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/BaseRule.java @@ -20,6 +20,7 @@ package ca.uhn.fhir.rest.server.interceptor.auth; * #L% */ +import ca.uhn.fhir.interceptor.api.Pointcut; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.server.interceptor.auth.AuthorizationInterceptor.Verdict; @@ -102,4 +103,9 @@ abstract class BaseRule implements IAuthRule { } return new Verdict(myMode, this); } + + protected boolean isResourceAccess(Pointcut thePointcut) { + return thePointcut.equals(Pointcut.STORAGE_PREACCESS_RESOURCES) || thePointcut.equals(Pointcut.STORAGE_PRESHOW_RESOURCES); + } + } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderGraphQL.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderGraphQL.java index 8f72347a237..1d4ee54f15f 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderGraphQL.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderGraphQL.java @@ -23,9 +23,8 @@ package ca.uhn.fhir.rest.server.interceptor.auth; public interface IAuthRuleBuilderGraphQL { /** - * Note that this is an all-or-nothing grant for now, it - * is not yet possible to specify individual resource security when - * using GraphQL. + * Allow any GraphQL request. Note that this does not mean that any specific sub-operations are permitted, as + * other rules may be required in order to actually access specific resources */ IAuthRuleFinished any(); } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/OperationRule.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/OperationRule.java index 6746e79f118..1abf691fa52 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/OperationRule.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/OperationRule.java @@ -87,8 +87,7 @@ class OperationRule extends BaseRule implements IAuthRule { // 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) { + if (isResourceAccess(thePointcut)) { return null; } 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 ba24f2ce507..12b9beac51a 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 @@ -240,7 +240,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { // so just let it through for now.. return newVerdict(theOperation, theRequestDetails, theInputResource, theInputResourceId, theOutputResource); } - if (theInputResource== null && myClassifierCompartmentOwners != null && myClassifierCompartmentOwners.size() > 0) { + if (theInputResource == null && myClassifierCompartmentOwners != null && myClassifierCompartmentOwners.size() > 0) { return newVerdict(theOperation, theRequestDetails, theInputResource, theInputResourceId, theOutputResource); } @@ -252,6 +252,12 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { break; case GRAPHQL: if (theOperation == RestOperationTypeEnum.GRAPHQL_REQUEST) { + + // Make sure that the requestor actually has sufficient access to see the given resource + if (isResourceAccess(thePointcut)) { + return null; + } + return newVerdict(theOperation, theRequestDetails, theInputResource, theInputResourceId, theOutputResource); } else { return null; @@ -276,8 +282,8 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { UrlUtil.UrlParts parts = UrlUtil.parseUrl(nextPart.getUrl()); - inputResourceId = theRequestDetails.getFhirContext().getVersion().newIdType(); - inputResourceId.setParts(null, parts.getResourceType(), parts.getResourceId(), null); + inputResourceId = theRequestDetails.getFhirContext().getVersion().newIdType(); + inputResourceId.setParts(null, parts.getResourceType(), parts.getResourceId(), null); } RestOperationTypeEnum operation; @@ -511,7 +517,7 @@ class RuleImplOp extends BaseRule /* implements IAuthRule */ { return verdict; } } - } else if (getMode() == PolicyEnum.ALLOW){ + } else if (getMode() == PolicyEnum.ALLOW) { return newVerdict(theOperation, theRequestDetails, theInputResource, theInputResourceId, theOutputResource); } break;