Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ServiceOperationInvoker#getValidatedServiceAccess inefficient reflection #1106

Open
wants to merge 1 commit into
base: releases/24.2
Choose a base branch
from
Open

ServiceOperationInvoker#getValidatedServiceAccess inefficient reflection #1106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions ...rt.server.test/src/test/java/org/eclipse/scout/rt/server/ServiceOperationInvokerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
51 changes: 30 additions & 21 deletions ...se.scout.rt.server/src/main/java/org/eclipse/scout/rt/server/ServiceOperationInvoker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -47,6 +50,8 @@
public class ServiceOperationInvoker {
private static final Logger LOG = LoggerFactory.getLogger(ServiceOperationInvoker.class);

protected final Map<Class, Method[]> m_publicMethodCache = new ConcurrentHashMap<>();

/**
* Invoke the service associated with the {@link ServiceTunnelRequest}. <br>
* Must be called within a transaction.
Expand Down Expand Up @@ -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
Expand All @@ -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).");
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}