Repository navigation
fix(auth): honor custom OAuth M2M scopes without forcing all-apis - #493
Conversation
NewAuthenticatorWithScopes always appended all-apis, so service principals with scoped secrets (e.g. sql only) failed with access_denied. Non-empty scopes are now requested as given; all-apis remains the default when none are passed, so NewAuthenticator is unchanged. The kernel backend now forwards M2M scopes via set_oauth_scopes instead of rejecting custom scopes. Fixes #476 Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
![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 Nit
Looks good — a clean, least-privilege fix that honors custom OAuth M2M scopes instead of force-appending all-apis, with matching Thrift and kernel wiring. Verified the removed M2MScopesSupported has no remaining callers, the kernel header confirms set_oauth_scopes semantics (comma-separated, M2M default all-apis, applied after mode selection), and test imports stay used so there's no build break. Only a cosmetic nit: m2m.GetScopes's hostName parameter is now unused.
Other findings
- ⚪ Nit —
GetScopesno longer references itshostNameparameter — it's now dead. Keeping the signature is reasonable for API compatibility (it's exported and mirrorsoauth.GetScopes), so this is purely cosmetic; a_ = hostNameor a doc note that the arg is retained only for signature parity would make the intent explicit. No action required.
Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
vuanhphung
added
the
integration-test
|
Go integration tests triggered ( |
Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
github-actions
Bot
removed
the
integration-test
|
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: efa7c9d |
vuanhphung
added
the
integration-test
|
Go integration tests triggered ( |
2 participants
Description
NewAuthenticatorWithScopesalways appendedall-apis, so service principals whose OAuth secret is scoped (e.g.sqlonly) failed withaccess_denied: Scopes 'all-apis' are not assigned to the client. Non-empty scopes are now requested as given;all-apisremains the default when none are passed, soNewAuthenticatoris unchanged. This matches JDBC'sAuth_Scopebehavior (databricks/databricks-jdbc#1707).On the kernel backend, custom M2M scopes were rejected because
set_auth_m2mtakes no scopes. The bundled kernel already exposeskernel_session_config_set_oauth_scopes, which applies to client-secret M2M (databricks/databricks-sql-kernel#263), so the driver now forwards the scopes through it.Behavior change: callers passing custom scopes to
NewAuthenticatorWithScopesno longer getall-apisappended.Fixes #476.
Testing
go vet ./...;go test ./auth/... ./internal/config/... ./internal/backend/... .CGO_ENABLED=1 go test -tags databricks_kernel ./internal/backend/kernel/ .(includes a newTestSetAuthByModecase calling the real scopes setter)sql-scoped secret.This pull request and its description were written by Isaac.
This PR was created with GitHub MCP.