Skip to content

Fix token endpoint filter for authorization code flow #120 #121

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

Closed
wants to merge 3 commits into from
Closed

Fix token endpoint filter for authorization code flow #120 #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed637f2
fix access token request bug
vincent-hsin Sep 24, 2020
c87bc1e
add oauth2 token endpoint filter uint-test request with code and clie…
vincent-hsin Sep 25, 2020
552ec5b
fix #120
vincent-hsin Sep 25, 2020
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
19 changes: 19 additions & 0 deletions ...auth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationToken.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,25 @@ public OAuth2AuthorizationCodeAuthenticationToken(String code,
this.redirectUri = redirectUri;
}

/**
* Constructs an {@code OAuth2AuthorizationCodeAuthenticationToken} using the provided parameters.
*
* @param code the authorization code
* @param clientId the client identifier
* @param clientPrincipal the authenticated client principal
* @param redirectUri the redirect uri
*/
public OAuth2AuthorizationCodeAuthenticationToken(String code,
String clientId, Authentication clientPrincipal, @Nullable String redirectUri) {
super(Collections.emptyList());
Assert.hasText(code, "code cannot be empty");
Assert.notNull(clientPrincipal, "clientPrincipal cannot be null");
this.code = code;
this.clientId = clientId;
this.clientPrincipal = clientPrincipal;
this.redirectUri = redirectUri;
}

@Override
public Object getPrincipal() {
return this.clientPrincipal != null ? this.clientPrincipal : this.clientId;
Expand Down
10 changes: 3 additions & 7 deletions ...g/springframework/security/oauth2/server/authorization/web/OAuth2TokenEndpointFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,16 +197,14 @@ public Authentication convert(HttpServletRequest request) {

MultiValueMap<String, String> parameters = OAuth2EndpointUtils.getParameters(request);

// client_id (REQUIRED)
// client_id (REQUIRED, if the client is not authenticating with the authorization server)
String clientId = parameters.getFirst(OAuth2ParameterNames.CLIENT_ID);
Authentication clientPrincipal = null;
if (StringUtils.hasText(clientId)) {
if (parameters.get(OAuth2ParameterNames.CLIENT_ID).size() != 1) {
throwError(OAuth2ErrorCodes.INVALID_REQUEST, OAuth2ParameterNames.CLIENT_ID);
}
} else {
clientPrincipal = SecurityContextHolder.getContext().getAuthentication();
}
Authentication clientPrincipal = SecurityContextHolder.getContext().getAuthentication();

// code (REQUIRED)
String code = parameters.getFirst(OAuth2ParameterNames.CODE);
Expand All @@ -223,9 +221,7 @@ public Authentication convert(HttpServletRequest request) {
throwError(OAuth2ErrorCodes.INVALID_REQUEST, OAuth2ParameterNames.REDIRECT_URI);
}

return clientPrincipal != null ?
new OAuth2AuthorizationCodeAuthenticationToken(code, clientPrincipal, redirectUri) :
new OAuth2AuthorizationCodeAuthenticationToken(code, clientId, redirectUri);
return new OAuth2AuthorizationCodeAuthenticationToken(code, clientId, clientPrincipal, redirectUri);
}
}

Expand Down
66 changes: 66 additions & 0 deletions ...ingframework/security/oauth2/server/authorization/web/OAuth2TokenEndpointFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,57 @@ public void doFilterWhenAuthorizationCodeTokenRequestValidThenAccessTokenRespons
assertThat(accessTokenResult.getScopes()).isEqualTo(accessToken.getScopes());
}

@Test
Copy link
Author

Choose a reason for hiding this comment

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

@jgrandja this unit test for #120

public void doFilterWhenAuthorizationCodeAndClientIdTokenRequestValidThenAccessTokenResponse() throws Exception {
RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build();
Authentication clientPrincipal = new OAuth2ClientAuthenticationToken(registeredClient);
OAuth2AccessToken accessToken = new OAuth2AccessToken(
OAuth2AccessToken.TokenType.BEARER, "token",
Instant.now(), Instant.now().plus(Duration.ofHours(1)),
new HashSet<>(Arrays.asList("scope1", "scope2")));
OAuth2AccessTokenAuthenticationToken accessTokenAuthentication =
new OAuth2AccessTokenAuthenticationToken(
registeredClient, clientPrincipal, accessToken);

when(this.authenticationManager.authenticate(any())).thenReturn(accessTokenAuthentication);

SecurityContext securityContext = SecurityContextHolder.createEmptyContext();
securityContext.setAuthentication(clientPrincipal);
SecurityContextHolder.setContext(securityContext);

MockHttpServletRequest request = createAuthorizationCodeAndClientIdTokenRequest(registeredClient);
MockHttpServletResponse response = new MockHttpServletResponse();
FilterChain filterChain = mock(FilterChain.class);

this.filter.doFilter(request, response, filterChain);

verifyNoInteractions(filterChain);

ArgumentCaptor<OAuth2AuthorizationCodeAuthenticationToken> authorizationCodeAuthenticationCaptor =
ArgumentCaptor.forClass(OAuth2AuthorizationCodeAuthenticationToken.class);
verify(this.authenticationManager).authenticate(authorizationCodeAuthenticationCaptor.capture());

OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication =
authorizationCodeAuthenticationCaptor.getValue();
assertThat(authorizationCodeAuthentication.getCode()).isEqualTo(
request.getParameter(OAuth2ParameterNames.CODE));
assertThat(authorizationCodeAuthentication.getPrincipal()).isEqualTo(clientPrincipal);
assertThat(authorizationCodeAuthentication.getRedirectUri()).isEqualTo(
request.getParameter(OAuth2ParameterNames.REDIRECT_URI));

assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
OAuth2AccessTokenResponse accessTokenResponse = readAccessTokenResponse(response);

OAuth2AccessToken accessTokenResult = accessTokenResponse.getAccessToken();
assertThat(accessTokenResult.getTokenType()).isEqualTo(accessToken.getTokenType());
assertThat(accessTokenResult.getTokenValue()).isEqualTo(accessToken.getTokenValue());
assertThat(accessTokenResult.getIssuedAt()).isBetween(
accessToken.getIssuedAt().minusSeconds(1), accessToken.getIssuedAt().plusSeconds(1));
assertThat(accessTokenResult.getExpiresAt()).isBetween(
accessToken.getExpiresAt().minusSeconds(1), accessToken.getExpiresAt().plusSeconds(1));
assertThat(accessTokenResult.getScopes()).isEqualTo(accessToken.getScopes());
}

@Test
public void doFilterWhenTokenRequestMultipleScopeThenInvalidRequestError() throws Exception {
RegisteredClient registeredClient = TestRegisteredClients.registeredClient2().build();
Expand Down Expand Up @@ -363,6 +414,21 @@ private static MockHttpServletRequest createAuthorizationCodeTokenRequest(Regist
return request;
}

private static MockHttpServletRequest createAuthorizationCodeAndClientIdTokenRequest(RegisteredClient registeredClient) {
String[] redirectUris = registeredClient.getRedirectUris().toArray(new String[0]);

String requestUri = OAuth2TokenEndpointFilter.DEFAULT_TOKEN_ENDPOINT_URI;
MockHttpServletRequest request = new MockHttpServletRequest("POST", requestUri);
request.setServletPath(requestUri);

request.addParameter(OAuth2ParameterNames.GRANT_TYPE, AuthorizationGrantType.AUTHORIZATION_CODE.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vincent-hsin This request is invalid.

As per spec, in Section 4.1.3. Access Token Request:

If the client type is confidential or the client was issued client
credentials (or assigned other authentication requirements), the
client MUST authenticate with the authorization server as described
in Section 3.2.1.

Only confidential clients are supported at the moment, so the client MUST authenticate at the token endpoint.

The change in this PR would allow a malicious user to obtain a token.

FYI, public client authentication support is coming in #45.

I'm going to close this PR and associated issue based on the above explanation.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I got it. I will contribute for #45.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, there is a PR for #45. See #93

request.addParameter(OAuth2ParameterNames.CODE, "code");
request.addParameter(OAuth2ParameterNames.REDIRECT_URI, redirectUris[0]);
request.addParameter(OAuth2ParameterNames.CLIENT_ID, registeredClient.getClientId());

return request;
}

private static MockHttpServletRequest createClientCredentialsTokenRequest(RegisteredClient registeredClient) {
String requestUri = OAuth2TokenEndpointFilter.DEFAULT_TOKEN_ENDPOINT_URI;
MockHttpServletRequest request = new MockHttpServletRequest("POST", requestUri);
Expand Down