diff --git a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java index 42e9d0ce5a..9546bcae0c 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java @@ -35,6 +35,7 @@ import org.springframework.context.MessageSource; import org.springframework.context.MessageSourceAware; import org.springframework.context.support.MessageSourceAccessor; import org.springframework.core.log.LogMessage; +import org.springframework.lang.Contract; import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException; import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationManager; @@ -253,7 +254,7 @@ public abstract class AbstractAuthenticationProcessingFilter extends GenericFilt return; } Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); - if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) { + if (shouldPerformMfa(current, authenticationResult)) { authenticationResult = authenticationResult.toBuilder() // @formatter:off .authorities((a) -> { @@ -286,6 +287,17 @@ public abstract class AbstractAuthenticationProcessingFilter extends GenericFilt } } + @Contract("null, _ -> false") + private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) { + if (current == null || !current.isAuthenticated()) { + return false; + } + if (!declaresToBuilder(authenticationResult)) { + return false; + } + return current.getName().equals(authenticationResult.getName()); + } + private static boolean declaresToBuilder(Authentication authentication) { for (Method method : authentication.getClass().getDeclaredMethods()) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { diff --git a/web/src/main/java/org/springframework/security/web/authentication/AuthenticationFilter.java b/web/src/main/java/org/springframework/security/web/authentication/AuthenticationFilter.java index 0425405200..f36ed0d899 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/AuthenticationFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/AuthenticationFilter.java @@ -30,6 +30,7 @@ import jakarta.servlet.http.HttpSession; import org.jspecify.annotations.Nullable; import org.springframework.http.HttpStatus; +import org.springframework.lang.Contract; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationManagerResolver; import org.springframework.security.core.Authentication; @@ -189,7 +190,7 @@ public class AuthenticationFilter extends OncePerRequestFilter { return; } Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); - if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) { + if (shouldPerformMfa(current, authenticationResult)) { authenticationResult = authenticationResult.toBuilder() // @formatter:off .authorities((a) -> { @@ -216,6 +217,17 @@ public class AuthenticationFilter extends OncePerRequestFilter { } } + @Contract("null, _ -> false") + private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) { + if (current == null || !current.isAuthenticated()) { + return false; + } + if (!declaresToBuilder(authenticationResult)) { + return false; + } + return current.getName().equals(authenticationResult.getName()); + } + private static boolean declaresToBuilder(Authentication authentication) { for (Method method : authentication.getClass().getDeclaredMethods()) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { diff --git a/web/src/main/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilter.java b/web/src/main/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilter.java index 7f0322cf6a..e59fe0c8f9 100755 --- a/web/src/main/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilter.java @@ -33,6 +33,7 @@ import org.jspecify.annotations.Nullable; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.core.log.LogMessage; +import org.springframework.lang.Contract; import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent; @@ -209,7 +210,7 @@ public abstract class AbstractPreAuthenticatedProcessingFilter extends GenericFi authenticationRequest.setDetails(this.authenticationDetailsSource.buildDetails(request)); Authentication authenticationResult = this.authenticationManager.authenticate(authenticationRequest); Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); - if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) { + if (shouldPerformMfa(current, authenticationResult)) { authenticationResult = authenticationResult.toBuilder() // @formatter:off .authorities((a) -> { @@ -235,6 +236,17 @@ public abstract class AbstractPreAuthenticatedProcessingFilter extends GenericFi } } + @Contract("null, _ -> false") + private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) { + if (current == null || !current.isAuthenticated()) { + return false; + } + if (!declaresToBuilder(authenticationResult)) { + return false; + } + return current.getName().equals(authenticationResult.getName()); + } + private static boolean declaresToBuilder(Authentication authentication) { for (Method method : authentication.getClass().getDeclaredMethods()) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { diff --git a/web/src/main/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilter.java b/web/src/main/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilter.java index 8e679b82ef..cb678959a9 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilter.java @@ -29,6 +29,7 @@ import jakarta.servlet.http.HttpServletResponse; import org.jspecify.annotations.Nullable; import org.springframework.core.log.LogMessage; +import org.springframework.lang.Contract; import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationManager; @@ -191,7 +192,7 @@ public class BasicAuthenticationFilter extends OncePerRequestFilter { if (authenticationIsRequired(username)) { Authentication authResult = this.authenticationManager.authenticate(authRequest); Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); - if (current != null && current.isAuthenticated() && declaresToBuilder(authResult)) { + if (shouldPerformMfa(current, authResult)) { authResult = authResult.toBuilder() // @formatter:off .authorities((a) -> { @@ -235,6 +236,17 @@ public class BasicAuthenticationFilter extends OncePerRequestFilter { chain.doFilter(request, response); } + @Contract("null, _ -> false") + private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) { + if (current == null || !current.isAuthenticated()) { + return false; + } + if (!declaresToBuilder(authenticationResult)) { + return false; + } + return current.getName().equals(authenticationResult.getName()); + } + private static boolean declaresToBuilder(Authentication authentication) { for (Method method : authentication.getClass().getDeclaredMethods()) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { diff --git a/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java index 5ecd64ac9d..447c815d1f 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java @@ -454,13 +454,32 @@ public class AbstractAuthenticationProcessingFilterTests { SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); MockHttpServletRequest request = createMockAuthenticationRequest(); MockHttpServletResponse response = new MockHttpServletResponse(); - MockAuthenticationFilter filter = new MockAuthenticationFilter(true); + Authentication newAuthn = UsernamePasswordAuthenticationToken.authenticated(existingAuthn.getName(), "test", + AuthorityUtils.createAuthorityList("TEST")); + MockAuthenticationFilter filter = new MockAuthenticationFilter(newAuthn); filter.doFilter(request, response, new MockFilterChain(false)); Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority) .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); } + // gh-18112 + @Test + void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception { + String ROLE_EXISTING = "ROLE_EXISTING"; + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + ROLE_EXISTING); + SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); + MockHttpServletRequest request = createMockAuthenticationRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + Authentication newAuthn = UsernamePasswordAuthenticationToken + .authenticated(existingAuthn.getName() + "different", "test", AuthorityUtils.createAuthorityList("TEST")); + MockAuthenticationFilter filter = new MockAuthenticationFilter(newAuthn); + filter.doFilter(request, response, new MockFilterChain(false)); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(authentication).isEqualTo(newAuthn); + } + /** * This is critical to avoid adding duplicate GrantedAuthority instances with the * same' authority when the issuedAt is too old and a new instance is requested. diff --git a/web/src/test/java/org/springframework/security/web/authentication/AuthenticationFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/AuthenticationFilterTests.java index c3dd84c11a..7184046b65 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/AuthenticationFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/AuthenticationFilterTests.java @@ -314,7 +314,7 @@ public class AuthenticationFilterTests { SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); given(this.authenticationConverter.convert(any())).willReturn(existingAuthn); given(this.authenticationManager.authenticate(any())) - .willReturn(new TestingAuthenticationToken("user", "password", "TEST")); + .willReturn(new TestingAuthenticationToken(existingAuthn.getName(), "password", "TEST")); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/"); MockHttpServletResponse response = new MockHttpServletResponse(); FilterChain chain = new MockFilterChain(); @@ -326,6 +326,27 @@ public class AuthenticationFilterTests { .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); } + // gh-18112 + @Test + public void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception { + String ROLE_EXISTING = "ROLE_EXISTING"; + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + ROLE_EXISTING); + SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); + given(this.authenticationConverter.convert(any())).willReturn(existingAuthn); + TestingAuthenticationToken expected = new TestingAuthenticationToken(existingAuthn.getName() + "different", + "password", "TEST"); + given(this.authenticationManager.authenticate(any())).willReturn(expected); + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/"); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain chain = new MockFilterChain(); + AuthenticationFilter filter = new AuthenticationFilter(this.authenticationManager, + this.authenticationConverter); + filter.doFilter(request, response, chain); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(authentication).isEqualTo(expected); + } + /** * This is critical to avoid adding duplicate GrantedAuthority instances with the * same' authority when the issuedAt is too old and a new instance is requested. diff --git a/web/src/test/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilterTests.java index 834acab5ce..b2644c9f2f 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilterTests.java @@ -415,6 +415,23 @@ public class AbstractPreAuthenticatedProcessingFilterTests { // @formatter:on } + // gh-18112 + @Test + void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception { + String ROLE_EXISTING = "ROLE_EXISTING"; + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + ROLE_EXISTING); + SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + TestingAuthenticationToken newAuthn = new TestingAuthenticationToken(existingAuthn.getName() + "different", + "password", "TEST"); + this.filter = createFilterAuthenticatesWith(newAuthn); + this.filter.doFilter(request, response, new MockFilterChain()); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(authentication).isEqualTo(newAuthn); + } + /** * This is critical to avoid adding duplicate GrantedAuthority instances with the * same' authority when the issuedAt is too old and a new instance is requested. diff --git a/web/src/test/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilterTests.java index c13422e8a3..185a6876d5 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilterTests.java @@ -517,6 +517,26 @@ public class BasicAuthenticationFilterTests { .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); } + // gh-18112 + @Test + void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception { + String ROLE_EXISTING = "ROLE_EXISTING"; + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + ROLE_EXISTING); + SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addHeader(HttpHeaders.AUTHORIZATION, "Basic " + CodecTestUtils.encodeBase64("a:b")); + MockHttpServletResponse response = new MockHttpServletResponse(); + AuthenticationManager manager = mock(AuthenticationManager.class); + TestingAuthenticationToken newAuthn = new TestingAuthenticationToken(existingAuthn.getName() + "different", + "password", "TEST"); + given(manager.authenticate(any())).willReturn(newAuthn); + BasicAuthenticationFilter filter = new BasicAuthenticationFilter(manager); + filter.doFilter(request, response, new MockFilterChain()); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(authentication).isEqualTo(newAuthn); + } + /** * This is critical to avoid adding duplicate GrantedAuthority instances with the * same' authority when the issuedAt is too old and a new instance is requested.