From 1e5e40f570ac9f6ecc65aebe6c84df7d1bd9530b Mon Sep 17 00:00:00 2001 From: Paolo Bazzi Date: Wed, 14 Aug 2024 16:58:26 +0200 Subject: [PATCH] ServiceOperationInvoker#getValidatedServiceAccess inefficient reflection Add lazy initialized cache for public service methods for each queried class. Additionally fetching all public methods of a class at once avoids catching NoSuchMethodExceptions when trying to find a method. This change avoids repeated NoSuchMethodException when checking if access is allowed for services using interfaces and abstract class in their hierarchy, where not every super class contains an implementation of the invoked method. 359668 --- .../server/ServiceOperationInvokerTest.java | 78 +++++++++++++++++++ .../rt/server/ServiceOperationInvoker.java | 51 +++++++----- 2 files changed, 108 insertions(+), 21 deletions(-) diff --git a/org.eclipse.scout.rt.server.test/src/test/java/org/eclipse/scout/rt/server/ServiceOperationInvokerTest.java b/org.eclipse.scout.rt.server.test/src/test/java/org/eclipse/scout/rt/server/ServiceOperationInvokerTest.java index 56b66a45a5b..60acf07545f 100644 --- a/org.eclipse.scout.rt.server.test/src/test/java/org/eclipse/scout/rt/server/ServiceOperationInvokerTest.java +++ b/org.eclipse.scout.rt.server.test/src/test/java/org/eclipse/scout/rt/server/ServiceOperationInvokerTest.java @@ -16,8 +16,14 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.Set; +import java.util.stream.Collectors; + import org.eclipse.scout.rt.platform.BEANS; import org.eclipse.scout.rt.platform.exception.ProcessingException; +import org.eclipse.scout.rt.platform.util.CollectionUtility; import org.eclipse.scout.rt.server.admin.inspector.ProcessInspector; import org.eclipse.scout.rt.server.context.ServerRunContext; import org.eclipse.scout.rt.server.context.ServerRunContexts; @@ -115,4 +121,76 @@ private void assertValidResponse(ServiceTunnelResponse res, String data) { assertEquals(data, res.getData()); } + @Test + public void testGetPublicMethod() { + ServiceOperationInvoker invoker = new ServiceOperationInvoker(); + assertNull(invoker.getPublicMethod(Fixture.class, "privateMethod", int.class)); + assertNull(invoker.getPublicMethod(Fixture.class, "defaultMethod", long.class)); + assertNull(invoker.getPublicMethod(Fixture.class, "protectedMethod", float.class)); + runTestGetPublicMethod(invoker, Fixture.class, "publicMethod", double.class); + + assertNull(invoker.getPublicMethod(SubFixture.class, "privateSubMethod", int.class)); + assertNull(invoker.getPublicMethod(SubFixture.class, "defaultSubMethod", long.class)); + assertNull(invoker.getPublicMethod(SubFixture.class, "protectedSubMethod", float.class)); + runTestGetPublicMethod(invoker, SubFixture.class, "publicMethod", double.class); + runTestGetPublicMethod(invoker, SubFixture.class, "publicSubMethod", double.class); + } + + protected void runTestGetPublicMethod(ServiceOperationInvoker invoker, Class clazz, String methodName, Class paramType) { + Method m = invoker.getPublicMethod(clazz, methodName, paramType); + assertEquals(methodName, m.getName()); + assertEquals(paramType, m.getParameterTypes()[0]); + assertEquals(void.class, m.getReturnType()); + } + + @Test + public void testGetAllDeclaredMethods() { + ServiceOperationInvoker invoker = new ServiceOperationInvoker(); + Method[] methods = invoker.getPublicMethods(Fixture.class); + // NOTE: do not assert method count and exact method set since test framework agents could add additional methods (e.g. $jacocoInit method used for profiling) + assertTrue(CollectionUtility.containsAll(Arrays.stream(methods).map(Method::getName).collect(Collectors.toSet()), Set.of("publicMethod"))); + assertSame(methods, invoker.getPublicMethods(Fixture.class)); + + Method[] methodsSub = invoker.getPublicMethods(SubFixture.class); + // NOTE: do not assert method count and exact method set since test framework agents could add additional methods (e.g. $jacocoInit method used for profiling) + assertTrue(CollectionUtility.containsAll(Arrays.stream(methodsSub).map(Method::getName).collect(Collectors.toSet()), Set.of("publicSubMethod", "publicMethod"))); + assertSame(methodsSub, invoker.getPublicMethods(SubFixture.class)); + } + + @SuppressWarnings("unused") + private static class Fixture { + private void privateMethod(int i) { + } + + void defaultMethod(long l) { + } + + protected void protectedMethod(float f) { + } + + public void publicMethod(double d) { + } + } + + + @SuppressWarnings("unused") + private static class SubFixture extends Fixture { + private void privateSubMethod(int i) { + } + + void defaultSubMethod(long l) { + } + + protected void protectedSubMethod(float f) { + } + + public void publicSubMethod(double d) { + } + + @Override + public void publicMethod(double d) { + // overridden method fixture + super.publicMethod(d); + } + } } diff --git a/org.eclipse.scout.rt.server/src/main/java/org/eclipse/scout/rt/server/ServiceOperationInvoker.java b/org.eclipse.scout.rt.server/src/main/java/org/eclipse/scout/rt/server/ServiceOperationInvoker.java index 1008adbf31d..a218dc6e9b3 100644 --- a/org.eclipse.scout.rt.server/src/main/java/org/eclipse/scout/rt/server/ServiceOperationInvoker.java +++ b/org.eclipse.scout.rt.server/src/main/java/org/eclipse/scout/rt/server/ServiceOperationInvoker.java @@ -10,7 +10,10 @@ package org.eclipse.scout.rt.server; import java.lang.reflect.Method; +import java.util.Arrays; import java.util.Date; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import org.eclipse.scout.rt.platform.ApplicationScoped; @@ -25,12 +28,12 @@ import org.eclipse.scout.rt.platform.serialization.SerializationUtility; import org.eclipse.scout.rt.platform.service.IService; import org.eclipse.scout.rt.platform.text.TEXTS; +import org.eclipse.scout.rt.security.ACCESS; import org.eclipse.scout.rt.server.admin.inspector.CallInspector; import org.eclipse.scout.rt.server.admin.inspector.ProcessInspector; import org.eclipse.scout.rt.server.admin.inspector.SessionInspector; import org.eclipse.scout.rt.server.session.ServerSessionProvider; import org.eclipse.scout.rt.shared.security.RemoteServiceAccessPermission; -import org.eclipse.scout.rt.security.ACCESS; import org.eclipse.scout.rt.shared.servicetunnel.RemoteServiceAccessDenied; import org.eclipse.scout.rt.shared.servicetunnel.RemoteServiceWithoutAuthorization; import org.eclipse.scout.rt.shared.servicetunnel.ServiceTunnelRequest; @@ -47,6 +50,8 @@ public class ServiceOperationInvoker { private static final Logger LOG = LoggerFactory.getLogger(ServiceOperationInvoker.class); + protected final Map m_publicMethodCache = new ConcurrentHashMap<>(); + /** * Invoke the service associated with the {@link ServiceTunnelRequest}.
* Must be called within a transaction. @@ -158,12 +163,8 @@ protected void checkRemoteServiceAccessByInterface(Class interfaceClass, Meth } //check: method is defined on service interface itself - Method verifyMethod; - try { - verifyMethod = interfaceClass.getMethod(interfaceMethod.getName(), interfaceMethod.getParameterTypes()); - } - catch (NoSuchMethodException | RuntimeException t) { - LOG.debug("Could not lookup service method", t); + Method verifyMethod = getPublicMethod(interfaceClass, interfaceMethod.getName(), interfaceMethod.getParameterTypes()); + if (verifyMethod == null) { throw new SecurityException("access denied (code 1c)."); } //exists @@ -183,13 +184,7 @@ protected void checkRemoteServiceAccessByAnnotations(Class interfaceClass, Cl Class c = implClass; while (c != null) { //method level - Method m = null; - try { - m = c.getMethod(interfaceMethod.getName(), interfaceMethod.getParameterTypes()); - } - catch (NoSuchMethodException | RuntimeException t) { - LOG.debug("Could not lookup service method", t); - } + Method m = getPublicMethod(c, interfaceMethod.getName(), interfaceMethod.getParameterTypes()); if (m != null && m.isAnnotationPresent(RemoteServiceAccessDenied.class)) { throw new SecurityException("access denied (code 2b)."); } @@ -239,13 +234,7 @@ protected boolean mustAuthorize(Class interfaceClass, Class implClass, Met Class c = implClass; while (c != null) { //method level - Method m = null; - try { - m = c.getMethod(interfaceMethod.getName(), interfaceMethod.getParameterTypes()); - } - catch (NoSuchMethodException | RuntimeException t) { - LOG.debug("Could not lookup service method", t); - } + Method m = getPublicMethod(c, interfaceMethod.getName(), interfaceMethod.getParameterTypes()); if (m != null && m.isAnnotationPresent(RemoteServiceWithoutAuthorization.class)) { //granted return false; @@ -316,4 +305,24 @@ protected Throwable interceptException(Throwable t) { p.setStackTrace(new StackTraceElement[0]); return p; } + + /** + * @return public {@link Method} of class {@code clazz} with given {@code methodName} and {@code parameterTypes} or + * {@code null} if no matching method could be found. + */ + protected Method getPublicMethod(Class clazz, String methodName, Class... parameterTypes) { + for (Method method : getPublicMethods(clazz)) { + if (method.getName().equals(methodName) && Arrays.equals(method.getParameterTypes(), parameterTypes)) { + return method; + } + } + return null; + } + + /** + * @return array of {@link Method} with all public methods of given {@code clazz}. + */ + protected Method[] getPublicMethods(Class clazz) { + return m_publicMethodCache.computeIfAbsent(clazz, Class::getMethods); + } }