Push up

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)

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), "")
 }