Skip to content

Commit ccd39a2

Browse files
rwinchrwinch
committed
Only perform MFA if Authentication.getName() is the same
Closes gh-18112
1 parent 793820a commit ccd39a2

File tree

8 files changed

+131
-6
lines changed

8 files changed

+131
-6
lines changed

‎web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.springframework.context.MessageSourceAware;
3636
import org.springframework.context.support.MessageSourceAccessor;
3737
import org.springframework.core.log.LogMessage;
38+
import org.springframework.lang.Contract;
3839
import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
3940
import org.springframework.security.authentication.AuthenticationDetailsSource;
4041
import org.springframework.security.authentication.AuthenticationManager;
@@ -253,7 +254,7 @@ private void doFilter(HttpServletRequest request, HttpServletResponse response,
253254
return;
254255
}
255256
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
256-
if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) {
257+
if (shouldPerformMfa(current, authenticationResult)) {
257258
authenticationResult = authenticationResult.toBuilder()
258259
// @formatter:off
259260
.authorities((a) -> {
@@ -286,6 +287,17 @@ private void doFilter(HttpServletRequest request, HttpServletResponse response,
286287
}
287288
}
288289

290+
@Contract("null, _ -> false")
291+
private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) {
292+
if (current == null || !current.isAuthenticated()) {
293+
return false;
294+
}
295+
if (!declaresToBuilder(authenticationResult)) {
296+
return false;
297+
}
298+
return current.getName().equals(authenticationResult.getName());
299+
}
300+
289301
private static boolean declaresToBuilder(Authentication authentication) {
290302
for (Method method : authentication.getClass().getDeclaredMethods()) {
291303
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

‎web/src/main/java/org/springframework/security/web/authentication/AuthenticationFilter.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import org.jspecify.annotations.Nullable;
3131

3232
import org.springframework.http.HttpStatus;
33+
import org.springframework.lang.Contract;
3334
import org.springframework.security.authentication.AuthenticationManager;
3435
import org.springframework.security.authentication.AuthenticationManagerResolver;
3536
import org.springframework.security.core.Authentication;
@@ -189,7 +190,7 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
189190
return;
190191
}
191192
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
192-
if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) {
193+
if (shouldPerformMfa(current, authenticationResult)) {
193194
authenticationResult = authenticationResult.toBuilder()
194195
// @formatter:off
195196
.authorities((a) -> {
@@ -216,6 +217,17 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
216217
}
217218
}
218219

220+
@Contract("null, _ -> false")
221+
private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) {
222+
if (current == null || !current.isAuthenticated()) {
223+
return false;
224+
}
225+
if (!declaresToBuilder(authenticationResult)) {
226+
return false;
227+
}
228+
return current.getName().equals(authenticationResult.getName());
229+
}
230+
219231
private static boolean declaresToBuilder(Authentication authentication) {
220232
for (Method method : authentication.getClass().getDeclaredMethods()) {
221233
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

‎web/src/main/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilter.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.springframework.context.ApplicationEventPublisher;
3434
import org.springframework.context.ApplicationEventPublisherAware;
3535
import org.springframework.core.log.LogMessage;
36+
import org.springframework.lang.Contract;
3637
import org.springframework.security.authentication.AuthenticationDetailsSource;
3738
import org.springframework.security.authentication.AuthenticationManager;
3839
import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent;
@@ -209,7 +210,7 @@ private void doAuthenticate(HttpServletRequest request, HttpServletResponse resp
209210
authenticationRequest.setDetails(this.authenticationDetailsSource.buildDetails(request));
210211
Authentication authenticationResult = this.authenticationManager.authenticate(authenticationRequest);
211212
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
212-
if (current != null && current.isAuthenticated() && declaresToBuilder(authenticationResult)) {
213+
if (shouldPerformMfa(current, authenticationResult)) {
213214
authenticationResult = authenticationResult.toBuilder()
214215
// @formatter:off
215216
.authorities((a) -> {
@@ -235,6 +236,17 @@ private void doAuthenticate(HttpServletRequest request, HttpServletResponse resp
235236
}
236237
}
237238

239+
@Contract("null, _ -> false")
240+
private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) {
241+
if (current == null || !current.isAuthenticated()) {
242+
return false;
243+
}
244+
if (!declaresToBuilder(authenticationResult)) {
245+
return false;
246+
}
247+
return current.getName().equals(authenticationResult.getName());
248+
}
249+
238250
private static boolean declaresToBuilder(Authentication authentication) {
239251
for (Method method : authentication.getClass().getDeclaredMethods()) {
240252
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

‎web/src/main/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilter.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.jspecify.annotations.Nullable;
3030

3131
import org.springframework.core.log.LogMessage;
32+
import org.springframework.lang.Contract;
3233
import org.springframework.security.authentication.AnonymousAuthenticationToken;
3334
import org.springframework.security.authentication.AuthenticationDetailsSource;
3435
import org.springframework.security.authentication.AuthenticationManager;
@@ -191,7 +192,7 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
191192
if (authenticationIsRequired(username)) {
192193
Authentication authResult = this.authenticationManager.authenticate(authRequest);
193194
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
194-
if (current != null && current.isAuthenticated() && declaresToBuilder(authResult)) {
195+
if (shouldPerformMfa(current, authResult)) {
195196
authResult = authResult.toBuilder()
196197
// @formatter:off
197198
.authorities((a) -> {
@@ -235,6 +236,17 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
235236
chain.doFilter(request, response);
236237
}
237238

239+
@Contract("null, _ -> false")
240+
private boolean shouldPerformMfa(@Nullable Authentication current, Authentication authenticationResult) {
241+
if (current == null || !current.isAuthenticated()) {
242+
return false;
243+
}
244+
if (!declaresToBuilder(authenticationResult)) {
245+
return false;
246+
}
247+
return current.getName().equals(authenticationResult.getName());
248+
}
249+
238250
private static boolean declaresToBuilder(Authentication authentication) {
239251
for (Method method : authentication.getClass().getDeclaredMethods()) {
240252
if (method.getName().equals("toBuilder") && method.getParameterTypes().length == 0) {

‎web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -454,13 +454,32 @@ void doFilterWhenAuthenticatedThenCombinesAuthorities() throws Exception {
454454
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
455455
MockHttpServletRequest request = createMockAuthenticationRequest();
456456
MockHttpServletResponse response = new MockHttpServletResponse();
457-
MockAuthenticationFilter filter = new MockAuthenticationFilter(true);
457+
Authentication newAuthn = UsernamePasswordAuthenticationToken.authenticated(existingAuthn.getName(), "test",
458+
AuthorityUtils.createAuthorityList("TEST"));
459+
MockAuthenticationFilter filter = new MockAuthenticationFilter(newAuthn);
458460
filter.doFilter(request, response, new MockFilterChain(false));
459461
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
460462
assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority)
461463
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST");
462464
}
463465

466+
// gh-18112
467+
@Test
468+
void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception {
469+
String ROLE_EXISTING = "ROLE_EXISTING";
470+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
471+
ROLE_EXISTING);
472+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
473+
MockHttpServletRequest request = createMockAuthenticationRequest();
474+
MockHttpServletResponse response = new MockHttpServletResponse();
475+
Authentication newAuthn = UsernamePasswordAuthenticationToken
476+
.authenticated(existingAuthn.getName() + "different", "test", AuthorityUtils.createAuthorityList("TEST"));
477+
MockAuthenticationFilter filter = new MockAuthenticationFilter(newAuthn);
478+
filter.doFilter(request, response, new MockFilterChain(false));
479+
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
480+
assertThat(authentication).isEqualTo(newAuthn);
481+
}
482+
464483
/**
465484
* This is critical to avoid adding duplicate GrantedAuthority instances with the
466485
* same' authority when the issuedAt is too old and a new instance is requested.

‎web/src/test/java/org/springframework/security/web/authentication/AuthenticationFilterTests.java‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ public void doFilterWhenAuthenticatedThenCombinesAuthorities() throws Exception
314314
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
315315
given(this.authenticationConverter.convert(any())).willReturn(existingAuthn);
316316
given(this.authenticationManager.authenticate(any()))
317-
.willReturn(new TestingAuthenticationToken("user", "password", "TEST"));
317+
.willReturn(new TestingAuthenticationToken(existingAuthn.getName(), "password", "TEST"));
318318
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
319319
MockHttpServletResponse response = new MockHttpServletResponse();
320320
FilterChain chain = new MockFilterChain();
@@ -326,6 +326,27 @@ public void doFilterWhenAuthenticatedThenCombinesAuthorities() throws Exception
326326
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST");
327327
}
328328

329+
// gh-18112
330+
@Test
331+
public void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception {
332+
String ROLE_EXISTING = "ROLE_EXISTING";
333+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
334+
ROLE_EXISTING);
335+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
336+
given(this.authenticationConverter.convert(any())).willReturn(existingAuthn);
337+
TestingAuthenticationToken expected = new TestingAuthenticationToken(existingAuthn.getName() + "different",
338+
"password", "TEST");
339+
given(this.authenticationManager.authenticate(any())).willReturn(expected);
340+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
341+
MockHttpServletResponse response = new MockHttpServletResponse();
342+
FilterChain chain = new MockFilterChain();
343+
AuthenticationFilter filter = new AuthenticationFilter(this.authenticationManager,
344+
this.authenticationConverter);
345+
filter.doFilter(request, response, chain);
346+
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
347+
assertThat(authentication).isEqualTo(expected);
348+
}
349+
329350
/**
330351
* This is critical to avoid adding duplicate GrantedAuthority instances with the
331352
* same' authority when the issuedAt is too old and a new instance is requested.

‎web/src/test/java/org/springframework/security/web/authentication/preauth/AbstractPreAuthenticatedProcessingFilterTests.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,23 @@ void doFilterWhenAuthenticatedThenCombinesAuthorities() throws Exception {
415415
// @formatter:on
416416
}
417417

418+
// gh-18112
419+
@Test
420+
void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception {
421+
String ROLE_EXISTING = "ROLE_EXISTING";
422+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
423+
ROLE_EXISTING);
424+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
425+
MockHttpServletRequest request = new MockHttpServletRequest();
426+
MockHttpServletResponse response = new MockHttpServletResponse();
427+
TestingAuthenticationToken newAuthn = new TestingAuthenticationToken(existingAuthn.getName() + "different",
428+
"password", "TEST");
429+
this.filter = createFilterAuthenticatesWith(newAuthn);
430+
this.filter.doFilter(request, response, new MockFilterChain());
431+
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
432+
assertThat(authentication).isEqualTo(newAuthn);
433+
}
434+
418435
/**
419436
* This is critical to avoid adding duplicate GrantedAuthority instances with the
420437
* same' authority when the issuedAt is too old and a new instance is requested.

‎web/src/test/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilterTests.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,26 @@ void doFilterWhenAuthenticatedThenCombinesAuthorities() throws Exception {
517517
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST");
518518
}
519519

520+
// gh-18112
521+
@Test
522+
void doFilterWhenDifferentPrincipalThenDoesNotCombine() throws Exception {
523+
String ROLE_EXISTING = "ROLE_EXISTING";
524+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
525+
ROLE_EXISTING);
526+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
527+
MockHttpServletRequest request = new MockHttpServletRequest();
528+
request.addHeader(HttpHeaders.AUTHORIZATION, "Basic " + CodecTestUtils.encodeBase64("a:b"));
529+
MockHttpServletResponse response = new MockHttpServletResponse();
530+
AuthenticationManager manager = mock(AuthenticationManager.class);
531+
TestingAuthenticationToken newAuthn = new TestingAuthenticationToken(existingAuthn.getName() + "different",
532+
"password", "TEST");
533+
given(manager.authenticate(any())).willReturn(newAuthn);
534+
BasicAuthenticationFilter filter = new BasicAuthenticationFilter(manager);
535+
filter.doFilter(request, response, new MockFilterChain());
536+
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
537+
assertThat(authentication).isEqualTo(newAuthn);
538+
}
539+
520540
/**
521541
* This is critical to avoid adding duplicate GrantedAuthority instances with the
522542
* same' authority when the issuedAt is too old and a new instance is requested.

0 commit comments

Comments
 (0)