Revise AuthorizationAnnotationUtils

core/src/main/java/org/springframework/security/authorization/method/AuthorizationAnnotationUtils.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 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.
@@ -17,31 +17,36 @@
 package org.springframework.security.authorization.method;
 
 import java.lang.annotation.Annotation;
-import java.lang.reflect.Executable;
+import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.Method;
+import java.util.List;
 
 import org.springframework.core.annotation.AnnotationConfigurationException;
-import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.core.annotation.MergedAnnotation;
 import org.springframework.core.annotation.MergedAnnotations;
+import org.springframework.core.annotation.MergedAnnotations.SearchStrategy;
 import org.springframework.core.annotation.RepeatableContainers;
 
 /**
- * A wrapper around {@link AnnotationUtils} that checks for, and errors on, conflicting
- * annotations. This is specifically important for Spring Security annotations which are
- * not designed to be repeatable.
+ * A collection of utility methods that check for, and error on, conflicting annotations.
+ * This is specifically important for Spring Security annotations which are not designed
+ * to be repeatable.
  *
+ * <p>
  * There are numerous ways that two annotations of the same type may be attached to the
  * same method. For example, a class may implement a method defined in two separate
- * interfaces. If both of those interfaces have a `@PreAuthorize` annotation, then it's
- * unclear which `@PreAuthorize` expression Spring Security should use.
+ * interfaces. If both of those interfaces have a {@code @PreAuthorize} annotation, then
+ * it's unclear which {@code @PreAuthorize} expression Spring Security should use.
  *
+ * <p>
  * Another way is when one of Spring Security's annotations is used as a meta-annotation.
  * In that case, two custom annotations can be declared, each with their own
- * `@PreAuthorize` declaration. If both custom annotations are used on the same method,
- * then it's unclear which `@PreAuthorize` expression Spring Security should use.
+ * {@code @PreAuthorize} declaration. If both custom annotations are used on the same
+ * method, then it's unclear which {@code @PreAuthorize} expression Spring Security should
+ * use.
  *
  * @author Josh Cummings
+ * @author Sam Brannen
  */
 final class AuthorizationAnnotationUtils {
 
@@ -50,84 +55,56 @@ final class AuthorizationAnnotationUtils {
 	 * the annotation of type {@code annotationType}, including any annotations using
 	 * {@code annotationType} as a meta-annotation.
 	 *
-	 * If more than one is found, then throw an error.
+	 * <p>
+	 * If more than one unique annotation is found, then throw an error.
 	 * @param method the method declaration to search from
 	 * @param annotationType the annotation type to search for
-	 * @return the unique instance of the annotation attributed to the method,
-	 * {@code null} otherwise
-	 * @throws AnnotationConfigurationException if more than one instance of the
+	 * @return a unique instance of the annotation attributed to the method, {@code null}
+	 * otherwise
+	 * @throws AnnotationConfigurationException if more than one unique instance of the
 	 * annotation is found
 	 */
 	static <A extends Annotation> A findUniqueAnnotation(Method method, Class<A> annotationType) {
-		MergedAnnotations mergedAnnotations = MergedAnnotations.from(method,
-				MergedAnnotations.SearchStrategy.TYPE_HIERARCHY, RepeatableContainers.none());
-		if (hasDuplicate(mergedAnnotations, annotationType)) {
-			throw new AnnotationConfigurationException("Found more than one annotation of type " + annotationType
-					+ " attributed to " + method
-					+ " Please remove the duplicate annotations and publish a bean to handle your authorization logic.");
-		}
-		return AnnotationUtils.findAnnotation(method, annotationType);
+		return findDistinctAnnotation(method, annotationType);
 	}
 
 	/**
 	 * Perform an exhaustive search on the type hierarchy of the given {@link Class} for
 	 * the annotation of type {@code annotationType}, including any annotations using
 	 * {@code annotationType} as a meta-annotation.
 	 *
-	 * If more than one is found, then throw an error.
+	 * <p>
+	 * If more than one unique annotation is found, then throw an error.
 	 * @param type the type to search from
 	 * @param annotationType the annotation type to search for
-	 * @return the unique instance of the annotation attributed to the method,
-	 * {@code null} otherwise
-	 * @throws AnnotationConfigurationException if more than one instance of the
+	 * @return a unique instance of the annotation attributed to the class, {@code null}
+	 * otherwise
+	 * @throws AnnotationConfigurationException if more than one unique instance of the
 	 * annotation is found
 	 */
 	static <A extends Annotation> A findUniqueAnnotation(Class<?> type, Class<A> annotationType) {
-		MergedAnnotations mergedAnnotations = MergedAnnotations.from(type,
-				MergedAnnotations.SearchStrategy.TYPE_HIERARCHY, RepeatableContainers.none());
-		if (hasDuplicate(mergedAnnotations, annotationType)) {
-			throw new AnnotationConfigurationException("Found more than one annotation of type " + annotationType
-					+ " attributed to " + type
-					+ " Please remove the duplicate annotations and publish a bean to handle your authorization logic.");
-		}
-		return AnnotationUtils.findAnnotation(type, annotationType);
+		return findDistinctAnnotation(type, annotationType);
 	}
 
-	private static <A extends Annotation> boolean hasDuplicate(MergedAnnotations mergedAnnotations,
+	private static <A extends Annotation> A findDistinctAnnotation(AnnotatedElement annotatedElement,
 			Class<A> annotationType) {
-		MergedAnnotation<Annotation> alreadyFound = null;
-		for (MergedAnnotation<Annotation> mergedAnnotation : mergedAnnotations) {
-			if (isSynthetic(mergedAnnotation.getSource())) {
-				continue;
-			}
-
-			if (mergedAnnotation.getType() != annotationType) {
-				continue;
-			}
-
-			if (alreadyFound == null) {
-				alreadyFound = mergedAnnotation;
-				continue;
-			}
-
-			// https://github.com/spring-projects/spring-framework/issues/31803
-			if (!mergedAnnotation.getSource().equals(alreadyFound.getSource())) {
-				return true;
-			}
-
-			if (mergedAnnotation.getRoot().getType() != alreadyFound.getRoot().getType()) {
-				return true;
-			}
-		}
-		return false;
-	}
-
-	private static boolean isSynthetic(Object object) {
-		if (object instanceof Executable) {
-			return ((Executable) object).isSynthetic();
-		}
-
-		return false;
+		MergedAnnotations mergedAnnotations = MergedAnnotations.from(annotatedElement, SearchStrategy.TYPE_HIERARCHY,
+				RepeatableContainers.none());
+
+		List<A> annotations = mergedAnnotations.stream(annotationType)
+			.map(MergedAnnotation::withNonMergedAttributes)
+			.map(MergedAnnotation::synthesize)
+			.distinct()
+			.toList();
+
+		return switch (annotations.size()) {
+			case 0 -> null;
+			case 1 -> annotations.get(0);
+			default -> throw new AnnotationConfigurationException("""
+					Please ensure there is one unique annotation of type @%s attributed to %s. \
+					Found %d competing annotations: %s""".formatted(annotationType.getName(), annotatedElement,
+					annotations.size(), annotations));
+		};
 	}
 
 	private AuthorizationAnnotationUtils() {

core/src/test/java/org/springframework/security/access/annotation/RequireUserRole.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2021 the original author or authors.
+ * Copyright 2002-2024 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.
@@ -25,7 +25,7 @@
 
 @Retention(RetentionPolicy.RUNTIME)
 @PreAuthorize("hasRole('USER')")
-@RolesAllowed("ADMIN")
+@RolesAllowed("USER")
 @Secured("USER")
 public @interface RequireUserRole {
 

core/src/test/java/org/springframework/security/authorization/method/AuthorizationAnnotationUtilsTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 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.
@@ -16,18 +16,26 @@
 
 package org.springframework.security.authorization.method;
 
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.lang.reflect.Method;
 import java.lang.reflect.Proxy;
 import java.util.List;
 
 import org.junit.jupiter.api.Test;
 
+import org.springframework.core.annotation.AliasFor;
+import org.springframework.core.annotation.AnnotationConfigurationException;
 import org.springframework.security.access.prepost.PreAuthorize;
 
-import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
 /**
- * Tests for {@link AuthorizationAnnotationUtils}
+ * Tests for {@link AuthorizationAnnotationUtils}.
+ *
+ * @author Josh Cummings
+ * @author Sam Brannen
  */
 class AuthorizationAnnotationUtilsTests {
 
@@ -37,15 +45,56 @@ void annotationsOnSyntheticMethodsShouldNotTriggerAnnotationConfigurationExcepti
 				Thread.currentThread().getContextClassLoader(), new Class[] { StringRepository.class },
 				(p, m, args) -> null);
 		Method method = proxy.getClass().getDeclaredMethod("findAll");
-		assertThatNoException()
-			.isThrownBy(() -> AuthorizationAnnotationUtils.findUniqueAnnotation(method, PreAuthorize.class));
+		PreAuthorize preAuthorize = AuthorizationAnnotationUtils.findUniqueAnnotation(method, PreAuthorize.class);
+		assertThat(preAuthorize.value()).isEqualTo("hasRole('someRole')");
 	}
 
 	@Test // gh-13625
 	void annotationsFromSuperSuperInterfaceShouldNotTriggerAnnotationConfigurationException() throws Exception {
-		Method method = HelloImpl.class.getMethod("sayHello");
-		assertThatNoException()
-			.isThrownBy(() -> AuthorizationAnnotationUtils.findUniqueAnnotation(method, PreAuthorize.class));
+		Method method = HelloImpl.class.getDeclaredMethod("sayHello");
+		PreAuthorize preAuthorize = AuthorizationAnnotationUtils.findUniqueAnnotation(method, PreAuthorize.class);
+		assertThat(preAuthorize.value()).isEqualTo("hasRole('someRole')");
+	}
+
+	@Test
+	void multipleIdenticalAnnotationsOnClassShouldNotTriggerAnnotationConfigurationException() {
+		Class<?> clazz = MultipleIdenticalPreAuthorizeAnnotationsOnClass.class;
+		PreAuthorize preAuthorize = AuthorizationAnnotationUtils.findUniqueAnnotation(clazz, PreAuthorize.class);
+		assertThat(preAuthorize.value()).isEqualTo("hasRole('someRole')");
+	}
+
+	@Test
+	void multipleIdenticalAnnotationsOnMethodShouldNotTriggerAnnotationConfigurationException() throws Exception {
+		Method method = MultipleIdenticalPreAuthorizeAnnotationsOnMethod.class.getDeclaredMethod("method");
+		PreAuthorize preAuthorize = AuthorizationAnnotationUtils.findUniqueAnnotation(method, PreAuthorize.class);
+		assertThat(preAuthorize.value()).isEqualTo("hasRole('someRole')");
+	}
+
+	@Test
+	void competingAnnotationsOnClassShouldTriggerAnnotationConfigurationException() {
+		Class<?> clazz = CompetingPreAuthorizeAnnotationsOnClass.class;
+		assertThatExceptionOfType(AnnotationConfigurationException.class)
+			.isThrownBy(() -> AuthorizationAnnotationUtils.findUniqueAnnotation(clazz, PreAuthorize.class))
+			.withMessageContainingAll("Found 2 competing annotations:", "someRole", "otherRole");
+	}
+
+	@Test
+	void competingAnnotationsOnMethodShouldTriggerAnnotationConfigurationException() throws Exception {
+		Method method = CompetingPreAuthorizeAnnotationsOnMethod.class.getDeclaredMethod("method");
+		assertThatExceptionOfType(AnnotationConfigurationException.class)
+			.isThrownBy(() -> AuthorizationAnnotationUtils.findUniqueAnnotation(method, PreAuthorize.class))
+			.withMessageContainingAll("Found 2 competing annotations:", "someRole", "otherRole");
+	}
+
+	@Test
+	void composedMergedAnnotationsAreNotSupported() {
+		Class<?> clazz = ComposedPreAuthAnnotationOnClass.class;
+		PreAuthorize preAuthorize = AuthorizationAnnotationUtils.findUniqueAnnotation(clazz, PreAuthorize.class);
+
+		// If you comment out .map(MergedAnnotation::withNonMergedAttributes) in
+		// AuthorizationAnnotationUtils.findDistinctAnnotation(), the value of
+		// the merged annotation would be "hasRole('composedRole')".
+		assertThat(preAuthorize.value()).isEqualTo("hasRole('metaRole')");
 	}
 
 	private interface BaseRepository<T> {
@@ -82,4 +131,60 @@ public String sayHello() {
 
 	}
 
+	@Retention(RetentionPolicy.RUNTIME)
+	@PreAuthorize("hasRole('someRole')")
+	private @interface RequireSomeRole {
+
+	}
+
+	@Retention(RetentionPolicy.RUNTIME)
+	@PreAuthorize("hasRole('otherRole')")
+	private @interface RequireOtherRole {
+
+	}
+
+	@RequireSomeRole
+	@PreAuthorize("hasRole('someRole')")
+	private static class MultipleIdenticalPreAuthorizeAnnotationsOnClass {
+
+	}
+
+	private static class MultipleIdenticalPreAuthorizeAnnotationsOnMethod {
+
+		@RequireSomeRole
+		@PreAuthorize("hasRole('someRole')")
+		void method() {
+		}
+
+	}
+
+	@RequireOtherRole
+	@PreAuthorize("hasRole('someRole')")
+	private static class CompetingPreAuthorizeAnnotationsOnClass {
+
+	}
+
+	private static class CompetingPreAuthorizeAnnotationsOnMethod {
+
+		@RequireOtherRole
+		@PreAuthorize("hasRole('someRole')")
+		void method() {
+		}
+
+	}
+
+	@Retention(RetentionPolicy.RUNTIME)
+	@PreAuthorize("hasRole('metaRole')")
+	private @interface ComposedPreAuth {
+
+		@AliasFor(annotation = PreAuthorize.class)
+		String value();
+
+	}
+
+	@ComposedPreAuth("hasRole('composedRole')")
+	private static class ComposedPreAuthAnnotationOnClass {
+
+	}
+
 }