diff --git a/internal/config/migration/multi_account.go b/internal/config/migration/multi_account.go index 6163856b7..d4e1e7a67 100644 --- a/internal/config/migration/multi_account.go +++ b/internal/config/migration/multi_account.go @@ -47,8 +47,6 @@ type tokenSource struct { // editor: vim // users: // williammartin: -// git_protocol: https -// editor: vim // // github.localhost: // user: monalisa @@ -56,9 +54,13 @@ type tokenSource struct { // oauth_token: xyz // users: // monalisa: -// git_protocol: https // oauth_token: xyz // +// For each hosts, we will create a new key `users` with the value of the host level +// `user` key as a list entry. If there is a host level `oauth_token` we will +// put that under the new user entry, otherwise there will be no value for the +// new user key. No other host level configuration will be copied to the new user. +// // 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 @@ -198,39 +200,21 @@ func migrateConfig(c *config.Config, hostname, username string) error { // written even if there are no keys set under it. c.Set(append(hostsKey, hostname, "users", username), "") - // e.g. [user, git_protocol, editor, ouath_token] - configEntryKeys, err := c.Keys(append(hostsKey, hostname)) + insecureToken, err := c.Get(append(hostsKey, hostname, "oauth_token")) + var keyNotFoundError *config.KeyNotFoundError + // If there is no token then we're done here + if errors.As(err, &keyNotFoundError) { + return nil + } + + // If there's another error (current implementation doesn't have any other error but we'll be defensive) + // then bubble something up. if err != nil { - return fmt.Errorf("couldn't get host configuration despite %q existing", hostname) - } - - for _, configEntryKey := range configEntryKeys { - // Do not re-write process the user and users keys. - if configEntryKey == "user" || configEntryKey == "users" { - 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 errors.New("hosts file has entries that are surprisingly deeply nested") - } - - configEntryValue, err := c.Get(append(hostsKey, hostname, configEntryKey)) - if err != nil { - return fmt.Errorf("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 fmt.Errorf("couldn't get oauth token for %s: %s", hostname, err) } + // Otherwise we'll set the token under the new key + c.Set(append(hostsKey, hostname, "users", username, "oauth_token"), insecureToken) return nil } diff --git a/internal/config/migration/multi_account_test.go b/internal/config/migration/multi_account_test.go index f70839881..8feb853bd 100644 --- a/internal/config/migration/multi_account_test.go +++ b/internal/config/migration/multi_account_test.go @@ -28,14 +28,8 @@ hosts: 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") + // First we'll check that the oauth tokens have been moved to their new locations 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 @@ -89,10 +83,9 @@ hosts: require.NoError(t, err) require.Equal(t, userTwoToken, gotUserTwoToken) - // First we'll check that the data has been copied to the new structure - requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "users", "userOne", "git_protocol"}, "ssh") - - requireKeyWithValue(t, cfg, []string{"hosts", "enterprise.com", "users", "userTwo", "git_protocol"}, "https") + // First we'll check that the users have been created with no config underneath them + requireKeyExists(t, cfg, []string{"hosts", "github.com", "users", "userOne"}) + requireKeyExists(t, cfg, []string{"hosts", "enterprise.com", "users", "userTwo"}) // Then we'll check that the old data has been left alone requireKeyWithValue(t, cfg, []string{"hosts", "github.com", "user"}, "userOne") @@ -112,21 +105,6 @@ func TestPostVersion(t *testing.T) { 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(``) @@ -185,46 +163,6 @@ hosts: require.Equal(t, token, gotToken) } -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. - token := "test-token" - keyring.MockInit() - require.NoError(t, keyring.Set("gh:github.com", "", 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, fmt.Sprintf("token %s", 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") - - // Verify token gets stored with host and username - gotToken, err := keyring.Get("gh:github.com", "monalisa") - require.NoError(t, err) - require.Equal(t, token, gotToken) - - // Verify token still exists with only host - gotToken, err = keyring.Get("gh:github.com", "") - require.NoError(t, err) - require.Equal(t, token, gotToken) -} - func TestMigrationReturnsSuccessfullyWhenAnonymousUserExistsAndInsecureStorage(t *testing.T) { // Simulates config that gets generated when a user logs // in with a token and git protocol is specified and @@ -250,7 +188,6 @@ hosts: 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 TestMigrationRemovesHostsWithInvalidTokens(t *testing.T) { @@ -287,6 +224,13 @@ hosts: require.ErrorContains(t, err, `couldn't find oauth token for "github.com": keyring test error`) } +func requireKeyExists(t *testing.T, cfg *config.Config, keys []string) { + t.Helper() + + _, err := cfg.Get(keys) + require.NoError(t, err) +} + func requireKeyWithValue(t *testing.T, cfg *config.Config, keys []string, value string) { t.Helper()