Skip to content

Commit 64c9e3e

Browse files
rwinchrwinch
committed
Prevent Dupliate GrantedAuthority#getAuthority()
If the GrantedAuthority is not equal, but contains a duplicate GrantedAuthority#getAuthority() then at the time of authentication, the Filter or WebFilter will duplicate the GrantedAuthority which leads to a memory leak. This is important to avoid for when we add support for a GrantedAuthority that might have an issuedAt attribute. If it is too old, then we'd want only the new GrantedAuthority to be added and the old instance to be removed. However, the two GrantedAuthority instances will not be equal because the issuedAt will not be equal. Closes gh-17981
1 parent c901034 commit 64c9e3e

File tree

11 files changed

+408
-15
lines changed

11 files changed

+408
-15
lines changed

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package org.springframework.security.web.authentication;
1818

1919
import java.io.IOException;
20+
import java.util.Set;
21+
import java.util.stream.Collectors;
2022

2123
import jakarta.servlet.FilterChain;
2224
import jakarta.servlet.ServletException;
@@ -39,6 +41,7 @@
3941
import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent;
4042
import org.springframework.security.core.Authentication;
4143
import org.springframework.security.core.AuthenticationException;
44+
import org.springframework.security.core.GrantedAuthority;
4245
import org.springframework.security.core.SpringSecurityMessageSource;
4346
import org.springframework.security.core.context.SecurityContext;
4447
import org.springframework.security.core.context.SecurityContextHolder;
@@ -251,8 +254,19 @@ private void doFilter(HttpServletRequest request, HttpServletResponse response,
251254
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
252255
if (current != null && current.isAuthenticated()) {
253256
authenticationResult = authenticationResult.toBuilder()
254-
.authorities((a) -> a.addAll(current.getAuthorities()))
257+
// @formatter:off
258+
.authorities((a) -> {
259+
Set<String> newAuthorities = a.stream()
260+
.map(GrantedAuthority::getAuthority)
261+
.collect(Collectors.toUnmodifiableSet());
262+
for (GrantedAuthority currentAuthority : current.getAuthorities()) {
263+
if (!newAuthorities.contains(currentAuthority.getAuthority())) {
264+
a.add(currentAuthority);
265+
}
266+
}
267+
})
255268
.build();
269+
// @formatter:on
256270
}
257271
this.sessionStrategy.onAuthentication(authenticationResult, request, response);
258272
// Authentication success

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package org.springframework.security.web.authentication;
1818

1919
import java.io.IOException;
20+
import java.util.Set;
21+
import java.util.stream.Collectors;
2022

2123
import jakarta.servlet.Filter;
2224
import jakarta.servlet.FilterChain;
@@ -31,6 +33,7 @@
3133
import org.springframework.security.authentication.AuthenticationManagerResolver;
3234
import org.springframework.security.core.Authentication;
3335
import org.springframework.security.core.AuthenticationException;
36+
import org.springframework.security.core.GrantedAuthority;
3437
import org.springframework.security.core.context.SecurityContext;
3538
import org.springframework.security.core.context.SecurityContextHolder;
3639
import org.springframework.security.core.context.SecurityContextHolderStrategy;
@@ -187,8 +190,19 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
187190
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
188191
if (current != null && current.isAuthenticated()) {
189192
authenticationResult = authenticationResult.toBuilder()
190-
.authorities((a) -> a.addAll(current.getAuthorities()))
193+
// @formatter:off
194+
.authorities((a) -> {
195+
Set<String> newAuthorities = a.stream()
196+
.map(GrantedAuthority::getAuthority)
197+
.collect(Collectors.toUnmodifiableSet());
198+
for (GrantedAuthority currentAuthority : current.getAuthorities()) {
199+
if (!newAuthorities.contains(currentAuthority.getAuthority())) {
200+
a.add(currentAuthority);
201+
}
202+
}
203+
})
191204
.build();
205+
// @formatter:on
192206
}
193207
HttpSession session = request.getSession(false);
194208
if (session != null) {

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package org.springframework.security.web.authentication.preauth;
1818

1919
import java.io.IOException;
20+
import java.util.Set;
21+
import java.util.stream.Collectors;
2022

2123
import jakarta.servlet.FilterChain;
2224
import jakarta.servlet.ServletException;
@@ -35,6 +37,7 @@
3537
import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent;
3638
import org.springframework.security.core.Authentication;
3739
import org.springframework.security.core.AuthenticationException;
40+
import org.springframework.security.core.GrantedAuthority;
3841
import org.springframework.security.core.context.SecurityContext;
3942
import org.springframework.security.core.context.SecurityContextHolder;
4043
import org.springframework.security.core.context.SecurityContextHolderStrategy;
@@ -207,8 +210,19 @@ private void doAuthenticate(HttpServletRequest request, HttpServletResponse resp
207210
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
208211
if (current != null && current.isAuthenticated()) {
209212
authenticationResult = authenticationResult.toBuilder()
210-
.authorities((a) -> a.addAll(current.getAuthorities()))
213+
// @formatter:off
214+
.authorities((a) -> {
215+
Set<String> newAuthorities = a.stream()
216+
.map(GrantedAuthority::getAuthority)
217+
.collect(Collectors.toUnmodifiableSet());
218+
for (GrantedAuthority currentAuthority : current.getAuthorities()) {
219+
if (!newAuthorities.contains(currentAuthority.getAuthority())) {
220+
a.add(currentAuthority);
221+
}
222+
}
223+
})
211224
.build();
225+
// @formatter:on
212226
}
213227
successfulAuthentication(request, response, authenticationResult);
214228
}

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818

1919
import java.io.IOException;
2020
import java.nio.charset.Charset;
21+
import java.util.Set;
22+
import java.util.stream.Collectors;
2123

2224
import jakarta.servlet.FilterChain;
2325
import jakarta.servlet.ServletException;
@@ -31,6 +33,7 @@
3133
import org.springframework.security.authentication.AuthenticationManager;
3234
import org.springframework.security.core.Authentication;
3335
import org.springframework.security.core.AuthenticationException;
36+
import org.springframework.security.core.GrantedAuthority;
3437
import org.springframework.security.core.context.SecurityContext;
3538
import org.springframework.security.core.context.SecurityContextHolder;
3639
import org.springframework.security.core.context.SecurityContextHolderStrategy;
@@ -188,7 +191,20 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
188191
Authentication authResult = this.authenticationManager.authenticate(authRequest);
189192
Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication();
190193
if (current != null && current.isAuthenticated()) {
191-
authResult = authResult.toBuilder().authorities((a) -> a.addAll(current.getAuthorities())).build();
194+
authResult = authResult.toBuilder()
195+
// @formatter:off
196+
.authorities((a) -> {
197+
Set<String> newAuthorities = a.stream()
198+
.map(GrantedAuthority::getAuthority)
199+
.collect(Collectors.toUnmodifiableSet());
200+
for (GrantedAuthority currentAuthority : current.getAuthorities()) {
201+
if (!newAuthorities.contains(currentAuthority.getAuthority())) {
202+
a.add(currentAuthority);
203+
}
204+
}
205+
})
206+
.build();
207+
// @formatter:on
192208
}
193209
SecurityContext context = this.securityContextHolderStrategy.createEmptyContext();
194210
context.setAuthentication(authResult);

‎web/src/main/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616

1717
package org.springframework.security.web.server.authentication;
1818

19+
import java.util.Set;
1920
import java.util.function.Function;
21+
import java.util.stream.Collectors;
2022

2123
import org.apache.commons.logging.Log;
2224
import org.apache.commons.logging.LogFactory;
@@ -27,6 +29,7 @@
2729
import org.springframework.security.authentication.ReactiveAuthenticationManagerResolver;
2830
import org.springframework.security.core.Authentication;
2931
import org.springframework.security.core.AuthenticationException;
32+
import org.springframework.security.core.GrantedAuthority;
3033
import org.springframework.security.core.context.ReactiveSecurityContextHolder;
3134
import org.springframework.security.core.context.SecurityContextImpl;
3235
import org.springframework.security.web.server.WebFilterExchange;
@@ -138,7 +141,20 @@ private Mono<Authentication> applyCurrentAuthenication(Authentication result) {
138141
if (!current.isAuthenticated()) {
139142
return result;
140143
}
141-
return result.toBuilder().authorities((a) -> a.addAll(current.getAuthorities())).build();
144+
return result.toBuilder()
145+
// @formatter:off
146+
.authorities((a) -> {
147+
Set<String> newAuthorities = a.stream()
148+
.map(GrantedAuthority::getAuthority)
149+
.collect(Collectors.toUnmodifiableSet());
150+
for (GrantedAuthority currentAuthority : current.getAuthorities()) {
151+
if (!newAuthorities.contains(currentAuthority.getAuthority())) {
152+
a.add(currentAuthority);
153+
}
154+
}
155+
})
156+
.build();
157+
// @formatter:on
142158
}).switchIfEmpty(Mono.just(result));
143159
}
144160

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

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,16 @@
1616

1717
package org.springframework.security.web.authentication;
1818

19+
import java.util.ArrayList;
20+
1921
import jakarta.servlet.FilterChain;
2022
import jakarta.servlet.ServletRequest;
2123
import jakarta.servlet.ServletResponse;
2224
import jakarta.servlet.http.HttpServletRequest;
2325
import jakarta.servlet.http.HttpServletResponse;
2426
import jakarta.servlet.http.HttpSession;
2527
import org.apache.commons.logging.Log;
28+
import org.jspecify.annotations.Nullable;
2629
import org.junit.jupiter.api.AfterEach;
2730
import org.junit.jupiter.api.BeforeEach;
2831
import org.junit.jupiter.api.Test;
@@ -35,12 +38,15 @@
3538
import org.springframework.security.authentication.BadCredentialsException;
3639
import org.springframework.security.authentication.InternalAuthenticationServiceException;
3740
import org.springframework.security.authentication.TestAuthentication;
41+
import org.springframework.security.authentication.TestingAuthenticationToken;
3842
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
3943
import org.springframework.security.core.Authentication;
4044
import org.springframework.security.core.AuthenticationException;
45+
import org.springframework.security.core.GrantedAuthority;
4146
import org.springframework.security.core.authority.AuthorityUtils;
4247
import org.springframework.security.core.context.SecurityContext;
4348
import org.springframework.security.core.context.SecurityContextHolder;
49+
import org.springframework.security.core.context.SecurityContextImpl;
4450
import org.springframework.security.web.authentication.rememberme.AbstractRememberMeServicesTests;
4551
import org.springframework.security.web.authentication.rememberme.TokenBasedRememberMeServices;
4652
import org.springframework.security.web.authentication.session.SessionAuthenticationStrategy;
@@ -438,6 +444,42 @@ public void loginErrorWithInternAuthenticationServiceExceptionLogsError() throws
438444
assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
439445
}
440446

447+
@Test
448+
void doFilterWhenAuthenticatedThenCombinesAuthorities() throws Exception {
449+
String ROLE_EXISTING = "ROLE_EXISTING";
450+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
451+
ROLE_EXISTING);
452+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
453+
MockHttpServletRequest request = createMockAuthenticationRequest();
454+
MockHttpServletResponse response = new MockHttpServletResponse();
455+
MockAuthenticationFilter filter = new MockAuthenticationFilter(true);
456+
filter.doFilter(request, response, new MockFilterChain(false));
457+
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
458+
assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority)
459+
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST");
460+
}
461+
462+
/**
463+
* This is critical to avoid adding duplicate GrantedAuthority instances with the
464+
* same' authority when the issuedAt is too old and a new instance is requested.
465+
* @throws Exception
466+
*/
467+
@Test
468+
void doFilterWhenDefaultEqualsAuthorityThenNoDuplicates() throws Exception {
469+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
470+
new DefaultEqualsGrantedAuthority());
471+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
472+
MockHttpServletRequest request = createMockAuthenticationRequest();
473+
MockHttpServletResponse response = new MockHttpServletResponse();
474+
MockAuthenticationFilter filter = new MockAuthenticationFilter(
475+
new TestingAuthenticationToken("username", "password", new DefaultEqualsGrantedAuthority()));
476+
filter.doFilter(request, response, new MockFilterChain(false));
477+
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
478+
assertThat(new ArrayList<GrantedAuthority>(authentication.getAuthorities()))
479+
.extracting(GrantedAuthority::getAuthority)
480+
.containsExactly(DefaultEqualsGrantedAuthority.AUTHORITY);
481+
}
482+
441483
/**
442484
* https://github.com/spring-projects/spring-security/pull/3905
443485
*/
@@ -453,38 +495,41 @@ private class MockAuthenticationFilter extends AbstractAuthenticationProcessingF
453495

454496
private AuthenticationException exceptionToThrow;
455497

456-
private boolean grantAccess;
498+
private final @Nullable Authentication authentication;
457499

458-
MockAuthenticationFilter(boolean grantAccess) {
459-
this();
500+
MockAuthenticationFilter(Authentication authentication) {
501+
super(DEFAULT_FILTER_PROCESSING_URL);
502+
this.authentication = authentication;
460503
setupRememberMeServicesAndAuthenticationException();
461-
this.grantAccess = grantAccess;
504+
}
505+
506+
MockAuthenticationFilter(boolean grantAccess) {
507+
this(createDefaultAuthentication(grantAccess));
462508
}
463509

464510
private MockAuthenticationFilter() {
465-
super(DEFAULT_FILTER_PROCESSING_URL);
511+
this(null);
466512
}
467513

468514
private MockAuthenticationFilter(String defaultFilterProcessingUrl,
469515
AuthenticationManager authenticationManager) {
470516
super(defaultFilterProcessingUrl, authenticationManager);
471517
setupRememberMeServicesAndAuthenticationException();
472-
this.grantAccess = true;
518+
this.authentication = createDefaultAuthentication(true);
473519
}
474520

475521
private MockAuthenticationFilter(RequestMatcher requiresAuthenticationRequestMatcher,
476522
AuthenticationManager authenticationManager) {
477523
super(requiresAuthenticationRequestMatcher, authenticationManager);
478524
setupRememberMeServicesAndAuthenticationException();
479-
this.grantAccess = true;
525+
this.authentication = createDefaultAuthentication(true);
480526
}
481527

482528
@Override
483529
public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response)
484530
throws AuthenticationException {
485-
if (this.grantAccess) {
486-
return UsernamePasswordAuthenticationToken.authenticated("test", "test",
487-
AuthorityUtils.createAuthorityList("TEST"));
531+
if (this.authentication != null) {
532+
return this.authentication;
488533
}
489534
else {
490535
throw this.exceptionToThrow;
@@ -496,6 +541,14 @@ private void setupRememberMeServicesAndAuthenticationException() {
496541
this.exceptionToThrow = new BadCredentialsException("Mock requested to do so");
497542
}
498543

544+
private static @Nullable Authentication createDefaultAuthentication(boolean grantAccess) {
545+
if (!grantAccess) {
546+
return null;
547+
}
548+
return UsernamePasswordAuthenticationToken.authenticated("test", "test",
549+
AuthorityUtils.createAuthorityList("TEST"));
550+
}
551+
499552
}
500553

501554
private class MockFilterChain implements FilterChain {

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.springframework.security.authentication.BadCredentialsException;
4040
import org.springframework.security.authentication.TestingAuthenticationToken;
4141
import org.springframework.security.core.Authentication;
42+
import org.springframework.security.core.GrantedAuthority;
4243
import org.springframework.security.core.context.SecurityContext;
4344
import org.springframework.security.core.context.SecurityContextHolder;
4445
import org.springframework.security.core.context.SecurityContextHolderStrategy;
@@ -303,6 +304,50 @@ public void filterWhenSuccessfulAuthenticationThenNoSessionCreated() throws Exce
303304
assertThat(request.getSession(false)).isNull();
304305
}
305306

307+
@Test
308+
public void doFilterWhenAuthenticatedThenCombinesAuthorities() throws Exception {
309+
String ROLE_EXISTING = "ROLE_EXISTING";
310+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
311+
ROLE_EXISTING);
312+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
313+
given(this.authenticationConverter.convert(any())).willReturn(existingAuthn);
314+
given(this.authenticationManager.authenticate(any()))
315+
.willReturn(new TestingAuthenticationToken("user", "password", "TEST"));
316+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/");
317+
MockHttpServletResponse response = new MockHttpServletResponse();
318+
FilterChain chain = new MockFilterChain();
319+
AuthenticationFilter filter = new AuthenticationFilter(this.authenticationManager,
320+
this.authenticationConverter);
321+
filter.doFilter(request, response, chain);
322+
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
323+
assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority)
324+
.containsExactlyInAnyOrder(ROLE_EXISTING, "TEST");
325+
}
326+
327+
/**
328+
* This is critical to avoid adding duplicate GrantedAuthority instances with the
329+
* same' authority when the issuedAt is too old and a new instance is requested.
330+
* @throws Exception
331+
*/
332+
@Test
333+
public void doFilterWhenDefaultEqualsGrantedAuthorityThenNoDuplicates() throws Exception {
334+
TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password",
335+
new DefaultEqualsGrantedAuthority());
336+
SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn));
337+
given(this.authenticationConverter.convert(any())).willReturn(existingAuthn);
338+
given(this.authenticationManager.authenticate(any()))
339+
.willReturn(new TestingAuthenticationToken("user", "password", new DefaultEqualsGrantedAuthority()));
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.getAuthorities()).extracting(GrantedAuthority::getAuthority)
348+
.containsExactlyInAnyOrder(DefaultEqualsGrantedAuthority.AUTHORITY);
349+
}
350+
306351
@Test
307352
public void filterWhenCustomSecurityContextRepositoryAndSuccessfulAuthenticationRepositoryUsed() throws Exception {
308353
SecurityContextRepository securityContextRepository = mock(SecurityContextRepository.class);

0 commit comments

Comments
 (0)