From d29a9a7f9613dab5b383d00c8584c0f96c9de188 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Tue, 15 May 2018 16:14:10 -0400 Subject: [PATCH] AutohrizationInterceptor should correctly recognize type-level operation invocations --- .../uhn/fhir/rest/server/ResourceBinding.java | 14 +- .../server/method/OperationMethodBinding.java | 26 ++ .../AuthorizationInterceptorR4Test.java | 330 +++++++++++------- src/changes/changes.xml | 5 + 4 files changed, 250 insertions(+), 125 deletions(-) diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/ResourceBinding.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/ResourceBinding.java index 68ba1e5d043..15c78eca6ec 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/ResourceBinding.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/ResourceBinding.java @@ -34,24 +34,24 @@ public class ResourceBinding { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ResourceBinding.class); private String resourceName; - private List> methods = new ArrayList>(); + private List> myMethodBindings = new ArrayList<>(); public ResourceBinding() { } public ResourceBinding(String resourceName, List> methods) { this.resourceName = resourceName; - this.methods = methods; + this.myMethodBindings = methods; } public BaseMethodBinding getMethod(RequestDetails theRequest) { - if (null == methods) { + if (null == myMethodBindings) { ourLog.warn("No methods exist for resource: {}", resourceName); return null; } ourLog.debug("Looking for a handler for {}", theRequest); - for (BaseMethodBinding rm : methods) { + for (BaseMethodBinding rm : myMethodBindings) { if (rm.incomingServerRequestMatchesMethod(theRequest)) { ourLog.debug("Handler {} matches", rm); return rm; @@ -70,15 +70,15 @@ public class ResourceBinding { } public List> getMethodBindings() { - return methods; + return myMethodBindings; } public void setMethods(List> methods) { - this.methods = methods; + this.myMethodBindings = methods; } public void addMethod(BaseMethodBinding method) { - this.methods.add(method); + this.myMethodBindings.add(method); } @Override diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/OperationMethodBinding.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/OperationMethodBinding.java index d3f3676a2ce..5deaa1209e9 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/OperationMethodBinding.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/OperationMethodBinding.java @@ -28,6 +28,8 @@ import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.*; +import org.apache.commons.lang3.builder.ToStringBuilder; +import org.apache.commons.lang3.builder.ToStringStyle; import org.hl7.fhir.instance.model.api.IBase; import org.hl7.fhir.instance.model.api.IBaseResource; @@ -220,6 +222,30 @@ public class OperationMethodBinding extends BaseResourceReturningMethodBinding { } + @Override + public RestOperationTypeEnum getRestOperationType(RequestDetails theRequestDetails) { + RestOperationTypeEnum retVal = super.getRestOperationType(theRequestDetails); + + if (retVal == RestOperationTypeEnum.EXTENDED_OPERATION_INSTANCE) { + if (theRequestDetails.getId() == null) { + retVal = RestOperationTypeEnum.EXTENDED_OPERATION_TYPE; + } + } + + return retVal; + } + + @Override + public String toString() { + return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE) + .append("name", myName) + .append("methodName", getMethod().getDeclaringClass().getSimpleName() + "." + getMethod().getName()) + .append("serverLevel", myCanOperateAtServerLevel) + .append("typeLevel", myCanOperateAtTypeLevel) + .append("instanceLevel", myCanOperateAtInstanceLevel) + .toString(); + } + @Override public Object invokeServer(IRestfulServer theServer, RequestDetails theRequest) throws BaseServerResponseException, IOException { if (theRequest.getRequestType() == RequestTypeEnum.POST) { diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorR4Test.java index 165ef33c6d4..01b20c93a37 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/AuthorizationInterceptorR4Test.java @@ -97,6 +97,13 @@ public class AuthorizationInterceptorR4Test { return retVal; } + private Organization createOrganization(int theIndex) { + Organization retVal = new Organization(); + retVal.setId("" + theIndex); + retVal.setName("Org " + theIndex); + return retVal; + } + private Resource createPatient(Integer theId) { Patient retVal = new Patient(); if (theId != null) { @@ -820,6 +827,118 @@ public class AuthorizationInterceptorR4Test { } + @Test + public void testOperationAppliesAtAnyLevel() throws Exception { + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("RULE 1").operation().named("opName").atAnyLevel().andThen() + .build(); + } + }); + + HttpGet httpGet; + HttpResponse status; + String response; + + // Server + ourHitMethod = false; + ourReturn = Collections.singletonList(createObservation(10, "Patient/2")); + httpGet = new HttpGet("http://localhost:" + ourPort + "/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + // Type + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + // Instance + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + // Instance Version + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/_history/2/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + } + + @Test + public void testOperationAppliesAtAnyLevelWrongOpName() throws Exception { + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("RULE 1").operation().named("opNameBadOp").atAnyLevel().andThen() + .build(); + } + }); + + HttpGet httpGet; + HttpResponse status; + String response; + + // Server + ourHitMethod = false; + ourReturn = Collections.singletonList(createObservation(10, "Patient/2")); + httpGet = new HttpGet("http://localhost:" + ourPort + "/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + + // Type + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + + // Instance + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + + // Instance Version + ourHitMethod = false; + ourReturn = Collections.singletonList(createPatient(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/_history/2/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + + } + @Test public void testOperationByInstanceOfTypeAllowed() throws Exception { ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { @@ -1158,119 +1277,6 @@ public class AuthorizationInterceptorR4Test { assertFalse(ourHitMethod); } - - @Test - public void testOperationAppliesAtAnyLevel() throws Exception { - ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { - @Override - public List buildRuleList(RequestDetails theRequestDetails) { - return new RuleBuilder() - .allow("RULE 1").operation().named("opName").atAnyLevel().andThen() - .build(); - } - }); - - HttpGet httpGet; - HttpResponse status; - String response; - - // Server - ourHitMethod = false; - ourReturn = Collections.singletonList(createObservation(10, "Patient/2")); - httpGet = new HttpGet("http://localhost:" + ourPort + "/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - assertEquals(200, status.getStatusLine().getStatusCode()); - assertTrue(ourHitMethod); - - // Type - ourHitMethod = false; - ourReturn = Collections.singletonList(createPatient(2)); - httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - ourLog.info(response); - assertEquals(200, status.getStatusLine().getStatusCode()); - assertTrue(ourHitMethod); - - // Instance - ourHitMethod = false; - ourReturn = Collections.singletonList(createPatient(2)); - httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - ourLog.info(response); - assertEquals(200, status.getStatusLine().getStatusCode()); - assertTrue(ourHitMethod); - - // Instance Version - ourHitMethod = false; - ourReturn = Collections.singletonList(createPatient(2)); - httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/_history/2/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - ourLog.info(response); - assertEquals(200, status.getStatusLine().getStatusCode()); - assertTrue(ourHitMethod); - - } - - @Test - public void testOperationAppliesAtAnyLevelWrongOpName() throws Exception { - ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { - @Override - public List buildRuleList(RequestDetails theRequestDetails) { - return new RuleBuilder() - .allow("RULE 1").operation().named("opNameBadOp").atAnyLevel().andThen() - .build(); - } - }); - - HttpGet httpGet; - HttpResponse status; - String response; - - // Server - ourHitMethod = false; - ourReturn = Collections.singletonList(createObservation(10, "Patient/2")); - httpGet = new HttpGet("http://localhost:" + ourPort + "/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - assertEquals(403, status.getStatusLine().getStatusCode()); - assertFalse(ourHitMethod); - - // Type - ourHitMethod = false; - ourReturn = Collections.singletonList(createPatient(2)); - httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - ourLog.info(response); - assertEquals(403, status.getStatusLine().getStatusCode()); - assertFalse(ourHitMethod); - - // Instance - ourHitMethod = false; - ourReturn = Collections.singletonList(createPatient(2)); - httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - ourLog.info(response); - assertEquals(403, status.getStatusLine().getStatusCode()); - assertFalse(ourHitMethod); - - // Instance Version - ourHitMethod = false; - ourReturn = Collections.singletonList(createPatient(2)); - httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1/_history/2/$opName"); - status = ourClient.execute(httpGet); - response = extractResponseAndClose(status); - ourLog.info(response); - assertEquals(403, status.getStatusLine().getStatusCode()); - assertFalse(ourHitMethod); - - } - @Test public void testOperationTypeLevel() throws Exception { ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { @@ -1408,6 +1414,64 @@ public class AuthorizationInterceptorR4Test { assertFalse(ourHitMethod); } + @Test + public void testOperationTypeLevelWithOperationMethodHavingOptionalIdParam() throws Exception { + ourServlet.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) { + @Override + public List buildRuleList(RequestDetails theRequestDetails) { + return new RuleBuilder() + .allow("RULE 1").operation().named("opName").onType(Organization.class).andThen() + .build(); + } + }); + + HttpGet httpGet; + HttpResponse status; + String response; + + // Server + ourHitMethod = false; + ourReturn = Collections.singletonList(createObservation(10, "Organization/2")); + httpGet = new HttpGet("http://localhost:" + ourPort + "/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + assertThat(response, containsString("Access denied by default policy")); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + + // Type + ourHitMethod = false; + ourReturn = Collections.singletonList(createOrganization(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Organization/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertTrue(ourHitMethod); + + // Wrong type + ourHitMethod = false; + ourReturn = Collections.singletonList(createOrganization(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Observation/1/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertThat(response, containsString("Access denied by default policy")); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + + // Instance + ourHitMethod = false; + ourReturn = Collections.singletonList(createOrganization(2)); + httpGet = new HttpGet("http://localhost:" + ourPort + "/Organization/1/$opName"); + status = ourClient.execute(httpGet); + response = extractResponseAndClose(status); + ourLog.info(response); + assertThat(response, containsString("Access denied by default policy")); + assertEquals(403, status.getStatusLine().getStatusCode()); + assertFalse(ourHitMethod); + } + @Test public void testOperationTypeLevelWithTenant() throws Exception { ourServlet.setTenantIdentificationStrategy(new UrlBaseTenantIdentificationStrategy()); @@ -2480,6 +2544,7 @@ public class AuthorizationInterceptorR4Test { DummyPatientResourceProvider patProvider = new DummyPatientResourceProvider(); DummyObservationResourceProvider obsProv = new DummyObservationResourceProvider(); + DummyOrganizationResourceProvider orgProv = new DummyOrganizationResourceProvider(); DummyEncounterResourceProvider encProv = new DummyEncounterResourceProvider(); DummyCarePlanResourceProvider cpProv = new DummyCarePlanResourceProvider(); PlainProvider plainProvider = new PlainProvider(); @@ -2487,7 +2552,7 @@ public class AuthorizationInterceptorR4Test { ServletHandler proxyHandler = new ServletHandler(); ourServlet = new RestfulServer(ourCtx); ourServlet.setFhirContext(ourCtx); - ourServlet.setResourceProviders(patProvider, obsProv, encProv, cpProv); + ourServlet.setResourceProviders(patProvider, obsProv, encProv, cpProv, orgProv); ourServlet.setPlainProviders(plainProvider); ourServlet.setPagingProvider(new FifoMemoryPagingProvider(100)); ServletHolder servletHolder = new ServletHolder(ourServlet); @@ -2541,6 +2606,25 @@ public class AuthorizationInterceptorR4Test { } } + public static class DummyOrganizationResourceProvider implements IResourceProvider { + + + @Override + public Class getResourceType() { + return Organization.class; + } + + /** + * This should come before operation1 + */ + @Operation(name = "opName", idempotent = true) + public Parameters operation0(@IdParam(optional = true) IdType theId) { + ourHitMethod = true; + return (Parameters) new Parameters().setId("1"); + } + + } + @SuppressWarnings("unused") public static class DummyObservationResourceProvider implements IResourceProvider { @@ -2565,14 +2649,20 @@ public class AuthorizationInterceptorR4Test { return Observation.class; } + /** + * This should come before operation1 + */ @Operation(name = "opName", idempotent = true) - public Parameters operation() { + public Parameters operation0(@IdParam IdType theId) { ourHitMethod = true; return (Parameters) new Parameters().setId("1"); } + /** + * This should come after operation0 + */ @Operation(name = "opName", idempotent = true) - public Parameters operation(@IdParam IdType theId) { + public Parameters operation1() { ourHitMethod = true; return (Parameters) new Parameters().setId("1"); } @@ -2670,13 +2760,17 @@ public class AuthorizationInterceptorR4Test { } @Operation(name = "opName", idempotent = true) - public Parameters operation() { + public Parameters operation0(@IdParam IdType theId) { ourHitMethod = true; return (Parameters) new Parameters().setId("1"); } + /** + * More generic method second to make sure that the + * other method takes precedence + */ @Operation(name = "opName", idempotent = true) - public Parameters operation(@IdParam IdType theId) { + public Parameters operation1() { ourHitMethod = true; return (Parameters) new Parameters().setId("1"); } diff --git a/src/changes/changes.xml b/src/changes/changes.xml index d6bf91ee1c1..58cbe75e407 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -167,6 +167,11 @@ to occasionally consume two database connections, which could lead to deadlocks under heavy load. This has been fixed. + + AuthorizationInterceptor sometimes incorrectly identified an operation + invocation at the type level as being at the instance level if the method + indicated that the IdParam parameter was optional. This has been fixed. +