Merge branch 'master' into mark-as-unread

2019-11-04 15:04:28 -05:00
--- a/app/bot.go
+++ b/app/bot.go
@@ -77,10 +77,15 @@ func (a *App) PatchBot(botUserId string, botPatch *model.BotPatch) (*model.Bot,
 	user.Username = patchedUser.Username
 	user.Email = patchedUser.Email
 	user.FirstName = patchedUser.FirstName
-	if _, err := a.Srv.Store.User().Update(user, true); err != nil {
+
+	userUpdate, err := a.Srv.Store.User().Update(user, true)
+	if err != nil {
 		return nil, err
 	}

+	ruser := userUpdate.New
+	a.sendUpdatedUserEvent(*ruser)
+
 	return a.Srv.Store.Bot().Update(bot)
 }

--- a/app/bot_test.go
+++ b/app/bot_test.go
@@ -172,6 +172,9 @@ func TestPatchBot(t *testing.T) {
 		patchedBot, err := th.App.PatchBot(createdBot.UserId, botPatch)
 		require.Nil(t, err)

+		// patchedBot should create a new .UpdateAt time
+		require.NotEqual(t, createdBot.UpdateAt, patchedBot.UpdateAt)
+
 		createdBot.Username = "username2"
 		createdBot.DisplayName = "updated bot"
 		createdBot.Description = "an updated bot"
--- a/app/config.go
+++ b/app/config.go
@@ -138,8 +138,10 @@ func (a *App) ensurePostActionCookieSecret() error {
 			return err
 		}
 		system.Value = string(v)
-		if err = a.Srv.Store.System().Save(system); err == nil {
-			// If we were able to save the key, use it, otherwise ignore the error.
+		// If we were able to save the key, use it, otherwise log the error.
+		if appErr := a.Srv.Store.System().Save(system); appErr != nil {
+			mlog.Error("Failed to save PostActionCookieSecret", mlog.Err(appErr))
+		} else {
 			secret = newSecret
 		}
 	}
@@ -199,8 +201,10 @@ func (a *App) ensureAsymmetricSigningKey() error {
 			return err
 		}
 		system.Value = string(v)
-		if err = a.Srv.Store.System().Save(system); err == nil {
-			// If we were able to save the key, use it, otherwise ignore the error.
+		// If we were able to save the key, use it, otherwise log the error.
+		if appErr := a.Srv.Store.System().Save(system); appErr != nil {
+			mlog.Error("Failed to save AsymmetricSigningKey", mlog.Err(appErr))
+		} else {
 			key = newKey
 		}
 	}
--- a/app/plugin_api.go
+++ b/app/plugin_api.go
@@ -695,6 +695,10 @@ func (api *PluginAPI) InstallPlugin(file io.Reader, replace bool) (*model.Manife

 // KV Store Section

+func (api *PluginAPI) KVSetWithOptions(key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) {
+	return api.app.SetPluginKeyWithOptions(api.id, key, value, options)
+}
+
 func (api *PluginAPI) KVSet(key string, value []byte) *model.AppError {
 	return api.app.SetPluginKey(api.id, key, value)
 }
--- a/app/plugin_key_value_store.go
+++ b/app/plugin_key_value_store.go
@@ -23,40 +23,30 @@ func (a *App) SetPluginKey(pluginId string, key string, value []byte) *model.App
 }

 func (a *App) SetPluginKeyWithExpiry(pluginId string, key string, value []byte, expireInSeconds int64) *model.AppError {
-	if expireInSeconds > 0 {
-		expireInSeconds = model.GetMillis() + (expireInSeconds * 1000)
+	options := model.PluginKVSetOptions{
+		ExpireInSeconds: expireInSeconds,
 	}
-
-	kv := &model.PluginKeyValue{
-		PluginId: pluginId,
-		Key:      key,
-		Value:    value,
-		ExpireAt: expireInSeconds,
-	}
-
-	if _, err := a.Srv.Store.Plugin().SaveOrUpdate(kv); err != nil {
-		mlog.Error("Failed to set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
-		return err
-	}
-
-	// Clean up a previous entry using the hashed key, if it exists.
-	if err := a.Srv.Store.Plugin().Delete(pluginId, getKeyHash(key)); err != nil {
-		mlog.Error("Failed to clean up previously hashed plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
-	}
-
-	return nil
+	_, err := a.SetPluginKeyWithOptions(pluginId, key, value, options)
+	return err
 }

 func (a *App) CompareAndSetPluginKey(pluginId string, key string, oldValue, newValue []byte) (bool, *model.AppError) {
-	kv := &model.PluginKeyValue{
-		PluginId: pluginId,
-		Key:      key,
-		Value:    newValue,
+	options := model.PluginKVSetOptions{
+		Atomic:   true,
+		OldValue: oldValue,
+	}
+	return a.SetPluginKeyWithOptions(pluginId, key, newValue, options)
+}
+
+func (a *App) SetPluginKeyWithOptions(pluginId string, key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) {
+	if err := options.IsValid(); err != nil {
+		mlog.Error("Failed to set plugin key value with options", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
+		return false, err
 	}

-	updated, err := a.Srv.Store.Plugin().CompareAndSet(kv, oldValue)
+	updated, err := a.Srv.Store.Plugin().SetWithOptions(pluginId, key, value, options)
 	if err != nil {
-		mlog.Error("Failed to compare and set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
+		mlog.Error("Failed to set plugin key value with options", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
 		return updated, err
 	}

--- a/app/plugin_test.go
+++ b/app/plugin_test.go
@@ -16,11 +16,12 @@ import (
 	"testing"

 	"github.com/gorilla/mux"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/plugin"
 	"github.com/mattermost/mattermost-server/utils/fileutils"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )

 func getHashedKey(key string) string {
@@ -186,6 +187,147 @@ func TestPluginKeyValueStoreCompareAndSet(t *testing.T) {
 	assert.Equal(t, []byte("test2"), ret)
 }

+func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	pluginId := "testpluginid"
+
+	defer func() {
+		assert.Nil(t, th.App.DeletePluginKey(pluginId, "key"))
+	}()
+
+	t.Run("fails with a non-serializable object as the new value", func(t *testing.T) {
+		result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", func() {}, model.PluginKVSetOptions{
+			EncodeJSON: true,
+		})
+		assert.False(t, result)
+		assert.NotNil(t, err)
+
+		// verify that after the failure it was not set
+		ret, err := th.App.GetPluginKey(pluginId, "key")
+		assert.Nil(t, err)
+		assert.Equal(t, []byte(nil), ret)
+	})
+
+	t.Run("fails with a non-serializable object as the old value", func(t *testing.T) {
+		result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{
+			"val-a": 10,
+		}, model.PluginKVSetOptions{
+			EncodeJSON: true,
+			Atomic:     true,
+			OldValue:   func() {},
+		})
+		assert.False(t, result)
+		assert.NotNil(t, err)
+
+		// verify that after the failure it was not set
+		ret, err := th.App.GetPluginKey(pluginId, "key")
+		assert.Nil(t, err)
+		assert.Equal(t, []byte(nil), ret)
+	})
+
+	t.Run("storing a value json encoded works", func(t *testing.T) {
+		result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{
+			"val-a": 10,
+		}, model.PluginKVSetOptions{
+			EncodeJSON: true,
+		})
+		assert.True(t, result)
+		assert.Nil(t, err)
+
+		// and I can get it back!
+		ret, err := th.App.GetPluginKey(pluginId, "key")
+		assert.Nil(t, err)
+		assert.Equal(t, []byte(`{"val-a":10}`), ret)
+	})
+
+	t.Run("test that setting it atomic when it doesn't match doesn't change anything", func(t *testing.T) {
+		result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{
+			"val-a": 30,
+		}, model.PluginKVSetOptions{
+			EncodeJSON: true,
+			Atomic:     true,
+			OldValue: map[string]interface{}{
+				"val-a": 20,
+			},
+		})
+		assert.False(t, result)
+		assert.Nil(t, err)
+
+		// test that the value didn't change
+		ret, err := th.App.GetPluginKey(pluginId, "key")
+		assert.Nil(t, err)
+		assert.Equal(t, []byte(`{"val-a":10}`), ret)
+	})
+
+	t.Run("test the atomic change with the proper old value", func(t *testing.T) {
+		result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{
+			"val-a": 30,
+		}, model.PluginKVSetOptions{
+			EncodeJSON: true,
+			Atomic:     true,
+			OldValue: map[string]interface{}{
+				"val-a": 10,
+			},
+		})
+		assert.True(t, result)
+		assert.Nil(t, err)
+
+		// test that the value did change
+		ret, err := th.App.GetPluginKey(pluginId, "key")
+		assert.Nil(t, err)
+		assert.Equal(t, []byte(`{"val-a":30}`), ret)
+	})
+}
+
+func TestPluginKeyValueStoreSetWithOptionsByteArray(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	pluginId := "testpluginid"
+
+	defer func() {
+		assert.Nil(t, th.App.DeletePluginKey(pluginId, "key"))
+	}()
+
+	// storing a value works
+	result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`myvalue`), model.PluginKVSetOptions{})
+	assert.True(t, result)
+	assert.Nil(t, err)
+
+	// and I can get it back!
+	ret, err := th.App.GetPluginKey(pluginId, "key")
+	assert.Nil(t, err)
+	assert.Equal(t, []byte(`myvalue`), ret)
+
+	// test that setting it atomic when it doesn't match doesn't change anything
+	result, err = th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`newvalue`), model.PluginKVSetOptions{
+		Atomic:   true,
+		OldValue: []byte(`differentvalue`),
+	})
+	assert.False(t, result)
+	assert.Nil(t, err)
+
+	// test that the value didn't change
+	ret, err = th.App.GetPluginKey(pluginId, "key")
+	assert.Nil(t, err)
+	assert.Equal(t, []byte(`myvalue`), ret)
+
+	// now do the atomic change with the proper old value
+	result, err = th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`newvalue`), model.PluginKVSetOptions{
+		Atomic:   true,
+		OldValue: []byte(`myvalue`),
+	})
+	assert.True(t, result)
+	assert.Nil(t, err)
+
+	// test that the value did change
+	ret, err = th.App.GetPluginKey(pluginId, "key")
+	assert.Nil(t, err)
+	assert.Equal(t, []byte(`newvalue`), ret)
+}
+
 func TestServePluginRequest(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()
--- a/app/session.go
+++ b/app/session.go
@@ -72,8 +72,7 @@ func (a *App) GetSession(token string) (*model.Session, *model.AppError) {
 		return nil, model.NewAppError("GetSession", "api.context.invalid_token.error", map[string]interface{}{"Token": token}, "", http.StatusUnauthorized)
 	}

-	if session != nil &&
-		*a.Config().ServiceSettings.SessionIdleTimeoutInMinutes > 0 &&
+	if *a.Config().ServiceSettings.SessionIdleTimeoutInMinutes > 0 &&
 		!session.IsOAuth &&
 		session.Props[model.SESSION_PROP_TYPE] != model.SESSION_TYPE_USER_ACCESS_TOKEN {