Skip to content

Expose bean setters in @Configuration used by @EnableWebFluxSecurity #6761

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.

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

Merged

Conversation

jgrandja
Copy link
Contributor

@jgrandja jgrandja commented Apr 9, 2019

Fixes gh-6624

}

@Autowired
void context(ApplicationContext context) {
Copy link

Choose a reason for hiding this comment

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

After having a look to the implementation, I am wondering if we could not avoid this ApplicationContext and just leverage regular bean autowiring which would be conceptually cleaner IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I reverted that change. I applied the same for ServerHttpSecurityConfiguration.beanFactory as well so reverted that as well.

FYI, I did a force-push to start fresh.

@jgrandja jgrandja force-pushed the gh-6624-expose-bean-setters branch from 85abc75 to 49ce135 Compare April 10, 2019 10:59
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.

I'm curious why the functional stuff is leveraging a class named @Configuration that contains @Autowired on it as well. This doesn't seem intuitive to me since the functional stuff is suppose to be free of @Configuration. Even if the annotations aren't processed, it isn't intuitive to be leveraging a class named ServerHttpSecurityConfiguration. I wonder if we should have the functional stuff use ServerHttpSecurity directly. If it needs the logic in ServerHttpSecurityConfiguration, perhaps we should extract out another class that can be used by both @Configuration and the functional code?

I think we should name the methods setXYZ since the return value is null. Alternatively, I'd be open to returning this and keeping the methods named as is.

@jgrandja
Copy link
Contributor Author

jgrandja commented Apr 10, 2019

I also feel that it can be confusing to a user looking at this code and wondering why is a @Configuration class being used by a functional API in it's implementation.

I agree with @rwinch, it might be the right direction to take as far as

If it needs the logic in ServerHttpSecurityConfiguration, perhaps we should extract out another class that can be used by both @Configuration and the functional code?

@sdeleuze
Copy link

@rwinch In Spring Fu case, the annotations are indeed not used, but I would like to leverage these classes to create the same infra via function bean registration like I do for Spring Boot, Spring MVC or WebFlux configuration. If the principle is not clear, please have a look to https://github.com/spring-projects/spring-fu/tree/master/autoconfigure-adapter. These initializers are leveraged by Kofu and Jafu DSLs. From my understanding, these configuration classes are used in a functional way and are needed to make it possible to have a ServerHttpSecurity instance. If my understanding is wrong, please let me know.

@jgrandja jgrandja force-pushed the gh-6624-expose-bean-setters branch from 49ce135 to edd9639 Compare April 23, 2019 14:07
@jgrandja
Copy link
Contributor Author

@sdeleuze I spoke with @rwinch and we came up with the following approach:

ServerHttpSecurityConfiguration configuration = new Configurer()
		.adapterRegistry(adapterRegistry)
		.authenticationManager(authenticationManager)
		.reactiveUserDetailsService(reactiveUserDetailsService)
		.passwordEncoder(passwordEncoder)
		.userDetailsPasswordService(userDetailsPasswordService)
		.configuration();

Does this work for you?

@jgrandja jgrandja force-pushed the gh-6624-expose-bean-setters branch from edd9639 to 5aacd0c Compare April 23, 2019 15:47
@jgrandja
Copy link
Contributor Author

@sdeleuze As per our discussion, you don't require a fluent configuration API so I've provided you standard setters with package-private access and moved @Autowired from field to these new setters.

@sdeleuze
Copy link

That's exactly what I need for programmatic use, and I like how it improves configuration testability, so 👍 from me.

@jgrandja jgrandja added type: enhancement A general enhancement in: config An issue in spring-security-config labels Apr 23, 2019
@jgrandja jgrandja added this to the 5.2.0.RC1 milestone Apr 23, 2019
@jgrandja jgrandja merged commit 5aacd0c into spring-projects:master Apr 23, 2019
@jgrandja jgrandja deleted the gh-6624-expose-bean-setters branch April 23, 2019 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to use Spring Security with functional bean registration
3 participants