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 <build@mattermost.com>
This commit is contained in:
Agniva De Sarker 2024-02-29 09:34:55 +05:30 committed by GitHub
parent 9e99280a40
commit c9fc6297f3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 46 additions and 43 deletions

View File

@ -460,7 +460,9 @@ func getFile(c *Context, w http.ResponseWriter, r *http.Request) {
} }
audit.AddEventParameterAuditable(auditRec, "file", info) 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) c.SetPermissionError(model.PermissionReadChannelContent)
return return
} }
@ -492,7 +494,8 @@ func getFileThumbnail(c *Context, w http.ResponseWriter, r *http.Request) {
return 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) c.SetPermissionError(model.PermissionReadChannelContent)
return return
} }
@ -535,7 +538,8 @@ func getFileLink(c *Context, w http.ResponseWriter, r *http.Request) {
} }
audit.AddEventParameterAuditable(auditRec, "file", info) 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) c.SetPermissionError(model.PermissionReadChannelContent)
return return
} }
@ -568,7 +572,8 @@ func getFilePreview(c *Context, w http.ResponseWriter, r *http.Request) {
return 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) c.SetPermissionError(model.PermissionReadChannelContent)
return return
} }
@ -602,7 +607,9 @@ func getFileInfo(c *Context, w http.ResponseWriter, r *http.Request) {
return 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) c.SetPermissionError(model.PermissionReadChannelContent)
return return
} }

View File

@ -774,7 +774,8 @@ func TestGetFile(t *testing.T) {
CheckUnauthorizedStatus(t, resp) CheckUnauthorizedStatus(t, resp)
_, _, err = th.SystemAdminClient.GetFile(context.Background(), fileId) _, _, err = th.SystemAdminClient.GetFile(context.Background(), fileId)
require.NoError(t, err) require.Error(t, err)
CheckUnauthorizedStatus(t, resp)
} }
func TestGetFileHeaders(t *testing.T) { func TestGetFileHeaders(t *testing.T) {
@ -888,7 +889,8 @@ func TestGetFileThumbnail(t *testing.T) {
client.Logout(context.Background()) client.Logout(context.Background())
_, _, err = th.SystemAdminClient.GetFileThumbnail(context.Background(), fileId) _, _, err = th.SystemAdminClient.GetFileThumbnail(context.Background(), fileId)
require.NoError(t, err) require.Error(t, err)
CheckForbiddenStatus(t, resp)
} }
func TestGetFileLink(t *testing.T) { func TestGetFileLink(t *testing.T) {
@ -1000,7 +1002,8 @@ func TestGetFilePreview(t *testing.T) {
client.Logout(context.Background()) client.Logout(context.Background())
_, _, err = th.SystemAdminClient.GetFilePreview(context.Background(), fileId) _, _, err = th.SystemAdminClient.GetFilePreview(context.Background(), fileId)
require.NoError(t, err) require.Error(t, err)
CheckForbiddenStatus(t, resp)
} }
func TestGetFileInfo(t *testing.T) { func TestGetFileInfo(t *testing.T) {
@ -1054,7 +1057,8 @@ func TestGetFileInfo(t *testing.T) {
client.Logout(context.Background()) client.Logout(context.Background())
_, _, err = th.SystemAdminClient.GetFileInfo(context.Background(), fileId) _, _, err = th.SystemAdminClient.GetFileInfo(context.Background(), fileId)
require.NoError(t, err) require.Error(t, err)
CheckForbiddenStatus(t, resp)
} }
func TestGetPublicFile(t *testing.T) { func TestGetPublicFile(t *testing.T) {

View File

@ -564,13 +564,13 @@ func getEditHistoryForPost(c *Context, w http.ResponseWriter, r *http.Request) {
return 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) c.SetPermissionError(model.PermissionEditPost)
return return
} }
originalPost, err := c.App.GetSinglePost(c.Params.PostId, false) if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditPost) {
if err != nil {
c.SetPermissionError(model.PermissionEditPost) c.SetPermissionError(model.PermissionEditPost)
return 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) originalPost, err := c.App.GetSinglePost(c.Params.PostId, false)
if err != nil { if err != nil {
c.SetPermissionError(model.PermissionEditPost) c.SetPermissionError(model.PermissionEditPost)
return return
} }
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditPost) {
c.SetPermissionError(model.PermissionEditPost)
return
}
auditRec.AddEventPriorState(originalPost) auditRec.AddEventPriorState(originalPost)
auditRec.AddEventObjectType("post") auditRec.AddEventObjectType("post")
@ -862,7 +863,7 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) {
post.FileIds = originalPost.FileIds post.FileIds = originalPost.FileIds
if c.AppContext.Session().UserId != originalPost.UserId { 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) c.SetPermissionError(model.PermissionEditOthersPosts)
return return
} }
@ -931,7 +932,7 @@ func patchPost(c *Context, w http.ResponseWriter, r *http.Request) {
permission = model.PermissionEditOthersPosts 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) c.SetPermissionError(permission)
return return
} }
@ -1023,19 +1024,19 @@ func saveIsPinnedPost(c *Context, w http.ResponseWriter, isPinned bool) {
audit.AddEventParameter(auditRec, "post_id", c.Params.PostId) audit.AddEventParameter(auditRec, "post_id", c.Params.PostId)
defer c.LogAuditRecWithLevel(auditRec, app.LevelContent) 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) post, err := c.App.GetSinglePost(c.Params.PostId, false)
if err != nil { if err != nil {
c.Err = err c.SetPermissionError(model.PermissionReadChannelContent)
return return
} }
auditRec.AddEventPriorState(post) auditRec.AddEventPriorState(post)
auditRec.AddEventObjectType("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 := &model.PostPatch{}
patch.IsPinned = model.NewBool(isPinned) patch.IsPinned = model.NewBool(isPinned)

View File

@ -78,17 +78,7 @@ func getReactions(c *Context, w http.ResponseWriter, r *http.Request) {
} }
func deleteReaction(c *Context, w http.ResponseWriter, r *http.Request) { func deleteReaction(c *Context, w http.ResponseWriter, r *http.Request) {
c.RequireUserId() c.RequireUserId().RequirePostId().RequireEmojiName()
if c.Err != nil {
return
}
c.RequirePostId()
if c.Err != nil {
return
}
c.RequireEmojiName()
if c.Err != nil { if c.Err != nil {
return return
} }

View File

@ -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 { 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 channelMember, err := a.Srv().Store().Channel().GetMemberForPost(postID, session.UserId, *a.Config().TeamSettings.ExperimentalViewArchivedChannels); err == nil {
if a.RolesGrantPermission(channelMember.GetRoles(), permission.Id) { if a.RolesGrantPermission(channelMember.GetRoles(), permission.Id) {
return true return true

View File

@ -52,7 +52,8 @@ func (a *App) SaveReactionForPost(c request.CTX, reaction *model.Reaction) (*mod
if channel.DeleteAt > 0 { if channel.DeleteAt > 0 {
return nil, model.NewAppError("SaveReactionForPost", "api.reaction.save.archived_channel.app_error", nil, "", http.StatusForbidden) 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) reaction, nErr := a.Srv().Store().Reaction().Save(reaction)
if nErr != nil { if nErr != nil {
var appErr *model.AppError var appErr *model.AppError
@ -81,9 +82,7 @@ func (a *App) SaveReactionForPost(c request.CTX, reaction *model.Reaction) (*mod
}, plugin.ReactionHasBeenAddedID) }, plugin.ReactionHasBeenAddedID)
}) })
a.Srv().Go(func() { a.sendReactionEvent(model.WebsocketEventReactionAdded, reaction, post)
a.sendReactionEvent(model.WebsocketEventReactionAdded, reaction, post)
})
return reaction, nil return reaction, nil
} }
@ -154,9 +153,7 @@ func (a *App) DeleteReactionForPost(c request.CTX, reaction *model.Reaction) *mo
}, plugin.ReactionHasBeenRemovedID) }, plugin.ReactionHasBeenRemovedID)
}) })
a.Srv().Go(func() { a.sendReactionEvent(model.WebsocketEventReactionRemoved, reaction, post)
a.sendReactionEvent(model.WebsocketEventReactionRemoved, reaction, post)
})
return nil return nil
} }