Skip to content

Commit

Permalink
Revert gh-13783
Browse files Browse the repository at this point in the history
This feature unfortunately regresses pre-existing behavior
like that found in gh-15352. As such, this functionality
has been removed.

Closes gh-15352
  • Loading branch information
jzheaux committed Jul 31, 2024
1 parent f059c05 commit f20ae1a
Show file tree
Hide file tree
Showing 14 changed files with 70 additions and 219 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
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2021 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -35,7 +35,6 @@
* For internal use only, as this contract is likely to change
*
* @author Evgeniy Cheban
* @author DingHao
*/
abstract class AbstractExpressionAttributeRegistry<T extends ExpressionAttribute> {

Expand Down Expand Up @@ -100,8 +99,4 @@ void setTemplateDefaults(PrePostTemplateDefaults defaults) {
@NonNull
abstract T resolveAttribute(Method method, Class<?> targetClass);

Class<?> targetClass(Method method, Class<?> targetClass) {
return (targetClass != null) ? targetClass : method.getDeclaringClass();
}

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -44,7 +44,6 @@
*
* @author Evgeniy Cheban
* @author Josh Cummings
* @author DingHao
* @since 5.6
*/
public final class Jsr250AuthorizationManager implements AuthorizationManager<MethodInvocation> {
Expand Down Expand Up @@ -122,8 +121,7 @@ AuthorizationManager<MethodInvocation> resolveManager(Method method, Class<?> ta
private Annotation findJsr250Annotation(Method method, Class<?> targetClass) {
Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
Annotation annotation = findAnnotation(specificMethod);
return (annotation != null) ? annotation
: findAnnotation((targetClass != null) ? targetClass : specificMethod.getDeclaringClass());
return (annotation != null) ? annotation : findAnnotation(specificMethod.getDeclaringClass());
}

private Annotation findAnnotation(Method method) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -33,7 +33,6 @@
* For internal use only, as this contract is likely to change.
*
* @author Evgeniy Cheban
* @author DingHao
* @since 5.8
*/
final class PostAuthorizeExpressionAttributeRegistry extends AbstractExpressionAttributeRegistry<ExpressionAttribute> {
Expand All @@ -50,33 +49,33 @@ final class PostAuthorizeExpressionAttributeRegistry extends AbstractExpressionA
@Override
ExpressionAttribute resolveAttribute(Method method, Class<?> targetClass) {
Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
PostAuthorize postAuthorize = findPostAuthorizeAnnotation(specificMethod, targetClass);
PostAuthorize postAuthorize = findPostAuthorizeAnnotation(specificMethod);
if (postAuthorize == null) {
return ExpressionAttribute.NULL_ATTRIBUTE;
}
Expression expression = getExpressionHandler().getExpressionParser().parseExpression(postAuthorize.value());
MethodAuthorizationDeniedHandler deniedHandler = resolveHandler(method, targetClass);
MethodAuthorizationDeniedHandler deniedHandler = resolveHandler(method);
return new PostAuthorizeExpressionAttribute(expression, deniedHandler);
}

private MethodAuthorizationDeniedHandler resolveHandler(Method method, Class<?> targetClass) {
private MethodAuthorizationDeniedHandler resolveHandler(Method method) {
Function<AnnotatedElement, HandleAuthorizationDenied> lookup = AuthorizationAnnotationUtils
.withDefaults(HandleAuthorizationDenied.class);
HandleAuthorizationDenied deniedHandler = lookup.apply(method);
if (deniedHandler != null) {
return this.handlerResolver.apply(deniedHandler.handlerClass());
}
deniedHandler = lookup.apply(targetClass(method, targetClass));
deniedHandler = lookup.apply(method.getDeclaringClass());
if (deniedHandler != null) {
return this.handlerResolver.apply(deniedHandler.handlerClass());
}
return this.defaultHandler;
}

private PostAuthorize findPostAuthorizeAnnotation(Method method, Class<?> targetClass) {
private PostAuthorize findPostAuthorizeAnnotation(Method method) {
Function<AnnotatedElement, PostAuthorize> lookup = findUniqueAnnotation(PostAuthorize.class);
PostAuthorize postAuthorize = lookup.apply(method);
return (postAuthorize != null) ? postAuthorize : lookup.apply(targetClass(method, targetClass));
return (postAuthorize != null) ? postAuthorize : lookup.apply(method.getDeclaringClass());
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -29,7 +29,6 @@
* For internal use only, as this contract is likely to change.
*
* @author Evgeniy Cheban
* @author DingHao
* @since 5.8
*/
final class PostFilterExpressionAttributeRegistry extends AbstractExpressionAttributeRegistry<ExpressionAttribute> {
Expand All @@ -38,7 +37,7 @@ final class PostFilterExpressionAttributeRegistry extends AbstractExpressionAttr
@Override
ExpressionAttribute resolveAttribute(Method method, Class<?> targetClass) {
Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
PostFilter postFilter = findPostFilterAnnotation(specificMethod, targetClass);
PostFilter postFilter = findPostFilterAnnotation(specificMethod);
if (postFilter == null) {
return ExpressionAttribute.NULL_ATTRIBUTE;
}
Expand All @@ -47,10 +46,10 @@ ExpressionAttribute resolveAttribute(Method method, Class<?> targetClass) {
return new ExpressionAttribute(postFilterExpression);
}

private PostFilter findPostFilterAnnotation(Method method, Class<?> targetClass) {
private PostFilter findPostFilterAnnotation(Method method) {
Function<AnnotatedElement, PostFilter> lookup = findUniqueAnnotation(PostFilter.class);
PostFilter postFilter = lookup.apply(method);
return (postFilter != null) ? postFilter : lookup.apply(targetClass(method, targetClass));
return (postFilter != null) ? postFilter : lookup.apply(method.getDeclaringClass());
}

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -33,7 +33,6 @@
* For internal use only, as this contract is likely to change.
*
* @author Evgeniy Cheban
* @author DingHao
* @since 5.8
*/
final class PreAuthorizeExpressionAttributeRegistry extends AbstractExpressionAttributeRegistry<ExpressionAttribute> {
Expand All @@ -50,33 +49,33 @@ final class PreAuthorizeExpressionAttributeRegistry extends AbstractExpressionAt
@Override
ExpressionAttribute resolveAttribute(Method method, Class<?> targetClass) {
Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
PreAuthorize preAuthorize = findPreAuthorizeAnnotation(specificMethod, targetClass);
PreAuthorize preAuthorize = findPreAuthorizeAnnotation(specificMethod);
if (preAuthorize == null) {
return ExpressionAttribute.NULL_ATTRIBUTE;
}
Expression expression = getExpressionHandler().getExpressionParser().parseExpression(preAuthorize.value());
MethodAuthorizationDeniedHandler handler = resolveHandler(method, targetClass);
MethodAuthorizationDeniedHandler handler = resolveHandler(method);
return new PreAuthorizeExpressionAttribute(expression, handler);
}

private MethodAuthorizationDeniedHandler resolveHandler(Method method, Class<?> targetClass) {
private MethodAuthorizationDeniedHandler resolveHandler(Method method) {
Function<AnnotatedElement, HandleAuthorizationDenied> lookup = AuthorizationAnnotationUtils
.withDefaults(HandleAuthorizationDenied.class);
HandleAuthorizationDenied deniedHandler = lookup.apply(method);
if (deniedHandler != null) {
return this.handlerResolver.apply(deniedHandler.handlerClass());
}
deniedHandler = lookup.apply(targetClass(method, targetClass));
deniedHandler = lookup.apply(method.getDeclaringClass());
if (deniedHandler != null) {
return this.handlerResolver.apply(deniedHandler.handlerClass());
}
return this.defaultHandler;
}

private PreAuthorize findPreAuthorizeAnnotation(Method method, Class<?> targetClass) {
private PreAuthorize findPreAuthorizeAnnotation(Method method) {
Function<AnnotatedElement, PreAuthorize> lookup = findUniqueAnnotation(PreAuthorize.class);
PreAuthorize preAuthorize = lookup.apply(method);
return (preAuthorize != null) ? preAuthorize : lookup.apply(targetClass(method, targetClass));
return (preAuthorize != null) ? preAuthorize : lookup.apply(method.getDeclaringClass());
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -29,7 +29,6 @@
* For internal use only, as this contract is likely to change.
*
* @author Evgeniy Cheban
* @author DingHao
* @since 5.8
*/
final class PreFilterExpressionAttributeRegistry
Expand All @@ -39,7 +38,7 @@ final class PreFilterExpressionAttributeRegistry
@Override
PreFilterExpressionAttribute resolveAttribute(Method method, Class<?> targetClass) {
Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
PreFilter preFilter = findPreFilterAnnotation(specificMethod, targetClass);
PreFilter preFilter = findPreFilterAnnotation(specificMethod);
if (preFilter == null) {
return PreFilterExpressionAttribute.NULL_ATTRIBUTE;
}
Expand All @@ -48,10 +47,10 @@ PreFilterExpressionAttribute resolveAttribute(Method method, Class<?> targetClas
return new PreFilterExpressionAttribute(preFilterExpression, preFilter.filterTarget());
}

private PreFilter findPreFilterAnnotation(Method method, Class<?> targetClass) {
private PreFilter findPreFilterAnnotation(Method method) {
Function<AnnotatedElement, PreFilter> lookup = findUniqueAnnotation(PreFilter.class);
PreFilter preFilter = lookup.apply(method);
return (preFilter != null) ? preFilter : lookup.apply(targetClass(method, targetClass));
return (preFilter != null) ? preFilter : lookup.apply(method.getDeclaringClass());
}

static final class PreFilterExpressionAttribute extends ExpressionAttribute {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -41,7 +41,6 @@
* contains a specified authority from the Spring Security's {@link Secured} annotation.
*
* @author Evgeniy Cheban
* @author DingHao
* @since 5.6
*/
public final class SecuredAuthorizationManager implements AuthorizationManager<MethodInvocation> {
Expand Down Expand Up @@ -87,14 +86,14 @@ 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(specificMethod);
return (secured != null) ? Set.of(secured.value()) : Collections.emptySet();
}

private Secured findSecuredAnnotation(Method method, Class<?> targetClass) {
private Secured findSecuredAnnotation(Method method) {
Secured secured = AuthorizationAnnotationUtils.findUniqueAnnotation(method, Secured.class);
return (secured != null) ? secured : AuthorizationAnnotationUtils
.findUniqueAnnotation((targetClass != null) ? targetClass : method.getDeclaringClass(), Secured.class);
return (secured != null) ? secured
: AuthorizationAnnotationUtils.findUniqueAnnotation(method.getDeclaringClass(), Secured.class);
}

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -225,56 +225,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
Loading

0 comments on commit f20ae1a

Please sign in to comment.