Browse Source

@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
Evgeniy Cheban 3 năm trước cách đây
mục cha
commit
9193e46800

+ 1 - 2
core/src/main/java/org/springframework/security/authorization/method/AbstractAuthorizationManagerRegistry.java

@@ -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));
 	}

+ 1 - 2
core/src/main/java/org/springframework/security/authorization/method/AbstractExpressionAttributeRegistry.java

@@ -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);
 	}
 

+ 43 - 0
core/src/test/java/org/springframework/security/authorization/method/PreAuthorizeAuthorizationManagerTests.java

@@ -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();
+		}
+
+	}
+
 }

+ 43 - 0
core/src/test/java/org/springframework/security/authorization/method/SecuredAuthorizationManagerTests.java

@@ -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();
+		}
+
+	}
+
 }