Fix possible race condition that can lead to duplicated messages (#627)

* Fix possible race condition that can lead to duplicated messages * Updating tests * Fixing tests * Adding a tests for discarded message on mattermost created message * Fixing tests * Making the message fingerprint something more centralized and changeable in the future * Fixing linter errors * Fixing linter errors * Fixing some tests
mattermost · May 17, 2024 · ae3640f · ae3640f
1 parent 1292481
commit ae3640f
Show file tree

Hide file tree

Showing 8 changed files with 106 additions and 18 deletions.
diff --git a/server/handlers/getters_test.go b/server/handlers/getters_test.go
@@ -67,6 +67,9 @@ func (pm *pluginMock) GetClientForTeamsUser(string) (msteams.Client, error) {
 func (pm *pluginMock) GenerateRandomPassword() string {
  return ""
 }
+func (pm *pluginMock) MessageFingerprint() string {
+ return "<abbr title=\"generated-from-mattermost\"></abbr>"
+}
 func (pm *pluginMock) GetSelectiveSync() bool { return pm.selectiveSync }
 func (pm *pluginMock) ChatShouldSync(channelID string) (bool, error) {
  return true, nil

diff --git a/server/handlers/handlers.go b/server/handlers/handlers.go
@@ -55,6 +55,7 @@ type PluginIface interface {
  GetSelectiveSync() bool
  IsRemoteUser(user *model.User) bool
  GetRemoteID() string
+ MessageFingerprint() string
 }
 
 type ActivityHandler struct {
@@ -273,6 +274,10 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
  return metrics.DiscardedReasonNotUserEvent
  }
 
+ if strings.HasSuffix(msg.Text, ah.plugin.MessageFingerprint()) {
+ return metrics.DiscardedReasonGeneratedFromMattermost
+ }
+
  isDirectOrGroupMessage := IsDirectOrGroupMessage(activityIds.ChatID)
 
  // Avoid possible duplication
@@ -354,6 +359,11 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
 
  post, skippedFileAttachments, errorFound := ah.msgToPost(channelID, senderID, msg, chat, []string{})
 
+ // Last second check to avoid possible duplication
+ if postInfo, _ = ah.plugin.GetStore().GetPostInfoByMSTeamsID(msg.ChatID+msg.ChannelID, msg.ID); postInfo != nil {
+ return metrics.DiscardedReasonDuplicatedPost
+ }
+
  newPost, appErr := ah.plugin.GetAPI().CreatePost(post)
  if appErr != nil {
  ah.plugin.GetAPI().LogWarn("Unable to create post", "error", appErr)

diff --git a/server/handlers/handlers_test.go b/server/handlers/handlers_test.go
@@ -116,6 +116,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetAPI").Return(mockAPI).Maybe()
  p.On("GetStore").Return(store).Maybe()
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
@@ -158,6 +159,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetStore").Return(store).Maybe()
  p.On("GetBotUserID").Return("mock-BotUserID").Times(1)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
@@ -200,6 +202,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetBotUserID").Return("mock-BotUserID").Times(1)
  p.On("GetSyncDirectMessages").Return(true).Times(1)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
@@ -244,6 +247,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetBotUserID").Return("mock-BotUserID").Times(1)
  p.On("GetSyncDirectMessages").Return(true).Times(1)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
@@ -294,6 +298,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetBotUserID").Return("mock-BotUserID").Times(3)
  p.On("GetSyncDirectMessages").Return(true).Times(1)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
@@ -323,7 +328,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  mockAPI.On("CreatePost", testutils.GetPostFromTeamsMessage(mmCreateAtTime)).Return(nil, testutils.GetInternalServerAppError("unable to create the post")).Times(1)
  },
  setupStore: func(store *mocksStore.Store) {
- store.On("GetPostInfoByMSTeamsID", testutils.GetChatID(), testutils.GetMessageID()).Return(nil, nil).Times(1)
+ store.On("GetPostInfoByMSTeamsID", testutils.GetChatID(), testutils.GetMessageID()).Return(nil, nil).Times(2)
  store.On("MattermostToTeamsUserID", "mock-BotUserID").Return(testutils.GetTeamsUserID(), nil).Times(1)
  store.On("TeamsToMattermostUserID", "mockUserID-1").Return("mockUserID-1", nil).Times(1)
  store.On("TeamsToMattermostUserID", "mockUserID-2").Return("mockUserID-2", nil).Times(1)
@@ -345,6 +350,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetBotUserID").Return("mock-BotUserID").Times(3)
  p.On("GetSyncDirectMessages").Return(true).Times(1)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
@@ -378,7 +384,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  store.On("TeamsToMattermostUserID", "mockUserID-1").Return("mockUserID-1", nil).Times(1)
  store.On("TeamsToMattermostUserID", "mockUserID-2").Return("mockUserID-2", nil).Times(1)
  store.On("TeamsToMattermostUserID", testutils.GetSenderID()).Return(testutils.GetUserID(), nil).Times(1)
- store.On("GetPostInfoByMSTeamsID", testutils.GetChatID(), testutils.GetMessageID()).Return(nil, nil).Times(1)
+ store.On("GetPostInfoByMSTeamsID", testutils.GetChatID(), testutils.GetMessageID()).Return(nil, nil).Times(2)
  store.On("LinkPosts", storemodels.PostInfo{
  MattermostID: testutils.GetID(),
  MSTeamsID: testutils.GetMessageID(),
@@ -392,6 +398,49 @@ func TestHandleCreatedActivity(t *testing.T) {
  mockmetrics.On("ObserveMessageDelay", metrics.ActionCreated, metrics.ActionSourceMSTeams, true, mock.AnythingOfType("time.Duration")).Times(1)
  },
  },
+ {
+ description: "Message generated from Mattermost",
+ activityIds: clientmodels.ActivityIds{
+ ChatID: testutils.GetChatID(),
+ MessageID: testutils.GetMessageID(),
+ },
+ setupPlugin: func(p *mocksPlugin.PluginIface, client *mocksClient.Client, mockAPI *plugintest.API, store *mocksStore.Store, mockmetrics *mocksMetrics.Metrics) {
+ p.On("GetClientForApp").Return(client).Maybe()
+ p.On("GetClientForTeamsUser", "mockUserID-1").Return(client, nil).Times(1)
+ p.On("GetAPI").Return(mockAPI).Maybe()
+ p.On("GetStore").Return(store).Maybe()
+ p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
+ },
+ setupClient: func(client *mocksClient.Client) {
+ client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
+ ID: testutils.GetChatID(),
+ Members: []clientmodels.ChatMember{
+ {UserID: "mockUserID-1"},
+ {UserID: "mockUserID-2"},
+ },
+ Type: "D",
+ }, nil).Times(1)
+ client.On("GetChatMessage", testutils.GetChatID(), testutils.GetMessageID()).Return(&clientmodels.Message{
+ ID: testutils.GetMessageID(),
+ UserID: testutils.GetSenderID(),
+ ChatID: testutils.GetChatID(),
+ UserDisplayName: "mockUserDisplayName",
+ Text: "mockText<abbr title=\"generated-from-mattermost\"></abbr>",
+ CreateAt: msteamsCreateAtTime,
+ LastUpdateAt: msteamsCreateAtTime,
+ }, nil).Times(1)
+ },
+ setupAPI: func(mockAPI *plugintest.API) {
+ mockAPI.On("GetDirectChannel", "mockUserID-1", "mockUserID-2").Return(&model.Channel{Id: testutils.GetChannelID()}, nil).Times(1)
+ mockAPI.On("GetUser", testutils.GetUserID()).Return(testutils.GetUser(model.ChannelAdminRoleId, "[email protected]"), nil).Once()
+ mockAPI.On("CreatePost", testutils.GetPostFromTeamsMessage(mmCreateAtTime)).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), mmCreateAtTime), nil).Times(1)
+ },
+ setupStore: func(store *mocksStore.Store) {
+ },
+ setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {
+ },
+ },
  {
  description: "Valid: chat message",
  activityIds: clientmodels.ActivityIds{
@@ -406,6 +455,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetBotUserID").Return("mock-BotUserID").Times(3)
  p.On("GetSyncDirectMessages").Return(true).Times(1)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetChat", testutils.GetChatID()).Return(&clientmodels.Chat{
@@ -439,7 +489,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  store.On("TeamsToMattermostUserID", "mockUserID-1").Return("mockUserID-1", nil).Times(1)
  store.On("TeamsToMattermostUserID", "mockUserID-2").Return("mockUserID-2", nil).Times(1)
  store.On("TeamsToMattermostUserID", testutils.GetSenderID()).Return(testutils.GetUserID(), nil).Times(1)
- store.On("GetPostInfoByMSTeamsID", testutils.GetChatID(), testutils.GetMessageID()).Return(nil, nil).Times(1)
+ store.On("GetPostInfoByMSTeamsID", testutils.GetChatID(), testutils.GetMessageID()).Return(nil, nil).Times(2)
  store.On("LinkPosts", storemodels.PostInfo{
  MattermostID: testutils.GetID(),
  MSTeamsID: testutils.GetMessageID(),
@@ -467,6 +517,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetStore").Return(store).Maybe()
  p.On("GetBotUserID").Return("mock-BotUserID").Times(1)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetMessage", "mockTeamID", testutils.GetChannelID(), testutils.GetMessageID()).Return(&clientmodels.Message{
@@ -506,6 +557,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  p.On("GetStore").Return(store).Maybe()
  p.On("GetBotUserID").Return("mock-BotUserID").Times(3)
  p.On("GetMetrics").Return(mockmetrics).Maybe()
+ p.On("MessageFingerprint").Return("<abbr title=\"generated-from-mattermost\"></abbr>").Maybe()
  },
  setupClient: func(client *mocksClient.Client) {
  client.On("GetMessage", "mockTeamID", testutils.GetChannelID(), testutils.GetMessageID()).Return(&clientmodels.Message{
@@ -531,7 +583,7 @@ func TestHandleCreatedActivity(t *testing.T) {
  Creator: "mockCreator",
  MattermostChannelID: testutils.GetChannelID(),
  }, nil).Times(1)
- store.On("GetPostInfoByMSTeamsID", testutils.GetChannelID(), testutils.GetMessageID()).Return(nil, nil).Times(1)
+ store.On("GetPostInfoByMSTeamsID", testutils.GetChannelID(), testutils.GetMessageID()).Return(nil, nil).Times(2)
  store.On("LinkPosts", storemodels.PostInfo{
  MattermostID: testutils.GetID(),
  MSTeamsID: testutils.GetMessageID(),

diff --git a/server/handlers/mocks/PluginIface.go b/server/handlers/mocks/PluginIface.go
diff --git a/server/message_hooks.go b/server/message_hooks.go
@@ -603,6 +603,8 @@ func (p *Plugin) SendChat(srcUser string, usersIDs []string, post *model.Post, c
  }
  }
 
+ content += p.MessageFingerprint()
+
  newMessage, err := client.SendChat(chat.ID, content, parentMessage, attachments, mentions)
  if err != nil {
  p.API.LogWarn("Error creating post on MS Teams", "error", err.Error())
@@ -689,6 +691,8 @@ func (p *Plugin) Send(teamID, channelID string, user *model.User, post *model.Po
 
  content, mentions := p.getMentionsData(content, teamID, channelID, "", client)
 
+ content += p.MessageFingerprint()
+
  newMessage, err := client.SendMessageWithAttachments(teamID, channelID, parentID, content, attachments, mentions)
  if err != nil {
  p.API.LogWarn("Error creating post on MS Teams", "error", err.Error())