Skip to content

Commit

Permalink
Mimic Annotation Fallback Logic
Browse files Browse the repository at this point in the history
For backward compatibility, this commit changes the annotation traversal
logic to match what is found in PrePostAnnotationSecurityMetadataSource.

This reverts gh-13783 which is a feature that unfortunately regressess
pre-existing behavior like that found in gh-15352. As such, that
functionality has been removed.

Issue gh-15352
  • Loading branch information
jzheaux committed Jul 31, 2024
1 parent 77bce14 commit 37a2812
Show file tree
Hide file tree
Showing 15 changed files with 84 additions and 192 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
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;
Expand All @@ -50,6 +51,7 @@
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;
Expand Down Expand Up @@ -944,6 +946,13 @@ void getUserWhenNotAuthorizedThenHandlerUsesCustomAuthorizationDecision() {
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<ConfigurableWebApplicationContext> disallowBeanOverriding() {
return (context) -> ((AnnotationConfigWebApplicationContext) context).setAllowBeanDefinitionOverriding(false);
}
Expand Down Expand Up @@ -1480,4 +1489,29 @@ MethodAuthorizationDeniedHandler methodAuthorizationDeniedHandler() {

}

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

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
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;
Expand Down Expand Up @@ -117,9 +116,8 @@ AuthorizationManager<MethodInvocation> resolveManager(Method method, Class<?> ta
}

private Annotation findJsr250Annotation(Method method, Class<?> 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<String> getAllowedRolesWithPrefix(RolesAllowed rolesAllowed) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

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;
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

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;
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

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;
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

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;
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

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;
Expand Down Expand Up @@ -90,8 +89,7 @@ private Set<String> getAuthorities(MethodInvocation methodInvocation) {
}

private Set<String> 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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
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
Expand Down Expand Up @@ -124,11 +125,29 @@ private MergedAnnotation<A> requireUnique(AnnotatedElement element, List<MergedA
}

private List<MergedAnnotation<A>> findMethodAnnotations(Method method, Class<?> targetClass) {
List<MergedAnnotation<A>> 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<MergedAnnotation<A>> 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<MergedAnnotation<A>> findClosestMethodAnnotations(Method method, Class<?> targetClass,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,56 +215,6 @@ public void checkInheritedAnnotationsWhenConflictingThenAnnotationConfigurationE
.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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,29 +156,6 @@ public void checkInheritedAnnotationsWhenConflictingThenAnnotationConfigurationE
.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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,34 +161,6 @@ public Object proceed() {
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 {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,28 +137,6 @@ public void checkTargetClassAwareWhenInterfaceLevelAnnotationsThenApplies() thro
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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,32 +215,6 @@ public void preFilterWhenStaticSecurityContextHolderStrategyAfterConstructorThen
SecurityContextHolder.setContextHolderStrategy(saved);
}

@Test
public void checkPreFilterWhenMethodsFromInheritThenApplies() throws Throwable {
List<String> 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<String> list) {

}

}

@PreFilter("filterObject == 'john'")
public static class TestClass implements InterfaceAnnotationsOne, InterfaceAnnotationsTwo {

Expand Down
Loading

0 comments on commit 37a2812

Please sign in to comment.