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

Jump to bottom

Replace bean method calls with injection #6853

Merged
merged 1 commit into from
Jun 3, 2019
Merged

Replace bean method calls with injection #6853

merged 1 commit into from
Jun 3, 2019

Conversation

eleftherias
Copy link
Contributor

This is so that our configuration classes do not rely on CGLIB to proxy bean methods.

Fixes gh-6818

@eleftherias eleftherias changed the title Replace bean method calls with with injection Replace bean method calls with injection May 9, 2019
@eleftherias eleftherias force-pushed the gh-2818-proxyless-configuration branch from bf01f1c to 2010510 Compare May 9, 2019 17:19
jgrandja
jgrandja reviewed May 9, 2019
View reviewed changes
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @eleftherias ! I left a couple comments for you.

...ork/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java Outdated
@@ -103,12 +103,10 @@ public static InitializeAuthenticationProviderBeanManagerConfigurer initializeAu
return new InitializeAuthenticationProviderBeanManagerConfigurer(context);
}

public AuthenticationManager getAuthenticationManager() throws Exception {
public AuthenticationManager getAuthenticationManager(AuthenticationManagerBuilder authBuilder) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a major API change so it could only be allowed in Spring Security 6.0. Is this issue scheduled for 6.0? For more info around versioning see Semantic Versioning

Copy link
Member

Choose a reason for hiding this comment

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

I struggle with this a little bit because we want to guarantee that the AuthenticationManagerBuilder is actually a Bean. Perhaps we could change to do this:

AuthenticationManagerBuilder authBuilder = this.applicationContext.getBean(AuthenticationManagerBuilder.class);

...ework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java
*/
@Bean
public MethodInterceptor methodSecurityInterceptor() throws Exception {
public MethodInterceptor methodSecurityInterceptor(MethodSecurityMetadataSource methodSecurityMetadataSource) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same reason as above re: Semantic Versioning

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is avoidable. Given the high priority of this and the low likely hood of causing users any problems, I think this breaking change is ok.

...g/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java Outdated
@@ -64,15 +62,13 @@
* @since 3.2
*/
@Configuration
public class WebSecurityConfiguration implements ImportAware, BeanClassLoaderAware {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same reason as above re: Semantic Versioning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change isn't neccessary, I did not consider the configurations that are extending this one. Will revert it back to implementing BeanClassLoaderAware.

@eleftherias eleftherias force-pushed the gh-2818-proxyless-configuration branch from 2010510 to 253f963 Compare May 9, 2019 21:21
rwinch
rwinch requested changes May 10, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @eleftherias!

I left questions inline. I also think it would be good for us to add some new tests that ensure that we are now supporting proxyBeanMethods = false properly.

...springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java Outdated
@@ -113,6 +119,10 @@ public CurrentSecurityContextArgumentResolver reactiveCurrentSecurityContextArgu

@Bean(HTTPSECURITY_BEAN_NAME)
@Scope("prototype")
public ServerHttpSecurity serverHttpSecurity() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this change is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a test that uses the httpSecurity() method.

spring-security/config/src/test/java/org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfigurationBuilder.java

Line 31 in 047bd16

return new ServerHttpSecurityConfiguration().httpSecurity();

If we separate the bean method from the method that creates the object then we avoid calling the bean method in the test.
Unless we aren't concerned about the test using the bean method because it's creating a new instance of ServerHttpSecurityConfiguration?

Copy link
Member

Choose a reason for hiding this comment

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

When it just performs new it won't create a Bean since nothing is processing the annotation.

...ork/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java Outdated
@@ -103,12 +103,10 @@ public static InitializeAuthenticationProviderBeanManagerConfigurer initializeAu
return new InitializeAuthenticationProviderBeanManagerConfigurer(context);
}

public AuthenticationManager getAuthenticationManager() throws Exception {
public AuthenticationManager getAuthenticationManager(AuthenticationManagerBuilder authBuilder) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

I struggle with this a little bit because we want to guarantee that the AuthenticationManagerBuilder is actually a Bean. Perhaps we could change to do this:

AuthenticationManagerBuilder authBuilder = this.applicationContext.getBean(AuthenticationManagerBuilder.class);

...g/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java Outdated
* {@code <SecurityConfigurer<FilterChainProxy, WebSecurityBuilder>} instances used to
* create the web configuration
* @throws Exception
*/
@Autowired(required = false)
public void setFilterChainProxySecurityConfigurer(
ObjectPostProcessor<Object> objectPostProcessor,
@Value("#{@autowiredWebSecurityConfigurersIgnoreParents.getWebSecurityConfigurers()}") List<SecurityConfigurer<Filter, WebSecurity>> webSecurityConfigurers)
AutowiredWebSecurityConfigurersIgnoreParents autowiredWebSecurityConfigurersIgnoreParents)
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this change is necessary? A concern I have is that setting List<SecurityConfigurer<Filter, WebSecurity>> is agnostic to have we got it and only the annotation instructs how to look up the value. Another concern is that AutowiredWebSecurityConfigurersIgnoreParents is package private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is not neccessary. I have reverted it.

...g/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java Outdated
@@ -71,8 +70,6 @@

private List<SecurityConfigurer<Filter, WebSecurity>> webSecurityConfigurers;

private ClassLoader beanClassLoader;
Copy link
Member

Choose a reason for hiding this comment

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

I see that we don't need beanClassLoader, but since it is unrelated can we please put it in another PR?

...springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java Show resolved Hide resolved
@eleftherias eleftherias force-pushed the gh-2818-proxyless-configuration branch 2 times, most recently from 40cad29 to 642abd5 Compare May 14, 2019 15:57
rwinch
rwinch requested changes May 16, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @eleftherias! It appears there are some other places that need to be fixed. For example, AbstractSecurityWebSocketMessageBrokerConfigurer invokes csrfChannelInterceptor(). The way I found it was by searching for @Bean in config/src/main/java and then for each method that used @Bean, I looked for references in the main source directory. Here is a listing of the locations I found @Bean that should be checked:

org/springframework/security/config/annotation/configuration/ObjectPostProcessorConfiguration.java:	@Bean
org/springframework/security/config/annotation/configuration/ObjectPostProcessorConfiguration.java-	public ObjectPostProcessor<Object> objectPostProcessor(
--
org/springframework/security/config/annotation/method/configuration/Jsr250MetadataSourceConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/Jsr250MetadataSourceConfiguration.java-	public Jsr250MethodSecurityMetadataSource jsr250MethodSecurityMetadataSource() {
--
org/springframework/security/config/annotation/web/reactive/WebFluxSecurityConfiguration.java:	@Bean(SPRING_SECURITY_WEBFILTERCHAINFILTER_BEAN_NAME)
org/springframework/security/config/annotation/web/reactive/WebFluxSecurityConfiguration.java-	@Order(value = WEB_FILTER_CHAIN_FILTER_ORDER)
--
org/springframework/security/config/annotation/web/reactive/WebFluxSecurityConfiguration.java:	@Bean(name = AbstractView.REQUEST_DATA_VALUE_PROCESSOR_BEAN_NAME)
org/springframework/security/config/annotation/web/reactive/WebFluxSecurityConfiguration.java-	public CsrfRequestDataValueProcessor requestDataValueProcessor() {
--
org/springframework/security/config/annotation/web/reactive/WebFluxSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/reactive/WebFluxSecurityConfiguration.java-	public static BeanFactoryPostProcessor conversionServicePostProcessor() {
--
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java-	public static DelegatingApplicationListener delegatingApplicationListener() {
--
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java-	@DependsOn(AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME)
--
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java:	@Bean(name = AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME)
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java-	public Filter springSecurityFilterChain() throws Exception {
--
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java-	@DependsOn(AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME)
--
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java-	public static BeanFactoryPostProcessor conversionServicePostProcessor() {
--
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.java-	public static AutowiredWebSecurityConfigurersIgnoreParents autowiredWebSecurityConfigurersIgnoreParents(
--
org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java-	public MethodInterceptor methodSecurityInterceptor(MethodSecurityMetadataSource methodSecurityMetadataSource) {
--
org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java-	public MethodSecurityMetadataSource methodSecurityMetadataSource() {
--
org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java-	public PreInvocationAuthorizationAdvice preInvocationAuthorizationAdvice() {
--
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java:	@Bean
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java-	public AuthenticationManagerBuilder authenticationManagerBuilder(
--
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java:	@Bean
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java-	public static GlobalAuthenticationConfigurerAdapter enableGlobalAuthenticationAutowiredConfigurer(
--
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java:	@Bean
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java-	public static InitializeUserDetailsBeanManagerConfigurer initializeUserDetailsBeanManagerConfigurer(ApplicationContext context) {
--
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java:	@Bean
org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java-	public static InitializeAuthenticationProviderBeanManagerConfigurer initializeAuthenticationProviderBeanManagerConfigurer(ApplicationContext context) {
--
org/springframework/security/config/annotation/web/configuration/WebMvcSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/configuration/WebMvcSecurityConfiguration.java-	public RequestDataValueProcessor requestDataValueProcessor() {
--
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java-	public WebFluxConfigurer authenticationPrincipalArgumentResolverConfigurer(
--
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java-	public AuthenticationPrincipalArgumentResolver authenticationPrincipalArgumentResolver() {
--
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java-	public CurrentSecurityContextArgumentResolver reactiveCurrentSecurityContextArgumentResolver() {
--
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java:	@Bean(HTTPSECURITY_BEAN_NAME)
org/springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java-	@Scope("prototype")
--
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java-	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
--
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java-	public DelegatingMethodSecurityMetadataSource methodMetadataSource(MethodSecurityExpressionHandler methodSecurityExpressionHandler) {
--
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java-	public PrePostAdviceReactiveMethodInterceptor securityMethodInterceptor(AbstractMethodSecurityMetadataSource source, MethodSecurityExpressionHandler handler) {
--
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java-	public DefaultMethodSecurityExpressionHandler methodSecurityExpressionHandler() {
--
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java:	@Bean
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java-	public CsrfChannelInterceptor csrfChannelInterceptor() {
--
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java:	@Bean
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java-	public ChannelSecurityInterceptor inboundChannelSecurity() {
--
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java:	@Bean
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java-	public SecurityContextChannelInterceptor securityContextChannelInterceptor() {
--
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java:	@Bean
org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java-	public MessageSecurityMetadataSource inboundMessageSecurityMetadataSource() {
--
org/springframework/security/config/annotation/web/servlet/configuration/WebMvcSecurityConfiguration.java:	@Bean
org/springframework/security/config/annotation/web/servlet/configuration/WebMvcSecurityConfiguration.java-	public RequestDataValueProcessor requestDataValueProcessor() {

@eleftherias
Copy link
Contributor Author

@rwinch Thanks for pointing that out. I limited my search to @Configuration, didn't think about classes that could be using other annotations!

@eleftherias eleftherias force-pushed the gh-2818-proxyless-configuration branch from 642abd5 to dcbeba5 Compare May 21, 2019 17:47
eleftherias
eleftherias commented May 21, 2019
View reviewed changes
.../security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java
@@ -153,9 +153,9 @@ public CsrfChannelInterceptor csrfChannelInterceptor() {
}

@Bean
public ChannelSecurityInterceptor inboundChannelSecurity() {
public ChannelSecurityInterceptor inboundChannelSecurity(MessageSecurityMetadataSource messageSecurityMetadataSource) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another breaking change.

@eleftherias eleftherias requested a review from rwinch May 21, 2019 18:40
rwinch
rwinch requested changes May 21, 2019
View reviewed changes
...ecurity/config/annotation/authentication/configuration/AuthenticationConfigurationTests.java Outdated
@@ -172,51 +171,49 @@ public AuthenticationManager authenticationManager() {
return this.authenticationManager;
}
}
//
Copy link
Member

Choose a reason for hiding this comment

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

There seem to be some formatting changes in the code within this commit. While overall, they are a good thing I think we should move it to another PR or at minimum a separate commit. The reason is it helps us know the intent of the changes when being reviewed or when looking through commit history if we are trying to figure out a bug at some point.

@eleftherias
Replace bean method calls with injection
a50d9e3 
This is so that our configuration classes do not rely on CGLIB to proxy bean methods.

Fixes spring-projectsgh-6818
@eleftherias eleftherias force-pushed the gh-2818-proxyless-configuration branch from dcbeba5 to a50d9e3 Compare May 22, 2019 13:26
@rwinch rwinch self-assigned this Jun 3, 2019
@rwinch rwinch added in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Jun 3, 2019
@rwinch rwinch added this to the 5.2.0.M3 milestone Jun 3, 2019
@rwinch rwinch merged commit 06d3b60 into spring-projects:master Jun 3, 2019
@rwinch
Copy link
Member

rwinch commented Jun 3, 2019

Thanks @eleftherias! This is now merged into master

@eleftherias eleftherias deleted the gh-2818-proxyless-configuration branch September 28, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgrandja jgrandja jgrandja left review comments

@rwinch rwinch rwinch approved these changes

Assignees

@rwinch rwinch

Labels
in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M3
Development

Successfully merging this pull request may close these issues.

Switch to proxy-less configuration by leveraging @Configuration(proxyBeanMethods = false)
3 participants
@eleftherias @rwinch @jgrandja