From cefc0cddec531645391af182d994efdb76cf91fc Mon Sep 17 00:00:00 2001 From: Josh Cummings <3627351+jzheaux@users.noreply.github.com> Date: Wed, 15 Oct 2025 15:46:30 -0600 Subject: [PATCH] Propagate All Missing Factors Closes gh-18000 --- .../configurers/FormLoginConfigurerTests.java | 3 ++- ...ngMissingAuthorityAccessDeniedHandler.java | 14 ++++++---- ...singAuthorityAccessDeniedHandlerTests.java | 26 +++++++++++++++++++ 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/FormLoginConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/FormLoginConfigurerTests.java index c166e4fd6e..071e5a1e00 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/FormLoginConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/FormLoginConfigurerTests.java @@ -403,7 +403,8 @@ public class FormLoginConfigurerTests { UserDetails user = PasswordEncodedUser.user(); this.mockMvc.perform(get("/profile").with(user(user))) .andExpect(status().is3xxRedirection()) - .andExpect(redirectedUrl("http://localhost/login?factor.type=password&factor.reason=missing")); + .andExpect(redirectedUrl( + "http://localhost/login?factor.type=password&factor.type=ott&factor.reason=missing&factor.reason=missing")); this.mockMvc .perform(post("/ott/generate").param("username", "rod") .with(user(user)) diff --git a/web/src/main/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandler.java b/web/src/main/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandler.java index eee4e286aa..a4f42b9c86 100644 --- a/web/src/main/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandler.java +++ b/web/src/main/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandler.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -97,17 +98,20 @@ public final class DelegatingMissingAuthorityAccessDeniedHandler implements Acce @Override public void handle(HttpServletRequest request, HttpServletResponse response, AccessDeniedException denied) throws IOException, ServletException { - List authorityErrors = authorityErrors(denied); - for (AuthorityRequiredFactorErrorEntry authorityError : authorityErrors) { + List errorEntries = authorityErrors(denied); + List errors = errorEntries.stream() + .map(AuthorityRequiredFactorErrorEntry::getError) + .filter(Objects::nonNull) + .toList(); + for (AuthorityRequiredFactorErrorEntry authorityError : errorEntries) { String requiredAuthority = authorityError.getAuthority(); AuthenticationEntryPoint entryPoint = this.entryPoints.get(requiredAuthority); if (entryPoint == null) { continue; } this.requestCache.saveRequest(request, response); - RequiredFactorError required = authorityError.getError(); - if (required != null) { - request.setAttribute(WebAttributes.REQUIRED_FACTOR_ERRORS, List.of(required)); + if (!errors.isEmpty()) { + request.setAttribute(WebAttributes.REQUIRED_FACTOR_ERRORS, errors); } String message = String.format("Missing Authorities %s", requiredAuthority); AuthenticationException ex = new InsufficientAuthenticationException(message, denied); diff --git a/web/src/test/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandlerTests.java b/web/src/test/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandlerTests.java index 7aa02ef980..be6fe12bba 100644 --- a/web/src/test/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandlerTests.java +++ b/web/src/test/java/org/springframework/security/web/access/DelegatingMissingAuthorityAccessDeniedHandlerTests.java @@ -17,6 +17,8 @@ package org.springframework.security.web.access; import java.util.Collection; +import java.util.List; +import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -29,13 +31,18 @@ import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.authorization.AuthorityAuthorizationDecision; import org.springframework.security.authorization.AuthorizationDeniedException; +import org.springframework.security.authorization.FactorAuthorizationDecision; +import org.springframework.security.authorization.RequiredFactor; +import org.springframework.security.authorization.RequiredFactorError; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.web.AuthenticationEntryPoint; +import org.springframework.security.web.WebAttributes; import org.springframework.security.web.savedrequest.RequestCache; import org.springframework.security.web.util.matcher.RequestHeaderRequestMatcher; import org.springframework.security.web.util.matcher.RequestMatcher; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -136,10 +143,29 @@ class DelegatingMissingAuthorityAccessDeniedHandlerTests { verify(basicPasswordEntryPoint).commence(any(), any(), any()); } + // gh-18000 + @Test + void whenMultipleFactorErrorsThenPropagatesAll() throws Exception { + AccessDeniedHandler accessDeniedHandler = this.builder.build(); + accessDeniedHandler.handle(this.request, this.response, missingFactors("FACTOR", "PASSWORD")); + verify(this.factorEntryPoint).commence(any(), any(), any()); + Object attribute = this.request.getAttribute(WebAttributes.REQUIRED_FACTOR_ERRORS); + assertThat(attribute).isInstanceOf(Collection.class); + assertThat((Collection) attribute).hasSize(2); + } + AuthorizationDeniedException missingAuthorities(String... authorities) { Collection granted = AuthorityUtils.createAuthorityList(authorities); AuthorityAuthorizationDecision decision = new AuthorityAuthorizationDecision(false, granted); return new AuthorizationDeniedException("access denied", decision); } + AuthorizationDeniedException missingFactors(String... factors) { + List errors = Stream.of(factors) + .map((f) -> RequiredFactorError.createMissing(RequiredFactor.withAuthority(f).build())) + .toList(); + FactorAuthorizationDecision decision = new FactorAuthorizationDecision(errors); + return new AuthorizationDeniedException("access denied", decision); + } + }