From 33a4e9e57f155f13ea51b38f80468ccd759efa1b Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 15 Mar 2016 09:51:11 +0100 Subject: [PATCH] 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 --- .../AnnotationJmxAttributeSource.java | 7 + .../AnotherAnnotationTestBeanImpl.java | 2 +- .../EnableMBeanExportConfigurationTests.java | 241 ++++++++++++------ src/asciidoc/integration.adoc | 3 + 4 files changed, 179 insertions(+), 74 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/jmx/export/annotation/AnnotationJmxAttributeSource.java b/spring-context/src/main/java/org/springframework/jmx/export/annotation/AnnotationJmxAttributeSource.java index 8b07ce08dc9..bb0a0abd007 100644 --- a/spring-context/src/main/java/org/springframework/jmx/export/annotation/AnnotationJmxAttributeSource.java +++ b/spring-context/src/main/java/org/springframework/jmx/export/annotation/AnnotationJmxAttributeSource.java @@ -19,6 +19,7 @@ package org.springframework.jmx.export.annotation; 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 @@ import org.springframework.util.StringValueResolver; * @author Rob Harrop * @author Juergen Hoeller * @author Jennifer Hickey + * @author Stephane Nicoll * @since 1.2 * @see ManagedResource * @see ManagedAttribute @@ -64,6 +66,11 @@ public class AnnotationJmxAttributeSource implements JmxAttributeSource, BeanFac 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; diff --git a/spring-context/src/test/java/org/springframework/jmx/export/annotation/AnotherAnnotationTestBeanImpl.java b/spring-context/src/test/java/org/springframework/jmx/export/annotation/AnotherAnnotationTestBeanImpl.java index 1acaefb0328..8857bcc08a6 100644 --- a/spring-context/src/test/java/org/springframework/jmx/export/annotation/AnotherAnnotationTestBeanImpl.java +++ b/spring-context/src/test/java/org/springframework/jmx/export/annotation/AnotherAnnotationTestBeanImpl.java @@ -19,7 +19,7 @@ package org.springframework.jmx.export.annotation; /** * @author Stephane Nicoll */ -class AnotherAnnotationTestBeanImpl implements AnotherAnnotationTestBean { +public class AnotherAnnotationTestBeanImpl implements AnotherAnnotationTestBean { private String bar; diff --git a/spring-context/src/test/java/org/springframework/jmx/export/annotation/EnableMBeanExportConfigurationTests.java b/spring-context/src/test/java/org/springframework/jmx/export/annotation/EnableMBeanExportConfigurationTests.java index caad3982993..2a294f2b7d8 100644 --- a/spring-context/src/test/java/org/springframework/jmx/export/annotation/EnableMBeanExportConfigurationTests.java +++ b/spring-context/src/test/java/org/springframework/jmx/export/annotation/EnableMBeanExportConfigurationTests.java @@ -19,7 +19,10 @@ package org.springframework.jmx.export.annotation; 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.Scope; 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 @@ import static org.junit.Assert.*; * 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 class EnableMBeanExportConfigurationTests { } } + @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; + } + } + } diff --git a/src/asciidoc/integration.adoc b/src/asciidoc/integration.adoc index 337b582ba3e..7bf89bc7537 100644 --- a/src/asciidoc/integration.adoc +++ b/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: