Skip to content

OAuth2ResourceServerSpec should allow its ServerBearerTokenAuthenticationConverter to be configurable #6186

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

Closed
edeandrea opened this issue Nov 29, 2018 · 5 comments
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement

Comments

@edeandrea
Copy link
Contributor

edeandrea commented Nov 29, 2018

Currently in ServerHttpSecurity.OAuth2ResourceServerSpec.JwtSpec.configure(ServerHttpSecurity) the ServerBearerTokenAuthenticationConverter is hard-coded in the configure method. The non-reactive side (OAuth2ResourceServerConfigurer) allows for the user to supply a BearerTokenResolver. The reactive side should allow for the same.

I propose adding similar functionality to ServerHttpSecurity.OAuth2ResourceServerSpec for this.

I'm happy to supply a PR for this if you would like.

@jgrandja
Copy link
Contributor

@edeandrea That would be great. Please do at your convenience.

@jgrandja jgrandja added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Nov 29, 2018
@edeandrea
Copy link
Contributor Author

edeandrea commented Nov 29, 2018

I'm almost wrapped up with it & wanted to ask a question regarding an implementation detail.

I'd like to change ServerBearerTokenAuthenticationConverter from a class to an interface and introduce a DefaultServerBearerTokenAuthenticationConverter class, mirroring what is there on the blocking side (BearerTokenResolver with a DefaultBearerTokenResolver).

It's implementation would look like

@FunctionalInterface
public interface ServerBearerTokenAuthenticationConverter extends ServerAuthenticationConverter {
	/**
	 * Resolve any <a href="https://tools.ietf.org/html/rfc6750#section-1.2" target="_blank">Bearer Token</a>
	 * value from the request
	 *
	 * @param request The {@link ServerHttpRequest}
	 * @return The token found in the request or {@code null} if none found
	 * @throws org.springframework.security.oauth2.core.OAuth2AuthenticationException If the found token is invalid
	 * @since 5.2
	 */
	@Nullable
	String resolveToken(ServerHttpRequest request);

	@Override
	default Mono<Authentication> convert(ServerWebExchange exchange) {
		return Mono.justOrEmpty(resolveToken(exchange.getRequest()))
			.map(BearerTokenAuthenticationToken::new);
	}
}

That would constitute a breaking change though. My thought on that though is that currently the ServerBearerTokenAuthenticationConverter is not "settable" anywhere in the configuration - so is a breaking change acceptable in this instance?

If not my thought was to modify ServerBearerTokenAuthenticationConverter's private token method and make it protected instead of private and re-name it to resolveToken as well as mark the convert method as final. This way anyone extending ServerBearerTokenAuthenticationConverter just has to override the resolveToken method.

I like the cleaner separation of an interface though and referencing the interface in the configuration DSL. I could also create a new interface and retrofit ServerBearerTokenAuthenticationConverter into that interface. We'd just have to come up with a creative name, since ServerBearerTokenAuthenticationConverter seems to be the best choice for a name.

@edeandrea
Copy link
Contributor Author

After thinking about it a bit more I'm going to go with the non-breaking approach - create a new interface & retro-fit ServerBearerTokenAuthenticationConverter into it.

@jzheaux
Copy link
Contributor

jzheaux commented Nov 29, 2018

@edeandrea So, we do have ServerAuthenticationConverter, which I think would suffice for this case.

And since an extra interface is really only saving the user a couple of lines (going from String to Authentication), we can probably afford to wait on that.

Note that this is how the same functionality is exposed in OAuth2Login, so it's also nice to stay consistent where possible.

@edeandrea
Copy link
Contributor Author

I just submitted #6190

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

No branches or pull requests

3 participants