Warn about the keyring fallback on the first credential read, not only on login

internal/auth/auth.go

@@ -1158,7 +1158,17 @@ func (m *Manager) GetUserEmail() string {
 
 // SetUserEmail stores the user email for the current credential key
 // without modifying the stored user ID.
+//
+// BASECAMP_TOKEN wins — match AccessToken() precedence. The email was
+// fetched with the environment token, so it names that token's user, not
+// whoever the stored credentials belong to; writing it there would
+// mislabel them. Skipping the store also keeps a token session off the
+// keyring probe and the fallback warning it can raise.
 func (m *Manager) SetUserEmail(email string) error {
+	if os.Getenv("BASECAMP_TOKEN") != "" {
+		return nil
+	}
+
 	credKey := m.credentialKey()
 	creds, err := m.store.Load(credKey)
 	if err != nil {

internal/auth/auth_test.go

@@ -16,6 +16,7 @@ import (
 	"time"
 
 	"github.com/basecamp/basecamp-sdk/go/pkg/basecamp/oauth"
+	"github.com/basecamp/cli/credstore"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -260,6 +261,23 @@ func TestSetUserEmail(t *testing.T) {
 	assert.Equal(t, "original-id", loaded.UserID)
 }
 
+// Regression: `basecamp people me` on BASECAMP_TOKEN stored the fetched
+// email through SetUserEmail, the one read path with no env-token guard.
+// That both mislabeled any stored credentials with the env token's user and
+// ran the keyring probe — so a CI host with a locked keychain now warned
+// about plaintext credentials it neither stored nor read.
+func TestSetUserEmailSkipsStoreOnEnvToken(t *testing.T) {
+	t.Setenv("BASECAMP_TOKEN", "bc_at_from_environment")
+	swapNewCredStore(t, func(credstore.StoreOptions) credStore {
+		t.Error("a token session must not construct the credential store")
+		return &fallenBackStore{}
+	})
+	manager := NewManager(&config.Config{BaseURL: "https://3.basecampapi.com"}, http.DefaultClient)
+	manager.store = NewStore(t.TempDir())
+
+	require.NoError(t, manager.SetUserEmail("someone@example.com"))
+}
+
 func TestSetUserIdentity(t *testing.T) {
 	tmpDir := t.TempDir()
 

internal/auth/keyring.go

@@ -40,12 +40,24 @@ type Credentials struct {
 type Store struct {
 	fallbackDir string
 	initOnce    sync.Once
-	inner       *credstore.Store
+	inner       credStore
 	warnOnce    sync.Once
 }
 
+// credStore is the slice of credstore.Store this wrapper uses, as an
+// interface so tests can substitute a store that fell back to file storage
+// without failing a real keyring probe.
+type credStore interface {
+	Load(key string) ([]byte, error)
+	Save(key string, data []byte) error
+	Delete(key string) error
+	MigrateToKeyring() error
+	UsingKeyring() bool
+	FallbackWarning() string
+}
+
 // newCredStore is replaceable in tests to avoid real keyring access.
-var newCredStore = credstore.NewStore
+var newCredStore = func(opts credstore.StoreOptions) credStore { return credstore.NewStore(opts) }
 
 // sessionIsHeadless reports that no human can answer a keyring unlock
 // prompt: stdin, stdout, and stderr are all non-terminals AND no GUI
@@ -80,7 +92,7 @@ func NewStore(fallbackDir string) *Store {
 // ensure constructs the underlying store on first use. Callers reach here
 // from paths that don't hold Manager.mu (e.g. IsAuthenticated), so the
 // sync.Once provides the synchronization.
-func (s *Store) ensure() *credstore.Store {
+func (s *Store) ensure() credStore {
 	s.initOnce.Do(func() {
 		opts := credstore.StoreOptions{
 			ServiceName:   "basecamp",
@@ -95,7 +107,13 @@ func (s *Store) ensure() *credstore.Store {
 	return s.inner
 }
 
-// warnFallback prints the keyring fallback warning once, on first credential write.
+// warnFallback prints the keyring fallback warning once per process, on the
+// first credential read or write. Reads warn too: a fallback read returns
+// whatever an earlier fallback left in credentials.json — possibly months
+// stale — rather than the credentials the keyring holds, and the probe
+// failure behind it would otherwise stay invisible until the next login.
+// Hosts that mean to use file storage set BASECAMP_NO_KEYRING, which skips
+// the probe and so never warns.
 func (s *Store) warnFallback() {
 	inner := s.ensure()
 	s.warnOnce.Do(func() {
@@ -107,6 +125,7 @@ func (s *Store) warnFallback() {
 
 // Load retrieves credentials for the given origin.
 func (s *Store) Load(origin string) (*Credentials, error) {
+	s.warnFallback()
 	data, err := s.ensure().Load(origin)
 	if err != nil {
 		return nil, err

internal/auth/keyring_test.go

@@ -1,6 +1,9 @@
 package auth
 
 import (
+	"io"
+	"os"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -10,7 +13,7 @@ import (
 )
 
 // swapNewCredStore replaces the credstore constructor seam for the test.
-func swapNewCredStore(t *testing.T, fn func(credstore.StoreOptions) *credstore.Store) {
+func swapNewCredStore(t *testing.T, fn func(credstore.StoreOptions) credStore) {
 	t.Helper()
 	orig := newCredStore
 	newCredStore = fn
@@ -28,7 +31,7 @@ func TestNewStoreIsLazy(t *testing.T) {
 	t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring
 
 	calls := 0
-	swapNewCredStore(t, func(opts credstore.StoreOptions) *credstore.Store {
+	swapNewCredStore(t, func(opts credstore.StoreOptions) credStore {
 		calls++
 		return credstore.NewStore(opts)
 	})
@@ -60,7 +63,7 @@ func ensureOptions(t *testing.T, headless bool) credstore.StoreOptions {
 	t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring
 
 	var got credstore.StoreOptions
-	swapNewCredStore(t, func(opts credstore.StoreOptions) *credstore.Store {
+	swapNewCredStore(t, func(opts credstore.StoreOptions) credStore {
 		got = opts
 		return credstore.NewStore(opts)
 	})
@@ -78,3 +81,55 @@ func TestEnsureBoundsProbeOnlyWhenHeadless(t *testing.T) {
 	assert.Equal(t, headlessProbeTimeout, ensureOptions(t, true).ProbeTimeout)
 	assert.Zero(t, ensureOptions(t, false).ProbeTimeout)
 }
+
+// fallenBackStore stands in for a credstore.Store whose keyring probe failed
+// and which is serving the plaintext file instead.
+type fallenBackStore struct{ warning string }
+
+func (f *fallenBackStore) Load(string) ([]byte, error) { return []byte(`{"access_token":"tok"}`), nil }
+func (f *fallenBackStore) Save(string, []byte) error   { return nil }
+func (f *fallenBackStore) Delete(string) error         { return nil }
+func (f *fallenBackStore) MigrateToKeyring() error     { return nil }
+func (f *fallenBackStore) UsingKeyring() bool          { return false }
+func (f *fallenBackStore) FallbackWarning() string     { return f.warning }
+
+// captureStderr returns what the callback wrote to os.Stderr.
+func captureStderr(t *testing.T, fn func()) string {
+	t.Helper()
+	r, w, err := os.Pipe()
+	require.NoError(t, err)
+	orig := os.Stderr
+	os.Stderr = w
+	defer func() { os.Stderr = orig }()
+
+	fn()
+
+	require.NoError(t, w.Close())
+	out, err := io.ReadAll(r)
+	require.NoError(t, err)
+	return string(out)
+}
+
+// Regression: the fallback warning printed only on Save, so a process that
+// merely read credentials — every command but login — could serve a stale
+// credentials.json after a failed keyring probe without a word about it.
+// The warning must show on the first read too, and still only once.
+func TestLoadWarnsOnceWhenKeyringFellBack(t *testing.T) {
+	warning := "system keyring unavailable (User interaction is not allowed.), credentials stored in plaintext at /tmp/credentials.json"
+	swapNewCredStore(t, func(credstore.StoreOptions) credStore { return &fallenBackStore{warning: warning} })
+	store := NewStore(t.TempDir())
+
+	reads := captureStderr(t, func() {
+		for range 2 {
+			_, err := store.Load("profile:work")
+			require.NoError(t, err)
+		}
+	})
+	assert.Equal(t, 1, strings.Count(reads, "warning: "), "the first read warns, the second does not")
+	assert.Contains(t, reads, "warning: "+warning+"\n")
+
+	write := captureStderr(t, func() {
+		require.NoError(t, store.Save("profile:work", &Credentials{AccessToken: "tok"}))
+	})
+	assert.Empty(t, write, "a write after the read has already warned stays quiet")
+}