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

Refactor Servlet/Server BearerExchangeFilterFunction #7353

Closed
jgrandja opened this issue Sep 4, 2019 · 3 comments · Fixed by #7355
Closed

Refactor Servlet/Server BearerExchangeFilterFunction #7353

jgrandja opened this issue Sep 4, 2019 · 3 comments · Fixed by #7355
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Milestone

Comments

@jgrandja
Copy link
Contributor

jgrandja commented Sep 4, 2019

Related #7330

I'd like to propose the following changes before this goes RC1:

ServletBearerExchangeFilterFunction

  • Move from package org.springframework.security.oauth2.server.resource.web to org.springframework.security.oauth2.server.resource.web.reactive.function.client - this aligns with packaging for ServletOAuth2AuthorizedClientExchangeFilterFunction and ExchangeFilterFunction
  • Make class final
  • Make defaultRequest() private
  • I'm wondering if we need to expose authentication(Authentication authentication) - isn't it sufficient just to look up in ThreadLocal? What is the use case where this would be set other than what's in the ThreadLocal?
  • I think if getOAuth2Token() returns null the filter will fail? It doesn't seem like we have a test for this?

ServerBearerExchangeFilterFunction

  • Apply the same (applicable) changes as ServletBearerExchangeFilterFunction
@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 4, 2019
@jgrandja jgrandja added this to the 5.2.0.RC1 milestone Sep 4, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Sep 4, 2019

This is reasonable feedback, thanks @jgrandja.

Regarding the authentication(Authentication) question, I was originally thinking that we'd use it in a background thread, but having talked with @jgrandja offline a bit, I agree now that we wouldn't use this in that way. Instead, the oauth2-client ExchangeFilterFunctions would be a better fit for that scenario.

@jzheaux
Copy link
Contributor

jzheaux commented Sep 4, 2019

Regarding whether to make defaultRequest() private or not, please see this comment, which I derived from a discussion I had with @rwinch about the same - https://github.com/spring-projects/spring-security/pull/7330/files#r320469791

Is there any flaw in that logic that you see about defaultRequest()'s value?

UPDATE: Actually, removing authentication(Authentication) I believe removes the need for defaultRequest() entirely, so the point is moot.

@robotmrv
Copy link
Contributor

robotmrv commented Sep 7, 2019

@jzheaux @jgrandja
Current implementation of ServletBearerExchangeFilterFunction was simplified quite a bit and so it could handle only the most simple cases
but it will fail if
I have my custom filter before ServletBearerExchangeFilterFunction
and I want to retry 3 times my request if it fails
because of deffered nature of the previous filter the Publisher from ServletBearerExchangeFilterFunction will be reconstructed and executed in a new thread without access to the initial Authentication

ServletBearerExchangeFilterFunction bearer = new ServletBearerExchangeFilterFunction();
WebClient webClient = webClientBuilder//it is a bean 
		.filter((request, next) -> //it could be added implicitly by WebClientCustomizer
				Mono.deferWithContext(ctx -> next.exchange(addMyData(request, ctx)))
		)
		.filter(bearer)
		.build();

// in my service
MyPojo data = webClient
		.get()
		.uri(myUri)
		.retrieve()
		.bodyToMono(MyPojo.class)
		.timeout(Duration.ofMillis(10000))
                .retry(3, TimeoutException.class::isInstance)
		.block();

So my questions:

  1. What cases does ServletBearerExchangeFilterFunction cover?
  2. should there be a restriction on the order of the ServletBearerExchangeFilterFunction (i.e. should be always first)?
  3. If it is so simple maybe it is worth to mention in javadocs what cases it covers and add links to the other filter which covers more complex cases? (as it was mentionned in Refactor Servlet/Server BearerExchangeFilterFunction #7353 (comment))

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 a pull request may close this issue.

3 participants