@EnableMethodSecurity doesn't resolve Method Security annotations on interfaces through a Proxy

Removed proxy unwrapping in case of resolving Method Security annotations,
this cause an issue when interfaces which are implemented by the proxy was skipped,
resulting in a missing security checks on those methods.

Closes gh-11175
This commit is contained in:
Evgeniy Cheban 2022-04-29 17:44:28 +03:00 committed by Rob Winch
parent 5ac5edc2e6
commit 286e95893a
4 changed files with 88 additions and 4 deletions

View File

@ -22,7 +22,6 @@ import java.util.concurrent.ConcurrentHashMap;
import org.aopalliance.intercept.MethodInvocation;
import org.springframework.aop.support.AopUtils;
import org.springframework.core.MethodClassKey;
import org.springframework.lang.NonNull;
import org.springframework.security.authorization.AuthorizationManager;
@ -46,7 +45,7 @@ abstract class AbstractAuthorizationManagerRegistry {
final AuthorizationManager<MethodInvocation> getManager(MethodInvocation methodInvocation) {
Method method = methodInvocation.getMethod();
Object target = methodInvocation.getThis();
Class<?> targetClass = (target != null) ? AopUtils.getTargetClass(target) : null;
Class<?> targetClass = (target != null) ? target.getClass() : null;
MethodClassKey cacheKey = new MethodClassKey(method, targetClass);
return this.cachedManagers.computeIfAbsent(cacheKey, (k) -> resolveManager(method, targetClass));
}

View File

@ -22,7 +22,6 @@ import java.util.concurrent.ConcurrentHashMap;
import org.aopalliance.intercept.MethodInvocation;
import org.springframework.aop.support.AopUtils;
import org.springframework.core.MethodClassKey;
import org.springframework.lang.NonNull;
@ -43,7 +42,7 @@ abstract class AbstractExpressionAttributeRegistry<T extends ExpressionAttribute
final T getAttribute(MethodInvocation mi) {
Method method = mi.getMethod();
Object target = mi.getThis();
Class<?> targetClass = (target != null) ? AopUtils.getTargetClass(target) : null;
Class<?> targetClass = (target != null) ? target.getClass() : null;
return getAttribute(method, targetClass);
}

View File

@ -22,6 +22,7 @@ import java.util.function.Supplier;
import org.junit.jupiter.api.Test;
import org.springframework.aop.TargetClassAware;
import org.springframework.core.annotation.AnnotationConfigurationException;
import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler;
import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler;
@ -133,6 +134,19 @@ public class PreAuthorizeAuthorizationManagerTests {
.isThrownBy(() -> manager.check(authentication, methodInvocation));
}
@Test
public void checkTargetClassAwareWhenInterfaceLevelAnnotationsThenApplies() throws Exception {
MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestTargetClassAware(),
TestTargetClassAware.class, "doSomething");
PreAuthorizeAuthorizationManager manager = new PreAuthorizeAuthorizationManager();
AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation);
assertThat(decision).isNotNull();
assertThat(decision.isGranted()).isFalse();
decision = manager.check(TestAuthentication::authenticatedAdmin, methodInvocation);
assertThat(decision).isNotNull();
assertThat(decision.isGranted()).isTrue();
}
public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo {
public void doSomething() {
@ -198,4 +212,33 @@ public class PreAuthorizeAuthorizationManagerTests {
}
@PreAuthorize("hasRole('ADMIN')")
public interface InterfaceLevelAnnotations {
}
public static class TestTargetClassAware extends TestClass implements TargetClassAware, InterfaceLevelAnnotations {
@Override
public Class<?> getTargetClass() {
return TestClass.class;
}
@Override
public void doSomething() {
super.doSomething();
}
@Override
public String doSomethingString(String s) {
return super.doSomethingString(s);
}
@Override
public void inheritedAnnotations() {
super.inheritedAnnotations();
}
}
}

View File

@ -22,6 +22,7 @@ import java.util.function.Supplier;
import org.junit.jupiter.api.Test;
import org.springframework.aop.TargetClassAware;
import org.springframework.core.annotation.AnnotationConfigurationException;
import org.springframework.security.access.annotation.Secured;
import org.springframework.security.access.intercept.method.MockMethodInvocation;
@ -127,6 +128,19 @@ public class SecuredAuthorizationManagerTests {
.isThrownBy(() -> manager.check(authentication, methodInvocation));
}
@Test
public void checkTargetClassAwareWhenInterfaceLevelAnnotationsThenApplies() throws Exception {
MockMethodInvocation methodInvocation = new MockMethodInvocation(new TestTargetClassAware(),
TestTargetClassAware.class, "doSomething");
SecuredAuthorizationManager manager = new SecuredAuthorizationManager();
AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation);
assertThat(decision).isNotNull();
assertThat(decision.isGranted()).isFalse();
decision = manager.check(TestAuthentication::authenticatedAdmin, methodInvocation);
assertThat(decision).isNotNull();
assertThat(decision.isGranted()).isTrue();
}
public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo {
public void doSomething() {
@ -192,4 +206,33 @@ public class SecuredAuthorizationManagerTests {
}
@Secured("ROLE_ADMIN")
public interface InterfaceLevelAnnotations {
}
public static class TestTargetClassAware extends TestClass implements TargetClassAware, InterfaceLevelAnnotations {
@Override
public Class<?> getTargetClass() {
return TestClass.class;
}
@Override
public void doSomething() {
super.doSomething();
}
@Override
public void securedUserOrAdmin() {
super.securedUserOrAdmin();
}
@Override
public void inheritedAnnotations() {
super.inheritedAnnotations();
}
}
}