From d3d2feb0fca99eec42e39fc4d649a4466f5378aa Mon Sep 17 00:00:00 2001 From: Taeho Kim Date: Thu, 30 Dec 2021 10:59:20 +0900 Subject: [PATCH 1/3] Refactoring: remove duplicate code, using ProblemHttpConfigurer --- .../ProblemSecurityBeanPostProcessor.java | 26 +++++----- .../ProblemSecurityBeanPostProcessorTest.java | 49 ++++++------------- 2 files changed, 28 insertions(+), 47 deletions(-) diff --git a/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessor.java b/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessor.java index 8a5cb2b6..869e6e46 100644 --- a/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessor.java +++ b/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessor.java @@ -1,34 +1,32 @@ package org.zalando.problem.spring.web.autoconfigure.security; -import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanCreationException; -import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.util.ClassUtils; -import org.zalando.problem.spring.web.advice.security.SecurityProblemSupport; + +import java.util.Objects; @Slf4j -@RequiredArgsConstructor public class ProblemSecurityBeanPostProcessor implements BeanPostProcessor { - private final ObjectProvider securityProblemSupport; @Override public Object postProcessAfterInitialization(final Object bean, final String beanName) throws BeansException { - if (ClassUtils.isAssignableValue(HttpSecurity.class, bean)) { - securityProblemSupport.ifAvailable(support -> register((HttpSecurity) bean, beanName, support)); + if (!ClassUtils.isAssignableValue(HttpSecurity.class, bean)) { + return bean; } - return bean; - } - - private void register(final HttpSecurity http, final String beanName, final SecurityProblemSupport support) { + final HttpSecurity http = (HttpSecurity) bean; try { - http.exceptionHandling().authenticationEntryPoint(support).accessDeniedHandler(support); + final ProblemHttpConfigurer configurer = http.getConfigurer(ProblemHttpConfigurer.class); + if (Objects.isNull(configurer)) { + http.apply(new ProblemHttpConfigurer()); + } } catch (final Exception cause) { - throw new BeanCreationException(beanName, cause); + throw new BeanCreationException("Fail to configure HttpSecurity's exceptionHandling", cause); } - log.info("Register HttpSecurity's exceptionHandling"); + return bean; } + } diff --git a/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java b/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java index 8020cb81..095d250e 100644 --- a/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java +++ b/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java @@ -12,68 +12,51 @@ import org.zalando.problem.spring.web.advice.security.SecurityProblemSupport; import java.util.HashMap; +import java.util.Map; import java.util.function.Consumer; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; class ProblemSecurityBeanPostProcessorTest { - private ObjectProvider provider; private ProblemSecurityBeanPostProcessor processor; + private HttpSecurity http; @SuppressWarnings("unchecked") @BeforeEach void setUp() { - provider = mock(ObjectProvider.class); - processor = new ProblemSecurityBeanPostProcessor(provider); + processor = new ProblemSecurityBeanPostProcessor(); + final ObjectPostProcessor objectPostProcessor = mock(ObjectPostProcessor.class); + final AuthenticationManagerBuilder authenticationManagerBuilder = mock(AuthenticationManagerBuilder.class); + http = spy(new HttpSecurity(objectPostProcessor, authenticationManagerBuilder, new HashMap<>())); } - @SuppressWarnings("unchecked") @Test void shouldBeConfigured() throws Exception { - final ObjectPostProcessor objectPostProcessor = mock(ObjectPostProcessor.class); - final AuthenticationManagerBuilder authenticationManagerBuilder = mock(AuthenticationManagerBuilder.class); - final SecurityProblemSupport securityProblemSupport = mock(SecurityProblemSupport.class); - final HttpSecurity http = new HttpSecurity(objectPostProcessor, authenticationManagerBuilder, new HashMap<>()); + assertThat(processor.postProcessAfterInitialization(http, "httpSecurity")).isEqualTo(http); + verify(http).apply(any(ProblemHttpConfigurer.class)); + } - doAnswer(answer -> { - final Consumer consumer = (Consumer) answer.getArguments()[0]; - consumer.accept(securityProblemSupport); - return null; - }).when(provider).ifAvailable(any(Consumer.class)); + @Test + void shouldBeNotConfiguredWhenAlreadyConfigured() throws Exception { + doReturn(new ProblemHttpConfigurer()).when(http).getConfigurer(ProblemHttpConfigurer.class); assertThat(processor.postProcessAfterInitialization(http, "httpSecurity")).isEqualTo(http); - assertThat(ReflectionTestUtils.getField(http.exceptionHandling(), "authenticationEntryPoint")).isEqualTo(securityProblemSupport); - assertThat(ReflectionTestUtils.getField(http.exceptionHandling(), "accessDeniedHandler")).isEqualTo(securityProblemSupport); + verify(http, never()).apply(any(ProblemHttpConfigurer.class)); } @Test void shouldBeNotConfiguredWhenBeanIsNotHttpSecurity() { assertThat(processor.postProcessAfterInitialization("test", "notHttpSecurity")).isEqualTo("test"); } - - @SuppressWarnings("unchecked") - @Test - void shouldBeThrowBeanCreationExceptionWhenRegisterExceptionHandling() throws Exception { - final ObjectPostProcessor objectPostProcessor = mock(ObjectPostProcessor.class); - final AuthenticationManagerBuilder authenticationManagerBuilder = mock(AuthenticationManagerBuilder.class); - final SecurityProblemSupport securityProblemSupport = mock(SecurityProblemSupport.class); - final HttpSecurity http = spy(new HttpSecurity(objectPostProcessor, authenticationManagerBuilder, new HashMap<>())); - - doAnswer(answer -> { - final Consumer consumer = (Consumer) answer.getArguments()[0]; - consumer.accept(securityProblemSupport); - return null; - }).when(provider).ifAvailable(any(Consumer.class)); - doThrow(new BeanInitializationException("test")).when(http).exceptionHandling(); - - assertThatThrownBy(() -> processor.postProcessAfterInitialization(http, "httpSecurity")).isExactlyInstanceOf(BeanCreationException.class); - } } From f9798a75d0c22f01de0bfa77f165eb7f485eddaf Mon Sep 17 00:00:00 2001 From: Taeho Kim Date: Thu, 30 Dec 2021 11:25:13 +0900 Subject: [PATCH 2/3] fix create SecurityProblemSupport bean after AdviceTrait created cause, HandlerExceptionResolver created by WebMvcAutoConfiguration, but ProblemSecurityAutoConfiguration created before WebMvcAutoConfiguration. Import(SecurityProblemSupport) annotation working before created HandlerExceptionResolver. If you don't define custom AdviceTrait bean, it doesn't well working. So SecurityProblemSupport bean depends on AdviceTrait bean --- .../ProblemSecurityAutoConfiguration.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityAutoConfiguration.java b/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityAutoConfiguration.java index 351e3022..03dd9c87 100644 --- a/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityAutoConfiguration.java +++ b/problem-spring-web-autoconfigure/src/main/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityAutoConfiguration.java @@ -1,7 +1,7 @@ package org.zalando.problem.spring.web.autoconfigure.security; import org.apiguardian.api.API; -import org.springframework.beans.factory.ObjectProvider; +import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.boot.autoconfigure.AutoConfigureAfter; import org.springframework.boot.autoconfigure.AutoConfigureBefore; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; @@ -12,9 +12,9 @@ import org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.Import; import org.springframework.security.config.annotation.web.WebSecurityConfigurer; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfiguration; +import org.springframework.web.servlet.HandlerExceptionResolver; import org.zalando.problem.spring.web.advice.AdviceTrait; import org.zalando.problem.spring.web.advice.security.SecurityProblemSupport; import org.zalando.problem.spring.web.autoconfigure.ProblemAutoConfiguration; @@ -26,7 +26,6 @@ @ConditionalOnWebApplication(type = Type.SERVLET) @ConditionalOnClass(WebSecurityConfigurer.class) @ConditionalOnBean(WebSecurityConfiguration.class) -@Import(SecurityProblemSupport.class) @AutoConfigureAfter(SecurityAutoConfiguration.class) @AutoConfigureBefore(ProblemAutoConfiguration.class) public class ProblemSecurityAutoConfiguration { @@ -38,7 +37,13 @@ public AdviceTrait securityExceptionHandling() { } @Bean - public ProblemSecurityBeanPostProcessor problemSecurityBeanPostProcessor(final ObjectProvider securityProblemSupport) { - return new ProblemSecurityBeanPostProcessor(securityProblemSupport); + public SecurityProblemSupport securityProblemSupport(@Qualifier("handlerExceptionResolver") final HandlerExceptionResolver resolver, + final AdviceTrait adviceTrait) { + return new SecurityProblemSupport(resolver); + } + + @Bean + public ProblemSecurityBeanPostProcessor problemSecurityBeanPostProcessor() { + return new ProblemSecurityBeanPostProcessor(); } } From 3a759fc2cd6dbb42ac977bda822ff2a072e6d6dd Mon Sep 17 00:00:00 2001 From: Taeho Kim Date: Thu, 30 Dec 2021 12:02:16 +0900 Subject: [PATCH 3/3] Improve test coverage in ProblemSecurityBeanPostProcessor --- .../security/ProblemSecurityBeanPostProcessorTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java b/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java index 095d250e..43bfdac1 100644 --- a/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java +++ b/problem-spring-web-autoconfigure/src/test/java/org/zalando/problem/spring/web/autoconfigure/security/ProblemSecurityBeanPostProcessorTest.java @@ -55,6 +55,13 @@ void shouldBeNotConfiguredWhenAlreadyConfigured() throws Exception { verify(http, never()).apply(any(ProblemHttpConfigurer.class)); } + @Test + void shouldBeFailWhenThrowException() { + doThrow(RuntimeException.class).when(http).getConfigurer(ProblemHttpConfigurer.class); + + assertThatThrownBy(() -> processor.postProcessAfterInitialization(http, "httpSecurity")).isInstanceOf(BeanCreationException.class); + } + @Test void shouldBeNotConfiguredWhenBeanIsNotHttpSecurity() { assertThat(processor.postProcessAfterInitialization("test", "notHttpSecurity")).isEqualTo("test");