From a63c444cfd8dcc5f67abc4e01410a310cc278d9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Sat, 27 Jul 2019 16:08:11 +0200 Subject: [PATCH 1/8] refactor(plugins): Improved and simplified verifier and plugin list * Added proper executable check for current user * Refactor plugin_list to make it more straightforward * Seperate errors and warnings * Don't return an error when no plugin is installed and `plugin list` is called * Simplified tests * Check assumption that a prefix is given in verify() --- docs/cmd/kn_plugin.md | 2 +- docs/cmd/kn_plugin_list.md | 13 +- go.mod | 2 +- pkg/kn/commands/plugin/path_verifier.go | 102 ------- pkg/kn/commands/plugin/plugin_handler.go | 2 +- pkg/kn/commands/plugin/plugin_list.go | 194 +++++-------- .../{plugin_flags.go => plugin_list_flags.go} | 18 +- ...lags_test.go => plugin_list_flags_test.go} | 24 +- pkg/kn/commands/plugin/plugin_list_test.go | 264 ++++++++---------- pkg/kn/commands/plugin/plugin_test_helper.go | 2 +- pkg/kn/commands/plugin/plugin_verifier.go | 219 +++++++++++++++ ...rifier_test.go => plugin_verifier_test.go} | 57 ++-- 12 files changed, 471 insertions(+), 428 deletions(-) delete mode 100644 pkg/kn/commands/plugin/path_verifier.go rename pkg/kn/commands/plugin/{plugin_flags.go => plugin_list_flags.go} (69%) rename pkg/kn/commands/plugin/{plugin_flags_test.go => plugin_list_flags_test.go} (73%) create mode 100644 pkg/kn/commands/plugin/plugin_verifier.go rename pkg/kn/commands/plugin/{path_verifier_test.go => plugin_verifier_test.go} (63%) diff --git a/docs/cmd/kn_plugin.md b/docs/cmd/kn_plugin.md index 335a038b12..5d17d2657a 100644 --- a/docs/cmd/kn_plugin.md +++ b/docs/cmd/kn_plugin.md @@ -32,5 +32,5 @@ kn plugin [flags] ### SEE ALSO * [kn](kn.md) - Knative client -* [kn plugin list](kn_plugin_list.md) - List all visible plugin executables +* [kn plugin list](kn_plugin_list.md) - List plugins diff --git a/docs/cmd/kn_plugin_list.md b/docs/cmd/kn_plugin_list.md index cff91c1a69..4fe71426a6 100644 --- a/docs/cmd/kn_plugin_list.md +++ b/docs/cmd/kn_plugin_list.md @@ -1,16 +1,16 @@ ## kn plugin list -List all visible plugin executables +List plugins ### Synopsis -List all visible plugin executables. +List all installed plugins. -Available plugin files are those that are: +Available plugins are those that are: - executable -- begin with "kn- -- anywhere on the path specified in Kn's config pluginDir variable, which: - * can be overridden with the --plugin-dir flag +- begin with "kn-" +- Kn's plugin directory ~/.kn/plugins +- Anywhere in the execution $PATH (if lookupInPath config variable is enabled) ``` kn plugin list [flags] @@ -23,6 +23,7 @@ kn plugin list [flags] --lookup-plugins-in-path look for kn plugins in $PATH --name-only If true, display only the binary name of each plugin, rather than its full path --plugins-dir string kn plugins directory (default "~/.kn/plugins") + --verbose verbose output ``` ### Options inherited from parent commands diff --git a/go.mod b/go.mod index f3e4e36950..8a1904faf9 100644 --- a/go.mod +++ b/go.mod @@ -25,7 +25,7 @@ require ( github.com/mitchellh/go-homedir v1.1.0 github.com/modern-go/reflect2 v1.0.1 // indirect github.com/peterbourgon/diskv v2.0.1+incompatible // indirect - github.com/pkg/errors v0.8.1 // indirect + github.com/pkg/errors v0.8.1 github.com/spf13/cobra v0.0.3 github.com/spf13/pflag v1.0.3 github.com/spf13/viper v1.3.1 diff --git a/pkg/kn/commands/plugin/path_verifier.go b/pkg/kn/commands/plugin/path_verifier.go deleted file mode 100644 index 5ef806bf97..0000000000 --- a/pkg/kn/commands/plugin/path_verifier.go +++ /dev/null @@ -1,102 +0,0 @@ -// Copyright © 2018 The Knative Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package plugin - -import ( - "fmt" - "os" - "path/filepath" - "runtime" - "strings" - - "github.com/spf13/cobra" -) - -// PathVerifier receives a path and determines if it is valid or not -type PathVerifier interface { - // Verify determines if a given path is valid - Verify(path string) []error -} - -// CommandOverrideVerifier verifies that existing kn commands are not overriden -type CommandOverrideVerifier struct { - Root *cobra.Command - SeenPlugins map[string]string -} - -// Verify implements PathVerifier and determines if a given path -// is valid depending on whether or not it overwrites an existing -// kn command path, or a previously seen plugin. -func (v *CommandOverrideVerifier) Verify(path string) []error { - if v.Root == nil { - return []error{fmt.Errorf("unable to verify path with nil root")} - } - - // extract the plugin binary name - segs := strings.Split(path, string(os.PathSeparator)) - binName := segs[len(segs)-1] - - cmdPath := strings.Split(binName, "-") - if len(cmdPath) > 1 { - // the first argument is always "kn" for a plugin binary - cmdPath = cmdPath[1:] - } - - errors := []error{} - isExec, err := isExecutable(path) - if err == nil && !isExec { - errors = append(errors, fmt.Errorf("warning: %s identified as a kn plugin, but it is not executable", path)) - } else if err != nil { - errors = append(errors, fmt.Errorf("error: unable to identify %s as an executable file: %v", path, err)) - } - - if existingPath, ok := v.SeenPlugins[binName]; ok { - errors = append(errors, fmt.Errorf("warning: %s is overshadowed by a similarly named plugin: %s", path, existingPath)) - } else { - v.SeenPlugins[binName] = path - } - - cmd, _, err := v.Root.Find(cmdPath) - if err == nil { - errors = append(errors, fmt.Errorf("warning: %s overwrites existing command: %q", binName, cmd.CommandPath())) - } - - return errors -} - -// Private functions - -func isExecutable(fullPath string) (bool, error) { - info, err := os.Stat(fullPath) - if err != nil { - return false, err - } - - if runtime.GOOS == "windows" { - fileExt := strings.ToLower(filepath.Ext(fullPath)) - - switch fileExt { - case ".bat", ".cmd", ".com", ".exe", ".ps1": - return true, nil - } - return false, nil - } - - if m := info.Mode(); !m.IsDir() && m&0111 != 0 { - return true, nil - } - - return false, nil -} diff --git a/pkg/kn/commands/plugin/plugin_handler.go b/pkg/kn/commands/plugin/plugin_handler.go index 28bedee678..367121305c 100644 --- a/pkg/kn/commands/plugin/plugin_handler.go +++ b/pkg/kn/commands/plugin/plugin_handler.go @@ -62,7 +62,7 @@ func (h *DefaultPluginHandler) Lookup(name string) (string, bool) { pluginPath := fmt.Sprintf("%s-%s", prefix, name) // Try to find plugin in pluginsDir - pluginDir, err := ExpandPath(h.PluginsDir) + pluginDir, err := expandPath(h.PluginsDir) if err != nil { return "", false } diff --git a/pkg/kn/commands/plugin/plugin_list.go b/pkg/kn/commands/plugin/plugin_list.go index b8823c9d66..6c550eb769 100644 --- a/pkg/kn/commands/plugin/plugin_list.go +++ b/pkg/kn/commands/plugin/plugin_list.go @@ -15,18 +15,17 @@ package plugin import ( - "bytes" "fmt" "io/ioutil" "os" "path/filepath" "strings" - "github.com/knative/client/pkg/kn/commands" "github.com/spf13/cobra" - "k8s.io/cli-runtime/pkg/genericclioptions" - homedir "github.com/mitchellh/go-homedir" + "github.com/knative/client/pkg/kn/commands" + + "github.com/mitchellh/go-homedir" ) // ValidPluginFilenamePrefixes controls the prefix for all kn plugins @@ -34,103 +33,88 @@ var ValidPluginFilenamePrefixes = []string{"kn"} // NewPluginListCommand creates a new `kn plugin list` command func NewPluginListCommand(p *commands.KnParams) *cobra.Command { - pluginFlags := PluginFlags{ - IOStreams: genericclioptions.IOStreams{ - In: os.Stdin, - Out: os.Stdout, - ErrOut: os.Stderr, - }, - } + plFlags := pluginListFlags{} pluginListCommand := &cobra.Command{ Use: "list", - Short: "List all visible plugin executables", - Long: `List all visible plugin executables. + Short: "List plugins", + Long: `List all installed plugins. -Available plugin files are those that are: +Available plugins are those that are: - executable -- begin with "kn- -- anywhere on the path specified in Kn's config pluginDir variable, which: - * can be overridden with the --plugin-dir flag`, +- begin with "kn-" +- Kn's plugin directory ~/.kn/plugins +- Anywhere in the execution $PATH (if lookupInPath config variable is enabled)`, RunE: func(cmd *cobra.Command, args []string) error { - err := pluginFlags.complete(cmd) - if err != nil { - return err - } - - err = pluginFlags.run() - if err != nil { - return err - } - - return nil + return listPlugins(cmd, plFlags) }, } AddPluginFlags(pluginListCommand) BindPluginsFlagToViper(pluginListCommand) - - pluginFlags.AddPluginFlags(pluginListCommand) + plFlags.AddPluginListFlags(pluginListCommand) return pluginListCommand } -// ExpandPath to a canonical path (need to see if Golang has a better option) -func ExpandPath(path string) (string, error) { - if strings.Contains(path, "~") { - var err error - path, err = expandHomeDir(path) - if err != nil { - return "", err - } - } - return path, nil -} - -// Private - -func (o *PluginFlags) complete(cmd *cobra.Command) error { - o.Verifier = &CommandOverrideVerifier{ - Root: cmd.Root(), - SeenPlugins: make(map[string]string, 0), - } +// List plugins by looking up in plugin directory and path +func listPlugins(cmd *cobra.Command, flags pluginListFlags) error { - pluginPath, err := ExpandPath(commands.Cfg.PluginsDir) + pluginPath, err := expandPath(commands.Cfg.PluginsDir) if err != nil { return err } - if commands.Cfg.LookupPluginsInPath { pluginPath = pluginPath + string(os.PathListSeparator) + os.Getenv("PATH") } - o.PluginPaths = filepath.SplitList(pluginPath) + pluginsFound, eaw := lookupPlugins(pluginPath) + + out := cmd.OutOrStdout() + + if len(pluginsFound) == 0 { + fmt.Fprintf(out, "No plugins found in path %s\n", pluginPath) + return nil + } + + if flags.verbose { + fmt.Fprintf(out, "The following plugins are available, using options:\n") + fmt.Fprintf(out, " - plugins dir: '%s'\n", commands.Cfg.PluginsDir) + fmt.Fprintf(out, " - lookup plugins in path: '%t'\n", commands.Cfg.LookupPluginsInPath) + } - return nil + verifier := newPluginVerifier(cmd.Root()) + for _, plugin := range pluginsFound { + name := plugin + if flags.nameOnly { + name = filepath.Base(plugin) + } + fmt.Fprintf(out, "%s\n", name) + eaw = verifier.verify(eaw, plugin) + } + eaw.printWarningsAndErrors(out) + return eaw.combinedError() } -func (o *PluginFlags) run() error { - pluginsFound := false - isFirstFile := true - pluginErrors := []error{} - pluginWarnings := 0 +func lookupPlugins(pluginPath string) ([]string, errorsAndWarnings) { + pluginsFound := make([]string, 0) + eaw := errorsAndWarnings{} + + for _, dir := range uniquePathsList(filepath.SplitList(pluginPath)) { - for _, dir := range uniquePathsList(o.PluginPaths) { - if dir == "" { + files, err := ioutil.ReadDir(dir) + + // Ignore non-existing directories + if os.IsNotExist(err) { continue } - files, err := ioutil.ReadDir(dir) if err != nil { - if _, ok := err.(*os.PathError); ok { - fmt.Fprintf(o.ErrOut, "Unable read directory '%s' from your plugins path: %v. Skipping...", dir, err) - continue - } - - pluginErrors = append(pluginErrors, fmt.Errorf("error: unable to read directory '%s' from your plugin path: %v", dir, err)) + eaw.addError("unable to read directory '%s' from your plugin path: %v", dir, err) continue } + // Check for plugins within given directory for _, f := range files { if f.IsDir() { continue @@ -138,55 +122,25 @@ func (o *PluginFlags) run() error { if !hasValidPrefix(f.Name(), ValidPluginFilenamePrefixes) { continue } - - if isFirstFile { - fmt.Fprintf(o.ErrOut, "The following compatible plugins are available, using options:\n") - fmt.Fprintf(o.ErrOut, " - plugins dir: '%s'\n", commands.Cfg.PluginsDir) - fmt.Fprintf(o.ErrOut, " - lookup plugins in path: '%t'\n\n", commands.Cfg.LookupPluginsInPath) - pluginsFound = true - isFirstFile = false - } - - pluginPath := f.Name() - if !o.NameOnly { - pluginPath = filepath.Join(dir, pluginPath) - } - - fmt.Fprintf(o.Out, "%s\n", pluginPath) - if errs := o.Verifier.Verify(filepath.Join(dir, f.Name())); len(errs) != 0 { - for _, err := range errs { - fmt.Fprintf(o.ErrOut, " - %s\n", err) - pluginWarnings++ - } - } + pluginsFound = append(pluginsFound, filepath.Join(dir, f.Name())) } } + return pluginsFound, eaw +} - if !pluginsFound { - pluginErrors = append(pluginErrors, fmt.Errorf("warning: unable to find any kn plugins in your plugin path: '%s'", o.PluginPaths)) - } - - if pluginWarnings > 0 { - if pluginWarnings == 1 { - pluginErrors = append(pluginErrors, fmt.Errorf("error: one plugin warning was found")) - } else { - pluginErrors = append(pluginErrors, fmt.Errorf("error: %v plugin warnings were found", pluginWarnings)) - } - } - if len(pluginErrors) > 0 { - fmt.Fprintln(o.ErrOut) - errs := bytes.NewBuffer(nil) - for _, e := range pluginErrors { - fmt.Fprintln(errs, e) +// Private +// expandPath to a canonical path (need to see if Golang has a better option) +func expandPath(path string) (string, error) { + if strings.Contains(path, "~") { + var err error + path, err = expandHomeDir(path) + if err != nil { + return "", err } - return fmt.Errorf("%s", errs.String()) } - - return nil + return path, nil } -// Private - // expandHomeDir replaces the ~ with the home directory value func expandHomeDir(path string) (string, error) { home, err := homedir.Dir() @@ -198,11 +152,21 @@ func expandHomeDir(path string) (string, error) { return strings.Replace(path, "~", home, -1), nil } +func hasValidPrefix(filepath string, validPrefixes []string) bool { + for _, prefix := range validPrefixes { + if !strings.HasPrefix(filepath, prefix+"-") { + continue + } + return true + } + return false +} + // uniquePathsList deduplicates a given slice of strings without // sorting or otherwise altering its order in any way. func uniquePathsList(paths []string) []string { seen := map[string]bool{} - newPaths := []string{} + var newPaths []string for _, p := range paths { if seen[p] { continue @@ -212,13 +176,3 @@ func uniquePathsList(paths []string) []string { } return newPaths } - -func hasValidPrefix(filepath string, validPrefixes []string) bool { - for _, prefix := range validPrefixes { - if !strings.HasPrefix(filepath, prefix+"-") { - continue - } - return true - } - return false -} diff --git a/pkg/kn/commands/plugin/plugin_flags.go b/pkg/kn/commands/plugin/plugin_list_flags.go similarity index 69% rename from pkg/kn/commands/plugin/plugin_flags.go rename to pkg/kn/commands/plugin/plugin_list_flags.go index b15852084f..8e219b1e04 100644 --- a/pkg/kn/commands/plugin/plugin_flags.go +++ b/pkg/kn/commands/plugin/plugin_list_flags.go @@ -16,20 +16,16 @@ package plugin import ( "github.com/spf13/cobra" - "k8s.io/cli-runtime/pkg/genericclioptions" ) -// PluginFlags contains all PLugin commands flags -type PluginFlags struct { - NameOnly bool - - Verifier PathVerifier - PluginPaths []string - - genericclioptions.IOStreams +// pluginListFlags contains all plugin commands flags +type pluginListFlags struct { + nameOnly bool + verbose bool } // AddPluginFlags adds the various flags to plugin command -func (p *PluginFlags) AddPluginFlags(command *cobra.Command) { - command.Flags().BoolVar(&p.NameOnly, "name-only", false, "If true, display only the binary name of each plugin, rather than its full path") +func (p *pluginListFlags) AddPluginListFlags(command *cobra.Command) { + command.Flags().BoolVar(&p.nameOnly, "name-only", false, "If true, display only the binary name of each plugin, rather than its full path") + command.Flags().BoolVar(&p.verbose, "verbose", false, "verbose output") } diff --git a/pkg/kn/commands/plugin/plugin_flags_test.go b/pkg/kn/commands/plugin/plugin_list_flags_test.go similarity index 73% rename from pkg/kn/commands/plugin/plugin_flags_test.go rename to pkg/kn/commands/plugin/plugin_list_flags_test.go index c4ecc9ee0c..9684b4b000 100644 --- a/pkg/kn/commands/plugin/plugin_flags_test.go +++ b/pkg/kn/commands/plugin/plugin_list_flags_test.go @@ -21,27 +21,27 @@ import ( "gotest.tools/assert" ) -func TestAddPluginFlags(t *testing.T) { +func TestAddPluginListFlags(t *testing.T) { var ( - pluginFlags *PluginFlags - cmd *cobra.Command + pFlags *pluginListFlags + cmd *cobra.Command ) - setup := func() { - pluginFlags = &PluginFlags{} + t.Run("adds plugin flag", func(t *testing.T) { + pFlags = &pluginListFlags{} cmd = &cobra.Command{} - } - - t.Run("adds plugin flag", func(t *testing.T) { - setup() - pluginFlags.AddPluginFlags(cmd) + pFlags.AddPluginListFlags(cmd) - assert.Assert(t, pluginFlags != nil) + assert.Assert(t, pFlags != nil) assert.Assert(t, cmd.Flags() != nil) nameOnly, err := cmd.Flags().GetBool("name-only") - assert.Assert(t, err == nil) + assert.NilError(t, err) assert.Assert(t, nameOnly == false) + + verbose, err := cmd.Flags().GetBool("verbose") + assert.NilError(t, err) + assert.Assert(t, verbose == false) }) } diff --git a/pkg/kn/commands/plugin/plugin_list_test.go b/pkg/kn/commands/plugin/plugin_list_test.go index 5c16dad3d0..adaa158ad0 100644 --- a/pkg/kn/commands/plugin/plugin_list_test.go +++ b/pkg/kn/commands/plugin/plugin_list_test.go @@ -15,6 +15,7 @@ package plugin import ( + "bytes" "fmt" "io/ioutil" "os" @@ -23,182 +24,170 @@ import ( "testing" "github.com/knative/client/pkg/kn/commands" + "github.com/knative/client/pkg/util" + "github.com/spf13/cobra" "gotest.tools/assert" ) -func TestPluginList(t *testing.T) { - var ( - rootCmd, pluginCmd, pluginListCmd *cobra.Command - tmpPathDir, pluginsDir, pluginsDirFlag string - err error - ) - - setup := func(t *testing.T) { - knParams := &commands.KnParams{} - pluginCmd = NewPluginCommand(knParams) - assert.Assert(t, pluginCmd != nil) +type testContext struct { + pluginsDir string + pathDir string + rootCmd *cobra.Command + out *bytes.Buffer + origPath string +} - rootCmd, _, _ = commands.CreateTestKnCommand(pluginCmd, knParams) - assert.Assert(t, rootCmd != nil) +func (ctx *testContext) execute(args ...string) error { + ctx.rootCmd.SetArgs(append(args, fmt.Sprintf("--plugins-dir=%s", ctx.pluginsDir))) + return ctx.rootCmd.Execute() +} - pluginListCmd = FindSubCommand(t, pluginCmd, "list") - assert.Assert(t, pluginListCmd != nil) +func (ctx *testContext) output() string { + return ctx.out.String() +} - tmpPathDir, err = ioutil.TempDir("", "plugin_list") - assert.Assert(t, err == nil) +func (ctx *testContext) cleanup() { + os.RemoveAll(ctx.pluginsDir) + os.RemoveAll(ctx.pathDir) + os.Setenv("PATH", ctx.origPath) +} - pluginsDir = filepath.Join(tmpPathDir, "plugins") - pluginsDirFlag = fmt.Sprintf("--plugins-dir=%s", pluginsDir) +func (ctx *testContext) createTestPlugin(pluginName string, fileMode os.FileMode, inPath bool) error { + path := ctx.pluginsDir + if inPath { + path = ctx.pathDir } + return ctx.createTestPluginWithPath(pluginName, fileMode, path) +} - cleanup := func(t *testing.T) { - err = os.RemoveAll(tmpPathDir) - assert.Assert(t, err == nil) +func (ctx *testContext) createTestPluginWithPath(pluginName string, fileMode os.FileMode, path string) error { + fullPath := filepath.Join(path, pluginName) + return ioutil.WriteFile(fullPath, []byte(KnTestPluginScript), fileMode) +} + +func TestPluginList(t *testing.T) { + + setup := func(t *testing.T) *testContext { + knParams := &commands.KnParams{} + pluginCmd := NewPluginCommand(knParams) + + rootCmd, _, out := commands.CreateTestKnCommand(pluginCmd, knParams) + pluginsDir, err := ioutil.TempDir("", "plugin-list-plugindir") + assert.NilError(t, err) + pathDir, err := ioutil.TempDir("", "plugin-list-pathdir") + assert.NilError(t, err) + + origPath := os.Getenv("PATH") + assert.NilError(t, os.Setenv("PATH", pathDir)) + + return &testContext{ + rootCmd: rootCmd, + out: out, + pluginsDir: pluginsDir, + pathDir: pathDir, + origPath: origPath, + } } t.Run("creates a new cobra.Command", func(t *testing.T) { - setup(t) - defer cleanup(t) + pluginCmd := NewPluginCommand(&commands.KnParams{}) + pluginListCmd := FindSubCommand(t, pluginCmd, "list") + assert.Assert(t, pluginListCmd != nil) assert.Assert(t, pluginListCmd != nil) assert.Assert(t, pluginListCmd.Use == "list") - assert.Assert(t, pluginListCmd.Short == "List all visible plugin executables") - assert.Assert(t, strings.Contains(pluginListCmd.Long, "List all visible plugin executables")) + assert.Assert(t, pluginListCmd.Short == "List plugins") + assert.Assert(t, strings.Contains(pluginListCmd.Long, "List all installed plugins")) assert.Assert(t, pluginListCmd.Flags().Lookup("plugins-dir") != nil) assert.Assert(t, pluginListCmd.RunE != nil) }) t.Run("when pluginsDir does not include any plugins", func(t *testing.T) { t.Run("when --lookup-plugins-in-path is true", func(t *testing.T) { - var pluginPath string - - beforeEach := func(t *testing.T) { - err = os.Setenv("PATH", tmpPathDir) - assert.Assert(t, err == nil) - } - t.Run("no plugins installed", func(t *testing.T) { - setup(t) - defer cleanup(t) - beforeEach(t) + ctx := setup(t) + defer ctx.cleanup() t.Run("warns user that no plugins found", func(t *testing.T) { - rootCmd.SetArgs([]string{"plugin", "list", "--lookup-plugins-in-path=true", pluginsDirFlag}) - err = rootCmd.Execute() - assert.Assert(t, err != nil) - assert.Assert(t, strings.Contains(err.Error(), "warning: unable to find any kn plugins in your plugin path:")) + err := ctx.execute("plugin", "list", "--lookup-plugins-in-path=true") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(ctx.output(), "No plugins found")) }) }) t.Run("plugins installed", func(t *testing.T) { t.Run("with valid plugin in $PATH", func(t *testing.T) { - beforeEach := func(t *testing.T) { - pluginPath = CreateTestPluginInPath(t, KnTestPluginName, KnTestPluginScript, FileModeExecutable, tmpPathDir) - assert.Assert(t, pluginPath != "") - - err = os.Setenv("PATH", tmpPathDir) - assert.Assert(t, err == nil) - } t.Run("list plugins in $PATH", func(t *testing.T) { - setup(t) - defer cleanup(t) - beforeEach(t) - - commands.CaptureStdout(t) - rootCmd.SetArgs([]string{"plugin", "list", "--lookup-plugins-in-path=true", pluginsDirFlag}) - err = rootCmd.Execute() - assert.Assert(t, err == nil) + ctx := setup(t) + defer ctx.cleanup() + + err := ctx.createTestPlugin(KnTestPluginName, FileModeExecutable, true) + assert.NilError(t, err) + + err = ctx.execute("plugin", "list", "--lookup-plugins-in-path=true") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(ctx.output(), KnTestPluginName)) }) }) t.Run("with non-executable plugin", func(t *testing.T) { - beforeEach := func(t *testing.T) { - pluginPath = CreateTestPluginInPath(t, KnTestPluginName, KnTestPluginScript, FileModeReadable, tmpPathDir) - assert.Assert(t, pluginPath != "") - } - t.Run("warns user plugin invalid", func(t *testing.T) { - setup(t) - defer cleanup(t) - beforeEach(t) - - rootCmd.SetArgs([]string{"plugin", "list", "--lookup-plugins-in-path=true", pluginsDirFlag}) - err = rootCmd.Execute() - assert.Assert(t, err != nil) - assert.Assert(t, strings.Contains(err.Error(), "warning: unable to find any kn plugins in your plugin path:")) + ctx := setup(t) + defer ctx.cleanup() + + err := ctx.createTestPlugin(KnTestPluginName, FileModeReadable, false) + assert.NilError(t, err) + + err = ctx.execute("plugin", "list", "--lookup-plugins-in-path=false") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(ctx.output(), "WARNING", "not executable", "current user")) }) }) t.Run("with plugins with same name", func(t *testing.T) { - var tmpPathDir2 string - - beforeEach := func(t *testing.T) { - pluginPath = CreateTestPluginInPath(t, KnTestPluginName, KnTestPluginScript, FileModeExecutable, tmpPathDir) - assert.Assert(t, pluginPath != "") - tmpPathDir2, err = ioutil.TempDir("", "plugins_list") - assert.Assert(t, err == nil) + t.Run("warns user about second (in $PATH) plugin shadowing first", func(t *testing.T) { + ctx := setup(t) + defer ctx.cleanup() - err = os.Setenv("PATH", tmpPathDir+string(os.PathListSeparator)+tmpPathDir2) - assert.Assert(t, err == nil) + err := ctx.createTestPlugin(KnTestPluginName, FileModeExecutable, true) + assert.NilError(t, err) - pluginPath = CreateTestPluginInPath(t, KnTestPluginName, KnTestPluginScript, FileModeExecutable, tmpPathDir2) - assert.Assert(t, pluginPath != "") - } + tmpPathDir2, err := ioutil.TempDir("", "plugins_list") + assert.NilError(t, err) + defer os.RemoveAll(tmpPathDir2) - afterEach := func(t *testing.T) { - err = os.RemoveAll(tmpPathDir) - assert.Assert(t, err == nil) + err = os.Setenv("PATH", ctx.pathDir+string(os.PathListSeparator)+tmpPathDir2) + assert.NilError(t, err) - err = os.RemoveAll(tmpPathDir2) - assert.Assert(t, err == nil) - } + err = ctx.createTestPluginWithPath(KnTestPluginName, FileModeExecutable, tmpPathDir2) + assert.NilError(t, err) - t.Run("warns user about second (in $PATH) plugin shadowing first", func(t *testing.T) { - setup(t) - defer cleanup(t) - beforeEach(t) - defer afterEach(t) - - rootCmd.SetArgs([]string{"plugin", "list", "--lookup-plugins-in-path=true", pluginsDirFlag}) - err = rootCmd.Execute() - assert.Assert(t, err != nil) - assert.Assert(t, strings.Contains(err.Error(), "error: one plugin warning was found")) + err = ctx.execute("plugin", "list", "--lookup-plugins-in-path=true") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(ctx.output(), "WARNING", "shadowed", "ignored")) }) }) t.Run("with plugins with name of existing command", func(t *testing.T) { - var fakeCmd *cobra.Command + t.Run("warns user about overwriting existing command", func(t *testing.T) { + ctx := setup(t) + defer ctx.cleanup() - beforeEach := func(t *testing.T) { - fakeCmd = &cobra.Command{ + fakeCmd := &cobra.Command{ Use: "fake", } - rootCmd.AddCommand(fakeCmd) + ctx.rootCmd.AddCommand(fakeCmd) + defer ctx.rootCmd.RemoveCommand(fakeCmd) - pluginPath = CreateTestPluginInPath(t, "kn-fake", KnTestPluginScript, FileModeExecutable, tmpPathDir) - assert.Assert(t, pluginPath != "") + err := ctx.createTestPlugin("kn-fake", FileModeExecutable, true) + assert.NilError(t, err) - err = os.Setenv("PATH", tmpPathDir) - assert.Assert(t, err == nil) - } - - afterEach := func(t *testing.T) { - rootCmd.RemoveCommand(fakeCmd) - } - - t.Run("warns user about overwritting exising command", func(t *testing.T) { - setup(t) - defer cleanup(t) - beforeEach(t) - defer afterEach(t) - - rootCmd.SetArgs([]string{"plugin", "list", "--lookup-plugins-in-path=true", pluginsDirFlag}) - err = rootCmd.Execute() - assert.Assert(t, err != nil) - assert.Assert(t, strings.Contains(err.Error(), "error: one plugin warning was found")) + err = ctx.execute("plugin", "list", "--lookup-plugins-in-path=true") + assert.ErrorContains(t, err, "overwrite", "built-in") + assert.Assert(t, util.ContainsAll(ctx.output(), "ERROR", "overwrite", "built-in")) }) }) }) @@ -206,35 +195,24 @@ func TestPluginList(t *testing.T) { }) t.Run("when pluginsDir has plugins", func(t *testing.T) { - var pluginPath string - - beforeEach := func(t *testing.T) { - pluginPath = CreateTestPluginInPath(t, KnTestPluginName, KnTestPluginScript, FileModeExecutable, tmpPathDir) - assert.Assert(t, pluginPath != "") - - err = os.Setenv("PATH", "") - assert.Assert(t, err == nil) - - pluginsDirFlag = fmt.Sprintf("--plugins-dir=%s", tmpPathDir) - } - t.Run("list plugins in --plugins-dir", func(t *testing.T) { - setup(t) - defer cleanup(t) - beforeEach(t) + ctx := setup(t) + defer ctx.cleanup() + + err := ctx.createTestPlugin(KnTestPluginName, FileModeExecutable, false) - rootCmd.SetArgs([]string{"plugin", "list", pluginsDirFlag}) - err = rootCmd.Execute() - assert.Assert(t, err == nil) + err = ctx.execute("plugin", "list") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(ctx.output(), KnTestPluginName)) }) t.Run("no plugins installed", func(t *testing.T) { - setup(t) - defer cleanup(t) + ctx := setup(t) + defer ctx.cleanup() - rootCmd.SetArgs([]string{"plugin", "list", pluginsDirFlag}) - err = rootCmd.Execute() - assert.Assert(t, err != nil) + err := ctx.execute("plugin", "list") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(ctx.output(), "No plugins")) }) }) } diff --git a/pkg/kn/commands/plugin/plugin_test_helper.go b/pkg/kn/commands/plugin/plugin_test_helper.go index d3bad5463b..eec6ed305e 100644 --- a/pkg/kn/commands/plugin/plugin_test_helper.go +++ b/pkg/kn/commands/plugin/plugin_test_helper.go @@ -57,7 +57,7 @@ func CreateTestPlugin(t *testing.T, name, script string, fileMode os.FileMode) s // CreateTestPluginInPath with name, path, script, and fileMode and return the tmp random path func CreateTestPluginInPath(t *testing.T, name, script string, fileMode os.FileMode, path string) string { err := ioutil.WriteFile(filepath.Join(path, name), []byte(script), fileMode) - assert.Assert(t, err == nil) + assert.NilError(t, err) return filepath.Join(path, name) } diff --git a/pkg/kn/commands/plugin/plugin_verifier.go b/pkg/kn/commands/plugin/plugin_verifier.go new file mode 100644 index 0000000000..9f6247250f --- /dev/null +++ b/pkg/kn/commands/plugin/plugin_verifier.go @@ -0,0 +1,219 @@ +// Copyright © 2018 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package plugin + +import ( + "fmt" + "io" + "os" + "path/filepath" + "runtime" + "strings" + "syscall" + + "github.com/pkg/errors" + "github.com/spf13/cobra" +) + +// pluginVerifier verifies that existing kn commands are not overriden +type pluginVerifier struct { + root *cobra.Command + seenPlugins map[string]string +} + +// collect errors and warnings on the way +type errorsAndWarnings struct { + errors []string + warnings []string +} + +// Create new verifier +func newPluginVerifier(root *cobra.Command) *pluginVerifier { + return &pluginVerifier{ + root: root, + seenPlugins: make(map[string]string), + } +} + +// permission bits for execute +const ( + UserExecute = 1 << 6 + GroupExecute = 1 << 3 + OtherExecute = 1 << 1 +) + +// Verify implements pathVerifier and determines if a given path +// is valid depending on whether or not it overwrites an existing +// kn command path, or a previously seen plugin. +func (v *pluginVerifier) verify(eaw errorsAndWarnings, path string) errorsAndWarnings { + if v.root == nil { + return eaw.addError("unable to verify path with nil root") + } + + // Verify that plugin actually exists + fileInfo, err := os.Stat(path) + if err != nil { + if err == os.ErrNotExist { + return eaw.addError("cannot find plugin in %s", path) + } + return eaw.addError("cannot stat %s: %v", path, err) + } + + eaw = v.addErrorIfWrongPrefix(eaw, path) + eaw = v.addWarningIfNotExecutable(eaw, path, fileInfo) + eaw = v.addWarningIfAlreadySeen(eaw, path) + eaw = v.addErrorIfOverwritingExistingCommand(eaw, path) + + return eaw +} + +func (v *pluginVerifier) addWarningIfAlreadySeen(eaw errorsAndWarnings, path string) errorsAndWarnings { + fileName := filepath.Base(path) + if existingPath, ok := v.seenPlugins[fileName]; ok { + return eaw.addWarning("%s is ignored because it is shadowed by a equally named plugin: %s.", path, existingPath) + } + v.seenPlugins[fileName] = path + return eaw +} + +func (v *pluginVerifier) addErrorIfOverwritingExistingCommand(eaw errorsAndWarnings, path string) errorsAndWarnings { + fileName := filepath.Base(path) + cmds := strings.Split(fileName, "-") + if len(cmds) < 2 { + return eaw.addError("%s is not a valid plugin filename as its missing a prefix", fileName) + } + cmds = cmds[1:] + + // Check both, commands with underscore and with dash because plugins can be called with both + overwrittenCommands := make(map[string]bool) + for _, c := range [][]string{cmds, convertUnderscoresToDashes(cmds)} { + cmd, _, err := v.root.Find(c) + if err == nil { + overwrittenCommands[cmd.CommandPath()] = true + } + } + for command := range overwrittenCommands { + eaw.addError("%s overwrites existing built-in command: %s", fileName, command) + } + return eaw +} + +func (v *pluginVerifier) addErrorIfWrongPrefix(eaw errorsAndWarnings, path string) errorsAndWarnings { + fileName := filepath.Base(path) + // Only pick the first prefix as it is very like that it will be reduced to + // a single prefix anyway (PR pending) + prefix := ValidPluginFilenamePrefixes[0] + if !strings.HasPrefix(fileName, prefix+"-") { + eaw.addWarning("%s plugin doesn't start with plugin prefix %s", fileName, prefix) + } + return eaw +} + +func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path string, fileInfo os.FileInfo) errorsAndWarnings { + if runtime.GOOS == "windows" { + fileExt := strings.ToLower(filepath.Ext(fileInfo.Name())) + + switch fileExt { + case ".bat", ".cmd", ".com", ".exe", ".ps1": + return eaw + } + return eaw.addWarning("%s is not executable as it does not have the proper extension", path) + } + + mode := fileInfo.Mode() + if !mode.IsRegular() { + return eaw.addWarning("%s is not a regular file", path) + } + perms := uint32(mode.Perm()) + + // All can execute + if perms&OtherExecute != 0 { + return eaw + } + + var sys *syscall.Stat_t + var ok bool + if sys, ok = fileInfo.Sys().(*syscall.Stat_t); !ok { + // We can check the files' owner/group + return eaw.addWarning("cannot check owner/group of file %s", path) + } + + // User can execute + if perms&UserExecute != 0 && int(sys.Uid) == os.Getuid() { + return eaw + } + + // User is in group which can execute + if perms&GroupExecute != 0 { + groups, err := os.Getgroups() + if err != nil { + return eaw.addError("cannot get group ids for checking executable status of file %s", path) + } + for _, gid := range groups { + if int(sys.Gid) == gid { + return eaw + } + } + } + + return eaw.addWarning("%s is not executable by current user", path) +} + +func (eaw *errorsAndWarnings) addError(format string, args ...interface{}) errorsAndWarnings { + eaw.errors = append(eaw.errors, fmt.Sprintf(format, args...)) + return *eaw +} + +func (eaw *errorsAndWarnings) addWarning(format string, args ...interface{}) errorsAndWarnings { + eaw.warnings = append(eaw.warnings, fmt.Sprintf(format, args...)) + return *eaw +} + +func (eaw *errorsAndWarnings) printWarningsAndErrors(out io.Writer) { + printSection(out, "ERROR", eaw.errors) + printSection(out, "WARNING", eaw.warnings) +} + +func (eaw *errorsAndWarnings) combinedError() error { + if len(eaw.errors) == 0 { + return nil + } + return errors.New(strings.Join(eaw.errors, ",")) +} + +func printSection(out io.Writer, label string, values []string) { + if len(values) > 0 { + printLabelWithConditionalPluralS(out, label, len(values)) + for _, value := range values { + fmt.Fprintf(out, " - %s\n", value) + } + } +} + +func printLabelWithConditionalPluralS(out io.Writer, label string, nr int) { + if nr == 1 { + fmt.Fprintf(out, "%s:\n", label) + } else { + fmt.Fprintf(out, "%ss:\n", label) + } +} + +func convertUnderscoresToDashes(cmds []string) []string { + ret := make([]string, len(cmds)) + for i := range cmds { + ret[i] = strings.ReplaceAll(cmds[i], "_", "-") + } + return ret +} diff --git a/pkg/kn/commands/plugin/path_verifier_test.go b/pkg/kn/commands/plugin/plugin_verifier_test.go similarity index 63% rename from pkg/kn/commands/plugin/path_verifier_test.go rename to pkg/kn/commands/plugin/plugin_verifier_test.go index e407c68824..5c61ae5f01 100644 --- a/pkg/kn/commands/plugin/path_verifier_test.go +++ b/pkg/kn/commands/plugin/plugin_verifier_test.go @@ -15,29 +15,26 @@ package plugin import ( - "fmt" - "strings" "testing" "github.com/knative/client/pkg/kn/commands" + "github.com/knative/client/pkg/util" + "github.com/spf13/cobra" "gotest.tools/assert" ) -func TestCommandOverrideVerifier(t *testing.T) { +func TestPluginVerifier(t *testing.T) { var ( pluginPath string rootCmd *cobra.Command - verifier *CommandOverrideVerifier + verifier *pluginVerifier ) setup := func(t *testing.T) { knParams := &commands.KnParams{} rootCmd, _, _ = commands.CreateTestKnCommand(NewPluginCommand(knParams), knParams) - verifier = &CommandOverrideVerifier{ - Root: rootCmd, - SeenPlugins: make(map[string]string), - } + verifier = newPluginVerifier(rootCmd) } cleanup := func(t *testing.T) { @@ -50,12 +47,12 @@ func TestCommandOverrideVerifier(t *testing.T) { t.Run("returns error verifying path", func(t *testing.T) { setup(t) defer cleanup(t) - verifier.Root = nil - - errs := verifier.Verify(pluginPath) - assert.Assert(t, len(errs) == 1) - assert.Assert(t, errs[0] != nil) - assert.Assert(t, strings.Contains(errs[0].Error(), "unable to verify path with nil root")) + verifier.root = nil + eaw := errorsAndWarnings{} + eaw = verifier.verify(eaw, pluginPath) + assert.Assert(t, len(eaw.errors) == 1) + assert.Assert(t, len(eaw.warnings) == 0) + assert.Assert(t, util.ContainsAll(eaw.errors[0], "nil root")) }) }) @@ -66,11 +63,11 @@ func TestCommandOverrideVerifier(t *testing.T) { pluginPath = CreateTestPlugin(t, KnTestPluginName, KnTestPluginScript, FileModeReadable) t.Run("fails with not executable error", func(t *testing.T) { - errs := verifier.Verify(pluginPath) - assert.Assert(t, len(errs) == 1) - assert.Assert(t, errs[0] != nil) - errorMsg := fmt.Sprintf("warning: %s identified as a kn plugin, but it is not executable", pluginPath) - assert.Assert(t, strings.Contains(errs[0].Error(), errorMsg)) + eaw := errorsAndWarnings{} + eaw = verifier.verify(eaw, pluginPath) + assert.Assert(t, len(eaw.warnings) == 1) + assert.Assert(t, len(eaw.errors) == 0) + assert.Assert(t, util.ContainsAll(eaw.warnings[0], pluginPath, "not executable")) }) }) @@ -81,15 +78,15 @@ func TestCommandOverrideVerifier(t *testing.T) { t.Run("when kn plugin in path shadows another", func(t *testing.T) { var shadowPluginPath = CreateTestPlugin(t, KnTestPluginName, KnTestPluginScript, FileModeExecutable) - verifier.SeenPlugins[KnTestPluginName] = pluginPath + verifier.seenPlugins[KnTestPluginName] = pluginPath defer DeleteTestPlugin(t, shadowPluginPath) t.Run("fails with overshadowed error", func(t *testing.T) { - errs := verifier.Verify(shadowPluginPath) - assert.Assert(t, len(errs) == 1) - assert.Assert(t, errs[0] != nil) - errorMsg := fmt.Sprintf("warning: %s is overshadowed by a similarly named plugin: %s", shadowPluginPath, pluginPath) - assert.Assert(t, strings.Contains(errs[0].Error(), errorMsg)) + eaw := errorsAndWarnings{} + eaw = verifier.verify(eaw, shadowPluginPath) + assert.Assert(t, len(eaw.errors) == 0) + assert.Assert(t, len(eaw.warnings) == 1) + assert.Assert(t, util.ContainsAll(eaw.warnings[0], "shadowed", "ignored")) }) }) }) @@ -101,11 +98,11 @@ func TestCommandOverrideVerifier(t *testing.T) { defer DeleteTestPlugin(t, overwritingPluginPath) t.Run("fails with overwrites error", func(t *testing.T) { - errs := verifier.Verify(overwritingPluginPath) - assert.Assert(t, len(errs) == 1) - assert.Assert(t, errs[0] != nil) - errorMsg := fmt.Sprintf("warning: %s overwrites existing command: %q", "kn-plugin", "kn plugin") - assert.Assert(t, strings.Contains(errs[0].Error(), errorMsg)) + eaw := errorsAndWarnings{} + eaw = verifier.verify(eaw, overwritingPluginPath) + assert.Assert(t, len(eaw.errors) == 1) + assert.Assert(t, len(eaw.warnings) == 0) + assert.Assert(t, util.ContainsAll(eaw.errors[0], "overwrite", "kn-plugin")) }) }) }) From 1b1ffae15c5f6001d492920b563329aa782cde07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Sun, 28 Jul 2019 08:15:07 +0200 Subject: [PATCH 2/8] fix(plugins): Fixed broken homedir expansion --- pkg/kn/commands/plugin/plugin_handler.go | 4 +++- pkg/kn/commands/plugin/plugin_list.go | 26 +----------------------- 2 files changed, 4 insertions(+), 26 deletions(-) diff --git a/pkg/kn/commands/plugin/plugin_handler.go b/pkg/kn/commands/plugin/plugin_handler.go index 367121305c..cf3536839d 100644 --- a/pkg/kn/commands/plugin/plugin_handler.go +++ b/pkg/kn/commands/plugin/plugin_handler.go @@ -22,6 +22,8 @@ import ( "path/filepath" "strings" "syscall" + + "github.com/mitchellh/go-homedir" ) // PluginHandler is capable of parsing command line arguments @@ -62,7 +64,7 @@ func (h *DefaultPluginHandler) Lookup(name string) (string, bool) { pluginPath := fmt.Sprintf("%s-%s", prefix, name) // Try to find plugin in pluginsDir - pluginDir, err := expandPath(h.PluginsDir) + pluginDir, err := homedir.Expand(h.PluginsDir) if err != nil { return "", false } diff --git a/pkg/kn/commands/plugin/plugin_list.go b/pkg/kn/commands/plugin/plugin_list.go index 6c550eb769..5de4d09832 100644 --- a/pkg/kn/commands/plugin/plugin_list.go +++ b/pkg/kn/commands/plugin/plugin_list.go @@ -60,7 +60,7 @@ Available plugins are those that are: // List plugins by looking up in plugin directory and path func listPlugins(cmd *cobra.Command, flags pluginListFlags) error { - pluginPath, err := expandPath(commands.Cfg.PluginsDir) + pluginPath, err := homedir.Expand(commands.Cfg.PluginsDir) if err != nil { return err } @@ -128,30 +128,6 @@ func lookupPlugins(pluginPath string) ([]string, errorsAndWarnings) { return pluginsFound, eaw } -// Private -// expandPath to a canonical path (need to see if Golang has a better option) -func expandPath(path string) (string, error) { - if strings.Contains(path, "~") { - var err error - path, err = expandHomeDir(path) - if err != nil { - return "", err - } - } - return path, nil -} - -// expandHomeDir replaces the ~ with the home directory value -func expandHomeDir(path string) (string, error) { - home, err := homedir.Dir() - if err != nil { - fmt.Fprintln(os.Stderr, err) - return "", err - } - - return strings.Replace(path, "~", home, -1), nil -} - func hasValidPrefix(filepath string, validPrefixes []string) bool { for _, prefix := range validPrefixes { if !strings.HasPrefix(filepath, prefix+"-") { From 40722b2bdb4614f3b6cf323e4a7e1912d980f086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Sun, 28 Jul 2019 08:55:27 +0200 Subject: [PATCH 3/8] fix(plugin): Adapted to proper permission handling E.g. if u-x, g+x and the user has the group of the file, but is also owner of the file then he is not allowed to execute the file. --- pkg/kn/commands/plugin/plugin_verifier.go | 57 +++++++++++++++-------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/pkg/kn/commands/plugin/plugin_verifier.go b/pkg/kn/commands/plugin/plugin_verifier.go index 9f6247250f..753a7fdf8c 100644 --- a/pkg/kn/commands/plugin/plugin_verifier.go +++ b/pkg/kn/commands/plugin/plugin_verifier.go @@ -51,7 +51,7 @@ func newPluginVerifier(root *cobra.Command) *pluginVerifier { const ( UserExecute = 1 << 6 GroupExecute = 1 << 3 - OtherExecute = 1 << 1 + OtherExecute = 1 << 0 ) // Verify implements pathVerifier and determines if a given path @@ -138,11 +138,6 @@ func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path s } perms := uint32(mode.Perm()) - // All can execute - if perms&OtherExecute != 0 { - return eaw - } - var sys *syscall.Stat_t var ok bool if sys, ok = fileInfo.Sys().(*syscall.Stat_t); !ok { @@ -150,27 +145,51 @@ func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path s return eaw.addWarning("cannot check owner/group of file %s", path) } - // User can execute - if perms&UserExecute != 0 && int(sys.Uid) == os.Getuid() { + isOwner := checkIfUserIsFileOwner(sys.Uid) + isInGroup, err := checkIfUserInGroup(sys.Gid) + if err != nil { + return eaw.addError("cannot get group ids for checking executable status of file %s", path) + } + + // User is owner and owner can execute + if perms&UserExecute != 0 && isOwner { return eaw } - // User is in group which can execute - if perms&GroupExecute != 0 { - groups, err := os.Getgroups() - if err != nil { - return eaw.addError("cannot get group ids for checking executable status of file %s", path) - } - for _, gid := range groups { - if int(sys.Gid) == gid { - return eaw - } - } + // User is in group which can execute, but user is not file owner + if perms&GroupExecute != 0 && !isOwner && isInGroup { + return eaw + } + + // All can execute, and the user is not file owner and not in the file's perm group + if perms&OtherExecute != 0 && !isOwner && !isInGroup { + return eaw } return eaw.addWarning("%s is not executable by current user", path) } +func checkIfUserInGroup(gid uint32) (bool, error) { + groups, err := os.Getgroups() + if err != nil { + return false, err + } + for _, g := range groups { + if int(gid) == g { + return true, nil + } + } + return false, nil +} + +func checkIfUserIsFileOwner(uid uint32) bool { + if int(uid) == os.Getuid() { + return true + } + return false +} + + func (eaw *errorsAndWarnings) addError(format string, args ...interface{}) errorsAndWarnings { eaw.errors = append(eaw.errors, fmt.Sprintf(format, args...)) return *eaw From 882560d71168394f33ecaae92952152241dc8e8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Sun, 28 Jul 2019 10:51:36 +0200 Subject: [PATCH 4/8] test(plugins): Added checks for the execution check for plugins --- pkg/kn/commands/plugin/plugin_list_test.go | 4 + pkg/kn/commands/plugin/plugin_test_helper.go | 7 +- .../commands/plugin/plugin_verifier_test.go | 164 +++++++++++++++++- 3 files changed, 167 insertions(+), 8 deletions(-) diff --git a/pkg/kn/commands/plugin/plugin_list_test.go b/pkg/kn/commands/plugin/plugin_list_test.go index adaa158ad0..1c4a5dd81f 100644 --- a/pkg/kn/commands/plugin/plugin_list_test.go +++ b/pkg/kn/commands/plugin/plugin_list_test.go @@ -20,6 +20,7 @@ import ( "io/ioutil" "os" "path/filepath" + "runtime" "strings" "testing" @@ -62,6 +63,9 @@ func (ctx *testContext) createTestPlugin(pluginName string, fileMode os.FileMode } func (ctx *testContext) createTestPluginWithPath(pluginName string, fileMode os.FileMode, path string) error { + if runtime.GOOS == "windows" { + pluginName += ".bat" + } fullPath := filepath.Join(path, pluginName) return ioutil.WriteFile(fullPath, []byte(KnTestPluginScript), fileMode) } diff --git a/pkg/kn/commands/plugin/plugin_test_helper.go b/pkg/kn/commands/plugin/plugin_test_helper.go index eec6ed305e..7af57a74d6 100644 --- a/pkg/kn/commands/plugin/plugin_test_helper.go +++ b/pkg/kn/commands/plugin/plugin_test_helper.go @@ -18,6 +18,7 @@ import ( "io/ioutil" "os" "path/filepath" + "runtime" "testing" "github.com/spf13/cobra" @@ -56,7 +57,11 @@ func CreateTestPlugin(t *testing.T, name, script string, fileMode os.FileMode) s // CreateTestPluginInPath with name, path, script, and fileMode and return the tmp random path func CreateTestPluginInPath(t *testing.T, name, script string, fileMode os.FileMode, path string) string { - err := ioutil.WriteFile(filepath.Join(path, name), []byte(script), fileMode) + fullPath := filepath.Join(path, name) + if runtime.GOOS == "windows" { + fullPath += ".bat" + } + err := ioutil.WriteFile(fullPath, []byte(script), fileMode) assert.NilError(t, err) return filepath.Join(path, name) diff --git a/pkg/kn/commands/plugin/plugin_verifier_test.go b/pkg/kn/commands/plugin/plugin_verifier_test.go index 5c61ae5f01..e91f7fd5f0 100644 --- a/pkg/kn/commands/plugin/plugin_verifier_test.go +++ b/pkg/kn/commands/plugin/plugin_verifier_test.go @@ -15,6 +15,14 @@ package plugin import ( + "errors" + "fmt" + "io/ioutil" + "os" + "os/user" + "path/filepath" + "runtime" + "strconv" "testing" "github.com/knative/client/pkg/kn/commands" @@ -25,6 +33,7 @@ import ( ) func TestPluginVerifier(t *testing.T) { + var ( pluginPath string rootCmd *cobra.Command @@ -57,17 +66,36 @@ func TestPluginVerifier(t *testing.T) { }) t.Run("with root command", func(t *testing.T) { - t.Run("when plugin in path not executable", func(t *testing.T) { + t.Run("whether plugin in path is executable (unix only)", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Skip test for windows as the permission check are for Unix only") + return + } + setup(t) defer cleanup(t) - pluginPath = CreateTestPlugin(t, KnTestPluginName, KnTestPluginScript, FileModeReadable) + pluginDir, err := ioutil.TempDir("", "plugin") + assert.NilError(t, err) + defer os.RemoveAll(pluginDir) + pluginPath := filepath.Join(pluginDir,"kn-execution-test") + err = ioutil.WriteFile(pluginPath, []byte{}, 0644) + assert.NilError(t, err) t.Run("fails with not executable error", func(t *testing.T) { - eaw := errorsAndWarnings{} - eaw = verifier.verify(eaw, pluginPath) - assert.Assert(t, len(eaw.warnings) == 1) - assert.Assert(t, len(eaw.errors) == 0) - assert.Assert(t, util.ContainsAll(eaw.warnings[0], pluginPath, "not executable")) + for _, data := range getExecutionCheckTestParams() { + eaw := errorsAndWarnings{} + assert.NilError(t,prepareFile(pluginPath, data.userId, data.groupId, data.mode)) + eaw = newPluginVerifier(rootCmd).verify(eaw, pluginPath) + + if data.isExecutable { + assert.Assert(t, len(eaw.warnings) == 0, "executable: %s | %v", data.string(), eaw.warnings) + assert.Assert(t, len(eaw.errors) == 0) + } else { + assert.Assert(t, len(eaw.warnings) == 1, "not executable: %s | %v", data.string(), eaw.warnings) + assert.Assert(t, len(eaw.errors) == 0) + assert.Assert(t, util.ContainsAll(eaw.warnings[0], pluginPath)) + } + } }) }) @@ -107,3 +135,125 @@ func TestPluginVerifier(t *testing.T) { }) }) } + +type executionCheckTestParams struct { + mode os.FileMode + isExecutable bool + userId int + groupId int +} + +func (d executionCheckTestParams) string() string { + return fmt.Sprintf("mode: %03o, isExecutable: %t, uid: %d, gid: %d", d.mode, d.isExecutable, d.userId, d.groupId) +} + +func getExecutionCheckTestParams() []executionCheckTestParams { + currentUser := os.Getuid() + currentGroup := os.Getgid() + + ret := []executionCheckTestParams { + {0000, false, currentUser, currentGroup}, + {0100, true, currentUser, currentGroup}, + {0010, false, currentUser, currentGroup}, + {0001, false, currentUser, currentGroup}, + {0110, true, currentUser, currentGroup}, + {0011, false, currentUser, currentGroup}, + {0101, true, currentUser, currentGroup}, + {0111, true, currentUser, currentGroup}, + } + + // The following parameters only work when running under root + // because otherwise you can't change file permissions to other users + // or groups you are not belonging to + if currentUser != 0 { + return ret + } + + foreignGroup, err := lookupForeignGroup() + if err == nil { + for _, param := range []executionCheckTestParams{ + {0000, false, currentUser, foreignGroup}, + {0100, true, currentUser, foreignGroup}, + {0010, false, currentUser, foreignGroup}, + {0001, false, currentUser, foreignGroup}, + {0110, true, currentUser, foreignGroup}, + {0011, false, currentUser, foreignGroup}, + {0101, true, currentUser, foreignGroup}, + {0111, true, currentUser, foreignGroup}, + } { + ret = append(ret, param) + } + } + + foreignUser, err := lookupForeignUser() + if err != nil { + return ret + } + + for _, param := range []executionCheckTestParams{ + {0000, false, foreignUser, foreignGroup}, + {0100, false, foreignUser, foreignGroup}, + {0010, false, foreignUser, foreignGroup}, + {0001, true, foreignUser, foreignGroup}, + {0110, false, foreignUser, foreignGroup}, + {0011, true, foreignUser, foreignGroup}, + {0101, true, foreignUser, foreignGroup}, + {0111, true, foreignUser, foreignGroup}, + } { + ret = append(ret, param) + } + + return ret +} + +func lookupForeignUser() (int, error) { + for _, probe := range []string { "daemon", "nobody", "_unknown" } { + u, err := user.Lookup(probe) + if err != nil { + continue + } + uid, err := strconv.Atoi(u.Uid) + if err != nil { + continue + } + if uid != os.Getuid() { + return uid, nil + } + } + return 0, errors.New("could not find foreign user") +} + +func lookupForeignGroup() (int, error) { + gids, err := os.Getgroups() + if err != nil { + return 0, err + } + OUTER: + for _, probe := range []string { "daemon", "wheel", "nobody", "nogroup", "admin" } { + group, err := user.LookupGroup(probe) + if err != nil { + continue + } + gid, err := strconv.Atoi(group.Gid) + if err != nil { + continue + } + + for _, g := range gids { + if gid == g { + continue OUTER + } + } + return gid, nil + } + return 0, errors.New("could not find a foreign group") +} + +func prepareFile(file string, uid int, gid int, perm os.FileMode) error { + err := os.Chmod(file, perm) + if err != nil { + return err + } + return os.Chown(file, uid, gid) +} + From 7c3be1f1e9e4185f1f2ec8fac4a252a312c457c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Sun, 28 Jul 2019 14:24:36 +0200 Subject: [PATCH 5/8] chore(plugins): optimized tests to reflect the real unix behaviour --- pkg/kn/commands/plugin/plugin_verifier.go | 28 +++- .../commands/plugin/plugin_verifier_test.go | 141 +++++++----------- 2 files changed, 76 insertions(+), 93 deletions(-) diff --git a/pkg/kn/commands/plugin/plugin_verifier.go b/pkg/kn/commands/plugin/plugin_verifier.go index 753a7fdf8c..60b29c0835 100644 --- a/pkg/kn/commands/plugin/plugin_verifier.go +++ b/pkg/kn/commands/plugin/plugin_verifier.go @@ -152,18 +152,33 @@ func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path s } // User is owner and owner can execute - if perms&UserExecute != 0 && isOwner { - return eaw + if perms&UserExecute != 0 { + if os.Getuid() == 0 { + return eaw + } + if isOwner { + return eaw + } } // User is in group which can execute, but user is not file owner - if perms&GroupExecute != 0 && !isOwner && isInGroup { - return eaw + if perms&GroupExecute != 0 { + if os.Getuid() == 0 { + return eaw + } + if !isOwner && isInGroup { + return eaw + } } // All can execute, and the user is not file owner and not in the file's perm group - if perms&OtherExecute != 0 && !isOwner && !isInGroup { - return eaw + if perms&OtherExecute != 0 { + if os.Getuid() == 0 { + return eaw + } + if !isOwner && !isInGroup { + return eaw + } } return eaw.addWarning("%s is not executable by current user", path) @@ -189,7 +204,6 @@ func checkIfUserIsFileOwner(uid uint32) bool { return false } - func (eaw *errorsAndWarnings) addError(format string, args ...interface{}) errorsAndWarnings { eaw.errors = append(eaw.errors, fmt.Sprintf(format, args...)) return *eaw diff --git a/pkg/kn/commands/plugin/plugin_verifier_test.go b/pkg/kn/commands/plugin/plugin_verifier_test.go index e91f7fd5f0..b6007b6fe9 100644 --- a/pkg/kn/commands/plugin/plugin_verifier_test.go +++ b/pkg/kn/commands/plugin/plugin_verifier_test.go @@ -16,9 +16,9 @@ package plugin import ( "errors" - "fmt" "io/ioutil" "os" + "os/exec" "os/user" "path/filepath" "runtime" @@ -74,29 +74,39 @@ func TestPluginVerifier(t *testing.T) { setup(t) defer cleanup(t) + pluginDir, err := ioutil.TempDir("", "plugin") assert.NilError(t, err) defer os.RemoveAll(pluginDir) - pluginPath := filepath.Join(pluginDir,"kn-execution-test") - err = ioutil.WriteFile(pluginPath, []byte{}, 0644) - assert.NilError(t, err) - - t.Run("fails with not executable error", func(t *testing.T) { - for _, data := range getExecutionCheckTestParams() { - eaw := errorsAndWarnings{} - assert.NilError(t,prepareFile(pluginPath, data.userId, data.groupId, data.mode)) - eaw = newPluginVerifier(rootCmd).verify(eaw, pluginPath) - - if data.isExecutable { - assert.Assert(t, len(eaw.warnings) == 0, "executable: %s | %v", data.string(), eaw.warnings) - assert.Assert(t, len(eaw.errors) == 0) - } else { - assert.Assert(t, len(eaw.warnings) == 1, "not executable: %s | %v", data.string(), eaw.warnings) - assert.Assert(t, len(eaw.errors) == 0) - assert.Assert(t, util.ContainsAll(eaw.warnings[0], pluginPath)) + pluginPath := filepath.Join(pluginDir, "kn-execution-test") + err = ioutil.WriteFile(pluginPath, []byte("#!/bin/sh\ntrue"), 0644) + assert.NilError(t, err, "can't create test plugin") + + for _, uid := range getExecTestUids() { + for _, gid := range getExecTestGids() { + for _, userPerm := range []int{0, UserExecute} { + for _, groupPerm := range []int{0, GroupExecute} { + for _, otherPerm := range []int{0, OtherExecute} { + perm := os.FileMode(userPerm | groupPerm | otherPerm + 0444) + assert.NilError(t, prepareFile(pluginPath, uid, gid, perm), "prepare plugin file, uid: %d, gid: %d, perm: %03o", uid, gid, perm) + + eaw := errorsAndWarnings{} + eaw = newPluginVerifier(rootCmd).verify(eaw, pluginPath) + + if isExecutable(pluginPath) { + assert.Assert(t, len(eaw.warnings) == 0, "executable: perm %03o | uid %d | gid %d | %v", perm, uid, gid, eaw.warnings) + assert.Assert(t, len(eaw.errors) == 0) + } else { + assert.Assert(t, len(eaw.warnings) == 1, "not executable: perm %03o | uid %d | gid %d | %v", perm, uid, gid, eaw.warnings) + assert.Assert(t, len(eaw.errors) == 0) + assert.Assert(t, util.ContainsAll(eaw.warnings[0], pluginPath)) + } + + } + } } } - }) + } }) t.Run("when kn plugin in path is executable", func(t *testing.T) { @@ -136,78 +146,38 @@ func TestPluginVerifier(t *testing.T) { }) } -type executionCheckTestParams struct { - mode os.FileMode - isExecutable bool - userId int - groupId int +func isExecutable(plugin string) bool { + _, err := exec.Command(plugin).Output() + return err == nil } -func (d executionCheckTestParams) string() string { - return fmt.Sprintf("mode: %03o, isExecutable: %t, uid: %d, gid: %d", d.mode, d.isExecutable, d.userId, d.groupId) +func getExecTestUids() []int { + currentUser := os.Getuid() + // Only root can switch ownership of a file + if currentUser == 0 { + foreignUser, err := lookupForeignUser() + if err == nil { + return []int{currentUser, foreignUser} + } + } + return []int{currentUser} } -func getExecutionCheckTestParams() []executionCheckTestParams { +func getExecTestGids() []int { currentUser := os.Getuid() currentGroup := os.Getgid() - - ret := []executionCheckTestParams { - {0000, false, currentUser, currentGroup}, - {0100, true, currentUser, currentGroup}, - {0010, false, currentUser, currentGroup}, - {0001, false, currentUser, currentGroup}, - {0110, true, currentUser, currentGroup}, - {0011, false, currentUser, currentGroup}, - {0101, true, currentUser, currentGroup}, - {0111, true, currentUser, currentGroup}, - } - - // The following parameters only work when running under root - // because otherwise you can't change file permissions to other users - // or groups you are not belonging to - if currentUser != 0 { - return ret - } - - foreignGroup, err := lookupForeignGroup() - if err == nil { - for _, param := range []executionCheckTestParams{ - {0000, false, currentUser, foreignGroup}, - {0100, true, currentUser, foreignGroup}, - {0010, false, currentUser, foreignGroup}, - {0001, false, currentUser, foreignGroup}, - {0110, true, currentUser, foreignGroup}, - {0011, false, currentUser, foreignGroup}, - {0101, true, currentUser, foreignGroup}, - {0111, true, currentUser, foreignGroup}, - } { - ret = append(ret, param) + // Only root can switch group of a file + if currentUser == 0 { + foreignGroup, err := lookupForeignGroup() + if err == nil { + return []int{currentGroup, foreignGroup} } } - - foreignUser, err := lookupForeignUser() - if err != nil { - return ret - } - - for _, param := range []executionCheckTestParams{ - {0000, false, foreignUser, foreignGroup}, - {0100, false, foreignUser, foreignGroup}, - {0010, false, foreignUser, foreignGroup}, - {0001, true, foreignUser, foreignGroup}, - {0110, false, foreignUser, foreignGroup}, - {0011, true, foreignUser, foreignGroup}, - {0101, true, foreignUser, foreignGroup}, - {0111, true, foreignUser, foreignGroup}, - } { - ret = append(ret, param) - } - - return ret + return []int{currentGroup} } func lookupForeignUser() (int, error) { - for _, probe := range []string { "daemon", "nobody", "_unknown" } { + for _, probe := range []string{"daemon", "nobody", "_unknown"} { u, err := user.Lookup(probe) if err != nil { continue @@ -224,12 +194,12 @@ func lookupForeignUser() (int, error) { } func lookupForeignGroup() (int, error) { - gids, err := os.Getgroups() + gids, err := os.Getgroups() if err != nil { return 0, err } - OUTER: - for _, probe := range []string { "daemon", "wheel", "nobody", "nogroup", "admin" } { +OUTER: + for _, probe := range []string{"daemon", "wheel", "nobody", "nogroup", "admin"} { group, err := user.LookupGroup(probe) if err != nil { continue @@ -250,10 +220,9 @@ func lookupForeignGroup() (int, error) { } func prepareFile(file string, uid int, gid int, perm os.FileMode) error { - err := os.Chmod(file, perm) + err := os.Chown(file, uid, gid) if err != nil { return err } - return os.Chown(file, uid, gid) + return os.Chmod(file, perm) } - From 4cafffd6ac6ee860f7e64f94dda99fe2e824188e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Wed, 7 Aug 2019 09:22:26 +0200 Subject: [PATCH 6/8] chore(plugins): Moved plugin seenMap to top-level verify method. Also, allow symlinks without warnings. --- pkg/kn/commands/plugin/plugin_verifier.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/pkg/kn/commands/plugin/plugin_verifier.go b/pkg/kn/commands/plugin/plugin_verifier.go index 60b29c0835..359789b9e8 100644 --- a/pkg/kn/commands/plugin/plugin_verifier.go +++ b/pkg/kn/commands/plugin/plugin_verifier.go @@ -57,6 +57,7 @@ const ( // Verify implements pathVerifier and determines if a given path // is valid depending on whether or not it overwrites an existing // kn command path, or a previously seen plugin. +// This method is not idempotent and must be called for each path only once. func (v *pluginVerifier) verify(eaw errorsAndWarnings, path string) errorsAndWarnings { if v.root == nil { return eaw.addError("unable to verify path with nil root") @@ -76,6 +77,9 @@ func (v *pluginVerifier) verify(eaw errorsAndWarnings, path string) errorsAndWar eaw = v.addWarningIfAlreadySeen(eaw, path) eaw = v.addErrorIfOverwritingExistingCommand(eaw, path) + // Remember each verified plugin for duplicate check + v.seenPlugins[filepath.Base(path)] = path + return eaw } @@ -84,7 +88,6 @@ func (v *pluginVerifier) addWarningIfAlreadySeen(eaw errorsAndWarnings, path str if existingPath, ok := v.seenPlugins[fileName]; ok { return eaw.addWarning("%s is ignored because it is shadowed by a equally named plugin: %s.", path, existingPath) } - v.seenPlugins[fileName] = path return eaw } @@ -133,8 +136,8 @@ func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path s } mode := fileInfo.Mode() - if !mode.IsRegular() { - return eaw.addWarning("%s is not a regular file", path) + if !mode.IsRegular() && !isSymlink(mode) { + return eaw.addWarning("%s is not a file", path) } perms := uint32(mode.Perm()) @@ -184,6 +187,10 @@ func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path s return eaw.addWarning("%s is not executable by current user", path) } +func isSymlink(mode os.FileMode) bool { + return mode&os.ModeSymlink != 0 +} + func checkIfUserInGroup(gid uint32) (bool, error) { groups, err := os.Getgroups() if err != nil { From d64e381ca18883ddeb77dca3a72ac02e8e40fb4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Thu, 8 Aug 2019 08:53:14 +0200 Subject: [PATCH 7/8] refactor(plugins): Split up some methods in plugin_verifier to make them shorter --- pkg/kn/commands/plugin/plugin_verifier.go | 88 +++++++++++++++-------- 1 file changed, 58 insertions(+), 30 deletions(-) diff --git a/pkg/kn/commands/plugin/plugin_verifier.go b/pkg/kn/commands/plugin/plugin_verifier.go index 359789b9e8..4d4207f086 100644 --- a/pkg/kn/commands/plugin/plugin_verifier.go +++ b/pkg/kn/commands/plugin/plugin_verifier.go @@ -126,13 +126,7 @@ func (v *pluginVerifier) addErrorIfWrongPrefix(eaw errorsAndWarnings, path strin func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path string, fileInfo os.FileInfo) errorsAndWarnings { if runtime.GOOS == "windows" { - fileExt := strings.ToLower(filepath.Ext(fileInfo.Name())) - - switch fileExt { - case ".bat", ".cmd", ".com", ".exe", ".ps1": - return eaw - } - return eaw.addWarning("%s is not executable as it does not have the proper extension", path) + return checkForWindowsExecutable(eaw, fileInfo, path) } mode := fileInfo.Mode() @@ -155,40 +149,31 @@ func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path s } // User is owner and owner can execute - if perms&UserExecute != 0 { - if os.Getuid() == 0 { - return eaw - } - if isOwner { - return eaw - } + if canOwnerExecute(perms, isOwner) { + return eaw } // User is in group which can execute, but user is not file owner - if perms&GroupExecute != 0 { - if os.Getuid() == 0 { - return eaw - } - if !isOwner && isInGroup { - return eaw - } + if canGroupExecute(perms, isOwner, isInGroup) { + return eaw } // All can execute, and the user is not file owner and not in the file's perm group - if perms&OtherExecute != 0 { - if os.Getuid() == 0 { - return eaw - } - if !isOwner && !isInGroup { - return eaw - } + if canOtherExecute(perms, isOwner, isInGroup) { + return eaw } return eaw.addWarning("%s is not executable by current user", path) } -func isSymlink(mode os.FileMode) bool { - return mode&os.ModeSymlink != 0 +func checkForWindowsExecutable(eaw errorsAndWarnings, fileInfo os.FileInfo, path string) errorsAndWarnings { + fileExt := strings.ToLower(filepath.Ext(fileInfo.Name())) + + switch fileExt { + case ".bat", ".cmd", ".com", ".exe", ".ps1": + return eaw + } + return eaw.addWarning("%s is not executable as it does not have the proper extension", path) } func checkIfUserInGroup(gid uint32) (bool, error) { @@ -211,6 +196,45 @@ func checkIfUserIsFileOwner(uid uint32) bool { return false } +// Check if all can execute, and the user is not file owner and not in the file's perm group +func canOtherExecute(perms uint32, isOwner bool, isInGroup bool) bool { + if perms&OtherExecute != 0 { + if os.Getuid() == 0 { + return true + } + if !isOwner && !isInGroup { + return true + } + } + return false +} + +// Check if user is owner and owner can execute +func canOwnerExecute(perms uint32, isOwner bool) bool { + if perms&UserExecute != 0 { + if os.Getuid() == 0 { + return true + } + if isOwner { + return true + } + } + return false +} + +// Check if user is in group which can execute, but user is not file owner +func canGroupExecute(perms uint32, isOwner bool, isInGroup bool) bool { + if perms&GroupExecute != 0 { + if os.Getuid() == 0 { + return true + } + if !isOwner && isInGroup { + return true + } + } + return false +} + func (eaw *errorsAndWarnings) addError(format string, args ...interface{}) errorsAndWarnings { eaw.errors = append(eaw.errors, fmt.Sprintf(format, args...)) return *eaw @@ -257,3 +281,7 @@ func convertUnderscoresToDashes(cmds []string) []string { } return ret } + +func isSymlink(mode os.FileMode) bool { + return mode&os.ModeSymlink != 0 +} From 0bc456c840281eb639cb1728a4df8d5b9de90a97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Thu, 8 Aug 2019 09:12:51 +0200 Subject: [PATCH 8/8] fix: Tuned `plugin list` for better handling when no plugin directory exists --- pkg/kn/commands/plugin/plugin_list.go | 26 +++++++++++++++++----- pkg/kn/commands/plugin/plugin_list_test.go | 18 ++++++++++----- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/pkg/kn/commands/plugin/plugin_list.go b/pkg/kn/commands/plugin/plugin_list.go index 5de4d09832..eefcac2ab4 100644 --- a/pkg/kn/commands/plugin/plugin_list.go +++ b/pkg/kn/commands/plugin/plugin_list.go @@ -72,17 +72,19 @@ func listPlugins(cmd *cobra.Command, flags pluginListFlags) error { out := cmd.OutOrStdout() - if len(pluginsFound) == 0 { - fmt.Fprintf(out, "No plugins found in path %s\n", pluginPath) - return nil - } - if flags.verbose { fmt.Fprintf(out, "The following plugins are available, using options:\n") - fmt.Fprintf(out, " - plugins dir: '%s'\n", commands.Cfg.PluginsDir) + fmt.Fprintf(out, " - plugins dir: '%s'%s\n", commands.Cfg.PluginsDir, extraLabelIfPathNotExists(pluginPath)) fmt.Fprintf(out, " - lookup plugins in path: '%t'\n", commands.Cfg.LookupPluginsInPath) } + if len(pluginsFound) == 0 { + if flags.verbose { + fmt.Fprintf(out, "No plugins found in path %s\n", pluginPath) + } + return nil + } + verifier := newPluginVerifier(cmd.Root()) for _, plugin := range pluginsFound { name := plugin @@ -152,3 +154,15 @@ func uniquePathsList(paths []string) []string { } return newPaths } + +// create an info label which can be appended to an verbose output +func extraLabelIfPathNotExists(path string) string { + _, err := os.Stat(path) + if err == nil { + return "" + } + if os.IsNotExist(err) { + return " (does not exist)" + } + return "" +} diff --git a/pkg/kn/commands/plugin/plugin_list_test.go b/pkg/kn/commands/plugin/plugin_list_test.go index 1c4a5dd81f..2c8ca6cda8 100644 --- a/pkg/kn/commands/plugin/plugin_list_test.go +++ b/pkg/kn/commands/plugin/plugin_list_test.go @@ -110,14 +110,22 @@ func TestPluginList(t *testing.T) { t.Run("when pluginsDir does not include any plugins", func(t *testing.T) { t.Run("when --lookup-plugins-in-path is true", func(t *testing.T) { t.Run("no plugins installed", func(t *testing.T) { - ctx := setup(t) - defer ctx.cleanup() - t.Run("warns user that no plugins found", func(t *testing.T) { - err := ctx.execute("plugin", "list", "--lookup-plugins-in-path=true") + t.Run("warns user that no plugins found in verbose mode", func(t *testing.T) { + ctx := setup(t) + defer ctx.cleanup() + err := ctx.execute("plugin", "list", "--lookup-plugins-in-path=true", "--verbose") assert.NilError(t, err) assert.Assert(t, util.ContainsAll(ctx.output(), "No plugins found")) }) + + t.Run("no output when no plugins found", func(t *testing.T) { + ctx := setup(t) + defer ctx.cleanup() + err := ctx.execute("plugin", "list", "--lookup-plugins-in-path=true") + assert.NilError(t, err) + assert.Equal(t, ctx.output(), "") + }) }) t.Run("plugins installed", func(t *testing.T) { @@ -216,7 +224,7 @@ func TestPluginList(t *testing.T) { err := ctx.execute("plugin", "list") assert.NilError(t, err) - assert.Assert(t, util.ContainsAll(ctx.output(), "No plugins")) + assert.Equal(t, ctx.output(), "") }) }) }