From cf6d7d4b430a414a12ef642d93936734d4751039 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 16:52:05 +0800 Subject: [PATCH 01/31] fix Signed-off-by: Patrick Zheng --- plugin/manager.go | 6 +++--- plugin/manager_unix.go | 12 ++++++++++++ plugin/manager_windows.go | 13 +++++++++++++ plugin/plugin.go | 30 ++++++++++++++---------------- plugin/plugin_test.go | 16 ++++++++-------- 5 files changed, 50 insertions(+), 27 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 65aea0bd..6230e6f8 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -127,7 +127,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) // input is not a dir, check if it's a single plugin executable file installFromNonDir = true pluginExecutableFile = installOpts.PluginPath - pluginName, err = ParsePluginName(filepath.Base(pluginExecutableFile)) + pluginName, err = parsePluginName(filepath.Base(pluginExecutableFile)) if err != nil { return nil, nil, fmt.Errorf("failed to read plugin name from file path %s: %w", pluginExecutableFile, err) } @@ -140,7 +140,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } // validate and get new plugin metadata - if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginExecutableFile), pluginName); err != nil { + if err := validatePluginFilenameAgainstOS(filepath.Base(pluginExecutableFile), pluginName); err != nil { return nil, nil, err } newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) @@ -246,7 +246,7 @@ func parsePluginFromDir(path string) (string, string, error) { } // only take regular files if info.Mode().IsRegular() { - if pluginName, err = ParsePluginName(d.Name()); err != nil { + if pluginName, err = parsePluginName(d.Name()); err != nil { // file name does not follow the notation-{plugin-name} format, // continue return nil diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index 6d173383..b867ae1c 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -17,7 +17,9 @@ package plugin import ( + "fmt" "os" + "strings" "github.com/notaryproject/notation-go/plugin/proto" ) @@ -38,3 +40,13 @@ func isExecutableFile(filePath string) (bool, error) { } return mode.Perm()&0100 != 0, nil } + +// parsePluginName checks if fileName is a valid plugin file name +// and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation +func parsePluginName(fileName string) (string, error) { + pluginName, found := strings.CutPrefix(fname, proto.Prefix) + if !found { + return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) + } + return pluginName, nil +} diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index c8504a0e..2bffbcf5 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -14,10 +14,12 @@ package plugin import ( + "fmt" "os" "path/filepath" "strings" + "github.com/notaryproject/notation-go/internal/file" "github.com/notaryproject/notation-go/plugin/proto" ) @@ -36,3 +38,14 @@ func isExecutableFile(filePath string) (bool, error) { } return strings.EqualFold(filepath.Ext(filepath.Base(filePath)), ".exe"), nil } + +// parsePluginName checks if fileName is a valid plugin file name +// and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation +func parsePluginName(fileName string) (string, error) { + fname := file.TrimFileExtension(fileName) + pluginName, found := strings.CutPrefix(fname, proto.Prefix) + if !found { + return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) + } + return pluginName, nil +} diff --git a/plugin/plugin.go b/plugin/plugin.go index 0afb9ea6..ef14c865 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -25,9 +25,7 @@ import ( "os" "os/exec" "path/filepath" - "strings" - "github.com/notaryproject/notation-go/internal/file" "github.com/notaryproject/notation-go/internal/slices" "github.com/notaryproject/notation-go/log" "github.com/notaryproject/notation-go/plugin/proto" @@ -227,14 +225,14 @@ func (c execCommander) Output(ctx context.Context, name string, command proto.Co // ParsePluginName checks if fileName is a valid plugin file name // and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation -func ParsePluginName(fileName string) (string, error) { - fname := file.TrimFileExtension(fileName) - pluginName, found := strings.CutPrefix(fname, proto.Prefix) - if !found { - return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) - } - return pluginName, nil -} +// func ParsePluginName(fileName string) (string, error) { +// fname := file.TrimFileExtension(fileName) +// pluginName, found := strings.CutPrefix(fname, proto.Prefix) +// if !found { +// return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) +// } +// return pluginName, nil +// } // validate checks if the metadata is correctly populated. func validate(metadata *proto.GetMetadataResponse) error { @@ -265,15 +263,15 @@ func validate(metadata *proto.GetMetadataResponse) error { return nil } -// validatePluginFileExtensionAgainstOS validates if plugin executable file -// extension aligns with the runtime OS. +// validatePluginFilenameAgainstOS validates if plugin executable file +// name aligns with the runtime OS. // // On windows, `.exe` extension is required. -// On other OS, MUST not have the `.exe` extension. -func validatePluginFileExtensionAgainstOS(fileName, pluginName string) error { +// On other OS, MUST NOT have the `.exe` extension. +func validatePluginFilenameAgainstOS(fileName, pluginName string) error { expectedPluginFile := binName(pluginName) - if filepath.Ext(fileName) != filepath.Ext(expectedPluginFile) { - return fmt.Errorf("invalid plugin file extension. Expecting file %s, but got %s", expectedPluginFile, fileName) + if fileName != expectedPluginFile { + return fmt.Errorf("invalid plugin file name. Expecting file %s, but got %s", expectedPluginFile, fileName) } return nil } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index bf592663..4542b372 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -273,7 +273,7 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { } func TestExtractPluginNameFromExecutableFileName(t *testing.T) { - pluginName, err := ParsePluginName("notation-my-plugin") + pluginName, err := parsePluginName("notation-my-plugin") if err != nil { t.Fatalf("expected nil err, got %v", err) } @@ -281,7 +281,7 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } - pluginName, err = ParsePluginName("notation-my-plugin.exe") + pluginName, err = parsePluginName("notation-my-plugin.exe") if err != nil { t.Fatalf("expected nil err, got %v", err) } @@ -289,13 +289,13 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } - _, err = ParsePluginName("myPlugin") + _, err = parsePluginName("myPlugin") expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) } - _, err = ParsePluginName("my-plugin") + _, err = parsePluginName("my-plugin") expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got my-plugin" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) @@ -304,24 +304,24 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { if runtime.GOOS == "windows" { - err := validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") + err := validatePluginFilenameAgainstOS("notation-foo.exe", "foo") if err != nil { t.Fatalf("expecting nil error, but got %s", err) } - err = validatePluginFileExtensionAgainstOS("notation-foo", "foo") + err = validatePluginFilenameAgainstOS("notation-foo", "foo") expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo.exe, but got notation-foo" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } return } - err := validatePluginFileExtensionAgainstOS("notation-foo", "foo") + err := validatePluginFilenameAgainstOS("notation-foo", "foo") if err != nil { t.Fatalf("expecting nil error, but got %s", err) } - err = validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") + err = validatePluginFilenameAgainstOS("notation-foo.exe", "foo") expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo, but got notation-foo.exe" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) From 190436cb6d6449b344ad78ab0c2add909791a3c0 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 16:57:20 +0800 Subject: [PATCH 02/31] fix Signed-off-by: Patrick Zheng --- plugin/plugin_test.go | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 4542b372..cf4a4827 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -272,7 +272,7 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { }) } -func TestExtractPluginNameFromExecutableFileName(t *testing.T) { +func TestParsePluginName(t *testing.T) { pluginName, err := parsePluginName("notation-my-plugin") if err != nil { t.Fatalf("expected nil err, got %v", err) @@ -281,14 +281,6 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } - pluginName, err = parsePluginName("notation-my-plugin.exe") - if err != nil { - t.Fatalf("expected nil err, got %v", err) - } - if pluginName != "my-plugin" { - t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) - } - _, err = parsePluginName("myPlugin") expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" if err == nil || err.Error() != expectedErrorMsg { @@ -300,6 +292,24 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) } + + if runtime.GOOS == "windows" { + pluginName, err = parsePluginName("notation-my-plugin.exe") + if err != nil { + t.Fatalf("expected nil err, got %v", err) + } + if pluginName != "my-plugin" { + t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) + } + } else { + pluginName, err = parsePluginName("notation-com.example.plugin") + if err != nil { + t.Fatalf("expected nil err, got %v", err) + } + if pluginName != "com.example.plugin" { + t.Fatalf("expected plugin name com.example.plugin, but got %s", pluginName) + } + } } func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { From 4123784659e09f703d36c66b1fa826f4b5414a26 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 17:00:10 +0800 Subject: [PATCH 03/31] fix Signed-off-by: Patrick Zheng --- plugin/manager_unix.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index b867ae1c..cedf8ee1 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -44,9 +44,9 @@ func isExecutableFile(filePath string) (bool, error) { // parsePluginName checks if fileName is a valid plugin file name // and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation func parsePluginName(fileName string) (string, error) { - pluginName, found := strings.CutPrefix(fname, proto.Prefix) + pluginName, found := strings.CutPrefix(fileName, proto.Prefix) if !found { - return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) + return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fileName) } return pluginName, nil } From a513dbc9d392de164281934cbdb0e0ce70a32271 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 17:41:42 +0800 Subject: [PATCH 04/31] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- plugin/manager_test.go | 68 ++++++++++++++++++++++++++++++++++++++- plugin/manager_unix.go | 14 ++++++++ plugin/manager_windows.go | 13 ++++++++ plugin/plugin.go | 24 -------------- plugin/plugin_test.go | 67 -------------------------------------- 6 files changed, 95 insertions(+), 93 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 6230e6f8..06445f1c 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -140,7 +140,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } // validate and get new plugin metadata - if err := validatePluginFilenameAgainstOS(filepath.Base(pluginExecutableFile), pluginName); err != nil { + if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginExecutableFile)); err != nil { return nil, nil, err } newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 8aa92f88..4c6acd68 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -313,7 +313,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "invalid plugin file extension. Expecting file notation-bar, but got notation-bar.exe" + expectedErrorMsg := "invalid plugin file extension. On OS other than windows, plugin executable file cannot have '.exe' file extension" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -517,6 +517,72 @@ func TestManager_Uninstall(t *testing.T) { } } +func TestParsePluginName(t *testing.T) { + pluginName, err := parsePluginName("notation-my-plugin") + if err != nil { + t.Fatalf("expected nil err, got %v", err) + } + if pluginName != "my-plugin" { + t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) + } + + _, err = parsePluginName("myPlugin") + expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, got %v", expectedErrorMsg, err) + } + + _, err = parsePluginName("my-plugin") + expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got my-plugin" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, got %v", expectedErrorMsg, err) + } + + if runtime.GOOS == "windows" { + pluginName, err = parsePluginName("notation-my-plugin.exe") + if err != nil { + t.Fatalf("expected nil err, got %v", err) + } + if pluginName != "my-plugin" { + t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) + } + } else { + pluginName, err = parsePluginName("notation-com.example.plugin") + if err != nil { + t.Fatalf("expected nil err, got %v", err) + } + if pluginName != "com.example.plugin" { + t.Fatalf("expected plugin name com.example.plugin, but got %s", pluginName) + } + } +} + +func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { + if runtime.GOOS == "windows" { + err := validatePluginFileExtensionAgainstOS("notation-foo.exe") + if err != nil { + t.Fatalf("expecting nil error, but got %s", err) + } + + err = validatePluginFileExtensionAgainstOS("notation-foo") + expectedErrorMsg := "invalid plugin file extension. On windows, plugin executable file MUST have '.exe' file extension" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + } else { + err := validatePluginFileExtensionAgainstOS("notation-foo") + if err != nil { + t.Fatalf("expecting nil error, but got %s", err) + } + + err = validatePluginFileExtensionAgainstOS("notation-foo.exe") + expectedErrorMsg := "invalid plugin file extension. On OS other than windows, plugin executable file cannot have '.exe' file extension" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + } +} + func metadataJSON(m proto.GetMetadataResponse) []byte { d, err := json.Marshal(m) if err != nil { diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index cedf8ee1..b9fd036d 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -17,8 +17,10 @@ package plugin import ( + "errors" "fmt" "os" + "path/filepath" "strings" "github.com/notaryproject/notation-go/plugin/proto" @@ -50,3 +52,15 @@ func parsePluginName(fileName string) (string, error) { } return pluginName, nil } + +// validatePluginFileExtensionAgainstOS validates if plugin executable file +// name aligns with the runtime OS. +// +// On windows, `.exe` extension is required. +// On other OS, MUST NOT have the `.exe` extension. +func validatePluginFileExtensionAgainstOS(fileName string) error { + if strings.EqualFold(filepath.Ext(fileName), ".exe") { + return errors.New("invalid plugin file extension. On OS other than windows, plugin executable file cannot have '.exe' file extension") + } + return nil +} diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 2bffbcf5..d47c5175 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -14,6 +14,7 @@ package plugin import ( + "errors" "fmt" "os" "path/filepath" @@ -49,3 +50,15 @@ func parsePluginName(fileName string) (string, error) { } return pluginName, nil } + +// validatePluginFileExtensionAgainstOS validates if plugin executable file +// name aligns with the runtime OS. +// +// On windows, `.exe` extension is required. +// On other OS, MUST NOT have the `.exe` extension. +func validatePluginFileExtensionAgainstOS(fileName string) error { + if !strings.EqualFold(filepath.Ext(fileName), ".exe") { + return errors.New("invalid plugin file extension. On windows, plugin executable file MUST have '.exe' file extension") + } + return nil +} diff --git a/plugin/plugin.go b/plugin/plugin.go index ef14c865..c9a77a7b 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -223,17 +223,6 @@ func (c execCommander) Output(ctx context.Context, name string, command proto.Co return stdout.Bytes(), nil, nil } -// ParsePluginName checks if fileName is a valid plugin file name -// and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation -// func ParsePluginName(fileName string) (string, error) { -// fname := file.TrimFileExtension(fileName) -// pluginName, found := strings.CutPrefix(fname, proto.Prefix) -// if !found { -// return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) -// } -// return pluginName, nil -// } - // validate checks if the metadata is correctly populated. func validate(metadata *proto.GetMetadataResponse) error { if metadata.Name == "" { @@ -262,16 +251,3 @@ func validate(metadata *proto.GetMetadataResponse) error { } return nil } - -// validatePluginFilenameAgainstOS validates if plugin executable file -// name aligns with the runtime OS. -// -// On windows, `.exe` extension is required. -// On other OS, MUST NOT have the `.exe` extension. -func validatePluginFilenameAgainstOS(fileName, pluginName string) error { - expectedPluginFile := binName(pluginName) - if fileName != expectedPluginFile { - return fmt.Errorf("invalid plugin file name. Expecting file %s, but got %s", expectedPluginFile, fileName) - } - return nil -} diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index cf4a4827..45a43fc4 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -20,7 +20,6 @@ import ( "fmt" "os" "reflect" - "runtime" "strconv" "strings" "testing" @@ -271,69 +270,3 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { } }) } - -func TestParsePluginName(t *testing.T) { - pluginName, err := parsePluginName("notation-my-plugin") - if err != nil { - t.Fatalf("expected nil err, got %v", err) - } - if pluginName != "my-plugin" { - t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) - } - - _, err = parsePluginName("myPlugin") - expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expected %s, got %v", expectedErrorMsg, err) - } - - _, err = parsePluginName("my-plugin") - expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got my-plugin" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expected %s, got %v", expectedErrorMsg, err) - } - - if runtime.GOOS == "windows" { - pluginName, err = parsePluginName("notation-my-plugin.exe") - if err != nil { - t.Fatalf("expected nil err, got %v", err) - } - if pluginName != "my-plugin" { - t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) - } - } else { - pluginName, err = parsePluginName("notation-com.example.plugin") - if err != nil { - t.Fatalf("expected nil err, got %v", err) - } - if pluginName != "com.example.plugin" { - t.Fatalf("expected plugin name com.example.plugin, but got %s", pluginName) - } - } -} - -func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { - if runtime.GOOS == "windows" { - err := validatePluginFilenameAgainstOS("notation-foo.exe", "foo") - if err != nil { - t.Fatalf("expecting nil error, but got %s", err) - } - - err = validatePluginFilenameAgainstOS("notation-foo", "foo") - expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo.exe, but got notation-foo" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) - } - return - } - err := validatePluginFilenameAgainstOS("notation-foo", "foo") - if err != nil { - t.Fatalf("expecting nil error, but got %s", err) - } - - err = validatePluginFilenameAgainstOS("notation-foo.exe", "foo") - expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo, but got notation-foo.exe" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) - } -} From c8ffdb091c9487918d79baf2ebfac751b6b088b7 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 17:54:02 +0800 Subject: [PATCH 05/31] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 8 ++++---- plugin/manager_test.go | 16 ++++++++-------- plugin/manager_unix.go | 2 +- plugin/manager_windows.go | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 06445f1c..16ee6d90 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -122,21 +122,21 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) pluginExecutableFile, pluginName, err := parsePluginFromDir(installOpts.PluginPath) if err != nil { if !errors.Is(err, file.ErrNotDirectory) { - return nil, nil, fmt.Errorf("failed to read plugin from directory %s: %w", installOpts.PluginPath, err) + return nil, nil, fmt.Errorf("failed to read plugin from input directory: %w", err) } // input is not a dir, check if it's a single plugin executable file installFromNonDir = true pluginExecutableFile = installOpts.PluginPath pluginName, err = parsePluginName(filepath.Base(pluginExecutableFile)) if err != nil { - return nil, nil, fmt.Errorf("failed to read plugin name from file path %s: %w", pluginExecutableFile, err) + return nil, nil, fmt.Errorf("failed to read plugin name from input file: %w", err) } isExec, err := isExecutableFile(pluginExecutableFile) if err != nil { - return nil, nil, fmt.Errorf("failed to check if file %s is executable: %w", pluginExecutableFile, err) + return nil, nil, fmt.Errorf("failed to check if input file is executable: %w", err) } if !isExec { - return nil, nil, fmt.Errorf("file %s is not executable", pluginExecutableFile) + return nil, nil, errors.New("input file is not executable") } } // validate and get new plugin metadata diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 4c6acd68..8e2a29e8 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -265,7 +265,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "failed to read plugin name from file path testdata/bar/bar: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" + expectedErrorMsg := "failed to read plugin name from file input file: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -289,7 +289,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "file testdata/bar/notation-bar is not executable" + expectedErrorMsg := "input file is not executable" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -313,7 +313,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "invalid plugin file extension. On OS other than windows, plugin executable file cannot have '.exe' file extension" + expectedErrorMsg := "invalid plugin file extension. On OS other than Windows, plugin executable file cannot have '.exe' file extension" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -329,7 +329,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "failed to read plugin from directory testdata/bar/notation-bar: stat testdata/bar/notation-bar: no such file or directory" + expectedErrorMsg := "failed to read plugin from input directory: stat testdata/bar/notation-bar: no such file or directory" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -450,7 +450,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "failed to read plugin from directory testdata/foo: no plugin executable file was found" + expectedErrorMsg := "failed to read plugin from input directory: no plugin executable file was found" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -481,7 +481,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "failed to read plugin from directory testdata/foo: found more than one plugin executable files" + expectedErrorMsg := "failed to read plugin from input directory: found more than one plugin executable files" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -565,7 +565,7 @@ func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { } err = validatePluginFileExtensionAgainstOS("notation-foo") - expectedErrorMsg := "invalid plugin file extension. On windows, plugin executable file MUST have '.exe' file extension" + expectedErrorMsg := "invalid plugin file extension. On Windows, plugin executable file MUST have '.exe' file extension" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -576,7 +576,7 @@ func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { } err = validatePluginFileExtensionAgainstOS("notation-foo.exe") - expectedErrorMsg := "invalid plugin file extension. On OS other than windows, plugin executable file cannot have '.exe' file extension" + expectedErrorMsg := "invalid plugin file extension. On OS other than Windows, plugin executable file cannot have '.exe' file extension" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index b9fd036d..3016c70c 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -60,7 +60,7 @@ func parsePluginName(fileName string) (string, error) { // On other OS, MUST NOT have the `.exe` extension. func validatePluginFileExtensionAgainstOS(fileName string) error { if strings.EqualFold(filepath.Ext(fileName), ".exe") { - return errors.New("invalid plugin file extension. On OS other than windows, plugin executable file cannot have '.exe' file extension") + return errors.New("invalid plugin file extension. On OS other than Windows, plugin executable file cannot have '.exe' file extension") } return nil } diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index d47c5175..7db5e9b9 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -58,7 +58,7 @@ func parsePluginName(fileName string) (string, error) { // On other OS, MUST NOT have the `.exe` extension. func validatePluginFileExtensionAgainstOS(fileName string) error { if !strings.EqualFold(filepath.Ext(fileName), ".exe") { - return errors.New("invalid plugin file extension. On windows, plugin executable file MUST have '.exe' file extension") + return errors.New("invalid plugin file extension. On Windows, plugin executable file MUST have '.exe' file extension") } return nil } From db54eb2aab8175989e01cd9cadcbcf5e9fa82750 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 17:57:50 +0800 Subject: [PATCH 06/31] fix test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 8e2a29e8..5e5f103c 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -265,7 +265,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "failed to read plugin name from file input file: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" + expectedErrorMsg := "failed to read plugin name from input file: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) From 8678aea6704f24da9e962f800094a7dfed6fc729 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 23:30:28 +0800 Subject: [PATCH 07/31] add test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 5e5f103c..644b995d 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -80,6 +80,11 @@ var validMetadataBar = proto.GetMetadataResponse{ SupportedContractVersions: []string{"1.0"}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, } +var validMetadataBarExample = proto.GetMetadataResponse{ + Name: "bar.example.plugin", Description: "friendly", Version: "1.0.0", URL: "example.com", + SupportedContractVersions: []string{"1.0"}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, +} + var invalidMetadataName = proto.GetMetadataResponse{ Name: "foobar", Description: "friendly", Version: "1", URL: "example.com", SupportedContractVersions: []string{"1.0"}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, @@ -214,6 +219,36 @@ func TestManager_Install(t *testing.T) { } }) + t.Run("success install with file extension", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notation-bar.example.plugin" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { + t.Fatal(err) + } + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(validMetadataBarExample), + } + defer mgr.Uninstall(context.Background(), "bar.example.plugin") + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), installOpts) + if err != nil { + t.Fatalf("expecting error to be nil, but got %v", err) + } + if existingPluginMetadata != nil { + t.Fatalf("expecting existingPluginMetadata to be nil, but got %v", existingPluginMetadata) + } + if newPluginMetadata.Version != validMetadataBar.Version { + t.Fatalf("new plugin version mismatch, new plugin version: %s, but got: %s", validMetadataBar.Version, newPluginMetadata.Version) + } + }) + t.Run("fail to install due to equal version", func(t *testing.T) { executor = testInstallCommander{ existedPluginFilePath: existedPluginFilePath, From 4e5e92e7cffcd969760111163bc5ebb5b4a905f1 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 5 Jan 2024 23:57:41 +0800 Subject: [PATCH 08/31] check if missing plugin name Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 24 ++++++++++++++++++++++++ plugin/manager_unix.go | 2 +- plugin/manager_windows.go | 2 +- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 644b995d..85beca79 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -307,6 +307,30 @@ func TestManager_Install(t *testing.T) { } }) + t.Run("fail to install due to plugin executable file name missing plugin name", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notaiton-" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { + t.Fatal(err) + } + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(validMetadataBar), + } + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got notation-" + _, _, err := mgr.Install(context.Background(), installOpts) + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + }) + t.Run("fail to install due to wrong plugin file permission", func(t *testing.T) { newPluginFilePath := "testdata/bar/notation-bar" newPluginDir := filepath.Dir(newPluginFilePath) diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index 3016c70c..e8aebb88 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -47,7 +47,7 @@ func isExecutableFile(filePath string) (bool, error) { // and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation func parsePluginName(fileName string) (string, error) { pluginName, found := strings.CutPrefix(fileName, proto.Prefix) - if !found { + if !found || pluginName == "" { return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fileName) } return pluginName, nil diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 7db5e9b9..3b09281a 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -45,7 +45,7 @@ func isExecutableFile(filePath string) (bool, error) { func parsePluginName(fileName string) (string, error) { fname := file.TrimFileExtension(fileName) pluginName, found := strings.CutPrefix(fname, proto.Prefix) - if !found { + if !found || pluginName == "" { return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) } return pluginName, nil From fd6a149030f9d53e2bd851b11b2a73b301599dbb Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Sat, 6 Jan 2024 00:00:34 +0800 Subject: [PATCH 09/31] fix test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 85beca79..8b5685fb 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -324,7 +324,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got notation-" + expectedErrorMsg := "failed to read plugin name from input file: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got notation-" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) From cbb7e25c896fa448111adbe99eda023134909d8c Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Sat, 6 Jan 2024 00:03:32 +0800 Subject: [PATCH 10/31] fix test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 8b5685fb..e24af2d7 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -308,7 +308,7 @@ func TestManager_Install(t *testing.T) { }) t.Run("fail to install due to plugin executable file name missing plugin name", func(t *testing.T) { - newPluginFilePath := "testdata/bar/notaiton-" + newPluginFilePath := "testdata/bar/notation-" newPluginDir := filepath.Dir(newPluginFilePath) if err := os.MkdirAll(newPluginDir, 0777); err != nil { t.Fatalf("failed to create %s: %v", newPluginDir, err) From f7c5cc9a0d8ca25807f98be16f8f07cff996ffd4 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 15:52:10 +0800 Subject: [PATCH 11/31] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 23 ++++++++++++++---- plugin/manager_test.go | 50 --------------------------------------- plugin/manager_unix.go | 14 ----------- plugin/manager_windows.go | 13 ---------- 4 files changed, 18 insertions(+), 82 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 16ee6d90..929d14ea 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -140,9 +140,6 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } // validate and get new plugin metadata - if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginExecutableFile)); err != nil { - return nil, nil, err - } newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) @@ -215,8 +212,9 @@ func (m *CLIManager) Uninstall(ctx context.Context, name string) error { } // parsePluginFromDir checks if a dir is a valid plugin dir which contains -// one and only one plugin executable file. The dir may contain extra lib files -// and LICENSE files. Sub-directories are ignored. +// one and only one plugin executable file candidate. +// The dir may contain extra lib files and LICENSE files. +// Sub-directories are ignored. // // On success, the plugin executable file path, plugin name and // nil error are returned. @@ -232,6 +230,7 @@ func parsePluginFromDir(path string) (string, string, error) { // walk the path var pluginExecutableFile, pluginName string var foundPluginExecutableFile bool + var filesWithValidNameFormat []string if err := filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { if err != nil { return err @@ -251,6 +250,7 @@ func parsePluginFromDir(path string) (string, string, error) { // continue return nil } + filesWithValidNameFormat = append(filesWithValidNameFormat, p) isExec, err := isExecutableFile(p) if err != nil { return err @@ -269,6 +269,19 @@ func parsePluginFromDir(path string) (string, string, error) { return "", "", err } if !foundPluginExecutableFile { + // if no executable file was found, but there's one and only one + // potential candidate, try install the candidate + if len(filesWithValidNameFormat) == 1 { + candidate := filesWithValidNameFormat[0] + candidateInfo, err := os.Stat(candidate) + if err != nil { + return "", "", fmt.Errorf("no plugin executable file was found: %w", err) + } + if err := os.Chmod(candidate, candidateInfo.Mode()|os.FileMode(0100)); err != nil { + return "", "", fmt.Errorf("no plugin executable file was found: %w", err) + } + return candidate, pluginName, nil + } return "", "", errors.New("no plugin executable file was found") } return pluginExecutableFile, pluginName, nil diff --git a/plugin/manager_test.go b/plugin/manager_test.go index e24af2d7..dc85d5fe 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -355,30 +355,6 @@ func TestManager_Install(t *testing.T) { } }) - t.Run("fail to install due to invalid new plugin file extension", func(t *testing.T) { - newPluginFilePath := "testdata/bar/notation-bar.exe" - newPluginDir := filepath.Dir(newPluginFilePath) - if err := os.MkdirAll(newPluginDir, 0777); err != nil { - t.Fatalf("failed to create %s: %v", newPluginDir, err) - } - defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { - t.Fatal(err) - } - executor = testInstallCommander{ - newPluginFilePath: newPluginFilePath, - newPluginStdout: metadataJSON(validMetadataBar), - } - installOpts := CLIInstallOptions{ - PluginPath: newPluginFilePath, - } - expectedErrorMsg := "invalid plugin file extension. On OS other than Windows, plugin executable file cannot have '.exe' file extension" - _, _, err := mgr.Install(context.Background(), installOpts) - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) - } - }) - t.Run("fail to install due to new plugin executable file does not exist", func(t *testing.T) { newPluginFilePath := "testdata/bar/notation-bar" executor = testInstallCommander{ @@ -616,32 +592,6 @@ func TestParsePluginName(t *testing.T) { } } -func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { - if runtime.GOOS == "windows" { - err := validatePluginFileExtensionAgainstOS("notation-foo.exe") - if err != nil { - t.Fatalf("expecting nil error, but got %s", err) - } - - err = validatePluginFileExtensionAgainstOS("notation-foo") - expectedErrorMsg := "invalid plugin file extension. On Windows, plugin executable file MUST have '.exe' file extension" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) - } - } else { - err := validatePluginFileExtensionAgainstOS("notation-foo") - if err != nil { - t.Fatalf("expecting nil error, but got %s", err) - } - - err = validatePluginFileExtensionAgainstOS("notation-foo.exe") - expectedErrorMsg := "invalid plugin file extension. On OS other than Windows, plugin executable file cannot have '.exe' file extension" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) - } - } -} - func metadataJSON(m proto.GetMetadataResponse) []byte { d, err := json.Marshal(m) if err != nil { diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index e8aebb88..8796af1a 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -17,10 +17,8 @@ package plugin import ( - "errors" "fmt" "os" - "path/filepath" "strings" "github.com/notaryproject/notation-go/plugin/proto" @@ -52,15 +50,3 @@ func parsePluginName(fileName string) (string, error) { } return pluginName, nil } - -// validatePluginFileExtensionAgainstOS validates if plugin executable file -// name aligns with the runtime OS. -// -// On windows, `.exe` extension is required. -// On other OS, MUST NOT have the `.exe` extension. -func validatePluginFileExtensionAgainstOS(fileName string) error { - if strings.EqualFold(filepath.Ext(fileName), ".exe") { - return errors.New("invalid plugin file extension. On OS other than Windows, plugin executable file cannot have '.exe' file extension") - } - return nil -} diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 3b09281a..0c8052ef 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -14,7 +14,6 @@ package plugin import ( - "errors" "fmt" "os" "path/filepath" @@ -50,15 +49,3 @@ func parsePluginName(fileName string) (string, error) { } return pluginName, nil } - -// validatePluginFileExtensionAgainstOS validates if plugin executable file -// name aligns with the runtime OS. -// -// On windows, `.exe` extension is required. -// On other OS, MUST NOT have the `.exe` extension. -func validatePluginFileExtensionAgainstOS(fileName string) error { - if !strings.EqualFold(filepath.Ext(fileName), ".exe") { - return errors.New("invalid plugin file extension. On Windows, plugin executable file MUST have '.exe' file extension") - } - return nil -} From 95b7ead476930f5b570bfe4b1019b262d2c53012 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 15:58:02 +0800 Subject: [PATCH 12/31] add tests Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index dc85d5fe..86d38f25 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -432,7 +432,7 @@ func TestManager_Install(t *testing.T) { t.Run("success to install from plugin dir", func(t *testing.T) { existedPluginFilePath := "testdata/plugins/foo/notation-foo" newPluginFilePath := "testdata/foo/notation-foo" - newPluginLibPath := "testdata/foo/libfoo" + newPluginLibPath := "testdata/foo/notation-libfoo" newPluginDir := filepath.Dir(newPluginFilePath) if err := os.MkdirAll(newPluginDir, 0777); err != nil { t.Fatalf("failed to create %s: %v", newPluginDir, err) @@ -465,6 +465,42 @@ func TestManager_Install(t *testing.T) { } }) + t.Run("success to install from plugin dir with one and only one valid candidate and no executable file", func(t *testing.T) { + existedPluginFilePath := "testdata/plugins/foo/notation-foo" + newPluginFilePath := "testdata/foo/notation-foo" + newPluginLibPath := "testdata/foo/libfoo" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + if err := createFileAndChmod(newPluginFilePath, 0600); err != nil { + t.Fatal(err) + } + if err := createFileAndChmod(newPluginLibPath, 0600); err != nil { + t.Fatal(err) + } + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadataHigherVersion), + } + installOpts := CLIInstallOptions{ + PluginPath: newPluginDir, + } + existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), installOpts) + if err != nil { + t.Fatalf("expecting nil error, but got %v", err) + } + if existingPluginMetadata.Version != "1.0.0" { + t.Fatalf("expecting existing plugin metadata to be 1.0.0, but got %s", existingPluginMetadata.Version) + } + if newPluginMetadata.Version != "1.1.0" { + t.Fatalf("expecting new plugin metadata to be 1.1.0, but got %s", newPluginMetadata.Version) + } + }) + t.Run("fail to install from plugin dir due to no plugin executable file", func(t *testing.T) { existedPluginFilePath := "testdata/plugins/foo/notation-foo" newPluginFilePath := "testdata/foo/foo" From c8abe7ea5a5b2b8244481d980189ee282935172d Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 16:28:45 +0800 Subject: [PATCH 13/31] update Signed-off-by: Patrick Zheng --- plugin/manager_unix.go | 1 + plugin/testdata/plugins/foo/libfoo | 0 2 files changed, 1 insertion(+) delete mode 100644 plugin/testdata/plugins/foo/libfoo diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index 8796af1a..fc43bacd 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -38,6 +38,7 @@ func isExecutableFile(filePath string) (bool, error) { if !mode.IsRegular() { return false, ErrNotRegularFile } + fmt.Println(fi.Name(), mode.Perm()) return mode.Perm()&0100 != 0, nil } diff --git a/plugin/testdata/plugins/foo/libfoo b/plugin/testdata/plugins/foo/libfoo deleted file mode 100644 index e69de29b..00000000 From 6ee86e04b888055221e75f436856b177cd51b53e Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 16:34:52 +0800 Subject: [PATCH 14/31] test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 2 +- plugin/plugin.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 86d38f25..38a21679 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -432,7 +432,7 @@ func TestManager_Install(t *testing.T) { t.Run("success to install from plugin dir", func(t *testing.T) { existedPluginFilePath := "testdata/plugins/foo/notation-foo" newPluginFilePath := "testdata/foo/notation-foo" - newPluginLibPath := "testdata/foo/notation-libfoo" + newPluginLibPath := "testdata/foo/libfoo" newPluginDir := filepath.Dir(newPluginFilePath) if err := os.MkdirAll(newPluginDir, 0777); err != nil { t.Fatalf("failed to create %s: %v", newPluginDir, err) diff --git a/plugin/plugin.go b/plugin/plugin.go index c9a77a7b..0a62beb6 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -107,6 +107,7 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque if err = validate(&metadata); err != nil { return nil, fmt.Errorf("invalid metadata: %w", err) } + fmt.Println(metadata.Name, p.name) if metadata.Name != p.name { return nil, fmt.Errorf("executable name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path)) } From c69508043770c65709fcd7aa1266baddf66dcf02 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 16:37:46 +0800 Subject: [PATCH 15/31] test Signed-off-by: Patrick Zheng --- plugin/manager_unix.go | 1 - plugin/plugin.go | 1 - 2 files changed, 2 deletions(-) diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index fc43bacd..8796af1a 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -38,7 +38,6 @@ func isExecutableFile(filePath string) (bool, error) { if !mode.IsRegular() { return false, ErrNotRegularFile } - fmt.Println(fi.Name(), mode.Perm()) return mode.Perm()&0100 != 0, nil } diff --git a/plugin/plugin.go b/plugin/plugin.go index 0a62beb6..c9a77a7b 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -107,7 +107,6 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque if err = validate(&metadata); err != nil { return nil, fmt.Errorf("invalid metadata: %w", err) } - fmt.Println(metadata.Name, p.name) if metadata.Name != p.name { return nil, fmt.Errorf("executable name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path)) } From 1e32eda19db01538ee99f90392ac951941a3223e Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 16:41:56 +0800 Subject: [PATCH 16/31] test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 38a21679..86d38f25 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -432,7 +432,7 @@ func TestManager_Install(t *testing.T) { t.Run("success to install from plugin dir", func(t *testing.T) { existedPluginFilePath := "testdata/plugins/foo/notation-foo" newPluginFilePath := "testdata/foo/notation-foo" - newPluginLibPath := "testdata/foo/libfoo" + newPluginLibPath := "testdata/foo/notation-libfoo" newPluginDir := filepath.Dir(newPluginFilePath) if err := os.MkdirAll(newPluginDir, 0777); err != nil { t.Fatalf("failed to create %s: %v", newPluginDir, err) From dc04d9075ea37f5f99dc6cf64600f81b6d1c4fb1 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 16:46:42 +0800 Subject: [PATCH 17/31] test Signed-off-by: Patrick Zheng --- plugin/plugin.go | 1 + 1 file changed, 1 insertion(+) diff --git a/plugin/plugin.go b/plugin/plugin.go index c9a77a7b..0a62beb6 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -107,6 +107,7 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque if err = validate(&metadata); err != nil { return nil, fmt.Errorf("invalid metadata: %w", err) } + fmt.Println(metadata.Name, p.name) if metadata.Name != p.name { return nil, fmt.Errorf("executable name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path)) } From 733d8d223d4507fcf457d9572e5bb937835bb0e9 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 16:50:42 +0800 Subject: [PATCH 18/31] test Signed-off-by: Patrick Zheng --- plugin/manager.go | 1 + 1 file changed, 1 insertion(+) diff --git a/plugin/manager.go b/plugin/manager.go index 929d14ea..5ed0373f 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -140,6 +140,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } // validate and get new plugin metadata + fmt.Println(pluginExecutableFile, pluginName) newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) From 109b652c7670c66b2de0b8c1d9ceafc0a818cab7 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 16:59:08 +0800 Subject: [PATCH 19/31] test Signed-off-by: Patrick Zheng --- plugin/manager.go | 1 + 1 file changed, 1 insertion(+) diff --git a/plugin/manager.go b/plugin/manager.go index 5ed0373f..53777d80 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -270,6 +270,7 @@ func parsePluginFromDir(path string) (string, string, error) { return "", "", err } if !foundPluginExecutableFile { + fmt.Println("no plugin executable file", filesWithValidNameFormat) // if no executable file was found, but there's one and only one // potential candidate, try install the candidate if len(filesWithValidNameFormat) == 1 { From 8f3ab4baac3a87fc3c7da0f1ca519375ea7a8ca2 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 17:07:27 +0800 Subject: [PATCH 20/31] fix Signed-off-by: Patrick Zheng --- plugin/manager.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 53777d80..7b4adfe1 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -246,7 +246,8 @@ func parsePluginFromDir(path string) (string, string, error) { } // only take regular files if info.Mode().IsRegular() { - if pluginName, err = parsePluginName(d.Name()); err != nil { + pName, err := parsePluginName(d.Name()) + if err != nil { // file name does not follow the notation-{plugin-name} format, // continue return nil @@ -264,13 +265,13 @@ func parsePluginFromDir(path string) (string, string, error) { } foundPluginExecutableFile = true pluginExecutableFile = p + pluginName = pName } return nil }); err != nil { return "", "", err } if !foundPluginExecutableFile { - fmt.Println("no plugin executable file", filesWithValidNameFormat) // if no executable file was found, but there's one and only one // potential candidate, try install the candidate if len(filesWithValidNameFormat) == 1 { From 43fee3f02bcc866ad6af48d22208532c8ce84d12 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 20:19:07 +0800 Subject: [PATCH 21/31] fix Signed-off-by: Patrick Zheng --- plugin/manager.go | 16 +++++++++------- plugin/manager_test.go | 12 ++++++++---- plugin/plugin.go | 1 - 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 7b4adfe1..be3706be 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -25,6 +25,7 @@ import ( "github.com/notaryproject/notation-go/dir" "github.com/notaryproject/notation-go/internal/file" "github.com/notaryproject/notation-go/internal/semver" + "github.com/notaryproject/notation-go/log" "github.com/notaryproject/notation-go/plugin/proto" ) @@ -119,7 +120,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, errors.New("plugin source path cannot be empty") } var installFromNonDir bool - pluginExecutableFile, pluginName, err := parsePluginFromDir(installOpts.PluginPath) + pluginExecutableFile, pluginName, err := parsePluginFromDir(ctx, installOpts.PluginPath) if err != nil { if !errors.Is(err, file.ErrNotDirectory) { return nil, nil, fmt.Errorf("failed to read plugin from input directory: %w", err) @@ -140,7 +141,6 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } // validate and get new plugin metadata - fmt.Println(pluginExecutableFile, pluginName) newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) @@ -219,7 +219,7 @@ func (m *CLIManager) Uninstall(ctx context.Context, name string) error { // // On success, the plugin executable file path, plugin name and // nil error are returned. -func parsePluginFromDir(path string) (string, string, error) { +func parsePluginFromDir(ctx context.Context, path string) (string, string, error) { // sanity check fi, err := os.Stat(path) if err != nil { @@ -228,8 +228,9 @@ func parsePluginFromDir(path string) (string, string, error) { if !fi.Mode().IsDir() { return "", "", file.ErrNotDirectory } + logger := log.GetLogger(ctx) // walk the path - var pluginExecutableFile, pluginName string + var pluginExecutableFile, pluginName, candidatePluginName string var foundPluginExecutableFile bool var filesWithValidNameFormat []string if err := filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { @@ -246,7 +247,7 @@ func parsePluginFromDir(path string) (string, string, error) { } // only take regular files if info.Mode().IsRegular() { - pName, err := parsePluginName(d.Name()) + candidatePluginName, err := parsePluginName(d.Name()) if err != nil { // file name does not follow the notation-{plugin-name} format, // continue @@ -265,7 +266,7 @@ func parsePluginFromDir(path string) (string, string, error) { } foundPluginExecutableFile = true pluginExecutableFile = p - pluginName = pName + pluginName = candidatePluginName } return nil }); err != nil { @@ -283,7 +284,8 @@ func parsePluginFromDir(path string) (string, string, error) { if err := os.Chmod(candidate, candidateInfo.Mode()|os.FileMode(0100)); err != nil { return "", "", fmt.Errorf("no plugin executable file was found: %w", err) } - return candidate, pluginName, nil + logger.Warnf("Found candidate plugin executable file %q without executable permission. Setting user executable bit and try install.", candidate) + return candidate, candidatePluginName, nil } return "", "", errors.New("no plugin executable file was found") } diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 86d38f25..c08dae18 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -465,7 +465,7 @@ func TestManager_Install(t *testing.T) { } }) - t.Run("success to install from plugin dir with one and only one valid candidate and no executable file", func(t *testing.T) { + t.Run("success to install from plugin dir with no executable file and one valid candidate file", func(t *testing.T) { existedPluginFilePath := "testdata/plugins/foo/notation-foo" newPluginFilePath := "testdata/foo/notation-foo" newPluginLibPath := "testdata/foo/libfoo" @@ -501,15 +501,19 @@ func TestManager_Install(t *testing.T) { } }) - t.Run("fail to install from plugin dir due to no plugin executable file", func(t *testing.T) { + t.Run("fail to install from plugin dir due to more than one candidate plugin executable files", func(t *testing.T) { existedPluginFilePath := "testdata/plugins/foo/notation-foo" - newPluginFilePath := "testdata/foo/foo" + newPluginFilePath := "testdata/foo/notation-foo1" + newPluginFilePath2 := "testdata/foo/notation-foo2" newPluginDir := filepath.Dir(newPluginFilePath) if err := os.MkdirAll(newPluginDir, 0777); err != nil { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { + if err := createFileAndChmod(newPluginFilePath, 0600); err != nil { + t.Fatal(err) + } + if err := createFileAndChmod(newPluginFilePath2, 0600); err != nil { t.Fatal(err) } executor = testInstallCommander{ diff --git a/plugin/plugin.go b/plugin/plugin.go index 0a62beb6..c9a77a7b 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -107,7 +107,6 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque if err = validate(&metadata); err != nil { return nil, fmt.Errorf("invalid metadata: %w", err) } - fmt.Println(metadata.Name, p.name) if metadata.Name != p.name { return nil, fmt.Errorf("executable name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path)) } From 73cbc83f3604a4647ad3390d9361bbebf5a05dc5 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 20:28:22 +0800 Subject: [PATCH 22/31] fix Signed-off-by: Patrick Zheng --- plugin/manager.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index be3706be..9d4001e2 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -247,8 +247,7 @@ func parsePluginFromDir(ctx context.Context, path string) (string, string, error } // only take regular files if info.Mode().IsRegular() { - candidatePluginName, err := parsePluginName(d.Name()) - if err != nil { + if candidatePluginName, err = parsePluginName(d.Name()); err != nil { // file name does not follow the notation-{plugin-name} format, // continue return nil From afb5b93cb7fa819cb15a2c78e5162804555a7ff8 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 21:21:06 +0800 Subject: [PATCH 23/31] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 6 +----- plugin/manager_unix.go | 9 +++++++++ plugin/manager_windows.go | 7 +++++++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 9d4001e2..37ac2c67 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -276,11 +276,7 @@ func parsePluginFromDir(ctx context.Context, path string) (string, string, error // potential candidate, try install the candidate if len(filesWithValidNameFormat) == 1 { candidate := filesWithValidNameFormat[0] - candidateInfo, err := os.Stat(candidate) - if err != nil { - return "", "", fmt.Errorf("no plugin executable file was found: %w", err) - } - if err := os.Chmod(candidate, candidateInfo.Mode()|os.FileMode(0100)); err != nil { + if err := setExecutable(candidate); err != nil { return "", "", fmt.Errorf("no plugin executable file was found: %w", err) } logger.Warnf("Found candidate plugin executable file %q without executable permission. Setting user executable bit and try install.", candidate) diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index 8796af1a..579926d3 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -50,3 +50,12 @@ func parsePluginName(fileName string) (string, error) { } return pluginName, nil } + +// setExecutable sets file to be user executable +func setExecutable(filePath string) error { + fileInfo, err := os.Stat(filePath) + if err != nil { + return err + } + return os.Chmod(filePath, fileInfo.Mode()|os.FileMode(0100)) +} diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 0c8052ef..410acbfc 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -49,3 +49,10 @@ func parsePluginName(fileName string) (string, error) { } return pluginName, nil } + +// setExecutable sets file to be executable by adding '.exe' file extension +func setExecutable(filePath string) error { + fileDir := filepath.Dir(filePath) + fileName := filepath.Base(filePath) + return os.Rename(filePath, filepath.Join(fileDir, fileName+".exe")) +} From 938b39808102d121ad0f15506ead7a092926a7cc Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 21:33:08 +0800 Subject: [PATCH 24/31] update Signed-off-by: Patrick Zheng --- plugin/manager_windows.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 410acbfc..458e9c60 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -50,9 +50,8 @@ func parsePluginName(fileName string) (string, error) { return pluginName, nil } -// setExecutable sets file to be executable by adding '.exe' file extension +// setExecutable returns error on Windows. User needs to install the correct +// plugin file. func setExecutable(filePath string) error { - fileDir := filepath.Dir(filePath) - fileName := filepath.Base(filePath) - return os.Rename(filePath, filepath.Join(fileDir, fileName+".exe")) + return fmt.Errorf("on Windows, plugin executable file must have file extension '.exe', but got %s", filePath) } From 8a2b3c206f64e8f819c0e7cc0883c824cc8f49d5 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 21:38:23 +0800 Subject: [PATCH 25/31] update Signed-off-by: Patrick Zheng --- plugin/manager_windows.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 458e9c60..38b0aaa1 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -53,5 +53,5 @@ func parsePluginName(fileName string) (string, error) { // setExecutable returns error on Windows. User needs to install the correct // plugin file. func setExecutable(filePath string) error { - return fmt.Errorf("on Windows, plugin executable file must have file extension '.exe', but got %s", filePath) + return fmt.Errorf("on Windows, plugin executable file must have file extension '.exe', but got %s", filepath.Base(filePath)) } From a733289d8ce747adbf9298e50047b60c7af0bd1b Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 8 Jan 2024 21:43:23 +0800 Subject: [PATCH 26/31] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager.go b/plugin/manager.go index 37ac2c67..74b0a26c 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -279,7 +279,7 @@ func parsePluginFromDir(ctx context.Context, path string) (string, string, error if err := setExecutable(candidate); err != nil { return "", "", fmt.Errorf("no plugin executable file was found: %w", err) } - logger.Warnf("Found candidate plugin executable file %q without executable permission. Setting user executable bit and try install.", candidate) + logger.Warnf("Found candidate plugin executable file %q without executable permission. Setting user executable bit and try install.", filepath.Base(candidate)) return candidate, candidatePluginName, nil } return "", "", errors.New("no plugin executable file was found") From 48f0fc24fc48e0b36e9f3f32c9b00264603f2254 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 10 Jan 2024 11:11:02 +0800 Subject: [PATCH 27/31] update per code review Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- plugin/manager_test.go | 56 ++++++++++++++++++++++++++++----------- plugin/manager_windows.go | 9 ++++--- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 74b0a26c..3ffbf0df 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -279,7 +279,7 @@ func parsePluginFromDir(ctx context.Context, path string) (string, string, error if err := setExecutable(candidate); err != nil { return "", "", fmt.Errorf("no plugin executable file was found: %w", err) } - logger.Warnf("Found candidate plugin executable file %q without executable permission. Setting user executable bit and try install.", filepath.Base(candidate)) + logger.Warnf("Found candidate plugin executable file %q without executable permission. Setting user executable bit and trying to install.", filepath.Base(candidate)) return candidate, candidatePluginName, nil } return "", "", errors.New("no plugin executable file was found") diff --git a/plugin/manager_test.go b/plugin/manager_test.go index c08dae18..7f841974 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -595,40 +595,66 @@ func TestManager_Uninstall(t *testing.T) { func TestParsePluginName(t *testing.T) { pluginName, err := parsePluginName("notation-my-plugin") if err != nil { - t.Fatalf("expected nil err, got %v", err) + t.Fatalf("expected nil err, but got %v", err) } if pluginName != "my-plugin" { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } - _, err = parsePluginName("myPlugin") - expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expected %s, got %v", expectedErrorMsg, err) - } - - _, err = parsePluginName("my-plugin") - expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got my-plugin" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expected %s, got %v", expectedErrorMsg, err) - } - if runtime.GOOS == "windows" { pluginName, err = parsePluginName("notation-my-plugin.exe") if err != nil { - t.Fatalf("expected nil err, got %v", err) + t.Fatalf("expected nil err, but got %v", err) } if pluginName != "my-plugin" { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } + + pluginName, err = parsePluginName("notation-com.plugin") + if err != nil { + t.Fatalf("expected nil err, but got %v", err) + } + if pluginName != "com.plugin" { + t.Fatalf("expected plugin name com.plugin, but got %s", pluginName) + } + + expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got my-plugin.exe" + _, err = parsePluginName("my-plugin.exe") + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) + } + + expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got notation-.exe" + _, err = parsePluginName("notation-.exe") + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) + } } else { pluginName, err = parsePluginName("notation-com.example.plugin") if err != nil { - t.Fatalf("expected nil err, got %v", err) + t.Fatalf("expected nil err, but got %v", err) } if pluginName != "com.example.plugin" { t.Fatalf("expected plugin name com.example.plugin, but got %s", pluginName) } + + expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" + _, err = parsePluginName("myPlugin") + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) + } + + expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got my-plugin" + _, err = parsePluginName("my-plugin") + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) + } + + expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got notation-" + _, err = parsePluginName("notation-") + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) + } } } diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 38b0aaa1..457c22c8 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -42,10 +42,13 @@ func isExecutableFile(filePath string) (bool, error) { // parsePluginName checks if fileName is a valid plugin file name // and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation func parsePluginName(fileName string) (string, error) { - fname := file.TrimFileExtension(fileName) + fname := fileName + if strings.EqualFold(filepath.Ext(fileName), ".exe") { + fname = file.TrimFileExtension(fileName) + } pluginName, found := strings.CutPrefix(fname, proto.Prefix) if !found || pluginName == "" { - return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) + return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got %s", fileName) } return pluginName, nil } @@ -53,5 +56,5 @@ func parsePluginName(fileName string) (string, error) { // setExecutable returns error on Windows. User needs to install the correct // plugin file. func setExecutable(filePath string) error { - return fmt.Errorf("on Windows, plugin executable file must have file extension '.exe', but got %s", filepath.Base(filePath)) + return fmt.Errorf(`plugin executable file must have file extension ".exe", but got %q`, filepath.Base(filePath)) } From 48ff47ff8c3fd4025a2029634c4f4ce3d16f9222 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 10 Jan 2024 11:14:10 +0800 Subject: [PATCH 28/31] add test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 7f841974..899df272 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -629,6 +629,12 @@ func TestParsePluginName(t *testing.T) { if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) } + + expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got my-plugin" + _, err = parsePluginName("my-plugin") + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) + } } else { pluginName, err = parsePluginName("notation-com.example.plugin") if err != nil { From 4d40f8160dfe6fdb1f55939167fdfad278255ff7 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 10 Jan 2024 13:02:13 +0800 Subject: [PATCH 29/31] update Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 30 ++++++++++++++---------------- plugin/manager_windows.go | 6 +++--- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 899df272..54353983 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -593,16 +593,8 @@ func TestManager_Uninstall(t *testing.T) { } func TestParsePluginName(t *testing.T) { - pluginName, err := parsePluginName("notation-my-plugin") - if err != nil { - t.Fatalf("expected nil err, but got %v", err) - } - if pluginName != "my-plugin" { - t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) - } - if runtime.GOOS == "windows" { - pluginName, err = parsePluginName("notation-my-plugin.exe") + pluginName, err := parsePluginName("notation-my-plugin.exe") if err != nil { t.Fatalf("expected nil err, but got %v", err) } @@ -610,15 +602,13 @@ func TestParsePluginName(t *testing.T) { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } - pluginName, err = parsePluginName("notation-com.plugin") - if err != nil { - t.Fatalf("expected nil err, but got %v", err) - } - if pluginName != "com.plugin" { - t.Fatalf("expected plugin name com.plugin, but got %s", pluginName) + expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got notation-com.plugin" + _, err = parsePluginName("notation-com.plugin") + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) } - expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got my-plugin.exe" + expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got my-plugin.exe" _, err = parsePluginName("my-plugin.exe") if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) @@ -636,6 +626,14 @@ func TestParsePluginName(t *testing.T) { t.Fatalf("expected %s, but got %v", expectedErrorMsg, err) } } else { + pluginName, err := parsePluginName("notation-my-plugin") + if err != nil { + t.Fatalf("expected nil err, but got %v", err) + } + if pluginName != "my-plugin" { + t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) + } + pluginName, err = parsePluginName("notation-com.example.plugin") if err != nil { t.Fatalf("expected nil err, but got %v", err) diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 457c22c8..26d3e11f 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -42,10 +42,10 @@ func isExecutableFile(filePath string) (bool, error) { // parsePluginName checks if fileName is a valid plugin file name // and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation func parsePluginName(fileName string) (string, error) { - fname := fileName - if strings.EqualFold(filepath.Ext(fileName), ".exe") { - fname = file.TrimFileExtension(fileName) + if !strings.EqualFold(filepath.Ext(fileName), ".exe") { + return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got %s", fileName) } + fname := file.TrimFileExtension(fileName) pluginName, found := strings.CutPrefix(fname, proto.Prefix) if !found || pluginName == "" { return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}.exe, but got %s", fileName) From cb7e6b2f0a9c10417acbdac3d77e900d12734353 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 12 Jan 2024 08:47:29 +0800 Subject: [PATCH 30/31] update per code review Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugin/manager.go b/plugin/manager.go index 3ffbf0df..37ff2af1 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -115,10 +115,12 @@ type CLIInstallOptions struct { // plugin is malfunctioning, it will be overwritten. func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) { // initialization + logger := log.GetLogger(ctx) overwrite := installOpts.Overwrite if installOpts.PluginPath == "" { return nil, nil, errors.New("plugin source path cannot be empty") } + logger.Debugf("Installing plugin from plugin path %s", installOpts.PluginPath) var installFromNonDir bool pluginExecutableFile, pluginName, err := parsePluginFromDir(ctx, installOpts.PluginPath) if err != nil { From 3d8621dbf47107d04186ebf956b0b837783834c7 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 12 Jan 2024 09:14:42 +0800 Subject: [PATCH 31/31] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 9 +++++---- plugin/manager_test.go | 6 +++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 37ff2af1..f43907cb 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -130,16 +130,17 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) // input is not a dir, check if it's a single plugin executable file installFromNonDir = true pluginExecutableFile = installOpts.PluginPath - pluginName, err = parsePluginName(filepath.Base(pluginExecutableFile)) + pluginExecutableFileName := filepath.Base(pluginExecutableFile) + pluginName, err = parsePluginName(pluginExecutableFileName) if err != nil { - return nil, nil, fmt.Errorf("failed to read plugin name from input file: %w", err) + return nil, nil, fmt.Errorf("failed to read plugin name from input file %s: %w", pluginExecutableFileName, err) } isExec, err := isExecutableFile(pluginExecutableFile) if err != nil { - return nil, nil, fmt.Errorf("failed to check if input file is executable: %w", err) + return nil, nil, fmt.Errorf("failed to check if input file %s is executable: %w", pluginExecutableFileName, err) } if !isExec { - return nil, nil, errors.New("input file is not executable") + return nil, nil, fmt.Errorf("input file %s is not executable", pluginExecutableFileName) } } // validate and get new plugin metadata diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 54353983..f0937cd6 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -300,7 +300,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "failed to read plugin name from input file: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" + expectedErrorMsg := "failed to read plugin name from input file bar: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -324,7 +324,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "failed to read plugin name from input file: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got notation-" + expectedErrorMsg := "failed to read plugin name from input file notation-: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got notation-" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -348,7 +348,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "input file is not executable" + expectedErrorMsg := "input file notation-bar is not executable" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)