From 7106129f650cd512d165d7a0179db08bff553a57 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 4 Dec 2023 13:49:28 +0100 Subject: [PATCH] Restore previous happy state on Switch failure --- internal/config/auth_config_test.go | 56 +++++++++++++++++- internal/config/config.go | 92 +++++++++++++++++++---------- 2 files changed, 115 insertions(+), 33 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 97373394c..4a95e00dd 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -454,12 +454,28 @@ func TestSwitchUserUpdatesTheActiveUser(t *testing.T) { require.Equal(t, "test-user-1", activeUser) } -func TestSwitchUserErrorsIfNoTokenMadeActive(t *testing.T) { +func TestSwitchUserErrorsImmediatelyIfTheActiveTokenComesFromEnvironment(t *testing.T) { + // Given we have a token in the env + authCfg := newTestAuthConfig(t) + t.Setenv("GH_TOKEN", "unimportant-test-value") + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", true) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", true) + require.NoError(t, err) + + // When we switch to a user + err = authCfg.SwitchUser("github.com", "test-user-1") + + // Then it errors immediately with an informative message + require.ErrorContains(t, err, "currently active token for github.com is from GH_TOKEN") +} + +func TestSwitchUserErrorsAndRestoresUserAndInsecureConfigUnderFailure(t *testing.T) { // Given we have a user but no token can be found (because we deleted them, simulating an error case) authCfg := newTestAuthConfig(t) _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", true) require.NoError(t, err) - _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", true) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", false) require.NoError(t, err) require.NoError(t, keyring.Delete(keyringServiceName("github.com"), "test-user-1")) @@ -468,8 +484,42 @@ func TestSwitchUserErrorsIfNoTokenMadeActive(t *testing.T) { err = authCfg.SwitchUser("github.com", "test-user-1") // Then it returns an error - // But also restores the previous require.EqualError(t, err, "no token found for 'test-user-1'") + + // And restores the previous state + activeUser, err := authCfg.User("github.com") + require.NoError(t, err) + require.Equal(t, "test-user-2", activeUser) + + token, source := authCfg.Token("github.com") + require.Equal(t, "test-token-2", token) + require.Equal(t, "oauth_token", source) +} + +func TestSwitchUserErrorsAndRestoresUserAndKeyringUnderFailure(t *testing.T) { + // Given we have a user but no token can be found (because we deleted them, simulating an error case) + authCfg := newTestAuthConfig(t) + _, err := authCfg.Login("github.com", "test-user-1", "test-token-1", "ssh", false) + require.NoError(t, err) + _, err = authCfg.Login("github.com", "test-user-2", "test-token-2", "ssh", true) + require.NoError(t, err) + + require.NoError(t, authCfg.cfg.Remove([]string{hostsKey, "github.com", usersKey, "test-user-1", oauthTokenKey})) + + // When we switch to the user + err = authCfg.SwitchUser("github.com", "test-user-1") + + // Then it returns an error + require.EqualError(t, err, "no token found for 'test-user-1'") + + // And restores the previous state + activeUser, err := authCfg.User("github.com") + require.NoError(t, err) + require.Equal(t, "test-user-2", activeUser) + + token, source := authCfg.Token("github.com") + require.Equal(t, "test-token-2", token) + require.Equal(t, "keyring", source) } func TestSwitchClearsActiveSecureTokenWhenSwitchingToInsecureUser(t *testing.T) { diff --git a/internal/config/config.go b/internal/config/config.go index 81e80e706..708a142bd 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "os" "path/filepath" @@ -332,42 +333,40 @@ func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secure c.cfg.Set([]string{hostsKey, hostname, usersKey, username}, "") } - // Then we perform a switch to the new user - return insecureStorageUsed, c.SwitchUser(hostname, username) + // Then we activate the new user + return insecureStorageUsed, c.activateUser(hostname, username) } -// TODO: How should git protocol be handled? Do we need to set it at the user level since it could have been changed? func (c *AuthConfig) SwitchUser(hostname, user string) error { - // We first need to idempotently clear out any set tokens for the host - _ = keyring.Delete(keyringServiceName(hostname), "") - _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + previouslyActiveUser, err := c.User(hostname) + if err != nil { + return fmt.Errorf("failed to get active user: %s", err) + } - // Then we'll move the keyring token or insecure token as necessary, only one of the - // following branches should be true. + previouslyActiveToken, previousSource := c.Token(hostname) + if previousSource != "keyring" && previousSource != "oauth_token" { + return fmt.Errorf("currently active token for %s is from %s", hostname, previousSource) + } - // If there is a token in the secure keyring for the user, move it to the active slot - var tokenSwitched bool - if token, err := keyring.Get(keyringServiceName(hostname), user); err == nil { - if err = keyring.Set(keyringServiceName(hostname), "", token); err != nil { - return fmt.Errorf("failed to move active token in keyring: %v", err) + err = c.activateUser(hostname, user) + if err != nil { + // Given that activateUser can only fail before the config is written, or when writing the config + // we know for sure that the config has not been written. However, we still should restore it back + // to its previous clean state just in case something else tries to make use of the config, or tries + // to write it again. + if previousSource == "keyring" { + err = errors.Join(err, keyring.Set(keyringServiceName(hostname), "", previouslyActiveToken)) } - tokenSwitched = true + + if previousSource == "oauth_token" { + c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, previouslyActiveToken) + } + c.cfg.Set([]string{hostsKey, hostname, userKey}, previouslyActiveUser) + + return err } - // If there is a token in the insecure config for the user, move it to the active field - if token, err := c.cfg.Get([]string{hostsKey, hostname, usersKey, user, oauthTokenKey}); err == nil { - c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) - tokenSwitched = true - } - - if !tokenSwitched { - return fmt.Errorf("no token found for '%s'", user) - } - - // Then we'll update the active user for the host - c.cfg.Set([]string{hostsKey, hostname, userKey}, user) - - return ghConfig.Write(c.cfg) + return nil } // Logout will remove user, git protocol, and auth token for the given hostname. @@ -401,8 +400,41 @@ func (c *AuthConfig) Logout(hostname, username string) error { return n != username }) - // And switch to them - return c.SwitchUser(hostname, users[switchUserIdx]) + // And activate them + return c.activateUser(hostname, users[switchUserIdx]) +} + +func (c *AuthConfig) activateUser(hostname, user string) error { + // We first need to idempotently clear out any set tokens for the host + _ = keyring.Delete(keyringServiceName(hostname), "") + _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + + // Then we'll move the keyring token or insecure token as necessary, only one of the + // following branches should be true. + + // If there is a token in the secure keyring for the user, move it to the active slot + var tokenSwitched bool + if token, err := keyring.Get(keyringServiceName(hostname), user); err == nil { + if err = keyring.Set(keyringServiceName(hostname), "", token); err != nil { + return fmt.Errorf("failed to move active token in keyring: %v", err) + } + tokenSwitched = true + } + + // If there is a token in the insecure config for the user, move it to the active field + if token, err := c.cfg.Get([]string{hostsKey, hostname, usersKey, user, oauthTokenKey}); err == nil { + c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) + tokenSwitched = true + } + + if !tokenSwitched { + return fmt.Errorf("no token found for '%s'", user) + } + + // Then we'll update the active user for the host + c.cfg.Set([]string{hostsKey, hostname, userKey}, user) + + return ghConfig.Write(c.cfg) } func (c *AuthConfig) UsersForHost(hostname string) ([]string, error) {