diff --git a/cmd/gh/main.go b/cmd/gh/main.go index bcce3b427..871afbb5f 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -56,6 +56,14 @@ func mainRun() exitCode { ctx := context.Background() + // cfg, err := cmdFactory.Config() + // if err != nil { + // fmt.Fprintf(stderr, "failed to load configuration to attempt migration: %s\n", err) + // } + // if err := cfg.MigrateMultiAccount(); err != nil { + // fmt.Fprintf(stderr, "failed to migrate configuration: %s\n", err) + // } + updateCtx, updateCancel := context.WithCancel(ctx) defer updateCancel() updateMessageChan := make(chan *update.ReleaseInfo) diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index afa47fbfd..c028ef6b3 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -4,6 +4,7 @@ import ( "errors" "testing" + "github.com/cli/cli/v2/internal/config/migration" ghConfig "github.com/cli/go-gh/v2/pkg/config" "github.com/stretchr/testify/require" "github.com/zalando/go-keyring" @@ -61,7 +62,7 @@ func TestTokenStoredInConfig(t *testing.T) { // 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) + require.Equal(t, oauthTokenKey, source) } func TestTokenStoredInEnv(t *testing.T) { @@ -285,13 +286,13 @@ func TestLoginSetsUserForProvidedHost(t *testing.T) { } func TestLoginSetsGitProtocolForProvidedHost(t *testing.T) { - // Given we are loggedin + // Given we are logged in authCfg := newTestAuthConfig(t) _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) require.NoError(t, err) // When we get the git protocol - protocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey}) + protocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", usersKey, "test-user", gitProtocolKey}) require.NoError(t, err) // Then it returns the git protocol we provided on login @@ -311,6 +312,23 @@ func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) { require.Contains(t, hosts, "github.com") } +// This test mimics the behaviour of logging in with a token and not providing +// a git protocol. +func TestLoginAddsUserToConfigWithoutGitProtocolOrSecureStorage(t *testing.T) { + // Given we are not logged in + authCfg := newTestAuthConfig(t) + + // When we log in without git protocol or secure storage + keyring.MockInit() + _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) + require.NoError(t, err) + + // Then the username is added under the users config + users, err := authCfg.cfg.Keys([]string{hostsKey, "github.com", usersKey}) + require.NoError(t, err) + require.Contains(t, users, "test-user") +} + func TestLogoutRemovesHostAndKeyringToken(t *testing.T) { // Given we are logged into a host keyring.MockInit() @@ -365,3 +383,199 @@ func requireNoKey(t *testing.T, cfg *ghConfig.Config, keys []string) { var keyNotFoundError *ghConfig.KeyNotFoundError require.ErrorAs(t, err, &keyNotFoundError) } + +// Post migration tests + +func TestUserWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + // Then we can still get the user correctly + user, err := authCfg.User("github.com") + require.NoError(t, err) + require.Equal(t, "test-user", user) +} + +func TestGitProtocolWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration with a non-default git protocol + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + // Then we can still get the git protocol correctly + gitProtocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey}) + require.NoError(t, err) + require.Equal(t, "ssh", gitProtocol) +} + +func TestHostsWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "ghe.io", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + // Then we can still get the hosts correctly + hosts := authCfg.Hosts() + require.Contains(t, hosts, "ghe.io") +} + +func TestDefaultHostWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration to an enterprise host + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "ghe.io", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + // Then the default host is still the enterprise host + defaultHost, source := authCfg.DefaultHost() + require.Equal(t, "ghe.io", defaultHost) + require.Equal(t, hostsKey, source) +} + +func TestTokenWorksRightAfterMigration(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + // Then we can still get the token correctly + token, source := authCfg.Token("github.com") + require.Equal(t, "test-token", token) + require.Equal(t, oauthTokenKey, source) +} + +func TestLogoutRigthAfterMigrationRemovesHost(t *testing.T) { + // Given we have logged in before migration + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we migrate and logout + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + require.NoError(t, authCfg.Logout("github.com")) + + // Then the host is removed from the config + requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com"}) +} + +func TestLoginInsecurePostMigrationUsesConfigForToken(t *testing.T) { + // Given we have not logged in + authCfg := newTestAuthConfig(t) + + // When we migrate and login with insecure storage + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + 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 + // both under the host and under the user + require.NoError(t, err) + + require.True(t, insecureStorageUsed, "expected to use insecure storage") + requireKeyWithValue(t, authCfg.cfg, []string{hostsKey, "github.com", oauthTokenKey}, "test-token") + requireKeyWithValue(t, authCfg.cfg, []string{hostsKey, "github.com", usersKey, "test-user", oauthTokenKey}, "test-token") +} + +func TestLoginPostMigrationSetsGitProtocol(t *testing.T) { + // Given we have logged in after migration + authCfg := newTestAuthConfig(t) + + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we get the git protocol + gitProtocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", usersKey, "test-user", gitProtocolKey}) + require.NoError(t, err) + + // Then it returns the git protocol we provided on login + require.Equal(t, "ssh", gitProtocol) +} + +func TestLoginPostMigrationSetsUser(t *testing.T) { + // Given we have logged in after migration + authCfg := newTestAuthConfig(t) + + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) + require.NoError(t, err) + + // When we get the user + user, err := authCfg.User("github.com") + + // Then it returns success and the user we provided on login + require.NoError(t, err) + require.Equal(t, "test-user", user) +} + +func TestLoginSecurePostMigrationRemovesTokenFromConfig(t *testing.T) { + // Given we have logged in insecurely + authCfg := newTestAuthConfig(t) + _, err := preMigrationLogin(authCfg, "github.com", "test-user", "test-token", "", false) + require.NoError(t, err) + + // When we migrate and login again with secure storage + var m migration.MultiAccount + require.NoError(t, Migrate(authCfg.cfg, m)) + + keyring.MockInit() + _, err = authCfg.Login("github.com", "test-user", "test-token", "", true) + + // Then it returns success, having removed the old insecure oauth token entry + require.NoError(t, err) + requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com", oauthTokenKey}) + requireNoKey(t, authCfg.cfg, []string{hostsKey, "github.com", usersKey, "test-user", oauthTokenKey}) +} + +// Copied and pasted directly from the trunk branch before doing any work on +// login, plus the addition of AuthConfig as the first arg since it is a method +// receiver in the real implementation. +func preMigrationLogin(c *AuthConfig, hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) { + var setErr error + if secureStorage { + if setErr = keyring.Set(keyringServiceName(hostname), "", token); setErr == nil { + // Clean up the previous oauth_token from the config file. + _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + } + } + insecureStorageUsed := false + if !secureStorage || setErr != nil { + c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) + insecureStorageUsed = true + } + + c.cfg.Set([]string{hostsKey, hostname, userKey}, username) + + if gitProtocol != "" { + c.cfg.Set([]string{hostsKey, hostname, gitProtocolKey}, gitProtocol) + } + return insecureStorageUsed, ghConfig.Write(c.cfg) +} diff --git a/internal/config/config.go b/internal/config/config.go index 4e48c8a83..23bff82ce 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" + "github.com/cli/cli/v2/internal/config/migration" "github.com/cli/cli/v2/internal/keyring" ghAuth "github.com/cli/go-gh/v2/pkg/auth" ghConfig "github.com/cli/go-gh/v2/pkg/config" @@ -19,6 +20,9 @@ const ( oauthTokenKey = "oauth_token" pagerKey = "pager" promptKey = "prompt" + userKey = "user" + usersKey = "users" + versionKey = "version" ) // This interface describes interacting with some persistent configuration for gh. @@ -37,6 +41,8 @@ type Config interface { HTTPUnixSocket(string) string Pager(string) string Prompt(string) string + + MigrateMultiAccount() error } func NewConfig() (Config, error) { @@ -126,6 +132,11 @@ func (c *cfg) Prompt(hostname string) string { return val } +func (c *cfg) MigrateMultiAccount() error { + var m migration.MultiAccount + return Migrate(c.cfg, m) +} + func defaultFor(key string) (string, bool) { for _, co := range ConfigOptions() { if co.Key == key { @@ -200,7 +211,7 @@ func (c *AuthConfig) TokenFromKeyring(hostname string) (string, error) { // User will retrieve the username for the logged in user at the given hostname. func (c *AuthConfig) User(hostname string) (string, error) { - return c.cfg.Get([]string{hostsKey, hostname, "user"}) + return c.cfg.Get([]string{hostsKey, hostname, userKey}) } func (c *AuthConfig) Hosts() []string { @@ -240,21 +251,33 @@ func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secure var setErr error if secureStorage { if setErr = keyring.Set(keyringServiceName(hostname), "", token); setErr == nil { - // Clean up the previous oauth_token from the config file. + // Clean up the previous oauth_tokens from the config file. _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + _ = c.cfg.Remove([]string{hostsKey, hostname, usersKey, username, oauthTokenKey}) } } insecureStorageUsed := false if !secureStorage || setErr != nil { + // Set the current active oauth token c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) + // And set the oauth token under the user to support later auth switch + // and logout switch without another migration. + c.cfg.Set([]string{hostsKey, hostname, usersKey, username, oauthTokenKey}, token) insecureStorageUsed = true } - c.cfg.Set([]string{hostsKey, hostname, "user"}, username) + c.cfg.Set([]string{hostsKey, hostname, userKey}, username) if gitProtocol != "" { - c.cfg.Set([]string{hostsKey, hostname, gitProtocolKey}, gitProtocol) + c.cfg.Set([]string{hostsKey, hostname, usersKey, username, gitProtocolKey}, gitProtocol) } + + // Create the username key with an empty value so it will be + // written even when there are no keys set under it. + if _, getErr := c.cfg.Get([]string{hostsKey, hostname, usersKey, username}); getErr != nil { + c.cfg.Set([]string{hostsKey, hostname, usersKey, username}, "") + } + return insecureStorageUsed, ghConfig.Write(c.cfg) } @@ -303,7 +326,12 @@ func fallbackConfig() *ghConfig.Config { return ghConfig.ReadFromString(defaultConfigStr) } +// The schema version in here should match the PostVersion of whatever the +// last migration we decided to run is. Therefore, if we run a new migration, +// this should be bumped. const defaultConfigStr = ` +# The current version of the config schema +version: 1 # What protocol to use when performing git operations. Supported values: ssh, https git_protocol: https # What editor gh should run when creating issues, pull requests, etc. If blank, will refer to environment. diff --git a/internal/config/config_mock.go b/internal/config/config_mock.go index aff4cb9d4..53e17998e 100644 --- a/internal/config/config_mock.go +++ b/internal/config/config_mock.go @@ -38,6 +38,9 @@ var _ Config = &ConfigMock{} // HTTPUnixSocketFunc: func(s string) string { // panic("mock out the HTTPUnixSocket method") // }, +// MigrateMultiAccountFunc: func() error { +// panic("mock out the MigrateMultiAccount method") +// }, // PagerFunc: func(s string) string { // panic("mock out the Pager method") // }, @@ -78,6 +81,9 @@ type ConfigMock struct { // HTTPUnixSocketFunc mocks the HTTPUnixSocket method. HTTPUnixSocketFunc func(s string) string + // MigrateMultiAccountFunc mocks the MigrateMultiAccount method. + MigrateMultiAccountFunc func() error + // PagerFunc mocks the Pager method. PagerFunc func(s string) string @@ -125,6 +131,9 @@ type ConfigMock struct { // S is the s argument value. S string } + // MigrateMultiAccount holds details about calls to the MigrateMultiAccount method. + MigrateMultiAccount []struct { + } // Pager holds details about calls to the Pager method. Pager []struct { // S is the s argument value. @@ -148,17 +157,18 @@ type ConfigMock struct { Write []struct { } } - lockAliases sync.RWMutex - lockAuthentication sync.RWMutex - lockBrowser sync.RWMutex - lockEditor sync.RWMutex - lockGetOrDefault sync.RWMutex - lockGitProtocol sync.RWMutex - lockHTTPUnixSocket sync.RWMutex - lockPager sync.RWMutex - lockPrompt sync.RWMutex - lockSet sync.RWMutex - lockWrite sync.RWMutex + lockAliases sync.RWMutex + lockAuthentication sync.RWMutex + lockBrowser sync.RWMutex + lockEditor sync.RWMutex + lockGetOrDefault sync.RWMutex + lockGitProtocol sync.RWMutex + lockHTTPUnixSocket sync.RWMutex + lockMigrateMultiAccount sync.RWMutex + lockPager sync.RWMutex + lockPrompt sync.RWMutex + lockSet sync.RWMutex + lockWrite sync.RWMutex } // Aliases calls AliasesFunc. @@ -379,6 +389,33 @@ func (mock *ConfigMock) HTTPUnixSocketCalls() []struct { return calls } +// MigrateMultiAccount calls MigrateMultiAccountFunc. +func (mock *ConfigMock) MigrateMultiAccount() error { + if mock.MigrateMultiAccountFunc == nil { + panic("ConfigMock.MigrateMultiAccountFunc: method is nil but Config.MigrateMultiAccount was just called") + } + callInfo := struct { + }{} + mock.lockMigrateMultiAccount.Lock() + mock.calls.MigrateMultiAccount = append(mock.calls.MigrateMultiAccount, callInfo) + mock.lockMigrateMultiAccount.Unlock() + return mock.MigrateMultiAccountFunc() +} + +// MigrateMultiAccountCalls gets all the calls that were made to MigrateMultiAccount. +// Check the length with: +// +// len(mockedConfig.MigrateMultiAccountCalls()) +func (mock *ConfigMock) MigrateMultiAccountCalls() []struct { +} { + var calls []struct { + } + mock.lockMigrateMultiAccount.RLock() + calls = mock.calls.MigrateMultiAccount + mock.lockMigrateMultiAccount.RUnlock() + return calls +} + // Pager calls PagerFunc. func (mock *ConfigMock) Pager(s string) string { if mock.PagerFunc == nil { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6acc32245..3fd082874 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -22,6 +22,7 @@ func TestNewConfigProvidesFallback(t *testing.T) { } _, err := NewConfig() require.NoError(t, err) + requireKeyWithValue(t, spiedCfg, []string{versionKey}, "1") requireKeyWithValue(t, spiedCfg, []string{gitProtocolKey}, "https") requireKeyWithValue(t, spiedCfg, []string{editorKey}, "") requireKeyWithValue(t, spiedCfg, []string{promptKey}, "enabled") diff --git a/internal/config/migrate.go b/internal/config/migrate.go new file mode 100644 index 000000000..1556f0a88 --- /dev/null +++ b/internal/config/migrate.go @@ -0,0 +1,49 @@ +package config + +import ( + "fmt" + + ghConfig "github.com/cli/go-gh/v2/pkg/config" +) + +//go:generate moq -rm -out migration_mock.go . Migration + +// Migration is the interace that config migrations must implement. +// +// Migrations will receive a copy of the config, and should modify that copy +// as necessary. After migration has completed, the modified config contents +// will be used. +// +// The calling code is expected to verify that the current version of the config +// matches the PreVersion of the migration before calling Do, and will set the +// config version to the PostVersion after the migration has completed successfully. +type Migration interface { + // PreVersion is the required config version for this to be applied + PreVersion() string + // PostVersion is the config version that must be applied after migration + PostVersion() string + // Do is expected to apply any necessary changes to the config in place + Do(*ghConfig.Config) error +} + +func Migrate(c *ghConfig.Config, m Migration) error { + // It is expected initially that there is no version key because we don't + // have one to begin with, so an error is expected. + version, _ := c.Get([]string{versionKey}) + if m.PreVersion() != version { + return fmt.Errorf("failed to migrate as %q pre migration version did not match config version %q", m.PreVersion(), version) + } + + if err := m.Do(c); err != nil { + return fmt.Errorf("failed to migrate config: %s", err) + } + + c.Set([]string{versionKey}, m.PostVersion()) + + // Then write out our migrated config. + if err := ghConfig.Write(c); err != nil { + return fmt.Errorf("failed to write config after migration: %s", err) + } + + return nil +} diff --git a/internal/config/migrate_test.go b/internal/config/migrate_test.go new file mode 100644 index 000000000..62f9d6062 --- /dev/null +++ b/internal/config/migrate_test.go @@ -0,0 +1,228 @@ +package config + +import ( + "bytes" + "errors" + "io" + "os" + "path/filepath" + "testing" + + ghConfig "github.com/cli/go-gh/v2/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestMigrationAppliedSuccessfully(t *testing.T) { + readConfig := StubWriteConfig(t) + + // Given we have a migrator that writes some keys to the top level config + // and hosts key + cfg := ghConfig.ReadFromString(testFullConfig()) + topLevelKey := []string{"toplevelkey"} + newHostKey := []string{hostsKey, "newhost"} + + migration := mockMigration(func(config *ghConfig.Config) error { + config.Set(topLevelKey, "toplevelvalue") + config.Set(newHostKey, "newhostvalue") + return nil + }) + + // When we run the migration + require.NoError(t, Migrate(cfg, migration)) + + // Then our original config is updated with the migration applied + requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, cfg, newHostKey, "newhostvalue") + + // And our config / hosts changes are persisted to their relevant files + // Note that this is real janky. We have writers that represent the + // top level config and the hosts key but we don't merge them back together + // so when we look into the hosts data, we don't nest the key we're + // looking for under the hosts key ¯\_(ツ)_/¯ + var configBuf bytes.Buffer + var hostsBuf bytes.Buffer + readConfig(&configBuf, &hostsBuf) + persistedCfg := ghConfig.ReadFromString(configBuf.String()) + persistedHosts := ghConfig.ReadFromString(hostsBuf.String()) + + requireKeyWithValue(t, persistedCfg, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, persistedHosts, []string{"newhost"}, "newhostvalue") +} + +func TestMigrationAppliedBumpsVersion(t *testing.T) { + readConfig := StubWriteConfig(t) + + // Given we have a migration with a pre version that matches + // the version in the config + cfg := ghConfig.ReadFromString(testFullConfig()) + cfg.Set([]string{versionKey}, "expected-pre-version") + topLevelKey := []string{"toplevelkey"} + + migration := &MigrationMock{ + DoFunc: func(config *ghConfig.Config) error { + config.Set(topLevelKey, "toplevelvalue") + return nil + }, + PreVersionFunc: func() string { + return "expected-pre-version" + }, + PostVersionFunc: func() string { + return "expected-post-version" + }, + } + + // When we migrate + require.NoError(t, Migrate(cfg, migration)) + + // Then our original config is updated with the migration applied + requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, cfg, []string{versionKey}, "expected-post-version") + + // And our config / hosts changes are persisted to their relevant files + var configBuf bytes.Buffer + readConfig(&configBuf, io.Discard) + persistedCfg := ghConfig.ReadFromString(configBuf.String()) + + requireKeyWithValue(t, persistedCfg, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, persistedCfg, []string{versionKey}, "expected-post-version") +} + +func TestMigrationErrorsWhenPreVersionMismatch(t *testing.T) { + StubWriteConfig(t) + + // Given we have a migration with a pre version that does not match + // the version in the config + cfg := ghConfig.ReadFromString(testFullConfig()) + cfg.Set([]string{versionKey}, "not-expected-pre-version") + topLevelKey := []string{"toplevelkey"} + + migration := &MigrationMock{ + DoFunc: func(config *ghConfig.Config) error { + config.Set(topLevelKey, "toplevelvalue") + return nil + }, + PreVersionFunc: func() string { + return "expected-pre-version" + }, + PostVersionFunc: func() string { + return "not-expected" + }, + } + + // When we run Migrate + err := Migrate(cfg, migration) + + // Then there is an error the migration is not applied and the version is not modified + require.ErrorContains(t, err, `failed to migrate as "expected-pre-version" pre migration version did not match config version "not-expected-pre-version"`) + requireNoKey(t, cfg, topLevelKey) + requireKeyWithValue(t, cfg, []string{versionKey}, "not-expected-pre-version") +} + +func TestMigrationErrorWritesNoFiles(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we have a migrator that errors + cfg := ghConfig.ReadFromString(testFullConfig()) + migration := mockMigration(func(config *ghConfig.Config) error { + return errors.New("failed to migrate in test") + }) + + // When we run the migration + err := Migrate(cfg, migration) + + // Then the error is wrapped and bubbled + require.EqualError(t, err, "failed to migrate config: failed to migrate in test") + + // And no files are written to disk + files, err := os.ReadDir(tempDir) + require.NoError(t, err) + require.Len(t, files, 0) +} + +func TestMigrationWriteErrors(t *testing.T) { + tests := []struct { + name string + unwriteableFile string + wantErrContains string + }{ + { + name: "failure to write hosts", + unwriteableFile: "hosts.yml", + wantErrContains: "failed to write config after migration", + }, + { + name: "failure to write config", + unwriteableFile: "config.yml", + wantErrContains: "failed to write config after migration", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("GH_CONFIG_DIR", tempDir) + + // Given we error when writing the files (because we chmod the files as trickery) + makeFileUnwriteable(t, filepath.Join(tempDir, tt.unwriteableFile)) + + cfg := ghConfig.ReadFromString(testFullConfig()) + topLevelKey := []string{"toplevelkey"} + hostsKey := []string{hostsKey, "newhost"} + + migration := mockMigration(func(c *ghConfig.Config) error { + c.Set(topLevelKey, "toplevelvalue") + c.Set(hostsKey, "newhostvalue") + return nil + }) + + // When we run the migration + err := Migrate(cfg, migration) + + // Then the error is wrapped and bubbled + require.ErrorContains(t, err, tt.wantErrContains) + }) + } +} + +func makeFileUnwriteable(t *testing.T, file string) { + t.Helper() + + f, err := os.Create(file) + require.NoError(t, err) + f.Close() + + require.NoError(t, os.Chmod(file, 0000)) +} + +func mockMigration(doFunc func(config *ghConfig.Config) error) *MigrationMock { + return &MigrationMock{ + DoFunc: doFunc, + PreVersionFunc: func() string { + return "" + }, + PostVersionFunc: func() string { + return "not-expected" + }, + } + +} + +func testFullConfig() string { + var data = ` +git_protocol: ssh +editor: +prompt: enabled +pager: less +hosts: + github.com: + user: user1 + oauth_token: xxxxxxxxxxxxxxxxxxxx + git_protocol: ssh + enterprise.com: + user: user2 + oauth_token: yyyyyyyyyyyyyyyyyyyy + git_protocol: https +` + return data +} diff --git a/internal/config/migration/multi_account.go b/internal/config/migration/multi_account.go new file mode 100644 index 000000000..92c00b6a9 --- /dev/null +++ b/internal/config/migration/multi_account.go @@ -0,0 +1,181 @@ +package migration + +import ( + "errors" + "fmt" + "net/http" + + "github.com/cli/cli/v2/internal/keyring" + ghAPI "github.com/cli/go-gh/v2/pkg/api" + "github.com/cli/go-gh/v2/pkg/config" +) + +type CowardlyRefusalError struct { + Reason string +} + +func (e CowardlyRefusalError) Error() string { + // Consider whether we should add a call to action here like "open an issue with the contents of your redacted hosts.yml" + return fmt.Sprintf("cowardly refusing to continue with multi account migration: %s", e.Reason) +} + +var hostsKey = []string{"hosts"} + +// This migration exists to take a hosts section of the following structure: +// +// github.com: +// user: williammartin +// git_protocol: https +// editor: vim +// github.localhost: +// user: monalisa +// git_protocol: https +// oauth_token: xyz +// +// We want this to migrate to something like: +// +// github.com: +// user: williammartin +// git_protocol: https +// editor: vim +// users: +// williammartin: +// git_protocol: https +// editor: vim +// +// github.localhost: +// user: monalisa +// git_protocol: https +// oauth_token: xyz +// users: +// monalisa: +// git_protocol: https +// oauth_token: xyz +// +// The reason for this is that we can then add new users under a host. +// Note that we are only copying the config under a new users key, and +// under a specific user. The original config is left alone. This is to +// allow forward compatability for older versions of gh and also to avoid +// breaking existing users of go-gh which looks at a specific location +// in the config for oauth tokens that are stored insecurely. + +type MultiAccount struct { + // Allow injecting a transport layer in tests. + Transport http.RoundTripper +} + +func (m MultiAccount) PreVersion() string { + return "" +} + +func (m MultiAccount) PostVersion() string { + return "1" +} + +func (m MultiAccount) Do(c *config.Config) error { + hostnames, err := c.Keys(hostsKey) + // [github.com, github.localhost] + // We wouldn't expect to have a hosts key when this is the first time anyone + // is logging in with the CLI. + var keyNotFoundError *config.KeyNotFoundError + if errors.As(err, &keyNotFoundError) { + return nil + } + if err != nil { + return CowardlyRefusalError{"couldn't get hosts configuration"} + } + + // If there are no hosts then it doesn't matter whether we migrate or not, + // so lets avoid any confusion and say there's no migration required. + if len(hostnames) == 0 { + return nil + } + + // Otherwise let's get to the business of migrating! + for _, hostname := range hostnames { + configEntryKeys, err := c.Keys(append(hostsKey, hostname)) + // e.g. [user, git_protocol, editor, ouath_token] + if err != nil { + return CowardlyRefusalError{fmt.Sprintf("couldn't get host configuration despite %q existing", hostname)} + } + + // Get the user so that we can nest under it in future + username, err := c.Get(append(hostsKey, hostname, "user")) + if err != nil { + return CowardlyRefusalError{fmt.Sprintf("couldn't get user name for %q", hostname)} + } + + // When anonymous user exists get the user login. + if username == "x-access-token" { + var token string + token, err := c.Get(append(hostsKey, hostname, "oauth_token")) + if err != nil || token == "" { + token, err = keyring.Get(keyringServiceName(hostname), "") + } + if err != nil || token == "" { + return CowardlyRefusalError{fmt.Sprintf("couldn't find oauth token for %q", hostname)} + } + username, err = getUsername(m.Transport, hostname, token) + if err != nil { + return CowardlyRefusalError{fmt.Sprintf("couldn't retrieve logged in user for %q", hostname)} + } + c.Set(append(hostsKey, hostname, "user"), username) + } + + // Create the username key with an empty value so it will be + // written even if there are no keys set under it. + c.Set(append(hostsKey, hostname, "users", username), "") + + for _, configEntryKey := range configEntryKeys { + // Do not re-write the user key. + if configEntryKey == "user" { + continue + } + + // We would expect that these keys map directly to values + // e.g. [williammartin, https, vim, gho_xyz...] but it's possible that a manually + // edited config file might nest further but we don't support that. + // + // We could consider throwing away the nested values, but I suppose + // I'd rather make the user take a destructive action even if we have a backup. + // If they have configuration here, it's probably for a reason. + keys, err := c.Keys(append(hostsKey, hostname, configEntryKey)) + if err == nil && len(keys) > 0 { + return CowardlyRefusalError{"hosts file has entries that are surprisingly deeply nested"} + } + + configEntryValue, err := c.Get(append(hostsKey, hostname, configEntryKey)) + if err != nil { + return CowardlyRefusalError{fmt.Sprintf("couldn't get configuration entry value despite %q / %q existing", hostname, configEntryKey)} + } + + // Set these entries in their new location under the user + c.Set(append(hostsKey, hostname, "users", username, configEntryKey), configEntryValue) + } + } + + return nil +} + +func getUsername(transport http.RoundTripper, hostname, token string) (string, error) { + opts := ghAPI.ClientOptions{ + Host: hostname, + AuthToken: token, + Transport: transport, + } + client, err := ghAPI.NewGraphQLClient(opts) + if err != nil { + return "", err + } + var query struct { + Viewer struct { + Login string + } + } + err = client.Query("CurrentUser", &query, nil) + return query.Viewer.Login, err +} + +func keyringServiceName(hostname string) string { + return "gh:" + hostname +} diff --git a/internal/config/migration/multi_account_test.go b/internal/config/migration/multi_account_test.go new file mode 100644 index 000000000..2d2c027f8 --- /dev/null +++ b/internal/config/migration/multi_account_test.go @@ -0,0 +1,185 @@ +package migration_test + +import ( + "testing" + + "github.com/cli/cli/v2/internal/config/migration" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/go-gh/v2/pkg/config" + "github.com/stretchr/testify/require" + "github.com/zalando/go-keyring" +) + +func TestMigration(t *testing.T) { + cfg := config.ReadFromString(` +hosts: + github.com: + user: user1 + oauth_token: xxxxxxxxxxxxxxxxxxxx + git_protocol: ssh + enterprise.com: + user: user2 + oauth_token: yyyyyyyyyyyyyyyyyyyy + git_protocol: https +`) + + var m migration.MultiAccount + require.NoError(t, m.Do(cfg)) + + // Do some simple checks here for depth and multiple migrations + // but I don't really want to write a full tree traversal matcher. + + // First we'll check that the data has been copied to the new structure + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "user1", "git_protocol"}, "ssh") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "user1", "oauth_token"}, "xxxxxxxxxxxxxxxxxxxx") + + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "users", "user2", "git_protocol"}, "https") + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "users", "user2", "oauth_token"}, "yyyyyyyyyyyyyyyyyyyy") + + // Then we'll check that the old data has been left alone + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "user1") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "oauth_token"}, "xxxxxxxxxxxxxxxxxxxx") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "git_protocol"}, "ssh") + + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "user"}, "user2") + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "oauth_token"}, "yyyyyyyyyyyyyyyyyyyy") + requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "git_protocol"}, "https") +} + +func TestPreVersionIsEmptyString(t *testing.T) { + var m migration.MultiAccount + require.Equal(t, "", m.PreVersion()) +} + +func TestPostVersion(t *testing.T) { + var m migration.MultiAccount + require.Equal(t, "1", m.PostVersion()) +} + +func TestMigrationErrorsWithDeeplyNestedEntries(t *testing.T) { + cfg := config.ReadFromString(` +hosts: + github.com: + user: user1 + nested: + too: deep +`) + + var m migration.MultiAccount + err := m.Do(cfg) + + require.ErrorContains(t, err, "hosts file has entries that are surprisingly deeply nested") +} + +func TestMigrationReturnsSuccessfullyWhenNoHostsEntry(t *testing.T) { + cfg := config.ReadFromString(``) + + var m migration.MultiAccount + require.NoError(t, m.Do(cfg)) +} + +func TestMigrationReturnsSuccessfullyWhenEmptyHosts(t *testing.T) { + cfg := config.ReadFromString(` +hosts: +`) + + var m migration.MultiAccount + require.NoError(t, m.Do(cfg)) +} + +func TestMigrationReturnsSuccessfullyWhenAnonymousUserExists(t *testing.T) { + // Simulates config that gets generated when a user logs + // in with a token and git protocol is not specified and + // secure storage is used. + keyring.MockInit() + require.NoError(t, keyring.Set("gh:github.com", "", "test-token")) + + cfg := config.ReadFromString(` +hosts: + github.com: + user: x-access-token +`) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query CurrentUser\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`), + ) + + m := migration.MultiAccount{Transport: reg} + require.NoError(t, m.Do(cfg)) + + require.Equal(t, "token test-token", reg.Requests[0].Header.Get("Authorization")) + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "monalisa") + // monalisa key gets created with no value + users, err := cfg.Keys([]string{"hosts", "github.com", "users"}) + require.NoError(t, err) + require.Equal(t, []string{"monalisa"}, users) +} + +func TestMigrationReturnsSuccessfullyWhenAnonymousUserExistsAndGitProtocol(t *testing.T) { + // Simulates config that gets generated when a user logs + // in with a token and git protocol is specified and + // secure storage is used. + keyring.MockInit() + require.NoError(t, keyring.Set("gh:github.com", "", "test-token")) + + cfg := config.ReadFromString(` +hosts: + github.com: + user: x-access-token + git_protocol: ssh +`) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query CurrentUser\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`), + ) + + m := migration.MultiAccount{Transport: reg} + require.NoError(t, m.Do(cfg)) + + require.Equal(t, "token test-token", reg.Requests[0].Header.Get("Authorization")) + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "monalisa") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "monalisa", "git_protocol"}, "ssh") +} + +func TestMigrationReturnsSuccessfullyWhenAnonymousUserExistsAndInsecureStorage(t *testing.T) { + // Simulates config that gets generated when a user logs + // in with a token and git protocol is specified and + // secure storage is not used. + cfg := config.ReadFromString(` +hosts: + github.com: + user: x-access-token + oauth_token: test-token + git_protocol: ssh +`) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQL(`query CurrentUser\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`), + ) + + m := migration.MultiAccount{Transport: reg} + require.NoError(t, m.Do(cfg)) + + require.Equal(t, "token test-token", reg.Requests[0].Header.Get("Authorization")) + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "monalisa") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "monalisa", "oauth_token"}, "test-token") + requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "monalisa", "git_protocol"}, "ssh") +} + +func requireKeyWithValue(t *testing.T, cfg *config.Config, keys []string, value string) { + t.Helper() + + actual, err := cfg.Get(keys) + require.NoError(t, err) + + require.Equal(t, value, actual) +} diff --git a/internal/config/migration_mock.go b/internal/config/migration_mock.go new file mode 100644 index 000000000..bf8133fb4 --- /dev/null +++ b/internal/config/migration_mock.go @@ -0,0 +1,149 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package config + +import ( + ghConfig "github.com/cli/go-gh/v2/pkg/config" + "sync" +) + +// Ensure, that MigrationMock does implement Migration. +// If this is not the case, regenerate this file with moq. +var _ Migration = &MigrationMock{} + +// MigrationMock is a mock implementation of Migration. +// +// func TestSomethingThatUsesMigration(t *testing.T) { +// +// // make and configure a mocked Migration +// mockedMigration := &MigrationMock{ +// DoFunc: func(config *ghConfig.Config) error { +// panic("mock out the Do method") +// }, +// PostVersionFunc: func() string { +// panic("mock out the PostVersion method") +// }, +// PreVersionFunc: func() string { +// panic("mock out the PreVersion method") +// }, +// } +// +// // use mockedMigration in code that requires Migration +// // and then make assertions. +// +// } +type MigrationMock struct { + // DoFunc mocks the Do method. + DoFunc func(config *ghConfig.Config) error + + // PostVersionFunc mocks the PostVersion method. + PostVersionFunc func() string + + // PreVersionFunc mocks the PreVersion method. + PreVersionFunc func() string + + // calls tracks calls to the methods. + calls struct { + // Do holds details about calls to the Do method. + Do []struct { + // Config is the config argument value. + Config *ghConfig.Config + } + // PostVersion holds details about calls to the PostVersion method. + PostVersion []struct { + } + // PreVersion holds details about calls to the PreVersion method. + PreVersion []struct { + } + } + lockDo sync.RWMutex + lockPostVersion sync.RWMutex + lockPreVersion sync.RWMutex +} + +// Do calls DoFunc. +func (mock *MigrationMock) Do(config *ghConfig.Config) error { + if mock.DoFunc == nil { + panic("MigrationMock.DoFunc: method is nil but Migration.Do was just called") + } + callInfo := struct { + Config *ghConfig.Config + }{ + Config: config, + } + mock.lockDo.Lock() + mock.calls.Do = append(mock.calls.Do, callInfo) + mock.lockDo.Unlock() + return mock.DoFunc(config) +} + +// DoCalls gets all the calls that were made to Do. +// Check the length with: +// +// len(mockedMigration.DoCalls()) +func (mock *MigrationMock) DoCalls() []struct { + Config *ghConfig.Config +} { + var calls []struct { + Config *ghConfig.Config + } + mock.lockDo.RLock() + calls = mock.calls.Do + mock.lockDo.RUnlock() + return calls +} + +// PostVersion calls PostVersionFunc. +func (mock *MigrationMock) PostVersion() string { + if mock.PostVersionFunc == nil { + panic("MigrationMock.PostVersionFunc: method is nil but Migration.PostVersion was just called") + } + callInfo := struct { + }{} + mock.lockPostVersion.Lock() + mock.calls.PostVersion = append(mock.calls.PostVersion, callInfo) + mock.lockPostVersion.Unlock() + return mock.PostVersionFunc() +} + +// PostVersionCalls gets all the calls that were made to PostVersion. +// Check the length with: +// +// len(mockedMigration.PostVersionCalls()) +func (mock *MigrationMock) PostVersionCalls() []struct { +} { + var calls []struct { + } + mock.lockPostVersion.RLock() + calls = mock.calls.PostVersion + mock.lockPostVersion.RUnlock() + return calls +} + +// PreVersion calls PreVersionFunc. +func (mock *MigrationMock) PreVersion() string { + if mock.PreVersionFunc == nil { + panic("MigrationMock.PreVersionFunc: method is nil but Migration.PreVersion was just called") + } + callInfo := struct { + }{} + mock.lockPreVersion.Lock() + mock.calls.PreVersion = append(mock.calls.PreVersion, callInfo) + mock.lockPreVersion.Unlock() + return mock.PreVersionFunc() +} + +// PreVersionCalls gets all the calls that were made to PreVersion. +// Check the length with: +// +// len(mockedMigration.PreVersionCalls()) +func (mock *MigrationMock) PreVersionCalls() []struct { +} { + var calls []struct { + } + mock.lockPreVersion.RLock() + calls = mock.calls.PreVersion + mock.lockPreVersion.RUnlock() + return calls +} diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index ae793ae85..fdfdfc244 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -173,8 +173,12 @@ func loginRun(opts *LoginOptions) error { if err := shared.HasMinimumScopes(httpClient, hostname, opts.Token); err != nil { return fmt.Errorf("error validating token: %w", err) } + username, err := shared.GetCurrentLogin(httpClient, hostname, opts.Token) + if err != nil { + return fmt.Errorf("error retrieving current user: %w", err) + } // Adding a user key ensures that a nonempty host section gets written to the config file. - _, loginErr := authCfg.Login(hostname, "x-access-token", opts.Token, opts.GitProtocol, !opts.InsecureStorage) + _, loginErr := authCfg.Login(hostname, username, opts.Token, opts.GitProtocol, !opts.InsecureStorage) return loginErr } diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index 72d796a39..c41f9ff1f 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -274,8 +274,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc123\n user: x-access-token\n", + wantHosts: "github.com:\n oauth_token: abc123\n users:\n monalisa:\n oauth_token: abc123\n user: monalisa\n", }, { name: "insecure with token and https git-protocol", @@ -287,8 +290,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc123\n user: x-access-token\n git_protocol: https\n", + wantHosts: "github.com:\n oauth_token: abc123\n users:\n monalisa:\n oauth_token: abc123\n git_protocol: https\n user: monalisa\n", }, { name: "with token and non-default host", @@ -299,8 +305,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "api/v3/"), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "albert.wesker:\n oauth_token: abc123\n user: x-access-token\n", + wantHosts: "albert.wesker:\n oauth_token: abc123\n users:\n monalisa:\n oauth_token: abc123\n user: monalisa\n", }, { name: "missing repo scope", @@ -333,8 +342,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,admin:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc456\n user: x-access-token\n", + wantHosts: "github.com:\n oauth_token: abc456\n users:\n monalisa:\n oauth_token: abc456\n user: monalisa\n", }, { name: "github.com token from environment", @@ -351,9 +363,9 @@ func Test_loginRun_nontty(t *testing.T) { }, wantErr: "SilentError", wantStderr: heredoc.Doc(` - The value of the GH_TOKEN environment variable is being used for authentication. - To have GitHub CLI store credentials instead, first clear the value from the environment. - `), + The value of the GH_TOKEN environment variable is being used for authentication. + To have GitHub CLI store credentials instead, first clear the value from the environment. + `), }, { name: "GHE token from environment", @@ -370,9 +382,9 @@ func Test_loginRun_nontty(t *testing.T) { }, wantErr: "SilentError", wantStderr: heredoc.Doc(` - The value of the GH_ENTERPRISE_TOKEN environment variable is being used for authentication. - To have GitHub CLI store credentials instead, first clear the value from the environment. - `), + The value of the GH_ENTERPRISE_TOKEN environment variable is being used for authentication. + To have GitHub CLI store credentials instead, first clear the value from the environment. + `), }, { name: "with token and secure storage", @@ -382,8 +394,11 @@ func Test_loginRun_nontty(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n user: x-access-token\n", + wantHosts: "github.com:\n user: monalisa\n users:\n monalisa:\n", wantSecureToken: "abc123", }, } @@ -485,11 +500,14 @@ func Test_loginRun_Survey(t *testing.T) { InsecureStorage: true, }, wantHosts: heredoc.Doc(` - rebecca.chambers: - oauth_token: def456 - user: jillv - git_protocol: https - `), + rebecca.chambers: + oauth_token: def456 + users: + jillv: + oauth_token: def456 + git_protocol: https + user: jillv + `), prompterStubs: func(pm *prompter.PrompterMock) { pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { switch prompt { @@ -516,11 +534,14 @@ func Test_loginRun_Survey(t *testing.T) { { name: "choose enterprise", wantHosts: heredoc.Doc(` - brad.vickers: - oauth_token: def456 - user: jillv - git_protocol: https - `), + brad.vickers: + oauth_token: def456 + users: + jillv: + oauth_token: def456 + git_protocol: https + user: jillv + `), opts: &LoginOptions{ Interactive: true, InsecureStorage: true, @@ -556,11 +577,14 @@ func Test_loginRun_Survey(t *testing.T) { { name: "choose github.com", wantHosts: heredoc.Doc(` - github.com: - oauth_token: def456 - user: jillv - git_protocol: https - `), + github.com: + oauth_token: def456 + users: + jillv: + oauth_token: def456 + git_protocol: https + user: jillv + `), opts: &LoginOptions{ Interactive: true, InsecureStorage: true, @@ -587,11 +611,14 @@ func Test_loginRun_Survey(t *testing.T) { { name: "sets git_protocol", wantHosts: heredoc.Doc(` - github.com: - oauth_token: def456 - user: jillv - git_protocol: ssh - `), + github.com: + oauth_token: def456 + users: + jillv: + oauth_token: def456 + git_protocol: ssh + user: jillv + `), opts: &LoginOptions{ Interactive: true, InsecureStorage: true, @@ -633,10 +660,12 @@ func Test_loginRun_Survey(t *testing.T) { rs.Register(`git config credential\.helper`, 1, "") }, wantHosts: heredoc.Doc(` - github.com: - user: jillv - git_protocol: https - `), + github.com: + user: jillv + users: + jillv: + git_protocol: https + `), wantErrOut: regexp.MustCompile("Logged in as jillv"), wantSecureToken: "def456", }, diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index 5f8a524cb..38ee46bde 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -177,9 +177,9 @@ func Login(opts *LoginOptions) error { if username == "" { var err error - username, err = getCurrentLogin(httpClient, hostname, authToken) + username, err = GetCurrentLogin(httpClient, hostname, authToken) if err != nil { - return fmt.Errorf("error using api: %w", err) + return fmt.Errorf("error retrieving current user: %w", err) } } @@ -238,7 +238,7 @@ func sshKeyUpload(httpClient *http.Client, hostname, keyFile string, title strin return add.SSHKeyUpload(httpClient, hostname, f, title) } -func getCurrentLogin(httpClient httpClient, hostname, authToken string) (string, error) { +func GetCurrentLogin(httpClient httpClient, hostname, authToken string) (string, error) { query := `query UserCurrent{viewer{login}}` reqBody, err := json.Marshal(map[string]interface{}{"query": query}) if err != nil {