MM-64226: improved post deduplication (#31004)

Require access to a post before allowing PendingPostId to deduplicate.

Fixes: https://mattermost.atlassian.net/browse/MM-64226

server/channels/api4/post_create_test.go

@@ -0,0 +1,75 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package api4
+
+import (
+	"context"
+	"fmt"
+	"strconv"
+	"testing"
+
+	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/stretchr/testify/require"
+)
+
+func makePendingPostId(user *model.User) string {
+	return fmt.Sprintf("%s:%s", user.Id, strconv.FormatInt(model.GetMillis(), 10))
+}
+
+func TestCreatePostWithPendingPostId(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+	client := th.Client
+
+	t.Run("should successfully create a post with PendingPostId", func(t *testing.T) {
+		pendingPostId := makePendingPostId(th.BasicUser)
+		post := &model.Post{
+			ChannelId:     th.BasicChannel.Id,
+			Message:       "message with pending id " + model.NewId(),
+			PendingPostId: pendingPostId,
+		}
+
+		rpost, resp, err := client.CreatePost(context.Background(), post)
+		require.NoError(t, err)
+		CheckCreatedStatus(t, resp)
+		require.NotNil(t, rpost)
+		require.Equal(t, post.Message, rpost.Message)
+		require.Equal(t, th.BasicUser.Id, rpost.UserId)
+		require.Equal(t, post.ChannelId, rpost.ChannelId)
+		require.Equal(t, pendingPostId, rpost.PendingPostId)
+	})
+
+	t.Run("should not collide with other recent posts not authorized for the user", func(t *testing.T) {
+		// First user creates a post with a PendingPostId
+		pendingPostId := makePendingPostId(th.BasicUser)
+
+		privateChannel := th.CreatePrivateChannel()
+
+		firstPost, resp, err := client.CreatePost(context.Background(), &model.Post{
+			ChannelId:     privateChannel.Id,
+			Message:       "message1",
+			PendingPostId: pendingPostId,
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, resp)
+		require.NotNil(t, firstPost)
+
+		// Second user attempts to create a post with the same PendingPostId
+		client2 := th.CreateClient()
+		_, _, err = client2.Login(context.Background(), th.BasicUser2.Username, th.BasicUser2.Password)
+		require.NoError(t, err)
+
+		secondPost, resp, err := client2.CreatePost(context.Background(), &model.Post{
+			ChannelId:     th.BasicChannel.Id,
+			Message:       "message2",
+			PendingPostId: pendingPostId,
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, resp)
+		require.NotNil(t, secondPost)
+
+		require.NotEqual(t, secondPost.Id, firstPost.Id)
+		require.Equal(t, "message2", secondPost.Message)
+	})
+}

server/channels/app/post.go

@@ -153,7 +153,7 @@ func (a *App) deduplicateCreatePost(rctx request.CTX, post *model.Post) (foundPo
 	}
 
 	if nErr != nil {
-		return nil, model.NewAppError("errorGetPostId", "api.post.error_get_post_id.pending", nil, "", http.StatusInternalServerError).Wrap(nErr)
+		return nil, model.NewAppError("deduplicateCreatePost", "api.post.error_get_post_id.pending", nil, "", http.StatusInternalServerError).Wrap(nErr)
 	}
 
 	// If another thread saved the cache record, but hasn't yet updated it with the actual post
@@ -165,8 +165,11 @@ func (a *App) deduplicateCreatePost(rctx request.CTX, post *model.Post) (foundPo
 
 	// If the other thread finished creating the post, return the created post back to the
 	// client, making the API call feel idempotent.
-	actualPost, err := a.GetSinglePost(rctx, postID, false)
+	actualPost, err := a.GetPostIfAuthorized(rctx, postID, rctx.Session(), false)
-	if err != nil {
+	if err != nil && err.StatusCode == http.StatusForbidden {
+		rctx.Logger().Warn("Ignoring pending_post_id for which the user is unauthorized", mlog.String("pending_post_id", post.PendingPostId), mlog.String("post_id", postID), mlog.Err(err))
+		return nil, nil
+	} else if err != nil {
 		return nil, model.NewAppError("deduplicateCreatePost", "api.post.deduplicate_create_post.failed_to_get", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 

server/channels/app/post_test.go

@@ -27,27 +27,38 @@ import (
 	"github.com/mattermost/mattermost/server/v8/platform/services/searchengine/mocks"
 )
 
+func makePendingPostId(user *model.User) string {
+	return fmt.Sprintf("%s:%s", user.Id, strconv.FormatInt(model.GetMillis(), 10))
+}
+
 func TestCreatePostDeduplicate(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()
 
 	t.Run("duplicate create post is idempotent", func(t *testing.T) {
-		pendingPostId := model.NewId()
+		session := &model.Session{
-		post, err := th.App.CreatePostAsUser(th.Context, &model.Post{
+			UserId: th.BasicUser.Id,
+		}
+		session, err := th.App.CreateSession(th.Context, session)
+		require.Nil(t, err)
+
+		pendingPostId := makePendingPostId(th.BasicUser)
+
+		post, err := th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
 			Message:       "message",
 			PendingPostId: pendingPostId,
-		}, "", true)
+		}, session.Id, true)
 		require.Nil(t, err)
 		require.Equal(t, "message", post.Message)
 
-		duplicatePost, err := th.App.CreatePostAsUser(th.Context, &model.Post{
+		duplicatePost, err := th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
 			Message:       "message",
 			PendingPostId: pendingPostId,
-		}, "", true)
+		}, session.Id, true)
 		require.Nil(t, err)
 		require.Equal(t, post.Id, duplicatePost.Id, "should have returned previously created post id")
 		require.Equal(t, "message", duplicatePost.Message)
@@ -81,23 +92,30 @@ func TestCreatePostDeduplicate(t *testing.T) {
 			}
 		`, `{"id": "testrejectfirstpost", "server": {"executable": "backend.exe"}}`, "testrejectfirstpost", th.App, th.Context)
 
-		pendingPostId := model.NewId()
+		session := &model.Session{
-		post, err := th.App.CreatePostAsUser(th.Context, &model.Post{
+			UserId: th.BasicUser.Id,
+		}
+		session, err := th.App.CreateSession(th.Context, session)
+		require.Nil(t, err)
+
+		pendingPostId := makePendingPostId(th.BasicUser)
+
+		post, err := th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
 			Message:       "message",
 			PendingPostId: pendingPostId,
-		}, "", true)
+		}, session.Id, true)
 		require.NotNil(t, err)
 		require.Equal(t, "Post rejected by plugin. rejected", err.Id)
 		require.Nil(t, post)
 
-		duplicatePost, err := th.App.CreatePostAsUser(th.Context, &model.Post{
+		duplicatePost, err := th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
 			Message:       "message",
 			PendingPostId: pendingPostId,
-		}, "", true)
+		}, session.Id, true)
 		require.Nil(t, err)
 		require.Equal(t, "message", duplicatePost.Message)
 	})
@@ -131,8 +149,14 @@ func TestCreatePostDeduplicate(t *testing.T) {
 			}
 		`, `{"id": "testdelayfirstpost", "server": {"executable": "backend.exe"}}`, "testdelayfirstpost", th.App, th.Context)
 
+		session := &model.Session{
+			UserId: th.BasicUser.Id,
+		}
+		session, err := th.App.CreateSession(th.Context, session)
+		require.Nil(t, err)
+
 		var post *model.Post
-		pendingPostId := model.NewId()
+		pendingPostId := makePendingPostId(th.BasicUser)
 
 		wg := sync.WaitGroup{}
 
@@ -142,12 +166,12 @@ func TestCreatePostDeduplicate(t *testing.T) {
 		go func() {
 			defer wg.Done()
 			var appErr *model.AppError
-			post, appErr = th.App.CreatePostAsUser(th.Context, &model.Post{
+			post, appErr = th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 				UserId:        th.BasicUser.Id,
 				ChannelId:     th.BasicChannel.Id,
 				Message:       "plugin delayed",
 				PendingPostId: pendingPostId,
-			}, "", true)
+			}, session.Id, true)
 			require.Nil(t, appErr)
 			require.Equal(t, post.Message, "plugin delayed")
 		}()
@@ -156,12 +180,12 @@ func TestCreatePostDeduplicate(t *testing.T) {
 		time.Sleep(2 * time.Second)
 
 		// Try creating a duplicate post
-		duplicatePost, err := th.App.CreatePostAsUser(th.Context, &model.Post{
+		duplicatePost, err := th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
 			Message:       "plugin delayed",
 			PendingPostId: pendingPostId,
-		}, "", true)
+		}, session.Id, true)
 		require.NotNil(t, err)
 		require.Equal(t, "api.post.deduplicate_create_post.pending", err.Id)
 		require.Nil(t, duplicatePost)
@@ -171,28 +195,84 @@ func TestCreatePostDeduplicate(t *testing.T) {
 	})
 
 	t.Run("duplicate create post after cache expires is not idempotent", func(t *testing.T) {
-		pendingPostId := model.NewId()
+		session := &model.Session{
-		post, err := th.App.CreatePostAsUser(th.Context, &model.Post{
+			UserId: th.BasicUser.Id,
+		}
+		session, err := th.App.CreateSession(th.Context, session)
+		require.Nil(t, err)
+
+		pendingPostId := makePendingPostId(th.BasicUser)
+
+		post, err := th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
 			Message:       "message",
 			PendingPostId: pendingPostId,
-		}, "", true)
+		}, session.Id, true)
 		require.Nil(t, err)
 		require.Equal(t, "message", post.Message)
 
 		time.Sleep(PendingPostIDsCacheTTL)
 
-		duplicatePost, err := th.App.CreatePostAsUser(th.Context, &model.Post{
+		duplicatePost, err := th.App.CreatePostAsUser(th.Context.WithSession(session), &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
 			Message:       "message",
 			PendingPostId: pendingPostId,
-		}, "", true)
+		}, session.Id, true)
 		require.Nil(t, err)
 		require.NotEqual(t, post.Id, duplicatePost.Id, "should have created new post id")
 		require.Equal(t, "message", duplicatePost.Message)
 	})
+
+	t.Run("Permissison to post required to resolve from pending post cache", func(t *testing.T) {
+		sessionBasicUser := &model.Session{
+			UserId: th.BasicUser.Id,
+		}
+		sessionBasicUser, err := th.App.CreateSession(th.Context, sessionBasicUser)
+		require.Nil(t, err)
+
+		sessionBasicUser2 := &model.Session{
+			UserId: th.BasicUser2.Id,
+		}
+		sessionBasicUser2, err = th.App.CreateSession(th.Context, sessionBasicUser2)
+		require.Nil(t, err)
+
+		pendingPostId := makePendingPostId(th.BasicUser)
+
+		privateChannel := th.CreatePrivateChannel(th.Context, th.BasicTeam)
+		th.AddUserToChannel(th.BasicUser, privateChannel)
+
+		post, err := th.App.CreatePostAsUser(th.Context.WithSession(sessionBasicUser), &model.Post{
+			UserId:        th.BasicUser.Id,
+			ChannelId:     privateChannel.Id,
+			Message:       "message",
+			PendingPostId: pendingPostId,
+		}, sessionBasicUser.Id, true)
+		require.Nil(t, err)
+		require.Equal(t, "message", post.Message)
+
+		postAsDifferentUser, err := th.App.CreatePostAsUser(th.Context.WithSession(sessionBasicUser2), &model.Post{
+			UserId:        th.BasicUser2.Id,
+			ChannelId:     th.BasicChannel.Id,
+			Message:       "message2",
+			PendingPostId: pendingPostId,
+		}, sessionBasicUser2.Id, true)
+		require.Nil(t, err)
+		require.NotEqual(t, post.Id, postAsDifferentUser.Id, "should have created new post id")
+		require.Equal(t, "message2", postAsDifferentUser.Message)
+
+		// Both posts should exist unchanged
+		actualPost, err := th.App.GetSinglePost(th.Context, post.Id, false)
+		require.Nil(t, err)
+		assert.Equal(t, "message", actualPost.Message)
+		assert.Equal(t, privateChannel.Id, actualPost.ChannelId)
+
+		actualPostAsDifferentUser, err := th.App.GetSinglePost(th.Context, postAsDifferentUser.Id, false)
+		require.Nil(t, err)
+		assert.Equal(t, "message2", actualPostAsDifferentUser.Message)
+		assert.Equal(t, th.BasicChannel.Id, actualPostAsDifferentUser.ChannelId)
+	})
 }
 
 func TestAttachFilesToPost(t *testing.T) {
@@ -463,7 +543,7 @@ func TestUpdatePostPluginHooks(t *testing.T) {
 			"testrejectfirstpost", "testupdatepost",
 		}, true, th.App, th.Context)
 
-		pendingPostId := model.NewId()
+		pendingPostId := makePendingPostId(th.BasicUser)
 		post, err := th.App.CreatePostAsUser(th.Context, &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,
@@ -530,7 +610,7 @@ func TestUpdatePostPluginHooks(t *testing.T) {
 			"testaddone", "testaddtwo",
 		}, true, th.App, th.Context)
 
-		pendingPostId := model.NewId()
+		pendingPostId := makePendingPostId(th.BasicUser)
 		post, err := th.App.CreatePostAsUser(th.Context, &model.Post{
 			UserId:        th.BasicUser.Id,
 			ChannelId:     th.BasicChannel.Id,