diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 4fe0f46d7..46b5490b7 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -17,6 +17,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/build" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/config/migration" "github.com/cli/cli/v2/internal/update" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmd/root" @@ -57,7 +58,8 @@ func mainRun() exitCode { ctx := context.Background() if cfg, err := cmdFactory.Config(); err == nil { - if err := cfg.MigrateMultiAccount(); err != nil { + var m migration.MultiAccount + if err := cfg.Migrate(m); err != nil { fmt.Fprintf(stderr, "failed to migrate configuration: %s\n", err) return exitError } diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index 3cbf71311..5fe67e877 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -319,13 +319,13 @@ 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) { +// This test mimics the behaviour of logging in with a token, not providing +// a git protocol, and using secure storage. +func TestLoginAddsUserToConfigWithoutGitProtocolAndWithSecureStorage(t *testing.T) { // Given we are not logged in authCfg := newTestAuthConfig(t) - // When we log in without git protocol or secure storage + // When we log in without git protocol and with secure storage keyring.MockInit() _, err := authCfg.Login("github.com", "test-user", "test-token", "", true) require.NoError(t, err) @@ -407,7 +407,8 @@ func TestUserWorksRightAfterMigration(t *testing.T) { // When we migrate var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) // Then we can still get the user correctly user, err := authCfg.User("github.com") @@ -423,7 +424,8 @@ func TestGitProtocolWorksRightAfterMigration(t *testing.T) { // When we migrate var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) // Then we can still get the git protocol correctly gitProtocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey}) @@ -439,7 +441,8 @@ func TestHostsWorksRightAfterMigration(t *testing.T) { // When we migrate var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) // Then we can still get the hosts correctly hosts := authCfg.Hosts() @@ -454,7 +457,8 @@ func TestDefaultHostWorksRightAfterMigration(t *testing.T) { // When we migrate var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) // Then the default host is still the enterprise host defaultHost, source := authCfg.DefaultHost() @@ -470,7 +474,8 @@ func TestTokenWorksRightAfterMigration(t *testing.T) { // When we migrate var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) // Then we can still get the token correctly token, source := authCfg.Token("github.com") @@ -490,7 +495,8 @@ func TestLogoutRigthAfterMigrationRemovesHost(t *testing.T) { // When we migrate and logout var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) require.NoError(t, authCfg.Logout(host, user)) @@ -504,7 +510,8 @@ func TestLoginInsecurePostMigrationUsesConfigForToken(t *testing.T) { // When we migrate and login with insecure storage var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", false) @@ -522,7 +529,8 @@ func TestLoginPostMigrationSetsGitProtocol(t *testing.T) { authCfg := newTestAuthConfig(t) var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) require.NoError(t, err) @@ -540,7 +548,8 @@ func TestLoginPostMigrationSetsUser(t *testing.T) { authCfg := newTestAuthConfig(t) var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) _, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false) require.NoError(t, err) @@ -561,7 +570,8 @@ func TestLoginSecurePostMigrationRemovesTokenFromConfig(t *testing.T) { // When we migrate and login again with secure storage var m migration.MultiAccount - require.NoError(t, Migrate(authCfg.cfg, m)) + c := cfg{authCfg.cfg} + require.NoError(t, c.Migrate(m)) keyring.MockInit() _, err = authCfg.Login("github.com", "test-user", "test-token", "", true) diff --git a/internal/config/config.go b/internal/config/config.go index 45437885d..172e4a903 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,10 +1,10 @@ package config import ( + "fmt" "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" @@ -32,6 +32,7 @@ type Config interface { GetOrDefault(string, string) (string, error) Set(string, string, string) Write() error + Migrate(Migration) error Aliases() *AliasConfig Authentication() *AuthConfig @@ -41,8 +42,27 @@ type Config interface { HTTPUnixSocket(string) string Pager(string) string Prompt(string) string + Version() string +} - MigrateMultiAccount() error +// 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. +// +//go:generate moq -rm -out migration_mock.go . Migration +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 NewConfig() (Config, error) { @@ -132,9 +152,36 @@ func (c *cfg) Prompt(hostname string) string { return val } -func (c *cfg) MigrateMultiAccount() error { - var m migration.MultiAccount - return Migrate(c.cfg, m) +func (c *cfg) Version() string { + val, _ := c.GetOrDefault("", versionKey) + return val +} + +func (c *cfg) Migrate(m Migration) error { + version := c.Version() + + // If migration has already occured then do not attempt to migrate again. + if m.PostVersion() == version { + return nil + } + + // If migration is incompatible with current version then return an error. + 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.cfg); err != nil { + return fmt.Errorf("failed to migrate config: %s", err) + } + + c.Set("", versionKey, m.PostVersion()) + + // Then write out our migrated config. + if err := c.Write(); err != nil { + return fmt.Errorf("failed to write config after migration: %s", err) + } + + return nil } func defaultFor(key string) (string, bool) { @@ -250,7 +297,10 @@ func (c *AuthConfig) SetDefaultHost(host, source string) { func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) { var setErr error if secureStorage { + // Set the current active oauth token if setErr = keyring.Set(keyringServiceName(hostname), "", token); setErr == nil { + // And set the oauth token under the user to support later auth switch + // and logout switch without another migration. setErr = keyring.Set(keyringServiceName(hostname), username, token) } if setErr == nil { diff --git a/internal/config/config_mock.go b/internal/config/config_mock.go index 53e17998e..586078a2c 100644 --- a/internal/config/config_mock.go +++ b/internal/config/config_mock.go @@ -38,8 +38,8 @@ var _ Config = &ConfigMock{} // HTTPUnixSocketFunc: func(s string) string { // panic("mock out the HTTPUnixSocket method") // }, -// MigrateMultiAccountFunc: func() error { -// panic("mock out the MigrateMultiAccount method") +// MigrateFunc: func(migration Migration) error { +// panic("mock out the Migrate method") // }, // PagerFunc: func(s string) string { // panic("mock out the Pager method") @@ -50,6 +50,9 @@ var _ Config = &ConfigMock{} // SetFunc: func(s1 string, s2 string, s3 string) { // panic("mock out the Set method") // }, +// VersionFunc: func() string { +// panic("mock out the Version method") +// }, // WriteFunc: func() error { // panic("mock out the Write method") // }, @@ -81,8 +84,8 @@ type ConfigMock struct { // HTTPUnixSocketFunc mocks the HTTPUnixSocket method. HTTPUnixSocketFunc func(s string) string - // MigrateMultiAccountFunc mocks the MigrateMultiAccount method. - MigrateMultiAccountFunc func() error + // MigrateFunc mocks the Migrate method. + MigrateFunc func(migration Migration) error // PagerFunc mocks the Pager method. PagerFunc func(s string) string @@ -93,6 +96,9 @@ type ConfigMock struct { // SetFunc mocks the Set method. SetFunc func(s1 string, s2 string, s3 string) + // VersionFunc mocks the Version method. + VersionFunc func() string + // WriteFunc mocks the Write method. WriteFunc func() error @@ -131,8 +137,10 @@ type ConfigMock struct { // S is the s argument value. S string } - // MigrateMultiAccount holds details about calls to the MigrateMultiAccount method. - MigrateMultiAccount []struct { + // Migrate holds details about calls to the Migrate method. + Migrate []struct { + // Migration is the migration argument value. + Migration Migration } // Pager holds details about calls to the Pager method. Pager []struct { @@ -153,22 +161,26 @@ type ConfigMock struct { // S3 is the s3 argument value. S3 string } + // Version holds details about calls to the Version method. + Version []struct { + } // Write holds details about calls to the Write method. Write []struct { } } - 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 + lockAliases sync.RWMutex + lockAuthentication sync.RWMutex + lockBrowser sync.RWMutex + lockEditor sync.RWMutex + lockGetOrDefault sync.RWMutex + lockGitProtocol sync.RWMutex + lockHTTPUnixSocket sync.RWMutex + lockMigrate sync.RWMutex + lockPager sync.RWMutex + lockPrompt sync.RWMutex + lockSet sync.RWMutex + lockVersion sync.RWMutex + lockWrite sync.RWMutex } // Aliases calls AliasesFunc. @@ -389,30 +401,35 @@ 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") +// Migrate calls MigrateFunc. +func (mock *ConfigMock) Migrate(migration Migration) error { + if mock.MigrateFunc == nil { + panic("ConfigMock.MigrateFunc: method is nil but Config.Migrate was just called") } callInfo := struct { - }{} - mock.lockMigrateMultiAccount.Lock() - mock.calls.MigrateMultiAccount = append(mock.calls.MigrateMultiAccount, callInfo) - mock.lockMigrateMultiAccount.Unlock() - return mock.MigrateMultiAccountFunc() + Migration Migration + }{ + Migration: migration, + } + mock.lockMigrate.Lock() + mock.calls.Migrate = append(mock.calls.Migrate, callInfo) + mock.lockMigrate.Unlock() + return mock.MigrateFunc(migration) } -// MigrateMultiAccountCalls gets all the calls that were made to MigrateMultiAccount. +// MigrateCalls gets all the calls that were made to Migrate. // Check the length with: // -// len(mockedConfig.MigrateMultiAccountCalls()) -func (mock *ConfigMock) MigrateMultiAccountCalls() []struct { +// len(mockedConfig.MigrateCalls()) +func (mock *ConfigMock) MigrateCalls() []struct { + Migration Migration } { var calls []struct { + Migration Migration } - mock.lockMigrateMultiAccount.RLock() - calls = mock.calls.MigrateMultiAccount - mock.lockMigrateMultiAccount.RUnlock() + mock.lockMigrate.RLock() + calls = mock.calls.Migrate + mock.lockMigrate.RUnlock() return calls } @@ -520,6 +537,33 @@ func (mock *ConfigMock) SetCalls() []struct { return calls } +// Version calls VersionFunc. +func (mock *ConfigMock) Version() string { + if mock.VersionFunc == nil { + panic("ConfigMock.VersionFunc: method is nil but Config.Version was just called") + } + callInfo := struct { + }{} + mock.lockVersion.Lock() + mock.calls.Version = append(mock.calls.Version, callInfo) + mock.lockVersion.Unlock() + return mock.VersionFunc() +} + +// VersionCalls gets all the calls that were made to Version. +// Check the length with: +// +// len(mockedConfig.VersionCalls()) +func (mock *ConfigMock) VersionCalls() []struct { +} { + var calls []struct { + } + mock.lockVersion.RLock() + calls = mock.calls.Version + mock.lockVersion.RUnlock() + return calls +} + // Write calls WriteFunc. func (mock *ConfigMock) Write() error { if mock.WriteFunc == nil { diff --git a/internal/config/migrate.go b/internal/config/migrate.go deleted file mode 100644 index 06a38027f..000000000 --- a/internal/config/migrate.go +++ /dev/null @@ -1,56 +0,0 @@ -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 migration has already occured then do not attempt to migrate again. - if m.PostVersion() == version { - return nil - } - - // If migration is incompatible with current version then return an error. - 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 index 62f9d6062..193848558 100644 --- a/internal/config/migrate_test.go +++ b/internal/config/migrate_test.go @@ -17,7 +17,8 @@ func TestMigrationAppliedSuccessfully(t *testing.T) { // Given we have a migrator that writes some keys to the top level config // and hosts key - cfg := ghConfig.ReadFromString(testFullConfig()) + c := ghConfig.ReadFromString(testFullConfig()) + topLevelKey := []string{"toplevelkey"} newHostKey := []string{hostsKey, "newhost"} @@ -28,11 +29,12 @@ func TestMigrationAppliedSuccessfully(t *testing.T) { }) // When we run the migration - require.NoError(t, Migrate(cfg, migration)) + conf := cfg{c} + require.NoError(t, conf.Migrate(migration)) // Then our original config is updated with the migration applied - requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue") - requireKeyWithValue(t, cfg, newHostKey, "newhostvalue") + requireKeyWithValue(t, c, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, c, 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 @@ -54,8 +56,8 @@ func TestMigrationAppliedBumpsVersion(t *testing.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") + c := ghConfig.ReadFromString(testFullConfig()) + c.Set([]string{versionKey}, "expected-pre-version") topLevelKey := []string{"toplevelkey"} migration := &MigrationMock{ @@ -72,11 +74,12 @@ func TestMigrationAppliedBumpsVersion(t *testing.T) { } // When we migrate - require.NoError(t, Migrate(cfg, migration)) + conf := cfg{c} + require.NoError(t, conf.Migrate(migration)) // Then our original config is updated with the migration applied - requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue") - requireKeyWithValue(t, cfg, []string{versionKey}, "expected-post-version") + requireKeyWithValue(t, c, topLevelKey, "toplevelvalue") + requireKeyWithValue(t, c, []string{versionKey}, "expected-post-version") // And our config / hosts changes are persisted to their relevant files var configBuf bytes.Buffer @@ -87,13 +90,40 @@ func TestMigrationAppliedBumpsVersion(t *testing.T) { requireKeyWithValue(t, persistedCfg, []string{versionKey}, "expected-post-version") } +func TestMigrationIsNoopWhenAlreadyApplied(t *testing.T) { + // Given we have a migration with a post version that matches + // the version in the config + c := ghConfig.ReadFromString(testFullConfig()) + c.Set([]string{versionKey}, "expected-post-version") + + migration := &MigrationMock{ + DoFunc: func(config *ghConfig.Config) error { + return errors.New("is not called") + }, + PreVersionFunc: func() string { + return "is not called" + }, + PostVersionFunc: func() string { + return "expected-post-version" + }, + } + + // When we run Migrate + conf := cfg{c} + err := conf.Migrate(migration) + + // Then there is nothing done and the config is not modified + require.NoError(t, err) + requireKeyWithValue(t, c, []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") + c := ghConfig.ReadFromString(testFullConfig()) + c.Set([]string{versionKey}, "not-expected-pre-version") topLevelKey := []string{"toplevelkey"} migration := &MigrationMock{ @@ -110,12 +140,13 @@ func TestMigrationErrorsWhenPreVersionMismatch(t *testing.T) { } // When we run Migrate - err := Migrate(cfg, migration) + conf := cfg{c} + err := conf.Migrate(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") + requireNoKey(t, c, topLevelKey) + requireKeyWithValue(t, c, []string{versionKey}, "not-expected-pre-version") } func TestMigrationErrorWritesNoFiles(t *testing.T) { @@ -123,13 +154,14 @@ func TestMigrationErrorWritesNoFiles(t *testing.T) { t.Setenv("GH_CONFIG_DIR", tempDir) // Given we have a migrator that errors - cfg := ghConfig.ReadFromString(testFullConfig()) + c := 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) + conf := cfg{c} + err := conf.Migrate(migration) // Then the error is wrapped and bubbled require.EqualError(t, err, "failed to migrate config: failed to migrate in test") @@ -166,18 +198,19 @@ func TestMigrationWriteErrors(t *testing.T) { // 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()) + c := 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") + migration := mockMigration(func(someCfg *ghConfig.Config) error { + someCfg.Set(topLevelKey, "toplevelvalue") + someCfg.Set(hostsKey, "newhostvalue") return nil }) // When we run the migration - err := Migrate(cfg, migration) + conf := cfg{c} + err := conf.Migrate(migration) // Then the error is wrapped and bubbled require.ErrorContains(t, err, tt.wantErrContains) diff --git a/internal/config/migration/multi_account.go b/internal/config/migration/multi_account.go index d8a6ed142..92a84d131 100644 --- a/internal/config/migration/multi_account.go +++ b/internal/config/migration/multi_account.go @@ -65,6 +65,8 @@ type MultiAccount struct { } func (m MultiAccount) PreVersion() string { + // It is expected that there is no version key since this migration + // introduces it. return "" } @@ -149,7 +151,10 @@ func getUsername(c *config.Config, hostname, token string, transport http.RoundT } } err = client.Query("CurrentUser", &query, nil) - return query.Viewer.Login, err + if err != nil { + return "", err + } + return query.Viewer.Login, nil } func migrateToken(hostname, username, token string, inKeyring bool) error { diff --git a/internal/config/stub.go b/internal/config/stub.go index 143c9b870..10d409804 100644 --- a/internal/config/stub.go +++ b/internal/config/stub.go @@ -26,6 +26,9 @@ func NewFromString(cfgStr string) *ConfigMock { mock.WriteFunc = func() error { return cfg.Write() } + mock.MigrateFunc = func(m Migration) error { + return cfg.Migrate(m) + } mock.AliasesFunc = func() *AliasConfig { return &AliasConfig{cfg: c} } @@ -69,6 +72,10 @@ func NewFromString(cfgStr string) *ConfigMock { val, _ := cfg.GetOrDefault(hostname, promptKey) return val } + mock.VersionFunc = func() string { + val, _ := cfg.GetOrDefault("", versionKey) + return val + } return mock }