Skip to content

Commit cb96370

Browse files
auth profiles: probe both surfaces and OR the results
Replace ResolveConfigType's "either account or workspace" routing with a permissive validator: probe whichever API surfaces the profile has a signal for (host shape or field presence), and mark the profile valid if any probe succeeds. This addresses review feedback on the previous fix — it now also handles SPOG workspace-scoped credentials (e.g. a PAT), which the strict "SPOG always validates as account" rule would have falsely flagged as invalid. Probes run sequentially against the shared cfg because the SDK's lazy Authenticate() chain writes cfg.Host (via fixHostIfNeeded) while the other client's construction reads cfg.Host unlocked — go test -race flags the parallel version. Profile-level parallelism in newProfilesCommand is unchanged, so overall auth profiles wall-clock is still bounded by the slowest profile. ResolveConfigType is removed; IsSPOGHost and IsClassicWorkspaceHost are added in libs/auth as named wrappers so the host-shape check in profiles.go reads as a flat block of named booleans. Tests: - TestProfileLoadSPOGWorkspaceCredential covers PAT-on-SPOG explicitly: workspace probe succeeds, account probe 403s, OR yields Valid=true. - TestIsSPOGHost and TestIsClassicWorkspaceHost cover the three-way host classification, including the accounts-dod.* variant. Co-authored-by: Isaac
1 parent b073505 commit cb96370

4 files changed

Lines changed: 181 additions & 125 deletions

File tree

cmd/auth/profiles.go

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -57,40 +57,51 @@ func (c *profileMetadata) Load(ctx context.Context, configFilePath string, skipV
5757
return
5858
}
5959

60-
configType := auth.ResolveConfigType(cfg)
61-
if configType != cfg.ConfigType() {
62-
log.Debugf(ctx, "Profile %q: overrode config type from %s to %s (SPOG host)", c.Name, cfg.ConfigType(), configType)
63-
}
60+
// Validate by probing the API surfaces this profile has a signal for.
61+
// Each signal — host shape or field presence — enables its corresponding
62+
// probe, and the OR of the probe results is the verdict.
63+
64+
// Host signals.
65+
// isAccountHost: classic accounts.* host.
66+
// isSPOGHost: unified host with account-scoped OIDC discovery.
67+
// isWorkspaceHost: classic workspace host (neither of the above).
68+
isAccountHost := auth.IsClassicAccountHost(cfg.CanonicalHostName())
69+
isSPOGHost := auth.IsSPOGHost(cfg)
70+
isWorkspaceHost := auth.IsClassicWorkspaceHost(cfg)
71+
72+
// Field signals.
73+
// hasAccountID: account_id is set (from file, env, or discovery back-fill).
74+
// hasRealWorkspaceID: workspace_id is set to a real value.
75+
hasAccountID := cfg.AccountID != ""
76+
// workspace_id is "" when not present in the profile, "none" when the user picked Skip during SPOG login.
77+
hasRealWorkspaceID := cfg.WorkspaceID != "" && cfg.WorkspaceID != auth.WorkspaceIDNone
78+
79+
tryAccount := isAccountHost || isSPOGHost || hasAccountID
80+
tryWorkspace := isWorkspaceHost || hasRealWorkspaceID
6481

65-
switch configType {
66-
case config.AccountConfig:
82+
var accountOK, workspaceOK bool
83+
if tryAccount {
6784
a, err := databricks.NewAccountClient((*databricks.Config)(cfg))
68-
if err != nil {
69-
return
70-
}
71-
_, err = a.Workspaces.List(ctx)
72-
c.Host = cfg.Host
73-
c.AuthType = cfg.AuthType
74-
if err != nil {
75-
return
85+
if err == nil {
86+
if _, err := a.Workspaces.List(ctx); err == nil {
87+
accountOK = true
88+
}
7689
}
77-
c.Valid = true
78-
case config.WorkspaceConfig:
90+
}
91+
if tryWorkspace {
7992
w, err := databricks.NewWorkspaceClient((*databricks.Config)(cfg))
80-
if err != nil {
81-
return
82-
}
83-
_, err = w.CurrentUser.Me(ctx)
84-
c.Host = cfg.Host
85-
c.AuthType = cfg.AuthType
86-
if err != nil {
87-
return
93+
if err == nil {
94+
if _, err := w.CurrentUser.Me(ctx); err == nil {
95+
workspaceOK = true
96+
}
8897
}
89-
c.Valid = true
90-
case config.InvalidConfig:
91-
// Invalid configuration, skip validation
92-
return
9398
}
99+
100+
// Capture Host/AuthType after the probes run: SDK Authenticate() sets
101+
// cfg.AuthType lazily based on the credentials it actually exercised.
102+
c.Host = cfg.Host
103+
c.AuthType = cfg.AuthType
104+
c.Valid = accountOK || workspaceOK
94105
}
95106

96107
func newProfilesCommand() *cobra.Command {

cmd/auth/profiles_test.go

Lines changed: 65 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,11 @@ func TestProfilesDefaultMarker(t *testing.T) {
8080
}
8181

8282
// newSPOGServer creates a mock SPOG server that returns account-scoped OIDC.
83-
// It serves only the account validation endpoint: every SPOG profile must
84-
// route through Workspaces.List, regardless of workspace_id (see
85-
// ResolveConfigType). The workspace endpoint deliberately returns 500 so any
86-
// regression that routes a SPOG profile to CurrentUser.Me fails the test.
83+
// The workspace endpoint deliberately returns 500 to mirror real SPOG hosts
84+
// where account-audience OAuth tokens can't load workspace OAuth config.
85+
// auth profiles probes both surfaces and accepts either success, so the test
86+
// passes when Workspaces.List succeeds even though CurrentUser.Me fails —
87+
// and a regression that drops the account probe surfaces as Valid=false.
8788
func newSPOGServer(t *testing.T, accountID string) *httptest.Server {
8889
t.Helper()
8990
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -107,8 +108,9 @@ func newSPOGServer(t *testing.T, accountID string) *httptest.Server {
107108
}
108109

109110
// newWorkspaceServer creates a mock workspace server that returns workspace-scoped
110-
// OIDC and only serves the workspace validation endpoint. The account validation
111-
// endpoint returns 404 to prove the workspace path was taken.
111+
// OIDC and a workspace_id in discovery (mirroring real workspace hosts since
112+
// PR #4809). It serves CurrentUser.Me; the account endpoint returns 404 so a
113+
// workspace probe is the only path that produces Valid=true.
112114
func newWorkspaceServer(t *testing.T, accountID string) *httptest.Server {
113115
t.Helper()
114116
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -117,6 +119,7 @@ func newWorkspaceServer(t *testing.T, accountID string) *httptest.Server {
117119
case "/.well-known/databricks-config":
118120
_ = json.NewEncoder(w).Encode(map[string]any{
119121
"account_id": accountID,
122+
"workspace_id": "ws-from-discovery",
120123
"oidc_endpoint": r.Host + "/oidc",
121124
})
122125
case "/api/2.0/preview/scim/v2/Me":
@@ -147,10 +150,10 @@ func TestProfileLoadSPOGConfigType(t *testing.T) {
147150
wantValid: true,
148151
},
149152
{
150-
// Regression: this case used to route to CurrentUser.Me. The
151-
// SPOG mock now returns 500 on that endpoint, so anything other
152-
// than the account path produces wantValid=false.
153-
name: "SPOG workspace profile validated as account",
153+
// SPOG with a real workspace_id: workspace probe (CurrentUser.Me)
154+
// fails on the mock, account probe (Workspaces.List) succeeds —
155+
// the OR makes the profile valid.
156+
name: "SPOG workspace profile valid when account probe succeeds",
154157
host: spogServer.URL,
155158
accountID: "spog-acct",
156159
workspaceID: "ws-123",
@@ -203,6 +206,52 @@ func TestProfileLoadSPOGConfigType(t *testing.T) {
203206
}
204207
}
205208

209+
// TestProfileLoadSPOGWorkspaceCredential covers the inverse of the
210+
// account-OAuth case: a workspace-scoped credential (e.g. a PAT) against a
211+
// SPOG host. CurrentUser.Me succeeds, Workspaces.List fails (no account-level
212+
// access). The OR of the two probes must still mark the profile Valid=true.
213+
func TestProfileLoadSPOGWorkspaceCredential(t *testing.T) {
214+
const accountID = "spog-acct"
215+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
216+
w.Header().Set("Content-Type", "application/json")
217+
switch r.URL.Path {
218+
case "/.well-known/databricks-config":
219+
_ = json.NewEncoder(w).Encode(map[string]any{
220+
"account_id": accountID,
221+
"oidc_endpoint": r.Host + "/oidc/accounts/" + accountID,
222+
})
223+
case "/api/2.0/preview/scim/v2/Me":
224+
_ = json.NewEncoder(w).Encode(map[string]any{"userName": "test-user"})
225+
case "/api/2.0/accounts/" + accountID + "/workspaces":
226+
http.Error(w, "user lacks account-level access", http.StatusForbidden)
227+
default:
228+
w.WriteHeader(http.StatusNotFound)
229+
}
230+
}))
231+
t.Cleanup(server.Close)
232+
233+
dir := t.TempDir()
234+
configFile := filepath.Join(dir, ".databrickscfg")
235+
t.Setenv("HOME", dir)
236+
if runtime.GOOS == "windows" {
237+
t.Setenv("USERPROFILE", dir)
238+
}
239+
240+
content := "[ws-cred-on-spog]\nhost = " + server.URL + "\ntoken = test-token\naccount_id = " + accountID + "\nworkspace_id = ws-123\n"
241+
require.NoError(t, os.WriteFile(configFile, []byte(content), 0o600))
242+
243+
p := &profileMetadata{
244+
Name: "ws-cred-on-spog",
245+
Host: server.URL,
246+
AccountID: accountID,
247+
}
248+
p.Load(t.Context(), configFile, false)
249+
250+
assert.True(t, p.Valid, "workspace probe alone should make the profile valid")
251+
assert.NotEmpty(t, p.Host)
252+
assert.NotEmpty(t, p.AuthType)
253+
}
254+
206255
func TestClassicAccountsHostConfigType(t *testing.T) {
207256
// Classic accounts.* hosts can't be tested through Load() because httptest
208257
// generates 127.0.0.1 URLs. Verify directly that ConfigType() classifies
@@ -219,9 +268,12 @@ func TestClassicAccountsHostConfigType(t *testing.T) {
219268
}
220269

221270
func TestProfileLoadNoDiscoveryStaysWorkspace(t *testing.T) {
222-
// When .well-known returns 404 and the unified-host fallback is false,
223-
// the SPOG override should NOT trigger even if account_id is set. The
224-
// profile should stay WorkspaceConfig and validate via CurrentUser.Me.
271+
// account_id can linger in a profile from a prior account login on the
272+
// same profile name (e.g. user logged into accounts.cloud.databricks.com,
273+
// logged out, then re-used the profile name for a workspace login). A
274+
// stale account_id must not promote the profile to account validation
275+
// when the host itself isn't an account/SPOG surface — the workspace
276+
// probe is still the right signal.
225277
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
226278
w.Header().Set("Content-Type", "application/json")
227279
switch r.URL.Path {

libs/auth/config_type.go

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,32 +27,30 @@ func HasUnifiedHostSignal(discoveryURL string) bool {
2727
// of an accountID guard: SPOG routing requires an account ID to construct the
2828
// OAuth URL, so a nil or empty accountID always returns false.
2929
//
30-
// The accountID parameter is separate from cfg.AccountID so that callers can
31-
// control the source: ResolveConfigType passes cfg.AccountID (from config file),
32-
// while ToOAuthArgument passes the caller-provided value to avoid env var
33-
// contamination (DATABRICKS_ACCOUNT_ID or .well-known back-fill).
30+
// The accountID parameter is separate from cfg.AccountID so that callers
31+
// (currently ToOAuthArgument) can pass the caller-provided value to avoid
32+
// env-var contamination (DATABRICKS_ACCOUNT_ID or .well-known back-fill)
33+
// that would otherwise misroute plain workspace hosts as SPOG.
3434
func IsSPOG(cfg *config.Config, accountID string) bool {
3535
if accountID == "" {
3636
return false
3737
}
3838
return HasUnifiedHostSignal(cfg.DiscoveryURL)
3939
}
4040

41-
// ResolveConfigType returns the effective ConfigType for a resolved config.
42-
// The SDK's ConfigType() never returns AccountConfig for SPOG hosts (they
43-
// don't match the accounts.* prefix and resolve to UnifiedHost). For SPOG
44-
// the OAuth issuer is account-scoped, so every token's audience is the
45-
// account and workspace-API validation is unreliable — return AccountConfig
46-
// regardless of workspace_id.
47-
//
48-
// The cfg must already be resolved (via EnsureResolved) before calling this.
49-
func ResolveConfigType(cfg *config.Config) config.ConfigType {
50-
configType := cfg.ConfigType()
51-
if configType == config.AccountConfig {
52-
return configType
53-
}
54-
if IsSPOG(cfg, cfg.AccountID) {
55-
return config.AccountConfig
41+
// IsSPOGHost reports whether cfg points at a unified SPOG host: account-scoped
42+
// OIDC discovery and NOT a classic accounts.* host. Classic accounts.* hosts
43+
// share the same OIDC shape, so IsSPOG alone can't tell them apart; layer
44+
// IsClassicAccountHost on top to disambiguate.
45+
func IsSPOGHost(cfg *config.Config) bool {
46+
if IsClassicAccountHost(cfg.CanonicalHostName()) {
47+
return false
5648
}
57-
return configType
49+
return IsSPOG(cfg, cfg.AccountID)
50+
}
51+
52+
// IsClassicWorkspaceHost reports whether cfg points at a classic workspace
53+
// host: neither a classic accounts.* host nor a SPOG host.
54+
func IsClassicWorkspaceHost(cfg *config.Config) bool {
55+
return !IsClassicAccountHost(cfg.CanonicalHostName()) && !IsSPOG(cfg, cfg.AccountID)
5856
}

libs/auth/config_type_test.go

Lines changed: 59 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -24,82 +24,77 @@ func TestHasUnifiedHostSignal(t *testing.T) {
2424
}
2525
}
2626

27-
func TestResolveConfigType(t *testing.T) {
27+
func TestIsSPOG(t *testing.T) {
2828
cases := []struct {
29-
name string
30-
cfg *config.Config
31-
want config.ConfigType
29+
name string
30+
cfg *config.Config
31+
accountID string
32+
want bool
3233
}{
3334
{
34-
name: "classic accounts host stays AccountConfig",
35-
cfg: &config.Config{
36-
Host: "https://accounts.cloud.databricks.com",
37-
AccountID: "acct-123",
38-
},
39-
want: config.AccountConfig,
35+
name: "account-scoped OIDC with account_id",
36+
cfg: &config.Config{DiscoveryURL: "https://spog.databricks.com/oidc/accounts/acct-123/.well-known/oauth-authorization-server"},
37+
accountID: "acct-123",
38+
want: true,
4039
},
4140
{
42-
name: "SPOG account-scoped OIDC without workspace routes to AccountConfig",
43-
cfg: &config.Config{
44-
Host: "https://spog.databricks.com",
45-
AccountID: "acct-123",
46-
DiscoveryURL: "https://spog.databricks.com/oidc/accounts/acct-123/.well-known/oauth-authorization-server",
47-
},
48-
want: config.AccountConfig,
41+
name: "account-scoped OIDC without account_id",
42+
cfg: &config.Config{DiscoveryURL: "https://spog.databricks.com/oidc/accounts/acct-123/.well-known/oauth-authorization-server"},
43+
accountID: "",
44+
want: false,
4945
},
5046
{
51-
// SPOG OAuth is account-scoped, so even profiles with a real
52-
// workspace_id route to AccountConfig — the token's audience is
53-
// the account, and workspace-API validation can't authenticate it.
54-
name: "SPOG with real workspace_id routes to AccountConfig",
55-
cfg: &config.Config{
56-
Host: "https://spog.databricks.com",
57-
AccountID: "acct-123",
58-
WorkspaceID: "ws-456",
59-
DiscoveryURL: "https://spog.databricks.com/oidc/accounts/acct-123/.well-known/oauth-authorization-server",
60-
},
61-
want: config.AccountConfig,
62-
},
63-
{
64-
name: "SPOG account-scoped OIDC with workspace_id=none routes to AccountConfig",
65-
cfg: &config.Config{
66-
Host: "https://spog.databricks.com",
67-
AccountID: "acct-123",
68-
WorkspaceID: "none",
69-
DiscoveryURL: "https://spog.databricks.com/oidc/accounts/acct-123/.well-known/oauth-authorization-server",
70-
},
71-
want: config.AccountConfig,
72-
},
73-
{
74-
name: "workspace-scoped OIDC with account_id stays WorkspaceConfig",
75-
cfg: &config.Config{
76-
Host: "https://workspace.databricks.com",
77-
AccountID: "acct-123",
78-
DiscoveryURL: "https://workspace.databricks.com/oidc/.well-known/oauth-authorization-server",
79-
},
80-
want: config.WorkspaceConfig,
81-
},
82-
{
83-
name: "no discovery stays WorkspaceConfig",
84-
cfg: &config.Config{
85-
Host: "https://workspace.databricks.com",
86-
AccountID: "acct-123",
87-
},
88-
want: config.WorkspaceConfig,
89-
},
90-
{
91-
name: "plain workspace without account_id",
92-
cfg: &config.Config{
93-
Host: "https://workspace.databricks.com",
94-
},
95-
want: config.WorkspaceConfig,
47+
name: "workspace-scoped OIDC with account_id back-filled",
48+
cfg: &config.Config{DiscoveryURL: "https://workspace.databricks.com/oidc/.well-known/oauth-authorization-server"},
49+
accountID: "acct-123",
50+
want: false,
9651
},
9752
}
9853

9954
for _, tc := range cases {
10055
t.Run(tc.name, func(t *testing.T) {
101-
got := ResolveConfigType(tc.cfg)
102-
assert.Equal(t, tc.want, got)
56+
assert.Equal(t, tc.want, IsSPOG(tc.cfg, tc.accountID))
10357
})
10458
}
10559
}
60+
61+
// Configs used across the host-classification tests below. The three host
62+
// shapes are mutually exclusive: exactly one helper returns true per cfg.
63+
// accounts-dod.* is a second classic accounts variant — same OIDC shape
64+
// and classification as accounts.*, different URL prefix.
65+
var (
66+
classicAccountCfg = &config.Config{
67+
Host: "https://accounts.cloud.databricks.com",
68+
AccountID: "acct-123",
69+
DiscoveryURL: "https://accounts.cloud.databricks.com/oidc/accounts/acct-123/.well-known/oauth-authorization-server",
70+
}
71+
classicAccountDodCfg = &config.Config{
72+
Host: "https://accounts-dod.cloud.databricks.us",
73+
AccountID: "acct-123",
74+
DiscoveryURL: "https://accounts-dod.cloud.databricks.us/oidc/accounts/acct-123/.well-known/oauth-authorization-server",
75+
}
76+
spogCfg = &config.Config{
77+
Host: "https://spog.gcp.databricks.com",
78+
AccountID: "acct-123",
79+
DiscoveryURL: "https://spog.gcp.databricks.com/oidc/accounts/acct-123/.well-known/oauth-authorization-server",
80+
}
81+
classicWorkspaceCfg = &config.Config{
82+
Host: "https://dbc-xxxx.cloud.databricks.com",
83+
AccountID: "acct-123",
84+
DiscoveryURL: "https://dbc-xxxx.cloud.databricks.com/oidc/.well-known/oauth-authorization-server",
85+
}
86+
)
87+
88+
func TestIsSPOGHost(t *testing.T) {
89+
assert.False(t, IsSPOGHost(classicAccountCfg), "classic accounts.* shares the SPOG OIDC shape but is not SPOG")
90+
assert.False(t, IsSPOGHost(classicAccountDodCfg), "classic accounts-dod.* shares the SPOG OIDC shape but is not SPOG")
91+
assert.True(t, IsSPOGHost(spogCfg))
92+
assert.False(t, IsSPOGHost(classicWorkspaceCfg))
93+
}
94+
95+
func TestIsClassicWorkspaceHost(t *testing.T) {
96+
assert.False(t, IsClassicWorkspaceHost(classicAccountCfg))
97+
assert.False(t, IsClassicWorkspaceHost(classicAccountDodCfg))
98+
assert.False(t, IsClassicWorkspaceHost(spogCfg))
99+
assert.True(t, IsClassicWorkspaceHost(classicWorkspaceCfg))
100+
}

0 commit comments

Comments
 (0)