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)); + } + + } + }