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.

Sign up for GitHub

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 support for access token in body parameter as per rfc 6750 Sec. 2.2 #15819

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+200 −26
Open

Add support for access token in body parameter as per rfc 6750 Sec. 2.2 #15819

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6803ac4
Add support for access token in body parameter as per rfc 6750 Sec. 2.2
jonah1und1 Sep 17, 2024
578a559
allow other body params (should've read the whole rfc 6750 doc)
jonah1und1 Sep 17, 2024
bbe31b5
Merge branch 'main' into gh-15818
jonah1und1 Oct 2, 2024
d14b2c4
make Java 8 compliant
jonah1und1 Oct 2, 2024
5e2b304
important import order
jonah1und1 Oct 2, 2024
157267a
replace left over vars for Java 8 compliance
jonah1und1 Oct 2, 2024
f478022
Merge branch 'main' into gh-15818
jonah1und1 Oct 29, 2024
0b7b3ec
Add missing javaDoc
jonah1und1 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 95 additions & 26 deletions ...2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,14 +16,20 @@

package org.springframework.security.oauth2.server.resource.web.server.authentication;

import static org.springframework.security.oauth2.server.resource.BearerTokenErrors.invalidRequest;

import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.util.function.Tuple2;
import reactor.util.function.Tuples;

import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;
import org.springframework.http.MediaType;
import org.springframework.http.server.reactive.ServerHttpRequest;
import org.springframework.security.core.Authentication;
import org.springframework.security.oauth2.core.OAuth2AuthenticationException;
Expand All @@ -47,16 +53,20 @@
*/
public class ServerBearerTokenAuthenticationConverter implements ServerAuthenticationConverter {

public static final String ACCESS_TOKEN_NAME = "access_token";
public static final String MULTIPLE_BEARER_TOKENS_ERROR_MSG = "Found multiple bearer tokens in the request";
private static final Pattern authorizationPattern = Pattern.compile("^Bearer (?<token>[a-zA-Z0-9-._~+/]+=*)$",
Pattern.CASE_INSENSITIVE);

private boolean allowUriQueryParameter = false;

private boolean allowFormEncodedBodyParameter = false;

private String bearerTokenHeaderName = HttpHeaders.AUTHORIZATION;

@Override
public Mono<Authentication> convert(ServerWebExchange exchange) {
return Mono.fromCallable(() -> token(exchange.getRequest())).map((token) -> {
return Mono.defer(() -> token(exchange)).map(token -> {
if (token.isEmpty()) {
BearerTokenError error = invalidTokenError();
throw new OAuth2AuthenticationException(error);
Expand All @@ -65,38 +75,53 @@ public Mono<Authentication> convert(ServerWebExchange exchange) {
});
}

private String token(ServerHttpRequest request) {
String authorizationHeaderToken = resolveFromAuthorizationHeader(request.getHeaders());
String parameterToken = resolveAccessTokenFromRequest(request);

if (authorizationHeaderToken != null) {
if (parameterToken != null) {
BearerTokenError error = BearerTokenErrors
.invalidRequest("Found multiple bearer tokens in the request");
throw new OAuth2AuthenticationException(error);
}
return authorizationHeaderToken;
}
if (parameterToken != null && isParameterTokenSupportedForRequest(request)) {
return parameterToken;
}
return null;
private Mono<String> token(ServerWebExchange exchange) {
final ServerHttpRequest request = exchange.getRequest();

return Flux.merge(resolveFromAuthorizationHeader(request.getHeaders()).map(s -> Tuples.of(s, TokenSource.HEADER)),
resolveAccessTokenFromRequest(request).map(s -> Tuples.of(s, TokenSource.QUERY_PARAMETER)),
resolveAccessTokenFromBody(exchange).map(s -> Tuples.of(s, TokenSource.BODY_PARAMETER)))
Comment on lines +81 to +83
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to resolve without the creation of Tuples here? It is generally preferred to avoid creation of additional objects when possible (e.g. Stream) and I think Tuple would fall in the same category here. Instead of the TokenSource, I believe Mono#switchIfEmpty could be used.

Copy link
Author

@jonah1und1 jonah1und1 Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am sorry, I fail to see how to resolve this without some kind of information where the token is from, to then check whether it is supported. Could you please give me an hint on how to use Mono#switchIfEmpty here?

Copy link
Member

Choose a reason for hiding this comment

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

My mistake. Since you're using Flux.merge(), it would actually be Flux#switchIfEmpty.

The new resolveAccessTokenFromBody() checks a boolean, method and content-type before returning a Mono. The resolveAccessTokenFromRequest() method can be refactored to do the same. In that case, Flux.merge() will return some number of tokens (0-3) that don't require checking the source again. It would end up looking something like this:

@Override
public Mono<Authentication> convert(ServerWebExchange exchange) {
	return Mono.defer(() -> token(exchange));
}

private Mono<Authentication> token(ServerWebExchange exchange) {
	ServerHttpRequest request = exchange.getRequest();
	return Flux.merge(resolveFromAuthorizationHeader(request.getHeaders()),
			resolveAccessTokenFromRequest(exchange.getRequest()),
			resolveAccessTokenFromBody(exchange))
		.switchIfEmpty(Mono.error(new OAuth2AuthenticationException(invalidTokenError())))
		.collectList()
		.flatMap((accessTokens) -> {
			if (accessTokens.size() > 1) {
				BearerTokenError error = BearerTokenErrors
					.invalidRequest("Found multiple bearer tokens in the request");
				return Mono.error(new OAuth2AuthenticationException(error));
			}

			return Mono.just(new BearerTokenAuthenticationToken(accessTokens.get(0)));
		});
}

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your answer!
Unfortunately, I think there might be two problems with this approach:

Firstly, if no token can be found the current implementation of ServerBearerTokenAuthenticationConverter#convert() returns an empty mono. An invalidTokenError() is only raised if the String containing the token is empty, but not if it's null.
This could be mitigated with relative ease imo, by leaving convert() in its current state and replacing switchIfEmpty() by a second if-clause inside of .flatMap():

 if (accessTokens.isEmpty()) {
    return Mono.empty();
}

Secondly, the tests resolveWhenValidHeaderIsPresentTogetherWithQueryParameterThenAuthenticationExceptionIsThrown() and resolveWhenQueryParameterHasMultipleAccessTokensThenOAuth2AuthenticationException() fail because they do not set allowUriQueryParameter to true first. While I am not quite sure about the expected behaviour here and whether adding this.converter.setAllowUriQueryParameter(true) to both tests is acceptable, doing so might change the current behaviour.
resolveWhenValidQueryParameterIsPresentTogetherWithBodyParameterThenAuthenticationExceptionIsThrown(), which was added by me, also fails because the query parameter access token is filtered due to the POST-request.

Copy link
Member

@sjohnr sjohnr Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Firstly, if no token can be found the current implementation of ServerBearerTokenAuthenticationConverter#convert() returns an empty mono. An invalidTokenError() is only raised if the String containing the token is empty, but not if it's null.

Hmm, fair point.

Secondly, the tests [...] do not set allowUriQueryParameter to true first.

Also a fair point. This is unfortunate, because it could be argued that leaving this false implies that such parameters should be ignored. The test and original implementation don't agree though and that makes this tricky. I think this comes down to whether it would be considered a bug to read the token from the request (e.g. query string or body parameters) and validate it when it is not explicitly enabled.

The spec states that

Resource servers MAY support this method.

for both body parameters and query string parameters. I don't see anything in the Error Codes section indicating that the request MUST be validated for these problems if the server doesn't support a particular method for resolving the token.

For those reasons, I'm inclined to open an bug to necessitate a change in behavior for both servlet and reactive implementations that would be a precursor to this enhancement. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the late reply!
Sounds sound to open a dedicated bug ticket for that. Do you want me to implement a PR for that?

Copy link
Member

Choose a reason for hiding this comment

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

@jonah1und1 I opened gh-16038. I think it would be nice to get this fixed in the release this month. Feel free to open a PR if you have availability to work on it, or if you're busy let me know and I can submit a fix.

Copy link
Author

Choose a reason for hiding this comment

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

I created a PR: #16039

.collectList()
.mapNotNull(tokenTuples -> {
switch (tokenTuples.size()) {
case 0:
return null;
case 1:
return getTokenIfSupported(tokenTuples.get(0), request);
default:
BearerTokenError error = invalidRequest(MULTIPLE_BEARER_TOKENS_ERROR_MSG);
throw new OAuth2AuthenticationException(error);
}
});
}

private static String resolveAccessTokenFromRequest(ServerHttpRequest request) {
List<String> parameterTokens = request.getQueryParams().get("access_token");
private static Mono<String> resolveAccessTokenFromRequest(ServerHttpRequest request) {
List<String> parameterTokens = request.getQueryParams().get(ACCESS_TOKEN_NAME);
if (CollectionUtils.isEmpty(parameterTokens)) {
return null;
return Mono.empty();
}
if (parameterTokens.size() == 1) {
return parameterTokens.get(0);
return Mono.just(parameterTokens.get(0));
}

BearerTokenError error = BearerTokenErrors.invalidRequest("Found multiple bearer tokens in the request");
BearerTokenError error = invalidRequest(MULTIPLE_BEARER_TOKENS_ERROR_MSG);
throw new OAuth2AuthenticationException(error);

}

private String getTokenIfSupported(Tuple2<String, TokenSource> tokenTuple, ServerHttpRequest request) {
switch (tokenTuple.getT2()) {
case HEADER:
return tokenTuple.getT1();
case QUERY_PARAMETER:
return isParameterTokenSupportedForRequest(request) ? tokenTuple.getT1() : null;
case BODY_PARAMETER:
return isBodyParameterTokenSupportedForRequest(request) ? tokenTuple.getT1() : null;
default:
throw new IllegalArgumentException();
}
}

/**
* Set if transport of access token using URI query parameter is supported. Defaults
* to {@code false}.
Expand All @@ -122,25 +147,69 @@ public void setBearerTokenHeaderName(String bearerTokenHeaderName) {
this.bearerTokenHeaderName = bearerTokenHeaderName;
}

private String resolveFromAuthorizationHeader(HttpHeaders headers) {
/**
* Set if transport of access token using form-encoded body parameter is supported.
* Defaults to {@code false}.
* @param allowFormEncodedBodyParameter if the form-encoded body parameter is
* supported
jonah1und1 marked this conversation as resolved.
Show resolved Hide resolved
*/
public void setAllowFormEncodedBodyParameter(boolean allowFormEncodedBodyParameter) {
this.allowFormEncodedBodyParameter = allowFormEncodedBodyParameter;
}

private Mono<String> resolveFromAuthorizationHeader(HttpHeaders headers) {
String authorization = headers.getFirst(this.bearerTokenHeaderName);
if (!StringUtils.startsWithIgnoreCase(authorization, "bearer")) {
return null;
return Mono.empty();
}
Matcher matcher = authorizationPattern.matcher(authorization);
if (!matcher.matches()) {
BearerTokenError error = invalidTokenError();
throw new OAuth2AuthenticationException(error);
}
return matcher.group("token");
return Mono.just(matcher.group("token"));
}

private static BearerTokenError invalidTokenError() {
return BearerTokenErrors.invalidToken("Bearer token is malformed");
}

private Mono<String> resolveAccessTokenFromBody(ServerWebExchange exchange) {
if (!allowFormEncodedBodyParameter) {
return Mono.empty();
}

final ServerHttpRequest request = exchange.getRequest();

if (request.getMethod() == HttpMethod.POST &&
MediaType.APPLICATION_FORM_URLENCODED.equalsTypeAndSubtype(request.getHeaders().getContentType())) {

return exchange.getFormData().mapNotNull(formData -> {
if (formData.isEmpty()) {
return null;
}
final List<String> tokens = formData.get(ACCESS_TOKEN_NAME);
if (tokens == null) {
return null;
}
if (tokens.size() > 1) {
BearerTokenError error = invalidRequest(MULTIPLE_BEARER_TOKENS_ERROR_MSG);
throw new OAuth2AuthenticationException(error);
}
return formData.getFirst(ACCESS_TOKEN_NAME);
});
}
return Mono.empty();
}

private boolean isBodyParameterTokenSupportedForRequest(ServerHttpRequest request) {
return this.allowFormEncodedBodyParameter && HttpMethod.POST == request.getMethod();
}

private boolean isParameterTokenSupportedForRequest(ServerHttpRequest request) {
return this.allowUriQueryParameter && HttpMethod.GET.equals(request.getMethod());
}

private enum TokenSource {HEADER, QUERY_PARAMETER, BODY_PARAMETER}

}
104 changes: 104 additions & 0 deletions ...ver/resource/web/server/authentication/ServerBearerTokenAuthenticationConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.springframework.http.HttpHeaders.AUTHORIZATION;
import static org.springframework.http.MediaType.APPLICATION_FORM_URLENCODED;
import static org.springframework.mock.http.server.reactive.MockServerHttpRequest.post;

/**
* @author Rob Winch
Expand Down Expand Up @@ -217,6 +220,107 @@ void resolveWhenQueryParameterHasMultipleAccessTokensThenOAuth2AuthenticationExc

}

@Test
void resolveWhenBodyParameterIsPresentThenTokenIsResolved() {
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest request = post("/").contentType(APPLICATION_FORM_URLENCODED)
.body("access_token=" + TEST_TOKEN);

assertThat(convertToToken(request).getToken()).isEqualTo(TEST_TOKEN);
}


@Test
void resolveWhenBodyParameterIsPresentButNotAllowedThenTokenIsNotResolved() {
this.converter.setAllowFormEncodedBodyParameter(false);
MockServerHttpRequest request = post("/").contentType(APPLICATION_FORM_URLENCODED)
.body("access_token=" + TEST_TOKEN);

assertThat(convertToToken(request)).isNull();
}

@Test
void resolveWhenBodyParameterHasMultipleAccessTokensThenOAuth2AuthenticationException() {
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest request = post("/").contentType(APPLICATION_FORM_URLENCODED)
.body("access_token=" + TEST_TOKEN + "&access_token=" + TEST_TOKEN);

assertThatExceptionOfType(OAuth2AuthenticationException.class)
.isThrownBy(() -> convertToToken(request))
.satisfies(ex -> {
BearerTokenError error = (BearerTokenError) ex.getError();
assertThat(error.getDescription()).isEqualTo("Found multiple bearer tokens in the request");
assertThat(error.getErrorCode()).isEqualTo(BearerTokenErrorCodes.INVALID_REQUEST);
assertThat(error.getUri()).isEqualTo("https://tools.ietf.org/html/rfc6750#section-3.1");
assertThat(error.getHttpStatus()).isEqualTo(HttpStatus.BAD_REQUEST);
});
}

@Test
void resolveBodyContainsOtherParameterAsWellThenTokenIsResolved() {
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest request = post("/").contentType(APPLICATION_FORM_URLENCODED)
.body("access_token=" + TEST_TOKEN + "&other_param=value");

assertThat(convertToToken(request).getToken()).isEqualTo(TEST_TOKEN);
}

@Test
void resolveWhenNoBodyParameterThenTokenIsNotResolved() {
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest.BaseBuilder<?> request = post("/").contentType(APPLICATION_FORM_URLENCODED);

assertThat(convertToToken(request)).isNull();
}

@Test
void resolveWhenWrongBodyParameterThenTokenIsNotResolved() {
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest request = post("/").contentType(APPLICATION_FORM_URLENCODED)
.body("other_param=value");

assertThat(convertToToken(request)).isNull();
}

@Test
void resolveWhenValidHeaderIsPresentTogetherWithBodyParameterThenAuthenticationExceptionIsThrown() {
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest request = post("/").header(AUTHORIZATION, "Bearer " + TEST_TOKEN)
.contentType(APPLICATION_FORM_URLENCODED)
.body("access_token=" + TEST_TOKEN);

assertThatExceptionOfType(OAuth2AuthenticationException.class)
.isThrownBy(() -> convertToToken(request))
.withMessageContaining("Found multiple bearer tokens in the request");
}

@Test
void resolveWhenValidQueryParameterIsPresentTogetherWithBodyParameterThenAuthenticationExceptionIsThrown() {
this.converter.setAllowUriQueryParameter(true);
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest request = post("/").queryParam("access_token", TEST_TOKEN)
.contentType(APPLICATION_FORM_URLENCODED)
.body("access_token=" + TEST_TOKEN);

assertThatExceptionOfType(OAuth2AuthenticationException.class)
.isThrownBy(() -> convertToToken(request))
.withMessageContaining("Found multiple bearer tokens in the request");
}

@Test
void resolveWhenValidQueryParameterIsPresentTogetherWithBodyParameterAndValidHeaderThenAuthenticationExceptionIsThrown() {
this.converter.setAllowUriQueryParameter(true);
this.converter.setAllowFormEncodedBodyParameter(true);
MockServerHttpRequest request = post("/").header(AUTHORIZATION, "Bearer " + TEST_TOKEN)
.queryParam("access_token", TEST_TOKEN)
.contentType(APPLICATION_FORM_URLENCODED)
.body("access_token=" + TEST_TOKEN);

assertThatExceptionOfType(OAuth2AuthenticationException.class)
.isThrownBy(() -> convertToToken(request))
.withMessageContaining("Found multiple bearer tokens in the request");
}

private BearerTokenAuthenticationToken convertToToken(MockServerHttpRequest.BaseBuilder<?> request) {
return convertToToken(request.build());
}
Expand Down
Loading