Fix config auto unmarshalling (#13)

server/configuration.go

@@ -0,0 +1,89 @@
+// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved.
+// See License for license information.
+
+package main
+
+import (
+	"reflect"
+
+	"github.com/pkg/errors"
+)
+
+// configuration captures the plugin's external configuration as exposed in the Mattermost server
+// configuration, as well as values computed from the configuration. Any public fields will be
+// deserialized from the Mattermost server configuration in OnConfigurationChange.
+//
+// As plugins are inherently concurrent (hooks being called asynchronously), and the plugin
+// configuration can change at any time, access to the configuration must be synchronized. The
+// strategy used in this plugin is to guard a pointer to the configuration, and clone the entire
+// struct whenever it changes. You may replace this with whatever strategy you choose.
+//
+// If you add non-reference types to your configuration struct, be sure to rewrite Clone as a deep
+// copy appropriate for your types.
+type configuration struct {
+	Enabled  bool
+	Secret   string
+	UserName string
+}
+
+// Clone shallow copies the configuration. Your implementation may require a deep copy if
+// your configuration has reference types.
+func (c *configuration) Clone() *configuration {
+	var clone = *c
+	return &clone
+}
+
+// getConfiguration retrieves the active configuration under lock, making it safe to use
+// concurrently. The active configuration may change underneath the client of this method, but
+// the struct returned by this API call is considered immutable.
+func (p *Plugin) getConfiguration() *configuration {
+	p.configurationLock.RLock()
+	defer p.configurationLock.RUnlock()
+
+	if p.configuration == nil {
+		return &configuration{}
+	}
+
+	return p.configuration
+}
+
+// setConfiguration replaces the active configuration under lock.
+//
+// Do not call setConfiguration while holding the configurationLock, as sync.Mutex is not
+// reentrant. In particular, avoid using the plugin API entirely, as this may in turn trigger a
+// hook back into the plugin. If that hook attempts to acquire this lock, a deadlock may occur.
+//
+// This method panics if setConfiguration is called with the existing configuration. This almost
+// certainly means that the configuration was modified without being cloned and may result in
+// an unsafe access.
+func (p *Plugin) setConfiguration(configuration *configuration) {
+	p.configurationLock.Lock()
+	defer p.configurationLock.Unlock()
+
+	if configuration != nil && p.configuration == configuration {
+		// Ignore assignment if the configuration struct is empty. Go will optimize the
+		// allocation for same to point at the same memory address, breaking the check
+		// above.
+		if reflect.ValueOf(*configuration).NumField() == 0 {
+			return
+		}
+
+		panic("setConfiguration called with the existing configuration")
+	}
+
+	p.configuration = configuration
+}
+
+// OnConfigurationChange is invoked when configuration changes may have been made.
+func (p *Plugin) OnConfigurationChange() error {
+	var configuration = new(configuration)
+
+	// Load the public configuration fields from the Mattermost server configuration.
+	if err := p.API.LoadPluginConfiguration(configuration); err != nil {
+		return errors.Wrap(err, "failed to load plugin configuration")
+	}
+
+	p.setConfiguration(configuration)
+
+	return nil
+}

server/plugin.go

@@ -7,6 +7,7 @@ import (
 	"crypto/subtle"
 	"encoding/json"
 	"net/http"
+	"sync"
 
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/plugin"
@@ -15,13 +16,17 @@ import (
 type Plugin struct {
 	plugin.MattermostPlugin
 
-	Enabled  bool
-	Secret   string
-	UserName string
+	// configurationLock synchronizes access to the configuration.
+	configurationLock sync.RWMutex
+
+	// configuration is the active plugin configuration. Consult getConfiguration and
+	// setConfiguration for usage.
+	configuration *configuration
 }
 
 func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) {
-	if !p.Enabled || p.Secret == "" || p.UserName == "" {
+	config := p.getConfiguration()
+	if !config.Enabled || config.Secret == "" || config.UserName == "" {
 		http.Error(w, "This plugin is not configured.", http.StatusForbidden)
 		return
 	} else if r.URL.Path != "/webhook" {
@@ -30,7 +35,7 @@ func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Req
 	} else if r.Method != http.MethodPost {
 		http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
 		return
-	} else if subtle.ConstantTimeCompare([]byte(r.URL.Query().Get("secret")), []byte(p.Secret)) != 1 {
+	} else if subtle.ConstantTimeCompare([]byte(r.URL.Query().Get("secret")), []byte(config.Secret)) != 1 {
 		http.Error(w, "You must provide the configured secret.", http.StatusForbidden)
 		return
 	}
@@ -45,7 +50,7 @@ func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Req
 		return
 	} else if r.URL.Query().Get("channel") == "" {
 		http.Error(w, "You must provide a channel.", http.StatusBadRequest)
-	} else if user, err := p.API.GetUserByUsername(p.UserName); err != nil {
+	} else if user, err := p.API.GetUserByUsername(config.UserName); err != nil {
 		http.Error(w, err.Message, err.StatusCode)
 	} else if channel, err := p.API.GetChannelByNameForTeamName(r.URL.Query().Get("team"), r.URL.Query().Get("channel"), false); err != nil {
 		http.Error(w, err.Message, err.StatusCode)

server/plugin_test.go

@@ -147,9 +147,11 @@ func TestPlugin(t *testing.T) {
 			}, (*model.AppError)(nil))
 
 			p := Plugin{}
-			p.Enabled = tc.Configuration.Enabled
-			p.Secret = tc.Configuration.Secret
-			p.UserName = tc.Configuration.UserName
+			p.setConfiguration(&configuration{
+				Enabled:  tc.Configuration.Enabled,
+				Secret:   tc.Configuration.Secret,
+				UserName: tc.Configuration.UserName,
+			})
 			p.SetAPI(api)
 
 			w := httptest.NewRecorder()