Only perform MFA if Authentication.getName() is the same

Closes gh-18112
This commit is contained in:
Rob Winch 2025-11-03 12:37:36 -06:00
parent 793820acfa
commit ccd39a23c9
No known key found for this signature in database
8 changed files with 131 additions and 6 deletions

View File

@ -35,6 +35,7 @@ import org.springframework.context.MessageSource;
import org.springframework.context.MessageSourceAware; import org.springframework.context.MessageSourceAware;
import org.springframework.context.support.MessageSourceAccessor; import org.springframework.context.support.MessageSourceAccessor;
import org.springframework.core.log.LogMessage; import org.springframework.core.log.LogMessage;
import org.springframework.lang.Contract;
import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException; import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationDetailsSource;
import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationManager;
@ -253,7 +254,7 @@ public abstract class AbstractAuthenticationProcessingFilter extends GenericFilt
return; return;
} }
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) { if (shouldPerformMfa(current, authenticationResult)) {
authenticationResult = authenticationResult.toBuilder() authenticationResult = authenticationResult.toBuilder()
// @formatter:off // @formatter:off
.authorities((a) -> { .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) { private static boolean declaresToBuilder(Authentication authentication) {
for (Method method : authentication.getClass().getDeclaredMethods()) { for (Method method : authentication.getClass().getDeclaredMethods()) {
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

View File

@ -30,6 +30,7 @@ import jakarta.servlet.http.HttpSession;
import org.jspecify.annotations.Nullable; import org.jspecify.annotations.Nullable;
import org.springframework.http.HttpStatus; import org.springframework.http.HttpStatus;
import org.springframework.lang.Contract;
import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationManager;
import org.springframework.security.authentication.AuthenticationManagerResolver; import org.springframework.security.authentication.AuthenticationManagerResolver;
import org.springframework.security.core.Authentication; import org.springframework.security.core.Authentication;
@ -189,7 +190,7 @@ public class AuthenticationFilter extends OncePerRequestFilter {
return; return;
} }
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) { if (shouldPerformMfa(current, authenticationResult)) {
authenticationResult = authenticationResult.toBuilder() authenticationResult = authenticationResult.toBuilder()
// @formatter:off // @formatter:off
.authorities((a) -> { .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) { private static boolean declaresToBuilder(Authentication authentication) {
for (Method method : authentication.getClass().getDeclaredMethods()) { for (Method method : authentication.getClass().getDeclaredMethods()) {
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

View File

@ -33,6 +33,7 @@ import org.jspecify.annotations.Nullable;
import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisher;
import org.springframework.context.ApplicationEventPublisherAware; import org.springframework.context.ApplicationEventPublisherAware;
import org.springframework.core.log.LogMessage; import org.springframework.core.log.LogMessage;
import org.springframework.lang.Contract;
import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationDetailsSource;
import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationManager;
import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent; import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent;
@ -209,7 +210,7 @@ public abstract class AbstractPreAuthenticatedProcessingFilter extends GenericFi
authenticationRequest.setDetails(this.authenticationDetailsSource.buildDetails(request)); authenticationRequest.setDetails(this.authenticationDetailsSource.buildDetails(request));
Authentication authenticationResult = this.authenticationManager.authenticate(authenticationRequest); Authentication authenticationResult = this.authenticationManager.authenticate(authenticationRequest);
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) { if (shouldPerformMfa(current, authenticationResult)) {
authenticationResult = authenticationResult.toBuilder() authenticationResult = authenticationResult.toBuilder()
// @formatter:off // @formatter:off
.authorities((a) -> { .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) { private static boolean declaresToBuilder(Authentication authentication) {
for (Method method : authentication.getClass().getDeclaredMethods()) { for (Method method : authentication.getClass().getDeclaredMethods()) {
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

View File

@ -29,6 +29,7 @@ import jakarta.servlet.http.HttpServletResponse;
import org.jspecify.annotations.Nullable; import org.jspecify.annotations.Nullable;
import org.springframework.core.log.LogMessage; import org.springframework.core.log.LogMessage;
import org.springframework.lang.Contract;
import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.authentication.AnonymousAuthenticationToken;
import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationDetailsSource;
import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationManager;
@ -191,7 +192,7 @@ public class BasicAuthenticationFilter extends OncePerRequestFilter {
if (authenticationIsRequired(username)) { if (authenticationIsRequired(username)) {
Authentication authResult = this.authenticationManager.authenticate(authRequest); Authentication authResult = this.authenticationManager.authenticate(authRequest);
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
if (current != null && current.isAuthenticated() && declaresToBuilder(authResult)) { if (shouldPerformMfa(current, authResult)) {
authResult = authResult.toBuilder() authResult = authResult.toBuilder()
// @formatter:off // @formatter:off
.authorities((a) -> { .authorities((a) -> {
@ -235,6 +236,17 @@ public class BasicAuthenticationFilter extends OncePerRequestFilter {
chain.doFilter(request, response); 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) { private static boolean declaresToBuilder(Authentication authentication) {
for (Method method : authentication.getClass().getDeclaredMethods()) { for (Method method : authentication.getClass().getDeclaredMethods()) {
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) { if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

View File

@ -454,13 +454,32 @@ public class AbstractAuthenticationProcessingFilterTests {
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
MockHttpServletRequest request = createMockAuthenticationRequest(); MockHttpServletRequest request = createMockAuthenticationRequest();
MockHttpServletResponse response = new MockHttpServletResponse(); 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)); filter.doFilter(request, response, new MockFilterChain(false));
Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority) assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority)
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); .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 * 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. * same' authority when the issuedAt is too old and a new instance is requested.

View File

@ -314,7 +314,7 @@ public class AuthenticationFilterTests {
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
given(this.authenticationConverter.convert(any())).willReturn(existingAuthn); given(this.authenticationConverter.convert(any())).willReturn(existingAuthn);
given(this.authenticationManager.authenticate(any())) given(this.authenticationManager.authenticate(any()))
.willReturn(new TestingAuthenticationToken("user", "password", "TEST")); .willReturn(new TestingAuthenticationToken(existingAuthn.getName(), "password", "TEST"));
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/"); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
MockHttpServletResponse response = new MockHttpServletResponse(); MockHttpServletResponse response = new MockHttpServletResponse();
FilterChain chain = new MockFilterChain(); FilterChain chain = new MockFilterChain();
@ -326,6 +326,27 @@ public class AuthenticationFilterTests {
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); .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 * 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. * same' authority when the issuedAt is too old and a new instance is requested.

View File

@ -415,6 +415,23 @@ public class AbstractPreAuthenticatedProcessingFilterTests {
// @formatter:on // @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 * 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. * same' authority when the issuedAt is too old and a new instance is requested.

View File

@ -517,6 +517,26 @@ public class BasicAuthenticationFilterTests {
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); .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 * 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. * same' authority when the issuedAt is too old and a new instance is requested.