From 751b5580a129f9b3a1da02c7f49bffd6ed35ef32 Mon Sep 17 00:00:00 2001 From: Rob Winch <362503+rwinch@users.noreply.github.com> Date: Thu, 23 Jan 2025 12:43:22 -0600 Subject: [PATCH] TestOneTimeTokenGenerationSuccessHandler.lastToken to non-static variable Previously there were race conditions on the static member lastToken of TestOneTimeTokenGenerationSuccessHandler. This is because the tests run in parallel and one test may override the other tests lastToken and thus make the assertion on it incorrect. This commit changes lastToken to be a non-static variable to ensure that each test has it's own lastToken for asserting the expected value. Closes gh-16471 --- .../ott/OneTimeTokenLoginConfigurerTests.java | 46 ++++++++++++++----- .../web/OneTimeTokenLoginDslTests.kt | 37 ++++++++++----- 2 files changed, 60 insertions(+), 23 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ott/OneTimeTokenLoginConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ott/OneTimeTokenLoginConfigurerTests.java index f89a37ae40..72474ee825 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ott/OneTimeTokenLoginConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ott/OneTimeTokenLoginConfigurerTests.java @@ -72,7 +72,7 @@ public class OneTimeTokenLoginConfigurerTests { this.mvc.perform(post("/ott/generate").param("username", "user").with(csrf())) .andExpectAll(status().isFound(), redirectedUrl("/login/ott")); - String token = TestOneTimeTokenGenerationSuccessHandler.lastToken.getTokenValue(); + String token = getLastToken().getTokenValue(); this.mvc.perform(post("/login/ott").param("token", token).with(csrf())) .andExpectAll(status().isFound(), redirectedUrl("/"), authenticated()); @@ -84,7 +84,7 @@ public class OneTimeTokenLoginConfigurerTests { this.mvc.perform(post("/generateurl").param("username", "user").with(csrf())) .andExpectAll(status().isFound(), redirectedUrl("/redirected")); - String token = TestOneTimeTokenGenerationSuccessHandler.lastToken.getTokenValue(); + String token = getLastToken().getTokenValue(); this.mvc.perform(post("/loginprocessingurl").param("token", token).with(csrf())) .andExpectAll(status().isFound(), redirectedUrl("/authenticated"), authenticated()); @@ -96,7 +96,7 @@ public class OneTimeTokenLoginConfigurerTests { this.mvc.perform(post("/ott/generate").param("username", "user").with(csrf())) .andExpectAll(status().isFound(), redirectedUrl("/login/ott")); - String token = TestOneTimeTokenGenerationSuccessHandler.lastToken.getTokenValue(); + String token = getLastToken().getTokenValue(); this.mvc.perform(post("/login/ott").param("token", token).with(csrf())) .andExpectAll(status().isFound(), redirectedUrl("/"), authenticated()); @@ -194,25 +194,37 @@ public class OneTimeTokenLoginConfigurerTests { """); } + private OneTimeToken getLastToken() { + OneTimeToken lastToken = this.spring.getContext() + .getBean(TestOneTimeTokenGenerationSuccessHandler.class).lastToken; + return lastToken; + } + @Configuration(proxyBeanMethods = false) @EnableWebSecurity @Import(UserDetailsServiceConfig.class) static class OneTimeTokenDefaultConfig { @Bean - SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { + SecurityFilterChain securityFilterChain(HttpSecurity http, + OneTimeTokenGenerationSuccessHandler ottSuccessHandler) throws Exception { // @formatter:off http .authorizeHttpRequests((authz) -> authz .anyRequest().authenticated() ) .oneTimeTokenLogin((ott) -> ott - .tokenGenerationSuccessHandler(new TestOneTimeTokenGenerationSuccessHandler()) + .tokenGenerationSuccessHandler(ottSuccessHandler) ); // @formatter:on return http.build(); } + @Bean + TestOneTimeTokenGenerationSuccessHandler ottSuccessHandler() { + return new TestOneTimeTokenGenerationSuccessHandler(); + } + } @Configuration(proxyBeanMethods = false) @@ -221,7 +233,8 @@ public class OneTimeTokenLoginConfigurerTests { static class OneTimeTokenDifferentUrlsConfig { @Bean - SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { + SecurityFilterChain securityFilterChain(HttpSecurity http, + OneTimeTokenGenerationSuccessHandler ottSuccessHandler) throws Exception { // @formatter:off http .authorizeHttpRequests((authz) -> authz @@ -229,7 +242,7 @@ public class OneTimeTokenLoginConfigurerTests { ) .oneTimeTokenLogin((ott) -> ott .tokenGeneratingUrl("/generateurl") - .tokenGenerationSuccessHandler(new TestOneTimeTokenGenerationSuccessHandler("/redirected")) + .tokenGenerationSuccessHandler(ottSuccessHandler) .loginProcessingUrl("/loginprocessingurl") .authenticationSuccessHandler(new SimpleUrlAuthenticationSuccessHandler("/authenticated")) ); @@ -237,6 +250,11 @@ public class OneTimeTokenLoginConfigurerTests { return http.build(); } + @Bean + TestOneTimeTokenGenerationSuccessHandler ottSuccessHandler() { + return new TestOneTimeTokenGenerationSuccessHandler("/redirected"); + } + } @Configuration(proxyBeanMethods = false) @@ -245,7 +263,8 @@ public class OneTimeTokenLoginConfigurerTests { static class OneTimeTokenFormLoginConfig { @Bean - SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { + SecurityFilterChain securityFilterChain(HttpSecurity http, + OneTimeTokenGenerationSuccessHandler ottSuccessHandler) throws Exception { // @formatter:off http .authorizeHttpRequests((authz) -> authz @@ -253,12 +272,17 @@ public class OneTimeTokenLoginConfigurerTests { ) .formLogin(Customizer.withDefaults()) .oneTimeTokenLogin((ott) -> ott - .tokenGenerationSuccessHandler(new TestOneTimeTokenGenerationSuccessHandler()) + .tokenGenerationSuccessHandler(ottSuccessHandler) ); // @formatter:on return http.build(); } + @Bean + TestOneTimeTokenGenerationSuccessHandler ottSuccessHandler() { + return new TestOneTimeTokenGenerationSuccessHandler(); + } + } @Configuration(proxyBeanMethods = false) @@ -282,7 +306,7 @@ public class OneTimeTokenLoginConfigurerTests { static class TestOneTimeTokenGenerationSuccessHandler implements OneTimeTokenGenerationSuccessHandler { - private static OneTimeToken lastToken; + private OneTimeToken lastToken; private final OneTimeTokenGenerationSuccessHandler delegate; @@ -297,7 +321,7 @@ public class OneTimeTokenLoginConfigurerTests { @Override public void handle(HttpServletRequest request, HttpServletResponse response, OneTimeToken oneTimeToken) throws IOException, ServletException { - lastToken = oneTimeToken; + this.lastToken = oneTimeToken; this.delegate.handle(request, response, oneTimeToken); } diff --git a/config/src/test/kotlin/org/springframework/security/config/annotation/web/OneTimeTokenLoginDslTests.kt b/config/src/test/kotlin/org/springframework/security/config/annotation/web/OneTimeTokenLoginDslTests.kt index 07833e283f..3df274b513 100644 --- a/config/src/test/kotlin/org/springframework/security/config/annotation/web/OneTimeTokenLoginDslTests.kt +++ b/config/src/test/kotlin/org/springframework/security/config/annotation/web/OneTimeTokenLoginDslTests.kt @@ -69,7 +69,7 @@ class OneTimeTokenLoginDslTests { .redirectedUrl("/login/ott") ) - val token = TestOneTimeTokenGenerationSuccessHandler.lastToken?.tokenValue + val token = getLastToken().tokenValue this.mockMvc.perform( MockMvcRequestBuilders.post("/login/ott").param("token", token) @@ -91,7 +91,7 @@ class OneTimeTokenLoginDslTests { ) .andExpectAll(MockMvcResultMatchers.status().isFound(), MockMvcResultMatchers.redirectedUrl("/redirected")) - val token = TestOneTimeTokenGenerationSuccessHandler.lastToken?.tokenValue + val token = getLastToken().tokenValue this.mockMvc.perform( MockMvcRequestBuilders.post("/loginprocessingurl").param("token", token) @@ -104,25 +104,36 @@ class OneTimeTokenLoginDslTests { ) } + private fun getLastToken(): OneTimeToken { + val lastToken: OneTimeToken = spring.context + .getBean(TestOneTimeTokenGenerationSuccessHandler::class.java).lastToken!! + return lastToken + } + @Configuration @EnableWebSecurity @Import(UserDetailsServiceConfig::class) open class OneTimeTokenConfig { @Bean - open fun securityFilterChain(http: HttpSecurity): SecurityFilterChain { + open fun securityFilterChain(http: HttpSecurity, ottSuccessHandler: OneTimeTokenGenerationSuccessHandler): SecurityFilterChain { // @formatter:off http { authorizeHttpRequests { authorize(anyRequest, authenticated) } oneTimeTokenLogin { - oneTimeTokenGenerationSuccessHandler = TestOneTimeTokenGenerationSuccessHandler() + oneTimeTokenGenerationSuccessHandler = ottSuccessHandler } } // @formatter:on return http.build() } + + @Bean + open fun ottSuccessHandler(): TestOneTimeTokenGenerationSuccessHandler { + return TestOneTimeTokenGenerationSuccessHandler() + } } @EnableWebSecurity @@ -130,7 +141,7 @@ class OneTimeTokenLoginDslTests { @Import(UserDetailsServiceConfig::class) open class OneTimeTokenDifferentUrlsConfig { @Bean - open fun securityFilterChain(http: HttpSecurity): SecurityFilterChain { + open fun securityFilterChain(http: HttpSecurity, ottSuccessHandler: OneTimeTokenGenerationSuccessHandler): SecurityFilterChain { // @formatter:off http { authorizeHttpRequests { @@ -138,7 +149,7 @@ class OneTimeTokenLoginDslTests { } oneTimeTokenLogin { tokenGeneratingUrl = "/generateurl" - oneTimeTokenGenerationSuccessHandler = TestOneTimeTokenGenerationSuccessHandler("/redirected") + oneTimeTokenGenerationSuccessHandler = ottSuccessHandler loginProcessingUrl = "/loginprocessingurl" authenticationSuccessHandler = SimpleUrlAuthenticationSuccessHandler("/authenticated") } @@ -146,6 +157,11 @@ class OneTimeTokenLoginDslTests { // @formatter:on return http.build() } + + @Bean + open fun ottSuccessHandler(): TestOneTimeTokenGenerationSuccessHandler { + return TestOneTimeTokenGenerationSuccessHandler("/redirected") + } } @Configuration(proxyBeanMethods = false) @@ -156,9 +172,10 @@ class OneTimeTokenLoginDslTests { InMemoryUserDetailsManager(PasswordEncodedUser.user(), PasswordEncodedUser.admin()) } - private class TestOneTimeTokenGenerationSuccessHandler : + class TestOneTimeTokenGenerationSuccessHandler : OneTimeTokenGenerationSuccessHandler { private val delegate: OneTimeTokenGenerationSuccessHandler + var lastToken: OneTimeToken? = null constructor() { this.delegate = @@ -175,12 +192,8 @@ class OneTimeTokenLoginDslTests { } override fun handle(request: HttpServletRequest, response: HttpServletResponse, oneTimeToken: OneTimeToken) { - lastToken = oneTimeToken + this.lastToken = oneTimeToken delegate.handle(request, response, oneTimeToken) } - - companion object { - var lastToken: OneTimeToken? = null - } } }