SecuredAuthorizationManager should cache annotation's value

core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 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.
@@ -16,8 +16,6 @@
 
 package org.springframework.security.authorization;
 
-import java.util.Arrays;
-import java.util.HashSet;
 import java.util.Set;
 import java.util.function.Supplier;
 
@@ -43,7 +41,7 @@ public final class AuthorityAuthorizationManager<T> implements AuthorizationMana
 	private final Set<String> authorities;
 
 	private AuthorityAuthorizationManager(String... authorities) {
-		this.authorities = new HashSet<>(Arrays.asList(authorities));
+		this.authorities = Set.of(authorities);
 	}
 
 	/**

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2021 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.
@@ -17,14 +17,18 @@
 package org.springframework.security.authorization.method;
 
 import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Supplier;
 
 import org.aopalliance.intercept.MethodInvocation;
 
 import org.springframework.aop.support.AopUtils;
-import org.springframework.lang.NonNull;
+import org.springframework.core.MethodClassKey;
 import org.springframework.security.access.annotation.Secured;
-import org.springframework.security.authorization.AuthorityAuthorizationManager;
+import org.springframework.security.authorization.AuthoritiesAuthorizationManager;
 import org.springframework.security.authorization.AuthorizationDecision;
 import org.springframework.security.authorization.AuthorizationManager;
 import org.springframework.security.core.Authentication;
@@ -39,7 +43,9 @@
  */
 public final class SecuredAuthorizationManager implements AuthorizationManager<MethodInvocation> {
 
-	private final SecuredAuthorizationManagerRegistry registry = new SecuredAuthorizationManagerRegistry();
+	private final AuthoritiesAuthorizationManager delegate = new AuthoritiesAuthorizationManager();
+
+	private final Map<MethodClassKey, Set<String>> cachedAuthorities = new ConcurrentHashMap<>();
 
 	/**
 	 * Determine if an {@link Authentication} has access to a method by evaluating the
@@ -51,26 +57,28 @@ public final class SecuredAuthorizationManager implements AuthorizationManager<M
 	 */
 	@Override
 	public AuthorizationDecision check(Supplier<Authentication> authentication, MethodInvocation mi) {
-		AuthorizationManager<MethodInvocation> delegate = this.registry.getManager(mi);
-		return delegate.check(authentication, mi);
+		Set<String> authorities = getAuthorities(mi);
+		return authorities.isEmpty() ? null : this.delegate.check(authentication, authorities);
 	}
 
-	private static final class SecuredAuthorizationManagerRegistry extends AbstractAuthorizationManagerRegistry {
-
-		@NonNull
-		@Override
-		AuthorizationManager<MethodInvocation> resolveManager(Method method, Class<?> targetClass) {
-			Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
-			Secured secured = findSecuredAnnotation(specificMethod);
-			return (secured != null) ? AuthorityAuthorizationManager.hasAnyAuthority(secured.value()) : NULL_MANAGER;
-		}
+	private Set<String> getAuthorities(MethodInvocation methodInvocation) {
+		Method method = methodInvocation.getMethod();
+		Object target = methodInvocation.getThis();
+		Class<?> targetClass = (target != null) ? target.getClass() : null;
+		MethodClassKey cacheKey = new MethodClassKey(method, targetClass);
+		return this.cachedAuthorities.computeIfAbsent(cacheKey, (k) -> resolveAuthorities(method, targetClass));
+	}
 
-		private Secured findSecuredAnnotation(Method method) {
-			Secured secured = AuthorizationAnnotationUtils.findUniqueAnnotation(method, Secured.class);
-			return (secured != null) ? secured
-					: AuthorizationAnnotationUtils.findUniqueAnnotation(method.getDeclaringClass(), Secured.class);
-		}
+	private Set<String> resolveAuthorities(Method method, Class<?> targetClass) {
+		Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
+		Secured secured = findSecuredAnnotation(specificMethod);
+		return (secured != null) ? Set.of(secured.value()) : Collections.emptySet();
+	}
 
+	private Secured findSecuredAnnotation(Method method) {
+		Secured secured = AuthorizationAnnotationUtils.findUniqueAnnotation(method, Secured.class);
+		return (secured != null) ? secured
+				: AuthorizationAnnotationUtils.findUniqueAnnotation(method.getDeclaringClass(), Secured.class);
 	}
 
 }

core/src/test/java/org/springframework/security/authorization/AuthoritiesAuthorizationManagerTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 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.
@@ -74,14 +74,14 @@ void checkWhenUserHasNotAnyAuthorityThenDeniedDecision() {
 	}
 
 	@Test
-	void hasRoleWhenRoleHierarchySetThenGreaterRoleTakesPrecedence() {
+	void checkWhenRoleHierarchySetThenGreaterRoleTakesPrecedence() {
 		AuthoritiesAuthorizationManager manager = new AuthoritiesAuthorizationManager();
 		RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();
 		roleHierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER");
 		manager.setRoleHierarchy(roleHierarchy);
 		Supplier<Authentication> authentication = () -> new TestingAuthenticationToken("user", "password",
 				"ROLE_ADMIN");
-		assertThat(manager.check(authentication, Collections.singleton("ROLE_ADMIN")).isGranted()).isTrue();
+		assertThat(manager.check(authentication, Collections.singleton("ROLE_USER")).isGranted()).isTrue();
 	}
 
 }