Skip to content

Commit 9baf02b

Browse files
committed
Surface notFoundHint message in auth token error path
token.go's loadToken rewrites cache.ErrNotFound to the constant "cache: databricks OAuth is not configured for this host" so older SDK versions (pre-v0.77.0) can substring-match the error and fall through to other auth providers. That rewrite, written before the secure-storage default, silently threw away the notFoundHintCache message a default-secure user would see post-upgrade: instead of the actionable "stored credentials from older CLI versions are no longer used; run auth login or set DATABRICKS_AUTH_STORAGE=plaintext" copy, they got the generic "Try logging in again ... If this fails, please report this issue" trailer, which steers expected upgrade behavior toward bug reports. Add storage.HintForNotFound to extract the notFoundHint message from an error chain, plus storage.NewNotFoundHint as a test factory. token.go now keeps the SDK-compat substring AND appends the hint when present, and skips the helpfulError trailer in that case. Co-authored-by: Isaac
1 parent a01ac02 commit 9baf02b

4 files changed

Lines changed: 120 additions & 2 deletions

File tree

cmd/auth/token.go

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,10 +285,22 @@ func loadToken(ctx context.Context, args loadTokenArgs) (*oauth2.Token, error) {
285285
//
286286
// Older SDK versions check for a particular substring to determine if
287287
// the OAuth authentication type can fall through or if it is a real error.
288-
// This means we need to keep this error message constant for backwards compatibility.
288+
// This means we need to keep "databricks OAuth is not configured for
289+
// this host" present in the error for backwards compatibility.
289290
//
290291
// This is captured in an acceptance test under "cmd/auth/token".
291-
err = errors.New("cache: databricks OAuth is not configured for this host")
292+
const compatSubstring = "databricks OAuth is not configured for this host"
293+
// When storage's notFoundHintCache wrapped the ErrNotFound with
294+
// an actionable hint (e.g. the post-upgrade "stored credentials
295+
// from older CLI versions are no longer used; run `databricks
296+
// auth login`..." copy), surface it instead of the generic
297+
// "Try logging in again with ... If this fails, please report
298+
// this issue" trailer. The hint is more specific and avoids
299+
// users reporting expected post-upgrade behavior as a bug.
300+
if hint := storage.HintForNotFound(err); hint != "" {
301+
return nil, fmt.Errorf("cache: %s. %s", compatSubstring, hint)
302+
}
303+
err = errors.New("cache: " + compatSubstring)
292304
}
293305
if rewritten, rewrittenErr := auth.RewriteAuthError(ctx, args.authArguments.Host, args.authArguments.AccountID, args.profileName, err); rewritten {
294306
return nil, rewrittenErr

cmd/auth/token_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,34 @@ import (
1010
"time"
1111

1212
"github.com/databricks/cli/libs/auth"
13+
"github.com/databricks/cli/libs/auth/storage"
1314
"github.com/databricks/cli/libs/cmdio"
1415
"github.com/databricks/cli/libs/databrickscfg/profile"
1516
"github.com/databricks/cli/libs/env"
1617
"github.com/databricks/databricks-sdk-go/credentials/u2m"
18+
"github.com/databricks/databricks-sdk-go/credentials/u2m/cache"
1719
"github.com/databricks/databricks-sdk-go/httpclient/fixtures"
1820
"github.com/stretchr/testify/assert"
1921
"golang.org/x/oauth2"
2022
)
2123

24+
// upgradeHintTokenCache returns a notFoundHint-wrapped ErrNotFound on
25+
// Lookup, mirroring what storage.notFoundHintCache produces in
26+
// production when ~/.databricks/token-cache.json has entries and the
27+
// resolver picked secure mode by default. Used by TestToken_loadToken
28+
// to verify that auth token surfaces the upgrade-specific hint instead
29+
// of dropping it for the SDK-compat constant string.
30+
type upgradeHintTokenCache struct{}
31+
32+
func (upgradeHintTokenCache) Store(string, *oauth2.Token) error { return nil }
33+
func (upgradeHintTokenCache) Lookup(string) (*oauth2.Token, error) {
34+
return nil, storage.NewNotFoundHint(
35+
"stored credentials from older CLI versions are no longer used; run `databricks auth login` to sign in again, or set DATABRICKS_AUTH_STORAGE=plaintext to keep using the file cache",
36+
)
37+
}
38+
39+
var _ cache.TokenCache = upgradeHintTokenCache{}
40+
2241
type failOnCallTransport struct{}
2342

2443
func (failOnCallTransport) RoundTrip(*http.Request) (*http.Response, error) {
@@ -408,6 +427,35 @@ func TestToken_loadToken(t *testing.T) {
408427
"Try logging in again with `databricks auth login --host https://nonexistent.cloud.databricks.com` before retrying. " +
409428
"If this fails, please report this issue to the Databricks CLI maintainers at https://github.com/databricks/cli/issues/new",
410429
},
430+
{
431+
// Regression test: when notFoundHintCache wraps ErrNotFound
432+
// with the upgrade copy (post-upgrade default-secure user
433+
// with a populated token-cache.json), `auth token` must
434+
// surface that hint instead of dropping it for the SDK-compat
435+
// constant string. The combined message keeps the
436+
// "OAuth is not configured for this host" substring older
437+
// SDK versions look for and skips the generic "Try logging
438+
// in again ... If this fails, please report this issue"
439+
// trailer, which would mislead users into reporting expected
440+
// post-upgrade behavior.
441+
name: "ErrNotFound carrying upgrade hint surfaces it",
442+
args: loadTokenArgs{
443+
authArguments: &auth.AuthArguments{},
444+
profileName: "",
445+
args: []string{"nonexistent.cloud.databricks.com"},
446+
tokenTimeout: 1 * time.Hour,
447+
profiler: profiler,
448+
tokenCache: upgradeHintTokenCache{},
449+
persistentAuthOpts: []u2m.PersistentAuthOption{
450+
u2m.WithTokenCache(upgradeHintTokenCache{}),
451+
u2m.WithOAuthEndpointSupplier(&MockApiClient{}),
452+
},
453+
},
454+
wantErr: "cache: databricks OAuth is not configured for this host. " +
455+
"stored credentials from older CLI versions are no longer used; " +
456+
"run `databricks auth login` to sign in again, " +
457+
"or set DATABRICKS_AUTH_STORAGE=plaintext to keep using the file cache",
458+
},
411459
{
412460
name: "errors with clear message for non-host non-profile positional arg",
413461
args: loadTokenArgs{

libs/auth/storage/not_found_hint.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,33 @@ type notFoundHint struct {
5858
func (e *notFoundHint) Error() string { return e.msg }
5959
func (e *notFoundHint) Unwrap() error { return cache.ErrNotFound }
6060

61+
// HintForNotFound extracts the actionable hint message from an error
62+
// chain produced by notFoundHintCache. Returns the empty string if the
63+
// chain does not contain a notFoundHint (e.g. an unwrapped
64+
// cache.ErrNotFound from a plain TokenCache).
65+
//
66+
// Used by call sites like `auth token` that rewrite the SDK error for
67+
// backwards-compatibility (the "databricks OAuth is not configured for
68+
// this host" substring is load-bearing for older SDK fall-through
69+
// logic) but want to surface the actionable hint to the user instead of
70+
// dropping it.
71+
func HintForNotFound(err error) string {
72+
var hint *notFoundHint
73+
if errors.As(err, &hint) {
74+
return hint.msg
75+
}
76+
return ""
77+
}
78+
79+
// NewNotFoundHint returns an error that renders as msg but unwraps to
80+
// cache.ErrNotFound, mirroring what notFoundHintCache produces in
81+
// production. Exported so tests in other packages (e.g. cmd/auth) can
82+
// construct a hint-wrapped error without going through the full
83+
// resolver setup.
84+
func NewNotFoundHint(msg string) error {
85+
return &notFoundHint{msg: msg}
86+
}
87+
6188
// withNotFoundHint wraps inner so ErrNotFound from Lookup carries an
6289
// actionable hint. The legacy file path is resolved up front (where ctx
6390
// is available) so Lookup can do its check without needing a context.

libs/auth/storage/not_found_hint_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package storage
33
import (
44
"encoding/json"
55
"errors"
6+
"fmt"
67
"os"
78
"path/filepath"
89
"testing"
@@ -118,6 +119,36 @@ func TestNotFoundHintCache_StoreIsDelegated(t *testing.T) {
118119
require.NoError(t, c.Store("k", &oauth2.Token{AccessToken: "abc"}))
119120
}
120121

122+
func TestHintForNotFound(t *testing.T) {
123+
t.Run("nil returns empty", func(t *testing.T) {
124+
assert.Empty(t, HintForNotFound(nil))
125+
})
126+
127+
t.Run("plain ErrNotFound returns empty", func(t *testing.T) {
128+
// An unwrapped cache.ErrNotFound carries no hint, so the caller
129+
// (e.g. `auth token`) falls back to its default error path.
130+
assert.Empty(t, HintForNotFound(cache.ErrNotFound))
131+
})
132+
133+
t.Run("unrelated error returns empty", func(t *testing.T) {
134+
assert.Empty(t, HintForNotFound(errors.New("something else")))
135+
})
136+
137+
t.Run("notFoundHint returns the hint message", func(t *testing.T) {
138+
h := &notFoundHint{msg: "do the thing"}
139+
assert.Equal(t, "do the thing", HintForNotFound(h))
140+
})
141+
142+
t.Run("notFoundHint behind an fmt.Errorf wrap returns the hint", func(t *testing.T) {
143+
// The SDK wraps every cache error with `cache: %w`, so the hint
144+
// is one Unwrap away when it surfaces in callers. errors.As must
145+
// still find it.
146+
h := &notFoundHint{msg: "do the thing"}
147+
wrapped := fmt.Errorf("cache: %w", h)
148+
assert.Equal(t, "do the thing", HintForNotFound(wrapped))
149+
})
150+
}
151+
121152
func TestLegacyCacheHasTokens(t *testing.T) {
122153
tmp := t.TempDir()
123154

0 commit comments

Comments
 (0)