From 64c9e3e210d60b775ae0bcdea004fc1fc213e1ca Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Wed, 1 Oct 2025 15:23:54 -0500 Subject: [PATCH] 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 --- ...bstractAuthenticationProcessingFilter.java | 16 +++- .../authentication/AuthenticationFilter.java | 16 +++- ...tractPreAuthenticatedProcessingFilter.java | 16 +++- .../www/BasicAuthenticationFilter.java | 18 ++++- .../AuthenticationWebFilter.java | 18 ++++- ...ctAuthenticationProcessingFilterTests.java | 73 ++++++++++++++--- .../AuthenticationFilterTests.java | 45 +++++++++++ .../DefaultEqualsGrantedAuthority.java | 37 +++++++++ ...PreAuthenticatedProcessingFilterTests.java | 56 +++++++++++++ .../www/BasicAuthenticationFilterTests.java | 47 +++++++++++ .../AuthenticationWebFilterTests.java | 81 +++++++++++++++++++ 11 files changed, 408 insertions(+), 15 deletions(-) create mode 100644 web/src/test/java/org/springframework/security/web/authentication/DefaultEqualsGrantedAuthority.java 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 6bbbb3f766..19c1dca43a 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 @@ -17,6 +17,8 @@ package org.springframework.security.web.authentication; import java.io.IOException; +import java.util.Set; +import java.util.stream.Collectors; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -39,6 +41,7 @@ import org.springframework.security.authentication.InternalAuthenticationService import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.SpringSecurityMessageSource; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; @@ -251,8 +254,19 @@ public abstract class AbstractAuthenticationProcessingFilter extends GenericFilt Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); if (current != null && current.isAuthenticated()) { authenticationResult = authenticationResult.toBuilder() - .authorities((a) -> a.addAll(current.getAuthorities())) + // @formatter:off + .authorities((a) -> { + Set newAuthorities = a.stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toUnmodifiableSet()); + for (GrantedAuthority currentAuthority : current.getAuthorities()) { + if (!newAuthorities.contains(currentAuthority.getAuthority())) { + a.add(currentAuthority); + } + } + }) .build(); + // @formatter:on } this.sessionStrategy.onAuthentication(authenticationResult, request, response); // Authentication success 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 8295a962bc..c1d6d08a24 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 @@ -17,6 +17,8 @@ package org.springframework.security.web.authentication; import java.io.IOException; +import java.util.Set; +import java.util.stream.Collectors; import jakarta.servlet.Filter; import jakarta.servlet.FilterChain; @@ -31,6 +33,7 @@ import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationManagerResolver; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextHolderStrategy; @@ -187,8 +190,19 @@ public class AuthenticationFilter extends OncePerRequestFilter { Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); if (current != null && current.isAuthenticated()) { authenticationResult = authenticationResult.toBuilder() - .authorities((a) -> a.addAll(current.getAuthorities())) + // @formatter:off + .authorities((a) -> { + Set newAuthorities = a.stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toUnmodifiableSet()); + for (GrantedAuthority currentAuthority : current.getAuthorities()) { + if (!newAuthorities.contains(currentAuthority.getAuthority())) { + a.add(currentAuthority); + } + } + }) .build(); + // @formatter:on } HttpSession session = request.getSession(false); if (session != null) { 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 b0910c85ed..3d8f8acc3f 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 @@ -17,6 +17,8 @@ package org.springframework.security.web.authentication.preauth; import java.io.IOException; +import java.util.Set; +import java.util.stream.Collectors; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -35,6 +37,7 @@ import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.event.InteractiveAuthenticationSuccessEvent; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextHolderStrategy; @@ -207,8 +210,19 @@ public abstract class AbstractPreAuthenticatedProcessingFilter extends GenericFi Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); if (current != null && current.isAuthenticated()) { authenticationResult = authenticationResult.toBuilder() - .authorities((a) -> a.addAll(current.getAuthorities())) + // @formatter:off + .authorities((a) -> { + Set newAuthorities = a.stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toUnmodifiableSet()); + for (GrantedAuthority currentAuthority : current.getAuthorities()) { + if (!newAuthorities.contains(currentAuthority.getAuthority())) { + a.add(currentAuthority); + } + } + }) .build(); + // @formatter:on } successfulAuthentication(request, response, authenticationResult); } 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 91f40b01b7..e2f6435e78 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 @@ -18,6 +18,8 @@ package org.springframework.security.web.authentication.www; import java.io.IOException; import java.nio.charset.Charset; +import java.util.Set; +import java.util.stream.Collectors; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -31,6 +33,7 @@ import org.springframework.security.authentication.AuthenticationDetailsSource; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextHolderStrategy; @@ -188,7 +191,20 @@ public class BasicAuthenticationFilter extends OncePerRequestFilter { Authentication authResult = this.authenticationManager.authenticate(authRequest); Authentication current = this.securityContextHolderStrategy.getContext().getAuthentication(); if (current != null && current.isAuthenticated()) { - authResult = authResult.toBuilder().authorities((a) -> a.addAll(current.getAuthorities())).build(); + authResult = authResult.toBuilder() + // @formatter:off + .authorities((a) -> { + Set newAuthorities = a.stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toUnmodifiableSet()); + for (GrantedAuthority currentAuthority : current.getAuthorities()) { + if (!newAuthorities.contains(currentAuthority.getAuthority())) { + a.add(currentAuthority); + } + } + }) + .build(); + // @formatter:on } SecurityContext context = this.securityContextHolderStrategy.createEmptyContext(); context.setAuthentication(authResult); diff --git a/web/src/main/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java b/web/src/main/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java index 2f83629181..47d06ee1ac 100644 --- a/web/src/main/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java +++ b/web/src/main/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java @@ -16,7 +16,9 @@ package org.springframework.security.web.server.authentication; +import java.util.Set; import java.util.function.Function; +import java.util.stream.Collectors; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -27,6 +29,7 @@ import org.springframework.security.authentication.ReactiveAuthenticationManager import org.springframework.security.authentication.ReactiveAuthenticationManagerResolver; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.ReactiveSecurityContextHolder; import org.springframework.security.core.context.SecurityContextImpl; import org.springframework.security.web.server.WebFilterExchange; @@ -138,7 +141,20 @@ public class AuthenticationWebFilter implements WebFilter { if (!current.isAuthenticated()) { return result; } - return result.toBuilder().authorities((a) -> a.addAll(current.getAuthorities())).build(); + return result.toBuilder() + // @formatter:off + .authorities((a) -> { + Set newAuthorities = a.stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toUnmodifiableSet()); + for (GrantedAuthority currentAuthority : current.getAuthorities()) { + if (!newAuthorities.contains(currentAuthority.getAuthority())) { + a.add(currentAuthority); + } + } + }) + .build(); + // @formatter:on }).switchIfEmpty(Mono.just(result)); } 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 caaef4a083..f57f601695 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 @@ -16,6 +16,8 @@ package org.springframework.security.web.authentication; +import java.util.ArrayList; + import jakarta.servlet.FilterChain; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletResponse; @@ -23,6 +25,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import jakarta.servlet.http.HttpSession; import org.apache.commons.logging.Log; +import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -35,12 +38,15 @@ import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.authentication.InternalAuthenticationServiceException; import org.springframework.security.authentication.TestAuthentication; +import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.context.SecurityContextImpl; import org.springframework.security.web.authentication.rememberme.AbstractRememberMeServicesTests; import org.springframework.security.web.authentication.rememberme.TokenBasedRememberMeServices; import org.springframework.security.web.authentication.session.SessionAuthenticationStrategy; @@ -438,6 +444,42 @@ public class AbstractAuthenticationProcessingFilterTests { assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); } + @Test + void doFilterWhenAuthenticatedThenCombinesAuthorities() 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(); + MockAuthenticationFilter filter = new MockAuthenticationFilter(true); + filter.doFilter(request, response, new MockFilterChain(false)); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority) + .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); + } + + /** + * 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. + * @throws Exception + */ + @Test + void doFilterWhenDefaultEqualsAuthorityThenNoDuplicates() throws Exception { + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + new DefaultEqualsGrantedAuthority()); + SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); + MockHttpServletRequest request = createMockAuthenticationRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + MockAuthenticationFilter filter = new MockAuthenticationFilter( + new TestingAuthenticationToken("username", "password", new DefaultEqualsGrantedAuthority())); + filter.doFilter(request, response, new MockFilterChain(false)); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(new ArrayList(authentication.getAuthorities())) + .extracting(GrantedAuthority::getAuthority) + .containsExactly(DefaultEqualsGrantedAuthority.AUTHORITY); + } + /** * https://github.com/spring-projects/spring-security/pull/3905 */ @@ -453,38 +495,41 @@ public class AbstractAuthenticationProcessingFilterTests { private AuthenticationException exceptionToThrow; - private boolean grantAccess; + private final @Nullable Authentication authentication; + + MockAuthenticationFilter(Authentication authentication) { + super(DEFAULT_FILTER_PROCESSING_URL); + this.authentication = authentication; + setupRememberMeServicesAndAuthenticationException(); + } MockAuthenticationFilter(boolean grantAccess) { - this(); - setupRememberMeServicesAndAuthenticationException(); - this.grantAccess = grantAccess; + this(createDefaultAuthentication(grantAccess)); } private MockAuthenticationFilter() { - super(DEFAULT_FILTER_PROCESSING_URL); + this(null); } private MockAuthenticationFilter(String defaultFilterProcessingUrl, AuthenticationManager authenticationManager) { super(defaultFilterProcessingUrl, authenticationManager); setupRememberMeServicesAndAuthenticationException(); - this.grantAccess = true; + this.authentication = createDefaultAuthentication(true); } private MockAuthenticationFilter(RequestMatcher requiresAuthenticationRequestMatcher, AuthenticationManager authenticationManager) { super(requiresAuthenticationRequestMatcher, authenticationManager); setupRememberMeServicesAndAuthenticationException(); - this.grantAccess = true; + this.authentication = createDefaultAuthentication(true); } @Override public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response) throws AuthenticationException { - if (this.grantAccess) { - return UsernamePasswordAuthenticationToken.authenticated("test", "test", - AuthorityUtils.createAuthorityList("TEST")); + if (this.authentication != null) { + return this.authentication; } else { throw this.exceptionToThrow; @@ -496,6 +541,14 @@ public class AbstractAuthenticationProcessingFilterTests { this.exceptionToThrow = new BadCredentialsException("Mock requested to do so"); } + private static @Nullable Authentication createDefaultAuthentication(boolean grantAccess) { + if (!grantAccess) { + return null; + } + return UsernamePasswordAuthenticationToken.authenticated("test", "test", + AuthorityUtils.createAuthorityList("TEST")); + } + } private class MockFilterChain implements FilterChain { 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 aaf17d62c9..9788ccf2f8 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 @@ -39,6 +39,7 @@ import org.springframework.security.authentication.AuthenticationManagerResolver import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextHolderStrategy; @@ -303,6 +304,50 @@ public class AuthenticationFilterTests { assertThat(request.getSession(false)).isNull(); } + @Test + public void doFilterWhenAuthenticatedThenCombinesAuthorities() 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); + given(this.authenticationManager.authenticate(any())) + .willReturn(new TestingAuthenticationToken("user", "password", "TEST")); + 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.getAuthorities()).extracting(GrantedAuthority::getAuthority) + .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); + } + + /** + * 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. + * @throws Exception + */ + @Test + public void doFilterWhenDefaultEqualsGrantedAuthorityThenNoDuplicates() throws Exception { + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + new DefaultEqualsGrantedAuthority()); + SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); + given(this.authenticationConverter.convert(any())).willReturn(existingAuthn); + given(this.authenticationManager.authenticate(any())) + .willReturn(new TestingAuthenticationToken("user", "password", new DefaultEqualsGrantedAuthority())); + 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.getAuthorities()).extracting(GrantedAuthority::getAuthority) + .containsExactlyInAnyOrder(DefaultEqualsGrantedAuthority.AUTHORITY); + } + @Test public void filterWhenCustomSecurityContextRepositoryAndSuccessfulAuthenticationRepositoryUsed() throws Exception { SecurityContextRepository securityContextRepository = mock(SecurityContextRepository.class); diff --git a/web/src/test/java/org/springframework/security/web/authentication/DefaultEqualsGrantedAuthority.java b/web/src/test/java/org/springframework/security/web/authentication/DefaultEqualsGrantedAuthority.java new file mode 100644 index 0000000000..1970ea2691 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/authentication/DefaultEqualsGrantedAuthority.java @@ -0,0 +1,37 @@ +/* + * Copyright 2004-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.web.authentication; + +import org.springframework.security.core.GrantedAuthority; + +/** + * Used for testing when a {@link GrantedAuthority} might not be equal, but the + * {@link #getAuthority()} is. + * + * @author Rob Winch + * @since 7.0 + */ +public class DefaultEqualsGrantedAuthority implements GrantedAuthority { + + public static final String AUTHORITY = "CUSTOM_AUTHORITY"; + + @Override + public String getAuthority() { + return AUTHORITY; + } + +} 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 94f66911ed..ebf578e4db 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 @@ -16,6 +16,8 @@ package org.springframework.security.web.authentication.preauth; +import java.util.ArrayList; + import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; @@ -33,14 +35,18 @@ import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.context.SecurityContextImpl; import org.springframework.security.core.userdetails.User; import org.springframework.security.web.WebAttributes; +import org.springframework.security.web.authentication.DefaultEqualsGrantedAuthority; import org.springframework.security.web.authentication.ForwardAuthenticationFailureHandler; import org.springframework.security.web.authentication.ForwardAuthenticationSuccessHandler; import org.springframework.security.web.context.SecurityContextRepository; +import org.springframework.security.web.util.matcher.AnyRequestMatcher; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; @@ -389,6 +395,56 @@ public class AbstractPreAuthenticatedProcessingFilterTests { verify(am).authenticate(any(PreAuthenticatedAuthenticationToken.class)); } + @Test + void doFilterWhenAuthenticatedThenCombinesAuthorities() 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(); + this.filter = createFilterAuthenticatesWith(new TestingAuthenticationToken("username", "password", "TEST")); + this.filter.doFilter(request, response, new MockFilterChain()); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + // @formatter:off + assertThat(authentication.getAuthorities()) + .extracting(GrantedAuthority::getAuthority) + .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); + // @formatter:on + } + + /** + * 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. + * @throws Exception + */ + @Test + void doFilterWhenDefaultEqualsGrantedAuthorityThenNoDuplicates() throws Exception { + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + new DefaultEqualsGrantedAuthority()); + SecurityContextHolder.setContext(new SecurityContextImpl(existingAuthn)); + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + this.filter = createFilterAuthenticatesWith( + new TestingAuthenticationToken("username", "password", new DefaultEqualsGrantedAuthority())); + this.filter.doFilter(request, response, new MockFilterChain()); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + // @formatter:off + assertThat(new ArrayList(authentication.getAuthorities())) + .extracting(GrantedAuthority::getAuthority) + .containsExactly(DefaultEqualsGrantedAuthority.AUTHORITY); + // @formatter:on + } + + private AbstractPreAuthenticatedProcessingFilter createFilterAuthenticatesWith(Authentication authentication) { + ConcretePreAuthenticatedProcessingFilter filter = new ConcretePreAuthenticatedProcessingFilter(); + filter.setRequiresAuthenticationRequestMatcher(AnyRequestMatcher.INSTANCE); + AuthenticationManager am = mock(AuthenticationManager.class); + given(am.authenticate(any())).willReturn(authentication); + filter.setAuthenticationManager(am); + return filter; + } + private void testDoFilter(boolean grantAccess) throws Exception { MockHttpServletRequest req = new MockHttpServletRequest(); MockHttpServletResponse res = new MockHttpServletResponse(); 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 c0bd3aab93..006e5f7b79 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 @@ -17,6 +17,7 @@ package org.springframework.security.web.authentication.www; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletRequest; @@ -28,6 +29,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; +import org.springframework.http.HttpHeaders; import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; @@ -37,12 +39,15 @@ import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextHolderStrategy; +import org.springframework.security.core.context.SecurityContextImpl; import org.springframework.security.test.web.CodecTestUtils; import org.springframework.security.web.authentication.AuthenticationConverter; +import org.springframework.security.web.authentication.DefaultEqualsGrantedAuthority; import org.springframework.security.web.authentication.WebAuthenticationDetails; import org.springframework.security.web.context.RequestAttributeSecurityContextRepository; import org.springframework.security.web.context.SecurityContextRepository; @@ -492,6 +497,48 @@ public class BasicAuthenticationFilterTests { verifyNoMoreInteractions(this.manager, filterChain); } + @Test + void doFilterWhenAuthenticatedThenCombinesAuthorities() 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); + given(manager.authenticate(any())).willReturn(new TestingAuthenticationToken("username", "password", "TEST")); + BasicAuthenticationFilter filter = new BasicAuthenticationFilter(manager); + filter.doFilter(request, response, new MockFilterChain()); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority) + .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); + } + + /** + * 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. + * @throws Exception + */ + @Test + void doFilterWhenDefaultEqualsGrantedAuthorityThenNoDuplicates() throws Exception { + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + new DefaultEqualsGrantedAuthority()); + 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); + given(manager.authenticate(any())) + .willReturn(new TestingAuthenticationToken("username", "password", new DefaultEqualsGrantedAuthority())); + BasicAuthenticationFilter filter = new BasicAuthenticationFilter(manager); + filter.doFilter(request, response, new MockFilterChain()); + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + assertThat(new ArrayList(authentication.getAuthorities())) + .extracting(GrantedAuthority::getAuthority) + .containsExactly(DefaultEqualsGrantedAuthority.AUTHORITY); + } + @Test public void doFilterWhenCustomAuthenticationConverterRequestThenAuthenticate() throws Exception { this.filter.setAuthenticationConverter(new TestAuthenticationConverter()); diff --git a/web/src/test/java/org/springframework/security/web/server/authentication/AuthenticationWebFilterTests.java b/web/src/test/java/org/springframework/security/web/server/authentication/AuthenticationWebFilterTests.java index 8613533e3a..a9c8c35b5a 100644 --- a/web/src/test/java/org/springframework/security/web/server/authentication/AuthenticationWebFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/server/authentication/AuthenticationWebFilterTests.java @@ -19,6 +19,7 @@ package org.springframework.security.web.server.authentication; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import reactor.core.publisher.Mono; @@ -28,12 +29,18 @@ import org.springframework.security.authentication.ReactiveAuthenticationManager import org.springframework.security.authentication.ReactiveAuthenticationManagerResolver; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.context.ReactiveSecurityContextHolder; +import org.springframework.security.core.context.SecurityContext; import org.springframework.security.test.web.reactive.server.WebTestClientBuilder; +import org.springframework.security.web.authentication.DefaultEqualsGrantedAuthority; import org.springframework.security.web.server.context.ServerSecurityContextRepository; import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher; import org.springframework.test.web.reactive.server.EntityExchangeResult; import org.springframework.test.web.reactive.server.WebTestClient; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.WebFilter; +import org.springframework.web.server.WebFilterChain; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; @@ -169,6 +176,60 @@ public class AuthenticationWebFilterTests { assertThat(result.getResponseCookies()).isEmpty(); } + @Test + public void filterWhenAuthenticatedThenCombinesAuthorities() { + String ROLE_EXISTING = "ROLE_EXISTING"; + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + ROLE_EXISTING); + given(this.authenticationManager.authenticate(any())) + .willReturn(Mono.just(new TestingAuthenticationToken("user", "password", "TEST"))); + given(this.securityContextRepository.save(any(), any())).willReturn(Mono.empty()); + this.filter = new AuthenticationWebFilter(this.authenticationManager); + this.filter.setSecurityContextRepository(this.securityContextRepository); + WebTestClient client = WebTestClientBuilder.bindToWebFilters(new RunAsWebFilter(existingAuthn), this.filter) + .build(); + client.get() + .uri("/") + .headers((headers) -> headers.setBasicAuth("test", "this")) + .exchange() + .expectStatus() + .isOk(); + ArgumentCaptor context = ArgumentCaptor.forClass(SecurityContext.class); + verify(this.securityContextRepository).save(any(), context.capture()); + Authentication authentication = context.getValue().getAuthentication(); + assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority) + .containsExactlyInAnyOrder(ROLE_EXISTING, "TEST"); + } + + /** + * 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. + * @throws Exception + */ + @Test + public void filterWhenDefaultEqualsAuthorityThenNoDuplicates() { + TestingAuthenticationToken existingAuthn = new TestingAuthenticationToken("username", "password", + new DefaultEqualsGrantedAuthority()); + given(this.authenticationManager.authenticate(any())).willReturn( + Mono.just(new TestingAuthenticationToken("user", "password", new DefaultEqualsGrantedAuthority()))); + given(this.securityContextRepository.save(any(), any())).willReturn(Mono.empty()); + this.filter = new AuthenticationWebFilter(this.authenticationManager); + this.filter.setSecurityContextRepository(this.securityContextRepository); + WebTestClient client = WebTestClientBuilder.bindToWebFilters(new RunAsWebFilter(existingAuthn), this.filter) + .build(); + client.get() + .uri("/") + .headers((headers) -> headers.setBasicAuth("test", "this")) + .exchange() + .expectStatus() + .isOk(); + ArgumentCaptor context = ArgumentCaptor.forClass(SecurityContext.class); + verify(this.securityContextRepository).save(any(), context.capture()); + Authentication authentication = context.getValue().getAuthentication(); + assertThat(authentication.getAuthorities()).extracting(GrantedAuthority::getAuthority) + .containsExactly(DefaultEqualsGrantedAuthority.AUTHORITY); + } + @Test public void filterWhenAuthenticationManagerResolverDefaultsAndAuthenticationFailThenUnauthorized() { given(this.authenticationManager.authenticate(any())) @@ -286,4 +347,24 @@ public class AuthenticationWebFilterTests { assertThatIllegalArgumentException().isThrownBy(() -> this.filter.setRequiresAuthenticationMatcher(null)); } + /** + * @author Rob Winch + * @since 7.0 + */ + private static final class RunAsWebFilter implements WebFilter { + + private final Authentication authentication; + + private RunAsWebFilter(Authentication authentication) { + this.authentication = authentication; + } + + @Override + public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { + return chain.filter(exchange) + .contextWrite(ReactiveSecurityContextHolder.withAuthentication(this.authentication)); + } + + } + }