lasdf1234 commented on code in PR #13676:
URL: https://github.com/apache/gravitino/pull/13676#discussion_r4237516126
##########
clients/client-python/gravitino/filesystem/gvfs_utils.py:
##########
@@ -103,7 +103,7 @@ def create_client(
_check_auth_config(auth_type, GVFSConfig.OAUTH2_SCOPE, oauth2_scope)
oauth2_token_provider: OAuth2TokenProvider =
DefaultOAuth2TokenProvider(
- oauth2_server_uri, oauth2_credential, oauth2_path, oauth2_scope
+ oauth2_server_uri, oauth2_credential, oauth2_scope, oauth2_path
Review Comment:
**High — GFVS still defaults to client_secret_post**
`DefaultOAuth2TokenProvider` is constructed without `authentication_method`,
so Python GFVS users cannot select `client_secret_basic` even though the
provider supports it. Consider `GVFSConfig.OAUTH2_CLIENT_AUTHENTICATION_METHOD`
(or similar) and pass it here.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/OAuth2ClientUtil.java:
##########
@@ -89,10 +107,49 @@ private static Pair<String, String> parseCredential(String
credential) {
}
}
- private static Map<String, String> clientCredentialsRequest(
- String credential, List<String> scopes) {
+ private static Pair<Map<String, String>, Map<String, String>>
clientCredentialsRequest(
+ String credential,
+ List<String> scopes,
+ OAuth2ClientAuthenticationMethod authenticationMethod) {
Pair<String, String> credentialPair = parseCredential(credential);
- return clientCredentialsRequest(credentialPair.getLeft(),
credentialPair.getRight(), scopes);
+
+ if (authenticationMethod ==
OAuth2ClientAuthenticationMethod.CLIENT_SECRET_BASIC) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(credentialPair.getLeft()),
+ "Client ID is required for client_secret_basic authentication");
+
+ String encodedClientId;
+ String encodedClientSecret;
+
+ try {
+ encodedClientId =
+ URLEncoder.encode(credentialPair.getLeft(),
StandardCharsets.UTF_8.name());
+ encodedClientSecret =
+ URLEncoder.encode(credentialPair.getRight(),
StandardCharsets.UTF_8.name());
Review Comment:
**Medium — Java/Python Basic header can diverge**
`URLEncoder.encode` leaves some characters (e.g. `*` in `a*b`) literal,
while Python uses `quote_plus(..., safe="")` and encodes `*` as `%2A`. The same
credential string can therefore produce different `Authorization: Basic ...`
headers in Java vs Python.
Please align encoding rules and add a Java unit test mirroring
`test_client_secret_basic_encodes_reserved_characters`.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/TestOAuth2TokenProvider.java:
##########
@@ -184,4 +186,71 @@ public void testTokenNotFetchedWhenValid() throws
Exception {
String oldToken = provider.getAccessToken();
Assertions.assertEquals(accessToken, oldToken);
}
+
+ @Test
+ public void testClientSecretBasicAuthentication() throws Exception {
+ OAuth2TokenProvider.Builder builder =
+ DefaultOAuth2TokenProvider.builder()
+ .withUri(String.format("http://127.0.0.1:%d";, PORT))
+ .withCredential("clientId:clientSecret")
+ .withPath("oauth/token")
+ .withScope("test")
+
.withClientAuthenticationMethod(OAuth2ClientAuthenticationMethod.CLIENT_SECRET_BASIC);
+
+ OAuth2TokenResponse response =
+ new OAuth2TokenResponse("access-token", "refresh-token", "bearer",
3600, "test", null);
+ String respJson =
ObjectMapperProvider.objectMapper().writeValueAsString(response);
+
+ String basicAuth =
Review Comment:
**Low — add reserved-character coverage**
This case only checks alphanumeric `clientId:clientSecret` (raw UTF-8 before
Base64). Python already has
`test_client_secret_basic_encodes_reserved_characters`. An equivalent Java test
would lock behavior with `OAuth2ClientUtil` and prevent Java/Python drift.
--
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]
