Skip to content

Conversation

jgrandja
Copy link
Contributor

@jgrandja jgrandja commented Sep 25, 2019

Fixes gh-7422

The intention of the new SecurityReactorContextConfiguration is that it's meant to be used/shared by both ServletOAuth2AuthorizedClientExchangeFilterFunction and ServletBearerExchangeFilterFunction. However, the changes in this PR work only for ServletOAuth2AuthorizedClientExchangeFilterFunction.

When this PR is merged than I believe we can remove OAuth2ResourceServerConfiguration and apply a very small change in ServletBearerExchangeFilterFunction, which will than resolve #7423.

@jgrandja jgrandja requested review from jzheaux and rwinch September 25, 2019 17:03
@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement labels Sep 25, 2019
@jgrandja jgrandja added this to the 5.2.0 milestone Sep 25, 2019
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

LGTM

Note: I pulled your changes locally to try them out with Resource Server, performing the needed changes, and it continued to work as expected.


@Override
public void afterPropertiesSet() throws Exception {
Hooks.onLastOperator(SECURITY_REACTOR_CONTEXT_OPERATOR_KEY,
Copy link
Contributor

@robotmrv robotmrv Sep 26, 2019

Choose a reason for hiding this comment

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

@jgrandja @rwinch
as far as function from Hooks.onLastOperator is invoked for each last operator in a chain it means that such simple code will print 201:

public static void main(String[] args) {
    final AtomicInteger counter = new AtomicInteger();
    Hooks.onLastOperator(pub -> {
        counter.incrementAndGet();
        return pub;
    });
    Flux.range(0, 200)
            .flatMap(it -> Mono.defer(() -> Mono.just(it)))//just do some work
            .blockLast();
    System.out.println("counter: " + counter);
}

it means that 200 publishers and probably subscribers and attributes maps will be created just for nothing.
To reduce allocation this code could be rewritten to

public void afterPropertiesSet() {
	Function<? super Publisher<Object>, ? extends Publisher<Object>> lifter =
			Operators.liftPublisher((s, sub) -> createRequestContextSubscriberIfNecessary(sub));
	Hooks.onLastOperator(SECURITY_REACTOR_CONTEXT_OPERATOR_KEY, pub -> {
		HttpServletRequest request = null;
		HttpServletResponse response = null;
		ServletRequestAttributes requestAttributes =
				(ServletRequestAttributes) RequestContextHolder.getRequestAttributes();
		if (requestAttributes != null) {
			request = requestAttributes.getRequest();
			response = requestAttributes.getResponse();
		}
		Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
		if (authentication == null && request == null && response == null) {
			// No need to create lift Publisher so return original
			return pub;
		}
		return lifter.apply(pub);
	});
}
...
<T> CoreSubscriber<T> createRequestContextSubscriberIfNecessary(CoreSubscriber<T> delegate) {
	if (delegate.currentContext().hasKey(SECURITY_CONTEXT_ATTRIBUTES)) {
        //already enriched. no nead to create Subscriber
		return delegate;
	}
... // the rest logic is the same 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robotmrv Your tips on the Reactive end of things has been very valuable. Thank you for keeping an eye out on improvements. I applied your suggested changes :) Thanks again!

@jgrandja jgrandja self-assigned this Sep 26, 2019
@jgrandja
Copy link
Contributor Author

Merged via 2a5bd6e

@jgrandja jgrandja closed this Sep 26, 2019
@jgrandja jgrandja deleted the gh-7422-coresubscriber branch September 26, 2019 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine subscription strategy for Servlet ExchangeFilterFunctions Servlet ExchangeFilterFunctions should align
4 participants