Prevent non public bean to be exposed to JMX

Previously, a package private `@ManagedResource` annotated bean was
registered to the JMX domain even if any attempt to invoke an operation
on it will fail since it has to be public.

This commit validates that any `@ManagedResource` annotated bean is
public and throws an InvalidMetadataException otherwise. Note that the
actual bean type does not have to be public as long as the class
annotated with `@ManagedResource` in the hierarchy is pubic and no extra
operations or attributes are defined on the child.

Issue: SPR-14042

Author: snicoll

spring-context/src/main/java/org/springframework/jmx/export/annotation/AnnotationJmxAttributeSource.java

@@ -19,6 +19,7 @@
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Array;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.Collection;
 import java.util.Set;
 
@@ -40,6 +41,7 @@
  * @author Rob Harrop
  * @author Juergen Hoeller
  * @author Jennifer Hickey
+ * @author Stephane Nicoll
  * @since 1.2
  * @see ManagedResource
  * @see ManagedAttribute
@@ -64,6 +66,11 @@ public org.springframework.jmx.export.metadata.ManagedResource getManagedResourc
 		if (ann == null) {
 			return null;
 		}
+		Class<?> declaringClass = AnnotationUtils.findAnnotationDeclaringClass(ManagedResource.class, beanClass);
+		Class<?> target = (declaringClass != null && !declaringClass.isInterface() ? declaringClass : beanClass);
+		if (!Modifier.isPublic(target.getModifiers())) {
+			throw new InvalidMetadataException("@ManagedResource class '" + target.getName() + "' must be public");
+		}
 		org.springframework.jmx.export.metadata.ManagedResource managedResource = new org.springframework.jmx.export.metadata.ManagedResource();
 		AnnotationBeanUtils.copyPropertiesToBean(ann, managedResource, this.embeddedValueResolver);
 		return managedResource;

spring-context/src/test/java/org/springframework/jmx/export/annotation/AnotherAnnotationTestBeanImpl.java

@@ -19,7 +19,7 @@
 /**
  * @author Stephane Nicoll
  */
-class AnotherAnnotationTestBeanImpl implements AnotherAnnotationTestBean {
+public class AnotherAnnotationTestBeanImpl implements AnotherAnnotationTestBean {
 
 	private String bar;
 

spring-context/src/test/java/org/springframework/jmx/export/annotation/EnableMBeanExportConfigurationTests.java

@@ -19,7 +19,10 @@
 import javax.management.MBeanServer;
 import javax.management.ObjectName;
 
+import org.junit.After;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import org.springframework.beans.factory.config.PropertyPlaceholderConfigurer;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
@@ -33,6 +36,7 @@
 import org.springframework.context.annotation.ScopedProxyMode;
 import org.springframework.jmx.export.MBeanExporterTests;
 import org.springframework.jmx.export.TestDynamicMBean;
+import org.springframework.jmx.export.metadata.InvalidMetadataException;
 import org.springframework.jmx.support.MBeanServerFactoryBean;
 import org.springframework.jmx.support.ObjectNameManager;
 import org.springframework.jmx.support.RegistrationPolicy;
@@ -44,109 +48,107 @@
  * Tests for {@link EnableMBeanExport} and {@link MBeanExportConfiguration}.
  *
  * @author Phillip Webb
+ * @author Stephane Nicoll
  * @see AnnotationLazyInitMBeanTests
  */
 public class EnableMBeanExportConfigurationTests {
 
+	@Rule
+	public final ExpectedException thrown = ExpectedException.none();
+
+	private AnnotationConfigApplicationContext ctx;
+
+	@After
+	public void closeContext() {
+		if (this.ctx != null) {
+			this.ctx.close();
+		}
+	}
+
 	@Test
 	public void testLazyNaming() throws Exception {
-		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(
-				LazyNamingConfiguration.class);
-		try {
-			MBeanServer server = (MBeanServer) ctx.getBean("server");
-			ObjectName oname = ObjectNameManager.getInstance("bean:name=testBean4");
-			assertNotNull(server.getObjectInstance(oname));
-			String name = (String) server.getAttribute(oname, "Name");
-			assertEquals("Invalid name returned", "TEST", name);
-		}
-		finally {
-			ctx.close();
-		}
+		load(LazyNamingConfiguration.class);
+		validateAnnotationTestBean();
+	}
+
+	private void load(Class<?>... config) {
+		this.ctx = new AnnotationConfigApplicationContext(config);
 	}
 
 	@Test
 	public void testOnlyTargetClassIsExposed() throws Exception {
-		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(
-				ProxyConfiguration.class);
-		try {
-			MBeanServer server = (MBeanServer) ctx.getBean("server");
-			ObjectName oname = ObjectNameManager.getInstance("bean:name=testBean4");
-			assertNotNull(server.getObjectInstance(oname));
-			assertEquals("TEST", server.getAttribute(oname, "Name"));
-		}
-		finally {
-			ctx.close();
-		}
+		load(ProxyConfiguration.class);
+		validateAnnotationTestBean();
+	}
+
+	@Test
+	public void testPackagePrivateExtensionCantBeExposed() {
+		this.thrown.expect(InvalidMetadataException.class);
+		this.thrown.expectMessage(PackagePrivateTestBean.class.getName());
+		this.thrown.expectMessage("must be public");
+		new AnnotationConfigApplicationContext(PackagePrivateConfiguration.class);
+	}
+
+	@Test
+	public void testPackagePrivateImplementationCantBeExposed() {
+		this.thrown.expect(InvalidMetadataException.class);
+		this.thrown.expectMessage(PackagePrivateAnnotationTestBean.class.getName());
+		this.thrown.expectMessage("must be public");
+		new AnnotationConfigApplicationContext(PackagePrivateInterfaceImplementationConfiguration.class);
+	}
+
+	@Test
+	public void testPackagePrivateClassExtensionCanBeExposed() throws Exception {
+		load(PackagePrivateExtensionConfiguration.class);
+		validateAnnotationTestBean();
 	}
 
 	@Test
 	public void testPlaceholderBased() throws Exception {
 		MockEnvironment env = new MockEnvironment();
 		env.setProperty("serverName", "server");
-		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
-		ctx.setEnvironment(env);
-		ctx.register(PlaceholderBasedConfiguration.class);
-		ctx.refresh();
-		try {
-			MBeanServer server = (MBeanServer) ctx.getBean("server");
-			ObjectName oname = ObjectNameManager.getInstance("bean:name=testBean4");
-			assertNotNull(server.getObjectInstance(oname));
-			String name = (String) server.getAttribute(oname, "Name");
-			assertEquals("Invalid name returned", "TEST", name);
-		}
-		finally {
-			ctx.close();
-		}
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext();
+		context.setEnvironment(env);
+		context.register(PlaceholderBasedConfiguration.class);
+		context.refresh();
+		this.ctx = context;
+		validateAnnotationTestBean();
 	}
 
 	@Test
 	public void testLazyAssembling() throws Exception {
 		System.setProperty("domain", "bean");
-		AnnotationConfigApplicationContext ctx =
-				new AnnotationConfigApplicationContext(LazyAssemblingConfiguration.class);
+		load(LazyAssemblingConfiguration.class);
 		try {
-			MBeanServer server = (MBeanServer) ctx.getBean("server");
-
-			ObjectName oname = ObjectNameManager.getInstance("bean:name=testBean4");
-			assertNotNull(server.getObjectInstance(oname));
-			String name = (String) server.getAttribute(oname, "Name");
-			assertEquals("Invalid name returned", "TEST", name);
-
-			oname = ObjectNameManager.getInstance("bean:name=testBean5");
-			assertNotNull(server.getObjectInstance(oname));
-			name = (String) server.getAttribute(oname, "Name");
-			assertEquals("Invalid name returned", "FACTORY", name);
-
-			oname = ObjectNameManager.getInstance("spring:mbean=true");
-			assertNotNull(server.getObjectInstance(oname));
-			name = (String) server.getAttribute(oname, "Name");
-			assertEquals("Invalid name returned", "Rob Harrop", name);
-
-			oname = ObjectNameManager.getInstance("spring:mbean=another");
-			assertNotNull(server.getObjectInstance(oname));
-			name = (String) server.getAttribute(oname, "Name");
-			assertEquals("Invalid name returned", "Juergen Hoeller", name);
+			MBeanServer server = (MBeanServer) this.ctx.getBean("server");
+
+			validateMBeanAttribute(server, "bean:name=testBean4", "TEST");
+			validateMBeanAttribute(server, "bean:name=testBean5", "FACTORY");
+			validateMBeanAttribute(server, "spring:mbean=true", "Rob Harrop");
+			validateMBeanAttribute(server, "spring:mbean=another", "Juergen Hoeller");
 		}
 		finally {
 			System.clearProperty("domain");
-			ctx.close();
 		}
 	}
 
 	@Test
 	public void testComponentScan() throws Exception {
-		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(
-				ComponentScanConfiguration.class);
-		try {
-			MBeanServer server = (MBeanServer) ctx.getBean("server");
-			ObjectName oname = ObjectNameManager.getInstance("bean:name=testBean4");
-			assertNotNull(server.getObjectInstance(oname));
-			String name = (String) server.getAttribute(oname, "Name");
-			assertNull(name);
-		}
-		finally {
-			ctx.close();
-		}
+		load(ComponentScanConfiguration.class);
+		MBeanServer server = (MBeanServer) this.ctx.getBean("server");
+		validateMBeanAttribute(server, "bean:name=testBean4", null);
+	}
+
+	private void validateAnnotationTestBean() throws Exception {
+		MBeanServer server = (MBeanServer) this.ctx.getBean("server");
+		validateMBeanAttribute(server,"bean:name=testBean4", "TEST");
+	}
+
+	private void validateMBeanAttribute(MBeanServer server, String objectName, String expected) throws Exception {
+		ObjectName oname = ObjectNameManager.getInstance(objectName);
+		assertNotNull(server.getObjectInstance(oname));
+		String name = (String) server.getAttribute(oname, "Name");
+		assertEquals("Invalid name returned", expected, name);
 	}
 
 
@@ -271,4 +273,97 @@ public MBeanServerFactoryBean server() throws Exception {
 		}
 	}
 
+	@Configuration
+	@EnableMBeanExport(server = "server")
+	static class PackagePrivateConfiguration {
+
+		@Bean
+		public MBeanServerFactoryBean server() throws Exception {
+			return new MBeanServerFactoryBean();
+		}
+
+		@Bean
+		public PackagePrivateTestBean testBean() {
+			return new PackagePrivateTestBean();
+		}
+	}
+
+	@ManagedResource(objectName = "bean:name=packagePrivate")
+	private static class PackagePrivateTestBean {
+
+		private String name;
+
+		@ManagedAttribute
+		public String getName() {
+			return this.name;
+		}
+
+		@ManagedAttribute
+		public void setName(String name) {
+			this.name = name;
+		}
+	}
+
+
+	@Configuration
+	@EnableMBeanExport(server = "server")
+	static class PackagePrivateExtensionConfiguration {
+
+		@Bean
+		public MBeanServerFactoryBean server() throws Exception {
+			return new MBeanServerFactoryBean();
+		}
+
+		@Bean
+		public PackagePrivateTestBeanExtension testBean() {
+			PackagePrivateTestBeanExtension bean = new PackagePrivateTestBeanExtension();
+			bean.setName("TEST");
+			return bean;
+		}
+	}
+
+	private static class PackagePrivateTestBeanExtension extends AnnotationTestBean {
+
+	}
+
+	@Configuration
+	@EnableMBeanExport(server = "server")
+	static class PackagePrivateInterfaceImplementationConfiguration {
+
+		@Bean
+		public MBeanServerFactoryBean server() throws Exception {
+			return new MBeanServerFactoryBean();
+		}
+
+		@Bean
+		public PackagePrivateAnnotationTestBean testBean() {
+			return new PackagePrivateAnnotationTestBean();
+		}
+	}
+
+	private static class PackagePrivateAnnotationTestBean implements AnotherAnnotationTestBean {
+
+		private String bar;
+
+		@Override
+		public void foo() {
+
+		}
+
+		@Override
+		public String getBar() {
+			return this.bar;
+		}
+
+		@Override
+		public void setBar(String bar) {
+			this.bar = bar;
+		}
+
+		@Override
+		public int getCacheEntries() {
+			return 0;
+		}
+	}
+
 }

src/asciidoc/integration.adoc

@@ -3508,6 +3508,9 @@ be marked with the `ManagedAttribute` annotation. When marking properties you ca
 either the annotation of the getter or the setter to create a write-only or read-only
 attribute respectively.
 
+NOTE: A `ManagedResource` annotated bean must be public as well as the methods exposing
+an operation or an attribute.
+
 The example below shows the annotated version of the `JmxTestBean` class that you saw
 earlier: