diff --git a/server/channels/api4/post_create_test.go b/server/channels/api4/post_create_test.go new file mode 100644 index 0000000000..d359a4304c --- /dev/null +++ b/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) + }) +} diff --git a/server/channels/app/post.go b/server/channels/app/post.go index b0bb32fc7d..9a925cdd6a 100644 --- a/server/channels/app/post.go +++ b/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) - if err != nil { + actualPost, err := a.GetPostIfAuthorized(rctx, postID, rctx.Session(), false) + 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) } diff --git a/server/channels/app/post_test.go b/server/channels/app/post_test.go index 5f67cb5f6f..be06542753 100644 --- a/server/channels/app/post_test.go +++ b/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() - post, err := th.App.CreatePostAsUser(th.Context, &model.Post{ + session := &model.Session{ + 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() - post, err := th.App.CreatePostAsUser(th.Context, &model.Post{ + session := &model.Session{ + 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() - post, err := th.App.CreatePostAsUser(th.Context, &model.Post{ + session := &model.Session{ + 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,