MM-11262: database config store (#10281)

* vendor github.com/jmoiron/sqlx

* MM-11262: introduce a database store

* revert unnecessary fmt.Errorf

* simplify unit test helper methods

* remote TODO re: retry

* relocate initializeConfigurationsTable for clarity

* factor out a commonStore

* acquire database config lock on close for safety

* add missing header

* fix lock comment

config/common.go

@@ -0,0 +1,140 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package config
+
+import (
+	"io"
+	"sync"
+
+	"github.com/mattermost/mattermost-server/model"
+	"github.com/pkg/errors"
+)
+
+// commonStore enables code sharing between different backing implementations
+type commonStore struct {
+	emitter
+
+	configLock           sync.RWMutex
+	config               *model.Config
+	environmentOverrides map[string]interface{}
+}
+
+// Get fetches the current, cached configuration.
+func (cs *commonStore) Get() *model.Config {
+	cs.configLock.RLock()
+	defer cs.configLock.RUnlock()
+
+	return cs.config
+}
+
+// GetEnvironmentOverrides fetches the configuration fields overridden by environment variables.
+func (cs *commonStore) GetEnvironmentOverrides() map[string]interface{} {
+	cs.configLock.RLock()
+	defer cs.configLock.RUnlock()
+
+	return cs.environmentOverrides
+}
+
+// set replaces the current configuration in its entirety, without updating the backing store.
+//
+// This function assumes no lock has been acquired, as it acquires a write lock itself.
+func (cs *commonStore) set(newCfg *model.Config, isValid func(*model.Config) error) (*model.Config, error) {
+	cs.configLock.Lock()
+	var unlockOnce sync.Once
+	defer unlockOnce.Do(cs.configLock.Unlock)
+
+	oldCfg := cs.config
+
+	// TODO: disallow attempting to save a directly modified config (comparing pointers). This
+	// wouldn't be an exhaustive check, given the use of pointers throughout the data
+	// structure, but might prevent common mistakes. Requires upstream changes first.
+	// if newCfg == oldCfg {
+	// 	return nil, errors.New("old configuration modified instead of cloning")
+	// }
+
+	newCfg = newCfg.Clone()
+	newCfg.SetDefaults()
+
+	// Sometimes the config is received with "fake" data in sensitive fielcs. Apply the real
+	// data from the existing config as necessary.
+	desanitize(oldCfg, newCfg)
+
+	if err := newCfg.IsValid(); err != nil {
+		return nil, errors.Wrap(err, "new configuration is invalid")
+	}
+
+	// Allow backing-store specific checks.
+	if isValid != nil {
+		if err := isValid(newCfg); err != nil {
+			return nil, err
+		}
+	}
+
+	// Ideally, Set would persist automatically and abstract this completely away from the
+	// client. Doing so requires a few upstream changes first, so for now an explicit Save()
+	// remains required.
+	// if err := cs.persist(newCfg); err != nil {
+	// 	return nil, errors.Wrap(err, "failed to persist")
+	// }
+
+	cs.config = newCfg
+
+	unlockOnce.Do(cs.configLock.Unlock)
+
+	// Notify listeners synchronously. Ideally, this would be asynchronous, but existing code
+	// assumes this and there would be increased complexity to avoid racing updates.
+	cs.invokeConfigListeners(oldCfg, newCfg)
+
+	return oldCfg, nil
+}
+
+// load updates the current configuration from the given io.ReadCloser.
+//
+// This function assumes no lock has been acquired, as it acquires a write lock itself.
+func (cs *commonStore) load(f io.ReadCloser, needsSave bool, persist func(*model.Config) error) error {
+	allowEnvironmentOverrides := true
+	loadedCfg, environmentOverrides, err := unmarshalConfig(f, allowEnvironmentOverrides)
+	if err != nil {
+		return errors.Wrapf(err, "failed to unmarshal config")
+	}
+
+	// SetDefaults generates various keys and salts if not previously configured. Determine if
+	// such a change will be made before invoking. This method will not effect the save: that
+	// remains the responsibility of the caller.
+	needsSave = needsSave || loadedCfg.SqlSettings.AtRestEncryptKey == nil || len(*loadedCfg.SqlSettings.AtRestEncryptKey) == 0
+	needsSave = needsSave || loadedCfg.FileSettings.PublicLinkSalt == nil || len(*loadedCfg.FileSettings.PublicLinkSalt) == 0
+	needsSave = needsSave || loadedCfg.EmailSettings.InviteSalt == nil || len(*loadedCfg.EmailSettings.InviteSalt) == 0
+
+	loadedCfg.SetDefaults()
+
+	if err := loadedCfg.IsValid(); err != nil {
+		return errors.Wrap(err, "invalid config")
+	}
+
+	if changed := fixConfig(loadedCfg); changed {
+		needsSave = true
+	}
+
+	cs.configLock.Lock()
+	var unlockOnce sync.Once
+	defer unlockOnce.Do(cs.configLock.Unlock)
+
+	if needsSave {
+		if err = persist(loadedCfg); err != nil {
+			return errors.Wrap(err, "failed to persist required changes after load")
+		}
+	}
+
+	oldCfg := cs.config
+	cs.config = loadedCfg
+	cs.environmentOverrides = environmentOverrides
+
+	unlockOnce.Do(cs.configLock.Unlock)
+
+	// Notify listeners synchronously. Ideally, this would be asynchronous, but existing code
+	// assumes this and there would be increased complexity to avoid racing updates.
+	cs.invokeConfigListeners(oldCfg, loadedCfg)
+
+	return nil
+}

config/common_test.go

@@ -0,0 +1,17 @@
+package config_test
+
+import (
+	"testing"
+
+	"github.com/mattermost/mattermost-server/model"
+)
+
+func prepareExpectedConfig(t *testing.T, expectedCfg *model.Config) *model.Config {
+	// These fields require special initialization for our tests.
+	expectedCfg = expectedCfg.Clone()
+	expectedCfg.MessageExportSettings.GlobalRelaySettings = &model.GlobalRelayMessageExportSettings{}
+	expectedCfg.PluginSettings.Plugins = make(map[string]map[string]interface{})
+	expectedCfg.PluginSettings.PluginStates = make(map[string]*model.PluginState)
+
+	return expectedCfg
+}

config/database.go

@@ -0,0 +1,219 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package config
+
+import (
+	"bytes"
+	"database/sql"
+	"io/ioutil"
+	"net/url"
+	"strings"
+
+	"github.com/jmoiron/sqlx"
+	"github.com/pkg/errors"
+
+	"github.com/mattermost/mattermost-server/mlog"
+	"github.com/mattermost/mattermost-server/model"
+
+	// Load the MySQL driver
+	_ "github.com/go-sql-driver/mysql"
+	// Load the Postgres driver
+	_ "github.com/lib/pq"
+)
+
+// DatabaseStore is a config store backed by a database.
+type DatabaseStore struct {
+	commonStore
+
+	originalDsn    string
+	driverName     string
+	dataSourceName string
+	db             *sqlx.DB
+}
+
+// NewDatabaseStore creates a new instance of a config store backed by the given database.
+func NewDatabaseStore(dsn string) (ds *DatabaseStore, err error) {
+	driverName, dataSourceName, err := parseDSN(dsn)
+	if err != nil {
+		return nil, errors.Wrap(err, "invalid DSN")
+	}
+
+	db, err := sqlx.Open(driverName, dataSourceName)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to connect to %s database", driverName)
+	}
+
+	ds = &DatabaseStore{
+		driverName:     driverName,
+		originalDsn:    dsn,
+		dataSourceName: dataSourceName,
+		db:             db,
+	}
+	if err = initializeConfigurationsTable(ds.db); err != nil {
+		return nil, errors.Wrap(err, "failed to initialize")
+	}
+
+	if err = ds.Load(); err != nil {
+		return nil, errors.Wrap(err, "failed to load")
+	}
+
+	return ds, nil
+}
+
+// initializeConfigurationsTable ensures the requisite tables in place to form the backing store.
+func initializeConfigurationsTable(db *sqlx.DB) error {
+	_, err := db.Exec(`
+		CREATE TABLE IF NOT EXISTS Configurations (
+		    Id VARCHAR(26) PRIMARY KEY,
+		    Value TEXT NOT NULL,
+		    CreateAt BIGINT NOT NULL,
+		    Active BOOLEAN NULL UNIQUE
+		)
+	`)
+	if err != nil {
+		return errors.Wrap(err, "failed to create Configurations table")
+	}
+
+	return nil
+}
+
+// parseDSN splits up a connection string into a driver name and data source name.
+//
+// For example:
+//	mysql://mmuser:mostest@dockerhost:5432/mattermost_test
+// returns
+//	driverName = mysql
+//	dataSourceName = mmuser:mostest@dockerhost:5432/mattermost_test
+//
+// By contrast, a Postgres DSN is returned unmodified.
+func parseDSN(dsn string) (string, string, error) {
+	// Treat the DSN as the URL that it is.
+	u, err := url.Parse(dsn)
+	if err != nil {
+		return "", "", errors.Wrap(err, "failed to parse DSN as URL")
+	}
+
+	scheme := u.Scheme
+	switch scheme {
+	case "mysql":
+		// Strip off the mysql:// for the dsn with which to connect.
+		u.Scheme = ""
+		dsn = strings.TrimPrefix(u.String(), "//")
+
+	case "postgres":
+		// No changes required
+
+	default:
+		return "", "", errors.Wrapf(err, "unsupported scheme %s", scheme)
+	}
+
+	return scheme, dsn, nil
+}
+
+// Set replaces the current configuration in its entirety, without updating the backing store.
+func (ds *DatabaseStore) Set(newCfg *model.Config) (*model.Config, error) {
+	return ds.commonStore.set(newCfg, nil)
+}
+
+// persist writes the configuration to the configured database.
+func (ds *DatabaseStore) persist(cfg *model.Config) error {
+	b, err := marshalConfig(cfg)
+	if err != nil {
+		return errors.Wrap(err, "failed to serialize")
+	}
+
+	id := model.NewId()
+	value := string(b)
+	createAt := model.GetMillis()
+
+	tx, err := ds.db.Beginx()
+	if err != nil {
+		return errors.Wrap(err, "failed to begin transaction")
+	}
+	defer func() {
+		// Rollback after Commit just returns sql.ErrTxDone.
+		if err := tx.Rollback(); err != nil && err != sql.ErrTxDone {
+			mlog.Error("Failed to rollback configuration transaction", mlog.Err(err))
+		}
+	}()
+
+	params := map[string]interface{}{
+		"id":        id,
+		"value":     value,
+		"create_at": createAt,
+		"key":       "ConfigurationId",
+	}
+
+	if _, err := tx.Exec("UPDATE Configurations SET Active = NULL WHERE Active"); err != nil {
+		return errors.Wrap(err, "failed to deactivate current configuration")
+	}
+
+	if _, err := tx.NamedExec("INSERT INTO Configurations (Id, Value, CreateAt, Active) VALUES (:id, :value, :create_at, TRUE)", params); err != nil {
+		return errors.Wrap(err, "failed to record new configuration")
+	}
+
+	if err := tx.Commit(); err != nil {
+		return errors.Wrap(err, "failed to commit transaction")
+	}
+
+	return nil
+}
+
+// Load updates the current configuration from the backing store.
+func (ds *DatabaseStore) Load() (err error) {
+	var needsSave bool
+	var configurationData []byte
+
+	row := ds.db.QueryRow("SELECT Value FROM Configurations WHERE Active")
+	if err = row.Scan(&configurationData); err != nil && err != sql.ErrNoRows {
+		return errors.Wrap(err, "failed to query active configuration")
+	}
+
+	// Initialize from the default config if no active configuration could be found.
+	if len(configurationData) == 0 {
+		needsSave = true
+
+		defaultCfg := model.Config{}
+		defaultCfg.SetDefaults()
+
+		// Assume the database storing the config is also to be used for the application.
+		// This can be overridden using environment variables on first start if necessary,
+		// or changed from the system console afterwards.
+		*defaultCfg.SqlSettings.DriverName = ds.driverName
+		*defaultCfg.SqlSettings.DataSource = ds.dataSourceName
+
+		configurationData, err = marshalConfig(&defaultCfg)
+		if err != nil {
+			return errors.Wrap(err, "failed to serialize default config")
+		}
+	}
+
+	return ds.commonStore.load(ioutil.NopCloser(bytes.NewReader(configurationData)), needsSave, ds.persist)
+}
+
+// Save writes the current configuration to the backing store.
+func (ds *DatabaseStore) Save() error {
+	ds.configLock.RLock()
+	defer ds.configLock.RUnlock()
+
+	return ds.persist(ds.config)
+}
+
+// String returns the path to the database backing the config, masking the password.
+func (ds *DatabaseStore) String() string {
+	u, _ := url.Parse(ds.originalDsn)
+
+	// Strip out the password to avoid leaking in logs.
+	u.User = url.User(u.User.Username())
+
+	return u.String()
+}
+
+// Close cleans up resources associated with the store.
+func (ds *DatabaseStore) Close() error {
+	ds.configLock.Lock()
+	defer ds.configLock.Unlock()
+
+	return ds.db.Close()
+}

config/database_test.go

@@ -0,0 +1,470 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package config_test
+
+import (
+	"bytes"
+	"fmt"
+	"net/url"
+	"os"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/jmoiron/sqlx"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/mattermost/mattermost-server/config"
+	"github.com/mattermost/mattermost-server/model"
+)
+
+func setupConfigDatabase(t *testing.T, cfg *model.Config) (string, func()) {
+	t.Helper()
+	os.Clearenv()
+	truncateTables(t)
+
+	cfgData, err := config.MarshalConfig(cfg)
+	require.NoError(t, err)
+
+	db := sqlx.NewDb(mainHelper.SqlSupplier.GetMaster().Db, *mainHelper.Settings.DriverName)
+	err = config.InitializeConfigurationsTable(db)
+	require.NoError(t, err)
+
+	id := model.NewId()
+	_, err = db.NamedExec("INSERT INTO Configurations (Id, Value, CreateAt, Active) VALUES(:Id, :Value, :CreateAt, TRUE)", map[string]interface{}{
+		"Id":       id,
+		"Value":    cfgData,
+		"CreateAt": model.GetMillis(),
+	})
+	require.NoError(t, err)
+
+	return id, func() {
+		truncateTables(t)
+	}
+}
+
+// getActualDatabaseConfig returns the active configuration in the database without relying on a config store.
+func getActualDatabaseConfig(t *testing.T) *model.Config {
+	t.Helper()
+
+	var actualCfgData []byte
+	db := sqlx.NewDb(mainHelper.SqlSupplier.GetMaster().Db, *mainHelper.Settings.DriverName)
+	err := db.Get(&actualCfgData, "SELECT Value FROM Configurations WHERE Active")
+	require.NoError(t, err)
+
+	actualCfg, _, err := config.UnmarshalConfig(bytes.NewReader(actualCfgData), false)
+	require.Nil(t, err)
+
+	return actualCfg
+}
+
+// assertDatabaseEqualsConfig verifies the active in-database configuration equals the given config.
+func assertDatabaseEqualsConfig(t *testing.T, expectedCfg *model.Config) {
+	t.Helper()
+
+	expectedCfg = prepareExpectedConfig(t, expectedCfg)
+	actualCfg := getActualDatabaseConfig(t)
+	assert.Equal(t, expectedCfg, actualCfg)
+}
+
+// assertDatabaseNotEqualsConfig verifies the in-database configuration does not equal the given config.
+func assertDatabaseNotEqualsConfig(t *testing.T, expectedCfg *model.Config) {
+	t.Helper()
+
+	expectedCfg = prepareExpectedConfig(t, expectedCfg)
+	actualCfg := getActualDatabaseConfig(t)
+	assert.NotEqual(t, expectedCfg, actualCfg)
+}
+
+func TestDatabaseStoreNew(t *testing.T) {
+	t.Run("no existing configuration - initialization required", func(t *testing.T) {
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		assert.Equal(t, model.SERVICE_SETTINGS_DEFAULT_SITE_URL, *ds.Get().ServiceSettings.SiteURL)
+	})
+
+	t.Run("existing config, initialization required", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, testConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		assert.Equal(t, "http://TestStoreNew", *ds.Get().ServiceSettings.SiteURL)
+		assertDatabaseNotEqualsConfig(t, testConfig)
+	})
+
+	t.Run("already minimally configured", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, minimalConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL)
+		assertDatabaseEqualsConfig(t, minimalConfig)
+	})
+
+	t.Run("invalid url", func(t *testing.T) {
+		_, err := config.NewDatabaseStore("")
+		require.Error(t, err)
+	})
+
+	t.Run("unsupported scheme", func(t *testing.T) {
+		_, err := config.NewDatabaseStore("invalid")
+		require.Error(t, err)
+	})
+}
+
+func TestDatabaseStoreGet(t *testing.T) {
+	_, tearDown := setupConfigDatabase(t, testConfig)
+	defer tearDown()
+
+	ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+	require.NoError(t, err)
+	defer ds.Close()
+
+	cfg := ds.Get()
+	assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL)
+
+	cfg2 := ds.Get()
+	assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL)
+
+	assert.True(t, cfg == cfg2, "Get() returned different configuration instances")
+
+	newCfg := &model.Config{}
+	oldCfg, err := ds.Set(newCfg)
+	require.NoError(t, err)
+
+	assert.True(t, oldCfg == cfg, "returned config after set() changed original")
+	assert.False(t, newCfg == cfg, "returned config should have been different from original")
+}
+
+func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
+	_, tearDown := setupConfigDatabase(t, testConfig)
+	defer tearDown()
+
+	ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+	require.NoError(t, err)
+	defer ds.Close()
+
+	assert.Equal(t, "http://TestStoreNew", *ds.Get().ServiceSettings.SiteURL)
+	assert.Empty(t, ds.GetEnvironmentOverrides())
+
+	os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override")
+
+	ds, err = config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+	require.NoError(t, err)
+	defer ds.Close()
+
+	assert.Equal(t, "http://override", *ds.Get().ServiceSettings.SiteURL)
+	assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, ds.GetEnvironmentOverrides())
+}
+
+func TestDatabaseStoreSet(t *testing.T) {
+	t.Run("set same pointer value", func(t *testing.T) {
+		t.Skip("not yet implemented")
+
+		_, tearDown := setupConfigDatabase(t, emptyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		_, err = ds.Set(ds.Get())
+		if assert.Error(t, err) {
+			assert.EqualError(t, err, "old configuration modified instead of cloning")
+		}
+	})
+
+	t.Run("defaults required", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, minimalConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		oldCfg := ds.Get()
+
+		newCfg := &model.Config{}
+
+		retCfg, err := ds.Set(newCfg)
+		require.NoError(t, err)
+		assert.Equal(t, oldCfg, retCfg)
+
+		assert.Equal(t, model.SERVICE_SETTINGS_DEFAULT_SITE_URL, *ds.Get().ServiceSettings.SiteURL)
+	})
+
+	t.Run("desanitization required", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, ldapConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		oldCfg := ds.Get()
+
+		newCfg := &model.Config{}
+		newCfg.LdapSettings.BindPassword = sToP(model.FAKE_SETTING)
+
+		retCfg, err := ds.Set(newCfg)
+		require.NoError(t, err)
+		assert.Equal(t, oldCfg, retCfg)
+
+		assert.Equal(t, "password", *ds.Get().LdapSettings.BindPassword)
+	})
+
+	t.Run("invalid", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, emptyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		newCfg := &model.Config{}
+		newCfg.ServiceSettings.SiteURL = sToP("invalid")
+
+		_, err = ds.Set(newCfg)
+		if assert.Error(t, err) {
+			assert.EqualError(t, err, "new configuration is invalid: Config.IsValid: model.config.is_valid.site_url.app_error, ")
+		}
+
+		assert.Equal(t, model.SERVICE_SETTINGS_DEFAULT_SITE_URL, *ds.Get().ServiceSettings.SiteURL)
+	})
+
+	t.Run("read-only ignored", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, readOnlyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		newCfg := &model.Config{
+			ServiceSettings: model.ServiceSettings{
+				SiteURL: sToP("http://new"),
+			},
+		}
+
+		_, err = ds.Set(newCfg)
+		require.NoError(t, err)
+
+		assert.Equal(t, "http://new", *ds.Get().ServiceSettings.SiteURL)
+	})
+
+	t.Run("persist failed", func(t *testing.T) {
+		t.Skip("skipping persistence test inside Set")
+		_, tearDown := setupConfigDatabase(t, emptyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		db := sqlx.NewDb(mainHelper.SqlSupplier.GetMaster().Db, *mainHelper.Settings.DriverName)
+		_, err = db.Exec("DROP TABLE Configurations")
+		require.NoError(t, err)
+
+		newCfg := &model.Config{}
+
+		_, err = ds.Set(newCfg)
+		if assert.Error(t, err) {
+			assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: failed to write to database"))
+		}
+
+		assert.Equal(t, model.SERVICE_SETTINGS_DEFAULT_SITE_URL, *ds.Get().ServiceSettings.SiteURL)
+	})
+
+	t.Run("listeners notified", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, emptyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		oldCfg := ds.Get()
+
+		called := make(chan bool, 1)
+		callback := func(oldfg, newCfg *model.Config) {
+			called <- true
+		}
+		ds.AddListener(callback)
+
+		newCfg := &model.Config{}
+
+		retCfg, err := ds.Set(newCfg)
+		require.NoError(t, err)
+		assert.Equal(t, oldCfg, retCfg)
+
+		select {
+		case <-called:
+		case <-time.After(5 * time.Second):
+			t.Fatal("callback should have been called when config written")
+		}
+	})
+}
+
+func TestDatabaseStoreLoad(t *testing.T) {
+	t.Run("active configuration no longer exists", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, emptyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		truncateTables(t)
+
+		err = ds.Load()
+		require.NoError(t, err)
+		assertDatabaseNotEqualsConfig(t, emptyConfig)
+	})
+
+	t.Run("honour environment", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, minimalConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override")
+
+		err = ds.Load()
+		require.NoError(t, err)
+		assert.Equal(t, "http://override", *ds.Get().ServiceSettings.SiteURL)
+		assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, ds.GetEnvironmentOverrides())
+	})
+
+	t.Run("invalid", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, emptyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		cfgData, err := config.MarshalConfig(invalidConfig)
+		require.NoError(t, err)
+
+		db := sqlx.NewDb(mainHelper.SqlSupplier.GetMaster().Db, *mainHelper.Settings.DriverName)
+		truncateTables(t)
+		id := model.NewId()
+		_, err = db.NamedExec("INSERT INTO Configurations (Id, Value, CreateAt, Active) VALUES(:Id, :Value, :CreateAt, TRUE)", map[string]interface{}{
+			"Id":       id,
+			"Value":    cfgData,
+			"CreateAt": model.GetMillis(),
+		})
+		require.NoError(t, err)
+
+		err = ds.Load()
+		if assert.Error(t, err) {
+			assert.EqualError(t, err, "invalid config: Config.IsValid: model.config.is_valid.site_url.app_error, ")
+		}
+	})
+
+	t.Run("fixes required", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, fixesRequiredConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		err = ds.Load()
+		require.NoError(t, err)
+		assertDatabaseNotEqualsConfig(t, fixesRequiredConfig)
+		assert.Equal(t, "http://trailingslash", *ds.Get().ServiceSettings.SiteURL)
+	})
+
+	t.Run("listeners notifed", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, emptyConfig)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		called := make(chan bool, 1)
+		callback := func(oldfg, newCfg *model.Config) {
+			called <- true
+		}
+		ds.AddListener(callback)
+
+		err = ds.Load()
+		require.NoError(t, err)
+
+		select {
+		case <-called:
+		case <-time.After(5 * time.Second):
+			t.Fatal("callback should have been called when config loaded")
+		}
+	})
+}
+
+func TestDatabaseStoreSave(t *testing.T) {
+	_, tearDown := setupConfigDatabase(t, minimalConfig)
+	defer tearDown()
+
+	ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+	require.NoError(t, err)
+	defer ds.Close()
+
+	newCfg := &model.Config{
+		ServiceSettings: model.ServiceSettings{
+			SiteURL: sToP("http://new"),
+		},
+	}
+
+	t.Run("set without save", func(t *testing.T) {
+		_, err = ds.Set(newCfg)
+		require.NoError(t, err)
+
+		err = ds.Load()
+		require.NoError(t, err)
+
+		assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL)
+	})
+
+	t.Run("set with save", func(t *testing.T) {
+		_, err = ds.Set(newCfg)
+		require.NoError(t, err)
+
+		err = ds.Save()
+		require.NoError(t, err)
+
+		err = ds.Load()
+		require.NoError(t, err)
+
+		assert.Equal(t, "http://new", *ds.Get().ServiceSettings.SiteURL)
+	})
+}
+
+func TestDatabaseStoreString(t *testing.T) {
+	_, tearDown := setupConfigDatabase(t, emptyConfig)
+	defer tearDown()
+
+	ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+	require.NoError(t, err)
+	defer ds.Close()
+
+	actualStringURL, err := url.Parse(ds.String())
+	require.NoError(t, err)
+
+	assert.Equal(t, *mainHelper.Settings.DriverName, actualStringURL.Scheme)
+	actualUsername := actualStringURL.User.Username()
+	actualPassword, _ := actualStringURL.User.Password()
+	assert.NotEmpty(t, actualUsername)
+	assert.Empty(t, actualPassword, "should mask password")
+}

config/export_test.go

@@ -0,0 +1,23 @@
+package config
+
+import (
+	"io"
+
+	"github.com/jmoiron/sqlx"
+	"github.com/mattermost/mattermost-server/model"
+)
+
+// MarshalConfig exposes the internal marshalConfig to tests only.
+func MarshalConfig(cfg *model.Config) ([]byte, error) {
+	return marshalConfig(cfg)
+}
+
+// UnmarshalConfig exposes the internal unmarshalConfig to tests only.
+func UnmarshalConfig(r io.Reader, allowEnvironmentOverrides bool) (*model.Config, map[string]interface{}, error) {
+	return unmarshalConfig(r, allowEnvironmentOverrides)
+}
+
+// InitializeConfigurationsTable exposes the internal initializeConfigurationsTable to test only.
+func InitializeConfigurationsTable(db *sqlx.DB) error {
+	return initializeConfigurationsTable(db)
+}

config/file.go

@@ -10,7 +10,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"sync"
 
 	"github.com/pkg/errors"
 
@@ -25,14 +24,11 @@ var (
 
 // FileStore is a config store backed by a file such as config/config.json.
 type FileStore struct {
-	emitter
+	commonStore
 
-	config               *model.Config
-	environmentOverrides map[string]interface{}
-	configLock           sync.RWMutex
-	path                 string
-	watch                bool
-	watcher              *watcher
+	path    string
+	watch   bool
+	watcher *watcher
 }
 
 // NewFileStore creates a new instance of a config store backed by the given file path.
@@ -90,68 +86,15 @@ func resolveConfigFilePath(path string) (string, error) {
 	return "", fmt.Errorf("failed to find config file %s", path)
 }
 
-// Get fetches the current, cached configuration.
-func (fs *FileStore) Get() *model.Config {
-	fs.configLock.RLock()
-	defer fs.configLock.RUnlock()
-
-	return fs.config
-}
-
-// GetEnvironmentOverrides fetches the configuration fields overridden by environment variables.
-func (fs *FileStore) GetEnvironmentOverrides() map[string]interface{} {
-	fs.configLock.RLock()
-	defer fs.configLock.RUnlock()
-
-	return fs.environmentOverrides
-}
-
 // Set replaces the current configuration in its entirety, without updating the backing store.
 func (fs *FileStore) Set(newCfg *model.Config) (*model.Config, error) {
-	fs.configLock.Lock()
-	var unlockOnce sync.Once
-	defer unlockOnce.Do(fs.configLock.Unlock)
+	return fs.commonStore.set(newCfg, func(cfg *model.Config) error {
+		if *fs.config.ClusterSettings.Enable && *fs.config.ClusterSettings.ReadOnlyConfig {
+			return ErrReadOnlyConfiguration
+		}
 
-	oldCfg := fs.config
-
-	// TODO: disallow attempting to save a directly modified config (comparing pointers). This
-	// wouldn't be an exhaustive check, given the use of pointers throughout the data
-	// structure, but might prevent common mistakes. Requires upstream changes first.
-	// if newCfg == oldCfg {
-	// 	return nil, errors.New("old configuration modified instead of cloning")
-	// }
-
-	newCfg = newCfg.Clone()
-	newCfg.SetDefaults()
-
-	// Sometimes the config is received with "fake" data in sensitive fields. Apply the real
-	// data from the existing config as necessary.
-	desanitize(oldCfg, newCfg)
-
-	if err := newCfg.IsValid(); err != nil {
-		return nil, errors.Wrap(err, "new configuration is invalid")
-	}
-
-	if *oldCfg.ClusterSettings.Enable && *oldCfg.ClusterSettings.ReadOnlyConfig {
-		return nil, ErrReadOnlyConfiguration
-	}
-
-	// Ideally, Set would persist automatically and abstract this completely away from the
-	// client. Doing so requires a few upstream changes first, so for now an explicit Save()
-	// remains required.
-	// if err := fs.persist(newCfg); err != nil {
-	// 	return nil, errors.Wrap(err, "failed to persist")
-	// }
-
-	fs.config = newCfg
-
-	unlockOnce.Do(fs.configLock.Unlock)
-
-	// Notify listeners synchronously. Ideally, this would be asynchronous, but existing code
-	// assumes this and there would be increased complexity to avoid racing updates.
-	fs.invokeConfigListeners(oldCfg, newCfg)
-
-	return oldCfg, nil
+		return nil
+	})
 }
 
 // persist writes the configuration to the configured file.
@@ -206,50 +149,7 @@ func (fs *FileStore) Load() (err error) {
 		}
 	}()
 
-	allowEnvironmentOverrides := true
-	loadedCfg, environmentOverrides, err := unmarshalConfig(f, allowEnvironmentOverrides)
-	if err != nil {
-		return errors.Wrapf(err, "failed to unmarshal config from %s", fs.path)
-	}
-
-	// SetDefaults generates various keys and salts if not previously configured. Determine if
-	// such a change will be made before invoking. This method will not effect the save: that
-	// remains the responsibility of the caller.
-	needsSave = needsSave || loadedCfg.SqlSettings.AtRestEncryptKey == nil || len(*loadedCfg.SqlSettings.AtRestEncryptKey) == 0
-	needsSave = needsSave || loadedCfg.FileSettings.PublicLinkSalt == nil || len(*loadedCfg.FileSettings.PublicLinkSalt) == 0
-	needsSave = needsSave || loadedCfg.EmailSettings.InviteSalt == nil || len(*loadedCfg.EmailSettings.InviteSalt) == 0
-
-	loadedCfg.SetDefaults()
-
-	if err := loadedCfg.IsValid(); err != nil {
-		return errors.Wrap(err, "invalid config")
-	}
-
-	if changed := fixConfig(loadedCfg); changed {
-		needsSave = true
-	}
-
-	fs.configLock.Lock()
-	var unlockOnce sync.Once
-	defer unlockOnce.Do(fs.configLock.Unlock)
-
-	if needsSave {
-		if err = fs.persist(loadedCfg); err != nil {
-			return errors.Wrap(err, "failed to persist required changes after load")
-		}
-	}
-
-	oldCfg := fs.config
-	fs.config = loadedCfg
-	fs.environmentOverrides = environmentOverrides
-
-	unlockOnce.Do(fs.configLock.Unlock)
-
-	// Notify listeners synchronously. Ideally, this would be asynchronous, but existing code
-	// assumes this and there would be increased complexity to avoid racing updates.
-	fs.invokeConfigListeners(oldCfg, loadedCfg)
-
-	return nil
+	return fs.commonStore.load(f, needsSave, fs.persist)
 }
 
 // Save writes the current configuration to the backing store.

config/file_test.go

@@ -104,36 +104,32 @@ func setupConfigFile(t *testing.T, cfg *model.Config) (string, func()) {
 	}
 }
 
-// assertFileEqualsConfig verifies the on disk contents of the given path equal the given config.
-func assertFileEqualsConfig(t *testing.T, expectedCfg *model.Config, path string) {
+// getActualFileConfig returns the configuration present in the given file without relying a config store.
+func getActualFileConfig(t *testing.T, path string) *model.Config {
+	t.Helper()
+
 	f, err := os.Open(path)
 	require.Nil(t, err)
-
-	// These fields require special initialization for our tests.
-	expectedCfg = expectedCfg.Clone()
-	expectedCfg.MessageExportSettings.GlobalRelaySettings = &model.GlobalRelayMessageExportSettings{}
-	expectedCfg.PluginSettings.Plugins = make(map[string]map[string]interface{})
-	expectedCfg.PluginSettings.PluginStates = make(map[string]*model.PluginState)
+	defer f.Close()
 
 	actualCfg, _, err := config.UnmarshalConfig(f, false)
 	require.Nil(t, err)
 
+	return actualCfg
+}
+
+// assertFileEqualsConfig verifies the on disk contents of the given path equal the given config.
+func assertFileEqualsConfig(t *testing.T, expectedCfg *model.Config, path string) {
+	expectedCfg = prepareExpectedConfig(t, expectedCfg)
+	actualCfg := getActualFileConfig(t, path)
+
 	assert.Equal(t, expectedCfg, actualCfg)
 }
 
 // assertFileNotEqualsConfig verifies the on disk contents of the given path does not equal the given config.
 func assertFileNotEqualsConfig(t *testing.T, expectedCfg *model.Config, path string) {
-	f, err := os.Open(path)
-	require.Nil(t, err)
-
-	// These fields require special initialization for our tests.
-	expectedCfg = expectedCfg.Clone()
-	expectedCfg.MessageExportSettings.GlobalRelaySettings = &model.GlobalRelayMessageExportSettings{}
-	expectedCfg.PluginSettings.Plugins = make(map[string]map[string]interface{})
-	expectedCfg.PluginSettings.PluginStates = make(map[string]*model.PluginState)
-
-	actualCfg, _, err := config.UnmarshalConfig(f, false)
-	require.Nil(t, err)
+	expectedCfg = prepareExpectedConfig(t, expectedCfg)
+	actualCfg := getActualFileConfig(t, path)
 
 	assert.NotEqual(t, expectedCfg, actualCfg)
 }

config/main_test.go

@@ -1,17 +1,45 @@
-package config
+package config_test
 
 import (
-	"io"
+	"testing"
 
+	"github.com/go-sql-driver/mysql"
 	"github.com/mattermost/mattermost-server/model"
+	"github.com/mattermost/mattermost-server/testlib"
+	"github.com/stretchr/testify/require"
 )
 
-// MarshalConfig exposes the internal marshalConfig to tests only.
-func MarshalConfig(cfg *model.Config) ([]byte, error) {
-	return marshalConfig(cfg)
+var mainHelper *testlib.MainHelper
+
+func TestMain(m *testing.M) {
+	mainHelper = testlib.NewMainHelper()
+	defer mainHelper.Close()
+
+	mainHelper.Main(m)
 }
 
-// UnmarshalConfig exposes the internal unmarshalConfig to tests only.
-func UnmarshalConfig(r io.Reader, allowEnvironmentOverrides bool) (*model.Config, map[string]interface{}, error) {
-	return unmarshalConfig(r, allowEnvironmentOverrides)
+// truncateTables clears tables used by the config package for reuse in other tests
+func truncateTables(t *testing.T) {
+	t.Helper()
+
+	switch *mainHelper.Settings.DriverName {
+	case model.DATABASE_DRIVER_MYSQL:
+		_, err := mainHelper.SqlSupplier.GetMaster().Db.Exec("TRUNCATE TABLE Configurations")
+		if err != nil {
+			if driverErr, ok := err.(*mysql.MySQLError); ok {
+				// Ignore if the Configurations table does not exist.
+				if driverErr.Number == 1146 {
+					return
+				}
+			}
+		}
+		require.NoError(t, err)
+
+	case model.DATABASE_DRIVER_POSTGRES:
+		_, err := mainHelper.SqlSupplier.GetMaster().Db.Exec("TRUNCATE TABLE Configurations")
+		require.NoError(t, err)
+
+	default:
+		t.Fatalf("unsupported driver name: %s", *mainHelper.Settings.DriverName)
+	}
 }

config/store.go

@@ -4,6 +4,8 @@
 package config
 
 import (
+	"strings"
+
 	"github.com/mattermost/mattermost-server/model"
 )
 
@@ -39,3 +41,12 @@ type Store interface {
 	// Close cleans up resources associated with the store.
 	Close() error
 }
+
+// NewStore creates a database or file store given a data source name by which to connect.
+func NewStore(dsn string, watch bool) (Store, error) {
+	if strings.HasPrefix(dsn, "mysql://") || strings.HasPrefix(dsn, "postgres://") {
+		return NewDatabaseStore(dsn)
+	}
+
+	return NewFileStore(dsn, watch)
+}

config/store_test.go

@@ -0,0 +1,35 @@
+package config_test
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/mattermost/mattermost-server/config"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewStore(t *testing.T) {
+	t.Run("database dsn", func(t *testing.T) {
+		ds, err := config.NewStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource), false)
+		require.NoError(t, err)
+		ds.Close()
+	})
+
+	t.Run("database dsn, watch ignored", func(t *testing.T) {
+		ds, err := config.NewStore(fmt.Sprintf("%s://%s", *mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource), true)
+		require.NoError(t, err)
+		ds.Close()
+	})
+
+	t.Run("file dsn", func(t *testing.T) {
+		fs, err := config.NewStore("config.json", false)
+		require.NoError(t, err)
+		fs.Close()
+	})
+
+	t.Run("file dsn, watch", func(t *testing.T) {
+		fs, err := config.NewStore("config.json", true)
+		require.NoError(t, err)
+		fs.Close()
+	})
+}