Address codex review on PinSecureMode

Two findings:

1. The persist failure was logged at debug, which silently weakens the
   stated pin-on-success guarantee: login succeeds, the keyring write
   succeeds, but auth_storage = secure is never persisted. A later
   default-secure read with a transient keyring probe failure would
   then fall back to plaintext, undoing the protection the pin was
   supposed to provide. Promote to log.Warnf so the user sees the
   failure during login and can investigate (file permissions, etc.).
   Login is still not blocked: pinning is best-effort.

2. PinSecureMode re-resolved the storage mode with StorageModeUnknown,
   so a caller-supplied override was invisible to the source check.
   The function's doc said "No-op when the user already chose a mode
   explicitly", but only env and config were detected; an override
   secure mode would still pin auth_storage = secure to config,
   silently turning an ephemeral per-invocation choice into a
   persistent one. No caller passes a non-empty override today, so the
   bug was dormant, but fix it before someone wires up a flag and
   notices the behavior.

   PinSecureMode now takes an override StorageMode parameter, and the
   three call sites (main login, discoveryLogin, runInlineLogin) pass
   storage.StorageModeUnknown explicitly. New table-driven case covers
   the override path.

Co-authored-by: Isaac

Author: simonfaltum

cmd/auth/login.go

@@ -298,7 +298,7 @@ a new profile is created.
 		// Lock secure mode in after a successful keyring write so a later
 		// transient keyring probe failure cannot silently demote this user
 		// to plaintext.
-		storage.PinSecureMode(ctx, mode)
+		storage.PinSecureMode(ctx, mode, storage.StorageModeUnknown)
 
 		// At this point, an OAuth token has been successfully minted and stored
 		// in the CLI cache. The rest of the command focuses on:
@@ -639,7 +639,7 @@ func discoveryLogin(ctx context.Context, in discoveryLoginInputs) error {
 	if err := persistentAuth.Challenge(); err != nil {
 		return discoveryErr("login via login.databricks.com failed", err)
 	}
-	storage.PinSecureMode(ctx, in.mode)
+	storage.PinSecureMode(ctx, in.mode, storage.StorageModeUnknown)
 
 	discoveredHost := arg.GetDiscoveredHost()
 	if discoveredHost == "" {

cmd/auth/token.go

@@ -433,7 +433,7 @@ func runInlineLogin(ctx context.Context, profiler profile.Profiler, tokenCache c
 	if err = persistentAuth.Challenge(); err != nil {
 		return "", nil, err
 	}
-	storage.PinSecureMode(ctx, mode)
+	storage.PinSecureMode(ctx, mode, storage.StorageModeUnknown)
 
 	clearKeys := oauthLoginClearKeys()
 	clearKeys = append(clearKeys, databrickscfg.ExperimentalIsUnifiedHostKey)

libs/auth/storage/cache.go

@@ -248,14 +248,20 @@ func persistPlaintextFallback(ctx context.Context) {
 // the user to plaintext.
 //
 // No-op when mode is not secure or when the user already chose a mode
-// explicitly. Best-effort: persistence failures are logged at debug and
-// never block login. Concurrent logins racing this write is benign because
-// both write the same value.
-func PinSecureMode(ctx context.Context, mode StorageMode) {
+// explicitly via override, env var, or config. override must be the same
+// value the caller passed to ResolveCacheForLogin so the source check sees
+// the caller's intent rather than re-resolving without it.
+//
+// Persistence failures are logged at warn: they do not block login, but
+// the user should know the pin did not happen, since a later transient
+// keyring failure could then silently route a default-secure user to
+// plaintext. Concurrent logins racing this write is benign because both
+// write the same value.
+func PinSecureMode(ctx context.Context, mode, override StorageMode) {
 	if mode != StorageModeSecure {
 		return
 	}
-	_, source, err := ResolveStorageModeWithSource(ctx, StorageModeUnknown)
+	_, source, err := ResolveStorageModeWithSource(ctx, override)
 	if err != nil {
 		log.Debugf(ctx, "pin secure mode: resolve: %v", err)
 		return
@@ -265,6 +271,6 @@ func PinSecureMode(ctx context.Context, mode StorageMode) {
 	}
 	configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE")
 	if err := databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModeSecure), configPath); err != nil {
-		log.Debugf(ctx, "persist auth_storage=secure pin failed: %v", err)
+		log.Warnf(ctx, "could not persist auth_storage=secure to %s: %v. Future commands may need DATABRICKS_AUTH_STORAGE=secure to keep using the OS keyring.", configPath, err)
 	}
 }

libs/auth/storage/cache_test.go

@@ -386,6 +386,7 @@ func TestPinSecureMode(t *testing.T) {
 	cases := []struct {
 		name        string
 		mode        StorageMode
+		override    StorageMode
 		envValue    string
 		configBody  string
 		wantWritten string
@@ -412,6 +413,16 @@ func TestPinSecureMode(t *testing.T) {
 			configBody:  "[__settings__]\nauth_storage = secure\n",
 			wantWritten: "secure",
 		},
+		{
+			// The override signal is per-invocation, so persisting it to
+			// config would silently turn an ephemeral choice into a
+			// persistent one. Honor the caller's explicit override by
+			// no-op'ing the pin.
+			name:        "secure from override is a no-op",
+			mode:        StorageModeSecure,
+			override:    StorageModeSecure,
+			wantWritten: "",
+		},
 	}
 
 	for _, tc := range cases {
@@ -426,7 +437,7 @@ func TestPinSecureMode(t *testing.T) {
 				ctx = env.Set(ctx, EnvVar, tc.envValue)
 			}
 
-			PinSecureMode(ctx, tc.mode)
+			PinSecureMode(ctx, tc.mode, tc.override)
 
 			got, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath)
 			require.NoError(t, err)
@@ -440,13 +451,13 @@ func TestPinSecureMode_IsIdempotent(t *testing.T) {
 	ctx := t.Context()
 	configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE")
 
-	PinSecureMode(ctx, StorageModeSecure)
+	PinSecureMode(ctx, StorageModeSecure, StorageModeUnknown)
 	first, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath)
 	require.NoError(t, err)
 	require.Equal(t, "secure", first)
 
 	// Second call should see source=Config and skip the write.
-	PinSecureMode(ctx, StorageModeSecure)
+	PinSecureMode(ctx, StorageModeSecure, StorageModeUnknown)
 	second, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath)
 	require.NoError(t, err)
 	assert.Equal(t, "secure", second)
@@ -461,8 +472,8 @@ func TestPinSecureMode_PersistFailureIsSwallowed(t *testing.T) {
 	configPath := filepath.Join(t.TempDir(), "no-such-dir", ".databrickscfg")
 	t.Setenv("DATABRICKS_CONFIG_FILE", configPath)
 
-	// Must not panic or block; failures are logged at debug.
-	PinSecureMode(ctx, StorageModeSecure)
+	// Must not panic or block; failure surfaces in the warn log.
+	PinSecureMode(ctx, StorageModeSecure, StorageModeUnknown)
 
 	// The persist failure must not have produced any file.
 	_, err := os.Stat(configPath)