Cherry-pick PR for guest-user-file-upload-permission for release-10.11 (#35073)

Automatic Merge

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

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)

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
+		}
+	}
+}

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 {

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()

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) {

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)
 	}

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")

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."