diff --git a/server/channels/api4/post.go b/server/channels/api4/post.go index 62e719454c..974e3efe3f 100644 --- a/server/channels/api4/post.go +++ b/server/channels/api4/post.go @@ -62,6 +62,13 @@ func createPostChecks(where string, c *Context, post *model.Post) { return } + if len(post.FileIds) > 0 { + if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), post.ChannelId, model.PermissionUploadFile); !ok { + c.SetPermissionError(model.PermissionUploadFile) + return + } + } + postHardenedModeCheckWithContext(where, c, post.GetProps()) if c.Err != nil { return @@ -1022,6 +1029,12 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) { post.FileIds = originalPost.FileIds } + // Check upload_file permission only if update is adding NEW files (not just keeping existing ones) + checkUploadFilePermissionForNewFiles(c, post.FileIds, originalPost) + if c.Err != nil { + return + } + if c.AppContext.Session().UserId != originalPost.UserId { // We don't need to check the member here, since we already checked it above if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditOthersPosts); !ok { @@ -1090,6 +1103,19 @@ func patchPost(c *Context, w http.ResponseWriter, r *http.Request) { return } + originalPost, err := c.App.GetSinglePost(c.AppContext, c.Params.PostId, false) + if err != nil { + c.SetPermissionError(model.PermissionEditPost) + return + } + + if post.FileIds != nil { + checkUploadFilePermissionForNewFiles(c, *post.FileIds, originalPost) + if c.Err != nil { + return + } + } + patchedPost, isMemberForPReviews, err := c.App.PatchPost(c.AppContext, c.Params.PostId, c.App.PostPatchWithProxyRemovedFromImageURLs(&post), nil) if err != nil { c.Err = err diff --git a/server/channels/api4/post_test.go b/server/channels/api4/post_test.go index 89c33da41b..0989c5042f 100644 --- a/server/channels/api4/post_test.go +++ b/server/channels/api4/post_test.go @@ -277,6 +277,46 @@ func TestCreatePost(t *testing.T) { assert.Nil(t, rpost) }) + t.Run("should prevent creating post with files when user lacks upload_file permission in target channel", func(t *testing.T) { + fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), th.BasicChannel.Id, "test-file.txt") + require.NoError(t, err) + CheckCreatedStatus(t, resp) + fileId := fileResp.FileInfos[0].Id + + th.RemovePermissionFromRole(model.PermissionUploadFile.Id, model.ChannelUserRoleId) + defer func() { + th.AddPermissionToRole(model.PermissionUploadFile.Id, model.ChannelUserRoleId) + }() + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "Test post with file", + FileIds: model.StringArray{fileId}, + } + rpost, resp, err := client.CreatePost(context.Background(), post) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + assert.Nil(t, rpost) + }) + + t.Run("should allow creating post with files when user has upload_file permission", func(t *testing.T) { + fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), th.BasicChannel.Id, "test-file.txt") + require.NoError(t, err) + CheckCreatedStatus(t, resp) + fileId := fileResp.FileInfos[0].Id + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "Test post with file", + FileIds: model.StringArray{fileId}, + } + rpost, resp, err := client.CreatePost(context.Background(), post) + require.NoError(t, err) + CheckCreatedStatus(t, resp) + require.NotNil(t, rpost) + assert.Contains(t, rpost.FileIds, fileId) + }) + t.Run("CreateAt should match the one provided in the request", func(t *testing.T) { post := basicPost() post.CreateAt = 123 @@ -1543,6 +1583,62 @@ func TestUpdatePost(t *testing.T) { CheckBadRequestStatus(t, resp) }) + t.Run("should prevent updating post with files when user lacks upload_file permission in target channel", func(t *testing.T) { + postWithoutFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: th.BasicUser.Id, + ChannelId: channel.Id, + Message: "Post without files", + }, channel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + + fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), channel.Id, "test-file.txt") + require.NoError(t, err) + CheckCreatedStatus(t, resp) + fileId := fileResp.FileInfos[0].Id + + th.RemovePermissionFromRole(model.PermissionUploadFile.Id, model.ChannelUserRoleId) + defer func() { + th.AddPermissionToRole(model.PermissionUploadFile.Id, model.ChannelUserRoleId) + }() + + updatePost := &model.Post{ + Id: postWithoutFiles.Id, + ChannelId: channel.Id, + Message: "Updated post with file", + FileIds: model.StringArray{fileId}, + } + updatedPost, resp, err := client.UpdatePost(context.Background(), postWithoutFiles.Id, updatePost) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + assert.Nil(t, updatedPost) + }) + + t.Run("should allow updating post with files when user has upload_file permission", func(t *testing.T) { + postWithoutFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: th.BasicUser.Id, + ChannelId: channel.Id, + Message: "Post without files", + }, channel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + + fileResp, resp, err := client.UploadFile(context.Background(), []byte("test file data"), channel.Id, "test-file.txt") + require.NoError(t, err) + CheckCreatedStatus(t, resp) + fileId := fileResp.FileInfos[0].Id + + updatePost := &model.Post{ + Id: postWithoutFiles.Id, + ChannelId: channel.Id, + Message: "Updated post with file", + FileIds: model.StringArray{fileId}, + } + updatedPost, resp, err := client.UpdatePost(context.Background(), postWithoutFiles.Id, updatePost) + require.NoError(t, err) + CheckOKStatus(t, resp) + require.NotNil(t, updatedPost) + assert.Contains(t, updatedPost.FileIds, fileId) + }) + t.Run("logged out", func(t *testing.T) { _, err := client.Logout(context.Background()) require.NoError(t, err) diff --git a/server/channels/api4/post_utils.go b/server/channels/api4/post_utils.go index 08d03578a5..edc5ee720d 100644 --- a/server/channels/api4/post_utils.go +++ b/server/channels/api4/post_utils.go @@ -41,3 +41,31 @@ func postPriorityCheckWithContext(where string, c *Context, priority *model.Post c.Err = appErr } } + +// checkUploadFilePermissionForNewFiles checks upload_file permission only when +// adding new files to a post, preventing permission bypass via cross-channel file attachments. +func checkUploadFilePermissionForNewFiles(c *Context, newFileIds []string, originalPost *model.Post) { + if len(newFileIds) == 0 { + return + } + + originalFileIDsMap := make(map[string]bool, len(originalPost.FileIds)) + for _, fileID := range originalPost.FileIds { + originalFileIDsMap[fileID] = true + } + + hasNewFiles := false + for _, fileID := range newFileIds { + if !originalFileIDsMap[fileID] { + hasNewFiles = true + break + } + } + + if hasNewFiles { + if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionUploadFile); !ok { + c.SetPermissionError(model.PermissionUploadFile) + return + } + } +} diff --git a/server/channels/api4/scheduled_post.go b/server/channels/api4/scheduled_post.go index 58dc665abd..fbe686d03e 100644 --- a/server/channels/api4/scheduled_post.go +++ b/server/channels/api4/scheduled_post.go @@ -74,6 +74,14 @@ func createSchedulePost(c *Context, w http.ResponseWriter, r *http.Request) { defer c.LogAuditRecWithLevel(auditRec, app.LevelContent) model.AddEventParameterAuditableToAuditRec(auditRec, "scheduledPost", &scheduledPost) + if len(scheduledPost.FileIds) > 0 { + hasPermission, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), scheduledPost.ChannelId, model.PermissionUploadFile) + if !hasPermission { + c.SetPermissionError(model.PermissionUploadFile) + return + } + } + scheduledPostChecks("Api4.createSchedulePost", c, &scheduledPost) if c.Err != nil { return @@ -169,12 +177,38 @@ func updateScheduledPost(c *Context, w http.ResponseWriter, r *http.Request) { defer c.LogAuditRecWithLevel(auditRec, app.LevelContent) model.AddEventParameterAuditableToAuditRec(auditRec, "scheduledPost", &scheduledPost) + userId := c.AppContext.Session().UserId + existingScheduledPost, err := c.App.Srv().Store().ScheduledPost().Get(scheduledPost.Id) + if err != nil { + c.Err = model.NewAppError("updateScheduledPost", "app.update_scheduled_post.get_scheduled_post.error", nil, "", http.StatusInternalServerError).Wrap(err) + return + } + if existingScheduledPost == nil { + c.Err = model.NewAppError("updateScheduledPost", "app.update_scheduled_post.existing_scheduled_post.not_exist", nil, "", http.StatusNotFound) + return + } + if existingScheduledPost.UserId != userId { + c.Err = model.NewAppError("updateScheduledPost", "app.update_scheduled_post.update_permission.error", nil, "", http.StatusForbidden) + return + } + + if len(scheduledPost.FileIds) > 0 { + originalPost, err := existingScheduledPost.ToPost() + if err != nil { + c.Err = model.NewAppError("updateScheduledPost", "app.update_scheduled_post.convert_to_post.error", nil, "", http.StatusInternalServerError).Wrap(err) + return + } + checkUploadFilePermissionForNewFiles(c, scheduledPost.FileIds, originalPost) + if c.Err != nil { + return + } + } + scheduledPostChecks("Api4.updateScheduledPost", c, &scheduledPost) if c.Err != nil { return } - userId := c.AppContext.Session().UserId updatedScheduledPost, appErr := c.App.UpdateScheduledPost(c.AppContext, userId, &scheduledPost, connectionID) if appErr != nil { c.Err = appErr @@ -209,6 +243,21 @@ func deleteScheduledPost(c *Context, w http.ResponseWriter, r *http.Request) { model.AddEventParameterToAuditRec(auditRec, "scheduledPostId", scheduledPostId) userId := c.AppContext.Session().UserId + + existingScheduledPost, err := c.App.Srv().Store().ScheduledPost().Get(scheduledPostId) + if err != nil { + c.Err = model.NewAppError("deleteScheduledPost", "app.delete_scheduled_post.get_scheduled_post.error", nil, "", http.StatusInternalServerError).Wrap(err) + return + } + if existingScheduledPost == nil { + c.Err = model.NewAppError("deleteScheduledPost", "app.delete_scheduled_post.existing_scheduled_post.not_exist", nil, "", http.StatusNotFound) + return + } + if existingScheduledPost.UserId != userId { + c.Err = model.NewAppError("deleteScheduledPost", "app.delete_scheduled_post.delete_permission.error", nil, "", http.StatusForbidden) + return + } + connectionID := r.Header.Get(model.ConnectionId) deletedScheduledPost, appErr := c.App.DeleteScheduledPost(c.AppContext, userId, scheduledPostId, connectionID) if appErr != nil { diff --git a/server/channels/api4/scheduled_post_test.go b/server/channels/api4/scheduled_post_test.go index 638aa758c7..63d95b4d1a 100644 --- a/server/channels/api4/scheduled_post_test.go +++ b/server/channels/api4/scheduled_post_test.go @@ -11,6 +11,88 @@ import ( "github.com/stretchr/testify/require" ) +func TestUpdateScheduledPost(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + t.Run("should not allow updating a scheduled post not belonging to the user", func(t *testing.T) { + scheduledPost := &model.ScheduledPost{ + Draft: model.Draft{ + CreateAt: model.GetMillis(), + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + Message: "this is a scheduled post", + }, + ScheduledAt: model.GetMillis() + 100000, + } + createdScheduledPost, _, err := th.Client.CreateScheduledPost(context.Background(), scheduledPost) + require.NoError(t, err) + require.NotNil(t, createdScheduledPost) + + originalMessage := createdScheduledPost.Message + originalScheduledAt := createdScheduledPost.ScheduledAt + + createdScheduledPost.ScheduledAt = model.GetMillis() + 9999999 + createdScheduledPost.Message = "Updated Message!!!" + + // Switch to BasicUser2 + th.LoginBasic2() + + _, resp, err := th.Client.UpdateScheduledPost(context.Background(), createdScheduledPost) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + + // Switch back to original user and verify the post wasn't modified + th.LoginBasic() + + fetchedPost, err := th.App.Srv().Store().ScheduledPost().Get(createdScheduledPost.Id) + require.NoError(t, err) + require.NotNil(t, fetchedPost) + require.Equal(t, originalMessage, fetchedPost.Message) + require.Equal(t, originalScheduledAt, fetchedPost.ScheduledAt) + }) +} + +func TestDeleteScheduledPost(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + t.Run("should not allow deleting a scheduled post not belonging to the user", func(t *testing.T) { + scheduledPost := &model.ScheduledPost{ + Draft: model.Draft{ + CreateAt: model.GetMillis(), + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + Message: "this is a scheduled post", + }, + ScheduledAt: model.GetMillis() + 100000, + } + createdScheduledPost, _, err := th.Client.CreateScheduledPost(context.Background(), scheduledPost) + require.NoError(t, err) + require.NotNil(t, createdScheduledPost) + + // Switch to BasicUser2 + th.LoginBasic2() + + _, resp, err := th.Client.DeleteScheduledPost(context.Background(), createdScheduledPost.Id) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + + // Switch back to original user and verify the post wasn't deleted + th.LoginBasic() + + fetchedPost, err := th.App.Srv().Store().ScheduledPost().Get(createdScheduledPost.Id) + require.NoError(t, err) + require.NotNil(t, fetchedPost) + require.Equal(t, createdScheduledPost.Id, fetchedPost.Id) + require.Equal(t, createdScheduledPost.Message, fetchedPost.Message) + }) +} + func TestCreateScheduledPost(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic() diff --git a/server/channels/app/channel_test.go b/server/channels/app/channel_test.go index 2b1bab136a..ead54d327f 100644 --- a/server/channels/app/channel_test.go +++ b/server/channels/app/channel_test.go @@ -725,7 +725,7 @@ func TestAddUserToChannelCreatesChannelMemberHistoryRecord(t *testing.T) { assert.Equal(t, channel.Id, history.ChannelId) channelMemberHistoryUserIds = append(channelMemberHistoryUserIds, history.UserId) } - assert.Equal(t, groupUserIds, channelMemberHistoryUserIds) + assert.ElementsMatch(t, groupUserIds, channelMemberHistoryUserIds) } func TestUsersAndPostsCreateActivityInChannel(t *testing.T) { diff --git a/server/channels/app/scheduled_post.go b/server/channels/app/scheduled_post.go index 65d55b9b30..9b7b9bf97b 100644 --- a/server/channels/app/scheduled_post.go +++ b/server/channels/app/scheduled_post.go @@ -63,7 +63,6 @@ func (a *App) UpdateScheduledPost(rctx request.CTX, userId string, scheduledPost return nil, validationErr } - // validate the scheduled post belongs to the said user existingScheduledPost, err := a.Srv().Store().ScheduledPost().Get(scheduledPost.Id) if err != nil { return nil, model.NewAppError("app.UpdateScheduledPost", "app.update_scheduled_post.get_scheduled_post.error", map[string]any{"user_id": userId, "scheduled_post_id": scheduledPost.Id}, "", http.StatusInternalServerError).Wrap(err) @@ -73,11 +72,6 @@ func (a *App) UpdateScheduledPost(rctx request.CTX, userId string, scheduledPost return nil, model.NewAppError("app.UpdateScheduledPost", "app.update_scheduled_post.existing_scheduled_post.not_exist", map[string]any{"user_id": userId, "scheduled_post_id": scheduledPost.Id}, "", http.StatusNotFound) } - if existingScheduledPost.UserId != userId { - return nil, model.NewAppError("app.UpdateScheduledPost", "app.update_scheduled_post.update_permission.error", map[string]any{"user_id": userId, "scheduled_post_id": scheduledPost.Id}, "", http.StatusForbidden) - } - - // This step is not required for update but is useful as we want to return the // updated scheduled post. It's better to do this before calling update than after. scheduledPost.RestoreNonUpdatableFields(existingScheduledPost) @@ -100,10 +94,6 @@ func (a *App) DeleteScheduledPost(rctx request.CTX, userId, scheduledPostId, con return nil, model.NewAppError("app.DeleteScheduledPost", "app.delete_scheduled_post.existing_scheduled_post.not_exist", map[string]any{"user_id": userId, "scheduled_post_id": scheduledPostId}, "", http.StatusNotFound) } - if scheduledPost.UserId != userId { - return nil, model.NewAppError("app.DeleteScheduledPost", "app.delete_scheduled_post.delete_permission.error", map[string]any{"user_id": userId, "scheduled_post_id": scheduledPostId}, "", http.StatusForbidden) - } - if err := a.Srv().Store().ScheduledPost().PermanentlyDeleteScheduledPosts([]string{scheduledPostId}); err != nil { return nil, model.NewAppError("app.DeleteScheduledPost", "app.delete_scheduled_post.delete_error", map[string]any{"user_id": userId, "scheduled_post_id": scheduledPostId}, "", http.StatusInternalServerError).Wrap(err) } diff --git a/server/channels/app/scheduled_post_test.go b/server/channels/app/scheduled_post_test.go index b5d17c5e64..e4c1814657 100644 --- a/server/channels/app/scheduled_post_test.go +++ b/server/channels/app/scheduled_post_test.go @@ -5,7 +5,6 @@ package app import ( "encoding/json" - "net/http" "testing" "time" @@ -518,54 +517,6 @@ func TestUpdateScheduledPost(t *testing.T) { require.Equal(t, "Updated Message!!!", updatedScheduledPost.Message) }) - t.Run("should ot be allowed to updated a scheduled post not belonging to the user", func(t *testing.T) { - // first we'll create a scheduled post - userId := model.NewId() - - channel, err := th.GetSqlStore().Channel().Save(th.Context, &model.Channel{ - Name: model.NewId(), - DisplayName: "Channel", - Type: model.ChannelTypeOpen, - }, 1000) - require.NoError(t, err) - - _, err = th.GetSqlStore().Channel().SaveMember(th.Context, &model.ChannelMember{ - ChannelId: channel.Id, - UserId: userId, - NotifyProps: model.GetDefaultChannelNotifyProps(), - SchemeGuest: false, - SchemeUser: true, - }) - require.NoError(t, err) - - defer func() { - _ = th.GetSqlStore().Channel().Delete(channel.Id, model.GetMillis()) - _ = th.GetSqlStore().Channel().RemoveMember(th.Context, channel.Id, userId) - }() - - scheduledPost := &model.ScheduledPost{ - Draft: model.Draft{ - CreateAt: model.GetMillis(), - UserId: userId, - ChannelId: channel.Id, - Message: "this is a scheduled post", - }, - ScheduledAt: model.GetMillis() + 100000, // 100 seconds in the future - } - createdScheduledPost, appErr := th.App.SaveScheduledPost(th.Context, scheduledPost, user1ConnID) - require.Nil(t, appErr) - require.NotNil(t, createdScheduledPost) - - // now we'll try updating it - newScheduledAtTime := model.GetMillis() + 9999999 - createdScheduledPost.ScheduledAt = newScheduledAtTime - createdScheduledPost.Message = "Updated Message!!!" - updatedScheduledPost, appErr := th.App.UpdateScheduledPost(th.Context, th.BasicUser2.Id, createdScheduledPost, user1ConnID) - require.NotNil(t, appErr) - require.Equal(t, http.StatusForbidden, appErr.StatusCode) - require.Nil(t, updatedScheduledPost) - }) - t.Run("should only allow updating limited fields", func(t *testing.T) { // first we'll create a scheduled post userId := model.NewId() @@ -716,41 +667,6 @@ func TestDeleteScheduledPost(t *testing.T) { require.Nil(t, reFetchedScheduledPost) }) - t.Run("should not allow deleting someone else's scheduled post", func(t *testing.T) { - // first we'll create a scheduled post - scheduledPost := &model.ScheduledPost{ - Draft: model.Draft{ - CreateAt: model.GetMillis(), - UserId: th.BasicUser.Id, - ChannelId: th.BasicChannel.Id, - Message: "this is a scheduled post", - }, - ScheduledAt: model.GetMillis() + 100000, // 100 seconds in the future - } - createdScheduledPost, appErr := th.App.SaveScheduledPost(th.Context, scheduledPost, user1ConnID) - require.Nil(t, appErr) - require.NotNil(t, createdScheduledPost) - - fetchedScheduledPost, err := th.Server.Store().ScheduledPost().Get(scheduledPost.Id) - require.NoError(t, err) - require.NotNil(t, fetchedScheduledPost) - require.Equal(t, createdScheduledPost.Id, fetchedScheduledPost.Id) - require.Equal(t, createdScheduledPost.Message, fetchedScheduledPost.Message) - - // now we'll delete it - var deletedScheduledPost *model.ScheduledPost - deletedScheduledPost, appErr = th.App.DeleteScheduledPost(th.Context, th.BasicUser2.Id, scheduledPost.Id, "connection_id") - require.NotNil(t, appErr) - require.Nil(t, deletedScheduledPost) - - // try to fetch it again - reFetchedScheduledPost, err := th.Server.Store().ScheduledPost().Get(scheduledPost.Id) - require.NoError(t, err) - require.NotNil(t, reFetchedScheduledPost) - require.Equal(t, createdScheduledPost.Id, reFetchedScheduledPost.Id) - require.Equal(t, createdScheduledPost.Message, reFetchedScheduledPost.Message) - }) - t.Run("should producer error when deleting non existing scheduled post", func(t *testing.T) { var deletedScheduledPost *model.ScheduledPost deletedScheduledPost, appErr := th.App.DeleteScheduledPost(th.Context, th.BasicUser.Id, model.NewId(), "connection_id") diff --git a/server/i18n/en.json b/server/i18n/en.json index 0bbb034d70..96a9e2b2cd 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -7364,6 +7364,10 @@ "id": "app.update_error", "translation": "update error" }, + { + "id": "app.update_scheduled_post.convert_to_post.error", + "translation": "Unable to convert scheduled post to post format." + }, { "id": "app.update_scheduled_post.existing_scheduled_post.not_exist", "translation": "Scheduled post does not exist."