From 98f1f5ec0d570a1b89cfe249cc6fff37dc6d7ab1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 3 Mar 2021 16:20:21 +0100 Subject: [PATCH 1/2] Use absolute path when configuring gh as git credential This keeps git operations working even when PATH is modified, e.g. `brew update` will work even though Homebrew runs the command explicitly without `/usr/local/bin` in PATH. Additionally, this inserts a blank value for `credential.*.helper` to instruct git to ignore previously configured credential helpers, i.e. those that might have been set up in system configuration files. We do this because otherwise, git will store the credential obtained from gh in every other credential helper in the chain, which we want to avoid. Before: git config --global credential.https://github.com.helper '!gh auth git-credential' After: git config --global credential.https://github.com.helper '' git config --global --add credential.https://github.com.helper '!/path/to/gh auth git-credential' --- pkg/cmd/auth/login/login.go | 5 ++++ pkg/cmd/auth/refresh/refresh.go | 3 +++ pkg/cmd/auth/shared/git_credential.go | 30 +++++++++++++++++++--- pkg/cmd/auth/shared/git_credential_test.go | 29 ++++++++++++++++++--- pkg/cmd/auth/shared/login_flow.go | 3 ++- pkg/cmd/factory/default.go | 6 +++++ pkg/cmdutil/factory.go | 3 +++ 7 files changed, 70 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 64baae10f..995405021 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -23,6 +23,8 @@ type LoginOptions struct { Config func() (config.Config, error) HttpClient func() (*http.Client, error) + MainExecutable string + Interactive bool Hostname string @@ -36,6 +38,8 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, + + MainExecutable: f.Executable, } var tokenStdin bool @@ -189,6 +193,7 @@ func loginRun(opts *LoginOptions) error { Interactive: opts.Interactive, Web: opts.Web, Scopes: opts.Scopes, + Executable: opts.MainExecutable, }) } diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 32f236ca9..58267182c 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -19,6 +19,8 @@ type RefreshOptions struct { IO *iostreams.IOStreams Config func() (config.Config, error) + MainExecutable string + Hostname string Scopes []string AuthFlow func(config.Config, *iostreams.IOStreams, string, []string) error @@ -34,6 +36,7 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. _, err := authflow.AuthFlowWithConfig(cfg, io, hostname, "", scopes) return err }, + MainExecutable: f.Executable, } cmd := &cobra.Command{ diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index e67d95a6e..d95004469 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -15,6 +15,8 @@ import ( ) type GitCredentialFlow struct { + Executable string + shouldSetup bool helper string scopes []string @@ -50,13 +52,26 @@ func (flow *GitCredentialFlow) ShouldSetup() bool { } func (flow *GitCredentialFlow) Setup(hostname, username, authToken string) error { - return GitCredentialSetup(hostname, username, authToken, flow.helper) + return flow.gitCredentialSetup(hostname, username, authToken) } -func GitCredentialSetup(hostname, username, password, helper string) error { - if helper == "" { +func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password string) error { + if flow.helper == "" { + // first use a blank value to indicate to git we want to sever the chain of credential helpers + preConfigureCmd, err := git.GitCommand("config", "--global", gitCredentialHelperKey(hostname), "") + if err != nil { + return err + } + if err = run.PrepareCmd(preConfigureCmd).Run(); err != nil { + return err + } + // use GitHub CLI as a credential helper (for this host only) - configureCmd, err := git.GitCommand("config", "--global", gitCredentialHelperKey(hostname), "!gh auth git-credential") + configureCmd, err := git.GitCommand( + "config", "--global", "--add", + gitCredentialHelperKey(hostname), + fmt.Sprintf("!%s auth git-credential", shellQuote(flow.Executable)), + ) if err != nil { return err } @@ -124,3 +139,10 @@ func isOurCredentialHelper(cmd string) bool { return strings.TrimSuffix(filepath.Base(args[0]), ".exe") == "gh" } + +func shellQuote(s string) string { + if strings.ContainsAny(s, " $") { + return "'" + s + "'" + } + return s +} diff --git a/pkg/cmd/auth/shared/git_credential_test.go b/pkg/cmd/auth/shared/git_credential_test.go index debcf2d12..5a0b8b10b 100644 --- a/pkg/cmd/auth/shared/git_credential_test.go +++ b/pkg/cmd/auth/shared/git_credential_test.go @@ -12,7 +12,12 @@ func TestGitCredentialSetup_configureExisting(t *testing.T) { cs.Register(`git credential reject`, 0, "") cs.Register(`git credential approve`, 0, "") - if err := GitCredentialSetup("example.com", "monalisa", "PASSWD", "osxkeychain"); err != nil { + f := GitCredentialFlow{ + Executable: "gh", + helper: "osxkeychain", + } + + if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { t.Errorf("GitCredentialSetup() error = %v", err) } } @@ -20,13 +25,29 @@ func TestGitCredentialSetup_configureExisting(t *testing.T) { func TestGitCredentialSetup_setOurs(t *testing.T) { cs, restoreRun := run.Stub() defer restoreRun(t) - cs.Register(`git config --global credential\.https://example\.com\.helper`, 0, "", func(args []string) { - if val := args[len(args)-1]; val != "!gh auth git-credential" { + cs.Register(`git config --global credential\.`, 0, "", func(args []string) { + if key := args[len(args)-2]; key != "credential.https://example.com.helper" { + t.Errorf("git config key was %q", key) + } + if val := args[len(args)-1]; val != "" { + t.Errorf("global credential helper configured to %q", val) + } + }) + cs.Register(`git config --global --add credential\.`, 0, "", func(args []string) { + if key := args[len(args)-2]; key != "credential.https://example.com.helper" { + t.Errorf("git config key was %q", key) + } + if val := args[len(args)-1]; val != "!/path/to/gh auth git-credential" { t.Errorf("global credential helper configured to %q", val) } }) - if err := GitCredentialSetup("example.com", "monalisa", "PASSWD", ""); err != nil { + f := GitCredentialFlow{ + Executable: "/path/to/gh", + helper: "", + } + + if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { t.Errorf("GitCredentialSetup() error = %v", err) } } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index 6f75a784f..42369c466 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -28,6 +28,7 @@ type LoginOptions struct { Interactive bool Web bool Scopes []string + Executable string sshContext sshContext } @@ -56,7 +57,7 @@ func Login(opts *LoginOptions) error { var additionalScopes []string - credentialFlow := &GitCredentialFlow{} + credentialFlow := &GitCredentialFlow{Executable: opts.Executable} if opts.Interactive && gitProtocol == "https" { if err := credentialFlow.Prompt(hostname); err != nil { return err diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 2c70ab5cd..ff8ca8ac9 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -44,6 +44,11 @@ func New(appVersion string) *cmdutil.Factory { } remotesFunc := rr.Resolver(hostOverride) + ghExecutable := "gh" + if exe, err := os.Executable(); err == nil { + ghExecutable = exe + } + return &cmdutil.Factory{ IOStreams: io, Config: configFunc, @@ -70,5 +75,6 @@ func New(appVersion string) *cmdutil.Factory { } return currentBranch, nil }, + Executable: ghExecutable, } } diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 62ed5d802..eb9546b52 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -16,4 +16,7 @@ type Factory struct { Remotes func() (context.Remotes, error) Config func() (config.Config, error) Branch func() (string, error) + + // Executable is the path to the currently invoked gh binary + Executable string } From cfbfb578f07e329623d1bd5c0010a89bbf613e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 4 Mar 2021 13:41:50 +0100 Subject: [PATCH 2/2] Read `Executable` from factory instead of from stdlib --- cmd/gh/main.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index e20452808..0934863fc 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -163,10 +163,7 @@ func main() { newRelease := <-updateMessageChan if newRelease != nil { - isHomebrew := false - if ghExe, err := os.Executable(); err == nil { - isHomebrew = isUnderHomebrew(ghExe) - } + isHomebrew := isUnderHomebrew(cmdFactory.Executable) if isHomebrew && isRecentRelease(newRelease.PublishedAt) { // do not notify Homebrew users before the version bump had a chance to get merged into homebrew-core return