diff --git a/internal/auth/auth.go b/internal/auth/auth.go index c7a39610..96964ce8 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,27 @@ 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. +// 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 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..805451f1 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2167,3 +2167,46 @@ 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()) +} + +// 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"})) + + require.NoError(t, m.SetUserIdentity("2", "who@example.com")) + creds, err := m.store.Load(key) + require.NoError(t, err) + assert.Equal(t, "2", creds.UserID) + assert.Equal(t, "who@example.com", creds.UserEmail) +} 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..16cf9122 100644 --- a/internal/commands/people.go +++ b/internal/commands/people.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/mail" + "os" "slices" "sort" "strconv" @@ -25,6 +26,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 +80,44 @@ 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} + // The person record fills gaps only; a field the authorization + // document already named is kept as its own. + if name == "" { + name = p.Name + } + if email == "" { + email = p.EmailAddress + } + } + } + // Stored for display purposes (non-fatal if it fails). Empty values are + // 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) + } // 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 +135,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 +171,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..b2eb15d3 100644 --- a/internal/commands/people_test.go +++ b/internal/commands/people_test.go @@ -1162,3 +1162,182 @@ 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") +} + +// 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) +} + +// 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 { + 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", "email_address": "person@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", 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") +}