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

Fix the client_auth_method check #3243

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Fix the client_auth_method check #3243

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
0cee89b
Test to ensure client credentials auth check
strehle Jan 17, 2025
a22e808
Fix the client_auth_method check
strehle Jan 17, 2025
2e28c11
Fix test
strehle Jan 17, 2025
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
1 change: 1 addition & 0 deletions model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public static List<String> getStringValues() {

public static final String CLIENT_AUTH_NONE = ClientAuthentication.NONE;
public static final String CLIENT_AUTH_EMPTY = "empty";
public static final String CLIENT_AUTH_SECRET = "secret";
public static final String CLIENT_AUTH_PRIVATE_KEY_JWT = ClientAuthentication.PRIVATE_KEY_JWT;

public static final String ID_TOKEN_HINT_PROMPT = "prompt";
Expand Down
9 changes: 7 additions & 2 deletions ...va/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
import org.cloudfoundry.identity.uaa.oauth.provider.token.AuthorizationServerTokenServices;
import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetailsService;

import java.util.List;

import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.CLIENT_AUTH_PRIVATE_KEY_JWT;
import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.CLIENT_AUTH_SECRET;
import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.GRANT_TYPE_CLIENT_CREDENTIALS;

/**
Expand All @@ -20,6 +24,8 @@
*/
public class ClientCredentialsTokenGranter extends AbstractTokenGranter {

private static final List<String> ALLOWED_AUTH_METHODS = List.of(CLIENT_AUTH_SECRET, CLIENT_AUTH_PRIVATE_KEY_JWT);

public ClientCredentialsTokenGranter(AuthorizationServerTokenServices tokenServices,
ClientDetailsService clientDetailsService, OAuth2RequestFactory requestFactory) {
this(tokenServices, clientDetailsService, requestFactory, GRANT_TYPE_CLIENT_CREDENTIALS);
Expand All @@ -33,13 +39,12 @@ protected ClientCredentialsTokenGranter(AuthorizationServerTokenServices tokenSe
@Override
public OAuth2AccessToken grant(String grantType, TokenRequest tokenRequest) {
OAuth2AccessToken token = super.grant(grantType, tokenRequest);
if (token != null) {
if (token != null && isValidClientAuthentication(ALLOWED_AUTH_METHODS)) {
DefaultOAuth2AccessToken norefresh = new DefaultOAuth2AccessToken(token);
// The spec says that client credentials should not be allowed to get a refresh token
norefresh.setRefreshToken(null);
token = norefresh;
}
return token;
}

}
30 changes: 30 additions & 0 deletions ...rc/main/java/org/cloudfoundry/identity/uaa/oauth/provider/token/AbstractTokenGranter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails;
import org.cloudfoundry.identity.uaa.oauth.common.OAuth2AccessToken;
import org.cloudfoundry.identity.uaa.oauth.common.exceptions.InvalidRequestException;
import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2Authentication;
import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2Request;
import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2RequestFactory;
Expand All @@ -11,8 +13,16 @@
import org.cloudfoundry.identity.uaa.oauth.common.exceptions.InvalidClientException;
import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetails;
import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetailsService;
import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants;
import org.cloudfoundry.identity.uaa.util.UaaSecurityContextUtils;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContext;
import org.springframework.security.core.context.SecurityContextHolder;

import java.util.Collection;
import java.util.List;
import java.util.Optional;

/**
* Moved class AbstractTokenGranter implementation of from spring-security-oauth2 into UAA
Expand Down Expand Up @@ -76,6 +86,26 @@ protected void validateGrantType(String grantType, ClientDetails clientDetails)
}
}

protected boolean isValidClientAuthentication(List<String> allowedMethods) {
SecurityContext context = SecurityContextHolder.getContext();
if (allowedMethods.contains(Optional.ofNullable(getUaaClientAuthenticationMethod(context.getAuthentication()))
.orElse(TokenConstants.CLIENT_AUTH_SECRET))) {
// internal null for client authentication means secret based (authorization header or post body)
return true;
} else {
throw new InvalidRequestException("Client credentials required");
}
}

protected String getUaaClientAuthenticationMethod(Authentication authentication) {
if (authentication instanceof UsernamePasswordAuthenticationToken && authentication.isAuthenticated() &&
authentication.getDetails() instanceof UaaAuthenticationDetails) {
return ((UaaAuthenticationDetails) authentication.getDetails()).getAuthenticationMethod();
} else {
return UaaSecurityContextUtils.getClientAuthenticationMethod(authentication);
}
}

protected AuthorizationServerTokenServices getTokenServices() {
return tokenServices;
}
Expand Down
30 changes: 30 additions & 0 deletions ...g/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@
package org.cloudfoundry.identity.uaa.oauth.provider.client;

import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails;
import org.cloudfoundry.identity.uaa.oauth.common.DefaultOAuth2AccessToken;
import org.cloudfoundry.identity.uaa.oauth.common.OAuth2AccessToken;
import org.cloudfoundry.identity.uaa.oauth.common.exceptions.InvalidRequestException;
import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetails;
import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetailsService;
import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2Request;
import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2RequestFactory;
import org.cloudfoundry.identity.uaa.oauth.provider.TokenRequest;
import org.cloudfoundry.identity.uaa.oauth.provider.token.AuthorizationServerTokenServices;
import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.context.SecurityContextHolder;

import java.util.Collections;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand All @@ -35,6 +42,11 @@ void setUp() {
clientCredentialsTokenGranter = new ClientCredentialsTokenGranter(tokenServices, clientDetailsService, requestFactory);
}

@AfterEach
void cleanUp() {
SecurityContextHolder.clearContext();
}

@Test
void grant() {
OAuth2Request oAuth2Request = mock(OAuth2Request.class);
Expand All @@ -54,4 +66,22 @@ void grantNoToken() {
when(oAuth2Request.getAuthorities()).thenReturn(Collections.emptyList());
assertThat(clientCredentialsTokenGranter.grant(TokenConstants.GRANT_TYPE_CLIENT_CREDENTIALS, tokenRequest)).isNull();
}

@Test
void grantNoSecretFails() {
UsernamePasswordAuthenticationToken authentication = new UsernamePasswordAuthenticationToken("username", null, null);
SecurityContextHolder.getContext().setAuthentication(authentication);
OAuth2Request oAuth2Request = mock(OAuth2Request.class);
UaaAuthenticationDetails uaaAuthenticationDetails = mock(UaaAuthenticationDetails.class);
authentication.setDetails(uaaAuthenticationDetails);
when(clientDetailsService.loadClientByClientId(any())).thenReturn(mock(ClientDetails.class));
when(requestFactory.createOAuth2Request(any(), any())).thenReturn(oAuth2Request);
when(tokenServices.createAccessToken(any())).thenReturn(new DefaultOAuth2AccessToken("eyJx"));
when(oAuth2Request.getAuthorities()).thenReturn(Collections.emptyList());
when(uaaAuthenticationDetails.getAuthenticationMethod()).thenReturn("none");
assertThatThrownBy(() -> clientCredentialsTokenGranter
.grant(TokenConstants.GRANT_TYPE_CLIENT_CREDENTIALS, tokenRequest))
.isInstanceOf(InvalidRequestException.class)
.hasMessage("Client credentials required");
}
}
4 changes: 2 additions & 2 deletions .../org/cloudfoundry/identity/uaa/security/UaaUsingDelegatingPasswordEncoderMockMvcTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,14 @@ void getClientCredentialsTokenWithValidPassword(String clientId) throws Exceptio
@ValueSource(strings = {
"client_id_with_empty_password"
})
void tryToGetTokenWithEmtpyPasswordSucceeds(String clientId) throws Exception {
void tryToGetTokenWithEmtpyPasswordMustFail(String clientId) throws Exception {
mockMvc.perform(post("/oauth/token")
.contentType(MediaType.APPLICATION_FORM_URLENCODED)
.accept(MediaType.APPLICATION_JSON)
.param("client_id", clientId)
.param("client_secret", "")
.param("grant_type", "client_credentials"))
.andExpect(status().isOk());
.andExpect(status().isBadRequest());
}

@ParameterizedTest
Expand Down
Loading