Repository navigation
Honor Auth_Scope for OAuth M2M client credentials - #1707
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-targeted change that honors Auth_Scope for client-secret M2M while preserving the SDK's all-apis default (verified: getAuthScope() returns null when unset, mutable ArrayList wrap matches the documented SDK sortScopes caveat, and both default/configured paths are tested). One low-severity edge case around an empty Auth_Scope value noted inline.
There was a problem hiding this comment.
Verdict: 1 Nit
Looks good — honors Auth_Scope for M2M client-secret auth with correct null/blank handling that preserves the SDK's all-apis default. Uses a mutable ArrayList (consistent with the U2M sortScopes() note), is covered by three new tests, and includes changelog + README updates. One nit in the summary: scope is handled as a single value (no multi-scope split), which is consistent with the existing U2M path.
Other findings
- ⚪ Nit —
Auth_Scopeis treated as a single opaque scope string (List.of(authScope.strip())), so a space- or comma-separated value likesql offline_accessbecomes one malformed scope rather than two. This mirrors the existinggetOAuthScopesForU2M()behavior, so it's consistent — but if multi-scope M2M is ever expected, this would need splitting. Flagging only for awareness; no change required for this PR's stated goal.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a focused, well-tested fix that honors Auth_Scope for the standard OAuth M2M client-secret flow. The null/blank guard, whitespace splitting, and mutable-list wrapper (to avoid the SDK's in-place sortScopes()) are all correct, and the default all-apis behavior is preserved and covered by tests. One low-severity note on parsing inconsistency between the M2M and U2M scope handling.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a focused, well-tested change that correctly parses Auth_Scope into space-separated scopes for client-secret M2M and U2M, preserving the all-apis/sql offline_access defaults on blank input, and wraps lists in mutable ArrayList before setScopes (avoiding the known SDK sortScopes immutability issue). The updated U2M Javadoc now matches the actual all-clouds default behavior. One low-severity consistency note: the JWT-assertion M2M path still uses singletonList(getAuthScope()) and does not benefit from the new splitting.
|
JDBC integration tests triggered ( |
|
many thanks for taking this up so quickly @vuanhphung |
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
ff72a6a to
71292c0
Compare
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 71292c0 |
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-tested fix that unifies Auth_Scope parsing across client-secret M2M, JWT-assertion M2M, and U2M. Scopes are correctly wrapped in mutable ArrayLists where the SDK sorts in place, blank values fall back to the right defaults per flow, and the stale Azure-null Javadoc is corrected to match code that already returned uniform defaults for all clouds. One low-severity test-coverage note (F1) on verifying the client-secret M2M scope actually reaches the token request.
| + "OAuth2ClientId=client-id;OAuth2Secret=client-secret;Auth_Scope= sql "; | ||
| IDatabricksConnectionContext connectionContext = | ||
| DatabricksConnectionContextFactory.create(jdbcUrl, new Properties()); | ||
| assertEquals(" sql ", connectionContext.getAuthScope()); |
There was a problem hiding this comment.
🔵 Low — The new client-secret M2M scope tests assert only databricksConfig.getScopes() (the config field), not that the scope actually reaches the OAuth token request. The JWT-assertion path tests (assertTokenRequestScope) go further and capture the outbound HTTP request's scope parameter via URLEncodedUtils. For client-secret M2M the driver relies on the SDK's OAuthM2MServicePrincipalCredentialsProvider to translate config.setScopes(...) into the token request — which this PR does not exercise (the author notes a live scoped-secret connection was not tested). Consider adding an end-to-end-style assertion that the configured scope is sent on the wire for the client-secret path too, to guard against the SDK silently ignoring the field.
3 participants
Description
Honor
Auth_Scopefor Databricks OAuth client-secret M2M so scoped service-principal secrets can authenticate. Client-secret M2M, JWT-assertion M2M, and U2M now parse one space-separated property value consistently. Blank values useall-apisfor client-secret M2M andsql offline_accessfor U2M; JWT-assertion M2M omits the scope parameter. Fixes #1706.Testing
mvn -q test -pl jdbc-core -Dtest=ClientConfiguratorTest,DatabricksConnectionContextTest(190 passed).mvn -q test -pl jdbc-core -Dtest=PrivateKeyClientCredentialProviderTest,JwtPrivateKeyClientCredentialsTest,ClientConfiguratorTest(61 passed).Telemetry Errors
Additional Notes to the Reviewer
A live connection with a
sql-scoped secret was not exercised.This PR was created with GitHub MCP.