From 7d627300ef6b4a3805fe9ded83c9777a79ec0a23 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:27:12 -0700 Subject: [PATCH 1/4] Name an id-only identity in me and never blank a stored email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An in-house (bc3) token's /authorization.json reports only identity.id. `basecamp me` rendered that as " <> - 1 Basecamp account(s)" and then stored the empty email over the one the login had written, so `auth status` forgot who the credential belonged to. The --expect-identity mismatch message opened with the same blank name. SetUserEmail and SetUserIdentity now treat an empty value as an omission. `me` names an id-only identity as "identity ", and when an account is configured it looks the person up there — the same call a login verifies against — for the name and email, adding them to the JSON as data.person; a failed lookup keeps the identity rendering. The label every login message renders names an id-only identity by its ids alone. --- internal/auth/auth.go | 21 ++++- internal/auth/auth_test.go | 27 ++++++ internal/commands/auth.go | 9 +- internal/commands/auth_identity_test.go | 18 ++++ internal/commands/people.go | 62 +++++++++++-- internal/commands/people_test.go | 110 ++++++++++++++++++++++++ 6 files changed, 232 insertions(+), 15 deletions(-) create mode 100644 internal/commands/auth_identity_test.go diff --git a/internal/auth/auth.go b/internal/auth/auth.go index c7a39610..dd76c0c4 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -1496,8 +1496,12 @@ func (m *Manager) GetUserEmail() string { // 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. +// +// An empty email is an omission, not a value: an in-house (bc3) token's +// authorization document carries only the identity id, and a caller +// relaying that must not blank what a login stored. func (m *Manager) SetUserEmail(email string) error { - if os.Getenv("BASECAMP_TOKEN") != "" { + if os.Getenv("BASECAMP_TOKEN") != "" || email == "" { return nil } @@ -1510,15 +1514,24 @@ func (m *Manager) SetUserEmail(email string) error { return m.store.Save(credKey, creds) } -// SetUserIdentity stores the user ID and email for the current credential key. +// SetUserIdentity stores the user ID and email for the current credential +// key. As with SetUserEmail, an empty value leaves the stored field alone. func (m *Manager) SetUserIdentity(userID, email string) error { + if userID == "" && email == "" { + return nil + } + credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { return err } - creds.UserID = userID - creds.UserEmail = email + if userID != "" { + creds.UserID = userID + } + if email != "" { + creds.UserEmail = email + } return m.store.Save(credKey, creds) } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 25778602..4600c07d 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2167,3 +2167,30 @@ func TestRefresh_HalfConfiguredClientHintsTheProfile(t *testing.T) { assert.Contains(t, cliErr.Message, "BASECAMP_OAUTH_CLIENT_SECRET is required") assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) } + +// TestSetUserIdentity_EmptyValuesAreOmissions: an authorization document +// that names only an identity id must not blank the name a login stored. +func TestSetUserIdentity_EmptyValuesAreOmissions(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "") + m := &Manager{cfg: config.Default(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "tok", OAuthType: "bc5", UserID: "1", UserEmail: "kept@example.com", + })) + + require.NoError(t, m.SetUserEmail("")) + require.NoError(t, m.SetUserIdentity("", "")) + creds, err := m.store.Load(key) + require.NoError(t, err) + assert.Equal(t, "1", creds.UserID) + assert.Equal(t, "kept@example.com", creds.UserEmail) + + require.NoError(t, m.SetUserIdentity("2", "")) + creds, err = m.store.Load(key) + require.NoError(t, err) + assert.Equal(t, "2", creds.UserID) + assert.Equal(t, "kept@example.com", creds.UserEmail, "an omitted email leaves the stored one") + + require.NoError(t, m.SetUserEmail("new@example.com")) + assert.Equal(t, "new@example.com", m.GetUserEmail()) +} diff --git a/internal/commands/auth.go b/internal/commands/auth.go index d964ab8b..28fca10e 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -688,7 +688,9 @@ type loginIdentity struct { } // label renders the identity for a one-line terminal sink. Name and email -// are server-supplied, so they are reduced to single lines first. +// are server-supplied, so they are reduced to single lines first. An +// identity the server reported without either (an in-house bc3 token's) +// is named by its ids alone rather than as a blank with ids in brackets. func (l *loginIdentity) label() string { label := richtext.SanitizeSingleLine(l.Name) if email := richtext.SanitizeSingleLine(l.Email); email != "" { @@ -701,7 +703,10 @@ func (l *loginIdentity) label() string { if l.PersonID != 0 { parts = append(parts, fmt.Sprintf("person %d", l.PersonID)) } - if len(parts) > 0 { + switch { + case strings.TrimSpace(label) == "": + return strings.Join(parts, ", ") + case len(parts) > 0: label += " (" + strings.Join(parts, ", ") + ")" } return strings.TrimSpace(label) diff --git a/internal/commands/auth_identity_test.go b/internal/commands/auth_identity_test.go new file mode 100644 index 00000000..f17ced4e --- /dev/null +++ b/internal/commands/auth_identity_test.go @@ -0,0 +1,18 @@ +package commands + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestLoginIdentityLabelWithoutNameOrEmail: an authorization document that +// reports only an identity id (an in-house bc3 token's) is named by its +// ids, never as a blank name with the ids in brackets. +func TestLoginIdentityLabelWithoutNameOrEmail(t *testing.T) { + assert.Equal(t, "identity 28142355", (&loginIdentity{IdentityID: 28142355}).label()) + assert.Equal(t, "identity 28142355, person 51177542", (&loginIdentity{IdentityID: 28142355, PersonID: 51177542}).label()) + assert.Equal(t, "identity 28142355", (&loginIdentity{IdentityID: 28142355, Name: " \t"}).label()) + assert.Equal(t, "Ada (identity 28142355)", (&loginIdentity{IdentityID: 28142355, Name: "Ada", Email: "ada@example.com"}).label()) + assert.Equal(t, " (identity 28142355)", (&loginIdentity{IdentityID: 28142355, Email: "ada@example.com"}).label()) +} diff --git a/internal/commands/people.go b/internal/commands/people.go index fe2335ff..d62d4719 100644 --- a/internal/commands/people.go +++ b/internal/commands/people.go @@ -25,6 +25,18 @@ import ( type MeOutput struct { Identity basecamp.Identity `json:"identity"` Accounts []AccountInfo `json:"accounts"` + // Person is the account-scoped person record, looked up when the + // authorization document names no one (an in-house bc3 token reports + // only the identity id) and an account is configured to ask. + Person *MePerson `json:"person,omitempty"` +} + +// MePerson is the person the credential resolves to within the configured +// account. +type MePerson struct { + ID int64 `json:"id"` + Name string `json:"name"` + Email string `json:"email"` } // AccountInfo represents an account in the me command output @@ -67,12 +79,33 @@ func runMe(cmd *cobra.Command, args []string) error { return convertSDKError(err) } - // Store user email for display purposes (non-fatal if fails). - _ = app.Auth.SetUserEmail(authInfo.Identity.EmailAddress) + currentAccountID := app.Config.AccountID + + // Who the credential belongs to, for the summary and the stored label. + // The authorization document is the first word; an in-house (bc3) + // token's document carries only the identity id, and then the configured + // account's person record — the same lookup a login verifies against — + // fills in the name and email. That lookup is best-effort: a person the + // account cannot resolve leaves the identity as reported. + name := strings.TrimSpace(authInfo.Identity.FirstName + " " + authInfo.Identity.LastName) + email := authInfo.Identity.EmailAddress + var person *MePerson + if (name == "" || email == "") && app.RequireAccount() == nil && authorizesAccount(authInfo, currentAccountID) { + if p, err := app.Account().People().Me(cmd.Context()); err == nil { + person = &MePerson{ID: p.ID, Name: p.Name, Email: p.EmailAddress} + name, email = p.Name, p.EmailAddress + } + } + // Stored for display purposes (non-fatal if it fails). Empty values are + // omissions and leave what a login stored in place. + if person != nil { + _ = app.Auth.SetUserIdentity(strconv.FormatInt(person.ID, 10), person.Email) + } else { + _ = app.Auth.SetUserEmail(email) + } // Build account output (already filtered to bc3 by SDK) var accounts []AccountInfo - currentAccountID := app.Config.AccountID for _, acct := range authInfo.Accounts { info := AccountInfo{ ID: acct.ID, @@ -90,14 +123,10 @@ func runMe(cmd *cobra.Command, args []string) error { result := MeOutput{ Identity: authInfo.Identity, Accounts: accounts, + Person: person, } - // Build summary - name := authInfo.Identity.FirstName - if authInfo.Identity.LastName != "" { - name += " " + authInfo.Identity.LastName - } - summary := fmt.Sprintf("%s <%s>", name, authInfo.Identity.EmailAddress) + summary := meLabel(authInfo.Identity.ID, name, email) if len(accounts) > 0 { summary += fmt.Sprintf(" - %d Basecamp account(s)", len(accounts)) } @@ -130,6 +159,21 @@ func runMe(cmd *cobra.Command, args []string) error { ) } +// meLabel names the authenticated user for the summary line: "Name " +// with whichever parts are known, or the bare identity id when neither is. +func meLabel(identityID int64, name, email string) string { + switch { + case name != "" && email != "": + return name + " <" + email + ">" + case name != "": + return name + case email != "": + return email + default: + return fmt.Sprintf("identity %d", identityID) + } +} + // updateAccountsCache updates the completion cache with account data. // Runs synchronously; errors are ignored (best-effort). func updateAccountsCache(accounts []AccountInfo, cacheDir string) { diff --git a/internal/commands/people_test.go b/internal/commands/people_test.go index 038a150e..28a5cee5 100644 --- a/internal/commands/people_test.go +++ b/internal/commands/people_test.go @@ -1162,3 +1162,113 @@ func TestPeopleOutOfOfficeShowStatus(t *testing.T) { require.NoError(t, json.Unmarshal(buf.Bytes(), &result), "output: %s", buf.String()) assert.True(t, result.Data.Enabled) } + +// setupIdentityOnlyTestApp mirrors an in-house (bc3) token: the stored +// credential is bc5-typed against a server whose /authorization.json names +// only the identity id, and whose account person record answers with +// personStatus. The stored credential carries a user email a login left. +func setupIdentityOnlyTestApp(t *testing.T, personStatus int) (*appctx.App, *bytes.Buffer) { + t.Helper() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/authorization.json": + json.NewEncoder(w).Encode(map[string]any{ + "identity": map[string]any{"id": 28142355}, + "accounts": []map[string]any{{"id": 555, "name": "Token Corp", "href": "https://3.basecampapi.com/555", "product": "bc3"}}, + }) + case "/555/my/profile.json": + w.WriteHeader(personStatus) + if personStatus == http.StatusOK { + json.NewEncoder(w).Encode(map[string]any{"id": 51177542, "name": "Ada Lovelace", "email_address": "ada@example.com"}) + } + default: + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("BASECAMP_NO_KEYRING", "1") + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + cfg := &config.Config{AccountID: "555", BaseURL: server.URL, CacheDir: t.TempDir()} + authMgr := auth.NewManager(cfg, nil) + authMgr.SetStore(auth.NewStore(filepath.Join(tmpDir, "basecamp"))) + require.NoError(t, authMgr.GetStore().Save(config.NormalizeBaseURL(server.URL), &auth.Credentials{ + AccessToken: "bc_at_stored", + OAuthType: "bc5", + UserID: "1", + UserEmail: "kept@example.com", + ExpiresAt: 9999999999, + })) + + buf := &bytes.Buffer{} + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: server.URL}, &peopleTestTokenProvider{}, basecamp.WithMaxRetries(1)) + app := &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), + Flags: appctx.GlobalFlags{Hints: true}, + } + return app, buf +} + +type meEnvelope struct { + Summary string `json:"summary"` + Data struct { + Identity struct { + ID int64 `json:"id"` + } `json:"identity"` + Person *struct { + ID int64 `json:"id"` + Name string `json:"name"` + Email string `json:"email"` + } `json:"person"` + } `json:"data"` +} + +// TestMeIdentityOnlyResolvesThePerson: an authorization document with only +// an identity id is completed from the configured account's person record, +// which the summary and the stored label then use. +func TestMeIdentityOnlyResolvesThePerson(t *testing.T) { + app, buf := setupIdentityOnlyTestApp(t, http.StatusOK) + + require.NoError(t, executePeopleCommand(NewMeCmd(), app)) + + var envelope meEnvelope + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Equal(t, "Ada Lovelace - 1 Basecamp account(s)", envelope.Summary) + assert.Equal(t, int64(28142355), envelope.Data.Identity.ID) + require.NotNil(t, envelope.Data.Person) + assert.Equal(t, int64(51177542), envelope.Data.Person.ID) + assert.Equal(t, "ada@example.com", envelope.Data.Person.Email) + + creds, err := app.Auth.GetStore().Load(app.Auth.CredentialKey()) + require.NoError(t, err) + assert.Equal(t, "51177542", creds.UserID) + assert.Equal(t, "ada@example.com", creds.UserEmail) +} + +// TestMeIdentityOnlyFallsBackToTheIdentity: when the person lookup fails, +// the summary names the identity id — never a blank " <>" — and the stored +// email is left as the login wrote it. +func TestMeIdentityOnlyFallsBackToTheIdentity(t *testing.T) { + app, buf := setupIdentityOnlyTestApp(t, http.StatusNotFound) + + require.NoError(t, executePeopleCommand(NewMeCmd(), app)) + + var envelope meEnvelope + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Equal(t, "identity 28142355 - 1 Basecamp account(s)", envelope.Summary) + assert.Nil(t, envelope.Data.Person) + + creds, err := app.Auth.GetStore().Load(app.Auth.CredentialKey()) + require.NoError(t, err) + assert.Equal(t, "1", creds.UserID) + assert.Equal(t, "kept@example.com", creds.UserEmail, "an omitted email must not blank the stored one") +} From 2dfb2c3de5d2939210e36566ef0b73b1fff469fa Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:44:47 -0700 Subject: [PATCH 2/4] Keep me from writing an environment token's identity over a stored one `me` now persists the person it resolves, and under BASECAMP_TOKEN that person belongs to the environment token, not to whatever credential is stored for the profile. SetUserIdentity takes the rule SetUserEmail already had: a BASECAMP_TOKEN session writes nothing. --- internal/auth/auth.go | 6 ++++-- internal/auth/auth_test.go | 18 ++++++++++++++++++ internal/commands/people_test.go | 19 +++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index dd76c0c4..0c60fdba 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -1515,9 +1515,11 @@ func (m *Manager) SetUserEmail(email string) error { } // SetUserIdentity stores the user ID and email for the current credential -// key. As with SetUserEmail, an empty value leaves the stored field alone. +// key. As with SetUserEmail, an empty value leaves the stored field alone, +// and a BASECAMP_TOKEN session writes nothing: what it learned names the +// environment token's user, not whoever the stored credential belongs to. func (m *Manager) SetUserIdentity(userID, email string) error { - if userID == "" && email == "" { + if os.Getenv("BASECAMP_TOKEN") != "" || (userID == "" && email == "") { return nil } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 4600c07d..4311cfde 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2194,3 +2194,21 @@ func TestSetUserIdentity_EmptyValuesAreOmissions(t *testing.T) { require.NoError(t, m.SetUserEmail("new@example.com")) assert.Equal(t, "new@example.com", m.GetUserEmail()) } + +// TestSetUserIdentity_EnvTokenWritesNothing: under BASECAMP_TOKEN the +// identity belongs to the environment token, so the stored credential is +// left alone — the same rule SetUserEmail applies. +func TestSetUserIdentity_EnvTokenWritesNothing(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "bc_at_env") + m := &Manager{cfg: config.Default(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "tok", OAuthType: "bc5", UserID: "1", UserEmail: "kept@example.com", + })) + + require.NoError(t, m.SetUserIdentity("2", "other@example.com")) + creds, err := m.store.Load(key) + require.NoError(t, err) + assert.Equal(t, "1", creds.UserID) + assert.Equal(t, "kept@example.com", creds.UserEmail) +} diff --git a/internal/commands/people_test.go b/internal/commands/people_test.go index 28a5cee5..e6f1f87e 100644 --- a/internal/commands/people_test.go +++ b/internal/commands/people_test.go @@ -1272,3 +1272,22 @@ func TestMeIdentityOnlyFallsBackToTheIdentity(t *testing.T) { assert.Equal(t, "1", creds.UserID) assert.Equal(t, "kept@example.com", creds.UserEmail, "an omitted email must not blank the stored one") } + +// TestMeUnderEnvTokenLeavesStoredIdentityAlone: the person `me` resolves +// for BASECAMP_TOKEN belongs to that token, so it is shown but never +// written over the stored credential's identity. +func TestMeUnderEnvTokenLeavesStoredIdentityAlone(t *testing.T) { + app, buf := setupIdentityOnlyTestApp(t, http.StatusOK) + t.Setenv("BASECAMP_TOKEN", "bc_at_env") + + require.NoError(t, executePeopleCommand(NewMeCmd(), app)) + + var envelope meEnvelope + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Equal(t, "Ada Lovelace - 1 Basecamp account(s)", envelope.Summary) + + creds, err := app.Auth.GetStore().Load(app.Auth.CredentialKey()) + require.NoError(t, err) + assert.Equal(t, "1", creds.UserID) + assert.Equal(t, "kept@example.com", creds.UserEmail) +} From 59e3bb072784c0b021f32b04ac39e311a59014b7 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:59:50 -0700 Subject: [PATCH 3/4] Skip the identity write in me under BASECAMP_TOKEN, not in the manager Guarding SetUserIdentity itself also silenced the write a login makes after storing its new credential, which runs under BASECAMP_TOKEN too and would have left that credential without its verified user. The guard now sits where the environment token's identity is learned: me skips the write, logins keep it. The person record fills gaps in the authorization document rather than replacing it: an email the document named survives a record that omits its own, and the merged email is what gets stored. --- internal/auth/auth.go | 9 +++--- internal/auth/auth_test.go | 18 +++++------- internal/commands/people.go | 22 ++++++++++---- internal/commands/people_test.go | 50 ++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 19 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 0c60fdba..96964ce8 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -1515,11 +1515,12 @@ func (m *Manager) SetUserEmail(email string) error { } // SetUserIdentity stores the user ID and email for the current credential -// key. As with SetUserEmail, an empty value leaves the stored field alone, -// and a BASECAMP_TOKEN session writes nothing: what it learned names the -// environment token's user, not whoever the stored credential belongs to. +// key. As with SetUserEmail, an empty value leaves the stored field alone. +// Unlike it, BASECAMP_TOKEN does not suppress the write: a login stores +// its new credential and then records who it verified as, whatever the +// environment holds, so the caller decides whose identity this is. func (m *Manager) SetUserIdentity(userID, email string) error { - if os.Getenv("BASECAMP_TOKEN") != "" || (userID == "" && email == "") { + if userID == "" && email == "" { return nil } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 4311cfde..805451f1 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2195,20 +2195,18 @@ func TestSetUserIdentity_EmptyValuesAreOmissions(t *testing.T) { assert.Equal(t, "new@example.com", m.GetUserEmail()) } -// TestSetUserIdentity_EnvTokenWritesNothing: under BASECAMP_TOKEN the -// identity belongs to the environment token, so the stored credential is -// left alone — the same rule SetUserEmail applies. -func TestSetUserIdentity_EnvTokenWritesNothing(t *testing.T) { +// TestSetUserIdentity_WritesUnderEnvToken: a login records who its new +// credential verified as whatever BASECAMP_TOKEN holds; the environment +// token is the caller's concern (me skips the write), not this method's. +func TestSetUserIdentity_WritesUnderEnvToken(t *testing.T) { t.Setenv("BASECAMP_TOKEN", "bc_at_env") m := &Manager{cfg: config.Default(), store: newTestStore(t, t.TempDir())} key := m.credentialKey() - require.NoError(t, m.store.Save(key, &Credentials{ - AccessToken: "tok", OAuthType: "bc5", UserID: "1", UserEmail: "kept@example.com", - })) + require.NoError(t, m.store.Save(key, &Credentials{AccessToken: "tok", OAuthType: "bc5"})) - require.NoError(t, m.SetUserIdentity("2", "other@example.com")) + require.NoError(t, m.SetUserIdentity("2", "who@example.com")) creds, err := m.store.Load(key) require.NoError(t, err) - assert.Equal(t, "1", creds.UserID) - assert.Equal(t, "kept@example.com", creds.UserEmail) + assert.Equal(t, "2", creds.UserID) + assert.Equal(t, "who@example.com", creds.UserEmail) } diff --git a/internal/commands/people.go b/internal/commands/people.go index d62d4719..1c7a7f79 100644 --- a/internal/commands/people.go +++ b/internal/commands/people.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/mail" + "os" "slices" "sort" "strconv" @@ -93,14 +94,25 @@ func runMe(cmd *cobra.Command, args []string) error { if (name == "" || email == "") && app.RequireAccount() == nil && authorizesAccount(authInfo, currentAccountID) { if p, err := app.Account().People().Me(cmd.Context()); err == nil { person = &MePerson{ID: p.ID, Name: p.Name, Email: p.EmailAddress} - name, email = p.Name, p.EmailAddress + // The person record fills gaps; a field the authorization document + // already named is kept when the record omits it. + if p.Name != "" { + name = p.Name + } + if p.EmailAddress != "" { + email = p.EmailAddress + } } } // Stored for display purposes (non-fatal if it fails). Empty values are - // omissions and leave what a login stored in place. - if person != nil { - _ = app.Auth.SetUserIdentity(strconv.FormatInt(person.ID, 10), person.Email) - } else { + // omissions and leave what a login stored in place. Under BASECAMP_TOKEN + // nothing is written: what was learned names the environment token's + // user, not whoever the stored credential belongs to. + switch { + case os.Getenv("BASECAMP_TOKEN") != "": + case person != nil: + _ = app.Auth.SetUserIdentity(strconv.FormatInt(person.ID, 10), email) + default: _ = app.Auth.SetUserEmail(email) } diff --git a/internal/commands/people_test.go b/internal/commands/people_test.go index e6f1f87e..8d725ee1 100644 --- a/internal/commands/people_test.go +++ b/internal/commands/people_test.go @@ -1291,3 +1291,53 @@ func TestMeUnderEnvTokenLeavesStoredIdentityAlone(t *testing.T) { assert.Equal(t, "1", creds.UserID) assert.Equal(t, "kept@example.com", creds.UserEmail) } + +// TestMeKeepsTheIdentityEmailWhenThePersonHasNone: the person record fills +// gaps in the authorization document; a field the document already named +// is not replaced by the record's omission. +func TestMeKeepsTheIdentityEmailWhenThePersonHasNone(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/authorization.json": + json.NewEncoder(w).Encode(map[string]any{ + "identity": map[string]any{"id": 28142355, "email_address": "identity@example.com"}, + "accounts": []map[string]any{{"id": 555, "name": "Token Corp", "product": "bc3"}}, + }) + case "/555/my/profile.json": + json.NewEncoder(w).Encode(map[string]any{"id": 51177542, "name": "Ada Lovelace"}) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("BASECAMP_NO_KEYRING", "1") + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + cfg := &config.Config{AccountID: "555", BaseURL: server.URL, CacheDir: t.TempDir()} + authMgr := auth.NewManager(cfg, nil) + authMgr.SetStore(auth.NewStore(filepath.Join(tmpDir, "basecamp"))) + require.NoError(t, authMgr.GetStore().Save(config.NormalizeBaseURL(server.URL), &auth.Credentials{ + AccessToken: "bc_at_stored", OAuthType: "bc5", ExpiresAt: 9999999999, + })) + buf := &bytes.Buffer{} + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: server.URL}, &peopleTestTokenProvider{}, basecamp.WithMaxRetries(1)) + app := &appctx.App{ + Config: cfg, Auth: authMgr, SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), + } + + require.NoError(t, executePeopleCommand(NewMeCmd(), app)) + + var envelope meEnvelope + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Equal(t, "Ada Lovelace - 1 Basecamp account(s)", envelope.Summary) + + creds, err := authMgr.GetStore().Load(authMgr.CredentialKey()) + require.NoError(t, err) + assert.Equal(t, "51177542", creds.UserID) + assert.Equal(t, "identity@example.com", creds.UserEmail, "the merged email is what gets stored") +} From 2cd06d498eb42dff229631adb6f675db6158432e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 19:08:40 -0700 Subject: [PATCH 4/4] Let the person record fill only what the authorization document left empty The merge tested the record's fields for emptiness rather than the document's, so an identity with its canonical email but no name had that email replaced by the account person's. The document's values now stay as its own; the record fills the empty ones. --- internal/commands/people.go | 8 ++++---- internal/commands/people_test.go | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/commands/people.go b/internal/commands/people.go index 1c7a7f79..16cf9122 100644 --- a/internal/commands/people.go +++ b/internal/commands/people.go @@ -94,12 +94,12 @@ func runMe(cmd *cobra.Command, args []string) error { if (name == "" || email == "") && app.RequireAccount() == nil && authorizesAccount(authInfo, currentAccountID) { if p, err := app.Account().People().Me(cmd.Context()); err == nil { person = &MePerson{ID: p.ID, Name: p.Name, Email: p.EmailAddress} - // The person record fills gaps; a field the authorization document - // already named is kept when the record omits it. - if p.Name != "" { + // The person record fills gaps only; a field the authorization + // document already named is kept as its own. + if name == "" { name = p.Name } - if p.EmailAddress != "" { + if email == "" { email = p.EmailAddress } } diff --git a/internal/commands/people_test.go b/internal/commands/people_test.go index 8d725ee1..b2eb15d3 100644 --- a/internal/commands/people_test.go +++ b/internal/commands/people_test.go @@ -1292,10 +1292,10 @@ func TestMeUnderEnvTokenLeavesStoredIdentityAlone(t *testing.T) { assert.Equal(t, "kept@example.com", creds.UserEmail) } -// TestMeKeepsTheIdentityEmailWhenThePersonHasNone: the person record fills -// gaps in the authorization document; a field the document already named -// is not replaced by the record's omission. -func TestMeKeepsTheIdentityEmailWhenThePersonHasNone(t *testing.T) { +// TestMeKeepsTheIdentityEmailOverThePersons: the person record fills gaps +// in the authorization document only; a field the document already named +// is kept even when the record carries a different value. +func TestMeKeepsTheIdentityEmailOverThePersons(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") switch r.URL.Path { @@ -1305,7 +1305,7 @@ func TestMeKeepsTheIdentityEmailWhenThePersonHasNone(t *testing.T) { "accounts": []map[string]any{{"id": 555, "name": "Token Corp", "product": "bc3"}}, }) case "/555/my/profile.json": - json.NewEncoder(w).Encode(map[string]any{"id": 51177542, "name": "Ada Lovelace"}) + json.NewEncoder(w).Encode(map[string]any{"id": 51177542, "name": "Ada Lovelace", "email_address": "person@example.com"}) default: http.NotFound(w, r) }