Revise AuthorizationAnnotationUtils

This commit revises AuthorizationAnnotationUtils as follows.

- Removes code duplication by treating both Class and Method as
  AnnotatedElement.

- Avoids duplicated annotation searches by processing merged
  annotations in a single Stream instead of first using the
  MergedAnnotations API to find possible duplicates and then again
  searching for a single annotation via AnnotationUtils (which
  effectively performs the same search using the MergedAnnotations API
  internally).

- Uses `.distinct()` within the Stream to avoid the need for the
  workaround introduced in gh-13625. Note that the semantics here
  result in duplicate "equivalent" annotations being ignored. In other
  words, if @⁠PreAuthorize("hasRole('someRole')") is present multiple
  times as a meta-annotation, no exception will be thrown and the first
  such annotation found will be used.

- Improves the error message when competing annotations are found by
  including the competing annotations in the error message.

- Updates AuthorizationAnnotationUtilsTests to cover all known,
  supported use cases.

- Configures correct role in @⁠RequireUserRole.

Please note this commit uses
`.map(MergedAnnotation::withNonMergedAttributes)` to retain backward
compatibility with previous versions of Spring Security. However, that
line can be deleted if the Spring Security team decides that it wishes
to support merged annotation attributes via custom composed
annotations. If that decision is made, the
composedMergedAnnotationsAreNotSupported() test should be renamed and
updated as explained in the comment in that method.

See gh-13625
See spring-projects/spring-framework#31803

Author: sbrannen

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 {
+
+	}
+
 }