From 3f3dc302c27ce746cb408c054b6d2111afb424bc Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 12 Aug 2026 14:23:26 -0400 Subject: [PATCH] fix(me): fall back to the Gmail profile when People returns no email 'gro me' printed the display name but a blank ('-') email, while 'config test' printed 'Authenticated as: ' - inconsistent identity output, with the canonical identity command being the less useful one. Root cause: people/me only returns the account's own email when the token carries an email-bearing scope. gro requests userinfo.profile but not userinfo.email, so People comes back email-less; the Gmail profile always knows the address under the always-granted gmail.modify scope. Fall back to the Gmail profile when People's email is blank (one extra API call, only in that case; best-effort - a fallback failure degrades to the old '-' rendering rather than failing me). Covers the one-liner and --id. Deliberately NOT fixed by adding the userinfo.email scope: a scope-set change trips the scope-drift gate and forces every existing user through re-auth, for data we can already read. --- internal/cmd/me/me.go | 16 ++++++ internal/cmd/me/me_test.go | 100 +++++++++++++++++++++++++++++++++++++ internal/cmd/me/output.go | 19 +++++++ 3 files changed, 135 insertions(+) diff --git a/internal/cmd/me/me.go b/internal/cmd/me/me.go index 6cd4078..a8bd3d7 100644 --- a/internal/cmd/me/me.go +++ b/internal/cmd/me/me.go @@ -91,6 +91,22 @@ func run(ctx context.Context, out, errOut io.Writer, idOnly, extended bool) erro return fmt.Errorf("getting current user: %w", err) } + // People's people/me only returns the account's own email when the token + // carries an email-bearing scope; gro requests userinfo.profile but not + // userinfo.email, so PrimaryEmail is blank on most tokens — while `config + // test` (Gmail profile) happily prints the address. `me` is the canonical + // identity surface, so fall back to the Gmail profile (gmail.modify is + // always granted) rather than rendering "-" where the email belongs. + // Best-effort: a fallback failure leaves the field blank instead of + // failing `me` — the People data is still worth showing. Adding the + // userinfo.email scope instead would force every existing user through a + // re-auth for data we can already read. + if profile.PrimaryEmail == "" { + if email, gerr := GmailEmailFactory(ctx); gerr == nil { + profile.PrimaryEmail = email + } + } + // Read token expiry AFTER the API call so any refresh by // PersistentTokenSource is reflected. var extras Extras diff --git a/internal/cmd/me/me_test.go b/internal/cmd/me/me_test.go index a3e4730..91c300c 100644 --- a/internal/cmd/me/me_test.go +++ b/internal/cmd/me/me_test.go @@ -35,6 +35,15 @@ func withMockClient(t *testing.T, c PeopleClient) { t.Cleanup(func() { ClientFactory = orig }) } +// withMockGmailEmail swaps GmailEmailFactory for the test and restores it on +// cleanup. +func withMockGmailEmail(t *testing.T, f func(ctx context.Context) (string, error)) { + t.Helper() + orig := GmailEmailFactory + GmailEmailFactory = f + t.Cleanup(func() { GmailEmailFactory = orig }) +} + func TestRenderOneLinerHappyPath(t *testing.T) { t.Parallel() var buf bytes.Buffer @@ -177,6 +186,97 @@ func TestRunDefaultPipeOneLiner(t *testing.T) { } } +// TestRunFallsBackToGmailEmail is the regression test for the blank-email +// one-liner: gro requests userinfo.profile but not userinfo.email, so +// people/me returns no email and `gro me` printed "-" where `config test` +// printed the address. The Gmail-profile fallback must fill it in. +func TestRunFallsBackToGmailEmail(t *testing.T) { + // Not Parallel: mutates package-global ClientFactory + GmailEmailFactory. + withMockClient(t, &mockPeopleClient{ + GetMeFunc: func(_ context.Context) (*people.Profile, error) { + return &people.Profile{ + ResourceName: "people/c1", + DisplayName: "Ada", + PrimaryEmail: "", // what people/me returns without userinfo.email + }, nil + }, + }) + withMockGmailEmail(t, func(_ context.Context) (string, error) { + return "ada@example.com", nil + }) + var out bytes.Buffer + if err := run(context.Background(), &out, &bytes.Buffer{}, false, false); err != nil { + t.Fatal(err) + } + want := "people/c1 | Ada | ada@example.com\n" + if got := out.String(); got != want { + t.Fatalf("got %q, want %q", got, want) + } +} + +// TestRunFallbackFailureLeavesEmailBlank proves a Gmail hiccup degrades to +// the old "-" rendering instead of failing `me` — the People data is still +// worth showing. +func TestRunFallbackFailureLeavesEmailBlank(t *testing.T) { + // Not Parallel: mutates package-global ClientFactory + GmailEmailFactory. + withMockClient(t, &mockPeopleClient{ + GetMeFunc: func(_ context.Context) (*people.Profile, error) { + return &people.Profile{ResourceName: "people/c1", DisplayName: "Ada"}, nil + }, + }) + withMockGmailEmail(t, func(_ context.Context) (string, error) { + return "", errors.New("gmail unavailable") + }) + var out bytes.Buffer + if err := run(context.Background(), &out, &bytes.Buffer{}, false, false); err != nil { + t.Fatal(err) + } + want := "people/c1 | Ada | -\n" + if got := out.String(); got != want { + t.Fatalf("got %q, want %q", got, want) + } +} + +// TestRunNoFallbackWhenEmailPresent proves the extra Gmail call is skipped +// when People already supplied the email. +func TestRunNoFallbackWhenEmailPresent(t *testing.T) { + // Not Parallel: mutates package-global ClientFactory + GmailEmailFactory. + withMockClient(t, &mockPeopleClient{ + GetMeFunc: func(_ context.Context) (*people.Profile, error) { + return &people.Profile{ResourceName: "people/c1", DisplayName: "Ada", PrimaryEmail: "ada@example.com"}, nil + }, + }) + withMockGmailEmail(t, func(_ context.Context) (string, error) { + t.Error("GmailEmailFactory must not be called when People supplied the email") + return "", nil + }) + var out bytes.Buffer + if err := run(context.Background(), &out, &bytes.Buffer{}, false, false); err != nil { + t.Fatal(err) + } +} + +// TestRunIDOnlyFallsBackToGmailEmail covers the scripting surface: `gro me +// --id` printing "-" is worse than useless in a pipeline. +func TestRunIDOnlyFallsBackToGmailEmail(t *testing.T) { + // Not Parallel: mutates package-global ClientFactory + GmailEmailFactory. + withMockClient(t, &mockPeopleClient{ + GetMeFunc: func(_ context.Context) (*people.Profile, error) { + return &people.Profile{ResourceName: "people/c1", DisplayName: "Ada"}, nil + }, + }) + withMockGmailEmail(t, func(_ context.Context) (string, error) { + return "ada@example.com", nil + }) + var out bytes.Buffer + if err := run(context.Background(), &out, &bytes.Buffer{}, true, false); err != nil { + t.Fatal(err) + } + if got := out.String(); got != "ada@example.com\n" { + t.Fatalf("got %q, want 'ada@example.com\\n'", got) + } +} + func TestRunIDOnlyEmitsEmail(t *testing.T) { // Not Parallel: mutates package-global ClientFactory. withMockClient(t, &mockPeopleClient{ diff --git a/internal/cmd/me/output.go b/internal/cmd/me/output.go index f770a59..f63203c 100644 --- a/internal/cmd/me/output.go +++ b/internal/cmd/me/output.go @@ -6,6 +6,7 @@ import ( "io" "strings" + "github.com/open-cli-collective/google-cli-common/gmail" "github.com/open-cli-collective/google-cli-common/keychain" "github.com/open-cli-collective/google-cli-common/people" ) @@ -20,6 +21,24 @@ var ClientFactory = func(ctx context.Context) (PeopleClient, error) { return people.NewClient(ctx) } +// GmailEmailFactory resolves the authenticated account's email address via +// the Gmail profile. It backs the fallback for People's missing email (see +// run): gro requests userinfo.profile but deliberately not userinfo.email, +// so people/me carries no email address on most tokens — while the Gmail +// profile always knows it under the always-granted gmail.modify scope. +// Override in tests to inject mocks. +var GmailEmailFactory = func(ctx context.Context) (string, error) { + c, err := gmail.NewClient(ctx) + if err != nil { + return "", err + } + p, err := c.GetProfile(ctx) + if err != nil { + return "", err + } + return p.EmailAddress, nil +} + // Extras is the data shown by --extended that doesn't come from People. type Extras struct { GrantedScopes []string