Fix auth logout failing to clear token for workspace profiles with account ID#4853
Conversation
mihaimitrea-db
changed the title
mihaimitrea-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: 4a4b68e
17 interesting tests: 10 SKIP, 7 RECOVERED
Top 46 slowest tests (at least 2 minutes):
|
mihaimitrea-db
changed the title
mihaimitrea-db
changed the title
mihaimitrea-db
requested review from
andrewnester,
anton-107,
denik,
pietern,
shreyas-goenka and
simonfaltum
as code owners
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Review from Isaac + Cursor (2-round deep review)
1 Critical | 1 Gap (Nit) | 1 Suggestion
The fix for the stale account_id case on classic workspace profiles looks correct. The concern is that switching to HostType() might regress SPOG/discovery-routed profiles. See inline comment for details.
[Gap - Nit] Missing test for the return "", nil branch
cmd/auth/logout_test.go: When HostType() returns AccountHost/UnifiedHost but p.AccountID is empty, the new code returns "", nil. This path doesn't have test coverage. It's an unlikely scenario (an accounts.* host with no account_id), but it's new behavior that differs from the old code (which would've fallen through to the workspace branch).
[Suggestion] Direct unit tests for hostCacheKeyAndMatchFn
This is a pure function with clear inputs/outputs, but it's only tested indirectly through runLogout. A table-driven TestHostCacheKeyAndMatchFn covering workspace, stale-account workspace, account, unified, and SPOG profiles would serve as documentation of the intended cache key shapes.
cmd/auth/logout.go
Outdated
| switch cfg.HostType() { | ||
| case config.AccountHost, config.UnifiedHost: | ||
| if p.AccountID == "" { | ||
| return "", nil | ||
| } | ||
| return host + "/oidc/accounts/" + p.AccountID, profile.WithHostAndAccountID(host, p.AccountID) | ||
| default: | ||
| return host, profile.WithHost(host) | ||
| } |
There was a problem hiding this comment.
[Critical] HostType() might regress logout for SPOG/discovery-routed profiles
HostType() classifies hosts by URL pattern: only accounts.* becomes AccountHost, only profiles with experimental_is_unified_host become UnifiedHost. Everything else is WorkspaceHost.
The problem is SPOG/discovery-routed profiles. During login, libs/auth/arguments.go ToOAuthArgument() routes SPOG hosts (e.g. https://spog.example.com) to unified OAuth when discovery returns an account-scoped OIDC endpoint. The token gets stored under host/oidc/accounts/<account_id>. But runHostDiscovery() does NOT set Experimental_IsUnifiedHost, and discoveryLogin() explicitly clears it when saving the profile.
So a valid SPOG profile has host + account_id + workspace_id but no unified flag. With this change, HostType() returns WorkspaceHost for these profiles, logout tries to delete just <host> instead of <host>/oidc/accounts/<account_id>, and the token is left behind. The old p.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 stale account_id but no workspace_id should 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.
simonfaltum
removed request for
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
…ale account_id logout derived the token-cache key by checking whether `account_id` was set on the profile, but a workspace profile can carry a stale `account_id` from an earlier account-level login. This caused logout to build an account-style cache key (`host/oidc/accounts/<id>`) that did not match the workspace-style key the SDK actually wrote, so the cached token was never deleted. Use `config.HostType()` instead to decide the cache-key shape, which matches the SDK's own OAuth routing logic and correctly treats workspace hosts as workspace profiles regardless of leftover account metadata. Co-authored-by: Isaac
mihaimitrea-db
force-pushed
the
mihaimitrea-db/logout-workspace-accountid-bug
branch
from
00421c4 to
bf6c3da
Compare
The title calls used trailing \n which produced unnecessary blank lines in the expected output. Align with the style of other titles in the same script.
|
This looks good to me. We're relying on Worth noting that we might remove the dual-write for host keys in the future as part of a broader effort to move tokens to secure storage. That's currently in the planning phase and nothing is decided yet, so no need to account for it here. |
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Looks good, tests pass locally.
…-workspace-accountid-bug
mihaimitrea-db
deleted the
mihaimitrea-db/logout-workspace-accountid-bug
branch
3 participants
Summary
Fix
auth logoutfailing to clear cached tokens for workspace profiles that carry anaccount_idin~/.databrickscfg.Background
The CLI's token cache stores tokens under two keys: a profile key (e.g.
"logfood") and a host key (eitherhostfor workspace profiles orhost/oidc/accounts/<id>for account/SPOG profiles). The host key shape is determined at login time byToOAuthArgument, which calls.well-known/databricks-configto discover whether the host uses workspace-scoped or account-scoped OIDC.The old
hostCacheKeyAndMatchFninauth logoutusedp.AccountID != ""as a proxy for "use the account-style cache key." This breaks because:runHostDiscoverypopulatesaccount_idon every workspace profile from the.well-knownendpoint. A regular workspace profile now routinely hasaccount_idset.account_id,workspace_id,experimental_is_unified_host) cannot reliably distinguish between a classic workspace, a SPOG host, or stale metadata from a prior login. The only reliable discriminator is theoidc_endpointshape from.well-known, which is resolved at runtime and not persisted to the profile.Changes
hostCacheKeyAndMatchFnnow delegates toauth.AuthArguments.ToOAuthArgument().GetCacheKey()— the same routing logic the SDK uses when writing the token during login. This includes the.well-known/databricks-confignetwork call that correctly distinguishes workspace hosts from SPOG hosts.Four acceptance tests that used hardcoded fake hostnames were switched to
$DATABRICKS_HOST(the mock test server) so that.well-knownresolves without warnings.Tests
TestHostCacheKeyAndMatchFn— table-driven test with mock HTTP servers covering classic workspace, stale-account workspace, classic account, unified, and SPOG profiles.TestLogoutSPOGProfile— verifies both profile-keyed and host-keyed tokens are cleared for a SPOG profile.TestLogouttable — includes a stale-account workspace row.stale-account-id-workspace-host— end-to-end test: workspace profile with staleaccount_id, runs logout, verifies token cache is cleared,auth profilesshows invalid,auth tokenfails.go test ./cmd/authandgo test ./acceptance -run TestAccept/cmd/auth/logoutpass.