Refactoring to fix issue #3232

Move the check for existing secrets to the
grant flow (runtime) and remove it from REST (configuration).

We support now private_key_jwt next to secret, so check all parts in REST is too less and will get complicated.
Fix a potential security issue because we must not create a token based on no authentication

model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java

@@ -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";

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

@@ -144,10 +144,6 @@ public ClientDetails validate(ClientDetails prototype, boolean create, boolean c
                 logger.debug("Invalid client: " + clientId + ". Scope cannot be empty for grant_type " + GRANT_TYPE_JWT_BEARER);
                 throw new InvalidClientDetailsException("Scope cannot be empty for grant_type " + GRANT_TYPE_JWT_BEARER);
             }
-            if (create && !StringUtils.hasText(client.getClientSecret())) {
-                logger.debug("Invalid client: " + clientId + ". Client secret is required for grant type " + GRANT_TYPE_JWT_BEARER);
-                throw new InvalidClientDetailsException("Client secret is required for grant type " + GRANT_TYPE_JWT_BEARER);
-            }
         }
 
         if (checkAdmin &&
@@ -235,11 +231,6 @@ public ClientDetails validate(ClientDetails prototype, boolean create, boolean c
             }
         }
         if (create) {
-            // Only check for missing secret if client is being created.
-            if (requestedGrantTypes.contains(GRANT_TYPE_CLIENT_CREDENTIALS) && !StringUtils.hasText(client.getClientSecret())) {
-                logger.debug("Client secret is required for client_credentials grant type");
-                throw new InvalidClientDetailsException("Client secret is required for client_credentials grant type");
-            }
             clientSecretValidator.validate(client.getClientSecret());
 
             if (prototype instanceof ClientDetailsCreation clientDetailsCreation) {

server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranter.java

@@ -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;
 
 /**
@@ -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);
@@ -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;
     }
-
 }

server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/token/AbstractTokenGranter.java

@@ -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;
@@ -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
@@ -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;
     }

server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/JwtTokenGranter.java

@@ -13,11 +13,18 @@
 import org.cloudfoundry.identity.uaa.oauth.common.exceptions.InvalidGrantException;
 import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetails;
 
+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_JWT_BEARER;
 
 public class JwtTokenGranter extends AbstractTokenGranter {
     final DefaultSecurityContextAccessor defaultSecurityContextAccessor;
 
+    private static final List<String> ALLOWED_AUTH_METHODS = List.of(CLIENT_AUTH_SECRET, CLIENT_AUTH_PRIVATE_KEY_JWT);
+            //comment I would add here CLIENT_AUTH_EMPTY to allow empty secret for jwt bearer, same as allowed for password
+
     public JwtTokenGranter(AuthorizationServerTokenServices tokenServices,
             MultitenantClientServices clientDetailsService,
             OAuth2RequestFactory requestFactory) {
@@ -41,7 +48,7 @@ protected Authentication validateRequest(TokenRequest request) {
         } else {
             throw new InvalidGrantException("User authentication not found");
         }
-        return SecurityContextHolder.getContext().getAuthentication();
+        return isValidClientAuthentication(ALLOWED_AUTH_METHODS) ? SecurityContextHolder.getContext().getAuthentication() : null;
     }
 
     @Override