Add discovery login via login.databricks.com#4702
Open
simonfaltum wants to merge 21 commits intomainfrom
Open
Conversation
Add IntrospectToken() which calls /api/2.0/tokens/introspect to extract account_id and workspace_id from workspace tokens. This will be used by the discovery login flow to enrich profiles with metadata. Note: go.mod contains a temporary replace directive pointing to the local SDK worktree with discovery changes. This will be reverted before the PR.
Extract shouldUseDiscovery() routing function and test it directly with table-driven tests instead of executing the full cobra command, which hangs waiting for an OAuth browser callback that never arrives in tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…essages Extract splitScopes helper to deduplicate scope parsing. Save scopes to profile in discoveryLogin (was previously dropped). Add actionable error messages with --host tip for setup failures and config file path for save failures. Make discoveryLogin testable by introducing package-level vars for PersistentAuth, OAuthArgument, and IntrospectToken. Fix typo in introspect test data. Use http.MethodGet constant and drain response body on error for TCP connection reuse. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
simonfaltum
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 5e91ce3
20 interesting tests: 9 SKIP, 7 KNOWN, 4 flaky
Top 18 slowest tests (at least 2 minutes):
|
Group same-type function parameters and use strconv.FormatInt instead of fmt.Sprintf for integer conversion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
- Extract repeated discovery fallback tip to discoveryFallbackTip const - Reuse splitScopes in token.go instead of inline split+trim (also fixes missing empty-entry filtering) - Rename TestDiscoveryLogin_IntrospectionSuccessExtractsMetadata to TestIntrospectToken_SuccessExtractsMetadata (it tests IntrospectToken directly, not discoveryLogin) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
The SDKs are not yet ready to handle account_id on workspace profiles as part of the cache key. Saving it now would break existing auth flows. Keeping the introspection call so workspace_id is still extracted. Co-authored-by: Isaac
…mismatch Reject --configure-cluster and --configure-serverless when discovery login is used (no host available), since these flags require a known workspace. Warn when the introspected account_id differs from the existing profile's account_id to surface potential misconfigurations early. Also use the discoveryFallbackTip constant instead of hardcoded strings. Co-authored-by: Isaac
Test that a warning is logged when the introspected account_id differs from the existing profile's account_id, and that no warning is emitted when they match. Co-authored-by: Isaac
- Remove TestIntrospectToken_ServerError (same code path as _HTTPError) - Merge TestIntrospectToken_VerifyAuthHeader and _VerifyEndpoint into one - Remove TestIntrospectToken_SuccessExtractsMetadata from login_test.go (duplicate of introspect_test.go:TestIntrospectToken_Success) Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Follow the codebase convention of using the context-aware env package for environment variable access. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
The existing non-discovery login path also used os.Getenv for DATABRICKS_CONFIG_FILE. Replaced with the context-aware env package to follow codebase conventions and enable test overrides. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
Note: This review was posted by Claude (AI assistant). Shreyas will do a separate, more thorough review pass.
Priority: MEDIUM — Well-structured with a few concerns
MEDIUM: http.DefaultClient in IntrospectToken bypasses SDK HTTP configuration
libs/auth/introspect.go uses http.DefaultClient, bypassing proxy, custom TLS, retry, and timeout settings configured in the SDK. In enterprise environments, this call will fail. Since introspection is best-effort, this is acceptable for v1 but should be tracked for improvement.
LOW: Missing discoveredHost validation
After arg.GetDiscoveredHost(), there's no check that the returned host is non-empty. This could save an empty host string to the profile.
LOW: Missing IsUnifiedHost in discovery profile save
The discovery path doesn't set Experimental_IsUnifiedHost nor clear experimental_is_unified_host, unlike the non-discovery path.
DESIGN: Verify introspection endpoint supports GET
The code uses http.MethodGet for /api/2.0/tokens/introspect. If this follows RFC 7662 (which expects POST), the call will silently fail with 405.
What looks good
- Clean
discoveryPersistentAuthnarrow interface shouldUseDiscoveryis pure and testable (6 table-driven test cases)splitScopesextraction reduces duplication- Best-effort introspection doesn't block login on failure
env.Get(ctx, ...)replacesos.Getenvcorrectly
After GetDiscoveredHost(), check that the returned host is not empty before saving it to the profile. An empty host would produce a broken profile entry. Return a clear error with the fallback tip so the user knows they can retry with --host.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
…ithout --host in discovery login These flags are meaningless in discovery mode (login.databricks.com) and silently ignored before this change. For example, 'databricks auth login --account-id <id>' would go through workspace discovery but getProfileName() would generate an ACCOUNT-<id> profile name for a workspace token. Now the CLI returns a clear error telling the user to provide --host when any of these flags are explicitly set.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Allow callers to pass a configured *http.Client so corporate proxy and custom CA certificate settings are respected. Falls back to http.DefaultClient when nil for backward compatibility. Co-authored-by: Isaac
Addresses four issues found in code review: 1. Pass a dedicated HTTP client (cloned transport) to IntrospectToken instead of nil, so proxy/TLS settings are inherited without sharing global state. 2. Reuse existing profile scopes during discovery relogin when --scopes is not explicitly set. Add regression tests for both fallback and override cases. 3. Add workspace_id to the clearKeys list so stale values from older hostless/unified profiles are removed. Fresh introspection values are re-written after clearing. Add regression tests for stale field cleanup and fresh workspace_id writes. 4. Replace string concatenation with discoveryFallbackTip with a discoveryErr helper for consistent, readable error construction. Co-authored-by: Isaac
Build the introspection HTTP client from a resolved SDK config.Config instead of cloning http.DefaultTransport. This picks up TLS settings (InsecureSkipVerify, custom transport) from environment variables, matching how the rest of the SDK handles HTTP connections. Add httpClientFromConfig helper with tests, and add a test verifying IntrospectToken uses the supplied HTTP client. Co-authored-by: Isaac
Replace httpClientFromConfig with httpClientForIntrospection that clones http.DefaultTransport. The previous approach tried to load TLS settings via EnsureResolved on a config with Host+Token already set, which short-circuits the config-file loader and never picks up profile TLS settings. The InsecureSkipVerify branch also created a zero-value transport missing proxy, dialer, and HTTP/2 settings. Since discovery login may not have a profile yet, the correct approach is to clone DefaultTransport which inherits system CA certs, proxy settings, timeouts, and connection pooling. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
reviewed
Mar 19, 2026
Contributor
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
A follow-up PR once this is merged, will merge the profile / new profile selector from auth token with auth login to ensure a uniform experience, and ensuring that users who already have profiles setup will be given the option to re-login as the primary option.
What do you mean by this? I suspect auth token already prompts with a profile list.
Member
Author
I mean, login will get the profile selector and "new profile" will get the discovery flow |
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
4 participants
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.
Why
When users run
databricks auth loginwithout specifying a host, the CLI had no fallback. Users had to know their workspace URL upfront. This adds a discovery flow via login.databricks.com where users authenticate in the browser, select a workspace, and the CLI automatically discovers the workspace URL.This PR adds discovery login as the default option. A follow-up PR once this is merged, will merge the profile / new profile selector from
auth tokenwithauth loginto ensure a uniform experience, and ensuring that users who already have profiles setup will be given the option to re-login as the primary option.Changes
Before:
databricks auth loginwithout--hostprompted for a host URL or failed.Now:
databricks auth loginwithout--hostopens login.databricks.com in the browser. After the user authenticates and selects a workspace, the CLI saves the profile with the discovered host, account ID, and workspace ID.Implementation details:
shouldUseDiscovery()detects when no host is available from flags, args, or existing profilediscoveryLogin()orchestrates the browser-based flow using the SDK's discovery OAuth supportIntrospectToken()(inlibs/auth) calls the workspace introspection endpoint to extract account/workspace IDs (best-effort, non-fatal on failure)splitScopes()deduplicates scope parsing across login pathsPersistentAuth,OAuthArgument,IntrospectToken) are injectable via package-level vars for testabilityPost-review fixes
IntrospectTokenaccepts an*http.Clientparameter instead of usinghttp.DefaultClient(enterprise proxy/TLS compatibility)http.DefaultTransportto inherit system CA certs, proxy settings, and timeouts--scopesis not explicitly set (consistent with normal login path)account_id,workspace_id,experimental_is_unified_host,cluster_id,serverless_compute_id) before saving discovery profileworkspace_idonly set when introspection returns a fresh valuediscoveryErr()helperTest plan
shouldUseDiscovery(table-driven, 6 cases)splitScopes(empty, single, whitespace, empty entries)IntrospectToken(success, zero workspace ID, HTTP errors, malformed JSON, auth header, endpoint path, custom HTTP client)discoveryLoginwith introspection failure (verifies profile is still saved)discoveryLoginwith introspection success (verifies metadata extraction)go test ./cmd/auth/... ./libs/auth/...passes