fix: get token for active user instead of blank if possible #11038

anuraaga · 2025-05-29T03:17:19Z

I have been following this issue for a while but finally tried checking the code. It seemed pretty simple to allow the GH_CONFIG_DIR switching mechanism to work, and the change seems easy to reason about to me so tried sending a PR.

Note that when using login, we set the username-keyed token

cli/internal/config/config.go

Line 345 in c50cdbd

setErr = keyring.Set(keyringServiceName(hostname), username, token)

so as far as I'm aware, there aren't really any gotchas, and in fact we could potentially remove the fallback to "" - I left it for safety in case there are still some cases not covered by getting the token for the active user. If there are old versions that didn't populate the username-keyed value to keyring when logging in, then I guess they may rely on the fallback.

pkg/cmd/api/api_test.go

Copilot

Pull Request Overview

Fixes token lookup to prefer an active user’s keyring entry before falling back to the blank key, and updates tests to cover and stub that behavior.

Prioritize user-specific token in TokenFromKeyring
Add new test to verify active-user token selection
Stub ActiveToken in API tests for consistent behavior

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
internal/config/config.go	Added logic in `TokenFromKeyring` to try `ActiveUser` key first
internal/config/auth_config_test.go	New test `TestTokenFromKeyringActiveUserNotBlankUser`
pkg/cmd/api/api_test.go	Introduced `stubAuthConfig` and updated `Config` closure signature for API tests

Comments suppressed due to low confidence (4)

pkg/cmd/api/api_test.go:1346

[nitpick] The name stubAuthConfig is ambiguous; consider renaming to mockAuthConfig or fakeAuthConfig for clearer intent.

type stubAuthConfig struct {

internal/config/auth_config_test.go:45

[nitpick] Consider renaming this to TestTokenFromKeyring_PrioritizesActiveUserToken to more explicitly describe the expected behavior.

func TestTokenFromKeyringActiveUserNotBlankUser(t *testing.T) {

internal/config/auth_config_test.go:73

Add a test case for when an active user is set but no user-specific keyring entry exists, ensuring it falls back to the default (blank) token.

token, err = authCfg.TokenFromKeyring("github.com")

pkg/cmd/api/api_test.go:1368

The named return parameters cfg and err aren’t used and add noise; revert to the unnamed signature func() (gh.Config, error) to match surrounding tests.

Config: func() (cfg gh.Config, err error) {

williammartin · 2025-05-29T14:36:41Z

internal/config/config.go

@@ -281,6 +281,14 @@ func (c *AuthConfig) SetActiveToken(token, source string) {
 // TokenFromKeyring will retrieve the auth token for the given hostname,
 // only searching in encrypted storage.
 func (c *AuthConfig) TokenFromKeyring(hostname string) (string, error) {
+	if user, err := c.ActiveUser(hostname); err == nil && user != "" {


I don't have time to review this properly @andyfeller but as the person that introduced 🌚 and triaged the original issue, I'm pretty sure this is conceptually what I had in mind as a first attempt at fixing this.

This wasn't marked as help-wanted but I do think its pretty bad, so I would welcome us prioritising this fix. I actually brought up wanting to spend some time fixing multi-account sharp edges this week with @mxie

Thanks @williammartin - just wondering if it's possible for anyone to review this? I think there's still more that can be done to generally improve UX of multi-auth, but hopefully this fix is contained and easy enough to reason about to make some steps towards that.

Mainly just a matter of priority. We're all meeting offsite this week so there's been a lot of travelling. Would love for you to drop your thoughts on improving multi-auth UX in a new discussion. I also have lots of thoughts, and again, it's really a matter of having the evidence to prioritise it.

Hopefully finished with offsites and such - would it be possible to get a review on this? I get priority for feature requests, but this is a bug fix so seems like it should generally be prioritized...

Thank you, @williammartin, for picking up the slack 🙇 Having returned from offiste and trips, I'm catching up on this PR now.

williammartin · 2025-06-17T14:04:49Z

Acceptance Criteria

Given I have two accounts on a single github host
And Given I have each of these accounts as the active user in separate hosts.yml files
When I set GH_CONFIG_DIR to either of the files
And When I run gh api /user .login
Then I see the active user is correct as per the hosts.yml file

# Current version
➜ gh version
gh version 2.74.1 (2025-06-10)
https://github.com/cli/cli/releases/tag/v2.74.1

# auth status
➜ gh auth status --hostname github.com
github.com
  ✓ Logged in to github.com account wilmartin_microsoft (keyring)
  - Active account: true
  - Git operations protocol: https
  - Token: gho_************************************
  - Token scopes: 'gist', 'read:org', 'repo', 'workflow'

  ✓ Logged in to github.com account williammartin (keyring)
  - Active account: false
  - Git operations protocol: https
  - Token: gho_************************************
  - Token scopes: 'gist', 'read:org', 'repo', 'workflow'

# Current hosts.yml
➜ cat ~/.config/gh/hosts.yml
github.com:
    git_protocol: https
    users:
        williammartin: {}
        wilmartin_microsoft:
    user: wilmartin_microsoft

# Hit the API to see the token in use
➜ gh api /user --jq .login | cat
wilmartin_microsoft

# Adjust the hosts.yml (as if swapping GH_CONFIG_DIR)

8000
➜  cat ~/.config/gh/hosts.yml
github.com:
    git_protocol: https
    users:
        williammartin: {}
        wilmartin_microsoft:
    user: williammartin
    
# Hit the API to see that the same (wrong token) is in use
➜ gh api /user --jq .login | cat
wilmartin_microsoft

# Build this branch
➜  cli git:(active-token-user) ✗ make
go build -trimpath -ldflags "-X github.com/cli/cli/v2/internal/build.Date=2025-06-17 -X github.com/cli/cli/v2/internal/build.Version=v2.74.0-7-g8deae3038 " -o bin/gh ./cmd/gh

# Set that it uses the correct token
➜ ./bin/gh api /user --jq .login | cat
williammartin

# And that auth status has correctly updated
➜ gh auth status --hostname github.com
github.com
  ✓ Logged in to github.com account williammartin (keyring)
  - Active account: true
  - Git operations protocol: https
  - Token: gho_************************************
  - Token scopes: 'gist', 'read:org', 'repo', 'workflow'

  ✓ Logged in to github.com account wilmartin_microsoft (keyring)
  - Active account: false
  - Git operations protocol: https
  - Token: gho_************************************
  - Token scopes: 'gist', 'read:org', 'repo', 'workflow'

williammartin

@anuraaga this is functionally correct but I think that perhaps we should approach this slightly differently. I think maybe we should take the logic in TokenFromKeyring and push it into ActiveToken. I think the ActiveToken / ActiveUser parallel is stronger and then maybe I'll come back later and start renaming things like TokenFromKeyring to indicate their legacy, or fallback nature.

Is there some reason you think this wouldn't work?

anuraaga · 2025-06-17T14:26:28Z

Thanks @williammartin - unless I'm missing something since this is all in /internal/ there wouldn't even need to be renames for legacy reasons, we can just go for the end state in one shot. If the current separation is desired for readability reasons that seems fine, if pushing up is clearer that seems fine too. I can tweak this PR if there's any preference.

williammartin · 2025-06-17T14:29:37Z

@anuraaga the renames would be for our maintainability. I just think I need to poke around a bit more to decide exactly what name I'd want, which is why I'm not suggesting you do it. I would like you to try and move the logic into Active 8000 Token so that it does the ActiveUser -> TokenFromKeyringForUser -> Fallback to TokenFromKeyring.

…user

anuraaga · 2025-06-18T00:57:41Z

Thanks @williammartin - moved the logic to ActiveToken, hopefully it looks ok

- ensure test user tokens are different from unkeyed token - ensure assertion expected / actual are in correct order

williammartin · 2025-06-20T18:40:45Z

internal/config/auth_config_test.go

@@ -773,7 +773,7 @@ func TestTokenPrioritizesActiveUserToken(t *testing.T) {
 	// Given a keyring that contains a token for a host
 	authCfg := newTestAuthConfig(t)
 	require.NoError(t, keyring.Set(keyringServiceName("github.com"), "", "test-token"))
-	require.NoError(t, keyring.Set(keyringServiceName("github.com"), "test-user1", "test-token"))
+	require.NoError(t, keyring.Set(keyringServiceName("github.com"), "test-user1", "test-token1"))


This isn't really representative of the most common case. In nearly all cases the token in the empty username keyring entry will match one of the username keyring entries. The previous test is much more likely to be the case.

andyfeller · 2025-06-20T18:41:33Z

Thank you for your patience, @anuraaga; my sincere apologies for not following up sooner! 🙇

I made a minor enhancement to one of the tests to differentiate the tokens and swapped order of test assertion. Just seeing CI checks pass for final review.

After discussing my previous change to the test, I'm restoring the previous keyring setup to reflect the specific situation. I added clarifying comments to help the next reviewer.

williammartin · 2025-06-25T14:42:55Z

Think this is probably ready to go @andyfeller ?

andyfeller

fix: get token for active user instead of blank if possible

f294a5f

Copilot AI review requested due to automatic review settings May 29, 2025 03:17

anuraaga requested a review from a team as a code owner May 29, 2025 03:17

anuraaga requested a review from andyfeller May 29, 2025 03:17

anuraaga temporarily deployed to cli-automation May 29, 2025 03:17 — with GitHub Actions Inactive

cliAutomation added the external pull request originating outside of the CLI core team label May 29, 2025

anuraaga commented May 29, 2025

View reviewed changes

pkg/cmd/api/api_test.go Outdated Show resolved Hide resolved

Copilot AI reviewed May 29, 2025

View reviewed changes

Cleanup

cc9a241

williammartin reviewed May 29, 2025

View reviewed changes

anuraaga and others added 2 commits June 3, 2025 09:43

Merge branch 'trunk' into active-token-user

85c8653

Use active token stubbing on auth config

8deae30

williammartin reviewed Jun 17, 2025

View reviewed changes

anuraaga added 2 commits June 18, 2025 09:56

Push up

848cedd

Merge branch 'trunk' of https://github.com/cli/cli into active-token-…

12a4133

…user

Enhance Activetoken prioritize test

b7c2b19

- ensure test user tokens are different from unkeyed token - ensure assertion expected / actual are in correct order

williammartin reviewed Jun 20, 2025

View reviewed changes

Restored original test setup, clarified

0180c7f

After discussing my previous change to the test, I'm restoring the previous keyring setup to reflect the specific situation. I added clarifying comments to help the next reviewer.

andyfeller approved these changes Jun 27, 2025

View reviewed changes

andyfeller merged commit e5913fe into cli:trunk Jun 27, 2025
10 checks passed

github-project-automation bot moved this to Done in VITA PRO MAX@MOSTAFAAMMER Jun 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: get token for active user instead of blank if possible #11038

fix: get token for active user instead of blank if possible #11038

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix: get token for active user instead of blank if possible #11038

fix: get token for active user instead of blank if possible #11038

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Acceptance Criteria

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!