Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andyfeller merged 8 commits into from
Jun 27, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f294a5f
fix: get token for active user instead of blank if possible
anuraaga May 29, 2025
cc9a241
Cleanup
anuraaga May 29, 2025
85c8653
Merge branch 'trunk' into active-token-user
anuraaga Jun 3, 2025
8deae30
Use active token stubbing on auth config
williammartin Jun 17, 2025
848cedd
Push up
anuraaga Jun 18, 2025
12a4133
Merge branch 'trunk' of https://github.com/cli/cli into active-token-…
anuraaga Jun 18, 2025
b7c2b19
Enhance Activetoken prioritize test
andyfeller Jun 20, 2025
0180c7f
Restored original test setup, clarified
andyfeller Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions internal/config/auth_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,62 @@ func TestTokenWorksRightAfterMigration(t *testing.T) {
require.Equal(t, oauthTokenKey, source)
}

func TestTokenPrioritizesActiveUserToken(t *testing.T) {
// Given a keyring where the active slot contains the token from a previous user
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-user2", "test-token2"))

// When no active user is set
authCfg.cfg.Remove([]string{hostsKey, "github.com", userKey})

// And get the token from the auth config
token, source := authCfg.ActiveToken("github.com")

// Then it returns the token from the keyring active slot
require.Equal(t, "keyring", source)
require.Equal(t, "test-token", token)

// When we set the active user to test-user1
authCfg.cfg.Set([]string{hostsKey, "github.com", userKey}, "test-user1")

// And get the token from the auth config
token, source = authCfg.ActiveToken("github.com")

// Then it returns the token from the active user entry in the keyring
require.Equal(t, "keyring", source)
require.Equal(t, "test-token", token)

// When we set the active user to test-user2
authCfg.cfg.Set([]string{hostsKey, "github.com", userKey}, "test-user2")

// And get the token from the auth config
token, source = authCfg.ActiveToken("github.com")

// Then it returns the token from the active user entry in the keyring
require.Equal(t, "keyring", source)
require.Equal(t, "test-token2", token)
}

func TestTokenWithActiveUserNotInKeyringFallsBackToBlank(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-token1"))
require.NoError(t, keyring.Set(keyringServiceName("github.com"), "test-user2", "test-token2"))

// When we set the active user to test-user3
authCfg.cfg.Set([]string{hostsKey, "github.com", userKey}, "test-user3")

// And get the token from the auth config
token, source := authCfg.ActiveToken("github.com")

// Then it returns successfully with the fallback token
require.Equal(t, "keyring", source)
require.Equal(t, "test-token", token)
}

func TestLogoutRightAfterMigrationRemovesHost(t *testing.T) {
// Given we have logged in before migration
authCfg := newTestAuthConfig(t)
Expand Down
12 changes: 11 additions & 1 deletion internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,18 @@ func (c *AuthConfig) ActiveToken(hostname string) (string, string) {
}
token, source := ghauth.TokenFromEnvOrConfig(hostname)
if token == "" {
var user string
var err error
token, err = c.TokenFromKeyring(hostname)
if user, err = c.ActiveUser(hostname); err == nil {
token, err = c.TokenFromKeyringForUser(hostname, user)
}
if err != nil {
// We should generally be able to find a token for the active user,
// but in some cases such as if the keyring was set up in a very old
// version of the CLI, it may only have a unkeyed token, so fallback
// to it.
token, err = c.TokenFromKeyring(hostname)
}
if err == nil {
source = "keyring"
}
Expand Down
7 changes: 6 additions & 1 deletion pkg/cmd/api/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1358,7 +1358,12 @@ func Test_apiRun_cache(t *testing.T) {
Config: func() (gh.Config, error) {
return &ghmock.ConfigMock{
AuthenticationFunc: func() gh.AuthConfig {
return &config.AuthConfig{}
cfg := &config.AuthConfig{}
// Required because the http client tries to get the active token and otherwise
// this goes down to to go-gh config and panics. Pretty bad solution, it would
// be better if this were black box.
cfg.SetActiveToken("token", "stub")
return cfg
},
// Cached responses are stored in a tempdir that gets automatically cleaned up
CacheDirFunc: func() string {
Expand Down