From 7250abc1859ded7927fcb987ee47ccb02bcc139f Mon Sep 17 00:00:00 2001 From: Marcus Da Coregio Date: Fri, 26 May 2023 09:41:28 -0300 Subject: [PATCH] Does not apply a Configurer when disabled from another DSL Closes gh-13203 --- .../AbstractConfiguredSecurityBuilder.java | 6 +++ ...bstractConfiguredSecurityBuilderTests.java | 45 +++++++++++++++++++ .../HttpSecurityConfigurationTests.java | 39 ++++++++++++++++ 3 files changed, 90 insertions(+) diff --git a/config/src/main/java/org/springframework/security/config/annotation/AbstractConfiguredSecurityBuilder.java b/config/src/main/java/org/springframework/security/config/annotation/AbstractConfiguredSecurityBuilder.java index b9d6ebed1e..588bcb7f52 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/AbstractConfiguredSecurityBuilder.java +++ b/config/src/main/java/org/springframework/security/config/annotation/AbstractConfiguredSecurityBuilder.java @@ -221,6 +221,7 @@ public abstract class AbstractConfiguredSecurityBuilder(); } + removeFromConfigurersAddedInInitializing(clazz); return new ArrayList<>(configs); } @@ -253,11 +254,16 @@ public abstract class AbstractConfiguredSecurityBuilder "Only one configurer expected for type " + clazz + ", but got " + configs); return (C) configs.get(0); } + private > void removeFromConfigurersAddedInInitializing(Class clazz) { + this.configurersAddedInInitializing.removeIf(clazz::isInstance); + } + /** * Specifies the {@link ObjectPostProcessor} to use. * @param objectPostProcessor the {@link ObjectPostProcessor} to use. Cannot be null diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/AbstractConfiguredSecurityBuilderTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/AbstractConfiguredSecurityBuilderTests.java index ee59b1aaf5..a7b55ef021 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/AbstractConfiguredSecurityBuilderTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/AbstractConfiguredSecurityBuilderTests.java @@ -30,6 +30,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; /** @@ -83,6 +84,24 @@ public class AbstractConfiguredSecurityBuilderTests { verify(DelegateSecurityConfigurer.CONFIGURER).configure(this.builder); } + @Test + public void buildWhenConfigurerAppliesAndRemoveAnotherConfigurerThenNotConfigured() throws Exception { + ApplyAndRemoveSecurityConfigurer.CONFIGURER = mock(SecurityConfigurer.class); + this.builder.apply(new ApplyAndRemoveSecurityConfigurer()); + this.builder.build(); + verify(ApplyAndRemoveSecurityConfigurer.CONFIGURER, never()).init(this.builder); + verify(ApplyAndRemoveSecurityConfigurer.CONFIGURER, never()).configure(this.builder); + } + + @Test + public void buildWhenConfigurerAppliesAndRemoveAnotherConfigurersThenNotConfigured() throws Exception { + ApplyAndRemoveAllSecurityConfigurer.CONFIGURER = mock(SecurityConfigurer.class); + this.builder.apply(new ApplyAndRemoveAllSecurityConfigurer()); + this.builder.build(); + verify(ApplyAndRemoveAllSecurityConfigurer.CONFIGURER, never()).init(this.builder); + verify(ApplyAndRemoveAllSecurityConfigurer.CONFIGURER, never()).configure(this.builder); + } + @Test public void getConfigurerWhenMultipleConfigurersThenThrowIllegalStateException() throws Exception { TestConfiguredSecurityBuilder builder = new TestConfiguredSecurityBuilder(mock(ObjectPostProcessor.class), @@ -130,6 +149,32 @@ public class AbstractConfiguredSecurityBuilderTests { assertThat(builder.getConfigurers(DelegateSecurityConfigurer.class)).hasSize(2); } + private static class ApplyAndRemoveSecurityConfigurer + extends SecurityConfigurerAdapter { + + private static SecurityConfigurer CONFIGURER; + + @Override + public void init(TestConfiguredSecurityBuilder builder) throws Exception { + builder.apply(CONFIGURER); + builder.removeConfigurer(CONFIGURER.getClass()); + } + + } + + private static class ApplyAndRemoveAllSecurityConfigurer + extends SecurityConfigurerAdapter { + + private static SecurityConfigurer CONFIGURER; + + @Override + public void init(TestConfiguredSecurityBuilder builder) throws Exception { + builder.apply(CONFIGURER); + builder.removeConfigurers(CONFIGURER.getClass()); + } + + } + private static class DelegateSecurityConfigurer extends SecurityConfigurerAdapter { diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configuration/HttpSecurityConfigurationTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/HttpSecurityConfigurationTests.java index 8713b17121..d65b11024c 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configuration/HttpSecurityConfigurationTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/HttpSecurityConfigurationTests.java @@ -21,6 +21,7 @@ import java.util.Arrays; import java.util.List; import java.util.concurrent.Callable; +import javax.servlet.Filter; import javax.servlet.http.HttpServletRequest; import com.google.common.net.HttpHeaders; @@ -45,6 +46,7 @@ import org.springframework.security.authentication.event.AbstractAuthenticationF import org.springframework.security.authentication.event.AuthenticationSuccessEvent; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer; +import org.springframework.security.config.annotation.web.configurers.FormLoginConfigurer; import org.springframework.security.config.test.SpringTestContext; import org.springframework.security.config.test.SpringTestContextExtension; import org.springframework.security.core.Authentication; @@ -55,6 +57,8 @@ import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.provisioning.InMemoryUserDetailsManager; import org.springframework.security.web.SecurityFilterChain; +import org.springframework.security.web.authentication.ui.DefaultLoginPageGeneratingFilter; +import org.springframework.security.web.authentication.ui.DefaultLogoutPageGeneratingFilter; import org.springframework.security.web.header.writers.frameoptions.XFrameOptionsHeaderWriter; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; @@ -292,6 +296,16 @@ public class HttpSecurityConfigurationTests { assertThat(configurer.configure).isTrue(); } + // gh-13203 + @Test + public void disableConfigurerWhenAppliedByAnotherConfigurerThenNotApplied() { + this.spring.register(ApplyCustomDslConfig.class).autowire(); + SecurityFilterChain filterChain = this.spring.getContext().getBean(SecurityFilterChain.class); + List filters = filterChain.getFilters(); + assertThat(filters).doesNotHaveAnyElementsOfTypes(DefaultLoginPageGeneratingFilter.class, + DefaultLogoutPageGeneratingFilter.class); + } + @RestController static class NameController { @@ -470,6 +484,31 @@ public class HttpSecurityConfigurationTests { } + @Configuration + @EnableWebSecurity + static class ApplyCustomDslConfig { + + @Bean + SecurityFilterChain filterChain(HttpSecurity http) throws Exception { + http.apply(CustomDsl.customDsl()); + return http.build(); + } + + } + + static class CustomDsl extends AbstractHttpConfigurer { + + @Override + public void init(HttpSecurity http) throws Exception { + http.formLogin(FormLoginConfigurer::disable); + } + + static CustomDsl customDsl() { + return new CustomDsl(); + } + + } + static class DefaultConfigurer extends AbstractHttpConfigurer { boolean init;