MM-58438: simplify settings (#688)

* always sync reactions (when chat/channel sync enabled)

* always sync file attachments (when chat/channel sync enabled)

* consolidate to experimentalSyncChats

* make selective sync experimental

* clarify syncNotifications

* s/DisableSyncMsg/UseSharedChannels/ and experimental

* make certificates experimental

* remove untested enabledTeams setting

* plugin.json: whitespace

* make disableCheckCredentials internal

* avoid directly mutating p.configuration in tests

plugin.json

@@ -52,18 +52,6 @@
                 "type": "generated",
                 "help_text": "Microsoft Teams will use this secret to send messages to Mattermost"
             },
-            {
-                "key": "certificatePublic",
-                "display_name": "Certificate Public",
-                "type": "longtext",
-                "help_text": "Certificate public part"
-            },
-            {
-                "key": "certificateKey",
-                "display_name": "Certificate Key",
-                "type": "longtext",
-                "help_text": "Certificate private key"
-            },
             {
                 "key": "evaluationAPI",
                 "display_name": "Use the evaluation API pay model",
@@ -89,28 +77,7 @@
                 "key": "syncNotifications",
                 "display_name": "Sync notifications",
                 "type": "bool",
-                "help_text": "Sync notifications for any connected user. Enabling this setting disables syncing of direct messages and group messages.",
-                "default": false
-            },
-            {
-                "key": "syncDirectMessages",
-                "display_name": "Sync direct messages",
-                "type": "bool",
-                "help_text": "Sync direct messages where any of the user in the conversation is a real Mattermost user connected to MS Teams account",
-                "default": false
-            },
-            {
-                "key": "syncGroupMessages",
-                "display_name": "Sync group messages",
-                "type": "bool",
-                "help_text": "Sync group messages where any of the user in the conversation is a real Mattermost user connected to MS Teams account",
-                "default": false
-            },
-            {
-                "key": "selectiveSync",
-                "display_name": "Selective sync",
-                "type": "bool",
-                "help_text": "Only sync chats and group chats to remote users. This avoids dual notifications when two connected Mattermost users are part of a conversation.",
+                "help_text": "Sync notifications of chat messages for any connected user that enables the feature.",
                 "default": false
             },
             {
@@ -120,27 +87,6 @@
                 "help_text": "Sync messages from channels linked between Mattermost and MS Teams",
                 "default": false
             },
-            {
-                "key": "syncReactions",
-                "display_name": "Sync reactions",
-                "type": "bool",
-                "help_text": "Sync reactions on messages",
-                "default": false
-            },
-            {
-                "key": "syncFileAttachments",
-                "display_name": "Sync file attachments",
-                "type": "bool",
-                "help_text": "Sync file attachments on messages",
-                "default": false
-            },
-            {
-                "key": "enabledTeams",
-                "display_name": "Enabled Teams",
-                "type": "text",
-                "help_text": "Mattermost teams where sync is enabled (comma separated Mattermost team names, empty means all enabled)",
-                "default": ""
-            },
             {
                 "key": "maxSizeForCompleteDownload",
                 "display_name": "Maximum size of attachments to support complete one time download (in MB)",
@@ -169,7 +115,7 @@
                 "help_text": "Invite pool size: the maximum number of connection invites that may be pending at a given time. When specified, connection invite direct messages will be sent to users as they become active, up to the maximum specified here. As invited users connect, spaces in the invite pool will open up and more invites will be sent out. Once invited, users may connect at any time. (Set to 0 or leave empty to disable connection invites.)",
                 "default": 0
             },
-			{
+            {
                 "key": "connectedUsersRestricted",
                 "display_name": "New User Connections: Restricted",
                 "type": "bool",
@@ -190,20 +136,6 @@
                 "help_text": "When true, synthetic users will be converted to members when they login for the first time.",
                 "default": false
             },
-            {
-                "key": "disableSyncMsg",
-                "display_name": "Disable using the sync msg infrastructure for tracking message changes",
-                "type": "bool",
-                "help_text": "When true, the plugin will not enable any sync msg infrastructure.",
-                "default": false
-            },
-            {
-                "key": "disableCheckCredentials",
-                "display_name": "Disable periodically checking the validity of the application credentials",
-                "type": "bool",
-                "help_text": "When true, the plugin will not periodically check the validity of the application credentials.",
-                "default": false
-            },
             {
                 "key": "syntheticUserAuthService",
                 "display_name": "Synthetic User Auth Service",

server/api_test.go

@@ -156,9 +156,8 @@ func TestProcessActivity(t *testing.T) {
 	t.Run("encrypted message on encrypted subscription", func(t *testing.T) {
 		th.Reset(t)
 
-		th.p.configuration.CertificateKey = "test"
-		t.Cleanup(func() {
-			th.p.configuration.CertificateKey = ""
+		th.setPluginConfigurationTemporarily(t, func(c *configuration) {
+			c.CertificateKey = "test"
 		})
 
 		activities := []msteams.Activity{
@@ -179,9 +178,8 @@ func TestProcessActivity(t *testing.T) {
 	t.Run("non-encrypted message on encrypted subscription", func(t *testing.T) {
 		th.Reset(t)
 
-		th.p.configuration.CertificateKey = "test"
-		t.Cleanup(func() {
-			th.p.configuration.CertificateKey = ""
+		th.setPluginConfigurationTemporarily(t, func(c *configuration) {
+			c.CertificateKey = "test"
 		})
 
 		activities := []msteams.Activity{

server/command.go

@@ -204,10 +204,6 @@ func (p *Plugin) executeLinkCommand(args *model.CommandArgs, parameters []string
 		return p.cmdError(args, "Invalid link command, please pass the MS Teams team id and channel id as parameters.")
 	}
 
-	if !p.store.CheckEnabledTeamByTeamID(args.TeamId) {
-		return p.cmdError(args, "This team is not enabled for MS Teams sync.")
-	}
-
 	link, err := p.store.GetLinkByChannelID(args.ChannelId)
 	if err == nil && link != nil {
 		return p.cmdError(args, "A link for this channel already exists. Please unlink the channel before you link again with another channel.")
@@ -259,7 +255,7 @@ func (p *Plugin) executeLinkCommand(args *model.CommandArgs, parameters []string
 		return p.cmdError(args, "Error occurred while saving the subscription")
 	}
 
-	if !p.getConfiguration().DisableSyncMsg {
+	if p.getConfiguration().UseSharedChannels {
 		if _, err = p.API.ShareChannel(&model.SharedChannel{
 			ChannelId: channelLink.MattermostChannelID,
 			TeamId:    channelLink.MattermostTeamID,
@@ -312,7 +308,7 @@ func (p *Plugin) executeUnlinkCommand(args *model.CommandArgs) (*model.CommandRe
 			p.API.LogWarn("Unable to delete the subscription from the DB", "subscription_id", subscription.SubscriptionID, "error", err.Error())
 		}
 
-		if !p.getConfiguration().DisableSyncMsg {
+		if p.getConfiguration().UseSharedChannels {
 			if _, err = p.API.UnshareChannel(link.MattermostChannelID); err != nil {
 				p.API.LogWarn("Failed to unshare channel", "channel_id", link.MattermostChannelID, "subscription_id", subscription.SubscriptionID, "error", err.Error())
 			} else {

server/command_test.go

@@ -45,8 +45,6 @@ func TestExecuteUnlinkCommand(t *testing.T) {
 	th.SetupWebsocketClientForUser(t, user1.Id)
 
 	t.Run("invalid channel", func(t *testing.T) {
-		th.p.configuration.DisableSyncMsg = true
-
 		args := &model.CommandArgs{
 			UserId:    user1.Id,
 			ChannelId: model.NewId(),
@@ -58,7 +56,6 @@ func TestExecuteUnlinkCommand(t *testing.T) {
 	})
 
 	t.Run("channel is a dm", func(t *testing.T) {
-		th.p.configuration.DisableSyncMsg = true
 		channel, err := th.p.API.GetDirectChannel(user1.Id, user2.Id)
 		require.Nil(t, err)
 
@@ -73,7 +70,6 @@ func TestExecuteUnlinkCommand(t *testing.T) {
 	})
 
 	t.Run("channel is a gm", func(t *testing.T) {
-		th.p.configuration.DisableSyncMsg = true
 		channel, err := th.p.API.GetGroupChannel([]string{user1.Id, user2.Id, user3.Id})
 		require.Nil(t, err)
 
@@ -88,7 +84,6 @@ func TestExecuteUnlinkCommand(t *testing.T) {
 	})
 
 	t.Run("not a channel admin", func(t *testing.T) {
-		th.p.configuration.DisableSyncMsg = true
 		channel := th.SetupPublicChannel(t, team)
 
 		args := &model.CommandArgs{
@@ -102,7 +97,6 @@ func TestExecuteUnlinkCommand(t *testing.T) {
 	})
 
 	t.Run("not a linked channel", func(t *testing.T) {
-		th.p.configuration.DisableSyncMsg = true
 		channel := th.SetupPublicChannel(t, team, WithMembers(user1))
 		_, appErr := th.p.API.UpdateChannelMemberRoles(channel.Id, user1.Id, model.ChannelAdminRoleId)
 		require.Nil(t, appErr)
@@ -118,7 +112,6 @@ func TestExecuteUnlinkCommand(t *testing.T) {
 	})
 
 	t.Run("successfully unlinked", func(t *testing.T) {
-		th.p.configuration.DisableSyncMsg = true
 		channel := th.SetupPublicChannel(t, team, WithMembers(user1))
 		_, appErr := th.p.API.UpdateChannelMemberRoles(channel.Id, user1.Id, model.ChannelAdminRoleId)
 		require.Nil(t, appErr)
@@ -144,8 +137,11 @@ func TestExecuteUnlinkCommand(t *testing.T) {
 		assertEphemeralResponse(th, t, args, "The MS Teams channel is no longer linked to this Mattermost channel.")
 	})
 
-	t.Run("successfully unlinked, sync msg enabled", func(t *testing.T) {
-		th.p.configuration.DisableSyncMsg = false
+	t.Run("successfully unlinked, use shared channels enabled", func(t *testing.T) {
+		th.setPluginConfigurationTemporarily(t, func(c *configuration) {
+			c.UseSharedChannels = true
+		})
+
 		channel := th.SetupPublicChannel(t, team, WithMembers(user1))
 		_, appErr := th.p.API.UpdateChannelMemberRoles(channel.Id, user1.Id, model.ChannelAdminRoleId)
 		require.Nil(t, appErr)
@@ -824,14 +820,9 @@ func TestExecuteConnectCommand(t *testing.T) {
 			ChannelId: model.NewId(),
 		}
 
-		configuration := th.p.configuration.Clone()
-		configuration.ConnectedUsersAllowed = 0
-		th.p.setConfiguration(configuration)
-		defer func() {
-			configuration := th.p.configuration.Clone()
-			configuration.ConnectedUsersAllowed = 1000
-			th.p.setConfiguration(configuration)
-		}()
+		th.setPluginConfigurationTemporarily(t, func(c *configuration) {
+			c.ConnectedUsersAllowed = 0
+		})
 
 		commandResponse, appErr := th.p.executeConnectCommand(args)
 		require.Nil(t, appErr)

server/configuration.go

@@ -26,18 +26,14 @@ type configuration struct {
 	EncryptionKey                      string `json:"encryptionkey"`
 	EvaluationAPI                      bool   `json:"evaluationapi"`
 	WebhookSecret                      string `json:"webhooksecret"`
-	EnabledTeams                       string `json:"enabledteams"`
 	SyncNotifications                  bool   `json:"syncnotifications"`
-	SyncDirectMessages                 bool   `json:"syncdirectmessages"`
-	SyncGroupMessages                  bool   `json:"syncgroupmessages"`
-	SelectiveSync                      bool   `json:"selectiveSync"`
+	SyncChats                          bool   `json:"experimentalSyncChats"`
+	SelectiveSync                      bool   `json:"experimentalSelectiveSync"`
 	SyncLinkedChannels                 bool   `json:"synclinkedchannels"`
-	SyncReactions                      bool   `json:"syncreactions"`
-	SyncFileAttachments                bool   `json:"syncfileattachments"`
 	SyncUsers                          int    `json:"syncusers"`
 	SyncGuestUsers                     bool   `json:"syncGuestUsers"`
-	CertificatePublic                  string `json:"certificatepublic"`
-	CertificateKey                     string `json:"certificatekey"`
+	CertificatePublic                  string `json:"experimentalcertificatepublic"`
+	CertificateKey                     string `json:"experimentalcertificatekey"`
 	MaxSizeForCompleteDownload         int    `json:"maxSizeForCompleteDownload"`
 	BufferSizeForFileStreaming         int    `json:"bufferSizeForFileStreaming"`
 	ConnectedUsersAllowed              int    `json:"connectedUsersAllowed"`
@@ -46,8 +42,8 @@ type configuration struct {
 	SyntheticUserAuthService           string `json:"syntheticUserAuthService"`
 	SyntheticUserAuthData              string `json:"syntheticUserAuthData"`
 	AutomaticallyPromoteSyntheticUsers bool   `json:"automaticallyPromoteSyntheticUsers"`
-	DisableSyncMsg                     bool   `json:"disableSyncMsg"`
-	DisableCheckCredentials            bool   `json:"disableCheckCredentials"`
+	UseSharedChannels                  bool   `json:"experimentalUseSharedChannels"`
+	DisableCheckCredentials            bool   `json:"internalDisableCheckCredentials"`
 }
 
 func (c *configuration) ProcessConfiguration() {
@@ -56,7 +52,6 @@ func (c *configuration) ProcessConfiguration() {
 	c.ClientSecret = strings.TrimSpace(c.ClientSecret)
 	c.EncryptionKey = strings.TrimSpace(c.EncryptionKey)
 	c.WebhookSecret = strings.TrimSpace(c.WebhookSecret)
-	c.EnabledTeams = strings.TrimSpace(c.EnabledTeams)
 }
 
 func (p *Plugin) validateConfiguration(configuration *configuration) error {

server/handlers.go

@@ -286,8 +286,8 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
 	var userIDs []string
 
 	if chat != nil {
-		if shouldSync, reason := ah.ShouldSyncDMGMChannel(chat); !shouldSync {
-			return reason
+		if !ah.plugin.GetSyncChats() {
+			return metrics.DiscardedReasonChatsDisabled
 		}
 
 		var channel *model.Channel
@@ -332,7 +332,7 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
 		return metrics.DiscardedReasonOther
 	}
 
-	post, skippedFileAttachments, errorFound := ah.msgToPost(channelID, senderID, msg, chat, ah.plugin.GetSyncFileAttachments(), []string{})
+	post, skippedFileAttachments, errorFound := ah.msgToPost(channelID, senderID, msg, chat, true, []string{})
 
 	// Last second check to avoid possible duplication
 	if postInfo, _ = ah.plugin.GetStore().GetPostInfoByMSTeamsID(msg.ChatID+msg.ChannelID, msg.ID); postInfo != nil {
@@ -447,8 +447,8 @@ func (ah *ActivityHandler) handleUpdatedActivity(msg *clientmodels.Message, subs
 		}
 		channelID = channelLink.MattermostChannelID
 	} else {
-		if shouldSync, reason := ah.ShouldSyncDMGMChannel(chat); !shouldSync {
-			return reason
+		if !ah.plugin.GetSyncChats() {
+			return metrics.DiscardedReasonChatsDisabled
 		}
 
 		post, postErr := ah.plugin.GetAPI().GetPost(postInfo.MattermostID)
@@ -481,7 +481,7 @@ func (ah *ActivityHandler) handleUpdatedActivity(msg *clientmodels.Message, subs
 		return metrics.DiscardedReasonInactiveUser
 	}
 
-	post, _, _ := ah.msgToPost(channelID, senderID, msg, chat, ah.plugin.GetSyncFileAttachments(), fileIDs)
+	post, _, _ := ah.msgToPost(channelID, senderID, msg, chat, true, fileIDs)
 	post.Id = postInfo.MattermostID
 
 	// For now, don't display this message on update
@@ -521,10 +521,6 @@ func (ah *ActivityHandler) handleUpdatedActivity(msg *clientmodels.Message, subs
 }
 
 func (ah *ActivityHandler) handleReactions(postID, channelID string, isDirectOrGroupMessage bool, reactions []clientmodels.Reaction) {
-	if !ah.plugin.GetSyncReactions() {
-		return
-	}
-
 	postReactions, appErr := ah.plugin.GetAPI().GetReactions(postID)
 	if appErr != nil {
 		return
@@ -646,14 +642,3 @@ func (ah *ActivityHandler) isRemoteUser(userID string) bool {
 func IsDirectOrGroupMessage(chatID string) bool {
 	return chatID != ""
 }
-
-func (ah *ActivityHandler) ShouldSyncDMGMChannel(chat *clientmodels.Chat) (bool, string) {
-	nb := len(chat.Members)
-	if nb <= 2 && !ah.plugin.GetSyncDirectMessages() {
-		return false, metrics.DiscardedReasonDirectMessagesDisabled
-	} else if nb > 2 && !ah.plugin.GetSyncGroupMessages() {
-		return false, metrics.DiscardedReasonGroupMessagesDisabled
-	}
-
-	return true, ""
-}