From 103e18cab548c7a9116a0bfe139f1285f0edfa49 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 1 Jul 2021 13:45:29 -0700 Subject: [PATCH 1/4] Disallow installing extensions with same name as gh command --- pkg/cmd/extensions/command.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/cmd/extensions/command.go b/pkg/cmd/extensions/command.go index f764e22e5..7abba2586 100644 --- a/pkg/cmd/extensions/command.go +++ b/pkg/cmd/extensions/command.go @@ -80,6 +80,17 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { if !strings.HasPrefix(repo.RepoName(), "gh-") { return errors.New("the repository name must start with `gh-`") } + + commandName := strings.TrimPrefix(repo.RepoName(), "gh-") + rootCmd := cmd.Root() + c, _, err := rootCmd.Traverse([]string{commandName}) + if err != nil { + return err + } + if c != rootCmd { + return fmt.Errorf("could not install %s: %s is already a gh command", args[0], commandName) + } + cfg, err := f.Config() if err != nil { return err From 0482e5cd9b6c30bdf3f6e7cfe365dcb8233383ad Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 1 Jul 2021 13:51:13 -0700 Subject: [PATCH 2/4] Disallow installing extensions with the same name --- pkg/cmd/extensions/command.go | 7 ++++- pkg/cmd/extensions/command_test.go | 43 +++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/extensions/command.go b/pkg/cmd/extensions/command.go index 7abba2586..d06d9dba8 100644 --- a/pkg/cmd/extensions/command.go +++ b/pkg/cmd/extensions/command.go @@ -88,7 +88,12 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { return err } if c != rootCmd { - return fmt.Errorf("could not install %s: %s is already a gh command", args[0], commandName) + return fmt.Errorf("could not install %s: %s is already a command", args[0], commandName) + } + for _, ext := range m.List() { + if ext.Name() == commandName { + return fmt.Errorf("could not install %s: %s is already an installed extension", args[0], commandName) + } } cfg, err := f.Config() diff --git a/pkg/cmd/extensions/command_test.go b/pkg/cmd/extensions/command_test.go index 7bf0b4429..ebc6959fa 100644 --- a/pkg/cmd/extensions/command_test.go +++ b/pkg/cmd/extensions/command_test.go @@ -2,7 +2,6 @@ package extensions import ( "io" - "io/ioutil" "os" "strings" "testing" @@ -25,23 +24,43 @@ func TestNewCmdExtensions(t *testing.T) { args []string managerStubs func(em *extensions.ExtensionManagerMock) func(*testing.T) wantErr bool - wantStdout string - wantStderr string + errMsg string }{ { name: "install an extension", args: []string{"install", "owner/gh-some-ext"}, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.ListFunc = func() []extensions.Extension { + return []extensions.Extension{} + } em.InstallFunc = func(s string, out, errOut io.Writer) error { return nil } return func(t *testing.T) { - calls := em.InstallCalls() - assert.Equal(t, 1, len(calls)) - assert.Equal(t, "https://github.com/owner/gh-some-ext.git", calls[0].URL) + installCalls := em.InstallCalls() + assert.Equal(t, 1, len(installCalls)) + assert.Equal(t, "https://github.com/owner/gh-some-ext.git", installCalls[0].URL) + listCalls := em.ListCalls() + assert.Equal(t, 1, len(listCalls)) } }, }, + { + name: "install an extension with same name as existing extension", + args: []string{"install", "owner/gh-existing-ext"}, + managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.ListFunc = func() []extensions.Extension { + e := &Extension{path: "owner2/gh-existing-ext"} + return []extensions.Extension{e} + } + return func(t *testing.T) { + calls := em.ListCalls() + assert.Equal(t, 1, len(calls)) + } + }, + wantErr: true, + errMsg: "could not install owner/gh-existing-ext: existing-ext is already an installed extension", + }, { name: "install local extension", args: []string{"install", "."}, @@ -60,6 +79,7 @@ func TestNewCmdExtensions(t *testing.T) { name: "upgrade error", args: []string{"upgrade"}, wantErr: true, + errMsg: "must specify an extension to upgrade", }, { name: "upgrade an extension", @@ -107,7 +127,7 @@ func TestNewCmdExtensions(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ios, _, stdout, stderr := iostreams.Test() + ios, _, _, _ := iostreams.Test() var assertFunc func(*testing.T) em := &extensions.ExtensionManagerMock{} @@ -125,12 +145,12 @@ func TestNewCmdExtensions(t *testing.T) { cmd := NewCmdExtensions(&f) cmd.SetArgs(tt.args) - cmd.SetOut(ioutil.Discard) - cmd.SetErr(ioutil.Discard) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) _, err := cmd.ExecuteC() if tt.wantErr { - assert.Error(t, err) + assert.EqualError(t, err, tt.errMsg) } else { assert.NoError(t, err) } @@ -138,9 +158,6 @@ func TestNewCmdExtensions(t *testing.T) { if assertFunc != nil { assertFunc(t) } - - assert.Equal(t, tt.wantStdout, stdout.String()) - assert.Equal(t, tt.wantStderr, stderr.String()) }) } } From 16d218508d8afb7035fcd0978f8c6b34d3437a68 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Fri, 2 Jul 2021 13:53:26 -0700 Subject: [PATCH 3/4] Fix tests --- pkg/cmd/extensions/command_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/extensions/command_test.go b/pkg/cmd/extensions/command_test.go index ebc6959fa..9e7243ef1 100644 --- a/pkg/cmd/extensions/command_test.go +++ b/pkg/cmd/extensions/command_test.go @@ -2,6 +2,7 @@ package extensions import ( "io" + "io/ioutil" "os" "strings" "testing" @@ -145,8 +146,8 @@ func TestNewCmdExtensions(t *testing.T) { cmd := NewCmdExtensions(&f) cmd.SetArgs(tt.args) - cmd.SetOut(io.Discard) - cmd.SetErr(io.Discard) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) _, err := cmd.ExecuteC() if tt.wantErr { From d6b0749ea2f55ab1e00883ae98275d6316194894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 16 Jul 2021 14:52:05 +0200 Subject: [PATCH 4/4] Tweak error messages and add more tests for extension install name check --- pkg/cmd/extensions/command.go | 40 +++++++++------ pkg/cmd/extensions/command_test.go | 79 +++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 17 deletions(-) diff --git a/pkg/cmd/extensions/command.go b/pkg/cmd/extensions/command.go index d06d9dba8..2bdf75b41 100644 --- a/pkg/cmd/extensions/command.go +++ b/pkg/cmd/extensions/command.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/extensions" "github.com/cli/cli/utils" "github.com/spf13/cobra" ) @@ -73,28 +74,14 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { } return m.InstallLocal(wd) } + repo, err := ghrepo.FromFullName(args[0]) if err != nil { return err } - if !strings.HasPrefix(repo.RepoName(), "gh-") { - return errors.New("the repository name must start with `gh-`") - } - - commandName := strings.TrimPrefix(repo.RepoName(), "gh-") - rootCmd := cmd.Root() - c, _, err := rootCmd.Traverse([]string{commandName}) - if err != nil { + if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { return err } - if c != rootCmd { - return fmt.Errorf("could not install %s: %s is already a command", args[0], commandName) - } - for _, ext := range m.List() { - if ext.Name() == commandName { - return fmt.Errorf("could not install %s: %s is already an installed extension", args[0], commandName) - } - } cfg, err := f.Config() if err != nil { @@ -145,3 +132,24 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { extCmd.Hidden = true return &extCmd } + +func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, extName string) error { + if !strings.HasPrefix(extName, "gh-") { + return errors.New("extension repository name must start with `gh-`") + } + + commandName := strings.TrimPrefix(extName, "gh-") + if c, _, err := rootCmd.Traverse([]string{commandName}); err != nil { + return err + } else if c != rootCmd { + return fmt.Errorf("%q matches the name of a built-in command", commandName) + } + + for _, ext := range m.List() { + if ext.Name() == commandName { + return fmt.Errorf("there is already an installed extension that provides the %q command", commandName) + } + } + + return nil +} diff --git a/pkg/cmd/extensions/command_test.go b/pkg/cmd/extensions/command_test.go index 9e7243ef1..41eae92b2 100644 --- a/pkg/cmd/extensions/command_test.go +++ b/pkg/cmd/extensions/command_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/extensions" "github.com/cli/cli/pkg/iostreams" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" ) @@ -60,7 +61,7 @@ func TestNewCmdExtensions(t *testing.T) { } }, wantErr: true, - errMsg: "could not install owner/gh-existing-ext: existing-ext is already an installed extension", + errMsg: "there is already an installed extension that provides the \"existing-ext\" command", }, { name: "install local extension", @@ -166,3 +167,79 @@ func TestNewCmdExtensions(t *testing.T) { func normalizeDir(d string) string { return strings.TrimPrefix(d, "/private") } + +func Test_checkValidExtension(t *testing.T) { + rootCmd := &cobra.Command{} + rootCmd.AddCommand(&cobra.Command{Use: "help"}) + rootCmd.AddCommand(&cobra.Command{Use: "auth"}) + + m := &extensions.ExtensionManagerMock{ + ListFunc: func() []extensions.Extension { + return []extensions.Extension{ + &extensions.ExtensionMock{ + NameFunc: func() string { return "screensaver" }, + }, + &extensions.ExtensionMock{ + NameFunc: func() string { return "triage" }, + }, + } + }, + } + + type args struct { + rootCmd *cobra.Command + manager extensions.ExtensionManager + extName string + } + tests := []struct { + name string + args args + wantError string + }{ + { + name: "valid extension", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gh-hello", + }, + }, + { + name: "invalid extension name", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gherkins", + }, + wantError: "extension repository name must start with `gh-`", + }, + { + name: "clashes with built-in command", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gh-auth", + }, + wantError: "\"auth\" matches the name of a built-in command", + }, + { + name: "clashes with an installed extension", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gh-triage", + }, + wantError: "there is already an installed extension that provides the \"triage\" command", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkValidExtension(tt.args.rootCmd, tt.args.manager, tt.args.extName) + if tt.wantError == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tt.wantError) + } + }) + } +}