Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove Lazy annotation from Flow security beans #18463

Merged
merged 5 commits into from
Jan 22, 2024
Merged

fix: remove Lazy annotation from Flow security beans #18463

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0277d4b
fix: remove Lazy annotation from Flow security beans
mcollovati Jan 15, 2024
e619927
Merge branch 'main' into issues/18458_springautoconf_remove_lazy_para…
czp13 Jan 15, 2024
539b63a
Apply suggestions from code review
mcollovati Jan 16, 2024
0c4b416
set proxyBeanMethods to false
mcollovati Jan 16, 2024
124c117
use try-with-resource for serialization/deserialization
mcollovati Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringSecurityAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ NavigationAccessControlConfigurer navigationAccessControlConfigurerCustomizer()
*/
@Bean
public AnnotatedViewAccessChecker annotatedViewAccessChecker(
@Lazy AccessAnnotationChecker accessAnnotationChecker) {
AccessAnnotationChecker accessAnnotationChecker) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return new AnnotatedViewAccessChecker(accessAnnotationChecker);
}

Expand All @@ -133,7 +133,7 @@ public AnnotatedViewAccessChecker annotatedViewAccessChecker(
*/
@Bean
public RoutePathAccessChecker routePathAccessChecker(
@Lazy AccessPathChecker accessPathChecker) {
AccessPathChecker accessPathChecker) {
return new RoutePathAccessChecker(accessPathChecker);
}

Expand Down
9 changes: 9 additions & 0 deletions vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();

/**
Expand Down
51 changes: 49 additions & 2 deletions vaadin-spring/src/test/java/com/vaadin/flow/spring/SpringSecurityAutoConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,16 @@

package com.vaadin.flow.spring;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
mcollovati marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand All @@ -36,9 +42,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;
Expand Down Expand Up @@ -124,6 +132,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);
Expand Down Expand Up @@ -151,11 +182,27 @@ private static void assertThatCustomNavigationAccessCheckerIsUsed(
Mockito.verify(navigationContext, Mockito.never()).neutral();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Marco!

out.writeObject(instance);
byte[] data = bs.toByteArray();
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;
}
}
Expand All @@ -164,7 +211,7 @@ AccessPathChecker customAccessPathChecker() {
static class CustomAccessAnnotationChecker extends VaadinWebSecurity {

@Bean
AccessAnnotationChecker accessAnnotationChecker() {
static AccessAnnotationChecker accessAnnotationChecker() {
return DISABLED_ANNOTATION_CHECKER;
}
}
Expand Down
Loading