From a3d65e0dcee92c68f55a518b1557b456400829b6 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 12:28:58 +0200 Subject: [PATCH 01/12] Extract units for configuring and updating git credential helpers --- pkg/cmd/auth/refresh/refresh.go | 13 +- pkg/cmd/auth/setupgit/setupgit.go | 12 +- pkg/cmd/auth/shared/git_credential.go | 127 ++++-------------- pkg/cmd/auth/shared/git_credential_test.go | 27 ++-- .../shared/gitcredentials/gitcredentials.go | 117 ++++++++++++++++ pkg/cmd/auth/shared/login_flow.go | 13 +- 6 files changed, 190 insertions(+), 119 deletions(-) create mode 100644 pkg/cmd/auth/shared/gitcredentials/gitcredentials.go diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 262b8fb31..fed0aae03 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/v2/internal/authflow" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/pkg/cmd/auth/shared" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/set" @@ -173,9 +174,15 @@ func refreshRun(opts *RefreshOptions) error { } credentialFlow := &shared.GitCredentialFlow{ - Executable: opts.MainExecutable, - Prompter: opts.Prompter, - GitClient: opts.GitClient, + Prompter: opts.Prompter, + GitClient: opts.GitClient, + HelperConfigurer: &gitcredentials.HelperConfigurer{ + SelfExecutablePath: opts.MainExecutable, + GitClient: opts.GitClient, + }, + Updater: &gitcredentials.Updater{ + GitClient: opts.GitClient, + }, } gitProtocol := cfg.GitProtocol(hostname).Value if opts.Interactive && gitProtocol == "https" { diff --git a/pkg/cmd/auth/setupgit/setupgit.go b/pkg/cmd/auth/setupgit/setupgit.go index 6bf85a7f4..e134ed10b 100644 --- a/pkg/cmd/auth/setupgit/setupgit.go +++ b/pkg/cmd/auth/setupgit/setupgit.go @@ -7,6 +7,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/pkg/cmd/auth/shared" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/spf13/cobra" @@ -53,8 +54,14 @@ func NewCmdSetupGit(f *cmdutil.Factory, runF func(*SetupGitOptions) error) *cobr `), RunE: func(cmd *cobra.Command, args []string) error { opts.gitConfigure = &shared.GitCredentialFlow{ - Executable: f.Executable(), - GitClient: f.GitClient, + GitClient: f.GitClient, + HelperConfigurer: &gitcredentials.HelperConfigurer{ + SelfExecutablePath: f.Executable(), + GitClient: f.GitClient, + }, + Updater: &gitcredentials.Updater{ + GitClient: f.GitClient, + }, } if opts.Hostname == "" && opts.Force { return cmdutil.FlagErrorf("`--force` must be used in conjunction with `--hostname`") @@ -111,6 +118,7 @@ func setupGitRun(opts *SetupGitOptions) error { } for _, hostname := range hostnames { + // TODO: Use gitcredentials.HelperConfigurer here if err := opts.gitConfigure.Setup(hostname, "", ""); err != nil { return fmt.Errorf("failed to set up git credential helper: %s", err) } diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index 8624cf00b..6d522acb6 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -1,23 +1,24 @@ package shared import ( - "bytes" "context" "errors" "fmt" "path/filepath" "strings" - "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" "github.com/google/shlex" ) type GitCredentialFlow struct { - Executable string - Prompter Prompt - GitClient *git.Client + Prompter Prompt + GitClient *git.Client + + HelperConfigurer *gitcredentials.HelperConfigurer + Updater *gitcredentials.Updater shouldSetup bool helper string @@ -61,101 +62,14 @@ func (flow *GitCredentialFlow) Setup(hostname, username, authToken string) error } func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password string) error { - gitClient := flow.GitClient - ctx := context.Background() - + // If there is no credential helper configured then we will set ourselves up as + // the credential helper for this host. if flow.helper == "" { - credHelperKeys := []string{ - gitCredentialHelperKey(hostname), - } - - gistHost := strings.TrimSuffix(ghinstance.GistHost(hostname), "/") - if strings.HasPrefix(gistHost, "gist.") { - credHelperKeys = append(credHelperKeys, gitCredentialHelperKey(gistHost)) - } - - var configErr error - - for _, credHelperKey := range credHelperKeys { - if configErr != nil { - break - } - // first use a blank value to indicate to git we want to sever the chain of credential helpers - preConfigureCmd, err := gitClient.Command(ctx, "config", "--global", "--replace-all", credHelperKey, "") - if err != nil { - configErr = err - break - } - if _, err = preConfigureCmd.Output(); err != nil { - configErr = err - break - } - - // second configure the actual helper for this host - configureCmd, err := gitClient.Command(ctx, - "config", "--global", "--add", - credHelperKey, - fmt.Sprintf("!%s auth git-credential", shellQuote(flow.Executable)), - ) - if err != nil { - configErr = err - } else { - _, configErr = configureCmd.Output() - } - } - - return configErr + return flow.HelperConfigurer.Configure(hostname) } - // clear previous cached credentials - rejectCmd, err := gitClient.Command(ctx, "credential", "reject") - if err != nil { - return err - } - - rejectCmd.Stdin = bytes.NewBufferString(heredoc.Docf(` - protocol=https - host=%s - `, hostname)) - - _, err = rejectCmd.Output() - if err != nil { - return err - } - - approveCmd, err := gitClient.Command(ctx, "credential", "approve") - if err != nil { - return err - } - - approveCmd.Stdin = bytes.NewBufferString(heredoc.Docf(` - protocol=https - host=%s - username=%s - password=%s - `, hostname, username, password)) - - _, err = approveCmd.Output() - if err != nil { - return err - } - - return nil -} - -func gitCredentialHelperKey(hostname string) string { - host := strings.TrimSuffix(ghinstance.HostPrefix(hostname), "/") - return fmt.Sprintf("credential.%s.helper", host) -} - -func gitCredentialHelper(gitClient *git.Client, hostname string) (helper string, err error) { - ctx := context.Background() - helper, err = gitClient.Config(ctx, gitCredentialHelperKey(hostname)) - if helper != "" { - return - } - helper, err = gitClient.Config(ctx, "credential.helper") - return + // Otherwise, we'll tell git to inform the existing credential helper of the new credentials. + return flow.Updater.Update(hostname, username, password) } func isOurCredentialHelper(cmd string) bool { @@ -179,9 +93,18 @@ func isGitMissing(err error) bool { return errors.As(err, &errNotInstalled) } -func shellQuote(s string) string { - if strings.ContainsAny(s, " $\\") { - return "'" + s + "'" - } - return s +func gitCredentialHelperKey(hostname string) string { + host := strings.TrimSuffix(ghinstance.HostPrefix(hostname), "/") + return fmt.Sprintf("credential.%s.helper", host) +} + +func gitCredentialHelper(gitClient *git.Client, hostname string) (helper string, err error) { + ctx := context.TODO() + + helper, err = gitClient.Config(ctx, gitCredentialHelperKey(hostname)) + if helper != "" { + return + } + helper, err = gitClient.Config(ctx, "credential.helper") + return } diff --git a/pkg/cmd/auth/shared/git_credential_test.go b/pkg/cmd/auth/shared/git_credential_test.go index 9d3e90cc4..017c649dd 100644 --- a/pkg/cmd/auth/shared/git_credential_test.go +++ b/pkg/cmd/auth/shared/git_credential_test.go @@ -5,6 +5,7 @@ import ( "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/run" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" ) func TestGitCredentialSetup_configureExisting(t *testing.T) { @@ -14,9 +15,11 @@ func TestGitCredentialSetup_configureExisting(t *testing.T) { cs.Register(`git credential approve`, 0, "") f := GitCredentialFlow{ - Executable: "gh", - helper: "osxkeychain", - GitClient: &git.Client{GitPath: "some/path/git"}, + helper: "osxkeychain", + GitClient: &git.Client{GitPath: "some/path/git"}, + Updater: &gitcredentials.Updater{ + GitClient: &git.Client{GitPath: "some/path/git"}, + }, } if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { @@ -61,9 +64,12 @@ func TestGitCredentialsSetup_setOurs_GH(t *testing.T) { }) f := GitCredentialFlow{ - Executable: "/path/to/gh", - helper: "", - GitClient: &git.Client{GitPath: "some/path/git"}, + helper: "", + GitClient: &git.Client{GitPath: "some/path/git"}, + HelperConfigurer: &gitcredentials.HelperConfigurer{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{GitPath: "some/path/git"}, + }, } if err := f.gitCredentialSetup("github.com", "monalisa", "PASSWD"); err != nil { @@ -93,9 +99,12 @@ func TestGitCredentialSetup_setOurs_nonGH(t *testing.T) { }) f := GitCredentialFlow{ - Executable: "/path/to/gh", - helper: "", - GitClient: &git.Client{GitPath: "some/path/git"}, + helper: "", + GitClient: &git.Client{GitPath: "some/path/git"}, + HelperConfigurer: &gitcredentials.HelperConfigurer{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{GitPath: "some/path/git"}, + }, } if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go new file mode 100644 index 000000000..d4f3616e3 --- /dev/null +++ b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go @@ -0,0 +1,117 @@ +package gitcredentials + +import ( + "bytes" + "context" + "fmt" + "strings" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/internal/ghinstance" +) + +type HelperConfigurer struct { + SelfExecutablePath string + GitClient *git.Client +} + +func (hc *HelperConfigurer) Configure(hostname string) error { + ctx := context.TODO() + + credHelperKeys := []string{ + gitCredentialHelperKey(hostname), + } + + gistHost := strings.TrimSuffix(ghinstance.GistHost(hostname), "/") + if strings.HasPrefix(gistHost, "gist.") { + credHelperKeys = append(credHelperKeys, gitCredentialHelperKey(gistHost)) + } + + var configErr error + + for _, credHelperKey := range credHelperKeys { + if configErr != nil { + break + } + // first use a blank value to indicate to git we want to sever the chain of credential helpers + preConfigureCmd, err := hc.GitClient.Command(ctx, "config", "--global", "--replace-all", credHelperKey, "") + if err != nil { + configErr = err + break + } + if _, err = preConfigureCmd.Output(); err != nil { + configErr = err + break + } + + // second configure the actual helper for this host + configureCmd, err := hc.GitClient.Command(ctx, + "config", "--global", "--add", + credHelperKey, + fmt.Sprintf("!%s auth git-credential", shellQuote(hc.SelfExecutablePath)), + ) + if err != nil { + configErr = err + } else { + _, configErr = configureCmd.Output() + } + } + + return configErr +} + +func gitCredentialHelperKey(hostname string) string { + host := strings.TrimSuffix(ghinstance.HostPrefix(hostname), "/") + return fmt.Sprintf("credential.%s.helper", host) +} + +func shellQuote(s string) string { + if strings.ContainsAny(s, " $\\") { + return "'" + s + "'" + } + return s +} + +type Updater struct { + GitClient *git.Client +} + +func (u *Updater) Update(hostname, username, password string) error { + ctx := context.TODO() + + // clear previous cached credentials + rejectCmd, err := u.GitClient.Command(ctx, "credential", "reject") + if err != nil { + return err + } + + rejectCmd.Stdin = bytes.NewBufferString(heredoc.Docf(` + protocol=https + host=%s + `, hostname)) + + _, err = rejectCmd.Output() + if err != nil { + return err + } + + approveCmd, err := u.GitClient.Command(ctx, "credential", "approve") + if err != nil { + return err + } + + approveCmd.Stdin = bytes.NewBufferString(heredoc.Docf(` + protocol=https + host=%s + username=%s + password=%s + `, hostname, username, password)) + + _, err = approveCmd.Output() + if err != nil { + return err + } + + return nil +} diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index 5a1df34c6..dc6ff3cad 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -15,6 +15,7 @@ import ( "github.com/cli/cli/v2/internal/authflow" "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" "github.com/cli/cli/v2/pkg/cmd/ssh-key/add" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/ssh" @@ -72,9 +73,15 @@ func Login(opts *LoginOptions) error { var additionalScopes []string credentialFlow := &GitCredentialFlow{ - Executable: opts.Executable, - Prompter: opts.Prompter, - GitClient: opts.GitClient, + Prompter: opts.Prompter, + GitClient: opts.GitClient, + HelperConfigurer: &gitcredentials.HelperConfigurer{ + SelfExecutablePath: opts.Executable, + GitClient: opts.GitClient, + }, + Updater: &gitcredentials.Updater{ + GitClient: opts.GitClient, + }, } if opts.Interactive && gitProtocol == "https" { if err := credentialFlow.Prompt(hostname); err != nil { From af589aa2f3f495fa65d91d581dade0923636a5f8 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 12:39:11 +0200 Subject: [PATCH 02/12] Move fetching configured helper into gitcredentials --- pkg/cmd/auth/refresh/refresh.go | 2 +- pkg/cmd/auth/setupgit/setupgit.go | 2 +- pkg/cmd/auth/shared/git_credential.go | 27 +++---------------- pkg/cmd/auth/shared/git_credential_test.go | 4 +-- .../shared/gitcredentials/gitcredentials.go | 23 ++++++++++++---- pkg/cmd/auth/shared/login_flow.go | 2 +- 6 files changed, 27 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index fed0aae03..2c059a670 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -176,7 +176,7 @@ func refreshRun(opts *RefreshOptions) error { credentialFlow := &shared.GitCredentialFlow{ Prompter: opts.Prompter, GitClient: opts.GitClient, - HelperConfigurer: &gitcredentials.HelperConfigurer{ + HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: opts.MainExecutable, GitClient: opts.GitClient, }, diff --git a/pkg/cmd/auth/setupgit/setupgit.go b/pkg/cmd/auth/setupgit/setupgit.go index e134ed10b..bcfd7207c 100644 --- a/pkg/cmd/auth/setupgit/setupgit.go +++ b/pkg/cmd/auth/setupgit/setupgit.go @@ -55,7 +55,7 @@ func NewCmdSetupGit(f *cmdutil.Factory, runF func(*SetupGitOptions) error) *cobr RunE: func(cmd *cobra.Command, args []string) error { opts.gitConfigure = &shared.GitCredentialFlow{ GitClient: f.GitClient, - HelperConfigurer: &gitcredentials.HelperConfigurer{ + HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: f.Executable(), GitClient: f.GitClient, }, diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index 6d522acb6..0c1bc0656 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -1,14 +1,11 @@ package shared import ( - "context" "errors" - "fmt" "path/filepath" "strings" "github.com/cli/cli/v2/git" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" "github.com/google/shlex" ) @@ -17,8 +14,8 @@ type GitCredentialFlow struct { Prompter Prompt GitClient *git.Client - HelperConfigurer *gitcredentials.HelperConfigurer - Updater *gitcredentials.Updater + HelperConfig *gitcredentials.HelperConfig + Updater *gitcredentials.Updater shouldSetup bool helper string @@ -27,7 +24,7 @@ type GitCredentialFlow struct { func (flow *GitCredentialFlow) Prompt(hostname string) error { var gitErr error - flow.helper, gitErr = gitCredentialHelper(flow.GitClient, hostname) + flow.helper, gitErr = flow.HelperConfig.ConfiguredHelper(hostname) if isOurCredentialHelper(flow.helper) { flow.scopes = append(flow.scopes, "workflow") return nil @@ -65,7 +62,7 @@ func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password s // If there is no credential helper configured then we will set ourselves up as // the credential helper for this host. if flow.helper == "" { - return flow.HelperConfigurer.Configure(hostname) + return flow.HelperConfig.Configure(hostname) } // Otherwise, we'll tell git to inform the existing credential helper of the new credentials. @@ -92,19 +89,3 @@ func isGitMissing(err error) bool { var errNotInstalled *git.NotInstalled return errors.As(err, &errNotInstalled) } - -func gitCredentialHelperKey(hostname string) string { - host := strings.TrimSuffix(ghinstance.HostPrefix(hostname), "/") - return fmt.Sprintf("credential.%s.helper", host) -} - -func gitCredentialHelper(gitClient *git.Client, hostname string) (helper string, err error) { - ctx := context.TODO() - - helper, err = gitClient.Config(ctx, gitCredentialHelperKey(hostname)) - if helper != "" { - return - } - helper, err = gitClient.Config(ctx, "credential.helper") - return -} diff --git a/pkg/cmd/auth/shared/git_credential_test.go b/pkg/cmd/auth/shared/git_credential_test.go index 017c649dd..b2f3df7f8 100644 --- a/pkg/cmd/auth/shared/git_credential_test.go +++ b/pkg/cmd/auth/shared/git_credential_test.go @@ -66,7 +66,7 @@ func TestGitCredentialsSetup_setOurs_GH(t *testing.T) { f := GitCredentialFlow{ helper: "", GitClient: &git.Client{GitPath: "some/path/git"}, - HelperConfigurer: &gitcredentials.HelperConfigurer{ + HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: "/path/to/gh", GitClient: &git.Client{GitPath: "some/path/git"}, }, @@ -101,7 +101,7 @@ func TestGitCredentialSetup_setOurs_nonGH(t *testing.T) { f := GitCredentialFlow{ helper: "", GitClient: &git.Client{GitPath: "some/path/git"}, - HelperConfigurer: &gitcredentials.HelperConfigurer{ + HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: "/path/to/gh", GitClient: &git.Client{GitPath: "some/path/git"}, }, diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go index d4f3616e3..1f8bafb07 100644 --- a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go +++ b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go @@ -11,21 +11,21 @@ import ( "github.com/cli/cli/v2/internal/ghinstance" ) -type HelperConfigurer struct { +type HelperConfig struct { SelfExecutablePath string GitClient *git.Client } -func (hc *HelperConfigurer) Configure(hostname string) error { +func (hc *HelperConfig) Configure(hostname string) error { ctx := context.TODO() credHelperKeys := []string{ - gitCredentialHelperKey(hostname), + keyFor(hostname), } gistHost := strings.TrimSuffix(ghinstance.GistHost(hostname), "/") if strings.HasPrefix(gistHost, "gist.") { - credHelperKeys = append(credHelperKeys, gitCredentialHelperKey(gistHost)) + credHelperKeys = append(credHelperKeys, keyFor(gistHost)) } var configErr error @@ -61,7 +61,20 @@ func (hc *HelperConfigurer) Configure(hostname string) error { return configErr } -func gitCredentialHelperKey(hostname string) string { +func (hc *HelperConfig) ConfiguredHelper(hostname string) (string, error) { + ctx := context.TODO() + + helper, err := hc.GitClient.Config(ctx, keyFor(hostname)) + if helper != "" { + // TODO: This is a direct refactoring removing named and naked returns + // but we should probably look closer at the error handling here + return helper, err + } + + return hc.GitClient.Config(ctx, "credential.helper") +} + +func keyFor(hostname string) string { host := strings.TrimSuffix(ghinstance.HostPrefix(hostname), "/") return fmt.Sprintf("credential.%s.helper", host) } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index dc6ff3cad..d357df5ff 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -75,7 +75,7 @@ func Login(opts *LoginOptions) error { credentialFlow := &GitCredentialFlow{ Prompter: opts.Prompter, GitClient: opts.GitClient, - HelperConfigurer: &gitcredentials.HelperConfigurer{ + HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: opts.Executable, GitClient: opts.GitClient, }, From bd4ba5c39a06cdcd1163436bd347b9995c0c906f Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 13:09:42 +0200 Subject: [PATCH 03/12] Make gitcredential helper smarter --- pkg/cmd/auth/refresh/refresh.go | 3 +- pkg/cmd/auth/setupgit/setupgit.go | 1 - pkg/cmd/auth/shared/git_credential.go | 43 +++-------- pkg/cmd/auth/shared/git_credential_test.go | 50 +----------- .../shared/gitcredentials/gitcredentials.go | 48 ++++++++++-- .../gitcredentials/gitcredentials_test.go | 76 +++++++++++++++++++ pkg/cmd/auth/shared/login_flow.go | 3 +- 7 files changed, 131 insertions(+), 93 deletions(-) create mode 100644 pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 2c059a670..4b8a7e4c0 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -174,8 +174,7 @@ func refreshRun(opts *RefreshOptions) error { } credentialFlow := &shared.GitCredentialFlow{ - Prompter: opts.Prompter, - GitClient: opts.GitClient, + Prompter: opts.Prompter, HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: opts.MainExecutable, GitClient: opts.GitClient, diff --git a/pkg/cmd/auth/setupgit/setupgit.go b/pkg/cmd/auth/setupgit/setupgit.go index bcfd7207c..4ac8d5f78 100644 --- a/pkg/cmd/auth/setupgit/setupgit.go +++ b/pkg/cmd/auth/setupgit/setupgit.go @@ -54,7 +54,6 @@ func NewCmdSetupGit(f *cmdutil.Factory, runF func(*SetupGitOptions) error) *cobr `), RunE: func(cmd *cobra.Command, args []string) error { opts.gitConfigure = &shared.GitCredentialFlow{ - GitClient: f.GitClient, HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: f.Executable(), GitClient: f.GitClient, diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index 0c1bc0656..5a9c20b44 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -2,30 +2,26 @@ package shared import ( "errors" - "path/filepath" - "strings" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" - "github.com/google/shlex" ) type GitCredentialFlow struct { - Prompter Prompt - GitClient *git.Client + Prompter Prompt HelperConfig *gitcredentials.HelperConfig Updater *gitcredentials.Updater shouldSetup bool - helper string + helper gitcredentials.Helper scopes []string } func (flow *GitCredentialFlow) Prompt(hostname string) error { - var gitErr error - flow.helper, gitErr = flow.HelperConfig.ConfiguredHelper(hostname) - if isOurCredentialHelper(flow.helper) { + var configuredHelperErr error + flow.helper, configuredHelperErr = flow.HelperConfig.ConfiguredHelper(hostname) + if flow.helper.IsOurs() { flow.scopes = append(flow.scopes, "workflow") return nil } @@ -37,9 +33,11 @@ func (flow *GitCredentialFlow) Prompt(hostname string) error { flow.shouldSetup = result if flow.shouldSetup { - if isGitMissing(gitErr) { - return gitErr + var errNotInstalled *git.NotInstalled + if errors.As(err, &errNotInstalled) { + return configuredHelperErr } + flow.scopes = append(flow.scopes, "workflow") } @@ -61,31 +59,10 @@ func (flow *GitCredentialFlow) Setup(hostname, username, authToken string) error func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password string) error { // If there is no credential helper configured then we will set ourselves up as // the credential helper for this host. - if flow.helper == "" { + if !flow.helper.IsConfigured() { return flow.HelperConfig.Configure(hostname) } // Otherwise, we'll tell git to inform the existing credential helper of the new credentials. return flow.Updater.Update(hostname, username, password) } - -func isOurCredentialHelper(cmd string) bool { - if !strings.HasPrefix(cmd, "!") { - return false - } - - args, err := shlex.Split(cmd[1:]) - if err != nil || len(args) == 0 { - return false - } - - return strings.TrimSuffix(filepath.Base(args[0]), ".exe") == "gh" -} - -func isGitMissing(err error) bool { - if err == nil { - return false - } - var errNotInstalled *git.NotInstalled - return errors.As(err, &errNotInstalled) -} diff --git a/pkg/cmd/auth/shared/git_credential_test.go b/pkg/cmd/auth/shared/git_credential_test.go index b2f3df7f8..211e8da99 100644 --- a/pkg/cmd/auth/shared/git_credential_test.go +++ b/pkg/cmd/auth/shared/git_credential_test.go @@ -15,8 +15,7 @@ func TestGitCredentialSetup_configureExisting(t *testing.T) { cs.Register(`git credential approve`, 0, "") f := GitCredentialFlow{ - helper: "osxkeychain", - GitClient: &git.Client{GitPath: "some/path/git"}, + helper: gitcredentials.Helper{Cmd: "osxkeychain"}, Updater: &gitcredentials.Updater{ GitClient: &git.Client{GitPath: "some/path/git"}, }, @@ -64,8 +63,7 @@ func TestGitCredentialsSetup_setOurs_GH(t *testing.T) { }) f := GitCredentialFlow{ - helper: "", - GitClient: &git.Client{GitPath: "some/path/git"}, + helper: gitcredentials.Helper{}, HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: "/path/to/gh", GitClient: &git.Client{GitPath: "some/path/git"}, @@ -99,8 +97,7 @@ func TestGitCredentialSetup_setOurs_nonGH(t *testing.T) { }) f := GitCredentialFlow{ - helper: "", - GitClient: &git.Client{GitPath: "some/path/git"}, + helper: gitcredentials.Helper{}, HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: "/path/to/gh", GitClient: &git.Client{GitPath: "some/path/git"}, @@ -111,44 +108,3 @@ func TestGitCredentialSetup_setOurs_nonGH(t *testing.T) { t.Errorf("GitCredentialSetup() error = %v", err) } } - -func Test_isOurCredentialHelper(t *testing.T) { - tests := []struct { - name string - arg string - want bool - }{ - { - name: "blank", - arg: "", - want: false, - }, - { - name: "invalid", - arg: "!", - want: false, - }, - { - name: "osxkeychain", - arg: "osxkeychain", - want: false, - }, - { - name: "looks like gh but isn't", - arg: "gh auth", - want: false, - }, - { - name: "ours", - arg: "!/path/to/gh auth", - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := isOurCredentialHelper(tt.arg); got != tt.want { - t.Errorf("isOurCredentialHelper() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go index 1f8bafb07..50ce7502d 100644 --- a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go +++ b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go @@ -4,11 +4,13 @@ import ( "bytes" "context" "fmt" + "path/filepath" "strings" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghinstance" + "github.com/google/shlex" ) type HelperConfig struct { @@ -61,17 +63,47 @@ func (hc *HelperConfig) Configure(hostname string) error { return configErr } -func (hc *HelperConfig) ConfiguredHelper(hostname string) (string, error) { - ctx := context.TODO() +type Helper struct { + Cmd string +} - helper, err := hc.GitClient.Config(ctx, keyFor(hostname)) - if helper != "" { - // TODO: This is a direct refactoring removing named and naked returns - // but we should probably look closer at the error handling here - return helper, err +func (h Helper) IsConfigured() bool { + return h.Cmd != "" +} + +func (h Helper) IsOurs() bool { + if !strings.HasPrefix(h.Cmd, "!") { + return false } - return hc.GitClient.Config(ctx, "credential.helper") + args, err := shlex.Split(h.Cmd[1:]) + if err != nil || len(args) == 0 { + return false + } + + return strings.TrimSuffix(filepath.Base(args[0]), ".exe") == "gh" +} + +func (hc *HelperConfig) ConfiguredHelper(hostname string) (Helper, error) { + ctx := context.TODO() + + hostHelperCmd, err := hc.GitClient.Config(ctx, keyFor(hostname)) + if hostHelperCmd != "" { + // TODO: This is a direct refactoring removing named and naked returns + // but we should probably look closer at the error handling here + return Helper{ + Cmd: hostHelperCmd, + }, err + } + + globalHelperCmd, err := hc.GitClient.Config(ctx, "credential.helper") + if globalHelperCmd != "" { + return Helper{ + Cmd: globalHelperCmd, + }, err + } + + return Helper{}, nil } func keyFor(hostname string) string { diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go new file mode 100644 index 000000000..2a9b6e6f9 --- /dev/null +++ b/pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go @@ -0,0 +1,76 @@ +package gitcredentials_test + +import ( + "testing" + + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" +) + +func TestHelperIsOurs(t *testing.T) { + tests := []struct { + name string + cmd string + want bool + }{ + { + name: "blank", + cmd: "", + want: false, + }, + { + name: "invalid", + cmd: "!", + want: false, + }, + { + name: "osxkeychain", + cmd: "osxkeychain", + want: false, + }, + { + name: "looks like gh but isn't", + cmd: "gh auth", + want: false, + }, + { + name: "ours", + cmd: "!/path/to/gh auth", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := gitcredentials.Helper{Cmd: tt.cmd} + if got := h.IsOurs(); got != tt.want { + t.Errorf("IsOurs() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestHelperIsConfigured(t *testing.T) { + tests := []struct { + name string + cmd string + want bool + }{ + { + name: "blank is not configured", + cmd: "", + want: false, + }, + { + name: "anything else is configured", + cmd: "unimportant", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := gitcredentials.Helper{Cmd: tt.cmd} + if got := h.IsConfigured(); got != tt.want { + t.Errorf("IsConfigured() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index d357df5ff..b0feddd2f 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -73,8 +73,7 @@ func Login(opts *LoginOptions) error { var additionalScopes []string credentialFlow := &GitCredentialFlow{ - Prompter: opts.Prompter, - GitClient: opts.GitClient, + Prompter: opts.Prompter, HelperConfig: &gitcredentials.HelperConfig{ SelfExecutablePath: opts.Executable, GitClient: opts.GitClient, From 177cf7d35ba8d93ecc404d618c56bc4888045b9c Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 13:11:00 +0200 Subject: [PATCH 04/12] Rename gitcredentials Configure to ConfigureOurs --- pkg/cmd/auth/shared/git_credential.go | 2 +- pkg/cmd/auth/shared/gitcredentials/gitcredentials.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index 5a9c20b44..7ddd9aeae 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -60,7 +60,7 @@ func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password s // If there is no credential helper configured then we will set ourselves up as // the credential helper for this host. if !flow.helper.IsConfigured() { - return flow.HelperConfig.Configure(hostname) + return flow.HelperConfig.ConfigureOurs(hostname) } // Otherwise, we'll tell git to inform the existing credential helper of the new credentials. diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go index 50ce7502d..2c1e7ddcf 100644 --- a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go +++ b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go @@ -18,7 +18,7 @@ type HelperConfig struct { GitClient *git.Client } -func (hc *HelperConfig) Configure(hostname string) error { +func (hc *HelperConfig) ConfigureOurs(hostname string) error { ctx := context.TODO() credHelperKeys := []string{ From c16836bcf712ad05b537b0cd089f2f98ad12f243 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 13:33:02 +0200 Subject: [PATCH 05/12] Use tighter interface in setup-git --- pkg/cmd/auth/setupgit/setupgit.go | 31 ++++++++++---------------- pkg/cmd/auth/setupgit/setupgit_test.go | 15 +++++-------- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/pkg/cmd/auth/setupgit/setupgit.go b/pkg/cmd/auth/setupgit/setupgit.go index 4ac8d5f78..c1200b475 100644 --- a/pkg/cmd/auth/setupgit/setupgit.go +++ b/pkg/cmd/auth/setupgit/setupgit.go @@ -6,23 +6,22 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/gh" - "github.com/cli/cli/v2/pkg/cmd/auth/shared" "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/spf13/cobra" ) -type gitConfigurator interface { - Setup(hostname, username, authToken string) error +type gitCredentialsConfigurer interface { + ConfigureOurs(hostname string) error } type SetupGitOptions struct { - IO *iostreams.IOStreams - Config func() (gh.Config, error) - Hostname string - Force bool - gitConfigure gitConfigurator + IO *iostreams.IOStreams + Config func() (gh.Config, error) + Hostname string + Force bool + CredentialsHelperConfig gitCredentialsConfigurer } func NewCmdSetupGit(f *cmdutil.Factory, runF func(*SetupGitOptions) error) *cobra.Command { @@ -53,14 +52,9 @@ func NewCmdSetupGit(f *cmdutil.Factory, runF func(*SetupGitOptions) error) *cobr $ gh auth setup-git --hostname enterprise.internal `), RunE: func(cmd *cobra.Command, args []string) error { - opts.gitConfigure = &shared.GitCredentialFlow{ - HelperConfig: &gitcredentials.HelperConfig{ - SelfExecutablePath: f.Executable(), - GitClient: f.GitClient, - }, - Updater: &gitcredentials.Updater{ - GitClient: f.GitClient, - }, + opts.CredentialsHelperConfig = &gitcredentials.HelperConfig{ + SelfExecutablePath: f.Executable(), + GitClient: f.GitClient, } if opts.Hostname == "" && opts.Force { return cmdutil.FlagErrorf("`--force` must be used in conjunction with `--hostname`") @@ -98,7 +92,7 @@ func setupGitRun(opts *SetupGitOptions) error { ) } - if err := opts.gitConfigure.Setup(opts.Hostname, "", ""); err != nil { + if err := opts.CredentialsHelperConfig.ConfigureOurs(opts.Hostname); err != nil { return fmt.Errorf("failed to set up git credential helper: %s", err) } @@ -117,8 +111,7 @@ func setupGitRun(opts *SetupGitOptions) error { } for _, hostname := range hostnames { - // TODO: Use gitcredentials.HelperConfigurer here - if err := opts.gitConfigure.Setup(hostname, "", ""); err != nil { + if err := opts.CredentialsHelperConfig.ConfigureOurs(hostname); err != nil { return fmt.Errorf("failed to set up git credential helper: %s", err) } } diff --git a/pkg/cmd/auth/setupgit/setupgit_test.go b/pkg/cmd/auth/setupgit/setupgit_test.go index 9e6550e54..8d9dc2d21 100644 --- a/pkg/cmd/auth/setupgit/setupgit_test.go +++ b/pkg/cmd/auth/setupgit/setupgit_test.go @@ -15,19 +15,16 @@ import ( "github.com/stretchr/testify/require" ) -type mockGitConfigurer struct { +type gitCredentialsConfigurerSpy struct { hosts []string setupErr error } -func (gf *mockGitConfigurer) SetupFor(hostname string) []string { - return gf.hosts -} - -func (gf *mockGitConfigurer) Setup(hostname, username, authToken string) error { +func (gf *gitCredentialsConfigurerSpy) ConfigureOurs(hostname string) error { gf.hosts = append(gf.hosts, hostname) return gf.setupErr } + func TestNewCmdSetupGit(t *testing.T) { tests := []struct { name string @@ -183,8 +180,8 @@ func Test_setupGitRun(t *testing.T) { } } - gcSpy := &mockGitConfigurer{setupErr: tt.setupErr} - tt.opts.gitConfigure = gcSpy + credentialsConfigurerSpy := &gitCredentialsConfigurerSpy{setupErr: tt.setupErr} + tt.opts.CredentialsHelperConfig = credentialsConfigurerSpy err := setupGitRun(tt.opts) if tt.expectedErr != nil { @@ -194,7 +191,7 @@ func Test_setupGitRun(t *testing.T) { } if tt.expectedHostsSetup != nil { - require.Equal(t, tt.expectedHostsSetup, gcSpy.hosts) + require.Equal(t, tt.expectedHostsSetup, credentialsConfigurerSpy.hosts) } require.Equal(t, tt.expectedErrOut, stderr.String()) From 818d4ba5b843ce448158e9c755cb2c43ecbcf8cc Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 13:11:58 +0200 Subject: [PATCH 06/12] Remove unnecessary credential setup private method --- pkg/cmd/auth/shared/git_credential.go | 6 +----- pkg/cmd/auth/shared/git_credential_test.go | 16 ++++++++-------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index 7ddd9aeae..d21c60dfc 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -53,10 +53,6 @@ func (flow *GitCredentialFlow) ShouldSetup() bool { } func (flow *GitCredentialFlow) Setup(hostname, username, authToken string) error { - return flow.gitCredentialSetup(hostname, username, authToken) -} - -func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password string) error { // If there is no credential helper configured then we will set ourselves up as // the credential helper for this host. if !flow.helper.IsConfigured() { @@ -64,5 +60,5 @@ func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password s } // Otherwise, we'll tell git to inform the existing credential helper of the new credentials. - return flow.Updater.Update(hostname, username, password) + return flow.Updater.Update(hostname, username, authToken) } diff --git a/pkg/cmd/auth/shared/git_credential_test.go b/pkg/cmd/auth/shared/git_credential_test.go index 211e8da99..19ab9b752 100644 --- a/pkg/cmd/auth/shared/git_credential_test.go +++ b/pkg/cmd/auth/shared/git_credential_test.go @@ -8,7 +8,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" ) -func TestGitCredentialSetup_configureExisting(t *testing.T) { +func TestSetup_configureExisting(t *testing.T) { cs, restoreRun := run.Stub() defer restoreRun(t) cs.Register(`git credential reject`, 0, "") @@ -21,8 +21,8 @@ func TestGitCredentialSetup_configureExisting(t *testing.T) { }, } - if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { - t.Errorf("GitCredentialSetup() error = %v", err) + if err := f.Setup("example.com", "monalisa", "PASSWD"); err != nil { + t.Errorf("Setup() error = %v", err) } } @@ -70,13 +70,13 @@ func TestGitCredentialsSetup_setOurs_GH(t *testing.T) { }, } - if err := f.gitCredentialSetup("github.com", "monalisa", "PASSWD"); err != nil { - t.Errorf("GitCredentialSetup() error = %v", err) + if err := f.Setup("github.com", "monalisa", "PASSWD"); err != nil { + t.Errorf("Setup() error = %v", err) } } -func TestGitCredentialSetup_setOurs_nonGH(t *testing.T) { +func TestSetup_setOurs_nonGH(t *testing.T) { cs, restoreRun := run.Stub() defer restoreRun(t) cs.Register(`git config --global --replace-all credential\.`, 0, "", func(args []string) { @@ -104,7 +104,7 @@ func TestGitCredentialSetup_setOurs_nonGH(t *testing.T) { }, } - if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { - t.Errorf("GitCredentialSetup() error = %v", err) + if err := f.Setup("example.com", "monalisa", "PASSWD"); err != nil { + t.Errorf("Setup() error = %v", err) } } From 187c5016f0c5e508093b56d0f4da78b7a1404118 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 13:28:03 +0200 Subject: [PATCH 07/12] Comment the git credential flow --- pkg/cmd/auth/shared/git_credential.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index d21c60dfc..cba252d72 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -19,13 +19,20 @@ type GitCredentialFlow struct { } func (flow *GitCredentialFlow) Prompt(hostname string) error { + // First we'll fetch the credential helper that would be used for this host var configuredHelperErr error flow.helper, configuredHelperErr = flow.HelperConfig.ConfiguredHelper(hostname) + // If the helper is gh itself, then we don't need to ask the user if they want to update their git credentials + // because it will happen automatically by virtue of the fact that gh will return the active token. + // + // Since gh is the helper, this token may be used for git operations, so we'll additionally request the workflow + // scope to ensure that git push operations that include workflow changes succeed. if flow.helper.IsOurs() { flow.scopes = append(flow.scopes, "workflow") return nil } + // Prompt the user for whether they want to configure git with the newly obtained token result, err := flow.Prompter.Confirm("Authenticate Git with your GitHub credentials?", true) if err != nil { return err @@ -33,11 +40,24 @@ func (flow *GitCredentialFlow) Prompt(hostname string) error { flow.shouldSetup = result if flow.shouldSetup { + // If the user does want to configure git, we'll check the error returned from fetching the configured helper + // above. If the error indicates that git isn't installed, we'll return an error now to ensure that the auth + // flow is aborted before the user goes any further. + // + // Note that this is _slightly_ naive because there may be other reasons that fetching the configured helper + // fails that might cause later failures but this code has existed for a long time and I don't want to change + // it as part of a refactoring. + // + // Refs: + // * https://git-scm.com/docs/git-config#_description + // * https://github.com/cli/cli/pull/4109 var errNotInstalled *git.NotInstalled if errors.As(err, &errNotInstalled) { return configuredHelperErr } + // On the other hand, if the user has requested setup we'll additionally request the workflow + // scope to ensure that git push operations that include workflow changes succeed. flow.scopes = append(flow.scopes, "workflow") } From d75548a63016b9001215a6e223cd82a1273e26d5 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 13:48:53 +0200 Subject: [PATCH 08/12] Comment the new gitcredentials package --- pkg/cmd/auth/shared/gitcredentials/gitcredentials.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go index 2c1e7ddcf..c4626ff5c 100644 --- a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go +++ b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go @@ -13,11 +13,14 @@ import ( "github.com/google/shlex" ) +// A HelperConfig is used to configure and inspect the state of git credential helpers. type HelperConfig struct { SelfExecutablePath string GitClient *git.Client } +// ConfigureOurs sets up the git credential helper chain to use the GitHub CLI credential helper for git repositories +// including gists. func (hc *HelperConfig) ConfigureOurs(hostname string) error { ctx := context.TODO() @@ -63,14 +66,17 @@ func (hc *HelperConfig) ConfigureOurs(hostname string) error { return configErr } +// A Helper represents a git credential helper configuration. type Helper struct { Cmd string } +// IsConfigured returns true if the helper has a non-empty command, i.e. the git config had an entry func (h Helper) IsConfigured() bool { return h.Cmd != "" } +// IsOurs returns true if the helper command is the GitHub CLI credential helper func (h Helper) IsOurs() bool { if !strings.HasPrefix(h.Cmd, "!") { return false @@ -84,6 +90,7 @@ func (h Helper) IsOurs() bool { return strings.TrimSuffix(filepath.Base(args[0]), ".exe") == "gh" } +// ConfiguredHelper returns the configured git credential helper for a given hostname. func (hc *HelperConfig) ConfiguredHelper(hostname string) (Helper, error) { ctx := context.TODO() @@ -118,10 +125,13 @@ func shellQuote(s string) string { return s } +// An Updater is used to update the git credentials for a given hostname. type Updater struct { GitClient *git.Client } +// Update updates the git credentials for a given hostname, first by rejecting any existing credentials and then +// approving the new credentials. func (u *Updater) Update(hostname, username, password string) error { ctx := context.TODO() From e07a26d81cc879f27690a0a431d16833564cd1bf Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 14:33:33 +0200 Subject: [PATCH 09/12] Move gitcredentials HelperConfig and add tests --- .../shared/gitcredentials/gitcredentials.go | 117 ------------ .../gitcredentials/gitcredentials_test.go | 76 -------- .../shared/gitcredentials/helper_config.go | 124 ++++++++++++ .../gitcredentials/helper_config_test.go | 176 ++++++++++++++++++ 4 files changed, 300 insertions(+), 193 deletions(-) delete mode 100644 pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go create mode 100644 pkg/cmd/auth/shared/gitcredentials/helper_config.go create mode 100644 pkg/cmd/auth/shared/gitcredentials/helper_config_test.go diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go index c4626ff5c..9ffa443e7 100644 --- a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go +++ b/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go @@ -3,128 +3,11 @@ package gitcredentials import ( "bytes" "context" - "fmt" - "path/filepath" - "strings" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/git" - "github.com/cli/cli/v2/internal/ghinstance" - "github.com/google/shlex" ) -// A HelperConfig is used to configure and inspect the state of git credential helpers. -type HelperConfig struct { - SelfExecutablePath string - GitClient *git.Client -} - -// ConfigureOurs sets up the git credential helper chain to use the GitHub CLI credential helper for git repositories -// including gists. -func (hc *HelperConfig) ConfigureOurs(hostname string) error { - ctx := context.TODO() - - credHelperKeys := []string{ - keyFor(hostname), - } - - gistHost := strings.TrimSuffix(ghinstance.GistHost(hostname), "/") - if strings.HasPrefix(gistHost, "gist.") { - credHelperKeys = append(credHelperKeys, keyFor(gistHost)) - } - - var configErr error - - for _, credHelperKey := range credHelperKeys { - if configErr != nil { - break - } - // first use a blank value to indicate to git we want to sever the chain of credential helpers - preConfigureCmd, err := hc.GitClient.Command(ctx, "config", "--global", "--replace-all", credHelperKey, "") - if err != nil { - configErr = err - break - } - if _, err = preConfigureCmd.Output(); err != nil { - configErr = err - break - } - - // second configure the actual helper for this host - configureCmd, err := hc.GitClient.Command(ctx, - "config", "--global", "--add", - credHelperKey, - fmt.Sprintf("!%s auth git-credential", shellQuote(hc.SelfExecutablePath)), - ) - if err != nil { - configErr = err - } else { - _, configErr = configureCmd.Output() - } - } - - return configErr -} - -// A Helper represents a git credential helper configuration. -type Helper struct { - Cmd string -} - -// IsConfigured returns true if the helper has a non-empty command, i.e. the git config had an entry -func (h Helper) IsConfigured() bool { - return h.Cmd != "" -} - -// IsOurs returns true if the helper command is the GitHub CLI credential helper -func (h Helper) IsOurs() bool { - if !strings.HasPrefix(h.Cmd, "!") { - return false - } - - args, err := shlex.Split(h.Cmd[1:]) - if err != nil || len(args) == 0 { - return false - } - - return strings.TrimSuffix(filepath.Base(args[0]), ".exe") == "gh" -} - -// ConfiguredHelper returns the configured git credential helper for a given hostname. -func (hc *HelperConfig) ConfiguredHelper(hostname string) (Helper, error) { - ctx := context.TODO() - - hostHelperCmd, err := hc.GitClient.Config(ctx, keyFor(hostname)) - if hostHelperCmd != "" { - // TODO: This is a direct refactoring removing named and naked returns - // but we should probably look closer at the error handling here - return Helper{ - Cmd: hostHelperCmd, - }, err - } - - globalHelperCmd, err := hc.GitClient.Config(ctx, "credential.helper") - if globalHelperCmd != "" { - return Helper{ - Cmd: globalHelperCmd, - }, err - } - - return Helper{}, nil -} - -func keyFor(hostname string) string { - host := strings.TrimSuffix(ghinstance.HostPrefix(hostname), "/") - return fmt.Sprintf("credential.%s.helper", host) -} - -func shellQuote(s string) string { - if strings.ContainsAny(s, " $\\") { - return "'" + s + "'" - } - return s -} - // An Updater is used to update the git credentials for a given hostname. type Updater struct { GitClient *git.Client diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go b/pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go deleted file mode 100644 index 2a9b6e6f9..000000000 --- a/pkg/cmd/auth/shared/gitcredentials/gitcredentials_test.go +++ /dev/null @@ -1,76 +0,0 @@ -package gitcredentials_test - -import ( - "testing" - - "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" -) - -func TestHelperIsOurs(t *testing.T) { - tests := []struct { - name string - cmd string - want bool - }{ - { - name: "blank", - cmd: "", - want: false, - }, - { - name: "invalid", - cmd: "!", - want: false, - }, - { - name: "osxkeychain", - cmd: "osxkeychain", - want: false, - }, - { - name: "looks like gh but isn't", - cmd: "gh auth", - want: false, - }, - { - name: "ours", - cmd: "!/path/to/gh auth", - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - h := gitcredentials.Helper{Cmd: tt.cmd} - if got := h.IsOurs(); got != tt.want { - t.Errorf("IsOurs() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestHelperIsConfigured(t *testing.T) { - tests := []struct { - name string - cmd string - want bool - }{ - { - name: "blank is not configured", - cmd: "", - want: false, - }, - { - name: "anything else is configured", - cmd: "unimportant", - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - h := gitcredentials.Helper{Cmd: tt.cmd} - if got := h.IsConfigured(); got != tt.want { - t.Errorf("IsConfigured() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/cmd/auth/shared/gitcredentials/helper_config.go b/pkg/cmd/auth/shared/gitcredentials/helper_config.go new file mode 100644 index 000000000..9e9b4eaad --- /dev/null +++ b/pkg/cmd/auth/shared/gitcredentials/helper_config.go @@ -0,0 +1,124 @@ +package gitcredentials + +import ( + "context" + "fmt" + "path/filepath" + "strings" + + "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/internal/ghinstance" + "github.com/google/shlex" +) + +// A HelperConfig is used to configure and inspect the state of git credential helpers. +type HelperConfig struct { + SelfExecutablePath string + GitClient *git.Client +} + +// ConfigureOurs sets up the git credential helper chain to use the GitHub CLI credential helper for git repositories +// including gists. +func (hc *HelperConfig) ConfigureOurs(hostname string) error { + ctx := context.TODO() + + credHelperKeys := []string{ + keyFor(hostname), + } + + gistHost := strings.TrimSuffix(ghinstance.GistHost(hostname), "/") + if strings.HasPrefix(gistHost, "gist.") { + credHelperKeys = append(credHelperKeys, keyFor(gistHost)) + } + + var configErr error + + for _, credHelperKey := range credHelperKeys { + if configErr != nil { + break + } + // first use a blank value to indicate to git we want to sever the chain of credential helpers + preConfigureCmd, err := hc.GitClient.Command(ctx, "config", "--global", "--replace-all", credHelperKey, "") + if err != nil { + configErr = err + break + } + if _, err = preConfigureCmd.Output(); err != nil { + configErr = err + break + } + + // second configure the actual helper for this host + configureCmd, err := hc.GitClient.Command(ctx, + "config", "--global", "--add", + credHelperKey, + fmt.Sprintf("!%s auth git-credential", shellQuote(hc.SelfExecutablePath)), + ) + if err != nil { + configErr = err + } else { + _, configErr = configureCmd.Output() + } + } + + return configErr +} + +// A Helper represents a git credential helper configuration. +type Helper struct { + Cmd string +} + +// IsConfigured returns true if the helper has a non-empty command, i.e. the git config had an entry +func (h Helper) IsConfigured() bool { + return h.Cmd != "" +} + +// IsOurs returns true if the helper command is the GitHub CLI credential helper +func (h Helper) IsOurs() bool { + if !strings.HasPrefix(h.Cmd, "!") { + return false + } + + args, err := shlex.Split(h.Cmd[1:]) + if err != nil || len(args) == 0 { + return false + } + + return strings.TrimSuffix(filepath.Base(args[0]), ".exe") == "gh" +} + +// ConfiguredHelper returns the configured git credential helper for a given hostname. +func (hc *HelperConfig) ConfiguredHelper(hostname string) (Helper, error) { + ctx := context.TODO() + + hostHelperCmd, err := hc.GitClient.Config(ctx, keyFor(hostname)) + if hostHelperCmd != "" { + // TODO: This is a direct refactoring removing named and naked returns + // but we should probably look closer at the error handling here + return Helper{ + Cmd: hostHelperCmd, + }, err + } + + globalHelperCmd, err := hc.GitClient.Config(ctx, "credential.helper") + if globalHelperCmd != "" { + return Helper{ + Cmd: globalHelperCmd, + }, err + } + + return Helper{}, nil +} + +func keyFor(hostname string) string { + host := strings.TrimSuffix(ghinstance.HostPrefix(hostname), "/") + return fmt.Sprintf("credential.%s.helper", host) +} + +func shellQuote(s string) string { + if strings.ContainsAny(s, " $\\") { + return "'" + s + "'" + } + return s +} diff --git a/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go b/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go new file mode 100644 index 000000000..acf5612ca --- /dev/null +++ b/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go @@ -0,0 +1,176 @@ +package gitcredentials_test + +import ( + "context" + "path/filepath" + "testing" + + "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" + "github.com/stretchr/testify/require" +) + +func withIsolatedGitConfig(t *testing.T) { + t.Helper() + + // https://git-scm.com/docs/git-config#ENVIRONMENT + // Set the global git config to a temporary file + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, ".gitconfig") + t.Setenv("GIT_CONFIG_GLOBAL", configFile) + + // And disable git reading the system config + t.Setenv("GIT_CONFIG_NOSYSTEM", "true") +} + +func configureTestCredentialHelper(t *testing.T, key string) { + t.Helper() + + gc := &git.Client{} + cmd, err := gc.Command(context.Background(), "config", "--global", "--add", key, "test-helper") + require.NoError(t, err) + require.NoError(t, cmd.Run()) +} + +func TestConfigureOursNoPreexistingHelpersConfigured(t *testing.T) { + // Given there are no credential helpers configured + withIsolatedGitConfig(t) + + // When we configure ourselves as the git credential helper + hc := gitcredentials.HelperConfig{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{}, + } + require.NoError(t, hc.ConfigureOurs("github.com")) + + // Then the git config should be updated to use our credential helper for both repos and gists + repoHelper, err := hc.ConfiguredHelper("github.com") + require.NoError(t, err) + require.True(t, repoHelper.IsConfigured(), "expected our helper to be configured") + require.True(t, repoHelper.IsOurs(), "expected the helper to be ours but was %q", repoHelper.Cmd) + + gistHelper, err := hc.ConfiguredHelper("gist.github.com") + require.NoError(t, err) + require.True(t, gistHelper.IsConfigured(), "expected our helper to be configured") + require.True(t, gistHelper.IsOurs(), "expected the helper to be ours but was %q", gistHelper.Cmd) +} + +func TestConfigureOursWithPreexistingHelpersConfigured(t *testing.T) { + // Given there are other credential helpers configured + withIsolatedGitConfig(t) + configureTestCredentialHelper(t, "credential.helper") + configureTestCredentialHelper(t, "credential.https://github.com.helper") + + // When we configure ourselves as the git credential helper + hc := gitcredentials.HelperConfig{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{}, + } + require.NoError(t, hc.ConfigureOurs("github.com")) + + // Then the git config should be updated to use our credential repoHelper for both repos and gists + repoHelper, err := hc.ConfiguredHelper("github.com") + require.NoError(t, err) + require.True(t, repoHelper.IsConfigured(), "expected our helper to be configured") + require.True(t, repoHelper.IsOurs(), "expected the helper to be ours but was %q", repoHelper.Cmd) + + gistHelper, err := hc.ConfiguredHelper("gist.github.com") + require.NoError(t, err) + require.True(t, gistHelper.IsConfigured(), "expected our helper to be configured") + require.True(t, gistHelper.IsOurs(), "expected the helper to be ours but was %q", gistHelper.Cmd) +} + +func TestConfiguredHelperNoPreexistingHelpersConfigured(t *testing.T) { + // Given there are no credential helpers configured + withIsolatedGitConfig(t) + + // When we check the configured helper for a hostname + hc := gitcredentials.HelperConfig{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{}, + } + helper, err := hc.ConfiguredHelper("github.com") + + // Then the helper should not be configured and not ours + require.NoError(t, err) + require.False(t, helper.IsConfigured(), "expected no helper to be configured") + require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) +} + +func TestConfiguredHelperWithPreexistingGlobalHelperConfigured(t *testing.T) { + // Given there is a global credential helper configured + withIsolatedGitConfig(t) + configureTestCredentialHelper(t, "credential.helper") + + // When we check the configured helper for a hostname + hc := gitcredentials.HelperConfig{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{}, + } + helper, err := hc.ConfiguredHelper("github.com") + + // Then the helper should be configured, but not ours + require.NoError(t, err) + require.True(t, helper.IsConfigured(), "expected no helper to be configured") + require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) +} + +func TestConfiguredHelperWithPreexistingHostHelperConfigured(t *testing.T) { + // Given there is a credential helper configured for a hostname + withIsolatedGitConfig(t) + configureTestCredentialHelper(t, "credential.https://github.com.helper") + + // When we check the configured helper for a hostname + hc := gitcredentials.HelperConfig{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{}, + } + helper, err := hc.ConfiguredHelper("github.com") + + // Then the helper should be configured, but not ours + require.NoError(t, err) + require.True(t, helper.IsConfigured(), "expected no helper to be configured") + require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) +} + +func TestHelperIsOurs(t *testing.T) { + tests := []struct { + name string + cmd string + want bool + }{ + { + name: "blank", + cmd: "", + want: false, + }, + { + name: "invalid", + cmd: "!", + want: false, + }, + { + name: "osxkeychain", + cmd: "osxkeychain", + want: false, + }, + { + name: "looks like gh but isn't", + cmd: "gh auth", + want: false, + }, + { + name: "ours", + cmd: "!/path/to/gh auth", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := gitcredentials.Helper{Cmd: tt.cmd} + if got := h.IsOurs(); got != tt.want { + t.Errorf("IsOurs() = %v, want %v", got, tt.want) + } + }) + } +} From 37abb3ec96a48c0082d5000d580e398095a9efeb Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 14:38:51 +0200 Subject: [PATCH 10/12] Fix mistaken git installation error check --- pkg/cmd/auth/shared/git_credential.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index cba252d72..c98992a76 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -52,7 +52,7 @@ func (flow *GitCredentialFlow) Prompt(hostname string) error { // * https://git-scm.com/docs/git-config#_description // * https://github.com/cli/cli/pull/4109 var errNotInstalled *git.NotInstalled - if errors.As(err, &errNotInstalled) { + if errors.As(configuredHelperErr, &errNotInstalled) { return configuredHelperErr } From 64d50e6fbc4438c2e07b65e5509fc55d7db1f75a Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 15:37:18 +0200 Subject: [PATCH 11/12] Add tests for gitcredentials Updater --- .../{gitcredentials.go => updater.go} | 0 .../shared/gitcredentials/updater_test.go | 85 +++++++++++++++++++ 2 files changed, 85 insertions(+) rename pkg/cmd/auth/shared/gitcredentials/{gitcredentials.go => updater.go} (100%) create mode 100644 pkg/cmd/auth/shared/gitcredentials/updater_test.go diff --git a/pkg/cmd/auth/shared/gitcredentials/gitcredentials.go b/pkg/cmd/auth/shared/gitcredentials/updater.go similarity index 100% rename from pkg/cmd/auth/shared/gitcredentials/gitcredentials.go rename to pkg/cmd/auth/shared/gitcredentials/updater.go diff --git a/pkg/cmd/auth/shared/gitcredentials/updater_test.go b/pkg/cmd/auth/shared/gitcredentials/updater_test.go new file mode 100644 index 000000000..10a9c5c6c --- /dev/null +++ b/pkg/cmd/auth/shared/gitcredentials/updater_test.go @@ -0,0 +1,85 @@ +package gitcredentials_test + +import ( + "bytes" + "context" + "fmt" + "path/filepath" + "testing" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" + "github.com/stretchr/testify/require" +) + +func configureStoreCredentialHelper(t *testing.T) { + t.Helper() + tmpCredentialsFile := filepath.Join(t.TempDir(), "credentials") + + gc := &git.Client{} + // Use `--file` to store credentials in a temporary file that gets cleaned up when the test has finished running + cmd, err := gc.Command(context.Background(), "config", "--global", "--add", "credential.helper", fmt.Sprintf("store --file %s", tmpCredentialsFile)) + require.NoError(t, err) + require.NoError(t, cmd.Run()) +} + +func fillCredentials(t *testing.T) string { + gc := &git.Client{} + fillCmd, err := gc.Command(context.Background(), "credential", "fill") + require.NoError(t, err) + + fillCmd.Stdin = bytes.NewBufferString(heredoc.Docf(` + protocol=https + host=%s + `, "github.com")) + + b, err := fillCmd.Output() + require.NoError(t, err) + + return string(b) +} + +func TestUpdateAddsNewCredentials(t *testing.T) { + // Given we have an isolated git config and we're using the built in store credential helper + // https://git-scm.com/docs/git-credential-store + withIsolatedGitConfig(t) + configureStoreCredentialHelper(t) + + // When we add new credentials + u := &gitcredentials.Updater{ + GitClient: &git.Client{}, + } + require.NoError(t, u.Update("github.com", "monalisa", "password")) + + // Then our credential description is successfully filled + require.Equal(t, heredoc.Doc(` +protocol=https +host=github.com +username=monalisa +password=password +`), fillCredentials(t)) +} + +func TestUpdateReplacesOldCredentials(t *testing.T) { + // Given we have an isolated git config and we're using the built in store credential helper + // https://git-scm.com/docs/git-credential-store + // and we have existing credentials + withIsolatedGitConfig(t) + configureStoreCredentialHelper(t) + + // When we replace old credentials + u := &gitcredentials.Updater{ + GitClient: &git.Client{}, + } + require.NoError(t, u.Update("github.com", "monalisa", "old-password")) + require.NoError(t, u.Update("github.com", "monalisa", "new-password")) + + // Then our credential description is successfully filled + require.Equal(t, heredoc.Doc(` +protocol=https +host=github.com +username=monalisa +password=new-password +`), fillCredentials(t)) +} From 3803fd04b9150e40bb5a573d28f562e712cf0ca4 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 May 2024 17:01:41 +0200 Subject: [PATCH 12/12] Add Helper test for Windows --- .../gitcredentials/helper_config_test.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go b/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go index acf5612ca..b7ddbb883 100644 --- a/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go +++ b/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go @@ -3,6 +3,7 @@ package gitcredentials_test import ( "context" "path/filepath" + "runtime" "testing" "github.com/cli/cli/v2/git" @@ -135,9 +136,10 @@ func TestConfiguredHelperWithPreexistingHostHelperConfigured(t *testing.T) { func TestHelperIsOurs(t *testing.T) { tests := []struct { - name string - cmd string - want bool + name string + cmd string + want bool + windowsOnly bool }{ { name: "blank", @@ -164,9 +166,19 @@ func TestHelperIsOurs(t *testing.T) { cmd: "!/path/to/gh auth", want: true, }, + { + name: "ours - Windows edition", + cmd: `!'C:\Program Files\GitHub CLI\gh.exe' auth git-credential`, + want: true, + windowsOnly: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.windowsOnly && runtime.GOOS != "windows" { + t.Skip("skipping test on non-Windows platform") + } + h := gitcredentials.Helper{Cmd: tt.cmd} if got := h.IsOurs(); got != tt.want { t.Errorf("IsOurs() = %v, want %v", got, tt.want)