auth describe: try both verification endpoints before reporting failure#5512
Open
simonfaltum wants to merge 4 commits into
Open
auth describe: try both verification endpoints before reporting failure#5512simonfaltum wants to merge 4 commits into
simonfaltum wants to merge 4 commits into
Conversation
getAuthStatus made exactly one verification call based on the client type MustAnyClient picked (account client -> Workspaces.List, workspace client -> CurrentUser.Me) and reported "Unable to authenticate" when that call failed, even when the credentials were valid. Account console profiles that also carry a workspace_id resolve to a workspace client, and CurrentUser.Me always fails against the accounts host (#5479). If the first verification call fails, build the other client type from the same resolved config (non-interactively, over the same config pointer) and try its verification call before reporting failure. If both fail, report the first error. Success paths still make exactly one call. Co-authored-by: Isaac
A 403 response means the server authenticated the caller and refused the operation; invalid credentials produce a 401. When both verification endpoints fail but at least one failure is an HTTP 403 API error, report success instead of failure. This makes describe truthful for non-admin users on account hosts, where Workspaces.List is account-admin-only (returns 403) and the account console cannot serve CurrentUser.Me (returns 400), so both checks fail even though the credentials are perfectly valid. Username stays empty in this case; the output template omits it. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 5d3fa86
24 interesting tests: 15 SKIP, 7 KNOWN, 2 flaky
Top 28 slowest tests (at least 2 minutes):
|
A 403 does not only come from authz denials: network-level gates (IP access lists, private link) also answer 403, and on workspace hosts those can fire before the token is validated, so treating 403 as proof of authentication can report success for invalid credentials. Report the verification failure as-is instead. Non-admin account users are covered by the server-side list-workspaces authz rollout, which already allows non-admins on GCP and AWS public prod. This reverts commit 5d3fa86. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
mihaimitrea-db
approved these changes
Jun 10, 2026
| require.Equal(t, "error", status.Status) | ||
| assert.ErrorIs(t, status.Error, listErr) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
[nit] Could we somehow remove duplication from these tests? Maybe a table driven if it's easy to do?
Member
Author
There was a problem hiding this comment.
Done in a2473db: folded the five fallback tests into one table-driven test (TestGetAuthStatusVerificationFallback). A zero status in the test server now means "this endpoint must not be called", which absorbs the no-account-id case into the table. Note the file also shrank in the meantime: the 403-as-success commit was dropped from the PR, so the three tests for it are gone.
Review feedback: the five fallback tests repeated the same wiring. One table-driven test now covers both branches; a zero status in describeVerifyServer marks an endpoint that must not be called, which absorbs the no-account-id case. Co-authored-by: Isaac
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
databricks auth describemakes a single verification call based on the client type it resolved (workspace client:CurrentUser.Me, account client:Workspaces.List). If that one call fails, describe prints "Unable to authenticate" even when the credentials are perfectly valid. A profile with an accounts host plus bothaccount_idandworkspace_id(older logins wrote this shape) resolves to a workspace client. The token works, butCurrentUser.Meagainst the accounts host always fails with HTTP 400, so describe reports failure whiledatabricks auth tokensucceeds.Fixes #5479.
Changes
Before, describe gave up after one failed verification call; now it tries the other endpoint and only reports failure when neither proves the credentials.
If the first verification call fails, describe builds the other client type from the same resolved config (non-interactively, over the same config pointer) and tries its verification call. Account branch falls back to
CurrentUser.Me; workspace branch falls back toWorkspaces.Listwhen anaccount_idis configured (account clients require one). If the fallback succeeds, describe reports success with the matching fields (username fromMe, account ID for account-side success). If both calls fail, describe reports the first error. Success paths still make exactly one call.An earlier revision also treated HTTP 403 on both checks as proof of authentication (to cover non-admin users on account hosts, where
Workspaces.Listis admin-only). That heuristic was dropped: 403 also comes from network-level gates such as IP access lists and private link, which can answer before credentials are validated, so it is not reliable proof. For non-admin account users, describe keeps reporting the underlying permission error as-is.The output templates are unchanged.
Test plan
cmd/auth/describe_test.go: workspace check fails then account check succeeds; account check fails then workspace check succeeds; both fail reports the first error; no second call without anaccount_idacceptance/cmd/auth/describe/account-host-with-workspace-id: end-to-end reproduction of the issue (workspace client on an account host,Mereturns 400, account fallback succeeds)./task checksThis pull request and its description were written by Isaac.