From a9acece7dd05304b699d74749a7c1c299560613a Mon Sep 17 00:00:00 2001 From: William Martin Date: Sun, 26 Nov 2023 14:49:09 +0100 Subject: [PATCH] Split Login into adding a user and switching It makes clear the steps that should be needed to "switch" which should be shared between Login (add user and switch to it), Logout (remove user and switch to another), and Switch (no modification and switch to a user) --- internal/config/config.go | 55 ++++++++++++++++++++++---------- pkg/cmd/auth/login/login_test.go | 32 +++++++++---------- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index ed82df99f..b779fca61 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -300,35 +300,24 @@ func (c *AuthConfig) SetDefaultHost(host, source string) { // If the encrypt option is specified it will first try to store the auth token // in encrypted storage and will fall back to the plain text config file. func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) { + // In this section we set up the users config 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) - } + // Try to set the token for this user in the encrypted storage for later switching + setErr = keyring.Set(keyringServiceName(hostname), username, token) if setErr == nil { - // Clean up the previous oauth_tokens from the config file. - _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + // Clean up the previous oauth_token from the config file, if there were one _ = 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. + // And set the oauth token under the user for later switching c.cfg.Set([]string{hostsKey, hostname, usersKey, username, oauthTokenKey}, token) insecureStorageUsed = true } - c.cfg.Set([]string{hostsKey, hostname, userKey}, username) - if gitProtocol != "" { - // Set the git protocol - c.cfg.Set([]string{hostsKey, hostname, gitProtocolKey}, gitProtocol) // And set the git protocol under the user to support later auth switch // and logout switch without another migration. c.cfg.Set([]string{hostsKey, hostname, usersKey, username, gitProtocolKey}, gitProtocol) @@ -340,7 +329,39 @@ func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secure c.cfg.Set([]string{hostsKey, hostname, usersKey, username}, "") } - return insecureStorageUsed, ghConfig.Write(c.cfg) + // Then we perform a switch to the new user + return insecureStorageUsed, c.switchUser(hostname, username) +} + +func (c *AuthConfig) switchUser(hostname, user string) error { + // We first need to idempotently clear out any set tokens for the host + _ = keyring.Delete(keyringServiceName(hostname), "") + _ = c.cfg.Remove([]string{hostsKey, hostname, oauthTokenKey}) + + // Then we'll move the keyring token or insecure token as necessary, only one of the + // following branches should be true. + + // If there is a token in the secure keyring for the user, move it to the active slot + if token, err := keyring.Get(keyringServiceName(hostname), user); err == nil { + if err = keyring.Set(keyringServiceName(hostname), "", token); err != nil { + return fmt.Errorf("failed to move active token in keyring: %v", err) + } + } + + // If there is a token in the insecure config for the user, move it to the active field + if token, err := c.cfg.Get([]string{hostsKey, hostname, usersKey, user, oauthTokenKey}); err == nil { + c.cfg.Set([]string{hostsKey, hostname, oauthTokenKey}, token) + } + + // Then we'll ensure the git protocol is moved as well + if gitProtocol, err := c.cfg.Get([]string{hostsKey, hostname, usersKey, user, gitProtocolKey}); err == nil { + c.cfg.Set([]string{hostsKey, hostname, gitProtocolKey}, gitProtocol) + } + + // Then we'll update the active user for the host + c.cfg.Set([]string{hostsKey, hostname, userKey}, user) + + return ghConfig.Write(c.cfg) } // Logout will remove user, git protocol, and auth token for the given hostname. diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index 9a909426c..ec98138b4 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -279,7 +279,7 @@ func Test_loginRun_nontty(t *testing.T) { httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc123\n users:\n monalisa:\n oauth_token: abc123\n user: monalisa\n", + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: abc123\n oauth_token: abc123\n user: monalisa\n", }, { name: "insecure with token and https git-protocol", @@ -295,7 +295,7 @@ func Test_loginRun_nontty(t *testing.T) { httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc123\n users:\n monalisa:\n oauth_token: abc123\n git_protocol: https\n user: monalisa\n git_protocol: https\n", + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: abc123\n git_protocol: https\n oauth_token: abc123\n git_protocol: https\n user: monalisa\n", }, { name: "with token and non-default host", @@ -310,7 +310,7 @@ func Test_loginRun_nontty(t *testing.T) { httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "albert.wesker:\n oauth_token: abc123\n users:\n monalisa:\n oauth_token: abc123\n user: monalisa\n", + wantHosts: "albert.wesker:\n users:\n monalisa:\n oauth_token: abc123\n oauth_token: abc123\n user: monalisa\n", }, { name: "missing repo scope", @@ -347,7 +347,7 @@ func Test_loginRun_nontty(t *testing.T) { httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n oauth_token: abc456\n users:\n monalisa:\n oauth_token: abc456\n user: monalisa\n", + wantHosts: "github.com:\n users:\n monalisa:\n oauth_token: abc456\n oauth_token: abc456\n user: monalisa\n", }, { name: "github.com token from environment", @@ -399,7 +399,7 @@ func Test_loginRun_nontty(t *testing.T) { httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data":{"viewer":{"login":"monalisa"}}}`)) }, - wantHosts: "github.com:\n user: monalisa\n users:\n monalisa:\n", + wantHosts: "github.com:\n users:\n monalisa:\n user: monalisa\n", wantSecureToken: "abc123", }, { @@ -425,8 +425,8 @@ func Test_loginRun_nontty(t *testing.T) { oauth_token: abc123 git_protocol: https newUser: - user: newUser git_protocol: https + user: newUser `), wantSecureToken: "newUserToken", }, @@ -504,13 +504,13 @@ func Test_loginRun_Survey(t *testing.T) { }, wantHosts: heredoc.Doc(` rebecca.chambers: - oauth_token: def456 users: jillv: oauth_token: def456 git_protocol: https - user: jillv + oauth_token: def456 git_protocol: https + user: jillv `), prompterStubs: func(pm *prompter.PrompterMock) { pm.SelectFunc = func(prompt, _ string, opts []string) (int, error) { @@ -539,13 +539,13 @@ func Test_loginRun_Survey(t *testing.T) { name: "choose enterprise", wantHosts: heredoc.Doc(` brad.vickers: - oauth_token: def456 users: jillv: oauth_token: def456 git_protocol: https - user: jillv + oauth_token: def456 git_protocol: https + user: jillv `), opts: &LoginOptions{ Interactive: true, @@ -583,13 +583,13 @@ func Test_loginRun_Survey(t *testing.T) { name: "choose github.com", wantHosts: heredoc.Doc(` github.com: - oauth_token: def456 users: jillv: oauth_token: def456 git_protocol: https - user: jillv + oauth_token: def456 git_protocol: https + user: jillv `), opts: &LoginOptions{ Interactive: true, @@ -618,13 +618,13 @@ func Test_loginRun_Survey(t *testing.T) { name: "sets git_protocol", wantHosts: heredoc.Doc(` github.com: - oauth_token: def456 users: jillv: oauth_token: def456 git_protocol: ssh - user: jillv + oauth_token: def456 git_protocol: ssh + user: jillv `), opts: &LoginOptions{ Interactive: true, @@ -668,11 +668,11 @@ func Test_loginRun_Survey(t *testing.T) { }, wantHosts: heredoc.Doc(` github.com: - user: jillv - git_protocol: https users: jillv: git_protocol: https + git_protocol: https + user: jillv `), wantErrOut: regexp.MustCompile("Logged in as jillv"), wantSecureToken: "def456",