fix: remove Lazy annotation from Flow security beans #18463
Conversation
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
| @@ -119,7 +119,7 @@ NavigationAccessControlConfigurer navigationAccessControlConfigurerCustomizer() | |||
| */ | |||
| @Bean | |||
| public AnnotatedViewAccessChecker annotatedViewAccessChecker( | |||
| @Lazy AccessAnnotationChecker accessAnnotationChecker) { | |||
| AccessAnnotationChecker accessAnnotationChecker) { | |||
There was a problem hiding this comment.
Would it also make sense to add proxyBeanMethods=false to the @Configuration so that all beans in that class aren't proxied? This would also ensure this isn't reintroduced by accident.
There was a problem hiding this comment.
If I recall correctly, proxyBeanMethods=false prevents the configuration class to be proxied, not the exposed beans.
Anyway, it makes sense to set that flag, since we have no direct method calls in SpringSecurityAutoConfiguration
There was a problem hiding this comment.
I had the same assumption in the past :) until I've read the javadocs of the flag:
Specify whether @bean methods should get proxied in order to enforce bean lifecycle behavior, e.g. to return shared singleton bean instances even in case of direct @bean method calls in user code. This feature requires method interception, implemented through a runtime-generated CGLIB subclass which comes with limitations such as the configuration class and its methods not being allowed to declare final.
There was a problem hiding this comment.
I read it as "configuration class is proxied so that bean methods will always return the same instance when called by other methods of the class". My understanding is that when, for example, you call annotatedViewAccessChecker() from another method inside the SpringSecurityAutoConfiguration class, it creates the instance at the first call, and subsequent invocation will return that one instead of a new instance how it would happen if the configuration class is not proxied.
So, SpringSecurityAutoConfiguration methods are proxied, but not their return value.
But I may be wrong. I'll double-check it
There was a problem hiding this comment.
Good thing to double check! I only remember an old issue where boot also switched all their configuration to false by default, also to increase performance: spring-projects/spring-boot#9068
There was a problem hiding this comment.
Before this is blocked: don't worry about it and do it later :)
There was a problem hiding this comment.
I added the flag anyway, since it completely makes sense to avoid proxying in this case.
Thanks for pointing out 👍
vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java
Outdated
Show resolved
Hide resolved
| private <T> void assertObjectIsSerializable(T instance) { | ||
| Object deserialized = Assertions.assertDoesNotThrow(() -> { | ||
| ByteArrayOutputStream bs = new ByteArrayOutputStream(); | ||
| ObjectOutputStream out = new ObjectOutputStream(bs); |
There was a problem hiding this comment.
Intellij like this version better:
| ObjectOutputStream out = new ObjectOutputStream(bs); | |
| try (ObjectOutputStream out = new ObjectOutputStream(bs)) { | |
| out.writeObject(instance); | |
| } |
Maybe we could use it.
Co-authored-by: Peter Czuczor <[email protected]>
|
* 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 <[email protected]> * set proxyBeanMethods to false * use try-with-resource for serialization/deserialization --------- Co-authored-by: Peter Czuczor <[email protected]>
* 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]>
Description
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
Type of change
Checklist
Additional for
Featuretype of change