From c9fc6297f37de5fa53ce0e8c6c02bf93063131e8 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 29 Feb 2024 09:34:55 +0530 Subject: [PATCH] MM-56877: Reduce usages of SessionHasPermissionToChannelByPost (#26239) 1. For file endpoints, a recent optimization added the ChannelID column to be part of the fileinfo table. Therefore, we can skip the postID and directly use the channelID. 2. For post endpoints, we reorder the sequence of calls such that we get the channelID first, and use it to check the permission of the channel rather than query the long way around by joining with the posts table in the permissions query. The benefit is that SessionHasPermissionToChannel is cache-backed. So in the happy path, we save a DB call. Because GetSinglePost anyways needed to be called. And in the bad path, we replace it with a more efficient call. Because SessionHasPermissionToChannel is cache-backed, so effectively we are replacing SessionHasPermissionToChannelByPost with GetSinglePost. 3. And then for the calls that don't have the channelID available, we change the implementation itself to get the channelID by querying the posts table first, and then calling SessionHasPermissionToChannel. This creates the happy path as mentioned earlier. While here, we also do some other optimizations: 4. Pre-populate the channelID while saving the reaction, so that we don't need to query the posts table for every single reaction save. 5. Remove unnecessary goroutine spawning for publishing reaction events, because anyways those are asynchronous. https://mattermost.atlassian.net/browse/MM-56877 ```release-note NONE ``` Co-authored-by: Mattermost Build --- server/channels/api4/file.go | 17 +++++++++----- server/channels/api4/file_test.go | 12 ++++++---- server/channels/api4/post.go | 33 ++++++++++++++-------------- server/channels/api4/reaction.go | 12 +--------- server/channels/app/authorization.go | 4 ++++ server/channels/app/reaction.go | 11 ++++------ 6 files changed, 46 insertions(+), 43 deletions(-) diff --git a/server/channels/api4/file.go b/server/channels/api4/file.go index ee91431e84..514bbf2313 100644 --- a/server/channels/api4/file.go +++ b/server/channels/api4/file.go @@ -460,7 +460,9 @@ func getFile(c *Context, w http.ResponseWriter, r *http.Request) { } audit.AddEventParameterAuditable(auditRec, "file", info) - if info.CreatorId != c.AppContext.Session().UserId && !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), info.PostId, model.PermissionReadChannelContent) { + perm := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), info.ChannelId, model.PermissionReadChannelContent) + + if info.CreatorId != c.AppContext.Session().UserId && !perm { c.SetPermissionError(model.PermissionReadChannelContent) return } @@ -492,7 +494,8 @@ func getFileThumbnail(c *Context, w http.ResponseWriter, r *http.Request) { return } - if info.CreatorId != c.AppContext.Session().UserId && !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), info.PostId, model.PermissionReadChannelContent) { + perm := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), info.ChannelId, model.PermissionReadChannelContent) + if info.CreatorId != c.AppContext.Session().UserId && !perm { c.SetPermissionError(model.PermissionReadChannelContent) return } @@ -535,7 +538,8 @@ func getFileLink(c *Context, w http.ResponseWriter, r *http.Request) { } audit.AddEventParameterAuditable(auditRec, "file", info) - if info.CreatorId != c.AppContext.Session().UserId && !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), info.PostId, model.PermissionReadChannelContent) { + perm := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), info.ChannelId, model.PermissionReadChannelContent) + if info.CreatorId != c.AppContext.Session().UserId && !perm { c.SetPermissionError(model.PermissionReadChannelContent) return } @@ -568,7 +572,8 @@ func getFilePreview(c *Context, w http.ResponseWriter, r *http.Request) { return } - if info.CreatorId != c.AppContext.Session().UserId && !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), info.PostId, model.PermissionReadChannelContent) { + perm := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), info.ChannelId, model.PermissionReadChannelContent) + if info.CreatorId != c.AppContext.Session().UserId && !perm { c.SetPermissionError(model.PermissionReadChannelContent) return } @@ -602,7 +607,9 @@ func getFileInfo(c *Context, w http.ResponseWriter, r *http.Request) { return } - if info.CreatorId != c.AppContext.Session().UserId && !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), info.PostId, model.PermissionReadChannelContent) { + perm := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), info.ChannelId, model.PermissionReadChannelContent) + + if info.CreatorId != c.AppContext.Session().UserId && !perm { c.SetPermissionError(model.PermissionReadChannelContent) return } diff --git a/server/channels/api4/file_test.go b/server/channels/api4/file_test.go index 65d919a3ff..10bd0c91bf 100644 --- a/server/channels/api4/file_test.go +++ b/server/channels/api4/file_test.go @@ -774,7 +774,8 @@ func TestGetFile(t *testing.T) { CheckUnauthorizedStatus(t, resp) _, _, err = th.SystemAdminClient.GetFile(context.Background(), fileId) - require.NoError(t, err) + require.Error(t, err) + CheckUnauthorizedStatus(t, resp) } func TestGetFileHeaders(t *testing.T) { @@ -888,7 +889,8 @@ func TestGetFileThumbnail(t *testing.T) { client.Logout(context.Background()) _, _, err = th.SystemAdminClient.GetFileThumbnail(context.Background(), fileId) - require.NoError(t, err) + require.Error(t, err) + CheckForbiddenStatus(t, resp) } func TestGetFileLink(t *testing.T) { @@ -1000,7 +1002,8 @@ func TestGetFilePreview(t *testing.T) { client.Logout(context.Background()) _, _, err = th.SystemAdminClient.GetFilePreview(context.Background(), fileId) - require.NoError(t, err) + require.Error(t, err) + CheckForbiddenStatus(t, resp) } func TestGetFileInfo(t *testing.T) { @@ -1054,7 +1057,8 @@ func TestGetFileInfo(t *testing.T) { client.Logout(context.Background()) _, _, err = th.SystemAdminClient.GetFileInfo(context.Background(), fileId) - require.NoError(t, err) + require.Error(t, err) + CheckForbiddenStatus(t, resp) } func TestGetPublicFile(t *testing.T) { diff --git a/server/channels/api4/post.go b/server/channels/api4/post.go index 6699aa465e..db5dd065fc 100644 --- a/server/channels/api4/post.go +++ b/server/channels/api4/post.go @@ -564,13 +564,13 @@ func getEditHistoryForPost(c *Context, w http.ResponseWriter, r *http.Request) { return } - if !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), c.Params.PostId, model.PermissionEditPost) { + originalPost, err := c.App.GetSinglePost(c.Params.PostId, false) + if err != nil { c.SetPermissionError(model.PermissionEditPost) return } - originalPost, err := c.App.GetSinglePost(c.Params.PostId, false) - if err != nil { + if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditPost) { c.SetPermissionError(model.PermissionEditPost) return } @@ -845,16 +845,17 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) { } } - if !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), c.Params.PostId, model.PermissionEditPost) { - c.SetPermissionError(model.PermissionEditPost) - return - } - originalPost, err := c.App.GetSinglePost(c.Params.PostId, false) if err != nil { c.SetPermissionError(model.PermissionEditPost) return } + + if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditPost) { + c.SetPermissionError(model.PermissionEditPost) + return + } + auditRec.AddEventPriorState(originalPost) auditRec.AddEventObjectType("post") @@ -862,7 +863,7 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) { post.FileIds = originalPost.FileIds if c.AppContext.Session().UserId != originalPost.UserId { - if !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), c.Params.PostId, model.PermissionEditOthersPosts) { + if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditOthersPosts) { c.SetPermissionError(model.PermissionEditOthersPosts) return } @@ -931,7 +932,7 @@ func patchPost(c *Context, w http.ResponseWriter, r *http.Request) { permission = model.PermissionEditOthersPosts } - if !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), c.Params.PostId, permission) { + if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, permission) { c.SetPermissionError(permission) return } @@ -1023,19 +1024,19 @@ func saveIsPinnedPost(c *Context, w http.ResponseWriter, isPinned bool) { audit.AddEventParameter(auditRec, "post_id", c.Params.PostId) defer c.LogAuditRecWithLevel(auditRec, app.LevelContent) - if !c.App.SessionHasPermissionToChannelByPost(*c.AppContext.Session(), c.Params.PostId, model.PermissionReadChannelContent) { - c.SetPermissionError(model.PermissionReadChannelContent) - return - } - post, err := c.App.GetSinglePost(c.Params.PostId, false) if err != nil { - c.Err = err + c.SetPermissionError(model.PermissionReadChannelContent) return } auditRec.AddEventPriorState(post) auditRec.AddEventObjectType("post") + if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), post.ChannelId, model.PermissionReadChannelContent) { + c.SetPermissionError(model.PermissionReadChannelContent) + return + } + patch := &model.PostPatch{} patch.IsPinned = model.NewBool(isPinned) diff --git a/server/channels/api4/reaction.go b/server/channels/api4/reaction.go index a70a1be743..8fbca84b49 100644 --- a/server/channels/api4/reaction.go +++ b/server/channels/api4/reaction.go @@ -78,17 +78,7 @@ func getReactions(c *Context, w http.ResponseWriter, r *http.Request) { } func deleteReaction(c *Context, w http.ResponseWriter, r *http.Request) { - c.RequireUserId() - if c.Err != nil { - return - } - - c.RequirePostId() - if c.Err != nil { - return - } - - c.RequireEmojiName() + c.RequireUserId().RequirePostId().RequireEmojiName() if c.Err != nil { return } diff --git a/server/channels/app/authorization.go b/server/channels/app/authorization.go index 7fac445d56..63c93e70bf 100644 --- a/server/channels/app/authorization.go +++ b/server/channels/app/authorization.go @@ -171,6 +171,10 @@ func (a *App) SessionHasPermissionToGroup(session model.Session, groupID string, } func (a *App) SessionHasPermissionToChannelByPost(session model.Session, postID string, permission *model.Permission) bool { + if postID == "" { + return false + } + if channelMember, err := a.Srv().Store().Channel().GetMemberForPost(postID, session.UserId, *a.Config().TeamSettings.ExperimentalViewArchivedChannels); err == nil { if a.RolesGrantPermission(channelMember.GetRoles(), permission.Id) { return true diff --git a/server/channels/app/reaction.go b/server/channels/app/reaction.go index 2a46fda2af..fd15140454 100644 --- a/server/channels/app/reaction.go +++ b/server/channels/app/reaction.go @@ -52,7 +52,8 @@ func (a *App) SaveReactionForPost(c request.CTX, reaction *model.Reaction) (*mod if channel.DeleteAt > 0 { return nil, model.NewAppError("SaveReactionForPost", "api.reaction.save.archived_channel.app_error", nil, "", http.StatusForbidden) } - + // Pre-populating the channelID to save a DB call in store. + reaction.ChannelId = post.ChannelId reaction, nErr := a.Srv().Store().Reaction().Save(reaction) if nErr != nil { var appErr *model.AppError @@ -81,9 +82,7 @@ func (a *App) SaveReactionForPost(c request.CTX, reaction *model.Reaction) (*mod }, plugin.ReactionHasBeenAddedID) }) - a.Srv().Go(func() { - a.sendReactionEvent(model.WebsocketEventReactionAdded, reaction, post) - }) + a.sendReactionEvent(model.WebsocketEventReactionAdded, reaction, post) return reaction, nil } @@ -154,9 +153,7 @@ func (a *App) DeleteReactionForPost(c request.CTX, reaction *model.Reaction) *mo }, plugin.ReactionHasBeenRemovedID) }) - a.Srv().Go(func() { - a.sendReactionEvent(model.WebsocketEventReactionRemoved, reaction, post) - }) + a.sendReactionEvent(model.WebsocketEventReactionRemoved, reaction, post) return nil }