From b94f45895021614f14b24ae90bca6397015ee998 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 15:51:46 +0200 Subject: [PATCH 01/21] Add initial tests for config Get --- internal/config/config_test.go | 95 ++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 internal/config/config_test.go diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 000000000..86ad79e99 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,95 @@ +package config_test + +import ( + "testing" + + "github.com/cli/cli/v2/internal/config" + "github.com/stretchr/testify/require" + + ghConfig "github.com/cli/go-gh/v2/pkg/config" +) + +func TestGetNonExistentKey(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we have no top level configuration + cfg, err := config.NewConfig() + require.NoError(t, err) + + // When we get a key that has no value + val, err := cfg.Get("", "non-existent-key") + + // Then it returns an error and the value is empty + var keyNotFoundError *ghConfig.KeyNotFoundError + require.ErrorAs(t, err, &keyNotFoundError) + require.Empty(t, val) +} + +func TestGetNonExistentHostSpecificKey(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given have no top level configuration + cfg, err := config.NewConfig() + require.NoError(t, err) + + // When we get a key for a host that has no value + val, err := cfg.Get("non-existent-host", "non-existent-key") + + // Then it returns an error and the value is empty + var keyNotFoundError *ghConfig.KeyNotFoundError + require.ErrorAs(t, err, &keyNotFoundError) + require.Empty(t, val) +} + +func TestGetExistingTopLevelKey(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given have a top level config entry + cfg, err := config.NewConfig() + require.NoError(t, err) + cfg.Set("", "top-level-key", "top-level-value") + + // When we get that key + val, err := cfg.Get("non-existent-host", "top-level-key") + + // Then it returns successfully with the correct value + require.NoError(t, err) + require.Equal(t, "top-level-value", val) +} + +func TestGetExistingHostSpecificKey(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given have a host specific config entry + cfg, err := config.NewConfig() + require.NoError(t, err) + cfg.Set("github.com", "host-specific-key", "host-specific-value") + + // When we get that key + val, err := cfg.Get("github.com", "host-specific-key") + + // Then it returns successfully with the correct value + require.NoError(t, err) + require.Equal(t, "host-specific-value", val) +} + +func TestGetHostnameSpecificKeyFallsBackToTopLevel(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given have a top level config entry + cfg, err := config.NewConfig() + require.NoError(t, err) + cfg.Set("", "key", "value") + + // When we get that key on a specific host + val, err := cfg.Get("github.com", "key") + + // Then it returns successfully, falling back to the top level config + require.NoError(t, err) + require.Equal(t, "value", val) +} From 5f645eadedd96110316e5cd4fc499aadf93148a9 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 16:10:53 +0200 Subject: [PATCH 02/21] Add tests for config GetOrDefault --- internal/config/config.go | 24 ++++----- internal/config/config_test.go | 94 ++++++++++++++++++++++++---------- 2 files changed, 79 insertions(+), 39 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 290c32577..335f70264 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -53,22 +53,13 @@ func (c *cfg) Get(hostname, key string) (string, error) { } func (c *cfg) GetOrDefault(hostname, key string) (string, error) { - var val string - var err error - if hostname != "" { - val, err = c.cfg.Get([]string{hosts, hostname, key}) - if err == nil { - return val, err - } - } - - val, err = c.cfg.Get([]string{key}) + val, err := c.Get(hostname, key) if err == nil { return val, err } - if defaultExists(key) { - return defaultFor(key), nil + if val, ok := def(key); ok { + return val, nil } return val, err @@ -94,6 +85,15 @@ func (c *cfg) Authentication() *AuthConfig { return &AuthConfig{cfg: c.cfg} } +func def(key string) (string, bool) { + for _, co := range configOptions { + if co.Key == key { + return co.DefaultValue, true + } + } + return "", false +} + func defaultFor(key string) string { for _, co := range configOptions { if co.Key == key { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 86ad79e99..df948a279 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,21 +1,22 @@ -package config_test +package config import ( "testing" - "github.com/cli/cli/v2/internal/config" "github.com/stretchr/testify/require" ghConfig "github.com/cli/go-gh/v2/pkg/config" ) -func TestGetNonExistentKey(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) +func newTestConfig() *cfg { + return &cfg{ + cfg: ghConfig.ReadFromString(""), + } +} +func TestGetNonExistentKey(t *testing.T) { // Given we have no top level configuration - cfg, err := config.NewConfig() - require.NoError(t, err) + cfg := newTestConfig() // When we get a key that has no value val, err := cfg.Get("", "non-existent-key") @@ -27,12 +28,8 @@ func TestGetNonExistentKey(t *testing.T) { } func TestGetNonExistentHostSpecificKey(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given have no top level configuration - cfg, err := config.NewConfig() - require.NoError(t, err) + cfg := newTestConfig() // When we get a key for a host that has no value val, err := cfg.Get("non-existent-host", "non-existent-key") @@ -44,12 +41,8 @@ func TestGetNonExistentHostSpecificKey(t *testing.T) { } func TestGetExistingTopLevelKey(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given have a top level config entry - cfg, err := config.NewConfig() - require.NoError(t, err) + cfg := newTestConfig() cfg.Set("", "top-level-key", "top-level-value") // When we get that key @@ -61,12 +54,8 @@ func TestGetExistingTopLevelKey(t *testing.T) { } func TestGetExistingHostSpecificKey(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given have a host specific config entry - cfg, err := config.NewConfig() - require.NoError(t, err) + cfg := newTestConfig() cfg.Set("github.com", "host-specific-key", "host-specific-value") // When we get that key @@ -78,12 +67,8 @@ func TestGetExistingHostSpecificKey(t *testing.T) { } func TestGetHostnameSpecificKeyFallsBackToTopLevel(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given have a top level config entry - cfg, err := config.NewConfig() - require.NoError(t, err) + cfg := newTestConfig() cfg.Set("", "key", "value") // When we get that key on a specific host @@ -93,3 +78,58 @@ func TestGetHostnameSpecificKeyFallsBackToTopLevel(t *testing.T) { require.NoError(t, err) require.Equal(t, "value", val) } + +func TestGetOrDefaultApplicationDefaults(t *testing.T) { + tests := []struct { + key string + expectedDefault string + }{ + {"git_protocol", "https"}, + {"editor", ""}, + {"prompt", "enabled"}, + {"pager", ""}, + {"http_unix_socket", ""}, + {"browser", ""}, + } + + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + // Given we have no top level configuration + cfg := newTestConfig() + + // When we get a key that has no value, but has a default + val, err := cfg.GetOrDefault("", tt.key) + + // Then it returns the default value + require.NoError(t, err) + require.Equal(t, tt.expectedDefault, val) + }) + } +} + +func TestGetOrDefaultExistingKey(t *testing.T) { + // Given have a top level config entry + cfg := newTestConfig() + cfg.Set("", "git_protocol", "ssh") + + // When we get that key + val, err := cfg.GetOrDefault("", "git_protocol") + + // Then it returns successfully with the correct value, and doesn't fall back + // to the default + require.NoError(t, err) + require.Equal(t, "ssh", val) +} + +func TestGetOrDefaultNotFoundAndNoDefault(t *testing.T) { + // Given have no configuration + cfg := newTestConfig() + + // When we get a non-existent-key that has no default + val, err := cfg.GetOrDefault("", "non-existent-key") + + // Then it returns an error and the value is empty + var keyNotFoundError *ghConfig.KeyNotFoundError + require.ErrorAs(t, err, &keyNotFoundError) + require.Empty(t, val) +} From 34b0b33f241de6e1ad3d8e92bf84ffe780e436b5 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 16:31:13 +0200 Subject: [PATCH 03/21] Add initial AuthConfig tests --- internal/config/auth_config_test.go | 91 +++++++++++++++++++++++++++++ internal/config/config.go | 1 + 2 files changed, 92 insertions(+) create mode 100644 internal/config/auth_config_test.go diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go new file mode 100644 index 000000000..92a3cc89d --- /dev/null +++ b/internal/config/auth_config_test.go @@ -0,0 +1,91 @@ +package config + +import ( + "testing" + + ghConfig "github.com/cli/go-gh/v2/pkg/config" + "github.com/stretchr/testify/require" + "github.com/zalando/go-keyring" +) + +func newTestAuthConfig() *AuthConfig { + return &AuthConfig{ + cfg: ghConfig.ReadFromString(""), + } +} + +func TestTokenFromKeyring(t *testing.T) { + // Given a keyring that contains a token for a host + keyring.MockInit() + keyring.Set(keyringServiceName("github.com"), "", "test-token") + + // When we get the token from the auth config + authCfg := newTestAuthConfig() + token, err := authCfg.TokenFromKeyring("github.com") + + // Then it returns successfully with the correct token + require.NoError(t, err) + require.Equal(t, "test-token", token) +} + +func TestTokenFromKeyringNonExistent(t *testing.T) { + // Given a keyring that doesn't contain any tokens + keyring.MockInit() + + // When we try to get a token from the auth config + authCfg := newTestAuthConfig() + _, err := authCfg.TokenFromKeyring("github.com") + + // Then it returns failure bubbling the ErrNotFound + require.ErrorIs(t, err, keyring.ErrNotFound) +} + +func TestNoUserInAuthConfig(t *testing.T) { + // Given a host configuration without a user + authCfg := newTestAuthConfig() + + // When we get the user + _, err := authCfg.User("github.com") + + // Then it returns failure, bubbling the KeyNotFoundError + var keyNotFoundError *ghConfig.KeyNotFoundError + require.ErrorAs(t, err, &keyNotFoundError) +} + +func TestUserInAuthConfig(t *testing.T) { + // Given an a host configuration with a user + authCfg := newTestAuthConfig() + authCfg.cfg.Set([]string{hosts, "github.com", "user"}, "test-user") + + // When we get the user + user, err := authCfg.User("github.com") + + // Then it returns success with the correct user + require.NoError(t, err) + require.Equal(t, "test-user", user) +} + +func TestNoGitProtocolInAuthConfig(t *testing.T) { + // Given a host configuration without a git protocol + authCfg := newTestAuthConfig() + + // When we get the git protocol + gitProtocol, err := authCfg.GitProtocol("github.com") + + // Then it returns success, using the default + require.NoError(t, err) + require.Equal(t, "https", gitProtocol) +} + +func TestGitProtocolInAuthConfig(t *testing.T) { + // Given an a host configuration with a git protocol + authCfg := newTestAuthConfig() + authCfg.cfg.Set([]string{hosts, "github.com", "git_protocol"}, "ssh") + + // When we get the git protocol + gitProtocol, err := authCfg.GitProtocol("github.com") + + // Then it returns success with the correct git protocol + require.NoError(t, err) + require.Equal(t, "ssh", gitProtocol) +} diff --git a/internal/config/config.go b/internal/config/config.go index 335f70264..1dafa7bbb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -178,6 +178,7 @@ func (c *AuthConfig) User(hostname string) (string, error) { // GitProtocol will retrieve the git protocol for the logged in user at the given hostname. // If none is set it will return the default value. +// TODO: although this returns an error, it actually has no path to error. func (c *AuthConfig) GitProtocol(hostname string) (string, error) { key := "git_protocol" val, err := c.cfg.Get([]string{hosts, hostname, key}) From 614e49296bc160d92e223daa2c68ddd4bb842cc2 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 16:57:54 +0200 Subject: [PATCH 04/21] Add tests for AuthConfig Login and Logout --- internal/config/auth_config_test.go | 149 +++++++++++++++++++++++++++- internal/config/config.go | 4 + 2 files changed, 152 insertions(+), 1 deletion(-) 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 } From 52ba99ebabd0f77257698326d0896591fae4809e Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 16:59:39 +0200 Subject: [PATCH 05/21] Update AuthConfig to use default fn --- internal/config/config.go | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index a413baa64..a9a421bd4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -58,7 +58,7 @@ func (c *cfg) GetOrDefault(hostname, key string) (string, error) { return val, err } - if val, ok := def(key); ok { + if val, ok := defaultFor(key); ok { return val, nil } @@ -85,7 +85,7 @@ func (c *cfg) Authentication() *AuthConfig { return &AuthConfig{cfg: c.cfg} } -func def(key string) (string, bool) { +func defaultFor(key string) (string, bool) { for _, co := range configOptions { if co.Key == key { return co.DefaultValue, true @@ -94,24 +94,6 @@ func def(key string) (string, bool) { return "", false } -func defaultFor(key string) string { - for _, co := range configOptions { - if co.Key == key { - return co.DefaultValue - } - } - return "" -} - -func defaultExists(key string) bool { - for _, co := range configOptions { - if co.Key == key { - return true - } - } - return false -} - // AuthConfig is used for interacting with some persistent configuration for gh, // with knowledge on how to access encrypted storage when neccesarry. // Behavior is scoped to authentication specific tasks. @@ -185,7 +167,12 @@ func (c *AuthConfig) GitProtocol(hostname string) (string, error) { if err == nil { return val, err } - return defaultFor(key), nil + + if val, ok := defaultFor(key); ok { + return val, nil + } + + return "", nil } func (c *AuthConfig) Hosts() []string { From 07110eca3322e838ca7ae5343f95a7614fcdb648 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 17:37:35 +0200 Subject: [PATCH 06/21] Add tests for AuthConfig Token --- internal/config/auth_config_test.go | 64 +++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 4fde4fd4f..7459f89c2 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -29,6 +29,70 @@ func TestTokenFromKeyring(t *testing.T) { require.Equal(t, "test-token", token) } +func TestTokenStoredInConfig(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // When the user has logged in insecurely + authCfg := newTestAuthConfig() + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + _, err := authCfg.Login("github.com", "test-user", "test-token", "", false) + require.NoError(t, err) + + // When we get the token + token, source := authCfg.Token("github.com") + + // Then the token is successfully fetched + // and the source is set to oauth_token but this isn't great + // but I can't find the issue # that references this. + require.Equal(t, "test-token", token) + require.Equal(t, "oauth_token", source) +} + +func TestTokenStoredInEnv(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // When the user is authenticated via env var + authCfg := newTestAuthConfig() + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + t.Setenv("GH_TOKEN", "test-token") + + // When we get the token + token, source := authCfg.Token("github.com") + + // Then the token is successfully fetched + // and the source is set to the name of the env var + require.Equal(t, "test-token", token) + require.Equal(t, "GH_TOKEN", source) +} + +func TestTokenStoredInKeyring(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // When the user has logged in securely + keyring.MockInit() + authCfg := newTestAuthConfig() + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) + require.NoError(t, err) + + // When we get the token + token, source := authCfg.Token("github.com") + + // Then the token is successfully fetched + // and the source is set to keyring + require.Equal(t, "test-token", token) + require.Equal(t, "keyring", source) +} + func TestTokenFromKeyringNonExistent(t *testing.T) { // Given a keyring that doesn't contain any tokens keyring.MockInit() From 465474ec5b551ac99cf13db6642a609b364ed111 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 17:38:46 +0200 Subject: [PATCH 07/21] Mod tidy go-md2man --- go.sum | 1 - 1 file changed, 1 deletion(-) diff --git a/go.sum b/go.sum index 45f962146..8746b26a3 100644 --- a/go.sum +++ b/go.sum @@ -32,7 +32,6 @@ github.com/cli/safeexec v1.0.1 h1:e/C79PbXF4yYTN/wauC4tviMxEV13BwljGj0N9j+N00= github.com/cli/safeexec v1.0.1/go.mod h1:Z/D4tTN8Vs5gXYHDCbaM1S/anmEDnJb1iW0+EJ5zx3Q= github.com/cli/shurcooL-graphql v0.0.3 h1:CtpPxyGDs136/+ZeyAfUKYmcQBjDlq5aqnrDCW5Ghh8= github.com/cli/shurcooL-graphql v0.0.3/go.mod h1:tlrLmw/n5Q/+4qSvosT+9/W5zc8ZMjnJeYBxSdb4nWA= -github.com/cpuguy83/go-md2man/v2 v2.0.2 h1:p1EgwI/C7NhT0JmVkwCD2ZBK8j4aeHQX2pMHHBfMQ6w= github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/cpuguy83/go-md2man/v2 v2.0.3 h1:qMCsGGgs+MAzDFyp9LpAe1Lqy/fY/qCovCm0qnXZOBM= github.com/cpuguy83/go-md2man/v2 v2.0.3/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= From e011633cdc5889c579b8835657dde2615e90291b Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Oct 2023 18:10:00 +0200 Subject: [PATCH 08/21] Add tests for AuthConfig HasEnvToken --- internal/config/auth_config_test.go | 48 +++++++++++++++++++++++++++++ internal/config/config.go | 8 +++++ 2 files changed, 56 insertions(+) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 7459f89c2..621bc9c8b 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -105,6 +105,54 @@ func TestTokenFromKeyringNonExistent(t *testing.T) { require.ErrorIs(t, err, keyring.ErrNotFound) } +func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { + // Given an empty hosts configuration + authCfg := newTestAuthConfig() + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + + // When we check if it has an env token + hasEnvToken := authCfg.HasEnvToken() + + // Then it returns false + require.False(t, hasEnvToken, "expected not to have env token") +} + +func TestHasEnvTokenWithEnvToken(t *testing.T) { + // Given an empty hosts configuration but a token set in the env var + authCfg := newTestAuthConfig() + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + t.Setenv("GH_ENTERPRISE_TOKEN", "test-token") + + // When we check if it has an env token + hasEnvToken := authCfg.HasEnvToken() + + // Then it returns true + require.True(t, hasEnvToken, "expected to have env token") +} + +func TestHasEnvTokenWithNoEnvTokenButAConfigVar(t *testing.T) { + t.Skip("this test is explicitly breaking some implementation assumptions") + + // Given a token in the config + authCfg := newTestAuthConfig() + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + // Using example.com here will cause the token to be returned from the config + _, err := authCfg.Login("example.com", "test-user", "test-token", "", false) + require.NoError(t, err) + + // When we check if it has an env token + hasEnvToken := authCfg.HasEnvToken() + + // Then it SHOULD return false + require.False(t, hasEnvToken, "expected not to have env token") +} + func TestNoUserInAuthConfig(t *testing.T) { // Given a host configuration without a user authCfg := newTestAuthConfig() diff --git a/internal/config/config.go b/internal/config/config.go index a9a421bd4..d93c37e3c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -135,6 +135,14 @@ func (c *AuthConfig) HasEnvToken() bool { return true } } + // TODO: This is _extremely_ knowledgable about the implementation of TokenFromEnvOrConfig + // It has to use a hostname that is not going to be found in the hosts so that it + // can guarantee that tokens will only be returned from a set env var. + // Discussed here, but maybe worth revisiting: https://github.com/cli/cli/pull/7169#discussion_r1136979033 + // + // By providing example.com. it's also _only_ looking for GH_ENTERPRISE_TOKEN or GITHUB_ENTERPRISE_TOKEN + // or the GITHUB_TOKEN if we happen to be in a codespace?? This doesn't seem to reflect the actual + // name of the function at all. token, _ := ghAuth.TokenFromEnvOrConfig(hostname) return token != "" } From 33808316721c2a2e0045e56047737029ea245702 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:05:14 +0200 Subject: [PATCH 09/21] Rewrite AuthConfig User and GitProtocol tests --- internal/config/auth_config_test.go | 49 ++++++++++++++++------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 621bc9c8b..461ebc723 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -153,8 +153,8 @@ func TestHasEnvTokenWithNoEnvTokenButAConfigVar(t *testing.T) { require.False(t, hasEnvToken, "expected not to have env token") } -func TestNoUserInAuthConfig(t *testing.T) { - // Given a host configuration without a user +func TestUserNotLoggedIn(t *testing.T) { + // Given we have not logged in authCfg := newTestAuthConfig() // When we get the user @@ -165,19 +165,6 @@ func TestNoUserInAuthConfig(t *testing.T) { require.ErrorAs(t, err, &keyNotFoundError) } -func TestUserInAuthConfig(t *testing.T) { - // Given an a host configuration with a user - authCfg := newTestAuthConfig() - authCfg.cfg.Set([]string{hosts, "github.com", "user"}, "test-user") - - // When we get the user - user, err := authCfg.User("github.com") - - // Then it returns success with the correct user - require.NoError(t, err) - require.Equal(t, "test-user", user) -} - func TestNoGitProtocolInAuthConfig(t *testing.T) { // Given a host configuration without a git protocol authCfg := newTestAuthConfig() @@ -274,22 +261,42 @@ func TestLoginInsecureStorage(t *testing.T) { requireKeyWithValue(t, authCfg.cfg, []string{hosts, "github.com", oauthToken}, "test-token") } -func TestLoginSetsUserAndGitProtocolInConfig(t *testing.T) { +func TestLoginSetsUserForProvidedHost(t *testing.T) { tempDir := t.TempDir() t.Setenv("GH_CONFIG_DIR", tempDir) - // Given a usable keyring + // Given a usable keyring and an empty config keyring.MockInit() authCfg := newTestAuthConfig() - // When we login with secure storage + // When we login _, 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 + // Then it returns success and the user is set 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") + user, err := authCfg.User("github.com") + require.NoError(t, err) + require.Equal(t, "test-user", user) +} + +func TestLoginSetsGitProtocolForProdivdedHost(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given a usable keyring and an empty config + keyring.MockInit() + authCfg := newTestAuthConfig() + + // When we login + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + + // Then it returns success and the git protocol is set + require.NoError(t, err) + + gitProtocol, err := authCfg.GitProtocol("github.com") + require.NoError(t, err) + require.Equal(t, "ssh", gitProtocol) } func TestLogoutRemovesHostAndKeyringToken(t *testing.T) { From cb7672e573735dee89df241844a0c4519f0e9837 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:05:40 +0200 Subject: [PATCH 10/21] Add additional comment on Logout ignoring errors --- internal/config/auth_config_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 461ebc723..a1f9b0833 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -323,6 +323,10 @@ func TestLogoutRemovesHostAndKeyringToken(t *testing.T) { // 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. +// +// The original intention here is that if the logout fails, the user can't +// really do anything to recover. On the other hand, a user might +// want to rectify this manually, for example if there were on a shared machine. func TestLogoutIgnoresErrorsFromConfigAndKeyring(t *testing.T) { tempDir := t.TempDir() t.Setenv("GH_CONFIG_DIR", tempDir) From e0bc2ff0caab8dbaf2fd2a5176e788fdfc0b8268 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:08:03 +0200 Subject: [PATCH 11/21] Add test that host is added on Login --- internal/config/auth_config_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index a1f9b0833..772452b3b 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -299,6 +299,27 @@ func TestLoginSetsGitProtocolForProdivdedHost(t *testing.T) { require.Equal(t, "ssh", gitProtocol) } +func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given a usable keyring and an empty config + keyring.MockInit() + authCfg := newTestAuthConfig() + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + + // When we login + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + + // Then it returns success and a host is added + require.NoError(t, err) + + hosts := authCfg.Hosts() + require.Contains(t, hosts, "github.com") +} + func TestLogoutRemovesHostAndKeyringToken(t *testing.T) { tempDir := t.TempDir() t.Setenv("GH_CONFIG_DIR", tempDir) From 07f5ca48d897563da5d6f4d19023b20b4222cb7a Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:24:05 +0200 Subject: [PATCH 12/21] Share config.Read setup in AuthConfig tests --- internal/config/auth_config_test.go | 35 ++++++++++------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 772452b3b..0ae8f29a2 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -10,9 +10,19 @@ import ( ) func newTestAuthConfig() *AuthConfig { - return &AuthConfig{ + authCfg := AuthConfig{ cfg: ghConfig.ReadFromString(""), } + // The real implementation of config.Read uses a sync.Once + // to read config files and initialise package level variables + // that are used from then on. + // + // This means that tests can't be isolated from each other, so + // we swap out the function here to return a new config each time. + ghConfig.Read = func() (*ghConfig.Config, error) { + return authCfg.cfg, nil + } + return &authCfg } func TestTokenFromKeyring(t *testing.T) { @@ -35,9 +45,6 @@ func TestTokenStoredInConfig(t *testing.T) { // When the user has logged in insecurely authCfg := newTestAuthConfig() - ghConfig.Read = func() (*ghConfig.Config, error) { - return authCfg.cfg, nil - } _, err := authCfg.Login("github.com", "test-user", "test-token", "", false) require.NoError(t, err) @@ -57,9 +64,6 @@ func TestTokenStoredInEnv(t *testing.T) { // When the user is authenticated via env var authCfg := newTestAuthConfig() - ghConfig.Read = func() (*ghConfig.Config, error) { - return authCfg.cfg, nil - } t.Setenv("GH_TOKEN", "test-token") // When we get the token @@ -78,9 +82,6 @@ func TestTokenStoredInKeyring(t *testing.T) { // When the user has logged in securely keyring.MockInit() authCfg := newTestAuthConfig() - ghConfig.Read = func() (*ghConfig.Config, error) { - return authCfg.cfg, nil - } _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) require.NoError(t, err) @@ -108,9 +109,6 @@ func TestTokenFromKeyringNonExistent(t *testing.T) { func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { // Given an empty hosts configuration authCfg := newTestAuthConfig() - ghConfig.Read = func() (*ghConfig.Config, error) { - return authCfg.cfg, nil - } // When we check if it has an env token hasEnvToken := authCfg.HasEnvToken() @@ -122,9 +120,6 @@ func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { func TestHasEnvTokenWithEnvToken(t *testing.T) { // Given an empty hosts configuration but a token set in the env var authCfg := newTestAuthConfig() - ghConfig.Read = func() (*ghConfig.Config, error) { - return authCfg.cfg, nil - } t.Setenv("GH_ENTERPRISE_TOKEN", "test-token") // When we check if it has an env token @@ -139,9 +134,6 @@ func TestHasEnvTokenWithNoEnvTokenButAConfigVar(t *testing.T) { // Given a token in the config authCfg := newTestAuthConfig() - ghConfig.Read = func() (*ghConfig.Config, error) { - return authCfg.cfg, nil - } // Using example.com here will cause the token to be returned from the config _, err := authCfg.Login("example.com", "test-user", "test-token", "", false) require.NoError(t, err) @@ -165,7 +157,7 @@ func TestUserNotLoggedIn(t *testing.T) { require.ErrorAs(t, err, &keyNotFoundError) } -func TestNoGitProtocolInAuthConfig(t *testing.T) { +func TestGitProtocolNotLoggedInDefaults(t *testing.T) { // Given a host configuration without a git protocol authCfg := newTestAuthConfig() @@ -306,9 +298,6 @@ func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { // Given a usable keyring and an empty config keyring.MockInit() authCfg := newTestAuthConfig() - ghConfig.Read = func() (*ghConfig.Config, error) { - return authCfg.cfg, nil - } // When we login _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) From fa80802c81eee9b7f3b213989edd25108394ec47 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:24:25 +0200 Subject: [PATCH 13/21] Add tests for AuthConfig DefaultHost --- internal/config/auth_config_test.go | 43 ++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 0ae8f29a2..335489525 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -169,17 +169,46 @@ func TestGitProtocolNotLoggedInDefaults(t *testing.T) { require.Equal(t, "https", gitProtocol) } -func TestGitProtocolInAuthConfig(t *testing.T) { - // Given an a host configuration with a git protocol +func TestDefaultHostFromEnvVar(t *testing.T) { + // Given the GH_HOST env var is set authCfg := newTestAuthConfig() - authCfg.cfg.Set([]string{hosts, "github.com", "git_protocol"}, "ssh") + t.Setenv("GH_HOST", "ghe.io") - // When we get the git protocol - gitProtocol, err := authCfg.GitProtocol("github.com") + // When we get the DefaultHost + defaultHost, source := authCfg.DefaultHost() - // Then it returns success with the correct git protocol + // Then the returned host and source are using the env var + require.Equal(t, "ghe.io", defaultHost) + require.Equal(t, "GH_HOST", source) +} + +func TestDefaultHostNotLoggedIn(t *testing.T) { + // Given we are not logged in + authCfg := newTestAuthConfig() + + // When we get the DefaultHost + defaultHost, source := authCfg.DefaultHost() + + // Then the returned host is always github.com + require.Equal(t, "github.com", defaultHost) + require.Equal(t, "default", source) +} + +func TestDefaultHostLoggedInToOnlyOneHost(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we are logged into one host (not github.com to differentiate from the fallback) + authCfg := newTestAuthConfig() + _, err := authCfg.Login("ghe.io", "test-user", "test-token", "", false) require.NoError(t, err) - require.Equal(t, "ssh", gitProtocol) + + // When we get the DefaultHost + defaultHost, source := authCfg.DefaultHost() + + // Then the returned host is that logged in host and the source is the hosts config + require.Equal(t, "ghe.io", defaultHost) + require.Equal(t, "hosts", source) } func TestLoginSecureStorageUsesKeyring(t *testing.T) { From c9de81786587231bf488f800d0ca6d4fefd7fb0f Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:26:52 +0200 Subject: [PATCH 14/21] Avoid using keyring unnecessarily in AuthConfig tests This is to avoid the impression that the keyring matters, when the test is actually about general auth behaviour --- internal/config/auth_config_test.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 335489525..09649bce8 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -286,12 +286,11 @@ func TestLoginSetsUserForProvidedHost(t *testing.T) { tempDir := t.TempDir() t.Setenv("GH_CONFIG_DIR", tempDir) - // Given a usable keyring and an empty config - keyring.MockInit() + // Given we are not logged in authCfg := newTestAuthConfig() // When we login - _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) // Then it returns success and the user is set require.NoError(t, err) @@ -301,16 +300,15 @@ func TestLoginSetsUserForProvidedHost(t *testing.T) { require.Equal(t, "test-user", user) } -func TestLoginSetsGitProtocolForProdivdedHost(t *testing.T) { +func TestLoginSetsGitProtocolForProvidedHost(t *testing.T) { tempDir := t.TempDir() t.Setenv("GH_CONFIG_DIR", tempDir) - // Given a usable keyring and an empty config - keyring.MockInit() + // Given we are not logged in authCfg := newTestAuthConfig() // When we login - _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) // Then it returns success and the git protocol is set require.NoError(t, err) @@ -324,12 +322,11 @@ func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { tempDir := t.TempDir() t.Setenv("GH_CONFIG_DIR", tempDir) - // Given a usable keyring and an empty config - keyring.MockInit() + // Given we are not logged in authCfg := newTestAuthConfig() // When we login - _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) // Then it returns success and a host is added require.NoError(t, err) From c430da732960ca673b3dd8c4f13f70190d5b41a2 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:31:41 +0200 Subject: [PATCH 15/21] Ensure all AuthConfig tests are isolated on disk --- internal/config/auth_config_test.go | 95 ++++++++++------------------- 1 file changed, 33 insertions(+), 62 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 09649bce8..221c5cb2f 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -9,10 +9,11 @@ import ( "github.com/zalando/go-keyring" ) -func newTestAuthConfig() *AuthConfig { +func newTestAuthConfig(t *testing.T) *AuthConfig { authCfg := AuthConfig{ cfg: ghConfig.ReadFromString(""), } + // The real implementation of config.Read uses a sync.Once // to read config files and initialise package level variables // that are used from then on. @@ -22,6 +23,14 @@ func newTestAuthConfig() *AuthConfig { ghConfig.Read = func() (*ghConfig.Config, error) { return authCfg.cfg, nil } + + // The config.Write method isn't defined in the same way as Read to allow + // the function to be swapped out and it does try to write to disk. + // + // We should consider whether it makes sense to change that but in the meantime + // we can use GH_CONFIG_DIR env var to ensure the tests remain isolated. + t.Setenv("GH_CONFIG_DIR", t.TempDir()) + return &authCfg } @@ -31,7 +40,7 @@ func TestTokenFromKeyring(t *testing.T) { require.NoError(t, keyring.Set(keyringServiceName("github.com"), "", "test-token")) // When we get the token from the auth config - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) token, err := authCfg.TokenFromKeyring("github.com") // Then it returns successfully with the correct token @@ -40,11 +49,8 @@ func TestTokenFromKeyring(t *testing.T) { } func TestTokenStoredInConfig(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // When the user has logged in insecurely - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) _, err := authCfg.Login("github.com", "test-user", "test-token", "", false) require.NoError(t, err) @@ -59,11 +65,8 @@ func TestTokenStoredInConfig(t *testing.T) { } func TestTokenStoredInEnv(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // When the user is authenticated via env var - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) t.Setenv("GH_TOKEN", "test-token") // When we get the token @@ -76,12 +79,9 @@ func TestTokenStoredInEnv(t *testing.T) { } func TestTokenStoredInKeyring(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // When the user has logged in securely keyring.MockInit() - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) require.NoError(t, err) @@ -99,7 +99,7 @@ func TestTokenFromKeyringNonExistent(t *testing.T) { keyring.MockInit() // When we try to get a token from the auth config - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) _, err := authCfg.TokenFromKeyring("github.com") // Then it returns failure bubbling the ErrNotFound @@ -108,7 +108,7 @@ func TestTokenFromKeyringNonExistent(t *testing.T) { func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { // Given an empty hosts configuration - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we check if it has an env token hasEnvToken := authCfg.HasEnvToken() @@ -119,7 +119,7 @@ func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { func TestHasEnvTokenWithEnvToken(t *testing.T) { // Given an empty hosts configuration but a token set in the env var - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) t.Setenv("GH_ENTERPRISE_TOKEN", "test-token") // When we check if it has an env token @@ -133,7 +133,7 @@ func TestHasEnvTokenWithNoEnvTokenButAConfigVar(t *testing.T) { t.Skip("this test is explicitly breaking some implementation assumptions") // Given a token in the config - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // Using example.com here will cause the token to be returned from the config _, err := authCfg.Login("example.com", "test-user", "test-token", "", false) require.NoError(t, err) @@ -147,7 +147,7 @@ func TestHasEnvTokenWithNoEnvTokenButAConfigVar(t *testing.T) { func TestUserNotLoggedIn(t *testing.T) { // Given we have not logged in - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we get the user _, err := authCfg.User("github.com") @@ -159,7 +159,7 @@ func TestUserNotLoggedIn(t *testing.T) { func TestGitProtocolNotLoggedInDefaults(t *testing.T) { // Given a host configuration without a git protocol - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we get the git protocol gitProtocol, err := authCfg.GitProtocol("github.com") @@ -171,7 +171,7 @@ func TestGitProtocolNotLoggedInDefaults(t *testing.T) { func TestDefaultHostFromEnvVar(t *testing.T) { // Given the GH_HOST env var is set - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) t.Setenv("GH_HOST", "ghe.io") // When we get the DefaultHost @@ -184,7 +184,7 @@ func TestDefaultHostFromEnvVar(t *testing.T) { func TestDefaultHostNotLoggedIn(t *testing.T) { // Given we are not logged in - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we get the DefaultHost defaultHost, source := authCfg.DefaultHost() @@ -195,11 +195,8 @@ func TestDefaultHostNotLoggedIn(t *testing.T) { } func TestDefaultHostLoggedInToOnlyOneHost(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given we are logged into one host (not github.com to differentiate from the fallback) - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) _, err := authCfg.Login("ghe.io", "test-user", "test-token", "", false) require.NoError(t, err) @@ -212,12 +209,9 @@ func TestDefaultHostLoggedInToOnlyOneHost(t *testing.T) { } func TestLoginSecureStorageUsesKeyring(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given a usable keyring keyring.MockInit() - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we login with secure storage insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", true) @@ -232,12 +226,9 @@ func TestLoginSecureStorageUsesKeyring(t *testing.T) { } 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 := newTestAuthConfig(t) authCfg.cfg.Set([]string{hosts, "github.com", oauthToken}, "old-token") // When we login with secure storage @@ -249,12 +240,9 @@ func TestLoginSecureStorageRemovesOldInsecureConfigToken(t *testing.T) { } 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() + authCfg := newTestAuthConfig(t) // When we login with secure storage insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", true) @@ -267,10 +255,8 @@ func TestLoginSecureStorageWithErrorFallsbackAndReports(t *testing.T) { } func TestLoginInsecureStorage(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - - authCfg := newTestAuthConfig() + // Given we are not logged in + authCfg := newTestAuthConfig(t) // When we login with insecure storage insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", false) @@ -283,11 +269,8 @@ func TestLoginInsecureStorage(t *testing.T) { } func TestLoginSetsUserForProvidedHost(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given we are not logged in - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we login _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) @@ -301,11 +284,8 @@ func TestLoginSetsUserForProvidedHost(t *testing.T) { } func TestLoginSetsGitProtocolForProvidedHost(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given we are not logged in - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we login _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) @@ -319,11 +299,8 @@ func TestLoginSetsGitProtocolForProvidedHost(t *testing.T) { } func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { - tempDir := t.TempDir() - t.Setenv("GH_CONFIG_DIR", tempDir) - // Given we are not logged in - authCfg := newTestAuthConfig() + authCfg := newTestAuthConfig(t) // When we login _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) @@ -336,12 +313,9 @@ func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { } 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() + authCfg := newTestAuthConfig(t) _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", true) require.NoError(t, err) @@ -364,13 +338,10 @@ func TestLogoutRemovesHostAndKeyringToken(t *testing.T) { // really do anything to recover. On the other hand, a user might // want to rectify this manually, for example if there were on a shared machine. 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() + authCfg := newTestAuthConfig(t) // When we logout err := authCfg.Logout("github.com") From aa8848471f18ab6d40c98cc09136be1c0bbd71bd Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:33:15 +0200 Subject: [PATCH 16/21] Remove Logout branch that handles empty hostname --- internal/config/config.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index d93c37e3c..65240aa3f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -243,11 +243,6 @@ 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 - } _ = c.cfg.Remove([]string{hosts, hostname}) _ = keyring.Delete(keyringServiceName(hostname), "") return ghConfig.Write(c.cfg) From 3c3fbcdb66b3ae4d98d9a020a833fd2dfce4249b Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:35:43 +0200 Subject: [PATCH 17/21] Remove Login branch that handles empty username --- internal/config/config.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 65240aa3f..faa6497e6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -229,11 +229,9 @@ 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) - } + + c.cfg.Set([]string{hosts, hostname, "user"}, username) + if gitProtocol != "" { c.cfg.Set([]string{hosts, hostname, "git_protocol"}, gitProtocol) } From b9cbee046203663bb013d53f8df5dde2ed5364b5 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:39:31 +0200 Subject: [PATCH 18/21] Remove incorrect comment about HasEnvToken --- internal/config/config.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index faa6497e6..96a7f0716 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -139,10 +139,6 @@ func (c *AuthConfig) HasEnvToken() bool { // It has to use a hostname that is not going to be found in the hosts so that it // can guarantee that tokens will only be returned from a set env var. // Discussed here, but maybe worth revisiting: https://github.com/cli/cli/pull/7169#discussion_r1136979033 - // - // By providing example.com. it's also _only_ looking for GH_ENTERPRISE_TOKEN or GITHUB_ENTERPRISE_TOKEN - // or the GITHUB_TOKEN if we happen to be in a codespace?? This doesn't seem to reflect the actual - // name of the function at all. token, _ := ghAuth.TokenFromEnvOrConfig(hostname) return token != "" } From 7d8c1af009705e867af334ea11ce3c8144f704dd Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:48:30 +0200 Subject: [PATCH 19/21] Fix GitProtocol signature and rework test descriptions --- internal/config/auth_config_test.go | 45 +++++++++++++++++------------ internal/config/config.go | 14 ++++----- pkg/cmd/auth/refresh/refresh.go | 2 +- pkg/cmd/auth/status/status.go | 2 +- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 221c5cb2f..2b0d0ff98 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -107,7 +107,7 @@ func TestTokenFromKeyringNonExistent(t *testing.T) { } func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { - // Given an empty hosts configuration + // Given we have no env set authCfg := newTestAuthConfig(t) // When we check if it has an env token @@ -118,7 +118,8 @@ func TestHasEnvTokenWithoutAnyEnvToken(t *testing.T) { } func TestHasEnvTokenWithEnvToken(t *testing.T) { - // Given an empty hosts configuration but a token set in the env var + // Given we have an env token set + // Note that any valid env var for tokens will do, not just GH_ENTERPRISE_TOKEN authCfg := newTestAuthConfig(t) t.Setenv("GH_ENTERPRISE_TOKEN", "test-token") @@ -158,17 +159,28 @@ func TestUserNotLoggedIn(t *testing.T) { } func TestGitProtocolNotLoggedInDefaults(t *testing.T) { - // Given a host configuration without a git protocol + // Given we have not logged in authCfg := newTestAuthConfig(t) // When we get the git protocol - gitProtocol, err := authCfg.GitProtocol("github.com") + gitProtocol := authCfg.GitProtocol("github.com") - // Then it returns success, using the default - require.NoError(t, err) + // Then it returns the default require.Equal(t, "https", gitProtocol) } +func TestHostsIncludesEnvVar(t *testing.T) { + // Given the GH_HOST env var is set + authCfg := newTestAuthConfig(t) + t.Setenv("GH_HOST", "ghe.io") + + // When we get the hosts + hosts := authCfg.Hosts() + + // Then the host in the env var is included + require.Contains(t, hosts, "ghe.io") +} + func TestDefaultHostFromEnvVar(t *testing.T) { // Given the GH_HOST env var is set authCfg := newTestAuthConfig(t) @@ -284,31 +296,28 @@ func TestLoginSetsUserForProvidedHost(t *testing.T) { } func TestLoginSetsGitProtocolForProvidedHost(t *testing.T) { - // Given we are not logged in + // Given we are loggedin authCfg := newTestAuthConfig(t) - - // When we login _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) - - // Then it returns success and the git protocol is set require.NoError(t, err) - gitProtocol, err := authCfg.GitProtocol("github.com") - require.NoError(t, err) + // When we get the git protocol + gitProtocol := authCfg.GitProtocol("github.com") + + // Then it returns the git protocol we provided on login require.Equal(t, "ssh", gitProtocol) } func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { - // Given we are not logged in + // Given we are logged in authCfg := newTestAuthConfig(t) - - // When we login _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) - - // Then it returns success and a host is added require.NoError(t, err) + // When we get the hosts hosts := authCfg.Hosts() + + // Then it includes our logged in host require.Contains(t, hosts, "github.com") } diff --git a/internal/config/config.go b/internal/config/config.go index 96a7f0716..587cc996b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -164,19 +164,19 @@ func (c *AuthConfig) User(hostname string) (string, error) { // GitProtocol will retrieve the git protocol for the logged in user at the given hostname. // If none is set it will return the default value. -// TODO: although this returns an error, it actually has no path to error. -func (c *AuthConfig) GitProtocol(hostname string) (string, error) { +func (c *AuthConfig) GitProtocol(hostname string) string { key := "git_protocol" - val, err := c.cfg.Get([]string{hosts, hostname, key}) - if err == nil { - return val, err + if val, err := c.cfg.Get([]string{hosts, hostname, key}); err == nil { + return val } if val, ok := defaultFor(key); ok { - return val, nil + return val } - return "", nil + // This should not happen, as we know there is a default value for this key. + // Perhaps it says something about our current default abstraction being not quite right? + return "" } func (c *AuthConfig) Hosts() []string { diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 489746179..30b5a3ff0 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -178,7 +178,7 @@ func refreshRun(opts *RefreshOptions) error { Prompter: opts.Prompter, GitClient: opts.GitClient, } - gitProtocol, _ := authCfg.GitProtocol(hostname) + gitProtocol := authCfg.GitProtocol(hostname) if opts.Interactive && gitProtocol == "https" { if err := credentialFlow.Prompt(hostname); err != nil { return err diff --git a/pkg/cmd/auth/status/status.go b/pkg/cmd/auth/status/status.go index 1739cfe09..bde74306a 100644 --- a/pkg/cmd/auth/status/status.go +++ b/pkg/cmd/auth/status/status.go @@ -139,7 +139,7 @@ func statusRun(opts *StatusOptions) error { } addMsg("%s Logged in to %s as %s (%s)", cs.SuccessIcon(), hostname, cs.Bold(username), tokenSource) - proto, _ := authCfg.GitProtocol(hostname) + proto := authCfg.GitProtocol(hostname) if proto != "" { addMsg("%s Git operations for %s configured to use %s protocol.", cs.SuccessIcon(), hostname, cs.Bold(proto)) From 6dc04bb1e2b19bfde12392fa09328a4799b153ae Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 12:57:19 +0200 Subject: [PATCH 20/21] Remove GetOrDefault uses in favour of GitProtocol --- internal/config/config.go | 1 + pkg/cmd/gist/clone/clone.go | 5 +---- pkg/cmd/pr/checkout/checkout.go | 2 +- pkg/cmd/pr/create/create.go | 2 +- pkg/cmd/repo/clone/clone.go | 10 ++-------- pkg/cmd/repo/create/create.go | 18 +++--------------- pkg/cmd/repo/rename/rename.go | 5 +---- 7 files changed, 10 insertions(+), 33 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 587cc996b..6f60f14b5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -176,6 +176,7 @@ func (c *AuthConfig) GitProtocol(hostname string) string { // This should not happen, as we know there is a default value for this key. // Perhaps it says something about our current default abstraction being not quite right? + // The defaults are also currently used to provide information about the config options. return "" } diff --git a/pkg/cmd/gist/clone/clone.go b/pkg/cmd/gist/clone/clone.go index 9b75a309c..cd477a745 100644 --- a/pkg/cmd/gist/clone/clone.go +++ b/pkg/cmd/gist/clone/clone.go @@ -80,10 +80,7 @@ func cloneRun(opts *CloneOptions) error { return err } hostname, _ := cfg.Authentication().DefaultHost() - protocol, err := cfg.GetOrDefault(hostname, "git_protocol") - if err != nil { - return err - } + protocol := cfg.Authentication().GitProtocol(hostname) gistURL = formatRemoteURL(hostname, gistURL, protocol) } diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index 193eea735..b91fb1765 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -84,7 +84,7 @@ func checkoutRun(opts *CheckoutOptions) error { if err != nil { return err } - protocol, _ := cfg.GetOrDefault(baseRepo.RepoHost(), "git_protocol") + protocol := cfg.Authentication().GitProtocol(baseRepo.RepoHost()) remotes, err := opts.Remotes() if err != nil { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 8c1047de7..81b2c05f3 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -761,7 +761,7 @@ func handlePush(opts CreateOptions, ctx CreateContext) error { return err } - cloneProtocol, _ := cfg.GetOrDefault(headRepo.RepoHost(), "git_protocol") + cloneProtocol := cfg.Authentication().GitProtocol(headRepo.RepoHost()) headRepoURL := ghrepo.FormatRemoteURL(headRepo, cloneProtocol) gitClient := ctx.GitClient origin, _ := remotes.FindByName("origin") diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go index 174951243..298ca30de 100644 --- a/pkg/cmd/repo/clone/clone.go +++ b/pkg/cmd/repo/clone/clone.go @@ -129,10 +129,7 @@ func cloneRun(opts *CloneOptions) error { return err } - protocol, err = cfg.GetOrDefault(repo.RepoHost(), "git_protocol") - if err != nil { - return err - } + protocol = cfg.Authentication().GitProtocol(repo.RepoHost()) } wantsWiki := strings.HasSuffix(repo.RepoName(), ".wiki") @@ -166,10 +163,7 @@ func cloneRun(opts *CloneOptions) error { // If the repo is a fork, add the parent as an upstream remote and set the parent as the default repo. if canonicalRepo.Parent != nil { - protocol, err := cfg.GetOrDefault(canonicalRepo.Parent.RepoHost(), "git_protocol") - if err != nil { - return err - } + protocol := cfg.Authentication().GitProtocol(canonicalRepo.Parent.RepoHost()) upstreamURL := ghrepo.FormatRemoteURL(canonicalRepo.Parent, protocol) upstreamName := opts.UpstreamName diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index c050607ed..c96aeb3f9 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -395,11 +395,7 @@ func createFromScratch(opts *CreateOptions) error { } if opts.Clone { - protocol, err := cfg.GetOrDefault(repo.RepoHost(), "git_protocol") - if err != nil { - return err - } - + protocol := cfg.Authentication().GitProtocol(repo.RepoHost()) remoteURL := ghrepo.FormatRemoteURL(repo, protocol) if !opts.AddReadme && opts.LicenseTemplate == "" && opts.GitIgnoreTemplate == "" && opts.Template == "" { @@ -496,11 +492,7 @@ func createFromTemplate(opts *CreateOptions) error { } if opts.Clone { - protocol, err := cfg.GetOrDefault(repo.RepoHost(), "git_protocol") - if err != nil { - return err - } - + protocol := cfg.Authentication().GitProtocol(repo.RepoHost()) remoteURL := ghrepo.FormatRemoteURL(repo, protocol) if err := cloneWithRetry(opts, remoteURL, templateRepoMainBranch); err != nil { @@ -622,11 +614,7 @@ func createFromLocal(opts *CreateOptions) error { fmt.Fprintln(stdout, repo.URL) } - protocol, err := cfg.GetOrDefault(repo.RepoHost(), "git_protocol") - if err != nil { - return err - } - + protocol := cfg.Authentication().GitProtocol(repo.RepoHost()) remoteURL := ghrepo.FormatRemoteURL(repo, protocol) if opts.Interactive { diff --git a/pkg/cmd/repo/rename/rename.go b/pkg/cmd/repo/rename/rename.go index 973eb7bd6..8dbc1326d 100644 --- a/pkg/cmd/repo/rename/rename.go +++ b/pkg/cmd/repo/rename/rename.go @@ -149,10 +149,7 @@ func updateRemote(repo ghrepo.Interface, renamed ghrepo.Interface, opts *RenameO return nil, err } - protocol, err := cfg.GetOrDefault(repo.RepoHost(), "git_protocol") - if err != nil { - return nil, err - } + protocol := cfg.Authentication().GitProtocol(repo.RepoHost()) remotes, err := opts.Remotes() if err != nil { From 4fc1999847c5a7c419cc31418c1ed7c129f0de66 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 19 Oct 2023 15:39:10 +0200 Subject: [PATCH 21/21] Add link to token source issue --- internal/config/auth_config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 2b0d0ff98..e4a1dc9c4 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -58,8 +58,8 @@ func TestTokenStoredInConfig(t *testing.T) { token, source := authCfg.Token("github.com") // Then the token is successfully fetched - // and the source is set to oauth_token but this isn't great - // but I can't find the issue # that references this. + // and the source is set to oauth_token but this isn't great: + // https://github.com/cli/go-gh/issues/94 require.Equal(t, "test-token", token) require.Equal(t, "oauth_token", source) }