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

Jump to bottom

Fix/user token #2193

Closed
wants to merge 5 commits into from
Closed

Fix/user token #2193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f87b6b1
fix: exclude unsupported response types in exception
mikeroda May 17, 2023
c762b0f
fix: allow bearer or Bearer for Authentication header
mikeroda May 17, 2023
3e93cae
Merge branch 'cloudfoundry:develop' into develop
mikeroda Aug 10, 2023
b105d77
Merge branch 'cloudfoundry:develop' into develop
mikeroda Aug 25, 2023
07ad972
fix: client in user_token grant must have secret
mikeroda Feb 2, 2023
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: 18 additions & 1 deletion server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/UserTokenGranter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,18 @@

package org.cloudfoundry.identity.uaa.oauth.token;

import org.cloudfoundry.identity.uaa.authentication.ClientDetailsAuthenticationProvider;
import org.cloudfoundry.identity.uaa.oauth.UaaOauth2Authentication;
import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices;
import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
import org.springframework.security.authentication.InsufficientAuthenticationException;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.oauth2.common.DefaultOAuth2AccessToken;
import org.springframework.security.oauth2.common.OAuth2AccessToken;
import org.springframework.security.oauth2.common.exceptions.InvalidClientException;
import org.springframework.security.oauth2.common.exceptions.InvalidGrantException;
import org.springframework.security.oauth2.provider.ClientDetails;
import org.springframework.security.oauth2.provider.OAuth2Authentication;
Expand All @@ -40,14 +44,17 @@ public class UserTokenGranter extends AbstractTokenGranter {

private MultitenantClientServices clientDetailsService;
private RevocableTokenProvisioning tokenStore;
private ClientDetailsAuthenticationProvider clientAuthenticationProvider;

public UserTokenGranter(AuthorizationServerTokenServices tokenServices,
MultitenantClientServices clientDetailsService,
OAuth2RequestFactory requestFactory,
RevocableTokenProvisioning tokenStore) {
RevocableTokenProvisioning tokenStore,
ClientDetailsAuthenticationProvider clientAuthenticationProvider) {
super(tokenServices, clientDetailsService, requestFactory, TokenConstants.GRANT_TYPE_USER_TOKEN);
this.clientDetailsService = clientDetailsService;
this.tokenStore = tokenStore;
this.clientAuthenticationProvider = clientAuthenticationProvider;
}

@Override
Expand Down Expand Up @@ -97,6 +104,16 @@ protected Authentication validateRequest(TokenRequest request) {
ClientDetails receiving = clientDetailsService.loadClientByClientId(request.getRequestParameters().get(CLIENT_ID), IdentityZoneHolder.get().getId());
super.validateGrantType(GRANT_TYPE_REFRESH_TOKEN, receiving);

//7. receiving client must have a client secret
Copy link
Member

Choose a reason for hiding this comment

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

I vote to perform the check in extra method, means, check if secret "" is set to client. I assume you agree if I provide a PR which would perform the check for you?

try {
UsernamePasswordAuthenticationToken clientAuth =
new UsernamePasswordAuthenticationToken(request.getRequestParameters().get(CLIENT_ID), "");
clientAuthenticationProvider.authenticate(clientAuth);
throw new InvalidClientException("Grant type is not allowed for this client");
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is very difficult to understand. It seems that an exception is expected in order for the grant to be validated. Could this (and perhaps the authenticate method that's called here) be restructured so that the requirement this is validating is easier to understand from looking at the code?

}
catch (AuthenticationException ignored) {
}

return oauth2Authentication.getUserAuthentication();
}

Expand Down
4 changes: 3 additions & 1 deletion server/src/test/java/org/cloudfoundry/identity/uaa/oauth/token/AddTokenGranterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package org.cloudfoundry.identity.uaa.oauth.token;

import org.cloudfoundry.identity.uaa.authentication.ClientDetailsAuthenticationProvider;
import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices;
import org.hamcrest.Matchers;
import org.junit.Before;
Expand Down Expand Up @@ -43,7 +44,8 @@ public void setup() {
mock(AuthorizationServerTokenServices.class),
mock(MultitenantClientServices.class),
mock(OAuth2RequestFactory.class),
mock(RevocableTokenProvisioning.class)
mock(RevocableTokenProvisioning.class),
mock(ClientDetailsAuthenticationProvider.class)
Copy link
Member

Choose a reason for hiding this comment

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

this would not be needed then

);
}

Expand Down
23 changes: 19 additions & 4 deletions server/src/test/java/org/cloudfoundry/identity/uaa/oauth/token/UserTokenGranterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package org.cloudfoundry.identity.uaa.oauth.token;

import org.cloudfoundry.identity.uaa.authentication.ClientDetailsAuthenticationProvider;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthentication;
import org.cloudfoundry.identity.uaa.oauth.UaaOauth2Authentication;
import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices;
Expand All @@ -22,7 +23,9 @@
import org.junit.Before;
import org.junit.Test;
import org.springframework.security.authentication.InsufficientAuthenticationException;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.oauth2.common.DefaultOAuth2AccessToken;
import org.springframework.security.oauth2.common.DefaultOAuth2RefreshToken;
Expand All @@ -42,6 +45,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.eq;
Expand All @@ -66,6 +70,7 @@ public class UserTokenGranterTest {
private BaseClientDetails requestingClient;
private BaseClientDetails receivingClient;
private RevocableTokenProvisioning tokenStore;
private ClientDetailsAuthenticationProvider clientAuthenticationProvider;

@Before
public void setup() {
Expand All @@ -74,13 +79,15 @@ public void setup() {
requestFactory = mock(OAuth2RequestFactory.class);
authentication = mock(UaaOauth2Authentication.class);
tokenStore = mock(RevocableTokenProvisioning.class);
clientAuthenticationProvider = mock(ClientDetailsAuthenticationProvider.class);

userAuthentication = mock(UaaAuthentication.class);
granter = new UserTokenGranter(
tokenServices,
clientDetailsService,
requestFactory,
tokenStore
tokenStore,
clientAuthenticationProvider
Copy link
Member

Choose a reason for hiding this comment

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

this would not be needed then

);
SecurityContextHolder.getContext().setAuthentication(authentication);

Expand All @@ -94,8 +101,6 @@ public void setup() {
requestParameters.put(CLIENT_ID, receivingClient.getClientId());
tokenRequest = new PublicTokenRequest();
tokenRequest.setRequestParameters(requestParameters);


}

@After
Expand Down Expand Up @@ -168,7 +173,8 @@ public void ensure_client_gets_swapped() {
tokenServices,
clientDetailsService,
requestFactory,
tokenStore
tokenStore,
clientAuthenticationProvider
) {
@Override
protected DefaultOAuth2AccessToken prepareForSerialization(DefaultOAuth2AccessToken token) {
Expand All @@ -188,6 +194,15 @@ protected Authentication validateRequest(TokenRequest request) {

@Test
public void happy_day() {
when(clientAuthenticationProvider.authenticate(any(UsernamePasswordAuthenticationToken.class)))
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this test? It was a little bit confusing before this change , but now with a mock that throws an exception but the name "happy_day" I can't tell if this is a happy path test or not.

.thenThrow(new BadCredentialsException("Bad credentials"));
missing_parameter("non existent");
}

@Test(expected = InvalidClientException.class)
public void test_client_no_secret() {
when(clientAuthenticationProvider.authenticate(any(UsernamePasswordAuthenticationToken.class)))
.thenReturn(null);
missing_parameter("non existent");
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this method was already there, but can we improve the name of missing_parameter()? Maybe validate_with_missing_parameter()?

}

Expand Down
1 change: 1 addition & 0 deletions uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
<constructor-arg name="clientDetailsService" ref="jdbcClientDetailsService"/>
<constructor-arg name="requestFactory" ref="authorizationRequestManager"/>
<constructor-arg name="tokenStore" ref="revocableTokenProvisioning"/>
<constructor-arg name="clientAuthenticationProvider" ref="clientAuthenticationProvider"/>
</bean>

<bean id="addUserTokenGranter"
Expand Down
Loading