lasdf1234 commented on code in PR #13676:
URL: https://github.com/apache/gravitino/pull/13676#discussion_r4215657731
##########
clients/client-python/gravitino/auth/default_oauth2_token_provider.py:
##########
@@ -43,20 +46,22 @@ class DefaultOAuth2TokenProvider(OAuth2TokenProvider):
_scope: Optional[str]
_path: Optional[str]
_token: Optional[str]
-
+ _authentication_method: OAuth2ClientAuthenticationMethod
def __init__(
self,
uri: str = None,
credential: str = None,
scope: str = None,
+ authentication_method: OAuth2ClientAuthenticationMethod =
+ OAuth2ClientAuthenticationMethod.CLIENT_SECRET_POST,
path: str = None,
Review Comment:
**Blocking: positional API break.**
Inserting `authentication_method` before `path` changes the meaning of
existing positional calls such as:
```python
DefaultOAuth2TokenProvider(uri, credential, scope, path)
```
After this change, `path` is bound to `authentication_method` and `_path`
stays `None`, so `validate()` fails.
Please put the new parameter **after** `path`, or make it keyword-only (`*,
authentication_method=...`).
Also note in-repo positional usage in `gvfs_utils.py`
(`DefaultOAuth2TokenProvider(uri, credential, path, scope)`), which would be
further broken by this ordering change.
##########
clients/client-python/gravitino/auth/default_oauth2_token_provider.py:
##########
@@ -103,16 +108,39 @@ def _parse_credential(self):
def _fetch_token(self) -> str:
client_id, client_secret = self._parse_credential()
- client_credential_request = OAuth2ClientCredentialRequest(
- grant_type=CLIENT_CREDENTIALS,
- client_id=client_id,
- client_secret=client_secret,
- scope=self._scope,
- )
+ headers = {}
+
+ if (
+ self._authentication_method
+ == OAuth2ClientAuthenticationMethod.CLIENT_SECRET_BASIC
+ ):
+ credentials = f"{client_id}:{client_secret}"
Review Comment:
**Blocking: missing client_id validation for `client_secret_basic`.**
When credential is secret-only, `_parse_credential()` returns
`client_id=None`, and this builds `None:<secret>` into the Basic header.
Please reject missing/blank `client_id` here (Java already checks this in
`OAuth2ClientUtil`).
##########
clients/client-python/gravitino/auth/default_oauth2_token_provider.py:
##########
@@ -103,16 +108,39 @@ def _parse_credential(self):
def _fetch_token(self) -> str:
client_id, client_secret = self._parse_credential()
- client_credential_request = OAuth2ClientCredentialRequest(
- grant_type=CLIENT_CREDENTIALS,
- client_id=client_id,
- client_secret=client_secret,
- scope=self._scope,
- )
+ headers = {}
+
+ if (
+ self._authentication_method
+ == OAuth2ClientAuthenticationMethod.CLIENT_SECRET_BASIC
+ ):
+ credentials = f"{client_id}:{client_secret}"
+ encoded_credentials = base64.b64encode(
+ credentials.encode("utf-8")
+ ).decode("utf-8")
+
+ headers["Authorization"] = f"Basic {encoded_credentials}"
+
+ client_credential_request = OAuth2ClientCredentialRequest(
+ grant_type=CLIENT_CREDENTIALS,
+ client_id=None,
+ client_secret=None,
+ scope=self._scope,
+
+ )
+
+ else:
+ client_credential_request = OAuth2ClientCredentialRequest(
+ grant_type=CLIENT_CREDENTIALS,
+ client_id=client_id,
+ client_secret=client_secret,
+ scope=self._scope,
+ )
resp = self._client.post_form(
self._path,
data=client_credential_request,
+ headers=headers,
Review Comment:
Nit: inconsistent indentation on `headers=headers` (extra leading space).
Please align with the neighboring kwargs.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/DefaultOAuth2TokenProvider.java:
##########
@@ -21,100 +21,124 @@
import com.google.common.base.Preconditions;
import java.util.Collections;
import org.apache.commons.lang3.StringUtils;
-
-/** This class is the default implement of OAuth2TokenProvider. */
+import org.apache.gravitino.client.OAuth2ClientAuthenticationMethod;
Review Comment:
Nit: unnecessary same-package import. `OAuth2ClientAuthenticationMethod` is
already in `org.apache.gravitino.client`.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/OAuth2ClientAuthenticationMethod.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
Review Comment:
Please use the full Apache 2.0 license header used by other files in this
module (including the license URL and AS-IS terms). The shortened header will
likely fail license checks.
##########
clients/client-python/tests/unittests/auth/test_oauth2_token_provider.py:
##########
@@ -114,3 +117,28 @@ def test_authentication_with_jwt(self, *mock_methods):
token_provider.get_token_data().decode("utf-8"),
AuthConstants.AUTHORIZATION_BEARER_HEADER + new_access_token,
)
+
+ @patch(
+ "gravitino.utils.http_client.HTTPClient.post_form",
+ return_value=mock_base.mock_authentication_with_basic_jwt(),
+ )
+ def test_client_secret_basic_authentication(self, mock_post_form):
+
+ token_provider = DefaultOAuth2TokenProvider(
+ uri=f"http://127.0.0.1:{OAUTH_PORT}";,
+ credential="clientId:clientSecret",
+ path="oauth/token",
+ scope="test",
+ authentication_method=(
+ OAuth2ClientAuthenticationMethod.CLIENT_SECRET_BASIC
+ ),
+ )
+
+ self.assertTrue(token_provider.has_token_data())
+
+ _, kwargs = mock_post_form.call_args
+
+ self.assertEqual(
+ kwargs["headers"]["Authorization"],
+ "Basic Y2xpZW50SWQ6Y2xpZW50U2VjcmV0",
Review Comment:
Please also assert the request `data` / form payload does not include
`client_id` or `client_secret` when using basic auth.
And add a case for secret-only credential + `CLIENT_SECRET_BASIC` expecting
a clear validation error.
##########
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:
Optional / RFC note: RFC 6749 ยง2.3.1 says the client id and secret should be
`application/x-www-form-urlencoded` encoded **before** Base64 for
`client_secret_basic`.
Direct UTF-8 encoding is common and fine for simple credentials, but secrets
with reserved characters may fail against strict IdPs. Worth aligning with the
RFC (the linked issue cites it), or at least documenting the limitation.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/TestOAuth2TokenProvider.java:
##########
@@ -29,159 +28,217 @@
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.security.KeyPair;
+import java.util.Base64;
import java.util.Date;
import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.dto.responses.OAuth2ErrorResponse;
import org.apache.gravitino.dto.responses.OAuth2TokenResponse;
import org.apache.gravitino.exceptions.BadRequestException;
import org.apache.gravitino.exceptions.UnauthorizedException;
+
import org.apache.hc.core5.http.HttpStatus;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.mockserver.integration.ClientAndServer;
import org.mockserver.matchers.Times;
+import org.mockserver.model.HttpRequest;
import org.mockserver.model.HttpResponse;
@SuppressWarnings("JavaUtilDate")
public class TestOAuth2TokenProvider {
- private static final int PORT = 1082;
- private static ClientAndServer mockServer;
-
- @BeforeAll
- public static void beforeClass() {
- mockServer = startClientAndServer(PORT);
- }
-
- @AfterAll
- public static void stopServer() throws IOException {
- mockServer.stop();
- }
-
- @Test
- public void testProviderInitException() throws Exception {
- DefaultOAuth2TokenProvider.Builder tokenProvider1 =
- DefaultOAuth2TokenProvider.builder().withUri("test");
- DefaultOAuth2TokenProvider.Builder tokenProvider2 =
-
DefaultOAuth2TokenProvider.builder().withUri("test").withCredential("xx");
- DefaultOAuth2TokenProvider.Builder tokenProvider3 =
-
DefaultOAuth2TokenProvider.builder().withUri("test").withCredential("xx").withScope("test");
-
- Assertions.assertThrows(IllegalArgumentException.class, () ->
tokenProvider1.build());
- Assertions.assertThrows(IllegalArgumentException.class, () ->
tokenProvider2.build());
- Assertions.assertThrows(IllegalArgumentException.class, () ->
tokenProvider3.build());
- }
-
- @Test
- public void testAuthenticationError() throws Exception {
-
- HttpResponse mockResponse =
-
HttpResponse.response().withStatusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR);
- OAuth2ErrorResponse respBody =
- new OAuth2ErrorResponse(OAuth2ClientUtil.INVALID_CLIENT_ERROR,
"invalid");
- String respJson =
ObjectMapperProvider.objectMapper().writeValueAsString(respBody);
- mockResponse = mockResponse.withBody(respJson);
- mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
- OAuth2TokenProvider.Builder builder =
- DefaultOAuth2TokenProvider.builder()
- .withUri(String.format("http://127.0.0.1:%d";, PORT))
- .withCredential("yy:xx")
- .withPath("oauth/token")
- .withScope("test");
- Assertions.assertThrows(UnauthorizedException.class, builder::build);
-
- respBody = new OAuth2ErrorResponse(OAuth2ClientUtil.INVALID_GRANT_ERROR,
"invalid");
- respJson =
ObjectMapperProvider.objectMapper().writeValueAsString(respBody);
- mockResponse = mockResponse.withBody(respJson);
- mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
- Assertions.assertThrows(BadRequestException.class, builder::build);
- }
-
- @Test
- public void testAuthenticationNormal() throws Exception {
- OAuth2TokenProvider.Builder builder =
- DefaultOAuth2TokenProvider.builder()
- .withUri(String.format("http://127.0.0.1:%d";, PORT))
- .withCredential("yy:xx")
- .withPath("oauth/token")
- .withScope("test");
-
- ObjectMapper objectMapper = ObjectMapperProvider.objectMapper();
- HttpResponse mockResponse =
HttpResponse.response().withStatusCode(HttpStatus.SC_OK);
- OAuth2TokenResponse response = new OAuth2TokenResponse("1", "2", "3", 1,
"test", null);
- String respJson = objectMapper.writeValueAsString(response);
- mockResponse = mockResponse.withBody(respJson);
- mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
- Assertions.assertThrows(IllegalArgumentException.class, builder::build);
- response = new OAuth2TokenResponse("1", "2", "bearer", 1, "test", null);
- respJson = objectMapper.writeValueAsString(response);
- mockResponse = mockResponse.withBody(respJson);
- mockServer.when(any(), Times.exactly(2)).respond(mockResponse);
- OAuth2TokenProvider provider = builder.build();
- Assertions.assertTrue(provider.hasTokenData());
- Assertions.assertNotNull(provider.getTokenData());
- KeyPair keyPair = Keys.keyPairFor(SignatureAlgorithm.RS256);
- String oldAccessToken =
- Jwts.builder()
- .setSubject("gravitino")
- .setExpiration(new Date(System.currentTimeMillis() - 5))
- .setAudience("service1")
- .signWith(keyPair.getPrivate(), SignatureAlgorithm.RS256)
- .compact();
-
- response = new OAuth2TokenResponse(oldAccessToken, "2", "bearer", 1,
"test", null);
- respJson = objectMapper.writeValueAsString(response);
- mockResponse = mockResponse.withBody(respJson);
- mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
- provider = builder.build();
- String accessToken =
- Jwts.builder()
- .setSubject("gravitino")
- .setExpiration(new Date(System.currentTimeMillis() + 10000))
- .setAudience("service1")
- .signWith(keyPair.getPrivate(), SignatureAlgorithm.RS256)
- .compact();
-
- response = new OAuth2TokenResponse(accessToken, "2", "bearer", 1, "test",
null);
- respJson =
ObjectMapperProvider.objectMapper().writeValueAsString(response);
- mockResponse = mockResponse.withBody(respJson);
- mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
- Assertions.assertNotEquals(accessToken, oldAccessToken);
- Assertions.assertEquals(
- AuthConstants.AUTHORIZATION_BEARER_HEADER + accessToken,
- new String(provider.getTokenData(), StandardCharsets.UTF_8));
- }
-
- @Test
- public void testTokenNotFetchedWhenValid() throws Exception {
- OAuth2TokenProvider.Builder builder =
- DefaultOAuth2TokenProvider.builder()
- .withUri(String.format("http://127.0.0.1:%d";, PORT))
- .withCredential("yy:xx")
- .withPath("oauth/token")
- .withScope("test");
- HttpResponse mockResponse =
HttpResponse.response().withStatusCode(HttpStatus.SC_OK);
- ObjectMapper objectMapper = ObjectMapperProvider.objectMapper();
- KeyPair keyPair = Keys.keyPairFor(SignatureAlgorithm.RS256);
- String accessToken =
- Jwts.builder()
- .setSubject("gravitino")
- .setExpiration(new Date(System.currentTimeMillis() + 10000))
- .setAudience("service1")
- .signWith(keyPair.getPrivate(), SignatureAlgorithm.RS256)
- .compact();
-
- OAuth2TokenResponse response =
- new OAuth2TokenResponse(accessToken, "2", "bearer", 1, "test", null);
- String respJson = objectMapper.writeValueAsString(response);
- mockResponse = mockResponse.withBody(respJson);
- mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
- OAuth2TokenProvider provider = builder.build();
- String token = provider.getAccessToken();
- Assertions.assertEquals(accessToken, token);
- String oldToken = provider.getAccessToken();
- Assertions.assertEquals(accessToken, oldToken);
- }
+ private static final int PORT = 1082;
+ private static ClientAndServer mockServer;
+
+ @BeforeAll
+ public static void beforeClass() {
+ mockServer = startClientAndServer(PORT);
+ }
+
+ @AfterAll
+ public static void stopServer() throws IOException {
+ mockServer.stop();
+ }
+
+ @Test
+ public void testProviderInitException() throws Exception {
+ DefaultOAuth2TokenProvider.Builder tokenProvider1
+ = DefaultOAuth2TokenProvider.builder().withUri("test");
+ DefaultOAuth2TokenProvider.Builder tokenProvider2
+ =
DefaultOAuth2TokenProvider.builder().withUri("test").withCredential("xx");
+ DefaultOAuth2TokenProvider.Builder tokenProvider3
+ =
DefaultOAuth2TokenProvider.builder().withUri("test").withCredential("xx").withScope("test");
+
+ Assertions.assertThrows(IllegalArgumentException.class, () ->
tokenProvider1.build());
+ Assertions.assertThrows(IllegalArgumentException.class, () ->
tokenProvider2.build());
+ Assertions.assertThrows(IllegalArgumentException.class, () ->
tokenProvider3.build());
+ }
+
+ @Test
+ public void testAuthenticationError() throws Exception {
+
+ HttpResponse mockResponse
+ =
HttpResponse.response().withStatusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR);
+ OAuth2ErrorResponse respBody
+ = new
OAuth2ErrorResponse(OAuth2ClientUtil.INVALID_CLIENT_ERROR, "invalid");
+ String respJson =
ObjectMapperProvider.objectMapper().writeValueAsString(respBody);
+ mockResponse = mockResponse.withBody(respJson);
+ mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
+ OAuth2TokenProvider.Builder builder
+ = DefaultOAuth2TokenProvider.builder()
+ .withUri(String.format("http://127.0.0.1:%d";, PORT))
+ .withCredential("yy:xx")
+ .withPath("oauth/token")
+ .withScope("test");
+ Assertions.assertThrows(UnauthorizedException.class, builder::build);
+
+ respBody = new
OAuth2ErrorResponse(OAuth2ClientUtil.INVALID_GRANT_ERROR, "invalid");
+ respJson =
ObjectMapperProvider.objectMapper().writeValueAsString(respBody);
+ mockResponse = mockResponse.withBody(respJson);
+ mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
+ Assertions.assertThrows(BadRequestException.class, builder::build);
+ }
+
+ @Test
+ public void testAuthenticationNormal() throws Exception {
+ OAuth2TokenProvider.Builder builder
+ = DefaultOAuth2TokenProvider.builder()
+ .withUri(String.format("http://127.0.0.1:%d";, PORT))
+ .withCredential("yy:xx")
+ .withPath("oauth/token")
+ .withScope("test");
+
+ ObjectMapper objectMapper = ObjectMapperProvider.objectMapper();
+ HttpResponse mockResponse =
HttpResponse.response().withStatusCode(HttpStatus.SC_OK);
+ OAuth2TokenResponse response = new OAuth2TokenResponse("1", "2", "3",
1, "test", null);
+ String respJson = objectMapper.writeValueAsString(response);
+ mockResponse = mockResponse.withBody(respJson);
+ mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
+ Assertions.assertThrows(IllegalArgumentException.class,
builder::build);
+ response = new OAuth2TokenResponse("1", "2", "bearer", 1, "test",
null);
+ respJson = objectMapper.writeValueAsString(response);
+ mockResponse = mockResponse.withBody(respJson);
+ mockServer.when(any(), Times.exactly(2)).respond(mockResponse);
+ OAuth2TokenProvider provider = builder.build();
+ Assertions.assertTrue(provider.hasTokenData());
+ Assertions.assertNotNull(provider.getTokenData());
+ KeyPair keyPair = Keys.keyPairFor(SignatureAlgorithm.RS256);
+ String oldAccessToken
+ = Jwts.builder()
+ .setSubject("gravitino")
+ .setExpiration(new Date(System.currentTimeMillis() -
5))
+ .setAudience("service1")
+ .signWith(keyPair.getPrivate(),
SignatureAlgorithm.RS256)
+ .compact();
+
+ response = new OAuth2TokenResponse(oldAccessToken, "2", "bearer", 1,
"test", null);
+ respJson = objectMapper.writeValueAsString(response);
+ mockResponse = mockResponse.withBody(respJson);
+ mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
+ provider = builder.build();
+ String accessToken
+ = Jwts.builder()
+ .setSubject("gravitino")
+ .setExpiration(new Date(System.currentTimeMillis() +
10000))
+ .setAudience("service1")
+ .signWith(keyPair.getPrivate(),
SignatureAlgorithm.RS256)
+ .compact();
+
+ response = new OAuth2TokenResponse(accessToken, "2", "bearer", 1,
"test", null);
+ respJson =
ObjectMapperProvider.objectMapper().writeValueAsString(response);
+ mockResponse = mockResponse.withBody(respJson);
+ mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
+ Assertions.assertNotEquals(accessToken, oldAccessToken);
+ Assertions.assertEquals(
+ AuthConstants.AUTHORIZATION_BEARER_HEADER + accessToken,
+ new String(provider.getTokenData(), StandardCharsets.UTF_8));
+ }
+
+ @Test
+ public void testTokenNotFetchedWhenValid() throws Exception {
+ OAuth2TokenProvider.Builder builder
+ = DefaultOAuth2TokenProvider.builder()
+ .withUri(String.format("http://127.0.0.1:%d";, PORT))
+ .withCredential("yy:xx")
+ .withPath("oauth/token")
+ .withScope("test");
+ HttpResponse mockResponse =
HttpResponse.response().withStatusCode(HttpStatus.SC_OK);
+ ObjectMapper objectMapper = ObjectMapperProvider.objectMapper();
+ KeyPair keyPair = Keys.keyPairFor(SignatureAlgorithm.RS256);
+ String accessToken
+ = Jwts.builder()
+ .setSubject("gravitino")
+ .setExpiration(new Date(System.currentTimeMillis() +
10000))
+ .setAudience("service1")
+ .signWith(keyPair.getPrivate(),
SignatureAlgorithm.RS256)
+ .compact();
+
+ OAuth2TokenResponse response
+ = new OAuth2TokenResponse(accessToken, "2", "bearer", 1,
"test", null);
+ String respJson = objectMapper.writeValueAsString(response);
+ mockResponse = mockResponse.withBody(respJson);
+ mockServer.when(any(), Times.exactly(1)).respond(mockResponse);
+ OAuth2TokenProvider provider = builder.build();
+ String token = provider.getAccessToken();
+ Assertions.assertEquals(accessToken, token);
+ 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
+ = "Basic "
+ + Base64.getEncoder()
+ .encodeToString(
+ "clientId:clientSecret"
+ .getBytes(StandardCharsets.UTF_8));
+
+ HttpResponse mockResponse
+ = HttpResponse.response()
+ .withStatusCode(HttpStatus.SC_OK)
+ .withBody(respJson);
+
+ mockServer
+ .when(
+ HttpRequest.request()
+ .withMethod("POST")
+ .withPath("/oauth/token")
+ .withHeader("Authorization", basicAuth),
Review Comment:
Good that the `Authorization` header is matched. Please also assert the form
body does **not** contain `client_id` / `client_secret` for
`client_secret_basic` (that is the main behavioral difference from
`client_secret_post`).
Also consider a negative test: basic auth + secret-only credential should
fail fast.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/DefaultOAuth2TokenProvider.java:
##########
@@ -21,100 +21,124 @@
import com.google.common.base.Preconditions;
import java.util.Collections;
import org.apache.commons.lang3.StringUtils;
-
-/** This class is the default implement of OAuth2TokenProvider. */
+import org.apache.gravitino.client.OAuth2ClientAuthenticationMethod;
+/**
+ * This class is the default implement of OAuth2TokenProvider.
+ */
public class DefaultOAuth2TokenProvider extends OAuth2TokenProvider {
private String credential;
private String scope;
private String path;
private String token;
+ private OAuth2ClientAuthenticationMethod authenticationMethod =
+ OAuth2ClientAuthenticationMethod.CLIENT_SECRET_POST;;
Review Comment:
Nit / style: double semicolon `;;`.
More importantly, this file (and `OAuth2ClientUtil` / the test class) looks
largely reformatted (indentation/layout) beyond the functional change. Please
revert unrelated formatting so Spotless/Google Java Style stays clean and the
review diff stays focused.
##########
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");
Review Comment:
Please use a blank-check, not only `!= null`.
`parseCredential` can return an empty client id (e.g. credential `":secret"`
after trim), which would currently pass and produce a malformed Basic header.
Suggest: `StringUtils.isNotBlank(credentialPair.getLeft())`.
##########
clients/client-python/gravitino/auth/oauth2_client_authentication_method.py:
##########
@@ -0,0 +1,6 @@
+from enum import Enum
+
+
+class OAuth2ClientAuthenticationMethod(Enum):
Review Comment:
Please add the standard Apache license header used by sibling modules (e.g.
`default_oauth2_token_provider.py`). Also add a trailing newline at EOF.
--
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]
