search

mattermost / mattermost-plugin-msteams

MS Teams plugin for Mattermost
Other
13 stars 11 forks source link

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

Closed jespino closed 2 months ago

jespino commented 2 months ago

This PR follows 2 approaches to avoid possible race conditions on the message sent to MS Teams from mattermost and receiving the "echo" too soon. 1 is, double check if the message is already associated with a mattermost message. Right before creating the new message.

The other strategy applied is to introduce a non-visible html label (an empty abbr lable, with the title generated-from-mattermost that tags the message itself as something generated from mattermost without affect the look and feel of the message in MSTeams side. Then we check if that "tag" exists in the message, and if that is the case, we discard the message as it was generated from mattermost.

jespino commented 2 months ago

@sbishel the reason to use <abbr> is the fact that is common enough to be accepted in MSTeams without sanitizing it, and is uncommon enough to probably not collide with anything else, but at the same time, It doesn't affect the layout because is by default inline, and also support a title attribute that is useful to pass the data that we want.

There is not problem in editions, because editions are not going to be detecting this, it only affects the creations, and for editions, normally we already have the information in the server anyway, so it is not that relevant. Also, creations is overwhelmingly bigger than editions, so this optimization is very relevant.

Also, I was thinking about the posibility of include the "remote-id" or any other "server side code", to be sure that if you have 2 mattermost instances connected to the same microsoft instance, you are replicating properly everything as expected. It is not a use case that we support, but there is not harm on adding that extra data, I guess.

jespino commented 2 months ago

@lieut-data The tests are already verifying that the messages generated are the right ones with the new string, and also I added a new tests for verifying that the message gets discarded after that text is found in the message.