Add discovery login via login.databricks.com#4702
Open
simonfaltum wants to merge 16 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: 5a21b82
16 interesting tests: 7 SKIP, 6 RECOVERED, 3 flaky
Top 21 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
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
3 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.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 testabilityTest 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)discoveryLoginwith introspection failure (verifies profile is still saved)discoveryLoginwith introspection success (verifies metadata extraction)go test ./cmd/auth/... ./libs/auth/...passes