This is an automated email from the ASF dual-hosted git repository.
jmclean pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new b434291f00 fix(cli): Fix OAuth/Kerberos authentication detection in
GravitinoConfig (#8187)
b434291f00 is described below
commit b434291f00038b153f43f9015851fafef7f11db6
Author: Mathieu Baurin <[email protected]>
AuthorDate: Thu Aug 21 00:16:46 2025 +0200
fix(cli): Fix OAuth/Kerberos authentication detection in GravitinoConfig
(#8187)
### What changes were proposed in this pull request ?
This PR fixes the OAuth/Kerberos authentication detection logic in
`GravitinoConfig.java`. The changes include:
1. **Fixed authentication type comparisons**: Replace
`authKey.equals("oauth")` with `"oauth".equals(authType)` and similar
for Kerberos
2. **Removed duplicate OAuth initialization**: Eliminated redundant
OAuth configuration block that was overwriting Kerberos settings
3. **Consolidated control flow**: Created clean if-else if structure for
proper authentication type handling
The root issue was that the code was comparing against the constant
`authKey` ("auth") instead of the parsed `authType` value from the
configuration file.
### Why are the changes needed?
The current implementation has critical bugs that prevent OAuth and
Kerberos authentication from working correctly:
1. **Wrong variable comparison**: Code used `authKey` (constant "auth")
instead of `authType` (parsed value like "oauth" or "kerberos")
2. **Configuration overwriting**: Duplicate OAuth logic was overwriting
any Kerberos configuration
3. **Unreachable code paths**: Kerberos authentication could never be
properly initialized
These bugs prevent the CLI from correctly detecting and configuring
authentication when `auth=oauth` or `auth=kerberos` is set in the
configuration file.
Fix: https://github.com/apache/gravitino/issues/8162
### Does this PR introduce _any_ user-facing change?
**No user-facing API changes**, but fixes critical authentication
functionality:
- When `auth=oauth` is configured, `config.getOAuth()` now correctly
returns valid OAuth configuration
- When `auth=kerberos` is configured, `config.getKerberos()` now
correctly returns valid Kerberos configuration
- No changes to configuration file format or property keys
### How was this patch tested?
### How was this patch tested?
1. **Code review**: Verified the logic fixes address all identified
issues
2. **Integration verification**: Confirmed the changes work with
existing `Command.java` usage patterns at lines 202-223
3. **Style compliance**: Ensured code follows Apache Gravitino
conventions using defensive null-safe comparisons
The existing unit tests in `TestGravitinoConfig.java` should pass
without modification
---
.../org/apache/gravitino/cli/GravitinoConfig.java | 13 +-
.../apache/gravitino/cli/TestGravitinoConfig.java | 219 +++++++++++++++++++++
2 files changed, 221 insertions(+), 11 deletions(-)
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoConfig.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoConfig.java
index 37f52814e0..8b0e843252 100644
--- a/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoConfig.java
+++ b/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoConfig.java
@@ -98,25 +98,16 @@ public class GravitinoConfig {
authType = prop.getProperty(authKey);
}
- if (authKey.equals("oauth")) {
+ if ("oauth".equals(authType)) {
oauth =
new OAuthData(
prop.getProperty("serverURI"),
prop.getProperty("credential"),
prop.getProperty("token"),
prop.getProperty("scope"));
- } else if (authKey.equals("kerberos")) {
+ } else if ("kerberos".equals(authType)) {
kerberos = new KerberosData(prop.getProperty("principal"),
prop.getProperty("keytabFile"));
}
-
- if (authKey.equals("oauth")) {
- oauth =
- new OAuthData(
- prop.getProperty("serverURI"),
- prop.getProperty("credential"),
- prop.getProperty("token"),
- prop.getProperty("scope"));
- }
}
/**
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestGravitinoConfig.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestGravitinoConfig.java
index b2c1ce7d5f..dfc58633f5 100644
---
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestGravitinoConfig.java
+++
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestGravitinoConfig.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.cli;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -45,6 +46,17 @@ public class TestGravitinoConfig {
private static final String URL_VALUE = "http://10.0.0.1:8090";;
private static final String IGNORE_KEY = "ignore";
private static final String IGNORE_VALUE = "true";
+ private static final String AUTH_KEY = "auth";
+ private static final String OAUTH_VALUE = "oauth";
+ private static final String KERBEROS_VALUE = "kerberos";
+ // OAuth test values
+ private static final String SERVER_URI_VALUE = "https://oauth.example.com";;
+ private static final String CREDENTIAL_VALUE = "test-credential";
+ private static final String TOKEN_VALUE = "test-token";
+ private static final String SCOPE_VALUE = "test-scope";
+ // Kerberos test values
+ private static final String PRINCIPAL_VALUE = "[email protected]";
+ private static final String KEYTAB_VALUE = "/path/to/test.keytab";
@BeforeEach
public void setUp() throws IOException {
@@ -184,4 +196,211 @@ public class TestGravitinoConfig {
assertNull(config.getGravitinoURL(), "URL should be null before reading
the config file");
assertFalse(config.getIgnore(), "Ignore should be null before reading the
config file");
}
+
+ @Test
+ public void oAuthConfiguration() throws IOException {
+ // Create a config file with OAuth settings
+ File tempFileWithOAuth =
+ new File(System.getProperty("java.io.tmpdir") +
"/oauth_config.properties");
+ Properties props = new Properties();
+ props.setProperty(AUTH_KEY, OAUTH_VALUE);
+ props.setProperty("serverURI", SERVER_URI_VALUE);
+ props.setProperty("credential", CREDENTIAL_VALUE);
+ props.setProperty("token", TOKEN_VALUE);
+ props.setProperty("scope", SCOPE_VALUE);
+
+ try (Writer writer =
+ Files.newBufferedWriter(tempFileWithOAuth.toPath(),
StandardCharsets.UTF_8)) {
+ props.store(writer, "OAuth Test Config");
+ }
+
+ GravitinoConfig config = new
GravitinoConfig(tempFileWithOAuth.getAbsolutePath());
+ config.read();
+
+ assertEquals(OAUTH_VALUE, config.getGravitinoAuthType(), "Auth type should
be oauth");
+ assertNotNull(config.getOAuth(), "OAuth configuration should not be null");
+ assertNull(config.getKerberos(), "Kerberos configuration should be null
for OAuth");
+
+ OAuthData oauth = config.getOAuth();
+ assertEquals(SERVER_URI_VALUE, oauth.getServerURI(), "OAuth server URI
should match");
+ assertEquals(CREDENTIAL_VALUE, oauth.getCredential(), "OAuth credential
should match");
+ assertEquals(TOKEN_VALUE, oauth.getToken(), "OAuth token should match");
+ assertEquals(SCOPE_VALUE, oauth.getScope(), "OAuth scope should match");
+
+ tempFileWithOAuth.delete();
+ }
+
+ @Test
+ public void oAuthConfigurationPartialProperties() throws IOException {
+ // Create a config file with only some OAuth properties
+ File tempFilePartialOAuth =
+ new File(System.getProperty("java.io.tmpdir") +
"/partial_oauth_config.properties");
+ Properties props = new Properties();
+ props.setProperty(AUTH_KEY, OAUTH_VALUE);
+ props.setProperty("serverURI", SERVER_URI_VALUE);
+ props.setProperty("credential", CREDENTIAL_VALUE);
+ // Missing token and scope
+
+ try (Writer writer =
+ Files.newBufferedWriter(tempFilePartialOAuth.toPath(),
StandardCharsets.UTF_8)) {
+ props.store(writer, "Partial OAuth Test Config");
+ }
+
+ GravitinoConfig config = new
GravitinoConfig(tempFilePartialOAuth.getAbsolutePath());
+ config.read();
+
+ assertEquals(OAUTH_VALUE, config.getGravitinoAuthType(), "Auth type should
be oauth");
+ assertNotNull(config.getOAuth(), "OAuth configuration should not be null");
+
+ OAuthData oauth = config.getOAuth();
+ assertEquals(SERVER_URI_VALUE, oauth.getServerURI(), "OAuth server URI
should match");
+ assertEquals(CREDENTIAL_VALUE, oauth.getCredential(), "OAuth credential
should match");
+ assertNull(oauth.getToken(), "OAuth token should be null when not
provided");
+ assertNull(oauth.getScope(), "OAuth scope should be null when not
provided");
+
+ tempFilePartialOAuth.delete();
+ }
+
+ @Test
+ public void getOAuthWithoutAuthType() throws IOException {
+ // Create a config file without auth type set to oauth
+ File tempFileWithoutOAuth =
+ new File(System.getProperty("java.io.tmpdir") +
"/no_oauth_config.properties");
+ Properties props = new Properties();
+ props.setProperty(METALAKE_KEY, METALAKE_VALUE);
+ // No auth property set
+
+ try (Writer writer =
+ Files.newBufferedWriter(tempFileWithoutOAuth.toPath(),
StandardCharsets.UTF_8)) {
+ props.store(writer, "No OAuth Test Config");
+ }
+
+ GravitinoConfig config = new
GravitinoConfig(tempFileWithoutOAuth.getAbsolutePath());
+ config.read();
+
+ assertNull(config.getGravitinoAuthType(), "Auth type should be null when
not configured");
+ assertNull(config.getOAuth(), "OAuth configuration should be null when
auth type is not oauth");
+ assertNull(
+ config.getKerberos(),
+ "Kerberos configuration should be null when auth type is not
kerberos");
+
+ tempFileWithoutOAuth.delete();
+ }
+
+ @Test
+ public void kerberosConfiguration() throws IOException {
+ // Create a config file with Kerberos settings
+ File tempFileWithKerberos =
+ new File(System.getProperty("java.io.tmpdir") +
"/kerberos_config.properties");
+ Properties props = new Properties();
+ props.setProperty(AUTH_KEY, KERBEROS_VALUE);
+ props.setProperty("principal", PRINCIPAL_VALUE);
+ props.setProperty("keytabFile", KEYTAB_VALUE);
+
+ try (Writer writer =
+ Files.newBufferedWriter(tempFileWithKerberos.toPath(),
StandardCharsets.UTF_8)) {
+ props.store(writer, "Kerberos Test Config");
+ }
+
+ GravitinoConfig config = new
GravitinoConfig(tempFileWithKerberos.getAbsolutePath());
+ config.read();
+
+ assertEquals(KERBEROS_VALUE, config.getGravitinoAuthType(), "Auth type
should be kerberos");
+ assertNotNull(config.getKerberos(), "Kerberos configuration should not be
null");
+ assertNull(config.getOAuth(), "OAuth configuration should be null for
Kerberos");
+
+ KerberosData kerberos = config.getKerberos();
+ assertEquals(PRINCIPAL_VALUE, kerberos.getPrincipal(), "Kerberos principal
should match");
+ assertEquals(KEYTAB_VALUE, kerberos.getKeytabFile(), "Kerberos keytab file
should match");
+
+ tempFileWithKerberos.delete();
+ }
+
+ @Test
+ public void kerberosConfigurationPartialProperties() throws IOException {
+ // Create a config file with only principal (missing keytab)
+ File tempFilePartialKerberos =
+ new File(System.getProperty("java.io.tmpdir") +
"/partial_kerberos_config.properties");
+ Properties props = new Properties();
+ props.setProperty(AUTH_KEY, KERBEROS_VALUE);
+ props.setProperty("principal", PRINCIPAL_VALUE);
+ // Missing keytabFile
+
+ try (Writer writer =
+ Files.newBufferedWriter(tempFilePartialKerberos.toPath(),
StandardCharsets.UTF_8)) {
+ props.store(writer, "Partial Kerberos Test Config");
+ }
+
+ GravitinoConfig config = new
GravitinoConfig(tempFilePartialKerberos.getAbsolutePath());
+ config.read();
+
+ assertEquals(KERBEROS_VALUE, config.getGravitinoAuthType(), "Auth type
should be kerberos");
+ assertNotNull(config.getKerberos(), "Kerberos configuration should not be
null");
+
+ KerberosData kerberos = config.getKerberos();
+ assertEquals(PRINCIPAL_VALUE, kerberos.getPrincipal(), "Kerberos principal
should match");
+ assertNull(kerberos.getKeytabFile(), "Kerberos keytab should be null when
not provided");
+
+ tempFilePartialKerberos.delete();
+ }
+
+ @Test
+ public void getKerberosWithoutAuthType() throws IOException {
+ // Create a config file with auth type set to something other than kerberos
+ File tempFileWithoutKerberos =
+ new File(System.getProperty("java.io.tmpdir") +
"/no_kerberos_config.properties");
+ Properties props = new Properties();
+ props.setProperty(AUTH_KEY, "simple"); // Different auth type
+ props.setProperty(METALAKE_KEY, METALAKE_VALUE);
+
+ try (Writer writer =
+ Files.newBufferedWriter(tempFileWithoutKerberos.toPath(),
StandardCharsets.UTF_8)) {
+ props.store(writer, "No Kerberos Test Config");
+ }
+
+ GravitinoConfig config = new
GravitinoConfig(tempFileWithoutKerberos.getAbsolutePath());
+ config.read();
+
+ assertEquals("simple", config.getGravitinoAuthType(), "Auth type should be
simple");
+ assertNull(
+ config.getKerberos(),
+ "Kerberos configuration should be null when auth type is not
kerberos");
+ assertNull(config.getOAuth(), "OAuth configuration should be null when
auth type is not oauth");
+
+ tempFileWithoutKerberos.delete();
+ }
+
+ @Test
+ public void getGravitinoAuthType() throws IOException {
+ // Create a config file with auth type
+ File tempFileWithAuthType =
+ new File(System.getProperty("java.io.tmpdir") +
"/auth_type_config.properties");
+ Properties props = new Properties();
+ props.setProperty(AUTH_KEY, OAUTH_VALUE);
+ props.setProperty(METALAKE_KEY, METALAKE_VALUE);
+
+ try (Writer writer =
+ Files.newBufferedWriter(tempFileWithAuthType.toPath(),
StandardCharsets.UTF_8)) {
+ props.store(writer, "Auth Type Test Config");
+ }
+
+ GravitinoConfig config = new
GravitinoConfig(tempFileWithAuthType.getAbsolutePath());
+ config.read();
+
+ assertEquals(
+ OAUTH_VALUE, config.getGravitinoAuthType(), "Auth type should match
configured value");
+
+ tempFileWithAuthType.delete();
+ }
+
+ @Test
+ public void authTypeWithoutConfig() {
+ GravitinoConfig config = new GravitinoConfig(NON_EXISTENT_FILE_PATH);
+ config.read(); // Should handle missing file gracefully
+
+ assertNull(
+ config.getGravitinoAuthType(), "Auth type should be null when config
file doesn't exist");
+ assertNull(config.getOAuth(), "OAuth should be null when config file
doesn't exist");
+ assertNull(config.getKerberos(), "Kerberos should be null when config file
doesn't exist");
+ }
}
