Polish AllRequiredFactorsAuthorizationManager.anyOf

- Add validation
- Extract to static inner class
- Uniqueness determined by Set rather than requiredFactor
  This is important for the failure with the same RequiredFactor, but a
  different reason
- Add documentation

Signed-off-by: Robert Winch <362503+rwinch@users.noreply.github.com>

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

@@ -20,11 +20,11 @@ import java.time.Clock;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -54,28 +54,23 @@ public final class AllRequiredFactorsAuthorizationManager<T> implements Authoriz
 
 	/**
 	 * Creates an {@link AuthorizationManager} that grants access if at least one
-	 * {@link AllRequiredFactorsAuthorizationManager} granted, collects
-	 * {@link RequiredFactorError}s omitting duplicate errors of the same factor.
+	 * {@link AllRequiredFactorsAuthorizationManager} granted. When all managers deny,
+	 * collects the unique {@link RequiredFactorError}s from each manager.
 	 * @param <T> the type of object that is being authorized
-	 * @param managers the {@link AllRequiredFactorsAuthorizationManager}s to use
+	 * @param managers the {@link AllRequiredFactorsAuthorizationManager}s to use; cannot
+	 * be empty or contain null elements
 	 * @return the {@link AuthorizationManager} to use
 	 * @since 7.1
 	 * @see AuthorizationManagers#anyOf(AuthorizationManager[])
 	 */
 	@SafeVarargs
 	public static <T> AuthorizationManager<T> anyOf(AllRequiredFactorsAuthorizationManager<T>... managers) {
-		return (authentication, object) -> {
-			Map<String, RequiredFactorError> factorErrors = new LinkedHashMap<>();
-			for (AllRequiredFactorsAuthorizationManager<T> manager : managers) {
-				FactorAuthorizationDecision decision = manager.authorize(authentication, object);
-				if (decision.isGranted()) {
-					return decision;
-				}
-				decision.getFactorErrors()
-					.forEach((e) -> factorErrors.putIfAbsent(e.getRequiredFactor().getAuthority(), e));
-			}
-			return new FactorAuthorizationDecision(List.copyOf(factorErrors.values()));
-		};
+		Assert.notEmpty(managers, "managers cannot be empty");
+		Assert.noNullElements(managers, "managers cannot contain null elements");
+		if (managers.length == 1) {
+			return managers[0];
+		}
+		return new AnyOfFactorsAuthorizationManager<>(managers);
 	}
 
 	/**
@@ -179,6 +174,38 @@ public final class AllRequiredFactorsAuthorizationManager<T> implements Authoriz
 		return new Builder<>();
 	}
 
+	/**
+	 * An {@link AuthorizationManager} that grants access if at least one
+	 * {@link AllRequiredFactorsAuthorizationManager} granted. When all deny, collects the
+	 * unique {@link RequiredFactorError}s from each manager.
+	 *
+	 * @param <T> the type of object being authorized
+	 */
+	private static final class AnyOfFactorsAuthorizationManager<T> implements AuthorizationManager<T> {
+
+		private final AllRequiredFactorsAuthorizationManager<T>[] managers;
+
+		AnyOfFactorsAuthorizationManager(AllRequiredFactorsAuthorizationManager<T>[] managers) {
+			Assert.notEmpty(managers, "managers cannot be empty");
+			Assert.noNullElements(managers, "managers cannot contain null elements");
+			this.managers = managers;
+		}
+
+		@Override
+		public AuthorizationResult authorize(Supplier<? extends @Nullable Authentication> authentication, T object) {
+			Set<RequiredFactorError> factorErrors = new LinkedHashSet<>();
+			for (AllRequiredFactorsAuthorizationManager<T> manager : this.managers) {
+				FactorAuthorizationDecision decision = manager.authorize(authentication, object);
+				if (decision.isGranted()) {
+					return decision;
+				}
+				factorErrors.addAll(decision.getFactorErrors());
+			}
+			return new FactorAuthorizationDecision(List.copyOf(factorErrors));
+		}
+
+	}
+
 	/**
 	 * A builder for {@link AllRequiredFactorsAuthorizationManager}.
 	 *

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

@@ -44,11 +44,11 @@ class AllRequiredFactorsAuthorizationManagerTests {
 
 	private static final Object DOES_NOT_MATTER = new Object();
 
-	private static final RequiredFactor REQUIRED_PASSWORD = RequiredFactor
+	private static RequiredFactor REQUIRED_PASSWORD = RequiredFactor
 		.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY)
 		.build();
 
-	private static final RequiredFactor EXPIRING_PASSWORD = RequiredFactor
+	private static RequiredFactor EXPIRING_PASSWORD = RequiredFactor
 		.withAuthority(FactorGrantedAuthority.PASSWORD_AUTHORITY)
 		.validDuration(Duration.ofHours(1))
 		.build();
@@ -255,7 +255,7 @@ class AllRequiredFactorsAuthorizationManagerTests {
 	}
 
 	@Test
-	void anyOfWhenRequiredFactorMissingThenMissing() {
+	void anyOfWhenSameAuthorityDifferentValidDurationThenBothErrorsReturned() {
 		AllRequiredFactorsAuthorizationManager<Object> passwordAndOtt = AllRequiredFactorsAuthorizationManager.builder()
 			.requireFactor(REQUIRED_PASSWORD)
 			.requireFactor(REQUIRED_OTT)
@@ -273,10 +273,62 @@ class AllRequiredFactorsAuthorizationManagerTests {
 		AuthorizationResult result = anyOf.authorize(() -> authentication, DOES_NOT_MATTER);
 		assertThat(result).asInstanceOf(type(FactorAuthorizationDecision.class)).satisfies((decision) -> {
 			assertThat(decision.isGranted()).isFalse();
-			assertThat(decision.getFactorErrors()).containsExactly(RequiredFactorError.createMissing(REQUIRED_OTT));
+			assertThat(decision.getFactorErrors()).containsExactly(RequiredFactorError.createMissing(REQUIRED_OTT),
+					RequiredFactorError.createMissing(EXPIRING_OTT));
 		});
 	}
 
+	@Test
+	void anyOfWhenIdenticalErrorInMultipleManagersThenDeduplicated() {
+		AllRequiredFactorsAuthorizationManager<Object> passwordAndOtt = AllRequiredFactorsAuthorizationManager.builder()
+			.requireFactor(REQUIRED_PASSWORD)
+			.requireFactor(REQUIRED_OTT)
+			.build();
+		AllRequiredFactorsAuthorizationManager<Object> passwordOnly = AllRequiredFactorsAuthorizationManager.builder()
+			.requireFactor(REQUIRED_PASSWORD)
+			.build();
+		AuthorizationManager<Object> anyOf = AllRequiredFactorsAuthorizationManager.anyOf(passwordAndOtt, passwordOnly);
+		Authentication authentication = new TestingAuthenticationToken("user", "password", "ROLE_USER");
+		AuthorizationResult result = anyOf.authorize(() -> authentication, DOES_NOT_MATTER);
+		assertThat(result).asInstanceOf(type(FactorAuthorizationDecision.class)).satisfies((decision) -> {
+			assertThat(decision.isGranted()).isFalse();
+			assertThat(decision.getFactorErrors()).containsOnly(RequiredFactorError.createMissing(REQUIRED_PASSWORD),
+					RequiredFactorError.createMissing(REQUIRED_OTT));
+		});
+	}
+
+	@Test
+	void anyOfWhenDeniedThenErrorsRetainedInManagerOrder() {
+		AllRequiredFactorsAuthorizationManager<Object> passwordOnly = AllRequiredFactorsAuthorizationManager.builder()
+			.requireFactor(REQUIRED_PASSWORD)
+			.build();
+		AllRequiredFactorsAuthorizationManager<Object> ottOnly = AllRequiredFactorsAuthorizationManager.builder()
+			.requireFactor(REQUIRED_OTT)
+			.build();
+		AuthorizationManager<Object> anyOf = AllRequiredFactorsAuthorizationManager.anyOf(passwordOnly, ottOnly);
+		Authentication authentication = new TestingAuthenticationToken("user", "password", "ROLE_USER");
+		AuthorizationResult result = anyOf.authorize(() -> authentication, DOES_NOT_MATTER);
+		assertThat(result).asInstanceOf(type(FactorAuthorizationDecision.class)).satisfies((decision) -> {
+			assertThat(decision.isGranted()).isFalse();
+			assertThat(decision.getFactorErrors()).containsExactly(RequiredFactorError.createMissing(REQUIRED_PASSWORD),
+					RequiredFactorError.createMissing(REQUIRED_OTT));
+		});
+	}
+
+	@Test
+	void anyOfWhenEmptyManagersThenIllegalArgumentException() {
+		assertThatIllegalArgumentException().isThrownBy(() -> AllRequiredFactorsAuthorizationManager.anyOf());
+	}
+
+	@Test
+	void anyOfWhenSingleManagerThenReturnsSameInstance() {
+		AllRequiredFactorsAuthorizationManager<Object> manager = AllRequiredFactorsAuthorizationManager.builder()
+			.requireFactor(REQUIRED_PASSWORD)
+			.build();
+		AuthorizationManager<Object> result = AllRequiredFactorsAuthorizationManager.anyOf(manager);
+		assertThat(result == manager).isTrue();
+	}
+
 	@Test
 	void setClockWhenNullThenIllegalArgumentException() {
 		AllRequiredFactorsAuthorizationManager<Object> allFactors = AllRequiredFactorsAuthorizationManager.builder()

docs/modules/ROOT/pages/servlet/authentication/mfa.adoc

@@ -125,6 +125,23 @@ include-code::./ValidDurationConfiguration[tag=httpSecurity,indent=0]
 <5> Otherwise, authentication is required, but it does not care if it is a password or how long ago authentication occurred
 <6> Set up the authentication mechanisms that can provide the required factors.
 
+[[all-factors-anyof]]
+== AllRequiredFactorsAuthorizationManager.anyOf
+
+In the previous examples, access requires satisfying that the user has authenticated with all factors.
+There are times when an application wants to allow users to satisfy one of several different combinations of factors.
+javadoc:org.springframework.security.authorization.AllRequiredFactorsAuthorizationManager#anyOf(AllRequiredFactorsAuthorizationManager...)[AllRequiredFactorsAuthorizationManager.anyOf] grants access if at least one of the provided combinations of factors is satisfied.
+
+Consider a scenario where a user can authenticate with WebAuthn alone, or with both a password and a one-time token.
+
+include-code::./AnyOfRequiredFactorsConfiguration[tag=httpSecurity,indent=0]
+<1> Require WebAuthn
+<2> Require both a password and a one-time token
+<3> Combine the combinations of factors with `anyOf`, granting access if either is satisfied
+<4> URLs that begin with `/protected/**` require the user to satisfy either combination of factors
+<5> All other requests require only authentication
+<6> Set up the authentication mechanisms that can provide the required factors
+
 [[programmatic-mfa]]
 == Programmatic MFA
 

docs/modules/ROOT/pages/whats-new.adoc

@@ -4,6 +4,7 @@
 == Core
 
 * https://github.com/spring-projects/spring-security/pull/18634[gh-18634] - Added javadoc:org.springframework.security.util.matcher.InetAddressMatcher[]
+* https://github.com/spring-projects/spring-security/issues/18960[gh-18960] - Added xref:servlet/authentication/mfa.adoc#all-factors-anyof[AllRequiredFactorsAuthorizationManager.anyOf]
 
 == Web
 * https://github.com/spring-projects/spring-security/issues/18755[gh-18755] - Include `charset` in `WWW-Authenticate` header

docs/src/test/java/org/springframework/security/docs/servlet/authentication/allfactorsanyof/AnyOfRequiredFactorsConfiguration.java

@@ -0,0 +1,76 @@
+package org.springframework.security.docs.servlet.authentication.allfactorsanyof;
+
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.security.authorization.AllRequiredFactorsAuthorizationManager;
+import org.springframework.security.authorization.DefaultAuthorizationManagerFactory;
+import org.springframework.security.config.Customizer;
+import org.springframework.security.config.annotation.web.builders.HttpSecurity;
+import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
+import org.springframework.security.core.userdetails.User;
+import org.springframework.security.core.userdetails.UserDetailsService;
+import org.springframework.security.provisioning.InMemoryUserDetailsManager;
+import org.springframework.security.web.SecurityFilterChain;
+import org.springframework.security.web.authentication.ott.OneTimeTokenGenerationSuccessHandler;
+import org.springframework.security.web.authentication.ott.RedirectOneTimeTokenGenerationSuccessHandler;
+
+@EnableWebSecurity
+@Configuration(proxyBeanMethods = false)
+class AnyOfRequiredFactorsConfiguration {
+
+	// tag::httpSecurity[]
+	@Bean
+	SecurityFilterChain springSecurity(HttpSecurity http) throws Exception {
+		// @formatter:off
+		// <1>
+		AllRequiredFactorsAuthorizationManager<Object> webauthn = AllRequiredFactorsAuthorizationManager
+				.<Object>builder()
+				.requireFactor((factor) -> factor.webauthnAuthority())
+				.build();
+		// <2>
+		AllRequiredFactorsAuthorizationManager<Object> passwordAndOtt = AllRequiredFactorsAuthorizationManager
+				.<Object>builder()
+				.requireFactor((factor) -> factor.passwordAuthority())
+				.requireFactor((factor) -> factor.ottAuthority())
+				.build();
+		// <3>
+		DefaultAuthorizationManagerFactory<Object> mfa = new DefaultAuthorizationManagerFactory<>();
+		mfa.setAdditionalAuthorization(AllRequiredFactorsAuthorizationManager.anyOf(webauthn, passwordAndOtt));
+		http
+			.authorizeHttpRequests((authorize) -> authorize
+				// <4>
+				.requestMatchers("/protected/**").access(mfa.authenticated())
+				// <5>
+				.anyRequest().authenticated()
+			)
+			// <6>
+			.formLogin(Customizer.withDefaults())
+			.oneTimeTokenLogin(Customizer.withDefaults())
+			.webAuthn((webAuthn) -> webAuthn
+				.rpName("Spring Security")
+				.rpId("example.com")
+				.allowedOrigins("https://example.com")
+			);
+		// @formatter:on
+		return http.build();
+	}
+
+	// end::httpSecurity[]
+
+	@Bean
+	UserDetailsService userDetailsService() {
+		return new InMemoryUserDetailsManager(
+				User.withDefaultPasswordEncoder()
+						.username("user")
+						.password("password")
+						.authorities("app")
+						.build()
+		);
+	}
+
+	@Bean
+	OneTimeTokenGenerationSuccessHandler tokenGenerationSuccessHandler() {
+		return new RedirectOneTimeTokenGenerationSuccessHandler("/ott/sent");
+	}
+
+}

docs/src/test/kotlin/org/springframework/security/kt/docs/servlet/authentication/allfactorsanyof/AnyOfRequiredFactorsConfiguration.kt

@@ -0,0 +1,77 @@
+package org.springframework.security.kt.docs.servlet.authentication.allfactorsanyof
+
+import org.springframework.context.annotation.Bean
+import org.springframework.context.annotation.Configuration
+import org.springframework.security.authorization.AllRequiredFactorsAuthorizationManager
+import org.springframework.security.authorization.DefaultAuthorizationManagerFactory
+import org.springframework.security.config.annotation.web.builders.HttpSecurity
+import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity
+import org.springframework.security.config.annotation.web.invoke
+import org.springframework.security.core.userdetails.User
+import org.springframework.security.core.userdetails.UserDetailsService
+import org.springframework.security.provisioning.InMemoryUserDetailsManager
+import org.springframework.security.web.SecurityFilterChain
+import org.springframework.security.web.authentication.ott.OneTimeTokenGenerationSuccessHandler
+import org.springframework.security.web.authentication.ott.RedirectOneTimeTokenGenerationSuccessHandler
+
+@EnableWebSecurity
+@Configuration(proxyBeanMethods = false)
+internal class AnyOfRequiredFactorsConfiguration {
+
+	// tag::httpSecurity[]
+	@Bean
+	@Throws(Exception::class)
+	fun springSecurity(http: HttpSecurity): SecurityFilterChain? {
+		// @formatter:off
+		// <1>
+		val webauthn = AllRequiredFactorsAuthorizationManager.builder<Any>()
+			.requireFactor { factor -> factor.webauthnAuthority() }
+			.build()
+		// <2>
+		val passwordAndOtt = AllRequiredFactorsAuthorizationManager.builder<Any>()
+			.requireFactor { factor -> factor.passwordAuthority() }
+			.requireFactor { factor -> factor.ottAuthority() }
+			.build()
+		// <3>
+		val mfa = DefaultAuthorizationManagerFactory<Any>()
+		mfa.setAdditionalAuthorization(AllRequiredFactorsAuthorizationManager.anyOf(webauthn, passwordAndOtt))
+		http {
+			authorizeHttpRequests {
+				// <4>
+				authorize("/protected/**", mfa.authenticated())
+				// <5>
+				authorize(anyRequest, authenticated)
+			}
+			// <6>
+			formLogin { }
+			oneTimeTokenLogin { }
+			webAuthn {
+				rpName = "Spring Security"
+				rpId = "example.com"
+				allowedOrigins = setOf("https://example.com")
+			}
+		}
+		// @formatter:on
+		return http.build()
+	}
+
+	// end::httpSecurity[]
+
+	@Suppress("DEPRECATION")
+	@Bean
+	fun userDetailsService(): UserDetailsService {
+		return InMemoryUserDetailsManager(
+			User.withDefaultPasswordEncoder()
+				.username("user")
+				.password("password")
+				.authorities("app")
+				.build()
+		)
+	}
+
+	@Bean
+	fun tokenGenerationSuccessHandler(): OneTimeTokenGenerationSuccessHandler {
+		return RedirectOneTimeTokenGenerationSuccessHandler("/ott/sent")
+	}
+
+}