mirror of
				https://github.com/spring-projects/spring-security.git
				synced 2025-10-31 06:38:42 +00:00 
			
		
		
		
	@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:
		
							parent
							
								
									3cbb60750d
								
							
						
					
					
						commit
						66bbfc7a50
					
				| @ -22,7 +22,6 @@ import java.util.concurrent.ConcurrentHashMap; | |||||||
| 
 | 
 | ||||||
| import org.aopalliance.intercept.MethodInvocation; | import org.aopalliance.intercept.MethodInvocation; | ||||||
| 
 | 
 | ||||||
| import org.springframework.aop.support.AopUtils; |  | ||||||
| import org.springframework.core.MethodClassKey; | import org.springframework.core.MethodClassKey; | ||||||
| import org.springframework.lang.NonNull; | import org.springframework.lang.NonNull; | ||||||
| import org.springframework.security.authorization.AuthorizationManager; | import org.springframework.security.authorization.AuthorizationManager; | ||||||
| @ -46,7 +45,7 @@ abstract class AbstractAuthorizationManagerRegistry { | |||||||
| 	final AuthorizationManager<MethodInvocation> getManager(MethodInvocation methodInvocation) { | 	final AuthorizationManager<MethodInvocation> getManager(MethodInvocation methodInvocation) { | ||||||
| 		Method method = methodInvocation.getMethod(); | 		Method method = methodInvocation.getMethod(); | ||||||
| 		Object target = methodInvocation.getThis(); | 		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); | 		MethodClassKey cacheKey = new MethodClassKey(method, targetClass); | ||||||
| 		return this.cachedManagers.computeIfAbsent(cacheKey, (k) -> resolveManager(method, targetClass)); | 		return this.cachedManagers.computeIfAbsent(cacheKey, (k) -> resolveManager(method, targetClass)); | ||||||
| 	} | 	} | ||||||
|  | |||||||
| @ -22,7 +22,6 @@ import java.util.concurrent.ConcurrentHashMap; | |||||||
| 
 | 
 | ||||||
| import org.aopalliance.intercept.MethodInvocation; | import org.aopalliance.intercept.MethodInvocation; | ||||||
| 
 | 
 | ||||||
| import org.springframework.aop.support.AopUtils; |  | ||||||
| import org.springframework.core.MethodClassKey; | import org.springframework.core.MethodClassKey; | ||||||
| import org.springframework.lang.NonNull; | import org.springframework.lang.NonNull; | ||||||
| 
 | 
 | ||||||
| @ -43,7 +42,7 @@ abstract class AbstractExpressionAttributeRegistry<T extends ExpressionAttribute | |||||||
| 	final T getAttribute(MethodInvocation mi) { | 	final T getAttribute(MethodInvocation mi) { | ||||||
| 		Method method = mi.getMethod(); | 		Method method = mi.getMethod(); | ||||||
| 		Object target = mi.getThis(); | 		Object target = mi.getThis(); | ||||||
| 		Class<?> targetClass = (target != null) ? AopUtils.getTargetClass(target) : null; | 		Class<?> targetClass = (target != null) ? target.getClass() : null; | ||||||
| 		return getAttribute(method, targetClass); | 		return getAttribute(method, targetClass); | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
|  | |||||||
| @ -22,6 +22,7 @@ import java.util.function.Supplier; | |||||||
| 
 | 
 | ||||||
| import org.junit.jupiter.api.Test; | import org.junit.jupiter.api.Test; | ||||||
| 
 | 
 | ||||||
|  | import org.springframework.aop.TargetClassAware; | ||||||
| import org.springframework.core.annotation.AnnotationConfigurationException; | import org.springframework.core.annotation.AnnotationConfigurationException; | ||||||
| import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler; | import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler; | ||||||
| import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler; | import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler; | ||||||
| @ -133,6 +134,19 @@ public class PreAuthorizeAuthorizationManagerTests { | |||||||
| 				.isThrownBy(() -> manager.check(authentication, methodInvocation)); | 				.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 static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { | ||||||
| 
 | 
 | ||||||
| 		public void doSomething() { | 		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(); | ||||||
|  | 		} | ||||||
|  | 
 | ||||||
|  | 	} | ||||||
|  | 
 | ||||||
| } | } | ||||||
|  | |||||||
| @ -22,6 +22,7 @@ import java.util.function.Supplier; | |||||||
| 
 | 
 | ||||||
| import org.junit.jupiter.api.Test; | import org.junit.jupiter.api.Test; | ||||||
| 
 | 
 | ||||||
|  | import org.springframework.aop.TargetClassAware; | ||||||
| import org.springframework.core.annotation.AnnotationConfigurationException; | import org.springframework.core.annotation.AnnotationConfigurationException; | ||||||
| import org.springframework.security.access.annotation.Secured; | import org.springframework.security.access.annotation.Secured; | ||||||
| import org.springframework.security.access.intercept.method.MockMethodInvocation; | import org.springframework.security.access.intercept.method.MockMethodInvocation; | ||||||
| @ -127,6 +128,19 @@ public class SecuredAuthorizationManagerTests { | |||||||
| 				.isThrownBy(() -> manager.check(authentication, methodInvocation)); | 				.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 static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { | ||||||
| 
 | 
 | ||||||
| 		public void doSomething() { | 		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(); | ||||||
|  | 		} | ||||||
|  | 
 | ||||||
|  | 	} | ||||||
|  | 
 | ||||||
| } | } | ||||||
|  | |||||||
		Loading…
	
	
			
			x
			
			
		
	
		Reference in New Issue
	
	Block a user