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 https://github.com/spring-projects/spring-framework/issues/31803
This commit is contained in:
Sam Brannen
2024-01-05 11:56:03 +01:00
committed by Josh Cummings
parent 3f65f600de
commit 2b7d296994
3 changed files with 158 additions and 76 deletions

View File

@@ -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 @@ import org.springframework.security.access.prepost.PreAuthorize;
@Retention(RetentionPolicy.RUNTIME)
@PreAuthorize("hasRole('USER')")
@RolesAllowed("ADMIN")
@RolesAllowed("USER")
@Secured("USER")
public @interface RequireUserRole {

View File

@@ -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 @@ class AuthorizationAnnotationUtilsTests {
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 @@ class AuthorizationAnnotationUtilsTests {
}
@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 {
}
}