From f294a5f53348f22f48ae11d6dec6cd4f502779a6 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Thu, 29 May 2025 12:13:21 +0900 Subject: [PATCH 1/6] fix: get token for active user instead of blank if possible --- internal/config/auth_config_test.go | 35 +++++++++++++++++++++++++++++ internal/config/config.go | 8 +++++++ pkg/cmd/api/api_test.go | 14 ++++++++++-- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 61245c650..20ece76b8 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -42,6 +42,41 @@ func TestTokenFromKeyringForUser(t *testing.T) { require.Equal(t, "test-token", token) } +func TestTokenFromKeyringActiveUserNotBlankUser(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-user2", "test-token2")) + + // When we get the token from the auth config + token, err := authCfg.TokenFromKeyring("github.com") + + // Then it returns successfully with the correct token + require.NoError(t, err) + 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, err = authCfg.TokenFromKeyring("github.com") + + // Then it returns successfully with the correct token + require.NoError(t, err) + 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, err = authCfg.TokenFromKeyring("github.com") + + // Then it returns successfully with the correct token + require.NoError(t, err) + require.Equal(t, "test-token2", token) +} + func TestTokenFromKeyringForUserErrorsIfUsernameIsBlank(t *testing.T) { authCfg := newTestAuthConfig(t) diff --git a/internal/config/config.go b/internal/config/config.go index 003a0ca17..3e652079a 100644 --- a/internal/config/config.go +++ b/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 != "" { + // Prioritize the user-specific token if it exists, which may be + // different from the blank active token, for example if a user uses + // GH_CONFIG_DIR to point to a different config directory. + if tok, err := c.TokenFromKeyringForUser(hostname, user); err == nil && tok != "" { + return tok, nil + } + } return keyring.Get(keyringServiceName(hostname), "") } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 321f7b7c0..dd5dd7c59 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -1343,6 +1343,16 @@ func Test_apiRun_inputFile(t *testing.T) { } } +type stubAuthConfig struct { + config.AuthConfig +} + +var _ gh.AuthConfig = (*stubAuthConfig)(nil) + +func (c *stubAuthConfig) ActiveToken(host string) (string, string) { + return "token", "stub" +} + func Test_apiRun_cache(t *testing.T) { // Given we have a test server that spies on the number of requests it receives requestCount := 0 @@ -1355,10 +1365,10 @@ func Test_apiRun_cache(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() options := ApiOptions{ IO: ios, - Config: func() (gh.Config, error) { + Config: func() (cfg gh.Config, err error) { return &ghmock.ConfigMock{ AuthenticationFunc: func() gh.AuthConfig { - return &config.AuthConfig{} + return &stubAuthConfig{} }, // Cached responses are stored in a tempdir that gets automatically cleaned up CacheDirFunc: func() string { From cc9a2411e0d81b37a616e76be0451163bfd8260d Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Thu, 29 May 2025 12:28:26 +0900 Subject: [PATCH 2/6] Cleanup --- internal/config/auth_config_test.go | 20 +++++++++++++++++++- pkg/cmd/api/api_test.go | 2 +- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 20ece76b8..4e220cbbf 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -42,7 +42,7 @@ func TestTokenFromKeyringForUser(t *testing.T) { require.Equal(t, "test-token", token) } -func TestTokenFromKeyringActiveUserNotBlankUser(t *testing.T) { +func TestTokenFromKeyringPrioritizesActiveUserToken(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")) @@ -77,6 +77,24 @@ func TestTokenFromKeyringActiveUserNotBlankUser(t *testing.T) { require.Equal(t, "test-token2", token) } +func TestTokenFromKeyringActiveUserNotInKeyringFallsBackToBlank(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, err := authCfg.TokenFromKeyring("github.com") + + // Then it returns successfully with the fallback token + require.NoError(t, err) + require.Equal(t, "test-token", token) +} + func TestTokenFromKeyringForUserErrorsIfUsernameIsBlank(t *testing.T) { authCfg := newTestAuthConfig(t) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index dd5dd7c59..a49911587 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -1365,7 +1365,7 @@ func Test_apiRun_cache(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() options := ApiOptions{ IO: ios, - Config: func() (cfg gh.Config, err error) { + Config: func() (gh.Config, error) { return &ghmock.ConfigMock{ AuthenticationFunc: func() gh.AuthConfig { return &stubAuthConfig{} From 8deae3038a1e8b398683dc9f5a89dded3b6de949 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 17 Jun 2025 15:53:13 +0200 Subject: [PATCH 3/6] Use active token stubbing on auth config --- pkg/cmd/api/api_test.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index a49911587..bf4d51c4c 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -1343,16 +1343,6 @@ func Test_apiRun_inputFile(t *testing.T) { } } -type stubAuthConfig struct { - config.AuthConfig -} - -var _ gh.AuthConfig = (*stubAuthConfig)(nil) - -func (c *stubAuthConfig) ActiveToken(host string) (string, string) { - return "token", "stub" -} - func Test_apiRun_cache(t *testing.T) { // Given we have a test server that spies on the number of requests it receives requestCount := 0 @@ -1368,7 +1358,12 @@ func Test_apiRun_cache(t *testing.T) { Config: func() (gh.Config, error) { return &ghmock.ConfigMock{ AuthenticationFunc: func() gh.AuthConfig { - return &stubAuthConfig{} + 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 { From 848cedd2c801a4f83ae10008f053af2cbcdf0d81 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Wed, 18 Jun 2025 09:56:44 +0900 Subject: [PATCH 4/6] Push up --- internal/config/auth_config_test.go | 106 ++++++++++++++-------------- internal/config/config.go | 20 +++--- 2 files changed, 64 insertions(+), 62 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 4e220cbbf..f685b9527 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -42,59 +42,6 @@ func TestTokenFromKeyringForUser(t *testing.T) { require.Equal(t, "test-token", token) } -func TestTokenFromKeyringPrioritizesActiveUserToken(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-user2", "test-token2")) - - // When we get the token from the auth config - token, err := authCfg.TokenFromKeyring("github.com") - - // Then it returns successfully with the correct token - require.NoError(t, err) - 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, err = authCfg.TokenFromKeyring("github.com") - - // Then it returns successfully with the correct token - require.NoError(t, err) - 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, err = authCfg.TokenFromKeyring("github.com") - - // Then it returns successfully with the correct token - require.NoError(t, err) - require.Equal(t, "test-token2", token) -} - -func TestTokenFromKeyringActiveUserNotInKeyringFallsBackToBlank(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, err := authCfg.TokenFromKeyring("github.com") - - // Then it returns successfully with the fallback token - require.NoError(t, err) - require.Equal(t, "test-token", token) -} - func TestTokenFromKeyringForUserErrorsIfUsernameIsBlank(t *testing.T) { authCfg := newTestAuthConfig(t) @@ -822,6 +769,59 @@ func TestTokenWorksRightAfterMigration(t *testing.T) { require.Equal(t, oauthTokenKey, source) } +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-user2", "test-token2")) + + // When we get the token from the auth config + token, source := authCfg.ActiveToken("github.com") + + // Then it returns successfully with the correct token + 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 successfully with the correct token + 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 successfully with the correct token + require.Equal(t, source, "keyring") + 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) diff --git a/internal/config/config.go b/internal/config/config.go index 3e652079a..47d9403b0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -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" } @@ -281,14 +291,6 @@ 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 != "" { - // Prioritize the user-specific token if it exists, which may be - // different from the blank active token, for example if a user uses - // GH_CONFIG_DIR to point to a different config directory. - if tok, err := c.TokenFromKeyringForUser(hostname, user); err == nil && tok != "" { - return tok, nil - } - } return keyring.Get(keyringServiceName(hostname), "") } From b7c2b19e703a9b819824bf35973cc77679ea0805 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 20 Jun 2025 14:36:45 -0400 Subject: [PATCH 5/6] Enhance Activetoken prioritize test - ensure test user tokens are different from unkeyed token - ensure assertion expected / actual are in correct order --- internal/config/auth_config_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index f685b9527..5350b4f5e 100644 --- a/internal/config/auth_config_test.go +++ b/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")) require.NoError(t, keyring.Set(keyringServiceName("github.com"), "test-user2", "test-token2")) // When we get the token from the auth config @@ -791,7 +791,7 @@ func TestTokenPrioritizesActiveUserToken(t *testing.T) { // Then it returns successfully with the correct token require.Equal(t, "keyring", source) - require.Equal(t, "test-token", token) + require.Equal(t, "test-token1", token) // When we set the active user to test-user2 authCfg.cfg.Set([]string{hostsKey, "github.com", userKey}, "test-user2") @@ -800,7 +800,7 @@ func TestTokenPrioritizesActiveUserToken(t *testing.T) { token, source = authCfg.ActiveToken("github.com") // Then it returns successfully with the correct token - require.Equal(t, source, "keyring") + require.Equal(t, "keyring", source) require.Equal(t, "test-token2", token) } From 0180c7fce4e30b7adc883ac0d235f3ee08f23409 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 20 Jun 2025 15:38:09 -0400 Subject: [PATCH 6/6] Restored original test setup, clarified 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. --- internal/config/auth_config_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 5350b4f5e..ca5f7e584 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -770,16 +770,19 @@ func TestTokenWorksRightAfterMigration(t *testing.T) { } func TestTokenPrioritizesActiveUserToken(t *testing.T) { - // Given a keyring that contains a token for a host + // 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-token1")) + 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 we get the token from the auth config + // 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 successfully with the correct token + // Then it returns the token from the keyring active slot require.Equal(t, "keyring", source) require.Equal(t, "test-token", token) @@ -789,9 +792,9 @@ func TestTokenPrioritizesActiveUserToken(t *testing.T) { // And get the token from the auth config token, source = authCfg.ActiveToken("github.com") - // Then it returns successfully with the correct token + // Then it returns the token from the active user entry in the keyring require.Equal(t, "keyring", source) - require.Equal(t, "test-token1", token) + 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") @@ -799,7 +802,7 @@ func TestTokenPrioritizesActiveUserToken(t *testing.T) { // And get the token from the auth config token, source = authCfg.ActiveToken("github.com") - // Then it returns successfully with the correct token + // Then it returns the token from the active user entry in the keyring require.Equal(t, "keyring", source) require.Equal(t, "test-token2", token) }