AutohrizationInterceptor should correctly recognize type-level operation

invocations

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<BaseMethodBinding<?>> methods = new ArrayList<BaseMethodBinding<?>>();
+	private List<BaseMethodBinding<?>> myMethodBindings = new ArrayList<>();
 
 	public ResourceBinding() {
 	}
 
 	public ResourceBinding(String resourceName, List<BaseMethodBinding<?>> 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<BaseMethodBinding<?>> getMethodBindings() {
-		return methods;
+		return myMethodBindings;
 	}
 
 	public void setMethods(List<BaseMethodBinding<?>> methods) {
-		this.methods = methods;
+		this.myMethodBindings = methods;
 	}
 
 	public void addMethod(BaseMethodBinding<?> method) {
-		this.methods.add(method);
+		this.myMethodBindings.add(method);
 	}
 
 	@Override

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) {

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<IAuthRule> 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<IAuthRule> 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<IAuthRule> 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<IAuthRule> 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<IAuthRule> 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<? extends IBaseResource> 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");
 		}

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.
 			</action>
+			<action type="fix">
+				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.
+			</action>
 		</release>
 		<release version="3.3.0" date="2018-03-29">
 			<action type="add">