fix: remove Lazy annotation from Flow security beans (#18463) (#18514)

* 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 * set proxyBeanMethods to false * use try-with-resource for serialization/deserialization --------- Co-authored-by: Marco Collovati <[email protected]> Co-authored-by: Peter Czuczor <[email protected]>
vaadin · Jan 22, 2024 · aa7f314 · aa7f314
1 parent 701eb16
commit aa7f314
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 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
@@ -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
@@ -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<NavigationAccessControl> 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
@@ -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 <T> 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;
         }
     }