fix: do not default a missing secret to an empty one

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,18 @@ 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) {
+                    throw new BadCredentialsException("Missing credentials");
+                }
+                super.additionalAuthenticationChecks(uaaClient, authentication);
                 error = null;
                 break;
             } catch (AuthenticationException e) {

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<String> secrets = (List<String>) 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;
             }

server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClient.java

@@ -1,24 +1,53 @@
 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<String, Object> additionalInformation;
 
+  private String secret;
+
   public UaaClient(String username, String password, Collection<? extends GrantedAuthority> authorities, Map<String, Object> 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<String, Object> 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<String, Object> getAdditionalInformation() {
     return this.additionalInformation;
   }
 
+  @Override
+  public String getPassword() {
+    return this.secret;
+  }
 }

server/src/main/java/org/cloudfoundry/identity/uaa/client/UaaClientDetailsUserDetailsService.java

@@ -3,39 +3,26 @@
 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.ClientDetails;
 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 {
     ClientDetails clientDetails;
     try {
       clientDetails = clientDetailsService.loadClientByClientId(username);
     } 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());
   }
 
 }

server/src/main/java/org/cloudfoundry/identity/uaa/zone/MultitenantJdbcClientDetailsService.java

@@ -155,7 +155,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);
         }

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<String, String[]> 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);

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) {

server/src/test/java/org/cloudfoundry/identity/uaa/client/ClientAdminBootstrapTests.java

@@ -443,6 +443,31 @@ void overrideClientWithEmptySecret() {
 
             reset(multitenantJdbcClientDetailsService);
 
+            Map<String, Object> 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<ClientDetails> 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<String, Object> 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<ClientDetails> 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());
         }
 

uaa/src/main/webapp/WEB-INF/spring/oauth-clients.xml

@@ -31,6 +31,7 @@
                     </entry>
                     <entry key="cf">
                         <map>
+                            <entry key="secret" value=""/>
                             <entry key="authorized-grant-types" value="implicit,password,refresh_token"/>
                             <entry key="scope"
                                    value="uaa.user,cloud_controller.read,cloud_controller.write,openid,password.write,scim.userids,cloud_controller.admin,scim.read,scim.write"/>
@@ -166,6 +167,7 @@
                     </entry>
                     <entry key="oauth_showcase_user_token_public">
                         <map>
+                            <entry key="secret" value=""/>
                             <entry key="authorized-grant-types" value="user_token,password,authorization_code"/>
                             <entry key="scope" value="openid,uaa.user"/>
                             <entry key="redirect-uri" value="http://localhost:8080/uaa/"/>

uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml

@@ -310,7 +310,6 @@
     <bean id="clientDetailsUserService"
           class="org.cloudfoundry.identity.uaa.client.UaaClientDetailsUserDetailsService">
         <constructor-arg ref="jdbcClientDetailsService"/>
-        <property name="passwordEncoder" ref="cachingPasswordEncoder"/>
     </bean>
 
     <bean id="oauthAuthorizeRequestMatcher" class="org.cloudfoundry.identity.uaa.security.web.UaaRequestMatcher">

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());
     }