From 2c74c034a61184afd26ef4de5ed850c0ee3b9326 Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Wed, 24 Sep 2025 16:10:10 +0000 Subject: [PATCH 1/6] Properly escape --- .../sdk/core/oauth/OAuthClient.java | 26 ++- .../sdk/core/oauth/OAuthClientTest.java | 157 ++++++++++++++++++ 2 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java index 38c61b6ac..22635bfeb 100644 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java @@ -4,7 +4,9 @@ import com.databricks.sdk.core.DatabricksException; import com.databricks.sdk.core.http.HttpClient; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.net.MalformedURLException; +import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; @@ -188,12 +190,30 @@ private static byte[] sha256(byte[] input) { } } - private static String urlEncode(String urlBase, Map params) { + protected static String urlEncode(String urlBase, Map params) { + if (params.isEmpty()) { + return urlBase; + } + String queryParams = params.entrySet().stream() - .map(entry -> entry.getKey() + "=" + entry.getValue()) + .sorted(Map.Entry.comparingByKey()) + .map(entry -> encodeParam(entry.getKey()) + "=" + encodeParam(entry.getValue())) .collect(Collectors.joining("&")); - return urlBase + "?" + queryParams.replaceAll(" ", "%20"); + + String separator = urlBase.contains("?") ? "&" : "?"; + return urlBase + separator + queryParams; + } + + private static String encodeParam(String value) { + try { + return URLEncoder.encode(value, "UTF-8"); + } catch (UnsupportedEncodingException e) { + // This should never happen. Though, the exception is a "checked" exception + // so we need to catch it here so that we do not have to propagate it to + // the method signature. + throw new RuntimeException("UTF-8 encoding not supported", e); + } } public Consent initiateConsent() throws MalformedURLException { diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java new file mode 100644 index 000000000..c529445c0 --- /dev/null +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java @@ -0,0 +1,157 @@ +package com.databricks.sdk.core.oauth; + +import static org.junit.jupiter.api.Assertions.*; + +import com.google.auto.value.AutoValue; +import java.util.HashMap; +import java.util.Map; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +public class OAuthClientTest { + + @AutoValue + public abstract static class UrlEncodeTestCase { + public abstract String description(); + + public abstract String baseUrl(); + + public abstract Map params(); + + public abstract String expectedResult(); + + public static UrlEncodeTestCase create( + String description, String baseUrl, Map params, String expectedResult) { + return new AutoValue_OAuthClientTest_UrlEncodeTestCase( + description, baseUrl, params, expectedResult); + } + } + + private static Stream urlEncodeTestCases() { + return Stream.of( + UrlEncodeTestCase.create( + "Basic parameters", + "https://example.com/auth", + createParams("client_id", "test-client", "response_type", "code"), + "https://example.com/auth?client_id=test-client&response_type=code"), + UrlEncodeTestCase.create( + "Empty parameters", + "https://example.com/auth", + createParams(), + "https://example.com/auth"), + UrlEncodeTestCase.create( + "Special characters in parameters", + "https://example.com/auth", + createParams( + "redirect_uri", + "http://localhost:8080/callback?extra=value", + "scope", + "read write", + "state", + "test&value=123"), + "https://example.com/auth?redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fcallback%3Fextra%3Dvalue&scope=read+write&state=test%26value%3D123"), + UrlEncodeTestCase.create( + "URL with existing query parameters", + "https://example.com/auth?existing=param", + createParams("client_id", "test-client"), + "https://example.com/auth?existing=param&client_id=test-client"), + UrlEncodeTestCase.create( + "Complex OAuth parameters", + "https://accounts.cloud.databricks.com/oidc/v1/authorize", + createParams( + "client_id", + "databricks-client", + "code_challenge", + "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + "code_challenge_method", + "S256", + "redirect_uri", + "https://app.example.com/callback?session=abc123", + "response_type", + "code", + "scope", + "sql clusters repos", + "state", + "random-state-123"), + "https://accounts.cloud.databricks.com/oidc/v1/authorize?client_id=databricks-client&code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM&code_challenge_method=S256&redirect_uri=https%3A%2F%2Fapp.example.com%2Fcallback%3Fsession%3Dabc123&response_type=code&scope=sql+clusters+repos&state=random-state-123"), + UrlEncodeTestCase.create( + "Parameter encoding with spaces", + "https://example.com", + createParams("scope", "read write admin"), + "https://example.com?scope=read+write+admin"), + UrlEncodeTestCase.create( + "Parameter encoding with special characters", + "https://example.com", + createParams("state", "value&with=special?chars#fragment"), + "https://example.com?state=value%26with%3Dspecial%3Fchars%23fragment"), + UrlEncodeTestCase.create( + "Parameter encoding with URL as value", + "https://example.com", + createParams("redirect_uri", "https://example.com/callback?param=value"), + "https://example.com?redirect_uri=https%3A%2F%2Fexample.com%2Fcallback%3Fparam%3Dvalue"), + UrlEncodeTestCase.create( + "Parameter encoding with plus signs", + "https://example.com", + createParams("scope", "scope+with+plus"), + "https://example.com?scope=scope%2Bwith%2Bplus"), + UrlEncodeTestCase.create( + "Parameter encoding with empty string", + "https://example.com", + createParams("empty", ""), + "https://example.com?empty="), + UrlEncodeTestCase.create( + "Parameter encoding with Unicode characters", + "https://example.com", + createParams("unicode", "测试数据"), + "https://example.com?unicode=%E6%B5%8B%E8%AF%95%E6%95%B0%E6%8D%AE"), + UrlEncodeTestCase.create( + "Integration OAuth flow with complex parameters", + "https://accounts.cloud.databricks.com/oidc/v1/authorize", + createParams( + "redirect_uri", + "https://app.example.com/oauth/callback?session=abc123&return=/dashboard", + "scope", + "sql clusters repos notebooks", + "state", + "test&state=value"), + "https://accounts.cloud.databricks.com/oidc/v1/authorize?redirect_uri=https%3A%2F%2Fapp.example.com%2Foauth%2Fcallback%3Fsession%3Dabc123%26return%3D%2Fdashboard&scope=sql+clusters+repos+notebooks&state=test%26state%3Dvalue")); + } + + private static Map createParams(String... keyValuePairs) { + Map params = new HashMap<>(); + for (int i = 0; i < keyValuePairs.length; i += 2) { + params.put(keyValuePairs[i], keyValuePairs[i + 1]); + } + return params; + } + + @ParameterizedTest + @MethodSource("urlEncodeTestCases") + public void testUrlEncode(UrlEncodeTestCase testCase) { + String result = OAuthClient.urlEncode(testCase.baseUrl(), testCase.params()); + assertEquals(testCase.expectedResult(), result, testCase.description()); + } + + @Test + public void testDeterministicParameterOrdering() { + // Test that parameters are always in the same order regardless of insertion order + Map params1 = new HashMap<>(); + params1.put("z_param", "value1"); + params1.put("a_param", "value2"); + params1.put("m_param", "value3"); + + Map params2 = new HashMap<>(); + params2.put("m_param", "value3"); + params2.put("z_param", "value1"); + params2.put("a_param", "value2"); + + String result1 = OAuthClient.urlEncode("https://example.com", params1); + String result2 = OAuthClient.urlEncode("https://example.com", params2); + + // Both should produce identical results (sorted by key) + assertEquals(result1, result2); + assertEquals("https://example.com?a_param=value2&m_param=value3&z_param=value1", result1); + } +} From bf671fe93cf128fe83942fe0445e94f482ff0d28 Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Wed, 24 Sep 2025 16:13:42 +0000 Subject: [PATCH 2/6] Minor comment change --- .../java/com/databricks/sdk/core/oauth/OAuthClient.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java index 22635bfeb..0a3e84c10 100644 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java @@ -209,9 +209,9 @@ private static String encodeParam(String value) { try { return URLEncoder.encode(value, "UTF-8"); } catch (UnsupportedEncodingException e) { - // This should never happen. Though, the exception is a "checked" exception - // so we need to catch it here so that we do not have to propagate it to - // the method signature. + // This should never happen. The exception is catched because it is + // a "checked" exception that we do not want to propagate to the method + // signature. throw new RuntimeException("UTF-8 encoding not supported", e); } } From b5c085e7c190cc1badd3731b3c524c681be5a63d Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Wed, 24 Sep 2025 16:18:39 +0000 Subject: [PATCH 3/6] Changelogs + fmt --- NEXT_CHANGELOG.md | 2 ++ .../main/java/com/databricks/sdk/core/oauth/OAuthClient.java | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 5c19ecb8b..ca0059f9c 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -6,6 +6,8 @@ ### Bug Fixes +* Fix OAuthClient to properly encode complex query parameters. + ### Documentation ### Internal Changes diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java index 0a3e84c10..767b8dfc1 100644 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java @@ -210,7 +210,7 @@ private static String encodeParam(String value) { return URLEncoder.encode(value, "UTF-8"); } catch (UnsupportedEncodingException e) { // This should never happen. The exception is catched because it is - // a "checked" exception that we do not want to propagate to the method + // a "checked" exception that we do not want to propagate to the method // signature. throw new RuntimeException("UTF-8 encoding not supported", e); } From 7e4f0c1b820f3f6fea3c0955142d8957c1efae5c Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Wed, 24 Sep 2025 16:22:31 +0000 Subject: [PATCH 4/6] Fix comments --- .../java/com/databricks/sdk/core/oauth/OAuthClientTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java index c529445c0..35417ab48 100644 --- a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/OAuthClientTest.java @@ -136,7 +136,7 @@ public void testUrlEncode(UrlEncodeTestCase testCase) { @Test public void testDeterministicParameterOrdering() { - // Test that parameters are always in the same order regardless of insertion order + // Test that parameters are always in the same order regardless of insertion order. Map params1 = new HashMap<>(); params1.put("z_param", "value1"); params1.put("a_param", "value2"); @@ -150,7 +150,7 @@ public void testDeterministicParameterOrdering() { String result1 = OAuthClient.urlEncode("https://example.com", params1); String result2 = OAuthClient.urlEncode("https://example.com", params2); - // Both should produce identical results (sorted by key) + // Both should produce identical results (sorted by key). assertEquals(result1, result2); assertEquals("https://example.com?a_param=value2&m_param=value3&z_param=value1", result1); } From c9cb608480365875a47a6549d1f79ade44aa395f Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Wed, 24 Sep 2025 18:41:07 +0000 Subject: [PATCH 5/6] Format --- .../java/com/databricks/sdk/core/oauth/OAuthClient.java | 7 ++++++- .../oauth/ExternalBrowserCredentialsProviderTest.java | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java index 767b8dfc1..9af534703 100644 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/OAuthClient.java @@ -191,7 +191,7 @@ private static byte[] sha256(byte[] input) { } protected static String urlEncode(String urlBase, Map params) { - if (params.isEmpty()) { + if (params == null || params.isEmpty()) { return urlBase; } @@ -217,6 +217,11 @@ private static String encodeParam(String value) { } public Consent initiateConsent() throws MalformedURLException { + if (authUrl == null) { + throw new DatabricksException( + "Authorization URL is not configured. OAuth endpoints may not be available."); + } + String state = tokenUrlSafe(16); String verifier = tokenUrlSafe(32); byte[] digest = sha256(verifier.getBytes(StandardCharsets.UTF_8)); diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java index c5c237349..31fc619e8 100644 --- a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java @@ -29,7 +29,9 @@ void clientAndConsentTest() throws IOException { new FixtureServer.FixtureMapping.Builder() .validateMethod("GET") .validatePath("/oidc/.well-known/oauth-authorization-server") - .withResponse("{\"token_endpoint\": \"tokenEndPointFromServer\"}", 200) + .withResponse( + "{\"token_endpoint\": \"tokenEndPointFromServer\", \"authorization_endpoint\": \"authEndPointFromServer\"}", + 200) .build(); try (FixtureServer fixtures = new FixtureServer()) { fixtures.with(fixture).with(fixture); @@ -73,7 +75,9 @@ void clientAndConsentTestWithCustomRedirectUrl() throws IOException { new FixtureServer.FixtureMapping.Builder() .validateMethod("GET") .validatePath("/oidc/.well-known/oauth-authorization-server") - .withResponse("{\"token_endpoint\": \"tokenEndPointFromServer\"}", 200) + .withResponse( + "{\"token_endpoint\": \"tokenEndPointFromServer\", \"authorization_endpoint\": \"authEndPointFromServer\"}", + 200) .build(); try (FixtureServer fixtures = new FixtureServer()) { fixtures.with(fixture).with(fixture); From 05b56838e7c2c4de4d224523f1ed9d2accf5d133 Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Wed, 24 Sep 2025 19:12:15 +0000 Subject: [PATCH 6/6] Fix tests --- .../core/oauth/ExternalBrowserCredentialsProviderTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java index 31fc619e8..70981a54b 100644 --- a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/oauth/ExternalBrowserCredentialsProviderTest.java @@ -64,7 +64,7 @@ void clientAndConsentTest() throws IOException { assertNotNull(authUrl); assertTrue(authUrl.contains("response_type=code")); assertTrue(authUrl.contains("client_id=test-client-id")); - assertTrue(authUrl.contains("redirect_uri=http://localhost:8080/callback")); + assertTrue(authUrl.contains("redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fcallback")); assertTrue(authUrl.contains("scope=all-apis")); } } @@ -112,7 +112,7 @@ void clientAndConsentTestWithCustomRedirectUrl() throws IOException { assertNotNull(authUrl); assertTrue(authUrl.contains("response_type=code")); assertTrue(authUrl.contains("client_id=test-client-id")); - assertTrue(authUrl.contains("redirect_uri=http://localhost:8010")); + assertTrue(authUrl.contains("redirect_uri=http%3A%2F%2Flocalhost%3A8010")); assertTrue(authUrl.contains("scope=sql")); } }