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.

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

ReactiveSecurityAutoConfiguration is missing conditions #14263

Closed
snicoll opened this issue Aug 31, 2018 · 5 comments
Closed

ReactiveSecurityAutoConfiguration is missing conditions #14263

snicoll opened this issue Aug 31, 2018 · 5 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Aug 31, 2018

It only does an import of another class that has some conditions on them. That looks weird to me. It should at the very least check Spring Security and Project reactor

@snicoll snicoll added the type: bug A general bug label Aug 31, 2018
@snicoll snicoll added this to the 2.0.x milestone Aug 31, 2018
@snicoll
Copy link
Member Author

snicoll commented Aug 31, 2018

I don't understand why ReactiveUserDetailsServiceAutoConfiguration is a separate auto-config. It looks like both this class and WebFluxSecurityConfiguration could be imported by what is current ReactiveSecurityAutoConfiguration and their basic conditions should move there.

Also a condition on reactor should be added as it less costly than checking the web application type.

@mbhave
Copy link
Contributor

mbhave commented Aug 31, 2018

@snicoll ReactiveUserDetailsServiceAutoConfiguration was pulled into a separate auto-config for this purpose.

I think we can the conditionals and configuration from WebFluxSecurityConfiguration to ReactiveSecurityAutoConfiguration instead of the single import.

I can't remember if there was a reason why ReactiveUserDetailsServiceAutoConfiguration is @ConditionalOnWebApplication because from what @wilkinsona pointed out here, it doesn't need to be.

@ayudovin
Copy link
Contributor

ayudovin commented Sep 7, 2018

Does it mean that
@Configuration
@EnableWebFluxSecurity
static class EnableWebFluxSecurityConfiguration {}
also should be located in ReactiveSecurityAutoConfiguration ?

@snicoll
Copy link
Member Author

snicoll commented Sep 7, 2018

@ayudovin You are correct. Madhura and I just discussed this one and I think we have this covered. I am assigning Madhura so that work is not duplicated.

@ayudovin
Copy link
Contributor

ayudovin commented Sep 7, 2018

@snicoll, ok

@mbhave mbhave modified the milestones: 2.0.x, 2.0.5 Sep 7, 2018
@mbhave mbhave closed this as completed in e413942 Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants