diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5d5195212..5bd836ed3 100644 --- a/internal/auth/auth.go +++ b/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 { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 923fdf9f2..4019313e3 100644 --- a/internal/auth/auth_test.go +++ b/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() diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 8deff4bd0..8fb20fcc9 100644 --- a/internal/auth/keyring.go +++ b/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 diff --git a/internal/auth/keyring_test.go b/internal/auth/keyring_test.go index 22b6701c9..3b67f021b 100644 --- a/internal/auth/keyring_test.go +++ b/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") +}