-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Add Servlet and ServerBearerExchangeFilterFunction #7330
Conversation
this.delegate = delegate; | ||
Context parentContext = this.delegate.currentContext(); | ||
Context context; | ||
if (parentContext.hasKey(REQUEST_CONTEXT_DATA_HOLDER_ATTR_NAME)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be better not to create empty RequestContextDataHolder?
by
if (authentication == null || parentContext.hasKey(REQUEST_CONTEXT_DATA_HOLDER_ATTR_NAME)) {}
*/ | ||
@Override | ||
public void afterPropertiesSet() throws Exception { | ||
Hooks.onLastOperator(REQUEST_CONTEXT_OPERATOR_KEY, Operators.lift((s, sub) -> createRequestContextSubscriber(sub))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operators.liftPublisher()
will not create unnecessary transformations into Scannable
of a publisher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip! I'll update that.
return oauth2Token(request.attributes()) | ||
.map(oauth2Token -> bearer(request, oauth2Token)) | ||
.flatMap(next::exchange) | ||
.switchIfEmpty(Mono.defer(() -> next.exchange(request))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe would be better to write
return oauth2Token(request.attributes())
.map(oauth2Token -> bearer(request, oauth2Token))
.defaultIfEmpty(request)
.flatMap(next::exchange);
This should create less Publishers/functions and it does not duplicate next::exchange
*/ | ||
public Consumer<WebClient.RequestHeadersSpec<?>> defaultRequest() { | ||
return spec -> spec.attributes(attrs -> { | ||
populateDefaultAuthentication(attrs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what case does it cover?
Isn't it covered by Hooks.onLastOperator()
from afterPropertiesSet() - it does the same.
It seems to me that this is just legacy solution copied from
https://github.com/spring-projects/spring-security/blob/master/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java
Or is it just an altarnative for case when Hook is not enabled and chaining is not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an alternative for when more sophisticated chaining is wanted by the application, so oauth2Configuration
would not suffice. Order obviously matters with filters, so, for example, I might need to default the request, add my own custom filter, and then add this filter after that.
.map(req -> getOAuth2Token(req.attributes())) | ||
.map(token -> bearer(request, token)) | ||
.flatMap(next::exchange) | ||
.switchIfEmpty(Mono.defer(() -> next.exchange(request))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there are activities to reduce number of Optional
usage and replacement of filter/map/flatMap()
to the handle()
in the spring core
so it could be rewritten to
may be it is a little bit more imperative, but it creates less operators and does not use create Optional
to check attribute presence
public Mono<ClientResponse> filter(ClientRequest request, ExchangeFunction next) {
return mergeRequestAttributesIfNecessary(request)
.handle((req, sink) -> {
Map<String, Object> attributes = req.attributes();
if (attributes.get(AUTHENTICATION_ATTR_NAME) == null) {
sink.next(request);
} else {
AbstractOAuth2Token token = getOAuth2Token(attributes);
sink.next(bearer(req, token));
}
})
.flatMap(next::exchange);
}
private Mono<ClientRequest> mergeRequestAttributesIfNecessary(ClientRequest request) {
if (request.attributes().get(AUTHENTICATION_ATTR_NAME) != null) {
return Mono.just(request);
}
return mergeRequestAttributesFromContext(request);
}
private Mono<ClientRequest> mergeRequestAttributesFromContext(ClientRequest request) {
return Mono.subscriberContext()
.map(ctx -> ClientRequest.from(request)
.attributes(attrs -> populateRequestAttributes(attrs, ctx))
.build()
);
}
Fixes gh-5334
Fixes gh-7284