diff --git a/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java b/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java index 69a42a7e47..a2f82afe6d 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java @@ -28,6 +28,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.function.Consumer; import java.util.function.Supplier; +import jakarta.annotation.security.DenyAll; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; import org.junit.jupiter.api.Test; @@ -50,6 +51,7 @@ import org.springframework.security.access.annotation.BusinessService; import org.springframework.security.access.annotation.BusinessServiceImpl; import org.springframework.security.access.annotation.ExpressionProtectedBusinessServiceImpl; import org.springframework.security.access.annotation.Jsr250BusinessServiceImpl; +import org.springframework.security.access.annotation.Secured; import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler; import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler; import org.springframework.security.access.hierarchicalroles.RoleHierarchy; @@ -944,6 +946,13 @@ public class PrePostMethodSecurityConfigurationTests { verify(handler, never()).handleDeniedInvocation(any(), any(Authz.AuthzResult.class)); } + // gh-15352 + @Test + void annotationsInChildClassesDoNotAffectSuperclasses() { + this.spring.register(AbstractClassConfig.class).autowire(); + this.spring.getContext().getBean(ClassInheritingAbstractClassWithNoAnnotations.class).method(); + } + private static Consumer disallowBeanOverriding() { return (context) -> ((AnnotationConfigWebApplicationContext) context).setAllowBeanDefinitionOverriding(false); } @@ -1480,4 +1489,29 @@ public class PrePostMethodSecurityConfigurationTests { } + abstract static class AbstractClassWithNoAnnotations { + + String method() { + return "ok"; + } + + } + + @PreAuthorize("denyAll()") + @Secured("DENIED") + @DenyAll + static class ClassInheritingAbstractClassWithNoAnnotations extends AbstractClassWithNoAnnotations { + + } + + @EnableMethodSecurity(securedEnabled = true, jsr250Enabled = true) + static class AbstractClassConfig { + + @Bean + ClassInheritingAbstractClassWithNoAnnotations inheriting() { + return new ClassInheritingAbstractClassWithNoAnnotations(); + } + + } + } diff --git a/core/src/main/java/org/springframework/security/authorization/method/Jsr250AuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/method/Jsr250AuthorizationManager.java index 34f6b20ca9..1af1edc0aa 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/Jsr250AuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/method/Jsr250AuthorizationManager.java @@ -29,7 +29,6 @@ import jakarta.annotation.security.PermitAll; import jakarta.annotation.security.RolesAllowed; import org.aopalliance.intercept.MethodInvocation; -import org.springframework.aop.support.AopUtils; import org.springframework.lang.NonNull; import org.springframework.security.authorization.AuthoritiesAuthorizationManager; import org.springframework.security.authorization.AuthorizationDecision; @@ -117,9 +116,8 @@ public final class Jsr250AuthorizationManager implements AuthorizationManager targetClass) { - Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass); - Class targetClassToUse = (targetClass != null) ? targetClass : specificMethod.getDeclaringClass(); - return this.synthesizer.synthesize(specificMethod, targetClassToUse); + Class targetClassToUse = (targetClass != null) ? targetClass : method.getDeclaringClass(); + return this.synthesizer.synthesize(method, targetClassToUse); } private Set getAllowedRolesWithPrefix(RolesAllowed rolesAllowed) { diff --git a/core/src/main/java/org/springframework/security/authorization/method/PostAuthorizeExpressionAttributeRegistry.java b/core/src/main/java/org/springframework/security/authorization/method/PostAuthorizeExpressionAttributeRegistry.java index 0152581f1b..b68ec67d62 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/PostAuthorizeExpressionAttributeRegistry.java +++ b/core/src/main/java/org/springframework/security/authorization/method/PostAuthorizeExpressionAttributeRegistry.java @@ -22,7 +22,6 @@ import java.util.function.Function; import reactor.util.annotation.NonNull; -import org.springframework.aop.support.AopUtils; import org.springframework.context.ApplicationContext; import org.springframework.expression.Expression; import org.springframework.security.access.prepost.PostAuthorize; @@ -56,8 +55,7 @@ final class PostAuthorizeExpressionAttributeRegistry extends AbstractExpressionA @NonNull @Override ExpressionAttribute resolveAttribute(Method method, Class targetClass) { - Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass); - PostAuthorize postAuthorize = findPostAuthorizeAnnotation(specificMethod, targetClass); + PostAuthorize postAuthorize = findPostAuthorizeAnnotation(method, targetClass); if (postAuthorize == null) { return ExpressionAttribute.NULL_ATTRIBUTE; } diff --git a/core/src/main/java/org/springframework/security/authorization/method/PostFilterExpressionAttributeRegistry.java b/core/src/main/java/org/springframework/security/authorization/method/PostFilterExpressionAttributeRegistry.java index 6859e90b27..2d8e316e71 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/PostFilterExpressionAttributeRegistry.java +++ b/core/src/main/java/org/springframework/security/authorization/method/PostFilterExpressionAttributeRegistry.java @@ -18,7 +18,6 @@ package org.springframework.security.authorization.method; import java.lang.reflect.Method; -import org.springframework.aop.support.AopUtils; import org.springframework.expression.Expression; import org.springframework.lang.NonNull; import org.springframework.security.access.prepost.PostFilter; @@ -39,8 +38,7 @@ final class PostFilterExpressionAttributeRegistry extends AbstractExpressionAttr @NonNull @Override ExpressionAttribute resolveAttribute(Method method, Class targetClass) { - Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass); - PostFilter postFilter = findPostFilterAnnotation(specificMethod, targetClass); + PostFilter postFilter = findPostFilterAnnotation(method, targetClass); if (postFilter == null) { return ExpressionAttribute.NULL_ATTRIBUTE; } diff --git a/core/src/main/java/org/springframework/security/authorization/method/PreAuthorizeExpressionAttributeRegistry.java b/core/src/main/java/org/springframework/security/authorization/method/PreAuthorizeExpressionAttributeRegistry.java index c93334a620..1b23f46278 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/PreAuthorizeExpressionAttributeRegistry.java +++ b/core/src/main/java/org/springframework/security/authorization/method/PreAuthorizeExpressionAttributeRegistry.java @@ -22,7 +22,6 @@ import java.util.function.Function; import reactor.util.annotation.NonNull; -import org.springframework.aop.support.AopUtils; import org.springframework.context.ApplicationContext; import org.springframework.expression.Expression; import org.springframework.security.access.prepost.PreAuthorize; @@ -56,8 +55,7 @@ final class PreAuthorizeExpressionAttributeRegistry extends AbstractExpressionAt @NonNull @Override ExpressionAttribute resolveAttribute(Method method, Class targetClass) { - Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass); - PreAuthorize preAuthorize = findPreAuthorizeAnnotation(specificMethod, targetClass); + PreAuthorize preAuthorize = findPreAuthorizeAnnotation(method, targetClass); if (preAuthorize == null) { return ExpressionAttribute.NULL_ATTRIBUTE; } diff --git a/core/src/main/java/org/springframework/security/authorization/method/PreFilterExpressionAttributeRegistry.java b/core/src/main/java/org/springframework/security/authorization/method/PreFilterExpressionAttributeRegistry.java index 06b8b32a14..eda0374486 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/PreFilterExpressionAttributeRegistry.java +++ b/core/src/main/java/org/springframework/security/authorization/method/PreFilterExpressionAttributeRegistry.java @@ -18,7 +18,6 @@ package org.springframework.security.authorization.method; import java.lang.reflect.Method; -import org.springframework.aop.support.AopUtils; import org.springframework.expression.Expression; import org.springframework.lang.NonNull; import org.springframework.security.access.prepost.PreFilter; @@ -40,8 +39,7 @@ final class PreFilterExpressionAttributeRegistry @NonNull @Override PreFilterExpressionAttribute resolveAttribute(Method method, Class targetClass) { - Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass); - PreFilter preFilter = findPreFilterAnnotation(specificMethod, targetClass); + PreFilter preFilter = findPreFilterAnnotation(method, targetClass); if (preFilter == null) { return PreFilterExpressionAttribute.NULL_ATTRIBUTE; } diff --git a/core/src/main/java/org/springframework/security/authorization/method/SecuredAuthorizationManager.java b/core/src/main/java/org/springframework/security/authorization/method/SecuredAuthorizationManager.java index e064234617..9d1d800577 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/SecuredAuthorizationManager.java +++ b/core/src/main/java/org/springframework/security/authorization/method/SecuredAuthorizationManager.java @@ -26,7 +26,6 @@ import java.util.function.Supplier; import org.aopalliance.intercept.MethodInvocation; -import org.springframework.aop.support.AopUtils; import org.springframework.core.MethodClassKey; import org.springframework.security.access.annotation.Secured; import org.springframework.security.authorization.AuthoritiesAuthorizationManager; @@ -90,8 +89,7 @@ public final class SecuredAuthorizationManager implements AuthorizationManager resolveAuthorities(Method method, Class targetClass) { - Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass); - Secured secured = findSecuredAnnotation(specificMethod, targetClass); + Secured secured = findSecuredAnnotation(method, targetClass); return (secured != null) ? Set.of(secured.value()) : Collections.emptySet(); } diff --git a/core/src/main/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizer.java b/core/src/main/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizer.java index 340fec9556..6eb5bfb237 100644 --- a/core/src/main/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizer.java +++ b/core/src/main/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizer.java @@ -31,6 +31,7 @@ import org.springframework.core.annotation.MergedAnnotation; import org.springframework.core.annotation.MergedAnnotations; import org.springframework.core.annotation.RepeatableContainers; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; /** * A strategy for synthesizing an annotation from an {@link AnnotatedElement} that @@ -124,11 +125,29 @@ final class UniqueMergedAnnotationSynthesizer implements A } private List> findMethodAnnotations(Method method, Class targetClass) { - List> annotations = findClosestMethodAnnotations(method, targetClass, new HashSet<>()); + // The method may be on an interface, but we need attributes from the target + // class. + // If the target class is null, the method will be unchanged. + Method specificMethod = ClassUtils.getMostSpecificMethod(method, targetClass); + List> annotations = findClosestMethodAnnotations(specificMethod, + specificMethod.getDeclaringClass(), new HashSet<>()); if (!annotations.isEmpty()) { return annotations; } - return findClosestClassAnnotations(targetClass, new HashSet<>()); + // Check the original (e.g. interface) method + if (specificMethod != method) { + annotations = findClosestMethodAnnotations(method, method.getDeclaringClass(), new HashSet<>()); + if (!annotations.isEmpty()) { + return annotations; + } + } + // Check the class-level (note declaringClass, not targetClass, which may not + // actually implement the method) + annotations = findClosestClassAnnotations(specificMethod.getDeclaringClass(), new HashSet<>()); + if (!annotations.isEmpty()) { + return annotations; + } + return Collections.emptyList(); } private List> findClosestMethodAnnotations(Method method, Class targetClass, diff --git a/core/src/test/java/org/springframework/security/authorization/method/Jsr250AuthorizationManagerTests.java b/core/src/test/java/org/springframework/security/authorization/method/Jsr250AuthorizationManagerTests.java index 54eb918c98..ba7324d6a5 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/Jsr250AuthorizationManagerTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/Jsr250AuthorizationManagerTests.java @@ -215,56 +215,6 @@ public class Jsr250AuthorizationManagerTests { .isThrownBy(() -> manager.check(authentication, methodInvocation)); } - @Test - public void checkRequiresUserWhenMethodsFromInheritThenApplies() throws Exception { - MockMethodInvocation methodInvocation = new MockMethodInvocation(new RolesAllowedClass(), - RolesAllowedClass.class, "securedUser"); - Jsr250AuthorizationManager manager = new Jsr250AuthorizationManager(); - AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation); - assertThat(decision.isGranted()).isTrue(); - } - - @Test - public void checkPermitAllWhenMethodsFromInheritThenApplies() throws Exception { - MockMethodInvocation methodInvocation = new MockMethodInvocation(new PermitAllClass(), PermitAllClass.class, - "securedUser"); - Jsr250AuthorizationManager manager = new Jsr250AuthorizationManager(); - AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation); - assertThat(decision.isGranted()).isTrue(); - } - - @Test - public void checkDenyAllWhenMethodsFromInheritThenApplies() throws Exception { - MockMethodInvocation methodInvocation = new MockMethodInvocation(new DenyAllClass(), DenyAllClass.class, - "securedUser"); - Jsr250AuthorizationManager manager = new Jsr250AuthorizationManager(); - AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation); - assertThat(decision.isGranted()).isFalse(); - } - - @RolesAllowed("USER") - public static class RolesAllowedClass extends ParentClass { - - } - - @PermitAll - public static class PermitAllClass extends ParentClass { - - } - - @DenyAll - public static class DenyAllClass extends ParentClass { - - } - - public static class ParentClass { - - public void securedUser() { - - } - - } - public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { public void doSomething() { diff --git a/core/src/test/java/org/springframework/security/authorization/method/PostAuthorizeAuthorizationManagerTests.java b/core/src/test/java/org/springframework/security/authorization/method/PostAuthorizeAuthorizationManagerTests.java index 952338b184..e3d28b1172 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/PostAuthorizeAuthorizationManagerTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/PostAuthorizeAuthorizationManagerTests.java @@ -156,29 +156,6 @@ public class PostAuthorizeAuthorizationManagerTests { .isThrownBy(() -> manager.check(authentication, result)); } - @Test - public void checkRequiresUserWhenMethodsFromInheritThenApplies() throws Exception { - MockMethodInvocation methodInvocation = new MockMethodInvocation(new PostAuthorizeClass(), - PostAuthorizeClass.class, "securedUser"); - MethodInvocationResult result = new MethodInvocationResult(methodInvocation, null); - PostAuthorizeAuthorizationManager manager = new PostAuthorizeAuthorizationManager(); - AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, result); - assertThat(decision.isGranted()).isTrue(); - } - - @PostAuthorize("hasRole('USER')") - public static class PostAuthorizeClass extends ParentClass { - - } - - public static class ParentClass { - - public void securedUser() { - - } - - } - public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { public void doSomething() { diff --git a/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java index 341b7bc720..07b520dcf4 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/PostFilterAuthorizationMethodInterceptorTests.java @@ -161,34 +161,6 @@ public class PostFilterAuthorizationMethodInterceptorTests { SecurityContextHolder.setContextHolderStrategy(saved); } - @Test - public void checkPostFilterWhenMethodsFromInheritThenApplies() throws Throwable { - String[] array = { "john", "bob" }; - MockMethodInvocation methodInvocation = new MockMethodInvocation(new PostFilterClass(), PostFilterClass.class, - "inheritMethod", new Class[] { String[].class }, new Object[] { array }) { - @Override - public Object proceed() { - return array; - } - }; - PostFilterAuthorizationMethodInterceptor advice = new PostFilterAuthorizationMethodInterceptor(); - Object result = advice.invoke(methodInvocation); - assertThat(result).asInstanceOf(InstanceOfAssertFactories.array(String[].class)).containsOnly("john"); - } - - @PostFilter("filterObject == 'john'") - public static class PostFilterClass extends ParentClass { - - } - - public static class ParentClass { - - public String[] inheritMethod(String[] array) { - return array; - } - - } - @PostFilter("filterObject == 'john'") public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { diff --git a/core/src/test/java/org/springframework/security/authorization/method/PreAuthorizeAuthorizationManagerTests.java b/core/src/test/java/org/springframework/security/authorization/method/PreAuthorizeAuthorizationManagerTests.java index 13ae1457f9..881410c189 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/PreAuthorizeAuthorizationManagerTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/PreAuthorizeAuthorizationManagerTests.java @@ -137,28 +137,6 @@ public class PreAuthorizeAuthorizationManagerTests { assertThat(decision.isGranted()).isTrue(); } - @Test - public void checkRequiresUserWhenMethodsFromInheritThenApplies() throws Exception { - MockMethodInvocation methodInvocation = new MockMethodInvocation(new PreAuthorizeClass(), - PreAuthorizeClass.class, "securedUser"); - PreAuthorizeAuthorizationManager manager = new PreAuthorizeAuthorizationManager(); - AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation); - assertThat(decision.isGranted()).isTrue(); - } - - @PreAuthorize("hasRole('USER')") - public static class PreAuthorizeClass extends ParentClass { - - } - - public static class ParentClass { - - public void securedUser() { - - } - - } - public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { public void doSomething() { diff --git a/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java index c86d0a0675..45080449c3 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/PreFilterAuthorizationMethodInterceptorTests.java @@ -215,32 +215,6 @@ public class PreFilterAuthorizationMethodInterceptorTests { SecurityContextHolder.setContextHolderStrategy(saved); } - @Test - public void checkPreFilterWhenMethodsFromInheritThenApplies() throws Throwable { - List list = new ArrayList<>(); - list.add("john"); - list.add("bob"); - MockMethodInvocation invocation = new MockMethodInvocation(new PreFilterClass(), PreFilterClass.class, - "inheritMethod", new Class[] { List.class }, new Object[] { list }); - PreFilterAuthorizationMethodInterceptor advice = new PreFilterAuthorizationMethodInterceptor(); - advice.invoke(invocation); - assertThat(list).hasSize(1); - assertThat(list.get(0)).isEqualTo("john"); - } - - @PreFilter("filterObject == 'john'") - public static class PreFilterClass extends ParentClass { - - } - - public static class ParentClass { - - public void inheritMethod(List list) { - - } - - } - @PreFilter("filterObject == 'john'") public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { diff --git a/core/src/test/java/org/springframework/security/authorization/method/SecuredAuthorizationManagerTests.java b/core/src/test/java/org/springframework/security/authorization/method/SecuredAuthorizationManagerTests.java index 0d4e6a921b..1e353c7078 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/SecuredAuthorizationManagerTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/SecuredAuthorizationManagerTests.java @@ -167,28 +167,6 @@ public class SecuredAuthorizationManagerTests { assertThat(decision.isGranted()).isTrue(); } - @Test - public void checkRequiresUserWhenMethodsFromInheritThenApplies() throws Exception { - MockMethodInvocation methodInvocation = new MockMethodInvocation(new SecuredSonClass(), SecuredSonClass.class, - "securedUser"); - SecuredAuthorizationManager manager = new SecuredAuthorizationManager(); - AuthorizationDecision decision = manager.check(TestAuthentication::authenticatedUser, methodInvocation); - assertThat(decision.isGranted()).isTrue(); - } - - @Secured("ROLE_USER") - public static class SecuredSonClass extends ParentClass { - - } - - public static class ParentClass { - - public void securedUser() { - - } - - } - public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo { public void doSomething() { diff --git a/core/src/test/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizerTests.java b/core/src/test/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizerTests.java index 56fe18c52b..444dc04e83 100644 --- a/core/src/test/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizerTests.java +++ b/core/src/test/java/org/springframework/security/core/annotation/UniqueMergedAnnotationSynthesizerTests.java @@ -242,6 +242,15 @@ public class UniqueMergedAnnotationSynthesizerTests { assertThat(preAuthorize.value()).isEqualTo("three"); } + // gh-15352 + @Test + void synthesizeWhenClassInheritingAbstractClassNoAnnotationsThenNoAnnotation() throws Exception { + Method method = ClassInheritingAbstractClassNoAnnotations.class.getMethod("otherMethod"); + Class targetClass = ClassInheritingAbstractClassNoAnnotations.class; + PreAuthorize preAuthorize = this.synthesizer.synthesize(method, targetClass); + assertThat(preAuthorize).isNull(); + } + @PreAuthorize("one") private interface AnnotationOnInterface { @@ -555,4 +564,17 @@ public class UniqueMergedAnnotationSynthesizerTests { } + public abstract static class AbstractClassNoAnnotations { + + public String otherMethod() { + return "ok"; + } + + } + + @PreAuthorize("twentynine") + private static class ClassInheritingAbstractClassNoAnnotations extends AbstractClassNoAnnotations { + + } + }