Feature/957 more auth mechanisms#1453
Open
ghjklw wants to merge 7 commits into
Open
Conversation
TestAuthDispatch.test_config_kwargs: 7 parametrized cases covering every dispatch path in the legacy code, asserting the exact kwargs passed to databricks.sdk.core.Config. The 3 external-browser cases (oauth alias, oauth with custom client_id, no credentials) will fail after the refactor — that failure is the signal showing the behavioral delta. TestValidateCreds: 6 cases covering validation rules on main, all unchanged by the refactor (host/http_path required, token and oauth accepted, client_id required when client_secret present, azure fields must be paired). All 13 cases pass against origin/main. https://claude.ai/code/session_0164JzVFw4g7yH2Z6UHC5i9p
Replace DatabricksCredentialManager's 4-branch dispatch (token → Azure SP → no-client-secret external-browser → heuristic) with a two-branch model: - Legacy heuristic (client_secret present, no auth_type, no azure_client_secret) preserved unchanged for backward compatibility - Everything else routed through to_sdk_config_kwargs(), which handles auth_type passthrough, field translation, and databricks_sdk_parameters Fixes gaps in the original dispatch: - Explicit auth_type values (azure-cli, azure-msi, databricks-cli, etc.) now forwarded to the SDK instead of silently falling back to external-browser - auth_type="oauth" alias cleanly maps to "external-browser" without leaking an empty client_secret into Config - client_id alone infers external-browser without requiring explicit auth_type - No-credential path preserves legacy external-browser fallback (CLIENT_ID used) - token + auth_type both forwarded (auth_type was previously dropped when token present) - explicit auth_type wins over azure_client_id/secret field inference - azure_tenant_id forwarded in the Azure SP path - databricks_sdk_parameters merged for all auth paths - validate_creds relaxed to accept any SDK auth_type, not just token or "oauth" 2 baseline tests now fail, marking exactly what changed in the external-browser path. https://claude.ai/code/session_0164JzVFw4g7yH2Z6UHC5i9p
Fix the 2 baseline tests that exposed behavioral changes in the refactor: - oauth_alias / oauth_alias_with_custom_client_id: remove client_secret="" that legacy leaked into Config for external-browser auth - no_credentials: external-browser fallback preserved; update expected value to drop the empty client_secret="" that legacy passed unnecessarily Add 16 new cases for behaviors enabled by the refactor: - client_id alone now infers external-browser (no explicit auth_type needed) - all SDK auth_type values forwarded: azure-cli, azure-msi (+ user-assigned identity and tenant variants), databricks-cli (+ profile), google-credentials, metadata-service - explicit auth_type="oauth-m2m" bypasses the legacy heuristic - token + auth_type both forwarded (auth_type was previously dropped) - explicit auth_type wins over azure_client_id/secret field inference - azure_tenant_id now forwarded in the Azure SP path - databricks_sdk_parameters merged for all paths (azure-cli and PAT examples) Add 5 TestValidateCreds cases confirming that SDK auth_types no longer require a token or auth_type='oauth' to pass validation. https://claude.ai/code/session_0164JzVFw4g7yH2Z6UHC5i9p
oauth_scopes is now passed as 'scopes' through to_sdk_config_kwargs(), so it reaches the Databricks SDK Config for all auth paths that go through that method (external-browser, explicit auth_type passthrough, PAT, etc.). oauth_redirect_url is removed: the Databricks SDK hardcodes the redirect URL to http://localhost:8020 and never reads it from Config, so the field had no effect. REDIRECT_URL and SCOPES module-level constants are also removed as they are no longer referenced. Test coverage: two new TestAuthDispatch cases assert that oauth_scopes is forwarded as 'scopes' for external-browser (U2M) and oauth-m2m (M2M). https://claude.ai/code/session_0164JzVFw4g7yH2Z6UHC5i9p
…ty table - Emit a deprecation warning when client_secret is used without auth_type, directing users to set auth_type: oauth-m2m or azure-client-secret explicitly - Docstring notes that extra fields (azure_tenant_id, oauth_scopes, etc.) are intentionally not forwarded in the heuristic path to preserve legacy behaviour - README: fix priority table order (heuristic row last, correctly flagged as deprecated); document client_id-only → external-browser inference; add a deprecated callout block with migration examples https://claude.ai/code/session_0164JzVFw4g7yH2Z6UHC5i9p
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Closes databricks/dbt-databricks#957.
Problem: The adapter's authentication layer had two classes of issues:
azure-cli,azure-msi,databricks-cli,metadata-service, etc.) were silently blocked —validate_creds()required either a token or the legacyauth_type: oauthalias, and any otherauth_typevalue was ignored rather than forwarded to the SDK.oauth_scopesandoauth_redirect_url— behaved incorrectly (see OAuth bugs below).Solution: Auth dispatch is simplified to two branches: the existing heuristic (preserved for backward compatibility but now deprecated with a runtime warning) and a pass-through to the Databricks SDK
Configfor everything else. Users still on the heuristic path will see a deprecation warning directing them to setauth_typeexplicitly. This makes every auth method the SDK supports available to users without requiring changes to the adapter for each new method.Authentication type reference
The table below covers every profile configuration. Rows marked unchanged behave identically to the previous release; rows marked fixed or new indicate a behavioral change.
auth_typetokenazure_client_id+azure_client_secretazure-client-secret(inferred)azure_tenant_idnow forwarded (was silently dropped)client_id+client_secretstarting withdoseoauth-m2mfirst. Deprecated: setauth_type: oauth-m2mexplicitlyclient_id+client_secret(non-dose)azure-client-secretfirst. Deprecated: setauth_type: azure-client-secretexplicitlyoauthorexternal-browserclient_secret=""no longer leaked into SDK Configclient_idoauth/external-browserclient_secret=""no longer leakedclient_idaloneclient_id(noclient_secret)external-browser(inferred)client_idwithoutclient_secretnow selects browser-based OAuthazure-clivalidate_creds()and silently ignoredazure-msiazure_client_idazure-msidatabricks_cli_profile,config_file(optional)databricks-cligoogle_credentialsorgoogle_service_accountgoogle-credentialsmetadata_service_url(optional)metadata-serviceoidc_token_envoroidc_token_filepathusername+passworddatabricks_sdk_parameters: {key: value}Configkwargs, merged last, override everything aboveAdditional field-forwarding fixes (previously silently dropped)
auth_typewhentokenis also setauth_typeignored; only PAT usedazure_tenant_idwithazure_client_id/azure_client_secretConfigdatabricks_sdk_parametersConfigkwargs for all auth pathsauth_typewithazure_client_id/azure_client_secretauth_typeignored; forced toazure-client-secretauth_typewinsRemoved field
oauth_redirect_urlhttp://localhost:8020internally and never reads this value fromConfig; accepting it created a false impression of controlOAuth
scopes/redirect_urlbugsTwo fields were silently broken in the previous implementation:
oauth_scopeswas never forwarded to the SDKDatabricksCredentialManagerwas initialised with theoauth_scopesvalue and a default of["all-apis", "offline_access"]was applied. However, none of theConfig(...)calls —authenticate_with_external_browser(),authenticate_with_oauth_m2m(), or any other path — includedscopes=.... The field was accepted, stored, and then discarded. Users who setoauth_scopesin their profile to restrict or extend token scopes were silently ignored.Fix:
oauth_scopesis now passed asscopestoConfigfor all SDK-delegated auth paths.oauth_redirect_urlhad no effectREDIRECT_URL = "http://localhost:8020"was defined as a module constant and used as the default forDatabricksCredentialManager.oauth_redirect_url. The Databricks SDK'sConfigclass does not accept a redirect URL parameter — it hardcodeshttp://localhost:8020internally. Any user who configuredoauth_redirect_urlin their profile to point to a different port or host was silently ignored, and the browser OAuth flow continued to uselocalhost:8020regardless.Fix: The field is removed entirely to avoid the false impression that it is configurable.
Test strategy
This PR was developed test-first to make the behavioral delta between old and new explicit and reviewable:
Step 1 — Baseline documentation (
a31eec8): Before any code change, a parametrizedTestAuthDispatch.test_config_kwargssuite was written to document the exactConfig(...)kwargs produced by every legacy dispatch path (7 cases: PAT, Azure SP with dedicated fields, dose-prefix heuristic, non-dose heuristic,oauthalias,oauthwith customclient_id, and no credentials). ATestValidateCredssuite (6 cases) pinned validation rules. All 13 cases passed againstorigin/main, confirming the baseline was an accurate snapshot of existing behavior.Step 2 — Refactor applied (
e57d441): The dispatch was simplified. Exactly 2 of the 13 baseline tests began failing, making the behavioral delta explicit: both failing cases were the external-browser paths where an emptyclient_secret=""was previously leaked intoConfig. This made the scope of the change fully visible before any test was updated.Step 3 — Tests updated and new coverage added (
daff8f9): The 2 failing cases were updated to reflect the corrected behavior (no emptyclient_secret). 15 new parametrized cases were added covering: all newauth_typepassthroughs (azure-cli,azure-msi,databricks-cli,google-credentials,metadata-service), field-forwarding fixes (azure_tenant_id,databricks_sdk_parameters,token+auth_typecombined, explicitauth_typeoverriding field inference), and new profile fields. 5 newTestValidateCredscases confirm that previously-blocked SDKauth_typevalues now pass validation.Step 4 —
oauth_scopescoverage (eda9a78): 2 additional cases assert thatoauth_scopesis forwarded asscopesfor bothexternal-browser(U2M) andoauth-m2m(M2M) flows.Step 5 — Deprecation warning and README (
b5cf6b6): The legacy heuristic now emits alogger.warningat runtime directing users to setauth_typeexplicitly._resolve_client_secret_heuristicis documented as deprecated and notes that extra fields (e.g.azure_tenant_id,oauth_scopes) are intentionally not forwarded in the heuristic path to preserve identical legacy behaviour.README.mdis updated: priority table order corrected (heuristic rows last, flagged as deprecated),client_id-only browser OAuth inference documented, and a migration callout block added.Final state: 24 parametrized dispatch cases + 11 validation cases, all passing. No functional tests required changes — the connector-level behavior (how connections are opened against Databricks) is unchanged; only the
Configkwargs constructed from profile fields change.Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.