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 38c61b6ac..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 @@ -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,15 +190,38 @@ private static byte[] sha256(byte[] input) { } } - private static String urlEncode(String urlBase, Map params) { + protected static String urlEncode(String urlBase, Map params) { + if (params == null || 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. 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); + } } 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..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 @@ -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); @@ -62,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")); } } @@ -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); @@ -108,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")); } } 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..35417ab48 --- /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); + } +}