diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java index b7c551aa564..05860983097 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java @@ -13,15 +13,14 @@ package org.cloudfoundry.identity.uaa.authentication; import org.cloudfoundry.identity.uaa.client.UaaClient; -import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants; import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationService; import org.cloudfoundry.identity.uaa.oauth.token.ClaimConstants; import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants; import org.cloudfoundry.identity.uaa.util.UaaStringUtils; +import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.authentication.dao.DaoAuthenticationProvider; import org.springframework.security.core.AuthenticationException; -import org.springframework.security.core.userdetails.User; import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.crypto.password.PasswordEncoder; @@ -56,18 +55,19 @@ protected void additionalAuthenticationChecks(UserDetails userDetails, UsernameP AuthenticationException error = null; for(String pwd: passwordList) { try { - User user = new User(userDetails.getUsername(), pwd, userDetails.isEnabled(), userDetails.isAccountNonExpired(), userDetails.isCredentialsNonExpired(), userDetails.isAccountNonLocked(), userDetails.getAuthorities()); - if (authentication.getCredentials() == null && isPublicGrantTypeUsageAllowed(authentication.getDetails()) && userDetails instanceof UaaClient) { - // in case of grant_type=authorization_code and code_verifier passed (PKCE) we check if client has option allowpublic with true and proceed even if no secret is provided - UaaClient uaaClient = (UaaClient) userDetails; - Object allowPublic = uaaClient.getAdditionalInformation().get(ClientConstants.ALLOW_PUBLIC); - if ((allowPublic instanceof String && Boolean.TRUE.toString().equalsIgnoreCase((String)allowPublic)) || - (allowPublic instanceof Boolean && Boolean.TRUE.equals(allowPublic))) { + UaaClient uaaClient = new UaaClient(userDetails, pwd); + if (authentication.getCredentials() == null) { + if (isPublicGrantTypeUsageAllowed(authentication.getDetails()) && uaaClient.isAllowPublic()) { + // in case of grant_type=authorization_code and code_verifier passed (PKCE) we check if client has option allowpublic with true and continue even if no secret is in request ((UaaAuthenticationDetails) authentication.getDetails()).setAuthenticationMethod(CLIENT_AUTH_NONE); break; } } - super.additionalAuthenticationChecks(user, authentication); + if (uaaClient.getPassword() == null) { + error = new BadCredentialsException("Missing credentials"); + break; + } + super.additionalAuthenticationChecks(uaaClient, authentication); error = null; break; } catch (AuthenticationException e) { diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrap.java b/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrap.java index e1bbc625833..8540f445911 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrap.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrap.java @@ -166,16 +166,14 @@ private void addNewClients() { String secondSecret = null; if (map.get("secret") instanceof List) { List secrets = (List) map.get("secret"); - if (secrets.isEmpty()) { - client.setClientSecret(""); - } else { + if (!secrets.isEmpty()) { client.setClientSecret(secrets.get(0) == null ? "" : secrets.get(0)); if (secrets.size() > 1) { secondSecret = secrets.get(1) == null ? "" : secrets.get(1); } } } else { - client.setClientSecret(map.get("secret") == null ? "" : (String) map.get("secret")); + client.setClientSecret((String) map.get("secret")); } Integer validity = (Integer) map.get("access-token-validity"); @@ -272,8 +270,10 @@ private void updatePasswordsIfChanged(String clientId, String rawPassword1, Stri // check if both passwords are still up to date // 1st line: client already has 2 passwords: check if both are still correct // 2nd line: client has only 1 pasword: check if password is correct and second password is null - if ( (existingPasswordHash.length > 1 && passwordEncoder.matches(rawPassword1, existingPasswordHash[0]) && passwordEncoder.matches(rawPassword2, existingPasswordHash[1]) ) - || (passwordEncoder.matches(rawPassword1, existingPasswordHash[0]) && rawPassword2 == null) ) { + if ( (existingPasswordHash.length > 1 && rawPassword1 != null + && passwordEncoder.matches(rawPassword1, existingPasswordHash[0]) + && rawPassword2 != null && passwordEncoder.matches(rawPassword2, existingPasswordHash[1]) ) + || (rawPassword1 != null && (passwordEncoder.matches(rawPassword1, existingPasswordHash[0]) && rawPassword2 == null)) ) { // no changes to passwords: nothing to do here return; } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClient.java b/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClient.java index 2edd451dc7e..fbea54c125e 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClient.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClient.java @@ -1,24 +1,58 @@ package org.cloudfoundry.identity.uaa.client; +import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.SpringSecurityCoreVersion; import org.springframework.security.core.userdetails.User; +import org.springframework.security.core.userdetails.UserDetails; import java.util.Collection; +import java.util.Collections; import java.util.Map; +import java.util.Optional; public class UaaClient extends User { private static final long serialVersionUID = SpringSecurityCoreVersion.SERIAL_VERSION_UID; private transient Map additionalInformation; + private final String secret; + public UaaClient(String username, String password, Collection authorities, Map additionalInformation) { - super(username, password, authorities); + super(username, password == null ? "" : password, authorities); this.additionalInformation = additionalInformation; + this.secret = password; + } + + public UaaClient(UserDetails userDetails, String secret) { + super(userDetails.getUsername(), secret == null ? "" : secret, userDetails.isEnabled(), userDetails.isAccountNonExpired(), + userDetails.isCredentialsNonExpired(), userDetails.isAccountNonLocked(), userDetails.getAuthorities()); + if (userDetails instanceof UaaClient) { + this.additionalInformation = ((UaaClient) userDetails).getAdditionalInformation(); + } + this.secret = secret; } - public Map getAdditionalInformation() { + public boolean isAllowPublic() { + Object allowPublic = Optional.ofNullable(additionalInformation).map(e -> e.get(ClientConstants.ALLOW_PUBLIC)).orElse(Collections.emptyMap()); + if ((allowPublic instanceof String && Boolean.TRUE.toString().equalsIgnoreCase((String) allowPublic)) || (allowPublic instanceof Boolean && Boolean.TRUE.equals(allowPublic))) { + return true; + } else { + return false; + } + } + + private Map getAdditionalInformation() { return this.additionalInformation; } + /** + * Allow to return a null password. Super class does not allow to omit a password, therefore use own method + * + * @return The password of the client, can be null if no secret is set + */ + @Override + public String getPassword() { + return this.secret; + } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClientDetailsUserDetailsService.java b/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClientDetailsUserDetailsService.java index 5824a0ad360..5f4d7776395 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClientDetailsUserDetailsService.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClientDetailsUserDetailsService.java @@ -3,26 +3,17 @@ import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.core.userdetails.UsernameNotFoundException; -import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.security.oauth2.provider.ClientDetailsService; import org.springframework.security.oauth2.provider.NoSuchClientException; public class UaaClientDetailsUserDetailsService implements UserDetailsService { private final ClientDetailsService clientDetailsService; - private String emptyPassword = ""; public UaaClientDetailsUserDetailsService(final ClientDetailsService clientDetailsService) { this.clientDetailsService = clientDetailsService; } - /** - * @param passwordEncoder the password encoder to set - */ - public void setPasswordEncoder(PasswordEncoder passwordEncoder) { - this.emptyPassword = passwordEncoder.encode(""); - } - public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException { UaaClientDetails clientDetails; try { @@ -30,11 +21,7 @@ public UserDetails loadUserByUsername(String username) throws UsernameNotFoundEx } catch (NoSuchClientException e) { throw new UsernameNotFoundException(e.getMessage(), e); } - String clientSecret = clientDetails.getClientSecret(); - if (clientSecret== null || clientSecret.trim().length()==0) { - clientSecret = emptyPassword; - } - return new UaaClient(username, clientSecret, clientDetails.getAuthorities(), clientDetails.getAdditionalInformation()); + return new UaaClient(username, clientDetails.getClientSecret(), clientDetails.getAuthorities(), clientDetails.getAdditionalInformation()); } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/zone/MultitenantJdbcClientDetailsService.java b/server/src/main/java/org/cloudfoundry/identity/uaa/zone/MultitenantJdbcClientDetailsService.java index c5ce7ee7466..78015f50461 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/zone/MultitenantJdbcClientDetailsService.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/zone/MultitenantJdbcClientDetailsService.java @@ -159,7 +159,7 @@ public void updateClientDetails(ClientDetails clientDetails, String zoneId) thro @Override public void updateClientSecret(String clientId, String secret, String zoneId) throws NoSuchClientException { - int count = jdbcTemplate.update(DEFAULT_UPDATE_SECRET_STATEMENT, passwordEncoder.encode(secret), clientId, zoneId); + int count = jdbcTemplate.update(DEFAULT_UPDATE_SECRET_STATEMENT, secret != null ? passwordEncoder.encode(secret) : null, clientId, zoneId); if (count != 1) { throw new NoSuchClientException("No client found with id = " + clientId); } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/UaaClientAuthenticationProviderTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/UaaClientAuthenticationProviderTest.java index fc14b22c74a..ffdc00cab52 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/UaaClientAuthenticationProviderTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/authentication/UaaClientAuthenticationProviderTest.java @@ -31,6 +31,7 @@ import java.util.Map; import static org.cloudfoundry.identity.uaa.oauth.client.ClientDetailsModification.SECRET; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -58,7 +59,6 @@ void setUpForClientTests() { jdbcClientDetailsService = new MultitenantJdbcClientDetailsService(jdbcTemplate, mockIdentityZoneManager, passwordEncoder); UaaClientDetailsUserDetailsService clientDetailsService = new UaaClientDetailsUserDetailsService(jdbcClientDetailsService); - clientDetailsService.setPasswordEncoder(passwordEncoder); client = createClient(); authenticationProvider = new ClientDetailsAuthenticationProvider(clientDetailsService, passwordEncoder); } @@ -195,6 +195,19 @@ void provider_authenticate_client_without_password_public_missing_code() { assertThrows(BadCredentialsException.class, () -> authenticationProvider.additionalAuthenticationChecks(new UaaClient("client", "secret", Collections.emptyList(), client.getAdditionalInformation()), a)); } + @Test + void provider_authenticate_client_without_secret_user_without_secret() { + client = new BaseClientDetails(generator.generate(), "", "", "client_credentials", "uaa.resource"); + jdbcClientDetailsService.addClientDetails(client); + UsernamePasswordAuthenticationToken a = mock(UsernamePasswordAuthenticationToken.class); + UaaAuthenticationDetails uaaAuthenticationDetails = mock(UaaAuthenticationDetails.class); + when(a.getDetails()).thenReturn(uaaAuthenticationDetails); + Map requestParameters = new HashMap<>(); + when(uaaAuthenticationDetails.getParameterMap()).thenReturn(requestParameters); + Exception e = assertThrows(BadCredentialsException.class, () -> authenticationProvider.additionalAuthenticationChecks(new UaaClient("client", null, Collections.emptyList(), client.getAdditionalInformation()), a)); + assertEquals("Missing credentials", e.getMessage()); + } + @Test void provider_authenticate_client_without_password_public_false() { client = createClient(ClientConstants.ALLOW_PUBLIC, false); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapMultipleSecretsTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapMultipleSecretsTest.java index afde19ba095..380a2af46fc 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapMultipleSecretsTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapMultipleSecretsTest.java @@ -223,7 +223,7 @@ public void newClientEmptyPasswordList() throws Exception { buildClient(null); clients.get(clientId).put("secret", new LinkedList<>()); clientAdminBootstrap.afterPropertiesSet(); - assertClient(""); + assertClient(null); } @Test @@ -280,7 +280,7 @@ public void updateTwoSecretClientSingletonNullList() throws Exception { private void assertClient(String password) { Assert.assertEquals(clientId, verifyClient.getClientId()); - Assert.assertEquals(password == null ? "" : password, verifyClient.getClientSecret()); + Assert.assertEquals(password, verifyClient.getClientSecret()); } private void buildClientSingletonList(String password1) { diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapTests.java index 8336a6bace1..d26b915bc66 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapTests.java @@ -443,6 +443,31 @@ void overrideClientWithEmptySecret() { reset(multitenantJdbcClientDetailsService); + Map map = new HashMap<>(); + map.put("secret", ""); + map.put("override", true); + map.put("authorized-grant-types", "client_credentials"); + clients.put(clientId, map); + + doThrow(new ClientAlreadyExistsException("Planned")) + .when(multitenantJdbcClientDetailsService).addClientDetails(any(ClientDetails.class), anyString()); + clientAdminBootstrap.afterPropertiesSet(); + verify(multitenantJdbcClientDetailsService, times(1)).addClientDetails(any(ClientDetails.class), anyString()); + ArgumentCaptor captor = ArgumentCaptor.forClass(ClientDetails.class); + verify(multitenantJdbcClientDetailsService, times(1)).updateClientDetails(captor.capture(), anyString()); + verify(multitenantJdbcClientDetailsService, times(1)).updateClientSecret(clientId, "", "uaa"); + assertEquals(new HashSet(Collections.singletonList("client_credentials")), captor.getValue().getAuthorizedGrantTypes()); + } + + @Test + void doNotOverrideClientWithNullSecret() { + String clientId = randomValueStringGenerator.generate(); + BaseClientDetails foo = new BaseClientDetails(clientId, "", "openid", "client_credentials,password", "uaa.none"); + foo.setClientSecret("secret"); + multitenantJdbcClientDetailsService.addClientDetails(foo); + + reset(multitenantJdbcClientDetailsService); + Map map = new HashMap<>(); map.put("secret", null); map.put("override", true); @@ -455,7 +480,7 @@ void overrideClientWithEmptySecret() { verify(multitenantJdbcClientDetailsService, times(1)).addClientDetails(any(ClientDetails.class), anyString()); ArgumentCaptor captor = ArgumentCaptor.forClass(ClientDetails.class); verify(multitenantJdbcClientDetailsService, times(1)).updateClientDetails(captor.capture(), anyString()); - verify(multitenantJdbcClientDetailsService, times(1)).updateClientSecret(clientId, "", "uaa"); + verify(multitenantJdbcClientDetailsService, times(1)).updateClientSecret(clientId, null, "uaa"); assertEquals(new HashSet(Collections.singletonList("client_credentials")), captor.getValue().getAuthorizedGrantTypes()); } diff --git a/uaa/src/main/webapp/WEB-INF/spring/oauth-clients.xml b/uaa/src/main/webapp/WEB-INF/spring/oauth-clients.xml index 8fa0667a055..b9710065b8d 100644 --- a/uaa/src/main/webapp/WEB-INF/spring/oauth-clients.xml +++ b/uaa/src/main/webapp/WEB-INF/spring/oauth-clients.xml @@ -31,6 +31,7 @@ + @@ -166,6 +167,7 @@ + diff --git a/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml b/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml index 85e239cf6f6..97e6e7ec8c7 100755 --- a/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml +++ b/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml @@ -310,7 +310,6 @@ - diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/util/MockMvcUtils.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/util/MockMvcUtils.java index 2d6b52d6ddb..a89ff0602bb 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/util/MockMvcUtils.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/util/MockMvcUtils.java @@ -1008,6 +1008,10 @@ public static BaseClientDetails createClient(MockMvc mockMvc, String accessToken public static BaseClientDetails createClient(ApplicationContext context, BaseClientDetails clientDetails, IdentityZone zone) { MultitenantJdbcClientDetailsService service = context.getBean(MultitenantJdbcClientDetailsService.class); + if (clientDetails.getClientSecret() == null) { + // provide for tests the empty secret behavior + clientDetails.setClientSecret(""); + } service.addClientDetails(clientDetails, zone.getId()); return (BaseClientDetails) service.loadClientByClientId(clientDetails.getClientId(), zone.getId()); } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/security/UaaUsingDelegatingPasswordEncoderMockMvcTest.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/security/UaaUsingDelegatingPasswordEncoderMockMvcTest.java index b660959b648..5e178e3155d 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/security/UaaUsingDelegatingPasswordEncoderMockMvcTest.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/security/UaaUsingDelegatingPasswordEncoderMockMvcTest.java @@ -29,7 +29,7 @@ void setUp() { jdbcTemplate.execute("INSERT INTO oauth_client_details (client_id, client_secret, authorized_grant_types, authorities) VALUES ('client_id_with_bcrypt', '{bcrypt}$2a$10$dXJ3SW6G7P50lGmMkkmwe.20cQQubK3.HZWzG3YB1tlRy.fqvM/BG', 'client_credentials', 'amazing.powers')"); jdbcTemplate.execute("INSERT INTO oauth_client_details (client_id, client_secret, authorized_grant_types, authorities) VALUES ('client_id_with_no_algorithm_id', 'password', 'client_credentials', 'amazing.powers')"); jdbcTemplate.execute("INSERT INTO oauth_client_details (client_id, client_secret, authorized_grant_types, authorities) VALUES ('client_id_with_no_password', NULL, 'client_credentials', 'amazing.powers')"); - jdbcTemplate.execute("INSERT INTO oauth_client_details (client_id, client_secret, authorized_grant_types, authorities) VALUES ('client_id_with_empty_password', '', 'client_credentials', 'amazing.powers')"); + jdbcTemplate.execute("INSERT INTO oauth_client_details (client_id, client_secret, authorized_grant_types, authorities) VALUES ('client_id_with_empty_password', '{noop}', 'client_credentials', 'amazing.powers')"); } @AfterEach @@ -54,10 +54,9 @@ void getClientCredentialsTokenWithValidPassword(String clientId) throws Exceptio @ParameterizedTest @ValueSource(strings = { - "client_id_with_no_password", "client_id_with_empty_password" }) - void tryToGetTokenWithNoPasswordSucceeds(String clientId) throws Exception { + void tryToGetTokenWithEmtpyPasswordSucceeds(String clientId) throws Exception { mockMvc.perform(post("/oauth/token") .contentType(MediaType.APPLICATION_FORM_URLENCODED) .accept(MediaType.APPLICATION_JSON) @@ -67,6 +66,20 @@ void tryToGetTokenWithNoPasswordSucceeds(String clientId) throws Exception { .andExpect(status().isOk()); } + @ParameterizedTest + @ValueSource(strings = { + "client_id_with_no_password" + }) + void tryToGetTokenWithEmtpyPasswordFails(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().isUnauthorized()); + } + @Test void tryToGetTokenWithInvalidPasswordFails() throws Exception { mockMvc.perform(post("/oauth/token")