From 7d99d8fba7c82b3935f36e8b4118719d72393dbc Mon Sep 17 00:00:00 2001 From: Shobhit Gupta Date: Fri, 17 Jan 2020 12:08:55 -0800 Subject: [PATCH] Add IsValid method to *Manifest struct (#13609) --- app/plugin_install.go | 5 +- model/manifest.go | 112 ++++++++++++- model/manifest_test.go | 156 ++++++++++++++++++ plugin/valid.go => model/plugin_valid.go | 6 +- .../plugin_valid_test.go | 8 +- 5 files changed, 275 insertions(+), 12 deletions(-) rename plugin/valid.go => model/plugin_valid.go (82%) rename plugin/valid_test.go => model/plugin_valid_test.go (85%) diff --git a/app/plugin_install.go b/app/plugin_install.go index 8a55fa538c..413f97c4a8 100644 --- a/app/plugin_install.go +++ b/app/plugin_install.go @@ -50,7 +50,6 @@ import ( "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" - "github.com/mattermost/mattermost-server/v5/plugin" "github.com/mattermost/mattermost-server/v5/services/filesstore" "github.com/mattermost/mattermost-server/v5/utils" ) @@ -290,8 +289,8 @@ func extractPlugin(pluginFile io.ReadSeeker, extractDir string) (*model.Manifest return nil, "", model.NewAppError("extractPlugin", "app.plugin.manifest.app_error", nil, err.Error(), http.StatusBadRequest) } - if !plugin.IsValidId(manifest.Id) { - return nil, "", model.NewAppError("extractPlugin", "app.plugin.invalid_id.app_error", map[string]interface{}{"Min": plugin.MinIdLength, "Max": plugin.MaxIdLength, "Regex": plugin.ValidIdRegex}, "", http.StatusBadRequest) + if !model.IsValidPluginId(manifest.Id) { + return nil, "", model.NewAppError("installPluginLocally", "app.plugin.invalid_id.app_error", map[string]interface{}{"Min": model.MinIdLength, "Max": model.MaxIdLength, "Regex": model.ValidIdRegex}, "", http.StatusBadRequest) } return manifest, extractDir, nil diff --git a/model/manifest.go b/model/manifest.go index 43966e057a..2befe60f96 100644 --- a/model/manifest.go +++ b/model/manifest.go @@ -5,7 +5,6 @@ package model import ( "encoding/json" - "errors" "fmt" "io" "io/ioutil" @@ -14,6 +13,7 @@ import ( "strings" "github.com/blang/semver" + "github.com/pkg/errors" "gopkg.in/yaml.v2" ) @@ -25,6 +25,19 @@ type PluginOption struct { Value string `json:"value" yaml:"value"` } +type PluginSettingType int + +const ( + Bool PluginSettingType = iota + Dropdown + Generated + Radio + Text + LongText + Username + Custom +) + type PluginSetting struct { // The key that the setting will be assigned to in the configuration file. Key string `json:"key" yaml:"key"` @@ -304,6 +317,103 @@ func (m *Manifest) MeetMinServerVersion(serverVersion string) (bool, error) { return true, nil } +func (m *Manifest) IsValid() error { + if !IsValidPluginId(m.Id) { + return errors.New("invalid plugin ID") + } + + if m.HomepageURL == "" || !IsValidHttpUrl(m.HomepageURL) { + return errors.New("invalid HomepageURL") + } + + if m.SupportURL == "" || !IsValidHttpUrl(m.SupportURL) { + return errors.New("invalid SupportURL") + } + + _, err := semver.Parse(m.Version) + if err != nil { + return errors.Wrap(err, "failed to parse Version") + } + + _, err2 := semver.Parse(m.MinServerVersion) + if err2 != nil { + return errors.Wrap(err2, "failed to parse MinServerVersion") + } + + if m.SettingsSchema != nil { + err3 := m.SettingsSchema.isValidSchema() + if err3 != nil { + return errors.Wrap(err3, "invalid settings schema") + } + } + + return nil +} + +func (s *PluginSettingsSchema) isValidSchema() error { + for _, setting := range s.Settings { + err := setting.isValid() + if err != nil { + return err + } + } + + return nil +} + +func (s *PluginSetting) isValid() error { + pluginSettingType, err := convertTypeToPluginSettingType(s.Type) + if err != nil { + return err + } + + if s.RegenerateHelpText != "" && pluginSettingType != Generated { + return errors.New("should not set RegenerateHelpText for setting type that is not generated") + } + + if s.Placeholder != "" && !(pluginSettingType == Text || pluginSettingType == Generated || pluginSettingType == Username) { + return errors.New("should not set Placeholder for setting type not in text, generated or username") + } + + if s.Options != nil { + if pluginSettingType != Radio && pluginSettingType != Dropdown { + return errors.New("should not set Options for setting type not in radio or dropdown") + } + + for _, option := range s.Options { + if option.DisplayName == "" || option.Value == "" { + return errors.New("should not have empty Displayname or Value for any option") + } + } + } + + return nil +} + +func convertTypeToPluginSettingType(t string) (PluginSettingType, error) { + var settingType PluginSettingType + switch t { + case "bool": + return Bool, nil + case "dropdown": + return Dropdown, nil + case "generated": + return Generated, nil + case "radio": + return Radio, nil + case "text": + return Text, nil + case "longtext": + return LongText, nil + case "username": + return Username, nil + case "custom": + return Custom, nil + default: + return settingType, errors.New("invalid setting type: " + t) + } +} + // FindManifest will find and parse the manifest in a given directory. // // In all cases other than a does-not-exist error, path is set to the path of the manifest file that was diff --git a/model/manifest_test.go b/model/manifest_test.go index 5161c8bca9..f5efd508af 100644 --- a/model/manifest_test.go +++ b/model/manifest_test.go @@ -17,6 +17,162 @@ import ( "github.com/stretchr/testify/require" ) +func TestIsValid(t *testing.T) { + testCases := []struct { + Title string + manifest *Manifest + ExpectError bool + }{ + {"Invalid Id", &Manifest{Id: "some id"}, true}, + {"Invalid homePageURL", &Manifest{Id: "com.company.test", HomepageURL: "some url"}, true}, + {"Invalid supportURL", &Manifest{Id: "com.company.test", HomepageURL: "http://someurl.com", SupportURL: "some url"}, true}, + {"Invalid version", &Manifest{Id: "com.company.test", HomepageURL: "http://someurl.com", SupportURL: "http://someotherurl.com", Version: "version"}, true}, + {"Invalid min version", &Manifest{Id: "com.company.test", HomepageURL: "http://someurl.com", SupportURL: "http://someotherurl.com", Version: "5.10.0", MinServerVersion: "version"}, true}, + {"SettingSchema error", &Manifest{Id: "com.company.test", HomepageURL: "http://someurl.com", SupportURL: "http://someotherurl.com", Version: "5.10.0", MinServerVersion: "5.10.8", SettingsSchema: &PluginSettingsSchema{ + Settings: []*PluginSetting{{Type: "Invalid"}}, + }}, true}, + {"Happy case", &Manifest{ + Id: "com.company.test", + Name: "thename", + Description: "thedescription", + HomepageURL: "http://someurl.com", + SupportURL: "http://someotherurl.com", + Version: "0.0.1", + MinServerVersion: "5.6.0", + Server: &ManifestServer{ + Executable: "theexecutable", + }, + Webapp: &ManifestWebapp{ + BundlePath: "thebundlepath", + BundleHash: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}, + }, + SettingsSchema: &PluginSettingsSchema{ + Header: "theheadertext", + Footer: "thefootertext", + Settings: []*PluginSetting{ + { + Key: "thesetting", + DisplayName: "thedisplayname", + Type: "dropdown", + HelpText: "thehelptext", + Options: []*PluginOption{ + { + DisplayName: "theoptiondisplayname", + Value: "thevalue", + }, + }, + Default: "thedefault", + }, + }, + }, + }, false}, + } + + for _, tc := range testCases { + t.Run(tc.Title, func(t *testing.T) { + err := tc.manifest.IsValid() + if tc.ExpectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestIsValidSettingsSchema(t *testing.T) { + testCases := []struct { + Title string + settingsSchema *PluginSettingsSchema + ExpectError bool + }{ + {"Invalid Setting", &PluginSettingsSchema{Settings: []*PluginSetting{{Type: "invalid"}}}, true}, + {"Happy case", &PluginSettingsSchema{Settings: []*PluginSetting{{Type: "text"}}}, false}, + } + + for _, tc := range testCases { + t.Run(tc.Title, func(t *testing.T) { + err := tc.settingsSchema.isValidSchema() + if tc.ExpectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestSettingIsValid(t *testing.T) { + testCases := []struct { + Title string + setting *PluginSetting + ExpectError bool + }{ + {"Invalid setting type", &PluginSetting{Type: "invalid"}, true}, + {"RegenerateHelpText error", &PluginSetting{Type: "text", RegenerateHelpText: "some text"}, true}, + {"Placeholder error", &PluginSetting{Type: "bool", Placeholder: "some text"}, true}, + {"Nil Options", &PluginSetting{Type: "bool"}, false}, + {"Options error", &PluginSetting{Type: "generated", Options: []*PluginOption{}}, true}, + {"Options displayName error", &PluginSetting{Type: "radio", Options: []*PluginOption{ + { + Value: "some value", + }, + }}, true}, + {"Options value error", &PluginSetting{Type: "radio", Options: []*PluginOption{ + { + DisplayName: "some name", + }, + }}, true}, + {"Happy case", &PluginSetting{Type: "radio", Options: []*PluginOption{ + { + DisplayName: "Name", + Value: "value", + }, + }}, false}, + } + + for _, tc := range testCases { + t.Run(tc.Title, func(t *testing.T) { + err := tc.setting.isValid() + if tc.ExpectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestConvertTypeToPluginSettingType(t *testing.T) { + testCases := []struct { + Title string + Type string + ExpectedSettingType PluginSettingType + ExpectError bool + }{ + {"bool", "bool", Bool, false}, + {"dropdown", "dropdown", Dropdown, false}, + {"generated", "generated", Generated, false}, + {"radio", "radio", Radio, false}, + {"text", "text", Text, false}, + {"longtext", "longtext", LongText, false}, + {"username", "username", Username, false}, + {"custom", "custom", Custom, false}, + {"invalid", "invalid", Bool, true}, + } + + for _, tc := range testCases { + t.Run(tc.Title, func(t *testing.T) { + settingType, err := convertTypeToPluginSettingType(tc.Type) + if !tc.ExpectError { + assert.Equal(t, settingType, tc.ExpectedSettingType) + } else { + assert.Error(t, err) + } + }) + } +} + func TestFindManifest(t *testing.T) { for _, tc := range []struct { Filename string diff --git a/plugin/valid.go b/model/plugin_valid.go similarity index 82% rename from plugin/valid.go rename to model/plugin_valid.go index 1121d45519..b614451300 100644 --- a/plugin/valid.go +++ b/model/plugin_valid.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package plugin +package model import ( "regexp" @@ -22,11 +22,11 @@ func init() { validId = regexp.MustCompile(ValidIdRegex) } -// IsValidId verifies that the plugin id has a minimum length of 3, maximum length of 190, and +// IsValidPluginId verifies that the plugin id has a minimum length of 3, maximum length of 190, and // contains only alphanumeric characters, dashes, underscores and periods. // // These constraints are necessary since the plugin id is used as part of a filesystem path. -func IsValidId(id string) bool { +func IsValidPluginId(id string) bool { if utf8.RuneCountInString(id) < MinIdLength { return false } diff --git a/plugin/valid_test.go b/model/plugin_valid_test.go similarity index 85% rename from plugin/valid_test.go rename to model/plugin_valid_test.go index cdc8fffd80..ad3dbafb57 100644 --- a/plugin/valid_test.go +++ b/model/plugin_valid_test.go @@ -1,17 +1,15 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package plugin_test +package model import ( "testing" "github.com/stretchr/testify/assert" - - "github.com/mattermost/mattermost-server/v5/plugin" ) -func TestIsValid(t *testing.T) { +func TestIsValidPluginId(t *testing.T) { t.Parallel() testCases := map[string]bool{ @@ -29,7 +27,7 @@ func TestIsValid(t *testing.T) { for id, valid := range testCases { t.Run(id, func(t *testing.T) { - assert.Equal(t, valid, plugin.IsValidId(id)) + assert.Equal(t, valid, IsValidPluginId(id)) }) } }