Fix Autolink admin config parsing (#184)

plugin.json

@@ -21,19 +21,19 @@
     "header": "Configure this plugin directly in the `config.json` file, or using the `/autolink` command. Learn more [in our documentation](https://github.com/mattermost/mattermost-plugin-autolink/blob/master/README.md).\n\n To report an issue, make a suggestion, or contribute, [check the plugin repository](https://github.com/mattermost/mattermost-plugin-autolink).",
     "settings": [
       {
-        "key": "EnableAdminCommand",
+        "key": "enableadmincommand",
         "display_name": "Enable administration with /autolink command:",
         "type": "bool",
         "default": true
       },
       {
-        "key": "EnableOnUpdate",
+        "key": "enableonupdate",
         "display_name": "Apply plugin to updated posts as well as new posts:",
         "type": "bool",
         "default": false
       },
       {
-        "key": "PluginAdmins",
+        "key": "pluginadmins",
         "display_name": "Admin User IDs:",
         "type": "text",
         "help_text": "Comma-separated list of user IDs authorized to administer the plugin in addition to the System Admins.\n \n User IDs can be found by navigating to **System Console > User Management > Users**."

server/api/api.go

@@ -2,10 +2,10 @@ package api
 
 import (
 	"encoding/json"
-	"fmt"
 	"net/http"
 
 	"github.com/gorilla/mux"
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-plugin-autolink/server/autolink"
 )
@@ -90,7 +90,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 func (h *Handler) setLink(w http.ResponseWriter, r *http.Request) {
 	var newLink autolink.Autolink
 	if err := json.NewDecoder(r.Body).Decode(&newLink); err != nil {
-		h.handleError(w, fmt.Errorf("unable to decode body: %w", err))
+		h.handleError(w, errors.Wrap(err, "unable to decode body"))
 		return
 	}
 
@@ -114,7 +114,7 @@ func (h *Handler) setLink(w http.ResponseWriter, r *http.Request) {
 	status := http.StatusNotModified
 	if changed {
 		if err := h.store.SaveLinks(links); err != nil {
-			h.handleError(w, fmt.Errorf("unable to save link: %w", err))
+			h.handleError(w, errors.Wrap(err, "unable to save link"))
 			return
 		}
 		status = http.StatusOK

server/autolink/autolink.go

@@ -7,14 +7,14 @@ import (
 
 // Autolink represents a pattern to autolink.
 type Autolink struct {
-	Name                 string
-	Disabled             bool
-	Pattern              string
-	Template             string
-	Scope                []string
-	WordMatch            bool
-	DisableNonWordPrefix bool
-	DisableNonWordSuffix bool
+	Name                 string   `json:"Name"`
+	Disabled             bool     `json:"Disabled"`
+	Pattern              string   `json:"Pattern"`
+	Template             string   `json:"Template"`
+	Scope                []string `json:"Scope"`
+	WordMatch            bool     `json:"WordMatch"`
+	DisableNonWordPrefix bool     `json:"DisableNonWordPrefix"`
+	DisableNonWordSuffix bool     `json:"DisableNonWordSuffix"`
 
 	template      string
 	re            *regexp.Regexp
@@ -156,19 +156,3 @@ func (l Autolink) ToMarkdown(i int) string {
 	}
 	return text
 }
-
-// ToConfig returns a JSON-encodable Link represented solely with map[string]
-// interface and []string types, compatible with gob/RPC, to be used in
-// SavePluginConfig
-func (l Autolink) ToConfig() map[string]interface{} {
-	return map[string]interface{}{
-		"Name":                 l.Name,
-		"Pattern":              l.Pattern,
-		"Template":             l.Template,
-		"Scope":                l.Scope,
-		"DisableNonWordPrefix": l.DisableNonWordPrefix,
-		"DisableNonWordSuffix": l.DisableNonWordSuffix,
-		"WordMatch":            l.WordMatch,
-		"Disabled":             l.Disabled,
-	}
-}

server/autolinkplugin/command.go

@@ -392,7 +392,13 @@ func parseBoolArg(arg string) (bool, error) {
 func saveConfigLinks(p *Plugin, links []autolink.Autolink) error {
 	conf := p.getConfig()
 	conf.Links = links
-	appErr := p.API.SavePluginConfig(conf.ToConfig())
+
+	configMap, err := p.getConfig().ToMap()
+	if err != nil {
+		return err
+	}
+
+	appErr := p.API.SavePluginConfig(configMap)
 	if appErr != nil {
 		return appErr
 	}

server/autolinkplugin/config.go

@@ -1,34 +1,36 @@
 package autolinkplugin
 
 import (
-	"fmt"
+	"encoding/json"
 	"sort"
 	"strings"
 
 	"github.com/mattermost/mattermost-server/v6/model"
+	"github.com/mattermost/mattermost-server/v6/plugin"
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-plugin-autolink/server/autolink"
 )
 
 // Config from config.json
 type Config struct {
-	EnableAdminCommand bool
-	EnableOnUpdate     bool
-	PluginAdmins       string
-	Links              []autolink.Autolink
+	EnableAdminCommand bool                `json:"enableadmincommand"`
+	EnableOnUpdate     bool                `json:"enableonupdate"`
+	PluginAdmins       string              `json:"pluginadmins"`
+	Links              []autolink.Autolink `json:"links"`
 
 	// AdminUserIds is a set of UserIds that are permitted to perform
 	// administrative operations on the plugin configuration (i.e. plugin
 	// admins). On each configuration change the contents of PluginAdmins
 	// config field is parsed into this field.
-	AdminUserIds map[string]struct{}
+	AdminUserIds map[string]struct{} `json:"-"`
 }
 
 // OnConfigurationChange is invoked when configuration changes may have been made.
 func (p *Plugin) OnConfigurationChange() error {
 	var c Config
 	if err := p.API.LoadPluginConfiguration(&c); err != nil {
-		return fmt.Errorf("failed to load configuration: %w", err)
+		return errors.Wrap(err, "failed to load plugin configuration")
 	}
 
 	for i := range c.Links {
@@ -40,7 +42,7 @@ func (p *Plugin) OnConfigurationChange() error {
 	// Plugin admin UserId parsing and validation errors are
 	// not fatal, if everything fails only sysadmin will be able to manage the
 	// config which is still OK
-	c.parsePluginAdminList(p)
+	c.parsePluginAdminList(p.API)
 
 	p.UpdateConfig(func(conf *Config) {
 		*conf = c
@@ -169,9 +171,15 @@ func (p *Plugin) SaveLinks(links []autolink.Autolink) error {
 	p.UpdateConfig(func(conf *Config) {
 		conf.Links = links
 	})
-	appErr := p.API.SavePluginConfig(p.getConfig().ToConfig())
+
+	configMap, err := p.getConfig().ToMap()
+	if err != nil {
+		return errors.Wrap(err, "unable convert config to map")
+	}
+
+	appErr := p.API.SavePluginConfig(configMap)
 	if appErr != nil {
-		return fmt.Errorf("unable to save links: %w", appErr)
+		return errors.Wrap(appErr, "unable to save links")
 	}
 
 	return nil
@@ -186,17 +194,18 @@ func (p *Plugin) UpdateConfig(f func(conf *Config)) {
 
 // ToConfig marshals Config into a tree of map[string]interface{} to pass down
 // to p.API.SavePluginConfig, otherwise RPC/gob barfs at the unknown type.
-func (conf *Config) ToConfig() map[string]interface{} {
-	links := []interface{}{}
-	for _, l := range conf.Links {
-		links = append(links, l.ToConfig())
+func (conf *Config) ToMap() (map[string]interface{}, error) {
+	var out map[string]interface{}
+	data, err := json.Marshal(conf)
+	if err != nil {
+		return nil, err
 	}
-	return map[string]interface{}{
-		"EnableAdminCommand": conf.EnableAdminCommand,
-		"EnableOnUpdate":     conf.EnableOnUpdate,
-		"PluginAdmins":       conf.PluginAdmins,
-		"Links":              links,
+	err = json.Unmarshal(data, &out)
+	if err != nil {
+		return nil, err
 	}
+
+	return out, nil
 }
 
 // Sorted returns a clone of the Config, with links sorted alphabetically
@@ -210,22 +219,21 @@ func (conf *Config) Sorted() *Config {
 }
 
 // parsePluginAdminList parses the contents of PluginAdmins config field
-func (conf *Config) parsePluginAdminList(p *Plugin) {
-	conf.AdminUserIds = make(map[string]struct{})
+func (conf *Config) parsePluginAdminList(api plugin.API) {
+	conf.AdminUserIds = make(map[string]struct{}, len(conf.PluginAdmins))
 
 	if len(conf.PluginAdmins) == 0 {
 		// There were no plugin admin users defined
 		return
 	}
 
 	userIDs := strings.Split(conf.PluginAdmins, ",")
-
-	for _, v := range userIDs {
-		userID := strings.TrimSpace(v)
+	for _, userID := range userIDs {
+		userID = strings.TrimSpace(userID)
 		// Let's verify that the given user really exists
-		_, appErr := p.API.GetUser(userID)
+		_, appErr := api.GetUser(userID)
 		if appErr != nil {
-			p.API.LogError(fmt.Sprintf("error occurred while verifying userID %s: %v", v, appErr))
+			api.LogWarn("Error occurred while verifying userID", "userID", userID, "error", appErr)
 		} else {
 			conf.AdminUserIds[userID] = struct{}{}
 		}

server/autolinkplugin/plugin.go

@@ -9,6 +9,7 @@ import (
 	"github.com/mattermost/mattermost-server/v6/model"
 	"github.com/mattermost/mattermost-server/v6/plugin"
 	"github.com/mattermost/mattermost-server/v6/shared/markdown"
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-plugin-autolink/server/api"
 )
@@ -39,8 +40,7 @@ func (p *Plugin) OnActivate() error {
 func (p *Plugin) IsAuthorizedAdmin(userID string) (bool, error) {
 	user, err := p.API.GetUser(userID)
 	if err != nil {
-		return false, fmt.Errorf(
-			"failed to obtain information about user `%s`: %w", userID, err)
+		return false, errors.Wrapf(err, "failed to obtain information about user `%s`", userID)
 	}
 	if strings.Contains(user.Roles, "system_admin") {
 		p.API.LogInfo(

server/autolinkplugin/plugin_test.go

@@ -258,7 +258,12 @@ func (suite *SuiteAuthorization) TestNonExistantUsersAreIgnored() {
 	}
 	suite.adminUsernames = "marynaId,karynaId"
 
-	suite.api.On("LogError", mock.AnythingOfType("string")).Return(nil)
+	suite.api.On("LogWarn", mock.AnythingOfType("string"),
+		"userID",
+		"karynaId",
+		"error",
+		mock.AnythingOfType("*model.AppError"),
+	).Return(nil)
 	suite.api.On("LogInfo", mock.AnythingOfType("string")).Return(nil)
 
 	p := New()