fix: remove Lazy annotation from Flow security beans #18463

mcollovati · 2024-01-15T14:19:46Z

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

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

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

github-actions · 2024-01-15T14:34:42Z

Test Results

1 055 files ± 0 1 055 suites ±0 1h 13m 58s ⏱️ +19s
6 799 tests + 1 6 754 ✅ + 1 45 💤 ±0 0 ❌ ±0
7 086 runs - 14 7 030 ✅ - 14 56 💤 ±0 0 ❌ ±0

Results for commit 124c117. ± Comparison against base commit 995caf0.

♻️ This comment has been updated with latest results.

knoobie · 2024-01-15T18:22:48Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringSecurityAutoConfiguration.java

@@ -119,7 +119,7 @@ NavigationAccessControlConfigurer navigationAccessControlConfigurerCustomizer()
     */
    @Bean
    public AnnotatedViewAccessChecker annotatedViewAccessChecker(
-            @Lazy AccessAnnotationChecker accessAnnotationChecker) {
+            AccessAnnotationChecker accessAnnotationChecker) {


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.

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

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.

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

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

Before this is blocked: don't worry about it and do it later :)

I added the flag anyway, since it completely makes sense to avoid proxying in this case.
Thanks for pointing out 👍

…meters

vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java

czp13 · 2024-01-15T22:12:02Z

vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java

+    private <T> void assertObjectIsSerializable(T instance) {
+        Object deserialized = Assertions.assertDoesNotThrow(() -> {
+            ByteArrayOutputStream bs = new ByteArrayOutputStream();
+            ObjectOutputStream out = new ObjectOutputStream(bs);


Intellij like this version better:

Suggested change

ObjectOutputStream out = new ObjectOutputStream(bs);

try (ObjectOutputStream out = new ObjectOutputStream(bs)) {

out.writeObject(instance);

}

Maybe we could use it.

Thanks, Marco!

Co-authored-by: Peter Czuczor <61667986+czp13@users.noreply.github.com>

czp13

Thank you for the changes! 🙇 I believe most issues have been addressed, with only a minor code modification suggested. It's a very small change, so I am marking this as approved.

sonarqubecloud · 2024-01-16T07:09:38Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

* 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>

* 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 <marco@vaadin.com> Co-authored-by: Peter Czuczor <61667986+czp13@users.noreply.github.com>

mcollovati added the target/24.3 label Jan 15, 2024

vaadin-bot added the +0.0.1 label Jan 15, 2024

knoobie reviewed Jan 15, 2024

View reviewed changes

Merge branch 'main' into issues/18458_springautoconf_remove_lazy_para…

e619927

…meters

czp13 reviewed Jan 15, 2024

View reviewed changes

vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java Outdated Show resolved Hide resolved

czp13 reviewed Jan 15, 2024

View reviewed changes

mcollovati and others added 2 commits January 16, 2024 08:33

Apply suggestions from code review

539b63a

Co-authored-by: Peter Czuczor <61667986+czp13@users.noreply.github.com>

set proxyBeanMethods to false

0c4b416

czp13 previously approved these changes Jan 16, 2024

View reviewed changes

use try-with-resource for serialization/deserialization

124c117

mcollovati dismissed czp13’s stale review via 124c117 January 16, 2024 06:59

czp13 approved these changes Jan 16, 2024

View reviewed changes

mshabarov requested a review from tltv January 22, 2024 12:41

tltv approved these changes Jan 22, 2024

View reviewed changes

tltv merged commit 3d2c654 into main Jan 22, 2024
26 checks passed

tltv deleted the issues/18458_springautoconf_remove_lazy_parameters branch January 22, 2024 14:39

vaadin-bot added the cherry-picked-24.3 label Jan 22, 2024

mcollovati mentioned this pull request Jan 29, 2024

Circular dependencies with VaadinWebSecurity and NavigationAccessControl #18561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove Lazy annotation from Flow security beans #18463

fix: remove Lazy annotation from Flow security beans #18463

mcollovati commented Jan 15, 2024

github-actions bot commented Jan 15, 2024 •

edited

Loading

knoobie Jan 15, 2024

mcollovati Jan 16, 2024

knoobie Jan 16, 2024

mcollovati Jan 16, 2024

knoobie Jan 16, 2024

knoobie Jan 16, 2024

mcollovati Jan 16, 2024

czp13 Jan 15, 2024

mcollovati Jan 16, 2024

czp13 Jan 16, 2024

czp13 left a comment

sonarqubecloud bot commented Jan 16, 2024

-            ObjectOutputStream out = new ObjectOutputStream(bs);
+            try (ObjectOutputStream out = new ObjectOutputStream(bs)) {
+                out.writeObject(instance);
+            }

fix: remove Lazy annotation from Flow security beans #18463

fix: remove Lazy annotation from Flow security beans #18463

Conversation

mcollovati commented Jan 15, 2024

Description

Type of change

Checklist

Additional for Feature type of change

github-actions bot commented Jan 15, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czp13 left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 16, 2024

Quality Gate passed

Additional for `Feature` type of change

github-actions bot commented Jan 15, 2024 •

edited

Loading