From 3d2c654d9b1aa2059a9b3ca822df8eda684b572e Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Mon, 22 Jan 2024 16:39:50 +0200 Subject: [PATCH] fix: remove Lazy annotation from Flow security beans (#18463) * fix: remove Lazy annotation from Flow security beans For parameters with Lazy annotation, Spring generates a not-serializable proxy. Since some security beans are used inside Flow listeners, they should be fully serializable (or defined transient, if possible). This change removes the unnecessary Lazy annotaions, moving the lazy evaluation to VaadinWebSecurity. Fixes #18458 * Apply suggestions from code review Co-authored-by: Peter Czuczor <61667986+czp13@users.noreply.github.com> * set proxyBeanMethods to false * use try-with-resource for serialization/deserialization --------- Co-authored-by: Peter Czuczor <61667986+czp13@users.noreply.github.com> --- .../SpringSecurityAutoConfiguration.java | 6 +-- .../spring/security/VaadinWebSecurity.java | 9 ++++ .../SpringSecurityAutoConfigurationTest.java | 52 ++++++++++++++++++- 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringSecurityAutoConfiguration.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringSecurityAutoConfiguration.java index 5c0f1f25d75..ad211e9fafb 100644 --- a/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringSecurityAutoConfiguration.java +++ b/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringSecurityAutoConfiguration.java @@ -48,7 +48,7 @@ * @author Vaadin Ltd * */ -@Configuration +@Configuration(proxyBeanMethods = false) @ConditionalOnClass(WebSecurityCustomizer.class) @EnableConfigurationProperties(VaadinConfigurationProperties.class) public class SpringSecurityAutoConfiguration { @@ -119,7 +119,7 @@ NavigationAccessControlConfigurer navigationAccessControlConfigurerCustomizer() */ @Bean public AnnotatedViewAccessChecker annotatedViewAccessChecker( - @Lazy AccessAnnotationChecker accessAnnotationChecker) { + AccessAnnotationChecker accessAnnotationChecker) { return new AnnotatedViewAccessChecker(accessAnnotationChecker); } @@ -133,7 +133,7 @@ public AnnotatedViewAccessChecker annotatedViewAccessChecker( */ @Bean public RoutePathAccessChecker routePathAccessChecker( - @Lazy AccessPathChecker accessPathChecker) { + AccessPathChecker accessPathChecker) { return new RoutePathAccessChecker(accessPathChecker); } diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java index 4e67a1d41e4..0ba98dcc868 100644 --- a/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java +++ b/vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java @@ -15,6 +15,7 @@ */ package com.vaadin.flow.spring.security; +import jakarta.annotation.PostConstruct; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; @@ -31,6 +32,7 @@ import java.util.stream.Stream; import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.context.ApplicationContext; @@ -120,8 +122,15 @@ public abstract class VaadinWebSecurity { private String servletContextPath; @Autowired + private ObjectProvider accessControlProvider; + private NavigationAccessControl accessControl; + @PostConstruct + void afterPropertiesSet() { + accessControl = accessControlProvider.getIfAvailable(); + } + private final AuthenticationContext authenticationContext = new AuthenticationContext(); /** diff --git a/vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java b/vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java index 96f1cd78048..7a270d5c235 100644 --- a/vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java +++ b/vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java @@ -16,10 +16,15 @@ package com.vaadin.flow.spring; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; import java.lang.reflect.Method; import java.security.Principal; import java.util.function.Function; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.mockito.ArgumentMatchers; import org.mockito.Mockito; @@ -36,9 +41,11 @@ import com.vaadin.flow.server.auth.AccessCheckDecision; import com.vaadin.flow.server.auth.AccessCheckResult; import com.vaadin.flow.server.auth.AccessPathChecker; +import com.vaadin.flow.server.auth.AnnotatedViewAccessChecker; import com.vaadin.flow.server.auth.NavigationAccessChecker; import com.vaadin.flow.server.auth.NavigationAccessControl; import com.vaadin.flow.server.auth.NavigationContext; +import com.vaadin.flow.server.auth.RoutePathAccessChecker; import com.vaadin.flow.spring.security.NavigationAccessControlConfigurer; import com.vaadin.flow.spring.security.SpringAccessPathChecker; import com.vaadin.flow.spring.security.SpringNavigationAccessControl; @@ -124,6 +131,29 @@ void customNavigationAccessCheckersConfigurerWithoutVaadinWebSecurityExtension() .run(SpringSecurityAutoConfigurationTest::assertThatCustomNavigationAccessCheckerIsUsed); } + @Test + void securityBeansAreSerializable() { + this.contextRunner.run((context) -> { + + // view access checker + assertThat(context).getBean(AccessAnnotationChecker.class) + .satisfies(this::assertObjectIsSerializable); + + assertThat(context).getBean(AnnotatedViewAccessChecker.class) + .satisfies(this::assertObjectIsSerializable); + + assertThat(context).getBean(AccessPathChecker.class) + .satisfies(this::assertObjectIsSerializable); + + assertThat(context).getBean(RoutePathAccessChecker.class) + .satisfies(this::assertObjectIsSerializable); + + assertThat(context).getBean(NavigationAccessControl.class) + .satisfies(this::assertObjectIsSerializable); + }); + + } + private static void assertThatCustomNavigationAccessCheckerIsUsed( AssertableWebApplicationContext context) { assertThat(context).hasSingleBean(NavigationAccessControl.class); @@ -151,11 +181,29 @@ private static void assertThatCustomNavigationAccessCheckerIsUsed( Mockito.verify(navigationContext, Mockito.never()).neutral(); } + private void assertObjectIsSerializable(T instance) { + Object deserialized = Assertions.assertDoesNotThrow(() -> { + ByteArrayOutputStream bs = new ByteArrayOutputStream(); + try (ObjectOutputStream out = new ObjectOutputStream(bs)) { + out.writeObject(instance); + } + byte[] data = bs.toByteArray(); + try (ObjectInputStream in = new ObjectInputStream( + new ByteArrayInputStream(data))) { + + @SuppressWarnings("unchecked") + T readObject = (T) in.readObject(); + return readObject; + } + }); + Assertions.assertNotNull(deserialized, "Deserialized object is null"); + } + @TestConfiguration(proxyBeanMethods = false) static class CustomAccessPathChecker extends VaadinWebSecurity { @Bean - AccessPathChecker customAccessPathChecker() { + static AccessPathChecker customAccessPathChecker() { return DISABLED_PATH_CHECKER; } } @@ -164,7 +212,7 @@ AccessPathChecker customAccessPathChecker() { static class CustomAccessAnnotationChecker extends VaadinWebSecurity { @Bean - AccessAnnotationChecker accessAnnotationChecker() { + static AccessAnnotationChecker accessAnnotationChecker() { return DISABLED_ANNOTATION_CHECKER; } }