-
Notifications
You must be signed in to change notification settings - Fork 155
Fix auth logout failing to clear token for workspace profiles with account ID #4853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
mihaimitrea-db
wants to merge
1
commit into
main
Choose a base branch
from
mihaimitrea-db/logout-workspace-accountid-bug
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
5 changes: 5 additions & 0 deletions
5
acceptance/cmd/auth/logout/stale-account-id-workspace-host/out.test.toml
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
42 changes: 42 additions & 0 deletions
42
acceptance/cmd/auth/logout/stale-account-id-workspace-host/output.txt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
|
|
||
| === Profiles before logout — logfood should be valid | ||
|
|
||
| >>> [CLI] auth profiles | ||
| Name Host Valid | ||
| logfood (Default) [DATABRICKS_URL] YES | ||
|
|
||
| === Token cache keys before logout | ||
| [ | ||
| "[DATABRICKS_URL]", | ||
| "logfood" | ||
| ] | ||
|
|
||
| === Logout without --delete | ||
| >>> [CLI] auth logout --profile logfood --force | ||
| Logged out of profile "logfood". Use --delete to also remove it from the config file. | ||
|
|
||
| === Config after logout — profile should still exist | ||
| ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. | ||
| [DEFAULT] | ||
|
|
||
| [logfood] | ||
| host = [DATABRICKS_URL] | ||
| account_id = stale-account | ||
| auth_type = databricks-cli | ||
|
|
||
| [__settings__] | ||
| default_profile = logfood | ||
|
|
||
| === Token cache keys after logout — both entries should be removed | ||
| [] | ||
|
|
||
| === Profiles after logout — logfood should be invalid | ||
|
|
||
| >>> [CLI] auth profiles | ||
| Name Host Valid | ||
| logfood (Default) [DATABRICKS_URL] NO | ||
|
|
||
| === Logged out profile should no longer return a token | ||
|
|
||
| >>> musterr [CLI] auth token --profile logfood | ||
| Error: cache: databricks OAuth is not configured for this host. Try logging in again with `databricks auth login --profile logfood` before retrying. If this fails, please report this issue to the Databricks CLI maintainers at https://github.com/databricks/cli/issues/new |
52 changes: 52 additions & 0 deletions
52
acceptance/cmd/auth/logout/stale-account-id-workspace-host/script
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| sethome "./home" | ||
|
|
||
| cat > "./home/.databrickscfg" <<EOF | ||
| ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. | ||
| [DEFAULT] | ||
|
|
||
| [logfood] | ||
| host = ${DATABRICKS_HOST} | ||
| account_id = stale-account | ||
| auth_type = databricks-cli | ||
|
|
||
| [__settings__] | ||
| default_profile = logfood | ||
| EOF | ||
|
|
||
| mkdir -p "./home/.databricks" | ||
| cat > "./home/.databricks/token-cache.json" <<EOF | ||
| { | ||
| "version": 1, | ||
| "tokens": { | ||
| "logfood": { | ||
| "access_token": "logfood-cached-token", | ||
| "token_type": "Bearer" | ||
| }, | ||
| "${DATABRICKS_HOST}": { | ||
| "access_token": "logfood-host-token", | ||
| "token_type": "Bearer" | ||
| } | ||
| } | ||
| } | ||
| EOF | ||
|
|
||
| title "Profiles before logout — logfood should be valid\n" | ||
| trace $CLI auth profiles | ||
|
|
||
| title "Token cache keys before logout\n" | ||
| jq -S '.tokens | keys' "./home/.databricks/token-cache.json" | ||
|
|
||
| title "Logout without --delete" | ||
| trace $CLI auth logout --profile logfood --force | ||
|
|
||
| title "Config after logout — profile should still exist\n" | ||
| cat "./home/.databrickscfg" | ||
|
|
||
| title "Token cache keys after logout — both entries should be removed\n" | ||
| jq -S '.tokens | keys' "./home/.databricks/token-cache.json" | ||
|
|
||
| title "Profiles after logout — logfood should be invalid\n" | ||
| trace $CLI auth profiles | ||
|
|
||
| title "Logged out profile should no longer return a token\n" | ||
| trace musterr $CLI auth token --profile logfood |
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical]
HostType()might regress logout for SPOG/discovery-routed profilesHostType()classifies hosts by URL pattern: onlyaccounts.*becomesAccountHost, only profiles withexperimental_is_unified_hostbecomeUnifiedHost. Everything else isWorkspaceHost.The problem is SPOG/discovery-routed profiles. During login,
libs/auth/arguments.goToOAuthArgument()routes SPOG hosts (e.g.https://spog.example.com) to unified OAuth when discovery returns an account-scoped OIDC endpoint. The token gets stored underhost/oidc/accounts/<account_id>. ButrunHostDiscovery()does NOT setExperimental_IsUnifiedHost, anddiscoveryLogin()explicitly clears it when saving the profile.So a valid SPOG profile has
host+account_id+workspace_idbut no unified flag. With this change,HostType()returnsWorkspaceHostfor these profiles, logout tries to delete just<host>instead of<host>/oidc/accounts/<account_id>, and the token is left behind. The oldp.AccountID != ""check actually handled this correctly (for the wrong reasons).I think the fix needs to account for profiles that carry
workspace_id(indicating they went through discovery). Those should keep the account-style cache key. Only profiles with a staleaccount_idbut noworkspace_idshould get downgraded to the plain host key. Adding regression tests for a SPOG profile (host+account_id+workspace_id, no unified flag) would be good.