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

Provide auto-configuration for WebFlux-based security #9925

Closed
hantsy opened this issue Aug 1, 2017 · 5 comments
Closed

Provide auto-configuration for WebFlux-based security #9925

hantsy opened this issue Aug 1, 2017 · 5 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@hantsy
Copy link

hantsy commented Aug 1, 2017

When I created a Spring Boot 2.0.0.M3 project, I found the existing security starter did not include spring-secuirty-webflux. I had to add this artifact explicitly in pom.xml.

I think it is better to create a spring-boot-starter-security-webflux starter to add spring-security-webflux into project dependencies and take over the responsibility of @EnableWebFluxSecurity .

@wilkinsona wilkinsona changed the title Please add a security-webflux starter Provide auto-configuration for WebFlux-based security Aug 1, 2017
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 1, 2017
@wilkinsona
Copy link
Member

This is already planned, although we don't appear to have a specific issue for it. We can use this one. Spring Security's WebFlux support is still a work in progress so this can't be implemented at the moment. We may be able to do this in M5, but it certainly won't be in M4.

@wilkinsona wilkinsona added priority: high type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 1, 2017
@wilkinsona wilkinsona added this to the 2.0.0.M5 milestone Aug 1, 2017
@mbhave mbhave self-assigned this Sep 19, 2017
@philwebb philwebb modified the milestones: 2.0.0.M5, 2.0.0.RC1, 2.0.0.M6 Sep 20, 2017
@philwebb
Copy link
Member

@rwinch We started to investigate how the auto-configuration for reactive security might look and hit a few things in the Spring Security code that might want to be reorganized. Specifically, classes from io.reactor are being used in spring-security-core and spring-security-config and it's not always clear if they will be available.

A few good examples are HttpSecurity, PrePostAdviceMethodInterceptor and AuthenticatedAuthorizationManager.

I'm not sure on the best strategy for keeping these types distinct. I guess you could use Reactive in their names to make it a little more obvious. Another option might be to move them all into the spring-webflux jar.

In Boot we faced a similar problem with the server support. We decided to organize things primarily by package. We also put checkstyle import rules in place to ensure that servlet/reactive classes weren't accidentally used in the wrong place.

Perhaps the Framework team might also have some advice about how they structured things.

@rwinch
Copy link
Member

rwinch commented Sep 21, 2017

Specifically, classes from io.reactor are being used in spring-security-core and spring-security-config and it's not always clear if they will be available.

Can you elaborate on the problem?

I'm not sure on the best strategy for keeping these types distinct. I guess you could use Reactive in their names to make it a little more obvious.

Another option might be to move them all into the spring-webflux jar.

This is not consistent with Spring Framework either. They have reactor treated as an optional dependency in quite a few places (spring-core, spring-web, spring-messaging, ...).

Perhaps the Framework team might also have some advice about how they structured things.

As far as I'm aware we are rather consistent with how Framework structured reactive. When speaking with them, they chose different names for the reactive counterparts, but at times create the same class name in different packages. Honestly, I've considered having a Reactive prefix, to the Reactive APIs (like Spring Data) but I'm not entirely sold on this approach either.

@philwebb
Copy link
Member

Can you elaborate on the problem?

I think I was worried that it will start to get hard to describe when certain variations should be used. For example, There is the UserDetailsRepository interface that returns a Mono<UserDetails>, and the UserDetailsService that returns UserDetails directly. The user needs to make sure they don't accidentally use the wrong one. Perhaps UserDetailsRepository and ReactiveUserDetailsRepository would be clearer?

... [Spring Framework] have reactor treated as an optional dependency in quite a few places

Yes that's true, I guess moving stuff to spring-security-webflux isn't that great.

As far as I'm aware we are rather consistent with how Framework structured reactive

They seem to have quite an interesting approach. The have a lot of .reactive packages (e.g. o.s.http.server.reactive to broadly group reactive alternatives. If you filter those out you get this:

$ grep -irl --include="*.java" "reactor" . | grep main | grep -v "spring\-webflux" | grep -v "/reactive/"

./spring-core/src/main/java/org/springframework/core/codec/AbstractDecoder.java
./spring-core/src/main/java/org/springframework/core/codec/AbstractSingleValueEncoder.java
./spring-core/src/main/java/org/springframework/core/codec/ByteArrayDecoder.java
./spring-core/src/main/java/org/springframework/core/codec/ByteArrayEncoder.java
./spring-core/src/main/java/org/springframework/core/codec/ByteBufferDecoder.java
./spring-core/src/main/java/org/springframework/core/codec/ByteBufferEncoder.java
./spring-core/src/main/java/org/springframework/core/codec/CharSequenceEncoder.java
./spring-core/src/main/java/org/springframework/core/codec/DataBufferDecoder.java
./spring-core/src/main/java/org/springframework/core/codec/DataBufferEncoder.java
./spring-core/src/main/java/org/springframework/core/codec/Decoder.java
./spring-core/src/main/java/org/springframework/core/codec/Encoder.java
./spring-core/src/main/java/org/springframework/core/codec/ResourceDecoder.java
./spring-core/src/main/java/org/springframework/core/codec/ResourceEncoder.java
./spring-core/src/main/java/org/springframework/core/codec/ResourceRegionEncoder.java
./spring-core/src/main/java/org/springframework/core/codec/StringDecoder.java
./spring-core/src/main/java/org/springframework/core/io/buffer/DataBufferUtils.java
./spring-core/src/main/java/org/springframework/core/ReactiveAdapterRegistry.java
./spring-messaging/src/main/java/org/springframework/messaging/simp/stomp/ReactorNettyTcpStompClient.java
./spring-messaging/src/main/java/org/springframework/messaging/simp/stomp/StompBrokerRelayMessageHandler.java
./spring-messaging/src/main/java/org/springframework/messaging/simp/stomp/StompReactorNettyCodec.java
./spring-messaging/src/main/java/org/springframework/messaging/tcp/reactor/AbstractMonoToListenableFutureAdapter.java
./spring-messaging/src/main/java/org/springframework/messaging/tcp/reactor/AbstractNioBufferReactorNettyCodec.java
./spring-messaging/src/main/java/org/springframework/messaging/tcp/reactor/MonoToListenableFutureAdapter.java
./spring-messaging/src/main/java/org/springframework/messaging/tcp/reactor/package-info.java
./spring-messaging/src/main/java/org/springframework/messaging/tcp/reactor/ReactorNettyCodec.java
./spring-messaging/src/main/java/org/springframework/messaging/tcp/reactor/ReactorNettyTcpClient.java
./spring-messaging/src/main/java/org/springframework/messaging/tcp/reactor/ReactorNettyTcpConnection.java
./spring-web/src/main/java/org/springframework/http/client/Netty4ClientHttpRequest.java
./spring-web/src/main/java/org/springframework/http/client/Netty4ClientHttpRequestFactory.java
./spring-web/src/main/java/org/springframework/http/client/Netty4ClientHttpResponse.java
./spring-web/src/main/java/org/springframework/http/codec/DecoderHttpMessageReader.java
./spring-web/src/main/java/org/springframework/http/codec/EncoderHttpMessageWriter.java
./spring-web/src/main/java/org/springframework/http/codec/FormHttpMessageReader.java
./spring-web/src/main/java/org/springframework/http/codec/FormHttpMessageWriter.java
./spring-web/src/main/java/org/springframework/http/codec/HttpMessageReader.java
./spring-web/src/main/java/org/springframework/http/codec/HttpMessageWriter.java
./spring-web/src/main/java/org/springframework/http/codec/json/AbstractJackson2Decoder.java
./spring-web/src/main/java/org/springframework/http/codec/json/AbstractJackson2Encoder.java
./spring-web/src/main/java/org/springframework/http/codec/json/Jackson2Tokenizer.java
./spring-web/src/main/java/org/springframework/http/codec/multipart/FilePart.java
./spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartHttpMessageReader.java
./spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriter.java
./spring-web/src/main/java/org/springframework/http/codec/multipart/Part.java
./spring-web/src/main/java/org/springframework/http/codec/multipart/SynchronossPartHttpMessageReader.java
./spring-web/src/main/java/org/springframework/http/codec/ResourceHttpMessageWriter.java
./spring-web/src/main/java/org/springframework/http/codec/ServerSentEventHttpMessageReader.java
./spring-web/src/main/java/org/springframework/http/codec/ServerSentEventHttpMessageWriter.java
./spring-web/src/main/java/org/springframework/http/codec/xml/Jaxb2XmlDecoder.java
./spring-web/src/main/java/org/springframework/http/codec/xml/Jaxb2XmlEncoder.java
./spring-web/src/main/java/org/springframework/http/codec/xml/XmlEventDecoder.java
./spring-web/src/main/java/org/springframework/http/ReactiveHttpInputMessage.java
./spring-web/src/main/java/org/springframework/http/ReactiveHttpOutputMessage.java
./spring-web/src/main/java/org/springframework/http/ZeroCopyHttpOutputMessage.java
./spring-web/src/main/java/org/springframework/web/bind/annotation/RequestMapping.java
./spring-web/src/main/java/org/springframework/web/bind/support/WebExchangeDataBinder.java
./spring-web/src/main/java/org/springframework/web/server/adapter/DefaultServerWebExchange.java
./spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java
./spring-web/src/main/java/org/springframework/web/server/DefaultServerWebExchangeBuilder.java
./spring-web/src/main/java/org/springframework/web/server/handler/DefaultWebFilterChain.java
./spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java
./spring-web/src/main/java/org/springframework/web/server/handler/FilteringWebHandler.java
./spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java
./spring-web/src/main/java/org/springframework/web/server/handler/WebHandlerDecorator.java
./spring-web/src/main/java/org/springframework/web/server/ServerWebExchange.java
./spring-web/src/main/java/org/springframework/web/server/ServerWebExchangeDecorator.java
./spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSession.java
./spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java
./spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java
./spring-web/src/main/java/org/springframework/web/server/session/WebSessionManager.java
./spring-web/src/main/java/org/springframework/web/server/session/WebSessionStore.java
./spring-web/src/main/java/org/springframework/web/server/WebExceptionHandler.java
./spring-web/src/main/java/org/springframework/web/server/WebFilter.java
./spring-web/src/main/java/org/springframework/web/server/WebFilterChain.java
./spring-web/src/main/java/org/springframework/web/server/WebHandler.java
./spring-web/src/main/java/org/springframework/web/server/WebSession.java

Some of those are still grouped at the package level (o.s.core.codec), a few have Reactive in the name and a lot seem semi internal classes (converters, adapters etc).

I really don't see a perfect solution, and it's a super hard problem to solve. Perhaps as more things get added it'll be clearer to see how to organize things. I just wanted to give you a heads-up that it might want some consideration.

@rwinch
Copy link
Member

rwinch commented Sep 22, 2017

@philwebb Thanks for the additional information

Perhaps UserDetailsRepository and ReactiveUserDetailsRepository would be clearer?

I've considered UserDetailsService and ReactiveUserDetailsService (cannot change the old UserDetailsService without breaking passivity and I'd prefer to be passive on that), but was advised by framework team to avoid prefixing with Reactive and to find a different name.

Overall, I'm not entirely convinced I like this advice but up until now I have tried to follow it. I like the idea of Reactive prefix so it is easier to find the reactive counterparts (and users familiar with non-reactive will understand the class structure quickly this way). However, having Reactive prefix also clutters things up quite a bit. A complaint I have about new names vs just using a prefix is that sometimes I just cannot find a new name for the reactive counterpart and have prefixed with Reactive.

The user needs to make sure they don't accidentally use the wrong one.

Personally, I think this is less of an issue. If you are in a reactive application, you need to have reactive responses (i.e. Mono or Flux).

Perhaps as more things get added it'll be clearer to see how to organize things. I just wanted to give you a heads-up that it might want some consideration.

Thanks for the heads up. It is very much appreciated and on my mind. I mostly wanted to be certain we weren't holding up Spring Boot's progress on reactive Spring Security integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants