ayush786hack commented on code in PR #13676:
URL: https://github.com/apache/gravitino/pull/13676#discussion_r4237254933


##########
clients/client-java/src/main/java/org/apache/gravitino/client/OAuth2ClientUtil.java:
##########
@@ -37,111 +39,167 @@
 // core/src/main/java/org/apache/iceberg/rest/OAuth2Util.java
 class OAuth2ClientUtil {
 
-  public static final String SCOPE = "scope";
-  private static final Splitter CREDENTIAL_SPLITTER = 
Splitter.on(":").limit(2).trimResults();
-  private static final String GRANT_TYPE = "grant_type";
-  private static final String CLIENT_CREDENTIALS = "client_credentials";
-
-  private static final String CLIENT_ID = "client_id";
-  private static final String CLIENT_SECRET = "client_secret";
-
-  // error type constants
-  public static final String INVALID_REQUEST_ERROR = "invalid_request";
-  public static final String INVALID_CLIENT_ERROR = "invalid_client";
-  public static final String INVALID_GRANT_ERROR = "invalid_grant";
-  public static final String UNAUTHORIZED_CLIENT_ERROR = "unauthorized_client";
-  public static final String UNSUPPORTED_GRANT_TYPE_ERROR = 
"unsupported_grant_type";
-  public static final String INVALID_SCOPE_ERROR = "invalid_scope";
-
-  private OAuth2ClientUtil() {}
-
-  public static OAuth2TokenResponse fetchToken(
-      RESTClient client,
-      Map<String, String> headers,
-      String credential,
-      String scope,
-      String path) {
-    Map<String, String> request =
-        clientCredentialsRequest(
-            credential, scope != null ? ImmutableList.of(scope) : 
ImmutableList.of());
-
-    OAuth2TokenResponse response =
-        client.postForm(
-            path, request, OAuth2TokenResponse.class, headers, 
ErrorHandlers.oauthErrorHandler());
-    response.validate();
-
-    return response;
-  }
-
-  private static Pair<String, String> parseCredential(String credential) {
-    Preconditions.checkArgument(credential != null, "Invalid credential: 
null");
-    List<String> parts = CREDENTIAL_SPLITTER.splitToList(credential);
-    switch (parts.size()) {
-      case 2:
-        // client ID and client secret
-        return Pair.of(parts.get(0), parts.get(1));
-      case 1:
-        // client secret
-        return Pair.of(null, parts.get(0));
-      default:
-        // this should never happen because the credential splitter is limited 
to 2
-        throw new IllegalArgumentException("Invalid credential: " + 
credential);
+    public static final String SCOPE = "scope";
+    private static final Splitter CREDENTIAL_SPLITTER = 
Splitter.on(":").limit(2).trimResults();
+    private static final String GRANT_TYPE = "grant_type";
+    private static final String CLIENT_CREDENTIALS = "client_credentials";
+
+    private static final String CLIENT_ID = "client_id";
+    private static final String CLIENT_SECRET = "client_secret";
+
+    // error type constants
+    public static final String INVALID_REQUEST_ERROR = "invalid_request";
+    public static final String INVALID_CLIENT_ERROR = "invalid_client";
+    public static final String INVALID_GRANT_ERROR = "invalid_grant";
+    public static final String UNAUTHORIZED_CLIENT_ERROR = 
"unauthorized_client";
+    public static final String UNSUPPORTED_GRANT_TYPE_ERROR = 
"unsupported_grant_type";
+    public static final String INVALID_SCOPE_ERROR = "invalid_scope";
+
+    private OAuth2ClientUtil() {
     }
-  }
-
-  private static Map<String, String> clientCredentialsRequest(
-      String credential, List<String> scopes) {
-    Pair<String, String> credentialPair = parseCredential(credential);
-    return clientCredentialsRequest(credentialPair.getLeft(), 
credentialPair.getRight(), scopes);
-  }
-
-  private static Map<String, String> clientCredentialsRequest(
-      String clientId, String clientSecret, List<String> scopes) {
-    ImmutableMap.Builder<String, String> formData = ImmutableMap.builder();
-    formData.put(GRANT_TYPE, CLIENT_CREDENTIALS);
-    if (clientId != null) {
-      formData.put(CLIENT_ID, clientId);
+
+    public static OAuth2TokenResponse fetchToken(
+            RESTClient client,
+            Map<String, String> headers,
+            String credential,
+            String scope,
+            String path,
+            OAuth2ClientAuthenticationMethod authenticationMethod) {
+
+        List<String> scopes
+                = scope != null ? ImmutableList.of(scope) : ImmutableList.of();
+
+        Pair<Map<String, String>, Map<String, String>> request
+                = clientCredentialsRequest(
+                        credential, scopes, authenticationMethod);
+
+        OAuth2TokenResponse response
+                = client.postForm(
+                        path,
+                        request.getLeft(),
+                        OAuth2TokenResponse.class,
+                        mergeHeaders(headers, request.getRight()),
+                        ErrorHandlers.oauthErrorHandler());
+
+        response.validate();
+
+        return response;
     }
-    formData.put(CLIENT_SECRET, clientSecret);
-    formData.put(SCOPE, toScope(scopes));
-
-    return formData.build();
-  }
-
-  /**
-   * If the token is a JWT, extracts the expiration timestamp from the ext 
claim or null.
-   *
-   * @param token a token String
-   * @return The epoch millisecond the token expires at or null if it's not a 
valid JWT.
-   */
-  static Long expiresAtMillis(String token) {
-    if (null == token) {
-      return null;
+
+    private static Pair<String, String> parseCredential(String credential) {
+        Preconditions.checkArgument(credential != null, "Invalid credential: 
null");
+        List<String> parts = CREDENTIAL_SPLITTER.splitToList(credential);
+        switch (parts.size()) {
+            case 2:
+                // client ID and client secret
+                return Pair.of(parts.get(0), parts.get(1));
+            case 1:
+                // client secret
+                return Pair.of(null, parts.get(0));
+            default:
+                // this should never happen because the credential splitter is 
limited to 2
+                throw new IllegalArgumentException("Invalid credential: " + 
credential);
+        }
     }
 
-    List<String> parts = Splitter.on('.').splitToList(token);
-    if (parts.size() != 3) {
-      return null;
+    private static Pair<Map<String, String>, Map<String, String>> 
clientCredentialsRequest(
+            String credential,
+            List<String> scopes,
+            OAuth2ClientAuthenticationMethod authenticationMethod) {
+
+        Pair<String, String> credentialPair = parseCredential(credential);
+
+        if (authenticationMethod
+                == OAuth2ClientAuthenticationMethod.CLIENT_SECRET_BASIC) {
+            Preconditions.checkArgument(
+                    credentialPair.getLeft() != null,
+                    "Client ID is required for client_secret_basic 
authentication");
+            String encoded
+                    = Base64.getEncoder()
+                            .encodeToString(
+                                    (credentialPair.getLeft()
+                                            + ":"
+                                            + credentialPair.getRight())
+                                            .getBytes(StandardCharsets.UTF_8));

Review Comment:
   Fixed. The Java and Python clients now URL-encode the client ID and client 
secret separately before Base64 encoding, following RFC 6749 ยง2.3.1. I've also 
added a Python test covering credentials with reserved characters.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to