diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 92a3cc89d..4fde4fd4f 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -1,6 +1,7 @@ package config import ( + "errors" "testing" ghConfig "github.com/cli/go-gh/v2/pkg/config" @@ -17,7 +18,7 @@ func newTestAuthConfig() *AuthConfig { func TestTokenFromKeyring(t *testing.T) { // Given a keyring that contains a token for a host keyring.MockInit() - keyring.Set(keyringServiceName("github.com"), "", "test-token") + require.NoError(t, keyring.Set(keyringServiceName("github.com"), "", "test-token")) // When we get the token from the auth config authCfg := newTestAuthConfig() @@ -89,3 +90,149 @@ func TestGitProtocolInAuthConfig(t *testing.T) { require.NoError(t, err) require.Equal(t, "ssh", gitProtocol) } + +func TestLoginSecureStorageUsesKeyring(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given a usable keyring + keyring.MockInit() + authCfg := newTestAuthConfig() + + // When we login with secure storage + insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", true) + + // Then it returns success, notes that insecure storage was not used, and stores the token in the keyring + require.NoError(t, err) + require.False(t, insecureStorageUsed, "expected to use secure storage") + + token, err := keyring.Get(keyringServiceName("github.com"), "") + require.NoError(t, err) + require.Equal(t, "test-token", token) +} + +func TestLoginSecureStorageRemovesOldInsecureConfigToken(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given a usable keyring and an oauth token in the config + keyring.MockInit() + authCfg := newTestAuthConfig() + authCfg.cfg.Set([]string{hosts, "github.com", oauthToken}, "old-token") + + // When we login with secure storage + _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) + + // Then it returns success, having also removed the old token from the config + require.NoError(t, err) + requireNoKey(t, authCfg.cfg, []string{hosts, "github.com", oauthToken}) +} + +func TestLoginSecureStorageWithErrorFallsbackAndReports(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given a keyring that errors + keyring.MockInitWithError(errors.New("test-explosion")) + authCfg := newTestAuthConfig() + + // When we login with secure storage + insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", true) + + // Then it returns success, reports that insecure storage was used, and stores the token in the config + require.NoError(t, err) + + require.True(t, insecureStorageUsed, "expected to use insecure storage") + requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", oauthToken}, "test-token") +} + +func TestLoginInsecureStorage(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + authCfg := newTestAuthConfig() + + // When we login with insecure storage + insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", false) + + // Then it returns success, notes that insecure storage was used, and stores the token in the config + require.NoError(t, err) + + require.True(t, insecureStorageUsed, "expected to use insecure storage") + requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", oauthToken}, "test-token") +} + +func TestLoginSetsUserAndGitProtocolInConfig(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given a usable keyring + keyring.MockInit() + authCfg := newTestAuthConfig() + + // When we login with secure storage + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + + // Then it returns success, and stores the user and git protocol in the config + require.NoError(t, err) + + requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", "user"}, "test-user") + requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", "git_protocol"}, "ssh") +} + +func TestLogoutRemovesHostAndKeyringToken(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we are logged into a host + keyring.MockInit() + authCfg := newTestAuthConfig() + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + require.NoError(t, err) + + // When we logout + err = authCfg.Logout("github.com") + + // Then we return success, and the host and token are removed from the config and keyring + require.NoError(t, err) + + requireNoKey(t, authCfg.cfg, []string{hosts, "github.com"}) + _, err = keyring.Get(keyringServiceName("github.com"), "") + require.ErrorIs(t, err, keyring.ErrNotFound) +} + +// Note that I'm not sure this test enforces particularly desirable behaviour +// since it leads users to believe a token has been removed when really +// that might have failed for some reason. +func TestLogoutIgnoresErrorsFromConfigAndKeyring(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we have keyring that errors, and a config that + // doesn't even have a hosts key (which would cause Remove to fail) + keyring.MockInitWithError(errors.New("test-explosion")) + authCfg := newTestAuthConfig() + + // When we logout + err := authCfg.Logout("github.com") + + // Then it returns success anyway, suppressing the errors + require.NoError(t, err) +} + +func requireKeyWithValue(t *testing.T, cfg *ghConfig.Config, keys []string, value string) { + t.Helper() + + actual, err := cfg.Get(keys) + require.NoError(t, err) + + require.Equal(t, value, actual) +} + +func requireNoKey(t *testing.T, cfg *ghConfig.Config, keys []string) { + t.Helper() + + _, err := cfg.Get(keys) + var keyNotFoundError *ghConfig.KeyNotFoundError + require.ErrorAs(t, err, &keyNotFoundError) +} diff --git a/internal/config/config.go b/internal/config/config.go index 1dafa7bbb..a413baa64 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -234,6 +234,8 @@ func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secure c.cfg.Set([]string{hosts, hostname, oauthToken}, token) insecureStorageUsed = true } + // TODO: I can't find anywhere that calls this function without a username, so that would seem like + // something we might want to error on? if username != "" { c.cfg.Set([]string{hosts, hostname, "user"}, username) } @@ -246,6 +248,8 @@ func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secure // Logout will remove user, git protocol, and auth token for the given hostname. // It will remove the auth token from the encrypted storage if it exists there. func (c *AuthConfig) Logout(hostname string) error { + // TODO: I can't find anywhere that expects to call this function without a hostname + // and that would seem like a programmer error? if hostname == "" { return nil }