Merge branch 'master' into mark-as-unread

app/auto_posts.go

@@ -92,8 +92,8 @@ func (cfg *AutoPostCreator) CreateRandomPostNested(parentId, rootId string) (*mo
 		RootId:    rootId,
 		Message:   postText,
 		FileIds:   fileIds}
-	rpost, err2 := cfg.client.CreatePost(post)
-	if err2 != nil {
+	rpost, resp := cfg.client.CreatePost(post)
+	if resp != nil && resp.Error != nil {
 		return nil, false
 	}
 	return rpost, true

app/channel.go

@@ -160,7 +160,7 @@ func (a *App) CreateChannelWithUser(channel *model.Channel, userId string) (*mod
 	}
 
 	// Get total number of channels on current team
-	count, err := a.GetNumberOfChannelsOnTeam(channel.TeamId, false)
+	count, err := a.GetNumberOfChannelsOnTeam(channel.TeamId)
 	if err != nil {
 		return nil, err
 	}
@@ -1724,23 +1724,12 @@ func (a *App) RemoveUserFromChannel(userIdToRemove string, removerUserId string,
 	return nil
 }
 
-func (a *App) GetNumberOfChannelsOnTeam(teamId string, includeDeleted bool) (int, *model.AppError) {
+func (a *App) GetNumberOfChannelsOnTeam(teamId string) (int, *model.AppError) {
 	// Get total number of channels on current team
 	list, err := a.Srv.Store.Channel().GetTeamChannels(teamId)
 	if err != nil {
 		return 0, err
 	}
-
-	if !includeDeleted {
-		count := 0
-		for _, channel := range *list {
-			if channel.DeleteAt == 0 {
-				count++
-			}
-		}
-		return count, nil
-	}
-
 	return len(*list), nil
 }
 

app/diagnostics.go

@@ -47,6 +47,7 @@ const (
 	TRACK_CONFIG_DATA_RETENTION     = "config_data_retention"
 	TRACK_CONFIG_MESSAGE_EXPORT     = "config_message_export"
 	TRACK_CONFIG_DISPLAY            = "config_display"
+	TRACK_CONFIG_GUEST_ACCOUNTS     = "config_guest_accounts"
 	TRACK_CONFIG_IMAGE_PROXY        = "config_image_proxy"
 	TRACK_PERMISSIONS_GENERAL       = "permissions_general"
 	TRACK_PERMISSIONS_SYSTEM_SCHEME = "permissions_system_scheme"
@@ -509,6 +510,7 @@ func (a *App) trackConfig() {
 		"isdefault_scoping_idp_provider_id":   isDefault(*cfg.SamlSettings.ScopingIDPProviderId, ""),
 		"isdefault_scoping_idp_name":          isDefault(*cfg.SamlSettings.ScopingIDPName, ""),
 		"isdefault_id_attribute":              isDefault(*cfg.SamlSettings.IdAttribute, model.SAML_SETTINGS_DEFAULT_ID_ATTRIBUTE),
+		"isdefault_guest_attribute":           isDefault(*cfg.SamlSettings.GuestAttribute, model.SAML_SETTINGS_DEFAULT_GUEST_ATTRIBUTE),
 		"isdefault_first_name_attribute":      isDefault(*cfg.SamlSettings.FirstNameAttribute, model.SAML_SETTINGS_DEFAULT_FIRST_NAME_ATTRIBUTE),
 		"isdefault_last_name_attribute":       isDefault(*cfg.SamlSettings.LastNameAttribute, model.SAML_SETTINGS_DEFAULT_LAST_NAME_ATTRIBUTE),
 		"isdefault_email_attribute":           isDefault(*cfg.SamlSettings.EmailAttribute, model.SAML_SETTINGS_DEFAULT_EMAIL_ATTRIBUTE),
@@ -630,6 +632,13 @@ func (a *App) trackConfig() {
 		"isdefault_custom_url_schemes": len(cfg.DisplaySettings.CustomUrlSchemes) != 0,
 	})
 
+	a.SendDiagnostic(TRACK_CONFIG_GUEST_ACCOUNTS, map[string]interface{}{
+		"enable":                                 *cfg.GuestAccountsSettings.Enable,
+		"allow_email_accounts":                   *cfg.GuestAccountsSettings.AllowEmailAccounts,
+		"enforce_multifactor_authentication":     *cfg.GuestAccountsSettings.EnforceMultifactorAuthentication,
+		"isdefault_restrict_creation_to_domains": isDefault(*cfg.GuestAccountsSettings.RestrictCreationToDomains, ""),
+	})
+
 	a.SendDiagnostic(TRACK_CONFIG_IMAGE_PROXY, map[string]interface{}{
 		"enable":                               *cfg.ImageProxySettings.Enable,
 		"image_proxy_type":                     *cfg.ImageProxySettings.ImageProxyType,

app/job_test.go

@@ -23,11 +23,9 @@ func TestGetJob(t *testing.T) {
 
 	defer th.App.Srv.Store.Job().Delete(status.Id)
 
-	if received, err := th.App.GetJob(status.Id); err != nil {
-		t.Fatal(err)
-	} else if received.Id != status.Id || received.Status != status.Status {
-		t.Fatal("inccorrect job status received")
-	}
+	received, err := th.App.GetJob(status.Id)
+	require.Nil(t, err)
+	require.Equal(t, status, received, "incorrect job status received")
 }
 
 func TestGetJobByType(t *testing.T) {
@@ -60,21 +58,14 @@ func TestGetJobByType(t *testing.T) {
 		defer th.App.Srv.Store.Job().Delete(status.Id)
 	}
 
-	if received, err := th.App.GetJobsByType(jobType, 0, 2); err != nil {
-		t.Fatal(err)
-	} else if len(received) != 2 {
-		t.Fatal("received wrong number of statuses")
-	} else if received[0].Id != statuses[2].Id {
-		t.Fatal("should've received newest job first")
-	} else if received[1].Id != statuses[0].Id {
-		t.Fatal("should've received second newest job second")
-	}
+	received, err := th.App.GetJobsByType(jobType, 0, 2)
+	require.Nil(t, err)
+	require.Len(t, received, 2, "received wrong number of statuses")
+	require.Equal(t, statuses[2], received[0], "should've received newest job first")
+	require.Equal(t, statuses[0], received[1], "should've received second newest job second")
 
-	if received, err := th.App.GetJobsByType(jobType, 2, 2); err != nil {
-		t.Fatal(err)
-	} else if len(received) != 1 {
-		t.Fatal("received wrong number of statuses")
-	} else if received[0].Id != statuses[1].Id {
-		t.Fatal("should've received oldest job last")
-	}
+	received, err = th.App.GetJobsByType(jobType, 2, 2)
+	require.Nil(t, err)
+	require.Len(t, received, 1, "received wrong number of statuses")
+	require.Equal(t, statuses[1], received[0], "should've received oldest job last")
 }

app/plugin.go

@@ -43,6 +43,7 @@ func (a *App) SetPluginsEnvironment(pluginsEnvironment *plugin.Environment) {
 }
 
 func (a *App) SyncPluginsActiveState() {
+	// Acquiring lock manually, as plugins might be disabled. See GetPluginsEnvironment.
 	a.Srv.PluginsLock.RLock()
 	pluginsEnvironment := a.Srv.PluginsEnvironment
 	a.Srv.PluginsLock.RUnlock()
@@ -124,6 +125,7 @@ func (a *App) NewPluginAPI(manifest *model.Manifest) plugin.API {
 }
 
 func (a *App) InitPlugins(pluginDir, webappPluginDir string) {
+	// Acquiring lock manually, as plugins might be disabled. See GetPluginsEnvironment.
 	a.Srv.PluginsLock.RLock()
 	pluginsEnvironment := a.Srv.PluginsEnvironment
 	a.Srv.PluginsLock.RUnlock()
@@ -257,9 +259,7 @@ func (a *App) SyncPlugins() *model.AppError {
 }
 
 func (a *App) ShutDownPlugins() {
-	a.Srv.PluginsLock.Lock()
-	pluginsEnvironment := a.Srv.PluginsEnvironment
-	defer a.Srv.PluginsLock.Unlock()
+	pluginsEnvironment := a.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
 		return
 	}
@@ -270,7 +270,15 @@ func (a *App) ShutDownPlugins() {
 
 	a.RemoveConfigListener(a.Srv.PluginConfigListenerId)
 	a.Srv.PluginConfigListenerId = ""
-	a.Srv.PluginsEnvironment = nil
+
+	// Acquiring lock manually before cleaning up PluginsEnvironment.
+	a.Srv.PluginsLock.Lock()
+	defer a.Srv.PluginsLock.Unlock()
+	if a.Srv.PluginsEnvironment == pluginsEnvironment {
+		a.Srv.PluginsEnvironment = nil
+	} else {
+		mlog.Warn("Another PluginsEnvironment detected while shutting down plugins.")
+	}
 }
 
 func (a *App) GetActivePluginManifests() ([]*model.Manifest, *model.AppError) {

app/plugin_api.go

@@ -466,7 +466,7 @@ func (api *PluginAPI) DeletePost(postId string) *model.AppError {
 }
 
 func (api *PluginAPI) GetPostThread(postId string) (*model.PostList, *model.AppError) {
-	return api.app.GetPostThread(postId)
+	return api.app.GetPostThread(postId, false)
 }
 
 func (api *PluginAPI) GetPost(postId string) (*model.Post, *model.AppError) {

app/plugin_deadlock_test.go

@@ -4,13 +4,14 @@
 package app
 
 import (
-	"github.com/stretchr/testify/require"
 	"os"
 	"strings"
 	"testing"
 	"text/template"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/mattermost/mattermost-server/model"
 )
 
@@ -209,4 +210,112 @@ func TestPluginDeadlock(t *testing.T) {
 			}()
 		}
 	})
+
+	t.Run("CreatePost on OnDeactivate Plugin", func(t *testing.T) {
+		th := Setup(t).InitBasic()
+
+		pluginPostOnActivate := template.Must(template.New("pluginPostOnActivate").Parse(`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/plugin"
+				"github.com/mattermost/mattermost-server/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) OnDeactivate() error {
+				_, err := p.API.CreatePost(&model.Post{
+					UserId: "{{.User.Id}}",
+					ChannelId: "{{.Channel.Id}}",
+					Message:   "OnDeactivate",
+				})
+				if err != nil {
+					panic(err.Error())
+				}
+
+				return nil
+			}
+
+			func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) {
+				updatedPost := &model.Post{
+					UserId: "{{.User.Id}}",
+					ChannelId: "{{.Channel.Id}}",
+					Message:   "messageUpdated",
+					Props: map[string]interface{}{
+						"from_plugin": true,
+					},
+				}
+
+				return updatedPost, ""
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+`,
+		))
+
+		templateData := struct {
+			User    *model.User
+			Channel *model.Channel
+		}{
+			th.BasicUser,
+			th.BasicChannel,
+		}
+
+		plugins := []string{}
+		pluginTemplates := []*template.Template{
+			pluginPostOnActivate,
+		}
+		for _, pluginTemplate := range pluginTemplates {
+			b := &strings.Builder{}
+			pluginTemplate.Execute(b, templateData)
+
+			plugins = append(plugins, b.String())
+		}
+
+		done := make(chan bool)
+		go func() {
+			posts, appErr := th.App.GetPosts(th.BasicChannel.Id, 0, 2)
+			require.Nil(t, appErr)
+			require.NotNil(t, posts)
+
+			messageWillBePostedCalled := false
+			for _, p := range posts.Posts {
+				if p.Message == "messageUpdated" {
+					messageWillBePostedCalled = true
+				}
+			}
+			require.False(t, messageWillBePostedCalled, "MessageWillBePosted should not have been called")
+
+			SetAppEnvironmentWithPlugins(t, plugins, th.App, th.App.NewPluginAPI)
+			th.TearDown()
+
+			posts, appErr = th.App.GetPosts(th.BasicChannel.Id, 0, 2)
+			require.Nil(t, appErr)
+			require.NotNil(t, posts)
+
+			messageWillBePostedCalled = false
+			for _, p := range posts.Posts {
+				if p.Message == "messageUpdated" {
+					messageWillBePostedCalled = true
+				}
+			}
+			require.True(t, messageWillBePostedCalled, "MessageWillBePosted was not called on deactivate")
+			close(done)
+		}()
+
+		select {
+		case <-done:
+		case <-time.After(30 * time.Second):
+			require.Fail(t, "plugin failed to activate: likely deadlocked")
+			go func() {
+				time.Sleep(5 * time.Second)
+				os.Exit(1)
+			}()
+		}
+	})
 }

app/plugin_install.go

@@ -251,6 +251,10 @@ func (a *App) removePlugin(id string) *model.AppError {
 		},
 	)
 
+	if err := a.notifyPluginStatusesChanged(); err != nil {
+		mlog.Error("Failed to notify plugin status changed", mlog.Err(err))
+	}
+
 	return nil
 }
 

app/post.go

@@ -167,7 +167,7 @@ func (a *App) CreatePost(post *model.Post, channel *model.Channel, triggerWebhoo
 	if len(post.RootId) > 0 {
 		pchan = make(chan store.StoreResult, 1)
 		go func() {
-			r, pErr := a.Srv.Store.Post().Get(post.RootId, true)
+			r, pErr := a.Srv.Store.Post().Get(post.RootId, false)
 			pchan <- store.StoreResult{Data: r, Err: pErr}
 			close(pchan)
 		}()
@@ -475,7 +475,7 @@ func (a *App) DeleteEphemeralPost(userId, postId string) {
 func (a *App) UpdatePost(post *model.Post, safeUpdate bool) (*model.Post, *model.AppError) {
 	post.SanitizeProps()
 
-	postLists, err := a.Srv.Store.Post().Get(post.Id, true)
+	postLists, err := a.Srv.Store.Post().Get(post.Id, false)
 	if err != nil {
 		return nil, err
 	}
@@ -634,8 +634,8 @@ func (a *App) GetSinglePost(postId string) (*model.Post, *model.AppError) {
 	return a.Srv.Store.Post().GetSingle(postId)
 }
 
-func (a *App) GetPostThread(postId string) (*model.PostList, *model.AppError) {
-	return a.Srv.Store.Post().Get(postId, false)
+func (a *App) GetPostThread(postId string, skipFetchThreads bool) (*model.PostList, *model.AppError) {
+	return a.Srv.Store.Post().Get(postId, skipFetchThreads)
 }
 
 func (a *App) GetFlaggedPosts(userId string, offset int, limit int) (*model.PostList, *model.AppError) {
@@ -789,7 +789,7 @@ func (a *App) GetPostsForChannelAroundLastUnread(channelId, userId string, limit
 		return model.NewPostList(), nil
 	}
 
-	postList, err := a.GetPostThread(lastUnreadPostId)
+	postList, err := a.GetPostThread(lastUnreadPostId, false)
 	if err != nil {
 		return nil, err
 	}

app/server_app_adapters.go

@@ -4,7 +4,6 @@
 package app
 
 import (
-	"fmt"
 	"net/http"
 	"net/url"
 	"path"
@@ -67,7 +66,7 @@ func (s *Server) RunOldAppInitalization() error {
 	}
 
 	if htmlTemplateWatcher, err := utils.NewHTMLTemplateWatcher("templates"); err != nil {
-		mlog.Error(fmt.Sprintf("Failed to parse server templates %v", err))
+		mlog.Error("Failed to parse server templates", mlog.Err(err))
 	} else {
 		s.FakeApp().Srv.htmlTemplateWatcher = htmlTemplateWatcher
 	}
@@ -131,7 +130,7 @@ func (s *Server) RunOldAppInitalization() error {
 		appErr = backend.TestConnection()
 	}
 	if appErr != nil {
-		mlog.Error("Problem with file storage settings: " + appErr.Error())
+		mlog.Error("Problem with file storage settings", mlog.Err(appErr))
 	}
 
 	if model.BuildEnterpriseReady == "true" {